feat[getAll]: ENG-11765 add fetchTotalCount param from Content API as an option for getAll in SDKs#4352
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
View your CI Pipeline Execution ↗ for commit 73b1d7d
☁️ Nx Cloud last updated this comment at |
🦋 Changeset detectedLatest commit: 73b1d7d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
AishwaryaParab
requested review from
a team,
midhunadarvin and
sanyamkamat
and removed request for
a team
| getAll( | ||
| modelName: string, | ||
| options?: GetContentOptions & { | ||
| req?: IncomingMessage; | ||
| res?: ServerResponse; | ||
| apiKey?: string; | ||
| authToken?: string; | ||
| } | ||
| ): Promise<BuilderContent[]>; |
There was a problem hiding this comment.
Is this function overload required as it's similar to the below one ?
There was a problem hiding this comment.
The below one is the implementation of the getAll() function. In total, we have two function overloads. The first one kicks in when fetchTotalCount is passed and totalCount is received. The second one is when fetchTotalCount is not passed and we don't get totalCount back.
The implementation signature is never visible to the callers. So, if we call builder.getAll('blog-post', {limit: 5}) [without the second overload], TS throws an error saying no matching signature found.
| res?: ServerResponse; | ||
| apiKey?: string; | ||
| authToken?: string; | ||
| fetchTotalCount: true; |
There was a problem hiding this comment.
why is this needed here?
There was a problem hiding this comment.
fetchTotalCount is passed as true even after defining it in GetContentOptions because here, we're explicitly saying that the value of fetchTotalCount will be true. If in case it's false or if it's not passed at all, we will directly return BuilderContent[] instead.
Alternatively, a cleaner way of doing this can be:
getAll<T extends boolean = false>(
modelName: string,
options?: GetContentOptions & {
req?: IncomingMessage;
res?: ServerResponse;
apiKey?: string;
authToken?: string;
fetchTotalCount?: T;
}
): Promise<T extends true ? { results: BuilderContent[]; totalCount: number } : BuilderContent[]>
rather than using 2 overloads. Do you think this is better?
There was a problem hiding this comment.
Go with cleaner approach 😄
| }) | ||
| .promise(); | ||
| .promise() | ||
| .then(results => { |
There was a problem hiding this comment.
why is this required when we are already passing fetchTotalCount in request params?
There was a problem hiding this comment.
observer.next(testModifiedResults) (https://github.com/BuilderIO/builder/blob/566a33238fb147a30b9390ea6fc8709a6939902a/packages/core/src/builder.class.ts#L2879C50-L2880C)
This only emits BuilderContent[] and the totalCount returned by result.totalCount is discarded at this point. That is why totalCountByKey is used as a cache to store totalCount during this flush.
| * When `true`, the API will also return the total number of matching entries | ||
| * regardless of `limit`/`offset`. Useful for building numbered pagination. | ||
| */ | ||
| fetchTotalCount?: boolean; |
There was a problem hiding this comment.
why are we adding to gen2 when we do not target it in this PR?
There was a problem hiding this comment.
removing this from here
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| return { | ||
| results, | ||
| totalCount: key !== undefined ? instance.totalCountByKey[key] ?? 0 : 0, | ||
| }; |
There was a problem hiding this comment.
Stale totalCount returned when key resolves to cached observable
Medium Severity
When queueGetContent finds an existing observable for the same key (line ~2484 in the currentObservable check), it replays the cached value via nextTick without making an API call. In that case, totalCountByKey is never populated (or holds a stale value from a previous call), but the .then() in getAll still reads from instance.totalCountByKey[key], returning either 0 or an outdated totalCount. This means repeated getAll calls with fetchTotalCount: true on the browser (where instance === this and observables persist) can return a stale or zero totalCount when the cache short-circuits the fetch.
Additional Locations (1)
3 participants
Description
The Content API (
/api/v3/content/:model) already supports afetchTotalCountquery parameter. WhenfetchTotalCount=trueis passed, the server:countDocumentsquery in parallel with the data fetch{ results: BuilderContent[], totalCount: number }instead of the usual{ results: BuilderContent[] }This is already used internally by admin UI components (e.g.
ComponentsUsageTable,SymbolEntriesTable) which call the API directly. However, the public SDKgetAllmethod does not expose this parameter and currently always returns onlyBuilderContent[], silently discardingtotalCountfrom the response.This PR:
fetchTotalCountoption inGetContentOptionsfetchTotalCount: true, returns{ results: BuilderContent[], totalCount: number }instead ofBuilderContent[]fetchTotalCountis absent orfalse, preserves the existing return shapeBuilderContent[]for full backwards compatibilityScreenshot
https://www.loom.com/share/72c25dbbd7fd4293ae5e3c794d4a8822
Note
Medium Risk
Adds an optional new response shape for
getAll()and threads a new query param through request building, which could affect caching/keying and downstream typing/handling when enabled. Default behavior remains unchanged, keeping backward-compat risk limited to users opting in.Overview
getAll()now accepts a newfetchTotalCountoption, forwards it to the Content API asfetchTotalCount=true, and (when enabled) resolves to{ results, totalCount }instead of a bareBuilderContent[].Core request handling stores
totalCountfrom API responses per request key, andgetAll()updates its keying/SSR behavior to reliably retrieve the count. Tests were added to cover URL param inclusion and the conditional return shape, and a changeset bumps@builder.io/sdkand@builder.io/reactminor versions.Written by Cursor Bugbot for commit 73b1d7d. This will update automatically on new commits. Configure here.