fix(desktop): stop preset-command leak in useV2WorkspaceRun#4582
Conversation
The async useEffect + setResolvedPresetCommandsById state pattern in useV2WorkspaceRun creates a render cycle: matchedPresets and resolvePresetCommands get fresh references on every tanstack-db / tRPC update, the effect fires, setResolvedPresetCommandsById always receives a new object literal (no Object.is bail-out), state change re-renders the hook, parent re-derives matchedPresets, effect fires again. Each cycle allocates a fresh async closure, Promise.all chain, N preset-resolver promises, and PromiseReactions; the `cancelled` flag suppresses setState but does not cancel in-flight promises — their captured closures live until GC. Bisected to ac2dd46 as the first commit producing renderer freezes on rapid workspace switches (heap and listener counts climb until the renderer OOMs; freezes amplify on switch because remount forces all upstream queries to refetch). resolvePresetCommands is already synchronous (returns string[]), so the async/state plumbing is unnecessary. Replace with a useMemo that calls resolvePresetCommands directly — same output, no state, no effect, no cycle.
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBoth hooks now resolve preset commands synchronously from in-memory state: the workspace hook removes effect/state-based async resolution and derives resolvedMatchedPresets via useMemo; the preset-execution hook uses the cached agents array to build launch commands and removes host-service requests. ChangesPreset command resolution refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 SummaryReplaces the leaking async
Confidence Score: 5/5Safe to merge — the follow-up commit correctly makes Both files are clean. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts | Made resolvePresetCommands synchronous by reading from the useV2AgentConfigs-cached agents array instead of issuing a live tRPC query. Removed the getHostServiceClientByUrl import and activeHostUrl dependency from the callback. executePreset drops the no-op await. Changes are correct and complete. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceRun/useV2WorkspaceRun.ts | Removed async useEffect + useState(Record<string,string[]>) preset-resolution machinery. resolvedMatchedPresets is now a useMemo that calls the now-synchronous resolvePresetCommands directly. Interface type updated to string[]. useEffect removed from imports. No residual dead imports or logic. |
Reviews (2): Last reviewed commit: "Make resolvePresetCommands sync, trustin..." | Re-trigger Greptile
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 (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceRun/useV2WorkspaceRun.ts (1)
70-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
UseV2WorkspaceRunArgs.resolvePresetCommandstype signature and usage are misaligned—causing Promise to flow into sync operations.At line 74,
resolvePresetCommandsis typed as(preset: V2TerminalPresetRow) => Promise<string[]>, but the newuseMemoat line 114 calls it without await:commands: resolvePresetCommands(preset),This assigns a
Promise<string[]>to thecommandsfield instead ofstring[]. SinceWorkspaceRunPresetLike.commandsis defined asstring[]andpresetToWorkspaceRun(inworkspace-run-definition.tsline 62) callsnonEmptyCommands(preset.commands)expecting an array, this will fail at runtime.Tighten the type signature to match the synchronous usage:
- resolvePresetCommands: (preset: V2TerminalPresetRow) => Promise<string[]>; + resolvePresetCommands: (preset: V2TerminalPresetRow) => string[];The caller (
useV2PresetExecution) must also be updated to return the resolved commands synchronously rather than async.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspaceRun/useV2WorkspaceRun.ts around lines 70 - 75, The resolvePresetCommands callback is typed async but used synchronously; change UseV2WorkspaceRunArgs.resolvePresetCommands to return string[] (not Promise<string[]>) and update its implementation in useV2PresetExecution to return the resolved commands synchronously, so the useMemo that sets commands: resolvePresetCommands(preset) provides a string[] compatible with WorkspaceRunPresetLike.commands and presetToWorkspaceRun (which calls nonEmptyCommands). Ensure all call sites expecting a Promise are updated to handle a direct string[] return.
🤖 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
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspaceRun/useV2WorkspaceRun.ts:
- Around line 70-75: The resolvePresetCommands callback is typed async but used
synchronously; change UseV2WorkspaceRunArgs.resolvePresetCommands to return
string[] (not Promise<string[]>) and update its implementation in
useV2PresetExecution to return the resolved commands synchronously, so the
useMemo that sets commands: resolvePresetCommands(preset) provides a string[]
compatible with WorkspaceRunPresetLike.commands and presetToWorkspaceRun (which
calls nonEmptyCommands). Ensure all call sites expecting a Promise are updated
to handle a direct string[] return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16e8073a-3683-42d7-abf4-b51433816270
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspaceRun/useV2WorkspaceRun.ts
Greptile flagged that the previous fix unwittingly assigned a Promise to `commands` because `resolvePresetCommands` had been made async (commit 1239d7a added an inline live `agentConfigs.list.query()` fetch). The right structural fix is to remove that runtime live fetch entirely: - `useV2AgentConfigs` is the cached source of truth for host agent configs. It uses `staleTime: Infinity` and is explicitly invalidated by every Settings → Agents mutation (see useV2AgentConfigs.ts comment). - The inline `getHostServiceClientByUrl(...).settings.agentConfigs.list.query()` call in `resolvePresetCommands` was duplicating that exact tRPC call on every render, bypassing a cache that's already correct. - That async-ness was what forced the consumer in `useV2WorkspaceRun` into the leaking useEffect+setState pattern. Drop the inline live fetch, resolve against the in-memory `agents` array synchronously. Update the `executePreset` callsite (drop the no-op `await`) and the `UseV2WorkspaceRunArgs` type signature. Combined with the previous commit (replacing the async effect with a sync useMemo), this eliminates the render cycle and the Promise/closure allocation that was driving renderer freezes on rapid workspace switch.
|
Greptile caught a real bug — the first commit on this branch assumed I pushed a follow-up that drops the inline live fetch and makes
The inline That async-ness is what forced the previous consumer ( After this update:
Re-trigger reviewers @greptile-apps when convenient. |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Summary
ac2dd469f(Add preset-backed workspace run #4335, "Add preset-backed workspace run")useV2WorkspaceRunresolves preset commands through an asyncuseEffect+useStatepattern that produces a render cycle and an unbounded Promise/closure leakuseMemothat calls the already-synchronousresolvePresetCommandsdirectly — same output, no state, no effect, no cycleRoot cause
In
useV2WorkspaceRun.ts:In the parent (
useV2PresetExecution):matchedPresets = useMemo(..., [allPresets, projectId])whereallPresetscomes from auseLiveQuery— every Electric sync emits a new array reference → newmatchedPresetsidentityresolvePresetCommands = useCallback(..., [agentCommandsById])whereagentCommandsByIdrebuilds on everyuseV2AgentConfigsrefetch → new callback identityEach tanstack-db update or agent-config refetch fires the effect;
setResolvedPresetCommandsById(newObject)always re-renders; the re-render produces fresh upstream references; effect fires again. Each cycle allocates an async closure, aPromise.allchain, N preset-resolver promises, N PromiseReactions, and a state object. Thecancelledflag suppresses stalesetStatebut does not cancel in-flight promises — their captured closures live until GC.The leak compounds on workspace switch because the
WorkspaceTrpcProviderkey remount forces every upstream query to refetch immediately.Diagnostic data
Heap snapshot diff between idle baseline and mid-leak showed:
Promiseinstances: +513PromiseReaction: +454EventListener: +218 (retained, ~0.6/sec idle leak)StackFrameInfo: +1249 (V8 trace info from accumulated closures)CDP
Performance.getMetricsshowed JS heap and listener counts climbing steadily during idle (no user input), then spiking 5–10× during a workspace switch.resolvePresetCommandsinuseV2PresetExecution.ts:76-84is synchronous: it reads from an in-memoryMap<string, string>keyed byagentId. There's no reason for the consumer to await it, store the resolved result, or rebuild that state via an effect — a single deriveduseMemoproduces the same value with no side effects.Why this matches "introduced since 1.8.8"
ac2dd469flanded on May 9 2026, between v1.8.8 and v1.8.9. v1.8.8 has nouseV2WorkspaceRun, so workspace switching in v1.8.8 does not trigger this code path. Bisect (desktop-v1.8.8good →figure-out-crashesbad) converged on this single commit.Test plan
/settings/agents, confirm preset commands still pick up the new value (lazyuseMemorecomputes whenagentCommandsByIdidentity changes)Summary by cubic
Fixes renderer freezes and a Promise/closure leak when switching v2 workspaces by making preset command resolution fully synchronous and removing the leaking async state in
useV2WorkspaceRun. Commands now resolve via a memo using theuseV2AgentConfigscache, eliminating re-render loops.useEffect+useStateinuseV2WorkspaceRunwithuseMemo(resolvePresetCommands).resolvePresetCommandssynchronous and removed the livegetHostServiceClientByUrl(...).settings.agentConfigs.list.query()fetch and no-opawaits.string[]; behavior unchanged—presets still reflect agent command updates.Written for commit 30296d7. Summary will update on new commits.
Summary by CodeRabbit