From 37a7cee83cc6fe5b29b72f93de718c85cf469242 Mon Sep 17 00:00:00 2001 From: michelhm Date: Tue, 21 Jul 2026 17:37:19 +0200 Subject: [PATCH] Fix editor hang from NaN query ID seed in the site editor withUniqueQueryId seeds generated ids with `postId * 1000`, where postId comes from `getCurrentPostId()`. In the post editor that is a numeric post ID, but in the site editor it is the template's string id (`theme//archive-opinion`), so the seed is NaN. NaN then poisons both comparisons the HOC relies on: - `NaN !== NaN` is always true, so the useEffect rewrote queryId on every render, marking the template dirty. - `[ NaN ].includes( NaN )` is true (includes uses SameValueZero, unlike indexOf) and `NaN++` stays NaN, so once any block on the page held a NaN queryId, the dedupe loop added in 64d4c9f never terminated and hung the editor. Any template with two or more query blocks whose ids are missing or duplicated reaches that loop, which is how it surfaced: block themes commonly carry patterns with hardcoded low queryIds alongside templates that omit them. Coerce the seed to a finite number, hashing non-numeric ids into a stable integer. Stability matters as much as finiteness here: a seed that changed between loads would rewrite ids on open and reintroduce the false "unsaved changes" warning 64d4c9f fixed. Post editor behaviour is unchanged, and a finite seed is what guarantees the dedupe loop terminates. The existing queryId tests all run in the post editor, so none covered the string-id path. Add a site editor test asserting ids are finite, unique and stable; it asserts `typeof templateId === 'string'` up front so it fails loudly rather than silently stops covering the bug if core ever returns a numeric id. Number.isFinite is the operative check, since typeof NaN is 'number'. --- src/index.js | 39 ++++++++++++++- tests/e2e/unique-query-id.spec.js | 81 +++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) 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 ); + } ); } );