Final Audit Opt#297
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR extends Claude slash-command and skill discovery with skill-root search logic, implements read-only control gating for cross-lane chat panels via ChangesClaude Slash Commands and Skills Discovery
Chat Service Claude Session and Skill Integration
Read-Only Control Gating for Cross-Lane Sessions
Workspace Availability Filtering and File Watching
Targeted PR Refresh Infrastructure
Run Page Multi-Target Pending Launch Tracking
Deferred Startup Initialization for Background Operations
GitHub Action Run and Job Fetch Limiting
Remote Runtime Connection Pool Retry Improvements
Skill Registry Indexing and Export Path Updates
System Prompt Assertions and CLI Guidance Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
@copilot review but do not make fixes |
boot open-project score 694.26 -> 204.38; removed ade.cto.getLinearConnectionStatus 2039ms from startup summary
Adapt multi-project runtime chat envelopes from runtime/event into the TUI onChatEvent stream; verified attached print-state and focused connection tests.
boot.open-project fitness 902.72 -> 53.71; localRuntime.callAction p95 2025ms -> 97ms; slow IPC channels 2 -> 0.
Prevents last lane/chat choices from one project influencing another when lane ids overlap. Keeps legacy global keys as migration fallback.
Contributor
Reviewed at c2c9cb6 with no code changes made, per request. I don’t see an immediate correctness blocker from a spot audit of the recent commits, but this PR is very broad (235 files), so I recommend narrowing follow-up audits to specific areas (Work, PRs, Files, or release workflow) for deeper confidence. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
force-pushed
the
ade/final-audit-opt-3575a904
branch
from
c2c9cb6 to
7bbba1d
Compare
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 14, 2026 09:24
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/components/prs/state/PrsContext.tsx (1)
777-780:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueued targeted refreshes lose their PR scope while a refresh is in flight.
When
refreshInFlight.currentis true, onlyrefreshPendingis recorded. The follow-up run at Line 826 always dropsgithubRefreshArgs, so a queued targeted refresh becomes an unscoped refresh.💡 Suggested fix
+const pendingGithubRefreshArgsRef = React.useRef<{ prId?: string; prIds?: string[] } | undefined>(undefined); + +function mergeGithubRefreshArgs( + a?: { prId?: string; prIds?: string[] }, + b?: { prId?: string; prIds?: string[] }, +): { prId?: string; prIds?: string[] } | undefined { + const ids = [ + ...(a?.prId ? [a.prId] : []), + ...(a?.prIds ?? []), + ...(b?.prId ? [b.prId] : []), + ...(b?.prIds ?? []), + ].map((id) => String(id ?? "").trim()).filter(Boolean); + const unique = [...new Set(ids)]; + if (unique.length === 0) return undefined; + return unique.length === 1 ? { prId: unique[0] } : { prIds: unique }; +} + if (refreshInFlight.current) { refreshPending.current = true; + pendingGithubRefreshArgsRef.current = mergeGithubRefreshArgs( + pendingGithubRefreshArgsRef.current, + options.githubRefreshArgs, + ); return; } ... if (refreshPending.current) { refreshPending.current = false; - void refreshCore({ githubRefreshMode: "await" }); + const queuedArgs = pendingGithubRefreshArgsRef.current; + pendingGithubRefreshArgsRef.current = undefined; + void refreshCore({ githubRefreshMode: "await", githubRefreshArgs: queuedArgs }); }Also applies to: 824-827, 831-842
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx` around lines 777 - 780, The queued targeted refresh loses its PR scope because when refreshInFlight.current is true the code only sets refreshPending.current = true and discards githubRefreshArgs; change this to also save the pending refresh arguments (e.g., set a new or existing variable like pendingGithubRefreshArgs = githubRefreshArgs or assign refreshPendingArgs.current = githubRefreshArgs) whenever refreshPending.current is set, and update the follow-up refresh runner (the logic that checks refreshPending.current and clears githubRefreshArgs) to prefer and consume pendingGithubRefreshArgs/current instead of always resetting to an unscoped refresh; apply the same pattern where similar early-return logic appears (the blocks around the refreshInFlight checks at lines referenced: 824-827, 831-842) so queued targeted refreshes retain their scope.apps/desktop/src/renderer/components/app/TopBar.tsx (1)
854-914:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClosing the phone-sync drawer wipes the snapshot and drops the subscription for 5 s.
Because
phoneSyncOpenis in this effect's dep array, every drawer toggle (including the close path) tears down thedisposeSyncEventssubscription and re-runs the body, which callssetSyncSnapshot(null)and schedules a fresh 5 s startup timer. The result:
- After a user closes the drawer, the header label flickers from e.g.
"1 phone connected to ADE Desktop"back to the"Phone sync"fallback and the dot reverts tobg-white/30for ~5 seconds before the newrefreshSyncStatusrepopulates it.- During that 5 s window there is no
sync.onEventsubscription, so anysync-statusevent broadcast by the main process is lost.- Even the open path tears down a perfectly fresh subscription only to immediately rebuild it.
The in-code comment ("Focus and explicit drawer opens still refresh immediately") suggests you only want an immediate refresh when the drawer opens, not a full subscription rebuild on every toggle. Suggest moving the "expedite startup on open" trigger into a separate effect (or a
startedRef) and keeping the subscription effect keyed only onproject?.rootPath/remoteBinding:♻️ Sketch of one possible split
useEffect(() => { let cancelled = false; let statusRequestVersion = 0; let started = false; let startupTimer: number | null = null; let disposeSyncEvents: (() => void) | null = null; if (!project?.rootPath || remoteBinding) { setSyncSnapshot(null); setPhoneSyncOpen(false); return () => { cancelled = true; }; } const refreshSyncStatus = () => { /* unchanged */ }; - setSyncSnapshot(null); + // Don't wipe an existing snapshot on re-run; refreshSyncStatus will + // replace it once the new request lands. const startSyncStatus = () => { /* unchanged */ }; const onFocus = () => { if (started) refreshSyncStatus(); else startSyncStatus(); }; - startupTimer = window.setTimeout( - startSyncStatus, - phoneSyncOpen ? 0 : PHONE_SYNC_STARTUP_DELAY_MS, - ); + startupTimer = window.setTimeout(startSyncStatus, PHONE_SYNC_STARTUP_DELAY_MS); window.addEventListener("focus", onFocus); + startSyncStatusRef.current = startSyncStatus; return () => { cancelled = true; if (startupTimer != null) window.clearTimeout(startupTimer); window.removeEventListener("focus", onFocus); disposeSyncEvents?.(); + startSyncStatusRef.current = null; }; - }, [phoneSyncOpen, project?.rootPath, remoteBinding]); + }, [project?.rootPath, remoteBinding]); + + // Expedite the first fetch when the drawer is explicitly opened. + useEffect(() => { + if (phoneSyncOpen) startSyncStatusRef.current?.(); + }, [phoneSyncOpen]);(
startSyncStatusRefwould be auseRef<(() => void) | null>(null)defined alongside the existing refs.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/app/TopBar.tsx` around lines 854 - 914, The effect currently depends on phoneSyncOpen which causes teardown of the subscription and setSyncSnapshot(null) on every drawer toggle; instead split the behavior: keep the subscription effect (the useEffect body using startSyncStatus, refreshSyncStatus, disposeSyncEvents, statusRequestVersion, cancelled, startupTimer) keyed only on project?.rootPath and remoteBinding so toggling the drawer does not recreate the subscription, and create a small separate effect that watches phoneSyncOpen and calls startSyncStatus (or refreshSyncStatus via a startSyncStatusRef) immediately when the drawer opens without calling setSyncSnapshot(null) or disposing the existing subscription; use a ref (e.g., startedRef or startSyncStatusRef) to expose startSyncStatus to that second effect so you can trigger an immediate refresh on open while preserving the long-lived subscription and avoiding the 5s teardown gap.apps/desktop/src/renderer/components/run/RunPage.tsx (1)
884-909:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConfirm multi-target group launch behavior and add test coverage for it.
In a group "Run all" launch, when any process emits a
runtimeevent withsessionId/ptyIdpopulated,revealRuntimeTerminalreturns true and line 897 setspendingRunLaunchRef.current = null, immediately clearing all remaining targets. Subsequentruntimeevents for group siblings then short-circuit at line 889 (if (!pending) return;) and never reach the!isActiveProcessStatuscleanup branch.The current test only emits a single runtime event, so it doesn't exercise the multi-target case. This behavior may be intentional (prevent foreground-stealing after the first terminal reveals), but it's worth confirming. If the intent is to keep tracking remaining targets for cleanup or to allow multiple terminals to surface, consider filtering only the matched target on success instead of nulling the entire ref:
♻️ Optional: filter only the matched target on reveal
if (revealRuntimeTerminal(event.runtime)) { - pendingRunLaunchRef.current = null; - return; + const remaining = pending.targets.filter((_, index) => index !== targetIndex); + pendingRunLaunchRef.current = remaining.length > 0 ? { targets: remaining } : null; + return; }Add a test that emits runtime events for both processes and assert whether the second one is/isn't revealed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/run/RunPage.tsx` around lines 884 - 909, The current onEvent handler nulls the entire pendingRunLaunchRef when revealRuntimeTerminal(event.runtime) returns true, which cancels remaining group targets; update the logic to only remove the matched target from pendingRunLaunchRef.current (using the found targetIndex) instead of setting it to null so sibling runtime events can still be processed and cleaned up, and add a unit/integration test that emits runtime events for both targets to assert whether the second process is revealed or only cleaned up; focus changes around the pendingRunLaunchRef usage, the revealRuntimeTerminal call, the targetIndex calculation, and the cleanup branch that uses isActiveProcessStatus(event.runtime.status).
🧹 Nitpick comments (5)
apps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.ts (1)
275-287: 💤 Low valueAdd guard for
__dirnameto match codebase patterns.
bundledSkillRoots()callspath.resolve(__dirname, ...)at lines 283–284 without guarding. While the main process compiles to CommonJS (.cjs), other files in the same codebase already protect__dirnamewithtypeof __dirname !== "string"checks (e.g.,openCodeBinaryManager.ts,adeCliService.ts). For consistency and defensive programming, add the same guard:if (typeof __dirname !== "string") { // fallback for ESM context return []; // or use fileURLToPath(import.meta.url) alternative }Additionally, consider memoizing the resolved roots—the function generates ~18 candidate paths on every call, though
fs.existsSyncfilters them downstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.ts` around lines 275 - 287, bundledSkillRoots currently uses __dirname unguarded which can break in ESM contexts; add a guard at the top of bundledSkillRoots (check typeof __dirname !== "string") and return a safe fallback (e.g., [] or derive a path via fileURLToPath(import.meta.url)) to mirror the pattern used in openCodeBinaryManager.ts / adeCliService.ts, and memoize the computed candidate array (cache in a module-level variable) so repeated calls reuse the ~18 resolved paths instead of rebuilding them each time; reference bundledSkillRoots and __dirname when making these changes.apps/desktop/src/main/services/chat/agentChatService.ts (3)
14829-14837: ⚡ Quick winPrewarm gated on
runtime?.kind === "claude"may skip Claude-provider sessions whose runtime hasn't been created yet.Hunks 4 and 9 broadened other Claude dispatch points to key off
managed.session.provider === "claude", but the fork branch here still gates the reset/prewarm oncreatedManaged.runtime?.kind === "claude". For a freshlyensureManagedSession'd session the runtime may not yet exist, in which caseclaudeBackgroundResumeSessionIdand the pointer are mirrored but no prewarm fires for what is effectively a Claude session. Consider switching the inner gate to a provider check (and usingensureClaudeSessionRuntime) to match the rest of the file.♻️ Suggested adjustment
if (handoffMode === "fork") { createdManaged.claudeBackgroundResumeSessionId = sourceSdkSessionId; mirrorClaudeSessionPointer(createdManaged, sourceSdkSessionId); - if (createdManaged.runtime?.kind === "claude") { - resetClaudeQuerySession(createdManaged, createdManaged.runtime, "session_reset", { clearSdkSessionId: true }); - createdManaged.runtime.forkFromSdkSessionId = sourceSdkSessionId; + if (createdManaged.session.provider === "claude") { + const createdRuntime = ensureClaudeSessionRuntime(createdManaged); + resetClaudeQuerySession(createdManaged, createdRuntime, "session_reset", { clearSdkSessionId: true }); + createdRuntime.forkFromSdkSessionId = sourceSdkSessionId; prewarmClaudeQuery(createdManaged); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 14829 - 14837, The fork branch currently checks createdManaged.runtime?.kind === "claude" which can miss freshly-created Claude sessions without a runtime; change the inner condition to check createdManaged.session.provider === "claude" and call ensureClaudeSessionRuntime(createdManaged) to create/return the runtime before invoking resetClaudeQuerySession, setting createdManaged.runtime.forkFromSdkSessionId and calling prewarmClaudeQuery, so that mirrorClaudeSessionPointer and claudeBackgroundResumeSessionId are mirrored but the reset/prewarm always run for Claude-provider sessions even if the runtime did not yet exist.
18883-18898: 💤 Low valueRedundant null check on
short.
managed.claudeBackgroundJobShortwas already proven truthy in the outer condition, so the innerif (short)can never be false. Drop the redundant check (and either inline the variable or keep it for readability).♻️ Suggested simplification
if (managed.session.provider === "claude" && managed.claudeBackgroundJobShort && managed.runtime?.kind !== "claude") { const short = managed.claudeBackgroundJobShort; - if (short) { - await runClaudeBackgroundCliCommand(managed, ["stop", short], 15_000).catch((error) => { - logger.warn("agent_chat.claude_background_stop_failed", { - sessionId, - short, - error: error instanceof Error ? error.message : String(error), - }); - }); - } + await runClaudeBackgroundCliCommand(managed, ["stop", short], 15_000).catch((error) => { + logger.warn("agent_chat.claude_background_stop_failed", { + sessionId, + short, + error: error instanceof Error ? error.message : String(error), + }); + }); markSessionIdleWithFreshCache(managed); persistChatState(managed); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 18883 - 18898, The inner null-check on short is redundant because managed.claudeBackgroundJobShort is already tested in the outer if; remove the inner if (short) and directly call await runClaudeBackgroundCliCommand(managed, ["stop", short], 15_000).catch(...). Keep the short local variable if you prefer readability (or inline managed.claudeBackgroundJobShort into runClaudeBackgroundCliCommand), and ensure the logger.warn, markSessionIdleWithFreshCache(managed), and persistChatState(managed) behavior remains unchanged.
20600-20607: 💤 Low value
ensureClaudeSessionRuntimemutates state on first invocation even when subsequent guards bail.
ensureClaudeSessionRuntime(managed)materializes a new Claude runtime regardless of whetherprewarmClaudeQuerywill actually execute. The function:
- Calls
evictLeastRecentRuntime()if at capacity (line 14160)—a side effect modifying globalmanagedSessionsstate- Creates and assigns a new runtime object to
managed.runtime(line 14181)—mutating the managed sessionWhile the function has an early return for existing runtimes (line 14157), on first invocation these side effects occur unconditionally. Since the subsequent checks (
runtime.busy,runtime.query,runtime.warmQuery,runtime.warmupDone) at lines 20601-20605 can cause early returns beforeprewarmClaudeQueryis called, consider materializing the runtime only when actually proceeding to prewarming to avoid unnecessary state allocation and eviction cycles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 20600 - 20607, The current code calls ensureClaudeSessionRuntime(managed) which may evict or allocate a new runtime even when later guards bail; change the logic to avoid creating/mutating runtime unless you will actually prewarm: first check for an existing runtime via managed.runtime (or add a non-mutating accessor like getClaudeSessionRuntimeIfExists/peekClaudeSessionRuntime) and only call ensureClaudeSessionRuntime(managed) when those guards pass and you are about to call prewarmClaudeQuery(managed); this prevents evictLeastRecentRuntime() and assignment to managed.runtime happening unnecessarily and keeps persistChatState(managed) paired only with true prewarm actions.apps/desktop/src/main/services/prs/prService.ts (1)
6462-6464: ⚡ Quick winAdd lightweight logging for skipped job details on fetch failure.
At Line 6462, the catch block silently drops job data. When jobs disappear in UI, this becomes hard to diagnose. Log a warning with PR/run context before returning empty jobs.
Proposed patch
- } catch { - // Jobs fetch failed; return empty jobs array + } catch (error) { + logger.warn("prs.action_run_jobs_fetch_failed", { + prId, + runId, + error: getErrorMessage(error), + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prService.ts` around lines 6462 - 6464, The empty catch at the jobs fetch silently swallows failures; update the catch in prs/prService.ts (the block that currently returns an empty jobs array) to log a lightweight warning before returning, using the repository/PR/run context available in scope (e.g., PR id, run id, commit SHA) and include the caught error message/stack; use the existing logger instance in this module (e.g., logger or processLogger) so the warning reads like "Failed to fetch jobs for PR <prId>/run <runId> (sha=<sha>): <error>" and then return the empty jobs array as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 3304-3312: Tighten the assertions to check the deterministic SDK
IDs used in the test: replace the loose expect.any(String) on
claudeSdkResumeSessionCompat to assert the exact stale ID string ("sdk-stale")
as the first argument, and replace the loose expect.any(String) on the persisted
SDK session value from readPersistedChatState(session.id).sdkSessionId to assert
the exact fresh ID string ("sdk-fresh"); keep the existing checks for
claudeSdkCreateSessionCompat, staleSession.close, and freshSession.send
unchanged.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 9167-9186: The system:init branch assigns initMsg.session_id
without trimming which can overwrite the already-trimmed runtime.sdkSessionId;
change the logic in the system:init block to trim initMsg.session_id once into a
local (e.g., trimmedSessionId), use that for the length check and comparison
against runtime.sdkSessionId, and assign runtime.sdkSessionId = trimmedSessionId
before calling mirrorClaudeSessionPointer(managed, runtime.sdkSessionId) and
persistChatState(managed); keep reportedInitModel =
normalizeReportedModelName(initMsg.model) unchanged.
In `@apps/desktop/src/main/services/memory/skillRegistryService.ts`:
- Around line 145-148: The description frontmatter interpolates user data
directly (see slugify(input.title) and the `description: Use this skill when
${input.trigger.trim() || "the workflow applies"}.` line), which risks YAML
injection; fix by producing a YAML-safe value for the description instead of raw
interpolation — either escape/quote input.trigger (e.g., wrap the description in
a proper quoted string or escape YAML special chars) or, preferably, build the
entire frontmatter with a YAML serializer (yaml.safeDump / YAML.stringify) using
input.title and input.trigger so the library handles quoting/escaping for you.
In `@apps/desktop/src/main/services/prs/prService.test.ts`:
- Around line 1109-1172: The mock workflowRuns are generated oldest-first but
GitHub returns runs newest-first, so update the test to return runs in
descending created_at order: reverse the workflowRuns array (or generate it from
20 down to 1) before returning it from the githubService.apiRequest handler for
path "/repos/test-owner/test-repo/actions/runs"; this ensures
service.getActionRuns hydrates jobs for the newest 6 runs and the assertions
about calls to githubService.apiRequest and job-fetching (the
/actions/runs/:id/jobs requests) match real API behavior.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 2224-2227: Handlers that currently only check controlsDisabled
(e.g., the branches that call setMessage(controlsDisabledMessage) and return)
must also prevent input when controls are owned remotely: extend the guard to
check ownedByOtherChat (and any lane-mismatch reason) before calling
typeText/tap/drag so remote ownership blocks input; update the message to
reflect ownership (use ownedByOtherChat-specific message or compose from
controlsDisabledMessage) and apply this change in the handlers referencing
controlsDisabled around the setMessage calls and the functions that call
typeText/tap/drag to ensure clicks/typing/dragging are blocked when
ownedByOtherChat is true.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Around line 867-869: The code only hydrates activity when prev.length === 0
causing stale activity after PR switches; change the setActivity call to always
rebuild from the loaded details instead of conditionally using prev: replace
setActivity((prev) => prev.length > 0 ? prev :
buildActivityFromLoadedDetail(loaded.checks, loaded.reviews, loaded.comments))
with setActivity(buildActivityFromLoadedDetail(loaded.checks, loaded.reviews,
loaded.comments)) (apply same fix at the other occurrence around lines 956-958),
referencing loadedDetailActivityRef, setActivity, and
buildActivityFromLoadedDetail so the new PR's checks/reviews/comments are used
immediately.
- Around line 314-340: The synthesized event IDs in PrDetailPane.tsx (the loops
over reviews and checks that push into events) are not unique (using
`review-${review.reviewer}-${review.submittedAt}` and `ci-${check.name}`) and
can collide; change the ID generation in those push calls to include a unique
discriminator such as the loop index or an intrinsic unique field (e.g.,
`review.id` / `check.id` if available) or a deterministic hash of the object, so
each event entry in the `events` array has a stable unique `id`; update both the
review and check push sites (the for (const review of reviews) and for (const
check of checks) blocks) to produce unique IDs and keep the rest of the payload
unchanged.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 854-914: The effect currently depends on phoneSyncOpen which
causes teardown of the subscription and setSyncSnapshot(null) on every drawer
toggle; instead split the behavior: keep the subscription effect (the useEffect
body using startSyncStatus, refreshSyncStatus, disposeSyncEvents,
statusRequestVersion, cancelled, startupTimer) keyed only on project?.rootPath
and remoteBinding so toggling the drawer does not recreate the subscription, and
create a small separate effect that watches phoneSyncOpen and calls
startSyncStatus (or refreshSyncStatus via a startSyncStatusRef) immediately when
the drawer opens without calling setSyncSnapshot(null) or disposing the existing
subscription; use a ref (e.g., startedRef or startSyncStatusRef) to expose
startSyncStatus to that second effect so you can trigger an immediate refresh on
open while preserving the long-lived subscription and avoiding the 5s teardown
gap.
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 777-780: The queued targeted refresh loses its PR scope because
when refreshInFlight.current is true the code only sets refreshPending.current =
true and discards githubRefreshArgs; change this to also save the pending
refresh arguments (e.g., set a new or existing variable like
pendingGithubRefreshArgs = githubRefreshArgs or assign
refreshPendingArgs.current = githubRefreshArgs) whenever refreshPending.current
is set, and update the follow-up refresh runner (the logic that checks
refreshPending.current and clears githubRefreshArgs) to prefer and consume
pendingGithubRefreshArgs/current instead of always resetting to an unscoped
refresh; apply the same pattern where similar early-return logic appears (the
blocks around the refreshInFlight checks at lines referenced: 824-827, 831-842)
so queued targeted refreshes retain their scope.
In `@apps/desktop/src/renderer/components/run/RunPage.tsx`:
- Around line 884-909: The current onEvent handler nulls the entire
pendingRunLaunchRef when revealRuntimeTerminal(event.runtime) returns true,
which cancels remaining group targets; update the logic to only remove the
matched target from pendingRunLaunchRef.current (using the found targetIndex)
instead of setting it to null so sibling runtime events can still be processed
and cleaned up, and add a unit/integration test that emits runtime events for
both targets to assert whether the second process is revealed or only cleaned
up; focus changes around the pendingRunLaunchRef usage, the
revealRuntimeTerminal call, the targetIndex calculation, and the cleanup branch
that uses isActiveProcessStatus(event.runtime.status).
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 14829-14837: The fork branch currently checks
createdManaged.runtime?.kind === "claude" which can miss freshly-created Claude
sessions without a runtime; change the inner condition to check
createdManaged.session.provider === "claude" and call
ensureClaudeSessionRuntime(createdManaged) to create/return the runtime before
invoking resetClaudeQuerySession, setting
createdManaged.runtime.forkFromSdkSessionId and calling prewarmClaudeQuery, so
that mirrorClaudeSessionPointer and claudeBackgroundResumeSessionId are mirrored
but the reset/prewarm always run for Claude-provider sessions even if the
runtime did not yet exist.
- Around line 18883-18898: The inner null-check on short is redundant because
managed.claudeBackgroundJobShort is already tested in the outer if; remove the
inner if (short) and directly call await runClaudeBackgroundCliCommand(managed,
["stop", short], 15_000).catch(...). Keep the short local variable if you prefer
readability (or inline managed.claudeBackgroundJobShort into
runClaudeBackgroundCliCommand), and ensure the logger.warn,
markSessionIdleWithFreshCache(managed), and persistChatState(managed) behavior
remains unchanged.
- Around line 20600-20607: The current code calls
ensureClaudeSessionRuntime(managed) which may evict or allocate a new runtime
even when later guards bail; change the logic to avoid creating/mutating runtime
unless you will actually prewarm: first check for an existing runtime via
managed.runtime (or add a non-mutating accessor like
getClaudeSessionRuntimeIfExists/peekClaudeSessionRuntime) and only call
ensureClaudeSessionRuntime(managed) when those guards pass and you are about to
call prewarmClaudeQuery(managed); this prevents evictLeastRecentRuntime() and
assignment to managed.runtime happening unnecessarily and keeps
persistChatState(managed) paired only with true prewarm actions.
In `@apps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.ts`:
- Around line 275-287: bundledSkillRoots currently uses __dirname unguarded
which can break in ESM contexts; add a guard at the top of bundledSkillRoots
(check typeof __dirname !== "string") and return a safe fallback (e.g., [] or
derive a path via fileURLToPath(import.meta.url)) to mirror the pattern used in
openCodeBinaryManager.ts / adeCliService.ts, and memoize the computed candidate
array (cache in a module-level variable) so repeated calls reuse the ~18
resolved paths instead of rebuilding them each time; reference bundledSkillRoots
and __dirname when making these changes.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 6462-6464: The empty catch at the jobs fetch silently swallows
failures; update the catch in prs/prService.ts (the block that currently returns
an empty jobs array) to log a lightweight warning before returning, using the
repository/PR/run context available in scope (e.g., PR id, run id, commit SHA)
and include the caught error message/stack; use the existing logger instance in
this module (e.g., logger or processLogger) so the warning reads like "Failed to
fetch jobs for PR <prId>/run <runId> (sha=<sha>): <error>" and then return the
empty jobs array as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: adb2c5c4-b260-4f30-8cb8-bdc3cd79b149
⛔ Files ignored due to path filters (30)
.github/workflows/prepare-release.ymlis excluded by none and included by none.github/workflows/release-core.ymlis excluded by none and included by none.github/workflows/release.ymlis excluded by none and included by noneapps/ade-cli/src/cli.test.tsis excluded by none and included by noneapps/ade-cli/src/cli.tsis excluded by none and included by noneapps/ade-cli/src/multiProjectRpcServer.test.tsis excluded by none and included by noneapps/ade-cli/src/multiProjectRpcServer.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxis excluded by none and included by noneapps/ade-cli/src/tuiClient/__tests__/connection.test.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/__tests__/project.test.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/__tests__/state.test.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/app.tsxis excluded by none and included by noneapps/ade-cli/src/tuiClient/cli.tsxis excluded by none and included by noneapps/ade-cli/src/tuiClient/connection.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/project.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/state.tsis excluded by none and included by noneapps/ade-cli/src/tuiClient/types.tsis excluded by none and included by noneapps/desktop/package.jsonis excluded by none and included by noneapps/desktop/resources/ade-cli-help.txtis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-app-control/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-browser/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-cli-control-plane/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-cto-missions/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-lanes-git/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-macos-vm/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-pr-workflows/SKILL.mdis excluded by none and included by noneapps/desktop/resources/agent-skills/ade-proof-artifacts/SKILL.mdis excluded by none and included by noneapps/desktop/scripts/validate-mac-artifacts.mjsis excluded by none and included by noneapps/desktop/scripts/validate-win-artifacts.mjsis excluded by none and included by none
📒 Files selected for processing (37)
apps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.test.tsapps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/main/services/files/fileService.tsapps/desktop/src/main/services/memory/skillRegistryService.test.tsapps/desktop/src/main/services/memory/skillRegistryService.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionPool.tsapps/desktop/src/renderer/components/app/LinearQuickViewButton.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/prs/PRsPage.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/desktop/src/renderer/components/run/RunPage.test.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.test.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/renderer/components/usage/HeaderUsageControl.tsxapps/desktop/src/shared/adeCliGuidance.ts
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
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Greptile Summary
This PR delivers a broad audit of ADE's agent infrastructure, touching skill discovery, chat session handling, file path security, CLI state management, and several UI panels.
.agents/skills,.ade/skills, bundled app skills, and user skill roots; adds YAML frontmatter to exported SKILL.md files and moves the export target from.claude/skillsto.ade/skills.refreshStatehistory hydration incremental (skips re-fetches when the session is unchanged), defers phone-sync subscription startup by 5 s, makes the Activity tab load lazily, adds targeted per-PR refresh, and gracefully falls back to the primary workspace when a worktree root goes missing.Confidence Score: 4/5
Safe to merge; the changes are additive hardening and incremental optimisation with no breaking API changes.
All three findings are non-blocking quality notes. The phone-sync subscription teardown on drawer toggle is a minor UX regression (brief status flash, brief event gap). The bundledSkillRoots path-walk and the double-realpathSync in transcript reads are low-impact inefficiencies that do not affect correctness.
apps/desktop/src/renderer/components/app/TopBar.tsx (phoneSyncOpen dep), apps/desktop/src/main/services/chat/claudeSlashCommandDiscovery.ts (bundledSkillRoots depth), apps/desktop/src/main/services/chat/agentChatService.ts (resolveReadableChatPath double-realpathSync)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[refreshState called] --> B{hydrateHistory option} B -- "false + same session" --> C[Skip getChatHistory] B -- "true OR session changed" --> D[getChatHistory] D --> E{isCurrentRefresh?} E -- No --> F[Abandon stale result] E -- Yes --> G[setEvents + loadedSessionIdRef update] C --> H[Update lanes/sessions/models] G --> HComments Outside Diff (1)
apps/desktop/src/main/services/chat/agentChatService.ts, line 10383-10510 (link)runClaudeBackgroundTurnhas no call siterunClaudeBackgroundTurn(and its entire dependency chain —dispatchClaudeBackgroundPrompt,monitorClaudeBackgroundJob,buildClaudeBackgroundPromptText,buildClaudeBackgroundArgs,buildClaudeBackgroundSystemPrompt) is defined but never invoked anywhere in the codebase. The turn-dispatch router at line ~10525 still routes all Claude sessions torunClaudeTurn. This adds ~600 lines of complex logic that is currently unreachable code. Is this intentional prep work missing its dispatch hookup, or shouldrunClaudeBackgroundTurnbe wired into the provider selection block?Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(audit): address release gate review ..." | Re-trigger Greptile