feat: reimplement XFoldViewNg scrolling using CSS transforms and cust…#2299
Open
PupilTong wants to merge 3 commits intolynx-family:mainfrom
Open
feat: reimplement XFoldViewNg scrolling using CSS transforms and cust…#2299PupilTong wants to merge 3 commits intolynx-family:mainfrom
PupilTong wants to merge 3 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: bf134ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
Contributor
📝 WalkthroughWalkthroughReimplements XFoldViewNg scrolling: replaces parent-resizeObserver approach with component-local ResizeObservers and CSS-transform scrolling, introduces symbol-keyed APIs (updateHeaderHeight, updateToolbarHeight, scrollCallbacks), centralizes touch/wheel handling into XFoldviewNgTouchEventsHandler, updates styles, tests, and build browserslist. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts (1)
65-85:⚠️ Potential issue | 🟠 MajorNested-foldview protection is incomplete (touchmove/wheel still leak to ancestors).
Line 124 gates only
#touchStart. Ancestor foldviews can still run#handleTouch/#handleWheel, callpreventDefault, and mutate scroll state for the same gesture.🐛 Proposed fix
+ `#isClosestFoldviewEvent`(path: EventTarget[]): boolean { + const closestFoldview = path.find( + el => el instanceof Element && el.tagName === 'X-FOLDVIEW-NG', + ); + return closestFoldview === this.#dom; + } + `#handleTouch` = (event: TouchEvent) => { + if (!this.#isClosestFoldviewEvent(event.composedPath())) { + return; + } if (this.#dom.getAttribute('scroll-enable') === 'false') { return; } @@ `#handleWheel` = (event: WheelEvent) => { + if (!this.#isClosestFoldviewEvent(event.composedPath())) { + return; + } if (this.#dom.getAttribute('scroll-enable') === 'false') { return; } @@ - const pathElements = event.composedPath(); - const closestFoldview = pathElements.find(el => - el instanceof Element && el.tagName === 'X-FOLDVIEW-NG' - ); - if (closestFoldview !== this.#dom) { + if (!this.#isClosestFoldviewEvent(event.composedPath())) { this.#elements = []; return; }Also applies to: 87-113, 119-127
🤖 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/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts` around lines 65 - 85, Nested foldviews still receive touchmove/wheel and mutate shared gesture state; update the gesture handlers to stop propagation when this instance claims the gesture. Specifically, in `#handleTouch` (and mirror the change in `#handleWheel`) after determining this foldview will handle vertical scrolling (when this.#scrollingVertically === true) call event.stopImmediatePropagation() (and/or event.stopPropagation()) before event.preventDefault() and before calling `#handleScrollDelta`; also ensure the same nested-gesture protection is applied wherever `#touchStart` is gated so ancestor handlers cannot run for the same gesture (add the same stopPropagation logic to the touchstart/wheel entry points).
🧹 Nitpick comments (3)
packages/web-platform/web-elements/tests/web-elements.spec.ts (1)
868-868: Consider adding a comment explaining why the WebKit skip was removed.The commented-out skip suggests a previous issue was resolved. A brief comment explaining this (e.g., "transform-based positioning resolves previous z-index issues") would help future maintainers understand why this test now runs on Safari.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-elements/tests/web-elements.spec.ts` at line 868, Add a short inline comment above or beside the previously-commented test.skip(browserName === 'webkit', 'z-index issues for safari'); in web-elements.spec.ts explaining why the WebKit skip was removed (for example: "Removed skip because transform-based positioning resolves previous Safari z-index issues; re-enabled after fix #<issue-or-PR>"). Reference the exact test.skip snippet when adding the comment and include either the resolving PR/issue number or a brief technical note so future maintainers know why the test now runs on Safari.packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts (1)
30-44: Consider a single helper for--foldview-scroll-heightupdates.Both methods duplicate the same style write. A shared helper reduces drift and keeps updates centralized.
♻️ Proposed refactor
+ `#syncScrollHeightStyle`() { + this.style.setProperty('--foldview-scroll-height', this.scrollHeight + 'px'); + } + [updateHeaderHeight](height: number) { this.#headerHeight = height; - this.style.setProperty( - '--foldview-scroll-height', - this.scrollHeight + 'px', - ); + this.#syncScrollHeightStyle(); } @@ [updateToolbarHeight](height: number) { this.#toolbarHeight = height; - this.style.setProperty( - '--foldview-scroll-height', - this.scrollHeight + 'px', - ); + this.#syncScrollHeightStyle(); }As per coding guidelines, "Group DOM updates for efficiency".
🤖 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/XFoldViewNg/XFoldviewNg.ts` around lines 30 - 44, The two methods updateHeaderHeight and updateToolbarHeight both set the same CSS variable; extract the duplicated this.style.setProperty('--foldview-scroll-height', this.scrollHeight + 'px') into a single private helper (e.g., updateFoldviewScrollHeight or setFoldviewScrollHeight) and call that helper from both updateHeaderHeight and updateToolbarHeight after updating their respective `#headerHeight/`#toolbarHeight fields so DOM updates are centralized and easier to maintain.packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewHeaderNg.ts (1)
22-29: Defer height propagation to a microtask to reduce sync layout churn.Line 28 reads layout (
clientHeight) and immediately triggers upstream style updates on connect/resize. Queueing the propagation avoids doing this work in the same synchronous turn.♻️ Proposed refactor
-import { LinearContainer } from '../../compat/index.js'; +import { boostedQueueMicrotask, LinearContainer } from '../../compat/index.js'; connectedCallback() { this.#resizeObserver = new ResizeObserver((entries) => { for (const entry of entries) { - this.#updateParent(entry.contentRect.height); + boostedQueueMicrotask(() => { + this.#updateParent(entry.contentRect.height); + }); } }); this.#resizeObserver.observe(this); - this.#updateParent(this.clientHeight); + boostedQueueMicrotask(() => { + this.#updateParent(this.clientHeight); + }); }As per coding guidelines, "Use boostedQueueMicrotask for DOM reads/writes to avoid synchronous layout thrashing".
🤖 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/XFoldViewNg/XFoldviewHeaderNg.ts` around lines 22 - 29, The header currently reads layout (this.clientHeight) and calls this.#updateParent synchronously in the ResizeObserver and on connect; change both sites to defer propagation via boostedQueueMicrotask so reads/writes run in a microtask: inside the ResizeObserver callback schedule boostedQueueMicrotask(() => this.#updateParent(entry.contentRect.height)) for each entry, and replace the immediate this.#updateParent(this.clientHeight) call with boostedQueueMicrotask(() => this.#updateParent(this.clientHeight)); keep using the existing `#resizeObserver` and `#updateParent` symbols.
🤖 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/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 46-48: The current scroll clamping logic can leave this.scrollTop
negative when scrollHeight < 0; update the clamping code that uses the
scrollHeight getter (override get scrollHeight()) and the code that assigns/sets
scrollTop so it first computes an effectiveScrollHeight = Math.max(0,
this.scrollHeight) (or otherwise guard for negative values) before applying the
if/else-if bounds checks and final assignment to this.scrollTop, ensuring you
never assign a negative value to scrollTop and that both lower and upper bounds
use the non-negative effectiveScrollHeight.
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts`:
- Around line 71-74: The touch handling mixes page and client coordinates
causing wrong deltas and elementsFromPoint hits; update
XFoldviewNgTouchEventsHandler to consistently use viewport (client) coordinates:
stop storing pageX/pageY in `#previousPageX/`#previousPageY (or add matching
`#previousClientX/`##previousClientY), compute/assign previous values from
touch.clientX/clientY (or convert stored page values to client by subtracting
window.scrollX/window.scrollY), and pass those client coordinates into
document.elementsFromPoint() and into the delta math in the handlers
(references: properties `#previousPageX/`#previousPageY, the delta calculation
block using clientX/clientY, and the code around elementsFromPoint() where page
coordinates are passed).
In `@packages/web-platform/web-elements/tests/web-elements.spec.ts`:
- Around line 859-862: The Playwright assertion call using
page.locator('x-foldview-slot-ng').toHaveCSS is async but not awaited; update
the test to await the assertion (i.e., prepend await to
expect(page.locator('x-foldview-slot-ng')).toHaveCSS(...)) and ensure the
surrounding test function is declared async so the await is valid.
In `@packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts`:
- Line 63: The variable scrollview (created via page.locator('#inner-scroll'))
is declared in the second test but never used; either remove that unused
declaration or actually use the locator in the test (for example add an
assertion or interaction like checking visibility/scroll) so the locator is
referenced. Update the second test to eliminate the unused const scrollview or
replace it with a meaningful use of the page.locator('#inner-scroll') locator to
satisfy the test expectations.
- Line 27: The test declares a locator variable scrollview via
page.locator('#inner-scroll') that is never used; either remove the unused
declaration or replace its usage where intended. Locate the scrollview
declaration in the x-foldview-ng-wheel.spec.ts test (the const scrollview =
page.locator('#inner-scroll') line) and either delete that line or use the
scrollview variable in assertions or interactions (e.g., awaiting scrollview
methods or checking visibility) so the test no longer contains an unused
variable.
---
Outside diff comments:
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts`:
- Around line 65-85: Nested foldviews still receive touchmove/wheel and mutate
shared gesture state; update the gesture handlers to stop propagation when this
instance claims the gesture. Specifically, in `#handleTouch` (and mirror the
change in `#handleWheel`) after determining this foldview will handle vertical
scrolling (when this.#scrollingVertically === true) call
event.stopImmediatePropagation() (and/or event.stopPropagation()) before
event.preventDefault() and before calling `#handleScrollDelta`; also ensure the
same nested-gesture protection is applied wherever `#touchStart` is gated so
ancestor handlers cannot run for the same gesture (add the same stopPropagation
logic to the touchstart/wheel entry points).
---
Nitpick comments:
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewHeaderNg.ts`:
- Around line 22-29: The header currently reads layout (this.clientHeight) and
calls this.#updateParent synchronously in the ResizeObserver and on connect;
change both sites to defer propagation via boostedQueueMicrotask so reads/writes
run in a microtask: inside the ResizeObserver callback schedule
boostedQueueMicrotask(() => this.#updateParent(entry.contentRect.height)) for
each entry, and replace the immediate this.#updateParent(this.clientHeight) call
with boostedQueueMicrotask(() => this.#updateParent(this.clientHeight)); keep
using the existing `#resizeObserver` and `#updateParent` symbols.
In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 30-44: The two methods updateHeaderHeight and updateToolbarHeight
both set the same CSS variable; extract the duplicated
this.style.setProperty('--foldview-scroll-height', this.scrollHeight + 'px')
into a single private helper (e.g., updateFoldviewScrollHeight or
setFoldviewScrollHeight) and call that helper from both updateHeaderHeight and
updateToolbarHeight after updating their respective `#headerHeight/`#toolbarHeight
fields so DOM updates are centralized and easier to maintain.
In `@packages/web-platform/web-elements/tests/web-elements.spec.ts`:
- Line 868: Add a short inline comment above or beside the previously-commented
test.skip(browserName === 'webkit', 'z-index issues for safari'); in
web-elements.spec.ts explaining why the WebKit skip was removed (for example:
"Removed skip because transform-based positioning resolves previous Safari
z-index issues; re-enabled after fix #<issue-or-PR>"). Reference the exact
test.skip snippet when adding the comment and include either the resolving
PR/issue number or a brief technical note so future maintainers know why the
test now runs on Safari.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 033262e3-922d-42bb-98cf-8860a83e11e1
⛔ Files ignored due to path filters (4)
packages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-chromium-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-firefox-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-webkit-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/size-parent-grow-children-specific/300px-inf-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.changeset/slimy-cows-skip.mdpackages/web-platform/web-elements/rsbuild.config.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewHeaderNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgEvents.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewToolbarNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/x-foldview-ng.csspackages/web-platform/web-elements/tests/fixtures/x-foldview-ng/item-fixed.htmlpackages/web-platform/web-elements/tests/web-elements.spec.tspackages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts
💤 Files with no reviewable changes (1)
- packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNg.ts
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts
Show resolved
Hide resolved
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+859
to
+862
| expect(page.locator('x-foldview-slot-ng')).toHaveCSS( | ||
| 'transform', | ||
| 'matrix(1, 0, 0, 1, 0, 200)', | ||
| ); |
Contributor
There was a problem hiding this comment.
Missing await on the expect() assertion.
The Playwright toHaveCSS() assertion is async and should be awaited.
🔧 Proposed fix
- expect(page.locator('x-foldview-slot-ng')).toHaveCSS(
+ await expect(page.locator('x-foldview-slot-ng')).toHaveCSS(
'transform',
'matrix(1, 0, 0, 1, 0, 200)',
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(page.locator('x-foldview-slot-ng')).toHaveCSS( | |
| 'transform', | |
| 'matrix(1, 0, 0, 1, 0, 200)', | |
| ); | |
| await expect(page.locator('x-foldview-slot-ng')).toHaveCSS( | |
| 'transform', | |
| 'matrix(1, 0, 0, 1, 0, 200)', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-platform/web-elements/tests/web-elements.spec.ts` around lines
859 - 862, The Playwright assertion call using
page.locator('x-foldview-slot-ng').toHaveCSS is async but not awaited; update
the test to await the assertion (i.e., prepend await to
expect(page.locator('x-foldview-slot-ng')).toHaveCSS(...)) and ensure the
surrounding test function is declared async so the await is valid.
| await goto(page, title); | ||
| const foldview = page.locator('#foldview'); | ||
| await wait(200); | ||
| const scrollview = page.locator('#inner-scroll'); |
Contributor
There was a problem hiding this comment.
Unused variable scrollview in first test.
The scrollview locator is declared but never used.
🧹 Proposed fix
await wait(200);
- const scrollview = page.locator('#inner-scroll');
await page.locator('#inner-scroll').hover({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const scrollview = page.locator('#inner-scroll'); | |
| await wait(200); | |
| await page.locator('#inner-scroll').hover({ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts` at line
27, The test declares a locator variable scrollview via
page.locator('#inner-scroll') that is never used; either remove the unused
declaration or replace its usage where intended. Locate the scrollview
declaration in the x-foldview-ng-wheel.spec.ts test (the const scrollview =
page.locator('#inner-scroll') line) and either delete that line or use the
scrollview variable in assertions or interactions (e.g., awaiting scrollview
methods or checking visibility) so the test no longer contains an unused
variable.
| await goto(page, title); | ||
| const foldview = page.locator('#foldview'); | ||
| await wait(200); | ||
| const scrollview = page.locator('#inner-scroll'); |
Contributor
There was a problem hiding this comment.
Unused variable scrollview in second test.
Same issue as the first test - the scrollview locator is declared but never used.
🧹 Proposed fix
await wait(200);
- const scrollview = page.locator('#inner-scroll');
await page.locator('#inner-scroll').hover({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const scrollview = page.locator('#inner-scroll'); | |
| await wait(200); | |
| await page.locator('#inner-scroll').hover({ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts` at line
63, The variable scrollview (created via page.locator('#inner-scroll')) is
declared in the second test but never used; either remove that unused
declaration or actually use the locator in the test (for example add an
assertion or interaction like checking visibility/scroll) so the locator is
referenced. Update the second test to eliminate the unused const scrollview or
replace it with a meaningful use of the page.locator('#inner-scroll') locator to
satisfy the test expectations.
Merging this PR will degrade performance by 6.56%
Performance Changes
Comparing 
Footnotes
|
Web Explorer#8018 Bundle Size — 385.17KiB (+0.17%).bf134ef(current) vs 0d41253 main#8009(baseline) Bundle metrics
|
| Current #8018 |
Baseline #8009 |
|
|---|---|---|
 Initial JS |
156.21KiB(+0.4%) |
155.59KiB |
| Initial CSS |
35.15KiB(+0.15%) |
35.1KiB |
 Cache Invalidation |
49.59% |
40.42% |
 Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
237(-0.42%) |
238 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
2.98% |
2.98% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 2 changes 2 regressions
| Current #8018 |
Baseline #8009 |
|
|---|---|---|
| JS |
254.17KiB (+0.24%) |
253.55KiB |
| Other |
95.85KiB |
95.85KiB |
| CSS |
35.15KiB (+0.15%) |
35.1KiB |
Bundle analysis report Branch PupilTong:p/hw/foldview-use-tran... Project dashboard
Generated by RelativeCI Documentation Report issue
PupilTong
force-pushed
the
p/hw/foldview-use-transform
branch
from
0d284d5 to
4d893df
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts (1)
20-97:⚠️ Potential issue | 🟡 MinorAdd a wheel test for
scroll-enable="false"behavior.This suite covers wheel flow well, but it doesn’t assert the new attribute gate introduced in the handler (
scroll-enable="false"should block scroll state changes).As per coding guidelines: "Ensure tests cover attribute changes, style updates, and event handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts` around lines 20 - 97, Add a new test in x-foldview-ng-wheel.spec.ts (e.g. 'x-foldview-ng/wheel-scroll-disabled') that sets the foldview attribute scroll-enable="false" on the `#foldview` element (using page.evaluate), then performs the same wheel interaction sequence (hover `#inner-scroll`, page.mouse.wheel) and asserts that neither (document.querySelector('#foldview') as HTMLElement).scrollTop nor (document.querySelector('#inner-scroll') as HTMLElement).scrollTop change from their initial values; also include the webkit skip and a follow-up assertion that toggling scroll-enable to "true" allows scrolling to continue (so test both blocked and re-enabled behavior). Use the existing helpers and selectors (`#foldview`, `#inner-scroll`), waits, and expectation style to match the suite.
♻️ Duplicate comments (2)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts (1)
71-75:⚠️ Potential issue | 🟠 MajorUse one coordinate space (client) for touch delta + hit-testing.
Line 119 captures
pageX/pageY, but Line 72 usesclientYagainst that stored value, and Line 130 passes page coordinates intoelementsFromPoint. This breaks scroll delta and target detection when the page is scrolled.🐛 Proposed fix
- const { pageX, pageY } = event.touches.item(0)!; + const { clientX, clientY } = event.touches.item(0)!; @@ - this.#elements = document.elementsFromPoint(pageX, pageY).filter(e => + this.#elements = document.elementsFromPoint(clientX, clientY).filter(e => this.#dom.contains(e) && this.#dom !== e ); - this.#previousPageY = pageY; - this.#previousPageX = pageX; + this.#previousPageY = clientY; + this.#previousPageX = clientX;#!/bin/bash # Verify coordinate usage consistency in touch handler (read-only). rg -n "elementsFromPoint|pageX|pageY|clientX|clientY|#previousPageX|#previousPageY" \ packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts -C 2Also applies to: 119-134
🤖 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/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts` around lines 71 - 75, Touch handler mixes page and client coordinates which breaks deltas and hit-testing when the page is scrolled; update the handler to use a single coordinate space (client) everywhere. Replace uses of `#previousPageX` and `#previousPageY` with client-based storage (or add `#previousClientX/`#previousClientY), set them from touch.clientX/clientY in the touch-start method, compute deltas using clientX/clientY in the touch-move logic (the block around deltaY, deltaX and `#scrollingVertically`), and pass clientX/clientY into elementsFromPoint (instead of page coords) so hit-testing and scrolling detection are consistent. Ensure all references in XFoldviewNgTouchEventsHandler (touch start/move methods and any helper that reads previous coords) are updated accordingly.packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts (1)
46-48:⚠️ Potential issue | 🟠 MajorClamp against a non-negative max scroll range.
scrollHeightcan go negative (headerHeight < toolbarHeight). With currentif/else if, Line 65-67 can assign a negativescrollTop, which then propagates invalid CSS transform state.🐛 Proposed fix
override get scrollHeight(): number { - return this.#headerHeight - this.#toolbarHeight; + return Math.max(0, this.#headerHeight - this.#toolbarHeight); } @@ override set scrollTop(value: number) { - if (value > this.scrollHeight) { - value = this.scrollHeight; - } else if (value < 0) { + const maxScrollTop = this.scrollHeight; + if (value > maxScrollTop) { + value = maxScrollTop; + } + if (value < 0) { value = 0; }Also applies to: 64-69
🤖 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/XFoldViewNg/XFoldviewNg.ts` around lines 46 - 48, The scrollHeight getter can return negative when `#headerHeight` < `#toolbarHeight`, causing negative scrollTop and invalid CSS transforms; update the logic in the scroll handling (referencing the scrollHeight getter and the code that assigns scrollTop around lines handling scrollTop/computed max — currently using this.scrollHeight) to clamp the maximum scroll range to a non-negative value (e.g., compute const maxScroll = Math.max(0, this.scrollHeight) and then constrain scrollTop to between 0 and maxScroll) so scrollTop never becomes negative and CSS transforms remain valid.
🤖 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/web-platform/web-elements/rsbuild.config.ts`:
- Line 14: Add a short inline comment above the overrideBrowserslist setting in
rsbuild.config.ts explaining why it uses 'last 1 Chrome versions' (e.g., package
manages its own compatibility via the compat module like LinearContainer,
polyfill: 'off' is intentional, and this package intentionally targets a
narrower Chrome range unlike other packages). Reference the overrideBrowserslist
property and mention the compat module name (LinearContainer) and polyfill:
'off' so future maintainers understand the rationale.
---
Outside diff comments:
In `@packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts`:
- Around line 20-97: Add a new test in x-foldview-ng-wheel.spec.ts (e.g.
'x-foldview-ng/wheel-scroll-disabled') that sets the foldview attribute
scroll-enable="false" on the `#foldview` element (using page.evaluate), then
performs the same wheel interaction sequence (hover `#inner-scroll`,
page.mouse.wheel) and asserts that neither (document.querySelector('#foldview')
as HTMLElement).scrollTop nor (document.querySelector('#inner-scroll') as
HTMLElement).scrollTop change from their initial values; also include the webkit
skip and a follow-up assertion that toggling scroll-enable to "true" allows
scrolling to continue (so test both blocked and re-enabled behavior). Use the
existing helpers and selectors (`#foldview`, `#inner-scroll`), waits, and
expectation style to match the suite.
---
Duplicate comments:
In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 46-48: The scrollHeight getter can return negative when
`#headerHeight` < `#toolbarHeight`, causing negative scrollTop and invalid CSS
transforms; update the logic in the scroll handling (referencing the
scrollHeight getter and the code that assigns scrollTop around lines handling
scrollTop/computed max — currently using this.scrollHeight) to clamp the maximum
scroll range to a non-negative value (e.g., compute const maxScroll =
Math.max(0, this.scrollHeight) and then constrain scrollTop to between 0 and
maxScroll) so scrollTop never becomes negative and CSS transforms remain valid.
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts`:
- Around line 71-75: Touch handler mixes page and client coordinates which
breaks deltas and hit-testing when the page is scrolled; update the handler to
use a single coordinate space (client) everywhere. Replace uses of
`#previousPageX` and `#previousPageY` with client-based storage (or add
`#previousClientX/`#previousClientY), set them from touch.clientX/clientY in the
touch-start method, compute deltas using clientX/clientY in the touch-move logic
(the block around deltaY, deltaX and `#scrollingVertically`), and pass
clientX/clientY into elementsFromPoint (instead of page coords) so hit-testing
and scrolling detection are consistent. Ensure all references in
XFoldviewNgTouchEventsHandler (touch start/move methods and any helper that
reads previous coords) are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f53a4792-b8fa-4675-ae00-f36e4a5d2c9c
⛔ Files ignored due to path filters (4)
packages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-chromium-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-firefox-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-webkit-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/size-parent-grow-children-specific/300px-inf-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.changeset/slimy-cows-skip.mdpackages/web-platform/web-elements/rsbuild.config.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewHeaderNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgEvents.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewToolbarNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/x-foldview-ng.csspackages/web-platform/web-elements/tests/fixtures/x-foldview-ng/item-fixed.htmlpackages/web-platform/web-elements/tests/web-elements.spec.tspackages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts
💤 Files with no reviewable changes (1)
- packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNg.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgEvents.ts
- packages/web-platform/web-elements/tests/web-elements.spec.ts
- packages/web-platform/web-elements/tests/fixtures/x-foldview-ng/item-fixed.html
- .changeset/slimy-cows-skip.md
| output: { | ||
| assetPrefix: 'auto', | ||
| polyfill: 'off', | ||
| overrideBrowserslist: ['last 1 Chrome versions'], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Browser targets across web-platform rsbuild configs =="
rg -n "overrideBrowserslist" packages/web-platform -g "rsbuild.config.ts"
echo
echo "== Polyfill settings in same configs =="
rg -n "polyfill\s*:" packages/web-platform -g "rsbuild.config.ts"
echo
echo "== Browser support policy docs =="
fd -HI "AGENTS.md|README.md|browserslist|.browserslistrc" packages/web-platformRepository: lynx-family/lynx-stack
Length of output: 1496
🏁 Script executed:
cat packages/web-platform/web-elements/AGENTS.mdRepository: lynx-family/lynx-stack
Length of output: 9923
🏁 Script executed:
head -50 packages/web-platform/web-elements/README.mdRepository: lynx-family/lynx-stack
Length of output: 1461
Consider documenting the rationale for 'last 1 Chrome versions' in a comment.
The narrower browser target is intentional—web-elements has a documented compat module (e.g., LinearContainer) with polyfills for missing features. Setting polyfill: 'off' is consistent across web-platform packages and allows the package to manage its own compatibility layer. However, adding a comment in rsbuild.config.ts explaining why this target was chosen would improve clarity for future maintenance, especially since it's narrower than other web-platform packages (e.g., 'last 2 Chrome versions' in web-rsbuild-server-middleware).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-platform/web-elements/rsbuild.config.ts` at line 14, Add a short
inline comment above the overrideBrowserslist setting in rsbuild.config.ts
explaining why it uses 'last 1 Chrome versions' (e.g., package manages its own
compatibility via the compat module like LinearContainer, polyfill: 'off' is
intentional, and this package intentionally targets a narrower Chrome range
unlike other packages). Reference the overrideBrowserslist property and mention
the compat module name (LinearContainer) and polyfill: 'off' so future
maintainers understand the rationale.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts (1)
101-104:⚠️ Potential issue | 🟡 MinorExclude the root foldview from wheel hit-test elements for consistent routing.
Wheel handling includes
this.#dominpointElements(line 102), but excludes it frompathElements(line 98), creating an asymmetry. Meanwhile, touch handling excludes it from both (line 131). This can inadvertently treat the root element as a scrollable child.🐛 Fix
const pointElements = document.elementsFromPoint(clientX, clientY).filter( - e => this.#dom.contains(e), + e => this.#dom.contains(e) && e !== this.#dom, );🤖 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/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts` around lines 101 - 104, The wheel hit-test collects pointElements with document.elementsFromPoint(clientX, clientY) but currently includes the root element this.#dom, causing asymmetric behavior versus pathElements; update the pointElements filtering in XFoldviewNgTouchEventsHandler (the code that defines pointElements and sets this.#elements) to exclude this.#dom (same as pathElements and touch handling) so that this.#elements = [...new Set([...pathElements, ...pointElements])] never contains the root foldview element and wheel routing remains consistent.
♻️ Duplicate comments (1)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts (1)
46-56:⚠️ Potential issue | 🟠 MajorClamp
scrollHeightat the source to prevent negative layout state.Line 47 can produce negative values, which then flow into Line 33/42 (
--foldview-scroll-height) and Line 55 (scrollableLength). That can publish invalid scroll metrics.🐛 Proposed fix
override get scrollHeight(): number { - return this.#headerHeight - this.#toolbarHeight; + return Math.max(0, this.#headerHeight - this.#toolbarHeight); }🤖 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/XFoldViewNg/XFoldviewNg.ts` around lines 46 - 56, The scrollHeight getter can return negative values which then pollute CSS var --foldview-scroll-height and scrollableLength; change the override get scrollHeight() to clamp the computed value to a minimum of 0 (e.g., use Math.max(0, this.#headerHeight - this.#toolbarHeight)) and ensure scrollableLength returns this same clamped value so consumers (and get [isHeaderShowing]) never receive negative metrics; update references in scrollableLength to use the clamped scrollHeight value rather than recalculating.
🧹 Nitpick comments (1)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts (1)
64-79: Consider batching scroll-driven DOM writes with microtask coalescing.
style.setPropertyon every scroll mutation (Line 71-74) is in a hot path. Coalescing updates would reduce DOM churn under touch/wheel bursts.As per coding guidelines: "Use
boostedQueueMicrotaskfor DOM reads/writes to avoid synchronous layout thrashing" and "Group DOM updates for efficiency".🤖 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/XFoldViewNg/XFoldviewNg.ts` around lines 64 - 79, The scrollTop setter currently writes to style (--foldview-scroll-top) synchronously on every assignment; change it to coalesce DOM writes using boostedQueueMicrotask: keep the immediate clamp/assign to this.#scrollTop but defer the style.setProperty, dispatchEvent('scroll') and invoking callbacks (this[scrollCallbacks]) into a single boostedQueueMicrotask callback so rapid successive sets only perform one DOM write and event/callback flush; update references in the setter (scrollTop, `#scrollTop`, style.setProperty, scrollCallbacks) to schedule the microtask when not already queued.
🤖 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/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 83-97: The scrollTo implementation can throw when arg1 is null and
can set NaN into the scrollTop setter; update scrollTo and scrollBy to first
guard that object arguments are non-null (arg1 !== null) and that extracted
numeric values (e.g., top, left, deltaY) are finite via Number.isFinite before
assigning to this.scrollTop/this.scrollLeft; additionally ensure the branch that
handles (typeof arg2 === 'number') also validates Number.isFinite(arg2) before
using it. Locate and modify the override scrollTo method and the scrollBy method
to perform these null and finite checks and skip/ignore invalid values so the
scrollTop setter never receives NaN or undefined.
---
Outside diff comments:
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts`:
- Around line 101-104: The wheel hit-test collects pointElements with
document.elementsFromPoint(clientX, clientY) but currently includes the root
element this.#dom, causing asymmetric behavior versus pathElements; update the
pointElements filtering in XFoldviewNgTouchEventsHandler (the code that defines
pointElements and sets this.#elements) to exclude this.#dom (same as
pathElements and touch handling) so that this.#elements = [...new
Set([...pathElements, ...pointElements])] never contains the root foldview
element and wheel routing remains consistent.
---
Duplicate comments:
In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 46-56: The scrollHeight getter can return negative values which
then pollute CSS var --foldview-scroll-height and scrollableLength; change the
override get scrollHeight() to clamp the computed value to a minimum of 0 (e.g.,
use Math.max(0, this.#headerHeight - this.#toolbarHeight)) and ensure
scrollableLength returns this same clamped value so consumers (and get
[isHeaderShowing]) never receive negative metrics; update references in
scrollableLength to use the clamped scrollHeight value rather than
recalculating.
---
Nitpick comments:
In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 64-79: The scrollTop setter currently writes to style
(--foldview-scroll-top) synchronously on every assignment; change it to coalesce
DOM writes using boostedQueueMicrotask: keep the immediate clamp/assign to
this.#scrollTop but defer the style.setProperty, dispatchEvent('scroll') and
invoking callbacks (this[scrollCallbacks]) into a single boostedQueueMicrotask
callback so rapid successive sets only perform one DOM write and event/callback
flush; update references in the setter (scrollTop, `#scrollTop`,
style.setProperty, scrollCallbacks) to schedule the microtask when not already
queued.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0160de6-eaa5-4592-9890-ffbd29eac44f
📒 Files selected for processing (2)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.ts
Comment on lines
+83
to
+97
| override scrollTo(arg1?: any, arg2?: any): void { | ||
| if (typeof arg1 === 'object') { | ||
| const { top, behavior } = arg1; | ||
| if (typeof top === 'number') { | ||
| if (behavior === 'smooth') { | ||
| // TODO: implement smooth scroll if needed, for now just instant | ||
| this.scrollTop = top; | ||
| } else { | ||
| this.scrollTop = top; | ||
| } | ||
| } | ||
| } else if (typeof arg2 === 'number') { | ||
| this.scrollTop = arg2; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the XFoldViewNg file
fd "XFoldviewNg.ts" packages/web-platform/web-elements/src/Repository: lynx-family/lynx-stack
Length of output: 140
🏁 Script executed:
# Get the file and see the full context around lines 73 and 83-97
cat -n packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts | head -120 | tail -60Repository: lynx-family/lynx-stack
Length of output: 2224
🏁 Script executed:
# Also check the entire file structure to understand the class
wc -l packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.tsRepository: lynx-family/lynx-stack
Length of output: 144
🏁 Script executed:
# Check the scrollBy method to see if there's a pattern
cat -n packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts | sed -n '99,111p'Repository: lynx-family/lynx-stack
Length of output: 579
🏁 Script executed:
# Look for similar scrollTo implementations in other element files
fd "\.ts$" packages/web-platform/web-elements/src/elements/ | xargs grep -l "scrollTo" | head -5Repository: lynx-family/lynx-stack
Length of output: 441
🏁 Script executed:
# Test what happens with NaN/Infinity in the clamping logic using node
node <<'EOF'
// Test Math.max/Math.min with NaN and Infinity
const maxScroll = 100;
// NaN case
let valueNaN = NaN;
valueNaN = Math.max(0, Math.min(valueNaN, maxScroll));
console.log('NaN result:', valueNaN, 'toString:', valueNaN.toString() + 'px');
// Infinity case
let valueInf = Infinity;
valueInf = Math.max(0, Math.min(valueInf, maxScroll));
console.log('Infinity result:', valueInf, 'toString:', valueInf.toString() + 'px');
// Test null destructuring
try {
const { top } = null;
console.log('null destructure: no error');
} catch(e) {
console.log('null destructure error:', e.message);
}
EOFRepository: lynx-family/lynx-stack
Length of output: 217
🏁 Script executed:
# Let's verify the exact clamping behavior with NaN more carefully
node <<'EOF'
const maxScroll = 100;
// Detailed NaN test
let value = NaN;
console.log('Initial:', value);
console.log('After Math.min(value, maxScroll):', Math.min(value, maxScroll));
console.log('After Math.max(0, Math.min(value, maxScroll)):', Math.max(0, Math.min(value, maxScroll)));
// Verify Number.isFinite
console.log('Number.isFinite(NaN):', Number.isFinite(NaN));
console.log('Number.isFinite(Infinity):', Number.isFinite(Infinity));
console.log('Number.isFinite(100):', Number.isFinite(100));
EOFRepository: lynx-family/lynx-stack
Length of output: 255
🏁 Script executed:
# Check if scrollBy has the same issue and if it's also affected
cat -n packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts | sed -n '101,111p'Repository: lynx-family/lynx-stack
Length of output: 462
Add guards against null and non-finite values in scrollTo and scrollBy.
Line 84 accepts null because typeof null === 'object', causing a destructuring error at line 85. Additionally, NaN passes through the clamping logic in the scrollTop setter (line 66) and produces invalid CSS: "NaNpx". The scrollBy method has the same vulnerabilities.
🐛 Proposed fix
override scrollTo(arg1?: any, arg2?: any): void {
- if (typeof arg1 === 'object') {
+ if (typeof arg1 === 'object' && arg1 !== null) {
const { top, behavior } = arg1;
- if (typeof top === 'number') {
+ if (typeof top === 'number' && Number.isFinite(top)) {
if (behavior === 'smooth') {
// TODO: implement smooth scroll if needed, for now just instant
this.scrollTop = top;
} else {
this.scrollTop = top;
}
}
- } else if (typeof arg2 === 'number') {
+ } else if (typeof arg2 === 'number' && Number.isFinite(arg2)) {
this.scrollTop = arg2;
}
}
override scrollBy(arg1?: any, arg2?: any): void {
- if (typeof arg1 === 'object') {
+ if (typeof arg1 === 'object' && arg1 !== null) {
const { top, behavior } = arg1;
this.scrollTo({
top: (typeof top === 'number' ? top : 0) + this.scrollTop,
behavior,
});
} else {
this.scrollTo(0, this.scrollTop + (arg2 || 0));
}
}🤖 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/XFoldViewNg/XFoldviewNg.ts`
around lines 83 - 97, The scrollTo implementation can throw when arg1 is null
and can set NaN into the scrollTop setter; update scrollTo and scrollBy to first
guard that object arguments are non-null (arg1 !== null) and that extracted
numeric values (e.g., top, left, deltaY) are finite via Number.isFinite before
assigning to this.scrollTop/this.scrollLeft; additionally ensure the branch that
handles (typeof arg2 === 'number') also validates Number.isFinite(arg2) before
using it. Locate and modify the override scrollTo method and the scrollBy method
to perform these null and finite checks and skip/ignore invalid values so the
scrollTop setter never receives NaN or undefined.
…om scroll handling, updating related events, styles, and tests.
…dling to use clientX/Y coordinates.
PupilTong
force-pushed
the
p/hw/foldview-use-transform
branch
from
2e55f3e to
bf134ef
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts (2)
83-97:⚠️ Potential issue | 🟡 MinorAdd guards against
nulland non-finite values inscrollTo.The
typeof arg1 === 'object'check at line 84 returnstruefornull, causing the destructuring at line 85 to throw. Additionally, non-finite values (NaN,Infinity) pass through and produce invalid CSS ("NaNpx").🛡️ Proposed fix
override scrollTo(arg1?: any, arg2?: any): void { - if (typeof arg1 === 'object') { + if (typeof arg1 === 'object' && arg1 !== null) { const { top, behavior } = arg1; - if (typeof top === 'number') { + if (typeof top === 'number' && Number.isFinite(top)) { if (behavior === 'smooth') { // TODO: implement smooth scroll if needed, for now just instant this.scrollTop = top; } else { this.scrollTop = top; } } - } else if (typeof arg2 === 'number') { + } else if (typeof arg2 === 'number' && Number.isFinite(arg2)) { this.scrollTop = arg2; } }🤖 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/XFoldViewNg/XFoldviewNg.ts` around lines 83 - 97, The scrollTo method can crash because typeof arg1 === 'object' is true for null and it may accept non-finite values; update the override scrollTo(arg1?: any, arg2?: any) to first check arg1 != null && typeof arg1 === 'object' before destructuring, use Number.isFinite(top) to validate the top value before assigning this.scrollTop, and similarly validate arg2 with Number.isFinite(arg2) when using the numeric overload; preserve the existing behavior handling of behavior === 'smooth' but avoid setting this.scrollTop when top/arg2 is non-finite.
101-111:⚠️ Potential issue | 🟡 MinorApply same null guard to
scrollBy.The
scrollBymethod has the same vulnerability withnullobjects.🛡️ Proposed fix
override scrollBy(arg1?: any, arg2?: any): void { - if (typeof arg1 === 'object') { + if (typeof arg1 === 'object' && arg1 !== null) { const { top, behavior } = arg1; this.scrollTo({ top: (typeof top === 'number' ? top : 0) + this.scrollTop, behavior, }); } else { this.scrollTo(0, this.scrollTop + (arg2 || 0)); } }🤖 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/XFoldViewNg/XFoldviewNg.ts` around lines 101 - 111, The scrollBy method treats any value with typeof 'object' (including null) as an options object and will throw when destructuring; update scrollBy to guard against null by checking arg1 !== null && typeof arg1 === 'object' (or use arg1 || {} before destructuring) before reading top/behavior, and keep the fallback numeric branch that calls scrollTo(0, this.scrollTop + (arg2 || 0)); reference the scrollBy method, the scrollTo call, and this.scrollTop when applying the null-safe check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.ts`:
- Around line 83-97: The scrollTo method can crash because typeof arg1 ===
'object' is true for null and it may accept non-finite values; update the
override scrollTo(arg1?: any, arg2?: any) to first check arg1 != null && typeof
arg1 === 'object' before destructuring, use Number.isFinite(top) to validate the
top value before assigning this.scrollTop, and similarly validate arg2 with
Number.isFinite(arg2) when using the numeric overload; preserve the existing
behavior handling of behavior === 'smooth' but avoid setting this.scrollTop when
top/arg2 is non-finite.
- Around line 101-111: The scrollBy method treats any value with typeof 'object'
(including null) as an options object and will throw when destructuring; update
scrollBy to guard against null by checking arg1 !== null && typeof arg1 ===
'object' (or use arg1 || {} before destructuring) before reading top/behavior,
and keep the fallback numeric branch that calls scrollTo(0, this.scrollTop +
(arg2 || 0)); reference the scrollBy method, the scrollTo call, and
this.scrollTop when applying the null-safe check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49530376-1b1e-4053-bc54-374e10f80e21
⛔ Files ignored due to path filters (4)
packages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-chromium-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-firefox-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/basic-with-lynx-wrapper/index-webkit-linux.pngis excluded by!**/*.pngpackages/web-platform/web-elements/tests/web-elements.spec.ts-snapshots/x-foldview-ng/size-parent-grow-children-specific/300px-inf-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.changeset/slimy-cows-skip.mdpackages/web-platform/web-elements/rsbuild.config.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewHeaderNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgEvents.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewNgTouchEventsHandler.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewToolbarNg.tspackages/web-platform/web-elements/src/elements/XFoldViewNg/x-foldview-ng.csspackages/web-platform/web-elements/tests/fixtures/x-foldview-ng/item-fixed.htmlpackages/web-platform/web-elements/tests/web-elements.spec.tspackages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts
💤 Files with no reviewable changes (1)
- packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNg.ts
✅ Files skipped from review due to trivial changes (1)
- packages/web-platform/web-elements/tests/fixtures/x-foldview-ng/item-fixed.html
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/slimy-cows-skip.md
- packages/web-platform/web-elements/src/elements/XFoldViewNg/x-foldview-ng.css
- packages/web-platform/web-elements/tests/web-elements.spec.ts
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
1 participant
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.
this breaks #878
The position:fixed elements in x-foldview-header-ng and x-foldview-slot-ng will be affected.
Summary by CodeRabbit
Scroll & Positioning
Browser Support
Testing
Checklist