fix(archive-metadata): make the OPF identifier writer match what the reader reports - #603
Conversation
…reader reports Five ways the writer disagreed with the document it was editing, each verified against an OPF the reader handles fine: - A created `<dc:identifier>` was tagged with a plain `opf:scheme` name. A package that uses the OPF namespace by default need not declare the legacy prefix, and the result serialized to XML that no longer parses. Both the element and the attribute are now bound through their namespace. - The root was matched as `tagName === "package"` and `<metadata>` via `getElementsByTagName`, so a package that prefixes the OPF namespace was rejected outright. Elements are matched by local name now, the same way the read side does. - Clearing the ISBN removed the element but left the `<meta refines>` entries pointing at its id. - The scheme was only read from attributes, so an EPUB 3 identifier typed by an `identifier-type` refinement was never found and a second ISBN was appended beside it — with `xmlns=""` on it. Refinements are read now, through the same ONIX codelist 5 table the reader uses. - Clearing an ISBN carried by the element `<package unique-identifier>` names removed it and left the reference dangling. That element is structural, so it is no longer deleted; the reader infers ISBN from a bare ISBN value anyway, so there is nothing an untagging could buy. The eight new cases fail against the previous writer and the fifteen existing ones are unchanged, so the behaviour every current caller relies on is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f514f9774
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? [] | ||
| : listChildrenByLocalName(metadata, "meta").filter( | ||
| function refinesElement(meta) { | ||
| return meta.getAttribute("refines")?.trim() === `#${id}` |
There was a problem hiding this comment.
When an EPUB uses refines="book-id", which the read side accepts by stripping an optional leading #, this exact comparison fails to associate the refinement with the existing identifier. Setting an ISBN then appends a duplicate identifier, while clearing it leaves the ISBN untouched; normalize the reference the same way as the reader before comparing it with id.
Useful? React with 👍 / 👎.
| /** | ||
| * The scheme an element states, wherever it states it: EPUB 2 puts it on the | ||
| * identifier, EPUB 3 refines it from a sibling `<meta>`. | ||
| */ |
There was a problem hiding this comment.
elementScheme
This comment merely narrates that elementScheme obtains a scheme from either the element or its refinement, which is already evident from the function name and implementation; keeping this kind of helper narration conflicts with the repository's explicit no-restatement comment convention.
AGENTS.md reference: AGENTS.md:L9-L13
Useful? React with 👍 / 👎.
… write The hardening this branch added re-derived from the DOM what archive-reader already reports: an ONIX codelist 5 table copied from upstream, the three spellings of the scheme attribute, the EPUB 3 `identifier-type` refinement walk, and the `unique-identifier` lookup. `parseOpf` reports each identifier verbatim — including the `id` that addresses its element — and `resolveArchiveMetadata` says which scheme the book ends up advertising for it. Pairing the two gives the writer every decision it was making by hand, from the code that owns those decisions, so the two cannot drift. Elements are located by `id` where the document gives one, falling back to position for identifiers authored without one — the only handle they have, and the order the parser reported them in. Same 23 tests, unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Problem
The OPF writer and the OPF reader disagreed about the same document in five ways. Each is reproducible against an OPF the reader handles fine, and each is fixed here against the writer's existing
isbn?: stringAPI — no behaviour change for callers.<dc:identifier>was taggedsetAttribute("opf:scheme", …)opfprefixtagName === "package",<metadata>viagetElementsByTagName<opf:package>was rejected outright<meta refines="#isbn-id">pointing at nothingidentifier-typerefinement was never found → duplicate ISBN appended, carryingxmlns=""<package unique-identifier="pub-id">left danglingBug 4 concretely, patching a document whose ISBN is typed by refinement:
The fix
Every one of these is the same root cause: the writer decided which element to edit by re-reading the DOM itself, instead of asking the reader. So it does that now.
parseOpfalready handles prefixed packages,<identifier>left unprefixed under a default DC namespace, all three spellings of the scheme attribute, andunique.resolveArchiveMetadataalready resolves EPUB 3identifier-typerefinements through the ONIX codelist 5 table. Pairing the two gives the writer every decision it had been making by hand, from the code that owns those decisions — so bugs 2, 3, 4 and 5 stop being possible rather than being separately patched, and the two sides cannot drift.The first commit fixes the bugs with the logic written out by hand; the second replaces that with the delegation above. Kept as two commits because the first is where each bug and its test are legible.
What stays genuinely local to the writer:
createElementNS(DC_NAMESPACE, …)andsetAttributeNS(OPF_NAMESPACE, "opf:scheme", …), so the output can be re-parsed (bug 1). The reader has no say here; it never writes.parseOpfreturns no element handles: byidwhere the document gives one, falling back to position among the identifier elements otherwise — the only handle an anonymous identifier has, and the order the parser reported it in.unique-identifierelement. It is structural, and untagging it instead would buy nothing since the reader infersISBNfrom a bare ISBN value.Why this is separate
Extracted from #586, which replaces the single ISBN field with a list of scheme-tagged identifiers. These five are defects in the writer that exists today — different review criteria from the feature, and independently revertible. #586 is stacked on this branch.
Worth stating plainly: extracting this did not shrink #586 much (−89 insertions), because #586 replaces this writer's entry points rather than building on them. The value here is that five corruption bugs land and are revertible on their own.
Testing
archive-metadata: 101 tests pass; tsc and biome clean🤖 Generated with Claude Code