From b2c6747ea99bfdb9b1658d225c4e35552b30de80 Mon Sep 17 00:00:00 2001 From: Dariusz Biela Date: Tue, 8 Sep 2026 15:19:18 +0200 Subject: [PATCH 1/4] fix: keep the parser registration alive across effect remounts 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 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 ) 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 , a parser identity change inside a hidden and a plain parser identity change; @types/react-dom is added so the suite typechecks. --- package-lock.json | 11 ++ package.json | 1 + src/MarkdownTextInput.tsx | 49 ++++++- src/__tests__/parserRegistration.test.tsx | 163 ++++++++++++++++++++++ 4 files changed, 217 insertions(+), 7 deletions(-) create mode 100644 src/__tests__/parserRegistration.test.tsx diff --git a/package-lock.json b/package-lock.json index 04a55a4bd..b522fb14b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -28,6 +28,7 @@ "@release-it/conventional-changelog": "^5.0.0", "@types/jest": "^29.5.14", "@types/react": "^19.2.0", + "@types/react-dom": "^19.2.7", "@typescript-eslint/eslint-plugin": "^8.53.1", "@typescript-eslint/parser": "^8.53.1", "del-cli": "^5.0.0", @@ -5288,6 +5289,16 @@ "csstype": "^3.2.2" } }, + "node_modules/@types/react-dom": { + "version": "19.2.7", + "resolved": "https://registry.npmjs.org/@types/react-dom/-/react-dom-19.2.7.tgz", + "integrity": "sha512-I8bPpDLcHBv1qiIiXDCy71Rt8eQDKJP0sMSWJphDdAcdqiJ1sGpZamavoEIRZmYzjia9LuEb2HlYdDpmoENpvQ==", + "dev": true, + "license": "MIT", + "peerDependencies": { + "@types/react": "^19.2.0" + } + }, "node_modules/@types/semver": { "version": "7.5.8", "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.5.8.tgz", diff --git a/package.json b/package.json index 7c67d13e0..566d13020 100644 --- a/package.json +++ b/package.json @@ -74,6 +74,7 @@ "@release-it/conventional-changelog": "^5.0.0", "@types/jest": "^29.5.14", "@types/react": "^19.2.0", + "@types/react-dom": "^19.2.7", "@typescript-eslint/eslint-plugin": "^8.53.1", "@typescript-eslint/parser": "^8.53.1", "del-cli": "^5.0.0", diff --git a/src/MarkdownTextInput.tsx b/src/MarkdownTextInput.tsx index 935b05548..520917681 100644 --- a/src/MarkdownTextInput.tsx +++ b/src/MarkdownTextInput.tsx @@ -70,6 +70,47 @@ type FormatSelectionResult = { type MarkdownTextInput = TextInput & React.Component; +type ParserRegistration = { + parser: MarkdownTextInputProps['parser']; + parserId: number; +}; + +// The effect body registers and its cleanup unregisters, so a remount of the effects alone (StrictMode, a hidden +// `` that is revealed) hands the decorator view a fresh id instead of leaving it on the erased one. The first +// registration happens in render, which spares every mount a second commit and a re-measure of the input. +// A layout effect flushes the replacement id in the same task as the commit that ran the cleanup, so both usually reach +// the mounting layer as one native transaction. The native parser formats nothing for an id it cannot resolve. +function useParserId(parser: MarkdownTextInputProps['parser']): number { + const initialRegistrationRef = React.useRef(null); + if (initialRegistrationRef.current === null) { + initialRegistrationRef.current = {parser, parserId: registerParser(parser)}; + } + const [parserId, setParserId] = React.useState(initialRegistrationRef.current.parserId); + const liveRegistrationRef = React.useRef(initialRegistrationRef.current); + + React.useLayoutEffect(() => { + const unregisterLiveParser = () => { + if (liveRegistrationRef.current === null) { + return; + } + unregisterParser(liveRegistrationRef.current.parserId); + liveRegistrationRef.current = null; + }; + + if (liveRegistrationRef.current?.parser === parser) { + return unregisterLiveParser; + } + + unregisterLiveParser(); + const nextParserId = registerParser(parser); + liveRegistrationRef.current = {parser, parserId: nextParserId}; + setParserId(nextParserId); + return unregisterLiveParser; + }, [parser]); + + return parserId; +} + function processColorsInMarkdownStyle(input: MarkdownStyle): MarkdownStyle { const output = JSON.parse(JSON.stringify(input)); @@ -104,13 +145,7 @@ const MarkdownTextInput = React.forwardRef { - return registerParser(props.parser); - }, [props.parser]); - - React.useEffect(() => { - return () => unregisterParser(parserId); - }, [parserId]); + const parserId = useParserId(props.parser); return ( `, so the component renders in jsdom through `react-dom`. + */ +const liveParserIds = new Set(); +let nextParserId = 1; +let registerCallCount = 0; + +jest.mock('react-native', () => ({ + Platform: {OS: 'ios', select: (options: {ios?: unknown; default?: unknown}) => options.ios ?? options.default}, + StyleSheet: {create: (styles: T) => styles}, + TextInput: (props: {testID?: string}) => , + TurboModuleRegistry: {get: () => null}, + processColor: (color: unknown) => color, +})); + +jest.mock('react-native-worklets', () => ({ + createSerializable: (worklet: unknown) => worklet, + createWorkletRuntime: () => ({}), +})); + +jest.mock('../MarkdownTextInputDecoratorViewNativeComponent', () => ({ + __esModule: true, + default: (props: {parserId: number; children: React.ReactNode}) =>
{props.children}
, +})); + +// The component refuses a parser that is not a worklet, and the worklets babel plugin does not run under Jest, so the +// hash that marks a function as a worklet is attached by hand. +function createParserWorklet(workletHash: number) { + return Object.assign((): MarkdownRange[] => [], {__workletHash: workletHash}); +} + +const parser = createParserWorklet(1); + +let container: HTMLDivElement; +let root: Root; + +function renderIntoRoot(element: React.ReactElement) { + act(() => { + root.render(element); + }); +} + +function renderInActivity(isHidden: boolean, currentParser: MarkdownTextInputProps['parser'] = parser) { + renderIntoRoot( + + + , + ); +} + +function getDecoratorParserId(): number { + const decorator = container.querySelector('[data-parser-id]'); + const parserId = Number(decorator?.getAttribute('data-parser-id')); + if (!Number.isInteger(parserId)) { + throw new Error('The decorator view rendered without a parser id'); + } + return parserId; +} + +function expectDecoratorOnTheOnlyLiveParserId() { + expect(liveParserIds.has(getDecoratorParserId())).toBe(true); + expect(liveParserIds.size).toBe(1); +} + +describe('MarkdownTextInput parser registration', () => { + beforeEach(() => { + liveParserIds.clear(); + nextParserId = 1; + registerCallCount = 0; + global.jsi_setMarkdownRuntime = jest.fn(); + global.jsi_registerMarkdownWorklet = () => { + const parserId = nextParserId; + nextParserId += 1; + registerCallCount += 1; + liveParserIds.add(parserId); + return parserId; + }; + global.jsi_unregisterMarkdownWorklet = (parserId: number) => { + liveParserIds.delete(parserId); + }; + + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => { + root.unmount(); + }); + container.remove(); + }); + + it('registers the parser once and renders its id on mount', () => { + renderIntoRoot(); + + expect(registerCallCount).toBe(1); + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('unregisters the parser on unmount', () => { + renderIntoRoot(); + + act(() => { + root.unmount(); + }); + + expect(liveParserIds.size).toBe(0); + }); + + it('keeps the decorator on a live id under StrictMode', () => { + renderIntoRoot( + + + , + ); + + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('keeps the decorator on a live id after a hidden is revealed', () => { + renderInActivity(false); + expectDecoratorOnTheOnlyLiveParserId(); + + renderInActivity(true); + renderInActivity(false); + expectDecoratorOnTheOnlyLiveParserId(); + + renderInActivity(true); + renderInActivity(false); + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('drops the initial registration when the parser changes identity inside a hidden ', () => { + const nextParser = createParserWorklet(2); + + renderInActivity(true); + renderInActivity(true, nextParser); + renderInActivity(false, nextParser); + + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('moves the decorator to a live id and drops the previous one when the parser changes identity', () => { + renderIntoRoot(); + const initialParserId = getDecoratorParserId(); + + renderIntoRoot(); + + expect(getDecoratorParserId()).not.toBe(initialParserId); + expectDecoratorOnTheOnlyLiveParserId(); + }); +}); From d2930c0ffb8627ceed01800f705a9d6032ea094c Mon Sep 17 00:00:00 2001 From: Dariusz Biela Date: Tue, 8 Sep 2026 17:42:19 +0200 Subject: [PATCH 2/4] refactor: trim the parser registration comment and test helpers --- src/MarkdownTextInput.tsx | 8 +++----- src/__tests__/parserRegistration.test.tsx | 18 +++++++----------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/MarkdownTextInput.tsx b/src/MarkdownTextInput.tsx index 520917681..8be2fc63f 100644 --- a/src/MarkdownTextInput.tsx +++ b/src/MarkdownTextInput.tsx @@ -75,11 +75,9 @@ type ParserRegistration = { parserId: number; }; -// The effect body registers and its cleanup unregisters, so a remount of the effects alone (StrictMode, a hidden -// `` that is revealed) hands the decorator view a fresh id instead of leaving it on the erased one. The first -// registration happens in render, which spares every mount a second commit and a re-measure of the input. -// A layout effect flushes the replacement id in the same task as the commit that ran the cleanup, so both usually reach -// the mounting layer as one native transaction. The native parser formats nothing for an id it cannot resolve. +// The first registration happens in render so the first commit already carries a resolvable id. The layout effect +// re-registers after its own cleanup (StrictMode, a revealed ``) and hands the fresh id to the decorator. +// `initialRegistrationRef` is never cleared, otherwise a render inside a hidden `` would register again. function useParserId(parser: MarkdownTextInputProps['parser']): number { const initialRegistrationRef = React.useRef(null); if (initialRegistrationRef.current === null) { diff --git a/src/__tests__/parserRegistration.test.tsx b/src/__tests__/parserRegistration.test.tsx index 1ec21946a..f6115ae1c 100644 --- a/src/__tests__/parserRegistration.test.tsx +++ b/src/__tests__/parserRegistration.test.tsx @@ -13,7 +13,6 @@ import type {MarkdownTextInputProps} from '../MarkdownTextInput'; */ const liveParserIds = new Set(); let nextParserId = 1; -let registerCallCount = 0; jest.mock('react-native', () => ({ Platform: {OS: 'ios', select: (options: {ios?: unknown; default?: unknown}) => options.ios ?? options.default}, @@ -33,13 +32,12 @@ jest.mock('../MarkdownTextInputDecoratorViewNativeComponent', () => ({ default: (props: {parserId: number; children: React.ReactNode}) =>
{props.children}
, })); -// The component refuses a parser that is not a worklet, and the worklets babel plugin does not run under Jest, so the -// hash that marks a function as a worklet is attached by hand. -function createParserWorklet(workletHash: number) { - return Object.assign((): MarkdownRange[] => [], {__workletHash: workletHash}); +// The worklets babel plugin does not run under Jest, so the hash that marks a function as a worklet is attached by hand. +function createParserWorklet() { + return Object.assign((): MarkdownRange[] => [], {__workletHash: 1}); } -const parser = createParserWorklet(1); +const parser = createParserWorklet(); let container: HTMLDivElement; let root: Root; @@ -76,12 +74,10 @@ describe('MarkdownTextInput parser registration', () => { beforeEach(() => { liveParserIds.clear(); nextParserId = 1; - registerCallCount = 0; global.jsi_setMarkdownRuntime = jest.fn(); global.jsi_registerMarkdownWorklet = () => { const parserId = nextParserId; nextParserId += 1; - registerCallCount += 1; liveParserIds.add(parserId); return parserId; }; @@ -104,7 +100,7 @@ describe('MarkdownTextInput parser registration', () => { it('registers the parser once and renders its id on mount', () => { renderIntoRoot(); - expect(registerCallCount).toBe(1); + expect(nextParserId).toBe(2); expectDecoratorOnTheOnlyLiveParserId(); }); @@ -142,7 +138,7 @@ describe('MarkdownTextInput parser registration', () => { }); it('drops the initial registration when the parser changes identity inside a hidden ', () => { - const nextParser = createParserWorklet(2); + const nextParser = createParserWorklet(); renderInActivity(true); renderInActivity(true, nextParser); @@ -155,7 +151,7 @@ describe('MarkdownTextInput parser registration', () => { renderIntoRoot(); const initialParserId = getDecoratorParserId(); - renderIntoRoot(); + renderIntoRoot(); expect(getDecoratorParserId()).not.toBe(initialParserId); expectDecoratorOnTheOnlyLiveParserId(); From 090c4a458d8c73625de12cf3547ca084be721e94 Mon Sep 17 00:00:00 2001 From: Dariusz Biela Date: Wed, 9 Sep 2026 16:15:41 +0200 Subject: [PATCH 3/4] fix: keep the parser worklet alive in the native view after JS unregisters 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 --- android/src/main/cpp/MarkdownParser.cpp | 42 ++++++++++++++++-- android/src/main/cpp/MarkdownParser.h | 30 +++++++++++-- .../livemarkdown/MarkdownParser.java | 19 ++++++++ .../MarkdownTextInputDecoratorView.java | 9 +++- .../expensify/livemarkdown/MarkdownUtils.java | 7 ++- apple/MarkdownParser.h | 6 +++ apple/MarkdownParser.mm | 43 +++++++++++++++++-- apple/RCTMarkdownUtils.mm | 7 +++ cpp/MarkdownGlobal.cpp | 5 ++- cpp/MarkdownGlobal.h | 5 ++- 10 files changed, 156 insertions(+), 17 deletions(-) diff --git a/android/src/main/cpp/MarkdownParser.cpp b/android/src/main/cpp/MarkdownParser.cpp index 4edd28ec9..dadeb7ed2 100644 --- a/android/src/main/cpp/MarkdownParser.cpp +++ b/android/src/main/cpp/MarkdownParser.cpp @@ -7,14 +7,46 @@ using namespace facebook; namespace expensify { namespace livemarkdown { + jni::local_ref MarkdownParser::initHybrid(jni::alias_ref) { + return makeCxxInstance(); + } + + void MarkdownParser::nativeSetParserId(const int parserId) { + std::unique_lock lock(mutex_); + if (parserId_ == parserId) { + return; + } + const auto markdownWorklet = findMarkdownWorklet(parserId); + if (markdownWorklet == nullptr) { + return; + } + parserId_ = parserId; + markdownWorklet_ = markdownWorklet; + } + + // A parse for the current id uses the worklet kept alive by `nativeSetParserId`. + // Any other id is looked up in the registry the way it always was. + std::shared_ptr MarkdownParser::workletForParserId(const int parserId) { + { + std::unique_lock lock(mutex_); + if (parserId_ == parserId) { + return markdownWorklet_; + } + } + + return findMarkdownWorklet(parserId); + } + jni::local_ref MarkdownParser::nativeParse( - jni::alias_ref jThis, jni::alias_ref text, const int parserId) { - const auto markdownRuntime = expensify::livemarkdown::getMarkdownRuntime(); - jsi::Runtime &rt = markdownRuntime->getJSIRuntime(); + const auto markdownWorklet = workletForParserId(parserId); + if (markdownWorklet == nullptr) { + return jni::make_jstring("[]"); + } - const auto markdownWorklet = expensify::livemarkdown::getMarkdownWorklet(parserId); + const auto markdownRuntime = getMarkdownRuntime(); + jsi::Runtime &rt = markdownRuntime->getJSIRuntime(); const auto input = jsi::String::createFromUtf8(rt, text->toStdString()); const auto output = markdownRuntime->runGuarded(markdownWorklet, input); @@ -25,6 +57,8 @@ namespace livemarkdown { void MarkdownParser::registerNatives() { registerHybrid({ + makeNativeMethod("initHybrid", MarkdownParser::initHybrid), + makeNativeMethod("nativeSetParserId", MarkdownParser::nativeSetParserId), makeNativeMethod("nativeParse", MarkdownParser::nativeParse)}); } diff --git a/android/src/main/cpp/MarkdownParser.h b/android/src/main/cpp/MarkdownParser.h index fc3313b54..c27e1c3fc 100644 --- a/android/src/main/cpp/MarkdownParser.h +++ b/android/src/main/cpp/MarkdownParser.h @@ -10,19 +10,33 @@ #include #include +#include + +#include +#include + using namespace facebook; +using namespace worklets; namespace expensify { namespace livemarkdown { - class MarkdownParser : public jni::HybridClass, - public jsi::HostObject { + class MarkdownParser : public jni::HybridClass { public: static constexpr auto kJavaDescriptor = "Lcom/expensify/livemarkdown/MarkdownParser;"; - static jni::local_ref nativeParse( - jni::alias_ref jThis, + static jni::local_ref initHybrid(jni::alias_ref); + + // Looks up the worklet registered under `parserId` and keeps it alive until + // another registered id is set or this parser is released. JS unregisters + // the id when React cleans up effects, which also happens for an input that + // is hidden but still mounted, so the registry can't be asked again at + // parse time. An id the registry doesn't know leaves the previous worklet + // in place. + void nativeSetParserId(const int parserId); + + jni::local_ref nativeParse( jni::alias_ref text, const int parserId); @@ -30,6 +44,14 @@ namespace livemarkdown { private: friend HybridBase; + + MarkdownParser() = default; + + std::shared_ptr workletForParserId(const int parserId); + + std::mutex mutex_; + int parserId_ = 0; + std::shared_ptr markdownWorklet_; }; } // namespace livemarkdown diff --git a/android/src/main/java/com/expensify/livemarkdown/MarkdownParser.java b/android/src/main/java/com/expensify/livemarkdown/MarkdownParser.java index 3e108db70..0588df55c 100644 --- a/android/src/main/java/com/expensify/livemarkdown/MarkdownParser.java +++ b/android/src/main/java/com/expensify/livemarkdown/MarkdownParser.java @@ -2,6 +2,8 @@ import androidx.annotation.NonNull; +import com.facebook.jni.HybridData; +import com.facebook.jni.annotations.DoNotStrip; import com.facebook.react.bridge.ReactContext; import com.facebook.react.util.RNLog; import com.facebook.soloader.SoLoader; @@ -20,6 +22,10 @@ public class MarkdownParser { SoLoader.loadLibrary("livemarkdown"); } + @DoNotStrip + @SuppressWarnings("unused") + private final HybridData mHybridData; + private final @NonNull ReactContext mReactContext; private String mPrevText; private int mPrevParserId; @@ -27,10 +33,23 @@ public class MarkdownParser { public MarkdownParser(@NonNull ReactContext reactContext) { mReactContext = reactContext; + mHybridData = initHybrid(); } + private static native HybridData initHybrid(); + + private native void nativeSetParserId(int parserId); + private native String nativeParse(@NonNull String text, int parserId); + /** + * Keeps the worklet registered under {@code parserId} alive in native code for as long as this parser lives, so a + * later parse still works after JS has unregistered the id. See {@code MarkdownParser.h} for why that happens. + */ + public synchronized void setParserId(int parserId) { + nativeSetParserId(parserId); + } + public synchronized List parse(@NonNull String text, int parserId) { try { Systrace.beginSection(0, "parse"); diff --git a/android/src/main/java/com/expensify/livemarkdown/MarkdownTextInputDecoratorView.java b/android/src/main/java/com/expensify/livemarkdown/MarkdownTextInputDecoratorView.java index 69427e8e6..5e8a0dcd0 100644 --- a/android/src/main/java/com/expensify/livemarkdown/MarkdownTextInputDecoratorView.java +++ b/android/src/main/java/com/expensify/livemarkdown/MarkdownTextInputDecoratorView.java @@ -15,12 +15,18 @@ public class MarkdownTextInputDecoratorView extends ReactViewGroup { public MarkdownTextInputDecoratorView(Context context) { super(context); + mMarkdownParser = new MarkdownParser((ReactContext) context); } private MarkdownStyle mMarkdownStyle; private int mParserId; + // Owned by the view rather than by `mMarkdownUtils`, which is recreated every + // time the view is attached, so the parser worklet stays alive for as long as + // the view is mounted. + private final MarkdownParser mMarkdownParser; + private MarkdownUtils mMarkdownUtils; private ReactEditText mReactEditText; @@ -33,7 +39,7 @@ protected void onAttachedToWindow() { View child = getChildAt(0); if (child instanceof ReactEditText) { - mMarkdownUtils = new MarkdownUtils((ReactContext) getContext()); + mMarkdownUtils = new MarkdownUtils((ReactContext) getContext(), mMarkdownParser); mMarkdownUtils.setMarkdownStyle(mMarkdownStyle); mMarkdownUtils.setParserId(mParserId); mReactEditText = (ReactEditText) child; @@ -64,6 +70,7 @@ protected void setMarkdownStyle(MarkdownStyle markdownStyle) { protected void setParserId(int parserId) { mParserId = parserId; + mMarkdownParser.setParserId(parserId); if (mMarkdownUtils != null) { mMarkdownUtils.setParserId(mParserId); } diff --git a/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java b/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java index 6877e46f9..34e7a4586 100644 --- a/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java +++ b/android/src/main/java/com/expensify/livemarkdown/MarkdownUtils.java @@ -11,7 +11,11 @@ public class MarkdownUtils { public MarkdownUtils(@NonNull ReactContext reactContext) { - mMarkdownParser = new MarkdownParser(reactContext); + this(reactContext, new MarkdownParser(reactContext)); + } + + public MarkdownUtils(@NonNull ReactContext reactContext, @NonNull MarkdownParser markdownParser) { + mMarkdownParser = markdownParser; mMarkdownFormatter = new MarkdownFormatter(reactContext.getAssets()); } @@ -27,6 +31,7 @@ public void setMarkdownStyle(@NonNull MarkdownStyle markdownStyle) { public void setParserId(int parserId) { mParserId = parserId; + mMarkdownParser.setParserId(parserId); } public void applyMarkdownFormatting(SpannableStringBuilder ssb) { diff --git a/apple/MarkdownParser.h b/apple/MarkdownParser.h index 1d73622d6..743492442 100644 --- a/apple/MarkdownParser.h +++ b/apple/MarkdownParser.h @@ -5,6 +5,12 @@ NS_ASSUME_NONNULL_BEGIN @interface MarkdownParser : NSObject +// Looks up the worklet registered under `parserId` and keeps it alive until +// another registered id is set or this parser is released. JS unregisters the +// id when React cleans up effects, which also happens for an input that is +// hidden but still mounted, so the registry can't be asked again at parse time. +- (void)setParserId:(nonnull NSNumber *)parserId; + - (NSArray *)parse:(nonnull NSString *)text withParserId:(nonnull NSNumber *)parserId; diff --git a/apple/MarkdownParser.mm b/apple/MarkdownParser.mm index 89bb209b4..721ada31e 100644 --- a/apple/MarkdownParser.mm +++ b/apple/MarkdownParser.mm @@ -56,6 +56,10 @@ @implementation MarkdownParser { NSNumber *_pendingParserId; void (^_pendingCompletion)(void); BOOL _warmupScheduled; + + // The worklet registered under `_parserId`, kept alive here (see the header). + NSNumber *_parserId; + std::shared_ptr _markdownWorklet; } - (instancetype)init @@ -82,6 +86,39 @@ + (dispatch_queue_t)cacheWarmupQueue return queue; } +// An id the registry doesn't know leaves the previous worklet in place. The +// measure path shares one parser between shadow node clones, so a clone that +// still carries an older, already unregistered id must not drop the worklet +// the current id resolved to. +- (void)setParserId:(nonnull NSNumber *)parserId +{ + @synchronized (self) { + if ([_parserId isEqualToNumber:parserId]) { + return; + } + const auto markdownWorklet = expensify::livemarkdown::findMarkdownWorklet([parserId intValue]); + if (markdownWorklet == nullptr) { + return; + } + _parserId = parserId; + _markdownWorklet = markdownWorklet; + } +} + +// A parse for the current id uses the worklet kept alive by `setParserId:`. +// Any other id comes from a shadow node clone that still carries an older id, +// so it is looked up in the registry the way it always was. +- (std::shared_ptr)workletForParserId:(nonnull NSNumber *)parserId +{ + @synchronized (self) { + if ([_parserId isEqualToNumber:parserId]) { + return _markdownWorklet; + } + } + + return expensify::livemarkdown::findMarkdownWorklet([parserId intValue]); +} + - (nullable NSArray *)cachedRangesForText:(nonnull NSString *)text withParserId:(nonnull NSNumber *)parserId { @@ -212,10 +249,8 @@ - (void)drainPendingWarmups const auto &markdownRuntime = expensify::livemarkdown::getMarkdownRuntime(); jsi::Runtime &rt = markdownRuntime->getJSIRuntime(); - std::shared_ptr markdownWorklet; - try { - markdownWorklet = expensify::livemarkdown::getMarkdownWorklet([parserId intValue]); - } catch (const std::out_of_range &error) { + const auto markdownWorklet = [self workletForParserId:parserId]; + if (markdownWorklet == nullptr) { return @[]; } diff --git a/apple/RCTMarkdownUtils.mm b/apple/RCTMarkdownUtils.mm index aa034dfe2..2e28cb7c3 100644 --- a/apple/RCTMarkdownUtils.mm +++ b/apple/RCTMarkdownUtils.mm @@ -17,6 +17,12 @@ - (instancetype)init return self; } +- (void)setParserId:(NSNumber *)parserId +{ + _parserId = parserId; + [_markdownParser setParserId:parserId]; +} + - (void)applyMarkdownFormatting:(nonnull NSMutableAttributedString *)attributedString withDefaultTextAttributes:(nonnull NSDictionary *)defaultTextAttributes { @@ -49,6 +55,7 @@ - (void)applyMarkdownFormatting:(nonnull NSMutableAttributedString *)attributedS _markdownStyle = markdownStyle; _parserId = parserId; } + [_markdownParser setParserId:parserId]; NSString *text = attributedString.string; NSArray *markdownRanges = [_markdownParser cachedRangesForText:text withParserId:parserId]; diff --git a/cpp/MarkdownGlobal.cpp b/cpp/MarkdownGlobal.cpp index 67f93eb43..c83d818a2 100644 --- a/cpp/MarkdownGlobal.cpp +++ b/cpp/MarkdownGlobal.cpp @@ -34,9 +34,10 @@ void unregisterMarkdownWorklet(const int parserId) { globalMarkdownShareableWorklets.erase(parserId); } -std::shared_ptr getMarkdownWorklet(const int parserId) { +std::shared_ptr findMarkdownWorklet(const int parserId) { std::unique_lock lock(globalMarkdownShareableWorkletsMutex); - return globalMarkdownShareableWorklets.at(parserId); + const auto it = globalMarkdownShareableWorklets.find(parserId); + return it == globalMarkdownShareableWorklets.end() ? nullptr : it->second; } } // namespace livemarkdown diff --git a/cpp/MarkdownGlobal.h b/cpp/MarkdownGlobal.h index e18172613..a9102bcde 100644 --- a/cpp/MarkdownGlobal.h +++ b/cpp/MarkdownGlobal.h @@ -18,7 +18,10 @@ const int registerMarkdownWorklet(const std::shared_ptr &ma void unregisterMarkdownWorklet(const int parserId); -std::shared_ptr getMarkdownWorklet(const int parserId); +// Returns nullptr when nothing is registered under `parserId`. Callers keep the +// result: JS drops the entry when React cleans up effects, which also happens +// for an input that is hidden but still mounted. +std::shared_ptr findMarkdownWorklet(const int parserId); } // namespace livemarkdown } // namespace expensify From a69d6dd4cf77b85c85f1a8d255fa5fb1d191bc98 Mon Sep 17 00:00:00 2001 From: Dariusz Biela Date: Fri, 11 Sep 2026 14:51:35 +0200 Subject: [PATCH 4/4] fix: register the parser only after commit Registering in render leaked an entry whenever React abandoned the render (a suspended tree, an 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. --- apple/MarkdownParser.mm | 8 +- src/MarkdownTextInput.tsx | 33 +----- src/__tests__/parserRegistration.test.tsx | 136 +++++++++++++++++++--- 3 files changed, 127 insertions(+), 50 deletions(-) diff --git a/apple/MarkdownParser.mm b/apple/MarkdownParser.mm index 721ada31e..4c13ec7ef 100644 --- a/apple/MarkdownParser.mm +++ b/apple/MarkdownParser.mm @@ -246,14 +246,16 @@ - (void)drainPendingWarmups - (NSArray *)parseUncached:(nonnull NSString *)text withParserId:(nonnull NSNumber *)parserId { - const auto &markdownRuntime = expensify::livemarkdown::getMarkdownRuntime(); - jsi::Runtime &rt = markdownRuntime->getJSIRuntime(); - + // The first commit carries parserId 0, before JS has created the worklet runtime. + // Resolve the worklet before accessing that runtime. const auto markdownWorklet = [self workletForParserId:parserId]; if (markdownWorklet == nullptr) { return @[]; } + const auto &markdownRuntime = expensify::livemarkdown::getMarkdownRuntime(); + jsi::Runtime &rt = markdownRuntime->getJSIRuntime(); + const auto &input = jsi::String::createFromUtf8(rt, [text UTF8String]); jsi::Value output; diff --git a/src/MarkdownTextInput.tsx b/src/MarkdownTextInput.tsx index 8be2fc63f..093e6baa8 100644 --- a/src/MarkdownTextInput.tsx +++ b/src/MarkdownTextInput.tsx @@ -70,40 +70,15 @@ type FormatSelectionResult = { type MarkdownTextInput = TextInput & React.Component; -type ParserRegistration = { - parser: MarkdownTextInputProps['parser']; - parserId: number; -}; - -// The first registration happens in render so the first commit already carries a resolvable id. The layout effect -// re-registers after its own cleanup (StrictMode, a revealed ``) and hands the fresh id to the decorator. -// `initialRegistrationRef` is never cleared, otherwise a render inside a hidden `` would register again. +// Register only after commit: a suspended or initially hidden render may never run an effect cleanup. +// Zero means no parser yet. A layout effect publishes the registered id and restores it when effects reconnect. function useParserId(parser: MarkdownTextInputProps['parser']): number { - const initialRegistrationRef = React.useRef(null); - if (initialRegistrationRef.current === null) { - initialRegistrationRef.current = {parser, parserId: registerParser(parser)}; - } - const [parserId, setParserId] = React.useState(initialRegistrationRef.current.parserId); - const liveRegistrationRef = React.useRef(initialRegistrationRef.current); + const [parserId, setParserId] = React.useState(0); React.useLayoutEffect(() => { - const unregisterLiveParser = () => { - if (liveRegistrationRef.current === null) { - return; - } - unregisterParser(liveRegistrationRef.current.parserId); - liveRegistrationRef.current = null; - }; - - if (liveRegistrationRef.current?.parser === parser) { - return unregisterLiveParser; - } - - unregisterLiveParser(); const nextParserId = registerParser(parser); - liveRegistrationRef.current = {parser, parserId: nextParserId}; setParserId(nextParserId); - return unregisterLiveParser; + return () => unregisterParser(nextParserId); }, [parser]); return parserId; diff --git a/src/__tests__/parserRegistration.test.tsx b/src/__tests__/parserRegistration.test.tsx index f6115ae1c..4fbb5b2e5 100644 --- a/src/__tests__/parserRegistration.test.tsx +++ b/src/__tests__/parserRegistration.test.tsx @@ -1,5 +1,5 @@ import {expect} from '@jest/globals'; -import React, {Activity, StrictMode, act} from 'react'; +import React, {Activity, StrictMode, Suspense, act} from 'react'; import {createRoot} from 'react-dom/client'; import type {Root} from 'react-dom/client'; import type {MarkdownRange} from '../commonTypes'; @@ -8,10 +8,10 @@ import type {MarkdownTextInputProps} from '../MarkdownTextInput'; /** * The parser worklet lives in a C++ registry keyed by the `parserId` prop the decorator view carries. The registry is - * replaced by a set of ids here, the decorator view by an element that exposes its `parserId` as a DOM attribute, and + * replaced by a map of worklets here, the decorator view by an element that exposes its `parserId` as a DOM attribute, and * the native text input by a plain ``, so the component renders in jsdom through `react-dom`. */ -const liveParserIds = new Set(); +const liveParsers = new Map(); let nextParserId = 1; jest.mock('react-native', () => ({ @@ -58,31 +58,32 @@ function renderInActivity(isHidden: boolean, currentParser: MarkdownTextInputPro function getDecoratorParserId(): number { const decorator = container.querySelector('[data-parser-id]'); - const parserId = Number(decorator?.getAttribute('data-parser-id')); - if (!Number.isInteger(parserId)) { + const attribute = decorator?.getAttribute('data-parser-id'); + const parserId = Number(attribute); + if (attribute == null || !Number.isInteger(parserId) || parserId < 0) { throw new Error('The decorator view rendered without a parser id'); } return parserId; } -function expectDecoratorOnTheOnlyLiveParserId() { - expect(liveParserIds.has(getDecoratorParserId())).toBe(true); - expect(liveParserIds.size).toBe(1); +function expectDecoratorOnTheOnlyLiveParserId(expectedParser: MarkdownTextInputProps['parser'] = parser) { + expect(liveParsers.get(getDecoratorParserId())).toBe(expectedParser); + expect(liveParsers.size).toBe(1); } describe('MarkdownTextInput parser registration', () => { beforeEach(() => { - liveParserIds.clear(); + liveParsers.clear(); nextParserId = 1; global.jsi_setMarkdownRuntime = jest.fn(); - global.jsi_registerMarkdownWorklet = () => { + global.jsi_registerMarkdownWorklet = (worklet) => { const parserId = nextParserId; nextParserId += 1; - liveParserIds.add(parserId); + liveParsers.set(parserId, worklet as unknown as MarkdownTextInputProps['parser']); return parserId; }; global.jsi_unregisterMarkdownWorklet = (parserId: number) => { - liveParserIds.delete(parserId); + liveParsers.delete(parserId); }; container = document.createElement('div'); @@ -95,9 +96,10 @@ describe('MarkdownTextInput parser registration', () => { root.unmount(); }); container.remove(); + expect(liveParsers.size).toBe(0); }); - it('registers the parser once and renders its id on mount', () => { + it('registers the parser once and publishes its id after mount', () => { renderIntoRoot(); expect(nextParserId).toBe(2); @@ -111,7 +113,58 @@ describe('MarkdownTextInput parser registration', () => { root.unmount(); }); - expect(liveParserIds.size).toBe(0); + expect(liveParsers.size).toBe(0); + }); + + it('keeps the registration when rerendering with the same parser', () => { + renderIntoRoot(); + const initialParserId = getDecoratorParserId(); + + renderIntoRoot( + , + ); + + expect(getDecoratorParserId()).toBe(initialParserId); + expect(nextParserId).toBe(2); + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('does not register a parser for an Activity that is removed without ever becoming visible', () => { + renderInActivity(true); + + expect(getDecoratorParserId()).toBe(0); + expect(liveParsers.size).toBe(0); + + renderIntoRoot(
); + + expect(nextParserId).toBe(1); + expect(liveParsers.size).toBe(0); + }); + + it('does not leak a registration when React abandons a suspended render', () => { + const pending = new Promise(() => { + // Keep the subtree suspended until its render is abandoned. + }); + function Suspend(): React.ReactNode { + throw pending; + } + + renderIntoRoot( + Loading}> + + + , + ); + + expect(container.textContent).toBe('Loading'); + expect(nextParserId).toBe(1); + expect(liveParsers.size).toBe(0); + + renderIntoRoot(); + expectDecoratorOnTheOnlyLiveParserId(); }); it('keeps the decorator on a live id under StrictMode', () => { @@ -137,23 +190,70 @@ describe('MarkdownTextInput parser registration', () => { expectDecoratorOnTheOnlyLiveParserId(); }); - it('drops the initial registration when the parser changes identity inside a hidden ', () => { + it('registers a replacement parser when a previously visible Activity is revealed', () => { + renderInActivity(false); + renderInActivity(true); + expect(liveParsers.size).toBe(0); + + const nextParser = createParserWorklet(); + renderInActivity(true, nextParser); + expect(liveParsers.size).toBe(0); + + renderInActivity(false, nextParser); + expectDecoratorOnTheOnlyLiveParserId(nextParser); + }); + + it('keeps registrations independent for inputs sharing the same parser', () => { + renderIntoRoot( +
+ + +
, + ); + const ids = Array.from(container.querySelectorAll('[data-parser-id]'), (element) => Number(element.getAttribute('data-parser-id'))); + expect(new Set(ids).size).toBe(2); + expect(liveParsers.size).toBe(2); + ids.forEach((id) => expect(liveParsers.get(id)).toBe(parser)); + + renderIntoRoot( +
+ +
, + ); + expect(getDecoratorParserId()).toBe(ids[1]); + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('registers only the latest parser when an initially hidden is revealed', () => { const nextParser = createParserWorklet(); renderInActivity(true); + expect(liveParsers.size).toBe(0); renderInActivity(true, nextParser); + expect(liveParsers.size).toBe(0); renderInActivity(false, nextParser); - expectDecoratorOnTheOnlyLiveParserId(); + expect(nextParserId).toBe(2); + expectDecoratorOnTheOnlyLiveParserId(nextParser); }); it('moves the decorator to a live id and drops the previous one when the parser changes identity', () => { renderIntoRoot(); const initialParserId = getDecoratorParserId(); - renderIntoRoot(); + const nextParser = createParserWorklet(); + renderIntoRoot(); expect(getDecoratorParserId()).not.toBe(initialParserId); - expectDecoratorOnTheOnlyLiveParserId(); + expectDecoratorOnTheOnlyLiveParserId(nextParser); }); });