Conversation
Deploys now show a real progress bar during artifact upload instead of just a spinner with a byte count. The bar tracks uncompressed bytes written to the tar against the known total from the manifest, so it works on both first deploys and delta uploads. When the upload completes, the summary line shows transfer stats alongside caching benefits — how many files were reused and how many bytes that saved. When everything is cached, it says so directly instead of showing a meaningless "0 bytes uploaded." Also removed the client-side buildkit startup timeout. Buildkit no longer launches per-deploy, so the 60-second timer was watching for a condition that doesn't happen anymore. If the server hangs, that's a server-side concern.
📝 WalkthroughWalkthroughThis pull request adds atomic-based tracking of uncompressed bytes during tar creation by extending 📝 Coding Plan
Comment |
Contributor
Author
|
Preview:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/commands/deploy.go (1)
568-571:⚠️ Potential issue | 🟠 MajorPreserve the fully-cached special case in explain mode.
When
neededPathsis empty, the only bytes sent are the synthetic empty tar wrapper, so this summary becomesUpload complete: 45 B ... reused 22/22 files. The feature goal — and the interactive path indeploy_ui.go— is to reportall 22 files reused from previous deployinstead of treating the empty tar as a real upload.💡 One way to keep the explain summary aligned with the interactive path
- summary := fmt.Sprintf("\rUpload complete: %s in %.1fs at %s", - upload.FormatBytes(uploadBytes), - uploadDuration.Seconds(), - upload.FormatSpeed(avgSpeed)) - if useOptimized && cachedFiles > 0 { - summary += fmt.Sprintf(", reused %d/%d files", cachedFiles, totalFiles) - if cachedBytes > 0 { - summary += fmt.Sprintf(" (saved %s)", upload.FormatBytes(cachedBytes)) - } - } + var summary string + if useOptimized && cachedFiles > 0 && int(cachedFiles) == totalFiles { + summary = fmt.Sprintf("\rUpload complete: all %d files reused from previous deploy", totalFiles) + } else { + summary = fmt.Sprintf("\rUpload complete: %s in %.1fs at %s", + upload.FormatBytes(uploadBytes), + uploadDuration.Seconds(), + upload.FormatSpeed(avgSpeed)) + if useOptimized && cachedFiles > 0 { + summary += fmt.Sprintf(", reused %d/%d files", cachedFiles, totalFiles) + if cachedBytes > 0 { + summary += fmt.Sprintf(" (saved %s)", upload.FormatBytes(cachedBytes)) + } + } + }Also applies to: 606-616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/deploy.go` around lines 568 - 571, The explain-mode summary currently treats the synthetic empty tar as a real upload when neededPaths is empty; update the block guarded by useOptimized && cachedFiles > 0 to check len(neededPaths) and preserve the fully-cached special case: if len(neededPaths) == 0 print the all-reused message (e.g., "All X/Y files reused from previous deploy") using ctx.Printf, otherwise keep the existing "Reused %d/%d files ... uploading %d" line; apply the same conditional change to the corresponding explain-mode summary at the other location referenced (the 606-616 block) so explain output matches the interactive deploy_ui.go path.
🧹 Nitpick comments (1)
pkg/tarx/tarx_test.go (1)
179-180: Add one non-nilcounter case.All of the updated call sites still pass
nil, so the newuncompressedBytespath inpkg/tarx/tarx.gonever gets asserted. A small test that drains the tar with a realatomic.Int64and checks it against the sum of included regular-file sizes would catch regressions in the progress-bar plumbing.Also applies to: 416-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tarx/tarx_test.go` around lines 179 - 180, Add a test case that passes a real non-nil counter to MakeTar so the uncompressedBytes path in pkg/tarx/tarx.go is exercised: create an atomic.Int64, call MakeTar(tmpDir, nil, counter) (or the existing test helper pattern around MakeTar), iterate/drain the returned tar reader to sum the sizes of all regular files, and then assert that the atomic counter value equals that sum; update the existing test around MakeTar in tarx_test.go (lines near the existing reader, err := MakeTar(...) usage) to include this non-nil counter case so regressions in the progress-bar/uncompressedBytes plumbing are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/commands/deploy_ui.go`:
- Around line 115-116: The progress rendering treats uploadPct as always
printable, so when uploadPct is -1 (unknown) the UI shows "0%"; change the code
that assigns uploadPct from msg.Fraction to preserve the -1 sentinel (e.g., set
uploadPct = msg.Fraction but if msg.Fraction < 0 keep -1) and update every place
that formats the percent (where uploadPct is used to build the progress string)
to conditionally render the percentage only if uploadPct >= 0 (otherwise render
an empty/placeholder like "?" or omit the percent). Ensure the same conditional
logic is applied wherever uploadEstTotal/uploadPct are used to display percent
(including the other progress-formatting blocks noted) so unknown totals never
display as 0%.
In `@cli/commands/deploy.go`:
- Around line 588-595: The code is writing raw ANSI/TTY control sequences
directly to stderr (fmt.Fprintf(os.Stderr, "\r\033[K" ...)) which bypasses the
progresswriter and will leak escapes in non-TTY or explain-format output;
replace these direct fmt.Fprintf calls with calls to the shared progresswriter
API (e.g. use progresswriter.Write/Printf/Update or the existing progressWriter
variable that handles TTY vs plain/rawjson formats) to render the "Uploading
artifacts: %d%% ..." line so formatting is conditional on TTY/explain-format,
and make the same replacement for the other occurrence referenced (the block at
606-609) so all progress output goes through progresswriter rather than
os.Stderr with raw escapes.
---
Outside diff comments:
In `@cli/commands/deploy.go`:
- Around line 568-571: The explain-mode summary currently treats the synthetic
empty tar as a real upload when neededPaths is empty; update the block guarded
by useOptimized && cachedFiles > 0 to check len(neededPaths) and preserve the
fully-cached special case: if len(neededPaths) == 0 print the all-reused message
(e.g., "All X/Y files reused from previous deploy") using ctx.Printf, otherwise
keep the existing "Reused %d/%d files ... uploading %d" line; apply the same
conditional change to the corresponding explain-mode summary at the other
location referenced (the 606-616 block) so explain output matches the
interactive deploy_ui.go path.
---
Nitpick comments:
In `@pkg/tarx/tarx_test.go`:
- Around line 179-180: Add a test case that passes a real non-nil counter to
MakeTar so the uncompressedBytes path in pkg/tarx/tarx.go is exercised: create
an atomic.Int64, call MakeTar(tmpDir, nil, counter) (or the existing test helper
pattern around MakeTar), iterate/drain the returned tar reader to sum the sizes
of all regular files, and then assert that the atomic counter value equals that
sum; update the existing test around MakeTar in tarx_test.go (lines near the
existing reader, err := MakeTar(...) usage) to include this non-nil counter case
so regressions in the progress-bar/uncompressedBytes plumbing are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad7cf909-c3df-4c7d-a697-ba40a9413bb3
📒 Files selected for processing (6)
cli/commands/deploy.gocli/commands/deploy_ui.gopkg/buildkit/build_test.gopkg/progress/upload/upload.gopkg/tarx/tarx.gopkg/tarx/tarx_test.go
When files are reused from a previous deploy, the summary now shows how much time that likely saved based on the observed upload speed, e.g. "reused 14/22 files (saved 2.1MB, ~32s)". Makes the caching benefit tangible instead of abstract.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/commands/deploy.go (1)
588-596:⚠️ Potential issue | 🟠 MajorRaw ANSI escapes leak in non-TTY/plain output.
These
fmt.Fprintfcalls write raw ANSI escape sequences (\r\033[K) directly to stderr, bypassing theprogresswriter. In--explain-format=plain/rawjsonor when stderr is redirected/non-TTY, these escapes will corrupt machine-readable output.Consider checking
isTTYbefore emitting escape sequences, or routing through the progresswriter.🐛 Proposed fix
progressReader := upload.NewProgressReader(r, func(progress upload.Progress) { enrichUploadProgress(&progress, &uncompressedWritten, totalUncompressed) uploadBytes = progress.BytesRead // Print progress every 500ms to avoid spamming - if progress.Fraction > 0 && time.Since(lastPrintTime) >= 500*time.Millisecond { + if isTTY && progress.Fraction > 0 && time.Since(lastPrintTime) >= 500*time.Millisecond { lastPrintTime = time.Now() fmt.Fprintf(os.Stderr, "\r\033[K") // Clear to end of line fmt.Fprintf(os.Stderr, "Uploading artifacts: %d%% — %s / ~%s at %s", int(progress.Fraction*100), upload.FormatBytes(progress.BytesRead), upload.FormatBytes(progress.EstimatedTotalBytes), upload.FormatSpeed(progress.BytesPerSecond)) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/deploy.go` around lines 588 - 596, The code writes raw ANSI escapes (`"\r\033[K"`) and progress lines directly to stderr using fmt.Fprintf inside the upload progress block (checks progress.Fraction and lastPrintTime); modify this so ANSI escapes are only emitted when stderr is a TTY (use an isTTY check) or, better, route all output through the existing progresswriter API instead of direct fmt.Fprintf; update the block that writes the progress (the fmt.Fprintf calls) to call progresswriter.WriteProgress or a new progresswriter.ClearLine/WriteLine method when non-TTY or when explain-format is plain/rawjson, and wrap the escape-emitting logic behind an isTTY condition so machine-readable output is not polluted.
🧹 Nitpick comments (1)
cli/commands/deploy.go (1)
850-860: Consider clampingFractionto the[0, 1]range.If
written.Load()exceedstotalUncompresseddue to timing or edge cases,Fractioncould exceed 1.0, leading to percentage displays over 100%. While unlikely, a simple clamp would be more defensive.♻️ Proposed defensive fix
func enrichUploadProgress(p *upload.Progress, written *atomic.Int64, totalUncompressed int64) { if totalUncompressed <= 0 { return } p.Fraction = float64(written.Load()) / float64(totalUncompressed) + if p.Fraction > 1.0 { + p.Fraction = 1.0 + } if p.Fraction > 0 { p.EstimatedTotalBytes = int64(float64(p.BytesRead) / p.Fraction) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/commands/deploy.go` around lines 850 - 860, enrichUploadProgress should defensively clamp the computed fraction to [0,1] to avoid >100% displays: compute frac := float64(written.Load())/float64(totalUncompressed), then clamp frac to 0 if <0 and to 1 if >1 before assigning p.Fraction; keep the existing guard that only computes p.EstimatedTotalBytes when p.Fraction > 0 (use the clamped value) so the division is safe and the estimate remains consistent with the clamped fraction. Refer to the enrichUploadProgress function and the p.Fraction, written.Load(), totalUncompressed, and p.EstimatedTotalBytes symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/commands/deploy_ui.go`:
- Around line 143-158: The deployInfo struct literal is missing initialization
of uploadPct so it defaults to 0.0; add uploadPct: -1 (per docs: -1 means
unknown) to the &deployInfo{...} initializer (the same block that sets spinner,
message, uploadSpeed, currentPhase, etc.) so the UI shows an unknown state until
progress updates arrive; locate the deployInfo constructor in deploy_ui.go and
insert uploadPct: -1 among the other fields.
---
Duplicate comments:
In `@cli/commands/deploy.go`:
- Around line 588-596: The code writes raw ANSI escapes (`"\r\033[K"`) and
progress lines directly to stderr using fmt.Fprintf inside the upload progress
block (checks progress.Fraction and lastPrintTime); modify this so ANSI escapes
are only emitted when stderr is a TTY (use an isTTY check) or, better, route all
output through the existing progresswriter API instead of direct fmt.Fprintf;
update the block that writes the progress (the fmt.Fprintf calls) to call
progresswriter.WriteProgress or a new progresswriter.ClearLine/WriteLine method
when non-TTY or when explain-format is plain/rawjson, and wrap the
escape-emitting logic behind an isTTY condition so machine-readable output is
not polluted.
---
Nitpick comments:
In `@cli/commands/deploy.go`:
- Around line 850-860: enrichUploadProgress should defensively clamp the
computed fraction to [0,1] to avoid >100% displays: compute frac :=
float64(written.Load())/float64(totalUncompressed), then clamp frac to 0 if <0
and to 1 if >1 before assigning p.Fraction; keep the existing guard that only
computes p.EstimatedTotalBytes when p.Fraction > 0 (use the clamped value) so
the division is safe and the estimate remains consistent with the clamped
fraction. Refer to the enrichUploadProgress function and the p.Fraction,
written.Load(), totalUncompressed, and p.EstimatedTotalBytes symbols when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3c3bc46-0488-4361-8342-966b43bbac8e
📒 Files selected for processing (2)
cli/commands/deploy.gocli/commands/deploy_ui.go
evanphx
approved these changes
Mar 16, 2026
phinze
deleted the
phinze/mir-798-deploy-upload-progress-bar-and-speedup-stats-mirsync-follow
branch
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
2 participants
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.
With the delta file transfer work from MIR-668, deploys compute a manifest, ask the server which files are cached, and only upload what changed. But the UI was still showing a spinner with raw byte count — no sense of progress toward completion, and no indication of how much the caching actually saved you.
Now during upload you get a real progress bar with percentage, bytes transferred, and speed. The bar is driven by an atomic counter that tracks uncompressed bytes as they stream into the tar — it increments per read chunk rather than per file, so even a single large file shows smooth progress instead of jumping from 0% to 100%.
When upload finishes, the summary line tells you what happened:
142KB at 68KB/s, reused 14/22 files (saved 2.1MB). If everything was cached, you just getall 22 files reused from previous deploy. Both interactive and explain/non-TTY modes got the treatment.While in the neighborhood, removed the client-side buildkit startup timeout machinery. Buildkit doesn't launch per-deploy anymore, so the 60-second watchdog was monitoring a ghost. Timeout detection belongs server-side.