Skip to content

fix: properly cleanup __DestroyLifetime listeners and listCallbacks in snapshotDestroyList#2224

Merged
hzy merged 3 commits intomainfrom
fix/cleanup-list-callbacks-when-destory
Feb 24, 2026
Merged

fix: properly cleanup __DestroyLifetime listeners and listCallbacks in snapshotDestroyList#2224
hzy merged 3 commits intomainfrom
fix/cleanup-list-callbacks-when-destory

Conversation

@gaoachao
Copy link
Copy Markdown
Collaborator

@gaoachao gaoachao commented Feb 11, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Fixed cleanup of list destruction so listeners are removed and no lingering handlers remain.
    • List public methods now return safe default values after a list is removed instead of throwing.
  • Tests

    • Updated tests to reflect post-removal behavior and the improved listener cleanup.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).
  • Changeset added, and when a BREAKING CHANGE occurs, it needs to be clearly marked (or not required).

@gaoachao gaoachao requested review from HuJean and hzy as code owners February 11, 2026 12:10
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Feb 11, 2026

🦋 Changeset detected

Latest commit: 8543dfe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lynx-js/react Patch

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 11, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b9468d and 8543dfe.

📒 Files selected for processing (1)
  • packages/react/runtime/src/list.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/react/runtime/src/list.ts

📝 Walkthrough

Walkthrough

Registers per-list __DestroyLifetime callbacks at snapshot creation and ensures those listeners are removed and callbacks nulled on list destruction; adds a runtime guard for calls on removed lists and updates tests and a changeset to reflect the changed post-destruction behavior.

Changes

Cohort / File(s) Summary
Changeset
\.changeset/tender-otters-cheat.md
Adds a patch changeset for @lynx-js/react documenting cleanup of __DestroyLifetime listeners and snapshot list callback cleanup.
Snapshot implementation
packages/react/runtime/src/snapshot/list.ts
Introduce destroyLifetimeHandlerMap; register named __DestroyLifetime callback in snapshotCreateList; remove listener and map entry in snapshotDestroyList; set sentinel callbacks on destroy.
Runtime guard
packages/react/runtime/src/list.ts
Add guard in componentAtIndexFactory to throw if signMap/recycleMap are missing for a list (safeguard for calls after removal).
Tests
packages/react/runtime/__test__/list.test.jsx, packages/react/testing-library/src/__tests__/list.test.jsx
Update expectations: removed-list methods now return -1/undefined instead of throwing; add test verifying __DestroyLifetime listener removal on list removal.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • HuJean
  • hzy

Poem

🐰 I hopped through maps to tidy each thread,
Named callbacks unhooked, no listeners left spread.
Lists fold like napkins, their echoes grow thin,
I nudge every handler — then quietly grin. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing cleanup of __DestroyLifetime listeners and listCallbacks in snapshotDestroyList, which aligns with the core modifications across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/cleanup-list-callbacks-when-destory

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Feb 11, 2026

Merging this PR will degrade performance by 27.43%

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 60 untouched benchmarks
⏩ 3 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
002-hello-reactLynx-destroyBackground 662.1 µs 912.3 µs -27.43%
basic-performance-div-10000 460.1 ms 484.5 ms -5.02%
transform 1000 view elements 45.1 ms 42.2 ms +6.72%

Comparing fix/cleanup-list-callbacks-when-destory (8543dfe) with main (98b2504)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@relativeci
Copy link
Copy Markdown

relativeci bot commented Feb 11, 2026

Web Explorer

#7776 Bundle Size — 383.74KiB (0%).

8543dfe(current) vs 98b2504 main#7766(baseline)

Bundle metrics  no changes
                 Current
#7776
     Baseline
#7766
No change  Initial JS 154.88KiB 154.88KiB
No change  Initial CSS 35.06KiB 35.06KiB
No change  Cache Invalidation 0% 0%
No change  Chunks 8 8
No change  Assets 8 8
No change  Modules 238 238
No change  Duplicate Modules 16 16
No change  Duplicate Code 2.99% 2.99%
No change  Packages 4 4
No change  Duplicate Packages 0 0
Bundle size by type  no changes
                 Current
#7776
     Baseline
#7766
No change  JS 252.83KiB 252.83KiB
No change  Other 95.85KiB 95.85KiB
No change  CSS 35.06KiB 35.06KiB

Bundle analysis reportBranch fix/cleanup-list-callbacks-when-...Project dashboard


Generated by RelativeCIDocumentationReport issue

@gaoachao gaoachao requested a review from DwwWxx February 12, 2026 03:34
Copy link
Copy Markdown
Collaborator

@DwwWxx DwwWxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hzy hzy merged commit c6995ed into main Feb 24, 2026
46 of 47 checks passed
@hzy hzy deleted the fix/cleanup-list-callbacks-when-destory branch February 24, 2026 12:16
colinaaa pushed a commit that referenced this pull request Mar 2, 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/react@0.116.4

### Patch Changes

- Support `ReactLynx::hooks::setState` trace for function components.
([#2198](#2198))

- fix: properly cleanup `__DestroyLifetime` listeners and listCallbacks
in `snapshotDestroyList`.
([#2224](#2224))

## @lynx-js/qrcode-rsbuild-plugin@0.4.6

### Patch Changes

- Print all entries with all schema URLs in non-TTY environments instead
of only showing the first entry's QR code.
([#2227](#2227))

## @lynx-js/react-rsbuild-plugin@0.12.9

### Patch Changes

- Add alias for `use-sync-external-store/with-selector.js` and
`use-sync-external-store/shim/with-selector.js` pointing to
@lynx-js/use-sync-external-store.
([#2200](#2200))

- Updated dependencies
\[[`9033e2d`](9033e2d)]:
    -   @lynx-js/template-webpack-plugin@0.10.4
    -   @lynx-js/react-alias-rsbuild-plugin@0.12.9
    -   @lynx-js/use-sync-external-store@1.5.0
    -   @lynx-js/react-refresh-webpack-plugin@0.3.4
    -   @lynx-js/react-webpack-plugin@0.7.4
    -   @lynx-js/css-extract-webpack-plugin@0.7.0

## @lynx-js/css-serializer@0.1.4

### Patch Changes

- Move `cssChunksToMap` implementation from
`@lynx-js/template-webpack-plugin` to `@lynx-js/css-serializer` for
future reuse.
([#2269](#2269))

## @lynx-js/web-core-wasm@0.0.4

### Patch Changes

- Refactor web element templates and server-side rendering logic
([#2205](#2205))

- Updated dependencies
\[[`94e5779`](94e5779),
[`9033e2d`](9033e2d)]:
    -   @lynx-js/web-elements@0.11.3
    -   @lynx-js/css-serializer@0.1.4

## @lynx-js/web-elements@0.11.3

### Patch Changes

- fix: firefox 147+ layout issue
([#2205](#2205))

## @lynx-js/template-webpack-plugin@0.10.4

### Patch Changes

- Move `cssChunksToMap` implementation from
`@lynx-js/template-webpack-plugin` to `@lynx-js/css-serializer` for
future reuse.
([#2269](#2269))

- Updated dependencies
\[[`9033e2d`](9033e2d)]:
    -   @lynx-js/css-serializer@0.1.4

## @lynx-js/react-alias-rsbuild-plugin@0.12.9

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants