feat(interaction): wave 3 — one hint registry, and a badge for vi - #66
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8f2f3fc4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| surface := m.surface() | ||
| m.hintBar.SetWidth(m.width) | ||
| m.hintBar.SetHints(m.hintsForMode()) | ||
| m.hintBar.SetHints(m.hintsFor(surface)) |
There was a problem hiding this comment.
Refresh surface chrome when the active surface changes
The resolved surface is copied into the hint bar and composer badge only when refreshChrome runs, but common transitions such as Tab/h/l focus changes, opening or closing contacts/help/palette/dialogs, and Esc entering vi command mode do not refresh or relayout. The immediately rendered frame therefore keeps the previous surface's hints and badge until the next one-second chromeTick; notably, the new VI badge does not appear immediately after Esc. Refresh these values during update reconciliation or whenever one of the resolver inputs changes.
Useful? React with 👍 / 👎.
| case in.focus == PanelChatView: | ||
| return SurfaceChatView | ||
| default: | ||
| return SurfaceChatList |
There was a problem hiding this comment.
Model panel-local text inputs as distinct surfaces
When / opens the chat-list filter or in-chat find input, resolution still depends only on panel focus because surfaceInputs contains neither chatList.FilterActive() nor chatView.SearchActive(). app.go routes every subsequent keypress exclusively to those text inputs, yet the resolver continues returning SurfaceChatList or SurfaceChatView, so after every refresh the bar advertises inert browsing actions and the badge says NORMAL even though printable keys type. Add these input states to surface resolution with insert-mode hint sets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Found one user-visible hint regression.
[P2] Do not advertise y/n on every dialog
internal/app/hints.go:318 hardcodes y/n: answer for SurfaceDialog, but the delete dialog accepts n/m/e. On that dialog the frame hint bar therefore advertises y even though y is inert, while the dialog's own dynamically generated row correctly says n/m/e. That is both misleading on a destructive surface and contrary to this wave's single-source-of-truth goal.
A focused delete-dialog test fails because hintsFor(SurfaceDialog) contains y/n. Please derive the answer accelerators from the active dialog, or omit the generic answer-letter hint from the frame and leave the dialog's button-derived line authoritative.
The existing full test suite passes; the focused regression test fails.
d45b539 to
305fb59
Compare
Review finding on #66. hintsFor(SurfaceDialog) hardcoded "y/n answer", which is right for a two-button confirm and wrong for the delete choice: that one answers to n/m/e, so the bar advertised an inert y on the one surface where a wrong key press deletes a message. The dialog's own line already said n/m/e, so the wave that exists to remove exactly this kind of disagreement shipped one. The letters are read off the live dialog now (dialog.Accelerators, the exported form of the button-set walk the dialog's own line already used). Both rows are renderings of the one button set rather than two copies of it, and with no dialog up there are no letters to name — hint() drops the row instead of leaving a blank. The allow-list entry that excused the composite "y/n" token is gone, and the drift test's own staleness check is what caught it going stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d8f2f3f to
9bea119
Compare
|
Fixed in
Two tests: Worth noting what caught the leftovers: the #67 carried the documentation half of this — |
305fb59 to
721aa14
Compare
Review finding on #66. hintsFor(SurfaceDialog) hardcoded "y/n answer", which is right for a two-button confirm and wrong for the delete choice: that one answers to n/m/e, so the bar advertised an inert y on the one surface where a wrong key press deletes a message. The dialog's own line already said n/m/e, so the wave that exists to remove exactly this kind of disagreement shipped one. The letters are read off the live dialog now (dialog.Accelerators, the exported form of the button-set walk the dialog's own line already used). Both rows are renderings of the one button set rather than two copies of it, and with no dialog up there are no letters to name — hint() drops the row instead of leaving a blank. The allow-list entry that excused the composite "y/n" token is gone, and the drift test's own staleness check is what caught it going stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9bea119 to
aba77df
Compare
3af11a4 to
516aa1b
Compare
Wave 3 of INTERACTION-REVIEW.md. Every hint the client draws comes from one table keyed by surface, and the composer's command state stops borrowing the browsing panels' badge. I-6 — hints are keyed by surface, from one table. The bar was keyed by MODE, which is a much coarser question than "what can I press": three surfaces share NORMAL and agree on almost no keys, so the chat-view set showed in the chat list, under contacts, under a confirm dialog and in a vi composer. internal/app/hints.go adds a Surface enum, a pure resolver with the same precedence Update dispatches in, and hintsFor(Surface) built from the resolved bindings. hintsForMode and helpLine — a correct per-focus generator that nothing rendered — are deleted. The surfaces that paint their own row are fed from the same registry rather than holding literals: the chat list footer (SetFooterHints), the media overlay and the reaction row, whose "takes yours off" wording depends on state a literal could not see. The footer is where this was found: it advertised "u unread" for a release with nothing bound to u, because a literal cannot be wrong in a way anything can detect. Two drift tests hold it. TestHintSurfacesMatchHelp walks every surface and refuses a hint naming a key the help card does not; the handful of composite spellings it cannot compare are listed with reasons, and a stale entry fails too. TestNoHintLiteralsInComponents reads the three component sources for the literals that were there. I-12 — a fourth badge, VI. InteractionMode gains ModeVi and the composer AppVi, mauve. The mode is now derived FROM the surface rather than resolved a second time beside it, which is what makes the badge and the bar unable to disagree. `:` opens the palette from NORMAL and from VI — vim's own muscle memory; the backtick stays NORMAL only. The badge column is padded to six cells rather than the seven the decision named, and the deviation is recorded as an amendment in docs/interaction-model.md: NORMAL and INSERT are six, so six is the width the column already had wherever it can change under the reader's eyes, and padding to seven would shift every existing frame by one cell — a geometry change, which the goldens exist to refuse. Fixtures regenerated for the hint rows only: the diffs are text on two rows, every row still exactly its stated width. The README's frame excerpt follows them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding on #66. hintsFor(SurfaceDialog) hardcoded "y/n answer", which is right for a two-button confirm and wrong for the delete choice: that one answers to n/m/e, so the bar advertised an inert y on the one surface where a wrong key press deletes a message. The dialog's own line already said n/m/e, so the wave that exists to remove exactly this kind of disagreement shipped one. The letters are read off the live dialog now (dialog.Accelerators, the exported form of the button-set walk the dialog's own line already used). Both rows are renderings of the one button set rather than two copies of it, and with no dialog up there are no letters to name — hint() drops the row instead of leaving a blank. The allow-list entry that excused the composite "y/n" token is gone, and the drift test's own staleness check is what caught it going stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aba77df to
4a45f69
Compare
Wave 3 of
INTERACTION-REVIEW.md, stacked on #65. Every hint the client draws comes from one table keyed by surface, and the composer's command state stops borrowing the browsing panels' badge.Base is
feat/interaction-wave-2— review #64 and #65 first.VI12 — one hint table, keyed by surface (I-6)
The bar was keyed by mode, and a mode is a much coarser question — "does the next printable key type or act?" — than "what can I press". Three surfaces share
ModeNormaland agree on almost no keys, so the chat-view set showed in the chat list, under contacts, under a confirm dialog and in a vi composer: four places where it named inert keys.New
internal/app/hints.go: aSurfaceenum, a pureresolveSurfacewith the same precedenceUpdatedispatches in, andhintsFor(Surface)built from the resolved bindings — the sets areinteraction-model.md's "Hints" table.hintsForModeandhelpLine(a correct per-focus generator that nothing rendered) are deleted.The surfaces that paint their own row are fed from the same registry rather than holding literals: the chat list footer, the media overlay, and the reaction row — whose "takes yours off" wording depends on state a literal could not see, which is why
Mine()is now exposed. The media overlay and the reaction row are fed where they open as well as on the chrome tick: a full-screen overlay whose row only says how to leave from the next tick onward is a trap for that long.Two drift tests hold it.
TestHintSurfacesMatchHelpwalks every surface and refuses a hint naming a key the help card does not; the handful of composite spellings it cannot compare (j/k,y/n,←/→,[/], …) are listed with reasons, and a stale entry fails too.TestNoHintLiteralsInComponentsreads the three component sources for the literals that were there, the wayTestAppFixedMatchesDispatcherreadsapp.go.13 — the fourth badge,
VI(I-12)InteractionModegainsModeVi, the composerAppVi(mauve). The mode is now derived from the surface rather than resolved a second time beside it, which is what makes the badge and the bar unable to disagree —resolveMode/modeInputsare gone, replaced byresolveSurface/surfaceInputsandSurface.Mode().:opens the palette fromNORMALand fromVI; the backtick staysNORMALonly.?still does not open help from a vi composer — the badge describes key routing rather than changing it.One recorded deviation. The badge column is padded to six cells, not the seven the decision named.
NORMALandINSERTare six, so six is the width the column already had wherever it can change under the reader's eyes; padding to seven would fix it atCOMMAND's width and shift every existing frame by one cell — a geometry change, which the goldens exist to refuse (TUI 2.0 decision 11). The amendment is written intodocs/interaction-model.mdunder I-12, as the work order requires, andTestTheBadgeColumnDoesNotMovepins the prompt against the badge changing.Fixtures
Regenerated for the hint rows only. The diffs are text on two rows and every row is still exactly its stated width — worth reading, because they are the visible half of this change:
The two rows now share their first three hints, because the footer draws "its first three or four hints from the same row of this table" as I-6 specifies and both are the chat list's surface. It is the decision working as written; if it reads badly to you on the real frame, the fix is a line in the model doc and a slice in
footerHints, not a literal back in the footer. The README's frame excerpt follows the fixture.Verification
go build ./...,go vet ./...,go test ./...green. New:TestEverySurfaceIsReachable(a surface nothing can produce is a hint set nobody sees),TestOverlaysOutrankFocus,TestResolveSurfacePrecedence/IsTotal,TestTheHintBarFollowsTheSurface(chat list, contacts, dialog and vi composer each get their own set),TestTheChatListFooterIsDerived(it follows a rebound key),TestSurfaceModes, and the three I-12 tests.Wave 4 branches off this one.
🤖 Generated with Claude Code