fix(tui): repair sync pipeline hang, ctrl+c, hover, hit-test bounds#144
Merged
Conversation
Fixes five bugs a /code-review (xhigh) flagged in the merged #142/#143 TUI work, each verified by driving a real pty and a reverse-verified regression test: - BLOCKER: pipeline-mode Init never armed waitForEvent, so the goroutine's installDoneMsg was never read — `install <slug>` on a TTY hung forever. Arm waitForEvent; add TestPipelineDrainsChannelAndCompletes (a real headless program that times out if the drain is missing). - ctrl+c couldn't abort a sync install: the run closure dropped its ctx and sync.Execute took none. Add sync.ExecuteContext(ctx, …) threading ctx to brew.InstallWithProgress; the pipeline closure now passes it, so a single ctrl+c cancels. - Data race + hidden errors on the TTY path: the CLI read syncpkg.Execute's *result across the goroutine boundary. Read only the returned error (delivered safely through the model) — race-free (`go test -race` clean) — and return it so cobra surfaces config-step failures. Non-TTY keeps the synchronous summary. - Hover highlight painted only the first span: lipgloss ends spans with a full reset (\e[0m), not \e[49m, so the old strip was a no-op. hoverBg now re-injects the background after every reset, spanning the whole row. - Mouse hit-tests had no bounds check, so a status-bar click mapped to a loadout/toggle on short terminals. Add Model.inBody(y) guarding all four. Claude-Session: https://claude.ai/code/session_01HLoiSd2k9EJJ1HPjjDgUgo
5 tasks
fullstackjam
added a commit
that referenced
this pull request
Jul 16, 2026
…l, wizard lows (#147) * fix(tui): repair ctrl+c abort lifecycle Three defects in the install-wizard abort/force-quit machinery (added in #144), all surfaced by an adversarial re-review of the v0.63.0..HEAD range: - C1: a single ctrl+c cancels the context, which SIGKILLs the in-flight brew/npm subprocess, so ApplyContext returns a non-nil error ("signal: killed"). The `m.aborting && installErr == nil` guard therefore never assigned ErrAborted for the common case (abort during the package phase), and the CLI misreported the abort as an install failure and nagged the screen-recording reminder. Now join ErrAborted with the cause whenever aborting, so errors.Is(err, ErrAborted) holds. - M3: the config steps (shell/dotfiles/macOS/post-install) take no ctx and ApplyContext had no ctx.Err() gate, so an aborted install kept symlinking dotfiles and rewriting macOS defaults after the user asked to stop. Gate between phases so an abort skips not-yet-started work. - C2: force-quit (2nd ctrl+c) returned tea.Quit immediately while the install goroutine was still running, racing the deferred os.Stdout restore and letting the engine mutate the system after control returned. Run/RunPipeline now join the goroutine (via a done channel closed once ApplyContext returns) before restoring stdout. The prior abort test only injected a nil-error installDoneMsg -- the one case that already worked -- masking C1. Updated to inject a killed- subprocess error, plus tests for the ctx gate and the goroutine join. Claude-Session: https://claude.ai/code/session_01LyhLX9U3aNuUUAKX1H9JbQ * fix(cli): run post-install scripts on interactive slug installs R1 regression from #146: routing `install <slug>` / `-u` / `--from` on a TTY through the wizard pipeline forces plan.Silent=true to keep prompts out of the alt-screen. But Silent is overloaded -- applyPostInstall also *skips execution* when Silent, so a RemoteConfig carrying a post_install block had its script silently dropped, where v0.63.0 (linear, Silent=false) previewed, confirmed, and ran it. The alt-screen can't host the script preview + confirm, so defer post-install to after teardown on a normal terminal (mirroring the screen-recording reminder): strip it from the streamed plan via splitPostInstall and run it via installer.RunPostInstallAfterTUI on a clean run, before the reminder (Step 8 order). A failing script stays a soft error, not fatal. Scope: RemoteConfig sources only. Presets, --packages-only, the bare wizard (no PostInstall in the plan), and sync are unaffected. Claude-Session: https://claude.ai/code/session_01LyhLX9U3aNuUUAKX1H9JbQ * fix(tui): npm package count, resize scroll, elapsed clock, select lows Grab-bag of install-wizard defects from the v0.63.0..HEAD review: - M1: the npm outer retry (applyNpm) re-runs the install and re-emits an "already installed" skip for every package a prior attempt installed, breaking the one-terminal-event-per-package invariant. The renderer's skippedPkgs counter (unclamped, unlike incPhase) inflated past the total, so the completion footer could show "-1 packages". Dedup: ignore a skip for a package that already produced a terminal event (terminalSeen). - M2: WindowSizeMsg updated height but didn't re-clamp scroll. selectList renders from a re-clamped copy while selectHitTest reads the stored scroll, so a click right after a resize (before any mouse motion re-clamps) could toggle the wrong package. Re-clamp on resize. - C3: the tick loop never stopped, so the completion footer's elapsed clock kept counting up while the user read it. Freeze it at done. - selVisible counted height-6 but selectList renders height-4 rows, leaving two blank rows at the bottom and two the keyboard cursor can't reach. Align the counts. - A click on the vertical divider column toggled the adjacent package; treat the divider as neither pane. - Fix the stale hover comment (hover no longer skips the keyboard-cursor row). Claude-Session: https://claude.ai/code/session_01LyhLX9U3aNuUUAKX1H9JbQ * chore(installer): log install_started on the pipeline install path The wizard pipeline path (PlanForConfig -> RunPipeline -> ApplyContext) skips runInstallContext, so a streamed slug/preset install logged install_completed with no matching install_started. Emit it in PlanForConfig, mirroring runInstallContext's event. Claude-Session: https://claude.ai/code/session_01LyhLX9U3aNuUUAKX1H9JbQ
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.
What does this PR do?
Fixes five bugs a
/code-review(xhigh, workflow-backed) flagged in the just-merged #142 (mouse) / #143 (sync pipeline) TUI work. Each was verified by driving a real pty and locked with a regression test.Why?
The review caught real defects my own tests and tmux checks had bypassed:
Initnever armedwaitForEvent→ the goroutine'sinstallDoneMsgwas never read →install <slug>on a TTY hung foreverwaitForEvent(m.events)in the pipelineInitbranchsync.Executetook none)sync.ExecuteContext(ctx, …)threading ctx tobrew.InstallWithProgress; pipeline closure passes it → single ctrl+c cancelsExecute's*resultacross the goroutine boundary)go test -raceclean) and return it so cobra surfaces failures\e[0m(full reset), not\e[49m, so the old strip was a no-ophoverBgre-injects the background after every reset → spans the whole rowModel.inBody(y)guarding all four hit-testsTesting
go vet ./..., fullinternal/...suite,go test -raceon wizard+sync (no race),make build— all greenTestPipelineDrainsChannelAndCompletes— a real headlesstea.Programthat times out if the drain is missing; reverse-verified (FAILs "pipeline hung" when the fix is reverted, PASSes with it)\e[0mRunPipelineharness on a tmux pty: normal run reaches DONE (no hang); single ctrl+c returnscontext canceled(graceful abort)Known follow-up (not in scope)
The DONE footer reads "N packages · Ns"; a config-only sync shows "0 packages" (the config steps are confirmed by the ✓ in the pipeline sidebar, not the footer). Informational only — surfacing a settings count needs plumbing the count into the model.
Cross-repo checklist
openboot.dev? — NoNotes for reviewer
sync.Execute(plan, dryRun)kept as acontext.Background()wrapper overExecuteContext(RunContext pattern) — zero churn to its ~10 existing test callers.https://claude.ai/code/session_01HLoiSd2k9EJJ1HPjjDgUgo