Cli Session Launch Latency#377
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughRemoves conditional PTY ChangesPTY initial-input, optimistic sessions, and terminal behavior
Composer Draft Persistence & Multi-Job Launch System
Claim / Tool Ownership APIs and CLI
UI wiring and tests for ownership & sidebar behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review
Scope: 16 file(s), +1268 / −459
Verdict: Minor issues
This PR speeds up Work draft chat/CLI launches with optimistic session rows, async session-list refresh, non-blocking PTY initial input, persisted composer drafts, and multi-step DraftLaunchJob status UI. The main residual risk is optimistic rows being overwritten by a fast forced refresh before the backend list includes the new session.
🐛 Functionality
[High] Forced refresh can drop optimistic lane sessions before the tab opens
File: apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts:L161-L168
Issue: launchPtySession inserts an optimistic row, opens the tab, then fires refresh({ force: true }) without awaiting it. refresh replaces sessions with the IPC list only; unlike useWorkSessions, there is no pending-optimistic merge, so a refresh that returns before persistence will remove the just-launched session id from sessions / laneOpenItemIds.
Repro: Launch a tracked CLI/shell session from a lane Work pane on a fast machine (or with a warm listSessionsCached); if the forced refresh wins the race, the new tab id disappears and the pane falls back to another session or draft until a later refresh.
Fix: Reuse the pendingOptimisticSessionsRef / mergePendingOptimisticSession pattern from useWorkSessions.ts, or merge the create result into refresh output until the authoritative list contains the session id.
[Medium] Initial prompt write failures are no longer surfaced for Work/Lane/sync launches
File: apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.ts:L522-L533
Issue: This PR stops passing awaitInitialInput: true when initialInput is present (also in useWorkSessions.ts and apps/ade-cli/src/services/sync/syncRemoteCommandService.ts). ptyService then schedules writeInitialInput() asynchronously and ignores failures (void writeInitialInput().catch(() => {}) in apps/desktop/src/main/services/pty/ptyService.ts around L3467), so create/launch can succeed while the prompt never lands.
Repro: Launch a Work draft CLI session (or work.startCliSession over sync) with initialInput against a PTY that exits immediately or rejects the first write; the UI shows a running session but no injected prompt and no launch error.
Fix: Keep awaitInitialInput: true for launch paths that depend on initialInput, or propagate write failures back through pty.create / the draft-launch job failed state.
⚡ Performance
[Medium] Composer draft persistence rewrites large visual context on every edit
File: apps/desktop/src/renderer/components/chat/AgentChatPane.tsx:L5145-L5170
Issue: The new writeComposerDraftSnapshot effect persists iosElementContextItems, appControlContextItems, and related fields (including screenshotDataUrl) to localStorage on every change to draft, attachments, controls, and context arrays. Large base64 screenshots make JSON.stringify + setItem run on each keystroke, which can jank the composer and hit quota (writes fail silently in the catch block at L1789-L1793).
Impact: Typing in a draft with one or more simulator/app-control screenshots can trigger multi‑MB main-thread writes per keypress; persistence may stop working without user feedback.
Fix: Debounce/throttle persistence, strip or externalize screenshot payloads from the stored snapshot (keep paths/ids only), or persist text/model separately from heavy context blobs.
Notes
Positive patterns worth keeping: hasFetchedOnceRef / userIsViewingDraft guards reduce draft↔tab fighting; freezing launch settings in DraftLaunchSnapshot avoids model/permission drift mid-flight; malformed localStorage draft entries are normalized instead of crashing the composer.
I did not run the desktop test suite in this environment; findings are from diff review and tracing the launch/refresh paths end to end.
Sent by Cursor Automation: BUGBOT in Versic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/components/chat/AgentChatPane.tsx (1)
5331-5345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let the first send override an orchestrator-lead launch back to the snapshot mode.
The create path explicitly forces
interactionMode: "orchestrator-lead"for orchestrator drafts, but the firstagentChat.send(...)still usesprepared.interactionModefrom the pre-launch snapshot. That can downgrade the very first turn to"default"/"plan"and skip the orchestration tool gates you just enabled. UsecreatedSession.interactionModehere, or omit the send-time override for orchestrator launches.Also applies to: 5693-5702
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 5331 - 5345, The first send call is using prepared.interactionMode (the pre-launch snapshot) which can downgrade an orchestrator launch; after creating the session with window.ade.agentChat.create (where orchestratorOverrides sets interactionMode: "orchestrator-lead"), change the send-time override so it uses created.interactionMode (or createdSession.interactionMode) instead of prepared.interactionMode — or simply omit the send-time interactionMode override for orchestrator launches; update the same pattern found around agentChat.send in the later block (lines ~5693-5702) to ensure the orchestrator-lead mode is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1653-1732: The renderer currently re-serializes and writes full
visual-context (including screenshotDataUrl blobs) on every draft keystroke;
update the draft-persistence effect in AgentChatPane to (1) debounce the save
(e.g., 300–500ms) so rapid keystrokes don't trigger synchronous writes and (2)
strip large fields before persisting by removing screenshotDataUrl (and any
other heavy binary/blob fields) from items produced by
normalizeComposerIosContextItems, normalizeComposerAppControlContextItems,
normalizeComposerBuiltInBrowserContextItems and
normalizeComposerMacosVmContextItems—preserve stable ids/attachment paths (id,
componentId, sessionId, any stored path fields) so restore still works. Ensure
the debounced function is cleaned up on unmount and that the persisted payload
is small enough to avoid localStorage quota failures.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 5331-5345: The first send call is using prepared.interactionMode
(the pre-launch snapshot) which can downgrade an orchestrator launch; after
creating the session with window.ade.agentChat.create (where
orchestratorOverrides sets interactionMode: "orchestrator-lead"), change the
send-time override so it uses created.interactionMode (or
createdSession.interactionMode) instead of prepared.interactionMode — or simply
omit the send-time interactionMode override for orchestrator launches; update
the same pattern found around agentChat.send in the later block (lines
~5693-5702) to ensure the orchestrator-lead mode is preserved.
🪄 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: 4c523276-071b-4305-897e-47970e88576d
⛔ Files ignored due to path filters (5)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (11)
apps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/services/sync/syncHostService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.test.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/OrchestratorComposerEntry.test.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.ts
💤 Files with no reviewable changes (4)
- apps/desktop/src/renderer/components/terminals/OrchestratorComposerEntry.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.test.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
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/resources/agent-skills/ade-browser/SKILL.md (1)
11-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
claimto the allowlisted-method list for consistency.Line 11 and Line 28 document
claim, but the method list in Line 17 omits it. That makes the same page internally inconsistent and can mislead operators.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/resources/agent-skills/ade-browser/SKILL.md` around lines 11 - 17, The allowlisted-methods list in SKILL.md for BuiltInBrowserService is missing the claim method described elsewhere; update the allowlisted methods array/list shown (the list that currently contains getStatus, showPanel, setBounds, navigate, createTab, switchTab, closeTab, reload, goBack, goForward, stop, startInspect, stopInspect, captureScreenshot, selectPoint, selectCurrent, clearSelection) to include claim so the documentation is internally consistent with the other references to claim and the CLI behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 5617-5627: The claim subcommand blocks (the branches where sub ===
"claim" in apps/ade-cli/src/cli.ts that build the execute action for
ios_simulator and the other two occurrences) must validate that a lane argument
was provided; before returning the execute descriptor (and before calling
collectGenericObjectArgs/readToolClaimArgs), check for the presence of the lane
flag/parameter (e.g., args.lane or the parsed flag used for lanes) and if
missing return a CLI parse error/usage failure (or throw) so the command fails
fast; update all three claim branches to perform this validation and surface a
clear message when lane is absent.
---
Outside diff comments:
In `@apps/desktop/resources/agent-skills/ade-browser/SKILL.md`:
- Around line 11-17: The allowlisted-methods list in SKILL.md for
BuiltInBrowserService is missing the claim method described elsewhere; update
the allowlisted methods array/list shown (the list that currently contains
getStatus, showPanel, setBounds, navigate, createTab, switchTab, closeTab,
reload, goBack, goForward, stop, startInspect, stopInspect, captureScreenshot,
selectPoint, selectCurrent, clearSelection) to include claim so the
documentation is internally consistent with the other references to claim and
the CLI behavior.
🪄 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: 3ab56131-842e-453f-83ed-024c41e64118
📒 Files selected for processing (34)
apps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/builtInBrowser/desktopBridgeClient.tsapps/desktop/resources/agent-skills/ade-app-control/SKILL.mdapps/desktop/resources/agent-skills/ade-browser/SKILL.mdapps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.mdapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/appControl/appControlService.test.tsapps/desktop/src/main/services/appControl/appControlService.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/builtInBrowser/desktopBridgeServer.tsapps/desktop/src/main/services/ios/iosSimulatorService.test.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.test.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.test.tsxapps/desktop/src/renderer/components/terminals/WorkSidebar.tsxapps/desktop/src/shared/types/appControl.tsapps/desktop/src/shared/types/builtInBrowser.tsapps/desktop/src/shared/types/iosSimulator.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx
- apps/desktop/resources/agent-skills/ade-ios-simulator/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/components/lanes/useLaneWorkSessions.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
arul28
force-pushed
the
ade/cli-session-launch-latency-5e13552e
branch
from
410c906 to
e8920a1
Compare
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
Greptile Summary
This PR reduces CLI session launch latency by making session creation optimistic — the terminal opens immediately after PTY launch returns rather than waiting for a backend refresh. It also introduces persistent composer draft snapshots (stored in
localStoragewith debounced writes), a new multi-job launch-status strip replacing the singlebackgroundLaunchNotice, and an explicit "claim" ownership model for browser, iOS simulator, and app-control panels.useLaneWorkSessionsnow stores apendingOptimisticSessionmap (TTL 2 min) so the terminal tab opens before the server confirms the session, merging the optimisticptyIdinto persisted rows as they arrive.writeComposerDraftSnapshotwrites aComposerDraftStorageSnapshottolocalStorageon a 350 ms debounce; the hydration effect reads it back on scope switch, guarded bycomposerDraftHydratingRefto skip the write-back triggered by the hydration state batch.builtInBrowserService,iosSimulatorService, andappControlServiceeach gain aclaim({ laneId, chatSessionId })endpoint, replacing the previous auto-attach-on-session-creation path for the iOS simulator.Confidence Score: 5/5
Safe to merge. The optimistic session, draft persistence, and claim ownership changes are well-scoped and independently fallback-safe.
The optimistic session map has a 2-minute TTL and graceful server-reconciliation logic, so stale entries clean themselves up. Draft persistence uses a debounced write with a flush-on-cleanup guard, and the hydration flag correctly prevents write-back races for the common case. The new claim endpoints only update in-memory owner state and emit status events with no persistent side effects. The failInitialInputLaunch change is a correctness improvement over the previous silent-discard. No data-loss or session-corruption paths were identified.
AgentChatPane.tsx warrants extra attention due to its size and the interplay between the hydration effect, write effect, and composerDraftHydratingRef guard.
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User participant CP as AgentChatPane participant LWS as useLaneWorkSessions participant PTY as ptyService participant BE as Backend U->>CP: Click Launch CP->>CP: buildDraftLaunchSnapshotForCurrentState() CP->>CP: clearDraftLaunchComposer(snapshot) CP->>CP: setDraftLaunchJobs creating-lane CP->>CP: resolveDraftLaunchLane [async] CP->>LWS: launchPtySession(launchFields) LWS->>PTY: spawn PTY process PTY-->>LWS: sessionId + ptyId [immediate] LWS->>LWS: pendingOptimisticSessions.set(sessionId) LWS->>LWS: upsertSessionSnapshot [UI shows terminal NOW] LWS->>LWS: selectLane / focusSession / openSessionTab LWS-->>BE: refresh() [background] PTY->>PTY: writeInitialInput() [async] BE-->>LWS: persisted session returned LWS->>LWS: mergePendingOptimisticSession CP->>CP: patchDraftLaunchJob ready CP->>CP: openLaunchedDraftSessionComments Outside Diff (2)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx, line 5389-5425 (link)writeComposerDraftSnapshotis called on every render where any dependency changes — includingdraft(which updates on every keystroke) andcurrentNativeControls(which may produce a new object reference on each render). With large context snapshots (many attachments, VM items, orchestration annotations), this is aJSON.stringify+localStorage.setItemcall per character typed. Consider debouncing the write or using asetTimeout/requestIdleCallbackto coalesce rapid changes.Prompt To Fix With AI
apps/desktop/src/renderer/components/terminals/OrchestratorComposerEntry.test.tsxade:work:start-orchestrator-chatevent contract droppedThis file was deleted but none of its tests were ported to
AgentChatPane.test.tsx. TheAgentChatPane.test.tsmigration did port all of its describe blocks (the diff explicitly labels them "consolidated fromAgentChatPane.test.ts"), butOrchestratorComposerEntry.test.tsxwas deleted silently. Theade:work:start-orchestrator-chatevent dispatch contract — the only mechanism by which the composer routes to thechat-orchestratordraft kind — is now untested.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "Fix rebase follow-up validation issues" | Re-trigger Greptile