Merged
Conversation
🦋 Changeset detectedLatest commit: 8a24244 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 |
Contributor
📝 WalkthroughWalkthroughAdds a main-thread memoization hook, conditions and deduplicates gesture config/relationship updates to avoid unnecessary execId bumps, adds tests for idempotency and memo behavior, updates build task dependency, and adds a changeset recording a patch bump for the package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.ts (1)
53-56: Consider: Returningundefinedmay surprise callers ifThas a non-void return type.When
currentFnis falsy, the function returnsundefined as ReturnType<T>. IfTis typed as() => number, callers might not expectundefined. This is a defensive edge case (should rarely occur given the ref is initialized), but documenting this behavior or throwing an error might be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.ts` around lines 53 - 56, The helper useMainThreadMemoizedFn currently returns undefined when currentFn is falsy which can violate callers' expectations for non-void T; update the function to either (A) assert/panic when currentFn is missing (throw a descriptive Error inside useMainThreadMemoizedFn where currentFn is checked) so callers never get undefined, or (B) make the function's return type explicit about optionality (change the signature to ReturnType<T> | undefined and update any call sites/types). Locate the logic around currentFn in useMainThreadMemoizedFn and implement one of these fixes (prefer throwing a clear error referencing useMainThreadMemoizedFn/currentFn if you want fail-fast behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lynx/gesture-runtime/__test__/gesture-config.test.ts`:
- Around line 15-17: The MainThreadFunction mock defined as MainThreadFunction =
{ _wkltId: 'bdd4:dd564:2' } is unused in gesture-config.test.ts; either remove
that declaration to clean up the test file or, if it is intended for future
assertions, use it explicitly in the relevant test(s) (e.g., reference
MainThreadFunction where a worker/main-thread id is asserted) and add a short
comment explaining its purpose so its presence is justified.
---
Nitpick comments:
In `@packages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.ts`:
- Around line 53-56: The helper useMainThreadMemoizedFn currently returns
undefined when currentFn is falsy which can violate callers' expectations for
non-void T; update the function to either (A) assert/panic when currentFn is
missing (throw a descriptive Error inside useMainThreadMemoizedFn where
currentFn is checked) so callers never get undefined, or (B) make the function's
return type explicit about optionality (change the signature to ReturnType<T> |
undefined and update any call sites/types). Locate the logic around currentFn in
useMainThreadMemoizedFn and implement one of these fixes (prefer throwing a
clear error referencing useMainThreadMemoizedFn/currentFn if you want fail-fast
behavior).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/breezy-hounds-doubt.mdpackages/lynx/gesture-runtime/__test__/gesture-config.test.tspackages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsxpackages/lynx/gesture-runtime/src/baseGesture.tspackages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
f0rdream
force-pushed
the
gesture_removal
branch
2 times, most recently
from
380a88a to
cc2f49d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lynx/gesture-runtime/__test__/gesture-config.test.ts (1)
244-255: Consider extending this test to composed/self and same-id clone cases.Current coverage validates duplicate insertion for the same instance, but not for composed arrays that may contain
this, nor clone-equivalent gestures with the sameid. Adding those cases would protect the dedupe contract more completely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lynx/gesture-runtime/__test__/gesture-config.test.ts` around lines 244 - 255, Extend the existing test that asserts execId doesn't increment for duplicate relationships to also cover (1) composed/self relationships: call externalSimultaneous with arrays that include the gesture itself (e.g., pass an array containing pan1 and other gestures) and re-call with the same composed array to assert pan1.execId remains unchanged, and (2) clone-equivalent gestures: create another gesture instance with the same id as pan1 (or use whatever cloning API produces an instance with identical id) and call externalSimultaneous with that clone, asserting execId does not increment. Target the existing symbols PanGesture, TapGesture, externalSimultaneous, and execId when adding these assertions so the dedupe contract for composed/self and same-id clones is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lynx/gesture-runtime/src/baseGesture.ts`:
- Around line 200-207: The current expansion of composed gestures can add "this"
or duplicate gestures by object reference; update the creation of newGestures
(after calling gesture.toGestureArray()) to first filter out any gesture whose
identity matches the current instance (compare g === this or g.id === this.id)
and then dedupe by gesture.id (use a Set of ids to only keep the first
occurrence of each id) before computing newGestures and updating this.waitFor
and this.execId; apply the same id-based self-filter-and-dedupe logic to the
other equivalent blocks that perform this expansion (the other uses of
toGestureArray()/this.waitFor in the file).
---
Nitpick comments:
In `@packages/lynx/gesture-runtime/__test__/gesture-config.test.ts`:
- Around line 244-255: Extend the existing test that asserts execId doesn't
increment for duplicate relationships to also cover (1) composed/self
relationships: call externalSimultaneous with arrays that include the gesture
itself (e.g., pass an array containing pan1 and other gestures) and re-call with
the same composed array to assert pan1.execId remains unchanged, and (2)
clone-equivalent gestures: create another gesture instance with the same id as
pan1 (or use whatever cloning API produces an instance with identical id) and
call externalSimultaneous with that clone, asserting execId does not increment.
Target the existing symbols PanGesture, TapGesture, externalSimultaneous, and
execId when adding these assertions so the dedupe contract for composed/self and
same-id clones is validated.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/breezy-hounds-doubt.mdpackages/lynx/gesture-runtime/__test__/gesture-config.test.tspackages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsxpackages/lynx/gesture-runtime/src/baseGesture.tspackages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.tspackages/lynx/gesture-runtime/turbo.jsonc
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.ts
Merging this PR will improve performance by 6.14%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#7888 Bundle Size — 383.64KiB (0%).8a24244(current) vs 8ca97fe main#7876(baseline) Bundle metrics
|
| Current #7888 |
Baseline #7876 |
|
|---|---|---|
 Initial JS |
154.78KiB |
154.78KiB |
| Initial CSS |
35.06KiB |
35.06KiB |
| Cache Invalidation |
0% |
0% |
| 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 #7888 |
Baseline #7876 |
|
|---|---|---|
| JS |
252.72KiB |
252.72KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.06KiB |
35.06KiB |
Bundle analysis report Branch f0rdream:gesture_removal Project dashboard
Generated by RelativeCI Documentation Report issue
f0rdream
force-pushed
the
gesture_removal
branch
from
fd57d13 to
2ecdcbe
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/lynx/gesture-runtime/src/baseGesture.ts (1)
200-260: Consider extracting shared relation-dedupe logic into a helper.
externalWaitFor,externalSimultaneous, andexternalContinueWithnow share nearly identical filtering/dedupe blocks. A small helper would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lynx/gesture-runtime/src/baseGesture.ts` around lines 200 - 260, Extract the repeated gestures filtering/dedupe logic into a private helper (e.g., normalizeAndDedupeRelations) and call it from externalWaitFor, externalSimultaneous and externalContinueWith; the helper should accept the incoming GestureKind (or already-expanded BaseGesture[]), the current relation array (this.waitFor / this.simultaneousWith / this.continueWith) and the current instance (this) and return an array of new gestures to add (excluding self and existing IDs, preserving dedupe). Replace the in-method filter blocks with a call to this helper, then only increment execId and concat the returned newGestures when its length > 0 to preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lynx/gesture-runtime/src/baseGesture.ts`:
- Around line 200-260: Extract the repeated gestures filtering/dedupe logic into
a private helper (e.g., normalizeAndDedupeRelations) and call it from
externalWaitFor, externalSimultaneous and externalContinueWith; the helper
should accept the incoming GestureKind (or already-expanded BaseGesture[]), the
current relation array (this.waitFor / this.simultaneousWith /
this.continueWith) and the current instance (this) and return an array of new
gestures to add (excluding self and existing IDs, preserving dedupe). Replace
the in-method filter blocks with a call to this helper, then only increment
execId and concat the returned newGestures when its length > 0 to preserve
existing behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/breezy-hounds-doubt.mdpackages/lynx/gesture-runtime/__test__/gesture-config.test.tspackages/lynx/gesture-runtime/__test__/gesture-relations.test.tsxpackages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsxpackages/lynx/gesture-runtime/src/baseGesture.tspackages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.tspackages/lynx/gesture-runtime/turbo.jsonc
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/lynx/gesture-runtime/turbo.jsonc
- packages/lynx/gesture-runtime/test/useMainThreadMemoizedFn.test.tsx
Yradex
reviewed
Mar 2, 2026
f0rdream
force-pushed
the
gesture_removal
branch
from
2ecdcbe to
8a24244
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/lynx/gesture-runtime/__test__/gesture-config.test.ts (1)
244-255: Optionally assert relation storage state in addition toexecId.You can strengthen this test by asserting the relation list length stays unchanged (e.g.,
simultaneousWith.length === 1) after duplicate registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lynx/gesture-runtime/__test__/gesture-config.test.ts` around lines 244 - 255, The test should also assert that adding a duplicate relationship does not change the internal relation storage: after calling pan1.externalSimultaneous(tap1) the second time, in addition to expecting pan1.execId to remain 1, assert that the internal relation list (e.g., pan1.simultaneousWith or whatever collection is used by PanGesture to track externalSimultaneous relationships) still has length 1 to ensure duplicates are not stored; update the test case around PanGesture, tap1, externalSimultaneous, execId, and simultaneousWith to include this extra expectation.packages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsx (1)
16-89: Consider extracting shared test harness for readability.The first two tests duplicate
Appsetup and update flow; a small helper would reduce repetition and future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsx` around lines 16 - 89, Both tests duplicate the same component/setup/update flow; extract a shared test harness to reduce repetition by creating a helper function (e.g., createMemoizedApp or mountMemoizedApp) that renders the App which uses useMainThreadMemoizedFn and returns {memoizedFnRef, setCount} (or exposes the run/render options), then replace the inline App definitions in both tests with calls to that helper and reuse its returned memoizedFnRef and _setCount; ensure the helper supports passing render options (enableMainThread/enableBackgroundThread) so both test variants can call runWorklet and act as before.packages/lynx/gesture-runtime/src/baseGesture.ts (1)
151-165: Optional: unify duplicated callback-update logic.
BaseGesture.updateCallbackandContinuousGesture.updateCallbacknow carry near-identical control flow; extracting shared internals would lower maintenance overhead.Also applies to: 279-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lynx/gesture-runtime/src/baseGesture.ts` around lines 151 - 165, BaseGesture.updateCallback and ContinuousGesture.updateCallback duplicate the same control flow; extract the shared logic into a single private helper (e.g., BaseGesture.prototype.setWrappedCallback or similar) that takes (k: keyof typeof this.callbacks, cb: GestureCallback<TEvent>) and performs the execId bump check, wraps the callback via wrapCallback(this.id, k) and assigns into this.callbacks preserving types; then have both BaseGesture.updateCallback and ContinuousGesture.updateCallback call that helper so execId, callbacks, id, and wrapCallback usage are centralized and types remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/lynx/gesture-runtime/__test__/gesture-config.test.ts`:
- Around line 244-255: The test should also assert that adding a duplicate
relationship does not change the internal relation storage: after calling
pan1.externalSimultaneous(tap1) the second time, in addition to expecting
pan1.execId to remain 1, assert that the internal relation list (e.g.,
pan1.simultaneousWith or whatever collection is used by PanGesture to track
externalSimultaneous relationships) still has length 1 to ensure duplicates are
not stored; update the test case around PanGesture, tap1, externalSimultaneous,
execId, and simultaneousWith to include this extra expectation.
In `@packages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsx`:
- Around line 16-89: Both tests duplicate the same component/setup/update flow;
extract a shared test harness to reduce repetition by creating a helper function
(e.g., createMemoizedApp or mountMemoizedApp) that renders the App which uses
useMainThreadMemoizedFn and returns {memoizedFnRef, setCount} (or exposes the
run/render options), then replace the inline App definitions in both tests with
calls to that helper and reuse its returned memoizedFnRef and _setCount; ensure
the helper supports passing render options
(enableMainThread/enableBackgroundThread) so both test variants can call
runWorklet and act as before.
In `@packages/lynx/gesture-runtime/src/baseGesture.ts`:
- Around line 151-165: BaseGesture.updateCallback and
ContinuousGesture.updateCallback duplicate the same control flow; extract the
shared logic into a single private helper (e.g.,
BaseGesture.prototype.setWrappedCallback or similar) that takes (k: keyof typeof
this.callbacks, cb: GestureCallback<TEvent>) and performs the execId bump check,
wraps the callback via wrapCallback(this.id, k) and assigns into this.callbacks
preserving types; then have both BaseGesture.updateCallback and
ContinuousGesture.updateCallback call that helper so execId, callbacks, id, and
wrapCallback usage are centralized and types remain intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/breezy-hounds-doubt.mdpackages/lynx/gesture-runtime/__test__/gesture-config.test.tspackages/lynx/gesture-runtime/__test__/gesture-relations.test.tsxpackages/lynx/gesture-runtime/__test__/useMainThreadMemoizedFn.test.tsxpackages/lynx/gesture-runtime/src/baseGesture.tspackages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.tspackages/lynx/gesture-runtime/turbo.jsonc
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/lynx/gesture-runtime/test/gesture-relations.test.tsx
- packages/lynx/gesture-runtime/src/utils/useMainThreadMemoizedFn.ts
- packages/lynx/gesture-runtime/turbo.jsonc
Yradex
approved these changes
Mar 2, 2026
colinaaa
pushed a commit
that referenced
this pull request
Mar 9, 2026
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/devtool-mcp-server@0.5.0 ### Minor Changes - Use `@lynx-js/devtool-connector` instead of `@lynx-js/debug-router-connector`. ([#2284](#2284)) The new connector avoids using keep-alive connections, which makes the connection much more reliable. - **BREAKING CHANGE**: Remove the `./debug-router-connector` exports. ([#2284](#2284)) ## @lynx-js/web-elements@0.12.0 ### Minor Changes - feat: add `willchange` event to `x-viewpager-ng` ([#2305](#2305)) ### Patch Changes - fix: firefox `@supports(width:1rex)` ([#2288](#2288)) - fix: check computed overflow style in `getTheMostScrollableKid` to avoid treating `overflow: visible` elements as scroll containers ([#2309](#2309)) - fix: the inline-truncation should only work as a direct child of x-text ([#2287](#2287)) - fix: getVisibleCells cannot work in firefox due to contentvisibilityautostatechange not propagate list-item ([#2308](#2308)) - fix: foldview stuck issue ([#2304](#2304)) ## @lynx-js/gesture-runtime@2.1.3 ### Patch Changes - Optimize gesture callbacks and relationships to prevent unnecessary gesture registration and rerenders. ([#2277](#2277)) ## @lynx-js/react@0.116.5 ### Patch Changes - Improve React runtime hook profiling. ([#2235](#2235)) Enable Profiling recording first, then enter the target page so the trace includes full render/hydrate phases. - Record trace events for `useEffect` / `useLayoutEffect` hook entry, callback, and cleanup phases. - Log trace events for `useState` setter calls. - Wire `profileFlowId` support in debug profile utilities and attach flow IDs to related hook traces. - Instrument hydrate/background snapshot profiling around patch operations with richer args (e.g. snapshot id/type, dynamic part index, value type, and source when available). - Capture vnode source mapping in dev and use it in profiling args to improve trace attribution. - Expand debug test coverage for profile utilities, hook profiling behavior, vnode source mapping, and hydrate profiling branches. - refactor: call loadWorkletRuntime once in each module ([#2315](#2315)) ## @lynx-js/rspeedy@0.13.5 ### Patch Changes - feat: opt-in the web platform's new binary output format ([#2281](#2281)) Introduce a new flag to enable the new binary output format. Currently it's an internal-use-only flag that will be removed in the future; set the corresponding environment variable to 'true' to enable it. - Avoid generating `Rsbuild vundefined` in greeting message. ([#2275](#2275)) - Updated dependencies \[]: - @lynx-js/web-rsbuild-server-middleware@0.19.8 ## @lynx-js/lynx-bundle-rslib-config@0.2.2 ### Patch Changes - Support bundle and load css in external bundle ([#2143](#2143)) ## @lynx-js/external-bundle-rsbuild-plugin@0.0.3 ### Patch Changes - Updated dependencies \[[`c28b051`](c28b051), [`4cbf809`](4cbf809)]: - @lynx-js/externals-loading-webpack-plugin@0.0.4 ## @lynx-js/react-rsbuild-plugin@0.12.10 ### Patch Changes - Support bundle and load css in external bundle ([#2143](#2143)) - Updated dependencies \[[`59f2933`](59f2933), [`453e006`](453e006)]: - @lynx-js/template-webpack-plugin@0.10.5 - @lynx-js/css-extract-webpack-plugin@0.7.0 - @lynx-js/react-webpack-plugin@0.7.4 - @lynx-js/react-alias-rsbuild-plugin@0.12.10 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 ## @lynx-js/web-core-wasm@0.0.5 ### Patch Changes - Updated dependencies \[[`4963907`](4963907), [`8fd936a`](8fd936a), [`0d41253`](0d41253), [`d32c4c6`](d32c4c6), [`7518b72`](7518b72), [`fca9d4a`](fca9d4a)]: - @lynx-js/web-elements@0.12.0 ## @lynx-js/externals-loading-webpack-plugin@0.0.4 ### Patch Changes - perf: optimize external bundle loading by merging multiple `fetchBundle` calls for the same URL into a single request. ([#2307](#2307)) - Support bundle and load css in external bundle ([#2143](#2143)) ## @lynx-js/template-webpack-plugin@0.10.5 ### Patch Changes - feat: allow `templateDebugUrl` to be customized via `output.publicPath` or the `beforeEncode` hook. ([#2274](#2274)) - feat: opt-in the web platform's new binary output format ([#2281](#2281)) Introduce a new flag to enable the new binary output format. Currently it's an internal-use-only flag that will be removed in the future; set the corresponding environment variable to 'true' to enable it. - Updated dependencies \[]: - @lynx-js/web-core-wasm@0.0.5 ## create-rspeedy@0.13.5 ## @lynx-js/react-alias-rsbuild-plugin@0.12.10 ## upgrade-rspeedy@0.13.5 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Optimize gesture callbacks and relationships to prevent unnecessary gesture registration and rerenders.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Checklist