Fix saved search navigation race#2669
Conversation
🦋 Changeset detectedLatest commit: 14aa5f6 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 |
|
@Sakshamm-Goyal is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes saved-search navigation after creating a saved search. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "fix(app): preserve absolute saved search..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep Review✅ No critical issues found. The change replaces a client-side 🟡 P2 -- recommended
🟡 P2 -- recommended (behavioral edge)
🔵 P3 nitpicks (2)
Reviewers (2 of 8 returned before synthesis was forced): maintainability, agent-native. Correctness, testing, project-standards, kieran-typescript, julik-frontend-races, and learnings-researcher were dispatched but had not returned when structured output was enforced; findings above are grounded in the orchestrator's direct read of the diff and the two completed reviewers. Agent-native review found no parity gap (the MCP Testing gaps:
|
eb218eb to
f2c86f1
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a flaky “save saved-search → navigate to /search/<id>” race in the Next.js app by switching the create flow to a hard document navigation, preventing useQueryStates from restoring stale /search?... URL state during the transition. It also updates Playwright E2E helpers/assertions to match the new navigation behavior and adds a changeset for the app patch release.
Changes:
- Switch saved-search create success handling from
router.push(...)towindow.location.assign(...), preserving only select URL state. - Update the E2E saved-search modal helper to wait for URL navigation at
domcontentloaded. - Extend an E2E test to validate the saved-search route loads stored config (and not config via URL params), and add an app changeset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/app/src/DBSearchPage.tsx | Replace client-side transition with hard navigation to prevent stale useQueryStates URL restoration during saved-search creation. |
| packages/app/tests/e2e/components/SavedSearchModalComponent.ts | Adjust waitForURL to stop waiting on full load for document navigations. |
| packages/app/tests/e2e/features/search/saved-search.spec.ts | Add assertions about query params and restored saved-search config after navigation. |
| .changeset/fix-saved-search-navigation-race.md | Patch changeset for @hyperdx/app. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const currentParams = new URLSearchParams(window.location.search); | ||
| const timeRangeParams = new URLSearchParams(); | ||
| const from = currentParams.get('from'); | ||
| const to = currentParams.get('to'); | ||
| if (from != null && to != null) { | ||
| timeRangeParams.set('from', from); | ||
| timeRangeParams.set('to', to); | ||
| } |
| // Start waiting for URL change BEFORE clicking submit to avoid race condition | ||
| const urlPromise = this.page.waitForURL(/\/search\/[a-f0-9]+/, { | ||
| timeout: 15000, | ||
| // Saving now uses a document navigation to avoid useQueryStates restoring | ||
| // stale search URL. Do not wait for unrelated resources to finish loading. | ||
| waitUntil: 'domcontentloaded', | ||
| }); | ||
|
|
||
| await this.submit(); | ||
|
|
||
| // Wait for navigation to complete | ||
| await urlPromise; | ||
|
|
||
| // Wait for modal to fully close | ||
| await expect(this.container).toBeHidden(); | ||
|
|
||
| await expect(this.savedSearchNameTitle).toBeVisible({ timeout: 5000 }); | ||
| await expect(this.savedSearchNameTitle).toHaveText(name, { timeout: 5000 }); | ||
| } |
| const savedSearchParams = new URL(page.url()).searchParams; | ||
| expect(savedSearchParams.has('from')).toBe(true); | ||
| expect(savedSearchParams.has('to')).toBe(true); | ||
| expect(savedSearchParams.has('where')).toBe(false); | ||
| expect(savedSearchParams.has('orderBy')).toBe(false); |
Summary
Fixes the saved-search save-to-navigation race reported in #2644.
The create flow now performs a document navigation to the new saved-search
route rather than a client-side transition that can be overwritten by stale
useQueryStatesURL state. The E2E helper waits fordomcontentloadedso itasserts the destination route without coupling the test to unrelated resource
loading after that document navigation.
Root cause
router.pushallowed the prior search page's URL state to be restored duringthe transition, intermittently replacing the new
/search/<id>route.Validation
yarn lint:fixThe focused full-stack E2E runner is currently blocked before its test by an
existing global setup timeout waiting for
page.goto('/search')to emitload; this is unrelated to the saved-search route behavior.