fix: issue #795#797
Conversation
🦋 Changeset detectedLatest commit: 4e6e6eb 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 |
atk
left a comment
atk
left a comment
There was a problem hiding this comment.
Please let's discuss the API. I dislike the boolean and that the params need to be wrapped in a function even if they are not supposed to change.
|
|
||
| /** | ||
| * Provides an easy way to implement infinite scrolling. | ||
| * A SolidJS utility to create an infinite scrolling experience with IntersectionObserver and reactivity. |
There was a problem hiding this comment.
There is no need to explicitly mention SolidJS here when the whole package is part of solid-primitives.
| * A function that takes a page number and optional parameters and returns a Promise resolving to an array of items. | ||
| * | ||
| * @param {Object} [options] - Optional configuration object. | ||
| * @param {() => P} [options.params] - A reactive accessor for dynamic fetcher parameters. |
There was a problem hiding this comment.
We usually use the pattern that we take a params object or a params object signal. If it is a signal, we expect that we should reset state on params change; otherwise, we don't.
|
@atk I have updated the api design. Please check once |
| export type _E = JSX.Element; | ||
|
|
||
| /** Function-shaped accessor with live .loading / .error properties */ | ||
| export type PagesAccessor<T> = (() => T[]) & { |
There was a problem hiding this comment.
This looks awefully similar to a Resource<T>. I don't think we should re-create Solid's types without a good reason.
There was a problem hiding this comment.
Yeah, that was my thought too, Solid’s types felt like a solid (no pun intended) starting point for this hook. Since the JS docs mentioned patterns like
pages.loading
pages.error
it made sense to align the structure with Solid’s Resource style for familiarity and consistency.
There was a problem hiding this comment.
Then why can't you use the original type?
There was a problem hiding this comment.
I have changed type to use original type now and made some other changes too. Please let me know if you want to discuss the API design any further...
…olidjs-community#795) Addresses review feedback on solidjs-community#797: drop the custom PagesAccessor<T> type and reuse solid-js's Resource<T> shape inline via Pick, so we're not re-creating a type that already exists in solid-js. Attach reactive loading/error getters to the pages accessor as documented. Also removes the dead _E export and the stacked onCleanup handlers in the loader ref-callback.
|
Hi @thetarnav, Could you please take a look when you get a chance? |
|
@atk we good with this? If so I'll merge and port the changes to 2.0 |
📝 WalkthroughWalkthroughThe ChangesInfinite Scroll Rework
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/pagination/src/index.ts (1)
384-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
refetchleaks the internalResp<T>wrapper.The public
refetchreturn type exposes the internal{ page, items }shape rather thanT[], surfacing an implementation detail to consumers. Consider wrapping it to returnT[](or documenting/narrowing the type) so the public API stays in terms ofT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pagination/src/index.ts` at line 384, The public refetch API in the pagination types is exposing the internal Resp<T> wrapper instead of the item list shape consumers should see. Update the refetch return type in the pagination module so it returns T[] (or a similarly narrowed public type) rather than Resp<T>, and make sure the related pagination methods/types stay consistent with that public contract. Use the refetch signature in the pagination index and any accompanying pagination result type definitions to locate and adjust the exposed API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pagination/src/index.ts`:
- Around line 425-443: The auto-pagination logic in the IntersectionObserver
callback can stall because it only advances when a new intersection event fires,
and it can also skip a failed page by incrementing page even after an error.
Update the observer handling around the IntersectionObserver setup, setPage,
end(), and res.loading so the sentinel is re-checked whenever loading or error
state changes, and only advance to the next page when the previous fetch
succeeded. If the current request failed, retry the same page instead of
incrementing.
---
Nitpick comments:
In `@packages/pagination/src/index.ts`:
- Line 384: The public refetch API in the pagination types is exposing the
internal Resp<T> wrapper instead of the item list shape consumers should see.
Update the refetch return type in the pagination module so it returns T[] (or a
similarly narrowed public type) rather than Resp<T>, and make sure the related
pagination methods/types stay consistent with that public contract. Use the
refetch signature in the pagination index and any accompanying pagination result
type definitions to locate and adjust the exposed API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51860ac4-c740-4ef6-ad97-655873207280
📒 Files selected for processing (2)
.changeset/pink-carpets-reply.mdpackages/pagination/src/index.ts
| if (!io) { | ||
| io = new IntersectionObserver( | ||
| entries => { | ||
| if (!entries.some(e => e.isIntersecting)) return; | ||
| if (end() || res.loading) return; | ||
| setPage(p => p + 1); | ||
| }, | ||
| { root: null, rootMargin: "0px 0px 50px 0px", threshold: 0 }, | ||
| ); | ||
| onCleanup(() => { | ||
| io?.disconnect(); | ||
| io = null; | ||
| }); | ||
| } | ||
| if (el) { | ||
| io.observe(el); | ||
| observed = el; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does IntersectionObserver re-invoke its callback when the observed element remains intersecting and no intersection state change occurs?
💡 Result:
No, the IntersectionObserver callback does not re-invoke when an observed element remains intersecting and no intersection state change (crossing a threshold) occurs [1][2][3]. The callback is specifically designed to trigger only when the visibility of the target element crosses one of the observer's defined thresholds [1][2][3]. If the element remains within its current state (i.e., its intersection ratio does not cross any defined threshold), the observer does not fire the callback [1][2][4]. There is one notable exception to this rule: the callback is guaranteed to fire at least once during the first render cycle after the observe method is called, regardless of whether a threshold has been crossed [5]. This initial invocation serves to provide the observer with the target's current intersection status [5]. Following this initial report, subsequent invocations will only occur when a threshold-crossing event takes place [1][2][5].
Citations:
- 1: https://github.com/w3c/IntersectionObserver/blob/main/explainer.md
- 2: https://w3c.github.io/IntersectionObserver/
- 3: https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver/IntersectionObserver
- 4: https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API/Timing_element_visibility
- 5: https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver/observe
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline packages/pagination/src/index.ts --view expanded
# Show the relevant section with line numbers
sed -n '360,520p' packages/pagination/src/index.ts
# Find where `res.loading`, `end()`, and `setPage` are defined/used
rg -n "res\.loading|end\(\)|setPage\(|IntersectionObserver|onCleanup|error" packages/pagination/src/index.tsRepository: solidjs-community/solid-primitives
Length of output: 4979
Auto-pagination can stall on intersecting sentinels and skip failed pages.
- If the sentinel stays intersecting while
res.loadingflips false, no new observer event fires, so short pages can stop loading until the element leaves and re-enters view. - On fetch error, the next intersection advances
pageinstead of retrying the failed request, so the failed page is skipped.
Re-check the sentinel when loading/error state changes, or gate setPage on a successful fetch state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pagination/src/index.ts` around lines 425 - 443, The auto-pagination
logic in the IntersectionObserver callback can stall because it only advances
when a new intersection event fires, and it can also skip a failed page by
incrementing page even after an error. Update the observer handling around the
IntersectionObserver setup, setPage, end(), and res.loading so the sentinel is
re-checked whenever loading or error state changes, and only advance to the next
page when the previous fetch succeeded. If the current request failed, retry the
same page instead of incrementing.
4 participants
This PR fixes the issue: #795
Summary by CodeRabbit
New Features
Bug Fixes