Repository navigation
fix(terminal): retry first-terminal seeding until a decision applies - #22919
Conversation
The seeding effect marked a workspace as attempted before its async activation check finished, so a StrictMode double run, an input change mid-check, or a 'blocked' result left the workspace marked with no terminal. Mark it only once an uncancelled, unblocked result is applied; a rerun shares the gate's in-flight check.
![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
- Retryable first-terminal seed — in the startup activation gate effect, the "already handled" mark is now added inside the gate's result callback instead of before the await, and only when the run wasn't cancelled, the outcome isn't
'blocked', and the workspace is still active. A cancelled run, a'blocked'answer, or a mid-check workspace switch now leaves the worktree unmarked so the next effect run retries;adopted/resumed/structuredstill mark once, and'empty'seeds exactly once. - Regression tests — three new tests mount the real effect and assert
createTabcall counts for the StrictMode double run, an input change mid-check, and a'blocked'answer followed by leaving and returning.
I read the full diff, traced the gate outcome vocabulary in worktree-agent-activation-gate.ts ('blocked' | 'adopted' | 'structured' | 'resumed' | 'empty'), confirmed the in-flight dedupe in gateWorktreeAgentActivation makes a retry cheap, and verified the source-text assertions in app-startup-routing.test.ts still hold. Ran pnpm test on both touched test files: 41 passed.
One pre-existing note, not introduced here and not blocking: this effect's .then has no rejection handler, so a rejected gate never reaches the callback (which is why it correctly leaves no mark) but surfaces as an unhandled rejection. The sibling resume effect handles rejection explicitly. Out of scope for this fix.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe startup activation pass now leaves cancelled, blocked, and stale worktree checks eligible for retry. It marks a worktree processed after an accepted gate result, and only an Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The terminal-seeding change is mergeable after normal checks; no unresolved issue requiring a fix was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is limited to terminal startup behavior. The reviewed path still waits for host authority, checks for existing terminal state, and permits only one default terminal after an accepted decision. No new security concern was established, though coverage is incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the user impact, mechanism, rationale, testing, visual proof, and compatibility considerations. However, the required Linked Issue section explicitly states “No ticket,” and the template requires an issue link.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
…on applies The seeding callback runs after an async activation check, so a row captured at render time can be stale by then. Read it from the store at decision time and drop it from the effect dependencies, which no longer need to restart the check when the row appears. Add tests for two separate empty checks and for a last terminal closed while the check runs.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since the prior pullfrog review (553f8a0 → 3fe9970):
- Decision-time tombstone read — the startup activation gate now derives the closed-last-terminal tombstone inside the gate callback (
Object.hasOwn(useAppStore.getState().tabsByWorktree, activeWorktreeId), afterreconcileWorktreeTabModel) instead of from a render-captured projection, so a row that appears or disappears while the check runs is seen. The raw store read agrees with the projected controller prop on key presence becausecreateWorktreeTabBucketProjectionpreserves every source key, andreconcileWorktreeTabModelnever creates atabsByWorktreerow from nothing. - Dropped the now-unnecessary dependency —
activeWorktreeHasTerminalStateleft the effect's dependency list, since the value is read at decision time rather than captured at render. - New regression coverage — a workspace whose last terminal closes during the in-flight check is not seeded, and leaving/returning across two empty checks seeds exactly once.
- Updated source-text wiring test — pins the live-store
Object.hasOwnread; the old assertion that the tombstone derivation stayed in the effect deps was removed to match the intentional dep removal.
I read the full diff and the range-diff, traced the gate outcome vocabulary, and checked the projected-vs-raw tabsByWorktree equivalence plus the reconcile/orphan-cleanup side effects. Ran pnpm test on the three touched files (45 passed) and the full components/terminal/ directory with terminal-parked-watcher-sync-entries.test.tsx (259 passed); app-startup-routing.test.ts:434 still holds.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: readyThe head, Review loops
Electron QA (main's version of the effect vs this PR; isolated profiles; relaunch per trial; before/after screenshots are in the description)
Performance
Not caused by this PR (noted for follow-up)
|
Ready to mergeChecked at head
|
ELI5
When you open a workspace that has never had a terminal, Orca is supposed to create one for you. It first runs a short background check for agents or sessions it should restore. The code wrote down "done" for that workspace before the check finished, so if the check was interrupted or came back "not yet", no terminal was created and nothing ever tried again. Now it writes down "done" only after it has acted on the check's answer.
What Changed
Before: the workspace could be left with no terminal for the rest of the session. It stayed empty until you opened it from the sidebar or the worktree palette, which create terminals through a different path. What users can hit:
pnpm deva never-visited workspace that is active at startup, or the first one you open, always stayed blank.'blocked'answer (for example, a live agent process whose recovery isn't settled yet) marked the workspace done without creating anything. I could not produce'blocked'for a never-visited local workspace, so this path is covered by unit tests only.After: in each of those cases the next run of the effect checks again, and the workspace gets its terminal (4 of 4 production trials, and every development-build trial). Once the effect has acted on an answer, it still never checks that workspace again, so it cannot create a second terminal.
Mechanism (
use-terminal-watcher-effects.ts, the startup seeding effect):startupActivationGateWorktreeIdsRef) used to be added beforeawait gateWorktreeAgentActivation(...). It is now added inside the result callback, and only when the run wasn't cancelled, the answer wasn't'blocked', and the workspace is still the active one.'blocked'answer, a rejected check, or a workspace switch mid-check leaves no mark, so the next run tries again.gateWorktreeAgentActivationalready hands every caller the same in-flight promise for a workspace.shouldAutoCreateInitialTerminal), including "an explicit empty row means the user closed the last terminal", is untouched.Why
The bug was a mark that nothing ever cleared. Clearing it on each failure path would add a delete in the cleanup, one on
'blocked'and one on rejection. Setting it only once an answer has been applied means there is nothing to clear, so this approach uses the fewest moving parts. It adds no new state: the existing in-flight dedupe in the activation gate is what makes a retry cheap.Alternatives considered:
'blocked'. Same behavior, more code paths, and a late cleanup could race a run that is already applying.ensureWorktreeHasInitialTerminal). Worthwhile, but a larger change than this bug needs, and the helper's closed-last-terminal handling differs (explicit activation re-seeds). Left for separate work.Differences from the common pattern
'blocked'or rejection. This effect marks after instead. The difference comes from cancellation: the resume effect has no cancelled path, so its in-flight run always reaches its callback. This effect cancels on cleanup, so an eager mark would have to be cleared in three places. The retry trigger is the same as the resume effect's: the next change to the effect's inputs, such as leaving and returning to the workspace.Linked Issue
No ticket. Found during QA of the chat-send work, where the first activation on a fresh profile showed no terminal.
Visual Proof
Production build. Workspace A was selected, the app switched to B while A's check was still running, then selected A again without the sidebar:
Testing
New tests in
initial-terminal-structured-launch.test.tsxmount the effect and assert whether a terminal is created:'blocked', then leave and return: a terminal is created on return.'empty'checks, separated by leaving the workspace: exactly one terminal.Each test was checked against the code without the fix:
main: all three fail with "created 0 times".'blocked'exclusion: the blocked test fails with "created 0 times".Other checks:
src/renderer/src/components/terminal/,terminal-parked-watcher-sync-entries.test.tsxandapp-startup-routing.test.tspass: 29 files, 287 tests.pnpm tc:web,oxlinton the changed files,check:code-quality:changedandcheck:react-doctor:changedall pass.Manual Electron checks, comparing main's version of the effect with this PR. Each check used an isolated profile and relaunched the app before every trial:
build:electron-vite, production React):'empty'; none answered'blocked'.Platforms: macOS. The change is platform-independent renderer logic.
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
activeWorktreeHostAuthority === 'none'). A retry after an interrupted check goes through that same gate, so it cannot seed into an SSH workspace whose host hasn't answered.'blocked'answer, the check runs again only when the effect's inputs change, the same cost the resume effect already pays.Author X: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)