feat: performance optimizations#60
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 16 minutes and 28 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR introduces task virtualization for performant large-list rendering, refactors task cards to accept assignment context via props, establishes a centralized event-driven cache invalidation system via LiveSyncProvider, and eliminates router-based cache invalidation across all mutation and query hooks in favor of React Query-only management. ChangesTask Virtualization and View Enhancement
Cache Management and Sync System Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/components/views/BoardView.tsx (1)
175-186: 💤 Low valueMissing
assigningTaskIdprop on drag overlay.The
DragOverlayreceivesusersbut notassigningTaskId. If a user starts dragging a task while an assignment mutation is in flight for that same task, the drag preview won't show the loading spinner. This is a minor UX inconsistency.Suggested fix
<TaskCardComponent task={activeTask} users={users} + assigningTaskId={assigningTaskId} showSprint={showSprint} showProject={showProject} taskLinkTo={taskLinkTo} gitSummary={gitSummaries?.[activeTask.id]} closingStatusId={closingStatusId} />🤖 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 `@src/components/views/BoardView.tsx` around lines 175 - 186, The DragOverlay rendering of TaskCardComponent is missing the assigningTaskId prop, so when a task is being dragged while an assignment mutation is in flight the preview won't show the loading spinner; update the DragOverlay block where TaskCardComponent is rendered (the conditional using activeTask) to pass assigningTaskId={assigningTaskId} along with users, showSprint, showProject, taskLinkTo, gitSummary (gitSummaries?.[activeTask.id]), and closingStatusId so the preview can reflect the in-flight assignment state.src/router.tsx (1)
23-25: ⚡ Quick winUnify cache timing constants across router and freshness helpers.
These values are duplicated across cache-related modules; if one side changes later, loaders can overfetch or skip hydration inconsistently.
♻️ Suggested refactor
diff --git a/src/lib/query-cache-fresh.ts b/src/lib/query-cache-fresh.ts @@ -const DEFAULT_STALE_TIME_MS = 60_000; +export const DEFAULT_STALE_TIME_MS = 60_000; +export const DEFAULT_GC_TIME_MS = 5 * 60_000; diff --git a/src/router.tsx b/src/router.tsx @@ +import { DEFAULT_GC_TIME_MS, DEFAULT_STALE_TIME_MS } from "./lib/query-cache-fresh"; @@ - staleTime: 60_000, - gcTime: 5 * 60_000, + staleTime: DEFAULT_STALE_TIME_MS, + gcTime: DEFAULT_GC_TIME_MS, @@ - defaultStaleTime: 60_000, - defaultGcTime: 5 * 60_000, + defaultStaleTime: DEFAULT_STALE_TIME_MS, + defaultGcTime: DEFAULT_GC_TIME_MS,🤖 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 `@src/router.tsx` around lines 23 - 25, The cache timing values (defaultStaleTime, defaultGcTime, defaultStaleReloadMode) are duplicated and must be centralized: extract these constants into a single shared export (e.g., CACHE_DEFAULTS or named exports defaultStaleTime, defaultGcTime, defaultStaleReloadMode) in a common module and replace the literal values in the router (where defaultStaleTime/defaultGcTime/defaultStaleReloadMode are used) and in the freshness helpers to import and consume those shared constants so both modules always use the same source of truth; ensure the new module exports stable names and update imports accordingly.src/hooks/live-sync-provider.tsx (2)
175-184: ⚡ Quick winUse
URLSearchParamsfor safer query string construction.Manual query string building doesn't handle URL encoding and is more error-prone than the standard
URLSearchParamsAPI.🔧 Refactor to URLSearchParams
const topics = [ ...COARSE_TOPICS, ...(ownerTopic ? [ownerTopic] : []), ...(notificationsTopic ? [notificationsTopic] : []), ]; - let topicsString = "?"; - for (const topic of topics) { - if (topicsString !== "?") topicsString += "&"; - topicsString += `topic[]=${topic}`; - } + const params = new URLSearchParams(); + for (const topic of topics) { + params.append("topic[]", topic); + } const sse = new EventSource( - `${SYNC_STREAM_BASE}/stream/${SYNC_APP_ID}${topicsString}` + `${SYNC_STREAM_BASE}/stream/${SYNC_APP_ID}?${params.toString()}` );🤖 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 `@src/hooks/live-sync-provider.tsx` around lines 175 - 184, The manual construction of topicsString from topics (built from COARSE_TOPICS, ownerTopic, notificationsTopic) should be replaced with URLSearchParams to ensure proper URL encoding and simpler code; create a new URLSearchParams instance, append each topic with key "topic[]" (iterating the topics array used in the existing block) and then use params.toString() to get the encoded query string instead of the current topicsString concatenation logic.
69-77: ⚡ Quick winRemove unreachable notification and owner topic checks.
The
onUpdatehandler (lines 154-162) already checks fornotificationsTopicandownerTopicand returns early before callinghandleCoarseSyncEvent(line 164). These duplicate checks inhandleCoarseSyncEventare dead code—they can never be true because those topics are filtered out upstream.♻️ Simplify by removing unreachable checks
function handleCoarseSyncEvent( queryClient: QueryClient, topic: string, data: unknown, entityId: string | undefined, ownerTopic: string | null, notificationsTopic: string | null ) { - if (notificationsTopic && topic === notificationsTopic) { - void queryClient.invalidateQueries({ queryKey: ["in-app-notifications"] }); - return; - } - - if (ownerTopic && topic === ownerTopic) { - void queryClient.invalidateQueries({ queryKey: ["dashboard-stats"] }); - return; - } - switch (topic) {🤖 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 `@src/hooks/live-sync-provider.tsx` around lines 69 - 77, The duplicate checks for notificationsTopic and ownerTopic inside handleCoarseSyncEvent are unreachable because onUpdate already filters those topics and returns early; remove the conditional blocks that compare topic === notificationsTopic and topic === ownerTopic and the associated void queryClient.invalidateQueries calls (the ones invalidating ["in-app-notifications"] and ["dashboard-stats"]), leaving handleCoarseSyncEvent to only handle remaining topic cases; keep the existing logic in onUpdate and ensure no other callers rely on those removed branches.
🤖 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 `@src/components/views/ListView.tsx`:
- Around line 266-282: The virtualization branch currently triggers whenever
sortedTasks.length > 25, bypassing manual sort; update the condition so
manualSortEnabled takes precedence—i.e., only render VirtualizedTaskListRows
when !manualSortEnabled && sortedTasks.length > 25, otherwise follow the
manual-sort branch that renders TaskListRow(s) using orderedTaskIds, location,
columnFlags, updateTask, closingStatusId and manualSortEnabled; locate the check
around VirtualizedTaskListRows and adjust the conditional accordingly.
In `@src/components/views/TaskViewsContainer.tsx`:
- Around line 90-113: The global "m" handler can trigger while the user is
typing or re-fire during an in-flight toggle; update handleKeyPress to
early-return if event.repeat is true or if the event target is an input/textarea
or an element with contentEditable (e.g., check event.target for instanceof
HTMLInputElement/HTMLTextAreaElement or
element.closest('[contenteditable="true"]')), and also guard against concurrent
mutations by returning when assigningTaskId is non-null; ensure you reference
and use the existing symbols handleKeyPress, assignTask, unassignTask,
setAssigningTaskId, assigningTaskId, hoveredTaskId, currentUserId and add
assigningTaskId and setAssigningTaskId to the useEffect dependency array so the
hook reacts correctly.
In `@src/components/views/VirtualizedTaskCards.tsx`:
- Around line 18-20: The effect that sets scrollElementRef.current is running
too late (it's keyed to items.length) so the virtualizer may see a null scroll
element on initial mount; replace this useEffect with a synchronous capture by
using useLayoutEffect with no items.length dependency (i.e., run once on mount)
or convert listRef to a callback ref that sets scrollElementRef.current from
listRef.current?.parentElement immediately; update the code references: replace
the useEffect that touches scrollElementRef and listRef with a useLayoutEffect
(or implement a callback ref assigned where listRef is used) so the
virtualizer's getScrollElement sees a non-null value on initial render.
In `@src/db/mutations/sync.ts`:
- Around line 67-76: The publish behavior is inconsistent: coarse topics
(COARSE_TOPIC_PREFIXES branch) send an envelope { topic, entityId, payload }
while fine-grained topics publish payload directly, causing consumers to have to
unwrap differently; pick one consistent contract and implement it across both
paths. Update the publishing call sites (the publishSyncTopic invocations for
fine-grained topics and in the COARSE_TOPIC_PREFIXES loop) to always use the
same envelope shape (e.g., { topic, entityId?: string, payload }) and adjust
callers/consumers accordingly, referencing publishSyncTopic,
COARSE_TOPIC_PREFIXES, coarseTopic, entityId and the code path that handles
fine-grained topic publishing so all subscribers receive the same structure.
Ensure tests or docs are updated to reflect the unified envelope contract.
In `@src/db/mutations/tasks.ts`:
- Around line 1346-1356: The detail cache update inside the
queryClient.setQueryData callback for ["tasks", input.taskId] only patches
statusId and updatedAt, causing sortOrder to become stale; update this patch to
also set the new sortOrder (compute it using input.targetIndex and the target
bucket ordering logic the same way patchMovedTask computes the reordered list)
or, if computing client-side is undesirable, call
queryClient.invalidateQueries(["tasks", input.taskId]) after the mutation
succeeds so the detail cache is refetched; refer to patchMovedTask,
input.targetIndex, input.taskId, and TaskWithRelations to locate and mirror the
server-side sortOrder calculation.
- Line 653: The map that resolves labelIds into label objects uses
labels.find(... )! which can yield undefined; change the mapping inside the
functions that build the labels array (the code using labels.find((label) =>
label.id === id)!) to produce only existing labels by replacing the non-null
assertion with a safe lookup and then filtering out missing values (e.g., map to
possibly-undefined and then .filter(Boolean) or use a type guard) so only actual
Label objects are pushed; apply this same fix to the other occurrence that maps
labelIds to labels in this file (the earlier mapping around the same
labels/labelIds logic).
- Around line 645-661: The list cache patching must be moved to run
optimistically before the server mutation so detail and list views stay
consistent; call getTaskListQueryKeys(queryClient, task), compute patchList
(using task.id, labelIds, labels) and apply it with queryClient.setQueryData for
each queryKey prior to sending the mutation (the same place where the detail
cache is updated), and ensure any rollback logic still restores original list
data on mutation error.
In `@src/hooks/live-sync-provider.tsx`:
- Around line 186-196: The EventSource connection (created as sse via new
EventSource(`${SYNC_STREAM_BASE}/stream/${SYNC_APP_ID}${topicsString}`)) lacks
error and open handlers; add sse.addEventListener("error", ...) to surface
connection failures (log the event/error and call a provided onError callback if
available) and sse.addEventListener("open", ...) to log/handle successful
reconnects (or call an onOpen callback), ensure the existing
onUpdate(event.data) remains, and keep the return cleanup sse.close() — this
surfaces network/server errors and connection state for live-sync debugging.
In `@src/routes/_signed-in.tsx`:
- Around line 29-40: The sidebar bundle gating currently sets needsSidebar using
only projects/sprints freshness; update the logic so needsSidebar also becomes
true when user preferences are stale. Specifically, modify the computation of
needsSidebar (which references isQueryCacheFresh,
projectsQueryOptions().queryKey and sprintsQueryOptions().queryKey) to include
userPreferencesQueryOptions().queryKey (or reuse needsPreferences) so that
fetchSidebarBundle() runs whenever preferences are stale; keep
fetchUserPreferences() logic as-is to still fetch preferences in parallel.
---
Nitpick comments:
In `@src/components/views/BoardView.tsx`:
- Around line 175-186: The DragOverlay rendering of TaskCardComponent is missing
the assigningTaskId prop, so when a task is being dragged while an assignment
mutation is in flight the preview won't show the loading spinner; update the
DragOverlay block where TaskCardComponent is rendered (the conditional using
activeTask) to pass assigningTaskId={assigningTaskId} along with users,
showSprint, showProject, taskLinkTo, gitSummary (gitSummaries?.[activeTask.id]),
and closingStatusId so the preview can reflect the in-flight assignment state.
In `@src/hooks/live-sync-provider.tsx`:
- Around line 175-184: The manual construction of topicsString from topics
(built from COARSE_TOPICS, ownerTopic, notificationsTopic) should be replaced
with URLSearchParams to ensure proper URL encoding and simpler code; create a
new URLSearchParams instance, append each topic with key "topic[]" (iterating
the topics array used in the existing block) and then use params.toString() to
get the encoded query string instead of the current topicsString concatenation
logic.
- Around line 69-77: The duplicate checks for notificationsTopic and ownerTopic
inside handleCoarseSyncEvent are unreachable because onUpdate already filters
those topics and returns early; remove the conditional blocks that compare topic
=== notificationsTopic and topic === ownerTopic and the associated void
queryClient.invalidateQueries calls (the ones invalidating
["in-app-notifications"] and ["dashboard-stats"]), leaving handleCoarseSyncEvent
to only handle remaining topic cases; keep the existing logic in onUpdate and
ensure no other callers rely on those removed branches.
In `@src/router.tsx`:
- Around line 23-25: The cache timing values (defaultStaleTime, defaultGcTime,
defaultStaleReloadMode) are duplicated and must be centralized: extract these
constants into a single shared export (e.g., CACHE_DEFAULTS or named exports
defaultStaleTime, defaultGcTime, defaultStaleReloadMode) in a common module and
replace the literal values in the router (where
defaultStaleTime/defaultGcTime/defaultStaleReloadMode are used) and in the
freshness helpers to import and consume those shared constants so both modules
always use the same source of truth; ensure the new module exports stable names
and update imports accordingly.
🪄 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 Plus
Run ID: 152846e4-3885-4c2b-99ff-9893975c259d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
package.jsonsrc/components/OpenTask.tsxsrc/components/TaskCard.tsxsrc/components/views/BoardView.tsxsrc/components/views/ListView.tsxsrc/components/views/TaskViewsContainer.tsxsrc/components/views/VirtualizedTaskCards.tsxsrc/components/views/VirtualizedTaskListRows.tsxsrc/components/views/task-view-types.tssrc/db/mutations/attachments.tssrc/db/mutations/comments.tssrc/db/mutations/labels.tssrc/db/mutations/projects.tssrc/db/mutations/sprints.tssrc/db/mutations/statuses.tssrc/db/mutations/sub-tasks.tssrc/db/mutations/sync.tssrc/db/mutations/tasks.tssrc/db/queries/attachments.tssrc/db/queries/bundles.tssrc/db/queries/comments.tssrc/db/queries/dashboard.tssrc/db/queries/hydrate-query-cache.tssrc/db/queries/labels.tssrc/db/queries/notifications.tssrc/db/queries/projects.tssrc/db/queries/sprints.tssrc/db/queries/statuses.tssrc/db/queries/tasks.tssrc/hooks/live-sync-provider.tsxsrc/hooks/use-event-source.tssrc/hooks/use-git-task-live-sync.tssrc/lib/query-cache-fresh.tssrc/router.tsxsrc/routes/_signed-in.tsxsrc/routes/_signed-in/dashboard.tsxsrc/routes/_signed-in/projects.$projectId.tasks.tsxsrc/routes/_signed-in/sprints.$sprintId.tasks.tsxsrc/routes/_signed-in/tasks.tsxvite.config.ts
💤 Files with no reviewable changes (1)
- src/hooks/use-event-source.ts
✅ Deploy Preview for pno-project-y ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
🎉 This PR is included in version 0.12.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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 by CodeRabbit
Release Notes
New Features
Performance
Improvements