From d0a380758e3ed5dbac6e5f2f41441765599f3119 Mon Sep 17 00:00:00 2001 From: Marcel Mraz Date: Tue, 30 Jul 2024 10:03:27 +0200 Subject: [PATCH] feat: ability to debug the state of fractional indices (#8235) --- packages/excalidraw/data/reconcile.ts | 28 ++++++- packages/excalidraw/fractionalIndex.ts | 79 ++++++++++++++++++- packages/excalidraw/global.d.ts | 1 + packages/excalidraw/scene/Scene.ts | 14 +++- .../excalidraw/tests/fractionalIndex.test.ts | 10 ++- 5 files changed, 122 insertions(+), 10 deletions(-) diff --git a/packages/excalidraw/data/reconcile.ts b/packages/excalidraw/data/reconcile.ts index ea32c8162..1df28e37e 100644 --- a/packages/excalidraw/data/reconcile.ts +++ b/packages/excalidraw/data/reconcile.ts @@ -1,5 +1,10 @@ +import { ENV } from "../constants"; import type { OrderedExcalidrawElement } from "../element/types"; -import { orderByFractionalIndex, syncInvalidIndices } from "../fractionalIndex"; +import { + orderByFractionalIndex, + syncInvalidIndices, + validateFractionalIndices, +} from "../fractionalIndex"; import type { AppState } from "../types"; import type { MakeBrand } from "../utility-types"; import { arrayToMap } from "../utils"; @@ -72,6 +77,27 @@ export const reconcileElements = ( const orderedElements = orderByFractionalIndex(reconciledElements); + if ( + import.meta.env.DEV || + import.meta.env.MODE === ENV.TEST || + window?.DEBUG_FRACTIONAL_INDICES + ) { + const elements = syncInvalidIndices( + // create new instances due to the mutation + orderedElements.map((x) => ({ ...x })), + ); + + validateFractionalIndices(elements, { + // throw in dev & test only, to remain functional on `DEBUG_FRACTIONAL_INDICES` + shouldThrow: import.meta.env.DEV || import.meta.env.MODE === ENV.TEST, + includeBoundTextValidation: true, + reconciliationContext: { + localElements, + remoteElements, + }, + }); + } + // de-duplicate indices syncInvalidIndices(orderedElements); diff --git a/packages/excalidraw/fractionalIndex.ts b/packages/excalidraw/fractionalIndex.ts index 01b6d7015..7d54cff07 100644 --- a/packages/excalidraw/fractionalIndex.ts +++ b/packages/excalidraw/fractionalIndex.ts @@ -6,6 +6,9 @@ import type { OrderedExcalidrawElement, } from "./element/types"; import { InvalidFractionalIndexError } from "./errors"; +import { hasBoundTextElement } from "./element/typeChecks"; +import { getBoundTextElement } from "./element/textElement"; +import { arrayToMap } from "./utils"; /** * Envisioned relation between array order and fractional indices: @@ -30,17 +33,80 @@ import { InvalidFractionalIndexError } from "./errors"; * @throws `InvalidFractionalIndexError` if invalid index is detected. */ export const validateFractionalIndices = ( - indices: (ExcalidrawElement["index"] | undefined)[], + elements: readonly ExcalidrawElement[], + { + shouldThrow = false, + includeBoundTextValidation = false, + reconciliationContext, + }: { + shouldThrow: boolean; + includeBoundTextValidation: boolean; + reconciliationContext?: { + localElements: ReadonlyArray; + remoteElements: ReadonlyArray; + }; + }, ) => { + const errorMessages = []; + const stringifyElement = (element: ExcalidrawElement | void) => + `${element?.index}:${element?.id}:${element?.type}:${element?.isDeleted}:${element?.version}:${element?.versionNonce}`; + + const indices = elements.map((x) => x.index); for (const [i, index] of indices.entries()) { const predecessorIndex = indices[i - 1]; const successorIndex = indices[i + 1]; if (!isValidFractionalIndex(index, predecessorIndex, successorIndex)) { - throw new InvalidFractionalIndexError( - `Fractional indices invariant for element has been compromised - ["${predecessorIndex}", "${index}", "${successorIndex}"] [predecessor, current, successor]`, + errorMessages.push( + `Fractional indices invariant has been compromised: "${stringifyElement( + elements[i - 1], + )}", "${stringifyElement(elements[i])}", "${stringifyElement( + elements[i + 1], + )}"`, ); } + + // disabled by default, as we don't fix it + if (includeBoundTextValidation && hasBoundTextElement(elements[i])) { + const container = elements[i]; + const text = getBoundTextElement(container, arrayToMap(elements)); + + if (text && text.index! <= container.index!) { + errorMessages.push( + `Fractional indices invariant for bound elements has been compromised: "${stringifyElement( + text, + )}", "${stringifyElement(container)}"`, + ); + } + } + } + + if (errorMessages.length) { + const error = new InvalidFractionalIndexError(); + const additionalContext = []; + + if (reconciliationContext) { + additionalContext.push("Additional reconciliation context:"); + additionalContext.push( + reconciliationContext.localElements.map((x) => stringifyElement(x)), + ); + additionalContext.push( + reconciliationContext.remoteElements.map((x) => stringifyElement(x)), + ); + } + + // report just once and with the stacktrace + console.error( + errorMessages.join("\n\n"), + error.stack, + elements.map((x) => stringifyElement(x)), + ...additionalContext, + ); + + if (shouldThrow) { + // if enabled, gather all the errors first, throw once + throw error; + } } }; @@ -83,10 +149,15 @@ export const syncMovedIndices = ( // try generatating indices, throws on invalid movedElements const elementsUpdates = generateIndices(elements, indicesGroups); + const elementsCandidates = elements.map((x) => + elementsUpdates.has(x) ? { ...x, ...elementsUpdates.get(x) } : x, + ); // ensure next indices are valid before mutation, throws on invalid ones validateFractionalIndices( - elements.map((x) => elementsUpdates.get(x)?.index || x.index), + elementsCandidates, + // we don't autofix invalid bound text indices, hence don't include it in the validation + { includeBoundTextValidation: false, shouldThrow: true }, ); // split mutation so we don't end up in an incosistent state diff --git a/packages/excalidraw/global.d.ts b/packages/excalidraw/global.d.ts index 4aeb8c428..ca5848208 100644 --- a/packages/excalidraw/global.d.ts +++ b/packages/excalidraw/global.d.ts @@ -4,6 +4,7 @@ interface Window { EXCALIDRAW_ASSET_PATH: string | undefined; EXCALIDRAW_EXPORT_SOURCE: string; EXCALIDRAW_THROTTLE_RENDER: boolean | undefined; + DEBUG_FRACTIONAL_INDICES: boolean | undefined; gtag: Function; sa_event: Function; fathom: { trackEvent: Function }; diff --git a/packages/excalidraw/scene/Scene.ts b/packages/excalidraw/scene/Scene.ts index 637415a72..c237059c4 100644 --- a/packages/excalidraw/scene/Scene.ts +++ b/packages/excalidraw/scene/Scene.ts @@ -274,9 +274,17 @@ class Scene { : Array.from(nextElements.values()); const nextFrameLikes: ExcalidrawFrameLikeElement[] = []; - if (import.meta.env.DEV || import.meta.env.MODE === ENV.TEST) { - // throw on invalid indices in test / dev to potentially detect cases were we forgot to sync moved elements - validateFractionalIndices(_nextElements.map((x) => x.index)); + if ( + import.meta.env.DEV || + import.meta.env.MODE === ENV.TEST || + window?.DEBUG_FRACTIONAL_INDICES + ) { + validateFractionalIndices(_nextElements, { + // validate everything + includeBoundTextValidation: true, + // throw only in dev & test, to remain functional on `DEBUG_FRACTIONAL_INDICES` + shouldThrow: import.meta.env.DEV || import.meta.env.MODE === ENV.TEST, + }); } this.elements = syncInvalidIndices(_nextElements); diff --git a/packages/excalidraw/tests/fractionalIndex.test.ts b/packages/excalidraw/tests/fractionalIndex.test.ts index 60cf4f5a5..10dbc004b 100644 --- a/packages/excalidraw/tests/fractionalIndex.test.ts +++ b/packages/excalidraw/tests/fractionalIndex.test.ts @@ -763,7 +763,10 @@ function test( // ensure the input is invalid (unless the flag is on) if (!expectValidInput) { expect(() => - validateFractionalIndices(elements.map((x) => x.index)), + validateFractionalIndices(elements, { + shouldThrow: true, + includeBoundTextValidation: true, + }), ).toThrowError(InvalidFractionalIndexError); } @@ -777,7 +780,10 @@ function test( expect(syncedElements.length).toBe(elements.length); expect(() => - validateFractionalIndices(syncedElements.map((x) => x.index)), + validateFractionalIndices(syncedElements, { + shouldThrow: true, + includeBoundTextValidation: true, + }), ).not.toThrowError(InvalidFractionalIndexError); syncedElements.forEach((synced, index) => {