Skip to content

fix(desktop): make ⌘K open the composer link editor for selections#1644

Merged
wesbillman merged 1 commit into
block:mainfrom
AaronGoldsmith:fix/cmd-k-link-shortcut
Jul 8, 2026
Merged

fix(desktop): make ⌘K open the composer link editor for selections#1644
wesbillman merged 1 commit into
block:mainfrom
AaronGoldsmith:fix/cmd-k-link-shortcut

Conversation

@AaronGoldsmith

Copy link
Copy Markdown
Contributor

Problem

The formatting toolbar's link button advertises ⌘K in its tooltip, and the keyboard-shortcuts settings panel lists "Insert link ⌘K" — but the binding was never implemented. The window-level quick-search handler in AppShell owned ⌘K unconditionally, so pressing it with composer text selected opened the channel search dialog instead of the add-link modal.

(A code comment even said "Media paste + ⌘K link shortcut via Tiptap editorProps" — but only the paste half was ever written.)

Fix

Bind ⌘K in the editor's raw ProseMirror handleKeyDown hook (the same proven layer as the ArrowUp edit-last-message handling) and consume it only when it applies:

State ⌘K behavior
Composer text selected Opens Add link dialog seeded with the selection
Caret inside an existing link Opens Edit link dialog seeded with that link
Empty caret in composer Falls through to quick search (unchanged)
Unselected drafted text (caret only) Falls through to quick search (unchanged)
Focus outside composer Quick search (unchanged)
Plain Ctrl+K on macOS Emacs kill-line (unchanged)

The conditional fall-through deliberately preserves the app-wide ⌘K quick-search muscle memory — the shortcut only claims the key when there's something to link.

Changes

  • useRichTextEditor.ts — new onLinkShortcut option handled in editorProps.handleKeyDown, with event.repeat / event.isComposing guards. hasPrimaryShortcutModifier still rejects Ctrl on macOS, so the macEmacsTextShortcuts Ctrl-K kill-line binding is untouched.
  • useLinkEditor.tsx — new openFromShortcut(): opens the dialog only when a selection or caret-adjacent link exists, returns whether it consumed the shortcut, and dismisses the link hover card first.
  • MessageComposer.tsx + ForumComposer.tsx — wired via the existing onEditLinkRef pattern (both useRichTextEditor consumers covered).
  • AppShell.tsx — global shortcut handler now bails on event.defaultPrevented (composer wins when it claims the key) and on event.repeat (held ⌘K can't stack quick-search over the link dialog).
  • keyboard-shortcuts.tsformat-link description updated to match actual behavior.
  • composer-link-shortcut.spec.ts — new Playwright spec covering all six states in the table above, registered in the smoke project.

Testing

  • ✅ 6/6 new e2e tests pass (including the mac Ctrl+K kill-line regression guard)
  • ✅ Full smoke regression sweep (smoke / spoiler / messaging / navigation): 68 passed
  • ✅ Unit tests: 2079 pass · tsc clean · biome clean (6 pre-existing warnings, unchanged)
  • ✅ Two independent adversarial review passes — both APPROVE WITH NITS; all nits addressed (repeat/IME guards, kill-line regression test, shortcut description accuracy, comment accuracy, handler simplification)

Mechanism notes (for reviewers)

ProseMirror attaches its keydown listener directly to the editor element; when handleKeyDown returns true, ProseMirror calls event.preventDefault() and the event still bubbles to the window with defaultPrevented === true, which the AppShell listener now respects. When the handler returns false the event bubbles untouched and quick search opens exactly as before.

The formatting toolbar's link button has always advertised ⌘K, and the
shortcuts panel lists 'Insert link ⌘K' — but the binding was never
implemented. The window-level quick-search handler in AppShell owned ⌘K
unconditionally, so pressing it with composer text selected opened the
search dialog instead of the add-link modal.

Bind ⌘K in the editor's raw ProseMirror handleKeyDown hook (the same
layer as the ArrowUp edit-last-message handling) and consume it only
when it applies: text is selected, or the caret sits inside an existing
link. A bare caret still falls through to quick search, preserving the
app-wide ⌘K muscle memory. AppShell's global handler now respects
event.defaultPrevented so the composer wins when it claims the key.

- useRichTextEditor: new onLinkShortcut option, handled in
  editorProps.handleKeyDown with repeat/IME-composition guards; plain
  Ctrl+K on macOS is still rejected by hasPrimaryShortcutModifier, so
  the Emacs kill-line binding is untouched
- useLinkEditor: new openFromShortcut() — opens the dialog only when a
  selection or caret-adjacent link exists, returns whether it consumed
  the shortcut, and dismisses the hover card first
- MessageComposer + ForumComposer: wired via the existing ref pattern
- AppShell: skip shortcut handling for defaultPrevented and repeat events
- keyboard-shortcuts: format-link description now matches the behavior
- e2e: new composer-link-shortcut.spec.ts covering all six states
  (selection → add dialog, caret-in-link → edit dialog, empty caret →
  search, unselected draft → search, mac Ctrl+K kill-line regression,
  focus outside composer → search)

Co-authored-by: Aaron Goldsmith <aarong@squareup.com>
Signed-off-by: Aaron Goldsmith <aarong@squareup.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve — reviewed on behalf of Wes (verified locally at head aba9752).

Verification: checked out the PR head and ran all 24 search-related smoke e2e tests (every existing ⌘K quick-search flow) plus the 6 new composer-link-shortcut specs — all green; tsc clean.

Why the ⌘K change is safe: the editor consumes the key only when openFromShortcut() finds a selection or caret-on-link; otherwise it returns false, no preventDefault(), and the event bubbles to AppShell's quick-search listener exactly as before. ⌘⇧K (new DM) and macOS Ctrl+K kill-line verified untouched. The selection→link / no-selection→palette discriminator matches GitHub/Notion/Linear precedent, and it's the behavior the toolbar tooltip and shortcuts panel have advertised all along.

One note: the main CI workflow was stuck at action_required at review time — get the full suite green on record before merging.

@wesbillman wesbillman merged commit 3e982fd into block:main Jul 8, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants