From 7d40688a40c87a662635c9412b89e7c3b8c0ec59 Mon Sep 17 00:00:00 2001 From: Richie Caputo Date: Thu, 19 Feb 2026 11:23:40 -0500 Subject: [PATCH 1/2] fix(ooxml): Preserve styles on untouched sheets during metadata writes (TJC-751) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../com/tjclp/xl/ooxml/style/StyleIndex.scala | 128 ++++++------ .../XlsxWriterCorruptionRegressionSpec.scala | 195 ++++++++++++++++++ 2 files changed, 256 insertions(+), 67 deletions(-) diff --git a/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala b/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala index c6778332..e2ca1084 100644 --- a/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala +++ b/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,6 @@ 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 sourcePath = ctx.sourcePath import scala.collection.mutable import java.util.zip.ZipInputStream @@ -251,61 +246,60 @@ 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 - val registry = sheet.styleRegistry - val remapping = mutable.Map[Int, Int]() - - // Map each local styleId to global index - registry.styles.zipWithIndex.foreach { case (style, localIdx) => - val key = style.canonicalKey - - // First, check if this key exists in original styles - unifiedIndex.get(key) match - case Some(indices) => - // Style exists in original - use FIRST matching index - // This preserves original layout and avoids adding duplicates - remapping(localIdx) = indices.head - case None => - // Not in original - check if we've already added it - additionalStyles.get(key) match - case Some(addedIdx) => - // Already added by earlier sheet processing - remapping(localIdx) = addedIdx - case None => - // Truly new style - add it now - unifiedStyles = unifiedStyles :+ style - additionalStyles(key) = nextIdx - remapping(localIdx) = nextIdx - nextIdx += 1 - - // CRITICAL: Also add new font/fill/border/numFmt if not already present - // Without this, new styles reference non-existent component indices - if !fontSet.contains(style.font) then - fontSet += style.font - fontsBuilder += style.font - - if !fillSet.contains(style.fill) then - fillSet += style.fill - fillsBuilder += style.fill - - if !borderSet.contains(style.border) then - borderSet += style.border - bordersBuilder += style.border - - style.numFmt match - case NumFmt.Custom(code) if !numFmtCodeSet.contains(code) => - numFmtCodeSet += code - numFmtsBuilder += ((nextNumFmtId, NumFmt.Custom(code))) - nextNumFmtId += 1 - case _ => () - } - - Some(sheetIdx -> remapping.toMap) - else - // Unmodified sheet - no remapping needed (uses original style IDs) - None + // Step 4: Process ALL sheets for style remapping + // Even unmodified sheets need remappings because metadata-only changes (add-sheet, + // reorder) cause ALL sheets to be regenerated from domain model. Without remappings, + // unmodified sheets fall back to styleId 0, stripping all styles (TJC-751). + val remappings = wb.sheets.zipWithIndex.map { case (sheet, sheetIdx) => + val registry = sheet.styleRegistry + val remapping = mutable.Map[Int, Int]() + + // Map each local styleId to global index + registry.styles.zipWithIndex.foreach { case (style, localIdx) => + val key = style.canonicalKey + + // First, check if this key exists in original styles + unifiedIndex.get(key) match + case Some(indices) => + // Style exists in original - use FIRST matching index + // This preserves original layout and avoids adding duplicates + remapping(localIdx) = indices.head + case None => + // Not in original - check if we've already added it + additionalStyles.get(key) match + case Some(addedIdx) => + // Already added by earlier sheet processing + remapping(localIdx) = addedIdx + case None => + // Truly new style - add it now + unifiedStyles = unifiedStyles :+ style + additionalStyles(key) = nextIdx + remapping(localIdx) = nextIdx + nextIdx += 1 + + // CRITICAL: Also add new font/fill/border/numFmt if not already present + // Without this, new styles reference non-existent component indices + if !fontSet.contains(style.font) then + fontSet += style.font + fontsBuilder += style.font + + if !fillSet.contains(style.fill) then + fillSet += style.fill + fillsBuilder += style.fill + + if !borderSet.contains(style.border) then + borderSet += style.border + bordersBuilder += style.border + + style.numFmt match + case NumFmt.Custom(code) if !numFmtCodeSet.contains(code) => + numFmtCodeSet += code + numFmtsBuilder += ((nextNumFmtId, NumFmt.Custom(code))) + nextNumFmtId += 1 + case _ => () + } + + sheetIdx -> remapping.toMap }.toMap // Step 5: Finalize component vectors (original + any new components) diff --git a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala index c4209936..482c8526 100644 --- a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala +++ b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala @@ -1128,6 +1128,201 @@ 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() + + // 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 + val output = Files.createTempFile("tjc-751-add-sheet", ".xlsx") + 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) + 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" + ) + + // Clean up + outputZip.close() + 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", + """ + + + + + + + +""" + ) + + writeEntry( + out, + "_rels/.rels", + """ + + +""" + ) + + writeEntry( + out, + "xl/workbook.xml", + """ + + + + + +""" + ) + + writeEntry( + out, + "xl/_rels/workbook.xml.rels", + """ + + + + +""" + ) + + // Styles: bold font, red solid fill, currency number format + writeEntry( + out, + "xl/styles.xml", + """ + + + + + + + + + + + + + + + + + + + + + + +""" + ) + + // Sheet 1: "Styled" — A1 is bold, B1 has red fill, C1 has currency format + writeEntry( + out, + "xl/worksheets/sheet1.xml", + """ + + + + Bold Header + Red Cell + 1234.56 + + +""" + ) + + // Sheet 2: "AlsoStyled" — A1 is bold + writeEntry( + out, + "xl/worksheets/sheet2.xml", + """ + + + + Bold in Sheet 2 + + +""" + ) + + 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") From e107bb8d825d5a1d5a036e7f49bb753fd0e54dc9 Mon Sep 17 00:00:00 2001 From: Richie Caputo Date: Thu, 19 Feb 2026 11:40:15 -0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(ooxml):=20Address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20perf=20guard,=20resource=20leak,=20reorder=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../com/tjclp/xl/ooxml/style/StyleIndex.scala | 113 ++++++------ .../XlsxWriterCorruptionRegressionSpec.scala | 168 +++++++++++------- 2 files changed, 167 insertions(+), 114 deletions(-) diff --git a/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala b/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala index e2ca1084..f61f1fe0 100644 --- a/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala +++ b/xl-ooxml/src/com/tjclp/xl/ooxml/style/StyleIndex.scala @@ -186,6 +186,8 @@ object StyleIndex: wb: Workbook, ctx: SourceContext ): (StyleIndex, Map[Int, Map[Int, Int]]) = + val tracker = ctx.modificationTracker + val needsRemappingForAll = tracker.modifiedMetadata || tracker.reorderedSheets val sourcePath = ctx.sourcePath import scala.collection.mutable import java.util.zip.ZipInputStream @@ -246,60 +248,65 @@ object StyleIndex: })) var nextNumFmtId = if numFmtsBuilder.isEmpty then 164 else numFmtsBuilder.map(_._1).max + 1 - // Step 4: Process ALL sheets for style remapping - // Even unmodified sheets need remappings because metadata-only changes (add-sheet, - // reorder) cause ALL sheets to be regenerated from domain model. Without remappings, - // unmodified sheets fall back to styleId 0, stripping all styles (TJC-751). + // 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) => - val registry = sheet.styleRegistry - val remapping = mutable.Map[Int, Int]() - - // Map each local styleId to global index - registry.styles.zipWithIndex.foreach { case (style, localIdx) => - val key = style.canonicalKey - - // First, check if this key exists in original styles - unifiedIndex.get(key) match - case Some(indices) => - // Style exists in original - use FIRST matching index - // This preserves original layout and avoids adding duplicates - remapping(localIdx) = indices.head - case None => - // Not in original - check if we've already added it - additionalStyles.get(key) match - case Some(addedIdx) => - // Already added by earlier sheet processing - remapping(localIdx) = addedIdx - case None => - // Truly new style - add it now - unifiedStyles = unifiedStyles :+ style - additionalStyles(key) = nextIdx - remapping(localIdx) = nextIdx - nextIdx += 1 - - // CRITICAL: Also add new font/fill/border/numFmt if not already present - // Without this, new styles reference non-existent component indices - if !fontSet.contains(style.font) then - fontSet += style.font - fontsBuilder += style.font - - if !fillSet.contains(style.fill) then - fillSet += style.fill - fillsBuilder += style.fill - - if !borderSet.contains(style.border) then - borderSet += style.border - bordersBuilder += style.border - - style.numFmt match - case NumFmt.Custom(code) if !numFmtCodeSet.contains(code) => - numFmtCodeSet += code - numFmtsBuilder += ((nextNumFmtId, NumFmt.Custom(code))) - nextNumFmtId += 1 - case _ => () - } - - sheetIdx -> remapping.toMap + if needsRemappingForAll || tracker.modifiedSheets.contains(sheetIdx) then + val registry = sheet.styleRegistry + val remapping = mutable.Map[Int, Int]() + + // Map each local styleId to global index + registry.styles.zipWithIndex.foreach { case (style, localIdx) => + val key = style.canonicalKey + + // First, check if this key exists in original styles + unifiedIndex.get(key) match + case Some(indices) => + // Style exists in original - use FIRST matching index + // This preserves original layout and avoids adding duplicates + remapping(localIdx) = indices.head + case None => + // Not in original - check if we've already added it + additionalStyles.get(key) match + case Some(addedIdx) => + // Already added by earlier sheet processing + remapping(localIdx) = addedIdx + case None => + // Truly new style - add it now + unifiedStyles = unifiedStyles :+ style + additionalStyles(key) = nextIdx + remapping(localIdx) = nextIdx + nextIdx += 1 + + // CRITICAL: Also add new font/fill/border/numFmt if not already present + // Without this, new styles reference non-existent component indices + if !fontSet.contains(style.font) then + fontSet += style.font + fontsBuilder += style.font + + if !fillSet.contains(style.fill) then + fillSet += style.fill + fillsBuilder += style.fill + + if !borderSet.contains(style.border) then + borderSet += style.border + bordersBuilder += style.border + + style.numFmt match + case NumFmt.Custom(code) if !numFmtCodeSet.contains(code) => + numFmtCodeSet += code + numFmtsBuilder += ((nextNumFmtId, NumFmt.Custom(code))) + nextNumFmtId += 1 + case _ => () + } + + sheetIdx -> remapping.toMap + else + // Unmodified sheet copied verbatim from source — remapping unused + sheetIdx -> Map.empty[Int, Int] }.toMap // Step 5: Finalize component vectors (original + any new components) diff --git a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala index 482c8526..92352b42 100644 --- a/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala +++ b/xl-ooxml/test/src/com/tjclp/xl/ooxml/XlsxWriterCorruptionRegressionSpec.scala @@ -1138,73 +1138,119 @@ class XlsxWriterCorruptionRegressionSpec extends FunSuite: // Impact: P0 - All styling (bold, colors, number formats) silently stripped from untouched sheets val source = createWorkbookWithRichStyles() - - // 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 val output = Files.createTempFile("tjc-751-add-sheet", ".xlsx") - 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) + 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" + ) - 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!)") + // 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!)" + ) - // Also verify at the raw XML level: check that cells in untouched sheets have s= attributes - val outputZip = new ZipFile(output.toFile) - val sheet1Xml = readEntryString(outputZip, outputZip.getEntry("xl/worksheets/sheet1.xml")) + // 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) + } - // 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" - ) + 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") - // Clean up - outputZip.close() - Files.deleteIfExists(source) - Files.deleteIfExists(output) + 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 =