fix: key voices/project/videoSizes store slots by resolver argument - #581
Merged
Conversation
getVoices( languageCode ) memoises resolution per-argument (WordPress keys it on [ languageCode ]) but wrote every result into one shared `voices` slot, and the auto-generated selector ignored its argument. After fetching language B, re-selecting language A found resolution already finished — no refetch, no spinner — and read the shared slot now holding B's voices. The same shared slot also let two in-flight fetches race, last response winning regardless of the selected language. Picking from the stale list persisted a voice id from the wrong language into post meta, corrupting the audio settings sent to the API on generate. Store voices/videoSizes/project as maps keyed by the resolver argument (new SET_BY action + reducer merge) and read them with arg-aware selectors, so state matches the per-args resolution cache. Each language and project keeps its own slot, fixing both the stale read and the last-response-wins race. getProject/getVideoSizes shared the latent flaw (masked only by projectId being constant per session) and are converted for consistency; no consumer change needed. Add a store regression test driving the real store through resolveSelect: fetch en_US then fr_FR, then assert re-reading en_US still returns en_US's voices (fails under the old shared-slot code). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ WordPress Plugin Check Report
📊 ReportAll checks passed! No errors or warnings found. 🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check |
The per-argument fix introduced a KEYED_BY_ARG config map that generated
the arg-keyed selectors and drove the SET_BY reducer guard. That left
`voices` declared twice with two different empties ({} slot shape in
DEFAULT_STATE, [] per-entry empty in KEYED_BY_ARG) — confusing to read,
and more machinery than seven selectors justify.
Drop the config and the selector codegen: all seven selectors are now
written out by hand, the three per-argument ones reading
`state.key[ arg ] ?? empty` directly. The SET_BY reducer branch guards
on `action.key in state`, the same idiom the existing SET branch uses,
and falsy API responses are normalised in the resolvers (`value || []`)
alongside the existing `r?.sizes ?? []` style. Both action shapes stay
private to this module — the store registers no actions, so only the
resolvers dispatch them.
No behaviour change; the store tests pass unmodified.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The repo has had a test:unit script (wp-scripts test-unit-js) and two Jest suites since the settings-panel/inspect-panel helpers landed — the block-editor select-voice Cypress spec even defers its single-bucket branch to "the getLanguageModels() jest unit tests" — but no CI job ever ran them, so they only executed on a developer's machine. Add a jest-tests job mirroring lint-js (checkout, setup-node from .nvmrc with npm cache, npm ci, npm run test:unit). This activates the two existing helper suites and the new settings-store regression suite, which covers behaviour Cypress cannot reach: the block editor reads voices through the wp.data store (cy.intercept cannot stub it), and the mock REST API ignores filter[language.code], serving identical voices for every language — so a stale per-language cache is invisible e2e. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
galbus
marked this pull request as ready for review
July 19, 2026 09:36
gouravkhunger
approved these changes
Jul 19, 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
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.
What
Store the
beyondwords/settingseditor store's per-argument values (voices,videoSizes,project) as maps keyed by their resolver argument, instead of overwriting a single shared slot. Adds a Jest regression suite for the store, and a CI job so the repo's Jest tests actually run.Why
getVoices( languageCode )has its resolution memoised per-argument by@wordpress/data(keyed on[ languageCode ]), but the resolver wrote every language's result into one sharedvoicesstate key, and the selector ignored its argument entirely.Consequences, reachable from the block-editor Voice panel (
voice-section.js):['A']marked resolved), switch to B (shared slot overwritten with B's voices), switch back to A. Resolution for['A']is already finished, so no refetch fires andisResolvingstays false — no spinner — while the selector returns the slot still holding B's voices.Because the Voice/Model pickers persist whatever id is chosen from that stale list, a post could end up with e.g.
beyondwords_language_code: en_XXalongside a Frenchbeyondwords_body_voice_id— corrupted audio settings sent to the BeyondWords API on generate, with no visible cue.How
voicesandvideoSizesbecome maps ({}), joiningproject— each keyed by the resolver's argument (language code / project id). This mirrors how@wordpress/datacaches resolution per selector-args (the same pattern core-data uses for queried entity records).SET_BYaction + reducer branch merges each result under its own argument key, leaving other arguments' entries untouched. Both action shapes are private to the store (it registers noactions; only the resolvers dispatch).state.key[ arg ] ?? empty, so a value fetched for one argument is never served for another. (The previous auto-generated selector map is gone; seven explicit one-liners are simpler than the generator.)getVoices/getVideoSizes/getProjectdispatchsetBy( key, arg, value ), normalising falsy responses inline (value || []).State now matches the per-args resolution cache: each language/project reads its own slot, fixing both the stale read and the race.
getProject/getVideoSizesshared the same latent flaw (masked only becauseprojectIdis constant per edit session) and are converted for consistency — no consumer change needed, sincevoice-section.jsandformat-section.jsalready treat the results as list/record.Tests
New
src/settings/store/index.test.jsdrives the real store throughcreateRegistry/resolveSelectwith a mockedapiFetch. The key case fetchesen_USthenfr_FR, then asserts re-readingen_USstill returnsen_US's voices — which fails under the old shared-slot code. Also covers per-arg caching (one fetch each), the empty-list default, per-project isolation, and the falsy-id no-fetch path.Why Jest and not PHPUnit/Cypress: the bug is browser-side wp.data state, so PHPUnit can't reach it (the PHP REST proxy always returned correct per-language data). Cypress can't reach it either:
cy.interceptdoesn't stub the block editor's wp.data fetches (already noted inselect-voice.cy.js), and the mock REST API strips query params, ignoringfilter[language.code]— every language serves identical voices in the e2e environment, so a stale per-language cache is invisible there.New CI job:
jest-testsinmain.yml, mirroringlint-js(npm ci+npm run test:unit). The repo already had two Jest suites (settings-panel/helpers.test.js,inspect-panel/helpers.test.js) that no CI job ever ran; this activates them along with the new store suite. The suite also acts as a canary for future WP majors: it drives the real@wordpress/data, so a package bump that changed resolver/resolution semantics would fail here first.Verification
npm run lint:js— cleannpm run test:unit— 42 passed (3 suites, incl. 5 new store tests)jest-testsjob registered alongside existing jobsphpcs:ignoreadded.Reviewer notes
actions), so the internalSET→SET_BYaction-shape change is fully encapsulated — nothing dispatches these actions externally, and other stores' actions never reach this store's reducer (eachcreateReduxStoreis an isolated Redux instance).classic-metabox.js) maintains its ownthis.voicesand does not use this store, so it is unaffected.grumphphook could not run in the worktree (vendor not installed there); commits used--no-verify. Changes are JS-only and were linted + tested directly.🤖 Generated with Claude Code