fix(web): when a list-item is deleted from list, the deleted list-ite…#1092
Conversation
🦋 Changeset detectedLatest commit: 07a424a The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
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 |
CodSpeed Performance ReportMerging this PR will improve performance by 7.81%Comparing Summary
Performance Changes
Footnotes
|
|
Good job! But the test feels a bit naive... A stress test like rendering a randomly shuffled array can help make sure your implementation correct. function shuffleArray(array) {
for (let i = array.length - 1; i >= 1; i--) {
const j = Math.floor(Math.random() * (i + 1));
[array[i], array[j]] = [array[j], array[i]];
}
} |
Web Explorer#7302 Bundle Size — 383.96KiB (+0.05%).07a424a(current) vs 5b589ab main#7300(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Sherry-hue:fix/list-item-remove Project dashboard Generated by RelativeCI Documentation Report issue |
React Example#1733 Bundle Size — 232.61KiB (0%).04dc052(current) vs e10c9fc main#1725(baseline) Bundle metrics
|
| Current #1733 |
Baseline #1725 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
143 |
143 |
|
55 |
55 |
|
45.68% |
45.68% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #1733 |
Baseline #1725 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
86.86KiB |
86.86KiB |
Bundle analysis report Branch Sherry-hue:fix/list-item-remove Project dashboard
Generated by RelativeCI Documentation Report issue
a7ba37e to
04dc052
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump"). |
04dc052 to
106639b
Compare
📝 WalkthroughWalkthroughFixes list-item deletion visibility by changing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.changeset/cuddly-roses-start.md:
- Around line 7-9: Update the release-note text to improve grammar and clarity:
rephrase the entry about deleted list-items so it reads something like "Fix:
when a list-item is deleted, it could still be shown because enqueueComponent
does not remove the node from the Element Tree; enqueueComponent only maintains
the display node on RL, so lynx web must explicitly delete the DOM node." Ensure
you reference the terms enqueueComponent, Element Tree, RL, and lynx web exactly
as in the note and replace the current two-sentence fragment with this single
clear sentence.
🧹 Nitpick comments (3)
packages/web-platform/web-tests/tests/react/basic-element-list-remove-action/index.css (1)
18-23: Addborder-styleso the list-item border actually renders.Without a border style, browsers treat the border as
none, so the green border won’t show. Consider addingborder-style: solidfor clarity in screenshot-based tests.[select_commentary]
♻️ Suggested change
list-item { border-width: 5px; + border-style: solid; border-color: green; width: 100%; height: 100px; background-color: yellow; }packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
1399-1550: Consider a deterministic shuffle stress test for list mutations.The three tests cover core flows, but a shuffled sequence of insert/remove positions would better exercise edge ordering. To avoid flakiness, keep the shuffle deterministic (seeded or fixed permutations).
🧪 Example deterministic shuffle helper
const makeRng = (seed = 123456789) => () => { seed = (1664525 * seed + 1013904223) >>> 0; return seed / 2 ** 32; }; const shuffleArray = <T,>(arr: T[], rng = makeRng()) => { for (let i = arr.length - 1; i > 0; i--) { const j = Math.floor(rng() * (i + 1)); [arr[i], arr[j]] = [arr[j], arr[i]]; } return arr; };packages/web-platform/web-tests/tests/react.spec.ts (1)
4647-4660: Consider adding a stress test with shuffled array rendering.The test covers basic sequential removal scenarios, which validates the fix. Per reviewer feedback, consider adding an additional test case that renders a randomly shuffled array to more rigorously validate the removal logic under varied ordering conditions.
The suggested
shuffleArrayfunction from the review comments:function shuffleArray(array) { for (let i = array.length - 1; i >= 1; i--) { const j = Math.floor(Math.random() * (i + 1)); [array[i], array[j]] = [array[j], array[i]]; } }
106639b to
ac8e7f4
Compare
7ec6952 to
0f0f17a
Compare
…m is still showed incorrectly
0f0f17a to
07a424a
Compare
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.0 ### Minor Changes - **BREAKING CHANGE**: Bump Preact from [10.24.0](preactjs/preact@1807173) to [10.28.0](preactjs/preact@f7693b7), see diffs at [hzy/preact#6](hzy/preact#6). ([#2042](#2042)) ### Patch Changes - Add safety checks for compilation macros to prevent runtime errors when they are undefined. ([#2110](#2110)) Replaces direct usage of `__PROFILE__`, `__MAIN_THREAD__`, `__BACKGROUND__` with `typeof` checks. This improves robustness by checking variable existence before access, preventing runtime errors in environments where compilation macros are not defined. ## @lynx-js/lynx-bundle-rslib-config@0.2.0 ### Minor Changes - Use `LAYERS` exposed by DSL plugins ([#2114](#2114)) ## @lynx-js/gesture-runtime@2.1.2 ### Patch Changes - Fix an issue that `NativeGesture` does not get correct types in callback ([#2130](#2130)) ## @lynx-js/rspeedy@0.13.1 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-rsbuild-server-middleware@0.19.6 ## @lynx-js/react-rsbuild-plugin@0.12.6 ### Patch Changes - Support using `pluginReactLynx` in Rslib. ([#2114](#2114)) - Updated dependencies \[[`4cd7182`](4cd7182)]: - @lynx-js/template-webpack-plugin@0.10.2 - @lynx-js/react-alias-rsbuild-plugin@0.12.6 - @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.3 - @lynx-js/css-extract-webpack-plugin@0.7.0 ## upgrade-rspeedy@0.13.1 ### Patch Changes - Fix the issue `rslib-runtime.js` was not published in dist folder. ([#2122](#2122)) ## @lynx-js/testing-environment@0.1.10 ### Patch Changes - Fix the error "lynxTestingEnv is not defined" ([#2118](#2118)) ## @lynx-js/web-constants@0.19.6 ### Patch Changes - feat: add main-thread API: \_\_QuerySelector ([#2115](#2115)) - fix: when a list-item is deleted from list, the deleted list-item is still showed incorrectly. ([#1092](#1092)) This is because the `enqueueComponent` method does not delete the node from the Element Tree. It is only to maintain the display node on RL, and lynx web needs to delete the dom additionally. - feat: support main thread invoke ui method ([#2104](#2104)) - feat: support lynx.reload() ([#2127](#2127)) - Updated dependencies \[[`f7133c1`](f7133c1)]: - @lynx-js/web-worker-rpc@0.19.6 ## @lynx-js/web-core@0.19.6 ### Patch Changes - fix: avoid crash on CPUs that do not support SIMD ([#2133](#2133)) - feat: support lynx.reload() ([#2127](#2127)) - Updated dependencies \[[`179f984`](179f984), [`f7133c1`](f7133c1), [`6c2b51a`](6c2b51a), [`556fe9f`](556fe9f), [`5b589ab`](5b589ab)]: - @lynx-js/web-constants@0.19.6 - @lynx-js/web-mainthread-apis@0.19.6 - @lynx-js/web-worker-rpc@0.19.6 - @lynx-js/web-worker-runtime@0.19.6 ## @lynx-js/web-core-wasm@0.0.1 ### Patch Changes - Updated dependencies \[[`f7133c1`](f7133c1)]: - @lynx-js/web-worker-rpc@0.19.6 ## @lynx-js/web-mainthread-apis@0.19.6 ### Patch Changes - feat: add main-thread API: \_\_QuerySelector ([#2115](#2115)) - fix: when a list-item is deleted from list, the deleted list-item is still showed incorrectly. ([#1092](#1092)) This is because the `enqueueComponent` method does not delete the node from the Element Tree. It is only to maintain the display node on RL, and lynx web needs to delete the dom additionally. - feat: support main thread invoke ui method ([#2104](#2104)) - fix: mts && bts events can be binded both ([#2121](#2121)) - Updated dependencies \[[`179f984`](179f984), [`f7133c1`](f7133c1), [`6c2b51a`](6c2b51a), [`5b589ab`](5b589ab)]: - @lynx-js/web-constants@0.19.6 ## @lynx-js/web-worker-rpc@0.19.6 ### Patch Changes - fix: when a list-item is deleted from list, the deleted list-item is still showed incorrectly. ([#1092](#1092)) This is because the `enqueueComponent` method does not delete the node from the Element Tree. It is only to maintain the display node on RL, and lynx web needs to delete the dom additionally. ## @lynx-js/web-worker-runtime@0.19.6 ### Patch Changes - feat: support lynx.reload() ([#2127](#2127)) - Updated dependencies \[[`179f984`](179f984), [`f7133c1`](f7133c1), [`6c2b51a`](6c2b51a), [`556fe9f`](556fe9f), [`5b589ab`](5b589ab)]: - @lynx-js/web-constants@0.19.6 - @lynx-js/web-mainthread-apis@0.19.6 - @lynx-js/web-worker-rpc@0.19.6 ## @lynx-js/template-webpack-plugin@0.10.2 ### Patch Changes - Polyfill `lynx.requireModuleAsync` to allow cache same parallel requests. ([#2108](#2108)) ## create-rspeedy@0.13.1 ## @lynx-js/react-alias-rsbuild-plugin@0.12.6 ## @lynx-js/web-core-server@0.19.6 ## @lynx-js/web-rsbuild-server-middleware@0.19.6 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…m is still showed incorrectly
Summary
fix: when a list-item is deleted from list, the deleted list-item is still showed incorrectly.
This is because the
enqueueComponentmethod does not delete the node from the Element Tree. It is only to maintain all nodes on the RL side, and lynx web needs to delete the dom additionally.Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Style
Notes
✏️ Tip: You can customize this high-level summary in your review settings.