Repository navigation
feat(tui): command palette and overlay compositor (#71, #59) - #142
Merged
Merged
Conversation
Address review findings on the ctrl+p command palette: strengthen two vacuous tests (TestPaletteHighlightSurvivesTruncation matched only past the observable window; TestPaletteOpensAndClosesWithoutTouchingNav never actually pressed ctrl+p), stop nav.go's overlay-dissolving note from becoming the package doc comment, and — the behavioral fix — make replace() dissolve the overlay only when the screen actually changes, so an async refresh landing back on the SAME screen (statusesMsg, membersMsg, entriesReloadedMsg/entriesErrMsg, logDoneMsg/timerMsg) no longer wipes a palette query the user is still typing. Deduplicates the repeated overlay-clearing pair in nav.go behind closePalette().
…ose coverage gaps Final fix wave after the whole-branch review of the command palette (#71): rewrite three comments that named the wrong mechanism (app.go's Quit-vs-Palette ordering, overlay.go's straddle guard twice), document composite's rectangular- box precondition, normalize British spellings introduced by this branch plus a handful of pre-existing ones, add three tests a mutation pass showed missing (refreshPalette's cursor reset, the query line's shaveToWidth, openPalette clearing helpAll), and fix two README nits (Invio/Enter consistency, the demo tape's temporary-PATH build step).
This was referenced Jul 30, 2026
Closed
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.
Fifth tranche of the v1.9 TUI design system milestone. Adds the
ctrl+pcommand palette and the overlay compositor that lets anything float over a screen instead of replacing it.Closes #71. Closes #59.
What changes for a user
ctrl+popens a fuzzy command palette that floats over the current screen. The screen underneath stays visible around the box — that is the point, since you pick a command by looking at what you are working on. Type a few letters,↑/↓to move,enterto run,escto close.Go to rates,Go to budgets, …). With an empty query the screen's commands come first — "what can I do here" above "take me elsewhere".exphighlights[Exp]ortandGo to [exp]ort.Notable decisions
The screen's commands are derived from its keymap, not written out a second time.
screenActionswalksscreenKeys(m).paletteBindings()— a curated twin of the existingallBindings()— and three properties fall out for free: the gating (a disabled binding simply is not offered, so the sameEnabled()that decides the footer decides the palette), the wording (the label is the footer's own description, so the two surfaces cannot drift), and the behavior (runreplays the key throughrouteKey, so there is no duplicated handler to diverge). The replay rests on an invariant — every palette binding's first key round-trips throughkeyMsgFor— andTestEveryPaletteBindingIsReplayablewalks every screen and mode to make it load-bearing rather than lucky.Global actions are navigation only. Anything that changes state stays a screen action, where the footer teaches it and the palette repeats it. That line is what keeps the second source short and predictable; #140 is where it gets a considered exception.
Which meant extracting the screen-opening methods first. A
"Go to export"cannot justgoTo(screenExport)— it has to build the sub-model too, which was the key handler's job. Copying those bodies would have been exactly the second source of truth the derivation avoids, so sixopenX()methods moved intointernal/tui/open.go, in the shapeopenListBrowser()already had. Handler and palette action now call the same method.internal/fuzzyis 90 lines of our own rather than a dependency. Not NIH: the matched indices are part of the UI contract, because they decide which letters light up, and that means the match has to be optimal rather than greedy. Greedyrtagainstreport tablereturns[0, 5]and highlightsrepor[t]; the right answer is[0, 7],[r]eport [t]able. It is a small dynamic program with a deterministic tie-break, pure, and testable without aModel.The box is built line by line rather than through
th.Box. For an overlay the rendered width is not decoration — it is the numbercompositeuses to decide where the body resumes. The previous tranche already paid once for trusting a library's border arithmetic (table.Width()counting only column separators and then amputating the right border), so every line here isboxWcells wide by construction, with a test that checks it.?stays as the expanded footer. #59 named the?overlay among the compositor's intended clients, and #69 closed with a note saying promoting it later would be "a change to one function". We are not promoting it, and not because it costs anything: a help panel that covers the very screen you are asking about is worse than a footer that covers nothing. That decision is recorded in the spec as final rather than deferred, so it stops being an open question instead of becoming an issue nobody closes.replace()clears the overlay only when the screen actually changes. The spec's rule was "an overlay belongs to the screen it was raised over; if that screen is swapped out from under it, it dissolves" — and the plan implemented that as "clear in all fournav.gofunctions", which is broader than the rule. Five handlers callreplace(<the screen they are already on>)as a routine refresh; with the broad version, astatusesMsglanding from the fetchopenFiltershad just launched would close the palette and wipe the query with nothing having changed on screen. The review caught it, it contradicted the plan text, and the ruling was that the finding governs.On the tests
Three more tests were caught passing against the very bug they existed to catch — the same failure mode that produced four in the previous tranche. All three came from the plan's own text, which is to say from me:
TestPaletteHighlightSurvivesTruncationwas reasoned backwards. It picked a query whose matches all landed far past the truncation point, on the theory that this exercised the drop-the-indices branch. It does the opposite: indices far past the cut are never read, because the render loop only walks the truncated label. The one observable index is the ellipsis column. A reviewer proved it by mutatinghighlightto drop nothing at all and watching the whole suite stay green.if key.Matches(msg, keysFor(m).Palette) { return m.openPalette(), nil }fromUpdateleftgo test ./...completely green —ctrl+pwould have stopped opening the palette and nothing would have noticed. Worse, the test that saidt.Fatal("ctrl+p did not open the palette")never pressed ctrl+p.msg != nil, and both the demo commands and the real ones always return a non-niltea.Msg— the real one returns astatusesMsgwith empty cached entries, or aretryableErrMsg, on any network failure. Removing theif m.demoguard would have left the test green while the code dialed out with the fake token, breaking the exact no-I/O promise the test existed to protect.There is a fourth of the same family that was caught in the plan, before any code:
TestPaletteQueryAcceptsQwas prescribed as proof that the overlay check must sit aboveQuit, and it cannot be —paletteKeysleavesQuitunassigned, soqreaches the query whatever the order. The plan's own second guard neutralised the plan's own red-green step. It is now documented as a regression test, andTestPaletteCtrlPClosesRatherThanReopeningis the one with teeth, becausectrl+pmisplaced would reopen the palette and wipe the query.The final review ran 27 mutations; 24 were caught. The three that survived became three new tests. One of them was hiding a real defect-in-waiting: with
refreshPalette's cursor reset removed, moving the cursor low and then typing a character that shrinks the list leavesidxpast the end, andenterpanics onitems[idx]. The code was already correct — nothing tested it.Every test written against a specific bug on this branch was verified failing against that bug before being accepted, with the transcript attached, and the reviewers reproduced the load-bearing ones independently.
Verification
gofmt,go vet,staticcheck,go build,go test ./... -raceall clean.go.modgains exactly one line —charmbracelet/x/ansipromoted from indirect to direct, already in the module graph via lipgloss, so nothing new is downloaded andgo.sumis untouched. Demo-mode parity has its own test. All four new goldens and three frames of the regenerateddocs/demo.gifwere inspected by eye, not just diffed. NoCo-Authored-Bytrailer on any of the 13 commits.Also swept in: twelve British spellings across the repo, five of them introduced by this branch's own plan text.
Follow-ups filed
#140 (the palette's month/list/member switcher, split out of #71's goal line into v1.12), #141 (
footer.go'sclampWidthbuilds a style on the default renderer — pre-existing from #69, behaviorally inert, and invisible to the suite becauseTestMainpins the profile).🤖 Generated with Claude Code