Search bar performance improvements#4072
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a ChangesShallow routing and SearchBox URL self-management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
general-adhoc
force-pushed
the
fix-search-enter-debounce
branch
from
a7278fc to
9f3dc69
Compare
general-adhoc
force-pushed
the
fix-search-enter-debounce
branch
from
9f3dc69 to
eb6fc81
Compare
|
@coderabbitai full review any regressions? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the PR diff summary
gh pr view 4072 --repo dubinc/dub --json files,additions,deletions,changedFiles
echo "=== PR Files Changed ==="
gh pr diff 4072 --repo dubinc/dub --name-only
echo "=== Full Diff ==="
gh pr diff 4072 --repo dubinc/dubLength of output: 18111
Overall AssessmentThe PR correctly addresses both root causes (debounce flush on Enter, RSC round-trips). The fixes are well-reasoned and the new test suite validates the core invariant. A few things worth reviewing before merging: 🔴 Potential Regression:
|
| Area | Status |
|---|---|
| Enter key flushes debounce | ✅ Correct |
| Clear cancels pending debounce | ✅ Correct |
| Operator precedence bug fixes | ✅ Genuine fixes |
Back/forward loop fix (setDebouncedValue) |
✅ Genuine fix |
| Shallow routing skips RSC round-trip | ✅ Works (Next.js patches pushState) |
scroll: false silently ignored with shallow: true |
|
| Global no-op guard behavioral change | |
Removed onChangeDebounced from SearchBoxPersisted |
|
| Test coverage | ✅ Covers the key invariants |
✅ Action performed
Full review finished.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ui/src/hooks/use-router-stuff.ts (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or enforce that
scrollis ignored in shallow mode.The early return on Lines 100-109 skips the scroll-preservation branch entirely, so callers can now pass
{ scroll: false, shallow: true }and get different semantics from the router path. Either handle scroll before the shallow return or make these options mutually exclusive to avoid a misleading API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/hooks/use-router-stuff.ts` around lines 99 - 109, The shallow-navigation branch in use-router-stuff’s router helper currently returns before the scroll-preservation logic, so scroll is silently ignored when shallow is true. Update the behavior in the same routing path so that `scroll` is either explicitly handled before the shallow early return or documented/enforced as incompatible with shallow mode, and make sure the semantics in the router helper match the public API expectations for `scroll` and `shallow`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/tests/ui/use-router-stuff.test.ts`:
- Around line 29-39: The renderQueryParams helper mounts a React root via
createRoot but never cleans it up, which can leak mounted trees across tests.
Update renderQueryParams to keep a reference to the root returned by createRoot,
and make sure it is unmounted and the container is removed after use; use the
Probe component and useRouterStuff hook as the key places to locate the setup.
If needed, wrap the helper’s mounting logic so cleanup runs reliably even when
assertions fail.
---
Nitpick comments:
In `@packages/ui/src/hooks/use-router-stuff.ts`:
- Around line 99-109: The shallow-navigation branch in use-router-stuff’s router
helper currently returns before the scroll-preservation logic, so scroll is
silently ignored when shallow is true. Update the behavior in the same routing
path so that `scroll` is either explicitly handled before the shallow early
return or documented/enforced as incompatible with shallow mode, and make sure
the semantics in the router helper match the public API expectations for
`scroll` and `shallow`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19177d4d-c7b9-465c-8aeb-a2b4b88e108d
📒 Files selected for processing (8)
apps/web/app/(ee)/admin.dub.co/(dashboard)/partners/network/page.tsxapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/program/campaigns/[campaignId]/campaign-events-modal.tsxapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/domains/page-client.tsxapps/web/app/app.dub.co/(dashboard)/[slug]/links/folders/page-client.tsxapps/web/app/app.dub.co/(dashboard)/[slug]/links/tags/page-client.tsxapps/web/tests/ui/use-router-stuff.test.tsapps/web/ui/shared/search-box.tsxpackages/ui/src/hooks/use-router-stuff.ts
| function renderQueryParams() { | ||
| let api: ReturnType<typeof useRouterStuff> | undefined; | ||
| function Probe() { | ||
| api = useRouterStuff(); | ||
| return null; | ||
| } | ||
| const container = document.createElement("div"); | ||
| document.body.appendChild(container); | ||
| flushSync(() => createRoot(container).render(createElement(Probe))); | ||
| if (!api) throw new Error("hook did not render"); | ||
| return api; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unmount the React root to keep tests isolated.
renderQueryParams creates and mounts a root but never unmounts/removes it. Over multiple tests, this leaks mounted trees and can cause cross-test interference once the hook grows effects.
Suggested fix
function renderQueryParams() {
let api: ReturnType<typeof useRouterStuff> | undefined;
function Probe() {
api = useRouterStuff();
return null;
}
const container = document.createElement("div");
document.body.appendChild(container);
- flushSync(() => createRoot(container).render(createElement(Probe)));
+ const root = createRoot(container);
+ flushSync(() => root.render(createElement(Probe)));
if (!api) throw new Error("hook did not render");
- return api;
+ return {
+ ...api,
+ cleanup: () => {
+ root.unmount();
+ container.remove();
+ },
+ };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/tests/ui/use-router-stuff.test.ts` around lines 29 - 39, The
renderQueryParams helper mounts a React root via createRoot but never cleans it
up, which can leak mounted trees across tests. Update renderQueryParams to keep
a reference to the root returned by createRoot, and make sure it is unmounted
and the container is removed after use; use the Probe component and
useRouterStuff hook as the key places to locate the setup. If needed, wrap the
helper’s mounting logic so cleanup runs reliably even when assertions fail.
1 participant
Improve global search bar performance
Summary
I noticed the search bars felt laggy across multiple screens (originally partner network search). Root caused it to the search bar component which impacted all 20 usages of
search-box.tsx(19 ofSearchBoxPersistedone ofSearchBoxdirectly)Separating this out from the partner network search PR as this has a larger potential blast radius than just partner search.
Two Problems
SearchBoxPersistedviews render results from client SWR fetches.Note: several components were worse with additional non-shallow write (tags, folders, domains, and admin network)
This caused an up to 500ms debounce delay + server round trip for RSC re-render BEFORE the client would begin fetching data, leading to noticeable lag.
Reproduce
Solution
shallow?: booleanon the sharedqueryParamshook - this updates URL via history API instead of router. This avoids triggering navigation, RSC round trips. 'useSearchParams' still reacts and SWR refetches.SearchBoxPersistedopts in to shallow by default. This is safe across all current usages.onChangeDebouncedhandlersshallowwrite uses history api and not routerqueryParamsskips when resulting URL is unchanged - prevent wasted same-URL navigation. This should be safe, want to call out.Summary by CodeRabbit
New Features
Bug Fixes
Tests