diff --git a/packages/excalidraw/tests/transaction.test.tsx b/packages/excalidraw/tests/transaction.test.tsx index 91d16038e4..361d306c88 100644 --- a/packages/excalidraw/tests/transaction.test.tsx +++ b/packages/excalidraw/tests/transaction.test.tsx @@ -9,6 +9,7 @@ import { Excalidraw } from "../index"; import { TransactionLedger, DEFAULT_TRANSACTION_MERGE_POLICY, + TRANSACTION_MERGE_MODES, collectChangedElementIds, } from "../transaction"; @@ -175,58 +176,119 @@ describe("TransactionLedger", () => { expect(elementsAfter.get(live.id)?.strokeColor).toBe("#ff006e"); }); - it("applies per-prop conflict handling and supports element-scope skip", () => { + it("supports all winner/scope combinations for conflicting updates", () => { const ledger = new TransactionLedger(); const baseline = API.createElement({ type: "rectangle", id: "rect-1", strokeColor: "#000000", backgroundColor: "#ffffff", + x: 0, }); const target = { ...baseline, strokeColor: "#ff006e", - backgroundColor: "#ffd8a8", + x: 200, version: baseline.version + 1, }; const live = { ...target, strokeColor: "#3a86ff", - backgroundColor: "#ffd8a8", + backgroundColor: "#00f5d4", version: target.version + 1, }; ledger.recordStep(arrayToMap([baseline]), arrayToMap([target])); - const propScope = ledger.buildSyntheticSnapshots( + const liveProp = ledger.buildSyntheticSnapshots( arrayToMap([live]), DEFAULT_TRANSACTION_MERGE_POLICY, ); - expect(propScope.elementsBefore.get(live.id)?.strokeColor).toBe("#3a86ff"); - expect(propScope.elementsAfter.get(live.id)?.strokeColor).toBe("#3a86ff"); - expect(propScope.elementsBefore.get(live.id)?.backgroundColor).toBe( - "#ffffff", + expect(liveProp.elementsBefore.get(live.id)?.strokeColor).toBe("#3a86ff"); + expect(liveProp.elementsAfter.get(live.id)?.strokeColor).toBe("#3a86ff"); + expect(liveProp.elementsBefore.get(live.id)?.x).toBe(0); + expect(liveProp.elementsAfter.get(live.id)?.x).toBe(200); + expect(liveProp.elementsBefore.get(live.id)?.backgroundColor).toBe( + "#00f5d4", ); - expect(propScope.elementsAfter.get(live.id)?.backgroundColor).toBe( - "#ffd8a8", + expect(liveProp.elementsAfter.get(live.id)?.backgroundColor).toBe( + "#00f5d4", ); - const elementScope = ledger.buildSyntheticSnapshots(arrayToMap([live]), { - ...DEFAULT_TRANSACTION_MERGE_POLICY, + const liveElement = ledger.buildSyntheticSnapshots(arrayToMap([live]), { + conflictWinner: "live", conflictScope: "element", }); - expect(elementScope.elementsBefore.get(live.id)?.strokeColor).toBe( - "#3a86ff", + expect(liveElement.elementsBefore.get(live.id)?.strokeColor).toBe("#3a86ff"); + expect(liveElement.elementsAfter.get(live.id)?.strokeColor).toBe("#3a86ff"); + expect(liveElement.elementsBefore.get(live.id)?.x).toBe(200); + expect(liveElement.elementsAfter.get(live.id)?.x).toBe(200); + expect(liveElement.elementsBefore.get(live.id)?.backgroundColor).toBe( + "#00f5d4", ); - expect(elementScope.elementsAfter.get(live.id)?.strokeColor).toBe( - "#3a86ff", + expect(liveElement.elementsAfter.get(live.id)?.backgroundColor).toBe( + "#00f5d4", ); - expect(elementScope.elementsBefore.get(live.id)?.backgroundColor).toBe( - "#ffd8a8", + + const transactionProp = ledger.buildSyntheticSnapshots(arrayToMap([live]), { + conflictWinner: "transaction", + conflictScope: "prop", + }); + expect(transactionProp.elementsBefore.get(live.id)?.strokeColor).toBe( + "#000000", ); - expect(elementScope.elementsAfter.get(live.id)?.backgroundColor).toBe( - "#ffd8a8", + expect(transactionProp.elementsAfter.get(live.id)?.strokeColor).toBe( + "#ff006e", ); + expect(transactionProp.elementsBefore.get(live.id)?.x).toBe(0); + expect(transactionProp.elementsAfter.get(live.id)?.x).toBe(200); + expect(transactionProp.elementsBefore.get(live.id)?.backgroundColor).toBe( + "#00f5d4", + ); + expect(transactionProp.elementsAfter.get(live.id)?.backgroundColor).toBe( + "#00f5d4", + ); + + const transactionElement = ledger.buildSyntheticSnapshots( + arrayToMap([live]), + { + conflictWinner: "transaction", + conflictScope: "element", + }, + ); + expect(transactionElement.elementsBefore.get(live.id)?.strokeColor).toBe( + "#000000", + ); + expect(transactionElement.elementsAfter.get(live.id)?.strokeColor).toBe( + "#ff006e", + ); + expect(transactionElement.elementsBefore.get(live.id)?.x).toBe(0); + expect(transactionElement.elementsAfter.get(live.id)?.x).toBe(200); + expect(transactionElement.elementsBefore.get(live.id)?.backgroundColor).toBe( + "#ffffff", + ); + expect(transactionElement.elementsAfter.get(live.id)?.backgroundColor).toBe( + "#ffffff", + ); + }); + + it("maps merge modes to the same winner/scope policies", () => { + expect(TRANSACTION_MERGE_MODES["live-wins-per-prop"]).toEqual({ + conflictWinner: "live", + conflictScope: "prop", + }); + expect(TRANSACTION_MERGE_MODES["live-wins-per-element"]).toEqual({ + conflictWinner: "live", + conflictScope: "element", + }); + expect(TRANSACTION_MERGE_MODES["transaction-wins-per-prop"]).toEqual({ + conflictWinner: "transaction", + conflictScope: "prop", + }); + expect(TRANSACTION_MERGE_MODES["transaction-wins-per-element"]).toEqual({ + conflictWinner: "transaction", + conflictScope: "element", + }); }); }); @@ -609,8 +671,16 @@ describe("createTransaction", () => { Keyboard.undo(); expect(getElement(element.id)!.strokeColor).toBe(element.strokeColor); }); +}); - it.only("keeps same-element same-property user edits separated from transaction rollback", async () => { +describe("createTransaction merge policy behavior", () => { + beforeEach(async () => { + unmountComponent(); + vi.restoreAllMocks(); + await render(); + }); + + const setupSamePropertyConflictScenario = () => { const element = API.createElement({ type: "rectangle", id: "shared", @@ -622,7 +692,12 @@ describe("createTransaction", () => { setSceneBaseline([element]); expect(API.getUndoStack().length).toBe(0); - const tx = h.app.createTransaction(); + const tx = h.app.createTransaction({ + mergePolicy: { + conflictWinner: "live", + conflictScope: "prop", + }, + }); act(() => { tx.updateScene({ @@ -643,28 +718,134 @@ describe("createTransaction", () => { commitTransaction(tx); expect(API.getUndoStack().length).toBe(2); - let live = getElement(element.id)!; - // tx merge strategy for conflicting edits should prefer live values + return element; + }; + + const setupElementScopeConflictScenario = ( + options: + | Parameters[0] + | undefined = undefined, + ) => { + const element = API.createElement({ + type: "rectangle", + id: "shared-element-scope", + x: 0, + y: 0, + strokeColor: "#000", + backgroundColor: "#fff", + }); + setSceneBaseline([element]); + expect(API.getUndoStack().length).toBe(0); + + const tx = h.app.createTransaction(options); + + act(() => { + tx.updateScene({ + elements: h.app.scene.getElementsIncludingDeleted().map((el) => + el.id === element.id + ? newElementWith(el, { + strokeColor: "#f00", + x: 200, + }) + : el, + ), + }); + }); + + // non-conflicting live edit on untouched prop + applyElementUpdate( + element.id, + { backgroundColor: "#00f5d4" }, + "IMMEDIATELY", + ); + // conflicting live edit on touched prop + applyElementUpdate(element.id, { strokeColor: "#f0f" }, "IMMEDIATELY"); + + commitTransaction(tx); + + return element; + }; + + it("tx merge strategy for conflicting edits should prefer live values", () => { + const element = setupSamePropertyConflictScenario(); + + const live = getElement(element.id)!; expect(live.strokeColor).toBe("#f0f"); expect(live.x).toBe(200); expect(live.backgroundColor).toBe("#fff"); + }); + + it("undoing transaction should keep conflicting live value while rolling back tx-only props", () => { + const element = setupSamePropertyConflictScenario(); - // Undo transaction Keyboard.undo(); - live = getElement(element.id)!; + + const live = getElement(element.id)!; // strokeColor is unchanged because the transaction itself didn't end up // touching it (it kept the current live value) expect(live.strokeColor).toBe("#f0f"); - // but the x value should be reverted to pre-transaction value - expect(live.x).toBe(0); - expect(live.backgroundColor).toBe("#fff"); - - // Undo regular edit - Keyboard.undo(); - live = getElement(element.id)!; - // FIXME should revert to original strokeColor - expect(live.strokeColor).toBe("#000"); + // x should be reverted to pre-transaction value expect(live.x).toBe(0); expect(live.backgroundColor).toBe("#fff"); }); + + it("undoing regular edit after tx rollback restores pre-edit live value", () => { + const element = setupSamePropertyConflictScenario(); + + Keyboard.undo(); + Keyboard.undo(); + + const live = getElement(element.id)!; + expect(live.strokeColor).toBe("#f00"); + expect(live.x).toBe(0); + expect(live.backgroundColor).toBe("#fff"); + }); + + it("mergeMode transaction-wins-per-element commits whole-element tx snapshots on conflict", () => { + const commitSpy = vi + .spyOn(h.store, "commitSyntheticIncrement") + .mockReturnValue(true); + const element = setupElementScopeConflictScenario({ + mergeMode: "transaction-wins-per-element", + }); + + expect(commitSpy).toHaveBeenCalledTimes(1); + const call = commitSpy.mock.calls[0]![0]; + expect(call.logicalBefore.elements.get(element.id)?.strokeColor).toBe( + "#000", + ); + expect(call.logicalBefore.elements.get(element.id)?.x).toBe(0); + expect(call.logicalBefore.elements.get(element.id)?.backgroundColor).toBe( + "#fff", + ); + expect(call.logicalAfter.elements.get(element.id)?.strokeColor).toBe("#f00"); + expect(call.logicalAfter.elements.get(element.id)?.x).toBe(200); + expect(call.logicalAfter.elements.get(element.id)?.backgroundColor).toBe( + "#fff", + ); + }); + + it("mergeMode transaction-wins-per-prop preserves untouched live props", () => { + const commitSpy = vi + .spyOn(h.store, "commitSyntheticIncrement") + .mockReturnValue(true); + const element = setupElementScopeConflictScenario({ + mergeMode: "transaction-wins-per-prop", + }); + + expect(commitSpy).toHaveBeenCalledTimes(1); + const call = commitSpy.mock.calls[0]![0]; + expect(call.logicalBefore.elements.get(element.id)?.strokeColor).toBe( + "#000", + ); + expect(call.logicalBefore.elements.get(element.id)?.x).toBe(0); + expect(call.logicalBefore.elements.get(element.id)?.backgroundColor).toBe( + "#00f5d4", + ); + expect(call.logicalAfter.elements.get(element.id)?.strokeColor).toBe("#f00"); + expect(call.logicalAfter.elements.get(element.id)?.x).toBe(200); + expect(call.logicalAfter.elements.get(element.id)?.backgroundColor).toBe( + "#00f5d4", + ); + }); }); diff --git a/packages/excalidraw/transaction.ts b/packages/excalidraw/transaction.ts index d244345b1d..286ba60b5f 100644 --- a/packages/excalidraw/transaction.ts +++ b/packages/excalidraw/transaction.ts @@ -19,23 +19,75 @@ import type { // Ledger types // --------------------------------------------------------------------------- -/** Which side wins when transaction output and live scene diverge. */ +/** + * Which side wins when transaction output and live scene diverge. + * + * - "live": keep what users currently see in the canvas for conflicting changes. + * - "transaction": force transaction changes for conflicting changes. + */ export type ConflictWinner = "live" | "transaction"; -/** Conflict granularity used by the merge policy. */ +/** + * Conflict granularity used by the merge policy. + * + * - "prop": resolve each touched property independently. + * - "element": when any touched property conflicts, resolve at whole-element level. + */ export type ConflictScope = "prop" | "element"; -/** Merge policy used when building synthetic before/after snapshots. */ +/** + * Merge policy used when building synthetic before/after snapshots. + * + * Four meaningful combinations are supported: + * - live + prop: keep live only for conflicting props, still apply non-conflicting tx props. + * - live + element: if any touched prop conflicts, skip the whole element update. + * - transaction + prop: force tx values for conflicting props, keep untouched live props. + * - transaction + element: if any touched prop conflicts, force the whole tx element. + */ export type TransactionMergePolicy = { conflictWinner: ConflictWinner; conflictScope: ConflictScope; }; +/** + * Named merge presets for clearer call-sites. + * + * These names are intentionally explicit to avoid ambiguity around + * winner/scope semantics. + */ +export type TransactionMergeMode = + | "live-wins-per-prop" + | "live-wins-per-element" + | "transaction-wins-per-prop" + | "transaction-wins-per-element"; + export const DEFAULT_TRANSACTION_MERGE_POLICY: TransactionMergePolicy = { conflictWinner: "live", conflictScope: "prop", }; +export const TRANSACTION_MERGE_MODES: Record< + TransactionMergeMode, + TransactionMergePolicy +> = { + "live-wins-per-prop": { + conflictWinner: "live", + conflictScope: "prop", + }, + "live-wins-per-element": { + conflictWinner: "live", + conflictScope: "element", + }, + "transaction-wins-per-prop": { + conflictWinner: "transaction", + conflictScope: "prop", + }, + "transaction-wins-per-element": { + conflictWinner: "transaction", + conflictScope: "element", + }, +}; + /** Per-element ledger record captured during a transaction session. */ export type TransactionLedgerEntry = { baselineElement: ExcalidrawElement | null; @@ -162,19 +214,58 @@ export const collectChangedElementIds = ( return [...changedIds]; }; -/** Determines whether live conflicts should skip the whole updated element. */ -const shouldSkipUpdateElementOnLiveConflict = ( +/** Detects if an updated element has any live-vs-target conflict on touched props. */ +const hasTouchedPropConflict = ( entry: TransactionLedgerEntry, liveElement: ExcalidrawElement, - policy: TransactionMergePolicy, + targetElement: ExcalidrawElement, ) => { - if (policy.conflictWinner === "transaction") { - return false; + for (const prop of entry.touchedProps) { + const liveValue = getElementProp(liveElement, prop); + const targetValue = getElementProp(targetElement, prop); + if (!isLedgerValueEqual(liveValue, targetValue)) { + return true; + } } - if (entry.touchedProps.has("*")) { - return true; + return false; +}; + +export type TransactionCreateOptions = + | { + mergeMode?: TransactionMergeMode; + mergePolicy?: never; + } + | { + mergeMode?: never; + mergePolicy?: Partial; + }; + +const resolveTransactionMergePolicy = ( + options?: TransactionCreateOptions, +): TransactionMergePolicy => { + if (!options) { + return DEFAULT_TRANSACTION_MERGE_POLICY; } - return policy.conflictScope === "element"; + + // Runtime guard for untyped callers. + if ("mergeMode" in options && "mergePolicy" in options) { + if (options.mergeMode && options.mergePolicy) { + throw new Error( + "Transaction options are ambiguous: pass either mergeMode or mergePolicy, not both.", + ); + } + } + + if ("mergeMode" in options && options.mergeMode) { + return TRANSACTION_MERGE_MODES[options.mergeMode]; + } + + const partialPolicy = + "mergePolicy" in options ? options.mergePolicy : undefined; + return { + ...DEFAULT_TRANSACTION_MERGE_POLICY, + ...partialPolicy, + }; }; // --------------------------------------------------------------------------- @@ -332,21 +423,30 @@ export class TransactionLedger { continue; } - if ( - shouldSkipUpdateElementOnLiveConflict(entry, liveElement, mergePolicy) - ) { - let hasConflict = false; - for (const prop of entry.touchedProps) { - const liveValue = getElementProp(liveElement, prop); - const targetValue = getElementProp(targetElement, prop); - if (!isLedgerValueEqual(liveValue, targetValue)) { - hasConflict = true; - break; - } - } - if (hasConflict) { + const hasElementConflict = hasTouchedPropConflict( + entry, + liveElement, + targetElement, + ); + + if (hasElementConflict && mergePolicy.conflictScope === "element") { + if (mergePolicy.conflictWinner === "live") { + // Example: user changed strokeColor while tx changed strokeColor+x. + // "live + element" keeps the whole live element, including x. continue; } + + // Example: same conflict as above but "transaction + element". + // This applies tx target for the whole element, including untouched props. + elementsBefore.set( + elementId, + deepCopyElement(baselineElement) as OrderedExcalidrawElement, + ); + elementsAfter.set( + elementId, + deepCopyElement(targetElement) as OrderedExcalidrawElement, + ); + continue; } // Deep-copy before mutating so we never touch live elements. @@ -473,13 +573,10 @@ export class Transaction { constructor( app: AppClassProperties, - mergePolicy?: Partial, + options?: TransactionCreateOptions, ) { this.app = app; - this.mergePolicy = { - ...DEFAULT_TRANSACTION_MERGE_POLICY, - ...mergePolicy, - }; + this.mergePolicy = resolveTransactionMergePolicy(options); this.initialAppState = { ...app.store.snapshot.appState }; } @@ -636,9 +733,7 @@ export class TransactionManager { this.app = app; } - create(options?: { - mergePolicy?: Partial; - }): Transaction { - return new Transaction(this.app, options?.mergePolicy); + create(options?: TransactionCreateOptions): Transaction { + return new Transaction(this.app, options); } }