Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/remote-caret-follows-moved-text.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@portabletext/editor': patch
---

fix: follow text moved by remote splits, merges, and block reorders when recovering the caret

The caret now follows content a collaborator's split, merge, block reorder, or decorator/annotation edit relocates, instead of collapsing to the edit's boundary. A remote block split moves a caret sitting in the tail into the new block; a remote span or block merge keeps the caret at the same character position instead of snapping to the end of the merged span; a remote block reorder keeps a caret inside the block instead of losing it to the reorder's own removal step. This includes spans a merge renamed to avoid key collisions: the caret follows the rename and rides into the merged block. Edits that don't match one of these shapes are unaffected: the caret falls back to its previous boundary behavior.
56 changes: 56 additions & 0 deletions packages/editor/src/editor/remote-patches.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@ import {pluginWithoutHistory} from '../engine-plugins/engine-plugin.without-hist
import {withoutPatching} from '../engine-plugins/engine-plugin.without-patching'
import {normalize} from '../engine/editor/normalize'
import {withoutNormalizing} from '../engine/editor/without-normalizing'
import {pointEquals} from '../engine/point/point-equals'
import {mapPointThroughSteps, type Step} from '../engine/point/step-mapper'
import {applySelect, resolveSelection} from '../internal-utils/apply-selection'
import {createApplyPatch} from '../internal-utils/applyPatch'
import {debug} from '../internal-utils/debug'
import {interpretTransaction} from '../internal-utils/interpret-transaction'
import {safeStringify} from '../internal-utils/safe-json'
import type {EditorSelection} from '../types/editor'
import type {PortableTextEditorEngine} from '../types/editor-engine'
import type {EditorActor} from './editor-machine'

Expand Down Expand Up @@ -36,6 +41,11 @@ export function setupRemotePatches({
bufferedPatches = []
let changed = false

const preApplySelection = editor.snapshot.context.selection
const steps = preApplySelection
? interpretTransaction(editor.snapshot.context.value, patches)
: null

withRemoteChanges(editor, () => {
withoutNormalizing(editor, () => {
withoutPatching(editor, () => {
Expand All @@ -62,6 +72,11 @@ export function setupRemotePatches({
})
if (changed) {
normalize(editor)

if (preApplySelection && steps) {
recoverSelection(editor, steps, preApplySelection)
}

editor.onChange()
}
})
Expand All @@ -85,3 +100,44 @@ export function setupRemotePatches({
}
})
}

/**
* Remote patches carry state deltas, not position mappings: a collaborator's
* span merge or block merge arrives as delete + insert, so the per-operation
* selection transforms collapse the local caret to a boundary instead of
* following the content. `interpretTransaction` recognizes those
* delete+insert pairs as moves, so mapping the pre-batch selection through
* its steps can recover a caret the per-operation transforms lost.
*/
function recoverSelection(
editor: PortableTextEditorEngine,
steps: Array<Step>,
preApplySelection: NonNullable<EditorSelection>,
): void {
const mappedAnchor = mapPointThroughSteps(steps, preApplySelection.anchor)
const mappedFocus = mapPointThroughSteps(steps, preApplySelection.focus)

if (!mappedAnchor || !mappedFocus) {
return
}

const currentSelection = editor.snapshot.context.selection
const alreadyMatches =
currentSelection !== null &&
pointEquals(mappedAnchor, currentSelection.anchor) &&
pointEquals(mappedFocus, currentSelection.focus)

if (alreadyMatches) {
return
}

const resolved = resolveSelection(editor, {
anchor: mappedAnchor,
focus: mappedFocus,
backward: preApplySelection.backward,
})

if (resolved) {
applySelect(editor, resolved)
}
}
96 changes: 96 additions & 0 deletions packages/editor/src/engine/point/step-mapper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,102 @@ describe(mapPointThroughStep.name, () => {
})
})

describe('move.text', () => {
test('maps a point at the start of the moved range to the destination start', () => {
const point = {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 4,
}
const step: Step = {
type: 'move.text',
from: {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 4,
length: 7,
},
to: {path: [{_key: 'b2'}, 'children', {_key: 's2'}], offset: 0},
}
expect(mapPointThroughStep(step, point)).toEqual({
path: [{_key: 'b2'}, 'children', {_key: 's2'}],
offset: 0,
})
})

test('maps a point at the end of the moved range to the destination end', () => {
const point = {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 11,
}
const step: Step = {
type: 'move.text',
from: {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 4,
length: 7,
},
to: {path: [{_key: 'b2'}, 'children', {_key: 's2'}], offset: 0},
}
expect(mapPointThroughStep(step, point)).toEqual({
path: [{_key: 'b2'}, 'children', {_key: 's2'}],
offset: 7,
})
})

test('is a no-op when the offset sits outside the moved range', () => {
const point = {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 3,
}
const step: Step = {
type: 'move.text',
from: {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 4,
length: 7,
},
to: {path: [{_key: 'b2'}, 'children', {_key: 's2'}], offset: 0},
}
expect(mapPointThroughStep(step, point)).toBe(point)
})

test('is a no-op on a different path', () => {
const point = {
path: [{_key: 'b1'}, 'children', {_key: 's9'}],
offset: 6,
}
const step: Step = {
type: 'move.text',
from: {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 4,
length: 7,
},
to: {path: [{_key: 'b2'}, 'children', {_key: 's2'}], offset: 0},
}
expect(mapPointThroughStep(step, point)).toBe(point)
})

test('offsets into the destination range by an offset target other than zero', () => {
const point = {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 6,
}
const step: Step = {
type: 'move.text',
from: {
path: [{_key: 'b1'}, 'children', {_key: 's1'}],
offset: 4,
length: 7,
},
to: {path: [{_key: 'b2'}, 'children', {_key: 's2'}], offset: 5},
}
expect(mapPointThroughStep(step, point)).toEqual({
path: [{_key: 'b2'}, 'children', {_key: 's2'}],
offset: 7,
})
})
})

describe('rekey', () => {
test('substitutes the old key with the new key at the segment directly under the step path', () => {
const point = {
Expand Down
45 changes: 45 additions & 0 deletions packages/editor/src/engine/point/step-mapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,23 @@ export type ReplaceChildrenStep = {
newChildren: Array<unknown>
}

/**
* `from` is both-ends inclusive: an offset landing exactly on the start or
* the end of the moved range still maps through to `to`, the same as
* `remove.text` treats its own start boundary.
*/
export type MoveTextStep = {
type: 'move.text'
from: {path: Path; offset: number; length: number}
to: {path: Path; offset: number}
}

export type MoveNodeStep = {
type: 'move.node'
from: Path
to: Path
}

export type Step =
| InsertTextStep
| RemoveTextStep
Expand All @@ -70,6 +87,8 @@ export type Step =
| UnsetTextStep
| RekeyStep
| ReplaceChildrenStep
| MoveTextStep
| MoveNodeStep

/**
* Map a point through one step. Returns the same `point` reference when the
Expand Down Expand Up @@ -177,6 +196,32 @@ export function mapPointThroughStep(

return point
}

case 'move.text': {
if (
pathEquals(step.from.path, point.path) &&
step.from.offset <= point.offset &&
point.offset <= step.from.offset + step.from.length
) {
return {
path: step.to.path,
offset: step.to.offset + (point.offset - step.from.offset),
}
}

return point
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move-text skips later source carets

Medium Severity

move.text remaps only offsets inside from. Carets after that range are left unshifted, unlike remove.text. assembleSteps drops the paired remove.text, so a non-suffix move (a mid-span delete paired with a matching node insert) leaves those carets at offsets past the remaining text.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 79f5dc7. Configure here.


case 'move.node': {
if (pathContains(step.from, point.path)) {
return {
path: [...step.to, ...point.path.slice(step.from.length)],
offset: point.offset,
}
}

return point
}
}
}

Expand Down
Loading
Loading