fix(web): seed the settings draft from the tracker, and don't re-apply a rejected one

Review round 8 on #2091, both findings — the two remaining places the
stale `servers` entry and the unpersisted draft still leaked.

The modal draft was seeded straight from `servers`, and the draft is the
input to the *next* write: after a write landed whose list reload failed,
opening the modal and saving any unrelated field sent the superseded
value back to disk. It now seeds through the tracker, which falls back to
the entry when nothing has landed, so the ordinary case is unchanged.

`useSettingsDraft` also keeps the draft when a save rejects, and closing
the modal unconditionally pushed that draft into the live client — so the
client returned to the value that had just failed to reach disk, undoing
the rollback that reported the failure. The server whose last modal save
rejected is now remembered, and close applies what landed for it instead
(the roots diff and the fetch-log resize included). The flag clears on
the next save for that server that lands, and on the close that acts on
it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 13:36:21 -04:00
commit ba633fc8d2

View file

@ -1628,6 +1628,9 @@ describe("App task wiring", () => {
type RootsFakeClient = EventTarget & {
setRoots: ReturnType<typeof vi.fn>;
getRoots: ReturnType<typeof vi.fn>;
// Close also pushes the settings it decided to apply, which is what the
// failed-save case asserts on (#2089).
setServerSettings: ReturnType<typeof vi.fn>;
};
const settingsWithRoots = (
@ -1718,6 +1721,47 @@ describe("App roots live-apply on settings-dialog close", () => {
);
expect(client.setRoots).not.toHaveBeenCalled();
});
it("applies the last persisted settings on close after a save failed (#2089)", async () => {
// `useSettingsDraft` keeps the draft when a save rejects, so closing the
// modal would push a value that never reached disk into the live client —
// undoing the rollback that reported the failure. Close has to apply what
// landed instead.
const user = userEvent.setup();
updateServerSettingsSpy.mockClear();
const failedDraft: InspectorServerSettings = {
...settingsWithRoots([{ uri: "file:///rejected" }]),
paginatedLists: true,
};
const client = await openSettingsForConnectedServer(failedDraft);
client.getRoots.mockReturnValue([]);
const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
if (!draftOptions) throw new Error("useSettingsDraft was never called");
// One save lands, so the tracker has an account of disk…
const persistedSettings: InspectorServerSettings = {
...settingsWithRoots([]),
paginatedLists: false,
};
await act(async () => {
await draftOptions.onPersist("A", persistedSettings);
});
// …then the next one rejects, leaving the draft holding an unsaved edit.
updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full"));
await act(async () => {
await draftOptions.onPersist("A", failedDraft).catch(() => undefined);
});
client.setServerSettings.mockClear();
await closeModal(user);
await waitFor(() => expect(client.setServerSettings).toHaveBeenCalled());
expect(client.setServerSettings).toHaveBeenLastCalledWith(
persistedSettings,
);
// …and the rejected roots edit is not advertised either.
expect(client.setRoots).not.toHaveBeenCalled();
});
});
describe("App history pin/replay", () => {
@ -2713,6 +2757,32 @@ describe("App paginated list pagination toggle (#1721)", () => {
);
});
it("seeds the settings modal from the last write, not the stale entry (#2089)", async () => {
// The modal draft is the input to the *next* write, so seeding it from a
// `servers` entry that a failed list reload froze would send the superseded
// value back to disk the moment the user saves any unrelated field.
const user = userEvent.setup();
renderWithMantine(<App />);
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(1));
await user.click(screen.getByText("paginated-on"));
await waitFor(() =>
expect(updateServerSettingsSpy).toHaveBeenCalledWith(
"A",
expect.objectContaining({ paginatedLists: true }),
),
);
// `servers` is mocked to a fixed entry here, so it still reports the
// pre-toggle value — exactly what a failed reload leaves behind.
const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
if (!draftOptions) throw new Error("useSettingsDraft was never called");
expect(draftOptions.resolveInitial("A")).toEqual(
expect.objectContaining({ paginatedLists: true }),
);
});
it("lets a successful list read supersede a concrete rollback override (#2089)", async () => {
// A rollback sets the override to a value rather than clearing it, so the
// effect that drops it cannot key on the persisted boolean alone: if a