Skip to content

Commit 71b72f4

Browse files
committed
fix: selection insert target validation and alias ref ambiguity
1 parent 997f2a7 commit 71b72f4

8 files changed

Lines changed: 215 additions & 52 deletions

File tree

packages/document-api/src/delete/delete.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,8 @@ function validateDeleteInput(input: unknown): asserts input is DeleteInput {
7272
});
7373
}
7474

75-
if (hasRef && typeof ref !== 'string') {
76-
throw new DocumentApiValidationError('INVALID_TARGET', 'ref must be a string.', {
75+
if (hasRef && (typeof ref !== 'string' || ref === '')) {
76+
throw new DocumentApiValidationError('INVALID_TARGET', 'ref must be a non-empty string.', {
7777
field: 'ref',
7878
value: ref,
7979
});

packages/document-api/src/format/format.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,8 @@ function validateTargetLocator(input: Record<string, unknown>, operation: string
115115
});
116116
}
117117

118-
if (hasRef && typeof input.ref !== 'string') {
119-
throw new DocumentApiValidationError('INVALID_TARGET', 'ref must be a string.', {
118+
if (hasRef && (typeof input.ref !== 'string' || input.ref === '')) {
119+
throw new DocumentApiValidationError('INVALID_TARGET', 'ref must be a non-empty string.', {
120120
field: 'ref',
121121
value: input.ref,
122122
});

packages/document-api/src/insert/insert.test.ts

Lines changed: 94 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,41 @@ function createStubWriteAdapter(): WriteAdapter {
1616
return { write: vi.fn(), insertStructured: vi.fn(), replaceStructured: vi.fn() } as unknown as WriteAdapter;
1717
}
1818

19+
function exec(input: unknown): void {
20+
executeInsert(createStubSelectionAdapter(), createStubWriteAdapter(), input as InsertInput);
21+
}
22+
23+
// ---------------------------------------------------------------------------
24+
// Input shape discrimination
25+
// ---------------------------------------------------------------------------
26+
27+
describe('executeInsert: input shape', () => {
28+
it('rejects non-object input', () => {
29+
expect(() => exec(null)).toThrow('non-null object');
30+
expect(() => exec('hello')).toThrow('non-null object');
31+
expect(() => exec(42)).toThrow('non-null object');
32+
});
33+
34+
it('rejects input with neither value nor content', () => {
35+
expect(() => exec({})).toThrow('either "value"');
36+
});
37+
38+
it('rejects input with both value and content', () => {
39+
expect(() => exec({ value: 'hello', content: { blocks: [] } })).toThrow('not both');
40+
});
41+
});
42+
1943
// ---------------------------------------------------------------------------
20-
// ref validation
44+
// Text insert: ref validation
2145
// ---------------------------------------------------------------------------
2246

2347
describe('executeInsert: ref validation', () => {
2448
it('rejects empty string ref', () => {
25-
const input: InsertInput = { value: 'hello', ref: '' } as unknown as InsertInput;
26-
expect(() => executeInsert(createStubSelectionAdapter(), createStubWriteAdapter(), input)).toThrow(
27-
'ref must be a non-empty string',
28-
);
49+
expect(() => exec({ value: 'hello', ref: '' })).toThrow('ref must be a non-empty string');
2950
});
3051

3152
it('rejects non-string ref', () => {
32-
const input = { value: 'hello', ref: 42 } as unknown as InsertInput;
33-
expect(() => executeInsert(createStubSelectionAdapter(), createStubWriteAdapter(), input)).toThrow(
34-
'ref must be a non-empty string',
35-
);
53+
expect(() => exec({ value: 'hello', ref: 42 })).toThrow('ref must be a non-empty string');
3654
});
3755

3856
it('does not reject undefined ref (untargeted insert is valid)', () => {
@@ -41,3 +59,70 @@ describe('executeInsert: ref validation', () => {
4159
expect(() => executeInsert(createStubSelectionAdapter(), writeAdapter, { value: 'hello' })).not.toThrow();
4260
});
4361
});
62+
63+
// ---------------------------------------------------------------------------
64+
// Text insert: target/ref mutual exclusivity
65+
// ---------------------------------------------------------------------------
66+
67+
describe('executeInsert: target/ref mutual exclusivity', () => {
68+
it('rejects input with both target and ref', () => {
69+
const target = {
70+
kind: 'selection',
71+
start: { kind: 'text', blockId: 'b1', offset: 0 },
72+
end: { kind: 'text', blockId: 'b1', offset: 0 },
73+
};
74+
expect(() => exec({ value: 'hello', target, ref: 'r1' })).toThrow('either "target" or "ref", not both');
75+
});
76+
});
77+
78+
// ---------------------------------------------------------------------------
79+
// Text insert: content type validation
80+
// ---------------------------------------------------------------------------
81+
82+
describe('executeInsert: content type validation', () => {
83+
it('rejects invalid content type', () => {
84+
expect(() => exec({ value: 'hello', type: 'pdf' })).toThrow('text, markdown, html');
85+
});
86+
87+
it('rejects non-string content type', () => {
88+
expect(() => exec({ value: 'hello', type: 123 })).toThrow('text, markdown, html');
89+
});
90+
});
91+
92+
// ---------------------------------------------------------------------------
93+
// Text insert: value validation
94+
// ---------------------------------------------------------------------------
95+
96+
describe('executeInsert: value validation', () => {
97+
it('rejects non-string value', () => {
98+
expect(() => exec({ value: 42 })).toThrow('value must be a string');
99+
});
100+
});
101+
102+
// ---------------------------------------------------------------------------
103+
// Text insert: unknown fields
104+
// ---------------------------------------------------------------------------
105+
106+
describe('executeInsert: unknown fields', () => {
107+
it('rejects unknown fields on text input', () => {
108+
expect(() => exec({ value: 'hello', bogus: true })).toThrow('bogus');
109+
});
110+
});
111+
112+
// ---------------------------------------------------------------------------
113+
// Structural insert: validation
114+
// ---------------------------------------------------------------------------
115+
116+
describe('executeInsert: structural input validation', () => {
117+
it('rejects structural input with text-only "type" field', () => {
118+
expect(() => exec({ content: { blocks: [] }, type: 'markdown' })).toThrow('"type" field is only valid');
119+
});
120+
121+
it('rejects invalid placement value', () => {
122+
expect(() => exec({ content: { blocks: [] }, placement: 'middle' })).toThrow('before, after');
123+
});
124+
125+
it('rejects text-only fields on structural input', () => {
126+
expect(() => exec({ content: { blocks: [] }, ref: 'r1' })).toThrow();
127+
});
128+
});

packages/document-api/src/insert/insert.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,19 @@ function validateStructuralInsertInput(input: Record<string, unknown>): void {
216216
// Execution
217217
// ---------------------------------------------------------------------------
218218

219+
/**
220+
* Executes an insert operation, routing to the appropriate adapter path.
221+
*
222+
* - Text inserts with `target` or `ref` route through the SelectionMutationAdapter.
223+
* - Structural inserts (SDFragment) and non-text content types route through WriteAdapter.
224+
* - Text inserts without a locator append at the document end via WriteAdapter.
225+
*
226+
* @param selectionAdapter - Adapter for target/ref-based text mutations.
227+
* @param writeAdapter - Adapter for structural and untargeted writes.
228+
* @param input - Insert payload (text string or structural SDFragment).
229+
* @param options - Optional mutation options (changeMode, dryRun, expectedRevision).
230+
* @returns Receipt indicating success/failure and mutation metadata.
231+
*/
219232
export function executeInsert(
220233
selectionAdapter: SelectionMutationAdapter,
221234
writeAdapter: WriteAdapter,

packages/document-api/src/replace/replace.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,8 +95,8 @@ function validateTargetLocator(input: Record<string, unknown>, operation: string
9595
});
9696
}
9797

98-
if (hasRef && typeof input.ref !== 'string') {
99-
throw new DocumentApiValidationError('INVALID_TARGET', 'ref must be a string.', {
98+
if (hasRef && (typeof input.ref !== 'string' || input.ref === '')) {
99+
throw new DocumentApiValidationError('INVALID_TARGET', 'ref must be a non-empty string.', {
100100
field: 'ref',
101101
value: input.ref,
102102
});

packages/super-editor/src/document-api-adapters/helpers/node-address-resolver.ts

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,20 @@ function resolveParagraphRuntimeIdentity(
157157
return toId(attrs.sdBlockId) ?? buildFallbackBlockNodeId(nodeType, pos, path);
158158
}
159159

160+
/**
161+
* Resolves the public document-api nodeId for a block-level ProseMirror node.
162+
*
163+
* ID resolution strategy varies by block family:
164+
* - **Paragraphs**: paraId → sdBlockId → deterministic fallback
165+
* - **Tables/cells**: legacy attrs → non-volatile sdBlockId → deterministic fallback
166+
* - **Other blocks**: blockId → id → paraId → uuid → sdBlockId
167+
*
168+
* @param node - The ProseMirror node.
169+
* @param pos - Absolute document position of the node.
170+
* @param nodeType - The mapped block node type.
171+
* @param path - Optional traversal path for deterministic fallback IDs.
172+
* @returns The resolved nodeId, or `undefined` if none could be determined.
173+
*/
160174
export function resolveBlockNodeId(
161175
node: ProseMirrorNode,
162176
pos: number,
@@ -346,10 +360,42 @@ export function findBlockByIdStrict(index: BlockIndex, address: BlockNodeAddress
346360
return candidate;
347361
}
348362

363+
/**
364+
* Resolves a nodeId against alias entries in the block index (e.g., sdBlockId
365+
* registered as an alias for a deterministic primary ID).
366+
*
367+
* @param index - The block index to search.
368+
* @param nodeId - The node ID to resolve via alias lookup.
369+
* @returns The single matching candidate, or `undefined` if no alias matches.
370+
* @throws {DocumentApiAdapterError} `AMBIGUOUS_TARGET` when multiple blocks share the alias.
371+
*/
372+
export function resolveBlockAlias(index: BlockIndex, nodeId: string): BlockCandidate | undefined {
373+
if (!index.byId) return undefined;
374+
375+
const aliasMatches = new Map<string, BlockCandidate>();
376+
for (const [key, candidate] of index.byId) {
377+
if (!key.endsWith(`:${nodeId}`)) continue;
378+
aliasMatches.set(`${candidate.nodeType}:${candidate.nodeId}`, candidate);
379+
}
380+
381+
if (aliasMatches.size > 1) {
382+
throw new DocumentApiAdapterError('AMBIGUOUS_TARGET', `Multiple blocks share nodeId "${nodeId}" via aliases.`, {
383+
nodeId,
384+
count: aliasMatches.size,
385+
matches: Array.from(aliasMatches.values()).map((candidate) => ({
386+
nodeType: candidate.nodeType,
387+
nodeId: candidate.nodeId,
388+
})),
389+
});
390+
}
391+
392+
return aliasMatches.size === 1 ? Array.from(aliasMatches.values())[0] : undefined;
393+
}
394+
349395
/**
350396
* Finds a block candidate by raw nodeId without requiring a nodeType.
351397
*
352-
* This is needed for create operations that position relative to _any_ block type.
398+
* Falls back to alias resolution when no primary match exists.
353399
*
354400
* @param index - The block index to search.
355401
* @param nodeId - The node ID to resolve.
@@ -370,26 +416,8 @@ export function findBlockByNodeIdOnly(index: BlockIndex, nodeId: string): BlockC
370416
}
371417

372418
// No primary match — check alias entries (e.g., sdBlockId for paragraph-like nodes).
373-
const aliasMatches = new Map<string, BlockCandidate>();
374-
for (const [key, candidate] of index.byId) {
375-
if (!key.endsWith(`:${nodeId}`)) continue;
376-
aliasMatches.set(`${candidate.nodeType}:${candidate.nodeId}`, candidate);
377-
}
378-
379-
if (aliasMatches.size === 1) {
380-
return Array.from(aliasMatches.values())[0]!;
381-
}
382-
383-
if (aliasMatches.size > 1) {
384-
throw new DocumentApiAdapterError('AMBIGUOUS_TARGET', `Multiple blocks share nodeId "${nodeId}" via aliases.`, {
385-
nodeId,
386-
count: aliasMatches.size,
387-
matches: Array.from(aliasMatches.values()).map((candidate) => ({
388-
nodeType: candidate.nodeType,
389-
nodeId: candidate.nodeId,
390-
})),
391-
});
392-
}
419+
const alias = resolveBlockAlias(index, nodeId);
420+
if (alias) return alias;
393421

394422
throw new DocumentApiAdapterError('TARGET_NOT_FOUND', `Block with nodeId "${nodeId}" was not found.`, { nodeId });
395423
}

packages/super-editor/src/document-api-adapters/plan-engine/compiler.ts

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,12 @@ import { getBlockIndex } from '../helpers/index-cache.js';
3232
import { getRevision } from './revision-tracker.js';
3333
import { executeTextSelector } from '../find/text-strategy.js';
3434
import { executeBlockSelector } from '../find/block-strategy.js';
35-
import { isTextBlockCandidate, type BlockCandidate, type BlockIndex } from '../helpers/node-address-resolver.js';
35+
import {
36+
isTextBlockCandidate,
37+
resolveBlockAlias,
38+
type BlockCandidate,
39+
type BlockIndex,
40+
} from '../helpers/node-address-resolver.js';
3641
import { resolveTextRangeInBlock } from '../helpers/text-offset-resolver.js';
3742
import { resolveSelectionTarget, resolveSelectionPointPosition } from '../helpers/selection-target-resolver.js';
3843
import { expandDeleteSelection } from '../helpers/expand-delete-selection.js';
@@ -675,18 +680,18 @@ function resolveTextRef(editor: Editor, index: BlockIndex, step: MutationStep, r
675680
}
676681

677682
function resolveBlockRef(editor: Editor, index: BlockIndex, step: MutationStep, ref: string): CompiledTarget[] {
678-
let candidate = index.candidates.find((c) => c.nodeId === ref);
683+
const primaryMatches = index.candidates.filter((c) => c.nodeId === ref);
684+
if (primaryMatches.length > 1) {
685+
throw planError('AMBIGUOUS_TARGET', `Multiple blocks share nodeId "${ref}".`, step.id, {
686+
ref,
687+
count: primaryMatches.length,
688+
});
689+
}
690+
679691
// Alias-aware fallback: if the ref is an sdBlockId registered as an alias
680692
// (e.g., volatile UUID replaced by a deterministic para-auto-* primary ID),
681-
// try the byId map which includes alias entries.
682-
if (!candidate && index.byId) {
683-
for (const [key, c] of index.byId) {
684-
if (key.endsWith(`:${ref}`)) {
685-
candidate = c;
686-
break;
687-
}
688-
}
689-
}
693+
// try the shared resolveBlockAlias helper which enforces ambiguity checks.
694+
const candidate = primaryMatches[0] ?? resolveBlockAlias(index, ref);
690695
if (!candidate) return [];
691696

692697
const blockText = getBlockText(editor, candidate);

packages/super-editor/src/document-api-adapters/plan-engine/plan-wrappers.ts

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,20 @@ export function selectionMutationWrapper(
567567
if (mode === 'tracked') ensureTrackedInlinePropertySupport(inlineKeys);
568568
}
569569

570+
// Text inserts require a position inside a textblock. Node-edge targets
571+
// (e.g., "before paragraph/table/image") resolve to block boundaries where
572+
// tr.insert() would place a text node at the doc/block level instead of
573+
// inside a textblock. Reject them up front before compilation.
574+
if (request.kind === 'insert' && request.target) {
575+
const hasNodeEdge = request.target.start.kind === 'nodeEdge' || request.target.end.kind === 'nodeEdge';
576+
if (hasNodeEdge) {
577+
throw new DocumentApiAdapterError(
578+
'INVALID_TARGET',
579+
'Text inserts do not support nodeEdge targets. Use a text-offset target inside a textblock.',
580+
);
581+
}
582+
}
583+
570584
const stepId = uuidv4();
571585
const where = buildSelectionWhere(request);
572586
const step = buildSelectionStepDef(stepId, request, where);
@@ -576,15 +590,14 @@ export function selectionMutationWrapper(
576590
// document state without mutating anything.
577591
const compiled = compilePlan(editor, [step]);
578592

579-
// Insert requires a collapsed (point) target. Reject non-collapsed ranges
580-
// and multi-segment spans so the receipt is never misleading about the
581-
// actual insertion point.
593+
// Insert requires a collapsed (point) target inside a textblock.
594+
// Reject spans, non-collapsed ranges, and positions that resolve to
595+
// block boundaries (e.g., refs to images/tables) so executeTextInsert()
596+
// never attempts tr.insert() outside a textblock.
582597
if (request.kind === 'insert') {
583598
const compiledStep = compiled.mutationSteps.find((s) => s.step.id === stepId);
584599
const target = compiledStep?.targets[0];
585600
if (target) {
586-
// Multi-segment refs (span) are never valid for point inserts.
587-
// Non-collapsed range/selection targets are also rejected.
588601
const isInvalidInsertTarget = target.kind === 'span' || target.absFrom !== target.absTo;
589602
if (isInvalidInsertTarget) {
590603
const resolution = buildSelectionResolutionFromCompiled(compiled, stepId);
@@ -594,6 +607,21 @@ export function selectionMutationWrapper(
594607
failure: { code: 'INVALID_TARGET', message: 'Insert operations require a collapsed target range.' },
595608
};
596609
}
610+
611+
// Verify the resolved position is inside a textblock — refs to
612+
// non-text leaf blocks (image, table, sdt) resolve to block
613+
// boundaries where a text node cannot be placed.
614+
if (target.kind === 'range') {
615+
const resolved = editor.state.doc.resolve(target.absFrom);
616+
if (!resolved.parent.isTextblock) {
617+
const resolution = buildSelectionResolutionFromCompiled(compiled, stepId);
618+
return {
619+
success: false,
620+
resolution,
621+
failure: { code: 'INVALID_TARGET', message: 'Text insert target must be inside a textblock.' },
622+
};
623+
}
624+
}
597625
}
598626
}
599627

@@ -602,8 +630,12 @@ export function selectionMutationWrapper(
602630
checkRevision(editor, options?.expectedRevision);
603631

604632
// Dry-run: compile and resolve, but do NOT execute.
633+
// Mirror apply-time validation so callers do not get false-positive previews.
605634
if (options?.dryRun) {
606635
const resolution = buildSelectionResolutionFromCompiled(compiled, stepId);
636+
if (request.kind === 'insert' && !request.text) {
637+
return { success: false, resolution, failure: { code: 'NO_OP', message: 'Insert text is empty.' } };
638+
}
607639
return { success: true, resolution };
608640
}
609641

0 commit comments

Comments
 (0)