feat(desktop): choose a Host for new tasks - #3122
Conversation
|
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:
📝 WalkthroughSummaryThis PR lets users select a Project from any ready Runtime Host in the desktop new-task composer. The selected Host and Project control model discovery, readiness, Skills, mentions, file search, session creation, and the first message. Existing Sessions and the default Host remain unchanged. Non-Local selections show a Host badge. Design assessmentThe PR extends the existing new-task, session, Runtime Host, project-management, and preload bridge paths. It does not create a parallel session flow. Existing-session operations continue to use session-scoped APIs. The new catalog, target, and draft-key state isolates unsaved composers by Host and Project. The grouped workspace picker supports Projects across multiple Hosts. These changes appear necessary for the requested behavior and form the smallest coherent solution. No safe deletion or simplification is evident from the current diff. The tests preserve coverage for target forwarding, permission modes, stale results, unresolved draft handoff, cleanup, failure feedback, project-selection side effects, and existing-session behavior. ValidationReported validation includes:
The supplied repository search found no direct check-result records. Required checks therefore remain unverified here. Review-relevant risks
WalkthroughNew-task creation now targets a selected Runtime Host and project. The preload bridge exposes host-scoped APIs. Renderer state, workspace selection, readiness, composer routing, drafts, models, permissions, and tests use target-specific context. ChangesHost-scoped new-task targeting
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to The change can lose a persisted draft during host/project handoff, leave model discovery unresolved when host state changes repeatedly, and route readiness recovery to the wrong host. These are bounded but concrete correctness and availability risks in the current implementation, so merge should wait for fixes or explicit owner acceptance. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
f409a92 to
e0683c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/desktop/src/main/__tests__/runtime-host-skills-ipc-main.test.ts (1)
47-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the explicit no-project selection.
The test covers a string
projectId. It does not coverprojectId: null, whichnormalizeNewSessionProjectIdpreserves as a distinct value and whichresolveNewSessionWorkspaceTargetinapps/desktop/src/main/runtime-host-boot.tsmaps to ahost_pathtarget for local hosts and toundefinedfor remote hosts. That branch is the one most likely to regress intoundefined.Assert that
resolvedProjectIdsreceivesnullwhen the new-session context carriesprojectId: null.apps/desktop/src/renderer/use-new-task-target.ts (1)
91-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared project-mutation flow.
addProjectandrelinkProjectdiffer only in the bridge call. The capability guard,pendinghandling, selection update,refresh(), and the error toast are identical. One helper that takes the bridge call keeps the two paths from drifting.♻️ Proposed extraction
+ async function commitProjectSelection( + host: ReadyHost, + run: () => Promise<{ ok: true; project: ProjectRecord } | { ok: false; reason: 'cancelled' }>, + ): Promise<void> { + if (!host.capabilities.chooseClientDirectory || pending) return; + setPending(true); + try { + const result = await run(); + if (!result.ok) return; + setSelectedProfileId(host.profile.id); + setProjectSelections((current) => + new Map(current).set(host.profile.id, result.project.id), + ); + await refresh(); + } catch (error) { + options.toastApi.error( + copy.selectDirectoryFailedTitle, + localizedShellErrorMessage(error, copy.readPathFailedFallback, options.uiLocale), + ); + } finally { + setPending(false); + } + } + + const addProject = (host: ReadyHost): Promise<void> => + commitProjectSelection(host, () => + window.maka.newTasks.addProject({ profileId: host.profile.id, hostId: host.hostId }), + ); + + const relinkProject = (host: ReadyHost, projectId: string): Promise<void> => + commitProjectSelection(host, () => + window.maka.newTasks.relinkProject( + { profileId: host.profile.id, hostId: host.hostId }, + projectId, + ), + );apps/desktop/src/renderer/use-shell-connections.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the snapshot key once, and use
useEffectfor the refresh.The key expression
options.newTaskHost?.hostId ?? (options.defaultHost ? DEFAULT_HOST_KEY : NO_HOST_KEY)appears at Line 42 and Line 65, and Line 66 restates the same "no host context" condition a third time. A single helper keeps the three in step.The refresh at Line 52 does not need layout timing.
useEffectavoids running an IPC dispatch in the pre-paint phase.♻️ Proposed consolidation
+ const hostKeyFor = (sessionId?: string): string | undefined => + sessionId + ? parseDesktopSessionKey(sessionId).hostId + : options.newTaskHost?.hostId ?? + (options.defaultHost ? DEFAULT_HOST_KEY : undefined); + - const snapshotKey = options.activeSessionId - ? parseDesktopSessionKey(options.activeSessionId).hostId - : options.newTaskHost?.hostId ?? (options.defaultHost ? DEFAULT_HOST_KEY : NO_HOST_KEY); + const snapshotKey = hostKeyFor(options.activeSessionId) ?? NO_HOST_KEY; @@ - useLayoutEffect(() => { + useEffect(() => { if (!options.activeSessionId && (options.newTaskHost || options.defaultHost)) { void refreshConnections(); } }, [options.activeSessionId, options.newTaskHost?.hostId, options.defaultHost]); @@ async function refreshConnections(sessionId?: string) { - const key = sessionId - ? parseDesktopSessionKey(sessionId).hostId - : options.newTaskHost?.hostId ?? (options.defaultHost ? DEFAULT_HOST_KEY : NO_HOST_KEY); - if (!sessionId && !options.newTaskHost && !options.defaultHost) return; + const key = hostKeyFor(sessionId); + if (!key) return;Also applies to: 52-56, 62-66
apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.ts (1)
174-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one case for a missing new-task target.
sendgains a new early return when there is no active session andnewTaskTargetis undefined (apps/desktop/src/renderer/app-shell-chat-actions.tslines 306-309). That branch is the only guard that stops a send with no Host. No test covers it, so a later refactor can drop it silently.Assert that
sendreturnsfalseand thatnewTasks.createis never called.💚 Proposed test
it('does not create a session when no new-task target is selected', async () => { let created = false; const restoreWindow = installWindow({ newTasks: { create: async () => { created = true; return { id: 'session-1' }; }, }, sessions: {}, }); try { const deps = { ...createActionsDeps(), newTaskTarget: undefined }; assert.equal(await createAppShellChatActions(deps).send('hello'), false); } finally { restoreWindow(); } assert.equal(created, false); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b319ef8c-545d-4ec5-803f-cfbb085a6214
📒 Files selected for processing (18)
apps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.tsapps/desktop/src/main/__tests__/runtime-host-skills-ipc-main.test.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/main/runtime-host-skills-ipc-main.tsapps/desktop/src/main/workspace-search-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell-chat-actions.tsapps/desktop/src/renderer/app-shell-session-start-actions.tsapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/locales/shell-copy.tsapps/desktop/src/renderer/styles/composer.cssapps/desktop/src/renderer/use-composer-mentions.tsapps/desktop/src/renderer/use-new-task-target.tsapps/desktop/src/renderer/use-shell-connections.tsapps/desktop/src/renderer/use-task-submission-readiness.tsapps/desktop/stories/app-shell.stories.tsxpackages/ui/src/workspace-picker.tsx
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
d9ae67d to
712372a
Compare
Allow the session-less composer to select a Project from any connected Runtime Host without changing the default Host. Bind model discovery, readiness, Skills, mentions, Session creation, and the first send to the same selected target. Generated-by: OpenAI Codex
Keep drafts, model and permission choices, connection projections, and recovery actions scoped to the selected Host and Project. Catalog failures now remain recoverable without borrowing or mutating the default Host's state. Generated-by: OpenAI Codex
712372a to
8e15584
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/__tests__/app-shell-busy-race-settlement.test.ts (1)
91-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the obsolete
newChatProjectIdfixture field.
createAppShellChatActionsreceivesnewTaskTarget. The unusednewChatProjectIdfield suggests that this test still covers the removed contract. Delete it.As per path instructions, “Flag concrete cases where code can be deleted or simplified.”
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51170732-db43-4d5b-92ec-d01c7f7ecc88
📒 Files selected for processing (23)
apps/desktop/src/main/__tests__/app-shell-busy-race-settlement.test.tsapps/desktop/src/main/__tests__/app-shell-first-send-cleanup.test.tsapps/desktop/src/main/__tests__/app-shell-session-settings-actions.test.tsapps/desktop/src/main/__tests__/bootstrap-selection-lease.test.tsapps/desktop/src/main/__tests__/runtime-host-skills-ipc-main.test.tsapps/desktop/src/main/runtime-host-skills-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell-chat-actions.tsapps/desktop/src/renderer/app-shell-command-actions.tsapps/desktop/src/renderer/app-shell-session-settings-actions.tsapps/desktop/src/renderer/app-shell-session-start-actions.tsapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/chat-composer-region.tsxapps/desktop/src/renderer/locales/shell-copy.tsapps/desktop/src/renderer/new-task-reload-intent.tsapps/desktop/src/renderer/styles/composer.cssapps/desktop/src/renderer/use-composer-mentions.tsapps/desktop/src/renderer/use-new-task-target.tsapps/desktop/src/renderer/use-shell-chat-model.tsapps/desktop/src/renderer/use-shell-connections.tspackages/ui/src/composer.tsxpackages/ui/src/workspace-picker.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/src/main/tests/runtime-host-skills-ipc-main.test.ts
- apps/desktop/src/preload/bridge-contract.d.ts
- apps/desktop/src/renderer/styles/composer.css
- apps/desktop/src/renderer/use-new-task-target.ts
- apps/desktop/src/main/tests/app-shell-first-send-cleanup.test.ts
- apps/desktop/src/renderer/app-shell-session-start-actions.ts
- apps/desktop/src/renderer/app-shell-chat-actions.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/renderer/use-composer-mentions.ts
- packages/ui/src/workspace-picker.tsx
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Keep the existing new-chat race coverage on the target-scoped Session creation path introduced by multi-Host task selection. Generated-by: OpenAI Codex
Keep target-specific model, thinking, permission, and reload draft choices when switching Hosts or resolving startup catalog state. Seed the default Host connection surface from onboarding so startup never presents a false empty model list. Generated-by: OpenAI Codex
8e15584 to
e0bdb8c
Compare
Keep draft Project selection independent from the Host's persisted current Project and preserve the unresolved new-task draft when catalog discovery settles behind another Session. Make empty ready Hosts enterable without moving the selected Host's fixed actions into the project scroller. Generated-by: OpenAI Codex
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 99481d52-8341-4606-a89f-0518b9c8763f
📒 Files selected for processing (13)
apps/desktop/src/main/__tests__/chat-composer-region-draft-handoff.test.tsxapps/desktop/src/main/__tests__/new-session-project.test.tsapps/desktop/src/main/__tests__/project-management-service.test.tsapps/desktop/src/main/app-ipc-main.tsapps/desktop/src/main/new-session-project.tsapps/desktop/src/main/project-management-service.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell-chat-actions.tsapps/desktop/src/renderer/chat-composer-region.tsxapps/desktop/stories/app-shell.stories.tsxpackages/ui/src/composer.tsxpackages/ui/src/use-composer-draft.tspackages/ui/src/workspace-picker.tsx
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/app-shell-chat-actions.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/chat-composer-region.tsx
- apps/desktop/src/preload/preload.ts
- packages/ui/src/workspace-picker.tsx
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Express the renderer regression with createElement so it remains a standard Desktop test entry and is included by the repository's unused-file check. Generated-by: OpenAI Codex
Let the draft owner hydrate a requested inactive slot from its persistence adapter when memory has no value. This preserves an unresolved new-task draft even when the renderer starts on an existing Session. Generated-by: OpenAI Codex
Keep the default Host connection projection independent from multi-Host catalog availability. Resolve implicit Project selection from the Host's configured and current preferences, and keep the picker usable during background refreshes. Generated-by: OpenAI Codex
|
I have completed the full verification, and after assessment, I believe this change has minimal impact on the existing system. I take full responsibility for this and decide to merge it. |
…hanges Draft text, staged attachments and staged quotes are all keyed by (profileId, hostId, projectId) since apache#3122, and the workspace picker that changes the project part sits directly under the composer — so choosing a Project re-keyed all three mid-composition and what the user was writing dropped out of view. Carry them to the target the user selects. The buckets stay keyed per target, so apache#3122's Host-scoped new-task state is unchanged; they move with the selection instead of staying behind under the key the user navigated away from. The move is unconditional: the target arrived at holds what was brought to it and nothing else. Carrying only into an empty target would leave copies under every key the composer passed through, and one would resurface later — send the task, come back to an empty composer, pick another target, and the text just sent would reappear as that target's own draft. The carry keys on the new-task target rather than on the composer's active key, so a Session keeps its own draft and attachments when the target moves behind it. Leaving the unresolved slot keeps its existing reload-lease guard: that transition is startup settling, not a choice, and its draft may belong to one specific target. Fixes apache#3408 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found two ways pending content could escape the owner an in-flight operation had captured. Staged files and quotes no longer follow the workspace picker at all. They are in-memory intent, not Host state — nothing persists them and nothing restores them per target — so they go back to one key that never moves, as they were before apache#3122. `send()` captures the key it submitted from and clears exactly that key once it resolves, so a key that followed the picker left the files it had already delivered staged under the new target, ready to be sent a second time. A key that cannot go stale needs no re-keying rule to keep it honest, which removes rekeyPending and the carry hook with it. The draft text keeps its target-scoped key, because its reload lease belongs to one specific target. ChatComposerRegion therefore defers its carry while a send is settling: the submission owns the text it submitted until the completion clears the key it captured, and text typed after that send is carried once it does. pickAttachments resolves its owner after the native dialog closes rather than capturing it before opening. The surface can change while a dialog is up, and files the user just chose belong in the composer they are looking at, not in a bucket they have left where the files are invisible but still sendable. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hanges (#3432) * fix(desktop): keep the new-task composer's contents when the target changes Draft text, staged attachments and staged quotes are all keyed by (profileId, hostId, projectId) since #3122, and the workspace picker that changes the project part sits directly under the composer — so choosing a Project re-keyed all three mid-composition and what the user was writing dropped out of view. Carry them to the target the user selects. The buckets stay keyed per target, so #3122's Host-scoped new-task state is unchanged; they move with the selection instead of staying behind under the key the user navigated away from. The move is unconditional: the target arrived at holds what was brought to it and nothing else. Carrying only into an empty target would leave copies under every key the composer passed through, and one would resurface later — send the task, come back to an empty composer, pick another target, and the text just sent would reappear as that target's own draft. The carry keys on the new-task target rather than on the composer's active key, so a Session keeps its own draft and attachments when the target moves behind it. Leaving the unresolved slot keeps its existing reload-lease guard: that transition is startup settling, not a choice, and its draft may belong to one specific target. Fixes #3408 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(desktop): give the new-task composer's staged content one owner Review found two ways pending content could escape the owner an in-flight operation had captured. Staged files and quotes no longer follow the workspace picker at all. They are in-memory intent, not Host state — nothing persists them and nothing restores them per target — so they go back to one key that never moves, as they were before #3122. `send()` captures the key it submitted from and clears exactly that key once it resolves, so a key that followed the picker left the files it had already delivered staged under the new target, ready to be sent a second time. A key that cannot go stale needs no re-keying rule to keep it honest, which removes rekeyPending and the carry hook with it. The draft text keeps its target-scoped key, because its reload lease belongs to one specific target. ChatComposerRegion therefore defers its carry while a send is settling: the submission owns the text it submitted until the completion clears the key it captured, and text typed after that send is carried once it does. pickAttachments resolves its owner after the native dialog closes rather than capturing it before opening. The surface can change while a dialog is up, and files the user just chose belong in the composer they are looking at, not in a bucket they have left where the files are invisible but still sendable. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
English
Summary
Refs #2522
Verification
中文
概要
关联 #2522
验证
Visual comparison
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with implementation, tests, and verification under the maintainer's direction
Checklist
Does this PR entail a change in behavior?