From 3bd3e29b00f2b54ad2c4689d7dd01f1fe865e7d5 Mon Sep 17 00:00:00 2001 From: Michael Hurhangee Date: Wed, 1 Jul 2026 15:06:27 +0100 Subject: [PATCH 1/2] fix(editor): persist page-setup changes on save (was dropped by selective save) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A page-setup change (size/orientation/margins) writes to the model-level finalSectionProperties, but it's not a ProseMirror transaction — so the change tracker never sees it, changedParaIds is empty, and the selective save path (the default) never re-emits the body sectPr. Result: the change was silently lost on reopen. Only a full repack serializes finalSectionProperties. Fix: usePageSetupControls flips a sectionPropsDirtyRef when page setup is applied; useFileIO ORs it into structuralChange, forcing attemptSelectiveSave to bail to a full repack (mirrors the existing hasInjectedReplies force), then resets the flag after a successful save. Regression test in selectiveSave.test.ts documents the bug (selective drops the sectPr change) and confirms the fix's mechanism (forcing structuralChange → repack persists it). Interim fix per SCRATCHPAD; the proper fix (PM-native section properties) remains. --- SCRATCHPAD.md | 20 ++++----- .../src/docx/selectiveSave.test.ts | 45 +++++++++++++++++++ .../src/editor/docx-editor.tsx | 5 +++ .../src/editor/fileOpen.test.tsx | 2 + .../src/editor/lifecycle/use-file-io.ts | 18 ++++++-- .../page-setup/use-page-setup-controls.ts | 10 ++++- 6 files changed, 86 insertions(+), 14 deletions(-) diff --git a/SCRATCHPAD.md b/SCRATCHPAD.md index 20555db3..fa233c05 100644 --- a/SCRATCHPAD.md +++ b/SCRATCHPAD.md @@ -23,15 +23,15 @@ re-add properly inside the owning feature slice rather than restoring the dead p ## Page setup lives outside ProseMirror Page setup (size/orientation/margins) is a model-level doc attribute (`finalSectionProperties`, no PM representation), applied via `usePageSetupControls` → `handleDocumentChange`. Two consequences: -- `Page-setup changes are dropped by selective save` **[high — data loss]**: the default save is - *selective* (`useFileIO` → `agent.toBuffer({ selective })`), and `attemptSelectiveSave` - (`docx/selectiveSave.ts`) only patches the `changedParaIds` it gets from the PM editor state and - never re-emits the body sectPr. A page-setup change is not a PM transaction, so `changedParaIds` - is empty and the original sectPr is kept → the margin/orientation/size change is NOT written to - the `.docx`. Only a full repack (`selective:false` → `repackDocx`, which serializes - `finalSectionProperties`, documentSerializer.ts:149-150) persists it. Confirmed by inspection + - user report. *Interim fix: force a full repack when section properties changed (mirror the - `hasInjectedReplies` force in `useFileIO`). Proper fix: make section properties PM-native.* +- `Page-setup changes are dropped by selective save` **[FIXED — interim, 2026-07-01]**: the default + save is *selective* (`useFileIO` → `agent.toBuffer({ selective })`), and `attemptSelectiveSave` + only patches `changedParaIds` and never re-emits the body sectPr; a page-setup change isn't a PM + transaction so `changedParaIds` is empty → the margin/orientation/size change was NOT written to + the `.docx`. **Interim fix shipped:** `usePageSetupControls` flips a `sectionPropsDirtyRef`, which + `useFileIO` reads to force `structuralChange` → full repack (mirrors the `hasInjectedReplies` + force); regression test in `selectiveSave.test.ts`. **Proper fix still open:** make section + properties PM-native (a `doc` attribute → undoable transaction the change tracker sees), which + also resolves the undo-stack point below and lets selective save persist them without a full repack. - `Page setup not on the Ctrl+Z undo stack` **[low — working-as-intended]**: not revertible via Ctrl+Z (PM undo only covers content transactions) — treated as a doc attribute, not content (deliberate; the old global-keydown history that reverted it was the A1 bug, removed in slice 1). @@ -96,7 +96,7 @@ one uniform `--docx-*` token namespace, single-source; the host `index.css` wire Deferred items surfaced during it: - `DEAD CSS SWEEP`: `styles/editor.css` `.paged-editor__decoration-overlay` + `.ProseMirror-yjs-cursor` reference the removed React PluginHost/`DecorationLayer` + y-prosemirror; and 5 dead `.docx-editor-vue__pages-viewport` scrollbar selectors (no Vue adapter). Verify no DOM emits these, then cut. - `DEAD-EXPORT SWEEP of docx-editor-core/src/utils`: `selectionHighlight.ts` turned out ENTIRELY dead (~578 lines, removed in #162); knip is exempt for the vendored editor, so other `utils/*` modules likely hide large unused surfaces. Worth a dedicated pass. -- `OVERLAY ACCENT COLOUR` (design, not refactor): the selection/image overlay accents are Google-blue (`--docx-selection-rect` / `--docx-image-accent`), while text-selection + chrome are emerald (`--primary`-derived). Now tokens → aligning them to emerald is a one-line change. *Michael's call.* +- (Overlay accent colour: DECIDED — keep the selection/image accents a distinct blue, not emerald; tokenised as `--docx-selection-rect` / `--docx-image-accent`, self-documented in tokens.css. No action.) ## Provenance / docs framing (one pass once the rework settles) diff --git a/packages/docx-editor-core/src/docx/selectiveSave.test.ts b/packages/docx-editor-core/src/docx/selectiveSave.test.ts index e9d14e99..6e0ca405 100644 --- a/packages/docx-editor-core/src/docx/selectiveSave.test.ts +++ b/packages/docx-editor-core/src/docx/selectiveSave.test.ts @@ -977,3 +977,48 @@ describe('attemptSelectiveSave — watermark header parts', () => { expect(result).toBeNull(); }); }); + +// ============================================================================ +// Page setup (body section properties) in selective save +// ============================================================================ + +describe('attemptSelectiveSave — page setup / section properties', () => { + test('a page-setup change is dropped by selective save but persisted by a full repack', async () => { + const buffer = await loadFixture('example-with-image.docx'); + const doc = await parseDocx(buffer, { preloadFonts: false }); + + const sectPr = doc.package.document.finalSectionProperties; + if (!sectPr || typeof sectPr.pageWidth !== 'number') { + console.log('Fixture has no numeric sectPr pageWidth, skipping'); + return; + } + const originalWidth = sectPr.pageWidth; + const newWidth = originalWidth + 720; // a page-setup change (0.5in wider) + sectPr.pageWidth = newWidth; + + // The bug: a page-setup change is not a paragraph edit, so a selective save + // (empty changedParaIds, no structural change) never re-emits the body sectPr + // — document.xml is untouched, so the change is silently dropped on reopen. + const selective = await attemptSelectiveSave(doc, buffer, { + changedParaIds: new Set(), + structuralChange: false, + hasUntrackedChanges: false, + }); + expect(selective).not.toBeNull(); + const afterSelective = await parseDocx(selective!, { preloadFonts: false }); + expect(afterSelective.package.document.finalSectionProperties?.pageWidth).toBe(originalWidth); + + // The fix: useFileIO forces structuralChange when page setup changed, so + // attemptSelectiveSave bails and the caller repacks — which serializes the + // updated sectPr, so the change survives the round-trip. + const bailed = await attemptSelectiveSave(doc, buffer, { + changedParaIds: new Set(), + structuralChange: true, + hasUntrackedChanges: false, + }); + expect(bailed).toBeNull(); + const repacked = await repackDocx(doc); + const afterRepack = await parseDocx(repacked, { preloadFonts: false }); + expect(afterRepack.package.document.finalSectionProperties?.pageWidth).toBe(newWidth); + }); +}); diff --git a/packages/docx-editor-react/src/editor/docx-editor.tsx b/packages/docx-editor-react/src/editor/docx-editor.tsx index b057043a..bdf953be 100644 --- a/packages/docx-editor-react/src/editor/docx-editor.tsx +++ b/packages/docx-editor-react/src/editor/docx-editor.tsx @@ -226,6 +226,9 @@ export const DocxEditor = forwardRef(function Do // Refs (pagedEditorRef is declared earlier — useCommentManagement needs it) const agentRef = useRef(null); const containerRef = useRef(null); + // Set when page setup changes (model-level, not a PM transaction); read by the + // save path to force a full repack so the body sectPr actually persists. + const sectionPropsDirtyRef = useRef(false); // Save the last known selection for restoring after toolbar interactions const lastSelectionRef = useRef<{ from: number; to: number } | null>(null); const editorContentRef = useRef(null); @@ -336,6 +339,7 @@ export const DocxEditor = forwardRef(function Do loadBuffer, getActiveEditorView, focusActiveEditor, + sectionPropsDirtyRef, }); // Mirror PM state on each external document load (mount-time view creation @@ -566,6 +570,7 @@ export const DocxEditor = forwardRef(function Do document: docState.state, readOnly, handleDocumentChange, + sectionPropsDirtyRef, }); const { diff --git a/packages/docx-editor-react/src/editor/fileOpen.test.tsx b/packages/docx-editor-react/src/editor/fileOpen.test.tsx index 8e4bf866..0f3d378f 100644 --- a/packages/docx-editor-react/src/editor/fileOpen.test.tsx +++ b/packages/docx-editor-react/src/editor/fileOpen.test.tsx @@ -33,6 +33,7 @@ function FileIOHarness({ const agentRef = useRef(null); const pagedEditorRef = useRef(null); const containerRef = useRef(null); + const sectionPropsDirtyRef = useRef(false); const fileIO = useFileIO({ agentRef, pagedEditorRef, @@ -42,6 +43,7 @@ function FileIOHarness({ loadBuffer, getActiveEditorView: () => null, focusActiveEditor: () => {}, + sectionPropsDirtyRef, }); return ( diff --git a/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts b/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts index 0524063c..d3f0e2cd 100644 --- a/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts +++ b/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts @@ -35,6 +35,7 @@ export function useFileIO({ loadBuffer, getActiveEditorView, focusActiveEditor, + sectionPropsDirtyRef, }: { agentRef: React.RefObject; pagedEditorRef: React.RefObject; @@ -44,6 +45,8 @@ export function useFileIO({ loadBuffer: (buffer: DocxInput) => Promise; getActiveEditorView: () => EditorView | null | undefined; focusActiveEditor: () => void; + /** Set when page setup changed — forces a full repack so the body sectPr persists. */ + sectionPropsDirtyRef: React.RefObject; }) { const imageInputRef = useRef(null); const docxInputRef = useRef(null); @@ -83,12 +86,17 @@ export function useFileIO({ // Force full repack if any reply comments exist (both comment replies and // tracked-change replies need range markers injected into document.xml, // which selective save can't handle since the affected paragraphs may not - // be in changedParaIds) + // be in changedParaIds), or if page setup changed. A page-setup change + // isn't a PM transaction, so it's not in changedParaIds and selective + // save never re-emits the body sectPr — only a full repack persists it. const hasInjectedReplies = comments.some((c) => c.parentId != null); selectiveOptions = { selective: { changedParaIds: getChangedParagraphIds(editorState), - structuralChange: hasStructuralChanges(editorState) || hasInjectedReplies, + structuralChange: + hasStructuralChanges(editorState) || + hasInjectedReplies || + sectionPropsDirtyRef.current, hasUntrackedChanges: hasUntrackedChanges(editorState), }, }; @@ -101,12 +109,16 @@ export function useFileIO({ view.dispatch(clearTrackedChanges(view.state)); } + // The save persisted the current state (a full repack when page setup + // changed) into originalBuffer, so the sectPr is now baked in. + if (buffer !== null) sectionPropsDirtyRef.current = false; + return buffer; } catch { return null; } }, - [agentRef, pagedEditorRef, comments] + [agentRef, pagedEditorRef, comments, sectionPropsDirtyRef] ); const handleDirectPrint = useCallback(() => { diff --git a/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.ts b/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.ts index 633dcc0a..e9c4c2df 100644 --- a/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.ts +++ b/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.ts @@ -9,15 +9,22 @@ import type { Document, SectionProperties } from '@eigenpal/docx-editor-core/typ * not on the Ctrl+Z undo stack (`finalSectionProperties` is a model-level field * with no ProseMirror representation; PM's undo covers content transactions). * Re-open the dialog to change it back. + * + * A page-setup change is NOT a ProseMirror transaction, so the change tracker + * never sees it and a selective save (which patches only changed paragraphs) + * would drop it — the body `sectPr` is only re-emitted by a full repack. So we + * flip `sectionPropsDirtyRef`, which `useFileIO` reads to force a full repack. */ export function usePageSetupControls({ document, readOnly, handleDocumentChange, + sectionPropsDirtyRef, }: { document: Document | null; readOnly: boolean; handleDocumentChange: (doc: Document) => void; + sectionPropsDirtyRef: React.RefObject; }) { const [showPageSetup, setShowPageSetup] = useState(false); const handleOpenPageSetup = useCallback(() => setShowPageSetup(true), []); @@ -38,9 +45,10 @@ export function usePageSetupControls({ }, }, }; + sectionPropsDirtyRef.current = true; handleDocumentChange(newDoc); }, - [document, readOnly, handleDocumentChange] + [document, readOnly, handleDocumentChange, sectionPropsDirtyRef] ); return { showPageSetup, setShowPageSetup, handleOpenPageSetup, handlePageSetupApply }; From 290f3ca39e122052b5a81fb7868cfabe919ab78e Mon Sep 17 00:00:00 2001 From: Michael Hurhangee Date: Wed, 1 Jul 2026 16:25:46 +0100 Subject: [PATCH 2/2] fix(editor): consume the page-setup dirty flag synchronously (close a save race) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #168 found the flag was reset AFTER the awaited toBuffer: a page-setup change applied during an in-flight autosave would have its dirty flag clobbered by the late reset and get dropped on the next save. Consume + reset the flag synchronously before the await (a set flag already forces this save to repack), and restore it if the save fails. Mirrors the mutateChart no-await-between rule. Also add a usePageSetupControls test guarding the producer side of the wiring (applying page setup dirties the flag; a read-only apply does not) — the earlier core test only covered the selective-save mechanism, not the React wiring. --- .../src/editor/lifecycle/use-file-io.ts | 15 ++++-- .../use-page-setup-controls.test.tsx | 54 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.test.tsx diff --git a/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts b/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts index d3f0e2cd..77d13a34 100644 --- a/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts +++ b/packages/docx-editor-react/src/editor/lifecycle/use-file-io.ts @@ -55,6 +55,12 @@ export function useFileIO({ async (options?: { selective?: boolean }): Promise => { if (!agentRef.current) return null; + // Consume the page-setup dirty flag SYNCHRONOUSLY, before any await — a + // change applied during an in-flight save would otherwise be clobbered by + // a post-await reset (a set flag already forces this save to full-repack). + const sectionPropsChanged = sectionPropsDirtyRef.current; + sectionPropsDirtyRef.current = false; + try { const agentDoc = agentRef.current.getDocument(); @@ -96,7 +102,7 @@ export function useFileIO({ structuralChange: hasStructuralChanges(editorState) || hasInjectedReplies || - sectionPropsDirtyRef.current, + sectionPropsChanged, hasUntrackedChanges: hasUntrackedChanges(editorState), }, }; @@ -109,12 +115,11 @@ export function useFileIO({ view.dispatch(clearTrackedChanges(view.state)); } - // The save persisted the current state (a full repack when page setup - // changed) into originalBuffer, so the sectPr is now baked in. - if (buffer !== null) sectionPropsDirtyRef.current = false; - return buffer; } catch { + // The save failed, so the sectPr change wasn't persisted — restore the + // dirty flag so the next save still forces a full repack. + if (sectionPropsChanged) sectionPropsDirtyRef.current = true; return null; } }, diff --git a/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.test.tsx b/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.test.tsx new file mode 100644 index 00000000..e2941ab0 --- /dev/null +++ b/packages/docx-editor-react/src/features/page-setup/use-page-setup-controls.test.tsx @@ -0,0 +1,54 @@ +import { GlobalRegistrator } from '@happy-dom/global-registrator'; +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; + +beforeAll(() => GlobalRegistrator.register()); +afterAll(() => GlobalRegistrator.unregister()); + +import { renderHook } from '@testing-library/react'; +import { useRef } from 'react'; +import type { Document } from '@eigenpal/docx-editor-core/types/document'; +import { usePageSetupControls } from './use-page-setup-controls'; + +function makeDoc(): Document { + return { + package: { document: { finalSectionProperties: { pageWidth: 12240 } } }, + } as unknown as Document; +} + +function setup(readOnly: boolean) { + const changes: Document[] = []; + const { result } = renderHook(() => { + const sectionPropsDirtyRef = useRef(false); + const api = usePageSetupControls({ + document: makeDoc(), + readOnly, + handleDocumentChange: (d) => changes.push(d), + sectionPropsDirtyRef, + }); + return { api, sectionPropsDirtyRef }; + }); + return { result, changes }; +} + +describe('usePageSetupControls', () => { + test('applying page setup marks section props dirty (so the save forces a full repack)', () => { + const { result, changes } = setup(false); + + result.current.api.handlePageSetupApply({ pageWidth: 15840 }); + + // The dirty flag is what useFileIO reads to force a full repack — without it + // the selective save silently drops the sectPr change. + expect(result.current.sectionPropsDirtyRef.current).toBe(true); + expect(changes).toHaveLength(1); + expect(changes[0].package.document.finalSectionProperties?.pageWidth).toBe(15840); + }); + + test('a read-only apply is a no-op and never dirties the flag', () => { + const { result, changes } = setup(true); + + result.current.api.handlePageSetupApply({ pageWidth: 15840 }); + + expect(result.current.sectionPropsDirtyRef.current).toBe(false); + expect(changes).toHaveLength(0); + }); +});