feat(web): toggle comment mode with Alt+C - #6768
Conversation
|
Thanks @Diyoncrz18 — reusing the existing comment-mode path instead of introducing a parallel toggle is the right shape here. Since this is opened as a draft, we'll hold off on code review until you mark it ready for review.
|
|
Thanks! The implementation and focused regression coverage are complete, so I’ve marked the PR as ready for review. I’ll address any feedback that comes up during the full review. |
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we’ll loop QA in once it’s merge-ready (and product/design have signed off where applicable). |
PerishCode
left a comment
There was a problem hiding this comment.
@Diyoncrz18 This adds the Alt+C toggle by routing through the existing Comment Mode action, while preserving active-viewer ownership, preview-mode eligibility, editable and modal input safety, and the asynchronous Manual Edit flush path. I reviewed both changed ranges and the focused regression matrix covering toggle behavior, analytics parity, retained viewers, ignored events, listener cleanup, and save ordering. Nicely scoped work, and thank you for documenting the validation and platform considerations so clearly.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
xxiaoxiong
left a comment
There was a problem hiding this comment.
LGTM — clean keyboard shortcut handler for alt+C comment-mode toggle.
Verified:
- New
isCommentShortcutOwnedTargetcorrectly bails when the active element is itself an editable target (INPUT, TEXTAREA, SELECT, contentEditable) — so typing alt+C inside a comment composer doesn't steal the keystroke and toggle the tool off. modalOwnsKeyboardInputchecks forrole="dialog"[aria-modal=true]orrole="alertdialog"[aria-modal=true]— comment shortcut is suppressed while any modal is open to keep alt+C from conflicting with whatever the modal is doing.- The event filter is precise: only
code === 'KeyC'withaltKeyand WITHOUTctrlKey/metaKey/shiftKey, skippingevent.repeatand IMEisComposing. Idle composition events won't toggle the tool. activateCommentToolRefkeeps a live reference to the latestactivateCommentToolso the effect depends only on a small set of layout/presentation flags, not the function identity.- Tool button update:
data-tooltipandtitlenow include(Alt+C)and the button gainsaria-keyshortcuts="Alt+C". - New 400+ line test file
FileViewer.comment-shortcut.test.tsxcovers: pure alt+C activates the tool; alt+C inside an[contenteditable]does not; combination with ctrl/shift/meta does not; modal-present suppresses; non-preview mode suppresses; inTabPresent/presentFullscreenPending suppress. Looks well-guarded.
|
Thank you both for the thorough review and approval! I appreciate you verifying the shortcut ownership, editable and modal safeguards, Manual Edit save ordering, and regression coverage. I’ll remain available if anything else is needed before merge. |
There was a problem hiding this comment.
LGTM — Alt+C routes through the existing activateCommentTool(), so toolbar parity, comment-draft cleanup, preview freeze, Manual Edit flush ordering and analytics all stay intact. Guards for editable/composer targets, modals, source mode, IME, repeat and modifier combos are precise, and the regression matrix covers them. CI is green.
Two non-blocking follow-ups:
- The hint is hardcoded as
`${t('fileViewer.comment')} (Alt+C)`. On macOS users expect⌥C, and the suffix bypasses i18n so it stays English in all locales. Worth moving into the translation key with a platform-aware label. - Option+C emits
çon macOS. Editable and composer surfaces are correctly exempted, so this should be safe, but a quick check on a real Mac keyboard before this ships would be good.
Closes #6365
Why
The issue reporter wants a faster way to enter and leave preview annotation mode while reviewing an artifact. Today, Comment Mode is available only through the preview toolbar, so repeated annotation work requires moving back to the toolbar between interactions.
This PR adds the requested Alt+C keyboard path without creating a second mode or a parallel state transition. The shortcut delegates to the existing
activateCommentTool()action, preserving the toolbar's board-mode toggle, comment-draft cleanup, preview freeze behavior, Manual Edit flush/exit sequence, and analytics event.What users will see
While an active, comment-capable HTML/site preview is in Preview mode:
Comment (Alt+C), and the button exposesaria-keyshortcuts="Alt+C".The shortcut is ignored for inactive/retained viewers, source mode, modal/presentation ownership, already-consumed events, IME composition, repeated keydown events, Ctrl/Meta/Shift combinations, and input, textarea, select, contenteditable, chat-composer, or comment-composer targets.
preventDefault()andstopPropagation()run only when the shortcut is actually consumed.If Manual Edit is active, Alt+C uses the existing asynchronous flush/exit path and does not activate Comment Mode until the pending save succeeds. Draw, Inspect, Present, and other preview tools are unchanged.
Changed files:
apps/web/src/components/FileViewer.tsxapps/web/tests/components/FileViewer.comment-shortcut.test.tsxSurface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-packflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)Screenshots
Not attached in this draft. The preview layout and Comment Mode visuals are unchanged; the only persistent UI change is the existing Comment tooltip now including
Alt+C. Tooltip text and accessible shortcut metadata are covered by the focused DOM regression test. I did not have a reliably seeded interactive desktop project available for a meaningful before/after capture.Bug fix verification
apps/web/tests/components/FileViewer.comment-shortcut.test.tsxmain: yes — 4 of the initial 7 scenarios failed because Alt+C did not toggle or consume the event and the toolbar exposed no shortcut hint.Validation
corepack pnpm exec vitest run -c vitest.config.ts --maxWorkers=2 tests/components/FileViewer.comment-shortcut.test.tsx(fromapps/web) — passed, 8/8 tests.corepack pnpm exec vitest run -c vitest.config.ts --maxWorkers=2 tests/components/FileViewer.test.tsx tests/components/FileViewer.manual-edit-history.test.tsx tests/components/FileViewer.board-mode-content-update.test.tsx(fromapps/web) — passed, 273/273 tests.corepack pnpm --filter @open-design/web typecheck— passed.pnpm typecheckwith a temporary Corepack shim first on PATH so nested workspace scripts use the repository-pinned pnpm 10.33.2 — passed for all workspace packages andscripts/tsconfig.json.git diff --cached --check— passed.corepack pnpm guard— attempted; all checks completed, but the command exited 1 on the existing packaged-leaf topology assertion:ci.yml no longer contains the guarded tools-dev, packaged unit, and focused E2E command block. This branch does not change.github/or guard code.corepack pnpm --filter @open-design/web test— attempted. The run passed the new shortcut test but was stopped after an extended Windows/jsdom canvas hang. Before the hang it reported unrelated failures insidecar-proxy.test.ts,sketch-help-modal.test.ts,chat-disclosure-accessibility.test.ts,design-system-modal-layer.test.ts,settings-polish.test.ts,ComposerPlusMenu.test.tsx, andfilter-pill.test.ts. The focused FileViewer suites above completed cleanly.Manual/platform notes: