fix(ui)+test: QA UI bugs (#216 #218) + test coverage (#206 #204 #192) #230

Merged
vvzvlad merged 5 commits from fix/qa-ui-bugs-216-218 into develop 2026-06-27 22:50:19 +03:00
13 changed files with 540 additions and 27 deletions
Showing only changes of commit 22852be2e2 - Show all commits

View File

@@ -0,0 +1,32 @@
import { describe, it, expect } from "vitest";
import { WebSocketStatus } from "@hocuspocus/provider";
import { isCollabSynced, isBodyEditable } from "./editor-sync-state";
describe("isCollabSynced", () => {
it("is true only when Connected and synced", () => {
expect(isCollabSynced(WebSocketStatus.Connected, true)).toBe(true);
});
it("is false while connecting or not yet synced", () => {
expect(isCollabSynced(WebSocketStatus.Connecting, true)).toBe(false);
expect(isCollabSynced(WebSocketStatus.Connected, false)).toBe(false);
expect(isCollabSynced(WebSocketStatus.Disconnected, true)).toBe(false);
});
});
describe("isBodyEditable (pre-sync data-loss gate, #218)", () => {
const base = { editable: true, inEditMode: true, showStatic: false };
it("allows editing only after the static (pre-sync) phase ends", () => {
expect(isBodyEditable(base)).toBe(true);
});
it("never editable while the static read-only editor is shown", () => {
expect(isBodyEditable({ ...base, showStatic: true })).toBe(false);
});
it("honors read-only and view mode", () => {
expect(isBodyEditable({ ...base, editable: false })).toBe(false);
expect(isBodyEditable({ ...base, inEditMode: false })).toBe(false);
});
});

View File

@@ -0,0 +1,32 @@
import { WebSocketStatus } from "@hocuspocus/provider";
/**
* The collab document is usable only once the provider is Connected AND has
* synced (both the local IndexedDB replica and the remote room). Until then the
* in-browser Y.Doc is empty/stale, so edits would either be dropped or clobber
* the server's authoritative doc when it finally arrives.
*/
export function isCollabSynced(
status: WebSocketStatus | string,
isSynced: boolean,
): boolean {
return status === WebSocketStatus.Connected && isSynced;
}
/**
* Whether the page BODY editor may accept edits.
*
* `showStatic` is true during the pre-sync window (a read-only static editor is
* shown). Gating editability on `!showStatic` guarantees the body never becomes
* editable before the collab doc is synced, so early keystrokes on a freshly
* created page can't land only in local ProseMirror and then be lost when the
* server's initial empty doc syncs in (#218). Read-only and view modes are
* still honored via `editable`/`inEditMode`.
*/
export function isBodyEditable(opts: {
editable: boolean;
inEditMode: boolean;
showStatic: boolean;
}): boolean {
return opts.editable && opts.inEditMode && !opts.showStatic;
}

View File

@@ -84,6 +84,10 @@ import { PageEmbedLookupProvider } from "@/features/editor/components/page-embed
import { PageEmbedAncestryProvider } from "@/features/editor/components/page-embed/page-embed-ancestry-context";
import PageEmbedPicker from "@/features/editor/components/page-embed/page-embed-picker";
import { useTranslation } from "react-i18next";
import {
isBodyEditable,
isCollabSynced,
} from "@/features/editor/editor-sync-state";
interface PageEditorProps {
pageId: string;
@@ -440,6 +444,9 @@ export default function PageEditor({
const isSynced = isLocalSynced && isRemoteSynced;
const hasConnectedOnceRef = useRef(false);
const [showStatic, setShowStatic] = useState(true);
useEffect(() => {
const timeout = setTimeout(() => {
if (yjsConnectionStatus === WebSocketStatus.Connecting || !isSynced) {
@@ -451,17 +458,21 @@ export default function PageEditor({
}, [yjsConnectionStatus, isSynced]);
useEffect(() => {
if (!editor) return;
editor.setEditable(editable && currentPageEditMode === PageEditMode.Edit);
}, [currentPageEditMode, editor, editable]);
const hasConnectedOnceRef = useRef(false);
const [showStatic, setShowStatic] = useState(true);
// Keep the body read-only until the collab doc has synced (showStatic), so
// early keystrokes on a freshly created page can't be lost (#218).
editor.setEditable(
isBodyEditable({
editable,
inEditMode: currentPageEditMode === PageEditMode.Edit,
showStatic,
}),
);
}, [currentPageEditMode, editor, editable, showStatic]);
useEffect(() => {
if (
!hasConnectedOnceRef.current &&
yjsConnectionStatus === WebSocketStatus.Connected &&
isSynced
isCollabSynced(yjsConnectionStatus, isSynced)
) {
hasConnectedOnceRef.current = true;
setShowStatic(false);
@@ -473,17 +484,43 @@ export default function PageEditor({
<PageEmbedLookupProvider>
<PageEmbedAncestryProvider hostPageId={pageId}>
{showStatic ? (
<EditorProvider
editable={false}
immediatelyRender={true}
extensions={mainExtensions}
content={content}
editorProps={{
attributes: {
"aria-label": t("Page content"),
},
}}
/>
<div style={{ position: "relative" }}>
{/* Surface the pre-sync read-only window so edits typed before the
collab provider connects aren't silently swallowed (#218). Shown
only when the user is otherwise allowed to edit. */}
{editable && currentPageEditMode === PageEditMode.Edit && (
<div
role="status"
aria-live="polite"
className="print-hide"
style={{
position: "absolute",
top: 0,
right: 0,
zIndex: 2,
padding: "2px 8px",
fontSize: "12px",
borderRadius: "4px",
background: "var(--mantine-color-gray-light)",
color: "var(--mantine-color-dimmed)",
pointerEvents: "none",
}}
>
{t("Connecting… (read-only)")}
</div>
)}
<EditorProvider
editable={false}
immediatelyRender={true}
extensions={mainExtensions}
content={content}
editorProps={{
attributes: {
"aria-label": t("Page content"),
},
}}
/>
</div>
) : (
<div className="editor-container" style={{ position: "relative" }}>
<div ref={menuContainerRef}>

View File

@@ -16,7 +16,10 @@ import { Link, useParams } from "react-router-dom";
import classes from "./breadcrumb.module.css";
import { SpaceTreeNode } from "@/features/page/tree/types.ts";
import { buildPageUrl } from "@/features/page/page.utils.ts";
import { usePageQuery } from "@/features/page/queries/page-query.ts";
import {
usePageQuery,
usePageBreadcrumbsQuery,
} from "@/features/page/queries/page-query.ts";
import { extractPageSlugId } from "@/lib";
import { useMediaQuery } from "@mantine/hooks";
import { useTranslation } from "react-i18next";
@@ -38,14 +41,43 @@ export default function Breadcrumb() {
const { data: currentPage } = usePageQuery({
pageId: extractPageSlugId(pageSlug),
});
// The page's own ancestor chain, fetched independently of the lazily-built
// sidebar tree so a deep page doesn't render a blank breadcrumb for seconds
// while the tree backfills (#218).
const { data: ancestors } = usePageBreadcrumbsQuery(currentPage?.id);
const isMobile = useMediaQuery("(max-width: 48em)");
useEffect(() => {
if (treeData?.length > 0 && currentPage) {
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);
setBreadcrumbNodes(breadcrumb || null);
if (breadcrumb) {
setBreadcrumbNodes(breadcrumb);
return;
}
}
}, [currentPage?.id, treeData]);
// 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[],
);
}
}, [currentPage?.id, treeData, ancestors]);
const HiddenNodesTooltipContent = () =>
breadcrumbNodes?.slice(1, -1).map((node) => (

View File

@@ -73,7 +73,10 @@ export default function ShareModal({ readOnly }: ShareModalProps) {
if (value) {
await createShareMutation.mutateAsync({
pageId: pageId,
includeSubPages: true,
// Opt-in: enabling a share must NOT silently expose the whole
// sub-tree (#216). Sub-pages are shared only when the user turns on
// the dedicated "Include sub-pages" toggle.
includeSubPages: false,
searchIndexing: false,
});
} else if (share && share.id) {

View File

@@ -73,6 +73,10 @@ export type IUpdateShare = ICreateShare & { shareId: string; pageId?: string };
export interface IShareInfoInput {
pageId: string;
// The share id/key from the `/share/:shareId/p/:slug` URL. When present the
// server binds content access to this exact share (#218): a forged/mismatched
// shareId 404s instead of rendering the page off its slug alone.
shareId?: string;
}
// Vanity /l/:alias pointer.

View File

@@ -24,6 +24,9 @@ export default function SharedPage() {
const { data, isLoading, isError, error } = useSharePageQuery({
pageId: extractPageSlugId(pageSlug),
// Forward the URL's shareId so the server binds content to this share
// (#218): a forged shareId 404s instead of rendering the page off its slug.
shareId,
});
const sharedTreeData = useAtomValue(sharedTreeDataAtom);

View File

@@ -0,0 +1,161 @@
import { NotFoundException } from '@nestjs/common';
import { ShareService } from './share.service';
/**
* Regression for issue #218: public-share content must be bound to the requested
* shareId. `getSharedPage` resolves the page off its slug, but when the caller
* supplies a shareId it must be reachable THROUGH that exact share — a forged or
* mismatched shareId 404s instead of rendering the page off its slug alone. A
* request with no shareId keeps the legacy slug-capability behavior.
*/
const WS = 'ws-1';
const PAGE_ID = 'page-uuid-1';
const OWN_SHARE_ID = 'share-own';
const OWN_SHARE_KEY = 'ownkey';
function buildService(over: {
resolvedShare?: any;
ancestorShare?: any; // returned by shareRepo.findById(requestedShareId)
ancestorFound?: boolean; // getShareAncestorPage result
} = {}) {
const resolvedShare = over.resolvedShare ?? {
id: OWN_SHARE_ID,
key: OWN_SHARE_KEY,
includeSubPages: false,
spaceId: 'space-1',
workspaceId: WS,
};
const page = { id: PAGE_ID, deletedAt: null, content: { type: 'doc' } };
const shareRepo = {
findById: jest.fn(async () => over.ancestorShare ?? null),
};
const service = new ShareService(
shareRepo as any,
{} as any, // pageRepo (resolveReadableSharePage is spied)
{} as any, // pagePermissionRepo
{} as any, // db
{} as any, // tokenService
{} as any, // transclusionService
{} as any, // workspaceRepo
);
jest
.spyOn(service, 'resolveReadableSharePage')
.mockResolvedValue({ share: resolvedShare, page } as any);
jest
.spyOn(service, 'updatePublicAttachments')
.mockResolvedValue(page.content as any);
jest
.spyOn(service, 'getShareAncestorPage')
.mockResolvedValue(over.ancestorFound ? { id: 'anc' } : null);
return { service, shareRepo, page, resolvedShare };
}
describe('ShareService.getSharedPage — share binding (#218)', () => {
it('returns the page when no shareId is supplied (legacy slug path)', async () => {
const { service } = buildService();
const out = await service.getSharedPage({ pageId: PAGE_ID } as any, WS);
expect(out.page.id).toBe(PAGE_ID);
});
it('returns the page when the shareId matches the resolved share key', async () => {
const { service } = buildService();
const out = await service.getSharedPage(
{ pageId: PAGE_ID, shareId: OWN_SHARE_KEY } as any,
WS,
);
expect(out.page.id).toBe(PAGE_ID);
});
it('returns the page when the shareId matches the resolved share id (case-insensitive key)', async () => {
const { service } = buildService();
const out = await service.getSharedPage(
{ pageId: PAGE_ID, shareId: OWN_SHARE_KEY.toUpperCase() } as any,
WS,
);
expect(out.page.id).toBe(PAGE_ID);
});
it('404s for a forged shareId that resolves to nothing', async () => {
const { service } = buildService({ ancestorShare: null });
await expect(
service.getSharedPage(
{ pageId: PAGE_ID, shareId: 'doesnotexist99' } as any,
WS,
),
).rejects.toBeInstanceOf(NotFoundException);
});
it('allows an includeSubPages ANCESTOR share that contains the page', async () => {
const { service } = buildService({
ancestorShare: {
id: 'ancestor-share',
pageId: 'ancestor-page',
includeSubPages: true,
workspaceId: WS,
},
ancestorFound: true,
});
const out = await service.getSharedPage(
{ pageId: PAGE_ID, shareId: 'ancestorkey' } as any,
WS,
);
expect(out.page.id).toBe(PAGE_ID);
});
it('404s for a different share WITHOUT includeSubPages', async () => {
const { service } = buildService({
ancestorShare: {
id: 'other-share',
pageId: 'other-page',
includeSubPages: false,
workspaceId: WS,
},
});
await expect(
service.getSharedPage(
{ pageId: PAGE_ID, shareId: 'otherkey' } as any,
WS,
),
).rejects.toBeInstanceOf(NotFoundException);
});
it('404s for an includeSubPages share that does NOT contain the page', async () => {
const { service } = buildService({
ancestorShare: {
id: 'unrelated-share',
pageId: 'unrelated-page',
includeSubPages: true,
workspaceId: WS,
},
ancestorFound: false,
});
await expect(
service.getSharedPage(
{ pageId: PAGE_ID, shareId: 'unrelatedkey' } as any,
WS,
),
).rejects.toBeInstanceOf(NotFoundException);
});
it('404s for a share in a different workspace', async () => {
const { service } = buildService({
ancestorShare: {
id: 'foreign-share',
pageId: 'foreign-page',
includeSubPages: true,
workspaceId: 'other-ws',
},
ancestorFound: true,
});
await expect(
service.getSharedPage(
{ pageId: PAGE_ID, shareId: 'foreignkey' } as any,
WS,
),
).rejects.toBeInstanceOf(NotFoundException);
});
});

View File

@@ -93,8 +93,30 @@ 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.
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 {
...shareData,
page: publicPage,
share: publicShare,
aiAssistant,
aiAssistantName,
features: this.licenseCheckService.resolveFeatures(

View File

@@ -189,9 +189,9 @@ export class ShareService {
}
async getSharedPage(dto: ShareInfoDto, workspaceId: string) {
// Resolve via the single canonical boundary. There is no independent
// requested shareId here (the share is resolved FROM the page), so no
// share-id match is performed.
// Resolve via the single canonical boundary. The share is resolved FROM the
// page (the request carries the page slug), so the boundary itself performs
// no share-id match here.
const resolved = await this.resolveReadableSharePage(
null,
dto.pageId,
@@ -205,11 +205,64 @@ export class ShareService {
const { share, page } = resolved;
// Bind content to the requested share (#218). When the caller supplies a
// shareId/key (the `/share/:shareId/p/:slug` route now forwards it), the
// page must be reachable THROUGH that exact share — a forged or mismatched
// shareId must 404 instead of rendering the page off its slug alone, and it
// must not be answerable with the page's real (canonical) share key. A
// 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.
if (dto.shareId) {
const reachable = await this.isPageReachableThroughShare(
dto.shareId,
share,
page.id,
workspaceId,
);
if (!reachable) {
throw new NotFoundException('Shared page not found');
}
}
page.content = await this.updatePublicAttachments(page);
return { page, share };
}
/**
* Does `requestedShareId` (a share id OR key) legitimately grant access to
* `pageId`? True when it names the page's own resolved share, or an ancestor
* share with `includeSubPages` that contains the page. Any other value
* (unknown key, wrong workspace, a sibling share that doesn't cover the page)
* is false, so a guessed slug paired with a forged shareId can't render.
*/
private async isPageReachableThroughShare(
requestedShareId: string,
resolvedShare: NonNullable<
Awaited<ReturnType<ShareService['getShareForPage']>>
>,
pageId: string,
workspaceId: string,
): Promise<boolean> {
// Fast path: the request names the page's own resolved share.
if (
requestedShareId === resolvedShare.id ||
requestedShareId.toLowerCase() === resolvedShare.key?.toLowerCase()
) {
return true;
}
// Otherwise it may name an includeSubPages ANCESTOR share: the page has its
// own closer share but is also served under the ancestor's public tree.
const requested = await this.shareRepo.findById(requestedShareId);
if (!requested || requested.workspaceId !== workspaceId) return false;
if (!requested.includeSubPages) return false;
const ancestor = await this.getShareAncestorPage(requested.pageId, pageId);
return !!ancestor;
}
async getShareForPage(pageId: string, workspaceId: string) {
// here we try to check if a page was shared directly or if it inherits the share from its closest shared ancestor
const share = await this.db

View File

@@ -0,0 +1,54 @@
import { describe, it, expect } from "vitest";
import { markdownToHtml } from "./marked.utils";
/**
* Regression for issue #218: pasting a GitHub-style `> [!type]` alert produced a
* literal `<blockquote>` containing `[!info]` instead of a callout node, because
* only the `:::type` form was tokenized. The editor paste path runs the same
* `markdownToHtml`, so these assertions pin the conversion at the source.
*/
function html(md: string): string {
const out = markdownToHtml(md);
if (typeof out !== "string") throw new Error("expected sync string output");
return out;
}
describe("markdownToHtml: GitHub `> [!type]` callouts", () => {
it("converts `> [!info]` to a callout node, not a literal blockquote", () => {
const out = html("> [!info]\n> Callout body text here");
expect(out).toContain('data-type="callout"');
expect(out).toContain('data-callout-type="info"');
expect(out).toContain("Callout body text here");
expect(out).not.toContain("[!info]");
expect(out).not.toContain("<blockquote");
});
it("maps GitHub alert aliases onto the supported banner types", () => {
expect(html("> [!NOTE]\n> x")).toContain('data-callout-type="info"');
expect(html("> [!TIP]\n> x")).toContain('data-callout-type="success"');
expect(html("> [!WARNING]\n> x")).toContain('data-callout-type="warning"');
expect(html("> [!CAUTION]\n> x")).toContain('data-callout-type="danger"');
});
it("accepts the editor's own type names directly", () => {
expect(html("> [!success]\n> x")).toContain('data-callout-type="success"');
expect(html("> [!danger]\n> x")).toContain('data-callout-type="danger"');
});
it("falls back to info for an unknown type", () => {
expect(html("> [!bogus]\n> x")).toContain('data-callout-type="info"');
});
it("preserves multi-line callout bodies", () => {
const out = html("> [!warning]\n> line one\n> line two");
expect(out).toContain('data-callout-type="warning"');
expect(out).toContain("line one");
expect(out).toContain("line two");
});
it("still converts the `:::type` form", () => {
const out = html(":::info\nbody\n:::");
expect(out).toContain('data-type="callout"');
expect(out).toContain('data-callout-type="info"');
});
});

View File

@@ -0,0 +1,78 @@
import { Token, marked } from 'marked';
interface GithubCalloutToken {
type: 'githubCallout';
calloutType: string;
text: string;
raw: string;
}
/**
* Map GitHub "alert" blockquote markers (`> [!NOTE]`, `> [!WARNING]`, …) onto
* the four callout banner types the editor schema supports. The editor's own
* type names (`info`/`success`/`warning`/`danger`) are also accepted directly,
* because users paste both forms. Anything unrecognized falls back to `info`,
* matching the `:::type` callout tokenizer.
*/
const GITHUB_ALERT_TYPE_MAP: Record<string, string> = {
note: 'info',
tip: 'success',
important: 'info',
warning: 'warning',
caution: 'danger',
info: 'info',
success: 'success',
danger: 'danger',
};
/**
* Tokenizer for GitHub-flavored alert callouts written as a blockquote whose
* first line is `[!type]`:
*
* > [!info]
* > body line one
* > body line two
*
* Without this, the default blockquote tokenizer wins and the marker renders as
* a literal `[!info]` inside a `<blockquote>`. The editor's paste path runs the
* same `markdownToHtml`, so registering this here also fixes pasting the syntax
* into the editor (issue #218), not just markdown import.
*/
export const githubCalloutExtension = {
name: 'githubCallout',
level: 'block' as const,
start(src: string) {
return src.match(/^ {0,3}>[ \t]*\[!/m)?.index ?? -1;
},
tokenizer(src: string): GithubCalloutToken | undefined {
const rule =
/^ {0,3}>[ \t]*\[!([a-zA-Z]+)\][^\n]*(?:\n {0,3}>[^\n]*)*(?:\n|$)/;
const match = rule.exec(src);
if (!match) return undefined;
const rawType = match[1].toLowerCase();
const calloutType = GITHUB_ALERT_TYPE_MAP[rawType] ?? 'info';
const text = match[0]
.replace(/\n+$/, '')
.split('\n')
// Strip the blockquote marker (`>` + optional space) from every line.
.map((line) => line.replace(/^ {0,3}>[ \t]?/, ''))
// Drop the `[!type]` marker that opens the first line.
.map((line, i) => (i === 0 ? line.replace(/^\[![a-zA-Z]+\][ \t]*/, '') : line))
.join('\n')
.trim();
return {
type: 'githubCallout',
calloutType,
raw: match[0],
text,
};
},
renderer(token: Token) {
const calloutToken = token as GithubCalloutToken;
const body = marked.parse(calloutToken.text);
return `<div data-type="callout" data-callout-type="${calloutToken.calloutType}">${body}</div>`;
},
};

View File

@@ -1,5 +1,6 @@
import { marked } from "marked";
import { calloutExtension } from "./callout.marked";
import { githubCalloutExtension } from "./github-callout.marked";
import { mathBlockExtension } from "./math-block.marked";
import { mathInlineExtension } from "./math-inline.marked";
import {
@@ -41,6 +42,7 @@ marked.use({
marked.use({
extensions: [
calloutExtension,
githubCalloutExtension,
mathBlockExtension,
mathInlineExtension,
footnoteReferenceExtension,