Skip to content

Report the revision and comment families the profiles cannot draw (#444) - #531

Merged
JSv4 merged 1 commit into
claude/issue-443-pdf-ratchetfrom
claude/issue-444-review-comment-profiles
Aug 25, 2026
Merged

Report the revision and comment families the profiles cannot draw (#444)#531
JSv4 merged 1 commit into
claude/issue-443-pdf-ratchetfrom
claude/issue-444-review-comment-profiles

Conversation

@JSv4

@JSv4 JSv4 commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Closes #444. This is the last of four stacked PRs; it sits on top of #530, and GitHub will retarget it at main as the ones below it merge.

Why

When you export a DOCX that contains tracked changes or comments, you pick a review profile — final shows the accepted result, original the rejected one, markup shows the edits themselves — and a comment profile that decides whether comments appear inline, as endnotes, in the margin, or not at all.

That all worked. What didn't work is what happened to content none of those profiles can put on the page. It was dropped, silently. The resulting PDF looked complete, and nothing anywhere told the caller that something in the document had failed to make it across. For a review PDF that gets signed or filed, that's the worst possible failure mode: it doesn't look like a failure.

This change makes the export say what it couldn't draw.

What it now reports

Under markup, two kinds of tracked change genuinely leave no visible trace:

  • Custom XML revision ranges. The HTML converter has no handling for these at all — they survive the export untouched and simply don't appear. Reported as revision_family_not_rendered.
  • Changes to paragraph, table, section, and numbering properties. Word records "this paragraph used to be left-aligned" as a revision; the converter renders the paragraph in its current state and draws nothing to indicate it was changed. Reported as revision_property_change_not_rendered.

Under any comment profile that shows comments, two things about the shape of a comment set are lost:

  • Threading. A reply is drawn as its own standalone comment. Reported as comment_thread_flattened.
  • Resolved state. A comment someone marked resolved looks exactly like one still open. Reported as comment_resolved_state_not_rendered. This one matters more than it sounds: a settled objection reading as a live one changes what the document appears to say.

Both of these live in a side file (commentsExtended) that the converter never opens, which is why the comment text comes through fine but the structure around it doesn't.

final and original need none of the revision warnings. They rewrite the document by accepting or rejecting everything, then check that no tracked change survived — so a change that couldn't be applied stops the export outright instead of slipping through unnoticed.

What it deliberately stays quiet about

A warning is only useful if it's true, so the interesting work here was ruling things out.

Tracked cell insert/delete/merge are drawn. The converter marks the cell and ships CSS that tints an inserted cell green, tints and strikes through a deleted one, and outlines a merged one in dashed orange. Warning about those would be telling the caller something is missing when it's plainly on the page.

Run-level format changes are drawn too — "this text used to be un-bolded" gets a marked underline. This one was subtle, because the document inventory counted every property change together in a single number, run-level ones included. Reporting that number would mean a redline where a reviewer only re-formatted some text — the most ordinary kind of redline there is, and exactly what markup exists for — got warned about content that was fully visible.

So the inventory now counts the run-level subset separately, and the warning reports only the difference. That's one extra tally in a pass the inventory was already making, not a second trip through the file.

One behaviour change to be aware of

There's an existing option, unsupportedContent, which defaults to "warn" but can be set to "strict" to make the export refuse rather than warn. These four new warnings honour it.

That means a document that used to export successfully under "strict" can now fail, if it contains one of the families above. That's the intended behaviour — the whole point of "strict" is not shipping a PDF that quietly omits something — but it is a change for anyone already using it, so it's filed under Changed in the changelog rather than Added. Switching to "warn" restores the old outcome.

How it was checked

Six tests, built on small hand-written DOCX files so that each one contains exactly the element under test and nothing else.

The tests cover each warning firing with the right text and count, each staying silent when it should, the strict-mode refusal actually refusing, and one case that asserts a warning is raised exactly once rather than merely present — the export inspects the document twice when it rewrites it for final/original, and an earlier draft of this reported every comment twice as a result.

The test that matters most asserts the cell-revision claim rather than trusting it: it exports a table with an inserted and a deleted cell, then looks in the resulting HTML for the marked cells and the styling rules that colour them, alongside the absence of any warning. The evidence and the silence are pinned together, so the warning can't come back without someone deliberately breaking the proof that those cells are visible.

Each of the three behavioural fixes was also checked by deliberately reintroducing the bug and confirming a test went red — otherwise a passing suite proves nothing about tests written after the fix.

Locally: the .NET suite is 4264 passing, and the browser suite is 632 passing with 5 pre-existing failures that are screenshot comparisons needing a font this machine doesn't have.

Still open, and now tracked

Each of the four things this PR reports is a limitation it discloses rather than fixes, so each has an issue:

Closing any of those means deleting the corresponding warning and its test here.

Two of #444's acceptance criteria are also unmet and are tracked in #541: test coverage for tracked changes in headers, footers and notes and for revisions overlapping comments, and whether text extraction from a markup PDF returns deleted text without marking it as deleted.

@JSv4
JSv4 force-pushed the claude/issue-443-pdf-ratchet branch from e024082 to b295021 Compare August 21, 2026 22:28
@JSv4
JSv4 force-pushed the claude/issue-444-review-comment-profiles branch from 89897c9 to 17ee11a Compare August 21, 2026 22:28
@JSv4
JSv4 force-pushed the claude/issue-443-pdf-ratchet branch from b295021 to 05004db Compare August 21, 2026 22:32
@JSv4
JSv4 force-pushed the claude/issue-444-review-comment-profiles branch from 17ee11a to bac1877 Compare August 21, 2026 22:32
@JSv4
JSv4 force-pushed the claude/issue-443-pdf-ratchet branch 2 times, most recently from a0a0f26 to 5f1858e Compare August 23, 2026 02:19
The profile vocabulary was already wired end to end -- handleProjectReviewProfile
routes final and original through accept/reject, commentProfile maps to
CommentRenderMode -- and the projection already refuses to mutate the source
package. What was missing is the part of #444 that says a profile must have
documented, deterministic semantics: the export was silently dropping revision
and comment families it cannot represent.

Two families are genuinely never drawn. A custom XML revision range has no
handling in the converter at all, and the block-level property revisions --
paragraph, table, section and numbering -- travel through the projection
untouched and leave no mark a reader can see. They now raise
revision_family_not_rendered and revision_property_change_not_rendered.

A warning is only honest if it counts what is actually missing. rPrChange is a
property revision the converter does draw, as a marked format change, so the
manifest now counts it apart as revisions.runPropertyChanges and the warning
reports the remainder. That split is one more counter in the pass the inventory
already makes, not a second pass over the package, and it is what keeps
unsupportedContent: "strict" honest -- a document whose only property revisions
are run-level format changes is drawn in full and must not fail closed. Tracked
cell insert/delete/merge are drawn too, as tinted, struck-through and dashed
cells, so they raise nothing; a test asserts the drawing and the silence
together, so the warning cannot come back without the render evidence going
with it.

final and original get none of the revision warnings. They apply the projection
and then assert the derived package retains no revisions at all, so a family
that cannot be applied fails the projection rather than passing through unseen.

Any visible comment profile renders bodies, ranges and authors but not the
topology in commentsExtended, which the converter never reads: a reply is drawn
as an independent comment and a resolved comment is drawn identically to an open
one. Both change what a review PDF means, so they are reported --
comment_thread_flattened and comment_resolved_state_not_rendered -- rather than
approximated. Comments survive the final/original projection unchanged, so these
run against the source package only; preflight sees both packages, and warning
on each would report every comment twice.

All of these route through the unsupportedContent policy, so a caller who asked
for strict output gets a closed failure instead of a warning they have to
notice. That is a behaviour change for an existing strict caller, hence the
CHANGELOG entry sits under Changed.
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