From c693d53b375dd9e301557028f782719a066a8afb Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Wed, 29 Apr 2026 17:02:59 -0300 Subject: [PATCH 1/2] feat(superdoc/ui): ui.commands.get(id) for dynamic toolbar lookup (SD-2814) Adds a typed string-indexed lookup on the commands surface so consumers iterating over command IDs from a config array can resolve handles without unsafe casts. The Proxy-driven `ui.commands` mixes per-command handles, the `register()` method, and custom IDs, which makes string-indexed lookup type-error today: consumers fall back to `as unknown as` casts at every dispatch site. `ui.commands.get(id)` returns a unified `DynamicCommandHandle` for built-in or custom IDs and `undefined` for unknown IDs. Custom takes priority so `register({ override: true })` is honored. The emitted state carries the `source` discriminator so a single render path can drive both built-ins and customs without branching. --- .../src/ui/create-super-doc-ui.ts | 106 ++++++++++ .../src/ui/custom-commands.test.ts | 200 ++++++++++++++++++ packages/super-editor/src/ui/index.ts | 1 + packages/super-editor/src/ui/types.ts | 62 ++++++ packages/superdoc/src/ui.d.ts | 1 + 5 files changed, 370 insertions(+) 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 20c73d9110..70173dbf65 100644 --- a/packages/super-editor/src/ui/create-super-doc-ui.ts +++ b/packages/super-editor/src/ui/create-super-doc-ui.ts @@ -21,6 +21,7 @@ import type { CommandHandle, CommandsHandle, CommentsHandle, + DynamicCommandHandle, EqualityFn, ReviewHandle, ReviewItem, @@ -104,6 +105,22 @@ const FALLBACK_COMMAND_STATE: ToolbarCommandHandleState = { value: undefined, }; +/** + * Default state emitted from a {@link DynamicCommandHandle} when the + * command has no entry in `state.toolbar.commands` yet (e.g. the + * snapshot has not populated, or the command was unregistered between + * subscribe and the first emit). Carries `source: 'built-in'` because + * the dynamic handle for a built-in id reaches this branch only for + * built-ins. Customs never produce `undefined` here (the registry's + * computeStates always returns a state for every entry). + */ +const FALLBACK_DYNAMIC_STATE: UIToolbarCommandState = { + active: false, + disabled: true, + value: undefined, + source: 'built-in', +}; + /** * Full set of registered toolbar command ids, used to seed the * internal `createHeadlessToolbar` call. Without this the controller @@ -848,6 +865,87 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { customCommandsRegistry.destroy(); }); + // Per-id cache for the type-erased dynamic handles returned by + // `ui.commands.get(id)`. Cached so handle identity is stable across + // repeated lookups for the same id (consumers can put the result in + // a React `useMemo` dep and not re-create observers per render). + // Caches lazily: entries are created on first `get(id)` call. + const dynamicHandleCache = new Map(); + + /** + * Build a {@link DynamicCommandHandle} for a built-in id. Reuses the + * per-command Subscribable so dynamic and per-id observers share the + * same selector subscription against `state.toolbar.commands?.[id]`. + * The emitted slice already carries `source: 'built-in'` after the + * computeState merge, so no remapping is needed beyond the fallback. + */ + const buildBuiltInDynamicHandle = (id: PublicToolbarItemId): DynamicCommandHandle => { + return { + observe(listener) { + return getCommandSubscribable(id).subscribe((cmdState) => { + // The subscribable's selector returns a value cast to + // `ToolbarCommandHandleState` (no `source` field), but the + // runtime slice is the merged `UIToolbarCommandState` with the + // discriminator already populated by computeState. Cast back + // to the public dynamic shape rather than re-allocating a fresh + // object per emit. + const next = (cmdState ?? FALLBACK_DYNAMIC_STATE) as UIToolbarCommandState; + try { + listener(next); + } catch { + // see scheduleNotify + } + }); + }, + execute(payload?: unknown): boolean { + return (toolbarController.execute as (id: PublicToolbarItemId, payload?: unknown) => boolean)(id, payload); + }, + }; + }; + + /** + * Bridge a {@link CustomCommandHandle} from the custom-commands + * registry into the unified {@link DynamicCommandHandle} shape. + * Custom handles already emit `CustomCommandHandleState` (which + * carries `source: 'custom'`) and `execute` already accepts an + * unknown payload, so the wrapper is mostly identity. It exists to + * satisfy the public type and to keep `dynamicHandleCache` stable. + */ + const buildCustomDynamicHandle = (id: string): DynamicCommandHandle | undefined => { + const customHandle = customCommandsRegistry.getHandle(id); + if (!customHandle) return undefined; + return { + observe(listener) { + return customHandle.observe(listener); + }, + execute(payload?: unknown) { + return (customHandle.execute as (payload?: unknown) => boolean | Promise)(payload); + }, + }; + }; + + const getDynamicHandle = (id: string): DynamicCommandHandle | undefined => { + if (typeof id !== 'string' || id.length === 0) return undefined; + // Custom takes priority: `register({ id, override: true })` lets a + // custom command shadow a built-in id, and the dynamic-lookup + // result must follow that shadowing so consumers iterating over + // mixed id arrays get the override semantics they configured. + if (customCommandsRegistry.has(id)) { + // Don't memoize the wrapper: a later `unregister()` followed by a + // fresh `register()` for the same id swaps the underlying handle, + // and a stale wrapper would observe / execute against the prior + // registration. Building on demand is cheap (two closures) and + // keeps semantics aligned with the Proxy `get` path. + return buildCustomDynamicHandle(id); + } + if (!BUILT_IN_COMMAND_ID_SET.has(id)) return undefined; + let cached = dynamicHandleCache.get(id); + if (cached) return cached; + cached = buildBuiltInDynamicHandle(id as PublicToolbarItemId); + dynamicHandleCache.set(id, cached); + return cached; + }; + const commands = new Proxy({} as CommandsHandle, { get(_, prop) { if (typeof prop !== 'string') return undefined; @@ -857,6 +955,13 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { if (prop === 'register') { return customCommandsRegistry.register.bind(customCommandsRegistry); } + // `get(id)` is the typed dynamic-lookup escape hatch (see + // `DynamicCommandHandle`). Returns undefined for unregistered ids + // instead of producing a fallback handle that emits forever + // disabled state, which is what the bare proxy lookup does today. + if (prop === 'get') { + return getDynamicHandle; + } // Custom-registered ids surface a typed handle from the registry. // Built-in ids fall through to the existing per-id cache so they // keep the same observe/execute shape they had before SD-2802. @@ -1266,6 +1371,7 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { stateChangeListeners.clear(); commandHandleCache.clear(); commandSubscribableCache.clear(); + dynamicHandleCache.clear(); teardown.forEach((fn) => { try { fn(); diff --git a/packages/super-editor/src/ui/custom-commands.test.ts b/packages/super-editor/src/ui/custom-commands.test.ts index 25bfe9be61..290a79d928 100644 --- a/packages/super-editor/src/ui/custom-commands.test.ts +++ b/packages/super-editor/src/ui/custom-commands.test.ts @@ -551,3 +551,203 @@ describe('ui.commands.register', () => { ui.destroy(); }); }); + +describe('ui.commands.get', () => { + it('returns undefined for unregistered ids', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + expect(ui.commands.get('definitely-not-a-command')).toBeUndefined(); + // Empty / non-string ids guard the entry early. + expect(ui.commands.get('')).toBeUndefined(); + expect(ui.commands.get('register')).toBeUndefined(); + expect(ui.commands.get('get')).toBeUndefined(); + + ui.destroy(); + }); + + it('returns a handle for a built-in id, observe emits state with source: "built-in"', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const handle = ui.commands.get('bold'); + expect(handle).toBeDefined(); + expect(typeof handle?.observe).toBe('function'); + expect(typeof handle?.execute).toBe('function'); + + const listener = vi.fn(); + const off = handle!.observe(listener); + + // Initial synchronous emit, like every Subscribable in the controller. + expect(listener).toHaveBeenCalledTimes(1); + const emitted = listener.mock.calls[0][0]; + expect(emitted.source).toBe('built-in'); + expect(typeof emitted.active).toBe('boolean'); + expect(typeof emitted.disabled).toBe('boolean'); + + off(); + ui.destroy(); + }); + + it('returns the same handle on repeated lookups for a built-in id', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const a = ui.commands.get('italic'); + const b = ui.commands.get('italic'); + expect(a).toBeDefined(); + expect(a).toBe(b); + + ui.destroy(); + }); + + it('returns a handle for a custom-registered id, observe emits state with source: "custom"', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + ui.commands.register({ + id: 'company.aiRewrite', + execute: vi.fn(() => true), + getState: () => ({ active: false, disabled: false, value: 'ready' }), + }); + + const handle = ui.commands.get('company.aiRewrite'); + expect(handle).toBeDefined(); + + const listener = vi.fn(); + handle!.observe(listener); + + expect(listener).toHaveBeenCalledTimes(1); + expect(listener.mock.calls[0][0]).toEqual({ + active: false, + disabled: false, + value: 'ready', + source: 'custom', + }); + + ui.destroy(); + }); + + it('execute on a custom handle forwards payload to the registered execute', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const execute = vi.fn(() => true); + ui.commands.register<{ prompt: string }>({ + id: 'company.aiRewrite', + execute, + }); + + const handle = ui.commands.get('company.aiRewrite'); + handle!.execute({ prompt: 'fix tone' }); + + expect(execute).toHaveBeenCalledTimes(1); + expect(execute).toHaveBeenCalledWith({ + payload: { prompt: 'fix tone' }, + superdoc, + }); + + ui.destroy(); + }); + + it('returns undefined after unregistering a custom command', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const reg = ui.commands.register({ + id: 'company.aiRewrite', + execute: vi.fn(() => true), + }); + + expect(ui.commands.get('company.aiRewrite')).toBeDefined(); + + reg.unregister(); + + expect(ui.commands.get('company.aiRewrite')).toBeUndefined(); + + ui.destroy(); + }); + + it('returns the custom handle when a built-in is overridden', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const customExecute = vi.fn(() => true); + ui.commands.register({ + id: 'bold', + override: true, + execute: customExecute, + getState: () => ({ active: true, disabled: false, value: 'overridden' }), + }); + + const handle = ui.commands.get('bold'); + expect(handle).toBeDefined(); + + const listener = vi.fn(); + handle!.observe(listener); + + expect(listener.mock.calls[0][0]).toEqual({ + active: true, + disabled: false, + value: 'overridden', + source: 'custom', + }); + + handle!.execute(); + expect(customExecute).toHaveBeenCalledTimes(1); + + ui.destroy(); + }); + + it('observers detach when unsubscribed', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const handle = ui.commands.get('bold'); + const listener = vi.fn(); + const off = handle!.observe(listener); + + expect(listener).toHaveBeenCalledTimes(1); + listener.mockClear(); + off(); + + // After unsubscribe, no further emits. Fire a stub event that + // would otherwise rebuild the snapshot. + (superdoc as unknown as { fireEditor(event: string): void }).fireEditor('selectionUpdate'); + + expect(listener).not.toHaveBeenCalled(); + + ui.destroy(); + }); + + it('enables dynamic toolbar configuration without unsafe casts', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + ui.commands.register({ + id: 'company.aiRewrite', + execute: vi.fn(() => true), + getState: () => ({ active: false, disabled: false, value: 'ready' }), + }); + + // The friction case from SD-2814: a config-driven toolbar. + const config: string[] = ['bold', 'italic', 'company.aiRewrite', 'unknown-id']; + const states = config.map((id) => { + const handle = ui.commands.get(id); + if (!handle) return { id, found: false }; + let state: unknown = null; + const off = handle.observe((s) => { + state = s; + }); + off(); + return { id, found: true, state }; + }); + + expect(states[0].found).toBe(true); + expect(states[1].found).toBe(true); + expect(states[2].found).toBe(true); + expect(states[3].found).toBe(false); + + ui.destroy(); + }); +}); diff --git a/packages/super-editor/src/ui/index.ts b/packages/super-editor/src/ui/index.ts index 99f5671f97..d61801b5ed 100644 --- a/packages/super-editor/src/ui/index.ts +++ b/packages/super-editor/src/ui/index.ts @@ -59,6 +59,7 @@ export type { CustomCommandHandleState, CustomCommandRegistration, CustomCommandRegistrationResult, + DynamicCommandHandle, ToolbarCommandHandleState, ToolbarHandle, ToolbarSnapshotSlice, diff --git a/packages/super-editor/src/ui/types.ts b/packages/super-editor/src/ui/types.ts index 4e9979f75f..b841ea1310 100644 --- a/packages/super-editor/src/ui/types.ts +++ b/packages/super-editor/src/ui/types.ts @@ -529,8 +529,70 @@ export type CommandsHandle = { register( registration: CustomCommandRegistration, ): CustomCommandRegistrationResult; + + /** + * Look up a command handle by string id at runtime. + * + * Returns a {@link DynamicCommandHandle} for any registered id, + * built-in (`'bold'`, `'italic'`, etc.) or custom (registered via + * {@link CommandsHandle.register}), and `undefined` for unknown ids. + * + * Use this instead of indexing `ui.commands[id]` when the id is only + * known at runtime: a toolbar driven by a `string[]` config, a + * keyboard-shortcut router, a plugin loop. Indexing the surface with + * a generic `string` type-errors today because the surface mixes + * per-command handles with the `register` method, so consumers + * otherwise reach for an unsafe `as` cast on every dispatch site. + * + * The returned handle's `observe` listener receives the full + * {@link UIToolbarCommandState} (active / disabled / value / source), + * so a single render path can drive built-in *and* custom buttons + * uniformly without branching on the id. + */ + get(id: string): DynamicCommandHandle | undefined; }; +/** + * Type-erased command handle returned from {@link CommandsHandle.get}. + * + * Bridges built-ins ({@link CommandHandle}) and customs + * ({@link CustomCommandHandle}) into one observe/execute surface so + * consumers iterating `string[]` ids don't have to branch. The emitted + * state carries `source` so a uniform renderer can still distinguish + * the two when it wants. + * + * `execute` accepts an optional `unknown` payload and returns + * `boolean | Promise` (built-ins are sync, customs may be + * async). Capture the typed registration result for type-safe + * payloads. `get(id)` is the dynamic-lookup fallback, not a + * replacement for the per-id typing of `ui.commands.bold`. + */ +export interface DynamicCommandHandle { + /** + * Subscribe to the command's state. The listener fires once + * synchronously with the current state, then again whenever the + * state changes by shallow equality. Returns an unsubscribe. + * + * For ids in the built-in registry that haven't received a + * snapshot yet (or whose value has gone stale), the listener is + * still called with a deterministic disabled fallback so consumer + * code can render without a null check on every emit. + */ + observe(listener: (state: UIToolbarCommandState) => void): () => void; + /** + * Execute the command. Forwards to the same dispatch path as + * `ui.toolbar.execute(id, payload)` for built-ins and the + * registered `execute` handler for customs. + * + * The payload is `unknown` because `get(id)` erases per-command + * payload typing. Pass the value the command expects (e.g. the + * `string` for `'font-size'`). The returned Promise resolves to + * `false` when a custom command's handler rejects or returns + * `false`; built-ins return synchronously. + */ + execute(payload?: unknown): boolean | Promise; +} + /** * Input shape for {@link CommandsHandle.register}. * diff --git a/packages/superdoc/src/ui.d.ts b/packages/superdoc/src/ui.d.ts index 1163dfbe5e..77223da845 100644 --- a/packages/superdoc/src/ui.d.ts +++ b/packages/superdoc/src/ui.d.ts @@ -9,6 +9,7 @@ export { type CustomCommandHandleState, type CustomCommandRegistration, type CustomCommandRegistrationResult, + type DynamicCommandHandle, type EntityAddress, type EqualityFn, type ReviewHandle, From 1b746088360499653f348b77eeccbdc4f7f258d8 Mon Sep 17 00:00:00 2001 From: Caio Pizzol Date: Wed, 29 Apr 2026 17:23:49 -0300 Subject: [PATCH 2/2] fix(superdoc/ui): cached dynamic handle dispatches through later override (SD-2814) Addresses PR #3013 review (P1): a `DynamicCommandHandle` returned by `ui.commands.get('bold')` before a later `register({ id: 'bold', override: true })` kept routing execute through `toolbarController.execute('bold', ...)` even though the same handle's observe stream now emits the merged custom state. Config driven toolbars that memoize handles once would render the override visually while clicks ran the original built-in. The built-in dynamic handle now re-resolves at dispatch time: `customCommandsRegistry.has(id)` is checked before falling back to the toolbar controller, so override semantics hold for long-lived handles. Two regression tests cover the override path and the revert-after-unregister path. --- .../src/ui/create-super-doc-ui.ts | 13 +++- .../src/ui/custom-commands.test.ts | 67 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) 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 70173dbf65..02ebac8695 100644 --- a/packages/super-editor/src/ui/create-super-doc-ui.ts +++ b/packages/super-editor/src/ui/create-super-doc-ui.ts @@ -897,7 +897,18 @@ export function createSuperDocUI(options: SuperDocUIOptions): SuperDocUI { } }); }, - execute(payload?: unknown): boolean { + execute(payload?: unknown): boolean | Promise { + // Re-resolve at dispatch time so a `register({ override: true })` + // call that lands AFTER this handle was cached still routes + // dispatch through the override. Without this, the cached + // handle's observe stream emits the merged custom state (the + // selector reads the merged `state.toolbar.commands[id]`) but + // execute keeps running the original built-in, leaving config + // driven toolbars showing the override visually while clicks + // run the wrong command. + if (customCommandsRegistry.has(id)) { + return customCommandsRegistry.execute(id, payload); + } return (toolbarController.execute as (id: PublicToolbarItemId, payload?: unknown) => boolean)(id, payload); }, }; diff --git a/packages/super-editor/src/ui/custom-commands.test.ts b/packages/super-editor/src/ui/custom-commands.test.ts index 290a79d928..b041b97c8e 100644 --- a/packages/super-editor/src/ui/custom-commands.test.ts +++ b/packages/super-editor/src/ui/custom-commands.test.ts @@ -699,6 +699,73 @@ describe('ui.commands.get', () => { ui.destroy(); }); + // Regression for PR #3013 review comment: cached dynamic handles + // for built-in ids must dispatch through any later + // `register({ id, override: true })`. A consumer that memoizes + // `ui.commands.get('bold')` once and only later registers an + // override would otherwise see the merged custom state on the + // observe stream while still routing execute() to the built-in, + // breaking override semantics for long-lived handles. + it('cached built-in dynamic handle dispatches through a later override', () => { + const { superdoc } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + // Cache the handle BEFORE registering the override. + const cachedHandle = ui.commands.get('bold'); + expect(cachedHandle).toBeDefined(); + + const customExecute = vi.fn(() => true); + ui.commands.register({ + id: 'bold', + override: true, + execute: customExecute, + getState: () => ({ active: true, disabled: false, value: 'overridden' }), + }); + + // Execute via the cached handle. The custom override's execute + // should run, not the built-in toolbar controller's bold. + cachedHandle!.execute(); + expect(customExecute).toHaveBeenCalledTimes(1); + + ui.destroy(); + }); + + it('cached built-in dynamic handle reverts to built-in dispatch after the override unregisters', () => { + const { superdoc, editor } = makeStubs(); + const ui = createSuperDocUI({ superdoc }); + + const cachedHandle = ui.commands.get('bold'); + const customExecute = vi.fn(() => true); + const reg = ui.commands.register({ + id: 'bold', + override: true, + execute: customExecute, + }); + + cachedHandle!.execute(); + expect(customExecute).toHaveBeenCalledTimes(1); + + // After unregister, the built-in dispatch path resumes. + reg.unregister(); + + // Reset the editor's bold spy so we can detect a built-in dispatch + // after unregister. The toolbarController is internal, but a + // built-in dispatch ultimately routes through the editor's + // commands surface; the stub's `commands.toggleBold` mock receives + // the call. (If toolbarController short-circuits before reaching + // the editor it still won't call customExecute, which is what we + // assert below.) + customExecute.mockClear(); + cachedHandle!.execute(); + expect(customExecute).not.toHaveBeenCalled(); + + // Editor reference is unused if toolbar dispatch routes elsewhere; + // the assertion that matters is `customExecute` did not fire. + void editor; + + ui.destroy(); + }); + it('observers detach when unsubscribed', () => { const { superdoc } = makeStubs(); const ui = createSuperDocUI({ superdoc });