fix(react): MTFs and gestures memory leak in PrimJS RC mode#2107
fix(react): MTFs and gestures memory leak in PrimJS RC mode#2107Yradex wants to merge 2 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9235133 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRegisters a main-thread destroy listener for Lynx MTS, clears snapshot instance values via updaters during removal, adds safe SystemInfo access for MTS, and includes tests plus a changeset note documenting the patch release. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/react/runtime/src/lynx/mts-destroy.ts:
- Around line 16-22: The destroyMts function can leave profiling open and the
event listener attached if root.removeChild throws; wrap the removal loop in a
try/finally so that lynx.performance.profileEnd() and
lynx.getNative().removeEventListener('__DestroyLifetime', destroyMts) always
run; place the root.childNodes.forEach(child => root.removeChild(child)) call
inside the try block and move profileEnd/removeEventListener into the finally
block, and if you catch an error rethrow it after the finally to preserve
original error behavior.
In @packages/react/runtime/src/snapshot.ts:
- Around line 263-287: The function clearSnapshotInstanceValuesWithUpdater uses
a non-null assertion on update[i] which can throw if the update array is sparse;
modify clearSnapshotInstanceValuesWithUpdater to first read const updater =
update[i] and skip the iteration if updater is undefined or null (i.e., only
call updater(v, i, oldValue) when updater is present), leaving the existing
handling of values intact and avoiding the non-null assertion.
🧹 Nitpick comments (1)
packages/react/runtime/__test__/lynx/mts-destroy.test.ts (1)
33-53: Consider testing error scenarios and edge cases.The test correctly verifies the happy path: listener registration, callback invocation removing children, profiling calls, and listener removal. However, consider adding edge case tests:
- What happens if
__roothas no children whendestroyMtsis called?- What if
removeChildthrows an error - does profiling still end correctly?💡 Optional: Additional edge case test
it('handles empty root gracefully on __DestroyLifetime', () => { (globalThis as any).SystemInfo = { lynxSdkVersion: '3.4' }; const addEventListener = vi.fn(); const removeEventListener = vi.fn(); (lynx as any).getNative = () => ({ addEventListener, removeEventListener }); const root = __root as unknown as SnapshotInstance; expect(root.childNodes.length).toBe(0); registerDestroyMts(); const cb = addEventListener.mock.calls[0]![1] as () => void; cb(); // Should not throw expect(lynx.performance.profileStart).toBeCalledWith('ReactLynx::destroyMts'); expect(lynx.performance.profileEnd).toBeCalled(); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/dark-candies-joke.mdpackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/__test__/snapshot/removeChildClearValues.test.tspackages/react/runtime/src/lynx.tspackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/src/snapshot.tspackages/react/runtime/src/snapshot/spread.tspackages/react/runtime/src/utils.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with the configuration specified in tsconfig.json
Files:
packages/react/runtime/src/snapshot/spread.tspackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/__test__/snapshot/removeChildClearValues.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.tspackages/react/runtime/src/snapshot.ts
packages/react/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use JSX syntax and ReactLynx components in React component files within packages/react/
Files:
packages/react/runtime/src/snapshot/spread.tspackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/__test__/snapshot/removeChildClearValues.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.tspackages/react/runtime/src/snapshot.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx}: Follow eslint rules as configured in eslint.config.js including React and TypeScript specific rules
Follow code formatting rules specified in .dprint.jsonc and biome.jsonc
Files:
packages/react/runtime/src/snapshot/spread.tspackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/__test__/snapshot/removeChildClearValues.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.tspackages/react/runtime/src/snapshot.ts
**/__test__/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/__test__/**/*.{ts,tsx,js}: Place test files in test/ directories adjacent to source files
Use vitest-based unit tests as configured in vitest.config.ts
Files:
packages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/__test__/snapshot/removeChildClearValues.test.ts
🧠 Learnings (30)
📓 Common learnings
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T05:10:01.608Z
Learning: Applies to packages/react/components/**/*.{ts,tsx} : Optimize component library in packages/react/components/ using ReactLynx syntax
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Learnt from: gaoachao
Repo: lynx-family/lynx-stack PR: 1771
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_component_with_static_sibling.js:2-2
Timestamp: 2025-09-18T04:43:54.426Z
Learning: In the lynx-family/lynx-stack repository, the `add_pure_comment` function in packages/react/transform/src/swc_plugin_compat/mod.rs (around lines 478-482) is specifically for `wrapWithLynxComponent` calls, not `createSnapshot` calls. The PURE comment injection for `createSnapshot` is handled separately in swc_plugin_snapshot/mod.rs.
📚 Learning: 2025-11-04T10:15:14.965Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1899
File: packages/react/runtime/__test__/snapshotPatch.test.jsx:725-749
Timestamp: 2025-11-04T10:15:14.965Z
Learning: In packages/react/runtime/src/snapshot.ts, the snapshotCreatorMap type signature uses `Record<string, (uniqId: string) => string>` (returning string) rather than void for backward compatibility. Old lazy bundles still use the pattern `const snapshot_xxx = createSnapshot(...)` directly, which requires createSnapshot to return a value. The snapshotCreatorMap creators that wrap createSnapshot calls must maintain the same return type to support these legacy bundles.
Applied to files:
packages/react/runtime/src/snapshot/spread.tspackages/react/runtime/__test__/snapshot/removeChildClearValues.test.tspackages/react/runtime/src/snapshot.ts
📚 Learning: 2025-08-21T07:21:51.621Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1562
File: packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs:261-283
Timestamp: 2025-08-21T07:21:51.621Z
Learning: In packages/react/transform/src/swc_plugin_snapshot/jsx_helpers.rs, the team prefers to keep the original unreachable! logic for JSXSpreadChild in jsx_is_children_full_dynamic function rather than implementing defensive error handling.
Applied to files:
packages/react/runtime/src/snapshot/spread.ts
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
Repo: lynx-family/lynx-stack PR: 1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, empty changeset files (containing only `---\n\n---`) are used for internal changes that modify src/** files but don't require meaningful release notes, such as private package changes or testing-only modifications. This satisfies CI requirements without generating user-facing release notes.
Applied to files:
.changeset/dark-candies-joke.md
📚 Learning: 2025-07-22T09:23:07.797Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/dark-candies-joke.md
📚 Learning: 2025-09-12T09:43:04.847Z
Learnt from: gaoachao
Repo: lynx-family/lynx-stack PR: 1736
File: .changeset/spotty-experts-smoke.md:1-3
Timestamp: 2025-09-12T09:43:04.847Z
Learning: In the lynx-family/lynx-stack repository, private packages (marked with "private": true in package.json) like lynx-js/react-transform don't require meaningful changeset entries even when their public APIs change, since they are not published externally and only affect internal development.
Applied to files:
.changeset/dark-candies-joke.mdpackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/src/lynx.ts
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
.changeset/dark-candies-joke.md
📚 Learning: 2025-12-26T05:10:01.608Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T05:10:01.608Z
Learning: Applies to packages/react/components/**/*.{ts,tsx} : Optimize component library in packages/react/components/ using ReactLynx syntax
Applied to files:
.changeset/dark-candies-joke.mdpackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/src/lynx.ts
📚 Learning: 2025-08-07T04:00:59.645Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1454
File: pnpm-workspace.yaml:46-46
Timestamp: 2025-08-07T04:00:59.645Z
Learning: In the lynx-family/lynx-stack repository, the webpack patch (patches/webpack5.101.0.patch) was created to fix issues with webpack5.99.9 but only takes effect on webpack5.100.0 and later versions. The patchedDependencies entry should use "webpack@^5.100.0" to ensure the patch applies to the correct version range.
Applied to files:
.changeset/dark-candies-joke.md
📚 Learning: 2025-08-12T16:09:32.413Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1497
File: packages/react/transform/tests/__swc_snapshots__/src/swc_plugin_snapshot/mod.rs/basic_full_static.js:9-10
Timestamp: 2025-08-12T16:09:32.413Z
Learning: In the Lynx stack, functions prefixed with `__` that are called in transformed code may be injected globally by the Lynx Engine at runtime rather than exported from the React runtime package. For example, `__CreateFrame` is injected globally by the Lynx Engine, not exported from lynx-js/react.
Applied to files:
.changeset/dark-candies-joke.mdpackages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.ts
📚 Learning: 2025-08-11T05:57:18.212Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1305
File: packages/testing-library/testing-environment/src/index.ts:255-258
Timestamp: 2025-08-11T05:57:18.212Z
Learning: In the ReactLynx testing environment (`packages/testing-library/testing-environment/src/index.ts`), the dual assignment pattern `target.console.method = console.method = () => {}` is required for rstest compatibility. This is because rstest provides `console` in an IIFE (Immediately Invoked Function Expression), and both the target and global console need to have these methods defined for proper test execution.
Applied to files:
packages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.ts
📚 Learning: 2025-08-21T08:46:54.494Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1370
File: packages/webpack/cache-events-webpack-plugin/src/LynxCacheEventsRuntimeModule.ts:23-27
Timestamp: 2025-08-21T08:46:54.494Z
Learning: In Lynx webpack runtime modules, the team prioritizes performance and simplicity over defensive runtime error handling. They prefer relying on compile-time type safety (TypeScript) rather than adding runtime checks like try-catch blocks or type validation, especially for performance-critical code like cache event setup/cleanup functions.
Applied to files:
packages/react/runtime/src/lynx/mts-destroy.tspackages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.ts
📚 Learning: 2025-07-16T06:26:22.230Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Applied to files:
packages/react/runtime/src/lynx/mts-destroy.ts
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/runtime/__test__/lynx/mts-destroy.test.ts
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/react/runtime/__test__/lynx/mts-destroy.test.ts
📚 Learning: 2025-10-11T06:16:12.517Z
Learnt from: Sherry-hue
Repo: lynx-family/lynx-stack PR: 1820
File: packages/web-platform/web-tests/tests/react.spec.ts:834-856
Timestamp: 2025-10-11T06:16:12.517Z
Learning: In packages/web-platform/web-tests/tests/react.spec.ts, the tests `basic-bindmouse` and `basic-mts-bindtouchstart` are NOT duplicates despite having similar test structures. They test different event types: `basic-bindmouse` validates mouse events (mousedown, mouseup, mousemove) with mouse-specific properties (button, buttons, x, y, pageX, pageY, clientX, clientY), while `basic-mts-bindtouchstart` validates touch events (touchstart) with touch arrays (touches, targetTouches, changedTouches). The similar test structure is coincidental and follows testing conventions.
Applied to files:
packages/react/runtime/__test__/lynx/mts-destroy.test.ts
📚 Learning: 2025-08-27T12:42:01.095Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1616
File: packages/webpack/cache-events-webpack-plugin/test/cases/not-cache-events/lazy-bundle/index.js:3-3
Timestamp: 2025-08-27T12:42:01.095Z
Learning: In webpack, properties like __webpack_require__.lynx_ce are injected during compilation/build time when webpack processes modules and generates bundles, not at runtime when dynamic imports execute. Tests for such properties don't need to wait for dynamic imports to complete.
Applied to files:
packages/react/runtime/__test__/lynx/mts-destroy.test.tspackages/react/runtime/src/utils.tspackages/react/runtime/src/lynx.ts
📚 Learning: 2026-01-04T11:17:24.013Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/web-platform/web-core-wasm/AGENTS.md:0-0
Timestamp: 2026-01-04T11:17:24.013Z
Learning: When modifying Rust core logic in src/main_thread, ALWAYS add corresponding tests in tests/element-apis.spec.ts to verify the JS-side behavior
Applied to files:
packages/react/runtime/__test__/lynx/mts-destroy.test.ts
📚 Learning: 2025-08-11T06:00:04.376Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1305
File: packages/react/testing-library/src/plugins/vitest.ts:59-61
Timestamp: 2025-08-11T06:00:04.376Z
Learning: In the lynx-family/lynx-stack repository, the `testingLibraryPlugin` in `packages/react/testing-library/src/plugins/vitest.ts` intentionally uses `process.exit` when jsdom installation fails, maintaining consistency with the previous implementation from `packages/react/testing-library/src/vitest.config.js`. This behavior should not be changed to use `this.error` despite being a Vite plugin best practice.
Applied to files:
packages/react/runtime/__test__/lynx/mts-destroy.test.ts
📚 Learning: 2025-10-29T10:28:27.519Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1899
File: packages/react/transform/crates/swc_plugin_snapshot/tests/__swc_snapshots__/lib.rs/should_static_extract_dynamic_inline_style.js:20-24
Timestamp: 2025-10-29T10:28:27.519Z
Learning: Files inside packages/react/transform/crates/swc_plugin_snapshot/tests/__swc_snapshots__/ are auto-generated test snapshot files and should not be manually updated. Any issues with the generated code should be addressed in the code generator/transform logic, not in the snapshots themselves.
Applied to files:
packages/react/runtime/__test__/snapshot/removeChildClearValues.test.ts
📚 Learning: 2025-12-26T05:10:01.608Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-26T05:10:01.608Z
Learning: Applies to **/__test__/**/*.{ts,tsx,js} : Use vitest-based unit tests as configured in vitest.config.ts
Applied to files:
packages/react/runtime/__test__/snapshot/removeChildClearValues.test.ts
📚 Learning: 2026-01-12T08:30:50.992Z
Learnt from: Sherry-hue
Repo: lynx-family/lynx-stack PR: 2104
File: packages/web-platform/web-mainthread-apis/ts/createMainThreadGlobalThis.ts:820-859
Timestamp: 2026-01-12T08:30:50.992Z
Learning: In packages/web-platform/web-mainthread-apis/ts/createMainThreadGlobalThis.ts, the `element` parameter in `__InvokeUIMethod` is a required parameter that is guaranteed to be provided by the caller, so defensive null/undefined checks are not necessary.
Applied to files:
packages/react/runtime/src/utils.ts
📚 Learning: 2025-11-06T01:19:23.670Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1917
File: packages/mcp-servers/devtool-mcp-server/tsconfig.json:8-8
Timestamp: 2025-11-06T01:19:23.670Z
Learning: The lynx-js/devtool-mcp-server package in lynx-family/lynx-stack targets Node.js >=18.19 (specified in its package.json engines), which is different from the root project's requirement of Node.js ^22 || ^24. The package uses "lib": ["ES2024.Promise"] in its tsconfig.json because it manually includes polyfills for Promise.withResolvers while maintaining compatibility with Node.js v18.
Applied to files:
packages/react/runtime/src/utils.ts
📚 Learning: 2025-09-28T07:52:03.601Z
Learnt from: Sherry-hue
Repo: lynx-family/lynx-stack PR: 1837
File: packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createNativeApp.ts:151-154
Timestamp: 2025-09-28T07:52:03.601Z
Learning: There are two different registerUpdateDataHandler functions in the lynx-stack codebase:
1. Main thread version in packages/web-platform/web-worker-runtime/src/mainThread/crossThreadHandlers/registerUpdateDataHandler.ts takes (mainThreadRpc: Rpc, backgroundThreadRpc: Rpc, runtime: MainThreadGlobalThis)
2. Background thread version in packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/crossThreadHandlers/registerUpdateDataHandler.ts takes only (rpc: Rpc, tt: NativeTTObject)
Applied to files:
packages/react/runtime/src/lynx.ts
📚 Learning: 2025-09-28T07:52:03.601Z
Learnt from: Sherry-hue
Repo: lynx-family/lynx-stack PR: 1837
File: packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createNativeApp.ts:151-154
Timestamp: 2025-09-28T07:52:03.601Z
Learning: There are two different registerUpdateDataHandler functions in the lynx-stack codebase:
1. Main thread version in packages/web-platform/web-worker-runtime/src/mainThread/crossThreadHandlers/registerUpdateDataHandler.ts takes (mainThreadRpc: Rpc, backgroundThreadRpc: Rpc, runtime: MainThreadGlobalThis)
2. Background thread version in packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/crossThreadHandlers/registerUpdateDataHandler.ts takes only (rpc: Rpc, tt: any)
Applied to files:
packages/react/runtime/src/lynx.ts
📚 Learning: 2025-09-28T08:46:43.177Z
Learnt from: f0rdream
Repo: lynx-family/lynx-stack PR: 1835
File: packages/react/worklet-runtime/src/workletRuntime.ts:52-55
Timestamp: 2025-09-28T08:46:43.177Z
Learning: The legacy worklet path with `_lepusWorkletHash` in `packages/react/worklet-runtime/src/workletRuntime.ts` is preserved for compatibility with MTS (Mini-app Threading Service) that doesn't support Initial Frame Rendering. This path will not be touched in current implementations.
Applied to files:
packages/react/runtime/src/lynx.ts
📚 Learning: 2025-08-13T11:46:43.737Z
Learnt from: colinaaa
Repo: lynx-family/lynx-stack PR: 1523
File: vitest.config.ts:5-6
Timestamp: 2025-08-13T11:46:43.737Z
Learning: In the lynx-stack codebase, default imports are consistently used for Node.js built-in modules (e.g., `import os from 'node:os'`, `import fs from 'node:fs'`). The TypeScript configuration supports esModuleInterop and allowSyntheticDefaultImports, making default imports the preferred pattern over namespace imports for Node.js built-ins.
Applied to files:
packages/react/runtime/src/lynx.ts
📚 Learning: 2026-01-12T11:23:23.257Z
Learnt from: CR
Repo: lynx-family/lynx-stack PR: 0
File: packages/rspeedy/create-rspeedy/template-common/AGENTS.md:0-0
Timestamp: 2026-01-12T11:23:23.257Z
Learning: When dealing with Lynx tasks, agents MUST read the llms.txt documentation at https://lynxjs.org/next/llms.txt
Applied to files:
packages/react/runtime/src/lynx.ts
📚 Learning: 2025-09-25T14:03:25.576Z
Learnt from: PupilTong
Repo: lynx-family/lynx-stack PR: 1834
File: packages/web-platform/web-worker-runtime/src/backgroundThread/background-apis/createChunkLoading.ts:162-171
Timestamp: 2025-09-25T14:03:25.576Z
Learning: In the lynx-stack codebase, for loadScriptAsync implementations in createChunkLoading.ts, unhandled promise rejections from readScriptAsync are intentionally not caught - the caller is expected to handle errors rather than the loadScriptAsync method itself invoking the callback with error messages.
Applied to files:
packages/react/runtime/src/lynx.ts
📚 Learning: 2025-08-19T12:49:05.875Z
Learnt from: upupming
Repo: lynx-family/lynx-stack PR: 1370
File: packages/webpack/template-webpack-plugin/src/LynxCacheEventsSetupListRuntimeModule.ts:21-25
Timestamp: 2025-08-19T12:49:05.875Z
Learning: In the Lynx cache events system, there are two separate runtime modules with distinct responsibilities: `LynxCacheEventsSetupListRuntimeModule` is only responsible for initializing the setup list with the setup functions, while `LynxCacheEventsRuntimeModule` guarantees the initialization of `loaded` and `cachedActions` properties. The modules have a dependency relationship where `lynxCacheEventsSetupList` is required by `lynxCacheEvents`.
Applied to files:
packages/react/runtime/src/lynx.ts
🧬 Code graph analysis (4)
packages/react/runtime/src/lynx/mts-destroy.ts (2)
packages/react/runtime/src/utils.ts (1)
isSdkVersionGt(51-55)packages/react/runtime/src/snapshot.ts (1)
SnapshotInstance(307-705)
packages/react/runtime/__test__/snapshot/removeChildClearValues.test.ts (2)
packages/react/runtime/__test__/utils/envManager.ts (1)
globalEnvManager(86-86)packages/react/runtime/src/snapshot.ts (2)
SnapshotInstance(307-705)createSnapshot(226-246)
packages/react/runtime/src/lynx.ts (1)
packages/react/runtime/src/lynx/mts-destroy.ts (1)
registerDestroyMts(9-14)
packages/react/runtime/src/snapshot.ts (1)
packages/react/runtime/src/pendingListUpdates.ts (1)
__pendingListUpdates(7-44)
🔇 Additional comments (17)
.changeset/dark-candies-joke.md (1)
1-5: LGTM!The changeset correctly documents this as a patch release for a bugfix. The description accurately reflects the memory leak fix for main thread events and gestures in PrimJS RC mode.
packages/react/runtime/src/snapshot/spread.ts (1)
49-50: LGTM!The defensive fallback to an empty object prevents runtime errors when
snapshot.__values![index]isundefinedornull. This aligns with the value-clearing behavior introduced inclearSnapshotInstanceValuesWithUpdater, ensuringupdateSpreadcan safely operate even after values have been cleared during removal.packages/react/runtime/src/utils.ts (2)
42-49: LGTM!The
safeMtsSystemInfo()function provides a robust fallback chain for accessingSystemInfoacross different runtime environments (global scope, lynx object, and test environments). This ensuresisSdkVersionGtworks reliably in all contexts, including MTS whereSystemInfomay be accessed differently.
51-52: LGTM!Using
safeMtsSystemInfo().lynxSdkVersionwith a'1.0'fallback ensures the version check degrades gracefully when SDK version info is unavailable, preventing runtime errors in edge cases or test environments.packages/react/runtime/src/snapshot.ts (2)
631-634: LGTM!Correctly invokes
clearSnapshotInstanceValuesWithUpdaterbefore removing the instance from the manager, ensuring event handlers and gestures are properly cleaned up during list holder removal.
647-659: LGTM!The value clearing is properly integrated into the non-list removal path, ensuring updaters run before the snapshot instance is fully cleaned up. This should address the memory leak for events and gestures by triggering their cleanup handlers.
packages/react/runtime/src/lynx.ts (2)
18-18: LGTM!Import correctly placed alongside other lynx module imports.
34-42: LGTM!
registerDestroyMts()is correctly invoked unconditionally within the__MAIN_THREAD__block, ensuring the destruction lifecycle hook is registered in all builds (not just dev). The function itself handles the SDK version check internally, so no additional guards are needed here.packages/react/runtime/__test__/lynx/mts-destroy.test.ts (3)
1-6: LGTM!Imports are correctly structured and follow the vitest-based testing pattern as per project conventions.
8-22: LGTM!The test setup correctly saves and restores
getNativeandSystemInfobetween tests, and usesvi.restoreAllMocks()for proper cleanup. The pattern of storing originals inbeforeEachand restoring inafterEachensures test isolation.
24-31: LGTM!Good negative test case verifying that event listener registration is skipped for SDK version 3.3 (not greater than 3.3).
packages/react/runtime/__test__/snapshot/removeChildClearValues.test.ts (4)
1-10: LGTM!Imports and test setup follow project conventions. The
beforeEachcorrectly resets the environment for test isolation.
12-22: Inline assertions in mocks strengthen the test.The updaters include inline
expectassertions verifying thatctx.__values?.[index]isundefinedat the time the updater is called. This confirms the clearing happens before the updater invocation, which is the intended behavior.
24-44: LGTM!The snapshot setup correctly defines:
- A parent with
ListChildrenslot type (triggering list-holder behavior)- A child with three updaters matching the
__valuesarrayThe
updatearray passed as[updater0, updater1, updater2] as anyaligns with the expected signature for snapshot updates.
46-60: LGTM!The test execution and assertions are well-structured:
- Creates parent/child relationship
- Removes child to trigger value clearing
- Verifies
updater0andupdater2are called with correct arguments- Verifies
updater1is not called (value wasundefined)- Confirms final value at index 2 is
undefinedThis provides good coverage for the
clearSnapshotInstanceValuesWithUpdaterfunctionality.packages/react/runtime/src/lynx/mts-destroy.ts (2)
1-7: LGTM!License header and imports are correctly structured. The type-only import for
SnapshotInstanceis appropriate.
9-14: LGTM!The SDK version check properly gates the feature registration, preventing registration on older SDK versions that don't support the
__DestroyLifetimeevent.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 410ba8bab3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merging this PR will degrade performance by 10.79%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#7364 Bundle Size — 384.24KiB (0%).9235133(current) vs 5e7e43c main#7359(baseline) Bundle metrics
|
| Current #7364 |
Baseline #7359 |
|
|---|---|---|
 Initial JS |
154.23KiB |
154.23KiB |
| Initial CSS |
35.05KiB |
35.05KiB |
 Cache Invalidation |
0% |
40.12% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
238(-0.42%) |
239 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.99% |
2.99% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7364 |
Baseline #7359 |
|
|---|---|---|
| JS |
252.17KiB |
252.17KiB |
| Other |
97.02KiB |
97.02KiB |
| CSS |
35.05KiB |
35.05KiB |
Bundle analysis report Branch Yradex:mts-remove-attrs-for-rc-2 Project dashboard
Generated by RelativeCI Documentation Report issue
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist