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..4c13ec7ef 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 { @@ -209,16 +246,16 @@ - (void)drainPendingWarmups - (NSArray *)parseUncached:(nonnull NSString *)text withParserId:(nonnull NSNumber *)parserId { - 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) { + // 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/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 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..093e6baa8 100644 --- a/src/MarkdownTextInput.tsx +++ b/src/MarkdownTextInput.tsx @@ -70,6 +70,20 @@ type FormatSelectionResult = { type MarkdownTextInput = TextInput & React.Component; +// 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 [parserId, setParserId] = React.useState(0); + + React.useLayoutEffect(() => { + const nextParserId = registerParser(parser); + setParserId(nextParserId); + return () => unregisterParser(nextParserId); + }, [parser]); + + return parserId; +} + function processColorsInMarkdownStyle(input: MarkdownStyle): MarkdownStyle { const output = JSON.parse(JSON.stringify(input)); @@ -104,13 +118,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 liveParsers = new Map(); +let nextParserId = 1; + +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 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(); + +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 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(expectedParser: MarkdownTextInputProps['parser'] = parser) { + expect(liveParsers.get(getDecoratorParserId())).toBe(expectedParser); + expect(liveParsers.size).toBe(1); +} + +describe('MarkdownTextInput parser registration', () => { + beforeEach(() => { + liveParsers.clear(); + nextParserId = 1; + global.jsi_setMarkdownRuntime = jest.fn(); + global.jsi_registerMarkdownWorklet = (worklet) => { + const parserId = nextParserId; + nextParserId += 1; + liveParsers.set(parserId, worklet as unknown as MarkdownTextInputProps['parser']); + return parserId; + }; + global.jsi_unregisterMarkdownWorklet = (parserId: number) => { + liveParsers.delete(parserId); + }; + + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + }); + + afterEach(() => { + act(() => { + root.unmount(); + }); + container.remove(); + expect(liveParsers.size).toBe(0); + }); + + it('registers the parser once and publishes its id after mount', () => { + renderIntoRoot(); + + expect(nextParserId).toBe(2); + expectDecoratorOnTheOnlyLiveParserId(); + }); + + it('unregisters the parser on unmount', () => { + renderIntoRoot(); + + act(() => { + root.unmount(); + }); + + 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', () => { + 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('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); + + 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(); + + const nextParser = createParserWorklet(); + renderIntoRoot(); + + expect(getDecoratorParserId()).not.toBe(initialParserId); + expectDecoratorOnTheOnlyLiveParserId(nextParser); + }); +});