fix: map Option+P to Meta+P escape sequence on macOS (#1359)#1900
fix: map Option+P to Meta+P escape sequence on macOS (#1359)#1900IkramBagban wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughAdds an opt-in "Option as Meta" behavior: new setting (DB + API + UI), wiring into terminal lifecycle, a keyboard handler path that sends ESC+letter for Option+letter on macOS when enabled, tests for the handler, and related constants and migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings UI
participant API as TRPC Settings Router
participant DB as Local DB
participant Terminal as Terminal Component
participant Handler as Keyboard Handler
User->>UI: Toggle "Use Option as Meta key"
UI->>API: setOptionAsMetaKey({ enabled })
API->>DB: upsert settings.option_as_meta_key
API-->>UI: { success }
Note right of UI: UI invalidates/fetches option state
UI->>API: getOptionAsMetaKey()
API->>DB: read settings.option_as_meta_key
API-->>UI: optionAsMetaKey
UI->>Terminal: pass useOptionAsMetaKey (true/false)
Note right of Terminal: lifecycle provides isOptionAsMetaKeyEnabled
User->>Terminal: Press Option+P (macOS)
Terminal->>Handler: setupKeyboardHandler receives event
alt isOptionAsMetaKeyEnabled == true
Handler->>Terminal: write ESC + "p"
else
Handler-->>Terminal: no write (allow default)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.test.ts (1)
182-193: Strengthen the test by asserting event suppression (falsereturn).The behavior depends on both writing
\x1bpand suppressing xterm processing; asserting the return value makes this regression-proof.✅ Suggested test tweak
- captured.handler?.({ + const handled = captured.handler?.({ type: "keydown", key: "π", code: "KeyP", altKey: true, metaKey: false, ctrlKey: false, shiftKey: false, } as KeyboardEvent); + expect(handled).toBe(false); expect(onWrite).toHaveBeenCalledWith("\x1bp");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.test.ts` around lines 182 - 193, The test should also assert that the keyboard event handler suppressed xterm processing by returning false: call captured.handler with the constructed KeyboardEvent (as is already done) and capture its return value, then add an expectation that this value is false in addition to the existing expect(onWrite).toHaveBeenCalledWith("\x1bp");; reference the captured.handler invocation and the onWrite mock in the test to locate where to add the expect(returnValue).toBe(false) assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.test.ts`:
- Around line 182-193: The test should also assert that the keyboard event
handler suppressed xterm processing by returning false: call captured.handler
with the constructed KeyboardEvent (as is already done) and capture its return
value, then add an expectation that this value is false in addition to the
existing expect(onWrite).toHaveBeenCalledWith("\x1bp");; reference the
captured.handler invocation and the onWrite mock in the test to locate where to
add the expect(returnValue).toBe(false) assertion.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
|
Thanks for the fix! This correctly solves the Option+P → Meta+P mapping for Claude Code's model picker, and follows the existing pattern for Option+Left/Right. However, we think this should be a more general solution rather than a per-key mapping. Every major terminal (iTerm2, VS Code, Alacritty, Warp) handles this with a global "Use Option as Meta key" setting that maps all Option+letter combos to Would you be open to implementing a global "Option as Meta" behavior setting instead? It would:
The existing Option+Left/Right mappings could stay as unconditional defaults (since those are universally expected for word navigation), but everything else would be gated behind the setting — that way users who need Option for special characters (like |
That makes sense, a global “Option as Meta” setting would definitely be the more scalable approach instead of handling individual keys. I’ll take a look at implementing it so that Option+letter combinations map to ESC + key when the setting is enabled, while keeping the current default behavior for users who rely on Option for special characters. The existing Option+Left/Right mappings can remain as they are. I’ll work on this when I get some time and update the PR once I have a draft ready. Thanks for the suggestion! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
662-705: Indentation inconsistency in router procedures.The procedures from
getOpenLinksInAppthroughgetDefaultEditor(lines 662-705) have an extra level of indentation compared to the preceding procedures (e.g.,setWorktreeBaseDirat line 647). This appears to be a formatting issue.🔧 Suggested fix to align indentation
The procedures should use the same indentation level as the rest of the router methods (one tab/indent from the
return router({block).- getOpenLinksInApp: publicProcedure.query(() => { - const row = getSettings(); - return row.openLinksInApp ?? DEFAULT_OPEN_LINKS_IN_APP; - }), + getOpenLinksInApp: publicProcedure.query(() => { + const row = getSettings(); + return row.openLinksInApp ?? DEFAULT_OPEN_LINKS_IN_APP; + }),Apply similarly to
setOpenLinksInApp,getOptionAsMetaKey,setOptionAsMetaKey, andgetDefaultEditor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/settings/index.ts` around lines 662 - 705, The listed router procedures (getOpenLinksInApp, setOpenLinksInApp, getOptionAsMetaKey, setOptionAsMetaKey, getDefaultEditor) are indented one extra level compared to other router methods; fix by aligning their indentation to the same level as surrounding procedures (the same single indent from the return router({ block) so their declarations and chained calls (.input/.mutation/.query) match the formatting of setWorktreeBaseDir and other methods.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
346-360: Indentation inconsistency in useTerminalLifecycle call.Lines 346-360 have extra indentation compared to the preceding arguments (lines 310-345). This appears to be a formatting issue introduced during the change.
🔧 Suggested fix
resetModes, - isAlternateScreenRef, - isBracketedPasteRef, - setPaneNameRef, - renameUnnamedWorkspaceRef, - handleTerminalFocusRef, - registerClearCallbackRef, - unregisterClearCallbackRef, - registerScrollToBottomCallbackRef, - unregisterScrollToBottomCallbackRef, - registerGetSelectionCallbackRef, - unregisterGetSelectionCallbackRef, - registerPasteCallbackRef, - unregisterPasteCallbackRef, - useOptionAsMetaKey: optionAsMetaKey ?? DEFAULT_OPTION_AS_META_KEY, - }); + isAlternateScreenRef, + isBracketedPasteRef, + setPaneNameRef, + renameUnnamedWorkspaceRef, + handleTerminalFocusRef, + registerClearCallbackRef, + unregisterClearCallbackRef, + registerScrollToBottomCallbackRef, + unregisterScrollToBottomCallbackRef, + registerGetSelectionCallbackRef, + unregisterGetSelectionCallbackRef, + registerPasteCallbackRef, + unregisterPasteCallbackRef, + useOptionAsMetaKey: optionAsMetaKey ?? DEFAULT_OPTION_AS_META_KEY, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx` around lines 346 - 360, The call to useTerminalLifecycle has inconsistent indentation for the later arguments (isAlternateScreenRef, isBracketedPasteRef, setPaneNameRef, renameUnnamedWorkspaceRef, handleTerminalFocusRef, registerClearCallbackRef, unregisterClearCallbackRef, registerScrollToBottomCallbackRef, unregisterScrollToBottomCallbackRef, registerGetSelectionCallbackRef, unregisterGetSelectionCallbackRef, registerPasteCallbackRef, unregisterPasteCallbackRef, useOptionAsMetaKey) causing a formatting mismatch; fix by aligning these argument lines with the preceding argument list (same indentation level as the earlier props passed into useTerminalLifecycle) so the entire object argument block is consistently indented and visually matches the arguments on lines 310–345.apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts (1)
65-70: Test placement within unrelated describe block.The new test for "option meta" search is placed inside the
"settings search - font settings"describe block, but it's unrelated to font settings. Consider moving it to a separate describe block for better organization.🧹 Suggested reorganization
it("font items have correct section", () => { // ... existing test }); +}); - it('searching "option meta" returns the Option as Meta key setting', () => { +describe("settings search - behavior settings", () => { + it('searching "option meta" returns the Option as Meta key setting', () => { const results = searchSettings("option meta"); const ids = getIds(results); expect(ids).toContain(SETTING_ITEM_ID.BEHAVIOR_OPTION_AS_META_KEY); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts` around lines 65 - 70, The test asserting that searchSettings("option meta") returns SETTING_ITEM_ID.BEHAVIOR_OPTION_AS_META_KEY is currently placed inside the "settings search - font settings" describe; move that it() block out of that describe into a new or existing describe that matches behavior/option settings (e.g., a "settings search - behavior settings" or a general "settings search" block) so the test context aligns with the subject; ensure the test still references searchSettings and getIds and keep the same expectation for SETTING_ITEM_ID.BEHAVIOR_OPTION_AS_META_KEY.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (1)
495-500: Indentation inconsistency in setupKeyboardHandler call.The
setupKeyboardHandlercall has extra indentation compared to surrounding code.🔧 Suggested fix
- const cleanupKeyboard = setupKeyboardHandler(xterm, { - onShiftEnter: () => handleWrite("\x1b\r"), - onClear: handleClear, - onWrite: handleWrite, - isOptionAsMetaKeyEnabled: () => useOptionAsMetaKeyRef.current, - }); + const cleanupKeyboard = setupKeyboardHandler(xterm, { + onShiftEnter: () => handleWrite("\x1b\r"), + onClear: handleClear, + onWrite: handleWrite, + isOptionAsMetaKeyEnabled: () => useOptionAsMetaKeyRef.current, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts` around lines 495 - 500, The call to setupKeyboardHandler is mis-indented relative to surrounding code; adjust the indentation of the entire setupKeyboardHandler(...) block so it aligns with adjacent statements (same level as the const cleanupKeyboard assignment and other nearby calls), ensuring the object argument lines (onShiftEnter, onClear, onWrite, isOptionAsMetaKeyEnabled) are indented one level inside the parentheses and remain aligned; locate the const cleanupKeyboard assignment that references xterm, handleWrite, handleClear and useOptionAsMetaKeyRef and normalize its indentation to match the file's style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx`:
- Around line 67-69: Replace the direct navigator.platform check in
BehaviorSettings.tsx with the canonical Electron tRPC platform query: remove the
navigator-based isMac calculation and instead call
electronTrpc.window.getPlatform.useQuery() (importing electronTrpc) inside the
component, read the returned platform string (or its loading/default state) and
derive isMac by checking platform.toLowerCase().includes("mac"); ensure you use
the query's data safely (fallback while loading) and update any references to
the old isMac variable to use the new derived value.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 662-705: The listed router procedures (getOpenLinksInApp,
setOpenLinksInApp, getOptionAsMetaKey, setOptionAsMetaKey, getDefaultEditor) are
indented one extra level compared to other router methods; fix by aligning their
indentation to the same level as surrounding procedures (the same single indent
from the return router({ block) so their declarations and chained calls
(.input/.mutation/.query) match the formatting of setWorktreeBaseDir and other
methods.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.ts`:
- Around line 65-70: The test asserting that searchSettings("option meta")
returns SETTING_ITEM_ID.BEHAVIOR_OPTION_AS_META_KEY is currently placed inside
the "settings search - font settings" describe; move that it() block out of that
describe into a new or existing describe that matches behavior/option settings
(e.g., a "settings search - behavior settings" or a general "settings search"
block) so the test context aligns with the subject; ensure the test still
references searchSettings and getIds and keep the same expectation for
SETTING_ITEM_ID.BEHAVIOR_OPTION_AS_META_KEY.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 495-500: The call to setupKeyboardHandler is mis-indented relative
to surrounding code; adjust the indentation of the entire
setupKeyboardHandler(...) block so it aligns with adjacent statements (same
level as the const cleanupKeyboard assignment and other nearby calls), ensuring
the object argument lines (onShiftEnter, onClear, onWrite,
isOptionAsMetaKeyEnabled) are indented one level inside the parentheses and
remain aligned; locate the const cleanupKeyboard assignment that references
xterm, handleWrite, handleClear and useOptionAsMetaKeyRef and normalize its
indentation to match the file's style.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx`:
- Around line 346-360: The call to useTerminalLifecycle has inconsistent
indentation for the later arguments (isAlternateScreenRef, isBracketedPasteRef,
setPaneNameRef, renameUnnamedWorkspaceRef, handleTerminalFocusRef,
registerClearCallbackRef, unregisterClearCallbackRef,
registerScrollToBottomCallbackRef, unregisterScrollToBottomCallbackRef,
registerGetSelectionCallbackRef, unregisterGetSelectionCallbackRef,
registerPasteCallbackRef, unregisterPasteCallbackRef, useOptionAsMetaKey)
causing a formatting mismatch; fix by aligning these argument lines with the
preceding argument list (same indentation level as the earlier props passed into
useTerminalLifecycle) so the entire object argument block is consistently
indented and visually matches the arguments on lines 310–345.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0cfafd1-e224-4e3c-a1d2-a9c8c28b5245
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.test.tsapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/shared/constants.tspackages/local-db/drizzle/0035_add_option_as_meta_key_setting.sqlpackages/local-db/drizzle/meta/0035_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
✅ Files skipped from review due to trivial changes (2)
- packages/local-db/drizzle/0035_add_option_as_meta_key_setting.sql
- packages/local-db/drizzle/meta/0035_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
| const isMac = | ||
| typeof navigator !== "undefined" && | ||
| navigator.platform.toLowerCase().includes("mac"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current platform detection usage in settings routes.
# Expected result: `BehaviorSettings.tsx` should align with `getPlatform.useQuery` usage
# and avoid direct `navigator.platform` checks.
rg -nP --type=tsx 'navigator\.platform|getPlatform\.useQuery' apps/desktop/src/renderer/routes/_authenticated/settings -C2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for platform detection patterns without type restriction
echo "=== Platform detection patterns in settings routes ==="
rg -n 'navigator\.platform|getPlatform' apps/desktop/src/renderer/routes/_authenticated/settings --max-count=20
echo ""
echo "=== Checking getPlatform implementation ==="
fd 'trpc' apps/desktop/src --type f | head -5
echo ""
echo "=== Search for getPlatform definition ==="
rg -n 'getPlatform' apps/desktop/src/lib --max-count=20Repository: superset-sh/superset
Length of output: 1060
🏁 Script executed:
#!/bin/bash
# Get the full context of BehaviorSettings.tsx around the flagged lines
echo "=== BehaviorSettings.tsx around lines 67-69 ==="
sed -n '60,75p' apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx
echo ""
echo "=== BehaviorSettings.tsx around lines 509-513 ==="
sed -n '502,520p' apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxRepository: superset-sh/superset
Length of output: 1175
🏁 Script executed:
#!/bin/bash
# Check layout.tsx to understand the referenced pattern
echo "=== Layout.tsx platform detection pattern ==="
sed -n '70,85p' apps/desktop/src/renderer/routes/_authenticated/settings/layout.tsxRepository: superset-sh/superset
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Verify getPlatform implementation - check what it returns for macOS
echo "=== getPlatform implementation ==="
sed -n '40,50p' apps/desktop/src/lib/trpc/routers/window.ts
echo ""
echo "=== Check all platform-related checks in BehaviorSettings.tsx ==="
rg -n 'isMac|platform|navigator' apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxRepository: superset-sh/superset
Length of output: 496
🏁 Script executed:
#!/bin/bash
# Check the imports and see if electronTrpc is already imported in BehaviorSettings.tsx
echo "=== BehaviorSettings.tsx imports section ==="
head -30 apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxRepository: superset-sh/superset
Length of output: 1048
Replace navigator.platform detection with canonical tRPC platform source.
BehaviorSettings.tsx uses navigator.platform for platform detection while other settings routes (layout.tsx, GeneralSettings.tsx) use the canonical electronTrpc.window.getPlatform.useQuery() pattern. This violates the coding guideline: "For Electron interprocess communication, ALWAYS use tRPC as defined in src/lib/trpc".
♻️ Proposed diff
- const isMac =
- typeof navigator !== "undefined" &&
- navigator.platform.toLowerCase().includes("mac");
+ const { data: platform } = electronTrpc.window.getPlatform.useQuery();
+ const isMac = platform === undefined || platform === "darwin";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isMac = | |
| typeof navigator !== "undefined" && | |
| navigator.platform.toLowerCase().includes("mac"); | |
| const { data: platform } = electronTrpc.window.getPlatform.useQuery(); | |
| const isMac = platform === undefined || platform === "darwin"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx`
around lines 67 - 69, Replace the direct navigator.platform check in
BehaviorSettings.tsx with the canonical Electron tRPC platform query: remove the
navigator-based isMac calculation and instead call
electronTrpc.window.getPlatform.useQuery() (importing electronTrpc) inside the
component, read the returned platform string (or its loading/default state) and
derive isMac by checking platform.toLowerCase().includes("mac"); ensure you use
the query's data safely (fallback while loading) and update any references to
the old isMac variable to use the new derived value.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 13 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
3 participants
Description
This PR fixes an issue where the Alt+P (Option+P) keyboard shortcut on macOS was outputting the special character π instead of triggering CLI actions. Specifically, this was breaking the model picker shortcut in Claude Code.
The fix involves intercepting the physical KeyP code combined with the altKey modifier in the xterm.js custom key event handler and manually sending the expected \x1bp (Meta+P) escape sequence.
Related Issues
Closes #1359
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit