@W-20038640 Fix: 400 Error#3443
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| [attributeId]: value | ||
| })) | ||
| }, []) | ||
|
|
There was a problem hiding this comment.
I see all 3 are using same code for auto selection of variant product.
Whats the different between three modals ? whats the use case for each ?
Also its being used at 3 places, can we extract it out in some util/helper ?
There was a problem hiding this comment.
Also do we need this auto selection now ? Does it help with the 400 bug ? How ?
There was a problem hiding this comment.
We need to refactor the duplicated code from:
app/components/product-view-modal/index.jsxapp/components/bonus-product-view-modal/index.jsxapp/components/product-view-modal/bundle.jsx
There was a problem hiding this comment.
- BonusProductViewModal: Shows bonus products that customers can select when they qualify for promotions (e.g., "Buy 2, get 1 free - choose your free item")
- ProductViewModal: Quick view modal for regular products from product listing pages
- BundleProductViewModal: Shows products that are part of a bundle on the cart page when editing bundle items
Since we are changing architecture for one modal, we should not let other modals use the URL architecture. All three needed the same refactoring because they all display products with variations that were previously using URL state.
The auto-selection logic was already there before in the old code - we just had to re-implement it in the new React state approach. Previously, auto-selection happened via URL parameters. Now, we're doing it in react state.
If I recall correctly, without it, the modal doesn't fetch image etc on the product-view modal. You had to click the size for it to start displaying the image
Yes, I'll refactor duplicate code
There was a problem hiding this comment.
@sf-deepali-bharmal @sf-cboscenco I changed it
Instead of all 3 modals, now only product-view modal will use react state.
A test was failing where clicking the update button on a product view modal was not sending the error notification if the update failed. I couldn't find how fix it and I was suspecting the changes to modals caused this. So I reverted making changes to other modals.
There was a problem hiding this comment.
@sf-shikhar-prasoon Please test all three modals.
| isProductPartOfSet = false, | ||
| isProductPartOfBundle = false | ||
| isProductPartOfBundle = false, | ||
| controlledVariationValues = null |
There was a problem hiding this comment.
controlledVariationValues = { } ?
There was a problem hiding this comment.
We use null as the default to distinguish between two modes: URL mode (when null) and controlled React state mode (when not null). The hook checks if (controlledVariationValues !== null) to decide whether to read from URL parameters or use the provided state. If we used {} as the default, the hook couldn't tell the difference between "use URL mode" vs "use controlled mode with empty state".
There was a problem hiding this comment.
Is there a possibility of controlledVariationValues being empty / {} ?
In that case code will return no values and will rely on the original params, how will UI behave in such case ?
There was a problem hiding this comment.
When controlledVariationValues = {}
// Line 35: Falls back to product's variationValues
let value = controlledVariationValues[key] || variationValues?.[key]
UI Behavior:
Case 1: Empty {} with No Defaults
variationParams = {}
variant = undefined // No variant selected
UI will show everything unselected and Add to cart button disabled
Case 2: Empty {} but Product has defaults
Line 35 falls back to product's variationValues
UI will show the default selection selected and add to cart button will be enabled
packages/template-retail-react-app/app/components/bonus-product-view-modal/index.jsx
Outdated
Show resolved
Hide resolved
| return value ? {...acc, [key]: value} : acc | ||
| }, {}) | ||
| } | ||
|
|
There was a problem hiding this comment.
If you pass controlledVariationValues it returns values from that but otherwise it returns values from params.
params is basically a call usePDPSearchParams(product.id) using productId.
Why can not we also use productId of the selectedBonusProduct variant instead of passing variant info as part of controlledVariationValues.
Whats the value of product here when we are in the Bonus Modal selecting various variants?
There was a problem hiding this comment.
On line 35 If controlledVariationValues[key] does not have key we fallback on variationValues anyway.
And variationValues comes from product which is being passed in this function.
Then why not rely on existing logic to get you params ?
There was a problem hiding this comment.
Question 1: Why not use productId of selected variant instead of controlledVariationValues?
the app currently follow this pattern of using useVariant() in use-variation-params.js and we are following that? And if we use productId , we would be bypassing that. If we think that the productId approach is better, I can change it.
Question 2: Whats the value of product here
product changes from master to variant as user selects
// 1. Modal Opens
product = {
id: 'master-product-123',
variationAttributes: [{id: 'color', values: [...]}],
variants: [{productId: 'variant-red-M'}, ...]
}
// 2. User Selects "Red"
controlledVariationValues = {color: 'red'}
variant = {productId: 'variant-red-123', ...} // Computed from master
// 3. useProductViewModal fetches variant
useProduct({id: 'variant-red-123'})
↓
product = {
id: 'variant-red-123', // ← NOW it's the variant product!
price: 29.99,
inventory: {...},
...
}
We are using existing logic, just conditionally.
There was a problem hiding this comment.
I am also saying we use existing useVariant(), but I think you don't need this new piece of code to return params using controlledVariationValues. Existing code might be doing the same already.
packages/template-retail-react-app/app/components/bonus-product-view-modal/index.jsx
Outdated
Show resolved
Hide resolved
|
Can you add a test to make sure the core issue of url pollution is fixed something like: })` Also check the integration failures and fix the issues |
|
@ddiazccrz added tests for URL Pollution Prevention |
|
@sf-shikhar-prasoon Could you attach a video of the working flow? |
* fix * add/update tests * variant selection fix, test fix * fix useVariant if product is already a variant * only use react state for product-view modal * removed the unnecessary useCallback * add tests- Variation State Reset | URL Pollution Prevention
Description
In this PR we are fixing this issue:
When we pick color and size of the bonus variants in the modal, the Url for the PDP product also picks up the same color and size and that makes PDP product url invalid.
bug.fix-.400.error.mov
I the page is refreshed in this state, the app breaks and shows a 400 Error.
Types of Changes
Changes
Architectural Changes
File Changes
Core Hooks (added controlled mode support):
app/hooks/use-product-view-modal.js - removed all URL management
app/hooks/use-variation-params.js - reads from controlled values when provided
app/hooks/use-variant.js - passes controlled values through
app/hooks/use-variation-attributes.js - uses onClick instead of href in controlled mode
app/hooks/use-derived-product.js - aggregates and passes controlled props
Components (now use React state):
app/components/product-view/index.jsx - accepts and passes controlled props
app/components/bonus-product-view-modal/index.jsx - manages variation state with useState
app/components/product-view-modal/index.jsx - manages variation state with useState
app/components/product-view-modal/bundle.jsx - manages variation state with useState
Tests (updated and added):
app/hooks/use-product-view-modal.test.js - removed URL-related tests, added tests for simplified behavior
app/hooks/use-variation-params.test.js - added 3 new tests for controlled mode
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization