From 8413185a1dc105f4dc60bf96e51d871a4580688a Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 15:12:21 +0300 Subject: [PATCH 01/16] fix(ai-chat): tick the live token counter between agent steps (#163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The header token badge (and the "Thinking… · N tokens" line) froze between agent steps and jumped in chunks instead of ticking smoothly. liveTurnTokens returned the authoritative server `usage` VERBATIM as soon as it appeared, but the server only attaches usage at a step boundary and it is cumulative over COMPLETED steps — so during the next (in-flight) step the figure stayed frozen at the previous boundary and the running text estimate was ignored. Combine both sources per component via max: always compute the running estimate (chars/≈4 over the message's reasoning/text parts, which includes the in-flight step) and take max(authoritativeBase, estimate). Between boundaries the estimate ticks the number up; at a boundary the authoritative figure snaps it exact; and because the server usage is cumulative and we only ever take the max, the counter is monotonic (never drops). Reasoning/output stay split; the #151 reasoning-only authoritative count is preserved. Backward compatible: in every existing test the estimate is <= the authoritative figure, so max returns the same value. +4 tests for the in-flight-step-exceeds- base case (output + reasoning), the authoritative-wins case, and monotonicity. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/utils/count-stream-tokens.test.ts | 52 ++++++++++++++ .../ai-chat/utils/count-stream-tokens.ts | 67 ++++++++++++------- 2 files changed, 95 insertions(+), 24 deletions(-) diff --git a/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts b/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts index 62256bc3..3e650f0d 100644 --- a/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts +++ b/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts @@ -117,3 +117,55 @@ describe("liveTurnTokens — authoritative path", () => { expect(r).toEqual({ reasoning: 0, output: 1, authoritative: false }); }); }); + +describe("liveTurnTokens — combined authoritative + estimate (#163)", () => { + it("ticks the in-flight step above the completed-steps authoritative base", () => { + // The authoritative usage is the sum over COMPLETED steps (step 1). The + // CURRENT step is streaming and its text is NOT in `usage` yet, but it IS in + // the parts -> the running estimate must push the live figure above the base + // so the badge keeps growing between step boundaries. + const longText = "x".repeat(800); // 800 chars -> 200 est output tokens + const r = liveTurnTokens( + msg([{ type: "text", text: longText }], { + usage: { inputTokens: 500, outputTokens: 40 }, // step-1 base: 40 output + }), + ); + // max(authOutput=40, estOutput=200) = 200 -> the counter ticks, not frozen. + expect(r.output).toBe(200); + expect(r.authoritative).toBe(true); + }); + + it("ticks reasoning of the in-flight step above the authoritative reasoning base", () => { + const longReasoning = "r".repeat(400); // 400 chars -> 100 est reasoning + const r = liveTurnTokens( + msg([{ type: "reasoning", text: longReasoning }], { + usage: { inputTokens: 100, outputTokens: 20, reasoningTokens: 20 }, + }), + ); + // reasoning: max(20, 100) = 100 ; output: max(max(0,20-20)=0, 0) = 0. + expect(r.reasoning).toBe(100); + expect(r.output).toBe(0); + expect(r.authoritative).toBe(true); + }); + + it("snaps to the authoritative figure once it exceeds the rough estimate", () => { + // Short on-screen text (estimate tiny) but a large authoritative output: + // the exact figure wins at the boundary (the counter never under-reports). + const r = liveTurnTokens( + msg([{ type: "text", text: "abcd" }], { + usage: { inputTokens: 10, outputTokens: 5000 }, + }), + ); + expect(r.output).toBe(5000); + }); + + it("is monotonic: max never drops below the authoritative base when the estimate is smaller", () => { + // Mirrors the legacy 'verbatim' tests: estimate < authoritative -> unchanged. + const r = liveTurnTokens( + msg([{ type: "text", text: "tiny" }], { + usage: { inputTokens: 500, outputTokens: 100, reasoningTokens: 30 }, + }), + ); + expect(r).toEqual({ reasoning: 30, output: 70, authoritative: true }); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts b/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts index e9cca6bb..9a900996 100644 --- a/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts +++ b/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts @@ -56,39 +56,58 @@ function metadataUsage(message: UIMessage): AuthoritativeUsage | undefined { /** * Token split for the given (streaming) assistant message. * - * Prefers AUTHORITATIVE `metadata.usage` when the server has attached it (at a - * step/turn boundary, incl. `reasoningTokens`) — so the live counter snaps to the - * provider's exact figures. Until then it returns a running ESTIMATE summed over - * the message parts: `reasoning` parts feed the reasoning estimate, `text` parts - * feed the output estimate. Multi-part / multi-step turns accumulate naturally - * because every part of the turn is summed. + * COMBINES the authoritative server usage with the running text estimate so the + * counter ticks in real time AND lands exact. The server only attaches + * `metadata.usage` at a step/turn boundary (`finish-step`/`finish`) and it is + * CUMULATIVE over COMPLETED steps — it does NOT yet include the in-flight step. + * So a multi-step turn that returned the authoritative figure verbatim would + * FREEZE between boundaries and jump in steps (issue #163). + * + * Instead we always compute the running ESTIMATE (chars/≈4 over the message's + * `reasoning`/`text` parts, which grows on every streamed delta) and take the + * per-component MAX of the authoritative base and the estimate: + * - between boundaries the estimate of the in-flight step ticks the number up; + * - at a boundary the authoritative figure snaps it to exact; + * - because the server's usage is cumulative and we only ever take the max, the + * number is MONOTONIC — it never drops. * * Providers that don't stream reasoning text still surface a reasoning count once - * the authoritative usage arrives (`usage.reasoningTokens`); on the pure estimate - * path such a turn simply shows `reasoning: 0` until then. + * the authoritative usage arrives (`max(reasoningTokens, 0)`); on the pure + * estimate path (no usage yet) such a turn shows `reasoning: 0` until then. */ export function liveTurnTokens(message: UIMessage | undefined): LiveTurnTokens { if (!message) return { reasoning: 0, output: 0, authoritative: false }; - const usage = metadataUsage(message); - if (usage) { - // Authoritative branch: outputTokens already INCLUDES reasoning tokens in the - // AI SDK usage shape, so subtract reasoning out for the "answer" figure (never - // go negative if a provider reports them inconsistently). - const reasoning = usage.reasoningTokens ?? 0; - const totalOutput = usage.outputTokens ?? 0; - const output = Math.max(0, totalOutput - reasoning); - return { reasoning, output, authoritative: true }; - } - - let reasoning = 0; - let output = 0; + // Running ESTIMATE over every reasoning/text part — grows on each delta. This + // includes the IN-FLIGHT step, which the authoritative usage does not cover yet. + let estReasoning = 0; + let estOutput = 0; for (const part of message.parts ?? []) { if (part.type === "reasoning") { - reasoning += estimateTokens((part as { text?: string }).text ?? ""); + estReasoning += estimateTokens((part as { text?: string }).text ?? ""); } else if (part.type === "text") { - output += estimateTokens((part as { text?: string }).text ?? ""); + estOutput += estimateTokens((part as { text?: string }).text ?? ""); } } - return { reasoning, output, authoritative: false }; + + const usage = metadataUsage(message); + if (!usage) { + // No authoritative usage streamed yet: the estimate IS the live figure. + return { reasoning: estReasoning, output: estOutput, authoritative: false }; + } + + // Authoritative sum over COMPLETED steps. `outputTokens` already INCLUDES + // reasoning in the AI SDK usage shape, so subtract it out for the "answer" + // figure (never go negative if a provider reports them inconsistently). + const authReasoning = usage.reasoningTokens ?? 0; + const authOutput = Math.max(0, (usage.outputTokens ?? 0) - authReasoning); + + // Per-component max: the in-flight step's estimate ticks above the completed- + // steps base between boundaries, and the authoritative figure wins once it + // exceeds the (rough) estimate at the next boundary. Monotonic by construction. + return { + reasoning: Math.max(authReasoning, estReasoning), + output: Math.max(authOutput, estOutput), + authoritative: true, + }; } -- 2.49.1 From 5e8cb628f0bc8e77c19fe059026cdf4db5130c00 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:04:11 +0300 Subject: [PATCH 02/16] =?UTF-8?q?feat(ai-chat):=20compact=20reasoning=20re?= =?UTF-8?q?ndering=20=E2=80=94=20collapse=20blank=20lines=20(#181)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Thinking" (reasoning) block rendered with large vertical gaps: models emit reasoning with a blank line (\n\n) between every list item and paragraph, which `marked` turns into loose lists (each
  • wrapped in a

    ) and separate

    paragraphs, each carrying a margin. - Add `collapseBlankLines(text)`: collapse 2+ newlines to one, EXCEPT inside fenced code blocks (``` / ~~~) where blank lines are significant. Applied in reasoning-block.tsx before renderChatMarkdown, so loose lists become tight (no

  • ) and paragraphs join; `breaks: true` keeps single \n as
    , preserving line breaks. Reasoning-only — the normal answer is untouched. - Drop `white-space: pre-wrap` from `.reasoningText`: on the rendered markdown

    it turned the newlines between block tags into visible blank lines on top of the margins. The plain-text fallback that needs pre-wrap already sets it inline. Tests: collapseBlankLines unit (collapse, fence preservation incl. tilde and unclosed fences) + rendered-HTML assertions that a blank-line-separated list becomes a tight list and still parses as a list after a paragraph. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ai-chat/components/ai-chat.module.css | 6 +- .../ai-chat/components/reasoning-block.tsx | 8 ++- .../utils/collapse-blank-lines.test.ts | 61 +++++++++++++++++++ .../ai-chat/utils/collapse-blank-lines.ts | 56 +++++++++++++++++ 4 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 apps/client/src/features/ai-chat/utils/collapse-blank-lines.test.ts create mode 100644 apps/client/src/features/ai-chat/utils/collapse-blank-lines.ts diff --git a/apps/client/src/features/ai-chat/components/ai-chat.module.css b/apps/client/src/features/ai-chat/components/ai-chat.module.css index 71cc0e9d..cd788cdd 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat.module.css +++ b/apps/client/src/features/ai-chat/components/ai-chat.module.css @@ -161,7 +161,11 @@ margin-top: 4px; font-size: var(--mantine-font-size-xs); color: light-dark(var(--mantine-color-gray-7), var(--mantine-color-dark-1)); - white-space: pre-wrap; + /* NOTE: `white-space: pre-wrap` is intentionally NOT set here. On the + rendered markdown
    it would turn the newlines between block tags + (
    • \n
  • ,

    \n
      ) into visible blank lines/indents on top of the + margins. The plain-text fallback that needs pre-wrap sets it + inline itself (see reasoning-block.tsx). */ } .reasoningText p { diff --git a/apps/client/src/features/ai-chat/components/reasoning-block.tsx b/apps/client/src/features/ai-chat/components/reasoning-block.tsx index 43e88a69..de35229a 100644 --- a/apps/client/src/features/ai-chat/components/reasoning-block.tsx +++ b/apps/client/src/features/ai-chat/components/reasoning-block.tsx @@ -3,6 +3,7 @@ import { Box, Collapse, Group, Text, UnstyledButton } from "@mantine/core"; import { IconChevronDown } from "@tabler/icons-react"; import { useTranslation } from "react-i18next"; import { estimateTokens } from "@/features/ai-chat/utils/count-stream-tokens.ts"; +import { collapseBlankLines } from "@/features/ai-chat/utils/collapse-blank-lines.ts"; import { renderChatMarkdown } from "@/features/ai-chat/utils/markdown.ts"; import classes from "@/features/ai-chat/components/ai-chat.module.css"; @@ -33,7 +34,12 @@ export default function ReasoningBlock({ text, tokens }: ReasoningBlockProps) { // Authoritative count wins; otherwise estimate live from the streamed text. const count = tokens && tokens > 0 ? tokens : estimateTokens(text); const trimmed = text.trim(); - const html = trimmed ? renderChatMarkdown(trimmed, {}) : ""; + // Collapse the blank-line gaps the model emits between every list item / + // paragraph so the reasoning renders compactly (tight lists, joined + // paragraphs) — see collapseBlankLines. ONLY here, not in the normal answer. + const html = trimmed + ? renderChatMarkdown(collapseBlankLines(trimmed), {}) + : ""; return ( diff --git a/apps/client/src/features/ai-chat/utils/collapse-blank-lines.test.ts b/apps/client/src/features/ai-chat/utils/collapse-blank-lines.test.ts new file mode 100644 index 00000000..d61315dd --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/collapse-blank-lines.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect } from "vitest"; +import { collapseBlankLines } from "@/features/ai-chat/utils/collapse-blank-lines.ts"; +import { renderChatMarkdown } from "@/features/ai-chat/utils/markdown.ts"; + +describe("collapseBlankLines", () => { + it("collapses a run of 2+ newlines to a single newline", () => { + expect(collapseBlankLines("a\n\nb")).toBe("a\nb"); + expect(collapseBlankLines("a\n\n\n\nb")).toBe("a\nb"); + }); + + it("keeps single newlines untouched", () => { + expect(collapseBlankLines("a\nb\nc")).toBe("a\nb\nc"); + }); + + it("preserves blank lines INSIDE a fenced code block", () => { + const src = "a\n\n\nb\n\n```\nx\n\n\ny\n```\n\nc"; + // Prose blanks collapse; the blank lines between the ``` fences survive. + expect(collapseBlankLines(src)).toBe("a\nb\n```\nx\n\n\ny\n```\nc"); + }); + + it("handles a tilde fence and preserves its interior blanks", () => { + const src = "p\n\n~~~\ncode\n\nmore\n~~~\n\nq"; + expect(collapseBlankLines(src)).toBe("p\n~~~\ncode\n\nmore\n~~~\nq"); + }); + + it("leaves an unclosed fence's remaining lines verbatim", () => { + const src = "intro\n\n```\nstill\n\nopen"; + expect(collapseBlankLines(src)).toBe("intro\n```\nstill\n\nopen"); + }); + + it("is a no-op for text with no blank lines", () => { + expect(collapseBlankLines("just one line")).toBe("just one line"); + }); +}); + +describe("collapseBlankLines + renderChatMarkdown (tight reasoning rendering)", () => { + it("renders a blank-line-separated list as a TIGHT list (no

    1. )", () => { + const loose = + "Intro paragraph.\n\n- item one\n\n- item two\n\n- item three"; + const html = renderChatMarkdown(collapseBlankLines(loose), {}); + // Tight list: each

    2. holds the text directly, not wrapped in a

      . + expect(html).toContain("

    3. item one
      4. "); + expect(html).not.toContain("

    4. "); + // The list still parses as a list after the paragraph (not a paragraph+
      ). + expect(html).toContain("

        "); + expect(html).toContain("

        Intro paragraph.

        "); + }); + + it("renders an ordered list (1. 2.) as tight after collapsing", () => { + const loose = "Intro.\n\n1. first\n\n2. second"; + const html = renderChatMarkdown(collapseBlankLines(loose), {}); + expect(html).toContain("
          "); + expect(html).toContain("
        1. first
          2. "); + expect(html).not.toContain("

        2. "); + }); + + it("the loose source WOULD render

        3. without collapsing (control)", () => { + const loose = "- a\n\n- b"; + expect(renderChatMarkdown(loose, {})).toContain("

        4. "); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/collapse-blank-lines.ts b/apps/client/src/features/ai-chat/utils/collapse-blank-lines.ts new file mode 100644 index 00000000..17d49902 --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/collapse-blank-lines.ts @@ -0,0 +1,56 @@ +// Pure helper for compact reasoning ("Thinking") rendering. Kept free of React +// so it can be unit-tested in isolation (see collapse-blank-lines.test.ts). + +/** + * Collapse runs of 2+ newlines down to a single newline, EXCEPT inside fenced + * code blocks (``` ... ``` or ~~~ ... ~~~), where blank lines are significant. + * + * Why: reasoning models emit thinking with a blank line (`\n\n`) between every + * list item and paragraph. `marked` turns those into "loose" lists (each `

        5. ` + * wrapped in a `

          `) and separate `

          ` paragraphs, each carrying a vertical + * margin — so the "Thinking" block renders with large, airy gaps. Removing the + * blank-line gaps yields tight lists (no `

        6. `) and joined paragraphs. The + * chat markdown renderer runs with `breaks: true`, so a single `\n` still + * becomes a `
          ` — line breaks inside the reasoning are preserved; only the + * empty gaps between blocks disappear. Apply ONLY to reasoning text, never to a + * normal assistant answer (where paragraph spacing is intentional). + * + * Fenced code is preserved verbatim: a fence opens on a line whose first + * non-space characters are ``` or ~~~ and closes on the next line that starts + * with the same fence character. Blank lines between fences (significant for + * code formatting) are never collapsed. + */ +export function collapseBlankLines(text: string): string { + const lines = text.split("\n"); + const out: string[] = []; + let inFence = false; + let fenceChar = ""; + + for (const line of lines) { + const fenceMatch = line.match(/^\s*(`{3,}|~{3,})/); + if (fenceMatch) { + const ch = fenceMatch[1][0]; + if (!inFence) { + inFence = true; + fenceChar = ch; + } else if (ch === fenceChar) { + inFence = false; + } + out.push(line); + continue; + } + + // Inside a fenced block every line (including blanks) is significant. + if (inFence) { + out.push(line); + continue; + } + + // Outside fences: drop blank lines so a `\n\n+` gap collapses to a single + // `\n` between the surrounding content lines. + if (line.trim() === "") continue; + out.push(line); + } + + return out.join("\n"); +} -- 2.49.1 From a76667257412e1046f5f68d0d021f5ae7133bef9 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:07:21 +0300 Subject: [PATCH 03/16] fix(mcp): replaceImage no longer yanks the cursor (#164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mutateLiveContentUnlocked` — the write path used by `replaceImage` — still did the pre-#152 destructive write (delete the whole fragment + applyUpdate a fresh Y.Doc), discarding every Yjs node id. y-prosemirror anchors the editor selection to those ids, so an open editor's cursor snapped to the document end on every image swap, exactly the #152 jump that the main write path no longer causes. Switch it to the same `applyDocToFragment(ydoc, newDoc)` structural diff (updateYFragment) as the main path, so unchanged nodes keep their ids and the live cursor stays put. It runs its own atomic transact, so the old explicit transact/delete is gone; the now-unused docmostExtensions import is dropped. Regression tests (cursor-stability suite): a sibling paragraph's RelativePosition survives a top-level image src/attachmentId swap, and an image nested in a callout, matching the shapes replaceImage produces. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/build/client.js | 19 +++--- packages/mcp/src/client.ts | 22 +++--- .../unit/comment-cursor-stability.test.mjs | 67 +++++++++++++++++++ 3 files changed, 85 insertions(+), 23 deletions(-) diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index 302d2a15..46380a0c 100644 --- a/packages/mcp/build/client.js +++ b/packages/mcp/build/client.js @@ -7,8 +7,7 @@ import { TiptapTransformer } from "@hocuspocus/transformer"; import * as Y from "yjs"; import WebSocket from "ws"; import { convertProseMirrorToMarkdown } from "./lib/markdown-converter.js"; -import { updatePageContentRealtime, replacePageContent, markdownToProseMirror, mutatePageContent, buildCollabWsUrl, assertYjsEncodable, } from "./lib/collaboration.js"; -import { docmostExtensions } from "./lib/docmost-schema.js"; +import { updatePageContentRealtime, replacePageContent, markdownToProseMirror, mutatePageContent, buildCollabWsUrl, assertYjsEncodable, applyDocToFragment, } from "./lib/collaboration.js"; import { footnoteWarningsField } from "./lib/footnote-analyze.js"; import { buildPageTree } from "./lib/tree.js"; import { serializeDocmostMarkdown, parseDocmostMarkdown, } from "./lib/markdown-document.js"; @@ -361,14 +360,14 @@ export class DocmostClient { finish(null, mutationResult); return; } - const tempDoc = TiptapTransformer.toYdoc(newDoc, "default", docmostExtensions); - const fragment = ydoc.getXmlFragment("default"); - ydoc.transact(() => { - if (fragment.length > 0) { - fragment.delete(0, fragment.length); - } - Y.applyUpdate(ydoc, Y.encodeStateAsUpdate(tempDoc)); - }); + // Structural diff into the live fragment (issue #152), mirroring + // the main write path: preserves the Yjs ids of unchanged nodes so + // an open editor's cursor is not yanked to the end of the document. + // The previous destructive rewrite (delete-all + applyUpdate of a + // fresh Y.Doc) discarded every node id, so replaceImage — the only + // caller of this method — still reproduced the #152 cursor jump + // (#164). applyDocToFragment runs its own atomic `transact`. + applyDocToFragment(ydoc, newDoc); } catch (e) { finish(e instanceof Error ? e : new Error(String(e))); diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 5a8aaaf7..6293d5ee 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -20,9 +20,9 @@ import { mutatePageContent, buildCollabWsUrl, assertYjsEncodable, + applyDocToFragment, MutationResult, } from "./lib/collaboration.js"; -import { docmostExtensions } from "./lib/docmost-schema.js"; import { footnoteWarningsField } from "./lib/footnote-analyze.js"; import { buildPageTree } from "./lib/tree.js"; import { @@ -479,18 +479,14 @@ export class DocmostClient { return; } - const tempDoc = TiptapTransformer.toYdoc( - newDoc, - "default", - docmostExtensions, - ); - const fragment = ydoc.getXmlFragment("default"); - ydoc.transact(() => { - if (fragment.length > 0) { - fragment.delete(0, fragment.length); - } - Y.applyUpdate(ydoc, Y.encodeStateAsUpdate(tempDoc)); - }); + // Structural diff into the live fragment (issue #152), mirroring + // the main write path: preserves the Yjs ids of unchanged nodes so + // an open editor's cursor is not yanked to the end of the document. + // The previous destructive rewrite (delete-all + applyUpdate of a + // fresh Y.Doc) discarded every node id, so replaceImage — the only + // caller of this method — still reproduced the #152 cursor jump + // (#164). applyDocToFragment runs its own atomic `transact`. + applyDocToFragment(ydoc, newDoc); } catch (e) { finish(e instanceof Error ? e : new Error(String(e))); return; diff --git a/packages/mcp/test/unit/comment-cursor-stability.test.mjs b/packages/mcp/test/unit/comment-cursor-stability.test.mjs index 23614fb9..1bcca2af 100644 --- a/packages/mcp/test/unit/comment-cursor-stability.test.mjs +++ b/packages/mcp/test/unit/comment-cursor-stability.test.mjs @@ -162,3 +162,70 @@ test("assertYjsEncodable rejects an un-hydratable doc at preview time (fromJSON /Failed to encode document to Yjs/, ); }); + +// Issue #164: `replaceImage` went through `mutateLiveContentUnlocked`, which +// (unlike the main write path fixed in #152) still deleted the whole fragment +// and re-applied a fresh Y.Doc — discarding every node id, so an open editor's +// cursor jumped to the document end on an image swap. That method now uses the +// same `applyDocToFragment`, so a sibling paragraph's cursor anchor survives an +// image `src`/`attachmentId` replacement. These exercise that routine on the +// image shapes `replaceImage` produces (top-level and nested in a callout). + +const image = (attachmentId, src) => ({ + type: "image", + attrs: { attachmentId, src, width: "640", align: "center" }, +}); + +test("replacing a top-level image keeps a sibling paragraph's cursor anchor (#164)", () => { + const ydoc = new Y.Doc(); + applyDocToFragment( + ydoc, + doc(para("Caption above"), image("att-old", "/files/old.png")), + ); + + // The user's cursor sits in the (unchanged) caption paragraph. + const relPos = Y.createRelativePositionFromTypeIndex(paragraphText(ydoc, 0), 7); + + // Agent repoints the image to a freshly uploaded attachment (new id + src). + applyDocToFragment( + ydoc, + doc(para("Caption above"), image("att-new", "/files/new.png")), + ); + + const abs = Y.createAbsolutePositionFromRelativePosition(relPos, ydoc); + assert.notEqual(abs, null, "the caption cursor anchor must still resolve"); + assert.equal(abs.index, 7, "the cursor must stay at the same offset"); + // The swap actually landed: the image now carries the new attachment id/src. + const img = ydoc.getXmlFragment("default").get(1); + assert.equal(img.nodeName, "image"); + assert.equal(img.getAttribute("attachmentId"), "att-new"); + assert.equal(img.getAttribute("src"), "/files/new.png"); +}); + +test("replacing an image nested in a callout keeps an outer paragraph's anchor (#164)", () => { + const callout = (attachmentId, src) => ({ + type: "callout", + attrs: { type: "info" }, + content: [image(attachmentId, src)], + }); + const ydoc = new Y.Doc(); + applyDocToFragment( + ydoc, + doc(para("Intro paragraph"), callout("att-old", "/files/old.png")), + ); + + const relPos = Y.createRelativePositionFromTypeIndex(paragraphText(ydoc, 0), 5); + + applyDocToFragment( + ydoc, + doc(para("Intro paragraph"), callout("att-new", "/files/new.png")), + ); + + const abs = Y.createAbsolutePositionFromRelativePosition(relPos, ydoc); + assert.notEqual(abs, null, "the outer paragraph anchor must still resolve"); + assert.equal(abs.index, 5, "the cursor must stay at the same offset"); + // The nested image was repointed. + const calloutEl = ydoc.getXmlFragment("default").get(1); + const img = calloutEl.get(0); + assert.equal(img.getAttribute("attachmentId"), "att-new"); +}); -- 2.49.1 From 1cfad1f6fbb1a654be131ab259dd7b47f4831380 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:19:12 +0300 Subject: [PATCH 04/16] fix(db): jsonb double-encoding follow-ups from PR #172 review (#173) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #172 fixed the jsonb double-encoding for `tool_allowlist` but the same class of bug, and the same re-derived workaround, remained elsewhere. 1. model_config (agent roles): jsonbObject still used the buggy `::jsonb` bind, so `ai_agent_roles.model_config` round-tripped as a jsonb STRING SCALAR. The read-path `typeof === 'object'` check then failed and the model override was SILENTLY dropped (role fell back to the default model). Fixed to `::text::jsonb` and added `parseModelConfig` + `normalizeRow` so every read self-heals already-corrupted rows (no migration). 2. Centralized the write workaround as `jsonbBind()` in database/utils.ts — one implementation with one explanation of the quirk — replacing the per-repo `jsonbArray` (mcp) and `jsonbObject` (roles). 3. Integration coverage (the fix is a DB round-trip a unit test cannot see; the read-side parser MASKS a write regression): new ai-mcp-server-repo.int-spec asserts `jsonb_typeof(tool_allowlist)='array'` after insert + heals a seeded string-scalar row; ai-agent-roles-repo int-spec gains the same for `model_config` (`'object'` + heal). 4. Updated the stale `ai-mcp-servers.types.ts` comment (the driver returns a JSON string for legacy rows; the repo normalizes every read). 5. Fail-open logging: a corrupt tool_allowlist degrades to "no restriction" (agent gets ALL tools) — normalizeRow now warns (server id only, never contents) so the silent widening leaves a trace. 6. Simplified parseToolAllowlist (normalize the string once, then a single array-of-strings check) — identical behaviour, all 12 cases still pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/ai-chat/roles/jsonb-object.spec.ts | 30 ----- apps/server/src/database/jsonb-bind.spec.ts | 38 ++++++ .../ai-agent-roles.repo.spec.ts | 8 +- .../ai-agent-roles/ai-agent-roles.repo.ts | 67 ++++++++--- .../ai-agent-roles/parse-model-config.spec.ts | 46 ++++++++ .../repos/ai-chat/ai-mcp-server.repo.ts | 78 ++++++------- .../database/types/ai-mcp-servers.types.ts | 4 +- apps/server/src/database/utils.ts | 33 ++++++ .../ai-agent-roles-repo.int-spec.ts | 108 ++++++++++++++++-- .../ai-mcp-server-repo.int-spec.ts | 94 +++++++++++++++ 10 files changed, 402 insertions(+), 104 deletions(-) delete mode 100644 apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts create mode 100644 apps/server/src/database/jsonb-bind.spec.ts create mode 100644 apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts create mode 100644 apps/server/test/integration/ai-mcp-server-repo.int-spec.ts diff --git a/apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts b/apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts deleted file mode 100644 index 96875748..00000000 --- a/apps/server/src/core/ai-chat/roles/jsonb-object.spec.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { jsonbObject } from '@docmost/db/repos/ai-agent-roles/ai-agent-roles.repo'; - -/** - * Unit tests for jsonbObject: the repo helper that encodes a model_config object - * as a jsonb bind (or null when there is nothing to persist). It is the last - * line of defence before the column write, so the null-vs-bind decision is what - * matters here. We assert only null vs non-null because the non-null value is a - * kysely `sql` template fragment whose internal shape is an implementation - * detail of the SQL tag. - */ -describe('jsonbObject', () => { - it('returns null for null', () => { - expect(jsonbObject(null)).toBeNull(); - }); - - it('returns null for undefined', () => { - expect(jsonbObject(undefined)).toBeNull(); - }); - - it('returns null for an empty object (nothing to persist)', () => { - expect(jsonbObject({})).toBeNull(); - }); - - it('returns a (non-null) jsonb bind for a non-empty object', () => { - const out = jsonbObject({ driver: 'gemini', chatModel: 'gemini-2.0-flash' }); - // A real sql fragment is produced, never null/undefined. - expect(out).not.toBeNull(); - expect(out).toBeDefined(); - }); -}); diff --git a/apps/server/src/database/jsonb-bind.spec.ts b/apps/server/src/database/jsonb-bind.spec.ts new file mode 100644 index 00000000..4e9d3ffa --- /dev/null +++ b/apps/server/src/database/jsonb-bind.spec.ts @@ -0,0 +1,38 @@ +import { jsonbBind } from './utils'; + +/** + * Unit tests for jsonbBind: THE shared helper that encodes a JS array/object as + * a jsonb bind (or null when there is nothing to persist). It is the last line + * of defence before a jsonb column write, so the null-vs-bind decision is what + * matters here. We assert only null vs non-null because the non-null value is a + * kysely `sql` template fragment whose internal shape is an implementation + * detail of the SQL tag (the `::text::jsonb` double-encoding fix is verified + * end-to-end by the repo integration specs, where a real DB round-trip can + * actually observe `jsonb_typeof`). + */ +describe('jsonbBind', () => { + it('returns null for null / undefined', () => { + expect(jsonbBind(null)).toBeNull(); + expect(jsonbBind(undefined)).toBeNull(); + }); + + it('returns null for an empty array (nothing to persist)', () => { + expect(jsonbBind([])).toBeNull(); + }); + + it('returns null for an empty object (nothing to persist)', () => { + expect(jsonbBind({})).toBeNull(); + }); + + it('returns a (non-null) bind for a non-empty array', () => { + const out = jsonbBind(['search', 'crawl']); + expect(out).not.toBeNull(); + expect(out).toBeDefined(); + }); + + it('returns a (non-null) bind for a non-empty object', () => { + const out = jsonbBind({ driver: 'gemini', chatModel: 'gemini-2.0-flash' }); + expect(out).not.toBeNull(); + expect(out).toBeDefined(); + }); +}); diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts index 723c7627..3f1d2ede 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts @@ -35,7 +35,13 @@ describe('AiAgentRoleRepo.findLiveEnabled', () => { const result = await repo.findLiveEnabled('r-1', 'ws-1'); - expect(result).toBe(role); + // The repo normalizes the row (modelConfig parse), so it returns a COPY, not + // the same reference; assert the row's fields are carried through. + expect(result).toMatchObject({ + id: 'r-1', + workspaceId: 'ws-1', + enabled: true, + }); expect(db.selectFrom).toHaveBeenCalledWith('aiAgentRoles'); // Every security filter must be present. expect(where).toHaveBeenCalledWith('id', '=', 'r-1'); diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts index fb950585..1621b3e5 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts @@ -1,8 +1,7 @@ import { Injectable } from '@nestjs/common'; import { InjectKysely } from 'nestjs-kysely'; -import { sql } from 'kysely'; import { KyselyDB, KyselyTransaction } from '../../types/kysely.types'; -import { dbOrTx } from '../../utils'; +import { dbOrTx, jsonbBind } from '../../utils'; import { AiAgentRole } from '@docmost/db/types/entity.types'; /** The jsonb shape persisted in `model_config` (loosely typed for the column). */ @@ -23,13 +22,14 @@ export class AiAgentRoleRepo { id: string, workspaceId: string, ): Promise { - return this.db + const row = await this.db .selectFrom('aiAgentRoles') .selectAll('aiAgentRoles') .where('id', '=', id) .where('workspaceId', '=', workspaceId) .where('deletedAt', 'is', null) .executeTakeFirst(); + return row ? normalizeRow(row) : row; } /** @@ -45,7 +45,7 @@ export class AiAgentRoleRepo { id: string, workspaceId: string, ): Promise { - return this.db + const row = await this.db .selectFrom('aiAgentRoles') .selectAll('aiAgentRoles') .where('id', '=', id) @@ -53,17 +53,19 @@ export class AiAgentRoleRepo { .where('deletedAt', 'is', null) .where('enabled', '=', true) .executeTakeFirst(); + return row ? normalizeRow(row) : row; } /** All live roles for the workspace (management list + chat picker). */ async listByWorkspace(workspaceId: string): Promise { - return this.db + const rows = await this.db .selectFrom('aiAgentRoles') .selectAll('aiAgentRoles') .where('workspaceId', '=', workspaceId) .where('deletedAt', 'is', null) .orderBy('createdAt', 'asc') .execute(); + return rows.map(normalizeRow); } async insert( @@ -83,7 +85,7 @@ export class AiAgentRoleRepo { trx?: KyselyTransaction, ): Promise { const db = dbOrTx(this.db, trx); - return db + const row = await db .insertInto('aiAgentRoles') .values({ workspaceId: values.workspaceId, @@ -92,7 +94,11 @@ export class AiAgentRoleRepo { emoji: values.emoji ?? null, description: values.description ?? null, instructions: values.instructions, - modelConfig: jsonbObject(values.modelConfig), + // Cast: the generated `model_config` column type is the broad JsonValue + // union, which the concrete RawBuilder is not structurally + // assignable to (same reason the old jsonbObject cast to any). + // eslint-disable-next-line @typescript-eslint/no-explicit-any + modelConfig: jsonbBind(values.modelConfig) as any, enabled: values.enabled ?? true, autoStart: values.autoStart ?? true, // Empty string is treated as "no custom text" => null. @@ -100,6 +106,7 @@ export class AiAgentRoleRepo { }) .returningAll() .executeTakeFirst(); + return normalizeRow(row); } async update( @@ -127,7 +134,7 @@ export class AiAgentRoleRepo { if (patch.description !== undefined) set.description = patch.description; if (patch.instructions !== undefined) set.instructions = patch.instructions; if (patch.modelConfig !== undefined) { - set.modelConfig = jsonbObject(patch.modelConfig); + set.modelConfig = jsonbBind(patch.modelConfig); } if (patch.enabled !== undefined) set.enabled = patch.enabled; if (patch.autoStart !== undefined) set.autoStart = patch.autoStart; @@ -163,16 +170,40 @@ export class AiAgentRoleRepo { } /** - * Encode an object as a jsonb bind for the `model_config` column. The postgres - * driver would otherwise need an explicit cast; bind the JSON text and cast it. - * Returns null for null/undefined/empty objects. Cast to `any` because the - * generated column type is the broad `JsonValue` union, which a concrete object - * type is not structurally assignable to. + * Parse the `model_config` value read from the DB into the object the entity + * type promises. Rows written by the old double-encoding bind (`::jsonb` instead + * of `::text::jsonb`) round-trip as a JSON STRING, so the driver hands back e.g. + * `'{"driver":"gemini"}'` rather than an object; the read-path check + * `typeof cfg === 'object'` then failed and the model override was SILENTLY + * dropped (the role fell back to the default model). Be tolerant: a JSON string + * is parsed; an already-parsed object passes through; null / a non-object (incl. + * an array) / unparseable value becomes null (= no override). This self-heals + * already-corrupted rows on read, no migration required. */ -export function jsonbObject(value: ModelConfigValue | undefined) { - if (value === null || value === undefined || Object.keys(value).length === 0) { - return null; +export function parseModelConfig( + value: unknown, +): Record | null { + let v: unknown = value; + if (typeof v === 'string') { + try { + v = JSON.parse(v); // legacy double-encoded read + } catch { + return null; + } } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return sql`${JSON.stringify(value)}::jsonb` as any; + return v !== null && typeof v === 'object' && !Array.isArray(v) + ? (v as Record) + : null; +} + +/** Normalize a DB row so `modelConfig` is always an object or null. The cast + * bridges parseModelConfig's concrete `Record | null` to the column's broad + * generated `JsonValue` type (an object is a valid JsonValue at runtime). */ +function normalizeRow(row: AiAgentRole): AiAgentRole { + return { + ...row, + modelConfig: parseModelConfig( + row.modelConfig, + ) as AiAgentRole['modelConfig'], + }; } diff --git a/apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts b/apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts new file mode 100644 index 00000000..16392305 --- /dev/null +++ b/apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts @@ -0,0 +1,46 @@ +import { parseModelConfig } from './ai-agent-roles.repo'; + +/** + * Unit tests for parseModelConfig: the read-side normalizer that repairs the + * jsonb double-encoding regression on `model_config`. Rows written by the old + * `::jsonb` bind round-trip as a JSON STRING, which the read path's + * `typeof === 'object'` check rejected — silently dropping the model override. + * parseModelConfig accepts an already-parsed object, parses a legacy JSON + * string, and rejects everything that is not an object (null = no override). + */ +describe('parseModelConfig', () => { + it('passes an already-parsed object through', () => { + expect(parseModelConfig({ driver: 'gemini' })).toEqual({ + driver: 'gemini', + }); + }); + + it('parses a legacy double-encoded JSON string into an object', () => { + expect(parseModelConfig('{"driver":"gemini","chatModel":"x"}')).toEqual({ + driver: 'gemini', + chatModel: 'x', + }); + }); + + it('returns null for null / undefined', () => { + expect(parseModelConfig(null)).toBeNull(); + expect(parseModelConfig(undefined)).toBeNull(); + }); + + it('returns null for a non-object JSON value (string/number/array)', () => { + expect(parseModelConfig('"justastring"')).toBeNull(); + expect(parseModelConfig('42')).toBeNull(); + // An array is an object in JS but not a valid model_config shape. + expect(parseModelConfig('["a","b"]')).toBeNull(); + expect(parseModelConfig(['a', 'b'])).toBeNull(); + }); + + it('returns null for an unparseable string', () => { + expect(parseModelConfig('not json at all')).toBeNull(); + }); + + it('returns null for a raw non-object primitive', () => { + expect(parseModelConfig(42 as unknown)).toBeNull(); + expect(parseModelConfig(true as unknown)).toBeNull(); + }); +}); diff --git a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts index a0f2da50..f17d7485 100644 --- a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts +++ b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts @@ -1,10 +1,11 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, Logger } from '@nestjs/common'; import { InjectKysely } from 'nestjs-kysely'; -import { sql } from 'kysely'; import { KyselyDB, KyselyTransaction } from '../../types/kysely.types'; -import { dbOrTx } from '../../utils'; +import { dbOrTx, jsonbBind } from '../../utils'; import { AiMcpServer } from '@docmost/db/types/entity.types'; +const logger = new Logger('AiMcpServerRepo'); + /** * Repository for per-workspace external MCP servers the agent may use (§5.4). * @@ -75,7 +76,7 @@ export class AiMcpServerRepo { headersEnc: values.headersEnc ?? null, // jsonb column: the postgres driver would otherwise encode a JS array as // a Postgres array literal. Bind the JSON text and cast it to jsonb. - toolAllowlist: jsonbArray(values.toolAllowlist), + toolAllowlist: jsonbBind(values.toolAllowlist), enabled: values.enabled ?? true, }) .returningAll() @@ -104,7 +105,7 @@ export class AiMcpServerRepo { if (patch.url !== undefined) set.url = patch.url; if (patch.headersEnc !== undefined) set.headersEnc = patch.headersEnc; if (patch.toolAllowlist !== undefined) { - set.toolAllowlist = jsonbArray(patch.toolAllowlist); + set.toolAllowlist = jsonbBind(patch.toolAllowlist); } if (patch.enabled !== undefined) set.enabled = patch.enabled; await db @@ -129,58 +130,43 @@ export class AiMcpServerRepo { } } -/** - * Encode a string[] as a jsonb bind for the `tool_allowlist` column. Passing a - * plain JS array to the postgres driver would serialize it as a Postgres array - * literal (incompatible with jsonb), so we bind the JSON text and cast it. - * - * The cast is `::text::jsonb`, NOT `::jsonb`: if the parameter is bound straight - * to a jsonb cast, node-postgres infers its type as jsonb and JSON-stringifies - * the (already-JSON) string a SECOND time, so the column ends up holding a jsonb - * STRING SCALAR (`"[\"a\"]"`) instead of a jsonb ARRAY. Forcing the param through - * `::text` first binds it as text (sent verbatim), and `::jsonb` then parses it - * into a real array. (`normalizeRow` below repairs rows written the old way.) - * - * Returns null for null/empty arrays (an empty allowlist means "no restriction" - * is not intended — callers pass null to clear; an empty array is normalized to - * null here so it never round-trips as `[]`). - */ -function jsonbArray(value: string[] | null | undefined) { - if (value === null || value === undefined || value.length === 0) { - return null; - } - // Typed as string[] so it is assignable to the toolAllowlist column. - return sql`${JSON.stringify(value)}::text::jsonb`; -} - /** * Parse the `toolAllowlist` value read from the DB into the `string[] | null` * the entity type promises. The jsonb column historically round-trips as a JSON - * STRING (rows written by the old double-encoding `jsonbArray`, see above), so - * the driver hands back a string like `'["a","b"]'` rather than an array. Be - * tolerant: an already-parsed array passes through; a JSON string is parsed; null - * / a non-array / unparseable value becomes null (unrestricted). + * STRING (rows written by the old double-encoding bind before the `::text::jsonb` + * fix), so the driver hands back a string like `'["a","b"]'` rather than an + * array. Be tolerant: normalize a JSON string to its value, then accept it only + * if it is an array of strings; null / a non-array / unparseable value / an + * array with a non-string element all become null (unrestricted). */ export function parseToolAllowlist(value: unknown): string[] | null { - if (value == null) return null; - if (Array.isArray(value)) { - return value.every((v) => typeof v === 'string') ? (value as string[]) : null; - } - if (typeof value === 'string') { + let v: unknown = value; + if (typeof v === 'string') { try { - const parsed = JSON.parse(value); - return Array.isArray(parsed) && - parsed.every((v) => typeof v === 'string') - ? (parsed as string[]) - : null; + v = JSON.parse(v); // legacy double-encoded read } catch { return null; } } - return null; + return Array.isArray(v) && v.every((x) => typeof x === 'string') + ? (v as string[]) + : null; } -/** Normalize a DB row so `toolAllowlist` is always `string[] | null`. */ +/** + * Normalize a DB row so `toolAllowlist` is always `string[] | null`. + * + * FAIL-OPEN logging: a stored value that is present but cannot be parsed into a + * string[] (corrupt JSON, a non-array, non-string elements) degrades to `null` = + * "no restriction", so the agent silently gets ALL of the server's tools. Log + * one line (server id only, never the contents) so that widening is not silent. + */ function normalizeRow(row: AiMcpServer): AiMcpServer { - return { ...row, toolAllowlist: parseToolAllowlist(row.toolAllowlist) }; + const parsed = parseToolAllowlist(row.toolAllowlist); + if (parsed === null && row.toolAllowlist != null) { + logger.warn( + `Corrupt tool_allowlist for MCP server ${row.id}; ignoring it (no tool restriction applied)`, + ); + } + return { ...row, toolAllowlist: parsed }; } diff --git a/apps/server/src/database/types/ai-mcp-servers.types.ts b/apps/server/src/database/types/ai-mcp-servers.types.ts index 677f45fe..c0d75622 100644 --- a/apps/server/src/database/types/ai-mcp-servers.types.ts +++ b/apps/server/src/database/types/ai-mcp-servers.types.ts @@ -20,7 +20,9 @@ export interface AiMcpServers { // Encrypted JSON of the auth headers. Nullable (a server may need no auth). headersEnc: string | null; // Optional allowlist of remote tool names to expose; null = expose all. - // Stored as jsonb; reads come back as a string[] from the postgres driver. + // Stored as jsonb. The postgres driver may return a JSON string for legacy + // double-encoded rows; `AiMcpServerRepo` normalizes every read to + // `string[] | null` via `parseToolAllowlist`. toolAllowlist: string[] | null; enabled: Generated; createdAt: Generated; diff --git a/apps/server/src/database/utils.ts b/apps/server/src/database/utils.ts index 6c11339c..c493798c 100644 --- a/apps/server/src/database/utils.ts +++ b/apps/server/src/database/utils.ts @@ -1,3 +1,4 @@ +import { sql, RawBuilder } from 'kysely'; import { KyselyDB, KyselyTransaction } from './types/kysely.types'; /* @@ -31,3 +32,35 @@ export function dbOrTx( return db; // Use normal database instance } } + +/** + * Bind a JS array/object as a `jsonb` column value, working around a postgres + * driver double-encoding quirk. THE single implementation — repos that persist + * jsonb (`tool_allowlist`, `model_config`, ...) call this instead of re-deriving + * the cast. + * + * THE QUIRK: with the `kysely-postgres-js` / postgres.js driver, casting a bound + * parameter straight to `::jsonb` makes the driver infer the param type as jsonb + * and JSON-stringify the (already-JSON) text a SECOND time, so the column ends + * up holding a jsonb STRING SCALAR (`"[\"a\"]"` / `"{\"k\":1}"`) instead of a + * real jsonb array/object. Read paths then see a string, not the structure, and + * silently fall back (an allowlist becomes "unrestricted", a model override is + * ignored). Forcing the param through `::text` first binds it as text (sent + * verbatim); `::jsonb` then parses it into a real array/object. Read-side + * parsers repair rows written the old buggy way without a migration. + * + * Returns `null` for null/undefined and for "empty" values (an empty array, or + * an object with no own enumerable keys) — callers treat empty as "clear/unset", + * so an empty allowlist/config never round-trips as `[]`/`{}`. + */ +export function jsonbBind( + value: T | null | undefined, +): RawBuilder | null { + if (value === null || value === undefined) return null; + if (Array.isArray(value)) { + if (value.length === 0) return null; + } else if (typeof value === 'object') { + if (Object.keys(value as object).length === 0) return null; + } + return sql`${JSON.stringify(value)}::text::jsonb`; +} diff --git a/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts b/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts index 9129dd75..454a6e1d 100644 --- a/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts +++ b/apps/server/test/integration/ai-agent-roles-repo.int-spec.ts @@ -1,4 +1,5 @@ -import { Kysely } from 'kysely'; +import { Kysely, sql } from 'kysely'; +import { randomUUID } from 'node:crypto'; import { AiAgentRoleRepo } from '@docmost/db/repos/ai-agent-roles/ai-agent-roles.repo'; import { getTestDb, destroyTestDb, createWorkspace } from './db'; @@ -25,8 +26,16 @@ describe('AiAgentRoleRepo isolation + partial unique index [integration]', () => }); it('findById / listByWorkspace exclude soft-deleted rows', async () => { - const live = await repo.insert({ workspaceId: w1, name: 'Live', instructions: 'x' }); - const dead = await repo.insert({ workspaceId: w1, name: 'Dead', instructions: 'x' }); + const live = await repo.insert({ + workspaceId: w1, + name: 'Live', + instructions: 'x', + }); + const dead = await repo.insert({ + workspaceId: w1, + name: 'Dead', + instructions: 'x', + }); await repo.softDelete(dead.id, w1); expect(await repo.findById(live.id, w1)).toBeDefined(); @@ -38,7 +47,11 @@ describe('AiAgentRoleRepo isolation + partial unique index [integration]', () => }); it('findById of a W2 role from W1 context returns undefined (tenant isolation)', async () => { - const w2role = await repo.insert({ workspaceId: w2, name: 'W2Role', instructions: 'x' }); + const w2role = await repo.insert({ + workspaceId: w2, + name: 'W2Role', + instructions: 'x', + }); expect(await repo.findById(w2role.id, w2)).toBeDefined(); // Same id, wrong workspace context -> not visible. @@ -58,21 +71,100 @@ describe('AiAgentRoleRepo isolation + partial unique index [integration]', () => }); it('same name is reusable after softDelete (partial unique index WHERE deleted_at IS NULL)', async () => { - const first = await repo.insert({ workspaceId: w1, name: 'Reusable', instructions: 'x' }); + const first = await repo.insert({ + workspaceId: w1, + name: 'Reusable', + instructions: 'x', + }); await repo.softDelete(first.id, w1); // Now inserting the same name must succeed because the soft-deleted row is // excluded from the partial unique index. - const second = await repo.insert({ workspaceId: w1, name: 'Reusable', instructions: 'x' }); + const second = await repo.insert({ + workspaceId: w1, + name: 'Reusable', + instructions: 'x', + }); expect(second.id).toBeDefined(); expect(second.id).not.toBe(first.id); }); it('same name in W1 and W2 is allowed (unique is per-workspace)', async () => { - const a = await repo.insert({ workspaceId: w1, name: 'CrossTenant', instructions: 'x' }); - const b = await repo.insert({ workspaceId: w2, name: 'CrossTenant', instructions: 'x' }); + const a = await repo.insert({ + workspaceId: w1, + name: 'CrossTenant', + instructions: 'x', + }); + const b = await repo.insert({ + workspaceId: w2, + name: 'CrossTenant', + instructions: 'x', + }); expect(a.id).toBeDefined(); expect(b.id).toBeDefined(); expect(a.id).not.toBe(b.id); }); + + // model_config jsonb round-trip (issue #173 §1): the same double-encoding bug + // PR #172 fixed for tool_allowlist lived in jsonbObject. A DB round-trip is the + // only way to observe it — the write must land as a real jsonb OBJECT, and a + // legacy string-scalar row must self-heal on read (else the model override is + // silently dropped and the role falls back to the default model). + const jsonbTypeof = async (id: string): Promise => { + const res = await sql<{ t: string | null }>` + SELECT jsonb_typeof(model_config) AS t + FROM ai_agent_roles WHERE id = ${id} + `.execute(db); + return res.rows[0]?.t ?? null; + }; + + it('insert stores model_config as a jsonb OBJECT and reads it back as an object', async () => { + const role = await repo.insert({ + workspaceId: w1, + name: `Model-${randomUUID()}`, + instructions: 'x', + modelConfig: { driver: 'gemini', chatModel: 'gemini-2.0-flash' }, + }); + expect(await jsonbTypeof(role.id)).toBe('object'); + // The returned row is already normalized to an object. + expect(role.modelConfig).toEqual({ + driver: 'gemini', + chatModel: 'gemini-2.0-flash', + }); + const found = await repo.findById(role.id, w1); + expect(found?.modelConfig).toEqual({ + driver: 'gemini', + chatModel: 'gemini-2.0-flash', + }); + }); + + it('an empty model_config is normalized to null (no override)', async () => { + const role = await repo.insert({ + workspaceId: w1, + name: `Empty-${randomUUID()}`, + instructions: 'x', + modelConfig: {}, + }); + // The column is SQL NULL, so jsonb_typeof returns SQL NULL (JS null). + expect(await jsonbTypeof(role.id)).toBeNull(); + expect((await repo.findById(role.id, w1))?.modelConfig).toBeNull(); + }); + + it('repairs a legacy double-encoded (string scalar) model_config on read', async () => { + const id = randomUUID(); + // Seed the corrupt string-scalar shape the old `::jsonb` bind produced. + await sql` + INSERT INTO ai_agent_roles (id, workspace_id, name, instructions, model_config) + VALUES ( + ${id}, ${w1}, ${`Legacy-${id}`}, 'x', + to_jsonb(${'{"driver":"openai","chatModel":"gpt"}'}::text) + ) + `.execute(db); + expect(await jsonbTypeof(id)).toBe('string'); // sanity: really corrupt + + expect((await repo.findById(id, w1))?.modelConfig).toEqual({ + driver: 'openai', + chatModel: 'gpt', + }); + }); }); diff --git a/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts b/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts new file mode 100644 index 00000000..c1949a57 --- /dev/null +++ b/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts @@ -0,0 +1,94 @@ +import { Kysely, sql } from 'kysely'; +import { randomUUID } from 'node:crypto'; +import { AiMcpServerRepo } from '@docmost/db/repos/ai-chat/ai-mcp-server.repo'; +import { getTestDb, destroyTestDb, createWorkspace } from './db'; + +/** + * AiMcpServerRepo `tool_allowlist` jsonb round-trip (PR #172 / issue #173 §3). + * + * The fix under test is a DB round-trip, so a unit test cannot observe it: the + * write must land as a real jsonb ARRAY (not a double-encoded string scalar), + * and the read must repair any legacy string-scalar rows. The read-side + * `parseToolAllowlist` MASKS a write regression (it parses the string back), so + * without this integration check, reverting `::text::jsonb` to `::jsonb` would + * keep every unit test green while silently corrupting the column again. + */ +describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () => { + let db: Kysely; + let repo: AiMcpServerRepo; + let ws: string; + + beforeAll(async () => { + db = getTestDb(); + repo = new AiMcpServerRepo(db as any); + ws = (await createWorkspace(db)).id; + }); + + afterAll(async () => { + await destroyTestDb(); + }); + + const jsonbTypeof = async (id: string): Promise => { + const res = await sql<{ t: string | null }>` + SELECT jsonb_typeof(tool_allowlist) AS t + FROM ai_mcp_servers WHERE id = ${id} + `.execute(db); + return res.rows[0]?.t ?? null; + }; + + it('insert stores the allowlist as a jsonb ARRAY (not a string scalar)', async () => { + const row = await repo.insert({ + workspaceId: ws, + name: `srv-${randomUUID()}`, + transport: 'http', + url: 'https://example.com/mcp', + toolAllowlist: ['search', 'crawl'], + }); + + // The column holds a real jsonb array — the whole point of ::text::jsonb. + expect(await jsonbTypeof(row.id)).toBe('array'); + + // And the read returns a genuine string[], not a JSON string. + const found = await repo.findById(row.id, ws); + expect(found?.toolAllowlist).toEqual(['search', 'crawl']); + expect(Array.isArray(found?.toolAllowlist)).toBe(true); + }); + + it('an empty allowlist is normalized to null (no restriction), not []', async () => { + const row = await repo.insert({ + workspaceId: ws, + name: `srv-${randomUUID()}`, + transport: 'http', + url: 'https://example.com/mcp', + toolAllowlist: [], + }); + // The column is SQL NULL, so jsonb_typeof returns SQL NULL (JS null). + expect(await jsonbTypeof(row.id)).toBeNull(); + expect((await repo.findById(row.id, ws))?.toolAllowlist).toBeNull(); + }); + + it('repairs a legacy double-encoded (string scalar) row on read (self-heal)', async () => { + // Seed a row whose tool_allowlist is a jsonb STRING SCALAR holding the JSON + // text — exactly what the old `::jsonb` double-encoding produced. + const id = randomUUID(); + await sql` + INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist) + VALUES ( + ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp', + to_jsonb(${'["alpha","beta"]'}::text) + ) + `.execute(db); + + // Sanity: the seeded column really IS the corrupt string-scalar shape. + expect(await jsonbTypeof(id)).toBe('string'); + + // The repo read heals it back to a real string[]. + expect((await repo.findById(id, ws))?.toolAllowlist).toEqual([ + 'alpha', + 'beta', + ]); + const enabled = await repo.listEnabled(ws); + const healed = enabled.find((r) => r.id === id); + expect(healed?.toolAllowlist).toEqual(['alpha', 'beta']); + }); +}); -- 2.49.1 From 47a2ae420b525b12219077f47013e5ae49a707ff Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:27:29 +0300 Subject: [PATCH 05/16] =?UTF-8?q?feat(footnotes):=20multi-backlinks=20?= =?UTF-8?q?=E2=80=94=20definition=20returns=20to=20ALL=20its=20references?= =?UTF-8?q?=20(#168)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After #166 a repeated `[^a]` is one footnote (reuse): one number, one definition, N forward links. But the definition's ↩ only returned to the FIRST reference. Now a definition with N references shows ↩ a b c …, each backlink scrolling to its own occurrence (Pandoc/Wikipedia convention); a single-reference footnote keeps the plain ↩ unchanged. - editor-ext: `computeFootnoteRefCounts(doc)` (id -> occurrence count) cached alongside the number map in the numbering plugin state; `getFootnoteRefCount` getter (O(1), no per-render doc walk). `scrollToReference(id, index?)` picks the index-th `sup[data-footnote-ref][data-id]` occurrence (document order), falling back to the first. - client: FootnoteDefinitionView renders one lettered link (a, b, c, … aa …) per occurrence when refCount > 1; the chrome stays after the contentDOM so the #146 caret invariant holds. i18n keys (ru) added. Tests: computeFootnoteRefCounts + getFootnoteRefCount (reuse counts, unknown id => 0); structure test gains 3 cases (N lettered links render, click jumps to the n-th occorrence, single ref => one ↩). NOTE: the visual layout of the backlink row needs a real browser to verify (jsdom can't); the structural and behavioral contract is covered headless. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../public/locales/ru-RU/translation.json | 2 + .../footnote/footnote-definition-view.tsx | 81 ++- .../footnote-views.structure.test.tsx | 83 ++- .../components/footnote/footnote.module.css | 15 + .../src/lib/footnote/footnote-numbering.ts | 35 +- .../src/lib/footnote/footnote-reference.ts | 55 +- .../src/lib/footnote/footnote-util.ts | 40 +- .../src/lib/footnote/footnote.test.ts | 521 +++++++++++------- 8 files changed, 558 insertions(+), 274 deletions(-) diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 0d4926cd..336e8688 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -405,6 +405,8 @@ "Footnote {{number}}": "Сноска {{number}}", "Go to footnote": "Перейти к сноске", "Back to reference": "Вернуться к ссылке", + "Back to references": "Вернуться к ссылкам", + "Back to reference {{label}}": "Вернуться к ссылке {{label}}", "Empty footnote": "Пустая сноска", "Math inline": "Строчная формула", "Insert inline math equation.": "Вставить математическое выражение в строку.", diff --git a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx index e3e0522a..7f6cc7b3 100644 --- a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx +++ b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx @@ -1,25 +1,45 @@ import { NodeViewContent, NodeViewProps, NodeViewWrapper } from "@tiptap/react"; import { useTranslation } from "react-i18next"; -import { getFootnoteNumber } from "@docmost/editor-ext"; +import { getFootnoteNumber, getFootnoteRefCount } from "@docmost/editor-ext"; import classes from "./footnote.module.css"; +/** + * A 0-based backlink index -> its lowercase letter label (0 -> "a", 25 -> "z", + * 26 -> "aa", ...), matching the Pandoc/Wikipedia "↩ a b c" convention. + */ +function backlinkLabel(index: number): string { + let out = ""; + let x = index; + while (x >= 0) { + out = String.fromCharCode(97 + (x % 26)) + out; + x = Math.floor(x / 26) - 1; + } + return out; +} + /** * NodeView for a single footnote definition: a decorative number marker, the * editable content (NodeViewContent), and a "↩" back-link to its reference. * The number is derived from the document (not stored). + * + * After #166 a footnote can be referenced more than once (one number, one + * definition, N forward links). When it is, the back-link becomes a row of + * per-occurrence links — ↩ a b c … — each scrolling to its own reference (#168); + * a single-reference footnote keeps the plain ↩. */ export default function FootnoteDefinitionView(props: NodeViewProps) { const { node, editor } = props; const { t } = useTranslation(); const id = node.attrs.id as string; - // Read the cached number from the numbering plugin (computed once per doc - // change) rather than recomputing the whole map on every render. + // Read the cached number/ref-count from the numbering plugin (computed once + // per doc change) rather than recomputing the whole map on every render. const number = getFootnoteNumber(editor.state, id) ?? "?"; + const refCount = getFootnoteRefCount(editor.state, id); - const handleBack = (e: React.MouseEvent) => { + const jumpTo = (e: React.MouseEvent, index: number) => { e.preventDefault(); - editor.commands.scrollToReference(id); + editor.commands.scrollToReference(id, index); }; return ( @@ -42,16 +62,47 @@ export default function FootnoteDefinitionView(props: NodeViewProps) { > {number}. - - ↩ - + {refCount > 1 ? ( + // Multiple references -> ↩ followed by one lettered link per occurrence. + + + {Array.from({ length: refCount }, (_, i) => ( + jumpTo(e, i)} + role="button" + aria-label={t("Back to reference {{label}}", { + label: backlinkLabel(i), + })} + title={t("Back to reference {{label}}", { + label: backlinkLabel(i), + })} + > + {backlinkLabel(i)} + + ))} + + ) : ( + // Single reference -> the plain ↩ (unchanged behavior). + jumpTo(e, 0)} + role="button" + aria-label={t("Back to reference")} + title={t("Back to reference")} + > + ↩ + + )} ); } diff --git a/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx b/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx index 3e28493d..e6cd46a6 100644 --- a/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx +++ b/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx @@ -1,5 +1,5 @@ -import { describe, it, expect, vi } from "vitest"; -import { render } from "@testing-library/react"; +import { describe, it, expect, vi, afterEach } from "vitest"; +import { render, fireEvent } from "@testing-library/react"; /** * Structural regression guard for #146 (PR #147). @@ -36,10 +36,14 @@ vi.mock("react-i18next", () => ({ useTranslation: () => ({ t: (key: string) => key }), })); -// footnote-definition-view reads a cached number from the numbering plugin; -// stub it so we don't need a live ProseMirror state. +// footnote-definition-view reads a cached number + reference count from the +// numbering plugin; stub them so we don't need a live ProseMirror state. The +// ref-count is a hoisted mutable so a test can drive the single-vs-multi +// backlink branch (#168). Default 1 = single reference (the #146 cases). +const { mockRefCount } = vi.hoisted(() => ({ mockRefCount: { value: 1 } })); vi.mock("@docmost/editor-ext", () => ({ getFootnoteNumber: () => 1, + getFootnoteRefCount: () => mockRefCount.value, })); // Mocks so CodeBlockView renders cheaply (no MantineProvider, no matchMedia). @@ -59,7 +63,8 @@ vi.mock("@mantine/core", () => ({ ), })); vi.mock("@/components/common/copy-button", () => ({ - CopyButton: ({ children }: any) => children({ copied: false, copy: () => {} }), + CopyButton: ({ children }: any) => + children({ copied: false, copy: () => {} }), })); vi.mock("@tabler/icons-react", () => ({ IconCheck: () => null, @@ -141,3 +146,71 @@ describe("#146 editable NodeView contentDOM-first invariant", () => { }, ); }); + +// #168: a footnote referenced more than once shows one lettered backlink per +// occurrence (↩ a b c), each scrolling to its own reference; a single-reference +// footnote keeps the plain ↩. +describe("#168 footnote definition multi-backlinks", () => { + afterEach(() => { + // Reset the shared ref-count mock so other tests see a single reference. + mockRefCount.value = 1; + }); + + const makeProps = () => + ({ + node: { attrs: { id: "fn-1" }, textContent: "" }, + editor: { + state: {}, + isEditable: true, + commands: { scrollToReference: vi.fn() }, + }, + getPos: () => 0, + updateAttributes: () => {}, + deleteNode: () => {}, + }) as any; + + it("renders one lettered backlink per reference (a, b, c) plus the ↩ arrow", () => { + mockRefCount.value = 3; + const { getByTestId } = render(); + const wrapper = getByTestId("nvw"); + + const links = wrapper.querySelectorAll('[role="button"]'); + expect(Array.from(links).map((l) => l.textContent)).toEqual([ + "a", + "b", + "c", + ]); + // The ↩ arrow is present (as decorative chrome, not a button). + expect(wrapper.textContent).toContain("↩"); + }); + + it("clicking the n-th backlink scrolls to the n-th occurrence (0-based)", () => { + mockRefCount.value = 3; + const props = makeProps(); + const { getByTestId } = render(); + const links = getByTestId("nvw").querySelectorAll('[role="button"]'); + + fireEvent.click(links[1]); // "b" + expect(props.editor.commands.scrollToReference).toHaveBeenCalledWith( + "fn-1", + 1, + ); + }); + + it("a single-reference footnote renders just one ↩ (no letters)", () => { + mockRefCount.value = 1; + const props = makeProps(); + const { getByTestId } = render(); + const wrapper = getByTestId("nvw"); + + const links = wrapper.querySelectorAll('[role="button"]'); + expect(links.length).toBe(1); + expect(links[0].textContent).toBe("↩"); + + fireEvent.click(links[0]); + expect(props.editor.commands.scrollToReference).toHaveBeenCalledWith( + "fn-1", + 0, + ); + }); +}); diff --git a/apps/client/src/features/editor/components/footnote/footnote.module.css b/apps/client/src/features/editor/components/footnote/footnote.module.css index 8f1ba9e7..fb21fc03 100644 --- a/apps/client/src/features/editor/components/footnote/footnote.module.css +++ b/apps/client/src/features/editor/components/footnote/footnote.module.css @@ -115,3 +115,18 @@ .backLink:hover { text-decoration: underline; } + +/* Multi-backlink row (#168): ↩ a b c — one lettered link per reference + occurrence. Sits on the right, after the content, like the single ↩. */ +.backLinks { + flex: 0 0 auto; + display: inline-flex; + align-items: baseline; + gap: 0.3em; + user-select: none; +} + +.backLinkArrow { + color: var(--mantine-color-dimmed); + font-size: 0.9em; +} diff --git a/packages/editor-ext/src/lib/footnote/footnote-numbering.ts b/packages/editor-ext/src/lib/footnote/footnote-numbering.ts index 8a487b1f..3a0950a4 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-numbering.ts +++ b/packages/editor-ext/src/lib/footnote/footnote-numbering.ts @@ -1,14 +1,15 @@ -import { EditorState, Plugin, PluginKey } from "@tiptap/pm/state"; -import { Decoration, DecorationSet } from "@tiptap/pm/view"; -import { Node as ProseMirrorNode } from "@tiptap/pm/model"; +import { EditorState, Plugin, PluginKey } from '@tiptap/pm/state'; +import { Decoration, DecorationSet } from '@tiptap/pm/view'; +import { Node as ProseMirrorNode } from '@tiptap/pm/model'; import { FOOTNOTE_DEFINITION_NAME, FOOTNOTE_REFERENCE_NAME, computeFootnoteNumbers, -} from "./footnote-util"; + computeFootnoteRefCounts, +} from './footnote-util'; export const footnoteNumberingPluginKey = new PluginKey( - "footnoteNumbering", + 'footnoteNumbering', ); /** @@ -21,6 +22,9 @@ export const footnoteNumberingPluginKey = new PluginKey( interface FootnoteNumberingState { /** referenceId -> 1-based display number, for the current doc. */ numbers: Map; + /** referenceId -> number of reference occurrences (>= 1), for the definition's + * multi-backlink UI (#168). */ + refCounts: Map; /** Decorations rendering those numbers (refs + definitions). */ decorations: DecorationSet; } @@ -46,6 +50,7 @@ function buildFootnoteNumberingState( doc: ProseMirrorNode, ): FootnoteNumberingState { const numbers = computeFootnoteNumbers(doc); + const refCounts = computeFootnoteRefCounts(doc); const decorations: Decoration[] = []; doc.descendants((node, pos) => { @@ -54,7 +59,7 @@ function buildFootnoteNumberingState( if (num != null) { decorations.push( Decoration.node(pos, pos + node.nodeSize, { - "data-footnote-number": String(num), + 'data-footnote-number': String(num), style: `--footnote-number: "${num}";`, }), ); @@ -65,7 +70,7 @@ function buildFootnoteNumberingState( if (num != null) { decorations.push( Decoration.node(pos, pos + node.nodeSize, { - "data-footnote-number": String(num), + 'data-footnote-number': String(num), style: `--footnote-number: "${num}";`, }), ); @@ -73,7 +78,11 @@ function buildFootnoteNumberingState( } }); - return { numbers, decorations: DecorationSet.create(doc, decorations) }; + return { + numbers, + refCounts, + decorations: DecorationSet.create(doc, decorations), + }; } /** @@ -90,6 +99,16 @@ export function getFootnoteNumber( return footnoteNumberingPluginKey.getState(state)?.numbers.get(id); } +/** + * Read the cached reference-occurrence count for `id` (how many `[^id]` links + * point at this definition). Drives the definition's multi-backlink UI (#168): + * `> 1` renders ↩ a b c …, each scrolling to its own occurrence. Returns 0 when + * the plugin is not installed or the id is unknown (caller treats as single). + */ +export function getFootnoteRefCount(state: EditorState, id: string): number { + return footnoteNumberingPluginKey.getState(state)?.refCounts.get(id) ?? 0; +} + /** * ProseMirror plugin that renders footnote numbers as decorations. It never * mutates the document (safe in read-only / share and in collaboration) — it diff --git a/packages/editor-ext/src/lib/footnote/footnote-reference.ts b/packages/editor-ext/src/lib/footnote/footnote-reference.ts index 7b47617d..751d8664 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-reference.ts +++ b/packages/editor-ext/src/lib/footnote/footnote-reference.ts @@ -1,14 +1,14 @@ -import { mergeAttributes, Node } from "@tiptap/core"; -import { TextSelection, Transaction } from "@tiptap/pm/state"; -import { ReactNodeViewRenderer } from "@tiptap/react"; +import { mergeAttributes, Node } from '@tiptap/core'; +import { TextSelection, Transaction } from '@tiptap/pm/state'; +import { ReactNodeViewRenderer } from '@tiptap/react'; import { FOOTNOTE_DEFINITION_NAME, FOOTNOTE_REFERENCE_NAME, FOOTNOTES_LIST_NAME, generateFootnoteId, -} from "./footnote-util"; -import { footnoteNumberingPlugin } from "./footnote-numbering"; -import { footnoteSyncPlugin, footnotePastePlugin } from "./footnote-sync"; +} from './footnote-util'; +import { footnoteNumberingPlugin } from './footnote-numbering'; +import { footnoteSyncPlugin, footnotePastePlugin } from './footnote-sync'; export interface FootnoteReferenceOptions { HTMLAttributes: Record; @@ -27,7 +27,7 @@ export interface FootnoteReferenceOptions { enableSync?: boolean; } -declare module "@tiptap/core" { +declare module '@tiptap/core' { interface Commands { footnote: { /** @@ -42,8 +42,11 @@ declare module "@tiptap/core" { removeFootnote: (id: string) => ReturnType; /** Scroll to (and focus) a footnote definition by id. */ scrollToFootnote: (id: string) => ReturnType; - /** Scroll to (and select) a footnote reference by id. */ - scrollToReference: (id: string) => ReturnType; + /** Scroll to a footnote reference by id. `index` selects WHICH occurrence + * to scroll to when the id is referenced more than once (reuse, #166): + * 0-based, defaults to the first. Used by the definition's multi-backlink + * UI (#168). */ + scrollToReference: (id: string, index?: number) => ReturnType; }; } } @@ -66,7 +69,7 @@ export const FootnoteReference = Node.create({ // Superscript mark's rule. priority: 101, - group: "inline", + group: 'inline', inline: true, atom: true, selectable: true, @@ -99,10 +102,10 @@ export const FootnoteReference = Node.create({ return { id: { default: null, - parseHTML: (element) => element.getAttribute("data-id"), + parseHTML: (element) => element.getAttribute('data-id'), renderHTML: (attributes) => { if (!attributes.id) return {}; - return { "data-id": attributes.id }; + return { 'data-id': attributes.id }; }, }, }; @@ -113,7 +116,7 @@ export const FootnoteReference = Node.create({ { // High priority so the Superscript mark (which also matches ) does // not claim a footnote reference and drop it as empty content. - tag: "sup[data-footnote-ref]", + tag: 'sup[data-footnote-ref]', priority: 100, }, ]; @@ -121,9 +124,9 @@ export const FootnoteReference = Node.create({ renderHTML({ HTMLAttributes }) { return [ - "sup", + 'sup', mergeAttributes( - { "data-footnote-ref": "", class: "footnote-ref" }, + { 'data-footnote-ref': '', class: 'footnote-ref' }, this.options.HTMLAttributes, HTMLAttributes, ), @@ -132,7 +135,7 @@ export const FootnoteReference = Node.create({ // Plain-text representation (used by generateText / markdown text fallbacks). renderText({ node }) { - return `[^${node.attrs.id ?? ""}]`; + return `[^${node.attrs.id ?? ''}]`; }, addNodeView() { @@ -170,8 +173,10 @@ export const FootnoteReference = Node.create({ // Make sure the parent accepts an inline atom here. const insertPos = selection.from; - if (!$from.parent.type.spec.content?.includes("inline") && - !$from.parent.isTextblock) { + if ( + !$from.parent.type.spec.content?.includes('inline') && + !$from.parent.isTextblock + ) { return false; } @@ -311,19 +316,23 @@ export const FootnoteReference = Node.create({ `[data-footnote-def][data-id="${id}"]`, ) as HTMLElement | null; if (!dom) return false; - dom.scrollIntoView({ behavior: "smooth", block: "center" }); + dom.scrollIntoView({ behavior: 'smooth', block: 'center' }); return true; }, scrollToReference: - (id: string) => + (id: string, index = 0) => ({ editor }) => { if (!id) return false; - const dom = editor.view.dom.querySelector( + // querySelectorAll returns the occurrences in document order, so the + // index maps 1:1 to the definition's a/b/c backlink (#168). Fall back + // to the first match for an out-of-range index. + const matches = editor.view.dom.querySelectorAll( `sup[data-footnote-ref][data-id="${id}"]`, - ) as HTMLElement | null; + ); + const dom = (matches[index] ?? matches[0]) as HTMLElement | undefined; if (!dom) return false; - dom.scrollIntoView({ behavior: "smooth", block: "center" }); + dom.scrollIntoView({ behavior: 'smooth', block: 'center' }); return true; }, }; diff --git a/packages/editor-ext/src/lib/footnote/footnote-util.ts b/packages/editor-ext/src/lib/footnote/footnote-util.ts index 56813288..d27c9685 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-util.ts +++ b/packages/editor-ext/src/lib/footnote/footnote-util.ts @@ -1,12 +1,12 @@ -import { Node as ProseMirrorNode } from "@tiptap/pm/model"; +import { Node as ProseMirrorNode } from '@tiptap/pm/model'; /** * Node type names for the footnote feature. Centralized so every part of the * feature (nodes, plugins, commands) references the same string. */ -export const FOOTNOTE_REFERENCE_NAME = "footnoteReference"; -export const FOOTNOTES_LIST_NAME = "footnotesList"; -export const FOOTNOTE_DEFINITION_NAME = "footnoteDefinition"; +export const FOOTNOTE_REFERENCE_NAME = 'footnoteReference'; +export const FOOTNOTES_LIST_NAME = 'footnotesList'; +export const FOOTNOTE_DEFINITION_NAME = 'footnoteDefinition'; /** * Generate a uuidv7-style id (time-ordered). Implemented locally so editor-ext @@ -15,10 +15,10 @@ export const FOOTNOTE_DEFINITION_NAME = "footnoteDefinition"; */ export function generateFootnoteId(): string { const now = Date.now(); - const timeHex = now.toString(16).padStart(12, "0"); + const timeHex = now.toString(16).padStart(12, '0'); const rand = (length: number) => { - let out = ""; + let out = ''; for (let i = 0; i < length; i++) { out += Math.floor(Math.random() * 16).toString(16); } @@ -26,19 +26,19 @@ export function generateFootnoteId(): string { }; // version 7 nibble, then variant (8..b) nibble. - const versioned = "7" + rand(3); + const versioned = '7' + rand(3); const variantNibble = (8 + Math.floor(Math.random() * 4)).toString(16); const variant = variantNibble + rand(3); return ( timeHex.slice(0, 8) + - "-" + + '-' + timeHex.slice(8, 12) + - "-" + + '-' + versioned + - "-" + + '-' + variant + - "-" + + '-' + rand(12) ); } @@ -89,7 +89,7 @@ export function deriveFootnoteId( * Purely deterministic. */ function suffix(n: number): string { - let out = ""; + let out = ''; let x = n; while (x > 0) { const rem = (x - 1) % 25; @@ -131,3 +131,19 @@ export function computeFootnoteNumbers( } return numbers; } + +/** + * Build a map of `referenceId -> number of reference occurrences` (>= 1) from + * document order. After #166 the same id may be referenced multiple times + * (reuse: one number, one definition, N forward links); this count drives the + * definition's multi-backlink UI (↩ a b c …, #168). Pure function of the doc. + */ +export function computeFootnoteRefCounts( + doc: ProseMirrorNode, +): Map { + const counts = new Map(); + for (const id of collectReferenceIds(doc)) { + counts.set(id, (counts.get(id) ?? 0) + 1); + } + return counts; +} diff --git a/packages/editor-ext/src/lib/footnote/footnote.test.ts b/packages/editor-ext/src/lib/footnote/footnote.test.ts index ff4e1625..11c868f6 100644 --- a/packages/editor-ext/src/lib/footnote/footnote.test.ts +++ b/packages/editor-ext/src/lib/footnote/footnote.test.ts @@ -1,25 +1,26 @@ -import { describe, it, expect } from "vitest"; -import { Editor, Extension, getSchema } from "@tiptap/core"; -import { Document } from "@tiptap/extension-document"; -import { Paragraph } from "@tiptap/extension-paragraph"; -import { Text } from "@tiptap/extension-text"; -import { Superscript } from "@tiptap/extension-superscript"; -import { Plugin, PluginKey } from "@tiptap/pm/state"; -import { Node as PMNode } from "@tiptap/pm/model"; -import { EditorState } from "@tiptap/pm/state"; -import { FootnoteReference } from "./footnote-reference"; -import { FootnotesList } from "./footnotes-list"; -import { FootnoteDefinition } from "./footnote-definition"; -import { TrailingNode } from "../trailing-node"; -import { footnoteSyncPlugin } from "./footnote-sync"; -import { getFootnoteNumber } from "./footnote-numbering"; +import { describe, it, expect } from 'vitest'; +import { Editor, Extension, getSchema } from '@tiptap/core'; +import { Document } from '@tiptap/extension-document'; +import { Paragraph } from '@tiptap/extension-paragraph'; +import { Text } from '@tiptap/extension-text'; +import { Superscript } from '@tiptap/extension-superscript'; +import { Plugin, PluginKey } from '@tiptap/pm/state'; +import { Node as PMNode } from '@tiptap/pm/model'; +import { EditorState } from '@tiptap/pm/state'; +import { FootnoteReference } from './footnote-reference'; +import { FootnotesList } from './footnotes-list'; +import { FootnoteDefinition } from './footnote-definition'; +import { TrailingNode } from '../trailing-node'; +import { footnoteSyncPlugin } from './footnote-sync'; +import { getFootnoteNumber, getFootnoteRefCount } from './footnote-numbering'; import { computeFootnoteNumbers, + computeFootnoteRefCounts, collectReferenceIds, FOOTNOTE_REFERENCE_NAME, FOOTNOTES_LIST_NAME, FOOTNOTE_DEFINITION_NAME, -} from "./footnote-util"; +} from './footnote-util'; const extensions = [ Document, @@ -33,7 +34,7 @@ const extensions = [ function makeEditor(content?: any) { return new Editor({ extensions, - content: content ?? { type: "doc", content: [{ type: "paragraph" }] }, + content: content ?? { type: 'doc', content: [{ type: 'paragraph' }] }, }); } @@ -45,19 +46,19 @@ function countType(doc: PMNode, name: string): number { return n; } -describe("footnote numbering (pure function)", () => { - it("numbers references in document order", () => { +describe('footnote numbering (pure function)', () => { + it('numbers references in document order', () => { const schema = getSchema(extensions); const doc = PMNode.fromJSON(schema, { - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + 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: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'x' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'y' } }, ], }, { @@ -65,32 +66,110 @@ describe("footnote numbering (pure function)", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "x" }, - content: [{ type: "paragraph" }], + attrs: { id: 'x' }, + content: [{ type: 'paragraph' }], }, { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "y" }, - content: [{ type: "paragraph" }], + attrs: { id: 'y' }, + content: [{ type: 'paragraph' }], }, ], }, ], }); - expect(collectReferenceIds(doc)).toEqual(["x", "y"]); + expect(collectReferenceIds(doc)).toEqual(['x', 'y']); const numbers = computeFootnoteNumbers(doc); - expect(numbers.get("x")).toBe(1); - expect(numbers.get("y")).toBe(2); + expect(numbers.get('x')).toBe(1); + expect(numbers.get('y')).toBe(2); + }); + + it('counts reference occurrences per id (reuse), one number per id (#168)', () => { + const schema = getSchema(extensions); + // `a` is referenced 3 times, `b` once. Reuse: one number each, 3 vs 1 links. + const doc = PMNode.fromJSON(schema, { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'a' } }, + { type: 'text', text: ' x ' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'b' } }, + { type: 'text', text: ' y ' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'a' } }, + { type: 'text', text: ' z ' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'a' } }, + ], + }, + { + type: FOOTNOTES_LIST_NAME, + content: [ + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: 'a' }, + content: [{ type: 'paragraph' }], + }, + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: 'b' }, + content: [{ type: 'paragraph' }], + }, + ], + }, + ], + }); + + const numbers = computeFootnoteNumbers(doc); + expect(numbers.get('a')).toBe(1); + expect(numbers.get('b')).toBe(2); + + const counts = computeFootnoteRefCounts(doc); + expect(counts.get('a')).toBe(3); + expect(counts.get('b')).toBe(1); + expect(counts.get('missing')).toBeUndefined(); }); }); -describe("setFootnote command", () => { - it("inserts a reference and a matching definition in the footnotes list", () => { +describe('getFootnoteRefCount (cached, live editor)', () => { + it('returns the live occurrence count and 0 for an unknown id', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ - { type: "paragraph", content: [{ type: "text", text: "Hello" }] }, + { + type: 'paragraph', + content: [ + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'a' } }, + { type: 'text', text: ' and ' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'a' } }, + ], + }, + { + type: FOOTNOTES_LIST_NAME, + content: [ + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: 'a' }, + content: [{ type: 'paragraph' }], + }, + ], + }, + ], + }); + + expect(getFootnoteRefCount(editor.state, 'a')).toBe(2); + expect(getFootnoteRefCount(editor.state, 'nope')).toBe(0); + editor.destroy(); + }); +}); + +describe('setFootnote command', () => { + it('inserts a reference and a matching definition in the footnotes list', () => { + const editor = makeEditor({ + type: 'doc', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: 'Hello' }] }, ], }); // Cursor at end of the word. @@ -115,12 +194,12 @@ describe("setFootnote command", () => { editor.destroy(); }); - it("inserts the definition at the correct position matching reference order", () => { + it('inserts the definition at the correct position matching reference order', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ - { type: "paragraph", content: [{ type: "text", text: "AAAA" }] }, - { type: "paragraph", content: [{ type: "text", text: "BBBB" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'AAAA' }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'BBBB' }] }, ], }); @@ -150,12 +229,12 @@ describe("setFootnote command", () => { }); }); -describe("removeFootnote command (cascade)", () => { - it("removes both the reference and its definition, and drops the empty list", () => { +describe('removeFootnote command (cascade)', () => { + it('removes both the reference and its definition, and drops the empty list', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ - { type: "paragraph", content: [{ type: "text", text: "Hello" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'Hello' }] }, ], }); editor.commands.setTextSelection(6); @@ -178,29 +257,29 @@ describe("removeFootnote command (cascade)", () => { }); }); -describe("footnote sync plugin (orphans)", () => { - it("creates an empty definition for a reference pasted without one", () => { +describe('footnote sync plugin (orphans)', () => { + it('creates an empty definition for a reference pasted without one', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "x" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "orphan-ref" } }, + { type: 'text', text: 'x' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'orphan-ref' } }, ], }, ], }); // Trigger a doc change so appendTransaction runs. - editor.commands.insertContentAt(1, " "); + editor.commands.insertContentAt(1, ' '); const doc = editor.state.doc; let defFound = false; doc.descendants((node) => { if ( node.type.name === FOOTNOTE_DEFINITION_NAME && - node.attrs.id === "orphan-ref" + node.attrs.id === 'orphan-ref' ) { defFound = true; } @@ -209,17 +288,17 @@ describe("footnote sync plugin (orphans)", () => { editor.destroy(); }); - it("merges multiple footnotesList nodes into one, preserving all definitions, as the last child", () => { + it('merges multiple footnotesList nodes into one, preserving all definitions, as the last child', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + 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: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'x' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'y' } }, ], }, // First (stray) footnotes list, e.g. from a paste/collab merge. @@ -228,27 +307,37 @@ describe("footnote sync plugin (orphans)", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "x" }, - content: [{ type: "paragraph", content: [{ type: "text", text: "X note" }] }], + attrs: { id: 'x' }, + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'X note' }], + }, + ], }, ], }, - { type: "paragraph", content: [{ type: "text", text: "tail" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'tail' }] }, // Second footnotes list (the "real" trailing one). { type: FOOTNOTES_LIST_NAME, content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "y" }, - content: [{ type: "paragraph", content: [{ type: "text", text: "Y note" }] }], + attrs: { id: 'y' }, + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'Y note' }], + }, + ], }, ], }, ], }); // Trigger a local doc change so appendTransaction runs. - editor.commands.insertContentAt(1, " "); + editor.commands.insertContentAt(1, ' '); const doc = editor.state.doc; // Converged to exactly ONE list. @@ -256,24 +345,25 @@ describe("footnote sync plugin (orphans)", () => { // Both definitions preserved (no tracking lost). const defIds: string[] = []; doc.descendants((node) => { - if (node.type.name === FOOTNOTE_DEFINITION_NAME) defIds.push(node.attrs.id); + if (node.type.name === FOOTNOTE_DEFINITION_NAME) + defIds.push(node.attrs.id); }); - expect(defIds.sort()).toEqual(["x", "y"]); + expect(defIds.sort()).toEqual(['x', 'y']); // The single list is the LAST child of the document. const lastChild = doc.child(doc.childCount - 1); expect(lastChild.type.name).toBe(FOOTNOTES_LIST_NAME); editor.destroy(); }); - it("leaves a correct doc (single trailing list) unchanged — no merge loop", () => { + it('leaves a correct doc (single trailing list) unchanged — no merge loop', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "a" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "x" } }, + { type: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'x' } }, ], }, { @@ -281,8 +371,13 @@ describe("footnote sync plugin (orphans)", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "x" }, - content: [{ type: "paragraph", content: [{ type: "text", text: "X note" }] }], + attrs: { id: 'x' }, + content: [ + { + type: 'paragraph', + content: [{ type: 'text', text: 'X note' }], + }, + ], }, ], }, @@ -290,7 +385,7 @@ describe("footnote sync plugin (orphans)", () => { }); const before = editor.state.doc.toJSON(); // A change that doesn't touch footnote structure. - editor.commands.insertContentAt(1, "z"); + editor.commands.insertContentAt(1, 'z'); const doc = editor.state.doc; // Still exactly one list, still last, definition preserved. expect(countType(doc, FOOTNOTES_LIST_NAME)).toBe(1); @@ -307,22 +402,22 @@ describe("footnote sync plugin (orphans)", () => { editor.destroy(); }); - it("repeated references REUSE one footnote; a duplicate definition is dropped (first-wins)", () => { + it('repeated references REUSE one footnote; a duplicate definition is dropped (first-wins)', () => { // Reuse semantics (#166): two references with id "d" are the SAME footnote // (one number, shared definition) — they are NEVER re-id'd. Two definitions // sharing id "d" are first-wins: the first keeps "d", the second is re-id'd // to a deterministic orphan id and then dropped by the orphan policy (it has // no matching reference). So the result is ONE reused footnote on "first". const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "a" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "d" } }, - { type: "text", text: "b" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "d" } }, + { type: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'd' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'd' } }, ], }, { @@ -330,16 +425,22 @@ describe("footnote sync plugin (orphans)", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "d" }, + attrs: { id: 'd' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "first" }] }, + { + type: 'paragraph', + content: [{ type: 'text', text: 'first' }], + }, ], }, { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "d" }, + attrs: { id: 'd' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "second" }] }, + { + type: 'paragraph', + content: [{ type: 'text', text: 'second' }], + }, ], }, ], @@ -347,7 +448,7 @@ describe("footnote sync plugin (orphans)", () => { ], }); // The first local keystroke fires the sync plugin's appendTransaction. - editor.commands.insertContentAt(1, " "); + editor.commands.insertContentAt(1, ' '); const doc = editor.state.doc; // One shared definition survives (first-wins); the duplicate is dropped. @@ -360,35 +461,36 @@ describe("footnote sync plugin (orphans)", () => { defTexts.push(node.textContent); } }); - expect(defTexts).toEqual(["first"]); - expect(defIds).toEqual(["d"]); + expect(defTexts).toEqual(['first']); + expect(defIds).toEqual(['d']); // Both references keep id "d" (reuse — not re-id'd). const refIds: string[] = []; doc.descendants((node) => { - if (node.type.name === FOOTNOTE_REFERENCE_NAME) refIds.push(node.attrs.id); + if (node.type.name === FOOTNOTE_REFERENCE_NAME) + refIds.push(node.attrs.id); }); - expect(refIds).toEqual(["d", "d"]); + expect(refIds).toEqual(['d', 'd']); editor.destroy(); }); - it("reuse outcome is DETERMINISTIC across clients (Yjs convergence)", () => { + it('reuse outcome is DETERMINISTIC across clients (Yjs convergence)', () => { // Cross-client determinism guard. Two collaborating clients each see the // SAME document and make a local edit; the sync plugin runs identically, so // the resolved state MUST be identical (else they diverge over Yjs). Under // reuse the three "d" references collapse to one footnote and the duplicate // definitions are dropped (first-wins) — deterministically on every client. const duplicateDoc = { - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "a" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "d" } }, - { type: "text", text: "b" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "d" } }, - { type: "text", text: "c" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "d" } }, + { type: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'd' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'd' } }, + { type: 'text', text: 'c' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'd' } }, ], }, { @@ -396,25 +498,25 @@ describe("footnote sync plugin (orphans)", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "d" }, + attrs: { id: 'd' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "one" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'one' }] }, ], }, { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "d" }, + attrs: { id: 'd' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "two" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'two' }] }, ], }, { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "d" }, + attrs: { id: 'd' }, content: [ { - type: "paragraph", - content: [{ type: "text", text: "three" }], + type: 'paragraph', + content: [{ type: 'text', text: 'three' }], }, ], }, @@ -427,7 +529,7 @@ describe("footnote sync plugin (orphans)", () => { // A fresh editor instance = an independent "client" running the same // plugin pipeline on the same starting document. const editor = makeEditor(structuredClone(duplicateDoc)); - editor.commands.insertContentAt(1, " "); // local keystroke -> sync runs + editor.commands.insertContentAt(1, ' '); // local keystroke -> sync runs const refIds: string[] = []; const defIds: string[] = []; const defTexts: string[] = []; @@ -449,29 +551,29 @@ describe("footnote sync plugin (orphans)", () => { // Both clients resolved to IDENTICAL state (the Yjs-convergence property). expect(clientA).toEqual(clientB); // Reuse: the three references stay "d"; one definition survives (first-wins). - expect(clientA.refIds).toEqual(["d", "d", "d"]); - expect(clientA.defIds).toEqual(["d"]); - expect(clientA.defTexts).toEqual(["one"]); + expect(clientA.refIds).toEqual(['d', 'd', 'd']); + expect(clientA.defIds).toEqual(['d']); + expect(clientA.defTexts).toEqual(['one']); }); - it("removes an orphan definition with no matching reference", () => { + it('removes an orphan definition with no matching reference', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ - { type: "paragraph", content: [{ type: "text", text: "x" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'x' }] }, { type: FOOTNOTES_LIST_NAME, content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "orphan-def" }, - content: [{ type: "paragraph" }], + attrs: { id: 'orphan-def' }, + content: [{ type: 'paragraph' }], }, ], }, ], }); - editor.commands.insertContentAt(1, "y"); + editor.commands.insertContentAt(1, 'y'); const doc = editor.state.doc; expect(countType(doc, FOOTNOTE_DEFINITION_NAME)).toBe(0); @@ -493,7 +595,7 @@ describe("footnote sync plugin (orphans)", () => { * transaction counter additionally fails fast with a bounded iteration cap, so * a regression surfaces as an explicit error instead of only a slow timeout. */ -describe("footnote sync plugin (no infinite loop — live editor)", () => { +describe('footnote sync plugin (no infinite loop — live editor)', () => { // Hard cap on how many doc-changing appendTransaction rounds we tolerate for a // single user action. Convergence takes a couple of rounds at most; anything // approaching this means the plugins are oscillating. @@ -508,13 +610,13 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { // throws if they exceed the cap, converting a would-be infinite loop into a // deterministic failure instead of a wall-clock hang. const LoopGuard = Extension.create({ - name: "footnoteLoopGuard", + name: 'footnoteLoopGuard', // Run last so it observes every other plugin's appended transaction. priority: -1000, addProseMirrorPlugins() { return [ new Plugin({ - key: new PluginKey("footnoteLoopGuard"), + key: new PluginKey('footnoteLoopGuard'), appendTransaction(transactions) { if (transactions.some((t) => t.docChanged)) { rounds += 1; @@ -543,7 +645,7 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { FootnotesList, FootnoteDefinition, ], - content: content ?? { type: "doc", content: [{ type: "paragraph" }] }, + content: content ?? { type: 'doc', content: [{ type: 'paragraph' }] }, }); return { editor, getRounds: () => rounds, resetRounds: () => (rounds = 0) }; } @@ -558,17 +660,17 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { if (listIndex === -1) return false; for (let i = listIndex + 1; i < doc.childCount; i++) { const child = doc.child(i); - if (!(child.type.name === "paragraph" && child.content.size === 0)) { + if (!(child.type.name === 'paragraph' && child.content.size === 0)) { return false; } } return true; } - it("setFootnote() RETURNS (no hang) and produces one ref + one def in a trailing list", () => { + it('setFootnote() RETURNS (no hang) and produces one ref + one def in a trailing list', () => { const { editor } = makeLiveEditor({ - type: "doc", - content: [{ type: "paragraph", content: [{ type: "text", text: "Hi" }] }], + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Hi' }] }], }); editor.commands.setTextSelection(3); const ok = editor.commands.setFootnote(); @@ -582,10 +684,10 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { editor.destroy(); }); - it("a second setFootnote() does not hang: two refs + two defs in one list", () => { + it('a second setFootnote() does not hang: two refs + two defs in one list', () => { const { editor } = makeLiveEditor({ - type: "doc", - content: [{ type: "paragraph", content: [{ type: "text", text: "Hi" }] }], + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Hi' }] }], }); editor.commands.setTextSelection(3); editor.commands.setFootnote(); @@ -600,10 +702,10 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { editor.destroy(); }); - it("converges and stabilizes: an unrelated edit does not keep producing transactions", () => { + it('converges and stabilizes: an unrelated edit does not keep producing transactions', () => { const { editor, getRounds, resetRounds } = makeLiveEditor({ - type: "doc", - content: [{ type: "paragraph", content: [{ type: "text", text: "Hi" }] }], + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Hi' }] }], }); editor.commands.setTextSelection(3); editor.commands.setFootnote(); @@ -612,14 +714,14 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { // assert the sync plugin converges in a bounded number of rounds and the // document is stable (one ref/def/list, list trailing). resetRounds(); - editor.commands.insertContentAt(1, "Z"); + editor.commands.insertContentAt(1, 'Z'); const afterFirst = editor.state.doc.toJSON(); const roundsAfterEdit = getRounds(); expect(roundsAfterEdit).toBeLessThan(MAX_ROUNDS); // A follow-up no-op-ish edit must not re-trigger structural rewrites: the // footnotes section is identical before and after a further unrelated edit. - editor.commands.insertContentAt(2, "Y"); + editor.commands.insertContentAt(2, 'Y'); const afterSecond = editor.state.doc.toJSON(); const listOf = (json: any) => @@ -629,17 +731,17 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { editor.destroy(); }); - it("two footnotesList nodes converge to one (merge) without looping", () => { + it('two footnotesList nodes converge to one (merge) without looping', () => { const { editor } = makeLiveEditor({ - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + 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: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'x' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'y' } }, ], }, { @@ -647,22 +749,22 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "x" }, + attrs: { id: 'x' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "X" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'X' }] }, ], }, ], }, - { type: "paragraph", content: [{ type: "text", text: "tail" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'tail' }] }, { type: FOOTNOTES_LIST_NAME, content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "y" }, + attrs: { id: 'y' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "Y" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'Y' }] }, ], }, ], @@ -670,7 +772,7 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { ], }); // Trigger a local doc change so appendTransaction runs (must not hang). - editor.commands.insertContentAt(1, " "); + editor.commands.insertContentAt(1, ' '); const doc = editor.state.doc; expect(countType(doc, FOOTNOTES_LIST_NAME)).toBe(1); @@ -679,7 +781,7 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { if (node.type.name === FOOTNOTE_DEFINITION_NAME) defIds.push(node.attrs.id); }); - expect(defIds.sort()).toEqual(["x", "y"]); + expect(defIds.sort()).toEqual(['x', 'y']); expect(lastFootnotesListIsTrailing(doc)).toBe(true); editor.destroy(); }); @@ -697,7 +799,7 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { * existing definition NODE INSTANCES are preserved (identity-equal) after the * sync pass, AND the derived numbers follow the new reference order. */ -describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () => { +describe('footnote sync plugin (no rebuild on reorder — data-loss guard)', () => { function reorderedDoc() { // The "out of order" end-state of a reorder: references occur as [b, a] but // the bottom list still physically holds definitions in [a, b] order. This @@ -706,15 +808,15 @@ describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () // the definition subtrees). The sync plugin must leave the definitions // ALONE here — no delete/recreate of any definition subtree. return { - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "p" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "b" } }, - { type: "text", text: "q" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "a" } }, + { type: 'text', text: 'p' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'b' } }, + { type: 'text', text: 'q' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'a' } }, ], }, { @@ -722,16 +824,16 @@ describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "a" }, + attrs: { id: 'a' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "A" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'A' }] }, ], }, { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "b" }, + attrs: { id: 'b' }, content: [ - { type: "paragraph", content: [{ type: "text", text: "B" }] }, + { type: 'paragraph', content: [{ type: 'text', text: 'B' }] }, ], }, ], @@ -743,32 +845,33 @@ describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () function getDefNodesById(doc: PMNode): Map { const m = new Map(); doc.descendants((node) => { - if (node.type.name === FOOTNOTE_DEFINITION_NAME) m.set(node.attrs.id, node); + if (node.type.name === FOOTNOTE_DEFINITION_NAME) + m.set(node.attrs.id, node); }); return m; } - it("does NOT delete/recreate existing definition subtrees for an out-of-order list (numbers still correct)", () => { + it('does NOT delete/recreate existing definition subtrees for an out-of-order list (numbers still correct)', () => { const editor = makeEditor(reorderedDoc()); // Capture the exact definition NODE INSTANCES before any sync pass. const before = getDefNodesById(editor.state.doc); // Sanity: both carry their content right now. - expect(before.get("a")!.textContent).toBe("A"); - expect(before.get("b")!.textContent).toBe("B"); + expect(before.get('a')!.textContent).toBe('A'); + expect(before.get('b')!.textContent).toBe('B'); // Trigger a local edit elsewhere in the body so the sync plugin runs. - editor.commands.insertContentAt(1, "z"); + editor.commands.insertContentAt(1, 'z'); const doc = editor.state.doc; // Reference order is [b, a]; the displayed numbers follow reference order // (decoration-only numbering): b -> 1, a -> 2 — regardless of physical list // order. - expect(collectReferenceIds(doc)).toEqual(["b", "a"]); + expect(collectReferenceIds(doc)).toEqual(['b', 'a']); const numbers = computeFootnoteNumbers(doc); - expect(numbers.get("b")).toBe(1); - expect(numbers.get("a")).toBe(2); + expect(numbers.get('b')).toBe(1); + expect(numbers.get('a')).toBe(2); // CRITICAL regression guard: both definitions still exist and are the SAME // node instances as before the edit — the plugin did NOT delete/recreate the @@ -776,11 +879,11 @@ describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () // concurrent-edit data-loss window). Identity equality proves the subtree // was preserved verbatim. const after = getDefNodesById(doc); - expect(after.get("a")).toBe(before.get("a")); - expect(after.get("b")).toBe(before.get("b")); + expect(after.get('a')).toBe(before.get('a')); + expect(after.get('b')).toBe(before.get('b')); // Content intact, exactly one list, both definitions present. - expect(after.get("a")!.textContent).toBe("A"); - expect(after.get("b")!.textContent).toBe("B"); + expect(after.get('a')!.textContent).toBe('A'); + expect(after.get('b')!.textContent).toBe('B'); expect(countType(doc, FOOTNOTES_LIST_NAME)).toBe(1); expect(countType(doc, FOOTNOTE_DEFINITION_NAME)).toBe(2); @@ -792,19 +895,19 @@ describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () * Sync-plugin guard paths that are awkward to exercise through a live editor: * the remote-transaction skip and the enableSync:false (read-only) mode. */ -describe("footnote sync plugin (guards)", () => { +describe('footnote sync plugin (guards)', () => { // Build a non-canonical document (an orphan reference with no definition) so a // sync pass would normally append a transaction. function nonCanonicalState() { const schema = getSchema(extensions); const doc = PMNode.fromJSON(schema, { - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "x" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "orphan" } }, + { type: 'text', text: 'x' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'orphan' } }, ], }, ], @@ -812,7 +915,7 @@ describe("footnote sync plugin (guards)", () => { return EditorState.create({ schema, doc }); } - it("isRemoteTransaction => true: appendTransaction returns null (no rebuild on remote txns)", () => { + it('isRemoteTransaction => true: appendTransaction returns null (no rebuild on remote txns)', () => { // The sync plugin must SKIP remote/collab transactions so orphan cleanup and // structural rewrites only ever run on local edits. const plugin = footnoteSyncPlugin(() => true); @@ -820,30 +923,26 @@ describe("footnote sync plugin (guards)", () => { // Produce a doc-changing transaction (insert a space) and feed it to the // plugin's appendTransaction exactly as ProseMirror would. - const tr = state.tr.insertText(" ", 1); + const tr = state.tr.insertText(' ', 1); const newState = state.apply(tr); - const result = plugin.spec.appendTransaction!( - [tr], - state, - newState, - ); + const result = plugin.spec.appendTransaction!([tr], state, newState); expect(result).toBeNull(); }); - it("isRemoteTransaction => false: appendTransaction DOES rebuild (sanity)", () => { + it('isRemoteTransaction => false: appendTransaction DOES rebuild (sanity)', () => { // Control: with a local (non-remote) transaction the same non-canonical doc // triggers a sync transaction, proving the null above is the remote guard // and not a no-op everywhere. const plugin = footnoteSyncPlugin(() => false); const state = nonCanonicalState(); - const tr = state.tr.insertText(" ", 1); + const tr = state.tr.insertText(' ', 1); const newState = state.apply(tr); const result = plugin.spec.appendTransaction!([tr], state, newState); expect(result).not.toBeNull(); expect(result!.docChanged).toBe(true); }); - it("enableSync:false: the plugin never mutates the doc (read-only viewer)", () => { + it('enableSync:false: the plugin never mutates the doc (read-only viewer)', () => { // Build an editor with sync disabled. An orphan reference (no definition) // must NOT trigger a definition insertion — the document is left untouched. const editor = new Editor({ @@ -856,27 +955,27 @@ describe("footnote sync plugin (guards)", () => { FootnoteDefinition, ], content: { - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + type: 'paragraph', content: [ - { type: "text", text: "x" }, - { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "orphan" } }, + { type: 'text', text: 'x' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'orphan' } }, ], }, ], }, }); // A local edit that would normally trigger orphan-definition synthesis. - editor.commands.insertContentAt(1, "y"); + editor.commands.insertContentAt(1, 'y'); const doc = editor.state.doc; // No definition (and no list) was ever created — sync is disabled. expect(countType(doc, FOOTNOTE_DEFINITION_NAME)).toBe(0); expect(countType(doc, FOOTNOTES_LIST_NAME)).toBe(0); // Numbering decorations still work: the reference is numbered 1. - expect(getFootnoteNumber(editor.state, "orphan")).toBe(1); + expect(getFootnoteNumber(editor.state, 'orphan')).toBe(1); editor.destroy(); }); }); @@ -887,18 +986,18 @@ describe("footnote sync plugin (guards)", () => { * recomputing the whole map per render. We assert the cache exists, is correct, * and stays current across edits. */ -describe("footnote numbering cache", () => { - it("exposes correct numbers via getFootnoteNumber and updates on edits", () => { +describe('footnote numbering cache', () => { + it('exposes correct numbers via getFootnoteNumber and updates on edits', () => { const editor = makeEditor({ - type: "doc", + type: 'doc', content: [ { - type: "paragraph", + 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: 'text', text: 'a' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'x' } }, + { type: 'text', text: 'b' }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: 'y' } }, ], }, { @@ -906,13 +1005,13 @@ describe("footnote numbering cache", () => { content: [ { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "x" }, - content: [{ type: "paragraph" }], + attrs: { id: 'x' }, + content: [{ type: 'paragraph' }], }, { type: FOOTNOTE_DEFINITION_NAME, - attrs: { id: "y" }, - content: [{ type: "paragraph" }], + attrs: { id: 'y' }, + content: [{ type: 'paragraph' }], }, ], }, @@ -920,22 +1019,22 @@ describe("footnote numbering cache", () => { }); // The cache mirrors computeFootnoteNumbers — but is read in O(1) per id. - expect(getFootnoteNumber(editor.state, "x")).toBe(1); - expect(getFootnoteNumber(editor.state, "y")).toBe(2); + expect(getFootnoteNumber(editor.state, 'x')).toBe(1); + expect(getFootnoteNumber(editor.state, 'y')).toBe(2); // The cached map is the SAME values a fresh full computation would yield. const fresh = computeFootnoteNumbers(editor.state.doc); - expect(getFootnoteNumber(editor.state, "x")).toBe(fresh.get("x")); - expect(getFootnoteNumber(editor.state, "y")).toBe(fresh.get("y")); + expect(getFootnoteNumber(editor.state, 'x')).toBe(fresh.get('x')); + expect(getFootnoteNumber(editor.state, 'y')).toBe(fresh.get('y')); // After inserting a new earlier reference, the cache updates so the numbers // shift (decoration-only numbering follows reference order). editor.commands.insertContentAt(1, { type: FOOTNOTE_REFERENCE_NAME, - attrs: { id: "z" }, + attrs: { id: 'z' }, }); - expect(getFootnoteNumber(editor.state, "z")).toBe(1); - expect(getFootnoteNumber(editor.state, "x")).toBe(2); - expect(getFootnoteNumber(editor.state, "y")).toBe(3); + expect(getFootnoteNumber(editor.state, 'z')).toBe(1); + expect(getFootnoteNumber(editor.state, 'x')).toBe(2); + expect(getFootnoteNumber(editor.state, 'y')).toBe(3); editor.destroy(); }); }); -- 2.49.1 From fdaf20ca7bab2097ce2cad13a063383c119b4dad Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:33:57 +0300 Subject: [PATCH 06/16] fix(mcp): refuse ambiguous patch_node/delete_node on duplicated ids (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docmost duplicates block ids on copy/paste, and copyPageContent writes the source document verbatim with the same ids. `patchNode`/`deleteNode` address a block by `attrs.id` via replaceNodeById/deleteNodeById, which act on EVERY node sharing the id — so a single patch_node/delete_node could silently replace/remove multiple unrelated blocks with no signal to the model (red-team finding #6). Guard both write paths: when more than one node matches the id, skip the write entirely (the transform returns null -> no mutation) and throw a clear "ambiguous id — N nodes share it" error so the model re-targets with a more specific anchor. Only an unambiguous single match is written; the 0-match and 1-match behavior is unchanged. The duplicate-count basis is covered by node-ops.test.mjs (replaceNodeById / deleteNodeById report count===2 for a 2-duplicate doc). The end-to-end guard is not unit-tested because patchNode/deleteNode require a live collab provider and the test suite has no provider mock. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/build/client.js | 25 +++++++++++++++++++++---- packages/mcp/src/client.ts | 25 +++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/mcp/build/client.js b/packages/mcp/build/client.js index 46380a0c..e1d2d82e 100644 --- a/packages/mcp/build/client.js +++ b/packages/mcp/build/client.js @@ -1292,13 +1292,22 @@ export class DocmostClient { replaced = 0; const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target); replaced = r; - if (replaced === 0) - return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost + // duplicates block ids on copy/paste (and copyPageContent writes them + // verbatim), so replacing "the node with id X" would silently clobber + // EVERY duplicate (#159). Refuse: skip the write and throw below so the + // model re-targets with a more specific anchor instead of corrupting the + // page. Only an unambiguous single match is written. + if (replaced !== 1) + return null; return nd; }); if (replaced === 0) { throw new Error(`patch_node: no node with id "${nodeId}" found on page ${pageId}`); } + if (replaced > 1) { + throw new Error(`patch_node: id "${nodeId}" is ambiguous — ${replaced} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to replace all of them; nothing was changed. Re-target with a more specific anchor.`); + } return { success: true, replaced, nodeId, verify: mutation.verify }; } /** @@ -1381,13 +1390,21 @@ export class DocmostClient { deleted = 0; const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId); deleted = d; - if (deleted === 0) - return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS (block + // ids are duplicated on copy/paste, #159): deleting "the node with id X" + // would silently remove EVERY duplicate. Refuse: skip the write and throw + // below so the model re-targets. Only an unambiguous single match is + // deleted. + if (deleted !== 1) + return null; return nd; }); if (deleted === 0) { throw new Error(`delete_node: no node with id "${nodeId}" found on page ${pageId}`); } + if (deleted > 1) { + throw new Error(`delete_node: id "${nodeId}" is ambiguous — ${deleted} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to delete all of them; nothing was changed. Re-target with a more specific anchor.`); + } return { success: true, deleted, nodeId, verify: mutation.verify }; } /** Build the public share URL for a page. */ diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 6293d5ee..c6419563 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -1625,7 +1625,13 @@ export class DocmostClient { replaced = 0; const { doc: nd, replaced: r } = replaceNodeById(liveDoc, nodeId, target); replaced = r; - if (replaced === 0) return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS: Docmost + // duplicates block ids on copy/paste (and copyPageContent writes them + // verbatim), so replacing "the node with id X" would silently clobber + // EVERY duplicate (#159). Refuse: skip the write and throw below so the + // model re-targets with a more specific anchor instead of corrupting the + // page. Only an unambiguous single match is written. + if (replaced !== 1) return null; return nd; }, ); @@ -1635,6 +1641,11 @@ export class DocmostClient { `patch_node: no node with id "${nodeId}" found on page ${pageId}`, ); } + if (replaced > 1) { + throw new Error( + `patch_node: id "${nodeId}" is ambiguous — ${replaced} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to replace all of them; nothing was changed. Re-target with a more specific anchor.`, + ); + } return { success: true, replaced, nodeId, verify: mutation.verify }; } @@ -1755,7 +1766,12 @@ export class DocmostClient { deleted = 0; const { doc: nd, deleted: d } = deleteNodeById(liveDoc, nodeId); deleted = d; - if (deleted === 0) return null; // no match -> skip the write entirely + // 0 matches -> skip the write. >1 matches -> the id is AMBIGUOUS (block + // ids are duplicated on copy/paste, #159): deleting "the node with id X" + // would silently remove EVERY duplicate. Refuse: skip the write and throw + // below so the model re-targets. Only an unambiguous single match is + // deleted. + if (deleted !== 1) return null; return nd; }, ); @@ -1765,6 +1781,11 @@ export class DocmostClient { `delete_node: no node with id "${nodeId}" found on page ${pageId}`, ); } + if (deleted > 1) { + throw new Error( + `delete_node: id "${nodeId}" is ambiguous — ${deleted} nodes on page ${pageId} share it (block ids are duplicated on copy/paste). Refusing to delete all of them; nothing was changed. Re-target with a more specific anchor.`, + ); + } return { success: true, deleted, nodeId, verify: mutation.verify }; } -- 2.49.1 From e536c6f9a93611b7188e62ce7017fb7820dc10cd Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:35:06 +0300 Subject: [PATCH 07/16] ci(test): run the server integration suite against real Postgres/Redis (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only test command in CI was `pnpm -r test` (unit `.spec.ts` on mocks). `test:int` (`.int-spec.ts`, real Postgres/Redis) ran nowhere in CI — there were no DB `services:` — so the cost-cap, FK-cascade, jsonb round-trip and real AI-apply integration tests never gated a PR, and regressions in those high-severity paths stayed green (red-team finding #7). Add `services: postgres (pgvector) + redis` and a `pnpm --filter server test:int` step. The pgvector image is required because migrations create vector columns and global-setup runs `CREATE EXTENSION vector`. Service credentials/db match the defaults in apps/server/test/integration (docmost / docmost_dev_pw, maintenance db `docmost`, redis 6379), so no TEST_*_URL overrides are needed; global-setup drops/recreates the isolated docmost_test DB and migrates it. NOTE: the workflow change itself can only be validated by an actual CI run (YAML parses locally); the int-spec suite is verified passing locally on this branch. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/test.yml | 41 +++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 955b0ac2..f2330749 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -15,6 +15,38 @@ permissions: jobs: test: runs-on: ubuntu-latest + # Real Postgres + Redis so the server integration suite (`*.int-spec.ts`, + # behind `pnpm --filter server test:int`) runs in CI (red-team finding #7). + # Without it, cost-cap / FK-cascade / jsonb-round-trip / real-apply tests + # only ran locally, so regressions in those paths stayed green in CI. + # Postgres uses the pgvector image because migrations create vector columns + # and global-setup runs `CREATE EXTENSION vector`. Credentials/db match the + # defaults in apps/server/test/integration/db.ts + global-setup.ts + # (docmost / docmost_dev_pw, maintenance db `docmost`, redis on 6379), so no + # TEST_*_URL overrides are needed. + services: + postgres: + image: pgvector/pgvector:pg16 + env: + POSTGRES_USER: docmost + POSTGRES_PASSWORD: docmost_dev_pw + POSTGRES_DB: docmost + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U docmost" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + redis: + image: redis:7 + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 steps: - name: Checkout uses: actions/checkout@v4 @@ -36,5 +68,12 @@ jobs: - name: Build editor-ext run: pnpm --filter @docmost/editor-ext build - - name: Run tests + - name: Run unit tests run: pnpm -r test + + # Integration suite against the real Postgres/Redis services above. Runs + # the FK-cascade, cost-cap, jsonb-round-trip and real-apply specs that the + # unit run (mocks only) cannot cover. global-setup drops/recreates the + # isolated `docmost_test` DB and migrates it to latest. + - name: Run server integration tests + run: pnpm --filter server test:int -- 2.49.1 From 77ccc596eaf77bee188bac7deb15d557d70595dc Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 04:52:05 +0300 Subject: [PATCH 08/16] feat(ai-chat): per-MCP-server instructions in the agent system prompt (#180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Admins can now give each EXTERNAL MCP server a free-text instruction ("how/ when to use this server's tools") that the agent receives in its SYSTEM PROMPT next to the tool descriptions — porting the built-in SERVER_INSTRUCTIONS idea to admin-configured servers. Trusted, admin-authored text (like a system prompt); NON-secret, so unlike headersEnc it IS returned in views/forms. - Migration: nullable `instructions text` on ai_mcp_servers (old rows = null = no guidance). Table type + repo insert/update (blank/whitespace -> null via blankToNull). DTO `@MaxLength(4000)`. Service threads it through McpServerView/toView. - mcp-clients: `McpServerInstruction { serverName, toolPrefix, instructions }` threaded through the toolset/cache/lease. Guidance is built ONLY for a server that actually connected AND contributed >=1 callable tool (the allowlist may filter all of them out) AND has non-blank text — so a guide never appears for tools the agent cannot call. Cached with the toolset, so an edit is picked up next turn via the existing CRUD cache invalidation. - System prompt: `buildMcpToolingBlock` renders an block INSIDE the safety sandwich (after context, before the trailing SAFETY_FRAMEWORK) so it informs tool choice but cannot override the rules; each section is headed by the server's `prefix_*` namespace. Empty/blank -> block omitted. The caller (ai-chat.service) now builds the external toolset BEFORE the prompt and passes external.instructions; client-handle lifecycle (close-once) unchanged. - Client: instructions field in types + a Textarea (autosize, maxLength 4000) in the MCP-server form with a namespace-prefix hint; i18n (en/ru). Tests across every layer (prompt block placement + both SAFETY copies; view blank->null; buildEntry includes guidance only for connected+>=1-tool+non-blank; DTO MaxLength; repo + integration round-trip; service wiring). Delegated impl reviewed (APPROVE); applied the import-type follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../public/locales/en-US/translation.json | 1 + .../public/locales/ru-RU/translation.json | 2 + .../components/ai-mcp-server-form.tsx | 29 +- .../services/ai-mcp-server-service.ts | 7 + .../src/core/ai-chat/ai-chat.prompt.spec.ts | 117 +++++- .../server/src/core/ai-chat/ai-chat.prompt.ts | 57 ++- .../src/core/ai-chat/ai-chat.service.spec.ts | 129 ++++++- .../src/core/ai-chat/ai-chat.service.ts | 345 +++++++++--------- .../external-mcp/dto/create-mcp-server.dto.ts | 9 + .../dto/mcp-server-instructions.dto.spec.ts | 75 ++++ .../external-mcp/dto/update-mcp-server.dto.ts | 7 + .../external-mcp/mcp-clients.service.ts | 91 ++++- .../external-mcp/mcp-instructions.spec.ts | 168 +++++++++ .../external-mcp/mcp-servers-to-view.spec.ts | 21 +- .../external-mcp/mcp-servers.service.ts | 8 + ...0625T120000-ai-mcp-servers-instructions.ts | 19 + .../repos/ai-chat/ai-mcp-server.repo.spec.ts | 30 +- .../repos/ai-chat/ai-mcp-server.repo.ts | 21 ++ .../database/types/ai-mcp-servers.types.ts | 5 + .../ai-mcp-server-repo.int-spec.ts | 81 ++++ 20 files changed, 1011 insertions(+), 211 deletions(-) create mode 100644 apps/server/src/core/ai-chat/external-mcp/dto/mcp-server-instructions.dto.spec.ts create mode 100644 apps/server/src/core/ai-chat/external-mcp/mcp-instructions.spec.ts create mode 100644 apps/server/src/database/migrations/20260625T120000-ai-mcp-servers-instructions.ts diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 95fbfc0c..cdad5023 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -710,6 +710,7 @@ "Authorization header": "Authorization header", "Tool allowlist": "Tool allowlist", "Optional. Leave empty to allow all tools the server exposes.": "Optional. Leave empty to allow all tools the server exposes.", + "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"_*\".": "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"_*\".", "Test": "Test", "Available tools": "Available tools", "No tools available": "No tools available", diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 336e8688..e3d46ad3 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -751,6 +751,8 @@ "Manage API keys for all users in the workspace. View the API documentation for usage details.": "Управляйте API-ключами для всех пользователей в рабочем пространстве. Смотрите документацию по API для получения информации об использовании.", "View the API documentation for usage details.": "Смотрите документацию по API для получения информации об использовании.", "View the MCP documentation.": "Смотрите документацию по MCP.", + "Instructions": "Инструкции", + "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"_*\".": "Необязательное указание агенту, как и когда использовать инструменты этого сервера. Добавляется в системный промпт. Инструменты сервера именуются с префиксом «<имя сервера>_*».", "Sources": "Источники", "AI Answers not available for attachments": "Ответы ИИ недоступны для вложений", "No answer available": "Ответ недоступен", diff --git a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx index a3d07a94..f3beb39b 100644 --- a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx +++ b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx @@ -11,6 +11,7 @@ import { Switch, TagsInput, Text, + Textarea, TextInput, } from "@mantine/core"; import { useForm } from "@mantine/form"; @@ -35,6 +36,8 @@ const formSchema = z.object({ // Write-only secret buffer. Empty string means "do not change" (unless cleared). authHeader: z.string(), toolAllowlist: z.array(z.string()), + // Admin-authored prompt guidance (#180). Capped to mirror the DTO MaxLength. + instructions: z.string().max(4000), enabled: z.boolean(), }); @@ -63,6 +66,7 @@ function buildInitialValues(server?: IAiMcpServer): FormValues { toolAllowlist: Array.isArray(server?.toolAllowlist) ? server.toolAllowlist : [], + instructions: server?.instructions ?? "", enabled: server?.enabled ?? true, }; } @@ -124,6 +128,8 @@ export default function AiMcpServerForm({ transport: values.transport, url: values.url, toolAllowlist: values.toolAllowlist, + // Always sent: a blank value clears the stored guidance (server -> null). + instructions: values.instructions, enabled: values.enabled, }; // Only attach headers when set or explicitly cleared (omit => unchanged). @@ -135,6 +141,8 @@ export default function AiMcpServerForm({ transport: values.transport, url: values.url, toolAllowlist: values.toolAllowlist, + // Blank => server stores null (no guidance). + instructions: values.instructions, enabled: values.enabled, }; // On create, only a typed value matters (no prior stored headers). @@ -158,10 +166,7 @@ export default function AiMcpServerForm({ return ( - +