fix(web): ensure x-list scrolltolower observer sentinel is detectable#2484
fix(web): ensure x-list scrolltolower observer sentinel is detectable#2484Sherry-hue merged 1 commit intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: f6140c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA changeset was added and XList sentinel elements gained Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web-platform/web-elements/src/elements/XList/x-list.css (1)
160-189: Nit: intentional asymmetry between upper and lower sentinel transforms?The lower sentinel gets both
margin-top: -1pxandtranslateY(1px)(and horizontal equivalents), while the upper sentinel only getsmargin-bottom: -1pxwith no counter-transform. If this asymmetry is deliberate (to push the lower sentinel just past the scroll boundary so the bottomIntersectionObserverfires at the edge), consider a brief comment in the CSS explaining why — it will save future maintainers from "symmetrizing" it and re-breaking the fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-elements/src/elements/XList/x-list.css` around lines 160 - 189, Add a brief clarifying comment above the sentinel CSS rules (the selectors using ::part(upper-threshold-sentinel) and ::part(lower-threshold-sentinel) and their horizontal variants) explaining that the lower sentinel intentionally uses margin-top: -1px plus transform: translateY(1px) (and translateX(1px) for horizontal) to push it just past the scroll boundary so the IntersectionObserver fires at the edge, and that the upper sentinel intentionally omits the counter-transform—this prevents future maintainers from “symmetrizing” the rules and reintroducing the bug.
🤖 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/web-platform/web-elements/src/elements/XList/x-list.css`:
- Around line 160-189: Add a brief clarifying comment above the sentinel CSS
rules (the selectors using ::part(upper-threshold-sentinel) and
::part(lower-threshold-sentinel) and their horizontal variants) explaining that
the lower sentinel intentionally uses margin-top: -1px plus transform:
translateY(1px) (and translateX(1px) for horizontal) to push it just past the
scroll boundary so the IntersectionObserver fires at the edge, and that the
upper sentinel intentionally omits the counter-transform—this prevents future
maintainers from “symmetrizing” the rules and reintroducing the bug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81801664-c1a0-400f-b557-7a9e76c964a3
📒 Files selected for processing (4)
.changeset/wild-wolves-love.mdpackages/web-platform/web-elements/src/elements/XList/x-list.csspackages/web-platform/web-elements/src/elements/htmlTemplates.tspackages/web-platform/web-elements/src/template.rs
Merging this PR will degrade performance by 19.25%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | 003-hello-list-destroyBackground |
2.8 ms | 3.4 ms | -19.25% |
| ⚡ | 002-hello-reactLynx-destroyBackground |
832.6 µs | 669.2 µs | +24.42% |
| ⚡ | transform 1000 view elements |
46.8 ms | 41.9 ms | +11.67% |
Comparing Sherry-hue:fix/list-lower (f6140c8) with main (25b09e9)
Footnotes
-
26 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. ↩
React External#616 Bundle Size — 583.28KiB (0%).f6140c8(current) vs 25b09e9 main#604(baseline) Bundle metrics
|
| Current #616 |
Baseline #604 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
17 |
17 |
|
5 |
5 |
|
8.59% |
8.59% |
|
0 |
0 |
|
0 |
0 |
Bundle analysis report Branch Sherry-hue:fix/list-lower Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9072 Bundle Size — 899.89KiB (+0.13%).f6140c8(current) vs 25b09e9 main#9060(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch Sherry-hue:fix/list-lower Project dashboard Generated by RelativeCI Documentation Report issue |
React MTF Example#631 Bundle Size — 195.57KiB (0%).f6140c8(current) vs 25b09e9 main#619(baseline) Bundle metrics
|
| Current #631 |
Baseline #619 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
173 |
173 |
|
66 |
66 |
|
44% |
44% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #631 |
Baseline #619 |
|
|---|---|---|
111.23KiB |
111.23KiB |
|
84.34KiB |
84.34KiB |
Bundle analysis report Branch Sherry-hue:fix/list-lower Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7498 Bundle Size — 224.41KiB (0%).f6140c8(current) vs 25b09e9 main#7486(baseline) Bundle metrics
|
| Current #7498 |
Baseline #7486 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
179 |
179 |
|
69 |
69 |
|
44.51% |
44.51% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #7498 |
Baseline #7486 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
78.65KiB |
78.65KiB |
Bundle analysis report Branch Sherry-hue:fix/list-lower Project dashboard
Generated by RelativeCI Documentation Report issue
fc03d1a to
f6140c8
Compare
Summary by CodeRabbit
Checklist