Skip to content

fix(shortcuts): match ⌘⌥ shortcuts by layout-aware key, not physical position - #71722

Closed
haacked wants to merge 2 commits into
masterfrom
posthog-code/fix-shortcut-physical-key-layout
Closed

fix(shortcuts): match ⌘⌥ shortcuts by layout-aware key, not physical position#71722
haacked wants to merge 2 commits into
masterfrom
posthog-code/fix-shortcut-physical-key-layout

Conversation

@haacked

@haacked haacked commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Problem

On non-QWERTY keyboard layouts (Dvorak, AZERTY, etc.), ⌘⌥ (command+option) shortcuts fired the wrong action because the shortcut engine resolved the pressed letter from the physical key position (event.code) instead of the layout-aware event.key. On Dvorak the key labelled "C" sits where QWERTY has "I", so a ⌘⌥C press was matched as if it were ⌘⌥I — landing the user on the wrong destination.

Since every app shortcut uses command+option (baseModifier), this affected all of them on non-QWERTY layouts.

Changes

shortcutLogic now prefers event.key (correct on every layout) when matching the ⌘⌥ chord. It only consults the physical event.code for the macOS quirk the branch was originally written for — where holding Option turns event.key into a special glyph or dead key (e.g. ⌥K → "˚") — and never when event.key already gives a usable letter or digit. This brings the engine in line with the older useKeyboardHotkeys hook, which already matches on event.key.

How did you test this code?

Added shortcutLogic.test.ts with two cases:

  • Layout-aware matching — a Dvorak-style event (event.key: 'c', event.code: 'KeyI') triggers the ⌘⌥C shortcut and not ⌘⌥I. This fails if the engine reverts to matching on event.code.
  • macOS Option-glyph fallback — an event whose event.key is the glyph "˚" with event.code: 'KeyK' still triggers ⌘⌥K. This fails if the physical-key fallback is dropped, which would break Option shortcuts on macOS.

No existing test covered shortcutLogic, so neither regression was previously caught. I was unable to run the JS test suite in this environment (no node_modules/toolchain available) — CI will execute the tests.

Docs update

No docs change — internal keyboard-handling behavior only.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Authored by the PostHog Slack app from a Slack thread. Traced the regression: the current shortcut engine (shortcutLogic, introduced in #41677) reimplemented keyboard handling from scratch and reintroduced physical-key (event.code) matching in its Option branch, whereas the older useKeyboardHotkeys hook it sits alongside matches on the layout-aware event.key. Fix keeps the macOS Option-dead-key fallback (the branch's original purpose) but gates it so it only triggers when event.key isn't already a usable letter/digit.

Skill invoked: /writing-tests (to gate the added regression tests).


Created with PostHog from a Slack thread

…position

The keyboard shortcut engine resolved the letter of a ⌘⌥ (command+option)
chord from event.code (physical key position) whenever Option/Alt was held.
Since every app shortcut uses command+option, this broke every ⌘⌥ shortcut on
non-QWERTY layouts (Dvorak, AZERTY, etc.): the physical key produces a
different letter than the one printed on the key.

Prefer the layout-aware event.key, which is already correct on every layout.
Only fall back to the physical event.code for the macOS quirk it was written
for — where holding Option turns event.key into a special glyph or dead key
(e.g. ⌥K becomes "˚") — and never when event.key already gives a usable
letter or digit.

The older useKeyboardHotkeys hook already matches on event.key; this brings
the newer engine in line.

Generated-By: PostHog Code
Task-Id: 570b6a35-794b-45bb-933f-7b54751b014a
@github-actions

Copy link
Copy Markdown
Contributor

Hey @haacked! 👋

It looks like your git author email on this PR isn't your @posthog.com address (haacked@gmail.com). Since you're on the PostHog team, it's worth pointing your local git author email at your @posthog.com address. Why it matters:

  • Consistent work identity in git history — internal tooling that attributes commits to team members keys off your @posthog.com address.
  • Keeps team contributions easy to tell apart from external community ones when scanning history.

You can fix it for this repo with:

git config user.email "you@posthog.com"

Or set it globally with git config --global user.email "you@posthog.com". No need to redo this PR — just a nudge for next time. 🙂

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

Bundle size — 🟢 -19 B (-0.0%)

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 64.82 MiB · 🟢 -19 B (-0.0%)

No file changed by more than 1000 B.

Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report

Eager graph — within budget

How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.

Root Eager (shipped) Δ vs base Budget
entry (logged-out pages, app bootstrap)
src/index.tsx
1.22 MiB · 22 files no change ███░░░░░░░ 28.4% of 4.29 MiB
authenticated shell (every logged-in page)
src/scenes/AuthenticatedShell.tsx
8.14 MiB · 2,987 files 🟢 -19 B (-0.0%) █████████░ 88.0% of 9.25 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx
Size File
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
24.6 KiB ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js
6.3 KiB ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js
4.5 KiB ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js
3.9 KiB ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js
1.4 KiB ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js
1.3 KiB src/RootErrorBoundary.tsx
912 B ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
789 B src/scenes/ChunkLoadErrorBoundary.tsx
762 B src/index.tsx
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
Size File
281.3 KiB ../node_modules/.pnpm/posthog-js@1.403.0/node_modules/posthog-js/dist/rrweb.js
267.7 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
235.5 KiB src/taxonomy/core-filter-definitions-by-group.json
223.0 KiB ../node_modules/.pnpm/posthog-js@1.403.0/node_modules/posthog-js/dist/module.js
164.0 KiB src/queries/validators.js
154.3 KiB ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
105.8 KiB src/lib/api.ts
93.3 KiB ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js
92.7 KiB ../packages/quill/packages/quill/dist/index.js

Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479

Dist folder size — 🔺 +1.8 KiB (+0.0%)

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 1327.22 MiB · 🔺 +1.8 KiB (+0.0%)

The partial lib/posthog-typed mock replaced the real posthog-js re-export,
which broke the common logics initKeaTests mounts, failing both tests in
setup. shortcutLogic is standalone, so skip common-logic mounting
(initKeaTests(false)) and drop the posthog mock; keep only the isMac mock so
the modifier deterministically resolves to command.

Generated-By: PostHog Code
Task-Id: 570b6a35-794b-45bb-933f-7b54751b014a
@trunk-io

trunk-io Bot commented Jul 16, 2026

Copy link
Copy Markdown

Static BadgeStatic BadgeStatic BadgeStatic Badge

Failed Test Failure Summary Logs
shortcutLogic falls back to the physical key only when Option turns event.key into a non-letter glyph A mock function was expected to be called once but was not called during the test. Logs ↗︎
shortcutLogic matches ⌘⌥ shortcuts by the layout-aware key, not the physical key position A mock function was expected to be called once but was not called during the test. Logs ↗︎

View Full Report ↗︎Docs

haacked commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Closing — this was opened against the wrong repo. The reported bug (Dvorak Cmd+C navigating to the inbox) is in PostHog/code, where the Inbox shortcut is mod+i matched by react-hotkeys-hook's physical-key (event.code) logic. The real fix is PostHog/code#3525. The change here was an incidental, unrelated hardening of this app's own shortcut engine and isn't needed.

@haacked haacked closed this Jul 16, 2026
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.

1 participant