fix(review): address #230 re-review — stale breadcrumb, swallowed error, i18n, docs
Approve-with-comments follow-ups: - breadcrumb: fix the reverse regression where navigating A->B to a page absent from the lazily-built tree (before its ancestors load) left the previous page's clickable chain on screen. New pure computeBreadcrumbState clears a stale chain that doesn't end at the current page, while keeping one that does (no blank flash for an already-resolved page); unit-tested for the navigated-to-absent-page case. - share.service: getShareAncestorPage no longer swallows DB errors silently — now a live public-share path (isPageReachableThroughShare), so a transient error is logged with ancestor/child ids and still fails closed (caller 404s) instead of becoming a traceless misleading "not found". - i18n: register the new "Connecting… (read-only)" key (U+2026 ellipsis) in en-US (source of truth) and ru-RU (Подключение… (только чтение)). - share.service: correct the FUTURE note — 3 callers pass no shareId (share-alias.controller/.service, share-seo.controller); the two ai-chat callers already pass a real shareId. - CHANGELOG: add Unreleased Changed/Fixed/Security entries for #216 opt-in sub-pages default, #218 trimmed page-info payload + forged-shareId 404, #204 export internal-link name, #206/#218 breadcrumb, #192 callout paste, #218 editor pre-sync read-only gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
38
CHANGELOG.md
38
CHANGELOG.md
@@ -42,8 +42,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
catalog's raw files; the image ships a per-branch default baked in CI, and it
|
catalog's raw files; the image ships a per-branch default baked in CI, and it
|
||||||
can be overridden at runtime via the env var (see `.env.example`). (#222)
|
can be overridden at runtime via the env var (see `.env.example`). (#222)
|
||||||
|
|
||||||
|
### Changed
|
||||||
|
|
||||||
|
- **Enabling a public share no longer auto-shares the whole sub-tree.** Turning
|
||||||
|
a page "Shared to web" now defaults to the page alone; descendant pages become
|
||||||
|
public only when you explicitly turn on the dedicated "Include sub-pages"
|
||||||
|
toggle. Previously the create call defaulted to including sub-pages, silently
|
||||||
|
exposing every child of a freshly shared page. (#216)
|
||||||
|
|
||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
|
- **Internal links in exported Markdown no longer lose their visible text.** A
|
||||||
|
link whose target page name had no file extension (e.g. a bare title) was
|
||||||
|
collapsed to empty text during export, producing an unclickable, label-less
|
||||||
|
link; the page name is now preserved. (#204)
|
||||||
|
- **Deep pages no longer render a blank breadcrumb while the sidebar tree loads.**
|
||||||
|
The breadcrumb now falls back to the page's own ancestor chain (fetched
|
||||||
|
independently of the lazily-built sidebar tree) so a deep page resolves its
|
||||||
|
trail immediately; navigating away no longer leaves the previously-viewed
|
||||||
|
page's breadcrumb showing until the new one resolves. (#206, #218)
|
||||||
|
- **Pasted GitHub-style callouts (`> [!NOTE]` …) now convert to real callouts.**
|
||||||
|
GitHub admonition blocks pasted as Markdown are recognized and rendered as
|
||||||
|
callout blocks instead of plain block-quotes. (#192)
|
||||||
|
- **The editor stays read-only until collaboration has synced.** While a page is
|
||||||
|
connecting, the body is shown as a non-editable static view with a
|
||||||
|
"Connecting… (read-only)" banner, so edits typed before the document finishes
|
||||||
|
syncing can no longer be silently dropped. (#218)
|
||||||
- **A shared page now keeps EXACTLY ONE custom address (`/l/:alias`).** Editing a
|
- **A shared page now keeps EXACTLY ONE custom address (`/l/:alias`).** Editing a
|
||||||
page's vanity slug previously inserted a second `share_aliases` row instead of
|
page's vanity slug previously inserted a second `share_aliases` row instead of
|
||||||
renaming the existing one, leaving the old `/l/<old>` link live forever and
|
renaming the existing one, leaving the old `/l/<old>` link live forever and
|
||||||
@@ -63,6 +87,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
enabled, so the existing reassign-confirm flow (`409 ALIAS_REASSIGN_REQUIRED` →
|
enabled, so the existing reassign-confirm flow (`409 ALIAS_REASSIGN_REQUIRED` →
|
||||||
"Move custom address?") is discoverable instead of reading as terminal. (#227)
|
"Move custom address?") is discoverable instead of reading as terminal. (#227)
|
||||||
|
|
||||||
|
### Security
|
||||||
|
|
||||||
|
- **The anonymous public-share page payload is trimmed to an explicit allowlist.**
|
||||||
|
The `/shares/page-info` route (the only unauthenticated path serializing a
|
||||||
|
page + its share) now returns only the fields the public renderer needs;
|
||||||
|
internal metadata — creator/last-updater/contributor ids, space/workspace ids,
|
||||||
|
AI/source bookkeeping, lock/template flags, parent/position and raw timestamps
|
||||||
|
— is no longer exposed to anonymous viewers. (#218)
|
||||||
|
- **A forged or mismatched share id can no longer render a page off its slug
|
||||||
|
alone.** When the public URL carries a share id/key, the page must be reachable
|
||||||
|
through that exact share (its own share or an ancestor `includeSubPages`
|
||||||
|
share); any other value now returns the generic "not found" instead of
|
||||||
|
serving the page. (#218)
|
||||||
|
|
||||||
## [0.94.0] - 2026-06-26
|
## [0.94.0] - 2026-06-26
|
||||||
|
|
||||||
This release makes AI chat durable and fast: assistant turns are persisted to
|
This release makes AI chat durable and fast: assistant turns are persisted to
|
||||||
|
|||||||
@@ -1364,5 +1364,6 @@
|
|||||||
"Already up to date": "Already up to date",
|
"Already up to date": "Already up to date",
|
||||||
"Updated to the latest version": "Updated to the latest version",
|
"Updated to the latest version": "Updated to the latest version",
|
||||||
"This role is no longer in the catalog": "This role is no longer in the catalog",
|
"This role is no longer in the catalog": "This role is no longer in the catalog",
|
||||||
"This language is no longer available in the catalog": "This language is no longer available in the catalog"
|
"This language is no longer available in the catalog": "This language is no longer available in the catalog",
|
||||||
|
"Connecting… (read-only)": "Connecting… (read-only)"
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1222,5 +1222,6 @@
|
|||||||
"Already up to date": "Уже актуальна",
|
"Already up to date": "Уже актуальна",
|
||||||
"Updated to the latest version": "Обновлено до последней версии",
|
"Updated to the latest version": "Обновлено до последней версии",
|
||||||
"This role is no longer in the catalog": "Эта роль больше не представлена в каталоге",
|
"This role is no longer in the catalog": "Эта роль больше не представлена в каталоге",
|
||||||
"This language is no longer available in the catalog": "Этот язык больше не доступен в каталоге"
|
"This language is no longer available in the catalog": "Этот язык больше не доступен в каталоге",
|
||||||
|
"Connecting… (read-only)": "Подключение… (только чтение)"
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
import { useAtomValue } from "jotai";
|
import { useAtomValue } from "jotai";
|
||||||
import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts";
|
import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts";
|
||||||
import React, { useCallback, useEffect, useState } from "react";
|
import React, { useCallback, useEffect, useState } from "react";
|
||||||
import { resolveBreadcrumbNodes } from "./breadcrumb.utils";
|
import { computeBreadcrumbState } from "./breadcrumb.utils";
|
||||||
import {
|
import {
|
||||||
Button,
|
Button,
|
||||||
Anchor,
|
Anchor,
|
||||||
@@ -51,17 +51,19 @@ export default function Breadcrumb() {
|
|||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!currentPage) return;
|
if (!currentPage) return;
|
||||||
|
|
||||||
// Selection/mapping lives in a pure, unit-tested helper (#218). Only update
|
// Selection/mapping + stale-clearing live in a pure, unit-tested helper
|
||||||
// when it resolves nodes so a transient miss keeps the prior breadcrumb
|
// (#218). It resolves the correct chain when possible and, on a transient
|
||||||
// rather than blanking it.
|
// miss, clears a chain left over from a previously-viewed page instead of
|
||||||
const nodes = resolveBreadcrumbNodes(
|
// showing the wrong trail — while keeping a chain already resolved for THIS
|
||||||
|
// page to avoid a blank flash.
|
||||||
|
setBreadcrumbNodes((previous) =>
|
||||||
|
computeBreadcrumbState(
|
||||||
treeData,
|
treeData,
|
||||||
ancestors as IPage[] | undefined,
|
ancestors as IPage[] | undefined,
|
||||||
currentPage.id,
|
currentPage.id,
|
||||||
|
previous,
|
||||||
|
),
|
||||||
);
|
);
|
||||||
if (nodes) {
|
|
||||||
setBreadcrumbNodes(nodes);
|
|
||||||
}
|
|
||||||
}, [currentPage?.id, treeData, ancestors]);
|
}, [currentPage?.id, treeData, ancestors]);
|
||||||
|
|
||||||
const HiddenNodesTooltipContent = () =>
|
const HiddenNodesTooltipContent = () =>
|
||||||
|
|||||||
@@ -1,5 +1,8 @@
|
|||||||
import { describe, it, expect } from "vitest";
|
import { describe, it, expect } from "vitest";
|
||||||
import { resolveBreadcrumbNodes } from "./breadcrumb.utils";
|
import {
|
||||||
|
computeBreadcrumbState,
|
||||||
|
resolveBreadcrumbNodes,
|
||||||
|
} from "./breadcrumb.utils";
|
||||||
import { SpaceTreeNode } from "@/features/page/tree/types.ts";
|
import { SpaceTreeNode } from "@/features/page/tree/types.ts";
|
||||||
import { IPage } from "@/features/page/types/page.types.ts";
|
import { IPage } from "@/features/page/types/page.types.ts";
|
||||||
|
|
||||||
@@ -79,3 +82,33 @@ describe("resolveBreadcrumbNodes", () => {
|
|||||||
expect(resolveBreadcrumbNodes(null, null, "x")).toBeNull();
|
expect(resolveBreadcrumbNodes(null, null, "x")).toBeNull();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("computeBreadcrumbState (stale-chain clearing on navigation)", () => {
|
||||||
|
it("uses a freshly resolved chain when available", () => {
|
||||||
|
const child = treeNode("B");
|
||||||
|
const root = treeNode("root", { hasChildren: true, children: [child] });
|
||||||
|
const next = computeBreadcrumbState([root], null, "B", null);
|
||||||
|
expect(next!.map((n) => n.id)).toEqual(["root", "B"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("navigating A->B to a page absent from treeData clears the previous A chain (no stale trail)", () => {
|
||||||
|
// Previous chain ends at page A; we are now on page B, which is not yet in
|
||||||
|
// the lazily-built tree and whose ancestors have not loaded.
|
||||||
|
const previous = [treeNode("rootA"), treeNode("A")];
|
||||||
|
const next = computeBreadcrumbState([treeNode("unrelated")], undefined, "B", previous);
|
||||||
|
// Must NOT keep showing A's (clickable) chain.
|
||||||
|
expect(next).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("keeps a chain that already ends at the current page through a transient miss", () => {
|
||||||
|
// We already resolved B once (chain ends at B); a transient miss must not
|
||||||
|
// blank it.
|
||||||
|
const previous = [treeNode("rootB"), treeNode("B")];
|
||||||
|
const next = computeBreadcrumbState([], undefined, "B", previous);
|
||||||
|
expect(next).toBe(previous);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns null when nothing resolves and there is no previous chain", () => {
|
||||||
|
expect(computeBreadcrumbState([], undefined, "B", null)).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -10,7 +10,8 @@ import { findBreadcrumbPath, pageToTreeNode } from "@/features/page/tree/utils";
|
|||||||
* resolves immediately instead of rendering a blank breadcrumb for seconds
|
* resolves immediately instead of rendering a blank breadcrumb for seconds
|
||||||
* while the tree backfills. Mapped through the canonical `pageToTreeNode`
|
* while the tree backfills. Mapped through the canonical `pageToTreeNode`
|
||||||
* (title -> name, hasChildren defaulted to false).
|
* (title -> name, hasChildren defaulted to false).
|
||||||
* 3. neither — no data yet, return null so the caller keeps its prior state.
|
* 3. neither — no data yet, return null (the caller decides whether to keep
|
||||||
|
* a prior chain via computeBreadcrumbState).
|
||||||
*/
|
*/
|
||||||
export function resolveBreadcrumbNodes(
|
export function resolveBreadcrumbNodes(
|
||||||
treeData: SpaceTreeNode[] | null | undefined,
|
treeData: SpaceTreeNode[] | null | undefined,
|
||||||
@@ -32,3 +33,29 @@ export function resolveBreadcrumbNodes(
|
|||||||
|
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Decide the next breadcrumb state, given the previous one. When a chain
|
||||||
|
* resolves (#218) it always wins. When nothing resolves yet, a stale chain from
|
||||||
|
* a previously-viewed page must be CLEARED rather than left showing the wrong,
|
||||||
|
* clickable trail (the reverse regression of the original blank-breadcrumb fix
|
||||||
|
* when navigating A -> B to a deep page not yet in the lazily-built tree). The
|
||||||
|
* one chain we keep through a transient miss is one that already ends at the
|
||||||
|
* current page — that means we already resolved THIS page, so keeping it avoids
|
||||||
|
* a needless blank flash without ever showing the previous page's chain.
|
||||||
|
*/
|
||||||
|
export function computeBreadcrumbState(
|
||||||
|
treeData: SpaceTreeNode[] | null | undefined,
|
||||||
|
ancestors: IPage[] | null | undefined,
|
||||||
|
pageId: string,
|
||||||
|
previous: SpaceTreeNode[] | null,
|
||||||
|
): SpaceTreeNode[] | null {
|
||||||
|
const resolved = resolveBreadcrumbNodes(treeData, ancestors, pageId);
|
||||||
|
if (resolved) {
|
||||||
|
return resolved;
|
||||||
|
}
|
||||||
|
|
||||||
|
const previousEndsAtCurrentPage =
|
||||||
|
previous != null && previous[previous.length - 1]?.id === pageId;
|
||||||
|
return previousEndsAtCurrentPage ? previous : null;
|
||||||
|
}
|
||||||
|
|||||||
@@ -215,9 +215,11 @@ export class ShareService {
|
|||||||
// the access secret there — an inherited Docmost design we don't widen.
|
// the access secret there — an inherited Docmost design we don't widen.
|
||||||
// FUTURE: this ancestor-aware match could fold INTO resolveReadableSharePage
|
// FUTURE: this ancestor-aware match could fold INTO resolveReadableSharePage
|
||||||
// (so the boundary's narrow `share.id === shareId` gate isn't effectively
|
// (so the boundary's narrow `share.id === shareId` gate isn't effectively
|
||||||
// dead). Deferred — it widens the contract for the 4 other callers that pass
|
// dead). Deferred — it widens the contract for the 3 other callers that pass
|
||||||
// no shareId, so kept here as a local post-check until that's worth the blast
|
// no shareId (share-alias.controller, share-alias.service, share-seo.controller);
|
||||||
// radius.
|
// the two ai-chat callers (public-share-chat.controller,
|
||||||
|
// public-share-chat-tools.service) already pass a real shareId. Kept here as
|
||||||
|
// a local post-check until that consolidation is worth the blast radius.
|
||||||
if (dto.shareId) {
|
if (dto.shareId) {
|
||||||
const reachable = await this.isPageReachableThroughShare(
|
const reachable = await this.isPageReachableThroughShare(
|
||||||
dto.shareId,
|
dto.shareId,
|
||||||
@@ -409,7 +411,14 @@ export class ShareService {
|
|||||||
.limit(1)
|
.limit(1)
|
||||||
.executeTakeFirst();
|
.executeTakeFirst();
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
// empty
|
// Fail closed (return null -> caller 404s), but never silently: this is
|
||||||
|
// now a live public-share path (isPageReachableThroughShare), so a
|
||||||
|
// transient DB error here would otherwise turn a legitimate viewer of an
|
||||||
|
// includeSubPages descendant into a misleading "not found" with no trace.
|
||||||
|
this.logger.error(
|
||||||
|
`getShareAncestorPage failed (ancestorPageId=${ancestorPageId}, childPageId=${childPageId})`,
|
||||||
|
err instanceof Error ? err.stack : String(err),
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
return ancestor;
|
return ancestor;
|
||||||
|
|||||||
Reference in New Issue
Block a user