Skip to content

fix(core): honour localChangesBackup: false - #945

Merged
fgatti675 merged 1 commit into
firecmsco:mainfrom
dropletalexbeu:fix/local-changes-backup-false
Jul 31, 2026
Merged

fix(core): honour localChangesBackup: false#945
fgatti675 merged 1 commit into
firecmsco:mainfrom
dropletalexbeu:fix/local-changes-backup-false

Conversation

@dropletalexbeu

Copy link
Copy Markdown
Contributor

Fixes #944.

EntityCollection.localChangesBackup documents three states, but false could not actually be used by consumers. Two changes, both one-liners.

1. false was unreachable

packages/firecms_core/src/util/collections.ts

-    if (!collection.localChangesBackup) {
+    if (collection.localChangesBackup === undefined) {
         return "manual_apply";
     }

false is falsy, so it took the same branch as undefined and always resolved to the "manual_apply" default. The declared type "manual_apply" | "auto_apply" | false admits three states while the implementation could only ever return two, so buildCollection({ localChangesBackup: false }) silently behaved as "manual_apply" and still showed the restore prompt it opted out of.

2. The backup write was never gated by the flag

packages/firecms_core/src/form/EntityForm.tsx

-            if (controller.dirty) {
+            if (controller.dirty && localChangesBackup !== false) {

onValuesChangeDeferred called saveEntityToCache on every deferred change without consulting the flag. Since the documented meaning of false is that local changes "will not be backed up", and backing up is exactly what that call does, fixing only the check above would disable applying cached drafts while still writing them to localStorage on every keystroke. localChangesBackup is already in scope (declared a few lines above), so no extra plumbing is needed.

This also matters for collections holding large or sensitive field values, where opting out of persisting form content to localStorage is the point.

Behaviour with localChangesBackup: false

  • no draft values written to localStorage
  • no cached draft applied to the form
  • no restore prompt / LocalChangesMenu

Values other than false are unaffected, and an unset flag still defaults to "manual_apply".

Tests

Adds packages/firecms_core/src/util/__tests__/collections.test.ts covering all three states plus the default. The false case is a genuine regression test — reverting the collections.ts change makes it fail with:

● getLocalChangesBackup › should return false when explicitly disabled, and not fall back to the default
    Expected: false
    Received: "manual_apply"

npx jest in packages/firecms_core goes from 230 to 234 passing tests. The 4 failing suites (VirtualTable.performance, date_conversion, navigation, validation — 3 failing tests) fail identically on an unmodified main checkout and are unrelated to this change.

Notes

There is currently no supported workaround: getLocalChangesBackup is the only symbol exported from this area, while clearEntityCache and removeEntityFromCache are internal, so consumers can neither opt out nor clear the cache through the public API.

🤖 Generated with Claude Code

`EntityCollection.localChangesBackup` documents three states, but `false`
could not be used by consumers.

`getLocalChangesBackup` tested the value for falsiness, so `false` took the
same branch as `undefined` and always resolved to the `"manual_apply"`
default. The declared type admits three states while the implementation
could only ever return two, meaning a collection that opted out still got
the restore prompt.

Separately, the backup write in `EntityForm.onValuesChangeDeferred` never
read the flag, so `saveEntityToCache` persisted to `localStorage` on every
deferred change regardless. Since the documented meaning of `false` is that
local changes "will not be backed up", fixing the check alone would have
stopped cached drafts being applied while still writing them on every
keystroke. `localChangesBackup` is already in scope, so the write is now
gated on it.

Adds unit tests covering all three states plus the default. The
`false` case fails before this change with
`Expected: false, Received: "manual_apply"`.

Fixes firecmsco#944

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes EntityCollection.localChangesBackup so that the documented false state is actually respected by the runtime behavior in core form handling, and adds regression tests to prevent the issue from reappearing.

Changes:

  • Fix getLocalChangesBackup so false is no longer treated as “unset” (defaulting only when the value is undefined).
  • Gate draft persistence to local storage in EntityForm so no cache writes occur when localChangesBackup === false.
  • Add Jest coverage for all supported localChangesBackup states plus the default.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/firecms_core/src/util/collections.ts Makes false reachable by defaulting only on undefined.
packages/firecms_core/src/util/tests/collections.test.ts Adds regression tests covering default, both string modes, and false.
packages/firecms_core/src/form/EntityForm.tsx Prevents saveEntityToCache writes when backups are explicitly disabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@fgatti675
fgatti675 merged commit 8070870 into firecmsco:main Jul 31, 2026
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.

localChangesBackup: false has no effect — the value is unreachable, and the backup write is never gated

3 participants