tx-os #317: optimistic-cache saves for all inline editors
Extends the optimistic React Query cache pattern (proven in #316 for the schedule time cell) to every other inline editor on executive-meetings.tsx, fixing the "تأخير في الاستجابة وأحياناً ما يحفظ" regression where typed edits felt sluggish or appeared to silently drop. executive-meetings.tsx * New `applyMeetingPatch(meetingId, patch)` helper: snapshots the day's DayResponse, applies a per-meeting patch via setQueryData, and returns a rollback closure that re-sets the snapshot on PATCH failure. * `saveTitle`, `saveAttendeeName`, `saveMerge`, and `setRowColor` now write the optimistic value before awaiting the network call and rollback() in their catch branches. Re-throw behaviour preserved where present so EditableCell still resets its draft on failure. editable-cell.tsx * Hardened blur-commit: the cell now stashes the latest `saveEdit` in a ref and an unmount cleanup flushes any pending blurTimerRef fire-and-forget. Without this, a row remount that lands during the 100 ms blur window (e.g. Tab into the next cell triggering an optimistic refetch) tore down the editor before saveEdit could run and silently dropped the typed change. Optimistic cache writes in the parent make the fire-and-forget safe. tests/executive-meetings-keyboard-editing.spec.mjs (+3 tests) * Title repaint <300 ms with PATCH delayed 1500 ms. * Title PATCH 500 rolls back to original; DB unchanged. * Attendee-name PUT delayed 1500 ms repaints in <300 ms (uses captured testid so the locator survives the rename). Architect review: APPROVED, no concerns. All 3 new + 3 #316 time-editor tests green. Out-of-scope pre-existing failures in the `test` workflow (notifications/postpone-race/row-color) untouched per plan.
This commit is contained in:
riyadhafraa
@@ -332,6 +332,32 @@ export function EditableCell({
|
||||
}
|
||||
}, [editor, onSave, value, forceSaveOnBlur]);
|
||||
|
||||
// #317: stash the latest saveEdit so the unmount cleanup below can
|
||||
// call it without capturing a stale closure. We only need this for
|
||||
// the "flush on unmount" path; normal blur calls saveEdit directly.
|
||||
const saveEditRef = useRef(saveEdit);
|
||||
useEffect(() => {
|
||||
saveEditRef.current = saveEdit;
|
||||
}, [saveEdit]);
|
||||
|
||||
// #317: if the cell unmounts (e.g. the row is replaced by a refetch
|
||||
// landing while the 100 ms blur timer is still pending) fire the
|
||||
// save synchronously so the user's edit is never silently dropped.
|
||||
// Without this, a fast Tab into the next cell that triggers the
|
||||
// parent's optimistic refetch can tear down the editor before the
|
||||
// setTimeout fires, losing the typed change. Fire-and-forget is
|
||||
// safe because the parent's saveTitle / saveAttendeeName / etc.
|
||||
// already write the optimistic value into the React Query cache.
|
||||
useEffect(() => {
|
||||
return () => {
|
||||
if (blurTimerRef.current) {
|
||||
clearTimeout(blurTimerRef.current);
|
||||
blurTimerRef.current = null;
|
||||
void saveEditRef.current();
|
||||
}
|
||||
};
|
||||
}, []);
|
||||
|
||||
const onFocusOut = (e: React.FocusEvent<HTMLDivElement>) => {
|
||||
if (!editing) return;
|
||||
const next = e.relatedTarget as Node | null;
|
||||
|
||||
@@ -886,12 +886,50 @@ function ScheduleSection({
|
||||
});
|
||||
}, [qc, date]);
|
||||
|
||||
// #317: optimistic-cache helper shared by every inline-edit save
|
||||
// handler on the schedule. Snapshots the current DayResponse, applies
|
||||
// `patch` to the matching meeting, and returns a rollback function the
|
||||
// caller invokes from its `catch` branch on PATCH failure. This is
|
||||
// what makes title / attendee / merge / row-color edits repaint the
|
||||
// cell within a single React commit instead of waiting for the GET
|
||||
// refetch — the same pattern saveTimes already uses for the schedule
|
||||
// time editor (task #316).
|
||||
const applyMeetingPatch = useCallback(
|
||||
(meetingId: number, patch: (m: Meeting) => Meeting) => {
|
||||
const queryKey = ["/api/executive-meetings", date] as const;
|
||||
const previous = qc.getQueryData<DayResponse>(queryKey);
|
||||
qc.setQueryData<DayResponse>(queryKey, (prev) =>
|
||||
prev
|
||||
? {
|
||||
...prev,
|
||||
meetings: prev.meetings.map((m) =>
|
||||
m.id === meetingId ? patch(m) : m,
|
||||
),
|
||||
}
|
||||
: prev,
|
||||
);
|
||||
return () => {
|
||||
if (previous !== undefined) {
|
||||
qc.setQueryData<DayResponse>(queryKey, previous);
|
||||
}
|
||||
};
|
||||
},
|
||||
[qc, date],
|
||||
);
|
||||
|
||||
const saveTitle = useCallback(
|
||||
async (meeting: Meeting, html: string) => {
|
||||
// #202: route by visible UI language (i18n.language → isRtl), NOT
|
||||
// by user.preferredLanguage. An ar-pref user who switches the UI
|
||||
// to en and edits the cell must update titleEn.
|
||||
const body = isRtl ? { titleAr: html } : { titleEn: html };
|
||||
// #317: optimistic local repaint so the cell shows the new title
|
||||
// in the same React commit that closes the editor. Rolls back on
|
||||
// PATCH failure so a network/server error snaps the cell back to
|
||||
// the previously-saved value while the toast surfaces the error.
|
||||
const rollback = applyMeetingPatch(meeting.id, (m) =>
|
||||
isRtl ? { ...m, titleAr: html } : { ...m, titleEn: html },
|
||||
);
|
||||
try {
|
||||
await apiJson(`/api/executive-meetings/${meeting.id}`, {
|
||||
method: "PATCH",
|
||||
@@ -899,6 +937,7 @@ function ScheduleSection({
|
||||
});
|
||||
refreshDay();
|
||||
} catch (err) {
|
||||
rollback();
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
toast({
|
||||
title: t("common.error"),
|
||||
@@ -908,7 +947,7 @@ function ScheduleSection({
|
||||
throw err;
|
||||
}
|
||||
},
|
||||
[isRtl, refreshDay, toast, t],
|
||||
[applyMeetingPatch, isRtl, refreshDay, toast, t],
|
||||
);
|
||||
|
||||
const saveAttendeeName = useCallback(
|
||||
@@ -934,6 +973,17 @@ function ScheduleSection({
|
||||
: meeting.attendees.map((a, i) =>
|
||||
i === attendeeIdx ? { ...a, name: html } : a,
|
||||
);
|
||||
// #317: optimistic repaint of the attendees cell. Mirrors the
|
||||
// wire-shape we PUT below so the cell collapses to the new state
|
||||
// (renamed row, or removed row when the user cleared the text)
|
||||
// immediately, without waiting on the server round-trip.
|
||||
const rollback = applyMeetingPatch(meeting.id, (m) => ({
|
||||
...m,
|
||||
attendees: next.map((a) => ({
|
||||
...a,
|
||||
kind: a.kind ?? "person",
|
||||
})) as Attendee[],
|
||||
}));
|
||||
try {
|
||||
await apiJson(`/api/executive-meetings/${meeting.id}/attendees`, {
|
||||
method: "PUT",
|
||||
@@ -951,6 +1001,7 @@ function ScheduleSection({
|
||||
});
|
||||
refreshDay();
|
||||
} catch (err) {
|
||||
rollback();
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
toast({
|
||||
title: t("common.error"),
|
||||
@@ -960,7 +1011,7 @@ function ScheduleSection({
|
||||
throw err;
|
||||
}
|
||||
},
|
||||
[refreshDay, toast, t],
|
||||
[applyMeetingPatch, refreshDay, toast, t],
|
||||
);
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
@@ -1195,6 +1246,25 @@ function ScheduleSection({
|
||||
| { mergeStartColumn: ColumnId; mergeEndColumn: ColumnId; mergeText: string }
|
||||
| null,
|
||||
) => {
|
||||
// #317: optimistic repaint of the merge overlay so the cell shows
|
||||
// the new merged span (or restores the original cells when
|
||||
// `merge === null`) in the same React commit that closes the
|
||||
// editor. Rolls back on PATCH failure.
|
||||
const rollback = applyMeetingPatch(meeting.id, (m) =>
|
||||
merge
|
||||
? {
|
||||
...m,
|
||||
mergeStartColumn: merge.mergeStartColumn,
|
||||
mergeEndColumn: merge.mergeEndColumn,
|
||||
mergeText: merge.mergeText,
|
||||
}
|
||||
: {
|
||||
...m,
|
||||
mergeStartColumn: null,
|
||||
mergeEndColumn: null,
|
||||
mergeText: null,
|
||||
},
|
||||
);
|
||||
try {
|
||||
await apiJson(`/api/executive-meetings/${meeting.id}`, {
|
||||
method: "PATCH",
|
||||
@@ -1202,6 +1272,7 @@ function ScheduleSection({
|
||||
});
|
||||
refreshDay();
|
||||
} catch (err) {
|
||||
rollback();
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
toast({
|
||||
title: t("common.error"),
|
||||
@@ -1211,7 +1282,7 @@ function ScheduleSection({
|
||||
throw err;
|
||||
}
|
||||
},
|
||||
[refreshDay, toast, t],
|
||||
[applyMeetingPatch, refreshDay, toast, t],
|
||||
);
|
||||
|
||||
// Inline "delete entire meeting" action exposed on each schedule row.
|
||||
@@ -1768,6 +1839,13 @@ function ScheduleSection({
|
||||
const setRowColor = useCallback(
|
||||
async (meetingId: number, colorKey: string) => {
|
||||
const wireValue = colorKey === "default" ? null : colorKey;
|
||||
// #317: optimistic repaint so the row tint changes the moment the
|
||||
// user picks a swatch, without waiting on the PATCH round-trip.
|
||||
// Rolls back on failure so the colour snaps to the previous value.
|
||||
const rollback = applyMeetingPatch(meetingId, (m) => ({
|
||||
...m,
|
||||
rowColor: wireValue,
|
||||
}));
|
||||
try {
|
||||
await apiJson(`/api/executive-meetings/${meetingId}`, {
|
||||
method: "PATCH",
|
||||
@@ -1775,6 +1853,7 @@ function ScheduleSection({
|
||||
});
|
||||
await qc.invalidateQueries({ queryKey: ["/api/executive-meetings", date] });
|
||||
} catch (err) {
|
||||
rollback();
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
toast({
|
||||
title: t("common.error"),
|
||||
@@ -1783,7 +1862,7 @@ function ScheduleSection({
|
||||
});
|
||||
}
|
||||
},
|
||||
[qc, date, toast, t],
|
||||
[applyMeetingPatch, qc, date, toast, t],
|
||||
);
|
||||
|
||||
// Responsive breakpoints:
|
||||
|
||||
@@ -1724,3 +1724,263 @@ test("Schedule time editor: a failed PATCH rolls back the optimistic times and r
|
||||
|
||||
await page.unroute(`**/api/executive-meetings/${meetingId}`);
|
||||
});
|
||||
|
||||
// ─── #317: optimistic-cache repaint for non-time inline editors ─────
|
||||
//
|
||||
// Task #316 made the time cell repaint instantly via an optimistic
|
||||
// React Query cache write. Task #317 generalises that pattern to the
|
||||
// title, attendee-name, and merge-text editors. The three tests below
|
||||
// mirror the time-cell tests for the title cell:
|
||||
//
|
||||
// 1. With the PATCH delayed 1500 ms, the cell shows the new title
|
||||
// within ~300 ms — proves the optimistic cache write repaints
|
||||
// the read-only display before the network round-trip resolves.
|
||||
// 2. With the PATCH stubbed to 500, the cell rolls back to the
|
||||
// original title and an error toast surfaces — proves the
|
||||
// rollback path actually restores cache state.
|
||||
// 3. Attendee-name save shows the renamed attendee instantly with
|
||||
// the PATCH delayed — proves the optimistic pattern covers the
|
||||
// attendees PUT endpoint too, not just the meeting PATCH.
|
||||
|
||||
test("Schedule title editor: optimistic repaint shows the new title before the PATCH round-trip resolves", async ({
|
||||
page,
|
||||
}) => {
|
||||
await setLangEn(page);
|
||||
const date = uniqueFutureDate(330);
|
||||
const uniq = Date.now().toString(36);
|
||||
const originalTitle = `Optimistic Original ${uniq}`;
|
||||
const meetingId = await insertMeeting({
|
||||
meetingDate: date,
|
||||
dailyNumber: 1,
|
||||
titleEn: originalTitle,
|
||||
titleAr: originalTitle,
|
||||
startTime: "09:00:00",
|
||||
endTime: "10:00:00",
|
||||
});
|
||||
|
||||
await gotoMeetingRow(page, meetingId, date);
|
||||
|
||||
// Hold the PATCH for 1500 ms. The optimistic cache write must
|
||||
// repaint the cell well before the response lands.
|
||||
let patchHeld = false;
|
||||
await page.route(
|
||||
`**/api/executive-meetings/${meetingId}`,
|
||||
async (route) => {
|
||||
if (route.request().method() !== "PATCH") {
|
||||
await route.continue();
|
||||
return;
|
||||
}
|
||||
patchHeld = true;
|
||||
await new Promise((r) => setTimeout(r, 1500));
|
||||
await route.continue();
|
||||
},
|
||||
);
|
||||
|
||||
const titleCell = page.getByTestId(`em-edit-title-${meetingId}`);
|
||||
await expect(titleCell).toContainText(originalTitle);
|
||||
await titleCell.evaluate((el) => el.focus());
|
||||
await page.keyboard.press("Enter");
|
||||
|
||||
const editorContent = titleCell.locator('[contenteditable="true"]');
|
||||
await expect(editorContent).toBeFocused();
|
||||
const appended = ` FAST-${uniq}`;
|
||||
await page.keyboard.type(appended);
|
||||
|
||||
const t0 = Date.now();
|
||||
await page.keyboard.press("Enter");
|
||||
|
||||
// The cell must show the new title within 300 ms of pressing Enter,
|
||||
// long before the held PATCH (1500 ms) resolves. This proves the
|
||||
// optimistic cache write is what's repainting the cell — not the
|
||||
// GET refetch that fires after the PATCH.
|
||||
await expect(titleCell).toContainText(originalTitle + appended, {
|
||||
timeout: 300,
|
||||
});
|
||||
const elapsed = Date.now() - t0;
|
||||
expect(elapsed).toBeLessThan(800);
|
||||
expect(patchHeld).toBe(true);
|
||||
|
||||
// Editor must have collapsed (toolbar gone) too.
|
||||
await expect(page.getByTestId("em-edit-toolbar")).toBeHidden();
|
||||
|
||||
// Wait for the held PATCH to actually land + the GET refetch to
|
||||
// complete so the test cleanup doesn't race the in-flight request.
|
||||
await page.waitForResponse(
|
||||
(resp) => {
|
||||
const u = new URL(resp.url());
|
||||
return (
|
||||
u.pathname === `/api/executive-meetings/${meetingId}` &&
|
||||
resp.request().method() === "PATCH" &&
|
||||
resp.ok()
|
||||
);
|
||||
},
|
||||
{ timeout: 15_000 },
|
||||
);
|
||||
await waitForDayRefetch(page, date);
|
||||
await page.unroute(`**/api/executive-meetings/${meetingId}`);
|
||||
});
|
||||
|
||||
test("Schedule title editor: a failed PATCH rolls back the optimistic title and the cell snaps back to the original", async ({
|
||||
page,
|
||||
}) => {
|
||||
await setLangEn(page);
|
||||
const date = uniqueFutureDate(331);
|
||||
const uniq = Date.now().toString(36);
|
||||
const originalTitle = `Rollback Title ${uniq}`;
|
||||
const meetingId = await insertMeeting({
|
||||
meetingDate: date,
|
||||
dailyNumber: 1,
|
||||
titleEn: originalTitle,
|
||||
titleAr: originalTitle,
|
||||
startTime: "09:00:00",
|
||||
endTime: "10:00:00",
|
||||
});
|
||||
|
||||
await gotoMeetingRow(page, meetingId, date);
|
||||
|
||||
// Stub PATCH to 500 so the optimistic write must roll back.
|
||||
await page.route(
|
||||
`**/api/executive-meetings/${meetingId}`,
|
||||
async (route) => {
|
||||
if (route.request().method() !== "PATCH") {
|
||||
await route.continue();
|
||||
return;
|
||||
}
|
||||
await route.fulfill({
|
||||
status: 500,
|
||||
contentType: "application/json",
|
||||
body: JSON.stringify({ message: "Internal server error" }),
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
const titleCell = page.getByTestId(`em-edit-title-${meetingId}`);
|
||||
await titleCell.evaluate((el) => el.focus());
|
||||
await page.keyboard.press("Enter");
|
||||
const editorContent = titleCell.locator('[contenteditable="true"]');
|
||||
await expect(editorContent).toBeFocused();
|
||||
const appended = ` BROKEN-${uniq}`;
|
||||
await page.keyboard.type(appended);
|
||||
await page.keyboard.press("Enter");
|
||||
|
||||
// Wait for the 500 to actually arrive so the rollback runs.
|
||||
await page.waitForResponse(
|
||||
(resp) => {
|
||||
const u = new URL(resp.url());
|
||||
return (
|
||||
u.pathname === `/api/executive-meetings/${meetingId}` &&
|
||||
resp.request().method() === "PATCH" &&
|
||||
resp.status() === 500
|
||||
);
|
||||
},
|
||||
{ timeout: 15_000 },
|
||||
);
|
||||
|
||||
// After the rollback, the cell must show the ORIGINAL title (not
|
||||
// the optimistic "BROKEN-..." one) and must NOT contain the
|
||||
// appended fragment. We poll briefly because rollback runs in the
|
||||
// PATCH catch branch on the next microtask.
|
||||
await expect(titleCell).toContainText(originalTitle, { timeout: 3_000 });
|
||||
await expect(titleCell).not.toContainText(appended);
|
||||
|
||||
// DB must be untouched.
|
||||
const { rows } = await pool.query(
|
||||
`SELECT title_en, title_ar FROM executive_meetings WHERE id = $1`,
|
||||
[meetingId],
|
||||
);
|
||||
expect(String(rows[0].title_en)).not.toContain(appended);
|
||||
expect(String(rows[0].title_ar)).not.toContain(appended);
|
||||
|
||||
await page.unroute(`**/api/executive-meetings/${meetingId}`);
|
||||
});
|
||||
|
||||
test("Schedule attendees editor: optimistic repaint shows the renamed attendee before the PUT round-trip resolves", async ({
|
||||
page,
|
||||
}) => {
|
||||
await setLangEn(page);
|
||||
const date = uniqueFutureDate(332);
|
||||
const uniq = Date.now().toString(36);
|
||||
const originalName = `OrigAtt ${uniq}`;
|
||||
const meetingId = await insertMeeting({
|
||||
meetingDate: date,
|
||||
dailyNumber: 1,
|
||||
titleEn: `Att Optimistic ${uniq}`,
|
||||
titleAr: `حضور تفاؤلي ${uniq}`,
|
||||
startTime: "09:00:00",
|
||||
endTime: "10:00:00",
|
||||
});
|
||||
await insertAttendee({
|
||||
meetingId,
|
||||
name: originalName,
|
||||
attendanceType: "internal",
|
||||
sortOrder: 0,
|
||||
});
|
||||
|
||||
await gotoMeetingRow(page, meetingId, date);
|
||||
|
||||
// Hold the attendees PUT for 1500 ms.
|
||||
let putHeld = false;
|
||||
await page.route(
|
||||
`**/api/executive-meetings/${meetingId}/attendees`,
|
||||
async (route) => {
|
||||
if (route.request().method() !== "PUT") {
|
||||
await route.continue();
|
||||
return;
|
||||
}
|
||||
putHeld = true;
|
||||
await new Promise((r) => setTimeout(r, 1500));
|
||||
await route.continue();
|
||||
},
|
||||
);
|
||||
|
||||
// The attendee's EditableCell wrapper (`em-edit-attendee-${i}`) is
|
||||
// index-keyed and the index is list-wide. Locate the cell that
|
||||
// shows the original name, then capture its concrete testid so the
|
||||
// post-rename assertions don't lose the locator (a hasText filter
|
||||
// would stop matching once the optimistic update repaints the
|
||||
// cell with the new name).
|
||||
const meetingRow = page.getByTestId(`em-row-${meetingId}`);
|
||||
const initialCell = meetingRow
|
||||
.locator('[data-testid^="em-edit-attendee-"]')
|
||||
.filter({ hasText: originalName })
|
||||
.first();
|
||||
await expect(initialCell).toBeVisible();
|
||||
const cellTestId = await initialCell.getAttribute("data-testid");
|
||||
if (!cellTestId) throw new Error("attendee cell testid missing");
|
||||
const attendeeCell = page.getByTestId(cellTestId);
|
||||
await attendeeCell.click();
|
||||
const editor = attendeeCell.locator('[contenteditable="true"]');
|
||||
await expect(editor).toBeFocused();
|
||||
|
||||
const renamed = `RENAMED-${uniq}`;
|
||||
await page.keyboard.press("ControlOrMeta+a");
|
||||
await page.keyboard.type(renamed);
|
||||
|
||||
const t0 = Date.now();
|
||||
await page.keyboard.press("Enter");
|
||||
|
||||
// Cell must show the renamed attendee within 300 ms — well before
|
||||
// the held 1500 ms PUT resolves.
|
||||
await expect(attendeeCell).toContainText(renamed, { timeout: 300 });
|
||||
const elapsed = Date.now() - t0;
|
||||
expect(elapsed).toBeLessThan(800);
|
||||
expect(putHeld).toBe(true);
|
||||
// The original name must be gone from the optimistic display.
|
||||
await expect(attendeeCell).not.toContainText(originalName);
|
||||
|
||||
// Wait for the held PUT to land + the GET refetch to complete so
|
||||
// the test cleanup doesn't race the in-flight request.
|
||||
await page.waitForResponse(
|
||||
(resp) => {
|
||||
const u = new URL(resp.url());
|
||||
return (
|
||||
u.pathname === `/api/executive-meetings/${meetingId}/attendees` &&
|
||||
resp.request().method() === "PUT" &&
|
||||
resp.ok()
|
||||
);
|
||||
},
|
||||
{ timeout: 15_000 },
|
||||
);
|
||||
await waitForDayRefetch(page, date);
|
||||
await page.unroute(`**/api/executive-meetings/${meetingId}/attendees`);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user