From f4b9af10bad17fe5a05fb0b2b7576a29053edc3e Mon Sep 17 00:00:00 2001 From: Stuart McAlpine Date: Sun, 12 Jul 2026 06:10:01 +0100 Subject: [PATCH 1/3] fix: key voices/project/videoSizes store slots by resolver argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/settings/store/index.js | 72 ++++++++++++++---- src/settings/store/index.test.js | 125 +++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 14 deletions(-) create mode 100644 src/settings/store/index.test.js diff --git a/src/settings/store/index.js b/src/settings/store/index.js index ec6eb305..0b8e3918 100644 --- a/src/settings/store/index.js +++ b/src/settings/store/index.js @@ -4,14 +4,31 @@ import apiFetch from '@wordpress/api-fetch'; import { createReduxStore } from '@wordpress/data'; -// Shared editor state. Adding a key here gives you a `getKey` selector for -// free; write a resolver below that returns `set( 'key', value )`. +// Shared editor state. Two shapes of key live here: +// +// * Plain keys hold one session-wide value. Adding one gives you a `getKey` +// selector for free; its resolver returns `set( 'key', value )`. +// * Per-argument keys (listed in `KEYED_BY_ARG`) hold a map from a resolver +// argument to its result — e.g. voices per language code. @wordpress/data +// memoises resolution per-args, so the *stored* value has to be per-args +// too. A single shared slot would let a later fetch (another language or +// project) overwrite it while the already-"resolved" selector keeps serving +// the wrong list. Their resolvers return `setBy( 'key', arg, value )` and +// they get a hand-written, arg-reading selector below. export const DEFAULT_STATE = { settings: {}, languages: [], - voices: [], + voices: {}, scriptTemplates: [], videoTemplates: [], + videoSizes: {}, + project: {}, +}; + +// Per-argument keys mapped to the value their selector returns for an argument +// that has not been fetched yet (voices/videoSizes are lists, project a record). +const KEYED_BY_ARG = { + voices: [], videoSizes: [], project: {}, }; @@ -21,17 +38,44 @@ const reducer = ( state = DEFAULT_STATE, action ) => { const empty = Array.isArray( DEFAULT_STATE[ action.key ] ) ? [] : {}; return { ...state, [ action.key ]: action.value || empty }; } + // Merge a per-argument result into its key's map, leaving other arguments' + // entries untouched so each language/project keeps its own value. + if ( action.type === 'SET_BY' && action.key in KEYED_BY_ARG ) { + return { + ...state, + [ action.key ]: { + ...state[ action.key ], + [ action.arg ]: action.value || KEYED_BY_ARG[ action.key ], + }, + }; + } return state; }; -const selectors = Object.fromEntries( - Object.keys( DEFAULT_STATE ).map( ( key ) => [ - `get${ key[ 0 ].toUpperCase() }${ key.slice( 1 ) }`, - ( state ) => state[ key ], - ] ) -); +const getterName = ( key ) => + `get${ key[ 0 ].toUpperCase() }${ key.slice( 1 ) }`; + +const selectors = { + // Plain keys read the whole slot: `getKey( state ) => state.key`. + ...Object.fromEntries( + Object.keys( DEFAULT_STATE ) + .filter( ( key ) => ! ( key in KEYED_BY_ARG ) ) + .map( ( key ) => [ getterName( key ), ( state ) => state[ key ] ] ) + ), + // Per-argument keys read only their own entry: `getKey( state, arg ) => + // state.key[ arg ] ?? empty`. Reading by the argument keeps the selector in + // step with @wordpress/data's per-args resolution cache, so a value fetched + // for one argument is never served for another. + ...Object.fromEntries( + Object.entries( KEYED_BY_ARG ).map( ( [ key, empty ] ) => [ + getterName( key ), + ( state, arg ) => state[ key ][ arg ] ?? empty, + ] ) + ), +}; const set = ( key, value ) => ( { type: 'SET', key, value } ); +const setBy = ( key, arg, value ) => ( { type: 'SET_BY', key, arg, value } ); const resolvers = { async getSettings() { @@ -46,7 +90,7 @@ const resolvers = { const value = await apiFetch( { path: `/beyondwords/v1/languages/${ languageCode }/voices`, } ); - return set( 'voices', value ); + return setBy( 'voices', languageCode, value ); }, async getScriptTemplates() { const value = await apiFetch( { @@ -62,23 +106,23 @@ const resolvers = { }, async getVideoSizes( projectId ) { if ( ! projectId ) { - return set( 'videoSizes', [] ); + return setBy( 'videoSizes', projectId, [] ); } const r = await apiFetch( { path: `/beyondwords/v1/projects/${ projectId }/video-settings`, } ); - return set( 'videoSizes', r?.sizes ?? [] ); + return setBy( 'videoSizes', projectId, r?.sizes ?? [] ); }, // The project carries the default `language` used to pre-select the Language // dropdown when Customize is enabled. Fetched on demand (behind the toggle). async getProject( projectId ) { if ( ! projectId ) { - return set( 'project', {} ); + return setBy( 'project', projectId, {} ); } const value = await apiFetch( { path: `/beyondwords/v1/projects/${ projectId }`, } ); - return set( 'project', value ); + return setBy( 'project', projectId, value ); }, }; diff --git a/src/settings/store/index.test.js b/src/settings/store/index.test.js new file mode 100644 index 00000000..d60a74bb --- /dev/null +++ b/src/settings/store/index.test.js @@ -0,0 +1,125 @@ +/* global jest, describe, it, expect, beforeEach */ + +/** + * WordPress dependencies + */ +import apiFetch from '@wordpress/api-fetch'; +import { createRegistry } from '@wordpress/data'; + +/** + * Internal dependencies + */ +import store from './index'; + +jest.mock( '@wordpress/api-fetch' ); + +const STORE = 'beyondwords/settings'; + +// Distinct data per resolver argument, so a value leaking from one argument's +// fetch into another's slot is observable in the assertions below. +const VOICES = { + en_US: [ { id: 1, name: 'Amy' } ], + fr_FR: [ { id: 2, name: 'Céline' } ], +}; + +const PROJECT = { + 11111: { id: 11111, language: 'en_US' }, + 22222: { id: 22222, language: 'fr_FR' }, +}; + +const VIDEO_SIZES = { + 11111: [ { name: 'square', enabled: true } ], + 22222: [ { name: 'portrait', enabled: true } ], +}; + +describe( 'beyondwords/settings store', () => { + let registry; + + beforeEach( () => { + apiFetch.mockReset(); + apiFetch.mockImplementation( ( { path } ) => { + let m = path.match( /languages\/([^/]+)\/voices$/ ); + if ( m ) { + return Promise.resolve( VOICES[ m[ 1 ] ] ?? [] ); + } + m = path.match( /projects\/([^/]+)\/video-settings$/ ); + if ( m ) { + return Promise.resolve( { + sizes: VIDEO_SIZES[ m[ 1 ] ] ?? [], + } ); + } + m = path.match( /projects\/([^/]+)$/ ); + if ( m ) { + return Promise.resolve( PROJECT[ m[ 1 ] ] ?? {} ); + } + return Promise.resolve( [] ); + } ); + + registry = createRegistry(); + registry.register( store ); + } ); + + describe( 'getVoices', () => { + it( 'keeps each language in its own slot after another is fetched', async () => { + const resolve = registry.resolveSelect( STORE ); + + // Fetch A, then B. With the old single shared `voices` slot, B's + // response overwrote the value A's already-finished resolution read. + expect( await resolve.getVoices( 'en_US' ) ).toEqual( + VOICES.en_US + ); + expect( await resolve.getVoices( 'fr_FR' ) ).toEqual( + VOICES.fr_FR + ); + + // Re-reading A must still return A's voices, not B's — no refetch is + // dispatched because resolution for [ 'en_US' ] is already finished. + const select = registry.select( STORE ); + expect( select.getVoices( 'en_US' ) ).toEqual( VOICES.en_US ); + expect( select.getVoices( 'fr_FR' ) ).toEqual( VOICES.fr_FR ); + } ); + + it( 'resolves each language once and reuses the cached result', async () => { + const resolve = registry.resolveSelect( STORE ); + await resolve.getVoices( 'en_US' ); + await resolve.getVoices( 'en_US' ); + await resolve.getVoices( 'fr_FR' ); + + // en_US fetched once (second call is a cache hit), fr_FR once. + expect( apiFetch ).toHaveBeenCalledTimes( 2 ); + } ); + + it( 'returns an empty list for a language that has not been fetched', () => { + expect( registry.select( STORE ).getVoices( 'de_DE' ) ).toEqual( + [] + ); + } ); + } ); + + describe( 'getProject / getVideoSizes', () => { + it( 'keeps each project id in its own slot', async () => { + const resolve = registry.resolveSelect( STORE ); + await resolve.getProject( '11111' ); + await resolve.getProject( '22222' ); + await resolve.getVideoSizes( '11111' ); + await resolve.getVideoSizes( '22222' ); + + const select = registry.select( STORE ); + expect( select.getProject( '11111' ) ).toEqual( PROJECT[ 11111 ] ); + expect( select.getProject( '22222' ) ).toEqual( PROJECT[ 22222 ] ); + expect( select.getVideoSizes( '11111' ) ).toEqual( + VIDEO_SIZES[ 11111 ] + ); + expect( select.getVideoSizes( '22222' ) ).toEqual( + VIDEO_SIZES[ 22222 ] + ); + } ); + + it( 'defaults to {} / [] and skips the fetch for a falsy id', async () => { + const resolve = registry.resolveSelect( STORE ); + expect( await resolve.getProject( '' ) ).toEqual( {} ); + expect( await resolve.getVideoSizes( '' ) ).toEqual( [] ); + expect( apiFetch ).not.toHaveBeenCalled(); + } ); + } ); +} ); From f5cff376a06d05a8c6bf6a429eb954689e5f1644 Mon Sep 17 00:00:00 2001 From: Stuart McAlpine Date: Sun, 19 Jul 2026 09:23:17 +0100 Subject: [PATCH 2/3] refactor: hand-write the settings store selectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/settings/store/index.js | 75 ++++++++++++++----------------------- 1 file changed, 28 insertions(+), 47 deletions(-) diff --git a/src/settings/store/index.js b/src/settings/store/index.js index 0b8e3918..83bad5e5 100644 --- a/src/settings/store/index.js +++ b/src/settings/store/index.js @@ -4,76 +4,57 @@ import apiFetch from '@wordpress/api-fetch'; import { createReduxStore } from '@wordpress/data'; -// Shared editor state. Two shapes of key live here: -// -// * Plain keys hold one session-wide value. Adding one gives you a `getKey` -// selector for free; its resolver returns `set( 'key', value )`. -// * Per-argument keys (listed in `KEYED_BY_ARG`) hold a map from a resolver -// argument to its result — e.g. voices per language code. @wordpress/data -// memoises resolution per-args, so the *stored* value has to be per-args -// too. A single shared slot would let a later fetch (another language or -// project) overwrite it while the already-"resolved" selector keeps serving -// the wrong list. Their resolvers return `setBy( 'key', arg, value )` and -// they get a hand-written, arg-reading selector below. +// Shared editor state, fetched lazily by the resolvers below; each key has a +// matching selector. Most keys hold a single session-wide value. `voices`, +// `videoSizes` and `project` are maps keyed by their resolver's argument: +// @wordpress/data marks resolution finished per selector-args, so each +// argument's result needs its own slot — with one shared slot, a later fetch +// (say, another language's voices) would overwrite it while the resolution +// cache keeps reporting the overwritten entry as fresh. export const DEFAULT_STATE = { settings: {}, languages: [], - voices: {}, + voices: {}, // languageCode → voice list scriptTemplates: [], videoTemplates: [], - videoSizes: {}, - project: {}, -}; - -// Per-argument keys mapped to the value their selector returns for an argument -// that has not been fetched yet (voices/videoSizes are lists, project a record). -const KEYED_BY_ARG = { - voices: [], - videoSizes: [], - project: {}, + videoSizes: {}, // projectId → size list + project: {}, // projectId → project record }; const reducer = ( state = DEFAULT_STATE, action ) => { + // Replace a session-wide key. if ( action.type === 'SET' && action.key in state ) { const empty = Array.isArray( DEFAULT_STATE[ action.key ] ) ? [] : {}; return { ...state, [ action.key ]: action.value || empty }; } - // Merge a per-argument result into its key's map, leaving other arguments' - // entries untouched so each language/project keeps its own value. - if ( action.type === 'SET_BY' && action.key in KEYED_BY_ARG ) { + // Merge one argument's result into a per-argument key, leaving the other + // arguments' entries untouched. + if ( action.type === 'SET_BY' && action.key in state ) { return { ...state, [ action.key ]: { ...state[ action.key ], - [ action.arg ]: action.value || KEYED_BY_ARG[ action.key ], + [ action.arg ]: action.value, }, }; } return state; }; -const getterName = ( key ) => - `get${ key[ 0 ].toUpperCase() }${ key.slice( 1 ) }`; - +// The per-argument selectors (voices, video sizes, project) read the entry for +// the requested argument only, with an empty value while it is unfetched. const selectors = { - // Plain keys read the whole slot: `getKey( state ) => state.key`. - ...Object.fromEntries( - Object.keys( DEFAULT_STATE ) - .filter( ( key ) => ! ( key in KEYED_BY_ARG ) ) - .map( ( key ) => [ getterName( key ), ( state ) => state[ key ] ] ) - ), - // Per-argument keys read only their own entry: `getKey( state, arg ) => - // state.key[ arg ] ?? empty`. Reading by the argument keeps the selector in - // step with @wordpress/data's per-args resolution cache, so a value fetched - // for one argument is never served for another. - ...Object.fromEntries( - Object.entries( KEYED_BY_ARG ).map( ( [ key, empty ] ) => [ - getterName( key ), - ( state, arg ) => state[ key ][ arg ] ?? empty, - ] ) - ), + getSettings: ( state ) => state.settings, + getLanguages: ( state ) => state.languages, + getVoices: ( state, languageCode ) => state.voices[ languageCode ] ?? [], + getScriptTemplates: ( state ) => state.scriptTemplates, + getVideoTemplates: ( state ) => state.videoTemplates, + getVideoSizes: ( state, projectId ) => state.videoSizes[ projectId ] ?? [], + getProject: ( state, projectId ) => state.project[ projectId ] ?? {}, }; +// Both action shapes are private to this store — it registers no `actions`, +// so the resolvers below are the only dispatchers. const set = ( key, value ) => ( { type: 'SET', key, value } ); const setBy = ( key, arg, value ) => ( { type: 'SET_BY', key, arg, value } ); @@ -90,7 +71,7 @@ const resolvers = { const value = await apiFetch( { path: `/beyondwords/v1/languages/${ languageCode }/voices`, } ); - return setBy( 'voices', languageCode, value ); + return setBy( 'voices', languageCode, value || [] ); }, async getScriptTemplates() { const value = await apiFetch( { @@ -122,7 +103,7 @@ const resolvers = { const value = await apiFetch( { path: `/beyondwords/v1/projects/${ projectId }`, } ); - return setBy( 'project', projectId, value ); + return setBy( 'project', projectId, value || {} ); }, }; From 23daebfc866ee2c97d9758def29d075e81638004 Mon Sep 17 00:00:00 2001 From: Stuart McAlpine Date: Sun, 19 Jul 2026 09:58:40 +0100 Subject: [PATCH 3/3] ci: run Jest unit tests in the workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/main.yml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bef21e6a..1e5e9bb7 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -239,6 +239,25 @@ jobs: - name: Lint JS run: npm run lint:js + # ###################################### + # Jest unit tests + # ###################################### + jest-tests: + name: Jest + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@v6 + + - uses: actions/setup-node@v6 + with: + node-version-file: '.nvmrc' + cache: npm + + - run: npm ci + + - name: Run Jest unit tests + run: npm run test:unit + # ###################################### # Plugin Check # ######################################