Skip to content

perf(terminal): bound stalled setup-split grid polling - #17228

Merged
nwparker merged 1 commit into
mainfrom
nwparker/perf-terminal-startup-readiness-cap
Aug 29, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/perf-terminal-startup-readiness-cap

Conversation

@nwparker

@nwparker nwparker commented Aug 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​24 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​24
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​15 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​15

ELI5

When Orca opens a terminal beside a setup-command split, it waits for the new split's geometry before starting the terminal. If that geometry gate never became ready, Orca kept asking Chromium to measure the pane every rendered frame forever. The terminal never connected, and the layout-reading loop could keep burning renderer CPU.

This gives that gate a generous but finite budget. If it is still closed after 120 rendered frames, Orca connects with the latest usable grid and lets the existing post-spawn size reconciliation handle any later resize.

What changed

  • Add a separate 120-frame readiness-wait cap, ten times the normal 12-frame settle window.
  • Retain the latest usable measurement while the external gate is closed and settle with it at the cap.
  • Keep ungated startup and the existing ready-frame stability/minimum/maximum behavior unchanged.

Measurement proof

The regression uses a deterministic frame scheduler, so it counts layout work instead of relying on machine timing.

  • Before (recorded red run): with a five-frame test budget, the old loop consumed all 100 simulated frames, left another frame queued, and never called the settle callback (expected 5, received 100).
  • After: the production-default test waits through frame 119, settles exactly on frame 120, performs exactly 120 measurements, and leaves zero queued work.
  • Complexity: a permanently closed gate changes from unbounded layout reads over pane lifetime to at most 120 reads for that startup.

This is intentionally a rendered-frame work bound, not a wall-clock promise. Chromium may throttle or pause requestAnimationFrame while the window is hidden; that hidden state also does not sustain the foreground layout-reading loop this fixes.

Regression proof

  • pnpm test across 7 related setup-split/startup files: 88/88 passed.
  • Final focused rerun: startup-grid settle + setup-split spawn, 16/16 passed.
  • pnpm tc:web: passed.
  • pnpm run check:code-quality:changed: zero code-quality, type-aware, or React Doctor findings across both files.
  • pnpm exec oxfmt --check ...: passed for both changed files.
  • Independent read-only review found no correctness regression in cap, cancellation, readiness-toggle, or caller semantics.

The pre-existing healthy-gate test still holds through eight pre-ready frames and then settles on the correct post-split 88x50 grid. The new test locks the permanently closed gate's exact default boundary.

Electron validation

Validated in this branch's own Electron dev instance over CDP:

  • queued the real startup-command + vertical setup-split flow;
  • visibly observed MAIN_STARTUP_READY in the primary pane and SETUP_SPLIT_READY in the setup pane;
  • confirmed two distinct PTY IDs mapped to the two split leaves;
  • observed zero console errors.

No visual artifact is attached because this changes failure-path scheduling, not styling or layout.

No-user-regression signoff

  • No feature is disabled, reduced, or made opt-in.
  • Ungated terminal startups never consult the new cap.
  • Healthy setup-split gates that open within 120 rendered frames keep the exact prior wait/stability behavior.
  • Only a gate that stays closed for the full budget falls back; the caller immediately performs its normal safeFit, and existing post-spawn reconciliation remains authoritative for later geometry changes.
  • No IPC/RPC, remote-wire, Git, filesystem-path, keyboard, provider, SSH execution-ownership, or folder-workspace contract changes.

Merge risk

Low. Two files, one internal optional limit, and one deterministic regression test. The residual risk is a genuinely healthy setup split needing more than 120 rendered frames; the 10x readiness window plus the existing post-spawn reconcile limit the consequence to connecting with the latest usable grid instead of waiting indefinitely.

Addresses root cause #2 in #15241 without closing the broader issue.

AI disclosure

Developed and reviewed with OpenAI Codex assistance.

@greptile-apps

greptile-apps Bot commented Aug 29, 2026

Copy link
Copy Markdown

Greptile Summary

This PR bounds setup-split startup-grid polling at 120 rendered frames, retaining the latest usable measurement so a permanently closed readiness gate no longer schedules layout reads indefinitely.

  • Adds an optional readiness-wait budget separate from the normal grid-stability window.
  • Settles at the budget boundary while preserving existing behavior for ungated and promptly ready panes.
  • Adds deterministic coverage for the exact default boundary, callback, measurement count, and pending-frame cleanup.

Confidence Score: 5/5

The PR appears safe to merge because the new polling bound preserves lifecycle guards and existing geometry reconciliation paths.

No concrete blocking or independently actionable non-blocking defect remains after tracing the capped readiness path through connection-time fitting, cancellation, disposal checks, and post-spawn resizing.

Important Files Changed

Filename Overview
src/renderer/src/components/terminal-pane/terminal-startup-grid-settle.ts Adds a bounded readiness-gate polling path while preserving cancellation, liveness, ordinary settling, and downstream resize reconciliation behavior.
src/renderer/src/components/terminal-pane/terminal-startup-grid-settle.test.ts Verifies that a permanently closed gate settles exactly on frame 120 with no remaining scheduled work.

Reviews (1): Last reviewed commit: "perf(terminal): bound stalled setup-spli..." | Re-trigger Greptile

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b2a89fe5-943a-4672-a308-cedea2f2db05

📥 Commits

Reviewing files that changed from the base of the PR and between a1f198b and 35e6506.

📒 Files selected for processing (2)
  • src/renderer/src/components/terminal-pane/terminal-startup-grid-settle.test.ts
  • src/renderer/src/components/terminal-pane/terminal-startup-grid-settle.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The startup grid settling logic now accepts maxReadinessWaitFrames and defaults it to ten times DEFAULT_MAX_FRAMES. It stores the latest usable dimensions while readiness remains false and settles at the frame cap instead of polling indefinitely. A test verifies the default 120-frame behavior and the measured dimensions returned on settlement.

Merge Risk: ⚪ Minimal · up to 35e65

The change prevents stalled setup-split terminal startup from polling indefinitely while preserving normal startup and resize behavior. No actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: bounding stalled setup-split grid polling.
Description check ✅ Passed The description is detailed and on-topic. It explains the problem, solution, scope, testing, validation, AI use, and regression risk. It references issue #15241 and explains why visual proof is not ap…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description is detailed and on-topic. It explains the problem, solution, scope, testing, validation, AI use, and regression risk. It references issue #15241 and explains why visual proof is not applicable, although some template headings and checklist items are not reproduced exactly.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nwparker
nwparker merged commit 5e3c4f3 into main Aug 29, 2026
27 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant