[HOLD for RN 0.88] fix: keep the parser registration alive across effect remounts - #776
Draft
dariusz-biela wants to merge 11 commits into
Draft
dariusz-biela wants to merge 11 commits into
dariusz-biela wants to merge 11 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
MarkdownTextInput registered its parser worklet from a useMemo and only unregistered it from an effect cleanup. Whenever React remounted the effects without unmounting the component (StrictMode in development, a hidden React <Activity> that is revealed again), the cleanup erased the entry from the C++ registry while the decorator view kept the same parserId and the re-run of the effect registered nothing. Every later parse resolved the id with std::unordered_map::at and threw: iOS caught std::out_of_range and returned no ranges, Android let it cross the JNI boundary where fbjni turns it into a Java exception that MarkdownParser.java swallows. The input silently stopped formatting markdown for the rest of its life. Registration and unregistration now live in one layout effect and the new id reaches the decorator view through state. The first registration stays in the first render, so a mount still carries a resolvable id in its first commit and does not pay a second commit and a re-measure of the input. The effect body also replaces a registration whose parser changed identity while the effects were not mounted (an input inside a hidden <Activity>) and unregisters the stale one. A layout effect keeps the re-registration in the same task as the commit that ran the cleanup, so the two commits usually collapse into one native transaction instead of leaving the view on the erased id for a frame. No native change is needed: both parsers already return no ranges for an id the registry cannot resolve, and the ranges are cached per (text, parserId), so the replacement id re-parses the same text as soon as the view receives it. The new Jest suite renders the component through react-dom into jsdom and covers mount, unmount, StrictMode, a hidden and revealed <Activity>, a parser identity change inside a hidden <Activity> and a plain parser identity change; @types/react-dom is added so the suite typechecks.
dariusz-biela
force-pushed
the
fix/parser-registration-effect-remount
branch
from
September 8, 2026 15:29
06ea942 to
b2c6747
Compare
Author
|
I have read the CLA Document and I hereby sign the CLA |
…sters it JS unregisters the parser id when React cleans up effects, which also happens for an input that is hidden but still mounted. The native parser now looks the worklet up once, when the id prop changes, and keeps it alive for as long as the view lives, so a parse in that window still formats markdown. The registry is only a handoff from JS to native. On iOS `MarkdownParser` holds the worklet and both `RCTMarkdownUtils` paths pass the id through. On Android `MarkdownParser` becomes an fbjni hybrid that holds it, and the decorator view owns the parser so the `MarkdownUtils` recreated on every attach does not drop it. Claude-Session: https://claude.ai/code/session_01F1MRNtwY27QsvZcV1GwQJ9
Registering in render leaked an entry whenever React abandoned the render (a suspended tree, an <Activity> removed while still hidden), and the extra bookkeeping in useParserId existed only to pair that render registration with the effect. The layout effect now registers, its cleanup unregisters, and the id reaches the decorator through state. The first commit carries 0, which both native parsers treat as no parser; on iOS parseUncached resolves the worklet before touching the worklet runtime, since that first commit can arrive before the runtime exists. The jsdom suite gains cases for a same-parser rerender, an Activity that is removed while hidden, an abandoned suspended render, a replacement parser in a previously visible Activity, and two inputs sharing one parser, and asserts that every test leaves the registry empty.
The parser id is now picked in JS, one per worklet identity, and the worklet is registered in `useInsertionEffect`. That effect runs before the host tree commits, so the first measure already sees the worklet, and a hidden `<Activity>` leaves it connected, so the id the native view holds never goes stale. Inputs sharing a parser share one registration through a retain count. This removes the extra render and commit of the layout effect approach and the native keep-alive of the worklet, which is no longer needed. The C++ registry accepts the id from JS and returns nullptr for unknown ids instead of throwing.
Two buttons hide and reveal the input in a React Activity, one with a plain tree and one with AlwaysPaintedView, a view whose display style is pinned to contents so the input stays painted while its JS side is hidden. A third button swaps the parser prop between ExpensiMark and a strikethrough worklet. The example passes an explicit max length to parseExpensiMark because the default parameter is evaluated before the worklet closure with react-native-worklets 0.10.2.
Inputs no longer share a registration per parser function. Each mounted input allocates its own id, and a fresh one whenever the parser prop changes, so the native parse cache keyed by text and parser id never serves ranges from a previous parser. This removes the parser-to-id WeakMap and the retain count.
…Activity buttons The example gets one button that picks the wrapper while the input is visible and one that hides or shows the Activity, instead of a hide button per wrapper.
dariusz-biela
changed the title
40 tasks
| const int parserId) { | ||
| const auto markdownWorklet = expensify::livemarkdown::findMarkdownWorklet(parserId); | ||
| if (markdownWorklet == nullptr) { | ||
| return jni::make_jstring("[]"); |
There was a problem hiding this comment.
Could we distinguish "no worklet registered" from "parser returned zero ranges", skip the cache write in the first case, and log a warning with the parserId? That would make the broken invariant visible. What do you think?
There was a problem hiding this comment.
By "broken invariant" I mean the parser is registered in useInsertionEffect, so it should always be available when the native view calls parse
Author
There was a problem hiding this comment.
Done in 327efd2: native returns null / nil for an unknown id, logs a warning with the parserId and leaves the cache alone (both platforms). On iOS the warmup also skips its completion in that case, otherwise the main thread would loop on re-measure.
One known false positive: the worklet is unregistered on the JS thread during the commit, but the native view gets the new parserId (or is removed) only when the UI thread runs the mount transaction. A parse in between (a keystroke, or an iOS warmup scheduled just before) still uses the old id and logs the warning. It is a few-ms window, only after a parser swap or an unmount, and harmless: nothing is cached and the view reformats once the new id lands. I did not suppress it on purpose - remembering unregistered ids would also hide a cleanup that runs without an unmount, which is the bug this PR fixes. If the noise matters I can log once per parserId instead.
Both platforms returned an empty range list for an unknown parser id, which the per-text cache then kept even after the parser showed up. Native now tells the two cases apart, logs the id and leaves the cache alone, so the next parse picks the parser up. On iOS the warmup also skips its completion in that case, since re-measuring would find nothing in the cache and loop.
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.
Important
On hold until React Native 0.88. The Fabric renderer in React Native 0.85–0.87 does not run the
useInsertionEffectcleanup for a component removed while its<Activity>is hidden (see "Native renderer caveat"). React 19.3, which React Native 0.88 ships, always runs it. The diff itself does not need to change.Details
MarkdownTextInputregistered its parser worklet during render and unregistered it in an effect cleanup. React can run that cleanup without unmounting the component (StrictMode, a hidden<Activity>), after which the input silently stopped formatting markdown.The fix
The parser id is now chosen in JS, one per mounted input and a fresh one on every
parserchange, and the worklet is registered inuseInsertionEffectinside the newuseParserIdhook. Insertion effects are neither double-invoked in StrictMode nor disconnected for a hidden<Activity>, and they run before the host tree is committed, so the registration lives exactly as long as the input and the first commit already carries a valid id.The native registry becomes a plain map from id to worklet; an unknown id yields no ranges instead of throwing.
Native renderer caveat
React Native 0.85–0.87 build the Fabric renderer with
enableHiddenSubtreeInsertionEffectCleanupoff, so removing an input behind a hidden<Activity>skips the insertion cleanup and leaks its worklet in the native registry. Formatting stays correct because ids are never reused.react-domhas the flag on, and React 19.3 removed it and always runs the cleanup (react/react#30954, #34372, #35918).Example app
New buttons reproduce the scenarios on a device: hide and show an
<Activity>around the input, keep the input painted while hidden (AlwaysPaintedView), and swap theparserprop.Related Issues
Expensify/App#98254
Manual Tests
src/__tests__/parserRegistration.test.tsxcovers mount, unmount, StrictMode, hidden and removed<Activity>, an abandoned render, shared parsers and a parser swap, and checks that nothing stays registered. The StrictMode and<Activity>cases fail without the fix.Example app on iOS and Android with rebuilt native code:
Hello *bold* and https://expensify.com: bold and link render as you type.<Activity>: the input comes back formatted and keeps formatting new text.<React.StrictMode>and type*sm*: it renders bold.ios-activity-buttons.mp4
android-activity-buttons.mp4
Linked PRs
Expensify/App#100318