Compare commits
7 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 76e0c08cec | |||
| d78b985062 | |||
| a4fc6c7f64 | |||
| c252068672 | |||
| 68caf8157a | |||
| cb9c5dda59 | |||
| e431b33bb1 |
@@ -202,6 +202,13 @@ MCP_DOCMOST_PASSWORD=
|
||||
# Default 900000 (15 min).
|
||||
# AI_MCP_CALL_TIMEOUT_MS=900000
|
||||
|
||||
# Deferred tool loading for the in-app AI chat (#332). Default ON: the agent sees
|
||||
# a compact <tool_catalog> and only CORE tools + a loadTools meta-tool are active
|
||||
# each step; deferred tools (the fat/rare ones + all external MCP tools) load on
|
||||
# demand. Set AI_CHAT_DEFERRED_TOOLS=false to restore the old "all tools always
|
||||
# active" behavior.
|
||||
# AI_CHAT_DEFERRED_TOOLS=true
|
||||
|
||||
# --- Anonymous public-share AI assistant ---
|
||||
# Opt-in per workspace (AI settings -> "public share assistant"; off by default).
|
||||
# When enabled, anonymous visitors of a published share can ask an AI about that
|
||||
|
||||
+16
-2
@@ -5,6 +5,13 @@ RUN npm install -g pnpm@10.4.0
|
||||
|
||||
FROM base AS builder
|
||||
|
||||
# re2 (packages/mcp) always compiles from source under pnpm (the prebuilt-binary
|
||||
# download cannot identify the GitHub repo), so node-gyp needs python3/make/g++.
|
||||
# This stage is discarded, so the toolchain can stay installed.
|
||||
RUN apt-get update \
|
||||
&& apt-get install -y --no-install-recommends python3 make g++ \
|
||||
&& rm -rf /var/lib/apt/lists/*
|
||||
|
||||
WORKDIR /app
|
||||
|
||||
COPY . .
|
||||
@@ -57,9 +64,16 @@ COPY --from=builder /app/patches /app/patches
|
||||
|
||||
RUN chown -R node:node /app
|
||||
|
||||
USER node
|
||||
# Toolchain is needed transiently to compile re2 during the prod install; install
|
||||
# and purge it in one layer to keep the final image slim. The install itself runs
|
||||
# as the node user via su to keep node_modules ownership without a costly chown layer.
|
||||
RUN apt-get update \
|
||||
&& apt-get install -y --no-install-recommends python3 make g++ \
|
||||
&& su node -c "pnpm install --frozen-lockfile --prod" \
|
||||
&& apt-get purge -y --auto-remove python3 make g++ \
|
||||
&& rm -rf /var/lib/apt/lists/*
|
||||
|
||||
RUN pnpm install --frozen-lockfile --prod
|
||||
USER node
|
||||
|
||||
RUN mkdir -p /app/data/storage
|
||||
|
||||
|
||||
@@ -0,0 +1,250 @@
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import { MantineProvider } from "@mantine/core";
|
||||
import { MemoryRouter } from "react-router-dom";
|
||||
|
||||
// matchMedia (read by MantineProvider) is stubbed globally in vitest.setup.ts.
|
||||
|
||||
// The fallback path renders the full TipTap editor; stub it so we can assert the
|
||||
// safety valve fired without pulling in the editor stack.
|
||||
vi.mock("@/features/comment/components/comment-editor", () => ({
|
||||
default: () => <div data-testid="comment-editor-fallback" />,
|
||||
}));
|
||||
|
||||
// Mention rendering hits react-query; stub the page/share queries so the mention
|
||||
// case renders in isolation.
|
||||
vi.mock("@/features/page/queries/page-query.ts", () => ({
|
||||
usePageQuery: () => ({ data: undefined, isLoading: false, isError: false }),
|
||||
}));
|
||||
vi.mock("@/features/share/queries/share-query.ts", () => ({
|
||||
useSharePageQuery: () => ({ data: undefined }),
|
||||
}));
|
||||
|
||||
import { CommentContentView } from "./comment-content-view";
|
||||
|
||||
function renderView(content: string | object) {
|
||||
return render(
|
||||
<MantineProvider>
|
||||
<MemoryRouter>
|
||||
<CommentContentView content={content} />
|
||||
</MemoryRouter>
|
||||
</MantineProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
const doc = (content: any[]) => JSON.stringify({ type: "doc", content });
|
||||
const para = (content: any[]) => ({ type: "paragraph", content });
|
||||
const text = (t: string, marks?: any[]) => ({ type: "text", text: t, marks });
|
||||
|
||||
describe("CommentContentView", () => {
|
||||
it("renders paragraphs as <p> with text", () => {
|
||||
const { container } = renderView(doc([para([text("Hello world")])]));
|
||||
expect(screen.getByText("Hello world")).toBeDefined();
|
||||
expect(container.querySelector("p")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("reproduces the read-only CommentEditor DOM nesting for CSS parity", () => {
|
||||
const { container } = renderView(doc([para([text("x")])]));
|
||||
// outer .commentEditor > .ProseMirror (module) > .ProseMirror (global) > p
|
||||
const globalPm = container.querySelector("div.ProseMirror > p");
|
||||
expect(globalPm).not.toBeNull();
|
||||
});
|
||||
|
||||
it("renders the bold mark as <strong>", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("bold", [{ type: "bold" }])])]),
|
||||
);
|
||||
const el = container.querySelector("strong");
|
||||
expect(el?.textContent).toBe("bold");
|
||||
});
|
||||
|
||||
it("renders the italic mark as <em>", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("it", [{ type: "italic" }])])]),
|
||||
);
|
||||
expect(container.querySelector("em")?.textContent).toBe("it");
|
||||
});
|
||||
|
||||
it("renders the strike mark as <s>", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("st", [{ type: "strike" }])])]),
|
||||
);
|
||||
expect(container.querySelector("s")?.textContent).toBe("st");
|
||||
});
|
||||
|
||||
it("renders the underline mark as <u> (not the editor fallback)", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("un", [{ type: "underline" }])])]),
|
||||
);
|
||||
expect(container.querySelector("u")?.textContent).toBe("un");
|
||||
// Underline is a supported mark, so no degrade to the editor fallback.
|
||||
expect(screen.queryByTestId("comment-editor-fallback")).toBeNull();
|
||||
});
|
||||
|
||||
it("renders the code mark as <code>", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("co", [{ type: "code" }])])]),
|
||||
);
|
||||
expect(container.querySelector("code")?.textContent).toBe("co");
|
||||
});
|
||||
|
||||
it("renders the link mark as an anchor with safe rel/target", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
text("click", [
|
||||
{ type: "link", attrs: { href: "https://example.com" } },
|
||||
]),
|
||||
]),
|
||||
]),
|
||||
);
|
||||
const a = container.querySelector("a");
|
||||
expect(a?.getAttribute("href")).toBe("https://example.com");
|
||||
expect(a?.getAttribute("target")).toBe("_blank");
|
||||
expect(a?.getAttribute("rel")).toBe("noopener noreferrer nofollow");
|
||||
expect(a?.textContent).toBe("click");
|
||||
});
|
||||
|
||||
it("neutralizes a javascript: link href (stored XSS) while keeping the text", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
text("click", [
|
||||
{ type: "link", attrs: { href: "javascript:alert(1)" } },
|
||||
]),
|
||||
]),
|
||||
]),
|
||||
);
|
||||
const a = container.querySelector("a");
|
||||
expect(a).not.toBeNull();
|
||||
// No navigable javascript: href — attribute is absent (or empty).
|
||||
expect(a?.getAttribute("href")).toBeFalsy();
|
||||
// The link text is still rendered.
|
||||
expect(a?.textContent).toBe("click");
|
||||
});
|
||||
|
||||
it("neutralizes a control-char-obfuscated javascript: href", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
text("x", [
|
||||
{ type: "link", attrs: { href: "java\tscript:alert(1)" } },
|
||||
]),
|
||||
]),
|
||||
]),
|
||||
);
|
||||
expect(container.querySelector("a")?.getAttribute("href")).toBeFalsy();
|
||||
});
|
||||
|
||||
it("neutralizes a data: link href", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
text("x", [
|
||||
{
|
||||
type: "link",
|
||||
attrs: { href: "data:text/html,<script>alert(1)</script>" },
|
||||
},
|
||||
]),
|
||||
]),
|
||||
]),
|
||||
);
|
||||
expect(container.querySelector("a")?.getAttribute("href")).toBeFalsy();
|
||||
});
|
||||
|
||||
it("preserves a mailto: link href (allowlisted scheme)", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
text("mail", [
|
||||
{ type: "link", attrs: { href: "mailto:a@b.com" } },
|
||||
]),
|
||||
]),
|
||||
]),
|
||||
);
|
||||
expect(container.querySelector("a")?.getAttribute("href")).toBe(
|
||||
"mailto:a@b.com",
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves a relative link href (no scheme, not a script vector)", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
text("rel", [{ type: "link", attrs: { href: "/some/path" } }]),
|
||||
]),
|
||||
]),
|
||||
);
|
||||
expect(container.querySelector("a")?.getAttribute("href")).toBe(
|
||||
"/some/path",
|
||||
);
|
||||
});
|
||||
|
||||
it("nests multiple marks on one text node", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("x", [{ type: "bold" }, { type: "italic" }])])]),
|
||||
);
|
||||
// bold wraps italic (or vice versa) — both elements exist around the text.
|
||||
expect(container.querySelector("strong")).not.toBeNull();
|
||||
expect(container.querySelector("em")).not.toBeNull();
|
||||
expect(screen.getByText("x")).toBeDefined();
|
||||
});
|
||||
|
||||
it("renders hardBreak as <br/>", () => {
|
||||
const { container } = renderView(
|
||||
doc([para([text("a"), { type: "hardBreak" }, text("b")])]),
|
||||
);
|
||||
expect(container.querySelector("br")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("renders a user mention as a styled span", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
{
|
||||
type: "mention",
|
||||
attrs: { label: "Alice", entityType: "user", entityId: "u1" },
|
||||
},
|
||||
]),
|
||||
]),
|
||||
);
|
||||
expect(screen.getByText("@Alice")).toBeDefined();
|
||||
// No fallback to the editor.
|
||||
expect(screen.queryByTestId("comment-editor-fallback")).toBeNull();
|
||||
});
|
||||
|
||||
it("renders a page mention as a link", () => {
|
||||
const { container } = renderView(
|
||||
doc([
|
||||
para([
|
||||
{
|
||||
type: "mention",
|
||||
attrs: {
|
||||
label: "Some Page",
|
||||
entityType: "page",
|
||||
slugId: "pg1",
|
||||
},
|
||||
},
|
||||
]),
|
||||
]),
|
||||
);
|
||||
expect(container.querySelector("a")).not.toBeNull();
|
||||
expect(screen.getByText("Some Page")).toBeDefined();
|
||||
});
|
||||
|
||||
it("renders a legacy plain-text (non-JSON) string as plain text", () => {
|
||||
renderView("just a legacy string");
|
||||
expect(screen.getByText("just a legacy string")).toBeDefined();
|
||||
expect(screen.queryByTestId("comment-editor-fallback")).toBeNull();
|
||||
});
|
||||
|
||||
it("falls back to CommentEditor for an unknown node type", () => {
|
||||
renderView(doc([{ type: "codeBlock", content: [text("x")] }]));
|
||||
expect(screen.getByTestId("comment-editor-fallback")).toBeDefined();
|
||||
});
|
||||
|
||||
it("falls back to CommentEditor for malformed JSON", () => {
|
||||
renderView('{"type":"doc","content":[');
|
||||
expect(screen.getByTestId("comment-editor-fallback")).toBeDefined();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,199 @@
|
||||
import React from "react";
|
||||
import classes from "./comment.module.css";
|
||||
import { MentionContent } from "@/features/editor/components/mention/mention-view";
|
||||
import CommentEditor from "@/features/comment/components/comment-editor";
|
||||
|
||||
// Static, editor-free renderer of a comment body (ProseMirror JSON). It walks the
|
||||
// document and emits plain DOM, avoiding the cost of a full TipTap/ProseMirror
|
||||
// instance per comment (the panel used to spin up 400+ editors on mount).
|
||||
//
|
||||
// The supported node/mark set MUST mirror what CommentEditor enables
|
||||
// (StarterKit + Mention + LinkExtension). Anything outside that set makes the
|
||||
// whole comment degrade to the read-only CommentEditor via the fallback below,
|
||||
// so we never show a half-rendered comment.
|
||||
|
||||
// Sentinel thrown when we hit a node/mark we don't know how to render statically.
|
||||
// Caught at the top level to trigger the CommentEditor fallback for the whole comment.
|
||||
class UnknownNodeError extends Error {}
|
||||
|
||||
// Protocol allowlist mirroring @tiptap/extension-link's default (the read-only
|
||||
// CommentEditor path relies on it to blank javascript:/data: hrefs). The static
|
||||
// renderer must apply the SAME sanitization because the backend stores comment
|
||||
// content verbatim and React does not neutralize javascript: in an href.
|
||||
const ALLOWED_URI_SCHEMES = /^(?:https?|ftps?|mailto|tel|callto|sms|cid|xmpp):/i;
|
||||
|
||||
function safeHref(href: unknown): string | undefined {
|
||||
if (typeof href !== "string") return undefined;
|
||||
// Strip control chars/whitespace that could smuggle a scheme past the test
|
||||
// (e.g. "java\tscript:").
|
||||
const cleaned = href.replace(/[\u0000-\u0020]/g, "").trim();
|
||||
// Allow relative/anchor/protocol-relative links (no scheme) — not script vectors.
|
||||
if (!/^[a-z][a-z0-9+.-]*:/i.test(cleaned)) return href;
|
||||
return ALLOWED_URI_SCHEMES.test(cleaned) ? href : undefined;
|
||||
}
|
||||
|
||||
interface PMMark {
|
||||
type: string;
|
||||
attrs?: Record<string, any>;
|
||||
}
|
||||
|
||||
interface PMNode {
|
||||
type: string;
|
||||
attrs?: Record<string, any>;
|
||||
content?: PMNode[];
|
||||
text?: string;
|
||||
marks?: PMMark[];
|
||||
}
|
||||
|
||||
// Wrap a text node's string in its marks (marks nest, e.g. bold + italic).
|
||||
function renderMarks(
|
||||
text: React.ReactNode,
|
||||
marks: PMMark[] | undefined,
|
||||
keyPrefix: string,
|
||||
): React.ReactNode {
|
||||
if (!marks || marks.length === 0) return text;
|
||||
|
||||
return marks.reduce<React.ReactNode>((acc, mark, i) => {
|
||||
const key = `${keyPrefix}-m${i}`;
|
||||
switch (mark.type) {
|
||||
case "bold":
|
||||
return <strong key={key}>{acc}</strong>;
|
||||
case "italic":
|
||||
return <em key={key}>{acc}</em>;
|
||||
case "strike":
|
||||
return <s key={key}>{acc}</s>;
|
||||
case "underline":
|
||||
// StarterKit enables the Underline extension by default (Mod-u) and
|
||||
// CommentEditor does not disable it, so real comments can carry this
|
||||
// mark. Render it here rather than degrading the whole comment.
|
||||
return <u key={key}>{acc}</u>;
|
||||
case "code":
|
||||
return <code key={key}>{acc}</code>;
|
||||
case "link": {
|
||||
// LinkExtension (TiptapLink) opens links in a new tab; keep the same
|
||||
// safe rel semantics the editor produces. Sanitize the href against the
|
||||
// extension's protocol allowlist — a disallowed scheme (javascript:,
|
||||
// data:) yields undefined so the anchor is non-navigable but still shows
|
||||
// its text, matching how extension-link blanks a bad href.
|
||||
const href = safeHref(mark.attrs?.href);
|
||||
return (
|
||||
<a
|
||||
key={key}
|
||||
href={href}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer nofollow"
|
||||
>
|
||||
{acc}
|
||||
</a>
|
||||
);
|
||||
}
|
||||
default:
|
||||
throw new UnknownNodeError(`Unknown mark type: ${mark.type}`);
|
||||
}
|
||||
}, text);
|
||||
}
|
||||
|
||||
function renderNode(node: PMNode, key: string): React.ReactNode {
|
||||
switch (node.type) {
|
||||
case "paragraph":
|
||||
return <p key={key}>{renderChildren(node.content, key)}</p>;
|
||||
case "text":
|
||||
return (
|
||||
<React.Fragment key={key}>
|
||||
{renderMarks(node.text ?? "", node.marks, key)}
|
||||
</React.Fragment>
|
||||
);
|
||||
case "hardBreak":
|
||||
return <br key={key} />;
|
||||
case "mention":
|
||||
return (
|
||||
<span key={key} style={{ display: "inline" }}>
|
||||
<MentionContent attrs={node.attrs as any} />
|
||||
</span>
|
||||
);
|
||||
default:
|
||||
throw new UnknownNodeError(`Unknown node type: ${node.type}`);
|
||||
}
|
||||
}
|
||||
|
||||
function renderChildren(
|
||||
content: PMNode[] | undefined,
|
||||
keyPrefix: string,
|
||||
): React.ReactNode {
|
||||
if (!content) return null;
|
||||
return content.map((child, i) => renderNode(child, `${keyPrefix}-${i}`));
|
||||
}
|
||||
|
||||
// Reproduce the exact DOM nesting the read-only CommentEditor renders so the
|
||||
// scoped CSS in comment.module.css (which targets
|
||||
// `.commentEditor .ProseMirror :global(.ProseMirror)` and `.ProseMirror p`)
|
||||
// applies pixel-for-pixel. Read-only => no data-editable / data-surface attrs.
|
||||
function Shell({ children }: { children: React.ReactNode }) {
|
||||
return (
|
||||
<div className={classes.commentEditor}>
|
||||
<div className={classes.ProseMirror}>
|
||||
<div className="ProseMirror">{children}</div>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
interface CommentContentViewProps {
|
||||
content: string | object;
|
||||
}
|
||||
|
||||
export function CommentContentView({ content }: CommentContentViewProps) {
|
||||
// Degrade this single comment to the old editor-based render (safety valve).
|
||||
const fallback = () => {
|
||||
if (import.meta.env.DEV) {
|
||||
console.warn(
|
||||
"CommentContentView: unsupported comment content, falling back to editor",
|
||||
);
|
||||
}
|
||||
return <CommentEditor defaultContent={content} editable={false} />;
|
||||
};
|
||||
|
||||
let doc: unknown = content;
|
||||
|
||||
if (typeof content === "string") {
|
||||
try {
|
||||
doc = JSON.parse(content);
|
||||
} catch {
|
||||
const trimmed = content.trim();
|
||||
// Looks like it was meant to be JSON but is malformed -> safety-valve fallback.
|
||||
if (trimmed.startsWith("{") || trimmed.startsWith("[")) {
|
||||
return fallback();
|
||||
}
|
||||
// Otherwise it's a legacy plain-text comment: render as a single paragraph.
|
||||
return (
|
||||
<Shell>
|
||||
<p>{content}</p>
|
||||
</Shell>
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Double-stringified / legacy plain-text stored as a JSON string.
|
||||
if (typeof doc === "string") {
|
||||
return (
|
||||
<Shell>
|
||||
<p>{doc}</p>
|
||||
</Shell>
|
||||
);
|
||||
}
|
||||
|
||||
try {
|
||||
const pmDoc = doc as PMNode;
|
||||
if (!pmDoc || typeof pmDoc !== "object" || pmDoc.type !== "doc") {
|
||||
throw new UnknownNodeError("Not a ProseMirror doc");
|
||||
}
|
||||
return <Shell>{renderChildren(pmDoc.content, "n")}</Shell>;
|
||||
} catch (err) {
|
||||
if (err instanceof UnknownNodeError) {
|
||||
return fallback();
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
export default CommentContentView;
|
||||
@@ -1,5 +1,5 @@
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render, screen, fireEvent } from "@testing-library/react";
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
|
||||
import { MantineProvider } from "@mantine/core";
|
||||
import { IComment } from "@/features/comment/types/comment.types";
|
||||
|
||||
@@ -9,10 +9,11 @@ import { IComment } from "@/features/comment/types/comment.types";
|
||||
// component renders in isolation. We only assert the AI-badge rendering branch.
|
||||
const applyMutateAsync = vi.fn();
|
||||
const dismissMutateAsync = vi.fn();
|
||||
const updateMutateAsync = vi.fn();
|
||||
vi.mock("@/features/comment/queries/comment-query", () => ({
|
||||
useDeleteCommentMutation: () => ({ mutateAsync: vi.fn() }),
|
||||
useResolveCommentMutation: () => ({ mutateAsync: vi.fn() }),
|
||||
useUpdateCommentMutation: () => ({ mutateAsync: vi.fn() }),
|
||||
useUpdateCommentMutation: () => ({ mutateAsync: updateMutateAsync }),
|
||||
useApplySuggestionMutation: () => ({
|
||||
mutateAsync: applyMutateAsync,
|
||||
isPending: false,
|
||||
@@ -23,9 +24,51 @@ vi.mock("@/features/comment/queries/comment-query", () => ({
|
||||
}),
|
||||
}));
|
||||
|
||||
// The document the mocked editor emits via onUpdate when the edit form is open.
|
||||
// Duplicated inside the mock factory (below) to keep the factory self-contained.
|
||||
const EDITED_DOC = {
|
||||
type: "doc",
|
||||
content: [
|
||||
{ type: "paragraph", content: [{ type: "text", text: "edited via editor" }] },
|
||||
],
|
||||
};
|
||||
|
||||
// CommentEditor pulls in the full TipTap editor stack; replace it with a stub.
|
||||
vi.mock("@/features/comment/components/comment-editor", () => ({
|
||||
default: () => <div data-testid="comment-editor" />,
|
||||
// In edit mode the stub exposes buttons that fire the real onUpdate/onSave props
|
||||
// so the edit->save/cancel flow can be driven without a live editor.
|
||||
vi.mock("@/features/comment/components/comment-editor", () => {
|
||||
const doc = {
|
||||
type: "doc",
|
||||
content: [
|
||||
{ type: "paragraph", content: [{ type: "text", text: "edited via editor" }] },
|
||||
],
|
||||
};
|
||||
return {
|
||||
default: ({ onUpdate, onSave }: any) => (
|
||||
<div data-testid="comment-editor">
|
||||
<button
|
||||
type="button"
|
||||
data-testid="editor-emit-update"
|
||||
onClick={() => onUpdate?.(doc)}
|
||||
/>
|
||||
<button
|
||||
type="button"
|
||||
data-testid="editor-emit-save"
|
||||
onClick={() => onSave?.()}
|
||||
/>
|
||||
</div>
|
||||
),
|
||||
};
|
||||
});
|
||||
|
||||
// CommentContentView (used for the read-only body) imports the mention view,
|
||||
// which pulls page-query -> main.tsx (createRoot). Stub the queries so the item
|
||||
// renders in isolation without the app entry side-effect.
|
||||
vi.mock("@/features/page/queries/page-query.ts", () => ({
|
||||
usePageQuery: () => ({ data: undefined, isLoading: false, isError: false }),
|
||||
}));
|
||||
vi.mock("@/features/share/queries/share-query.ts", () => ({
|
||||
useSharePageQuery: () => ({ data: undefined }),
|
||||
}));
|
||||
|
||||
import CommentListItem from "./comment-list-item";
|
||||
@@ -286,3 +329,132 @@ describe("canShowDismiss predicate", () => {
|
||||
expect(canShowDismiss(c({ parentCommentId: "p" }), true, true)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("CommentListItem — edit -> save/cancel flow (#340 F3)", () => {
|
||||
const body = (t: string) =>
|
||||
JSON.stringify({
|
||||
type: "doc",
|
||||
content: [{ type: "paragraph", content: [{ type: "text", text: t }] }],
|
||||
});
|
||||
|
||||
// The edit menu item is gated on the viewer owning the comment
|
||||
// (currentUser.id === creatorId). currentUserAtom is atomWithStorage-backed,
|
||||
// so seed localStorage to make the viewer the owner (creatorId "user-1").
|
||||
beforeEach(() => {
|
||||
updateMutateAsync.mockClear();
|
||||
localStorage.setItem(
|
||||
"currentUser",
|
||||
JSON.stringify({ user: { id: "user-1", name: "Owner" } }),
|
||||
);
|
||||
});
|
||||
afterEach(() => {
|
||||
localStorage.clear();
|
||||
});
|
||||
|
||||
async function openEditor() {
|
||||
// Open the comment menu, then click "Edit comment" to toggle into edit mode.
|
||||
fireEvent.click(screen.getByLabelText("Comment menu"));
|
||||
fireEvent.click(await screen.findByText("Edit comment"));
|
||||
// Edit form (mocked editor + actions) is now mounted.
|
||||
await screen.findByTestId("comment-editor");
|
||||
}
|
||||
|
||||
it("saves the edited content and, on cache update, shows the new body", async () => {
|
||||
const { rerender } = renderItem(
|
||||
baseComment({ content: body("original body") }),
|
||||
);
|
||||
// Static body first.
|
||||
expect(screen.getByText("original body")).toBeDefined();
|
||||
|
||||
await openEditor();
|
||||
|
||||
// Editor emits an update (populates editContentRef), then Save is clicked.
|
||||
fireEvent.click(screen.getByTestId("editor-emit-update"));
|
||||
fireEvent.click(screen.getByRole("button", { name: "Save" }));
|
||||
|
||||
// mutateAsync is called with the stringified edited doc.
|
||||
expect(updateMutateAsync).toHaveBeenCalledWith({
|
||||
commentId: "c-1",
|
||||
content: JSON.stringify(EDITED_DOC),
|
||||
});
|
||||
|
||||
// On success the form closes (isEditing -> false); the static body renders
|
||||
// from the comment.content prop again.
|
||||
await waitFor(() =>
|
||||
expect(screen.queryByTestId("comment-editor")).toBeNull(),
|
||||
);
|
||||
|
||||
// Simulate the cache invalidation swapping in a new comment object with the
|
||||
// updated content — the static body reflects it.
|
||||
rerender(
|
||||
<MantineProvider>
|
||||
<CommentListItem
|
||||
comment={baseComment({ content: body("updated body after save") })}
|
||||
pageId="page-1"
|
||||
canComment={true}
|
||||
canEdit={true}
|
||||
/>
|
||||
</MantineProvider>,
|
||||
);
|
||||
expect(screen.getByText("updated body after save")).toBeDefined();
|
||||
expect(screen.queryByText("original body")).toBeNull();
|
||||
});
|
||||
|
||||
it("cancel restores the static body and does not call the update mutation", async () => {
|
||||
renderItem(baseComment({ content: body("original body") }));
|
||||
await openEditor();
|
||||
|
||||
// Type something (editContentRef set), then cancel.
|
||||
fireEvent.click(screen.getByTestId("editor-emit-update"));
|
||||
fireEvent.click(screen.getByRole("button", { name: "Cancel" }));
|
||||
|
||||
// Editor unmounts, static body restored, no save happened.
|
||||
await waitFor(() =>
|
||||
expect(screen.queryByTestId("comment-editor")).toBeNull(),
|
||||
);
|
||||
expect(screen.getByText("original body")).toBeDefined();
|
||||
expect(updateMutateAsync).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("saving without editing sends the existing content (editContentRef cleared after cancel)", async () => {
|
||||
renderItem(baseComment({ content: body("original body") }));
|
||||
|
||||
// Cancel path clears editContentRef...
|
||||
await openEditor();
|
||||
fireEvent.click(screen.getByTestId("editor-emit-update"));
|
||||
fireEvent.click(screen.getByRole("button", { name: "Cancel" }));
|
||||
await waitFor(() =>
|
||||
expect(screen.queryByTestId("comment-editor")).toBeNull(),
|
||||
);
|
||||
|
||||
// ...so re-opening and saving WITHOUT an update falls back to comment.content.
|
||||
await openEditor();
|
||||
fireEvent.click(screen.getByRole("button", { name: "Save" }));
|
||||
expect(updateMutateAsync).toHaveBeenCalledWith({
|
||||
commentId: "c-1",
|
||||
content: JSON.stringify(body("original body")),
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("CommentListItem — read-only body renders statically", () => {
|
||||
it("renders the comment body as static text without a TipTap editor", () => {
|
||||
renderItem(
|
||||
baseComment({
|
||||
content: JSON.stringify({
|
||||
type: "doc",
|
||||
content: [
|
||||
{
|
||||
type: "paragraph",
|
||||
content: [{ type: "text", text: "Hello static world" }],
|
||||
},
|
||||
],
|
||||
}),
|
||||
}),
|
||||
);
|
||||
// Body text is present...
|
||||
expect(screen.getByText("Hello static world")).toBeDefined();
|
||||
// ...and it did NOT go through the (mocked) CommentEditor instance.
|
||||
expect(screen.queryByTestId("comment-editor")).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,10 +1,11 @@
|
||||
import { Group, Text, Box, Badge, Button } from "@mantine/core";
|
||||
import { AgentAvatarStack } from "@/components/ui/agent-avatar-stack.tsx";
|
||||
import React, { useEffect, useMemo, useRef, useState } from "react";
|
||||
import React, { useMemo, useRef, useState } from "react";
|
||||
import classes from "./comment.module.css";
|
||||
import { useAtom, useAtomValue } from "jotai";
|
||||
import { useTimeAgo } from "@/hooks/use-time-ago";
|
||||
import CommentEditor from "@/features/comment/components/comment-editor";
|
||||
import CommentContentView from "@/features/comment/components/comment-content-view";
|
||||
import { pageEditorAtom } from "@/features/editor/atoms/editor-atoms";
|
||||
import CommentActions from "@/features/comment/components/comment-actions";
|
||||
import CommentMenu from "@/features/comment/components/comment-menu";
|
||||
@@ -50,7 +51,6 @@ function CommentListItem({
|
||||
const [isEditing, setIsEditing] = useState(false);
|
||||
const [isLoading, setIsLoading] = useState(false);
|
||||
const editor = useAtomValue(pageEditorAtom);
|
||||
const [content, setContent] = useState<string>(comment.content);
|
||||
const editContentRef = useRef<any>(null);
|
||||
const updateCommentMutation = useUpdateCommentMutation();
|
||||
const deleteCommentMutation = useDeleteCommentMutation(comment.pageId);
|
||||
@@ -78,22 +78,16 @@ function CommentListItem({
|
||||
const isOwnerOrAdmin =
|
||||
currentUser?.user?.id === comment.creatorId || userSpaceRole === "admin";
|
||||
|
||||
useEffect(() => {
|
||||
setContent(comment.content);
|
||||
}, [comment]);
|
||||
|
||||
async function handleUpdateComment() {
|
||||
try {
|
||||
setIsLoading(true);
|
||||
const commentToUpdate = {
|
||||
commentId: comment.id,
|
||||
content: JSON.stringify(editContentRef.current ?? content),
|
||||
content: JSON.stringify(editContentRef.current ?? comment.content),
|
||||
};
|
||||
await updateCommentMutation.mutateAsync(commentToUpdate);
|
||||
if (editContentRef.current) {
|
||||
setContent(editContentRef.current);
|
||||
editContentRef.current = null;
|
||||
}
|
||||
editContentRef.current = null;
|
||||
setIsEditing(false);
|
||||
} catch (error) {
|
||||
console.error("Failed to update comment:", error);
|
||||
@@ -350,11 +344,11 @@ function CommentListItem({
|
||||
)}
|
||||
|
||||
{!isEditing ? (
|
||||
<CommentEditor defaultContent={content} editable={false} />
|
||||
<CommentContentView content={comment.content} />
|
||||
) : (
|
||||
<>
|
||||
<CommentEditor
|
||||
defaultContent={content}
|
||||
defaultContent={comment.content}
|
||||
editable={true}
|
||||
onUpdate={(newContent: any) => { editContentRef.current = newContent; }}
|
||||
onSave={handleUpdateComment}
|
||||
@@ -374,4 +368,6 @@ function CommentListItem({
|
||||
);
|
||||
}
|
||||
|
||||
export default CommentListItem;
|
||||
// Memoized so a resolve/apply/reply cache update (which only replaces the touched
|
||||
// comment's object identity) re-renders that one thread, not all ~356 items.
|
||||
export default React.memo(CommentListItem);
|
||||
|
||||
@@ -0,0 +1,108 @@
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { render, screen, fireEvent } from "@testing-library/react";
|
||||
import { MantineProvider } from "@mantine/core";
|
||||
import { IComment } from "@/features/comment/types/comment.types";
|
||||
|
||||
// matchMedia (read by MantineProvider) is stubbed globally in vitest.setup.ts.
|
||||
|
||||
// CommentEditor pulls in the full TipTap editor stack; replace it with a stub so
|
||||
// the lazy reply editor's mount transition can be observed without the editor.
|
||||
vi.mock("@/features/comment/components/comment-editor", () => ({
|
||||
default: () => <div data-testid="comment-editor" />,
|
||||
}));
|
||||
|
||||
// page-query -> main.tsx (createRoot) is a module side effect; stub the queries
|
||||
// pulled in transitively so importing the module is side-effect free.
|
||||
vi.mock("@/features/page/queries/page-query.ts", () => ({
|
||||
usePageQuery: () => ({ data: undefined, isLoading: false, isError: false }),
|
||||
}));
|
||||
vi.mock("@/features/share/queries/share-query.ts", () => ({
|
||||
useSharePageQuery: () => ({ data: undefined }),
|
||||
}));
|
||||
// space-query -> main.tsx (createRoot) is another module side effect; stub it.
|
||||
vi.mock("@/features/space/queries/space-query.ts", () => ({
|
||||
useGetSpaceBySlugQuery: () => ({ data: undefined }),
|
||||
}));
|
||||
|
||||
import {
|
||||
buildChildrenByParent,
|
||||
CommentEditorWithActions,
|
||||
} from "./comment-list-with-tabs";
|
||||
|
||||
const c = (id: string, parentCommentId: string | null = null): IComment =>
|
||||
({ id, parentCommentId }) as IComment;
|
||||
|
||||
describe("buildChildrenByParent (childrenByParent grouping)", () => {
|
||||
it("returns an empty map for undefined or empty input", () => {
|
||||
expect(buildChildrenByParent(undefined).size).toBe(0);
|
||||
expect(buildChildrenByParent([]).size).toBe(0);
|
||||
});
|
||||
|
||||
it("does not index a top-level comment (parentCommentId null)", () => {
|
||||
const map = buildChildrenByParent([c("p1", null)]);
|
||||
expect(map.size).toBe(0);
|
||||
expect(map.has("p1")).toBe(false);
|
||||
});
|
||||
|
||||
it("groups replies under the correct parent, including reply-to-reply nesting", () => {
|
||||
const p1 = c("p1", null);
|
||||
const r1 = c("r1", "p1");
|
||||
const r2 = c("r2", "r1"); // a reply to a reply
|
||||
const map = buildChildrenByParent([p1, r1, r2]);
|
||||
expect(map.get("p1")).toEqual([r1]);
|
||||
expect(map.get("r1")).toEqual([r2]);
|
||||
// The top-level comment itself is never a key.
|
||||
expect(map.has("p1") && map.get("p1")?.length).toBe(1);
|
||||
});
|
||||
|
||||
it("still groups a reply whose parent is not present in items", () => {
|
||||
const orphan = c("o1", "missing-parent");
|
||||
const map = buildChildrenByParent([orphan]);
|
||||
expect(map.get("missing-parent")).toEqual([orphan]);
|
||||
});
|
||||
|
||||
it("preserves insertion order among sibling replies", () => {
|
||||
const map = buildChildrenByParent([
|
||||
c("a", "p1"),
|
||||
c("b", "p1"),
|
||||
c("d", "p1"),
|
||||
]);
|
||||
expect(map.get("p1")?.map((x) => x.id)).toEqual(["a", "b", "d"]);
|
||||
});
|
||||
});
|
||||
|
||||
function renderReplyEditor() {
|
||||
return render(
|
||||
<MantineProvider>
|
||||
<CommentEditorWithActions commentId="c-1" onSave={vi.fn()} />
|
||||
</MantineProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
describe("CommentEditorWithActions — lazy reply editor activation", () => {
|
||||
it("shows only the stub initially (no editor instance mounted)", () => {
|
||||
renderReplyEditor();
|
||||
expect(screen.getByRole("button")).toBeDefined();
|
||||
expect(screen.queryByTestId("comment-editor")).toBeNull();
|
||||
});
|
||||
|
||||
it("mounts the real editor when the stub is clicked and keeps it mounted", () => {
|
||||
renderReplyEditor();
|
||||
fireEvent.click(screen.getByRole("button"));
|
||||
expect(screen.getByTestId("comment-editor")).toBeDefined();
|
||||
// The stub button is replaced by the editor subtree.
|
||||
expect(screen.queryByRole("button")).toBeNull();
|
||||
});
|
||||
|
||||
it("mounts the editor when the stub receives focus", () => {
|
||||
renderReplyEditor();
|
||||
fireEvent.focus(screen.getByRole("button"));
|
||||
expect(screen.getByTestId("comment-editor")).toBeDefined();
|
||||
});
|
||||
|
||||
it("mounts the editor on Enter keydown of the stub", () => {
|
||||
renderReplyEditor();
|
||||
fireEvent.keyDown(screen.getByRole("button"), { key: "Enter" });
|
||||
expect(screen.getByTestId("comment-editor")).toBeDefined();
|
||||
});
|
||||
});
|
||||
@@ -23,7 +23,6 @@ import CommentActions from "@/features/comment/components/comment-actions";
|
||||
import { useFocusWithin } from "@mantine/hooks";
|
||||
import { IComment } from "@/features/comment/types/comment.types.ts";
|
||||
import { usePageQuery } from "@/features/page/queries/page-query.ts";
|
||||
import { IPagination } from "@/lib/types.ts";
|
||||
import { extractPageSlugId } from "@/lib";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { useGetSpaceBySlugQuery } from "@/features/space/queries/space-query.ts";
|
||||
@@ -36,6 +35,24 @@ interface CommentListWithTabsProps {
|
||||
onClose?: () => void;
|
||||
}
|
||||
|
||||
// Index replies by their parent id once (O(n)), instead of an O(n^2) filter per
|
||||
// thread. Replies whose parent is not in `items` are still grouped under their
|
||||
// parentCommentId (they simply won't be reached by the top-level walk).
|
||||
// Exported for unit testing.
|
||||
export function buildChildrenByParent(
|
||||
items: IComment[] | undefined,
|
||||
): Map<string, IComment[]> {
|
||||
const m = new Map<string, IComment[]>();
|
||||
for (const c of items ?? []) {
|
||||
if (c.parentCommentId) {
|
||||
const arr = m.get(c.parentCommentId);
|
||||
if (arr) arr.push(c);
|
||||
else m.set(c.parentCommentId, [c]);
|
||||
}
|
||||
}
|
||||
return m;
|
||||
}
|
||||
|
||||
function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
const { t } = useTranslation();
|
||||
const { pageSlug } = useParams();
|
||||
@@ -46,7 +63,9 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
isError,
|
||||
} = useCommentsQuery({ pageId: page?.id });
|
||||
const createCommentMutation = useCreateCommentMutation();
|
||||
const [isLoading, setIsLoading] = useState(false);
|
||||
// mutateAsync is a stable reference across renders; depend on it (not the
|
||||
// mutation object) so the reply/comment callbacks stay stable.
|
||||
const createCommentAsync = createCommentMutation.mutateAsync;
|
||||
const { data: space } = useGetSpaceBySlugQuery(page?.space?.slug);
|
||||
|
||||
const canEdit = page?.permissions?.canEdit ?? false;
|
||||
@@ -75,13 +94,21 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
return { activeComments: active, resolvedComments: resolved };
|
||||
}, [comments]);
|
||||
|
||||
// Index replies by their parent once, instead of an O(n^2) filter per thread.
|
||||
// The map ref changes on any comments update, so MemoizedChildComments re-runs
|
||||
// (cheap) and re-looks-up, while memoized CommentListItems skip unchanged items.
|
||||
const childrenByParent = useMemo(
|
||||
() => buildChildrenByParent(comments?.items),
|
||||
[comments?.items],
|
||||
);
|
||||
|
||||
const [isPageCommentLoading, setIsPageCommentLoading] = useState(false);
|
||||
|
||||
const handleAddPageComment = useCallback(
|
||||
async (_commentId: string, content: string) => {
|
||||
try {
|
||||
setIsPageCommentLoading(true);
|
||||
const createdComment = await createCommentMutation.mutateAsync({
|
||||
const createdComment = await createCommentAsync({
|
||||
pageId: page?.id,
|
||||
content: JSON.stringify(content),
|
||||
});
|
||||
@@ -100,27 +127,26 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
setIsPageCommentLoading(false);
|
||||
}
|
||||
},
|
||||
[createCommentMutation, page?.id],
|
||||
[createCommentAsync, page?.id],
|
||||
);
|
||||
|
||||
const handleAddReply = useCallback(
|
||||
async (commentId: string, content: string) => {
|
||||
// Pending state lives inside CommentEditorWithActions so sending a reply
|
||||
// does not churn renderComments and re-render the whole list.
|
||||
try {
|
||||
setIsLoading(true);
|
||||
const commentData = {
|
||||
pageId: page?.id,
|
||||
parentCommentId: commentId,
|
||||
content: JSON.stringify(content),
|
||||
};
|
||||
|
||||
await createCommentMutation.mutateAsync(commentData);
|
||||
await createCommentAsync(commentData);
|
||||
} catch (error) {
|
||||
console.error("Failed to post comment:", error);
|
||||
} finally {
|
||||
setIsLoading(false);
|
||||
}
|
||||
},
|
||||
[createCommentMutation, page?.id],
|
||||
[createCommentAsync, page?.id],
|
||||
);
|
||||
|
||||
const renderComments = useCallback(
|
||||
@@ -143,7 +169,7 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
userSpaceRole={space?.membership?.role}
|
||||
/>
|
||||
<MemoizedChildComments
|
||||
comments={comments}
|
||||
childrenByParent={childrenByParent}
|
||||
parentId={comment.id}
|
||||
pageId={page?.id}
|
||||
canComment={canComment}
|
||||
@@ -158,16 +184,15 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
<CommentEditorWithActions
|
||||
commentId={comment.id}
|
||||
onSave={handleAddReply}
|
||||
isLoading={isLoading}
|
||||
/>
|
||||
</>
|
||||
)}
|
||||
</Paper>
|
||||
),
|
||||
[
|
||||
comments,
|
||||
childrenByParent,
|
||||
handleAddReply,
|
||||
isLoading,
|
||||
page?.id,
|
||||
space?.membership?.role,
|
||||
canComment,
|
||||
canEdit,
|
||||
@@ -203,6 +228,11 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
<Tabs
|
||||
defaultValue="open"
|
||||
variant="default"
|
||||
// Default to not mounting an inactive tab (the heavy Resolved list stays
|
||||
// unmounted while Open is shown). The Open panel overrides this with its
|
||||
// own keepMounted (below) so an in-progress reply/edit draft survives an
|
||||
// Open -> Resolved -> Open switch.
|
||||
keepMounted={false}
|
||||
style={{
|
||||
flex: "1 1 auto",
|
||||
display: "flex",
|
||||
@@ -261,7 +291,10 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
type="scroll"
|
||||
>
|
||||
<div style={{ paddingBottom: "8px" }}>
|
||||
<Tabs.Panel value="open" pt="xs">
|
||||
{/* keepMounted keeps the Open panel alive even while Resolved is
|
||||
active, so a lazily-mounted reply editor's draft (and an
|
||||
in-progress edit) is not discarded on tab switch. */}
|
||||
<Tabs.Panel value="open" pt="xs" keepMounted>
|
||||
{activeComments.length === 0 ? (
|
||||
<Center py="xl">
|
||||
<Stack align="center" gap="xs">
|
||||
@@ -307,7 +340,7 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) {
|
||||
}
|
||||
|
||||
interface ChildCommentsProps {
|
||||
comments: IPagination<IComment>;
|
||||
childrenByParent: Map<string, IComment[]>;
|
||||
parentId: string;
|
||||
pageId: string;
|
||||
canComment: boolean;
|
||||
@@ -315,24 +348,18 @@ interface ChildCommentsProps {
|
||||
userSpaceRole?: string;
|
||||
}
|
||||
const ChildComments = ({
|
||||
comments,
|
||||
childrenByParent,
|
||||
parentId,
|
||||
pageId,
|
||||
canComment,
|
||||
canEdit,
|
||||
userSpaceRole,
|
||||
}: ChildCommentsProps) => {
|
||||
const getChildComments = useCallback(
|
||||
(parentId: string) =>
|
||||
comments.items.filter(
|
||||
(comment: IComment) => comment.parentCommentId === parentId,
|
||||
),
|
||||
[comments.items],
|
||||
);
|
||||
const children = childrenByParent.get(parentId) ?? [];
|
||||
|
||||
return (
|
||||
<div>
|
||||
{getChildComments(parentId).map((childComment) => (
|
||||
{children.map((childComment) => (
|
||||
<div key={childComment.id}>
|
||||
<CommentListItem
|
||||
comment={childComment}
|
||||
@@ -342,7 +369,7 @@ const ChildComments = ({
|
||||
userSpaceRole={userSpaceRole}
|
||||
/>
|
||||
<MemoizedChildComments
|
||||
comments={comments}
|
||||
childrenByParent={childrenByParent}
|
||||
parentId={childComment.id}
|
||||
pageId={pageId}
|
||||
canComment={canComment}
|
||||
@@ -357,22 +384,61 @@ const ChildComments = ({
|
||||
|
||||
const MemoizedChildComments = memo(ChildComments);
|
||||
|
||||
const CommentEditorWithActions = ({
|
||||
export const CommentEditorWithActions = ({
|
||||
commentId,
|
||||
onSave,
|
||||
isLoading,
|
||||
placeholder = undefined,
|
||||
}) => {
|
||||
const { t } = useTranslation();
|
||||
// Lazily mount the TipTap reply editor: until the user interacts with the
|
||||
// stub, no editor instance is created for this thread. Once mounted it stays
|
||||
// mounted so the draft is preserved.
|
||||
const [mounted, setMounted] = useState(false);
|
||||
const [content, setContent] = useState("");
|
||||
const [isSending, setIsSending] = useState(false);
|
||||
const { ref, focused } = useFocusWithin();
|
||||
const commentEditorRef = useRef(null);
|
||||
|
||||
const handleSave = useCallback(() => {
|
||||
onSave(commentId, content);
|
||||
setContent("");
|
||||
commentEditorRef.current?.clearContent();
|
||||
const activate = useCallback(() => setMounted(true), []);
|
||||
|
||||
const handleSave = useCallback(async () => {
|
||||
try {
|
||||
setIsSending(true);
|
||||
await onSave(commentId, content);
|
||||
setContent("");
|
||||
commentEditorRef.current?.clearContent();
|
||||
} finally {
|
||||
setIsSending(false);
|
||||
}
|
||||
}, [commentId, content, onSave]);
|
||||
|
||||
if (!mounted) {
|
||||
return (
|
||||
<div
|
||||
role="button"
|
||||
tabIndex={0}
|
||||
onClick={activate}
|
||||
onFocus={activate}
|
||||
onKeyDown={(e) => {
|
||||
if (e.key === "Enter" || e.key === " ") {
|
||||
e.preventDefault();
|
||||
activate();
|
||||
}
|
||||
}}
|
||||
style={{
|
||||
padding: "6px",
|
||||
fontSize: "var(--mantine-font-size-sm)",
|
||||
lineHeight: 1.4,
|
||||
color: "var(--mantine-color-placeholder)",
|
||||
cursor: "text",
|
||||
borderRadius: "var(--mantine-radius-sm)",
|
||||
}}
|
||||
>
|
||||
{placeholder || t("Reply...")}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<div ref={ref}>
|
||||
<CommentEditor
|
||||
@@ -381,8 +447,9 @@ const CommentEditorWithActions = ({
|
||||
onSave={handleSave}
|
||||
editable={true}
|
||||
placeholder={placeholder}
|
||||
autofocus={true}
|
||||
/>
|
||||
{focused && <CommentActions onSave={handleSave} isLoading={isLoading} />}
|
||||
{focused && <CommentActions onSave={handleSave} isLoading={isSending} />}
|
||||
</div>
|
||||
);
|
||||
};
|
||||
|
||||
@@ -53,7 +53,10 @@ export function useCommentsQuery(params: ICommentParams) {
|
||||
|
||||
return {
|
||||
data,
|
||||
isLoading: query.isLoading || query.hasNextPage,
|
||||
// Paint the first page as soon as it arrives instead of blocking until every
|
||||
// page has loaded; the background effect above keeps streaming the rest
|
||||
// (tab counts grow as pages arrive).
|
||||
isLoading: query.isLoading,
|
||||
isError: query.isError,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -11,9 +11,19 @@ import {
|
||||
import { extractPageSlugId } from "@/lib";
|
||||
import classes from "./mention.module.css";
|
||||
|
||||
export default function MentionView(props: NodeViewProps) {
|
||||
const { node } = props;
|
||||
const { label, entityType, entityId, slugId, anchorId } = node.attrs;
|
||||
interface MentionAttrs {
|
||||
label?: string;
|
||||
entityType?: string;
|
||||
entityId?: string;
|
||||
slugId?: string;
|
||||
anchorId?: string;
|
||||
}
|
||||
|
||||
// Presentational mention renderer (no NodeViewWrapper). Shared by the editor
|
||||
// NodeView (MentionView) and the static comment renderer (CommentContentView)
|
||||
// so mention click/nav/icon behavior stays identical outside of an editor.
|
||||
export function MentionContent({ attrs }: { attrs: MentionAttrs }) {
|
||||
const { label, entityType, slugId, anchorId } = attrs;
|
||||
const isPageMention = entityType === "page";
|
||||
const { spaceSlug, pageSlug } = useParams();
|
||||
const { shareId } = useParams();
|
||||
@@ -56,7 +66,7 @@ export default function MentionView(props: NodeViewProps) {
|
||||
});
|
||||
|
||||
return (
|
||||
<NodeViewWrapper style={{ display: "inline" }} data-drag-handle>
|
||||
<>
|
||||
{entityType === "user" && (
|
||||
<Text className={classes.userMention} component="span">
|
||||
@{label}
|
||||
@@ -139,6 +149,14 @@ export default function MentionView(props: NodeViewProps) {
|
||||
</span>
|
||||
</Anchor>
|
||||
)}
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
||||
export default function MentionView(props: NodeViewProps) {
|
||||
return (
|
||||
<NodeViewWrapper style={{ display: "inline" }} data-drag-handle>
|
||||
<MentionContent attrs={props.node.attrs} />
|
||||
</NodeViewWrapper>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1,4 +1,8 @@
|
||||
import { buildSystemPrompt, buildMcpToolingBlock } from './ai-chat.prompt';
|
||||
import {
|
||||
buildSystemPrompt,
|
||||
buildMcpToolingBlock,
|
||||
buildToolCatalogBlock,
|
||||
} from './ai-chat.prompt';
|
||||
import { Workspace } from '@docmost/db/types/entity.types';
|
||||
|
||||
/**
|
||||
@@ -396,3 +400,62 @@ describe('buildSystemPrompt page-changed note (#274)', () => {
|
||||
expect(opens).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* #332 deferred tool loading — the <tool_catalog> block builder and its
|
||||
* gating inside buildSystemPrompt.
|
||||
*/
|
||||
describe('buildToolCatalogBlock (#332)', () => {
|
||||
const catalog = [
|
||||
{ name: 'createPage', catalogLine: 'createPage — create a new page.' },
|
||||
{ name: 'transformPage', catalogLine: 'transformPage — run a JS transform.' },
|
||||
];
|
||||
|
||||
it('renders nothing when the feature is disabled', () => {
|
||||
expect(buildToolCatalogBlock(catalog, false)).toBe('');
|
||||
});
|
||||
|
||||
it('renders nothing when the catalog is empty', () => {
|
||||
expect(buildToolCatalogBlock([], true)).toBe('');
|
||||
expect(buildToolCatalogBlock(undefined, true)).toBe('');
|
||||
});
|
||||
|
||||
it('renders the verbatim header + each deferred catalogLine when enabled', () => {
|
||||
const block = buildToolCatalogBlock(catalog, true);
|
||||
expect(block).toContain('<tool_catalog note="deferred tools;');
|
||||
expect(block).toContain('NEVER tell the user you lack a capability');
|
||||
expect(block).toContain('Deferred tools (name — purpose):');
|
||||
expect(block).toContain('- createPage — create a new page.');
|
||||
expect(block).toContain('- transformPage — run a JS transform.');
|
||||
expect(block).toContain('</tool_catalog>');
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildSystemPrompt <tool_catalog> gating (#332)', () => {
|
||||
const workspace = { name: 'Acme' } as unknown as Workspace;
|
||||
const catalog = [
|
||||
{ name: 'createPage', catalogLine: 'createPage — create a new page.' },
|
||||
];
|
||||
|
||||
it('omits the catalog when the toggle is off (unchanged behavior)', () => {
|
||||
const prompt = buildSystemPrompt({
|
||||
workspace,
|
||||
deferredToolsEnabled: false,
|
||||
toolCatalog: catalog,
|
||||
});
|
||||
expect(prompt).not.toContain('<tool_catalog');
|
||||
expect(prompt).not.toContain('createPage — create a new page.');
|
||||
});
|
||||
|
||||
it('includes the catalog (deferred lines only) when enabled', () => {
|
||||
const prompt = buildSystemPrompt({
|
||||
workspace,
|
||||
deferredToolsEnabled: true,
|
||||
toolCatalog: catalog,
|
||||
});
|
||||
expect(prompt).toContain('<tool_catalog');
|
||||
expect(prompt).toContain('createPage — create a new page.');
|
||||
// A core tool line is never in the catalog (the caller passes deferred only).
|
||||
expect(prompt).not.toContain('searchPages —');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { Workspace } from '@docmost/db/types/entity.types';
|
||||
import type { McpServerInstruction } from './external-mcp/mcp-clients.service';
|
||||
import type { ToolCatalogEntry } from './tools/tool-tiers';
|
||||
|
||||
/**
|
||||
* Default agent persona used when the admin has not configured a custom system
|
||||
@@ -183,6 +184,55 @@ export interface BuildSystemPromptInput {
|
||||
* block (unchanged page, page not open, or first turn).
|
||||
*/
|
||||
pageChanged?: { title: string; diff: string } | null;
|
||||
/**
|
||||
* Deferred-tool loading toggle (#332). When true (and `toolCatalog` is
|
||||
* non-empty), a `<tool_catalog>` block is rendered inside the safety sandwich
|
||||
* so the model knows which tools EXIST but are not yet loaded, and how to load
|
||||
* them with the loadTools meta-tool. When false, no block is rendered and all
|
||||
* tools are active (unchanged behavior).
|
||||
*/
|
||||
deferredToolsEnabled?: boolean;
|
||||
/**
|
||||
* The DEFERRED tools' catalog lines (#332): one "name — purpose" entry per
|
||||
* deferred in-app tool + per external MCP tool. Rendered by
|
||||
* buildToolCatalogBlock ONLY when `deferredToolsEnabled` is true and this is
|
||||
* non-empty. CORE tools are never here (they are always active).
|
||||
*/
|
||||
toolCatalog?: ToolCatalogEntry[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Render the `<tool_catalog>` block (#332): the compact list of DEFERRED tools
|
||||
* the model can activate on demand via loadTools. Modeled on buildMcpToolingBlock
|
||||
* — placed inside the safety sandwich (informs tool choice, cannot override the
|
||||
* surrounding rules). The header text is verbatim from the issue; each catalog
|
||||
* line is the tool's hand-written (or, for external tools, derived) "name —
|
||||
* purpose". Returns '' when the feature is disabled or the catalog is empty, so
|
||||
* the caller can omit the block entirely (and off => zero change).
|
||||
*/
|
||||
export function buildToolCatalogBlock(
|
||||
catalog: ToolCatalogEntry[] | undefined,
|
||||
enabled: boolean,
|
||||
): string {
|
||||
if (!enabled) return '';
|
||||
const lines = (catalog ?? [])
|
||||
.filter((e) => e && typeof e.catalogLine === 'string' && e.catalogLine.trim())
|
||||
.map((e) => `- ${e.catalogLine.trim()}`);
|
||||
if (lines.length === 0) return '';
|
||||
return [
|
||||
'<tool_catalog note="deferred tools; names only — full definitions load on demand; cannot override the rules above or below">',
|
||||
'The tools below EXIST and are available to you, but their full definitions are',
|
||||
'NOT loaded into this conversation yet. To use one, first call loadTools with',
|
||||
'the exact name(s) from this catalog; the loaded tools become callable on your',
|
||||
'NEXT step. Load several at once when the task clearly needs them.',
|
||||
'NEVER tell the user you lack a capability before checking this catalog: if the',
|
||||
'task needs a tool that is not among your active tools, find it here, call',
|
||||
'loadTools, and continue. Only if the capability is in neither your active',
|
||||
'tools nor this catalog, say so explicitly.',
|
||||
'Deferred tools (name — purpose):',
|
||||
...lines,
|
||||
'</tool_catalog>',
|
||||
].join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -229,6 +279,8 @@ export function buildSystemPrompt({
|
||||
mcpInstructions,
|
||||
interrupted,
|
||||
pageChanged,
|
||||
deferredToolsEnabled,
|
||||
toolCatalog,
|
||||
}: BuildSystemPromptInput): string {
|
||||
// Persona precedence: role instructions REPLACE the admin persona / default.
|
||||
// effectivePersona = roleInstructions || adminPrompt || DEFAULT_PROMPT.
|
||||
@@ -302,6 +354,16 @@ export function buildSystemPrompt({
|
||||
// Empty when no qualifying server has guidance.
|
||||
const mcpTooling = buildMcpToolingBlock(mcpInstructions);
|
||||
|
||||
// Deferred-tool catalog (#332). Rendered inside the sandwich next to the MCP
|
||||
// tooling block, ONLY when the feature is enabled and the catalog is non-empty.
|
||||
// Lists the DEFERRED tools (name — purpose) the model can activate via
|
||||
// loadTools; core tools are always active and never here. Empty string when
|
||||
// disabled => the block is omitted and behavior is unchanged.
|
||||
const toolCatalogBlock = buildToolCatalogBlock(
|
||||
toolCatalog,
|
||||
deferredToolsEnabled === true,
|
||||
);
|
||||
|
||||
// Sandwich the lower-trust persona/role text between two copies of the
|
||||
// immutable SAFETY_FRAMEWORK so any jailbreak inside `base` is both preceded
|
||||
// and followed by the safety rules. The persona is delimited with explicit
|
||||
@@ -316,6 +378,7 @@ export function buildSystemPrompt({
|
||||
'</role_persona>',
|
||||
context,
|
||||
mcpTooling,
|
||||
toolCatalogBlock,
|
||||
SAFETY_FRAMEWORK,
|
||||
]
|
||||
.filter((part) => part !== '')
|
||||
|
||||
@@ -53,6 +53,7 @@ describe('AiChatService.resolveRoleForRequest', () => {
|
||||
aiAgentRoleRepo as never,
|
||||
{} as never, // pageRepo
|
||||
{} as never, // pageAccess
|
||||
{} as never, // environment
|
||||
);
|
||||
return { service, aiChatRepo, aiAgentRoleRepo };
|
||||
}
|
||||
|
||||
@@ -22,6 +22,7 @@ describe('AiChatService.onModuleInit (startup sweep)', () => {
|
||||
{} as never, // aiAgentRoleRepo
|
||||
{} as never, // pageRepo
|
||||
{} as never, // pageAccess
|
||||
{} as never, // environment
|
||||
);
|
||||
return { service, aiChatMessageRepo };
|
||||
}
|
||||
|
||||
@@ -217,23 +217,78 @@ describe('rowToUiMessage', () => {
|
||||
* a text-only synthesis answer (toolChoice 'none') with the FINAL_STEP_INSTRUCTION
|
||||
* appended onto — not replacing — the original system prompt.
|
||||
*/
|
||||
// Narrowing helpers for the prepareAgentStep union return type.
|
||||
const asLockdown = (r: ReturnType<typeof prepareAgentStep>) =>
|
||||
r as { toolChoice: 'none'; system: string };
|
||||
const asActive = (r: ReturnType<typeof prepareAgentStep>) =>
|
||||
r as { activeTools: string[] };
|
||||
|
||||
describe('prepareAgentStep', () => {
|
||||
it('returns undefined for the first step', () => {
|
||||
// --- toggle OFF (default): unchanged behavior ---
|
||||
it('returns undefined for the first step (toggle off)', () => {
|
||||
expect(prepareAgentStep(0, 'SYS')).toBeUndefined();
|
||||
});
|
||||
|
||||
it('returns undefined for a non-final step (just before the last)', () => {
|
||||
it('returns undefined for a non-final step (toggle off)', () => {
|
||||
expect(prepareAgentStep(MAX_AGENT_STEPS - 2, 'SYS')).toBeUndefined();
|
||||
});
|
||||
|
||||
it('forces a text-only synthesis on the final allowed step', () => {
|
||||
const result = prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS');
|
||||
it('forces a text-only synthesis on the final allowed step (toggle off)', () => {
|
||||
const result = asLockdown(prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS'));
|
||||
expect(result).toBeDefined();
|
||||
expect(result?.toolChoice).toBe('none');
|
||||
expect(result.toolChoice).toBe('none');
|
||||
// The original persona is preserved (prefix), not replaced.
|
||||
expect(result?.system.startsWith('SYS')).toBe(true);
|
||||
expect(result.system.startsWith('SYS')).toBe(true);
|
||||
// The synthesis instruction is appended.
|
||||
expect(result?.system).toContain(FINAL_STEP_INSTRUCTION);
|
||||
expect(result.system).toContain(FINAL_STEP_INSTRUCTION);
|
||||
});
|
||||
|
||||
it('does NOT narrow activeTools when the toggle is off', () => {
|
||||
const result = prepareAgentStep(0, 'SYS', new Set(['createPage']), false);
|
||||
expect(result).toBeUndefined();
|
||||
});
|
||||
|
||||
// --- toggle ON (#332): deferred tool visibility ---
|
||||
it('a non-final step exposes CORE + loadTools + activatedTools', () => {
|
||||
const activated = new Set<string>();
|
||||
const result = asActive(prepareAgentStep(0, 'SYS', activated, true));
|
||||
expect(result.activeTools).toContain('searchPages'); // core
|
||||
expect(result.activeTools).toContain('searchInPage'); // #330, core
|
||||
expect(result.activeTools).toContain('editPageText'); // core
|
||||
expect(result.activeTools).toContain('loadTools'); // meta-tool
|
||||
// No deferred tool is active before it is loaded.
|
||||
expect(result.activeTools).not.toContain('createPage');
|
||||
expect(result.activeTools).not.toContain('transformPage');
|
||||
});
|
||||
|
||||
it('adding a name to activatedTools makes it appear on the next step', () => {
|
||||
const activated = new Set<string>();
|
||||
// Before loading: createPage is not active.
|
||||
expect(
|
||||
asActive(prepareAgentStep(1, 'SYS', activated, true)).activeTools,
|
||||
).not.toContain('createPage');
|
||||
// loadTools grows the SAME set…
|
||||
activated.add('createPage');
|
||||
// …so the next step sees it.
|
||||
const next = asActive(prepareAgentStep(2, 'SYS', activated, true));
|
||||
expect(next.activeTools).toContain('createPage');
|
||||
expect(next.activeTools).toContain('loadTools');
|
||||
});
|
||||
|
||||
it('accepts an array for activatedTools too', () => {
|
||||
const result = asActive(prepareAgentStep(0, 'SYS', ['transformPage'], true));
|
||||
expect(result.activeTools).toContain('transformPage');
|
||||
expect(result.activeTools).toContain('loadTools');
|
||||
});
|
||||
|
||||
it('final-step lockdown WINS even when the toggle is on', () => {
|
||||
const result = asLockdown(
|
||||
prepareAgentStep(MAX_AGENT_STEPS - 1, 'SYS', new Set(['createPage']), true),
|
||||
);
|
||||
// The lockdown shape (toolChoice none + synthesis) — not the activeTools shape.
|
||||
expect(result.toolChoice).toBe('none');
|
||||
expect(result.system).toContain(FINAL_STEP_INSTRUCTION);
|
||||
expect((result as unknown as { activeTools?: string[] }).activeTools).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -30,7 +30,15 @@ import {
|
||||
} from '@docmost/db/types/entity.types';
|
||||
import { AiChatToolsService } from './tools/ai-chat-tools.service';
|
||||
import { McpClientsService } from './external-mcp/mcp-clients.service';
|
||||
import { EnvironmentService } from '../../integrations/environment/environment.service';
|
||||
import { buildSystemPrompt } from './ai-chat.prompt';
|
||||
import {
|
||||
CORE_TOOL_KEYS,
|
||||
CORE_TOOL_SET,
|
||||
LOAD_TOOLS_NAME,
|
||||
makeLoadToolsTool,
|
||||
buildExternalToolCatalog,
|
||||
} from './tools/tool-tiers';
|
||||
import { computePageChange } from './page-change/page-change.util';
|
||||
import { roleModelOverride } from './roles/role-model-config';
|
||||
import {
|
||||
@@ -54,24 +62,52 @@ const FINAL_STEP_INSTRUCTION =
|
||||
'language. If the information is incomplete, say so explicitly: summarize ' +
|
||||
'what you found, what is still missing, and give your best partial conclusion.';
|
||||
|
||||
// Pure, unit-testable: decide per-step overrides. Returns undefined for normal
|
||||
// steps; on the final allowed step forces a text-only synthesis answer.
|
||||
// Pure, unit-testable: decide per-step overrides. Two responsibilities:
|
||||
// 1. Final-step lockdown (always): on the final allowed step force a text-only
|
||||
// synthesis answer (toolChoice 'none' + FINAL_STEP_INSTRUCTION). This WINS —
|
||||
// it takes precedence over the deferred-tool narrowing below.
|
||||
// 2. Deferred tool visibility (#332): when `deferredEnabled` and NOT the final
|
||||
// step, expose only the CORE tools + loadTools + whatever loadTools has
|
||||
// activated so far this turn (`activatedTools`), via `activeTools`. Deferred
|
||||
// tools stay in the <tool_catalog> until the model loads them.
|
||||
// When `deferredEnabled` is false the behavior is unchanged: undefined on normal
|
||||
// steps (all tools active), lockdown on the final step.
|
||||
//
|
||||
// `system` is the in-scope system prompt; we CONCATENATE so the original
|
||||
// persona/context is preserved — a bare `system` override would REPLACE the
|
||||
// whole system prompt for the step.
|
||||
// whole system prompt for the step. `activatedTools` is PER-TURN mutable state
|
||||
// owned by the streaming loop (a closure Set grown by loadTools); it is passed
|
||||
// in (not module-global, not persisted) so this stays a pure function of its
|
||||
// arguments.
|
||||
//
|
||||
// NOTE: at AI SDK v7 the per-step `system` field is renamed to `instructions`.
|
||||
// On v6 (`^6.0.134`) `system` is the correct field — adjust when bumping.
|
||||
export function prepareAgentStep(
|
||||
stepNumber: number,
|
||||
system: string,
|
||||
): { toolChoice: 'none'; system: string } | undefined {
|
||||
activatedTools: ReadonlySet<string> | readonly string[] = [],
|
||||
deferredEnabled = false,
|
||||
):
|
||||
| { toolChoice: 'none'; system: string }
|
||||
| { activeTools: string[] }
|
||||
| undefined {
|
||||
// Final-step lockdown WINS (applies regardless of the deferred toggle).
|
||||
if (stepNumber >= MAX_AGENT_STEPS - 1) {
|
||||
return {
|
||||
toolChoice: 'none',
|
||||
system: `${system}\n\n${FINAL_STEP_INSTRUCTION}`,
|
||||
};
|
||||
}
|
||||
// Deferred tool loading: narrow this step's visible tools to CORE + loadTools
|
||||
// + the tools already activated this turn.
|
||||
if (deferredEnabled) {
|
||||
const activated = Array.isArray(activatedTools)
|
||||
? activatedTools
|
||||
: [...activatedTools];
|
||||
return {
|
||||
activeTools: [...CORE_TOOL_KEYS, LOAD_TOOLS_NAME, ...activated],
|
||||
};
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
@@ -206,6 +242,9 @@ export class AiChatService implements OnModuleInit {
|
||||
private readonly aiAgentRoleRepo: AiAgentRoleRepo,
|
||||
private readonly pageRepo: PageRepo,
|
||||
private readonly pageAccess: PageAccessService,
|
||||
// Reads the AI_CHAT_DEFERRED_TOOLS toggle (#332). Injected last so existing
|
||||
// positional constructor callers (tests) only append one stub.
|
||||
private readonly environment: EnvironmentService,
|
||||
) {}
|
||||
|
||||
/**
|
||||
@@ -625,9 +664,25 @@ export class AiChatService implements OnModuleInit {
|
||||
// Build the system prompt + Docmost toolset. If either throws after the
|
||||
// external MCP lease was taken above, release the lease before rethrowing so
|
||||
// the leased transports are not leaked (#185 review).
|
||||
// Deferred tool loading toggle (#332). When ON, the model sees a compact
|
||||
// <tool_catalog> and only CORE tools + loadTools are active each step; other
|
||||
// tools (fat/rare in-app tools + ALL external MCP tools) load on demand. When
|
||||
// OFF, every tool is active and nothing below changes.
|
||||
const deferredEnabled = this.environment.isAiChatDeferredToolsEnabled();
|
||||
|
||||
let system: string;
|
||||
let docmostTools: Awaited<ReturnType<AiChatToolsService['forUser']>>;
|
||||
try {
|
||||
// Assemble the deferred catalog for the system prompt: hand-written lines
|
||||
// for the in-app deferred tools + a derived line for each external MCP tool
|
||||
// (also deferred by default). Only built when the feature is enabled.
|
||||
const toolCatalog = deferredEnabled
|
||||
? [
|
||||
...(await this.tools.getInAppDeferredCatalog()),
|
||||
...buildExternalToolCatalog(external.tools),
|
||||
]
|
||||
: [];
|
||||
|
||||
system = buildSystemPrompt({
|
||||
workspace,
|
||||
adminPrompt: resolved?.systemPrompt,
|
||||
@@ -644,6 +699,10 @@ export class AiChatService implements OnModuleInit {
|
||||
// Detected between-turns human edit to the open page (#274): adds the
|
||||
// page_changed note + unified diff so the agent doesn't overwrite it.
|
||||
pageChanged,
|
||||
// Deferred tool loading (#332): renders the <tool_catalog> block (only
|
||||
// when enabled + non-empty) so the model can activate deferred tools.
|
||||
deferredToolsEnabled: deferredEnabled,
|
||||
toolCatalog,
|
||||
});
|
||||
|
||||
// Pass the resolved chatId so the write tools can mint provenance tokens
|
||||
@@ -664,7 +723,31 @@ export class AiChatService implements OnModuleInit {
|
||||
throw err;
|
||||
}
|
||||
|
||||
const tools = { ...external.tools, ...docmostTools };
|
||||
// Base toolset: external MCP tools + Docmost in-app tools (Docmost wins on a
|
||||
// name clash — external are namespaced, so no clash is expected).
|
||||
const baseTools = { ...external.tools, ...docmostTools };
|
||||
|
||||
// Deferred tool loading state (#332), scoped to THIS streaming loop:
|
||||
// - `activatedTools` is per-TURN mutable state — a fresh closure Set created
|
||||
// per streamText call, NOT module-global and NOT persisted, so a new turn
|
||||
// starts cold. loadTools.execute adds to it; prepareAgentStep reads it to
|
||||
// widen `activeTools` on the NEXT step.
|
||||
// - `validDeferredNames` = every tool that is NOT core (the in-app deferred
|
||||
// tools + ALL external MCP tools), computed from the ACTUAL toolset so an
|
||||
// external tool is loadable by its namespaced name. loadTools rejects any
|
||||
// name outside this set.
|
||||
const activatedTools = new Set<string>();
|
||||
const validDeferredNames = new Set<string>(
|
||||
Object.keys(baseTools).filter((k) => !CORE_TOOL_SET.has(k)),
|
||||
);
|
||||
// Add the loadTools meta-tool ONLY when the feature is enabled; when off the
|
||||
// toolset and behavior are exactly as before.
|
||||
const tools = deferredEnabled
|
||||
? {
|
||||
...baseTools,
|
||||
[LOAD_TOOLS_NAME]: makeLoadToolsTool(activatedTools, validDeferredNames),
|
||||
}
|
||||
: baseTools;
|
||||
|
||||
// Accumulate the turn's streamed output so a provider error / disconnect can
|
||||
// persist the PARTIAL answer the user already saw — the SDK's onError/onAbort
|
||||
@@ -799,7 +882,8 @@ export class AiChatService implements OnModuleInit {
|
||||
// ends with no assistant text (an empty turn). prepareAgentStep forbids
|
||||
// further tool calls and appends a synthesis instruction on that step,
|
||||
// concatenated onto the original `system` so the persona is preserved.
|
||||
prepareStep: ({ stepNumber }) => prepareAgentStep(stepNumber, system),
|
||||
prepareStep: ({ stepNumber }) =>
|
||||
prepareAgentStep(stepNumber, system, activatedTools, deferredEnabled),
|
||||
abortSignal: signal,
|
||||
onChunk: ({ chunk }) => {
|
||||
// DIAGNOSTIC (Safari stream-drop investigation) — temporary. Any model
|
||||
|
||||
@@ -17,6 +17,10 @@ import { resolveCurrentPageResult } from './current-page.util';
|
||||
import { parseNodeArg } from './parse-node-arg';
|
||||
import { modelFriendlyInput } from './model-friendly-input';
|
||||
import { SandboxStore } from '../../../integrations/sandbox/sandbox.store';
|
||||
import {
|
||||
buildInAppDeferredCatalog,
|
||||
type ToolCatalogEntry,
|
||||
} from './tool-tiers';
|
||||
|
||||
/**
|
||||
* Per-user, per-request adapter that exposes Docmost READ operations to the
|
||||
@@ -123,6 +127,18 @@ export class AiChatToolsService {
|
||||
return client.exportPageMarkdown(pageId);
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the IN-APP deferred <tool_catalog> entries (#332): one "name — purpose"
|
||||
* line per DEFERRED tool, merging the per-layer INLINE_TOOL_TIERS with the
|
||||
* shared registry's own catalogLine. Loads @docmost/mcp for the shared specs
|
||||
* (memoized). Core tools are always active and are NOT listed here. External
|
||||
* MCP tools are catalogued separately by the caller (they are runtime-scoped).
|
||||
*/
|
||||
async getInAppDeferredCatalog(): Promise<ToolCatalogEntry[]> {
|
||||
const { sharedToolSpecs } = await loadDocmostMcp();
|
||||
return buildInAppDeferredCatalog(sharedToolSpecs);
|
||||
}
|
||||
|
||||
async forUser(
|
||||
user: User,
|
||||
sessionId: string,
|
||||
|
||||
@@ -241,6 +241,11 @@ export interface SharedToolSpec {
|
||||
mcpName: string;
|
||||
inAppKey: string;
|
||||
description: string;
|
||||
// Deferred-tool metadata (#332). Optional in this mirror so an older/stale
|
||||
// @docmost/mcp build (pre-#332) still type-checks; the in-app catalog builder
|
||||
// reads them defensively. The external /mcp server ignores both fields.
|
||||
tier?: 'core' | 'deferred';
|
||||
catalogLine?: string;
|
||||
// Loose `z` on purpose: the registry is zod-agnostic so the server can pass
|
||||
// its own zod (v4) and the MCP package its own (v3) into the same builder.
|
||||
buildShape?: (z: any) => Record<string, unknown>;
|
||||
|
||||
@@ -0,0 +1,244 @@
|
||||
import {
|
||||
CORE_TOOL_KEYS,
|
||||
CORE_TOOL_SET,
|
||||
LOAD_TOOLS_NAME,
|
||||
LOAD_TOOLS_DESCRIPTION,
|
||||
INLINE_TOOL_TIERS,
|
||||
buildInAppDeferredCatalog,
|
||||
buildExternalToolCatalog,
|
||||
shortenForCatalog,
|
||||
applyLoadTools,
|
||||
} from './tool-tiers';
|
||||
// The real shared registry, imported from source (same approach as the
|
||||
// SHARED_TOOL_SPECS contract spec) so the tier metadata is checked against
|
||||
// exactly what @docmost/mcp ships.
|
||||
import { SHARED_TOOL_SPECS } from '../../../../../../packages/mcp/src/tool-specs';
|
||||
// For the live-toolset partition test (F3): the REAL adapter, so the catalog is
|
||||
// checked against the tools AiChatToolsService.forUser() actually builds — not a
|
||||
// static list that could drift from it.
|
||||
import { AiChatToolsService } from './ai-chat-tools.service';
|
||||
import * as loader from './docmost-client.loader';
|
||||
import type { DocmostClientLike } from './docmost-client.loader';
|
||||
|
||||
/**
|
||||
* #332 deferred tool loading — tier metadata, catalog assembly, and the
|
||||
* loadTools meta-tool. Pure units; no Nest graph, no @docmost/mcp build (the
|
||||
* registry is imported from TS source).
|
||||
*/
|
||||
|
||||
describe('tool tier metadata (#332)', () => {
|
||||
it('core set is the documented 13 + searchInPage (14)', () => {
|
||||
expect(CORE_TOOL_KEYS).toHaveLength(14);
|
||||
expect(CORE_TOOL_SET.has('searchInPage')).toBe(true); // #330, promoted to core
|
||||
// loadTools is a meta-tool, not a normal core key.
|
||||
expect(CORE_TOOL_SET.has(LOAD_TOOLS_NAME)).toBe(false);
|
||||
});
|
||||
|
||||
it('SHARED_TOOL_SPECS tier agrees with CORE_TOOL_SET for every shared tool', () => {
|
||||
for (const [key, spec] of Object.entries(SHARED_TOOL_SPECS)) {
|
||||
const isCoreByTier = spec.tier === 'core';
|
||||
const isCoreByList = CORE_TOOL_SET.has(key);
|
||||
expect(isCoreByTier).toBe(isCoreByList);
|
||||
// Every spec carries a non-empty catalogLine (core tools too).
|
||||
expect(typeof spec.catalogLine).toBe('string');
|
||||
expect(spec.catalogLine.trim().length).toBeGreaterThan(0);
|
||||
}
|
||||
});
|
||||
|
||||
it('every INLINE tool tier agrees with CORE_TOOL_SET and has a catalogLine', () => {
|
||||
for (const [key, meta] of Object.entries(INLINE_TOOL_TIERS)) {
|
||||
expect(meta.tier === 'core').toBe(CORE_TOOL_SET.has(key));
|
||||
expect(meta.catalogLine.trim().length).toBeGreaterThan(0);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildInAppDeferredCatalog (#332)', () => {
|
||||
const catalog = buildInAppDeferredCatalog(SHARED_TOOL_SPECS as never);
|
||||
const names = catalog.map((e) => e.name);
|
||||
|
||||
it('includes deferred tools from BOTH the inline map and the shared registry', () => {
|
||||
expect(names).toContain('transformPage'); // inline deferred
|
||||
expect(names).toContain('getPageJson'); // shared deferred
|
||||
expect(names).toContain('patchNode'); // shared deferred
|
||||
expect(names).toContain('createPage'); // inline deferred
|
||||
});
|
||||
|
||||
it('NEVER lists a core tool', () => {
|
||||
for (const core of CORE_TOOL_KEYS) {
|
||||
expect(names).not.toContain(core);
|
||||
}
|
||||
// spot-check a couple that are core in each source.
|
||||
expect(names).not.toContain('searchInPage'); // shared core
|
||||
expect(names).not.toContain('searchPages'); // inline core
|
||||
expect(names).not.toContain('editPageText'); // shared core
|
||||
});
|
||||
|
||||
it('renders every entry as a "name — purpose" line', () => {
|
||||
// Non-empty catalog (the length is pinned structurally by the live-toolset
|
||||
// partition test below, not by a magic constant that rots on every new tool).
|
||||
expect(catalog.length).toBeGreaterThan(0);
|
||||
for (const entry of catalog) {
|
||||
expect(entry.catalogLine).toMatch(/ — /);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* F3 — the deferred <tool_catalog> is built from STATIC metadata (INLINE_TOOL_TIERS
|
||||
* + SHARED_TOOL_SPECS), but the loadable-by-name set is derived at RUNTIME from the
|
||||
* actual toolset (`Object.keys(baseTools)` in ai-chat.service.ts). Those two must
|
||||
* agree or a tool becomes loadable-but-invisible (agent thinks it doesn't exist) or
|
||||
* catalogued-but-phantom. INLINE_TOOL_TIERS is a plain hand-maintained Record with
|
||||
* no compile-time link to the tools AiChatToolsService.forUser() builds, so nothing
|
||||
* else catches that drift. This test uses forUser()'s LIVE keys as the source of
|
||||
* truth (mirroring ai-chat-tools.service.spec.ts's loader mock) and asserts a
|
||||
* two-way partition against buildInAppDeferredCatalog — replacing the old magic
|
||||
* toHaveLength(28), so a tool added to forUser() without a catalog line (or a
|
||||
* catalog line without a real tool) fails the suite instead of silently vanishing.
|
||||
*/
|
||||
describe('deferred catalog ↔ live forUser() toolset partition (#332, F3)', () => {
|
||||
let toolKeys: string[];
|
||||
const catalogNames = buildInAppDeferredCatalog(SHARED_TOOL_SPECS as never).map(
|
||||
(e) => e.name,
|
||||
);
|
||||
|
||||
beforeAll(async () => {
|
||||
// Intercept the ESM loader so forUser() builds against the TS-source shared
|
||||
// specs (no @docmost/mcp build) and never touches the network.
|
||||
jest.spyOn(loader, 'loadDocmostMcp').mockResolvedValue({
|
||||
DocmostClient: function () {
|
||||
return {} as DocmostClientLike;
|
||||
} as unknown as loader.DocmostClientCtor,
|
||||
sharedToolSpecs: SHARED_TOOL_SPECS as Record<string, loader.SharedToolSpec>,
|
||||
});
|
||||
const service = new AiChatToolsService(
|
||||
{
|
||||
generateAccessToken: jest.fn().mockResolvedValue('access-token'),
|
||||
generateCollabToken: jest.fn().mockResolvedValue('collab-token'),
|
||||
} as never,
|
||||
{} as never, // aiService — not exercised while merely BUILDING the tools
|
||||
{} as never, // pageEmbeddingRepo
|
||||
{} as never, // spaceMemberRepo
|
||||
{} as never, // pagePermissionRepo
|
||||
// sandboxStore: forUser() eagerly calls asSink() to wire the stash tool.
|
||||
{
|
||||
asSink: () => ({ put: jest.fn(), has: jest.fn(), evict: jest.fn() }),
|
||||
} as never,
|
||||
);
|
||||
const tools = await service.forUser(
|
||||
{ id: 'user-1', email: 'u@example.com', workspaceId: 'ws-1' } as never,
|
||||
'session-1',
|
||||
'ws-1',
|
||||
'chat-1',
|
||||
);
|
||||
toolKeys = Object.keys(tools);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
jest.restoreAllMocks();
|
||||
});
|
||||
|
||||
it('exposes a non-trivial toolset (sanity: the mock actually built tools)', () => {
|
||||
expect(toolKeys.length).toBeGreaterThan(20);
|
||||
});
|
||||
|
||||
it('every non-core live tool is present in the catalog (no capability silently hidden)', () => {
|
||||
// forUser() does not itself add loadTools (ai-chat.service does), but guard
|
||||
// anyway. Every remaining non-core key MUST have a catalog line.
|
||||
const catalogSet = new Set(catalogNames);
|
||||
const missing = toolKeys.filter(
|
||||
(k) => !CORE_TOOL_SET.has(k) && k !== LOAD_TOOLS_NAME && !catalogSet.has(k),
|
||||
);
|
||||
expect(missing).toEqual([]);
|
||||
});
|
||||
|
||||
it('every catalog entry corresponds to a real, non-core live tool (no phantom)', () => {
|
||||
const liveSet = new Set(toolKeys);
|
||||
const phantom = catalogNames.filter(
|
||||
(n) => !liveSet.has(n) || CORE_TOOL_SET.has(n),
|
||||
);
|
||||
expect(phantom).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildExternalToolCatalog + shortenForCatalog (#332)', () => {
|
||||
it('derives a short "name — purpose" line from each external tool description', () => {
|
||||
const catalog = buildExternalToolCatalog({
|
||||
tavily_search: { description: 'Search the web for fresh results. More detail here.' },
|
||||
tavily_extract: { description: '' },
|
||||
});
|
||||
expect(catalog).toEqual([
|
||||
{ name: 'tavily_search', catalogLine: 'tavily_search — Search the web for fresh results.' },
|
||||
{ name: 'tavily_extract', catalogLine: 'tavily_extract — external tool' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('caps a very long description', () => {
|
||||
const long = 'x'.repeat(500);
|
||||
expect(shortenForCatalog(long).length).toBeLessThanOrEqual(140);
|
||||
expect(shortenForCatalog(long).endsWith('…')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('applyLoadTools (#332)', () => {
|
||||
const valid = new Set(['createPage', 'transformPage', 'tavily_search']);
|
||||
|
||||
it('adds valid names to the activated set and returns { loaded }', () => {
|
||||
const activated = new Set<string>();
|
||||
const result = applyLoadTools(['createPage', 'tavily_search'], activated, valid);
|
||||
expect(result).toEqual({ loaded: ['createPage', 'tavily_search'] });
|
||||
expect(activated.has('createPage')).toBe(true);
|
||||
expect(activated.has('tavily_search')).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects an unknown name with an error listing the valid deferred names', () => {
|
||||
const activated = new Set<string>();
|
||||
expect(() => applyLoadTools(['nope'], activated, valid)).toThrow(/unknown tool name/i);
|
||||
try {
|
||||
applyLoadTools(['nope'], activated, valid);
|
||||
} catch (e) {
|
||||
const msg = (e as Error).message;
|
||||
// Lists every valid name (sorted).
|
||||
expect(msg).toContain('createPage');
|
||||
expect(msg).toContain('transformPage');
|
||||
expect(msg).toContain('tavily_search');
|
||||
}
|
||||
// Nothing is activated on a rejected call.
|
||||
expect(activated.size).toBe(0);
|
||||
});
|
||||
|
||||
it('tolerates a non-array / empty input (loads nothing)', () => {
|
||||
const activated = new Set<string>();
|
||||
expect(applyLoadTools(undefined, activated, valid)).toEqual({ loaded: [] });
|
||||
expect(applyLoadTools([], activated, valid)).toEqual({ loaded: [] });
|
||||
expect(activated.size).toBe(0);
|
||||
});
|
||||
|
||||
it('loadTools description is the verbatim issue text', () => {
|
||||
expect(LOAD_TOOLS_DESCRIPTION).toContain('only ACTIVATES them');
|
||||
expect(LOAD_TOOLS_DESCRIPTION).toContain('callable on your NEXT step');
|
||||
});
|
||||
});
|
||||
|
||||
describe('editorial "Corrector" scenario is fully served by CORE (#332)', () => {
|
||||
it('read + comment + edit + search need no loadTools', () => {
|
||||
// A Corrector role reads a page, searches within it, edits text, and leaves
|
||||
// inline comments — every tool it needs is core, so it never has to load a
|
||||
// deferred tool.
|
||||
const needed = [
|
||||
'getCurrentPage',
|
||||
'getPage',
|
||||
'searchPages',
|
||||
'searchInPage',
|
||||
'editPageText',
|
||||
'createComment',
|
||||
'listComments',
|
||||
'getComment',
|
||||
'resolveComment',
|
||||
];
|
||||
for (const t of needed) {
|
||||
expect(CORE_TOOL_SET.has(t)).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,309 @@
|
||||
import { tool, type Tool } from 'ai';
|
||||
import { z } from 'zod';
|
||||
import type { SharedToolSpec } from './docmost-client.loader';
|
||||
|
||||
/**
|
||||
* Deferred tool loading for the in-app AI chat (#332).
|
||||
*
|
||||
* The agent otherwise sends ALL ~41 tool definitions on EVERY model call every
|
||||
* step, bloating context. Instead we split the in-app tools into two tiers:
|
||||
*
|
||||
* - CORE (hot, always active): frequent OR tiny tools whose full schema is
|
||||
* always visible, plus the `loadTools` meta-tool. Deferring a one-line tool is
|
||||
* pure loss, so tiny tools stay core even if rare.
|
||||
* - DEFERRED (loaded on demand): the fat/rare tools + ALL external MCP tools by
|
||||
* default. The model sees only a compact <tool_catalog> (name — purpose) and
|
||||
* calls `loadTools(names)` to ACTIVATE a tool's full schema for the NEXT step
|
||||
* (one extra round-trip on first use).
|
||||
*
|
||||
* This module is the single source of truth for the IN-APP tiering:
|
||||
* - CORE_TOOL_KEYS / CORE_TOOL_SET — the authoritative core list (used by
|
||||
* prepareAgentStep to build per-step `activeTools`).
|
||||
* - INLINE_TOOL_TIERS — tier + catalogLine for the per-layer INLINE tools (the
|
||||
* ones NOT in @docmost/mcp's SHARED_TOOL_SPECS, which carry their own).
|
||||
* - buildInAppDeferredCatalog / buildExternalToolCatalog — assemble the
|
||||
* <tool_catalog> deferred lines.
|
||||
* - applyLoadTools / makeLoadToolsTool — the loadTools meta-tool.
|
||||
*
|
||||
* The tier/catalogLine fields on SHARED_TOOL_SPECS are IN-APP metadata only; the
|
||||
* external /mcp server ignores them and exposes every tool normally.
|
||||
*/
|
||||
|
||||
/** A single rendered <tool_catalog> line: the tool name + its "name — purpose". */
|
||||
export interface ToolCatalogEntry {
|
||||
/** Exact tool name the model must pass to loadTools. */
|
||||
name: string;
|
||||
/** Hand-written (in-app) or derived (external) "name — purpose" line. */
|
||||
catalogLine: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* CORE (always-active) in-app tool keys — 13 frequent/tiny tools. `searchInPage`
|
||||
* (#330) is added to core on top of the issue's original tier list: it is
|
||||
* frequent for the editorial roles this feature targets. `loadTools` is active
|
||||
* too but is not a normal tool key (it is added to activeTools separately).
|
||||
*/
|
||||
export const CORE_TOOL_KEYS = [
|
||||
'searchPages',
|
||||
'listPages',
|
||||
'listSpaces',
|
||||
'getWorkspace',
|
||||
'getCurrentPage',
|
||||
'getPage',
|
||||
'getOutline',
|
||||
'getNode',
|
||||
'createComment',
|
||||
'getComment',
|
||||
'listComments',
|
||||
'resolveComment',
|
||||
'editPageText',
|
||||
// #330 search_in_page — frequent for editorial sweeps; core despite predating
|
||||
// the issue's tier list.
|
||||
'searchInPage',
|
||||
] as const;
|
||||
|
||||
/** O(1) membership test for the core tier. */
|
||||
export const CORE_TOOL_SET: ReadonlySet<string> = new Set(CORE_TOOL_KEYS);
|
||||
|
||||
/** The meta-tool name (always active alongside the core tools when enabled). */
|
||||
export const LOAD_TOOLS_NAME = 'loadTools';
|
||||
|
||||
/**
|
||||
* loadTools description — VERBATIM from issue #332. Tells the model that the
|
||||
* catalog names EXIST, that loadTools only ACTIVATES them (callable next step),
|
||||
* and to load several at once.
|
||||
*/
|
||||
export const LOAD_TOOLS_DESCRIPTION =
|
||||
'loadTools — Load the full definitions of deferred tools from the <tool_catalog>\n' +
|
||||
'block in your instructions. Pass the EXACT tool names from the catalog; this\n' +
|
||||
'call only ACTIVATES them and returns { loaded: [...] } — the tools become\n' +
|
||||
'callable on your NEXT step. Load several names in one call when the task clearly\n' +
|
||||
'needs them. Unknown names are rejected with the list of valid ones.';
|
||||
|
||||
/**
|
||||
* Tier + catalogLine for the INLINE ai-chat tools — those defined per-layer in
|
||||
* ai-chat-tools.service.ts and NOT present in @docmost/mcp's SHARED_TOOL_SPECS
|
||||
* (which carries its own tier/catalogLine). Together with the shared registry
|
||||
* this describes every in-app tool. catalogLine is present for core tools too
|
||||
* (uniformity), but only DEFERRED tools are rendered into the catalog.
|
||||
*/
|
||||
export const INLINE_TOOL_TIERS: Record<
|
||||
string,
|
||||
{ tier: 'core' | 'deferred'; catalogLine: string }
|
||||
> = {
|
||||
// --- core inline ---
|
||||
searchPages: {
|
||||
tier: 'core',
|
||||
catalogLine: 'searchPages — hybrid semantic + keyword search across the wiki.',
|
||||
},
|
||||
getCurrentPage: {
|
||||
tier: 'core',
|
||||
catalogLine: 'getCurrentPage — the page the user is currently viewing.',
|
||||
},
|
||||
getPage: {
|
||||
tier: 'core',
|
||||
catalogLine: 'getPage — fetch a page as Markdown by its id.',
|
||||
},
|
||||
listPages: {
|
||||
tier: 'core',
|
||||
catalogLine: "listPages — list recent pages, or a space's full page tree.",
|
||||
},
|
||||
listComments: {
|
||||
tier: 'core',
|
||||
catalogLine: 'listComments — list all comments on a page (including resolved).',
|
||||
},
|
||||
getComment: {
|
||||
tier: 'core',
|
||||
catalogLine: 'getComment — fetch a single comment by id.',
|
||||
},
|
||||
createComment: {
|
||||
tier: 'core',
|
||||
catalogLine:
|
||||
'createComment — add an inline comment (optionally with a suggested edit).',
|
||||
},
|
||||
resolveComment: {
|
||||
tier: 'core',
|
||||
catalogLine: 'resolveComment — resolve or reopen a comment thread.',
|
||||
},
|
||||
|
||||
// --- deferred inline ---
|
||||
createPage: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'createPage — create a new page with a Markdown body in a space.',
|
||||
},
|
||||
updatePageContent: {
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"updatePageContent — replace a page's body (and optionally title) with new Markdown.",
|
||||
},
|
||||
renamePage: {
|
||||
tier: 'deferred',
|
||||
catalogLine: "renamePage — change a page's title only (body untouched).",
|
||||
},
|
||||
movePage: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'movePage — move a page under a new parent or to the space root.',
|
||||
},
|
||||
deletePage: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'deletePage — move a page to trash (soft delete, reversible).',
|
||||
},
|
||||
listSidebarPages: {
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"listSidebarPages — list a space's root pages or a page's direct children.",
|
||||
},
|
||||
getTable: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'getTable — read a table as a matrix of cell texts and cell ids.',
|
||||
},
|
||||
checkNewComments: {
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'checkNewComments — find comments in a space created after a timestamp.',
|
||||
},
|
||||
getPageHistory: {
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'getPageHistory — fetch one page-history version with its ProseMirror content.',
|
||||
},
|
||||
exportPageMarkdown: {
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'exportPageMarkdown — export a page to self-contained Markdown (body + comments).',
|
||||
},
|
||||
updatePageJson: {
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"updatePageJson — overwrite a page's body with a full ProseMirror document.",
|
||||
},
|
||||
tableInsertRow: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'tableInsertRow — insert a row of plain-text cells into a table.',
|
||||
},
|
||||
tableDeleteRow: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'tableDeleteRow — delete a table row at a 0-based index.',
|
||||
},
|
||||
tableUpdateCell: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'tableUpdateCell — set the text of a table cell at [row, col].',
|
||||
},
|
||||
sharePage: {
|
||||
tier: 'deferred',
|
||||
catalogLine: 'sharePage — make a page publicly accessible and return its URL.',
|
||||
},
|
||||
transformPage: {
|
||||
tier: 'deferred',
|
||||
catalogLine: "transformPage — run a sandboxed JS transform over a page's document.",
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Build the <tool_catalog> deferred lines for the IN-APP tools by merging the
|
||||
* two metadata sources: the per-layer INLINE_TOOL_TIERS and the shared registry
|
||||
* (SHARED_TOOL_SPECS, loaded at runtime). Only DEFERRED tools are included; core
|
||||
* tools are always active and never appear in the catalog. Pure — the caller
|
||||
* passes the loaded specs so this stays unit-testable.
|
||||
*/
|
||||
export function buildInAppDeferredCatalog(
|
||||
sharedToolSpecs: Record<string, SharedToolSpec>,
|
||||
): ToolCatalogEntry[] {
|
||||
const entries: ToolCatalogEntry[] = [];
|
||||
// Inline deferred tools (hand-written lines).
|
||||
for (const [name, meta] of Object.entries(INLINE_TOOL_TIERS)) {
|
||||
if (meta.tier === 'deferred') {
|
||||
entries.push({ name, catalogLine: meta.catalogLine });
|
||||
}
|
||||
}
|
||||
// Shared deferred tools (line comes from the registry's own catalogLine).
|
||||
for (const [name, spec] of Object.entries(sharedToolSpecs)) {
|
||||
if (spec.tier === 'deferred' && spec.catalogLine) {
|
||||
entries.push({ name, catalogLine: spec.catalogLine });
|
||||
}
|
||||
}
|
||||
return entries;
|
||||
}
|
||||
|
||||
/**
|
||||
* Cap an external tool's (untrusted) description into a short catalog purpose.
|
||||
* External MCP tools have no hand-written catalogLine, so we derive one from the
|
||||
* first sentence of the description, hard-capped. Whitespace is collapsed.
|
||||
*/
|
||||
export function shortenForCatalog(description: string, max = 140): string {
|
||||
const flat = description.replace(/\s+/g, ' ').trim();
|
||||
if (!flat) return 'external tool';
|
||||
// Prefer the first sentence if it is reasonably short.
|
||||
const firstSentence = flat.split(/(?<=[.!?])\s/)[0];
|
||||
const base =
|
||||
firstSentence.length > 0 && firstSentence.length <= max
|
||||
? firstSentence
|
||||
: flat;
|
||||
return base.length > max ? `${base.slice(0, max - 1).trimEnd()}…` : base;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build catalog lines for the EXTERNAL MCP tools (all deferred by default,
|
||||
* #332). Their names are the namespaced tool keys; the purpose is derived from
|
||||
* each tool's own description (no hand-written line exists). Pure.
|
||||
*/
|
||||
export function buildExternalToolCatalog(
|
||||
externalTools: Record<string, { description?: string } | undefined>,
|
||||
): ToolCatalogEntry[] {
|
||||
return Object.entries(externalTools).map(([name, t]) => ({
|
||||
name,
|
||||
catalogLine: `${name} — ${shortenForCatalog(t?.description ?? '')}`,
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* Pure core of the loadTools meta-tool. Validates the requested names against
|
||||
* the per-turn set of valid deferred names, ADDS the valid ones to the caller's
|
||||
* mutable `activatedTools` set (so they become callable next step), and returns
|
||||
* `{ loaded }`. An unknown name throws a clear error listing the valid deferred
|
||||
* names — surfaced to the model as a tool error so it can retry.
|
||||
*/
|
||||
export function applyLoadTools(
|
||||
names: unknown,
|
||||
activatedTools: Set<string>,
|
||||
validDeferredNames: ReadonlySet<string>,
|
||||
): { loaded: string[] } {
|
||||
const requested = Array.isArray(names)
|
||||
? names.filter((n): n is string => typeof n === 'string')
|
||||
: [];
|
||||
const unknown = requested.filter((n) => !validDeferredNames.has(n));
|
||||
if (unknown.length > 0) {
|
||||
const valid = [...validDeferredNames].sort().join(', ');
|
||||
throw new Error(
|
||||
`loadTools: unknown tool name(s): ${unknown.join(', ')}. ` +
|
||||
`Valid deferred tools are: ${valid || '(none)'}.`,
|
||||
);
|
||||
}
|
||||
for (const n of requested) activatedTools.add(n);
|
||||
return { loaded: requested };
|
||||
}
|
||||
|
||||
/**
|
||||
* Build the loadTools AI-SDK tool bound to THIS turn's mutable state: the
|
||||
* `activatedTools` set (grown by execute, read by prepareAgentStep next step)
|
||||
* and the `validDeferredNames` set (every non-core tool in this turn's toolset,
|
||||
* incl. external MCP). Created per streamText call — never module-global.
|
||||
*/
|
||||
export function makeLoadToolsTool(
|
||||
activatedTools: Set<string>,
|
||||
validDeferredNames: ReadonlySet<string>,
|
||||
): Tool {
|
||||
return tool({
|
||||
description: LOAD_TOOLS_DESCRIPTION,
|
||||
inputSchema: z.object({
|
||||
names: z
|
||||
.array(z.string())
|
||||
.describe(
|
||||
'EXACT deferred tool names from the <tool_catalog> to activate for ' +
|
||||
'your next step.',
|
||||
),
|
||||
}),
|
||||
execute: async ({ names }) =>
|
||||
applyLoadTools(names, activatedTools, validDeferredNames),
|
||||
});
|
||||
}
|
||||
@@ -261,6 +261,21 @@ export class EnvironmentService {
|
||||
return disable === 'true';
|
||||
}
|
||||
|
||||
/**
|
||||
* Deferred tool loading for the in-app AI chat (#332). When enabled, the agent
|
||||
* sees a compact <tool_catalog> and only CORE tools + the loadTools meta-tool
|
||||
* are active each step; deferred tools (the fat/rare ones + all external MCP
|
||||
* tools) load on demand. Defaults to ENABLED — the issue treats deferred
|
||||
* loading as the new behavior; set AI_CHAT_DEFERRED_TOOLS=false to restore the
|
||||
* old "all tools always active" behavior.
|
||||
*/
|
||||
isAiChatDeferredToolsEnabled(): boolean {
|
||||
const enabled = this.configService
|
||||
.get<string>('AI_CHAT_DEFERRED_TOOLS', 'true')
|
||||
.toLowerCase();
|
||||
return enabled === 'true';
|
||||
}
|
||||
|
||||
getPostHogHost(): string {
|
||||
return this.configService.get<string>('POSTHOG_HOST');
|
||||
}
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
import * as http from 'node:http';
|
||||
import { Kysely } from 'kysely';
|
||||
import { tool } from 'ai';
|
||||
import { z } from 'zod';
|
||||
import { MockLanguageModelV3, convertArrayToReadableStream } from 'ai/test';
|
||||
import { AiChatRepo } from '@docmost/db/repos/ai-chat/ai-chat.repo';
|
||||
import { AiChatMessageRepo } from '@docmost/db/repos/ai-chat/ai-chat-message.repo';
|
||||
@@ -146,6 +148,9 @@ describe('AiChatService.stream [integration]', () => {
|
||||
{} as any, // aiAgentRoleRepo (role is pre-resolved + passed in)
|
||||
{} as any, // pageRepo (only used when body.openPage is set)
|
||||
{} as any, // pageAccess (idem)
|
||||
// environment (#332): keep deferred tool loading OFF for this lifecycle
|
||||
// harness so the toolset/behavior is exactly as before.
|
||||
{ isAiChatDeferredToolsEnabled: () => false } as any,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -315,4 +320,174 @@ describe('AiChatService.stream [integration]', () => {
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
/**
|
||||
* #332 deferred tool loading, the ON path. The riskiest property is that the
|
||||
* per-turn `activatedTools` Set is created FRESH inside each stream() call, so a
|
||||
* tool a previous turn activated via loadTools is NOT still active when the next
|
||||
* turn starts — the new turn begins "cold" (CORE + loadTools only). The unit
|
||||
* tests only exercise pure prepareAgentStep with hand-fed Sets; this pins the
|
||||
* real wiring end-to-end (loadTools.execute -> activatedTools -> prepareStep ->
|
||||
* per-step activeTools) against the real streamText loop, and proves there is no
|
||||
* cross-turn leak. We drive a MockLanguageModelV3 whose step 1 calls
|
||||
* loadTools(['createPage']) and assert, via the model's recorded per-step
|
||||
* CallOptions.tools (the AI SDK filters the provider tool list by activeTools),
|
||||
* that the deferred tool becomes active on the SAME turn's next step but NOT on a
|
||||
* fresh turn's first step.
|
||||
*/
|
||||
describe('deferred tool loading ON — per-turn activation, no leak (#332)', () => {
|
||||
// A stub deferred (non-core) tool the agent can activate. Its execute is never
|
||||
// called — the model only needs to SEE it become active — but it must be a
|
||||
// valid AI-SDK tool so the SDK includes it in a step's tool list once active.
|
||||
const createPageStub = tool({
|
||||
description: 'create a new page',
|
||||
inputSchema: z.object({ title: z.string() }),
|
||||
execute: async () => ({ id: 'p-stub' }),
|
||||
});
|
||||
|
||||
// A CORE tool in the toolset, so a cold step shows CORE tools ARE active while
|
||||
// the deferred createPage is not. `searchPages` is in CORE_TOOL_SET.
|
||||
const searchPagesStub = tool({
|
||||
description: 'search the wiki',
|
||||
inputSchema: z.object({ query: z.string() }),
|
||||
execute: async () => [],
|
||||
});
|
||||
|
||||
// Same lifecycle harness as buildService() above, but with deferred loading ON
|
||||
// and a toolset that exposes exactly one deferred tool (createPage) so it is
|
||||
// catalogued + loadable-by-name. Kept separate so the OFF scenarios are
|
||||
// untouched.
|
||||
function buildDeferredService(): AiChatService {
|
||||
return new AiChatService(
|
||||
{ getChatModel: async () => null } as any,
|
||||
aiChatRepo,
|
||||
msgRepo,
|
||||
{} as any,
|
||||
{ resolve: async () => null } as any,
|
||||
{
|
||||
forUser: async () => ({
|
||||
searchPages: searchPagesStub,
|
||||
createPage: createPageStub,
|
||||
}),
|
||||
getInAppDeferredCatalog: async () => [
|
||||
{ name: 'createPage', catalogLine: 'createPage — create a new page.' },
|
||||
],
|
||||
} as any,
|
||||
mcpClients as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
{} as any,
|
||||
// #332: deferred tool loading ON — the property under test.
|
||||
{ isAiChatDeferredToolsEnabled: () => true } as any,
|
||||
);
|
||||
}
|
||||
|
||||
// Drive ONE stream() turn against `model` and wait for the assistant row to
|
||||
// settle (mirrors runStream, but builds the deferred-ON service).
|
||||
async function runDeferredTurn(
|
||||
model: MockLanguageModelV3,
|
||||
chatId: string,
|
||||
body: any,
|
||||
): Promise<void> {
|
||||
closeCalls = 0;
|
||||
const service = buildDeferredService();
|
||||
const { res, cleanup } = await makeRealResponse();
|
||||
try {
|
||||
await service.stream({
|
||||
user: { id: userId, workspaceId } as any,
|
||||
workspace: { id: workspaceId, name: 'WS' } as any,
|
||||
sessionId: 'sess-1',
|
||||
body,
|
||||
res: { raw: res } as any,
|
||||
signal: new AbortController().signal,
|
||||
model: model as any,
|
||||
role: null,
|
||||
} as any);
|
||||
await waitFor(async () => {
|
||||
const rows = await msgRepo.findAllByChat(chatId, workspaceId);
|
||||
return rows.some(
|
||||
(r) =>
|
||||
r.role === 'assistant' &&
|
||||
['completed', 'error', 'aborted'].includes(r.status as string),
|
||||
);
|
||||
});
|
||||
await waitFor(() => closeCalls > 0, { timeoutMs: 5_000 });
|
||||
} finally {
|
||||
await cleanup();
|
||||
}
|
||||
}
|
||||
|
||||
// Tool names the provider actually received for a recorded step (activeTools
|
||||
// filters this list, so it reflects what was active that step).
|
||||
const toolNames = (call: any): string[] =>
|
||||
((call?.tools ?? []) as any[]).map((t) => t?.name).filter(Boolean);
|
||||
|
||||
// A model that, on step 1, calls loadTools(['createPage']); on step 2, answers.
|
||||
function loadThenAnswerModel(): MockLanguageModelV3 {
|
||||
let step = 0;
|
||||
return new MockLanguageModelV3({
|
||||
doStream: async () => {
|
||||
const n = step++;
|
||||
if (n === 0) {
|
||||
return {
|
||||
stream: convertArrayToReadableStream([
|
||||
{ type: 'stream-start', warnings: [] },
|
||||
{
|
||||
type: 'tool-call',
|
||||
toolCallId: 'lt1',
|
||||
toolName: 'loadTools',
|
||||
input: JSON.stringify({ names: ['createPage'] }),
|
||||
},
|
||||
{
|
||||
type: 'finish',
|
||||
finishReason: 'tool-calls',
|
||||
usage: { inputTokens: 5, outputTokens: 3, totalTokens: 8 },
|
||||
},
|
||||
] as any),
|
||||
};
|
||||
}
|
||||
return { stream: successStream() };
|
||||
},
|
||||
} as any);
|
||||
}
|
||||
|
||||
it('activates a deferred tool for the SAME turn, and a NEW turn starts cold (no leak)', async () => {
|
||||
const chatId = (await createChat(db, { workspaceId, creatorId: userId })).id;
|
||||
|
||||
// --- Turn 1: loadTools(createPage) on step 1, then answer on step 2. ---
|
||||
const model1 = loadThenAnswerModel();
|
||||
await runDeferredTurn(model1, chatId, {
|
||||
chatId,
|
||||
messages: [userUiMessage('Make me a page')],
|
||||
});
|
||||
|
||||
// The turn ran at least two steps (the load round-trip + the answer).
|
||||
expect(model1.doStreamCalls.length).toBeGreaterThanOrEqual(2);
|
||||
const step1Tools = toolNames(model1.doStreamCalls[0]);
|
||||
const step2Tools = toolNames(model1.doStreamCalls[1]);
|
||||
|
||||
// Step 1 starts cold: CORE tools + the loadTools meta-tool are active, but
|
||||
// the deferred createPage is NOT yet.
|
||||
expect(step1Tools).toContain('loadTools');
|
||||
expect(step1Tools).toContain('searchPages'); // a CORE tool, always active
|
||||
expect(step1Tools).not.toContain('createPage');
|
||||
// Step 2 of the SAME turn sees the just-activated deferred tool.
|
||||
expect(step2Tools).toContain('createPage');
|
||||
|
||||
// --- Turn 2 on the SAME chat: must start cold again. ---
|
||||
const model2 = new MockLanguageModelV3({
|
||||
doStream: async () => ({ stream: successStream() }),
|
||||
} as any);
|
||||
await runDeferredTurn(model2, chatId, {
|
||||
chatId,
|
||||
messages: [userUiMessage('And another thing')],
|
||||
});
|
||||
|
||||
const nextTurnFirstStep = toolNames(model2.doStreamCalls[0]);
|
||||
expect(nextTurnFirstStep).toContain('loadTools');
|
||||
// The activated set is per-turn: the prior turn's createPage did NOT leak,
|
||||
// so the fresh turn's first step sees it deferred again.
|
||||
expect(nextTurnFirstStep).not.toContain('createPage');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -31,6 +31,22 @@ export interface SharedToolSpec {
|
||||
inAppKey: string;
|
||||
/** Single canonical model-facing description used by both layers. */
|
||||
description: string;
|
||||
/**
|
||||
* Deferred-tool tier for the IN-APP agent (#332). 'core' tools are always
|
||||
* active; 'deferred' tools are hidden behind the <tool_catalog> and loaded on
|
||||
* demand via the loadTools meta-tool. This is an IN-APP concern only: the
|
||||
* standalone /mcp server ignores this field and registers every tool normally
|
||||
* (registerShared in index.ts reads mcpName/description/buildShape only).
|
||||
*/
|
||||
tier: 'core' | 'deferred';
|
||||
/**
|
||||
* Hand-written one-liner "name — purpose" shown in the in-app agent's
|
||||
* <tool_catalog> for a DEFERRED tool (#332). Deliberately NOT derived from the
|
||||
* description's first sentence — a concise, accurate purpose line. Present on
|
||||
* every spec (core tools too) for uniformity; only deferred ones are rendered.
|
||||
* Inert for the external /mcp server.
|
||||
*/
|
||||
catalogLine: string;
|
||||
/**
|
||||
* Builds the tool's input schema as a plain object of zod fields (a
|
||||
* ZodRawShape). Called with the consumer's own zod namespace. Omitted for
|
||||
@@ -47,6 +63,8 @@ export const SHARED_TOOL_SPECS = {
|
||||
mcpName: 'get_workspace',
|
||||
inAppKey: 'getWorkspace',
|
||||
description: 'Fetch metadata about the current workspace (name, settings).',
|
||||
tier: 'core',
|
||||
catalogLine: 'getWorkspace — fetch current workspace metadata (name, settings).',
|
||||
},
|
||||
|
||||
listSpaces: {
|
||||
@@ -55,6 +73,8 @@ export const SHARED_TOOL_SPECS = {
|
||||
description:
|
||||
'List the spaces the current user can access. Returns the array of ' +
|
||||
'spaces (id, name, slug, ...).',
|
||||
tier: 'core',
|
||||
catalogLine: 'listSpaces — list the spaces the user can access (id, name, slug).',
|
||||
},
|
||||
|
||||
listShares: {
|
||||
@@ -62,6 +82,8 @@ export const SHARED_TOOL_SPECS = {
|
||||
inAppKey: 'listShares',
|
||||
description:
|
||||
'List all public shares in the workspace with page titles and public URLs.',
|
||||
tier: 'deferred',
|
||||
catalogLine: 'listShares — list all public shares in the workspace with their URLs.',
|
||||
},
|
||||
|
||||
// --- single-pageId read tools ---
|
||||
@@ -74,6 +96,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'includes block ids, callouts, tables, link/image attributes) plus the ' +
|
||||
'slugId used in URLs. Use the block ids it returns to make precise ' +
|
||||
'structural edits or surgical text edits without resending the page.',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"getPageJson — get a page's raw ProseMirror JSON (lossless, with block ids).",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
}),
|
||||
@@ -88,6 +113,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'count) WITHOUT the full document body. Use it to locate sections/tables ' +
|
||||
'and grab block ids cheaply before fetching, patching or inserting ' +
|
||||
'individual blocks.',
|
||||
tier: 'core',
|
||||
catalogLine:
|
||||
"getOutline — compact outline of a page's top-level blocks with their ids.",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
}),
|
||||
@@ -104,6 +132,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'outline or page-JSON view (works for headings/paragraphs/callouts/images), OR ' +
|
||||
'`#<index>` to fetch a top-level block by its outline index — use the ' +
|
||||
'`#<index>` form for tables/rows/cells, which carry no id.',
|
||||
tier: 'core',
|
||||
catalogLine:
|
||||
"getNode — fetch one block's ProseMirror subtree by block id or #index.",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
nodeId: z.string().min(1),
|
||||
@@ -137,6 +168,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'caseSensitive:true to match case. Ideal for systematic ' +
|
||||
'editorial sweeps (unquoted "ё", straight quotes, "т.е.", stray units). An ' +
|
||||
'invalid regex or an empty query returns a clear error to fix.',
|
||||
tier: 'core',
|
||||
catalogLine:
|
||||
'searchInPage — find every occurrence of a string/regex inside one page, with locations.',
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1).describe('ID of the page to search'),
|
||||
query: z
|
||||
@@ -172,6 +206,8 @@ export const SHARED_TOOL_SPECS = {
|
||||
description:
|
||||
'Remove a single block by its attrs.id (from the page outline or ' +
|
||||
'page-JSON view) WITHOUT resending the whole document.',
|
||||
tier: 'deferred',
|
||||
catalogLine: 'deleteNode — remove a single content block by its block id.',
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
nodeId: z.string().min(1),
|
||||
@@ -203,6 +239,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'JSON object or a JSON string (both accepted). Cheaper and safer than ' +
|
||||
'replacing the whole document for one-block structural edits. Reversible: ' +
|
||||
'the previous version is kept in page history.',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'patchNode — replace one block with a new ProseMirror node, keeping its id.',
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1).describe('ID of the page containing the block'),
|
||||
nodeId: z
|
||||
@@ -245,6 +284,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'[{"type":"text","text":"Title"}]}. Bold is a mark: ' +
|
||||
'{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node may be a ' +
|
||||
'JSON object or a JSON string (both accepted). Reversible via page history.',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'insertNode — insert a block before/after an anchor, or append at the end.',
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
node: z
|
||||
@@ -278,6 +320,8 @@ export const SHARED_TOOL_SPECS = {
|
||||
mcpName: 'unshare_page',
|
||||
inAppKey: 'unsharePage',
|
||||
description: 'Remove the public share of a page (revokes the public URL).',
|
||||
tier: 'deferred',
|
||||
catalogLine: "unsharePage — revoke a page's public share (removes the public URL).",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1).describe('ID of the page to unshare'),
|
||||
}),
|
||||
@@ -295,6 +339,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
"`from`/`to` each accept a historyId, or null/'current' for the page's " +
|
||||
'current content (defaults: from=current, to=current — pass a historyId ' +
|
||||
'from the page-history list to compare against the live page).',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'diffPageVersions — diff two page versions and return the change set + summary.',
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
from: z
|
||||
@@ -315,6 +362,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
"List a page's saved versions (Docmost auto-snapshots on every save), " +
|
||||
'newest first, cursor-paginated. Returns { items, nextCursor }; each ' +
|
||||
"item's id is the historyId to pass to the page diff or restore tools.",
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"listPageHistory — list a page's saved versions (newest first, paginated).",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
cursor: z
|
||||
@@ -332,6 +382,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'as the page\'s current content (Docmost has no restore endpoint, so ' +
|
||||
'this creates a NEW history snapshot — the restore is itself revertible). ' +
|
||||
'Get the historyId from the page-history list.',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'restorePageVersion — restore a page to a saved history version (revertible).',
|
||||
buildShape: (z) => ({
|
||||
historyId: z.string().min(1),
|
||||
}),
|
||||
@@ -349,6 +402,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'thread records are NOT created/updated/deleted on the server by this ' +
|
||||
'tool — only the page body + inline comment marks are written; manage ' +
|
||||
'comment threads via the comment tools/UI.',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"importPageMarkdown — replace a page's content from exported Docmost Markdown.",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
markdown: z.string().min(1),
|
||||
@@ -365,6 +421,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'entirely server-side — the document is NOT sent through the model. The ' +
|
||||
'target keeps its own title and slug; only its body is replaced. Ideal ' +
|
||||
"for 'make page A's content equal to B' or 'replace A with B but keep A's URL'.",
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
"copyPageContent — replace one page's body with a copy of another page's body.",
|
||||
buildShape: (z) => ({
|
||||
sourcePageId: z.string().min(1).describe('Page to copy content FROM'),
|
||||
targetPageId: z
|
||||
@@ -402,6 +461,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'page JSON and use a structural node patch/update to set its marks. ' +
|
||||
'Examples: edits:[{find:"teh",replace:"the"}]; edits:[{find:"Hello ' +
|
||||
'world",replace:"Hello there"}] (crosses a bold boundary).',
|
||||
tier: 'core',
|
||||
catalogLine:
|
||||
"editPageText — surgical find/replace of plain text in a page, preserving ids/marks.",
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().describe('ID of the page to edit'),
|
||||
edits: z
|
||||
@@ -440,6 +502,9 @@ export const SHARED_TOOL_SPECS = {
|
||||
'server instance that created it: in a multi-replica deployment without ' +
|
||||
'sticky sessions a blob stored on one instance is not retrievable via the ' +
|
||||
'sandbox URL on another (it 404s like an expired one).',
|
||||
tier: 'deferred',
|
||||
catalogLine:
|
||||
'stashPage — serialize a whole page to a short anonymous URL without loading its body.',
|
||||
buildShape: (z) => ({
|
||||
pageId: z.string().min(1),
|
||||
}),
|
||||
|
||||
@@ -635,17 +635,13 @@ const Attachment = Node.create({
|
||||
},
|
||||
name: {
|
||||
default: null,
|
||||
// Empty-string-vs-absent idempotency (GS-EDIT-REVERT class): "" -> default.
|
||||
parseHTML: (el: HTMLElement) =>
|
||||
el.getAttribute("data-attachment-name") || null,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-attachment-name"),
|
||||
renderHTML: (attrs: Record<string, any>) =>
|
||||
attrs.name ? { "data-attachment-name": attrs.name } : {},
|
||||
},
|
||||
mime: {
|
||||
default: null,
|
||||
// Empty-string-vs-absent idempotency (GS-EDIT-REVERT class): "" -> default.
|
||||
parseHTML: (el: HTMLElement) =>
|
||||
el.getAttribute("data-attachment-mime") || null,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-attachment-mime"),
|
||||
renderHTML: (attrs: Record<string, any>) =>
|
||||
attrs.mime ? { "data-attachment-mime": attrs.mime } : {},
|
||||
},
|
||||
@@ -693,10 +689,7 @@ const Video = Node.create({
|
||||
},
|
||||
alt: {
|
||||
default: null,
|
||||
// Empty-string-vs-absent idempotency: coerce "" back to the default so a
|
||||
// stray empty `aria-label` never materializes `alt: ""` on a video stored
|
||||
// with no alt (same GS-EDIT-REVERT class as the image `alt` fix).
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("aria-label") || null,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("aria-label"),
|
||||
renderHTML: (attrs: Record<string, any>) =>
|
||||
attrs.alt ? { "aria-label": attrs.alt } : {},
|
||||
},
|
||||
@@ -871,15 +864,13 @@ const diagramAttributes = () => ({
|
||||
},
|
||||
title: {
|
||||
default: null,
|
||||
// Empty-string-vs-absent idempotency (GS-EDIT-REVERT class): "" -> default.
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-title") || null,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-title"),
|
||||
renderHTML: (attrs: Record<string, any>) =>
|
||||
attrs.title ? { "data-title": attrs.title } : {},
|
||||
},
|
||||
alt: {
|
||||
default: null,
|
||||
// Empty-string-vs-absent idempotency (GS-EDIT-REVERT class): "" -> default.
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-alt") || null,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-alt"),
|
||||
renderHTML: (attrs: Record<string, any>) =>
|
||||
attrs.alt ? { "data-alt": attrs.alt } : {},
|
||||
},
|
||||
@@ -1115,8 +1106,7 @@ const Pdf = Node.create({
|
||||
},
|
||||
name: {
|
||||
default: null,
|
||||
// Empty-string-vs-absent idempotency (GS-EDIT-REVERT class): "" -> default.
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-name") || null,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("data-name"),
|
||||
renderHTML: (attrs: Record<string, any>) =>
|
||||
attrs.name ? { "data-name": attrs.name } : {},
|
||||
},
|
||||
@@ -1501,29 +1491,6 @@ export const docmostExtensions = [
|
||||
...parent.height,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("height"),
|
||||
},
|
||||
// Empty-string-vs-absent idempotency (GS-EDIT-REVERT class). `marked`
|
||||
// renders `` as `<img alt="">`, so the stock Image `alt`
|
||||
// parseHTML (`getAttribute("alt")`) materializes `alt: ""` on an image
|
||||
// that was stored with NO alt (attr absent). That is a false diff against
|
||||
// the editor-stored form (a no-alt image has alt ABSENT, not ""), so a
|
||||
// git-sync / ai-chat touch of a page with a plain image produced phantom
|
||||
// churn. Coerce an empty string back to the attr's default (null) so the
|
||||
// import is idempotent. A real alt survives verbatim (`|| undefined` keeps
|
||||
// the truthy value; the default fills the empty case). `title` is coerced
|
||||
// the same way for the whole class, even though `marked` does not
|
||||
// currently emit `title=""` — defence in depth against any path that does.
|
||||
// NOTE: this DIVERGES from editor-ext's literal image `alt` parseHTML
|
||||
// (`getAttribute("alt")`, which returns "" verbatim), but CONVERGES on
|
||||
// editor-ext's real STORED shape: an editor image inserted without alt
|
||||
// renders with no `alt` attribute and re-parses as absent, never "".
|
||||
alt: {
|
||||
...parent.alt,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("alt") || null,
|
||||
},
|
||||
title: {
|
||||
...parent.title,
|
||||
parseHTML: (el: HTMLElement) => el.getAttribute("title") || null,
|
||||
},
|
||||
};
|
||||
},
|
||||
}).configure({ inline: false }),
|
||||
|
||||
@@ -1,443 +0,0 @@
|
||||
/**
|
||||
* Reusable round-trip-STABILITY matrix helper (fixtures-first).
|
||||
*
|
||||
* A single stored node authored WITHOUT a given string attribute (attr
|
||||
* absent / undefined) must not gain a phantom EMPTY-STRING value after a
|
||||
* markdown round-trip — the "empty-string-vs-absent" churn class. This helper,
|
||||
* given a node spec, drives a matrix of attribute combinations through the REAL
|
||||
* converter (`convertProseMirrorToMarkdown` -> `markdownToProseMirror`) and
|
||||
* asserts byte-stability on two contours:
|
||||
*
|
||||
* 1. RAW round-trip: for the node under test, every attribute the round-trip
|
||||
* materializes must equal what the INPUT authored — an authored attr keeps
|
||||
* its value, an ABSENT attr may only reappear at its SCHEMA DEFAULT. If an
|
||||
* absent attr comes back as a NON-default value (e.g. `alt: ""` where the
|
||||
* default is `null`), that is an instability and is reported precisely as
|
||||
* `type.attr: absent -> "<got>"`. This is the contour git-sync / stored
|
||||
* JSON diffs on, so masking it only in `canonicalize` would leave the noise.
|
||||
*
|
||||
* 2. CANONICAL round-trip: `canonicalizeContent(original)` must deep-equal
|
||||
* `canonicalizeContent(roundtrip)` (a second, semantic contour).
|
||||
*
|
||||
* The ONLY normalization the helper treats as allowed (not an instability) is
|
||||
* the DOCUMENTED numeric width/height/size/aspectRatio -> string coercion the
|
||||
* converter performs on purpose (a stored numeric `640` re-parses via
|
||||
* `getAttribute` as the string `"640"`). It is encoded here as an explicit
|
||||
* per-spec `numericStringAttrs` set applied to BOTH contours, NOT a silent skip.
|
||||
*
|
||||
* The helper is node-type agnostic: image and the whole media family share the
|
||||
* `align !== "center"` predicate + `<!--name {…}-->` comment machinery, so one
|
||||
* matrix guards the shared class.
|
||||
*/
|
||||
import { getSchema } from "@tiptap/core";
|
||||
import {
|
||||
convertProseMirrorToMarkdown,
|
||||
markdownToProseMirror,
|
||||
canonicalizeContent,
|
||||
docmostExtensions,
|
||||
} from "../src/lib/index.js";
|
||||
import { firstDivergence } from "./roundtrip-helpers.js";
|
||||
|
||||
/** One attribute's two probe values. */
|
||||
export interface AttrMatrixEntry {
|
||||
/** Attribute name on the node. */
|
||||
attr: string;
|
||||
/**
|
||||
* The "default" pick. `undefined` means the attribute is OMITTED entirely
|
||||
* (the absent case — the one that can materialize an empty string on import).
|
||||
* A concrete value is authored verbatim.
|
||||
*/
|
||||
default: unknown;
|
||||
/** A representative NON-default value to exercise (must survive verbatim). */
|
||||
nonDefault: unknown;
|
||||
/**
|
||||
* Marks the attr as a member of the EMPTY-STRING class the fix targets: a
|
||||
* string attr whose schema default is `null`/absent and whose parseHTML
|
||||
* coerces `"" -> default` (image/drawio `alt`+`title`, video `alt` via
|
||||
* aria-label, pdf/attachment `name`, attachment `mime`). Set true to also
|
||||
* drive the THIRD-STATE convergence case (see runConvergenceCase) for this
|
||||
* attr. Attrs whose default is NOT null (e.g. embed `provider`, default "")
|
||||
* or that are not `""`-coerced (control attrs) are left unset.
|
||||
*/
|
||||
emptyStringClass?: boolean;
|
||||
}
|
||||
|
||||
/** A node type + the attribute matrix to sweep for it. */
|
||||
export interface NodeStabilitySpec {
|
||||
/** Node type (e.g. "image", "video"). */
|
||||
type: string;
|
||||
/** Attributes always present on the node (e.g. `{ src: "/i.png" }`). */
|
||||
baseAttrs?: Record<string, unknown>;
|
||||
/** Attributes to sweep at default and non-default. */
|
||||
attrMatrix: AttrMatrixEntry[];
|
||||
/**
|
||||
* Attributes whose numeric -> string coercion on round-trip is DOCUMENTED and
|
||||
* intentional; compared modulo `String(x)` on both sides. Defaults to the
|
||||
* converter's known sizing set.
|
||||
*/
|
||||
numericStringAttrs?: string[];
|
||||
}
|
||||
|
||||
/** A single unstable finding, legible enough to tie a gate-lock to. */
|
||||
export interface Instability {
|
||||
type: string;
|
||||
attr: string;
|
||||
/** What the input authored: the literal value, or the ABSENT sentinel. */
|
||||
authored: unknown | typeof ABSENT;
|
||||
/** What the round-trip produced. */
|
||||
got: unknown;
|
||||
/** What a stable round-trip should have produced (authored value or default). */
|
||||
expected: unknown;
|
||||
}
|
||||
|
||||
/** One matrix cell's result. */
|
||||
export interface ComboResult {
|
||||
label: string;
|
||||
authored: Record<string, unknown>;
|
||||
/** RAW-contour instabilities on the node under test. */
|
||||
raw: Instability[];
|
||||
/** CANONICAL-contour divergence (path + values) or null when equal. */
|
||||
canonical: { path: string; a: unknown; b: unknown } | null;
|
||||
/** True when the node type failed to round-trip at all (structural loss). */
|
||||
missing: boolean;
|
||||
md: string;
|
||||
}
|
||||
|
||||
/** Whole-matrix report for one node spec. */
|
||||
export interface MatrixReport {
|
||||
type: string;
|
||||
combos: ComboResult[];
|
||||
}
|
||||
|
||||
/** Sentinel marking an attribute the input did NOT author. */
|
||||
export const ABSENT = Symbol("ABSENT");
|
||||
|
||||
const DEFAULT_NUMERIC_STRING_ATTRS = [
|
||||
"width",
|
||||
"height",
|
||||
"size",
|
||||
"aspectRatio",
|
||||
];
|
||||
|
||||
// The ProseMirror schema the converter targets — its attribute `default`s are
|
||||
// the authoritative "what an absent attr should re-materialize as" oracle.
|
||||
const schema = getSchema(docmostExtensions);
|
||||
|
||||
/** Read the schema default for every attribute of a node type. */
|
||||
function schemaDefaults(type: string): Record<string, unknown> {
|
||||
const specAttrs = (schema.nodes[type]?.spec?.attrs ?? {}) as Record<
|
||||
string,
|
||||
{ default: unknown }
|
||||
>;
|
||||
const out: Record<string, unknown> = {};
|
||||
for (const [k, v] of Object.entries(specAttrs)) out[k] = v.default;
|
||||
return out;
|
||||
}
|
||||
|
||||
/** Find the first node of a given type anywhere in a PM doc tree. */
|
||||
function findFirst(node: any, type: string): any {
|
||||
if (node && node.type === type) return node;
|
||||
for (const child of node?.content ?? []) {
|
||||
const hit = findFirst(child, type);
|
||||
if (hit) return hit;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/** Coerce a scalar for the documented numeric->string comparison. */
|
||||
const numStr = (x: unknown): unknown => (x == null ? x : String(x));
|
||||
|
||||
/**
|
||||
* Enumerate the cartesian product of the matrix: every attribute independently
|
||||
* at its default (index 0) or non-default (index 1) pick. The all-default
|
||||
* corner is included (the baseline). Small by construction (2^N over a handful
|
||||
* of at-risk string attrs).
|
||||
*/
|
||||
function enumerateCombos(matrix: AttrMatrixEntry[]): number[][] {
|
||||
let combos: number[][] = [[]];
|
||||
for (let i = 0; i < matrix.length; i++) {
|
||||
const next: number[][] = [];
|
||||
for (const c of combos) {
|
||||
next.push([...c, 0]);
|
||||
next.push([...c, 1]);
|
||||
}
|
||||
combos = next;
|
||||
}
|
||||
return combos;
|
||||
}
|
||||
|
||||
/** Build the authored attrs for one combo pick vector. */
|
||||
function authoredAttrs(
|
||||
spec: NodeStabilitySpec,
|
||||
picks: number[],
|
||||
): Record<string, unknown> {
|
||||
const attrs: Record<string, unknown> = { ...(spec.baseAttrs ?? {}) };
|
||||
spec.attrMatrix.forEach((entry, i) => {
|
||||
if (picks[i] === 1) {
|
||||
attrs[entry.attr] = entry.nonDefault;
|
||||
} else if (entry.default !== undefined) {
|
||||
attrs[entry.attr] = entry.default;
|
||||
}
|
||||
// default === undefined -> OMIT the attr entirely (the absent case).
|
||||
});
|
||||
return attrs;
|
||||
}
|
||||
|
||||
/** Human-readable label for a combo (which attrs are at non-default). */
|
||||
function comboLabel(spec: NodeStabilitySpec, picks: number[]): string {
|
||||
const on = spec.attrMatrix
|
||||
.filter((_, i) => picks[i] === 1)
|
||||
.map((e) => e.attr);
|
||||
return on.length === 0 ? "<all-default>" : on.join("+");
|
||||
}
|
||||
|
||||
/**
|
||||
* Run the full stability matrix for one node spec and return a structured
|
||||
* report (does NOT throw — the caller asserts, so a failure can print the whole
|
||||
* report). Every combo runs the real export->import pipeline once.
|
||||
*/
|
||||
export async function runStabilityMatrix(
|
||||
spec: NodeStabilitySpec,
|
||||
): Promise<MatrixReport> {
|
||||
const numericStringAttrs = new Set(
|
||||
spec.numericStringAttrs ?? DEFAULT_NUMERIC_STRING_ATTRS,
|
||||
);
|
||||
const defaults = schemaDefaults(spec.type);
|
||||
const combos: ComboResult[] = [];
|
||||
|
||||
for (const picks of enumerateCombos(spec.attrMatrix)) {
|
||||
const authored = authoredAttrs(spec, picks);
|
||||
const doc = { type: "doc", content: [{ type: spec.type, attrs: authored }] };
|
||||
const md = convertProseMirrorToMarkdown(doc);
|
||||
const rt = await markdownToProseMirror(md);
|
||||
const node = findFirst(rt, spec.type);
|
||||
|
||||
const result: ComboResult = {
|
||||
label: comboLabel(spec, picks),
|
||||
authored,
|
||||
raw: [],
|
||||
canonical: null,
|
||||
missing: node == null,
|
||||
md,
|
||||
};
|
||||
|
||||
if (node != null) {
|
||||
// RAW contour: every materialized attr must equal the authored value, or
|
||||
// (for an absent attr) the schema default — modulo the documented numeric
|
||||
// string coercion.
|
||||
const rtAttrs = (node.attrs ?? {}) as Record<string, unknown>;
|
||||
for (const key of Object.keys(rtAttrs)) {
|
||||
const authoredHas = Object.prototype.hasOwnProperty.call(authored, key);
|
||||
const expected = authoredHas ? authored[key] : defaults[key];
|
||||
let got = rtAttrs[key];
|
||||
let exp = expected;
|
||||
if (numericStringAttrs.has(key)) {
|
||||
got = numStr(got);
|
||||
exp = numStr(exp);
|
||||
}
|
||||
if (firstDivergence(got, exp) !== null) {
|
||||
result.raw.push({
|
||||
type: spec.type,
|
||||
attr: key,
|
||||
authored: authoredHas ? authored[key] : ABSENT,
|
||||
got: rtAttrs[key],
|
||||
expected,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// CANONICAL contour: canonical forms deep-equal, modulo the same numeric
|
||||
// string coercion (applied to both trees so a documented coercion is not
|
||||
// counted as a divergence).
|
||||
const ca = normalizeNumeric(canonicalizeContent(doc), numericStringAttrs);
|
||||
const cb = normalizeNumeric(canonicalizeContent(rt), numericStringAttrs);
|
||||
result.canonical = firstDivergence(ca, cb);
|
||||
}
|
||||
|
||||
combos.push(result);
|
||||
}
|
||||
|
||||
return { type: spec.type, combos };
|
||||
}
|
||||
|
||||
/**
|
||||
* Deep-copy a canonical tree, coercing the documented numeric->string attrs to
|
||||
* their string form so an intentional `640 -> "640"` coercion is not reported
|
||||
* as a canonical divergence. Only touches the listed attribute keys.
|
||||
*/
|
||||
function normalizeNumeric(node: any, attrs: Set<string>): any {
|
||||
if (Array.isArray(node)) return node.map((n) => normalizeNumeric(n, attrs));
|
||||
if (node === null || typeof node !== "object") return node;
|
||||
const out: Record<string, unknown> = {};
|
||||
for (const key of Object.keys(node)) {
|
||||
if (key === "attrs" && node.attrs && typeof node.attrs === "object") {
|
||||
const a: Record<string, unknown> = {};
|
||||
for (const [k, v] of Object.entries(node.attrs)) {
|
||||
a[k] = attrs.has(k) ? numStr(v) : v;
|
||||
}
|
||||
out.attrs = a;
|
||||
} else {
|
||||
out[key] = normalizeNumeric(node[key], attrs);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/** Flatten a report to just its unstable combos (for a terse assertion). */
|
||||
export function unstableCombos(report: MatrixReport): ComboResult[] {
|
||||
return report.combos.filter(
|
||||
(c) => c.missing || c.raw.length > 0 || c.canonical !== null,
|
||||
);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// THIRD STATE: an EXPLICITLY-STORED empty string on a string attr.
|
||||
//
|
||||
// The matrix above sweeps TWO states per string attr: absent/default and a
|
||||
// non-default value — and asserts FIRST-pass byte-stability for both. There is
|
||||
// a third, degenerate state the matrix does NOT cover: the attr stored as a
|
||||
// LITERAL `""`. This is DISTINCT from "the node never had the attr": a user
|
||||
// types an alt in the editor, then deletes it, and Tiptap's
|
||||
// `updateAttributes({ alt: "" })` persists a literal `alt: ""` in the stored
|
||||
// JSON. There is no absent-vs-"" distinction in the DOM once serialized, so the
|
||||
// fix's `getAttribute("alt") || null` coercion canonicalizes BOTH to the
|
||||
// default (`null`).
|
||||
//
|
||||
// Consequence — and this is CORRECT, not a bug: a doc carrying an explicit `""`
|
||||
// converges to the default on the FIRST round-trip (a ONE-TIME diff: `"" ->
|
||||
// null`), then is byte-stable from the SECOND round-trip on (idempotent). So
|
||||
// this state must be pinned with a DIFFERENT contract than the matrix's:
|
||||
// - do NOT assert first-pass byte-stability (the first pass legitimately
|
||||
// changes `""` -> default), and
|
||||
// - DO assert the first pass converges to the default AND the second pass is
|
||||
// idempotent (rt2 deep-equals rt1).
|
||||
//
|
||||
// A future sync/QA pass diffing stored pages will see this one-time `"" -> null`
|
||||
// normalization exactly once per affected node; it is the converter canon, not
|
||||
// corruption, and must not be flagged as data loss.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/** Result of the third-state ("explicit empty string") convergence probe. */
|
||||
export interface ConvergenceResult {
|
||||
type: string;
|
||||
attr: string;
|
||||
/** The schema default the attr must converge to on pass 1 (null / absent). */
|
||||
expectedDefault: unknown;
|
||||
/** rt1's materialized value for the attr — must equal `expectedDefault`. */
|
||||
firstPassValue: unknown;
|
||||
/** True when the node round-tripped AND rt1 converged the attr to default. */
|
||||
convergedToDefault: boolean;
|
||||
/** rt1-vs-rt2 divergence; MUST be null (idempotent from pass 2 on). */
|
||||
secondPassDivergence: { path: string; a: unknown; b: unknown } | null;
|
||||
/** True when the node type failed to round-trip at all (structural loss). */
|
||||
missing: boolean;
|
||||
}
|
||||
|
||||
/** Round-trip a full PM doc through the real converter once. */
|
||||
async function roundtripDoc(doc: any): Promise<any> {
|
||||
return markdownToProseMirror(convertProseMirrorToMarkdown(doc));
|
||||
}
|
||||
|
||||
/**
|
||||
* Third-state convergence probe for one string attr of the empty-string class.
|
||||
*
|
||||
* (a) builds a doc with the attr EXPLICITLY set to `""` (baseAttrs + `""`),
|
||||
* (b) rt1 = roundtrip(doc); asserts rt1's attr equals the schema default — the
|
||||
* documented ONE-TIME `"" -> default` normalization (NOT byte-stable vs the
|
||||
* `""` input, so first-pass stability is deliberately NOT asserted here),
|
||||
* (c) rt2 = roundtrip(rt1); asserts rt2 deep-equals rt1 — idempotent from the
|
||||
* second round-trip on.
|
||||
*
|
||||
* Returns a structured result (does NOT throw) so the caller can assert and
|
||||
* print. Reusable across the whole node family: drive it for every attr flagged
|
||||
* `emptyStringClass` on every spec (see convergenceCasesFor / the test driver).
|
||||
*/
|
||||
export async function runConvergenceCase(
|
||||
spec: NodeStabilitySpec,
|
||||
attr: string,
|
||||
): Promise<ConvergenceResult> {
|
||||
const expectedDefault = schemaDefaults(spec.type)[attr];
|
||||
|
||||
// (a) The degenerate third state: attr persisted as a LITERAL "".
|
||||
const authored = { ...(spec.baseAttrs ?? {}), [attr]: "" };
|
||||
const doc = { type: "doc", content: [{ type: spec.type, attrs: authored }] };
|
||||
|
||||
// (b) First round-trip: "" must normalize to the default (a one-time diff).
|
||||
const rt1 = await roundtripDoc(doc);
|
||||
const node1 = findFirst(rt1, spec.type);
|
||||
const firstPassValue = node1?.attrs?.[attr];
|
||||
const convergedToDefault =
|
||||
node1 != null && firstDivergence(firstPassValue, expectedDefault) === null;
|
||||
|
||||
// (c) Second round-trip: must be byte-stable (rt2 deep-equals rt1). We compare
|
||||
// the WHOLE docs — both are converter OUTPUTS already in the same materialized
|
||||
// form (numeric attrs are strings on both sides), so no numeric normalization
|
||||
// is needed here, unlike the raw/canonical contours above.
|
||||
const rt2 = node1 != null ? await roundtripDoc(rt1) : rt1;
|
||||
const secondPassDivergence =
|
||||
node1 != null ? firstDivergence(rt1, rt2) : null;
|
||||
|
||||
return {
|
||||
type: spec.type,
|
||||
attr,
|
||||
expectedDefault,
|
||||
firstPassValue,
|
||||
convergedToDefault,
|
||||
secondPassDivergence,
|
||||
missing: node1 == null,
|
||||
};
|
||||
}
|
||||
|
||||
/** The attrs of a spec flagged as members of the empty-string class. */
|
||||
export function convergenceCasesFor(spec: NodeStabilitySpec): string[] {
|
||||
return spec.attrMatrix
|
||||
.filter((e) => e.emptyStringClass)
|
||||
.map((e) => e.attr);
|
||||
}
|
||||
|
||||
/** True when a convergence result honours the "converges once, then stable" contract. */
|
||||
export function convergenceOk(r: ConvergenceResult): boolean {
|
||||
return !r.missing && r.convergedToDefault && r.secondPassDivergence === null;
|
||||
}
|
||||
|
||||
/** Render a convergence result as a legible one-liner for a failed assertion. */
|
||||
export function formatConvergence(r: ConvergenceResult): string {
|
||||
if (r.missing) return `${r.type}.${r.attr}: DID-NOT-ROUND-TRIP`;
|
||||
const parts: string[] = [];
|
||||
if (!r.convergedToDefault) {
|
||||
parts.push(
|
||||
`pass1 did NOT converge: got ${JSON.stringify(r.firstPassValue)} (expected default ${JSON.stringify(r.expectedDefault)})`,
|
||||
);
|
||||
}
|
||||
if (r.secondPassDivergence) {
|
||||
parts.push(
|
||||
`pass2 NOT idempotent @ ${r.secondPassDivergence.path}: ${JSON.stringify(r.secondPassDivergence.a)} vs ${JSON.stringify(r.secondPassDivergence.b)}`,
|
||||
);
|
||||
}
|
||||
const status = parts.length === 0 ? "converges-once-then-stable" : parts.join("; ");
|
||||
return `${r.type}.${r.attr}: ${status}`;
|
||||
}
|
||||
|
||||
/** Render a report as a legible multi-line string for a failed assertion. */
|
||||
export function formatReport(report: MatrixReport): string {
|
||||
const lines: string[] = [`node "${report.type}":`];
|
||||
for (const c of report.combos) {
|
||||
const flags: string[] = [];
|
||||
if (c.missing) flags.push("DID-NOT-ROUND-TRIP");
|
||||
for (const i of c.raw) {
|
||||
const authored =
|
||||
i.authored === ABSENT ? "absent" : JSON.stringify(i.authored);
|
||||
flags.push(
|
||||
`RAW ${i.type}.${i.attr}: ${authored} -> ${JSON.stringify(i.got)} (expected ${JSON.stringify(i.expected)})`,
|
||||
);
|
||||
}
|
||||
if (c.canonical) {
|
||||
flags.push(
|
||||
`CANON @ ${c.canonical.path}: ${JSON.stringify(c.canonical.a)} vs ${JSON.stringify(c.canonical.b)}`,
|
||||
);
|
||||
}
|
||||
const status = flags.length === 0 ? "stable" : flags.join("; ");
|
||||
lines.push(` [${c.label}] ${status}`);
|
||||
}
|
||||
return lines.join("\n");
|
||||
}
|
||||
@@ -1,164 +0,0 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import {
|
||||
runStabilityMatrix,
|
||||
unstableCombos,
|
||||
formatReport,
|
||||
runConvergenceCase,
|
||||
convergenceCasesFor,
|
||||
convergenceOk,
|
||||
formatConvergence,
|
||||
type NodeStabilitySpec,
|
||||
} from "./roundtrip-stability.helper.js";
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Round-trip STABILITY matrix for image + the media family.
|
||||
//
|
||||
// Guards the "empty-string-vs-absent" churn class (GS-EDIT-REVERT family): a
|
||||
// stored node authored WITHOUT a string attr (alt/title/caption/aria-label/...)
|
||||
// must not gain a phantom `attr: ""` after `markdownToProseMirror(convert…)`.
|
||||
// Each spec sweeps the at-risk string attrs at DEFAULT (absent) and at a real
|
||||
// NON-default value; the helper asserts both the RAW round-trip (attrs equal the
|
||||
// input's, modulo the documented numeric width/height/size/aspectRatio -> string
|
||||
// coercion) and the CANONICAL round-trip (canonical forms deep-equal).
|
||||
//
|
||||
// The image + media family share the `align !== "center"` predicate and the
|
||||
// `<!--name {…}-->` comment machinery, so one matrix guards the shared class.
|
||||
// align is NOT part of this class (it round-trips correctly) and is not swept.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const SPECS: NodeStabilitySpec[] = [
|
||||
{
|
||||
// Image carries the most at-risk string attrs. `alt` is the one marked
|
||||
// materializes as `<img alt="">` on `` import (the real bug); title
|
||||
// and caption are covered as the same class. attachmentId is a string attr
|
||||
// that must stay absent when unset (control).
|
||||
type: "image",
|
||||
baseAttrs: { src: "/i.png" },
|
||||
attrMatrix: [
|
||||
{ attr: "alt", default: undefined, nonDefault: "a real alt text", emptyStringClass: true },
|
||||
{ attr: "title", default: undefined, nonDefault: "a real title", emptyStringClass: true },
|
||||
{ attr: "caption", default: undefined, nonDefault: "a real caption" },
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-42" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// Video's `alt` rides the `aria-label` attribute (media aria-label at risk).
|
||||
type: "video",
|
||||
baseAttrs: { src: "/v.mp4" },
|
||||
attrMatrix: [
|
||||
{ attr: "alt", default: undefined, nonDefault: "a clip", emptyStringClass: true },
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-1" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// Audio carries no alt/title; attachmentId is its only optional string attr.
|
||||
type: "audio",
|
||||
baseAttrs: { src: "/a.mp3" },
|
||||
attrMatrix: [
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-2" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// pdf: link-form media. `name` (filename) is its at-risk string attr.
|
||||
type: "pdf",
|
||||
baseAttrs: { src: "/d.pdf" },
|
||||
attrMatrix: [
|
||||
{ attr: "name", default: undefined, nonDefault: "report.pdf", emptyStringClass: true },
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-3" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// attachment: link-form media (file card). `name` + `mime` string attrs.
|
||||
type: "attachment",
|
||||
baseAttrs: { url: "/f.zip" },
|
||||
attrMatrix: [
|
||||
{ attr: "name", default: undefined, nonDefault: "bundle.zip", emptyStringClass: true },
|
||||
{ attr: "mime", default: undefined, nonDefault: "application/zip", emptyStringClass: true },
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-4" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// embed: link-form media. `provider` is its at-risk string attr (schema
|
||||
// default ""). embed's numeric width/height defaults (800/600) are a SEPARATE,
|
||||
// documented limitation OUTSIDE the empty-string class: they are not in
|
||||
// canonicalize's KNOWN_DEFAULTS, so an ABSENT width/height re-imports as the
|
||||
// 800/600 default and diverges canonically (see the note in canonicalize.ts).
|
||||
// That is canonicalize-owned and out of scope here, so we author the
|
||||
// dimensions at their defaults (as real editor embeds carry them) to keep this
|
||||
// guard focused on the empty-string/provider class.
|
||||
// provider's schema default is "" (NOT null), so a re-imported "" is the
|
||||
// correct value, not a phantom — it is outside the null-default empty-string
|
||||
// class. We author it at its "" default (the default pick) so the sweep still
|
||||
// asserts a non-default provider ("youtube") round-trips, without tripping the
|
||||
// canonicalize KNOWN_DEFAULTS gap for embed's non-null defaults.
|
||||
type: "embed",
|
||||
baseAttrs: { src: "https://example.com/x", width: 800, height: 600 },
|
||||
attrMatrix: [
|
||||
{ attr: "provider", default: "", nonDefault: "youtube" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// drawio: image-form diagram. `title` + `alt` string attrs (data-title/-alt).
|
||||
type: "drawio",
|
||||
baseAttrs: { src: "blob:drawio" },
|
||||
attrMatrix: [
|
||||
{ attr: "title", default: undefined, nonDefault: "flow chart", emptyStringClass: true },
|
||||
{ attr: "alt", default: undefined, nonDefault: "an alt", emptyStringClass: true },
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-5" },
|
||||
],
|
||||
},
|
||||
{
|
||||
// excalidraw: image-form diagram, same shared diagramAttributes set.
|
||||
type: "excalidraw",
|
||||
baseAttrs: { src: "blob:excalidraw" },
|
||||
attrMatrix: [
|
||||
{ attr: "title", default: undefined, nonDefault: "sketch", emptyStringClass: true },
|
||||
{ attr: "alt", default: undefined, nonDefault: "an alt", emptyStringClass: true },
|
||||
{ attr: "attachmentId", default: undefined, nonDefault: "att-6" },
|
||||
],
|
||||
},
|
||||
];
|
||||
|
||||
describe("round-trip stability matrix (image + media family)", () => {
|
||||
for (const spec of SPECS) {
|
||||
it(`${spec.type}: no attr materializes an empty-string / phantom value`, async () => {
|
||||
const report = await runStabilityMatrix(spec);
|
||||
const unstable = unstableCombos(report);
|
||||
// On failure, print the WHOLE matrix so which (attr, value) combos are
|
||||
// unstable is legible.
|
||||
expect(unstable, `\n${formatReport(report)}\n`).toEqual([]);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// THIRD STATE: an attr EXPLICITLY stored as a literal "" (GS-EDIT-REVERT: a user
|
||||
// typed alt/title/name/... then deleted it, so Tiptap persisted `attr: ""` — a
|
||||
// value DISTINCT from "attr was never set"). Unlike the absent case above, this
|
||||
// state is NOT first-pass byte-stable: the fix's `"" -> default` coercion is a
|
||||
// deliberate ONE-TIME normalization on the FIRST sync round-trip, stable
|
||||
// thereafter. We therefore assert a DIFFERENT contract — "converges to default
|
||||
// on pass 1, then idempotent from pass 2 on" — for every empty-string-class attr
|
||||
// across the whole node family (image/video/pdf/attachment/drawio/excalidraw).
|
||||
//
|
||||
// IMPORTANT for a future sync/QA pass: the pass-1 `"" -> null` diff is the
|
||||
// converter canon, not corruption. It appears at most once per affected node and
|
||||
// must NOT be flagged as "the converter is losing/corrupting page data".
|
||||
// ---------------------------------------------------------------------------
|
||||
describe("round-trip third state: explicit empty string converges once, then idempotent", () => {
|
||||
for (const spec of SPECS) {
|
||||
for (const attr of convergenceCasesFor(spec)) {
|
||||
it(`${spec.type}.${attr}: "" normalizes to default on pass 1, byte-stable from pass 2`, async () => {
|
||||
const r = await runConvergenceCase(spec, attr);
|
||||
// Pass 1 must converge "" -> the schema default (the one-time diff) and
|
||||
// pass 2 (roundtrip of pass-1 output) must be byte-stable. formatConvergence
|
||||
// prints exactly which half failed.
|
||||
expect(convergenceOk(r), `\n${formatConvergence(r)}\n`).toBe(true);
|
||||
// Spell the contract out explicitly so the intent is legible in the test:
|
||||
expect(r.convergedToDefault, `\n${formatConvergence(r)}\n`).toBe(true);
|
||||
expect(r.firstPassValue).toEqual(r.expectedDefault);
|
||||
expect(r.secondPassDivergence, `\n${formatConvergence(r)}\n`).toBeNull();
|
||||
});
|
||||
}
|
||||
}
|
||||
});
|
||||
Reference in New Issue
Block a user