Repository navigation
fix(desktop): annotation tool keys work after clicking a toolbar button - #26211
juliusmarminge wants to merge 4 commits into
Conversation
…icking a toolbar button Adds a jsdom test project for the desktop package and a PickPreload test that starts an annotation, clicks the Region toolbar button, then presses V. On main the tool stays on Region because the window keydown handler ignores every key whose target is inside the overlay host. These tests reproduce #7700 and fail until it is fixed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The annotation overlay ignored every non-Escape key whose target was inside the overlay host, so V/R/D/E did nothing once a toolbar button held focus. Skip tool keys only when an input, textarea or select inside the overlay's shadow root has focus. Fixes #7700 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…each test A cold transform of the preload under heavy load exceeded the 15s test timeout when it ran inside the test body. Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — Restores annotation-tool keyboard shortcuts after toolbar clicks while preserving normal typing in editor controls. The accompanying test-project split and session-scoped behavior introduce no obvious material risk. You can add or adjust custom eligibility rules. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
juliusmarminge
added
the
bugfix
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shortcut focus handling and DOM tests apps/desktop/src/preview/PickPreload.ts, apps/desktop/src/preview/PickPreload.test.ts, apps/desktop/vite.config.ts, apps/desktop/package.json |
The key handler now checks whether an input, textarea, or select is active before ignoring tool shortcuts. Tests check shortcut behavior with a focused Region button and a focused comment textarea. Vitest adds a jsdom project for the DOM tests and jsdom as a development dependency. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- pingdotgg/t3code#23965: Changes the annotation key-delivery path after adding and focusing a PDF input layer.
- pingdotgg/t3code#24022: Changes annotation shortcut routing in
PickPreload.ts.
Suggested reviewers: maria-rcks, t3dotgg
Merge Risk: 
77870
-
No merge-blocking issue was identified in the shortcut change or its test setup.
Pre-merge checks: 
-
Check name Status Explanation Title check The title is concise, conventional, and clearly describes the main fix: annotation tool keyboard shortcuts work after toolbar focus. Description check The description covers the problem, reproduction steps, technical change, linked issue and approval context, focused verification, visual evidence, limitations, and agent details. Linked Issues check Directly linked issue #7700asks for reliable annotation-mode shortcuts (Escape, V, R, D, E) in apps/desktop. The reviewed change addresses the V/R/D/E failure. In apps/desktop/src/preview/PickPreload…Out of Scope Changes check All changed areas connect to the linked issue or to its test coverage. The PickPreload.ts change is the production fix for the keyboard guard. PickPreload.test.ts adds focused regression tests for the… Full details: Linked Issues check
-
Explanation
Directly linked issue
#7700asks for reliable annotation-mode shortcuts (Escape, V, R, D, E) in apps/desktop. The reviewed change addresses the V/R/D/E failure. In apps/desktop/src/preview/PickPreload.ts, the window keydown guard in startAnnotation no longer ignores every non-Escape key whose target is the overlay host. It now ignores tool keys only when shadowRoot.activeElement is an input, textarea, or select. Focus on a toolbar button therefore no longer blocks tool keys. Escape handling runs before this guard and is unchanged. The PR description reports Escape working from a toolbar button, a comment box, a text input and a select (R12), with reopening working. Text-field protection is kept (R05 to R09). The new test in apps/desktop/src/preview/PickPreload.test.ts, switches tools from the keyboard after a toolbar button is clicked, targets the reported failure. The second test, keeps the tool while typing a tool key in the comment, covers the text-field guard. Test infrastructure (jsdom devDependency and a dom Vitest project) supports these tests. Limits: the evidence is from Linux with Electron and synthetic input. Windows, the reporter's platform, was not checked. The reporter's Escape failure was not shown as a before/after pair. I did not run the test suite. The conclusion relies on the PR description, the change summary, and the stated test results.
Full details: Out of Scope Changes check
-
Explanation
All changed areas connect to the linked issue or to its test coverage. The PickPreload.ts change is the production fix for the keyboard guard. PickPreload.test.ts adds focused regression tests for the same behavior. apps/desktop/package.json adds jsdom as a devDependency. This is required to run the DOM test. apps/desktop/vite.config.ts splits Vitest into unit and dom projects so that the DOM test can run with jsdom. The PR states that existing desktop test files remain under the unit project, and the change summary confirms that unit tests keep their existing includes and setup files. The commit that moves preload loading to test collection addresses a test timeout in the new test. It does not alter application behavior. pnpm-lock.yaml is excluded from review by the ignored_files rule, so its contents were not inspected. Its change is limited to the jsdom dependency already described in package.json, and this does not affect the assessment. No unrelated production behavior change was identified in the reviewed summary.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Problem
In the desktop browser's annotation mode, the tool keys V, R, D and E stop working as soon as you click one of the toolbar buttons (Select, Region, Draw, Erase). Steps: open the browser panel, load any page, press Annotate, click Region, press V. The tool stays on Region, as in the reporter's recording.
Change
The overlay lives in a closed shadow root. Clicking a toolbar button leaves focus on that button, and at the window keydown listener every such keypress is retargeted to the overlay host. The handler skipped every non-Escape key whose target was the overlay, which was meant to protect text fields but also caught the buttons. It now skips tool keys only when the focused element inside the overlay (
shadowRoot.activeElement) is aninput,textareaorselect. Escape, submission keys and page isolation are unchanged.The test runs the real preload against a DOM.
apps/desktophad no DOM test setup and the repo's lint rule rejects per-file environment comments, soapps/desktop/vite.config.tsgains adomVitest project next to the existing suite (the patternapps/mobileuses), andjsdom(already in the lockfile for mobile) becomes a desktop devDependency.Scope and approval
Fixes #7700
Triaged via the automated issue flow.
Verification
Focused tests in
apps/desktop/src/preview/PickPreload.test.ts:switches tools from the keyboard after a toolbar button is clickedfails against the oldPickPreload.ts(expected 'marquee' to be 'select') and passes with the fix.keeps the tool while typing a tool key in the commentpasses, and fails if the text-field guard is removed.vp run check:changedpasses (format, lint, knip, typecheck, tests). The desktop suite keeps all its existing test files under theunitproject.Evidence
Real Linux desktop app: Electron 44.9.3 / Chromium 152.0.7977.167 under Xvfb, no emulation, app 1256×780, guest 1000×700, DPR 1, zoom and font scaling 100%, dark theme, a fresh database on each side, a synthetic fixture page. Input was trusted CDP mouse and key events. Before is 5808469018, after is 1667f6fbf1. R12 was recorded at 7787049cda, whose production change is the same.
What else changed:
Diff0 marks every nonzero RGB-channel difference in bright pink, so a one-level change is amplified to a visible pixel. DiffT12 marks pixels whose maximum absolute RGB-channel difference exceeds 12/255. Both use the full 1256×780 frame. Changed pixels (Diff0 / DiffT12): R01: 2052 / 1977, R02: 2081 / 2006, R03: 2002 / 1932, R04: 0 / 0.
R01 — Select button focused; v/r/d/e · guest 1000×700 · dark
Linux x86_64 (fs-06), real Electron 44.9.3 / Chromium 152.0.7977.167 under headless-desktop/Xvfb; no device emulation; full app 1256×780, guest 1000×700 CSS px, DPR 1, zoom 100%, font scaling 100%, dark theme. The 1256×860 recordings add an 80px narration band below the complete app surface.
Before: Select remains active for all four keys. After: Select → Region → Draw → Erase; Select retains keyboard focus.
Before steps (seconds from recording start): 1.43s Select button focus; trusted v/r/d/e → select; 1.88s Click Select → select; 2.67s Key v → select; 3.73s Key r → select; 4.79s Key d → select; 5.84s Key e → select.
After steps (seconds from recording start): 1.42s Select button focus; trusted v/r/d/e → select; 1.86s Click Select → select; 2.66s Key v → select; 3.71s Key r → marquee; 4.76s Key d → draw; 5.81s Key e → erase.
Read-only focus/tool telemetry: after-R01-events.json · before-R01-events.json
Contact sheets (derived from the recordings):
R02 — Region button focused; v/r/d/e · guest 1000×700 · dark
Linux x86_64 (fs-06), real Electron 44.9.3 / Chromium 152.0.7977.167 under headless-desktop/Xvfb; no device emulation; full app 1256×780, guest 1000×700 CSS px, DPR 1, zoom 100%, font scaling 100%, dark theme. The 1256×860 recordings add an 80px narration band below the complete app surface.
Before: Region remains active, including after v. After: Select → Region → Draw → Erase; Region retains keyboard focus.
Before steps (seconds from recording start): 1.42s Region button focus; trusted v/r/d/e → select; 1.90s Click Region → marquee; 2.71s Key v → marquee; 3.76s Key r → marquee; 4.82s Key d → marquee; 5.87s Key e → marquee.
After steps (seconds from recording start): 1.42s Region button focus; trusted v/r/d/e → select; 1.86s Click Region → marquee; 2.65s Key v → select; 3.70s Key r → marquee; 4.75s Key d → draw; 5.80s Key e → erase.
Read-only focus/tool telemetry: after-R02-events.json · before-R02-events.json
Contact sheets (derived from the recordings):
R03 — Draw button focused; v/r/d/e · guest 1000×700 · dark
Linux x86_64 (fs-06), real Electron 44.9.3 / Chromium 152.0.7977.167 under headless-desktop/Xvfb; no device emulation; full app 1256×780, guest 1000×700 CSS px, DPR 1, zoom 100%, font scaling 100%, dark theme. The 1256×860 recordings add an 80px narration band below the complete app surface.
Before: Draw remains active for all four keys. After: Select → Region → Draw → Erase; Draw retains keyboard focus.
Before steps (seconds from recording start): 1.42s Draw button focus; trusted v/r/d/e → select; 1.85s Click Draw → draw; 2.64s Key v → draw; 3.69s Key r → draw; 4.74s Key d → draw; 5.79s Key e → draw.
After steps (seconds from recording start): 1.42s Draw button focus; trusted v/r/d/e → select; 1.85s Click Draw → draw; 2.65s Key v → select; 3.70s Key r → marquee; 4.75s Key d → draw; 5.79s Key e → erase.
Read-only focus/tool telemetry: after-R03-events.json · before-R03-events.json
Contact sheets (derived from the recordings):
R04 — Erase button focused; v/r/d/e · guest 1000×700 · dark
Linux x86_64 (fs-06), real Electron 44.9.3 / Chromium 152.0.7977.167 under headless-desktop/Xvfb; no device emulation; full app 1256×780, guest 1000×700 CSS px, DPR 1, zoom 100%, font scaling 100%, dark theme. The 1256×860 recordings add an 80px narration band below the complete app surface.
Before: Erase remains active for all four keys. After: Select → Region → Draw → Erase. The identical final Erase frame does not replace the recording of intermediate transitions.
Before steps (seconds from recording start): 1.42s Erase button focus; trusted v/r/d/e → select; 1.86s Click Erase → erase; 2.66s Key v → erase; 3.70s Key r → erase; 4.75s Key d → erase; 5.79s Key e → erase.
After steps (seconds from recording start): 1.42s Erase button focus; trusted v/r/d/e → select; 1.86s Click Erase → erase; 2.66s Key v → select; 3.71s Key r → marquee; 4.76s Key d → draw; 5.81s Key e → erase.
Read-only focus/tool telemetry: after-R04-events.json · before-R04-events.json
Contact sheets (derived from the recordings):
After-only rows (recordings and contact sheets)
Matrix: fixture-matrix.md. Drivers and fixtures to rebuild any state: reproduction.md, reproduction-bundle.zip, r12-retake-3-drivers.zip. Full evidence index: evidence.md.
Not checked: Windows desktop (the reporter's platform) and macOS desktop. The changed code is platform-independent and the bug reproduced on Linux with the reporter's steps. Light theme and narrow viewports were not captured; the change does not touch layout.
Model and harness: Claude Opus 5.5 (lead, red test, implementation, review B) and GPT-6.1 Sol (review A, proof audits, captures), in Claude Code and Codex via T3 Code.
🤖 Generated with Claude Code