fix(evalboard): prevent search bar from resetting mid-type

[joeysbase]

Summary

• Fixes a race where the search input was overwritten with a stale URL value when the user typed faster than the debounce/navigation cycle
• Adds a typingAhead ref that gates the URL→state sync effect: while the user has typed ahead of the last resolved URL, incoming urlQ changes from async navigation do not overwrite local input state
• Adds 3 tests covering the race condition, external URL sync, and external clear

Root cause

The sync effect (useEffect on urlQ) unconditionally called setQ(urlQ) whenever the URL changed. If the user typed "foobar" while a navigation for "foo" was still resolving, the effect would fire with urlQ = "foo" and reset the input to "foo", losing the extra characters.

Test plan

• pnpm test passes (322 tests)
• Manually: type quickly in the search box and confirm input is never truncated mid-word

🤖 Generated with Claude Code

[github-actions]

Claude finished @joeysbase's task in 1m 43s —— View job

---

Code Review in Progress

Todo List

• Read .github/code_review.md for review guidelines
• Read CLAUDE.md for project conventions
• Examine the full diff (git diff origin/main...HEAD)
• Read complete changed files for context
• Perform cross-file consistency checks
• Conduct "what's missing" analysis
• Apply design-level scrutiny
• Format and post review findings

[uipreliga]

Review: coder_eval — pr:13

Scope: pr:13 · branch joey/search-bar-fix · 9037ce5 · 2026-07-10T18:41Z · workflow variant

Change class: complex — reworks the URL-sync effect with a typingAhead useRef to fix an async race between debounced navigation and in-progress typing; correctness requires reasoning about React effect ordering and concurrency

coder_eval is in excellent shape (9.9/10) with clean type safety, security, and eval-harness axes; the only real risks are confined to one frontend file (evalboard/app/_components/search-box.tsx), where a sub-300ms debounce race can silently overwrite an external q navigation and the search-clear fix ships without a direct test — none of which affect a task's score or final_status, so the bottom line is ship-ready with minor UI-correctness and test-coverage follow-ups.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	10 / 10	0	0	0	0	—
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	9.5 / 10	0	0	1	0	PR's fixed behavior (clear-button path and typingAhead latch-release recovery branch) lacks test coverage
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	9.9 / 10	0	0	0	1	Two effects coordinate through a non-reactive typingAhead ref with 3 scattered write sites + 1 remote read — latent implicit coupling
6. Error Handling & Resilience	9.8 / 10	0	0	0	2	Debounce write-effect captures stale pre-update q during external navigation, dropping/overwriting the external value
7. API Surface & Maintainability	10 / 10	0	0	0	0	—
8. Evaluation Harness Quality	10 / 10	0	0	0	0	—

Overall Score: 9.9 / 10 · Weakest Axis: Test Health at 9.5 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 1 · 🔵 3 across 8 axes.

Blockers

None.

Non-blocking, but please consider before merge

1. [Axis 3] PR's fixed behavior (clear-button path and typingAhead latch-release recovery branch) lacks test coverage (evalboard/app/_components/__tests__/search-box.test.tsx:1) — The PR (commit 0cd9aa8 'fix(evalboard): fix search bar clear issue') exists to stop a resolving navigation from repopulating a just-cleared input, but no test clicks the clear button onClick={() => setQ("")} (search-box.tsx:71). Test 1 reproduces a type-ahead race starting from empty ('foo'→'foobar'), and test 3 clears the input via an EXTERNAL URL change (navState.q = ""), not the user-initiated clear. The actual fixed scenario — settled state (q === urlQ === 'foo') → user clicks clear (setQ('')) → typingAhead latches true → a stale navigation reporting q='foo' re-renders and must NOT repopulate the box — is never exercised. Add a test that renders with q='foo', fires a click on the 'Clear search' button, then rerenders with navState.q still 'foo' and asserts the input stays empty; this directly covers the branch the PR ships to fix.

Nits

1. [Axis 5] Two effects coordinate through a non-reactive typingAhead ref with 3 scattered write sites + 1 remote read — latent implicit coupling (evalboard/app/_components/search-box.tsx:24) — The URL→state sync effect gates on typingAhead.current (read at line 30: if (typingAhead.current) return;), while the state→URL debounce effect owns the flag across three separate mutation sites — cleared on catch-up (typingAhead.current = false; line 37), set on divergence (typingAhead.current = true; line 40), and cleared again when the debounce timer fires (typingAhead.current = false; line 42). The invariant 'typingAhead is true iff local input is ahead of the URL' is thus spread across two effects with no compiler or dependency-array enforcement, since a ref is non-reactive. A future edit to the debounce effect (adding a branch, changing the guard, or moving the timer clear) can drop or misplace one assignment and silently re-break the very sync-clobber race this change fixes, with no test failure at the edit site. This is the appropriate and arguably necessary pattern (a value-only comparison of q vs urlQ cannot distinguish 'user typed ahead' from an external URL change, both of which produce a transient divergence), and it is well-commented and localized — so this is a minor cohesion note, not a redesign ask. If cheap, centralize the flag transitions or document the three-site invariant as one block comment near the ref declaration so the coupling is discoverable from a single place.
2. [Axis 6] Debounce write-effect captures stale pre-update q during external navigation, dropping/overwriting the external value (evalboard/app/_components/search-box.tsx:30) — Effect 1's comment (lines 26-27) claims it syncs the input on external navigation: "Sync local state when the URL changes externally (back/forward, link clicks)". But while a debounce is pending, typingAhead.current is true, so if (typingAhead.current) return; (line 30) suppresses that sync, AND effect 2 re-runs with the stale trimmed still != the new urlQ, keeping typingAhead true and arming a fresh timer whose callback (lines 41-51) unconditionally re-writes params.set("q", trimmed) from the old typed value. Net effect: if a user types "foo" and within ~300ms triggers a real q-changing navigation — browser Back/Forward, or run-view.tsx clearAll which does params.delete("q") (run-view.tsx:238) — that navigation is silently lost and the URL is forced back to "foo". The window.location.search live-read (line 44) only preserves OTHER params (tag/window); it does not protect an external write to q itself. This is a narrow (<=300ms), timing-dependent window and arguably an intended "active typing wins" tradeoff, but it is a remaining race window that contradicts the stated back/forward-sync behavior, so flagging it as informational. Consider comparing the pending trimmed against the freshly-read live q before overwriting, or documenting that back/forward is intentionally forfeited during active typing.
3. [Axis 6] Cleanup comment's reasoning is incomplete about when effect-2 cleanup fires (evalboard/app/_components/search-box.tsx:53) — The comment at lines 53-54 justifies not resetting the ref in cleanup: "Don't clear typingAhead in cleanup — the timer was cancelled because the user typed another character, so they're still ahead of the URL." But effect 2's cleanup (return () => clearTimeout(timer);, line 55) also fires when any of urlQ, pathname, or router change (the effect deps at line 56), not only on a keystroke (q change). The code remains correct because the effect body always re-runs after cleanup and re-evaluates typingAhead (resetting it at line 37 when trimmed === urlQ), but the stated reason ("cancelled because the user typed another character") is only one of several triggers and could mislead a future maintainer into assuming the ref lifecycle is coupled to keystrokes alone. Reword to state that cleanup may run on any dep change and the body re-run is what re-establishes the correct typingAhead value.

What's Missing

Tests:

• 🟡 The PR's named fix — the user-initiated clear path (setQ("") at search-box.tsx:71) and the typingAhead latch-release branch (search-box.tsx:37/42) — has no direct test: no test fires a click on the "Clear search" button, and none asserts the latch RELEASES after a typing-ahead episode so a later genuine external navigation re-syncs. Add (a) a test that renders q='foo', clicks Clear, rerenders with navState.q still 'foo', asserts the input stays empty; and (b) a test that after the debounce fires (typingAhead=false) a subsequent external nav does update the input, guarding against a stuck-true latch. _(trigger: evalboard/app/components/tests/search-box.test.tsx) (restates: Axis 3: PR's fixed clear-button + latch-release branch lacks test coverage)
• 🔵 No test pins the <300ms debounce-window collision where the user types then an external q write fires (browser Back/forward, or run-view.tsx:236 clearAll's params.delete("q")). The branch that currently forfeits the external q value (search-box.tsx:30 guard + re-armed stale-trimmed timer) is unexercised, so whichever behavior is intended ("active typing wins") is undocumented and a future change to that tradeoff would fail silently. _(trigger: evalboard/app/components/search-box.tsx) (restates: Axis 6: Debounce window drops external q navigation during active typing)

Parallel paths:

• 🔵 The new typingAhead latch lives only in SearchBox, but q is also written by parallel paths that were not reconciled with it — run-view.tsx:236 clearAll (params.delete("q")) and tag-rail.tsx hrefForTag. Those writers use the "read live URL at commit" pattern for their own params, but none coordinates with the latch on an external q delete during active typing, so the latch's invariant is only half-owned across the q-writer set. _(trigger: evalboard/app/components/search-box.tsx) (restates: Axis 6: Debounce window drops external q navigation during active typing)

Daily/nightly:

• 🔵 Blast radius is confined to the evalboard Next.js UI: the change touches only client-side URL-param sync and does not alter the run path, the task.json/run-record schema, report JSON shape, or CLI output — the cross-repo contract consumed by the coder-eval-uipath/eval-runner nightly pipeline. The only gap is that the PR does not state this explicitly; no container image or persisted-schema rebuild is required. _(trigger: evalboard/app/components/search-box.tsx)

Harness & Lint Improvements

Static checks (lint / type):

• (none)

Harness improvements (not statically reachable):

• Add a vitest regression test to search-box.test.tsx that drives the user-initiated clear path the PR is named for: render with q='foo' (settled: q===urlQ), fireEvent.click the 'Clear search' button (search-box.tsx:71 setQ('')), then rerender with navState.q still 'foo' and assert the input stays empty. This exercises the exact typingAhead latch-release recovery branch the fix ships; the current 3 tests only hit a type-ahead race from empty and an external URL clear, so the shipped branch is undriven. Why not static: Requires driving a click + rerender and asserting over the resulting React render output (input value stays empty) — a behavioral assertion on rendered state, not a grep/AST-detectable pattern. Also outside the four Python-only static-check kinds, since the file is TypeScript. Prevents: A3 (medium) — clear-button / typingAhead latch-release recovery branch has no test coverage.
• Add a fake-timer vitest regression test for the debounce-vs-external-navigation race: type 'foo', then within the ~300ms debounce window trigger a real q-changing external nav (navState.q change or params.delete('q') as run-view.tsx clearAll does), advance timers, and assert the pending debounce callback does NOT force the URL back to 'foo'. This pins the currently-unguarded timing window where the stale closure overwrites the external value. Why not static: Needs fake-timer control plus observation of the debounced timer callback's URL write over time — a runtime timing behavior, not a static code shape. TypeScript file, also outside the allowed Python static-check kinds. Prevents: A6 / A1 (low) — debounce write-effect captures stale pre-update q and overwrites external navigation within the debounce window.
• Wire a JS-side lint/typecheck gate for evalboard into make verify / CI: add eslint with react-hooks (rules-of-hooks + exhaustive-deps) plus tsc --noEmit, run on the evalboard package. Today evalboard ships only vitest/tsconfig with no eslint config, so effect-dependency and hooks mistakes in .tsx have zero mechanical gate — which is why the ref-coupling, stale-closure, and stale-comment findings all landed in an ungated file. This also gives a home for a future custom hooks/effect-cleanup rule. Why not static: The repo's static gates (ce-lint, ruff, pyright, bandit) are all Python-AST/Python-only and structurally cannot parse .tsx; the correct mechanical backstop is a JS-toolchain lint gate that lives outside the four allowed static-check kinds. It is therefore a CI-wiring / toolchain gap, not a check expressible as ce-lint/ruff/pyright/bandit. Note: exhaustive-deps would not directly catch the ref-based coupling (A5) or comment-accuracy (A7) issues — those remain non-mechanical cohesion/doc notes — but the gate catches the broader hooks-dependency class and is the durable home for it. Prevents: Motivated by A5, A6, A1, A7 all landing in a .tsx file with no static gate; directly backstops the hooks-dependency class (A6/A1) and provides enforcement infrastructure evalboard currently lacks.

Top 5 Priority Actions

1. Close the debounce stale-q overwrite race in search-box.tsx:30/41-51 by comparing the pending trimmed against a freshly live-read q before params.set("q", ...), so a back/forward or run-view.tsx:238 clearAll (params.delete("q")) firing within ~300ms of typing is not silently reverted to the old value.
2. Add the missing clear-button test in evalboard/app/_components/tests/search-box.test.tsx that renders with q='foo', fires a click on the 'Clear search' button (search-box.tsx:71 onClick={() => setQ("")}), then rerenders with navState.q still 'foo' and asserts the input stays empty — directly covering the branch commit 0cd9aa8 was named to fix.
3. Reduce the implicit coupling of the non-reactive typingAhead ref (search-box.tsx:24) by centralizing its three write sites (lines 37/40/42) or documenting the 'true iff local input is ahead of the URL' invariant as one block comment at the ref declaration so a future edit cannot silently re-break the sync-clobber fix.
4. Correct the incomplete cleanup comment at search-box.tsx:53-54 to state that effect-2 cleanup fires on any of its urlQ/pathname/router dep changes (not only a keystroke) and that the effect-body re-run is what re-establishes the correct typingAhead value.
5. Explicitly document the intended 'active typing wins over back/forward' tradeoff near the debounce timer callback (search-box.tsx:44) if the residual race window is accepted, so the behavior no longer contradicts effect 1's stated external-sync comment (lines 26-27).

---

Stats: 0 🔴 · 0 🟠 · 1 🟡 · 3 🔵 across 8 axes reviewed.