Repository navigation
fix(desktop): v2 Open In editor is per-project, not per-workspace - #3393
Conversation
The v2 top-bar "Open In <editor>" dropdown was storing the choice in the per-workspace localStorage collection, so switching between workspaces in the same project reset the picker. Move it to the per-project v2SidebarProjects collection so the choice is shared across every workspace in a project. Also drops the unused workspaceId prop and no longer passes projectId to openInApp, since persistence now happens entirely client-side on the react-db collection.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigrates the default "open in" preference from workspace-local React state to a per-project persisted field, renames component prop from Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant UI as "V2OpenInMenuButton"
participant Sidebar as "Sidebar Projects (DB)"
participant Ensurer as "ensureProjectInSidebar"
User->>UI: Trigger open-in (click/hotkey)
UI->>Sidebar: useLiveQuery(projectId) — read defaultOpenInApp
Sidebar-->>UI: return defaultOpenInApp (resolvedApp)
UI->>UI: compute currentApp / payload with resolvedApp
UI->>Sidebar: openInApp.mutate(...)
UI->>Ensurer: persistDefaultApp -> ensure project exists
Ensurer->>Sidebar: upsert defaultOpenInApp for projectId
Sidebar-->>UI: upsert success
UI-->>User: open-in action completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 scoping bug where the "Open In <editor>" picker in the v2 desktop top-bar was persisting its choice to the per-workspace Key changes:
The one noteworthy concern is the defensive Confidence Score: 4/5Safe to merge after reviewing the insert-fallback behaviour in persistDefaultApp; no data-loss or security risk. The core scoping fix is correct and well-implemented: reading via useLiveQuery from v2SidebarProjects keyed by projectId is the right reactive pattern, and removing the dead defaultOpenInApp field from workspaceLocalStateSchema is clean. The one P1 concern is the defensive insert in persistDefaultApp which can create a v2SidebarProjects entry with tabOrder: 0 for projects that have no existing sidebar record, potentially affecting sidebar ordering or blocking the sidebar's own insert later. If the team is confident this path is not exercised in normal usage, the PR is ready to merge; otherwise a small guard is warranted. apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx — the persistDefaultApp insert fallback (lines 57–64). Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant V2WorkspaceOpenInButton
participant v2Workspaces as v2Workspaces Collection
participant V2OpenInMenuButton
participant v2SidebarProjects as v2SidebarProjects Collection
participant ElectronTRPC as electronTrpc (IPC)
User->>V2WorkspaceOpenInButton: render (workspaceId prop)
V2WorkspaceOpenInButton->>v2Workspaces: useLiveQuery(workspaceId)
v2Workspaces-->>V2WorkspaceOpenInButton: { branch, projectId, hostMachineId }
V2WorkspaceOpenInButton->>V2OpenInMenuButton: render(branch, worktreePath, projectId)
V2OpenInMenuButton->>v2SidebarProjects: useLiveQuery(projectId)
v2SidebarProjects-->>V2OpenInMenuButton: { defaultOpenInApp } or []
Note over V2OpenInMenuButton: resolvedApp = defaultOpenInApp ?? "finder"
User->>V2OpenInMenuButton: clicks Open or picks app from dropdown
V2OpenInMenuButton->>ElectronTRPC: openInApp.mutate({ path, app })
ElectronTRPC-->>V2OpenInMenuButton: onSuccess(app)
alt project record exists
V2OpenInMenuButton->>v2SidebarProjects: .update(projectId, draft.defaultOpenInApp = app)
else no project record yet
V2OpenInMenuButton->>v2SidebarProjects: .insert({ projectId, defaultOpenInApp: app, tabOrder: 0 })
end
v2SidebarProjects-->>V2OpenInMenuButton: re-render via useLiveQuery
Note over V2OpenInMenuButton: picker now shows newly-chosen app
Reviews (1): Last reviewed commit: "fix(desktop): v2 Open In editor is per-p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const persistDefaultApp = useCallback( | ||
| (app: ExternalApp) => { | ||
| setDefaultApp(app); | ||
| collections.v2WorkspaceLocalState.update(workspaceId, (draft) => { | ||
| draft.defaultOpenInApp = app; | ||
| if (collections.v2SidebarProjects.get(projectId)) { | ||
| collections.v2SidebarProjects.update(projectId, (draft) => { | ||
| draft.defaultOpenInApp = app; | ||
| }); | ||
| return; | ||
| } | ||
| collections.v2SidebarProjects.insert({ | ||
| projectId, | ||
| createdAt: new Date(), | ||
| tabOrder: 0, | ||
| isCollapsed: false, | ||
| defaultOpenInApp: app, | ||
| }); | ||
| }, | ||
| [collections, workspaceId], | ||
| [collections, projectId], | ||
| ); |
There was a problem hiding this comment.
When no v2SidebarProjects record exists for projectId (e.g. the user navigated directly to a workspace URL without ever opening the project in the sidebar), the fallback insert creates a record with tabOrder: 0 and isCollapsed: false. Because the v2SidebarProjects collection is keyed by projectId, this synthetic entry will be visible to every component that reads the collection — including the sidebar renderer. Depending on how the sidebar orders and filters its project list, this could:
- Surface a "ghost" project row with
tabOrder: 0(likely sorting it to the very top). - Block the sidebar's own insert when it later tries to create its canonical entry for the same
projectId, potentially swallowing the error silently or producing an unexpected no-op.
A safer approach would be to scope this field write so it only mutates an already-existing record, and treats a missing record as "no preference stored yet":
const persistDefaultApp = useCallback(
(app: ExternalApp) => {
if (collections.v2SidebarProjects.get(projectId)) {
collections.v2SidebarProjects.update(projectId, (draft) => {
draft.defaultOpenInApp = app;
});
}
// No insert fallback: the sidebar manages the authoritative record.
},
[collections, projectId],
);If the intent is genuinely to persist the choice before the sidebar creates its record, the insertion should use the sidebar's own "create project entry" logic so that tabOrder is computed correctly rather than hard-coded to 0.
| const resolvedApp: ExternalApp = | ||
| (sidebarProjectRows[0]?.defaultOpenInApp as ExternalApp | null) ?? "finder"; |
There was a problem hiding this comment.
string | null to ExternalApp
defaultOpenInApp is typed as string | null in the schema, but it is cast directly to ExternalApp | null here. If a stale or manually-edited localStorage entry contains a string that is not a member of the ExternalApp union, resolvedApp will silently carry an invalid value. getAppOption(resolvedApp) does guard against this (returning null → currentApp is null → the button shows "Select an editor from the dropdown"), so there is no crash, but the mismatch between the declared type and the runtime value could hide bugs in callers that trust the type annotation.
Consider validating against the known ExternalApp values before the cast, or narrowing the schema to an enum so Zod rejects unknown strings at parse time:
const rawApp = sidebarProjectRows[0]?.defaultOpenInApp ?? null;
const resolvedApp: ExternalApp = (isExternalApp(rawApp) ? rawApp : null) ?? "finder";where isExternalApp is a simple type-guard against the ExternalApp union.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx:57">
P2: This fallback `insert` creates a `v2SidebarProjects` record with hard-coded `tabOrder: 0` and `isCollapsed: false`. Since this collection is read by the sidebar renderer, this can surface a phantom project row (likely sorted to the top) and may conflict with the sidebar's own insert logic for the same `projectId`. Consider only updating existing records here and skipping the insert entirely — let the sidebar manage authoritative record creation.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx:60">
P2: The defensive `v2SidebarProjects.insert` uses `tabOrder: 0`, which can reorder the sidebar by forcing that project to the top.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Avoid a phantom v2SidebarProjects row with hard-coded tabOrder: 0 when persisting the default Open In app. Delegate record creation to the canonical sidebar state helper, which computes tabOrder correctly.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…idebarProjects Align with the per-project v2 "Open In" pattern established in #3393: resolve the editor from collections.v2SidebarProjects.defaultOpenInApp (falling back to finder) and call electronTrpc.external.openInApp — instead of openFileInEditor, which does v1-era backend resolution off the projects.defaultApp column that v2 doesn't write to.
#3398) * feat(desktop): modifier+click in file tree opens external editor / new tab - Meta/Ctrl+click: open in external editor (works on files and folders) - Shift+click: open file in a new internal tab - Plain click: unchanged (preview pane / toggle folder) Adds the behavior to both v1 (FileTreeItem, FileSearchResultItem) and v2 (WorkspaceFilesTreeItem) file trees. v2 guards the external-editor path with an isLocalWorkspace check so remote hosts toast instead of silently calling local electron IPC against a path that does not exist. * fix(desktop): v2 file-tree meta+click reads defaultOpenInApp from v2SidebarProjects Align with the per-project v2 "Open In" pattern established in #3393: resolve the editor from collections.v2SidebarProjects.defaultOpenInApp (falling back to finder) and call electronTrpc.external.openInApp — instead of openFileInEditor, which does v1-era backend resolution off the projects.defaultApp column that v2 doesn't write to. * fix(desktop): address PR review on v2 file-tree meta+click - Gate the isLocal check on the host live query actually resolving, so a meta+click before the collections query settles silently no-ops instead of toasting a misleading "only supported on local workspaces" error (cubic/greptile/coderabbit). - Use a separator-safe split when deriving the new-tab title from a file path so Windows host paths (backslash-separated) don't produce a full path as the tab title (cubic).
…radevAI#3393) * fix(desktop): v2 Open In editor is per-project, not per-workspace The v2 top-bar "Open In <editor>" dropdown was storing the choice in the per-workspace localStorage collection, so switching between workspaces in the same project reset the picker. Move it to the per-project v2SidebarProjects collection so the choice is shared across every workspace in a project. Also drops the unused workspaceId prop and no longer passes projectId to openInApp, since persistence now happens entirely client-side on the react-db collection. * fix(desktop): use ensureProjectInSidebar helper in V2OpenInMenuButton Avoid a phantom v2SidebarProjects row with hard-coded tabOrder: 0 when persisting the default Open In app. Delegate record creation to the canonical sidebar state helper, which computes tabOrder correctly.
ParadevAI#3398) * feat(desktop): modifier+click in file tree opens external editor / new tab - Meta/Ctrl+click: open in external editor (works on files and folders) - Shift+click: open file in a new internal tab - Plain click: unchanged (preview pane / toggle folder) Adds the behavior to both v1 (FileTreeItem, FileSearchResultItem) and v2 (WorkspaceFilesTreeItem) file trees. v2 guards the external-editor path with an isLocalWorkspace check so remote hosts toast instead of silently calling local electron IPC against a path that does not exist. * fix(desktop): v2 file-tree meta+click reads defaultOpenInApp from v2SidebarProjects Align with the per-project v2 "Open In" pattern established in ParadevAI#3393: resolve the editor from collections.v2SidebarProjects.defaultOpenInApp (falling back to finder) and call electronTrpc.external.openInApp — instead of openFileInEditor, which does v1-era backend resolution off the projects.defaultApp column that v2 doesn't write to. * fix(desktop): address PR review on v2 file-tree meta+click - Gate the isLocal check on the host live query actually resolving, so a meta+click before the collections query settles silently no-ops instead of toasting a misleading "only supported on local workspaces" error (cubic/greptile/coderabbit). - Use a separator-safe split when deriving the new-tab title from a file path so Windows host paths (backslash-separated) don't produce a full path as the tab title (cubic).
Summary
v2WorkspaceLocalState.defaultOpenInApp(per-workspace localStorage), so switching workspaces inside the same project silently reset the picker.v2SidebarProjectscollection: read reactively viauseLiveQuery, persist viacollections.v2SidebarProjects.update(projectId, ...)with a defensive insert fallback. The choice is now shared across every workspace in a project.projectIdthroughV2WorkspaceOpenInButtonand drop the now-unusedworkspaceIdprop.openInAppmutations no longer passprojectId, so persistence lives entirely on the client-side react-db collection and the server-sideprojects.defaultAppSQLite column is left alone.defaultOpenInAppfield fromworkspaceLocalStateSchema. Stale values in existing localStorage entries are silently dropped by Zod on next parse.Test plan
bun run lintbunx turbo typecheck --filter @superset/desktopbunx turbo test --filter @superset/desktop(1593 pass)Note: v2 file-tree / diff-pane clicks still resolve
openFileInEditorthroughresolveDefaultEditor(projectId)in the main process, which reads the SQLiteprojects.defaultAppcolumn (now unwritten by the v2 path) and falls back tosettings.defaultEditor. Making those call sites honor the per-project react-db choice is a follow-up.Summary by cubic
Make the v2 top-bar “Open In ” setting per-project, so the choice persists across workspaces in the same project. Persistence lives in the per-project sidebar collection and uses the sidebar state helper to create rows correctly.
v2SidebarProjects(read viauseLiveQuery); callensureProjectInSidebarbeforeupdate(...)to avoid phantom rows and keep tab order.projectIdtoV2WorkspaceOpenInButton/V2OpenInMenuButtonand removeworkspaceId; do not sendprojectIdtoopenInApp.defaultOpenInApptodashboardSidebarProjectSchema; remove it fromworkspaceLocalStateSchemaand ignore stale localStorage values.Written for commit d2722b8. Summary will update on new commits.
Summary by CodeRabbit