From 4f55e8e5f3e5ebcd0e61418c44d5fd530677f7a1 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Tue, 5 May 2026 08:06:14 -0300 Subject: [PATCH] fix(ui): aggregate comments across body + headers/footers/notes (SD-2938) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ui.comments.observe() collapsed to one section as the user moved focus between the body and a header/footer/footnote/endnote. Each section editor owns its own converter.comments store, and the controller's snapshot was sourced from whichever editor was routed at the time — losing the rest. The fix adds an `in?: StoryLocator | 'all'` option to CommentsListQuery (mirroring the trackChanges pattern), extends CommentAddress with an optional `story`, and stamps that field on non-body results. The controller now reads from the host editor with `{ in: 'all' }`, so snapshots stay stable across focus changes. Action routing reuses the same locator: resolve, reopen, reply, delete, and scrollTo look the comment up in the cached snapshot and dispatch on the editor that owns it. Body comments still route the same way they did before — story is omitted, fallback path is unchanged. createFromSelection / createFromCapture stay routed because they operate on the live selection. Event fan-out is intentionally conservative: existing commentsUpdate/commentsLoaded refresh + the explicit refreshAndNotify after UI mutations handle the main bug. Multi-story listener wiring is left for a follow-up if it's needed. --- .../src/comments/comments.types.ts | 16 + packages/document-api/src/index.ts | 3 +- packages/document-api/src/types/address.ts | 16 +- .../presentation-editor/PresentationEditor.ts | 35 +- .../plan-engine/comments-wrappers.test.ts | 188 +++++++++++ .../plan-engine/comments-wrappers.ts | 149 +++++++- packages/super-editor/src/ui/comments.test.ts | 319 ++++++++++++++++++ .../src/ui/create-super-doc-ui.ts | 117 ++++++- 8 files changed, 809 insertions(+), 34 deletions(-) diff --git a/packages/document-api/src/comments/comments.types.ts b/packages/document-api/src/comments/comments.types.ts index 95cede1e23..f2bd31a678 100644 --- a/packages/document-api/src/comments/comments.types.ts +++ b/packages/document-api/src/comments/comments.types.ts @@ -1,8 +1,17 @@ import type { CommentAddress, CommentStatus, TextTarget } from '../types/index.js'; import type { DiscoveryOutput } from '../types/discovery.js'; +import type { StoryLocator } from '../types/story.types.js'; export type { CommentStatus } from '../types/index.js'; +/** + * Scope marker used by {@link CommentsListQuery.in} to request comments + * across every story (body + headers + footers + footnotes + endnotes). + * Equivalent to a multi-story aggregate list. + */ +export const COMMENTS_IN_ALL = 'all' as const; +export type CommentsInAll = typeof COMMENTS_IN_ALL; + export interface CommentInfo { address: CommentAddress; commentId: string; @@ -22,6 +31,13 @@ export interface CommentsListQuery { includeResolved?: boolean; limit?: number; offset?: number; + /** + * Story scope. + * - `undefined` (default): body only (backward compatible). + * - A {@link StoryLocator}: only that story. + * - `'all'`: flat list across body + every non-body story. + */ + in?: StoryLocator | CommentsInAll; } /** diff --git a/packages/document-api/src/index.ts b/packages/document-api/src/index.ts index 94175ab1c6..2bf4cc2973 100644 --- a/packages/document-api/src/index.ts +++ b/packages/document-api/src/index.ts @@ -1402,7 +1402,8 @@ export type { GoToCommentInput, SetCommentActiveInput, } from './comments/comments.js'; -export type { CommentInfo, CommentsListQuery, CommentsListResult } from './comments/comments.types.js'; +export type { CommentInfo, CommentsListQuery, CommentsListResult, CommentsInAll } from './comments/comments.types.js'; +export { COMMENTS_IN_ALL } from './comments/comments.types.js'; export { DocumentApiValidationError } from './errors.js'; export { textReceiptToSDReceipt, buildStructuralReceipt } from './receipt-bridge.js'; export type { StructuralReceiptParams } from './receipt-bridge.js'; diff --git a/packages/document-api/src/types/address.ts b/packages/document-api/src/types/address.ts index 78758019be..a37ffe3e70 100644 --- a/packages/document-api/src/types/address.ts +++ b/packages/document-api/src/types/address.ts @@ -119,13 +119,9 @@ export type EntityType = 'comment' | 'trackedChange'; export type CommentAddress = { kind: 'entity'; entityType: 'comment'; - /** - * Comment navigation is currently body-scoped only. - * - * Unlike bookmark and tracked-change navigation, `navigateTo()` does not yet - * accept a `story` locator for comments. - */ entityId: string; + /** Story containing this comment. Omit for body (backward compatible). */ + story?: StoryLocator; }; export type TrackedChangeAddress = { @@ -169,11 +165,11 @@ export type BlockNavigationAddress = { * Supports navigation to: * - Blocks by `nodeId` in the body story * - Bookmarks by `name`, optionally scoped to a `story` - * - Comments by `entityId` in the body story + * - Comments by `entityId`, optionally scoped to a `story` * - Tracked changes by `entityId`, optionally scoped to a `story` * - * Story-aware navigation is currently supported for bookmarks and tracked - * changes. Block and comment targets remain body-only until the runtime gains - * equivalent non-body resolution paths. + * Story-aware navigation is supported for bookmarks, tracked changes, and + * comments. Block targets remain body-only until the runtime gains equivalent + * non-body resolution paths. */ export type NavigableAddress = BlockNavigationAddress | BookmarkAddress | CommentAddress | TrackedChangeAddress; diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts index 158a7851c6..8e131e4dfd 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts @@ -8140,7 +8140,7 @@ export class PresentationEditor extends EventEmitter { return await this.#navigateToBookmark(target); } if (target.entityType === 'comment') { - return await this.#navigateToComment(target.entityId, options); + return await this.#navigateToComment(target.entityId, resolveStoryKeyFromAddress(target.story), options); } if (target.entityType === 'trackedChange') { return await this.#navigateToTrackedChange( @@ -8222,11 +8222,34 @@ export class PresentationEditor extends EventEmitter { async #navigateToComment( entityId: string, + storyKey: string | undefined, options: { behavior?: ScrollBehavior; block?: 'start' | 'center' | 'end' | 'nearest' } = {}, ): Promise { const editor = this.#editor; if (!editor) return false; + const behavior = options.behavior ?? 'auto'; + const block = options.block ?? 'center'; + + // Non-body comments live in stories whose anchors aren't reachable + // through the body editor's `setCursorById`. Fall back to scrolling + // the rendered DOM element directly. `findRenderedCommentElements` + // is already story-aware. When the story isn't currently painted + // (e.g. a footnote that hasn't been expanded), the lookup returns + // an empty array and we surface a structured `false` rather than + // landing the cursor in the wrong place. + if (storyKey && storyKey !== BODY_STORY_KEY) { + const elements = findRenderedCommentElements(this.#painterHost, entityId, storyKey); + const candidate = elements[0]; + if (!candidate) return false; + try { + candidate.scrollIntoView({ behavior, block, inline: 'nearest' }); + return true; + } catch { + return false; + } + } + const setCursorById = editor.commands?.setCursorById; if (typeof setCursorById !== 'function') return false; @@ -8234,12 +8257,10 @@ export class PresentationEditor extends EventEmitter { return false; } - // Scroll the viewport — setCursorById places the cursor but doesn't - // scroll in presentation mode where DomPainter renders the output. - await this.scrollToPositionAsync(editor.state.selection.from, { - behavior: options.behavior ?? 'auto', - block: options.block ?? 'center', - }); + // Scroll the viewport. `setCursorById` places the cursor but + // doesn't scroll in presentation mode where DomPainter renders + // the output. + await this.scrollToPositionAsync(editor.state.selection.from, { behavior, block }); return true; } diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.test.ts b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.test.ts index 3a44dce750..d37d70eaf8 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.test.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.test.ts @@ -30,9 +30,20 @@ vi.mock('../helpers/adapter-utils.js', async () => { }; }); +vi.mock('../tracked-changes/enumerate-stories.js', () => ({ + enumerateRevisionCapableStories: vi.fn(() => [{ kind: 'story', storyType: 'body' }]), +})); + +vi.mock('../story-runtime/resolve-story-runtime.js', () => ({ + resolveStoryRuntime: vi.fn(), +})); + import { listCommentAnchors } from '../helpers/comment-target-resolver.js'; import { resolveTextTarget } from '../helpers/adapter-utils.js'; import { executeDomainCommand } from './plan-wrappers.js'; +import { enumerateRevisionCapableStories } from '../tracked-changes/enumerate-stories.js'; +import { resolveStoryRuntime } from '../story-runtime/resolve-story-runtime.js'; +import type { StoryLocator } from '@superdoc/document-api'; function makeAnchor( overrides: Partial & { commentId: string; pos: number; end: number }, @@ -840,3 +851,180 @@ describe('comments-wrappers: addCommentHandler multi-segment targets', () => { expect(editor.commands!.addComment).not.toHaveBeenCalled(); }); }); + +describe('comments-wrappers: cross-section listing (SD-2938)', () => { + beforeEach(() => { + vi.clearAllMocks(); + // Earlier suites set per-test mockReturnValue on listCommentAnchors + // to seed anchors for "c1". clearAllMocks wipes call history but + // not implementations, so reset to the default empty list here. + vi.mocked(listCommentAnchors).mockReturnValue([]); + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }]); + }); + + const headerLocator: StoryLocator = { + kind: 'story', + storyType: 'headerFooterPart', + refId: 'rId6', + }; + + it('returns body-only items when query.in is undefined (backward compatible)', () => { + const editor = makeEditor([{ commentId: 'c-body', commentText: 'body comment' }]); + const wrapper = createCommentsWrapper(editor); + + const result = wrapper.list(); + + expect(result.items).toHaveLength(1); + expect(result.items[0]!.id).toBe('c-body'); + expect(result.items[0]!.address.story).toBeUndefined(); + expect(enumerateRevisionCapableStories).not.toHaveBeenCalled(); + expect(resolveStoryRuntime).not.toHaveBeenCalled(); + }); + + it('aggregates body + header comments and stamps story on the header item when in: "all"', () => { + const bodyEditor = makeEditor([{ commentId: 'c-body', commentText: 'body comment', createdTime: 1 }]); + const headerEditor = makeEditor([{ commentId: 'c-header', commentText: 'header comment', createdTime: 2 }]); + + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }, headerLocator]); + vi.mocked(resolveStoryRuntime).mockReturnValue({ editor: headerEditor } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + const result = wrapper.list({ in: 'all' }); + + const ids = result.items.map((i) => i.id); + expect(ids).toEqual(['c-body', 'c-header']); + + const bodyItem = result.items.find((i) => i.id === 'c-body')!; + const headerItem = result.items.find((i) => i.id === 'c-header')!; + + expect(bodyItem.address.story).toBeUndefined(); + expect(headerItem.address.story).toEqual(headerLocator); + }); + + it('returns only the targeted story when query.in is a non-body locator', () => { + const bodyEditor = makeEditor([{ commentId: 'c-body', commentText: 'body comment' }]); + const headerEditor = makeEditor([{ commentId: 'c-header', commentText: 'header comment' }]); + + vi.mocked(resolveStoryRuntime).mockReturnValue({ editor: headerEditor } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + const result = wrapper.list({ in: headerLocator }); + + expect(result.items).toHaveLength(1); + expect(result.items[0]!.id).toBe('c-header'); + expect(result.items[0]!.address.story).toEqual(headerLocator); + expect(enumerateRevisionCapableStories).not.toHaveBeenCalled(); + }); + + it('skips a non-body story when its runtime cannot be resolved', () => { + const bodyEditor = makeEditor([{ commentId: 'c-body', commentText: 'body comment' }]); + + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }, headerLocator]); + vi.mocked(resolveStoryRuntime).mockImplementation(() => { + throw new Error('story runtime missing'); + }); + + const wrapper = createCommentsWrapper(bodyEditor); + const result = wrapper.list({ in: 'all' }); + + expect(result.items).toHaveLength(1); + expect(result.items[0]!.id).toBe('c-body'); + }); + + it('deduplicates if the same comment id appears in body and a non-body store', () => { + const bodyEditor = makeEditor([{ commentId: 'c-shared', commentText: 'body copy', createdTime: 1 }]); + const headerEditor = makeEditor([{ commentId: 'c-shared', commentText: 'header copy', createdTime: 2 }]); + + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }, headerLocator]); + vi.mocked(resolveStoryRuntime).mockReturnValue({ editor: headerEditor } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + const result = wrapper.list({ in: 'all' }); + + expect(result.items).toHaveLength(1); + expect(result.items[0]!.id).toBe('c-shared'); + // Body wins because body is enumerated first. + expect(result.items[0]!.address.story).toBeUndefined(); + }); + + it('prefers the story-stamped record when body has only a definition (no anchor)', () => { + // OOXML imports flatten every comment definition into the body's + // converter.comments. For a comment anchored in the header, the + // body pass yields a definition-only record (no target). The + // header pass yields the same id with target + story. The merge + // must keep the header version so UI actions route by story. + const bodyEditor = makeEditor([{ commentId: 'c-header-only', commentText: 'lives in header' }]); + const headerEditor = makeEditor([{ commentId: 'c-header-only', commentText: 'lives in header' }]); + + vi.mocked(listCommentAnchors).mockImplementation(((editor: unknown) => { + if (editor === bodyEditor) return [] as never; + return [makeAnchor({ commentId: 'c-header-only', pos: 0, end: 6 })] as never; + }) as never); + + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }, headerLocator]); + vi.mocked(resolveStoryRuntime).mockReturnValue({ editor: headerEditor } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + const result = wrapper.list({ in: 'all' }); + + expect(result.items).toHaveLength(1); + const item = result.items[0]!; + expect(item.id).toBe('c-header-only'); + expect(item.address.story).toEqual(headerLocator); + expect(item.target).toBeDefined(); + expect(item.target?.story).toEqual(headerLocator); + }); + + it('disposes a non-cacheable runtime returned during the in: all walk', () => { + const bodyEditor = makeEditor(); + const headerEditor = makeEditor(); + const dispose = vi.fn(); + + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }, headerLocator]); + vi.mocked(resolveStoryRuntime).mockReturnValue({ + editor: headerEditor, + cacheable: false, + dispose, + } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + wrapper.list({ in: 'all' }); + + expect(dispose).toHaveBeenCalledTimes(1); + }); + + it('does not dispose a cacheable runtime during the in: all walk', () => { + const bodyEditor = makeEditor(); + const headerEditor = makeEditor(); + const dispose = vi.fn(); + + vi.mocked(enumerateRevisionCapableStories).mockReturnValue([{ kind: 'story', storyType: 'body' }, headerLocator]); + vi.mocked(resolveStoryRuntime).mockReturnValue({ + editor: headerEditor, + cacheable: true, + dispose, + } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + wrapper.list({ in: 'all' }); + + expect(dispose).not.toHaveBeenCalled(); + }); + + it('disposes a non-cacheable runtime returned for a single-story query', () => { + const bodyEditor = makeEditor(); + const headerEditor = makeEditor(); + const dispose = vi.fn(); + + vi.mocked(resolveStoryRuntime).mockReturnValue({ + editor: headerEditor, + cacheable: false, + dispose, + } as never); + + const wrapper = createCommentsWrapper(bodyEditor); + wrapper.list({ in: headerLocator }); + + expect(dispose).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts index a828ee85a4..2c2c40e2b0 100644 --- a/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts +++ b/packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/comments-wrappers.ts @@ -26,10 +26,13 @@ import type { RevisionGuardOptions, SetCommentActiveInput, SetCommentInternalInput, + StoryLocator, TextSegment, TextTarget, } from '@superdoc/document-api'; -import { buildResolvedHandle, buildDiscoveryItem, buildDiscoveryResult } from '@superdoc/document-api'; +import { buildResolvedHandle, buildDiscoveryItem, buildDiscoveryResult, COMMENTS_IN_ALL } from '@superdoc/document-api'; +import { resolveStoryRuntime } from '../story-runtime/resolve-story-runtime.js'; +import { enumerateRevisionCapableStories } from '../tracked-changes/enumerate-stories.js'; import { TextSelection } from 'prosemirror-state'; import { v4 as uuidv4 } from 'uuid'; import { DocumentApiAdapterError } from '../errors.js'; @@ -370,7 +373,20 @@ function mergeAnchorData( } } -function buildCommentInfos(editor: Editor): CommentInfo[] { +/** + * Stamps `address.story` and `target.story` on each comment when a non-body + * locator is supplied. Body comments leave both `story` fields undefined for + * backward compatibility, matching the track-changes convention. + */ +function stampStoryLocator(infosById: Map, story: StoryLocator | undefined): void { + if (!story || story.storyType === 'body') return; + for (const info of infosById.values()) { + info.address = { ...info.address, story }; + if (info.target) info.target = { ...info.target, story }; + } +} + +function buildCommentInfos(editor: Editor, story?: StoryLocator): CommentInfo[] { const store = getCommentEntityStore(editor); const infosById = new Map(); @@ -381,6 +397,7 @@ function buildCommentInfos(editor: Editor): CommentInfo[] { } mergeAnchorData(editor, infosById, listCommentAnchorsSafe(editor)); + stampStoryLocator(infosById, story); // Inherit target + anchoredText from nearest anchored ancestor for replies. // Walks up the parent chain so deep threads resolve regardless of iteration order. @@ -1022,10 +1039,136 @@ function getCommentHandler(editor: Editor, input: GetCommentInput): CommentInfo return found; } +/** + * Comparator that matches `buildCommentInfos`'s in-story sort: createdTime + * ascending, target start ascending, then commentId. Re-applied to the + * aggregated cross-story list so the merge order is deterministic. + */ +function compareCommentInfos(left: CommentInfo, right: CommentInfo): number { + const leftCreated = left.createdTime ?? 0; + const rightCreated = right.createdTime ?? 0; + if (leftCreated !== rightCreated) return leftCreated - rightCreated; + + const leftStart = left.target?.segments[0]?.range.start ?? Number.MAX_SAFE_INTEGER; + const rightStart = right.target?.segments[0]?.range.start ?? Number.MAX_SAFE_INTEGER; + if (leftStart !== rightStart) return leftStart - rightStart; + + return left.commentId.localeCompare(right.commentId); +} + +/** + * Aggregates comments across body + every non-body story (headers, footers, + * footnotes, endnotes). Reuses `enumerateRevisionCapableStories` because the + * comment-capable story set matches the revision-capable one. Anywhere a + * user can type, they can comment. + * + * Each non-body editor is resolved lazily through `resolveStoryRuntime`. That + * helper caches per-story so repeated calls are cheap. Runtimes flagged as + * `cacheable: false` are headless views over a part that has no live editor; + * we dispose them at the end of the walk so a `{ in: 'all' }` refresh against + * a doc with non-mounted stories does not allocate a fresh editor on every + * call. + * + * The body pass observes the global `comments.xml` definition list (every + * comment in the doc, regardless of where its anchor lives). For comments + * anchored outside the body, that pass produces a definition-only record + * with no `target`. Later passes for the owning story produce the same id + * with a `target` and a `story` locator. The merge below prefers the + * story-stamped record so UI actions route by `address.story` instead of + * mistaking a header comment for a body one. + */ +function listCommentsAcrossStories(hostEditor: Editor): CommentInfo[] { + const stories = enumerateRevisionCapableStories(hostEditor); + const merged = new Map(); + const transientRuntimes: Array<() => void> = []; + + try { + for (const story of stories) { + let storyEditor: Editor | null = null; + if (story.storyType === 'body') { + storyEditor = hostEditor; + } else { + let runtime; + try { + runtime = resolveStoryRuntime(hostEditor, story); + } catch { + // Story can't be resolved (missing converter data, in-flight teardown); + // skip rather than fail the whole list call. + continue; + } + storyEditor = runtime.editor; + if (runtime.cacheable === false && typeof runtime.dispose === 'function') { + transientRuntimes.push(runtime.dispose); + } + } + + const infos = buildCommentInfos(storyEditor, story); + for (const info of infos) { + const existing = merged.get(info.commentId); + if (!existing) { + merged.set(info.commentId, info); + continue; + } + // Prefer the entry with anchor data + story locator over a body + // definition-only record. Without this, headers/footers get + // routed as body comments after the first list({ in: 'all' }). + if (!existing.target && info.target) { + merged.set(info.commentId, info); + } + } + } + } finally { + for (const dispose of transientRuntimes) { + try { + dispose(); + } catch { + // best-effort + } + } + } + + return Array.from(merged.values()).sort(compareCommentInfos); +} + function listCommentsHandler(editor: Editor, query?: CommentsListQuery): CommentsListResult { validatePaginationInput(query?.offset, query?.limit); - const comments = buildCommentInfos(editor); + const scope = query?.in; + let comments: CommentInfo[]; + let disposeTransient: (() => void) | undefined; + if (scope === COMMENTS_IN_ALL) { + comments = listCommentsAcrossStories(editor); + } else if (scope && scope.storyType !== 'body') { + let storyEditor: Editor; + try { + const runtime = resolveStoryRuntime(editor, scope); + storyEditor = runtime.editor; + if (runtime.cacheable === false && typeof runtime.dispose === 'function') { + disposeTransient = runtime.dispose; + } + } catch { + return buildDiscoveryResult({ + evaluatedRevision: getRevision(editor), + total: 0, + items: [], + page: { limit: query?.limit ?? 0, offset: query?.offset ?? 0, returned: 0 }, + }); + } + try { + comments = buildCommentInfos(storyEditor, scope); + } finally { + if (disposeTransient) { + try { + disposeTransient(); + } catch { + // best-effort + } + } + } + } else { + comments = buildCommentInfos(editor); + } + const includeResolved = query?.includeResolved ?? true; const filtered = includeResolved ? comments : comments.filter((comment) => comment.status !== 'resolved'); const evaluatedRevision = getRevision(editor); diff --git a/packages/super-editor/src/ui/comments.test.ts b/packages/super-editor/src/ui/comments.test.ts index 516dfe9b3b..871323b875 100644 --- a/packages/super-editor/src/ui/comments.test.ts +++ b/packages/super-editor/src/ui/comments.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it, vi } from 'vitest'; +vi.mock('../editors/v1/document-api-adapters/story-runtime/resolve-story-runtime.js', () => ({ + resolveStoryRuntime: vi.fn(), +})); + import { createSuperDocUI } from './create-super-doc-ui.js'; +import { resolveStoryRuntime } from '../editors/v1/document-api-adapters/story-runtime/resolve-story-runtime.js'; +import type { StoryLocator } from '@superdoc/document-api'; import type { SuperDocLike } from './types.js'; /** @@ -495,3 +501,316 @@ describe('ui.comments — actions route through editor.doc.*', () => { ui.destroy(); }); }); + +describe('ui.comments cross-section routing (SD-2938)', () => { + const headerLocator: StoryLocator = { + kind: 'story', + storyType: 'headerFooterPart', + refId: 'rId6', + }; + + /** + * Stub builder that mirrors `makeStubs` but returns a host (body) + * editor and a separate header editor with its own comments + * adapter. The host's `comments.list({ in: 'all' })` returns both + * comments. The body one has no `address.story`; the header one + * has the locator stamped. That mirrors what the wrapper does in + * production. + */ + function makeHostWithHeader() { + const headerCreate = vi.fn((_input: unknown) => ({ success: true as const })); + const headerPatch = vi.fn((_input: unknown) => ({ success: true as const })); + const headerDelete = vi.fn((_input: unknown) => ({ success: true as const })); + + const bodyCreate = vi.fn((_input: unknown) => ({ success: true as const })); + const bodyPatch = vi.fn((_input: unknown) => ({ success: true as const })); + const bodyDelete = vi.fn((_input: unknown) => ({ success: true as const })); + + const bodyList = vi.fn((query?: { in?: unknown }) => { + if (query?.in === 'all') { + return { + evaluatedRevision: 'r1', + total: 2, + items: [ + { + id: 'body-1', + handle: { ref: 'comment:body-1', refStability: 'stable' as const, targetKind: 'comment' as const }, + address: { kind: 'entity' as const, entityType: 'comment' as const, entityId: 'body-1' }, + status: 'open' as const, + }, + { + id: 'header-1', + handle: { ref: 'comment:header-1', refStability: 'stable' as const, targetKind: 'comment' as const }, + address: { + kind: 'entity' as const, + entityType: 'comment' as const, + entityId: 'header-1', + story: headerLocator, + }, + status: 'open' as const, + }, + ], + page: { limit: 50, offset: 0, returned: 2 }, + }; + } + return { evaluatedRevision: 'r1', total: 0, items: [], page: { limit: 0, offset: 0, returned: 0 } }; + }); + + const navigateTo = vi.fn(async (_target: unknown) => true); + + const editorListeners = new Map void>>(); + const superdocListeners = new Map void>>(); + + const headerEditor = { + doc: { + comments: { create: headerCreate, patch: headerPatch, delete: headerDelete, list: vi.fn(() => undefined) }, + }, + }; + + const hostEditor: { + on: ReturnType; + off: ReturnType; + doc: unknown; + presentationEditor: { navigateTo: typeof navigateTo; getActiveEditor: () => unknown }; + } = { + on: vi.fn((event: string, handler: (...args: unknown[]) => void) => { + if (!editorListeners.has(event)) editorListeners.set(event, new Set()); + editorListeners.get(event)!.add(handler); + }), + off: vi.fn((event: string, handler: (...args: unknown[]) => void) => { + editorListeners.get(event)?.delete(handler); + }), + doc: { + selection: { + current: vi.fn(() => ({ empty: true, text: '', target: null, activeCommentIds: [], activeChangeIds: [] })), + }, + comments: { create: bodyCreate, patch: bodyPatch, delete: bodyDelete, list: bodyList }, + }, + presentationEditor: undefined as never, + }; + hostEditor.presentationEditor = { navigateTo, getActiveEditor: () => hostEditor }; + + const superdoc: SuperDocLike & { fireEditor(event: string, ...args: unknown[]): void } = { + activeEditor: hostEditor as never, + config: { documentMode: 'editing' }, + on: vi.fn((event: string, handler: (...args: unknown[]) => void) => { + if (!superdocListeners.has(event)) superdocListeners.set(event, new Set()); + superdocListeners.get(event)!.add(handler); + }), + off: vi.fn((event: string, handler: (...args: unknown[]) => void) => { + superdocListeners.get(event)?.delete(handler); + }), + fireEditor(event, ...args) { + const handlers = editorListeners.get(event); + if (!handlers) return; + [...handlers].forEach((handler) => handler(...args)); + }, + }; + + const commit = vi.fn(); + const commitEditor = vi.fn(); + const dispose = vi.fn(); + let runtimeCacheable = true; + + vi.mocked(resolveStoryRuntime).mockImplementation(((_host: unknown, locator?: StoryLocator) => { + if (!locator || locator.storyType === 'body') return { editor: hostEditor }; + return { + editor: headerEditor, + cacheable: runtimeCacheable, + commit, + commitEditor, + dispose, + }; + }) as never); + + return { + superdoc, + hostEditor, + headerEditor, + setRuntimeCacheable(value: boolean) { + runtimeCacheable = value; + }, + mocks: { + bodyCreate, + bodyPatch, + bodyDelete, + bodyList, + headerCreate, + headerPatch, + headerDelete, + navigateTo, + commit, + commitEditor, + dispose, + }, + }; + } + + it('snapshot includes both body and header comments via host list-all', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + const snap = ui.comments.getSnapshot(); + const ids = snap.items.map((i) => (i as { id: string }).id); + expect(ids).toEqual(['body-1', 'header-1']); + expect(mocks.bodyList).toHaveBeenCalledWith({ in: 'all' }); + + ui.destroy(); + }); + + it('resolve(headerCommentId) routes through the header editor adapter', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + const receipt = ui.comments.resolve('header-1'); + + expect(receipt.success).toBe(true); + expect(mocks.headerPatch).toHaveBeenCalledTimes(1); + expect(mocks.headerPatch).toHaveBeenCalledWith({ commentId: 'header-1', status: 'resolved' }); + expect(mocks.bodyPatch).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('delete(headerCommentId) routes through the header editor adapter', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + const receipt = ui.comments.delete('header-1'); + + expect(receipt.success).toBe(true); + expect(mocks.headerDelete).toHaveBeenCalledTimes(1); + expect(mocks.headerDelete).toHaveBeenCalledWith({ commentId: 'header-1' }); + expect(mocks.bodyDelete).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('reply(headerCommentId) routes through the header editor adapter', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + const receipt = ui.comments.reply('header-1', { text: 'thanks' }); + + expect(receipt.success).toBe(true); + expect(mocks.headerCreate).toHaveBeenCalledTimes(1); + expect(mocks.headerCreate).toHaveBeenCalledWith({ parentCommentId: 'header-1', text: 'thanks' }); + expect(mocks.bodyCreate).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('resolve(bodyCommentId) stays on the body editor adapter', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + ui.comments.resolve('body-1'); + + expect(mocks.bodyPatch).toHaveBeenCalledTimes(1); + expect(mocks.headerPatch).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('scrollTo(headerCommentId) passes a story-aware target to navigateTo', async () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + await ui.comments.scrollTo('header-1'); + + expect(mocks.navigateTo).toHaveBeenCalledTimes(1); + const target = mocks.navigateTo.mock.calls[0][0] as { + kind: string; + entityType: string; + entityId: string; + story?: StoryLocator; + }; + expect(target).toEqual({ kind: 'entity', entityType: 'comment', entityId: 'header-1', story: headerLocator }); + + ui.destroy(); + }); + + it('scrollTo(bodyCommentId) does not include story (backward compatible)', async () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + await ui.comments.scrollTo('body-1'); + + expect(mocks.navigateTo).toHaveBeenCalledTimes(1); + const target = mocks.navigateTo.mock.calls[0][0] as Record; + expect(target).toEqual({ kind: 'entity', entityType: 'comment', entityId: 'body-1' }); + expect(target.story).toBeUndefined(); + + ui.destroy(); + }); + + it('resolve(headerCommentId) commits the story editor back to the host part', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + ui.comments.resolve('header-1'); + + // commitEditor takes precedence over commit when present. + expect(mocks.commitEditor).toHaveBeenCalledTimes(1); + expect(mocks.commit).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('delete(headerCommentId) commits the story editor back to the host part', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + ui.comments.delete('header-1'); + + expect(mocks.commitEditor).toHaveBeenCalledTimes(1); + + ui.destroy(); + }); + + it('reply(headerCommentId) commits the story editor back to the host part', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + ui.comments.reply('header-1', { text: 'thanks' }); + + expect(mocks.commitEditor).toHaveBeenCalledTimes(1); + + ui.destroy(); + }); + + it('resolve(bodyCommentId) does not call commit (body persists directly)', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + ui.comments.resolve('body-1'); + + expect(mocks.commit).not.toHaveBeenCalled(); + expect(mocks.commitEditor).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('disposes a non-cacheable runtime after resolving a header comment', () => { + const stubs = makeHostWithHeader(); + stubs.setRuntimeCacheable(false); + const ui = createSuperDocUI({ superdoc: stubs.superdoc }); + + ui.comments.resolve('header-1'); + + expect(stubs.mocks.dispose).toHaveBeenCalledTimes(1); + + ui.destroy(); + }); + + it('does not dispose a cacheable runtime after resolving a header comment', () => { + const { superdoc, mocks } = makeHostWithHeader(); + const ui = createSuperDocUI({ superdoc }); + + ui.comments.resolve('header-1'); + + expect(mocks.dispose).not.toHaveBeenCalled(); + + ui.destroy(); + }); +}); diff --git a/packages/super-editor/src/ui/create-super-doc-ui.ts b/packages/super-editor/src/ui/create-super-doc-ui.ts index e454bcdcd6..8f57f809c2 100644 --- a/packages/super-editor/src/ui/create-super-doc-ui.ts +++ b/packages/super-editor/src/ui/create-super-doc-ui.ts @@ -12,9 +12,12 @@ import type { Receipt, ScrollIntoViewInput, ScrollIntoViewOutput, + StoryLocator, TrackChangesListResult, } from '@superdoc/document-api'; +import { COMMENTS_IN_ALL } from '@superdoc/document-api'; import { collectEntityHitsFromChain } from './entity-at.js'; +import { resolveStoryRuntime } from '../editors/v1/document-api-adapters/story-runtime/resolve-story-runtime.js'; import { shallowEqual } from './equality.js'; import { scrollRangeIntoView } from './scroll-into-view.js'; import { getSelectionAnchorRect, getSelectionRects } from './selection-rects.js'; @@ -371,14 +374,20 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { }; let commentsListCache: CommentsListResult = EMPTY_COMMENTS_LIST; const refreshCommentsListCache = () => { - const editor = resolveRoutedEditor(superdoc); + // Always go through the host editor with `in: 'all'` so the + // snapshot includes comments anchored in headers, footers, + // footnotes, and endnotes, not just whichever section currently + // has focus. The routed editor's snapshot would collapse to one + // section and drop the rest as the user moves focus, which the + // SD-2938 reproduction surfaced. + const editor = resolveHostEditor(superdoc); const list = editor?.doc?.comments?.list; if (typeof list !== 'function') { commentsListCache = EMPTY_COMMENTS_LIST; return; } try { - const result = list.call(editor.doc!.comments, undefined) as CommentsListResult | undefined; + const result = list.call(editor.doc!.comments, { in: COMMENTS_IN_ALL }) as CommentsListResult | undefined; commentsListCache = result ?? EMPTY_COMMENTS_LIST; } catch { // Reset to empty rather than retaining the previous editor's @@ -1209,6 +1218,83 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { return api; }; + /** + * Look up a comment in the current snapshot and return its + * `address.story` if any. The snapshot is sourced from the host + * editor with `{ in: 'all' }`, so the returned locator (if + * non-body) is what the action needs to route by. + */ + const findCommentStory = (commentId: string): StoryLocator | undefined => { + for (const item of commentsListCache.items) { + if (item.id === commentId || item.importedId === commentId) { + return item.address?.story; + } + } + return undefined; + }; + + /** + * Resolve the comments adapter that owns the given comment, plus a + * `finalize()` tail that the caller invokes after the mutation has + * landed. For body comments the tail is a no-op. For non-body + * comments the tail commits the story editor's state back to the + * canonical OOXML part (otherwise the mutation lives only in the + * temporary story editor and is undone by the next refresh) and + * disposes any non-cacheable runtime so headless story editors do + * not leak. + * + * Falls back to the routed editor's adapter when the comment isn't + * in the snapshot yet (e.g. a just-created reply whose refresh + * hasn't landed) or when the story can't be resolved. + */ + const resolveCommentRoute = ( + commentId: string, + ): { api: NonNullable['comments']; finalize: () => void } => { + const noop = () => undefined; + const story = findCommentStory(commentId); + if (!story || story.storyType === 'body') { + const host = resolveHostEditor(superdoc); + const api = host?.doc?.comments; + if (api) return { api, finalize: noop }; + return { api: requireDocComments(), finalize: noop }; + } + const host = resolveHostEditor(superdoc); + if (host) { + try { + const runtime = resolveStoryRuntime(host as never, story); + const api = (runtime.editor as unknown as SuperDocEditorLike)?.doc?.comments; + if (api) { + const finalize = () => { + try { + if (typeof runtime.commitEditor === 'function') { + runtime.commitEditor(host as never, runtime.editor); + } else if (typeof runtime.commit === 'function') { + runtime.commit(host as never); + } + } catch { + // Persistence failure is surfaced via the receipt the + // action returned; swallow here so the controller does + // not throw out of a UI handler. + } + if (runtime.cacheable === false && typeof runtime.dispose === 'function') { + try { + runtime.dispose(); + } catch { + // best-effort + } + } + }; + return { api, finalize }; + } + } catch { + // Fall through to routed-editor adapter so the user sees a + // structured failure receipt rather than a thrown error when a + // story silently disappears between snapshot and action. + } + } + return { api: requireDocComments(), finalize: noop }; + }; + /** * Run `scrollRangeIntoView` against the host editor — the * presentation editor lives at the host level and its @@ -1286,52 +1372,57 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { failure: { code: 'NO_OP', message: 'ui.comments.reply: text is empty.' }, }; } - const api = requireDocComments(); + const { api, finalize } = resolveCommentRoute(parentCommentId); const receipt = (api.create as (input: unknown, options?: unknown) => Receipt).call(api, { parentCommentId, text, }); + finalize(); refreshAndNotify(); return receipt; }, resolve(commentId) { - const api = requireDocComments(); + const { api, finalize } = resolveCommentRoute(commentId); const receipt = (api.patch as (input: unknown, options?: unknown) => Receipt).call(api, { commentId, status: 'resolved', }); + finalize(); refreshAndNotify(); return receipt; }, reopen(commentId) { // Routes through `comments.patch({ status: 'active' })`. Today - // doc-api validation rejects anything other than 'resolved' — + // doc-api validation rejects anything other than 'resolved'. // SD-2789 widens the union and ships the lifecycle inverse. // Until then this surfaces an INVALID_INPUT receipt or throws, // which is the correct visible behavior for a not-yet-shipped // operation rather than a silent no-op. - const api = requireDocComments(); + const { api, finalize } = resolveCommentRoute(commentId); const receipt = (api.patch as (input: unknown, options?: unknown) => Receipt).call(api, { commentId, status: 'active', }); + finalize(); refreshAndNotify(); return receipt; }, delete(commentId) { - const api = requireDocComments(); + const { api, finalize } = resolveCommentRoute(commentId); const receipt = (api.delete as (input: unknown, options?: unknown) => Receipt).call(api, { commentId }); + finalize(); refreshAndNotify(); return receipt; }, async scrollTo(commentId) { - // `CommentAddress` is body-scoped in the contract — it has no - // `story` field today. Story-aware comment navigation lands as - // a separate doc-API extension; until then, just route the id - // and let `presentation.navigateTo` resolve through the comment - // entity store. + // Pass `address.story` from the cached snapshot so + // `presentation.navigateTo` activates the right story (header, + // footer, footnote, endnote) before scrolling. For body + // comments the story is omitted, which leaves navigation + // body-scoped exactly like before. + const story = findCommentStory(commentId); return runScrollIntoView({ - target: { kind: 'entity', entityType: 'comment', entityId: commentId }, + target: { kind: 'entity', entityType: 'comment', entityId: commentId, ...(story ? { story } : {}) }, block: 'center', behavior: 'smooth', });