fix: quick fixes 3 — terminal, chat, files, state improvements#146
Merged
fix: quick fixes 3 — terminal, chat, files, state improvements#146
Conversation
Assorted fixes across main process, renderer components, and tests including terminal session handling, agent chat UX, file watcher, and store updates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds per-project last-activated timestamps and a serialized project-context rebalance/eviction; exposes active embedded MCP connection counts on AppContext; expands context disposal; tightens chat history sizing/retention and session LRU caches; introduces session suspension UI for inactive tiles; reduces terminal scrollback caps and updates tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/renderer/components/terminals/TerminalView.tsx (1)
501-515:⚠️ Potential issue | 🟡 MinorKeep queued frame writes parked when the last view unmounts.
If a flush is already queued when
runtime.refsdrops to0, this callback still joinsframeWriteChunksand callsterm.write()into the parked host. That reintroduces the exact race this change is trying to remove: some output is flushed while unmounted instead of staying buffered until remount. Bail out before draining the queue when there are no active refs; Line 952 already reschedules the flush after remount.Proposed fix
function scheduleFrameWriteFlush(runtime: CachedRuntime) { if (runtime.flushRafId != null) return; runtime.flushRafId = requestAnimationFrame(() => { runtime.flushRafId = null; if (runtime.disposed) return; if (runtime.frameWriteChunks.length === 0) return; + if (runtime.refs === 0) return; const merged = runtime.frameWriteChunks.join(""); runtime.frameWriteChunks.length = 0; runtime.frameWriteBytes = 0; try { runtime.term.write(merged);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx` around lines 501 - 515, The scheduled flush in scheduleFrameWriteFlush drains frameWriteChunks and calls runtime.term.write even if there are no active views, reintroducing the race; update the function to check runtime.refs and return early (do not join or clear frameWriteChunks or call term.write) when runtime.refs === 0 (and still respect runtime.disposed and empty-chunk checks), so queued writes remain buffered until remount (the remount logic already reschedules the flush).
🧹 Nitpick comments (2)
apps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.ts (1)
9-18: Redundant normalization can be avoided.
clearWorkspaceEntriesnormalizesrootPathon line 10, but when called fromreplaceDirtyBuffersForWorkspace(line 28), the path is already normalized on line 27. Consider accepting an already-normalized path to avoid the redundant operation.♻️ Optional: Skip normalization for internal calls
-function clearWorkspaceEntries(rootPath: string): void { - const rootNorm = path.normalize(rootPath); +function clearWorkspaceEntries(rootNorm: string): void { const prefix = rootNorm.endsWith(path.sep) ? rootNorm : `${rootNorm}${path.sep}`; for (const key of [...dirtyByAbsPath.keys()]) { const kn = path.normalize(key); if (kn === rootNorm || kn.startsWith(prefix)) { dirtyByAbsPath.delete(key); } } }Then update
clearDirtyBuffersForWorkspaceto normalize before calling:export function clearDirtyBuffersForWorkspace(rootPath: string): void { - clearWorkspaceEntries(rootPath); + clearWorkspaceEntries(path.normalize(rootPath)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.ts` around lines 9 - 18, clearWorkspaceEntries currently re-normalizes rootPath unnecessarily; change it to assume the caller provides a normalized path (remove the path.normalize call and use rootPath directly for rootNorm/prefix), and update callers that may pass raw paths—specifically ensure replaceDirtyBuffersForWorkspace (or clearDirtyBuffersForWorkspace) performs path.normalize(rootPath) before calling clearWorkspaceEntries so only the caller normalizes once.apps/desktop/src/renderer/components/files/FilesPage.tsx (1)
521-533: Consider debouncing high-frequency session snapshots.This effect runs on every
openTabschange, including content updates during typing (line 1245 updates tab content on editor change). With 8 cached scopes and simple operations, performance should be acceptable, but if profiling reveals issues, consider debouncing the snapshot:♻️ Optional: Debounce session persistence for typing performance
// At module level let snapshotTimer: number | null = null; // In effect useEffect(() => { if (snapshotTimer) window.clearTimeout(snapshotTimer); snapshotTimer = window.setTimeout(() => { setFilesPageSession(sessionKey, snapshotFilesPageSessionState({ workspaceId, workspaceRootPath: activeWorkspace?.rootPath ?? null, // ... rest of properties })); }, 100); return () => { if (snapshotTimer) window.clearTimeout(snapshotTimer); }; }, [sessionKey, workspaceId, activeWorkspace?.rootPath, /* ... */]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/files/FilesPage.tsx` around lines 521 - 533, The effect that calls setFilesPageSession with snapshotFilesPageSessionState runs on every openTabs change (including high-frequency editor updates) and should be debounced: add a module-scoped timer variable (e.g., snapshotTimer), replace the direct call inside the useEffect with a window.setTimeout that delays invoking setFilesPageSession by ~100ms, clear any existing timeout before scheduling, and return a cleanup that clears the timeout to avoid leaks; keep the same dependency array (sessionKey, workspaceId, activeWorkspace?.rootPath, allowPrimaryEdit, selectedNodePath, openTabs, activeTabPath, mode, searchQuery, editorTheme) so snapshots still run after the delay.
🤖 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/main.ts`:
- Around line 885-905: Before evicting each projectRoot from idleRoots,
revalidate the live state: reread currentActiveRoot and ctx (use
projectContexts.get(projectRoot)), skip if activeProjectRoot !==
currentActiveRoot or projectRoot === activeProjectRoot or
warmRoots.has(projectRoot), and explicitly check the context for any live work
(e.g., session, mission, sync peer, MCP connection on ctx — inspect whatever
fields/methods represent activeSession/activeMission/syncPeer/mcpConnection) and
only then log "project.context_evicted" and call
closeProjectContext(projectRoot); this prevents tearing down a context that
became active between the probe and eviction.
- Around line 3139-3148: Before disposing embeddingService, ensure the
embeddingWorkerService finishes queued work by awaiting its idle state: call
await embeddingWorkerService.waitForIdle() (or equivalent) after stopping health
checks and before calling ctx.embeddingService.dispose(), so any pending
embeddingService.embed() calls complete; keep the existing try/catch around
stopHealthCheck and dispose but insert the await
embeddingWorkerService.waitForIdle() between them, referencing
ctx.embeddingService and embeddingWorkerService.waitForIdle to locate where to
change.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 505-507: trimChatEventHistory currently does a blind tail slice
which can drop unresolved control events that deriveRuntimeState() relies on
(e.g., status.started, pending input/steer markers) causing turnActive to be
recomputed incorrectly; update trimChatEventHistory to retain the newest
unresolved control events instead of naively taking the last N events: scan
backwards from the end of events to include any trailing control events needed
by deriveRuntimeState (status.started, queued/unresolved input markers, pending
steers) and expand the kept window to include their corresponding start markers
so the replayed log preserves in-progress turns and pending state; keep
references to trimChatEventHistory, deriveRuntimeState, AgentChatEventEnvelope,
and the status.started/pending markers when implementing this logic.
- Around line 1170-1190: The current refreshLockedSessionSummary reuses
initialSessionSummary every call when sessionId matches, causing stale fields;
add a consumed flag (e.g., seededInitialSummaryRef) and use
initialSessionSummary only once: if seededInitialSummaryRef.current is false and
initialSessionSummary?.sessionId === lockSessionId then use
initialSessionSummary and set seededInitialSummaryRef.current = true, otherwise
always call window.ade.agentChat.getSummary({ sessionId: lockSessionId }); keep
the rest of the state updates (setSessions, setTurnActiveBySession,
draftSelectionLockedRef.current, setSelectedSessionId) unchanged so subsequent
refreshes read fresh data.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 47-52: The fallback preview string for suspended tiles currently
always tells the user to "resume the live ... view" even for non-resumable or
ended sessions; update the logic that builds preview (where session.summary,
session.lastOutputPreview and isChatToolType(session.toolType) are used) to
first inspect session.status or a resumability check (e.g., a small helper like
canResume(session) or checking a ended/disposed flag) and choose messaging
accordingly—if the session is resumable keep the current "Select this tile to
resume..." copy, otherwise reuse the ended-state wording (or a concrete,
stateful message like "This session has ended and cannot be resumed") so
non-resumable tiles never show a prompt to resume. Ensure you update the preview
assignment in the same block that references secondarySessionLabel and
sessionStatusDot.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Line 247: The store clamps scrollback to a maximum of 30_000 (return
Math.max(..., Math.min(30_000, ...))) while the settings UI (GeneralSection
component) still offers 50_000; update either side so they match — either change
the settings option value(s) in GeneralSection to use 30_000 instead of 50_000,
or raise the clamp constant in the appStore scrollback calculation to
50_000—make the single source of truth consistent and keep the numeric literal
(30_000/50_000) in sync.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/terminals/TerminalView.tsx`:
- Around line 501-515: The scheduled flush in scheduleFrameWriteFlush drains
frameWriteChunks and calls runtime.term.write even if there are no active views,
reintroducing the race; update the function to check runtime.refs and return
early (do not join or clear frameWriteChunks or call term.write) when
runtime.refs === 0 (and still respect runtime.disposed and empty-chunk checks),
so queued writes remain buffered until remount (the remount logic already
reschedules the flush).
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/files/FilesPage.tsx`:
- Around line 521-533: The effect that calls setFilesPageSession with
snapshotFilesPageSessionState runs on every openTabs change (including
high-frequency editor updates) and should be debounced: add a module-scoped
timer variable (e.g., snapshotTimer), replace the direct call inside the
useEffect with a window.setTimeout that delays invoking setFilesPageSession by
~100ms, clear any existing timeout before scheduling, and return a cleanup that
clears the timeout to avoid leaks; keep the same dependency array (sessionKey,
workspaceId, activeWorkspace?.rootPath, allowPrimaryEdit, selectedNodePath,
openTabs, activeTabPath, mode, searchQuery, editorTheme) so snapshots still run
after the delay.
In `@apps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.ts`:
- Around line 9-18: clearWorkspaceEntries currently re-normalizes rootPath
unnecessarily; change it to assume the caller provides a normalized path (remove
the path.normalize call and use rootPath directly for rootNorm/prefix), and
update callers that may pass raw paths—specifically ensure
replaceDirtyBuffersForWorkspace (or clearDirtyBuffersForWorkspace) performs
path.normalize(rootPath) before calling clearWorkspaceEntries so only the caller
normalizes once.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aaeb35ba-ab73-4372-9e34-6b54988c69a5
📒 Files selected for processing (15)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/tests/testService.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/LaneTerminalsPanel.tsxapps/desktop/src/renderer/components/lanes/TilingLayout.tsxapps/desktop/src/renderer/components/project/ProjectHomePage.tsxapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useSessionDelta.tsapps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.ts
Comment on lines
+3139
to
+3148
| try { | ||
| ctx.embeddingService?.stopHealthCheck?.(); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| try { | ||
| await ctx.embeddingService?.dispose?.(); | ||
| } catch { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
worker_file="$(fd 'embeddingWorkerService\.ts$' apps/desktop/src/main/services/memory | head -n1)"
main_file="apps/desktop/src/main/main.ts"
printf '== embedding worker API ==\n'
sed -n '1,260p' "$worker_file" | rg -n -C2 'createEmbeddingWorkerService|return \{|start\(|stop\(|dispose\('
printf '\n== current teardown sites in main.ts ==\n'
rg -n -C3 'embeddingWorkerService|embeddingService\?\.dispose|stopHealthCheck' "$main_file"Repository: arul28/ADE
Length of output: 3471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and read the complete embeddingWorkerService.ts file
worker_file="$(fd 'embeddingWorkerService\.ts$' apps/desktop/src/main/services/memory | head -n1)"
echo "=== Complete embeddingWorkerService.ts ==="
wc -l "$worker_file"
cat -n "$worker_file"Repository: arul28/ADE
Length of output: 11665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete teardown section around lines 3139-3148 in main.ts
echo "=== Teardown section in main.ts (lines 3100-3200) ==="
sed -n '3100,3200p' apps/desktop/src/main/main.ts | cat -nRepository: arul28/ADE
Length of output: 2654
Wait for embedding worker to complete processing before disposing embeddingService.
The worker queue is in-memory and can have pending embedding work. Currently, embeddingService is disposed without ensuring the worker has finished processing. If the worker calls embeddingService.embed() on queued items after the service is disposed, it will fail.
The embeddingWorkerService does not expose a stop() or dispose() method. Instead, call await embeddingWorkerService.waitForIdle() before disposing embeddingService to ensure all queued work completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/main.ts` around lines 3139 - 3148, Before disposing
embeddingService, ensure the embeddingWorkerService finishes queued work by
awaiting its idle state: call await embeddingWorkerService.waitForIdle() (or
equivalent) after stopping health checks and before calling
ctx.embeddingService.dispose(), so any pending embeddingService.embed() calls
complete; keep the existing try/catch around stopHealthCheck and dispose but
insert the await embeddingWorkerService.waitForIdle() between them, referencing
ctx.embeddingService and embeddingWorkerService.waitForIdle to locate where to
change.
Comment on lines
+505
to
+507
| function trimChatEventHistory(events: AgentChatEventEnvelope[], maxEvents: number): AgentChatEventEnvelope[] { | ||
| return events.length > maxEvents ? events.slice(-maxEvents) : events; | ||
| } |
There was a problem hiding this comment.
Do not prune away the state transition that deriveRuntimeState() depends on.
trimChatEventHistory() is a blind tail slice, but deriveRuntimeState() recomputes turnActive, pending inputs, and pending steers by replaying the retained log. A long active turn can exceed 240/1200 events before done arrives, which drops the last status.started or unresolved queued markers from the kept window and makes the pane think the session is idle mid-turn. Pruning needs to preserve the newest unresolved control events, or runtime state needs to be tracked incrementally before the trim.
Also applies to: 1391-1396, 1694-1699
🤖 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 505
- 507, trimChatEventHistory currently does a blind tail slice which can drop
unresolved control events that deriveRuntimeState() relies on (e.g.,
status.started, pending input/steer markers) causing turnActive to be recomputed
incorrectly; update trimChatEventHistory to retain the newest unresolved control
events instead of naively taking the last N events: scan backwards from the end
of events to include any trailing control events needed by deriveRuntimeState
(status.started, queued/unresolved input markers, pending steers) and expand the
kept window to include their corresponding start markers so the replayed log
preserves in-progress turns and pending state; keep references to
trimChatEventHistory, deriveRuntimeState, AgentChatEventEnvelope, and the
status.started/pending markers when implementing this logic.
Comment on lines
+47
to
+52
| if (suspended) { | ||
| const secondary = secondarySessionLabel(session); | ||
| const status = sessionStatusDot(session); | ||
| const preview = session.summary?.trim() | ||
| || session.lastOutputPreview?.trim() | ||
| || (isChatToolType(session.toolType) ? "Select this tile to resume the live chat view." : "Select this tile to resume the live terminal view."); |
There was a problem hiding this comment.
Avoid “resume live … view” copy for non-resumable tiles.
This fallback runs for every suspended tile, including ended/disposed sessions, so it can tell the user to resume something that is no longer live. Derive the message from session.status/resume capability, or reuse the ended-state wording here.
As per coding guidelines, "Keep user-facing copy concrete and stateful: say what changed, what is blocked, and what the next action is".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx` around lines
47 - 52, The fallback preview string for suspended tiles currently always tells
the user to "resume the live ... view" even for non-resumable or ended sessions;
update the logic that builds preview (where session.summary,
session.lastOutputPreview and isChatToolType(session.toolType) are used) to
first inspect session.status or a resumability check (e.g., a small helper like
canResume(session) or checking a ended/disposed flag) and choose messaging
accordingly—if the session is resumable keep the current "Select this tile to
resume..." copy, otherwise reuse the ended-state wording (or a concrete,
stateful message like "This session has ended and cannot be resumed") so
non-resumable tiles never show a prompt to resume. Ensure you update the preview
assignment in the same block that references secondarySessionLabel and
sessionStatusDot.
- Fix test-desktop shard 1 failures: tests now use status: "idle" for sessions where the Send button is expected (refreshLockedSessionSummary now correctly derives turnActive from session status) - Fix stale locked session summary: use initialSessionSummary only for the first render seed, then always call getSummary for fresh data - Add getSummary and editSteer mocks to test harness - Remove stale expect(list).toHaveBeenCalled() assertions (locked mode no longer calls list) - Sync scrollback options: GeneralSection now offers 30k max to match appStore clamp - Add TOCTOU guard in rebalanceProjectContexts: re-check workloads immediately before eviction - Add eslint-disable comment for intentional dep omission in FilesPage session-switch effect - Guard terminal flush when unmounted: skip frame writes when refs === 0 to keep data buffered until remount https://claude.ai/code/session_01QxY6yqm5AEtA4E4JijBdH4
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
- Merge readiness + stats sidebar now use unified checks (check-runs + action-runs) instead of only check-runs API, fixing "No checks" / "0/0 passing" when action runs have data but check-runs returns empty - Add 60s polling for actionRuns/activity/reviewThreads (previously loaded once) - Auto-detect review bot sources from [bot] suffix instead of hardcoded patterns; Greptile, Seer, and any future GitHub App bots work without code changes - Add error logging to fetchCheckRuns/fetchCombinedStatus (were silently swallowed) - Fix summarizeChecks + convergence panel to count skipped/neutral as passing - Widen IssueSource type to accept arbitrary bot source strings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/renderer/components/files/FilesPage.tsx`:
- Around line 147-151: The code currently reads evictedSession.workspaceRootPath
directly and can see null during interim snapshots; update the logic that checks
evictedSession?.workspaceRootPath so it uses a cached/previous value when
available (e.g., store lastKnownWorkspaceRoot for the session) until
activeWorkspace/lookup finishes, then call
hasFilesPageSessionForWorkspaceRoot(lastKnownWorkspaceRoot, evicted) and only
call clearDirtyBuffersForWorkspace(lastKnownWorkspaceRoot) if that lookup
confirms no active session; apply the same change to the other occurrences
referencing evictedSession.workspaceRootPath (the blocks that call
hasFilesPageSessionForWorkspaceRoot and clearDirtyBuffersForWorkspace) so we
never pass null and prematurely clear buffers.
- Around line 114-117: getFilesPageSession currently calls touchFilesPageSession
during render, mutating filesPageSessionLru; change it to perform a non-touching
read by returning filesPageSessionByScope.get(sessionKey) without calling
touchFilesPageSession, and add a useEffect in the FilesPage component that calls
touchFilesPageSession(sessionKey) after mount/commit and whenever sessionKey
changes (the existing touch in the sessionKey-change effect at line ~506 can
remain); ensure all other touches are moved to committed effects so no LRU
mutations happen during render.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0535d6db-8ac4-4647-9d87-56d88476e3e3
📒 Files selected for processing (6)
apps/desktop/src/main/main.tsapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/components/settings/GeneralSection.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/components/terminals/TerminalView.tsx
Comment on lines
+114
to
+117
| function getFilesPageSession(sessionKey: string): FilesPageSessionState | undefined { | ||
| const session = filesPageSessionByScope.get(sessionKey); | ||
| if (session) touchFilesPageSession(sessionKey); | ||
| return session; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'createRoot|StrictMode' apps/desktop/srcRepository: arul28/ADE
Length of output: 4531
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/files/FilesPage.tsx | sed -n '110,125p'Repository: arul28/ADE
Length of output: 790
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/files/FilesPage.tsx | sed -n '415,430p'Repository: arul28/ADE
Length of output: 1231
🏁 Script executed:
rg -n -B5 -A5 'filesPageSessionLru|filesPageSessionByScope' apps/desktop/src/renderer/components/files/FilesPage.tsxRepository: arul28/ADE
Length of output: 2626
🏁 Script executed:
rg -n 'touchFilesPageSession' apps/desktop/src/renderer/components/files/FilesPage.tsxRepository: arul28/ADE
Length of output: 211
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/files/FilesPage.tsx | sed -n '415,500p'Repository: arul28/ADE
Length of output: 5370
🏁 Script executed:
rg -n 'useEffect|useCallback' apps/desktop/src/renderer/components/files/FilesPage.tsx | head -20Repository: arul28/ADE
Length of output: 1070
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/files/FilesPage.tsx | sed -n '487,520p'Repository: arul28/ADE
Length of output: 1710
🏁 Script executed:
rg -n 'getFilesPageSession|touchFilesPageSession' apps/desktop/src/renderer/components/files/FilesPage.tsxRepository: arul28/ADE
Length of output: 420
🏁 Script executed:
cat -n apps/desktop/src/renderer/components/files/FilesPage.tsx | sed -n '482,540p'Repository: arul28/ADE
Length of output: 2633
Use a non-touching read for the initial snapshot; move the LRU touch to an effect.
getFilesPageSession() mutates filesPageSessionLru at line 421 during render. In React 18, discarded or replayed renders can reorder that LRU, making later evictions inconsistent with actual committed scope switches. Extract a non-touching read (e.g., filesPageSessionByScope.get(sessionKey)) for the initial snapshot, and add a useEffect to touch the session after mount. The touch at line 506 (inside the sessionKey change effect) is already safe, but for consistency, move all LRU touches to committed effects only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/files/FilesPage.tsx` around lines 114 -
117, getFilesPageSession currently calls touchFilesPageSession during render,
mutating filesPageSessionLru; change it to perform a non-touching read by
returning filesPageSessionByScope.get(sessionKey) without calling
touchFilesPageSession, and add a useEffect in the FilesPage component that calls
touchFilesPageSession(sessionKey) after mount/commit and whenever sessionKey
changes (the existing touch in the sessionKey-change effect at line ~506 can
remain); ensure all other touches are moved to committed effects so no LRU
mutations happen during render.
Comment on lines
+147
to
+151
| if ( | ||
| evictedSession?.workspaceRootPath | ||
| && !hasFilesPageSessionForWorkspaceRoot(evictedSession.workspaceRootPath, evicted) | ||
| ) { | ||
| clearDirtyBuffersForWorkspace(evictedSession.workspaceRootPath); |
There was a problem hiding this comment.
Preserve the last known workspaceRootPath until the workspace lookup resolves.
Both snapshot paths can write workspaceRootPath: null even when this session already has a known root from a previous snapshot. If cache trimming runs in that window, hasFilesPageSessionForWorkspaceRoot() can miss the active session and clearDirtyBuffersForWorkspace() will clear that workspace’s dirty-buffer entries. Please keep the previously cached root until activeWorkspace is available.
Suggested fix
useEffect(() => {
if (prevSessionKeyRef.current === sessionKey) return;
// Save current state under the old scope before switching
+ const previousWorkspaceRootPath =
+ activeWorkspace?.rootPath
+ ?? filesPageSessionByScope.get(prevSessionKeyRef.current)?.workspaceRootPath
+ ?? null;
setFilesPageSession(prevSessionKeyRef.current, snapshotFilesPageSessionState({
workspaceId,
- workspaceRootPath: activeWorkspace?.rootPath ?? null,
+ workspaceRootPath: previousWorkspaceRootPath,
allowPrimaryEdit,
selectedNodePath,
openTabs,
@@
useEffect(() => {
+ const currentWorkspaceRootPath =
+ activeWorkspace?.rootPath
+ ?? filesPageSessionByScope.get(sessionKey)?.workspaceRootPath
+ ?? null;
setFilesPageSession(sessionKey, snapshotFilesPageSessionState({
workspaceId,
- workspaceRootPath: activeWorkspace?.rootPath ?? null,
+ workspaceRootPath: currentWorkspaceRootPath,
allowPrimaryEdit,
selectedNodePath,
openTabs,Also applies to: 493-496, 523-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/files/FilesPage.tsx` around lines 147 -
151, The code currently reads evictedSession.workspaceRootPath directly and can
see null during interim snapshots; update the logic that checks
evictedSession?.workspaceRootPath so it uses a cached/previous value when
available (e.g., store lastKnownWorkspaceRoot for the session) until
activeWorkspace/lookup finishes, then call
hasFilesPageSessionForWorkspaceRoot(lastKnownWorkspaceRoot, evicted) and only
call clearDirtyBuffersForWorkspace(lastKnownWorkspaceRoot) if that lookup
confirms no active session; apply the same change to the other occurrences
referencing evictedSession.workspaceRootPath (the blocks that call
hasFilesPageSessionForWorkspaceRoot and clearDirtyBuffersForWorkspace) so we
never pass null and prematurely clear buffers.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.tsx (1)
1037-1041:⚠️ Potential issue | 🟡 Minor
neutral/skippedare treated as passing in header, but not in row rendering/order.With this change, the summary can show “all passing” while individual checks still render as non-passing (
Eye) and stay inotherChecks. Align the per-check pass predicate everywhere.💡 Suggested consistency fix
+function isPassingConclusion(conclusion: PrCheck["conclusion"] | null | undefined): boolean { + return conclusion === "success" || conclusion === "neutral" || conclusion === "skipped"; +} function CheckRow({ check }: { check: PrCheck }) { - const isPassing = check.conclusion === "success"; + const isPassing = isPassingConclusion(check.conclusion); const isFailing = check.conclusion === "failure";-const allChecksPassing = checks.length > 0 && checks.every((c) => c.conclusion === "success" || c.conclusion === "neutral" || c.conclusion === "skipped"); -const passingChecks = checks.filter((c) => c.conclusion === "success"); +const allChecksPassing = checks.length > 0 && checks.every((c) => isPassingConclusion(c.conclusion)); +const passingChecks = checks.filter((c) => isPassingConclusion(c.conclusion)); const otherChecks = checks.filter( - (c) => c.conclusion !== "failure" && c.conclusion !== "success" && c.status !== "in_progress", + (c) => c.conclusion !== "failure" && !isPassingConclusion(c.conclusion) && c.status !== "in_progress", );🤖 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/PrConvergencePanel.tsx` around lines 1037 - 1041, The header uses the predicate that treats "neutral" and "skipped" as passing (allChecksPassing), but row/order code uses different predicates; update the per-check predicates to match: change passingChecks to include conclusions "success", "neutral", and "skipped" (use the same predicate as allChecksPassing for individual pass detection) and change otherChecks to exclude those three conclusions (and still exclude in_progress) so rows/order and header are consistent; refer to the variables allChecksPassing, passingChecks, otherChecks and the checks array when making the changes.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/prs/prService.ts (1)
1518-1518: Prefer structuredlogger.warnoverconsole.warnfor fetch fallbacks.These catches are a good resiliency improvement, but
console.warnbypasses structured main-process logging. Uselogger.warnwith consistent event keys and context (prId,sha, call site), while keeping the same safe fallback returns.♻️ Suggested logging pattern
- headSha ? fetchCheckRuns(repo, headSha).catch((err) => { console.warn("[prService] fetchCheckRuns failed in refreshOne:", err?.message ?? err); return []; }) : Promise.resolve([]), + headSha + ? fetchCheckRuns(repo, headSha).catch((error) => { + logger.warn("prs.fetch_check_runs_failed", { + scope: "refreshOne", + prId: row.id, + headSha, + error: getErrorMessage(error), + }); + return []; + }) + : Promise.resolve([]), - fetchCombinedStatus(repo, headSha).catch((err) => { console.warn("[prService] fetchCombinedStatus failed in getChecks:", err?.message ?? err); return { state: "", statuses: [] }; }), + fetchCombinedStatus(repo, headSha).catch((error) => { + logger.warn("prs.fetch_combined_status_failed", { + scope: "getChecks", + prId, + headSha, + error: getErrorMessage(error), + }); + return { state: "", statuses: [] }; + }),Also applies to: 1581-1581, 1633-1634
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/prs/prService.ts` at line 1518, Replace console.warn fallbacks with the structured logger.warn and include contextual fields: when calling fetchCheckRuns(headSha) in refreshOne (and the similar catch sites for fetchCheckSuites / fetchCheckRuns elsewhere), catch the error and call logger.warn with a descriptive message and an object containing prId (or pr.id), sha/headSha, and a callSite or tag like "prService:refreshOne:fetchCheckRuns", then return the same safe fallback ([]). Update the other catch blocks noted (the other fetchCheckRuns/fetchCheckSuites catches in the same file) to follow the same pattern so all fallback warnings use structured logging and consistent event keys.apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx (1)
1684-1684: Consider memoizing unified checks to avoid redundant computation.
buildUnifiedChecks(checks, actionRuns)is called here in the tab definition, and again insideOverviewTab(line 2262) andChecksTab(line 3166). SinceDETAIL_TABSis recalculated on every render, this means the same deduplication/sorting logic runs multiple times per render cycle.For a small number of checks this is negligible, but consider lifting a single
useMemocall toPrDetailPaneand passing the result down to child components.♻️ Suggested refactor
+ const unifiedChecks = React.useMemo( + () => buildUnifiedChecks(checks, actionRuns), + [checks, actionRuns], + ); const DETAIL_TABS: Array<{ id: DetailTab; label: string; icon: React.ElementType; count?: number }> = [ { id: "overview", label: "Overview", icon: Eye }, { id: "convergence", label: "Path to Merge", icon: Sparkle, count: newIssueCount > 0 ? newIssueCount : undefined }, { id: "files", label: "Files", icon: Code, count: files.length }, - { id: "checks", label: "CI / Checks", icon: Play, count: buildUnifiedChecks(checks, actionRuns).length }, + { id: "checks", label: "CI / Checks", icon: Play, count: unifiedChecks.length }, { id: "activity", label: "Activity", icon: ClockCounterClockwise, count: activity.length > 0 ? activity.length : (comments.length + reviews.length) }, ];Then pass
unifiedCheckstoOverviewTabandChecksTabinstead of having each compute it independently.🤖 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` at line 1684, DETAIL_TABS currently calls buildUnifiedChecks multiple times per render (also invoked inside OverviewTab and ChecksTab); lift this into PrDetailPane by computing const unifiedChecks = useMemo(() => buildUnifiedChecks(checks, actionRuns), [checks, actionRuns]) and pass unifiedChecks as a prop into OverviewTab and ChecksTab (and replace other direct buildUnifiedChecks calls), so the deduplication/sorting runs once per relevant dependency change rather than on every render.
🤖 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/prs/issueInventoryService.ts`:
- Around line 75-79: The current logic in issueInventoryService (when checking
the GitHub App bot via the name variable and returning baseName) can produce raw
bot slugs that collide with reserved literals; change the fallback to namespace
bot-derived sources (e.g. return a namespaced value like `bot:${baseName}`
instead of baseName) or persist the raw app name separately, and update the
shared IssueSource type to include a `bot:${string}` pattern and explicit
fallbacks (e.g. "bot" | "unknown" | specific providers) so downstream filtering
and the reopen exemption logic (referencing the IssueSource type) treat bot apps
distinctly from human/known sources.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.tsx`:
- Around line 1037-1041: The header uses the predicate that treats "neutral" and
"skipped" as passing (allChecksPassing), but row/order code uses different
predicates; update the per-check predicates to match: change passingChecks to
include conclusions "success", "neutral", and "skipped" (use the same predicate
as allChecksPassing for individual pass detection) and change otherChecks to
exclude those three conclusions (and still exclude in_progress) so rows/order
and header are consistent; refer to the variables allChecksPassing,
passingChecks, otherChecks and the checks array when making the changes.
---
Nitpick comments:
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Line 1518: Replace console.warn fallbacks with the structured logger.warn and
include contextual fields: when calling fetchCheckRuns(headSha) in refreshOne
(and the similar catch sites for fetchCheckSuites / fetchCheckRuns elsewhere),
catch the error and call logger.warn with a descriptive message and an object
containing prId (or pr.id), sha/headSha, and a callSite or tag like
"prService:refreshOne:fetchCheckRuns", then return the same safe fallback ([]).
Update the other catch blocks noted (the other fetchCheckRuns/fetchCheckSuites
catches in the same file) to follow the same pattern so all fallback warnings
use structured logging and consistent event keys.
In `@apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx`:
- Line 1684: DETAIL_TABS currently calls buildUnifiedChecks multiple times per
render (also invoked inside OverviewTab and ChecksTab); lift this into
PrDetailPane by computing const unifiedChecks = useMemo(() =>
buildUnifiedChecks(checks, actionRuns), [checks, actionRuns]) and pass
unifiedChecks as a prop into OverviewTab and ChecksTab (and replace other direct
buildUnifiedChecks calls), so the deduplication/sorting runs once per relevant
dependency change rather than on every render.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 473fef6d-becd-411b-bf0e-c49819f44af3
📒 Files selected for processing (9)
apps/desktop/src/main/services/ai/providerTaskRunner.tsapps/desktop/src/main/services/feedback/feedbackReporterService.tsapps/desktop/src/main/services/prs/issueInventoryService.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/renderer/components/app/FeedbackReporterModal.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/shared/PrConvergencePanel.tsxapps/desktop/src/shared/types/feedback.tsapps/desktop/src/shared/types/prs.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/shared/types/feedback.ts
Comment on lines
+75
to
79
| // Auto-detect any GitHub App bot (has [bot] suffix or "bot" in the name) | ||
| if (/\[bot\]/i.test(name)) { | ||
| // Return the base name as-is (e.g. "sonarqube-review" → "sonarqube-review") | ||
| return baseName; | ||
| } |
There was a problem hiding this comment.
Don’t persist raw bot slugs as first-class sources.
Returning baseName verbatim can collide with reserved literals. A bot named human[bot] or ade[bot] would be stored as "human"/"ade", which changes downstream filtering and the Line 776 reopen exemption. Namespace the generic fallback or store the raw app name separately.
Suggested direction
- if (/\[bot\]/i.test(name)) {
- // Return the base name as-is (e.g. "sonarqube-review" → "sonarqube-review")
- return baseName;
- }
+ if (/\[bot\]/i.test(name)) {
+ return `bot:${baseName}`;
+ }Then tighten the shared type to an explicit fallback such as:
export type IssueSource =
| "coderabbit"
| "codex"
| "copilot"
| "human"
| "ade"
| "greptile"
| "seer"
| "bot"
| "unknown"
| `bot:${string}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/prs/issueInventoryService.ts` around lines 75
- 79, The current logic in issueInventoryService (when checking the GitHub App
bot via the name variable and returning baseName) can produce raw bot slugs that
collide with reserved literals; change the fallback to namespace bot-derived
sources (e.g. return a namespaced value like `bot:${baseName}` instead of
baseName) or persist the raw app name separately, and update the shared
IssueSource type to include a `bot:${string}` pattern and explicit fallbacks
(e.g. "bot" | "unknown" | specific providers) so downstream filtering and the
reopen exemption logic (referencing the IssueSource type) treat bot apps
distinctly from human/known sources.
- Fix stale test expecting greptile-review[bot] → "unknown" (now → "greptile") - Add 6 focused tests: null/empty, known aliases, bot suffix auto-detect, unknown bots, "bot" keyword fallback, human authors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Assorted fixes across main process, renderer components, and tests including terminal session handling, agent chat UX, file watcher, and store updates.
Summary by CodeRabbit
New Features
Improvements
Settings
Greptile Summary
This PR delivers a broad set of correctness and memory-management improvements: per-project context eviction in the main process, terminal output buffering while a view is unmounted, chat-event history caps with LRU pruning, files-page session LRU, and CI/Checks treating
skipped/neutralas passing. All changes are well-scoped and the added tests cover the new behaviours.The only items worth a second look are both P2: the
settledguard inproviderTaskRunnerappears to be dead code as written (it is never settruein child-exit handlers), and theprServicecatch blocks useconsole.warninstead of the structured logger.Confidence Score: 5/5
Safe to merge — all findings are P2 style/cleanup issues with no correctness impact.
No P0 or P1 issues found. Terminal buffering, context eviction, chat-event trimming, and CI-check reclassification are all correctly implemented and covered by new tests. The three P2 findings (dead
settledguard,console.warninstead of logger, unbounded dirty-tab accumulation) do not affect runtime correctness.apps/desktop/src/main/services/ai/providerTaskRunner.ts (settled guard), apps/desktop/src/main/services/prs/prService.ts (console.warn)
Important Files Changed
refreshLockedSessionSummaryto handle locked-mode refreshes independently.scheduleFrameWriteFlush; PTY output is now buffered while the view is unmounted (refs === 0) and replayed on remount, preserving live terminal TUI state.suspendedprop toSessionSurface; non-active grid tiles render a lightweight preview card instead of mounting a full terminal/chat pane.detectSourcefor testing.settledflag to the timeout handler; the guard may be incomplete if child-exit handlers don't also setsettled = true.deltaCachefrom a plain Map to an LRU cache capped at 128 entries usingtouchDeltaCacheEntry.console.warnlogging to previously silent catch blocks; inconsistent with the structured logger used elsewhere in the file.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User switches active project] --> B[setActiveProject\nupdates projectLastActivatedAt] B --> C[scheduleProjectContextRebalance\nchains onto serial promise queue] C --> D[rebalanceProjectContexts] D --> E{For each non-active\nproject context} E --> F[hasActiveProjectWorkloads?] F -->|Yes| G[Retain — policy: active_workload] F -->|No| H[Add to idleRoots] H --> I[Sort by projectLastActivatedAt desc] I --> J{In top MAX_WARM_IDLE=1?} J -->|Yes| K[Retain — policy: warm_idle] J -->|No| L[Re-check workloads TOCTOU guard] L -->|Became active| M[Retain — policy: became_active_during_rebalance] L -->|Still idle| N[closeProjectContext\nevict + delete from map] subgraph TerminalParking T1[TerminalView unmounts] --> T2[refs dec to 0\nno RAF scheduled for flush] T2 --> T3[PTY data buffered in\nframeWriteChunks] T3 --> T4[TerminalView remounts] T4 --> T5[scheduleFrameWriteFlush\nreplays buffered output] endComments Outside Diff (1)
apps/desktop/src/renderer/components/files/FilesPage.tsx, line 489-514 (link)This
useEffectdeliberately omitsworkspaceId,openTabs,activeTabPath, etc. from its dependency array — it captures their values at the momentsessionKeychanges, which is the intended "snapshot before leaving" pattern. Without an explanatory comment the exhaustive-deps lint rule will flag these as missing, and future readers will be tempted to add them (which would break the guardif (prevSessionKeyRef.current === sessionKey) return). A brief// eslint-disable-next-line react-hooks/exhaustive-deps – intentional snapshot of outgoing statewould protect the intent.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "test: update detectSource tests for auto..." | Re-trigger Greptile