perf(replies): prefer cached board preview replies#1101
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds cache-aware reply preview logic: components check cached preview replies via a new Changes
Sequence Diagram(s)sequenceDiagram
actor Component as Post Component
participant Cache as useReplies<br/>(onlyIfCached)
participant Live as useReplies<br/>(live fetch)
participant Render as Render<br/>Preview
Component->>Cache: request cachedPreviewReplies
Cache-->>Component: cachedPreviewReplies
Component->>Component: hasEnoughPreviewReplies(replyCount, cachedCount, visibleCount)?
alt Enough Cached Data
Component->>Render: render using cachedPreviewReplies
else Insufficient Cached Data
Component->>Live: request live preview replies
Live-->>Component: livePreviewReplies
Component->>Render: render using livePreviewReplies (or merged result)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/post-desktop/post-desktop.tsx (1)
830-867: Extract the preview-source selection into a shared hook.This block now mirrors the new mobile preview-selection flow almost line-for-line. Pulling it into
src/hooks/would keep the cache-hit rule, reply-source fallbacks, and future fixes aligned in one place.As per coding guidelines, "Avoid copy-paste logic across components. Extract custom hooks in
src/hooks/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/post-desktop/post-desktop.tsx` around lines 830 - 867, The preview/source selection logic (variables shouldUsePreview, shouldFetchFull, cachedPreviewRepliesResult, cachedPreviewReplies, hasEnoughCachedPreview via hasEnoughPreviewReplies, previewRepliesResult, fullRepliesResult, livePreviewReplies and previewReplies) should be extracted into a shared custom hook (e.g. usePreviewRepliesSelection or useRepliesPreview) under src/hooks; move the decision logic that computes which comment to pass to useReplies, the cache-only call, the cache-vs-live selection and the final previewReplies fallback into that hook, expose the same outputs (previewReplies, fullRepliesResult, previewRepliesResult, cachedPreviewRepliesResult or a simplified API) and replace the duplicated block with calls to this hook and useReplies to preserve the cache-hit rule and fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/post-mobile/post-mobile.tsx`:
- Around line 579-583: The cache-hit check uses cachedPreviewReplies.length
which includes deleted replies and can falsely mark the cache sufficient; change
the call to hasEnoughPreviewReplies to pass the count of displayable
(non-deleted) cached replies instead. Specifically, compute the visible cached
count from cachedPreviewReplies (filtering out deleted/hidden replies) and pass
that value as the loadedCount to hasEnoughPreviewReplies (alongside
resolvedPost?.replyCount and BOARD_REPLIES_PREVIEW_VISIBLE_COUNT) so the preview
decision uses only displayable replies.
In `@src/lib/utils/__tests__/misc-utils.test.ts`:
- Around line 189-192: Add/update component tests for PostMobile and PostDesktop
to cover the cache-hit/cache-miss branches by asserting whether the live-preview
hook useReplies() is invoked based on hasEnoughPreviewReplies output: create two
test cases per component (one where hasEnoughPreviewReplies returns true and one
false), mock or spy on the module that exports useReplies() to track calls, mock
hasEnoughPreviewReplies to return the desired boolean for each test, render the
component with props matching the existing helper tests
(replyCount/loadedCount/visibleCount) and assert that useReplies was not called
when hasEnoughPreviewReplies is true and was called when false; ensure mocks/
spies are properly restored between tests.
In `@src/lib/utils/replies-preview-utils.ts`:
- Around line 68-70: The test's partial mock for the replies-preview-utils
module is missing the newly exported hasEnoughPreviewReplies, causing failures;
update the mock used in the test that stubs replies-preview-utils to either add
a mock implementation for hasEnoughPreviewReplies or replace the partial mock
with a spread of the real module (e.g.,
...jest.requireActual('.../replies-preview-utils')) so the exported function is
available; ensure you reference the hasEnoughPreviewReplies export and adjust
any expectations in the test accordingly.
---
Nitpick comments:
In `@src/components/post-desktop/post-desktop.tsx`:
- Around line 830-867: The preview/source selection logic (variables
shouldUsePreview, shouldFetchFull, cachedPreviewRepliesResult,
cachedPreviewReplies, hasEnoughCachedPreview via hasEnoughPreviewReplies,
previewRepliesResult, fullRepliesResult, livePreviewReplies and previewReplies)
should be extracted into a shared custom hook (e.g. usePreviewRepliesSelection
or useRepliesPreview) under src/hooks; move the decision logic that computes
which comment to pass to useReplies, the cache-only call, the cache-vs-live
selection and the final previewReplies fallback into that hook, expose the same
outputs (previewReplies, fullRepliesResult, previewRepliesResult,
cachedPreviewRepliesResult or a simplified API) and replace the duplicated block
with calls to this hook and useReplies to preserve the cache-hit rule and
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c94d4ce-9fa8-4171-b252-fc240dedc561
📒 Files selected for processing (4)
src/components/post-desktop/post-desktop.tsxsrc/components/post-mobile/post-mobile.tsxsrc/lib/utils/__tests__/misc-utils.test.tssrc/lib/utils/replies-preview-utils.ts
| expect(hasEnoughPreviewReplies({ replyCount: 2, loadedCount: 2, visibleCount: 5 })).toBe(true); | ||
| expect(hasEnoughPreviewReplies({ replyCount: 9, loadedCount: 4, visibleCount: 5 })).toBe(false); | ||
| expect(hasEnoughPreviewReplies({ replyCount: undefined, loadedCount: 5, visibleCount: 5 })).toBe(true); | ||
| expect(hasEnoughPreviewReplies({ replyCount: undefined, loadedCount: 4, visibleCount: 5 })).toBe(false); |
There was a problem hiding this comment.
Add one component regression test for the cache-hit/cache-miss branches.
These assertions cover the helper math, but the fix in this PR is the conditional useReplies() wiring in PostMobile and PostDesktop. Please extend the existing component tests to assert that the live preview hook is skipped when cached replies are enough and still used when they are not.
As per coding guidelines, "Add or update tests for bug fixes and non-trivial logic changes when the code is reasonably testable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/utils/__tests__/misc-utils.test.ts` around lines 189 - 192,
Add/update component tests for PostMobile and PostDesktop to cover the
cache-hit/cache-miss branches by asserting whether the live-preview hook
useReplies() is invoked based on hasEnoughPreviewReplies output: create two test
cases per component (one where hasEnoughPreviewReplies returns true and one
false), mock or spy on the module that exports useReplies() to track calls, mock
hasEnoughPreviewReplies to return the desired boolean for each test, render the
component with props matching the existing helper tests
(replyCount/loadedCount/visibleCount) and assert that useReplies was not called
when hasEnoughPreviewReplies is true and was called when false; ensure mocks/
spies are properly restored between tests.
|
Addressed the valid review findings in the latest commit. Fixed the missing test mock export that was breaking CI, and fixed the mobile preview path so cached previews cannot be bypassed by stale live-hook ; I also use the displayable cached count for the mobile cache-hit check. Remaining bot suggestions about extracting a shared hook and adding broader component-branch tests are deferred because they are non-blocking for correctness and merge safety. |
|
Addressed the valid review findings in Fixed:
Deferred as non-blocking follow-up work:
Local verification after the review-fix commit:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/__tests__/post-community-address-compat.test.tsx`:
- Around line 329-330: The test mock for hasEnoughPreviewReplies should
replicate production's numeric/non-negative guard for replyCount: update the
mock implementation of hasEnoughPreviewReplies to coerce replyCount to a finite
non-negative number (e.g., use Number(...) and Math.max(0, ...) or an isFinite
check) before using it in Math.min with visibleCount so edge cases (undefined,
non-numeric, negative) match runtime behavior in the hasEnoughPreviewReplies
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0617ab43-cee4-43f3-8390-67d801120aaa
📒 Files selected for processing (2)
src/components/__tests__/post-community-address-compat.test.tsxsrc/components/post-mobile/post-mobile.tsx
| hasEnoughPreviewReplies: ({ replyCount, loadedCount, visibleCount }: { replyCount?: number; loadedCount: number; visibleCount: number }) => | ||
| loadedCount >= Math.min(visibleCount, replyCount ?? visibleCount), |
There was a problem hiding this comment.
Mirror production semantics in the hasEnoughPreviewReplies test mock.
This mock currently skips the production helper’s numeric/non-negative guard for replyCount, so edge inputs can behave differently in tests vs runtime.
Proposed fix
- hasEnoughPreviewReplies: ({ replyCount, loadedCount, visibleCount }: { replyCount?: number; loadedCount: number; visibleCount: number }) =>
- loadedCount >= Math.min(visibleCount, replyCount ?? visibleCount),
+ hasEnoughPreviewReplies: ({ replyCount, loadedCount, visibleCount }: { replyCount?: number; loadedCount: number; visibleCount: number }) => {
+ const requiredCount =
+ typeof replyCount === 'number' && replyCount >= 0 ? Math.min(visibleCount, replyCount) : visibleCount;
+ return requiredCount === 0 || loadedCount >= requiredCount;
+ },📝 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.
| hasEnoughPreviewReplies: ({ replyCount, loadedCount, visibleCount }: { replyCount?: number; loadedCount: number; visibleCount: number }) => | |
| loadedCount >= Math.min(visibleCount, replyCount ?? visibleCount), | |
| hasEnoughPreviewReplies: ({ replyCount, loadedCount, visibleCount }: { replyCount?: number; loadedCount: number; visibleCount: number }) => { | |
| const requiredCount = | |
| typeof replyCount === 'number' && replyCount >= 0 ? Math.min(visibleCount, replyCount) : visibleCount; | |
| return requiredCount === 0 || loadedCount >= requiredCount; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/__tests__/post-community-address-compat.test.tsx` around lines
329 - 330, The test mock for hasEnoughPreviewReplies should replicate
production's numeric/non-negative guard for replyCount: update the mock
implementation of hasEnoughPreviewReplies to coerce replyCount to a finite
non-negative number (e.g., use Number(...) and Math.max(0, ...) or an isFinite
check) before using it in Math.min with visibleCount so edge cases (undefined,
non-numeric, negative) match runtime behavior in the hasEnoughPreviewReplies
helper.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| replyCount: resolvedPost?.replyCount, | ||
| loadedCount: cachedPreviewReplies.length, | ||
| visibleCount: BOARD_REPLIES_PREVIEW_VISIBLE_COUNT, | ||
| }); |
There was a problem hiding this comment.
Desktop cached preview count includes deleted replies
Medium Severity
The desktop hasEnoughCachedPreview check uses cachedPreviewReplies.length (raw count), which includes deleted replies. The mobile version correctly computes filterRepliesForDisplay(cachedPreviewReplies).length to exclude deleted replies before checking sufficiency. When cached replies contain deleted items, the desktop path may incorrectly skip the live fetch, resulting in fewer visible replies than expected in the board preview.
Additional Locations (1)
1 participant
Use cached
useReplies({ onlyIfCached: true })preview data on board cards before falling back to the existing live preview request. This keeps the current 5-reply board preview UX while avoiding unnecessary refetches when cached replies already cover the preview.Closes #1100
Note
Medium Risk
Changes reply-loading behavior in
PostDesktop/PostMobileto reduce network fetches; risk is moderate because it affects when/which replies are shown in board previews and could cause stale/partial previews if the sufficiency logic is wrong.Overview
Board reply previews now prefer cached data. Desktop and mobile post cards first read cached preview replies via
useReplies({ onlyIfCached: true })and only issue the existing live preview request when the cache cannot satisfy the visible preview slice.Adds
hasEnoughPreviewRepliesto centralize the “is cache sufficient?” check (handling unknownreplyCount), and updates reply-count calculations/tests to account for cached vs live preview lengths and the new helper.Written by Cursor Bugbot for commit 7fad66b. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Performance Improvements
Tests