From f4db1061bcb464f80c3f65be91f71f0ea8877315 Mon Sep 17 00:00:00 2001 From: NiallJoeMaher Date: Wed, 12 Aug 2026 08:25:50 +0100 Subject: [PATCH] fix(discussion): drop the frozen sort snapshot, serialise votes per comment Follow-up to #1341. Review found the score-freezing in that change was both more than the fix needed and wrong on its own terms. Displayed counts still updated on refetch while the frozen ordering did not, so a thread could render a 42-point comment sitting below a 3-point one. The documented "re-pick the sort to get the live ranking" escape hatch never fired, because selecting the already-selected option is a no-op. And the tiebreak it introduced made Top identical to New on the common all-zero thread. Not refetching after a successful vote is the whole fix on its own: the new count lives in VoteControl, `discussions` is untouched, so nothing re-ranks under the reader. Ordering goes back to being derived from the data on screen, so it can never contradict the counts beside it. Votes are now serialised per comment, newest click replacing any queued one. #1341 removed the in-flight guard without replacing it, so overlapping requests could land in either order and leave the stored vote disagreeing with the UI; eight rapid clicks now collapse to two requests rather than eight against the rate limit. The remount that resyncs a failed vote is per comment too, so one failure no longer discards the optimistic state of votes on other comments or of one still in flight, and server messages (rate limiting) reach the toast instead of a generic string. --- components/Discussion/DiscussionArea.tsx | 130 ++++++++++------------- 1 file changed, 55 insertions(+), 75 deletions(-) diff --git a/components/Discussion/DiscussionArea.tsx b/components/Discussion/DiscussionArea.tsx index 1c9d89ec..65508a2c 100644 --- a/components/Discussion/DiscussionArea.tsx +++ b/components/Discussion/DiscussionArea.tsx @@ -1,6 +1,6 @@ "use client"; -import React, { useMemo, useRef, useState } from "react"; +import React, { useRef, useState } from "react"; import { Menu, MenuButton, @@ -91,28 +91,61 @@ const DiscussionArea = ({ contentId, noWrapper = false }: Props) => { }, }); - // 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); + // VoteControl owns its own optimistic state, so a failed vote has to be told + // to resync. Bumping this comment's key remounts just that control, which + // reseeds from the cache — which never saw the failed vote, so it is the + // truth. Keyed per comment so one failure cannot discard the optimistic state + // of votes on other comments (or ones still in flight). + const [voteResetKeys, setVoteResetKeys] = useState>( + {}, + ); + + const { mutateAsync: vote } = api.discussion.vote.useMutation({ + onError(error, variables) { + toast.error(error.message || "Something went wrong, try again."); + setVoteResetKeys((keys) => ({ + ...keys, + [variables.discussionId]: (keys[variables.discussionId] ?? 0) + 1, + })); }, }); + // One request in flight per comment, with the newest click replacing any + // queued one. Without this, double-clicking an arrow fires two overlapping + // requests whose writes can land in either order, leaving the stored vote + // disagreeing with what the reader sees — and burning rate-limit budget. + const voteQueues = useRef( + new Map(), + ); + + const drainVotes = async (discussionId: string) => { + const queue = voteQueues.current.get(discussionId); + if (!queue || queue.running) return; + + queue.running = true; + while (queue.next !== undefined) { + const voteType = queue.next; + queue.next = undefined; + try { + await vote({ discussionId, voteType }); + } catch { + // onError has already reported and resynced this comment; drop + // whatever was queued behind the failure rather than replaying it. + break; + } + } + queue.running = false; + }; + const voteDiscussion = ( discussionId: string, voteType: "up" | "down" | null, ) => { if (!session) return signIn(); - vote({ discussionId, voteType }); + const queue = voteQueues.current.get(discussionId) ?? { running: false }; + queue.next = voteType; + voteQueues.current.set(discussionId, queue); + void drainVotes(discussionId); }; const discussions = discussionsResponse?.data; @@ -135,71 +168,18 @@ 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]); - + // Ordering is derived from `discussions`, so it only changes when that data + // changes. A successful vote deliberately does not refetch (see the mutation + // above), which is what stops a liked comment from re-ranking under the + // reader — the new count lives in VoteControl until the next load. const sortDiscussions = ( items: Discussions | Children | undefined, ): typeof items => { if (!items) return items; + // Array#sort is stable, so equal scores keep the server's order. const sorted = [...items].sort((a, b) => { if (sortOrder === "top") { - 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() - ); + return b.score - a.score; } if (sortOrder === "oldest") { return ( @@ -432,7 +412,7 @@ const DiscussionArea = ({ contentId, noWrapper = false }: Props) => {