Skip to content

fix(sync): send first update without initial document state #7787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mejo- merged 3 commits into from
Oct 16, 2025
Merged

fix(sync): send first update without initial document state #7787

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbbf4ec
fix(sync): send first update without initial document state
max-nextcloud Oct 15, 2025
6f8fd00
fix(sync): bump version after loading document state
max-nextcloud Oct 15, 2025
205c7cb
chore(cleanup): remove unused $newVersion.
max-nextcloud Oct 16, 2025
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
Prev Previous commit
Next Next commit
fix(sync): bump version after loading document state 
* Emit the documents last saved version in the push response
  if document state is send as well.
* Otherwise send the version as 0 so it cannot increase the sync service version.
* Use the versions returned by create and push requests alongside the document state
  to construct a consistent step with the last saved version that matches the document state.

This will result in the sync service version getting bumped
to the documents last saved version
once the coresponding document state has been processed.

Signed-off-by: Max <[email protected]>
  • Loading branch information
@max-nextcloud
max-nextcloud committed Oct 16, 2025
commit 6f8fd00669a1924ac404ba8fa86232e31ae4459a
2 changes: 1 addition & 1 deletion lib/Service/DocumentService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ public function addStep(Document $document, Session $session, array $steps, int

return [
'steps' => $stepsToReturn,
'version' => $newVersion,
'version' => isset($documentState) ? $document->getLastSavedVersion() : 0,
Copy link
Member

@juliusknorr juliusknorr Oct 16, 2025

Choose a reason for hiding this comment

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

Can you tell if this has any other side effects? We now no longer return the latest version (number of steps in the db) but just the last saved. We used this in the past to compare and see if the document had unsaved changes, but I think that part was changed already.

Copy link
Member

@juliusknorr juliusknorr Oct 16, 2025

Choose a reason for hiding this comment

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

Otherwise $newVersion is no longer used as a variable

Copy link
Collaborator Author

@max-nextcloud max-nextcloud Oct 16, 2025

Choose a reason for hiding this comment

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

Good catch wrt $newVersion. We were not using the version in the response on the client side anymore.

The unsaved changes check is on the server when cleaning up docs, right? That should still work. this is not changing the data structures there.

Copy link
Member

@juliusknorr juliusknorr Oct 16, 2025

Choose a reason for hiding this comment

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

Right there we still have it, i just remembered that we also used it in the frontend in the early days

'documentState' => $documentState
];
}
Expand Down
2 changes: 1 addition & 1 deletion src/apis/sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface PushResponse {
data: {
steps: Step[]
documentState: string
awareness: Record<string, string>
version: number
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/helpers/yjs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ export function applyDocumentState(
* and encode it and wrap it in a step data structure.
*
* @param documentState - base64 encoded doc state
* @param version - last saved version for the document state
* @return base64 encoded yjs sync protocol update message and version
*/
export function documentStateToStep(documentState: string): Step {
export function documentStateToStep(documentState: string, version: number): Step {
const message = documentStateToUpdateMessage(documentState)
return { data: [encodeArrayBuffer(message)], sessionId: 0, version: -1 }
return { data: [encodeArrayBuffer(message)], sessionId: 0, version }
}

/**
Expand Down
15 changes: 9 additions & 6 deletions src/services/SyncService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,15 @@ class SyncService {
console.error('Opened the connection but now it is undefined')
return
}
this.version = data.document.lastSavedVersion
this.backend = new PollingBackend(this, this.connection.value, data)
// Make sure to only emit this once the backend is in place.
this.bus.emit('opened', data)
// Emit sync after opened, so websocket onmessage comes after onopen.
if (data.documentState) {
this._emitDocumentStateStep(data.documentState)
this._emitDocumentStateStep(
data.documentState,
data.document.lastSavedVersion,
)
}
}

Expand All @@ -202,8 +204,8 @@ class SyncService {
}
}

_emitDocumentStateStep(documentState: string) {
const documentStateStep = documentStateToStep(documentState)
_emitDocumentStateStep(documentState: string, version: number) {
const documentStateStep = documentStateToStep(documentState, version)
this.bus.emit('sync', {
steps: [documentStateStep],
})
Expand Down Expand Up @@ -249,12 +251,13 @@ class SyncService {
})
.then((response) => {
this.#outbox.clearSentData(sendable)
const { steps, documentState } = response.data as {
const { steps, documentState, version } = response.data as {
steps: Step[]
documentState: string
version: number
}
if (documentState) {
this._emitDocumentStateStep(documentState)
this._emitDocumentStateStep(documentState, version)
}
this.pushError = 0
this.#sending = false
Expand Down
4 changes: 2 additions & 2 deletions src/tests/helpers/yjs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ describe('Yjs base64 wrapped with our helpers', function () {
sourceMap.set('keyA', 'valueA')

const stateA = getDocumentState(source)
const step0A = documentStateToStep(stateA)
const step0A = documentStateToStep(stateA, 123)
applyStep(target, step0A)
expect(targetMap.get('keyA')).to.be.eq('valueA')

// Add keyB to source, don't apply to target yet
sourceMap.set('keyB', 'valueB')
const stateB = getDocumentState(source)
const step0B = documentStateToStep(stateB)
const step0B = documentStateToStep(stateB, 124)

// Add keyC to source, apply to target
sourceMap.set('keyC', 'valueC')
Expand Down