Pre-push review: T-1960/bugfix-search-testing-debounce-cancel

What Changed

Five lines moved around inside a helper that only exists when the app is built for testing, plus one new test and a one-line fix to an old test.

The app's search box does not search on every keystroke — that would be wasteful. Instead it waits 200 milliseconds after you stop typing, then searches. That pause is called a debounce.

Tests do not want to wait 200 ms, so there is a shortcut helper that jams a search query straight in. The helper tried to be tidy by cancelling any pending 200 ms timer first, and only then setting the query. But setting the query is exactly what starts a new timer. So the helper cancelled an old timer and then immediately lit a new one it never cleaned up.

Why It Matters

That stray timer went off about 200 ms later and wiped out which search result was currently selected. Any test that called the helper and then did anything slow — waiting for a web view to load, say — would find its selected match mysteriously gone.

The cruel part: the number of matches survived, only the selection vanished. So a broken test looked like the search feature was broken, not the test helper. Engineers went looking in the wrong place.

Key Concepts

• Debounce — wait for a lull before acting, so rapid input triggers one action, not many.
• Property observer (didSet) — Swift lets you attach code that runs automatically every time a value is assigned. Handy, but it means an innocent-looking assignment can start real work.
• Order of operations — the entire fix is putting the cancel after the assignment instead of before. Same five lines, different order, bug gone.

Architecture

SearchCoordinator separates the raw input (searchQuery, bound to the UI) from the applied query (activeSearchQuery, which drives highlighting). A didSet on searchQuery calls scheduleSearchUpdate(), which cancels the previous searchDebounceTask, then stores a new Task that sleeps 200 ms and applies the query.

The DEBUG-only setActiveSearchQueryForTesting(_:) bypasses the wait so tests stay synchronous. Its old body was:

searchDebounceTask?.cancel()   // kills a task from an EARLIER call
searchDebounceTask = nil
searchQuery = query            // didSet -> schedules a NEW task
activeSearchQuery = query
currentGlobalMatchIndex = nil

The cancel guarded the wrong task. Every call left one live task behind.

Patterns

This is the classic hazard of coupling side effects to property observers: a caller cannot assign the property without also triggering the work, so any cleanup must be sequenced after the assignment, not before. The fix therefore reads as an ordering constraint, and the diff documents it in place so the next reader does not "tidy" the cancel back to the top.

The cancel is deliberately kept unconditional. In the same-query case the assignment is a no-op (Swift's didSet fires, but the guard if searchQuery != oldValue suppresses scheduling), so no new task exists — yet a task from an earlier direct searchQuery write may still be pending, and only an unconditional cancel kills it.

Trade-offs

The chosen shape is ordering-dependent, which is inherently a little fragile. Three alternatives were weighed and rejected: a shared private applyQuery (doesn't fix anything — the bug is the searchQuery write itself, which the helper must perform because production UI reads that property); a private-storage split behind a computed searchQuery (needs manual @Observable withMutation/access plumbing on a hot production property to serve a DEBUG helper); and a generation counter checked in the task body (would make ordering irrelevant, but adds production state to protect two lines of a test seam).

What makes the ordering hack tolerable is that the new regression test pins both implicit dependencies — MainActor inheritance and the guard !Task.isCancelled check. Delete either and the test goes red.

Deep dive: why the reorder is deterministic

Two properties make this safe rather than lucky.

1. No suspension point between creation and cancellation. scheduleSearchUpdate() is a synchronous @MainActor method. The unstructured Task {} it creates inherits MainActor isolation, so its body is enqueued on the main actor and cannot begin executing while the current synchronous run — which continues into the helper's .cancel() two lines later — still holds the actor. The window in which the task could observe uncancelled state does not exist.

2. Cancellation is sticky and checked twice. Even were the task to start, it suspends immediately on try? await Task.sleep(for: .milliseconds(200)), which throws CancellationError on an already-cancelled task; the try? swallows it and the subsequent guard !Task.isCancelled else { return } catches the residual path. Neither activeSearchQuery nor currentGlobalMatchIndex is reachable post-cancel.

State-path equivalence

All four paths were traced in both orderings, with SearchService.minimumQueryLength == 2:

• Differing, ≥2 chars — schedule (resets hasAnnouncedResultsForQuery, writes nothing) → activeSearchQuery = query → recomputeMatchCounts (writes matchCountsPerBlock, clamps, may announce) → cursor nil. Identical; old order additionally leaves a live task.
• Differing, 1 char — scheduleSearchUpdate early-returns via the guard newQuery.count >= minimumQueryLength path, setting activeSearchQuery = "", matchCountsPerBlock = [], cursor nil; the helper then writes activeSearchQuery = query, whose didSet recompute clears again. Identical in both orders.
• Empty query — same early return; the helper's second activeSearchQuery = "" is suppressed by the inequality guard. Identical.
• Same query — didSet body guarded, no schedule, no flag reset, no recompute; only the cursor is nil'd. Identical, plus the leftover task is now killed.

Crucially, hasAnnouncedResultsForQuery = false is set inside scheduleSearchUpdate in both orderings, and in both it lands before the activeSearchQuery write that triggers recomputeMatchCounts — so the VoiceOver announce decision is unchanged. The single observable delta is the elimination of the late currentGlobalMatchIndex = nil.

Edge cases and residual risk

searchDebounceTask = nil is now meaningful where before it was theatre: in the old position it was immediately overwritten with a live task by scheduleSearchUpdate, so the field's value carried no information. In the new position it establishes the invariant the docstring advertises and mirrors clearSearch().

Production paths do not share the trap: clearSearch() assigns searchQuery = "", which takes the same below-minimum early return and schedules nothing. This is genuinely a harness-only defect with no missing production counterpart.

The one residual soft spot is the new test's 400 ms sleep against a hard-coded 200 literal. Post-fix nothing mutates state, so the test cannot flake red — the only load-induced failure mode is the opposite, the pre-fix task not having fired inside 400 ms, i.e. the regression test silently losing its teeth. The same risk applies if the debounce interval is ever raised above 400 ms. Hoisting 200 into a static let and sleeping 2 × it would close that, and is the recommended follow-up rather than a gate.