fix(ui): honor Alt/Cmd+Backspace and take the keyboard while opening - #350
Draft
forketyfork wants to merge 5 commits into
Draft
fix(ui): honor Alt/Cmd+Backspace and take the keyboard while opening#350forketyfork wants to merge 5 commits into
forketyfork wants to merge 5 commits into
Conversation
Issue: There was no logging anywhere in the resize-settle hold, so there was no way to tell from the logs why a terminal entered the sweeping animation after a dimension change, how long it held, or why it released. Solution: Emit debug lines when the hold starts (with old and new cell dimensions), when a repaint wave re-arms it mid- or post-sweep, and when it releases. The release line carries a classified reason derived from the exit branches of resizeSettleHoldActive, so quiet settling, hitting the hard cap while still streaming, and a foreground process that never reacted to the SIGWINCH are distinguishable in the log.
Issue: In the recent-folders overlay Alt+Backspace and Cmd+Backspace only deleted a single character instead of a word or the whole query, and the first key pressed after Cmd+O was lost because the overlay did not accept input until its expand animation finished. Solution: The backspace handler ignored modifiers entirely, so all three variants took the same path; both input gates tested state == .Open, and during the ~200 ms .Expanding window the event fell through UiRoot into the focused terminal. Backspace semantics now live in ui/text_edit.zig (Cmd clears, Alt deletes a word, plain deletes one UTF-8 codepoint) and every text input in the app uses it, replacing four divergent implementations. The open-or-opening test moves onto ExpandingOverlay.State as isOpenOrOpening() and is applied to every overlay that gated on .Open, so keys are no longer dropped into the terminal during an expand; startCollapsing now reverses from the overlay's current size so an interrupted expand does not snap to full size first.
Issue: New tests added for the recent-folders fixes passed without ever running. Zig only collects tests from files it actually analyzes, and src/main.zig keeps an explicit registry for that reason, but three files declaring tests were missing from it, including gfx/shimmer.zig, whose tests had never run. Solution: A deliberately failing canary test reported as green, and the test-name table in the compiled binary confirmed the omission. Registering the missing files takes the suite from 283 to 302 tests. Because transitive reachability depends on which decls Sema happens to analyze, the rule is now that every file with tests is listed explicitly, enforced by scripts/check-test-registry.sh from just lint, which CI already runs.
There was a problem hiding this comment.
Pull request overview
This PR fixes input handling bugs in UI overlays (Backspace modifier semantics and keyboard capture during the opening animation), adds diagnostics for the resize-settle lifecycle, and closes a Zig test-discovery gap by explicitly registering test-bearing files and enforcing that via lint.
Changes:
- Introduces
src/ui/text_edit.zigas the shared, tested implementation of macOS-style Backspace behavior (plain/⌥/⌘) and wires multiple overlays to it. - Standardizes “overlay owns keyboard while expanding” by adding
ExpandingOverlay.State.isOpenOrOpening()and updating event gates/toggles accordingly (including smoother mid-expand collapse). - Adds resize-settle debug logging + release-reason classification, and enforces explicit Zig test registration via
scripts/check-test-registry.sh+just lint.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ui/text_edit.zig | New shared helper for consistent Backspace semantics (UTF-8 safe) with tests. |
| src/ui/components/worktree_overlay.zig | Uses shared backspace logic; fixes overlay key gating during open/expanding. |
| src/ui/components/story_overlay.zig | Delegates backspace handling to shared helper for search field. |
| src/ui/components/recent_folders_overlay.zig | Fixes modifier-aware backspace + keyboard capture during expand; adds tests. |
| src/ui/components/reader_overlay.zig | Delegates backspace handling to shared helper for search field. |
| src/ui/components/pill_group.zig | Uses isOpenOrOpening() to collapse other overlays consistently. |
| src/ui/components/help_overlay.zig | Uses isOpenOrOpening() for toggle gating. |
| src/ui/components/expanding_overlay.zig | Adds isOpenOrOpening() and fixes interrupted collapse geometry; adds tests. |
| src/ui/components/diff_overlay.zig | Delegates backspace handling to shared helper for comment editor. |
| src/session/state.zig | Adds resize-settle release-reason classifier + debug logs; adds tests. |
| src/main.zig | Registers additional test-bearing files to ensure tests actually run. |
| src/app/layout.zig | Adds debug log for resize-settle hold start with old/new dimensions. |
| scripts/check-test-registry.sh | New lint check to enforce explicit test registration (see comments). |
| README.md | Documents immediate keyboard capture and consistent Backspace semantics. |
| justfile | Runs test-registry check during just lint. |
| docs/perf-debugging.md | Documents new resize-settle debug tracing and meanings. |
| docs/development.md | Documents Zig test discovery/registration requirement. |
| docs/ARCHITECTURE.md | Documents the new ui/text_edit.zig module and overlay gating convention. |
| CLAUDE.md | Updates contributor guidance to match new conventions/checks. |
Suppressed comments (1)
scripts/check-test-registry.sh:37
- Same portability issue as above: this summary line also uses GNU grep’s
--include, which will break on BSD grep. Reuse the samefind ... -exec grep ...pipeline here as well.
echo "test registry: all $(grep -rl '^test ' src --include='*.zig' | wc -l | tr -d ' ') files with tests are reachable"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue: The search fields had no visible caret, Cmd+A did nothing, and Cmd+C/Cmd+V were not handled anywhere, so text could not be copied into or out of any field — including pasting characters that cannot be typed. Solution: text_edit grows a TextInput that owns the buffer plus the state a focused field needs (caret blink phase, select-all flag) and handles the macOS editing keys: Backspace, Cmd+A, Cmd+C, Cmd+V. All five fields store their text in one, so behavior can no longer drift between them. The model is append-only: the caret sits at the end and the only selection is everything, which is what these single-purpose fields need and keeps the per-field rendering to a caret plus one highlight band. Fields are configured with a separator set, a length cap and a character filter, so pasting multi-line text into a one-line box drops the newlines and pasting into the worktree name field drops characters it would reject when typed. Reader and story overlays now request frames while their search is open, without which the caret cannot blink under idle throttling.
forketyfork
marked this pull request as draft
August 5, 2026 07:11
forketyfork
marked this pull request as ready for review
August 5, 2026 09:55
Issue: A long pasted line in the search bar rendered past the bar's edges, and emoji pasted into a field or shown in reader/story mode were drawn enormously larger than the surrounding text. Solution: Both come from rendering a text texture at whatever size SDL_ttf returns. Apple Color Emoji is a non-scalable bitmap font with a single 160 px strike, measured here: at 14 pt it reports height 210, "hello" renders 30x17 but "hi <emoji>" renders 174x160, and TTF_SetFontSize reports success while changing nothing. The terminal already avoids this by scaling every glyph into its cell; UI text had no equivalent. The new text_render module does the same job for a line: it splits the string into emoji and text runs, renders each with the font that owns it, scales the emoji runs to the text line height and composes them into one surface. Its drawClippedTail draws a line inside a fixed box, tail-aligned so the caret end stays visible, and fades the leading edge into the field's own fill instead of cutting mid-glyph. The search bar now renders its prefix, query and match count separately so the query can be clipped to the space between them.
forketyfork
marked this pull request as draft
August 5, 2026 12:10
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.
Two reported bugs in the recent-folders overlay, the shared cleanup they pointed at, and a test-discovery gap found while verifying the fixes. No tracking issue existed for any of these; linkage can be added during cleanup if issues are filed retroactively.
Solution
1. Resize-settle debug logging (
ec9ef56)Nothing in the resize-settle lifecycle was logged, so there was no way to tell from
architect.logwhy a terminal entered the sweeping animation, how long the hold lasted, or why it ended. Threelog.debuglines now cover it:The release reason comes from a new
ResizeSettleReleaseenum whose classifier mirrors the exit branches ofresizeSettleHoldActive, soquiet(agent settled on its own),max_duration(hit the hard cap while still streaming),no_response(never reacted to the SIGWINCH) andsession_goneare distinguishable. Requires[logging].min_level = "debug".2. Recent-folders input fixes (
7315020)⌥/⌘Backspace did nothing special. The handler at
recent_folders_overlay.zigmatchedSDLK_BACKSPACEwithout reading modifiers at all, so every variant deleted one byte.The first key after ⌘O was lost.
startExpandingsets.Expandingfor the ~200 ms animation, but both theSDL_EVENT_TEXT_INPUTandSDL_EVENT_KEY_DOWNgates testedstate == .Open. The event fell throughUiRoottoruntime.zig:1803and was typed into the focused terminal — not merely dropped.Rather than patch the two call sites, both patterns became shared:
src/ui/text_edit.zigis now the single source of truth for backspace semantics (DeleteScope,scopeFromMods,backspace): ⌘ clears, ⌥ deletes a word, plain deletes one UTF-8 codepoint. A survey found four text inputs with four different behaviors; all now use it.ExpandingOverlay.State.isOpenOrOpening()replacesstate == .Openwherever the question is "does this overlay own the keyboard / is it visible". The worktree overlay was dropping ⌘T and ⌘1–9 into the terminal during its own expand;pill_group.zighad the same comparison spelled out six times.startCollapsingalso now reverses from the overlay's current size instead of snapping to full size first, which the mid-expand close path would otherwise have made visible.3. Test registry gap (
bab75d6)The new tests passed without running. Zig only collects tests from files it analyzes;
src/main.zigkeeps an explicit registry for exactly that reason, and three files were missing from it — includinggfx/shimmer.zig, whose tests had never run. Confirmed with a deliberately failing canary that the suite reported green, then by inspecting the test-name table in the compiled binary.Registering them takes the suite from 283 to 302 tests. Since transitive reachability depends on which decls Sema happens to analyze, the rule is now "list every file with tests, explicitly", enforced by
scripts/check-test-registry.shinjust lint(already run by CI).4. Text-field editing across the app (
90096f7)Follow-up from testing the branch: the search fields had no visible caret, ⌘A did nothing, and ⌘C/⌘V were unhandled — so text could not be pasted into a field at all (which is also why the emoji step in the test plan below was untestable).
text_editnow exposes aTextInputthat owns the buffer plus the state a focused field needs (caret blink phase, select-all flag) and handles the macOS editing keys. All five fields store their text in one, so behavior can no longer drift between them:The model is append-only by design (agreed with the author before implementing): the caret sits at the end and the only selection is "everything". Caret movement, partial selection and click-to-position are not included and would be a separate change.
⌘1–9 quick-select in the recent-folders picker is checked before the field's own ⌘ shortcuts, so it keeps winning.
Review feedback
Copilot flagged
grep --includeinscripts/check-test-registry.shas GNU-only. Verified and replied inline: macOS shipsBSD grep, GNU compatible 2.6.0-FreeBSD, which supports--include, and CI's Linux runner is GNU grep — so neither environment breaks. The duplicated-work half of the observation was real and is fixed: the count is now a loop counter instead of a second recursive grep.5. Field overflow and emoji sizing (
1017708)Two more issues found while testing: a long pasted line rendered past the search bar's edges, and emoji were drawn enormously larger than the text around them — in fields, and also in reader mode and stories.
Both come from drawing a text texture at whatever size SDL_ttf hands back. Measured on this machine at 14 pt:
TTF_FontIsScalable(Apple Color Emoji)false— one fixed 160 px bitmap strikeTTF_GetFontHeightat ptsize 14"hello""hi 🙂"TTF_SetFontSize(emoji, 14)true, changes nothingSo a single emoji in a string makes the whole line ~9× the text height. The terminal never hit this because
font.zigscales every rendered glyph into its cell; UI text had no equivalent.The new
src/ui/text_render.zigdoes the same job per line: split the string into emoji and text runs, render each with the font that owns it, scale the emoji runs to the text line height, compose into one surface.drawClippedTaildraws a line inside a fixed box — tail-aligned so the caret end stays visible — and fades the leading edge into the field's own fill rather than cutting mid-glyph. The search bar now renders its prefix, query and match count as separate pieces so the query can be clipped to the space between them.Adopted by the search bars, recent-folder entries, reader and story document text, and diff comments. Static labels (buttons, badges, "Reader Mode") keep the cheap single-render path.
Verification
Both bugs are covered by tests confirmed to fail before the fix and pass after — the backspace test reports
expected "dev/github/" found "dev/github/architec"and the expand-animation test fails outright on the old gates. Same for the interrupted-collapse geometry (expected 220, found 400).zig buildclean ·zig build test322 + 3 passed ·zig fmt --checkclean ·just lint"No issues found".TextInputis covered by unit tests (blink cycle, ⌘A replace-on-type, Backspace scopes with and without a selection, modifier gating so plain letters and ⌃/⌥ combos fall through,max_lenand the character filter) and the recent-folders overlay has component-level tests driving real SDL events throughhandleEvent.text_renderis covered by unit tests for run splitting (ZWJ sequences, keycaps, skin tones, emoji at both ends) and emoji scaling, plus a test that opens the real Apple Color Emoji font and asserts a composed line comes back at the text line height instead of 160 px. That test was confirmed to fail when the scaling is removed (expected 17, found 160).Not covered by automated tests: clipboard round-trips (⌘C/⌘V call into SDL, which needs a window), caret / selection / fade rendering, and the reader/story/diff adoptions of the shared field — those are verified by build and inspection only.
Test plan
[logging].min_level = "debug", toggle grid/full on a live codex session and confirm~/Library/Logs/Architect/architect.logshows the hold start → re-arm → release lines with a sensible reason.