fix: cap clear width to the current terminal size (#106) - #235
Open
Shishir2405 wants to merge 1 commit into
Open
fix: cap clear width to the current terminal size (#106)#235Shishir2405 wants to merge 1 commit into
Shishir2405 wants to merge 1 commit into
Conversation
When the terminal shrinks after the widest bar has been rendered, clearProgressBar padded to that old (wider) maxLineWidth. The terminal then wraps the overlong clear string onto a new line instead of clearing in place, so the bar appears to restart on every render. Cache the terminal width observed by the existing termWidth() calls in renderProgressBar/fitProgressBarWidth (state.knownTermWidth) and cap the clear width to it when it's narrower than maxLineWidth, via a new pure clampClearWidth helper. This adds no new termWidth() call to the clear path -- clear runs before render each cycle, so it uses the value from the previous cycle.
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
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.
Closes #106.
The bug
clearProgressBarpads the clear line tostate.maxLineWidth, the widest the bar has ever been rendered:If the terminal is resized narrower after that widest render, this pads past the current terminal's column count. The terminal itself then wraps the overlong space-string onto a new line instead of clearing in place — the cursor moves down, and the bar appears to restart on every subsequent render instead of staying on one line, exactly as shown in the issue's screen recording.
The fix
renderProgressBarandfitProgressBarWidthalready calltermWidth(c.writer)on every render that isn't a full-width or an ignoreLength (spinner) bar (that covers the issue's repro — a plainprogressbar.Default(...)bar). I cache that observed width into a newstate.knownTermWidthfield, andclearProgressBarcaps to it via a new pureclampClearWidth(maxLineWidth, knownTermWidth int) inthelper, instead of callingtermWidthitself.clearProgressBarruns beforerenderProgressBarin each render cycle (see(*ProgressBar).render()), so this uses last cycle's observed width — a resize is picked up within one render, and no newtermWidth()call is added to the render path.A
-racefinding along the way — please read before mergingI first tried calling
termWidth(c.writer)directly insideclearProgressBar(obvious, one extra call, same guard). The logic was identical, but it madego test -race .(the exact command CI runs) fail on every run. I switched to the cached-value approach above specifically to avoid adding a call to that hot path — but honestly,go test -race .still fails intermittently on this branch (I reproduced it 3 of the runs I tried). On unmodifiedmain, the same command passed clean across 8 consecutive runs on my machine.The race itself is not in the logic I'm changing. Both failures I captured trace back to this, in
NewOptions64:TestOptionSetSpinnerChangeInterval(and similar tests) build such a bar withmax: -1and never callFinish()/Exit()on it, so this ticker keeps callingrender()→clearProgressBar()for the rest of the test binary's life. Once that leaked goroutine'srender()calls land at the wrong moment, they race against a different, already-finished test's buffer or the sharedtermWidthtest hook:This is a real, pre-existing characteristic (a spinner bar with
OptionSetSpinnerChangeIntervalis designed to animate forever until the caller finishes it — the "leak" is really that these particular tests never do), not a bug in the code I'm changing. But I can't pretend my diff has zero relationship to it either: adding even a couple of cheap lines torender()'s hot path is apparently enough to shift scheduling and make this pre-existing timing-dependent race land far more often in my testing than it did onmain. I don't think fixing that goroutine's lifecycle belongs in this PR — it's a separate, bigger concern (how/whether to stop an indeterminate bar's ticker without changing the "animate untilFinish()" behavior it's designed to have) than what #106 asked for. Flagging it here rather than shipping a PR that looks clean and then surprises you in CI.Happy to help chase the leak down in a follow-up if that's useful, or to hear if you'd rather this PR waited on that being fixed first.
Test plan
clampClearWidth(pure function): shrunk terminal caps down, still-wider terminal leaves it alone, unknown width falls back, exact-equal is a no-op.go test -run TestClampClearWidth -v .go build ./...,go vet .,gofmt -l— clean.go test -vet=off .(no-race) — passes.go mod tidy -diff— no diff (no new dependencies).go test -race -v -cover -vet=off .— the changed logic itself is race-free in isolation (-run TestClampClearWidth), but the full suite intermittently hits the pre-existing goroutine-leak race described above. See that section for details and reproduction.