Sync fork with upstream/main and preserve multi-provider integrations#39
Merged
aaditagrawal merged 46 commits intomainfrom Mar 30, 2026
Merged
Sync fork with upstream/main and preserve multi-provider integrations#39aaditagrawal merged 46 commits intomainfrom
aaditagrawal merged 46 commits intomainfrom
Conversation
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: smalitobules <207318034+smalitobules@users.noreply.github.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
…otgg#1488) Co-authored-by: Julius Marminge <julius0216@outlook.com>
…SIS installer during update process (pingdotgg#1461) Co-authored-by: Daniel Schwarz <daniel.schwarz@serviceware-se.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
…to open context menu (pingdotgg#873) Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Julius Marminge <julius0216@outlook.com>
Co-authored-by: Jono Kemball <jono-kemball@idexx.com> Co-authored-by: Ariaj Sarkar <rajsarkar02205@gmail.com> Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.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 thread-archive commands/events and DB migrations, exposes generateThreadTitle across text-generation providers, broadens provider probing and settings compatibility, standardizes worker forking/start semantics, improves Git output truncation and bootstrap FD handling, refactors web settings/routes, and adds tests, utilities, and a mock update server. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant API as Server API
participant Engine as Orchestration Engine
participant DB as Projection DB
rect rgba(128, 200, 255, 0.5)
UI->>API: dispatch `thread.archive` command
API->>Engine: enqueue command for processing
Engine->>Engine: decideOrchestrationCommand -> emit `thread.archived` event
Engine->>DB: persist orchestration event
DB-->>Engine: ack
Engine->>DB: projection upsert sets `archived_at`
DB-->>API: projection read returns updated thread
API-->>UI: respond with updated read model / confirmation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/syncShellEnvironment.ts (1)
10-15:⚠️ Potential issue | 🟠 MajorLinux fallback shell is still macOS-specific.
Line 11 enables Linux, but Line 14 still falls back to
"/bin/zsh". On Linux hosts without zsh (and missingSHELL), env sync silently no-ops due to the catch block.💡 Proposed fix
-import { readEnvironmentFromLoginShell, ShellEnvironmentReader } from "@t3tools/shared/shell"; +import { + readEnvironmentFromLoginShell, + resolveLoginShell, + ShellEnvironmentReader, +} from "@t3tools/shared/shell"; @@ const platform = options.platform ?? process.platform; if (platform !== "darwin" && platform !== "linux") return; try { - const shell = env.SHELL?.trim() || "/bin/zsh"; + const shell = resolveLoginShell(platform, env.SHELL); + if (!shell) return; const shellEnvironment = (options.readEnvironment ?? readEnvironmentFromLoginShell)(shell, [ "PATH", "SSH_AUTH_SOCK", ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/syncShellEnvironment.ts` around lines 10 - 15, The fallback shell literal is macOS-specific ("/bin/zsh") so on Linux hosts without zsh the env.SHELL missing causes readEnvironmentFromLoginShell to fail; update the shell selection in syncShellEnvironment (the const shell = env.SHELL?.trim() || "/bin/zsh" line) to pick a sensible platform-specific default (e.g., use env.SHELL if set, otherwise choose "/bin/zsh" for darwin and "/bin/sh" or "/bin/bash" for linux) before calling readEnvironmentFromLoginShell; ensure the change references env.SHELL and the call site (readEnvironmentFromLoginShell or options.readEnvironment) so the fallback is conditional on the platform.apps/web/src/store.ts (1)
641-670:⚠️ Potential issue | 🟠 MajorInclude
archivedAtin the structural-sharing equality guard.Line 641 adds
archivedAt, but the reuse check (Lines 651-670) omits it. That can skip state updates when archive status changes.🔧 Suggested fix
if ( existing && existing.id === normalizedThread.id && existing.codexThreadId === normalizedThread.codexThreadId && existing.projectId === normalizedThread.projectId && existing.title === normalizedThread.title && areUnknownValuesEqual(existing.modelSelection, normalizedThread.modelSelection) && existing.runtimeMode === normalizedThread.runtimeMode && existing.interactionMode === normalizedThread.interactionMode && existing.session === normalizedThread.session && existing.messages === normalizedThread.messages && existing.proposedPlans === normalizedThread.proposedPlans && existing.error === normalizedThread.error && existing.createdAt === normalizedThread.createdAt && + existing.archivedAt === normalizedThread.archivedAt && existing.updatedAt === normalizedThread.updatedAt && existing.latestTurn === normalizedThread.latestTurn && existing.lastVisitedAt === normalizedThread.lastVisitedAt && existing.branch === normalizedThread.branch && existing.worktreePath === normalizedThread.worktreePath && existing.turnDiffSummaries === normalizedThread.turnDiffSummaries && existing.activities === normalizedThread.activities ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/store.ts` around lines 641 - 670, The structural-sharing equality guard that compares existing and normalizedThread is missing archivedAt, so state updates can be skipped when archive status changes; update the condition inside the reuse check (the block comparing existing.* to normalizedThread.*) to include a comparison for existing.archivedAt === normalizedThread.archivedAt (referencing existing, normalizedThread, and the archivedAt property) so the guard considers archive status when deciding to reuse the existing thread object.
🟡 Minor comments (14)
docs/effect-fn-checklist.md-20-25 (1)
20-25:⚠️ Potential issue | 🟡 MinorReserved keyword used as variable name in example.
Line 22 uses
newas a variable name, which is a reserved keyword in JavaScript/TypeScript and will cause a syntax error. This could confuse developers following the example.📝 Proposed fix
```ts // New -const new = Effect.fn('functionName')(function* () { +const newFn = Effect.fn('functionName')(function* () { ... })</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/effect-fn-checklist.mdaround lines 20 - 25, The example incorrectly
uses the reserved keywordnewas a variable; change the example variable name
(e.g., renamenewtonewFn) whereEffect.fn('functionName')(function* () { ... })is assigned so the sample compiles without syntax errors, and update any
subsequent references in that snippet to the new identifier.</details> </blockquote></details> <details> <summary>packages/shared/src/String.ts-1-3 (1)</summary><blockquote> `1-3`: _⚠️ Potential issue_ | _🟡 Minor_ **Guard `maxLength` against invalid values.** On Line 1, negative (or non-integer) `maxLength` produces inconsistent truncation behavior. Add an explicit runtime check before the length comparison. <details> <summary>Proposed fix</summary> ```diff export function truncate(text: string, maxLength = 50): string { + if (!Number.isInteger(maxLength) || maxLength < 0) { + throw new RangeError("maxLength must be a non-negative integer"); + } const trimmed = text.trim(); if (trimmed.length <= maxLength) { return trimmed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/String.ts` around lines 1 - 3, The truncate function should validate its maxLength parameter before use: inside function truncate (before computing trimmed.length and the comparison) add a runtime guard that ensures maxLength is a finite integer >= 0 (e.g. Number.isInteger(maxLength) && maxLength >= 0); if the check fails, throw a RangeError (or coerce/normalize by using Math.floor on valid numeric values) so negative or non-integer inputs cannot produce inconsistent truncation behavior..github/VOUCHED.td-30-32 (1)
30-32:⚠️ Potential issue | 🟡 MinorEntries are not in alphabetical order.
The file header (line 11) states "Keep entries sorted alphabetically," but the new entries violate this requirement:
github:eggfriedrice24should be placed betweengithub:cursoragent(line 15) andgithub:gbarros-dev(line 16)github:shivamhwpshould be placed betweengithub:shiroyasha9(line 28) andgithub:Yash-Singh1(line 29)📋 Proposed fix to restore alphabetical ordering
Remove the entries from their current positions and place them in the correct alphabetical positions:
github:cursoragent +github:eggfriedrice24 github:gbarros-dev github:github-actions[bot]github:shiroyasha9 +github:shivamhwp github:Yash-Singh1 -github:eggfriedrice24 github:Ymit24 -github:shivamhwp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/VOUCHED.td around lines 30 - 32, The new GitHub handle entries are out of alphabetical order; remove github:eggfriedrice24 and github:shivamhwp from their current positions and insert github:eggfriedrice24 between github:cursoragent and github:gbarros-dev, and insert github:shivamhwp between github:shiroyasha9 and github:Yash-Singh1 so the list remains sorted as required by the file header.apps/web/src/components/ProjectFavicon.tsx-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorReset load state when
cwd/srcchanges.The
statusstate is only initialized once. If this component stays mounted andcwdchanges, it can keep stale"loaded"/"error"state for the newsrc.Proposed fix
-import { useState } from "react"; +import { useEffect, useState } from "react"; @@ const [status, setStatus] = useState<"loading" | "loaded" | "error">(() => loadedProjectFaviconSrcs.has(src) ? "loaded" : "loading", ); + + useEffect(() => { + setStatus(loadedProjectFaviconSrcs.has(src) ? "loaded" : "loading"); + }, [src]);Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ProjectFavicon.tsx` at line 2, Component ProjectFavicon currently initializes the status state once and never resets it when the incoming props (cwd or src) change, causing stale "loaded"/"error" state; add a useEffect in ProjectFavicon that watches the src and cwd values and resets the related state (e.g., call setStatus to an initial value like "loading" or "" and reset any other related flags referenced around lines 28-30) whenever src or cwd changes so each new image load starts from a fresh state.apps/server/src/persistence/Migrations.ts-33-34 (1)
33-34:⚠️ Potential issue | 🟡 MinorMigration filename/ID mismatch.
Migration0018is imported from017_ProjectionThreadsArchivedAt.tsbut registered with ID 18. Similarly,Migration0019is imported from018_ProjectionThreadsArchivedAtIndex.tsand registered with ID 19. The file prefixes are off-by-one from their registration IDs.While this doesn't break functionality (the
migrationEntriesarray IDs govern execution order), the inconsistent naming creates maintenance confusion. The files should be renamed:
017_ProjectionThreadsArchivedAt.ts→018_ProjectionThreadsArchivedAt.ts018_ProjectionThreadsArchivedAtIndex.ts→019_ProjectionThreadsArchivedAtIndex.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/persistence/Migrations.ts` around lines 33 - 34, The import filenames for Migration0018 and Migration0019 are off-by-one vs their registered IDs; rename the source files and update imports so names match the migration IDs: rename 017_ProjectionThreadsArchivedAt.ts → 018_ProjectionThreadsArchivedAt.ts and 018_ProjectionThreadsArchivedAtIndex.ts → 019_ProjectionThreadsArchivedAtIndex.ts, then update the import statements for Migration0018 and Migration0019 in Migrations.ts (and any other references) to the new filenames so the module names, file prefixes and the IDs used in the migrationEntries registration remain consistent.apps/server/src/provider/Layers/ProviderRegistry.ts-168-231 (1)
168-231:⚠️ Potential issue | 🟡 MinorThis publish guard won't debounce no-op refreshes.
Each snapshot builder stamps a fresh
checkedAt, sohaveProvidersChanged(previousProviders, mergedProviders)at Line 428 will keep seeing a change even when the substantive provider state is identical. The 60-second sync loop will therefore continue publishingserver.providersUpdatedon every pass.Also applies to: 426-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderRegistry.ts` around lines 168 - 231, The snapshot builders (buildDisabledSnapshot, buildWarningSnapshot, buildReadySnapshot) always stamp a fresh checkedAt, causing haveProvidersChanged(...) to see differences even when nothing else changed; change the builders to accept an optional checkedAt parameter (or stop setting checkedAt inside them) and move timestamp assignment to the caller that detects real changes (the code path that calls haveProvidersChanged and publishes server.providersUpdated) so checkedAt is only updated when substantive provider state actually changes; update all call sites of buildDisabledSnapshot/buildWarningSnapshot/buildReadySnapshot to pass through the preserved checkedAt or only set a new timestamp when haveProvidersChanged indicates a change.apps/server/src/codexAppServerManager.test.ts-239-272 (1)
239-272:⚠️ Potential issue | 🟡 MinorThis bypasses the stderr path you actually want to lock down.
Calling
emitNotificationEventdirectly only proves that helper serializes a notification. A regression in raw stderr classification or in the code that dispatches classified lines would still pass here. Please drive this assertion through the manager entry point that consumes stderr so the test covers the behavior changed in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/codexAppServerManager.test.ts` around lines 239 - 272, The test currently bypasses the stderr classification logic by calling emitNotificationEvent directly; instead, invoke the CodexAppServerManager entry point that consumes raw stderr lines (the production-facing method on CodexAppServerManager that receives/processes stderr lines) so the classification and dispatch path is exercised, keep the spy on emitEvent, feed a raw stderr string like "fatal: permission denied" into that stderr consumer, and assert emitEvent was called with the same expectation; replace the direct call to emitNotificationEvent with a call to the manager's stderr-consumption method while leaving the emitEvent spy and the expect(...) assertion unchanged.apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts-113-126 (1)
113-126:⚠️ Potential issue | 🟡 MinorNormalize legacy
turn.completedevents into the canonical shape only.On Line 116, spreading
eventkeeps the legacy top-levelstatusanderrorMessagefields on the normalized object. That makes this harness compatible with code that still reads the deprecated shape, so these tests can miss regressions in the canonicalpayload.statepath.🧹 Suggested cleanup
const normalizeLegacyEvent = (event: LegacyProviderRuntimeEvent): ProviderRuntimeEvent => { if (isLegacyTurnCompletedEvent(event)) { + const { status, errorMessage, ...rest } = event; const normalized: Extract<ProviderRuntimeEvent, { type: "turn.completed" }> = { - ...(event as Omit<Extract<ProviderRuntimeEvent, { type: "turn.completed" }>, "payload">), + ...(rest as Omit<Extract<ProviderRuntimeEvent, { type: "turn.completed" }>, "payload">), payload: { - state: event.status, - ...(typeof event.errorMessage === "string" ? { errorMessage: event.errorMessage } : {}), + state: status, + ...(typeof errorMessage === "string" ? { errorMessage } : {}), }, }; return normalized; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts` around lines 113 - 126, The normalizeLegacyEvent helper currently spreads the legacy event into the normalized object which leaves deprecated top-level fields (status, errorMessage) on the result; update the normalizeLegacyEvent implementation (the branch guarded by isLegacyTurnCompletedEvent) to construct the Extract<ProviderRuntimeEvent, { type: "turn.completed" }> explicitly without spreading the original event so the returned object contains only the canonical fields (type and payload), set payload.state = event.status and conditionally include payload.errorMessage when present, and ensure no legacy top-level status or errorMessage remain on the returned object.apps/server/src/git/Layers/SessionTextGeneration.ts-15-16 (1)
15-16:⚠️ Potential issue | 🟡 MinorRemove unused import
ThreadTitleGenerationInput.Static analysis correctly flagged that
ThreadTitleGenerationInputis imported but never used. The type is inferred fromSessionTextGenerationShape["generateThreadTitle"].-import type { - ThreadTitleGenerationInput, - ThreadTitleGenerationResult, -} from "../Services/TextGeneration.ts"; +import type { ThreadTitleGenerationResult } from "../Services/TextGeneration.ts";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/SessionTextGeneration.ts` around lines 15 - 16, Remove the unused type import ThreadTitleGenerationInput from the import list at the top of SessionTextGeneration.ts; keep ThreadTitleGenerationResult if used and rely on SessionTextGenerationShape["generateThreadTitle"] for the input type inference, ensuring no other references to ThreadTitleGenerationInput remain (search for ThreadTitleGenerationInput and delete its import).apps/server/src/git/Layers/GitCore.test.ts-449-459 (1)
449-459:⚠️ Potential issue | 🟡 MinorAvoid
Effect.runPromiseinside an Effect.The pipeline flagged this pattern. Using
Effect.runPromiseinsideEffect.genbreaks Effect's structured concurrency guarantees. Consider usingEffect.tryPromiseor restructuring the test to keep vitest assertions outside the Effect pipeline.Possible refactor
- yield* Effect.promise(() => - vi.waitFor( - async () => { - const details = await Effect.runPromise(core.statusDetails(source)); - expect(details.branch).toBe(featureBranch); - expect(details.aheadCount).toBe(0); - expect(details.behindCount).toBe(1); - }, - { timeout: 20_000 }, - ), - ); + yield* Effect.retry( + Effect.gen(function* () { + const details = yield* core.statusDetails(source); + expect(details.branch).toBe(featureBranch); + expect(details.aheadCount).toBe(0); + expect(details.behindCount).toBe(1); + }), + { times: 40, schedule: Schedule.spaced(Duration.millis(500)) }, + );scripts/mock-update-server.ts-13-16 (1)
13-16:⚠️ Potential issue | 🟡 MinorRoot containment check incorrectly rejects dot-prefixed paths.
The current logic
!relative(...).startsWith(".")blocks legitimate in-root paths like.well-knownbecause any relative path starting with a dot is rejected. Instead, check explicitly for parent traversal (..) and absolute escapes.🔧 Proposed fix
-import { resolve, relative } from "node:path"; +import { resolve, relative, isAbsolute } from "node:path"; import { realpathSync } from "node:fs"; @@ function isWithinRoot(filePath: string): boolean { try { - return !relative(realpathSync(root), realpathSync(filePath)).startsWith("."); + const rootRealPath = realpathSync(root); + const targetRealPath = realpathSync(filePath); + const rel = relative(rootRealPath, targetRealPath); + return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); } catch (error) { mockServerLog("error", `Error checking if file is within root: ${error}`); return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/mock-update-server.ts` around lines 13 - 16, The isWithinRoot function currently rejects any relative path that starts with "." which incorrectly blocks valid in-root dot-prefixed names (e.g., ".well-known"); change the logic to compute rel = relative(realpathSync(root), realpathSync(filePath)) and return true when rel does not escape to a parent (i.e., the first path segment is not "..") and rel is not an absolute path — e.g., return !path.isAbsolute(rel) && !rel.split(path.sep)[0].startsWith(".."); replace the old startsWith(".") check and ensure you import/use path (path.sep, path.isAbsolute) and keep realpathSync(root) and realpathSync(filePath) as before.apps/web/src/components/Sidebar.logic.test.ts-810-838 (1)
810-838:⚠️ Potential issue | 🟡 MinorTest intent and setup are misaligned for archived-thread behavior.
At Line 837, the
.filter((thread) => thread.archivedAt === null)removes archived threads beforesortProjectsForSidebarruns, so this test doesn’t actually validate that the function ignores archived threads.🧪 Suggested test fix
[ makeThread({ id: ThreadId.makeUnsafe("thread-visible"), projectId: ProjectId.makeUnsafe("project-1"), updatedAt: "2026-03-09T10:02:00.000Z", archivedAt: null, }), makeThread({ id: ThreadId.makeUnsafe("thread-archived"), projectId: ProjectId.makeUnsafe("project-2"), updatedAt: "2026-03-09T10:10:00.000Z", archivedAt: "2026-03-09T10:11:00.000Z", }), - ].filter((thread) => thread.archivedAt === null), + ], "updated_at", );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.logic.test.ts` around lines 810 - 838, The test is accidentally removing archived threads before calling sortProjectsForSidebar, so remove the .filter((thread) => thread.archivedAt === null) and pass the full threads array (including the archived thread created by makeThread with archivedAt set) into sortProjectsForSidebar; ensure the assertion checks that sorting treats the project with only archived threads as older (i.e., the "Visible project" sorts before "Archived-only project") to validate that sortProjectsForSidebar ignores archived threads when computing order.apps/web/src/components/desktopUpdate.logic.ts-8-10 (1)
8-10:⚠️ Potential issue | 🟡 MinorKeep the tooltip consistent with the new install-first rule.
Line 8 now treats any
downloadedVersionas an installable update, but Lines 56-58 still advertise “ready to download” wheneverstatus === "available".SidebarUpdatePilluses both helpers, so the same state can render a restart CTA with a download tooltip.Minimal fix
export function getDesktopUpdateButtonTooltip(state: DesktopUpdateState): string { + if (state.status === "available" && state.downloadedVersion) { + return `Update ${state.downloadedVersion} downloaded. Click to restart and install.`; + } if (state.status === "available") { return `Update ${state.availableVersion ?? "available"} ready to download`; }Also applies to: 55-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/desktopUpdate.logic.ts` around lines 8 - 10, The tooltip logic is inconsistent with the install-first rule: update the helper used by SidebarUpdatePill so it checks state.downloadedVersion before status === "available" and returns a "ready to install" (or similar “ready to restart/install”) tooltip when downloadedVersion is present; only return the “ready to download” text when status === "available" and there is no downloadedVersion. Adjust the helper(s) that reference state.downloadedVersion and the status === "available" branch (the block around lines 55-66) so both helpers consistently treat downloadedVersion as an installable state.apps/web/src/routes/settings.tsx-16-29 (1)
16-29:⚠️ Potential issue | 🟡 MinorDon’t let Escape eject the user from focused form controls.
This listener runs for every keydown on a form-heavy page. Pressing Escape inside an input, select, or path field will call
history.back()unless that control explicitly prevents default first, which is an easy way to kick users out of settings while they’re editing. Ignore editable targets before handling the shortcut.Possible guard
const onKeyDown = (event: KeyboardEvent) => { if (event.defaultPrevented) return; + const target = event.target; + if ( + target instanceof HTMLElement && + (target.isContentEditable || ["INPUT", "TEXTAREA", "SELECT"].includes(target.tagName)) + ) { + return; + } if (event.key === "Escape") { event.preventDefault(); window.history.back(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/settings.tsx` around lines 16 - 29, The onKeyDown handler used in the useEffect currently calls window.history.back() for any Escape key press; update the onKeyDown function to early-return if the event originated from an editable control (check event.target for tagName "INPUT", "TEXTAREA", "SELECT", or node.isContentEditable, and also ignore elements with role="textbox" or when an ancestor is contentEditable) before calling event.preventDefault() and window.history.back(); keep the handler attached/detached the same.
🧹 Nitpick comments (15)
apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.test.ts (1)
310-377: Consider de-coupling assertions from array indexes.These checks are now index-fragile (this change already required index shifts). Prefer asserting by
event_idmap so fixture insertions don’t force broad churn.♻️ Example refactor
- const eventRows = yield* sql<{ - readonly payloadJson: string; - }>` - SELECT payload_json AS "payloadJson" + const eventRows = yield* sql<{ + readonly eventId: string; + readonly payloadJson: string; + }>` + SELECT + event_id AS "eventId", + payload_json AS "payloadJson" FROM orchestration_events ORDER BY rowid ASC `; + const payloadByEventId = new Map( + eventRows.map((row) => [row.eventId, JSON.parse(row.payloadJson)] as const), + ); - assert.deepStrictEqual(JSON.parse(eventRows[3]!.payloadJson), { + assert.deepStrictEqual(payloadByEventId.get("event-thread-created"), { threadId: "thread-1", // ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.test.ts` around lines 310 - 377, The assertions are brittle because they index into eventRows (e.g., eventRows[3]!.payloadJson, eventRows[4]!.payloadJson, etc.); instead build a map keyed by event_id (or whichever unique identifier exists on each row) from eventRows first, then JSON.parse the payloadJson for each expected event and assert against map[eventId].payloadJson — update tests referencing eventRows[...] to use that map and the unique event_id values (e.g., the thread/turn ids used in the expected payloads) so fixture insertions won’t break index positions.apps/web/src/components/chat/MessagesTimeline.test.tsx (1)
54-97: Consider extracting sharedMessagesTimelineprops to reduce duplication.Both tests repeat a large common prop set, which makes future updates error-prone.
♻️ Optional refactor sketch
+const baseTimelineProps = { + hasMessages: true, + isWorking: false, + activeTurnInProgress: false, + activeTurnStartedAt: null, + scrollContainer: null, + completionDividerBeforeEntryId: null, + completionSummary: null, + turnDiffSummaryByAssistantMessageId: new Map(), + nowIso: "2026-03-17T19:12:30.000Z", + expandedWorkGroups: {}, + onToggleWorkGroup: () => {}, + onOpenTurnDiff: () => {}, + revertTurnCountByUserMessageId: new Map(), + onRevertUserMessage: () => {}, + isRevertingCheckpoint: false, + onImageExpand: () => {}, + markdownCwd: undefined, + resolvedTheme: "light" as const, + timestampFormat: "locale" as const, + workspaceRoot: undefined, +}; ... -<MessagesTimeline - hasMessages - isWorking={false} - ... - workspaceRoot={undefined} -/> +<MessagesTimeline + {...baseTimelineProps} + timelineEntries={[/* test-specific entries */]} +/>Also applies to: 111-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/MessagesTimeline.test.tsx` around lines 54 - 97, Tests duplicate a large prop set for the MessagesTimeline component; extract the shared props into a single reusable constant or factory (e.g., defaultTimelineProps or makeTimelineProps) in MessagesTimeline.test.tsx and spread it into the <MessagesTimeline ... /> calls, then override only the differing props (like timelineEntries, nowIso, expandedWorkGroups) per-test; update both occurrences (around the current block and lines 111-146) to import/inline the shared object so future changes only touch one place.docs/effect-fn-checklist.md (1)
48-183: Use relative paths for better portability.All file references use absolute paths rooted at
/Users/julius/Development/Work/codething-mvp/, which are specific to one developer's environment. These links won't work for other contributors or in CI environments.♻️ Suggested approach
Convert absolute paths to repository-relative paths. For example:
-- [x] [buildUserMessageEffect](/Users/julius/Development/Work/codething-mvp/apps/server/src/provider/Layers/ClaudeAdapter.ts#L554) +- [x] [buildUserMessageEffect](../apps/server/src/provider/Layers/ClaudeAdapter.ts#L554)Or use paths relative to the repository root without the leading
/Users/...:-- [x] [buildUserMessageEffect](/Users/julius/Development/Work/codething-mvp/apps/server/src/provider/Layers/ClaudeAdapter.ts#L554) +- [x] [buildUserMessageEffect](apps/server/src/provider/Layers/ClaudeAdapter.ts#L554)This will make the checklist portable across all development environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/effect-fn-checklist.md` around lines 48 - 183, The checklist contains absolute filesystem links (e.g., /Users/julius/Development/Work/codething-mvp/...) which break portability; update each link in docs/effect-fn-checklist.md to use repository-relative paths (for example change /Users/.../apps/server/src/provider/Layers/ClaudeAdapter.ts#L554 to apps/server/src/provider/Layers/ClaudeAdapter.ts#L554) so references like buildUserMessageEffect, makeClaudeAdapter, makeGitCore, handleTraceLine, runProjectorForEvent, makeProviderService, finalizeAssistantMessage, makeCodexAdapter, captureCheckpoint, toLogMessage, etc. remain valid across environments and CI. Ensure no remaining absolute prefixes anywhere in the file.apps/web/src/components/chat/CompactComposerControlsMenu.browser.tsx (1)
235-239: Assert absence of the warning message directly instead of generic"ultrathink"text.The current negative check is case-sensitive and broad. Targeting the exact warning copy is more reliable for this scenario.
Proposed diff
await vi.waitFor(() => { const text = document.body.textContent ?? ""; expect(text).toContain("Effort"); - expect(text).not.toContain("ultrathink"); + expect(text).not.toContain( + 'Your prompt contains "ultrathink" in the text. Remove it to change effort.', + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/CompactComposerControlsMenu.browser.tsx` around lines 235 - 239, Replace the broad, case-sensitive negative assertion that checks for "ultrathink" inside the vi.waitFor block with an assertion that targets the exact warning copy used by the component (e.g., the full warning sentence shown to users) so the test explicitly verifies the absence of that warning; locate the vi.waitFor(...) block in CompactComposerControlsMenu.browser.tsx and change expect(text).not.toContain("ultrathink") to expect(text).not.toContain("<exact warning text>") (or a precise regex if punctuation/whitespace may vary) using the same vi.waitFor and expect calls.apps/web/src/truncateTitle.ts (1)
1-8: Prefer delegatingtruncateTitleto sharedtruncateutility.This logic now duplicates
packages/shared/src/String.ts; centralizing here will prevent drift and simplify maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/truncateTitle.ts` around lines 1 - 8, Replace the local truncation logic in function truncateTitle by importing and delegating to the shared truncate utility (use the shared function named truncate) instead of manually trimming/slicing; keep the truncateTitle signature and default maxLength, import truncate at top, and return truncate(text, maxLength) so the module uses the centralized implementation from the shared String utility.packages/shared/src/String.test.ts (1)
5-17: Add boundary-case tests formaxLength.Consider adding cases for
maxLength = 0and invalid values (e.g., negative/non-integer) to lock expected behavior and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/String.test.ts` around lines 5 - 17, Add unit tests in String.test.ts for boundary and invalid maxLength values: add a case calling truncate("abc", 0) and assert the expected behavior (decide and lock it in — e.g., expect(truncate("abc", 0)).toBe("") if you want zero length to return an empty string), and add tests for invalid inputs like negative and non-integer values (e.g., truncate("abc", -1) and truncate("abc", 2.5)) asserting the chosen contract (either expect a thrown error via toThrow or a coerced/truncated result). Reference the truncate function in these new tests so behavior is explicit and future changes are caught.apps/server/src/bootstrap.ts (1)
139-155: Consider logging or narrowing the catch scope.The bare
catchat line 146 silently swallows all errors before falling back toNet.Socket. While this "try everything" approach ensures maximum resilience, it could mask unexpected failure modes (e.g., resource exhaustion, permission issues unrelated to fd type).Since this is intentional fallback behavior and
Net.Socketis the final resort, this is acceptable—but adding a debug-level log or narrowing to expected error codes would aid troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/bootstrap.ts` around lines 139 - 155, The catch in makeDirectBootstrapStream currently swallows all errors from NFS.createReadStream before falling back to new Net.Socket; update it to either narrow the catch to expected errors (e.g., check err.code for ENOSYS/EBADF/EPERM) or log the caught error at debug level so unexpected failures are visible; locate the NFS.createReadStream call inside makeDirectBootstrapStream and add a debug/processLogger.debug (or rethrow for non-expected codes) before constructing the Net.Socket fallback.apps/web/src/modelSelection.ts (1)
21-69: Reduce provider map duplication to prevent driftAt Line 27-Line 68, provider keys are hardcoded repeatedly. This is easy to miss during future provider additions and can desync UI options from supported providers.
♻️ Proposed refactor
export function getCustomModelOptionsByProvider( settings: UnifiedSettings, _providers?: ReadonlyArray<ServerProvider>, selectedProvider?: ProviderKind | null, selectedModel?: string | null, ) { - return { - codex: getAppModelOptions( - "codex", - settings.providers.codex.customModels, - selectedProvider === "codex" ? selectedModel : undefined, - ), - copilot: getAppModelOptions( - "copilot", - settings.providers.copilot.customModels, - selectedProvider === "copilot" ? selectedModel : undefined, - ), - claudeAgent: getAppModelOptions( - "claudeAgent", - settings.providers.claudeAgent.customModels, - selectedProvider === "claudeAgent" ? selectedModel : undefined, - ), - cursor: getAppModelOptions( - "cursor", - settings.providers.cursor.customModels, - selectedProvider === "cursor" ? selectedModel : undefined, - ), - opencode: getAppModelOptions( - "opencode", - settings.providers.opencode.customModels, - selectedProvider === "opencode" ? selectedModel : undefined, - ), - geminiCli: getAppModelOptions( - "geminiCli", - settings.providers.geminiCli.customModels, - selectedProvider === "geminiCli" ? selectedModel : undefined, - ), - amp: getAppModelOptions( - "amp", - settings.providers.amp.customModels, - selectedProvider === "amp" ? selectedModel : undefined, - ), - kilo: getAppModelOptions( - "kilo", - settings.providers.kilo.customModels, - selectedProvider === "kilo" ? selectedModel : undefined, - ), - } as const; + const providerOrder = [ + "codex", + "copilot", + "claudeAgent", + "cursor", + "opencode", + "geminiCli", + "amp", + "kilo", + ] as const; + + return Object.fromEntries( + providerOrder.map((provider) => [ + provider, + getAppModelOptions( + provider, + settings.providers[provider].customModels, + selectedProvider === provider ? selectedModel : undefined, + ), + ]), + ) as { + readonly [P in (typeof providerOrder)[number]]: ReturnType<typeof getAppModelOptions>; + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modelSelection.ts` around lines 21 - 69, The provider map in getCustomModelOptionsByProvider is duplicated and brittle; change it to iterate over settings.providers (or a single source-of-truth provider list) and dynamically build the returned object by calling getAppModelOptions(providerKey, settings.providers[providerKey].customModels, selectedProvider === providerKey ? selectedModel : undefined); keep the same function signature (getCustomModelOptionsByProvider) and types (UnifiedSettings, ServerProvider, ProviderKind), ensure the resulting object is typed/const-cast similarly to the original return, and preserve behavior for determining selectedModel per provider.apps/server/src/orchestration/decider.ts (1)
193-236: Consider using the archived-state invariants to enforce valid transitions.The
requireThreadArchivedandrequireThreadNotArchivedhelpers were added incommandInvariants.tsbut are not used here. Currently:
thread.archiveallows re-archiving an already archived threadthread.unarchiveallows unarchiving a thread that isn't archivedIf idempotency is intended, this is fine. Otherwise, consider enforcing valid state transitions:
♻️ Suggested enforcement of archive-state invariants
+import { + requireProject, + requireProjectAbsent, + requireThread, + requireThreadAbsent, + requireThreadNotArchived, + requireThreadArchived, +} from "./commandInvariants.ts"; case "thread.archive": { - yield* requireThread({ + yield* requireThreadNotArchived({ readModel, command, threadId: command.threadId, }); // ... } case "thread.unarchive": { - yield* requireThread({ + yield* requireThreadArchived({ readModel, command, threadId: command.threadId, }); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/decider.ts` around lines 193 - 236, The archive/unarchive handlers allow invalid state transitions because they call requireThread but not the new invariants; update the "thread.archive" case to call requireThreadArchived? no — call requireThreadNotArchived({ readModel, command, threadId: command.threadId }) before creating the thread.archived event, and update the "thread.unarchive" case to call requireThreadArchived({ readModel, command, threadId: command.threadId }) before creating the thread.unarchived event (keep the existing requireThread call if still needed), so the handlers use the requireThreadNotArchived and requireThreadArchived helpers to enforce valid transitions.apps/server/src/provider/Layers/ProviderRegistry.test.ts (1)
451-487: Assert a few snapshot fields here, not just provider order.This only proves enumeration. It will still pass if the newly added probes regress on
status,installed, orauthStatus, which is the main behavior this PR is changing. A couple of representative snapshot assertions would make this test much harder to false-pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderRegistry.test.ts` around lines 451 - 487, The test currently only asserts provider order; add assertions that verify key snapshot fields (e.g., status, installed, authStatus) for a few representative providers to catch regressions in probe behavior: after obtaining providers via ProviderRegistry/registry.getProviders (from providerRegistryLayer/ProviderRegistryLive), assert that the provider objects for "codex", "claudeAgent", and one CLI-based provider like "geminiCli" have the expected installed (boolean), status (e.g., "available" or "unavailable"), and authStatus (e.g., "Logged in"/"Authenticated" or similar) values based on the mockCommandSpawnerLayer responses; keep these as explicit equality checks alongside the existing provider order assertion.apps/server/src/os-jank.test.ts (1)
6-38: Add one test for Linux with missingSHELL.Current coverage validates Linux with explicit
SHELLand Windows no-op, but it skips the Linux default-shell path. Adding that case would lock in the intended fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/os-jank.test.ts` around lines 6 - 38, Add a new unit test that verifies fixPath falls back to the Linux default shell when SHELL is absent: create an env without SHELL (only PATH), call fixPath with platform: "linux" and a stubbed readPath that returns a new PATH string, then assert readPath was called with the Linux default shell (e.g., "/bin/bash") and that env.PATH was updated to the returned value; reference the fixPath function, the readPath mock, and env object to locate where to add this test.apps/server/src/git/Layers/CodexTextGeneration.test.ts (1)
361-379: Avoid asserting the exact truncation boundary here.This makes the integration test depend on the current character budget of the shared title sanitizer rather than the wiring in
CodexTextGeneration. A harmless tweak to the normalization heuristic will break this test even if the feature still works. I'd assert the important invariants here instead: single-line output, quotes removed, non-empty result, and within the sidebar limit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/CodexTextGeneration.test.ts` around lines 361 - 379, The test currently asserts an exact truncation string which couples it to the sanitizer's character budget; instead, update the test around TextGeneration.generateThreadTitle to assert invariants: that generated.title is a single-line string (no newline characters), has surrounding quotes removed (no leading/trailing quotes), is non-empty, and its length is <= the sidebar title max length used by your UI (reference the same constant/limit used by the sanitizer or UI) rather than an exact truncated text; keep the existing sample input and only change the expect assertions in CodexTextGeneration.test.ts.apps/server/integration/OrchestrationEngineHarness.integration.ts (1)
308-311: Make thisTextGenerationfake type-complete.The harness stub implements only
generateBranchNameandgenerateThreadTitle, butTextGenerationShaperequires four methods:generateCommitMessage,generatePrContent,generateBranchName, andgenerateThreadTitle. Theas unknown as TextGenerationShapecast suppresses this gap. If an integration path calls a missing method, it will fail at runtime instead of at type-check time. Implement all four methods or remove the unsafe cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/integration/OrchestrationEngineHarness.integration.ts` around lines 308 - 311, The TextGeneration fake created via Layer.succeed (assigned to textGenerationLayer) is incomplete and uses an unsafe cast to TextGenerationShape; implement all required methods (generateCommitMessage and generatePrContent in addition to generateBranchName and generateThreadTitle) on the fake object so it fully matches TextGenerationShape, return appropriate Effect.succeed(...) values for each method (mirroring the existing style) and remove the "as unknown as TextGenerationShape" cast so TypeScript can verify completeness.apps/server/src/git/Layers/SessionTextGeneration.ts (1)
461-466: Note: Different field access pattern than other methods.
generateThreadTitleaccessesinput.modelSelection.providerandinput.modelSelection.model, while the other methods (generateCommitMessage,generatePrContent,generateBranchName) accessinput.providerandinput.modeldirectly. This is correct based on the type definitions but creates an inconsistency in the API surface. Consider whether future refactoring should consolidate these patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/SessionTextGeneration.ts` around lines 461 - 466, The generateThreadTitle call is using input.modelSelection.provider and input.modelSelection.model which is inconsistent with other methods that use input.provider and input.model; update the generateThreadTitle implementation (where runProviderJson is invoked) to use input.provider and input.model to match generateCommitMessage/generatePrContent/generateBranchName, or alternatively adjust the other methods and types to uniformly use modelSelection — ensure the function name generateThreadTitle and the runProviderJson invocation are updated to reference the chosen fields and update any affected type definitions or callers to keep the API surface consistent.apps/web/src/types.ts (1)
104-104: Consider makingarchivedAtnon-optional if normalization guarantees presence.If all thread objects are normalized to include
archivedAt(string | null), changing this to a required field reducesundefined/nulldual-state checks in UI code.Suggested type tweak
-export interface Thread { +export interface Thread { id: ThreadId; codexThreadId: string | null; projectId: ProjectId; title: string; modelSelection: ModelSelection; runtimeMode: RuntimeMode; interactionMode: ProviderInteractionMode; session: ThreadSession | null; messages: ChatMessage[]; proposedPlans: ProposedPlan[]; error: string | null; createdAt: string; - archivedAt?: string | null; + archivedAt: string | null; updatedAt?: string | undefined; latestTurn: OrchestrationLatestTurn | null; lastVisitedAt?: string | undefined; branch: string | null; worktreePath: string | null; turnDiffSummaries: TurnDiffSummary[]; activities: OrchestrationThreadActivity[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/types.ts` at line 104, The Thread type's archivedAt property is declared optional (archivedAt?: string | null) but normalization guarantees it exists; make archivedAt required by removing the optional marker (change to archivedAt: string | null) in the interface/ type definition in types.ts (e.g., the Thread or relevant type that currently declares archivedAt) and run a quick search for places that assumed undefined to update any checks to handle string | null instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e2c0170-2382-48f7-bbef-d46823eebf19
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (121)
.gitattributes.github/VOUCHED.td.github/workflows/pr-size.yml.gitignoreapps/desktop/package.jsonapps/desktop/src/preload.tsapps/desktop/src/syncShellEnvironment.test.tsapps/desktop/src/syncShellEnvironment.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/package.jsonapps/server/src/bootstrap.test.tsapps/server/src/bootstrap.tsapps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.tsapps/server/src/checkpointing/Layers/CheckpointStore.test.tsapps/server/src/codexAppServerManager.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/git/Layers/ClaudeTextGeneration.test.tsapps/server/src/git/Layers/ClaudeTextGeneration.tsapps/server/src/git/Layers/CodexTextGeneration.test.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/RoutingTextGeneration.tsapps/server/src/git/Layers/SessionTextGeneration.tsapps/server/src/git/Prompts.test.tsapps/server/src/git/Prompts.tsapps/server/src/git/Services/GitCore.tsapps/server/src/git/Services/TextGeneration.tsapps/server/src/git/Utils.tsapps/server/src/keybindings.test.tsapps/server/src/keybindings.tsapps/server/src/main.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.tsapps/server/src/orchestration/Layers/OrchestrationEngine.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/orchestration/Schemas.tsapps/server/src/orchestration/commandInvariants.test.tsapps/server/src/orchestration/commandInvariants.tsapps/server/src/orchestration/decider.tsapps/server/src/orchestration/projector.test.tsapps/server/src/orchestration/projector.tsapps/server/src/os-jank.test.tsapps/server/src/os-jank.tsapps/server/src/persistence/Layers/ProjectionRepositories.test.tsapps/server/src/persistence/Layers/ProjectionThreads.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/016_CanonicalizeModelSelections.test.tsapps/server/src/persistence/Migrations/017_ProjectionThreadsArchivedAt.tsapps/server/src/persistence/Migrations/018_ProjectionThreadsArchivedAtIndex.tsapps/server/src/persistence/Services/ProjectionThreads.tsapps/server/src/processRunner.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/wsServer/pushBus.tsapps/server/vitest.config.tsapps/web/package.jsonapps/web/src/appSettings.tsapps/web/src/components/AppSidebarLayout.tsxapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/ChatView.logic.tsapps/web/src/components/ProjectFavicon.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/chat/CompactComposerControlsMenu.browser.tsxapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/desktopUpdate.logic.test.tsapps/web/src/components/desktopUpdate.logic.tsapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/components/settings/SettingsSidebarNav.tsxapps/web/src/components/sidebar/SidebarUpdatePill.tsxapps/web/src/contextMenuFallback.tsapps/web/src/hooks/useLocalStorage.tsapps/web/src/hooks/useSettings.test.tsapps/web/src/hooks/useSettings.tsapps/web/src/hooks/useThreadActions.tsapps/web/src/keybindings.tsapps/web/src/lib/desktopUpdateReactQuery.test.tsapps/web/src/lib/desktopUpdateReactQuery.tsapps/web/src/lib/storage.tsapps/web/src/modelSelection.tsapps/web/src/routeTree.gen.tsapps/web/src/routes/_chat.settings.tsxapps/web/src/routes/settings.archived.tsxapps/web/src/routes/settings.general.tsxapps/web/src/routes/settings.tsxapps/web/src/store.tsapps/web/src/terminal-links.test.tsapps/web/src/terminal-links.tsapps/web/src/terminalStateStore.tsapps/web/src/timestampFormat.tsapps/web/src/truncateTitle.test.tsapps/web/src/truncateTitle.tsapps/web/src/types.tsapps/web/src/worktreeCleanup.test.tsdocs/effect-fn-checklist.mdpackage.jsonpackages/contracts/package.jsonpackages/contracts/src/ipc.tspackages/contracts/src/keybindings.test.tspackages/contracts/src/keybindings.tspackages/contracts/src/orchestration.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.tspackages/shared/src/DrainableWorker.tspackages/shared/src/String.test.tspackages/shared/src/String.tspackages/shared/src/shell.tsscripts/mock-update-server.ts
💤 Files with no reviewable changes (2)
- apps/web/src/truncateTitle.test.ts
- apps/web/src/routes/_chat.settings.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/server/src/provider/Layers/ProviderRegistry.ts (1)
260-272:⚠️ Potential issue | 🟠 MajorDon't turn a missing CLI into an empty discovery result here.
The
isCommandMissingCause(...) => []fallback only works when a later version probe is guaranteed.cursor,opencode, andkilodo not provideresolveProbeBinaryPath, so a missing command on PATH skipsrunVersionCommand()entirely and falls through tobuildReadySnapshot()withinstalled: true.💡 Minimal fix
}).pipe( Effect.catchCause((cause) => { const error = unwrapProbeError(Cause.squash(cause)); - if (isCommandMissingCause(error)) { + if (isCommandMissingCause(error) && binaryPath) { return Effect.succeed<ReadonlyArray<{ slug: string; name: string }>>([]); } return Effect.failCause(cause); }), )Also applies to: 366-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderRegistry.ts` around lines 260 - 272, The current catch in the discoveredModels flow treats a missing CLI (isCommandMissingCause) as an empty discovery result which hides "command missing" errors and leads to incorrect installed: true behavior downstream; instead, remove the special-case that returns [] for isCommandMissingCause and let the cause propagate (i.e., fail the effect) so runVersionCommand()/resolveProbeBinaryPath can handle missing binaries correctly; apply the same change to the other identical block around runVersionCommand (the similar code at lines 366-380) and reference discoveredModels, options.fetchDiscoveredModels, wrapProbeError, unwrapProbeError, and isCommandMissingCause when making the change.
🧹 Nitpick comments (2)
apps/web/src/components/settings/SettingsPanels.tsx (2)
711-716: MutationObserver cleanup may fire after component unmount.The observer callback calls
scrollToEnd()which references theelelement. If the component unmounts before the 2-second timeout, the callback may execute on a detached element. While this likely fails silently, disconnecting the observer on component unmount would be cleaner.Consider moving this scroll logic to a
useEffectwith proper cleanup if this pattern is reused, or accept the current behavior as a minor nit sincescrollToon a detached element is a no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/settings/SettingsPanels.tsx` around lines 711 - 716, The MutationObserver created in SettingsPanels.tsx can fire after unmount and call scrollToEnd() on a detached el; wrap this observer logic in a useEffect tied to the component lifecycle (or the element ref) so you create the MutationObserver inside useEffect, call observer.observe(el, { childList: true, subtree: true }) there, and ensure you call observer.disconnect() in the effect cleanup to prevent callbacks after unmount; reference the observer, scrollToEnd, and the el/ref when moving the logic so disconnect is always executed.
406-413: Prefer async native dialog over synchronouswindow.confirm.Line 408 uses the synchronous
window.confirm()for update installation confirmation, whilerestoreDefaults(line 544) uses the async native dialog API. Consider using the native API consistently for a better user experience on desktop.♻️ Suggested refactor to use native dialog
- const confirmed = window.confirm( - getDesktopUpdateInstallConfirmationMessage( - updateState ?? { availableVersion: null, downloadedVersion: null }, - ), - ); + const api = readNativeApi(); + const message = getDesktopUpdateInstallConfirmationMessage( + updateState ?? { availableVersion: null, downloadedVersion: null }, + ); + const confirmed = await (api ?? { dialogs: { confirm: (msg: string) => Promise.resolve(window.confirm(msg)) } }).dialogs.confirm(message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/settings/SettingsPanels.tsx` around lines 406 - 413, The synchronous window.confirm call inside the "install" branch (where action === "install") should be replaced with the app's async native dialog usage (the same pattern used in restoreDefaults) to avoid blocking the renderer; call the existing async dialog helper instead of window.confirm with the message produced by getDesktopUpdateInstallConfirmationMessage(updateState ?? { availableVersion: null, downloadedVersion: null }), await its boolean result, and return early if the user cancels; ensure the containing handler is marked async and update any callsites so promise handling matches the pattern used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 260-272: The current catch in the discoveredModels flow treats a
missing CLI (isCommandMissingCause) as an empty discovery result which hides
"command missing" errors and leads to incorrect installed: true behavior
downstream; instead, remove the special-case that returns [] for
isCommandMissingCause and let the cause propagate (i.e., fail the effect) so
runVersionCommand()/resolveProbeBinaryPath can handle missing binaries
correctly; apply the same change to the other identical block around
runVersionCommand (the similar code at lines 366-380) and reference
discoveredModels, options.fetchDiscoveredModels, wrapProbeError,
unwrapProbeError, and isCommandMissingCause when making the change.
---
Nitpick comments:
In `@apps/web/src/components/settings/SettingsPanels.tsx`:
- Around line 711-716: The MutationObserver created in SettingsPanels.tsx can
fire after unmount and call scrollToEnd() on a detached el; wrap this observer
logic in a useEffect tied to the component lifecycle (or the element ref) so you
create the MutationObserver inside useEffect, call observer.observe(el, {
childList: true, subtree: true }) there, and ensure you call
observer.disconnect() in the effect cleanup to prevent callbacks after unmount;
reference the observer, scrollToEnd, and the el/ref when moving the logic so
disconnect is always executed.
- Around line 406-413: The synchronous window.confirm call inside the "install"
branch (where action === "install") should be replaced with the app's async
native dialog usage (the same pattern used in restoreDefaults) to avoid blocking
the renderer; call the existing async dialog helper instead of window.confirm
with the message produced by
getDesktopUpdateInstallConfirmationMessage(updateState ?? { availableVersion:
null, downloadedVersion: null }), await its boolean result, and return early if
the user cancels; ensure the containing handler is marked async and update any
callsites so promise handling matches the pattern used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8cf77ed-d312-480b-a418-a8b534d85c60
📒 Files selected for processing (16)
.github/workflows/pr-size.ymlapps/server/src/orchestration/projector.tsapps/server/src/persistence/NodeSqliteClient.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/serverSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/ProjectFavicon.tsxapps/web/src/components/settings/SettingsPanels.tsxapps/web/src/components/sidebar/SidebarUpdatePill.tsxapps/web/src/hooks/useSettings.test.tsapps/web/src/hooks/useSettings.tsapps/web/src/hooks/useThreadActions.tspackages/contracts/src/settings.ts
✅ Files skipped from review due to trivial changes (3)
- apps/web/src/components/KeybindingsToast.browser.tsx
- apps/web/src/hooks/useSettings.test.ts
- apps/server/src/serverSettings.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/hooks/useSettings.ts
- apps/web/src/components/ProjectFavicon.tsx
- apps/web/src/components/sidebar/SidebarUpdatePill.tsx
- apps/server/src/provider/Layers/ProviderRegistry.test.ts
- .github/workflows/pr-size.yml
- apps/web/src/hooks/useThreadActions.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/provider/Layers/ProviderRegistry.ts (1)
398-401:⚠️ Potential issue | 🟠 MajorUse a stable projection for provider comparison instead of raw objects.
buildServerProviderreturns plain object literals, which don't have Effect's value-based equality semantics. Since every sync rebuilds the provider array with a freshcheckedAttimestamp,Equal.equalswill always detect a difference even when provider state is actually unchanged. This causes unnecessary change publications on every periodic sync cycle.Compare a stable projection of the provider state (excluding the timestamp) or implement
Equalsemantics forServerProviderinstead of comparing the raw objects directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderRegistry.ts` around lines 398 - 401, haveProvidersChanged currently compares raw ServerProvider objects with Equal.equals, which always differs because buildServerProvider injects a fresh checkedAt timestamp; change it to compare a stable projection (e.g., map previousProviders and nextProviders to a shape excluding checkedAt and other volatile fields) or implement value-based equality for ServerProvider, then run Equal.equals on that projection. Locate haveProvidersChanged and update the comparison to project each ServerProvider (created by buildServerProvider) to a stable key/state (for example {id, name, config, status, ...} without checkedAt) before calling Equal.equals, ensuring unchanged providers don't trigger diffs.
🤖 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/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 422-437: The merge logic in applyManagedProviderSnapshot (and the
similar block at 443-456) currently does Ref.get → compute mergedProviders →
Ref.set which is racy; replace both with a single atomic
Ref.modify/Ref.updateAndGet on providersRef so the merge is based on the latest
cache state. Inside the atomic update compute previousProviders from the
supplied current value, build mergedProviders using ALL_PROVIDERS (same mapping
logic that checks snapshot.provider vs candidate), set the ref to
mergedProviders and return both previousProviders and mergedProviders so you can
check haveProvidersChanged(previousProviders, mergedProviders) and then
conditionally PubSub.publish(changesPubSub, mergedProviders). Ensure you use the
same symbols: applyManagedProviderSnapshot, providersRef, snapshot,
ALL_PROVIDERS, haveProvidersChanged, and changesPubSub.
---
Outside diff comments:
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 398-401: haveProvidersChanged currently compares raw
ServerProvider objects with Equal.equals, which always differs because
buildServerProvider injects a fresh checkedAt timestamp; change it to compare a
stable projection (e.g., map previousProviders and nextProviders to a shape
excluding checkedAt and other volatile fields) or implement value-based equality
for ServerProvider, then run Equal.equals on that projection. Locate
haveProvidersChanged and update the comparison to project each ServerProvider
(created by buildServerProvider) to a stable key/state (for example {id, name,
config, status, ...} without checkedAt) before calling Equal.equals, ensuring
unchanged providers don't trigger diffs.
🪄 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: 4798fc0b-63af-4473-9b7f-8715a60b87d6
📒 Files selected for processing (3)
apps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/web/src/components/settings/SettingsPanels.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/server/src/provider/Layers/ProviderRegistry.test.ts
- apps/web/src/components/settings/SettingsPanels.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/ProviderRegistry.test.ts`:
- Around line 633-637: Move the baseline capture to after the first invocation
of registry.getProviders so you compare the probe count between the first and
second calls: call yield* registry.getProviders once, set initialProbeCount =
probeCount, call yield* registry.getProviders again, then
assert.strictEqual(probeCount, initialProbeCount). This change touches the
probeCount variable and the registry.getProviders calls in the test.
🪄 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: 35a16a89-0255-48b2-a3f4-f97a2d244c0e
📒 Files selected for processing (4)
apps/server/src/orchestration/projector.tsapps/server/src/persistence/NodeSqliteClient.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/persistence/NodeSqliteClient.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/server/src/orchestration/projector.ts
- apps/server/src/provider/Layers/ProviderRegistry.ts
The published beta.42 changed SqlError to require a `reason: SqlErrorReason`
field instead of `{ cause, message }`. Use `classifySqliteError` to produce
the correct reason type from native SQLite errors.
Update assertion to match the current UI copy.
This was referenced Mar 30, 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
20 participants
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
upstream/maininto the fork integration branch while preserving the fork's multi-provider architectureValidation
bun fmtbun lintbun typecheckbun run testbun x vitest run apps/server/src/provider/Layers/ProviderRegistry.test.ts --config apps/server/vitest.config.tsSummary by CodeRabbit
New Features
Improvements
Chores