From 467057e5b69bc6b0bfc56c4790771ed45ae6fc01 Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Thu, 7 May 2026 15:26:45 +0100 Subject: [PATCH 1/2] fix: restore shape ID counter before createPresentation in restorePresentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a handler is recompiled (new handler registered), ESM module-level variables reset to initial values. The shape ID counter in ooxml-core resets to 1. Previously, restorePresentation called createPresentation first, then set the counter — but any shapes created between those two calls would get IDs starting from 1, colliding with restored slides. Fix: set the shape ID counter FIRST, before createPresentation, so all subsequent shape creation uses the correct counter value. Also adds a fallback for older serialized presentations that lack shapeIdCounter: scans all restored slides for the maximum existing shape ID and sets the counter to that value, preventing collisions even without explicit counter tracking. Signed-off-by: Simon Davies --- builtin-modules/pptx.json | 2 +- builtin-modules/src/pptx.ts | 26 ++++++++++++++++++++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/builtin-modules/pptx.json b/builtin-modules/pptx.json index b7c38f1..e0129d8 100644 --- a/builtin-modules/pptx.json +++ b/builtin-modules/pptx.json @@ -3,7 +3,7 @@ "description": "PowerPoint PPTX presentation builder - slides, text, shapes, themes, layouts", "author": "system", "mutable": false, - "sourceHash": "sha256:0139928e7286a4f7", + "sourceHash": "sha256:b88d325d9db848b0", "dtsHash": "sha256:0e08c6d6b51b36e8", "importStyle": "named", "hints": { diff --git a/builtin-modules/src/pptx.ts b/builtin-modules/src/pptx.ts index 27ef2b6..7a48689 100644 --- a/builtin-modules/src/pptx.ts +++ b/builtin-modules/src/pptx.ts @@ -5869,6 +5869,26 @@ export function restorePresentation(state: SerializedPresentation): Pres { ); } + // Restore shape ID counter FIRST — before createPresentation or any other + // function that might generate shapes. This prevents ID collisions when + // the module's global counter has been reset (e.g., handler recompilation + // resets ESM module-level variables to initial values). + if (typeof state.shapeIdCounter === "number") { + setShapeIdCounter(state.shapeIdCounter); + } else if (state.slides && Array.isArray(state.slides)) { + // No saved counter — scan existing slides to find the max shape ID + // so new shapes don't collide with restored ones. + let maxId = 1; + for (const slide of state.slides) { + const idMatches = slide.shapes.matchAll(/ maxId) maxId = id; + } + } + setShapeIdCounter(maxId); + } + // Recreate the presentation with the same options const pres = createPresentation({ theme: state.themeName, @@ -5899,12 +5919,6 @@ export function restorePresentation(state: SerializedPresentation): Pres { pres._chartEntries = state.chartEntries; } - // Restore shape ID counter to prevent duplicate IDs when adding new shapes - // This is critical for addSlideNumbers/addFooter called after restore - if (typeof state.shapeIdCounter === "number") { - setShapeIdCounter(state.shapeIdCounter); - } - return pres; } From 9ad65a749e76d07a48d991b0d376ddd4a24614ba Mon Sep 17 00:00:00 2001 From: Simon Davies Date: Thu, 7 May 2026 16:06:53 +0100 Subject: [PATCH 2/2] fix: address PR #113 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Validate shapeIdCounter with Number.isFinite + Number.isInteger before passing to setShapeIdCounter (rejects NaN/Infinity/floats) - Extract shared SHAPE_ID_REGEX constant and _extractShapeIds helper used by both _validatePresentation and restorePresentation - Add 2 tests for shape ID continuity across serialize/restore: 1. Normal flow: serialize → restore → addSlideNumbers → buildZip 2. Legacy fallback: missing shapeIdCounter → scan slides for max ID Signed-off-by: Simon Davies --- builtin-modules/pptx.json | 2 +- builtin-modules/src/pptx.ts | 76 +++++++++++++++++++++++++---------- tests/pptx-validation.test.ts | 41 +++++++++++++++++++ 3 files changed, 96 insertions(+), 23 deletions(-) diff --git a/builtin-modules/pptx.json b/builtin-modules/pptx.json index e0129d8..38ed304 100644 --- a/builtin-modules/pptx.json +++ b/builtin-modules/pptx.json @@ -3,7 +3,7 @@ "description": "PowerPoint PPTX presentation builder - slides, text, shapes, themes, layouts", "author": "system", "mutable": false, - "sourceHash": "sha256:b88d325d9db848b0", + "sourceHash": "sha256:b9ec89c9f86fc4b5", "dtsHash": "sha256:0e08c6d6b51b36e8", "importStyle": "named", "hints": { diff --git a/builtin-modules/src/pptx.ts b/builtin-modules/src/pptx.ts index 7a48689..9970c2a 100644 --- a/builtin-modules/src/pptx.ts +++ b/builtin-modules/src/pptx.ts @@ -3916,7 +3916,9 @@ function textFitsInBox( * @param items - Array of shape XML strings or objects with toString() * @returns Combined XML string */ -export function shapes(items: Array): ShapeFragment { +export function shapes( + items: Array, +): ShapeFragment { if (!Array.isArray(items)) { throw new Error( `shapes(): expected an array of ShapeFragment items, but got ${typeof items}. ` + @@ -4389,6 +4391,25 @@ export interface ValidationResult { * @returns ValidationResult with any issues found * @internal */ + +/** Regex to extract shape IDs from OOXML slide shapes XML. + * Used by both validation (duplicate detection) and restore (max ID scan). */ +const SHAPE_ID_REGEX = /(); - for (const m of idMatches) { - const id = m[1]; - if (slideIds.has(id)) { + for (const id of slideShapeIds) { + const idStr = String(id); + if (slideIds.has(idStr)) { errors.push({ code: "PPTX_DUPLICATE_SHAPE_ID", severity: "error", @@ -4449,15 +4474,16 @@ function _validatePresentation( hint: "Each shape must have a unique ID. This usually means shapes were copy-pasted incorrectly.", }); } - slideIds.add(id); + slideIds.add(idStr); // Track globally too - if (!globalShapeIds.has(id)) globalShapeIds.set(id, []); - globalShapeIds.get(id)!.push(i); + if (!globalShapeIds.has(idStr)) globalShapeIds.set(idStr, []); + globalShapeIds.get(idStr)!.push(i); } // A4: Check balanced required tags for (const tag of ["p:sp", "p:pic", "p:graphicFrame", "p:cxnSp"]) { - const opens = (shapes.match(new RegExp(`<${tag}[\\s>]`, "g")) || []).length; + const opens = (shapes.match(new RegExp(`<${tag}[\\s>]`, "g")) || []) + .length; const closes = (shapes.match(new RegExp(``, "g")) || []).length; if (opens !== closes) { errors.push({ @@ -4521,19 +4547,20 @@ function _validatePresentation( errors.push({ code: "PPTX_CHART_MISSING_ROOT", severity: "error", - message: "Chart XML missing required or elements.", + message: + "Chart XML missing required or elements.", part: entry.name, hint: "Regenerate the chart using barChart/pieChart/lineChart/comboChart.", }); } // B3b: Axis ID/crossAx consistency (for bar/line/combo charts) - const axIds = [...xml.matchAll(//g)].map((m) => - m[1], + const axIds = [...xml.matchAll(//g)].map( + (m) => m[1], + ); + const crossAxIds = [...xml.matchAll(//g)].map( + (m) => m[1], ); - const crossAxIds = [ - ...xml.matchAll(//g), - ].map((m) => m[1]); for (const crossId of crossAxIds) { if (!axIds.includes(crossId)) { errors.push({ @@ -4937,7 +4964,9 @@ export function createPresentation(opts?: CreatePresentationOptions) { ); } // Validate and convert ShapeFragment(s) to XML, then delegate to internal method - const shapesStr = fragmentsToXml(shapesInput as ShapeFragment | ShapeFragment[]); + const shapesStr = fragmentsToXml( + shapesInput as ShapeFragment | ShapeFragment[], + ); pres._addBodyRaw(shapesStr, slideOpts); }, @@ -5873,16 +5902,19 @@ export function restorePresentation(state: SerializedPresentation): Pres { // function that might generate shapes. This prevents ID collisions when // the module's global counter has been reset (e.g., handler recompilation // resets ESM module-level variables to initial values). - if (typeof state.shapeIdCounter === "number") { + if ( + typeof state.shapeIdCounter === "number" && + Number.isFinite(state.shapeIdCounter) && + Number.isInteger(state.shapeIdCounter) && + state.shapeIdCounter >= 1 + ) { setShapeIdCounter(state.shapeIdCounter); } else if (state.slides && Array.isArray(state.slides)) { - // No saved counter — scan existing slides to find the max shape ID + // No valid saved counter — scan existing slides to find the max shape ID // so new shapes don't collide with restored ones. let maxId = 1; for (const slide of state.slides) { - const idMatches = slide.shapes.matchAll(/ maxId) maxId = id; } } diff --git a/tests/pptx-validation.test.ts b/tests/pptx-validation.test.ts index 7ca0724..c4216c7 100644 --- a/tests/pptx-validation.test.ts +++ b/tests/pptx-validation.test.ts @@ -2464,6 +2464,47 @@ describe("layout helpers", () => { }); expect(pres2._images[0].relId).toBe("rIdImage1"); }); + + it("should preserve shape ID counter across serialize/restore", () => { + // Part 1: Create presentation with several shapes + const pres1 = pptx.createPresentation(); + pptx.titleSlide(pres1, { title: "Slide 1" }); + pptx.contentSlide(pres1, { title: "Slide 2", body: "Content" }); + pptx.contentSlide(pres1, { title: "Slide 3", body: "More" }); + + const state = pres1.serialize(); + // Counter should be > 1 after creating shapes + expect(state.shapeIdCounter).toBeGreaterThan(1); + + // Part 2: Restore and add slide numbers (which create new shapes) + const pres2 = pptx.restorePresentation(state); + pptx.addSlideNumbers(pres2); + + // Should NOT throw duplicate shape ID validation errors + const zip = pres2.buildZip(); + expect(zip).toBeInstanceOf(Uint8Array); + expect(zip.length).toBeGreaterThan(0); + }); + + it("should scan slides for max ID when shapeIdCounter is missing", () => { + // Simulate a legacy serialized presentation without shapeIdCounter + const pres1 = pptx.createPresentation(); + pptx.titleSlide(pres1, { title: "Title" }); + pptx.contentSlide(pres1, { title: "Content", body: "Body" }); + + const state = pres1.serialize(); + // Remove the counter to simulate legacy data + delete (state as Record).shapeIdCounter; + + // Restore should scan and find max ID from shapes + const pres2 = pptx.restorePresentation(state); + pptx.addSlideNumbers(pres2); + + // Should NOT throw duplicate shape ID validation errors + const zip = pres2.buildZip(); + expect(zip).toBeInstanceOf(Uint8Array); + expect(zip.length).toBeGreaterThan(0); + }); }); });