From bde98153897b3672c7303167e503af16ff426a19 Mon Sep 17 00:00:00 2001 From: NiallJoeMaher Date: Wed, 12 Aug 2026 07:49:23 +0100 Subject: [PATCH] fix(discussion): stop comments jumping around when you vote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- components/Discussion/DiscussionArea.tsx | 81 +++++++++++++++++++++--- server/api/router/discussion.ts | 48 +++++++------- 2 files changed, 95 insertions(+), 34 deletions(-) diff --git a/components/Discussion/DiscussionArea.tsx b/components/Discussion/DiscussionArea.tsx index c9e9779dd..1c9d89ec1 100644 --- a/components/Discussion/DiscussionArea.tsx +++ b/components/Discussion/DiscussionArea.tsx @@ -1,6 +1,6 @@ "use client"; -import React, { useState } from "react"; +import React, { useMemo, useRef, useState } from "react"; import { Menu, MenuButton, @@ -91,12 +91,19 @@ const DiscussionArea = ({ contentId, noWrapper = false }: Props) => { }, }); - const { mutate: vote, status: voteStatus } = api.discussion.vote.useMutation({ - onSettled() { - refetch(); - }, - onError() { + // Bumped to remount every VoteControl, which owns its own optimistic state. + // Only needed when a vote fails and that local state has to resync with the + // server; a successful vote needs no refetch, so the thread never reflows. + const [voteResetKey, setVoteResetKey] = useState(0); + + const { mutate: vote } = api.discussion.vote.useMutation({ + async onError() { toast.error("Something went wrong, try again."); + // Refetch BEFORE remounting: the controls seed their state on mount, so + // bumping the key first would reseed them from the pre-vote cache and + // strand every earlier successful vote showing its old count. + await refetch(); + setVoteResetKey((key) => key + 1); }, }); @@ -105,7 +112,6 @@ const DiscussionArea = ({ contentId, noWrapper = false }: Props) => { voteType: "up" | "down" | null, ) => { if (!session) return signIn(); - if (voteStatus === "pending") return; vote({ discussionId, voteType }); }; @@ -129,13 +135,71 @@ const DiscussionArea = ({ contentId, noWrapper = false }: Props) => { type Discussions = typeof discussions; type Children = typeof firstChild; + // "Top" ranks by score, but re-ranking on every vote makes comments jump + // around while somebody is reading them. So each comment's sort score is + // frozen the first time we see it and reused from then on: the thread still + // reorders by votes, it just does it on the next load instead of mid-read. + // (The date-based sorts need no freezing — createdAt never changes.) + // + // The trade-off is deliberate: a tab left open for hours keeps the ranking it + // loaded with, even after other people's votes arrive with a later refetch. + // Re-picking the sort below drops the snapshot, so a reader who wants the + // current ranking has one click to get it. + const frozenSortScores = useRef(new Map()); + const frozenForSort = useRef(sortOrder); + + // Minimal shape the tree walkers below need, so they can recurse without + // depending on the full inferred tRPC comment type. + type ScoredNode = { id: string; score: number; children?: ScoredNode[] }; + + // Identity of the comment *set*, which changes only when comments are added + // or removed — not when their scores change. Drives the capture below. + const commentSetKey = useMemo(() => { + const ids: string[] = []; + const collect = (items: ScoredNode[] | undefined) => { + items?.forEach((item) => { + ids.push(item.id); + collect(item.children); + }); + }; + collect(discussions as ScoredNode[] | undefined); + return ids.join(","); + }, [discussions]); + + 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; + const capture = (items: ScoredNode[] | undefined) => { + items?.forEach((item) => { + if (!captured.has(item.id)) captured.set(item.id, item.score); + capture(item.children); + }); + }; + capture(discussions as ScoredNode[] | undefined); + return new Map(captured); + // Intentionally keyed on the comment set rather than `discussions` itself: + // a score changing must NOT re-capture, or the freeze does nothing. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [commentSetKey, sortOrder]); + const sortDiscussions = ( items: Discussions | Children | undefined, ): typeof items => { if (!items) return items; const sorted = [...items].sort((a, b) => { if (sortOrder === "top") { - return b.score - a.score; + const scoreDiff = + (sortScores.get(b.id) ?? b.score) - (sortScores.get(a.id) ?? a.score); + if (scoreDiff !== 0) return scoreDiff; + // Newest-first within a score tie, so ordering stays deterministic. + return ( + new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() + ); } if (sortOrder === "oldest") { return ( @@ -368,6 +432,7 @@ const DiscussionArea = ({ contentId, noWrapper = false }: Props) => {
0) { - await ctx.db - .delete(comment_votes) - .where(eq(comment_votes.id, existingVote[0].id)); - } + await ctx.db + .delete(comment_votes) + .where( + and( + eq(comment_votes.commentId, commentId), + eq(comment_votes.userId, userId), + ), + ); return { voteType: null }; - } else if (existingVote.length === 0) { - await ctx.db.insert(comment_votes).values({ + } + + await ctx.db + .insert(comment_votes) + .values({ commentId, userId, voteType: voteType as "up" | "down", + }) + .onConflictDoUpdate({ + target: [comment_votes.commentId, comment_votes.userId], + set: { voteType: voteType as "up" | "down" }, }); - return { voteType }; - } else if (existingVote[0].voteType !== voteType) { - await ctx.db - .update(comment_votes) - .set({ voteType: voteType as "up" | "down" }) - .where(eq(comment_votes.id, existingVote[0].id)); - return { voteType }; - } return { voteType }; }),