fix: paginate listEntries with scroll#3384
Conversation
`ContentService.getPage` was returning only the first page of entries from Content Platform because `listEntries` wasn't following the `scroll` token. Result: pages backed by CP silently missed content beyond the first batch. In `packages/core/src/server/content/service.ts`, the single `listEntries` call is replaced by a loop that accumulates entries while a `scroll` token is returned, then passes the full list to `fillEntriesWithData`. - On a store using the CP data source, create enough entries for a given content type to exceed a single page. - Request that page and verify all entries are returned (not just the first batch). - Confirm non-CP (hCMS) path is unaffected. N/A N/A <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Content retrieval now fully paginates via scrolling, accumulating entries across pages and avoiding truncated results (stops safely with a max-page cap and logs a warning if reached). * **Enhancements** * Product listing (PLP) and product detail (PDP) now attempt a direct single-item fetch first and return immediately on direct matches, improving accuracy and performance. * **Notes** * No changes to public APIs or exported entities. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/vtex/faststore/pull/3282?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
vitorflg
requested review from
ommeirelles and
renatomaurovtex
and removed request for
a team
WalkthroughContentService content retrieval is enhanced with a client library upgrade, scroll-based pagination capped at 25 pages for multi-entry fetches, and optimized direct-match fast paths in PLP and PDP methods that fall back to template selection when no direct match is found. ChangesContent Fetching Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Cherry-pick into 3.x. The pagination loop is solid and the getSingleContent short-circuit is well structured — getFromCP returns undefined on not-found (404 caught at service.ts:134), so the if (directMatch) fallback to getMultipleContent works as intended. A few things to confirm before merge.
packages/core/package.json / pnpm-lock.yaml
🟠 [deps] @vtex/client-cp 0.3.5 → 0.6.0 in a published package
This jumps three minor versions on a 0.x package, where minors can be breaking. The new code relies on the listEntries(params, scroll) 2nd arg and result.scroll (new surface). It also keeps calling getEntry/getEntryBySlug/previewEntry* — please confirm those signatures didn't change across 0.4–0.6 and that 0.6.0 is the intended published version. Lockfile is updated, good. Since this is a runtime dep on @faststore/core, a maintainer sign-off on the bump is worth noting in the PR body.
packages/core/src/server/content/service.ts
🟠 [behavior] directMatch now short-circuits the multi-template selection
const directMatch = await this.getSingleContent<PLPContentType>(plpParams)
if (directMatch) {
return directMatch
}Before this PR, PLP/PDP always went through getMultipleContent → findBestPLPTemplate(pages, slug, rewrites) / findBestPDPTemplate(pages, product). Now an exact-slug entry returned by getEntryBySlug wins and the rewrites/best-template logic is skipped entirely. For most stores "exact slug wins" is the desired behavior, but for stores relying on multiple templates + rewrites this is a behavior change. Can you add an explicit test step covering a store that has both an exact-slug entry and rewrite-matched templates, to confirm the exact one is meant to win? (product isn't used in the PDP fast path either — same question.)
💬 [behavior] Silent truncation past MAX_PAGES
if (pages >= MAX_PAGES) {
console.warn('listEntries pagination hit MAX_PAGES cap', serviceParams)
}The infinite-loop guard (next === scroll → break) and the cap are good defensive moves. Note that hitting the cap silently re-introduces the exact bug this PR fixes (missing entries beyond page 25). The TODO makes the temporary intent clear, so this is fine — just flagging that the warn is the only signal. Consider routing it through the diagnostics logger instead of console.warn (cf. the logger work in #3300).
🟡 [convention] Commit type
Title is feat: but the PR is checked as 🐛 Bug fix (and it's a fix cherry-picked into the 3.x maintenance branch). Type drives the release bump, so this should likely be fix(core): rather than feat:.
Verdict: Approved with comments
Blocking (🟠):
- Confirm
@vtex/client-cp0.6.0 is intended and thegetEntry*/previewEntry*calls still match its API across the 0.3→0.6 jump (note the runtime-dep bump for maintainer sign-off). - Confirm the
directMatchshort-circuit is intended to win overfindBestPLPTemplate/findBestPDPTemplate+ rewrites; add a test step for the multi-template case.
Non-blocking (🟡/💬):
feat:→fix(core):to match the bug-fix intent / release bump.MAX_PAGEStruncation is silent; consider the diagnostics logger overconsole.warn.
Checks to confirm before merge: pnpm lint · build · pnpm turbo run test --filter=@faststore/core · pnpm size.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/server/content/service.ts`:
- Around line 67-71: The code passes the entire accumulated allEntries list into
fillEntriesWithData which causes pages × entriesPerPage concurrent
getEntryData() calls; instead, limit concurrency by processing entries in
bounded batches or adding a concurrency limiter. Change the call-site or
fillEntriesWithData signature to accept a batchSize or concurrency option (e.g.,
batchSize/concurrency param) and then iterate over allEntries in chunks (or use
a p-limit style worker pool) so only N getEntryData() calls run in parallel;
reference the allEntries variable, fillEntriesWithData(), getEntryData(), and
serviceParams/options.isPreview when implementing batching or a concurrency
limiter. Ensure the final result order is preserved by collecting batch results
in sequence and returning the same aggregated array.
- Around line 61-65: The loop in listEntries can exit due to hitting MAX_PAGES
while scroll is still truthy, leaving allEntries incomplete and then continuing
to template selection; modify listEntries so that if pages >= MAX_PAGES and
scroll is still set you do not proceed with downstream selection: detect this
condition (pages >= MAX_PAGES && scroll) after the loop and either (a)
abort/return a clear error or truncated-result indicator to the caller (so
template selection is skipped) or (b) explicitly clear/complete the scroll and
fetch remaining pages before proceeding; reference allEntries, scroll,
MAX_PAGES, listEntries and serviceParams when making this change so template
selection code only runs on a complete result set.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f39d2a8-53de-41fa-b4db-c789d786c461
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (2)
packages/core/package.jsonpackages/core/src/server/content/service.ts
| } while (scroll && ++pages < MAX_PAGES) | ||
|
|
||
| if (pages >= MAX_PAGES) { | ||
| console.warn('listEntries pagination hit MAX_PAGES cap', serviceParams) | ||
| } |
There was a problem hiding this comment.
Don't continue with truncated results after the page cap.
If this loop exits with scroll still set, allEntries is incomplete but the code still runs template selection on it. That recreates the original bug for large CP datasets, just at a higher threshold, and can pick the wrong PLP/PDP content.
Suggested change
- if (pages >= MAX_PAGES) {
- console.warn('listEntries pagination hit MAX_PAGES cap', serviceParams)
- }
+ if (scroll) {
+ throw new Error(
+ `listEntries pagination exceeded MAX_PAGES (${MAX_PAGES})`
+ )
+ }📝 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.
| } while (scroll && ++pages < MAX_PAGES) | |
| if (pages >= MAX_PAGES) { | |
| console.warn('listEntries pagination hit MAX_PAGES cap', serviceParams) | |
| } | |
| } while (scroll && ++pages < MAX_PAGES) | |
| if (scroll) { | |
| throw new Error( | |
| `listEntries pagination exceeded MAX_PAGES (${MAX_PAGES})` | |
| ) | |
| } |
🤖 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/core/src/server/content/service.ts` around lines 61 - 65, The loop
in listEntries can exit due to hitting MAX_PAGES while scroll is still truthy,
leaving allEntries incomplete and then continuing to template selection; modify
listEntries so that if pages >= MAX_PAGES and scroll is still set you do not
proceed with downstream selection: detect this condition (pages >= MAX_PAGES &&
scroll) after the loop and either (a) abort/return a clear error or
truncated-result indicator to the caller (so template selection is skipped) or
(b) explicitly clear/complete the scroll and fetch remaining pages before
proceeding; reference allEntries, scroll, MAX_PAGES, listEntries and
serviceParams when making this change so template selection code only runs on a
complete result set.
| return this.fillEntriesWithData( | ||
| allEntries, | ||
| serviceParams, | ||
| options.isPreview | ||
| ) |
There was a problem hiding this comment.
Bound the CP enrichment fan-out before passing all pages downstream.
This now turns a single request into pages × entriesPerPage parallel getEntryData() calls through fillEntriesWithData(). On large catalogs that can blow the request budget or throttle CP hard. Please batch page enrichment or add a concurrency limit instead of sending the whole accumulated list at once.
🤖 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/core/src/server/content/service.ts` around lines 67 - 71, The code
passes the entire accumulated allEntries list into fillEntriesWithData which
causes pages × entriesPerPage concurrent getEntryData() calls; instead, limit
concurrency by processing entries in bounded batches or adding a concurrency
limiter. Change the call-site or fillEntriesWithData signature to accept a
batchSize or concurrency option (e.g., batchSize/concurrency param) and then
iterate over allEntries in chunks (or use a p-limit style worker pool) so only N
getEntryData() calls run in parallel; reference the allEntries variable,
fillEntriesWithData(), getEntryData(), and serviceParams/options.isPreview when
implementing batching or a concurrency limiter. Ensure the final result order is
preserved by collecting batch results in sequence and returning the same
aggregated array.
](https://app.coderabbit.ai/change-stack/vtex/faststore/pull/3282?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
3 participants
What's the purpose of this pull request?
ContentService.getPagewas returning only the first page of entries from Content Platform becauselistEntrieswasn't following thescrolltoken. Pages backed by CP silently missed content beyond the first batch.How it works?
In
packages/core/src/server/content/service.ts, the singlelistEntriescall is replaced by a loop that accumulates entries while ascrolltoken is returned, then passes the full list tofillEntriesWithData.How to test it?
Starters Deploy Preview
N/A
References
Cherry-pick of 2e43804 from
devinto3.x.Type of Change
Summary by CodeRabbit
Chores
Refactor