perf(desktop): fix tasks page rendering performance#1537
Closed
AviPeltz wants to merge 2 commits into
Closed
Conversation
Eliminate N+1 useLiveQuery calls in AssigneeCell by deferring the users query until the dropdown is opened. Lazily build Fuse.js search indexes only when a search is actually performed. Debounce the search input to prevent synchronous table re-renders on every keystroke.
Contributor
📝 WalkthroughWalkthroughImplements a debounced local search input in TasksTopBar, refactors useHybridSearch to build Fuse indexes via refs with lazy initialization, and moves user-provisioning for assignee rendering from internal queries into the table hook (useTasksTable) so AssigneeCell receives users as a prop. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useHybridSearch/useHybridSearch.ts (1)
19-20: Writing to a ref during render — safe here but worth a note.Assigning
tasksRef.current = tasksduring render is a well-known pattern for "latest value" refs, but in Concurrent React it can theoretically read a value from a discarded render. This is benign here because the ref is only consumed inside event-driven callbacks (search), never during render output. Just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useHybridSearch/useHybridSearch.ts` around lines 19 - 20, The ref write to tasksRef.current during render can read values from discarded renders in concurrent React; instead update the ref inside an effect so the latest tasks are set after commit — e.g., replace the inline assignment with a useEffect that sets tasksRef.current = tasks (or, if you intentionally rely on the render-time assignment, add a clear comment above tasksRef/useRef and where search uses it explaining it’s only read from event-driven callbacks). Update references: tasksRef, useRef, and the search callback to match this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/TasksTopBar.tsx`:
- Around line 61-63: The effect that syncs localSearch to an empty parent
searchQuery must also cancel any pending debounced update so a stale
onSearchChange(oldValue) doesn't fire; inside the useEffect that checks if
(searchQuery === "") { setLocalSearch("") } also clear the debounce (e.g.,
clearTimeout(searchDebounceRef.current) or call
searchDebounceRef.current.cancel() if using lodash/debounce) and reset the ref,
ensuring no pending onSearchChange(value) will run after the parent clears the
query.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useHybridSearch/useHybridSearch.ts`:
- Around line 19-20: The ref write to tasksRef.current during render can read
values from discarded renders in concurrent React; instead update the ref inside
an effect so the latest tasks are set after commit — e.g., replace the inline
assignment with a useEffect that sets tasksRef.current = tasks (or, if you
intentionally rely on the render-time assignment, add a clear comment above
tasksRef/useRef and where search uses it explaining it’s only read from
event-driven callbacks). Update references: tasksRef, useRef, and the search
callback to match this change.
Comment on lines
+61
to
+63
| useEffect(() => { | ||
| if (searchQuery === "") setLocalSearch(""); | ||
| }, [searchQuery]); |
Contributor
There was a problem hiding this comment.
Stale debounce fires after parent clears the search.
When the parent resets searchQuery to "" (e.g., on tab change), localSearch is synced to "", but a pending debounced onSearchChange(value) from a prior keystroke can still fire ~150 ms later, re-populating the search with the old value.
Cancel the timer alongside the sync:
Proposed fix
useEffect(() => {
- if (searchQuery === "") setLocalSearch("");
+ if (searchQuery === "") {
+ clearTimeout(debounceRef.current);
+ setLocalSearch("");
+ }
}, [searchQuery]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (searchQuery === "") setLocalSearch(""); | |
| }, [searchQuery]); | |
| useEffect(() => { | |
| if (searchQuery === "") { | |
| clearTimeout(debounceRef.current); | |
| setLocalSearch(""); | |
| } | |
| }, [searchQuery]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/TasksTopBar.tsx`
around lines 61 - 63, The effect that syncs localSearch to an empty parent
searchQuery must also cancel any pending debounced update so a stale
onSearchChange(oldValue) doesn't fire; inside the useEffect that checks if
(searchQuery === "") { setLocalSearch("") } also clear the debounce (e.g.,
clearTimeout(searchDebounceRef.current) or call
searchDebounceRef.current.cancel() if using lodash/debounce) and reset the ref,
ensuring no pending onSearchChange(value) will run after the parent clears the
query.
Query all users once at the table level and pass them down as a prop, eliminating the per-cell useLiveQuery entirely. Fixes the empty dropdown flash on first open since user data is already available.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/useTasksTable.tsx (1)
307-328: Consider using a ref to avoid column recreation on user-list changes.The dependency array changed from
[]to[users], which is necessary for correctness since theAssigneeCellrenderer closes overusers. However, this means the entire column definition array is recreated whenever the users list changes, which triggers a full table re-render.If
useLiveQueryreturns a newallUsersreference on every Electric sync tick (even when the actual user data is unchanged), this could partially offset the performance gains from eliminating per-row queries. You could use a ref + structural comparison to keepusersreferentially stable:♻️ Optional: stabilize users reference
+ import { useRef } from "react"; + + function useStableArray<T>(arr: T[]): T[] { + const ref = useRef(arr); + if ( + arr.length !== ref.current.length || + arr.some((item, i) => item !== ref.current[i]) + ) { + ref.current = arr; + } + return ref.current; + }Then wrap
users:- const users = useMemo(() => allUsers ?? [], [allUsers]); + const users = useStableArray(useMemo(() => allUsers ?? [], [allUsers]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/useTasksTable.tsx` around lines 307 - 328, The columns are being recreated whenever the `users` array identity changes; stabilize it by introducing a ref (e.g., `usersRef`) that you update only when the user list actually changes (structural compare by id/length or deep equality) and then pass `usersRef.current` into the `AssigneeCell` renderer instead of the raw `users` array; update the column `useMemo` dependency to `[]` (or other stable deps) so the column definitions (e.g., the `columnHelper.accessor` that renders `AssigneeCell`) aren't recreated on every sync tick.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/hooks/useTasksTable/useTasksTable.tsx`:
- Around line 307-328: The columns are being recreated whenever the `users`
array identity changes; stabilize it by introducing a ref (e.g., `usersRef`)
that you update only when the user list actually changes (structural compare by
id/length or deep equality) and then pass `usersRef.current` into the
`AssigneeCell` renderer instead of the raw `users` array; update the column
`useMemo` dependency to `[]` (or other stable deps) so the column definitions
(e.g., the `columnHelper.accessor` that renders `AssigneeCell`) aren't recreated
on every sync tick.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useLiveQuerycalls, eager Fuse.js index rebuilds, and unbuffered search inputAssigneeCellrow fired its own live query to fetch all users — now deferred until dropdown opensChanges
useLiveQuerybehindopenstate so the query only runs when the dropdown is opened (matches existingStatusCellpattern)useMemo-based Fuse instances with ref-based lazy initialization viaensureIndex()— indexes are only built whensearch()is called and invalidated by reference equality on the tasks arrayTest Plan
Summary by CodeRabbit
Performance Improvements
Bug Fixes / Reliability