perf(desktop): M3 memory optimizations — idle webview unloading, terminal caps, chat message limit#2950
Closed
wendeus0 wants to merge 6 commits into
Closed
perf(desktop): M3 memory optimizations — idle webview unloading, terminal caps, chat message limit#2950wendeus0 wants to merge 6 commits into
wendeus0 wants to merge 6 commits into
Conversation
1.1 WebView Idle Unloading: destroy parked webviews after 30min of inactivity and recreate transparently on refocus. Adds suspended flag to Pane, lastActiveTimestamps registry, idle sweep timer in useBrowserLifecycle, and suspend/resume actions in tabs store. 1.2 Terminal Scrollback Hard Cap: enforce MAX_TERMINAL_SCROLLBACK (10,000 lines) in headless xterm emulator via Math.min(), preventing unbounded scrollback buffer growth. 1.3 Terminal Idle Session Culling: auto-kill terminal sessions with no attached clients after 1 hour of inactivity. Uses existing lastAttachedAt metadata and kill mechanism; sweep runs every 10 minutes. 1.4 React Query Cache Optimization: add staleTime (30s) and gcTime (5min) defaults to QueryClient, and invalidate all queries on workspace switch to prevent stale cross-workspace data accumulation.
…inal RSS limit - Chat: cap displayed messages at 100, add "Load earlier messages" button to reveal older history on demand (use-chat-display, ChatMessageList) - Terminal: kill sessions whose process tree RSS exceeds 512 MB, using captureProcessSnapshot + getSubtreeResources (every 5 min sweep)
- resumeBrowserPane: add pane.type !== "webview" guard (mirrors suspendBrowserPane) - useBrowserLifecycle idle sweep: check layout visibility instead of focus; prevents suspending visible-but-unfocused panes in split layouts - usePersistentWebview: read `suspended` via ref in main lifecycle effect, removing it from deps to break the resumeBrowserPane → re-trigger loop that caused immediate teardown/re-attach on every restore-from-suspension - use-chat-display: reset showAllMessages to false on sessionId change, preventing "Load earlier messages" state from leaking across sessions
…allowing silently
Contributor
📝 WalkthroughWalkthroughClamps terminal scrollback, adds TerminalHost idle and RSS sweeps, tracks and suspends idle browser webviews with last-active timestamps, adds chat message pagination with "load all", tweaks TanStack Query timings, and invalidates workspace queries on workspaceId change. Changes
Sequence Diagram(s)sequenceDiagram
participant IdleTimer as IdleSweep Timer
participant RSSTimer as RssSweep Timer
participant TH as TerminalHost
participant Snapshot as captureProcessSnapshot
participant Resources as getSubtreeResources
participant Kill as kill(sessionId)
IdleTimer->>TH: tick (every 10min)
TH->>TH: filter attachable sessions with clientCount === 0
TH->>TH: compare lastAttachedAt vs 1h timeout
alt idle exceeded
TH->>Kill: kill(sessionId, deleteHistory: false)
end
RSSTimer->>TH: tick (every 5min)
TH->>Snapshot: captureProcessSnapshot()
alt snapshot succeeds
Snapshot-->>TH: process tree
TH->>Resources: getSubtreeResources(tree)
Resources-->>TH: per-session RSS
alt RSS > 512MB
TH->>Kill: kill(sessionId, deleteHistory: false)
end
else snapshot fails
Snapshot-->>TH: error
TH->>TH: log and abort sweep
end
sequenceDiagram
participant App as Application
participant Lifecycle as useBrowserLifecycle
participant Store as TabsStore
participant WebviewMgr as usePersistentWebview
participant Cleanup as destroyPersistentWebview
participant Inactive as lastActiveTimestamps
Lifecycle->>Lifecycle: tick (every 5min)
Lifecycle->>Store: read panes & active tab layouts
Lifecycle->>Lifecycle: compute visible pane IDs
loop each pane of type "webview"
Lifecycle->>Inactive: read/update lastActiveTimestamps[paneId]
alt pane not visible and not suspended and idle > 30min
Lifecycle->>Cleanup: destroyPersistentWebview(paneId)
Cleanup->>WebviewMgr: remove DOM node and clear registries
Cleanup->>Inactive: delete lastActiveTimestamps[paneId]
Lifecycle->>Store: unregisterBrowser({ paneId })
Lifecycle->>Store: suspendBrowserPane(paneId)
else pane visible
Lifecycle->>Inactive: clear lastActiveTimestamps[paneId]
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
destroyPersistentWebview removes paneId from webviewRegistry, but the effect cleanup unconditionally called lastActiveTimestamps.set and getHiddenContainer().appendChild, resurrecting the removed node. Now checks webviewRegistry.has(paneId) before re-parking.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Author
|
Updated: added fix for The cleanup function in Fix: gate the re-park block on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/lib/terminal-host/headless-emulator.ts`:
- Line 92: The current assignment only caps the upper bound for scrollback but
allows negative, NaN, or non-integer values; compute a sanitized numeric value
first (convert scrollback to Number, treat non-finite/NaN as 0), clamp it to the
range [0, MAX_TERMINAL_SCROLLBACK], and ensure it's an integer (e.g., floor)
before passing into Terminal(...); update the code that constructs the Terminal
(the scrollback expression referencing scrollback and MAX_TERMINAL_SCROLLBACK)
to use this sanitized value.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx:
- Around line 95-103: The current call to
electronQueryClient.invalidateQueries() is too broad and invalidates all cached
queries; change it to only invalidate workspace-scoped queries when workspaceId
changes by using the prevWorkspaceIdRef and a predicate or specific queryKey
filter on electronQueryClient.invalidateQueries so only queries associated with
the old workspace are invalidated (e.g., match query keys that include
'workspace' or prevWorkspaceIdRef.current); update the block that references
prevWorkspaceIdRef, workspaceId, and electronQueryClient.invalidateQueries to
use that targeted filter instead of the unfiltered invalidateQueries() call.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts`:
- Around line 392-394: The cleanup in the effect always re-attaches the
closed-over wv even if destroyPersistentWebview() already removed/destroyed it;
add a guard in the effect cleanup to avoid re-parking a destroyed webview: have
destroyPersistentWebview(mark) the webview as destroyed (e.g. add the wv or
paneId to a destroyedWebviews Set) and in the effect cleanup (the code that
currently calls lastActiveTimestamps.set(paneId, Date.now());
getHiddenContainer().appendChild(wv);) check that the webview has not been
destroyed before appending (e.g. if (!destroyedWebviews.has(wv) &&
!wv.isConnected) then record timestamp and append), or otherwise skip the
append; reference the effect cleanup, wv, paneId, lastActiveTimestamps,
getHiddenContainer(), and destroyPersistentWebview() when adding the guard.
- Around line 111-117: The effect that manages webview lifecycle in
usePersistentWebview currently reads suspended via suspendedRef to avoid a
self-loop but lacks a visibility/active-pane trigger, causing suspended panes to
stay blank after tab switches; modify the hook to accept (or read) an explicit
visibility flag (e.g., isActive or isVisible from the TabView/parent) and
include that flag in the main lifecycle effect's dependency list so the
creation/resume branch runs when the pane becomes visible again; update
references to suspendedRef and resumeBrowserPane (and the same logic around
lines noted: 111-117, 212-223, 397-405) to check suspendedRef.current but re-run
the effect when the new visibility prop flips true to deterministically
recreate/resume the webview.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPaneInterface/components/ChatMessageList/ChatMessageList.tsx`:
- Around line 179-189: The "Load earlier messages" button handler (rendered when
hasMoreMessages && onLoadAllMessages in ChatMessageList) needs to preserve the
user's viewport when many messages are prepended; before invoking
onLoadAllMessages capture the scroll container element and its scrollHeight and
scrollTop (or enable/adjust CSS scroll anchoring via overflow-anchor) and after
the new messages render restore scrollTop by computing newScrollTop =
newScrollHeight - oldScrollHeight + oldScrollTop (or use element.scrollTo with
that value); update the onClick flow around the onLoadAllMessages invocation in
the ChatMessageList component to perform this capture/restore so the viewport
does not jump when messages are loaded.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/hooks/useBrowserLifecycle/useBrowserLifecycle.ts`:
- Around line 48-75: The sweep currently uses lastActiveTimestamps.get(paneId)
?? now which resets the "hidden since" time each interval; change the logic so
when a webview pane transitions from visible to hidden you persist the first
hidden timestamp into lastActiveTimestamps (set lastActiveTimestamps.set(paneId,
Date.now()) if no entry exists), then in the sweep always use that stored
timestamp (do not fallback to now). Also ensure you clear the entry
(lastActiveTimestamps.delete(paneId)) when the pane becomes visible again or is
reclaimed/unmounted (e.g. in
destroyPersistentWebview/unregisterBrowser/suspendBrowserPane flows) so timing
is correct; reference functions/variables: lastActiveTimestamps,
IDLE_WEBVIEW_TIMEOUT_MS, extractPaneIdsFromLayout, destroyPersistentWebview,
unregisterBrowser, useTabsStore.getState().suspendBrowserPane.
In `@packages/chat/src/client/hooks/use-chat-display/use-chat-display.ts`:
- Around line 165-167: The useEffect currently lists sessionId in its dependency
array but never references it; update the effect body in use-chat-display.ts to
directly read sessionId (for example, guard on it being defined or different)
and then call setShowAllMessages(false) when sessionId changes so the effect
actually uses sessionId and satisfies the exhaustive-deps lint rule—modify the
effect that contains useEffect, setShowAllMessages, and sessionId to reference
sessionId explicitly (e.g., a conditional check) before resetting show-all
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45d73abc-6e08-488d-8b4e-5de6e0c56c62
📒 Files selected for processing (16)
apps/desktop/src/main/lib/terminal-host/headless-emulator.tsapps/desktop/src/main/terminal-host/terminal-host-rss-sweep.test.tsapps/desktop/src/main/terminal-host/terminal-host.tsapps/desktop/src/renderer/providers/ElectronTRPCProvider/ElectronTRPCProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPaneInterface/ChatPaneInterface.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPaneInterface/components/ChatMessageList/ChatMessageList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPaneInterface/components/ChatMessageList/ChatMessageList.types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/hooks/useBrowserLifecycle/useBrowserLifecycle.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/constants.tsapps/desktop/src/shared/tabs-types.tspackages/chat/src/client/hooks/use-chat-display/use-chat-display.ts
Comment on lines
+95
to
+103
| useEffect(() => { | ||
| if ( | ||
| prevWorkspaceIdRef.current !== null && | ||
| prevWorkspaceIdRef.current !== workspaceId | ||
| ) { | ||
| void electronQueryClient.invalidateQueries(); | ||
| } | ||
| prevWorkspaceIdRef.current = workspaceId; | ||
| }, [workspaceId]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Unscoped invalidateQueries() calls"
rg -nP --type=ts --type=tsx 'invalidateQueries\(\s*\)' apps/desktop/src -C2
echo
echo "2) Workspace page invalidation call site"
rg -nP --type=tsx 'prevWorkspaceIdRef|invalidateQueries\(' 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx' -C3
echo
echo "3) Example non-workspace query namespaces in renderer (potentially invalidated today)"
rg -nP --type=ts --type=tsx 'electronTrpc\.(auth|projects|settings|users)\..*\.useQuery' apps/desktop/src -C1Repository: superset-sh/superset
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Unscoped invalidateQueries() calls ==="
rg -n 'invalidateQueries\(\s*\)' apps/desktop/src --type ts -C2 2>/dev/null || echo "No matches for ts files"
echo
echo "=== 2) Workspace page invalidation call site ==="
cat -n 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx' | head -110 | tail -30
echo
echo "=== 3) Search for all invalidateQueries calls ==="
rg -n 'invalidateQueries' apps/desktop/src --type ts -C1
echo
echo "=== 4) Find query definitions in tRPC router ==="
fd -t f -e ts -e tsx 'packages/trpc' | head -20Repository: superset-sh/superset
Length of output: 2561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Find tRPC router structure ==="
fd -t f . 'packages/trpc' | head -30
echo
echo "=== 2) Find electronTrpc usage patterns in renderer ==="
rg -n 'electronTrpc\.' apps/desktop/src --type ts -o | head -30
echo
echo "=== 3) Look for workspace queries ==="
rg -n 'electronTrpc\.workspaces\.' apps/desktop/src/renderer/routes --type ts -C1 | head -50
echo
echo "=== 4) Look for non-workspace queries ==="
rg -n 'electronTrpc\.(auth|settings|projects|files)' apps/desktop/src/renderer --type ts | head -20Repository: superset-sh/superset
Length of output: 12966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Look for query key structure/factory ==="
rg -n 'queryKey|getQueryKey' apps/desktop/src --type ts -C1 | head -50
echo
echo "=== 2) Check tRPC index to understand query namespace structure ==="
cat apps/desktop/src/lib/trpc/index.ts | head -50
echo
echo "=== 3) Find where electronQueryClient is created ==="
rg -n 'electronQueryClient' apps/desktop/src/renderer --type ts -C2 | head -40
echo
echo "=== 4) Look at actual query usage to understand keys ==="
rg -n 'useQuery\(' apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/\$workspaceId/page.tsx -C2Repository: superset-sh/superset
Length of output: 12907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Check electronTrpc definition and query key generation ==="
rg -n 'electronTrpc' apps/desktop/src/renderer/lib --type ts -C2 | head -50
echo
echo "=== 2) Search for createTRPCReact usage ==="
rg -n 'createTRPCReact' apps/desktop/src --type ts -C3
echo
echo "=== 3) Find workspace router definition ==="
cat apps/desktop/src/lib/trpc/routers/workspaces/index.ts 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 5922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Find workspace router structure ==="
cat apps/desktop/src/lib/trpc/routers/workspaces/index.ts 2>/dev/null | head -150
echo
echo "=== 2) Check if there are any workspace-related queries outside 'workspaces' namespace ==="
rg -n 'useQuery\(' apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/\$workspaceId/page.tsx -A2
echo
echo "=== 3) Look at full page to see all query calls ==="
wc -l apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/\$workspaceId/page.tsxRepository: superset-sh/superset
Length of output: 988
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Get workspace router file path ==="
find apps/desktop/src/lib/trpc/routers/workspaces -name "*.ts" | head -20
echo
echo "=== 2) Look at workspace router structure ==="
head -100 apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts
echo
echo "=== 3) Verify query key generation with tRPC docs understanding ==="
rg -n 'getQueryKey|createTRPCProxyClient' apps/desktop/src/renderer/lib --type ts -C2Repository: superset-sh/superset
Length of output: 4103
Scope workspace-switch invalidation; current call invalidates all queries including non-workspace data.
At line 100, invalidateQueries() without filters marks all cached queries stale and refetches active ones, including unrelated queries like projects.getDefaultApp and settings data. This creates unnecessary churn on every workspace navigation.
💡 Suggested change
useEffect(() => {
if (
prevWorkspaceIdRef.current !== null &&
prevWorkspaceIdRef.current !== workspaceId
) {
- void electronQueryClient.invalidateQueries();
+ void electronQueryClient.invalidateQueries({
+ predicate: (query) => {
+ const [scope] = query.queryKey;
+ return Array.isArray(scope) && scope[0] === "workspaces";
+ },
+ refetchType: "none",
+ });
}
prevWorkspaceIdRef.current = workspaceId;
}, [workspaceId]);📝 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
| useEffect(() => { | |
| if ( | |
| prevWorkspaceIdRef.current !== null && | |
| prevWorkspaceIdRef.current !== workspaceId | |
| ) { | |
| void electronQueryClient.invalidateQueries(); | |
| } | |
| prevWorkspaceIdRef.current = workspaceId; | |
| }, [workspaceId]); | |
| useEffect(() => { | |
| if ( | |
| prevWorkspaceIdRef.current !== null && | |
| prevWorkspaceIdRef.current !== workspaceId | |
| ) { | |
| void electronQueryClient.invalidateQueries({ | |
| predicate: (query) => { | |
| const [scope] = query.queryKey; | |
| return Array.isArray(scope) && scope[0] === "workspaces"; | |
| }, | |
| refetchType: "none", | |
| }); | |
| } | |
| prevWorkspaceIdRef.current = workspaceId; | |
| }, [workspaceId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx
around lines 95 - 103, The current call to
electronQueryClient.invalidateQueries() is too broad and invalidates all cached
queries; change it to only invalidate workspace-scoped queries when workspaceId
changes by using the prevWorkspaceIdRef and a predicate or specific queryKey
filter on electronQueryClient.invalidateQueries so only queries associated with
the old workspace are invalidated (e.g., match query keys that include
'workspace' or prevWorkspaceIdRef.current); update the block that references
prevWorkspaceIdRef, workspaceId, and electronQueryClient.invalidateQueries to
use that targeted filter instead of the unfiltered invalidateQueries() call.
- headless-emulator: sanitize scrollback input before Math.min — guard against NaN, non-finite and negative values with Number()/isFinite/floor - page.tsx: scope workspace query invalidation to the active workspace id via electronTrpc.useUtils() instead of the global electronQueryClient - usePersistentWebview: add isActiveTab selector so suspended panes only recreate their webview when their tab is actually focused; guard effect body with early return when webview is absent and tab is inactive - useBrowserLifecycle: fix idle sweep ?? now fallback that reset the hidden-since clock on every cycle; record first-hidden timestamp on initial observation and clear it when the pane becomes visible again - ChatMessageList: preserve scroll position when loading earlier messages by capturing scrollHeight/scrollTop before and restoring after rAF - use-chat-display: reference sessionId in showAllMessages reset effect body so the dep is used consistently (fixes lint warning)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx (1)
94-95:⚠️ Potential issue | 🔴 CriticalFix duplicate
utilsdeclaration inWorkspacePage(build blocker).Line 94 introduces
const utils = electronTrpc.useUtils();, but the same identifier is already declared at Line 381 in the same function scope. This is a duplicate identifier error that will prevent the code from building.💡 Suggested fix
Rename the first
utilsdeclaration totrpcUtilsand update its usage:- const utils = electronTrpc.useUtils(); + const trpcUtils = electronTrpc.useUtils(); const prevWorkspaceIdRef = useRef<string | null>(null); useEffect(() => { const prevId = prevWorkspaceIdRef.current; prevWorkspaceIdRef.current = workspaceId; if (prevId !== null && prevId !== workspaceId) { - void utils.workspaces.get.invalidate({ id: workspaceId }); + void trpcUtils.workspaces.get.invalidate({ id: workspaceId }); } - }, [workspaceId, utils]); + }, [workspaceId, trpcUtils]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx around lines 94 - 95, The function WorkspacePage currently declares const utils = electronTrpc.useUtils(); which duplicates another utils declaration later in the same scope and breaks the build; rename the first instance (the electronTrpc.useUtils() binding) to trpcUtils and update all places in WorkspacePage that reference that first binding (e.g., any uses near prevWorkspaceIdRef or initializations that rely on electronTrpc.useUtils()) to use trpcUtils so the later utils declaration can remain unchanged.
🤖 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/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts`:
- Around line 202-203: The reclaim path currently updates
lastActiveTimestamps.set(paneId, Date.now()) which contradicts the semantics in
useBrowserLifecycle (where timestamps are deleted when a pane becomes visible);
instead remove the timestamp on reclaim to mark the pane as active. Replace the
set operation with deletion of the key (use lastActiveTimestamps.delete(paneId))
in the reclaim branch so visibility logic in useBrowserLifecycle and
first-observation logic for hidden panes remain consistent.
- Around line 201-205: The reclaim branch currently unconditionally re-attaches
a parked webview which resets lastActiveTimestamps and defeats the parking
mechanism; update the code in usePersistentWebview so that when webview exists
you only append and call syncStoreFromWebview if isActiveTab is true (i.e., gate
the reclaim logic on isActiveTab), otherwise leave the webview parked and do not
touch lastActiveTimestamps or call syncStoreFromWebview for that paneId; target
the block that references webview, lastActiveTimestamps.set(paneId, Date.now()),
container.appendChild(webview) and syncStoreFromWebview(webview) to add this
conditional.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx:
- Around line 94-95: The function WorkspacePage currently declares const utils =
electronTrpc.useUtils(); which duplicates another utils declaration later in the
same scope and breaks the build; rename the first instance (the
electronTrpc.useUtils() binding) to trpcUtils and update all places in
WorkspacePage that reference that first binding (e.g., any uses near
prevWorkspaceIdRef or initializations that rely on electronTrpc.useUtils()) to
use trpcUtils so the later utils declaration can remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edb2ca0f-0ff6-496f-b3de-3f2e05b07853
📒 Files selected for processing (6)
apps/desktop/src/main/lib/terminal-host/headless-emulator.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/ChatPane/ChatPaneInterface/components/ChatMessageList/ChatMessageList.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/hooks/useBrowserLifecycle/useBrowserLifecycle.tspackages/chat/src/client/hooks/use-chat-display/use-chat-display.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/chat/src/client/hooks/use-chat-display/use-chat-display.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/hooks/useBrowserLifecycle/useBrowserLifecycle.ts
Comment on lines
201
to
205
| if (webview) { | ||
| // Reclaim from hidden container | ||
| // Reclaim from hidden container — update activity timestamp | ||
| lastActiveTimestamps.set(paneId, Date.now()); | ||
| container.appendChild(webview); | ||
| syncStoreFromWebview(webview); |
Contributor
There was a problem hiding this comment.
Reclaim branch ignores isActiveTab, defeating the parking mechanism.
When isActiveTab toggles from true to false (user switches away):
- Cleanup parks the webview into the hidden container
- Effect re-runs with
isActiveTab = false - The
if (webview)branch executes unconditionally and immediately reclaims the parked webview back into the (nowdisplay:none) container
This defeats the purpose of parking and resets the idle timestamp on every tab switch—background webviews will never reach the 30-minute idle threshold because the timestamp refreshes each time.
🐛 Proposed fix: gate reclaim on `isActiveTab`
- if (webview) {
+ if (webview && isActiveTab) {
// Reclaim from hidden container — update activity timestamp
- lastActiveTimestamps.set(paneId, Date.now());
+ lastActiveTimestamps.delete(paneId); // pane is visible now
container.appendChild(webview);
syncStoreFromWebview(webview);
} else if (isActiveTab) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts`
around lines 201 - 205, The reclaim branch currently unconditionally re-attaches
a parked webview which resets lastActiveTimestamps and defeats the parking
mechanism; update the code in usePersistentWebview so that when webview exists
you only append and call syncStoreFromWebview if isActiveTab is true (i.e., gate
the reclaim logic on isActiveTab), otherwise leave the webview parked and do not
touch lastActiveTimestamps or call syncStoreFromWebview for that paneId; target
the block that references webview, lastActiveTimestamps.set(paneId, Date.now()),
container.appendChild(webview) and syncStoreFromWebview(webview) to add this
conditional.
Comment on lines
+202
to
+203
| // Reclaim from hidden container — update activity timestamp | ||
| lastActiveTimestamps.set(paneId, Date.now()); |
Contributor
There was a problem hiding this comment.
Timestamp semantics mismatch: should delete on reclaim, not set.
useBrowserLifecycle (context snippet lines 69-72) deletes the timestamp when a pane becomes visible and only sets it on first observation of a hidden pane. Setting the timestamp here when reclaiming to a visible tab conflicts with that expectation—the timestamp should be deleted to indicate the pane is no longer idle.
♻️ Suggested change
if (webview && isActiveTab) {
- // Reclaim from hidden container — update activity timestamp
- lastActiveTimestamps.set(paneId, Date.now());
+ // Pane is visible again — clear idle timestamp
+ lastActiveTimestamps.delete(paneId);
container.appendChild(webview);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts`
around lines 202 - 203, The reclaim path currently updates
lastActiveTimestamps.set(paneId, Date.now()) which contradicts the semantics in
useBrowserLifecycle (where timestamps are deleted when a pane becomes visible);
instead remove the timestamp on reclaim to mark the pane as active. Replace the
set operation with deletion of the key (use lastActiveTimestamps.delete(paneId))
in the reclaim branch so visibility logic in useBrowserLifecycle and
first-observation logic for hidden panes remain consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR ports the M3 memory optimization work from the Linux fork back to main. All changes are platform-agnostic and apply to Windows/macOS as well.
Phase 1 — ~500MB–1.4GB reduction
suspendedflag toPane,lastActiveTimestampsregistry, idle sweep timer inuseBrowserLifecycle, andsuspend/resumeactions in the tabs store.MAX_TERMINAL_SCROLLBACK(10,000 lines) in the headless xterm emulator viaMath.min(), preventing unbounded scrollback buffer growth.lastAttachedAtmetadata and kill mechanism; sweep runs every 10 minutes.staleTime(30s) andgcTime(5min) defaults toQueryClient; invalidate workspace-scoped queries on workspace switch to prevent stale cross-workspace data accumulation.Phase 3
use-chat-display,ChatMessageList).captureProcessSnapshot+getSubtreeResources(sweep every 5 minutes). Includes 6 unit tests covering all sweep paths.Bug fixes (Sprint 2 — found during review)
resumeBrowserPane: addpane.type !== "webview"guard to matchsuspendBrowserPane, preventing non-webview panes from having their state mutated.useBrowserLifecycleidle sweep: check layout visibility instead of focus, so visible-but-unfocused panes in split layouts are not suspended.usePersistentWebview: readsuspendedvia ref in the main lifecycle effect, removing it from deps to break theresumeBrowserPane → re-triggerloop that caused immediate teardown/re-attach on every restore-from-suspension.use-chat-display: resetshowAllMessagestofalseonsessionIdchange, preventing "Load earlier messages" state from leaking across sessions.terminal-host: logcaptureProcessSnapshotfailures instead of swallowing them silently.Test plan
bun test apps/desktop/src/main/terminal-host/terminal-host-rss-sweep.test.ts— all 6 cases passSummary by cubic
Backports M3 memory optimizations to desktop: unload idle webviews, cap terminal resource usage (scrollback, idle, RSS), limit chat rendering, and tune
@tanstack/react-querycache to cut RAM by ~500MB–1.4GB in heavy sessions.New Features
staleTime30s andgcTime5m; invalidate workspace queries on workspace switch.Bug Fixes
suspendedvia ref.captureProcessSnapshotfailures; sanitize scrollback input to ignore NaN/negative values.Written for commit a959bc6. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests