fix(discussion): stop comments jumping around when you vote - #1341
Conversation
Voting refetched the thread, and "Top" re-sorts by score, so liking a comment yanked it up the page mid-read. A successful vote no longer refetches — VoteControl already updates optimistically — and each comment's sort score is now frozen the first time it is seen, so later data refreshes cannot reshuffle a thread somebody is reading. Ordering still updates, just on the next load, or immediately if the reader re-picks the sort. Two things fall out of that: - The global "a vote is in flight" guard is gone. It blocked votes on every other comment while one was pending, and swallowed the click after VoteControl had already toggled itself, leaving the UI showing a vote that was never sent. - With clicks no longer serialised, the vote mutation's read-then-write could race itself: two overlapping requests both saw "no vote" and both inserted, tripping comment_votes_comment_id_user_id_key. It is now a single delete-by-key or upsert, so whichever request lands last wins. Failed votes still resync: the refetch now completes before the controls are remounted, otherwise they would reseed from the pre-vote cache and strand earlier successful votes showing their old counts.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe PR changes discussion voting to use atomic server writes and client-side error recovery. It also stabilizes top-score ordering by snapshotting scores and applying creation-time tie-breaking. ChangesDiscussion voting and ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VoteControl
participant DiscussionArea
participant DiscussionAPI
participant VoteDatabase
VoteControl->>DiscussionArea: submit vote
DiscussionArea->>DiscussionAPI: invoke vote mutation
DiscussionAPI->>VoteDatabase: delete or upsert vote
VoteDatabase-->>DiscussionAPI: return mutation result
DiscussionAPI-->>DiscussionArea: return success or error
alt vote error
DiscussionArea->>DiscussionAPI: refetch discussion
DiscussionArea->>VoteControl: increment reset key
VoteControl-->>DiscussionArea: remount with server state
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/Discussion/DiscussionArea.tsx (2)
190-212: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
sortDiscussionsreadssortScoresbut is not memoized per subtree.
generateDiscussionscallssortDiscussionsonce per node and again for each child list at line 481. Each call copies and sorts the array. For a deep thread this repeats work on every render, including every keystroke in an open editor, becauseshowCommentBoxId,editContent, andvoteResetKeyall live in this component.The current thread sizes probably make this acceptable. If threads grow, memoize the sorted tree once per
discussions/sortOrder/sortScoreschange instead of sorting during render.🤖 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 `@components/Discussion/DiscussionArea.tsx` around lines 190 - 212, Memoize the sorted discussion tree so sorting is recomputed only when discussions, sortOrder, or sortScores changes. Update the sortDiscussions/generateDiscussions flow to reuse the memoized result for the root and child lists rather than copying and sorting each subtree during every render.
169-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the snapshot reset out of the render phase.
sortScoresmutatesfrozenSortScores.currentand assignsfrozenForSort.currentwhile rendering. React 19 can start a render, discard it, and render again. If a render that clears and re-captures the snapshot is discarded, the captured scores come from a tree version that React throws away. StrictMode double-invokes the memo, which also runs the reset branch during render.The capture itself is idempotent, so this is unlikely to produce a visible defect today. It is still fragile under concurrent rendering.
A safer shape keys the snapshot by
sortOrderand avoids the reset branch entirely.♻️ Proposed refactor: key the snapshot by sort order
- const frozenSortScores = useRef(new Map<string, number>()); - const frozenForSort = useRef<SortOrder>(sortOrder); + const frozenSortScores = useRef(new Map<SortOrder, Map<string, number>>());const sortScores = useMemo(() => { - // Re-picking a sort is a deliberate "show me the current ranking", so let - // that re-rank from live scores. Passive vote traffic must not. - if (frozenForSort.current !== sortOrder) { - frozenForSort.current = sortOrder; - frozenSortScores.current.clear(); - } - const captured = frozenSortScores.current; + // Re-picking a sort is a deliberate "show me the current ranking", so it + // starts a fresh snapshot. Passive vote traffic reuses the existing one. + let captured = frozenSortScores.current.get(sortOrder); + if (!captured) { + captured = new Map<string, number>(); + frozenSortScores.current.set(sortOrder, captured); + }Note that this variant keeps a snapshot per sort order, so re-picking a previously used sort reuses its old snapshot. If you want re-picking to always re-rank, keep a monotonic sort-selection counter in state and clear the map in an effect instead.
🤖 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 `@components/Discussion/DiscussionArea.tsx` around lines 169 - 188, Refactor sortScores to avoid mutating frozenForSort.current or frozenSortScores.current during render: key the stored snapshots by sortOrder and reuse the corresponding snapshot without a reset branch. Update the capture logic to populate only that sort order’s snapshot, preserving frozen scores across passive vote updates and allowing React to discard or replay renders safely.
🤖 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.
Nitpick comments:
In `@components/Discussion/DiscussionArea.tsx`:
- Around line 190-212: Memoize the sorted discussion tree so sorting is
recomputed only when discussions, sortOrder, or sortScores changes. Update the
sortDiscussions/generateDiscussions flow to reuse the memoized result for the
root and child lists rather than copying and sorting each subtree during every
render.
- Around line 169-188: Refactor sortScores to avoid mutating
frozenForSort.current or frozenSortScores.current during render: key the stored
snapshots by sortOrder and reuse the corresponding snapshot without a reset
branch. Update the capture logic to populate only that sort order’s snapshot,
preserving frozen scores across passive vote updates and allowing React to
discard or replay renders safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a7f4140-c54a-4c97-a53a-dfc22bb2e71c
📒 Files selected for processing (2)
components/Discussion/DiscussionArea.tsxserver/api/router/discussion.ts
|
Uh oh! @vercel[bot], the image you shared is missing helpful alt text. Check #1341 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs. |
1 participant
What
Liking a comment made it jump up the thread under your cursor. Voting refetched the discussion, and "Top" re-sorts by score, so the comment you just liked was immediately re-ranked mid-read.
Ordering by votes is still the point — it just shouldn't happen while someone is reading. So:
VoteControlalready updates optimistically, so the count responds instantly and the thread never reflows.The trade-off is deliberate and commented: a tab left open for hours keeps the ranking it loaded with.
Two things that fell out of it
The global in-flight guard is gone.
voteStatus === "pending"blocked voting on every comment while any one vote was in flight, and it swallowed the click afterVoteControlhad already toggled itself — leaving the UI showing a vote that was never sent.So the vote mutation had to become race-safe. With clicks no longer serialised,
discussion.vote's SELECT-then-INSERT could race itself: two overlapping requests both see "no vote" and both insert, trippingcomment_votes_comment_id_user_id_key(a 500), or the delete variant no-ops and leaves a vote the UI doesn't show. It's now a single delete-by-key orinsert … onConflictDoUpdate, so whichever request lands last simply wins. The count triggers are unaffected — a same-value update is a no-op fortr_comment_vote_counts.Failed votes still resync. The refetch now completes before the controls are remounted; bumping the remount key first would reseed them from the pre-vote cache and strand earlier successful votes showing their old counts.
Verified locally
npm run lint,npm run prettier,npm run test:unit(118 passing),npm run build.