Refactor Index class#166
Open
gohabereg wants to merge 4 commits into
Open
Conversation
Unit Tests
Mutation Tests
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the document model “Index” concept from a single mutable class + IndexBuilder into a set of concrete, immutable index types (BlockIndex, TextIndex, etc.) with a compact JSON wire format, and updates the rest of the codebase/tests/docs to use the new API.
Changes:
- Introduce
IndexBase+ concrete index subclasses and a new JSON serialization/deserialization format (Index.parse()/index.serialize()), removingIndexBuilder. - Update model/core/collaboration/dom-adapters/UI to use
instanceof/IndexKindand new factory methods likeIndex.text()/Index.block(). - Add/refresh documentation for the new index model and serialization format.
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/InlineToolbar/InlineToolbar.ts | Switches selection/index handling to TextIndex segments. |
| packages/sdk/src/index.ts | Re-exports new index types and removes IndexBuilder export. |
| packages/sdk/src/entities/BlockToolAdapter.ts | Adapts model update indexing to new index types. |
| packages/playground/src/utils/isObject.ts | Minor doc comment tweak. |
| packages/playground/src/components/Value.vue | Adds prop documentation. |
| packages/playground/src/components/Node.vue | Formatting + richer JSDoc. |
| packages/playground/src/components/Indent.vue | Script formatting / semicolons cleanup. |
| packages/playground/src/components/CaretIndex.vue | Adds documentation around caret index map/props. |
| packages/playground/src/App.vue | Minor doc/comment cleanup. |
| packages/ot-server/src/DocumentManager.spec.ts | Updates tests to use Index.parse() and JSON wire indices. |
| packages/model/src/entities/ValueNode/index.ts | Replaces IndexBuilder usage with PartialIndex for events. |
| packages/model/src/entities/inline-fragments/ParentInlineNode/index.ts | Replaces IndexBuilder with PartialIndex for text events. |
| packages/model/src/entities/EditorDocument/index.ts | Switches to concrete index classes and resolves PartialIndex during bubbling. |
| packages/model/src/entities/EditorDocument/EditorDocument.spec.ts | Updates tests to new Index.* factories and PartialIndex. |
| packages/model/src/entities/BlockTune/index.ts | Replaces IndexBuilder with PartialIndex for tune events. |
| packages/model/src/entities/BlockNode/index.ts | Replaces IndexBuilder with PartialIndex for bubbled events. |
| packages/model/src/entities/BlockNode/BlockNode.spec.ts | Updates tests to new index API. |
| packages/model/src/EditorJSModel.ts | Updates event/caret handling to use new index immutability (with*). |
| packages/model/src/EditorJSModel.spec.ts | Updates caret/model tests to new index API. |
| packages/model/src/CaretManagement/CaretManager.spec.ts | Updates tests to use Index.block() instead of new Index(). |
| packages/model/src/CaretManagement/Caret/Caret.spec.ts | Updates tests to use Index.block() instead of new Index(). |
| packages/model-types/src/indexing.ts | Removes DocumentIndex alias type. |
| packages/model-types/src/Index/TuneIndex.ts | Adds concrete TuneIndex type. |
| packages/model-types/src/Index/TextIndex.ts | Adds concrete TextIndex type with composite support. |
| packages/model-types/src/Index/PropertyIndex.ts | Adds concrete PropertyIndex type. |
| packages/model-types/src/Index/PartialIndex.ts | Adds PartialIndex used for internal event bubbling before full context exists. |
| packages/model-types/src/Index/IndexBuilder.ts | Removes legacy builder implementation. |
| packages/model-types/src/Index/IndexBuilder.spec.ts | Removes legacy builder tests. |
| packages/model-types/src/Index/IndexBase.ts | Adds IndexBase + IndexKind discriminant. |
| packages/model-types/src/Index/index.ts | Refactors Index into factory + parse helpers and exports concrete classes. |
| packages/model-types/src/Index/Index.spec.ts | Rewrites index tests for new class hierarchy + JSON wire format. |
| packages/model-types/src/Index/DocumentIndex.ts | Adds concrete DocumentIndex type. |
| packages/model-types/src/Index/DataIndex.ts | Adds concrete DataIndex type. |
| packages/model-types/src/Index/BlockIndex.ts | Adds concrete BlockIndex type. |
| packages/model-types/src/index.ts | Updates public exports to include new index hierarchy. |
| packages/model-types/src/events/ValueModifiedEvent.ts | Types index payload as DataIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/TuneModifiedEvent.ts | Types index payload as TuneIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/TextUnformattedEvent.ts | Types index payload as TextIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/TextRemovedEvent.ts | Types index payload as TextIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/TextFormattedEvent.ts | Types index payload as TextIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/TextAddedEvent.ts | Types index payload as TextIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/PropertyModifiedEvent.ts | Types index payload as PropertyIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/DataNodeRemovedEvent.ts | Types index payload as DataIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/DataNodeAddedEvent.ts | Types index payload as DataIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/BlockRemovedEvent.ts | Types index payload as BlockIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/events/BlockAddedEvent.ts | Types index payload as BlockIndex (+ supports PartialIndex in ctor). |
| packages/model-types/src/BaseDocumentEvent.ts | Generalizes event payload typing over IndexBase and allows PartialIndex ctor arg. |
| packages/dom-adapters/src/CaretAdapter/index.ts | Updates DOM selection restore/sort logic to new TextIndex composite API. |
| packages/dom-adapters/src/BlockToolAdapter/index.ts | Updates caret/index updates and narrows text events to TextIndex. |
| packages/core/src/index.ts | Reorders UndoRedoManager init relative to plugins. |
| packages/core/src/components/UndoRedoManager.ts | Updates text index batching checks to instanceof TextIndex. |
| packages/core/src/components/UndoRedoManager.spec.ts | Updates mocks to model new instanceof TextIndex behavior. |
| packages/core/src/components/SelectionManager.ts | Updates caret index handling to new index classes. |
| packages/core/src/components/SelectionManager.spec.ts | Updates mocks/tests to align with new index classes. |
| packages/core/src/components/BlockRenderer.ts | Uses event.detail.index.blockIndex directly (new BlockIndex). |
| packages/core/src/components/BlockRenderer.spec.ts | Updates typings in tests for BlockIndex. |
| packages/core/src/components/BlockManager.ts | Narrows caret index handling via TextIndex. |
| packages/core/src/components/BlockManager.spec.ts | Updates mocks/tests for TextIndex caret indices. |
| packages/collaboration-manager/src/UndoRedoManager.spec.ts | Updates tests to new index factories and casts. |
| packages/collaboration-manager/src/OperationsTransformer.ts | Refactors transformation logic to concrete index classes (BlockIndex, TextIndex, DataIndex). |
| packages/collaboration-manager/src/OperationsTransformer.spec.ts | Updates transformer tests to new index factories and casts. |
| packages/collaboration-manager/src/Operation.ts | Uses Index.parse() / clone() instead of IndexBuilder; adjusts range logic. |
| packages/collaboration-manager/src/Operation.spec.ts | Updates operation tests to new index factories and types. |
| packages/collaboration-manager/src/CollaborationManager.spec.ts | Updates collaboration manager tests to new index factories. |
| packages/collaboration-manager/src/client/OTClient.spec.ts | Updates OT client tests to new index factories. |
| packages/collaboration-manager/src/BatchedOperation.ts | Updates batching guard to instanceof TextIndex. |
| packages/collaboration-manager/src/BatchedOperation.spec.ts | Updates batching tests to new index factories/casts. |
| docs/README.md | Updates overview and links to new serialization doc. |
| docs/model.md | Updates model docs to reflect new index hierarchy and construction. |
| docs/index-serialization.md | Adds formal documentation for JSON index wire format. |
| docs/diagrams/architecture-overview.mmd | Updates architecture diagram to reflect new Index API. |
Comments suppressed due to low confidence (1)
packages/sdk/src/entities/BlockToolAdapter.ts:9
- These imports are only used in type positions. Importing them as values can break builds when
verbatimModuleSyntax/type-only import rules are enabled, and it adds unnecessary runtime dependencies. Switch back toimport type(and keep only the event classes/constants as value imports).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public insertData(index: Index, data: string | BlockNodeInit[] | BlockNodeDataSerializedValue): void { | ||
| switch (true) { | ||
| case index.isTextIndex: | ||
| case index instanceof TextIndex: |
| public removeData(index: Index, data: string | BlockNodeInit[] | BlockNodeDataSerializedValue): void { | ||
| switch (true) { | ||
| case index.isTextIndex: | ||
| case index instanceof TextIndex: |
| /** | ||
| * @todo implement other actions | ||
| */ | ||
| if (index instanceof TextIndex) { |
| * Also, we need to sort inputs inside first/last block by document order to restore selection | ||
| */ | ||
| const segments = caretIndex.getTextSegments(); | ||
| const segments = (caretIndex as TextIndex).getTextSegments(); |
neSpecc
reviewed
Jul 17, 2026
Comment on lines
+278
to
+283
| const op1 = new Operation(OperationType.Insert, Index.text([{ blockIndex: 0, | ||
| dataKey: createDataKey('text'), | ||
| textRange: [0, 0] }]), { payload: 'a' }, userId); | ||
| const op2 = new Operation(OperationType.Insert, Index.text([{ blockIndex: 0, | ||
| dataKey: createDataKey('text'), | ||
| textRange: [1, 1] }]), { payload: 'b' }, userId); |
Contributor
There was a problem hiding this comment.
bad indentations, hard to read.
My suggestion:
const op1 = new Operation(
OperationType.Insert,
Index.text([{
blockIndex: 0,
dataKey: createDataKey('text'),
textRange: [0, 0]
}]),
{
payload: 'a'
},
userId
);
Comment on lines
+326
to
+328
| const index = Index.text([{ blockIndex: 0, | ||
| dataKey: createDataKey('text'), | ||
| textRange: [0, 3] }]); |
Contributor
There was a problem hiding this comment.
same here and on other places
Copilot flagged instanceof TextIndex checks in insertData/removeData/modifyData and a TextIndex cast in SelectionManager that could throw on composite/non-TextIndex values. Verified none are reachable with current call paths (index always comes from PartialIndex.resolve(), caret.index is always null or TextIndex), so leaving TODOs to guard explicitly if that ever changes rather than adding dead code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reformat cramped multi-line Index.text(...) and Operation(...) constructions per review feedback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See docs for the changes description