Harden runtime packaging and clean up onboarding#232
Merged
Conversation
Stage a shared runtime artifact for desktop and CLI builds, validate it before packaging, and tighten the macOS install/startup flow with better diagnostics and CI smoke checks. Also clean up onboarding by importing only valid git repos, improving recent-project source selection, and handling the Deus clone/import edge cases more clearly. Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a staged runtime pipeline and validation; centralizes runtime paths; moves agent-server lifecycle into Electron main with coordinated agent→backend startup and restarts; implements a recent-projects discovery service and repo inspection; updates CLI/runtime packaging, CI macOS artifact checks and DMG smoke tests; adds macOS install preflight and startup diagnostics. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Electron as Electron Main
participant Agent as Agent-Server (child)
participant Backend as Backend (child)
User->>Electron: Launch Deus.app
Electron->>Electron: ensureInstalledInApplications()
alt Not in Applications (macOS packaged)
Electron->>User: Prompt Move to Applications or Quit
User-->>Electron: Choose Move
Electron->>Electron: app.moveToApplicationsFolder()
end
Electron->>Electron: initMainProcessLogging()
Electron->>Agent: spawn agent-server (child)
Agent-->>Electron: stdout: LISTEN_URL=...
Electron->>Electron: parse LISTEN_URL
Electron->>Backend: spawn backend with AGENT_SERVER_URL
Backend-->>Electron: stdout: [BACKEND_PORT] <auth>
Electron->>Electron: parse port & authToken
Electron->>User: expose port/auth to renderer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Keep just the Deus machine title + Devs swap animation with the CTA button. The scrolling manifesto text between them is removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/desktop.ts (1)
330-338:⚠️ Potential issue | 🟠 MajorKeep the DMG mounted on manual-install fallback.
If
dittofails afterhdiutil attachsucceeds, this path opens the DMG and tells the user to dragDeus.app, but thefinallyblock still detachesmountPoint. That closes the volume immediately, so the fallback cannot complete.Suggested fix
case "darwin": { let mountPoint: string | undefined; + let keepMounted = false; try { const mountOutput = execSync(`hdiutil attach "${filePath}" -nobrowse`, { encoding: "utf-8", }); @@ if (existsSync(appPath)) { execSync(`rm -rf "${destPath}"`, { stdio: "pipe" }); execSync(`ditto "${appPath}" "${destPath}"`, { stdio: "pipe" }); s.succeed(`Installed to ${c.dim("/Applications/Deus.app")}`); launchDesktop(destPath); } else { + keepMounted = true; s.warn("DMG mounted — drag Deus to Applications to finish"); } return true; } catch { s.fail("Auto-install failed — opening DMG manually"); + keepMounted = true; execSync(`open "${filePath}"`); hint("Drag Deus.app to Applications, then launch it from Applications."); return false; } finally { - if (mountPoint) { + if (mountPoint && !keepMounted) { try { execSync(`hdiutil detach "${mountPoint}" -quiet`, { stdio: "pipe" }); } catch { // Detach may fail if already unmounted — ignore }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/desktop.ts` around lines 330 - 338, The finally block is unconditionally detaching the DMG (execSync `hdiutil detach ...`) even when the catch branch falls back to manual install (which opens `filePath` and tells the user to drag Deus.app), so modify the flow to preserve the mount when falling back: introduce a boolean flag (e.g., keepMounted or manualFallback) set to true inside the catch branch that calls `execSync(\`open "${filePath}"\`)` and `hint(...)`, and update the finally block to only run `execSync("hdiutil detach ...")` if `mountPoint` exists AND the flag is false; reference the existing variables `mountPoint`, `filePath`, and the `hdiutil detach` call in `finally`.
🧹 Nitpick comments (4)
test/unit/shared/runtime.test.ts (1)
20-28: Consider adding a Windows fallback test case.The Windows test covers the case where
appDatais provided, but there's no test for the fallback path whenappDatais undefined (which would usehomeDir/AppData/Roaming).📝 Suggested additional test case
it("resolves the canonical Windows data directory", () => { expect( resolveDefaultDataDir({ platform: "win32", homeDir: "C:/Users/deus", appData: "C:/Users/deus/AppData/Roaming", }) ).toBe(String.raw`C:\Users\deus\AppData\Roaming\com.deus.app`); + + // Fallback when APPDATA is not set + expect( + resolveDefaultDataDir({ + platform: "win32", + homeDir: "C:/Users/deus", + }) + ).toBe(String.raw`C:\Users\deus\AppData\Roaming\com.deus.app`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/shared/runtime.test.ts` around lines 20 - 28, Add a unit test for resolveDefaultDataDir that covers the Windows fallback when appData is undefined: call resolveDefaultDataDir with platform: "win32", homeDir: "C:/Users/deus" and appData: undefined (or omitted) and assert it resolves to the canonical path "C:\Users\deus\AppData\Roaming\com.deus.app" (use String.raw or equivalent to match backslashes); place it alongside the existing Windows test so both the primary and fallback code paths for resolveDefaultDataDir are exercised.test/unit/runtime/validate-runtime.test.ts (1)
69-71: Assert the stale-bundle error, not just the generic hint.This matcher will pass for any
createBuildRuntimeErrorthat appends the same remediation text, so it doesn't actually prove the mtime guard fired. Matching the stale bundle fragment keeps the test targeted.Suggested fix
expect(() => validateRuntimeStage({ projectRoot, log: () => {} })).toThrow( - /Run `bun run build:runtime` before packaging\./ + /backend bundle is stale/ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/runtime/validate-runtime.test.ts` around lines 69 - 71, Update the test assertion in validate-runtime.test.ts to assert the specific stale-bundle error text instead of only the generic remediation hint: change the expect(...).toThrow(...) matcher used around validateRuntimeStage to match the unique stale-bundle fragment produced by createBuildRuntimeError (e.g., include the word "stale" or "stale-bundle" together with the existing "Run `bun run build:runtime` before packaging." text) so the test proves the mtime guard fired rather than any error that appends the same hint.apps/runtime/validate.ts (1)
101-107: JSON.stringify comparison is order-sensitive.The manifest comparison uses
JSON.stringifywhich is sensitive to property order. IfbuildExpectedManifestand the staged manifest have properties in different orders (though unlikely with controlled generation), this could produce false negatives.For robustness, consider a deep equality check, though this is low risk since both are generated by controlled code paths.
♻️ Optional: Use deep equality instead
+function deepEqual(a: unknown, b: unknown): boolean { + return JSON.stringify(sortKeys(a)) === JSON.stringify(sortKeys(b)); +} + +function sortKeys(obj: unknown): unknown { + if (obj === null || typeof obj !== "object") return obj; + if (Array.isArray(obj)) return obj.map(sortKeys); + return Object.keys(obj as Record<string, unknown>) + .sort() + .reduce((acc, key) => { + acc[key] = sortKeys((obj as Record<string, unknown>)[key]); + return acc; + }, {} as Record<string, unknown>); +} - if (JSON.stringify(manifest) !== JSON.stringify(expectedManifest)) { + if (!deepEqual(manifest, expectedManifest)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/runtime/validate.ts` around lines 101 - 107, The current equality check between manifest and expectedManifest uses JSON.stringify which is order-sensitive; replace that check in validate.ts (where manifest is read via readManifest(stagePaths.manifest) and expectedManifest built via buildExpectedManifest(projectRoot)) with a deep structural equality comparison (e.g., use a utility like lodash/isEqual or a local deepEqual helper) and keep the same error path that throws createBuildRuntimeError when they differ; ensure you compare the parsed objects (manifest and expectedManifest) rather than their stringified forms.apps/backend/src/routes/repos.ts (1)
178-183: Expose a stable conflict code here.The onboarding flow now differentiates these two 409s by parsing the error text. That makes a copy change in this handler a behavior change for the client, even if the status code stays 409. Returning a machine-readable code alongside the human-readable message would make this contract much safer to evolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/routes/repos.ts` around lines 178 - 183, Add a stable machine-readable conflict code to each 409 thrown here so clients can distinguish cases without parsing messages: update the two ConflictError instances in this handler (the one checking for ".git" via gitMetadataPath and the subsequent directory-exists branch) to include a reproducible code property (e.g., attach code: "conflict.git_repository" for the git repo case and code: "conflict.directory_exists" for the non-git directory case) by using the ConflictError constructor/metadata pattern your project supports (e.g., new ConflictError(message, { code: ... }) or set error.code = "...") so the HTTP response includes both the human message and the stable code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 160-187: getClaudeSessionJsonlFiles currently appends files from
the session root then the subagents folder which breaks recency ordering because
parent dir mtime doesn't change when subagent files update; instead gather
Dirent entries from both sessionDir and subagentsDir (use readdirSync with
withFileTypes), normalize their full paths and mtimes, then merge and sort those
entries by their mtime before returning the .jsonl paths so readClaudeProjects
sees correct most-recent activity; update getClaudeSessionJsonlFiles (and the
analogous logic around lines 225-229) to perform a single unified sort across
root and subagents rather than appending subagent files last.
- Around line 140-155: The current logic in
readJsonlPrefix/extractClaudeCwdFromJsonl reads the file head and returns the
first "cwd", which can return an outdated session; change this to read from the
file tail and find the last "cwd" occurrence instead. Replace or add a function
(e.g., readJsonlSuffix) that uses fs.statSync to get file size and read the last
N bytes (use CLAUDE_JSONL_PREFIX_BYTES as the window) via openSync/readSync,
then in extractClaudeCwdFromJsonl call that suffix reader and search for the
final match of /"cwd":"((?:[^"\\]|\\.)+)"/ (or split by newline and scan from
end) and JSON.parse the captured group; ensure file descriptor is closed in a
finally block and handle empty/no-match by returning null.
In `@apps/web/src/features/onboarding/ui/steps/DeusStep.tsx`:
- Around line 68-75: The branch treating an existing repo as "already_cloned"
must verify the repo is actually the Deus project before calling
RepoService.add(target); when conflictKind === "already_cloned" call a check
(e.g., use a helper like isRepoMatchingTarget or a git utility) to read the
target repo's origin URL and compare against REPO.url (and allowed fork
patterns), and only set alreadyCloned = true if it matches; otherwise show an
error/toast and return so RepoService.add(target) won't register the wrong
repository.
In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx`:
- Around line 68-69: The catch block in ProjectSelectionStep that currently
always calls toast.error("Couldn’t add that folder. Make sure it’s a git
repository.") should preserve and surface the real error: capture the caught
error (e) and include e.message (or map known error codes like EACCES, ENOENT,
or a repo-already-registered/conflict error) in the toast or choose a specific
message for each case; update the catch in the function handling folder
browse/add (the try/catch around the browse/add logic in ProjectSelectionStep)
to log the original error (e.g., console.error or process logger) and show a
contextual toast that includes the error details rather than always blaming the
folder.
- Around line 39-52: The batch-add logic in ProjectSelectionStep treats
“Repository already exists” as a failure because addRepoMutation.mutateAsync()
rejects; update the results processing to treat clone-conflict errors as
successes: import getErrorMessage and classifyCloneConflict, map
Promise.allSettled results and for each rejected result call
getErrorMessage(result.reason) and if classifyCloneConflict(error) consider it
succeeded (increment succeeded and do not count as failed), only show the
“couldn’t add any” toast and block onNext when there are truly zero non-conflict
successes, and adjust the partial-failure toast to report only real failures
(exclude already-registered repos).
---
Outside diff comments:
In `@apps/cli/src/desktop.ts`:
- Around line 330-338: The finally block is unconditionally detaching the DMG
(execSync `hdiutil detach ...`) even when the catch branch falls back to manual
install (which opens `filePath` and tells the user to drag Deus.app), so modify
the flow to preserve the mount when falling back: introduce a boolean flag
(e.g., keepMounted or manualFallback) set to true inside the catch branch that
calls `execSync(\`open "${filePath}"\`)` and `hint(...)`, and update the finally
block to only run `execSync("hdiutil detach ...")` if `mountPoint` exists AND
the flag is false; reference the existing variables `mountPoint`, `filePath`,
and the `hdiutil detach` call in `finally`.
---
Nitpick comments:
In `@apps/backend/src/routes/repos.ts`:
- Around line 178-183: Add a stable machine-readable conflict code to each 409
thrown here so clients can distinguish cases without parsing messages: update
the two ConflictError instances in this handler (the one checking for ".git" via
gitMetadataPath and the subsequent directory-exists branch) to include a
reproducible code property (e.g., attach code: "conflict.git_repository" for the
git repo case and code: "conflict.directory_exists" for the non-git directory
case) by using the ConflictError constructor/metadata pattern your project
supports (e.g., new ConflictError(message, { code: ... }) or set error.code =
"...") so the HTTP response includes both the human message and the stable code.
In `@apps/runtime/validate.ts`:
- Around line 101-107: The current equality check between manifest and
expectedManifest uses JSON.stringify which is order-sensitive; replace that
check in validate.ts (where manifest is read via
readManifest(stagePaths.manifest) and expectedManifest built via
buildExpectedManifest(projectRoot)) with a deep structural equality comparison
(e.g., use a utility like lodash/isEqual or a local deepEqual helper) and keep
the same error path that throws createBuildRuntimeError when they differ; ensure
you compare the parsed objects (manifest and expectedManifest) rather than their
stringified forms.
In `@test/unit/runtime/validate-runtime.test.ts`:
- Around line 69-71: Update the test assertion in validate-runtime.test.ts to
assert the specific stale-bundle error text instead of only the generic
remediation hint: change the expect(...).toThrow(...) matcher used around
validateRuntimeStage to match the unique stale-bundle fragment produced by
createBuildRuntimeError (e.g., include the word "stale" or "stale-bundle"
together with the existing "Run `bun run build:runtime` before packaging." text)
so the test proves the mtime guard fired rather than any error that appends the
same hint.
In `@test/unit/shared/runtime.test.ts`:
- Around line 20-28: Add a unit test for resolveDefaultDataDir that covers the
Windows fallback when appData is undefined: call resolveDefaultDataDir with
platform: "win32", homeDir: "C:/Users/deus" and appData: undefined (or omitted)
and assert it resolves to the canonical path
"C:\Users\deus\AppData\Roaming\com.deus.app" (use String.raw or equivalent to
match backslashes); place it alongside the existing Windows test so both the
primary and fallback code paths for resolveDefaultDataDir are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90ec8204-c670-4cad-a503-f8ccc76c6a4e
📒 Files selected for processing (33)
.github/workflows/release.ymlREADME.mdapps/backend/build.tsapps/backend/src/lib/database.tsapps/backend/src/routes/onboarding.tsapps/backend/src/routes/repos.tsapps/backend/src/server.tsapps/backend/src/services/recent-projects.service.tsapps/backend/test/unit/routes/repos.test.tsapps/backend/test/unit/services/recent-projects.service.test.tsapps/cli/README.mdapps/cli/build.tsapps/cli/package.jsonapps/cli/src/desktop.tsapps/cli/src/start.tsapps/desktop/main/backend-process.tsapps/desktop/main/index.tsapps/desktop/main/install-preflight.tsapps/desktop/main/startup-diagnostics.tsapps/runtime/build.tsapps/runtime/electron-builder-before-pack.cjsapps/runtime/stage.tsapps/runtime/validate.tsapps/web/src/features/onboarding/lib/deus-import.tsapps/web/src/features/onboarding/ui/steps/DeusStep.tsxapps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsxelectron-builder.ymlpackage.jsonshared/runtime.tstest/unit/onboarding/deus-import.test.tstest/unit/runtime/validate-runtime.test.tstest/unit/shared/runtime.test.tstsconfig.node.json
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
391-398:⚠️ Potential issue | 🟠 MajorKeep the publish job on Bun end-to-end.
Line 398 still invokes
npm publish, so this touched release path now mixes package managers in CI. Please switch the publish step to the repo’s Bun-based publish flow instead.As per coding guidelines, "Use Bun as the package manager. Always use
bun add,bun install,bun run,bunx. Never use npm or yarn. CI must usebun install --frozen-lockfile."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 391 - 398, Replace the npm publish invocation with the repo's Bun-based publish flow: in the job currently named "Publish to npm" (working-directory: apps/cli) remove `npm publish --access public` and run the Bun publish command instead (e.g., `bun publish --access public` or `bunx publish --access public`) and ensure you run `bun install --frozen-lockfile` in that job before publishing; keep the build step (`bun run build`) in "Build CLI" but ensure CI uses only Bun commands across the workflow.
♻️ Duplicate comments (3)
apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx (2)
67-68:⚠️ Potential issue | 🟡 MinorPreserve the actual browse error instead of always blaming git.
This catch still collapses conflicts, permission errors, and path issues into the same “make sure it’s a git repository” toast, which gives the wrong guidance for common failures like an already-registered repo or an inaccessible folder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx` around lines 67 - 68, The catch in ProjectSelectionStep.tsx currently swallows the thrown error and always shows “Couldn’t add that folder. Make sure it’s a git repository.” — change the catch in the code that calls the folder browse/registration (the try/catch around the add/register logic in ProjectSelectionStep) to capture the caught error (e.g., const err) and surface more accurate feedback: include err.message in the toast or branch on known conditions (e.g., “already registered”, permission denied, path not found) to show an appropriate toast instead of always blaming git; ensure the toast still falls back to the original generic message if err.message is absent.
39-52:⚠️ Potential issue | 🟠 MajorDon't treat already-registered repos as failed imports.
This still counts
Repository already existsrejections as failures. If every selected repo is already in the DB, the step shows “Couldn’t add any...” and blocksonNext()even though the user picked valid projects. Normalize thealready_clonedcase before computingsucceededandfailed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx` around lines 39 - 52, Normalize Promise.allSettled results so rejections with the "already_cloned"/"Repository already exists" case are treated as successes before computing succeeded/failed; after calling addRepoMutation.mutateAsync for each path, map results and count a result as successful if result.status === "fulfilled" OR result.status === "rejected" && (result.reason?.code === "already_cloned" || /Repository already exists/i.test(result.reason?.message)); use that normalized succeeded count to decide toast messages and whether to call onNext (so already-registered repos no longer block progress).apps/web/src/features/onboarding/ui/steps/DeusStep.tsx (1)
68-74:⚠️ Potential issue | 🟠 MajorVerify the existing repo is actually Deus before accepting
already_cloned.
classifyCloneConflict()only proves that the target path already contains some git repo. If~/Developer/deusalready holds an unrelated repository, this branch proceeds toRepoService.add(target)and can register the wrong project. Check the repo’s origin/name againstREPO.urlbefore settingalreadyCloned = true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/onboarding/ui/steps/DeusStep.tsx` around lines 68 - 74, The code currently treats any existing git repo as "already_cloned" after calling classifyCloneConflict; instead, when conflictKind === "already_cloned" perform an explicit check of the repo's remote origin (e.g., read the git remote URL from the repo at target) and compare it against REPO.url (or parsed owner/name) before setting alreadyCloned = true; if the origin does not match, do not mark alreadyCloned (handle as a conflicting/unexpected repo: show an error toast or abort) and only call RepoService.add(target) when the origin matches the expected REPO.url.
🧹 Nitpick comments (4)
apps/backend/src/services/recent-projects.service.ts (1)
132-138: Minor TOCTOU risk:statSyncmay throw if file is deleted afterreaddirSync.If a file is removed between the
readdirSynccall andstatSync, this will throw. The callers (resolveClaudeProjectPath,readClaudeProjects) wrap this in try-catch, so it degrades gracefully to returningnull/empty. However, a file disappearing mid-sort abandons the entire directory scan rather than just skipping that entry.♻️ Optional: graceful skip for deleted files
function sortDirentsByMtime(dirents: Dirent[], parentDir: string): Dirent[] { - return [...dirents].sort((left, right) => { - const leftTime = statSync(join(parentDir, left.name)).mtimeMs; - const rightTime = statSync(join(parentDir, right.name)).mtimeMs; - return rightTime - leftTime; - }); + const withMtime = dirents.map((d) => { + try { + return { d, mtime: statSync(join(parentDir, d.name)).mtimeMs }; + } catch { + return null; + } + }).filter((x): x is { d: Dirent; mtime: number } => x !== null); + + return withMtime.sort((a, b) => b.mtime - a.mtime).map((x) => x.d); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/services/recent-projects.service.ts` around lines 132 - 138, The sortDirentsByMtime function risks throwing if statSync fails for an entry; change it to defend per-entry by wrapping the statSync calls for left and right in try/catch (or compute mtimes in a pre-pass), treating missing/unstatable entries as the oldest (e.g., mtimeMs = -Infinity or Number.MIN_SAFE_INTEGER) so they sort to the end, and skip or ignore entries whose stat fails rather than letting statSync propagate an exception; update sortDirentsByMtime to compute safeLeftTime/safeRightTime for left/right using the join(parentDir, dirent.name) and the Dirent names referenced in the function to locate the code.apps/backend/src/routes/repos.ts (1)
178-183: Return a machine-readable conflict code here.These new
ConflictErrormessages are now part of the onboarding contract that the web client parses. A copy-only edit here would silently change frontend behavior. Prefer emitting a stable 409 code and let the client map that to user-facing text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/routes/repos.ts` around lines 178 - 183, The ConflictError thrown in repos handler (references gitMetadataPath and the two throw new ConflictError(...) sites) must include a stable machine-readable conflict code (e.g., set a property like error.code = "TARGET_CONTAINS_GIT" or status = 409) so the frontend can reliably parse it; update both throw sites to instantiate ConflictError with a consistent code/value (or pass an explicit { code: "TARGET_CONTAINS_GIT", status: 409 } payload) instead of only human text, and ensure the ConflictError constructor (or the place constructing the response) propagates that code into the HTTP 409 response body so clients can map to UI strings.apps/runtime/build.ts (1)
7-9: Consider improving error logging for debugging.When
erroris not anErrorinstance,console.errormay not display useful information. For build scripts, showing the stack trace helps diagnose failures.🔧 Suggested improvement
} catch (error) { - console.error("Runtime staging failed:", error); + console.error("Runtime staging failed:", error instanceof Error ? error.stack ?? error.message : error); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/runtime/build.ts` around lines 7 - 9, The catch block that currently does console.error("Runtime staging failed:", error); should be enhanced to always surface useful debugging info: in the catch for the top-level build routine (the catch handling the runtime staging failure and calling process.exit(1)), detect if error is an Error and log error.stack (or error.message + stack) otherwise serialize the value (e.g., using util.inspect or JSON.stringify with fallback) and include it in the console.error call so a full stack trace or detailed representation is emitted before process.exit(1).apps/cli/build.ts (1)
13-15: Derive the esbuild externals fromCLI_RUNTIME_DEPENDENCIES.The runtime dependency contract now lives in
shared/runtime.ts, but this list is still maintained separately here. If they drift, the CLI build and the staged-runtime validator will stop describing the same artifact.♻️ Proposed refactor
-import { resolveRuntimeStagePaths } from "../../shared/runtime"; +import { CLI_RUNTIME_DEPENDENCIES, resolveRuntimeStagePaths } from "../../shared/runtime"; @@ - external: [ - "better-sqlite3", - "node-pty", - "ws", - "@sentry/node", - "@openai/codex", - "@openai/codex-sdk", - "@napi-rs/canvas", - "agent-browser", - ], + external: [...CLI_RUNTIME_DEPENDENCIES],Also applies to: 32-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/build.ts` around lines 13 - 15, The hard-coded esbuild external list in build.ts should be derived from the canonical CLI_RUNTIME_DEPENDENCIES exported by shared/runtime (instead of duplicating it); import CLI_RUNTIME_DEPENDENCIES and use it to compute the externals array passed to esbuild (e.g., replace the manually maintained externals with Array.from(CLI_RUNTIME_DEPENDENCIES) or a mapping/filter over CLI_RUNTIME_DEPENDENCIES), and remove the duplicated list; ensure the same change is applied to the other block that builds externals (the block referencing stageRuntime/resolveRuntimeStagePaths) so both places use CLI_RUNTIME_DEPENDENCIES as the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 118-121: The current parsing of file:// URIs produces invalid
Windows paths because uri.replace("file://", "") yields a leading slash (e.g.
"/C:/..."); update the logic that computes fsPath in recent-projects.service.ts
to use the URL API (e.g., new URL(uri).pathname) and decode it via
decodeURIComponent, then normalize for Windows by stripping a leading slash when
the pathname matches /^\/[A-Za-z]:\//; adjust the code that calls pushProject so
it passes the corrected fsPath (referencing variables/functions: uri, fsPath,
decodeURIComponent, basename, pushProject) and ensure both POSIX and Windows
paths are handled correctly.
- Around line 279-293: The code currently uses macOS-only hardcoded paths when
calling readVscdbProjects for cursorProjects and vscodeProjects, causing
failures on Windows/Linux; replace those literal joins with a cross-platform
helper (e.g., getVscdbPath or similar) that returns the correct state.vscdb path
based on process.platform (handle "win32" using process.env.APPDATA ||
join(homeDir, "AppData/Roaming"), "darwin" with join(homeDir,
"Library/Application Support", appName, ...), and otherwise join(homeDir,
".config", appName, ...)), then call readVscdbProjects(joinedPath,
"cursor"|"vscode", readerOptions) or use options.readers overrides as before.
In `@apps/backend/test/unit/routes/repos.test.ts`:
- Around line 177-178: The test builds filesystem paths with hardcoded '/' which
won't match normalized paths on Windows; update the test to use path.join() when
constructing targetPath and the related .git path (refer to the targetPath
variable and mockFsExistsSync mock as well as where the test constructs the
repository .git path around lines 193–196) so the mock implementation compares
normalized, platform-correct paths; ensure path is imported in the test file if
not already.
In `@apps/cli/src/start.ts`:
- Around line 20-24: The current check around resolveRuntimeStagePaths only
tests for existence and can pick up stale staged bundles; update the startup
logic in start.ts to validate the staged runtime before using it (e.g., compare
a runtime version/checksum or build-timestamp inside the staged runtime against
the current dist/runtime build) and refuse to use the staged path if it doesn't
match. Specifically, enhance the code that uses resolveRuntimeStagePaths (and
any code that falls back to dist/runtime/common/*) to read a version or checksum
manifest from the staged runtime and compare it to the expected value from the
current build (or from package.json) and only proceed when they match, otherwise
log a clear error and fall back to rebuilding/runtime lookup. Ensure the
validation is applied where existence is currently checked (the block around
resolveRuntimeStagePaths usage) so monorepo mode no longer silently boots stale
bundles.
In `@apps/desktop/main/backend-process.ts`:
- Around line 72-81: The relayWorkspaceProgress function currently targets
BrowserWindow.getAllWindows()[0], which is unstable; change it to broadcast the
payload to all live windows or send to a designated main window reference
instead: inside relayWorkspaceProgress (and the "workspace:progress" send),
iterate BrowserWindow.getAllWindows() and call
win.webContents.send("workspace:progress", payload) for each win, or replace the
getAllWindows()[0] lookup with your known main window variable/ getter (e.g.,
mainWindow) so the event reliably reaches the intended renderer.
In `@apps/desktop/main/index.ts`:
- Around line 41-47: Before calling app.setPath("userData",
canonicalUserDataPath) migrate any existing Electron userData to the new
canonical location: capture the current path via app.getPath("userData") into a
variable (oldUserDataPath), check if oldUserDataPath exists and is different
from canonicalUserDataPath, and if so copy/merge its contents into
canonicalUserDataPath (preserving files/permissions and skipping/merging
existing files) before calling app.setPath; ensure errors are handled and logged
so backend-process.ts (which derives DATABASE_PATH from app.getPath("userData"))
will see the migrated data on upgrade.
In `@apps/desktop/main/startup-diagnostics.ts`:
- Around line 26-39: logMainProcess currently uses appendFileSync (blocking) on
the Electron main thread; replace the synchronous disk append with a
non-blocking writer: create a singleton write stream (or a small buffered async
queue) tied to getMainLogPath and use stream.write(`${line}\n`) instead of
appendFileSync inside logMainProcess, keep the existing
recentLines/MAX_RECENT_LINES logic, ensure the stream is lazily opened once (and
errors are caught and logged without throwing), and add tidy-up handling to
flush/close the stream on app shutdown so logging remains non-blocking while
preserving durability.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 391-398: Replace the npm publish invocation with the repo's
Bun-based publish flow: in the job currently named "Publish to npm"
(working-directory: apps/cli) remove `npm publish --access public` and run the
Bun publish command instead (e.g., `bun publish --access public` or `bunx
publish --access public`) and ensure you run `bun install --frozen-lockfile` in
that job before publishing; keep the build step (`bun run build`) in "Build CLI"
but ensure CI uses only Bun commands across the workflow.
---
Duplicate comments:
In `@apps/web/src/features/onboarding/ui/steps/DeusStep.tsx`:
- Around line 68-74: The code currently treats any existing git repo as
"already_cloned" after calling classifyCloneConflict; instead, when conflictKind
=== "already_cloned" perform an explicit check of the repo's remote origin
(e.g., read the git remote URL from the repo at target) and compare it against
REPO.url (or parsed owner/name) before setting alreadyCloned = true; if the
origin does not match, do not mark alreadyCloned (handle as a
conflicting/unexpected repo: show an error toast or abort) and only call
RepoService.add(target) when the origin matches the expected REPO.url.
In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx`:
- Around line 67-68: The catch in ProjectSelectionStep.tsx currently swallows
the thrown error and always shows “Couldn’t add that folder. Make sure it’s a
git repository.” — change the catch in the code that calls the folder
browse/registration (the try/catch around the add/register logic in
ProjectSelectionStep) to capture the caught error (e.g., const err) and surface
more accurate feedback: include err.message in the toast or branch on known
conditions (e.g., “already registered”, permission denied, path not found) to
show an appropriate toast instead of always blaming git; ensure the toast still
falls back to the original generic message if err.message is absent.
- Around line 39-52: Normalize Promise.allSettled results so rejections with the
"already_cloned"/"Repository already exists" case are treated as successes
before computing succeeded/failed; after calling addRepoMutation.mutateAsync for
each path, map results and count a result as successful if result.status ===
"fulfilled" OR result.status === "rejected" && (result.reason?.code ===
"already_cloned" || /Repository already exists/i.test(result.reason?.message));
use that normalized succeeded count to decide toast messages and whether to call
onNext (so already-registered repos no longer block progress).
---
Nitpick comments:
In `@apps/backend/src/routes/repos.ts`:
- Around line 178-183: The ConflictError thrown in repos handler (references
gitMetadataPath and the two throw new ConflictError(...) sites) must include a
stable machine-readable conflict code (e.g., set a property like error.code =
"TARGET_CONTAINS_GIT" or status = 409) so the frontend can reliably parse it;
update both throw sites to instantiate ConflictError with a consistent
code/value (or pass an explicit { code: "TARGET_CONTAINS_GIT", status: 409 }
payload) instead of only human text, and ensure the ConflictError constructor
(or the place constructing the response) propagates that code into the HTTP 409
response body so clients can map to UI strings.
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 132-138: The sortDirentsByMtime function risks throwing if
statSync fails for an entry; change it to defend per-entry by wrapping the
statSync calls for left and right in try/catch (or compute mtimes in a
pre-pass), treating missing/unstatable entries as the oldest (e.g., mtimeMs =
-Infinity or Number.MIN_SAFE_INTEGER) so they sort to the end, and skip or
ignore entries whose stat fails rather than letting statSync propagate an
exception; update sortDirentsByMtime to compute safeLeftTime/safeRightTime for
left/right using the join(parentDir, dirent.name) and the Dirent names
referenced in the function to locate the code.
In `@apps/cli/build.ts`:
- Around line 13-15: The hard-coded esbuild external list in build.ts should be
derived from the canonical CLI_RUNTIME_DEPENDENCIES exported by shared/runtime
(instead of duplicating it); import CLI_RUNTIME_DEPENDENCIES and use it to
compute the externals array passed to esbuild (e.g., replace the manually
maintained externals with Array.from(CLI_RUNTIME_DEPENDENCIES) or a
mapping/filter over CLI_RUNTIME_DEPENDENCIES), and remove the duplicated list;
ensure the same change is applied to the other block that builds externals (the
block referencing stageRuntime/resolveRuntimeStagePaths) so both places use
CLI_RUNTIME_DEPENDENCIES as the single source of truth.
In `@apps/runtime/build.ts`:
- Around line 7-9: The catch block that currently does console.error("Runtime
staging failed:", error); should be enhanced to always surface useful debugging
info: in the catch for the top-level build routine (the catch handling the
runtime staging failure and calling process.exit(1)), detect if error is an
Error and log error.stack (or error.message + stack) otherwise serialize the
value (e.g., using util.inspect or JSON.stringify with fallback) and include it
in the console.error call so a full stack trace or detailed representation is
emitted before process.exit(1).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a33b9281-614e-4a8d-960e-d7fff687661c
📒 Files selected for processing (35)
.github/workflows/release.ymlREADME.mdapps/backend/build.tsapps/backend/src/lib/database.tsapps/backend/src/routes/onboarding.tsapps/backend/src/routes/repos.tsapps/backend/src/server.tsapps/backend/src/services/recent-projects.service.tsapps/backend/test/unit/routes/repos.test.tsapps/backend/test/unit/services/recent-projects.service.test.tsapps/cli/README.mdapps/cli/build.tsapps/cli/package.jsonapps/cli/src/desktop.tsapps/cli/src/start.tsapps/desktop/main/backend-process.tsapps/desktop/main/index.tsapps/desktop/main/install-preflight.tsapps/desktop/main/startup-diagnostics.tsapps/runtime/build.tsapps/runtime/electron-builder-before-pack.cjsapps/runtime/stage.tsapps/runtime/validate.tsapps/web/src/features/onboarding/lib/deus-import.tsapps/web/src/features/onboarding/ui/steps/DeusStep.tsxapps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsxapps/web/src/features/onboarding/ui/steps/WelcomeStep.tsxapps/web/src/global.csselectron-builder.ymlpackage.jsonshared/runtime.tstest/unit/onboarding/deus-import.test.tstest/unit/runtime/validate-runtime.test.tstest/unit/shared/runtime.test.tstsconfig.node.json
💤 Files with no reviewable changes (1)
- apps/web/src/global.css
Comment on lines
+41
to
+47
| const canonicalUserDataPath = resolveDefaultDataDir({ | ||
| platform: process.platform, | ||
| homeDir: process.env.HOME || homedir(), | ||
| appData: process.env.APPDATA, | ||
| xdgDataHome: process.env.XDG_DATA_HOME, | ||
| }); | ||
| app.setPath("userData", canonicalUserDataPath); |
There was a problem hiding this comment.
Migrate the old userData directory before overriding it.
Line 47 switches Electron to a new data root with no first-run migration. apps/desktop/main/backend-process.ts now derives DATABASE_PATH from app.getPath("userData"), so upgraded installs will boot against a fresh DB/preferences directory and appear to lose state unless the previous Electron userData contents are copied forward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/main/index.ts` around lines 41 - 47, Before calling
app.setPath("userData", canonicalUserDataPath) migrate any existing Electron
userData to the new canonical location: capture the current path via
app.getPath("userData") into a variable (oldUserDataPath), check if
oldUserDataPath exists and is different from canonicalUserDataPath, and if so
copy/merge its contents into canonicalUserDataPath (preserving files/permissions
and skipping/merging existing files) before calling app.setPath; ensure errors
are handled and logged so backend-process.ts (which derives DATABASE_PATH from
app.getPath("userData")) will see the migrated data on upgrade.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx (2)
39-52:⚠️ Potential issue | 🟠 MajorAlready-registered repos are still counted as failures.
The past review concern remains: when
addRepoMutation.mutateAsync()rejects with"Repository already exists", it's counted as a failure. This means:
- If a user selects only already-registered repos, they see "Couldn't add any of the selected projects" even though those repos are valid and available.
- The partial-failure toast (
Added X, but Y failed) is misleading when "failures" include already-present repos.Consider normalizing results by inspecting the rejection reason and treating
"already exists"as a non-fatal outcome:Suggested approach
const results = await Promise.allSettled( [...selectedPaths].map((projectPath) => addRepoMutation.mutateAsync(projectPath)) ); - const succeeded = results.filter((result) => result.status === "fulfilled").length; - const failed = results.length - succeeded; + const isAlreadyExistsError = (reason: unknown) => + reason instanceof Error && reason.message.includes("already exists"); + + const realFailures = results.filter( + (r) => r.status === "rejected" && !isAlreadyExistsError(r.reason) + ).length; + const succeeded = results.length - realFailures; - if (succeeded === 0) { + if (realFailures === results.length) { toast.error("Couldn't add any of the selected projects."); return; } - if (failed > 0) { - toast.error(`Added ${succeeded} project${succeeded > 1 ? "s" : ""}, but ${failed} failed.`); + if (realFailures > 0) { + toast.error(`Added ${succeeded} project${succeeded > 1 ? "s" : ""}, but ${realFailures} failed.`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx` around lines 39 - 52, The Promise.allSettled result handling treats any rejection as a failure; update the results normalization to treat rejections where the rejection reason/message indicates an already-registered repo (e.g., includes "Repository already exists" or similar) as non-fatal: iterate over results from addRepoMutation.mutateAsync, count fulfilled as succeeded, count rejected with reason matching "already exists" as alreadyExists, and count only other rejected as failed; then adjust the toast logic (using variables succeeded, alreadyExists, failed) so that selecting only already-registered repos shows a success/info toast rather than "Couldn’t add any...", and the partial-failure message excludes alreadyExists from failed.
67-68:⚠️ Potential issue | 🟡 MinorGeneric error message loses real failure context.
The past review concern remains: the catch block uses a blanket message regardless of the actual error. An already-registered repo, an inaccessible folder (permissions), or a missing path all receive the same "not a git repository" guidance, which is misleading.
Suggested improvement
- } catch { - toast.error("Couldn't add that folder. Make sure it's a git repository."); + } catch (e) { + const message = e instanceof Error ? e.message : "Unknown error"; + if (message.includes("already exists")) { + toast.error("That repository is already added."); + } else { + toast.error(`Couldn't add that folder: ${message}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx` around lines 67 - 68, The catch block currently swallows the thrown error and always shows "Couldn’t add that folder. Make sure it’s a git repository."; change the catch to capture the error (e.g., catch (err)), inspect err.code or err.message inside ProjectSelectionStep, and produce a more specific toast via toast.error: if err.code is 'EACCES' or similar show a permissions message, if 'ENOENT' show a missing-path message, if the error indicates an already-registered repo show an "already added" message, otherwise include the original err.message (or a short sanitized version) alongside the generic guidance so users and debuggers get real failure context. Ensure you reference the catch block and toast.error usage when making the change.apps/desktop/main/startup-diagnostics.ts (1)
26-35:⚠️ Potential issue | 🟠 MajorKeep
logMainProcess()off the synchronous I/O path.This helper sits on the main-process logging hot path, so
appendFileSync()can stall menus and window events under verbose backend or agent output. A shared write stream or buffered async append keeps diagnostics durable without blocking the Electron thread.Expected result: this should show
appendFileSyncinstartup-diagnostics.tsand the child stdout/stderr plumbing that feedslogMainProcess(...)from the desktop main-process startup code.#!/bin/bash set -euo pipefail rg -n -C2 'appendFileSync|logMainProcess\s*\(|stdout|stderr' apps/desktop/main🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/startup-diagnostics.ts` around lines 26 - 35, logMainProcess is performing synchronous file I/O via appendFileSync on the main Electron thread which can block UI; replace the synchronous call with a non-blocking buffered writer: create a shared async write stream or a background logger task that opens getMainLogPath() once and accepts lines (from logMainProcess) via a small in-memory ring buffer (use recentLines/MAX_RECENT_LINES for in-process retention) or an async queue; update logMainProcess to enqueue the formatted line and return immediately, and ensure errors from the stream are caught and sent to processLogger or a fallback async appendFile to avoid losing diagnostics.apps/cli/src/start.ts (1)
270-279:⚠️ Potential issue | 🟠 MajorValidate the staged runtime before accepting these dev-mode bundle paths.
This block still trusts existence alone. If someone rebuilds
apps/backend/distorapps/agent-server/distwithout rerunningbuild:runtime,deus startcan silently boot staledist/runtime/common/*artifacts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/start.ts` around lines 270 - 279, The current existence-only check for runtimePaths.common.agentServerBundle and backendBundle can return stale staged bundles; add a strict validation step before returning those paths: implement and call a helper (e.g., validateStagedRuntime or ensureRuntimeIsFresh) from start.ts that (1) verifies a runtime manifest or marker (e.g., runtime_manifest.json) exists in runtimePaths.common and its version/timestamp matches the monorepo/package version, or (2) compares mtimes of runtimePaths.common.* bundles against the source dist folders (apps/backend/dist and apps/agent-server/dist) to ensure bundles are newer; if validation fails, do not return the staged bundle paths so the code will fall through to trigger build:runtime or a rebuild prompt for deus start. Ensure the helper references resolveRuntimeStagePaths, runtimePaths.common.agentServerBundle, and runtimePaths.common.backendBundle so it's easy to locate and replace the simple existsSync checks.apps/desktop/main/index.ts (1)
41-47:⚠️ Potential issue | 🟠 MajorMigrate existing userData before overriding the path.
Setting a new canonical
userDatapath without migrating existing data will cause upgraded users to lose their database and preferences. The previous review flagged this same concern.🛡️ Proposed migration logic
+import { existsSync, cpSync } from "fs"; + const canonicalUserDataPath = resolveDefaultDataDir({ platform: process.platform, homeDir: process.env.HOME || homedir(), appData: process.env.APPDATA, xdgDataHome: process.env.XDG_DATA_HOME, }); + +// Migrate old userData if it exists and differs from canonical path +const oldUserDataPath = app.getPath("userData"); +if (oldUserDataPath !== canonicalUserDataPath && existsSync(oldUserDataPath) && !existsSync(canonicalUserDataPath)) { + try { + cpSync(oldUserDataPath, canonicalUserDataPath, { recursive: true }); + console.log(`[main] Migrated userData from ${oldUserDataPath} to ${canonicalUserDataPath}`); + } catch (err) { + console.error(`[main] Failed to migrate userData:`, err); + } +} + app.setPath("userData", canonicalUserDataPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/index.ts` around lines 41 - 47, You are overriding Electron's userData path (app.setPath("userData", canonicalUserDataPath)) without migrating existing data, which will make upgraded users lose DB/preferences; before calling app.setPath, check the current path via app.getPath("userData"), and if it differs from canonicalUserDataPath, perform a safe migration: stop writes, copy/merge the existing directory to the new canonical path (preserving permissions and hidden files), prefer existing files in the destination or perform a safe backup/rename strategy (e.g., move original to a .bak if copy fails), handle errors and race conditions, and only set app.setPath("userData", canonicalUserDataPath) after successful migration; use resolveDefaultDataDir and canonicalUserDataPath as the target identifiers and guard the migration to run once at startup.apps/desktop/main/backend-process.ts (1)
72-85:⚠️ Potential issue | 🟠 MajorBroadcast workspace progress to all windows, not just the first.
BrowserWindow.getAllWindows()[0]is unstable when the detached browser window exists. The previous review flagged this same issue.🐛 Fix: Broadcast to all live windows
function relayWorkspaceProgress(line: string): void { if (!line.startsWith("DEUS_WORKSPACE_PROGRESS:")) return; const jsonStr = line.slice("DEUS_WORKSPACE_PROGRESS:".length); try { const payload = JSON.parse(jsonStr); - const win = BrowserWindow.getAllWindows()[0]; - if (win) { - win.webContents.send("workspace:progress", payload); + for (const win of BrowserWindow.getAllWindows()) { + if (!win.isDestroyed()) { + win.webContents.send("workspace:progress", payload); + } } } catch { // Ignore malformed progress lines } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/backend-process.ts` around lines 72 - 85, The relayWorkspaceProgress function currently sends "workspace:progress" only to the first window using BrowserWindow.getAllWindows()[0], which fails when detached windows change order; update relayWorkspaceProgress to iterate over BrowserWindow.getAllWindows() and call webContents.send("workspace:progress", payload) for each live window (use the same payload and preserve the try/catch for JSON.parse), ensuring you reference the existing symbols relayWorkspaceProgress, BrowserWindow.getAllWindows, win.webContents.send, "workspace:progress", and payload when making the change.
🧹 Nitpick comments (4)
apps/web/src/features/onboarding/ui/steps/WelcomeStep.tsx (1)
51-112: Use Framer Motion for the onboarding choreography.This step uses inline CSS transitions with state-driven phase changes instead of Framer Motion. Migrate this animation choreography to Framer Motion variants/transitions per repo standards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/onboarding/ui/steps/WelcomeStep.tsx` around lines 51 - 112, The component currently uses inline style transitions and local state (phase, isAnimating, showDevs) for choreography; replace those with Framer Motion variants and motion components. Import motion from 'framer-motion', define variants for the title container (states: center, swap, exit) that map to transform/opacity values currently produced in the style block, define opacity variants for the two overlapping spans ("Deus" and "Devs") to handle the crossfade, and add a variant for the CTA wrapper/button that animates based on isAnimating; then swap the div/h1/span/button elements to motion.div/motion.h1/motion.span/motion.button and drive their animate prop from the same phase/isAnimating state (or map phase -> variant key) while using transition objects (matching EASE_OUT_QUART_CSS timing) instead of inline transition styles. Ensure refs like titleRef and handler names (handleRun) remain and preserve accessibility/classes.apps/backend/src/routes/onboarding.ts (1)
6-8: Consider async execution for file I/O operations.
listRecentProjects()performs synchronous file operations (SQLite queries,readdirSync,statSync,execFileSync). On a busy server this could block the event loop. Consider making the service async or running it in a worker, though for desktop use this is likely acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/routes/onboarding.ts` around lines 6 - 8, listRecentProjects currently runs synchronous I/O (SQLite queries, fs.readdirSync/statSync, execFileSync) which can block the event loop; change it to an asynchronous implementation or offload it to a worker: replace sync filesystem calls with their async counterparts (fs.promises.readdir, fs.promises.stat), use an async SQLite API or promisified queries, and replace execFileSync with child_process.execFile/promises.execFile; then update the route handler in onboarding.ts to await listRecentProjects() (i.e., app.get("/onboarding/recent-projects", async (c) => c.json({ projects: await listRecentProjects() }))) or dispatch the work to a worker thread if heavy CPU is expected.apps/runtime/validate.ts (1)
101-107: Consider using deep equality instead of JSON.stringify for manifest comparison.
JSON.stringifycomparison is key-order sensitive. If the manifest is written with different key ordering (e.g., due to object spread differences), validation could fail on identical content. This is currently mitigated bybuildExpectedManifestandstageRuntimeusing the same property order, but it's fragile.♻️ Safer alternative using sorted stringify
+function stableStringify(obj: unknown): string { + return JSON.stringify(obj, Object.keys(obj as object).sort()); +} + const manifest = readManifest(stagePaths.manifest); const expectedManifest = buildExpectedManifest(projectRoot); -if (JSON.stringify(manifest) !== JSON.stringify(expectedManifest)) { +if (stableStringify(manifest) !== stableStringify(expectedManifest)) { throw createBuildRuntimeError(Alternatively, use a deep-equal utility if available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/runtime/validate.ts` around lines 101 - 107, The manifest comparison using JSON.stringify is key-order sensitive; replace it with a proper deep equality check between the object returned by readManifest(stagePaths.manifest) and buildExpectedManifest(projectRoot) (or canonicalize both objects by sorting object keys recursively before stringifying) to avoid false negatives; update the block that currently compares JSON.stringify(manifest) !== JSON.stringify(expectedManifest) (referencing readManifest, buildExpectedManifest, and createBuildRuntimeError) to use a deep-equal utility (or a small recursive key-sorting function) and throw the same createBuildRuntimeError if they differ.test/unit/runtime/validate-runtime.test.ts (1)
49-72: Good coverage of the core validation scenarios.The tests verify both the happy path (fresh staging) and the staleness detection via
utimesSync. Consider adding a test for missing source bundles to complete the validation contract coverage.💡 Optional: Add test for missing bundles
it("fails when source bundles are missing", () => { const projectRoot = createTempProjectRoot(); // Don't write fixture - bundles will be missing expect(() => validateRuntimeStage({ projectRoot, log: () => {} })).toThrow( /Missing.*bundle/ ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/runtime/validate-runtime.test.ts` around lines 49 - 72, Add a new unit test that asserts validateRuntimeStage throws when source bundles are missing: create a temp project root with createTempProjectRoot(), deliberately do NOT call writeProjectFixture() or stageRuntime(), then call validateRuntimeStage({ projectRoot, log: () => {} }) inside an expect(...).toThrow(...) asserting a message matching /Missing.*bundle/ (or the exact error text thrown by validateRuntimeStage) to cover the missing-bundles validation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/server.ts`:
- Around line 127-136: The startup currently warns when AGENT_SERVER_URL is
missing but still calls serve(...) and emits readiness; fix by validating
process.env.AGENT_SERVER_URL before serve(...) and aborting startup
(process.exit(1)) if it's absent, or alternatively require an explicit opt-in
like AGENT_ALLOW_AGENTLESS=true to allow agentless mode. Specifically, move the
AGENT_SERVER_URL check so it runs before serve(...) and only call
agentService.init(agentServerUrl) when present; if opting into explicit opt-in
mode, read AGENT_ALLOW_AGENTLESS and only proceed without agentService.init when
that flag is truthy, otherwise log a clear error and exit. Ensure references:
AGENT_SERVER_URL, AGENT_ALLOW_AGENTLESS (new), agentService.init, and serve are
updated accordingly.
In `@apps/cli/src/desktop.ts`:
- Around line 319-321: The current flow deletes the existing app (existsSync
check then execSync rm -rf on destPath) before copying with ditto; change this
to stage the new bundle into a temporary sibling directory (create a temp path
next to destPath, use execSync ditto to copy appPath into that temp), verify the
ditto succeeded, then atomically replace the existing destPath (rename/move the
temp sibling to destPath or use an atomic move) and only remove the old bundle
after successful swap; update references to appPath and destPath and replace the
two execSync calls so the sequence is: ditto -> verify -> move/rename temp ->
cleanup.
In `@apps/web/src/features/onboarding/lib/deus-import.ts`:
- Around line 13-19: The conditional in deus-import.ts that checks
normalizedMessage.includes("destination path") is too broad and can yield false
positives; update the check inside the if-block that returns "already_cloned" to
require the fuller git message context (e.g., match "destination path '... '
already exists") or simply remove the "destination path" check if the other two
checks ("already contains a git repository" and "repository already exists")
suffice; implement this by replacing the plain includes("destination path") test
on normalizedMessage with a more specific pattern match (regex) or combine it
with an additional includes check so only messages like "destination path '...'
already exists" trigger the "already_cloned" branch.
In `@README.md`:
- Around line 24-28: Change the ambiguous "### Desktop app" section to
explicitly indicate macOS (e.g., "### Desktop app (macOS)") and update the
paragraph that describes DMG steps (Open the DMG, drag `Deus.app` into
`Applications`...) to state these steps are macOS-specific; also add a short
one-line note for other platforms such as "For Linux, see <link or other
section>" or "Linux users: follow the Linux desktop installation instructions
below" to avoid confusion. Target the heading "### Desktop app" and the
DMG/install instructions in README.md when making this change.
---
Duplicate comments:
In `@apps/cli/src/start.ts`:
- Around line 270-279: The current existence-only check for
runtimePaths.common.agentServerBundle and backendBundle can return stale staged
bundles; add a strict validation step before returning those paths: implement
and call a helper (e.g., validateStagedRuntime or ensureRuntimeIsFresh) from
start.ts that (1) verifies a runtime manifest or marker (e.g.,
runtime_manifest.json) exists in runtimePaths.common and its version/timestamp
matches the monorepo/package version, or (2) compares mtimes of
runtimePaths.common.* bundles against the source dist folders (apps/backend/dist
and apps/agent-server/dist) to ensure bundles are newer; if validation fails, do
not return the staged bundle paths so the code will fall through to trigger
build:runtime or a rebuild prompt for deus start. Ensure the helper references
resolveRuntimeStagePaths, runtimePaths.common.agentServerBundle, and
runtimePaths.common.backendBundle so it's easy to locate and replace the simple
existsSync checks.
In `@apps/desktop/main/backend-process.ts`:
- Around line 72-85: The relayWorkspaceProgress function currently sends
"workspace:progress" only to the first window using
BrowserWindow.getAllWindows()[0], which fails when detached windows change
order; update relayWorkspaceProgress to iterate over
BrowserWindow.getAllWindows() and call webContents.send("workspace:progress",
payload) for each live window (use the same payload and preserve the try/catch
for JSON.parse), ensuring you reference the existing symbols
relayWorkspaceProgress, BrowserWindow.getAllWindows, win.webContents.send,
"workspace:progress", and payload when making the change.
In `@apps/desktop/main/index.ts`:
- Around line 41-47: You are overriding Electron's userData path
(app.setPath("userData", canonicalUserDataPath)) without migrating existing
data, which will make upgraded users lose DB/preferences; before calling
app.setPath, check the current path via app.getPath("userData"), and if it
differs from canonicalUserDataPath, perform a safe migration: stop writes,
copy/merge the existing directory to the new canonical path (preserving
permissions and hidden files), prefer existing files in the destination or
perform a safe backup/rename strategy (e.g., move original to a .bak if copy
fails), handle errors and race conditions, and only set app.setPath("userData",
canonicalUserDataPath) after successful migration; use resolveDefaultDataDir and
canonicalUserDataPath as the target identifiers and guard the migration to run
once at startup.
In `@apps/desktop/main/startup-diagnostics.ts`:
- Around line 26-35: logMainProcess is performing synchronous file I/O via
appendFileSync on the main Electron thread which can block UI; replace the
synchronous call with a non-blocking buffered writer: create a shared async
write stream or a background logger task that opens getMainLogPath() once and
accepts lines (from logMainProcess) via a small in-memory ring buffer (use
recentLines/MAX_RECENT_LINES for in-process retention) or an async queue; update
logMainProcess to enqueue the formatted line and return immediately, and ensure
errors from the stream are caught and sent to processLogger or a fallback async
appendFile to avoid losing diagnostics.
In `@apps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsx`:
- Around line 39-52: The Promise.allSettled result handling treats any rejection
as a failure; update the results normalization to treat rejections where the
rejection reason/message indicates an already-registered repo (e.g., includes
"Repository already exists" or similar) as non-fatal: iterate over results from
addRepoMutation.mutateAsync, count fulfilled as succeeded, count rejected with
reason matching "already exists" as alreadyExists, and count only other rejected
as failed; then adjust the toast logic (using variables succeeded,
alreadyExists, failed) so that selecting only already-registered repos shows a
success/info toast rather than "Couldn’t add any...", and the partial-failure
message excludes alreadyExists from failed.
- Around line 67-68: The catch block currently swallows the thrown error and
always shows "Couldn’t add that folder. Make sure it’s a git repository.";
change the catch to capture the error (e.g., catch (err)), inspect err.code or
err.message inside ProjectSelectionStep, and produce a more specific toast via
toast.error: if err.code is 'EACCES' or similar show a permissions message, if
'ENOENT' show a missing-path message, if the error indicates an
already-registered repo show an "already added" message, otherwise include the
original err.message (or a short sanitized version) alongside the generic
guidance so users and debuggers get real failure context. Ensure you reference
the catch block and toast.error usage when making the change.
---
Nitpick comments:
In `@apps/backend/src/routes/onboarding.ts`:
- Around line 6-8: listRecentProjects currently runs synchronous I/O (SQLite
queries, fs.readdirSync/statSync, execFileSync) which can block the event loop;
change it to an asynchronous implementation or offload it to a worker: replace
sync filesystem calls with their async counterparts (fs.promises.readdir,
fs.promises.stat), use an async SQLite API or promisified queries, and replace
execFileSync with child_process.execFile/promises.execFile; then update the
route handler in onboarding.ts to await listRecentProjects() (i.e.,
app.get("/onboarding/recent-projects", async (c) => c.json({ projects: await
listRecentProjects() }))) or dispatch the work to a worker thread if heavy CPU
is expected.
In `@apps/runtime/validate.ts`:
- Around line 101-107: The manifest comparison using JSON.stringify is key-order
sensitive; replace it with a proper deep equality check between the object
returned by readManifest(stagePaths.manifest) and
buildExpectedManifest(projectRoot) (or canonicalize both objects by sorting
object keys recursively before stringifying) to avoid false negatives; update
the block that currently compares JSON.stringify(manifest) !==
JSON.stringify(expectedManifest) (referencing readManifest,
buildExpectedManifest, and createBuildRuntimeError) to use a deep-equal utility
(or a small recursive key-sorting function) and throw the same
createBuildRuntimeError if they differ.
In `@apps/web/src/features/onboarding/ui/steps/WelcomeStep.tsx`:
- Around line 51-112: The component currently uses inline style transitions and
local state (phase, isAnimating, showDevs) for choreography; replace those with
Framer Motion variants and motion components. Import motion from
'framer-motion', define variants for the title container (states: center, swap,
exit) that map to transform/opacity values currently produced in the style
block, define opacity variants for the two overlapping spans ("Deus" and "Devs")
to handle the crossfade, and add a variant for the CTA wrapper/button that
animates based on isAnimating; then swap the div/h1/span/button elements to
motion.div/motion.h1/motion.span/motion.button and drive their animate prop from
the same phase/isAnimating state (or map phase -> variant key) while using
transition objects (matching EASE_OUT_QUART_CSS timing) instead of inline
transition styles. Ensure refs like titleRef and handler names (handleRun)
remain and preserve accessibility/classes.
In `@test/unit/runtime/validate-runtime.test.ts`:
- Around line 49-72: Add a new unit test that asserts validateRuntimeStage
throws when source bundles are missing: create a temp project root with
createTempProjectRoot(), deliberately do NOT call writeProjectFixture() or
stageRuntime(), then call validateRuntimeStage({ projectRoot, log: () => {} })
inside an expect(...).toThrow(...) asserting a message matching
/Missing.*bundle/ (or the exact error text thrown by validateRuntimeStage) to
cover the missing-bundles validation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b77da88e-f80c-472b-be9b-1dc542bf01e5
📒 Files selected for processing (35)
.github/workflows/release.ymlREADME.mdapps/backend/build.tsapps/backend/src/lib/database.tsapps/backend/src/routes/onboarding.tsapps/backend/src/routes/repos.tsapps/backend/src/server.tsapps/backend/src/services/recent-projects.service.tsapps/backend/test/unit/routes/repos.test.tsapps/backend/test/unit/services/recent-projects.service.test.tsapps/cli/README.mdapps/cli/build.tsapps/cli/package.jsonapps/cli/src/desktop.tsapps/cli/src/start.tsapps/desktop/main/backend-process.tsapps/desktop/main/index.tsapps/desktop/main/install-preflight.tsapps/desktop/main/startup-diagnostics.tsapps/runtime/build.tsapps/runtime/electron-builder-before-pack.cjsapps/runtime/stage.tsapps/runtime/validate.tsapps/web/src/features/onboarding/lib/deus-import.tsapps/web/src/features/onboarding/ui/steps/DeusStep.tsxapps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsxapps/web/src/features/onboarding/ui/steps/WelcomeStep.tsxapps/web/src/global.csselectron-builder.ymlpackage.jsonshared/runtime.tstest/unit/onboarding/deus-import.test.tstest/unit/runtime/validate-runtime.test.tstest/unit/shared/runtime.test.tstsconfig.node.json
💤 Files with no reviewable changes (1)
- apps/web/src/global.css
Improve recent-project discovery by using newer Claude session activity, Windows-safe file URI parsing, and cross-platform editor state paths. Verify the Deus repo before treating an existing checkout as already cloned, handle already-registered projects cleanly during onboarding, and preserve real import errors. Also harden runtime launch behavior by validating staged CLI runtime output, requiring AGENT_SERVER_URL unless agentless mode is explicit, broadcasting workspace progress to all windows, and making macOS app replacement safer.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/backend/src/routes/repos.ts (1)
41-93: Blocking synchronous calls on request thread.
inspectRepositoryRootuses synchronous I/O (fs.realpathSync,fs.statSync,execFileSync) which blocks the request thread. While the 2-second timeouts mitigate runaway git commands, under load these blocking calls could degrade throughput.Consider refactoring to async equivalents (
fs.promises.realpath,fs.promises.stat,execFileAsync) if this endpoint sees concurrent traffic. For now, if inspection is infrequent (onboarding only), this is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/routes/repos.ts` around lines 41 - 93, The inspectRepositoryRoot function currently uses blocking APIs (fs.realpathSync, fs.accessSync, fs.statSync, execFileSync and the git remote execFileSync) which can block the request thread; refactor inspectRepositoryRoot to be async: use fs.promises.realpath, fs.promises.access, fs.promises.stat and replace execFileSync calls with an async execFile wrapper (e.g., util.promisify(child_process.execFile) or child_process.spawn with a Promise) for both git rev-parse and git remote get-url; update the function signature to async and await those calls, propagate the async change to callers (and to detectDefaultBranch if it currently uses sync git calls) and preserve the existing timeout/error handling and same thrown ValidationError/AppError semantics.apps/backend/src/services/recent-projects.service.ts (1)
309-317: The first Claude directory sort is redundant.
entriesis ordered by directorymtime, butresolvedProjectsis immediately re-sorted byactivityMtimeMs, so the first sort can't change the result. Dropping it removes extrastatSyncwork on this request path.♻️ Suggested simplification
- const entries = sortDirentsByMtime( - readdirSync(dir, { withFileTypes: true }).filter((entry) => entry.isDirectory()), - dir - ); + const entries = readdirSync(dir, { withFileTypes: true }).filter((entry) => + entry.isDirectory() + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/services/recent-projects.service.ts` around lines 309 - 317, The initial sort using sortDirentsByMtime is redundant because resolvedProjects is immediately re-sorted by activityMtimeMs; remove the call to sortDirentsByMtime and pass the raw filtered Dirent array from readdirSync(...).filter(entry => entry.isDirectory()) into the subsequent map/resolveClaudeProject pipeline (leave resolveClaudeProject, the .filter(project): project is ResolvedClaudeProject and the final .sort((left,right) => right.activityMtimeMs - left.activityMtimeMs) intact) so you avoid the extra statSync work performed by sortDirentsByMtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 175-184: The loop in recent-projects.service.ts only uses
entry.folderUri and therefore drops workspace entries; update the loop that
iterates data.entries (the block containing parseFileUriPath, basename, and
pushProject) to also check for entry.workspace?.configPath, ensure it starts
with "file://", convert it to a filesystem path via parseFileUriPath, take its
parent directory (not the .code-workspace file path) and then call
pushProject(projects, seenPaths, { path: parentFsPath, name:
basename(parentFsPath), source }, options); preserve existing guards (skip if no
URI or parse fails) and reuse parseFileUriPath, basename, pushProject, projects,
seenPaths, source, and options identifiers.
- Around line 71-80: The parseFileUriPath function currently uses new
URL(uri).pathname which drops the host for file:// URIs (breaking UNC paths);
update parseFileUriPath to use Node's fileURLToPath from "node:url" to correctly
convert file URIs (including drive letters and UNC/server share paths) to
filesystem paths, keep the existing try/catch semantics and return type, and
remove the manual Windows slice logic since fileURLToPath handles drive-letter
and UNC normalization for you.
In `@apps/desktop/main/backend-process.ts`:
- Around line 143-145: The restart logic skips recovery while startupInProgress
is true, so modify startup flow to register child processes immediately upon
spawning (e.g., in spawnBackend() and wherever agent-server is spawned) by
adding them to the same child-tracking structure used by scheduleRestart(), and
if a child exits during startup either reject/fail the pending startup promise
(so spawnBackend()/startup flow cannot resolve as successful) or enqueue a
restart request that scheduleRestart() will execute once startupInProgress
clears; ensure checks for isQuitting, restartAttempt and MAX_RESTART_ATTEMPTS
remain enforced but that spawnBackend() and the agent-server spawn path both
call the common registerChild(...) routine right after child process creation so
exits aren’t lost.
- Around line 120-140: The shutdown currently is fire-and-forget which allows
new spawns to race with old processes and abort the restart loop; change
stopRuntimeChildren to be async and await termination of both backendProcess and
agentServerProcess by making terminateManagedProcess return a Promise that
resolves when the child has actually exited (or is null) and after the force
kill timeout is cleared; then call await stopRuntimeChildren() before attempting
spawnBackend() so respawns serialize; also modify the restart logic that calls
spawnBackend() so that on rejection it continues retrying up to
MAX_RESTART_ATTEMPTS (don’t swallow errors) instead of stopping after the first
failure. Ensure you reference terminateManagedProcess, stopRuntimeChildren,
spawnBackend, backendProcess, agentServerProcess, and MAX_RESTART_ATTEMPTS when
making these changes.
---
Nitpick comments:
In `@apps/backend/src/routes/repos.ts`:
- Around line 41-93: The inspectRepositoryRoot function currently uses blocking
APIs (fs.realpathSync, fs.accessSync, fs.statSync, execFileSync and the git
remote execFileSync) which can block the request thread; refactor
inspectRepositoryRoot to be async: use fs.promises.realpath, fs.promises.access,
fs.promises.stat and replace execFileSync calls with an async execFile wrapper
(e.g., util.promisify(child_process.execFile) or child_process.spawn with a
Promise) for both git rev-parse and git remote get-url; update the function
signature to async and await those calls, propagate the async change to callers
(and to detectDefaultBranch if it currently uses sync git calls) and preserve
the existing timeout/error handling and same thrown ValidationError/AppError
semantics.
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 309-317: The initial sort using sortDirentsByMtime is redundant
because resolvedProjects is immediately re-sorted by activityMtimeMs; remove the
call to sortDirentsByMtime and pass the raw filtered Dirent array from
readdirSync(...).filter(entry => entry.isDirectory()) into the subsequent
map/resolveClaudeProject pipeline (leave resolveClaudeProject, the
.filter(project): project is ResolvedClaudeProject and the final
.sort((left,right) => right.activityMtimeMs - left.activityMtimeMs) intact) so
you avoid the extra statSync work performed by sortDirentsByMtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 438d0eee-dfcd-4177-9eef-7a5147a621d9
📒 Files selected for processing (12)
apps/backend/src/routes/repos.tsapps/backend/src/server.tsapps/backend/src/services/recent-projects.service.tsapps/backend/test/unit/routes/repos.test.tsapps/backend/test/unit/services/recent-projects.service.test.tsapps/cli/src/desktop.tsapps/cli/src/start.tsapps/desktop/main/backend-process.tsapps/web/src/features/onboarding/lib/deus-import.tsapps/web/src/features/onboarding/ui/steps/DeusStep.tsxapps/web/src/features/onboarding/ui/steps/ProjectSelectionStep.tsxtest/unit/onboarding/deus-import.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/cli/src/start.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/unit/onboarding/deus-import.test.ts
- apps/cli/src/desktop.ts
- apps/web/src/features/onboarding/lib/deus-import.ts
- apps/backend/test/unit/routes/repos.test.ts
- apps/backend/test/unit/services/recent-projects.service.test.ts
Handle editor recent-project discovery more reliably across platforms and make desktop runtime restarts wait for child shutdown before respawning. Also clarify that the DMG install flow in the README is macOS-specific.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/main/backend-process.ts (1)
287-307:⚠️ Potential issue | 🟠 MajorAgent-server can be dead during startup success window.
If
agent-serveremitsLISTEN_URLand crashes beforebackendemits[BACKEND_PORT], the exit handler callsscheduleRestart()but queues it topendingRestartHooks(sincestartupInProgressis true). Meanwhile,backendsucceeds andspawnBackendreturns{ port, authToken }. The caller inindex.ts(line 194) then uses this port without validating thatagent-serveris still alive—only after the caller's code runs does the finally block trigger the queued restart.Add a liveness check before returning
{ port, authToken }, or explicitly document why accepting this brief partial-failure window is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/backend-process.ts` around lines 287 - 307, The exit handler for child processes (inside the child.on("exit") callback using consumeExpectedExit, scheduleRestart, pendingRestartHooks and startupInProgress) can let agent-server die after emitting LISTEN_URL but before backend emits BACKEND_PORT, causing spawnBackend to return { port, authToken } while agent-server is dead; fix by adding a liveness check for the agent-server process before returning from spawnBackend (or in the caller in index.ts where spawnBackend result is used): verify the agent-server child is still running (e.g., check the process ref or process.pid/alive or a health probe) after backend reports port/authToken and before resolving/returning, and if the agent-server is dead treat it as startup failure (reject/throw) so the queued scheduleRestart is processed immediately; update spawnBackend (or the call-site) to perform this check and handle the failure path consistently with consumeExpectedExit/clearProcessRef logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 136-154: The synchronous git probe resolveGitProjectRoot is being
invoked for every candidate which can block startup; reorder and bound probes in
pushProject and the readers: first check existsSync(project.path),
seenPaths.has(project.path) and isIgnoredRecentProjectPath(project.path,
options) using the raw path before calling resolveGitProjectRoot, and add a
probe counter (e.g., probeCount / probeLimit derived from options.limit) so
pushProject/listRecentProjects and the reader loops stop attempting
resolveGitProjectRoot once the probe limit is reached; ensure listRecentProjects
passes the limit via ReaderOptions and honor that cap in the reader functions
and pushProject to avoid unbounded synchronous git subprocesses.
- Around line 242-247: The code currently shifts the first JSONL line whenever
truncated is true, which incorrectly drops a valid record when the 16KB suffix
begins exactly at a newline; change the condition to only discard the first line
when the suffix actually starts mid-record by checking the raw suffix content:
use the returned contents from readJsonlSuffix and only call lines.shift() if
truncated && contents[0] !== "\n" && lines.length > 0 (so you keep valid lines
when the suffix begins at a newline).
---
Duplicate comments:
In `@apps/desktop/main/backend-process.ts`:
- Around line 287-307: The exit handler for child processes (inside the
child.on("exit") callback using consumeExpectedExit, scheduleRestart,
pendingRestartHooks and startupInProgress) can let agent-server die after
emitting LISTEN_URL but before backend emits BACKEND_PORT, causing spawnBackend
to return { port, authToken } while agent-server is dead; fix by adding a
liveness check for the agent-server process before returning from spawnBackend
(or in the caller in index.ts where spawnBackend result is used): verify the
agent-server child is still running (e.g., check the process ref or
process.pid/alive or a health probe) after backend reports port/authToken and
before resolving/returning, and if the agent-server is dead treat it as startup
failure (reject/throw) so the queued scheduleRestart is processed immediately;
update spawnBackend (or the call-site) to perform this check and handle the
failure path consistently with consumeExpectedExit/clearProcessRef logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ddb8e2e-7321-49ce-ad56-a0f0ec5818da
📒 Files selected for processing (4)
README.mdapps/backend/src/services/recent-projects.service.tsapps/backend/test/unit/services/recent-projects.service.test.tsapps/desktop/main/backend-process.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/test/unit/services/recent-projects.service.test.ts
Limit expensive synchronous git root probes during onboarding recent-project collection and preserve valid Claude JSONL records when the scan window starts on a newline boundary.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/backend/src/services/recent-projects.service.ts (1)
370-373:⚠️ Potential issue | 🟠 Major
readClaudeProjects()still walks the full Claude history before honoring the cap.Line 370 resolves every project directory up front, and
resolveClaudeProject()does synchronousreaddirSync/statSync/readSyncwork. Since/onboarding/recent-projectscalls this inline, latency still scales with the size of~/.claude/projectseven whenlimitis small. Please add a scan budget/cap to this phase too instead of materializingresolvedProjectsfor the entire tree first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/services/recent-projects.service.ts` around lines 370 - 373, readClaudeProjects() currently maps all entries and materializes resolvedProjects even when limit is small; change the logic to iterate the entries sequentially (use the existing entries array), for each entry call resolveClaudeProject(join(dir, entry.name), options) and push non-null results into resolvedProjects, and stop scanning once you have collected the requested limit (and optionally a small extra scan budget) to avoid resolving the entire tree; reference entries, resolveClaudeProject(), resolvedProjects and readClaudeProjects() when making this change so the synchronous readdirSync/statSync/readSync work stops early instead of running for every entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 166-178: The loop should deduplicate raw candidate paths before
doing any Git probing: introduce a new Set (e.g., rawSeenPaths) and check
rawSeenPaths.has(project.path) early (alongside the existing checks) to return
immediately for duplicate raw entries, then add project.path to rawSeenPaths
once seen; keep the existing seenPaths.add(projectRoot) logic for canonical
roots and only call consumeGitProbeBudget()/resolveGitProjectRoot() for raw
paths that haven't been seen yet so duplicate workspace/subdirectory entries
won't burn the shared maxGitProbes budget.
---
Duplicate comments:
In `@apps/backend/src/services/recent-projects.service.ts`:
- Around line 370-373: readClaudeProjects() currently maps all entries and
materializes resolvedProjects even when limit is small; change the logic to
iterate the entries sequentially (use the existing entries array), for each
entry call resolveClaudeProject(join(dir, entry.name), options) and push
non-null results into resolvedProjects, and stop scanning once you have
collected the requested limit (and optionally a small extra scan budget) to
avoid resolving the entire tree; reference entries, resolveClaudeProject(),
resolvedProjects and readClaudeProjects() when making this change so the
synchronous readdirSync/statSync/readSync work stops early instead of running
for every entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7244e76-9178-4036-818a-4d0fe2017aac
📒 Files selected for processing (2)
apps/backend/src/services/recent-projects.service.tsapps/backend/test/unit/services/recent-projects.service.test.ts
Validate the Claude workspace cwd before SDK spawn so deleted or transient workspaces fail with a clear path error instead of the SDK's misleading "binary not found" message.
This was referenced Apr 16, 2026
This was referenced May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Risk tier
Tier 1 — Critical
Changed areas: runtime/build pipeline, desktop main process, backend routes/services/database path, CLI packaging, onboarding UI.
Test plan
bun run typecheckbunx vitest run --config apps/backend/vitest.config.ts apps/backend/test/unit/services/recent-projects.service.test.ts apps/backend/test/unit/routes/repos.test.tsbunx vitest run --config test/vitest.config.ts test/unit/onboarding/deus-import.test.ts test/unit/runtime/validate-runtime.test.ts test/unit/shared/runtime.test.tsbun run build:allCSC_IDENTITY_AUTO_DISCOVERY=false bunx electron-builder --dir --mac --arm64 --publish neverChanges
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation