Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 207 additions & 0 deletions packages/react/src/SuperDocEditor.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,213 @@ describe('SuperDocEditor', () => {
});
});

describe('prop stability (SD-2635)', () => {
it('does not destroy/re-init when user prop is a new object literal with identical content', async () => {
const ref = createRef<SuperDocRef>();
const onReady = vi.fn();
const onEditorDestroy = vi.fn();

const { rerender } = render(
<SuperDocEditor
ref={ref}
user={{ name: 'Alex', email: 'alex@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

await waitFor(() => expect(onReady).toHaveBeenCalled(), { timeout: 5000 });
const instanceBefore = ref.current?.getInstance();
expect(instanceBefore).toBeTruthy();

// Re-render with a *new* object literal carrying the same content —
// this is the idiomatic React pattern that used to trigger a full
// destroy + re-init loop before SD-2635.
rerender(
<SuperDocEditor
ref={ref}
user={{ name: 'Alex', email: 'alex@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

// Same underlying instance proves no destroy+rebuild happened.
expect(ref.current?.getInstance()).toBe(instanceBefore);
expect(onEditorDestroy).not.toHaveBeenCalled();
});

it('does not destroy/re-init when users prop is a new array literal with identical content', async () => {
const ref = createRef<SuperDocRef>();
const onReady = vi.fn();
const onEditorDestroy = vi.fn();

const { rerender } = render(
<SuperDocEditor
ref={ref}
users={[{ name: 'Alex', email: 'alex@example.com' }]}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

await waitFor(() => expect(onReady).toHaveBeenCalled(), { timeout: 5000 });
const instanceBefore = ref.current?.getInstance();

rerender(
<SuperDocEditor
ref={ref}
users={[{ name: 'Alex', email: 'alex@example.com' }]}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

expect(ref.current?.getInstance()).toBe(instanceBefore);
expect(onEditorDestroy).not.toHaveBeenCalled();
});

it('rebuilds and remounts a new instance when user prop value actually changes', async () => {
const ref = createRef<SuperDocRef>();
const onReady = vi.fn();
const onEditorDestroy = vi.fn();

const { rerender } = render(
<SuperDocEditor
ref={ref}
user={{ name: 'Alex', email: 'alex@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

await waitFor(() => expect(onReady).toHaveBeenCalled(), { timeout: 5000 });
const instanceBefore = ref.current?.getInstance();

rerender(
<SuperDocEditor
ref={ref}
user={{ name: 'Jamie', email: 'jamie@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

// Old instance torn down, new instance ready.
await waitFor(() => expect(onEditorDestroy).toHaveBeenCalled(), { timeout: 5000 });
await waitFor(() => expect(onReady).toHaveBeenCalledTimes(2), { timeout: 5000 });
expect(ref.current?.getInstance()).not.toBe(instanceBefore);
});

it('stays stable under StrictMode double-invocation on rerender', async () => {
const ref = createRef<SuperDocRef>();
const onReady = vi.fn();
const onEditorDestroy = vi.fn();

const { rerender } = render(
<StrictMode>
<SuperDocEditor
ref={ref}
user={{ name: 'Alex', email: 'alex@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>
</StrictMode>,
);

await waitFor(() => expect(onReady).toHaveBeenCalled(), { timeout: 5000 });
const instanceBefore = ref.current?.getInstance();
const destroysBefore = onEditorDestroy.mock.calls.length;

rerender(
<StrictMode>
<SuperDocEditor
ref={ref}
user={{ name: 'Alex', email: 'alex@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>
</StrictMode>,
);

expect(ref.current?.getInstance()).toBe(instanceBefore);
expect(onEditorDestroy.mock.calls.length).toBe(destroysBefore);
});

it('still rebuilds under StrictMode when user prop value actually changes', async () => {
// The same-content StrictMode test above proves memoization survives
// double-invocation. This test proves the positive path — a real
// value change under StrictMode still tears down and remounts.
const ref = createRef<SuperDocRef>();
const onReady = vi.fn();
const onEditorDestroy = vi.fn();

const { rerender } = render(
<StrictMode>
<SuperDocEditor
ref={ref}
user={{ name: 'Alex', email: 'alex@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>
</StrictMode>,
);

await waitFor(() => expect(onReady).toHaveBeenCalled(), { timeout: 5000 });
const instanceBefore = ref.current?.getInstance();

rerender(
<StrictMode>
<SuperDocEditor
ref={ref}
user={{ name: 'Jamie', email: 'jamie@example.com' }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>
</StrictMode>,
);

await waitFor(() => expect(onEditorDestroy).toHaveBeenCalled(), { timeout: 5000 });
await waitFor(() => expect(ref.current?.getInstance()).not.toBe(instanceBefore), { timeout: 5000 });
});

it('rebuilds when a new modules object is passed, even if content looks equal', async () => {
// `modules` is intentionally kept on reference identity in the dep
// array because it can carry functions and live objects that a
// structural compare would miss. This test pins that contract —
// if a future refactor wraps `modules` in useStructuralMemo, this
// test will fail and flag the regression.
const ref = createRef<SuperDocRef>();
const onReady = vi.fn();
const onEditorDestroy = vi.fn();

const { rerender } = render(
<SuperDocEditor
ref={ref}
modules={{ comments: { visible: true } }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

await waitFor(() => expect(onReady).toHaveBeenCalled(), { timeout: 5000 });
const instanceBefore = ref.current?.getInstance();

rerender(
<SuperDocEditor
ref={ref}
modules={{ comments: { visible: true } }}
onReady={onReady}
onEditorDestroy={onEditorDestroy}
/>,
);

await waitFor(() => expect(onEditorDestroy).toHaveBeenCalled(), { timeout: 5000 });
await waitFor(() => expect(onReady).toHaveBeenCalledTimes(2), { timeout: 5000 });
expect(ref.current?.getInstance()).not.toBe(instanceBefore);
});
});

describe('unique IDs', () => {
it('should generate unique container IDs for multiple instances', () => {
const { container: container1 } = render(<SuperDocEditor />);
Expand Down
17 changes: 12 additions & 5 deletions packages/react/src/SuperDocEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
type CSSProperties,
type ForwardedRef,
} from 'react';
import { useStableId } from './utils';
import { useStableId, useMemoByValue } from './utils';
import type {
CallbackProps,
DocumentMode,
Expand Down Expand Up @@ -50,8 +50,8 @@ function SuperDocEditorInner(props: SuperDocEditorProps, ref: ForwardedRef<Super
onException,
// Key props that trigger rebuild when changed
document: documentProp,
user,
users,
user: userProp,
users: usersProp,
modules,
// All other props passed through
...restProps
Expand All @@ -61,6 +61,13 @@ function SuperDocEditorInner(props: SuperDocEditorProps, ref: ForwardedRef<Super
const documentMode = props.documentMode ?? 'editing';
const role = props.role ?? 'editor';

// `user` and `users` are memoized by value so inline literals don't
// trigger a rebuild. `modules` stays on reference identity — it can
// carry functions and live objects (e.g. `collaboration.provider`)
// that a consumer may intentionally swap. See SD-2635.
const user = useMemoByValue(userProp);
const users = useMemoByValue(usersProp);

const instanceRef = useRef<SuperDocInstance | null>(null);
const toolbarContainerRef = useRef<HTMLDivElement | null>(null);

Expand Down Expand Up @@ -223,8 +230,8 @@ function SuperDocEditorInner(props: SuperDocEditorProps, ref: ForwardedRef<Super
destroyed = true;
};
// Only these props trigger a full rebuild. Other props (rulers, etc.) are
// initial values - use getInstance() methods to change them at runtime.
// Note: restProps is intentionally excluded to avoid rebuilds on every render.
// initial values use getInstance() methods to change them at runtime.
// restProps is intentionally excluded to avoid rebuilds on every render.
// documentMode is handled separately via setDocumentMode() for efficiency.
}, [documentProp, user, users, modules, role, hideToolbar, contained, containerId, toolbarId]);

Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ export type {

// Callback event types
Editor,
EditorSurface,
SuperDocReadyEvent,
SuperDocEditorCreateEvent,
SuperDocEditorUpdateEvent,
SuperDocTransactionEvent,
SuperDocContentErrorEvent,
SuperDocExceptionEvent,
} from './types';
19 changes: 15 additions & 4 deletions packages/react/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,22 @@ export interface SuperDocEditorCreateEvent {
editor: Editor;
}

/** Event passed to onEditorUpdate callback */
export type SuperDocEditorUpdateEvent = Parameters<NonNullable<SuperDocConstructorConfig['onEditorUpdate']>>[0];
/** Surface where an editor event originated. */
export type EditorSurface = 'body' | 'header' | 'footer';

/** Event passed to onTransaction callback */
export type SuperDocTransactionEvent = Parameters<NonNullable<SuperDocConstructorConfig['onTransaction']>>[0];
/** Event passed to onEditorUpdate callback. Mirrors superdoc's EditorUpdateEvent. */
export interface SuperDocEditorUpdateEvent {
/** The primary editor associated with the update. For header/footer edits, this is the main body editor. */
editor: Editor;
/** The editor instance that emitted the update. For body edits, this matches `editor`. */
sourceEditor: Editor;
/** The surface where the edit originated. */
surface: EditorSurface;
/** Relationship ID for header/footer edits. */
headerId?: string | null;
/** Header/footer variant (`default`, `first`, `even`, `odd`) when available. */
sectionType?: string | null;
}

/** Event passed to onContentError callback */
export interface SuperDocContentErrorEvent {
Expand Down
75 changes: 75 additions & 0 deletions packages/react/src/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { describe, it, expect } from 'vitest';
import { renderHook } from '@testing-library/react';
import { useMemoByValue } from './utils';

describe('useMemoByValue', () => {
it('returns the same reference across renders when content is unchanged', () => {
const initial = { name: 'Alex', email: 'alex@example.com' };
const { result, rerender } = renderHook(({ value }) => useMemoByValue(value), {
initialProps: { value: initial },
});

const first = result.current;
expect(first).toBe(initial);

// Parent passes a fresh object literal with identical content
rerender({ value: { name: 'Alex', email: 'alex@example.com' } });
expect(result.current).toBe(first); // same reference — critical for effect deps

// And again, still stable
rerender({ value: { name: 'Alex', email: 'alex@example.com' } });
expect(result.current).toBe(first);
});

it('returns a new reference when the content actually changes', () => {
const { result, rerender } = renderHook(({ value }) => useMemoByValue(value), {
initialProps: { value: { name: 'Alex' } },
});

const first = result.current;
rerender({ value: { name: 'Jamie' } });
expect(result.current).not.toBe(first);
expect(result.current.name).toBe('Jamie');
});

it('handles undefined and null stably', () => {
const { result, rerender } = renderHook(({ value }) => useMemoByValue(value as unknown), {
initialProps: { value: undefined },
});

const first = result.current;
rerender({ value: undefined });
expect(result.current).toBe(first);

rerender({ value: null });
expect(result.current).toBe(null);
});

it('stabilizes arrays the same way as objects', () => {
const { result, rerender } = renderHook(({ value }) => useMemoByValue(value), {
initialProps: { value: [{ id: 1 }, { id: 2 }] },
});

const first = result.current;
rerender({ value: [{ id: 1 }, { id: 2 }] });
expect(result.current).toBe(first);

rerender({ value: [{ id: 1 }, { id: 3 }] });
expect(result.current).not.toBe(first);
});

it('adopts a new reference on circular input (JSON.stringify throws)', () => {
const circularA: { self?: unknown; name: string } = { name: 'a' };
circularA.self = circularA;

const { result, rerender } = renderHook(({ value }) => useMemoByValue(value), {
initialProps: { value: circularA },
});

const circularB: { self?: unknown; name: string } = { name: 'a' };
circularB.self = circularB;
rerender({ value: circularB });
// The compare throws; the hook falls back to adopting the new reference.
expect(result.current).toBe(circularB);
});
});
Loading
Loading