fix(desktop): honor agent selection in new-workspace modal#3699
Merged
Conversation
The modal's agent picker was cosmetic — `selectedAgent` was persisted to localStorage for the UI but never written to the pending row, so the pending page's `buildForkAgentLaunch` always called `getFallbackAgentId` (which hard-prefers claude). Selecting codex, cursor, etc. silently launched claude instead. Thread the selected agent through the pending row into the launch build; "none" now genuinely skips the agent launch (no silent claude substitution).
Contributor
📝 WalkthroughWalkthroughThreads a nullable Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as PromptGroup (UI)
participant Hook as useSubmitWorkspace
participant DB as PendingWorkspaces DB
participant Launcher as buildForkAgentLaunch
participant Resolver as resolveAgentId
User->>UI: select agent + enter prompt
UI->>Hook: handleSubmit(projectId, selectedAgent)
Hook->>DB: insert pending workspace (includes agentId)
Hook->>Launcher: call buildForkAgentLaunch(pending)
Launcher->>Resolver: resolveAgentId(pending.agentId)
alt agentId is valid & enabled
Resolver->>Launcher: return matching enabled config
Launcher->>Launcher: build launch with agent config
else agentId null/"none" or not enabled
Resolver->>Launcher: return null
Launcher->>Launcher: abort/skip agent launch
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the new-workspace modal's agent picker being cosmetic —
Confidence Score: 4/5Safe to merge after fixing the broken test helpers — the production code path is correct. The runtime fix is sound and the data flow is correctly wired end-to-end. However, four pre-existing unit tests in buildForkAgentLaunch.test.ts are broken by the agentId: null default added to pendingBase, which is a real P1 defect that should be resolved before merge to keep CI green. apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts — pendingBase() tests need agentId overrides to pass
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.ts | Core fix: replaces getFallbackAgentId with resolveAgentId(pending.agentId, configs), correctly honouring the user-selected agent; logic is sound. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts | Adds agentId: null to pendingBase default, breaking 4 existing tests that expect non-null launch results but now receive null immediately from resolveAgentId. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts | Adds agentId to the pending pick type so it flows through to buildForkAgentLaunch; warning toast fires even when user deliberately chose "No agent". |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | Adds agentId field (string |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.ts | Threads selectedAgent into the pending row insert; dependency array correctly includes selectedAgent. |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/PromptGroup.tsx | Passes selectedAgent to useSubmitWorkspace; one-line wiring change with no issues. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.ts | Adds agentId: null to the makePending factory to satisfy the updated schema type; no functional impact. |
Sequence Diagram
sequenceDiagram
participant Modal as NewWorkspaceModal
participant Hook as useSubmitWorkspace
participant Store as CollectionsProvider
participant Pending as PendingPage
participant Build as buildForkAgentLaunch
participant Dispatch as dispatchForkLaunch
Modal->>Hook: useSubmitWorkspace(projectId, selectedAgent)
Hook->>Store: insert pendingRow { agentId: selectedAgent }
Store-->>Pending: navigate /pending/:id
Pending->>Dispatch: dispatchForkLaunch({ pending })
Dispatch->>Build: buildForkAgentLaunch({ pending.agentId, agentConfigs })
alt agentId === null or "none"
Build-->>Dispatch: null
Dispatch-->>Pending: toast warning (if userGaveInput)
else agentId is valid and enabled
Build-->>Dispatch: PendingLaunchBuild (terminal or chat)
Dispatch-->>Pending: onApplyToRow({ terminalLaunch or chatLaunch })
end
Comments Outside Diff (2)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts, line 10-20 (link)Existing tests will fail with
agentId: nulldefaultAdding
agentId: nulltopendingBasecausesresolveAgentId(null, configs)to returnnullimmediately (the!selectedbranch), makingbuildForkAgentLaunchreturnnullbefore checking sources or agent configs. At least four existing tests now expect a non-null result but will receivenull:"prompt-only → terminal launch via default agent (claude)"— expectsbuild.launch.name === "Claude""linked internal task renders into the command"— expects terminal build"attachments produce disk-ready bytes + matching names"— expects terminal build"chat agent → chat launch with initialPrompt + files"— expects chat build
Each of these tests calls
pendingBase(...)without overridingagentId, so they all run withagentId: nulland hit the early-return. They need to pass a validagentId, e.g.pendingBase({ prompt: "...", agentId: "claude" }).Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts Line: 10-20 Comment: **Existing tests will fail with `agentId: null` default** Adding `agentId: null` to `pendingBase` causes `resolveAgentId(null, configs)` to return `null` immediately (the `!selected` branch), making `buildForkAgentLaunch` return `null` before checking sources or agent configs. At least four existing tests now expect a non-null result but will receive `null`: - `"prompt-only → terminal launch via default agent (claude)"` — expects `build.launch.name === "Claude"` - `"linked internal task renders into the command"` — expects terminal build - `"attachments produce disk-ready bytes + matching names"` — expects terminal build - `"chat agent → chat launch with initialPrompt + files"` — expects chat build Each of these tests calls `pendingBase(...)` without overriding `agentId`, so they all run with `agentId: null` and hit the early-return. They need to pass a valid `agentId`, e.g. `pendingBase({ prompt: "...", agentId: "claude" })`. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts, line 101-119 (link)Warning toast fires when user explicitly chose "No agent"
When the user picks "No agent" (
agentId: "none") and submits with a prompt or attachments,buildForkAgentLaunchreturnsnullanduserGaveInputistrue, so the toast "Workspace created but no agent launched / Enable an agent in Settings → Agents to auto-launch on new workspaces" is shown. This is confusing UX — the user already made a deliberate choice; the "enable an agent in Settings" tip is misleading for them.Consider suppressing the toast when the user explicitly opted out:
const userGaveInput = pending.agentId !== "none" && ( (pending.prompt?.trim().length ?? 0) > 0 || pending.linkedIssues.length > 0 || !!pending.linkedPR || (loadedAttachments?.length ?? 0) > 0 );
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts Line: 101-119 Comment: **Warning toast fires when user explicitly chose "No agent"** When the user picks "No agent" (`agentId: "none"`) and submits with a prompt or attachments, `buildForkAgentLaunch` returns `null` and `userGaveInput` is `true`, so the toast "Workspace created but no agent launched / Enable an agent in Settings → Agents to auto-launch on new workspaces" is shown. This is confusing UX — the user already made a deliberate choice; the "enable an agent in Settings" tip is misleading for them. Consider suppressing the toast when the user explicitly opted out: ```ts const userGaveInput = pending.agentId !== "none" && ( (pending.prompt?.trim().length ?? 0) > 0 || pending.linkedIssues.length > 0 || !!pending.linkedPR || (loadedAttachments?.length ?? 0) > 0 ); ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts
Line: 10-20
Comment:
**Existing tests will fail with `agentId: null` default**
Adding `agentId: null` to `pendingBase` causes `resolveAgentId(null, configs)` to return `null` immediately (the `!selected` branch), making `buildForkAgentLaunch` return `null` before checking sources or agent configs. At least four existing tests now expect a non-null result but will receive `null`:
- `"prompt-only → terminal launch via default agent (claude)"` — expects `build.launch.name === "Claude"`
- `"linked internal task renders into the command"` — expects terminal build
- `"attachments produce disk-ready bytes + matching names"` — expects terminal build
- `"chat agent → chat launch with initialPrompt + files"` — expects chat build
Each of these tests calls `pendingBase(...)` without overriding `agentId`, so they all run with `agentId: null` and hit the early-return. They need to pass a valid `agentId`, e.g. `pendingBase({ prompt: "...", agentId: "claude" })`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Line: 188-191
Comment:
**Schema comment contradicts actual runtime behavior**
The comment says `null` is "treated as 'use fallback'" but `resolveAgentId` does the opposite — `!selected` (which is true for `null`) immediately returns `null`, skipping the launch entirely. A legacy row with `agentId: null` and a prompt will show the "Workspace created but no agent launched" warning toast, not fall back to Claude as the comment implies. The comment should reflect the actual no-op semantics.
```suggestion
// User-selected agent from the modal. `"none"` = user explicitly chose not
// to launch; any other string = `AgentDefinitionId`; null = legacy rows
// (predating this field), treated as no-op (launch is skipped).
agentId: z.string().nullable().default(null),
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts
Line: 101-119
Comment:
**Warning toast fires when user explicitly chose "No agent"**
When the user picks "No agent" (`agentId: "none"`) and submits with a prompt or attachments, `buildForkAgentLaunch` returns `null` and `userGaveInput` is `true`, so the toast "Workspace created but no agent launched / Enable an agent in Settings → Agents to auto-launch on new workspaces" is shown. This is confusing UX — the user already made a deliberate choice; the "enable an agent in Settings" tip is misleading for them.
Consider suppressing the toast when the user explicitly opted out:
```ts
const userGaveInput = pending.agentId !== "none" && (
(pending.prompt?.trim().length ?? 0) > 0 ||
pending.linkedIssues.length > 0 ||
!!pending.linkedPR ||
(loadedAttachments?.length ?? 0) > 0
);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): honor agent selection in n..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+188
to
+191
| // User-selected agent from the modal. `"none"` = user explicitly chose not | ||
| // to launch; any other string = `AgentDefinitionId`; null = legacy rows | ||
| // (predating this field), treated as "use fallback". | ||
| agentId: z.string().nullable().default(null), |
There was a problem hiding this comment.
The comment says null is "treated as 'use fallback'" but resolveAgentId does the opposite — !selected (which is true for null) immediately returns null, skipping the launch entirely. A legacy row with agentId: null and a prompt will show the "Workspace created but no agent launched" warning toast, not fall back to Claude as the comment implies. The comment should reflect the actual no-op semantics.
Suggested change
| // User-selected agent from the modal. `"none"` = user explicitly chose not | |
| // to launch; any other string = `AgentDefinitionId`; null = legacy rows | |
| // (predating this field), treated as "use fallback". | |
| agentId: z.string().nullable().default(null), | |
| // User-selected agent from the modal. `"none"` = user explicitly chose not | |
| // to launch; any other string = `AgentDefinitionId`; null = legacy rows | |
| // (predating this field), treated as no-op (launch is skipped). | |
| agentId: z.string().nullable().default(null), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Line: 188-191
Comment:
**Schema comment contradicts actual runtime behavior**
The comment says `null` is "treated as 'use fallback'" but `resolveAgentId` does the opposite — `!selected` (which is true for `null`) immediately returns `null`, skipping the launch entirely. A legacy row with `agentId: null` and a prompt will show the "Workspace created but no agent launched" warning toast, not fall back to Claude as the comment implies. The comment should reflect the actual no-op semantics.
```suggestion
// User-selected agent from the modal. `"none"` = user explicitly chose not
// to launch; any other string = `AgentDefinitionId`; null = legacy rows
// (predating this field), treated as no-op (launch is skipped).
agentId: z.string().nullable().default(null),
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts (1)
10-21:⚠️ Potential issue | 🔴 CriticalFix test fixture:
pendingBase()defaultsagentIdto null, causing test failures
pendingBase()setsagentId: null, which causesbuildForkAgentLaunch()to returnnullat line 130. However, tests at lines 125–138, 140–158, 160–179, and 188–206 don't overrideagentIdand expect non-null builds (e.g.,expect(build?.kind).toBe("terminal")). WithagentId: null, these assertions will fail.Update each affected test to explicitly set
agentId:Examples
- pending: pendingBase({ prompt: "refactor the auth middleware" }), + pending: pendingBase({ prompt: "refactor the auth middleware", agentId: "claude" }),- pending: pendingBase({ + pending: pendingBase({ + agentId: "claude", prompt: "do it", linkedIssues: [ ... ], }),- pending: pendingBase({ prompt: "help me refactor" }), + pending: pendingBase({ prompt: "help me refactor", agentId: "superset-chat" }),🤖 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/pending/`$pendingId/buildForkAgentLaunch.test.ts around lines 10 - 21, The fixture pendingBase currently defaults agentId to null which causes buildForkAgentLaunch to return null; update the failing tests that call pendingBase() without overrides to explicitly set a non-null agentId (e.g., pass { agentId: "agent-123" } or use any existing AGENT_ID constant) when constructing inputs for buildLaunchSourcesFromPending so buildForkAgentLaunch produces a non-null build; locate usages in the tests that call pendingBase() and add the agentId override or alternatively change pendingBase's default agentId to a valid test agent id, ensuring assertions like expect(build?.kind).toBe("terminal") receive a non-null build.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts (1)
191-191: Consider narrowingagentIdto a validated union.
z.string().nullable()accepts arbitrary strings, while the producer side (WorkspaceCreateAgentinPromptGroup.tsx) is"none" | AgentDefinitionId. Since the downstream consumer (resolveAgentId) already casts viaas AgentDefinitionId, a tighter schema (e.g.z.union([z.literal("none"), z.string()]).nullable()or the explicitAgentDefinitionIdenum if exported) would better document the contract and catch malformed persisted rows at parse time. Non-blocking — current shape is intentionally permissive to tolerate future agent IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts` at line 191, The schema's agentId field is currently z.string().nullable() which allows arbitrary strings; change it to a tighter union that matches the producer/consumer contract (e.g. use z.union([z.literal("none"), z.string()]).nullable() or, if available, the exported AgentDefinitionId enum/brand) so invalid persisted agent IDs are caught at parse time; update the agentId definition in schema.ts (referencing agentId), ensure consistency with WorkspaceCreateAgent in PromptGroup.tsx and resolveAgentId usage, and run the existing validation/tests to confirm no regressions.apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.ts (2)
129-135: Redundant re-lookup ofagentConfigafterresolveAgentId.
resolveAgentIdalready callsindexResolvedAgentConfigs(configs)and verifiesmatch.enabledbefore returningmatch.id. The subsequent call here re-indexes the same list and re-checks!agentConfig.enabled. Consider havingresolveAgentIdreturn the resolved config (or a{ id, config }tuple) so you avoid the secondindexResolvedAgentConfigspass and the redundant guard.♻️ Proposed refactor
- const agentId = resolveAgentId(inputs.pending.agentId, inputs.agentConfigs); - if (!agentId) return null; - - const agentConfig = indexResolvedAgentConfigs(inputs.agentConfigs).get( - agentId, - ); - if (!agentConfig || !agentConfig.enabled) return null; + const agentConfig = resolveAgentConfig( + inputs.pending.agentId, + inputs.agentConfigs, + ); + if (!agentConfig) return null; + const agentId = agentConfig.id;-function resolveAgentId( +function resolveAgentConfig( selected: string | null, configs: ResolvedAgentConfig[], -): AgentDefinitionId | null { +): ResolvedAgentConfig | null { if (!selected || selected === "none") return null; const match = indexResolvedAgentConfigs(configs).get( selected as AgentDefinitionId, ); - return match?.enabled ? match.id : null; + return match?.enabled ? match : null; }🤖 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/pending/`$pendingId/buildForkAgentLaunch.ts around lines 129 - 135, resolveAgentId is redoing indexResolvedAgentConfigs and the enabled check, so avoid the duplicate work by changing resolveAgentId to return both the resolved id and its config (e.g., { id, config } or a tuple) instead of only id; then in buildForkAgentLaunch.ts replace the current agentId = resolveAgentId(...) call with a destructuring of the returned { id, config }, remove the subsequent indexResolvedAgentConfigs(...) lookup and the duplicated enabled guard (use the returned config to null-check), and adjust downstream uses from agentId/agentConfig accordingly.
167-176: Cast of user-suppliedselectedtoAgentDefinitionIdis functionally safe but could be avoided.
selectedis typed asstring | null(matching the schema'sz.string().nullable()), but is cast toAgentDefinitionIdbefore the.get(...)lookup. SinceindexResolvedAgentConfigsreturns aMap<AgentDefinitionId, ResolvedAgentConfig>andMap.getsafely returnsundefinedfor unknown keys, the cast is harmless. However, it silently accepts any string persisted in legacy rows.To tighten this, the schema could use
z.string().refine()with the exportedisBuiltinAgentId()orisCustomAgentId()helpers (from@superset/shared/agent-catalog), which would validate the actualAgentDefinitionIdunion at the source and eliminate the need for the cast.🤖 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/pending/`$pendingId/buildForkAgentLaunch.ts around lines 167 - 176, The resolveAgentId function currently casts the user-supplied selected string to AgentDefinitionId before looking it up in the Map returned by indexResolvedAgentConfigs; instead, validate the value first using the exported helpers and avoid the cast: either update the input schema to use z.string().refine(...) with isBuiltinAgentId/isCustomAgentId so selected is typed as AgentDefinitionId upstream, or add a runtime guard in resolveAgentId that calls isBuiltinAgentId(selected) || isCustomAgentId(selected) and only then calls indexResolvedAgentConfigs(configs).get(selected) to return match?.enabled ? match.id : null; this removes the unsafe cast and tightens validation.
🤖 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/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`:
- Around line 188-191: The schema comment for agentId is misleading: instead of
saying "null = legacy rows ... treated as 'use fallback'", update the comment to
state that null indicates legacy/in-flight rows that should be treated as a
no-op (no launch) because resolveAgentId returns null and buildForkAgentLaunch
will return null; reference the agentId field in this file and the
resolveAgentId and buildForkAgentLaunch functions so maintainers understand the
actual downstream behavior.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/buildForkAgentLaunch.test.ts:
- Around line 10-21: The fixture pendingBase currently defaults agentId to null
which causes buildForkAgentLaunch to return null; update the failing tests that
call pendingBase() without overrides to explicitly set a non-null agentId (e.g.,
pass { agentId: "agent-123" } or use any existing AGENT_ID constant) when
constructing inputs for buildLaunchSourcesFromPending so buildForkAgentLaunch
produces a non-null build; locate usages in the tests that call pendingBase()
and add the agentId override or alternatively change pendingBase's default
agentId to a valid test agent id, ensuring assertions like
expect(build?.kind).toBe("terminal") receive a non-null build.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/buildForkAgentLaunch.ts:
- Around line 129-135: resolveAgentId is redoing indexResolvedAgentConfigs and
the enabled check, so avoid the duplicate work by changing resolveAgentId to
return both the resolved id and its config (e.g., { id, config } or a tuple)
instead of only id; then in buildForkAgentLaunch.ts replace the current agentId
= resolveAgentId(...) call with a destructuring of the returned { id, config },
remove the subsequent indexResolvedAgentConfigs(...) lookup and the duplicated
enabled guard (use the returned config to null-check), and adjust downstream
uses from agentId/agentConfig accordingly.
- Around line 167-176: The resolveAgentId function currently casts the
user-supplied selected string to AgentDefinitionId before looking it up in the
Map returned by indexResolvedAgentConfigs; instead, validate the value first
using the exported helpers and avoid the cast: either update the input schema to
use z.string().refine(...) with isBuiltinAgentId/isCustomAgentId so selected is
typed as AgentDefinitionId upstream, or add a runtime guard in resolveAgentId
that calls isBuiltinAgentId(selected) || isCustomAgentId(selected) and only then
calls indexResolvedAgentConfigs(configs).get(selected) to return match?.enabled
? match.id : null; this removes the unsafe cast and tightens validation.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`:
- Line 191: The schema's agentId field is currently z.string().nullable() which
allows arbitrary strings; change it to a tighter union that matches the
producer/consumer contract (e.g. use z.union([z.literal("none"),
z.string()]).nullable() or, if available, the exported AgentDefinitionId
enum/brand) so invalid persisted agent IDs are caught at parse time; update the
agentId definition in schema.ts (referencing agentId), ensure consistency with
WorkspaceCreateAgent in PromptGroup.tsx and resolveAgentId usage, and run the
existing validation/tests to confirm no regressions.
🪄 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: a6ca0d19-2e0a-4e6a-9dc3-0095839d96ea
📒 Files selected for processing (7)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildIntentPayload.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/PromptGroup.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useSubmitWorkspace/useSubmitWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Comment on lines
+188
to
+191
| // User-selected agent from the modal. `"none"` = user explicitly chose not | ||
| // to launch; any other string = `AgentDefinitionId`; null = legacy rows | ||
| // (predating this field), treated as "use fallback". | ||
| agentId: z.string().nullable().default(null), |
Contributor
There was a problem hiding this comment.
Comment contradicts actual null-handling behavior.
The comment states null = legacy rows (predating this field), treated as "use fallback", but downstream resolveAgentId in buildForkAgentLaunch.ts returns null when selected is null, which causes buildForkAgentLaunch to return null (no launch). This matches the PR's stated behavior ("existing in-flight pending rows with null agentId … no-op the launch rather than crashing") but directly contradicts "use fallback" in this schema comment.
Recommend aligning the comment with the actual no-op behavior so future maintainers don't expect a fallback resolution path.
📝 Proposed comment fix
- // User-selected agent from the modal. `"none"` = user explicitly chose not
- // to launch; any other string = `AgentDefinitionId`; null = legacy rows
- // (predating this field), treated as "use fallback".
+ // User-selected agent from the modal. `"none"` = user explicitly chose not
+ // to launch; any other string = `AgentDefinitionId`; null = legacy rows
+ // (predating this field) — treated the same as `"none"` (no-op launch on
+ // retry) rather than falling back to a default agent.
agentId: z.string().nullable().default(null),📝 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
| // User-selected agent from the modal. `"none"` = user explicitly chose not | |
| // to launch; any other string = `AgentDefinitionId`; null = legacy rows | |
| // (predating this field), treated as "use fallback". | |
| agentId: z.string().nullable().default(null), | |
| // User-selected agent from the modal. `"none"` = user explicitly chose not | |
| // to launch; any other string = `AgentDefinitionId`; null = legacy rows | |
| // (predating this field) — treated the same as `"none"` (no-op launch on | |
| // retry) rather than falling back to a default agent. | |
| agentId: z.string().nullable().default(null), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`
around lines 188 - 191, The schema comment for agentId is misleading: instead of
saying "null = legacy rows ... treated as 'use fallback'", update the comment to
state that null indicates legacy/in-flight rows that should be treated as a
no-op (no launch) because resolveAgentId returns null and buildForkAgentLaunch
will return null; reference the agentId field in this file and the
resolveAgentId and buildForkAgentLaunch functions so maintainers understand the
actual downstream behavior.
…election Tests were written against the old fallback-to-claude behavior — they passed `agentId: null` and expected Claude to launch. Under the new contract, null is treated as "no selection → no launch". Pass explicit `agentId` in each case. Add coverage for the new null / "none" paths.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts (1)
210-236: Negative-case coverage looks solid.The three cases now cleanly separate the reasons a launch may resolve to
null:
- disabled selection (line 210) — selection exists but agent is disabled,
- no selection (line 220) —
agentId: null,- explicit opt-out (line 229) —
agentId: "none".Nit (optional): consider adding a positive assertion in the
"none"test thatbuildLaunchSourcesFromPendingstill produces sources for the same pending, to pin down that the null result comes from the agent-resolution step and not from an empty source list. Not blocking.🤖 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/pending/`$pendingId/buildForkAgentLaunch.test.ts around lines 210 - 236, Add a positive assertion in the 'agentId "none" → null (explicit opt-out)' test to ensure the null return is due to agent resolution and not empty sources: call buildLaunchSourcesFromPending with the same pending (pendingBase({ prompt: "hi", agentId: "none" })) and assert it returns a non-empty array (or expected sources) before asserting buildForkAgentLaunch(...) is null; update the test to reference buildForkAgentLaunch and buildLaunchSourcesFromPending so reviewers can see the source list is present while the final launch is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/buildForkAgentLaunch.test.ts:
- Around line 210-236: Add a positive assertion in the 'agentId "none" → null
(explicit opt-out)' test to ensure the null return is due to agent resolution
and not empty sources: call buildLaunchSourcesFromPending with the same pending
(pendingBase({ prompt: "hi", agentId: "none" })) and assert it returns a
non-empty array (or expected sources) before asserting buildForkAgentLaunch(...)
is null; update the test to reference buildForkAgentLaunch and
buildLaunchSourcesFromPending so reviewers can see the source list is present
while the final launch is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fe38300-b6c6-47d1-adbe-3ed9ccb121ec
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/buildForkAgentLaunch.test.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
11 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
selectedAgentwas read from localStorage for the UI but never written into the pending row.buildForkAgentLaunchalways resolved viagetFallbackAgentId, which hard-prefersclaude, so selecting codex/cursor/etc. silently launched claude."none"now genuinely skips agent launch instead of falling back to claude.Test plan
agentId) don't crash on retry — they just no-op the launchSummary by cubic
Fixes the new-workspace modal so the agent you pick actually launches. Selecting "No agent" now truly skips agent launch; no silent fallback.
selectedAgentasagentIdon the pending row and thread it throughdispatchForkLaunchandbuildForkAgentLaunch.agentIdonly; remove fallback to Claude. Launch only if the chosen agent is enabled, otherwise skip.agentId: null) safely; retries no-op without errors.Written for commit 9bc6c38. Summary will update on new commits.
Summary by CodeRabbit