Improve ADE PR, lane, and TUI workflows#312
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR integrates four major features and supporting infrastructure changes: Claude terminal control input/UI toggle (Ctrl-T), lane deletion lifecycle cleanup with environment teardown, GitHub PR snapshot caching simplification (removing external-PR variants), and PR detail hydration logic to prevent stale snapshots from overwriting live data. ChangesMulti-feature Integration: Terminal Control, Lane Cleanup, and PR Data Layer Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans multiple coordinated features (terminal control, lane cleanup, snapshot caching, detail hydration) across heterogeneous platforms (CLI/TUI, desktop, iOS) with significant logic density in PR caching/hydration refactors, widespread consumers and test updates, and intricate state management around live-vs-snapshot precedence and in-flight request coordination. 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 |
|
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. |
Owner
Author
|
@copilot review but do not make fixes |
Contributor
Reviewed commit 6c3e39a end-to-end (PR routing/hydration, lane delete sequencing/cleanup, IPC timeout branching, and TUI terminal-control rendering) and did not apply any fixes as requested. I did not find a new blocker in those paths. I also attempted focused test runs, but this runner currently lacks test deps ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/ade-cli/tsup.config.ts (1)
80-87: 💤 Low valueConsider joining banner statements with newlines for readability.
The banner statements are currently joined with spaces, creating a single long line. While this works correctly, joining with
\nwould improve maintainability.♻️ Optional readability improvement
banner: { - js: [ - "import { createRequire as __adeCreateRequire } from 'node:module';", - "import { fileURLToPath as __adeFileURLToPath } from 'node:url';", - "import { dirname as __adeDirname } from 'node:path';", - "const require = __adeCreateRequire(import.meta.url);", - "const __filename = __adeFileURLToPath(import.meta.url);", - "const __dirname = __adeDirname(__filename);", - ].join(" "), + js: [ + "import { createRequire as __adeCreateRequire } from 'node:module';", + "import { fileURLToPath as __adeFileURLToPath } from 'node:url';", + "import { dirname as __adeDirname } from 'node:path';", + "const require = __adeCreateRequire(import.meta.url);", + "const __filename = __adeFileURLToPath(import.meta.url);", + "const __dirname = __adeDirname(__filename);", + ].join("\n"), },🤖 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/ade-cli/tsup.config.ts` around lines 80 - 87, The banner array assigned to js currently joins entries with a space which yields a single long line; update the join call from join(" ") to join("\n") so the created banner string places each statement on its own line (affecting the js array containing "__adeCreateRequire", "__adeFileURLToPath", "__adeDirname", the require declaration, __filename, and __dirname) to improve readability and maintainability.apps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsx (1)
404-404: ⚡ Quick winConsider using role-based query for consistency.
Other tests in this file use
getByRole("button", { name: "..." })to locate buttons (see lines 321, 333). For consistency and better accessibility testing practices, consider using a role-based query here as well:- expect(screen.queryByTitle("Open terminal")).toBeNull(); + expect(screen.queryByRole("button", { name: /open terminal/i })).toBeNull();This approach aligns with React Testing Library best practices and makes the test more resilient to implementation changes that might affect the
titleattribute.🤖 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.companionDrawers.test.tsx` at line 404, Replace the title-based query in the test that uses screen.queryByTitle("Open terminal") with a role-based query to match other tests: use screen.queryByRole("button", { name: "Open terminal" }) and keep the same assertion (toBeNull()) so the test asserts absence via role/name instead of title; update the test referencing the query in AgentChatPane.companionDrawers.test (the line with screen.queryByTitle("Open terminal")) to this new role-based query for consistency with other tests that use getByRole("button", { name: "..." }).apps/desktop/src/preload/preload.ts (1)
7104-7111: ⚡ Quick winDrop the dead
includeExternalClosedoption from the preload API.This refactor removes external-PR handling, but
ade.prs.getGitHubSnapshot()still advertisesincludeExternalClosed. Keeping it here preserves a no-op public contract and lets stale callers survive silently instead of being caught by TypeScript.Suggested fix
getGitHubSnapshot: (args?: { force?: boolean; - includeExternalClosed?: boolean; }): Promise<GitHubPrSnapshot> => callPrReadRuntimeActionOr( "getGithubSnapshot", { args: args ?? {} }, () => ipcRenderer.invoke(IPC.prsGetGitHubSnapshot, args ?? {}), ),🤖 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/preload/preload.ts` around lines 7104 - 7111, The preload API still advertises the dead includeExternalClosed option on getGitHubSnapshot; remove it from the public args shape so TypeScript will catch stale callers. Update the getGitHubSnapshot signature (the args object type used by getGitHubSnapshot / GitHubPrSnapshot call) to only include force?: boolean, and propagate that shape into the callPrReadRuntimeActionOr invocation and the ipcRenderer.invoke(IPC.prsGetGitHubSnapshot, ...) call (keep the existing args ?? {} handling). Ensure no other references rely on includeExternalClosed in this function so callers that still pass it will now be flagged by the compiler.
🤖 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/services/sync/syncRemoteCommandService.ts`:
- Around line 1478-1486: The pre-delete overlay context resolution is missing
archived lanes; update the call to resolveLaneOverlayContext (used when
args.laneEnvironmentService is truthy) to request archived lanes by passing
includeArchived: true (e.g., resolveLaneOverlayContext(args, deleteArgs.laneId,
{ includeArchived: true }) or the appropriate options argument), so envContext
includes archived lanes during pre-delete teardown and preserve the existing
catch/log behavior for errors.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 5119-5124: The current condition allows an in-flight snapshot to
publish when cachedGithubSnapshot === null, so a stale request can repopulate
the cache after invalidateGithubSnapshotCache() clears it; update the guard so
only the matching in-flight token can publish and only if the cache wasn't
explicitly cleared — i.e., change the condition using githubSnapshotInFlight,
inFlight, and cachedGithubSnapshot so publishGithubSnapshot(snapshot,
capturedAt) is called only when githubSnapshotInFlight === inFlight AND
cachedGithubSnapshot !== null (remove the || cachedGithubSnapshot === null
branch).
In `@apps/desktop/src/preload/preload.ts`:
- Around line 1422-1426: The local subscription created by
subscribeLocalSessionChangedEvents (via createLocalIpcEventSubscription and
IPC.sessionsChanged) does not clear sessionDeltaCache before notifying
subscribers, so handlers that call ade.sessions.getDelta() may read stale data;
modify the local fanout path in the subscription handler to invalidate/clear
sessionDeltaCache (the same way the remote/runtime-streamed path does)
immediately before emitting the "session changed" event to subscribers so any
onChanged handlers see a fresh getDelta().
---
Nitpick comments:
In `@apps/ade-cli/tsup.config.ts`:
- Around line 80-87: The banner array assigned to js currently joins entries
with a space which yields a single long line; update the join call from join("
") to join("\n") so the created banner string places each statement on its own
line (affecting the js array containing "__adeCreateRequire",
"__adeFileURLToPath", "__adeDirname", the require declaration, __filename, and
__dirname) to improve readability and maintainability.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 7104-7111: The preload API still advertises the dead
includeExternalClosed option on getGitHubSnapshot; remove it from the public
args shape so TypeScript will catch stale callers. Update the getGitHubSnapshot
signature (the args object type used by getGitHubSnapshot / GitHubPrSnapshot
call) to only include force?: boolean, and propagate that shape into the
callPrReadRuntimeActionOr invocation and the
ipcRenderer.invoke(IPC.prsGetGitHubSnapshot, ...) call (keep the existing args
?? {} handling). Ensure no other references rely on includeExternalClosed in
this function so callers that still pass it will now be flagged by the compiler.
In
`@apps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsx`:
- Line 404: Replace the title-based query in the test that uses
screen.queryByTitle("Open terminal") with a role-based query to match other
tests: use screen.queryByRole("button", { name: "Open terminal" }) and keep the
same assertion (toBeNull()) so the test asserts absence via role/name instead of
title; update the test referencing the query in
AgentChatPane.companionDrawers.test (the line with screen.queryByTitle("Open
terminal")) to this new role-based query for consistency with other tests that
use getByRole("button", { name: "..." }).
🪄 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: f7da1c08-904f-414c-82e3-687e751be896
⛔ Files ignored due to path filters (12)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/browser-mock.mdis excluded by!docs/**docs/codex migration plan.mdis excluded by!docs/**docs/codex-cli-passthrough-audit.mdis excluded by!docs/**docs/features/ade-code/README.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/app-control.mdis excluded by!docs/**docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**docs/plans/ade-31-claude-agent-sdk-rewrite.mdis excluded by!docs/**docs/transcript.txtis excluded by!docs/**
📒 Files selected for processing (41)
apps/ade-cli/scripts/verify-built-cli.mjsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/HeaderFooter.test.tsxapps/ade-cli/src/tuiClient/__tests__/TerminalPane.test.tsxapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/cli.tsxapps/ade-cli/src/tuiClient/components/FooterControls.tsxapps/ade-cli/src/tuiClient/components/TerminalPane.tsxapps/ade-cli/tsup.config.tsapps/desktop/scripts/validate-mac-artifacts.mjsapps/desktop/scripts/validate-win-artifacts.mjsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ipc/ipcTimeouts.test.tsapps/desktop/src/main/services/ipc/ipcTimeouts.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/prs/prService.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/chat/AgentChatPane.companionDrawers.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/lanes/LanesPage.test.tsapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/lanes/lanePageModel.tsapps/desktop/src/renderer/components/prs/PRsPage.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.issueResolver.test.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/prs/prsRouteState.test.tsapps/desktop/src/renderer/components/prs/prsRouteState.tsapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.test.tsxapps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsxapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PRs/PrHelpers.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swiftapps/ios/ADETests/ADETests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 14, 2026 23:45
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
May 15, 2026 00:11
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\n- tighten PR snapshot routing and progressive PR detail hydration\n- add lane delete cleanup parity across desktop, sync, iOS, and runtime actions\n- improve ADE Code terminal control packaging and validation\n- update internal docs and prune obsolete notes\n\n## Validation\n- npm install in apps/desktop, apps/ade-cli, apps/web (no lockfile drift)\n- npm run typecheck in apps/desktop, apps/ade-cli, apps/web\n- npm --prefix apps/desktop run lint\n- desktop vitest shards 1/8 through 8/8; shard 8 had one flaky failure, rerun of src/main/services/chat/cursorModelsDiscovery.test.ts passed\n- npm test in apps/ade-cli\n- npm run build in apps/desktop, apps/ade-cli, apps/web\n- node scripts/validate-docs.mjs and internal docs link/source-map checks\n- xcrun swiftc -parse for changed iOS app/test Swift files
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
This PR tightens PR detail hydration to prevent stale snapshot data from overwriting live responses, extends lane deletion to clean up associated environments and port leases across desktop, sync, iOS, and runtime action paths, and adds a Ctrl+T toggle for Claude terminal control in the TUI with animated border feedback.
PrsContext.tsxnow gatesmarkPrimarySettledbehindif (cancelled) return;in.finally(), resolving the stale-closure race from the previous review.PrDetailPane.tsxtracks per-PR live-load refs so snapshot hydration can't overwrite already-delivered live data on a back-navigation.runLaneDeleteBatchSequentiallyreplacesPromise.allSettledto carry lane identity in failure results;deleteLaneWithRuntimeCleanupand the registrydeleteaction both callteardownEnv+portAllocationService.releasebefore/after the core delete, with test coverage across desktop, sync, and iOS.isTerminalControlToggle/splitTerminalControlInputadded toapp.tsx;TerminalPanerenders a spinning-color rounded border and adjusted dimensions when Claude has control.FooterControlssurfaces^thints contextually.Confidence Score: 5/5
Safe to merge. All three feature areas are well-tested and the stale-closure fix in PrsContext is correctly guarded.
The stale
.finally()closure bug from the previous review is addressed by theif (cancelled) return;gate. Lane delete cleanup is consistent across all four code paths. The only finding is a leftoverincludeExternalClosed: booleanfield in theinFlightSnapshotReftype — purely cosmetic.apps/desktop/src/renderer/components/prs/tabs/GitHubTab.tsx — stale ref type annotation should be cleaned up.
Important Files Changed
if (cancelled) return;guard in.finally()to fix the stale-closure race flagged in the previous review.allItemsand removes cross-repo fallback. StaleinFlightSnapshotReftype still referencesincludeExternalClosed.runLaneDeleteBatchSequentiallyandselectTerminalGithubUpdateForPr. Logic and tests are sound.includeExternalClosed/ external-PR query path entirely. Simplifies cache epoch tracking withgithubSnapshotCacheEpoch.deleteto the lane runtime action domain with env teardown and port release. UsesresolveLanewhich already passesincludeArchived: true.callPrReadRuntimeActionOrhelper andcreateLocalIpcEventSubscriptionfactory for shared-listener multiplexing.splitTerminalControlInputandclaudeTerminalControlActiveresize adjustments.commandTimeoutNanoseconds(for:)givinglanes.deletea 240-second budget.Comments Outside Diff (1)
apps/desktop/src/renderer/components/prs/state/PrsContext.tsx, line 1136-1215 (link)markPrimarySettledfires even for cancelled requests.finally(() => markPrimarySettled(fulfilled))always executes — it is not gated bycancelled. On a rapid A→B→A PR selection (i.e. user opens PR A, switches to B, then back to A), the four requests from the first A fetch are still in-flight withcancelled = true. When they settle, their.then()returns early (sofulfilledstaysfalse), but.finally()still callsmarkPrimarySettled(false). BecauseselectedPrIdRef.current === prIdis true again (user returned to A), those stale completions drive two side effects that corrupt the new A fetch's state:primarySettledCount === 1),detailFetchInProgress.currentis prematurely reset tofalsefor the new fetch.primarySettledCount === 4),setDetailLiveDataPrId(null)is called (becauseprimaryFulfilledCount === 0), overriding whatever the new A fetch has already written.The old
Promise.allSettledcode was immune because the entire.then()block was guarded byif (cancelled) return. The fix is a one-linecancelledguard in.finally().Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: guard stale PR detail settlements" | Re-trigger Greptile