Fix progress bar rendering with double-width unicode characters by yotsuda · Pull Request #26185 · PowerShell/PowerShell

yotsuda · 2025-10-13T14:54:02Z

Summary

Fixes #21293 - Progress bar rendering corruption when StatusDescription contains Japanese or other double-width unicode characters.

Problem

The progress bar's closing bracket ] would appear on the next line and remain visible after completion when using double-wide characters (Japanese, Chinese, Korean, emojis, etc.).

Root cause: The code used string.Length to calculate display widths, which counts characters rather than buffer cells. Double-width characters occupy 2 buffer cells but count as 1 character, causing incorrect width calculations.

Solution

Add rawUI parameter to RenderAnsi method for buffer cell calculations
Calculate display widths using LengthInBufferCells() before adding VT sequences
Use TruncateToBufferCellWidth() for proper text truncation
Calculate VT sequence insertion position character-by-character to respect character boundaries
Fix char to string conversion for LengthInBufferCells calls

Key Technical Details

Why PR #21303 Failed

PR #21303 attempted to fix this issue but was closed due to unresolved technical problems identified in code review:

VT sequence handling: That PR called LengthInBufferCells() on strings that already contained VT escape sequences, leading to incorrect calculations
Incorrect assumptions: The reviewer pointed out that LengthInBufferCells doesn't strip VT sequences when counting cells

This PR's Approach

This PR solves the issue by separating concerns:

Before (incorrect):

sb.Append(PSStyle.Instance.Reverse);  // Add VT sequence first
sb.Append(StatusDescription);
int emptyPadLength = barWidth - rawUI.LengthInBufferCells(sb.ToString()) - secRemainLength;
// ^ This includes VT sequence length incorrectly

After (correct):

// Calculate display width BEFORE adding VT sequences
int statusPartDisplayWidth = rawUI.LengthInBufferCells(StatusDescription);
int emptyPadLength = barWidth - statusPartDisplayWidth - secRemainLength;

// Now add VT sequences
sb.Append(PSStyle.Instance.Reverse);
sb.Append(statusPart);
sb.Append(padding);

Character Boundary Protection

The VT insertion position calculation respects character boundaries:

// Calculate position character-by-character
for (int i = 0; i < statusPart.Length && currentCellCount < barLength; i++)
{
    currentCellCount += rawUI.LengthInBufferCells(statusPart.Substring(i, 1));
    stringPos++;
}
// This prevents splitting double-width characters

Performance Impact

Minimal. The change adds:

One additional LengthInBufferCells call for Activity
Pre-calculation of status display width (moves existing calculation earlier)
Character-by-character loop only when progress bar is drawn (not on every call)

Backward Compatibility

✅ No breaking changes. The fix only affects internal calculations and produces correct output for all character types.

Testing

Comprehensive testing performed with:

✅ Japanese, Chinese (Simplified/Traditional), Korean
✅ Emojis
✅ Thai (complex combining characters)
✅ Arabic (RTL text)
✅ Mixed languages
✅ Long strings (truncation)

All tests passed successfully. The progress bar now renders correctly with any unicode characters.

Test Script

Click to view the comprehensive test script used (VerifyProgressFix.ps1)

Write-Host "========================================" -ForegroundColor Cyan
Write-Host "Progress Bar Test with Modified PowerShell" -ForegroundColor Cyan
Write-Host "Comprehensive Multi-language and Emoji Test" -ForegroundColor Cyan
Write-Host "========================================`n" -ForegroundColor Cyan

# Test 1: English only
Write-Host "Test 1: English only" -ForegroundColor Yellow
for ($i = 1; $i -le 3; $i++) {
    Write-Progress -Activity "English Test" -Status "Step $i of 3" -PercentComplete ($i * 33)
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "English Test" -Completed
Write-Host "✅ English test completed`n" -ForegroundColor Green

# Test 2: Japanese (Issue #21293 reproduction)
Write-Host "Test 2: Japanese (Issue #21293)" -ForegroundColor Yellow
$japaneseStatus = @(
    "初期化中",
    "データ読み込み中",
    "処理実行中",
    "結果を保存中",
    "完了!"
)
for ($i = 0; $i -lt $japaneseStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $japaneseStatus.Count)
    Write-Progress -Activity "日本語テスト" -Status "$($i+1)/$($japaneseStatus.Count) $($japaneseStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "日本語テスト" -Completed
Write-Host "✅ Japanese test completed`n" -ForegroundColor Green

# Test 3: Chinese (Simplified)
Write-Host "Test 3: Chinese (Simplified)" -ForegroundColor Yellow
$chineseStatus = @(
    "初始化",
    "加载数据",
    "处理中",
    "保存结果",
    "完成!"
)
for ($i = 0; $i -lt $chineseStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $chineseStatus.Count)
    Write-Progress -Activity "中文测试" -Status "$($i+1)/$($chineseStatus.Count) $($chineseStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "中文测试" -Completed
Write-Host "✅ Chinese (Simplified) test completed`n" -ForegroundColor Green

# Test 4: Chinese (Traditional)
Write-Host "Test 4: Chinese (Traditional)" -ForegroundColor Yellow
$traditionalChineseStatus = @(
    "初始化",
    "載入資料",
    "處理中",
    "儲存結果",
    "完成!"
)
for ($i = 0; $i -lt $traditionalChineseStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $traditionalChineseStatus.Count)
    Write-Progress -Activity "繁體中文測試" -Status "$($i+1)/$($traditionalChineseStatus.Count) $($traditionalChineseStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "繁體中文測試" -Completed
Write-Host "✅ Chinese (Traditional) test completed`n" -ForegroundColor Green

# Test 5: Korean
Write-Host "Test 5: Korean" -ForegroundColor Yellow
$koreanStatus = @(
    "초기화 중",
    "데이터 로딩 중",
    "처리 중",
    "결과 저장 중",
    "완료!"
)
for ($i = 0; $i -lt $koreanStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $koreanStatus.Count)
    Write-Progress -Activity "한국어 테스트" -Status "$($i+1)/$($koreanStatus.Count) $($koreanStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "한국어 테스트" -Completed
Write-Host "✅ Korean test completed`n" -ForegroundColor Green

# Test 6: Emojis
Write-Host "Test 6: Emojis" -ForegroundColor Yellow
$emojiStatus = @(
    "🚀 Starting up",
    "📂 Loading files",
    "⚙️ Processing data",
    "💾 Saving results",
    "✅ Done!"
)
for ($i = 0; $i -lt $emojiStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $emojiStatus.Count)
    Write-Progress -Activity "🎯 Emoji Test" -Status "$($i+1)/$($emojiStatus.Count) $($emojiStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "🎯 Emoji Test" -Completed
Write-Host "✅ Emoji test completed`n" -ForegroundColor Green

# Test 7: Mixed (English + Japanese + Emojis)
Write-Host "Test 7: Mixed (English + Japanese + Emojis)" -ForegroundColor Yellow
$mixedStatus = @(
    "🔄 Initializing システム初期化",
    "📊 Loading data データ読み込み中",
    "⚡ Processing 処理実行中",
    "💾 Saving 保存中",
    "🎉 Complete 完了!"
)
for ($i = 0; $i -lt $mixedStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $mixedStatus.Count)
    Write-Progress -Activity "🌍 Mixed Language Test" -Status "$($i+1)/$($mixedStatus.Count) $($mixedStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "🌍 Mixed Language Test" -Completed
Write-Host "✅ Mixed language test completed`n" -ForegroundColor Green

# Test 8: Long strings (truncation test)
Write-Host "Test 8: Long strings (truncation test)" -ForegroundColor Yellow
$longStatus = @(
    "これは非常に長いステータスメッセージで、画面幅を超える可能性があります This is a very long status message that may exceed screen width",
    "日本語と英語が混在した長いメッセージでプログレスバーの切り詰め機能をテストします Testing truncation with mixed Japanese and English",
    "🚀🎯⚡💾✅ 絵文字も含めた超長文テストケース with emojis and very long text to test the truncation feature properly"
)
for ($i = 0; $i -lt $longStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $longStatus.Count)
    Write-Progress -Activity "Long Text Test" -Status "$($i+1)/$($longStatus.Count) $($longStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 600
}
Write-Progress -Activity "Long Text Test" -Completed
Write-Host "✅ Long string test completed`n" -ForegroundColor Green

# Test 9: Thai (complex combining characters)
Write-Host "Test 9: Thai (complex combining characters)" -ForegroundColor Yellow
$thaiStatus = @(
    "กำลังเริ่มต้น",
    "กำลังโหลดข้อมูล",
    "กำลังประมวลผล",
    "กำลังบันทึก",
    "เสร็จสิ้น!"
)
for ($i = 0; $i -lt $thaiStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $thaiStatus.Count)
    Write-Progress -Activity "การทดสอบภาษาไทย" -Status "$($i+1)/$($thaiStatus.Count) $($thaiStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "การทดสอบภาษาไทย" -Completed
Write-Host "✅ Thai test completed`n" -ForegroundColor Green

# Test 10: Arabic (RTL text)
Write-Host "Test 10: Arabic (RTL text)" -ForegroundColor Yellow
$arabicStatus = @(
    "التهيئة",
    "تحميل البيانات",
    "المعالجة",
    "حفظ النتائج",
    "اكتمل!"
)
for ($i = 0; $i -lt $arabicStatus.Count; $i++) {
    $percent = [int](($i + 1) * 100 / $arabicStatus.Count)
    Write-Progress -Activity "اختبار اللغة العربية" -Status "$($i+1)/$($arabicStatus.Count) $($arabicStatus[$i])" -PercentComplete $percent
    Start-Sleep -Milliseconds 400
}
Write-Progress -Activity "اختبار اللغة العربية" -Completed
Write-Host "✅ Arabic test completed`n" -ForegroundColor Green

# Final results
Write-Host "========================================" -ForegroundColor Cyan
Write-Host "✅ All tests completed!" -ForegroundColor Green
Write-Host "========================================" -ForegroundColor Cyan
Write-Host ""
Write-Host "Verification points:" -ForegroundColor Yellow
Write-Host "  ✓ Progress bar closing bracket ']' appeared on the same line" -ForegroundColor White
Write-Host "  ✓ All languages and emojis displayed correctly" -ForegroundColor White
Write-Host "  ✓ Long strings were properly truncated" -ForegroundColor White
Write-Host "  ✓ Progress bar disappeared correctly after completion" -ForegroundColor White
Write-Host ""

yotsuda · 2025-10-13T15:21:31Z

@microsoft-github-policy-service agree

iSazonov · 2025-10-14T17:19:59Z

It would be great to have tests. Perhaps we could use our TestHost from test\tools\Modules\HelpersHostCS

yotsuda · 2025-10-15T12:57:26Z

Added Tests and Fixed Padding Bug

Thank you for the feedback! I've added comprehensive tests using TestHost and discovered a bug in the original PR code.

Tests Added

Added test file test/xUnit/csharp/test_ProgressNode.cs with 9 test cases:

✅ Issue21293_ProgressBar_MustNotExceedMaxWidth - Reproduces the original issue
✅ ProgressBar_WithDoubleWidthChars_MustNotExceedMaxWidth - Japanese text
✅ ProgressBar_WithEmoji_MustNotExceedMaxWidth - Emoji characters
✅ ProgressBar_VariousDoubleWidthLengths_MustNotExceedMaxWidth - Theory test with 5 parameter sets
✅ ProgressBar_MixedWidthText_MustNotExceedMaxWidth - Mixed ASCII + CJK
✅ ProgressBar_LongDoubleWidthString_MustNotExceedMaxWidth - Long double-width strings
✅ ProgressBar_Truncation_MustRespectCharacterBoundaries - Truncation scenarios

The tests use TestProgressRawUI that properly:

Strips VT sequences before width calculation
Counts double-width characters (CJK, emoji) as 2 cells
Simulates actual console rendering

Bug Found and Fixed

While running the tests, I discovered the original PR code still had a bug:

Problem: Lines 410 and 494 had incorrect padding logic

// Line 410 - character count based, not cell width based
int padding = maxWidth + PSStyle.Instance.Progress.Style.Length 
                       + PSStyle.Instance.Reverse.Length 
                       + PSStyle.Instance.ReverseOff.Length;

// Line 494 - PadRight() adds extra spaces for double-width characters
.PadRight(padding)

Why it was wrong:

padding calculation mixed cell width (maxWidth) with character count (VT sequence lengths)
PadRight() operates on character count, not cell width
For double-width characters: fewer characters → PadRight adds extra spaces → exceeds maxWidth

Fix: Removed both the padding variable and .PadRight(padding) call

// Line 410 - deleted
// Line 492 - changed from:
                    PSStyle.Instance.Reset)
                .PadRight(padding));

// to:
                    PSStyle.Instance.Reset));

Why this is correct:

emptyPadLength (line 439) already correctly calculates padding using LengthInBufferCells()
Additional PadRight() was unnecessary and harmful
All padding is now consistently cell-width based

Test Results

Version	Tests Passed	Tests Failed
Before PR	0/9	9/9 ❌
Original PR (with padding bug)	2/9	7/9 ⚠️
Fixed PR	9/9 ✅	0/9

All tests now pass, confirming Issue #21293 is completely resolved.

Changes Summary

src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs
- Removed incorrect padding calculation (line 410)
- Removed .PadRight(padding) call (line 494)
test/xUnit/csharp/test_ProgressNode.cs (new file)
- Added 9 comprehensive test cases
- Tests cover Japanese, Chinese, Korean, emoji, and mixed-width text
test/xUnit/xUnit.tests.csproj
- Added Microsoft.PowerShell.ConsoleHost project reference

Status: Tests passing (9/9). Found an additional truncation edge case during manual testing - fixing statusPartDisplayWidth calculation to use actual measured width instead of estimated value. Will update shortly.

iSazonov · 2025-10-15T12:59:48Z

@yotsuda Thank you for your efforts! I'm afraid that these tests are too low-level and they won't work if the implementation changes. It would be great if you used the module that I specified to create tests on PowerShell.
(Without a doubt, we should not directly use int LengthInBufferCells(string str))

yotsuda · 2025-10-15T15:03:06Z

Update: Replaced C# xUnit tests with PowerShell tests

Thank you for the feedback. I have replaced the low-level C# xUnit tests with PowerShell-based tests as requested.

Changes in this update:

Removed C# xUnit tests that directly used LengthInBufferCells()
Added PowerShell tests (Write-Progress.DoubleWidth.Tests.ps1) that test the Write-Progress cmdlet from a user perspective
Fixed the rendering bug by properly calculating ellipsis width and removing unnecessary padding

About the tests:

The new tests verify that Write-Progress handles various character types correctly:

✅ Japanese characters (日本語)
✅ Chinese characters (中文) - Issue The progress bar displayed by WriteProgress() in the console becomes corrupted. #21293 scenario
✅ Korean characters (한국어)
✅ Emoji (🚀)
✅ Mixed ASCII and double-width characters
✅ Very long text requiring truncation
✅ Multiple progress operations with parent-child relationship

Technical limitation:

I wanted to clarify a technical constraint: It is not possible to verify the actual rendering width (that the progress bar does not exceed maxWidth) from PowerShell tests.

Here's why:

Write-Progress renders directly to the console and does not output to stdout/stderr
The PowerShell TestHost framework does not capture the actual rendered output
The existing Write-Progress.Tests.ps1 also does not test rendering width - it only tests that the cmdlet does not throw exceptions

What the current tests verify:

The cmdlet does not crash with various character types
ProgressRecord objects are created correctly
Various edge cases (very long text, parent-child relationships, etc.) work without errors

What cannot be verified at PowerShell level:

That the rendered progress bar does not exceed maxWidth
The actual visual appearance of the progress bar

Manual verification:

I have manually verified the fix works correctly by:

Building PowerShell with the fix
Running Write-Progress with Japanese text (Issue The progress bar displayed by WriteProgress() in the console becomes corrupted. #21293 scenario)
Confirming visually that the progress bar no longer exceeds the window width

I believe this approach aligns with your guidance to use PowerShell-level tests rather than implementation-specific C# tests. However, if there is a way to verify the rendering width from PowerShell that I'm not aware of, I'd be happy to add those tests.

Let me know if you have any suggestions or if this approach is acceptable.

iSazonov · 2025-10-16T08:09:58Z

@yotsuda Please don't rebase until maintainers ask because this kills commit history and force them to re-review.

yotsuda · 2025-10-16T09:33:12Z

@iSazonov Thanks for letting me know.
I understand — I won’t rebase or force-push unless maintainers ask.

iSazonov · 2025-10-16T11:01:33Z

/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging

azure-pipelines · 2025-10-16T11:01:42Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

Copilot

Pull Request Overview

This PR fixes a rendering corruption issue (#21293) where progress bars display incorrectly when StatusDescription contains double-width Unicode characters (Japanese, Chinese, Korean, emojis). The root cause was using string.Length for width calculations, which counts characters rather than buffer cells—double-width characters occupy 2 buffer cells but count as 1 character.

Key changes:

Modified RenderAnsi to accept rawUI parameter for accurate buffer cell width calculations
Replaced string.Length with LengthInBufferCells() and TruncateToBufferCellWidth() for proper display width handling
Added character-by-character VT sequence insertion position calculation to respect character boundaries

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
test/powershell/Modules/Microsoft.PowerShell.Utility/Write-Progress.DoubleWidth.Tests.ps1	Adds comprehensive test coverage for double-width characters in progress bars across multiple languages and scenarios
src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs	Fixes progress bar rendering by using buffer cell width calculations instead of character counts for Unicode text

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

iSazonov · 2025-10-22T10:26:09Z

@yotsuda Please look Copilot comments and rebase to get latest build workflows.

Fixes PowerShell#21293 The progress bar was corrupted when StatusDescription contained Japanese or other double-width unicode characters. The closing bracket would appear on the next line and remain after completion. Root cause: The code used string.Length to calculate display widths, which counts characters rather than buffer cells. Double-width characters (CJK, emojis) occupy 2 buffer cells but count as 1 character. Changes: - Add rawUI parameter to RenderAnsi method for buffer cell calculations - Calculate display widths before adding VT sequences - Use LengthInBufferCells() for Activity and StatusDescription - Use TruncateToBufferCellWidth() for proper truncation - Calculate VT insertion position character-by-character to respect character boundaries - Fix char to string conversion for LengthInBufferCells calls Tested with: - Japanese, Chinese (Simplified/Traditional), Korean - Emojis - Thai (complex combining characters) - Arabic (RTL text) - Mixed languages - Long strings (truncation) All tests passed successfully. The progress bar now renders correctly with any unicode characters.

Fixes PowerShell#21293 This commit addresses an issue where progress bars containing double-width Unicode characters (such as Japanese, Chinese, Korean, and emoji) would exceed the terminal width, causing rendering issues. Changes: - Remove incorrect padding calculation that used character count - Properly calculate ellipsis width using LengthInBufferCells() - Remove unnecessary PadRight() call that caused width overflow - All width calculations now consistently use buffer cell width The fix ensures that the progress bar never exceeds maxWidth by: 1. Calculating the actual display width of the ellipsis 2. Removing the character-count-based padding variable 3. Relying on the correct emptyPadLength calculation that already uses LengthInBufferCells() Tests: - Add PowerShell-based test suite (Write-Progress.DoubleWidth.Tests.ps1) - 11 test cases covering various scenarios: - Japanese, Chinese, Korean characters - Emoji and mixed-width text - Very long text requiring truncation - Parent-child progress relationships - All tests verify the cmdlet handles double-width characters without crashing Note: The tests verify functional correctness but cannot verify the actual rendering width at PowerShell test level, as Write-Progress renders directly to the console and does not output to stdout/stderr.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…gress.DoubleWidth.Tests.ps1 Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

yotsuda · 2025-10-24T08:29:50Z

Summary

Reviewed and incorporated Copilot's suggestions. Rebase completed.

Changes

Applied both fixes (accurate width calculation and performance optimization). Details in Copilot's comments above.

Verification

Built and tested with Japanese, Chinese, Korean text and emojis. Issue #21293 remains fixed.

Status

Ready for re-review. Apologies for the oversight.

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

yotsuda · 2025-10-26T11:41:23Z

@iSazonov Thank you for the feedback!

I believe the test_ProgressNode.cs that I removed based on your feedback was actually doing what you're suggesting - testing the rendering output (ANSI sequences), not just the Progress stream data.

That test:

Called ProgressNode.Render() to get the actual ANSI output string
Stripped VT sequences from the rendered output
Verified that the final rendered width doesn't exceed maxWidth in buffer cells

Regarding alternative testing approaches:
I previously explained the technical limitations in this comment. In summary:

PowerShell-level tests cannot capture the actual rendered output (Write-Progress renders directly to console, not to stdout/stderr)
C# unit tests can verify rendering, but require testing ProgressNode.Render() directly

My question:
If testing ProgressNode.Render() directly is too low-level, could you suggest an alternative approach for verifying that the rendered progress bar respects maxWidth? The existing Write-Progress.Tests.ps1 only verifies that the cmdlet doesn't crash - it cannot verify the rendering behavior that this PR fixes.

iSazonov · 2025-10-27T05:19:29Z

@yotsuda PowerShell uses many overloaded layers for outputting to host terminal. Unit tests skip the layers and does not guaranty that user see on host terminal. That's why we need tests on PowerShell itself, not on C#.
(As I mentioned, historically we have limited support for full Unicode with many workarounds. And your change is another step in that direction - many thanks!)

To enable VT sequences in the test host there is SupportsVirtualTerminal property, I think we need to set it to true.

$host = New-TestHost
$host.UI

RawUI                   : TestHost.TestHostRawUI
SupportsVirtualTerminal : False
ReadLineData            : This is readline data
PromptedChoice          : 0
StringForSecureString   : TEST
UserNameForCredential   : Admin
promptResponse          : this is a prompt response
Streams                 : TestHost.Streams

yotsuda · 2025-10-27T11:26:31Z

@iSazonov I investigated the test framework and found that the C# code in HelpersHostCS.psm1 has a mock WriteProgress that does not call ProgressNode.RenderAnsi:

public override void WriteProgress(long sourceId, ProgressRecord record)
{
    Streams.Progress.Add(record);  // Only stores the record, no rendering
}

Therefore, it seems impossible to write an appropriate test for this PR using .ps1 tests.

I believe we need either:

Allow C# unit test, or
Rewrite the test framework to call the actual ConsoleHost implementation

However, rewriting the test framework appears technically challenging. Even if possible, I lack the knowledge to assess the impact on other tests, so this work would need to be done by someone from the PowerShell team.

What approach would you recommend?

iSazonov · 2025-10-27T17:30:46Z

I looked at commit history and there really aren't any rendering tests. Writing reliable tests requires improving the test module, which is beyond the scope of this change. (Although you can do it if you want.) So let's ditch script tests in favor of xUnit ones.

…Shell#21293) Replace Pester tests with comprehensive xUnit tests covering: - 10 test scenarios with 144 total test cases - Multiple window widths including odd (25, 73, 93, 155) and irregular sizes - Double-width characters (Japanese, Chinese, Korean, emoji) - Edge cases: truncation, surrogate pairs, mixed-width text

…mance

iSazonov · 2025-10-28T12:03:40Z

+        /// - Double-width characters (CJK, emoji) which occupy 2 cells
+        /// - Regular characters which occupy 1 cell
+        /// </summary>
+        public override int LengthInBufferCells(string str)


Tests have access to internal methods so no need to implement the method.

yotsuda · 2025-10-30T00:36:09Z

Update: Code Review Feedback Addressed

Thank you @iSazonov for the review feedback. I've made the following improvements:

Main Changes (commits `463cd77`..`60f69c1`)

1. Simplified Test Implementation (35d2336)

Replaced custom LengthInBufferCells implementation in TestProgressRawUI with direct calls to ConsoleControl.LengthInBufferCells
Removed 89 lines of redundant width calculation logic
Eliminated StripVTSequences helper method from tests
Tests now use the same internal method as the production code, ensuring consistency

2. Code Quality Improvements

96eb58f: Removed duplicate assignment in ternary operator
2b8c054: Fixed CodeFactor style issues in test file
60f69c1: Reordered classes (public before internal) for better code organization

3. Refactoring (5a77b9a)

Eliminated redundant width calculations in RenderAnsi method
Improved code readability and maintainability

4. Documentation (0d65578, 2c6230a)

Added XML documentation for test method parameters (maxWidth, statusText)

Summary

Net change: -39 lines (189 deletions, 150 additions)
Files modified: ProgressNode.cs, test_ProgressNode.cs
All existing tests continue to pass with the simplified implementation

The PR is now ready for another review. Please let me know if there are any other concerns.

iSazonov

Only style comments at the time.

iSazonov · 2025-10-30T11:27:59Z

+        [InlineData(20)]
+        [InlineData(25)]
+        [InlineData(37)]
+        [InlineData(40)]
+        [InlineData(60)]
+        [InlineData(73)]
+        [InlineData(80)]
+        [InlineData(93)]
+        [InlineData(100)]
+        [InlineData(120)]
+        [InlineData(155)]
+        [InlineData(200)]


Do we really need so many tests?
I'd check edge cases and and scenarios when the size of the screen width is -1, 0, +1 of the required?

iSazonov · 2025-10-30T11:29:19Z

+        public override ConsoleColor ForegroundColor { get; set; }
+
+        public override ConsoleColor BackgroundColor { get; set; }
+
+        public override Coordinates CursorPosition { get; set; }


I'd remove blank all lines to compress the helper.

iSazonov · 2025-10-30T11:30:15Z

+        public override void SetBufferContents(Rectangle rectangle, BufferCell fill)
+        {
+            throw new NotImplementedException();
+        }


To compress the helper I suggest follow style:

Suggested change

public override void SetBufferContents(Rectangle rectangle, BufferCell fill)

{

throw new NotImplementedException();

}

public override void SetBufferContents(Rectangle rectangle, BufferCell fill) => throw new NotImplementedException();

iSazonov · 2025-10-30T11:32:22Z

            // if the activity is really long, only use up to half the width
            string activity;
-            if (Activity.Length > maxWidth / 2)
+            int activityDisplayWidth = rawUI.LengthInBufferCells(Activity);


Maybe rename to activityDisplayCellsWidth

iSazonov · 2025-10-30T11:38:50Z

+            int emptyPadLength = barWidth - statusPartDisplayWidth - secRemainLength;
            if (emptyPadLength > 0)
            {
                sb.Append(string.Empty.PadRight(emptyPadLength));


nit: Append(char value, int repeatCount)

Changes in response to maintainer feedback on PR PowerShell#26185: ProgressNode.cs: - Rename activityDisplayWidth to activityDisplayCellsWidth for clarity - Use StringBuilder.Append(char, int) for better performance test_ProgressNode.cs: - Reduce test cases to edge cases around standard terminal width (79, 80, 81) - Simplify TestProgressRawUI with expression-bodied members Test results: 36/36 pass Before fix: 33/36 failed (91.7% failure rate) After fix: 36/36 pass (100% success rate)

yotsuda · 2025-11-02T01:51:11Z

Code Review Feedback Addressed

Thank you @iSazonov for the detailed review feedback. I've addressed your style comments in commit e0d89df.

Changes Made

ProgressNode.cs:

✅ Renamed activityDisplayWidth to activityDisplayCellsWidth for clarity (line 395)
✅ Changed sb.Append(string.Empty.PadRight(emptyPadLength)) to sb.Append(' ', emptyPadLength) for better performance (line 444)

test_ProgressNode.cs:

✅ Reduced test cases from 156 to 36 (77% reduction) by focusing on edge cases around standard terminal width (79, 80, 81 columns)
✅ Simplified TestProgressRawUI helper class using expression-bodied members
⚠️ Regarding blank lines removal: I attempted to remove blank lines between elements in TestProgressRawUI as suggested, but this caused StyleCop SA1516 errors ("Elements should be separated by blank line"). The current implementation uses expression-bodied members to compress methods while maintaining blank lines to comply with the project's StyleCop rules.

Test Results

Before fix:

33/36 tests failed (91.7% failure rate)
Double-width characters caused progress bars to exceed maxWidth

After fix:

✅ 36/36 tests pass (100% success rate)
All double-width character scenarios now render correctly within maxWidth

Verification

Built and tested locally:

All xUnit tests pass
StyleCop violations resolved
No regressions introduced

Ready for re-review. Please let me know if any other changes are needed.

yotsuda · 2026-04-11T22:58:13Z

Friendly ping — this PR has been waiting for review for over 5 months. All feedback has been addressed (final commits on 2025-11-02, commit e0d89df), and only minor style comments were outstanding at the last review round, which have all been resolved.

@SteveL-MSFT @iSazonov would you have time to take another look? The fix resolves #21293 (progress bar corruption with double-width characters) and is covered by the xUnit tests added in this PR. I'd really appreciate getting this merged. Thank you!

jshigetomi · 2026-06-22T21:49:57Z

@yotsuda Thanks for taking this on! Could you address the CodeFactor issues and the test failures?

The xUnit build was failing with CA1852 (type can be sealed). Seal the TestProgressRawUI helper and move it into its own file, which also resolves CodeFactor's 'single type per file' on test_ProgressNode.cs. Remove the now-unused usings from test_ProgressNode.cs.

yotsuda · 2026-06-23T03:49:12Z

Thanks @jshigetomi! Pushed a fix.

Test failures: the xUnit build was failing on CA1852 — I sealed the TestProgressRawUI test helper and moved it to its own file (which also clears CodeFactor's "single type per file"). Locally on net11.0 the test project builds and all ProgressNode tests pass.
The two remaining CodeFactor items are pre-existing, not introduced by this PR: "file name should match first type name" is the repo-wide test_*.cs convention, and the "Complex Method" on RenderAnsi is existing complexity in production code — I left ProgressNode.cs untouched so this doesn't re-open the reviewed change. Happy to take that on if you'd like.

Since this is a fork PR, the workflow runs are waiting on approval — could you approve them so CI can verify?

jshigetomi · 2026-06-23T20:48:46Z

@yotsuda Tests seem to be passing. Just a couple more CodeFactor issues left.

Reduce the (pre-existing) cyclomatic complexity of RenderAnsi flagged by CodeFactor by extracting the status-truncation and ReverseOff-insertion logic into helpers. Behavior is unchanged and covered by the existing ProgressNode tests.

yotsuda · 2026-06-24T02:16:35Z

Pushed the RenderAnsi refactor — CodeFactor's "Complex Method" is now resolved (2 → 1).

The one remaining CodeFactor item is "file name should match first type name" on test_ProgressNode.cs. That's just our repo-wide test_*.cs test-file convention (every file under test/xUnit/csharp/ is named that way), so I've left it as-is for consistency. Happy to rename it to ProgressNodeTests.cs if you'd prefer, but that would make it the odd one out.

jshigetomi · 2026-06-24T21:33:35Z

LGTM

yotsuda requested review from a team, SeeminglyScience, TravisEz13 and jshigetomi as code owners October 13, 2025 14:54

yotsuda force-pushed the fix/progress-bar-unicode branch from 40387ce to b837291 Compare October 13, 2025 15:00

yotsuda mentioned this pull request Oct 13, 2025

The progress bar displayed by WriteProgress() in the console becomes corrupted. #21293

Closed

5 tasks

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 14, 2025

yotsuda force-pushed the fix/progress-bar-unicode branch from 818382e to b837291 Compare October 14, 2025 23:16

yotsuda force-pushed the fix/progress-bar-unicode branch from a5227d3 to 382d0e4 Compare October 15, 2025 14:52

iSazonov requested a review from Copilot October 22, 2025 10:19

Copilot AI reviewed Oct 22, 2025

View reviewed changes

Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs Outdated

Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs Outdated

Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Write-Progress.DoubleWidth.Tests.ps1 Outdated

yotsuda and others added 5 commits October 24, 2025 15:36

Update src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

9a571e8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update test/powershell/Modules/Microsoft.PowerShell.Utility/Write-Pro…

f7d1397

…gress.DoubleWidth.Tests.ps1 Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/Microsoft.PowerShell.ConsoleHost/host/msh/ProgressNode.cs

281a5cb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

yotsuda force-pushed the fix/progress-bar-unicode branch from 8c9c630 to 281a5cb Compare October 24, 2025 06:37

iSazonov requested a review from Copilot October 24, 2025 11:25

Copilot AI reviewed Oct 24, 2025

View reviewed changes

Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Write-Progress.DoubleWidth.Tests.ps1 Outdated

yotsuda added 2 commits October 28, 2025 20:34

Fix CodeFactor warnings: remove unused code and optimize Regex perfor…

d65f013

…mance

iSazonov reviewed Oct 28, 2025

View reviewed changes

yotsuda added 7 commits October 28, 2025 21:44

Use ConsoleControl.LengthInBufferCells directly in tests

35d2336

Fix CodeFactor issue: remove duplicate assignment in ternary operator

96eb58f

Refactor: eliminate redundant calculations in RenderAnsi method

5a77b9a

Fix CodeFactor style issues in test_ProgressNode.cs

2b8c054

Add XML documentation for maxWidth parameter in tests

0d65578

Add XML documentation for statusText parameter in tests

2c6230a

Reorder classes to place public before internal

60f69c1

iSazonov reviewed Oct 30, 2025

View reviewed changes

microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Nov 9, 2025

Merge branch 'master' into fix/progress-bar-unicode

82eb674

jshigetomi approved these changes Jun 24, 2026

View reviewed changes

jshigetomi merged commit 7d596de into PowerShell:master Jun 26, 2026
43 of 46 checks passed

Uh oh!

Conversation

yotsuda commented Oct 13, 2025

Summary

Problem

Solution

Key Technical Details

Why PR #21303 Failed

This PR's Approach

Character Boundary Protection

Performance Impact

Backward Compatibility

Testing

Related

Test Script

Uh oh!

yotsuda commented Oct 13, 2025

Uh oh!

iSazonov commented Oct 14, 2025

Uh oh!

yotsuda commented Oct 15, 2025

Added Tests and Fixed Padding Bug

Tests Added

Bug Found and Fixed

Test Results

Changes Summary

Uh oh!

iSazonov commented Oct 15, 2025

Uh oh!

yotsuda commented Oct 15, 2025

Update: Replaced C# xUnit tests with PowerShell tests

Changes in this update:

About the tests:

Technical limitation:

Manual verification:

Uh oh!

iSazonov commented Oct 16, 2025

Uh oh!

yotsuda commented Oct 16, 2025

Uh oh!

iSazonov commented Oct 16, 2025

Uh oh!

azure-pipelines Bot commented Oct 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov commented Oct 22, 2025

Uh oh!

yotsuda commented Oct 24, 2025

Summary

Changes

Verification

Status

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

yotsuda commented Oct 26, 2025

Uh oh!

iSazonov commented Oct 27, 2025

Uh oh!

yotsuda commented Oct 27, 2025

Uh oh!

iSazonov commented Oct 27, 2025

Uh oh!

iSazonov Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yotsuda commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update: Code Review Feedback Addressed

iSazonov Oct 28, 2025 •

edited

Loading

yotsuda commented Oct 30, 2025 •

edited

Loading

Main Changes (commits `463cd77`..`60f69c1`)