From 060afc848c8f787046d069a986f1ad3b86a64003 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:35:52 +0800 Subject: [PATCH] refactor(web): migrate reaction optimistic updates from cache to UI pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace cache-level optimistic updates (onMutate with temp IDs) with TQ v5's UI-level pattern (useMutationState + render-time derivation) for both issue-level and comment-level reaction toggles. The cache-level approach caused a race condition: temp IDs in the cache couldn't be deduplicated against real IDs from WS events, causing reaction counts to briefly flash incorrect values (e.g. 0→1→2→1). The UI pattern keeps the cache clean (always server-confirmed data) and derives optimistic state at render time from pending mutation variables. WS events safely update the cache without conflicting with temp data. Co-Authored-By: Claude Opus 4.6 (1M context) --- apps/web/core/issues/mutations.ts | 111 +----------------- .../issues/hooks/use-issue-reactions.ts | 62 +++++++++- .../issues/hooks/use-issue-timeline.ts | 64 +++++++++- 3 files changed, 119 insertions(+), 118 deletions(-) diff --git a/apps/web/core/issues/mutations.ts b/apps/web/core/issues/mutations.ts index 42575267..678978f9 100644 --- a/apps/web/core/issues/mutations.ts +++ b/apps/web/core/issues/mutations.ts @@ -334,6 +334,7 @@ export function useDeleteComment(issueId: string) { export function useToggleCommentReaction(issueId: string) { const qc = useQueryClient(); return useMutation({ + mutationKey: ["toggleCommentReaction", issueId] as const, mutationFn: async ({ commentId, emoji, @@ -349,73 +350,6 @@ export function useToggleCommentReaction(issueId: string) { } return api.addReaction(commentId, emoji); }, - onMutate: async ({ commentId, emoji, existing }) => { - await qc.cancelQueries({ queryKey: issueKeys.timeline(issueId) }); - const prev = qc.getQueryData(issueKeys.timeline(issueId)); - - if (existing) { - // Remove - qc.setQueryData( - issueKeys.timeline(issueId), - (old) => - old?.map((e) => - e.id === commentId - ? { - ...e, - reactions: (e.reactions ?? []).filter( - (r) => r.id !== existing.id, - ), - } - : e, - ), - ); - } else { - // Add temp - const tempReaction: Reaction = { - id: `temp-${Date.now()}`, - comment_id: commentId, - actor_type: "", - actor_id: "", - emoji, - created_at: new Date().toISOString(), - }; - qc.setQueryData( - issueKeys.timeline(issueId), - (old) => - old?.map((e) => - e.id === commentId - ? { ...e, reactions: [...(e.reactions ?? []), tempReaction] } - : e, - ), - ); - } - return { prev }; - }, - onSuccess: (reaction, { commentId }) => { - if (reaction) { - // Replace temp with real - qc.setQueryData( - issueKeys.timeline(issueId), - (old) => - old?.map((e) => - e.id === commentId - ? { - ...e, - reactions: (e.reactions ?? []).map((r) => - r.id.startsWith("temp-") && r.emoji === reaction.emoji - ? reaction - : r, - ), - } - : e, - ), - ); - } - }, - onError: (_err, _vars, ctx) => { - if (ctx?.prev) - qc.setQueryData(issueKeys.timeline(issueId), ctx.prev); - }, onSettled: () => { qc.invalidateQueries({ queryKey: issueKeys.timeline(issueId) }); }, @@ -429,6 +363,7 @@ export function useToggleCommentReaction(issueId: string) { export function useToggleIssueReaction(issueId: string) { const qc = useQueryClient(); return useMutation({ + mutationKey: ["toggleIssueReaction", issueId] as const, mutationFn: async ({ emoji, existing, @@ -442,48 +377,6 @@ export function useToggleIssueReaction(issueId: string) { } return api.addIssueReaction(issueId, emoji); }, - onMutate: async ({ emoji, existing }) => { - await qc.cancelQueries({ queryKey: issueKeys.reactions(issueId) }); - const prev = qc.getQueryData(issueKeys.reactions(issueId)); - - if (existing) { - qc.setQueryData( - issueKeys.reactions(issueId), - (old) => old?.filter((r) => r.id !== existing.id), - ); - } else { - const temp: IssueReaction = { - id: `temp-${Date.now()}`, - issue_id: issueId, - actor_type: "", - actor_id: "", - emoji, - created_at: new Date().toISOString(), - }; - qc.setQueryData( - issueKeys.reactions(issueId), - (old) => [...(old ?? []), temp], - ); - } - return { prev }; - }, - onSuccess: (reaction) => { - if (reaction) { - qc.setQueryData( - issueKeys.reactions(issueId), - (old) => - old?.map((r) => - r.id.startsWith("temp-") && r.emoji === reaction.emoji - ? reaction - : r, - ), - ); - } - }, - onError: (_err, _vars, ctx) => { - if (ctx?.prev) - qc.setQueryData(issueKeys.reactions(issueId), ctx.prev); - }, onSettled: () => { qc.invalidateQueries({ queryKey: issueKeys.reactions(issueId) }); }, diff --git a/apps/web/features/issues/hooks/use-issue-reactions.ts b/apps/web/features/issues/hooks/use-issue-reactions.ts index 7fec1fd8..44a5a30f 100644 --- a/apps/web/features/issues/hooks/use-issue-reactions.ts +++ b/apps/web/features/issues/hooks/use-issue-reactions.ts @@ -1,7 +1,7 @@ "use client"; -import { useCallback } from "react"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { useCallback, useMemo } from "react"; +import { useQuery, useQueryClient, useMutationState } from "@tanstack/react-query"; import type { IssueReaction } from "@/shared/types"; import type { IssueReactionAddedPayload, @@ -13,7 +13,7 @@ import { useWSEvent, useWSReconnect } from "@/features/realtime"; export function useIssueReactions(issueId: string, userId?: string) { const qc = useQueryClient(); - const { data: reactions = [], isLoading: loading } = useQuery( + const { data: serverReactions = [], isLoading: loading } = useQuery( issueReactionsOptions(issueId), ); @@ -26,7 +26,7 @@ export function useIssueReactions(issueId: string, userId?: string) { }, [qc, issueId]), ); - // --- WS event handlers --- + // --- WS event handlers (update server cache for other users' actions) --- useWSEvent( "issue_reaction:added", @@ -70,12 +70,62 @@ export function useIssueReactions(issueId: string, userId?: string) { ), ); + // --- Optimistic UI derivation --- + // Instead of writing temp data into the cache (which races with WS events), + // derive optimistic state at render time from pending mutation variables. + + const pendingVars = useMutationState({ + filters: { + mutationKey: ["toggleIssueReaction", issueId], + status: "pending", + }, + select: (m) => + m.state.variables as + | { emoji: string; existing: IssueReaction | undefined } + | undefined, + }); + + const reactions = useMemo(() => { + if (pendingVars.length === 0) return serverReactions; + + let result = [...serverReactions]; + for (const vars of pendingVars) { + if (!vars) continue; + if (vars.existing) { + // Pending removal + result = result.filter((r) => r.id !== vars.existing!.id); + } else { + // Pending add — skip if server already has it (WS arrived first) + const alreadyExists = result.some( + (r) => + r.emoji === vars.emoji && + r.actor_type === "member" && + r.actor_id === userId, + ); + if (!alreadyExists) { + result = [ + ...result, + { + id: `optimistic-${vars.emoji}`, + issue_id: issueId, + actor_type: "member", + actor_id: userId ?? "", + emoji: vars.emoji, + created_at: new Date().toISOString(), + }, + ]; + } + } + } + return result; + }, [serverReactions, pendingVars, issueId, userId]); + // --- Mutation --- const toggleReaction = useCallback( async (emoji: string) => { if (!userId) return; - const existing = reactions.find( + const existing = serverReactions.find( (r) => r.emoji === emoji && r.actor_type === "member" && @@ -83,7 +133,7 @@ export function useIssueReactions(issueId: string, userId?: string) { ); toggleMutation.mutate({ emoji, existing }); }, - [userId, reactions, toggleMutation], + [userId, serverReactions, toggleMutation], ); return { reactions, loading, toggleReaction }; diff --git a/apps/web/features/issues/hooks/use-issue-timeline.ts b/apps/web/features/issues/hooks/use-issue-timeline.ts index 894c2880..cd37e68d 100644 --- a/apps/web/features/issues/hooks/use-issue-timeline.ts +++ b/apps/web/features/issues/hooks/use-issue-timeline.ts @@ -1,7 +1,7 @@ "use client"; -import { useState, useCallback } from "react"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { useState, useCallback, useMemo } from "react"; +import { useQuery, useQueryClient, useMutationState } from "@tanstack/react-query"; import type { Comment, TimelineEntry, Reaction } from "@/shared/types"; import type { CommentCreatedPayload, @@ -259,9 +259,67 @@ export function useIssueTimeline(issueId: string, userId?: string) { [deleteCommentMutation], ); + // --- Optimistic UI derivation for comment reactions --- + // Instead of writing temp data into the cache (which races with WS events), + // derive optimistic state at render time from pending mutation variables. + + const pendingReactionVars = useMutationState({ + filters: { + mutationKey: ["toggleCommentReaction", issueId], + status: "pending", + }, + select: (m) => + m.state.variables as + | { commentId: string; emoji: string; existing: Reaction | undefined } + | undefined, + }); + + const optimisticTimeline = useMemo(() => { + if (pendingReactionVars.length === 0) return timeline; + + return timeline.map((entry) => { + const pendingForEntry = pendingReactionVars.filter( + (v) => v && v.commentId === entry.id, + ); + if (pendingForEntry.length === 0) return entry; + + let reactions = entry.reactions ?? []; + for (const vars of pendingForEntry) { + if (!vars) continue; + if (vars.existing) { + // Pending removal + reactions = reactions.filter((r) => r.id !== vars.existing!.id); + } else { + // Pending add — skip if server already has it (WS arrived first) + const alreadyExists = reactions.some( + (r) => + r.emoji === vars.emoji && + r.actor_type === "member" && + r.actor_id === userId, + ); + if (!alreadyExists) { + reactions = [ + ...reactions, + { + id: `optimistic-${vars.emoji}`, + comment_id: vars.commentId, + actor_type: "member", + actor_id: userId ?? "", + emoji: vars.emoji, + created_at: new Date().toISOString(), + }, + ]; + } + } + } + return { ...entry, reactions }; + }); + }, [timeline, pendingReactionVars, userId]); + const toggleReaction = useCallback( async (commentId: string, emoji: string) => { if (!userId) return; + // Read from server timeline (not optimistic) to find the real reaction const entry = timeline.find((e) => e.id === commentId); const existing: Reaction | undefined = (entry?.reactions ?? []).find( (r) => @@ -275,7 +333,7 @@ export function useIssueTimeline(issueId: string, userId?: string) { ); return { - timeline, + timeline: optimisticTimeline, loading, submitting, submitComment,