diff --git a/src/index.js b/src/index.js index 03048c2..3bbeb4a 100644 --- a/src/index.js +++ b/src/index.js @@ -60,6 +60,40 @@ addFilter( addQueryLoopAttributes ); +/** + * Derive a numeric seed for generated query IDs from the current post ID. + * + * In the post editor `getCurrentPostId()` returns a numeric post ID, but in the + * site editor it returns the template's string id (`theme//archive-opinion`), + * for which `postId * 1000` is NaN. NaN poisons every comparison downstream: + * `NaN !== NaN` is always true, so the id would be rewritten on every render, + * and `[ NaN ].includes( NaN )` is true, so the dedupe loop below would spin + * forever and hang the editor. + * + * Non-numeric ids are hashed to a stable integer instead. Stability matters: a + * seed that changed between loads would rewrite ids on open and reintroduce the + * false "unsaved changes" warning this HOC exists to avoid. + * + * @param {number|string} postId Current post or template ID. + * @return {number} A finite, stable numeric seed. + */ +const getQueryIdSeed = ( postId ) => { + const numericId = Number( postId ); + + if ( Number.isFinite( numericId ) ) { + return numericId; + } + + let hash = 0; + const key = String( postId ); + for ( let i = 0; i < key.length; i++ ) { + // Bounded each round so the seed stays a small, exact integer. + hash = ( hash * 31 + key.charCodeAt( i ) ) % 100000; + } + + return hash; +}; + /** * Ensure each Query Loop block has a unique queryId attribute. * @@ -116,11 +150,12 @@ const withUniqueQueryId = createHigherOrderComponent( ( BlockEdit ) => { } // Generate a unique id, skipping any already used by another - // query block on the page. + // query block on the page. The seed is always finite, which is + // what guarantees this loop terminates. const otherIds = allQueryBlocks .filter( ( id ) => id !== clientId ) .map( idOf ); - let candidate = postId * 1000 + index + 1; + let candidate = getQueryIdSeed( postId ) * 1000 + index + 1; while ( otherIds.includes( candidate ) ) { candidate++; } diff --git a/tests/e2e/unique-query-id.spec.js b/tests/e2e/unique-query-id.spec.js index 60b1003..7b03bcc 100644 --- a/tests/e2e/unique-query-id.spec.js +++ b/tests/e2e/unique-query-id.spec.js @@ -382,4 +382,85 @@ test.describe( 'Unique Query IDs', () => { const uniqueTitles = [ ...new Set( allPostTitles ) ]; expect( uniqueTitles ).toHaveLength( 20 ); } ); + + test( 'should assign finite, unique queryIds in the site editor', async ( { + page, + editor, + blockEditor, + } ) => { + // The site editor's "post ID" is the template's string id + // (`theme//slug`), not a number. Generating ids from it without + // coercing to a number yields NaN, and because `NaN !== NaN` and + // `[ NaN ].includes( NaN )` is true, that previously rewrote the + // attribute on every render and hung the editor in the dedupe loop. + await blockEditor.visitSiteEditor( 'index' ); + + const templateId = await page.evaluate( () => + window.wp.data.select( 'core/editor' ).getCurrentPostId() + ); + + // Guard the premise of this test: if core ever starts handing the site + // editor a numeric id, this test would silently stop covering the bug. + expect( typeof templateId ).toBe( 'string' ); + + // Two query blocks sharing a queryId. The first keeps its id; the + // second is a duplicate, so it takes the id-generation path. + await editor.setContent( + ` +
+ +
+ + + +
+ +
+` + ); + + // Let the HOC settle. A regression hangs the editor here, so the + // evaluate below times out rather than returning a wrong value. + await page.waitForTimeout( 2000 ); + + const queryIds = await page.evaluate( () => { + const { select } = window.wp.data; + return select( 'core/block-editor' ) + .getBlocksByName( 'core/query' ) + .map( + ( clientId ) => + select( 'core/block-editor' ).getBlockAttributes( + clientId + ).queryId + ); + } ); + + expect( queryIds ).toHaveLength( 2 ); + + // The regression assertion: NaN is a number but not a finite one, so + // `typeof` alone would not have caught it. + for ( const queryId of queryIds ) { + expect( Number.isFinite( queryId ) ).toBe( true ); + } + + expect( [ ...new Set( queryIds ) ] ).toHaveLength( 2 ); + + // Ids must also be stable once assigned — an id that keeps changing + // marks the template dirty on every render. + await page.waitForTimeout( 1000 ); + + const settledQueryIds = await page.evaluate( () => { + const { select } = window.wp.data; + return select( 'core/block-editor' ) + .getBlocksByName( 'core/query' ) + .map( + ( clientId ) => + select( 'core/block-editor' ).getBlockAttributes( + clientId + ).queryId + ); + } ); + + expect( settledQueryIds ).toEqual( queryIds ); + } ); } );