feat(api): Migrate to VTEX Intelligent Search V1 API#3385
Conversation
Port the Intelligent Search V1 migration from PR #3376 onto the v4 (dev) line. The search client now targets the IS V1 endpoints and exposes fetchProduct/productsByIdentifier/productCount, with request building centralized in the new intelligentSearchRequest util. PDP, products and SKU loaders hydrate via productsByIdentifier. Co-authored-by: Cursor <cursoragent@cursor.com>
salesfelipe
requested review from
hellofanny and
lucasfp13
and removed request for
a team
|
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughVTEX intelligent search client refactored to use a unified v1 request-building utility with segment cookie parsing and structured facet handling. Search client, GraphQL resolvers, and SKU loader migrated to new single-product and bulk-identifier lookup APIs. All test mocks and integration tests updated for v1 endpoint shapes and response structures. ChangesIntelligent Search v1 Client Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/platforms/vtex/loaders/sku.ts (1)
15-22:⚠️ Potential issue | 🟠 MajorVTEX SKU loader can fan out to up to 99 parallel requests (N+1)
productsByIdentifieris implemented asPromise.all(values.map(fetchProduct)), andfetchProductcalls the VTEX v1/api/intelligent-search/v1/productsendpoint with a singlefield+valuepair (no bulk/CSV support in the request builder). WithmaxBatchSize: 99inpackages/api/src/platforms/vtex/loaders/sku.ts, a full batch can trigger up to 99 concurrent HTTP requests, risking higher latency and potential rate-limit/throttling under load. Consider adding a concurrency limiter / loweringmaxBatchSize, or redesigning the integration to use a bulk-capable lookup; otherwise add a quick rationale/benchmark for why this level of parallelism is safe.🤖 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/api/src/platforms/vtex/loaders/sku.ts` around lines 15 - 22, The VTEX SKU loader currently lets productsByIdentifier (which internally runs Promise.all over values via fetchProduct) fan out up to maxBatchSize (99) concurrent HTTP calls; reduce concurrency by either lowering maxBatchSize or adding a concurrency limiter around the batch call—specifically modify the loader in sku.ts to wrap the values iteration (the call site that invokes clients.search.productsByIdentifier / fetchProduct) with a worker pool/limit (e.g., p-limit or a simple semaphore) or set maxBatchSize to a safer value and document the choice; ensure the change targets the code that constructs the values array and calls productsByIdentifier/fetchProduct so the loader no longer issues 99 parallel requests.
🧹 Nitpick comments (5)
packages/api/test/unit/platforms/vtex/utils/intelligentSearchRequest.test.ts (1)
374-411: ⚡ Quick winAdd explicit malformed/missing-cookie tests for
parseSegmentCookiefallback contract.This block validates real valid cookies well, but it misses the failure-path contract (
{}on invalid/missingvtex_segment). Adding those tests will lock in the non-throw behavior and prevent regressions.Suggested test additions
describe('real vtex_segment cookie examples', () => { + it('returns empty object when cookie header is missing', () => { + expect(parseSegmentCookie(undefined)).toEqual({}) + }) + + it('returns empty object for malformed base64/json cookie', () => { + expect(parseSegmentCookie('vtex_segment=not-base64')).toEqual({}) + }) + // '{"campaigns":null,"channel":"1",...,"facets":"zip-code=01002020;country=BRA;..."}' const segmentWithShippingBase64 =🤖 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/api/test/unit/platforms/vtex/utils/intelligentSearchRequest.test.ts` around lines 374 - 411, Add unit tests for parseSegmentCookie that assert its fallback contract: when vtex_segment is missing or malformed it should return an empty object ({}), not throw. Use the existing cookieHeader helper to pass an empty string (no cookie) and an invalid base64 string (e.g. 'invalid') into parseSegmentCookie, and add assertions like expect(segment).toEqual({}) for both cases to lock in the non-throw behavior.packages/api/src/platforms/vtex/clients/search/index.ts (2)
154-159: ⚡ Quick winUnnecessary
Promise.resolvewrapper.
fetchAPIalready returns a Promise, so wrapping it inPromise.resolve()adds no value.♻️ Suggested simplification
- return Promise.resolve( - fetchAPI( - `${base}/api/intelligent-search/v1/products?${request.params.toString()}`, - { headers } - ) - ) + return fetchAPI( + `${base}/api/intelligent-search/v1/products?${request.params.toString()}`, + { headers } + )🤖 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/api/src/platforms/vtex/clients/search/index.ts` around lines 154 - 159, The return currently wraps fetchAPI(...) in Promise.resolve unnecessarily; update the function to return fetchAPI(...) directly instead of Promise.resolve(fetchAPI(...))—locate the return statement that builds the URL with `${base}/api/intelligent-search/v1/products?${request.params.toString()}` and headers and replace the wrapped call so it simply returns the Promise from fetchAPI with the same arguments.
162-182: Bound N-request parallelism inproductsByIdentifier(no IS v1 bulk endpoint)
productsByIdentifierissues one Intelligent Search v1/productsrequest pervaluesentry viaPromise.all. In the SKU loader path it’s capped byDataLoader’smaxBatchSize: 99, but it can still burst up to 99 concurrent upstream calls. Since VTEX Intelligent Search v1 doesn’t expose a bulk/batch products endpoint, consider adding a concurrency limit (or otherwise throttling) and document the expected maxvaluescardinality.🤖 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/api/src/platforms/vtex/clients/search/index.ts` around lines 162 - 182, productsByIdentifier currently fires one fetchProduct call per value via Promise.all which can burst many concurrent requests (up to DataLoader maxBatchSize), so limit concurrency: replace Promise.all over values with a controlled parallelism runner (e.g., use p-map/p-limit or an internal semaphore/queue) to call fetchProduct with a fixed concurrency (choose a sensible default like 5–10) and keep the same return filtering; reference productsByIdentifier and fetchProduct when making the change and add a short code comment/docstring noting the chosen max concurrency and expected max values cardinality so callers understand throttling behavior.packages/api/test/integration/mutations.test.ts (1)
101-113: ⚡ Quick winClarify the expected fetch calls in the comment.
The comment states "only checkout calls are made" but the test expects 2 calls while only
checkoutOrderFormValidFetchis in the explicit array. The second call appears to be a dynamic v1/products fetch handled by the interceptor at lines 70-74. Update the comment to explicitly state both calls for easier debugging.📝 Suggested comment revision
- // When cart is valid and etag is up to date, only checkout calls are made. + // When cart is valid and etag is up to date: + // 1. GET orderForm (checkoutOrderFormValidFetch) + // 2. GET product data via v1/products (handled dynamically) expect(mockedFetch).toHaveBeenCalledTimes(2)🤖 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/api/test/integration/mutations.test.ts` around lines 101 - 113, The test comment for the `validateCart` case is misleading: it says "only checkout calls are made" but `mockedFetch` is expected to be called twice because the test's fetch interceptor (`pickFetchAPICallResult`) handles both the explicit `checkoutOrderFormValidFetch` and an implicit dynamic v1/products call; update the inline comment to enumerate both expected calls (checkout and v1/products) so future readers understand why `mockedFetch` is called twice — locate the test around `ValidateCartMutation`, `mockedFetch`, `checkoutOrderFormValidFetch`, and the `pickFetchAPICallResult` usage to change the comment accordingly.packages/api/test/mocks/ValidateCartMutation.ts (1)
499-512: ⚡ Quick winAdd explicit return type annotation.
The
createProductFetchResultForSkuhelper would benefit from an explicit return type for better TypeScript safety and IDE autocomplete.🔧 Suggested type annotation
-export const createProductFetchResultForSku = (skuId: string) => { +export const createProductFetchResultForSku = (skuId: string): typeof productSearchPage1Count1Fetch.result => { const { items, ...product } = productSearchPage1Count1Fetch.result🤖 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/api/test/mocks/ValidateCartMutation.ts` around lines 499 - 512, The helper createProductFetchResultForSku currently returns an inferred object; add an explicit return type to its function signature (for example use the appropriate existing shape such as the type of productSearchPage1Count1Fetch.result or the concrete ProductFetchResult interface if one exists) so TypeScript and IDEs get proper typing; update the function declaration to: export const createProductFetchResultForSku = (skuId: string): <appropriate-type> => { ... } and ensure the chosen type matches the structure (productId, items with itemId) and import or reference that type where needed.
🤖 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.
Outside diff comments:
In `@packages/api/src/platforms/vtex/loaders/sku.ts`:
- Around line 15-22: The VTEX SKU loader currently lets productsByIdentifier
(which internally runs Promise.all over values via fetchProduct) fan out up to
maxBatchSize (99) concurrent HTTP calls; reduce concurrency by either lowering
maxBatchSize or adding a concurrency limiter around the batch call—specifically
modify the loader in sku.ts to wrap the values iteration (the call site that
invokes clients.search.productsByIdentifier / fetchProduct) with a worker
pool/limit (e.g., p-limit or a simple semaphore) or set maxBatchSize to a safer
value and document the choice; ensure the change targets the code that
constructs the values array and calls productsByIdentifier/fetchProduct so the
loader no longer issues 99 parallel requests.
---
Nitpick comments:
In `@packages/api/src/platforms/vtex/clients/search/index.ts`:
- Around line 154-159: The return currently wraps fetchAPI(...) in
Promise.resolve unnecessarily; update the function to return fetchAPI(...)
directly instead of Promise.resolve(fetchAPI(...))—locate the return statement
that builds the URL with
`${base}/api/intelligent-search/v1/products?${request.params.toString()}` and
headers and replace the wrapped call so it simply returns the Promise from
fetchAPI with the same arguments.
- Around line 162-182: productsByIdentifier currently fires one fetchProduct
call per value via Promise.all which can burst many concurrent requests (up to
DataLoader maxBatchSize), so limit concurrency: replace Promise.all over values
with a controlled parallelism runner (e.g., use p-map/p-limit or an internal
semaphore/queue) to call fetchProduct with a fixed concurrency (choose a
sensible default like 5–10) and keep the same return filtering; reference
productsByIdentifier and fetchProduct when making the change and add a short
code comment/docstring noting the chosen max concurrency and expected max values
cardinality so callers understand throttling behavior.
In `@packages/api/test/integration/mutations.test.ts`:
- Around line 101-113: The test comment for the `validateCart` case is
misleading: it says "only checkout calls are made" but `mockedFetch` is expected
to be called twice because the test's fetch interceptor
(`pickFetchAPICallResult`) handles both the explicit
`checkoutOrderFormValidFetch` and an implicit dynamic v1/products call; update
the inline comment to enumerate both expected calls (checkout and v1/products)
so future readers understand why `mockedFetch` is called twice — locate the test
around `ValidateCartMutation`, `mockedFetch`, `checkoutOrderFormValidFetch`, and
the `pickFetchAPICallResult` usage to change the comment accordingly.
In `@packages/api/test/mocks/ValidateCartMutation.ts`:
- Around line 499-512: The helper createProductFetchResultForSku currently
returns an inferred object; add an explicit return type to its function
signature (for example use the appropriate existing shape such as the type of
productSearchPage1Count1Fetch.result or the concrete ProductFetchResult
interface if one exists) so TypeScript and IDEs get proper typing; update the
function declaration to: export const createProductFetchResultForSku = (skuId:
string): <appropriate-type> => { ... } and ensure the chosen type matches the
structure (productId, items with itemId) and import or reference that type where
needed.
In
`@packages/api/test/unit/platforms/vtex/utils/intelligentSearchRequest.test.ts`:
- Around line 374-411: Add unit tests for parseSegmentCookie that assert its
fallback contract: when vtex_segment is missing or malformed it should return an
empty object ({}), not throw. Use the existing cookieHeader helper to pass an
empty string (no cookie) and an invalid base64 string (e.g. 'invalid') into
parseSegmentCookie, and add assertions like expect(segment).toEqual({}) for both
cases to lock in the non-throw behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4c420f0-51ca-43b6-9a9d-b249ad135771
📒 Files selected for processing (12)
packages/api/src/platforms/vtex/clients/search/index.tspackages/api/src/platforms/vtex/loaders/sku.tspackages/api/src/platforms/vtex/resolvers/query.tspackages/api/src/platforms/vtex/utils/intelligentSearchRequest.tspackages/api/test/integration/mutations.test.tspackages/api/test/integration/queries.test.tspackages/api/test/mocks/AllProductsQuery.tspackages/api/test/mocks/ProductQuery.tspackages/api/test/mocks/RedirectQuery.tspackages/api/test/mocks/SearchQuery.tspackages/api/test/mocks/ValidateCartMutation.tspackages/api/test/unit/platforms/vtex/utils/intelligentSearchRequest.test.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
| page: Math.ceil(after / first) || 0, | ||
| count: first, | ||
| query: term ?? undefined, | ||
| sort: SORT_MAP[sort ?? 'score_desc'], |
There was a problem hiding this comment.
If sort is defined but not present on the SORT_MAP it would pass sort as undefined.
| sort: SORT_MAP[sort ?? 'score_desc'], | |
| sort: SORT_MAP[sort] ?? SORT_MAP['score_desc'], |
| sponsoredCount: sponsoredCount ?? undefined, | ||
| const productSearchPromise: Promise<ProductSearchResult> = | ||
| ctx.clients.search | ||
| .productsByIdentifier({ field: 'id', values: productIds }) |
There was a problem hiding this comment.
Here we are hiting the productsByIdentifier endpoint directly without any dataloader controlling how many requests it should batch as it is done in the getSKULoader. Shouldn't it have some logic to control max request fired simultaneously?
| page: 0, | ||
| count: productIds.length, | ||
| query, | ||
| const products = await search.productsByIdentifier({ |
There was a problem hiding this comment.
Same as before. Should we hit this directly without a dataloader controlling the batch size and caching repeated keys requests?
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Reviewed the port of the Intelligent Search v1 migration onto dev (v4). The refactor is clean and the request builder is well factored; the trust-boundary handling in resolveSegmentData (server defaults authoritative over the client-supplied vtex_segment cookie) is exactly right and good to see called out in a comment. A few things before merge.
packages/api/src/platforms/vtex/clients/search/index.ts
🟠 [bug] productsByIdentifier rejects the whole batch on a single missing identifier
fetchProduct returns fetchAPI(...), and fetchAPI throws (NotFoundError, etc.) on any non-2xx — it never resolves to null. So the Promise.all has no per-item catch, which means one 404 (a removed/invalid SKU, or a cross-selling id the catalog no longer has) rejects the entire call. That makes the trailing .filter((product) => product !== null) dead code, and it's a behavior regression vs the old single sku:a;b;c / id:a;b;c query, which silently omitted missing items. This path feeds the SKU loader (cart) and the cross-selling / productsById shelves, so one bad id can blank an entire shelf or fail a cart load.
const productsResult = await Promise.all(
values.map((value) =>
fetchProduct({
field,
value,
hideUnavailableItems: hideUnavailable,
showInvisibleItems,
}).catch(() => null)
)
)
return productsResult.filter(
(product): product is Product => product !== null
)
Can you confirm what the v1 products endpoint returns for a valid-but-unavailable SKU under hideUnavailableItems (404 vs product with empty sellers)? Either way the per-item catch restores the old resilience, and a small int test for "one identifier missing → the rest still return" would lock it in.
💬 [perf] Unbounded fan-out
productsByIdentifier now issues one HTTP request per identifier instead of one batched query. For a large cart or a wide cross-sell that's N round-trips to IS with no concurrency cap. p-limit is already a dependency here — worth wrapping the map to bound concurrency and avoid hammering / rate-limiting IS.
packages/api/src/platforms/vtex/resolvers/query.ts
💬 [behavior] cross-selling now returns a synthetic ProductSearchResult with empty pagination (recordsFiltered: products.length, all proxyURLs ''). Since we slice to first and hydrate by id, paging beyond the first set is gone. Was cross-selling pagination relied on anywhere downstream, or is bounding to first the intended contract now?
💬 [behavior] shouldFallbackToProductRoute is a nice tightening — the product resolver no longer masks infra/transient errors by falling back to the product route, only NotFound / InvalidSkuId / slug-mismatch. Good change; just flagging it's a behavior shift (a 5xx on skuLoader now surfaces instead of silently retrying via route).
💬 [nit] The title says "with caching" but I don't see cache logic in the diff — is that referring to upstream caching enabled by the v1 endpoint, or is a caching layer expected in this PR?
Verdict: Approved with comments
Blocking (🟠):
productsByIdentifier: add a per-itemcatch(() => null)(or confirm v1 never 404s here) so a single missing identifier doesn't fail the whole cart/shelf; the current.filteris dead code.
Non-blocking (🟡/💬):
- Bound the per-identifier fan-out with the already-present
p-limit. - Confirm cross-selling no longer needs real pagination metadata.
- Note the
productresolver fallback narrowing (intended). - Clarify what "caching" in the title refers to.
Checks to confirm before merge: pnpm lint · build · pnpm test:unit + pnpm test:int in packages/api (incl. a partial-failure test for productsByIdentifier) · validate against an account already on IS v1.
salesfelipe
changed the title
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
|
Thanks @salesfelipe!
|
4 participants
Summary
Ports the changes from #3376 (originally opened against
v3) onto thedev(v4) line./api/intelligent-search/v1/...), addingfetchProduct,productsByIdentifier, andproductCount.utils/intelligentSearchRequest.tsutil, withparseSegmentCookiefor segment-aware requests.productsresolver, cross-selling, and the SKU loader now hydrate products viaproductsByIdentifier/fetchProduct, honoringhideUnavailableItems.v3 → v4 adaptations made while porting
GraphqlContext(the v3 PR usedContext) and treatedOptionsas the global type rather than importing it.IStoreSelectedFacetimport in the search client..filter(...)in theproductsresolver.vitestimports in the new unit test (v4 does not enable vitest globals).Test plan
pnpm test:unitinpackages/api(26 passed)pnpm test:intinpackages/api(44 passed)ci/codesandboxcheckMade with Cursor
Summary by CodeRabbit