Skip to content

refactor: one copy of the helpers that were copied, and comments that match the code - #559

Merged
productdevbook merged 1 commit into
mainfrom
refactor/architecture-cleanup
Aug 13, 2026
Merged

refactor: one copy of the helpers that were copied, and comments that match the code#559
productdevbook merged 1 commit into
mainfrom
refactor/architecture-cleanup

Conversation

@productdevbook

Copy link
Copy Markdown
Owner

A cleanup pass with no behaviour change. The commit message has the full
inventory; this is what is worth knowing about it.

The claim is checked, not asserted

"No behaviour change" is the easiest thing in a refactor to believe and
not verify — and pnpm test cannot settle it on its own, because a
refactor that breaks something the suite does not cover looks exactly
like one that breaks nothing.

So the digests of what the writers produce were compared across the
change: writeXlsx (styled cells, CJK, number formats, a checkbox, a
formula, print titles, an auto-filter, frozen panes, a table with a
total row, a bar chart with a legend and data labels), saveXlsx,
writeOds, writeCsv, plus parseCsv and streamXlsxRows on the read
side. Every digest is identical before and after.

Dates had to be pinned to get there — docProps/core.xml and
meta:creation-date carry the clock, so the first run of the probe
disagreed with itself before it could disagree with main.

The readers are covered by the golden JSON fixtures, which compare
against files Excel and openpyxl wrote rather than against hucre's own
output.

Also re-run: the ODF and OOXML schema validators (#552, #553) — all
parts still valid — verify-package.mjs, and pnpm size, all within
budget.

What it did

Nine byte-identical copies of findChild, plus four of
childElements, three of resolvePath / dirname / findRIdAttr, and
parseIntSafe under a fifth name as parseIntOr. Two full copies of
buildNamedRanges and computeTableRange — and applyPrintDefinedNames
in the reader depends on every writer deriving the print names from
pageSetup the same way (#407), so that pair was the copy worth removing
first.

The FeaturePropertyBag GUID, namespace, content type, part path and
relationship type were declared as constants in one module and then typed
out again as literals in five others.

Comments that had stopped being true

This is the half that matters more than the deduplication, since a wrong
comment costs more than no comment.

_style.ts listed styles.resolveStyle among the deep-copy callers. It
is deliberately the opposite — the parsed font/fill/border records are
shared, because copying them per cell takes peak memory from 407 MB to
787 MB over 720,000 styled cells, and docs/PARITY.md states the sharing
as a contract. A reader who trusted that header would have "fixed" a
documented decision.

chart/util.ts claimed a consolidation that had only half happened —
two chart modules still carried private copies. styles-writer.ts had
the #437 snapshot-on-registration note orphaned above a function that
does no snapshotting. writer.ts labelled the FeaturePropertyBag part
xl/vbaProject.bin. Two comments still called the library defter.

Found and deliberately not fixed

Reported rather than folded in, because none of them is a cleanup:

  1. src/ods/writer.ts:1401 — every ODS hucre writes stamps
    <meta:generator>defter</meta:generator>, the pre-rename project
    name, and a test pins it. Shipped output, so it is someone's call, not
    a refactor's.
  2. src/xlsx/pivot-writer.ts:648 — a local parseCellRef that
    throws on a bad ref shadows the name of the exported, deliberately
    lenient one in xlsx/worksheet.ts. Same name, opposite error
    contract.
  3. decodeUtf8 means two different things — via decodePart (with
    the fix(read): a part over the string ceiling gets a typed error — closes #503 #514 MAX_STRING_LENGTH ceiling) in the buffered readers, a bare
    TextDecoder in the streaming ones. That matches PARITY
    ("streamXlsxRows still has no ceiling") and is intentional; the
    shared name hides it.

Left alone as too big for a cleanup pass: parseWorkbookXml exists in
both the buffered and streaming readers (a genuine subset, but the
<sheets> and date1904 handling is duplicated and can drift), and the
chart/* modules carry three identical 49-line build*SpPr bodies, nine
identical font-family normalizers and nine tri-state clone resolvers.
Each of those carries host-specific documentation; consolidating them is
a project with its own risk, not a tidy-up.

🤖 Generated with Claude Code

… match the code

A cleanup pass. No behaviour change: every writer emits the same bytes and
every reader returns the same model. `pnpm test` is green — 10,583 tests
across 234 files, the same counts as before. The no-behaviour-change claim
is checked rather than assumed: the digests of what `writeXlsx`,
`saveXlsx`, `writeOds`, `writeCsv`, `parseCsv` and `streamXlsxRows`
produce are identical before and after.

## Duplication removed

Each of these was the same question answered in two or more places, which
is how the copies drift:

- `src/xml/tree.ts` (new) holds `findChild`, `childElements`,
  `findDescendant`, `readChildText` and `parseIntSafe`. There were nine
  identical copies of `findChild` alone. The xlsx part readers
  (metadata, pivot, slicer, threaded-comments, cell-images,
  external-link) and `chart/util` now use it; `chart/util` re-exports
  the walk so the eleven chart modules keep their one import.
  `metadata.ts`'s `parseIntOr` was the same function under a fifth name.
- `src/xlsx/derived-ranges.ts` (new) holds `buildNamedRanges` and
  `computeTableRange`, which `writer.ts` and `roundtrip.ts` each carried
  in full. `applyPrintDefinedNames` in the reader depends on every
  writer deriving the print names from `pageSetup` the same way (#407),
  so two copies of that rule was the risk worth removing.
- `resolvePath`, `dirname` and `findRIdAttr` move to
  `xlsx/relationships.ts`, the module that already owns OPC wiring.
  `readXlsx`, `streamXlsxRows` and `readXlsb` each had their own.
- `chart/shape.ts` and `chart/text.ts` dropped their private
  `findChild`; both sit next to `chart/util`, which exports it.
- `auto-size.ts` uses `auto-width.ts`'s `measureLineWidth` instead of a
  byte-identical `measureTextWidth` + `isCjk` pair.
- `sheet-ops.ts` uses the exported `parseRange` from `cell-utils.ts`.
- `csv/stream.ts` uses `csv/reader.ts`'s `startsWith`.
- The FeaturePropertyBag part path, content type, namespace, GUID and
  relationship type were declared in `feature-property-bag.ts` and then
  written out again as literals in five other files. The constants are
  now used; `FEATURE_PROPERTY_BAG_PART_PATH` is derived from
  `FPB_PART_PATH` so the two spellings cannot disagree.

## Dead code

- `chart/text.ts`'s `readBoolAttrValue` was identical to
  `chart/util.ts`'s `readBoolVal` and called by nothing but a test; the
  test now covers the surviving one.
- `ods/writer.ts` computed `decimalsFromCode(section)` twice and used
  neither, which was the only caller of that function.
- `external-link-reader.ts`'s `REL_EXTERNAL_LINK_PATH_TYPES` was
  exported for callers that cannot reach it — the module is not an entry
  point — and its two constants fed nothing else. The reader resolves
  the target by `r:id`, which is why the rel-type URIs had no use; that
  is now written on `parseExternalLink`.
- `src/xml/index.ts` and `src/xlsx/chart/index.ts` were barrels nothing
  imported. `src/xml/index.ts` was unreachable by name as well:
  `./xml` resolves to `src/xml.ts`. (`src/zip/index.ts` stays — a test
  imports it.)
- Nine empty section headers left in `chart-reader.ts` by code that
  moved into `chart/`.

## Comments corrected

All of these described code that had since changed:

- `_style.ts` listed `styles.resolveStyle` as one of the deep-copy
  callers. It is deliberately the opposite — shared, not copied, at 407
  MB against 787 MB over 720,000 styled cells — and `docs/PARITY.md`
  states it as a contract.
- `chart/util.ts` said the chart modules no longer carried private
  copies of `findChild`; two still did. `chart/shape.ts` said
  `chart-clone` carried one; it does not.
- `styles-writer.ts` had the snapshot-on-registration note (#437)
  orphaned above `createStylesCollector`, which does no snapshotting; it
  now sits on the three `add*` registers that do.
- `feature-property-bag.ts` said hucre models the property bag nowhere
  else; `styles.ts` reads the xf complement and `styles-writer.ts`
  writes it.
- `_validate.ts` said its Excel limits are exported for callers to
  check. No entry point re-exports them.
- `writer.ts` labelled the FeaturePropertyBag part `xl/vbaProject.bin`.
- `roundtrip.ts` still called the library `defter` in two comments.

## Also

`scripts/validate-odf.mjs` imported `readFileSync` and never used it.
`src/` is now free of lint warnings.
@productdevbook
productdevbook merged commit db04ead into main Aug 13, 2026
6 checks passed
@productdevbook

Copy link
Copy Markdown
Owner Author

Correcting the verification described above, which claimed more coverage than it had.

The probe's styled sheet put its cells inline in rows:

rows: [[{ value: "Header", style: { font: { bold: true, size: 14 } } }, ]]

That shape is dropped by the buffered writers — the finding that became #562. So the sheet I described as "styled cells, CJK, number formats, a checkbox, a formula" was written as a grid of empty cells, and the digests I compared were digests of a nearly empty document. The style writer, the shared-string table and the checkbox path were not exercised at all.

Re-run, with cells that land

Same comparison, ad0f4a5 (the commit before this PR) against db04ead (the merge), with the styles moved into the cells map where the writer reads them. Verified they land first: <cellXfs count="4"> and s="1" / s="2" / s="3" on the cells that asked.

The digests are identical. So the conclusion holds — but it holds on evidence now rather than on the evidence I cited.

And a limitation worth stating

A mutation check on the shared findChildc.localc.tag — turns 1,409 tests red but leaves the probe's digests unchanged: for unprefixed elements the two are the same string, and the fixtures the probe writes and re-reads are unprefixed.

So the probe pins the writers; the suite and the golden fixtures pin the readers. Neither covers the other, and the probe on its own would not have caught a broken findChild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant