perf: reduce memory allocation pressure in VTEX transforms and CSV#1535
Merged
aka-sacci-ccr merged 4 commits intomainfrom Mar 2, 2026
Merged
perf: reduce memory allocation pressure in VTEX transforms and CSV#1535aka-sacci-ccr merged 4 commits intomainfrom
aka-sacci-ccr merged 4 commits intomainfrom
Conversation
…direct parsing
Addresses memory profiling findings — the main bottleneck is ephemeral
object allocation rate (not leaks), which increases GC pressure under
high traffic.
Changes:
- Internalize schema.org URL strings as module-level constants so V8
reuses a single reference instead of allocating per-call copies
- Extract shared buildOffer() to eliminate the spread copy in
toOfferLegacy ({...toOffer(seller), teasers})
- Reuse options object in toProduct recursive calls instead of
spreading {…options, imagesByKey} per variant
- Replace .concat() chains with push()-based array building
- Mutate product offers in-place during simulation instead of creating
spread copies of every offer/product/spec object
- Merge 4-pass CSV parsing in redirectsFromCsv into a single loop
producing Route[] directly, eliminating 3 intermediate arrays
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds schema constants and refactors offer construction; simulation now mutates product offers in place and selectively re-aggregates when prices change; CSV redirect parsing switched from functional chaining to an explicit loop with inline validation. (48 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
vtex/utils/transform.ts (1)
456-467: Guards on always-truthy arrays are misleading.
toAdditionalPropertyCategories,toAdditionalPropertyClusters, andtoAdditionalPropertyReferenceIdsall returnPropertyValue[](possibly empty), nevernullorundefined. An empty array is truthy, soif (categoryAdditionalProperties)and the others are always true — the push always runs, even spreading[]as a no-op. The guards add no protection and may confuse future readers.♻️ Simplify to unconditional pushes
- if (categoryAdditionalProperties) { - additionalProperty.push(...categoryAdditionalProperties); - } - if (clusterAdditionalProperties) { - additionalProperty.push(...clusterAdditionalProperties); - } - if (referenceIdAdditionalProperty) { - additionalProperty.push(...referenceIdAdditionalProperty); - } + additionalProperty.push(...categoryAdditionalProperties); + additionalProperty.push(...clusterAdditionalProperties); + additionalProperty.push(...referenceIdAdditionalProperty);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/transform.ts` around lines 456 - 467, The if-guards around pushing categoryAdditionalProperties, clusterAdditionalProperties, and referenceIdAdditionalProperty are misleading because toAdditionalPropertyCategories, toAdditionalPropertyClusters, and toAdditionalPropertyReferenceIds return PropertyValue[] (never null/undefined), so empty arrays are truthy; remove the unnecessary conditionals and unconditionally merge those arrays into additionalProperty (e.g., push(...categoryAdditionalProperties) / push(...clusterAdditionalProperties) / push(...referenceIdAdditionalProperty) or initialize additionalProperty with all spreads) and keep references to additionalProperty, specificationsAdditionalProperty, categoryAdditionalProperties, clusterAdditionalProperties, and referenceIdAdditionalProperty for locating the change.website/loaders/redirectsFromCsv.ts (1)
68-69:from/tovariable names shadow the outerfromparameter.
fromdeclared here shadows thefromparameter ofgetRedirectFromFile(Line 37). TypeScript scoping keeps this safe, but it can mislead readers who scan the function signature. ConsidercsvFrom/csvToorsourcePath/targetPath.♻️ Proposed rename
- const from = parts[0]; - const to = parts[1]; + const csvFrom = parts[0]; + const csvTo = parts[1]; - if (!from || !to || from === to) continue; + if (!csvFrom || !csvTo || csvFrom === csvTo) continue; routes.push({ - pathTemplate: from, + pathTemplate: csvFrom, isHref: true, handler: { value: { __resolveType: "website/handlers/redirect.ts", - to, + to: csvTo, type: type as Redirect["type"], discardQueryParameters, }, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/loaders/redirectsFromCsv.ts` around lines 68 - 69, The local variables assigned from parts should be renamed so they don't shadow the outer parameter `from` of getRedirectFromFile; change the two locals (currently declared as `from` and `to`) to clearer names like `csvFrom` and `csvTo` (or `sourcePath`/`targetPath`) and update all subsequent uses in getRedirectFromFile accordingly so the outer `from` parameter remains unshadowed and the intent is clear.vtex/utils/extensions/simulation.ts (1)
111-112:changedis set even when the simulated price equals the existing offer price.
o.price = salePrice; changed = true;unconditionally marks the product as changed whenever a matching simulated offer exists. If the simulation returns the same price that is already on the offer,aggregateOffers(which sorts in-place) is still invoked, providing no benefit. The optimisation in this PR is slightly undermined for the stable-price case.♻️ Proposed fix — only set `changed` on an actual price difference
+ if (o.price !== salePrice) { o.price = salePrice; changed = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/extensions/simulation.ts` around lines 111 - 112, The code currently sets o.price = salePrice and changed = true unconditionally; update the block that handles the simulated offer (the variables o, salePrice, and changed) to first compare the existing o.price to salePrice and only assign o.price and set changed = true when they differ (use a strict/normalized numeric comparison to avoid false positives from type/coercion). Ensure any subsequent call that relies on changed (e.g., aggregateOffers) only runs when changed is true so stable-price cases skip the in-place sort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vtex/utils/extensions/simulation.ts`:
- Around line 109-110: Replace the current falsy checks with explicit
null/undefined checks so a simulated value of 0 is preserved: change the
salePrice assignment to use simulated.price != null ? simulated.price / 100 :
o.price and change listPrice to simulated.listPrice != null ?
simulated.listPrice / 100 : undefined (or whatever the previous “no simulated
listPrice” sentinel was). Update the assignments around the salePrice and
listPrice variables in simulation.ts where those symbols are defined to ensure
both zero and non-zero simulated values are handled consistently.
In `@vtex/utils/transform.ts`:
- Around line 375-383: The code currently mutates the caller's options by
setting options.imagesByKey, causing cross-product contamination; change this to
create a local imagesByKey variable (built from items via getImageKey) instead
of assigning into options, and update any recursive or subsequent calls to
toProduct/toProductVariant to accept and propagate that local imagesByKey (pass
it through the call chain where options.imagesByKey was used) so the caller's
options object is never mutated. Ensure all references that read
options.imagesByKey use the new local imagesByKey variable or the propagated
map.
---
Nitpick comments:
In `@vtex/utils/extensions/simulation.ts`:
- Around line 111-112: The code currently sets o.price = salePrice and changed =
true unconditionally; update the block that handles the simulated offer (the
variables o, salePrice, and changed) to first compare the existing o.price to
salePrice and only assign o.price and set changed = true when they differ (use a
strict/normalized numeric comparison to avoid false positives from
type/coercion). Ensure any subsequent call that relies on changed (e.g.,
aggregateOffers) only runs when changed is true so stable-price cases skip the
in-place sort.
In `@vtex/utils/transform.ts`:
- Around line 456-467: The if-guards around pushing
categoryAdditionalProperties, clusterAdditionalProperties, and
referenceIdAdditionalProperty are misleading because
toAdditionalPropertyCategories, toAdditionalPropertyClusters, and
toAdditionalPropertyReferenceIds return PropertyValue[] (never null/undefined),
so empty arrays are truthy; remove the unnecessary conditionals and
unconditionally merge those arrays into additionalProperty (e.g.,
push(...categoryAdditionalProperties) / push(...clusterAdditionalProperties) /
push(...referenceIdAdditionalProperty) or initialize additionalProperty with all
spreads) and keep references to additionalProperty,
specificationsAdditionalProperty, categoryAdditionalProperties,
clusterAdditionalProperties, and referenceIdAdditionalProperty for locating the
change.
In `@website/loaders/redirectsFromCsv.ts`:
- Around line 68-69: The local variables assigned from parts should be renamed
so they don't shadow the outer parameter `from` of getRedirectFromFile; change
the two locals (currently declared as `from` and `to`) to clearer names like
`csvFrom` and `csvTo` (or `sourcePath`/`targetPath`) and update all subsequent
uses in getRedirectFromFile accordingly so the outer `from` parameter remains
unshadowed and the intent is clear.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
vtex/utils/extensions/simulation.tsvtex/utils/transform.tswebsite/loaders/redirectsFromCsv.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="vtex/utils/transform.ts">
<violation number="1" location="vtex/utils/transform.ts:375">
P2: Mutating `options.imagesByKey` makes `toProduct` reuse the first product’s image map for all subsequent products in the same request, which can mis-resolve image URLs across different products. Avoid mutating the shared options object and pass a per-product `imagesByKey` instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…in toProduct - simulation.ts: use explicit null checks (!= null) instead of truthy checks for simulated.price and simulated.listPrice, so a value of 0 is preserved instead of falling back to the original price - transform.ts: stop mutating the caller's options.imagesByKey which caused cross-product contamination when the same options object was shared across a .map(); compute imagesByKey locally and create one variantOptions object only when needed for recursive calls Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vtex/utils/transform.ts (1)
459-470: Optional: theifguards oncategoryAdditionalProperties,clusterAdditionalProperties, andreferenceIdAdditionalPropertyare always truthy.All three are produced by
.map()calls intoAdditionalPropertyCategories,toAdditionalPropertyClusters, andtoAdditionalPropertyReferenceIds, so they are alwaysPropertyValue[](possibly empty, nevernull/undefined). An empty array is truthy, so the guards never short-circuit. They add a false implication that these values can be absent.🧹 Proposed cleanup
- const additionalProperty: PropertyValue[] = [ - ...specificationsAdditionalProperty, - ]; - if (categoryAdditionalProperties) { - additionalProperty.push(...categoryAdditionalProperties); - } - if (clusterAdditionalProperties) { - additionalProperty.push(...clusterAdditionalProperties); - } - if (referenceIdAdditionalProperty) { - additionalProperty.push(...referenceIdAdditionalProperty); - } + const additionalProperty: PropertyValue[] = [ + ...specificationsAdditionalProperty, + ...categoryAdditionalProperties, + ...clusterAdditionalProperties, + ...referenceIdAdditionalProperty, + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/transform.ts` around lines 459 - 470, The if guards around categoryAdditionalProperties, clusterAdditionalProperties, and referenceIdAdditionalProperty are redundant because those values are always arrays (from toAdditionalPropertyCategories / toAdditionalPropertyClusters / toAdditionalPropertyReferenceIds) and an empty array is truthy; remove the conditional checks and either push their contents unconditionally (push(...categoryAdditionalProperties), push(...clusterAdditionalProperties), push(...referenceIdAdditionalProperty)) or build additionalProperty by concatenating all arrays (e.g., [...specificationsAdditionalProperty, ...categoryAdditionalProperties, ...clusterAdditionalProperties, ...referenceIdAdditionalProperty]) to simplify the logic and avoid implying they can be null/undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vtex/utils/extensions/simulation.ts`:
- Around line 115-116: The code sets changed = true unconditionally after
assigning o.price = salePrice, which causes aggregateOffers to run even when
salePrice equals the existing o.price (notably when simulated.price == null and
salePrice falls back to o.price). Modify the logic in the function handling
offers (references: variables o.price, salePrice, changed and the
aggregateOffers call) so that you only set changed = true when the new salePrice
is different from the current o.price (e.g., compare salePrice !== o.price
before assignment) — update the assignment to only mutate o.price and flip
changed when the values differ to prevent unnecessary re-aggregation.
---
Nitpick comments:
In `@vtex/utils/transform.ts`:
- Around line 459-470: The if guards around categoryAdditionalProperties,
clusterAdditionalProperties, and referenceIdAdditionalProperty are redundant
because those values are always arrays (from toAdditionalPropertyCategories /
toAdditionalPropertyClusters / toAdditionalPropertyReferenceIds) and an empty
array is truthy; remove the conditional checks and either push their contents
unconditionally (push(...categoryAdditionalProperties),
push(...clusterAdditionalProperties), push(...referenceIdAdditionalProperty)) or
build additionalProperty by concatenating all arrays (e.g.,
[...specificationsAdditionalProperty, ...categoryAdditionalProperties,
...clusterAdditionalProperties, ...referenceIdAdditionalProperty]) to simplify
the logic and avoid implying they can be null/undefined.
mcandeia
approved these changes
Mar 2, 2026
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
2 participants
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.
…direct parsing
Addresses memory profiling findings — the main bottleneck is ephemeral object allocation rate (not leaks), which increases GC pressure under high traffic.
Changes:
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by cubic
Reduce transient allocations in VTEX transforms, offer simulation, and CSV redirect parsing to lower GC pressure and improve throughput. Fix zero-price handling and prevent options mutation across product recursion.
Refactors
Bug Fixes
Written for commit 74c8fb6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores