Fix spinner flicker in embedded terminals - #42
Merged
Merged
Conversation
The spinner output flickered in terminals that don't handle rapid ANSI cursor movement well (Copilot CLI terminal, VS Code terminal, some iTerm2 configs). Root cause: renderWorkersLocked made N+1 separate fmt.Fprintf calls per frame (one per worker line + cursor-up), and Write passed the spinner frame through before appending workers, so the terminal briefly rendered partial frames between syscalls. Three changes: 1. Extract buildWorkerFrameLocked that writes to a strings.Builder instead of directly to the terminal. renderWorkersLocked and Write both use it to compose a complete frame in memory first. 2. Write now combines the spinner frame bytes and worker row escapes into a single buffer before writing, eliminating the gap between the spinner frame and its worker rows. 3. Wrap all multi-line output in DEC synchronized output sequences (\033[?2026h / \033[?2026l). Terminals that support mode 2026 (kitty, iTerm2, VS Code, foot, WezTerm) defer rendering until the end marker, eliminating flicker entirely. Terminals that don't recognize the sequence ignore it harmlessly. clearSpinnerLines gets the same single-write treatment for consistency, though it fires less frequently.
Two more sources of primary-bar flicker: 1. The spinner library writes \r+content without erasing the line first. When the label shrinks (e.g. [10/10] -> [1/1]), leftover characters from the previous longer frame remain visible for one tick. Fix: replace the leading \r in Write with \r\033[2K so the line is always cleared before the new frame is painted. 2. The cursor is visible during animation and its per-tick repositioning creates visual noise, especially in terminals with slower cursor rendering. Fix: hide the cursor in startAnimator (\033[?25l) and restore it unconditionally in stopAnimator (\033[?25h), so it stays hidden for exactly the duration of the animation and can't be left permanently invisible if the process exits early.
…goroutine The spinner library (briandowns/spinner) reads s.Prefix and s.Suffix inside s.mu on every tick. renderProgress was writing u.spinner.Suffix directly from the calling goroutine without holding s.mu — a genuine data race. On amd64 a string is pointer+length (two 64-bit words), so a concurrent write can produce a torn read where pointer and length are from different updates, yielding garbage output or the wrong label text for one frame. go test -race confirms the race is now gone. Fix: add pendingSuffix to spinnerWriter. renderProgress writes it under sw.mu. A PreUpdate callback on the spinner (called while s.mu is held, before s.Suffix is read for the frame) applies pendingSuffix to s.Suffix atomically. Lock order is s.mu → sw.mu, consistent with how the spinner goroutine already calls sw.Write under s.mu. This eliminates the 'Resolving actions' / 'Planning pins' label text flickering at phase transitions: the label now only changes at tick boundaries, never mid-frame, and never races with the render goroutine.
…azily The old code cleared worker slot data then waited up to 120ms for the next spinner tick to erase the stale rows from the terminal. During that window the rows sat on screen, then all vanished at once — visible as a jump at the Resolving→Planning phase transition. Fix: call renderWorkersLocked (already under sw.mu) before releasing the lock. It sees all-empty workers, erases the previously-rendered rows, and resets nRendered to 0, all in one synchronized write.
Experiment: remove the dynamic label text from the spinner's top line entirely. The top line now shows only the braille glyph — static, never redraws. All per-phase and per-action information lives in the worker rows below, which already handle it well. This eliminates the last remaining source of top-line flicker: label text changing between phases. Similar to how npm/yarn show a bare spinner during installs — the glyph signals 'working' and the rows below tell you what. UpdateLabel is now headless-only (logs phase transitions for --json / CI mode) and a no-op on TTY.
Switch from Suffix-based labels to Prefix-based (cli/cli style). Label sits left of the glyph: 'Resolving actions ⠋'. Label changes come from UpdateLabel, which writes pendingPrefix under sw.mu; PreUpdate reads it under s.mu→sw.mu eliminating the data race. renderProgress is stripped down to only the detail/slot-0 path. Dead fields (progLabel, progLast) removed.
UpdateLabel is now a TTY no-op. The label set in StartProgress stays for the spinner's lifetime, eliminating any mid-run flicker from label swaps. Headless mode still logs phase boundaries. PreUpdate hook and pendingPrefix field removed — no longer needed.
briandowns calls Write twice per tick: once with \r\033[K (erase the previous frame) and once with the actual frame glyph+prefix. Previously both triggered buildWorkerFrameLocked, giving two full cursor-down/up sequences per 120ms tick — the root cause of residual flicker. Now the erase write passes straight through; worker rows are refreshed only on the frame write, halving cursor movement per tick.
Both lines now render as 'text ⠋' — label left, glyph right — consistent with the cli/cli pattern used for the main spinner. Also strips the GH_ACTIONS_PIN_DEBUG_SPINNER trace instrumentation added for diagnosis.
… drop []byte allocs
Undocumented env var for lab/non-standard environments where workflows live outside .github/workflows/. When set, workflow discovery and lockfile I/O (both read and write) use the override directory. Implementation: - workflowfile: extract DiscoverWorkflowsIn(dir) from DiscoverWorkflows - lockfile: replace State.repoRoot with State.lockPath; add LoadStateAt for explicit lockfile paths - root: read env var in newRun, route to DiscoverWorkflowsIn and LoadStateAt when set
PresentResults and renderPinSummary both printed 'All N workflows valid' on the happy path. Remove the one in PresentResults — renderPinSummary is the canonical post-remediation summary renderer.
The --no-fix block printed the SSO authorization URL separately from the canonical location that fires after remediation. Remove the --no-fix copy so there is one authoritative SSO hint site.
'All 0 workflows valid' is confusing when a repo has no workflow files. Print 'No workflows to check' and return early instead.
TermCaution is for non-fatal but important warnings. Red reads as an error; yellow matches the caution semantics and keeps the '!' icon.
TermNeutral renders dim gray, making the path hard to read. TermDetail is visible but not prominent — right for the one thing users copy.
- Unit test: PresentResults no longer emits 'All valid' success line - Scenario: no_workflows_summary asserts no 'All 0' in output - Scenario: sso_no_fix_single_url asserts SSO URL shown once in --no-fix
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR primarily targets terminal UI stability by reducing spinner/progress flicker via atomic frame writes and improved worker-row rendering, while also introducing workflow/lockfile path flexibility and tweaking summary output behavior.
Changes:
- Compose spinner + worker rows into single synchronized writes; improve cursor/line clearing and worker-row glyph placement to reduce flicker in embedded terminals.
- Add support for discovering workflows in an explicit directory and loading/saving the lockfile at an explicit path (used via
GH_ACTIONS_PIN_WORKFLOWS_DIR). - Adjust summary/output behavior for “no workflows” and shift where success messaging is emitted; add/extend scenario + unit test coverage.
Show a summary per file
| File | Description |
|---|---|
| internal/ui/ui.go | Reworks spinner/worker rendering (single-write frames, synchronized output, cursor hide/restore) and adjusts some terminal message styling. |
| internal/workflowfile/workflowfile.go | Refactors workflow discovery to allow scanning an explicit directory. |
| internal/workflowfile/workflowfile_test.go | Adds unit tests for DiscoverWorkflowsIn. |
| internal/lockfile/state.go | Introduces LoadStateAt and stores an explicit lockfile path for saves. |
| cmd/gh-actions-pin/root.go | Adds GH_ACTIONS_PIN_WORKFLOWS_DIR support and wires workflow/lockfile path behavior. |
| cmd/gh-actions-pin/pin_summary.go | Prints “No workflows to check” and avoids “All 0 … valid”. |
| cmd/gh-actions-pin/format/terminal.go | Removes success-line emission from PresentResults (now owned elsewhere). |
| cmd/gh-actions-pin/format/terminal_test.go | Adds coverage ensuring PresentResults no longer prints the success line. |
| cmd/gh-actions-pin/check.go | Tweaks terminal output (resolution record line) and removes an SSO URL emission in --no-fix path. |
| test/scenarios/catalog.yml | Adds scenarios for SSO URL dedupe and “no workflows” summary behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 5
Comment on lines
127
to
131
Comment on lines
+390
to
+393
| isErase := (len(p) == 4 && string(p) == "\r\033[K\n") || | ||
| (len(p) == 3 && string(p) == "\r\033[K") || | ||
| (len(p) == 5 && string(p) == "\r\033[2K\n") || | ||
| (len(p) == 4 && string(p) == "\r\033[2K") |
Comment on lines
1283
to
1286
| detail := u.progDetail | ||
|
|
||
| if label == "" { | ||
| if u.progLast == "" { | ||
| return | ||
| } | ||
| label = u.progLast | ||
| } else { | ||
| u.progLast = label | ||
| if detail == "" { | ||
| return | ||
| } |
Comment on lines
+1303
to
+1306
| if u.spinWriter != nil { | ||
| u.spinWriter.setDetail(detail) | ||
| } | ||
| u.progHasDetail = detail != "" | ||
|
|
||
| var suffix string | ||
| if !u.noColor { | ||
| suffix = u.output.String(label).Bold().String() | ||
| } else { | ||
| suffix = label | ||
| } | ||
|
|
||
| u.spinner.Prefix = "" | ||
| if suffix != "" { | ||
| u.spinner.Suffix = " " + suffix | ||
| } else { | ||
| u.spinner.Suffix = "" | ||
| } | ||
| u.progHasDetail = true |
Comment on lines
+536
to
+537
| buf.WriteString(sw.prefix) | ||
| buf.WriteString(workerSpinFrames[0]) // placeholder glyph; real tick replaces it |
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.
What
Fixes spinner/progress bar flicker in embedded terminals (Copilot CLI, VS Code, iTerm2 configurations), plus several related CLI UX improvements originally developed on the
nodeselector/scenario-matrix-live-testingstack that did not land in main when that branch merged.Spinner flicker fixes (
internal/ui/ui.go)1. Unbuffered multi-write per frame
Each tick made N+1 separate writes — one for the spinner frame, one per worker row. Fixed by composing the full frame into a single
strings.Builderand writing it atomically, wrapped in DEC synchronized output (\033[?2026h/l) for terminals that support it.2. Ghost characters on label shrink
When the spinner suffix shrank, previous longer text left ghost characters. Fixed with
\r\033[2Kbefore each frame. Hides cursor during animation (\033[?25l), restores unconditionally on stop. Both cursor sequences are now written undersw.muinside synchronized blocks to prevent interleaving.3. Data race on spinner Suffix
renderProgress()wroteu.spinner.Suffixfrom the calling goroutine while the spinner goroutine read it. Fixed with apendingSuffixfield applied inPreUpdate, which fires inside the spinner's internal lock.4. Lazy worker row erase at phase transitions
ClearWorkerStatuseszeroed slot data but waited up to 120ms for the next tick to erase rows. Fixed by callingrenderWorkersLocked()while still holding the lock.5. Double render per tick
briandownscallsWritetwice per tick: once to erase (\r\033[K, 4 bytes) and once for the new frame. Both previously triggeredbuildWorkerFrameLocked. Fixed by detecting erase writes via allocation-free byte comparison (p[0]=='\r' && p[1]=='\033' && p[2]=='[' && p[3]=='K') and passing them straight through.6. One-tick blank line on resume
After
PauseProgress→ResumeProgress, briandowns never writes a frame immediately onStart()— waits for first ticker tick (~120ms blank). Fixed by writing a synthetic frame instartAnimatorbefore launching the ticker. Frame is always written regardless of whether a label prefix is set.7. Spinner style inconsistency
Main spinner had label on right; worker rows had glyph on left. Aligned both with cli/cli style: label left, glyph right —
Resolving actions ⠋. Label is static for the spinner's lifetime.8.
renderProgressempty-detail handlingWhen
progDetailwas cleared,renderProgressreturned early without clearing worker slot 0 or resettingprogHasDetail, leaving stale detail visible and causingclearSpinnerLinesto assume an extra row. Fixed to explicitly clear slot 0 and reset the flag when detail transitions to empty.CLI UX fixes (from #39 and #40)
GH_ACTIONS_PIN_WORKFLOWS_DIRenv var (cmd/gh-actions-pin/root.go,internal/workflowfile/)Override the workflows directory for non-standard layouts (monorepos, custom CI paths).
Duplicate output cleanup (
cmd/gh-actions-pin/format/terminal.go,pin_summary.go)--no-fixpathVisibility improvements (
internal/lockfile/state.go)TermCautionuses yellow instead of redTermDetailfor better visibilityScope
Changes span
internal/ui/ui.go,cmd/gh-actions-pin/,internal/lockfile/, andinternal/workflowfile/. No changes to exit codes. Tested with-race; no races detected.