fix: restore shape ID counter before createPresentation in restorePre… (#113)

* fix: restore shape ID counter before createPresentation in restorePresentation

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 <simongdavies@users.noreply.github.com>

* fix: address PR #113 review feedback

- 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 <simongdavies@users.noreply.github.com>

---------

Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>

Author: simongdavies

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:b9ec89c9f86fc4b5",
   "dtsHash": "sha256:0e08c6d6b51b36e8",
   "importStyle": "named",
   "hints": {

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 | null | undefined>): ShapeFragment {
+export function shapes(
+  items: Array<ShapeFragment | null | undefined>,
+): 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 = /<p:cNvPr\s+id="(\d+)"/g;
+
+/**
+ * Scan slide shapes XML and return all numeric shape IDs found.
+ * @param shapes - Raw OOXML shapes XML string from a slide
+ * @returns Array of numeric shape IDs
+ */
+function _extractShapeIds(shapes: string): number[] {
+  const ids: number[] = [];
+  for (const m of shapes.matchAll(SHAPE_ID_REGEX)) {
+    const n = parseInt(m[1], 10);
+    if (Number.isFinite(n)) ids.push(n);
+  }
+  return ids;
+}
+
 function _validatePresentation(
   slides: SlideData[],
   charts: ChartEntry[],
@@ -4422,7 +4443,11 @@ function _validatePresentation(
     if (topTags) {
       for (const tag of topTags) {
         const nodeName = tag.match(/<(p:\w+)/)?.[1];
-        if (nodeName && !ALLOWED_SHAPE_NODES.has(nodeName) && nodeName !== "p:txBody") {
+        if (
+          nodeName &&
+          !ALLOWED_SHAPE_NODES.has(nodeName) &&
+          nodeName !== "p:txBody"
+        ) {
           // Only warn for unexpected nodes (not errors, since some are legit internal use)
           warnings.push({
             code: "PPTX_UNEXPECTED_SHAPE_NODE",
@@ -4436,11 +4461,11 @@ function _validatePresentation(
     }
 
     // A3: Detect duplicate shape IDs within a slide
-    const idMatches = shapes.matchAll(/<p:cNvPr\s+id="(\d+)"/g);
+    const slideShapeIds = _extractShapeIds(shapes);
     const slideIds = new Set<string>();
-    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(`</${tag}>`, "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 <c:chartSpace> or <c:chart> elements.",
+          message:
+            "Chart XML missing required <c:chartSpace> or <c:chart> 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(/<c:axId val="(\d+)"\/>/g)].map((m) =>
-        m[1],
+      const axIds = [...xml.matchAll(/<c:axId val="(\d+)"\/>/g)].map(
+        (m) => m[1],
+      );
+      const crossAxIds = [...xml.matchAll(/<c:crossAx val="(\d+)"\/>/g)].map(
+        (m) => m[1],
       );
-      const crossAxIds = [
-        ...xml.matchAll(/<c:crossAx val="(\d+)"\/>/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);
     },
 
@@ -5869,6 +5898,29 @@ 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" &&
+    Number.isFinite(state.shapeIdCounter) &&
+    Number.isInteger(state.shapeIdCounter) &&
+    state.shapeIdCounter >= 1
+  ) {
+    setShapeIdCounter(state.shapeIdCounter);
+  } else if (state.slides && Array.isArray(state.slides)) {
+    // 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) {
+      for (const id of _extractShapeIds(slide.shapes)) {
+        if (id > maxId) maxId = id;
+      }
+    }
+    setShapeIdCounter(maxId);
+  }
+
   // Recreate the presentation with the same options
   const pres = createPresentation({
     theme: state.themeName,
@@ -5899,12 +5951,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;
 }
 

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<string, unknown>).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);
+    });
   });
 });