Repository navigation
feat: support atomic cross-slide element updates through the public API - #291
ChristopherVR merged 5 commits into
Conversation
ChristopherVR
left a comment
There was a problem hiding this comment.
Nice extraction: element-update-batch.ts is a clean pure-decision-function (validate/no-op-detect without mutating, then a thin commitElementUpdateBatch adapter each binding wires to its own history), and Angular/Vanilla reuse existing proven commit paths (applyReplacement, pushHistory+store.set) rather than inventing new ones. Traced the React flushSync/ref choreography against the existing history-tracking effect and label propagation through undo/redo — looks correctly synchronized (the shared cloneHistorySnapshot drops actionLabel, which is what stops a batch's label leaking into a later unrelated edit). Left a few inline notes below, nothing blocking.
| return; | ||
| } | ||
| // Expose the public handle for development and cross-binding API tests. | ||
| Object.defineProperty(window, '__pptxViewer', { |
There was a problem hiding this comment.
This window.__pptxViewer dev-hook is added identically (modulo syntax) in this file, demos/demo-vue/src/App.vue, demos/demo-angular/src/app.component.ts, and demos/demo-svelte/src/App.svelte. The actual logic (Object.defineProperty(window, '__pptxViewer', { get }) + Reflect.deleteProperty cleanup) has no framework dependency — could this be one small shared helper (e.g. installDevViewerHandle(getHandle)) that each binding's lifecycle hook just calls, instead of four copies?
Separately: demos/demo-vanilla/src/main.ts:185 already does window.__pptxViewer = viewer, but unconditionally, not gated by import.meta.env.DEV like these four new ones. After this PR, 1 of 5 demos leaks the live handle in production builds and 4 don't. Since these demos are the ones embedded live on the docs site, worth reconciling (either gate vanilla's too, or note why the asymmetry is fine).
| state.shapeAdjustmentDragStateRef.current || | ||
| state.isDrawingRef.current | ||
| ) { | ||
| throw new Error('Finish the current pointer interaction before updating elements.'); |
There was a problem hiding this comment.
This pointer-interaction guard (reusing the same refs as the existing hasActivePointerInteraction() gate for the ordinary history-tracking effect) only exists in React's useElementUpdateBatch. commitElementUpdateBatch — the path Vue, Angular, Svelte, and Vanilla all go through — has no equivalent check.
Is that safe because those four bindings apply the live drag/resize/marquee preview outside the authoritative slides store and only commit it at pointer-up (so an intervening updateElements just operates on the last-committed, pre-gesture state)? If so a one-line comment on commitElementUpdateBatch would help; if not, calling updateElements mid-drag in Vue/Angular/Svelte/Vanilla might be able to race the gesture's own commit in a way this PR doesn't test for.
| commitElementUpdateBatch(updates, options, { | ||
| getTarget: () => ({ | ||
| canEdit: deps.canEdit(), | ||
| mode: editor.masterViewTarget ? 'master' : deps.getMode(), |
There was a problem hiding this comment.
deps.getMode() here is wired to derived.viewerMode (see create-viewer-state-derived.svelte.ts), which already resolves to 'master' whenever editor.masterViewTarget is set:
const viewerMode = $derived<ViewerMode>(
editor.masterViewTarget
? 'master'
: viewer.isFullscreen
? 'present'
: deps.getEditable()
? 'edit'
: 'preview',
);So editor.masterViewTarget ? 'master' : deps.getMode() is redundant (the left branch can never differ from what the right branch would already return). Not wrong, just dead/confusing — Angular's equivalent trusts getMode() alone. Could probably just be mode: deps.getMode().
Read live gesture state in every binding before touching pending text, slides, or history, and release canceled gestures so batching can resume. Share the development-only demo handle lifecycle and simplify the Svelte mode check. Co-Authored-By: Codex <noreply@openai.com>
Reproduce a live Vue drag racing with a cross-slide batch. Exercise move and resize rejection, cancellation recovery, and separate undo entries in all five demos. Verify production handle gating and ownership-aware cleanup. Co-Authored-By: Codex <noreply@openai.com>
|
Thanks for the review! I pushed follow-up commits
Validation: 225 targeted tests passed, including React 18 and the demo-handle lifecycle tests; 75 browser tests passed across all five bindings. The browser matrix covers move/resize while held, normal release, cancellation, and independent gesture/batch undo steps. Build, typecheck, lint, formatting, and the framework-neutral E2E contract check passed locally. |
What does this change?
Adds
updateElements(updates, { label? })to the public viewer API across all five bindings. A host action can update titles on several slides without navigating, then undo or redo the whole action in one step.For the two-slide report in #287, a batch changes title positions from
[67, 67]to[84, 90]; one undo restores[67, 67]. The existingupdateElementremains a current-slide edit. This adds an explicit batch boundary rather than treating separate edits as one operation implicitly.Fixes #287.
Type of change
Cross-binding parity
Validation, target resolution, patch copying, and no-op detection live in
pptx-viewer-shared. Each binding commits through its own history implementation. No new UI controls or translated UI labels are introduced.Testing
element-update-batchesandinspector-undo: 75 passed across all five bindings using local Chrome. Includes active move/resize rejection before and after movement, release/cancel recovery, and separate gesture/batch undo steps.updateElementsresolved while a gesture was still active.Checks run locally
bun run lintbun run fmt:checkbun run typecheckbun run build(plus final rebuilds for the affected bindings)bun run e2e:contractConventional Commits