[juliusmarminge]

SourceControlWritingSettings.browser.test.tsx > "resets mixed template preferences without replacing each environment's instructions" fails now and then in CI with expected "vi.fn()" to be called 1 times, but got 0 times (failing job, on a PR that doesn't touch this area).

Root cause

The reset button sits inside the Advanced fold, a Base UI CollapsiblePanel that animates its height over 200ms with overflow: hidden. The test clicked the button right after opening the fold, while the panel was still animating:

1. Playwright's visible and stable checks pass mid-animation, because the button's own box is non-empty and doesn't move while the panel grows.
2. To reach the clipped button, Playwright scrolls the overflow: hidden panel. A diagnostic run logged the panel at height: 0.2px with scrollTop: 39 when pointerdown hit the button.
3. As the panel keeps growing, the browser clamps scrollTop, and the content, button included, moves down.
4. Playwright checks the hit target only on the first pointer event. If the button moves between press and release, click fires on a common ancestor, so onClick never runs and updateSettings is never called.

There's no async step between the click and updateSettings (the reset onClick calls it synchronously), so wrapping the assertions in vi.waitFor would not help: the click itself is lost. The component has no race either: a person can't click a button that is still clipped.

Fix

Before clicking, wait until the button is fully visible: await expect.element(button).toBeInViewport({ ratio: 1 }). toBeInViewport uses an IntersectionObserver, which accounts for clipping by the panel. Once the button is fully visible at scrollTop: 0, Playwright doesn't scroll the panel, and the rest of the animation doesn't move the button.

Verification

A loaded runner lengthens the gap between Playwright's press and release, so the repro sets Playwright's click delay (the hold between mousedown and mouseup), with the real 200ms transition left as is:

• Before (main), 6 runs per hold of 10/30/60/100ms: 11 of 24 failed with the CI error, every one at 60ms or 100ms.
• After, same sweep: 24/24 passed. 12 runs per hold under 16 yes > /dev/null stressors: 48/48 passed.
• The unmodified test passes, and vp run check:changed passes.

Plain CPU stress (40 repeats with 16 stressors) and Chromium CPU throttling did not reproduce the failure on their own on this machine.

Model and harness: Claude Opus 5.5 in Claude Code, run from T3 Code.

🤖 Generated with Claude Code

[macroscopeapp]

Approvability

Verdict: Approved at bc8162d

Macroscope's review found this PR approvable — The change is confined to an ignored browser-test path and is described as waiting for the reset button to become visible before clicking. No material production behavior or product-default change is indicated.

Notes:

• No code objects were reviewed. Approvability was decided on eligibility alone.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The browser test now waits for the “Reset change request templates” button to be fully in the viewport after opening Advanced and before clicking it. Existing assertions remain unchanged.

Changes: Reset button browser test

Layer / File(s)	Summary
Wait for reset button visibility apps/web/src/components/settings/SourceControlWritingSettings.browser.test.tsx	The test documents the panel-scroll and click-timing issue, then waits for the reset button to be fully in the viewport before clicking it.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Possibly related PRs

• pingdotgg/t3code#25651: Adds the Advanced settings panel that contains the reset row tested here.
• pingdotgg/t3code#28842: Places the source-control template controls in Advanced settings and updates the related test flow.

Suggested reviewers: maria-rcks

Merge Risk: Minimal · up to bc816

The test now waits for full viewport visibility before clicking, with no identified merge-blocking risk.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check		The description covers the problem, the root cause, the fix, and verification. The Problem section describes the flaky CI failure and its error text. The Change section explains why waiting for the bu…	Add a "Scope and approval" section. Link the triaged bug issue or the discussion where a maintainer approved this direction, including the approval comment. If no issue or discussion exists, explain why this test-only fix for an obvious fla…

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title "test(web): source control writing settings test waits for the reset button" matches the one-file change. The change makes the browser test wait for the Reset change request templates button…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "test(web): source control writing settings test waits for the reset button" matches the one-file change. The change makes the browser test wait for the Reset change request templates button to be in the viewport before clicking it. The title is specific and uses a conventional commit prefix. It is a test-only fix, so the title describes the main change.

Full details: Description check

Explanation

The description covers the problem, the root cause, the fix, and verification. The Problem section describes the flaky CI failure and its error text. The Change section explains why waiting for the button to be fully in the viewport fixes the race. The Verification section gives repro counts before and after the fix and lists the checks that ran. The template's Scope and approval section is missing. The author does not link a triaged issue or maintainer approval, and does not explain why the change qualifies as a small, obvious fix without prior approval. The description also has no before/after screenshots, but the template only requires them for UI changes, and this change is test-only. The description is mostly complete, but the missing Scope and approval section is a required gap.

Resolution

Add a "Scope and approval" section. Link the triaged bug issue or the discussion where a maintainer approved this direction, including the approval comment. If no issue or discussion exists, explain why this test-only fix for an obvious flake qualifies as a very small, focused fix without prior approval. Remove the trailing HTML comment and template prose from the final description if they were copied in.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: t3/fix-source-control-writing-settings-flake
• File: .coderabbit.config.ts
• Commit: bc8162d
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Team
• Run ID: f8fbef2b-2275-456a-a5ef-87ade2392cac

Commits

Reviewing files that changed from the base of the PR and between d1ec71e and bc8162d.

Files selected for processing: 1

• apps/web/src/components/settings/SourceControlWritingSettings.browser.test.tsx

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