fix(tui): wire up features that were built but never reachable - #490
Merged
Conversation
Several TUI capabilities existed in the tree but were never actually connected. This connects them and adds tests pinning each contract. - Ctrl+L (documented in KEYBINDINGS.md, never implemented): now forces a full repaint — invalidates the layout cache and clears the terminal. - Kitty keyboard protocol was detected and reported but the enhancement flags were never pushed, so the Ctrl+Enter / Shift+Enter disambiguation the UI advertises was never requested. Now pushed last on entry and popped first on exit, in the restore path and the panic hook, balanced so we never pop flags we did not push. Deliberately skipped on browser-engine terminal widgets embedded in code editors, which mis-encode shifted printables under this protocol. - Desktop notifications: NotifierService was implemented and tested but never constructed. The TUI now notifies when attention is required and the terminal is unfocused (permission/plan/question modal opening, turn completion), dispatched off the event loop since the backends probe the environment synchronously. - NO_COLOR mapped to 16-colour output — i.e. still colour, contrary to no-color.org. Adds a real monochrome mode that drops foreground and background while preserving bold/dim/italic/underline, honours CLICOLOR=0, and lets FORCE_COLOR override detection upward. - Entering the TUI on a non-TTY died inside enable_raw_mode with a raw errno. It now fails early naming the offending stream and pointing at --prompt / --output-format json. Note: a suspected `/tasks` shadowing bug was investigated and does not exist — the toggle matches the bare form exactly, so arguments already reach the command bridge. Added a regression test pinning both halves.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The Windows console rejects PushKeyboardEnhancementFlags outright (the sequence is a Unix terminal concept), so the pushed flag correctly stays false there — but the test assumed the push always succeeds and failed on windows-latest. Assert the real invariant instead: we record a push exactly when bytes were emitted, and only ever pop what we pushed. Both branches are checked, so the balance guarantee is covered on every platform rather than only where the protocol exists.
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.
Summary
A TUI audit turned up several capabilities that exist in the tree but are never actually connected — code that is written, sometimes tested, and unreachable at runtime. This wires them up and pins each contract with tests.
What was dead, and what it does now
Ctrl+LKEYBINDINGS.md, no handler existedPushKeyboardEnhancementFlagswas never called anywhere in the repo — so theCtrl+Enter/Shift+Enterdisambiguation the UI advertises was never requestedNotifierServicefully implemented and tested, never constructedNO_COLORCLICOLOR=0;FORCE_COLORcan override upwardenable_raw_mode()with a raw errno--prompt/--output-format jsonNotes on the trickier two
Kitty protocol is deliberately not enabled on browser-engine terminal widgets embedded in code editors — they mis-encode shifted printables under this protocol. The decision is a pure function (
keyboard_enhancement_allowed) so it's unit-tested rather than inferred at runtime. Push/pop ordering matches the existing teardown discipline exactly (pushed last, popped first, including in the panic hook) — a leaked keyboard mode wrecks the user's shell.Notifications are dispatched off the event loop via
spawn_blocking: the Linux/Windows backends do a synchronouswhich/where.exeprobe and the macOS focus probe shells out, so dispatching inline would stall rendering. The gate uses the terminal's own focus-change reporting rather than the service's probe, which always returnsfalseon Linux/Windows.A suspected bug that turned out not to exist
The audit flagged
/tasksas shadowed by the pane toggle, making the background-task CLI unreachable. Investigated: false. The line in question is a skill-shadow skip list (stopping a user skill namedtasksfrom hijacking the command); the toggle matches bare/tasksexactly, so/tasks output 3already falls through to the command bridge. No code change — but a regression test now pins both halves so a future edit can't introduce the bug that was suspected.Verification
clippy --all-targets -- -D warningsclean,fmt --checkclean.NO_COLOR/CLICOLOR/FORCE_COLORprecedence, mono attribute preservation, per-slot palette stripping, notifier decision logic, and the/tasksrouting contract.no_color_overrides_apple_terminalnow expects mono instead of 16-colour — that is the fix.