Commit cd84f8e
authored
feat(document-api): selection primitives + multi-block comment targets (SD-2668) (#2941)
* feat(document-api): selection primitives + multi-block comment targets (SD-2668)
Adds `editor.doc.selection.current()` and `editor.doc.selection.onChange()` so
consumers can build custom toolbars, comments sidebars, and selection-driven
UIs without reaching into ProseMirror internals. Widens `comments.create`
target to accept `TextTarget` so multi-block selections anchor across blocks
instead of silently collapsing to the first segment.
- Consumers previously had to resolve PM positions through
`editor.state.doc`, walk the PM tree for the containing block's `sdBlockId`,
and convert to flattened-text offsets — ~30 lines of editor internals.
`selection.current()` returns a portable `SelectionInfo { empty, target,
activeMarks, text? }` with a multi-segment `TextTarget` ready for
`comments.create`.
- `comments.list().target` already used multi-segment `TextTarget`; the
write side (`comments.create`) only accepted single-block `TextAddress`,
so drag-selecting across paragraphs lost data with no warning.
- Contract, schemas, dispatch, and adapter wired. Super-editor adapter
projects PM selections into the flattened-text model via the existing
`computeTextContentLength` helper.
Part of the SD-2667 drop-in assessment umbrella.
* fix(document-api): address PR review on selection + comments scope
1. Drop the aspirational `in: StoryLocator` parameter from
`selection.current`. The adapter always read the live editor selection
and merely copied `input.in` into the returned `TextTarget.story`, so a
body selection could be mislabeled as a header/footer selection. The
operation now documents that it always reflects whichever story holds
focus; story-scoped selection reads are out of scope.
2. Narrow `comments.create` multi-segment handling to contiguous ranges.
Validation previously accepted any non-empty segment array; the handler
spanned `first.start → last.end` as a single PM range, so disjoint or
out-of-order segments would silently anchor the comment over text the
caller never selected. `addCommentHandler` now resolves every segment,
rejects out-of-order pairs, and rejects any pair with non-empty text
between them (`INVALID_TARGET`).
3. Fix schema inconsistency: `SelectionInfo.target` is `TextTarget | null`
at the type level, but the published schema required it as an `object`.
Schema now uses `oneOf: [textTargetSchema, { type: 'null' }]` so empty
selections validate against the exported contract.
4. Make `SelectionAdapter` optional on `DocumentApiAdapters`. This is a
public exported interface; adding a required member is a source
breaking change for external adapter constructors. The factory now
throws `SELECTION_ADAPTER_UNAVAILABLE` when selection operations are
called without a registered adapter.
Tests: add 3 new cases (multi-segment forward, missing-adapter for
`current` + `onChange`). 1374 pass, 0 fail.
* fix(document-api): correct selection offset mapping + cancel pending flush
Two bot findings on PR 2924:
- P1: `collectTextSegments` was deriving block-relative offsets with
`selStart - blockStart`, treating raw PM positions as flattened text
offsets. That's only equivalent when the block contains pure text; it
diverges whenever the block has inline wrappers (e.g. `run` marks) or
leaf atoms whose PM boundary tokens do not count in the flattened
model. A selection inside a run would therefore return a `TextTarget`
off by the number of wrapper boundary tokens, and `comments.create`
using that target would anchor to the wrong text.
Added `pmPositionToTextOffset(blockNode, blockPos, pmPos)` alongside
the existing `resolveTextRangeInBlock` in `text-offset-resolver.ts` —
it walks the block with the same flattened model (text = length,
leaf = 1, block separator = 1, inline wrapper tokens = 0) and
returns the correct offset. `collectTextSegments` now uses it for
both endpoints.
- P2: `subscribeToSelection` scheduled `flush` via `queueMicrotask` but
the returned unsubscribe only detached listeners — a microtask
already queued before cleanup would still fire, invoking the listener
after unsubscribe returned (stale state updates on component unmount).
Added a `cancelled` closure flag set by unsubscribe and checked in
`flush` and `schedule`.
Tests: 5 new cases for `pmPositionToTextOffset` covering plain text,
inline wrapper transparency, leaf atoms with `nodeSize > 1`, before-
block-start, and past-block-end. 11515 super-editor pass, 0 fail.
* test(document-api): positive-path + write-side selection coverage
Addresses PR review findings on test coverage:
- Adds `selection-info-resolver.test.ts` (11 cases) covering
`resolveCurrentSelectionInfo` projection (empty state, single-block
selection, multi-block segment-per-touched-block, missing blockId,
includeText on/off, active-marks empty path) and
`subscribeToSelection` (listener fires once per tick, unsubscribe
stops firing, queued-microtask-cancellation on unmount).
- Adds 3 write-side cases to `comments-wrappers.test.ts` for the
multi-segment path: out-of-order rejection with INVALID_TARGET /
"document order" message, non-contiguous-gap rejection with
"contiguous" message, and the contiguous-success path verifying
the spanned PM range [first.from, last.to] is applied.
- Updates `assemble-adapters.test.ts` to assert both
`selection.current` and `selection.onChange` are wired on the
adapter bag.
- Adds a new section in `tests/consumer-typecheck/src/customer-scenario.ts`
exercising the exported `editor.doc.selection.current()` surface,
`SelectionInfo` destructuring, multi-segment `TextTarget` pass-through
to `comments.create`, and the `onChange` subscription shape. Requires
threading the new types through `packages/super-editor/src/index.ts`
and `packages/superdoc/src/index.js` JSDoc typedefs so they are
reachable from the `superdoc` package entrypoint.
- Exempts `selection.onChange` from the contract-parity member-path
check via `META_MEMBER_PATHS` — it is a subscription primitive, not
a request/response operation, so it does not belong in
`OPERATION_DEFINITIONS` / schemas / dispatch. Fixes the `validate`
CI job that otherwise rejects the new runtime member.
Deferred to SD-2671 (follow-up):
- doc-api-stories/comments/multi-segment-target.ts (CLI-harness story)
- doc-api-stories/selection story
- Playwright behavior test that drives selection.current → comments.create
Verified: document-api 1374 pass, super-editor 11529 pass,
tests/consumer-typecheck compiles clean against the packed tarball.
* fix(document-api): type-guard comment target routing + intersect marks per-node
Two bot findings on PR 2941:
- Comment-target routing used `'segments' in target` to pick between
the TextAddress and TextTarget branches. That misclassifies a
TextAddress that happens to carry an extra `segments` field (e.g.
from object spread or a caller with wider union types) and then
crashes on `segments[0]` inside `targetToSegments`. Replaced with an
`isTextTargetShape` guard that requires `kind === 'text'` plus a
non-empty `segments` array — matching the runtime contract the
document-api validator already enforces. Malformed payloads now fall
through to the single-block branch and surface as clean
`INVALID_TARGET` responses.
- `markTypesPresentEverywhere` expanded each text node's mark set once
per selected character and intersected those arrays afterwards. For a
10 KB selection that allocated 10,000 Set references per event, and
`selection.onChange` fires frequently during editing. Rewrote as a
running intersection over text nodes: initialise from the first
overlapping node's mark set, intersect with each subsequent node,
stop descending once the intersection empties. O(text-nodes) with
bounded allocation; same return value.
Tests: +4 regression cases (type-guard misclassification, per-node
intersection across multiple runs, empty-intersection for a mixed
marked/unmarked selection, 10k-char selection completes under 50ms).
Adapter suite 3190 pass, 0 fail.
* fix(document-api): close 5 multi-segment / selection edge cases on PR 2941
Five real correctness regressions surfaced in review-round-2:
- Hybrid TextAddress+TextTarget payloads (both shapes carried in one
object) passed both `isTextAddress` and `isTextTarget` validators, then
routed through the segments branch and silently dropped blockId/range.
Hardened `isTextTargetShape` to reject TextAddress-style fields, so
hybrids fall through to the explicit-block path.
- Two collapsed segments in different blocks slipped both the order +
contiguity guards AND the spanning-range collapse check (because
firstResolved.from < lastResolved.to across the block boundary), then
silently anchored a comment over content the caller never selected.
Added a per-segment collapse check before the resolution loop.
- `textBetween(prev.to, curr.from, '')` returns '' when the gap is
composed entirely of inline atoms (images, math, etc), so an
atom-only gap passed the contiguity check. Pass a `leafText`
callback so atoms still register as gap content. Block separators
remain represented by an empty `blockSeparator`, so legitimate
cross-block adjacency still produces an empty gap and accepts.
- `collectTextSegments` skipped textblocks with no addressable id and
emitted segments for the rest, returning a TextTarget that didn't
cover the user's selection. Now bails out and returns null for the
whole selection so callers can refuse the action rather than act on
partial data.
- `subscribeToSelection` listened to both `selectionUpdate` and
`transaction` to catch programmatic selection changes that don't fire
the former. The `transaction` event also fires on every keystroke, so
the listener ran per character even when SelectionInfo was unchanged.
Added a content-key dedupe (empty + segments + activeMarks) so
identical states fire the listener once. The listener still fires
immediately when SelectionInfo changes for any reason.
Also: relaxed the 10k-char wall-clock perf bound from 50ms to 500ms
(the functional assertion is the real correctness check; the timing
is a smoke check that won't flake on noisy CI workers).
Tests: +6 regression cases (hybrid routing, collapsed multi-block,
atom-only gap, non-addressable block aborts the walk, transaction
dedupe, dedupe doesn't become sticky after a real change).
Adapter suite 3198 pass, 0 fail.1 parent a4b136a commit cd84f8e
35 files changed
Lines changed: 1887 additions & 53 deletions
File tree
- apps/docs
- document-api
- reference
- capabilities
- comments
- selection
- document-engine
- packages
- document-api
- scripts
- src
- comments
- contract
- invoke
- selection
- sdk
- langs/browser/src
- tools
- super-editor/src
- editors/v1/document-api-adapters
- helpers
- plan-engine
- superdoc/src
- tests/consumer-typecheck/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
41 | 41 | | |
42 | 42 | | |
43 | 43 | | |
| 44 | + | |
44 | 45 | | |
45 | 46 | | |
46 | 47 | | |
| |||
366 | 367 | | |
367 | 368 | | |
368 | 369 | | |
| 370 | + | |
369 | 371 | | |
370 | 372 | | |
371 | 373 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
355 | 355 | | |
356 | 356 | | |
357 | 357 | | |
| 358 | + | |
| 359 | + | |
358 | 360 | | |
359 | 361 | | |
360 | 362 | | |
| |||
989 | 991 | | |
990 | 992 | | |
991 | 993 | | |
| 994 | + | |
| 995 | + | |
| 996 | + | |
| 997 | + | |
| 998 | + | |
| 999 | + | |
| 1000 | + | |
992 | 1001 | | |
993 | 1002 | | |
994 | 1003 | | |
| |||
1018 | 1027 | | |
1019 | 1028 | | |
1020 | 1029 | | |
1021 | | - | |
| 1030 | + | |
1022 | 1031 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1895 | 1895 | | |
1896 | 1896 | | |
1897 | 1897 | | |
| 1898 | + | |
| 1899 | + | |
| 1900 | + | |
| 1901 | + | |
| 1902 | + | |
1898 | 1903 | | |
1899 | 1904 | | |
1900 | 1905 | | |
| |||
4116 | 4121 | | |
4117 | 4122 | | |
4118 | 4123 | | |
| 4124 | + | |
| 4125 | + | |
| 4126 | + | |
| 4127 | + | |
| 4128 | + | |
4119 | 4129 | | |
4120 | 4130 | | |
4121 | 4131 | | |
| |||
17469 | 17479 | | |
17470 | 17480 | | |
17471 | 17481 | | |
| 17482 | + | |
| 17483 | + | |
| 17484 | + | |
| 17485 | + | |
| 17486 | + | |
| 17487 | + | |
| 17488 | + | |
| 17489 | + | |
| 17490 | + | |
| 17491 | + | |
| 17492 | + | |
| 17493 | + | |
| 17494 | + | |
| 17495 | + | |
| 17496 | + | |
| 17497 | + | |
| 17498 | + | |
| 17499 | + | |
| 17500 | + | |
| 17501 | + | |
| 17502 | + | |
| 17503 | + | |
| 17504 | + | |
| 17505 | + | |
| 17506 | + | |
| 17507 | + | |
| 17508 | + | |
| 17509 | + | |
| 17510 | + | |
| 17511 | + | |
| 17512 | + | |
| 17513 | + | |
| 17514 | + | |
| 17515 | + | |
| 17516 | + | |
17472 | 17517 | | |
17473 | 17518 | | |
17474 | 17519 | | |
| |||
19756 | 19801 | | |
19757 | 19802 | | |
19758 | 19803 | | |
| 19804 | + | |
19759 | 19805 | | |
19760 | 19806 | | |
19761 | 19807 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
27 | 27 | | |
28 | 28 | | |
29 | 29 | | |
30 | | - | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
| 30 | + | |
36 | 31 | | |
37 | 32 | | |
38 | 33 | | |
| |||
118 | 113 | | |
119 | 114 | | |
120 | 115 | | |
121 | | - | |
122 | | - | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
123 | 125 | | |
124 | 126 | | |
125 | 127 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| 52 | + | |
52 | 53 | | |
53 | 54 | | |
54 | 55 | | |
| |||
583 | 584 | | |
584 | 585 | | |
585 | 586 | | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
| 590 | + | |
| 591 | + | |
| 592 | + | |
586 | 593 | | |
587 | 594 | | |
588 | 595 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
581 | 581 | | |
582 | 582 | | |
583 | 583 | | |
| 584 | + | |
584 | 585 | | |
585 | 586 | | |
586 | 587 | | |
| |||
1042 | 1043 | | |
1043 | 1044 | | |
1044 | 1045 | | |
| 1046 | + | |
1045 | 1047 | | |
1046 | 1048 | | |
1047 | 1049 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
31 | 38 | | |
32 | 39 | | |
33 | 40 | | |
34 | 41 | | |
| 42 | + | |
35 | 43 | | |
36 | 44 | | |
37 | 45 | | |
| |||
0 commit comments