Skip to content

fix(theme-store): reset search state on close; drop unreachable dialog.close() - #295

Merged
nedtwigg merged 2 commits into
mainfrom
nightly/theme-store-reset-state
Jul 27, 2026
Merged

fix(theme-store): reset search state on close; drop unreachable dialog.close()#295
nedtwigg merged 2 commits into
mainfrom
nightly/theme-store-reset-state

Conversation

@dormouse-bot

Copy link
Copy Markdown
Collaborator

Surfaced by the nightly code-quality survey.

ThemeStoreDialog is permanently mounted by ThemePicker and only renders null while closed (if (!open) return null), so it never unmounts — its query, results, and error state persist across open/close. Reopening the store therefore flashes the previous search query, the stale result list, and any leftover error banner from the last session.

This PR:

  • Resets search state when the store closes. A new open-keyed effect clears query, results, error, and loading when open becomes false, so a reopen starts from a clean slate. This can't live in the existing showModal effect, which early-returns while closed (the <dialog> ref is null).
  • Removes an unreachable branch. The if (!open && dialog.open) dialog.close() line could never fire: if (!open) return null unmounts the <dialog> before that effect re-runs with open === false, so dialogRef.current is already null and the effect early-returns. Closing is handled by the unmount; the branch was dead code. A comment now records why only the open transition needs a native call.

Test: added ThemeStoreDialog.test.tsx (jsdom, the repo's createRoot/act pattern) pinning that a query typed before close is gone after reopen. Verified it fails on main (expected 'dracula' to be '') and passes with the fix.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 24, 2026

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: f84984a
Status: ✅  Deploy successful!
Preview URL: https://b6a1845b.mouseterm.pages.dev
Branch Preview URL: https://nightly-theme-store-reset-st.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dead-code removal is correct — if (!open) return null unmounts the <dialog> before the [open] effect re-runs with open === false, so dialogRef.current is already null and the old .close() branch could never fire.

One gap in the clean-slate goal: the reset effect clears state but leaves the pending debounce timer running. If the store is closed within ~300ms of the last keystroke, the scheduled doSearch(value) still fires while closed and repopulates results/loading for the old query — so a quick reopen shows the stale result list (and possibly the spinner) the reset was meant to prevent, even though the input box reads empty. Clearing the timer alongside the state closes that path.

// would otherwise persist — a reopen would flash the previous query, result
// list, and any error banner. Reset to a clean slate when the store closes.
useEffect(() => {
if (!open) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!open) {
if (!open) {
if (debounceRef.current) clearTimeout(debounceRef.current);

Otherwise a debounce scheduled by the last keystroke fires doSearch after close and repopulates results for the old query.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f84984a — the reset effect now clears debounceRef before resetting state, so a doSearch scheduled by the last keystroke can't fire after close. Added a fake-timers test that closes inside the 300ms window and asserts searchThemes is never called (fails without the fix).

The reset effect cleared search state on close but left the debounce timer running. A doSearch scheduled within 300ms of close fired while closed and repopulated results/loading for the old query, so a quick reopen showed the stale list (and possibly the spinner) the reset was meant to prevent. Clear debounceRef alongside the state.

Adds a fake-timers test pinning that searchThemes is not called when the store closes inside the debounce window (fails without the fix).
@nedtwigg
nedtwigg merged commit c80bb76 into main Jul 27, 2026
9 checks passed
@nedtwigg
nedtwigg deleted the nightly/theme-store-reset-state branch July 27, 2026 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants