fix(model): replace inline fragment data on tool re-application#169
Open
gohabereg wants to merge 2 commits into
Open
fix(model): replace inline fragment data on tool re-application#169gohabereg wants to merge 2 commits into
gohabereg wants to merge 2 commits into
Conversation
Re-applying a data-carrying inline tool (e.g. link) over a range already formatted with that same tool used to be a no-op, so editing a link's URL silently kept the old href. FormattingInlineNode.format() short-circuited whenever the tool matched, never comparing or replacing data, and the inline-link tool worked around it by sending FormattingAction.None (which SelectionManager ignores) so the new value never reached the model. Make inline-fragment data handling data-aware: - Add isSameInlineData(), a deep-equality helper (missing data == empty). - FormattingInlineNode.format(): same tool + equal data is a no-op; same tool + different data replaces the data, splitting the node so only the re-applied range changes (reusing the unformat split pattern). - Store data in a private #data field with a public getter so it can be replaced internally while staying read-only to callers. - FormattingInlineNode.isEqual()/mergeWith() compare data so normalization no longer merges same-tool fragments whose data differs. - ParentInlineNode.getFragments() only coalesces adjacent fragments when their data matches, so the read/serialization path keeps replaced data. - inline-link now always sends FormattingAction.Format on confirm. Add an optional InlineTool.isSameData() seam in the SDK as a documented extension point; wiring a per-tool comparator through the model is deferred (tracked by the wire-inline-tool-data-comparator change stub). Tests: integration coverage for replace / no-op / partial-split / no-merge, plus a focused isSameInlineData unit spec. Planned and recorded via OpenSpec (replace-inline-fragment-data-on-reformat, now archived). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Unit Tests
Mutation Tests
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes inline-fragment formatting data-aware so re-applying the same inline tool (e.g. link) can replace existing fragment data instead of being treated as a no-op, and updates the link tool to always send FormattingAction.Format so edited URLs reach the model.
Changes:
- Add default deep-equality comparison for inline tool data (
isSameInlineData) and use it to drive no-op vs data-replace behavior and merge/normalization decisions. - Update
FormattingInlineNodeto store data privately and support partial-range data replacement via splits. - Update
inline-linkto always applyFormattingAction.Format, and add integration/unit tests + OpenSpec updates (plus a deferred-change stub for per-tool comparators).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tools/inline-link/src/index.ts | Always re-applies link as Format so href edits propagate to the model. |
| packages/sdk/src/entities/InlineTool.ts | Adds isSameData comparator seam (currently documented as deferred). |
| packages/model/src/utils/isSameInlineData.ts | Introduces model-side deep equality helper (treats missing data as {}). |
| packages/model/src/utils/isSameInlineData.spec.ts | Unit tests for isSameInlineData behavior (undefined/empty/deep compare/arrays). |
| packages/model/src/utils/index.ts | Re-exports isSameInlineData from utils index. |
| packages/model/src/entities/inline-fragments/specs/InlineTree.integration.spec.ts | Integration tests covering replace/no-op/partial-split/no-merge for data-carrying tools. |
| packages/model/src/entities/inline-fragments/ParentInlineNode/index.ts | Makes fragment coalescing in getFragments() data-aware. |
| packages/model/src/entities/inline-fragments/FormattingInlineNode/index.ts | Implements data-aware same-tool re-apply and prevents merging fragments with differing data. |
| openspec/specs/tools/spec.md | Updates tool requirements to specify link confirm always uses FormattingAction.Format. |
| openspec/specs/model/spec.md | Updates model requirements/scenarios around data-aware re-apply and fragment equality. |
| openspec/changes/wire-inline-tool-data-comparator/proposal.md | Adds stub proposal to later wire per-tool comparators through core→model. |
| openspec/changes/wire-inline-tool-data-comparator/.openspec.yaml | Metadata for the deferred comparator-wiring change stub. |
| openspec/changes/archive/2026-07-17-replace-inline-fragment-data-on-reformat/tasks.md | Archives completed tasks list for this change. |
| openspec/changes/archive/2026-07-17-replace-inline-fragment-data-on-reformat/specs/tools/spec.md | Archived delta spec for tools requirement changes. |
| openspec/changes/archive/2026-07-17-replace-inline-fragment-data-on-reformat/specs/model/spec.md | Archived delta spec for model requirement changes. |
| openspec/changes/archive/2026-07-17-replace-inline-fragment-data-on-reformat/proposal.md | Archived proposal describing motivation and scope. |
| openspec/changes/archive/2026-07-17-replace-inline-fragment-data-on-reformat/design.md | Archived design decisions for data-aware re-apply and comparator seam. |
| openspec/changes/archive/2026-07-17-replace-inline-fragment-data-on-reformat/.openspec.yaml | Metadata for the archived change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+91
to
+96
| /** | ||
| * Optionally overrides how the model decides whether two data objects of this tool are equal. | ||
| * | ||
| * The model uses this to tell a no-op re-apply from a data replacement, and to decide whether | ||
| * two adjacent same-tool fragments may be merged. When omitted, the model falls back to a deep | ||
| * structural comparison. |
|
|
||
| ### Requirement: Inline text tree | ||
| The system SHALL provide `TextNode` (extending `ParentInlineNode`) as the inline-fragment tree root for a text data node, supporting text insert/remove, format/unformat over ranges, fragment listing, and index/range validation with auto-merging/normalization of children. | ||
| The system SHALL provide `TextNode` (extending `ParentInlineNode`) as the inline-fragment tree root for a text data node, supporting text insert/remove, format/unformat over ranges, fragment listing, and index/range validation with auto-merging/normalization of children. When a data-carrying inline tool is re-applied over a range already formatted with that same tool, the system SHALL replace the affected fragment's data with the newly supplied data instead of ignoring it. Two fragments of the same tool SHALL be considered equal (for normalization/merging) only when their data is also equal; data equality SHALL default to a deep structural comparison, which an inline tool MAY override with its own comparator. |
|
|
||
| #### Scenario: Re-applying the same tool with identical data | ||
| - **GIVEN** a fragment already formatted with a data-carrying tool | ||
| - **WHEN** `format` is applied over that range with the same tool and data that is equal under the effective comparator |
Add the two cases missed by the initial tests, restoring full coverage on the changed files: - FormattingInlineNode: re-applying a tool over a prefix range (start === 0, end < length) exercises the else branch of the data-replace split. - isSameInlineData: array vs non-array values return not-equal. Co-Authored-By: Claude Opus 4.8 <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.
Re-applying a data-carrying inline tool (e.g. link) over a range already formatted with that same tool used to be a no-op, so editing a link's URL silently kept the old href. FormattingInlineNode.format() short-circuited whenever the tool matched, never comparing or replacing data, and the inline-link tool worked around it by sending FormattingAction.None (which SelectionManager ignores) so the new value never reached the model.
Make inline-fragment data handling data-aware:
Add an optional InlineTool.isSameData() seam in the SDK as a documented extension point; wiring a per-tool comparator through the model is deferred (tracked by the wire-inline-tool-data-comparator change stub).
Tests: integration coverage for replace / no-op / partial-split / no-merge, plus a focused isSameInlineData unit spec.
Planned and recorded via OpenSpec
(replace-inline-fragment-data-on-reformat, now archived).