Skip to content

[STR-453] fix: clear stale selectedImageVariationSKU when the selected item changes - #88

Open
iago1501 wants to merge 1 commit into
masterfrom
fix/STR-453-clear-stale-image-variation-sku
Open

[STR-453] fix: clear stale selectedImageVariationSKU when the selected item changes#88
iago1501 wants to merge 1 commit into
masterfrom
fix/STR-453-clear-stale-image-variation-sku

Conversation

@iago1501

@iago1501 iago1501 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What problem is this solving?

skuSelector.selectedImageVariationSKU is only ever assigned when the shopper picks a colour variation — SKUSelector guards that dispatch behind isColor(variationName). Nothing ever invalidated it afterwards: SET_SELECTED_ITEM swapped selectedItem and left the field pointing at the previously selected SKU.

Two consumers give that field absolute priority over selectedItem, falling back only when it is null:

  • vtex.store-componentsreact/components/ProductImages/Wrapper.js (the PDP gallery)
  • vtex.store-componentsreact/ProductSKUAttributes.tsx

So once a colour was picked, changing any non-colour specification (Voltage, Size, Model…) moved selectedItem to the new SKU while the gallery stayed frozen on the images of the old one.

The fix clears the field on SET_SELECTED_ITEM, but only when the item genuinely changed and the stored SKU is not the incoming one. Both guards are load-bearing: the colour selection flow dispatches SELECT_IMAGE_VARIATION and only then redirects, landing here with that same SKU, and the provider re-dispatches this action whenever the product object identity changes with the item unchanged.

Fixes STR-453.

How to test it?

Workspace

On /classic-shoes/p, click the colour swatch so the image variation gets pinned, then move the selection to a different SKU. Same page and same interaction, before and after this change:

selectedItem selectedImageVariationSKU gallery images
before 37 35 (stale) 155470, 155471 — SKU 35
after 37 null 155474, 155475 — SKU 37

Verified live on the linked workspace that the two guards hold, so the existing colour behaviour is untouched:

  • the pin survives when the incoming item is the pinned SKU (colour → redirect flow);
  • the pin survives when the same item is re-dispatched (provider re-dispatch on product identity change);
  • the pin clears only when a genuinely different item arrives.

react/__tests__/reducer.test.ts pins those same branches. yarn test passes: 2 suites, 11 tests, including the pre-existing ProductContextProvider suite.

Describe alternatives you've considered, if any.

Dropping the isColor() guard in store-components' SKUSelector so that every variation change dispatches SELECT_IMAGE_VARIATION. Rejected: it would make the gallery jump to an arbitrary matching SKU while the shopper still has variations unselected, and it leaves the invariant unenforced — any other component that changes the selected SKU would reintroduce the same stale state. Keeping the correction in the reducer holds the guarantee in one place and fixes ProductSKUAttributes at the same time.

Related to / Depends on

STR-453. No sibling PRs — the fix is contained in this repository, even though the visible symptom is reported against vtex.store-components.

…nges

The image variation SKU is only ever set when a colour variation is picked,
but nothing invalidated it afterwards. Changing any non-colour specification
moved selectedItem to a new SKU while the field kept pointing at the old one,
and consumers that give it priority over selectedItem (the product gallery in
store-components, and ProductSKUAttributes) kept rendering the previous SKU.

Clear it on SET_SELECTED_ITEM, but only when the item genuinely changed and
the stored SKU is not the incoming one, so the colour-selection redirect and
the provider's re-dispatch on product identity changes both keep working.

Co-authored-by: Cursor <[email protected]>
@iago1501
iago1501 requested a review from a team as a code owner August 28, 2026 19:15
@iago1501
iago1501 requested review from gabpaladino, leo-prange-vtex and vmourac-vtex and removed request for a team August 28, 2026 19:15
@vtex-io-ci-cd

vtex-io-ci-cd Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@vtex-io-docs-bot

Copy link
Copy Markdown

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

  • There's nothing new to document 🤔
  • I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

  • I just updated 🎉🎉

@iago1501 iago1501 changed the title fix: clear stale selectedImageVariationSKU when the selected item changes [STR-453] fix: clear stale selectedImageVariationSKU when the selected item changes Aug 28, 2026

@mendescamara mendescamara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O diagnóstico e a escolha do reducer como lugar da correção me parecem certos — confirmei que SELECT_IMAGE_VARIATION só é despachado sob isColor (SKUSelector/index.tsx:368-378), que o valor guardado é um itemId (logo a comparação com args.item?.itemId é homogênea) e que ninguém além do provider despacha SET_SELECTED_ITEM.

Tenho uma ressalva, porém, sobre o caso em que o pin realmente tem efeito.

Risco de regressão quando a seleção está incompleta

Quando todas as variações estão selecionadas, selectedItem === selectedImageVariationSKU e o pin é redundante. O pin só muda o que a galeria renderiza quando selectedItem ≠ pin — ou seja, exatamente quando o shopper escolheu uma cor sem ter todas as variações selecionadas. É esse cenário que a mudança pode quebrar:

  1. URL com ?skuId=35, todas as variações selecionadas.
  2. O shopper desmarca uma variação não-cor (clicar no valor já selecionado → isRemoving). Não há redirect, a query fica intacta.
  3. O shopper clica numa cor → SELECT_IMAGE_VARIATION(40) é despachado (SKUSelector/index.tsx:368), mas allSelected === false, então skuIdToRedirect = null e roda redirectToSku(null)setQuery({ skuId: null }) (SKUSelector/index.tsx:267 e 380-398).
  4. skuId sai da query → useSelectedItemFromId refaz getSelectedItem(undefined, items)primeiro item disponível (ex.: 30), que não é nem o item anterior nem o pin.
  5. No reducer: itemChanged = true e pointsToAnotherItem = trueo pin é limpo.

Resultado: o shopper clica numa cor e a galeria volta para as imagens do SKU default. Hoje isso funciona. Os três casos verificados no workspace (pin igual ao item entrante, re-dispatch por identidade de product, item genuinamente diferente) não cobrem esse — ele exige a desmarcação prévia do passo 2.

Sugestão

O sinal que falta ao reducer é se o item veio de uma seleção explícita ou de um fallback. O provider sabe disso:

// ProductContextProvider.tsx:101-104
dispatch({
  type: 'SET_SELECTED_ITEM',
  args: { item: getSelectedItem(skuId, items), fromQueryString: Boolean(skuId) },
})

E no reducer, limpar apenas quando args.fromQueryString !== false (default limpando, para não mudar o comportamento de quem despachar sem o campo).

Isso mantém o bug original corrigido — na troca de Voltagem o redirectToSku('37') coloca skuId=37 na query, então fromQueryString é true — e preserva o pin no caminho de fallback acima.

Vale reproduzir os passos 2-4 no workspace antes de decidir: é o único cenário não coberto, e é onde a feature de fato vive.

@iago1501

Copy link
Copy Markdown
Contributor Author

Regression sweep on the linked workspace

Because this changes a reducer that every useProduct() consumer depends on, I ran a broad sweep A/B against master (unlinked) — same pages, same scripted interactions, same viewport — so that pre-existing store noise doesn't get read as a regression.

Before / after

The scripted flow: pin an image variation on SKU 35, then move the selection to SKU 310124175. Same product, same viewport, only the environment differs.

before, on master

after, with this PR

Page load — 14 pages

Home, 2 category pages and 11 PDPs, covering a Color+Size+Durabilidade matrix (working-shirt), 12 colour SKUs (star-color-top), non-colour-only variations (motorola-one, vintage-camera), a Portuguese Cor variation (traveler-backpack), a single-SKU product (orange-juice) and a multi-SKU product with no variations (custom-bell).

Every structural metric matched master exactly on every page: same initial selectedItem, same gallery image count, same swatch count, same price block count. No context ended up with an orphan selectedImageVariationSKU, including the category pages where 6–8 ProductContextProviders are mounted at once.

Interaction — 22 swatch clicks across 8 PDPs

The click-by-click trails (selectedItem / pinned SKU / rendered image, after every click) came out identical between master and the linked workspace, with zero violations of the gallery invariant on either side.

On its own that result proves nothing, and it's worth being explicit about why: through the UI, clicking a colour always redirects to the very SKU being pinned, which is exactly the case the equality guard preserves. That path would never produce a violation, not even against the unfixed code.

Sensitivity control — the flow that actually breaks

So I ran the pathological flow (pin a SKU, then change the selection through another route) on 6 products in both environments:

environment products left with a stale SKU gallery following the selected SKU
master 6 of 6 0 of 6
this PR 0 of 6 6 of 6

Products: classic-shoes, working-shirt, tank-top, star-color-top, blouse, custom-bell. This confirms the check detects the bug when it is present, and finds nothing once the fix is in.

State leakage

Two paths where a context reducer typically leaks were checked on the linked workspace: a category page holding 8 concurrent providers (all pins null), and client-side navigation from a PDP with an active pin out to the category page and into another PDP — the pin did not survive the transition, and the gallery was correct on arrival.

About the console noise

The linked workspace logs more errors than master, but that is the linked app running in dev mode: StrictMode warnings, UNSAFE_componentWillReceiveProps, findDOMNode deprecations — all stripped from the production build. The one error exclusive to the linked environment traces to [email protected]/react/useRenderSession.js:61, an unrelated app; it also fires on the home page, where no product context is mounted at all.

Two products render no price block (traveler-backpack, motorola-one) — they render none on master either, it's catalog data without a configured price.


The two images above live on the str-453-evidence-assets branch, which is not meant to be merged — GitHub only accepts a URL in a PR body, not an upload through the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants