Merge remote-tracking branch 'origin/fix/silent-data-loss-batch' into release/v1.32.1

This commit is contained in:
Marc-André Bombeck committed 2026-07-23 10:27:10 +02:00
commit 0598c90af2
11 files changed
+671 -30

No files matched your search

+15
View File
@@ -2,6 +2,21 @@
## [Unreleased]
- **Dashboard layout changes can no longer be reverted by a concurrent
score-ring save.** The instant score-ring toggle now sends only the ring
fields instead of resending a cached snapshot of the whole layout, closing
a race where an in-flight ring update could land after a tile/chart Save
and silently restore the older layout.
- **Measurement-reminder cadence edits recompute against the cadence that
was actually saved.** Clearing `intervalDays` or `rrule` to switch a
reminder's schedule no longer leaves the next-due date computed off the
just-replaced value for one cycle.
- **ntfy notification settings keep a saved auth token across unrelated
edits.** Toggling the channel, or editing the server URL or topic, no
longer silently clears a previously configured token.
No migrations. No breaking changes.
## [1.32.0] — 2026-07-22
- **Provider ingestion and daily reactions now preserve the correct identity.**
@@ -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);
});
});
+14 -2
View File
@@ -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
@@ -0,0 +1,229 @@
/**
* v1.32.1 — regression for iOS coordination issue #62: the `nextDueAt`
* recomputation on `PATCH /api/measurement-reminders/{id}` used to read an
* explicit `null` cadence clear as "field omitted" (`?? existing.field`
* cannot distinguish the two — both are nullish) and recompute against the
* STALE cadence for one cycle, even though the persisted row itself already
* carried the correct cleared value.
*
* Every test below drives the REAL `computeReminderNextDueAt` (not a mock)
* so the "correct" and "stale" comparison values are genuine recurrence-
* engine output, not hand-picked dates — and asserts the fixtures actually
* differ before trusting the route's answer, so a fixture collision can't
* hide a regression.
*/
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { NextRequest } from "next/server";
vi.mock("@/lib/api-handler", () => ({
apiHandler: <T extends (...args: unknown[]) => unknown>(fn: T) => fn,
requireAuth: vi.fn(async () => ({ user: { id: "u1", locale: "en" } })),
}));
vi.mock("@/lib/auth/audit", () => ({
auditLog: vi.fn().mockResolvedValue(undefined),
}));
vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() }));
const findFirstMock = vi.fn();
const updateMock = vi.fn();
const findUserMock = vi.fn();
const auditLogCreateMock = vi.fn();
vi.mock("@/lib/db", () => ({
prisma: {
measurementReminder: {
findFirst: (...a: unknown[]) => findFirstMock(...a),
update: (...a: unknown[]) => updateMock(...a),
},
user: { findUnique: (...a: unknown[]) => findUserMock(...a) },
auditLog: { create: (...a: unknown[]) => auditLogCreateMock(...a) },
},
}));
import { PATCH } from "../route";
import { computeReminderNextDueAt } from "@/lib/measurement-reminders/scheduling";
const BASE_ROW = {
id: "r1",
userId: "u1",
label: "Blutdruck messen",
measurementType: "BLOOD_PRESSURE_SYS",
intervalDays: 30,
rrule: null as string | null,
anchorDate: null as Date | null,
endsOn: null,
origin: "VORSORGE",
notifyHour: 9,
location: null,
nextDueAt: new Date("2026-01-31T09:00:00Z"),
lastSatisfiedAt: null as Date | null,
enabled: true,
createdAt: new Date("2026-01-01T00:00:00Z"),
updatedAt: new Date("2026-01-01T00:00:00Z"),
deletedAt: null,
};
const NOW = new Date("2026-06-15T08:00:00.000Z");
const TZ = "Europe/Berlin";
function makeRequest(body: unknown): NextRequest {
return new NextRequest("http://localhost/api/measurement-reminders/r1", {
method: "PATCH",
headers: { "content-type": "application/json" },
body: JSON.stringify(body),
});
}
const params = { params: Promise.resolve({ id: "r1" }) };
beforeEach(() => {
vi.clearAllMocks();
findUserMock.mockResolvedValue({ timezone: TZ });
updateMock.mockImplementation(
async ({ data }: { data: Record<string, unknown> }) => ({
...BASE_ROW,
...data,
}),
);
vi.useFakeTimers();
vi.setSystemTime(NOW);
});
afterEach(() => {
vi.useRealTimers();
});
describe("PATCH /api/measurement-reminders/[id] — nextDueAt honours explicit-null cadence clears (regression iOS #62)", () => {
it("interval → RRULE: recomputes off the NEW rrule, not the just-cleared interval", async () => {
// Existing rolling reminder: intervalDays=30, rrule=null.
findFirstMock.mockResolvedValue(BASE_ROW);
const res = await PATCH(
makeRequest({ intervalDays: null, rrule: "FREQ=YEARLY" }),
params,
);
expect(res.status).toBe(200);
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
string,
unknown
>;
// The explicit-null clear + the new rrule both persist correctly —
// this half was never broken.
expect(persisted.intervalDays).toBeNull();
expect(persisted.rrule).toBe("FREQ=YEARLY");
const correct = computeReminderNextDueAt(
{ ...BASE_ROW, intervalDays: null, rrule: "FREQ=YEARLY" },
TZ,
NOW,
);
// The bug: `?? existing` reads the cleared intervalDays as "omitted"
// and recomputes against the OLD rolling cadence for one cycle.
const staleBug = computeReminderNextDueAt(
{ ...BASE_ROW, intervalDays: 30, rrule: null },
TZ,
NOW,
);
// Mutation-check guard: the fixtures must actually diverge, or this
// test can't tell a fix from a no-op.
expect(correct).not.toEqual(staleBug);
expect(persisted.nextDueAt).toEqual(correct);
expect(persisted.nextDueAt).not.toEqual(staleBug);
});
it("RRULE → interval: recomputes off the NEW interval, not the just-cleared rrule (incl. its BYHOUR)", async () => {
findFirstMock.mockResolvedValue({
...BASE_ROW,
intervalDays: null,
rrule: "FREQ=DAILY;BYHOUR=7,19",
});
const res = await PATCH(
makeRequest({ rrule: null, intervalDays: 14 }),
params,
);
expect(res.status).toBe(200);
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
string,
unknown
>;
expect(persisted.rrule).toBeNull();
expect(persisted.intervalDays).toBe(14);
const correct = computeReminderNextDueAt(
{ ...BASE_ROW, intervalDays: 14, rrule: null },
TZ,
NOW,
);
const staleBug = computeReminderNextDueAt(
{ ...BASE_ROW, intervalDays: null, rrule: "FREQ=DAILY;BYHOUR=7,19" },
TZ,
NOW,
);
expect(correct).not.toEqual(staleBug);
expect(persisted.nextDueAt).toEqual(correct);
expect(persisted.nextDueAt).not.toEqual(staleBug);
});
it("explicit anchorDate: null clear recomputes off no-anchor, not the stale anchor", async () => {
findFirstMock.mockResolvedValue({
...BASE_ROW,
intervalDays: 30,
anchorDate: new Date("2026-01-15T00:00:00Z"),
});
const res = await PATCH(makeRequest({ anchorDate: null }), params);
expect(res.status).toBe(200);
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
string,
unknown
>;
expect(persisted.anchorDate).toBeNull();
const correct = computeReminderNextDueAt(
{ ...BASE_ROW, intervalDays: 30, anchorDate: null },
TZ,
NOW,
);
const staleBug = computeReminderNextDueAt(
{
...BASE_ROW,
intervalDays: 30,
anchorDate: new Date("2026-01-15T00:00:00Z"),
},
TZ,
NOW,
);
expect(correct).not.toEqual(staleBug);
expect(persisted.nextDueAt).toEqual(correct);
expect(persisted.nextDueAt).not.toEqual(staleBug);
});
it("omitted cadence fields on a label-only edit leave the cadence — and nextDueAt — unchanged", async () => {
findFirstMock.mockResolvedValue(BASE_ROW);
const res = await PATCH(makeRequest({ label: "New label" }), params);
expect(res.status).toBe(200);
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
string,
unknown
>;
expect(persisted.label).toBe("New label");
// The cadence keys are never written at all on a label-only edit — no
// clear, no touch.
expect("intervalDays" in persisted).toBe(false);
expect("rrule" in persisted).toBe(false);
expect("anchorDate" in persisted).toBe(false);
const expected = computeReminderNextDueAt(BASE_ROW, TZ, NOW);
expect(persisted.nextDueAt).toEqual(expected);
});
});
@@ -133,18 +133,35 @@ export const PATCH = apiHandler(
// Recompute next-due against the merged cadence. Floor the search at
// the last-satisfied instant (or now) so a cadence edit re-anchors
// cleanly off the user's last fulfilment.
//
// v1.32.1 — `Object.hasOwn(updateData, field)` replaces a `?? existing`
// fallback here (issue #62). `??` cannot tell an explicit `null` clear
// apart from an omitted field — both are nullish — so a PATCH that
// cleared `intervalDays` to switch a reminder onto an RRULE still
// recomputed `nextDueAt` against the OLD rolling interval for one
// cycle (rolling cadence wins the dispatcher's precedence order, so the
// stale interval silently overrode the just-set RRULE). The same gap
// hit the RRULE → interval direction and an explicit `anchorDate: null`
// clear. `updateData` already encodes the field-by-field truth of what
// is ACTUALLY being persisted — including the mutual-exclusivity
// auto-clear a few lines up — so keying off its own keys (present, even
// when the value is `null`) can never diverge from what the database
// ends up holding.
const timezone = await resolveTimezone(user.id);
const now = new Date();
const merged: ReminderScheduleInput = {
intervalDays:
(updateData.intervalDays as number | null | undefined) ??
existing.intervalDays,
rrule: (updateData.rrule as string | null | undefined) ?? existing.rrule,
anchorDate:
(updateData.anchorDate as Date | null | undefined) ??
existing.anchorDate,
notifyHour:
(updateData.notifyHour as number | undefined) ?? existing.notifyHour,
intervalDays: Object.hasOwn(updateData, "intervalDays")
? (updateData.intervalDays as number | null)
: existing.intervalDays,
rrule: Object.hasOwn(updateData, "rrule")
? (updateData.rrule as string | null)
: existing.rrule,
anchorDate: Object.hasOwn(updateData, "anchorDate")
? (updateData.anchorDate as Date | null)
: existing.anchorDate,
notifyHour: Object.hasOwn(updateData, "notifyHour")
? (updateData.notifyHour as number)
: existing.notifyHour,
lastSatisfiedAt: existing.lastSatisfiedAt,
createdAt: existing.createdAt,
// v1.18.1 — preserve the course window so an edit of a finite
@@ -298,3 +298,121 @@ describe("notification channel enable-only routes", () => {
},
);
});
/**
* v1.32.1 — regression for iOS coordination issue #63: `PUT
* /api/settings/ntfy` reconstructed the entire stored config from the
* request body. `GET` never returns the write-only `authToken` (only
* `hasAuthToken: true/false`), so both native and web clients correctly
* OMIT — or round-trip an empty string through — the field on any
* unrelated edit. Rebuilding `config` without a preserve fallback
* silently dropped a working token on the next save.
*
* The fix mirrors the pre-existing `headerValue` preserve-on-empty
* contract in `src/app/api/settings/webhook/route.ts` (asserted here too,
* as a parity pin — that route was never broken, but any future drift
* between the two should fail the same suite). Covers exactly the four
* cases the report asks for.
*/
describe("PUT /api/settings/ntfy — write-only authToken preserve-on-omit contract", () => {
function upsertConfig() {
const call = vi.mocked(prisma.notificationChannel.upsert).mock
.calls[0]?.[0] as {
create: { config: string };
update: { config: string };
};
// The route always upserts — assert both arms agree, then decode one.
expect(call.update.config).toBe(call.create.config);
return JSON.parse(call.update.config.replace(/^encrypted:/, "")) as {
serverUrl: string;
topic: string;
authToken?: string;
};
}
it("an omitted authToken preserves the stored secret", async () => {
vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({
config:
'encrypted:{"serverUrl":"https://ntfy.sh","topic":"saved-topic","authToken":"secret"}',
} as never);
const response = await put("ntfy", {
serverUrl: "https://ntfy.sh",
topic: "saved-topic",
enabled: true,
});
expect(response.status).toBe(200);
expect(upsertConfig().authToken).toBe("secret");
});
it("an explicit empty-string authToken preserves the stored secret (matches current clients)", async () => {
vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({
config:
'encrypted:{"serverUrl":"https://ntfy.sh","topic":"saved-topic","authToken":"secret"}',
} as never);
const response = await put("ntfy", {
serverUrl: "https://ntfy.sh",
topic: "saved-topic",
authToken: "",
enabled: true,
});
expect(response.status).toBe(200);
expect(upsertConfig().authToken).toBe("secret");
});
it("a non-empty authToken replaces the stored secret", async () => {
vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({
config:
'encrypted:{"serverUrl":"https://ntfy.sh","topic":"saved-topic","authToken":"old-secret"}',
} as never);
const response = await put("ntfy", {
serverUrl: "https://ntfy.sh",
topic: "saved-topic",
authToken: "new-secret",
enabled: true,
});
expect(response.status).toBe(200);
expect(upsertConfig().authToken).toBe("new-secret");
});
it("creating without a token remains valid where anonymous ntfy is allowed", async () => {
vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue(null);
const response = await put("ntfy", {
serverUrl: "https://ntfy.sh",
topic: "public-topic",
enabled: false,
});
expect(response.status).toBe(200);
const config = upsertConfig();
expect(config.authToken).toBeUndefined();
expect(config.topic).toBe("public-topic");
});
it("parity pin: the webhook headerValue preserve-on-empty contract this fix mirrors is intact", async () => {
vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({
config:
'encrypted:{"url":"https://saved.example/hook","headerName":"Authorization","headerValue":"secret"}',
} as never);
const response = await put("webhook", {
url: "https://saved.example/hook",
headerName: "Authorization",
enabled: true,
});
expect(response.status).toBe(200);
const call = vi.mocked(prisma.notificationChannel.upsert).mock
.calls[0]?.[0] as { update: { config: string } };
const config = JSON.parse(
call.update.config.replace(/^encrypted:/, ""),
) as { headerValue?: string };
expect(config.headerValue).toBe("secret");
});
});
+22 -1
View File
@@ -113,10 +113,31 @@ export const PUT = apiHandler(async (request: NextRequest) => {
);
}
// v1.32.1 — preserve an existing auth token when the client sends an
// empty one (issue #63). GET never returns the secret (`hasAuthToken`
// only), so both native and web clients correctly omit — or round-trip
// an empty string through — the field on any unrelated edit (e.g.
// toggling `enabled`). Reconstructing `config` from scratch without this
// fallback silently dropped the token on every such save. Mirrors the
// identical `headerValue` preserve-on-empty contract in
// `src/app/api/settings/webhook/route.ts`. A non-empty value replaces it.
let nextAuthToken = authToken || undefined;
if (!nextAuthToken) {
const existing = await prisma.notificationChannel.findUnique({
where: { userId_type: { userId: user.id, type: "NTFY" } },
});
if (existing) {
const prev = JSON.parse(decrypt(existing.config)) as {
authToken?: string;
};
nextAuthToken = prev.authToken || undefined;
}
}
const config = JSON.stringify({
serverUrl: serverUrl || "https://ntfy.sh",
topic: topic || "",
...(authToken ? { authToken } : {}),
...(nextAuthToken ? { authToken: nextAuthToken } : {}),
});
const encryptedConfig = encrypt(config);
@@ -531,4 +531,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({
+57 -4
View File
@@ -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);
});
});
+47 -3
View File
@@ -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;
/**