Merge pull request #23258 from Yoast/209-html-parser---investigate-when-to-build-the-tree

refactor: optimize HTML tree building to reuse existing trees

Author: JorPV

packages/js/src/initializers/analysis.js

@@ -101,6 +101,9 @@ export function collectData() {
 		...data,
 		textTitle: getEditorDataTitle(),
 		isFrontPage: getIsFrontPage(),
+		shortcodes: Array.isArray( window.wpseoScriptData.analysis.plugins.shortcodes?.wpseo_shortcode_tags )
+			? window.wpseoScriptData.analysis.plugins.shortcodes.wpseo_shortcode_tags
+			: [],
 	};
 
 	const analysisData = applyAnalysisModifications( data );

packages/js/tests/initializers/analysis.test.js

@@ -0,0 +1,80 @@
+/**
+ * Tests for the shortcode-extraction behaviour of `collectData` in
+ * src/initializers/analysis.js. `collectData` is the main-thread data source the Insights panel
+ * uses for its own analysis, so the shortcodes it surfaces must match what the analyze cycle is
+ * told about — otherwise the Insights and assessment results can drift on shortcode-rich posts.
+ */
+
+jest.mock( "@wordpress/data", () => ( {
+	select: jest.fn(),
+	dispatch: jest.fn(),
+	subscribe: jest.fn(),
+	combineReducers: jest.requireActual( "@wordpress/data" ).combineReducers,
+} ) );
+
+jest.mock( "@wordpress/hooks", () => ( {
+	applyFilters: ( hook, value ) => value,
+	doAction: jest.fn(),
+} ) );
+
+jest.mock( "../../src/initializers/pluggable", () => ( {
+	applyModifications: ( modification, value ) => value,
+} ) );
+
+import { select } from "@wordpress/data";
+import { collectData } from "../../src/initializers/analysis";
+
+const originalWpseoScriptData = window.wpseoScriptData;
+
+describe( "collectData — shortcodes extraction", () => {
+	beforeEach( () => {
+		select.mockImplementation( () => ( {
+			getAnalysisData: () => ( { text: "", title: "", description: "" } ),
+			getEditorDataTitle: () => "",
+			getIsFrontPage: () => false,
+		} ) );
+	} );
+
+	afterAll( () => {
+		window.wpseoScriptData = originalWpseoScriptData;
+	} );
+
+	it( "passes the wpseo_shortcode_tags array through when it is present", () => {
+		window.wpseoScriptData = {
+			// eslint-disable-next-line camelcase
+			analysis: { plugins: { shortcodes: { wpseo_shortcode_tags: [ "gallery", "caption" ] } } },
+		};
+
+		expect( collectData().shortcodes ).toEqual( [ "gallery", "caption" ] );
+	} );
+
+	it( "falls back to an empty array when plugins.shortcodes is missing", () => {
+		window.wpseoScriptData = { analysis: { plugins: {} } };
+
+		expect( collectData().shortcodes ).toEqual( [] );
+	} );
+
+	it( "falls back to an empty array when wpseo_shortcode_tags is missing on the shortcodes object", () => {
+		window.wpseoScriptData = { analysis: { plugins: { shortcodes: {} } } };
+
+		expect( collectData().shortcodes ).toEqual( [] );
+	} );
+
+	it( "falls back to an empty array when wpseo_shortcode_tags is null", () => {
+		window.wpseoScriptData = {
+			// eslint-disable-next-line camelcase
+			analysis: { plugins: { shortcodes: { wpseo_shortcode_tags: null } } },
+		};
+
+		expect( collectData().shortcodes ).toEqual( [] );
+	} );
+
+	it( "falls back to an empty array when wpseo_shortcode_tags is a non-array value", () => {
+		window.wpseoScriptData = {
+			// eslint-disable-next-line camelcase
+			analysis: { plugins: { shortcodes: { wpseo_shortcode_tags: { gallery: true } } } },
+		};
+
+		expect( collectData().shortcodes ).toEqual( [] );
+	} );
+} );

packages/yoastseo/spec/languageProcessing/researches/keyphraseDistributionSpec.js

@@ -13,6 +13,7 @@ import JapaneseResearcher from "../../../src/languageProcessing/languages/ja/Res
 import getMorphologyData from "../../specHelpers/getMorphologyData";
 import { realWorldULExample1, realWorldULExample2 } from "../helpers/sanitize/mergeListItemsSpec";
 import buildTree from "../../specHelpers/parse/buildTree";
+import getSentencesFromTree from "../../../src/languageProcessing/helpers/sentence/getSentencesFromTree";
 import { primeLanguageSpecificData } from "../../../src/languageProcessing/helpers/morphology/buildTopicStems";
 
 const morphologyData = getMorphologyData( "en" );
@@ -1460,6 +1461,71 @@ describe( "Test for the research", function() {
 		expect( keyphraseDistributionResearcher( paperWithList, researcherListCondition ).keyphraseDistractionPercentage ).toEqual(
 			keyphraseDistributionResearcher( paperWithWords, researcherWordsCondition ).keyphraseDistractionPercentage );
 	} );
+
+	describe( "leaves no sentenceParentNode back-references on the tree", () => {
+		// The research uses getSentencesFromTree( tree, true ) which sets `sentence.sentenceParentNode = parentNode`
+		// on every sentence in the tree. The parent paragraph in turn holds the sentence in its `sentences` array,
+		// which creates a paragraph -> sentence -> sentenceParentNode -> paragraph cycle. When the tree is reused
+		// across calls (post tree-build dedup), that cycle escapes into downstream research/analyze results and
+		// blows Transporter.serialize. Assert the research cleans up after itself so the tree stays acyclic.
+		it( "strips sentenceParentNode from tree sentences after running on a paragraph-only paper", () => {
+			const paper = new Paper(
+				"<p>The keyphrase appears here. The keyphrase appears again.</p>" +
+				"<p>This is a distinct paragraph without the keyphrase. Another sentence.</p>",
+				{ locale: "en_US", keyword: "keyphrase" }
+			);
+			const researcher = new Researcher( paper );
+			buildTree( paper, researcher );
+			researcher.addResearchData( "morphology", morphologyData );
+
+			keyphraseDistributionResearcher( paper, researcher );
+
+			const sentences = getSentencesFromTree( paper.getTree() );
+			expect( sentences.length ).toBeGreaterThan( 0 );
+			sentences.forEach( sentence => {
+				expect( sentence.sentenceParentNode ).toBeUndefined();
+			} );
+		} );
+
+		it( "strips sentenceParentNode from tree sentences after running on a paper with list items", () => {
+			// The list-items code path merges sentences and writes sentenceParentNode (as an array) on the merged
+			// sentence as well. The tree's original sentences also carry the back-ref and must be cleaned up.
+			const paper = new Paper(
+				"<p>The keyphrase intro paragraph.</p>" +
+				"<ul><li>First list item with the keyphrase.</li><li>Second list item without it.</li></ul>",
+				{ locale: "en_US", keyword: "keyphrase" }
+			);
+			const researcher = new Researcher( paper );
+			buildTree( paper, researcher );
+			researcher.addResearchData( "morphology", morphologyData );
+
+			keyphraseDistributionResearcher( paper, researcher );
+
+			const sentences = getSentencesFromTree( paper.getTree() );
+			expect( sentences.length ).toBeGreaterThan( 0 );
+			sentences.forEach( sentence => {
+				expect( sentence.sentenceParentNode ).toBeUndefined();
+			} );
+		} );
+
+		it( "leaves the tree JSON-stringifiable (no cycle) after running", () => {
+			// JSON.stringify throws "Converting circular structure to JSON" on cycles, which is exactly the failure
+			// mode the bug produced downstream. A successful stringify is the strongest end-to-end guarantee that
+			// the cleanup actually broke the cycle, independent of which specific property held the back-reference.
+			const paper = new Paper(
+				"<p>The keyphrase appears here. The keyphrase appears again.</p>" +
+				"<ul><li>First list item with the keyphrase.</li><li>Second list item without it.</li></ul>",
+				{ locale: "en_US", keyword: "keyphrase" }
+			);
+			const researcher = new Researcher( paper );
+			buildTree( paper, researcher );
+			researcher.addResearchData( "morphology", morphologyData );
+
+			keyphraseDistributionResearcher( paper, researcher );
+
+			expect( () => JSON.stringify( paper.getTree() ) ).not.toThrow();
+		} );
+	} );
 } );
 
 describe( "a test for exact match of keyphrase in English", () => {

packages/yoastseo/spec/scoring/assessors/assessorSpec.js

@@ -134,6 +134,38 @@ describe( "an assessor object", function() {
 		} );
 	} );
 
+	describe( "tree building", function() {
+		it( "builds the HTML tree exactly once when the paper does not already have one", function() {
+			const paper = new Paper( "<p>Hello world.</p>" );
+			const assessor = new Assessor( new DefaultResearcher() );
+
+			expect( paper.getTree() ).toBeNull();
+			const setTreeSpy = jest.spyOn( paper, "setTree" );
+
+			assessor.assess( paper );
+
+			expect( setTreeSpy ).toHaveBeenCalledTimes( 1 );
+			expect( paper.getTree() ).not.toBeNull();
+		} );
+
+		it( "does not rebuild the HTML tree when the paper already has one", function() {
+			const paper = new Paper( "<p>Hello world.</p>" );
+			const assessor = new Assessor( new DefaultResearcher() );
+
+			// Populate the tree once via a first assess() call so it's a real, valid tree.
+			assessor.assess( paper );
+			const existingTree = paper.getTree();
+			expect( existingTree ).not.toBeNull();
+
+			const setTreeSpy = jest.spyOn( paper, "setTree" );
+
+			assessor.assess( paper );
+
+			expect( setTreeSpy ).not.toHaveBeenCalled();
+			expect( paper.getTree() ).toBe( existingTree );
+		} );
+	} );
+
 	describe( "hasMarker", function() {
 		let assessor;
 

packages/yoastseo/spec/values/PaperSpec.js

@@ -197,6 +197,68 @@ describe( "Paper", function() {
 		} );
 	} );
 
+	describe( "hasSameTreeInputsAs", function() {
+		it( "returns true for two papers with identical text and tree-relevant attributes", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>" );
+			const paper2 = new Paper( "<p>Hello world.</p>" );
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( true );
+		} );
+
+		it( "returns true when only non-tree attributes (keyword, title, description, …) differ", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>", { keyword: "alpha", title: "Title A", description: "Desc A" } );
+			const paper2 = new Paper( "<p>Hello world.</p>", { keyword: "beta",  title: "Title B", description: "Desc B" } );
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( true );
+		} );
+
+		it( "returns false when the text differs", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>" );
+			const paper2 = new Paper( "<p>Goodbye world.</p>" );
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( false );
+		} );
+
+		it( "returns false when the locale differs", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>", { locale: "en_US" } );
+			const paper2 = new Paper( "<p>Hello world.</p>", { locale: "nl_NL" } );
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( false );
+		} );
+
+		it( "returns false when the shortcodes list differs", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>", { shortcodes: [ "gallery" ] } );
+			const paper2 = new Paper( "<p>Hello world.</p>", { shortcodes: [ "gallery", "caption" ] } );
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( false );
+		} );
+
+		it( "returns false when the wpBlocks list differs", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>", { wpBlocks: [ { name: "core/paragraph" } ] } );
+			const paper2 = new Paper( "<p>Hello world.</p>", { wpBlocks: [ { name: "core/heading" } ] } );
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( false );
+		} );
+
+		it( "returns false when compared against a falsy value", function() {
+			const paper = new Paper( "<p>Hello world.</p>" );
+			expect( paper.hasSameTreeInputsAs( null ) ).toBe( false );
+		} );
+
+		it( "treats a missing shortcodes attribute as equivalent to an empty array", function() {
+			// A paper deserialized from a payload that omits `shortcodes` ends up with the attribute undefined,
+			// but it produces the same tree as a paper that explicitly carries shortcodes: [], so the predicate
+			// must not report them as different tree inputs.
+			const paper1 = new Paper( "<p>Hello world.</p>", { shortcodes: [] } );
+			const paper2 = new Paper( "<p>Hello world.</p>" );
+			delete paper2._attributes.shortcodes;
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( true );
+			expect( paper2.hasSameTreeInputsAs( paper1 ) ).toBe( true );
+		} );
+
+		it( "treats a missing wpBlocks attribute as equivalent to an empty array", function() {
+			const paper1 = new Paper( "<p>Hello world.</p>", { wpBlocks: [] } );
+			const paper2 = new Paper( "<p>Hello world.</p>" );
+			delete paper2._attributes.wpBlocks;
+			expect( paper1.hasSameTreeInputsAs( paper2 ) ).toBe( true );
+			expect( paper2.hasSameTreeInputsAs( paper1 ) ).toBe( true );
+		} );
+	} );
+
 	describe( "hasText", function() {
 		it( "should return true if contains raw text", function() {
 			const paper = new Paper( "This is a test" );