mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-13 22:32:04 +00:00
fix(dashboard): stop the score-ring PUT from resending a stale layout
The Settings page's instant score-ring toggle PUT the full dashboard layout, built from the client's cached `remote` snapshot. That snapshot can go stale mid-flight: if a normal tile/chart Save committed a newer layout first, the ring request's explicit stale `widgets` (and `comparisonBaseline` / `chartOverlayPrefs`) still won on write and silently reverted the just-saved layout whenever it resolved last. The ring mutation now PUTs only the two ring fields (`buildRingMutationPayload`). `widgets` joins the existing preserve-when-absent merge (`LAYOUT_FIELD_MERGE_DISPOSITION`) so an omitted field is carried forward from whatever is currently stored instead of a client-held copy, closing the race regardless of request order.
This commit is contained in:
1 parent
62eb431d66
commit
8736f44442
6 files changed
+262
-24
No files matched your search
@@ -62,7 +62,13 @@ import {
|
||||
DASHBOARD_WIDGET_IDS,
|
||||
DASHBOARD_IOS_ONLY_WIDGET_IDS,
|
||||
DASHBOARD_WIDGET_CATALOGUE_IDS,
|
||||
DEFAULT_DASHBOARD_LAYOUT,
|
||||
serializeDashboardLayout,
|
||||
// v1.32.1 — the exact payload builder `ringMutation.mutationFn` calls
|
||||
// (see the settings component). Importing the shipped function rather
|
||||
// than re-implementing its shape means this test exercises the REAL
|
||||
// client contract against the REAL route, not a hand-rolled stand-in.
|
||||
buildRingMutationPayload,
|
||||
type DashboardLayout,
|
||||
} from "@/lib/dashboard-layout";
|
||||
|
||||
@@ -636,3 +642,102 @@ describe("dashboard widgets — preserve-when-absent on PUT", () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* v1.32.1 — regression for issue #581: dashboard layout changes silently
|
||||
* overwritten by a concurrent score-ring save.
|
||||
*
|
||||
* The scenario from the report: the user edits tile/chart visibility
|
||||
* (local draft B), then reorders/toggles a hero score ring before hitting
|
||||
* Save. The ring PUT (`ringMutation`) fires immediately, built from
|
||||
* whatever layout the client had cached (layout A — the state BEFORE the
|
||||
* draft edit). The normal Save button then PUTs the full draft (layout B)
|
||||
* and commits first. If the earlier-fired ring request resolves AFTER
|
||||
* Save, its body — built from the stale A snapshot — used to carry
|
||||
* `widgets: A` explicitly, and an explicitly-present field always wins on
|
||||
* write. Save's B silently reverted to A even though the ring PUT reported
|
||||
* 200 and only meant to touch the ring selection.
|
||||
*
|
||||
* The fix is `buildRingMutationPayload` (imported from the real component
|
||||
* module, not re-implemented here): it never includes `widgets` at all.
|
||||
* This test proves the SERVER half of the fix — a PUT shaped exactly like
|
||||
* the shipped ring mutation's body preserves whatever layout is CURRENTLY
|
||||
* stored, so the request that resolves last can no longer matter.
|
||||
*/
|
||||
describe("dashboard widgets — ring-only PUT cannot race a concurrent full-layout Save (regression #581)", () => {
|
||||
it("preserves the widgets a concurrent Save already committed, even though the ring request started from a stale snapshot", async () => {
|
||||
// The state AFTER the normal Save committed layout B — a tile turned
|
||||
// off that was on in the stale snapshot A the ring mutation started
|
||||
// from. Starts from the FULL default widget catalogue (rather than a
|
||||
// single-widget array) so `resolveDashboardLayout`'s auto-upgrade
|
||||
// append (missing catalogue ids get seeded invisible) is a no-op here
|
||||
// and the persisted array can be compared for exact equality below.
|
||||
const savedLayoutB: DashboardLayout = serializeDashboardLayout({
|
||||
version: 1,
|
||||
widgets: DEFAULT_DASHBOARD_LAYOUT.widgets.map((w) =>
|
||||
w.id === "weight" ? { ...w, visible: false, tileVisible: false } : w,
|
||||
),
|
||||
comparisonBaseline: "lastYear",
|
||||
selectedScoreRings: ["MED_COMPLIANCE"],
|
||||
heroRingOrder: ["HEALTH_SCORE", "MED_COMPLIANCE"],
|
||||
});
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
dashboardWidgetsJson: savedLayoutB,
|
||||
} as never);
|
||||
vi.mocked(prisma.user.update).mockResolvedValue({} as never);
|
||||
|
||||
// The ring mutation's real, shipped payload — built without any
|
||||
// knowledge of layout B. If this were still `{...staleA, ...}` the
|
||||
// stale `widgets`/`comparisonBaseline` would land here explicitly and
|
||||
// overwrite B; `buildRingMutationPayload` never carries them.
|
||||
const ringOnlyBody = buildRingMutationPayload({
|
||||
selectedScoreRings: ["READINESS"],
|
||||
heroRingOrder: ["HEALTH_SCORE", "READINESS"],
|
||||
});
|
||||
|
||||
const res = await callPut(makeReq(ringOnlyBody));
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
const updateArg = vi.mocked(prisma.user.update).mock
|
||||
.calls[0]?.[0] as unknown as {
|
||||
data: { dashboardWidgetsJson: DashboardLayout };
|
||||
};
|
||||
// B's widgets (and comparisonBaseline) survive — the ring PUT never
|
||||
// touched them, so nothing it carries can outrace the Save that
|
||||
// already committed.
|
||||
expect(updateArg.data.dashboardWidgetsJson.widgets).toEqual(
|
||||
savedLayoutB.widgets,
|
||||
);
|
||||
expect(updateArg.data.dashboardWidgetsJson.comparisonBaseline).toBe(
|
||||
"lastYear",
|
||||
);
|
||||
// The ring selection itself DID apply.
|
||||
expect(updateArg.data.dashboardWidgetsJson.selectedScoreRings).toEqual([
|
||||
"READINESS",
|
||||
]);
|
||||
|
||||
const body = (await res.json()) as { data: DashboardLayout };
|
||||
expect(body.data.widgets[0].visible).toBe(false);
|
||||
expect(body.data.widgets[0].tileVisible).toBe(false);
|
||||
expect(body.data.selectedScoreRings).toEqual(["READINESS"]);
|
||||
});
|
||||
|
||||
it("still accepts a normal full-layout Save (widgets present + replace semantics unchanged)", async () => {
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
dashboardWidgetsJson: null,
|
||||
} as never);
|
||||
vi.mocked(prisma.user.update).mockResolvedValue({} as never);
|
||||
|
||||
const res = await callPut(
|
||||
makeReq({
|
||||
version: 1,
|
||||
widgets: [
|
||||
{ id: "weight", visible: false, tileVisible: false, order: 0 },
|
||||
],
|
||||
}),
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
const body = (await res.json()) as { data: DashboardLayout };
|
||||
expect(body.data.widgets[0].visible).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -84,7 +84,15 @@ const layoutSchema = z.object({
|
||||
// catalogue (42 ids). The cap only has to clear the catalogue size —
|
||||
// the enum bounds each entry and the id set is closed — so it carries
|
||||
// headroom rather than tracking the count exactly.
|
||||
.max(50),
|
||||
.max(50)
|
||||
// v1.32.1 — optional + preserve-when-absent (issue #581). The
|
||||
// Settings page's instant score-ring toggle now PUTs ONLY the ring
|
||||
// fields; omitting `widgets` lets the server carry forward whatever
|
||||
// is CURRENTLY stored instead of the ring mutation resending a
|
||||
// client-held snapshot that can go stale mid-flight and race a
|
||||
// concurrent full-layout Save. A normal Save still always sends
|
||||
// `widgets` — this only widens what a PARTIAL PUT can leave out.
|
||||
.optional(),
|
||||
// v1.4.16 phase B8 — comparison baseline (Vormonat / Vorjahr) rides
|
||||
// on the layout blob per research §7 Q3 (no Prisma migration). Optional
|
||||
// so v1.4.15 clients that don't know the field can still PUT.
|
||||
@@ -285,7 +293,11 @@ export const PUT = apiHandler(async (request: NextRequest) => {
|
||||
// visibility / order, while chart prefs are PUT through their own route
|
||||
// (`/api/dashboard/chart-overlay-prefs`), the hero rings come from
|
||||
// Settings, and `comparisonBaseline` is web-only — an omission from any
|
||||
// one surface must not reset a choice made on another.
|
||||
// one surface must not reset a choice made on another. `widgets` itself
|
||||
// joined this set in v1.32.1 (issue #581): the Settings page's instant
|
||||
// score-ring PUT omits it entirely so it always carries forward
|
||||
// whatever is CURRENTLY stored, rather than a client-held snapshot that
|
||||
// could be stale relative to a concurrent full-layout Save.
|
||||
//
|
||||
// The preserve set is not a list maintained here: it is derived from
|
||||
// `LAYOUT_FIELD_MERGE_DISPOSITION`, which the type system forces to cover
|
||||
|
||||
@@ -69,10 +69,7 @@ vi.mock("@/hooks/use-auth", () => ({
|
||||
}));
|
||||
|
||||
import { I18nProvider } from "@/lib/i18n/context";
|
||||
import {
|
||||
DashboardLayoutSection,
|
||||
reorderWidgets,
|
||||
} from "../dashboard-layout-section";
|
||||
import { DashboardLayoutSection, reorderWidgets } from "../dashboard-layout-section";
|
||||
|
||||
function render(node: React.ReactElement, locale: "en" | "de" = "en") {
|
||||
return renderToStaticMarkup(
|
||||
@@ -531,4 +528,19 @@ describe("<DashboardLayoutSection> — health-score anchor + reorder (v1.27.27)"
|
||||
// The order is the single source of truth: selection is derived from it.
|
||||
expect(src).toMatch(/id !== HEALTH_SCORE_RING_ID/);
|
||||
});
|
||||
|
||||
it("the ring mutation builds its body via buildRingMutationPayload, not a `...remote` spread (regression #581 source pin)", () => {
|
||||
const src = readFileSync(
|
||||
join(
|
||||
process.cwd(),
|
||||
"src/components/settings/dashboard-layout-section.tsx",
|
||||
),
|
||||
"utf8",
|
||||
);
|
||||
expect(src).toContain("buildRingMutationPayload(next)");
|
||||
// The pre-fix shape must never come back: spreading a query-cache
|
||||
// snapshot into the ring PUT body is exactly the stale-`widgets`
|
||||
// race issue #581 reported.
|
||||
expect(src).not.toMatch(/\.\.\.remote,/);
|
||||
});
|
||||
});
|
||||
@@ -47,6 +47,7 @@ import {
|
||||
MAX_SELECTED_SCORE_RINGS,
|
||||
HEALTH_SCORE_RING_ID,
|
||||
resolveHeroRingOrder,
|
||||
buildRingMutationPayload,
|
||||
} from "@/lib/dashboard-layout";
|
||||
import {
|
||||
Select,
|
||||
@@ -327,23 +328,34 @@ export function DashboardLayoutSection({ id }: { id: string }) {
|
||||
* machine the rest of the section keeps. The first cut routed ring
|
||||
* toggles through the draft, and the combination of an unflushed
|
||||
* draft, the server snapshot cache, and client staleness read as
|
||||
* "flipping does nothing, rings appear later". The mutation PUTs the
|
||||
* SERVER layout copy with only `selectedScoreRings` changed (an open
|
||||
* draft's other edits stay unsent and untouched), optimistically
|
||||
* updates the widgets query, and invalidates the dashboard snapshot on
|
||||
* settle so the hero reflects the choice on the next visit.
|
||||
* "flipping does nothing, rings appear later". The mutation
|
||||
* optimistically updates the widgets query and invalidates the
|
||||
* dashboard snapshot on settle so the hero reflects the choice on the
|
||||
* next visit.
|
||||
*
|
||||
* v1.32.1 — the PUT body used to be the FULL server layout copy,
|
||||
* spreading this component's `remote` query-cache snapshot in ahead of
|
||||
* the two ring fields. That snapshot can be stale by the time the
|
||||
* request actually lands: if the normal
|
||||
* tile/chart Save button committed a newer layout first, this
|
||||
* request's explicit — present, not omitted — stale `widgets` (and
|
||||
* `comparisonBaseline` / `chartOverlayPrefs`) still won on write and
|
||||
* silently reverted the just-saved layout (issue #581). The payload
|
||||
* now carries ONLY the ring fields; the server's preserve-when-absent
|
||||
* merge (`LAYOUT_FIELD_MERGE_DISPOSITION` — `widgets` joined it in the
|
||||
* same release) carries every omitted field forward from whatever is
|
||||
* CURRENTLY stored, so this request can no longer race a concurrent
|
||||
* Save no matter which one lands last.
|
||||
*/
|
||||
const ringMutation = useMutation({
|
||||
mutationFn: async (next: {
|
||||
selectedScoreRings: ScoreRingId[];
|
||||
heroRingOrder: HeroRingId[];
|
||||
}) => {
|
||||
if (!remote) throw new Error("layout not loaded");
|
||||
return apiPut<DashboardLayout>("/api/dashboard/widgets", {
|
||||
...remote,
|
||||
selectedScoreRings: next.selectedScoreRings,
|
||||
heroRingOrder: next.heroRingOrder,
|
||||
});
|
||||
return apiPut<DashboardLayout>(
|
||||
"/api/dashboard/widgets",
|
||||
buildRingMutationPayload(next),
|
||||
);
|
||||
},
|
||||
onMutate: async (next) => {
|
||||
await queryClient.cancelQueries({
|
||||
|
||||
@@ -13,6 +13,7 @@ import {
|
||||
PRESERVED_LAYOUT_FIELDS,
|
||||
layoutNeedsPreserveRead,
|
||||
mergePreservedLayoutFields,
|
||||
buildRingMutationPayload,
|
||||
type DashboardLayout,
|
||||
} from "@/lib/dashboard-layout";
|
||||
|
||||
@@ -1006,20 +1007,24 @@ describe("layout field merge disposition", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("preserves comparisonBaseline alongside the other client-owned fields", () => {
|
||||
it("preserves comparisonBaseline and widgets alongside the other client-owned fields", () => {
|
||||
// The regression itself: `comparisonBaseline` must be in the preserve
|
||||
// set, not merely present in the disposition map.
|
||||
// set, not merely present in the disposition map. `widgets` joined it in
|
||||
// v1.32.1 (issue #581) — see the dedicated describe block below.
|
||||
expect([...PRESERVED_LAYOUT_FIELDS].sort()).toEqual([
|
||||
"chartOverlayPrefs",
|
||||
"comparisonBaseline",
|
||||
"heroRingOrder",
|
||||
"selectedScoreRings",
|
||||
"widgets",
|
||||
]);
|
||||
});
|
||||
|
||||
it("replaces the fields the request itself owns", () => {
|
||||
it("replaces only the field every request must state", () => {
|
||||
// `version` is the sole `"replace"` field left once `widgets` joined
|
||||
// the preserve set — a PUT that omits it is rejected by Zod, so there
|
||||
// is never a stored value to fall back to.
|
||||
expect(LAYOUT_FIELD_MERGE_DISPOSITION.version).toBe("replace");
|
||||
expect(LAYOUT_FIELD_MERGE_DISPOSITION.widgets).toBe("replace");
|
||||
});
|
||||
|
||||
it("skips the stored-layout read when the client sent every preserved field", () => {
|
||||
@@ -1086,3 +1091,51 @@ describe("layout field merge disposition", () => {
|
||||
expect(merged.widgets).toEqual(fullyPopulatedLayout.widgets);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* v1.32.1 — regression for issue #581 (dashboard layout race). The
|
||||
* Settings page's instant score-ring PUT used to resend the FULL layout,
|
||||
* built from the `remote` query-cache snapshot
|
||||
* (`{...remote, selectedScoreRings, heroRingOrder}`). That snapshot can be
|
||||
* stale relative to an in-flight or already-committed tile/chart Save;
|
||||
* because `widgets` was a `"replace"`-disposition field on the server, an
|
||||
* explicitly-present stale copy always won on write and silently reverted
|
||||
* the just-saved layout.
|
||||
*
|
||||
* `buildRingMutationPayload` is the exact function
|
||||
* `ringMutation.mutationFn` (in `dashboard-layout-section.tsx`) calls to
|
||||
* build its request body. Pinning its shape here — no `widgets`, no
|
||||
* `comparisonBaseline`, no `chartOverlayPrefs` — is what makes the race
|
||||
* structurally impossible: a payload that never carries those fields
|
||||
* cannot overwrite them no matter which request lands last. See
|
||||
* `src/app/api/dashboard/widgets/__tests__/route.test.ts` for the
|
||||
* server-side half — a PUT shaped like this preserves whatever layout is
|
||||
* CURRENTLY stored, not a client-held snapshot.
|
||||
*/
|
||||
describe("buildRingMutationPayload — instant score-ring PUT payload (regression #581)", () => {
|
||||
it("carries only version + the two ring fields", () => {
|
||||
const payload = buildRingMutationPayload({
|
||||
selectedScoreRings: ["READINESS"],
|
||||
heroRingOrder: ["HEALTH_SCORE", "READINESS"],
|
||||
});
|
||||
expect(Object.keys(payload).sort()).toEqual([
|
||||
"heroRingOrder",
|
||||
"selectedScoreRings",
|
||||
"version",
|
||||
]);
|
||||
expect(payload.version).toBe(1);
|
||||
expect(payload.selectedScoreRings).toEqual(["READINESS"]);
|
||||
expect(payload.heroRingOrder).toEqual(["HEALTH_SCORE", "READINESS"]);
|
||||
});
|
||||
|
||||
it("never includes widgets, comparisonBaseline, or chartOverlayPrefs — the fields a concurrent Save owns", () => {
|
||||
const payload = buildRingMutationPayload({
|
||||
selectedScoreRings: ["READINESS"],
|
||||
heroRingOrder: ["HEALTH_SCORE", "READINESS"],
|
||||
});
|
||||
expect(payload.widgets).toBeUndefined();
|
||||
expect(payload.comparisonBaseline).toBeUndefined();
|
||||
expect(payload.chartOverlayPrefs).toBeUndefined();
|
||||
expect("widgets" in payload).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -574,8 +574,8 @@ export interface DashboardLayout {
|
||||
* omits it.
|
||||
*
|
||||
* - `"replace"` — the field is the point of the request. A PUT that omits it
|
||||
* is either rejected by Zod (`version`, `widgets` are required) or means
|
||||
* the caller genuinely has nothing to say.
|
||||
* is rejected by Zod (`version` is required) — the caller always has
|
||||
* something to say about a `"replace"` field.
|
||||
* - `"preserve"` — the field is owned by a surface OTHER than the one making
|
||||
* this request, so an omission means "I don't know about this", never
|
||||
* "clear it". The route reads the stored layout and carries the existing
|
||||
@@ -591,6 +591,18 @@ export interface DashboardLayout {
|
||||
* clamps a missing value to `"none"`, so every native layout save wiped the
|
||||
* baseline the user had picked on the web.
|
||||
*
|
||||
* `widgets` moved from `"replace"` to `"preserve"` in v1.32.1 (issue #581).
|
||||
* The Settings page's instant score-ring toggle used to resend the FULL
|
||||
* layout — including `widgets` — built from the query cache's `remote`
|
||||
* snapshot. That snapshot can be stale by the time the ring request lands:
|
||||
* if a normal tile/chart Save committed a newer layout first, the
|
||||
* ring request's explicit (present, not omitted) stale `widgets` value
|
||||
* still won on write and silently reverted the just-saved layout. Making
|
||||
* `widgets` optional + preserve-when-absent lets the ring mutation omit it
|
||||
* entirely, so the field it never touches can no longer race a concurrent
|
||||
* Save — the route always carries forward whatever is CURRENTLY stored,
|
||||
* not a client-held copy.
|
||||
*
|
||||
* `satisfies Record<keyof DashboardLayout, …>` is the load-bearing part.
|
||||
* Adding a field to `DashboardLayout` without adding it here is a TYPE error,
|
||||
* so the next field cannot repeat the `comparisonBaseline` mistake by
|
||||
@@ -598,7 +610,7 @@ export interface DashboardLayout {
|
||||
*/
|
||||
export const LAYOUT_FIELD_MERGE_DISPOSITION = {
|
||||
version: "replace",
|
||||
widgets: "replace",
|
||||
widgets: "preserve",
|
||||
comparisonBaseline: "preserve",
|
||||
chartOverlayPrefs: "preserve",
|
||||
selectedScoreRings: "preserve",
|
||||
@@ -647,6 +659,38 @@ export function mergePreservedLayoutFields(
|
||||
return merged;
|
||||
}
|
||||
|
||||
/**
|
||||
* v1.32.1 — pure payload builder for the Settings page's instant score-ring
|
||||
* PUT (`ringMutation` in `dashboard-layout-section.tsx`). Lives in this
|
||||
* client-safe module (imported by both the client component and the server
|
||||
* route already) rather than the "use client" component file, so a
|
||||
* server-side regression test can import it without pulling the component's
|
||||
* UI dependency graph across the client/server boundary — see
|
||||
* `src/app/api/dashboard/widgets/__tests__/route.test.ts`.
|
||||
*
|
||||
* Deliberately returns ONLY the ring fields (plus `version`) — no
|
||||
* `widgets`, `comparisonBaseline`, or `chartOverlayPrefs`. Before v1.32.1 the
|
||||
* ring mutation resent the FULL layout built from the client's `remote`
|
||||
* query-cache snapshot, which can be stale by the time the request lands: if
|
||||
* a normal tile/chart Save committed a newer layout first, the ring
|
||||
* request's explicit — present, not omitted — stale `widgets` still won on
|
||||
* write and silently reverted the just-saved layout (issue #581). Those
|
||||
* three fields are all `"preserve"`-disposition here
|
||||
* (`LAYOUT_FIELD_MERGE_DISPOSITION`): omitting them means the route always
|
||||
* carries forward whatever is CURRENTLY stored, so this payload can no
|
||||
* longer race a concurrent Save no matter which request lands last.
|
||||
*/
|
||||
export function buildRingMutationPayload(next: {
|
||||
selectedScoreRings: ScoreRingId[];
|
||||
heroRingOrder: HeroRingId[];
|
||||
}): Partial<DashboardLayout> {
|
||||
return {
|
||||
version: DASHBOARD_LAYOUT_VERSION,
|
||||
selectedScoreRings: next.selectedScoreRings,
|
||||
heroRingOrder: next.heroRingOrder,
|
||||
};
|
||||
}
|
||||
|
||||
const DASHBOARD_LAYOUT_VERSION = 1;
|
||||
|
||||
/**
|
||||
|
||||
Reference in new issue
Block a user