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 1/2] 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, From 17e37ec4db13c7f8812158d7151459abdc4812c8 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Wed, 8 Apr 2026 15:41:16 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix(web):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?shared=20types=20and=20stable=20optimistic=20data?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract ToggleCommentReactionVars and ToggleIssueReactionVars shared types so mutation definitions and useMutationState consumers stay in sync without as-casts on inline types - Replace new Date().toISOString() with empty string in optimistic reaction objects to avoid unstable references in useMemo Co-Authored-By: Claude Opus 4.6 (1M context) --- apps/web/core/issues/mutations.ts | 27 ++++++++++++------- .../issues/hooks/use-issue-reactions.ts | 8 +++--- .../issues/hooks/use-issue-timeline.ts | 7 +++-- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/apps/web/core/issues/mutations.ts b/apps/web/core/issues/mutations.ts index 678978f9..85c15166 100644 --- a/apps/web/core/issues/mutations.ts +++ b/apps/web/core/issues/mutations.ts @@ -11,6 +11,22 @@ import type { } from "@/shared/types"; import type { TimelineEntry, IssueSubscriber, Reaction } from "@/shared/types"; +// --------------------------------------------------------------------------- +// Shared mutation variable types — used by both mutation hooks and +// useMutationState consumers to keep the type assertion in sync. +// --------------------------------------------------------------------------- + +export type ToggleCommentReactionVars = { + commentId: string; + emoji: string; + existing: Reaction | undefined; +}; + +export type ToggleIssueReactionVars = { + emoji: string; + existing: IssueReaction | undefined; +}; + // --------------------------------------------------------------------------- // Done issue pagination // --------------------------------------------------------------------------- @@ -339,11 +355,7 @@ export function useToggleCommentReaction(issueId: string) { commentId, emoji, existing, - }: { - commentId: string; - emoji: string; - existing: Reaction | undefined; - }) => { + }: ToggleCommentReactionVars) => { if (existing) { await api.removeReaction(commentId, emoji); return null; @@ -367,10 +379,7 @@ export function useToggleIssueReaction(issueId: string) { mutationFn: async ({ emoji, existing, - }: { - emoji: string; - existing: IssueReaction | undefined; - }) => { + }: ToggleIssueReactionVars) => { if (existing) { await api.removeIssueReaction(issueId, emoji); return null; diff --git a/apps/web/features/issues/hooks/use-issue-reactions.ts b/apps/web/features/issues/hooks/use-issue-reactions.ts index 44a5a30f..6435d77a 100644 --- a/apps/web/features/issues/hooks/use-issue-reactions.ts +++ b/apps/web/features/issues/hooks/use-issue-reactions.ts @@ -8,7 +8,7 @@ import type { IssueReactionRemovedPayload, } from "@/shared/types"; import { issueReactionsOptions, issueKeys } from "@core/issues/queries"; -import { useToggleIssueReaction } from "@core/issues/mutations"; +import { useToggleIssueReaction, type ToggleIssueReactionVars } from "@core/issues/mutations"; import { useWSEvent, useWSReconnect } from "@/features/realtime"; export function useIssueReactions(issueId: string, userId?: string) { @@ -80,9 +80,7 @@ export function useIssueReactions(issueId: string, userId?: string) { status: "pending", }, select: (m) => - m.state.variables as - | { emoji: string; existing: IssueReaction | undefined } - | undefined, + m.state.variables as ToggleIssueReactionVars | undefined, }); const reactions = useMemo(() => { @@ -111,7 +109,7 @@ export function useIssueReactions(issueId: string, userId?: string) { actor_type: "member", actor_id: userId ?? "", emoji: vars.emoji, - created_at: new Date().toISOString(), + created_at: "", }, ]; } diff --git a/apps/web/features/issues/hooks/use-issue-timeline.ts b/apps/web/features/issues/hooks/use-issue-timeline.ts index cd37e68d..c21baa58 100644 --- a/apps/web/features/issues/hooks/use-issue-timeline.ts +++ b/apps/web/features/issues/hooks/use-issue-timeline.ts @@ -17,6 +17,7 @@ import { useUpdateComment, useDeleteComment, useToggleCommentReaction, + type ToggleCommentReactionVars, } from "@core/issues/mutations"; import { useWSEvent, useWSReconnect } from "@/features/realtime"; import { toast } from "sonner"; @@ -269,9 +270,7 @@ export function useIssueTimeline(issueId: string, userId?: string) { status: "pending", }, select: (m) => - m.state.variables as - | { commentId: string; emoji: string; existing: Reaction | undefined } - | undefined, + m.state.variables as ToggleCommentReactionVars | undefined, }); const optimisticTimeline = useMemo(() => { @@ -306,7 +305,7 @@ export function useIssueTimeline(issueId: string, userId?: string) { actor_type: "member", actor_id: userId ?? "", emoji: vars.emoji, - created_at: new Date().toISOString(), + created_at: "", }, ]; }