fix: prevent using stale address data - #358
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
==========================================
- Coverage 82.79% 82.74% -0.06%
==========================================
Files 689 689
Lines 11590 11591 +1
Branches 835 842 +7
==========================================
- Hits 9596 9591 -5
+ Misses 1170 1168 -2
- Partials 824 832 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
50ad6a9 to
4dd3c3a
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a checkout bug where delivery options could be resolved against a previous (stale) address selection when toggling “ship to different address”, leading to incorrect country-based delivery option behavior.
Changes:
- Update
updateCheckoutFormto deriveaddressTypefrom the freshly transformed form data (instead of a lagging DOM read). - Update
updateDeliveryOptionsto compare the effective country of the active address in each state when deciding whether to reset/refresh context. - Add a Vitest regression suite covering address-type toggling and context refetch behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.ts | Switches country comparison to use active address per-state to avoid stale DOM-derived addressType. |
| libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts | Adds regression tests for address-type toggling and conditional context refetching. |
| libs/checkout-common/src/listeners/updateCheckoutForm.ts | Ensures addressType is computed from the just-read form data to prevent “one event behind” state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (checkout.state.addressTypes.includes(key as AddressType)) { | ||
| return; | ||
| } | ||
|
|
||
| transformedData[key as PdkField] = getEntry(data, value as string); | ||
| (transformedData as Record<PdkField, string>)[key as PdkField] = getEntry(data, value as string); |
43cec9a to
7f694a3
Compare
7f694a3 to
5f42081
Compare
5f42081 to
40761d6
Compare
40761d6 to
63cfd14
Compare
63cfd14 to
1d43ef5
Compare
| // Fails today: shows 'NL'. getDeliveryOptionsAddress reads the committed addressType, but updateCheckoutForm | ||
| // computes that from getAddressType() reading the *previous* form, so it lags one event behind the DOM. | ||
| expect(currentDeliveryCc()).toBe(SHIPPING_CC); |
| // Turn the checkbox off again: should immediately fall back to billing (NL). Today it shows the lagged 'FR'. | ||
| await changeAddressTypeTo(AddressType.Billing); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts:57
- These comments are now misleading after the fix: the test should describe the previous buggy behavior as historical ("previously"/"regression"), not as something that still fails today.
// Fails today: shows 'NL'. getDeliveryOptionsAddress reads the committed addressType, but updateCheckoutForm
// computes that from getAddressType() reading the *previous* form, so it lags one event behind the DOM.
libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts:60
- This comment refers to the buggy behavior as happening "Today", but the PR fixes it. Rephrase to indicate the behavior occurred previously to avoid confusion when the test passes.
// Turn the checkbox off again: should immediately fall back to billing (NL). Today it shows the lagged 'FR'.
250d505 to
72621f6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
libs/checkout-common/src/listeners/updateCheckoutForm.ts:32
- The second loop filters out address-type keys based on
checkout.state.addressTypes. If an address type is disabled viaconfig.hasAddressType()(so it’s not inaddressTypes), its key (e.g.'shipping') will fall through and be treated as aPdkField, overwriting the nested address object with a string. This can corrupttransformedDataand break later reads likeform[AddressType.Shipping][...].
Object.entries(config.formData).forEach(([key, value]) => {
if (checkout.state.addressTypes.includes(key as AddressType)) {
return;
}
(transformedData as Record<PdkField, string>)[key as PdkField] = getEntry(data, value as string);
});
libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts:57
- These test comments are written as time-relative (“Fails today…”, “Today it shows…”). Once the fix is merged, they’ll become inaccurate and confusing for future readers; it’s better to describe the regression/behavior neutrally.
// Fails today: shows 'NL'. getDeliveryOptionsAddress reads the committed addressType, but updateCheckoutForm
// computes that from getAddressType() reading the *previous* form, so it lags one event behind the DOM.
libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts:60
- Same as above: this comment is time-relative (“Today it shows…”). Prefer a neutral description so the test remains accurate over time.
// Turn the checkbox off again: should immediately fall back to billing (NL). Today it shows the lagged 'FR'.
72621f6 to
7bd3dec
Compare
7bd3dec to
7032d8e
Compare
7032d8e to
6d504f4
Compare
6d504f4 to
3ef1a58
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts:60
- This comment refers to "Today" showing a lagged value; after the fix it should be phrased historically to avoid becoming misleading documentation.
// Turn the checkbox off again: should immediately fall back to billing (NL). Today it shows the lagged 'FR'.
libs/checkout-delivery-options/src/listeners/updateDeliveryOptions.spec.ts:57
- Test comment says "Fails today" and describes the pre-fix behavior. After this change lands, that wording becomes inaccurate and can confuse future readers when the test passes.
// Fails today: shows 'NL'. getDeliveryOptionsAddress reads the committed addressType, but updateCheckoutForm
// computes that from getAddressType() reading the *previous* form, so it lags one event behind the DOM.
3ef1a58 to
ed14a3f
Compare
INT-1680