Skip to content

fix(headers): surface tracked changes inside header/footer parts - #28

Open
xsvm wants to merge 2 commits into
kklimuk:mainfrom
xsvm:fix/marginal-revisions
Open

xsvm wants to merge 2 commits into
kklimuk:mainfrom
xsvm:fix/marginal-revisions

Conversation

@xsvm

@xsvm xsvm commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Summary

A tracked change inside a page header or footer was invisible to an agent. Repro (header saved by Word 365): <w:r><w:t>Header text</w:t></w:r><w:ins …><w:r><w:t> edited</w:t></w:r></w:ins>:

Before Cause
headers list and the docx:header hint read just Header text marginalText only looked inside <w:hyperlink>, so it skipped <w:ins>
track-changes list printed no tracked changes, so accept/reject couldn't reach it the reader registered revisions from the body and notes only, never from header/footer parts

The fix reuses the existing paths

  • readMarginalTrackedChanges (ast/read.ts) runs after readNoteTrackedChanges and registers each header/footer part's run-level ins/del/moveFrom/moveTo through the existing registerNoteRunChanges, which now takes a blockId. The blockId is the marginal's hdrN/ftrN, so list says where the change is. A part shared by several sections is registered once. tcN ids still come from the one walk, continuing after the notes.
  • marginalText decides what to show with the existing isWrapperVisibleInView(tag, "accepted"): insertions show and deletions stay hidden.
  • Accept/reject needed no changes: apply edits the part tree in place and MarginalsView.writeTo saves it. A paragraph-mark lookup by hdrN finds nothing, so these stay run-level.
  • list reads a note or header wrapper's text from its own runs (wrapperRunText). Before this, note-body edits showed as "¶ paragraph mark".
  • computeMaxRevisionId also scans header/footer parts, so a newly created revision id can't collide with one already in a header.

After:

2 tracked changes (1 logical):

  rev0  replace  hdr0  "old" → "edited"

accept --all gives Header text edited and reject --all gives Header text old. Both results pass docx validate.

Not covered: revisions inside a table in a header, paragraph-mark revisions in a header, and <w:rPrChange> (already documented as out of scope).

Note: this overlaps with #27 in list-view.ts, since both fill in list text for runs the body loop doesn't see. Whichever merges second needs a small rebase.

Test plan

  • bun run check
  • 4 new tests in tests/cli/headers-footers.test.ts: read view, list, accept, reject
  • headers / track-changes / footnotes / info / output-contract suites pass
  • Full bun test: the only failures are the existing Windows-only ones (benchmark metrics timeout, render /bin/sh probe, compiled-binary upgrade)

A reviewer's <w:ins>/<w:del> inside word/header{N}.xml / footer{N}.xml was
invisible: headers list and the docx:header hint dropped the insertion,
track-changes list didn't enumerate it, so accept/reject couldn't reach it.

- read.ts: readMarginalTrackedChanges registers run-level revisions in each
  header/footer part (once per part) after the notes, blockId = hdrN/ftrN.
- marginalText reads the accepted view via isWrapperVisibleInView.
- list reads a note/marginal wrapper's text off its own runs.
- computeMaxRevisionId scans the header/footer parts too.
@kklimuk

kklimuk commented Oct 28, 2026

Copy link
Copy Markdown
Owner

🤖 docx-cli triage

Thanks @xsvm, this is a real gap and the fix goes in the right place. I reviewed 2ce00d2 and have a follow-up commit ready, but the push to your branch was rejected by my environment's git proxy (it isn't authorized for xsvm/docx-cli). The details are below so the maintainer can apply it.

What the PR claims

A tracked <w:ins>/<w:del> inside word/header{N}.xml/footer{N}.xml was invisible to headers list, the docx:header hint, and track-changes list/accept/reject. The PR registers these revisions as tcN ids with blockId hdrN/ftrN and reads header text in the accepted view.

Before/after repro

Setup: create → headers set --text "Header text" → raw part edit word/header1.xml adding <w:ins> edited</w:ins><w:del> old</w:del>.

origin/main PR
headers list text Header text Header text edited
track-changes list no tracked changes rev0 replace hdr0 "old" → "edited"
accept --all 0 changes, revisions stay in header1.xml 2 changes → Header text edited
reject --all 0 changes 2 changes → Header text old

Other results:

  • apply --accept tc0,tc1 --reject rev0 works on a doc that has both body and header changes, and the output passes docx validate.
  • New body revisions now get ids 903/904, after the header's 901/902, so they no longer collide with ids already in the header.
  • Footnote-body edits now list real text ("Note body" → "Note body changed"). Before, they showed as ¶ → ¶.

bun run check ✅ and bun run test:unit ✅ on the PR as-is (1913 tests).

The follow-up commit (b574e71, not pushed)

Bugs I found and reproduced through the CLI, each covered by a new test:

  1. Text-box revisions were counted twice. A revision inside a header text box (mc:AlternateContent, with the same story in Choice and Fallback) was listed twice. Rejecting the Fallback copy exited 0, then the save-time Choice→Fallback mirror silently undid it. Now only the chosen branch is registered.
  2. Revisions inside block-level content controls and tables were missed. Word's page-number gallery wraps the footer paragraph in a block-level <w:sdt>. Revisions there (and in header tables) were never listed, so accept --all reported 0 changes, exited 0, and left the <w:ins> in place.
  3. Edits in different paragraphs were paired as one replace. Every header revision shares the part's hdrN blockId, so a deletion in one header paragraph and an insertion in another were paired into a single revN replace that accept --at revN would apply as one decision. Pairing now requires the same container. This also applies to note bodies, whose blockId is "".

Cleanups:

  • wrapperRunText was identical to collectGlyphText in task-list/detect.ts, so I moved it to core/parser/run-ops.ts and both callers share it.
  • registerNoteRunChanges is renamed registerStandaloneRunChanges.
  • Marginal refs are resolved once per read instead of twice.
  • nodeText now descends wrappers via wrapperContent, per src/core/CLAUDE.md.
  • Updated doc comments.

The commit touches 10 files under src/ and tests/, +217/−72, and all 1916 unit tests pass.

Docs: the PR's src/cli/track-changes/CLAUDE.md paragraph puts wrapperRunText "in list-view.ts" and says registration covers paragraphs only. With the follow-up commit, both are out of date: the helper now lives in core/parser/run-ops.ts, and registration also walks block-level sdt, tables and AlternateContent. That paragraph needs a one-line update. Otherwise the CLAUDE.md changes match the code.

Open product questions

  • Do we want this at all? It's a standalone PR with no issue behind it. I think yes: Word does put reviewer edits in header parts, and before this PR accept --all silently left them in place.
  • Header text ignores the read view. read --baseline and read --current still show the accepted header text (Header text edited), which breaks the documented rule that read --baseline matches reject --all. Should Marginal.text follow the view, or should the docx:header hint flag pending revisions (e.g. revisions="2")? As it stands, read gives no sign that a header has tracked changes. Only track-changes list shows them.
  • Some revision types are still out of scope. Paragraph-mark revisions (<w:pPr><w:rPr><w:ins/>) and pPrChange/rPrChange in headers are not registered, so accept --all can exit 0 with revisions still pending in a header. Is that OK to ship, or should it be a follow-up issue?
  • Text boxes in footnotes/endnotes. Note parts aren't Fallback-synced on save. After the dedup fix, a revision in a footnote text box is accepted or rejected in the Choice copy only, so the Fallback keeps a stale wrapper. This is rare and Word reads Choice, but it is a behavior change.
  • No new fixture. This is a read-side fix for a shape Word writes, and there is no CLI verb that authors header revisions. The tests build it with raw part edit. Is that enough, or do you want a header revision added to headers-footers.docx so it goes through the LibreOffice round-trip?
  • Overlap with fix(track-changes): show tracked equations, images and line breaks #27. This PR overlaps fix(track-changes): show tracked equations, images and line breaks #27 in list-view.ts, and whichever merges second needs a rebase.

Action needed: routine blocked. My push of b574e71 to xsvm/docx-cli:fix/marginal-revisions was rejected by the session's git proxy (fork not in the authorized repo set). Either authorize the fork and re-run triage, or apply the patch yourself, then answer the questions above. After anyone other than @xsvm pushes, the "Default" ruleset's last-push rule needs a second approver or an admin-bypass merge.


Generated by Claude Code

…within a line

- Text-box revisions in a header were listed twice (Choice and Fallback
  carry the same story); rejecting the Fallback copy exited 0 and the
  save-time Choice->Fallback mirror undid it. The run walk now descends an
  mc:AlternateContent through its chosen branch only.
- Revisions inside a block-level <w:sdt> (Word's page-number gallery) or a
  header table were never registered, so accept --all reported 0 changes
  and left them in place. registerMarginalBlockChanges walks sdt, tables
  and AlternateContent down to every paragraph.
- A note's "" and a marginal's hdrN/ftrN name a whole part, so revision
  groups there now also require a shared container: a deletion in one
  header line and an insertion in the next are no longer one revN.
- wrapperRunText moves to core/parser/run-ops.ts and replaces the identical
  collectGlyphText in task-list/detect.ts; registerNoteRunChanges is
  renamed registerStandaloneRunChanges; marginal refs resolve once per read;
  marginalText descends wrappers via wrapperContent.

Follows the triage review on kklimuk#28.
@xsvm

xsvm commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Before changing anything I reproduced all three bugs on 2ce00d2. A header text-box insertion listed as tc0 and tc1 with the same revisionId. A revision inside a block-level <w:sdt> or a header table didn't list at all, and accept --all returned applied: [] with exit 0. A <w:del> in one header line and a <w:ins> in the next were grouped as rev0. Your patch couldn't be pushed, so I reimplemented it in a822b3e. I pushed that myself, so the last-push rule doesn't get in the way:

  • Text-box duplicates: the standalone run walk now descends an mc:AlternateContent through alternateContentBranch only. A rejected Fallback copy can no longer be undone by the save-time mirror.
  • sdt / tables: a new registerMarginalBlockChanges walks w:sdt/w:sdtContent/w:tbl/w:tr/w:tc and block-level AlternateContent down to every paragraph.
  • Cross-paragraph pairing: I kept this in the CLI-layer revisionGroups. When the blockId names a whole part (a note's "" or a marginal's hdrN/ftrN), the two halves must also share a parent. Body blockIds behave as before. A replace inside one header line still pairs as one revN, and there's a test for that.
  • Cleanups: wrapperRunText moved to core/parser/run-ops.ts and replaces collectGlyphText in task-list/detect.ts. registerNoteRunChanges is now registerStandaloneRunChanges. Marginal refs are resolved once per read and passed to both readMarginals and readMarginalTrackedChanges. nodeText descends via wrapperContent. The src/cli/track-changes/CLAUDE.md paragraph is updated, including that paragraph-mark and pPrChange/rPrChange revisions in a marginal are still out of scope.

There are four new tests in headers-footers.test.ts. The three bug tests fail with the src change reverted, and the one-line pairing control passes either way. biome, knip, tsc and test:unit pass locally. The only failure is the Windows-only compiled-binary upgrade test, which also fails on main.

One related gap I noticed but left alone: marginalText still reads only the root <w:p>s, so text inside a page-number-gallery <w:sdt> doesn't show in headers list. The same is true on main, so I'd treat it as a separate issue. I'm happy to follow up there, and on the open questions (header text following the read view, paragraph-mark revisions, a fixture for header revisions) once you've decided on them.

@kklimuk

kklimuk commented Oct 29, 2026

Copy link
Copy Markdown
Owner

🤖 docx-cli triage

Thanks @xsvm. I re-reviewed a822b3e, and everything from my previous review is addressed. I didn't push anything this time.

Prior review items

Item Status
Text-box revisions listed twice, Fallback reject undone on save ✅ walk descends alternateContentBranch only; listed once, reject sticks
Revisions in block-level <w:sdt> / header tables missed ✅ registerMarginalBlockChanges
del/ins in different header lines paired as one revN ✅ part-wide blockIds ("", hdrN/ftrN) also need a shared parent; same-line replace still pairs
wrapperRunText → core/parser/run-ops.ts, replaces collectGlyphText ✅
registerNoteRunChanges → registerStandaloneRunChanges; refs resolved once; nodeText uses wrapperContent ✅
src/cli/track-changes/CLAUDE.md out of date ✅ updated, including the out-of-scope revision types. The other three CLAUDE.md edits are accurate too.

Checks

  • bun run check ✅. bun run test:unit ✅ (1917 tests). On the first run, one test timed out: the slow-pipe output-sink test, which has nothing to do with this diff. It passed on rerun.
  • The PR's 8 new header tests all fail on origin/main and pass on the PR.

Before/after (one header with a same-line replace, an sdt, a table cell, and a del and an ins on separate lines)

origin/main PR
track-changes list no tracked changes rev0 replace hdr0 "90" → "30", plus tc2–tc5 (sdt X, cell Y, gone, added) as separate decisions
accept --all 0 changes, 4 <w:ins> left in header1.xml 6 changes, 0 left
reject --all → headers list Net \nOne\nTwo Net 90\nOne gone\nTwo
docx validate (both outputs) valid valid

I also checked these on the PR:

  • A header part shared by two sections is listed once (hdr0), and accepting it clears it for hdr2 too.
  • A <w:moveTo> in a footer lists as move-to ftr0.
  • A tracked body edit after header ids up to 6 mints ids 7 and 8, so there's no collision.
  • read shows docx:header hdr0 text="Shared new".

Minor gaps (not blocking, could be follow-ups)

  • A block-level <w:sdt> inside a header text box is still skipped. registerStandaloneRunChanges skips every w:sdt, so in that case accept --all returns applied: [], exits 0, and leaves the revision in place. It's a rare shape, but the CLAUDE.md line "the walk reaches every paragraph in the part — inside block-level <w:sdt>, tables, and text boxes" claims a little more than the code does.
  • headers list text omits sdt and table content. You flagged this already, and it's the same on main. In my repro, track-changes list shows the revisions X and Y, but the header text never shows "Page X" or "Cell Y".

Open product questions (carried over; only the maintainer can answer these)

  • Do we want this at all? It's a standalone PR. I recommend yes: before it, accept --all silently left header revisions in place.
  • Header text ignores the read view. read --baseline and --current still show the accepted header text, and read gives no hint that a header has pending revisions. Should Marginal.text follow the view, or should the docx:header hint flag them (e.g. revisions="N")? This PR or a follow-up?
  • Out-of-scope revision types. Paragraph-mark and pPrChange/rPrChange revisions in headers, plus the sdt-in-text-box gap above, can still leave accept --all at exit 0 with revisions pending. OK to ship as-is, with a follow-up issue?
  • Text boxes in footnotes/endnotes. These now also register only the Choice copy, and note parts aren't Fallback-synced on save, so a decision leaves the Fallback stale. Word reads Choice. Acceptable?
  • Fixture. The tests build header revisions with raw part edit/replace, and no fixture covers them. Do you want a header revision added to headers-footers.docx so it gets the LibreOffice round-trip?
  • fix(track-changes): show tracked equations, images and line breaks #27 overlap in list-view.ts: whichever PR merges second needs a rebase.

Action needed: approve/merge — answer the questions above, then approve and merge PR #28. @xsvm made the last push, so your own approval satisfies the last-push rule.


Generated by Claude Code

@kklimuk

kklimuk commented Oct 30, 2026

Copy link
Copy Markdown
Owner

@xsvm hold off from making further changes till i've had a bit of time to read through and do my own reviews, it's gonna take me a few days.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants