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

[arcaputo3]

Summary

• Root cause: StyleIndex.fromWorkbookWithSource only created style remappings for sheets in tracker.modifiedSheets, but metadata-only changes (add-sheet, reorder) force ALL sheets to be regenerated from the domain model. Unmodified sheets received Map.empty remappings, causing every cell's styleId to fall back to 0 (default).
• Fix: Generate remappings for all sheets unconditionally — unmodified sheets' styles all originate from the source file, so the remapping is identity-like with no side effects.
• Regression test: Creates a 2-sheet styled workbook, adds a new sheet (triggers modifiedMetadata only), verifies styles survive on untouched sheets at both domain and XML levels.

Test plan

• New regression test: add-sheet preserves styles on untouched sheets (TJC-751)
• All 837 existing tests pass (./mill __.test)
• Format check passes (./mill __.checkFormat)
• Dogfood: generated_AIT_CapTable_Waterfall.xlsx — add-sheet + batch put/style, then diff HTML output of untouched sheets against original (identical)
• Visual verification in Excel: TJC blue headers, borders, number formats all preserved

Closes TJC-751

🤖 Generated with Claude Code

[claude]

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

Summary

The root cause diagnosis is correct and the fix is sound. When modifiedMetadata=true (triggered by add-sheet, rename-sheet, reorder), unifiedWrite regenerates all sheets from the domain model (sheetsToRegenerate = workbook.sheets.indices.toSet), but the old fromWorkbookWithSource only produced remappings for modifiedSheets. The missing remappings caused every cell on unmodified-but-regenerated sheets to use the Map.empty fallback in sheetRemappings.getOrElse(idx, Map.empty), collapsing all style IDs to 0.

The fix—unconditionally generating remappings for all sheets—is correct and minimal.

---

Issues

1. Test: ZipFile resource leak on assertion failure

In the regression test, outputZip is opened but only closed at the end if no assertion throws first:

val outputZip = new ZipFile(output.toFile)
val sheet1Xml = readEntryString(outputZip, outputZip.getEntry("xl/worksheets/sheet1.xml"))
// ... assertions that can throw ...
outputZip.close()  // never reached if assertion above fails
Files.deleteIfExists(source)
Files.deleteIfExists(output)

If an assertion fails the ZipFile handle and temp files leak. Wrap with try/finally or scala.util.Using:

Using(new ZipFile(output.toFile)) { outputZip =>
  val sheet1Xml = readEntryString(outputZip, outputZip.getEntry("xl/worksheets/sheet1.xml"))
  // assertions...
}

Similarly Files.deleteIfExists for source and output should be in a try/finally block; currently they won't run if an earlier assertion throws.

2. Minor performance regression for partial-sheet-modify path

Previously, when only modifiedSheets was set (not modifiedMetadata), unmodified sheets were skipped entirely. Now every sheet's StyleRegistry is scanned unconditionally. For workbooks with hundreds of sheets where only one was modified, this is O(n × styles_per_sheet) of avoidable work—the generated remappings for unmodified-but-copied-verbatim sheets are never consumed.

A targeted guard would restore the optimisation without reintroducing the bug:

val needsRemappingForAll = ctx.modificationTracker.modifiedMetadata || ctx.modificationTracker.reorderedSheets
val remappings = wb.sheets.zipWithIndex.map { case (sheet, sheetIdx) =>
  if needsRemappingForAll || ctx.modificationTracker.modifiedSheets.contains(sheetIdx) then
    // ... existing remapping logic ...
    sheetIdx -> remapping.toMap
  else
    sheetIdx -> Map.empty[Int, Int]  // copied verbatim; remapping unused
}.toMap

This is not a correctness issue—the current code is correct—but worth considering given the project's performance focus.

3. No coverage for rename-sheet and reorder-sheet variants

The regression test covers add-sheet → modifiedMetadata=true. The same bug would have occurred for rename-sheet and sheet reorder (which sets reorderedSheets=true), since both also trigger full-sheet regeneration. Adding a parameterised or separate test for at least one of those cases would close that gap.

---

What's good

• Fix is minimal and targeted: The only structural change is flatMap { … if condition … Some/None } → map { … }, removing dead tracker/modifiedSheetIndices variables cleanly.
• Scaladoc updated to reflect the new semantics.
• Test is thorough: it validates domain-level style properties (bold font, fill) and raw XML (s="N" attributes), covering both the parsing and serialisation layers.
• Tracker state assertion (modifiedMetadata=true, existing sheets not in modifiedSheets) documents the exact precondition the bug required, making the test self-explanatory.

---

Verdict

Correct fix for a legitimate P0 regression. The resource-leak in the test should be fixed before merge. The performance point is optional but worth a note. Happy to approve once the ZipFile/temp-file cleanup is wrapped in try/finally.

[claude]

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

The three issues flagged in the previous round (resource leak, performance regression, missing reorder coverage) are all addressed in the current diff. The fix and tests are in good shape. A few remaining observations:

---

Remaining observations

1. needsRemappingForAll name is slightly misleading

When this flag is false, modified sheets still get full remappings — only truly-unmodified sheets get Map.empty. A name like allSheetsRegeneratedFromDomain or metadataForcesFullRegen would better capture why all sheets need remapping rather than that they do:

// Current — reads like an unconditional flag
val needsRemappingForAll = tracker.modifiedMetadata || tracker.reorderedSheets

// Suggested — captures the causal reason
val allSheetsRegeneratedFromDomain = tracker.modifiedMetadata || tracker.reorderedSheets

Minor, but this code path is subtle enough that future readers will benefit from the extra clarity.

2. Callers using Map.contains vs getOrElse on the returned remappings

The return type semantics changed: previously absent keys meant "no remapping" (unmodified sheets not included); now every sheet index has an entry (possibly Map.empty). If any downstream caller checks sheetRemappings.contains(idx) to decide whether to regenerate a sheet (rather than just using getOrElse(idx, Map.empty)), that caller's behaviour has silently changed. The passing test suite strongly suggests this isn't the case, but it's worth a quick grep through the callers of fromWorkbookWithSource to confirm no contains-based branch exists.

3. rename-sheet not exercised

Both add-sheet (sets modifiedMetadata) and reorder (sets reorderedSheets) are covered. rename-sheet also sets modifiedMetadata and would have triggered the same bug. A short comment in the test noting this ("rename-sheet triggers the same code path via modifiedMetadata") would close the documentation gap without needing another test.

4. .get calls in tests (very minor)

assert(a1Style.get.font.bold, "A1 should be bold ...")

The preceding assert(a1Style.isDefined) makes this safe at runtime, but in MUnit a failing isDefined assert won't short-circuit the subsequent .get. If the isDefined assertion fails, the .get on the next line will throw a NoSuchElementException that obscures the failure message. Prefer a1Style.fold(fail("A1 has no style"))(s => assert(s.font.bold, ...)) or just assertEquals(a1Style.map(\.font.bold), Some(true)).

---

What's good

• All previous concerns addressed: resource cleanup, performance guard, and reorder test are present.
• Fix remains minimal: two-line logic change (flatMap→map, condition update); no collateral changes.
• Dual-layer test: domain-model properties (bold font, fill) and raw XML (s="N" attributes) — covers both the parsing and serialisation layers.
• Tracker state assertion documents the exact precondition for the bug, making the regression test self-explanatory.
• Scaladoc updated accurately reflects the new semantics.

---

Verdict

Correct, minimal fix for a P0 regression. Items 1–3 above are non-blocking (naming and documentation). Item 4 is a minor test-robustness nit. Happy to approve as-is or after any of those are addressed.