feat(desktop): add global default editor setting#1649
Conversation
📝 WalkthroughWalkthroughThis PR implements a global default editor setting feature. It adds a database column for storing a global default editor, creates settings router procedures to manage it, introduces a resolver function that prioritizes project-level then global defaults, and updates UI components to use this resolver with proper fallback handling. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as UI Component
participant Resolver as resolveDefaultEditor()
participant DB as Database
participant App as External App
User->>UI: Request to open in editor
UI->>Resolver: resolveDefaultEditor(projectId)
Resolver->>DB: Query project settings<br/>(if projectId provided)
DB-->>Resolver: project defaultApp or null
alt Project default found
Resolver-->>UI: Return project default
else No project default
Resolver->>DB: Query global settings
DB-->>Resolver: global defaultEditor or null
Resolver-->>UI: Return global default or null
end
alt Default app exists
UI->>App: Launch app with path
App-->>UI: Success
UI->>DB: Persist as project default<br/>(if projectId available)
UI->>DB: Persist as global default<br/>(via ensureGlobalDefaultEditor)
else No default configured
UI-->>User: Error: Select editor<br/>from dropdown
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsx (1)
32-38: Good use ofgetDefaultEditorfor the settings page context.The inline comments clearly explain why no
projectIdis used here.Minor UX gap: the
openInAppmutation has noonSuccessinvalidation forsettings.getDefaultEditor. IfensureGlobalDefaultEditorauto-sets the global default on the server (first-time use), the "Default" badge won't appear until react-query refetches. Consider adding invalidation:Suggested improvement
+ const utils = electronTrpc.useUtils(); const openInApp = electronTrpc.external.openInApp.useMutation({ + onSuccess: () => { + utils.settings.getDefaultEditor.invalidate(); + }, onError: (error) => toast.error(`Failed to open: ${error.message}`), });🤖 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/components/ClickablePath/ClickablePath.tsx` around lines 32 - 38, Add a success handler to the openInApp mutation so the settings.getDefaultEditor cache is refreshed after a successful call: update the openInApp = electronTrpc.external.openInApp.useMutation({ ... }) setup to include an onSuccess that invalidates or refetches the electronTrpc.settings.getDefaultEditor.useQuery cache (or calls its refetch) so the "Default" badge appears immediately when ensureGlobalDefaultEditor sets the global default on the server.apps/desktop/src/lib/trpc/routers/external/index.ts (1)
151-163: Consider whetherBAD_REQUESTis the best error code here.The error is triggered when no editor is configured — this isn't really a malformed request from the client, but rather a "precondition not met" scenario.
PRECONDITION_FAILEDmight be more semantically accurate, thoughBAD_REQUESTworks in practice. This is a very minor nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 151 - 163, The current tRPC mutation throws a TRPCError with code "BAD_REQUEST" when resolveDefaultEditor(input.projectId) returns no editor; change the error code to "PRECONDITION_FAILED" to better reflect a missing precondition rather than a malformed request, keeping the same message; update the throw in the mutation (the TRPCError instantiation that references resolveDefaultEditor) and run type-checks to ensure "PRECONDITION_FAILED" is an accepted TRPCError code in your `@trpc/server` version.apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx (1)
102-103:getAppOptionstill silently falls back toAPP_OPTIONS[1](Cursor) for unknown IDs.When
defaultAppis defined but contains an unrecognizedExternalAppvalue (e.g., from a stale DB entry or future migration),getAppOptionsilently resolves to Cursor. Now that the PR explicitly removes hardcoded Cursor defaults elsewhere, this hidden fallback is inconsistent with the new design intent. Consider returningundefinedinstead and handling the miss at call sites, or at least logging a warning.♻️ Suggested change
-export const getAppOption = (id: ExternalApp) => - ALL_APP_OPTIONS.find((app) => app.id === id) ?? APP_OPTIONS[1]; +export const getAppOption = (id: ExternalApp) => + ALL_APP_OPTIONS.find((app) => app.id === id);Then update call sites (this file and
OpenInMenuButton.tsx) to handleundefined:const currentApp = defaultApp ? getAppOption(defaultApp) ?? null : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx` around lines 102 - 103, getAppOption currently falls back to APP_OPTIONS[1] (Cursor) when an unknown ExternalApp id is passed; change getAppOption (and any helper using ALL_APP_OPTIONS) to return undefined instead of defaulting to APP_OPTIONS[1] so callers can handle unknown IDs explicitly. Update call sites (e.g., OpenInButton/OpenInMenuButton where getAppOption is used) to null-check/handle undefined (per suggestion: const currentApp = defaultApp ? getAppOption(defaultApp) ?? null : null) and optionally log a warning when a stored defaultApp is unrecognized so migrations/stale DB entries are visible. Ensure the function signature and any downstream types reflect the possibility of undefined.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx (1)
70-73: Guard on!defaultAppis purely defensive here — consider adding a toast for keyboard/assistive-tech users.The button is visually disabled when
!currentApp, so sighted mouse users won't triggerhandleOpenInEditor. However, in some edge cases (e.g., the query resolves toundefinedafter the button was already focused and enabled, or assistive technology bypassingdisabled), the silentreturnoffers no feedback. TheusePathActionshook shows a toast in this scenario. A toast here would provide a consistent experience, though the risk is low given the disabled guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx` around lines 70 - 73, handleOpenInEditor silently returns when defaultApp is falsy; instead, call the path-actions toast so keyboard/assistive-tech users get feedback. Update the useCallback for handleOpenInEditor to check if (!defaultApp) then invoke the toast from usePathActions (or the showToast function exposed by usePathActions) with a short message and return, otherwise proceed to call openInApp.mutate({ path: worktreePath, app: defaultApp, projectId }). Keep existing guards for openInApp.isPending and copyPath.isPending and ensure the toast call is included in the callback dependencies.
🤖 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/lib/trpc/routers/external/index.ts`:
- Around line 151-163: The current tRPC mutation throws a TRPCError with code
"BAD_REQUEST" when resolveDefaultEditor(input.projectId) returns no editor;
change the error code to "PRECONDITION_FAILED" to better reflect a missing
precondition rather than a malformed request, keeping the same message; update
the throw in the mutation (the TRPCError instantiation that references
resolveDefaultEditor) and run type-checks to ensure "PRECONDITION_FAILED" is an
accepted TRPCError code in your `@trpc/server` version.
In `@apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx`:
- Around line 102-103: getAppOption currently falls back to APP_OPTIONS[1]
(Cursor) when an unknown ExternalApp id is passed; change getAppOption (and any
helper using ALL_APP_OPTIONS) to return undefined instead of defaulting to
APP_OPTIONS[1] so callers can handle unknown IDs explicitly. Update call sites
(e.g., OpenInButton/OpenInMenuButton where getAppOption is used) to
null-check/handle undefined (per suggestion: const currentApp = defaultApp ?
getAppOption(defaultApp) ?? null : null) and optionally log a warning when a
stored defaultApp is unrecognized so migrations/stale DB entries are visible.
Ensure the function signature and any downstream types reflect the possibility
of undefined.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx`:
- Around line 70-73: handleOpenInEditor silently returns when defaultApp is
falsy; instead, call the path-actions toast so keyboard/assistive-tech users get
feedback. Update the useCallback for handleOpenInEditor to check if
(!defaultApp) then invoke the toast from usePathActions (or the showToast
function exposed by usePathActions) with a short message and return, otherwise
proceed to call openInApp.mutate({ path: worktreePath, app: defaultApp,
projectId }). Keep existing guards for openInApp.isPending and
copyPath.isPending and ensure the toast call is included in the callback
dependencies.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsx`:
- Around line 32-38: Add a success handler to the openInApp mutation so the
settings.getDefaultEditor cache is refreshed after a successful call: update the
openInApp = electronTrpc.external.openInApp.useMutation({ ... }) setup to
include an onSuccess that invalidates or refetches the
electronTrpc.settings.getDefaultEditor.useQuery cache (or calls its refetch) so
the "Default" badge appears immediately when ensureGlobalDefaultEditor sets the
global default on the server.
de1mat
force-pushed
the
feat/global-default-editor
branch
from
2e46357 to
624805a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx (1)
303-307:projectId as stringtype assertion suppresses type safety — prefer a safer narrowing.The
{ enabled: !!projectId }guard makes this safe at runtime, but theas stringcast hides thestring | undefinedtype from the input object, defeating TypeScript's narrowing. A cleaner pattern is to pass the narrowed value explicitly:♻️ Suggested refactor
- const { data: defaultApp } = - electronTrpc.projects.getDefaultApp.useQuery( - { projectId: projectId as string }, - { enabled: !!projectId }, - ); + const { data: defaultApp } = + electronTrpc.projects.getDefaultApp.useQuery( + { projectId: projectId ?? "" }, + { enabled: !!projectId }, + );Alternatively, if tRPC supports
skipToken(react-query v5+):+import { skipToken } from "@tanstack/react-query"; ... - electronTrpc.projects.getDefaultApp.useQuery( - { projectId: projectId as string }, - { enabled: !!projectId }, - ); + electronTrpc.projects.getDefaultApp.useQuery( + projectId ? { projectId } : skipToken, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx around lines 303 - 307, The code casts projectId with "projectId as string" which suppresses type safety; instead, pass a properly narrowed value to electronTrpc.projects.getDefaultApp.useQuery by only supplying the params when projectId is truthy (e.g., call useQuery with the object built from projectId or use a skip/skipToken/undefined pattern supported by your TRPC/react-query integration), or use the skipToken mechanism if available; update the call referencing electronTrpc.projects.getDefaultApp.useQuery and the projectId variable so the input type remains string | undefined-safe without using "as string".apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx (1)
129-133:projectId as stringcast is a minor type smell.
enabled: !!projectIdmakes this safe at runtime, butprojectId as stringasserts awayundefined. A non-casting alternative (e.g.,projectId!orprojectId ?? "") would be equivalent and more explicit about the intent.🔧 Alternative without widening cast
- const { data: defaultApp } = - electronTrpc.projects.getDefaultApp.useQuery( - { projectId: projectId as string }, - { enabled: !!projectId }, - ); + const { data: defaultApp } = + electronTrpc.projects.getDefaultApp.useQuery( + // eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` + { projectId: projectId! }, + { enabled: !!projectId }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx` around lines 129 - 133, The code casts away undefined with projectId as string when calling electronTrpc.projects.getDefaultApp.useQuery; instead rely on the enabled guard and use a non-widening expression (e.g., projectId! or projectId ?? "") to make the intent explicit. Update the call to electronTrpc.projects.getDefaultApp.useQuery to pass projectId! (or a safe default via ?? "") and keep enabled: !!projectId so types reflect that projectId is guaranteed when the query runs.apps/desktop/src/lib/trpc/routers/external/index.ts (1)
27-41:onConflictDoUpdateinside the null-guard doesn't give atomic "set if not set" semantics.Two concurrent callers could both pass
if (!row?.defaultEditor), then each unconditionally overwritedefaultEditorin the upsert — last writer wins, rather than "first use wins." This is very low risk in practice (Electron IPC is largely serialized), but the intent isn't fully expressed by the code. A purely advisory note: a singleUPDATE settings SET default_editor = ? WHERE default_editor IS NULL(raw SQL) would be atomic without the two-step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 27 - 41, ensureGlobalDefaultEditor currently reads row then upserts, which allows a race where two callers both see null and both overwrite; replace that read-then-upsert with an atomic operation: execute a single UPDATE on the settings row (targeting id 1) that sets defaultEditor = app WHERE defaultEditor IS NULL and check affected row count; if the UPDATE affected 0 rows then perform an insert (or an upsert that does nothing on conflict) to handle the case the settings row doesn't exist. Use the UPDATE-first approach in ensureGlobalDefaultEditor to ensure "first use wins" semantics atomically.
🤖 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/lib/trpc/routers/external/index.ts`:
- Around line 131-134: The call to ensureGlobalDefaultEditor can throw and must
not cause the openInApp mutation to fail after openPathInApp already succeeded;
wrap the ensureGlobalDefaultEditor(input.app) invocation in a try/catch that
swallows any errors and logs them (e.g., console.error or the module's logger)
so the mutation remains successful and client onSuccess still runs; do not
change the openPathInApp call or its await, only protect the side-effect call
(ensureGlobalDefaultEditor) from propagating exceptions.
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 667-680: The mutation setDefaultEditor currently accepts any
EXTERNAL_APPS value (including non-editors) — update the input validation to
reject NON_EDITOR_APPS so only real editors (or null) can be stored; import (and
if missing, export) NON_EDITOR_APPS from external/index.ts and change the
procedure input from z.enum(EXTERNAL_APPS).nullable() to a schema that refines
or validates values against NON_EDITOR_APPS (e.g., allow null or ensure the
value is not in NON_EDITOR_APPS) before writing to localDb, keeping references
to setDefaultEditor, EXTERNAL_APPS, NON_EDITOR_APPS, openFileInEditor,
openPathInApp, ensureGlobalDefaultEditor, localDb, and settings to locate the
related logic.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx`:
- Around line 103-126: In OpenInMenuButton adjust the className logic so
pointer-events-none is applied only when isLoading (not when !currentApp); keep
disabled={isLoading || !currentApp} as-is so clicks are blocked but remove
!currentApp from the pointer-events-none condition so the guidance tooltip
("Select an editor from the dropdown") can still appear when no editor is
configured; update the conditional expression that currently reads (isLoading ||
!currentApp) && "opacity-50 pointer-events-none" to only include isLoading for
pointer-events-none while preserving opacity-50 for the !currentApp state if
desired.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 27-41: ensureGlobalDefaultEditor currently reads row then upserts,
which allows a race where two callers both see null and both overwrite; replace
that read-then-upsert with an atomic operation: execute a single UPDATE on the
settings row (targeting id 1) that sets defaultEditor = app WHERE defaultEditor
IS NULL and check affected row count; if the UPDATE affected 0 rows then perform
an insert (or an upsert that does nothing on conflict) to handle the case the
settings row doesn't exist. Use the UPDATE-first approach in
ensureGlobalDefaultEditor to ensure "first use wins" semantics atomically.
In `@apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx`:
- Around line 129-133: The code casts away undefined with projectId as string
when calling electronTrpc.projects.getDefaultApp.useQuery; instead rely on the
enabled guard and use a non-widening expression (e.g., projectId! or projectId
?? "") to make the intent explicit. Update the call to
electronTrpc.projects.getDefaultApp.useQuery to pass projectId! (or a safe
default via ?? "") and keep enabled: !!projectId so types reflect that projectId
is guaranteed when the query runs.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx:
- Around line 303-307: The code casts projectId with "projectId as string" which
suppresses type safety; instead, pass a properly narrowed value to
electronTrpc.projects.getDefaultApp.useQuery by only supplying the params when
projectId is truthy (e.g., call useQuery with the object built from projectId or
use a skip/skipToken/undefined pattern supported by your TRPC/react-query
integration), or use the skipToken mechanism if available; update the call
referencing electronTrpc.projects.getDefaultApp.useQuery and the projectId
variable so the input type remains string | undefined-safe without using "as
string".
c1b9fde to
07e3c53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.ts`:
- Around line 23-28: The openInAppMutation call inside the openInEditor callback
lacks an onError handler so failures (e.g., missing app bundle) produce no user
feedback; add an onError to the electronTrpc.external.openInApp.useMutation
invocation (symmetric to electronTrpc.external.openFileInEditor.useMutation's
onError) that calls toast.error with a clear message and includes error.message
in the description so users see a failure notification when
openInAppMutation.mutate(...) fails.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 128-132: The call to ensureGlobalDefaultEditor should be treated
as best-effort and must not cause the mutation to fail after openPathInApp
succeeds; wrap ensureGlobalDefaultEditor(input.app) in a try/catch immediately
after the successful await of openPathInApp so any thrown errors are caught and
logged (e.g., processLogger.warn or console.error) and not rethrown, ensuring
the mutation returns success even if persisting the global default fails; keep
openPathInApp and ensureGlobalDefaultEditor calls in the same order and do not
change the primary operation.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx`:
- Around line 109-116: The button's class currently applies
"pointer-events-none" when !currentApp which blocks the tooltip; update the
conditional in OpenInMenuButton's className so pointer-events-none is only
applied for the loading state (isLoading) and not when no editor is
configured—e.g., replace (isLoading || !currentApp) && "opacity-50
pointer-events-none" with isLoading && "opacity-50 pointer-events-none" (keep
the disabled prop for the !currentApp case so clicks remain prevented) so the
guidance tooltip can still receive hover events when no editor is selected.
07e3c53 to
0b06abe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
143-166: Acceptlineandcolumnparameters in the input schema, but don't forward them to the editor.The
openFileInEditormutation acceptslineandcolumn(lines 147–148) butopenPathInApp(filePath, app)on line 165 doesn't receive them. Multiple callers already pass these values expecting editor navigation (e.g.,Terminal/helpers.ts,useFileLinkClick.ts), but the implementation silently discards them.Consider extending
openPathInAppto accept and forwardlineandcolumnto support editor jump-to-line functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 143 - 166, openFileInEditor currently parses line and column from the input (z.object) but discards them when calling openPathInApp; update the flow so these cursor coordinates are forwarded: change openPathInApp signature to accept optional line and column (or an options object) and update the mutation to call openPathInApp(filePath, app, { line: input.line, column: input.column }) after resolving filePath and app; adjust any affected callers and type definitions (and resolvePath/resolveDefaultEditor usage if needed) so editors can receive and act on jump-to-line coordinates.
🤖 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/lib/trpc/routers/external/index.ts`:
- Around line 143-166: openFileInEditor currently parses line and column from
the input (z.object) but discards them when calling openPathInApp; update the
flow so these cursor coordinates are forwarded: change openPathInApp signature
to accept optional line and column (or an options object) and update the
mutation to call openPathInApp(filePath, app, { line: input.line, column:
input.column }) after resolving filePath and app; adjust any affected callers
and type definitions (and resolvePath/resolveDefaultEditor usage if needed) so
editors can receive and act on jump-to-line coordinates.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
120-140:⚠️ Potential issue | 🟡 MinorProject
defaultAppis updated beforeopenPathInAppsucceeds.Line 122-127 persists
input.appas the project's default beforeopenPathInAppis awaited. If the app isn't installed andopenPathInAppthrows, the project'sdefaultAppis already set to an app that failed to launch. Consider moving the project update after theawait openPathInApp(...)call, similar to howensureGlobalDefaultEditoris placed.Proposed fix — move project update after successful open
.mutation(async ({ input }) => { - if (input.projectId) { - localDb - .update(projects) - .set({ defaultApp: input.app }) - .where(eq(projects.id, input.projectId)) - .run(); - } - await openPathInApp(input.path, input.app); + if (input.projectId) { + localDb + .update(projects) + .set({ defaultApp: input.app }) + .where(eq(projects.id, input.projectId)) + .run(); + } + // Auto-set global default editor on first successful use (best-effort)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 120 - 140, The project defaultApp is being persisted before openPathInApp succeeds; move the localDb.update(projects).set({ defaultApp: input.app }).where(eq(projects.id, input.projectId)).run() call to after the awaited openPathInApp(input.path, input.app) so the DB update only occurs on successful launch; ensure you await the .run() promise and keep the existing projectId guard and the try/catch around ensureGlobalDefaultEditor(input.app).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 120-140: The project defaultApp is being persisted before
openPathInApp succeeds; move the localDb.update(projects).set({ defaultApp:
input.app }).where(eq(projects.id, input.projectId)).run() call to after the
awaited openPathInApp(input.path, input.app) so the DB update only occurs on
successful launch; ensure you await the .run() promise and keep the existing
projectId guard and the try/catch around ensureGlobalDefaultEditor(input.app).
975cabb to
0479b6f
Compare
|
Re: project Good catch. Moved the project default update (and global default auto-set) to after the |
de1mat
force-pushed
the
feat/global-default-editor
branch
2 times, most recently
from
f001fc5 to
1f4af46
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
684-714: LGTM!
getDefaultEditorandsetDefaultEditorfollow the same upsert pattern established by every other settings accessor in this file. The.refine()validation againstNON_EDITOR_APPScorrectly prevents non-editor apps from being stored as the global default, as previously discussed and confirmed.🤖 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 684 - 714, The review approves the implementation: no code changes required — leave getDefaultEditor and setDefaultEditor as-is (they correctly use EXTERNAL_APPS with .refine against NON_EDITOR_APPS and perform the upsert via localDb.insert(settings)...onConflictDoUpdate), so keep these functions and their validation/upsert logic unchanged.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
154-167:openFileInEditornow has a clear error when no editor is configured.
resolveDefaultEditormay return a non-editor app (e.g.,"finder") if that's what the project'sdefaultAppis set to — which would causeopenPathInAppto callshell.showItemInFolderinstead of opening in an actual editor. This is pre-existing behavior from the per-project default, but now thatopenFileInEditoris explicitly named "editor", it might be worth filtering non-editor apps here too.Consider whether
resolveDefaultEditorshould skip non-editor project defaults for this specific code path, or whether the function name is sufficient documentation of intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 154 - 167, The code calls resolveDefaultEditor(...) and passes its result to openPathInApp(...), but resolveDefaultEditor can return non-editor apps like "finder" which causes shell.showItemInFolder to be used; update the mutation to validate the returned app is an actual editor before calling openPathInApp: after const app = resolveDefaultEditor(input.projectId) check for an editor-specific property or type (e.g., app.kind === 'editor' or a helper isEditorApp(app)), and if it isn’t an editor throw the same TRPCError("PRECONDITION_FAILED", "No default editor configured...") or call resolveDefaultEditor with a flag to skip non-editors; reference resolveDefaultEditor and openPathInApp in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 684-714: The review approves the implementation: no code changes
required — leave getDefaultEditor and setDefaultEditor as-is (they correctly use
EXTERNAL_APPS with .refine against NON_EDITOR_APPS and perform the upsert via
localDb.insert(settings)...onConflictDoUpdate), so keep these functions and
their validation/upsert logic unchanged.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 154-167: The code calls resolveDefaultEditor(...) and passes its
result to openPathInApp(...), but resolveDefaultEditor can return non-editor
apps like "finder" which causes shell.showItemInFolder to be used; update the
mutation to validate the returned app is an actual editor before calling
openPathInApp: after const app = resolveDefaultEditor(input.projectId) check for
an editor-specific property or type (e.g., app.kind === 'editor' or a helper
isEditorApp(app)), and if it isn’t an editor throw the same
TRPCError("PRECONDITION_FAILED", "No default editor configured...") or call
resolveDefaultEditor with a flag to skip non-editors; reference
resolveDefaultEditor and openPathInApp in your change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.tspackages/local-db/drizzle/0032_add_default_editor_setting.sqlpackages/local-db/drizzle/meta/0032_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.ts
✅ Files skipped from review due to trivial changes (1)
- packages/local-db/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.ts
- packages/local-db/src/schema/schema.ts
- packages/local-db/src/schema/zod.ts
- Add `defaultEditor` column to settings table with migration - Extract `resolveDefaultEditor()` and `ensureGlobalDefaultEditor()` helpers - Auto-set global default on first successful editor launch - Filter non-editor apps (Finder, terminals) from becoming default - Add `getDefaultEditor`/`setDefaultEditor` tRPC endpoints - Null-safe UI: fallback icon, disabled state, hotkey guard - Show toast when no editor configured instead of silent no-op
de1mat
force-pushed
the
feat/global-default-editor
branch
from
1f4af46 to
ca36c2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/trpc/routers/external/index.ts`:
- Around line 41-53: resolveDefaultEditor currently returns project.defaultApp
without verifying it's an editor, allowing non-editor values (e.g., "finder",
"terminal") to be used; update resolveDefaultEditor to only return
project.defaultApp when it is a valid editor (e.g., via an existing helper like
isEditorApp or by checking against the canonical editor list) and otherwise fall
back to settings.defaultEditor; reference resolveDefaultEditor,
project.defaultApp, settings.defaultEditor, and the related helpers
ensureGlobalDefaultEditor/setDefaultEditor so the project-level lookup mirrors
the global guard.
In `@apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx`:
- Around line 129-132: The query call using
electronTrpc.projects.getDefaultApp.useQuery currently casts projectId with
"projectId as string" which weakens type safety; change the argument to use the
non-null assertion "projectId!" instead. Locate the use of
electronTrpc.projects.getDefaultApp.useQuery in OpenInButton and replace the
cast with projectId! so the enabled guard ({ enabled: !!projectId }) remains
correct and TypeScript keeps the tighter non-null typing for the query input.
Ensure no other occurrences in OpenInButton use the "as string" cast for
projectId.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.tspackages/local-db/drizzle/0032_add_default_editor_setting.sqlpackages/local-db/drizzle/meta/0032_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/local-db/src/schema/schema.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsx
- packages/local-db/drizzle/0032_add_default_editor_setting.sql
- packages/local-db/src/schema/zod.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/hooks/usePathActions.ts
- apps/desktop/src/lib/trpc/routers/projects/projects.ts
- packages/local-db/drizzle/meta/0032_snapshot.json
- packages/local-db/drizzle/meta/_journal.json
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx
| /** Resolves the default editor from project setting, then global setting. */ | ||
| export function resolveDefaultEditor(projectId?: string): ExternalApp | null { | ||
| if (projectId) { | ||
| const project = localDb | ||
| .select() | ||
| .from(projects) | ||
| .where(eq(projects.id, projectId)) | ||
| .get(); | ||
| if (project?.defaultApp) return project.defaultApp; | ||
| } | ||
| const row = localDb.select().from(settings).get(); | ||
| return row?.defaultEditor ?? null; | ||
| } |
There was a problem hiding this comment.
resolveDefaultEditor can return a non-editor app from project.defaultApp.
The global default (settings.defaultEditor) is properly guarded against non-editor apps by both ensureGlobalDefaultEditor and setDefaultEditor. However, project.defaultApp stores whatever app was last used (including Finder, Terminal, etc.), and resolveDefaultEditor returns it without filtering.
If a user's last action on a project was "Open in Finder", then openFileInEditor would resolve to "finder" and call shell.showItemInFolder instead of launching an editor.
Consider filtering out non-editor apps from the project-level lookup:
Proposed fix
export function resolveDefaultEditor(projectId?: string): ExternalApp | null {
if (projectId) {
const project = localDb
.select()
.from(projects)
.where(eq(projects.id, projectId))
.get();
- if (project?.defaultApp) return project.defaultApp;
+ if (project?.defaultApp && !nonEditorSet.has(project.defaultApp))
+ return project.defaultApp;
}
const row = localDb.select().from(settings).get();
return row?.defaultEditor ?? null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 41 - 53,
resolveDefaultEditor currently returns project.defaultApp without verifying it's
an editor, allowing non-editor values (e.g., "finder", "terminal") to be used;
update resolveDefaultEditor to only return project.defaultApp when it is a valid
editor (e.g., via an existing helper like isEditorApp or by checking against the
canonical editor list) and otherwise fall back to settings.defaultEditor;
reference resolveDefaultEditor, project.defaultApp, settings.defaultEditor, and
the related helpers ensureGlobalDefaultEditor/setDefaultEditor so the
project-level lookup mirrors the global guard.
| const { data: defaultApp } = electronTrpc.projects.getDefaultApp.useQuery( | ||
| { projectId: projectId as string }, | ||
| { enabled: !!projectId }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxRepository: superset-sh/superset
Length of output: 12118
🏁 Script executed:
rg -n "OpenInButton" --type=tsx --type=ts -B 2 -A 5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg -n "OpenInButton" -B 2 -A 5Repository: superset-sh/superset
Length of output: 6954
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/ConfigFilePreview/ConfigFilePreview.tsx | grep -A 10 -B 5 "OpenInButton"Repository: superset-sh/superset
Length of output: 1277
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OpenInMenuButton/OpenInMenuButton.tsx | head -80Repository: superset-sh/superset
Length of output: 3371
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/settings/components/ClickablePath/ClickablePath.tsx | head -80Repository: superset-sh/superset
Length of output: 3199
🏁 Script executed:
rg "<OpenInButton" -B 3 -A 5Repository: superset-sh/superset
Length of output: 1034
🏁 Script executed:
rg "OpenInButton" --type ts --type tsx | grep -v "import\|export\|interface\|from\|props\|Props"Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/ConfigFilePreview/ConfigFilePreview.tsx | head -20Repository: superset-sh/superset
Length of output: 762
projectId as string breaks type safety; replace with non-null assertion projectId!.
The idiomatic pattern for enabled-guarded tRPC queries is the non-null assertion projectId! rather than projectId as string. It's equally concise but maintains type safety:
Proposed fix
- const { data: defaultApp } = electronTrpc.projects.getDefaultApp.useQuery(
- { projectId: projectId as string },
+ const { data: defaultApp } = electronTrpc.projects.getDefaultApp.useQuery(
+ { projectId: projectId! },Note: While projectId is optional in the props interface, OpenInButton is currently only rendered in ConfigFilePreview, which always supplies a projectId. The global default handling is out of scope for this component (see ClickablePath for the pattern used when handling settings without project context).
📝 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 { data: defaultApp } = electronTrpc.projects.getDefaultApp.useQuery( | |
| { projectId: projectId as string }, | |
| { enabled: !!projectId }, | |
| ); | |
| const { data: defaultApp } = electronTrpc.projects.getDefaultApp.useQuery( | |
| { projectId: projectId! }, | |
| { enabled: !!projectId }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx` around
lines 129 - 132, The query call using
electronTrpc.projects.getDefaultApp.useQuery currently casts projectId with
"projectId as string" which weakens type safety; change the argument to use the
non-null assertion "projectId!" instead. Locate the use of
electronTrpc.projects.getDefaultApp.useQuery in OpenInButton and replace the
cast with projectId! so the enabled guard ({ enabled: !!projectId }) remains
correct and TypeScript keeps the tighter non-null typing for the query input.
Ensure no other occurrences in OpenInButton use the "as string" cast for
projectId.
|
Hey @Kitenite - heads up, I rebased onto main and force-pushed which overwrote your 3 cleanup commits (9408227, 5ab01e5, 975cabb). Apologies for the overlap. The current state incorporates all the same changes - conflict resolution, migration renumbering (now 0032), and the CodeRabbit feedback. I also moved Let me know if anything looks off. |
|
No worries @de1mat its my fault for not just merging. Apologies and will pull again |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
41-53:⚠️ Potential issue | 🟡 MinorFilter project defaults to editors before returning.
project.defaultAppcan be a non-editor (Finder/Terminal), which would leak into editor-only flows.🛠️ Suggested fix
- if (project?.defaultApp) return project.defaultApp; + if (project?.defaultApp && !nonEditorSet.has(project.defaultApp)) { + return project.defaultApp; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 41 - 53, resolveDefaultEditor currently returns project.defaultApp even if it's not an editor; update resolveDefaultEditor to validate that the project default is an editor before returning it (e.g., add an isEditor check or whitelist allowed ExternalApp values) and only return project.defaultApp when it passes that editor check; otherwise fall through to read settings.defaultEditor as before. Target the resolveDefaultEditor function and the project.defaultApp/row?.defaultEditor values in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 41-53: resolveDefaultEditor currently returns project.defaultApp
even if it's not an editor; update resolveDefaultEditor to validate that the
project default is an editor before returning it (e.g., add an isEditor check or
whitelist allowed ExternalApp values) and only return project.defaultApp when it
passes that editor check; otherwise fall through to read settings.defaultEditor
as before. Target the resolveDefaultEditor function and the
project.defaultApp/row?.defaultEditor values in your change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxpackages/local-db/drizzle/0033_nosy_overlord.sqlpackages/local-db/drizzle/meta/0033_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/zod.ts
✅ Files skipped from review due to trivial changes (2)
- packages/local-db/drizzle/meta/0033_snapshot.json
- packages/local-db/drizzle/0033_nosy_overlord.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/settings/index.ts
|
Merged, sorry for the delay @de1mat , looks like it's much cleaner too! |
|
My pleasure @Kitenite. |
|
Hmm when I updated to 0.0.84 locally I am getting an error
Is that just me? I wonder if something was wrong with the migration? I am looking into a few things now to double-check |
|
Uh oh looks like your local db got messed up probably from migration order. Most likely the work you did in dev changed your prod db config locally and now new migrations are being skipped. |
Ok you can ignore me, it was because my local install was dirty because of the dev work I had been doing. Phew! I have cleaned that up and now working fine. |
|
Manually running that migration or adding the column in your db should fix it. Database should be under ~/.superset/*.db and it's sql |
|
Nice glad that's fixed haha |
2 participants
Description
Adds a global default editor setting that acts as a fallback when no per-project default is configured. The first time a user opens any project in an editor, that editor is automatically saved as the global default. Non-editor apps (Finder, Terminal, iTerm, Warp, Ghostty) are filtered out.
Resolution order: per-project default -> global default -> error with clear toast message.
Related Issues
Closes #1647
Type of Change
Testing
Additional Notes
Schema change: Adds
default_editortext column to thesettingstable via migration0031_add_default_editor_setting.sql.Key files:
external/index.ts-resolveDefaultEditor()andensureGlobalDefaultEditor()helpers,NON_EDITOR_APPSfiltersettings/index.ts- NewgetDefaultEditor/setDefaultEditortRPC endpointsprojects.ts- Replaced inline fallback logic with sharedresolveDefaultEditor()importSummary by CodeRabbit
Release Notes
New Features
Improvements