Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change adds Merge Risk: 🟡 Moderate · up to Workspace recovery can restore a terminal after a newer activation intentionally chose a non-terminal surface, producing an unexpected workspace layout. This ordering issue should be resolved before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the problem, implementation, tradeoffs, visual proof, testing, platform gaps, and review guidance. However, it omits the required Linked Issue section and issue reference, and it does not include the required template checklist sections or Agent skill upstream boundary checkbox. Resolution Add a Linked Issue section with a valid issue reference, complete the Agent skill upstream boundary checkbox, and include or explicitly address the required checklist items. Confirm the AI Disclosure and Review sections according to repository policy.
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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 |
![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
🧹 Nitpick comments (1)
src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.ts (1)
45-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the test exercises the asynchronous path.
The final tab count alone does not prove that the gate returned
emptyor that reseeding occurred in the continuation. Assert the pre-gate tab count and the gate outcome.Proposed test strengthening
activateAndRevealWorktree(worktree.id, { notifyHostRuntime: false }) - await waitForWorktreeAgentActivationGateForTests(worktree.id) + expect(useAppStore.getState().tabsByWorktree[worktree.id]).toHaveLength(0) + const outcome = await waitForWorktreeAgentActivationGateForTests(worktree.id) + expect(outcome).toBe('empty') expect(useAppStore.getState().tabsByWorktree[worktree.id]).toHaveLength(1)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: da25d922-28d3-498a-a98c-1fcc76416577
📒 Files selected for processing (4)
src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.tssrc/renderer/src/lib/worktree-activation.tssrc/renderer/src/lib/worktree-initial-terminal-seeding.tstests/e2e/completed-worker-retirement-resume.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { reseedEmptiedWorkspace: true } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Revalidate the activation intent before reseeding.
This delayed callback uses the earlier activation's options. If a second activation sets providesInitialSurface: true before the gate resolves, activeWorktreeId remains the same and this call still creates a terminal. Track the latest activation generation, or revalidate the current activation intent before reseeding.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Async-gate reseed —
activateAndRevealWorktree'soutcome === 'empty'continuation now passes{ reseedEmptiedWorkspace: true }toensureWorktreeHasInitialTerminal, matching the explicit-activation path (worktree-activation.ts:281) and the folder path, so a worktree whose last terminal was closed re-seeds a fallback terminal once the async agent inventory confirms it is empty. - Folder helper relocation —
ensureFolderWorkspaceInitialTerminalmoves verbatim fromworktree-activation.tsintoworktree-initial-terminal-seeding.tsto keep the activation module under its line limit; theFolderWorkspace/folderWorkspaceKeyimports move with it, and both call sites inactivateAndRevealFolderWorkspaceare unchanged in behavior. - Regression test — a new focused test drives the async inventory gate against an emptied worktree with a closed-last-terminal tombstone and asserts a re-seed. I traced the without-fix path: the tombstone is honored (
shouldHonourClosedTerminalTombstone), so no terminal is created and the assertion fails — the test is real coverage, not theatre. - E2E expectation correction — the worker-retirement spec now expects
state: 'done', matching the explicitdoneevent its setup writes last, which is what the persisted recovery record holds.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Seam census updated — added
worktree-initial-terminal-seeding.tsto theSEAM_FILEScensus inworktree-activation-surface-caller-wiring.test.ts, so the ratchet that requires everyprovidesInitialSurfacemention undersrc/to be listed stays green now thatensureFolderWorkspaceInitialTerminallives in that module. Placement inSEAM_FILES(notSURFACE_PROVIDING_CALLERS) is correct: the file declares and forwards the seam viareseedEmptiedWorkspace: providesInitialSurface !== truerather than opting out of tombstone re-seeding. Verified against the source tree that every production mention is now accounted for.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…ding No behavior change; the moved helper returned a local it never used.
|
#18886 — yes, close it Unambiguously redundant. Main already has reseedGatedEmptyWorkspace (commit 06a607a, #16904) wired into both gate continuations, and the e2e hunk verbatim. My PR fixes a bug that's already fixed. I'd salvage one thing: my new test drives the real runWorktreeAgentActivationGate with a stubbed window.api, whereas main's equivalent mocks the gate — strictly better coverage of the same behavior. Land that as a small test-only PR, or just close #18886 outright if you'd rather not spend another review cycle. Either is defensible; the fix itself is dead either way. Worth naming the process failure: I reviewed #18886 in depth and recommended keeping it, without ever checking its base against current main. It was 227 commits stale, and the fix had landed underneath me. My earlier "keep it, this is a real inconsistency" was confidently wrong for that reason. The same staleness check would have saved most of the work on it — and it's the same class of gap as the missing post-merge gate on main: nothing in the loop compares a branch against reality until CI complains. |
ELI5
Orca lets you close the last terminal in a workspace, and it remembers you did that on purpose so it doesn't keep popping a new one back at you. But when you later click that workspace open again, you're asking for it back — so you should get a fresh terminal.
That only worked some of the time. When Orca first had to check in the background whether any agent sessions were still running there, the check came back "nothing running" and then Orca forgot the last step: actually creating the terminal. You clicked the workspace and got a blank screen with nothing to type into, and had to add a terminal by hand.
The background check now finishes the job the same way the normal path already did, so opening an emptied workspace gives you a usable terminal either way.
Opening a workspace after its last terminal is retired can leave a blank workspace. The asynchronous agent-inventory check correctly reports no sessions, but its continuation omits
reseedEmptiedWorkspace, so the closed-tab tombstone prevents the fallback terminal from being created.Pass the same reseeding option used by normal explicit activation after the inventory gate reports empty. Move the existing folder seeding helper into the terminal-seeding module to keep the activation module within its line limit; folder behavior is unchanged. Correct the worker-retirement E2E setup expectation to
done, matching its explicit done event and the durable recovery contract.Validation: the new asynchronous-inventory regression fails before the fix; 27 focused unit tests pass afterward, including folder and remote activation cases. Targeted lint and
pnpm tc:webpass. The existing worker-retirement E2E reproduced the blank workspace before the fix. A separate e2e build now passes both retirement modes; two further runs of both modes plus the whole-tab close/restart scenario passed (8 rendered cases total).Application behavior change: left open for user review; do not merge automatically.
Reliability:
agent-session.provider-ownershipcovers the invariant that explicit activation may seed an emptied workspace only after session inventory resolves empty; existing sessions still retain ownership. The new unit oracle controls the asynchronous inventory and asserts terminal creation, while the rendered oracle closes/retires terminals and reopens the workspace. No new polling, provider scans, timers, or subprocesses were added. macOS local/daemon behavior is covered by the eight rendered cases; folder and remote activation branches have focused unit coverage. Rendered Linux, Windows, SSH, WSL, and mobile/relay validation remain gaps. Playwright traces and the asynchronous inventory regression provide failure diagnostics.Tradeoffs
These are the genuine user-facing costs of making the gated path re-seed:
pendingActivationSpawn, so a PTY is spawned. On an SSH or relay host that is a remote shell process on the server, not just a local one. In the gated path you previously got nothing and paid nothing.providesInitialSurface.renderableTabCountand bails if any surface appeared meanwhile, and bails entirely when the execution host owns terminal creation (hostAuthority === 'live'), which covers the live-remote case.gateWorktreeAgentActivation(...).then(...)still has no.catch()(pre-existing on both call sites), and the continuation now does more work — tab creation plus store writes — inside it. A throw there becomes an unhandled rejection instead of a no-op.reseedEmptiedWorkspace: trueon one call. The blast radius is every non-providesInitialSurfaceactivation that hits the async agent-inventory gate, which is the common sidebar/palette open path — high traffic, so a mistake here would be widely felt even though the change is one line.Not affected: no wire/RPC/IPC payload, persisted schema, or platform-specific code changes; folder-workspace behavior is unchanged (that path already passed the flag).
Visual Proof
N/A — no rendered UI surface changes. The fix restores an existing code path
(creating the workspace's fallback terminal); it introduces no new component,
layout, styling, or copy. Local Electron launches are paused for this task per
AGENTS.md, so rendered validation is deferred to CI, where the existing
completed-worker-retirement-resumee2e spec covers the reopen-after-retirementflow that originally exposed the blank workspace.
Note: why the e2e expectation moves to
donetests/e2e/completed-worker-retirement-resume.spec.tsdrivessetAgentStatustwice on the same pane key —
working, thendone— and asserted the recoveryrecord stayed
working. That was correct until #16308 ("stop ghost resume tabsafter finished turns",
c60d2ba8958) addedexisting.state === next.statetorecoveryRecordMatches(
src/renderer/src/store/slices/agent-status-recovery-equivalence.ts), so thedoneupdate now correctly replaces theworkingcheckpoint.That same commit already flipped this expectation in the sibling unit test
tests/e2e/completed-worker-retirement-resume.unit.test.ts, but missed thePlaywright spec's copy — this spec only runs when it changes, so the stale
expectation went unnoticed. Recording
doneis the desired behavior: it is whatmarks passive completed hibernation evidence and suppresses auto-resume, which is
exactly what this spec's later assertions (
resumeClaimCount: 0, no--resume)require. Nothing on this branch touches the
agent-status-*slices.