fix(ooxml): Preserve styles on untouched sheets during metadata writes (TJC-751) (#220)

* fix(ooxml): Preserve styles on untouched sheets during metadata writes (TJC-751)

StyleIndex.fromWorkbookWithSource only created style remappings for
sheets in tracker.modifiedSheets, but metadata-only changes (add-sheet,
reorder) cause ALL sheets to be regenerated from the domain model.
Unmodified sheets received empty remappings, causing every cell's
styleId to fall back to 0 (default), silently stripping all styling.

Generate remappings for all sheets unconditionally. Unmodified sheets'
styles all originate from the source file, so every canonical key is
found in the unified index — no new styles are added, and the remapping
is identity-like. When sheets are copied verbatim (the normal surgical
path), the extra remappings simply go unused.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(ooxml): Address PR review — perf guard, resource leak, reorder test

1. Performance: restore skip for unmodified sheets when only
   modifiedSheets is set (not modifiedMetadata/reorderedSheets).
   Avoids O(n × styles) of unused remapping work.

2. Test: wrap ZipFile and temp files in try/finally to prevent
   resource leaks on assertion failure.

3. Test: add reorder variant (reorderedSheets=true) covering
   the other code path that triggers full-sheet regeneration.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: arcaputo3

xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala

@@ -160,22 +160,20 @@ object StyleIndex:
    * Build style index for workbook with source, preserving original styles.
    *
    * This variant is used during surgical modification to avoid corruption:
-   *   - Deduplicates styles ONLY from modified sheets (optimal compression)
-   *   - Preserves original styles from source for unmodified sheets (no remapping needed)
-   *   - Ensures unmodified sheets' style references remain valid after write
+   *   - Preserves ALL original styles from source (including duplicates)
+   *   - Deduplicates only truly new styles from modified sheets
+   *   - Generates remappings for ALL sheets (not just modified ones)
    *
-   * Strategy:
-   *   1. Parse original styles.xml to get complete WorkbookStyles
-   *   2. Deduplicate styles from modified sheets only
-   *   3. Ensure all original styles are present in output (fill gaps if needed)
-   *   4. Return remappings ONLY for modified sheets (unmodified sheets use original IDs)
+   * All sheets need remappings because metadata-only changes (add-sheet, reorder) cause ALL sheets
+   * to be regenerated from the domain model, not copied verbatim from source. Without remappings,
+   * unmodified-but-regenerated sheets would fall back to styleId 0, stripping all styles (TJC-751).
    *
    * @param wb
    *   The workbook with modified sheets
    * @param ctx
    *   Source context providing modification tracker and original file path
    * @return
-   *   (StyleIndex with all original + deduplicated styles, Map[modifiedSheetIdx -> remapping])
+   *   (StyleIndex with all original + new styles, Map[sheetIdx -> remapping])
    */
   @SuppressWarnings(
     Array(
@@ -188,9 +186,8 @@ object StyleIndex:
     wb: Workbook,
     ctx: SourceContext
   ): (StyleIndex, Map[Int, Map[Int, Int]]) =
-    // Extract values from context
     val tracker = ctx.modificationTracker
-    val modifiedSheetIndices = tracker.modifiedSheets
+    val needsRemappingForAll = tracker.modifiedMetadata || tracker.reorderedSheets
     val sourcePath = ctx.sourcePath
     import scala.collection.mutable
     import java.util.zip.ZipInputStream
@@ -251,9 +248,13 @@ object StyleIndex:
     }))
     var nextNumFmtId = if numFmtsBuilder.isEmpty then 164 else numFmtsBuilder.map(_._1).max + 1
 
-    // Step 4: Process ONLY modified sheets for style remapping
-    val remappings = wb.sheets.zipWithIndex.flatMap { case (sheet, sheetIdx) =>
-      if modifiedSheetIndices.contains(sheetIdx) then
+    // Step 4: Process sheets for style remapping
+    // When metadata or reorder changes force ALL sheets to be regenerated from domain model,
+    // every sheet needs a remapping — otherwise unmodified sheets fall back to styleId 0,
+    // stripping all styles (TJC-751). When only specific sheets are modified, unmodified
+    // sheets are copied verbatim from source and don't need remappings.
+    val remappings = wb.sheets.zipWithIndex.map { case (sheet, sheetIdx) =>
+      if needsRemappingForAll || tracker.modifiedSheets.contains(sheetIdx) then
         val registry = sheet.styleRegistry
         val remapping = mutable.Map[Int, Int]()
 
@@ -302,10 +303,10 @@ object StyleIndex:
                     case _ => ()
         }
 
-        Some(sheetIdx -> remapping.toMap)
+        sheetIdx -> remapping.toMap
       else
-        // Unmodified sheet - no remapping needed (uses original style IDs)
-        None
+        // Unmodified sheet copied verbatim from source — remapping unused
+        sheetIdx -> Map.empty[Int, Int]
     }.toMap
 
     // Step 5: Finalize component vectors (original + any new components)

xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala

@@ -1128,6 +1128,247 @@ class XlsxWriterCorruptionRegressionSpec extends FunSuite:
     Files.deleteIfExists(output)
   }
 
+  // ===== Regression Test for Style Stripping on Untouched Sheets (TJC-751) =====
+
+  test("add-sheet preserves styles on untouched sheets (TJC-751)") {
+    // Regression test for: styles stripped on ALL untouched sheets during metadata-only writes
+    // Bug: StyleIndex.fromWorkbookWithSource only created remappings for modified sheets,
+    //      but metadata changes (add-sheet) force ALL sheets to be regenerated from domain model.
+    //      Unmodified sheets got Map.empty remapping → all styles fell back to styleId 0 (default).
+    // Impact: P0 - All styling (bold, colors, number formats) silently stripped from untouched sheets
+
+    val source = createWorkbookWithRichStyles()
+    val output = Files.createTempFile("tjc-751-add-sheet", ".xlsx")
+
+    try
+      // Read the workbook (populates SourceContext)
+      val wb = XlsxReader.read(source).fold(err => fail(s"Failed to read: $err"), identity)
+      assertEquals(wb.sheets.size, 2, "Source should have 2 sheets")
+
+      // Add a new sheet — triggers modifiedMetadata=true, modifiedSheets stays empty
+      val withNewSheet = wb.put(Sheet("NewComps").put(ref"A1" -> "New data"))
+
+      // Verify tracker state: metadata modified but no existing sheets marked modified
+      val tracker = withNewSheet.sourceContext.get.modificationTracker
+      assert(tracker.modifiedMetadata, "modifiedMetadata should be true after add-sheet")
+      assert(
+        !tracker.modifiedSheets.contains(0) && !tracker.modifiedSheets.contains(1),
+        "Existing sheets should NOT be in modifiedSheets"
+      )
+
+      // Write the modified workbook
+      XlsxWriter
+        .write(withNewSheet, output)
+        .fold(err => fail(s"Failed to write: $err"), identity)
+
+      // Read back and verify
+      val result = XlsxReader.read(output).fold(err => fail(s"Failed to reload: $err"), identity)
+      assertEquals(result.sheets.size, 3, "Output should have 3 sheets")
+
+      // CRITICAL: Verify styles on "Styled" sheet are preserved (not stripped to default)
+      val styled = result("Styled").fold(err => fail(s"Sheet Styled not found: $err"), identity)
+
+      // A1 should be bold
+      val a1Style = styled(ref"A1").styleId
+        .flatMap(id => styled.styleRegistry.get(id))
+      assert(a1Style.isDefined, "A1 should have a style")
+      assert(a1Style.get.font.bold, "A1 should be bold (was stripped to default!)")
+
+      // B1 should have a colored fill
+      val b1Style = styled(ref"B1").styleId
+        .flatMap(id => styled.styleRegistry.get(id))
+      assert(b1Style.isDefined, "B1 should have a style")
+      assert(
+        b1Style.get.fill != Fill.default,
+        "B1 should have colored fill (was stripped to default!)"
+      )
+
+      // CRITICAL: Verify styles on "AlsoStyled" sheet are preserved
+      val alsoStyled =
+        result("AlsoStyled").fold(err => fail(s"Sheet AlsoStyled not found: $err"), identity)
+
+      val a1Style2 = alsoStyled(ref"A1").styleId
+        .flatMap(id => alsoStyled.styleRegistry.get(id))
+      assert(a1Style2.isDefined, "AlsoStyled A1 should have a style")
+      assert(a1Style2.get.font.bold, "AlsoStyled A1 should be bold (was stripped!)")
+
+      // Also verify at the raw XML level: check that cells in untouched sheets have s= attributes
+      val outputZip = new ZipFile(output.toFile)
+      try
+        val sheet1Xml = readEntryString(outputZip, outputZip.getEntry("xl/worksheets/sheet1.xml"))
+
+        // Cells with styles should have s="N" where N > 0
+        val cellsWithStyles =
+          """s="(\d+)"""".r.findAllMatchIn(sheet1Xml).map(_.group(1).toInt).toList
+        assert(
+          cellsWithStyles.exists(_ > 0),
+          s"Sheet1 cells should have non-zero style indices but found: $cellsWithStyles"
+        )
+      finally outputZip.close()
+    finally
+      Files.deleteIfExists(source)
+      Files.deleteIfExists(output)
+  }
+
+  test("reorder preserves styles on all sheets (TJC-751 variant)") {
+    // Same bug trigger via reorderedSheets=true instead of modifiedMetadata=true
+    val source = createWorkbookWithRichStyles()
+    val output = Files.createTempFile("tjc-751-reorder", ".xlsx")
+
+    try
+      val wb = XlsxReader.read(source).fold(err => fail(s"Failed to read: $err"), identity)
+
+      // Reorder sheets: [Styled, AlsoStyled] → [AlsoStyled, Styled]
+      val reordered = wb
+        .reorder(Vector(SheetName.unsafe("AlsoStyled"), SheetName.unsafe("Styled")))
+        .fold(err => fail(s"Failed to reorder: $err"), identity)
+
+      val tracker = reordered.sourceContext.get.modificationTracker
+      assert(tracker.reorderedSheets, "reorderedSheets should be true")
+
+      XlsxWriter
+        .write(reordered, output)
+        .fold(err => fail(s"Failed to write: $err"), identity)
+
+      val result = XlsxReader.read(output).fold(err => fail(s"Failed to reload: $err"), identity)
+      assertEquals(result.sheetNames.map(_.value), Vector("AlsoStyled", "Styled"))
+
+      // Verify styles survived the reorder
+      val styled = result("Styled").fold(err => fail(s"Sheet not found: $err"), identity)
+      val a1Style = styled(ref"A1").styleId.flatMap(id => styled.styleRegistry.get(id))
+      assert(a1Style.isDefined, "A1 should have a style after reorder")
+      assert(a1Style.get.font.bold, "A1 should be bold after reorder (was stripped!)")
+
+      val b1Style = styled(ref"B1").styleId.flatMap(id => styled.styleRegistry.get(id))
+      assert(b1Style.isDefined, "B1 should have a style after reorder")
+      assert(b1Style.get.fill != Fill.default, "B1 fill stripped after reorder!")
+
+      val alsoStyled =
+        result("AlsoStyled").fold(err => fail(s"Sheet not found: $err"), identity)
+      val a1Style2 = alsoStyled(ref"A1").styleId.flatMap(id => alsoStyled.styleRegistry.get(id))
+      assert(a1Style2.isDefined, "AlsoStyled A1 should have a style after reorder")
+      assert(a1Style2.get.font.bold, "AlsoStyled A1 should be bold after reorder!")
+    finally
+      Files.deleteIfExists(source)
+      Files.deleteIfExists(output)
+  }
+
+  private def createWorkbookWithRichStyles(): Path =
+    val path = Files.createTempFile("test-rich-styles", ".xlsx")
+    val out = new ZipOutputStream(Files.newOutputStream(path))
+    out.setLevel(1)
+
+    try
+      writeEntry(
+        out,
+        "[Content_Types].xml",
+        """<?xml version="1.0"?>
+<Types xmlns="http://schemas.openxmlformats.org/package/2006/content-types">
+  <Default Extension="rels" ContentType="application/vnd.openxmlformats-package.relationships+xml"/>
+  <Default Extension="xml" ContentType="application/xml"/>
+  <Override PartName="/xl/workbook.xml" ContentType="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.main+xml"/>
+  <Override PartName="/xl/worksheets/sheet1.xml" ContentType="application/vnd.openxmlformats-officedocument.spreadsheetml.worksheet+xml"/>
+  <Override PartName="/xl/worksheets/sheet2.xml" ContentType="application/vnd.openxmlformats-officedocument.spreadsheetml.worksheet+xml"/>
+  <Override PartName="/xl/styles.xml" ContentType="application/vnd.openxmlformats-officedocument.spreadsheetml.styles+xml"/>
+</Types>"""
+      )
+
+      writeEntry(
+        out,
+        "_rels/.rels",
+        """<?xml version="1.0"?>
+<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships">
+  <Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/officeDocument" Target="xl/workbook.xml"/>
+</Relationships>"""
+      )
+
+      writeEntry(
+        out,
+        "xl/workbook.xml",
+        """<?xml version="1.0"?>
+<workbook xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships">
+  <sheets>
+    <sheet name="Styled" sheetId="1" r:id="rId1"/>
+    <sheet name="AlsoStyled" sheetId="2" r:id="rId2"/>
+  </sheets>
+</workbook>"""
+      )
+
+      writeEntry(
+        out,
+        "xl/_rels/workbook.xml.rels",
+        """<?xml version="1.0"?>
+<Relationships xmlns="http://schemas.openxmlformats.org/package/2006/relationships">
+  <Relationship Id="rId1" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/worksheet" Target="worksheets/sheet1.xml"/>
+  <Relationship Id="rId2" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/worksheet" Target="worksheets/sheet2.xml"/>
+  <Relationship Id="rId3" Type="http://schemas.openxmlformats.org/officeDocument/2006/relationships/styles" Target="styles.xml"/>
+</Relationships>"""
+      )
+
+      // Styles: bold font, red solid fill, currency number format
+      writeEntry(
+        out,
+        "xl/styles.xml",
+        """<?xml version="1.0"?>
+<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
+  <numFmts count="1">
+    <numFmt numFmtId="164" formatCode="&quot;$&quot;#,##0.00"/>
+  </numFmts>
+  <fonts count="2">
+    <font><sz val="11"/><name val="Calibri"/></font>
+    <font><b/><sz val="11"/><name val="Calibri"/></font>
+  </fonts>
+  <fills count="3">
+    <fill><patternFill patternType="none"/></fill>
+    <fill><patternFill patternType="gray125"/></fill>
+    <fill><patternFill patternType="solid"><fgColor rgb="FFFF0000"/></patternFill></fill>
+  </fills>
+  <borders count="1"><border><left/><right/><top/><bottom/><diagonal/></border></borders>
+  <cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs>
+  <cellXfs count="4">
+    <xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/>
+    <xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/>
+    <xf numFmtId="0" fontId="0" fillId="2" borderId="0" xfId="0" applyFill="1"/>
+    <xf numFmtId="164" fontId="1" fillId="0" borderId="0" xfId="0" applyNumberFormat="1" applyFont="1"/>
+  </cellXfs>
+  <cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles>
+</styleSheet>"""
+      )
+
+      // Sheet 1: "Styled" — A1 is bold, B1 has red fill, C1 has currency format
+      writeEntry(
+        out,
+        "xl/worksheets/sheet1.xml",
+        """<?xml version="1.0"?>
+<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
+  <sheetData>
+    <row r="1">
+      <c r="A1" s="1" t="inlineStr"><is><t>Bold Header</t></is></c>
+      <c r="B1" s="2" t="inlineStr"><is><t>Red Cell</t></is></c>
+      <c r="C1" s="3"><v>1234.56</v></c>
+    </row>
+  </sheetData>
+</worksheet>"""
+      )
+
+      // Sheet 2: "AlsoStyled" — A1 is bold
+      writeEntry(
+        out,
+        "xl/worksheets/sheet2.xml",
+        """<?xml version="1.0"?>
+<worksheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
+  <sheetData>
+    <row r="1">
+      <c r="A1" s="1" t="inlineStr"><is><t>Bold in Sheet 2</t></is></c>
+    </row>
+  </sheetData>
+</worksheet>"""
+      )
+
+    finally out.close()
+
+    path
+
   private def createWorkbookWithoutSST(): Path =
     // Create a minimal workbook that uses inline strings (no sharedStrings.xml)
     val path = Files.createTempFile("test-no-sst", ".xlsx")