Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a typed desktop state persistence system: renames CLI package to Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Electron Main
participant Stores as DesktopStores
participant FS as FileSystem
participant Migrator as LegacyMigrator
participant Storage as DesktopLowdbAdapter
participant Orch as Orchestrators
Main->>Stores: initialize()
activate Stores
Stores->>FS: ensureDesktopStorageDirs()
Stores->>Migrator: shouldMigrate()
alt migration needed
Stores->>Migrator: migrate(envOrch, workspaceOrch, uiStore)
Migrator->>Storage: write migrated domain & UI files
Migrator-->>Stores: migration summary
end
Stores->>Orch: init orchestrators & uiStore & composer
deactivate Stores
sequenceDiagram
participant UI as Renderer
participant IPC as IPC Main
participant Composer as WorkspaceComposer
participant Git as Worktree Manager
participant UiStore as UiStore
UI->>IPC: workspace-activate(workspaceId)
IPC->>Composer: composeWorkspaceState(workspace)
Composer->>Git: scanWorktrees(repoPath)
Git-->>Composer: scannedWorktrees
Composer->>UiStore: readWorkspaceUiState(workspaceId)
Composer-->>IPC: ComposedWorkspaceState
IPC-->>UI: IpcResponse { success, data }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (104)
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: 2
🧹 Nitpick comments (22)
apps/desktop/src/main/lib/workspace-ipcs.ts (1)
775-843: Reduce duplication and avoid redundant dynamic imports in new workspace IPCsThe activation and rescan handlers look logically sound and match the composition plan, but there are two maintainability nits:
- You already have a static
import worktreeManager from "./worktree-manager";at the top of the file; the additionalawait import("./worktree-manager")inside both handlers is redundant and shadows the existing import. Unless you’re explicitly trying to lazy-load here (in which case the top-level import should be removed), consider reusing the existingworktreeManagerto avoid confusion.- The mapping from
composed.worktrees/rescanResult.state.worktreesinto the IPC payload is effectively duplicated betweenworkspace-activateandworkspace-rescan. Extracting a small helper (e.g.toIpcWorktreePayload(...)) would make it easier to evolve the shape later without missing one of the call sites.These are not blockers, but cleaning them up will make the IPC layer easier to maintain.
Also applies to: 845-918
apps/desktop/src/main/lib/desktop-stores.ts (2)
28-54: Clarify async initialization order to avoid subtle lifecycle bugs
initializeAsync()is invoked beforedomainStorage,uiStore, and the orchestrators are assigned, yetrunMigrationIfNeeded()depends on those fields. BecauseinitializeAsyncimmediately hits anawait ensureDesktopStorageDirs(), the orchestrators are assigned before they’re actually used, but this relies on async scheduling details and is easy to regress.Consider moving the
this.initializeAsync();call to the end of the constructor (after all fields are set), or passing the dependencies explicitly intoinitializeAsync/runMigrationIfNeeded, so that usage order is structurally guaranteed rather than timing-dependent.Also applies to: 56-66
68-96: Expose (or document) a readiness signal for migration-dependent consumers
runMigrationIfNeeded()runs fire-and-forget from the constructor, and there’s no public “ready” Promise onDesktopStores. Callers who immediately use the orchestrators after importingdesktop-storescould observe pre-migration state (e.g., workspace lookups failing during a first-run migration from legacy config).If you intend consumers like the new IPC handlers to rely on migrated data, consider:
- Exposing a
ready: Promise<void>onDesktopStoresthat main-process bootstrap code canawaitbefore wiring IPCs, or- Making it explicit in documentation that orchestrator consumers must tolerate “no data yet” on first access.
This isn’t a correctness bug today but tightening the contract will prevent hard-to-reproduce first-run issues.
apps/desktop/docs/STATE_PERSISTENCE_PLAN.md (1)
1-148: Plan aligns well with the implemented architectureThe persistence plan here lines up with the code in this PR (db/ui/cache split under
~/.superset/desktop, Git-backed worktrees, workspace activation/rescan IPCs, and migration from legacy config). Nothing blocking from a design perspective.If you care about polishing wording later, a few of the LanguageTool style/grammar suggestions (e.g., hyphenation, repeated “Do not …” sentences) could be applied, but they’re purely cosmetic.
apps/desktop/src/main/lib/ui-ipcs.ts (1)
38-69: Tighten IPC payload/result typing to shared UI contractsThe handler logic looks solid, but the inline types here are quite loose (
Partial<unknown>,Record<string, unknown>) and may drift from the canonical UI contracts and UiStore shapes over time.A few concrete tweaks to consider:
- For
ui-workspace-update, replaceworktrees?: Record<string, Partial<unknown>>;with the actual
WorktreeUiMetadata(or similar) patch type exported from your UI store / shared IPC types, so callers and handlers stay in sync at compile time.
For
ui-settings-update, havepreferencesuse the same type assettings.preferencesrather thanRecord<string, unknown>, and ensuresettings.preferencesis always initialized to avoid object spread onundefined.In
ui-set-active, you’re returningsuccessfromupdateWorkspaceUiState, but also mutating settings whenactiveWorktreePath === null. If those operations can fail independently, it may be worth either:
- returning a combined success value, or
- short‑circuiting the settings write when the workspace update fails.
Also worth double‑checking that the payload/response shapes here exactly match
apps/desktop/src/shared/ipc-channels/ui.tsso the renderer stays type‑aligned with main.Also applies to: 71-135, 156-194
apps/desktop/src/main/lib/storage/orchestrators/process-orchestrator.ts (1)
17-139: Desktop implementation matches CLI semantics; keep them in lockstepThis orchestrator closely mirrors the CLI
ProcessOrchestrator(ID generation, immutable fields onupdate, agent status handling instop/stopAll, and cascade deletion ofagentSummaries), which is good for consistency across environments.Two small follow‑ups you might consider later:
- If processes ever scale up,
stopAllcould be optimized to batch viaupdateCollection("processes", …)instead of manysetcalls, mirroring whatever pattern you settle on elsewhere.- It could be useful to add a light test that asserts desktop and CLI orchestrators produce identical behavior for a few core flows (create → update → stop → delete) to guard against future drift.
Nothing blocking here from my side.
apps/desktop/src/main/lib/workspace-rescan.ts (1)
15-28: Avoid overlapping rescans when usingsetIntervalwith async work
startPeriodicRescanuses:const interval = setInterval(async () => { await this.rescanWorkspace(workspaceId); }, intervalMs);Because
setIntervaldoesn’t await the promise, a slowrescanWorkspace(e.g., large repo, IO hiccups) can cause overlapping rescans for the same workspace as timers keep firing.To keep at most one rescan in flight per workspace, consider one of:
- Self‑scheduling loop (no
setInterval):private inFlight = new Set<string>(); private scheduleNext(workspaceId: string, intervalMs: number) { const timeout = setTimeout(async () => { if (this.inFlight.has(workspaceId)) { return this.scheduleNext(workspaceId, intervalMs); } this.inFlight.add(workspaceId); try { await this.rescanWorkspace(workspaceId); } finally { this.inFlight.delete(workspaceId); this.scheduleNext(workspaceId, intervalMs); } }, intervalMs); this.rescanIntervals.set(workspaceId, timeout); }
- Or simpler: track an
inFlightflag per workspace and early‑return from the interval callback if a previous rescan hasn’t finished yet.Either approach keeps behavior the same under normal conditions but avoids concurrent rescans hammering storage and Git.
Also applies to: 50-125
apps/desktop/src/main/lib/storage/orchestrators/change-orchestrator.ts (1)
22-31: Aligncreateparameter type with the immutablecreatedAtfield
Changeclearly has acreatedAtfield (you strip it out inupdate), butcreateis typed as:async create(change: Omit<Change, "id" | "timestamp">): Promise<Change> { const newChange: Change = { ...change, id: randomUUID(), createdAt: new Date(), }; }Since
timestampisn’t part ofChangeanymore, this effectively only omitsid, while still allowing callers to passcreatedAteven though you overwrite it.To make the type contract match the runtime behavior, consider:
- async create(change: Omit<Change, "id" | "timestamp">): Promise<Change> { + async create(change: Omit<Change, "id" | "createdAt">): Promise<Change> {This makes
createdAtclearly owned by the orchestrator and avoids confusion about which fields callers are expected to supply.Also applies to: 39-48
apps/desktop/src/main/lib/workspace-composition/types.ts (1)
5-54: Type modeling for composed worktrees/workspaces looks solid; a couple of tiny nitsThe structures for
ScannedWorktree,ComposedWorktree,ComposedWorkspaceState, andRescanResultare consistent withWorktreeInfoandWorktreeUiMetadata, and look serialization‑friendly.Two very small suggestions:
- The
mergedJSDoc (“merged into main”) onScannedWorktree(Line 11) might age better if phrased as “merged into the default branch” (many repos still usemasteror custom defaults).- If you ever introduce non‑local workspace types into composition, you may want to widen
ComposedWorkspaceState["workspace"]fromLocalWorkspaceto a more genericWorkspace(or make the interface generic over the workspace type) to avoid widening later.Nothing blocking here; just future‑proofing.
apps/desktop/src/main/lib/storage/orchestrators/workspace-orchestrator.ts (2)
36-57: Enforce a path invariant for local workspaces increateRight now,
createallowstype === "local"with an undefinedpath, which produces a workspace withtype: "local"but nopath. That can be surprising if other code assumes all local workspaces have a repo path, and it’s easy for future callers to forget the optionalpathargument.Consider asserting that
pathis present whenevertypeis local and failing fast otherwise. For example:async create( environmentId: string, type: WorkspaceType, path?: string, ): Promise<Workspace> { - const workspace: Workspace | LocalWorkspace = - type === "local" && path - ? ({ - id: randomUUID(), - type, - environmentId, - path, - } as LocalWorkspace) - : { - id: randomUUID(), - type, - environmentId, - }; + if (type === "local" && !path) { + throw new Error("Local workspaces require a path"); + } + + const id = randomUUID(); + + const workspace: Workspace | LocalWorkspace = + type === "local" + ? ({ + id, + type, + environmentId, + path: path!, + } as LocalWorkspace) + : { + id, + type, + environmentId, + }; await this.storage.set("workspaces", workspace.id, workspace); return workspace; }This keeps the domain model honest and prevents half‑initialized local workspaces from leaking in.
69-115: Cascade delete behavior is correct but duplicates env orchestrator logic and re‑reads collections in loopsThe cascade in
deletecorrectly walksprocesses → agentSummariesandchanges → fileDiffsbefore deleting the workspace itself, which is great for referential integrity.Two non‑blocking nits:
- The cascade logic is nearly identical to
DesktopEnvironmentOrchestrator.deleteWorkspaceCascade(apps/desktop/src/main/lib/storage/orchestrators/environment-orchestrator.ts, Lines 66–110). Duplicating it in two places means any future schema changes (e.g., new child collections) have to be remembered in both. A shared helper or delegating environment deletion to the workspace orchestrator (or vice versa) would reduce drift risk.getCollection("agentSummaries")andgetCollection("fileDiffs")are called inside the loops, so the entire collection is re‑loaded for each process/change. If the adapter does real I/O per call, you can cheaply micro‑optimize by reading each collection once per delete and then filtering it in memory.Not blockers, but worth tightening when you touch this path next.
apps/desktop/src/main/lib/storage/orchestrators/environment-orchestrator.ts (1)
47-110: Environment cascade delete is correct but duplicates workspace orchestrator logicThe environment
delete+deleteWorkspaceCascadeimplementation correctly walks and deletes childworkspaces,processes,agentSummaries,changes, andfileDiffs, then the environment itself — behavior looks consistent with the workspace orchestrator.Two non‑blocking points:
- The body of
deleteWorkspaceCascadeis effectively the same as the cascade inDesktopWorkspaceOrchestrator.delete(apps/desktop/src/main/lib/storage/orchestrators/workspace-orchestrator.ts, Lines 69–115). Centralizing this logic (e.g., shared helper or delegating to the workspace orchestrator) would avoid divergence if you ever add more child entities.- As with the workspace orchestrator,
getCollection("agentSummaries")andgetCollection("fileDiffs")are called inside loops, which may be unnecessarily expensive if each call does I/O. Loading each collection once per cascade and reusing it in memory would make this path cheaper for environments with many processes/changes.Functionally this looks good; these are just maintainability/performance nits for later.
apps/desktop/src/main/lib/storage/config.ts (1)
11-83: Storage path helpers and directory creation look solid; small optional simplificationThe env override + homedir fallback and the 0700 directory creation for
db,ui, andcacheall look correct and safe. You could optionally simplifyensureDesktopStorageDirsby callingmkdir(..., { recursive: true })unconditionally (and dropping theexistsSyncchecks) to avoid the extra sync I/O, but that's purely cosmetic here.apps/desktop/src/main/lib/workspace-composition/composer.ts (2)
23-190: Workspace composition and rescan logic look good; consider a couple of refinementsThe Git scan + UI merge and the rescan diffing (added/removed/changed) look consistent and should behave as expected.
Two small follow‑ups you might consider:
- In
composeWorkspaceState, if a worktree path changes but you still match by branch,activeWorktreePathmay now point to a stale path and the active worktree will silently clear. You could optionally add a branch‑based fallback for the active selection as well, similar to how you remap UI metadata, to preserve the active tab across renames.- In
rescanWorkspace, you callscanWorktreesonce directly and then again indirectly viacomposeWorkspaceStatewhen computingnewState. IflistWorktrees/isBranchMergedare even moderately expensive, you might consider passingnewScannedinto a helper version ofcomposeWorkspaceState(or factoring the merge logic out) so the second scan can be avoided.
195-216: initializeWorktreeDefaults is sensible; consider dropping the unused workspaceIdThe default terminal tab + initial
WorktreeUiMetadatashape looks correct and consistent with the UI types. TheworkspaceIdparameter isn’t used, though; if you don’t expect to persist or log it here later, you could remove it from the signature to keep the API minimal.apps/desktop/src/main/lib/storage/types.ts (1)
15-63: Domain DB + serialization types are coherent; watch for nested Date fields laterThe split between
DesktopDatabaseandSerializedDesktopDatabase, plus thecreateEmptyDesktopDatabaseinitializer, all look clean and match the intended JSON‑on‑disk shape.One thing to keep in mind:
Serialized<T>is a shallow mapper, so only top‑levelDate/Date | undefinedproperties are converted to strings. If any of the domain types later grow nested objects or arrays that containDates, those won’t be reflected in theSerialized*aliases and you may want to revisit this helper (e.g., make it recursive or explicitly handle the affected fields).apps/desktop/src/main/lib/ui-store/types.ts (1)
37-78: UI state shapes look good; consider renaming theUiStoreinterface to avoid confusionThe
WorktreeUiMetadataandWorkspaceUiStatestructures match how the store and composer use them, so the type layer looks solid.The
UiStoreinterface at the bottom, though, represents a serialized state bag, whileUiStoreinstore.tsis a persistence class. If both are (or will be) re‑exported from a barrel, having two different concepts with the same name can be confusing and can also complicate type imports. It might be worth renaming this interface to something likeUiState/UiStoreStateto keep the distinction clear.apps/desktop/src/main/lib/ui-store/store.ts (1)
24-240: UiStore implementation is solid; small resilience tweak for UI version parsingThe directory setup, per‑file JSON persistence, and merge semantics in
updateWorkspaceUiState/updateWorktreeMetadataall look good and align with the defined types. Error handling with safe fallbacks is also appropriate for a desktop UI store.One minor resilience nit: in
readUiVersion, if the version file is present but contains an invalid value,Number.parseIntwill returnNaN, andneedsMigration()will treat that as “no migration needed” (NaN < 1is false). If you want to be safer in the face of a corruptedui.versionfile, you could normalizeNaNback to0before returning, e.g. via:const raw = Number.parseInt(content.trim(), 10); return Number.isNaN(raw) ? 0 : raw;apps/desktop/src/shared/ipc-channels/ui.ts (1)
22-23: Consider defining proper types for better type safety.Several properties use
unknowntypes which weakens type safety:
- Line 22:
tabs: unknown[]- Line 23:
mosaicTree?: unknown- Line 37:
worktrees?: Record<string, Partial<unknown>>(wherePartial<unknown>is essentially{})If the structure of these properties is known, consider defining proper interfaces. If they're truly dynamic, consider using branded types or documentation to clarify the expected shape.
Also applies to: 37-37
apps/desktop/src/main/lib/storage/lowdb-adapter.ts (1)
122-125: Date regex only matches UTC format.The regex only matches ISO dates ending in
Z(UTC). WhileDate.toISOString()always produces this format, if the JSON files are edited manually or populated from external sources with timezone offsets (e.g.,2024-01-01T12:00:00+05:00), those dates won't be deserialized.This is likely acceptable if all dates are serialized by this class, but consider documenting this assumption.
apps/desktop/src/shared/ipc-channels/workspace.ts (2)
136-147: Define proper types for dynamic structures.Similar to the UI channels, these response types use
unknownfor:
- Line 143:
tabs?: unknown[]- Line 144:
mosaicTree?: unknown- Line 211:
tabs: unknown[]This reduces type safety and IDE support. Consider defining proper interfaces or using branded types to document the expected structure.
Also applies to: 211-211
115-223: Extract shared types to reduce duplication.The response structures for
workspace-activate(lines 117-156) and thestatefield inworkspace-rescan(lines 192-221) are nearly identical. This duplication creates maintenance burden and risk of inconsistencies.Consider extracting a shared type definition like:
type WorkspaceStateResponse = { workspace: { id: string; type: "local"; environmentId: string; path: string; }; worktrees: Array<{ // ... worktree structure }>; ui: { activeWorktreePath: string | null; activeTabId: string | null; }; };Then reference it in both channel definitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
apps/cli/package.json(2 hunks)apps/desktop/docs/STATE_PERSISTENCE_PLAN.md(1 hunks)apps/desktop/electron.vite.config.ts(2 hunks)apps/desktop/package.json(2 hunks)apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/desktop-stores.ts(1 hunks)apps/desktop/src/main/lib/migration/migrator.ts(1 hunks)apps/desktop/src/main/lib/storage/adapter.ts(1 hunks)apps/desktop/src/main/lib/storage/config.ts(1 hunks)apps/desktop/src/main/lib/storage/index.ts(1 hunks)apps/desktop/src/main/lib/storage/lowdb-adapter.ts(1 hunks)apps/desktop/src/main/lib/storage/orchestrators/change-orchestrator.ts(1 hunks)apps/desktop/src/main/lib/storage/orchestrators/environment-orchestrator.ts(1 hunks)apps/desktop/src/main/lib/storage/orchestrators/index.ts(1 hunks)apps/desktop/src/main/lib/storage/orchestrators/process-orchestrator.ts(1 hunks)apps/desktop/src/main/lib/storage/orchestrators/workspace-orchestrator.ts(1 hunks)apps/desktop/src/main/lib/storage/types.ts(1 hunks)apps/desktop/src/main/lib/storage/version.ts(1 hunks)apps/desktop/src/main/lib/types/cli-types.ts(1 hunks)apps/desktop/src/main/lib/ui-ipcs.ts(1 hunks)apps/desktop/src/main/lib/ui-store/index.ts(1 hunks)apps/desktop/src/main/lib/ui-store/store.ts(1 hunks)apps/desktop/src/main/lib/ui-store/types.ts(1 hunks)apps/desktop/src/main/lib/workspace-composition/composer.ts(1 hunks)apps/desktop/src/main/lib/workspace-composition/index.ts(1 hunks)apps/desktop/src/main/lib/workspace-composition/types.ts(1 hunks)apps/desktop/src/main/lib/workspace-ipcs.ts(1 hunks)apps/desktop/src/main/lib/workspace-rescan.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(2 hunks)apps/desktop/src/shared/ipc-channels/ui.ts(1 hunks)apps/desktop/src/shared/ipc-channels/workspace.ts(1 hunks)packages/scripts/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
apps/desktop/src/shared/ipc-channels/index.ts (2)
apps/desktop/src/shared/ipc-channels/window.ts (1)
WindowChannels(7-17)apps/desktop/src/shared/ipc-channels/ui.ts (1)
UiChannels(7-68)
apps/desktop/src/main/lib/ui-ipcs.ts (1)
apps/desktop/src/main/lib/desktop-stores.ts (1)
desktopStores(134-134)
apps/desktop/src/main/lib/storage/adapter.ts (1)
apps/desktop/src/main/lib/storage/types.ts (1)
DesktopDatabase(15-22)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/ui-ipcs.ts (1)
registerUiIPCs(7-195)
apps/desktop/src/main/lib/workspace-composition/types.ts (3)
apps/desktop/src/main/lib/worktree-manager.ts (1)
WorktreeInfo(9-13)apps/desktop/src/main/lib/ui-store/types.ts (1)
WorktreeUiMetadata(37-56)apps/desktop/src/main/lib/types/cli-types.ts (1)
LocalWorkspace(12-12)
apps/desktop/src/main/lib/ui-store/types.ts (2)
apps/desktop/src/shared/types.ts (2)
Tab(44-56)MosaicNode(35-35)apps/desktop/src/main/lib/ui-store/store.ts (1)
UiStore(18-241)
apps/desktop/src/main/lib/storage/version.ts (1)
apps/desktop/src/main/lib/storage/config.ts (1)
getDomainVersionPath(54-56)
apps/desktop/src/main/lib/migration/migrator.ts (5)
apps/desktop/src/main/lib/storage/orchestrators/environment-orchestrator.ts (1)
DesktopEnvironmentOrchestrator(12-111)apps/desktop/src/main/lib/storage/orchestrators/workspace-orchestrator.ts (1)
DesktopWorkspaceOrchestrator(14-116)apps/desktop/src/main/lib/ui-store/store.ts (1)
UiStore(18-241)apps/desktop/src/shared/types.ts (1)
WorkspaceConfig(84-87)apps/desktop/src/main/lib/storage/version.ts (1)
DomainVersion(7-54)
apps/desktop/src/main/lib/workspace-rescan.ts (1)
apps/desktop/src/main/lib/desktop-stores.ts (1)
desktopStores(134-134)
apps/desktop/src/main/lib/storage/orchestrators/process-orchestrator.ts (2)
apps/desktop/src/main/lib/storage/adapter.ts (1)
DesktopStorageAdapter(7-70)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
ProcessOrchestrator(17-140)
apps/desktop/src/shared/ipc-channels/ui.ts (1)
apps/desktop/src/shared/ipc-channels/index.ts (2)
IpcResponse(20-20)NoRequest(21-21)
apps/desktop/src/main/lib/workspace-ipcs.ts (1)
apps/desktop/src/main/lib/desktop-stores.ts (1)
desktopStores(134-134)
apps/desktop/src/main/lib/desktop-stores.ts (10)
apps/desktop/src/main/lib/storage/lowdb-adapter.ts (1)
DesktopLowdbAdapter(21-221)apps/desktop/src/main/lib/ui-store/store.ts (1)
UiStore(18-241)apps/desktop/src/main/lib/workspace-composition/composer.ts (1)
WorkspaceComposer(17-217)apps/desktop/src/main/lib/storage/orchestrators/environment-orchestrator.ts (1)
DesktopEnvironmentOrchestrator(12-111)apps/desktop/src/main/lib/storage/orchestrators/workspace-orchestrator.ts (1)
DesktopWorkspaceOrchestrator(14-116)apps/desktop/src/main/lib/storage/orchestrators/process-orchestrator.ts (1)
DesktopProcessOrchestrator(17-140)apps/desktop/src/main/lib/storage/orchestrators/change-orchestrator.ts (1)
DesktopChangeOrchestrator(12-65)apps/desktop/src/main/lib/storage/config.ts (1)
ensureDesktopStorageDirs(69-83)apps/desktop/src/main/lib/migration/migrator.ts (1)
LegacyMigrator(14-154)apps/desktop/src/main/lib/storage/version.ts (1)
DomainVersion(7-54)
apps/desktop/src/main/lib/storage/lowdb-adapter.ts (3)
apps/desktop/src/main/lib/storage/adapter.ts (1)
DesktopStorageAdapter(7-70)apps/desktop/src/main/lib/storage/types.ts (1)
DesktopDatabase(15-22)apps/desktop/src/main/lib/storage/config.ts (1)
getDomainCollectionPath(47-49)
apps/desktop/src/shared/ipc-channels/workspace.ts (1)
apps/desktop/src/shared/ipc-channels/types.ts (1)
IpcResponse(8-12)
apps/desktop/src/main/lib/storage/orchestrators/environment-orchestrator.ts (2)
apps/desktop/src/main/lib/storage/adapter.ts (1)
DesktopStorageAdapter(7-70)apps/desktop/src/main/lib/types/cli-types.ts (1)
Environment(6-6)
apps/desktop/src/main/lib/storage/orchestrators/change-orchestrator.ts (1)
apps/desktop/src/main/lib/storage/adapter.ts (1)
DesktopStorageAdapter(7-70)
apps/desktop/src/main/lib/workspace-composition/composer.ts (4)
apps/desktop/src/main/lib/ui-store/store.ts (1)
UiStore(18-241)apps/desktop/src/main/lib/ui-store/types.ts (2)
UiStore(74-78)WorktreeUiMetadata(37-56)apps/desktop/src/main/lib/workspace-composition/types.ts (4)
ScannedWorktree(8-13)ComposedWorkspaceState(27-37)ComposedWorktree(18-21)RescanResult(42-54)apps/desktop/src/main/lib/types/cli-types.ts (1)
LocalWorkspace(12-12)
apps/desktop/src/main/lib/ui-store/store.ts (2)
apps/desktop/src/main/lib/ui-store/types.ts (5)
UiStore(74-78)WindowState(11-21)DesktopSettings(26-31)WorkspaceUiState(61-69)WorktreeUiMetadata(37-56)apps/desktop/src/main/lib/storage/config.ts (2)
getDesktopUiDir(31-33)getUiVersionPath(61-63)
apps/desktop/src/main/lib/storage/orchestrators/workspace-orchestrator.ts (1)
apps/desktop/src/main/lib/storage/adapter.ts (1)
DesktopStorageAdapter(7-70)
🪛 LanguageTool
apps/desktop/docs/STATE_PERSISTENCE_PLAN.md
[style] ~9-~9: To elevate your writing, try using a synonym here.
Context: ...ync with Git) and makes the state model hard to evolve. We also want to align domain...
(HARD_TO)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ime dependencies or database engines. - Do not prescribe file‑level code changes; ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~39-~39: Ensure spelling is correct
Context: ...description, prUrl, merged). - Derived (in‑memory / cache) - Worktrees scanned from Git...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~39-~39: Ensure spelling is correct
Context: ..., prUrl, merged). - Derived (in‑memory / cache) - Worktrees scanned from Git (b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~56-~56: Use a hyphen to join words.
Context: ...t one big JSON” requirement. If a single domain DB file is preferred later, keep ...
(QB_NEW_EN_HYPHEN)
[grammar] ~89-~89: Use a hyphen to join words.
Context: ...s.list/create/stop/stopAll following CLI type semantics Use existing type‑safe I...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (17)
packages/scripts/package.json (1)
18-18: Approved: commander dependency bump.The patch version bump from ^14.0.1 to ^14.0.2 is a routine maintenance update that maintains backward compatibility. This aligns with the broader monorepo updates and supports the CLI infrastructure improvements outlined in the PR objectives.
apps/desktop/electron.vite.config.ts (2)
25-30: LGTM! Proper externalization configuration.Excluding
@superset/clifrom externalization ensures the workspace dependency is properly bundled into the main process.
43-47: LGTM! Alias configuration supports monorepo workflow.The alias pointing to the CLI source directory enables direct TypeScript imports, which is appropriate for the monorepo development workflow.
apps/desktop/src/shared/ipc-channels/index.ts (1)
13-13: LGTM! Proper extension of IPC channels.The addition of
UiChannelsfollows the established pattern for extending the IPC surface.Also applies to: 37-38
apps/desktop/package.json (1)
37-37: LGTM! Dependencies align with persistence layer.The addition of
@superset/cli(workspace dependency) andlowdb(file-based storage) support the new persistence infrastructure.Also applies to: 49-49
apps/desktop/src/main/lib/ui-store/index.ts (1)
1-3: LGTM! Clean barrel export.Standard barrel export pattern providing a clean public API for the ui-store module.
apps/desktop/src/main/lib/workspace-composition/index.ts (1)
1-3: LGTM! Clean barrel export.Standard barrel export pattern providing a clean public API for the workspace-composition module.
apps/desktop/src/main/index.ts (1)
15-15: LGTM! UI IPC registration properly integrated.The UI IPC handlers are correctly registered during the startup sequence, following the established pattern for IPC initialization.
Also applies to: 54-54
apps/desktop/src/main/lib/storage/index.ts (1)
1-6: LGTM! Comprehensive storage module exports.Standard barrel export consolidating all storage-related modules into a clean public API.
apps/cli/package.json (2)
2-2: LGTM! Scoped package with type exports.The package rename to
@superset/cliand the addition of type exports enable clean TypeScript type imports across the monorepo. Exposingsrc/typesthrough theexportsfield is an intentional design pattern for type-only imports.Also applies to: 22-28
30-30: Commander version bump is safe—no issues found.Verification confirms that version 14.0.2 introduces no breaking changes compared to 14.0.1, containing only minor improvements including an enhanced negative-number auto-detection test and dev dependency updates.
apps/desktop/src/main/lib/storage/orchestrators/index.ts (1)
1-4: Barrel re-exports look goodConsolidated orchestrator exports here are straightforward and will simplify imports elsewhere. No issues.
apps/desktop/src/main/lib/types/cli-types.ts (1)
1-35: All CLI type re-exports are correctly aligned and properly structuredVerification confirms that all type and value names re-exported in
cli-types.tsmatch their sources in@superset/cli/types/*. The package exports configuration atapps/cli/package.jsoncorrectly supports the import paths, and the split between type-only and value exports (for enums) is idiomatic and correct. No issues found.apps/desktop/src/main/lib/storage/adapter.ts (1)
7-70: Storage adapter contract is clear and type‑safeThe
DesktopStorageAdapterinterface looks well‑designed: collection keys are correctly constrained viaK extends keyof DesktopDatabase, and item‑level access usesDesktopDatabase[K][string]consistently. The surface area (read/write, collection, entity, existence, and clear) is a good fit for the orchestrators using it.No changes needed from my side.
apps/desktop/src/main/lib/storage/lowdb-adapter.ts (3)
169-211: Consider the trade-off between freshness and performance.Each individual operation (
get,set,delete,has) callsawait db.read(), which reads from disk every time. While this ensures data freshness (important in multi-process scenarios), it may impact performance if these methods are called frequently in tight loops.If the desktop app is single-process or if you have other synchronization mechanisms, you might consider caching reads. Otherwise, the current approach is safer for correctness.
Is the desktop app single-process, or could multiple processes access these files concurrently?
72-92: Date deserialization logic looks correct.The recursive deserialization correctly handles nested objects and arrays. The strict ISO date regex minimizes false positives, though there's a theoretical risk of converting string values that happen to match the pattern but aren't dates.
Given the strict format and controlled usage within the desktop storage layer, this risk is negligible.
22-25: Type safety trade-off with lowdb integration.The collections Map and
JSONFilePresetcalls useanytypes, which weakens internal type safety. However, this appears necessary for lowdb integration, and the publicDesktopStorageAdapterinterface maintains type safety at the API boundary.This trade-off is reasonable, but be aware that errors inside the adapter methods won't be caught by TypeScript.
Also applies to: 43-43
| // Create default environment | ||
| const defaultEnv = await envOrch.create(); | ||
|
|
||
| // Migrate workspaces | ||
| let migratedWorkspaces = 0; | ||
|
|
||
| for (const legacyWs of legacyConfig.workspaces) { | ||
| // Create domain workspace (LocalWorkspace) | ||
| const domainWorkspace = await workspaceOrch.create( | ||
| defaultEnv.id, | ||
| WorkspaceType.LOCAL, | ||
| legacyWs.repoPath, | ||
| ); | ||
|
|
||
| // Migrate worktree UI metadata | ||
| const worktrees: Record<string, any> = {}; | ||
| for (const legacyWt of legacyWs.worktrees) { | ||
| worktrees[legacyWt.path] = { | ||
| path: legacyWt.path, | ||
| branch: legacyWt.branch, | ||
| description: legacyWt.description, | ||
| prUrl: legacyWt.prUrl, | ||
| merged: legacyWt.merged, | ||
| tabs: legacyWt.tabs, | ||
| mosaicTree: undefined, // Will be derived from tabs structure if needed | ||
| activeTabId: legacyWs.activeTabId, | ||
| updatedAt: legacyWs.updatedAt || new Date().toISOString(), | ||
| }; | ||
| } | ||
|
|
||
| // Create workspace UI state | ||
| const uiState = { | ||
| workspaceId: domainWorkspace.id, | ||
| activeWorktreePath: | ||
| legacyWs.worktrees.find( | ||
| (wt) => wt.id === legacyWs.activeWorktreeId, | ||
| )?.path ?? null, | ||
| worktrees, | ||
| updatedAt: legacyWs.updatedAt || new Date().toISOString(), | ||
| }; | ||
|
|
||
| uiStore.writeWorkspaceUiState(uiState); | ||
| migratedWorkspaces++; | ||
| } | ||
|
|
||
| // Update settings | ||
| const settings = uiStore.readSettings(); | ||
| settings.lastActiveWorkspaceId = legacyConfig.activeWorkspaceId; | ||
| uiStore.writeSettings(settings); | ||
|
|
There was a problem hiding this comment.
Bug: lastActiveWorkspaceId points at a non‑existent workspace after migration
During migration you create new domain workspaces with fresh randomUUID() IDs:
- For each
legacyWsyou callworkspaceOrch.create(defaultEnv.id, WorkspaceType.LOCAL, legacyWs.repoPath)(Lines 86–92), which generates a newdomainWorkspace.id. - Later, when updating settings, you copy
legacyConfig.activeWorkspaceIdstraight intosettings.lastActiveWorkspaceId(Lines 125–128).
Because the new workspaces do not reuse the legacy IDs, lastActiveWorkspaceId will typically refer to a workspace that no longer exists in the new storage. Depending on how settings are consumed, that can either silently drop the last‑active workspace or surface as a runtime “workspace not found” error.
You likely want to map the legacy active workspace to its newly created counterpart. One way to do this:
- // Migrate workspaces
- let migratedWorkspaces = 0;
-
- for (const legacyWs of legacyConfig.workspaces) {
+ // Migrate workspaces
+ let migratedWorkspaces = 0;
+ let newActiveWorkspaceId: string | null = null;
+
+ for (const legacyWs of legacyConfig.workspaces) {
// Create domain workspace (LocalWorkspace)
const domainWorkspace = await workspaceOrch.create(
defaultEnv.id,
WorkspaceType.LOCAL,
legacyWs.repoPath,
);
+
+ // Track mapping for the previously active workspace
+ if (
+ legacyConfig.activeWorkspaceId &&
+ legacyConfig.activeWorkspaceId === legacyWs.id
+ ) {
+ newActiveWorkspaceId = domainWorkspace.id;
+ }
@@
- uiStore.writeWorkspaceUiState(uiState);
- migratedWorkspaces++;
- }
-
- // Update settings
- const settings = uiStore.readSettings();
- settings.lastActiveWorkspaceId = legacyConfig.activeWorkspaceId;
- uiStore.writeSettings(settings);
+ uiStore.writeWorkspaceUiState(uiState);
+ migratedWorkspaces++;
+ }
+
+ // Update settings
+ const settings = uiStore.readSettings();
+ settings.lastActiveWorkspaceId = newActiveWorkspaceId;
+ uiStore.writeSettings(settings);(Assumes legacy workspace entries expose an id that matches activeWorkspaceId; if not, you’ll need a different key to perform the mapping.)
Also, a minor reliability nit: DomainVersion.write(), uiStore.writeUiVersion(), and the various write* methods all return booleans but their results are ignored. If the writes fail (e.g., due to disk issues), migrate will still report success: true. If version consistency is important, consider treating failures there as part of the migration error path or surfacing them in the returned payload.
Committable suggestion skipped: line range outside the PR's diff.
| static read(): number { | ||
| try { | ||
| const versionPath = getDomainVersionPath(); | ||
| if (!existsSync(versionPath)) { | ||
| return 0; | ||
| } | ||
| const content = readFileSync(versionPath, "utf-8"); | ||
| return Number.parseInt(content.trim(), 10); | ||
| } catch (error) { | ||
| console.error("Failed to read domain version:", error); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Guard against NaN when parsing the version file
If db.version contains non‑numeric content, Number.parseInt returns NaN (no throw), so read() will return NaN. Then needsMigration() evaluates NaN < CURRENT_VERSION as false, which can silently skip necessary migrations and propagate NaN to callers.
Consider normalizing invalid parses to 0 (or another safe default), e.g.:
- const content = readFileSync(versionPath, "utf-8");
- return Number.parseInt(content.trim(), 10);
+ const content = readFileSync(versionPath, "utf-8");
+ const parsed = Number.parseInt(content.trim(), 10);
+ return Number.isNaN(parsed) ? 0 : parsed;This keeps the happy path unchanged but makes corruption/legacy formats fall back to a safe “no version” state.
Also applies to: 51-52
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/storage/version.ts around lines 13 to 24 (and
similarly at lines 51-52), the code uses Number.parseInt(content.trim(), 10)
which can produce NaN for invalid/non-numeric file contents; change the logic to
parse the string into a number, check Number.isNaN(result), and if NaN return 0
(or another safe default) so corrupted/legacy version files normalize to a safe
“no version” value; implement the same NaN-guarded parse at the other occurrence
on lines 51-52.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
apps/desktop/docs/STATE_PERSISTENCE_PLAN.md (4)
19-23: Vary opening words for better readability in Non-Goals.The three bullets on lines 20–22 each begin with "Do not," creating repetitive phrasing. Consider restructuring one or two for variety (e.g., "Keep CLI and Desktop persistence separate…" or "Avoid merging CLI and Desktop persistence…").
67-67: Clarify worktree metadata keying strategy for ambiguous cases.The document proposes keying per-worktree UI metadata by "worktree path (primary) and branch (secondary)" (lines 67, 79). However, if two worktrees in the same workspace share the same branch name but have different paths, the secondary key strategy could be ambiguous.
Line 111 mentions "prompt on ambiguity," but the exact resolution (manual selection, merge, or skip) is not specified. Consider documenting the fallback behavior explicitly or noting that this requires user input during activation.
Also applies to: 79-79, 111-111
48-48: Clarify semantics of db.version and ui.version.The document mentions
db.versionandui.version(lines 48, 52, 105) but does not explicitly explain what these versions track:
- Are they file-format/schema versions for migration purposes?
- Are they incremental counters for detecting stale state?
- Are they used to validate compatibility?
Consider adding a brief note (e.g., in the Persistence Split or Migration Strategy section) defining the versioning scheme and its role in validation and rollback.
Also applies to: 105-105
58-73: Emphasize critical rescan-timing constraint for implementation safety.The note on line 73—"The first rescan after activation produces diffs relative to the activation-time snapshot only. If the renderer triggers a rescan before activation finishes, diffs will be empty."—is a critical constraint that could be easily violated.
Consider adding this as a validated assertion or guard in the implementation (e.g., reject rescan requests before activation is complete, or document the assumption explicitly in the IPC contract). This will prevent silent bugs where diffs are missing due to out-of-order calls.
apps/desktop/src/main/index.ts (1)
2-27: Avoid magic depth limit in upward .env searchUsing a fixed
for (let i = 0; i < 6; i++)introduces a brittle assumption about directory depth; if the layout changes,.envdiscovery may silently stop working. Consider looping until you reach the filesystem root instead:-function findEnvFile(): string | undefined { - let currentDir = __dirname; - for (let i = 0; i < 6; i++) { - const envPath = resolve(currentDir, ".env"); - if (existsSync(envPath)) { - return envPath; - } - currentDir = dirname(currentDir); - } - return undefined; -} +function findEnvFile(): string | undefined { + let currentDir = __dirname; + // Walk up until we stop changing directories (reached filesystem root) + // to robustly handle different layouts. + // Prevent infinite loops by breaking when dirname doesn't change. + // (On Windows this also handles drive roots.) + // eslint-disable-next-line no-constant-condition + while (true) { + const envPath = resolve(currentDir, ".env"); + if (existsSync(envPath)) { + return envPath; + } + const parentDir = dirname(currentDir); + if (parentDir === currentDir) break; + currentDir = parentDir; + } + return undefined; +}apps/desktop/src/main/lib/workspace-ipcs.ts (1)
775-924: Harden workspace lookup and consider avoiding duplicate scan/shape logicA couple of robustness and maintainability points for these new handlers:
- Before accessing
workspace.type, it’s safer to handle a missing workspace explicitly instead of relying on thecatchblock to wrap aTypeError. For example:- const workspace = await workspaceOrch.get(input.workspaceId); - if (workspace.type !== "local") { + const workspace = await workspaceOrch.get(input.workspaceId); + if (!workspace) { + return { + success: false, + error: "Workspace not found", + }; + } + if (workspace.type !== "local") { return { success: false, error: "Only local workspaces are supported", }; }Applying the same pattern in
"workspace-rescan"will give clearer errors and avoid noisy logs if clients send stale IDs.
Both handlers construct nearly identical “public” workspace/worktree/UI shapes from
composed.worktrees/rescanResult.state.worktrees. Extracting a small helper (e.g.,toIpcWorkspaceState(workspace, composedState)) would reduce duplication and the risk of the two responses drifting apart.
composer.rescanWorkspacealready performs a Git scan; the"workspace-rescan"handler then callscomposeWorkspaceState, which triggers another scan internally. If scan cost becomes noticeable on large repos, consider threading the scanned worktree list through to composition so the second scan can be skipped.apps/desktop/src/main/lib/ui-ipcs.ts (1)
72-138: Clarify ui-set-active semantics and propagate full success stateTwo small behavioral points in
"ui-set-active":
activeTabIdupdates are gated oninput.activeWorktreePathbeing provided and truthy:if (input.activeTabId !== undefined && input.activeWorktreePath) { // ... }This means a call that only wants to change the active tab (reusing the current active worktree) becomes a silent no-op. If the intended contract allows “tab-only” updates, consider falling back to the current
activeWorktreePathfrom UI state when the input omits it.
- The returned
successonly reflectsupdateWorkspaceUiState; failures fromupdateWorktreeMetadataorwriteSettings(whenupdateGlobalActiveWorkspaceis true) are ignored. To avoid reporting success when later writes fail, you could aggregate the results, e.g.:- const success = uiStore.updateWorkspaceUiState( + let success = uiStore.updateWorkspaceUiState( input.workspaceId, updates, ); // ... - uiStore.updateWorktreeMetadata( + success = + success && + uiStore.updateWorktreeMetadata( // ... ); // ... - uiStore.writeSettings(settings); + success = success && uiStore.writeSettings(settings);and keep returning that combined
successflag.apps/desktop/src/main/lib/workspace-composition/composer.ts (1)
137-186: Consider avoiding double scans and cleaning up unused parameterTwo small follow-ups:
rescanWorkspacecallsscanWorktreesand thencomposeWorkspaceState, which callsscanWorktreesagain. If repositories have many worktrees, this doubles the Git worktree/merge checks for each rescan. A future optimization could be to accept ascannedWorktreesparameter incomposeWorkspaceStatesorescanWorkspacecan reuse the results it already computed.
initializeWorktreeDefaultsacceptsworkspaceIdbut doesn’t use it. If it’s not needed for tab metadata, consider removing it from the signature to avoid confusion, or, if theTabtype includes workspace context, wiring it through appropriately.Also applies to: 191-212
apps/desktop/src/main/lib/ui-store/store.ts (1)
33-178: UiStore persistence and merge semantics look solid overallThe directory setup +
atomicWritepattern, and the per-file read/write methods with localized error handling, are all well-structured and appropriate for desktop, sync-FS usage. The workspace/worktree merge behavior inupdateWorkspaceUiState+updateWorktreeMetadatagives you clear last-writer-wins semantics and keepsupdatedAtserver-side, which is a good default.One minor thing to be aware of:
updateWorktreeMetadatacallsreadWorkspaceUiStateand thenupdateWorkspaceUiState, which reads again. This is harmless but does double I/O; if this becomes hot, you could add an overload toupdateWorkspaceUiStatethat accepts a pre-fetchedexistingstate to avoid the extra read. Not urgent, just a potential micro-optimization if profiling ever points here.apps/desktop/src/shared/ipc-channels/ui.ts (1)
8-83: Avoid duplicating UI state shapes inline; consider shared DTO typesThe IPC contracts here mirror
WorkspaceUiState,WorktreeUiMetadata, andDesktopSettingsalmost 1:1, but they’re re-declared inline. That’s fine short term, but it makes future schema changes (e.g., adding a field to worktree metadata) easy to miss in the IPC layer.If layering allows, consider extracting shared DTO interfaces in
apps/desktop/src/shared/types.ts(or a nearby shared module) and referencing them here, e.g.IpcResponse<WorkspaceUiStateDto>andRecord<string, WorktreeUiMetadataDto>. That keeps the IPC surface and the persisted/logic types in sync and reduces maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.gitignore(1 hunks)apps/desktop/docs/STATE_PERSISTENCE_PLAN.md(1 hunks)apps/desktop/package.json(1 hunks)apps/desktop/src/main/index.ts(3 hunks)apps/desktop/src/main/lib/desktop-stores.ts(1 hunks)apps/desktop/src/main/lib/ui-ipcs.ts(1 hunks)apps/desktop/src/main/lib/ui-store/store.ts(1 hunks)apps/desktop/src/main/lib/workspace-composition/composer.ts(1 hunks)apps/desktop/src/main/lib/workspace-ipcs.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(2 hunks)apps/desktop/src/shared/ipc-channels/ui.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/desktop-stores.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/desktop/src/main/index.ts (5)
apps/desktop/src/main/lib/desktop-stores.ts (1)
DesktopStores(145-145)apps/desktop/src/main/lib/workspace-ipcs.ts (1)
registerWorkspaceIPCs(17-925)apps/desktop/src/main/lib/ui-ipcs.ts (1)
registerUiIPCs(8-198)apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
makeAppSetup(14-71)apps/desktop/src/main/lib/workspace-rescan.ts (1)
workspaceRescanManager(137-137)
apps/desktop/src/main/lib/workspace-composition/composer.ts (3)
apps/desktop/src/main/lib/workspace-composition/types.ts (4)
ScannedWorktree(8-13)ComposedWorkspaceState(27-37)ComposedWorktree(18-21)RescanResult(42-54)apps/desktop/src/main/lib/ui-store/store.ts (1)
UiStore(27-266)apps/desktop/src/main/lib/ui-store/types.ts (2)
UiStore(74-78)WorktreeUiMetadata(37-56)
apps/desktop/src/main/lib/ui-store/store.ts (2)
apps/desktop/src/main/lib/ui-store/types.ts (5)
UiStore(74-78)WindowState(11-21)DesktopSettings(26-31)WorkspaceUiState(61-69)WorktreeUiMetadata(37-56)apps/desktop/src/main/lib/storage/config.ts (2)
getDesktopUiDir(31-33)getUiVersionPath(61-63)
apps/desktop/src/main/lib/ui-ipcs.ts (2)
apps/desktop/src/main/lib/desktop-stores.ts (1)
desktopStores(144-144)apps/desktop/src/main/lib/ui-store/types.ts (1)
WorktreeUiMetadata(37-56)
apps/desktop/src/shared/ipc-channels/ui.ts (2)
apps/desktop/src/shared/ipc-channels/index.ts (2)
IpcResponse(20-20)NoRequest(21-21)apps/desktop/src/shared/types.ts (2)
Tab(44-56)MosaicNode(35-35)
apps/desktop/src/shared/ipc-channels/index.ts (1)
apps/desktop/src/shared/ipc-channels/ui.ts (1)
UiChannels(8-83)
apps/desktop/src/main/lib/workspace-ipcs.ts (2)
apps/desktop/src/main/lib/desktop-stores.ts (1)
desktopStores(144-144)apps/desktop/src/main/lib/workspace-rescan.ts (1)
workspaceRescanManager(137-137)
🪛 LanguageTool
apps/desktop/docs/STATE_PERSISTENCE_PLAN.md
[style] ~9-~9: To elevate your writing, try using a synonym here.
Context: ...ync with Git) and makes the state model hard to evolve. We also want to align domain...
(HARD_TO)
[style] ~23-~23: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ime dependencies or database engines. - Do not prescribe file‑level code changes; ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~39-~39: Ensure spelling is correct
Context: ...description, prUrl, merged). - Derived (in‑memory / cache) - Worktrees scanned from Git...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~39-~39: Ensure spelling is correct
Context: ..., prUrl, merged). - Derived (in‑memory / cache) - Worktrees scanned from Git (b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~56-~56: Use a hyphen to join words.
Context: ...t one big JSON” requirement. If a single domain DB file is preferred later, keep ...
(QB_NEW_EN_HYPHEN)
[grammar] ~92-~92: Use a hyphen to join words.
Context: ...s.list/create/stop/stopAll following CLI type semantics Use existing type‑safe I...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (8)
.gitignore (1)
57-59: Sensible addition aligned with TypeScript changes.The
*.tsbuildinfopattern appropriately ignores TypeScript incremental build artifacts, which is a best practice and aligns with the TypeScript-based changes throughout this PR.apps/desktop/package.json (3)
37-37: Add @superset/cli workspace reference for CLI type exports.The addition of
@superset/clias a workspace dependency aligns with the PR objective to introduce CLI commands and their type re-exports. Ensure that the CLI package properly exports the necessary types and that the alias resolution inelectron.vite.config.tsis correctly configured.
50-50: lowdb v7.0.1 is current and TypeScript-compatible — approved.Verification confirms that lowdb v7.0.1 is the latest published release and actively maintained, with full support for TypeScript-typed adapters and generics. The addition is suitable for your desktop state persistence system. Note that there are some open issues in the v7.x releases (edge cases with async/ENOENT), so monitor the project's issue tracker if you encounter edge-case behaviors.
45-45: Dotenv 17.2.3 integration verified as secure and follows best practices.The version 17.2.3 is actively maintained with no known security vulnerabilities. Reviewing the desktop app implementation confirms:
- Error handling: Conditional check gracefully handles missing
.envfiles with console warnings in bothsrc/main/index.tsandelectron.vite.config.ts- Logging safety: Only logs the
.envfile path, never exposes environment variable contents- Variable security: All environment variables accessed (
NODE_ENV,ENABLE_NEW_UI,USER,SHELL,HOME,SUPERSET_DESKTOP_DATA_DIR) are non-sensitive, standard system variables- Desktop-appropriate usage: Robust upward directory search works correctly whether running from source or compiled code;
override: trueis documented for intentional environment precedenceThe promotion to production dependencies is appropriate and safe.
apps/desktop/src/shared/ipc-channels/index.ts (1)
13-38: UiChannels integration into IpcChannels looks correctExtending
IpcChannelswithUiChannelscleanly exposes the new UI IPC surface to shared typings; no issues from a typing or structure standpoint as long as UI channel names remain unique.apps/desktop/src/main/index.ts (1)
36-80: DesktopStores initialization, UI IPC registration, and rescan cleanup are sequenced sensiblyInitializing
DesktopStoresbefore registering workspace/UI IPCs, then registeringregisterUiIPCs()alongside existing IPCs, and finally stopping all periodic rescans onbefore-quitaligns well with the new persistence/orchestration model; no blocking issues here.Also applies to: 88-94
apps/desktop/src/main/lib/ui-ipcs.ts (1)
1-70: UI IPC surface and error handling look solidThe new
registerUiIPCsfunction cleanly encapsulates UI-related IPCs and consistently usesdesktopStores.getUiStore()with try/catch and structured{ success, error }responses; this is a good fit with the existing IPC patterns.apps/desktop/src/main/lib/workspace-composition/composer.ts (1)
67-132: Workspace composition logic reads clearly and matches the intended modelMerging Git-scanned worktrees with UI metadata (by path, then by branch as a fallback) and enforcing Git as the source of truth for
path/branchwhile preserving UI fields like tabs and layout is a clean design; active selection derivation from the composed state also looks consistent.
| - Cache the scan result as the "activation-time snapshot" for diff tracking. | ||
| - Start background tasks: periodic rescans (every 30s), port detection for terminals in the active worktree, update proxy targets. | ||
| 3. Refresh | ||
| - Manual rescan via IPC and periodic rescan reconcile UI metadata with Git (remove orphans after a grace period; retain notes when possible). |
There was a problem hiding this comment.
Resolve inconsistency: grace period for orphaned metadata.
Line 72 (Refresh section) describes orphan removal "after a grace period" as settled behavior, but line 145 (Open Questions) lists "Grace period policy for orphaned UI metadata (immediate prune vs delayed cleanup)?" as an unanswered question.
Clarify whether a grace period is decided and documented here, or whether it remains open for the implementation team to decide.
Also applies to: 145-145
🤖 Prompt for AI Agents
In apps/desktop/docs/STATE_PERSISTENCE_PLAN.md at lines 72 and 145, the doc
currently asserts orphaned UI metadata is removed "after a grace period" (line
72) but later lists the grace period policy as an open question (line 145);
reconcile these by either (A) deciding the policy and updating line 72 to state
the chosen grace period and rationale and removing the "open question" entry at
line 145, or (B) if undecided, change line 72 to a tentative wording (e.g., "may
be removed pending grace period policy") and keep line 145 as an active open
question; apply the corresponding text edits so both locations consistently
reflect the final decision and include any chosen duration, retention rules, and
who owns the decision.
|
|
||
| - Worktree rename/path changes: primary key by path, secondary by branch; prompt on ambiguity. | ||
| - Large repos: throttle rescans; make resumable/cancellable; limit scope to worktrees rather than full repo. | ||
| - Partial writes: **Implemented** - UI store uses atomic write pattern (write to *.tmp, fsync, rename) for all persistence operations (window-state.json, settings.json, per-workspace UI state). This prevents data corruption on crash. |
There was a problem hiding this comment.
Clarify implementation status of atomic write pattern.
Line 113 claims the atomic write pattern is "Implemented," but this is a Proposed plan document. Either this pattern was implemented in a prior commit (if so, reference it) or remove the "Implemented" marker to avoid confusion about what is currently in place vs. planned.
🤖 Prompt for AI Agents
In apps/desktop/docs/STATE_PERSISTENCE_PLAN.md around line 113, the document
incorrectly labels the atomic write pattern as "Implemented"; verify if the
pattern already exists by searching commits/PRs and, if found, update the line
to include a reference to the implementing commit or PR (hash/URL and file path)
and a brief note about where the implementation lives; if no implementation
exists, remove the "Implemented" marker and change the wording to "Proposed" or
"Planned" (and optionally include an action item/ticket reference) so the
document accurately reflects the current status.
| ipcMain.handle( | ||
| "ui-settings-update", | ||
| async ( | ||
| _event, | ||
| input: { | ||
| lastActiveWorkspaceId?: string | null; | ||
| preferences?: Record<string, unknown>; | ||
| }, | ||
| ) => { | ||
| try { | ||
| const uiStore = desktopStores.getUiStore(); | ||
| const settings = uiStore.readSettings(); | ||
|
|
||
| if (input.lastActiveWorkspaceId !== undefined) { | ||
| settings.lastActiveWorkspaceId = input.lastActiveWorkspaceId; | ||
| } | ||
| if (input.preferences !== undefined) { | ||
| settings.preferences = { | ||
| ...settings.preferences, | ||
| ...input.preferences, | ||
| }; | ||
| } | ||
|
|
||
| const success = uiStore.writeSettings(settings); | ||
|
|
||
| return { | ||
| success, | ||
| error: success ? undefined : "Failed to update settings", | ||
| }; | ||
| } catch (error) { | ||
| console.error("[UiIPC] Error updating settings:", error); | ||
| return { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : "Unknown error", | ||
| }; | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Guard against undefined preferences when merging UI settings
ui-settings-update merges preferences via:
settings.preferences = {
...settings.preferences,
...input.preferences,
};If settings.preferences can be undefined (which is allowed by the typings), this spread will throw at runtime. It’s safer to coalesce to an empty object:
- if (input.preferences !== undefined) {
- settings.preferences = {
- ...settings.preferences,
- ...input.preferences,
- };
- }
+ if (input.preferences !== undefined) {
+ settings.preferences = {
+ ...(settings.preferences ?? {}),
+ ...input.preferences,
+ };
+ }This keeps behavior the same when preferences are already initialized, and prevents a crash if they’re ever missing.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/ui-ipcs.ts around lines 160 to 197, the merge of
preferences uses spread on settings.preferences which may be undefined per
typings and will throw; update the merge to coalesce settings.preferences to an
empty object before spreading (e.g., use (settings.preferences ?? {}) as the
base) so merging works even when preferences are missing, then writeSettings as
before and return the same success/error shape.
| /** | ||
| * Get UI version | ||
| */ | ||
| readUiVersion(): number { | ||
| try { | ||
| const versionPath = getUiVersionPath(); | ||
| if (!existsSync(versionPath)) { | ||
| return 0; | ||
| } | ||
| const content = readFileSync(versionPath, "utf-8"); | ||
| return Number.parseInt(content.trim(), 10); | ||
| } catch (error) { | ||
| console.error("Failed to read UI version:", error); | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Write UI version | ||
| */ | ||
| writeUiVersion(version: number = 1): boolean { | ||
| try { | ||
| const versionPath = getUiVersionPath(); | ||
| this.atomicWrite(versionPath, String(version)); | ||
| return true; | ||
| } catch (error) { | ||
| console.error("Failed to write UI version:", error); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Check if UI migration is needed | ||
| */ | ||
| needsMigration(): boolean { | ||
| return this.readUiVersion() < 1; | ||
| } |
There was a problem hiding this comment.
Handle NaN from readUiVersion to avoid silently skipping migrations
If ui.version becomes corrupted with a non-numeric value, Number.parseInt will return NaN but not throw. In that case, readUiVersion() currently returns NaN, and needsMigration() will evaluate NaN < 1 to false, so a required migration would be silently skipped.
You probably want invalid content to behave like 0 (i.e., “no version yet”) so migrations still run. A small tweak in readUiVersion would fix this:
readUiVersion(): number {
try {
const versionPath = getUiVersionPath();
if (!existsSync(versionPath)) {
return 0;
}
const content = readFileSync(versionPath, "utf-8");
- return Number.parseInt(content.trim(), 10);
+ const parsed = Number.parseInt(content.trim(), 10);
+ return Number.isNaN(parsed) ? 0 : parsed;
} catch (error) {
console.error("Failed to read UI version:", error);
return 0;
}
}With this, any malformed file falls back to 0, and needsMigration() will correctly indicate that migration is needed.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/ui-store/store.ts around lines 229 to 265,
readUiVersion can return NaN when the file contains non-numeric content which
causes needsMigration() to silently skip required migrations; update
readUiVersion so after parsing the file you check Number.isNaN(parsed) and
return 0 if the parsed value is NaN (or otherwise coerce invalid/malformed
content to 0), keeping the existing try/catch and logging behavior so malformed
files are treated as version 0 and migrations will run.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/docs/DND_BACKEND_UNIFICATION.md(1 hunks)apps/desktop/src/renderer/lib/dnd.ts(1 hunks)apps/desktop/src/renderer/screens/main/MainScreen.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (1)
apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (1)
apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
⏰ 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). (1)
- GitHub Check: Typecheck
🔇 Additional comments (10)
apps/desktop/docs/DND_BACKEND_UNIFICATION.md (1)
1-63: LGTM! Excellent documentation.This documentation clearly explains the DnD backend unification strategy, the problem it solves, and provides comprehensive implementation details and regression guardrails. The file paths and code patterns are accurate and align with the actual implementation in the PR.
apps/desktop/src/renderer/screens/main/MainScreen.tsx (2)
3-3: LGTM! Correct import of shared DnD manager.The import of the centralized
dragDropManageris consistent with the DnD unification strategy documented in the PR.
137-137: LGTM! Correctly wired to shared manager.The
DndProvidernow usesmanager={dragDropManager}instead ofbackend={HTML5Backend}, which ensures a single DnD backend instance across the renderer and prevents the "Cannot have two HTML5 backends at the same time" error.apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (2)
21-21: LGTM! Correct import of shared DnD manager.The import aligns with the centralized DnD approach and enables the Tree component to use the shared manager.
415-415: LGTM! Correctly propagated shared manager to Tree.Passing
dndManager={dragDropManager}to the Arborist Tree component prevents it from creating its own DnD provider and ensures it reuses the shared manager, consistent with the DnD unification strategy.apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (2)
25-25: LGTM! Correct import of shared DnD manager.The import statement correctly brings in the centralized
dragDropManagerfor use in this component.
1065-1065: LGTM! Correctly propagated shared manager to Tree.Passing
dndManager={dragDropManager}ensures the Arborist Tree component uses the shared DnD backend, preventing multiple HTML5 backend instances and aligning with the unification strategy.apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (3)
3-6: LGTM! Import additions are correct.The
MosaicWindowimport is properly added and used in therenderTilefunction (line 175). The import restructuring maintains clarity.
12-12: LGTM! Correct import of shared DnD manager.The import of
dragDropManagerenables Mosaic to use the shared DnD backend.
247-247: LGTM! Correctly wired to shared manager.Passing
dragAndDropManager={dragDropManager}to the Mosaic component ensures it reuses the shared DnD backend instead of creating its own. Note that Mosaic usesdragAndDropManageras the prop name (notdndManager), which is correct for thereact-mosaic-componentAPI.
| import { createDragDropManager } from "dnd-core"; | ||
| import { HTML5Backend } from "react-dnd-html5-backend"; | ||
|
|
||
| // Single, shared DragDropManager for the entire renderer | ||
| export const dragDropManager = createDragDropManager(HTML5Backend); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Clean singleton implementation.
This creates a single, shared DragDropManager that serves as the source of truth for all DnD operations in the renderer. The implementation is minimal and correct.
However, verify that the required dependencies are properly installed and listed in the package manifest.
Run the following script to verify the dependencies are installed:
🏁 Script executed:
#!/bin/bash
# Verify dnd-core and react-dnd-html5-backend are in package.json
# Find package.json files in the desktop app
fd -t f "^package\.json$" apps/desktop
# Check for the dependencies in the main package.json
echo "=== Checking for dnd-core and react-dnd-html5-backend ==="
cat apps/desktop/package.json | jq -r '.dependencies, .devDependencies' | grep -E "(dnd-core|react-dnd-html5-backend)" || echo "Dependencies not found in package.json"Length of output: 123
I need to verify more thoroughly whether dnd-core is present in the dependencies. The grep output only shows react-dnd-html5-backend, but dnd-core may be missing.
---
🏁 Script executed:
```shell
#!/bin/bash
# More detailed check for dnd-core in package.json
echo "=== Full content of dependencies and devDependencies ==="
cat apps/desktop/package.json | jq '.dependencies + .devDependencies | keys[] | select(contains("dnd"))'
echo ""
echo "=== Checking for dnd-core explicitly ==="
cat apps/desktop/package.json | jq '.dependencies.["dnd-core"] // .devDependencies.["dnd-core"] // "NOT FOUND"'
echo ""
echo "=== Checking react-dnd-html5-backend version ==="
cat apps/desktop/package.json | jq '.dependencies.["react-dnd-html5-backend"] // .devDependencies.["react-dnd-html5-backend"]'
Length of output: 734
I can see the problem now. Let me verify the correct import path and confirm the missing dependency:
🏁 Script executed:
#!/bin/bash
# Check the actual packages with correct jq syntax
echo "=== All dnd-related packages installed ==="
cat apps/desktop/package.json | jq -r '.dependencies + .devDependencies | to_entries[] | select(.key | contains("dnd")) | "\(.key): \(.value)"'
echo ""
echo "=== Check if dnd-core exists anywhere ==="
cat apps/desktop/package.json | jq -r '.dependencies + .devDependencies | keys[]' | grep -i "dnd-core" || echo "dnd-core NOT FOUND"
echo ""
echo "=== Verify react-dnd and react-dnd-html5-backend versions ==="
cat apps/desktop/package.json | jq '.dependencies | with_entries(select(.key | test("react-dnd")))'Length of output: 364
🏁 Script executed:
#!/bin/bash
# Check the actual packages with correct jq syntax
echo "=== All dnd-related packages installed ==="
cat apps/desktop/package.json | jq -r '.dependencies + .devDependencies | to_entries[] | select(.key | contains("dnd")) | "\(.key): \(.value)"'
echo ""
echo "=== Check if dnd-core exists anywhere ==="
cat apps/desktop/package.json | jq -r '.dependencies + .devDependencies | keys[]' | grep -i "dnd-core" || echo "dnd-core NOT FOUND"
echo ""
echo "=== Verify react-dnd and react-dnd-html5-backend versions ==="
cat apps/desktop/package.json | jq '.dependencies | with_entries(select(.key | test("react-dnd")))'Length of output: 364
Now I need to verify the correct way to use createDragDropManager with react-dnd 16.0.1:
🌐 Web query:
react-dnd 16.0.1 createDragDropManager dnd-core import
💡 Result:
Yes — in react-dnd / dnd-core v16.0.1 you import it from the dnd-core package:
Example:
import { createDragDropManager } from 'dnd-core'
const manager = createDragDropManager(BackendFactory /* e.g. TestBackend */, globalThis, options, false)
(BackendFactory can be e.g. the TestBackend or an HTML5/touch backend exported by a react-dnd backend package.) [1][2]
Sources: npm dnd-core package info; dnd-core API docs. [1] [2]
Add missing dnd-core dependency to package.json.
Verification confirms that dnd-core is not listed in apps/desktop/package.json, but the code imports from it. The correct import path requires importing createDragDropManager from the dnd-core package. This will cause a runtime failure.
Add dnd-core to the dependencies in apps/desktop/package.json alongside the existing react-dnd-html5-backend (^16.0.1).
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/lib/dnd.ts lines 1-6, the code imports
createDragDropManager from dnd-core but that package is missing from
apps/desktop/package.json; add "dnd-core" to the dependencies (use a version
compatible with react-dnd-html5-backend, e.g. "dnd-core": "^16.0.1"), update the
lockfile by running your package manager install (npm/yarn/pnpm), and commit the
updated package.json and lockfile so the import works at runtime.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores