fix(web): remove the wall-clock race in the import-JSON debounce guard test

[cliffhall]

Closes #2250

ServerImportJsonModal › guards against a live edit made before the debounce re-validates was a genuine timing race, not merely a slow-machine symptom, and it is fixed by removing the race rather than by widening a timeout.

The race

The test needs the window between an edit and the VALIDATE_DEBOUNCE_MS (300 ms) debounce that re-disables Add Server — that is the window the submit-time guard in useServerJsonImport.submit() exists for. It opened that window on the real clock:

await pasteJson("{not json");
fireEvent.click(screen.getByRole("button", { name: "Add Server" }));

If more than 300 ms of wall clock elapses between those two lines, the debounce lands first, canAdd goes false, the button is disabled, and fireEvent.click on a disabled button is a no-op. onAddServer is still not called — so the first assertion passes and hides the problem — but submit() never runs, no submitError is set, and findByText(/Fix the validation errors/) times out. That is exactly the reported failure, and it explains why run 4 in the issue failed on a quiet machine with the test itself taking only 1.6 s: it only needs one 300 ms scheduling gap, which parallel workers make likely without needing sustained load.

Proof, before and after

A throwaway probe held both shapes side by side with a deterministic 400 ms stall injected between the paste and the click:

shape	400 ms stall before the click	result
old (real timers)	yes	fails — findByText(/Fix the validation errors/) times out
new (fake timers)	yes, 400 ms of real wall clock	passes

The old shape reproduces the reported failure exactly; the new shape is indifferent to the same stall, because the debounce cannot advance at all unless the test advances it.

The fix

Run that one test on fake timers end to end:

• vi.useFakeTimers() before the render, restored in a finally, so no other test in the file is affected.
• The first validation is landed explicitly with await act(async () => vi.advanceTimersByTimeAsync(VALIDATE_DEBOUNCE_MS)) rather than a waitFor on the real clock — VALIDATE_DEBOUNCE_MS is imported from the hook, so the test cannot drift from the value it depends on.
• After the second paste the timers are simply not advanced. The pending re-validation therefore cannot land, the window stays open by construction, and findByText becomes a synchronous getByText because there is nothing left to wait for.

No timeout was widened and no production code changed — the guard being tested is unchanged.

On the second suspect

The issue asked to check ServerSettingsModal › maps the OAuth insufficient-scope policy into settings (SEP-2350) before assuming the two are independent. They are independent: that test has no debounce and no timer window at all — it is a userEvent click sequence through a Mantine Select — and it failed once, on the run where the box was at load ~150 and the file took 90 787 ms. There is no analogous "state changes underneath the assertion after N ms" shape to remove there, so nothing in this PR touches it. If it recurs on a quiet machine it should get its own issue with that evidence.

Verification

Full npm run local:gate in a clean worktree, several consecutive runs — see the comment thread on the PR for the run log.

---

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQgwZ1kGzhkdkMJ81JVg42

[Copilot]

🟢 Approval recommended

The focused test-only change deterministically exercises the intended pre-debounce submission window and safely restores real timers.

Pull request overview

Makes the import-JSON debounce guard test deterministic by controlling timer advancement.

Changes:

• Uses fake timers for the race-sensitive test.
• Advances the shared debounce duration explicitly.
• Replaces the asynchronous error lookup with a synchronous assertion.

File summaries

File	Description
clients/web/src/components/groups/ServerImportJsonModal/ServerImportJsonModal.test.tsx	Removes reliance on wall-clock timing in the debounce guard test.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The focused test-only change deterministically exercises the intended debounce window and safely restores real timers.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[cliffhall]

Determinism evidence

The acceptance criterion asks for the test to pass inside a full parallel run, over several consecutive runs — not just in isolation. Five consecutive full --project=unit runs (all 326 files in parallel, which is the condition the flake needs), on the branch:

run	1-min load at start	result
1	50.0	326/326 files, 5835/5835 tests passed
2	157.3	326/326 files, 5835/5835 tests passed
3	157.4	326/326 files, 5835/5835 tests passed
4	149.4	326/326 files, 5835/5835 tests passed
5	171.0	326/326 files, 5835/5835 tests passed

Four of the five ran at load 149–171 — higher than any run in the issue's evidence table (which topped out around 150, and where run 2 failed at load 50–126). The box was genuinely contended throughout: two other worktrees were running their own gates concurrently.

Plus a full npm run local:gate on the branch: exit 0, all stages, including web unit 5835/5835, the coverage project 7418/7418 across 399 files, and 122 Storybook files.

A note on the first gate attempt, since it is instructive

The first local:gate run on this branch came back red — and it is worth recording why it does not count, because it is the same misreading the issue is about.

Five tests failed: four with Test timed out in 5000ms (App.test.tsx #1914, AppsScreen tool-input replay, and two ServerSettingsModal header tests) and one wall-clock assertion in revocation.test.ts (expected 77 to be less than 70). The #2250 test itself passed. A second full local:gate was running concurrently out of another worktree and the box was at load 152.

Non-causation is settled by the diff rather than by argument: this PR changes one file, a test file, and none of the five failing tests live in it.

The revocation.test.ts failure is separately accounted for — it is the subject of #2252, in flight on its own branch.

On the second suspect named in the issue

The issue asked to check ServerSettingsModal › maps the OAuth insufficient-scope policy into settings (SEP-2350) before assuming the two are independent. They are independent. That test has no debounce and no timer window at all — it is a userEvent click sequence through a Mantine Select. There is no "state changes underneath the assertion after N ms" shape to remove, so nothing here touches it.

What the first gate run above adds is a useful data point in the other direction: under load, the ServerSettingsModal failures were different tests in that file (the two custom-header ones), and all four timeouts were plain 5 s exhaustion across four unrelated files. That is the signature of contention, not of a shared root cause — a real shared race would not wander between tests. If it recurs on a quiet machine it deserves its own issue with that evidence, the way run 4 in #2250 earned this one.