feat(desktop): replace lowdb with SQLite for local database#477
feat(desktop): replace lowdb with SQLite for local database#477saddlepaddle merged 5 commits intomainfrom
Conversation
|
Warning Rate limit exceeded@saddlepaddle has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (44)
WalkthroughReplaces legacy lowdb JSON persistence with a SQLite-backed local DB (better-sqlite3 + Drizzle) via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Startup
participant LocalDbMod as `@superset/local-db` (module init)
participant LegacyJson as legacy db.json
participant Sqlite as SQLite (better-sqlite3)
participant Routers as tRPC Routers
participant Renderer as Renderer/UI
App->>LocalDbMod: import localDb (module top-level)
LocalDbMod->>Sqlite: open connection (better-sqlite3, WAL)
LocalDbMod->>Sqlite: run Drizzle migrations
alt legacy db.json exists && sqlite empty
LocalDbMod->>LegacyJson: read legacy data (projects/worktrees/workspaces/settings)
LocalDbMod->>Sqlite: insert migrated rows into tables
LocalDbMod->>LegacyJson: backup legacy file (db.json.backup)
end
LocalDbMod-->>App: localDb ready
Routers->>LocalDbMod: query/mutate via localDb.select/insert/update/delete
LocalDbMod->>Sqlite: execute SQL statements
Routers-->>Renderer: return data/results
Renderer->>Renderer: render UI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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.
Actionable comments posted: 7
🧹 Nitpick comments (8)
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (1)
26-46: Standardize null/undefined handling in path helper functions.The return type inconsistency between
getWorktreePath(returnsstring | undefined) andgetWorkspacePath(returnsstring | null) can cause confusion for callers. Standardize both functions to return eithernullorundefinedconsistently.SelectWorkspace correctly includes all required properties (
type,projectId,worktreeId), withworktreeIdappropriately optional since it's only set for worktree-type workspaces.apps/desktop/src/lib/trpc/routers/config/config.ts (1)
64-73: Consider verifying update affected a row.The
dismissConfigToastmutation always returns{ success: true }without verifying if the update actually modified a row. If theprojectIddoesn't exist, the operation silently succeeds.This may be intentional (fire-and-forget semantics), but if you need confirmation:
🔎 Optional: Verify update result
dismissConfigToast: publicProcedure .input(z.object({ projectId: z.string() })) .mutation(({ input }) => { - localDb + const result = localDb .update(projects) .set({ configToastDismissed: true }) .where(eq(projects.id, input.projectId)) .run(); - return { success: true }; + return { success: result.changes > 0 }; }),apps/desktop/src/main/lib/local-db/index.ts (2)
1-12: Organize imports as flagged by the linter.The pipeline indicates imports need sorting. Additionally, consider grouping external dependencies separately from local imports.
🔎 Suggested import organization
import * as schema from "@superset/local-db"; import { projects, settings, workspaces, worktrees } from "@superset/local-db"; import Database from "better-sqlite3"; import { count } from "drizzle-orm"; import { drizzle } from "drizzle-orm/better-sqlite3"; import { migrate } from "drizzle-orm/better-sqlite3/migrator"; import { app } from "electron"; import { existsSync, readFileSync, renameSync } from "node:fs"; import { join } from "node:path"; +import { env } from "../../env.main"; import { DB_PATH as LEGACY_DB_PATH, SUPERSET_HOME_DIR } from "../app-environment"; -import { env } from "../../env.main";
72-82: Consider checking all tables before declaring "empty".The migration only checks if the
projectstable is empty. If a partial migration occurred previously (e.g., projects migrated but settings failed), running again would skip migration entirely, potentially leaving settings unmigrated.🔎 More thorough emptiness check
function migrateFromLegacyDb(): void { if (!existsSync(LEGACY_DB_PATH)) { return; } - // Check if SQLite is empty + // Check if SQLite is empty (check projects as primary indicator) const projectCount = localDb.select({ count: count() }).from(projects).get(); if (projectCount && projectCount.count > 0) { console.log("[local-db] SQLite already has data, skipping legacy migration"); return; } + + // Also skip if legacy file was already backed up + if (existsSync(`${LEGACY_DB_PATH}.backup`)) { + console.log("[local-db] Legacy backup exists, skipping migration"); + return; + }apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
1-14: Organize imports as flagged by the linter.
520-546: Simplify query and consider transaction for atomicity.
The WHERE clause
eq(projects.tabOrder, projects.tabOrder)is a tautology (always true for non-null) and doesn't filter at the DB level. The filtering happens in JS (line 525). Just remove it:The loop of individual updates could leave tabOrder in an inconsistent state if interrupted. Wrap in a transaction for atomicity.
🔎 Suggested improvements
.mutation(({ input }) => { const { fromIndex, toIndex } = input; const activeProjects = localDb .select() .from(projects) - .where(eq(projects.tabOrder, projects.tabOrder)) // Just get all with non-null tabOrder .all() .filter((p) => p.tabOrder !== null) .sort((a, b) => (a.tabOrder ?? 0) - (b.tabOrder ?? 0)); if ( fromIndex < 0 || fromIndex >= activeProjects.length || toIndex < 0 || toIndex >= activeProjects.length ) { throw new Error("Invalid fromIndex or toIndex"); } const [removed] = activeProjects.splice(fromIndex, 1); activeProjects.splice(toIndex, 0, removed); + // Update all tabOrders atomically + localDb.transaction((tx) => { for (let i = 0; i < activeProjects.length; i++) { - localDb + tx .update(projects) .set({ tabOrder: i }) .where(eq(projects.id, activeProjects[i].id)) .run(); } + }); return { success: true }; }),apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
39-59: Consider using direct update since getSettings() guarantees the row exists.Since
getSettings()creates the row if it doesn't exist, the subsequentinsert().onConflictDoUpdate()will always hit the conflict case. A directupdate()would be cleaner and more efficient, though the current approach is functionally correct.🔎 Optional: Use direct update
.mutation(({ input }) => { const preset: TerminalPreset = { id: crypto.randomUUID(), ...input, }; const row = getSettings(); const presets = row.terminalPresets ?? []; presets.push(preset); - localDb - .insert(settings) - .values({ id: 1, terminalPresets: presets }) - .onConflictDoUpdate({ - target: settings.id, - set: { terminalPresets: presets }, - }) - .run(); + localDb + .update(settings) + .set({ terminalPresets: presets }) + .where(eq(settings.id, 1)) + .run(); return preset; }),Note: This would require importing
eqfromdrizzle-orm.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
139-147: Consider optimizing tab order calculation.The current approach fetches all workspaces to calculate the maximum tab order. For better performance with many workspaces, consider using Drizzle's
max()aggregation function.🔎 Proposed optimization
+import { max } from "drizzle-orm"; // Get max tab order for this project's workspaces -const projectWorkspaces = localDb +const result = localDb .select() + .select({ maxTabOrder: max(workspaces.tabOrder) }) .from(workspaces) .where(eq(workspaces.projectId, input.projectId)) - .all(); -const maxTabOrder = - projectWorkspaces.length > 0 - ? Math.max(...projectWorkspaces.map((w) => w.tabOrder)) - : -1; + .get(); +const maxTabOrder = result?.maxTabOrder ?? -1;Note: This same pattern is used in multiple places (lines 222-230, 292-296, 328-337, etc.) and could benefit from extraction into a helper function.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
apps/admin/package.jsonapps/admin/src/proxy.tsapps/desktop/electron-builder.tsapps/desktop/electron.vite.config.tsapps/desktop/package.jsonapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/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/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/db/index.tsapps/desktop/src/main/lib/db/schemas.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxpackages/db/src/index.tspackages/local-db/drizzle.config.tspackages/local-db/drizzle/0000_initial_schema.sqlpackages/local-db/drizzle/meta/0000_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/package.jsonpackages/local-db/src/index.tspackages/local-db/src/schema/index.tspackages/local-db/src/schema/relations.tspackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.tspackages/local-db/tsconfig.json
💤 Files with no reviewable changes (3)
- apps/desktop/src/main/lib/db/schemas.ts
- apps/desktop/src/main/lib/db/index.ts
- packages/db/src/index.ts
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tspackages/local-db/src/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/electron.vite.config.tspackages/local-db/src/schema/relations.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/electron-builder.tspackages/local-db/src/schema/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsxpackages/local-db/drizzle.config.tsapps/desktop/src/main/index.tspackages/local-db/src/schema/schema.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/changes/branches.tspackages/local-db/src/schema/zod.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tspackages/local-db/src/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/electron.vite.config.tspackages/local-db/src/schema/relations.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/electron-builder.tspackages/local-db/src/schema/index.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsxpackages/local-db/drizzle.config.tsapps/desktop/src/main/index.tspackages/local-db/src/schema/schema.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/changes/branches.tspackages/local-db/src/schema/zod.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/electron.vite.config.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/electron-builder.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsxapps/desktop/src/main/index.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
**/{components,features}/**/[!.]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
**/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsxapps/desktop/src/main/index.ts
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + TailwindCSS v4 + shadcn/ui for UI development
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
**/{components,features}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,features}/**/*.tsx: Nest components in parent's components/ folder if used only once, promote to highest shared parent's components/ if used 2+ times
Use one component per file - do not combine multiple components in a single file
Files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/main/index.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/api/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Create database migrations by modifying Drizzle schema in packages/db/src/schema and running drizzle-kit generate with snake_case migration names
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
Applied to files:
packages/local-db/src/index.tspackages/local-db/package.jsonapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.tsapps/admin/package.jsonapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tspackages/local-db/src/schema/relations.tspackages/local-db/tsconfig.jsonapps/desktop/src/lib/trpc/routers/terminal/terminal.tspackages/local-db/src/schema/index.tspackages/local-db/drizzle/0000_initial_schema.sqlpackages/local-db/drizzle.config.tsapps/desktop/src/main/index.tspackages/local-db/src/schema/schema.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/changes/branches.tspackages/local-db/drizzle/meta/0000_snapshot.jsonapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to src/components/{ui,ai-elements,react-flow}/**/*.tsx : Use kebab-case single files for shadcn/ui components in src/components/ui/, src/components/ai-elements, and src/components/react-flow/ directories
Applied to files:
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsx
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/api/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations
Applied to files:
packages/local-db/package.jsonapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.tsapps/admin/package.jsonpackages/local-db/src/schema/relations.tspackages/local-db/tsconfig.jsonapps/desktop/src/lib/trpc/routers/terminal/terminal.tspackages/local-db/drizzle.config.tsapps/desktop/src/main/index.tspackages/local-db/src/schema/schema.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/*.{tsx,css} : Use React + TailwindCSS v4 + shadcn/ui for UI development
Applied to files:
apps/desktop/package.json
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxpackages/local-db/tsconfig.jsonapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsx
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Applied to files:
apps/desktop/scripts/copy-native-modules.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/electron.vite.config.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/src/lib/trpc/routers/external/helpers.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/lib/trpc/routers/external/helpers.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx} : Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports
Applied to files:
apps/desktop/electron.vite.config.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/{components,features}/**/[!.]*.tsx : Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Applied to files:
packages/local-db/src/schema/index.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Create database migrations by modifying Drizzle schema in packages/db/src/schema and running drizzle-kit generate with snake_case migration names
Applied to files:
packages/local-db/drizzle/0000_initial_schema.sqlpackages/local-db/drizzle.config.tspackages/local-db/drizzle/meta/0000_snapshot.json
🧬 Code graph analysis (10)
apps/desktop/src/main/lib/local-db/index.ts (2)
apps/desktop/src/main/lib/app-environment.ts (2)
DB_PATH(8-8)SUPERSET_HOME_DIR(5-5)packages/local-db/src/schema/schema.ts (4)
projects(14-31)worktrees(39-57)workspaces(65-88)settings(96-103)
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)packages/local-db/src/schema/schema.ts (3)
worktrees(39-57)SelectWorkspace(91-91)projects(14-31)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)
packages/local-db/src/schema/schema.ts (3)
packages/local-db/src/schema/zod.ts (5)
GitStatus(12-12)GitHubStatus(48-48)WorkspaceType(68-68)ExternalApp(97-97)TerminalPreset(61-61)apps/desktop/src/lib/trpc/routers/external/helpers.ts (1)
ExternalApp(97-97)apps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.ts (1)
TerminalPreset(3-3)
apps/desktop/src/lib/trpc/routers/external/index.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)packages/local-db/src/schema/schema.ts (1)
settings(96-103)
apps/desktop/src/lib/trpc/routers/changes/branches.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)packages/local-db/src/schema/schema.ts (1)
worktrees(39-57)
packages/local-db/src/schema/zod.ts (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.ts (1)
TerminalPreset(3-3)apps/desktop/src/lib/trpc/routers/external/helpers.ts (2)
EXTERNAL_APPS(97-97)ExternalApp(97-97)
apps/desktop/src/lib/trpc/routers/config/config.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)packages/local-db/src/schema/schema.ts (1)
projects(14-31)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)packages/local-db/src/schema/schema.ts (5)
projects(14-31)worktrees(39-57)workspaces(65-88)settings(96-103)SelectWorktree(60-60)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
packages/local-db/src/schema/schema.ts (2)
SelectProject(34-34)projects(14-31)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(60-60)apps/desktop/src/lib/trpc/routers/projects/utils/colors/colors.ts (1)
assignRandomColor(3-7)
🪛 GitHub Actions: CI
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/package.json
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/main/lib/local-db/index.ts
[warning] 1-1: assist/source/organizeImports: The imports and exports are not sorted. Safe fix suggested: Organize Imports (Biome).
apps/desktop/src/main/lib/notification-sound.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/scripts/copy-native-modules.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/main/windows/main.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
[warning] 1-1: assist/source/organizeImports: The imports and exports are not sorted. Safe fix suggested: Organize Imports (Biome).
apps/desktop/electron.vite.config.ts
[warning] 49-51: Formatter would have printed: reorganize migrationsSrc path. Fix formatting with the formatter.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/electron-builder.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
packages/local-db/src/schema/index.ts
[warning] 1-1: assist/source/organizeImports: The imports and exports are not sorted. Safe fix suggested: Organize Imports (Biome).
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/main/index.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
packages/local-db/src/schema/schema.ts
[warning] 17-32: Formatter would rearrange field definitions for IDs and defaults. Apply formatting fixes.
apps/desktop/src/lib/trpc/routers/external/index.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/lib/trpc/routers/settings/index.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/lib/trpc/routers/external/helpers.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/admin/src/proxy.ts
[warning] 1-1: assist/source/organizeImports: Imports are not sorted. Safe fix suggested: Organize Imports (Biome).
apps/desktop/src/lib/trpc/routers/changes/branches.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/lib/trpc/routers/config/config.ts
[error] 1-1: Command failed: bun run build exited with exit code 1.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
[warning] 8-8: lint/correctness/noUnusedImports: Unused import 'SelectWorkspace' detected. This import is fixable.
[warning] 3-11: Formatter would have rearranged imports for proper ordering. Apply Biome organize-imports fixes.
apps/desktop/src/lib/trpc/routers/projects/projects.ts
[warning] 1-1: assist/source/organizeImports: The imports and exports are not sorted. Safe fix suggested: Organize Imports (Biome).
🪛 GitHub Check: Test
apps/desktop/src/main/lib/local-db/index.ts
[failure] 43-43: TypeError: app.getAppPath is not a function. (In 'app.getAppPath()'
at getMigrationsDirectory (/home/runner/work/superset/superset/apps/desktop/src/main/lib/local-db/index.ts:43:27)
at /home/runner/work/superset/superset/apps/desktop/src/main/lib/local-db/index.ts:52:26
at loadAndEvaluateModule (2:1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
| function getMigrationsDirectory(): string { | ||
| if (app.isPackaged) { | ||
| return join(process.resourcesPath, "resources/migrations"); | ||
| } | ||
|
|
||
| const isDev = env.NODE_ENV === "development"; | ||
|
|
||
| if (isDev) { | ||
| // Development: source files in monorepo | ||
| return join(app.getAppPath(), "../../packages/local-db/drizzle"); | ||
| } | ||
|
|
||
| // Preview mode: __dirname is dist/main, so go up one level to dist/resources/migrations | ||
| const previewPath = join(__dirname, "../resources/migrations"); | ||
| if (existsSync(previewPath)) { | ||
| return previewPath; | ||
| } | ||
|
|
||
| // Fallback: try monorepo path | ||
| const srcPath = join(app.getAppPath(), "../../packages/local-db/drizzle"); | ||
| if (existsSync(srcPath)) { | ||
| return srcPath; | ||
| } | ||
|
|
||
| console.warn(`[local-db] Migrations directory not found at: ${previewPath}`); | ||
| return previewPath; | ||
| } | ||
|
|
||
| const migrationsFolder = getMigrationsDirectory(); | ||
|
|
||
| const sqlite = new Database(DB_PATH); | ||
| sqlite.pragma("journal_mode = WAL"); | ||
|
|
||
| console.log(`[local-db] Database initialized at: ${DB_PATH}`); | ||
| console.log(`[local-db] Running migrations from: ${migrationsFolder}`); | ||
|
|
||
| export const localDb = drizzle(sqlite, { schema }); | ||
|
|
||
| migrate(localDb, { migrationsFolder }); |
There was a problem hiding this comment.
Test failure: app.getAppPath is not a function in test environment.
The module executes getMigrationsDirectory() and database initialization at import time (lines 52-64). When tests import modules that transitively import this file, Electron's app module isn't available, causing the test failure.
Consider either:
- Lazy initialization: Export an
initLocalDb()function called explicitly during app startup instead of at module load - Guard for test environment: Check if running in a test context before calling Electron APIs
🔎 Option 1: Lazy initialization pattern
-const migrationsFolder = getMigrationsDirectory();
-
-const sqlite = new Database(DB_PATH);
-sqlite.pragma("journal_mode = WAL");
-
-console.log(`[local-db] Database initialized at: ${DB_PATH}`);
-console.log(`[local-db] Running migrations from: ${migrationsFolder}`);
-
-export const localDb = drizzle(sqlite, { schema });
-
-migrate(localDb, { migrationsFolder });
-
-console.log("[local-db] Migrations complete");
+let _localDb: ReturnType<typeof drizzle<typeof schema>> | null = null;
+
+export function initLocalDb() {
+ if (_localDb) return _localDb;
+
+ const migrationsFolder = getMigrationsDirectory();
+ const sqlite = new Database(DB_PATH);
+ sqlite.pragma("journal_mode = WAL");
+
+ console.log(`[local-db] Database initialized at: ${DB_PATH}`);
+ console.log(`[local-db] Running migrations from: ${migrationsFolder}`);
+
+ _localDb = drizzle(sqlite, { schema });
+ migrate(_localDb, { migrationsFolder });
+
+ console.log("[local-db] Migrations complete");
+ migrateFromLegacyDb();
+
+ return _localDb;
+}
+
+export const localDb = new Proxy({} as ReturnType<typeof drizzle<typeof schema>>, {
+ get(_, prop) {
+ if (!_localDb) {
+ throw new Error("localDb not initialized. Call initLocalDb() first.");
+ }
+ return Reflect.get(_localDb, prop);
+ },
+});Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Test
[failure] 43-43: TypeError: app.getAppPath is not a function. (In 'app.getAppPath()'
at getMigrationsDirectory (/home/runner/work/superset/superset/apps/desktop/src/main/lib/local-db/index.ts:43:27)
at /home/runner/work/superset/superset/apps/desktop/src/main/lib/local-db/index.ts:52:26
at loadAndEvaluateModule (2:1)
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/local-db/index.ts around lines 24-62, the file runs
getMigrationsDirectory() and initializes the Database at module import time
causing test failures because app.getAppPath/app isn't available in test env;
refactor to lazy initialization: remove top-level creation of
sqlite/drizzle/migrate and migrationsFolder, export an async
initLocalDb(options?) function that calls getMigrationsDirectory(), constructs
the Database, sets PRAGMA, logs, initializes drizzle and runs migrate, and
returns (or sets) the localDb instance; ensure getMigrationsDirectory() is safe
in tests by only calling it inside initLocalDb and add a simple guard (if
(process.env.NODE_ENV === 'test') return a noop or throw a clear error) if you
need explicit behavior in tests; update callers to invoke await initLocalDb()
during app startup.
| try { | ||
| const legacyData = JSON.parse(readFileSync(LEGACY_DB_PATH, "utf-8")); | ||
|
|
||
| // Migrate projects | ||
| if (legacyData.projects?.length > 0) { | ||
| for (const p of legacyData.projects) { | ||
| localDb.insert(projects).values({ | ||
| id: p.id, | ||
| mainRepoPath: p.mainRepoPath, | ||
| name: p.name, | ||
| color: p.color, | ||
| tabOrder: p.tabOrder, | ||
| lastOpenedAt: p.lastOpenedAt, | ||
| createdAt: p.createdAt, | ||
| configToastDismissed: p.configToastDismissed, | ||
| defaultBranch: p.defaultBranch, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.projects.length} projects`); | ||
| } | ||
|
|
||
| // Migrate worktrees | ||
| if (legacyData.worktrees?.length > 0) { | ||
| for (const w of legacyData.worktrees) { | ||
| localDb.insert(worktrees).values({ | ||
| id: w.id, | ||
| projectId: w.projectId, | ||
| path: w.path, | ||
| branch: w.branch, | ||
| baseBranch: w.baseBranch, | ||
| createdAt: w.createdAt, | ||
| gitStatus: w.gitStatus, | ||
| githubStatus: w.githubStatus, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.worktrees.length} worktrees`); | ||
| } | ||
|
|
||
| // Migrate workspaces | ||
| if (legacyData.workspaces?.length > 0) { | ||
| for (const ws of legacyData.workspaces) { | ||
| // Get branch from worktree if not set on workspace | ||
| let branch = ws.branch; | ||
| if (!branch && ws.worktreeId) { | ||
| const worktree = legacyData.worktrees?.find( | ||
| (wt: { id: string }) => wt.id === ws.worktreeId | ||
| ); | ||
| branch = worktree?.branch ?? "unknown"; | ||
| } | ||
|
|
||
| localDb.insert(workspaces).values({ | ||
| id: ws.id, | ||
| projectId: ws.projectId, | ||
| worktreeId: ws.worktreeId, | ||
| type: ws.type ?? "worktree", // Default to worktree for legacy data | ||
| branch: branch ?? "unknown", | ||
| name: ws.name, | ||
| tabOrder: ws.tabOrder, | ||
| createdAt: ws.createdAt, | ||
| updatedAt: ws.updatedAt, | ||
| lastOpenedAt: ws.lastOpenedAt, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.workspaces.length} workspaces`); | ||
| } | ||
|
|
||
| // Migrate settings | ||
| if (legacyData.settings) { | ||
| const s = legacyData.settings; | ||
| localDb.insert(settings).values({ | ||
| id: 1, | ||
| lastActiveWorkspaceId: s.lastActiveWorkspaceId, | ||
| lastUsedApp: s.lastUsedApp, | ||
| terminalPresets: s.terminalPresets, | ||
| terminalPresetsInitialized: s.terminalPresetsInitialized, | ||
| selectedRingtoneId: s.selectedRingtoneId, | ||
| }).run(); | ||
| console.log("[local-db] Migrated settings"); | ||
| } | ||
|
|
||
| // Backup the legacy db.json | ||
| const backupPath = `${LEGACY_DB_PATH}.backup`; | ||
| renameSync(LEGACY_DB_PATH, backupPath); | ||
| console.log(`[local-db] Legacy db.json backed up to ${backupPath}`); | ||
|
|
||
| console.log("[local-db] Legacy migration complete!"); | ||
| } catch (error) { | ||
| console.error("[local-db] Failed to migrate legacy data:", error); | ||
| // Don't throw - app can continue with empty db | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Wrap legacy migration in a transaction for atomicity.
If the migration fails partway through (e.g., after inserting some projects but before worktrees), the database will be left in an inconsistent state. The backup won't be created, but the partial data will remain, and subsequent runs will skip migration because projectCount > 0.
🔎 Wrap in transaction
try {
const legacyData = JSON.parse(readFileSync(LEGACY_DB_PATH, "utf-8"));
+ // Use a transaction for atomic migration
+ localDb.transaction((tx) => {
// Migrate projects
if (legacyData.projects?.length > 0) {
for (const p of legacyData.projects) {
- localDb.insert(projects).values({
+ tx.insert(projects).values({
// ... values
}).run();
}
// ... rest of migration using tx instead of localDb
}
+ });
// Backup the legacy db.json
const backupPath = `${LEGACY_DB_PATH}.backup`;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/local-db/index.ts around lines 86-176, the legacy
migration is performed without a DB transaction so partial inserts can leave the
DB inconsistent if an error occurs; wrap the entire migration (all inserts for
projects, worktrees, workspaces, settings and the backup/rename) inside a single
database transaction so it either fully commits or rolls back on error, and move
the rename/backup to occur only after a successful commit; ensure errors inside
the transaction cause a rollback and preserve the original legacy file so
subsequent runs can retry the migration.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The biggest risks are runtime correctness in production and data integrity: getMigrationsDirectory() likely builds an incorrect packaged path (process.resourcesPath + "resources/..."), and DB migrations/legacy migration run at import-time without lifecycle control. Several routers introduce non-atomic multi-step updates (reorder loops, JSON blob settings updates) that can corrupt ordering/state on crashes or concurrent calls. Finally, there’s a clear bug in projects.reorder using eq(projects.tabOrder, projects.tabOrder) as a pseudo-filter.
Additional notes (1)
- Readability |
apps/desktop/src/lib/trpc/routers/projects/projects.ts:38-57
InupsertProject,Date.now()is computed multiple times. That can lead to returning alastOpenedAtthat doesn’t match what’s persisted (tiny difference, but avoidable) and makes testing harder.
This also affects other mutations where timestamps are used in multiple operations.
Summary of changes
What changed
Local DB migration (lowdb ➜ SQLite)
- Introduced a new Electron-local SQLite database layer using
better-sqlite3+ Drizzle (apps/desktop/src/main/lib/local-db/index.ts). - Updated multiple desktop tRPC routers (projects/workspaces/settings/terminal/external/branches/config, etc.) to query/update via
drizzle-orminstead oflowdb’sdb.data. - Removed the legacy lowdb implementation (
apps/desktop/src/main/lib/db/*) and dependency (lowdbremoved fromapps/desktop/package.json).
New package: @superset/local-db
- Added
packages/local-db/containing Drizzle SQLite schema, relations, and Zod types/validators. - Added initial SQL migration
drizzle/0000_initial_schema.sql+ Drizzle meta journal/snapshot.
Build & packaging updates
- Added
better-sqlite3native module handling:- Added to
asarUnpackandfilesinapps/desktop/electron-builder.ts. - Marked as Rollup external in
apps/desktop/electron.vite.config.ts. - Added to
copy-native-modules.ts.
- Added to
- Added copying of DB migrations into
dist/resources/migrationsfor preview/packaging.
Admin app change
- Updated admin proxy to import
dbfrom@superset/db/clientand importeqfromdrizzle-orm. - Added
drizzle-ormdependency toapps/admin/package.json.
Misc
packages/db/src/index.tsno longer re-exports Drizzle operators (e.g.eq,and).
| const migrationsFolder = getMigrationsDirectory(); | ||
|
|
||
| const sqlite = new Database(DB_PATH); | ||
| sqlite.pragma("journal_mode = WAL"); | ||
|
|
||
| console.log(`[local-db] Database initialized at: ${DB_PATH}`); | ||
| console.log(`[local-db] Running migrations from: ${migrationsFolder}`); | ||
|
|
||
| export const localDb = drizzle(sqlite, { schema }); | ||
|
|
||
| migrate(localDb, { migrationsFolder }); | ||
|
|
||
| console.log("[local-db] Migrations complete"); | ||
|
|
||
| /** | ||
| * Migrate data from legacy db.json (lowdb) to SQLite. | ||
| * Only runs if: | ||
| * 1. db.json exists | ||
| * 2. SQLite projects table is empty (first run after migration) | ||
| */ | ||
| function migrateFromLegacyDb(): void { | ||
| if (!existsSync(LEGACY_DB_PATH)) { | ||
| return; | ||
| } | ||
|
|
||
| // Check if SQLite is empty | ||
| const projectCount = localDb.select({ count: count() }).from(projects).get(); | ||
| if (projectCount && projectCount.count > 0) { | ||
| console.log("[local-db] SQLite already has data, skipping legacy migration"); | ||
| return; | ||
| } | ||
|
|
||
| console.log("[local-db] Migrating data from legacy db.json..."); | ||
|
|
||
| try { | ||
| const legacyData = JSON.parse(readFileSync(LEGACY_DB_PATH, "utf-8")); | ||
|
|
||
| // Migrate projects | ||
| if (legacyData.projects?.length > 0) { | ||
| for (const p of legacyData.projects) { | ||
| localDb.insert(projects).values({ | ||
| id: p.id, | ||
| mainRepoPath: p.mainRepoPath, | ||
| name: p.name, | ||
| color: p.color, | ||
| tabOrder: p.tabOrder, | ||
| lastOpenedAt: p.lastOpenedAt, | ||
| createdAt: p.createdAt, | ||
| configToastDismissed: p.configToastDismissed, | ||
| defaultBranch: p.defaultBranch, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.projects.length} projects`); | ||
| } | ||
|
|
||
| // Migrate worktrees | ||
| if (legacyData.worktrees?.length > 0) { | ||
| for (const w of legacyData.worktrees) { | ||
| localDb.insert(worktrees).values({ | ||
| id: w.id, | ||
| projectId: w.projectId, | ||
| path: w.path, | ||
| branch: w.branch, | ||
| baseBranch: w.baseBranch, | ||
| createdAt: w.createdAt, | ||
| gitStatus: w.gitStatus, | ||
| githubStatus: w.githubStatus, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.worktrees.length} worktrees`); | ||
| } | ||
|
|
||
| // Migrate workspaces | ||
| if (legacyData.workspaces?.length > 0) { | ||
| for (const ws of legacyData.workspaces) { | ||
| // Get branch from worktree if not set on workspace | ||
| let branch = ws.branch; | ||
| if (!branch && ws.worktreeId) { | ||
| const worktree = legacyData.worktrees?.find( | ||
| (wt: { id: string }) => wt.id === ws.worktreeId | ||
| ); | ||
| branch = worktree?.branch ?? "unknown"; | ||
| } | ||
|
|
||
| localDb.insert(workspaces).values({ | ||
| id: ws.id, | ||
| projectId: ws.projectId, | ||
| worktreeId: ws.worktreeId, | ||
| type: ws.type ?? "worktree", // Default to worktree for legacy data | ||
| branch: branch ?? "unknown", | ||
| name: ws.name, | ||
| tabOrder: ws.tabOrder, | ||
| createdAt: ws.createdAt, | ||
| updatedAt: ws.updatedAt, | ||
| lastOpenedAt: ws.lastOpenedAt, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.workspaces.length} workspaces`); | ||
| } | ||
|
|
||
| // Migrate settings | ||
| if (legacyData.settings) { | ||
| const s = legacyData.settings; | ||
| localDb.insert(settings).values({ | ||
| id: 1, | ||
| lastActiveWorkspaceId: s.lastActiveWorkspaceId, | ||
| lastUsedApp: s.lastUsedApp, | ||
| terminalPresets: s.terminalPresets, | ||
| terminalPresetsInitialized: s.terminalPresetsInitialized, | ||
| selectedRingtoneId: s.selectedRingtoneId, | ||
| }).run(); | ||
| console.log("[local-db] Migrated settings"); | ||
| } | ||
|
|
||
| // Backup the legacy db.json | ||
| const backupPath = `${LEGACY_DB_PATH}.backup`; | ||
| renameSync(LEGACY_DB_PATH, backupPath); | ||
| console.log(`[local-db] Legacy db.json backed up to ${backupPath}`); | ||
|
|
||
| console.log("[local-db] Legacy migration complete!"); | ||
| } catch (error) { | ||
| console.error("[local-db] Failed to migrate legacy data:", error); | ||
| // Don't throw - app can continue with empty db | ||
| } | ||
| } | ||
|
|
||
| migrateFromLegacyDb(); | ||
|
|
There was a problem hiding this comment.
migrate(localDb, { migrationsFolder }) and migrateFromLegacyDb() run at import time. That makes module evaluation have side effects (disk IO + schema changes) and can happen earlier than Electron’s app.whenReady() lifecycle. It also makes tests / secondary processes (e.g. preload) unexpectedly touch the DB.
This should be an explicit initialization step (e.g. initLocalDb()), ideally called once from main after app.whenReady() or after SUPERSET_HOME_DIR is guaranteed to exist. If you want to preserve “import triggers init”, at least guard it so it only runs in the main process and only once.
Suggestion
Consider refactoring to an explicit initializer and making the exported handle lazily created.
Example direction:
let _localDb: ReturnType<typeof drizzle> | null = null;
export function initLocalDb(): LocalDb {
if (_localDb) return _localDb;
const migrationsFolder = getMigrationsDirectory();
const sqlite = new Database(DB_PATH);
sqlite.pragma("journal_mode = WAL");
const db = drizzle(sqlite, { schema });
migrate(db, { migrationsFolder });
migrateFromLegacyDb(db);
_localDb = db;
return db;
}
export function getLocalDb(): LocalDb {
if (!_localDb) throw new Error("Local DB not initialized");
return _localDb;
}Then in main/index.ts call initLocalDb() after await app.whenReady().
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| function getMigrationsDirectory(): string { | ||
| if (app.isPackaged) { | ||
| return join(process.resourcesPath, "resources/migrations"); | ||
| } |
There was a problem hiding this comment.
The migration folder path resolution is inconsistent between build-time copy and runtime expectations:
electron.vite.config.tscopies from../../packages/local-db/drizzletodist/resources/migrations.electron-builder.tscopies fromdist/resources/migrationstoresources/migrations.- But
getMigrationsDirectory()(packaged) returnsprocess.resourcesPath + "resources/migrations".
In packaged Electron apps, process.resourcesPath usually already points at .../Contents/Resources (macOS) / resources (Windows/Linux). Appending an extra resources/ often produces .../Resources/resources/migrations (double resources), which will break migrations in production.
This is a correctness issue: prod users may not get migrations, and the DB schema may never be created.
Suggestion
Fix packaged path to match where electron-builder places the folder.
If you copy to the app’s resources/migrations directory, the runtime path should typically be:
if (app.isPackaged) {
return join(process.resourcesPath, "migrations");
}Or, if you intentionally want resources/migrations under a nested directory, make the electron-builder copy match it and document it.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| /** | ||
| * Migrate data from legacy db.json (lowdb) to SQLite. | ||
| * Only runs if: | ||
| * 1. db.json exists | ||
| * 2. SQLite projects table is empty (first run after migration) | ||
| */ | ||
| function migrateFromLegacyDb(): void { | ||
| if (!existsSync(LEGACY_DB_PATH)) { | ||
| return; | ||
| } | ||
|
|
||
| // Check if SQLite is empty | ||
| const projectCount = localDb.select({ count: count() }).from(projects).get(); | ||
| if (projectCount && projectCount.count > 0) { | ||
| console.log("[local-db] SQLite already has data, skipping legacy migration"); | ||
| return; | ||
| } | ||
|
|
||
| console.log("[local-db] Migrating data from legacy db.json..."); | ||
|
|
||
| try { | ||
| const legacyData = JSON.parse(readFileSync(LEGACY_DB_PATH, "utf-8")); | ||
|
|
||
| // Migrate projects | ||
| if (legacyData.projects?.length > 0) { | ||
| for (const p of legacyData.projects) { | ||
| localDb.insert(projects).values({ | ||
| id: p.id, | ||
| mainRepoPath: p.mainRepoPath, | ||
| name: p.name, | ||
| color: p.color, | ||
| tabOrder: p.tabOrder, | ||
| lastOpenedAt: p.lastOpenedAt, | ||
| createdAt: p.createdAt, | ||
| configToastDismissed: p.configToastDismissed, | ||
| defaultBranch: p.defaultBranch, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.projects.length} projects`); | ||
| } | ||
|
|
||
| // Migrate worktrees | ||
| if (legacyData.worktrees?.length > 0) { | ||
| for (const w of legacyData.worktrees) { | ||
| localDb.insert(worktrees).values({ | ||
| id: w.id, | ||
| projectId: w.projectId, | ||
| path: w.path, | ||
| branch: w.branch, | ||
| baseBranch: w.baseBranch, | ||
| createdAt: w.createdAt, | ||
| gitStatus: w.gitStatus, | ||
| githubStatus: w.githubStatus, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.worktrees.length} worktrees`); | ||
| } | ||
|
|
||
| // Migrate workspaces | ||
| if (legacyData.workspaces?.length > 0) { | ||
| for (const ws of legacyData.workspaces) { | ||
| // Get branch from worktree if not set on workspace | ||
| let branch = ws.branch; | ||
| if (!branch && ws.worktreeId) { | ||
| const worktree = legacyData.worktrees?.find( | ||
| (wt: { id: string }) => wt.id === ws.worktreeId | ||
| ); | ||
| branch = worktree?.branch ?? "unknown"; | ||
| } | ||
|
|
||
| localDb.insert(workspaces).values({ | ||
| id: ws.id, | ||
| projectId: ws.projectId, | ||
| worktreeId: ws.worktreeId, | ||
| type: ws.type ?? "worktree", // Default to worktree for legacy data | ||
| branch: branch ?? "unknown", | ||
| name: ws.name, | ||
| tabOrder: ws.tabOrder, | ||
| createdAt: ws.createdAt, | ||
| updatedAt: ws.updatedAt, | ||
| lastOpenedAt: ws.lastOpenedAt, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.workspaces.length} workspaces`); | ||
| } | ||
|
|
||
| // Migrate settings | ||
| if (legacyData.settings) { | ||
| const s = legacyData.settings; | ||
| localDb.insert(settings).values({ | ||
| id: 1, | ||
| lastActiveWorkspaceId: s.lastActiveWorkspaceId, | ||
| lastUsedApp: s.lastUsedApp, | ||
| terminalPresets: s.terminalPresets, | ||
| terminalPresetsInitialized: s.terminalPresetsInitialized, | ||
| selectedRingtoneId: s.selectedRingtoneId, | ||
| }).run(); | ||
| console.log("[local-db] Migrated settings"); | ||
| } | ||
|
|
||
| // Backup the legacy db.json | ||
| const backupPath = `${LEGACY_DB_PATH}.backup`; | ||
| renameSync(LEGACY_DB_PATH, backupPath); | ||
| console.log(`[local-db] Legacy db.json backed up to ${backupPath}`); | ||
|
|
||
| console.log("[local-db] Legacy migration complete!"); | ||
| } catch (error) { | ||
| console.error("[local-db] Failed to migrate legacy data:", error); | ||
| // Don't throw - app can continue with empty db | ||
| } |
There was a problem hiding this comment.
migrateFromLegacyDb() parses unvalidated JSON (JSON.parse(readFileSync(...))) and then writes many rows. Two issues:
- There’s no transaction. If the app crashes mid-migration, you can end up with a partially migrated DB and the original db.json may still get renamed (or not), leading to hard-to-recover states.
- The “empty check” only looks at the
projectstable count. If a previous attempt partially inserted worktrees/workspaces/settings but not projects (or vice versa), you can skip/redo incorrectly.
This is a data integrity risk.
Suggestion
Wrap the migration in a SQLite transaction and use a more robust “already migrated” marker.
Minimal improvement:
sqlite.transaction(() => {
// insert projects/worktrees/workspaces/settings
// only rename legacy db.json *after* transaction succeeds
})();Better: create a meta table or a settings.migratedFromLowdbAt column and set it on success.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| try { | ||
| const legacyData = JSON.parse(readFileSync(LEGACY_DB_PATH, "utf-8")); | ||
|
|
||
| // Migrate projects | ||
| if (legacyData.projects?.length > 0) { | ||
| for (const p of legacyData.projects) { | ||
| localDb.insert(projects).values({ | ||
| id: p.id, | ||
| mainRepoPath: p.mainRepoPath, | ||
| name: p.name, | ||
| color: p.color, | ||
| tabOrder: p.tabOrder, | ||
| lastOpenedAt: p.lastOpenedAt, | ||
| createdAt: p.createdAt, | ||
| configToastDismissed: p.configToastDismissed, | ||
| defaultBranch: p.defaultBranch, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.projects.length} projects`); | ||
| } | ||
|
|
||
| // Migrate worktrees | ||
| if (legacyData.worktrees?.length > 0) { | ||
| for (const w of legacyData.worktrees) { | ||
| localDb.insert(worktrees).values({ | ||
| id: w.id, | ||
| projectId: w.projectId, | ||
| path: w.path, | ||
| branch: w.branch, | ||
| baseBranch: w.baseBranch, | ||
| createdAt: w.createdAt, | ||
| gitStatus: w.gitStatus, | ||
| githubStatus: w.githubStatus, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.worktrees.length} worktrees`); | ||
| } | ||
|
|
||
| // Migrate workspaces | ||
| if (legacyData.workspaces?.length > 0) { | ||
| for (const ws of legacyData.workspaces) { | ||
| // Get branch from worktree if not set on workspace | ||
| let branch = ws.branch; | ||
| if (!branch && ws.worktreeId) { | ||
| const worktree = legacyData.worktrees?.find( | ||
| (wt: { id: string }) => wt.id === ws.worktreeId | ||
| ); | ||
| branch = worktree?.branch ?? "unknown"; | ||
| } | ||
|
|
||
| localDb.insert(workspaces).values({ | ||
| id: ws.id, | ||
| projectId: ws.projectId, | ||
| worktreeId: ws.worktreeId, | ||
| type: ws.type ?? "worktree", // Default to worktree for legacy data | ||
| branch: branch ?? "unknown", | ||
| name: ws.name, | ||
| tabOrder: ws.tabOrder, | ||
| createdAt: ws.createdAt, | ||
| updatedAt: ws.updatedAt, | ||
| lastOpenedAt: ws.lastOpenedAt, | ||
| }).run(); | ||
| } | ||
| console.log(`[local-db] Migrated ${legacyData.workspaces.length} workspaces`); | ||
| } | ||
|
|
||
| // Migrate settings | ||
| if (legacyData.settings) { | ||
| const s = legacyData.settings; | ||
| localDb.insert(settings).values({ | ||
| id: 1, | ||
| lastActiveWorkspaceId: s.lastActiveWorkspaceId, | ||
| lastUsedApp: s.lastUsedApp, | ||
| terminalPresets: s.terminalPresets, | ||
| terminalPresetsInitialized: s.terminalPresetsInitialized, | ||
| selectedRingtoneId: s.selectedRingtoneId, | ||
| }).run(); | ||
| console.log("[local-db] Migrated settings"); | ||
| } |
There was a problem hiding this comment.
Legacy migration uses JSON.parse(readFileSync(...)) on untrusted local files, and then inserts values directly into SQLite. While this is local-only, it still needs basic validation because malformed JSON or unexpected types can lead to runtime crashes or silently corrupted state.
You already have Zod schemas in @superset/local-db; none are used here.
Suggestion
Validate legacyData before inserting.
Pragmatic options:
- Create a
legacyDatabaseSchema(matching the old shape) andsafeParseit. - Or validate each record minimally (e.g., required keys) before insertion.
Fail fast with a clear log and keep the legacy file in place if validation fails.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| .mutation(({ input }) => { | ||
| const { fromIndex, toIndex } = input; | ||
|
|
||
| const activeProjects = localDb | ||
| .select() | ||
| .from(projects) | ||
| .where(eq(projects.tabOrder, projects.tabOrder)) // Just get all with non-null tabOrder | ||
| .all() | ||
| .filter((p) => p.tabOrder !== null) | ||
| .sort((a, b) => (a.tabOrder ?? 0) - (b.tabOrder ?? 0)); | ||
|
|
||
| if ( | ||
| fromIndex < 0 || | ||
| fromIndex >= activeProjects.length || | ||
| toIndex < 0 || | ||
| toIndex >= activeProjects.length | ||
| ) { | ||
| throw new Error("Invalid fromIndex or toIndex"); | ||
| } | ||
|
|
||
| const [removed] = activeProjects.splice(fromIndex, 1); | ||
| activeProjects.splice(toIndex, 0, removed); | ||
| const [removed] = activeProjects.splice(fromIndex, 1); | ||
| activeProjects.splice(toIndex, 0, removed); | ||
|
|
||
| activeProjects.forEach((project, index) => { | ||
| const p = data.projects.find((p) => p.id === project.id); | ||
| if (p) { | ||
| p.tabOrder = index; | ||
| } | ||
| }); | ||
| }); | ||
| for (let i = 0; i < activeProjects.length; i++) { | ||
| localDb | ||
| .update(projects) | ||
| .set({ tabOrder: i }) | ||
| .where(eq(projects.id, activeProjects[i].id)) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
Same issue as workspace reorder: projects.reorder runs N updates without a transaction, and it also builds the ordering by fetching all rows and filtering in JS.
Additionally, because activeProjects is derived from current DB state and then you update each row individually, concurrent reorder requests (or other updates changing tabOrder) can interleave and corrupt ordering.
Suggestion
Do two improvements:
- Use a proper SQL filter (
isNotNull(projects.tabOrder)) +orderBy(projects.tabOrder). - Wrap the update loop in a transaction (or implement a single-statement CASE update).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| .mutation(({ input }) => { | ||
| const { projectId, fromIndex, toIndex } = input; | ||
|
|
||
| const projectWorkspaces = localDb | ||
| .select() | ||
| .from(workspaces) | ||
| .where(eq(workspaces.projectId, projectId)) | ||
| .all() | ||
| .sort((a, b) => a.tabOrder - b.tabOrder); | ||
|
|
||
| if ( | ||
| fromIndex < 0 || | ||
| fromIndex >= projectWorkspaces.length || | ||
| toIndex < 0 || | ||
| toIndex >= projectWorkspaces.length | ||
| ) { | ||
| throw new Error("Invalid fromIndex or toIndex"); | ||
| } | ||
|
|
||
| const [removed] = projectWorkspaces.splice(fromIndex, 1); | ||
| projectWorkspaces.splice(toIndex, 0, removed); | ||
| const [removed] = projectWorkspaces.splice(fromIndex, 1); | ||
| projectWorkspaces.splice(toIndex, 0, removed); | ||
|
|
||
| projectWorkspaces.forEach((workspace, index) => { | ||
| const ws = data.workspaces.find((w) => w.id === workspace.id); | ||
| if (ws) { | ||
| ws.tabOrder = index; | ||
| } | ||
| }); | ||
| }); | ||
| for (let i = 0; i < projectWorkspaces.length; i++) { | ||
| localDb | ||
| .update(workspaces) | ||
| .set({ tabOrder: i }) | ||
| .where(eq(workspaces.id, projectWorkspaces[i].id)) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
Project/workspace reorder operations were converted to loops that execute N separate UPDATEs. That increases IO and makes the reorder non-atomic (a crash mid-loop leaves inconsistent tab orders). This is especially risky because tabOrder drives UI ordering.
For SQLite, you should either (a) wrap the loop in a transaction, or (b) perform a batch update strategy (e.g. temporary offsets, CASE expression) so the reorder is atomic.
Suggestion
At minimum, wrap the sequential updates in a transaction.
With better-sqlite3 you can do:
const tx = sqlite.transaction(() => {
for (let i = 0; i < projectWorkspaces.length; i++) {
localDb.update(workspaces)
.set({ tabOrder: i })
.where(eq(workspaces.id, projectWorkspaces[i].id))
.run();
}
});
tx();You’ll need access to the underlying sqlite instance (export it or provide a runInTransaction helper from main/lib/local-db).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| import { localDb } from "./lib/local-db"; | ||
| import { terminalManager } from "./lib/terminal"; | ||
| import { MainWindow } from "./windows/main"; | ||
|
|
||
| // Initialize local SQLite database (runs migrations + legacy data migration on import) | ||
| console.log("[main] Local database ready:", !!localDb); | ||
|
|
There was a problem hiding this comment.
main/index.ts now relies on side effects from importing localDb (comment says migrations + legacy migration run on import). But the code also removed await initDb().
That makes the app lifecycle harder to reason about: DB setup occurs before app.whenReady() and cannot be awaited/handled if it fails. If migrations fail, you’ll still proceed into app startup, potentially causing confusing runtime errors later.
This ties into the import-time side effects in main/lib/local-db/index.ts and should be fixed together.
Suggestion
Move DB initialization into the app.whenReady() block and fail fast if migrations fail.
await app.whenReady();
initLocalDb(); // throws on fatal migration issuesReply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Copy database migrations from local-db package | ||
| const migrationsSrc = resolve(__dirname, "../../packages/local-db/drizzle"); | ||
| const migrationsDest = resolve(devPath, "resources/migrations"); | ||
|
|
||
| if (existsSync(migrationsSrc)) { | ||
| if (existsSync(migrationsDest)) { | ||
| rmSync(migrationsDest, { recursive: true }); | ||
| } | ||
| mkdirSync(migrationsDest, { recursive: true }); | ||
| cpSync(migrationsSrc, migrationsDest, { recursive: true }); |
There was a problem hiding this comment.
copyResourcesPlugin() copies the entire packages/local-db/drizzle directory into resources/migrations. That directory includes meta/ and possibly other non-migration artifacts, and the migrator generally expects SQL migration files (and possibly a meta folder depending on how you run it).
This may work today but is brittle: any future generated artifacts in drizzle/ will get packaged into the app resources unnecessarily. Also, the code comment says these are “migrations”, but you’re actually copying the whole Drizzle output folder.
At minimum, consider copying only *.sql (and whatever the migrator truly requires).
Suggestion
Narrow the copy filter to expected files, e.g.:
cpSync(migrationsSrc, migrationsDest, {
recursive: true,
filter: (src) => src.endsWith(".sql") || src.includes("meta"),
});Or keep the Drizzle folder structure but explicitly justify which subfolders are required.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
apps/desktop/src/main/lib/local-db/index.ts (5)
27-70: Path resolution for packaged app may produce incorrect path.On line 34, the path uses
join(process.resourcesPath, "resources/migrations"). In packaged Electron apps,process.resourcesPathalready points to the resources directory (e.g.,.../Contents/Resourceson macOS), so this could produce a double-nested path like.../Resources/resources/migrations.
72-84: Module-level side effects may cause issues in test environments.Database initialization and migrations run at import time, which can cause test failures when Electron's
appmodule isn't available. Consider lazy initialization via an explicitinitLocalDb()function called afterapp.whenReady().
108-213: Wrap legacy migration in a transaction for atomicity.If the migration fails partway through (e.g., after inserting projects but before worktrees), the database is left in an inconsistent state. The backup won't occur, but partial data remains.
109-109: Validate legacy JSON before inserting.
JSON.parse(readFileSync(...))on untrusted local files without validation can cause runtime crashes or silently corrupted state. Consider using Zod schemas from@superset/local-dbto validate the legacy data structure before migration.
216-218: Consider explicit initialization instead of side-effect execution.
migrateFromLegacyDb()runs at module load time, coupling database migration to import order. Moving this to an explicit initialization step would improve testability and control.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
296-302: Sequential updates without transaction risk inconsistent tab ordering.Shifting
tabOrderfor existing workspaces runs N separateUPDATEstatements. If the process crashes mid-loop, workspaces will have inconsistent ordering. Consider wrapping in a transaction.
999-1005: Reorder loop runs N updates without a transaction.This makes the reorder non-atomic. A crash mid-loop leaves inconsistent
tabOrdervalues, which drives UI ordering.apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
545-551: Reorder loop runs N updates without a transaction.Multiple
UPDATEstatements without a transaction wrapper means a crash mid-loop leavestabOrdervalues inconsistent.
525-531: Tautological query condition always returns all rows.Line 528 uses
eq(projects.tabOrder, projects.tabOrder)which is always true (a column equals itself), effectively returning all projects regardless oftabOrdervalue. The subsequent.filter((p) => p.tabOrder !== null)on line 530 does the actual filtering in JavaScript.Use
isNotNullinstead for a proper SQL filter:🔎 Proposed fix
+import { desc, eq, isNotNull } from "drizzle-orm"; // ... const activeProjects = localDb .select() .from(projects) - .where(eq(projects.tabOrder, projects.tabOrder)) // Just get all with non-null tabOrder + .where(isNotNull(projects.tabOrder)) + .orderBy(projects.tabOrder) .all() - .filter((p) => p.tabOrder !== null) - .sort((a, b) => (a.tabOrder ?? 0) - (b.tabOrder ?? 0)); + .sort((a, b) => (a.tabOrder ?? 0) - (b.tabOrder ?? 0)); // Keep sort for safety
🧹 Nitpick comments (4)
apps/desktop/electron.vite.config.ts (1)
50-63: Consider filtering migrations to required files only.Copying the entire
packages/local-db/drizzledirectory works, but may include generated artifacts beyond what the migrator needs. Drizzle's migrator typically requires.sqlfiles and themeta/directory.Suggested filter (optional)
cpSync(migrationsSrc, migrationsDest, { recursive: true, + filter: (src) => { + const name = src.split('/').pop() ?? ''; + return name.endsWith('.sql') || name === 'meta' || !name.includes('.'); + }, });This is low priority if the current approach works in testing.
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (1)
8-15: Consider aligning return type withgetWorkspacePath.
getWorktreePathreturnsstring | undefinedwhilegetWorkspacePathreturnsstring | null. For consistency within the same module, consider using the same "not found" representation.Optional: Use null consistently
-export function getWorktreePath(worktreeId: string): string | undefined { +export function getWorktreePath(worktreeId: string): string | null { const worktree = localDb .select() .from(worktrees) .where(eq(worktrees.id, worktreeId)) .get(); - return worktree?.path; + return worktree?.path ?? null; }apps/desktop/src/main/lib/local-db/index.ts (1)
92-104: Migration check only inspectsprojectstable count.If a previous migration attempt partially inserted data into other tables (worktrees, workspaces, settings) but failed before inserting projects, those orphaned records would remain and subsequent runs would re-attempt migration, potentially causing duplicates or constraint violations.
Consider using a dedicated migration marker (e.g., a
metatable withmigratedFromLowdbAttimestamp) for more robust detection.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
38-68: Return value after update may have stale fields.In
upsertProject, after updating an existing project (lines 48-52), line 53 returns a manually constructed object:return { ...existing, lastOpenedAt: Date.now(), defaultBranch };This spreads
existing(which has the oldlastOpenedAtvalue read before the update) and overwrites only two fields. While currently correct since only those fields are updated, this pattern can lead to subtle bugs if additional fields are updated in the future but forgotten in the return.Consider using
.returning().get()on the update, or re-fetching the row:🔎 Suggested improvement
if (existing) { - localDb + const updated = localDb .update(projects) .set({ lastOpenedAt: Date.now(), defaultBranch }) .where(eq(projects.id, existing.id)) + .returning() + .get(); - .run(); - return { ...existing, lastOpenedAt: Date.now(), defaultBranch }; + return updated; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/admin/src/proxy.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/test-setup.tspackages/local-db/src/schema/index.tspackages/local-db/src/schema/schema.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsx
- apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
packages/local-db/src/schema/index.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/test-setup.tsapps/desktop/electron.vite.config.tspackages/local-db/src/schema/schema.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
packages/local-db/src/schema/index.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/test-setup.tsapps/desktop/electron.vite.config.tspackages/local-db/src/schema/schema.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.ts
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/test-setup.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/api/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
Applied to files:
packages/local-db/src/schema/index.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/electron.vite.config.tspackages/local-db/src/schema/schema.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/{components,features}/**/[!.]*.tsx : Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Applied to files:
packages/local-db/src/schema/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Applied to files:
packages/local-db/src/schema/index.tsapps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/api/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations
Applied to files:
apps/desktop/src/main/lib/local-db/index.tspackages/local-db/src/schema/schema.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/lib/trpc/routers/projects/projects.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables in src/main/index.ts and electron.vite.config.ts with override: true before other imports
Applied to files:
apps/desktop/electron.vite.config.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/electron.vite.config.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Applied to files:
apps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx} : Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx} : Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
🧬 Code graph analysis (4)
apps/desktop/src/main/lib/local-db/index.ts (2)
apps/desktop/src/main/lib/app-environment.ts (2)
DB_PATH(8-8)SUPERSET_HOME_DIR(5-5)packages/local-db/src/schema/schema.ts (4)
projects(14-39)worktrees(47-69)workspaces(77-108)settings(116-127)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (5)
projects(14-39)worktrees(47-69)workspaces(77-108)settings(116-127)SelectWorktree(72-72)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
packages/local-db/src/schema/schema.ts (2)
SelectProject(42-42)projects(14-39)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)apps/desktop/src/lib/trpc/routers/projects/utils/colors/colors.ts (1)
assignRandomColor(3-7)
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (3)
worktrees(47-69)SelectWorkspace(111-111)projects(14-39)
🔇 Additional comments (27)
apps/admin/src/proxy.ts (1)
2-5: LGTM! Import refactoring follows Drizzle ORM best practices.The import path changes are correct:
- Importing
dbfrom@superset/db/clientseparates the database client from the schema- Importing
eqdirectly fromdrizzle-ormfollows the recommended pattern (operators should come from the ORM library, not re-exported)The behavioral logic remains unchanged, and the query usage at lines 31-33 is still correct.
packages/local-db/src/schema/index.ts (1)
1-3: LGTM!The barrel exports are correctly ordered alphabetically (relations, schema, zod), which satisfies Biome's import/export organization rules.
apps/desktop/test-setup.ts (2)
70-71: LGTM!Good additions for the Electron app mock—
getAppPathandisPackagedare commonly needed for path resolution in the main process.
97-136: Local DB mock structure looks reasonable.The chainable mock API mirrors Drizzle ORM's query builder pattern. One consideration: the mock always returns static values (
null,[],{ id: "test-id" }), which works for basic smoke tests but won't help verify actual query logic.For tests that need to verify database interactions, consider either:
- Using integration tests with a real in-memory SQLite database
- Allowing per-test mock customization via
mockImplementationThe current approach is pragmatic given better-sqlite3's incompatibility with Bun.
apps/desktop/src/main/windows/main.ts (2)
2-3: LGTM!Imports are correctly structured—schema entities from
@superset/local-db, query helper fromdrizzle-orm, and the database instance from the local module.Also applies to: 8-8
92-106: Clean migration to Drizzle ORM queries.The workspace name resolution logic correctly:
- Queries workspace by ID
- Conditionally fetches the related worktree if
worktreeIdexists- Falls back through
workspace.name→worktree.branch→"Workspace"The synchronous
.get()calls are appropriate here since better-sqlite3 is synchronous and this runs in the main process notification handler.apps/desktop/electron.vite.config.ts (2)
38-48: LGTM!Good variable renaming for clarity—
soundsSrc/soundsDestmakes the purpose explicit.
104-104: Correct externalization of native module.
better-sqlite3must remain external since it's a native Node.js addon that can't be bundled by Rollup.apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (2)
1-3: LGTM!Imports are well-organized—external packages first (
@superset/local-db,drizzle-orm), then local modules.
22-43: Clean implementation of workspace path resolution.The branching logic correctly handles both workspace types:
branchtype → fetch project'smainRepoPathworktreetype → fetch worktree'spathThe null coalescing (
?? null) provides explicit null returns when entities aren't found.apps/desktop/src/main/lib/local-db/index.ts (1)
1-15: LGTM!Imports are well-organized, using the new
@superset/local-dbpackage for schema and Drizzle ORM for database operations. This aligns with the learnings to use Drizzle ORM for all database operations.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (7)
3-11: LGTM!Import structure is clean and properly organized. The previous unused
SelectWorkspaceimport has been removed, leaving only the necessarySelectWorktreetype import.
50-54: LGTM!Clean migration to Drizzle ORM query pattern. The
localDb.select().from(projects).where(eq(...)).get()pattern correctly retrieves a single project record.
120-135: Good use of.returning().get()for insert.The pattern correctly inserts and retrieves the new worktree record in a single operation, avoiding a separate query.
162-170: Good use of upsert pattern for settings.The
onConflictDoUpdatepattern correctly handles the singleton settings row, ensuring idempotent updates.
512-515: Type narrowing for workspace.type.The cast
workspace.type as "worktree" | "branch"at line 551 and similar locations (lines 637, etc.) is necessary because Drizzle infersstringfrom the schema's.$type<WorkspaceType>(). This is acceptable but could be improved by ensuring the schema type flows through correctly.
840-850: Null coalescing to undefined is handled correctly.Line 850 uses
?? undefinedto convert the potentialnullfrom.get()toundefinedfor theworktreevariable. This aligns with the conditional logic below that checksif (worktree && project).
891-895: Consider cascade behavior for workspace-worktree deletion.The workspace is deleted first (line 891), then the worktree (line 894). Given the schema has
onDelete: "cascade"onworkspaces.worktreeId, deleting the worktree first would automatically cascade to workspaces. However, the current order works correctly since the workspace deletion doesn't trigger a cascade from worktree.packages/local-db/src/schema/schema.ts (6)
1-9: LGTM!Imports are well-organized, bringing in Drizzle SQLite utilities and the custom Zod types for type-safe JSON columns.
14-39: Well-structured projects table.Good schema design with:
- UUID primary key with auto-generation
- Appropriate indexes on
mainRepoPathandlastOpenedAt- Nullable
tabOrderallowing projects to be "inactive" (not in sidebar)- Boolean mode for
configToastDismissed
47-69: Worktrees table with proper foreign key cascade.The
onDelete: "cascade"onprojectIdensures worktrees are automatically cleaned up when a project is deleted. JSON columns forgitStatusandgithubStatuswith typed generics provide type safety.
77-108: Workspaces table with dual foreign key relationships.Good design where:
projectIdis required with cascade deleteworktreeIdis optional (null for branch-type workspaces) with cascade deletetabOrderis required (unlike projects) since workspaces always appear in UI
116-127: Settings table uses singleton pattern.The
id.default(1)ensures a single settings row. This works well with theonConflictDoUpdateupsert pattern used in the routers.
41-42: Good type exports for Insert/Select patterns.Exporting both
InsertXandSelectXtypes for each table enables type-safe operations throughout the codebase.Also applies to: 71-72, 110-111, 129-130
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
4-8: LGTM!Clean imports from the new
@superset/local-dbpackage and drizzle-orm operators.
70-135: Good URL parsing and validation logic.The
extractRepoNamefunction properly handles:
- HTTP/HTTPS URLs
- SSH-style URLs (git@host:user/repo)
- Query strings and hashes
.gitsuffix stripping- Percent-encoded characters
- Safe filename validation
400-428: Good handling of stale project records.The clone logic properly handles the case where a project record exists but the filesystem path is gone—it deletes the stale record and continues with the clone. This is defensive coding that prevents orphaned records.
saddlepaddle
force-pushed
the
homely-quail-98f31a
branch
from
c7918b6 to
dab84a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
1016-1022: Same atomicity concern ascreateBranchWorkspace.This loop also executes sequential updates without transaction wrapping, risking inconsistent
tabOrdervalues on failure.apps/desktop/src/main/lib/local-db/index.ts (1)
108-214: Wrap legacy migration in a transaction for atomicity.The migration inserts data across multiple tables (projects, worktrees, workspaces, settings) without transaction wrapping. If the app crashes or an error occurs mid-migration, the database will be left in an inconsistent state—some records inserted, others not—and the backup may not be created.
Additionally, the "already migrated" check only examines the
projectstable count (line 98-99). If a previous attempt partially inserted worktrees or workspaces but failed before projects, subsequent runs could produce duplicate or inconsistent data.🔎 Wrap in a transaction for atomic migration
try { const legacyData = JSON.parse(readFileSync(LEGACY_DB_PATH, "utf-8")); + // Use a transaction for atomic migration + sqlite.transaction(() => { // Migrate projects if (legacyData.projects?.length > 0) { for (const p of legacyData.projects) { localDb .insert(projects) .values({ // ... existing values }) .run(); } // ... rest of migration } + })(); // Backup the legacy db.json (only after successful transaction) const backupPath = `${LEGACY_DB_PATH}.backup`;This requires exporting the
sqliteinstance or providing arunInTransactionhelper from this module.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
522-551: Transaction and query issues already flagged.The concerns about missing transaction and the confusing query pattern on line 528 (
eq(projects.tabOrder, projects.tabOrder)) were raised in the previous review.As a quick note: the
eq(projects.tabOrder, projects.tabOrder)condition is a SQL tautology that exploitsNULL ≠ NULLbehavior to exclude NULL rows, but it's non-obvious. UsingisNotNull(projects.tabOrder)fromdrizzle-ormwould be clearer and would also eliminate the redundant JS.filter()on line 530.
🧹 Nitpick comments (5)
apps/desktop/src/main/lib/local-db/index.ts (1)
72-84: Consider lazy initialization to avoid test environment issues.Database initialization and migrations run at module import time (lines 72-84), which can cause issues in test environments where Electron's
appmodule isn't available. While theisElectronguard at lines 29-31 provides some protection, the initialization still occurs eagerly.A lazy initialization pattern (as suggested in past reviews) would allow explicit control over when the database is initialized:
🔎 Lazy initialization pattern
-const migrationsFolder = getMigrationsDirectory(); - -const sqlite = new Database(DB_PATH); -sqlite.pragma("journal_mode = WAL"); - -console.log(`[local-db] Database initialized at: ${DB_PATH}`); -console.log(`[local-db] Running migrations from: ${migrationsFolder}`); - -export const localDb = drizzle(sqlite, { schema }); - -migrate(localDb, { migrationsFolder }); - -console.log("[local-db] Migrations complete"); +let _localDb: ReturnType<typeof drizzle<typeof schema>> | null = null; + +export function initLocalDb() { + if (_localDb) return _localDb; + + const migrationsFolder = getMigrationsDirectory(); + const sqlite = new Database(DB_PATH); + sqlite.pragma("journal_mode = WAL"); + + console.log(`[local-db] Database initialized at: ${DB_PATH}`); + console.log(`[local-db] Running migrations from: ${migrationsFolder}`); + + _localDb = drizzle(sqlite, { schema }); + migrate(_localDb, { migrationsFolder }); + + console.log("[local-db] Migrations complete"); + migrateFromLegacyDb(); + + return _localDb; +} + +export function getLocalDb() { + if (!_localDb) throw new Error("localDb not initialized. Call initLocalDb() first."); + return _localDb; +}Then call
initLocalDb()afterapp.whenReady()in the main process.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
906-943: Consider transaction for delete cleanup operations.The delete mutation performs multiple DB operations (delete workspace, delete worktree, update project tabOrder, update settings) without transaction wrapping. While less critical than reordering (since external operations precede DB cleanup), wrapping these in a transaction would improve atomicity.
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
39-58: Consider extracting the repeated upsert pattern into a helper.The same
insert(...).values({id: 1, ...}).onConflictDoUpdate({target: settings.id, set: {...}}).run()pattern appears 6 times. Extracting it would reduce duplication and make future changes easier.Example helper
function updateSettings(patch: Partial<typeof settings.$inferInsert>) { localDb .insert(settings) .values({ id: 1, ...patch }) .onConflictDoUpdate({ target: settings.id, set: patch, }) .run(); }Then usages simplify to:
updateSettings({ terminalPresets: presets }); updateSettings({ selectedRingtoneId: input.ringtoneId });Also applies to: 73-96, 103-115, 138-145, 157-164
apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
38-68: Consider using.returning()on the update path for consistency.Line 53 manually constructs the return object
{ ...existing, lastOpenedAt: Date.now(), defaultBranch }. If the schema adds fields with DB-side defaults or triggers, this manual construction may diverge from actual DB state.Suggested fix
if (existing) { - localDb + const updated = localDb .update(projects) .set({ lastOpenedAt: Date.now(), defaultBranch }) .where(eq(projects.id, existing.id)) - .run(); - return { ...existing, lastOpenedAt: Date.now(), defaultBranch }; + .returning() + .get(); + return updated; }
447-456: Minor duplication: this insert mirrorsupsertProject.Lines 447-456 duplicate the insert logic from
upsertProject(lines 56-65). Consider reusingupsertProject(clonePath, defaultBranch)here to keep project creation logic centralized.Suggested change
// Create new project - const name = basename(clonePath); const defaultBranch = await getDefaultBranch(clonePath); - const project = localDb - .insert(projects) - .values({ - mainRepoPath: clonePath, - name, - color: assignRandomColor(), - defaultBranch, - }) - .returning() - .get(); + const project = upsertProject(clonePath, defaultBranch);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
apps/admin/package.jsonapps/admin/src/proxy.tsapps/desktop/electron-builder.tsapps/desktop/electron.vite.config.tsapps/desktop/package.jsonapps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/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/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/db/index.tsapps/desktop/src/main/lib/db/schemas.tsapps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/lib/notification-sound.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.tsapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsxapps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsxapps/desktop/test-setup.tspackages/db/src/index.tspackages/local-db/drizzle.config.tspackages/local-db/drizzle/0000_initial_schema.sqlpackages/local-db/drizzle/meta/0000_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/package.jsonpackages/local-db/src/index.tspackages/local-db/src/schema/index.tspackages/local-db/src/schema/relations.tspackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.tspackages/local-db/tsconfig.json
💤 Files with no reviewable changes (4)
- apps/desktop/src/main/lib/db/index.ts
- apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts
- apps/desktop/src/main/lib/db/schemas.ts
- packages/db/src/index.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 (24)
- packages/local-db/src/index.ts
- apps/desktop/scripts/copy-native-modules.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksSummary/ChecksSummary.tsx
- apps/desktop/src/lib/trpc/routers/external/index.ts
- packages/local-db/tsconfig.json
- apps/desktop/src/main/lib/notification-sound.ts
- packages/local-db/package.json
- apps/admin/package.json
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabsCommandDialog/index.tsx
- apps/desktop/src/lib/trpc/routers/config/config.ts
- apps/desktop/src/renderer/screens/main/components/SettingsView/PresetsSettings/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/WorkspaceFooter/components/WorkspaceFooterRight/WorkspaceFooterRight.tsx
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/components/CheckItemRow/CheckItemRow.tsx
- apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts
- packages/local-db/drizzle.config.ts
- packages/local-db/drizzle/meta/0000_snapshot.json
- apps/desktop/package.json
- packages/local-db/src/schema/zod.ts
- packages/local-db/src/schema/schema.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceHoverCard/components/ChecksList/ChecksList.tsx
- apps/desktop/src/main/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
- apps/desktop/electron.vite.config.ts
- apps/desktop/src/lib/trpc/routers/external/helpers.ts
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid using any type in TypeScript - maintain type safety unless absolutely necessary
Files:
packages/local-db/src/schema/index.tsapps/desktop/src/main/lib/local-db/index.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/test-setup.tspackages/local-db/src/schema/relations.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Files:
packages/local-db/src/schema/index.tsapps/desktop/src/main/lib/local-db/index.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/test-setup.tspackages/local-db/src/schema/relations.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
apps/desktop/src/main/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Accept object parameters in IPC handlers - do not use positional parameters in ipcMain.handle()
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/main/windows/main.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/local-db/index.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/electron-builder.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tsapps/desktop/test-setup.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
**/{components,features}/**/[!.]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
**/{components,features}/**/*.{ts,tsx,test.ts,test.tsx,stories.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + TailwindCSS v4 + shadcn/ui for UI development
Files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
**/{components,features}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,features}/**/*.tsx: Nest components in parent's components/ folder if used only once, promote to highest shared parent's components/ if used 2+ times
Use one component per file - do not combine multiple components in a single file
Files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to packages/db/src/**/*.{ts,tsx} : Store database schema in packages/db/src/ and use Drizzle ORM for schema management
Applied to files:
packages/local-db/src/schema/index.tsapps/desktop/src/main/lib/local-db/index.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/projects/projects.tspackages/local-db/drizzle/0000_initial_schema.sqlapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tspackages/local-db/src/schema/relations.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/{components,features}/**/[!.]*.tsx : Organize project structure with one folder per component: ComponentName/ComponentName.tsx with index.ts barrel export
Applied to files:
packages/local-db/src/schema/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Run Biome for formatting, linting, import organization, and safe fixes at the root level using bun run lint:fix
Applied to files:
packages/local-db/src/schema/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/api/src/**/*.{ts,tsx} : Use Drizzle ORM for all database operations
Applied to files:
apps/desktop/src/main/lib/local-db/index.tsapps/admin/src/proxy.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/changes/branches.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.tspackages/local-db/src/schema/relations.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules in renderer process or shared code - use only in main process (src/main/)
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Applies to apps/desktop/src/{main,renderer,preload}/**/*.{ts,tsx} : Use type-safe IPC communication - define channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts
📚 Learning: 2025-12-18T23:19:10.415Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T23:19:10.415Z
Learning: Create database migrations by modifying Drizzle schema in packages/db/src/schema and running drizzle-kit generate with snake_case migration names
Applied to files:
packages/local-db/drizzle/0000_initial_schema.sql
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use alias as defined in `tsconfig.json` when possible
Applied to files:
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx
🧬 Code graph analysis (8)
apps/desktop/src/main/lib/local-db/index.ts (2)
apps/desktop/src/main/lib/app-environment.ts (2)
DB_PATH(8-8)SUPERSET_HOME_DIR(5-5)packages/local-db/src/schema/schema.ts (4)
projects(14-39)worktrees(47-69)workspaces(77-108)settings(116-127)
apps/desktop/src/main/windows/main.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (2)
workspaces(77-108)worktrees(47-69)
apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (3)
worktrees(47-69)SelectWorkspace(111-111)projects(14-39)
apps/desktop/src/lib/trpc/routers/changes/branches.ts (2)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (1)
worktrees(47-69)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
packages/local-db/src/schema/schema.ts (2)
SelectProject(42-42)projects(14-39)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)apps/desktop/src/lib/trpc/routers/projects/utils/colors/colors.ts (1)
assignRandomColor(3-7)
apps/desktop/src/lib/trpc/routers/settings/index.ts (4)
apps/desktop/src/shared/ringtones.ts (2)
RINGTONES(21-129)DEFAULT_RINGTONE_ID(131-131)apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (1)
settings(116-127)packages/local-db/src/schema/zod.ts (1)
TerminalPreset(61-61)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
apps/desktop/src/main/lib/local-db/index.ts (1)
localDb(80-80)packages/local-db/src/schema/schema.ts (5)
projects(14-39)worktrees(47-69)workspaces(77-108)settings(116-127)SelectWorktree(72-72)apps/desktop/src/main/lib/analytics/index.ts (1)
track(45-66)
packages/local-db/src/schema/relations.ts (1)
packages/local-db/src/schema/schema.ts (3)
projects(14-39)worktrees(47-69)workspaces(77-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (28)
apps/desktop/electron-builder.ts (3)
36-40: LGTM! Correct handling of native module.The addition of better-sqlite3 to
asarUnpackis necessary and correct. Native modules with binary dependencies must be unpacked from the ASAR archive to be accessible at runtime. This follows the same pattern as the existing node-pty configuration.
59-63: LGTM! Consistent native module handling.The better-sqlite3 copy rule is correctly configured and follows the same pattern as node-pty. The dual inclusion strategy (in
filesarray +asarUnpack) is necessary for native modules:
- The
filesentry ensures it's included in the ASAR- The
asarUnpackentry (line 36) ensures it's extracted and accessible at runtime
50-55: Migration copy chain is properly configured.The migration copy rule correctly makes Drizzle migrations accessible at runtime. Verified that
electron.vite.config.tsis configured to copy migrations frompackages/local-db/drizzletodist/resources/migrationsduring the Vite build, andelectron-builder.tsproperly copies them fromdist/resources/migrationsto the packaged app'sresources/migrationsdirectory.apps/admin/src/proxy.ts (1)
2-5: LGTM!The import refactoring correctly sources
dbfrom the more specific@superset/db/clientpath andeqdirectly fromdrizzle-orm. This aligns with the PR's broader migration pattern and improves import clarity.apps/desktop/test-setup.ts (2)
70-71: LGTM!Good additions to the Electron app mock.
getAppPathandisPackagedare commonly needed Electron APIs that the code may depend on.
110-148: The mock adequately covers the Drizzle ORM builder APIs actually used in the codebase. SincelocalDb.queryis not referenced anywhere in the code, adding relational query support to this mock is unnecessary.Likely an incorrect or invalid review comment.
apps/desktop/src/lib/trpc/routers/changes/branches.ts (2)
1-3: LGTM!Clean imports following the new local-db pattern. Using
eqfromdrizzle-ormandworktreesschema from@superset/local-dbis consistent with the project's migration approach.
64-87: LGTM!The migration from in-memory db to Drizzle ORM is well-implemented:
- Synchronous
.get()is appropriate for better-sqlite3- The gitStatus update correctly preserves existing status while updating the branch field
- Using
.run()for the update is correct since no return value is neededpackages/local-db/drizzle/0000_initial_schema.sql (2)
1-55: Good schema design with appropriate indexes.The schema includes:
- Proper cascade deletes for referential integrity
- Indexes on foreign keys and frequently queried columns (
last_opened_at,created_at,branch)- Sensible column types with
textfor UUIDs andintegerfor timestamps
24-37: The foreign key ordering is not problematic for this initial schema. PRAGMA foreign_keys setting is not required for creating foreign keys, but it is required for enforcing foreign keys. Since this migration creates all tables in a single transaction before any data insertion, theworkspacestable can safely referenceworktreesbefore it's created—SQLite only validates foreign key constraints during data modification operations, not during table creation.packages/local-db/src/schema/index.ts (1)
1-3: LGTM!Clean barrel export with alphabetically sorted exports, satisfying Biome linting requirements.
apps/desktop/src/renderer/components/OpenInButton/OpenInButton.tsx (1)
1-1: LGTM!The
import typesyntax ensures only TypeScript types are imported without pulling in any Node.js runtime code from@superset/local-db. This is safe for the renderer process as per the coding guidelines.apps/desktop/src/main/windows/main.ts (2)
2-3: LGTM!Clean import additions following the local-db migration pattern. Separating schema imports (
@superset/local-db) from the database instance (main/lib/local-db) is a good practice.Also applies to: 8-8
92-106: LGTM!Well-structured migration to the new local-db layer:
- Two-step fetch pattern correctly handles the workspace → worktree relationship
- Synchronous
.get()calls are appropriate for better-sqlite3- Fallback chain (
workspace?.name || worktree?.branch || "Workspace") provides sensible defaults- Error handling preserves resilience with try/catch and console logging
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3)
3-11: LGTM! Imports are correctly organized for the local-db migration.The imports properly bring in the necessary entities (
projects,workspaces,worktrees) from@superset/local-db, theeqoperator fromdrizzle-orm, and thelocalDbinstance. This aligns with the Drizzle ORM pattern used throughout the PR.
53-70: LGTM! Workspace and project lookups correctly migrated to localDb.The queries use the correct Drizzle ORM pattern with synchronous
.get()for single-record lookups, which is appropriate for better-sqlite3. The conditional project lookup properly handles the case when workspace is undefined.
178-197: LGTM! Improved null-safety for worktree lookups.The explicit guard for
!workspace.worktreeId(lines 188-190) is a good addition that handles workspaces without associated worktrees (e.g., branch workspaces). This aligns with the schema whereworktreeIdis nullable.apps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.ts (2)
1-3: LGTM! Imports correctly updated for local-db migration.The imports properly bring in
SelectWorkspacetype and table references from@superset/local-db, along with theeqoperator andlocalDbinstance.
8-43: LGTM! Clean implementation of workspace path resolution.Both functions correctly implement the local-db migration:
getWorktreePathprovides a simple worktree lookup by IDgetWorkspacePathproperly handles both workspace types: returningmainRepoPathfor branch workspaces andworktree.pathfor worktree workspacesThe consistent use of
?? nullensures a predictable return type.packages/local-db/src/schema/relations.ts (1)
1-26: LGTM! Relations correctly define the entity relationships.The relation definitions properly mirror the foreign key constraints in the schema:
projectsRelations: One project has many worktrees and workspacesworktreesRelations: Each worktree belongs to one project and can have many workspacesworkspacesRelations: Each workspace belongs to one project and optionally one worktreeThe field/reference mappings are accurate and follow Drizzle ORM conventions. Based on learnings, this correctly uses Drizzle ORM for schema management in the
packages/layer.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
3-16: LGTM! Imports correctly organized for local-db migration.The imports properly bring in table entities (
projects,settings,workspaces,worktrees) and theSelectWorktreetype from@superset/local-db, along with Drizzle ORM operators (and,desc,eq,isNotNull).
121-171: LGTM! Worktree and workspace creation correctly uses localDb.The insert operations properly use:
.returning().get()to retrieve the inserted record with generated IDonConflictDoUpdatefor settings upsert (appropriate for the singleton settings row)- Correct field mappings matching the schema
528-540: The type annotation correctly reflects Drizzle's behavior: nullable columns returnstring | null, notstring | undefined. However, this is not a breaking change in practice. All consumer code in the renderer uses truthiness checks (if (workspace.worktreeId)) or ternary operators, which safely handle bothnullandundefined. No code attempts to distinguish between them with strict equality checks like=== undefined. The existing usage patterns are compatible with the nullable type.apps/desktop/src/lib/trpc/routers/settings/index.ts (2)
9-18: LGTM!The
getSettings()helper ensures a settings row exists before reads, preventing null-pointer issues across all consumers. Good pattern for a singleton settings row.
120-147: Good self-healing pattern for stale ringtone IDs.The fallback logic that detects invalid stored IDs, logs a warning, and persists the default is a solid defensive approach that prevents UI issues from orphaned configuration.
apps/desktop/src/lib/trpc/routers/projects/projects.ts (3)
78-135: LGTM!The
extractRepoNamefunction handles HTTP/HTTPS URLs, SSH-style URLs, query strings,.gitsuffixes, and percent-encoding robustly. TheSAFE_REPO_NAME_REGEXvalidation prevents path traversal attacks.
151-157: LGTM!Clean migration to Drizzle ORM. The
desc(projects.lastOpenedAt)ordering is correctly applied at the SQL level.
400-428: LGTM!Good stale-entry handling: verifies filesystem presence, updates
lastOpenedAton hit, and cleans up orphaned DB records when the directory no longer exists before proceeding with a fresh clone.
| // Shift existing workspaces to make room at front | ||
| const projectWorkspaces = localDb | ||
| .select() | ||
| .from(workspaces) | ||
| .where(eq(workspaces.projectId, input.projectId)) | ||
| .all(); | ||
| for (const ws of projectWorkspaces) { | ||
| localDb | ||
| .update(workspaces) | ||
| .set({ tabOrder: ws.tabOrder + 1 }) | ||
| .where(eq(workspaces.id, ws.id)) | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
Wrap tab reordering in a transaction for atomicity.
The loop at lines 304-310 executes N separate UPDATE statements to shift workspace tab orders. If the app crashes mid-loop, the database will have inconsistent tabOrder values, causing incorrect UI ordering.
The same issue exists in the reorder mutation (lines 1016-1022).
🔎 Wrap in transaction
You'll need access to the underlying sqlite instance. Consider exporting a transaction helper from main/lib/local-db:
// In local-db/index.ts
export function runInTransaction<T>(fn: () => T): T {
return sqlite.transaction(fn)();
}Then use it:
+import { runInTransaction } from "main/lib/local-db";
+
// Shift existing workspaces to make room at front
const projectWorkspaces = localDb
.select()
.from(workspaces)
.where(eq(workspaces.projectId, input.projectId))
.all();
-for (const ws of projectWorkspaces) {
- localDb
- .update(workspaces)
- .set({ tabOrder: ws.tabOrder + 1 })
- .where(eq(workspaces.id, ws.id))
- .run();
-}
+runInTransaction(() => {
+ for (const ws of projectWorkspaces) {
+ localDb
+ .update(workspaces)
+ .set({ tabOrder: ws.tabOrder + 1 })
+ .where(eq(workspaces.id, ws.id))
+ .run();
+ }
+});Committable suggestion skipped: line range outside the PR's diff.
saddlepaddle
force-pushed
the
homely-quail-98f31a
branch
from
4e99db8 to
dd316e7
Compare
Replace crypto.randomUUID() with uuid v4 to avoid node:crypto module resolution issues in CI environments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
@superset/local-dbpackage with Drizzle ORM schema for SQLiteChanges
packages/local-db/- Drizzle schema, relations, and Zod validatorsprojects,workspaces,terminal,settings,external,config,branches) to use SQLitesrc/main/lib/db/folder and lowdb dependencyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.