Skip to content

SF-2722 Send blank ops created when the user comes online to ShareDB#3935

Open
pmachapman wants to merge 1 commit into
masterfrom
fix/SF-2722
Open

SF-2722 Send blank ops created when the user comes online to ShareDB#3935
pmachapman wants to merge 1 commit into
masterfrom
fix/SF-2722

Conversation

@pmachapman

@pmachapman pmachapman commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

This PR where verse numbers disappear when coming online then editing a text by ensuring that an blank ops created by fixSegments() make their way to ShareDB.


This change is Reviewable

@pmachapman pmachapman added the will require testing PR should not be merged until testers confirm testing is complete label Jun 10, 2026
@pmachapman pmachapman temporarily deployed to screenshot_diff June 10, 2026 01:54 — with GitHub Actions Inactive
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.67%. Comparing base (eec50dc) to head (516d283).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3935      +/-   ##
==========================================
+ Coverage   80.66%   80.67%   +0.01%     
==========================================
  Files         634      634              
  Lines       41090    41092       +2     
  Branches     6681     6682       +1     
==========================================
+ Hits        33144    33150       +6     
+ Misses       6893     6890       -3     
+ Partials     1053     1052       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@Nateowami

Copy link
Copy Markdown
Collaborator

From the time the change was originally made, I've been uncomfortable with the approach of not creating blank ops offline, though I didn't have a clear alternative to suggest, or a clear problem they caused, other than the editor working differently for users offline.

Now that we've identified a clear problem that solution caused, I really think we should see if we can identify a different solution altogether rather than patch the existing solution. One approach might be composing contiguous blank ops that pertain to the same verse, though I don't know for certain whether that would work well, or if there might be other solutions.

@pmachapman

Copy link
Copy Markdown
Collaborator Author

Now that we've identified a clear problem that solution caused, I really think we should see if we can identify a different solution altogether rather than patch the existing solution. One approach might be composing contiguous blank ops that pertain to the same verse, though I don't know for certain whether that would work well, or if there might be other solutions.

@Nateowami I'm not sure that would work. The root of the problem is that we use a embed to represent editable space. Embeds should only represent non-editable space, i.e. in the spaces around editable space. Generally embeds should only be inserted or removed via ops, but as we place this in editable space, and do this as a form of "correction" on the frontend, we get bugs which I best describe as "mashing".

In my brief research, it looks like someone else encountered a similar limitation of the rich-text format, and so built their own variant of it that acts more like a real op by allowing inversion: https://github.com/Teamwork/ot-rich-text (this format is incompatible with ours). By implementing invert, they force all items (and thus all actions) to be indexed. Our implementation of rich-text does not index actions, instead the entire doc is converted to a string, and simple slice() style operations performed on the string then it is reconstituted.

SF-2272 sought to address the bug by not creating blank ops when offline, however a follow-on bug of that PR was the local-only creation of these automatically by fixSegments when the user came online again, and ops streamed in from ShareDB. Any future op calcuations by the editor would be offset by this blank op, which the ShareSD server would be unaware of, and so the corruption and verse removal bug. This PR addresses that by ensuring that any ops created this way are sent back to ShareDB, but I still feel that this is hack solution I have implemented.

Probably the best "non-hack" solution is to remove use blank ops altogether (SF-3437) which I more-or-less wrote in #3339.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

will require testing PR should not be merged until testers confirm testing is complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants