fix: debounce URL navigation in skills search to reduce input lag#587
fix: debounce URL navigation in skills search to reduce input lag#587neeravmakwana wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
@neeravmakwana is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bf6ccaf62
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }) | ||
| window.clearTimeout(navigateTimer.current) | ||
| const trimmed = next.trim() | ||
| navigateTimer.current = window.setTimeout(() => { |
There was a problem hiding this comment.
Cancel pending query navigation on external URL changes
Scheduling navigate() in onQueryChange without any cancellation path other than another keystroke means a stale timer can overwrite user-driven navigation on the same page. If a user types and then immediately uses browser back/forward (or any other search-param navigation) within 220ms, the queued callback still runs and replaces the URL with the old q, effectively undoing that navigation and re-syncing the input to stale state.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR debounces the Key observations:
Confidence Score: 3/5
Last reviewed commit: 3bf6cca |
| useEffect(() => { | ||
| return () => window.clearTimeout(navigateTimer.current) | ||
| }, []) |
There was a problem hiding this comment.
Pending timer not cancelled on external URL change
The useEffect on line 76–78 syncs query from search.q when the URL changes externally (e.g. browser back/forward, a direct link). However, the pending navigateTimer is not cleared at that point. This means:
- User types "foo" →
setQuery("foo")fires immediately, 220 ms timer starts. - User presses browser back before the timer fires →
search.qreverts (or becomesundefined), and theuseEffectcorrectly resetsquery. - After 220 ms the pending timer fires and calls
navigate({ q: "foo" }), overwriting the browser-back navigation.
The fix is to clear the timer in the same effect that responds to external URL changes:
| useEffect(() => { | |
| return () => window.clearTimeout(navigateTimer.current) | |
| }, []) | |
| useEffect(() => { | |
| window.clearTimeout(navigateTimer.current) | |
| setQuery(search.q ?? '') | |
| }, [search.q]) |
This ensures any in-flight debounce is cancelled whenever the URL is updated from outside the component (back/forward, programmatic navigation, direct link load).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/routes/skills/-useSkillsBrowseModel.ts
Line: 221-223
Comment:
**Pending timer not cancelled on external URL change**
The `useEffect` on line 76–78 syncs `query` from `search.q` when the URL changes externally (e.g. browser back/forward, a direct link). However, the pending `navigateTimer` is not cleared at that point. This means:
1. User types "foo" → `setQuery("foo")` fires immediately, 220 ms timer starts.
2. User presses browser **back** before the timer fires → `search.q` reverts (or becomes `undefined`), and the `useEffect` correctly resets `query`.
3. After 220 ms the pending timer fires and calls `navigate({ q: "foo" })`, **overwriting the browser-back** navigation.
The fix is to clear the timer in the same effect that responds to external URL changes:
```suggestion
useEffect(() => {
window.clearTimeout(navigateTimer.current)
setQuery(search.q ?? '')
}, [search.q])
```
This ensures any in-flight debounce is cancelled whenever the URL is updated from outside the component (back/forward, programmatic navigation, direct link load).
How can I resolve this? If you propose a fix, please make it concise.onQueryChange called navigate() on every keystroke to sync the query to the URL. Each navigate triggers history.replaceState, TanStack Router re-evaluation, and useSearch() invalidation, causing multiple re-renders per keystroke. Keep setQuery() immediate so the controlled input stays responsive, but debounce the navigate() call at 220 ms (matching the existing search-action debounce). Cancel the pending timer when search.q changes externally (browser back/forward) to prevent the debounced navigate from overwriting the external URL change. Made-with: Cursor
3bf6cca to
ec3e1b7
Compare
|
Updated — addressed the race condition identified in review: the Also moved the |
Summary
navigate()call inonQueryChange(skills browse page) so the URL is not updated on every keystrokesetQuery()remains immediate so the controlled input stays responsiveuseEffectProblem
Typing in the skills search input on
/skillscauses noticeable lag. Each keystroke callsnavigate({ search: … })synchronously, which triggershistory.replaceState, TanStack Router re-evaluation, anduseSearch()invalidation — resulting in multiple React re-render cycles per keystroke on top of thesetQueryrender.The Convex search action was already debounced at 220 ms, but the
navigatecall was not.Test plan
?q=param updates within ~220 ms?qparam from the URL?q=termstill pre-populates the search on load?qvalues still syncs the inputbun run lint— 0 warnings, 0 errorsbun run test— 604 tests passMade with Cursor