fix(review): address PR #230 review — payload type, breadcrumb helper, tests

Review follow-ups for the combined QA-UI fixes (#216/#206/#204/#218/#192):

- export/utils: correct the misleading getInternalLinkPageName comment — a
  bare `v1.2` loses its last dot-segment (`v1`); dots survive only in
  multi-segment names like `v1.2.md` -> `v1.2`.
- share: extract toPublicSharePayload(page, share): PublicSharePayload, an
  explicit allowlist type+mapper replacing the inline literal in the
  /shares/page-info anonymous path (#218). Add share.controller.spec.ts that
  stubs getSharedPage returning internal fields and asserts the response key
  set EXACTLY equals the whitelist (page + share), so any `...shareData`
  regression or new leaking field fails. Also key-tests the extracted mapper.
- breadcrumb: extract pure resolveBreadcrumbNodes(treeData, ancestors, pageId)
  (tree-hit -> tree; tree-miss -> map ancestors via canonical pageToTreeNode,
  dropping the as-any casts; else null) and unit-test all three branches.
- share-modal: RTL test asserting enabling a share calls mutateAsync with
  includeSubPages: false (#216 security default).
- share.service: one-line note at getSharedPage on the deferred consolidation
  of the ancestor-aware match into resolveReadableSharePage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/client/src/features/page/components/breadcrumbs/breadcrumb.tsx

@@ -1,7 +1,7 @@
 import { useAtomValue } from "jotai";
 import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts";
 import React, { useCallback, useEffect, useState } from "react";
-import { findBreadcrumbPath } from "@/features/page/tree/utils";
+import { resolveBreadcrumbNodes } from "./breadcrumb.utils";
 import {
   Button,
   Anchor,
@@ -15,6 +15,7 @@ import { IconCornerDownRightDouble, IconDots } from "@tabler/icons-react";
 import { Link, useParams } from "react-router-dom";
 import classes from "./breadcrumb.module.css";
 import { SpaceTreeNode } from "@/features/page/tree/types.ts";
+import { IPage } from "@/features/page/types/page.types.ts";
 import { buildPageUrl } from "@/features/page/page.utils.ts";
 import {
   usePageQuery,
@@ -50,32 +51,16 @@ export default function Breadcrumb() {
   useEffect(() => {
     if (!currentPage) return;
 
-    // Prefer the sidebar tree once it actually contains this page's ancestor
-    // chain — it stays live with renames/moves happening in the sidebar.
-    if (treeData?.length > 0) {
-      const breadcrumb = findBreadcrumbPath(treeData, currentPage.id);
-      if (breadcrumb) {
-        setBreadcrumbNodes(breadcrumb);
-        return;
-      }
-    }
-
-    // Otherwise fall back to the page's own ancestor data so the breadcrumb
-    // resolves immediately instead of staying blank.
-    if (ancestors?.length) {
-      setBreadcrumbNodes(
-        (ancestors as any[]).map((node) => ({
-          id: node.id,
-          slugId: node.slugId,
-          name: node.title,
-          icon: node.icon,
-          position: node.position,
-          spaceId: node.spaceId,
-          parentPageId: node.parentPageId,
-          hasChildren: node.hasChildren ?? false,
-          children: [],
-        })) as SpaceTreeNode[],
+    // Selection/mapping lives in a pure, unit-tested helper (#218). Only update
+    // when it resolves nodes so a transient miss keeps the prior breadcrumb
+    // rather than blanking it.
+    const nodes = resolveBreadcrumbNodes(
+      treeData,
+      ancestors as IPage[] | undefined,
+      currentPage.id,
     );
+    if (nodes) {
+      setBreadcrumbNodes(nodes);
     }
   }, [currentPage?.id, treeData, ancestors]);
 

apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.test.ts

@@ -0,0 +1,81 @@
+import { describe, it, expect } from "vitest";
+import { resolveBreadcrumbNodes } from "./breadcrumb.utils";
+import { SpaceTreeNode } from "@/features/page/tree/types.ts";
+import { IPage } from "@/features/page/types/page.types.ts";
+
+// Pure selection/mapping behind the breadcrumb (#218): tree-hit prefers the live
+// sidebar tree, tree-miss maps the page's own ancestors, and "no data" returns
+// null so the component keeps its prior state.
+
+function treeNode(id: string, over?: Partial<SpaceTreeNode>): SpaceTreeNode {
+  return {
+    id,
+    slugId: `slug-${id}`,
+    name: `node-${id}`,
+    icon: null,
+    position: "a",
+    hasChildren: false,
+    spaceId: "space-1",
+    parentPageId: null,
+    children: [],
+    ...over,
+  } as SpaceTreeNode;
+}
+
+function ancestorPage(id: string, over?: Partial<IPage>): IPage {
+  return {
+    id,
+    slugId: `slug-${id}`,
+    title: `title-${id}`,
+    icon: "📄",
+    position: "m",
+    spaceId: "space-1",
+    parentPageId: null,
+    hasChildren: true,
+    ...over,
+  } as IPage;
+}
+
+describe("resolveBreadcrumbNodes", () => {
+  it("tree-hit: returns the path found in the live sidebar tree", () => {
+    const child = treeNode("child");
+    const root = treeNode("root", { hasChildren: true, children: [child] });
+    // findBreadcrumbPath walks the tree; the chain ends at the target page.
+    const result = resolveBreadcrumbNodes([root], [ancestorPage("child")], "child");
+
+    expect(result).not.toBeNull();
+    expect(result!.map((n) => n.id)).toEqual(["root", "child"]);
+    // Came from the tree, NOT the ancestor mapping (icon stays the tree's null).
+    expect(result![result!.length - 1].icon).toBeNull();
+  });
+
+  it("tree-miss: maps the page's own ancestors (title->name, hasChildren default)", () => {
+    // Tree has no node for the target page -> findBreadcrumbPath misses.
+    const unrelated = treeNode("unrelated");
+    const ancestors = [
+      ancestorPage("a", { hasChildren: true }),
+      ancestorPage("b", { hasChildren: undefined as any }),
+    ];
+
+    const result = resolveBreadcrumbNodes([unrelated], ancestors, "missing-page");
+
+    expect(result).not.toBeNull();
+    expect(result!.map((n) => n.id)).toEqual(["a", "b"]);
+    // Non-trivial field transform: title -> name.
+    expect(result![0].name).toBe("title-a");
+    // hasChildren defaults to false when the ancestor row omits it.
+    expect(result![1].hasChildren).toBe(false);
+    expect(result![0].hasChildren).toBe(true);
+  });
+
+  it("falls back to ancestors when the tree is empty", () => {
+    const result = resolveBreadcrumbNodes([], [ancestorPage("a")], "a");
+    expect(result!.map((n) => n.id)).toEqual(["a"]);
+  });
+
+  it("returns null when there is no tree hit and no ancestor data", () => {
+    expect(resolveBreadcrumbNodes([], [], "x")).toBeNull();
+    expect(resolveBreadcrumbNodes(undefined, undefined, "x")).toBeNull();
+    expect(resolveBreadcrumbNodes(null, null, "x")).toBeNull();
+  });
+});

apps/client/src/features/page/components/breadcrumbs/breadcrumb.utils.ts

@@ -0,0 +1,34 @@
+import { IPage } from "@/features/page/types/page.types.ts";
+import { SpaceTreeNode } from "@/features/page/tree/types.ts";
+import { findBreadcrumbPath, pageToTreeNode } from "@/features/page/tree/utils";
+
+/**
+ * Pure selection/mapping for the breadcrumb nodes (#218). Three branches:
+ *   1. tree-hit  — the lazily-built sidebar tree already contains this page's
+ *      ancestor chain, so prefer it (stays live with sidebar renames/moves).
+ *   2. tree-miss — fall back to the page's own ancestor data so a deep page
+ *      resolves immediately instead of rendering a blank breadcrumb for seconds
+ *      while the tree backfills. Mapped through the canonical `pageToTreeNode`
+ *      (title -> name, hasChildren defaulted to false).
+ *   3. neither   — no data yet, return null so the caller keeps its prior state.
+ */
+export function resolveBreadcrumbNodes(
+  treeData: SpaceTreeNode[] | null | undefined,
+  ancestors: IPage[] | null | undefined,
+  pageId: string,
+): SpaceTreeNode[] | null {
+  if (treeData && treeData.length > 0) {
+    const breadcrumb = findBreadcrumbPath(treeData, pageId);
+    if (breadcrumb) {
+      return breadcrumb;
+    }
+  }
+
+  if (ancestors && ancestors.length > 0) {
+    return ancestors.map((page) =>
+      pageToTreeNode(page, { hasChildren: page.hasChildren ?? false }),
+    );
+  }
+
+  return null;
+}

apps/client/src/features/share/components/share-modal.test.tsx

@@ -0,0 +1,74 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { render, screen, fireEvent, waitFor } from "@testing-library/react";
+import { MantineProvider } from "@mantine/core";
+import { MemoryRouter } from "react-router-dom";
+
+// matchMedia / storage are stubbed globally in vitest.setup.ts.
+
+// Enabling a public share must NOT silently expose the whole sub-tree (#216):
+// the create call defaults includeSubPages to false. This was a one-literal,
+// security-relevant default with no test — lock it.
+
+const createMutateAsync = vi.fn(async () => ({}));
+const deleteMutateAsync = vi.fn(async () => ({}));
+
+// No existing share for this page (toggle starts OFF).
+let shareData: any = undefined;
+
+vi.mock("react-i18next", () => ({
+  useTranslation: () => ({ t: (key: string) => key }),
+}));
+
+vi.mock("@/features/share/queries/share-query.ts", () => ({
+  useCreateShareMutation: () => ({ mutateAsync: createMutateAsync }),
+  useDeleteShareMutation: () => ({ mutateAsync: deleteMutateAsync }),
+  useUpdateShareMutation: () => ({ mutateAsync: vi.fn() }),
+  useShareForPageQuery: () => ({ data: shareData }),
+}));
+
+vi.mock("@/features/page/queries/page-query.ts", () => ({
+  usePageQuery: () => ({ data: { id: "page-1", title: "Doc" } }),
+}));
+
+vi.mock("@/features/space/queries/space-query.ts", () => ({
+  useSpaceQuery: () => ({ data: { settings: {} } }),
+}));
+
+import ShareModal from "./share-modal";
+
+function renderModal() {
+  return render(
+    <MemoryRouter>
+      <MantineProvider>
+        <ShareModal readOnly={false} />
+      </MantineProvider>
+    </MemoryRouter>,
+  );
+}
+
+describe("ShareModal — enabling a share defaults includeSubPages to false (#216)", () => {
+  beforeEach(() => {
+    createMutateAsync.mockClear();
+    deleteMutateAsync.mockClear();
+    shareData = undefined;
+  });
+
+  it("creates the share with includeSubPages: false when the user turns it on", async () => {
+    renderModal();
+
+    // Open the share popover.
+    fireEvent.click(screen.getByRole("button", { name: "Share" }));
+
+    // The "Share to web" toggle is the only switch in the not-yet-shared state.
+    const toggle = await screen.findByRole("switch");
+    fireEvent.click(toggle);
+
+    await waitFor(() => expect(createMutateAsync).toHaveBeenCalledTimes(1));
+    expect(createMutateAsync).toHaveBeenCalledWith(
+      expect.objectContaining({
+        pageId: "page-1",
+        includeSubPages: false,
+      }),
+    );
+  });
+});

apps/server/src/core/share/share-public-payload.ts

@@ -0,0 +1,69 @@
+import { Page } from '@docmost/db/types/entity.types';
+
+/**
+ * The EXACT shape returned to anonymous public-share viewers by the
+ * `/shares/page-info` route — the only unauthenticated path that serializes the
+ * full {page, share} records. This is a security boundary (#218): the raw rows
+ * carry internal metadata — creatorId/lastUpdatedById/contributorIds,
+ * spaceId/workspaceId, AI/source bookkeeping, lock/template flags,
+ * parent/position and raw timestamps — none of which may leak to an
+ * unauthenticated viewer. Keeping the allowlist as an explicit TYPE plus a
+ * single mapper means a new leaking field cannot be returned without also
+ * widening this contract (and tripping its key-test in share.controller.spec.ts).
+ */
+export interface PublicSharePayload {
+  page: {
+    id: string;
+    slugId: string;
+    title: string | null;
+    icon: string | null;
+    content: unknown;
+  };
+  share: {
+    id: string;
+    key: string;
+    includeSubPages: boolean | null;
+    searchIndexing: boolean | null;
+    level: number;
+    sharedPage: unknown;
+  };
+}
+
+/**
+ * The subset of the resolved share read by the public payload. Declared
+ * structurally so the richer getShareForPage result (which adds `level` and
+ * `sharedPage` on top of the base Shares row) passes without a cast.
+ */
+interface PublicShareSource {
+  id: string;
+  key: string;
+  includeSubPages: boolean | null;
+  searchIndexing: boolean | null;
+  // `level` is derived via a SQL literal in getShareForPage, so it surfaces as
+  // `unknown` in the resolved share; it is a number at runtime.
+  level: unknown;
+  sharedPage: unknown;
+}
+
+export function toPublicSharePayload(
+  page: Page,
+  share: PublicShareSource,
+): PublicSharePayload {
+  return {
+    page: {
+      id: page.id,
+      slugId: page.slugId,
+      title: page.title,
+      icon: page.icon,
+      content: page.content,
+    },
+    share: {
+      id: share.id,
+      key: share.key,
+      includeSubPages: share.includeSubPages,
+      searchIndexing: share.searchIndexing,
+      level: share.level as number,
+      sharedPage: share.sharedPage,
+    },
+  };
+}

apps/server/src/core/share/share.controller.spec.ts

@@ -0,0 +1,190 @@
+import { ShareController } from './share.controller';
+import {
+  PublicSharePayload,
+  toPublicSharePayload,
+} from './share-public-payload';
+
+// The `/shares/page-info` route is the ONLY anonymous path that serializes the
+// full {page, share} records. Trimming the response to an explicit allowlist is
+// a security control (#218): a regression that returns `...shareData` (or adds a
+// new field to the allowlist) must fail loudly. These tests lock the exact key
+// set returned to anonymous viewers so internal metadata can never silently leak.
+
+const PAGE_KEYS = ['id', 'slugId', 'title', 'icon', 'content'].sort();
+const SHARE_KEYS = [
+  'id',
+  'key',
+  'includeSubPages',
+  'searchIndexing',
+  'level',
+  'sharedPage',
+].sort();
+
+// A page row carrying internal metadata that MUST NOT reach anonymous viewers.
+function internalPage() {
+  return {
+    id: 'page-1',
+    slugId: 'slug-1',
+    title: 'Public Title',
+    icon: '📄',
+    content: { type: 'doc', content: [] },
+    // --- leaky internals ---
+    creatorId: 'user-1',
+    lastUpdatedById: 'user-2',
+    contributorIds: ['user-1', 'user-2'],
+    spaceId: 'space-1',
+    workspaceId: 'ws-1',
+    parentPageId: 'parent-1',
+    position: 'aa',
+    isLocked: true,
+    isTemplate: false,
+    textContent: 'secret text content',
+    ydoc: Buffer.from('binary'),
+    createdAt: new Date('2020-01-01'),
+    updatedAt: new Date('2020-01-02'),
+    deletedAt: null,
+  } as any;
+}
+
+// A resolved share carrying internal metadata.
+function internalShare() {
+  return {
+    id: 'share-1',
+    key: 'share-key',
+    includeSubPages: false,
+    searchIndexing: true,
+    level: 0,
+    sharedPage: { id: 'page-1', slugId: 'slug-1', title: 'Public Title' },
+    // --- leaky internals ---
+    creatorId: 'user-1',
+    spaceId: 'space-1',
+    workspaceId: 'ws-1',
+    pageId: 'page-1',
+    createdAt: new Date('2020-01-01'),
+    updatedAt: new Date('2020-01-02'),
+    deletedAt: null,
+  } as any;
+}
+
+function buildController(over?: { aiAssistant?: boolean }) {
+  const shareService = {
+    // Deliberately returns the FULL internal records (as the real service does).
+    getSharedPage: jest.fn(async () => ({
+      page: internalPage(),
+      share: internalShare(),
+    })),
+    isSharingAllowed: jest.fn(async () => true),
+  };
+  const aiSettings = {
+    isPublicShareAssistantEnabled: jest.fn(
+      async () => over?.aiAssistant ?? false,
+    ),
+    resolvePublicShareAssistantName: jest.fn(async () => 'Assistant'),
+  };
+  const licenseCheckService = {
+    resolveFeatures: jest.fn(() => ({ tier: 'free' })),
+  };
+
+  const controller = new ShareController(
+    shareService as any,
+    {} as any, // shareRepo
+    {} as any, // pageRepo
+    {} as any, // pagePermissionRepo
+    {} as any, // pageAccessService
+    licenseCheckService as any,
+    aiSettings as any,
+    {} as any, // auditService
+  );
+
+  return { controller, shareService, aiSettings, licenseCheckService };
+}
+
+const workspace = {
+  id: 'ws-1',
+  licenseKey: null,
+  plan: 'free',
+} as any;
+
+describe('ShareController.getSharedPageInfo — public payload whitelist (#218)', () => {
+  it('returns EXACTLY the page allowlist keys (no leaked internals)', async () => {
+    const { controller } = buildController();
+
+    const res = await controller.getSharedPageInfo(
+      { pageId: 'page-1' } as any,
+      workspace,
+    );
+
+    expect(Object.keys(res.page).sort()).toEqual(PAGE_KEYS);
+    for (const leaked of [
+      'creatorId',
+      'lastUpdatedById',
+      'contributorIds',
+      'spaceId',
+      'workspaceId',
+      'parentPageId',
+      'position',
+      'textContent',
+      'ydoc',
+      'createdAt',
+      'updatedAt',
+      'deletedAt',
+    ]) {
+      expect((res.page as any)[leaked]).toBeUndefined();
+    }
+    // The serialized payload must not carry the secret text content either.
+    expect(JSON.stringify(res.page)).not.toContain('secret text content');
+  });
+
+  it('returns EXACTLY the share allowlist keys (no leaked internals)', async () => {
+    const { controller } = buildController();
+
+    const res = await controller.getSharedPageInfo(
+      { pageId: 'page-1' } as any,
+      workspace,
+    );
+
+    expect(Object.keys(res.share).sort()).toEqual(SHARE_KEYS);
+    for (const leaked of [
+      'creatorId',
+      'spaceId',
+      'workspaceId',
+      'pageId',
+      'createdAt',
+      'updatedAt',
+      'deletedAt',
+    ]) {
+      expect((res.share as any)[leaked]).toBeUndefined();
+    }
+  });
+
+  it('surfaces the public AI-assistant flags and license features alongside the trimmed payload', async () => {
+    const { controller } = buildController({ aiAssistant: true });
+
+    const res = await controller.getSharedPageInfo(
+      { pageId: 'page-1' } as any,
+      workspace,
+    );
+
+    expect(res.aiAssistant).toBe(true);
+    expect(res.aiAssistantName).toBe('Assistant');
+    expect(res.features).toEqual({ tier: 'free' });
+    // Top-level keys are limited to the trimmed payload + the public extras.
+    expect(Object.keys(res).sort()).toEqual(
+      ['page', 'share', 'aiAssistant', 'aiAssistantName', 'features'].sort(),
+    );
+  });
+});
+
+describe('toPublicSharePayload — key set is the contract', () => {
+  it('copies only the allowlisted page/share keys', () => {
+    const payload: PublicSharePayload = toPublicSharePayload(
+      internalPage(),
+      internalShare(),
+    );
+
+    expect(Object.keys(payload.page).sort()).toEqual(PAGE_KEYS);
+    expect(Object.keys(payload.share).sort()).toEqual(SHARE_KEYS);
+    expect(payload.page.id).toBe('page-1');
+    expect(payload.share.key).toBe('share-key');
+  });
+});

apps/server/src/core/share/share.controller.ts

@@ -36,6 +36,7 @@ import {
   IAuditService,
 } from '../../integrations/audit/audit.service';
 import { AiSettingsService } from '../../integrations/ai/ai-settings.service';
+import { toPublicSharePayload } from './share-public-payload';
 
 @UseGuards(JwtAuthGuard)
 @Controller('shares')
@@ -93,30 +94,13 @@ export class ShareController {
       ? await this.aiSettings.resolvePublicShareAssistantName(workspace.id)
       : null;
 
-    // Trim the public payload to what the anonymous renderer actually needs
-    // (#218). Internal metadata — creatorId/lastUpdatedById/contributorIds,
-    // spaceId/workspaceId, AI/source bookkeeping, lock/template flags,
-    // parent/position, raw timestamps — must not leak to anonymous viewers.
+    // Trim the public payload to the explicit allowlist the anonymous renderer
+    // needs (#218); the PublicSharePayload type + mapper guarantee internal
+    // metadata can never leak to anonymous viewers (see share-public-payload.ts).
     const { page, share } = shareData;
-    const publicPage = {
-      id: page.id,
-      slugId: page.slugId,
-      title: page.title,
-      icon: page.icon,
-      content: page.content,
-    };
-    const publicShare = {
-      id: share.id,
-      key: share.key,
-      includeSubPages: share.includeSubPages,
-      searchIndexing: share.searchIndexing,
-      level: share.level,
-      sharedPage: share.sharedPage,
-    };
 
     return {
-      page: publicPage,
-      share: publicShare,
+      ...toPublicSharePayload(page, share),
       aiAssistant,
       aiAssistantName,
       features: this.licenseCheckService.resolveFeatures(

apps/server/src/core/share/share.service.ts

@@ -213,6 +213,11 @@ export class ShareService {
     // request with no shareId keeps the legacy slug-capability behavior (the
     // `/share/p/:slug` route + internal title look-ups); the slug nanoid stays
     // the access secret there — an inherited Docmost design we don't widen.
+    // FUTURE: this ancestor-aware match could fold INTO resolveReadableSharePage
+    // (so the boundary's narrow `share.id === shareId` gate isn't effectively
+    // dead). Deferred — it widens the contract for the 4 other callers that pass
+    // no shareId, so kept here as a local post-check until that's worth the blast
+    // radius.
     if (dto.shareId) {
       const reachable = await this.isPageReachableThroughShare(
         dto.shareId,

apps/server/src/integrations/export/utils.ts

@@ -109,8 +109,10 @@ export function getInternalLinkPageName(path: string, currentFilePath?: string):
   // Strip a trailing file extension from the basename, but only when there IS
   // one: an extensionless link target (e.g. "My Page") has no extension to drop,
   // so `split('.').slice(0,-1)` would otherwise collapse it to an empty string,
-  // producing an internal link with no visible text (#204 export bug). Dotted
-  // page names without an extension (e.g. "v1.2") keep their dots.
+  // producing an internal link with no visible text (#204 export bug). The last
+  // dot-segment is always treated as an extension and dropped whenever there is
+  // more than one segment, so dots are preserved only in multi-segment names
+  // like `v1.2.md` -> `v1.2`; a bare `v1.2` becomes `v1`.
   const base = path?.split('/').pop();
   const parts = base?.split('.');
   const name = parts && parts.length > 1 ? parts.slice(0, -1).join('.') : base;