Reduce renderer polling for resource and terminal surfaces#4581
Merged
Conversation
|
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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds developer-facing render/stress instrumentation and structured logging, refactors background-terminal polling and counting (client + host-service), introduces remote-host status policy and unavailable UI, gates resource-monitor querying with a policy, adds workspace-client idle eviction and ref counting, updates collection indices, and miscellaneous test/shell/script/compatibility fixes. ChangesRenderer dev instrumentation & main-process logging
Background terminals: query, UI, and host-service count
Remote host status policy and host-unavailable UI
Resource monitor policy and UI refactor
Dashboard sidebar data and hotkey behaviors
Workspace client cache & eviction policy
Collection indices and small infra updates
Host-service compatibility & tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 2
🧹 Nitpick comments (2)
packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.ts (1)
10-22: 💤 Low valueConsider documenting the best-effort eviction behavior.
The eviction logic correctly prioritizes idle entries and protects active clients, but when
entries.length > maxEntriesand insufficient idle entries exist (e.g., most entries haveactiveRefs > 0), the cache will temporarily exceedmaxEntries. This is the correct behavior—you cannot evict clients in use—but a brief comment would clarify the intent for future maintainers.📝 Optional clarifying comment
export function getIdleWorkspaceClientEvictionKeys( entries: readonly WorkspaceClientCachePolicyEntry[], maxEntries = MAX_WORKSPACE_CLIENT_CACHE_ENTRIES, protectedKey?: string, ): string[] { if (entries.length <= maxEntries) return []; + // Best-effort eviction: only idle entries (activeRefs === 0) are eligible. + // Cache may temporarily exceed maxEntries if most entries are active. const idleEntries = entries .filter((entry) => entry.activeRefs === 0 && entry.key !== protectedKey) .sort((left, right) => left.lastAccessedAt - right.lastAccessedAt);🤖 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 `@packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.ts` around lines 10 - 22, Add a brief comment to getIdleWorkspaceClientEvictionKeys explaining that eviction is best-effort: the function only evicts idle entries (entries.filtered by activeRefs === 0 and not matching protectedKey) and therefore if there are fewer idle entries than evictionCount the cache may temporarily exceed maxEntries until active clients become idle; mention the protectedKey behavior and that this is intentional to avoid evicting in-use clients.packages/workspace-client/test/workspaceClientCachePolicy.test.ts (1)
14-27: ⚡ Quick winExpand test coverage for edge cases.
The current tests validate the happy paths, but several edge cases remain untested:
- All entries active: What happens when
entries.length > maxEntriesbut every entry hasactiveRefs > 0? (Expected: returns[], cache exceeds limit temporarily)- Protected key is the only idle entry: Verify that eviction returns
[]when the sole idle entry is protected.- Empty entries array: Boundary check for
entries.length === 0.- Tie-breaking: Multiple entries with identical
lastAccessedAtvalues.These cases would clarify the best-effort eviction contract and prevent future regressions.
🧪 Proposed additional test cases
+ test("does not evict when all entries are active", () => { + expect( + getIdleWorkspaceClientEvictionKeys( + [ + { key: "active-1", activeRefs: 1, lastAccessedAt: 1 }, + { key: "active-2", activeRefs: 2, lastAccessedAt: 2 }, + { key: "active-3", activeRefs: 1, lastAccessedAt: 3 }, + ], + 2, + ), + ).toEqual([]); + }); + + test("does not evict when only idle entry is protected", () => { + expect( + getIdleWorkspaceClientEvictionKeys( + [ + { key: "active", activeRefs: 1, lastAccessedAt: 1 }, + { key: "protected-idle", activeRefs: 0, lastAccessedAt: 2 }, + ], + 1, + "protected-idle", + ), + ).toEqual([]); + });🤖 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 `@packages/workspace-client/test/workspaceClientCachePolicy.test.ts` around lines 14 - 27, Add tests for edge cases to workspaceClientCachePolicy.test.ts by extending the existing suite that calls getIdleWorkspaceClientEvictionKeys: (1) assert when entries.length > maxEntries but every entry has activeRefs > 0 the function returns [] (cache can temporarily exceed limit), (2) assert when the only idle entry matches the protected key the function returns [], (3) assert an empty entries array returns [], and (4) assert deterministic tie-breaking when multiple entries share identical lastAccessedAt (e.g., ensure stable ordering or expected keys are evicted). Use the same test structure as the "evicts least recently used idle clients first" test and reference getIdleWorkspaceClientEvictionKeys for each case.
🤖 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/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/WorkspaceHostUnavailableState.tsx`:
- Around line 41-43: The heading in WorkspaceHostUnavailableState (component
WorkspaceHostUnavailableState and the JSX h1 that renders {title}) is not
selectable; update the h1 element's className to include the error-text policy
classes "select-text cursor-text" (e.g., append " select-text cursor-text" to
the existing className string) so users can copy the error heading even when
body selection is disabled.
In `@packages/workspace-client/test/workspaceClientCachePolicy.test.ts`:
- Around line 1-28: Move the test so it lives next to the implementation of
getIdleWorkspaceClientEvictionKeys (co-locate the .test.ts with the source
file), then update the import in the moved test to import
getIdleWorkspaceClientEvictionKeys from the module file using a relative path
from the new test location, and delete the original test in the test/ directory;
ensure the test filename retains the .test.ts suffix and the test continues to
reference the getIdleWorkspaceClientEvictionKeys symbol unchanged.
---
Nitpick comments:
In
`@packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.ts`:
- Around line 10-22: Add a brief comment to getIdleWorkspaceClientEvictionKeys
explaining that eviction is best-effort: the function only evicts idle entries
(entries.filtered by activeRefs === 0 and not matching protectedKey) and
therefore if there are fewer idle entries than evictionCount the cache may
temporarily exceed maxEntries until active clients become idle; mention the
protectedKey behavior and that this is intentional to avoid evicting in-use
clients.
In `@packages/workspace-client/test/workspaceClientCachePolicy.test.ts`:
- Around line 14-27: Add tests for edge cases to
workspaceClientCachePolicy.test.ts by extending the existing suite that calls
getIdleWorkspaceClientEvictionKeys: (1) assert when entries.length > maxEntries
but every entry has activeRefs > 0 the function returns [] (cache can
temporarily exceed limit), (2) assert when the only idle entry matches the
protected key the function returns [], (3) assert an empty entries array returns
[], and (4) assert deterministic tie-breaking when multiple entries share
identical lastAccessedAt (e.g., ensure stable ordering or expected keys are
evicted). Use the same test structure as the "evicts least recently used idle
clients first" test and reference getIdleWorkspaceClientEvictionKeys for each
case.
🪄 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: 55f99952-79a6-407c-a3de-a9ab74498137
📒 Files selected for processing (31)
apps/desktop/src/lib/electron-app/factories/app/setup.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/lib/performance/stress-instrumentation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/WorkspaceHostUnavailableState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/useRemoteHostStatus.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.tspackages/host-service/test/integration/terminal.integration.test.tspackages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsxpackages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.tspackages/workspace-client/test/workspaceClientCachePolicy.test.tsturbo.jsonc
Comment on lines
+1
to
+28
| import { describe, expect, test } from "bun:test"; | ||
| import { getIdleWorkspaceClientEvictionKeys } from "../src/providers/WorkspaceClientProvider/workspaceClientCachePolicy"; | ||
|
|
||
| describe("workspace client cache policy", () => { | ||
| test("does not evict when the cache is under the limit", () => { | ||
| expect( | ||
| getIdleWorkspaceClientEvictionKeys([ | ||
| { key: "a", activeRefs: 0, lastAccessedAt: 1 }, | ||
| { key: "b", activeRefs: 0, lastAccessedAt: 2 }, | ||
| ]), | ||
| ).toEqual([]); | ||
| }); | ||
|
|
||
| test("evicts least recently used idle clients first", () => { | ||
| expect( | ||
| getIdleWorkspaceClientEvictionKeys( | ||
| [ | ||
| { key: "active-old", activeRefs: 1, lastAccessedAt: 1 }, | ||
| { key: "idle-old", activeRefs: 0, lastAccessedAt: 2 }, | ||
| { key: "idle-new", activeRefs: 0, lastAccessedAt: 3 }, | ||
| { key: "protected", activeRefs: 0, lastAccessedAt: 4 }, | ||
| ], | ||
| 2, | ||
| "protected", | ||
| ), | ||
| ).toEqual(["idle-old", "idle-new"]); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Relocate test file to follow co-location guideline.
The coding guidelines require test files to be co-located with their source files using a .test.ts suffix. This test file should be moved from:
packages/workspace-client/test/workspaceClientCachePolicy.test.ts
to:
packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.test.ts
Other test files in the codebase follow this pattern (e.g., resourceConsumptionPolicy.test.ts, remoteHostStatusPolicy.test.ts). As per coding guidelines, co-locate test files with their source files using .test.ts or .test.tsx suffix.
🤖 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 `@packages/workspace-client/test/workspaceClientCachePolicy.test.ts` around
lines 1 - 28, Move the test so it lives next to the implementation of
getIdleWorkspaceClientEvictionKeys (co-locate the .test.ts with the source
file), then update the import in the moved test to import
getIdleWorkspaceClientEvictionKeys from the module file using a relative path
from the new test location, and delete the original test in the test/ directory;
ensure the test filename retains the .test.ts suffix and the test continues to
reference the getIdleWorkspaceClientEvictionKeys symbol unchanged.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR addresses a renderer crash triggered by rapid workspace switching by combining source-level backpressure, bounded client lifecycles, lazy polling, and clean subtree teardown.
Confidence Score: 4/5Safe to merge; the crash path is well-targeted and the changes are tested. One architectural concern in the client cache is worth addressing before the pattern spreads. The hotkey throttle, forced subtree remount, cold-polling splits, and LRU eviction all address the described crash path correctly and are backed by focused unit tests. The one concern is that getWorkspaceClients cancels dispose timers and mutates the LRU cache during the React render phase — if a render is interrupted or replayed, the timer is gone but retainWorkspaceClients has not yet run, leaving a workspace client with zero active refs and no scheduled cleanup. Impact is bounded by the 8-entry LRU limit, so this is not a correctness regression, but it is a lifecycle invariant worth tightening. packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx — the getWorkspaceClients render-time side effects (timer cancellation, eviction) are the main area to revisit.
|
| Filename | Overview |
|---|---|
| packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx | Adds ref-counted LRU cache with idle dispose timers for per-workspace clients; getWorkspaceClients mutates cache and cancels timers during the render phase, which is unsafe under Strict Mode and concurrent rendering. |
| packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.ts | New LRU eviction policy for workspace client cache; logic is correct, evicts oldest idle entries first when over the 8-entry cap, protects the current client from self-eviction. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts | Adds dual-gate backpressure (in-flight flag + 160 ms throttle) to prev/next workspace hotkeys; timer is properly cleaned up on unmount; uses replace to avoid bloating browser history. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.ts | New pure utilities for hotkey throttle and workspace target resolution; well-tested and straightforward. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.ts | New policy function extracts host-status derivation logic; adds offline/unreachable distinction and uses strict equality check so null/unknown online status correctly falls through to the connectivity-probe path. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsx | Splits into a cold trigger and a lazy-mounted content component so polling only runs while open; hardcoded enabled and open params in shouldQueryResourceMonitor are harmless but misleading. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.tsx | Splits into separate count query (closed) and list query (open) for cold-start; debounces attached-terminal key to avoid spurious refetches; memo comparison is correct. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | Adds BasicIndex secondary indexes on frequently-joined columns (hostId, projectId, machineId, tabOrder) and moves BasicIndex import to core @tanstack/db package. |
| packages/host-service/src/terminal/terminal.ts | New countTerminalSessions function mirrors listTerminalSessions logic but returns a count; correctly filters by listed, workspaceId, exited, and excluded IDs. |
| apps/desktop/src/renderer/lib/performance/stress-instrumentation.ts | New dev-only render-rate counter and stress event logger; gated on NODE_ENV check so zero production overhead. |
Comments Outside Diff (1)
-
packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx, line 111-122 (link)Dispose-timer mutation during render phase
getWorkspaceClientscallsclearTimeoutand nullsdisposeTimersynchronously during the render phase. In React 18 Strict Mode, renders are invoked, cleaned up, then invoked again — so the dispose timer is cancelled during the first (discarded) render, but the correspondingretainWorkspaceClientseffect only runs after commit of the second render. The gap means the client can sit withactiveRefs = 0and no scheduled cleanup timer until the effect fires. In concurrent mode an interrupted render widens this window further.Moving the
clearTimeout/ timer-cancel logic insideretainWorkspaceClients(so it runs only in the committed effect, not during render) would make the lifetime contract safe regardless of how many times React calls the render function.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx Line: 111-122 Comment: **Dispose-timer mutation during render phase** `getWorkspaceClients` calls `clearTimeout` and nulls `disposeTimer` synchronously during the render phase. In React 18 Strict Mode, renders are invoked, cleaned up, then invoked again — so the dispose timer is cancelled during the first (discarded) render, but the corresponding `retainWorkspaceClients` effect only runs after commit of the second render. The gap means the client can sit with `activeRefs = 0` and no scheduled cleanup timer until the effect fires. In concurrent mode an interrupted render widens this window further. Moving the `clearTimeout` / timer-cancel logic inside `retainWorkspaceClients` (so it runs only in the committed effect, not during render) would make the lifetime contract safe regardless of how many times React calls the render function. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx:111-122
**Dispose-timer mutation during render phase**
`getWorkspaceClients` calls `clearTimeout` and nulls `disposeTimer` synchronously during the render phase. In React 18 Strict Mode, renders are invoked, cleaned up, then invoked again — so the dispose timer is cancelled during the first (discarded) render, but the corresponding `retainWorkspaceClients` effect only runs after commit of the second render. The gap means the client can sit with `activeRefs = 0` and no scheduled cleanup timer until the effect fires. In concurrent mode an interrupted render widens this window further.
Moving the `clearTimeout` / timer-cancel logic inside `retainWorkspaceClients` (so it runs only in the committed effect, not during render) would make the lifetime contract safe regardless of how many times React calls the render function.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsx:255-259
**`shouldQueryResourceMonitor` params are hardcoded — function is a no-op wrapper here**
Both `enabled` and `open` are hardcoded to `true`, so `shouldQueryResourceMonitor` is equivalent to `metadataReady` at this call site. The outer `ResourceConsumption` component already guards against `enabled=false` and `open=false`, so the behaviour is correct, but the policy function's parameters become dead code here and could mislead future readers into thinking the inner component re-validates those conditions independently.
```suggestion
// ResourceConsumptionContent is only mounted when enabled=true and open=true
// (enforced by the parent ResourceConsumption component), so we only need to
// gate on metadata readiness here.
const shouldQueryMetrics = v2MetadataReady;
```
Reviews (1): Last reviewed commit: "fix(desktop): stabilize rapid workspace ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+255
to
+259
| const shouldQueryMetrics = shouldQueryResourceMonitor({ | ||
| enabled: true, | ||
| open: true, | ||
| metadataReady: v2MetadataReady, | ||
| }); |
There was a problem hiding this comment.
shouldQueryResourceMonitor params are hardcoded — function is a no-op wrapper here
Both enabled and open are hardcoded to true, so shouldQueryResourceMonitor is equivalent to metadataReady at this call site. The outer ResourceConsumption component already guards against enabled=false and open=false, so the behaviour is correct, but the policy function's parameters become dead code here and could mislead future readers into thinking the inner component re-validates those conditions independently.
Suggested change
| const shouldQueryMetrics = shouldQueryResourceMonitor({ | |
| enabled: true, | |
| open: true, | |
| metadataReady: v2MetadataReady, | |
| }); | |
| // ResourceConsumptionContent is only mounted when enabled=true and open=true | |
| // (enforced by the parent ResourceConsumption component), so we only need to | |
| // gate on metadata readiness here. | |
| const shouldQueryMetrics = v2MetadataReady; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsx
Line: 255-259
Comment:
**`shouldQueryResourceMonitor` params are hardcoded — function is a no-op wrapper here**
Both `enabled` and `open` are hardcoded to `true`, so `shouldQueryResourceMonitor` is equivalent to `metadataReady` at this call site. The outer `ResourceConsumption` component already guards against `enabled=false` and `open=false`, so the behaviour is correct, but the policy function's parameters become dead code here and could mislead future readers into thinking the inner component re-validates those conditions independently.
```suggestion
// ResourceConsumptionContent is only mounted when enabled=true and open=true
// (enforced by the parent ResourceConsumption component), so we only need to
// gate on metadata readiness here.
const shouldQueryMetrics = v2MetadataReady;
```
How can I resolve this? If you propose a fix, please make it concise.
Kitenite
force-pushed
the
debug-renderer-crash-regr
branch
from
add8428 to
be6f5a1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/electron-app/factories/app/setup.ts (1)
77-82: ⚡ Quick winConsider validating the port number value.
The code reads
RENDERER_REMOTE_DEBUG_PORTdirectly from the environment without validating that it's a numeric port in the valid range (1-65535). Invalid values could cause Chromium to silently ignore the switch or produce confusing behavior.🛡️ Suggested validation
if (env.NODE_ENV === "development" && process.env.RENDERER_REMOTE_DEBUG_PORT) { + const port = Number.parseInt(process.env.RENDERER_REMOTE_DEBUG_PORT, 10); + if (Number.isNaN(port) || port < 1 || port > 65535) { + console.warn( + `Invalid RENDERER_REMOTE_DEBUG_PORT: ${process.env.RENDERER_REMOTE_DEBUG_PORT}. Must be 1-65535.`, + ); + } else { app.commandLine.appendSwitch( "remote-debugging-port", process.env.RENDERER_REMOTE_DEBUG_PORT, ); + } }🤖 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/lib/electron-app/factories/app/setup.ts` around lines 77 - 82, Validate RENDERER_REMOTE_DEBUG_PORT before passing it to app.commandLine.appendSwitch: parse the env value (process.env.RENDERER_REMOTE_DEBUG_PORT) as an integer, ensure it's a finite integer between 1 and 65535, and only call app.commandLine.appendSwitch("remote-debugging-port", ...) when the check passes; otherwise log a warning or ignore the setting. Update the conditional around env.NODE_ENV === "development" to include this validation and reference process.env.RENDERER_REMOTE_DEBUG_PORT and app.commandLine.appendSwitch in your fix.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/lib/electron-app/factories/app/setup.ts`:
- Around line 77-82: Validate RENDERER_REMOTE_DEBUG_PORT before passing it to
app.commandLine.appendSwitch: parse the env value
(process.env.RENDERER_REMOTE_DEBUG_PORT) as an integer, ensure it's a finite
integer between 1 and 65535, and only call
app.commandLine.appendSwitch("remote-debugging-port", ...) when the check
passes; otherwise log a warning or ignore the setting. Update the conditional
around env.NODE_ENV === "development" to include this validation and reference
process.env.RENDERER_REMOTE_DEBUG_PORT and app.commandLine.appendSwitch in your
fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 747e48d0-8a23-4e20-98c8-93714d247039
📒 Files selected for processing (31)
apps/desktop/src/lib/electron-app/factories/app/setup.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/lib/performance/stress-instrumentation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/WorkspaceHostUnavailableState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/useRemoteHostStatus.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.tspackages/host-service/test/integration/terminal.integration.test.tspackages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsxpackages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.tspackages/workspace-client/test/workspaceClientCachePolicy.test.tsturbo.jsonc
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/index.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.ts
- packages/host-service/test/integration/terminal.integration.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.test.ts
- packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.ts
- packages/host-service/src/terminal/terminal.ts
- packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx
- packages/host-service/src/trpc/router/terminal/terminal.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/useRemoteHostStatus.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/WorkspaceHostUnavailableState.tsx
- apps/desktop/src/main/windows/main.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.ts
- apps/desktop/src/renderer/lib/performance/stress-instrumentation.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsx
Kitenite
force-pushed
the
debug-renderer-crash-regr
branch
from
be6f5a1 to
5340b49
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.superset/lib/setup/main.sh (1)
44-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix duplicate step numbering.
Lines 44 and 49 both use "Step 6" in their comments. This is a pre-existing numbering inconsistency. The correct sequence should be:
- Line 44: Step 6 (Seed auth token)
- Line 49: Step 7 (Setup Neon branch) ← currently labeled "Step 6"
- Line 54: Step 8 (Allocate port base) ← currently labeled "Step 7"
- And so on...
📝 Proposed fix
# Step 6: Seed auth token into superset-dev-data/ if ! step_seed_auth_token; then step_failed "Seed auth token" fi - # Step 6: Setup Neon branch + # Step 7: Setup Neon branch if ! step_setup_neon_branch; then step_failed "Setup Neon branch" fi - # Step 7: Allocate port base (file-backed) + # Step 8: Allocate port base (file-backed) if ! allocate_port_base; then step_failed "Allocate port base" fi - # Step 8: Prepare Electric SQL env + # Step 9: Prepare Electric SQL env if ! step_prepare_electric; then step_failed "Prepare Electric SQL" fiAnd continue renumbering steps 9→10, 10→11, 11→12.
🤖 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 @.superset/lib/setup/main.sh around lines 44 - 62, Update the inline step comments to correct the sequence: change the comment before step_seed_auth_token to "Step 6: Seed auth token", change the comment before step_setup_neon_branch to "Step 7: Setup Neon branch", change the comment before allocate_port_base to "Step 8: Allocate port base (file-backed)", and change the comment before step_prepare_electric to "Step 9: Prepare Electric SQL"; locate these comments near the calls to step_seed_auth_token, step_setup_neon_branch, allocate_port_base, and step_prepare_electric and renumber any subsequent step comments accordingly..superset/lib/setup/steps.sh (1)
472-483:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
ELECTRIC_PORTentries written to .env.With the new step ordering,
step_prepare_electric(step 8) exportsELECTRIC_PORTbeforestep_write_env(step 9) runs. This causesELECTRIC_PORTto be written twice:
- Line 476 conditionally writes the exported value
- Line 521 writes the local variable (defined at line 503)
While both values are identical and most .env parsers will handle this, duplicate entries are unnecessary and could be confusing.
🔧 Suggested fix
Remove the conditional Electric variable writes (lines 472-483) since
step_prepare_electricis now always called beforestep_write_env:write_env_var "DATABASE_URL_UNPOOLED" "$DIRECT_URL" fi - echo "" - echo "# Workspace Electric SQL (Docker)" - if [ -n "${ELECTRIC_CONTAINER:-}" ]; then - write_env_var "ELECTRIC_CONTAINER" "$ELECTRIC_CONTAINER" - fi - if [ -n "${ELECTRIC_PORT:-}" ]; then - write_env_var "ELECTRIC_PORT" "$ELECTRIC_PORT" - fi - if [ -n "${ELECTRIC_URL:-}" ]; then - write_env_var "ELECTRIC_URL" "$ELECTRIC_URL" - fi - if [ -n "${ELECTRIC_SECRET:-}" ]; then - write_env_var "ELECTRIC_SECRET" "$ELECTRIC_SECRET" - fi - # Port allocation for multi-worktree dev instancesThe port allocation section (starting at line 509) will write all Electric variables including these, using the exported values from
step_prepare_electric.Also applies to: 503-503, 521-521
🤖 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 @.superset/lib/setup/steps.sh around lines 472 - 483, Remove the redundant conditional writes that call write_env_var for ELECTRIC_CONTAINER, ELECTRIC_PORT, ELECTRIC_URL, and ELECTRIC_SECRET in the step_write_env area because step_prepare_electric now runs before step_write_env and will already export these values; delete that whole if-block that checks -n "${ELECTRIC_*:-}" and writes them, relying instead on the port allocation/variable-writing later (the code that writes Electric vars after step_prepare_electric) to produce a single .env entry.
🧹 Nitpick comments (1)
.superset/lib/setup/steps.sh (1)
267-270: ⚡ Quick winConsider adding
lsofto the dependency check.The port conflict check using
lsofis a useful addition, butlsofis not listed instep_check_dependencies. Iflsofis unavailable, the check silently fails (non-zero exit) and is skipped, deferring the port conflict error to Docker startup with a less clear message.✨ Suggested addition to step_check_dependencies
Add the following check after the existing dependency checks (around line 47):
if ! command -v caddy &> /dev/null; then warn "caddy not found — HTTP/2 proxy for Electric won't work (Run: brew install caddy && caddy trust)" fi + + if ! command -v lsof &> /dev/null; then + warn "lsof not found — port conflict detection will be skipped (usually available by default on macOS/Linux)" + fi if [ ${`#missing`[@]} -gt 0 ]; then🤖 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 @.superset/lib/setup/steps.sh around lines 267 - 270, Add lsof to the dependency verification performed by step_check_dependencies so the port-check that uses lsof (the block checking lsof -nP -iTCP:"$ELECTRIC_PORT" -sTCP:LISTEN) will fail fast with a clear error if lsof is missing; update step_check_dependencies to test for the lsof binary (similar to existing checks for docker/other tools), and emit a clear error message and non-zero return when lsof is not found so the port conflict detection using ELECTRIC_PORT behaves reliably.
🤖 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.
Outside diff comments:
In @.superset/lib/setup/main.sh:
- Around line 44-62: Update the inline step comments to correct the sequence:
change the comment before step_seed_auth_token to "Step 6: Seed auth token",
change the comment before step_setup_neon_branch to "Step 7: Setup Neon branch",
change the comment before allocate_port_base to "Step 8: Allocate port base
(file-backed)", and change the comment before step_prepare_electric to "Step 9:
Prepare Electric SQL"; locate these comments near the calls to
step_seed_auth_token, step_setup_neon_branch, allocate_port_base, and
step_prepare_electric and renumber any subsequent step comments accordingly.
In @.superset/lib/setup/steps.sh:
- Around line 472-483: Remove the redundant conditional writes that call
write_env_var for ELECTRIC_CONTAINER, ELECTRIC_PORT, ELECTRIC_URL, and
ELECTRIC_SECRET in the step_write_env area because step_prepare_electric now
runs before step_write_env and will already export these values; delete that
whole if-block that checks -n "${ELECTRIC_*:-}" and writes them, relying instead
on the port allocation/variable-writing later (the code that writes Electric
vars after step_prepare_electric) to produce a single .env entry.
---
Nitpick comments:
In @.superset/lib/setup/steps.sh:
- Around line 267-270: Add lsof to the dependency verification performed by
step_check_dependencies so the port-check that uses lsof (the block checking
lsof -nP -iTCP:"$ELECTRIC_PORT" -sTCP:LISTEN) will fail fast with a clear error
if lsof is missing; update step_check_dependencies to test for the lsof binary
(similar to existing checks for docker/other tools), and emit a clear error
message and non-zero return when lsof is not found so the port conflict
detection using ELECTRIC_PORT behaves reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b0af856-9139-4c32-9e9a-03d6fb8b214d
📒 Files selected for processing (33)
.superset/lib/setup/main.sh.superset/lib/setup/steps.shapps/desktop/src/lib/electron-app/factories/app/setup.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/lib/performance/stress-instrumentation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/WorkspaceHostUnavailableState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/useRemoteHostStatus.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/terminal/terminal.tspackages/host-service/test/integration/terminal.integration.test.tspackages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsxpackages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.tspackages/workspace-client/test/workspaceClientCachePolicy.test.tsturbo.jsonc
✅ Files skipped from review due to trivial changes (2)
- turbo.jsonc
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/index.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceHostUnavailableState/WorkspaceHostUnavailableState.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/resourceConsumptionPolicy.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.test.ts
- packages/workspace-client/test/workspaceClientCachePolicy.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.utils.ts
- packages/workspace-client/src/providers/WorkspaceClientProvider/workspaceClientCachePolicy.ts
- packages/host-service/src/terminal/terminal.ts
- apps/desktop/src/renderer/lib/performance/stress-instrumentation.ts
- apps/desktop/src/lib/electron-app/factories/app/setup.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/components/TerminalSessionDropdown/TerminalSessionDropdown.utils.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/useRemoteHostStatus.ts
- packages/host-service/test/integration/terminal.integration.test.ts
- apps/desktop/src/main/windows/main.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/hooks/useRemoteHostStatus/remoteHostStatusPolicy.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.test.ts
- packages/host-service/src/trpc/router/terminal/terminal.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.tsx
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts
- packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/ResourceConsumption/ResourceConsumption.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit ba8e064 on May 16, 2026 12:58am UTC. |
Kitenite
changed the title
6 tasks
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
Reduces renderer work from polling-heavy surfaces without changing workspace switching behavior. The PR keeps expensive data fetching cold until the relevant UI is opened, adds a lightweight background terminal count path for the closed tab-bar button, and preserves fast explicit background-terminal UX with a local optimistic marker.
What changed
countBackgroundSessionsquery every 10s, and the full list runs every 2s only while the dropdown is open..env, detects occupied Electric ports, and stops inactive Electric containers on startup failure.Notes
Passive background terminal creation is detected by the shallow closed-state poll, so it can take up to 10s to appear. Explicit “move to background” actions still appear immediately.
Validation
bun test 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/BackgroundTerminalsButton/BackgroundTerminalsButton.utils.test.ts'bun run lintbun run typecheckbun run test:e2einpackages/host-service