Skip to content

fix(ci): repair terminal link handling and E2E recovery fixtures - #23677

Merged
nwparker merged 4 commits into
mainfrom
nwparker/fix-ci-e2e-09-28
Sep 28, 2026
Merged

nwparker merged 4 commits into
mainfrom
nwparker/fix-ci-e2e-09-28

Conversation

@nwparker

@nwparker nwparker commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​66 $\color{#cf222e}{\Huge{\mathbf{−}}}$​21 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​45
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 0

ELI5

Fix three failures from today's E2E runs: a disabled terminal link menu still opened, the browser restart test could inspect a temporary empty state during recovery, and the daemon-close fixture lacked the profile store now required for workspace lookup.

What Changed

Plain clicks now honor the disabled setting even when middle-click actions remain enabled. The shared link handler rejects the click before claiming terminal mouse input; the existing E2E checks that the child receives it. A focused unit test covers that combination.

The restart test now waits for the whole recovery condition: exactly one tab with its original identity and client placement. It still checks the rendered destination and checks the count again afterward.

The daemon-close test client now uses the existing SQLite profile-store factory and closes that store during cleanup. Its close-intent and process-survival assertions are unchanged.

Why

The terminal regression came from keeping a link-action context alive for middle clicks without checking the plain-click setting. Fixing the handler preserves both preferences independently.

The restart trace shows a successful presence read followed by an empty read; its failure screenshot already shows the recovered tab at the right destination. Waiting for the complete contract preserves the assertions while avoiding a race between separate observations.

The prerequisite unit-test fix #23675 is now merged.

Linked Issue

Observed failures: https://github.com/stablyai/orca/actions/runs/36461449615 and https://github.com/stablyai/orca/actions/runs/36467205524.

Visual Proof

Disabled link action, before:

Before

After the same click with the fix:

After

Testing

  • Manually inspected before/after hidden-renderer screenshots on macOS.
  • Added a regression test: all 16 link-routing unit tests pass.
  • Terminal E2E reproduced before; all four cases pass after a fresh build.
  • Updated browser restart E2E passes three repetitions. The original passed 11 local runs; the transient failure was observed in Linux CI.
  • Daemon-close E2E reproduced folder_workspace_not_found before and passes with the fixture store.
  • Changed-code quality gate passes. Full platform checks run in CI.

Review

Exact restart identity/count checks remain, and disabled clicks do not claim or cancel child terminal input.

Agent skill upstream boundary

  • Not applicable.

Notes

No wire, Git provider, filesystem-path, or SSH execution changes. The shared renderer handler applies to local and remote terminals and folder workspaces.

Checklist

  • Focused fixes for observed failures
  • Explained behavior, mechanism, and choice
  • Before/after screenshots attached
  • Self-reviewed
  • Cross-platform and remote impact considered
  • Full CI validation

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 6bafbec6-261c-4270-a527-8cec6fba8676

📥 Commits

Reviewing files that changed from the base of the PR and between 0034ede and 167d3e5.

📒 Files selected for processing (8)
  • src/main/claude/claude-structured-control-actions.test.ts
  • src/main/claude/claude-structured-queued-stop.test.ts
  • src/main/claude/claude-structured-real-cli.test.ts
  • src/renderer/src/components/terminal-pane/terminal-link-action-request.ts
  • src/renderer/src/components/terminal-pane/terminal-link-action-routing.test.ts
  • tests/e2e/fixtures/daemon-generation-legacy-close-client.ts
  • tests/e2e/fixtures/daemon-generation-profile-store.ts
  • tests/e2e/paired-client-hosted-browser-restart-survival.spec.ts

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


📝 Walkthrough

Walkthrough

The changes update three cancellation settlement expectations with structured rejection data. They also add a guard and test for disabled plain-click link actions. The end-to-end test fixtures now create and clean up a profile store, and the restart-survival test retries the recovered-tab checks and verifies the row count after rendering.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 167d3

No actionable merge-blocking risk remains. The changes are mergeable after normal checks.

🚥 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 3 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title is concise and accurately summarizes the main changes: terminal link handling fixes and E2E recovery fixture repairs.
Description check ✅ Passed The description is complete and relevant. It explains the user impact, implementation, reasons, visual proof, testing results, scope, and remaining CI status. The Linked Issue section lists CI runs an…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

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 changed the title fix(ci): respect disabled terminal links and await browser recovery fix(ci): repair terminal link handling and E2E recovery fixtures Sep 28, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the branch's net diff against main (the generated diff also carried two tests/e2e/fixtures/daemon-generation-* entries that are absent from the branch head; those are a stale-base artifact, not part of this PR).

  • Disabled plain-click gate — requestTerminalLinkAction now requires context.plainClickBehavior !== 'none' for a plain-click activation, so a disabled plain click returns false before claimPtyMouse, preventDefault, or request instead of opening the action popover. Middle-click behavior stays independently gated; isTerminalLinkActionActivation is button-0 only, so the added clause cannot suppress middle click, and production always supplies a resolved behavior (terminal-pane-mount-preparation.ts:134) with undefined still meaning "ask".
  • Regression unit test — pins disabled plain click + enabled middle click returning false with no PTY claim, no preventDefault, no request, and no primary run; it fails against the pre-fix handler.
  • Restart-survival E2E retry — the survivor-presence, single-row count, original identity, and client placement checks are now one toPass block, and the row count is re-checked after the recovered guest renders. Permanent duplicate/replay detection is preserved (a create-URL replay keeps the count at 2 and still fails).
  • Claude queued-send expectations — the rejection: { kind: 'cancelled' } assertions come from #23675, which shares this head branch (nwparker/fix-ci-09-28); production emits that field, so the expectations are correct. Merge order: land #23675 first as the PR body notes.

Existing coverage is genuine: terminal-link-click-ownership.spec.ts:134 exercises the disabled-plain/middle-open combination end to end and asserts the child still receives PTY mouse reports, and native-chat-web-link-actions.ts:65 already honored none, so no parallel fix was needed there.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of the delta since my prior review at 08e697b8: commit 167d3e5e gives the daemon-generation legacy-close fixture a real profile store.

  • New fixture store — tests/e2e/fixtures/daemon-generation-profile-store.ts builds a Store from the production createProfileStateStore factory (synchronous SQLite authority, after installOrcadHostAdapters()), and daemon-generation-legacy-close-client.ts passes it to OrcaRuntimeService and closes it with store?.freezeWritesAsync() in finally.
  • Verified the load-bearing pieces: RuntimeStore is the structural subset of Store the runtime constructor accepts, so the uncast argument typechecks; freezeWritesAsync drains pending writes and closes the profile-state authority (profile-state-maintenance.ts:41), matching every other test fixture; and the DB/JSON files land under config.cwd = runtime.rootDir, which the harness asserts is a disposable temp root, so nothing pollutes the worktree. The sibling daemon-generation-reconnect-client.ts still constructs without a store, but its listMobileSessionTabs path resolves through getKnownWorktreeIds (empty set, no folder_workspace_not_found throw), so it correctly needs no change.

No inline comments — the previously reviewed terminal-link and restart-survival changes are unchanged in this delta.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit 55aea85 into main Sep 28, 2026
40 checks passed
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