Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughIntroduces a comprehensive app control system enabling desktop Electron application orchestration via Chrome DevTools Protocol, along with chat-scoped terminal support. New Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Copilot stopped work on behalf of
arul28 due to an error
April 30, 2026 03:12
![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: 13
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/SessionListPane.tsx (1)
239-319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep child nesting bucket-local in time/status views.
childrenByParentIdandexcludedTopLevelIdsare derived fromallSessions, then reused for every grouping mode. If a parent is inrunningbut its child is inended/older, the child gets removed from its own bucket and either shows under the wrong header or disappears entirely when the parent bucket is collapsed. Build the child map from the current rendered list, or limit this nesting behavior toby-lane.Also applies to: 320-360, 427-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx` around lines 239 - 319, childrenByParentId and excludedTopLevelIds are built from allSessions and reused for every grouping mode which causes children to be moved or hidden when parent and child fall into different time/status buckets; change the logic so the child map is built from the currently rendered session list for the active grouping mode (e.g. for time/status views use sessions derived from timeBuckets or the current bucket list, while preserving full nesting only for by-lane views using sessionsGroupedByLane). Update the computation sites for childrenByParentId and excludedTopLevelIds to accept a source array (or create two variants) so expandSessionWithChildren and collectVisibleIds operate against the grouping-local child map, and ensure their use in the time/status render paths uses the grouping-local map rather than the global allSessions-derived map.
🧹 Nitpick comments (2)
apps/ade-cli/src/cli.test.ts (1)
1314-1439: ⚡ Quick winAdd one bootstrap-level App Control smoke test.
These assertions stop at
buildCliPlan(), but the newapp-control/terminalsurface only really pays off oncebootstrap.tspicks the desktop socket transport and dispatches the RPC. One small end-to-end CLI test there would catch routing regressions this suite cannot see.Based on learnings: "For ADE CLI changes, verify both headless mode and the desktop socket-backed ADE RPC path."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.test.ts` around lines 1314 - 1439, Add a bootstrap-level smoke test that goes beyond buildCliPlan by invoking the application's bootstrap entry (bootstrap.ts) to exercise the desktop-socket ADE RPC path: start the bootstrap process (the module/function that selects transports in bootstrap.ts), point it at a test desktop socket transport or a mocked desktop socket, then run a real CLI invocation equivalent to buildCliPlan(["app-control","status"]) and assert the RPC reached App Control (e.g., that the dispatched RPC action/domain is "app_control"/"getStatus" and returned/caused the expected execute step). Ensure the test also runs the headless path for parity; reference buildCliPlan and the bootstrap startup code in bootstrap.ts to wire the transport selection and teardown the bootstrap process after the assertion.apps/desktop/src/main/services/appControl/appControlService.test.ts (1)
127-133: ⚡ Quick winTear down the polling service between tests.
Each case leaves a connected service running under fake timers. That background health poll can consume
mockState.httpResponsesor emit WebSocket traffic during a later test, which makes the suite order-dependent. Track created services and dispose them in teardown, or at least clear timers after each test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/appControl/appControlService.test.ts` around lines 127 - 133, Tests are leaving the background health polling service running under fake timers; track any services created by appControlService in the tests and ensure they are torn down in afterEach by calling their stop/dispose method (e.g., service.dispose() or service.stopPolling()) and by clearing fake timers with vi.clearAllTimers() and restoring with vi.useRealTimers(); additionally reset mockState arrays as already done to avoid cross-test interference.
🤖 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/ade-cli/src/bootstrap.ts`:
- Around line 483-497: In resolveLaneId, the chatSession lookup currently uses
sessionService.get (which reads terminal_sessions) and attachedRootPath uses
only equality; change the chat lookup to use the chat-session store/service
(e.g., chatSessionService.get or chatSessionStore.get) instead of
sessionService.get so chatSessionId resolves actual ADE chat sessions, and
update the lane match logic in the lanes.find loop to mirror the worktreePath
descendant check for attachedRootPath (use path.resolve and
startsWith(`${attachedRootPath}${path.sep}`) in addition to exact equality) so
--cwd inside attached lane subdirectories matches correctly; keep using
laneService.list to load lanes and preserve the existing fallback behavior.
In `@apps/ade-cli/src/cli.ts`:
- Around line 2359-2364: readTrailingCommand currently rebuilds the post---
command by joining tokens which destroys quoted boundaries; change
readTrailingCommand to return string[] | null (preserve original tokens) by
doing const trailing = args.slice(index + 1); args.splice(index); return
trailing.length ? trailing : null, and update all callers of readTrailingCommand
(e.g., launch code that expects a single string) to accept the array and either
pass it directly as argv to child_process.spawn/execFile or only join with
spaces when intentionally invoking a shell.
In `@apps/desktop/scripts/dev.cjs`:
- Around line 204-207: The current validation of remoteDebugPortRaw using
Number.parseInt allows malformed values like "9222abc" and doesn't enforce the
TCP port range; update the check around remoteDebugPortRaw/remoteDebugPort to
first ensure remoteDebugPortRaw is a pure decimal string (e.g. /^\d+$/) before
parsing, then parse with base 10 and assert Number.isFinite(remoteDebugPort) and
that remoteDebugPort is between 1 and 65535 inclusive, otherwise throw the same
Error with the original raw value; apply this to the block that defines
remoteDebugPort and the related error creation.
In `@apps/desktop/src/main/main.ts`:
- Around line 2728-2733: The lane matching predicate in matchingLane currently
only checks exact equality for attachedRootPath, which misses cases where
targetRoot is a subdirectory of the attached root; update the predicate inside
lanes.find (the arrow function using worktreePath, attachedRootPath, and
targetRoot) to also treat targetRoot as belonging to the attached root when it
starts with `${attachedRootPath}${path.sep}` (and guard for attachedRootPath
being null), mirroring the existing startsWith logic used for worktreePath so
subdirectories of attachedRootPath resolve to the correct lane.
In `@apps/desktop/src/main/services/appControl/appControlService.ts`:
- Around line 1246-1276: stop() is marking terminal-backed sessions as "stopped"
immediately after signaling the PTY; instead, when terminalSessionId is present
and args.ptyService is used, keep the session in "stopping" and do not set
activeSession to "stopped" or emit the final "session-stopped" here — let the
PTY exit callback (the existing PTY/terminal exit handler) set the final
"stopped" | "exited" | "failed" state and emit session-updated/session-stopped.
Concretely: in stop() (reference symbols: stop, activeSession,
terminalSessionId, args.ptyService.signalTerminal, stopScreencast, emit) avoid
creating stoppedSession/setting activeSession to stopped and avoid emitting
"session-stopped" when terminalSessionId is truthy; instead preserve "stopping",
return the previous session in its stopping state, and ensure the PTY exit
handler is responsible for publishing final session status.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 5815-5864: Handlers ipcMain.handle for IPC.appControlScroll,
IPC.appControlAttachToTarget and IPC.appControlDispatchKey are currently
coercing bad payloads instead of rejecting them; update each handler to validate
inputs and fail closed: for appControlScroll use Number.isFinite on numeric
fields returned by numberOr and reject if any required numeric is NaN/Infinity,
for appControlAttachToTarget treat rawTargetId as valid only when it's a
non-empty string and reject otherwise (do not convert missing to ""), and for
appControlDispatchKey check rawType against the allowed set
("keyDown","keyUp","rawKeyDown","char") and reject unknown values rather than
defaulting to "keyDown"; in all three handlers call the existing App Control
validators (via ensureAppControl() validation API) or throw a bad-payload error
so the request is rejected instead of synthesizing inputs.
In `@apps/desktop/src/main/services/pty/ptyService.test.ts`:
- Around line 1811-1821: The test currently checks exclusion of the other-chat
terminal by matching a string pattern; instead capture the terminal returned by
service.create for the "C" chat (e.g., const c = await service.create(...)) and
assert the list of terminal IDs from service.listTerminals({ chatSessionId:
"chat-1" }) does not contain that actual terminal/session id (compare
c.sessionId to the mapped terminalId values), and keep the final assertion that
list[0]?.terminalId equals b.sessionId to verify sorting.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 1265-1284: resolveTerminalId can return a stale dead
activeTerminalId after clearing activeTerminalByChatSession; update the final
branch so that when no live replacement is found you delete the stale map entry
and return null (not the old activeTerminalId). In practice, keep the existing
checks (cleanOptionalId, liveEntryBySessionId(activeTerminalId), replacement
logic using ptys and createdAt) but change the last line in resolveTerminalId to
return null after activeTerminalByChatSession.delete(chatSessionId) so callers
like readTerminal({ chatSessionId }) won't receive a closed session id.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 2619-2636: The badge for the App Control count is absolutely
positioned but its toggle button is not a positioned ancestor, causing the pill
to drift; update the AgentChatComposer button (the element using
onToggleAppControl, appControlOpen and rendering the Desktop icon and
appControlContextItems) to be a positioned ancestor by adding a positioning
class (e.g., Tailwind "relative") to the button's className so the absolutely
positioned badge anchors to the button.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1562-1582: When the companionStateKey changes the UI is rehydrated
but the App Control context (appControlContextItems) remains global and can leak
between chats; update the companion-state-change logic (around
companionHydrationKeyRef, companionStateKey, readChatCompanionUiState,
writeChatCompanionUiState) to also reset or re-key the App Control context:
either include companionStateKey when storing/reading appControlContextItems or
explicitly clear/reset appControlContextItems (via whatever setter e.g.
setAppControlContextItems or equivalent) in the same effect that reads the saved
UI state so the new chat starts with an empty App Control context.
In `@apps/desktop/src/renderer/components/chat/ChatAppControlPanel.tsx`:
- Around line 1192-1257: The inspect overlay math currently always uses
snapshot.screenshot.width/height causing drift when the <img> shows the live
screencast; create a small helper (e.g., getDisplayedImageDims or
computeDisplayedDims) that returns the displayed image width/height and scale
based on liveFrameActive and liveFrameDimsRef (fall back to
screenshot.width/height when not live), then update the onMouseMove hit-test
(the anonymous handler that sets hoveredElementId), and all overlay style
computations for selectedElement, hoverElement and selectedPoint to use that
helper's width/height/scale instead of snapshot.screenshot.width/height so
hover/selection coordinates match the visible image (also ensure
handleImageClick uses the same helper or liveFrameDimsRef as it already does).
In `@apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx`:
- Around line 262-301: The effects treat revealRequest as sticky truthy state
causing stale requests to block auto-create; add a handledRevealNonceRef (or
lastHandledChatKey) to record the nonce/chat-key from revealRequest when you
consume it in the reveal-handling useEffect (the block that builds nextEntry and
setsActiveTabId) and update both useEffect checks to ignore revealRequest whose
nonce equals handledRevealNonceRef.current so revealRequest is edge-triggered:
when you handle a revealRequest set handledRevealNonceRef to its nonce (or chat
key) and in the auto-create useEffect only bail out for revealRequest if its
nonce is different from handledRevealNonceRef.current; ensure this ref is
consulted alongside pendingAutoCreateRef, creatingTab, restoringTabs and when
creating a tab clear/consume the nonce so subsequent opens can auto-create
normally.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/terminals/SessionListPane.tsx`:
- Around line 239-319: childrenByParentId and excludedTopLevelIds are built from
allSessions and reused for every grouping mode which causes children to be moved
or hidden when parent and child fall into different time/status buckets; change
the logic so the child map is built from the currently rendered session list for
the active grouping mode (e.g. for time/status views use sessions derived from
timeBuckets or the current bucket list, while preserving full nesting only for
by-lane views using sessionsGroupedByLane). Update the computation sites for
childrenByParentId and excludedTopLevelIds to accept a source array (or create
two variants) so expandSessionWithChildren and collectVisibleIds operate against
the grouping-local child map, and ensure their use in the time/status render
paths uses the grouping-local map rather than the global allSessions-derived
map.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.test.ts`:
- Around line 1314-1439: Add a bootstrap-level smoke test that goes beyond
buildCliPlan by invoking the application's bootstrap entry (bootstrap.ts) to
exercise the desktop-socket ADE RPC path: start the bootstrap process (the
module/function that selects transports in bootstrap.ts), point it at a test
desktop socket transport or a mocked desktop socket, then run a real CLI
invocation equivalent to buildCliPlan(["app-control","status"]) and assert the
RPC reached App Control (e.g., that the dispatched RPC action/domain is
"app_control"/"getStatus" and returned/caused the expected execute step). Ensure
the test also runs the headless path for parity; reference buildCliPlan and the
bootstrap startup code in bootstrap.ts to wire the transport selection and
teardown the bootstrap process after the assertion.
In `@apps/desktop/src/main/services/appControl/appControlService.test.ts`:
- Around line 127-133: Tests are leaving the background health polling service
running under fake timers; track any services created by appControlService in
the tests and ensure they are torn down in afterEach by calling their
stop/dispose method (e.g., service.dispose() or service.stopPolling()) and by
clearing fake timers with vi.clearAllTimers() and restoring with
vi.useRealTimers(); additionally reset mockState arrays as already done to avoid
cross-test interference.
🪄 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: 2a016dd1-a6f0-4f12-8e8e-d3656826e10b
⛔ Files ignored due to path filters (6)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/computer-use/README.mdis excluded by!docs/**docs/features/computer-use/app-control.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (33)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/scripts/dev.cjsapps/desktop/src/main/main.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/chat/agentChatService.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/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatAppControlPanel.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/appControl.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/sessions.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Resources/DatabaseBootstrap.sqlapps/ios/ADE/Services/Database.swiftapps/ios/ADETests/ADETests.swift
arul28
force-pushed
the
ade/electron-viewer-0d20e278
branch
from
8241f6e to
8976756
Compare
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 30, 2026 03:36
…edaction/validation, app-control coord system, terminal session lifecycle)
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 30, 2026 03:56
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
CLI Updates
app-controlcommand suite for status, launch, connect, and interaction.terminalcommand for session management.Greptile Summary
This PR introduces an App Control system that uses Chrome DevTools Protocol (CDP) over WebSocket to launch, screenshot, inspect, and interact with Electron applications from within the chat interface. It also adds chat-scoped terminal management (persistent PTY sessions visible in a new
ChatTerminalDrawer) and corresponding CLI commands (app-control,terminal).The core service (
appControlService.ts) is well-structured: thesourceFileCacheis correctly scoped per-instance (not module-level), the IPC rate-limit buckets now include a stale-entry sweep, andresolveTerminalIdcorrectly returnsnullafter clearing a stale cache entry — all previous review concerns appear addressed. Two P2-level observations remain around the free-port allocation race and an async window in the launch path.Confidence Score: 5/5
Safe to merge; only P2-level quality observations, no blocking bugs found.
All findings are P2 (speculative timing issues). The three P1 items from previous review rounds are resolved. Trusted-sender assertion, rate limiting, and input validation are properly implemented on all new IPC handlers.
apps/desktop/src/main/services/appControl/appControlService.ts — findFreePort TOCTOU and concurrent-launch async gap.
Important Files Changed
findFreePortand an async window where a concurrent launch can orphan a PTY.Sequence Diagram
sequenceDiagram participant UI as ChatAppControlPanel participant IPC as registerIpc (main) participant ACS as AppControlService participant PTY as PtyService participant CDP as CdpClient (WebSocket) participant App as Electron App UI->>IPC: appControlLaunch(command, env) IPC->>IPC: assertTrustedSender + rateLimit IPC->>ACS: launch(args) ACS->>ACS: findFreePort() ACS->>ACS: resolveLaunch(args, port) ACS->>PTY: create({ startupCommand, env }) PTY-->>ACS: { sessionId, ptyId } ACS->>ACS: startCdpPoller(sessionId, port) App-->>CDP: binds --remote-debugging-port loop CDP poll (500ms) ACS->>App: GET /json/list App-->>ACS: CdpTarget[] ACS->>CDP: CdpClient.connect(wsUrl) ACS->>ACS: updateSession(connected) ACS->>CDP: Page.startScreencast end CDP-->>ACS: Page.screencastFrame events ACS-->>UI: appControlEvent(frame) UI->>IPC: appControlClick(x, y) IPC->>ACS: click(args) ACS->>CDP: Input.dispatchMouseEvent (press+release) UI->>IPC: appControlGetSnapshot() IPC->>ACS: getSnapshot() ACS->>CDP: Runtime.evaluate(domSnapshotScript) CDP-->>ACS: DomSnapshotPayload ACS-->>UI: AppControlSnapshot (elements + screenshot)Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: iteration 1 — address coderabbit r..." | Re-trigger Greptile