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
+36 −28
Merged

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

Show file tree
Hide file tree
Changes from all commits
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
10 changes: 3 additions & 7 deletions lib/Service/DocumentService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ public function addStep(Document $document, Session $session, array $steps, int
$stepsToInsert = [];
$stepsIncludeQuery = false;
$documentState = null;
$newVersion = $version;
foreach ($steps as $step) {
$message = YjsMessage::fromBase64($step);
if ($readOnly && $message->isUpdate()) {
Expand All @@ -228,7 +227,7 @@ public function addStep(Document $document, Session $session, array $steps, int
if ($readOnly) {
throw new NotPermittedException('Read-only client tries to push steps with changes');
}
$newVersion = $this->insertSteps($document, $session, $stepsToInsert);
$this->insertSteps($document, $session, $stepsToInsert);
}

// By default, send all steps the user has not received yet.
Expand Down Expand Up @@ -265,7 +264,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 All @@ -275,14 +274,12 @@ public function addStep(Document $document, Session $session, array $steps, int
* @param Session $session
* @param Step[] $steps
*
* @return int
*
* @throws DoesNotExistException
* @throws InvalidArgumentException
*
* @psalm-param non-empty-list<mixed> $steps
*/
private function insertSteps(Document $document, Session $session, array $steps): int {
private function insertSteps(Document $document, Session $session, array $steps): void {
$stepsVersion = null;
try {
$stepsJson = json_encode($steps, JSON_THROW_ON_ERROR);
Expand All @@ -298,7 +295,6 @@ private function insertSteps(Document $document, Session $session, array $steps)
$this->logger->debug('Adding steps to ' . $document->getId() . ": bumping version from $stepsVersion to $newVersion");
$this->cache->set('document-version-' . $document->getId(), $newVersion);
// TODO write steps to cache for quicker reading
return $newVersion;
} catch (\Throwable $e) {
if ($stepsVersion !== null) {
$this->logger->error('This should never happen. An error occurred when storing the version, trying to recover the last stable one', ['exception' => $e]);
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: 1 addition & 4 deletions src/components/Editor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ import { CollaborationCursor } from '../extensions/index.js'
import { exposeForDebugging, removeFromDebugging } from '../helpers/debug.js'
import { logger } from '../helpers/logger.js'
import { setInitialYjsState } from '../helpers/setInitialYjsState.js'
import { applyDocumentState } from '../helpers/yjs.ts'
import { ERROR_TYPE, IDLE_TIMEOUT } from '../services/SyncService.ts'
import { fetchNode } from '../services/WebdavClient.ts'
import {
Expand Down Expand Up @@ -516,9 +515,7 @@ export default defineComponent({
// Fetch the document state after syntax highlights are loaded
this.lowlightLoaded.then(() => {
this.syncService.startSync()
if (documentState) {
applyDocumentState(this.ydoc, documentState, this.syncProvider)
} else {
if (!documentState) {
setInitialYjsState(this.ydoc, content, {
isRichEditor: this.isRichEditor,
})
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
23 changes: 17 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,10 +173,16 @@ 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,
data.document.lastSavedVersion,
)
}
}

startSync() {
Expand All @@ -198,6 +204,13 @@ class SyncService {
}
}

_emitDocumentStateStep(documentState: string, version: number) {
const documentStateStep = documentStateToStep(documentState, version)
this.bus.emit('sync', {
steps: [documentStateStep],
})
}

sendStep(step: Uint8Array<ArrayBufferLike>) {
this.#outbox.storeStep(step)
this.sendSteps()
Expand Down Expand Up @@ -238,15 +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) {
const documentStateStep = documentStateToStep(documentState)
this.bus.emit('sync', {
steps: [documentStateStep],
})
this._emitDocumentStateStep(documentState, version)
}
this.pushError = 0
this.#sending = false
Expand Down
15 changes: 9 additions & 6 deletions src/services/WebSocketPolyfill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default function initWebSocketPolyfill(
onopen?: () => void
#notifyPushBus
#onSync
#onOpened
#processingVersion = 0

constructor(url: string) {
Expand All @@ -34,6 +35,13 @@ export default function initWebSocketPolyfill(
this.#url = url
logger.debug('WebSocketPolyfill#constructor', { url, fileId })

this.#onOpened = () => {
if (syncService.hasActiveConnection()) {
this.onopen?.()
}
}
syncService.bus.on('opened', this.#onOpened)

this.#onSync = ({ steps }: { steps: Step[] }) => {
if (steps) {
this.#processSteps(steps)
Expand All @@ -43,14 +51,9 @@ export default function initWebSocketPolyfill(
})
}
}

syncService.bus.on('sync', this.#onSync)

syncService.open().then(() => {
if (syncService.hasActiveConnection()) {
this.onopen?.()
}
})
syncService.open()
}

/**
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
Loading