fix(ui-test): give Search.test.tsx a real wall-clock wait budget (BLO-30247)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its CI exposes a required aggregate check (verify) whose red state blocks merge, fed by the General tests (workspaces-a) shard that runs the ui vitest suite
• ui/src/pages/Search.test.tsx intermittently reds that shard with Number of calls: 0 — three known instances (fix(recovery): stop the liveness detector blocking an issue on its own recovery row (BLO-28618) #1420, fix(productivity-review): make three evidence labels true of what they count (BLO-29535) #1466, fix(issues): evaluate the assignee comment-wake guard on the post-update status (BLO-29821) #1497), every one of them on a server-only diff that cannot reach this UI file
• It kept getting re-run rather than read, because the test looks timing-robust: assertions are wrapped in a waitForAssertion(..., attempts = 50) helper that reads like a retry safety net
• That helper is not a retry helper in any useful sense — each attempt is a microtask drain plus setTimeout(0), which cannot advance a pending 250 ms debounce timer, so the fixed setTimeout(resolve, 350) sleep was the only real wait, on a 100 ms margin
• This pull request gives the helper an actual wall-clock budget and derives both it and the pre-assertion sleep from SEARCH_DEBOUNCE_MS
• The benefit is that a contended runner is absorbed by retries instead of eating the margin, and raising the debounce can no longer silently re-open the flake

Linked Issues or Issue Description

• Refs BLO-30247 (Paperclip issue; no GitHub issue)
• Same class as test(heartbeat): de-flake the forward-cursor resume test (BLO-22418) #1329 / test(heartbeat): de-flake the queued-backlog hard-ceiling test (BLO-22418) #1098 (heartbeat de-flakes) and the CopyText auto-dismiss race in BLO-21603, but a different file and a different timer. Dedup search over open PRs found no other PR touching ui/src/pages/Search*.

What Changed

• waitForAssertion is now deadline-based with a real 10 ms per-attempt delay, replacing the attempt-count loop that had no wall-clock budget.
• ASSERTION_TIMEOUT_MS (SEARCH_DEBOUNCE_MS * 8) and the pre-assertion sleep both derive from the source constant, so raising the debounce cannot silently re-open this.
• All 6 fixed-sleep sites audited: the 5 load-bearing setTimeout(resolve, 350) calls became settleDebounce(); flush()'s setTimeout(resolve, 0) is documented as a sound macrotask yield (a yield, not a wait for a specific timer).
• ui/src/pages/Search.tsx: the existing SEARCH_DEBOUNCE_MS constant is now importable, plus a comment. No behavioural change.

Verification

CPU contention could not serve as the negative control: at 48 spinners on 32 cores, vitest's worker startup times out (Tests no tests) before the debounce is perturbed, so it never reproduces the assertion failure. I therefore reproduced the mechanism deterministically by raising SEARCH_DEBOUNCE_MS so it outlasts the old fixed 350 ms sleep. If the helper had a real budget, these would still pass.

SEARCH_DEBOUNCE_MS	pre-fix (master 237500ca2)	post-fix
250 (real)	19 passed	19 passed
400	19 passed	—
500	3 failed — Number of calls: 0	19 passed
600	4 failed — Number of calls: 0	19 passed
1000	—	19 passed

The pre-fix break between 400 and 500 quantifies the real budget: 350 ms sleep plus only ~50–100 ms of retry drain, against a 250 ms debounce. The failing signature matches the field failures exactly.

cd ui && npx vitest run src/pages/Search.test.tsx --pool=threads --maxWorkers=1 --no-file-parallelism

Note for the issue author (me): both commands in BLO-30247's Verifying signal — --poolOptions.threads.singleThread and --repeat — are rejected by vitest 4.1.8 as unknown options. The supported equivalents are above; I am correcting the issue.

No UI change, so no screenshots.

Risks

Low. Test-harness only; Search.tsx gains no runtime behaviour.

Two things worth a reviewer's eye:

• waitForAssertion's signature changed from attempts = 50 to timeoutMs. No call site passed the second argument (verified), so this is source-compatible — but it is a behavioural change for any future caller that assumes a count.
• A genuinely failing assertion now takes up to ASSERTION_TIMEOUT_MS (2 s) to surface instead of failing in microseconds, so a real regression in this file reports slightly slower. Since the helper returns on first success, the longer budget cannot change which assertions pass — only how long a true failure takes to report.

Model Used

Claude Opus 4.5 (claude-opus-4-5), extended thinking, via Claude Code / Claude Agent SDK with tool use and code execution.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI change
• I have updated relevant documentation to reflect my changes — n/a, in-file comments only
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-30247

[allyblockcast]

🔗 Paperclip issue: BLO-30247

[allyblockcast]

@ally please review at head 8fba06765ea6ff0e5e2f30cd0e5cbcb27e5e2c56 — BLO-30247, test-harness only.

Focus:

1. waitForAssertion is now deadline-based (ui/src/pages/Search.test.tsx). No caller passed the old attempts argument, so the signature change should be safe — please confirm. It still returns on first success, so a longer budget cannot change which assertions pass, only how long a genuine failure takes to surface.
2. settleDebounce() sleeps SEARCH_DEBOUNCE_MS (250) where the old code slept a hard-coded 350. That is intentional — the retry budget is meant to be the load-bearing wait now — but flag it if any assertion actually depends on the sleep alone.
3. Search.tsx changes only by making the existing constant importable. Please confirm no behavioural change.
4. The negative control is a debounce bump, not CPU contention (contention saturates vitest worker startup before it perturbs the debounce). Tell me if you consider that insufficient evidence for the flake claim.

[allyblockcast]

Correction to the review request above: the head SHA I quoted was wrong — I expanded it from the short hash instead of reading it. The actual head is 8fba06765cc8da1d13f65d5bce4a09d481e14a11 (short 8fba06765, the only commit on this branch). The review focus points are unchanged.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8fba067

Looks good. The diagnosis is right and the fix is the right shape: a retry count was never a wall-clock budget, and swapping it for a real deadline derived from the production debounce is a durable fix rather than another magic-number bump. No blockers.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [comments] ui/src/pages/Search.test.tsx:115 — "Both numbers below derive from SEARCH_DEBOUNCE_MS" is not true of ASSERTION_POLL_MS = 10 (:119), which is a bare literal; only ASSERTION_TIMEOUT_MS (:118) derives. A fixed poll interval is the correct choice — the comment just tells a future reader the opposite, which is exactly the kind of thing this comment exists to prevent. Suggest "The budget below derives from SEARCH_DEBOUNCE_MS …" and, if worth saying, a clause noting the poll interval is deliberately independent of it.

• [code] ui/src/pages/Search.test.tsx:142 — settleDebounce() may be worth deleting entirely rather than shortening. The request is only issued after the debounce sets committedQuery and React re-renders and react-query runs, so a sleep of exactly SEARCH_DEBOUNCE_MS is never sufficient on its own; at all five call sites the following waitForAssertion is what actually finishes the wait. It buys ~25 skipped assertion evaluations in exchange for a second wait concept in the file and a hard 250ms floor per call site. Letting waitForAssertion poll from t=0 would be simpler and no slower. The :139 comment already concedes it is not load-bearing — that is a good argument for removing it.

Strengths

• The root-cause narrative at :110-116 is accurate and verifiable from the diff: flush() is one microtask drain plus a setTimeout(0), so attempts = 50 bought roughly 50-100ms and the fixed 350ms sleep was carrying the test. Fixing the budget rather than raising the sleep is the correct call.
• Deriving ASSERTION_TIMEOUT_MS from the production SEARCH_DEBOUNCE_MS couples the budget to the thing it must outlast, so raising the debounce cannot silently re-open this flake. That is the part that makes the fix durable.
• The signature change from attempts = 50 to timeoutMs = ASSERTION_TIMEOUT_MS is safe: all 27 call sites pass a single argument, so no caller silently reinterprets an attempt count as milliseconds.
• The loop always makes one final attempt at or after the deadline (:124-131) before throwing, and lastError is always assigned before break — no window where it throws undefined.
• Shortening 350ms to 250ms cannot weaken the negative assertions: expect(searchApiMock.search).not.toHaveBeenCalled() at :536 is synchronous and runs before settleDebounce(), so it still asserts the debounce has not fired.
• Nor can it cause a stale pass in the mock.calls.at(-1) tests at :778 and :824 — the expected shape differs from the preceding call, so a not-yet-fired debounce fails the assertion and retries rather than matching the previous call.
• Exporting SEARCH_DEBOUNCE_MS is purely additive, and the module already exports a non-component helper (buildSearchUrl) that the test consumes. The repo ships no ESLint config, so there is no react-refresh/only-export-components exposure to weigh here.
• ui/vitest.config.ts sets testTimeout: 30_000, which comfortably absorbs the new 2s worst case per assertion. A genuine regression will still surface the real assertion error rather than being masked by a suite timeout — worth confirming, since a larger budget under the stock 5s cap would have degraded diagnosability.

Recommended Action

1. No Critical issues.
2. No Important issues.
3. Consider the two Suggestions opportunistically; neither blocks merge.

[kkroo]

Reviewed current head 8fba067. The deadline-based helper fixes the flake mechanism and derives its budget from the production debounce; remaining suggestions are non-blocking test cleanup. No blocking findings.