Skip to content

perf(replies): prefer cached board preview replies #1101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tomcasaburi merged 2 commits into from
Mar 17, 2026
Merged

perf(replies): prefer cached board preview replies #1101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d398a33
perf(replies): prefer cached board preview replies
tomcasaburi Mar 17, 2026
7fad66b
fix(replies): address review feedback
tomcasaburi Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/__tests__/post-community-address-compat.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,8 @@ vi.mock('../../lib/utils/replies-preview-utils', () => ({
filterRepliesForDisplay: (replies: TestComment[]) => replies,
getPreviewDisplayReplies: (replies: TestComment[]) => replies,
getTotalReplyCount: ({ replyCount }: { replyCount?: number }) => replyCount ?? 0,
hasEnoughPreviewReplies: ({ replyCount, loadedCount, visibleCount }: { replyCount?: number; loadedCount: number; visibleCount: number }) =>
loadedCount >= Math.min(visibleCount, replyCount ?? visibleCount),
Comment on lines +329 to +330
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

}));

vi.mock('../../lib/utils/thread-scroll-utils', () => ({
Expand Down
33 changes: 28 additions & 5 deletions src/components/post-desktop/post-desktop.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,13 @@ import useQuotedByMap from '../../hooks/use-quoted-by-map';
import useProgressiveRender from '../../hooks/use-progressive-render';
import useFreshReplies from '../../hooks/use-fresh-replies';
import { BOARD_REPLIES_PREVIEW_FETCH_SIZE, BOARD_REPLIES_PREVIEW_VISIBLE_COUNT, REPLIES_PER_PAGE } from '../../lib/constants';
import { computeOmittedCount, filterRepliesForDisplay, getPreviewDisplayReplies, getTotalReplyCount } from '../../lib/utils/replies-preview-utils';
import {
computeOmittedCount,
filterRepliesForDisplay,
getPreviewDisplayReplies,
getTotalReplyCount,
hasEnoughPreviewReplies,
} from '../../lib/utils/replies-preview-utils';
import { isCommentArchived } from '../../lib/utils/comment-moderation-utils';
import { getThreadTopNavigationState, scrollThreadContainerToTop } from '../../lib/utils/thread-scroll-utils';
import useDeleteFailedPost from '../../hooks/use-delete-failed-post';
Expand Down Expand Up @@ -821,11 +827,27 @@ const PostDesktop = ({

const { showOmittedReplies, setShowOmittedReplies } = useShowOmittedReplies();

const shouldFetchPreview = showReplies && !isModQueue && !showAllReplies && showOmittedReplies[cid] !== true;
const shouldUsePreview = showReplies && !isModQueue && !showAllReplies;
const shouldFetchFull = showReplies && !isModQueue && (showAllReplies || showOmittedReplies[cid]);

const cachedPreviewRepliesResult = useReplies({
comment: shouldUsePreview ? resolvedPost : undefined,
onlyIfCached: true,
sortType: 'new',
flat: true,
repliesPerPage: BOARD_REPLIES_PREVIEW_FETCH_SIZE,
accountComments: { newerThan: Infinity, append: true },
});
const cachedPreviewReplies = (cachedPreviewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies?.length
? (cachedPreviewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies!
: cachedPreviewRepliesResult.replies || [];
const hasEnoughCachedPreview = hasEnoughPreviewReplies({
replyCount: resolvedPost?.replyCount,
loadedCount: cachedPreviewReplies.length,
visibleCount: BOARD_REPLIES_PREVIEW_VISIBLE_COUNT,
});
Copy link

@cursor cursor bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

const previewRepliesResult = useReplies({
comment: shouldFetchPreview ? resolvedPost : undefined,
comment: shouldUsePreview && !showOmittedReplies[cid] && !hasEnoughCachedPreview ? resolvedPost : undefined,
sortType: 'new',
flat: true,
repliesPerPage: BOARD_REPLIES_PREVIEW_FETCH_SIZE,
Expand All @@ -839,9 +861,10 @@ const PostDesktop = ({
accountComments: { newerThan: Infinity, append: true },
});

const previewReplies = (previewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies?.length
const livePreviewReplies = (previewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies?.length
? (previewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies!
: previewRepliesResult.replies || [];
const previewReplies = hasEnoughCachedPreview ? cachedPreviewReplies : livePreviewReplies;
const fullReplies = (fullRepliesResult as { updatedReplies?: Comment[] }).updatedReplies?.length
? (fullRepliesResult as { updatedReplies?: Comment[] }).updatedReplies!
: fullRepliesResult.replies || [];
Expand Down Expand Up @@ -875,7 +898,7 @@ const PostDesktop = ({
const totalReplyCount = getTotalReplyCount({
replyCount: resolvedPost?.replyCount,
fullLoadedCount: fullReplies.length,
previewLoadedCount: previewReplies.length,
previewLoadedCount: Math.max(cachedPreviewReplies.length, livePreviewReplies.length),
});
const repliesCount = computeOmittedCount({
totalReplyCount,
Expand Down
28 changes: 25 additions & 3 deletions src/components/post-mobile/post-mobile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import useProgressiveRender from '../../hooks/use-progressive-render';
import useFreshReplies from '../../hooks/use-fresh-replies';
import { BOARD_REPLIES_PREVIEW_FETCH_SIZE, BOARD_REPLIES_PREVIEW_VISIBLE_COUNT, REPLIES_PER_PAGE } from '../../lib/constants';
import { isCommentArchived } from '../../lib/utils/comment-moderation-utils';
import { filterRepliesForDisplay, getPreviewDisplayReplies } from '../../lib/utils/replies-preview-utils';
import { filterRepliesForDisplay, getPreviewDisplayReplies, hasEnoughPreviewReplies } from '../../lib/utils/replies-preview-utils';
import { getRenderableMobileBacklinks } from '../../lib/utils/reply-backlink-utils';
import { getThreadTopNavigationState, scrollThreadContainerToTop } from '../../lib/utils/thread-scroll-utils';
import useDeleteFailedPost from '../../hooks/use-delete-failed-post';
Expand Down Expand Up @@ -564,8 +564,26 @@ const PostMobile = ({
const boardPath = communityAddress ? getBoardPath(communityAddress, directories) : undefined;
const linksCount = useCountLinksInReplies(resolvedPost);
const shouldFetchReplies = showReplies && !isModQueue;
const shouldUsePreview = shouldFetchReplies && !showAllReplies;
const cachedPreviewRepliesResult = useReplies({
comment: shouldUsePreview ? resolvedPost : undefined,
onlyIfCached: true,
sortType: 'new',
flat: true,
repliesPerPage: BOARD_REPLIES_PREVIEW_FETCH_SIZE,
accountComments: { newerThan: Infinity, append: true },
});
const cachedPreviewReplies = (cachedPreviewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies?.length
? (cachedPreviewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies!
: cachedPreviewRepliesResult.replies || [];
const cachedPreviewDisplayCount = filterRepliesForDisplay(cachedPreviewReplies).length;
const hasEnoughCachedPreview = hasEnoughPreviewReplies({
replyCount: resolvedPost?.replyCount,
loadedCount: cachedPreviewDisplayCount,
visibleCount: BOARD_REPLIES_PREVIEW_VISIBLE_COUNT,
});
const previewRepliesResult = useReplies({
comment: shouldFetchReplies && !showAllReplies ? resolvedPost : undefined,
comment: shouldUsePreview && !hasEnoughCachedPreview ? resolvedPost : undefined,
sortType: 'new',
flat: true,
repliesPerPage: BOARD_REPLIES_PREVIEW_FETCH_SIZE,
Expand All @@ -578,7 +596,11 @@ const PostMobile = ({
repliesPerPage: REPLIES_PER_PAGE,
accountComments: { newerThan: Infinity, append: true },
});
const repliesResult = showAllReplies ? fullRepliesResult : previewRepliesResult;
const livePreviewReplies = (previewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies?.length
? (previewRepliesResult as { updatedReplies?: Comment[] }).updatedReplies!
: previewRepliesResult.replies || [];
const previewReplies = hasEnoughCachedPreview ? cachedPreviewReplies : livePreviewReplies;
const repliesResult = showAllReplies ? fullRepliesResult : { ...previewRepliesResult, replies: previewReplies, updatedReplies: previewReplies };
const { replies, hasMore, loadMore } = repliesResult;
const updatedReplies = (repliesResult as { updatedReplies?: Comment[] }).updatedReplies;
const repliesForRender = updatedReplies?.length ? updatedReplies : replies || [];
Expand Down
6 changes: 5 additions & 1 deletion src/lib/utils/__tests__/misc-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { copyToClipboard } from '../clipboard-utils';
import { hashStringToColor, getTextColorForBackground, removeMarkdown } from '../post-utils';
import { preloadReplyModal, preloadThemeAssets, resolveAssetUrl } from '../preload-utils';
import { computeOmittedCount, filterRepliesForDisplay, getPreviewDisplayReplies, getTotalReplyCount } from '../replies-preview-utils';
import { computeOmittedCount, filterRepliesForDisplay, getPreviewDisplayReplies, getTotalReplyCount, hasEnoughPreviewReplies } from '../replies-preview-utils';
import { getQuotedCidsFromContent, mergeQuotedCids } from '../reply-quote-utils';
import { formatUserIDForDisplay, truncateWithEllipsisInMiddle } from '../string-utils';
import { getFormattedDate, getFormattedTimeAgo, isChristmas } from '../time-utils';
Expand Down Expand Up @@ -186,6 +186,10 @@ describe('misc utils', () => {

expect(computeOmittedCount({ totalReplyCount: 2, visibleCount: 5 })).toBe(0);
expect(computeOmittedCount({ totalReplyCount: 9, visibleCount: 5 })).toBe(4);
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);
Comment on lines +189 to +192
Copy link

@coderabbitai coderabbitai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

expect(getTotalReplyCount({ replyCount: undefined, fullLoadedCount: 7, previewLoadedCount: 5 })).toBe(7);
expect(getTotalReplyCount({ replyCount: 12, fullLoadedCount: 7, previewLoadedCount: 5 })).toBe(12);
});
Expand Down
16 changes: 16 additions & 0 deletions src/lib/utils/replies-preview-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,22 @@ export function computeOmittedCount({ totalReplyCount, visibleCount }: ComputeOm
return Math.max(0, totalReplyCount - visibleCount);
}

interface HasEnoughPreviewRepliesParams {
replyCount: number | undefined;
loadedCount: number;
visibleCount: number;
}

/**
* Board previews can skip a live fetch when cached replies already cover all
* replies that could be shown. If total reply count is unknown, require a full
* visible slice so UX matches the current live-preview behavior.
*/
export function hasEnoughPreviewReplies({ replyCount, loadedCount, visibleCount }: HasEnoughPreviewRepliesParams): boolean {
const requiredCount = typeof replyCount === 'number' && replyCount >= 0 ? Math.min(visibleCount, replyCount) : visibleCount;
return requiredCount === 0 || loadedCount >= requiredCount;
}

interface GetTotalReplyCountParams {
replyCount: number | undefined;
fullLoadedCount: number;
Expand Down
Loading