Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Claude CLI executable resolution and runtime probing, embedding cache probing, a memory repair service and episodic memory format, passive proof-observer ingestion, procedural learning updates, many AI/provider/runtime status changes, and broad renderer UI updates renaming External MCP to “ADE-managed MCP”. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Owner
Author
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
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 (1)
apps/desktop/src/main/services/missions/missionPreflightService.test.ts (1)
517-576:⚠️ Potential issue | 🟡 MinorThis case still doesn't exercise the unavailable-status branch.
getBackendStatus()returning{ backends: [], ... }covers the "status known, no backends" path. The new production branch is thebackendStatus === nullcase, so a regression there would still go untested. Please add a companion case withcomputerUseArtifactBrokerServiceomitted orgetBackendStatus: () => null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/missions/missionPreflightService.test.ts` around lines 517 - 576, The test "blocks launch when proof is required but no backends are available and fallback is off" only covers the "status known, backends empty" path because computerUseArtifactBrokerService.getBackendStatus returns an object; add a companion test that exercises the production branch where backendStatus === null by either omitting computerUseArtifactBrokerService from the service factory or stubbing getBackendStatus to return null, ensuring the missionPreflightService logic that checks for a null backend status is executed and assertions mirror the original case's expectations.
🟡 Minor comments (10)
apps/desktop/src/renderer/components/prs/shared/prFormatters.ts-40-46 (1)
40-46:⚠️ Potential issue | 🟡 MinorUse
toLocaleStringinstead oftoLocaleDateStringfor date+time formatting.Line 40 uses
toLocaleDateStringwithhour/minuteoptions, which is not reliable across JavaScript runtimes. ThetoLocaleDateStringAPI is designed for date-only output and doesn't consistently support time-related options. UsetoLocaleStringinstead for guaranteed date+time formatting.🔧 Proposed fix
- return d.toLocaleDateString(undefined, { + return d.toLocaleString(undefined, { month: "short", day: "numeric", year: "numeric", hour: "2-digit", minute: "2-digit", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/shared/prFormatters.ts` around lines 40 - 46, The return uses toLocaleDateString with hour/minute options which is unreliable; change the call to toLocaleString so the date+time options are honored (i.e., replace the toLocaleDateString(...) call in the return expression with toLocaleString(undefined, { month: "short", day: "numeric", year: "numeric", hour: "2-digit", minute: "2-digit" })). Ensure the rest of the formatting options and variable d remain unchanged.apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx-597-601 (1)
597-601:⚠️ Potential issue | 🟡 MinorKeep the tab badge on the same source of truth as the list.
This count sums raw
checksplus all Action jobs, but the tab itself renders a dedupedbuildUnifiedChecks(checks, actionRuns)list. As soon as a GitHub Actions job is represented in both sources, the badge is larger than the number of rows the user can actually inspect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx` around lines 597 - 601, The CI / Checks tab badge is computed from a raw sum (checks + action job counts) but the UI renders the deduplicated list produced by buildUnifiedChecks(checks, actionRuns); update the count calculation in DETAIL_TABS (the entry with id "checks") to use the deduped list length instead—call buildUnifiedChecks(checks, actionRuns) (or reuse an existing unifiedChecks variable if present) and set count: unifiedChecks.length so the badge matches the rendered rows.apps/desktop/src/renderer/components/prs/state/PrsContext.tsx-363-369 (1)
363-369:⚠️ Potential issue | 🟡 MinorGuard these background refreshes against stale responses.
This file already maintains
rebaseNeedsandautoRebaseStatusesthrough event subscriptions and a periodic 60-second scan. The fire-and-forget calls inrefresh()(lines 364–368) introduce a race condition: they can resolve out of order and overwrite fresher event-driven updates with stale data, causing brief UI regressions after refresh.Coordinate these async updates by either gating them with a sequence token or awaiting them as part of the refresh cycle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx` around lines 363 - 369, The fire-and-forget background calls in refresh() (window.ade.rebase.scanNeeds and window.ade.lanes.listAutoRebaseStatuses) can resolve out of order and overwrite newer event-driven state; modify refresh() to coordinate these async updates by introducing a sequence token (or incrementing request id) captured before each call and checked before calling setRebaseNeeds and setAutoRebaseStatuses, or by awaiting the promises as part of the refresh cycle so they complete in a controlled order; ensure you reference and update the existing state setters (setRebaseNeeds, setAutoRebaseStatuses) only when the token matches the latest refresh to avoid applying stale results.apps/desktop/src/renderer/components/settings/MemoryHealthTab.tsx-1181-1194 (1)
1181-1194:⚠️ Potential issue | 🟡 MinorBulk actions fire parallel requests without error handling.
The "Approve All" and "Archive All" buttons iterate
candidateEntriesand fire offhandlePromote/handleArchivecalls withvoid, causing all requests to run in parallel. If any fail, errors may be silently lost or race with each other. Consider sequential execution or collecting results.🛡️ Suggested improvement for sequential execution
- onClick={() => { for (const e of candidateEntries) void handlePromote(e.id); }} + onClick={async () => { + for (const e of candidateEntries) { + await handlePromote(e.id); + } + }}Or use
Promise.allSettledif parallel execution is preferred:- onClick={() => { for (const e of candidateEntries) void handlePromote(e.id); }} + onClick={() => { + void Promise.allSettled(candidateEntries.map((e) => handlePromote(e.id))); + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/MemoryHealthTab.tsx` around lines 1181 - 1194, The bulk "Approve All" and "Archive All" handlers currently iterate candidateEntries and call handlePromote/handleArchive with void, causing unhandled parallel requests and lost errors; change the button onClick handlers to await each call or to collect promises and use Promise.allSettled, ensuring you handle rejections and surface errors (e.g., update UI or show a toast) for failures; locate the buttons in MemoryHealthTab.tsx and modify the onClick logic that iterates candidateEntries to either perform sequential awaits over handlePromote(id) / handleArchive(id) or gather promises and call Promise.allSettled(promises) and process results to handle errors and disable buttons while running.apps/desktop/src/main/services/ai/providerResolver.ts-239-241 (1)
239-241:⚠️ Potential issue | 🟡 MinorTreat blank Claude executable overrides as unset.
Line 239 only falls back on
null/undefined. Ifopts.cli.claude.pathToClaudeCodeExecutableis persisted as""or whitespace, this bypasses the new resolver and hands the Claude provider an unusable path.Suggested fix
- if (provider === "claude" && settings.pathToClaudeCodeExecutable == null) { - settings.pathToClaudeCodeExecutable = resolveClaudeCodeExecutable({ auth }).path; - } + if (provider === "claude") { + const configuredPath = + typeof settings.pathToClaudeCodeExecutable === "string" + ? settings.pathToClaudeCodeExecutable.trim() + : ""; + settings.pathToClaudeCodeExecutable = + configuredPath.length > 0 + ? configuredPath + : resolveClaudeCodeExecutable({ auth }).path; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/providerResolver.ts` around lines 239 - 241, The current Claude fallback only checks for null/undefined and misses blank or whitespace values; update the clause in providerResolver where provider === "claude" to treat empty/whitespace strings as unset by checking the trimmed value of settings.pathToClaudeCodeExecutable (e.g., if it's null/undefined or trim() === ""), and if unset call resolveClaudeCodeExecutable({ auth }).path to populate settings.pathToClaudeCodeExecutable before handing the path to the Claude provider.apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts-8-21 (1)
8-21:⚠️ Potential issue | 🟡 MinorThe new MCP assertion doesn't prove the ADE launch wiring is still used.
normalizeCliMcpServersis mocked to return a populatedadeentry up front, and the assertion only checksexpect.any(Object). This still passes ifprobeClaudeRuntimeHealth()stops callingresolveAdeMcpServerLaunchentirely. Please assert the resolver call and/or the exact normalized server shape, and add a second case for the desktop socket-backed launch flow.Based on learnings, "For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path".
Also applies to: 34-44, 98-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts` around lines 8 - 21, The test currently stubs normalizeCliMcpServers to always return an `ade` server and only asserts a loose `expect.any(Object)`, which doesn't ensure probeClaudeRuntimeHealth still calls resolveAdeMcpServerLaunch or that the normalized shape is correct; update the test to (1) assert that resolveAdeMcpServerLaunch was called (or spied) with the expected inputs, (2) assert the exact normalized `ade` shape returned by normalizeCliMcpServers (match `type`, `command`/`cmdArgs`, and `env` keys) rather than `expect.any(Object)`, and (3) add a second test case covering the desktop socket-backed MCP path by mocking normalizeCliMcpServers to return the socket-backed shape and asserting probeClaudeRuntimeHealth chooses that path and/or calls the appropriate resolver; reference the existing mocks resolveClaudeCodeExecutable, normalizeCliMcpServers, resolveAdeMcpServerLaunch, and the function under test probeClaudeRuntimeHealth when making these assertions.apps/desktop/src/main/services/computerUse/controlPlane.ts-353-380 (1)
353-380:⚠️ Potential issue | 🟡 MinorPrefer a connected backend before a merely available one.
This fallback now picks the first
availablebackend. Elsewhere in this file,availableandconnectedare distinct states, so a disconnected/installed backend can mask a later connected backend and surface the wrongactiveBackendin the chat UI.🔧 Suggested adjustment
const connectedBackend = backendStatus.backends.find((backend) => backend.available && backend.state === "connected") ?? null; const availableBackend = backendStatus.backends.find((backend) => backend.available) ?? null; + const fallbackBackend = connectedBackend ?? availableBackend; @@ - } else if (availableBackend) { + } else if (fallbackBackend) { activeBackend = { - name: availableBackend.name, - style: availableBackend.style, - detail: availableBackend.detail, + name: fallbackBackend.name, + style: fallbackBackend.style, + detail: fallbackBackend.detail, source: "available", }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/computerUse/controlPlane.ts` around lines 353 - 380, The activeBackend selection currently prefers availableBackend before checking for any connected backend; update the fallback order to prefer connectedBackend over availableBackend by inserting a check for connectedBackend before the availableBackend branch (use the existing connectedBackend, availableBackend, preferredBackend, latestArtifact and activeBackend identifiers), so that if latestArtifact is absent and preferredBackend is absent but a connectedBackend exists it becomes the activeBackend, otherwise fall back to availableBackend.apps/desktop/src/renderer/components/settings/ComputerUseSection.tsx-64-72 (1)
64-72:⚠️ Potential issue | 🟡 MinorUse the computed status color for the state dot.
The dot is still keyed off
available, so rows that intentionally render as"Installed","Installed, not connected", or"Available as fallback"can still show the gray "not detected" indicator. The new ADE Local row hits this immediately whenlocalFallback.availableis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/ComputerUseSection.tsx` around lines 64 - 72, The state dot in ComputerUseSection is using the raw `available` boolean for its background, which causes rows like "Installed", "Installed, not connected", or "Available as fallback" (e.g., `localFallback.available`) to display the wrong gray indicator; update the span's background to use the component's computed `statusColor` (or the same status-to-color logic used to render the row label) instead of `available ? COLORS.success : COLORS.textDim` so the dot reflects the actual computed status color for entries such as the ADE Local row.apps/desktop/src/main/services/chat/agentChatService.test.ts-1001-1009 (1)
1001-1009:⚠️ Potential issue | 🟡 MinorAwait the permission change and assert the field that actually mutates.
This test currently passes even if
changePermissionMode()never completes, because the promise isn't awaited and the assertion only rechecksprovider, which never changes here.💡 Suggested fix
- service.changePermissionMode({ + await service.changePermissionMode({ sessionId: session.id, permissionMode: "full-auto", }); - // Verify by getting summary const summary = await service.getSessionSummary(session.id); - expect(summary).not.toBeNull(); - expect(summary!.provider).toBe("unified"); + expect(summary?.permissionMode).toBe("full-auto");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.test.ts` around lines 1001 - 1009, Await the changePermissionMode call and assert the actual mutated field instead of only rechecking provider: replace the fire-and-forget call to service.changePermissionMode(...) with await service.changePermissionMode(...), then call await service.getSessionSummary(session.id) and assert the permissionMode (or the exact field that changePermissionMode mutates on the returned summary) equals "full-auto" in addition to any other checks.apps/desktop/src/main/services/chat/agentChatService.ts-2065-2066 (1)
2065-2066:⚠️ Potential issue | 🟡 MinorKeep
sequencemonotonic when hydrating an existing session.
commitChatEventincrementssequence, but resumed sessions still start fromeventSequence: 0. After an app restart, the next envelope goes back to1, so transcript/live consumers can see duplicate sequence numbers within the same session. Seed this from persisted state or the last transcript entry before emitting new events.Also applies to: 2499-2499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 2065 - 2066, The commit points out that commitChatEvent increments managed.eventSequence but hydrated/resumed sessions reset eventSequence to 0, causing duplicate sequence numbers; update the session hydration logic in agentChatService so managed.eventSequence is seeded from persisted state or the last transcript entry before emitting new events (e.g., read the persisted eventSequence or the highest sequence from the transcript and assign it to managed.eventSequence during session resume/hydrate), ensuring commitChatEvent continues monotonic increments from that seeded value.
🧹 Nitpick comments (13)
apps/desktop/src/main/services/pty/ptyService.test.ts (1)
303-313: The new failure telemetry still isn't pinned by this suite.These tests only prove that a log happened. They never assert the added fields on
pty.spawn_retry/pty.spawn_failed, and there is no case wheremockPty.writethrows to exercisepty.startup_command_failed. Please add explicitexpect.objectContaining(...)checks plus a startup-command failure test so the new diagnostic payloads can't regress silently.Also applies to: 513-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.test.ts` around lines 303 - 313, Tests currently only assert that a log occurred and don't validate the new telemetry payload fields for pty.spawn_retry/pty.spawn_failed, nor exercise pty.startup_command_failed when startup command write fails; update the tests to assert the telemetry payloads using expect.objectContaining(...) for the added fields (refer to telemetry event names "pty.spawn_retry", "pty.spawn_failed", and "pty.startup_command_failed") and add a new case where mockPty.write is made to throw (or rejects) during service.create/startup to trigger and assert the pty.startup_command_failed payload; mirror the same expect.objectContaining assertions in the other test block mentioned (lines ~513-534) so both suites validate the diagnostic payloads.apps/desktop/src/main/services/memory/knowledgeCaptureService.test.ts (1)
247-291: Assert that the low-signal path leaves no durable side effects.This only proves that nothing visible landed in
unified_memories. IfcapturePrFeedback()still inserts amemory_capture_ledgerrow, creates an episode, or firesonEpisodeSaved, the PR feedback is still durable and future higher-signal captures for the same PR can be suppressed while this test passes. Add assertions for those side effects too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/knowledgeCaptureService.test.ts` around lines 247 - 291, The test only checks unified_memories; extend it to also assert that capturePrFeedback produced no durable side effects by verifying (1) no memory_capture_ledger entry was inserted for the prId (query the DB fixture.db or expose a helper to count rows for memory_capture_ledger for pr-2), (2) no episodes were created (assert fixture.memoryService.listEpisodes or equivalent returns zero for that project/PR), and (3) the proceduralLearningService.onEpisodeSaved mock was not called (use the mock passed into createKnowledgeCaptureService and expect(onEpisodeSaved).not.toHaveBeenCalled()). Locate capturePrFeedback, the test fixture (createFixture), and the proceduralLearningService/onEpisodeSaved mock to add these assertions.apps/desktop/src/main/services/ai/tools/systemPrompt.ts (1)
91-104: Don't outlaw all fix recipes here.This new bullet is broader than the “not derivable from the code, git history, or project files” rule above. Non-obvious vendor/environment workarounds are exactly the kind of durable gotchas memory should keep, but this wording tells the agent to drop them. Narrow it to fixes already obvious from the final code/diff, and mirror the same wording in
apps/desktop/src/main/services/ai/tools/memoryTools.tsso both prompts stay aligned.✏️ Suggested wording
- "- Debugging solutions or fix recipes (the fix is in the code; the commit message has the context)", + "- Fixes already obvious from the final code/diff. Do save non-obvious root causes or external workarounds that are not recoverable from the repo.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/systemPrompt.ts` around lines 91 - 104, Update the "BAD memories" bullet that currently reads "Debugging solutions or fix recipes (the fix is in the code; the commit message has the context)" to a narrower rule that only excludes fix recipes which are derivable from the final code/diff or commit context (e.g., "Do not save debugging solutions or fix recipes that are directly derivable from the final code/diff or commit message"), and ensure the wording is mirrored exactly in the corresponding prompt definition in apps/desktop/src/main/services/ai/tools/memoryTools.ts so both prompts remain aligned; locate and edit the prompt constant(s) that contain the bullets (e.g., the system prompt string/array in systemPrompt.ts and the similar prompt variable in memoryTools.ts) and replace the single broad exclusion with the new narrowed phrasing.apps/desktop/src/main/services/memory/embeddingService.ts (1)
300-321: Consider using asyncreaddirfor non-blocking startup.The
probeCachemethod uses synchronousfs.readdirSyncwhich could briefly block the main process during startup. While the early-return guards (state === "ready" || state === "loading") limit when this runs, consider usingfs.promises.readdirfor consistency with the async pattern.♻️ Optional: async readdir
async function probeCache(): Promise<void> { if (state === "ready" || state === "loading") return; try { - const entries = fs.readdirSync(cacheDir); + const entries = await fs.promises.readdir(cacheDir); if (entries.length === 0) return; logger.info("memory.embedding.probe_cache", { modelId, cacheDir, entries: entries.length }); await ensureExtractor(); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/embeddingService.ts` around lines 300 - 321, The probeCache function uses blocking fs.readdirSync; change it to the async variant (await fs.promises.readdir(cacheDir)) inside probeCache so startup isn’t blocked, keep the existing guard (state checks), try/catch and logging behavior, preserve the empty-check (if entries.length === 0 return), and continue to call ensureExtractor() after awaiting the non-blocking readdir; reference symbols: probeCache, cacheDir, ensureExtractor, logger.apps/desktop/src/main/services/memory/memoryRepairService.ts (1)
171-176: The suggestedwhere project_id = ?filter is not feasible—memory_procedure_sourcestable does not have aproject_idcolumn.The table schema contains only
procedure_memory_id,episode_memory_id,created_at, and foreign key constraints. However, the underlying efficiency concern is valid: the query loads all procedure sources globally. Since procedure filtering already occurs downstream (procedureRows are filtered by project_id), you could optimize by joining with unified_memories:select ps.procedure_memory_id, ps.episode_memory_id from memory_procedure_sources ps join unified_memories pm on ps.procedure_memory_id = pm.id where pm.project_id = ?This reduces scan scope to the current project only, consistent with other queries in this method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/memoryRepairService.ts` around lines 171 - 176, The current query loading all rows into procedureSources from memory_procedure_sources is inefficient because that table lacks project_id; change the query used by procedureSources to join memory_procedure_sources (alias ps) with unified_memories (alias pm) on ps.procedure_memory_id = pm.id and add a where pm.project_id = ? to restrict to the current project_id (pass the same project id used to filter procedureRows); update the code that constructs procedureSources (variable/procedureSources query) to perform this join/filter so downstream filtering by project is not required.apps/desktop/src/renderer/components/chat/useChatMcpSummary.ts (1)
60-66: Silent error swallowing may hinder debugging.The
.catch(() => {})on line 64 silently discards any errors from the re-fetch. While this prevents unhandled promise rejections, it also makes it difficult to diagnose issues when MCP summary refresh fails after an event.Consider at least logging the error in development:
Suggested improvement
return window.ade.externalMcp.onEvent(() => { cachedSummary = null; - void fetchChatMcpSummary().then(setSummary).catch(() => {}); + void fetchChatMcpSummary().then(setSummary).catch((err) => { + if (import.meta.env.DEV) console.warn("[useChatMcpSummary] refresh failed:", err); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/useChatMcpSummary.ts` around lines 60 - 66, In useChatMcpSummary, the promise rejection from fetchChatMcpSummary is silently swallowed by .catch(() => {}); update the onEvent handler to catch and surface errors (e.g., log them) instead of ignoring them so failures to refresh the MCP summary are visible during debugging; specifically, in the cleanup returned from window.ade.externalMcp.onEvent that clears cachedSummary and calls fetchChatMcpSummary().then(setSummary).catch(...), replace the empty catch with an error handler that logs the error (using console.error or the app logger) and optionally guards by NODE_ENV === 'development' to avoid noisy production logs. Ensure you reference the same symbols: useChatMcpSummary, cachedSummary, fetchChatMcpSummary, setSummary, and window.ade.externalMcp.onEvent.apps/desktop/src/renderer/components/chat/useChatMcpSummary.test.ts (1)
54-57: Consider usingwaitForinstead ofsetTimeoutfor async assertions.Using
setTimeoutfor waiting on async state updates can be flaky and may cause intermittent test failures if the async operation takes longer than expected. The@testing-library/reactpackage provideswaitForwhich polls until the assertion passes.Suggested improvement
+import { act, renderHook, waitFor } from "@testing-library/react"; -import { act, renderHook } from "@testing-library/react"; // Then in tests: - await act(async () => { - await new Promise((r) => setTimeout(r, 10)); - }); - - expect(result.current).toEqual({ + await waitFor(() => expect(result.current).toEqual({ configuredCount: 3, connectedCount: 2, - }); + }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/useChatMcpSummary.test.ts` around lines 54 - 57, Replace the brittle act+setTimeout wait in useChatMcpSummary.test.ts with testing-library's waitFor: import waitFor from `@testing-library/react` (or use the existing import) and change the await act(async () => await new Promise(...)) block to await waitFor(() => /* assertion that depends on async update */) so the test polls until the desired state from useChatMcpSummary is reached; reference the existing act and setTimeout usage and ensure the assertion(s) inside waitFor match the expectations previously checked after the timeout.apps/desktop/src/main/services/ai/providerConnectionStatus.test.ts (1)
128-165: Add the matchingauth-failedregression case.The production change now special-cases both
auth-failedandruntime-failed, but this file only exercises the latter. A sibling test would keep the login-required blocker text and theruntimeAvailable = falseoverride from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/providerConnectionStatus.test.ts` around lines 128 - 165, Add a sibling test mirroring the existing "treats runtime probe failures as launch blockers" case but for the "auth-failed" runtime state: in the new it(...) set mockState.readClaudeCredentials to return credentials and mockState.getProviderRuntimeHealth to return an object with provider: "claude", state: "auth-failed", message: "<login-required blocker text>", checkedAt: new Date().toISOString(); call buildProviderConnections with the same provider inputs and assert result.claude.authAvailable is true, result.claude.runtimeDetected is true, result.claude.runtimeAvailable is false, and result.claude.blocker equals the login-required blocker text (use the exact string your production code emits for auth-failed). Ensure you reference the same symbols: mockState.getProviderRuntimeHealth, mockState.readClaudeCredentials, and buildProviderConnections so the test parallels the runtime-failed case.apps/desktop/src/main/services/ai/providerResolver.test.ts (1)
21-26: Make the Claude executable resolver mock observable.Because Lines 21-26 always return the same object, the Claude-path test below never proves that
authwas actually passed intoresolveClaudeCodeExecutable. If that wiring regresses, this suite still stays green.Suggested fix
-const { createClaudeCodeMock } = vi.hoisted(() => ({ - createClaudeCodeMock: vi.fn(), -})); +const { createClaudeCodeMock, resolveClaudeCodeExecutableMock } = vi.hoisted(() => ({ + createClaudeCodeMock: vi.fn(), + resolveClaudeCodeExecutableMock: vi.fn(() => ({ + path: "/mock/bin/claude", + source: "auth", + })), +})); @@ -vi.mock("./claudeCodeExecutable", () => ({ - resolveClaudeCodeExecutable: () => ({ - path: "/mock/bin/claude", - source: "auth", - }), -})); +vi.mock("./claudeCodeExecutable", () => ({ + resolveClaudeCodeExecutable: resolveClaudeCodeExecutableMock, +})); @@ expect(createClaudeCodeMock).toHaveBeenCalledWith( expect.objectContaining({ defaultSettings: expect.objectContaining({ cwd: "/tmp/worktree", pathToClaudeCodeExecutable: "/mock/bin/claude", @@ }), }), ); + expect(resolveClaudeCodeExecutableMock).toHaveBeenCalledWith({ auth });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/providerResolver.test.ts` around lines 21 - 26, The mock for resolveClaudeCodeExecutable always returns the same object so tests can't verify the caller passed the expected "auth" source; replace the static mock with an observable spy: mock resolveClaudeCodeExecutable as a vi.fn implementation that accepts the incoming args and either returns an object that includes the provided source (e.g., { path: "/mock/bin/claude", source: sourceParam }) or at minimum allows asserting vi.mocked(resolveClaudeCodeExecutable).toHaveBeenCalledWith("auth"); update the mock in providerResolver.test.ts to use vi.fn so the test can assert the function was invoked with "auth".apps/desktop/src/main/services/computerUse/proofObserver.test.ts (1)
20-98: Please pin the de-dup key in this suite.The extraction paths are covered, but nothing here calls
observe()twice with the samesessionId+itemId. That is the contract most likely to regress once observer wiring changes.🧪 Example regression case
it("ignores duplicate tool_result events for the same chat item", () => { const { observer, requests } = createHarness(); const event = { type: "tool_result", tool: "functions.exec_command", result: "Saved screenshot to /tmp/proof.png", itemId: "item-1", status: "completed", } as const; observer.observe(event, "chat-1"); observer.observe(event, "chat-1"); expect(requests).toHaveLength(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/computerUse/proofObserver.test.ts` around lines 20 - 98, The tests don't verify deduplication of tool_result events by sessionId+itemId; add a test in the proofObserver suite that pins the de-dup key by calling observer.observe(...) twice with the same event object and same session id (use the existing createHarness() and the same event shape used in the first spec, e.g. type: "tool_result", itemId: "item-1", status: "completed"), then assert requests.length remains 1 to ensure observe() (the dedup logic) ignores duplicate tool_result events for the same chat item.apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.test.tsx (1)
27-36: Add a regression case for the legacy MCP aliases.
resolveIntegrationTab()still acceptsintegration=mcpandintegration=external-mcp, but this suite only exercisesmanaged-mcp. One deep-link test for each alias would pin the backwards-compat path the component is now carrying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.test.tsx` around lines 27 - 36, Add regression tests to cover legacy MCP aliases: extend the IntegrationsSettingsSection.test.tsx suite to include two more deep-link cases that use initialEntries with integration=mcp and integration=external-mcp (in addition to managed-mcp) and assert they open the same "Managed MCP section" and not the "GitHub section". Locate the tests around the existing MemoryRouter usage and the resolveIntegrationTab behavior in IntegrationsSettingsSection, and duplicate the pattern used in the current "opens the managed MCP tab..." test for both legacy aliases to pin the backwards-compatible path.apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts (1)
112-125: Add one probe test per ADE MCP launch transport.
resolveProbeMcpServers()now serializes the ADE launch config directly into Claude CLI MCP settings. Please lock both the headless stdio shape and the desktop socket-backed shape here; otherwise a transport-specific regression will only surface at runtime.Based on learnings, For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts` around lines 112 - 125, resolveProbeMcpServers currently emits a single ADE MCP entry from resolveAdeMcpServerLaunch which risks missing transport-specific regressions; update resolveProbeMcpServers to return both the headless stdio shape and the desktop socket-backed shape so tests can cover each transport. Concretely, call resolveAdeMcpServerLaunch as before, then produce two serialized MCP server entries (e.g. one representing stdio/headless and one representing desktop-socket) by cloning the launch payload and adjusting the transport-specific fields (transport/socketPath or equivalent command/args/env differences) before passing both into normalizeCliMcpServers("claude", ...); keep references to resolveAdeMcpServerLaunch, resolveProbeRuntimeRoot, normalizeCliMcpServers and the resolveProbeMcpServers function so the two variants are clearly emitted.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
629-639: Reuse the existing status response instead of fetching it twice.
refreshAvailableModels()already callswindow.ade.ai.getStatus(), and this new effect triggers a second status round-trip immediately afterward on pane load. Consider hydratingproviderConnectionsfrom the first response and keeping the dedicated refresh only for the active-turn poll.Also applies to: 678-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 629 - 639, The code is calling window.ade.ai.getStatus() twice on load; instead of fetching again in refreshProviderConnections, hydrate providerConnections from the status already obtained by refreshAvailableModels and only use refreshProviderConnections for the active-turn poll. Modify refreshAvailableModels to expose or set the initial status (or call a shared setter) so providerConnections is initialized from that response (referencing refreshAvailableModels, refreshProviderConnections, providerConnections, and window.ade.ai.getStatus), and change refreshProviderConnections to skip the initial getStatus call on pane load and only perform status refresh when the active-turn poll requires it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e2d62ad-5682-4ebb-b33d-373d76d9651c
⛔ Files ignored due to path filters (7)
docs/architecture/AI_INTEGRATION.mdis excluded by!docs/**docs/architecture/DESKTOP_APP.mdis excluded by!docs/**docs/architecture/SYSTEM_OVERVIEW.mdis excluded by!docs/**docs/features/AGENTS.mdis excluded by!docs/**docs/features/CHAT.mdis excluded by!docs/**docs/features/CONFLICTS.mdis excluded by!docs/**docs/features/PULL_REQUESTS.mdis excluded by!docs/**
📒 Files selected for processing (84)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/claudeCodeExecutable.test.tsapps/desktop/src/main/services/ai/claudeCodeExecutable.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.test.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.tsapps/desktop/src/main/services/ai/providerConnectionStatus.test.tsapps/desktop/src/main/services/ai/providerConnectionStatus.tsapps/desktop/src/main/services/ai/providerResolver.test.tsapps/desktop/src/main/services/ai/providerResolver.tsapps/desktop/src/main/services/ai/tools/memoryTools.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/ai/tools/workflowTools.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/buildComputerUseDirective.test.tsapps/desktop/src/main/services/computerUse/controlPlane.tsapps/desktop/src/main/services/computerUse/proofObserver.test.tsapps/desktop/src/main/services/computerUse/proofObserver.tsapps/desktop/src/main/services/git/gitOperationsService.test.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/memory/embeddingService.tsapps/desktop/src/main/services/memory/episodeFormat.tsapps/desktop/src/main/services/memory/episodicSummaryService.tsapps/desktop/src/main/services/memory/knowledgeCaptureService.test.tsapps/desktop/src/main/services/memory/knowledgeCaptureService.tsapps/desktop/src/main/services/memory/memoryLifecycleService.tsapps/desktop/src/main/services/memory/memoryRepairService.test.tsapps/desktop/src/main/services/memory/memoryRepairService.tsapps/desktop/src/main/services/memory/proceduralLearningService.test.tsapps/desktop/src/main/services/memory/proceduralLearningService.tsapps/desktop/src/main/services/missions/missionPreflightService.test.tsapps/desktop/src/main/services/missions/missionPreflightService.tsapps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.tsapps/desktop/src/main/services/prs/prIssueResolver.tsapps/desktop/src/main/services/prs/prPollingService.tsapps/desktop/src/main/services/prs/prRebaseResolver.tsapps/desktop/src/main/services/prs/resolverUtils.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/deviceRegistryService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/app/SettingsPage.tsxapps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsxapps/desktop/src/renderer/components/chat/chatNavigation.test.tsapps/desktop/src/renderer/components/chat/chatNavigation.tsapps/desktop/src/renderer/components/chat/useChatMcpSummary.test.tsapps/desktop/src/renderer/components/chat/useChatMcpSummary.tsapps/desktop/src/renderer/components/cto/TeamPanel.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/missions/ChatMessageArea.tsxapps/desktop/src/renderer/components/missions/WorkerPermissionsEditor.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/prFormatters.tsapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/prs/tabs/NormalTab.tsxapps/desktop/src/renderer/components/prs/tabs/QueueTab.tsxapps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsxapps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsxapps/desktop/src/renderer/components/settings/ComputerUseSection.tsxapps/desktop/src/renderer/components/settings/ExternalMcpSection.tsxapps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.test.tsxapps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsxapps/desktop/src/renderer/components/settings/MemoryHealthTab.tsxapps/desktop/src/renderer/components/shared/ExternalMcpAccessEditor.tsxapps/desktop/src/renderer/lib/computerUse.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/computerUseArtifacts.tsapps/desktop/src/shared/types/prs.tsapps/ios/ADE/Resources/DatabaseBootstrap.sql
💤 Files with no reviewable changes (3)
- apps/desktop/src/main/services/ai/tools/workflowTools.ts
- apps/desktop/src/renderer/components/app/SettingsPage.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx
Comment on lines
+32
to
+43
| function resolveFromPathEntries(command: string, pathValue: string | undefined): string | null { | ||
| if (!pathValue) return null; | ||
| for (const entry of pathValue.split(path.delimiter)) { | ||
| const trimmed = entry.trim(); | ||
| if (!trimmed) continue; | ||
| const candidatePath = path.join(trimmed, command); | ||
| if (isExecutableFile(candidatePath)) { | ||
| return candidatePath; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js/Electron on Windows, when manually resolving an executable from PATH, do you need to account for PATHEXT extensions like .exe/.cmdand thePATH/Path environment-variable casing?
💡 Result:
Yes, when manually resolving an executable from PATH in Node.js/Electron on Windows, you need to account for both PATHEXT extensions (like .exe, .cmd) and PATH/Path environment variable casing. Node.js child_process.spawn and spawnSync do not automatically handle PATHEXT; they only find .exe files by default and ignore PATHEXT for non-.exe files like .bat/.cmd. For manual resolution matching Windows shell behavior, implement PATH splitting, PATHEXT appending, and file existence checks. The PATH environment variable in process.env may appear as 'PATH' or 'Path' due to Windows case-insensitivity; check both or use case-insensitive lookup (e.g., find key where key.toUpperCase === 'PATH'). Use libraries like 'which' npm package, which properly handles PATH/PATHEXT on Windows. Electron uses the same Node.js child_process, so same rules apply.
Citations:
- 1: child_process.spawn ignores PATHEXT on Windows nodejs/node#6671
- 2: child_process.spawn ignores PATHEXT on Windows nodejs/node-v0.x-archive#2318
- 3: child_process spawn does not pass environment key-value pairs on Windows nodejs/node#34667
- 4: https://www.npmjs.com/package/which
- 5: [BUG]
process.env.PATHcan be undefined on Windows npm/node-which#108
🏁 Script executed:
# First, let's look at the file structure and locate the target file
find apps -name "claudeCodeExecutable.ts" -type fRepository: arul28/ADE
Length of output: 111
🏁 Script executed:
# Read the file to understand the full context
cat -n apps/desktop/src/main/services/ai/claudeCodeExecutable.tsRepository: arul28/ADE
Length of output: 2929
🏁 Script executed:
# Let's also check if the function is called elsewhere and how it's used
rg "resolveFromPathEntries" apps/desktop/Repository: arul28/ADE
Length of output: 333
🏁 Script executed:
# Check what the isExecutableFile function does
rg -A 5 "function isExecutableFile|const isExecutableFile" apps/desktop/src/main/services/ai/Repository: arul28/ADE
Length of output: 633
Windows executables require extension handling and case-insensitive PATH lookup.
On Windows, the claude executable is typically installed as claude.exe or claude.cmd. The current code only checks for the literal name claude without extensions, which will fail to find these installations on Windows. Additionally, the code only accesses env.PATH, but on Windows this environment variable may be cased as Path due to case-insensitivity.
This affects both the PATH probe (line 67) and the COMMON_BIN_DIRS probe (lines 72-76), causing the code to fall through to the fallback "claude" which relies on the shell to resolve the executable.
Suggested fix
+function resolvePathValue(env: NodeJS.ProcessEnv): string | undefined {
+ return env.PATH ?? env.Path;
+}
+
+function executableNames(command: string, env: NodeJS.ProcessEnv): string[] {
+ if (process.platform !== "win32") return [command];
+ const pathExt = (env.PATHEXT ?? ".COM;.EXE;.BAT;.CMD")
+ .split(";")
+ .map((ext) => ext.toLowerCase())
+ .filter(Boolean);
+ return [...new Set([command, ...pathExt.map((ext) => `${command}${ext}`)])];
+}
+
function resolveFromPathEntries(command: string, pathValue: string | undefined): string | null {
if (!pathValue) return null;
for (const entry of pathValue.split(path.delimiter)) {
const trimmed = entry.trim();
if (!trimmed) continue;
- const candidatePath = path.join(trimmed, command);
- if (isExecutableFile(candidatePath)) {
- return candidatePath;
+ for (const executableName of executableNames(command, env)) {
+ const candidatePath = path.join(trimmed, executableName);
+ if (isExecutableFile(candidatePath)) {
+ return candidatePath;
+ }
}
}
return null;
}
- const pathResolved = resolveFromPathEntries("claude", env.PATH);
+ const pathResolved = resolveFromPathEntries("claude", resolvePathValue(env));
for (const binDir of COMMON_BIN_DIRS) {
- const candidatePath = path.join(binDir, "claude");
- if (isExecutableFile(candidatePath)) {
- return { path: candidatePath, source: "common-dir" };
+ for (const executableName of executableNames("claude", env)) {
+ const candidatePath = path.join(binDir, executableName);
+ if (isExecutableFile(candidatePath)) {
+ return { path: candidatePath, source: "common-dir" };
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ai/claudeCodeExecutable.ts` around lines 32 -
43, The PATH probe fails on Windows because resolveFromPathEntries only checks
the literal command string and reads env.PATH with a fixed case; update
resolution to (1) read PATH case-insensitively (check process.env.PATH and
process.env.Path) before calling resolveFromPathEntries, and (2) modify
resolveFromPathEntries to, when on Windows (process.platform === "win32"), try
the command with common executable extensions (e.g., .exe, .cmd, .bat) as well
as the literal name by attempting candidatePath for each extension and calling
isExecutableFile(candidatePath); also apply the same extension-aware check to
the COMMON_BIN_DIRS probe code that looks for "claude" so Windows finds
"claude.exe"/"claude.cmd". Ensure you reference the existing functions/variables
resolveFromPathEntries, isExecutableFile, and COMMON_BIN_DIRS when making
changes.
Comment on lines
164
to
178
| const probe = (async (): Promise<ClaudeRuntimeProbeResult> => { | ||
| const abortController = new AbortController(); | ||
| const timeout = setTimeout(() => abortController.abort(), PROBE_TIMEOUT_MS); | ||
| const claudeExecutable = resolveClaudeCodeExecutable(); | ||
| const stream = claudeQuery({ | ||
| prompt: "System initialization check. Respond with only the word READY.", | ||
| options: { | ||
| cwd: projectRoot, | ||
| permissionMode: "plan", | ||
| tools: [], | ||
| pathToClaudeCodeExecutable: claudeExecutable.path, | ||
| mcpServers: resolveProbeMcpServers(projectRoot) as any, | ||
| abortController, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Keep probe setup inside the guarded path and reuse the resolved executable path.
resolveClaudeCodeExecutable(), resolveProbeMcpServers(), and claudeQuery(...) construction currently happen before the try, so any synchronous failure escapes the runtime-failed conversion entirely. The warning logger then resolves the executable a second time, which can throw again or log a different path than the one that actually failed.
🛡️ Suggested hardening
+ let claudeExecutablePath: string | null = null;
const probe = (async (): Promise<ClaudeRuntimeProbeResult> => {
const abortController = new AbortController();
const timeout = setTimeout(() => abortController.abort(), PROBE_TIMEOUT_MS);
- const claudeExecutable = resolveClaudeCodeExecutable();
- const stream = claudeQuery({
- prompt: "System initialization check. Respond with only the word READY.",
- options: {
- cwd: projectRoot,
- permissionMode: "plan",
- tools: [],
- pathToClaudeCodeExecutable: claudeExecutable.path,
- mcpServers: resolveProbeMcpServers(projectRoot) as any,
- abortController,
- },
- });
+ let stream: ReturnType<typeof claudeQuery> | null = null;
try {
+ claudeExecutablePath = resolveClaudeCodeExecutable().path;
+ stream = claudeQuery({
+ prompt: "System initialization check. Respond with only the word READY.",
+ options: {
+ cwd: projectRoot,
+ permissionMode: "plan",
+ tools: [],
+ pathToClaudeCodeExecutable: claudeExecutablePath,
+ mcpServers: resolveProbeMcpServers(projectRoot) as any,
+ abortController,
+ },
+ });
for await (const message of stream) {
const result = resultFromSdkMessage(message);
if (result) {
return cacheResult(projectRoot, result);
}
@@
} finally {
clearTimeout(timeout);
try {
- stream.close();
+ stream?.close();
} catch {
// Best effort cleanup — avoid leaving the probe subprocess running.
}
}
})();
@@
args.logger?.warn?.("ai.claude_runtime_probe.failed", {
projectRoot,
state: result.state,
message: result.message,
- claudeExecutablePath: resolveClaudeCodeExecutable().path,
+ claudeExecutablePath,
});Also applies to: 180-205, 216-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts` around lines 164 -
178, Move the synchronous setup (calls to resolveClaudeCodeExecutable(),
resolveProbeMcpServers(projectRoot) and the claudeQuery(...) construction) into
the try block so any thrown errors are converted to the "runtime-failed" path;
capture and reuse the resolved executable path (the result of
resolveClaudeCodeExecutable()) for both the claudeQuery options and any
subsequent warning/error logs to avoid resolving twice; also create the
AbortController and timeout after entering the try so aborts are handled under
the same guarded flow and references like PROBE_TIMEOUT_MS, projectRoot,
claudeQuery, resolveProbeMcpServers and abortController all live inside the
try/catch that converts failures.
Comment on lines
+865
to
+867
| "You have computer-use capabilities available. ADE will automatically capture screenshots and other artifacts from your tool calls into the proof drawer — you do not need to manually call ingest_computer_use_artifacts.", | ||
| "", | ||
| "Call `get_computer_use_backend_status` to check available backends before attempting computer use.", |
There was a problem hiding this comment.
Gate the “automatic proof capture” instructions on passive capture actually being enabled.
This prompt says screenshots/traces are captured automatically and that the agent does not need ingest_computer_use_artifacts, but passive capture only exists when proofObserver is wired up. In the no-broker/headless path, that guidance can drop artifacts because nothing is observing tool_result events. Please pass a passiveCaptureEnabled flag into buildComputerUseDirective(...) and fall back to manual-ingest instructions when it is false.
Based on learnings: "For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path."
Also applies to: 920-921, 1164-1166, 2072-2075
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 865 -
867, The prompt unconditionally claims ADE will automatically capture
screenshots/artifacts, but that only holds when the proofObserver is wired;
update buildComputerUseDirective(...) to accept a passiveCaptureEnabled boolean
and branch the returned instructions: when passiveCaptureEnabled is true keep
the existing automatic-proof-capture text, otherwise replace that part with
guidance to call ingest_computer_use_artifacts (i.e., explicit manual ingest)
and still advise calling get_computer_use_backend_status; update all call sites
that construct the directive (references at or near lines using
buildComputerUseDirective, including the occurrences noted) to pass the correct
passiveCaptureEnabled value from the runtime (headless vs socket-backed MCP) so
headless/no-broker paths receive the manual-ingest guidance.
apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx
Outdated
Show resolved
Hide resolved
apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR finalizes several UX + infrastructure updates across ADE’s desktop app, centered on (1) memory quality/format improvements, (2) passive computer-use proof capture and “computer use always available” policy, (3) PR/queue/rebase workflow improvements, and (4) consistent “ADE-managed MCP” branding and navigation.
Changes:
- Memory pipeline: new episodic memory dual-format (human-readable + embedded JSON), UI rendering updates, memory repair/filters, and adjusted promotion thresholds.
- Computer use + MCP: remove “off” mode, add passive proof observer, improve backend detection/guidance, add integrations sub-tabs and managed MCP labeling.
- PR/AI/runtime improvements: PR polling fingerprinting + interval docs, queue-aware rebase docs, shared PR UI formatters, provider runtime health surfacing, and Claude CLI executable resolution/probing.
Reviewed changes
Copilot reviewed 93 out of 93 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/features/PULL_REQUESTS.md | Updates PR surface docs (polling/fingerprint, review threads, queue-aware rebase, conflict parsing, checks status logic). |
| docs/features/CONFLICTS.md | Updates conflicts feature matrix with queue-aware rebase entry. |
| docs/features/CHAT.md | Documents text batching + session identity propagation. |
| docs/features/AGENTS.md | Doc timestamp refresh. |
| docs/architecture/SYSTEM_OVERVIEW.md | Updates architecture doc for PR polling, queue-aware rebase, and memory pipeline quality filters. |
| docs/architecture/DESKTOP_APP.md | Notes embedding cache probing at startup. |
| docs/architecture/AI_INTEGRATION.md | Updates provider/model registry docs; adds session identity + computer use policy guidance. |
| apps/ios/ADE/Resources/DatabaseBootstrap.sql | Adds chat_session_id to external MCP usage events + index. |
| apps/desktop/src/shared/types/prs.ts | Expands PR event payload with additional PR/repo/branch metadata. |
| apps/desktop/src/shared/types/computerUseArtifacts.ts | Removes off mode; migrates persisted values; keeps shim for compatibility. |
| apps/desktop/src/shared/types/chat.ts | Adds optional sequence to chat event envelopes. |
| apps/desktop/src/shared/modelRegistry.ts | Adds resolveProviderGroupForModel helper. |
| apps/desktop/src/renderer/state/appStore.ts | Adds lane-scoped work view state keyed by project+lane. |
| apps/desktop/src/renderer/state/appStore.test.ts | Adds test coverage for appStore state normalization and lane-scoped work view state. |
| apps/desktop/src/renderer/lib/computerUse.ts | Updates computer-use mode formatting to match removed “off” mode. |
| apps/desktop/src/renderer/components/shared/ExternalMcpAccessEditor.tsx | Renames UI copy to “ADE-managed MCP access”. |
| apps/desktop/src/renderer/components/settings/MemoryHealthTab.tsx | Adds episode parsing/rendering, strips embedded JSON from display, and adds bulk approve/archive controls. |
| apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx | Adds integrations sub-tabs driven by URL search params. |
| apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.test.tsx | Tests integrations sub-tab routing and switching. |
| apps/desktop/src/renderer/components/settings/ExternalMcpSection.tsx | Updates UI copy and error strings to “ADE-managed MCP”; improves explanatory text. |
| apps/desktop/src/renderer/components/settings/ComputerUseSection.tsx | Redesigns computer use settings UI; shows detected backends and Ghost OS recommendation. |
| apps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsx | Switches to shared PR formatter utilities. |
| apps/desktop/src/renderer/components/prs/tabs/RebaseTab.tsx | Switches to shared PR formatter utilities. |
| apps/desktop/src/renderer/components/prs/tabs/QueueTab.tsx | Switches to shared PR formatter utilities (error formatting). |
| apps/desktop/src/renderer/components/prs/tabs/NormalTab.tsx | Switches to shared PR formatter utilities (compact “time ago”). |
| apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx | Switches to shared PR formatter utilities (timestamps + “time ago”). |
| apps/desktop/src/renderer/components/prs/state/PrsContext.tsx | Refreshes rebase needs + auto-rebase statuses alongside main PR refresh. |
| apps/desktop/src/renderer/components/prs/shared/prFormatters.ts | New shared formatter module for PR UI. |
| apps/desktop/src/renderer/components/missions/WorkerPermissionsEditor.tsx | Renames “External MCP” to “ADE-managed MCP” in mission permissions UI. |
| apps/desktop/src/renderer/components/missions/ChatMessageArea.tsx | Updates MCP badge labeling + tooltip to “ADE-managed MCP”. |
| apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts | New hook for lane work sessions, using lane-scoped work view state and background refresh logic. |
| apps/desktop/src/renderer/components/lanes/LaneWorkPane.tsx | Replaces older split terminal/chat UI with unified WorkViewArea and entry options. |
| apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx | Fixes style usage to set border rather than borderColor in several buttons. |
| apps/desktop/src/renderer/components/cto/TeamPanel.tsx | Updates worker MCP policy description copy. |
| apps/desktop/src/renderer/components/chat/useChatMcpSummary.ts | Adds event-driven refresh when external MCP registry changes. |
| apps/desktop/src/renderer/components/chat/useChatMcpSummary.test.ts | Adds tests for useChatMcpSummary, including event refresh. |
| apps/desktop/src/renderer/components/chat/chatNavigation.ts | Deep-links settings navigation to managed MCP sub-tab. |
| apps/desktop/src/renderer/components/chat/chatNavigation.test.ts | Tests navigation URL + popstate dispatch. |
| apps/desktop/src/renderer/components/chat/ChatSubagentStrip.tsx | Simplifies pluralization logic for summary labels. |
| apps/desktop/src/renderer/components/chat/AgentChatPane.tsx | Removes “off” mode behavior; adds provider runtime blocker banner and polling while turns active. |
| apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts | Updates tests for removed computer-use-off behavior. |
| apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx | Adds regression test for reasoning block separation across tool boundaries. |
| apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx | Removes UI toggle for computer-use off; refactors slash-command defaults mapping. |
| apps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsx | Updates tests to match removed computer-use toggle behavior. |
| apps/desktop/src/renderer/components/automations/components/RuleEditorPanel.tsx | Renames automation palette label to “Managed MCP”. |
| apps/desktop/src/renderer/components/app/SettingsPage.tsx | Removes old settings tab alias for external-mcp. |
| apps/desktop/src/renderer/components/app/CommandPalette.tsx | Updates integrations hint copy to “managed MCP”. |
| apps/desktop/src/renderer/browserMock.ts | Adds update-related stubs to renderer mock. |
| apps/desktop/src/main/services/sync/deviceRegistryService.ts | Preserves persisted device IPs/tailscale IP when available (improves persistence). |
| apps/desktop/src/main/services/pty/ptyService.ts | Adds richer spawn/startup failure logging context. |
| apps/desktop/src/main/services/prs/resolverUtils.ts | New shared helpers for PR resolvers (permission mode mapping, recent commits). |
| apps/desktop/src/main/services/prs/prRebaseResolver.ts | Uses new shared resolver helpers. |
| apps/desktop/src/main/services/prs/prPollingService.ts | Switches PR polling to stable fingerprint-based change detection; extends event payload fields. |
| apps/desktop/src/main/services/prs/prIssueResolver.ts | Uses new shared resolver helpers. |
| apps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.ts | Improves runtime root detection; normalizes env injection; uses provider-group resolver; validates permission modes. |
| apps/desktop/src/main/services/missions/missionPreflightService.ts | Removes “computer use off” gating; updates proof readiness logic. |
| apps/desktop/src/main/services/missions/missionPreflightService.test.ts | Updates tests for new proof gating behavior. |
| apps/desktop/src/main/services/memory/proceduralLearningService.ts | Uses shared episode parser; filters PR-feedback and low-signal episodes; clarifies outcome mapping. |
| apps/desktop/src/main/services/memory/memoryRepairService.ts | New service to rewrite legacy episodes and archive low-value PR-feedback-derived memories/procedures. |
| apps/desktop/src/main/services/memory/memoryRepairService.test.ts | Adds tests for memory repair behavior. |
| apps/desktop/src/main/services/memory/memoryLifecycleService.ts | Adjusts candidate promotion thresholds (system-sourced entries can promote at lower confidence). |
| apps/desktop/src/main/services/memory/knowledgeCaptureService.ts | Adds low-signal PR feedback filtering; changes episode content formatting to dual-format; stops writing companion episodes for PR feedback. |
| apps/desktop/src/main/services/memory/knowledgeCaptureService.test.ts | Updates tests: no PR feedback episodes + ignores low-signal nudges. |
| apps/desktop/src/main/services/memory/episodicSummaryService.ts | Writes episode content in dual-format; loosens “trivial” threshold. |
| apps/desktop/src/main/services/memory/episodeFormat.ts | New shared episode parse/format module for dual-format episodes. |
| apps/desktop/src/main/services/memory/embeddingService.ts | Adds best-effort cache probe to auto-load previously downloaded models. |
| apps/desktop/src/main/services/lanes/laneService.test.ts | Adjusts git mock handling. |
| apps/desktop/src/main/services/git/gitOperationsService.test.ts | Updates expectations to include staged patch diff call. |
| apps/desktop/src/main/services/computerUse/proofObserver.ts | New passive proof observer that ingests artifacts from tool results. |
| apps/desktop/src/main/services/computerUse/proofObserver.test.ts | Adds tests for proof observer extraction/normalization behavior. |
| apps/desktop/src/main/services/computerUse/controlPlane.ts | Updates Ghost OS guidance/copy; removes off-mode gating; refactors active backend + proof summary derivation. |
| apps/desktop/src/main/services/computerUse/controlPlane.test.ts | Mocks ghost doctor process health for stable tests. |
| apps/desktop/src/main/services/chat/buildComputerUseDirective.test.ts | Updates directive tests for backend-aware behavior and no-off-mode semantics. |
| apps/desktop/src/main/services/ai/tools/workflowTools.ts | Removes off-mode guard for captureScreenshot (still respects fallback/backends). |
| apps/desktop/src/main/services/ai/tools/systemPrompt.ts | Tightens memory-writing guidance to avoid derivable/low-value memories. |
| apps/desktop/src/main/services/ai/tools/systemPrompt.test.ts | Adds test coverage for system prompt composition and sections. |
| apps/desktop/src/main/services/ai/tools/memoryTools.ts | Updates memoryAdd guidance to avoid code-derivable facts and fix recipes. |
| apps/desktop/src/main/services/ai/providerResolver.ts | Normalizes MCP server transport/type logic; sets explicit Claude executable path in default settings. |
| apps/desktop/src/main/services/ai/providerResolver.test.ts | Adds Claude provider resolution test + mocks executable resolver. |
| apps/desktop/src/main/services/ai/providerConnectionStatus.ts | Treats runtime probe failures as blockers; refactors connection status assembly. |
| apps/desktop/src/main/services/ai/providerConnectionStatus.test.ts | Adds coverage for runtime-failed health state behavior. |
| apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts | Runs probe with explicit Claude executable + ADE MCP config; improves runtime root detection and reporting. |
| apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts | Adds mocks/assertions for executable + MCP config injection during probe. |
| apps/desktop/src/main/services/ai/claudeCodeExecutable.ts | New helper to resolve Claude Code CLI executable path from env/auth/PATH/common dirs. |
| apps/desktop/src/main/services/ai/claudeCodeExecutable.test.ts | Adds tests for executable resolution precedence. |
| apps/desktop/src/main/main.ts | Wires embedding cache probe at startup; runs memory repair on project open. |
| .github/workflows/ci.yml | Increases Vitest shard count from 3 to 5. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+577
to
+587
| logger.warn("pty.spawn_retry", { | ||
| ptyId, | ||
| sessionId, | ||
| shell: shell.file, | ||
| cwd, | ||
| toolType: toolTypeHint, | ||
| startupCommandPresent: Boolean(startupCommand), | ||
| envShell: process.env.SHELL ?? "", | ||
| envPath: process.env.PATH ?? "", | ||
| resourcesPath: process.resourcesPath ?? "", | ||
| err: String(err), |
There was a problem hiding this comment.
These logs include the full process.env.PATH (and SHELL/resourcesPath) in warn/error paths. This can leak user-specific filesystem details into logs and tends to be very noisy/large. Consider logging a sanitized subset (e.g., whether PATH is empty, or a hash/first N chars), or gating the full env dump behind an explicit debug flag.
Comment on lines
+35
to
+46
| /** Full date/time label: "Jan 15, 2026, 02:30 PM" */ | ||
| export function formatTimestampFull(iso: string | null): string { | ||
| if (!iso) return "---"; | ||
| const d = new Date(iso); | ||
| if (Number.isNaN(d.getTime())) return iso; | ||
| return d.toLocaleDateString(undefined, { | ||
| month: "short", | ||
| day: "numeric", | ||
| year: "numeric", | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| }); |
There was a problem hiding this comment.
formatTimestampFull uses Date#toLocaleDateString but passes hour/minute options and the docstring says it returns a full date/time label. toLocaleDateString is date-only in many environments and may ignore the time options, so this can silently drop the time. Use toLocaleString (or Intl.DateTimeFormat) for a true date+time formatter.
Comment on lines
2213
to
+2235
| create table if not exists external_mcp_usage_events ( | ||
| id text primary key, | ||
| project_id text not null, | ||
| server_name text not null, | ||
| tool_name text not null, | ||
| namespaced_tool_name text not null, | ||
| safety text not null, | ||
| caller_role text not null, | ||
| caller_id text not null, | ||
| chat_session_id text, | ||
| mission_id text, | ||
| run_id text, | ||
| step_id text, | ||
| attempt_id text, | ||
| owner_id text, | ||
| cost_cents integer not null default 0, | ||
| estimated integer not null default 0, | ||
| occurred_at text not null, | ||
| created_at text not null | ||
| ); | ||
|
|
||
| alter table external_mcp_usage_events add column chat_session_id text; | ||
|
|
There was a problem hiding this comment.
external_mcp_usage_events defines chat_session_id in the create table statement and then immediately runs alter table external_mcp_usage_events add column chat_session_id. On a fresh bootstrap (table just created), this alter table will fail because the column already exists. Either remove the column from the create table block (and rely on the alter table for migrations), or drop/guard the alter table (e.g., ADD COLUMN IF NOT EXISTS if you can rely on a new-enough SQLite).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8652023aee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+5753
to
+5757
| noticeKind: "info", | ||
| message: "Steer queued — waiting for current turn to complete.", | ||
| turnId: runtime.activeTurnId ?? undefined, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
This branch returns early without enqueuing the new steer, but the emitted notice says it was queued. In the full-queue case, the user's instruction is silently discarded while the UI tells them it will run after the current turn, which can cause lost guidance and confusing behavior during long-running turns. Emit a "queue full/dropped" notice (or actually enqueue after eviction) instead of the current "queued" message.
Useful? React with 👍 / 👎.
Comment on lines
+376
to
+380
| /** Track ingested itemIds per session for de-duplication. */ | ||
| const ingestedBySession = new Map<string, Set<string>>(); | ||
|
|
||
| function getSessionSet(sessionId: string): Set<string> { | ||
| let set = ingestedBySession.get(sessionId); |
There was a problem hiding this comment.
The observer keeps a per-session Set of every ingested itemId, but this map is unbounded and only shrinks if clearSession is called. In the current integration path, chat events are continuously observed and these sets grow with tool activity, so long-lived app sessions can accumulate stale dedupe state and memory. Tie this cache to session lifecycle (dispose/end) or add eviction.
Useful? React with 👍 / 👎.
Comment on lines
+414
to
+417
| onClick={onClose} | ||
| title="Open backend settings" | ||
| > | ||
| <div className="font-sans text-[12px] font-medium text-fg/84">Allow local fallback</div> | ||
| <div className="mt-1 text-[11px] leading-5 text-fg/56"> | ||
| {policy.allowLocalFallback | ||
| ? "ADE may fall back to its local runtime when a connected backend is missing or unavailable." | ||
| : "The chat must rely on connected external tooling only."} | ||
| </div> | ||
| Backend settings |
There was a problem hiding this comment.
The "Backend settings" button is labeled and titled as navigation, but it only calls onClose, so clicking it just dismisses the modal and does not open any backend configuration surface. This creates a dead-end UX for users trying to configure computer-use backends from chat.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
98-111: Use a single source of truth for total shards.
5is duplicated in the matrix and CLI argument, which can drift in future edits. Consider centralizing it in an env var.♻️ Suggested cleanup
test-desktop: needs: install runs-on: ubuntu-latest + env: + DESKTOP_TOTAL_SHARDS: 5 strategy: fail-fast: false matrix: shard: [1, 2, 3, 4, 5] @@ - - run: cd apps/desktop && npx vitest run --shard=${{ matrix.shard }}/5 + - run: cd apps/desktop && npx vitest run --shard=${{ matrix.shard }}/${{ env.DESKTOP_TOTAL_SHARDS }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 98 - 111, Centralize the total shard count by adding an env var (e.g., TOTAL_SHARDS) and replace the hardcoded "5" with that var so the matrix and the vitest CLI stay consistent; specifically, define TOTAL_SHARDS in the job/env and update the matrix or generation logic for shard values to derive from TOTAL_SHARDS (or keep explicit shard list but document it comes from TOTAL_SHARDS), then change the test step invocation from "npx vitest run --shard=${{ matrix.shard }}/5" to "npx vitest run --shard=${{ matrix.shard }}/ ${{ env.TOTAL_SHARDS }}" so matrix.shard and the vitest --shard denominator use the same single source of truth.apps/desktop/src/main/services/computerUse/controlPlane.test.ts (1)
8-18: Add a companion test for the socket-backed MCP path.This mock improves stale-state coverage for one runtime mode, but I don’t see coverage here for the desktop socket-backed MCP flow. Please add a parallel assertion for that mode so stale guidance stays consistent across both execution paths.
Based on learnings: For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/computerUse/controlPlane.test.ts` around lines 8 - 18, The test currently mocks getGhostDoctorProcessHealth in localComputerUse to assert stale-state guidance for one runtime mode but lacks a parallel test for the desktop socket-backed MCP flow; add a companion test in controlPlane.test.ts that repeats the same assertion path but configures/boots the code under test to use the socket-backed MCP flow (the same mocked getGhostDoctorProcessHealth from localComputerUse) and verifies the stale guidance/output for that mode as well. Locate the existing vi.mock of "./localComputerUse" and the test that asserts stale behavior, duplicate the test setup but switch to the socket-backed execution path (e.g., toggle whatever runtime-mode flag or constructor used by the control plane to select socket-backed MCP) and assert identical stale-state handling for the socket-backed path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 98-111: Centralize the total shard count by adding an env var
(e.g., TOTAL_SHARDS) and replace the hardcoded "5" with that var so the matrix
and the vitest CLI stay consistent; specifically, define TOTAL_SHARDS in the
job/env and update the matrix or generation logic for shard values to derive
from TOTAL_SHARDS (or keep explicit shard list but document it comes from
TOTAL_SHARDS), then change the test step invocation from "npx vitest run
--shard=${{ matrix.shard }}/5" to "npx vitest run --shard=${{ matrix.shard }}/
${{ env.TOTAL_SHARDS }}" so matrix.shard and the vitest --shard denominator use
the same single source of truth.
In `@apps/desktop/src/main/services/computerUse/controlPlane.test.ts`:
- Around line 8-18: The test currently mocks getGhostDoctorProcessHealth in
localComputerUse to assert stale-state guidance for one runtime mode but lacks a
parallel test for the desktop socket-backed MCP flow; add a companion test in
controlPlane.test.ts that repeats the same assertion path but configures/boots
the code under test to use the socket-backed MCP flow (the same mocked
getGhostDoctorProcessHealth from localComputerUse) and verifies the stale
guidance/output for that mode as well. Locate the existing vi.mock of
"./localComputerUse" and the test that asserts stale behavior, duplicate the
test setup but switch to the socket-backed execution path (e.g., toggle whatever
runtime-mode flag or constructor used by the control plane to select
socket-backed MCP) and assert identical stale-state handling for the
socket-backed path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9c5b082-7b74-4306-bc7f-392128e259fc
📒 Files selected for processing (2)
.github/workflows/ci.ymlapps/desktop/src/main/services/computerUse/controlPlane.test.ts
- claudeCodeExecutable: iterate all auth entries, skip unusable ones - claudeRuntimeProbe: move probe setup inside try block, prevent unhandled throws - agentChatService: move auto-memory await inside try, fix steer queue-full message - episodicSummaryService/episodeFormat: base64-encode episode delimiters - AgentChatComposer: remove dead "Backend settings" button - AgentChatMessageList: add command/file_change to merge boundary check - useLaneWorkSessions: preserve queued force refresh flags - PrDetailPane: collapse reruns, fix dedup keys, fix non-success summary - IntegrationsSettingsSection: derive activeTab from URL instead of state - appStore: guard stale refreshLanes responses on project switch - computerUseArtifacts: fix normalizeComputerUsePolicy mode precedence - prFormatters: toLocaleDateString → toLocaleString for full timestamp - DatabaseBootstrap.sql: remove duplicate chat_session_id column definition Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- claudeCodeExecutable: remove isExecutableFile check in findClaudeAuthPath (auth paths are trusted, don't need disk validation in tests/CI) - AgentChatMessageList.test: update test to expect no coalescing across command boundaries (matches the new correct behavior) - DatabaseBootstrap.sql: regenerate from kvDb.ts source of truth (the try/catch ALTER TABLE is an intentional migration pattern) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/memory/knowledgeCaptureService.ts (1)
222-248:⚠️ Potential issue | 🟠 MajorEscape the reserved
<!--episode:marker in visible text.
toEpisodeContent()now writes raw user-derived text before the structured comment, butparseEpisode()inapps/desktop/src/main/services/memory/episodeFormat.ts:37-60only parses the first<!--episode:...-->block it finds. IfsourceLabel,summary, or any list item already contains that sentinel, the real payload becomes unreachable and the episode stops round-tripping.💡 Proposed fix
+ const escapeEpisodeMarker = (value: string): string => value.replace(/<!--episode:/g, "<!-- episode:"); const lines: string[] = []; - if (args.sourceLabel) lines.push(args.sourceLabel); - if (args.summary && args.summary !== args.sourceLabel) lines.push(args.summary); + if (args.sourceLabel) lines.push(escapeEpisodeMarker(args.sourceLabel)); + if (args.summary && args.summary !== args.sourceLabel) lines.push(escapeEpisodeMarker(args.summary)); const patterns = (args.patterns ?? []).filter(Boolean); - if (patterns.length > 0) lines.push(`Patterns: ${patterns.join("; ")}`); + if (patterns.length > 0) lines.push(`Patterns: ${patterns.map(escapeEpisodeMarker).join("; ")}`); const gotchas = (args.gotchas ?? []).filter(Boolean); - if (gotchas.length > 0) lines.push(`Pitfalls: ${gotchas.join("; ")}`); + if (gotchas.length > 0) lines.push(`Pitfalls: ${gotchas.map(escapeEpisodeMarker).join("; ")}`); const decisions = (args.decisions ?? []).filter(Boolean); - if (decisions.length > 0) lines.push(`Decisions: ${decisions.join("; ")}`); + if (decisions.length > 0) lines.push(`Decisions: ${decisions.map(escapeEpisodeMarker).join("; ")}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/memory/knowledgeCaptureService.ts` around lines 222 - 248, toEpisodeContent() must sanitize any user-derived fields so they cannot contain the reserved sentinel "<!--episode:" before appending the structured base64 block; update toEpisodeContent to scan args.sourceLabel, args.summary, each string in args.patterns, args.gotchas, args.decisions (and args.fileScopePattern if present) and escape or encode any instances of the exact substring "<!--episode:" (for example by inserting an escape character or substituting a safe representation) so visible text cannot produce a fake sentinel; keep the existing episode object and the final lines.push of the base64 comment intact so parseEpisode() (which reads the first <!--episode:...-->) always finds the real payload.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx (2)
1896-1920:⚠️ Potential issue | 🟡 MinorDon't report neutral/cancelled checks as “skipped.”
The
skippedbucket currently includesneutralandcancelled, so this summary can still emit strings likeAll 2 checks skippedfor a set of cancelled or neutral results. Split those conclusions out, or switch this bucket to a broader label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx` around lines 1896 - 1920, The summary currently counts neutral and cancelled checks as "skipped" which mislabels results; update the calculation in PrDetailPane so that skipped only includes checks with conclusion === "skipped" and add separate counters for neutral and cancelled (e.g., neutralCount and cancelledCount) or else change the label to a broader term; then update the summaryText logic (references: unifiedChecks, passing, failing, pending, skipped, summaryText, toggleExpand) to use the new counters or label so neutral/cancelled are not reported as skipped.
1797-1848:⚠️ Potential issue | 🟠 MajorReintroduce the unique bare-name fallback when deduping Actions jobs.
coveredNamesnow only tracks theworkflow / jobvariants. If the mirrored Actions check reacheschecksas justjob.name, the second pass will keep it, so the same job shows up twice and the summary counts it twice. The safer fix is the one-time bare-name fallback only for job names that are unique across the latest runs.Proposed fix
function buildUnifiedChecks(checks: PrCheck[], actionRuns: PrActionRun[]): UnifiedCheckItem[] { const items: UnifiedCheckItem[] = []; const coveredNames = new Set<string>(); + const bareJobNameCounts = new Map<string, number>(); // Collapse reruns: for each workflow name, keep only the newest run // (multiple runs with the same name are reruns of the same workflow) const latestRunByWorkflow = new Map<string, PrActionRun>(); for (const run of actionRuns) { @@ } const dedupedRuns = Array.from(latestRunByWorkflow.values()); + + for (const run of dedupedRuns) { + for (const job of run.jobs) { + const bareName = job.name.toLowerCase(); + bareJobNameCounts.set(bareName, (bareJobNameCounts.get(bareName) ?? 0) + 1); + } + } // First: add all jobs from the latest action runs (these have the most detail) for (const run of dedupedRuns) { for (const job of run.jobs) { // Build the canonical name to match against checks API const canonicalName = `${run.name} / ${job.name}`; coveredNames.add(canonicalName.toLowerCase()); // Use composite key to avoid collisions across workflows (e.g. two workflows both having a "build" job) coveredNames.add(`${run.name}/${job.name}`.toLowerCase()); + if ((bareJobNameCounts.get(job.name.toLowerCase()) ?? 0) === 1) { + coveredNames.add(job.name.toLowerCase()); + } const duration = job.startedAt && job.completedAt ? Math.round((new Date(job.completedAt).getTime() - new Date(job.startedAt).getTime()) / 1000) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx` around lines 1797 - 1848, When deduping Actions jobs we need to reintroduce a one-time bare-name fallback for unique job names to avoid duplicate entries; compute a frequency map of job.name (case-insensitive) across dedupedRuns (use symbols latestRunByWorkflow/dedupedRuns/actionRuns) before building coveredNames, then when iterating runs/jobs and adding keys to coveredNames (in the loop that pushes items), also add job.name.toLowerCase() only if the frequency for that bare job name equals 1; keep the existing composite keys (`${run.name}/${job.name}`) and canonical `"${run.name} / ${job.name}"` handling unchanged so non-unique bare names are not added.
🧹 Nitpick comments (7)
apps/desktop/src/renderer/components/settings/MemoryHealthTab.tsx (1)
1181-1194: Bulk actions fire all operations concurrently without error aggregation.The current implementation iterates and fires all
handlePromote/handleArchivecalls in parallel usingvoid. This has a few concerns:
handlePromoteinternally callsloadDashboard({ quiet: true })after each operation, so the dashboard may reload N times concurrently- Partial failures are silently lost since individual errors aren't aggregated
- The UI could show inconsistent state if some operations fail
Consider batching the operations or handling them sequentially with error collection:
♻️ Suggested approach with sequential processing and error aggregation
<button type="button" style={primaryButton({ height: 24, padding: "0 10px", fontSize: 10 })} - onClick={() => { for (const e of candidateEntries) void handlePromote(e.id); }} + onClick={async () => { + const errors: string[] = []; + for (const e of candidateEntries) { + try { + await memoryApi.promote({ id: e.id }); + } catch (err) { + errors.push(err instanceof Error ? err.message : String(err)); + } + } + if (errors.length > 0) setActionError(`Failed to approve ${errors.length} entries`); + void loadDashboard({ quiet: true }); + }} > Approve All </button>Apply a similar pattern for "Archive All".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/MemoryHealthTab.tsx` around lines 1181 - 1194, The bulk buttons currently fire handlePromote/handleArchive for every candidateEntry in parallel (using void), causing multiple concurrent loadDashboard calls and lost errors; change the click handlers to iterate sequentially (for..of with await) over candidateEntries calling handlePromote(id) / handleArchive(id), collect any thrown errors into an array, and after the loop call loadDashboard({ quiet: true }) once and surface aggregated errors (e.g., count/message) to the UI so partial failures are not silently ignored; reference handlePromote, handleArchive, loadDashboard, and candidateEntries when making the change.apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts (1)
30-30: Module-level cache persists across workspace switches.
runtimeRootCacheis set once and never cleared. If the user switches projects or workspaces, the cached runtime root from the previous project will be reused. Consider either clearing this cache whenresetClaudeRuntimeProbeCache()is called, or making it project-aware.♻️ Clear runtimeRootCache alongside probe cache
export function resetClaudeRuntimeProbeCache(): void { probeCache.clear(); + runtimeRootCache = null; }Also applies to: 93-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts` at line 30, The module-level variable runtimeRootCache currently persists across workspace switches; update the probe cache reset logic so runtimeRootCache is cleared when resetClaudeRuntimeProbeCache() is called (or refactor runtimeRootCache to be project-aware keyed by workspace/project id). Locate runtimeRootCache and the resetClaudeRuntimeProbeCache() function and ensure the reset path explicitly sets runtimeRootCache = null (or removes the entry for the current project) so the cached runtime root cannot leak between workspaces; apply the same change for any other cache variables referenced in the probe (lines around the probe's main functions referenced in the diff).apps/desktop/src/main/services/ai/claudeCodeExecutable.ts (1)
12-21: COMMON_BIN_DIRS omits Windows paths.The list only contains Unix-style paths. On Windows, Claude CLI is typically installed in user-profile directories or
%LOCALAPPDATA%\Programs. Consider adding Windows-specific paths whenprocess.platform === "win32".💡 Suggested enhancement
+const WINDOWS_BIN_DIRS = [ + `${process.env.LOCALAPPDATA ?? ""}\\Programs\\claude-cli`, + `${process.env.APPDATA ?? ""}\\.claude\\bin`, +].filter((p) => p && !p.startsWith("\\")); + const COMMON_BIN_DIRS = [ "/opt/homebrew/bin", "/opt/homebrew/sbin", "/usr/local/bin", "/usr/local/sbin", "/usr/bin", "/bin", `${HOME_DIR}/.local/bin`, `${HOME_DIR}/.nvm/current/bin`, -].filter(Boolean); + ...(process.platform === "win32" ? WINDOWS_BIN_DIRS : []), +].filter(Boolean);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/claudeCodeExecutable.ts` around lines 12 - 21, COMMON_BIN_DIRS currently lists only Unix-style locations; update the initialization to include Windows-specific directories when process.platform === "win32" by adding paths derived from environment variables (e.g., process.env.USERPROFILE and process.env.LOCALAPPDATA) such as "%USERPROFILE%\\AppData\\Local\\Programs" and "%USERPROFILE%\\AppData\\Local\\Microsoft\\WindowsApps"; implement this by branching on process.platform === "win32" and appending those Windows paths to the COMMON_BIN_DIRS array (referencing COMMON_BIN_DIRS and HOME_DIR) so Claude CLI installs on Windows are discovered.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (2)
2476-2476: UseWORK_LOG_CARD_CLASSconstant for consistency.The inline styles duplicate the
WORK_LOG_CARD_CLASSconstant defined at line 180-181. Using the constant improves maintainability.♻️ Suggested fix
- <div className={cn("rounded-lg border border-white/[0.06] bg-[`#111317`]/70")}> + <div className={cn("rounded-lg", WORK_LOG_CARD_CLASS)}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` at line 2476, The div in AgentChatMessageList.tsx uses an inline className string ("rounded-lg border border-white/[0.06] bg-[`#111317`]/70") that duplicates the WORK_LOG_CARD_CLASS constant; replace that inline string with the WORK_LOG_CARD_CLASS constant (import/use the constant where the div is rendered) so the component uses WORK_LOG_CARD_CLASS consistently (locate the div in the AgentChatMessageList component and swap the inline className passed to cn(...) for WORK_LOG_CARD_CLASS).
276-288: Missing cleanup for timeout can cause state update on unmounted component.If the component unmounts before the 1500ms timeout fires, React may warn about a state update on an unmounted component. Consider using a ref to track mounted state or returning a cleanup function.
♻️ Suggested fix using a ref
function MessageCopyButton({ value, className, }: { value: string; className?: string; }) { const [copied, setCopied] = useState(false); + const mountedRef = useRef(true); + + useEffect(() => { + return () => { + mountedRef.current = false; + }; + }, []); const handleCopy = useCallback(() => { if (typeof navigator === "undefined" || !navigator.clipboard?.writeText) return; void navigator.clipboard.writeText(value) .then(() => { - setCopied(true); - window.setTimeout(() => setCopied(false), 1_500); + if (mountedRef.current) { + setCopied(true); + window.setTimeout(() => { + if (mountedRef.current) setCopied(false); + }, 1_500); + } }) .catch(() => { - setCopied(false); + if (mountedRef.current) setCopied(false); }); }, [value]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around lines 276 - 288, The handleCopy handler sets a 1_500ms timeout but doesn't clear it on unmount, risking setCopied on an unmounted component; fix by creating a mutable ref (e.g., copyTimeoutRef) to store the timeout id inside AgentChatMessageList, assign the timeout id when calling window.setTimeout, and clear that timeout in a useEffect cleanup (and before creating a new timeout if any) so setCopied(false) is never called after unmount; reference the existing copied/setCopied state and handleCopy callback when implementing the ref and cleanup.apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx (1)
49-90: Expose the new tab UI as tabs to assistive tech.Right now this is announced as a row of generic buttons plus an unlabeled panel, so the selected integration/panel relationship is invisible to screen readers. Please wire this up as a real
tablist/tab/tabpanelpattern and verify it with a quick keyboard + screen-reader smoke test.As per coding guidelines,
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx` around lines 49 - 90, The tab UI is currently plain buttons and an unlabeled panel so screen readers cannot associate tabs and panels; update the TABS rendering and panel markup to implement the ARIA tab pattern: wrap the buttons container with role="tablist", render each tab button (mapped from TABS in the same loop that calls activateTab) with role="tab", a stable id (e.g., `tab-${tab.id}`), aria-selected={activeTab===tab.id}, aria-controls={`settings-${tab.id}`}, and manage keyboard focus/activation (Left/Right/Home/End and Enter/Space) in the activateTab handler or a new onKeyDown that calls activateTab; for the content container use role="tabpanel", id={`settings-${activeTab}`}, and aria-labelledby={`tab-${activeTab}`} and only render the matching section components (GitHubSection, LinearSection, ExternalMcpSection, ComputerUseSection) inside that panel. Ensure tabIndex is 0 for the selected tab and -1 for others so keyboard navigation works.apps/desktop/src/renderer/state/appStore.ts (1)
226-244: Skip no-op writes insetLaneWorkViewState.The new hook already returns
prevon its reconciliation no-op paths, but this setter still cloneslaneWorkViewByScopeand republishes the same entry. That turns “no change” paths into store updates and rerenders every subscriber to the scoped map.♻️ Proposed fix
setLaneWorkViewState: (projectRoot, laneId, next) => { const key = normalizeLaneWorkScopeKey(projectRoot, laneId); if (!key) return; set((prev) => { const current = prev.laneWorkViewByScope[key] ?? createDefaultWorkProjectViewState(); const updated = typeof next === "function" ? next(current) : { ...current, ...next, }; + if (updated === current) { + return prev; + } return { laneWorkViewByScope: { ...prev.laneWorkViewByScope, [key]: updated, }, }; }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/state/appStore.ts` around lines 226 - 244, The setter setLaneWorkViewState currently always clones laneWorkViewByScope even when no changes occur; update it so after computing updated (in setLaneWorkViewState) you compare updated to current (first by reference equality and then by shallow/deep equality as appropriate) and if they are equal return prev instead of returning a new object; use normalizeLaneWorkScopeKey and createDefaultWorkProjectViewState to find current, and avoid updating laneWorkViewByScope when there is no actual change to prevent needless re-publishes and rerenders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/memory/knowledgeCaptureService.ts`:
- Around line 179-210: The low-signal filter in isLowSignalPrFeedbackContent can
misclassify actionable short phrases and durable-rule comments because the
durable-signal check (hasDurablePrFeedbackSignals) runs only at the end and the
URL fast-path can short-circuit; update isLowSignalPrFeedbackContent so you call
hasDurablePrFeedbackSignals(trimmed) early and immediately return false (not
low-signal) if it finds durable signals, and change the URL fast-path to first
check durable signals so lines that contain both a durable phrase and a URL
(e.g., "Always validate input https://...") are preserved; keep references to
hasDurablePrFeedbackSignals, isLowSignalPrFeedbackContent, inferCaptureCategory,
and recordCandidate so you can locate and validate the behavior after the
change.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Line 360: The modal now only displays snapshot state (activeBackend,
snapshot.computerUsePolicy) but no longer allows editing computerUsePolicy and
the "Open proof drawer" CTA can close the modal when proof toggling is
unavailable; in AgentChatComposer, either restore a settings action by making
computerUsePolicy editable (add a control bound to snapshot.computerUsePolicy
and call the existing settings update handler when changed) or explicitly treat
the UI as read-only by changing labels to "Status" and disabling/hiding the
proof CTA when onToggleProof/openProofDrawer (or its callback) is undefined;
ensure references include activeBackend, snapshot.computerUsePolicy,
AgentChatComposer, openProofDrawer/onToggleProof so reviewers can locate and
adjust the appropriate JSX and handlers.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 3222-3249: The animate-[fadeSlideIn_200ms_ease-out] utility used
by stickyStreamingBanner is a custom animation not defined in Tailwind; add an
entry to the tailwind config's extend.animation (e.g., fadeSlideIn: "fadeSlideIn
200ms ease-out forwards") and corresponding keyframes under extend.keyframes
(from { opacity: 0, transform: "translateY(-10px)" } to { opacity: 1, transform:
"translateY(0)" }) so the <div> using animate-[fadeSlideIn_200ms_ease-out]
actually animates.
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts`:
- Around line 82-118: refresh can apply stale lane/project state because it only
depends on laneId and writes state unguarded; update the implementation to
compute a scopeKey at request time (e.g., combine laneId and projectRoot) and
store it in latestScopeKeyRef, then before calling setSessions, setLoading, or
touching hasLoadedOnceRef validate that latestScopeKeyRef.current ===
requestScopeKey; when queuing retries, push the queued payload into
refreshQueuedRef but replay via latestRefreshRef.current() (a ref that always
holds the latest refresh function) instead of calling the closed-over refresh,
and add scopeKey (or projectRoot) to the refresh dependencies so the closure
updates when project changes; ensure refreshInFlightRef and refreshQueuedRef
behavior remains the same but guarded by the scope check so late responses don’t
mutate state for a different lane.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 597-601: The Checks tab badge is computed from raw checks +
actionRuns jobs but the UI list uses the deduplicated/merged output from
buildUnifiedChecks; compute unifiedChecks = buildUnifiedChecks(checks,
actionRuns) before DETAIL_TABS and replace the "checks" tab count expression
(currently checks.length + actionRuns.reduce(...)) with unifiedChecks.length so
the badge matches the displayed list; update references to use unifiedChecks
where appropriate (e.g., in DETAIL_TABS and any nearby logic that expects the
unified list).
In
`@apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx`:
- Around line 25-34: The useEffect currently returns early for blank or default
params, leaving the URL and rendered tab out of sync; update the effect in
IntegrationsSettingsSection so it only returns when integrationParam ===
canonicalIntegration and otherwise always set the "integration" search param to
canonicalIntegration (using resolveIntegrationTab, integrationParam,
canonicalIntegration, searchParams, setSearchParams) so blank, "github", or
unknown values are normalized in the URL to the canonical tab.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 252-287: refreshLanes can suffer from race conditions where an
earlier request returns after a later one and prunes laneWorkViewByScope based
on stale lanes; introduce a monotonically-incrementing request counter (or
timestamp) attached to the store (e.g., lastLanesRefreshId) and capture a local
refreshId at the start of refreshLanes, incrementing the shared counter before
awaiting window.ade.lanes.list; after the await, compare the captured refreshId
with the current lastLanesRefreshId and abort if it’s not the latest, then
proceed to compute projectKey, nextSelected/nextRunLane and call set — reference
the refreshLanes function, requestedProjectKey/projectKey checks, and the
laneWorkViewByScope/laneInspectorTabs updates so you only apply the latest
response.
In `@apps/desktop/src/shared/types/computerUseArtifacts.ts`:
- Around line 301-303: The helper isComputerUseModeEnabled was replaced with an
unconditional true, causing all checks (including localComputerUseAllowed
derived in workflowTools.ts) to treat "auto" or disabled modes as enabled;
restore proper mode checking by implementing isComputerUseModeEnabled(mode:
ComputerUsePolicyMode | null | undefined): boolean to return true only when mode
explicitly permits local computer use (handle "on"/"enabled" states and treat
null/undefined/"off"/"disabled"/"auto" appropriately per the
ComputerUsePolicyMode enum semantics), and ensure callers like the code that
computes localComputerUseAllowed still call this function to enforce policy
rather than always allowing use.
---
Outside diff comments:
In `@apps/desktop/src/main/services/memory/knowledgeCaptureService.ts`:
- Around line 222-248: toEpisodeContent() must sanitize any user-derived fields
so they cannot contain the reserved sentinel "<!--episode:" before appending the
structured base64 block; update toEpisodeContent to scan args.sourceLabel,
args.summary, each string in args.patterns, args.gotchas, args.decisions (and
args.fileScopePattern if present) and escape or encode any instances of the
exact substring "<!--episode:" (for example by inserting an escape character or
substituting a safe representation) so visible text cannot produce a fake
sentinel; keep the existing episode object and the final lines.push of the
base64 comment intact so parseEpisode() (which reads the first
<!--episode:...-->) always finds the real payload.
---
Duplicate comments:
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 1896-1920: The summary currently counts neutral and cancelled
checks as "skipped" which mislabels results; update the calculation in
PrDetailPane so that skipped only includes checks with conclusion === "skipped"
and add separate counters for neutral and cancelled (e.g., neutralCount and
cancelledCount) or else change the label to a broader term; then update the
summaryText logic (references: unifiedChecks, passing, failing, pending,
skipped, summaryText, toggleExpand) to use the new counters or label so
neutral/cancelled are not reported as skipped.
- Around line 1797-1848: When deduping Actions jobs we need to reintroduce a
one-time bare-name fallback for unique job names to avoid duplicate entries;
compute a frequency map of job.name (case-insensitive) across dedupedRuns (use
symbols latestRunByWorkflow/dedupedRuns/actionRuns) before building
coveredNames, then when iterating runs/jobs and adding keys to coveredNames (in
the loop that pushes items), also add job.name.toLowerCase() only if the
frequency for that bare job name equals 1; keep the existing composite keys
(`${run.name}/${job.name}`) and canonical `"${run.name} / ${job.name}"` handling
unchanged so non-unique bare names are not added.
---
Nitpick comments:
In `@apps/desktop/src/main/services/ai/claudeCodeExecutable.ts`:
- Around line 12-21: COMMON_BIN_DIRS currently lists only Unix-style locations;
update the initialization to include Windows-specific directories when
process.platform === "win32" by adding paths derived from environment variables
(e.g., process.env.USERPROFILE and process.env.LOCALAPPDATA) such as
"%USERPROFILE%\\AppData\\Local\\Programs" and
"%USERPROFILE%\\AppData\\Local\\Microsoft\\WindowsApps"; implement this by
branching on process.platform === "win32" and appending those Windows paths to
the COMMON_BIN_DIRS array (referencing COMMON_BIN_DIRS and HOME_DIR) so Claude
CLI installs on Windows are discovered.
In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts`:
- Line 30: The module-level variable runtimeRootCache currently persists across
workspace switches; update the probe cache reset logic so runtimeRootCache is
cleared when resetClaudeRuntimeProbeCache() is called (or refactor
runtimeRootCache to be project-aware keyed by workspace/project id). Locate
runtimeRootCache and the resetClaudeRuntimeProbeCache() function and ensure the
reset path explicitly sets runtimeRootCache = null (or removes the entry for the
current project) so the cached runtime root cannot leak between workspaces;
apply the same change for any other cache variables referenced in the probe
(lines around the probe's main functions referenced in the diff).
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Line 2476: The div in AgentChatMessageList.tsx uses an inline className string
("rounded-lg border border-white/[0.06] bg-[`#111317`]/70") that duplicates the
WORK_LOG_CARD_CLASS constant; replace that inline string with the
WORK_LOG_CARD_CLASS constant (import/use the constant where the div is rendered)
so the component uses WORK_LOG_CARD_CLASS consistently (locate the div in the
AgentChatMessageList component and swap the inline className passed to cn(...)
for WORK_LOG_CARD_CLASS).
- Around line 276-288: The handleCopy handler sets a 1_500ms timeout but doesn't
clear it on unmount, risking setCopied on an unmounted component; fix by
creating a mutable ref (e.g., copyTimeoutRef) to store the timeout id inside
AgentChatMessageList, assign the timeout id when calling window.setTimeout, and
clear that timeout in a useEffect cleanup (and before creating a new timeout if
any) so setCopied(false) is never called after unmount; reference the existing
copied/setCopied state and handleCopy callback when implementing the ref and
cleanup.
In
`@apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx`:
- Around line 49-90: The tab UI is currently plain buttons and an unlabeled
panel so screen readers cannot associate tabs and panels; update the TABS
rendering and panel markup to implement the ARIA tab pattern: wrap the buttons
container with role="tablist", render each tab button (mapped from TABS in the
same loop that calls activateTab) with role="tab", a stable id (e.g.,
`tab-${tab.id}`), aria-selected={activeTab===tab.id},
aria-controls={`settings-${tab.id}`}, and manage keyboard focus/activation
(Left/Right/Home/End and Enter/Space) in the activateTab handler or a new
onKeyDown that calls activateTab; for the content container use role="tabpanel",
id={`settings-${activeTab}`}, and aria-labelledby={`tab-${activeTab}`} and only
render the matching section components (GitHubSection, LinearSection,
ExternalMcpSection, ComputerUseSection) inside that panel. Ensure tabIndex is 0
for the selected tab and -1 for others so keyboard navigation works.
In `@apps/desktop/src/renderer/components/settings/MemoryHealthTab.tsx`:
- Around line 1181-1194: The bulk buttons currently fire
handlePromote/handleArchive for every candidateEntry in parallel (using void),
causing multiple concurrent loadDashboard calls and lost errors; change the
click handlers to iterate sequentially (for..of with await) over
candidateEntries calling handlePromote(id) / handleArchive(id), collect any
thrown errors into an array, and after the loop call loadDashboard({ quiet: true
}) once and surface aggregated errors (e.g., count/message) to the UI so partial
failures are not silently ignored; reference handlePromote, handleArchive,
loadDashboard, and candidateEntries when making the change.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 226-244: The setter setLaneWorkViewState currently always clones
laneWorkViewByScope even when no changes occur; update it so after computing
updated (in setLaneWorkViewState) you compare updated to current (first by
reference equality and then by shallow/deep equality as appropriate) and if they
are equal return prev instead of returning a new object; use
normalizeLaneWorkScopeKey and createDefaultWorkProjectViewState to find current,
and avoid updating laneWorkViewByScope when there is no actual change to prevent
needless re-publishes and rerenders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9acff035-5617-4d43-8aad-d57dd40bbb5f
📒 Files selected for processing (17)
apps/desktop/src/main/services/ai/claudeCodeExecutable.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/memory/episodeFormat.tsapps/desktop/src/main/services/memory/episodicSummaryService.tsapps/desktop/src/main/services/memory/knowledgeCaptureService.tsapps/desktop/src/main/services/memory/proceduralLearningService.test.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/prFormatters.tsapps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsxapps/desktop/src/renderer/components/settings/MemoryHealthTab.tsxapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/types/computerUseArtifacts.tsapps/ios/ADE/Resources/DatabaseBootstrap.sql
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/components/prs/shared/prFormatters.ts
- apps/desktop/src/main/services/memory/episodeFormat.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/services/memory/episodicSummaryService.ts
- apps/ios/ADE/Resources/DatabaseBootstrap.sql
- apps/desktop/src/main/services/memory/proceduralLearningService.test.ts
- apps/desktop/src/main/services/chat/agentChatService.ts
Comment on lines
+179
to
+210
| function hasDurablePrFeedbackSignals(value: string): boolean { | ||
| const normalized = normalizeText(value); | ||
| if (!normalized) return false; | ||
| return ( | ||
| normalized.includes("always ") | ||
| || normalized.includes("never ") | ||
| || normalized.includes("must ") | ||
| || normalized.includes("should ") | ||
| || normalized.includes("prefer ") | ||
| || normalized.includes("unless ") | ||
| || normalized.includes("before ") | ||
| || normalized.includes("after ") | ||
| || normalized.includes("break") | ||
| || normalized.includes("regression") | ||
| || normalized.includes("coverage") | ||
| || normalized.includes("fallback") | ||
| || normalized.includes("validation") | ||
| ); | ||
| } | ||
|
|
||
| function isLowSignalPrFeedbackContent(value: string): boolean { | ||
| const trimmed = cleanText(value); | ||
| if (!trimmed.length) return true; | ||
| if (GENERIC_PR_FEEDBACK_PATTERNS.some((pattern) => pattern.test(trimmed))) return true; | ||
| if (DERIVABLE_PR_FEEDBACK_PATTERNS.some((pattern) => pattern.test(trimmed))) return true; | ||
|
|
||
| const withoutUrls = trimmed.replace(/https?:\/\/\S+/gi, " ").replace(/\s+/g, " ").trim(); | ||
| const wordCount = withoutUrls.split(/\s+/).filter(Boolean).length; | ||
| if (/https?:\/\//i.test(trimmed) && wordCount <= 8) return true; | ||
|
|
||
| return !hasDurablePrFeedbackSignals(trimmed) && wordCount < 7; | ||
| } |
There was a problem hiding this comment.
Don’t let the low-signal filter outrank your own actionability rules.
inferCaptureCategory() already treats short phrases like Do not mutate props or For migrations, add rollback as actionable, but this helper still classifies them as low-signal. The URL fast-path on Lines 205-207 also drops durable comments like Always validate input https://..., so valid PR guidance never reaches recordCandidate().
💡 Proposed fix
function hasDurablePrFeedbackSignals(value: string): boolean {
const normalized = normalizeText(value);
if (!normalized) return false;
+ if (inferCaptureCategory(value)) return true;
return (
- normalized.includes("always ")
- || normalized.includes("never ")
- || normalized.includes("must ")
- || normalized.includes("should ")
- || normalized.includes("prefer ")
- || normalized.includes("unless ")
- || normalized.includes("before ")
- || normalized.includes("after ")
- || normalized.includes("break")
- || normalized.includes("regression")
- || normalized.includes("coverage")
- || normalized.includes("fallback")
- || normalized.includes("validation")
+ normalized.includes("unless ")
+ || normalized.includes("coverage")
+ || normalized.includes("fallback")
+ || normalized.includes("validation")
);
}
function isLowSignalPrFeedbackContent(value: string): boolean {
const trimmed = cleanText(value);
if (!trimmed.length) return true;
+ const hasDurableSignals = hasDurablePrFeedbackSignals(trimmed);
if (GENERIC_PR_FEEDBACK_PATTERNS.some((pattern) => pattern.test(trimmed))) return true;
if (DERIVABLE_PR_FEEDBACK_PATTERNS.some((pattern) => pattern.test(trimmed))) return true;
const withoutUrls = trimmed.replace(/https?:\/\/\S+/gi, " ").replace(/\s+/g, " ").trim();
const wordCount = withoutUrls.split(/\s+/).filter(Boolean).length;
- if (/https?:\/\//i.test(trimmed) && wordCount <= 8) return true;
+ if (/https?:\/\//i.test(trimmed) && wordCount <= 8 && !hasDurableSignals) return true;
- return !hasDurablePrFeedbackSignals(trimmed) && wordCount < 7;
+ return !hasDurableSignals && wordCount < 7;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/memory/knowledgeCaptureService.ts` around
lines 179 - 210, The low-signal filter in isLowSignalPrFeedbackContent can
misclassify actionable short phrases and durable-rule comments because the
durable-signal check (hasDurablePrFeedbackSignals) runs only at the end and the
URL fast-path can short-circuit; update isLowSignalPrFeedbackContent so you call
hasDurablePrFeedbackSignals(trimmed) early and immediately return false (not
low-signal) if it finds durable signals, and change the URL fast-path to first
check durable signals so lines that contain both a durable phrase and a URL
(e.g., "Always validate input https://...") are preserved; keep references to
hasDurablePrFeedbackSignals, isLowSignalPrFeedbackContent, inferCaptureCategory,
and recordCandidate so you can locate and validate the behavior after the
change.
|
|
||
| const computerUseAllowed = policy.mode !== "off"; | ||
| const activeBackend = snapshot?.activeBackend?.name ?? (policy.allowLocalFallback ? "Fallback allowed" : "No fallback"); | ||
| const activeBackend = snapshot?.activeBackend?.name ?? "None"; |
There was a problem hiding this comment.
This no longer behaves like a settings modal.
The advanced popover still routes users here for "Computer use details", but after this change the modal only shows snapshot state and an "Open proof drawer" CTA. There is no way to change computerUsePolicy from this flow anymore, and when proof toggling is unavailable that CTA just closes the modal because the parent guards the toggle with an optional callback. Either restore a real settings action here, or relabel this as a read-only status dialog and gate the proof CTA on availability.
Also applies to: 370-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` at line 360,
The modal now only displays snapshot state (activeBackend,
snapshot.computerUsePolicy) but no longer allows editing computerUsePolicy and
the "Open proof drawer" CTA can close the modal when proof toggling is
unavailable; in AgentChatComposer, either restore a settings action by making
computerUsePolicy editable (add a control bound to snapshot.computerUsePolicy
and call the existing settings update handler when changed) or explicitly treat
the UI as read-only by changing labels to "Status" and disabling/hiding the
proof CTA when onToggleProof/openProofDrawer (or its callback) is undefined;
ensure references include activeBackend, snapshot.computerUsePolicy,
AgentChatComposer, openProofDrawer/onToggleProof so reviewers can locate and
adjust the appropriate JSX and handlers.
Comment on lines
+3222
to
+3249
| const stickyStreamingBanner = showStreamingIndicator && activeTurnId ? ( | ||
| <div className="sticky top-3 z-20 mb-4 flex animate-[fadeSlideIn_200ms_ease-out] justify-center px-1"> | ||
| <div | ||
| className="flex w-full max-w-[32rem] items-center gap-3 rounded-[14px] border border-sky-400/18 bg-[#10161d]/96 px-3.5 py-2.5 shadow-[0_12px_40px_rgba(3,7,18,0.35)] backdrop-blur transition-all duration-200" | ||
| > | ||
| <span className="inline-flex items-center gap-1 text-sky-200/85"> | ||
| <ThinkingDots toneClass="bg-sky-300/75" /> | ||
| </span> | ||
| <div className="min-w-0 flex-1"> | ||
| <div className="truncate font-sans text-[12px] font-medium text-sky-100/90"> | ||
| {latestActivity?.detail ? replaceInternalToolNames(latestActivity.detail) : "Still working"} | ||
| </div> | ||
| <div className="truncate font-mono text-[10px] uppercase tracking-[0.16em] text-sky-200/55"> | ||
| {activeElapsedLabel ? `Working for ${activeElapsedLabel}` : "Turn running"} | ||
| </div> | ||
| </div> | ||
| {!stickToBottom ? ( | ||
| <button | ||
| type="button" | ||
| className="rounded-full border border-sky-300/18 bg-sky-400/[0.08] px-2.5 py-1 font-sans text-[10px] font-medium text-sky-100 transition-colors hover:bg-sky-400/[0.14]" | ||
| onClick={jumpToLiveOutput} | ||
| > | ||
| Jump to live output | ||
| </button> | ||
| ) : null} | ||
| </div> | ||
| </div> | ||
| ) : null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for fadeSlideIn animation definition
rg -n "fadeSlideIn" --type=css
rg -n "fadeSlideIn" tailwind.config.*Repository: arul28/ADE
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Find tailwind config files
echo "=== Searching for tailwind config files ==="
fd -e "js" -e "ts" -e "cjs" -e "mjs" "tailwind.config"
# Search for fadeSlideIn in all files (not just CSS)
echo -e "\n=== Searching for fadeSlideIn in all files ==="
rg "fadeSlideIn" -t js -t ts -t css -t json --max-count 20
# Also check if there's a custom CSS/animation setup
echo -e "\n=== Checking for global CSS or animation definitions ==="
fd -e "css" -e "scss" "global" | head -20Repository: arul28/ADE
Length of output: 471
🏁 Script executed:
#!/bin/bash
cat -n apps/desktop/tailwind.config.cjsRepository: arul28/ADE
Length of output: 1741
Define the fadeSlideIn animation in your Tailwind config.
The animate-[fadeSlideIn_200ms_ease-out] class references a custom animation that is not defined in apps/desktop/tailwind.config.cjs. Without this animation definition, the banner will not animate as intended. Add the animation to the Tailwind config's extend section:
animation: {
fadeSlideIn: "fadeSlideIn 200ms ease-out forwards"
}And define the keyframes (either in the same config section or in global CSS):
keyframes: {
fadeSlideIn: {
from: { opacity: "0", transform: "translateY(-10px)" },
to: { opacity: "1", transform: "translateY(0)" }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` around
lines 3222 - 3249, The animate-[fadeSlideIn_200ms_ease-out] utility used by
stickyStreamingBanner is a custom animation not defined in Tailwind; add an
entry to the tailwind config's extend.animation (e.g., fadeSlideIn: "fadeSlideIn
200ms ease-out forwards") and corresponding keyframes under extend.keyframes
(from { opacity: 0, transform: "translateY(-10px)" } to { opacity: 1, transform:
"translateY(0)" }) so the <div> using animate-[fadeSlideIn_200ms_ease-out]
actually animates.
Comment on lines
+82
to
+118
| const refresh = useCallback( | ||
| async (options: { showLoading?: boolean; force?: boolean } = {}) => { | ||
| if (!laneId) { | ||
| setSessions([]); | ||
| hasLoadedOnceRef.current = true; | ||
| return; | ||
| } | ||
| const showLoading = options.showLoading ?? true; | ||
| if (refreshInFlightRef.current) { | ||
| refreshQueuedRef.current = { | ||
| showLoading: (refreshQueuedRef.current?.showLoading ?? false) || showLoading, | ||
| force: (refreshQueuedRef.current?.force ?? false) || Boolean(options.force), | ||
| }; | ||
| return; | ||
| } | ||
| refreshInFlightRef.current = true; | ||
| if (showLoading) setLoading(true); | ||
| try { | ||
| const rows = await listSessionsCached( | ||
| { laneId, limit: 200 }, | ||
| options.force ? { force: true } : undefined, | ||
| ); | ||
| setSessions(rows.filter((session) => !isRunOwnedSession(session))); | ||
| hasLoadedOnceRef.current = true; | ||
| } catch (err) { | ||
| console.warn("[useLaneWorkSessions] Failed to refresh sessions:", err); | ||
| } finally { | ||
| if (showLoading) setLoading(false); | ||
| refreshInFlightRef.current = false; | ||
| const queued = refreshQueuedRef.current; | ||
| refreshQueuedRef.current = null; | ||
| if (queued) { | ||
| void refresh(queued); | ||
| } | ||
| } | ||
| }, | ||
| [laneId], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsRepository: arul28/ADE
Length of output: 16348
Stale lane refresh can populate wrong lane's session list; add scope guards before state writes.
The issue is real: when Lane A is still loading and the user switches to Lane B, Lane A's late completion can call setSessions() via the stale refresh closure (which captures laneId=A). The queued retry in finally (line 114) also uses the stale closure.
The root causes:
refreshdependency is[laneId]only; it does not includeprojectRoot, so scope changes from project switches are missed.- No guards prevent
setSessionsorsetLoadingfrom executing after the scope has changed. - Queued retries via
refresh(queued)use the stale closure instead of the current callback.
The suggested fix correctly:
- Captures scope identity at request time via
latestScopeKeyRef - Guards
setSessions,setLoading, and queued retries against stale scopes - Uses
latestRefreshRefto replay queued work with the current callback - Adds
scopeKeyto dependencies, which covers bothlaneIdandprojectRootchanges
🧭 Suggested guard
const refreshInFlightRef = useRef(false);
const refreshQueuedRef = useRef<{ showLoading: boolean; force: boolean } | null>(null);
const backgroundRefreshTimerRef = useRef<number | null>(null);
const hasActiveSessionsRef = useRef(false);
const hasLoadedOnceRef = useRef(false);
+ const latestScopeKeyRef = useRef(scopeKey);
+ latestScopeKeyRef.current = scopeKey;
+ const latestRefreshRef =
+ useRef<(options?: { showLoading?: boolean; force?: boolean }) => Promise<void>>(async () => {});
const refresh = useCallback(
async (options: { showLoading?: boolean; force?: boolean } = {}) => {
+ const requestScopeKey = scopeKey;
if (!laneId) {
setSessions([]);
hasLoadedOnceRef.current = true;
return;
}
@@
const rows = await listSessionsCached(
{ laneId, limit: 200 },
options.force ? { force: true } : undefined,
);
+ if (latestScopeKeyRef.current !== requestScopeKey) return;
setSessions(rows.filter((session) => !isRunOwnedSession(session)));
hasLoadedOnceRef.current = true;
} catch (err) {
console.warn("[useLaneWorkSessions] Failed to refresh sessions:", err);
} finally {
- if (showLoading) setLoading(false);
+ if (showLoading && latestScopeKeyRef.current === requestScopeKey) setLoading(false);
refreshInFlightRef.current = false;
const queued = refreshQueuedRef.current;
refreshQueuedRef.current = null;
if (queued) {
- void refresh(queued);
+ void latestRefreshRef.current(queued);
}
}
},
- [laneId],
+ [laneId, scopeKey],
);
+ latestRefreshRef.current = refresh;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts` around
lines 82 - 118, refresh can apply stale lane/project state because it only
depends on laneId and writes state unguarded; update the implementation to
compute a scopeKey at request time (e.g., combine laneId and projectRoot) and
store it in latestScopeKeyRef, then before calling setSessions, setLoading, or
touching hasLoadedOnceRef validate that latestScopeKeyRef.current ===
requestScopeKey; when queuing retries, push the queued payload into
refreshQueuedRef but replay via latestRefreshRef.current() (a ref that always
holds the latest refresh function) instead of calling the closed-over refresh,
and add scopeKey (or projectRoot) to the refresh dependencies so the closure
updates when project changes; ensure refreshInFlightRef and refreshQueuedRef
behavior remains the same but guarded by the scope check so late responses don’t
mutate state for a different lane.
Comment on lines
597
to
601
| const DETAIL_TABS: Array<{ id: DetailTab; label: string; icon: React.ElementType; count?: number }> = [ | ||
| { id: "overview", label: "Overview", icon: Eye }, | ||
| { id: "files", label: "Files", icon: Code, count: files.length }, | ||
| { id: "checks", label: "CI / Checks", icon: Play, count: checks.length }, | ||
| { id: "checks", label: "CI / Checks", icon: Play, count: checks.length + actionRuns.reduce((sum, run) => sum + run.jobs.length, 0) }, | ||
| { id: "activity", label: "Activity", icon: ClockCounterClockwise, count: activity.length > 0 ? activity.length : (comments.length + reviews.length) }, |
There was a problem hiding this comment.
Keep the tab badge count aligned with the unified checks list.
This badge still sums raw checks plus every job from every actionRun, but the Checks tab later collapses reruns and drops duplicate check/job pairs in buildUnifiedChecks. That makes the badge larger than the list whenever a workflow was rerun or a check is mirrored by an Actions job.
Proposed fix
+ const unifiedCheckCount = React.useMemo(
+ () => buildUnifiedChecks(checks, actionRuns).length,
+ [checks, actionRuns],
+ );
+
const DETAIL_TABS: Array<{ id: DetailTab; label: string; icon: React.ElementType; count?: number }> = [
{ id: "overview", label: "Overview", icon: Eye },
{ id: "files", label: "Files", icon: Code, count: files.length },
- { id: "checks", label: "CI / Checks", icon: Play, count: checks.length + actionRuns.reduce((sum, run) => sum + run.jobs.length, 0) },
+ { id: "checks", label: "CI / Checks", icon: Play, count: unifiedCheckCount },
{ id: "activity", label: "Activity", icon: ClockCounterClockwise, count: activity.length > 0 ? activity.length : (comments.length + reviews.length) },
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const DETAIL_TABS: Array<{ id: DetailTab; label: string; icon: React.ElementType; count?: number }> = [ | |
| { id: "overview", label: "Overview", icon: Eye }, | |
| { id: "files", label: "Files", icon: Code, count: files.length }, | |
| { id: "checks", label: "CI / Checks", icon: Play, count: checks.length }, | |
| { id: "checks", label: "CI / Checks", icon: Play, count: checks.length + actionRuns.reduce((sum, run) => sum + run.jobs.length, 0) }, | |
| { id: "activity", label: "Activity", icon: ClockCounterClockwise, count: activity.length > 0 ? activity.length : (comments.length + reviews.length) }, | |
| const unifiedCheckCount = React.useMemo( | |
| () => buildUnifiedChecks(checks, actionRuns).length, | |
| [checks, actionRuns], | |
| ); | |
| const DETAIL_TABS: Array<{ id: DetailTab; label: string; icon: React.ElementType; count?: number }> = [ | |
| { id: "overview", label: "Overview", icon: Eye }, | |
| { id: "files", label: "Files", icon: Code, count: files.length }, | |
| { id: "checks", label: "CI / Checks", icon: Play, count: unifiedCheckCount }, | |
| { id: "activity", label: "Activity", icon: ClockCounterClockwise, count: activity.length > 0 ? activity.length : (comments.length + reviews.length) }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx` around
lines 597 - 601, The Checks tab badge is computed from raw checks + actionRuns
jobs but the UI list uses the deduplicated/merged output from
buildUnifiedChecks; compute unifiedChecks = buildUnifiedChecks(checks,
actionRuns) before DETAIL_TABS and replace the "checks" tab count expression
(currently checks.length + actionRuns.reduce(...)) with unifiedChecks.length so
the badge matches the displayed list; update references to use unifiedChecks
where appropriate (e.g., in DETAIL_TABS and any nearby logic that expects the
unified list).
Comment on lines
+25
to
+34
| const integrationParam = searchParams.get("integration")?.trim().toLowerCase() ?? ""; | ||
| const canonicalIntegration = resolveIntegrationTab(integrationParam); | ||
| const activeTab = canonicalIntegration ?? "github"; | ||
|
|
||
| useEffect(() => { | ||
| if (!integrationParam || !canonicalIntegration || integrationParam === canonicalIntegration) return; | ||
| const nextParams = new URLSearchParams(searchParams); | ||
| nextParams.set("integration", canonicalIntegration); | ||
| setSearchParams(nextParams, { replace: true }); | ||
| }, [canonicalIntegration, integrationParam, searchParams, setSearchParams]); |
There was a problem hiding this comment.
Normalize blank/invalid/default integration params too.
Line 27 already falls back to the GitHub tab, but Line 30 returns before cleaning up ?integration=, ?integration=github, or unknown values. That leaves the rendered tab and the shareable URL out of sync.
🧭 Suggested fix
- const integrationParam = searchParams.get("integration")?.trim().toLowerCase() ?? "";
+ const rawIntegrationParam = searchParams.get("integration");
+ const integrationParam = rawIntegrationParam?.trim().toLowerCase() ?? "";
const canonicalIntegration = resolveIntegrationTab(integrationParam);
const activeTab = canonicalIntegration ?? "github";
useEffect(() => {
- if (!integrationParam || !canonicalIntegration || integrationParam === canonicalIntegration) return;
+ if (rawIntegrationParam == null) return;
const nextParams = new URLSearchParams(searchParams);
- nextParams.set("integration", canonicalIntegration);
+
+ if (!canonicalIntegration || canonicalIntegration === "github") {
+ nextParams.delete("integration");
+ } else if (rawIntegrationParam !== canonicalIntegration) {
+ nextParams.set("integration", canonicalIntegration);
+ } else {
+ return;
+ }
+
setSearchParams(nextParams, { replace: true });
- }, [canonicalIntegration, integrationParam, searchParams, setSearchParams]);
+ }, [canonicalIntegration, rawIntegrationParam, searchParams, setSearchParams]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const integrationParam = searchParams.get("integration")?.trim().toLowerCase() ?? ""; | |
| const canonicalIntegration = resolveIntegrationTab(integrationParam); | |
| const activeTab = canonicalIntegration ?? "github"; | |
| useEffect(() => { | |
| if (!integrationParam || !canonicalIntegration || integrationParam === canonicalIntegration) return; | |
| const nextParams = new URLSearchParams(searchParams); | |
| nextParams.set("integration", canonicalIntegration); | |
| setSearchParams(nextParams, { replace: true }); | |
| }, [canonicalIntegration, integrationParam, searchParams, setSearchParams]); | |
| const rawIntegrationParam = searchParams.get("integration"); | |
| const integrationParam = rawIntegrationParam?.trim().toLowerCase() ?? ""; | |
| const canonicalIntegration = resolveIntegrationTab(integrationParam); | |
| const activeTab = canonicalIntegration ?? "github"; | |
| useEffect(() => { | |
| if (rawIntegrationParam == null) return; | |
| const nextParams = new URLSearchParams(searchParams); | |
| if (!canonicalIntegration || canonicalIntegration === "github") { | |
| nextParams.delete("integration"); | |
| } else if (rawIntegrationParam !== canonicalIntegration) { | |
| nextParams.set("integration", canonicalIntegration); | |
| } else { | |
| return; | |
| } | |
| setSearchParams(nextParams, { replace: true }); | |
| }, [canonicalIntegration, rawIntegrationParam, searchParams, setSearchParams]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/settings/IntegrationsSettingsSection.tsx`
around lines 25 - 34, The useEffect currently returns early for blank or default
params, leaving the URL and rendered tab out of sync; update the effect in
IntegrationsSettingsSection so it only returns when integrationParam ===
canonicalIntegration and otherwise always set the "integration" search param to
canonicalIntegration (using resolveIntegrationTab, integrationParam,
canonicalIntegration, searchParams, setSearchParams) so blank, "github", or
unknown values are normalized in the URL to the canonical tab.
Comment on lines
252
to
+287
| refreshLanes: async (options) => { | ||
| const requestedProjectKey = normalizeProjectKey(get().project?.rootPath); | ||
| const lanes = await window.ade.lanes.list({ | ||
| includeArchived: false, | ||
| includeStatus: options?.includeStatus ?? true, | ||
| }); | ||
| const projectKey = normalizeProjectKey(get().project?.rootPath); | ||
| if (projectKey !== requestedProjectKey) { | ||
| return; | ||
| } | ||
| const selected = get().selectedLaneId; | ||
| const runLane = get().runLaneId; | ||
| const nextSelected = selected && lanes.some((l) => l.id === selected) ? selected : lanes[0]?.id ?? null; | ||
| const nextRunLane = runLane && lanes.some((l) => l.id === runLane) ? runLane : nextSelected; | ||
| set((prev) => { | ||
| const allowed = new Set(lanes.map((lane) => lane.id)); | ||
| const nextTabs: Record<string, LaneInspectorTab> = {}; | ||
| const nextLaneWorkViews: Record<string, WorkProjectViewState> = {}; | ||
| for (const [laneId, tab] of Object.entries(prev.laneInspectorTabs)) { | ||
| if (allowed.has(laneId)) nextTabs[laneId] = tab as LaneInspectorTab; | ||
| } | ||
| return { lanes, selectedLaneId: nextSelected, runLaneId: nextRunLane, laneInspectorTabs: nextTabs }; | ||
| for (const [scopeKey, viewState] of Object.entries(prev.laneWorkViewByScope)) { | ||
| if (!projectKey || !scopeKey.startsWith(`${projectKey}::`)) { | ||
| nextLaneWorkViews[scopeKey] = viewState; | ||
| continue; | ||
| } | ||
| const laneId = scopeKey.slice(projectKey.length + 2); | ||
| if (allowed.has(laneId)) nextLaneWorkViews[scopeKey] = viewState; | ||
| } | ||
| return { | ||
| lanes, | ||
| selectedLaneId: nextSelected, | ||
| runLaneId: nextRunLane, | ||
| laneInspectorTabs: nextTabs, | ||
| laneWorkViewByScope: nextLaneWorkViews, | ||
| }; |
There was a problem hiding this comment.
Order overlapping refreshLanes() results, not just cross-project ones.
The new requestedProjectKey guard only filters project switches. This file still schedules overlapping same-project refreshes, so an older response can arrive last and prune laneWorkViewByScope against stale lane ids, dropping newer lane-scoped UI state or reverting the latest lane list.
🧭 Proposed fix
+let laneRefreshRequestSeq = 0;
+
export const useAppStore = create<AppState>((set, get) => ({
...
refreshLanes: async (options) => {
const requestedProjectKey = normalizeProjectKey(get().project?.rootPath);
+ const requestSeq = ++laneRefreshRequestSeq;
const lanes = await window.ade.lanes.list({
includeArchived: false,
includeStatus: options?.includeStatus ?? true,
});
const projectKey = normalizeProjectKey(get().project?.rootPath);
- if (projectKey !== requestedProjectKey) {
+ if (projectKey !== requestedProjectKey || requestSeq !== laneRefreshRequestSeq) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| refreshLanes: async (options) => { | |
| const requestedProjectKey = normalizeProjectKey(get().project?.rootPath); | |
| const lanes = await window.ade.lanes.list({ | |
| includeArchived: false, | |
| includeStatus: options?.includeStatus ?? true, | |
| }); | |
| const projectKey = normalizeProjectKey(get().project?.rootPath); | |
| if (projectKey !== requestedProjectKey) { | |
| return; | |
| } | |
| const selected = get().selectedLaneId; | |
| const runLane = get().runLaneId; | |
| const nextSelected = selected && lanes.some((l) => l.id === selected) ? selected : lanes[0]?.id ?? null; | |
| const nextRunLane = runLane && lanes.some((l) => l.id === runLane) ? runLane : nextSelected; | |
| set((prev) => { | |
| const allowed = new Set(lanes.map((lane) => lane.id)); | |
| const nextTabs: Record<string, LaneInspectorTab> = {}; | |
| const nextLaneWorkViews: Record<string, WorkProjectViewState> = {}; | |
| for (const [laneId, tab] of Object.entries(prev.laneInspectorTabs)) { | |
| if (allowed.has(laneId)) nextTabs[laneId] = tab as LaneInspectorTab; | |
| } | |
| return { lanes, selectedLaneId: nextSelected, runLaneId: nextRunLane, laneInspectorTabs: nextTabs }; | |
| for (const [scopeKey, viewState] of Object.entries(prev.laneWorkViewByScope)) { | |
| if (!projectKey || !scopeKey.startsWith(`${projectKey}::`)) { | |
| nextLaneWorkViews[scopeKey] = viewState; | |
| continue; | |
| } | |
| const laneId = scopeKey.slice(projectKey.length + 2); | |
| if (allowed.has(laneId)) nextLaneWorkViews[scopeKey] = viewState; | |
| } | |
| return { | |
| lanes, | |
| selectedLaneId: nextSelected, | |
| runLaneId: nextRunLane, | |
| laneInspectorTabs: nextTabs, | |
| laneWorkViewByScope: nextLaneWorkViews, | |
| }; | |
| let laneRefreshRequestSeq = 0; | |
| export const useAppStore = create<AppState>((set, get) => ({ | |
| ... | |
| refreshLanes: async (options) => { | |
| const requestedProjectKey = normalizeProjectKey(get().project?.rootPath); | |
| const requestSeq = ++laneRefreshRequestSeq; | |
| const lanes = await window.ade.lanes.list({ | |
| includeArchived: false, | |
| includeStatus: options?.includeStatus ?? true, | |
| }); | |
| const projectKey = normalizeProjectKey(get().project?.rootPath); | |
| if (projectKey !== requestedProjectKey || requestSeq !== laneRefreshRequestSeq) { | |
| return; | |
| } | |
| const selected = get().selectedLaneId; | |
| const runLane = get().runLaneId; | |
| const nextSelected = selected && lanes.some((l) => l.id === selected) ? selected : lanes[0]?.id ?? null; | |
| const nextRunLane = runLane && lanes.some((l) => l.id === runLane) ? runLane : nextSelected; | |
| set((prev) => { | |
| const allowed = new Set(lanes.map((lane) => lane.id)); | |
| const nextTabs: Record<string, LaneInspectorTab> = {}; | |
| const nextLaneWorkViews: Record<string, WorkProjectViewState> = {}; | |
| for (const [laneId, tab] of Object.entries(prev.laneInspectorTabs)) { | |
| if (allowed.has(laneId)) nextTabs[laneId] = tab as LaneInspectorTab; | |
| } | |
| for (const [scopeKey, viewState] of Object.entries(prev.laneWorkViewByScope)) { | |
| if (!projectKey || !scopeKey.startsWith(`${projectKey}::`)) { | |
| nextLaneWorkViews[scopeKey] = viewState; | |
| continue; | |
| } | |
| const laneId = scopeKey.slice(projectKey.length + 2); | |
| if (allowed.has(laneId)) nextLaneWorkViews[scopeKey] = viewState; | |
| } | |
| return { | |
| lanes, | |
| selectedLaneId: nextSelected, | |
| runLaneId: nextRunLane, | |
| laneInspectorTabs: nextTabs, | |
| laneWorkViewByScope: nextLaneWorkViews, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/state/appStore.ts` around lines 252 - 287,
refreshLanes can suffer from race conditions where an earlier request returns
after a later one and prunes laneWorkViewByScope based on stale lanes; introduce
a monotonically-incrementing request counter (or timestamp) attached to the
store (e.g., lastLanesRefreshId) and capture a local refreshId at the start of
refreshLanes, incrementing the shared counter before awaiting
window.ade.lanes.list; after the await, compare the captured refreshId with the
current lastLanesRefreshId and abort if it’s not the latest, then proceed to
compute projectKey, nextSelected/nextRunLane and call set — reference the
refreshLanes function, requestedProjectKey/projectKey checks, and the
laneWorkViewByScope/laneInspectorTabs updates so you only apply the latest
response.
Comment on lines
+301
to
+303
| /** Shim kept for call-site compatibility — always returns true now that "off" mode is removed. */ | ||
| export function isComputerUseModeEnabled(_mode: ComputerUsePolicyMode | null | undefined): boolean { | ||
| return true; |
There was a problem hiding this comment.
Don't turn the mode check into an unconditional pass.
Line 303 makes every remaining isComputerUseModeEnabled() check evaluate to true. apps/desktop/src/main/services/ai/tools/workflowTools.ts:42-60 still uses this helper to derive localComputerUseAllowed, so "auto" now behaves like explicit enablement there.
🛠️ Safe interim fix
-/** Shim kept for call-site compatibility — always returns true now that "off" mode is removed. */
-export function isComputerUseModeEnabled(_mode: ComputerUsePolicyMode | null | undefined): boolean {
- return true;
+/** Preserve the mode distinction until all callers are migrated off this helper. */
+export function isComputerUseModeEnabled(mode: ComputerUsePolicyMode | null | undefined): boolean {
+ return mode === "enabled";
}Based on learnings: Treat policy enforcement and artifact ownership as hard requirements, not prompt guidance, in computer-use changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/shared/types/computerUseArtifacts.ts` around lines 301 -
303, The helper isComputerUseModeEnabled was replaced with an unconditional
true, causing all checks (including localComputerUseAllowed derived in
workflowTools.ts) to treat "auto" or disabled modes as enabled; restore proper
mode checking by implementing isComputerUseModeEnabled(mode:
ComputerUsePolicyMode | null | undefined): boolean to return true only when mode
explicitly permits local computer use (handle "on"/"enabled" states and treat
null/undefined/"off"/"disabled"/"auto" appropriately per the
ComputerUsePolicyMode enum semantics), and ensure callers like the code that
computes localComputerUseAllowed still call this function to enforce policy
rather than always allowing use.
Merged
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
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 by CodeRabbit
New Features
Improvements
Bug Fixes