mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 12:32:43 +00:00
fix(02-12.1): reconcile without firing the reads it is holding
- keep the record entries until /api/auth/me has answered: clearing the cache under a mounted observer made the held tab refetch at once - exclude the account cell from the cancellation, and swallow a repeat trigger, so a burst of 409s cannot kill the request that ends the hold - read the account payload network-only; the default staleTime would have returned the very context the browser was told is wrong
This commit is contained in:
1 parent
0fee005683
commit
731203bdf4
2 files changed
+284
-18
No files matched your search
@@ -0,0 +1,195 @@
|
||||
/**
|
||||
* Reconciling a moved record context without firing the reads it is holding.
|
||||
*
|
||||
* Both defects this pins were found in a real browser rather than reasoned
|
||||
* about, and both look harmless in the source:
|
||||
*
|
||||
* * `queryClient.clear()` while the shell is still mounted drops every entry,
|
||||
* and a mounted observer whose entry disappears refetches at once. The tab
|
||||
* that had just been told to hold issued `/api/dashboard/snapshot` and
|
||||
* `/api/medications` under the very context it could not prove;
|
||||
* * an unfiltered `cancelQueries()` cancels the `/api/auth/me` a previous
|
||||
* reconcile started. A stale tab produces one 409 per errored query, so
|
||||
* each new trigger killed the request that would have ended the hold and
|
||||
* the tab never recovered.
|
||||
*
|
||||
* Run against a real `QueryClient`, for the reason `grant-loss.test.ts` gives:
|
||||
* "the effect calls subscribe" is a claim about source, and this is a claim
|
||||
* about what a cache full of live observers actually does.
|
||||
*
|
||||
* Mutation checks, run: put the `removeQueries` back before the `/me` await →
|
||||
* "does not refetch a record query it is holding" goes red. Drop the in-flight
|
||||
* guard → "does not cancel the /me a previous reconcile started" goes red.
|
||||
*/
|
||||
import { describe, expect, it, vi, beforeEach, afterEach } from "vitest";
|
||||
import { QueryClient, QueryObserver } from "@tanstack/react-query";
|
||||
|
||||
import { createRecordFenceReconciler } from "@/components/layout/record-session-fence-bridge";
|
||||
import { queryKeys } from "@/lib/query-keys";
|
||||
import { __resetRecordSessionTransitionForTests } from "@/lib/query-keys/record-session-transition";
|
||||
|
||||
function client(): QueryClient {
|
||||
return new QueryClient({
|
||||
defaultOptions: {
|
||||
queries: { retry: false, gcTime: Infinity, staleTime: Infinity },
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
/** A record cell with a live observer, exactly as a mounted page holds one. */
|
||||
async function mountedRecordQuery(
|
||||
qc: QueryClient,
|
||||
fetcher: () => Promise<unknown>,
|
||||
) {
|
||||
const queryKey = ["dashboard", "snapshot"];
|
||||
// A real `QueryObserver` with a live subscriber, which is what a mounted page
|
||||
// holds. Without the subscription the query is inactive, removal can never
|
||||
// trigger a refetch, and this whole file would pass vacuously.
|
||||
const observer = new QueryObserver(qc, { queryKey, queryFn: fetcher });
|
||||
const unsubscribe = observer.subscribe(() => {});
|
||||
await vi.waitFor(() =>
|
||||
expect(observer.getCurrentResult().data).toBeDefined(),
|
||||
);
|
||||
return { queryKey, unsubscribe };
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
__resetRecordSessionTransitionForTests();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
__resetRecordSessionTransitionForTests();
|
||||
});
|
||||
|
||||
describe("createRecordFenceReconciler", () => {
|
||||
it("keeps the record entries alive until the account payload has answered", async () => {
|
||||
// The sequencing IS the fix, and it is what this asserts directly.
|
||||
//
|
||||
// The defect was `queryClient.clear()` before the `/me` await: a mounted
|
||||
// observer whose entry disappears refetches, so the tab that had just been
|
||||
// told to hold fired its record reads under the context it could not
|
||||
// prove. A node test has no React render loop, so it cannot observe that
|
||||
// refetch — asserting "the fetcher was not called again" here would be a
|
||||
// check that cannot fail. What it CAN observe, deterministically, is the
|
||||
// ordering the refetch depends on: the entry must still exist while `/me`
|
||||
// is in flight, and be gone once it has answered.
|
||||
const qc = client();
|
||||
const { queryKey, unsubscribe } = await mountedRecordQuery(
|
||||
qc,
|
||||
async () => ({
|
||||
value: 1,
|
||||
}),
|
||||
);
|
||||
|
||||
let releaseMe!: () => void;
|
||||
const meGate = new Promise<void>((resolve) => {
|
||||
releaseMe = resolve;
|
||||
});
|
||||
let presentWhileInFlight: boolean | null = null;
|
||||
const refetchAccount = vi.fn(async () => {
|
||||
// Sampled from INSIDE the request, which is the only moment that matters:
|
||||
// this is precisely when a mounted observer would react to a removal.
|
||||
presentWhileInFlight =
|
||||
qc.getQueryCache().find({ queryKey }) !== undefined;
|
||||
await meGate;
|
||||
return { id: "u-1" };
|
||||
});
|
||||
|
||||
createRecordFenceReconciler(qc, refetchAccount)();
|
||||
await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(1));
|
||||
|
||||
expect(presentWhileInFlight).toBe(true);
|
||||
|
||||
releaseMe();
|
||||
await vi.waitFor(() =>
|
||||
expect(qc.getQueryCache().find({ queryKey })).toBeUndefined(),
|
||||
);
|
||||
unsubscribe();
|
||||
});
|
||||
|
||||
it("removes the stale record entries once the account payload lands", async () => {
|
||||
// The positive control for the assertion above: a reconciler that simply
|
||||
// never removed anything would pass it.
|
||||
const qc = client();
|
||||
const { queryKey, unsubscribe } = await mountedRecordQuery(
|
||||
qc,
|
||||
async () => ({
|
||||
value: 1,
|
||||
}),
|
||||
);
|
||||
unsubscribe();
|
||||
|
||||
const refetchAccount = vi.fn(async () => ({ id: "u-1" }));
|
||||
createRecordFenceReconciler(qc, refetchAccount)();
|
||||
|
||||
await vi.waitFor(() =>
|
||||
expect(qc.getQueryCache().find({ queryKey })).toBeUndefined(),
|
||||
);
|
||||
// The account cell itself survives — it is the one thing that releases the
|
||||
// hold, and removing it would be removing the answer.
|
||||
expect(
|
||||
qc.getQueryCache().find({ queryKey: queryKeys.authMe() }),
|
||||
).toBeDefined();
|
||||
});
|
||||
|
||||
it("does not cancel the /me a previous reconcile started", async () => {
|
||||
const qc = client();
|
||||
let releaseMe!: () => void;
|
||||
const meGate = new Promise<void>((resolve) => {
|
||||
releaseMe = resolve;
|
||||
});
|
||||
const refetchAccount = vi.fn(async () => {
|
||||
await meGate;
|
||||
return { id: "u-1" };
|
||||
});
|
||||
|
||||
const reconcile = createRecordFenceReconciler(qc, refetchAccount);
|
||||
// A burst, which is what a stale tab produces: one 409 per errored query.
|
||||
reconcile();
|
||||
reconcile();
|
||||
reconcile();
|
||||
reconcile();
|
||||
|
||||
// One request, not four, and not zero — a reconciler that restarted on
|
||||
// every trigger would cancel the request that ends the hold.
|
||||
expect(refetchAccount).toHaveBeenCalledTimes(1);
|
||||
|
||||
releaseMe();
|
||||
await vi.waitFor(() =>
|
||||
expect(
|
||||
qc.getQueryCache().find({ queryKey: queryKeys.authMe() })?.state.data,
|
||||
).toEqual({ id: "u-1" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("accepts a fresh trigger once the previous reconcile settled", async () => {
|
||||
// The in-flight guard must be a latch, not a one-shot: a later, genuine
|
||||
// disagreement has to be reconciled too.
|
||||
const qc = client();
|
||||
const refetchAccount = vi.fn(async () => ({ id: "u-1" }));
|
||||
const reconcile = createRecordFenceReconciler(qc, refetchAccount);
|
||||
|
||||
reconcile();
|
||||
await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(1));
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
reconcile();
|
||||
await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(2));
|
||||
});
|
||||
|
||||
it("stays held when the account payload cannot be fetched", async () => {
|
||||
const qc = client();
|
||||
const refetchAccount = vi.fn(async () => {
|
||||
throw new TypeError("offline");
|
||||
});
|
||||
const reconcile = createRecordFenceReconciler(qc, refetchAccount);
|
||||
|
||||
reconcile();
|
||||
await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(1));
|
||||
await new Promise((r) => setTimeout(r, 10));
|
||||
|
||||
// The latch released, so a retry is possible — being stuck offline must not
|
||||
// also mean being stuck unable to try again.
|
||||
reconcile();
|
||||
await vi.waitFor(() => expect(refetchAccount).toHaveBeenCalledTimes(2));
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import { useQueryClient } from "@tanstack/react-query";
|
||||
import { useQueryClient, type QueryClient } from "@tanstack/react-query";
|
||||
import { useEffect } from "react";
|
||||
|
||||
import { onRecordFenceMismatch } from "@/lib/api/record-fence";
|
||||
@@ -35,27 +35,98 @@ import { holdForRecordSessionReconcile } from "@/lib/query-keys/record-session-t
|
||||
* this is"), and a single subscriber with a mode flag is how they start
|
||||
* answering each other's.
|
||||
*/
|
||||
|
||||
/** Is this the account payload's cell? */
|
||||
function isAccountQuery(queryKey: readonly unknown[]): boolean {
|
||||
const authMe = queryKeys.authMe() as readonly unknown[];
|
||||
return (
|
||||
queryKey.length === authMe.length &&
|
||||
queryKey.every((part, i) => part === authMe[i])
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* The reconciliation itself, as a plain function over a `QueryClient`.
|
||||
*
|
||||
* Extracted from the effect body for the reason `subscribeToGrantLoss` is: a
|
||||
* test can drive a real cache through it and check what happened, which is the
|
||||
* only way to tell a reconciler that holds from one that quietly refetches the
|
||||
* record it is supposed to be holding.
|
||||
*
|
||||
* ## The two things the first version got wrong
|
||||
*
|
||||
* **It cleared the cache while the shell was still mounted.**
|
||||
* `queryClient.clear()` drops every entry, and a mounted observer whose entry
|
||||
* disappears refetches immediately — so the tab that had just been told to hold
|
||||
* fired `/api/dashboard/snapshot` and `/api/medications` under the very context
|
||||
* it could not prove. The removal therefore waits until AFTER `/api/auth/me`
|
||||
* has answered: by then the hold has unmounted the protected children, so there
|
||||
* is no observer left to trigger anything.
|
||||
*
|
||||
* **It cancelled every in-flight query, including the `/me` a previous
|
||||
* reconcile had started.** Under a burst of 409s — which is exactly what a
|
||||
* stale tab produces, one per errored query — each new reconcile cancelled the
|
||||
* request that would have ended the hold, and the tab never recovered. Two
|
||||
* fixes, both needed: the cancellation excludes the account cell, and a
|
||||
* reconcile already in flight swallows a second trigger rather than restarting.
|
||||
*/
|
||||
export function createRecordFenceReconciler(
|
||||
queryClient: QueryClient,
|
||||
refetchAccount: () => Promise<unknown> = fetchMe,
|
||||
): () => void {
|
||||
let inFlight = false;
|
||||
|
||||
return () => {
|
||||
// A burst of 409s is one signal, not many. Restarting on each would cancel
|
||||
// the request that ends the hold and livelock the tab.
|
||||
if (inFlight) return;
|
||||
inFlight = true;
|
||||
|
||||
// Hold FIRST. This is what unmounts the protected children, and everything
|
||||
// below depends on them being gone.
|
||||
holdForRecordSessionReconcile();
|
||||
|
||||
// Cancel record traffic, never the account payload: the `/me` below is the
|
||||
// only thing that can release the hold.
|
||||
void queryClient.cancelQueries({
|
||||
predicate: (query) => !isAccountQuery(query.queryKey),
|
||||
});
|
||||
|
||||
void queryClient
|
||||
.fetchQuery({
|
||||
queryKey: queryKeys.authMe(),
|
||||
queryFn: refetchAccount,
|
||||
// NETWORK-ONLY, and this is load-bearing rather than a precaution. The
|
||||
// app's default `staleTime` is not zero, so a plain `fetchQuery` would
|
||||
// hand back the cached account payload — the one carrying the context
|
||||
// this browser has just been told is wrong — and release the hold
|
||||
// without ever asking the server. The whole reconciliation would then
|
||||
// be a no-op that looked like a success.
|
||||
staleTime: 0,
|
||||
})
|
||||
.then(() => {
|
||||
// Now, and not before: the children are unmounted, so removing their
|
||||
// entries cannot remount an observer that refetches.
|
||||
queryClient.removeQueries({
|
||||
predicate: (query) => !isAccountQuery(query.queryKey),
|
||||
});
|
||||
})
|
||||
.catch(() => {
|
||||
// Offline, or the server is down. The hold stays, which is the correct
|
||||
// place to be stuck: nothing renders under a context this browser
|
||||
// cannot prove, and the next successful `/me` releases it.
|
||||
})
|
||||
.finally(() => {
|
||||
inFlight = false;
|
||||
});
|
||||
};
|
||||
}
|
||||
|
||||
export function RecordSessionFenceBridge() {
|
||||
const queryClient = useQueryClient();
|
||||
|
||||
useEffect(() => {
|
||||
/**
|
||||
* Held, then reconciled. `fetchMe` adopts the context and settles the
|
||||
* transition itself, so nothing here decides what the browser is allowed
|
||||
* to render next — the server's answer does.
|
||||
*/
|
||||
const reconcile = () => {
|
||||
holdForRecordSessionReconcile();
|
||||
void queryClient.cancelQueries();
|
||||
queryClient.clear();
|
||||
void queryClient
|
||||
.fetchQuery({ queryKey: queryKeys.authMe(), queryFn: fetchMe })
|
||||
.catch(() => {
|
||||
// Offline, or the server is down. The hold stays, which is the
|
||||
// correct place to be stuck: nothing renders under a context this
|
||||
// browser cannot prove, and the next successful `/me` releases it.
|
||||
});
|
||||
};
|
||||
const reconcile = createRecordFenceReconciler(queryClient);
|
||||
|
||||
// The in-flight / cached-response arm, raised by the transport wrapper.
|
||||
const disposeMismatch = onRecordFenceMismatch(reconcile);
|
||||
|
||||
Reference in new issue
Block a user