From 48d983d38c3d1301ab9993eb88a8db584d913ef2 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Tue, 5 May 2026 14:47:16 -0300 Subject: [PATCH 1/2] feat(ui): right-click context bundle (SD-2945) Adds ui.viewport.contextAt({ x, y }) returning the full { point, entities, position, selection, insideSelection } bundle every right-click consumer ends up assembling by hand. insideSelection is an AABB hit-test against the current selection rects so predicates can distinguish "right-clicked the selection" from "right-clicked elsewhere" without re-running geometry. Widens ContextMenuWhenInput with optional point / position / insideSelection (additive, old { entities, selection } predicates keep working). getContextMenuItems(input) now accepts a ViewportContext OR the legacy { entities } shape; bundle inputs make the same context available on ContextMenuItem.invoke() and pass it into the registered execute({ context }) so handlers act on the click target without threading payloads. Direct ui.commands.execute / commands.get(id).execute calls leave context undefined. Tests cover the AABB helper boundaries, contextAt composition, predicate input shape for both call styles, invoke() forwarding to execute, and the legacy path keeping invoke absent. --- .../src/ui/create-super-doc-ui.ts | 42 ++++- .../src/ui/custom-commands.test.ts | 144 ++++++++++++++++++ .../super-editor/src/ui/custom-commands.ts | 60 +++++++- packages/super-editor/src/ui/index.ts | 2 + packages/super-editor/src/ui/types.ts | 132 +++++++++++++++- .../src/ui/viewport-context.test.ts | 103 +++++++++++++ .../super-editor/src/ui/viewport-context.ts | 54 +++++++ packages/super-editor/src/ui/viewport.test.ts | 35 +++++ packages/superdoc/src/ui.d.ts | 2 + 9 files changed, 560 insertions(+), 14 deletions(-) create mode 100644 packages/super-editor/src/ui/viewport-context.test.ts create mode 100644 packages/super-editor/src/ui/viewport-context.ts 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 682f230c17..79db1e9094 100644 --- a/packages/super-editor/src/ui/create-super-doc-ui.ts +++ b/packages/super-editor/src/ui/create-super-doc-ui.ts @@ -17,6 +17,7 @@ import type { import { collectEntityHitsFromChain } from './entity-at.js'; import { shallowEqual } from './equality.js'; import { resolvePositionAt } from './position-at.js'; +import { buildViewportContext } from './viewport-context.js'; import { shortcutFromEvent } from './keyboard-shortcuts.js'; import { scrollRangeIntoView } from './scroll-into-view.js'; import { getSelectionAnchorRect, getSelectionRects } from './selection-rects.js'; @@ -49,6 +50,8 @@ import type { ToolbarHandle, ToolbarSnapshotSlice, UIToolbarCommandState, + ViewportContext, + ViewportContextAtInput, ViewportEntityAtInput, ViewportEntityHit, ViewportGetRectInput, @@ -1295,9 +1298,24 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { // contributed items here. Computed against the current snapshot // (so `selection` matches what observers just saw) and the // caller-supplied entities from `ui.viewport.entityAt`. + // + // SD-2945: input can also be the full {@link ViewportContext} + // bundle from `ui.viewport.contextAt({ x, y })`. Detected by the + // presence of `point`. The legacy `{ entities }` shape doesn't + // carry one. When a bundle is passed, predicates receive the + // full shape (point / position / insideSelection) and each + // returned item gets an `invoke()` closure that fires execute + // with `context` bound. if (prop === 'getContextMenuItems') { - return (input?: { entities?: ViewportEntityHit[] }): ContextMenuItem[] => { - return customCommandsRegistry.getContextMenuItems(computeState(), input?.entities ?? []); + return (input?: { entities?: ViewportEntityHit[] } | ViewportContext): ContextMenuItem[] => { + const isBundle = !!input && typeof (input as ViewportContext).point === 'object'; + if (isBundle) { + return customCommandsRegistry.getContextMenuItems(computeState(), input as ViewportContext); + } + return customCommandsRegistry.getContextMenuItems( + computeState(), + (input as { entities?: ViewportEntityHit[] } | undefined)?.entities ?? [], + ); }; } // Custom-registered ids surface a typed handle from the registry. @@ -1744,6 +1762,26 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { input.y, ); }, + + contextAt(input: ViewportContextAtInput): ViewportContext { + // Coerce non-numeric coords to 0 so the bundle is still + // well-formed (entities = [], position = null, + // insideSelection = false). Consumers can ignore `point` / + // `position` themselves; returning a partial bundle would + // force every consumer to null-check. + const x = typeof input?.x === 'number' ? input.x : 0; + const y = typeof input?.y === 'number' ? input.y : 0; + const hostEditor = resolveHostEditor(superdoc); + const routedEditor = resolveRoutedEditor(superdoc); + const entities = viewport.entityAt({ x, y }); + const position = viewport.positionAt({ x, y }); + const selectionSlice = computeState().selection; + const selectionRects = getSelectionRects( + hostEditor as unknown as Parameters[0], + routedEditor as unknown as Parameters[1], + ); + return buildViewportContext({ x, y, entities, position, selection: selectionSlice, selectionRects }); + }, }; // ---- ui.selection ------------------------------------------------------ diff --git a/packages/super-editor/src/ui/custom-commands.test.ts b/packages/super-editor/src/ui/custom-commands.test.ts index f65caa39aa..1a647acdc8 100644 --- a/packages/super-editor/src/ui/custom-commands.test.ts +++ b/packages/super-editor/src/ui/custom-commands.test.ts @@ -1147,6 +1147,150 @@ describe('ui.commands.getContextMenuItems', () => { }); }); +// SD-2945: getContextMenuItems accepts the full ViewportContext +// bundle from `viewport.contextAt(...)`. When passed a bundle: +// - the `when` predicate sees `point` / `position` / `insideSelection` +// in addition to `entities` and `selection` +// - each returned item carries an `invoke()` closure that fires +// `execute` with the bundle bound, so handlers can read `context` +// without re-running geometry +// The legacy `{ entities }` shape keeps working, with `invoke` +// absent on returned items. +describe('ui.commands.getContextMenuItems - ViewportContext bundle', () => { + function makeBundle( + overrides: Partial<{ x: number; y: number; insideSelection: boolean; entities: unknown[]; position: unknown }> = {}, + ) { + return { + point: { x: overrides.x ?? 100, y: overrides.y ?? 200 }, + entities: (overrides.entities ?? []) as never[], + position: (overrides.position ?? null) as never, + selection: { + empty: true, + target: null, + selectionTarget: null, + activeMarks: [], + activeCommentIds: [], + activeChangeIds: [], + quotedText: '', + }, + insideSelection: overrides.insideSelection ?? false, + }; + } + + it('passes point / position / insideSelection to the when predicate when called with a bundle', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + const whenSpy = vi.fn(() => true); + + ui.commands.register({ + id: 'test.bundle.when', + execute: () => true, + contextMenu: { label: 'Bundle', when: whenSpy }, + }); + + const bundle = makeBundle({ x: 50, y: 60, insideSelection: true }); + ui.commands.getContextMenuItems(bundle); + + expect(whenSpy).toHaveBeenCalledTimes(1); + expect(whenSpy.mock.calls[0]![0]).toMatchObject({ + entities: [], + point: { x: 50, y: 60 }, + insideSelection: true, + }); + expect((whenSpy.mock.calls[0]![0] as { selection: unknown }).selection).toBeDefined(); + + ui.destroy(); + }); + + it('omits point / position / insideSelection from the when input when called with the legacy { entities } shape', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + const whenSpy = vi.fn(() => true); + + ui.commands.register({ + id: 'test.legacy.when', + execute: () => true, + contextMenu: { label: 'Legacy', when: whenSpy }, + }); + + ui.commands.getContextMenuItems({ entities: [{ type: 'comment', id: 'c1' }] }); + + expect(whenSpy).toHaveBeenCalledTimes(1); + const arg = whenSpy.mock.calls[0]![0] as Record; + expect(arg.entities).toEqual([{ type: 'comment', id: 'c1' }]); + expect(arg.selection).toBeDefined(); + // No bundle fields when the consumer didn't pass one. Handlers + // that only destructure { entities, selection } see the same + // shape they always have. + expect(arg.point).toBeUndefined(); + expect(arg.position).toBeUndefined(); + expect(arg.insideSelection).toBeUndefined(); + + ui.destroy(); + }); + + it('returns items with an invoke() closure that fires execute with the bundle bound to context', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + const executeSpy = vi.fn(() => true); + + ui.commands.register({ + id: 'test.bundle.execute', + execute: executeSpy, + contextMenu: { label: 'Bundle execute' }, + }); + + const bundle = makeBundle({ x: 10, y: 20, insideSelection: false }); + const items = ui.commands.getContextMenuItems(bundle); + expect(items).toHaveLength(1); + expect(typeof items[0]!.invoke).toBe('function'); + + items[0]!.invoke!(); + expect(executeSpy).toHaveBeenCalledTimes(1); + const args = executeSpy.mock.calls[0]![0] as Record; + expect((args.context as { point: unknown }).point).toEqual({ x: 10, y: 20 }); + expect((args.context as { insideSelection: unknown }).insideSelection).toBe(false); + + ui.destroy(); + }); + + it('omits invoke() from returned items when called with the legacy { entities } shape', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + ui.commands.register({ + id: 'test.legacy.no-invoke', + execute: () => true, + contextMenu: { label: 'Legacy' }, + }); + + const items = ui.commands.getContextMenuItems({ entities: [] }); + expect(items).toHaveLength(1); + expect(items[0]!.invoke).toBeUndefined(); + + ui.destroy(); + }); + + it('does not pass context when the command is invoked through `commands.get(id).execute()` directly', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + const executeSpy = vi.fn(() => true); + + ui.commands.register({ + id: 'test.direct.execute', + execute: executeSpy, + contextMenu: { label: 'Direct' }, + }); + + ui.commands.get('test.direct.execute').execute(); + expect(executeSpy).toHaveBeenCalledTimes(1); + const args = executeSpy.mock.calls[0]![0] as Record; + expect(args.context).toBeUndefined(); + + ui.destroy(); + }); +}); + describe('ui.commands.register — shortcut field', () => { function makeStubsWithHost() { const stubs = makeStubs(); diff --git a/packages/super-editor/src/ui/custom-commands.ts b/packages/super-editor/src/ui/custom-commands.ts index e3d247bc94..65afb336b7 100644 --- a/packages/super-editor/src/ui/custom-commands.ts +++ b/packages/super-editor/src/ui/custom-commands.ts @@ -11,6 +11,7 @@ import type { SuperDocUIState, Subscribable, UIToolbarCommandState, + ViewportContext, ViewportEntityHit, } from './types.js'; @@ -129,8 +130,13 @@ export interface CustomCommandsRegistry { */ getHandle(id: string): CustomCommandHandle | undefined; - /** Run `execute` for a registered id. Returns false if not registered. */ - execute(id: string, payload?: unknown): boolean | Promise; + /** + * Run `execute` for a registered id. Returns false if not registered. + * `context` (SD-2945) forwards the {@link ViewportContext} bundle when + * the dispatch came from a `ContextMenuItem.invoke()`; direct + * controller calls leave it `undefined`. + */ + execute(id: string, payload?: unknown, context?: ViewportContext): boolean | Promise; /** * Collect context-menu items contributed by registered customs. @@ -138,8 +144,14 @@ export interface CustomCommandsRegistry { * supplied entities + the current selection slice; sorted by * `(group, order, registrationSeq)`. Errors from `when` are * caught and the item is hidden for that query. + * + * SD-2945: when `input` is the full {@link ViewportContext} bundle, + * predicates receive `point` / `position` / `insideSelection` and + * each returned item carries an `invoke()` closure that fires + * execute with the bundle bound. Pass an entities array for the + * legacy "entities only" call shape. */ - getContextMenuItems(state: SuperDocUIState, entities: ViewportEntityHit[]): ContextMenuItem[]; + getContextMenuItems(state: SuperDocUIState, input: ViewportEntityHit[] | ViewportContext): ContextMenuItem[]; /** * Look up the custom command id (if any) bound to a normalized @@ -498,7 +510,7 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): getHandle, - execute(id, payload) { + execute(id, payload, context) { const entry = entries.get(id); if (!entry) return false; try { @@ -506,16 +518,23 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): // `register(...)` signature carries the consumer's // payload type to the captured handle, but the runtime registry // stores entries with the default `void` payload. Cast to bridge. + // `context` (SD-2945) is forwarded only when the dispatch came + // from a `ContextMenuItem.invoke()`; direct + // `ui.commands.execute` and `commands.get(id).execute()` calls + // pass it through as `undefined`, leaving the prior payload + // shape untouched for handlers that don't care about clicks. const result = ( entry.execute as (args: { payload?: unknown; superdoc: SuperDocLike; editor: SuperDocEditorLike | null; + context?: ViewportContext; }) => unknown )({ payload, superdoc: deps.superdoc, editor: deps.getEditor(), + context, }); if (result instanceof Promise) { return result.then( @@ -533,7 +552,17 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): } }, - getContextMenuItems(state, entities) { + // SD-2945: input is either the legacy `entities` array (consumer + // built the menu via `viewport.entityAt(...)` only) or the full + // {@link ViewportContext} bundle from `viewport.contextAt(...)`. + // Bundle inputs surface `point` / `position` / `insideSelection` + // on the `when` predicate AND wire `invoke()` on each returned + // item so consumers can fire execute with context bound. + getContextMenuItems(state, input) { + const isBundle = !Array.isArray(input); + const entities: ViewportEntityHit[] = isBundle ? (input as ViewportContext).entities : (input ?? []); + const context: ViewportContext | null = isBundle ? (input as ViewportContext) : null; + const items: ContextMenuItem[] = []; for (const entry of entries.values()) { const contribution = entry.contextMenu; @@ -542,7 +571,16 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): if (contribution.when) { let applies = true; try { - applies = contribution.when({ entities, selection: state.selection }) === true; + const whenInput = context + ? { + entities, + selection: state.selection, + point: context.point, + position: context.position, + insideSelection: context.insideSelection, + } + : { entities, selection: state.selection }; + applies = contribution.when(whenInput) === true; } catch (err) { // Same dedupe posture as `getState` errors: log once per // distinct message so a buggy `when` predicate doesn't @@ -559,11 +597,21 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): entry.lastContextMenuErrorMessage = null; } + // SD-2945: when called with a {@link ViewportContext} bundle, + // attach an `invoke()` closure that fires `execute` with + // `context` bound. Captures the captured `id` and the bundle + // by reference; consumers that mutate the bundle after + // calling `getContextMenuItems` would change what the + // closure sees, but the bundle is shaped as immutable data + // for this use case (contextAt builds it fresh per call). + const itemId = entry.id; + const invoke = context ? () => registry.execute(itemId, undefined, context) : undefined; items.push({ id: entry.id, label: contribution.label, group: contribution.group ?? DEFAULT_CONTEXT_MENU_GROUP, order: contribution.order ?? 0, + ...(invoke ? { invoke } : {}), }); } diff --git a/packages/super-editor/src/ui/index.ts b/packages/super-editor/src/ui/index.ts index e9a34bc72f..96f1d8c8a5 100644 --- a/packages/super-editor/src/ui/index.ts +++ b/packages/super-editor/src/ui/index.ts @@ -124,6 +124,8 @@ export type { TrackChangesSlice, // Viewport + ViewportContext, + ViewportContextAtInput, ViewportEntityAtInput, ViewportEntityHit, ViewportGetRectInput, diff --git a/packages/super-editor/src/ui/types.ts b/packages/super-editor/src/ui/types.ts index 5e04f9de85..101f3dccf2 100644 --- a/packages/super-editor/src/ui/types.ts +++ b/packages/super-editor/src/ui/types.ts @@ -1075,9 +1075,13 @@ export type CommandsHandle = { * ```ts * scope.on(editorHost, 'contextmenu', (event) => { * event.preventDefault(); - * const entities = ui.viewport.entityAt({ x: event.clientX, y: event.clientY }); - * const items = ui.commands.getContextMenuItems({ entities }); - * renderMenu(items, event.clientX, event.clientY); + * // SD-2945: pass the full bundle so predicates filter on the + * // same shape handlers receive, and `item.invoke()` fires + * // execute with context bound. The legacy `{ entities }` shape + * // still works for apps that haven't migrated. + * const context = ui.viewport.contextAt({ x: event.clientX, y: event.clientY }); + * const items = ui.commands.getContextMenuItems(context); + * renderMenu(items, event.clientX, event.clientY, (item) => item.invoke?.()); * }); * ``` * @@ -1092,7 +1096,7 @@ export type CommandsHandle = { * extension (`disableContextMenu: true`) and roll their own menu — * built-in entries belong to the consumer's renderer at that point. */ - getContextMenuItems(input?: { entities?: ViewportEntityHit[] }): ContextMenuItem[]; + getContextMenuItems(input?: { entities?: ViewportEntityHit[] } | ViewportContext): ContextMenuItem[]; }; /** @@ -1163,11 +1167,18 @@ export type CustomCommandRegistration = { * Execute the command. Receives: * * - `payload` (typed per registration), - * - the host `superdoc` instance, and + * - the host `superdoc` instance, * - the routed `editor` — the same editor `ui.commands.*` mutations * target. Use `editor.doc.*` for direct Document API access without * reaching `superdoc.activeEditor`. `editor` is `null` before the - * editor has reported ready, so guard early. + * editor has reported ready, so guard early, and + * - `context` (SD-2945): the {@link ViewportContext} bundle, present + * only when the command was invoked via `ContextMenuItem.invoke()` + * from a menu opened with `ui.viewport.contextAt(...)`. Lets + * right-click handlers act on the click target ("Paste here", + * "Comment here") without re-running entityAt / positionAt or + * threading payloads. `undefined` for direct + * `commands.execute` / `commands.get(id).execute()` calls. * * Return value is normalized to `boolean` for the synchronous result; * async commands return a Promise the runtime awaits internally. @@ -1176,6 +1187,7 @@ export type CustomCommandRegistration = { payload?: TPayload; superdoc: SuperDocLike; editor: SuperDocEditorLike | null; + context?: ViewportContext; }) => boolean | void | Promise; /** * Optional state deriver. Runs on every snapshot rebuild. If omitted, @@ -1281,6 +1293,28 @@ export interface ContextMenuWhenInput { entities: ViewportEntityHit[]; /** Current selection slice. Mirrors `state.selection`. */ selection: SelectionSlice; + /** + * SD-2945: viewport-relative click point. Present only when the + * consumer called `getContextMenuItems(viewport.contextAt({ x, y }))` + * (or passed a {@link ViewportContext} directly). Predicates that + * only care about entities / selection can keep destructuring the + * old two fields; the new ones are additive. + */ + point?: { x: number; y: number }; + /** + * Resolved caret position at the click point, or `null` when the + * click is outside the painted host. Present only when the consumer + * passed a {@link ViewportContext}. + */ + position?: ViewportPositionHit | null; + /** + * `true` when the click point is inside the currently painted + * selection rects. Lets predicates distinguish "right-clicked the + * selection" from "right-clicked elsewhere" without re-running + * geometry. Present only when the consumer passed a + * {@link ViewportContext}. + */ + insideSelection?: boolean; } /** @@ -1296,6 +1330,19 @@ export interface ContextMenuItem { label: string; group: string; order: number; + /** + * SD-2945: convenience invoker that fires the registered command's + * `execute` with the {@link ViewportContext} bundle bound. Present + * only when the items came from + * `getContextMenuItems(viewport.contextAt(...))`. The bundle is + * captured in the closure so the handler receives the same shape + * the predicate filtered on, without the consumer re-threading a + * payload at every dispatch site. + * + * Consumers can still call `ui.commands.get(item.id).execute()` + * directly when they don't need context (no behavior change). + */ + invoke?(): boolean | Promise; } /** Return value from {@link CommandsHandle.register}. */ @@ -1633,6 +1680,36 @@ export interface ViewportHandle { * about to consume them. */ positionAt(input: ViewportPositionAtInput): ViewportPositionHit | null; + + /** + * Resolve a viewport `(x, y)` coordinate to the full right-click + * context bundle: `entities` under the point, the resolved + * `position`, the live `selection`, the `point` itself, and + * `insideSelection` (whether the click landed inside the painted + * selection rects). + * + * Composes `entityAt`, `positionAt`, the `selection` slice, and an + * AABB hit-test against `selection.getRects()` so consumers building + * right-click menus don't reassemble the same shape at every site. + * Pass the returned bundle to `getContextMenuItems(context)` so + * predicates filter on the same shape handlers receive, and to + * `ContextMenuItem.invoke()` so `execute({ context })` can act on + * the click target without re-running geometry. + * + * Always returns a bundle (no `null`). Every field has a + * deterministic default for the "outside the host" case so consumer + * code can destructure without null checks. + */ + contextAt(input: ViewportContextAtInput): ViewportContext; +} + +/** + * Input shape for {@link ViewportHandle.contextAt}. Same coordinate + * space as `MouseEvent.clientX` / `clientY`. + */ +export interface ViewportContextAtInput { + x: number; + y: number; } /** @@ -1662,6 +1739,49 @@ export interface ViewportPositionHit { target: import('@superdoc/document-api').SelectionTarget; } +/** + * The "what did the user right-click on?" bundle returned by + * {@link ViewportHandle.contextAt}. Composes `entityAt`, `positionAt`, + * the live selection slice, and an AABB hit-test against the current + * selection rects so consumers don't reassemble the same shape at + * every register site. + * + * Threaded into both `ContextMenuContribution.when` (so predicates can + * filter on entity / position / selection containment) and the + * registered `execute` (via {@link ContextMenuItem.invoke}) so the + * handler doesn't redo work the controller already did. + */ +export interface ViewportContext { + /** + * The viewport-relative coordinate the consumer asked about. + * Echoed back so handlers that anchor floating UI to the click + * point don't have to remember it separately. + */ + point: { x: number; y: number }; + /** + * Entities under the click point, ordered innermost-first. Same + * shape and ordering {@link ViewportHandle.entityAt} returns + * directly. Empty when the click is over no painted entity. + */ + entities: ViewportEntityHit[]; + /** + * Resolved caret position at the click point, or `null` when the + * point is outside the painted host or no editor is mounted. Same + * shape {@link ViewportHandle.positionAt} returns. + */ + position: ViewportPositionHit | null; + /** The live selection slice. Mirrors `state.selection`. */ + selection: SelectionSlice; + /** + * `true` when the click point is inside any of the rects the live + * selection currently paints. Distinguishes "right-clicked the + * selection itself" (act on the selection) from "right-clicked + * elsewhere" (act on the click target). Always `false` for an + * empty / collapsed selection. + */ + insideSelection: boolean; +} + /** * Input shape for {@link ViewportHandle.entityAt}. Coordinates are * viewport-relative (the same space `MouseEvent.clientX` / diff --git a/packages/super-editor/src/ui/viewport-context.test.ts b/packages/super-editor/src/ui/viewport-context.test.ts new file mode 100644 index 0000000000..95361e4827 --- /dev/null +++ b/packages/super-editor/src/ui/viewport-context.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest'; + +import { buildViewportContext, pointInsideRects } from './viewport-context.js'; +import type { SelectionSlice, ViewportRect } from './types.js'; + +const rect = (top: number, left: number, width: number, height: number, pageIndex = 0): ViewportRect => ({ + top, + left, + width, + height, + pageIndex, +}); + +const emptySelection: SelectionSlice = { + empty: true, + target: null, + selectionTarget: null, + activeMarks: [], + activeCommentIds: [], + activeChangeIds: [], + quotedText: '', +}; + +describe('pointInsideRects', () => { + it('returns false for an empty rects array (no live selection)', () => { + expect(pointInsideRects(50, 50, [])).toBe(false); + }); + + it('returns true when the point lands strictly inside a rect', () => { + expect(pointInsideRects(50, 50, [rect(40, 40, 100, 20)])).toBe(true); + }); + + it('treats rect edges as inside (inclusive bounds, no flicker on selection borders)', () => { + const r = rect(40, 40, 100, 20); + expect(pointInsideRects(40, 40, [r])).toBe(true); // top-left corner + expect(pointInsideRects(140, 60, [r])).toBe(true); // bottom-right corner + }); + + it('returns false for points outside every rect', () => { + expect(pointInsideRects(0, 0, [rect(40, 40, 100, 20)])).toBe(false); + expect(pointInsideRects(200, 200, [rect(40, 40, 100, 20)])).toBe(false); + }); + + it('returns true when the point is inside any one of multiple rects (multi-line selection)', () => { + // First rect spans x:[40,140], y:[40,60]; second rect spans x:[40,120], y:[64,84]. + // The gap row at y=62 sits between both — outside both rects. + const rects = [rect(40, 40, 100, 20), rect(64, 40, 80, 20)]; + expect(pointInsideRects(50, 70, rects)).toBe(true); // inside second rect + expect(pointInsideRects(50, 62, rects)).toBe(false); // gap between rows + }); +}); + +describe('buildViewportContext', () => { + it('echoes the click point and composes the supplied primitives verbatim', () => { + const entities = [{ type: 'comment', id: 'c1' } as const]; + const position = { + point: { kind: 'text', blockId: 'p1', offset: 3 } as const, + target: { + kind: 'selection', + start: { kind: 'text', blockId: 'p1', offset: 3 }, + end: { kind: 'text', blockId: 'p1', offset: 3 }, + } as const, + }; + const ctx = buildViewportContext({ + x: 100, + y: 200, + entities, + position, + selection: emptySelection, + selectionRects: [], + }); + + expect(ctx.point).toEqual({ x: 100, y: 200 }); + expect(ctx.entities).toBe(entities); + expect(ctx.position).toBe(position); + expect(ctx.selection).toBe(emptySelection); + expect(ctx.insideSelection).toBe(false); + }); + + it('reports insideSelection when the click is inside any selection rect', () => { + const ctx = buildViewportContext({ + x: 60, + y: 50, + entities: [], + position: null, + selection: emptySelection, + selectionRects: [rect(40, 40, 100, 20)], + }); + expect(ctx.insideSelection).toBe(true); + }); + + it('reports insideSelection=false when rects exist but the click is outside them', () => { + const ctx = buildViewportContext({ + x: 0, + y: 0, + entities: [], + position: null, + selection: emptySelection, + selectionRects: [rect(40, 40, 100, 20)], + }); + expect(ctx.insideSelection).toBe(false); + }); +}); diff --git a/packages/super-editor/src/ui/viewport-context.ts b/packages/super-editor/src/ui/viewport-context.ts new file mode 100644 index 0000000000..71f830529d --- /dev/null +++ b/packages/super-editor/src/ui/viewport-context.ts @@ -0,0 +1,54 @@ +/** + * `ui.viewport.contextAt({ x, y })` helper. Composes the existing + * primitives (entityAt, positionAt, the live selection slice, plus an + * AABB hit-test against the current selection rects) into a single + * bundle so right-click consumers don't have to stitch them by hand. + * + * The bundle is computed once when the menu opens, fed to predicates + * via `ContextMenuContribution.when`, and threaded into the registered + * `execute` when the user picks an item via `ContextMenuItem.invoke()`. + */ + +import type { ViewportContext, ViewportEntityHit, ViewportPositionHit, ViewportRect, SelectionSlice } from './types.js'; + +/** + * Returns true when `(x, y)` lies inside any of the provided + * viewport-relative rects. Uses inclusive bounds so a click on a + * rect's edge still counts as "inside" — consumers rendering selection + * highlights expect right-clicks on the highlight border to be treated + * as inside the selection. + * + * Empty rects array (no live selection / collapsed caret) returns + * false, which is what `insideSelection` semantically means. + */ +export function pointInsideRects(x: number, y: number, rects: ReadonlyArray): boolean { + for (const rect of rects) { + if (x >= rect.left && x <= rect.left + rect.width && y >= rect.top && y <= rect.top + rect.height) { + return true; + } + } + return false; +} + +/** + * Build the {@link ViewportContext} bundle from already-resolved + * primitives. The controller calls each primitive itself (entityAt, + * positionAt, ui.selection.getRects, current selection slice) and + * passes the results in here so this helper stays pure. + */ +export function buildViewportContext(args: { + x: number; + y: number; + entities: ViewportEntityHit[]; + position: ViewportPositionHit | null; + selection: SelectionSlice; + selectionRects: ReadonlyArray; +}): ViewportContext { + return { + point: { x: args.x, y: args.y }, + entities: args.entities, + position: args.position, + selection: args.selection, + insideSelection: pointInsideRects(args.x, args.y, args.selectionRects), + }; +} diff --git a/packages/super-editor/src/ui/viewport.test.ts b/packages/super-editor/src/ui/viewport.test.ts index 63ba51e601..e3706880c3 100644 --- a/packages/super-editor/src/ui/viewport.test.ts +++ b/packages/super-editor/src/ui/viewport.test.ts @@ -520,3 +520,38 @@ describe('ui.viewport.positionAt - resolution', () => { ui.destroy(); }); }); + +// SD-2945: `ui.viewport.contextAt({ x, y })` is the bundle right-click +// menus pass to `getContextMenuItems`. Verifies it composes the +// existing primitives (entityAt / positionAt / selection slice / rect +// hit-test) and always returns a well-formed shape, even when the +// editor surfaces nothing under the click. +describe('ui.viewport.contextAt - bundle composition', () => { + it('echoes the click point and surfaces empty primitives when nothing is under the click', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const ctx = ui.viewport.contextAt({ x: 100, y: 200 }); + expect(ctx.point).toEqual({ x: 100, y: 200 }); + expect(ctx.entities).toEqual([]); + expect(ctx.position).toBeNull(); + expect(ctx.insideSelection).toBe(false); + expect(ctx.selection).toBeDefined(); + expect(ctx.selection.empty).toBe(true); + + ui.destroy(); + }); + + it('coerces non-numeric input to a well-formed default bundle', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const ctx = ui.viewport.contextAt({} as never); + expect(ctx.point).toEqual({ x: 0, y: 0 }); + expect(ctx.entities).toEqual([]); + expect(ctx.position).toBeNull(); + expect(ctx.insideSelection).toBe(false); + + ui.destroy(); + }); +}); diff --git a/packages/superdoc/src/ui.d.ts b/packages/superdoc/src/ui.d.ts index 021e75a1f9..e68dc68d45 100644 --- a/packages/superdoc/src/ui.d.ts +++ b/packages/superdoc/src/ui.d.ts @@ -55,6 +55,8 @@ export { type TrackChangesSlice, type TrackedChangeAddress, type UIToolbarCommandState, + type ViewportContext, + type ViewportContextAtInput, type ViewportEntityAtInput, type ViewportEntityHit, type ViewportGetRectInput, From 5aaf4b504cad9ed038986f22c0d43c4ee228b570 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Tue, 5 May 2026 15:01:11 -0300 Subject: [PATCH 2/2] fix(ui): tighter context bundle detection + invoke stale-handle guard (SD-2945) isViewportContextBundle is the single guard both the controller proxy and the registry route through, so the two layers can't disagree on ambiguous inputs ({ point: null }, undefined, partial bundles). Closes the typeof-null trap in the proxy and the !Array.isArray-on-undefined trap in the registry. ContextMenuItem.invoke() now mirrors the captured-handle pattern at buildHandle.execute: a closure captures the entry that produced the menu item and refuses to dispatch when a later register({ id }) has replaced it. A stale menu item returns false instead of firing the new owner's handler with the old item's label/predicate/bundle. Weakens the contextAt JSDoc claim about deterministic empty-bundle defaults: non-numeric coords coerce to (0, 0), which is itself a valid viewport point. Three regression tests: invoke-after-replacement, partial-input legacy routing ({ entities, point: null }), and isViewportContextBundle boundaries (null point, missing x/y, non-object). --- .../src/ui/create-super-doc-ui.ts | 19 +++-- .../src/ui/custom-commands.test.ts | 70 +++++++++++++++++++ .../super-editor/src/ui/custom-commands.ts | 36 ++++++---- packages/super-editor/src/ui/types.ts | 12 +++- .../src/ui/viewport-context.test.ts | 29 +++++++- .../super-editor/src/ui/viewport-context.ts | 19 +++++ 6 files changed, 158 insertions(+), 27 deletions(-) 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 79db1e9094..4ace6f16c8 100644 --- a/packages/super-editor/src/ui/create-super-doc-ui.ts +++ b/packages/super-editor/src/ui/create-super-doc-ui.ts @@ -17,7 +17,7 @@ import type { import { collectEntityHitsFromChain } from './entity-at.js'; import { shallowEqual } from './equality.js'; import { resolvePositionAt } from './position-at.js'; -import { buildViewportContext } from './viewport-context.js'; +import { buildViewportContext, isViewportContextBundle } from './viewport-context.js'; import { shortcutFromEvent } from './keyboard-shortcuts.js'; import { scrollRangeIntoView } from './scroll-into-view.js'; import { getSelectionAnchorRect, getSelectionRects } from './selection-rects.js'; @@ -1300,17 +1300,16 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { // caller-supplied entities from `ui.viewport.entityAt`. // // SD-2945: input can also be the full {@link ViewportContext} - // bundle from `ui.viewport.contextAt({ x, y })`. Detected by the - // presence of `point`. The legacy `{ entities }` shape doesn't - // carry one. When a bundle is passed, predicates receive the - // full shape (point / position / insideSelection) and each - // returned item gets an `invoke()` closure that fires execute - // with `context` bound. + // bundle from `ui.viewport.contextAt({ x, y })`. Detected by a + // valid `point: { x, y }` field. `typeof null === 'object'`, so + // we explicitly require `point` to be a non-null object before + // routing to the bundle path; otherwise a hand-built input like + // `{ entities, point: null }` would be misclassified and the + // bundle's other fields would arrive as undefined. if (prop === 'getContextMenuItems') { return (input?: { entities?: ViewportEntityHit[] } | ViewportContext): ContextMenuItem[] => { - const isBundle = !!input && typeof (input as ViewportContext).point === 'object'; - if (isBundle) { - return customCommandsRegistry.getContextMenuItems(computeState(), input as ViewportContext); + if (isViewportContextBundle(input)) { + return customCommandsRegistry.getContextMenuItems(computeState(), input); } return customCommandsRegistry.getContextMenuItems( computeState(), diff --git a/packages/super-editor/src/ui/custom-commands.test.ts b/packages/super-editor/src/ui/custom-commands.test.ts index 1a647acdc8..4e31e47e7a 100644 --- a/packages/super-editor/src/ui/custom-commands.test.ts +++ b/packages/super-editor/src/ui/custom-commands.test.ts @@ -1289,6 +1289,76 @@ describe('ui.commands.getContextMenuItems - ViewportContext bundle', () => { ui.destroy(); }); + + // A menu held open across a re-registration must not dispatch the + // replacement's handler. The captured-handle pattern at + // `buildHandle.execute` already guards `commands.get(id).execute()` + // against this; `invoke()` follows the same identity check so a + // stale menu item cleanly returns false instead of firing the new + // owner's handler with the old item's label/predicate. + it('invoke() returns false (no dispatch) when the entry was replaced after the menu opened', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + const oldExecute = vi.fn(() => true); + const newExecute = vi.fn(() => true); + + ui.commands.register({ + id: 'replaceable', + execute: oldExecute, + contextMenu: { label: 'Old' }, + }); + + const items = ui.commands.getContextMenuItems(makeBundle()); + expect(items).toHaveLength(1); + expect(typeof items[0]!.invoke).toBe('function'); + + // Replace the registration after the menu items are captured. + ui.commands.register({ + id: 'replaceable', + execute: newExecute, + contextMenu: { label: 'New' }, + override: true, + }); + + const result = items[0]!.invoke!(); + expect(result).toBe(false); + expect(oldExecute).not.toHaveBeenCalled(); + expect(newExecute).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + // Bundle vs legacy-shape detection must reject inputs whose `point` + // is null or non-numeric. A consumer hand-building + // `{ entities, point: null }` should keep the legacy path; without + // this guard `typeof null === 'object'` would route them to the + // bundle branch and the registry would read `position` / + // `insideSelection` as undefined. + it('routes inputs with `point: null` through the legacy entities path, not the bundle path', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + const whenSpy = vi.fn(() => true); + + ui.commands.register({ + id: 'test.partial.input', + execute: () => true, + contextMenu: { label: 'Partial', when: whenSpy }, + }); + + ui.commands.getContextMenuItems({ + entities: [{ type: 'comment', id: 'c1' }], + point: null, + } as never); + + expect(whenSpy).toHaveBeenCalledTimes(1); + const arg = whenSpy.mock.calls[0]![0] as Record; + expect(arg.entities).toEqual([{ type: 'comment', id: 'c1' }]); + // Legacy shape: bundle-only fields stay absent. + expect(arg.point).toBeUndefined(); + expect(arg.insideSelection).toBeUndefined(); + + ui.destroy(); + }); }); describe('ui.commands.register — shortcut field', () => { diff --git a/packages/super-editor/src/ui/custom-commands.ts b/packages/super-editor/src/ui/custom-commands.ts index 65afb336b7..30d0a4f84b 100644 --- a/packages/super-editor/src/ui/custom-commands.ts +++ b/packages/super-editor/src/ui/custom-commands.ts @@ -1,4 +1,5 @@ import { normalizeShortcut } from './keyboard-shortcuts.js'; +import { isViewportContextBundle } from './viewport-context.js'; import type { ContextMenuContribution, ContextMenuItem, @@ -552,16 +553,18 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): } }, - // SD-2945: input is either the legacy `entities` array (consumer - // built the menu via `viewport.entityAt(...)` only) or the full + // SD-2945: input is either an entities array (consumer built the + // menu via `viewport.entityAt(...)` only) or a full // {@link ViewportContext} bundle from `viewport.contextAt(...)`. + // Routed through the same `isViewportContextBundle` guard the + // controller proxy uses so the two layers can't disagree on + // ambiguous inputs (e.g. `{ point: null }`, `undefined`). // Bundle inputs surface `point` / `position` / `insideSelection` // on the `when` predicate AND wire `invoke()` on each returned // item so consumers can fire execute with context bound. getContextMenuItems(state, input) { - const isBundle = !Array.isArray(input); - const entities: ViewportEntityHit[] = isBundle ? (input as ViewportContext).entities : (input ?? []); - const context: ViewportContext | null = isBundle ? (input as ViewportContext) : null; + const context = isViewportContextBundle(input) ? input : null; + const entities: ViewportEntityHit[] = context ? context.entities : Array.isArray(input) ? input : []; const items: ContextMenuItem[] = []; for (const entry of entries.values()) { @@ -597,15 +600,22 @@ export function createCustomCommandsRegistry(deps: CustomCommandsRegistryDeps): entry.lastContextMenuErrorMessage = null; } - // SD-2945: when called with a {@link ViewportContext} bundle, - // attach an `invoke()` closure that fires `execute` with - // `context` bound. Captures the captured `id` and the bundle - // by reference; consumers that mutate the bundle after - // calling `getContextMenuItems` would change what the - // closure sees, but the bundle is shaped as immutable data - // for this use case (contextAt builds it fresh per call). + // Identity-guarded `invoke()` mirrors the captured-handle + // pattern at `buildHandle.execute`: the closure refuses to + // dispatch when a later `register({ id })` has replaced this + // entry between menu open and click. Without that guard, a + // menu held open across a re-registration would fire the new + // owner's handler with the old item's label / predicate / + // bundle, which is exactly the stale-handle class of bug the + // prior pattern was added to prevent. + const ownEntry = entry; const itemId = entry.id; - const invoke = context ? () => registry.execute(itemId, undefined, context) : undefined; + const invoke = context + ? (): boolean | Promise => { + if (entries.get(itemId) !== ownEntry) return false; + return registry.execute(itemId, undefined, context); + } + : undefined; items.push({ id: entry.id, label: contribution.label, diff --git a/packages/super-editor/src/ui/types.ts b/packages/super-editor/src/ui/types.ts index 101f3dccf2..37199f6195 100644 --- a/packages/super-editor/src/ui/types.ts +++ b/packages/super-editor/src/ui/types.ts @@ -1696,9 +1696,15 @@ export interface ViewportHandle { * `ContextMenuItem.invoke()` so `execute({ context })` can act on * the click target without re-running geometry. * - * Always returns a bundle (no `null`). Every field has a - * deterministic default for the "outside the host" case so consumer - * code can destructure without null checks. + * Always returns a bundle (no `null`) so consumer code can + * destructure without null-checking the top-level result; the + * inner fields still carry the absent-case defaults each primitive + * defines (`entities = []`, `position = null`, + * `insideSelection = false`). Non-numeric coordinates coerce to + * `(0, 0)` rather than short-circuiting to an empty bundle, since + * `(0, 0)` is itself a valid viewport point and may legitimately + * sit inside the painted host; pass real coordinates if you want + * the result to reflect a specific click. */ contextAt(input: ViewportContextAtInput): ViewportContext; } diff --git a/packages/super-editor/src/ui/viewport-context.test.ts b/packages/super-editor/src/ui/viewport-context.test.ts index 95361e4827..9231f4ffa4 100644 --- a/packages/super-editor/src/ui/viewport-context.test.ts +++ b/packages/super-editor/src/ui/viewport-context.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { buildViewportContext, pointInsideRects } from './viewport-context.js'; +import { buildViewportContext, isViewportContextBundle, pointInsideRects } from './viewport-context.js'; import type { SelectionSlice, ViewportRect } from './types.js'; const rect = (top: number, left: number, width: number, height: number, pageIndex = 0): ViewportRect => ({ @@ -101,3 +101,30 @@ describe('buildViewportContext', () => { expect(ctx.insideSelection).toBe(false); }); }); + +describe('isViewportContextBundle', () => { + it('returns true only for objects whose `point` is `{ x: number, y: number }`', () => { + expect(isViewportContextBundle({ point: { x: 0, y: 0 }, entities: [] })).toBe(true); + expect(isViewportContextBundle({ point: { x: 100, y: 200 }, entities: [], position: null })).toBe(true); + }); + + it('rejects null / undefined', () => { + expect(isViewportContextBundle(null)).toBe(false); + expect(isViewportContextBundle(undefined)).toBe(false); + }); + + it('rejects the legacy `{ entities }` call shape', () => { + expect(isViewportContextBundle({ entities: [] })).toBe(false); + expect(isViewportContextBundle({})).toBe(false); + }); + + it('rejects an object whose `point` is null (avoids the `typeof null === "object"` trap)', () => { + expect(isViewportContextBundle({ entities: [], point: null })).toBe(false); + }); + + it('rejects partially-built bundles missing numeric x / y', () => { + expect(isViewportContextBundle({ point: {}, entities: [] })).toBe(false); + expect(isViewportContextBundle({ point: { x: 'a', y: 0 }, entities: [] })).toBe(false); + expect(isViewportContextBundle({ point: { x: 0 }, entities: [] })).toBe(false); + }); +}); diff --git a/packages/super-editor/src/ui/viewport-context.ts b/packages/super-editor/src/ui/viewport-context.ts index 71f830529d..77ef8d33d1 100644 --- a/packages/super-editor/src/ui/viewport-context.ts +++ b/packages/super-editor/src/ui/viewport-context.ts @@ -52,3 +52,22 @@ export function buildViewportContext(args: { insideSelection: pointInsideRects(args.x, args.y, args.selectionRects), }; } + +/** + * Type guard for the bundle vs the legacy `{ entities }` call shape + * accepted by `ui.commands.getContextMenuItems(input)`. + * + * Requires `point` to be a non-null object with numeric `x` / `y`. + * `typeof null === 'object'` is the easy trap here, plus a hand-built + * `{ entities, point: null }` should keep the legacy path so the + * registry doesn't read `entities` from a partial bundle whose other + * fields are `undefined`. Both call layers (controller proxy and + * registry) route through this guard so they cannot disagree. + */ +export function isViewportContextBundle(input: unknown): input is ViewportContext { + if (input == null || typeof input !== 'object') return false; + const candidate = input as { point?: unknown }; + if (candidate.point == null || typeof candidate.point !== 'object') return false; + const p = candidate.point as { x?: unknown; y?: unknown }; + return typeof p.x === 'number' && typeof p.y === 'number'; +}