Skip to content

Conversation

marksvc
Copy link
Collaborator

@marksvc marksvc commented Sep 25, 2025

This change is Reviewable

@marksvc
Copy link
Collaborator Author

marksvc commented Sep 25, 2025

editor.component.spec.ts TestEnvironment.resolveNote races a call to noteDoc.submitJson0Op before calling realtimeService.updateQueryAdaptersRemote:

noteDoc.submitJson0Op(op => op.set(n => n.status, NoteStatus.Resolved));
this.realtimeService.updateQueryAdaptersRemote();

If we change the winner of the race by modifing the code so that submitJson0Op will finish before updateQueryAdaptersRemote starts, like so

    noteDoc
      .submitJson0Op(op => op.set(n => n.status, NoteStatus.Resolved))
      .then(() => {
        this.realtimeService.updateQueryAdaptersRemote();
      });

then editor.component.spec.ts test 'should remove resolved notes after a remote update' shows a failure, saying

Error: Expected 5 to equal 4.

(And it might also be meaningful to modify realtime-doc.ts submit() so the call to adapter.submitOp is awaited rather than raced.)

When the test does noteDoc.submitJson0Op, we get into realtime-query.ts onChange and it changes the count from 5 to 4. But it is a local operation and so does not emit a remote change.
When the test does realtimeService.updateQueryAdaptersRemote, we get into realtime-query.ts onChange, but count is already 4, and it does not see a change to emit about.
One might argue that there was a remote change, as understood by memory-realtime-remote-store.ts MemoryRealtimeQueryAdapter.perfomQuery seeing that it remembers the remote data being different. (Though realtime-query.ts onChange 'remembers' the remote's up-to-date information.)

The test applies an op locally to resolve a note, but is titled as removing such notes after a remote update.

It is not a successful fix to the test to change editor.component.spec.ts resolveNote to specify source as false. If we do this, memory-realtime-remote-store.ts MemoryRealtimeDocAdapter.submitOp does call emitRemoteChange, causing editor.component.ts loadNoteThreadDocs to detect changes from noteThreadQuery (presumably remoteDocChanges$). But the note thread query RealtimeQuery.docs is not up-to-date for presumably editor.component.currentChapterNoteThreadDocs to read an updated list. (Because realtime-query.ts onChange has not been run.)
realtime-doc.ts submit calls RealtimeService.onLocalDocUpdate, even if source is remote. submit does not seem to be designed to apply "purely remote" ops.

With this PR's proposed change, we don't call noteDoc.submitJson0Op (getting to realtime-doc.ts submit, to realtime.service.ts onLocalDocUpdate, to realtime-query.ts localQuery) and so don't get to realtime-query.ts onChange. Instead, the test uses a modified MemoryRealtimeDocAdapter.submitOp to apply changes without emitting about it, and calls RealtimeService.updateQueryAdaptersRemote which gets into realtime-query.ts onChange where a change is found and RealtimeQuery.remoteChanges$ is emitted. This gets us into loadNoteThreadDocs where we adjust notes.

Copy link

codecov bot commented Sep 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.18%. Comparing base (1c26d26) to head (c2a1a2d).
⚠️ Report is 14 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3466      +/-   ##
==========================================
- Coverage   82.20%   82.18%   -0.02%     
==========================================
  Files         611      611              
  Lines       36433    36439       +6     
  Branches     6004     5982      -22     
==========================================
- Hits        29951    29949       -2     
- Misses       5608     5623      +15     
+ Partials      874      867       -7     

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

@marksvc marksvc force-pushed the task/remote-resolvenote branch from 2ab09e1 to 3e4d263 Compare September 25, 2025 20:30
@marksvc marksvc force-pushed the task/remote-resolvenote branch from 3e4d263 to c2a1a2d Compare September 25, 2025 21:03
@marksvc marksvc closed this Sep 26, 2025
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.

1 participant