From e9e4c1028d5e97ad2010855202ae9a75fe306d15 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 4 Jul 2026 22:49:48 +0300 Subject: [PATCH] perf(editor): cut per-keystroke work on the typing hot path (#343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The editor lagged while typing (worse with doc size, and under collaboration the same cost is paid for every REMOTE keystroke). ProseMirror itself was fine — the overhead was the surrounding work done on every transaction. Behavior is 1:1; only WHEN work runs changed. - getJSON() off the keystroke path: `onUpdate` no longer serializes the whole doc synchronously — the serialization now runs inside a 3s debounce (new hook use-page-content-cache.ts), flushed on unmount so the last snapshot isn't lost. - footnote numbering: merged 3 per-docChanged O(n) doc walks into one, and short-circuit the whole-doc renumber when the doc has no footnotes and the transaction didn't insert one (step-slice scan — covers typing/paste/collab). - toolbar: replaced per-keystroke `editor.can().undo()/.redo()` dry-runs with cheap history-depth reads (Yjs undoManager stack length / pm-history depth). - render side-effect bug: `remote.attach()` moved out of the render body into a useEffect. - debounced the TOC all-headings rescan and memoized the slash-command suggestion build (was rebuilt twice per keystroke). - node menus (image/video/audio/pdf/callout/subpages): the per-transaction selectors early-return a cheap isActive check instead of running getAttributes + multiple alignment probes while their node type is inactive (shouldShow still controls display — appears exactly when it did). - code blocks: the global selectionUpdate listener is now added only for mermaid blocks (the only consumer of the selected state), eliminating N listeners + N setStates per caret move for normal code blocks. Deferred (documented, collab hot-path risk): full conditional menu MOUNTING (menu-less-frame risk on same-tx context switch) and code-block re-tokenization debounce / language-persist (self-dispatching meta tx + node-attr writes interact with collab/undo). The route split from #342 already keeps lowlight off startup. Gate: editor-ext build + 252/252 tests, client editor tests pass, tsc --noEmit 0, client build ok. New tests: footnote no-footnote-doc → 0 traversals + numbering unchanged; page-content-cache onUpdate-no-sync-getJSON + flush-on-unmount. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../editor/components/audio/audio-menu.tsx | 7 + .../components/callout/callout-menu.tsx | 9 +- .../components/code-block/code-block-view.tsx | 11 +- .../fixed-toolbar/use-toolbar-state.ts | 54 +++++-- .../editor/components/image/image-menu.tsx | 8 ++ .../editor/components/pdf/pdf-menu.tsx | 7 + .../components/subpages/subpages-menu.tsx | 9 +- .../table-of-contents/table-of-contents.tsx | 15 +- .../editor/components/video/video-menu.tsx | 7 + .../editor/extensions/slash-command.ts | 23 ++- .../hooks/use-page-content-cache.test.ts | 100 +++++++++++++ .../editor/hooks/use-page-content-cache.ts | 50 +++++++ .../src/features/editor/page-editor.tsx | 39 ++--- .../lib/footnote/footnote-numbering.test.ts | 134 ++++++++++++++++++ .../src/lib/footnote/footnote-numbering.ts | 128 +++++++++++++---- 15 files changed, 535 insertions(+), 66 deletions(-) create mode 100644 apps/client/src/features/editor/hooks/use-page-content-cache.test.ts create mode 100644 apps/client/src/features/editor/hooks/use-page-content-cache.ts create mode 100644 packages/editor-ext/src/lib/footnote/footnote-numbering.test.ts diff --git a/apps/client/src/features/editor/components/audio/audio-menu.tsx b/apps/client/src/features/editor/components/audio/audio-menu.tsx index bd649482..59a3ce23 100644 --- a/apps/client/src/features/editor/components/audio/audio-menu.tsx +++ b/apps/client/src/features/editor/components/audio/audio-menu.tsx @@ -46,6 +46,13 @@ export function AudioMenu({ editor }: EditorMenuProps) { return null; } + // #343 PART 1: skip getAttributes unless an audio node is active. The menu + // only shows for an active audio node (shouldShow), so the null state while + // inactive is never rendered — behavior unchanged. + if (!ctx.editor.isActive("audio")) { + return null; + } + const audioAttrs = ctx.editor.getAttributes("audio"); return { diff --git a/apps/client/src/features/editor/components/callout/callout-menu.tsx b/apps/client/src/features/editor/components/callout/callout-menu.tsx index 3ce022da..7e910496 100644 --- a/apps/client/src/features/editor/components/callout/callout-menu.tsx +++ b/apps/client/src/features/editor/components/callout/callout-menu.tsx @@ -43,8 +43,15 @@ export function CalloutMenu({ editor }: EditorMenuProps) { return null; } + // #343 PART 1: skip the per-type isActive() probes unless a callout is + // active. The menu only shows for an active callout (shouldShow), so the + // null state while inactive is never rendered — behavior unchanged. + if (!ctx.editor.isActive("callout")) { + return null; + } + return { - isCallout: ctx.editor.isActive("callout"), + isCallout: true, isInfo: ctx.editor.isActive("callout", { type: "info" }), isNote: ctx.editor.isActive("callout", { type: "note" }), isSuccess: ctx.editor.isActive("callout", { type: "success" }), diff --git a/apps/client/src/features/editor/components/code-block/code-block-view.tsx b/apps/client/src/features/editor/components/code-block/code-block-view.tsx index 3f06d9f9..68b0412e 100644 --- a/apps/client/src/features/editor/components/code-block/code-block-view.tsx +++ b/apps/client/src/features/editor/components/code-block/code-block-view.tsx @@ -22,6 +22,12 @@ export default function CodeBlockView(props: NodeViewProps) { const [isSelected, setIsSelected] = useState(false); useEffect(() => { + // #343 PART 6: `isSelected` only drives the mermaid source's visibility (the + // `hidden` prop below). For every non-mermaid code block it is never read, + // so skip the per-block `selectionUpdate` listener entirely — otherwise N + // code blocks each add a global listener + a setState on every caret move. + if (language !== "mermaid") return; + const updateSelection = () => { const { state } = editor; const { from, to } = state.selection; @@ -32,11 +38,14 @@ export default function CodeBlockView(props: NodeViewProps) { setIsSelected(isNodeSelected); }; + // Initialize on attach so switching a block's language to "mermaid" reflects + // the current selection immediately (the listener was not running before). + updateSelection(); editor.on("selectionUpdate", updateSelection); return () => { editor.off("selectionUpdate", updateSelection); }; - }, [editor, getPos(), node.nodeSize]); + }, [editor, getPos(), node.nodeSize, language]); function changeLanguage(language: string) { setLanguageValue(language); diff --git a/apps/client/src/features/editor/components/fixed-toolbar/use-toolbar-state.ts b/apps/client/src/features/editor/components/fixed-toolbar/use-toolbar-state.ts index 589dbeea..2f8e3a7f 100644 --- a/apps/client/src/features/editor/components/fixed-toolbar/use-toolbar-state.ts +++ b/apps/client/src/features/editor/components/fixed-toolbar/use-toolbar-state.ts @@ -1,5 +1,7 @@ import type { Editor } from "@tiptap/react"; import { useEditorState } from "@tiptap/react"; +import { undoDepth, redoDepth } from "@tiptap/pm/history"; +import { yUndoPluginKey } from "@tiptap/y-tiptap"; export interface ToolbarState { isBold: boolean; @@ -16,14 +18,45 @@ export interface ToolbarState { canRedo: boolean; } -// Undo/redo come from either StarterKit's history or the Yjs collaboration -// history extension. During the brief moment a page is rendered with the -// static editor (mainExtensions only, undoRedo disabled), neither is loaded -// and editor.can().undo/redo is undefined. -function safeCan(editor: Editor, command: "undo" | "redo"): boolean { - const can = editor.can() as Record; - const fn = can[command]; - return typeof fn === "function" ? (fn as () => boolean)() : false; +// Undo/redo availability, computed WITHOUT `editor.can().undo()/.redo()`. +// +// `editor.can()` runs the command as a dry-run (building a throwaway state + +// transaction) — the most expensive work in this selector, and it ran on every +// keystroke (and every REMOTE keystroke under collaboration). Instead we read +// the history stack depth directly, which is a cheap plugin-state lookup and +// mirrors exactly what the undo/redo commands themselves check: +// +// - Collaboration (Yjs): the yjs UndoManager's undo/redo stack lengths — the +// same `undoStack.length === 0` / `redoStack.length === 0` guard the +// Collaboration extension's undo/redo commands use. +// - Plain history (templates / non-collab): prosemirror-history's undoDepth / +// redoDepth, which back the UndoRedo extension. +// +// When neither history backend is installed (the pre-sync static editor — +// mainExtensions only, undoRedo disabled), both fall through to 0 -> false, +// matching the previous `safeCan` behavior. +function historyAvailability(editor: Editor): { + canUndo: boolean; + canRedo: boolean; +} { + const state = editor.state; + + // Collaboration history (Yjs) takes precedence when present. + const yState = yUndoPluginKey.getState(state) as + | { undoManager?: { undoStack: unknown[]; redoStack: unknown[] } } + | undefined; + if (yState?.undoManager) { + return { + canUndo: yState.undoManager.undoStack.length > 0, + canRedo: yState.undoManager.redoStack.length > 0, + }; + } + + // Plain prosemirror-history (returns 0 when the history plugin is absent). + return { + canUndo: undoDepth(state) > 0, + canRedo: redoDepth(state) > 0, + }; } export function useToolbarState(editor: Editor | null): ToolbarState | null { @@ -31,6 +64,7 @@ export function useToolbarState(editor: Editor | null): ToolbarState | null { editor, selector: (ctx) => { if (!ctx.editor) return null; + const { canUndo, canRedo } = historyAvailability(ctx.editor); return { isBold: ctx.editor.isActive("bold"), isItalic: ctx.editor.isActive("italic"), @@ -42,8 +76,8 @@ export function useToolbarState(editor: Editor | null): ToolbarState | null { isBulletList: ctx.editor.isActive("bulletList"), isOrderedList: ctx.editor.isActive("orderedList"), isTaskList: ctx.editor.isActive("taskList"), - canUndo: safeCan(ctx.editor, "undo"), - canRedo: safeCan(ctx.editor, "redo"), + canUndo, + canRedo, }; }, }); diff --git a/apps/client/src/features/editor/components/image/image-menu.tsx b/apps/client/src/features/editor/components/image/image-menu.tsx index e91a1868..1eb51da3 100644 --- a/apps/client/src/features/editor/components/image/image-menu.tsx +++ b/apps/client/src/features/editor/components/image/image-menu.tsx @@ -38,6 +38,14 @@ export function ImageMenu({ editor }: EditorMenuProps) { return null; } + // #343 PART 1: skip the expensive per-keystroke work (getAttributes + the + // alignment isActive() probes) unless an image is actually active. The + // menu is only shown when an image is active (see shouldShow), so a null + // state while inactive is never rendered — behavior is unchanged. + if (!ctx.editor.isActive("image")) { + return null; + } + const imageAttrs = ctx.editor.getAttributes("image"); return { diff --git a/apps/client/src/features/editor/components/pdf/pdf-menu.tsx b/apps/client/src/features/editor/components/pdf/pdf-menu.tsx index 3fc8b6fd..3160957f 100644 --- a/apps/client/src/features/editor/components/pdf/pdf-menu.tsx +++ b/apps/client/src/features/editor/components/pdf/pdf-menu.tsx @@ -25,6 +25,13 @@ export function PdfMenu({ editor }: EditorMenuProps) { return null; } + // #343 PART 1: skip getAttributes unless a pdf node is active. The menu + // only shows for an active pdf node (shouldShow), so the null state while + // inactive is never rendered — behavior unchanged. + if (!ctx.editor.isActive("pdf")) { + return null; + } + const pdfAttrs = ctx.editor.getAttributes("pdf"); return { diff --git a/apps/client/src/features/editor/components/subpages/subpages-menu.tsx b/apps/client/src/features/editor/components/subpages/subpages-menu.tsx index 05f05ea6..64281182 100644 --- a/apps/client/src/features/editor/components/subpages/subpages-menu.tsx +++ b/apps/client/src/features/editor/components/subpages/subpages-menu.tsx @@ -70,7 +70,14 @@ export const SubpagesMenu = React.memo( // toggle without re-rendering on every keystroke. const isRecursive = useEditorState({ editor, - selector: (ctx) => ctx.editor?.getAttributes("subpages")?.recursive ?? false, + // #343 PART 1: skip getAttributes unless a subpages node is active. The + // menu only shows for an active subpages node (shouldShow), so the value + // is only read then; getAttributes on an inactive node returns the default + // (recursive === false) anyway, so this is behavior-preserving. + selector: (ctx) => + ctx.editor?.isActive("subpages") + ? (ctx.editor.getAttributes("subpages")?.recursive ?? false) + : false, }); return ( diff --git a/apps/client/src/features/editor/components/table-of-contents/table-of-contents.tsx b/apps/client/src/features/editor/components/table-of-contents/table-of-contents.tsx index 4b76e08d..f24c5c79 100644 --- a/apps/client/src/features/editor/components/table-of-contents/table-of-contents.tsx +++ b/apps/client/src/features/editor/components/table-of-contents/table-of-contents.tsx @@ -4,6 +4,7 @@ import React, { FC, useEffect, useRef, useState } from "react"; import classes from "./table-of-contents.module.css"; import clsx from "clsx"; import { Box, Text, Title } from "@mantine/core"; +import { useDebouncedCallback } from "@mantine/hooks"; import { useTranslation } from "react-i18next"; type TableOfContentsProps = { @@ -79,13 +80,21 @@ export const TableOfContents: FC = (props) => { setHeadingDOMNodes(result.nodes); }; + // Debounce the update-driven rescan: `$nodes("heading")` scans every heading + // in the document, and it previously ran on EVERY keystroke while the TOC + // panel was open. The panel is derived UI, so recomputing ~300ms after typing + // settles keeps it correct without doing an all-headings scan per keystroke + // (#343, PART 7). `useDebouncedCallback` returns a stable reference and always + // invokes the latest `handleUpdate`. + const debouncedHandleUpdate = useDebouncedCallback(handleUpdate, 300); + useEffect(() => { - props.editor?.on("update", handleUpdate); + props.editor?.on("update", debouncedHandleUpdate); return () => { - props.editor?.off("update", handleUpdate); + props.editor?.off("update", debouncedHandleUpdate); }; - }, [props.editor]); + }, [props.editor, debouncedHandleUpdate]); useEffect( () => { diff --git a/apps/client/src/features/editor/components/video/video-menu.tsx b/apps/client/src/features/editor/components/video/video-menu.tsx index bfbaf27a..c13fb116 100644 --- a/apps/client/src/features/editor/components/video/video-menu.tsx +++ b/apps/client/src/features/editor/components/video/video-menu.tsx @@ -31,6 +31,13 @@ export function VideoMenu({ editor }: EditorMenuProps) { return null; } + // #343 PART 1: skip getAttributes + alignment isActive() probes unless a + // video is active. The menu only shows for an active video (shouldShow), + // so the null state while inactive is never rendered — behavior unchanged. + if (!ctx.editor.isActive("video")) { + return null; + } + const videoAttrs = ctx.editor.getAttributes("video"); return { diff --git a/apps/client/src/features/editor/extensions/slash-command.ts b/apps/client/src/features/editor/extensions/slash-command.ts index 947a6d58..fc93ec18 100644 --- a/apps/client/src/features/editor/extensions/slash-command.ts +++ b/apps/client/src/features/editor/extensions/slash-command.ts @@ -6,6 +6,23 @@ import getSuggestionItems from '@/features/editor/components/slash-menu/menu-ite export const slashMenuPluginKey = new PluginKey('slash-command'); +// getSuggestionItems fuzzy-matches EVERY command against the query (plus its +// wrong-keyboard-layout remaps) and, while the slash menu is open, is invoked +// TWICE per keystroke: once by the synchronous `allow` gate below and once by +// the popup's `items` builder. A synchronous gating predicate can't be +// debounced without breaking the suggestion decoration/activation, so instead we +// memoize the LAST query's result: the two same-query calls in one keystroke +// build the list only once, and the cache invalidates the moment the query +// changes — so there is no stale-state risk (#343, PART 7). +let lastQuery: string | null = null; +let lastResult: ReturnType | null = null; +function suggestionItemsForQuery(query: string) { + if (query === lastQuery && lastResult) return lastResult; + lastQuery = query; + lastResult = getSuggestionItems({ query }); + return lastResult; +} + // @ts-ignore const Command = Extension.create({ name: 'slash-command', @@ -38,7 +55,7 @@ const Command = Extension.create({ // non-matching queries while keeping multi-word matches (e.g. // "/Heading 1") working. const query = state.doc.textBetween(range.from + 1, range.to); - const groups = getSuggestionItems({ query }); + const groups = suggestionItemsForQuery(query); const hasMatches = Object.values(groups).some( (items) => items.length > 0, ); @@ -61,7 +78,9 @@ const Command = Extension.create({ const SlashCommand = Command.configure({ suggestion: { - items: getSuggestionItems, + // Share the per-query memo with `allow` so the pair of same-query calls in a + // single keystroke rebuilds the list once (#343, PART 7). + items: ({ query }: { query: string }) => suggestionItemsForQuery(query), render: renderItems, }, }); diff --git a/apps/client/src/features/editor/hooks/use-page-content-cache.test.ts b/apps/client/src/features/editor/hooks/use-page-content-cache.test.ts new file mode 100644 index 00000000..75ea61e7 --- /dev/null +++ b/apps/client/src/features/editor/hooks/use-page-content-cache.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import type { MutableRefObject } from "react"; +import type { Editor } from "@tiptap/react"; + +// Mock the app entry so importing the hook doesn't boot the whole app; the hook +// only needs queryClient's cache read/write, which we stub here. Declared via +// vi.hoisted so the spies exist before the hoisted vi.mock factory runs. +const { getQueryData, setQueryData } = vi.hoisted(() => ({ + getQueryData: vi.fn(() => undefined as unknown), + setQueryData: vi.fn(), +})); +vi.mock("@/main.tsx", () => ({ + queryClient: { getQueryData, setQueryData }, +})); + +import { usePageContentCache } from "./use-page-content-cache"; + +const SNAPSHOT = { type: "doc", content: [] }; + +function makeFakeEditor(overrides: Partial = {}): Editor { + return { + isEmpty: false, + isDestroyed: false, + getJSON: vi.fn(() => SNAPSHOT), + ...overrides, + } as unknown as Editor; +} + +describe("usePageContentCache (#343 PART 3) — getJSON off the keystroke path", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + // A cached page exists so the write path runs. + getQueryData.mockReturnValue({ id: "p1", content: {} }); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it("onUpdate (calling the debounced fn) does NOT call getJSON synchronously", () => { + const editor = makeFakeEditor(); + const editorRef = { current: editor } as MutableRefObject; + + const { result } = renderHook(() => + usePageContentCache(editorRef, "slug-1", 3000), + ); + + // Simulate a keystroke's onUpdate -> only schedules the debounce. + act(() => { + result.current(); + result.current(); + result.current(); + }); + + // The whole-doc serialization must NOT have happened yet. + expect(editor.getJSON).not.toHaveBeenCalled(); + expect(setQueryData).not.toHaveBeenCalled(); + + // Once the debounce window elapses, getJSON runs exactly once (not per call). + act(() => vi.advanceTimersByTime(3000)); + expect(editor.getJSON).toHaveBeenCalledTimes(1); + expect(setQueryData).toHaveBeenCalledWith(["pages", "slug-1"], { + id: "p1", + content: SNAPSHOT, + }); + }); + + it("flushes the pending snapshot on unmount so the last edit isn't lost", () => { + const editor = makeFakeEditor(); + const editorRef = { current: editor } as MutableRefObject; + + const { result, unmount } = renderHook(() => + usePageContentCache(editorRef, "slug-1", 3000), + ); + + act(() => result.current()); + expect(editor.getJSON).not.toHaveBeenCalled(); + + // Navigation/unmount must flush (not drop) the pending write. + act(() => unmount()); + expect(editor.getJSON).toHaveBeenCalledTimes(1); + expect(setQueryData).toHaveBeenCalledTimes(1); + }); + + it("skips the write when the editor is destroyed (flush racing teardown)", () => { + const editor = makeFakeEditor({ isDestroyed: true }); + const editorRef = { current: editor } as MutableRefObject; + + const { result } = renderHook(() => + usePageContentCache(editorRef, "slug-1", 3000), + ); + + act(() => result.current()); + act(() => vi.advanceTimersByTime(3000)); + + expect(editor.getJSON).not.toHaveBeenCalled(); + expect(setQueryData).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/client/src/features/editor/hooks/use-page-content-cache.ts b/apps/client/src/features/editor/hooks/use-page-content-cache.ts new file mode 100644 index 00000000..dbaf8853 --- /dev/null +++ b/apps/client/src/features/editor/hooks/use-page-content-cache.ts @@ -0,0 +1,50 @@ +import type { MutableRefObject } from "react"; +import { useDebouncedCallback } from "@mantine/hooks"; +import type { Editor } from "@tiptap/react"; +import { queryClient } from "@/main.tsx"; +import { IPage } from "@/features/page/types/page.types.ts"; + +/** + * Off-keystroke local page-cache updater (issue #343, PART 3). + * + * The editor's `onUpdate` fires on every keystroke — and, under collaboration, + * on every REMOTE keystroke too. Serializing the WHOLE document with + * `editor.getJSON()` on that hot path is expensive, and the previous 3s debounce + * only guarded the cache WRITE, not the serialization: `getJSON()` still ran per + * keystroke. + * + * This hook moves the serialization INSIDE the debounced callback, so the + * full-doc traversal happens at most once per `delay`, not per keystroke. Call + * the returned function from `onUpdate` (it only schedules the debounce); the + * `getJSON()` snapshot is taken when the debounce fires. + * + * On unmount/navigation the pending snapshot is FLUSHED (via `flushOnUnmount`) + * so the last edits within the debounce window aren't lost from the local cache. + * The source of truth is collab/Yjs, but the cache must not go stale. + * + * IMPORTANT: call this hook BEFORE `useEditor`. React runs effect cleanups in + * declaration order on unmount, so the debounce's flush cleanup must be declared + * before `useEditor`'s teardown to run while the editor is still alive; the + * `isDestroyed` guard keeps a flush that still races teardown safe (it skips). + */ +export function usePageContentCache( + editorRef: MutableRefObject, + slugId: string | undefined, + delay = 3000, +) { + return useDebouncedCallback( + () => { + const e = editorRef.current; + if (!e || e.isDestroyed || e.isEmpty) return; + const pageData = queryClient.getQueryData(["pages", slugId]); + if (pageData) { + // getJSON() (full-doc serialization) runs HERE, off the keystroke path. + queryClient.setQueryData(["pages", slugId], { + ...pageData, + content: e.getJSON(), + }); + } + }, + { delay, flushOnUnmount: true }, + ); +} diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index 9953d6c5..8b826615 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -62,7 +62,7 @@ import ExcalidrawMenu from "./components/excalidraw/excalidraw-menu-lazy"; import DrawioMenu from "./components/drawio/drawio-menu"; import { useCollabToken } from "@/features/auth/queries/auth-query.tsx"; import SearchAndReplaceDialog from "@/features/editor/components/search-and-replace/search-and-replace-dialog.tsx"; -import { useDebouncedCallback, useDocumentVisibility } from "@mantine/hooks"; +import { useDocumentVisibility } from "@mantine/hooks"; import { useIdle } from "@/hooks/use-idle.ts"; import { queryClient } from "@/main.tsx"; import { IPage } from "@/features/page/types/page.types.ts"; @@ -79,6 +79,7 @@ import { PageEditMode } from "@/features/user/types/user.types.ts"; import { jwtDecode } from "jwt-decode"; import { searchSpotlight } from "@/features/search/constants.ts"; import { useEditorScroll } from "./hooks/use-editor-scroll"; +import { usePageContentCache } from "./hooks/use-page-content-cache"; import { useScrollRestoreOnSwap } from "./hooks/use-scroll-position"; import { useSwapHeightReservation } from "./hooks/use-swap-height-reservation"; import { EditorLinkMenu } from "@/features/editor/components/link/link-menu"; @@ -267,8 +268,13 @@ export default function PageEditor({ } }, [isIdle, documentState, providersReady, resetIdle]); - // Attach here, to make sure the connection gets properly established - providersRef.current?.remote.attach(); + // Attach the remote provider once it's ready (and again after a pageId swap + // recreates it) to make sure the connection gets properly established. This + // used to run in the render body — a side effect during render (#343, PART 7). + // `attach()` is idempotent, so re-running it on these deps is safe. + useEffect(() => { + providersRef.current?.remote.attach(); + }, [providersReady, pageId]); const extensions = useMemo(() => { if (!providersReady || !providersRef.current || !currentUser?.user) { @@ -283,6 +289,12 @@ export default function PageEditor({ ]; }, [providersReady, currentUser?.user]); + // getJSON() serialization + cache write live in the hook, off the keystroke + // path, and flush on unmount so the last snapshot survives navigation (#343). + // MUST be declared before useEditor: React runs effect cleanups in declaration + // order on unmount, so the flush must run before the editor is torn down. + const debouncedUpdateContent = usePageContentCache(editorRef, slugId); + const editor = useEditor( { extensions, @@ -353,11 +365,11 @@ export default function PageEditor({ editorRef.current = editor; } }, - onUpdate({ editor }) { - if (editor.isEmpty) return; - const editorJson = editor.getJSON(); - //update local page cache to reduce flickers - debouncedUpdateContent(editorJson); + onUpdate() { + // Only schedule the debounce here — the whole-doc getJSON() serialization + // happens INSIDE the debounced callback (see usePageContentCache), so it + // no longer runs synchronously on every (local or remote) keystroke. + debouncedUpdateContent(); }, }, [pageId, editable, extensions], @@ -403,17 +415,6 @@ export default function PageEditor({ }; }, [editor, pageId, editorIsEditable]); - const debouncedUpdateContent = useDebouncedCallback((newContent: any) => { - const pageData = queryClient.getQueryData(["pages", slugId]); - - if (pageData) { - queryClient.setQueryData(["pages", slugId], { - ...pageData, - content: newContent, - }); - } - }, 3000); - const handleActiveCommentEvent = (event) => { const { commentId, resolved } = event.detail; diff --git a/packages/editor-ext/src/lib/footnote/footnote-numbering.test.ts b/packages/editor-ext/src/lib/footnote/footnote-numbering.test.ts new file mode 100644 index 00000000..f08730e6 --- /dev/null +++ b/packages/editor-ext/src/lib/footnote/footnote-numbering.test.ts @@ -0,0 +1,134 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { getSchema } from '@tiptap/core'; +import { Document } from '@tiptap/extension-document'; +import { Paragraph } from '@tiptap/extension-paragraph'; +import { Text } from '@tiptap/extension-text'; +import { EditorState } from '@tiptap/pm/state'; +import { Node as PMNode } from '@tiptap/pm/model'; +import { FootnoteReference } from './footnote-reference'; +import { FootnotesList } from './footnotes-list'; +import { FootnoteDefinition } from './footnote-definition'; +import { + footnoteNumberingPlugin, + footnoteNumberingPluginKey, + getFootnoteNumber, +} from './footnote-numbering'; +import { + FOOTNOTE_REFERENCE_NAME, + FOOTNOTES_LIST_NAME, + FOOTNOTE_DEFINITION_NAME, +} from './footnote-util'; + +const extensions = [ + Document, + Paragraph, + Text, + FootnoteReference, + FootnotesList, + FootnoteDefinition, +]; + +const schema = getSchema(extensions); + +function makeState(docJson: any): EditorState { + return EditorState.create({ + doc: PMNode.fromJSON(schema, docJson), + plugins: [footnoteNumberingPlugin()], + }); +} + +const withTwoFootnotes = { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { type: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'x' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'y' } }, + ], + }, + { + type: FOOTNOTES_LIST_NAME, + content: [ + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: 'x' }, + content: [{ type: 'paragraph' }], + }, + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: 'y' }, + content: [{ type: 'paragraph' }], + }, + ], + }, + ], +}; + +describe('footnote numbering plugin — short-circuit (#343 PART 5)', () => { + afterEach(() => vi.restoreAllMocks()); + + it('does ZERO document traversals on a docChanged transaction when the doc has no footnotes', () => { + const state = makeState({ + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'hi' }] }], + }); + + // Only count traversals caused by the transaction, not the initial build. + const descendantsSpy = vi.spyOn(PMNode.prototype, 'descendants'); + + const before = footnoteNumberingPluginKey.getState(state); + // A real content edit (docChanged) that introduces no footnote node. + const next = state.apply(state.tr.insertText('!', 3)); + const after = footnoteNumberingPluginKey.getState(next); + + // The plugin never walked the document... + expect(descendantsSpy).not.toHaveBeenCalled(); + // ...and reused the exact same (empty) state object — proof it short-circuited. + expect(after).toBe(before); + expect(after?.hasFootnotes).toBe(false); + }); + + it('rebuilds (numbering appears) the first time a footnote is inserted into a footnote-free doc', () => { + const state = makeState({ + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'hi' }] }], + }); + expect(footnoteNumberingPluginKey.getState(state)?.hasFootnotes).toBe(false); + + const ref = schema.nodes[FOOTNOTE_REFERENCE_NAME].create({ id: 'x' }); + const next = state.apply(state.tr.insert(3, ref)); + + const after = footnoteNumberingPluginKey.getState(next); + expect(after?.hasFootnotes).toBe(true); + expect(getFootnoteNumber(next, 'x')).toBe(1); + }); +}); + +describe('footnote numbering plugin — numbering unchanged with footnotes (#343 PART 5)', () => { + it('numbers references in document order via the single merged walk', () => { + const state = makeState(withTwoFootnotes); + expect(getFootnoteNumber(state, 'x')).toBe(1); + expect(getFootnoteNumber(state, 'y')).toBe(2); + }); + + it('produces a decoration for every reference and matching definition', () => { + const state = makeState(withTwoFootnotes); + const decos = footnoteNumberingPluginKey.getState(state)?.decorations; + // 2 references + 2 definitions = 4 number decorations. + expect(decos?.find().length).toBe(4); + }); + + it('keeps numbering current after an edit while footnotes exist', () => { + const state = makeState(withTwoFootnotes); + // Insert a NEW reference (id "z") before the others: it must become #1 and + // shift x -> #2, y -> #3 (deterministic document-order numbering). + const ref = schema.nodes[FOOTNOTE_REFERENCE_NAME].create({ id: 'z' }); + const next = state.apply(state.tr.insert(1, ref)); + expect(getFootnoteNumber(next, 'z')).toBe(1); + expect(getFootnoteNumber(next, 'x')).toBe(2); + expect(getFootnoteNumber(next, 'y')).toBe(3); + }); +}); diff --git a/packages/editor-ext/src/lib/footnote/footnote-numbering.ts b/packages/editor-ext/src/lib/footnote/footnote-numbering.ts index 3a0950a4..5840be66 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-numbering.ts +++ b/packages/editor-ext/src/lib/footnote/footnote-numbering.ts @@ -1,11 +1,9 @@ -import { EditorState, Plugin, PluginKey } from '@tiptap/pm/state'; +import { EditorState, Plugin, PluginKey, Transaction } from '@tiptap/pm/state'; import { Decoration, DecorationSet } from '@tiptap/pm/view'; -import { Node as ProseMirrorNode } from '@tiptap/pm/model'; +import { Node as ProseMirrorNode, Slice } from '@tiptap/pm/model'; import { FOOTNOTE_DEFINITION_NAME, FOOTNOTE_REFERENCE_NAME, - computeFootnoteNumbers, - computeFootnoteRefCounts, } from './footnote-util'; export const footnoteNumberingPluginKey = new PluginKey( @@ -27,8 +25,22 @@ interface FootnoteNumberingState { refCounts: Map; /** Decorations rendering those numbers (refs + definitions). */ decorations: DecorationSet; + /** Whether the document contains ANY footnote reference/definition node. + * Cached so `apply` can skip the whole-doc walk on every keystroke in the + * common case (documents with no footnotes), recomputing only once a + * transaction actually inserts a footnote node (#343, PART 5). */ + hasFootnotes: boolean; } +/** Reusable empty state for footnote-free documents — avoids reallocating an + * empty map/decoration set on every keystroke while there are no footnotes. */ +const EMPTY_STATE: FootnoteNumberingState = { + numbers: new Map(), + refCounts: new Map(), + decorations: DecorationSet.empty, + hasFootnotes: false, +}; + /** * Build the decoration set for footnote numbers. Pure function of the document: * walk references in document order, assign 1-based numbers, then attach a @@ -41,50 +53,101 @@ export function buildFootnoteDecorations(doc: ProseMirrorNode): DecorationSet { return buildFootnoteNumberingState(doc).decorations; } +function numberDecoration(pos: number, nodeSize: number, num: number): Decoration { + return Decoration.node(pos, pos + nodeSize, { + 'data-footnote-number': String(num), + style: `--footnote-number: "${num}";`, + }); +} + /** - * Compute both the number map AND the decorations for `doc` in a single walk. - * The plugin caches the result so NodeViews can read numbers without - * recomputing. + * Compute the number map, reference counts AND the decorations for `doc` in a + * SINGLE document walk (previously three separate O(n) traversals per + * docChanged — computeFootnoteNumbers + computeFootnoteRefCounts + a decoration + * pass, #343 PART 5). The plugin caches the result so NodeViews can read numbers + * without recomputing. + * + * References are numbered and decorated as they are encountered (document + * order). Definition positions are collected during the same walk and decorated + * afterwards from the completed number map — so a definition that appears before + * its reference in document order still resolves to the correct number, and the + * output is identical to the previous three-pass implementation. (Decoration + * insertion order does not matter: DecorationSet.create indexes by position.) */ function buildFootnoteNumberingState( doc: ProseMirrorNode, ): FootnoteNumberingState { - const numbers = computeFootnoteNumbers(doc); - const refCounts = computeFootnoteRefCounts(doc); + const numbers = new Map(); + const refCounts = new Map(); const decorations: Decoration[] = []; + const definitions: { id: string; pos: number; nodeSize: number }[] = []; + let n = 0; + let hasFootnotes = false; doc.descendants((node, pos) => { - if (node.type.name === FOOTNOTE_REFERENCE_NAME) { - const num = numbers.get(node.attrs.id); - if (num != null) { - decorations.push( - Decoration.node(pos, pos + node.nodeSize, { - 'data-footnote-number': String(num), - style: `--footnote-number: "${num}";`, - }), - ); - } - } - if (node.type.name === FOOTNOTE_DEFINITION_NAME) { - const num = numbers.get(node.attrs.id); - if (num != null) { - decorations.push( - Decoration.node(pos, pos + node.nodeSize, { - 'data-footnote-number': String(num), - style: `--footnote-number: "${num}";`, - }), - ); + const typeName = node.type.name; + if (typeName === FOOTNOTE_REFERENCE_NAME) { + hasFootnotes = true; + const id = node.attrs.id; + if (id) { + if (!numbers.has(id)) numbers.set(id, ++n); + refCounts.set(id, (refCounts.get(id) ?? 0) + 1); + decorations.push(numberDecoration(pos, node.nodeSize, numbers.get(id)!)); } + } else if (typeName === FOOTNOTE_DEFINITION_NAME) { + hasFootnotes = true; + const id = node.attrs.id; + if (id != null) definitions.push({ id, pos, nodeSize: node.nodeSize }); } }); + if (!hasFootnotes) return EMPTY_STATE; + + for (const def of definitions) { + const num = numbers.get(def.id); + if (num != null) { + decorations.push(numberDecoration(def.pos, def.nodeSize, num)); + } + } + return { numbers, refCounts, decorations: DecorationSet.create(doc, decorations), + hasFootnotes: true, }; } +/** + * Cheap check: does any of a transaction's inserted content contain a footnote + * reference/definition node? Footnote nodes can only ENTER the document through + * replace steps (ReplaceStep / ReplaceAroundStep both expose a `.slice`), so + * scanning only the inserted slices — O(change size), not O(doc) — is sufficient + * to detect a newly-added footnote. Mark/attr steps never introduce nodes. + * Lets `apply` keep skipping the whole-doc walk until a footnote first appears. + */ +function transactionInsertsFootnote(tr: Transaction): boolean { + for (const step of tr.steps) { + const slice = (step as unknown as { slice?: Slice }).slice; + if (!slice || slice.content.size === 0) continue; + let found = false; + slice.content.descendants((node) => { + if (found) return false; + const typeName = node.type.name; + if ( + typeName === FOOTNOTE_REFERENCE_NAME || + typeName === FOOTNOTE_DEFINITION_NAME + ) { + found = true; + return false; + } + return true; + }); + if (found) return true; + } + return false; +} + /** * Read the cached footnote number for `id` from the numbering plugin's state. * This is the source NodeViews should use instead of calling @@ -126,6 +189,13 @@ export function footnoteNumberingPlugin(): Plugin { // the number map NodeViews read stays current on every edit while // non-doc transactions (selection, etc.) reuse the cache for free. if (!tr.docChanged) return old; + // Short-circuit the whole-doc walk while the document has no footnotes: + // if there were none and this transaction did not INSERT one, there is + // still nothing to number, so reuse the empty state (#343, PART 5). Once + // a footnote exists we always rebuild (covers renumbering/deletion). + if (!old.hasFootnotes && !transactionInsertsFootnote(tr)) { + return old; + } return buildFootnoteNumberingState(tr.doc); }, }, -- 2.52.0