From dab4a968432269fa59dd0dc75e865d0d4ccd7942 Mon Sep 17 00:00:00 2001 From: snowyukitty Date: Sat, 15 Aug 2026 05:44:26 +0900 Subject: [PATCH] fix: prevent comparison swap refetches (#200) Co-authored-by: snowyukitty <270071858+snowyukitty@users.noreply.github.com> --- components/home-page-client.tsx | 163 ++++++++++++++++++-------------- lib/compare-request.ts | 107 +++++++++++++++++++++ test/ui/compare-request.test.ts | 132 ++++++++++++++++++++++++++ 3 files changed, 330 insertions(+), 72 deletions(-) create mode 100644 lib/compare-request.ts create mode 100644 test/ui/compare-request.test.ts diff --git a/components/home-page-client.tsx b/components/home-page-client.tsx index c5edf71..692ca47 100644 --- a/components/home-page-client.tsx +++ b/components/home-page-client.tsx @@ -18,6 +18,13 @@ import { SafeApiError, } from "@/types/api-response"; import { cn } from "@/lib/utils"; +import { + createComparisonQuery, + createComparisonRequest, + isComparisonFetchDuplicate, + reconcileComparisonData, + sanitizeSelectedLanguages, +} from "@/lib/compare-request"; type ComparisonData = { user1: UserResult; @@ -45,26 +52,6 @@ type UsernameErrors = { const EXIT_ANIMATION_MS = 240; -function sanitizeSelectedLanguages(languages: string[]): string[] { - const seen = new Set(); - const output: string[] = []; - - for (const language of languages) { - const trimmed = language.trim(); - const normalized = trimmed.toLowerCase(); - if (!trimmed || seen.has(normalized)) { - continue; - } - output.push(trimmed); - seen.add(normalized); - if (output.length >= 5) { - break; - } - } - - return output; -} - function normalizeUsers(body: ApiResponse): { user1: UserResult; user2: UserResult } | null { if (body.users && body.users.length >= 2) { return { user1: body.users[0], user2: body.users[1] }; @@ -100,6 +87,9 @@ export function HomePageClient() { const lastFetchedKeyRef = useRef(null); const inFlightFetchKeyRef = useRef(null); const inFlightPromiseRef = useRef | null>(null); + const latestRequestRef = useRef( + createComparisonRequest(initialUsername1, initialUsername2, initialSelectedLanguages), + ); const hideTimerRef = useRef(null); const localizeErrorMessage = (message?: string, details?: SafeApiError) => { @@ -206,24 +196,14 @@ export function HomePageClient() { setGeneralError(localizedMessage); }; - const createFetchKey = ( - u1: string, - u2: string, - options: CompareOptions, - ) => - JSON.stringify({ - u1, - u2, - selectedLanguages: [...sanitizeSelectedLanguages(options.selectedLanguages)].sort(), - }); - const handleCompare = async ( u1: string, u2: string, options: CompareOptions, ) => { - const sanitizedLanguages = sanitizeSelectedLanguages(options.selectedLanguages); - const fetchKey = createFetchKey(u1, u2, options); + const request = createComparisonRequest(u1, u2, options.selectedLanguages); + latestRequestRef.current = request; + const fetchKey = request.fetchKey; if (inFlightFetchKeyRef.current === fetchKey && inFlightPromiseRef.current) { return inFlightPromiseRef.current; @@ -231,51 +211,57 @@ export function HomePageClient() { // If we've already fetched this exact comparison and have the data, skip. if (lastFetchedKeyRef.current === fetchKey && data) { + const reconciled = reconcileComparisonData(data, fetchKey, request); + if (reconciled) { + setData(reconciled); + setDisplayData(reconciled); + } return Promise.resolve(); } lastFetchedKeyRef.current = fetchKey; // update duplicate fetch state for current form values + const currentFetchKey = createComparisonRequest( + username1, + username2, + selectedLanguages, + ).fetchKey; setDisableDuplicateFetch( - Boolean(lastFetchedKeyRef.current === createFetchKey(username1.trim(), username2.trim(), { selectedLanguages }) && (data || inFlightFetchKeyRef.current === fetchKey)), + isComparisonFetchDuplicate( + currentFetchKey, + lastFetchedKeyRef.current, + inFlightFetchKeyRef.current, + Boolean(data), + ), ); const requestPromise = (async () => { if (options.updateUrl !== false) { - const params = new URLSearchParams(); - params.append("username", u1); - params.append("username", u2); - for (const language of sanitizedLanguages) { - params.append("selectedLanguage", language); - } - router.push(`/?${params.toString()}`, { scroll: false }); + router.push(`/?${createComparisonQuery(request)}`, { scroll: false }); } setLoading(true); resetErrors(); try { - const requestParams = new URLSearchParams(); - requestParams.append("username", u1); - requestParams.append("username", u2); - for (const language of sanitizedLanguages) { - requestParams.append("selectedLanguage", language); - } - - const res = await fetch(`/api/compare?${requestParams.toString()}`); + const res = await fetch(`/api/compare?${createComparisonQuery(request)}`); const body: ApiResponse = await res.json(); if (!res.ok) { + if (latestRequestRef.current.fetchKey !== fetchKey) { + return; + } setData(null); - applyApiError(u1, u2, body); + applyApiError(latestRequestRef.current.user1, latestRequestRef.current.user2, body); return; } const users = normalizeUsers(body); if (!body.success || !users) { + if (latestRequestRef.current.fetchKey !== fetchKey) return; setData(null); - applyApiError(u1, u2, body); + applyApiError(latestRequestRef.current.user1, latestRequestRef.current.user2, body); return; } @@ -296,9 +282,25 @@ export function HomePageClient() { scoreVersion: body.scoreVersion, }; - setData(nextData); - setDisplayData(nextData); + const reconciled = reconcileComparisonData( + nextData, + fetchKey, + latestRequestRef.current, + ); + if (!reconciled) { + if (latestRequestRef.current.fetchKey === fetchKey) { + setData(null); + setGeneralError(t("error.generic")); + } + return; + } + + setData(reconciled); + setDisplayData(reconciled); } catch (err: unknown) { + if (latestRequestRef.current.fetchKey !== fetchKey) { + return; + } setData(null); setUsernameErrors({ username1: null, @@ -309,8 +311,8 @@ export function HomePageClient() { if (inFlightFetchKeyRef.current === fetchKey) { inFlightFetchKeyRef.current = null; inFlightPromiseRef.current = null; + setLoading(false); } - setLoading(false); } })(); @@ -319,7 +321,12 @@ export function HomePageClient() { // mark duplicate fetch disabled while request is in-flight setDisableDuplicateFetch( - Boolean(lastFetchedKeyRef.current === createFetchKey(username1.trim(), username2.trim(), { selectedLanguages }) && (data || inFlightFetchKeyRef.current === fetchKey)), + isComparisonFetchDuplicate( + currentFetchKey, + lastFetchedKeyRef.current, + inFlightFetchKeyRef.current, + Boolean(data), + ), ); return requestPromise; @@ -332,6 +339,7 @@ export function HomePageClient() { setSelectedLanguages(languages); if (!u1 || !u2) { + latestRequestRef.current = createComparisonRequest(u1, u2, languages); lastFetchedKeyRef.current = null; setData(null); resetErrors(); @@ -339,14 +347,6 @@ export function HomePageClient() { return; } - const nextKey = createFetchKey(u1, u2, { - selectedLanguages: languages, - }); - - if (lastFetchedKeyRef.current === nextKey && data) { - return; - } - void handleCompare(u1, u2, { selectedLanguages: languages, updateUrl: false, @@ -399,14 +399,21 @@ export function HomePageClient() { useEffect(() => { - const currentFetchKey = createFetchKey(username1.trim(), username2.trim(), { + const currentFetchKey = createComparisonRequest( + username1, + username2, selectedLanguages, - }); + ).fetchKey; const lastKey = lastFetchedKeyRef.current; const inFlightKey = inFlightFetchKeyRef.current; - const disabled = Boolean(lastKey === currentFetchKey && (data || inFlightKey === currentFetchKey)); + const disabled = isComparisonFetchDuplicate( + currentFetchKey, + lastKey, + inFlightKey, + Boolean(data), + ); setDisableDuplicateFetch(disabled); }, [username1, username2, selectedLanguages, data, loading]); @@ -430,6 +437,7 @@ export function HomePageClient() { resetErrors(); inFlightFetchKeyRef.current = null; inFlightPromiseRef.current = null; + latestRequestRef.current = createComparisonRequest("", "", []); setDisableDuplicateFetch(false); setUsername1(""); setUsername2(""); @@ -440,16 +448,27 @@ export function HomePageClient() { const swapUsers = () => { const nextUsername1 = username2; const nextUsername2 = username1; + const nextRequest = createComparisonRequest( + nextUsername1, + nextUsername2, + selectedLanguages, + ); + latestRequestRef.current = nextRequest; setUsername1(nextUsername1); setUsername2(nextUsername2); - router.push( - `/?username=${encodeURIComponent(nextUsername1)}&username=${encodeURIComponent(nextUsername2)}`, - { scroll: false }, - ); + router.push(`/?${createComparisonQuery(nextRequest)}`, { scroll: false }); - if (!data) return; - setData((current) => (current ? { ...current, user1: current.user2, user2: current.user1 } : current)); + setData((current) => + current + ? reconcileComparisonData(current, nextRequest.fetchKey, nextRequest) + : current, + ); + setDisplayData((current) => + current + ? reconcileComparisonData(current, nextRequest.fetchKey, nextRequest) + : current, + ); }; return ( diff --git a/lib/compare-request.ts b/lib/compare-request.ts new file mode 100644 index 0000000..49710c5 --- /dev/null +++ b/lib/compare-request.ts @@ -0,0 +1,107 @@ +const MAX_SELECTED_LANGUAGES = 5; +export type ComparisonPresentationRequest = { + user1: string; + user2: string; + selectedLanguages: string[]; + fetchKey: string; +}; +type ComparisonUser = { + username: string; +}; +type ComparisonData = { + user1: TUser; + user2: TUser; +}; +function normalizeUsername(username: string): string { + return username.trim().toLowerCase(); +} +export function sanitizeSelectedLanguages(languages: string[]): string[] { + const seen = new Set(); + const output: string[] = []; + for (const language of languages) { + const trimmed = language.trim(); + const normalized = trimmed.toLowerCase(); + if (!trimmed || seen.has(normalized)) { + continue; + } + + output.push(trimmed); + seen.add(normalized); + if (output.length >= MAX_SELECTED_LANGUAGES) { + break; + } + } + return output; +} +export function createComparisonRequest( + user1: string, + user2: string, + selectedLanguages: string[], +): ComparisonPresentationRequest { + const sanitizedLanguages = sanitizeSelectedLanguages(selectedLanguages); + const canonicalUsers = [normalizeUsername(user1), normalizeUsername(user2)].sort(); + const canonicalLanguages = sanitizedLanguages + .map((language) => language.toLowerCase()) + .sort(); + return { + user1: user1.trim(), + user2: user2.trim(), + selectedLanguages: sanitizedLanguages, + fetchKey: JSON.stringify({ + users: canonicalUsers, + selectedLanguages: canonicalLanguages, + }), + }; +} +export function createComparisonQuery(request: ComparisonPresentationRequest): string { + const params = new URLSearchParams(); + params.append("username", request.user1); + params.append("username", request.user2); + for (const language of request.selectedLanguages) { + params.append("selectedLanguage", language); + } + return params.toString(); +} +export function isComparisonFetchDuplicate( + currentFetchKey: string, + lastFetchedKey: string | null, + inFlightFetchKey: string | null, + hasData: boolean, +): boolean { + return lastFetchedKey === currentFetchKey && (hasData || inFlightFetchKey === currentFetchKey); +} +export function swapComparisonRequest( + request: ComparisonPresentationRequest, +): ComparisonPresentationRequest { + return createComparisonRequest(request.user2, request.user1, request.selectedLanguages); +} +export function reconcileComparisonData< + TUser extends ComparisonUser, + TData extends ComparisonData, +>( + data: TData, + responseFetchKey: string, + latestRequest: ComparisonPresentationRequest, +): TData | null { + if (responseFetchKey !== latestRequest.fetchKey) { + return null; + } + + const users = [data.user1, data.user2]; + const firstUsername = normalizeUsername(latestRequest.user1); + const secondUsername = normalizeUsername(latestRequest.user2); + if (!firstUsername || !secondUsername || firstUsername === secondUsername) { + return null; + } + + const first = users.find((user) => normalizeUsername(user.username) === firstUsername); + const second = users.find((user) => normalizeUsername(user.username) === secondUsername); + if (!first || !second || first === second) { + return null; + } + return { + ...data, + user1: first, + user2: second, + }; +} diff --git a/test/ui/compare-request.test.ts b/test/ui/compare-request.test.ts new file mode 100644 index 0000000..0901c4a --- /dev/null +++ b/test/ui/compare-request.test.ts @@ -0,0 +1,132 @@ +import { describe, expect, test } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { + createComparisonQuery, + createComparisonRequest, + isComparisonFetchDuplicate, + reconcileComparisonData, + sanitizeSelectedLanguages, + swapComparisonRequest, +} from "@/lib/compare-request"; + +function comparison(user1: string, user2: string) { + return { + user1: { username: user1, score: 1 }, + user2: { username: user2, score: 2 }, + marker: "preserved", + }; +} +describe("comparison request identity", () => { + test("treats a username swap and language reordering as the same fetch", () => { + const original = createComparisonRequest(" Alice ", "BOB", ["TypeScript", "Rust"]); + const swapped = createComparisonRequest("bob", "alice", ["rust", "typescript"]); + + expect(swapped.fetchKey).toBe(original.fetchKey); + }); + + test("keeps true user and language changes distinct", () => { + const original = createComparisonRequest("alice", "bob", ["TypeScript"]); + + expect(createComparisonRequest("alice", "carol", ["TypeScript"]).fetchKey).not.toBe(original.fetchKey); + expect(createComparisonRequest("alice", "bob", ["Rust"]).fetchKey).not.toBe(original.fetchKey); + }); + + test("preserves ordered presentation and sanitized languages in the query", () => { + const request = createComparisonRequest(" bob ", "alice", [ + " TypeScript ", + "typescript", + "Rust", + "", + ]); + const params = new URLSearchParams(createComparisonQuery(request)); + + expect(params.getAll("username")).toEqual(["bob", "alice"]); + expect(params.getAll("selectedLanguage")).toEqual(["TypeScript", "Rust"]); + }); + + test("keeps sanitizer limits and case-insensitive deduplication", () => { + expect( + sanitizeSelectedLanguages([" A ", "a", "B", "C", "D", "E", "F"]), + ).toEqual(["A", "B", "C", "D", "E"]); + }); + + test("two swaps restore the original presentation and identity", () => { + const original = createComparisonRequest("alice", "bob", ["Go", "Rust"]); + const restored = swapComparisonRequest(swapComparisonRequest(original)); + + expect(restored).toEqual(original); + }); + + test("preserves duplicate-submit disabling for data and in-flight requests", () => { + const original = createComparisonRequest("alice", "bob", ["Go"]); + const swapped = swapComparisonRequest(original); + + expect(isComparisonFetchDuplicate(swapped.fetchKey, original.fetchKey, null, true)).toBe( + true, + ); + expect(isComparisonFetchDuplicate(swapped.fetchKey, original.fetchKey, original.fetchKey, false)).toBe(true); + expect(isComparisonFetchDuplicate(swapped.fetchKey, null, null, false)).toBe(false); + }); +}); + +describe("comparison response reconciliation", () => { + test("reorders history data to the latest presentation without refetching", () => { + const original = createComparisonRequest("alice", "bob", ["Go"]); + const history = createComparisonRequest("bob", "alice", ["Go"]); + const data = comparison("alice", "bob"); + + const nextData = reconcileComparisonData(data, original.fetchKey, history); + const nextDisplayData = reconcileComparisonData(data, original.fetchKey, history); + + expect(nextData?.user1.username).toBe("bob"); + expect(nextData?.user2.username).toBe("alice"); + expect(nextDisplayData).toEqual(nextData); + expect(nextData?.marker).toBe("preserved"); + }); + + test("uses the latest presentation when a swap occurs in flight", () => { + const started = createComparisonRequest("alice", "bob", ["Go"]); + const latest = swapComparisonRequest(started); + const response = comparison("alice", "bob"); + + expect(reconcileComparisonData(response, started.fetchKey, latest)).toMatchObject({ + user1: { username: "bob" }, + user2: { username: "alice" }, + }); + }); + + test("rejects a completion for a different latest canonical identity", () => { + const started = createComparisonRequest("alice", "bob", ["Go"]); + const latest = createComparisonRequest("alice", "carol", ["Go"]); + + expect( + reconcileComparisonData(comparison("alice", "bob"), started.fetchKey, latest), + ).toBeNull(); + }); + + test("rejects malformed responses that cannot satisfy the latest order", () => { + const latest = createComparisonRequest("alice", "bob", []); + + expect( + reconcileComparisonData(comparison("alice", "carol"), latest.fetchKey, latest), + ).toBeNull(); + }); + + test("binds asynchronous completion to the latest presentation ref", () => { + const source = readFileSync( + resolve(process.cwd(), "components", "home-page-client.tsx"), + "utf8", + ); + + expect(source).toMatch( + /reconcileComparisonData\(\s*nextData,\s*fetchKey,\s*latestRequestRef\.current/, + ); + expect(source).toMatch(/if \(!body\.success \|\| !users\) \{\s*if \(latestRequestRef\.current\.fetchKey !== fetchKey\)/); + expect(source).toMatch(/const reset = \(\) => \{[\s\S]*?latestRequestRef\.current = createComparisonRequest\("", "", \[\]\)/); + expect(source).toMatch(/if \(!res\.ok\) \{\s*if \(latestRequestRef\.current\.fetchKey !== fetchKey\)/); + expect(source).toMatch(/catch \(err: unknown\) \{\s*if \(latestRequestRef\.current\.fetchKey !== fetchKey\)/); + expect(source).toMatch(/applyApiError\(latestRequestRef\.current\.user1, latestRequestRef\.current\.user2, body\)/); + expect(source).toMatch(/if \(!reconciled\) \{\s*if \(latestRequestRef\.current\.fetchKey === fetchKey\) \{\s*setData\(null\);\s*setGeneralError\(t\("error\.generic"\)\)/); + }); +});