From 859223db1a341596e32d7dd50674ff1aca43b41c Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:15:43 +0300 Subject: [PATCH 01/11] fix(page-templates): show a template marker icon in the page tree (#38) Template pages were toggleable but indistinguishable in the sidebar tree. Render an IconTemplate next to the title when node.isTemplate is true, wrapped in a Tooltip(label='Template') with an aria-label + role='img' for AT. The icon is a child of the row Link so clicks navigate as normal; pointer events stay enabled so the tooltip's hover handlers fire. Adds the 'Template' i18n key to en-US and ru-RU (other locales fall back to en-US). Co-Authored-By: Claude Opus 4.8 --- .../public/locales/en-US/translation.json | 1 + .../public/locales/ru-RU/translation.json | 1 + .../page/tree/components/space-tree-row.tsx | 22 ++++++++++++++++++- 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index c04fc72d..651800ee 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -183,6 +183,7 @@ "Successfully imported": "Successfully imported", "Successfully restored": "Successfully restored", "System settings": "System settings", + "Template": "Template", "Templates": "Templates", "Theme": "Theme", "To change your email, you have to enter your password and new email.": "To change your email, you have to enter your password and new email.", diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 25ff2530..238c42fd 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -183,6 +183,7 @@ "Successfully imported": "Успешно импортировано", "Successfully restored": "Успешно восстановлено", "System settings": "Системные настройки", + "Template": "Шаблон", "Templates": "Шаблоны", "Theme": "Тема", "To change your email, you have to enter your password and new email.": "Чтобы изменить электронную почту, вам нужно ввести пароль и новый адрес.", diff --git a/apps/client/src/features/page/tree/components/space-tree-row.tsx b/apps/client/src/features/page/tree/components/space-tree-row.tsx index df371498..c5c08b1c 100644 --- a/apps/client/src/features/page/tree/components/space-tree-row.tsx +++ b/apps/client/src/features/page/tree/components/space-tree-row.tsx @@ -2,13 +2,14 @@ import { useRef } from "react"; import { Link, useParams } from "react-router-dom"; import { useAtom } from "jotai"; import { useTranslation } from "react-i18next"; -import { ActionIcon, rem } from "@mantine/core"; +import { ActionIcon, rem, Tooltip } from "@mantine/core"; import { IconChevronDown, IconChevronRight, IconFileDescription, IconPlus, IconPointFilled, + IconTemplate, } from "@tabler/icons-react"; import EmojiPicker from "@/components/ui/emoji-picker.tsx"; @@ -171,6 +172,25 @@ export function SpaceTreeRow({ {node.name || t("untitled")} + {node.isTemplate === true && ( + + + + )} +
From c9eb495688b49c508afe423b8d77e55a436af4f6 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:21:32 +0300 Subject: [PATCH 02/11] fix(page-templates): clean up page-embed node chrome (#39) Two design problems on the whole-page embed (pageEmbed) node: - Double selection frame: the generic square cyan .ProseMirror-selectednode outline stacked on top of the rounded .includeWrap border. Add node-pageEmbed to the existing outline:none rule (already covering the transclusion nodes) so only the single rounded border remains. - Redundant 'open source' controls: the floating toolbar's external-link button duplicated the header badge title link. Remove the toolbar button; the badge title is now the single way to open the source (kept Refresh + ... menu). Also swap the badge fallback icon IconArrowsMaximize (read as 'expand') for a neutral IconFileText. Follow-ups from review: render the badge whenever the source resolves (so the only open-source link can't vanish when title+icon are empty), and label the link (title/aria-label) + add the 'Open source page' i18n key (en-US, ru-RU). Co-Authored-By: Claude Opus 4.8 --- .../public/locales/en-US/translation.json | 1 + .../public/locales/ru-RU/translation.json | 1 + .../components/page-embed/page-embed-view.tsx | 26 ++++++------------- .../transclusion/transclusion.module.css | 3 ++- 4 files changed, 12 insertions(+), 19 deletions(-) diff --git a/apps/client/public/locales/en-US/translation.json b/apps/client/public/locales/en-US/translation.json index 651800ee..0f6a1a9f 100644 --- a/apps/client/public/locales/en-US/translation.json +++ b/apps/client/public/locales/en-US/translation.json @@ -474,6 +474,7 @@ "Make sub-pages public too": "Make sub-pages public too", "Allow search engines to index page": "Allow search engines to index page", "Open page": "Open page", + "Open source page": "Open source page", "Page": "Page", "Delete public share link": "Delete public share link", "Delete share": "Delete share", diff --git a/apps/client/public/locales/ru-RU/translation.json b/apps/client/public/locales/ru-RU/translation.json index 238c42fd..ef5d3dc7 100644 --- a/apps/client/public/locales/ru-RU/translation.json +++ b/apps/client/public/locales/ru-RU/translation.json @@ -472,6 +472,7 @@ "Make sub-pages public too": "Сделать подстраницы тоже общедоступными", "Allow search engines to index page": "Разрешить поисковым системам индексировать страницу", "Open page": "Открыть страницу", + "Open source page": "Открыть исходную страницу", "Page": "Страница", "Delete public share link": "Удалить публичную ссылку", "Delete share": "Удалить общий доступ", diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx index 63890eec..707a051f 100644 --- a/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx +++ b/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx @@ -2,10 +2,9 @@ import { NodeViewProps, NodeViewWrapper } from "@tiptap/react"; import { ActionIcon, Menu, Tooltip } from "@mantine/core"; import { IconAlertTriangle, - IconArrowsMaximize, IconDots, - IconExternalLink, IconEyeOff, + IconFileText, IconInfoCircle, IconRefresh, IconRepeat, @@ -136,20 +135,6 @@ function PageEmbedBody({ - {sourceHref && ( - - - - - - )} @@ -170,13 +155,18 @@ function PageEmbedBody({ ) : null; const header = - sourceTitle || sourceIcon ? ( + // Render the badge whenever the source resolves (sourceHref), not only when + // it has a title/icon — the title link is now the single way to open the + // source, so it must not disappear when title and icon are both empty. + sourceTitle || sourceIcon || sourceHref ? (
- {sourceIcon ? `${sourceIcon} ` : } + {sourceIcon ? `${sourceIcon} ` : } {sourceHref ? ( {sourceTitle || t("Untitled")} diff --git a/apps/client/src/features/editor/components/transclusion/transclusion.module.css b/apps/client/src/features/editor/components/transclusion/transclusion.module.css index 4d8d321a..168da0c7 100644 --- a/apps/client/src/features/editor/components/transclusion/transclusion.module.css +++ b/apps/client/src/features/editor/components/transclusion/transclusion.module.css @@ -183,7 +183,8 @@ } :global(.react-renderer.node-transclusionSource.ProseMirror-selectednode), -:global(.react-renderer.node-transclusionReference.ProseMirror-selectednode) { +:global(.react-renderer.node-transclusionReference.ProseMirror-selectednode), +:global(.react-renderer.node-pageEmbed.ProseMirror-selectednode) { outline: none; } From b8655ae52c70cf391988fc9a974fc89139421b6e Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:26:42 +0300 Subject: [PATCH 03/11] fix(page-templates): make page-embed Refresh actually re-render (#40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read-only embed renderer mounts a Tiptap EditorProvider with the looked-up content, but Tiptap consumes the `content` option only at initial mount. After Refresh busted the lookup cache and re-fetched fresh content, the new content prop never reached the sub-editor, so the embed appeared not to update at all. Key PageEmbedContent on result.sourceUpdatedAt (the source page's updatedAt, already returned by the lookup and bumped on every persisted content change) so the component and its EditorProvider remount and apply the refreshed content when the source changes. Note: server-side freshness vs. live collab edits is bounded by the 10s persist debounce (collaboration.gateway.ts) — that separate limitation stays documented in #40 and is out of scope here; this commit fixes the client never re-rendering. Co-Authored-By: Claude Opus 4.8 --- .../editor/components/page-embed/page-embed-view.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx index 707a051f..b51607db 100644 --- a/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx +++ b/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx @@ -214,7 +214,17 @@ function PageEmbedBody({ sourcePageId={sourcePageId} hostPageId={hostPageId} > - + {/* + Tiptap's EditorProvider consumes `content` only at initial mount, so a + changed `content` prop (e.g. after Refresh re-fetches fresh content) + would not update the read-only sub-editor. Key on the source's + updatedAt to remount PageEmbedContent (and its inner EditorProvider) + whenever the source page changes, applying the refreshed content. + */} + ); } else if (result.status === "no_access") { From 4536d27ad2834a94e945e8935e948e61452b909a Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:42:30 +0300 Subject: [PATCH 04/11] fix(page-templates): never strand a page-embed id in-flight (#35) In the page-embed lookup flush(), the success branch cleared inFlightRef and resolved waiters only for ids present in the response items. A short/partial server response would leave a requested id stuck in inFlightRef forever (the subscribe/refresh path is guarded by !inFlightRef.has(id)) and its refresh() promise would never resolve. After processing returned items, also clear + resolve any requested id that wasn't returned, mirroring the catch branch. Cannot trigger under today's exact-mapping server contract; this is hardening. Co-Authored-By: Claude Opus 4.8 --- .../page-embed/page-embed-lookup-context.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-lookup-context.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-lookup-context.tsx index aa2a8caf..d29c19dc 100644 --- a/apps/client/src/features/editor/components/page-embed/page-embed-lookup-context.tsx +++ b/apps/client/src/features/editor/components/page-embed/page-embed-lookup-context.tsx @@ -55,7 +55,9 @@ export function PageEmbedLookupProvider({ try { const { items } = await lookupTemplate({ sourcePageIds: ids }); + const returned = new Set(); for (const r of items) { + returned.add(r.sourcePageId); resultCacheRef.current.set(r.sourcePageId, r); inFlightRef.current.delete(r.sourcePageId); const subs = subscribersRef.current.get(r.sourcePageId); @@ -64,6 +66,17 @@ export function PageEmbedLookupProvider({ } resolveWaiters(r.sourcePageId); } + // Harden against a partial/short server response: any requested id not + // present in `items` would otherwise stay in `inFlightRef` forever + // (subscribe/refresh are guarded by `!inFlightRef.has(id)`) and its + // refresh() promise would never resolve. Clear + resolve those ids, + // mirroring the catch branch, so no id can be stranded in-flight. + for (const id of ids) { + if (!returned.has(id)) { + inFlightRef.current.delete(id); + resolveWaiters(id); + } + } } catch (err) { // Surface the failure: errors must never be swallowed silently. console.error("[pageEmbed] template lookup failed", err); From 22887c474a937f44163b8b986ddf3f67009057bd Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:42:30 +0300 Subject: [PATCH 05/11] chore(page-templates): tidy ts suppression in duplicatePage pageEmbed remap (#37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace bare //@ts-ignore (no space, no reason) with // @ts-expect-error plus a reason on the pageEmbed sourcePageId reassignment, matching the codebase style. ProseMirror Attrs is read-only typed, so the reassignment genuinely errors — @ts-expect-error is valid here. Co-Authored-By: Claude Opus 4.8 --- apps/server/src/core/page/services/page.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 5373801d..8e5c28b6 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -716,7 +716,7 @@ export class PageService { const sourcePageId = node.attrs.sourcePageId; if (sourcePageId && pageMap.has(sourcePageId)) { const mappedPage = pageMap.get(sourcePageId); - //@ts-ignore + // @ts-expect-error ProseMirror Attrs is read-only typed; reassigning sourcePageId to the duplicated page copy is intentional here node.attrs.sourcePageId = mappedPage.newPageId; } } From a15cccf5579a2c70cfe09c92df730aafa1615326 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:42:49 +0300 Subject: [PATCH 06/11] chore(page-templates): remove dead findReferencePageIdsBySource (#34) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'used in N pages' reverse-navigation method had zero callers in the merged PR #17 — unreachable, untested code. Remove it. The reverse-navigation feature can be (re)added with the method if/when it's actually built. Co-Authored-By: Claude Opus 4.8 --- .../page-template-references.repo.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts b/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts index a678422f..8493e901 100644 --- a/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts +++ b/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts @@ -22,21 +22,6 @@ export class PageTemplateReferencesRepo { .execute(); } - async findReferencePageIdsBySource( - sourcePageId: string, - workspaceId: string, - trx?: KyselyTransaction, - ): Promise { - const rows = await dbOrTx(this.db, trx) - .selectFrom('pageTemplateReferences') - .select('referencePageId') - .distinct() - .where('workspaceId', '=', workspaceId) - .where('sourcePageId', '=', sourcePageId) - .execute(); - return rows.map((r) => r.referencePageId); - } - async insertMany( rows: InsertablePageTemplateReference[], trx?: KyselyTransaction, From 79d096ed7a091b3a7855ee9d39b9957e64fcdba5 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 21:42:49 +0300 Subject: [PATCH 07/11] fix(page-templates): defense-in-depth workspace checks (#36) Consistency hardening from #17 review (not currently exploitable): - toggleTemplate now explicitly rejects a page outside the caller's workspace (page.workspaceId !== user.workspaceId -> NotFound, avoiding existence leak) instead of relying solely on the space-membership model. - PageTemplateReferencesRepo.deleteByReferenceAndSources is now workspace-scoped (adds a workspaceId filter + param), matching the 'scope by workspaceId everywhere' invariant; the sole caller threads its workspaceId. The PAGE_TEMPLATE_THROTTLER limit is intentionally left as-is (the issue's throttle item was 'consider only'; no change without usage data). Co-Authored-By: Claude Opus 4.8 --- .../src/core/page/transclusion/page-template.controller.ts | 6 ++++++ .../src/core/page/transclusion/transclusion.service.ts | 1 + .../page-template-references.repo.ts | 3 +++ 3 files changed, 10 insertions(+) diff --git a/apps/server/src/core/page/transclusion/page-template.controller.ts b/apps/server/src/core/page/transclusion/page-template.controller.ts index 555a487f..db20ea42 100644 --- a/apps/server/src/core/page/transclusion/page-template.controller.ts +++ b/apps/server/src/core/page/transclusion/page-template.controller.ts @@ -67,6 +67,12 @@ export class PageTemplateController { throw new NotFoundException('Page not found'); } + if (page.workspaceId !== user.workspaceId) { + // Defense-in-depth: never act on a page outside the caller's workspace. + // Use NotFound (not Forbidden) to avoid leaking cross-workspace existence. + throw new NotFoundException('Page not found'); + } + await this.pageAccessService.validateCanEdit(page, user); const isTemplate = diff --git a/apps/server/src/core/page/transclusion/transclusion.service.ts b/apps/server/src/core/page/transclusion/transclusion.service.ts index f8f3b464..76bb8cfb 100644 --- a/apps/server/src/core/page/transclusion/transclusion.service.ts +++ b/apps/server/src/core/page/transclusion/transclusion.service.ts @@ -317,6 +317,7 @@ export class TransclusionService { if (toDelete.length > 0) { await this.pageTemplateReferencesRepo.deleteByReferenceAndSources( referencePageId, + workspaceId, toDelete, trx, ); diff --git a/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts b/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts index 8493e901..ac358bc6 100644 --- a/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts +++ b/apps/server/src/database/repos/page-template-references/page-template-references.repo.ts @@ -38,12 +38,15 @@ export class PageTemplateReferencesRepo { async deleteByReferenceAndSources( referencePageId: string, + workspaceId: string, sourcePageIds: string[], trx?: KyselyTransaction, ): Promise { if (sourcePageIds.length === 0) return; await dbOrTx(this.db, trx) .deleteFrom('pageTemplateReferences') + // Defense-in-depth: scope deletes to the caller's workspace. + .where('workspaceId', '=', workspaceId) .where('referencePageId', '=', referencePageId) .where('sourcePageId', 'in', sourcePageIds) .execute(); From 4f46f91db419536f3c45a93c4e39518c91c970fb Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 22:22:56 +0300 Subject: [PATCH 08/11] test(page-templates): fix TransclusionService spec constructor arity The transclusion specs predated two added constructor params, so they failed to compile (TS2554: expected 11 args, got 10) and the suites couldn't run. Add the missing mock args: workspaceRepo (param 11) in the lookup/access specs, and pageTemplateReferencesRepo (param 4, which had shifted pageRepo into the wrong slot) in the unsync-html-embed spec. All three suites now compile and pass. Co-Authored-By: Claude Opus 4.8 --- .../core/page/transclusion/spec/page-template-access.spec.ts | 4 +++- .../core/page/transclusion/spec/page-template-lookup.spec.ts | 1 + .../transclusion/spec/transclusion-unsync-html-embed.spec.ts | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts index 3c497d80..1f16605b 100644 --- a/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts +++ b/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts @@ -68,6 +68,7 @@ describe('TransclusionService — template access core (real filter)', () => { {} as any, // attachmentRepo {} as any, // storageService {} as any, // pageAccessService + {} as any, // workspaceRepo ); return { service, db, pageRepo, spaceMemberRepo, pagePermissionRepo }; @@ -216,7 +217,8 @@ describe('TransclusionService.syncPageTemplateReferences — workspace scoping', {} as any, {} as any, {} as any, - {} as any, + {} as any, // pageAccessService + {} as any, // workspaceRepo ); return { service, insertMany, pageTemplateReferencesRepo }; diff --git a/apps/server/src/core/page/transclusion/spec/page-template-lookup.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template-lookup.spec.ts index f62a047c..0ecd306e 100644 --- a/apps/server/src/core/page/transclusion/spec/page-template-lookup.spec.ts +++ b/apps/server/src/core/page/transclusion/spec/page-template-lookup.spec.ts @@ -34,6 +34,7 @@ describe('TransclusionService.lookupTemplate (access mapping)', () => { {} as any, // attachmentRepo {} as any, // storageService {} as any, // pageAccessService + {} as any, // workspaceRepo ); jest diff --git a/apps/server/src/core/page/transclusion/spec/transclusion-unsync-html-embed.spec.ts b/apps/server/src/core/page/transclusion/spec/transclusion-unsync-html-embed.spec.ts index 8ad13121..4d149369 100644 --- a/apps/server/src/core/page/transclusion/spec/transclusion-unsync-html-embed.spec.ts +++ b/apps/server/src/core/page/transclusion/spec/transclusion-unsync-html-embed.spec.ts @@ -56,6 +56,7 @@ function buildService(featureEnabled = true) { {} as any, // db (unused on this path) pageTransclusionsRepo as any, pageTransclusionReferencesRepo as any, + {} as any, // pageTemplateReferencesRepo (unused on this path) pageRepo as any, {} as any, // pagePermissionRepo (unused) {} as any, // spaceMemberRepo (unused) From 98769155d3bf5efdeafdcf54286ecb92953513e9 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 22:37:35 +0300 Subject: [PATCH 09/11] test(page-templates): cover client pageEmbed cycle/self-embed/depth guard (#31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cycle/self-embed/depth guard (PAGE_EMBED_MAX_DEPTH=5) lives only on the client and is the sole protection against runaway nested rendering — and was untested. Extract the inline predicates into pure, behavior-identical exported helpers (isPageEmbedCycle, isPageEmbedTooDeep in the ancestry context; filterPageEmbedOptions in the picker) so they're unit-testable without mounting the heavy Tiptap NodeView, and add vitest coverage (20 tests): ancestry chain/ host accumulation, cycle (ancestor-in-chain + top-level self-embed), too-deep at the cap, and picker host-exclusion. Co-Authored-By: Claude Opus 4.8 --- .../page-embed-ancestry-context.test.tsx | 149 ++++++++++++++++++ .../page-embed-ancestry-context.tsx | 23 +++ .../page-embed/page-embed-picker.test.ts | 44 ++++++ .../page-embed/page-embed-picker.tsx | 16 +- .../components/page-embed/page-embed-view.tsx | 14 +- 5 files changed, 237 insertions(+), 9 deletions(-) create mode 100644 apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.test.tsx create mode 100644 apps/client/src/features/editor/components/page-embed/page-embed-picker.test.ts diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.test.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.test.tsx new file mode 100644 index 00000000..867922d2 --- /dev/null +++ b/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.test.tsx @@ -0,0 +1,149 @@ +import { describe, it, expect } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { + PageEmbedAncestryProvider, + usePageEmbedAncestry, + isPageEmbedCycle, + isPageEmbedTooDeep, + PAGE_EMBED_MAX_DEPTH, +} from "./page-embed-ancestry-context"; + +/** + * Tiny probe that renders the current ancestry context as serialized data + * attributes so tests can assert the accumulated chain / threaded hostPageId + * without mounting the heavy Tiptap node view. + */ +function AncestryProbe({ testId = "probe" }: { testId?: string }) { + const { chain, hostPageId } = usePageEmbedAncestry(); + return ( + + ); +} + +describe("PageEmbedAncestryProvider", () => { + it("defaults to an empty chain and null host with no provider", () => { + render(); + const probe = screen.getByTestId("probe"); + expect(probe.getAttribute("data-chain")).toBe(""); + expect(probe.getAttribute("data-chain-length")).toBe("0"); + expect(probe.getAttribute("data-host")).toBe(""); + }); + + it("accumulates sourcePageId into the chain across nested providers", () => { + render( + + + + + + + , + ); + const probe = screen.getByTestId("probe"); + // Chain is built outermost -> innermost. + expect(probe.getAttribute("data-chain")).toBe("a,b,c"); + expect(probe.getAttribute("data-chain-length")).toBe("3"); + }); + + it("threads the host page id from the outermost provider down the tree", () => { + render( + + + + + , + ); + const probe = screen.getByTestId("probe"); + // The first host wins (parent.hostPageId ?? hostPageId); deeper hosts are + // ignored so the original host is preserved for self-embed detection. + expect(probe.getAttribute("data-host")).toBe("host-page"); + }); + + it("does not add an entry to the chain when sourcePageId is missing", () => { + render( + + + + + + + , + ); + const probe = screen.getByTestId("probe"); + // null / undefined sources are pass-through: chain stays ["a"], host kept. + expect(probe.getAttribute("data-chain")).toBe("a"); + expect(probe.getAttribute("data-host")).toBe("host"); + }); + + it("adopts a host provided only at a deeper level when the root had none", () => { + render( + + + + + , + ); + const probe = screen.getByTestId("probe"); + expect(probe.getAttribute("data-host")).toBe("late-host"); + }); +}); + +describe("isPageEmbedCycle", () => { + it("is false when the source is not in the chain and is not the host", () => { + expect(isPageEmbedCycle(["a", "b"], "host", "c")).toBe(false); + }); + + it("is true when the source is already present in the ancestor chain", () => { + expect(isPageEmbedCycle(["a", "b", "c"], "host", "b")).toBe(true); + }); + + it("is true for a top-level self-embed (host === source, empty chain)", () => { + expect(isPageEmbedCycle([], "self", "self")).toBe(true); + }); + + it("is true when the source equals the host even mid-chain", () => { + expect(isPageEmbedCycle(["x"], "self", "self")).toBe(true); + }); + + it("is false when there is no source id (nothing to embed yet)", () => { + expect(isPageEmbedCycle(["a"], "host", null)).toBe(false); + expect(isPageEmbedCycle([], "host", "")).toBe(false); + }); + + it("is false when host is null and source is not in the chain", () => { + expect(isPageEmbedCycle(["a", "b"], null, "c")).toBe(false); + }); +}); + +describe("isPageEmbedTooDeep", () => { + it("is false below the max depth", () => { + expect(isPageEmbedTooDeep([])).toBe(false); + expect( + isPageEmbedTooDeep(new Array(PAGE_EMBED_MAX_DEPTH - 1).fill("x")), + ).toBe(false); + }); + + it("is true once the chain length reaches the max depth", () => { + expect( + isPageEmbedTooDeep(new Array(PAGE_EMBED_MAX_DEPTH).fill("x")), + ).toBe(true); + }); + + it("is true when the chain length exceeds the max depth", () => { + expect( + isPageEmbedTooDeep(new Array(PAGE_EMBED_MAX_DEPTH + 3).fill("x")), + ).toBe(true); + }); + + it("guards at exactly PAGE_EMBED_MAX_DEPTH (=5)", () => { + // Pin the documented constant so an accidental change is caught. + expect(PAGE_EMBED_MAX_DEPTH).toBe(5); + expect(isPageEmbedTooDeep(["1", "2", "3", "4"])).toBe(false); + expect(isPageEmbedTooDeep(["1", "2", "3", "4", "5"])).toBe(true); + }); +}); diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.tsx index c989ee21..cdd7f109 100644 --- a/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.tsx +++ b/apps/client/src/features/editor/components/page-embed/page-embed-ancestry-context.tsx @@ -51,3 +51,26 @@ export function PageEmbedAncestryProvider({ export function usePageEmbedAncestry() { return useContext(PageEmbedAncestryContext); } + +/** + * Pure cycle predicate used by the page-embed node view. Returns true when the + * source page would recurse into itself: either it is already present in the + * ancestor chain, or it is the host page (top-level self-embed). Extracted so + * the anti-DoS guard can be unit-tested without mounting the Tiptap NodeView. + */ +export function isPageEmbedCycle( + chain: string[], + hostPageId: string | null, + sourcePageId: string | null, +): boolean { + if (!sourcePageId) return false; + return chain.includes(sourcePageId) || hostPageId === sourcePageId; +} + +/** + * Pure depth-limit predicate. Returns true once the ancestor chain has reached + * the hard cap, before a deeper nested editor is mounted. + */ +export function isPageEmbedTooDeep(chain: string[]): boolean { + return chain.length >= PAGE_EMBED_MAX_DEPTH; +} diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-picker.test.ts b/apps/client/src/features/editor/components/page-embed/page-embed-picker.test.ts new file mode 100644 index 00000000..257f09fd --- /dev/null +++ b/apps/client/src/features/editor/components/page-embed/page-embed-picker.test.ts @@ -0,0 +1,44 @@ +import { describe, it, expect } from "vitest"; +import { filterPageEmbedOptions } from "./page-embed-picker"; + +type Page = { id: string; title?: string }; + +describe("filterPageEmbedOptions", () => { + const pages: Page[] = [ + { id: "p1", title: "One" }, + { id: "host", title: "Host" }, + { id: "p2", title: "Two" }, + ]; + + it("excludes the host page from the options (self-embed guard)", () => { + const result = filterPageEmbedOptions(pages, "host"); + expect(result.map((p) => p.id)).toEqual(["p1", "p2"]); + }); + + it("keeps all pages when the host id matches nothing", () => { + const result = filterPageEmbedOptions(pages, "other"); + expect(result.map((p) => p.id)).toEqual(["p1", "host", "p2"]); + }); + + it("keeps all pages when no host id is provided", () => { + const result = filterPageEmbedOptions(pages, undefined); + expect(result.map((p) => p.id)).toEqual(["p1", "host", "p2"]); + }); + + it("drops nullish entries defensively", () => { + const dirty = [ + { id: "p1" }, + null as unknown as Page, + undefined as unknown as Page, + { id: "p2" }, + ]; + const result = filterPageEmbedOptions(dirty, "host"); + expect(result.map((p) => p.id)).toEqual(["p1", "p2"]); + }); + + it("returns an empty array for nullish input", () => { + expect( + filterPageEmbedOptions(null as unknown as Page[], "host"), + ).toEqual([]); + }); +}); diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-picker.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-picker.tsx index 7648b05e..5e914a57 100644 --- a/apps/client/src/features/editor/components/page-embed/page-embed-picker.tsx +++ b/apps/client/src/features/editor/components/page-embed/page-embed-picker.tsx @@ -9,6 +9,18 @@ import type { IPage } from "@/features/page/types/page.types"; export const PAGE_EMBED_PICKER_EVENT = "open-page-embed-picker"; +/** + * Pure filter excluding the host page (and any nullish entries) from the picker + * results. Extracted so the self-embed guard at insertion time is unit-testable + * without mounting the modal/search query. + */ +export function filterPageEmbedOptions( + pages: T[], + hostPageId?: string, +): T[] { + return (pages ?? []).filter((p) => p && p.id !== hostPageId); +} + type PickerDetail = { editor: Editor; range: Range; @@ -55,9 +67,7 @@ export default function PageEmbedPicker() { }); const hostPageId = detailRef.current?.hostPageId; - const pages = ((data?.pages ?? []) as IPage[]).filter( - (p) => p && p.id !== hostPageId, - ); + const pages = filterPageEmbedOptions((data?.pages ?? []) as IPage[], hostPageId); const handleSelect = (page: IPage) => { const detail = detailRef.current; diff --git a/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx b/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx index b51607db..a06a3063 100644 --- a/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx +++ b/apps/client/src/features/editor/components/page-embed/page-embed-view.tsx @@ -20,7 +20,8 @@ import { usePageEmbedLookup } from "./page-embed-lookup-context"; import { PageEmbedAncestryProvider, usePageEmbedAncestry, - PAGE_EMBED_MAX_DEPTH, + isPageEmbedCycle, + isPageEmbedTooDeep, } from "./page-embed-ancestry-context"; import PageEmbedContent from "./page-embed-content"; @@ -100,11 +101,12 @@ function PageEmbedBody({ // --- Cycle / depth guard (evaluated before any lookup is rendered) --------- // Self-embed or a source already present in the ancestor chain → cycle. - const isCycle = - !!sourcePageId && - (ancestry.chain.includes(sourcePageId) || - ancestry.hostPageId === sourcePageId); - const isTooDeep = ancestry.chain.length >= PAGE_EMBED_MAX_DEPTH; + const isCycle = isPageEmbedCycle( + ancestry.chain, + ancestry.hostPageId, + sourcePageId, + ); + const isTooDeep = isPageEmbedTooDeep(ancestry.chain); const sourceTitle = result && !("status" in result) ? result.title : null; From bc1ea792f554f745a19d2011678348bfdff83da5 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 22:37:35 +0300 Subject: [PATCH 10/11] test(page-templates): cover duplicatePage pageEmbed remap + reference sync (#32) Extract the per-node pageEmbed remap decision into a shared pure helper (remapPageEmbedSourceId) and use it BOTH in PageService.duplicatePage and the JSON walker, so the test guards the real production path (not a mirror that could drift). Behavior is identical: source in the copied set -> new copy id; otherwise keep the original. Add jest coverage (16 tests): the remap helper (in-set/out-of-set/null/nested), syncPageTemplateReferences toDelete (stale refs removed with the right workspaceId), and insertTemplateReferencesForPages multi-workspace grouping/filtering. Co-Authored-By: Claude Opus 4.8 --- .../src/core/page/services/page.service.ts | 12 +- .../spec/page-embed-remap.util.spec.ts | 200 +++++++++++ .../page-template-references-sync.spec.ts | 310 ++++++++++++++++++ .../utils/transclusion-prosemirror.util.ts | 58 ++++ 4 files changed, 574 insertions(+), 6 deletions(-) create mode 100644 apps/server/src/core/page/transclusion/spec/page-embed-remap.util.spec.ts create mode 100644 apps/server/src/core/page/transclusion/spec/page-template-references-sync.spec.ts diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 8e5c28b6..dc17e188 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -62,6 +62,7 @@ import { markdownToHtml } from '@docmost/editor-ext'; import { WatcherService } from '../../watcher/watcher.service'; import { sql } from 'kysely'; import { TransclusionService } from '../transclusion/transclusion.service'; +import { remapPageEmbedSourceId } from '../transclusion/utils/transclusion-prosemirror.util'; import { AuthProvenanceData } from '../../../common/decorators/auth-provenance.decorator'; @Injectable() @@ -713,12 +714,11 @@ export class PageService { // source page is also part of the copied set, point at its new copy; // otherwise leave it pointing at the original (live embed of original). if (node.type.name === 'pageEmbed') { - const sourcePageId = node.attrs.sourcePageId; - if (sourcePageId && pageMap.has(sourcePageId)) { - const mappedPage = pageMap.get(sourcePageId); - // @ts-expect-error ProseMirror Attrs is read-only typed; reassigning sourcePageId to the duplicated page copy is intentional here - node.attrs.sourcePageId = mappedPage.newPageId; - } + // @ts-expect-error ProseMirror Attrs is read-only typed; intentional remap to the duplicated copy + node.attrs.sourcePageId = remapPageEmbedSourceId( + node.attrs.sourcePageId, + (id) => pageMap.get(id)?.newPageId, + ); } // Update internal page links in link marks diff --git a/apps/server/src/core/page/transclusion/spec/page-embed-remap.util.spec.ts b/apps/server/src/core/page/transclusion/spec/page-embed-remap.util.spec.ts new file mode 100644 index 00000000..47fa46c4 --- /dev/null +++ b/apps/server/src/core/page/transclusion/spec/page-embed-remap.util.spec.ts @@ -0,0 +1,200 @@ +import { + remapPageEmbedSourceId, + remapPageEmbedSourceIds, +} from '../utils/transclusion-prosemirror.util'; + +/** + * Unit tests for the `pageEmbed` remap used by `PageService.duplicatePage`: + * + * - source page within the copied set -> rewrite to the COPY's new id + * - source page NOT in the copied set -> keep the ORIGINAL id (live embed) + * + * `remapPageEmbedSourceId` is the per-node decision the production + * `duplicatePage` callback now calls directly, so these tests guard the real + * path rather than a parallel copy. `remapPageEmbedSourceIds` is the JSON + * walker that delegates to the same helper; its tests exercise the shared + * decision transitively across nested ProseMirror containers. + */ +describe('remapPageEmbedSourceId (shared per-node decision used by duplicatePage)', () => { + it('returns the new copy id when the source IS in the copied set', () => { + const idMap = new Map([['old-src', 'new-copy']]); + + const out = remapPageEmbedSourceId('old-src', (id) => idMap.get(id)); + + expect(out).toBe('new-copy'); + }); + + it('returns the original id when the source is NOT in the copied set', () => { + const idMap = new Map([['old-src', 'new-copy']]); + + const out = remapPageEmbedSourceId('external', (id) => idMap.get(id)); + + expect(out).toBe('external'); + }); + + it('returns the original id when resolveNewId yields undefined', () => { + const out = remapPageEmbedSourceId('some-id', () => undefined); + + expect(out).toBe('some-id'); + }); + + it('leaves a null source unchanged without consulting the resolver', () => { + const resolve = jest.fn(() => 'should-not-be-used'); + + const out = remapPageEmbedSourceId(null, resolve); + + expect(out).toBeNull(); + expect(resolve).not.toHaveBeenCalled(); + }); + + it('leaves an undefined source unchanged without consulting the resolver', () => { + const resolve = jest.fn(() => 'should-not-be-used'); + + const out = remapPageEmbedSourceId(undefined, resolve); + + expect(out).toBeUndefined(); + expect(resolve).not.toHaveBeenCalled(); + }); +}); + +describe('remapPageEmbedSourceIds (duplicatePage pageEmbed remap)', () => { + const docWithEmbeds = (ids: string[]) => ({ + type: 'doc', + content: ids.map((id) => ({ + type: 'pageEmbed', + attrs: { sourcePageId: id }, + })), + }); + + it('remaps a source that IS within the copied set to its new copy id', () => { + const doc = docWithEmbeds(['old-src']); + const idMap = new Map([['old-src', 'new-copy']]); + + const out = remapPageEmbedSourceIds(doc, idMap); + + expect(out.content[0].attrs.sourcePageId).toBe('new-copy'); + }); + + it('keeps the original id for a source NOT in the copied set', () => { + const doc = docWithEmbeds(['external']); + const idMap = new Map([['old-src', 'new-copy']]); // does not contain "external" + + const out = remapPageEmbedSourceIds(doc, idMap); + + expect(out.content[0].attrs.sourcePageId).toBe('external'); + }); + + it('handles a mixed doc: in-set remapped, out-of-set preserved', () => { + const doc = docWithEmbeds(['in-set', 'external']); + const idMap = new Map([['in-set', 'in-set-copy']]); + + const out = remapPageEmbedSourceIds(doc, idMap); + + expect(out.content.map((n: any) => n.attrs.sourcePageId)).toEqual([ + 'in-set-copy', + 'external', + ]); + }); + + it('remaps pageEmbeds nested inside columns', () => { + const doc = { + type: 'doc', + content: [ + { + type: 'columnList', + content: [ + { + type: 'column', + content: [ + { type: 'pageEmbed', attrs: { sourcePageId: 'nested-in' } }, + ], + }, + { + type: 'column', + content: [ + { type: 'pageEmbed', attrs: { sourcePageId: 'nested-out' } }, + ], + }, + ], + }, + ], + }; + const idMap = new Map([['nested-in', 'nested-in-copy']]); + + const out = remapPageEmbedSourceIds(doc, idMap) as any; + + const col0 = out.content[0].content[0].content[0]; + const col1 = out.content[0].content[1].content[0]; + expect(col0.attrs.sourcePageId).toBe('nested-in-copy'); + expect(col1.attrs.sourcePageId).toBe('nested-out'); + }); + + it('remaps pageEmbeds nested inside a callout', () => { + const doc = { + type: 'doc', + content: [ + { + type: 'callout', + content: [ + { type: 'pageEmbed', attrs: { sourcePageId: 'in-callout' } }, + ], + }, + ], + }; + const idMap = new Map([['in-callout', 'in-callout-copy']]); + + const out = remapPageEmbedSourceIds(doc, idMap) as any; + + expect(out.content[0].content[0].attrs.sourcePageId).toBe( + 'in-callout-copy', + ); + }); + + it('does not descend into a transclusionSource (schema-isolated)', () => { + const doc = { + type: 'doc', + content: [ + { + type: 'transclusionSource', + attrs: { id: 'src' }, + content: [ + { type: 'pageEmbed', attrs: { sourcePageId: 'hidden' } }, + ], + }, + ], + }; + const idMap = new Map([['hidden', 'should-not-apply']]); + + const out = remapPageEmbedSourceIds(doc, idMap) as any; + + // The embed inside a source must be left untouched. + expect(out.content[0].content[0].attrs.sourcePageId).toBe('hidden'); + }); + + it('leaves embeds missing a sourcePageId untouched', () => { + const doc = { + type: 'doc', + content: [ + { type: 'pageEmbed', attrs: {} }, + { type: 'pageEmbed', attrs: { sourcePageId: '' } }, + ], + }; + const idMap = new Map([['', 'x']]); + + const out = remapPageEmbedSourceIds(doc, idMap) as any; + + expect(out.content[0].attrs.sourcePageId).toBeUndefined(); + expect(out.content[1].attrs.sourcePageId).toBe(''); + }); + + it('returns the doc unchanged when idMap is empty', () => { + const doc = docWithEmbeds(['a', 'b']); + + const out = remapPageEmbedSourceIds(doc, new Map()); + + expect(out.content.map((n: any) => n.attrs.sourcePageId)).toEqual([ + 'a', + 'b', + ]); + }); +}); diff --git a/apps/server/src/core/page/transclusion/spec/page-template-references-sync.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template-references-sync.spec.ts new file mode 100644 index 00000000..4afad554 --- /dev/null +++ b/apps/server/src/core/page/transclusion/spec/page-template-references-sync.spec.ts @@ -0,0 +1,310 @@ +import { TransclusionService } from '../transclusion.service'; + +/** + * Covers two untested, high-risk write paths around `page_template_references`: + * + * 1. `syncPageTemplateReferences` — the `toDelete` branch: stale references are + * removed when the host page no longer embeds a source, while genuinely new + * embeds are inserted. We assert `deleteByReferenceAndSources` / `insertMany` + * receive the correct rows and the returned `{ inserted, deleted }` counts. + * + * 2. `insertTemplateReferencesForPages` — the multi-workspace grouping/filtering + * branch: candidate source ids are grouped per workspace, each workspace is + * validated independently, and cross-workspace sources are dropped. + * + * Setup/mocking mirrors the existing transclusion specs (page-template-access / + * page-template-lookup): `new TransclusionService(...)` is built with the same + * 11 positional mock args; only the deps each test touches are real stubs. + */ + +/** + * Chainable kysely `db` stub used by `filterInWorkspaceSourceIds`. Every + * `selectFrom(...).select(...).where(...)` returns the same builder; `.execute()` + * resolves whatever rows the per-call resolver returns. The resolver receives + * the captured `where('id','in', )` and `where('workspaceId','=', ws)` + * arguments so a test can decide, per workspace, which ids "exist". + */ +function makeWorkspaceScopedDb( + resolve: (ids: string[], workspaceId: string) => string[], +) { + const state = { ids: [] as string[], workspaceId: '' }; + const builder: any = {}; + builder.selectFrom = jest.fn(() => builder); + builder.select = jest.fn(() => builder); + builder.where = jest.fn((col: string, _op: string, val: any) => { + if (col === 'id') state.ids = val as string[]; + if (col === 'workspaceId') state.workspaceId = val as string; + return builder; + }); + builder.execute = jest.fn(async () => + resolve(state.ids, state.workspaceId).map((id) => ({ id })), + ); + return builder; +} + +function buildService(opts: { + db: any; + pageTemplateReferencesRepo: any; +}) { + return new TransclusionService( + opts.db, + {} as any, // pageTransclusionsRepo + {} as any, // pageTransclusionReferencesRepo + opts.pageTemplateReferencesRepo, + {} as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // spaceMemberRepo + {} as any, // attachmentRepo + {} as any, // storageService + {} as any, // pageAccessService + {} as any, // workspaceRepo + ); +} + +const pageEmbedDoc = (sourceIds: string[]) => ({ + type: 'doc', + content: sourceIds.map((id) => ({ + type: 'pageEmbed', + attrs: { sourcePageId: id }, + })), +}); + +describe('TransclusionService.syncPageTemplateReferences — toDelete branch', () => { + it('deletes stale references and inserts new ones with correct args/counts', async () => { + // Every candidate id is treated as in-workspace by the existence query. + const db = makeWorkspaceScopedDb((ids) => ids); + + const insertMany = jest.fn().mockResolvedValue(undefined); + const deleteByReferenceAndSources = jest.fn().mockResolvedValue(undefined); + const pageTemplateReferencesRepo = { + // existing refs: "keep" stays embedded, "stale-a"/"stale-b" no longer are + findByReferencePageId: jest.fn().mockResolvedValue([ + { sourcePageId: 'keep' }, + { sourcePageId: 'stale-a' }, + { sourcePageId: 'stale-b' }, + ]), + insertMany, + deleteByReferenceAndSources, + }; + + const service = buildService({ db, pageTemplateReferencesRepo }); + + // host now embeds: keep (unchanged) + fresh (new). stale-a/stale-b gone. + const result = await service.syncPageTemplateReferences( + 'host', + 'w1', + pageEmbedDoc(['keep', 'fresh']), + ); + + expect(result).toEqual({ inserted: 1, deleted: 2 }); + + // only the genuinely new embed is inserted (keep already existed) + expect(insertMany).toHaveBeenCalledTimes(1); + expect(insertMany.mock.calls[0][0]).toEqual([ + { workspaceId: 'w1', referencePageId: 'host', sourcePageId: 'fresh' }, + ]); + + // stale references removed, scoped to host + workspace + expect(deleteByReferenceAndSources).toHaveBeenCalledTimes(1); + const [refPageId, workspaceId, staleSources] = + deleteByReferenceAndSources.mock.calls[0]; + expect(refPageId).toBe('host'); + expect(workspaceId).toBe('w1'); + expect([...staleSources].sort()).toEqual(['stale-a', 'stale-b']); + }); + + it('deletes ALL existing references when the host embeds nothing anymore', async () => { + const db = makeWorkspaceScopedDb((ids) => ids); + const insertMany = jest.fn().mockResolvedValue(undefined); + const deleteByReferenceAndSources = jest.fn().mockResolvedValue(undefined); + const pageTemplateReferencesRepo = { + findByReferencePageId: jest + .fn() + .mockResolvedValue([{ sourcePageId: 'a' }, { sourcePageId: 'b' }]), + insertMany, + deleteByReferenceAndSources, + }; + + const service = buildService({ db, pageTemplateReferencesRepo }); + + const result = await service.syncPageTemplateReferences( + 'host', + 'w1', + pageEmbedDoc([]), // no embeds left + ); + + expect(result).toEqual({ inserted: 0, deleted: 2 }); + expect(insertMany).not.toHaveBeenCalled(); + const [, , staleSources] = deleteByReferenceAndSources.mock.calls[0]; + expect([...staleSources].sort()).toEqual(['a', 'b']); + }); + + it('treats a cross-workspace embed as stale: it never survives to be kept', async () => { + // existence query drops "cross-ws"; so an existing ref to it must be deleted + const db = makeWorkspaceScopedDb((ids) => ids.filter((id) => id !== 'cross-ws')); + const insertMany = jest.fn().mockResolvedValue(undefined); + const deleteByReferenceAndSources = jest.fn().mockResolvedValue(undefined); + const pageTemplateReferencesRepo = { + findByReferencePageId: jest + .fn() + .mockResolvedValue([{ sourcePageId: 'cross-ws' }]), + insertMany, + deleteByReferenceAndSources, + }; + + const service = buildService({ db, pageTemplateReferencesRepo }); + + // host still "embeds" cross-ws in its doc, but it is not in-workspace + const result = await service.syncPageTemplateReferences( + 'host', + 'w1', + pageEmbedDoc(['cross-ws']), + ); + + expect(result).toEqual({ inserted: 0, deleted: 1 }); + expect(insertMany).not.toHaveBeenCalled(); + const [, , staleSources] = deleteByReferenceAndSources.mock.calls[0]; + expect([...staleSources]).toEqual(['cross-ws']); + }); + + it('no-ops both repos when desired and existing already match', async () => { + const db = makeWorkspaceScopedDb((ids) => ids); + const insertMany = jest.fn().mockResolvedValue(undefined); + const deleteByReferenceAndSources = jest.fn().mockResolvedValue(undefined); + const pageTemplateReferencesRepo = { + findByReferencePageId: jest + .fn() + .mockResolvedValue([{ sourcePageId: 'same' }]), + insertMany, + deleteByReferenceAndSources, + }; + + const service = buildService({ db, pageTemplateReferencesRepo }); + + const result = await service.syncPageTemplateReferences( + 'host', + 'w1', + pageEmbedDoc(['same']), + ); + + expect(result).toEqual({ inserted: 0, deleted: 0 }); + expect(insertMany).not.toHaveBeenCalled(); + expect(deleteByReferenceAndSources).not.toHaveBeenCalled(); + }); +}); + +describe('TransclusionService.insertTemplateReferencesForPages — multi-workspace grouping', () => { + it('groups candidates per workspace and validates each workspace independently', async () => { + // Each workspace "owns" only its own source ids. The existence query is + // workspace-scoped, so an id from another workspace is dropped. + const owned: Record = { + w1: ['s1'], + w2: ['s2'], + }; + const executeArgs: Array<{ ids: string[]; workspaceId: string }> = []; + const db = makeWorkspaceScopedDb((ids, workspaceId) => { + executeArgs.push({ ids: [...ids], workspaceId }); + const ownedSet = new Set(owned[workspaceId] ?? []); + return ids.filter((id) => ownedSet.has(id)); + }); + + const insertMany = jest.fn().mockResolvedValue(undefined); + const pageTemplateReferencesRepo = { insertMany }; + + const service = buildService({ db, pageTemplateReferencesRepo }); + + // page-a in w1 embeds s1 (valid) + s2 (belongs to w2 -> dropped) + // page-b in w2 embeds s2 (valid) + const result = await service.insertTemplateReferencesForPages([ + { id: 'page-a', workspaceId: 'w1', content: pageEmbedDoc(['s1', 's2']) }, + { id: 'page-b', workspaceId: 'w2', content: pageEmbedDoc(['s2']) }, + ]); + + expect(result).toEqual({ inserted: 2 }); + + expect(insertMany).toHaveBeenCalledTimes(1); + const rows = insertMany.mock.calls[0][0]; + expect(rows).toEqual([ + { workspaceId: 'w1', referencePageId: 'page-a', sourcePageId: 's1' }, + { workspaceId: 'w2', referencePageId: 'page-b', sourcePageId: 's2' }, + ]); + + // one existence query per workspace, each scoped to that workspace's candidates + expect(executeArgs).toHaveLength(2); + const w1Call = executeArgs.find((c) => c.workspaceId === 'w1'); + const w2Call = executeArgs.find((c) => c.workspaceId === 'w2'); + expect(w1Call?.ids.sort()).toEqual(['s1', 's2']); + expect(w2Call?.ids).toEqual(['s2']); + }); + + it('drops every cross-workspace source and inserts nothing when none are in-workspace', async () => { + // No id is owned by its page's workspace -> all filtered out. + const db = makeWorkspaceScopedDb(() => []); + const insertMany = jest.fn().mockResolvedValue(undefined); + const service = buildService({ + db, + pageTemplateReferencesRepo: { insertMany }, + }); + + const result = await service.insertTemplateReferencesForPages([ + { id: 'page-a', workspaceId: 'w1', content: pageEmbedDoc(['x']) }, + { id: 'page-b', workspaceId: 'w2', content: pageEmbedDoc(['y']) }, + ]); + + expect(result).toEqual({ inserted: 0 }); + expect(insertMany).not.toHaveBeenCalled(); + }); + + it('dedupes a sourceId shared by two pages in the same workspace into one validation', async () => { + const executeArgs: Array<{ ids: string[]; workspaceId: string }> = []; + const db = makeWorkspaceScopedDb((ids, workspaceId) => { + executeArgs.push({ ids: [...ids], workspaceId }); + return ids; // all in-workspace + }); + const insertMany = jest.fn().mockResolvedValue(undefined); + const service = buildService({ + db, + pageTemplateReferencesRepo: { insertMany }, + }); + + // both pages embed the same source "shared" in w1 + const result = await service.insertTemplateReferencesForPages([ + { id: 'page-a', workspaceId: 'w1', content: pageEmbedDoc(['shared']) }, + { id: 'page-b', workspaceId: 'w1', content: pageEmbedDoc(['shared']) }, + ]); + + // a row per (page, source) pair, but only one existence query for w1 + expect(result).toEqual({ inserted: 2 }); + expect(executeArgs).toHaveLength(1); + expect(executeArgs[0]).toEqual({ ids: ['shared'], workspaceId: 'w1' }); + + const rows = insertMany.mock.calls[0][0]; + expect(rows).toEqual([ + { workspaceId: 'w1', referencePageId: 'page-a', sourcePageId: 'shared' }, + { workspaceId: 'w1', referencePageId: 'page-b', sourcePageId: 'shared' }, + ]); + }); + + it('returns inserted:0 without querying when no page has embeds', async () => { + const execute = jest.fn(); + const db = makeWorkspaceScopedDb(() => { + execute(); + return []; + }); + const insertMany = jest.fn().mockResolvedValue(undefined); + const service = buildService({ + db, + pageTemplateReferencesRepo: { insertMany }, + }); + + const result = await service.insertTemplateReferencesForPages([ + { id: 'page-a', workspaceId: 'w1', content: pageEmbedDoc([]) }, + ]); + + expect(result).toEqual({ inserted: 0 }); + expect(insertMany).not.toHaveBeenCalled(); + // filterInWorkspaceSourceIds short-circuits on empty candidates, so the + // existence query never runs. + expect(execute).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/core/page/transclusion/utils/transclusion-prosemirror.util.ts b/apps/server/src/core/page/transclusion/utils/transclusion-prosemirror.util.ts index eeeea0e2..d8cdf3bf 100644 --- a/apps/server/src/core/page/transclusion/utils/transclusion-prosemirror.util.ts +++ b/apps/server/src/core/page/transclusion/utils/transclusion-prosemirror.util.ts @@ -99,6 +99,64 @@ export function collectReferencesFromPmJson( return out; } +/** + * Decide the sourcePageId a duplicated pageEmbed should point to: the copy's new + * id when the embedded source is part of the copied set, otherwise the original + * (a live embed of the original page). Pure — shared by PageService.duplicatePage + * (the real path) and the JSON walker below, so both stay in lockstep. + */ +export function remapPageEmbedSourceId( + sourcePageId: string | null | undefined, + resolveNewId: (id: string) => string | undefined, +): string | null | undefined { + if (sourcePageId) { + const mapped = resolveNewId(sourcePageId); + if (mapped) return mapped; + } + return sourcePageId; +} + +/** + * Remap the `sourcePageId` of every `pageEmbed` node in a ProseMirror JSON doc + * according to `idMap` (old page id -> new page id). Delegates the per-node + * decision to the shared `remapPageEmbedSourceId` helper that + * `PageService.duplicatePage` also uses, so the production path and this walker + * stay in lockstep: when the embedded source page is part of the copied set + * (present in `idMap`) the embed is pointed at its new copy; otherwise the + * original `sourcePageId` is preserved so it stays a live embed of the original + * page. Mutates `doc` in place (and returns it) to match the service's in-place + * ProseMirror mutation. Recurses through arbitrary block containers (columns, + * callouts, etc.) the same way the collectors do, but does NOT descend into a + * `transclusionSource` (schema-isolated). + */ +export function remapPageEmbedSourceIds( + doc: T, + idMap: Map, +): T { + const visit = (node: any): void => { + if (!node || typeof node !== 'object') return; + + if (node.type === PAGE_EMBED_TYPE) { + if (node.attrs) { + node.attrs.sourcePageId = remapPageEmbedSourceId( + node.attrs.sourcePageId, + (id) => idMap.get(id), + ); + } + return; // atom node - no children + } + + if (node.type === TRANSCLUSION_TYPE) return; + + if (Array.isArray(node.content)) { + for (const child of node.content) visit(child); + } + }; + + visit(doc); + return doc; +} + /** * Walks a ProseMirror JSON document and returns one snapshot per unique * `sourcePageId` found on `pageEmbed` nodes (whole-page live embeds). Order From 39f3eacf897e1365c89e8ef18bf4854c3d2f3ae5 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 22:37:35 +0300 Subject: [PATCH 11/11] test(page-templates): cover lookupTemplate anti-leak + edge cases (#33) - The security-relevant catch->not_found branch in lookupTemplate (returns not_found instead of raw content when comment-mark stripping throws) is now tested by forcing the strip to throw with a malformed text node, asserting no content/marks leak. - not_found for a soft-deleted source resolved through the REAL filterViewerAccessiblePageIds (deletedAt-excluded), not the stubbed filter. - Rename the misleading 'honours <=50 cap' test to reflect it only exercises dedup (the cap lives in the DTO, never engaged in the service unit). - Cover the onlyTemplates search filter (restricts to is_template=true). Also fix two pre-existing failing 'should be defined' specs (search service + controller) that couldn't resolve the @InjectKysely token via createTestingModule. Co-Authored-By: Claude Opus 4.8 --- .../spec/page-template-access.spec.ts | 9 +- .../spec/page-template-lookup-edge.spec.ts | 183 ++++++++++++++++++ .../src/core/search/search.controller.spec.ts | 18 +- .../src/core/search/search.service.spec.ts | 105 ++++++++-- 4 files changed, 295 insertions(+), 20 deletions(-) create mode 100644 apps/server/src/core/page/transclusion/spec/page-template-lookup-edge.spec.ts diff --git a/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts index 1f16605b..78707dd8 100644 --- a/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts +++ b/apps/server/src/core/page/transclusion/spec/page-template-access.spec.ts @@ -173,8 +173,13 @@ describe('TransclusionService — template access core (real filter)', () => { expect((items[2] as any).status).toBe('no_access'); // not space-visible }); - it('honours the DTO-level ≤50 cap by deduping ids passed to the filter', async () => { - // The DTO enforces ArrayMaxSize(50); the service dedupes before filtering. + it('dedupes source ids before passing them to the access filter', async () => { + // NOTE: this test only covers DEDUP, not the ≤50 cap. The ArrayMaxSize(50) + // limit is enforced by the DTO (validation layer), so it is never engaged in + // the service under unit test — the service receives an already-validated + // array and merely dedupes it. Renamed from the old "honours ≤50 cap" title, + // which misleadingly implied the cap was exercised here. A real cap test would + // belong in a controller/DTO-validation spec, not in this service unit test. const ids = ['a', 'a', 'b']; const { service, db } = makeService({ spaceVisibleRows: [], diff --git a/apps/server/src/core/page/transclusion/spec/page-template-lookup-edge.spec.ts b/apps/server/src/core/page/transclusion/spec/page-template-lookup-edge.spec.ts new file mode 100644 index 00000000..59dec763 --- /dev/null +++ b/apps/server/src/core/page/transclusion/spec/page-template-lookup-edge.spec.ts @@ -0,0 +1,183 @@ +import { TransclusionService } from '../transclusion.service'; + +/** + * Edge-case + anti-leak coverage for `lookupTemplate` that the existing + * `page-template-lookup.spec.ts` (stubbed filter) and `page-template-access.spec.ts` + * (real filter, happy paths) do not exercise: + * + * 1. SECURITY anti-leak: when comment-mark stripping THROWS, the item must come + * back as `not_found` and NEVER carry raw content (the source's comment marks + * could otherwise leak to a viewer). See the `catch` branch in `lookupTemplate`. + * 2. A soft-deleted source page resolved through the REAL + * `filterViewerAccessiblePageIds` (space-visibility query filters `deletedAt`), + * asserting it maps to `not_found`/`no_access` rather than content. + */ +describe('TransclusionService.lookupTemplate — anti-leak catch branch', () => { + const now = new Date('2026-06-20T00:00:00.000Z'); + + function makeService(opts: { + accessibleIds: string[]; + pages: Array<{ + id: string; + slugId?: string; + title: string | null; + icon: string | null; + content: unknown; + updatedAt: Date; + }>; + }) { + const pageRepo = { + findManyByIds: jest.fn().mockResolvedValue(opts.pages), + }; + + const service = new TransclusionService( + {} as any, // db + {} as any, // pageTransclusionsRepo + {} as any, // pageTransclusionReferencesRepo + {} as any, // pageTemplateReferencesRepo + pageRepo as any, + {} as any, // pagePermissionRepo + {} as any, // spaceMemberRepo + {} as any, // attachmentRepo + {} as any, // storageService + {} as any, // pageAccessService + {} as any, // workspaceRepo + ); + + // Stub the access decision; we are testing the content-prep stage, not access. + jest + .spyOn(service, 'filterViewerAccessiblePageIds') + .mockResolvedValue(opts.accessibleIds); + + return { service, pageRepo }; + } + + it('returns not_found (NOT raw content) when comment-mark stripping throws', async () => { + // An accessible, present page whose stored content is structurally invalid PM: + // a `text` node without a `text` field. `jsonToNode` (called inside the try + // block) throws "Invalid text node in JSON" on this, which exercises the + // service's catch -> not_found anti-leak guard. This uses a REAL malformed + // input (no module mocking) so the test stays faithful to production behaviour. + const malformedContent = { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { + // Missing `text` — Node.fromJSON rejects this and jsonToNode rethrows. + type: 'text', + marks: [{ type: 'comment', attrs: { commentId: 'leak-me' } }], + }, + ], + }, + ], + }; + + const { service } = makeService({ + accessibleIds: ['p1'], + pages: [ + { + id: 'p1', + slugId: 's1', + title: 'Secret', + icon: '📄', + content: malformedContent, + updatedAt: now, + }, + ], + }); + + // Silence the expected error log so the suite output stays clean. + jest.spyOn((service as any).logger, 'error').mockImplementation(() => {}); + + const { items } = await service.lookupTemplate(['p1'], 'u1', 'w1'); + + expect(items).toHaveLength(1); + const item = items[0] as any; + + // Must degrade to not_found... + expect(item.status).toBe('not_found'); + expect(item.sourcePageId).toBe('p1'); + + // ...and must NOT leak ANY content/metadata of the source page. + expect(item).not.toHaveProperty('content'); + expect(item).not.toHaveProperty('title'); + expect(item).not.toHaveProperty('icon'); + expect(item).not.toHaveProperty('slugId'); + expect(item).not.toHaveProperty('sourceUpdatedAt'); + + // Hard guarantee: the would-be-leaked comment mark appears nowhere in output. + expect(JSON.stringify(item)).not.toContain('leak-me'); + expect(JSON.stringify(item)).not.toContain('comment'); + }); +}); + +describe('TransclusionService.lookupTemplate — soft-deleted source via real filter', () => { + const now = new Date('2026-06-20T00:00:00.000Z'); + + /** + * Chainable kysely `db` stub mirroring `page-template-access.spec.ts`. The + * space-visibility query in `filterViewerAccessiblePageIds` filters + * `where('deletedAt','is',null)`; a soft-deleted page is therefore absent from + * the rows we resolve here, so the REAL filter is what drops it. + */ + function makeDb(executeRows: Array<{ id: string }>) { + const builder: any = {}; + builder.selectFrom = jest.fn(() => builder); + builder.select = jest.fn(() => builder); + builder.where = jest.fn(() => builder); + builder.execute = jest.fn(async () => executeRows); + return builder; + } + + it('resolves a soft-deleted source to not_found/no_access through the REAL filter', async () => { + // The page IS soft-deleted, so the space-visibility query returns no rows for + // it (deletedAt filter). We let the real filter run end-to-end. + const db = makeDb([]); // soft-deleted -> excluded by the deletedAt='is null' clause + + const spaceMemberRepo = { + getUserSpaceIdsQuery: jest.fn(() => ({ __subquery: true })), + }; + const pagePermissionRepo = { + filterAccessiblePageIds: jest.fn().mockResolvedValue([]), + }; + const pageRepo = { + // Even if it were queried, the page is gone; assert via the filter instead. + findManyByIds: jest.fn().mockResolvedValue([]), + }; + + const service = new TransclusionService( + db as any, + {} as any, + {} as any, + {} as any, + pageRepo as any, + pagePermissionRepo as any, + spaceMemberRepo as any, + {} as any, + {} as any, + {} as any, + {} as any, + ); + + const { items } = await service.lookupTemplate(['deleted-src'], 'u1', 'w1'); + + // Soft-deleted source must never resolve to content. + expect(items).toEqual([ + { sourcePageId: 'deleted-src', status: 'no_access' }, + ]); + const item = items[0] as any; + expect(item).not.toHaveProperty('content'); + + // The real filter short-circuited before page-permission filtering because + // the deletedAt-filtered space-visibility query returned nothing. + expect(pagePermissionRepo.filterAccessiblePageIds).not.toHaveBeenCalled(); + // And the verb on the db builder included a deletedAt 'is null' guard, proving + // the real path (not a stub) excluded the soft-deleted page. + const deletedAtCall = db.where.mock.calls.find( + (c: any[]) => c[0] === 'deletedAt', + ); + expect(deletedAtCall).toEqual(['deletedAt', 'is', null]); + }); +}); diff --git a/apps/server/src/core/search/search.controller.spec.ts b/apps/server/src/core/search/search.controller.spec.ts index 6d6bad58..1b0e42cb 100644 --- a/apps/server/src/core/search/search.controller.spec.ts +++ b/apps/server/src/core/search/search.controller.spec.ts @@ -1,15 +1,19 @@ -import { Test, TestingModule } from '@nestjs/testing'; import { SearchController } from './search.controller'; +// Direct instantiation with stub deps. The Test.createTestingModule form failed +// to resolve SearchService's @InjectKysely() connection token at compile() (the +// same Nest-DI/Kysely-token issue addressed in search.service.spec), and this +// unit only needs the controller to construct. describe('SearchController', () => { let controller: SearchController; - beforeEach(async () => { - const module: TestingModule = await Test.createTestingModule({ - controllers: [SearchController], - }).compile(); - - controller = module.get(SearchController); + beforeEach(() => { + controller = new SearchController( + {} as any, // searchService + {} as any, // spaceAbility + {} as any, // environmentService + {} as any, // moduleRef + ); }); it('should be defined', () => { diff --git a/apps/server/src/core/search/search.service.spec.ts b/apps/server/src/core/search/search.service.spec.ts index 63fc48c0..efd4d2b8 100644 --- a/apps/server/src/core/search/search.service.spec.ts +++ b/apps/server/src/core/search/search.service.spec.ts @@ -1,18 +1,101 @@ -import { Test, TestingModule } from '@nestjs/testing'; import { SearchService } from './search.service'; describe('SearchService', () => { - let service: SearchService; - - beforeEach(async () => { - const module: TestingModule = await Test.createTestingModule({ - providers: [SearchService], - }).compile(); - - service = module.get(SearchService); - }); - it('should be defined', () => { + // Construct directly with stub deps. The previous Test.createTestingModule + // form could not resolve the @InjectKysely() connection token and failed at + // compile() — manual construction mirrors the rest of these unit specs. + const service = new SearchService( + {} as any, // db + {} as any, // pageRepo + {} as any, // shareRepo + {} as any, // spaceMemberRepo + {} as any, // pagePermissionRepo + ); expect(service).toBeDefined(); }); }); + +/** + * Focused coverage for the `onlyTemplates` flag in `searchSuggestions`, which + * restricts page suggestions to template pages (`is_template = true`). The kysely + * query builder and repos are mocked the same way the access specs mock chainable + * builders: every builder method returns the same builder, `.execute()` resolves + * the supplied rows. We assert whether `.where('isTemplate', '=', true)` is added. + */ +describe('SearchService.searchSuggestions — onlyTemplates filter', () => { + function makeService(pageRows: Array<{ id: string }>) { + // Chainable page-search builder. Record every `.where(...)` call so we can + // assert on the is_template restriction. + const pageBuilder: any = {}; + pageBuilder.select = jest.fn(() => pageBuilder); + pageBuilder.where = jest.fn(() => pageBuilder); + pageBuilder.orderBy = jest.fn(() => pageBuilder); + pageBuilder.limit = jest.fn(() => pageBuilder); + pageBuilder.execute = jest.fn(async () => pageRows); + + const db: any = { + // searchSuggestions only touches `pages` here (includePages: true). + selectFrom: jest.fn(() => pageBuilder), + }; + + const pageRepo = { + // `.select((eb) => this.pageRepo.withSpace(eb))` — return value is ignored + // by our builder stub, so a sentinel is enough. + withSpace: jest.fn(() => ({ __withSpace: true })), + }; + const shareRepo = {}; + const spaceMemberRepo = { + getUserSpaceIds: jest.fn().mockResolvedValue(['space-1']), + }; + const pagePermissionRepo = { + // Let every found page through page-level permission filtering. + filterAccessiblePageIds: jest + .fn() + .mockImplementation(async ({ pageIds }: { pageIds: string[] }) => pageIds), + }; + + const service = new SearchService( + db as any, + pageRepo as any, + shareRepo as any, + spaceMemberRepo as any, + pagePermissionRepo as any, + ); + + return { service, db, pageBuilder }; + } + + const isTemplateWhereCall = (pageBuilder: any) => + pageBuilder.where.mock.calls.find((c: any[]) => c[0] === 'isTemplate'); + + it('restricts page suggestions to is_template = true when onlyTemplates is set', async () => { + const { service, pageBuilder } = makeService([{ id: 'tmpl-1' }]); + + const result = await service.searchSuggestions( + { query: 'plan', includePages: true, onlyTemplates: true } as any, + 'user-1', + 'ws-1', + ); + + // The is_template restriction must be applied to the page query. + const call = isTemplateWhereCall(pageBuilder); + expect(call).toEqual(['isTemplate', '=', true]); + + // Sanity: the (template) page made it through. + expect(result.pages.map((p: any) => p.id)).toEqual(['tmpl-1']); + }); + + it('does NOT restrict to templates when onlyTemplates is absent', async () => { + const { service, pageBuilder } = makeService([{ id: 'any-1' }]); + + await service.searchSuggestions( + { query: 'plan', includePages: true } as any, + 'user-1', + 'ws-1', + ); + + // No is_template clause should be added for a normal page suggestion search. + expect(isTemplateWhereCall(pageBuilder)).toBeUndefined(); + }); +});