⚡ perf(stripe): optimize variant update array traversals#61
⚡ perf(stripe): optimize variant update array traversals#61
Conversation
Replaces O(N^2) Array.prototype.find() lookups with O(1) Map lookups when matching incoming primary and secondary product variants against existing ones during the update process in ProductStripeService. Co-authored-by: AJFrio <20246916+AJFrio@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7431ed5ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (Array.isArray(updates.variants)) { | ||
| const incomingVariants = updates.variants | ||
| const existingVariants = Array.isArray(existingProduct.variants) ? existingProduct.variants : [] | ||
| const existingVariantsMap = new Map(existingVariants.filter(ev => ev.id).map(ev => [ev.id, ev])) |
There was a problem hiding this comment.
existingVariantsMap is now built eagerly with existingVariants.filter(ev => ev.id), which throws if stored variant arrays contain null/undefined elements; this can happen because the admin update route accepts raw JSON and does not validate variant item shape before persistence (src/routes/admin/products.js). Before this commit, the find path only ran when an incoming variant had an id, so updates containing only new variants could still succeed despite malformed historical entries. With the eager map build, those same updates now fail immediately with a 500, so the map construction should be null-safe (and mirrored for variants2).
Useful? React with 👍 / 👎.
1 participant
💡 What:
Replaced O(N^2)
Array.prototype.find()lookups with O(1)Maplookups insideupdateProductVariantsinsrc/services/ProductStripeService.jsfor both primary and secondary variant groups.🎯 Why:
When updating a product, the service iterated through all incoming variants and used
.find()on the existing variants array to locate the matching prior variant by ID. For a product withNexisting variants andMupdated variants, this resulted in an O(N * M) time complexity. Creating aMapof existing variants by ID before the loop reduces the lookup complexity to O(1), improving the overall time complexity to O(N + M).📊 Measured Improvement:
Created a new performance benchmark (
tests/services/ProductStripeService.perf.test.js) testing the update process with 10,000 existing primary and secondary variants.PR created automatically by Jules for task 936471685218604161 started by @AJFrio