Skip to content

✨ Add diff to compute an op between two docs - #18

Merged
dodoarg merged 22 commits into
masterfrom
expose-diff
Aug 18, 2026
Merged

✨ Add diff to compute an op between two docs#18
dodoarg merged 22 commits into
masterfrom
expose-diff

Conversation

@dodoarg

@dodoarg dodoarg commented Jul 21, 2026

Copy link
Copy Markdown

What

Adds type.diff(before, after) — returns a json0 op that transforms before into after.

  • Objects: added/removed/changed keys → oi/od; nested objects recurse along the path.

  • Strings: delegated to the owning subtype via isOfType, not diffed inline; text0 owns strings and now computes its string diff with fast-diff (the same algorithm quill-delta uses for rich-text), so json0 and rich-text produce consistent string ops. A string change becomes a text0 subtype op — composing/transforming cleanly instead of a whole-value od/oi.

  • Arrays — structural diff via arraydiff, emitting li/ld/lm. Unchanged elements are retained, so concurrent edits to other elements survive; a relocated element stays an lm (keeps identity). An element changed in place (a single remove+insert at the same index, both the same container kind) recurses into diffValue — so a changed field becomes a deep oi/od or a nested subtype op, not a whole-element ld+li. Primitives, object↔array kind switches, and batch replaces stay literal ld+li. Moves pick the smaller side (a small block to a far end is a couple of lms, not O(n)).

  • Equal values → no-op.

Why

(specifics are TBD but we are gonna need this anyway)
@reedsy/reedsy-sharedb's migration middleware needs to rebase a schema-stale op onto a migrated document shape (diff(applied, migratedApplied)) and $fixup the submitted op with the change, so a stale edit to a reshaped field still lands and all clients converge. json0 had apply/transform/invert/compose but no diff.

New dependency: arraydiff

We need a move-aware structural array diff — one that emits lm for a relocated element, not just insert/delete — otherwise a reordered element becomes ld+li and concurrent edits to it are dropped on rebase.
Other weighted options:

  • jsondiffpatch also detects moves, but it's heavyweight (~163KB, +2 transitive deps, needs an objectHash, emits a nested delta we'd have to adapt).
  • the maintained lightweight options (fast-array-diff, fast-myers-diff, jsdiff) are insert/delete only.

arraydiff is genuinely the one library whose op vocabulary (insert/remove/move) maps almost 1:1 onto li/ld/lm; it has zero sub-deps and is the same diff ShareDB itself has used in production for years. Its one real drawback is age (last release 2016), and it is effectively unmaintained. This sounds acceptable because it's genuinely a small pure function.

I also considered:

  • hand-rolling an array diff algorithm instead, but that would mean owning the move-detection heuristic ourselves, arguably more risk than reusing a battle-tested one
  • vendoring the source arraydiff into the repo, but with no local modifications that only adds maintenance burden over a pinned dependency.

Known limitation

Cross-array moves (an element relocating from one array into another) can't be expressed as lm — json0 has no cross-path move — so they diff as ld+li. Concurrent edits to such an element don't carry across. (Parked for discussion.)

Tests

#diff() is a new top-level block, sub-grouped (objects / subtypes / lists / scalars): subtype dispatch (delegates, empty-op drop, one-side-owned fallback), list insert/delete/replace/relocate/smaller-side move, nested recursion, plus a 2000-case round-trip-via-apply fuzzer over random array reshapes. Every #diff() case now round-trips through apply() as a further sanity check.

Produces a json0 op transforming one document into another. Strings are
diffed to a single contiguous change (sd/si) so the result composes and
transforms cleanly past concurrent edits instead of replacing whole values.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 21, 2026 09:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a diff(before, after) helper to the json0 type to generate an operation that transforms one document snapshot into another, with special handling for strings to produce a single contiguous sd/si change (better behavior under concurrent edits).

Changes:

  • Added json.diff() implementation (recursive object diff + contiguous string diff).
  • Added CoffeeScript tests covering object key changes, value changes, string diffs, nesting, and apply round-trips.
  • Extended TypeScript declarations to expose type.diff(before, after).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

File Description
lib/json0.js Introduces json.diff() plus internal helpers for value/object/string diffing.
test/json0.coffee Adds a #diff() test suite verifying expected op shapes and apply round-trips.
lib/index.d.ts Exposes diff(before, after) on the exported type in TS typings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/json0.js Outdated
Comment thread lib/json0.js
@dodoarg
dodoarg marked this pull request as draft July 21, 2026 09:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

Comment thread lib/json0.js
Comment thread lib/json0.js
Comment thread test/json0.coffee Outdated
@dodoarg dodoarg changed the title ➕ Expose diff to compute an op between two docs ✨ Add diff to compute an op between two docs to exposed API Jul 22, 2026
@dodoarg dodoarg changed the title ✨ Add diff to compute an op between two docs to exposed API ✨ Add diff to compute an op between two docs Jul 22, 2026
@dodoarg
dodoarg requested a review from alecgibson July 22, 2026 15:25
@dodoarg
dodoarg marked this pull request as ready for review July 27, 2026 12:47
Comment thread lib/index.d.ts
Comment thread lib/json0.js
Comment thread lib/json0.js Outdated
Comment thread lib/json0.js
@dodoarg
dodoarg requested a review from alecgibson July 29, 2026 11:35
Comment thread lib/index.d.ts Outdated
// such a value by shape, since a raw value carries no type tag.
export interface DiffableSubtype {
diff(before: any, after: any): any;
isDoc(value: any): boolean;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This does make me wonder if we should make some change to track this "properly" as part of the document structure. We already have to hack around this in reedsy-sharedb with some imperfect heuristics. Definitely out of scope of this change, though.

Comment thread lib/json0.js Outdated

for (var name in subtypes) {
var st = subtypes[name]
if (typeof st.diff === 'function' && st.isDoc && st.isDoc(before) && st.isDoc(after)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. This is begging for a isTypeDiffable() helper
  2. Should use a typeof check on isDoc too
  3. Should we skip the isDoc() check if before or after are undefined? I can't remember off the top of my head how apply() works: basically if I'm allowed to do something like {p: ['does-not-exist', 0], si: 'foo'}, then we should allow an undefined before. If I'm not, then I guess we shouldn't (and now I write it down, I actually assume it's not allowed) — at any rate, please make sure this is recorded in tests

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok! Confirmed apply throws on si-ing a field that doesn't exist hence isDoc should reject undefined. Adding contract tests on this behavior

Comment thread lib/json0.js Outdated
var op = [];

arrayDiff(before, after, deepEqual).forEach(function(diff) {
var i;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this is declared at the top of this scope instead of inline?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

not really a strong reason just matching ES5 vibes from elsewhere in the file/library but I saw you've left some traces of more modern syntax, I can just inline let i = 0 instead

Comment thread lib/json0.js Outdated
Comment on lines +437 to +438
op.push({p: path.concat(diff.index), ld: work[diff.index]});
work.splice(diff.index, 1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't see a test for the multiple deletion case (I couldn't remember off the top of my head if this code should work), which I'd expect to see.

eg I checked this (green) test locally:

it 'handles list deletions of multiple elements', ->
  before = [1, 2, 3, 4]
  after = [1, 4]
  diff = type.diff before, after
  assert.deepEqual after, type.apply before, diff

Comment thread lib/json0.js Outdated
} else {
// arrayDiff always frames a reorder as moving its block left, but the same
// reshape is moving the displaced gap right. Emit whichever needs fewer lms.
if (diff.howMany <= diff.from - diff.to) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Am I right in thinking howMany is the number of elements being moved from base index from to new base index to? So [1, 2, 3, 4] -> [3, 4, 1, 2] is something like {howMany: 2, from: 0, to: 2}? And then this logic is handling the fact that this needs to map to 2 separate lm ops, and you're doing it in the way that results in fewest by comparing the length of the subarray vs the size of the move?

If I've understood that all correctly, it wasn't immediately obvious from the code comment 😅

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yea you got it exactly right, it's a small optimization that can be nice in long strings. I'm making the comment a little more explicit

Comment thread lib/text0.js
};

/** Compute the op that transforms `before` into `after` */
text.diff = function(before, after) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I applaud implementing this yourself, but I do wonder if this is a weirdly non-trivial problem, sufficiently so that we should use a library for this as well? Even Delta outsources diffing, and I feel like if we used the same library, you'd at least get identical string diff behaviour with rich-text, which is probably a good thing (or at least consistent).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok I see. This was actually just the code from older impl. of json0.diff (before subtype delegation) but we might as well not write new potentially wrong/inconsistent code if we can just further delegate to fast-diff :)

Comment thread lib/text0.js Outdated
return op
}

text.isDoc = function(snapshot) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if this will read better? At the very least I wouldn't use the word Doc, which has a very specific meaning in sharedb (it's a particular class).

Suggested change
text.isDoc = function(snapshot) {
text.isOfType = function(snapshot) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I admittedly couldn't make up my mind on how to name this. I also thought of something like owns as in e.g. "text0.owns(snapshot)" but I'll go by your suggestion

Comment thread test/json0.coffee


describe '#diff()', ->
roundTrips = (before, after) ->

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel like it's worth setting up the tests to round trip literally every test. It's cheap to run, and adds sense checks on all the edge cases you've thought to test, which can only be a good thing.

Comment thread test/json0.coffee
it 'diffs a renamed key with od + oi', ->
assert.deepEqual [{p: ['title'], od: 'x'}, {p: ['heading'], oi: 'x'}], type.diff {title: 'x'}, {heading: 'x'}

it 'diffs nested objects along their path', ->

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we also add a test for type.diff {}, {a: {b: 2}}

Comment thread test/json0.coffee Outdated
type.diff {x: [9, 1, 2, 3, 4]}, {x: [1, 2, 3, 4, 9]}

it 'diffs a changed object element with ld + li', ->
assert.deepEqual [{p: ['x', 1], ld: {v: 1}}, {p: ['x', 1], li: {v: 2}}],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I'd expect this behaviour. What's the motivation of this over a deeply addressed oi/od?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah this comes from diffArray delegating to arraydiff which just works on a deepEqual basis and emits remove/insert/move diffs with no concept of mutated-in-place elements.

I thought this would be too complicated but actually here's the rule I came up with:

for each remove@i {X} immediately followed by insert@i {Y} of a single element where X and Y are the same type of container (array/object) recurse into diffValue(X, Y) instead of emitting li/ld pair

added contract tests for this behavior (this one in particular now emits a od + oi at the nested path

Comment thread lib/json0.js
return diffObject(path, before, after);
}

return [{p: path, od: before, oi: after}];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

JSON0 supports more operations.

{p:[path], na:x} - adds x to the number at [path].
{p:[path,offset], si:s} - inserts the string s at offset offset into the string at [path] (uses subtypes internally).
{p:[path,offset], sd:s} - deletes the string s at offset offset from the string at [path] (uses subtypes internally).

I wonder if we should utilise them, but maybe it is safer to just do what you did which is

[{p: path, od: before, oi: after}];

@dodoarg dodoarg Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Interesting point.
About si/sd:
these are already taken care of by the subtyping machinery. We delegate diffing strings to text0 and we get {t: 'text0', o: [...]} back with si/sd inside.
The only caveat is consumers need to register text0 as a subtype or isTypeDiffable would return false and the json0.diff would fall back to coarse od/oi. There's an argument where there is some kind of "specialized fallback" for diffing strings when consumers haven't registered text0 that would handle this case instead of the generic fallback, but I don't think it's worth implementing.

About na, this is a bit trickier.
It is true that no before/after pair emits na ops, and after investigating it turns out it is arguably correct. Essentially the core reason is if you're given a before and an after you can't tell the difference between "set a given number to X" and "increment a given number by X". The only way is to know the intent of the op author, but here we only have the starting and final state of the doc.
So if we emitted na ops as diff output we'd essentially be fabricating an incrementing intent. This could be dangerous in the context of rebasing stale ops (which is the entire reason we need to expose this diff machinery for) because two na ops transform additively via OT and that causes a double increment where there might not have been one.
Think:

  • doc is {n: 10}
  • Alice submits something like {p: ['n'], od: 10, oi: 12} (sets n to 12)
  • Bob concurrently submits {p: ['n'], na: 5} (increments n by 5)
  • a new hypothetical json0.diff branch emits {p: ['n'], na: 2} for Alice's op
  • that is transformed over Bob's op, the two nas are applied consecutively and finally doc is {n: 17}

While the od/oi fallback would result in last-write-wins over na i.e. {n: 12} which is arguably more reasonable as it's what currently happens for concurrent conflicting od/oi vs na

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That are very good points!

Comment thread lib/json0.js
@@ -1,4 +1,5 @@
var deepEqual = require('fast-deep-equal');
var arrayDiff = require('arraydiff');

@dawidreedsy dawidreedsy Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Any reason we used that package that hasn't been updated for last 11 years, there is some other one updated 3 years ago https://www.npmjs.com/package/fast-array-diff

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yea there is actually an explanation of this (and more additional approaches considered for diffing arrays) in the PR description. Basically the one you linked is lacking an equivalent of lm. Which is bad mostly because ld+li kills concurrent edits to the same moved element (and also because any plain reorder results in way larger ops)

Comment thread test/json0.coffee
Comment on lines +487 to +488
diffsTo {x: [{a: 1}, {v: 1}]}, {x: [{a: 1}, {v: 2}]},
[{p: ['x', 1, 'v'], od: 1, oi: 2}]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can we add tests like this:

        {
          x: [
            {a: 1}, 
            {v: 1}
          ]
        }, 
        {
          x: [
            {a: 1}, 
            {b: 1}, 
            {v: 1}
          ]
        },
        {
          x: [
            {a: 1}, 
            {v: 1}
          ]
        }, 
        {
          x: [
            {a: 1}, 
            {b: 1}, 
            {v: 2}
          ]
        },
        {
          x: [
            {a: 1}, 
            {b: 1}, 
            {v: 1}
          ]
        }, 
        {
          x: [
            {a: 1}, 
            {v: 1}
          ]
        },
        {
          x: [
            {a: 1}, 
            {b: 1}, 
            {v: 1}
          ]
        }, 
        {
          x: [
            {a: 1}, 
            {v: 2}
          ]
        },
        {
          x: [
            {a: 1}, 
            {v: 1}
          ]
        },
        {
          x: [
            {a: 1}, 
            {b: 1}, 
            {c: 1}, 
            {v: 1}
          ]
        }, 
        {
          x: []
        },
        {
          x: []
        }, 
        {
          x: {}
        },
        {
          x: null
        }, 

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok I've added these.
Two are particularly interesting

  • [{a: 1}, {v: 1}] -> [{a: 1}, {b: 1}, {v: 2}] which reads like "edit nested v value to 2 + insert b"
  • [{a: 1}, {b: 1}, {v: 1}] -> [{a: 1}, {v: 2}] which reads like "edit nested v value to 2 + remove b"

With the current algorithm, diffs emit a b insert/delete as expected but treats the v change as a naive ld/li. The reason recursion doesn't happen in this case is the detectSubstitutions rule only captures 1 remove -> 1 insert at the same index, while here we have insert/remove of b which shifts v's index; because v's value also changed it's not a "move" and it comes back out of arraydiff bundled into the same multi-element remove/insert as b . So we have {2x insert, 1x remove} and {2x remove, 1x insert} respectively in these two cases => no "substitution" detected => no recursion via diffValue into {v}.

Now, trying to merge a pair insert/remove to recurse into would mean we'd need to guess which pair exactly. In this case it looks obvious which element was edited and which one inserted/removed, but consider:

before: [{id: 7, v: 1}]
after: [{id: 9, w: 0}, {id: 7, v: 2}]

We could read this as "the second element had 'v' changed", it sounds more reasonable to the reader than "the first element had 'id' changed to 9, v deleted and w added"; but the algorithm needs to consistently make this decision and to do that we would need to come up with some kind of similarity metric.
So for now I documented this behavior around the tests. The current behavior is still correct and round trips, only thing the more comprehensive changes to diffArray would net us is slightly smaller operations in such edge cases, at the risk of mis-pairing unrelated elements/recursing for object-diffing into the wrong fields.

@dawidreedsy dawidreedsy Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah that's why I added 😂 , because of this we may endup with larger ops sequence than needed. I think fun exercise (not for now, but let's create issue) would be to use something like GumTree algorithm to find node matches and find possible smallest diff.

Comment thread lib/text0.js
Comment on lines +128 to +140
fastDiff(before, after).forEach(function(segment) {
var kind = segment[0];
var value = segment[1];
if (kind === fastDiff.EQUAL) {
p += value.length;
} else if (kind === fastDiff.DELETE) {
op.push({p: p, d: value});
} else {
op.push({p: p, i: value});
p += value.length;
}
});
return op;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should we have the same diff for strings in json0?

@dodoarg dodoarg Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So this is the text0 diff; json0 already reuses it via the subtype path, so json0 does get identical string diffing whenever text0 is registered. Back to my point to the other thread we could implement a string "fallback" for when no text0 has been registered on the consumer end so doing it would be "correct" but arguably redundant? Since this is our fork and we know we will register text0 on the sharedb consumer for the rebase machinery (which is probably gonna stay its one consumer)

@dodoarg
dodoarg requested a review from dawidreedsy August 12, 2026 13:44
Comment thread test/json0.coffee

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we should test some edge cases like:

json0.diff(null, null);
json0.diff(null,{});
json0.diff({},null);
json0.diff(1,'a');

which in case of json0 should throw

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added these, all round trip.
The null cases actually are legit json0 apparently (create() returns null); the last one is a bit more suspicious because clearly scalars aren't json0 docs however that one also round trips. Which leads to a question, since you're suggesting diff should throw. Should json0.create and json0.apply throw too when given that sort of input? json0 has no eager root-type validations anywhere currently.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I mean 1 is valid json but i always though json0 requires root to be object...

@dodoarg dodoarg Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yea totally reasonable I think that's a sharedb convention, but json0 itself doesn't enforce anything on the root so we'd be adding new invariants across methods for a reason that is arguably unrelated (adding this diff capability).
Another way of looking at this is if somehow diff is given json0.diff(1,'a') input we are not asserting that 1 is a valid json0 doc per se, only that diff handles scalars at root level reasonably well (it emits plain od/oi which vaguely makes sense)

Comment thread test/text0.coffee
Comment on lines +87 to +94
test '', ''
test '', 'a'
test 'a', ''
test 'a', 'ab'
test 'ab', 'a'
test 'ab', 'ac'
test 'abc', 'ac'
test 'ac', 'abc'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Let's add some more edge cases. Emoji might be tricky as they have 2 bytes.

test '😊a😊', '😊b😊'
test '😊a😊', '😊ac'
test '😊a', '☎️a'
test '😊', '☎️'
test '', '☎️'
test '😊', ''
test 'existing' 'totally changed'
test 'with\nnew line' 'without\tnew tab'
test 'with\nnew line' 'without new line'
test 'with\r\nwindows new line' 'without windows new line'
test 'with\r\nwindows new line' 'with\nlinux new line'
test 'this is something' 'these are something too'
test 'a\u030A' 'å'
test ' a' 'a'
test ' a ' 'a'
test 'a ' 'a'
test 'a' ' a'
test 'a' ' a '
test 'a' 'a '
test 'a' 'a '
test 'a'.repeat(10_000_000) ('b'.repeat(5_000_000) + 'a' + 'b'.repeat(4_999_999)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added these, all cases round trip clean except for the last one which opened some pandora box.
So that is a pathological fastdiff case (million chars strings with little to no overlap) and it scales very poorly - essentially o(n^2). So as is that can't just be added to specs as it takes hours, and still minutes if we scale it down to millions/hundreds of thousands chars.

So I added a guard to the text0.diff: if the residual after stripping common prefix/suffix is longer than a threshold, then just treat it as a plain replacement d/i ops. The threshold is kind of a magic number we can tweak, but 4k combined residuals technically keeps fast-diffing time below 50ms.
I think the most defensible argument here is if the two strings have little to nothing in common a complete replace d/i is reasonable anyway rather than being a degraded fallback. And clearly, it is better than failing/timing out and it still round-trips.
Tests now assert that above the threshold we get the two d+i ops (and the test finishes fast) while below the threshold we get the fine fastdiff char-by-char list of ops.

Things to keep in mind:

  • crucially, this might not matter too much in json0 because the string fields in this case are more likely headers, metadata etc. not huge prose content. Where this might matter way more is Delta.diff and if we agree on this approach I need to open an issue on this repo to mirror the same guard there.
  • this perf. issue technically already exists for consumers of Delta.diff (which uses fastdiff underneath) and never bit us only because current live callers always diff highly overlapping versions of a document (migrations, track-changes). I think the rebase case will arguably be the first potential offender so it warrants this guard.

@dawidreedsy dawidreedsy Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we do some testing what could be treshold here? I also do not like the replacement as it measn we basically on ever keystroke save the whole text twice in sd and si

@dawidreedsy dawidreedsy Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm can we maybe chunk the work? So basically do the diff on each chunk then transform chunks to match indexes. Like this

  1. Split into chunks of same length (only the last X chunks may have different length)
  2. The same length chunk and first chunk with different length just run diff and add to all the indexes (chunk number * chunk length)
  3. if after chunk is empty string just do operation as delete the whole chunk
  4. if before chunk is empty just do operation as insert the whole chunk

We can try to normalize the ops then

@dodoarg dodoarg Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll clarify scope first because that guard would fire only in way more remote scenarios than every keystroke on the live path. Live keystrokes emit si/sd ops from the editor as usual and are not meant to go through diff.

  • this diff will be used by the rebase-correction path for offline stale ops w.r.t server
  • there must be a single op, submitted offline (or the result of multiple ops composed into one by ShareDB), that replaces a huge span with unrelated text
  • the text must really be almost completely unrelated (little to no common prefix/suffix), something caused by "select all chapter and then paste something entirely different over it" while offline and while a migration was deployed (since that's the only path that would trigger the rebase path)

On the chunking, it's an interesting idea to try and salvage the fine fastdiff in all cases, but I don't think it works here. If you have:

before: "AAAA|BBBB|CCCC"   (chunks of 4)
after:  "XAAA|ABBB|BCCC|C"  (prepend X)

fixed-offset chunks don't move when content shifts (like in this case with an insertion), so every pair ends up different, while the actual diff should just be one char {p: 0, i: 'X'}.
I thought maybe this should be XAAAA|BBBB|CCCC chunking so only the first pair of chunks has a diff and the total diff is the correct one, however the problem is we can't pre-align chunks without paying the O(n*d) cost, because knowing how to align essentially is the fastdiff problem we are solving...

I had Claude simulate fastdiff against different text sizes, here's the quadratic progression:

  • residual is the number of chars left to diff against after stripping common prefix/suffix
  • dissimilar is the worst case scenario (before and after have completely unrelated char sets)
  • shuffled-prose is the realistic-ish scenario where the two strings have some nonzero overlap
residual (per string) dissimilar shuffled-prose
500 2ms 2ms
1000 5ms 3.5ms
2000 21ms 14ms
3000 47ms 29ms
4000 83ms 51ms
5000 131ms 79ms
10000 724ms 304ms
20000 2.1s 1.2s
40000 8.5s 4.9s

the guard in the code atm checks the combined residual of the two strings so it's around the 2k-per-string figure in the table above. Technically 4k combined residuals (the number I picked) takes ~20ms in the absolute worst case for fine-diffing through fastdiff

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this diff will be used by the rebase-correction path for offline stale ops w.r.t server

This you cannot guarantee. This is the observation and assumption it won't happen in the future.

fixed-offset chunks don't move when content shifts (like in this case with an insertion), so every pair ends up...

Yeah, it is not normalised op, the op that chunks would give us would be:

chunk 1: {p:0, i: 'X', d: 'A'}
chunk 2: {p:0, i: 'A', d: 'B'}
chunk 3: {p:0, i: 'B', d: 'C'}
chunk 4: {p:0, i: 'C'}

so final op would be:

[
  {p:0, i: 'X', d: 'A'}, 
  {p:4, i: 'A', d: 'B'},
  {p:8, i: 'B', d: 'C'},
  {p:12, i: 'C'}
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

From the table i would assume cthe chanuk should be somewhere around 5k chars.

@dodoarg dodoarg Aug 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

as discussed, I stripped the max residual guard + algo branching (accepting the potential cost degradation case as we already do in other places where Delta.diff is called) and opened issue with our findings/ideas

@dodoarg
dodoarg requested a review from dawidreedsy August 13, 2026 10:36
@dodoarg
dodoarg merged commit 19195e3 into master Aug 18, 2026
1 check passed
@dodoarg
dodoarg deleted the expose-diff branch August 18, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants