diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 955b0ac2..3a756656 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -15,6 +15,38 @@ permissions: jobs: test: runs-on: ubuntu-latest + # Real Postgres + Redis so the server integration suite (`*.int-spec.ts`, + # behind `pnpm --filter server test:int`) runs in CI (red-team finding #7). + # Without it, cost-cap / FK-cascade / jsonb-round-trip / real-apply tests + # only ran locally, so regressions in those paths stayed green in CI. + # Postgres uses the pgvector image because migrations create vector columns + # and global-setup runs `CREATE EXTENSION vector`. Credentials/db match the + # defaults in apps/server/test/integration/db.ts + global-setup.ts + # (docmost / docmost_dev_pw, maintenance db `docmost`, redis on 6379), so no + # TEST_*_URL overrides are needed. + services: + postgres: + image: pgvector/pgvector:pg18 + env: + POSTGRES_USER: docmost + POSTGRES_PASSWORD: docmost_dev_pw + POSTGRES_DB: docmost + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U docmost" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + redis: + image: redis:7 + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 10s + --health-timeout 5s + --health-retries 5 steps: - name: Checkout uses: actions/checkout@v4 @@ -36,5 +68,12 @@ jobs: - name: Build editor-ext run: pnpm --filter @docmost/editor-ext build - - name: Run tests + - name: Run unit tests run: pnpm -r test + + # Integration suite against the real Postgres/Redis services above. Runs + # the FK-cascade, cost-cap, jsonb-round-trip and real-apply specs that the + # unit run (mocks only) cannot cover. global-setup drops/recreates the + # isolated `docmost_test` DB and migrates it to latest. + - name: Run server integration tests + run: pnpm --filter server test:int diff --git a/CHANGELOG.md b/CHANGELOG.md index 90293ba7..6c2aa9c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 OpenRouter, etc.; `openai` uses the official provider (real-OpenAI reasoning-model request shaping). Chosen explicitly rather than inferred from the base URL, since a custom URL can front real OpenAI too. (#175, #177) +- **Per-MCP-server instructions in the agent prompt.** Each external MCP server + now has an admin-authored `instructions` field ("how/when to use this server's + tools") that is injected into the agent's system prompt next to that server's + tool descriptions. Trusted text, rendered inside the prompt safety sandwich; + shown only for a server that actually connected and contributed ≥1 callable + tool. (#180) +- **Footnote multi-backlinks.** A footnote referenced more than once now shows a + back-link per reference (↩ a b c …), each scrolling to its own occurrence, like + Pandoc/Wikipedia; a single-reference footnote keeps the plain ↩. (#168) ### Changed @@ -78,6 +87,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 are nudged after a paste to refresh stale hit-testing geometry. The caret symptom is macOS-specific and was confirmed manually on macOS; the automated guard pins the DOM-order invariant, not the caret behavior itself. (#146, #147) +- **AI chat: the live token counter now ticks between agent steps.** During a + multi-step turn the header token badge (and the "Thinking… · N tokens" line) + no longer froze on the previous step's authoritative usage; the current step's + estimate is combined per-component with `max`, so the count rises smoothly and + never jumps backwards. (#163) ## [0.93.0] - 2026-06-21 diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 00605374..bd8c4ed3 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -711,6 +711,7 @@ "Authorization header": "Authorization header", "Tool allowlist": "Tool allowlist", "Optional. Leave empty to allow all tools the server exposes.": "Optional. Leave empty to allow all tools the server exposes.", + "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"_*\".": "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"_*\".", "Test": "Test", "Available tools": "Available tools", "No tools available": "No tools available", @@ -1078,6 +1079,8 @@ "Undo": "Undo", "Redo": "Redo", "Backlinks": "Backlinks", + "Back to references": "Back to references", + "Back to reference {{label}}": "Back to reference {{label}}", "Last updated by": "Last updated by", "Last updated": "Last updated", "Stats": "Stats", diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 5478893f..f8c59436 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -406,6 +406,8 @@ "Footnote {{number}}": "Сноска {{number}}", "Go to footnote": "Перейти к сноске", "Back to reference": "Вернуться к ссылке", + "Back to references": "Вернуться к ссылкам", + "Back to reference {{label}}": "Вернуться к ссылке {{label}}", "Empty footnote": "Пустая сноска", "Math inline": "Строчная формула", "Insert inline math equation.": "Вставить математическое выражение в строку.", @@ -750,6 +752,8 @@ "Manage API keys for all users in the workspace. View the API documentation for usage details.": "Управляйте API-ключами для всех пользователей в рабочем пространстве. Смотрите документацию по API для получения информации об использовании.", "View the API documentation for usage details.": "Смотрите документацию по API для получения информации об использовании.", "View the MCP documentation.": "Смотрите документацию по MCP.", + "Instructions": "Инструкции", + "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"_*\".": "Необязательное указание агенту, как и когда использовать инструменты этого сервера. Добавляется в системный промпт. Инструменты сервера именуются с префиксом «<имя сервера>_*».", "Sources": "Источники", "AI Answers not available for attachments": "Ответы ИИ недоступны для вложений", "No answer available": "Ответ недоступен", diff --git a/apps/client/src/features/ai-chat/components/ai-chat.module.css b/apps/client/src/features/ai-chat/components/ai-chat.module.css index 71cc0e9d..cd788cdd 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat.module.css +++ b/apps/client/src/features/ai-chat/components/ai-chat.module.css @@ -161,7 +161,11 @@ margin-top: 4px; font-size: var(--mantine-font-size-xs); color: light-dark(var(--mantine-color-gray-7), var(--mantine-color-dark-1)); - white-space: pre-wrap; + /* NOTE: `white-space: pre-wrap` is intentionally NOT set here. On the + rendered markdown
it would turn the newlines between block tags + (\n
  • ,

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

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

    2. holds the text directly, not wrapped in a

      . + expect(html).toContain("

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

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

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

        Intro paragraph.

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

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

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

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

        5. ` + * wrapped in a `

          `) and separate `

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

        6. `) and joined paragraphs. The + * chat markdown renderer runs with `breaks: true`, so a single `\n` still + * becomes a `
          ` — line breaks inside the reasoning are preserved; only the + * empty gaps between blocks disappear. Apply ONLY to reasoning text, never to a + * normal assistant answer (where paragraph spacing is intentional). + * + * Fenced code is preserved verbatim: a fence opens on a line whose first + * non-space characters are ``` or ~~~ and closes on the next line that starts + * with the same fence character. Blank lines between fences (significant for + * code formatting) are never collapsed. + */ +export function collapseBlankLines(text: string): string { + const lines = text.split("\n"); + const out: string[] = []; + let inFence = false; + let fenceChar = ""; + + for (const line of lines) { + const fenceMatch = line.match(/^\s*(`{3,}|~{3,})/); + if (fenceMatch) { + const ch = fenceMatch[1][0]; + if (!inFence) { + inFence = true; + fenceChar = ch; + } else if (ch === fenceChar) { + inFence = false; + } + out.push(line); + continue; + } + + // Inside a fenced block every line (including blanks) is significant. + if (inFence) { + out.push(line); + continue; + } + + // Outside fences: drop blank lines so a `\n\n+` gap collapses to a single + // `\n` between the surrounding content lines. + if (line.trim() === "") continue; + out.push(line); + } + + return out.join("\n"); +} diff --git a/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts b/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts index 62256bc3..3e650f0d 100644 --- a/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts +++ b/apps/client/src/features/ai-chat/utils/count-stream-tokens.test.ts @@ -117,3 +117,55 @@ describe("liveTurnTokens — authoritative path", () => { expect(r).toEqual({ reasoning: 0, output: 1, authoritative: false }); }); }); + +describe("liveTurnTokens — combined authoritative + estimate (#163)", () => { + it("ticks the in-flight step above the completed-steps authoritative base", () => { + // The authoritative usage is the sum over COMPLETED steps (step 1). The + // CURRENT step is streaming and its text is NOT in `usage` yet, but it IS in + // the parts -> the running estimate must push the live figure above the base + // so the badge keeps growing between step boundaries. + const longText = "x".repeat(800); // 800 chars -> 200 est output tokens + const r = liveTurnTokens( + msg([{ type: "text", text: longText }], { + usage: { inputTokens: 500, outputTokens: 40 }, // step-1 base: 40 output + }), + ); + // max(authOutput=40, estOutput=200) = 200 -> the counter ticks, not frozen. + expect(r.output).toBe(200); + expect(r.authoritative).toBe(true); + }); + + it("ticks reasoning of the in-flight step above the authoritative reasoning base", () => { + const longReasoning = "r".repeat(400); // 400 chars -> 100 est reasoning + const r = liveTurnTokens( + msg([{ type: "reasoning", text: longReasoning }], { + usage: { inputTokens: 100, outputTokens: 20, reasoningTokens: 20 }, + }), + ); + // reasoning: max(20, 100) = 100 ; output: max(max(0,20-20)=0, 0) = 0. + expect(r.reasoning).toBe(100); + expect(r.output).toBe(0); + expect(r.authoritative).toBe(true); + }); + + it("snaps to the authoritative figure once it exceeds the rough estimate", () => { + // Short on-screen text (estimate tiny) but a large authoritative output: + // the exact figure wins at the boundary (the counter never under-reports). + const r = liveTurnTokens( + msg([{ type: "text", text: "abcd" }], { + usage: { inputTokens: 10, outputTokens: 5000 }, + }), + ); + expect(r.output).toBe(5000); + }); + + it("is monotonic: max never drops below the authoritative base when the estimate is smaller", () => { + // Mirrors the legacy 'verbatim' tests: estimate < authoritative -> unchanged. + const r = liveTurnTokens( + msg([{ type: "text", text: "tiny" }], { + usage: { inputTokens: 500, outputTokens: 100, reasoningTokens: 30 }, + }), + ); + expect(r).toEqual({ reasoning: 30, output: 70, authoritative: true }); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts b/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts index e9cca6bb..9a900996 100644 --- a/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts +++ b/apps/client/src/features/ai-chat/utils/count-stream-tokens.ts @@ -56,39 +56,58 @@ function metadataUsage(message: UIMessage): AuthoritativeUsage | undefined { /** * Token split for the given (streaming) assistant message. * - * Prefers AUTHORITATIVE `metadata.usage` when the server has attached it (at a - * step/turn boundary, incl. `reasoningTokens`) — so the live counter snaps to the - * provider's exact figures. Until then it returns a running ESTIMATE summed over - * the message parts: `reasoning` parts feed the reasoning estimate, `text` parts - * feed the output estimate. Multi-part / multi-step turns accumulate naturally - * because every part of the turn is summed. + * COMBINES the authoritative server usage with the running text estimate so the + * counter ticks in real time AND lands exact. The server only attaches + * `metadata.usage` at a step/turn boundary (`finish-step`/`finish`) and it is + * CUMULATIVE over COMPLETED steps — it does NOT yet include the in-flight step. + * So a multi-step turn that returned the authoritative figure verbatim would + * FREEZE between boundaries and jump in steps (issue #163). + * + * Instead we always compute the running ESTIMATE (chars/≈4 over the message's + * `reasoning`/`text` parts, which grows on every streamed delta) and take the + * per-component MAX of the authoritative base and the estimate: + * - between boundaries the estimate of the in-flight step ticks the number up; + * - at a boundary the authoritative figure snaps it to exact; + * - because the server's usage is cumulative and we only ever take the max, the + * number is MONOTONIC — it never drops. * * Providers that don't stream reasoning text still surface a reasoning count once - * the authoritative usage arrives (`usage.reasoningTokens`); on the pure estimate - * path such a turn simply shows `reasoning: 0` until then. + * the authoritative usage arrives (`max(reasoningTokens, 0)`); on the pure + * estimate path (no usage yet) such a turn shows `reasoning: 0` until then. */ export function liveTurnTokens(message: UIMessage | undefined): LiveTurnTokens { if (!message) return { reasoning: 0, output: 0, authoritative: false }; - const usage = metadataUsage(message); - if (usage) { - // Authoritative branch: outputTokens already INCLUDES reasoning tokens in the - // AI SDK usage shape, so subtract reasoning out for the "answer" figure (never - // go negative if a provider reports them inconsistently). - const reasoning = usage.reasoningTokens ?? 0; - const totalOutput = usage.outputTokens ?? 0; - const output = Math.max(0, totalOutput - reasoning); - return { reasoning, output, authoritative: true }; - } - - let reasoning = 0; - let output = 0; + // Running ESTIMATE over every reasoning/text part — grows on each delta. This + // includes the IN-FLIGHT step, which the authoritative usage does not cover yet. + let estReasoning = 0; + let estOutput = 0; for (const part of message.parts ?? []) { if (part.type === "reasoning") { - reasoning += estimateTokens((part as { text?: string }).text ?? ""); + estReasoning += estimateTokens((part as { text?: string }).text ?? ""); } else if (part.type === "text") { - output += estimateTokens((part as { text?: string }).text ?? ""); + estOutput += estimateTokens((part as { text?: string }).text ?? ""); } } - return { reasoning, output, authoritative: false }; + + const usage = metadataUsage(message); + if (!usage) { + // No authoritative usage streamed yet: the estimate IS the live figure. + return { reasoning: estReasoning, output: estOutput, authoritative: false }; + } + + // Authoritative sum over COMPLETED steps. `outputTokens` already INCLUDES + // reasoning in the AI SDK usage shape, so subtract it out for the "answer" + // figure (never go negative if a provider reports them inconsistently). + const authReasoning = usage.reasoningTokens ?? 0; + const authOutput = Math.max(0, (usage.outputTokens ?? 0) - authReasoning); + + // Per-component max: the in-flight step's estimate ticks above the completed- + // steps base between boundaries, and the authoritative figure wins once it + // exceeds the (rough) estimate at the next boundary. Monotonic by construction. + return { + reasoning: Math.max(authReasoning, estReasoning), + output: Math.max(authOutput, estOutput), + authoritative: true, + }; } diff --git a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx index e3e0522a..b8fe182f 100644 --- a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx +++ b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx @@ -1,25 +1,45 @@ import { NodeViewContent, NodeViewProps, NodeViewWrapper } from "@tiptap/react"; import { useTranslation } from "react-i18next"; -import { getFootnoteNumber } from "@docmost/editor-ext"; +import { getFootnoteNumber, getFootnoteRefCount } from "@docmost/editor-ext"; import classes from "./footnote.module.css"; +/** + * A 0-based backlink index -> its lowercase letter label (0 -> "a", 25 -> "z", + * 26 -> "aa", ...), matching the Pandoc/Wikipedia "↩ a b c" convention. + */ +export function backlinkLabel(index: number): string { + let out = ""; + let x = index; + while (x >= 0) { + out = String.fromCharCode(97 + (x % 26)) + out; + x = Math.floor(x / 26) - 1; + } + return out; +} + /** * NodeView for a single footnote definition: a decorative number marker, the * editable content (NodeViewContent), and a "↩" back-link to its reference. * The number is derived from the document (not stored). + * + * After #166 a footnote can be referenced more than once (one number, one + * definition, N forward links). When it is, the back-link becomes a row of + * per-occurrence links — ↩ a b c … — each scrolling to its own reference (#168); + * a single-reference footnote keeps the plain ↩. */ export default function FootnoteDefinitionView(props: NodeViewProps) { const { node, editor } = props; const { t } = useTranslation(); const id = node.attrs.id as string; - // Read the cached number from the numbering plugin (computed once per doc - // change) rather than recomputing the whole map on every render. + // Read the cached number/ref-count from the numbering plugin (computed once + // per doc change) rather than recomputing the whole map on every render. const number = getFootnoteNumber(editor.state, id) ?? "?"; + const refCount = getFootnoteRefCount(editor.state, id); - const handleBack = (e: React.MouseEvent) => { + const jumpTo = (e: React.MouseEvent, index: number) => { e.preventDefault(); - editor.commands.scrollToReference(id); + editor.commands.scrollToReference(id, index); }; return ( @@ -42,16 +62,47 @@ export default function FootnoteDefinitionView(props: NodeViewProps) { > {number}. - - ↩ - + {refCount > 1 ? ( + // Multiple references -> ↩ followed by one lettered link per occurrence. + + + {Array.from({ length: refCount }, (_, i) => ( + jumpTo(e, i)} + role="button" + aria-label={t("Back to reference {{label}}", { + label: backlinkLabel(i), + })} + title={t("Back to reference {{label}}", { + label: backlinkLabel(i), + })} + > + {backlinkLabel(i)} + + ))} + + ) : ( + // Single reference -> the plain ↩ (unchanged behavior). + jumpTo(e, 0)} + role="button" + aria-label={t("Back to reference")} + title={t("Back to reference")} + > + ↩ + + )} ); } diff --git a/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx b/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx index 3e28493d..bfffac90 100644 --- a/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx +++ b/apps/client/src/features/editor/components/footnote/footnote-views.structure.test.tsx @@ -1,5 +1,5 @@ -import { describe, it, expect, vi } from "vitest"; -import { render } from "@testing-library/react"; +import { describe, it, expect, vi, afterEach } from "vitest"; +import { render, fireEvent } from "@testing-library/react"; /** * Structural regression guard for #146 (PR #147). @@ -36,10 +36,14 @@ vi.mock("react-i18next", () => ({ useTranslation: () => ({ t: (key: string) => key }), })); -// footnote-definition-view reads a cached number from the numbering plugin; -// stub it so we don't need a live ProseMirror state. +// footnote-definition-view reads a cached number + reference count from the +// numbering plugin; stub them so we don't need a live ProseMirror state. The +// ref-count is a hoisted mutable so a test can drive the single-vs-multi +// backlink branch (#168). Default 1 = single reference (the #146 cases). +const { mockRefCount } = vi.hoisted(() => ({ mockRefCount: { value: 1 } })); vi.mock("@docmost/editor-ext", () => ({ getFootnoteNumber: () => 1, + getFootnoteRefCount: () => mockRefCount.value, })); // Mocks so CodeBlockView renders cheaply (no MantineProvider, no matchMedia). @@ -59,7 +63,8 @@ vi.mock("@mantine/core", () => ({ ), })); vi.mock("@/components/common/copy-button", () => ({ - CopyButton: ({ children }: any) => children({ copied: false, copy: () => {} }), + CopyButton: ({ children }: any) => + children({ copied: false, copy: () => {} }), })); vi.mock("@tabler/icons-react", () => ({ IconCheck: () => null, @@ -70,7 +75,9 @@ vi.mock("@/features/editor/components/code-block/mermaid-view.tsx", () => ({ })); import FootnotesListView from "./footnotes-list-view"; -import FootnoteDefinitionView from "./footnote-definition-view"; +import FootnoteDefinitionView, { + backlinkLabel, +} from "./footnote-definition-view"; import CodeBlockView from "../code-block/code-block-view"; // Minimal NodeViewProps stub: definition view only touches node.attrs.id and @@ -141,3 +148,84 @@ describe("#146 editable NodeView contentDOM-first invariant", () => { }, ); }); + +// #168: a footnote referenced more than once shows one lettered backlink per +// occurrence (↩ a b c), each scrolling to its own reference; a single-reference +// footnote keeps the plain ↩. +describe("#168 footnote definition multi-backlinks", () => { + afterEach(() => { + // Reset the shared ref-count mock so other tests see a single reference. + mockRefCount.value = 1; + }); + + const makeProps = () => + ({ + node: { attrs: { id: "fn-1" }, textContent: "" }, + editor: { + state: {}, + isEditable: true, + commands: { scrollToReference: vi.fn() }, + }, + getPos: () => 0, + updateAttributes: () => {}, + deleteNode: () => {}, + }) as any; + + it("renders one lettered backlink per reference (a, b, c) plus the ↩ arrow", () => { + mockRefCount.value = 3; + const { getByTestId } = render(); + const wrapper = getByTestId("nvw"); + + const links = wrapper.querySelectorAll('[role="button"]'); + expect(Array.from(links).map((l) => l.textContent)).toEqual([ + "a", + "b", + "c", + ]); + // The ↩ arrow is present (as decorative chrome, not a button). + expect(wrapper.textContent).toContain("↩"); + }); + + it("clicking the n-th backlink scrolls to the n-th occurrence (0-based)", () => { + mockRefCount.value = 3; + const props = makeProps(); + const { getByTestId } = render(); + const links = getByTestId("nvw").querySelectorAll('[role="button"]'); + + fireEvent.click(links[1]); // "b" + expect(props.editor.commands.scrollToReference).toHaveBeenCalledWith( + "fn-1", + 1, + ); + }); + + it("a single-reference footnote renders just one ↩ (no letters)", () => { + mockRefCount.value = 1; + const props = makeProps(); + const { getByTestId } = render(); + const wrapper = getByTestId("nvw"); + + const links = wrapper.querySelectorAll('[role="button"]'); + expect(links.length).toBe(1); + expect(links[0].textContent).toBe("↩"); + + fireEvent.click(links[0]); + expect(props.editor.commands.scrollToReference).toHaveBeenCalledWith( + "fn-1", + 0, + ); + }); +}); + +// #185 re-review pt 7: backlinkLabel is base-26 (a..z, then aa…). The component +// tests only cover a,b,c (index 0-2); pin the >= 26 carry boundary. +describe("backlinkLabel base-26 boundary (#168)", () => { + it("maps 0->a, 25->z, 26->aa, 27->ab, 51->az, 52->ba", () => { + expect(backlinkLabel(0)).toBe("a"); + expect(backlinkLabel(25)).toBe("z"); + expect(backlinkLabel(26)).toBe("aa"); + expect(backlinkLabel(27)).toBe("ab"); + expect(backlinkLabel(51)).toBe("az"); + expect(backlinkLabel(52)).toBe("ba"); + }); +}); diff --git a/apps/client/src/features/editor/components/footnote/footnote.module.css b/apps/client/src/features/editor/components/footnote/footnote.module.css index 8f1ba9e7..fb21fc03 100644 --- a/apps/client/src/features/editor/components/footnote/footnote.module.css +++ b/apps/client/src/features/editor/components/footnote/footnote.module.css @@ -115,3 +115,18 @@ .backLink:hover { text-decoration: underline; } + +/* Multi-backlink row (#168): ↩ a b c — one lettered link per reference + occurrence. Sits on the right, after the content, like the single ↩. */ +.backLinks { + flex: 0 0 auto; + display: inline-flex; + align-items: baseline; + gap: 0.3em; + user-select: none; +} + +.backLinkArrow { + color: var(--mantine-color-dimmed); + font-size: 0.9em; +} diff --git a/apps/client/src/features/page/queries/page-query.ts b/apps/client/src/features/page/queries/page-query.ts index 4e279621..ee44b775 100644 --- a/apps/client/src/features/page/queries/page-query.ts +++ b/apps/client/src/features/page/queries/page-query.ts @@ -274,7 +274,10 @@ export function useRestorePageMutation() { queryClient.setQueryData(["pages", restoredPage.slugId], merge); }, onError: (error) => { - notifications.show({ message: t("Failed to restore page"), color: "red" }); + notifications.show({ + message: t("Failed to restore page"), + color: "red", + }); }, }); } @@ -285,10 +288,10 @@ export function useGetSidebarPagesQuery( return useInfiniteQuery({ queryKey: ["sidebar-pages", data], enabled: !!data?.pageId || !!data?.spaceId, - queryFn: ({ pageParam }) => getSidebarPages({ ...data, cursor: pageParam, limit: 100 }), + queryFn: ({ pageParam }) => + getSidebarPages({ ...data, cursor: pageParam, limit: 100 }), initialPageParam: undefined, - getNextPageParam: (lastPage) => - lastPage.meta?.nextCursor ?? undefined, + getNextPageParam: (lastPage) => lastPage.meta?.nextCursor ?? undefined, }); } @@ -296,11 +299,14 @@ export function useGetRootSidebarPagesQuery(data: SidebarPagesParams) { return useInfiniteQuery({ queryKey: ["root-sidebar-pages", data.spaceId], queryFn: async ({ pageParam }) => { - return getSidebarPages({ spaceId: data.spaceId, cursor: pageParam, limit: 100 }); + return getSidebarPages({ + spaceId: data.spaceId, + cursor: pageParam, + limit: 100, + }); }, initialPageParam: undefined, - getNextPageParam: (lastPage) => - lastPage.meta?.nextCursor ?? undefined, + getNextPageParam: (lastPage) => lastPage.meta?.nextCursor ?? undefined, }); } @@ -323,12 +329,17 @@ export function usePageBreadcrumbsQuery( }); } -export async function fetchAllAncestorChildren(params: SidebarPagesParams) { +export async function fetchAllAncestorChildren( + params: SidebarPagesParams, + // `fresh: true` forces a server refetch (staleTime 0) — used by the reconnect + // refresh (#159 #8), which must NOT receive the 30-min-cached children. + opts?: { fresh?: boolean }, +) { // not using a hook here, so we can call it inside a useEffect hook const response = await queryClient.fetchQuery({ queryKey: ["sidebar-pages", params], queryFn: () => getAllSidebarPages(params), - staleTime: 30 * 60 * 1000, + staleTime: opts?.fresh ? 0 : 30 * 60 * 1000, }); const allItems = response.pages.flatMap((page) => page.items); @@ -347,11 +358,15 @@ export function useRecentChangesQuery(spaceId?: string) { }); } -export function useCreatedByQuery(params?: { userId?: string; spaceId?: string }) { +export function useCreatedByQuery(params?: { + userId?: string; + spaceId?: string; +}) { const { userId, spaceId } = params ?? {}; return useInfiniteQuery({ queryKey: ["pages-created-by-user", { userId, spaceId }], - queryFn: ({ pageParam }) => getCreatedByPages({ userId, spaceId, cursor: pageParam, limit: 15 }), + queryFn: ({ pageParam }) => + getCreatedByPages({ userId, spaceId, cursor: pageParam, limit: 15 }), initialPageParam: undefined as string | undefined, getNextPageParam: (lastPage) => lastPage.meta.hasNextPage ? lastPage.meta.nextCursor : undefined, diff --git a/apps/client/src/features/page/tree/components/space-tree.tsx b/apps/client/src/features/page/tree/components/space-tree.tsx index b2cfb994..affcbac3 100644 --- a/apps/client/src/features/page/tree/components/space-tree.tsx +++ b/apps/client/src/features/page/tree/components/space-tree.tsx @@ -29,9 +29,11 @@ import { collectBranchIds, openBranches, closeIds, + loadedOpenBranchIds, } from "@/features/page/tree/utils/utils.ts"; import { SpaceTreeNode } from "@/features/page/tree/types.ts"; import { treeModel } from "@/features/page/tree/model/tree-model"; +import { socketAtom } from "@/features/websocket/atoms/socket-atom.ts"; import { getPageBreadcrumbs, getSpaceTree, @@ -39,11 +41,7 @@ import { import { IPage } from "@/features/page/types/page.types.ts"; import { extractPageSlugId } from "@/lib"; import { isCompactPageTreeEnabled } from "@/lib/config.ts"; -import { - DocTree, - ROW_HEIGHT_COMPACT, - ROW_HEIGHT_STANDARD, -} from "./doc-tree"; +import { DocTree, ROW_HEIGHT_COMPACT, ROW_HEIGHT_STANDARD } from "./doc-tree"; import { SpaceTreeRow } from "./space-tree-row"; interface SpaceTreeProps { @@ -193,6 +191,54 @@ const SpaceTree = forwardRef(function SpaceTree( [openTreeNodes], ); + // Latest tree + open-state for the reconnect handler (its closure would + // otherwise read stale snapshots). + const [socket] = useAtom(socketAtom); + const dataRef = useRef(data); + dataRef.current = data; + const openIdsRef = useRef(openIds); + openIdsRef.current = openIds; + + // Reconnect refresh (#159 #8): on a socket reconnect, re-fetch and reconcile + // the children of every currently-open, already-loaded branch of THIS space, + // so a move/rename/delete that happened INSIDE a loaded branch while events + // were missed (laptop sleep / wifi gap) is reflected instead of left stale. + // The ROOT level is reconciled separately by the root-query refetch + + // mergeRootTrees; an UNLOADED branch is skipped (lazy-load fetches it fresh on + // expand). No first-connect guard is needed: space-tree usually mounts AFTER + // the initial connect, so every `connect` it sees is a reconnect; the rare + // initial-connect case has an empty tree, so the refresh is a harmless no-op. + useEffect(() => { + if (!socket) return; + const onConnect = async () => { + const effectSpaceId = spaceIdRef.current; + const branchIds = loadedOpenBranchIds( + dataRef.current.filter((n) => n?.spaceId === effectSpaceId), + openIdsRef.current, + ); + if (branchIds.length === 0) return; + for (const id of branchIds) { + try { + // `fresh: true` bypasses the 30-min sidebar-pages cache so the + // reconcile sees the server's CURRENT children (handler-order + // independent — no reliance on the global reconnect invalidation). + const fresh = await fetchAllAncestorChildren( + { pageId: id, spaceId: effectSpaceId }, + { fresh: true }, + ); + if (spaceIdRef.current !== effectSpaceId) return; // space switched + setData((prev) => treeModel.reconcileChildren(prev, id, fresh)); + } catch (err) { + console.error("[tree] reconnect branch refresh failed", err); + } + } + }; + socket.on("connect", onConnect); + return () => { + socket.off("connect", onConnect); + }; + }, [socket, setData]); + const handleToggle = useCallback( async (id: string, isOpen: boolean) => { setOpenTreeNodes((prev) => ({ ...prev, [id]: isOpen })); @@ -245,8 +291,7 @@ const SpaceTree = forwardRef(function SpaceTree( notifications.show({ color: "red", message: t("Couldn't expand the tree: {{reason}}", { - reason: - err?.response?.data?.message ?? err?.message ?? String(err), + reason: err?.response?.data?.message ?? err?.message ?? String(err), }), }); } finally { @@ -262,11 +307,11 @@ const SpaceTree = forwardRef(function SpaceTree( setOpenTreeNodes((prev) => closeIds(prev, ids)); }, [filteredData, setOpenTreeNodes]); - useImperativeHandle( - ref, - () => ({ expandAll, collapseAll, isExpanding }), - [expandAll, collapseAll, isExpanding], - ); + useImperativeHandle(ref, () => ({ expandAll, collapseAll, isExpanding }), [ + expandAll, + collapseAll, + isExpanding, + ]); // Stable callbacks for DocTree. Without these, every parent render recreates // the props and tears down every row's draggable/dropTarget subscription, diff --git a/apps/client/src/features/page/tree/model/tree-model.test.ts b/apps/client/src/features/page/tree/model/tree-model.test.ts index 25c0bd1e..a2dbd6b9 100644 --- a/apps/client/src/features/page/tree/model/tree-model.test.ts +++ b/apps/client/src/features/page/tree/model/tree-model.test.ts @@ -1,218 +1,309 @@ -import { describe, it, expect } from 'vitest'; -import { treeModel } from './tree-model'; -import type { TreeNode } from './tree-model.types'; +import { describe, it, expect } from "vitest"; +import { treeModel } from "./tree-model"; +import type { TreeNode } from "./tree-model.types"; type N = TreeNode<{ name: string }>; const fixture: N[] = [ { - id: 'a', - name: 'A', + id: "a", + name: "A", children: [ - { id: 'a1', name: 'A1', children: [{ id: 'a1a', name: 'A1a' }] }, - { id: 'a2', name: 'A2' }, + { id: "a1", name: "A1", children: [{ id: "a1a", name: "A1a" }] }, + { id: "a2", name: "A2" }, ], }, - { id: 'b', name: 'B' }, + { id: "b", name: "B" }, ]; -describe('treeModel.find', () => { - it('finds a root node', () => { - expect(treeModel.find(fixture, 'a')?.name).toBe('A'); +describe("treeModel.find", () => { + it("finds a root node", () => { + expect(treeModel.find(fixture, "a")?.name).toBe("A"); }); - it('finds a deeply nested node', () => { - expect(treeModel.find(fixture, 'a1a')?.name).toBe('A1a'); + it("finds a deeply nested node", () => { + expect(treeModel.find(fixture, "a1a")?.name).toBe("A1a"); }); - it('returns null for unknown id', () => { - expect(treeModel.find(fixture, 'zzz')).toBeNull(); + it("returns null for unknown id", () => { + expect(treeModel.find(fixture, "zzz")).toBeNull(); }); }); -describe('treeModel.path', () => { - it('returns root-to-leaf path for nested id', () => { - const p = treeModel.path(fixture, 'a1a'); - expect(p?.map((n) => n.id)).toEqual(['a', 'a1', 'a1a']); +describe("treeModel.path", () => { + it("returns root-to-leaf path for nested id", () => { + const p = treeModel.path(fixture, "a1a"); + expect(p?.map((n) => n.id)).toEqual(["a", "a1", "a1a"]); }); - it('returns [node] for root-level id', () => { - expect(treeModel.path(fixture, 'b')?.map((n) => n.id)).toEqual(['b']); + it("returns [node] for root-level id", () => { + expect(treeModel.path(fixture, "b")?.map((n) => n.id)).toEqual(["b"]); }); - it('returns null for unknown id', () => { - expect(treeModel.path(fixture, 'zzz')).toBeNull(); + it("returns null for unknown id", () => { + expect(treeModel.path(fixture, "zzz")).toBeNull(); }); }); -describe('treeModel.siblingsOf', () => { - it('returns siblings + parent + index for a child', () => { - const info = treeModel.siblingsOf(fixture, 'a2'); - expect(info?.parentId).toBe('a'); - expect(info?.siblings.map((n) => n.id)).toEqual(['a1', 'a2']); +describe("treeModel.siblingsOf", () => { + it("returns siblings + parent + index for a child", () => { + const info = treeModel.siblingsOf(fixture, "a2"); + expect(info?.parentId).toBe("a"); + expect(info?.siblings.map((n) => n.id)).toEqual(["a1", "a2"]); expect(info?.index).toBe(1); }); - it('returns parentId null + root siblings for a root id', () => { - const info = treeModel.siblingsOf(fixture, 'b'); + it("returns parentId null + root siblings for a root id", () => { + const info = treeModel.siblingsOf(fixture, "b"); expect(info?.parentId).toBeNull(); - expect(info?.siblings.map((n) => n.id)).toEqual(['a', 'b']); + expect(info?.siblings.map((n) => n.id)).toEqual(["a", "b"]); expect(info?.index).toBe(1); }); - it('returns null for unknown id', () => { - expect(treeModel.siblingsOf(fixture, 'zzz')).toBeNull(); + it("returns null for unknown id", () => { + expect(treeModel.siblingsOf(fixture, "zzz")).toBeNull(); }); }); -describe('treeModel.isDescendant', () => { - it('returns true when descendantId is nested under ancestorId', () => { - expect(treeModel.isDescendant(fixture, 'a', 'a1a')).toBe(true); +describe("treeModel.isDescendant", () => { + it("returns true when descendantId is nested under ancestorId", () => { + expect(treeModel.isDescendant(fixture, "a", "a1a")).toBe(true); }); - it('returns false when ids are siblings', () => { - expect(treeModel.isDescendant(fixture, 'a1', 'a2')).toBe(false); + it("returns false when ids are siblings", () => { + expect(treeModel.isDescendant(fixture, "a1", "a2")).toBe(false); }); - it('returns false when ancestorId is the same as descendantId', () => { - expect(treeModel.isDescendant(fixture, 'a', 'a')).toBe(false); + it("returns false when ancestorId is the same as descendantId", () => { + expect(treeModel.isDescendant(fixture, "a", "a")).toBe(false); }); - it('returns false for unknown ids', () => { - expect(treeModel.isDescendant(fixture, 'zzz', 'a')).toBe(false); + it("returns false for unknown ids", () => { + expect(treeModel.isDescendant(fixture, "zzz", "a")).toBe(false); }); }); -describe('treeModel.visible', () => { - it('returns only root nodes when no openIds', () => { +describe("treeModel.visible", () => { + it("returns only root nodes when no openIds", () => { const v = treeModel.visible(fixture, new Set()); - expect(v.map((n) => n.id)).toEqual(['a', 'b']); + expect(v.map((n) => n.id)).toEqual(["a", "b"]); }); - it('includes children of open ids in DFS order', () => { - const v = treeModel.visible(fixture, new Set(['a'])); - expect(v.map((n) => n.id)).toEqual(['a', 'a1', 'a2', 'b']); + it("includes children of open ids in DFS order", () => { + const v = treeModel.visible(fixture, new Set(["a"])); + expect(v.map((n) => n.id)).toEqual(["a", "a1", "a2", "b"]); }); - it('recursively descends through chains of open ids', () => { - const v = treeModel.visible(fixture, new Set(['a', 'a1'])); - expect(v.map((n) => n.id)).toEqual(['a', 'a1', 'a1a', 'a2', 'b']); + it("recursively descends through chains of open ids", () => { + const v = treeModel.visible(fixture, new Set(["a", "a1"])); + expect(v.map((n) => n.id)).toEqual(["a", "a1", "a1a", "a2", "b"]); }); - it('ignores openIds that are not in the tree', () => { - const v = treeModel.visible(fixture, new Set(['ghost'])); - expect(v.map((n) => n.id)).toEqual(['a', 'b']); + it("ignores openIds that are not in the tree", () => { + const v = treeModel.visible(fixture, new Set(["ghost"])); + expect(v.map((n) => n.id)).toEqual(["a", "b"]); }); }); -describe('treeModel.insert', () => { +describe("treeModel.insert", () => { const leaf = (id: string): N => ({ id, name: id.toUpperCase() }); - it('inserts at end when index is undefined', () => { - const t = treeModel.insert(fixture, 'a', leaf('a3')); - expect(treeModel.siblingsOf(t, 'a3')?.siblings.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'a3', + it("inserts at end when index is undefined", () => { + const t = treeModel.insert(fixture, "a", leaf("a3")); + expect(treeModel.siblingsOf(t, "a3")?.siblings.map((n) => n.id)).toEqual([ + "a1", + "a2", + "a3", ]); }); - it('inserts at index 0', () => { - const t = treeModel.insert(fixture, 'a', leaf('a0'), 0); - expect(treeModel.siblingsOf(t, 'a0')?.siblings.map((n) => n.id)).toEqual([ - 'a0', 'a1', 'a2', + it("inserts at index 0", () => { + const t = treeModel.insert(fixture, "a", leaf("a0"), 0); + expect(treeModel.siblingsOf(t, "a0")?.siblings.map((n) => n.id)).toEqual([ + "a0", + "a1", + "a2", ]); }); - it('inserts in the middle', () => { - const t = treeModel.insert(fixture, 'a', leaf('a1half'), 1); + it("inserts in the middle", () => { + const t = treeModel.insert(fixture, "a", leaf("a1half"), 1); expect( - treeModel.siblingsOf(t, 'a1half')?.siblings.map((n) => n.id), - ).toEqual(['a1', 'a1half', 'a2']); + treeModel.siblingsOf(t, "a1half")?.siblings.map((n) => n.id), + ).toEqual(["a1", "a1half", "a2"]); }); - it('inserts at root when parentId is null', () => { - const t = treeModel.insert(fixture, null, leaf('c')); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'c']); + it("inserts at root when parentId is null", () => { + const t = treeModel.insert(fixture, null, leaf("c")); + expect(t.map((n) => n.id)).toEqual(["a", "b", "c"]); }); - it('returns same array reference for unknown parentId', () => { - const t = treeModel.insert(fixture, 'ghost', leaf('zz')); + it("returns same array reference for unknown parentId", () => { + const t = treeModel.insert(fixture, "ghost", leaf("zz")); expect(t).toBe(fixture); }); - it('initializes children array when parent had no children', () => { - const t = treeModel.insert(fixture, 'b', leaf('b1')); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['b1']); + it("initializes children array when parent had no children", () => { + const t = treeModel.insert(fixture, "b", leaf("b1")); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["b1"]); }); }); -describe('treeModel.insertByPosition', () => { +describe("treeModel.insertByPosition", () => { // Server-authoritative broadcasts ship the node's fractional `position`; the // receiver inserts among already-loaded siblings ordered by `position`. type P = TreeNode<{ name: string; position?: string }>; const roots: P[] = [ - { id: 'a', name: 'A', position: 'a0' }, - { id: 'b', name: 'B', position: 'a2' }, - { id: 'c', name: 'C', position: 'a4' }, + { id: "a", name: "A", position: "a0" }, + { id: "b", name: "B", position: "a2" }, + { id: "c", name: "C", position: "a4" }, ]; - it('inserts a root node in position order (middle)', () => { - const node: P = { id: 'x', name: 'X', position: 'a3' }; + it("inserts a root node in position order (middle)", () => { + const node: P = { id: "x", name: "X", position: "a3" }; const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'x', 'c']); + expect(t.map((n) => n.id)).toEqual(["a", "b", "x", "c"]); }); - it('inserts a root node at the front when its position sorts first', () => { - const node: P = { id: 'x', name: 'X', position: 'a-' }; + it("inserts a root node at the front when its position sorts first", () => { + const node: P = { id: "x", name: "X", position: "a-" }; const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['x', 'a', 'b', 'c']); + expect(t.map((n) => n.id)).toEqual(["x", "a", "b", "c"]); }); - it('appends a root node when its position sorts last', () => { - const node: P = { id: 'x', name: 'X', position: 'a9' }; + it("appends a root node when its position sorts last", () => { + const node: P = { id: "x", name: "X", position: "a9" }; const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'c', 'x']); + expect(t.map((n) => n.id)).toEqual(["a", "b", "c", "x"]); }); - it('produces the same order regardless of which siblings are loaded', () => { + it("produces the same order regardless of which siblings are loaded", () => { // Client 1 loaded all siblings; client 2 only loaded a subset. The inserted // node lands in a consistent relative position for both. const full: P[] = roots; const partial: P[] = [roots[0], roots[2]]; // a, c (b not loaded) - const node: P = { id: 'x', name: 'X', position: 'a3' }; + const node: P = { id: "x", name: "X", position: "a3" }; expect( treeModel.insertByPosition(full, null, node).map((n) => n.id), - ).toEqual(['a', 'b', 'x', 'c']); + ).toEqual(["a", "b", "x", "c"]); expect( treeModel.insertByPosition(partial, null, node).map((n) => n.id), - ).toEqual(['a', 'x', 'c']); + ).toEqual(["a", "x", "c"]); }); - it('inserts a child in position order under the parent', () => { + it("inserts a child in position order under the parent", () => { const tree: P[] = [ { - id: 'p', - name: 'P', - position: 'a0', + id: "p", + name: "P", + position: "a0", children: [ - { id: 'p1', name: 'P1', position: 'a0' }, - { id: 'p2', name: 'P2', position: 'a2' }, + { id: "p1", name: "P1", position: "a0" }, + { id: "p2", name: "P2", position: "a2" }, ], }, ]; - const node: P = { id: 'p15', name: 'P1.5', position: 'a1' }; - const t = treeModel.insertByPosition(tree, 'p', node); - expect(treeModel.find(t, 'p')?.children?.map((n) => n.id)).toEqual([ - 'p1', 'p15', 'p2', + const node: P = { id: "p15", name: "P1.5", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual([ + "p1", + "p15", + "p2", ]); }); - it('appends when the new node has no position', () => { - const node: P = { id: 'x', name: 'X' }; - const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'c', 'x']); + // #159 #1: inserting/moving a node under a parent whose children are NOT + // loaded (`children === undefined`, e.g. a collapsed page) must NOT materialize + // a partial `[node]` list — that would defeat the lazy-load gate and hide the + // parent's other real children. The node is left to be lazy-loaded; only + // `hasChildren` is flagged so the chevron appears. + it("does NOT materialize a child under an UNLOADED parent (children undefined)", () => { + type PH = TreeNode<{ + name: string; + position?: string; + hasChildren?: boolean; + }>; + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + const parent = treeModel.find(t, "p"); + // The node was NOT inserted (children stay unloaded -> lazy-load fetches the + // full set, including this node, on expand). + expect(parent?.children).toBeUndefined(); + expect(treeModel.find(t, "x")).toBeNull(); + // ...but the chevron is enabled so the user can expand to load it. + expect((parent as PH).hasChildren).toBe(true); }); - it('tie-break: a node whose position EQUALS a sibling lands deterministically (strict >)', () => { + it("DOES insert under a LOADED-but-empty parent (children: [])", () => { + type PH = TreeNode<{ + name: string; + position?: string; + hasChildren?: boolean; + }>; + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: false, children: [] }, + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + // A loaded (empty) child list is complete, so the node IS inserted. + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]); + }); + + it("appends when the new node has no position", () => { + const node: P = { id: "x", name: "X" }; + const t = treeModel.insertByPosition(roots, null, node); + expect(t.map((n) => n.id)).toEqual(["a", "b", "c", "x"]); + }); + + it("tie-break: a node whose position EQUALS a sibling lands deterministically (strict >)", () => { // The insertion index is the first sibling whose position sorts STRICTLY // after the new node's. An equal sibling is not strictly after, so it is // skipped — the new node lands immediately AFTER every equal-position // sibling and before the first strictly-greater one. This is deterministic: // a tie always resolves the same way on every client. - const node: P = { id: 'x', name: 'X', position: 'a2' }; // equals b's position + const node: P = { id: "x", name: "X", position: "a2" }; // equals b's position const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'x', 'c']); + expect(t.map((n) => n.id)).toEqual(["a", "b", "x", "c"]); + }); +}); + +// reconcileChildren (#159 #8): on a socket-reconnect refresh, an already-loaded +// branch is reconciled against a fresh server fetch — removed children drop, +// new ones appear, order follows the server, and surviving children keep their +// own loaded grandchildren (deeper expansion is not collapsed). +describe("treeModel.reconcileChildren", () => { + type N = TreeNode<{ name: string }>; + const leaf = (id: string): N => ({ id, name: id.toUpperCase() }); + + it("drops removed children, adds new ones, and follows the fresh order", () => { + const tree: N[] = [ + { id: "p", name: "P", children: [leaf("a"), leaf("b")] }, + ]; + // Server now has b, c (a was deleted/moved away; c is new) in this order. + const next = treeModel.reconcileChildren(tree, "p", [leaf("b"), leaf("c")]); + expect(treeModel.find(next, "p")?.children?.map((n) => n.id)).toEqual([ + "b", + "c", + ]); + expect(treeModel.find(next, "a")).toBeNull(); + }); + + it("preserves a surviving child's loaded grandchildren", () => { + const tree: N[] = [ + { + id: "p", + name: "P", + children: [{ id: "a", name: "A", children: [leaf("a1")] }, leaf("b")], + }, + ]; + // Fresh fetch returns only top-level children (no grandchildren). + const next = treeModel.reconcileChildren(tree, "p", [leaf("a"), leaf("b")]); + // 'a' keeps its previously loaded grandchild 'a1'. + expect(treeModel.find(next, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + ]); + }); + + it("leaves an UNLOADED parent (children undefined) untouched", () => { + const tree: N[] = [{ id: "p", name: "P" }]; // children: undefined + const next = treeModel.reconcileChildren(tree, "p", [leaf("a")]); + expect(next).toBe(tree); // no-op: lazy-load handles an unloaded branch + expect(treeModel.find(next, "p")?.children).toBeUndefined(); }); }); // addTreeNode idempotency: the receiver early-returns when the node id already // exists, so re-delivery (or the author's optimistic node) is never duplicated. // This guards the find-then-skip contract insertByPosition relies on. -describe('addTreeNode idempotency (find-then-skip)', () => { +describe("addTreeNode idempotency (find-then-skip)", () => { type P = TreeNode<{ name: string; position?: string }>; const applyAddTreeNode = (tree: P[], node: P): P[] => { @@ -220,22 +311,22 @@ describe('addTreeNode idempotency (find-then-skip)', () => { return treeModel.insertByPosition(tree, null, node); }; - it('does not insert a duplicate when the id already exists', () => { - const tree: P[] = [{ id: 'a', name: 'A', position: 'a0' }]; - const node: P = { id: 'a', name: 'A again', position: 'a5' }; + it("does not insert a duplicate when the id already exists", () => { + const tree: P[] = [{ id: "a", name: "A", position: "a0" }]; + const node: P = { id: "a", name: "A again", position: "a5" }; const t1 = applyAddTreeNode(tree, node); expect(t1).toBe(tree); - expect(t1.map((n) => n.id)).toEqual(['a']); + expect(t1.map((n) => n.id)).toEqual(["a"]); }); - it('inserts once, then is a no-op on repeat delivery', () => { - let tree: P[] = [{ id: 'a', name: 'A', position: 'a0' }]; - const node: P = { id: 'x', name: 'X', position: 'a5' }; + it("inserts once, then is a no-op on repeat delivery", () => { + let tree: P[] = [{ id: "a", name: "A", position: "a0" }]; + const node: P = { id: "x", name: "X", position: "a5" }; tree = applyAddTreeNode(tree, node); - expect(tree.map((n) => n.id)).toEqual(['a', 'x']); + expect(tree.map((n) => n.id)).toEqual(["a", "x"]); const again = applyAddTreeNode(tree, node); expect(again).toBe(tree); - expect(again.filter((n) => n.id === 'x')).toHaveLength(1); + expect(again.filter((n) => n.id === "x")).toHaveLength(1); }); }); @@ -243,7 +334,7 @@ describe('addTreeNode idempotency (find-then-skip)', () => { // now guarded by `treeModel.find` (same contract as the addTreeNode socket // handler) because the server's broadcast can win the race and insert the node // first. Whichever runs first inserts; the second is a no-op. Exactly one row. -describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { +describe("handleCreate optimistic-insert idempotency (find-then-skip)", () => { // Mirrors the guarded optimistic insert in use-tree-mutation handleCreate. const applyOptimisticInsert = ( tree: N[], @@ -256,17 +347,21 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { }; // Mirrors the addTreeNode socket handler guard. - const applyAddTreeNode = (tree: N[], parentId: string | null, node: N): N[] => { + const applyAddTreeNode = ( + tree: N[], + parentId: string | null, + node: N, + ): N[] => { if (treeModel.find(tree, node.id)) return tree; return treeModel.insert(tree, parentId, node); }; - const created: N = { id: 'new', name: '' }; + const created: N = { id: "new", name: "" }; - it('optimistic insert is a no-op when server addTreeNode already inserted it', () => { + it("optimistic insert is a no-op when server addTreeNode already inserted it", () => { // Reverse-of-reverse race: server wins. const afterServer = applyAddTreeNode(fixture, null, created); - expect(afterServer.filter((n) => n.id === 'new')).toHaveLength(1); + expect(afterServer.filter((n) => n.id === "new")).toHaveLength(1); const afterOptimistic = applyOptimisticInsert( afterServer, null, @@ -274,20 +369,27 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { afterServer.length, ); expect(afterOptimistic).toBe(afterServer); // skipped - expect(afterOptimistic.filter((n) => n.id === 'new')).toHaveLength(1); + expect(afterOptimistic.filter((n) => n.id === "new")).toHaveLength(1); }); - it('server addTreeNode is a no-op when optimistic insert already ran (optimistic-first)', () => { - const afterOptimistic = applyOptimisticInsert(fixture, null, created, fixture.length); - expect(afterOptimistic.filter((n) => n.id === 'new')).toHaveLength(1); + it("server addTreeNode is a no-op when optimistic insert already ran (optimistic-first)", () => { + const afterOptimistic = applyOptimisticInsert( + fixture, + null, + created, + fixture.length, + ); + expect(afterOptimistic.filter((n) => n.id === "new")).toHaveLength(1); const afterServer = applyAddTreeNode(afterOptimistic, null, created); expect(afterServer).toBe(afterOptimistic); // skipped - expect(afterServer.filter((n) => n.id === 'new')).toHaveLength(1); + expect(afterServer.filter((n) => n.id === "new")).toHaveLength(1); }); - it('inserts exactly once when only the optimistic path runs', () => { - const t = applyOptimisticInsert(fixture, 'a', { id: 'a3', name: '' }, 2); - expect(treeModel.find(t, 'a')?.children?.filter((n) => n.id === 'a3')).toHaveLength(1); + it("inserts exactly once when only the optimistic path runs", () => { + const t = applyOptimisticInsert(fixture, "a", { id: "a3", name: "" }, 2); + expect( + treeModel.find(t, "a")?.children?.filter((n) => n.id === "a3"), + ).toHaveLength(1); }); }); @@ -295,7 +397,7 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { // by `position` (NOT index 0) and apply the `pageData` the payload carries so a // moved node's title/icon/chevron stay correct. This mirrors the reducer in // use-tree-socket.ts so the contract is unit-tested without rendering the hook. -describe('moveTreeNode handler (place by position + apply pageData)', () => { +describe("moveTreeNode handler (place by position + apply pageData)", () => { type P = TreeNode<{ name: string; position?: string; @@ -310,7 +412,11 @@ describe('moveTreeNode handler (place by position + apply pageData)', () => { id: string; parentId: string | null; position: string; - pageData?: { title?: string | null; icon?: string | null; hasChildren?: boolean }; + pageData?: { + title?: string | null; + icon?: string | null; + hasChildren?: boolean; + }; }, ): P[] => { if (!treeModel.find(tree, payload.id)) return tree; @@ -325,8 +431,10 @@ describe('moveTreeNode handler (place by position + apply pageData)', () => { } as Partial

          ; const pd = payload.pageData; if (pd) { - if (pd.title !== undefined) (patch as { name?: string }).name = pd.title ?? ''; - if (pd.icon !== undefined) (patch as { icon?: string }).icon = pd.icon ?? undefined; + if (pd.title !== undefined) + (patch as { name?: string }).name = pd.title ?? ""; + if (pd.icon !== undefined) + (patch as { icon?: string }).icon = pd.icon ?? undefined; if (pd.hasChildren !== undefined) (patch as { hasChildren?: boolean }).hasChildren = pd.hasChildren; } @@ -335,118 +443,128 @@ describe('moveTreeNode handler (place by position + apply pageData)', () => { const tree: P[] = [ { - id: 'dst', - name: 'DST', - position: 'a0', + id: "dst", + name: "DST", + position: "a0", children: [ - { id: 'c1', name: 'C1', position: 'a1' }, - { id: 'c2', name: 'C2', position: 'a3' }, - { id: 'c3', name: 'C3', position: 'a5' }, + { id: "c1", name: "C1", position: "a1" }, + { id: "c2", name: "C2", position: "a3" }, + { id: "c3", name: "C3", position: "a5" }, ], }, - { id: 'src', name: 'SRC', position: 'a9' }, + { id: "src", name: "SRC", position: "a9" }, ]; - it('lands the moved node in the correct MIDDLE slot, not at index 0', () => { + it("lands the moved node in the correct MIDDLE slot, not at index 0", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'dst', - position: 'a4', + id: "src", + parentId: "dst", + position: "a4", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'src', 'c3', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "src", + "c3", ]); }); - it('lands the moved node at the END when position sorts last', () => { + it("lands the moved node at the END when position sorts last", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'dst', - position: 'a8', + id: "src", + parentId: "dst", + position: "a8", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'c3', 'src', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "c3", + "src", ]); }); - it('applies pageData (title/icon/hasChildren) to the moved node', () => { + it("applies pageData (title/icon/hasChildren) to the moved node", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'dst', - position: 'a4', - pageData: { title: 'Renamed', icon: '🔥', hasChildren: true }, + id: "src", + parentId: "dst", + position: "a4", + pageData: { title: "Renamed", icon: "🔥", hasChildren: true }, }); - const moved = treeModel.find(t, 'src'); - expect(moved?.name).toBe('Renamed'); - expect(moved?.icon).toBe('🔥'); + const moved = treeModel.find(t, "src"); + expect(moved?.name).toBe("Renamed"); + expect(moved?.icon).toBe("🔥"); expect(moved?.hasChildren).toBe(true); - expect(moved?.position).toBe('a4'); + expect(moved?.position).toBe("a4"); }); - it('falls back to removing the node when the destination parent is not loaded', () => { + it("falls back to removing the node when the destination parent is not loaded", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'not-loaded', - position: 'a4', + id: "src", + parentId: "not-loaded", + position: "a4", }); - expect(treeModel.find(t, 'src')).toBeNull(); + expect(treeModel.find(t, "src")).toBeNull(); }); }); -describe('treeModel.remove', () => { - it('removes a leaf', () => { - const t = treeModel.remove(fixture, 'a2'); - expect(treeModel.find(t, 'a2')).toBeNull(); +describe("treeModel.remove", () => { + it("removes a leaf", () => { + const t = treeModel.remove(fixture, "a2"); + expect(treeModel.find(t, "a2")).toBeNull(); }); - it('removes a subtree', () => { - const t = treeModel.remove(fixture, 'a1'); - expect(treeModel.find(t, 'a1')).toBeNull(); - expect(treeModel.find(t, 'a1a')).toBeNull(); + it("removes a subtree", () => { + const t = treeModel.remove(fixture, "a1"); + expect(treeModel.find(t, "a1")).toBeNull(); + expect(treeModel.find(t, "a1a")).toBeNull(); }); - it('removes a root node', () => { - const t = treeModel.remove(fixture, 'b'); - expect(t.map((n) => n.id)).toEqual(['a']); + it("removes a root node", () => { + const t = treeModel.remove(fixture, "b"); + expect(t.map((n) => n.id)).toEqual(["a"]); }); - it('returns same array reference for unknown id', () => { - expect(treeModel.remove(fixture, 'ghost')).toBe(fixture); + it("returns same array reference for unknown id", () => { + expect(treeModel.remove(fixture, "ghost")).toBe(fixture); }); }); -describe('treeModel.update', () => { - it('shallow-merges a patch on the matching node', () => { - const t = treeModel.update(fixture, 'a1', { name: 'A1-renamed' }); - expect(treeModel.find(t, 'a1')?.name).toBe('A1-renamed'); +describe("treeModel.update", () => { + it("shallow-merges a patch on the matching node", () => { + const t = treeModel.update(fixture, "a1", { name: "A1-renamed" }); + expect(treeModel.find(t, "a1")?.name).toBe("A1-renamed"); }); - it('returns same array reference for unknown id', () => { - expect(treeModel.update(fixture, 'ghost', { name: 'x' })).toBe(fixture); + it("returns same array reference for unknown id", () => { + expect(treeModel.update(fixture, "ghost", { name: "x" })).toBe(fixture); }); it("preserves children when patching parent's own fields", () => { - const t = treeModel.update(fixture, 'a', { name: 'A-renamed' }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', + const t = treeModel.update(fixture, "a", { name: "A-renamed" }); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", ]); }); - it('preserves reference identity of unrelated subtrees', () => { - const t = treeModel.update(fixture, 'a1', { name: 'X' }); + it("preserves reference identity of unrelated subtrees", () => { + const t = treeModel.update(fixture, "a1", { name: "X" }); expect(t[1]).toBe(fixture[1]); }); }); -describe('treeModel.appendChildren', () => { +describe("treeModel.appendChildren", () => { const kid = (id: string): N => ({ id, name: id }); - it('appends to existing children', () => { - const t = treeModel.appendChildren(fixture, 'a', [kid('a3'), kid('a4')]); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'a3', 'a4', + it("appends to existing children", () => { + const t = treeModel.appendChildren(fixture, "a", [kid("a3"), kid("a4")]); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", + "a3", + "a4", ]); }); - it('initializes children when parent had none', () => { - const t = treeModel.appendChildren(fixture, 'b', [kid('b1')]); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['b1']); + it("initializes children when parent had none", () => { + const t = treeModel.appendChildren(fixture, "b", [kid("b1")]); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["b1"]); }); - it('returns same array reference for unknown parentId', () => { - expect(treeModel.appendChildren(fixture, 'ghost', [kid('zz')])).toBe( + it("returns same array reference for unknown parentId", () => { + expect(treeModel.appendChildren(fixture, "ghost", [kid("zz")])).toBe( fixture, ); }); @@ -454,58 +572,60 @@ describe('treeModel.appendChildren', () => { // Regression: lazy-load + auto-expand can race and call appendChildren with // children that overlap what's already there. React then crashes on duplicate // keys. Defensive dedup at the model level. - it('dedups against existing children by id', () => { - const t1 = treeModel.appendChildren(fixture, 'a', [ - kid('a3'), - kid('a4'), + it("dedups against existing children by id", () => { + const t1 = treeModel.appendChildren(fixture, "a", [kid("a3"), kid("a4")]); + const t2 = treeModel.appendChildren(t1, "a", [ + kid("a3"), + kid("a4"), + kid("a5"), ]); - const t2 = treeModel.appendChildren(t1, 'a', [ - kid('a3'), - kid('a4'), - kid('a5'), - ]); - expect(treeModel.find(t2, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'a3', 'a4', 'a5', + expect(treeModel.find(t2, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", + "a3", + "a4", + "a5", ]); }); - it('returns same array reference when every child is a duplicate', () => { - const t1 = treeModel.appendChildren(fixture, 'a', [kid('a3')]); - const t2 = treeModel.appendChildren(t1, 'a', [kid('a3')]); + it("returns same array reference when every child is a duplicate", () => { + const t1 = treeModel.appendChildren(fixture, "a", [kid("a3")]); + const t2 = treeModel.appendChildren(t1, "a", [kid("a3")]); expect(t2).toBe(t1); }); }); -describe('treeModel.place', () => { - it('moves a node to a new parent at a given index', () => { - const t = treeModel.place(fixture, 'a2', { parentId: 'b', index: 0 }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual(['a1']); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['a2']); +describe("treeModel.place", () => { + it("moves a node to a new parent at a given index", () => { + const t = treeModel.place(fixture, "a2", { parentId: "b", index: 0 }); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual(["a1"]); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["a2"]); }); - it('moves a node to root', () => { - const t = treeModel.place(fixture, 'a1', { parentId: null, index: 0 }); - expect(t.map((n) => n.id)).toEqual(['a1', 'a', 'b']); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual(['a2']); + it("moves a node to root", () => { + const t = treeModel.place(fixture, "a1", { parentId: null, index: 0 }); + expect(t.map((n) => n.id)).toEqual(["a1", "a", "b"]); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual(["a2"]); }); - it('reorders within the same parent', () => { - const t = treeModel.place(fixture, 'a2', { parentId: 'a', index: 0 }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a2', 'a1', + it("reorders within the same parent", () => { + const t = treeModel.place(fixture, "a2", { parentId: "a", index: 0 }); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a2", + "a1", ]); }); - it('returns same array reference for unknown source', () => { - expect( - treeModel.place(fixture, 'ghost', { parentId: 'a', index: 0 }), - ).toBe(fixture); + it("returns same array reference for unknown source", () => { + expect(treeModel.place(fixture, "ghost", { parentId: "a", index: 0 })).toBe( + fixture, + ); }); - it('returns same array reference for unknown destination parent', () => { + it("returns same array reference for unknown destination parent", () => { expect( - treeModel.place(fixture, 'a1', { parentId: 'ghost', index: 0 }), + treeModel.place(fixture, "a1", { parentId: "ghost", index: 0 }), ).toBe(fixture); }); }); -describe('treeModel.placeByPosition', () => { +describe("treeModel.placeByPosition", () => { // Server-authoritative `moveTreeNode` ships the moved node's fractional // `position`; the receiver must sort it into the correct slot among the new // siblings — NOT drop it at index 0. @@ -513,198 +633,221 @@ describe('treeModel.placeByPosition', () => { const tree: P[] = [ { - id: 'dst', - name: 'DST', - position: 'a0', + id: "dst", + name: "DST", + position: "a0", children: [ - { id: 'c1', name: 'C1', position: 'a1' }, - { id: 'c2', name: 'C2', position: 'a3' }, - { id: 'c3', name: 'C3', position: 'a5' }, + { id: "c1", name: "C1", position: "a1" }, + { id: "c2", name: "C2", position: "a3" }, + { id: "c3", name: "C3", position: "a5" }, ], }, - { id: 'src', name: 'SRC', position: 'a9' }, + { id: "src", name: "SRC", position: "a9" }, ]; - it('places the moved node in the MIDDLE of new siblings by position', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a4', + it("places the moved node in the MIDDLE of new siblings by position", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a4", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'src', 'c3', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "src", + "c3", ]); }); - it('places the moved node at the END when its position sorts last', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a8', + it("places the moved node at the END when its position sorts last", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a8", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'c3', 'src', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "c3", + "src", ]); }); - it('places the moved node at the FRONT only when its position sorts first', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a0', + it("places the moved node at the FRONT only when its position sorts first", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a0", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'src', 'c1', 'c2', 'c3', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "src", + "c1", + "c2", + "c3", ]); }); - it('stamps the authoritative position onto the moved node', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a4', + it("stamps the authoritative position onto the moved node", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a4", }); - expect(treeModel.find(t, 'src')?.position).toBe('a4'); + expect(treeModel.find(t, "src")?.position).toBe("a4"); }); - it('reorders within the same parent by position (not to index 0)', () => { + it("reorders within the same parent by position (not to index 0)", () => { const same: P[] = [ { - id: 'p', - name: 'P', - position: 'a0', + id: "p", + name: "P", + position: "a0", children: [ - { id: 'x', name: 'X', position: 'a1' }, - { id: 'y', name: 'Y', position: 'a2' }, - { id: 'z', name: 'Z', position: 'a3' }, + { id: "x", name: "X", position: "a1" }, + { id: "y", name: "Y", position: "a2" }, + { id: "z", name: "Z", position: "a3" }, ], }, ]; // Move x to between y and z. - const t = treeModel.placeByPosition(same, 'x', { - parentId: 'p', - position: 'a25', + const t = treeModel.placeByPosition(same, "x", { + parentId: "p", + position: "a25", }); - expect(treeModel.find(t, 'p')?.children?.map((n) => n.id)).toEqual([ - 'y', 'x', 'z', + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual([ + "y", + "x", + "z", ]); }); - it('returns same array reference for unknown source', () => { + it("returns same array reference for unknown source", () => { expect( - treeModel.placeByPosition(tree, 'ghost', { parentId: 'dst', position: 'a4' }), + treeModel.placeByPosition(tree, "ghost", { + parentId: "dst", + position: "a4", + }), ).toBe(tree); }); - it('returns same array reference when destination parent is not loaded', () => { + it("returns same array reference when destination parent is not loaded", () => { expect( - treeModel.placeByPosition(tree, 'src', { parentId: 'ghost', position: 'a4' }), + treeModel.placeByPosition(tree, "src", { + parentId: "ghost", + position: "a4", + }), ).toBe(tree); }); - it('moves a node to root by position', () => { + it("moves a node to root by position", () => { const roots: P[] = [ - { id: 'r1', name: 'R1', position: 'a1' }, - { id: 'r2', name: 'R2', position: 'a5' }, + { id: "r1", name: "R1", position: "a1" }, + { id: "r2", name: "R2", position: "a5" }, { - id: 'rp', - name: 'RP', - position: 'a7', - children: [{ id: 'child', name: 'CHILD', position: 'a1' }], + id: "rp", + name: "RP", + position: "a7", + children: [{ id: "child", name: "CHILD", position: "a1" }], }, ]; - const t = treeModel.placeByPosition(roots, 'child', { + const t = treeModel.placeByPosition(roots, "child", { parentId: null, - position: 'a3', + position: "a3", }); - expect(t.map((n) => n.id)).toEqual(['r1', 'child', 'r2', 'rp']); + expect(t.map((n) => n.id)).toEqual(["r1", "child", "r2", "rp"]); }); }); -describe('treeModel.move', () => { - it('reorder-before within same parent: moves source to target index', () => { - const { tree: t, result } = treeModel.move(fixture, 'a2', { - kind: 'reorder-before', - targetId: 'a1', +describe("treeModel.move", () => { + it("reorder-before within same parent: moves source to target index", () => { + const { tree: t, result } = treeModel.move(fixture, "a2", { + kind: "reorder-before", + targetId: "a1", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a2', 'a1', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a2", + "a1", ]); - expect(result).toEqual({ parentId: 'a', index: 0 }); + expect(result).toEqual({ parentId: "a", index: 0 }); }); - it('reorder-after within same parent', () => { - const { tree: t, result } = treeModel.move(fixture, 'a1', { - kind: 'reorder-after', - targetId: 'a2', + it("reorder-after within same parent", () => { + const { tree: t, result } = treeModel.move(fixture, "a1", { + kind: "reorder-after", + targetId: "a2", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a2', 'a1', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a2", + "a1", ]); - expect(result).toEqual({ parentId: 'a', index: 1 }); + expect(result).toEqual({ parentId: "a", index: 1 }); }); - it('make-child appends at end of target children', () => { - const { tree: t, result } = treeModel.move(fixture, 'b', { - kind: 'make-child', - targetId: 'a', + it("make-child appends at end of target children", () => { + const { tree: t, result } = treeModel.move(fixture, "b", { + kind: "make-child", + targetId: "a", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'b', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", + "b", ]); - expect(result).toEqual({ parentId: 'a', index: 2 }); + expect(result).toEqual({ parentId: "a", index: 2 }); }); - it('make-child initializes children when target had none', () => { - const { tree: t, result } = treeModel.move(fixture, 'a2', { - kind: 'make-child', - targetId: 'b', + it("make-child initializes children when target had none", () => { + const { tree: t, result } = treeModel.move(fixture, "a2", { + kind: "make-child", + targetId: "b", }); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['a2']); - expect(result).toEqual({ parentId: 'b', index: 0 }); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["a2"]); + expect(result).toEqual({ parentId: "b", index: 0 }); }); - it('reorder-before across parents', () => { - const { tree: t, result } = treeModel.move(fixture, 'b', { - kind: 'reorder-before', - targetId: 'a1', + it("reorder-before across parents", () => { + const { tree: t, result } = treeModel.move(fixture, "b", { + kind: "reorder-before", + targetId: "a1", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'b', 'a1', 'a2', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "b", + "a1", + "a2", ]); - expect(result).toEqual({ parentId: 'a', index: 0 }); + expect(result).toEqual({ parentId: "a", index: 0 }); }); - it('reorder-after to root', () => { - const { tree: t, result } = treeModel.move(fixture, 'a1', { - kind: 'reorder-after', - targetId: 'a', + it("reorder-after to root", () => { + const { tree: t, result } = treeModel.move(fixture, "a1", { + kind: "reorder-after", + targetId: "a", }); - expect(t.map((n) => n.id)).toEqual(['a', 'a1', 'b']); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual(['a2']); + expect(t.map((n) => n.id)).toEqual(["a", "a1", "b"]); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual(["a2"]); expect(result).toEqual({ parentId: null, index: 1 }); }); - it('no-op when sourceId === targetId', () => { - const out = treeModel.move(fixture, 'a', { - kind: 'make-child', - targetId: 'a', + it("no-op when sourceId === targetId", () => { + const out = treeModel.move(fixture, "a", { + kind: "make-child", + targetId: "a", }); expect(out.tree).toBe(fixture); }); - it('no-op when target is descendant of source', () => { - const out = treeModel.move(fixture, 'a', { - kind: 'make-child', - targetId: 'a1a', + it("no-op when target is descendant of source", () => { + const out = treeModel.move(fixture, "a", { + kind: "make-child", + targetId: "a1a", }); expect(out.tree).toBe(fixture); }); - it('no-op when source is unknown', () => { - const out = treeModel.move(fixture, 'ghost', { - kind: 'reorder-before', - targetId: 'a', + it("no-op when source is unknown", () => { + const out = treeModel.move(fixture, "ghost", { + kind: "reorder-before", + targetId: "a", }); expect(out.tree).toBe(fixture); }); - it('no-op when target is unknown', () => { - const out = treeModel.move(fixture, 'a1', { - kind: 'reorder-before', - targetId: 'ghost', + it("no-op when target is unknown", () => { + const out = treeModel.move(fixture, "a1", { + kind: "reorder-before", + targetId: "ghost", }); expect(out.tree).toBe(fixture); }); - it('cross-parent move does NOT apply the same-parent adjust (no off-by-one)', () => { + it("cross-parent move does NOT apply the same-parent adjust (no off-by-one)", () => { // Source `x3` sits at index 2 in parent `x`; target `y1` sits at index 0 in // parent `y`. sourceInfo.index (2) > info.index (0) AND the parents differ, // so the `sameParent && source.index < info.index` adjust must be 0 — the @@ -712,36 +855,36 @@ describe('treeModel.move', () => { // drop it at a wrong slot / off-by-one). const crossFixture: N[] = [ { - id: 'x', - name: 'X', + id: "x", + name: "X", children: [ - { id: 'x1', name: 'X1' }, - { id: 'x2', name: 'X2' }, - { id: 'x3', name: 'X3' }, + { id: "x1", name: "X1" }, + { id: "x2", name: "X2" }, + { id: "x3", name: "X3" }, ], }, { - id: 'y', - name: 'Y', + id: "y", + name: "Y", children: [ - { id: 'y1', name: 'Y1' }, - { id: 'y2', name: 'Y2' }, + { id: "y1", name: "Y1" }, + { id: "y2", name: "Y2" }, ], }, ]; - const { tree: t, result } = treeModel.move(crossFixture, 'x3', { - kind: 'reorder-before', - targetId: 'y1', + const { tree: t, result } = treeModel.move(crossFixture, "x3", { + kind: "reorder-before", + targetId: "y1", }); - expect(result).toEqual({ parentId: 'y', index: 0 }); - expect(treeModel.find(t, 'y')?.children?.map((n) => n.id)).toEqual([ - 'x3', - 'y1', - 'y2', + expect(result).toEqual({ parentId: "y", index: 0 }); + expect(treeModel.find(t, "y")?.children?.map((n) => n.id)).toEqual([ + "x3", + "y1", + "y2", ]); - expect(treeModel.find(t, 'x')?.children?.map((n) => n.id)).toEqual([ - 'x1', - 'x2', + expect(treeModel.find(t, "x")?.children?.map((n) => n.id)).toEqual([ + "x1", + "x2", ]); }); }); diff --git a/apps/client/src/features/page/tree/model/tree-model.ts b/apps/client/src/features/page/tree/model/tree-model.ts index 2dd100aa..aa13d8b4 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -1,4 +1,4 @@ -import type { TreeNode, SiblingsInfo } from './tree-model.types'; +import type { TreeNode, SiblingsInfo } from "./tree-model.types"; function findInternal( nodes: TreeNode[], @@ -19,7 +19,10 @@ export const treeModel = { return findInternal(tree, id)?.node ?? null; }, - path(tree: TreeNode[], id: string): TreeNode[] | null { + path( + tree: TreeNode[], + id: string, + ): TreeNode[] | null { const found = findInternal(tree, id); if (!found) return null; return [...found.parents, found.node]; @@ -123,6 +126,23 @@ export const treeModel = { return treeModel.insert(tree, null, node, index(tree)); } const parent = treeModel.find(tree, parentId); + // The parent is in the tree but its children have NOT been lazy-loaded yet + // (`children === undefined`, distinct from a loaded-but-empty `[]`). Inserting + // here would MATERIALIZE a misleading partial child list (`[node]`) that + // defeats the lazy-load gate — which fetches only when children are + // absent/empty — so the parent's OTHER real children would never load and the + // moved/added node would be the only one shown (a silent data loss, #159 #1). + // Instead, leave the children unloaded and just flag `hasChildren` so the + // chevron appears; expanding fetches the FULL set (including this node). + if (parent && parent.children === undefined) { + return treeModel.update( + tree, + parentId, + // hasChildren is not part of the generic T constraint; tree nodes carry + // it. Cast narrowly so this stays a single, well-understood exception. + { hasChildren: true } as unknown as Omit, "id" | "children">, + ); + } const kids = (parent?.children as TreeNode[] | undefined) ?? []; return treeModel.insert(tree, parentId, node, index(kids)); }, @@ -203,6 +223,48 @@ export const treeModel = { return touched ? out : tree; }, + // Replace a parent's DIRECT children with the authoritative `fresh` set while + // PRESERVING each surviving child's already-loaded grandchildren (deeper + // expansion). Unlike `appendChildren` (add-only), this DROPS children that are + // no longer present and reorders to `fresh` — so a move/delete/rename that + // happened inside a loaded branch while events were missed (a socket reconnect + // gap) is reflected, not left stale (#159 #8). Only used to reconcile an + // already-loaded branch against a fresh fetch; a parent with no loaded children + // (`children === undefined`) is left untouched (lazy-load handles it). + reconcileChildren( + tree: TreeNode[], + parentId: string, + fresh: TreeNode[], + ): TreeNode[] { + let touched = false; + const walk = (nodes: TreeNode[]): TreeNode[] => + nodes.map((n) => { + if (n.id === parentId) { + // Only reconcile a branch whose children were actually loaded; an + // unloaded parent stays unloaded (lazy-load fetches it fresh later). + if (n.children === undefined) return n; + const prevById = new Map(n.children.map((c) => [c.id, c])); + const merged = fresh.map((f) => { + const prev = prevById.get(f.id); + // Preserve the surviving child's previously loaded grandchildren so + // deeper expansion is not collapsed by the reconcile. + return prev?.children !== undefined + ? { ...f, children: prev.children } + : f; + }); + touched = true; + return { ...n, children: merged }; + } + if (n.children) { + const next = walk(n.children); + if (next !== n.children) return { ...n, children: next }; + } + return n; + }); + const out = walk(tree); + return touched ? out : tree; + }, + place( tree: TreeNode[], sourceId: string, @@ -242,9 +304,10 @@ export const treeModel = { move( tree: TreeNode[], sourceId: string, - op: import('./tree-model.types').DropOp, - ): { tree: TreeNode[]; result: import('./tree-model.types').DropResult } { - if (sourceId === op.targetId) return { tree, result: { parentId: null, index: 0 } }; + op: import("./tree-model.types").DropOp, + ): { tree: TreeNode[]; result: import("./tree-model.types").DropResult } { + if (sourceId === op.targetId) + return { tree, result: { parentId: null, index: 0 } }; if (!treeModel.find(tree, sourceId) || !treeModel.find(tree, op.targetId)) { return { tree, result: { parentId: null, index: 0 } }; } @@ -255,7 +318,7 @@ export const treeModel = { let parentId: string | null; let index: number; - if (op.kind === 'make-child') { + if (op.kind === "make-child") { parentId = op.targetId; const target = treeModel.find(tree, op.targetId)!; index = target.children?.length ?? 0; @@ -264,9 +327,8 @@ export const treeModel = { parentId = info.parentId; const sourceInfo = treeModel.siblingsOf(tree, sourceId)!; const sameParent = sourceInfo.parentId === parentId; - const adjust = - sameParent && sourceInfo.index < info.index ? -1 : 0; - index = info.index + adjust + (op.kind === 'reorder-after' ? 1 : 0); + const adjust = sameParent && sourceInfo.index < info.index ? -1 : 0; + index = info.index + adjust + (op.kind === "reorder-after" ? 1 : 0); } const next = treeModel.place(tree, sourceId, { parentId, index }); diff --git a/apps/client/src/features/page/tree/utils/utils.test.ts b/apps/client/src/features/page/tree/utils/utils.test.ts index 14366b73..4ea181b5 100644 --- a/apps/client/src/features/page/tree/utils/utils.test.ts +++ b/apps/client/src/features/page/tree/utils/utils.test.ts @@ -6,6 +6,8 @@ import { collectBranchIds, openBranches, closeIds, + mergeRootTrees, + loadedOpenBranchIds, } from "./utils"; import type { IPage } from "@/features/page/types/page.types.ts"; import type { SpaceTreeNode } from "@/features/page/tree/types.ts"; @@ -44,10 +46,7 @@ function flatNode( } // Nested SpaceTreeNode factory for collectAllIds / collectBranchIds. -function treeNode( - id: string, - children: SpaceTreeNode[] = [], -): SpaceTreeNode { +function treeNode(id: string, children: SpaceTreeNode[] = []): SpaceTreeNode { return { id, slugId: `slug-${id}`, @@ -94,11 +93,7 @@ describe("collectBranchIds", () => { ]), treeNode("root2", [treeNode("leaf3")]), ]; - expect(collectBranchIds(tree).sort()).toEqual([ - "branch1", - "root", - "root2", - ]); + expect(collectBranchIds(tree).sort()).toEqual(["branch1", "root", "root2"]); }); it("returns [] for a leaf-only tree", () => { @@ -273,3 +268,95 @@ describe("closeIds", () => { expect(twice).toEqual({ keep: true, a: false, b: false }); }); }); + +describe("mergeRootTrees (#159 #2 reconnect reconcile)", () => { + // Root node with a position and optional already-loaded children. + function root( + id: string, + position: string, + children?: SpaceTreeNode[], + ): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: id.toUpperCase(), + icon: undefined, + position, + spaceId: "space-1", + parentPageId: null as unknown as string, + hasChildren: !!children?.length, + children: children as SpaceTreeNode[], + }; + } + + it("DROPS a stale root that is absent from the incoming (authoritative) set", () => { + // 'ghost' was a root before the gap; the server's current roots no longer + // include it (deleted / moved under another page). It must not linger. + const prev = [root("a", "a0"), root("ghost", "a2"), root("b", "a4")]; + const incoming = [root("a", "a0"), root("b", "a4")]; + const merged = mergeRootTrees(prev, incoming); + expect(merged.map((n) => n.id)).toEqual(["a", "b"]); + expect(merged.find((n) => n.id === "ghost")).toBeUndefined(); + }); + + it("PRESERVES a surviving root's lazy-loaded children (subtree not lost on refetch)", () => { + const loadedChild = root("a1", "a0"); + const prev = [root("a", "a0", [loadedChild])]; + // The root query returns only top-level roots (no children). + const incoming = [root("a", "a0")]; + const merged = mergeRootTrees(prev, incoming); + expect(merged[0].children?.map((c) => c.id)).toEqual(["a1"]); + }); + + it("ADDS a new incoming root", () => { + const prev = [root("a", "a0")]; + const incoming = [root("a", "a0"), root("new", "a2")]; + const merged = mergeRootTrees(prev, incoming); + expect(merged.map((n) => n.id)).toEqual(["a", "new"]); + }); + + it("REFRESHES a surviving root's own fields from the incoming copy (e.g. rename)", () => { + const prev = [{ ...root("a", "a0"), name: "OLD" }]; + const incoming = [{ ...root("a", "a0"), name: "NEW" }]; + const merged = mergeRootTrees(prev, incoming); + expect(merged[0].name).toBe("NEW"); + }); +}); + +describe("loadedOpenBranchIds (#159 #8 reconnect refresh targets)", () => { + function n(id: string, children?: SpaceTreeNode[]): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: id.toUpperCase(), + icon: undefined, + position: "a0", + spaceId: "space-1", + parentPageId: null as unknown as string, + hasChildren: !!children, + children: children as SpaceTreeNode[], + }; + } + + it("returns OPEN branches whose children are loaded (array)", () => { + const tree = [n("a", [n("a1")]), n("b", [n("b1")])]; + const ids = loadedOpenBranchIds(tree, new Set(["a"])); + expect(ids).toEqual(["a"]); // b is closed; a is open+loaded + }); + + it("skips an open branch whose children are NOT loaded (undefined)", () => { + const tree = [n("a")]; // children undefined + expect(loadedOpenBranchIds(tree, new Set(["a"]))).toEqual([]); + }); + + it("includes a loaded-but-empty open branch (a child may have been added during the gap)", () => { + const tree = [n("a", [])]; + expect(loadedOpenBranchIds(tree, new Set(["a"]))).toEqual(["a"]); + }); + + it("walks nested open+loaded branches (deep chain refreshes every level)", () => { + const tree = [n("a", [n("a1", [n("a1a")])])]; + const ids = loadedOpenBranchIds(tree, new Set(["a", "a1"])); + expect(ids.sort()).toEqual(["a", "a1"]); + }); +}); diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index 53d787c6..56f6ab02 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -214,21 +214,59 @@ export function appendNodeChildren( } /** - * Merge root nodes; keep existing ones intact, append new ones, + * Reconcile the loaded root nodes to the authoritative INCOMING set (the + * server's complete current roots for the space), preserving any lazy-loaded + * children/subtree of a root that still exists. + * + * This runs only once all root pages are fetched, so `incomingRoots` is the full + * server root set and is authoritative for WHICH roots exist: + * - a root in BOTH: kept, with its own fields refreshed from `incoming` (so a + * rename/move during a gap shows) while PRESERVING its previously lazy-loaded + * `children` (expanded subtrees + open-state survive a refetch); + * - a root only in `incoming`: a new root, added as-is; + * - a root only in `prev`: it was DELETED or moved under another page while we + * were not receiving events (e.g. a socket reconnect after a sleep/wifi gap). + * It is DROPPED instead of lingering as a 404 "ghost" root (#159 #2). The old + * append-only merge kept it forever. */ export function mergeRootTrees( prevRoots: SpaceTreeNode[], incomingRoots: SpaceTreeNode[], ): SpaceTreeNode[] { - const seen = new Set(prevRoots.map((r) => r.id)); + const prevById = new Map(prevRoots.map((r) => [r.id, r])); - // add new roots that were not present before - const merged = [...prevRoots]; - incomingRoots.forEach((node) => { - if (!seen.has(node.id)) merged.push(node); + const reconciled = incomingRoots.map((incoming) => { + const prev = prevById.get(incoming.id); + // Preserve the previously loaded children/subtree (the root query returns + // only top-level roots, so `incoming` carries no children); refresh the + // node's own fields from the authoritative incoming copy. + return prev ? { ...incoming, children: prev.children } : incoming; }); - return sortPositionKeys(merged); + return sortPositionKeys(reconciled); +} + +/** + * Ids of branches a socket-reconnect refresh should re-fetch and reconcile + * (#159 #8): a node that is currently OPEN and whose children are LOADED + * (`children` is an array — possibly empty). An unloaded branch (`children === + * undefined`) is skipped because lazy-load fetches it fresh on the next expand, + * so there is nothing stale to reconcile. Walks the whole tree (a deep open + * chain refreshes every loaded level). + */ +export function loadedOpenBranchIds( + tree: SpaceTreeNode[], + openIds: ReadonlySet, +): string[] { + const ids: string[] = []; + const walk = (nodes: SpaceTreeNode[]) => { + for (const n of nodes) { + if (openIds.has(n.id) && Array.isArray(n.children)) ids.push(n.id); + if (n.children) walk(n.children); + } + }; + walk(tree); + return ids; } // Collect every node id in the tree (roots, branches, leaves). Used by diff --git a/apps/client/src/features/websocket/tree-socket-reducers.test.ts b/apps/client/src/features/websocket/tree-socket-reducers.test.ts index 52228949..f59f27cc 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.test.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.test.ts @@ -81,6 +81,38 @@ describe("applyMoveTreeNode", () => { ]); }); + it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159)", () => { + // `dstCollapsed` is in the tree but its children were never lazy-loaded + // (children === undefined). The OLD behavior inserted `src` as the ONLY + // child ([src]), which defeated the lazy-load gate and HID the parent's + // other real children. Now the move leaves children unloaded (so expanding + // fetches the FULL set, including src) and just flags hasChildren. + const tree: SpaceTreeNode[] = [ + node("dstCollapsed", { + position: "a0", + hasChildren: false, + children: undefined as unknown as SpaceTreeNode[], + }), + node("src", { position: "a9" }), + ]; + const next = applyMoveTreeNode(tree, { + id: "src", + parentId: "dstCollapsed", + oldParentId: null, + index: 0, + position: "a4", + pageData: {}, + }); + const dst = treeModel.find(next, "dstCollapsed"); + // Children stay unloaded -> the lazy-load gate fetches the FULL set (incl. + // src) on expand, rather than showing a misleading partial [src] list. + expect(dst?.children).toBeUndefined(); + expect(dst?.hasChildren).toBe(true); + // src moved away from its old root slot (it lives under dstCollapsed + // server-side and reappears when the parent is expanded/loaded). + expect(next.map((n) => n.id)).not.toContain("src"); + }); + it("flips the OLD parent's hasChildren to false when it is left childless", () => { // src is the only child of `old`; moving it to `dst` empties `old`. const tree: SpaceTreeNode[] = [ @@ -164,7 +196,9 @@ describe("applyDeleteTreeNode", () => { position: "a1", parentPageId: "p", hasChildren: true, - children: [node("grandchild", { position: "a1", parentPageId: "child" })], + children: [ + node("grandchild", { position: "a1", parentPageId: "child" }), + ], }), ], }), diff --git a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx index a3d07a94..f3beb39b 100644 --- a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx +++ b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx @@ -11,6 +11,7 @@ import { Switch, TagsInput, Text, + Textarea, TextInput, } from "@mantine/core"; import { useForm } from "@mantine/form"; @@ -35,6 +36,8 @@ const formSchema = z.object({ // Write-only secret buffer. Empty string means "do not change" (unless cleared). authHeader: z.string(), toolAllowlist: z.array(z.string()), + // Admin-authored prompt guidance (#180). Capped to mirror the DTO MaxLength. + instructions: z.string().max(4000), enabled: z.boolean(), }); @@ -63,6 +66,7 @@ function buildInitialValues(server?: IAiMcpServer): FormValues { toolAllowlist: Array.isArray(server?.toolAllowlist) ? server.toolAllowlist : [], + instructions: server?.instructions ?? "", enabled: server?.enabled ?? true, }; } @@ -124,6 +128,8 @@ export default function AiMcpServerForm({ transport: values.transport, url: values.url, toolAllowlist: values.toolAllowlist, + // Always sent: a blank value clears the stored guidance (server -> null). + instructions: values.instructions, enabled: values.enabled, }; // Only attach headers when set or explicitly cleared (omit => unchanged). @@ -135,6 +141,8 @@ export default function AiMcpServerForm({ transport: values.transport, url: values.url, toolAllowlist: values.toolAllowlist, + // Blank => server stores null (no guidance). + instructions: values.instructions, enabled: values.enabled, }; // On create, only a typed value matters (no prior stored headers). @@ -158,10 +166,7 @@ export default function AiMcpServerForm({ return ( - +