fix(desktop): Cmd+O firing open-in twice on v1 workspace route#3511
Conversation
OPEN_IN_APP was registered both in OpenInMenuButton and in the v1 workspace page, so pressing Cmd+O ran two separate openInApp mutations (one per component's own useMutation instance) while clicking the button only ran one. Remove the page-level registration so the shortcut goes through the same handler as the click.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe hotkey registration for Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a double-invocation bug on the v1 workspace route where pressing Cmd+O (the The fix removes the redundant page-level hotkey registration while keeping Key changes:
Confidence Score: 5/5Safe to merge — single-line removal of a duplicate hotkey registration with no functional regressions The change is a minimal, targeted removal of one useHotkey call that was proven to cause a double-fire. The retained code is still needed for WorkspaceLayout props. The v2 route is untouched. No logic is rewritten; no new code is introduced. Risk of regression is essentially zero. No files require special attention
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx | Removed duplicate useHotkey("OPEN_IN_APP", handleOpenInApp) registration; handleOpenInApp / resolvedDefaultApp correctly retained for WorkspaceLayout props — fix is minimal and targeted |
Sequence Diagram
sequenceDiagram
participant User
participant ReactHotkeys as react-hotkeys-hook
participant OpenInMenuButton
participant PageComponent as workspace/page.tsx (removed)
participant Mutation as electronTrpc.external.openInApp
Note over ReactHotkeys,PageComponent: BEFORE fix — Cmd+O fires both registered handlers
User->>ReactHotkeys: Press Cmd+O
ReactHotkeys->>OpenInMenuButton: handleOpenInEditor()
OpenInMenuButton->>Mutation: mutate({ path, app, projectId })
Mutation-->>OpenInMenuButton: success → invalidate getDefaultApp
ReactHotkeys->>PageComponent: handleOpenInApp() [DUPLICATE]
PageComponent->>Mutation: mutate({ path, app, projectId }) [RACE]
Mutation-->>PageComponent: success → invalidate getDefaultApp
Note over ReactHotkeys,PageComponent: AFTER fix — Cmd+O fires only OpenInMenuButton handler
User->>ReactHotkeys: Press Cmd+O
ReactHotkeys->>OpenInMenuButton: handleOpenInEditor()
OpenInMenuButton->>Mutation: mutate({ path, app, projectId })
Mutation-->>OpenInMenuButton: success → invalidate getDefaultApp
Reviews (1): Last reviewed commit: "fix(desktop): Cmd+O firing open-in twice..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…set-sh#3511) OPEN_IN_APP was registered both in OpenInMenuButton and in the v1 workspace page, so pressing Cmd+O ran two separate openInApp mutations (one per component's own useMutation instance) while clicking the button only ran one. Remove the page-level registration so the shortcut goes through the same handler as the click.
FORK NOTE: upstream superset-sh#3511 removed the page-level OPEN_IN_APP hotkey on the v1 workspace route to avoid firing Cmd+O twice with OpenInMenuButton. The fork renders no TopBar in tearoff windows (layout.tsx gates it on !isTearoff), so removing the page-level registration killed Cmd+O inside tearoff windows entirely. Guard the registration with isTearoffWindow() so the main window still gets the upstream double-fire fix while tearoff windows retain the shortcut.
upstream取り込み: Cmd+O 二重発火修正 (superset-sh#3511)
Upstream commits processed (cherry-picked, then adapted where needed): - 07c1ee0 fix(desktop): Cmd+O firing open-in twice on v1 workspace route (superset-sh#3511) → PR #302 (with fork tearoff-window adaptation for Cmd+O) - 4a1f41a chore(deps): upgrade tanstack/db + electric, drop durable-streams patch (superset-sh#3509) → PR #303 (fork keeps fstream patch) - a3df489 feat(desktop): v2 PR checkout via widened checkout procedure (superset-sh#3525) → PR #304 (clean) - c504a50 feat(desktop): v2 file editor — foundation, views, and stability pass (superset-sh#3526) → PR #310 (foundation: 58 files path-checkout) → PR #311 (adaptation: 20 files manual port with SpreadsheetViewer/memo/fork-hotkeys preserved) - 78b7dc8 feat(desktop): promote "Create Section Below" to top-level on workspace menu (superset-sh#3537) → PR #308 Record merge so upstream/main..main shows 0 behind.
Summary
OPEN_IN_APPwas registered in two places on the v1 workspace route —OpenInMenuButtonand the page component — each with its ownelectronTrpc.external.openInApp.useMutation()instance.react-hotkeys-hookfires every registered listener, so Cmd+O triggered two mutations while clicking the button triggered one, causing the external app to be opened twice (and racingprojects.getDefaultAppinvalidation).useHotkey("OPEN_IN_APP", handleOpenInApp)inworkspace/$workspaceId/page.tsxso the shortcut is handled only byOpenInMenuButton, matching the click path exactly.resolvedDefaultApp/handleOpenInAppstay in the page becauseWorkspaceLayoutstill uses them (defaultExternalApp,onOpenInApp) for EmptyTabView etc.Test plan
onOpenInAppfrom the page).Summary by cubic
Fixes Cmd+O on the v1 workspace route opening the external app twice by removing the duplicate
OPEN_IN_APPhotkey listener so the shortcut uses the same handler as the Open In button.useHotkey("OPEN_IN_APP", handleOpenInApp)fromworkspace/$workspaceId/page.tsx;OpenInMenuButtonnow exclusively handles the shortcut.resolvedDefaultApp/onOpenInAppon the page forWorkspaceLayout/EmptyTabView; v2 workspace route unchanged.Written for commit 753fdf2. Summary will update on new commits.
Summary by CodeRabbit