Repository navigation
fix(app): open a newly saved search without reloading the page - #3565
Conversation
Saving a new search left the page through window.location, which reloaded the whole app and, on the static ClickStack export, 404s because /search/<id> has no exported document. That reload was a workaround for nuqs flushing a queued query-string write onto the router's pre-transition path, which sent the client-side navigation back to /search. Pause live tail while the save modal is open and drain the nuqs queue before pushing, so the push can stand.
… page The sessions page writes its default time range to the URL as a pushed history entry shortly after mount, so a single Back landed on either the saved search or bare /sessions depending on which won.
🦋 Changeset detectedLatest commit: 31e5fcc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ), | ||
| ); | ||
| await onCreate(savedSearch.id); | ||
| onClose(); |
There was a problem hiding this comment.
🔵 minor — After a successful create, the modal closes without resetting the form, so the typed name is still there next time it opens
Call closeAndReset() instead of onClose() after await onCreate(...). Before this PR, the full page reload threw away the modal's react-hook-form state. Now SaveSearchModal stays mounted, and in create mode useForm gets no values. So if the user clicks "Save as new" on the new saved-search page (onClickSaveAsNew → setSaveSearchModalState('create')), the name field is pre-filled with the previous search's name.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useMe: () => ({ data: { team: {} }, isSuccess: true }), | ||
| }, | ||
| useCompleteOnboardingTask: () => ({ mutate: jest.fn() }), | ||
| })); |
There was a problem hiding this comment.
🔵 minor — The drain-before-push test mocks nuqs, so it never checks the behaviour the race fix depends on
The test only checks that setSearchedConfig({}) is called before router.push. It doesn't check that setSearchedConfig({}) actually returns the pending-flush promise in nuqs 1.17. Because useQueryStates is mocked to resolve right away, the test would still pass if an empty update were a no-op that resolved before the queue flushed. Add a test that uses the real nuqs useQueryStates with a router adapter, queues a write (for example from/to), and asserts that the write is applied before the push. Otherwise, treat the E2E test as the only real coverage.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. The change is a well-reasoned, mechanically sound fix. Navigation moves from a full-page 🟡 P2 -- recommended
🟡 P2 -- recommended (testing)
🔵 P3 nitpicks (2)
Reviewers (7): correctness, adversarial, frontend-races, typescript, testing, maintainability, project-standards. Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results✅ All tests passed • 462 passed • 1 skipped • 1503s
Tests ran across 4 shards in parallel. |
Summary
Saving a new search left the page through
window.location.assign, so the whole app reloaded. On the static ClickStack export that reload is worse than slow: the export only emitssearch/[savedSearchId].html(the literal bracket filename, confirmed by runningyarn build:clickhouse), so a document request for/clickstack/search/<id>has nothing to serve and the user lands on a 404 instead of their new search.The reload was added in #2669 to work around a real race, not out of preference. nuqs 1.17 batches query-string writes and, at flush time, renders them onto
next/router'sasPath— which during a client-side transition is still the old path. Live tail rewritesfrom/tothrough nuqs every 10s withhistory: 'push', so a flush landing mid-transition pushed/search?...over the in-flight navigation to/search/<id>and the user bounced back to an unsaved search.This removes the workaround and closes the race at both ends instead:
await setSearchedConfig({})) beforerouter.push, so any write already queued is applied before the transition starts rather than after it.Navigation moves to the page component, which is where the query-state setter lives; the modal reports the new id through an
onCreateprop.buildSavedSearchNavigationUrlno longer takesbasePath, sincerouter.pushprepends it.Screenshots or video
Not a visual change — the diff is in how the page gets to
/search/<id>. This is the new E2E test recorded in slow motion: the modal closes and the header switches to the saved search with no white flash and no sidebar re-mount.How to reproduce
Starting point: full-stack
yarn dev(the save flow needs the API), on/searchwith live tail on, which is the default.On
main:On this branch:
How to test on Vercel preview
N/A — the preview runs in local mode, where saved searches live in localStorage and the reload is survivable; the failure this fixes needs the static ClickStack export or a full-stack deploy.
Tests
DBSearchPage.saveSearch.test.tsx): saving pushes/search/<id>through the router (basePath-free), and the queue drain is ordered before the push.document.documentElementsurvives the save, which it cannot across a document load.buildSavedSearchNavigationUrlunit tests updated for the droppedbasePath, plus a case for a live-tail range.should handle navigation via browser back buttonE2E test was racing the sessions page, which writes its default time range as a pushed history entry shortly after mount — one Back landed on the saved search or on bare/sessionsdepending on which won. It now waits for that write before going back. This is a pre-existing flake that the faster client-side save made easier to hit; the app-side wart (a junk history entry per page load) is untouched.make ci-lintandmake ci-unitpass. Thesaved-searchE2E file passes 40/40 over two repeat runs.References
Fixes HDX-5916