Skip to content

test(terminal): pin layout invariants the mirror refactor must keep - #26073

Merged
Jinwoo-H merged 2 commits into
mainfrom
test/terminal-topology-b2-gates
Oct 7, 2026
Merged

Jinwoo-H merged 2 commits into
mainfrom
test/terminal-topology-b2-gates

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Oct 7, 2026 •

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

ELI5

We're about to change who's in charge of terminal layout: main will own it and the window will only show it. This PR adds tests for the rules that change has to keep, and one test for the bug it has to fix, so the change is checked against a bar set beforehand. No product code changes.

What Changed

Test-only. Five new files:

Must pass today and after the change

  • terminal-topology-model.test.ts (main): a seeded, repeatable random test. Each seed makes 40 changes through main's real Store on SQLite, the way the desktop window makes them today: new tab, split, close pane, close tab, drag a pane out to its own tab, a terminal exits, a pane respawns, an agent sleeps or wakes, restart, and quit-then-restart. It covers a git worktree, a folder workspace and the floating terminal. After every step, and again after a cold reload, it checks that:

    • no terminal is bound to two panes, and no pane is in two tabs;
    • main's projected layout matches a simple reference model: tabs, panes, which terminal each pane holds (an exited pane keeps its binding), and sleeping records.

    A failure prints the seed and its list of steps. 6 seeds take about 1.5 s. 300 seeds also pass on main.

  • terminal-sleep-quit-resume.test.ts (main): an agent is slept, then the app quits through the before-unload path and reopens. The resume record is still there, it's the newer quit capture rather than the earlier periodic one, it shows up in main's projected layout, it survives the relaunched window's first save, and it still builds a codex resume <id> command. A second case covers a record that only the before-unload save carried.

  • terminal-exited-pane-liveness.test.ts (window): Ctrl-D in one pane of a split takes only that terminal out of the live list. The pane's saved binding stays. When both panes exit, the tab reads as not running.

Expected to fail until the mirror PR fixes STA-9417

  • setup-split-activation-single-owner.test.ts (window), ported from the Stage A branch. A host-created worktree runs its setup script in a split of a tab that isn't mounted yet, and then gets its first activation.

    • "main reveals the split": passes.
    • "the split reveal never arrives": it.fails. Today the activation sweep creates a second tab for the setup terminal.
    • 8 create-then-activate cycles: it.fails (4 duplicates today).

    B2-4, in the mirror PR, flips both by removing .fails. In the no-reveal case, that PR delivers main's split before the sweep runs.

Shared setup lives in terminal-topology-profile-fixture.ts.

Why

The mirror PR replaces how the window writes layout. Without a bar fixed in advance, it would write its own tests to match its own behavior. Writing these against today's main shows which behavior is current (it passes now) and which is the named fix (expected to fail now). Store-level tests run in the PR unit gate. End-to-end tests don't block merges.

Not included, and why:

  • Tests of the mirror's own code (identical update keeps the same object, no save echo, boot order, pending panes, SSH no-export-loop, local split geometry, placement checks for floating and folder workspaces). They need APIs the mirror PR adds, so that PR writes them.
  • The model test's window-mirror checks (the mirror matches main, mounted panes match the mirror). The mirror PR adds those to this harness.
  • ssh: workspaces in the model test.
  • Follow-ups: the latency budget harness and the nightly end-to-end rows.

Linked Issue

STA-9417 (expected-failure test only; not fixed here).

Visual Proof

N/A: test-only, with no UI or behavior change.

Testing

  • pnpm tc passes.

  • pnpm run check:code-quality:changed passes.

  • New tests ran 3 times: 6 passed and 2 expected-fail each time.

  • 300 model seeds pass.

  • Checked that the tests can fail:

    • a forced duplicate binding trips the owner check;
    • skipping the main-side close trips the model;
    • dropping the before-unload stage loses the record;
    • removing .fails shows the STA-9417 duplicate tab.
  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Test-only. Paths go through path.join, and the resume command is built for a pinned platform.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 58950e75-ec56-436f-bc8f-c36b2c571261
📥 Commits

Reviewing files that changed from the base of the PR and between 0ff240a and 1ea751b.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 68adba48-8930-4a9d-aa6e-c5b33800729c
📥 Commits

Reviewing files that changed from the base of the PR and between 7f83e0a and 0ff240a.

📒 Files selected for processing (5)
  • src/main/persistence/terminal-topology/terminal-sleep-quit-resume.test.ts
  • src/main/persistence/terminal-topology/terminal-topology-model.test.ts
  • src/main/persistence/terminal-topology/terminal-topology-profile-fixture.ts
  • src/renderer/src/lib/setup-split-activation-single-owner.test.ts
  • src/renderer/src/store/terminals/terminal-exited-pane-liveness.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The pull request adds tests for terminal topology persistence, seeded topology operations, setup-split ownership during first activation, and exited-pane liveness. It also adds helpers to seed and reopen topology stores for persistence tests.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 0ff24

This change adds terminal-layout tests and makes no product-code changes, so it does not change application behavior. Two tests are deliberately marked as expected failures to record a known duplicate-tab bug that the planned refactor will fix. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 5 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 summarizes the main change: tests that preserve terminal layout invariants during a planned mirror refactor.
Description check ✅ Passed The description covers the required sections and explains the scope, rationale, tests, expected failures, and lack of UI changes. The linked issue is identified as STA-9417, but the template asks for …
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.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving 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.

❤️ Share

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

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