fix(ios): preserve unsaved annotations when switching annotation tools (Fabric)#422
Merged
ignaciokit merged 1 commit intoJun 17, 2026
Conversation
ignaciokit
force-pushed
the
ignacio/fix-ios-fabric-document-reload-on-prop-change
branch
from
962a55c to
809c04e
Compare
On the New Architecture, NutrientView.updateProps is invoked with the full prop set on every re-render. The document block was guarded only by `!newProps->document.empty()`, so since `document` is always set it ran on every update — creating a fresh PSPDFDocument and reloading the controller on any prop change (e.g. a dynamic `menuItemGrouping` driven by React state). That reload discarded unsaved in-memory annotations. Guard the re-apply on an actual document change (`newProps->document != oldProps->document`), mirroring the document-identity check already used on Android. Tool switching via a dynamic menuItemGrouping now preserves unsaved annotations without calling document.save(). Adds a Catalog example (SwitchAnnotationTools) demonstrating the issue with a dynamic/static grouping toggle.
ignaciokit
force-pushed
the
ignacio/fix-ios-fabric-document-reload-on-prop-change
branch
from
809c04e to
3bd53a1
Compare
ignaciokit
deleted the
ignacio/fix-ios-fabric-document-reload-on-prop-change
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On iOS with the New Architecture (Fabric), unsaved in-memory annotations are lost when switching annotation creation tools via a dynamic
menuItemGrouping, unlessdocument.save()is called first.Root cause
NutrientView.updateProps:oldProps:(ios/Fabric/NutrientView.mm) runs with the full prop set on every re-render. The document block was guarded only by!newProps->document.empty():Since
documentis always set, this ran on every prop update.applyDocumentFromJSONassigns a freshly createdPSPDFDocumentto the controller (view.pdfController.document = ...), which reloads from disk. Any prop change — e.g. amenuItemGroupingrecomputed from React state on each tool switch — therefore reloaded the document and discarded unsaved annotations. With autosave off they were never persisted, so they were lost.Android does not reproduce this:
PdfView.javaguards the reload by document UID (!pdfFragment.getDocument().getUid().equals(document.getUid())). iOS Fabric had no equivalent guard.Fix
Only re-apply the document when the path actually changed, mirroring Android's identity check:
A dynamic-
menuItemGroupingpattern now preserves unsaved annotations across tool switches with no client-side change and nodocument.save().Testing
Tracking