fix: progress display alignment and SYSLIB0060#29
Merged
Conversation
Sync all three delta specs for the progress-display-redesign change: - progress-display: replace FileState enum (adds Hashed, removes QueuedInTar/UploadingTar/TarId), add TrackedTar entity, update handlers table and aggregate counters for new scanning/TAR events - archive-pipeline: add per-file FileScannedEvent + ScanCompleteEvent, TarBundleStartedEvent notification, TAR upload ProgressStream wiring, OnHashQueueReady/OnUploadQueueReady callbacks on ArchiveOptions, replace TarBundleSealingEvent-with-content-hashes with new event model - cli: replace archive display layout with 3-section design (scanning header, hashing with queue depth/dedup, uploading with queue depth, TAR bundle lines), update progress callback wiring for dual TrackedFile/TrackedTar lookup, update streaming events list
…veBytes.Pbkdf2 (SYSLIB0060)
Replace manual PadLeft string interpolation in BuildArchiveDisplay with a borderless 7-column Spectre Table (name | bar | state | % | current | / | total+unit). Add SplitSizePair helper that returns (current, total, unit) strings sharing the unit of the total so each column can be independently aligned. Remove FormatSizePair (superseded by SplitSizePair).
SplitSizePair now formats values with CultureInfo.InvariantCulture so decimal separators are always '.' regardless of system locale. All percentage displays are clamped to 100 before formatting to prevent >100% showing when uploaded bytes slightly exceed the recorded TAR size.
Spectre NoBorder table does not pad cells, so right-aligning current/total in separate columns had no effect. Fix: collect all row data first, compute max widths for cur, tot, pct, and stateLabel, then pad with PadLeft/PadRight before inserting into a single pre-formatted size column.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #29 +/- ##
==========================================
+ Coverage 82.04% 82.12% +0.07%
==========================================
Files 27 27
Lines 3002 3026 +24
Branches 354 360 +6
==========================================
+ Hits 2463 2485 +22
Misses 457 457
- Partials 82 84 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughThe PR updates archive pipeline event semantics to emit per-file scan events and new tar bundle lifecycle events, adds queue-depth callbacks to ArchiveOptions, refactors progress state to track TAR bundles as separate entities from files, updates CLI progress rendering for the new model, and simplifies PBKDF2 key derivation using a single static call. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/specs/progress-display/spec.md (1)
247-250:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code block.
The code block showing
RestoreFileEventrecord definition is missing a language specifier.Proposed fix
**`RestoreFileEvent` record** (internal): -``` +```csharp RestoreFileEvent(string RelativePath, long FileSize, bool Skipped)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@openspec/specs/progress-display/spec.mdaround lines 247 - 250, The fenced
code block for the RestoreFileEvent record is missing a language specifier;
update the markdown where the RestoreFileEvent(string RelativePath, long
FileSize, bool Skipped) record is shown to use a fenced code block with the
"csharp" language (i.e., change the opening triple backticks to ```csharp) so
syntax highlighting and tooling recognize the C# record definition.</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@src/Arius.Cli/CliBuilder.cs`: - Around line 1026-1036: SplitSizePair can produce NaN when totalInfo.LargestWholeNumberValue is 0; change the divisor and currentVal calculations to guard against a zero largest-whole-number value by checking totalInfo.LargestWholeNumberValue > 0 before computing divisor = (double)total / totalInfo.LargestWholeNumberValue, otherwise set divisor = 1.0, then compute currentVal = divisor > 0 ? current / divisor : 0.0 and keep totalVal = totalInfo.LargestWholeNumberValue (which may be 0) so formatting remains safe; update only the computations in SplitSizePair (variables totalInfo.LargestWholeNumberValue, divisor, currentVal, totalVal) to implement this guard. --- Outside diff comments: In `@openspec/specs/progress-display/spec.md`: - Around line 247-250: The fenced code block for the RestoreFileEvent record is missing a language specifier; update the markdown where the RestoreFileEvent(string RelativePath, long FileSize, bool Skipped) record is shown to use a fenced code block with the "csharp" language (i.e., change the opening triple backticks to ```csharp) so syntax highlighting and tooling recognize the C# record definition.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID:
fc2be98a-c69a-446d-af17-c19b1c0cb8d4📒 Files selected for processing (12)
openspec/changes/archive/2026-03-28-progress-display-redesign/.openspec.yamlopenspec/changes/archive/2026-03-28-progress-display-redesign/design.mdopenspec/changes/archive/2026-03-28-progress-display-redesign/proposal.mdopenspec/changes/archive/2026-03-28-progress-display-redesign/specs/archive-pipeline/spec.mdopenspec/changes/archive/2026-03-28-progress-display-redesign/specs/cli/spec.mdopenspec/changes/archive/2026-03-28-progress-display-redesign/specs/progress-display/spec.mdopenspec/changes/archive/2026-03-28-progress-display-redesign/tasks.mdopenspec/specs/archive-pipeline/spec.mdopenspec/specs/cli/spec.mdopenspec/specs/progress-display/spec.mdsrc/Arius.Cli/CliBuilder.cssrc/Arius.Core/Encryption/PassphraseEncryptionService.cs
Comment on lines
+1026
to
+1036
| internal static (string current, string total, string unit) SplitSizePair(long current, long total) | ||
| { | ||
| var totalInfo = total.Bytes(); | ||
| var unit = totalInfo.LargestWholeNumberSymbol; | ||
| var divisor = total > 0 ? (double)total / totalInfo.LargestWholeNumberValue : 1.0; | ||
| var currentVal = divisor > 0 ? current / divisor : 0.0; | ||
| var totalVal = totalInfo.LargestWholeNumberValue; | ||
| return (currentVal.ToString("0.00", CultureInfo.InvariantCulture), | ||
| totalVal .ToString("0.00", CultureInfo.InvariantCulture), | ||
| unit); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential division-by-zero edge case in SplitSizePair.
When total is 0, totalInfo.LargestWholeNumberValue will be 0, and the divisor calculation (double)total / totalInfo.LargestWholeNumberValue becomes 0 / 0 which is NaN. This causes currentVal = current / divisor to also be NaN, resulting in display issues.
Proposed fix
internal static (string current, string total, string unit) SplitSizePair(long current, long total)
{
+ if (total <= 0)
+ return ("0.00", "0.00", "B");
+
var totalInfo = total.Bytes();
var unit = totalInfo.LargestWholeNumberSymbol;
var divisor = total > 0 ? (double)total / totalInfo.LargestWholeNumberValue : 1.0;
var currentVal = divisor > 0 ? current / divisor : 0.0;
var totalVal = totalInfo.LargestWholeNumberValue;
return (currentVal.ToString("0.00", CultureInfo.InvariantCulture),
totalVal .ToString("0.00", CultureInfo.InvariantCulture),
unit);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| internal static (string current, string total, string unit) SplitSizePair(long current, long total) | |
| { | |
| var totalInfo = total.Bytes(); | |
| var unit = totalInfo.LargestWholeNumberSymbol; | |
| var divisor = total > 0 ? (double)total / totalInfo.LargestWholeNumberValue : 1.0; | |
| var currentVal = divisor > 0 ? current / divisor : 0.0; | |
| var totalVal = totalInfo.LargestWholeNumberValue; | |
| return (currentVal.ToString("0.00", CultureInfo.InvariantCulture), | |
| totalVal .ToString("0.00", CultureInfo.InvariantCulture), | |
| unit); | |
| } | |
| internal static (string current, string total, string unit) SplitSizePair(long current, long total) | |
| { | |
| if (total <= 0) | |
| return ("0.00", "0.00", "B"); | |
| var totalInfo = total.Bytes(); | |
| var unit = totalInfo.LargestWholeNumberSymbol; | |
| var divisor = total > 0 ? (double)total / totalInfo.LargestWholeNumberValue : 1.0; | |
| var currentVal = divisor > 0 ? current / divisor : 0.0; | |
| var totalVal = totalInfo.LargestWholeNumberValue; | |
| return (currentVal.ToString("0.00", CultureInfo.InvariantCulture), | |
| totalVal .ToString("0.00", CultureInfo.InvariantCulture), | |
| unit); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Arius.Cli/CliBuilder.cs` around lines 1026 - 1036, SplitSizePair can
produce NaN when totalInfo.LargestWholeNumberValue is 0; change the divisor and
currentVal calculations to guard against a zero largest-whole-number value by
checking totalInfo.LargestWholeNumberValue > 0 before computing divisor =
(double)total / totalInfo.LargestWholeNumberValue, otherwise set divisor = 1.0,
then compute currentVal = divisor > 0 ? current / divisor : 0.0 and keep
totalVal = totalInfo.LargestWholeNumberValue (which may be 0) so formatting
remains safe; update only the computations in SplitSizePair (variables
totalInfo.LargestWholeNumberValue, divisor, currentVal, totalVal) to implement
this guard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rfc2898DeriveBytesconstructor withRfc2898DeriveBytes.Pbkdf2(fixes SYSLIB0060 warning)SplitSizePairhelperPadLeft/PadRight— SpectreNoBorderdoes not pad cells so alignment must be done manuallyCommits
45f1bf1fix: replace obsolete Rfc2898DeriveBytes constructor (SYSLIB0060)bc4241crefactor: use Spectre Table for per-file and TAR bundle progress rowsa057a33fix: use InvariantCulture and clamp pct to 100%b6d459ffix: use fixed 2-decimal format in SplitSizePaird1f9c6dfix: pre-pad size columns manually for true decimal alignmentSummary by CodeRabbit
New Features
Refactor