Skip to content

SF-3913 Pause sending of ops to ShareDB if offline - #4088

Merged
pmachapman merged 1 commit into
masterfrom
fix/SF-3913
Sep 20, 2026
Merged

pmachapman merged 1 commit into
masterfrom
fix/SF-3913

Conversation

@pmachapman

@pmachapman pmachapman commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

I isolated the root of this issue to client 1 thinking it was offline, and so triggering the logic in TextViewModel.fixSegment() to not create blanks (see SF-2272 and SF-2722), but the client was not completely offline - it was just a flaky connection. Because an op was sent to ShareDB that cleared a segment, but did not create a blank in its place, another listening client creates the blank, and sends it to ShareDB. When client 1 comes back online, it can under certain circumstances become confused, as it will attempt to create a blank. This will often resolve correctly, but if other edits took place, duplication of segments can occur sometimes (but not all the time). The duplication results from ShareDB's resolution of any incoming and outgoing ops on the segment, and its attempt to reconcile them.

Resolution

To resolve this issue, I pause any updates being sent to ShareDB for the text document if Scripture Forge believes itself to be offline. Ops are resumed when Scripture Forge next is online. This means that the omission of blank ops by TextViewModel.fixSegment() is not replicated to ShareDB (it is not supposed to be as this is offline-only functionality to ensure offline edits do not corrupt valid incoming edits from other users - see SF-2272)

Other Solutions

As the root of this issue is blank ops, and the view model's handling of them, the only other solution I could come to is the currently mothballed SF-3437 / #3339.


This change is Reviewable

@pmachapman pmachapman added will require testing PR should not be merged until testers confirm testing is complete e2e Run e2e tests for this pull request labels Sep 6, 2026
@codecov

codecov Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.94%. Comparing base (ef25688) to head (a571fc2).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...src/xforge-common/sharedb-realtime-remote-store.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4088   +/-   ##
=======================================
  Coverage   80.94%   80.94%           
=======================================
  Files         664      664           
  Lines       42170    42182   +12     
  Branches     6949     6951    +2     
=======================================
+ Hits        34136    34146   +10     
- Misses       6868     6870    +2     
  Partials     1166     1166           

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

@RaymondLuong3 RaymondLuong3 left a comment

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.

@RaymondLuong3 reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1232 at r1 (raw file):

    if (!this.onlineStatusService.isOnline) {
      textDoc.adapter.pause();
    }

Do you think these lines are necessary? There may be a small window when the component initializes and the app is offline but onlineStatus$ has not emitted yet, but that window is so small. Is that what these lines are for?

Code quote:

    if (!this.onlineStatusService.isOnline) {
      textDoc.adapter.pause();
    }

@pmachapman pmachapman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@pmachapman made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/text.component.ts line 1232 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Do you think these lines are necessary? There may be a small window when the component initializes and the app is offline but onlineStatus$ has not emitted yet, but that window is so small. Is that what these lines are for?

Yes - I found it was necessary to do this in my testing.

@RaymondLuong3 RaymondLuong3 left a comment

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.

@RaymondLuong3 resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@RaymondLuong3 RaymondLuong3 added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Sep 10, 2026
@RaymondLuong3 RaymondLuong3 self-assigned this Sep 10, 2026
@pmachapman
pmachapman deployed to screenshot_diff September 14, 2026 00:05 — with GitHub Actions Active
@pmachapman pmachapman added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test e2e Run e2e tests for this pull request labels Sep 20, 2026
@pmachapman
pmachapman deployed to screenshot_diff September 20, 2026 20:55 — with GitHub Actions Active
@pmachapman
pmachapman merged commit 3f221d3 into master Sep 20, 2026
28 of 29 checks passed
@pmachapman
pmachapman deleted the fix/SF-3913 branch September 20, 2026 20:58

This branch was successfully deployed

2 active (1 outdated) deployments
screenshot_diff — a571fc22 Deployed Sep 20, 2026 by pmachapman via Compare Screenshots #914
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing complete Testing of PR is complete and should no longer hold up merging of the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants