Skip to content

fix(builder): guard unsaved schema changes and repair field deletion - #55

Merged
Morialkar merged 2 commits into
mainfrom
fix/builder-unsaved-guard
Jul 31, 2026
Merged

fix(builder): guard unsaved schema changes and repair field deletion#55
Morialkar merged 2 commits into
mainfrom
fix/builder-unsaved-guard

Conversation

@Morialkar

Copy link
Copy Markdown
Owner

Manual QA reported that leaving the schema builder lost work with no warning. Fixing that surfaced a second, unrelated defect in the same screen.

1. Unsaved-changes guard

The builder held every edit in local state and only persisted it through the explicit Save button. There was no dirty tracking, no router blocker and no beforeunload handler, so any navigation discarded the draft silently.

The page now keeps a snapshot of what the server holds, keyed by draft field id, and compares the live draft against it. The baseline is rebased on load, after a successful save-all, after a confirmed deletion and after a confirmed destructive type change, so only genuinely unsaved work triggers the guard. useBlocker covers in-app navigation and, through enableBeforeUnload, tab close and reload.

The canvas also advertised "Brouillon enregistré automatiquement" / "Autosaved draft" — a behaviour that does not exist. That false reassurance plausibly contributed to the reported data loss, so it is replaced by a live saved/unsaved indicator.

2. Field deletion never persisted

Found while verifying the above against the running app, and worth reviewing on its own.

SchemaChangeService::isDestructiveChange treats every field deletion as destructive, so the API rejects a DELETE without a confirmation token. The client only requested a token when preview-impact reported affected records, so deleting a field with no affected records always returned 409. deleteFieldMutation had no onError, so the failure was swallowed — the field disappeared from the canvas and reappeared on reload.

The client now always fetches the token (the impact only decides whether the user is warned first), restores the field and shows an error if the request fails, and apiClient.delete accepts a body so the token can be sent.

This also keeps the guard honest: its baseline assumes a deletion has already persisted.

Verification

Driven against the running stack, not only tests:

  • edit a field → indicator flips to "Modifications non enregistrées"; leaving raises the modal; Stay keeps the page and the edit; Leave navigates away
  • save → indicator returns to "enregistrées" and navigation is no longer blocked
  • delete a field with zero affected records → the field is gone from the API (previously 409, silently)

Also: 63/63 client tests (3 new, covering clean → dirty → blocked and the modal buttons), tsc -b, ESLint, Prettier.

Out of scope

Two adjacent defects are deliberately left for their own changes:

  • saving twice in a row duplicates fields, because created fields keep isNew locally
  • the Layout tab has its own unsaved state that this guard does not cover

🤖 Generated with Claude Code

Morialkar and others added 2 commits July 30, 2026 22:30
The structure builder held every edit in local state and only persisted it
through the explicit Save button, with no dirty tracking, no router blocker
and no beforeunload handler. Navigating away discarded the work silently.

Track a snapshot of what the server holds, keyed by draft field id, and
compare the live draft against it. The baseline is rebased on load, after a
successful save-all, after a field deletion (which persists immediately) and
after a confirmed destructive type change, so only genuinely unsaved work
triggers the guard. useBlocker covers in-app navigation and, via
enableBeforeUnload, tab close and reload.

Also replace the canvas' 'Autosaved draft' label, which claimed a behaviour
that does not exist, with a live saved/unsaved indicator.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Found while verifying the unsaved-changes guard against the running app.

The API treats every field deletion as destructive and rejects a DELETE
without a confirmation token, but the client only requested a token when
preview-impact reported affected records. A deletion with no affected
records therefore always returned 409. deleteFieldMutation had no onError,
so the failure was swallowed while the field had already been dropped from
local state: it vanished from the canvas and came back on reload.

Always fetch the confirmation token (the impact now only decides whether the
user is warned first), restore the field and show an error when the request
fails, and let apiClient.delete carry a body so the token can be sent.

This also keeps the unsaved-changes baseline honest, since it treats a
deletion as already persisted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Morialkar
Morialkar merged commit b5c1a5e into main Jul 31, 2026
5 checks passed
Morialkar added a commit that referenced this pull request Aug 1, 2026
Conflict in handleSaveAll: #55 added a savedSnapshot rebase for the
unsaved-changes guard in the same .then() this branch rewrote.

Resolved to keep both. This branch settles each save individually instead of
using a bare Promise.all, so .then() now runs even when a field fails — main's
`setSavedSnapshot(snapshotFields(fields))` would have marked failed fields
clean and silently disarmed the guard. The snapshot is therefore merged for
the fields that actually round-tripped, leaving failed ones dirty.

Also adds useBlocker to this branch's router mock, and covers the combined
behaviour: dirty clears after a successful save, and a field whose save failed
stays new and unsaved (retried as a create, not a duplicate update).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant