Execution targets: workspace picker, chat tagging, and persistence#139
Closed
Execution targets: workspace picker, chat tagging, and persistence#139
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds execution-target support: types, persistence, IPC, preload bridge, renderer hook, UI components, and plumbing through chat/session/terminal flows to select, display, and persist local or SSH targets per project. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
Owner
Author
|
@copilot review |
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98571d816e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+47
to
+49
| const next: AdeExecutionTargetsState = { | ||
| ...state, | ||
| activeTargetId: state.profiles.some((p) => p.id === id) ? id : ADE_LOCAL_EXECUTION_TARGET_ID, |
There was a problem hiding this comment.
setActiveTargetId builds next by spreading the hook-local state, but this hook only refreshes on mount/project-root change, so parallel consumers (TopBar, Work/Files pages, Settings) can easily hold stale snapshots. If one surface updates profiles (e.g., adds/removes an SSH target) and another stale surface then calls setActiveTargetId, executionTargets.set is invoked with outdated profiles, which can silently overwrite and drop newer targets. This makes execution-target configuration non-durable across normal UI flows.
Useful? React with 👍 / 👎.
Contributor
Here's the review of this PR: Bug fixed (4763225): Other observations (no changes needed):
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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/components/terminals/TerminalsPage.tsx (1)
157-209:⚠️ Potential issue | 🟠 MajorAdd
activeTargetIdtopaneConfigsmemo dependencies to avoid stale target badges.
projectActiveTargetIdis consumed at Line 189, butactiveTargetIdis missing from theuseMemodependency array (Line 200-Line 208). This can keepSessionListPaneon an old target context after switching targets.🔧 Proposed fix
const paneConfigs: Record<string, PaneConfig> = useMemo( () => ({ sessions: { title: "Work", headerActions: sessionsHeaderActions, children: ( <SessionListPane @@ projectActiveTargetId={activeTargetId} /> ), }, @@ [ work, sortedLanes, + activeTargetId, handleSelectSession, handleInfoClick, handleContextMenu, sessionsHeaderActions, workViewArea, ], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx` around lines 157 - 209, The memo for paneConfigs (created by useMemo) is missing activeTargetId in its dependency array, causing SessionListPane to receive stale projectActiveTargetId; update the useMemo dependency list for paneConfigs to include activeTargetId so that paneConfigs (and its child SessionListPane prop projectActiveTargetId) are recalculated when the active target changes.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
1216-1250: Consider adding accessibility attributes to the execution target dropdown.The dropdown works functionally, but lacks ARIA attributes for screen reader users:
- Button:
aria-expanded,aria-haspopup- Menu:
role="listbox"orrole="menu"- Items:
role="option"orrole="menuitem"♿ Proposed accessibility improvements
{executionTargetProfiles?.length && onExecutionTargetChange && executionTargetId ? ( <div ref={targetMenuRef} className="relative"> <button type="button" className="inline-flex h-6 max-w-[140px] items-center gap-1 rounded-md border border-white/[0.06] bg-white/[0.02] px-1.5 font-sans text-[10px] font-medium text-muted-fg/55 transition-colors hover:border-white/[0.10] hover:text-fg/70" onClick={() => setTargetMenuOpen((o) => !o)} title="Tagged execution target for this chat. Remote runs are not wired yet — tools still execute on this computer." + aria-expanded={targetMenuOpen} + aria-haspopup="listbox" > <DesktopTower size={11} className="shrink-0 text-sky-400/70" /> <span className="min-w-0 truncate">{executionTargetLabel ?? executionTargetId}</span> <CaretDown size={9} className="shrink-0 opacity-50" /> </button> {targetMenuOpen ? ( - <div className="absolute bottom-full left-0 z-40 mb-1 min-w-[200px] overflow-hidden rounded-lg border border-white/[0.08] bg-card py-1 shadow-lg"> + <div + role="listbox" + aria-label="Execution targets" + className="absolute bottom-full left-0 z-40 mb-1 min-w-[200px] overflow-hidden rounded-lg border border-white/[0.08] bg-card py-1 shadow-lg" + > {executionTargetProfiles.map((p) => ( <button key={p.id} type="button" + role="option" + aria-selected={p.id === executionTargetId} className={cn( "flex w-full items-center gap-2 px-2 py-1.5 text-left text-[11px] transition-colors hover:bg-white/[0.04]", p.id === executionTargetId && "bg-white/[0.06]", )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 1216 - 1250, The execution target dropdown lacks ARIA accessibility attributes; update the trigger button (the element using setTargetMenuOpen and rendering DesktopTower/CaretDown) to include aria-expanded={targetMenuOpen} and aria-haspopup="listbox" and associate it with the menu via aria-controls or an id; add role="listbox" (or role="menu") plus aria-activedescendant if you implement keyboard focus management to the menu container that uses executionTargetProfiles and targetMenuOpen; give each item button rendered in the executionTargetProfiles.map (which calls onExecutionTargetChange and executionTargetSummaryLabel) a proper role="option" (or role="menuitem"), an id, and aria-selected={p.id === executionTargetId} and ensure keyboard navigation (Arrow keys/Escape) toggles setTargetMenuOpen and updates focus on items.apps/desktop/src/main/services/chat/agentChatService.ts (1)
10248-10255: Consider centralizing execution-target normalizationNormalization/defaulting logic is duplicated across create/update/summary. A shared helper would reduce drift and prevent future id/label mismatch regressions.
Also applies to: 12281-12292, 13041-13065
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 10248 - 10255, The normalization of execution target (rawExecTargetId, normalizedExecTargetId, normalizedExecTargetLabel) is duplicated; add a shared helper (e.g., normalizeExecutionTarget(requestedExecutionTargetId, requestedExecutionTargetLabel)) that returns { id, label } using ADE_LOCAL_EXECUTION_TARGET_ID and the existing trimming/defaulting rules, then replace the duplicated blocks in create/update/summary to call this helper so id/label logic is centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 13041-13065: When executionTargetId changes but
executionTargetLabel is omitted the code currently preserves any prior non-empty
managed.session.executionTargetLabel; change the logic inside the if
(executionTargetId !== undefined) branch so that after setting
managed.session.executionTargetId = nextId you always reset
managed.session.executionTargetLabel to the appropriate default when
executionTargetLabel is undefined (i.e. set to "This computer" if nextId ===
ADE_LOCAL_EXECUTION_TARGET_ID, otherwise set to nextId); keep the existing
handling when executionTargetLabel is provided (trimmed value) and remove the
branch that leaves the old label intact.
In
`@apps/desktop/src/main/services/executionTargets/executionTargetsStateService.ts`:
- Around line 23-24: The normalized executionTargets state may contain duplicate
execution target ids, so update setExecutionTargetsState (the code that calls
normalizeExecutionTargetsState and db.setJson(keyForProject(pid), normalized))
to validate the normalized object before persisting: scan
normalized.executionTargets (or the structure produced by
normalizeExecutionTargetsState) for duplicate id values, and if any duplicates
are found reject the update (throw or return an error) or deduplicate
deterministically (e.g., keep first occurrence) and only then call db.setJson;
ensure the check references normalizeExecutionTargetsState and
keyForProject(pid) so the validation lives at the persistence boundary.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 831-842: The mock's executionTargets.set() currently ignores its
input and always returns a static payload, so executionTargets.get() never
reflects updates; make the mock stateful by introducing an internal variable
initialized to the same payload used by executionTargets.get (version:1,
profiles [{id:"local", kind:"local", label:"This computer"}],
activeTargetId:"local"), then change executionTargets.get to return that
variable (via resolved) and change executionTargets.set to update that variable
with the incoming argument (and return it via resolvedArg) so caller updates
persist and mirror real IPC semantics (referencing executionTargets.get and
executionTargets.set in the diff).
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 2024-2045: handleComposerExecutionTargetChange updates
composerExecutionTargetId/composerExecutionTargetLabel before calling
window.ade.agentChat.updateSession, so if the IPC call fails the UI stays on the
new (unsaved) target; modify the handler to capture the previous id/label (read
current composerExecutionTargetId and composerExecutionTargetLabel or store them
in local vars before calling
setComposerExecutionTargetId/setComposerExecutionTargetLabel), then in the catch
block call setComposerExecutionTargetId(previousId) and
setComposerExecutionTargetLabel(previousLabel) to rollback the picker state,
while still setting the error and keeping the existing updateSession,
patchSessionSummary and refreshSessions logic.
In
`@apps/desktop/src/renderer/components/executionTargets/ExecutionTargetContextBanner.tsx`:
- Around line 18-21: The banner string in ExecutionTargetContextBanner currently
uses profile.sshHost/workspacePath for SSH profiles and reads like an active
remote workspace; change the SSH case (the detail variable when profile?.kind
=== "ssh") to a concrete, stateful message such as "<sshHost> · <workspacePath>
(Saved as remote target; remote execution is blocked — work is running on this
computer)" so it states the target was saved, remote execution is blocked, and
work is happening locally; update the same copy where similar SSH messaging
appears in the component (other occurrences referenced around lines 28-30 and
45-56) to use the same explicit wording, referencing
ExecutionTargetContextBanner, detail, profile.sshHost and profile.workspacePath
to locate places to change.
In `@apps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsx`:
- Around line 166-175: The inputs in ExecutionTargetsSection use only
placeholders (Display name, SSH destination, Remote workspace path, Jump host)
which disappear for screen readers; add persistent labels for each input element
(e.g., a visible <label> or aria-label/aria-labelledby) so screen readers and
voice control always have the field name; update the inputs tied to state
setters (label/setLabel, sshHost/setSshHost, workspacePath/setWorkspacePath,
jumpHost/setJumpHost) to reference those labels and ensure accessibility
attributes (htmlFor/id or aria-label) are present and unique for each field.
- Around line 36-45: The save operation currently swallows persist() failures
(saveState calls persist but only toggles setBusy), so update saveState to catch
persist errors and surface them to the UI (either by rethrowing the error so
callers can await it or by setting a local error state like saveError via
setSaveError) and ensure setBusy is still cleared in finally; then update the
click handlers that call saveState (the add/remove/activate handlers) to await
saveState and show an inline error/toast/modal when it rejects or when saveError
is set so IPC write failures are visible to the user; apply the same change to
the other saveState usages mentioned (the additional call sites in this
component).
- Around line 147-155: The icon-only trash button in ExecutionTargetsSection.tsx
lacks an accessible name; update the button (the element that calls
removeTarget(p.id) and renders <Trash />) to include an aria-label that includes
the target's label (e.g., "Remove target {p.label}") so screen readers can
identify which row will be removed; you can keep the existing title but ensure
aria-label is present and built from the row's label property used in the
component.
In `@apps/desktop/src/renderer/hooks/useExecutionTargets.ts`:
- Around line 13-28: The refresh() and persist() handlers (which call
window.ade.executionTargets.get() and setState/defaultExecutionTargetsState())
must ignore stale IPC responses after a project switch: capture the current
project identifier (e.g. compute const root = typeof projectRoot === "string" ?
projectRoot.trim() : "" or a local token) before awaiting, then after each await
compare that captured value to the current projectRoot (or compare a shared
requestId/token) and only call setState or setLoading if they still match; apply
the same pattern to persist() so late responses from a previous project do not
overwrite the new project state.
In `@apps/desktop/src/shared/types/executionTargets.ts`:
- Around line 63-90: normalizeExecutionTargetsState currently allows duplicate
local entries and SSH profiles with the same id; update the loop over profilesIn
(use symbols profilesIn, profiles, sawLocal, ADE_LOCAL_EXECUTION_TARGET_ID) to
deduplicate by tracking seen ids: create a Set seenIds, when adding the local
entry mark ADE_LOCAL_EXECUTION_TARGET_ID as seen and skip additional locals if
sawLocal or seenIds has that id, and for SSH entries skip any profile whose
trimmed id is already in seenIds; add each pushed profile's id to seenIds so
duplicates are dropped during normalization.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx`:
- Around line 157-209: The memo for paneConfigs (created by useMemo) is missing
activeTargetId in its dependency array, causing SessionListPane to receive stale
projectActiveTargetId; update the useMemo dependency list for paneConfigs to
include activeTargetId so that paneConfigs (and its child SessionListPane prop
projectActiveTargetId) are recalculated when the active target changes.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 10248-10255: The normalization of execution target
(rawExecTargetId, normalizedExecTargetId, normalizedExecTargetLabel) is
duplicated; add a shared helper (e.g.,
normalizeExecutionTarget(requestedExecutionTargetId,
requestedExecutionTargetLabel)) that returns { id, label } using
ADE_LOCAL_EXECUTION_TARGET_ID and the existing trimming/defaulting rules, then
replace the duplicated blocks in create/update/summary to call this helper so
id/label logic is centralized and consistent.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 1216-1250: The execution target dropdown lacks ARIA accessibility
attributes; update the trigger button (the element using setTargetMenuOpen and
rendering DesktopTower/CaretDown) to include aria-expanded={targetMenuOpen} and
aria-haspopup="listbox" and associate it with the menu via aria-controls or an
id; add role="listbox" (or role="menu") plus aria-activedescendant if you
implement keyboard focus management to the menu container that uses
executionTargetProfiles and targetMenuOpen; give each item button rendered in
the executionTargetProfiles.map (which calls onExecutionTargetChange and
executionTargetSummaryLabel) a proper role="option" (or role="menuitem"), an id,
and aria-selected={p.id === executionTargetId} and ensure keyboard navigation
(Arrow keys/Escape) toggles setTargetMenuOpen and updates focus on items.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3603204-3e2a-44da-b5c4-da7d2a1df62e
📒 Files selected for processing (29)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/executionTargets/executionTargetsStateService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/projects/projectService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/executionTargets/ExecutionTargetContextBanner.tsxapps/desktop/src/renderer/components/executionTargets/TopBarExecutionTargetSelect.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsxapps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/hooks/useExecutionTargets.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/executionTargets.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/sessions.ts
Comment on lines
+23
to
+24
| const normalized = normalizeExecutionTargetsState(next); | ||
| db.setJson(keyForProject(pid), normalized); |
There was a problem hiding this comment.
Reject duplicate execution target ids before saving.
setExecutionTargetsState() relies on normalizeExecutionTargetsState(), but the normalizer in apps/desktop/src/shared/types/executionTargets.ts currently preserves duplicate SSH ids. That lets the store hold multiple profiles behind the same foreign key even though activeTargetId and chat tags resolve by id, so later lookups or rename/delete flows can hit the wrong target.
💡 Local fix at the persistence boundary
-import type { AdeExecutionTargetsState } from "../../../shared/types";
-import { defaultExecutionTargetsState, normalizeExecutionTargetsState } from "../../../shared/types";
+import type { AdeExecutionTargetsState } from "../../../shared/types";
+import {
+ ADE_LOCAL_EXECUTION_TARGET_ID,
+ defaultExecutionTargetsState,
+ normalizeExecutionTargetsState,
+} from "../../../shared/types";
...
export function setExecutionTargetsState(
db: AdeDb | null,
projectId: string,
next: AdeExecutionTargetsState,
): AdeExecutionTargetsState {
const pid = projectId.trim();
if (!db || !pid.length) return defaultExecutionTargetsState();
const normalized = normalizeExecutionTargetsState(next);
- db.setJson(keyForProject(pid), normalized);
- return normalized;
+ const profiles = Array.from(
+ new Map(normalized.profiles.map((profile) => [profile.id, profile])).values(),
+ );
+ const safe = {
+ ...normalized,
+ profiles,
+ activeTargetId: profiles.some((profile) => profile.id === normalized.activeTargetId)
+ ? normalized.activeTargetId
+ : ADE_LOCAL_EXECUTION_TARGET_ID,
+ };
+ db.setJson(keyForProject(pid), safe);
+ return safe;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/main/services/executionTargets/executionTargetsStateService.ts`
around lines 23 - 24, The normalized executionTargets state may contain
duplicate execution target ids, so update setExecutionTargetsState (the code
that calls normalizeExecutionTargetsState and db.setJson(keyForProject(pid),
normalized)) to validate the normalized object before persisting: scan
normalized.executionTargets (or the structure produced by
normalizeExecutionTargetsState) for duplicate id values, and if any duplicates
are found reject the update (throw or return an error) or deduplicate
deterministically (e.g., keep first occurrence) and only then call db.setJson;
ensure the check references normalizeExecutionTargetsState and
keyForProject(pid) so the validation lives at the persistence boundary.
Comment on lines
+831
to
+842
| executionTargets: { | ||
| get: resolved({ | ||
| version: 1, | ||
| profiles: [{ id: "local", kind: "local" as const, label: "This computer" }], | ||
| activeTargetId: "local", | ||
| }), | ||
| set: resolvedArg({ | ||
| version: 1, | ||
| profiles: [{ id: "local", kind: "local" as const, label: "This computer" }], | ||
| activeTargetId: "local", | ||
| }), | ||
| }, |
There was a problem hiding this comment.
Make browser mock executionTargets.set() stateful to match IPC semantics.
Line 837 currently ignores the incoming state and always returns the same static payload, so get() on Line 832 never reflects user updates. This breaks execution-target flows in browser-preview mode and drifts from the real IPC contract.
🔧 Proposed fix
import { getDefaultModelDescriptor } from "../shared/modelRegistry";
+import {
+ defaultExecutionTargetsState,
+ normalizeExecutionTargetsState,
+ type AdeExecutionTargetsState,
+} from "../shared/types/executionTargets";
@@
const DEFAULT_BROWSER_MOCK_CODEX_MODEL = getDefaultModelDescriptor("codex")?.id ?? "openai/gpt-5.4-codex";
+let mockExecutionTargetsState: AdeExecutionTargetsState = defaultExecutionTargetsState();
@@
executionTargets: {
- get: resolved({
- version: 1,
- profiles: [{ id: "local", kind: "local" as const, label: "This computer" }],
- activeTargetId: "local",
- }),
- set: resolvedArg({
- version: 1,
- profiles: [{ id: "local", kind: "local" as const, label: "This computer" }],
- activeTargetId: "local",
- }),
+ get: async () => mockExecutionTargetsState,
+ set: async (next: AdeExecutionTargetsState) => {
+ mockExecutionTargetsState = normalizeExecutionTargetsState(next);
+ return mockExecutionTargetsState;
+ },
},As per coding guidelines, "Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/browserMock.ts` around lines 831 - 842, The mock's
executionTargets.set() currently ignores its input and always returns a static
payload, so executionTargets.get() never reflects updates; make the mock
stateful by introducing an internal variable initialized to the same payload
used by executionTargets.get (version:1, profiles [{id:"local", kind:"local",
label:"This computer"}], activeTargetId:"local"), then change
executionTargets.get to return that variable (via resolved) and change
executionTargets.set to update that variable with the incoming argument (and
return it via resolvedArg) so caller updates persist and mirror real IPC
semantics (referencing executionTargets.get and executionTargets.set in the
diff).
Comment on lines
+2024
to
+2045
| const handleComposerExecutionTargetChange = useCallback( | ||
| async (targetId: string) => { | ||
| const id = targetId.trim() || ADE_LOCAL_EXECUTION_TARGET_ID; | ||
| const profile = targetProfilesForPicker.find((p) => p.id === id); | ||
| const label = profile ? executionTargetSummaryLabel(profile) : id === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : id; | ||
| setComposerExecutionTargetId(id); | ||
| setComposerExecutionTargetLabel(label); | ||
| if (!selectedSessionId) return; | ||
| try { | ||
| await window.ade.agentChat.updateSession({ | ||
| sessionId: selectedSessionId, | ||
| executionTargetId: id, | ||
| executionTargetLabel: label, | ||
| }); | ||
| patchSessionSummary(selectedSessionId, { executionTargetId: id, executionTargetLabel: label }); | ||
| void refreshSessions().catch(() => {}); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| }, | ||
| [patchSessionSummary, refreshSessions, selectedSessionId, targetProfilesForPicker], | ||
| ); |
There was a problem hiding this comment.
Rollback the picker when the session update fails.
This updates composerExecutionTargetId/composerExecutionTargetLabel before the IPC call, but the catch path only stores error. On a failed update, the composer keeps showing the new target even though the session stayed on the old tag.
Proposed fix
const handleComposerExecutionTargetChange = useCallback(
async (targetId: string) => {
const id = targetId.trim() || ADE_LOCAL_EXECUTION_TARGET_ID;
const profile = targetProfilesForPicker.find((p) => p.id === id);
const label = profile ? executionTargetSummaryLabel(profile) : id === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : id;
+ const previousId = composerExecutionTargetId;
+ const previousLabel = composerExecutionTargetLabel;
setComposerExecutionTargetId(id);
setComposerExecutionTargetLabel(label);
if (!selectedSessionId) return;
try {
await window.ade.agentChat.updateSession({
@@
patchSessionSummary(selectedSessionId, { executionTargetId: id, executionTargetLabel: label });
void refreshSessions().catch(() => {});
} catch (err) {
+ setComposerExecutionTargetId(previousId);
+ setComposerExecutionTargetLabel(previousLabel);
setError(err instanceof Error ? err.message : String(err));
}
},
- [patchSessionSummary, refreshSessions, selectedSessionId, targetProfilesForPicker],
+ [
+ composerExecutionTargetId,
+ composerExecutionTargetLabel,
+ patchSessionSummary,
+ refreshSessions,
+ selectedSessionId,
+ targetProfilesForPicker,
+ ],
);📝 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
| const handleComposerExecutionTargetChange = useCallback( | |
| async (targetId: string) => { | |
| const id = targetId.trim() || ADE_LOCAL_EXECUTION_TARGET_ID; | |
| const profile = targetProfilesForPicker.find((p) => p.id === id); | |
| const label = profile ? executionTargetSummaryLabel(profile) : id === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : id; | |
| setComposerExecutionTargetId(id); | |
| setComposerExecutionTargetLabel(label); | |
| if (!selectedSessionId) return; | |
| try { | |
| await window.ade.agentChat.updateSession({ | |
| sessionId: selectedSessionId, | |
| executionTargetId: id, | |
| executionTargetLabel: label, | |
| }); | |
| patchSessionSummary(selectedSessionId, { executionTargetId: id, executionTargetLabel: label }); | |
| void refreshSessions().catch(() => {}); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : String(err)); | |
| } | |
| }, | |
| [patchSessionSummary, refreshSessions, selectedSessionId, targetProfilesForPicker], | |
| ); | |
| const handleComposerExecutionTargetChange = useCallback( | |
| async (targetId: string) => { | |
| const id = targetId.trim() || ADE_LOCAL_EXECUTION_TARGET_ID; | |
| const profile = targetProfilesForPicker.find((p) => p.id === id); | |
| const label = profile ? executionTargetSummaryLabel(profile) : id === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : id; | |
| const previousId = composerExecutionTargetId; | |
| const previousLabel = composerExecutionTargetLabel; | |
| setComposerExecutionTargetId(id); | |
| setComposerExecutionTargetLabel(label); | |
| if (!selectedSessionId) return; | |
| try { | |
| await window.ade.agentChat.updateSession({ | |
| sessionId: selectedSessionId, | |
| executionTargetId: id, | |
| executionTargetLabel: label, | |
| }); | |
| patchSessionSummary(selectedSessionId, { executionTargetId: id, executionTargetLabel: label }); | |
| void refreshSessions().catch(() => {}); | |
| } catch (err) { | |
| setComposerExecutionTargetId(previousId); | |
| setComposerExecutionTargetLabel(previousLabel); | |
| setError(err instanceof Error ? err.message : String(err)); | |
| } | |
| }, | |
| [ | |
| composerExecutionTargetId, | |
| composerExecutionTargetLabel, | |
| patchSessionSummary, | |
| refreshSessions, | |
| selectedSessionId, | |
| targetProfilesForPicker, | |
| ], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines
2024 - 2045, handleComposerExecutionTargetChange updates
composerExecutionTargetId/composerExecutionTargetLabel before calling
window.ade.agentChat.updateSession, so if the IPC call fails the UI stays on the
new (unsaved) target; modify the handler to capture the previous id/label (read
current composerExecutionTargetId and composerExecutionTargetLabel or store them
in local vars before calling
setComposerExecutionTargetId/setComposerExecutionTargetLabel), then in the catch
block call setComposerExecutionTargetId(previousId) and
setComposerExecutionTargetLabel(previousLabel) to rollback the picker state,
while still setting the error and keeping the existing updateSession,
patchSessionSummary and refreshSessions logic.
Comment on lines
+18
to
+21
| const detail = | ||
| profile?.kind === "ssh" | ||
| ? `${profile.sshHost} · ${profile.workspacePath}` | ||
| : "Files, terminals, and lanes use this machine."; |
There was a problem hiding this comment.
Make the SSH banner say the current blocked state.
For SSH profiles the banner mostly reads like an active remote workspace, but this PR only stores remote targets as metadata and Work/Files still run locally. Planned / (SSH not connected yet) is too easy to misread here; the banner should state that the target was saved, remote execution is blocked today, and work is still happening on this computer.
✏️ Copy tweak
const detail =
profile?.kind === "ssh"
- ? `${profile.sshHost} · ${profile.workspacePath}`
- : "Files, terminals, and lanes use this machine.";
+ ? profile.connectionMode === "planned"
+ ? `Saved target ${profile.sshHost} · ${profile.workspacePath}. SSH execution is not connected yet, so work still runs on this computer.`
+ : `${profile.sshHost} · ${profile.workspacePath}`
+ : "Work runs on this computer.";
...
{profile?.kind === "ssh" && profile.connectionMode === "planned" ? (
- <span className="text-amber-400/80">(SSH not connected yet)</span>
+ <span className="text-amber-400/80">(saved target, SSH not connected)</span>
) : null}
...
{profile?.kind === "ssh" && profile.connectionMode === "planned" ? (
<span className="shrink-0 rounded border border-amber-500/25 bg-amber-500/10 px-1.5 py-0.5 text-[9px] font-medium uppercase tracking-wide text-amber-200/80">
- Planned
+ SSH not connected
</span>
) : null}Also applies to: 28-30, 45-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/executionTargets/ExecutionTargetContextBanner.tsx`
around lines 18 - 21, The banner string in ExecutionTargetContextBanner
currently uses profile.sshHost/workspacePath for SSH profiles and reads like an
active remote workspace; change the SSH case (the detail variable when
profile?.kind === "ssh") to a concrete, stateful message such as "<sshHost> ·
<workspacePath> (Saved as remote target; remote execution is blocked — work is
running on this computer)" so it states the target was saved, remote execution
is blocked, and work is happening locally; update the same copy where similar
SSH messaging appears in the component (other occurrences referenced around
lines 28-30 and 45-56) to use the same explicit wording, referencing
ExecutionTargetContextBanner, detail, profile.sshHost and profile.workspacePath
to locate places to change.
Comment on lines
+36
to
+45
| const saveState = useCallback( | ||
| async (next: AdeExecutionTargetsState) => { | ||
| setBusy(true); | ||
| try { | ||
| await persist(next); | ||
| } finally { | ||
| setBusy(false); | ||
| } | ||
| }, | ||
| [persist], |
There was a problem hiding this comment.
Surface execution-target save failures in the settings UI.
persist() can reject, but saveState() only flips busy, and the click handlers below discard the returned promise. A failed IPC write therefore becomes a silent/unhandled rejection while the add/remove/activate action appears to do nothing.
Also applies to: 137-152, 188-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsx`
around lines 36 - 45, The save operation currently swallows persist() failures
(saveState calls persist but only toggles setBusy), so update saveState to catch
persist errors and surface them to the UI (either by rethrowing the error so
callers can await it or by setting a local error state like saveError via
setSaveError) and ensure setBusy is still cleared in finally; then update the
click handlers that call saveState (the add/remove/activate handlers) to await
saveState and show an inline error/toast/modal when it rejects or when saveError
is set so IPC write failures are visible to the user; apply the same change to
the other saveState usages mentioned (the additional call sites in this
component).
Comment on lines
+147
to
+155
| <button | ||
| type="button" | ||
| style={{ ...outlineButton({ height: 24, padding: "0 6px", fontSize: 9 }), color: COLORS.danger }} | ||
| title="Remove target" | ||
| onClick={() => void removeTarget(p.id)} | ||
| disabled={busy} | ||
| > | ||
| <Trash size={12} /> | ||
| </button> |
There was a problem hiding this comment.
Give the trash button an accessible name.
title is not a reliable accessible name for an icon-only button. Add an aria-label that includes the target label so assistive tech can tell which row will be removed.
Proposed fix
<button
type="button"
style={{ ...outlineButton({ height: 24, padding: "0 6px", fontSize: 9 }), color: COLORS.danger }}
title="Remove target"
+ aria-label={`Remove target ${executionTargetSummaryLabel(p)}`}
onClick={() => void removeTarget(p.id)}
disabled={busy}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsx`
around lines 147 - 155, The icon-only trash button in
ExecutionTargetsSection.tsx lacks an accessible name; update the button (the
element that calls removeTarget(p.id) and renders <Trash />) to include an
aria-label that includes the target's label (e.g., "Remove target {p.label}") so
screen readers can identify which row will be removed; you can keep the existing
title but ensure aria-label is present and built from the row's label property
used in the component.
Comment on lines
+166
to
+175
| <div style={{ display: "grid", gap: 8 }}> | ||
| <input style={FIELD} placeholder="Display name" value={label} onChange={(e) => setLabel(e.target.value)} /> | ||
| <input style={FIELD} placeholder="SSH destination (user@host)" value={sshHost} onChange={(e) => setSshHost(e.target.value)} /> | ||
| <input | ||
| style={FIELD} | ||
| placeholder="Remote workspace path" | ||
| value={workspacePath} | ||
| onChange={(e) => setWorkspacePath(e.target.value)} | ||
| /> | ||
| <input style={FIELD} placeholder="Jump host (optional)" value={jumpHost} onChange={(e) => setJumpHost(e.target.value)} /> |
There was a problem hiding this comment.
Add real labels for the SSH target fields.
These inputs rely on placeholders as their only label. Once a value is entered, screen readers and voice control lose the field name, which makes the form much harder to complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsx`
around lines 166 - 175, The inputs in ExecutionTargetsSection use only
placeholders (Display name, SSH destination, Remote workspace path, Jump host)
which disappear for screen readers; add persistent labels for each input element
(e.g., a visible <label> or aria-label/aria-labelledby) so screen readers and
voice control always have the field name; update the inputs tied to state
setters (label/setLabel, sshHost/setSshHost, workspacePath/setWorkspacePath,
jumpHost/setJumpHost) to reference those labels and ensure accessibility
attributes (htmlFor/id or aria-label) are present and unique for each field.
Comment on lines
+13
to
+28
| const refresh = useCallback(async () => { | ||
| const root = typeof projectRoot === "string" ? projectRoot.trim() : ""; | ||
| if (!root) { | ||
| setState(defaultExecutionTargetsState()); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| try { | ||
| const next = await window.ade.executionTargets.get(); | ||
| setState(next); | ||
| } catch { | ||
| setState(defaultExecutionTargetsState()); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [projectRoot]); |
There was a problem hiding this comment.
Ignore stale IPC results after a project switch.
Both refresh() and persist() call setState() after awaiting IPC, but nothing checks that the hook is still scoped to the same project when the promise resolves. If the user switches projects while one of these requests is in flight, a late response from the previous project can overwrite the current execution-target state.
Also applies to: 34-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/hooks/useExecutionTargets.ts` around lines 13 - 28,
The refresh() and persist() handlers (which call
window.ade.executionTargets.get() and setState/defaultExecutionTargetsState())
must ignore stale IPC responses after a project switch: capture the current
project identifier (e.g. compute const root = typeof projectRoot === "string" ?
projectRoot.trim() : "" or a local token) before awaiting, then after each await
compare that captured value to the current projectRoot (or compare a shared
requestId/token) and only call setState or setLoading if they still match; apply
the same pattern to persist() so late responses from a previous project do not
overwrite the new project state.
Comment on lines
+63
to
+90
| for (const entry of profilesIn) { | ||
| if (!entry || typeof entry !== "object") continue; | ||
| const e = entry as Partial<AdeExecutionTargetProfile>; | ||
| if (e.kind === "local" && e.id === ADE_LOCAL_EXECUTION_TARGET_ID) { | ||
| const label = typeof e.label === "string" && e.label.trim() ? e.label.trim() : "This computer"; | ||
| profiles.push({ id: ADE_LOCAL_EXECUTION_TARGET_ID, kind: "local", label }); | ||
| sawLocal = true; | ||
| continue; | ||
| } | ||
| if (e.kind === "ssh" && typeof e.id === "string" && e.id.trim()) { | ||
| const id = e.id.trim(); | ||
| if (id === ADE_LOCAL_EXECUTION_TARGET_ID) continue; | ||
| const label = typeof e.label === "string" && e.label.trim() ? e.label.trim() : id; | ||
| const sshHost = typeof e.sshHost === "string" ? e.sshHost.trim() : ""; | ||
| const workspacePath = typeof e.workspacePath === "string" ? e.workspacePath.trim() : ""; | ||
| const jumpHost = typeof e.jumpHost === "string" && e.jumpHost.trim() ? e.jumpHost.trim() : undefined; | ||
| const mode = e.connectionMode === "ade-runner" || e.connectionMode === "planned" ? e.connectionMode : "ssh-shell"; | ||
| if (!sshHost || !workspacePath) continue; | ||
| profiles.push({ | ||
| id, | ||
| kind: "ssh", | ||
| label, | ||
| sshHost, | ||
| workspacePath, | ||
| ...(jumpHost ? { jumpHost } : {}), | ||
| connectionMode: mode, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Drop duplicate target ids during normalization.
normalizeExecutionTargetsState() still accepts multiple local entries and multiple SSH profiles with the same id. Every consumer keys rows and selection by p.id, so duplicated ids lead to ambiguous activeTargetId resolution and duplicate React keys in the renderer.
Proposed fix
const profiles: AdeExecutionTargetProfile[] = [];
+ const seenIds = new Set<AdeExecutionTargetId>();
let sawLocal = false;
for (const entry of profilesIn) {
if (!entry || typeof entry !== "object") continue;
const e = entry as Partial<AdeExecutionTargetProfile>;
if (e.kind === "local" && e.id === ADE_LOCAL_EXECUTION_TARGET_ID) {
+ if (seenIds.has(ADE_LOCAL_EXECUTION_TARGET_ID)) continue;
const label = typeof e.label === "string" && e.label.trim() ? e.label.trim() : "This computer";
profiles.push({ id: ADE_LOCAL_EXECUTION_TARGET_ID, kind: "local", label });
+ seenIds.add(ADE_LOCAL_EXECUTION_TARGET_ID);
sawLocal = true;
continue;
}
if (e.kind === "ssh" && typeof e.id === "string" && e.id.trim()) {
const id = e.id.trim();
- if (id === ADE_LOCAL_EXECUTION_TARGET_ID) continue;
+ if (id === ADE_LOCAL_EXECUTION_TARGET_ID || seenIds.has(id)) continue;
const label = typeof e.label === "string" && e.label.trim() ? e.label.trim() : id;
const sshHost = typeof e.sshHost === "string" ? e.sshHost.trim() : "";
const workspacePath = typeof e.workspacePath === "string" ? e.workspacePath.trim() : "";
const jumpHost = typeof e.jumpHost === "string" && e.jumpHost.trim() ? e.jumpHost.trim() : undefined;
const mode = e.connectionMode === "ade-runner" || e.connectionMode === "planned" ? e.connectionMode : "ssh-shell";
if (!sshHost || !workspacePath) continue;
profiles.push({
id,
kind: "ssh",
label,
sshHost,
workspacePath,
...(jumpHost ? { jumpHost } : {}),
connectionMode: mode,
});
+ seenIds.add(id);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/shared/types/executionTargets.ts` around lines 63 - 90,
normalizeExecutionTargetsState currently allows duplicate local entries and SSH
profiles with the same id; update the loop over profilesIn (use symbols
profilesIn, profiles, sawLocal, ADE_LOCAL_EXECUTION_TARGET_ID) to deduplicate by
tracking seen ids: create a Set seenIds, when adding the local entry mark
ADE_LOCAL_EXECUTION_TARGET_ID as seen and skip additional locals if sawLocal or
seenIds has that id, and for SSH entries skip any profile whose trimmed id is
already in seenIds; add each pushed profile's id to seenIds so duplicates are
dropped during normalization.
- Add AdeExecutionTargetsState in project SQLite via ade_execution_targets key - Expose executionTargets get/set IPC and preload API - Include optional projectId on ProjectInfo for stable project scoping - Persist executionTargetId/label on agent chat sessions and summaries - Merge chat target fields into sessions.list for Work sidebar Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
- Top bar workspace target switcher and settings CRUD for SSH profiles - Context banners on Work and Files; chat composer target picker when configured - Session list badges when chat target differs from project active target - Wire Work/Lane work surfaces with full target profile list for picker Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
Agent-Logs-Url: https://github.com/arul28/ADE/sessions/25761bd5-f927-430a-ba53-270d1e565027 Co-authored-by: arul28 <31745423+arul28@users.noreply.github.com>
arul28
force-pushed
the
cursor/-bc-1763e942-e33d-49c1-9cb6-fa4101a980d4-aafb
branch
from
4763225 to
ec1c73e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
2024-2045:⚠️ Potential issue | 🟡 MinorRollback composer target state when session update fails
Line [2029] and Line [2030] optimistically update local picker state, but on failure (Line [2040]) the UI stays on an unsaved target. Please restore previous id/label in
catchso local state matches persisted state.Proposed fix
const handleComposerExecutionTargetChange = useCallback( async (targetId: string) => { const id = targetId.trim() || ADE_LOCAL_EXECUTION_TARGET_ID; const profile = targetProfilesForPicker.find((p) => p.id === id); const label = profile ? executionTargetSummaryLabel(profile) : id === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : id; + const previousId = composerExecutionTargetId; + const previousLabel = composerExecutionTargetLabel; setComposerExecutionTargetId(id); setComposerExecutionTargetLabel(label); if (!selectedSessionId) return; try { await window.ade.agentChat.updateSession({ @@ patchSessionSummary(selectedSessionId, { executionTargetId: id, executionTargetLabel: label }); void refreshSessions().catch(() => {}); } catch (err) { + setComposerExecutionTargetId(previousId); + setComposerExecutionTargetLabel(previousLabel); setError(err instanceof Error ? err.message : String(err)); } }, - [patchSessionSummary, refreshSessions, selectedSessionId, targetProfilesForPicker], + [ + composerExecutionTargetId, + composerExecutionTargetLabel, + patchSessionSummary, + refreshSessions, + selectedSessionId, + targetProfilesForPicker, + ], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 2024 - 2045, The handler handleComposerExecutionTargetChange currently optimistically calls setComposerExecutionTargetId/setComposerExecutionTargetLabel before persisting via window.ade.agentChat.updateSession, so if updateSession throws the catch should restore the previous local state; capture the prior id and label (e.g., prevId = composerExecutionTargetId, prevLabel = composerExecutionTargetLabel) before calling setComposerExecutionTargetId/setComposerExecutionTargetLabel, and in the catch block call setComposerExecutionTargetId(prevId) and setComposerExecutionTargetLabel(prevLabel) (and then setError as currently done); keep the existing calls to patchSessionSummary/updateSession/refreshSessions but ensure rollback happens only on failure to keep UI in sync with persisted session.apps/desktop/src/main/services/chat/agentChatService.ts (1)
13041-13065:⚠️ Potential issue | 🟠 MajorReset the label whenever the target id changes.
When
executionTargetIdchanges between two non-local targets and no new label is passed, Lines 13053-13056 keep any previous non-emptyexecutionTargetLabel. That leaves the session showing the old target name for the new id, and the same block still does not backfill a missing id before a label-only update.💡 Suggested fix
+ const previousExecutionTargetId = + typeof managed.session.executionTargetId === "string" && managed.session.executionTargetId.trim().length + ? managed.session.executionTargetId.trim() + : ADE_LOCAL_EXECUTION_TARGET_ID; + if (executionTargetId !== undefined) { const nextId = typeof executionTargetId === "string" && executionTargetId.trim().length ? executionTargetId.trim() : ADE_LOCAL_EXECUTION_TARGET_ID; managed.session.executionTargetId = nextId; + const targetIdChanged = nextId !== previousExecutionTargetId; if (executionTargetLabel !== undefined) { const nextLabel = typeof executionTargetLabel === "string" && executionTargetLabel.trim().length ? executionTargetLabel.trim() : nextId === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : nextId; managed.session.executionTargetLabel = nextLabel; } else if (nextId === ADE_LOCAL_EXECUTION_TARGET_ID) { managed.session.executionTargetLabel = "This computer"; - } else if (!managed.session.executionTargetLabel?.trim()) { + } else if (targetIdChanged || !managed.session.executionTargetLabel?.trim()) { managed.session.executionTargetLabel = nextId; } } else if (executionTargetLabel !== undefined) { + if (!managed.session.executionTargetId?.trim()) { + managed.session.executionTargetId = ADE_LOCAL_EXECUTION_TARGET_ID; + } const nextLabel = typeof executionTargetLabel === "string" && executionTargetLabel.trim().length ? executionTargetLabel.trim() : managed.session.executionTargetId === ADE_LOCAL_EXECUTION_TARGET_ID ? "This computer" : (managed.session.executionTargetId ?? "This computer"); managed.session.executionTargetLabel = nextLabel; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 13041 - 13065, When executionTargetId changes, always update managed.session.executionTargetLabel to reflect the new id (use "This computer" for ADE_LOCAL_EXECUTION_TARGET_ID or the trimmed id for non-local) instead of only setting it when the previous label is empty; also when handling a label-only update (executionTargetId === undefined and executionTargetLabel provided), ensure you backfill a missing managed.session.executionTargetId by setting it to ADE_LOCAL_EXECUTION_TARGET_ID before computing/storing managed.session.executionTargetLabel. Target symbols: executionTargetId, executionTargetLabel, ADE_LOCAL_EXECUTION_TARGET_ID, managed.session.executionTargetId, managed.session.executionTargetLabel.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/executionTargets/TopBarExecutionTargetSelect.tsx (1)
24-30: Consider adding error handling forsetActiveTargetId.The
onPickcallback awaitssetActiveTargetIdbut doesn't handle potential rejection. If the IPC call fails, the dropdown still closes and the error is silently swallowed.🛡️ Suggested improvement
const onPick = useCallback( async (id: string) => { - await setActiveTargetId(id); - setOpen(false); + try { + await setActiveTargetId(id); + } catch (err) { + console.error("Failed to set execution target:", err); + } finally { + setOpen(false); + } }, [setActiveTargetId], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/executionTargets/TopBarExecutionTargetSelect.tsx` around lines 24 - 30, The onPick callback currently awaits setActiveTargetId but does not handle rejections, causing the dropdown (setOpen) to close even if the IPC call fails; update the onPick function to wrap the await setActiveTargetId(id) in try/catch, only call setOpen(false) on success, and in the catch block log or surface the error (using existing logger or UI error handler) so failures are not silently swallowed; reference the onPick callback and the functions setActiveTargetId and setOpen when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 5643-5650: The rebuilt managed session can end up with
executionTargetId unset for pre-feature chats causing divergence vs
summarizeSessionRow(); when reconstructing the managed.session from persisted
(using persisted.executionTargetId and persisted.executionTargetLabel) ensure
you normalize missing/empty executionTargetId to the local target (e.g., set
executionTargetId to "local") so upgraded chats get a consistent target; update
the logic around persisted.executionTargetId / persisted.executionTargetLabel in
the managed session rebuild code (the block referencing
persisted.executionTargetId and executionTargetLabel) to substitute the local
identifier when executionTargetId is null/empty.
---
Duplicate comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 13041-13065: When executionTargetId changes, always update
managed.session.executionTargetLabel to reflect the new id (use "This computer"
for ADE_LOCAL_EXECUTION_TARGET_ID or the trimmed id for non-local) instead of
only setting it when the previous label is empty; also when handling a
label-only update (executionTargetId === undefined and executionTargetLabel
provided), ensure you backfill a missing managed.session.executionTargetId by
setting it to ADE_LOCAL_EXECUTION_TARGET_ID before computing/storing
managed.session.executionTargetLabel. Target symbols: executionTargetId,
executionTargetLabel, ADE_LOCAL_EXECUTION_TARGET_ID,
managed.session.executionTargetId, managed.session.executionTargetLabel.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 2024-2045: The handler handleComposerExecutionTargetChange
currently optimistically calls
setComposerExecutionTargetId/setComposerExecutionTargetLabel before persisting
via window.ade.agentChat.updateSession, so if updateSession throws the catch
should restore the previous local state; capture the prior id and label (e.g.,
prevId = composerExecutionTargetId, prevLabel = composerExecutionTargetLabel)
before calling setComposerExecutionTargetId/setComposerExecutionTargetLabel, and
in the catch block call setComposerExecutionTargetId(prevId) and
setComposerExecutionTargetLabel(prevLabel) (and then setError as currently
done); keep the existing calls to
patchSessionSummary/updateSession/refreshSessions but ensure rollback happens
only on failure to keep UI in sync with persisted session.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/components/executionTargets/TopBarExecutionTargetSelect.tsx`:
- Around line 24-30: The onPick callback currently awaits setActiveTargetId but
does not handle rejections, causing the dropdown (setOpen) to close even if the
IPC call fails; update the onPick function to wrap the await
setActiveTargetId(id) in try/catch, only call setOpen(false) on success, and in
the catch block log or surface the error (using existing logger or UI error
handler) so failures are not silently swallowed; reference the onPick callback
and the functions setActiveTargetId and setOpen when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 831be6fe-497b-484e-909b-b65950e167ff
📒 Files selected for processing (29)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/executionTargets/executionTargetsStateService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/projects/projectService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/executionTargets/ExecutionTargetContextBanner.tsxapps/desktop/src/renderer/components/executionTargets/TopBarExecutionTargetSelect.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/LaneWorkPane.tsxapps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsxapps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/hooks/useExecutionTargets.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/executionTargets.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/sessions.ts
✅ Files skipped from review due to trivial changes (6)
- apps/desktop/src/renderer/components/settings/WorkspaceSettingsSection.tsx
- apps/desktop/src/shared/types/index.ts
- apps/desktop/src/shared/types/chat.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/shared/types/executionTargets.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/desktop/src/renderer/components/terminals/SessionListPane.tsx
- apps/desktop/src/renderer/components/app/TopBar.tsx
- apps/desktop/src/shared/ipc.ts
- apps/desktop/src/main/services/projects/projectService.ts
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
- apps/desktop/src/renderer/components/terminals/SessionCard.tsx
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/main/services/executionTargets/executionTargetsStateService.ts
- apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx
- apps/desktop/src/renderer/components/executionTargets/ExecutionTargetContextBanner.tsx
- apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx
- apps/desktop/src/renderer/components/settings/ExecutionTargetsSection.tsx
- apps/desktop/src/renderer/hooks/useExecutionTargets.ts
- apps/desktop/src/renderer/browserMock.ts
Comment on lines
+5643
to
+5650
| ...(persisted?.executionTargetId != null && String(persisted.executionTargetId).trim().length | ||
| ? { | ||
| executionTargetId: String(persisted.executionTargetId).trim(), | ||
| ...(persisted?.executionTargetLabel != null && String(persisted.executionTargetLabel).trim().length | ||
| ? { executionTargetLabel: String(persisted.executionTargetLabel).trim() } | ||
| : {}), | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
Normalize upgraded chats to the local target when rebuilding the managed session.
Pre-feature chat metadata will not have these fields, so resumed sessions can keep managed.session.executionTargetId unset here while summarizeSessionRow() later reports "local". That split state will make upgraded chats behave differently depending on which API the UI reads.
💡 Suggested fix
- ...(persisted?.executionTargetId != null && String(persisted.executionTargetId).trim().length
- ? {
- executionTargetId: String(persisted.executionTargetId).trim(),
- ...(persisted?.executionTargetLabel != null && String(persisted.executionTargetLabel).trim().length
- ? { executionTargetLabel: String(persisted.executionTargetLabel).trim() }
- : {}),
- }
- : {}),
+ ...(() => {
+ const explicitExecutionTargetId =
+ typeof persisted?.executionTargetId === "string" && persisted.executionTargetId.trim().length
+ ? persisted.executionTargetId.trim()
+ : null;
+ const executionTargetId = explicitExecutionTargetId ?? ADE_LOCAL_EXECUTION_TARGET_ID;
+ const executionTargetLabel =
+ explicitExecutionTargetId && typeof persisted?.executionTargetLabel === "string" && persisted.executionTargetLabel.trim().length
+ ? persisted.executionTargetLabel.trim()
+ : executionTargetId === ADE_LOCAL_EXECUTION_TARGET_ID
+ ? "This computer"
+ : executionTargetId;
+ return { executionTargetId, executionTargetLabel };
+ })(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 5643 -
5650, The rebuilt managed session can end up with executionTargetId unset for
pre-feature chats causing divergence vs summarizeSessionRow(); when
reconstructing the managed.session from persisted (using
persisted.executionTargetId and persisted.executionTargetLabel) ensure you
normalize missing/empty executionTargetId to the local target (e.g., set
executionTargetId to "local") so upgraded chats get a consistent target; update
the logic around persisted.executionTargetId / persisted.executionTargetLabel in
the managed session rebuild code (the block referencing
persisted.executionTargetId and executionTargetLabel) to substitute the local
identifier when executionTargetId is null/empty.
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
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
Introduces a first-cut execution target model aligned with the “single active workspace target + per-chat tagging” direction: users can define targets (local + SSH metadata), pick an active target from the top bar, see context banners on Work and Files, and tag chats with a target from the composer. Agent tools still run on the local machine; SSH targets are stored as metadata with explicit “planned” vs future connection modes until remote execution is implemented.
What changed
AdeExecutionTargetsStatestored in project SQLite underade_execution_targets:<projectId>.window.ade.executionTargets.get/set.projectIdfor stable scoping (set when a project context is initialized).executionTargetId/executionTargetLabelon create, update, persisted chat JSON, summaries, and merged intosessions.listfor the Work sidebar.Validation
npm --prefix apps/desktop run typechecknpm --prefix apps/desktop run lintnpm --prefix apps/desktop run test -- --run src/main/services/chat/agentChatService.test.tsFollow-ups (not in this PR)
Summary by CodeRabbit