test(e2e): stabilize chronically-failing e2e suite

[nwparker]

Summary

The scheduled E2E suite has been red for 3+ weeks (40+ runs), with ~19 tests failing deterministically across 9 of 10 shards. This stabilizes the whole suite.

Every failure is test-side — stale assertions, CI-timing races, over-strict perf thresholds, and fixture gaps. No product regressions were found. Most tests actually fail on a fresh local build too (the suite had drifted from current app behavior); a handful only fail on the loaded OSS Linux runners.

Each fix was verified locally against a fresh --mode e2e build; the env/timing-only ones are validated by this branch's E2E run.

App changes (test-support only)

• GitHubItemDialog: added a stable data-testid="github-item-detail" — GitHub item details render inline (not a Radix dialog), so the test's getByRole('dialog') locator had nothing to match.
• worktree-sidebar-reveal: honor prefers-reduced-motion by jumping instead of smooth-scrolling (a genuine a11y improvement; also makes the reveal deterministic in headless where the smooth-scroll animation never ticks).

Test fixes by cause

Stale assertions (app UI changed, tests weren't updated — these fail on any fresh build):

• pr-comments-sidebar-cards: the per-card "Open" badge was removed in Remove Open badge from PR comment actions #7338.
• github-cli-stall-repro: a stalled gh detail fetch now degrades to an explicit "Unable to load details" error (Fix runtime GitHub work item details #6473), not an empty shell — the recovery signal the test checks.
• onboarding (Skip → server UI): the Add Project dialog was redesigned to a Host selector ("env-e2e Connected" + "Browse folder"/"Create new project"); test also now seeds a healthy runtime status so the host reads available (the disconnected-when-statusless default is intentional, Fix Cmd+J host badge spacing and false "Connected" runtime state #5362).
• settings-display-name-ime: persistence is deferred to compositionend (fix(settings): defer Display Name persistence until IME composition ends #6238), so the test now commits the composition.

CI-timing races (pass locally, fail under loaded runners):

• source-control-commit-* / workspace-space-git-status: poll worktrees.list past its 5s detection-scan cache; match git-reported store paths instead of realpath'd ones.
• terminal-column-desync-repro: poll PTY↔xterm columns to convergence instead of a fixed 400ms wait.
• tab-close-navigation: drain a late async startup-terminal spawn.
• terminal-shortcuts: guard the split still exists before Cmd/Ctrl+W, and confirm the "Stop and Close" dialog (the old locator looked for "Close").

Over-strict / stale metrics:

• terminal-tui-wheel-reports: space notches past the 45ms burst-acceleration window so a notched wheel maps 1:1 (the burst feature, Refine TUI wheel scroll feel #7179, is working as designed).
• terminal-tui-wheel-drain: reduced the dense CDP stream 240→120 events + test.slow() to fit the 120s budget.
• combined-diff-scroll-restore: ignore virtualizer remeasurement (scrollHeight changes) in the backward-scroll-jump metric.
• artificial-opencode-terminal-load: the timer-drift gate now tolerates a single scheduler spike (reports 2nd-worst drift) — sustained blocking still fails.
• onboarding (Continue): broaden the step predicate for auto-skipped optional steps (e.g. Integrations when gh is installed).
• worktree (pasted-URL reuse): resolve the create base to the local HEAD branch (fixture has no origin); assert URL-resolution reuse via the lookup count (the resolvePrBase timing is nondeterministic).

Verification

• pnpm typecheck ✅ · oxlint ✅ · reliability gates ✅
• All 19 tests pass locally in isolation/small groups against a fresh e2e build.

Made with Orca 🐋

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR adds a data-testid to the GitHub item detail wrapper, adjusts worktree sidebar scroll reveal to short-circuit when no scrolling is needed and to respect reduced-motion preferences, and makes terminal rename inputs submit on Tab. Numerous e2e tests are updated: replacing fixed waits/single-shot assertions with polling for timer drift, scroll jumps, terminal drain, column sync, and repo loading; adjusting timing thresholds and workload sizes; making a shared setup fetch best-effort; and revising selection/assertion logic in activity pane and worktree reveal tests.

Related PRs: None identified.

Suggested labels: e2e, tests, renderer

Suggested reviewers: None identified.

🐰 A hop through code both quick and neat,
Polling loops replace the wait's retreat,
Tab now submits, scroll bends to your rest,
A testid marks the detail nest,
Stable tests, a rabbit's small feat.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and clearly summarizes the main change: stabilizing the E2E suite.
Description check	✅ Passed	It covers summary and verification well, but omits the template's Screenshots, Testing checklist, AI Review Report, Security Audit, and Notes sections.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

tests/e2e/artificial-opencode-scroll-scenario.ts (1)

109-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Field still named maxTimerDriftMs but now returns second-worst drift.

The top-2 tracking logic itself is correct, but the value returned by stop() is the second-worst drift, while the property is still called maxTimerDriftMs (line 185) and consumed identically downstream (artificial-opencode-main-pressure-scenario.ts asserts scrollMeasurement.maxTimerDriftMs). Anyone reading the assertion without this file's context will assume it's the true maximum, which could mask the intent behind future changes to the threshold.

Consider renaming the field (e.g. secondWorstTimerDriftMs) across producer/consumer, or at least a short doc comment on the ScrollMeasurement type clarifying the semantic change.

Also applies to: 185-185

tests/e2e/source-control-commit-draft-persistence.spec.ts (1)

68-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct, but duplicated verbatim across specs.

The deadline-based polling logic and correctness here are sound. However, this exact block (deadline loop + normalizeMacTmpPath) is duplicated near-verbatim in tests/e2e/source-control-commit-message-ai.spec.ts (lines 65-83), and normalizeMacTmpPath alone is duplicated again in tests/e2e/workspace-space-git-status.spec.ts. Since this "5s detection-scan cache" workaround is likely to recur in other specs too, consider extracting a shared helper (e.g., in tests/e2e/helpers/) so the timeout/interval and cache-staleness fix stay consistent in one place.

♻️ Suggested shared helper sketch

// tests/e2e/helpers/worktree-polling.ts
export function normalizeMacTmpPath(value: string): string {
  return value.startsWith('/private/var/') ? value.slice('/private'.length) : value
}

export async function pollForWorktreeByPath(
  list: () => Promise<{ path: string }[]>,
  targetPath: string,
  timeoutMs = 10_000,
  intervalMs = 250
): Promise<{ path: string }[]> {
  const deadline = Date.now() + timeoutMs
  let entries = await list()
  while (
    !entries.some((e) => normalizeMacTmpPath(e.path) === normalizeMacTmpPath(targetPath)) &&
    Date.now() < deadline
  ) {
    await new Promise((resolve) => setTimeout(resolve, intervalMs))
    entries = await list()
  }
  return entries
}

tests/e2e/terminal-column-desync-repro.spec.ts (1)

137-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Trim the new "Why" comments to 1-2 lines.

Both explanatory blocks run 4 lines each, longer than the repo's convention for these comments.

As per coding guidelines, **/*.{ts,tsx,js,jsx}: "Keep comments short — one or two lines, capturing only the non-obvious reason."

✂️ Suggested trims

-    // Why: the resize chain (ResizeObserver → rAF fit → PTY resize IPC) needs
-    // longer than a fixed wait under loaded CI, and the two columns are sampled
-    // non-atomically. Poll until they converge — a genuinely dropped resize
-    // never converges and still fails, so this keeps the regression guard.
+    // Why: resize settles asynchronously under CI load, so poll for convergence
+    // instead of a fixed wait; a genuinely dropped resize still fails.

-    // Why: poll until pty:getSize converges to the real applied columns instead
-    // of sampling once after a fixed wait — the resize can still be settling on
-    // loaded CI. A getSize that reports intent (not the applied size) never
-    // converges to process.stdout.columns and still fails the guard.
+    // Why: poll for convergence instead of a fixed wait, since resize can still
+    // be settling under CI load; a getSize reporting intent instead of applied
+    // size still fails to converge.

Also applies to: 183-186

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0be4e550-1a3d-415d-ae85-beb5ec43a3d2

📥 Commits

Reviewing files that changed from the base of the PR and between 0fa8589 and 0bf7458.

📒 Files selected for processing (18)

• src/renderer/src/components/GitHubItemDialog.tsx
• src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts
• tests/e2e/artificial-opencode-scroll-scenario.ts
• tests/e2e/combined-diff-scroll-restore.spec.ts
• tests/e2e/github-cli-stall-repro.spec.ts
• tests/e2e/onboarding.spec.ts
• tests/e2e/pr-comments-sidebar-cards.spec.ts
• tests/e2e/settings-display-name-ime.spec.ts
• tests/e2e/source-control-commit-draft-persistence.spec.ts
• tests/e2e/source-control-commit-message-ai.spec.ts
• tests/e2e/tab-close-navigation.spec.ts
• tests/e2e/terminal-column-desync-repro.spec.ts
• tests/e2e/terminal-shortcuts.spec.ts
• tests/e2e/terminal-tui-wheel-drain.spec.ts
• tests/e2e/terminal-tui-wheel-reports.spec.ts
• tests/e2e/workspace-space-git-status.spec.ts
• tests/e2e/worktree-scroll-to-current.spec.ts
• tests/e2e/worktree.spec.ts

💤 Files with no reviewable changes (1)

• tests/e2e/pr-comments-sidebar-cards.spec.ts

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Add a short settle wait after the final commit

The sibling IME helper waits briefly after Input.insertText before returning. This helper returns immediately, so the async store echo can still race the caller’s next assertion. A small post-commit pause would keep this path aligned with the rest of the IME coverage.

🐛 Proposed fix

   await session.send('Input.insertText', { text: `${committed}${pending}` })
+  // Let the async store echo settle after the final commit.
+  await page.waitForTimeout(150)
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Why: commit the composition the way a real IME does when the user accepts
	// the buffer. Since #6238 the input defers persistence until compositionend,
	// so without this the store never receives the composed syllables.
	await session.send('Input.insertText', { text: `${committed}${pending}` })
	// Why: commit the composition the way a real IME does when the user accepts
	// the buffer. Since `#6238` the input defers persistence until compositionend,
	// so without this the store never receives the composed syllables.
	await session.send('Input.insertText', { text: `${committed}${pending}` })
	// Let the async store echo settle after the final commit.
	await page.waitForTimeout(150)

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect the waitForPtyColumnsAtMost implementation and READ_ANY_COLS semantics
rg -n 'READ_ANY_COLS|function waitForPtyColumnsAtMost' tests/e2e/terminal-column-desync-repro.spec.ts tests/e2e/helpers -A 15

# Check whether the resize test's timeout has been raised
rg -n 'test\.setTimeout|test\(' tests/e2e/terminal-column-desync-repro.spec.ts -B2 -A2

Repository: stablyai/orca

Length of output: 5176

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the helper implementation and the exact test blocks around the new polling.
rg -n 'waitForPtyColumnsAtMost|READ_ANY_COLS|expectColumnsInSync|test\.setTimeout|//|/\*\*' tests/e2e/terminal-column-desync-repro.spec.ts -A 8 -B 8

# Locate the helper if it lives elsewhere.
rg -n 'waitForPtyColumnsAtMost' tests/e2e -A 12 -B 6

Repository: stablyai/orca

Length of output: 26370

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the PTY column probe helper to determine whether it already polls/retries internally.
cat -n tests/e2e/terminal-column-probes.ts | sed -n '1,220p'

# Show the full resize test block around the new helper call for timeout context.
cat -n tests/e2e/terminal-column-desync-repro.spec.ts | sed -n '128,210p'

Repository: stablyai/orca

Length of output: 9747

---

Avoid stacking the 30s PTY probe inside a 30s poll readPtyCols() already retries for up to 30_000ms, so wrapping it in expect.poll({ timeout: 30_000 }) leaves almost no room for a second attempt on slow CI. Shorten the inner probe timeout or give the outer poll more budget; the same pattern applies to the pty:getSize check.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts (1)

54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Trim the "Why" comments to the guideline's 1–2 line limit.

Both blocks explain good non-obvious reasoning, but the guideline caps "Why" comments at one or two lines. The block at Lines 68-74 in particular runs 7 lines.

♻️ Suggested condensation

-  // Why: honor the user's reduced-motion preference by jumping instantly instead
-  // of animating a smooth scroll (also makes the reveal deterministic in headless
-  // environments that never tick the smooth-scroll animation).
+  // Why: reduced-motion users get an instant jump instead of animating (also
+  // deterministic for headless tests, which never tick smooth-scroll frames).

-  // Why: an instant scroll applies synchronously, but the browser clamps it to
-  // scrollHeight, which can momentarily lag a freshly measured (just-activated)
-  // row's real height — leaving the row clipped short. Report "not fully
-  // revealed" so the caller re-stages via the virtualizer and retries on the next
-  // frame instead of clearing while the row is still clipped. A row taller than
-  // the viewport can never fully fit, so treat that as done to avoid an endless
-  // retry.
+  // Why: an instant scroll can clip a just-activated row if scrollHeight lags
+  // its real height; report unresolved so the caller retries next frame
+  // (oversized rows are treated as done to avoid an endless retry).

As per coding guidelines: "Keep comments short — one or two lines, capturing only the non-obvious reason."

Also applies to: 68-74

Source: Coding guidelines

src/renderer/src/components/terminal-pane/TerminalPaneHeaderOverlay.tsx (1)

194-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Trim the Tab-branch comment to comply with comment-length guideline.

The added rationale comment spans 6 lines; the logic itself (calling onRenameSubmit() on Tab, mirroring the Enter branch) is otherwise sound and consistent with existing patterns and is covered by an existing test.

As per coding guidelines, "Keep comments short — one or two lines, capturing only the non-obvious reason."

✏️ Suggested comment trim

                   } else if (event.key === 'Tab') {
-                    // Why: commit on Tab directly instead of relying on the
-                    // browser advancing focus (which fires blur). Headless / no
-                    // window-focus environments (xvfb, some SSH sessions) don't
-                    // always move focus off the input, so the blur-driven commit
-                    // never runs. Submitting closes the editor, so the default
-                    // Tab focus move is moot and any follow-on blur is a no-op.
+                    // Why: commit on Tab directly; blur-driven commit is unreliable
+                    // in headless/no-window-focus environments.
                     onRenameSubmit()

Source: Coding guidelines

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 74362bf2-8ec2-49ff-9ef1-51b098d9e410

📥 Commits

Reviewing files that changed from the base of the PR and between 0bf7458 and d63d0b3.

📒 Files selected for processing (6)

• src/renderer/src/components/sidebar/worktree-sidebar-reveal.ts
• src/renderer/src/components/terminal-pane/TerminalPaneHeaderOverlay.tsx
• tests/e2e/activity-agent-pane-isolation.spec.ts
• tests/e2e/artificial-opencode-hidden-pressure-scenario.ts
• tests/e2e/project-group-manual-sort.spec.ts
• tests/e2e/terminal-tui-wheel-drain.spec.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/e2e/terminal-tui-wheel-drain.spec.ts

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fallback re-introduces the exact flakiness being fixed.

When snapshot.panes has no pane whose leafId !== activeLeafId (e.g., a single-pane scenario), .find() returns undefined and the ?? fallback silently substitutes snapshot.panes[0] — which is the active pane. The guard on the next line never fires (since panes[0] is truthy), so the test can seed the thread on the active pane, hitting the auto-ack race the comment above explicitly warns about.

🐛 Proposed fix

-    const targetPane =
-      snapshot.panes.find((pane) => pane.leafId !== activeLeafId) ?? snapshot.panes[0]
+    const targetPane = snapshot.panes.find((pane) => pane.leafId !== activeLeafId)
     if (!targetPane) {
-      throw new Error('Activity acknowledgement test needs a split pane')
+      throw new Error('Activity acknowledgement test needs a non-active split pane')
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const activeLeafId = await orcaPage.evaluate(
	(tabId) => window.__store?.getState().terminalLayoutsByTabId[tabId]?.activeLeafId ?? null,
	snapshot.tabId
	)
	const targetPane =
	snapshot.panes.find((pane) => pane.leafId !== activeLeafId) ?? snapshot.panes[0]
	if (!targetPane) {
	throw new Error('Activity acknowledgement test needs a split pane')
	}
	const activeLeafId = await orcaPage.evaluate(
	(tabId) => window.__store?.getState().terminalLayoutsByTabId[tabId]?.activeLeafId ?? null,
	snapshot.tabId
	)
	const targetPane = snapshot.panes.find((pane) => pane.leafId !== activeLeafId)
	if (!targetPane) {
	throw new Error('Activity acknowledgement test needs a non-active split pane')
	}

[coderabbitai]

🧹 Nitpick comments (1)

tests/e2e/artificial-opencode-hidden-pressure-scenario.ts (1)

209-222: 🚀 Performance & Scalability | 🔵 Trivial

Loosened worst-case threshold reduces regression sensitivity in the 300ms–3000ms band.

The rationale is well-documented and the median guard remains strict, but a 10x threshold loosening means moderate regressions in worstLatencyMs will now pass silently — only the median (<75ms) or catastrophic hangs (<3000ms) are caught. Given this is annotated (Line 192-201) but not otherwise trended, consider whether CI should track/alert on worstLatencyMs drift over time so a slow regression toward the new ceiling isn't missed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 91999525-43ff-469d-94ba-930cc574f552

📥 Commits

Reviewing files that changed from the base of the PR and between 5851a3c and 546a416.

📒 Files selected for processing (2)

• tests/e2e/artificial-opencode-hidden-pressure-scenario.ts
• tests/e2e/worktree-scroll-to-current.spec.ts