fix(theme-store): let only the newest theme search write its results - #750
Conversation
Two stale writers reach `results`/`error`/`loading` in ThemeStoreDialog, both of them searches that have already started, which the existing debounce cancel cannot stop: - A request still in flight when the store closes resolves afterwards and repopulates the list the close's reset effect just cleared. The component only renders null while closed, so the state survives: reopening shows the previous query's rows under the empty-state line. - An earlier query whose response lands after a later one's overwrites the newer results, so the list no longer matches the box. One epoch ref, bumped on every search and on every close, gates all three state writes on still being the newest search.
Deploying mouseterm with
|
| Latest commit: |
e647f86
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e4a8e929.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-theme-store-stale-search.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark the PR ready when you want the full review.
The epoch gate is the right mechanism and both described races are closed by it, but moving the bump above the !value.trim() early return leaves one exit where loading is never cleared: emptying the search box while a request is in flight invalidates that request's finally, and nothing else in that path turns the spinner off. The dialog then sits on "Searching..." with an empty box — the Search for a VS Code theme to install empty state is gated on !loading, so it stays suppressed until the next completed search or a close/reopen. On main the same sequence instead repopulated the cleared box with the old query's rows, so this path swaps one wrong state for another rather than being fixed; it's worth a third test alongside the two you added. Inline suggestion below.
The epoch is taken before the empty-query early return, so `doSearch('')`
supersedes an in-flight request and gates off its `setLoading(false)` —
and that branch returned without touching `loading`, leaving the dialog
on "Searching..." with an empty box until the next completed search.
The empty-query path is the newest search, so it clears the spinner itself.
2 participants
ThemeStoreDialogapplies a theme search's result whether or not anyone is still waiting for it. Three searches can be stale by the time they resolve, and all three have already leftsearchThemes, so the debounce cancel added in #295 — which only stops searches that have not started — reaches none of them:nullwhile closed but never unmounts, so the latesetResultssticks. Reopening renders the previous query's rows underneath "Search for a VS Code theme to install", because the empty-state line is gated onquery(which the reset cleared) while the rows are gated onresults(which the late response refilled).loadinghas to be cleared by the path that invalidated it, or the dialog sits on "Searching..." above an empty box.All three are closed by one
searchEpochref, bumped on every search and every close.doSearchcaptures its epoch and gates all three state writes —setResults,setError,setLoading— on still holding the newest one; the empty-query exit clearsloadingitself, since it is the newest search. That is a smaller mechanism than anAbortControllerper request and covers the close case, which aborting the fetch alone would not: by then the response can already be in the microtask queue.Verification. Three tests added to the existing
ThemeStoreDialog.test.tsx, resolving mockedsearchThemespromises by hand so the ordering is explicit. Each fails without the fix with the symptom described —Received: "…Search for a VS Code theme to installVSDracula Officialtest - 1 downloadsInstall"for the close case, the older query's row for the ordering case,Received: "…Searching..."for the emptied box — and all pass with it.lib'stsc -bis clean and the rest of its suite is unchanged.Local suite state
The full
libvitest run is 3678 passed / 4 failed. The four aresrc/host/remote/native-direct-peer.test.ts, which negotiates a real WebRTC channel overnode-datachannel; they time out waiting for ICE in this sandbox and fail identically with the change stashed, so they are an environment limit here rather than a result of this diff. CI on both pushed commits is green.Surfaced by the nightly code-quality survey. The third race was introduced by this PR's own first commit and caught by the draft review on it;
e647f86ccloses it.