nextcloud / text

đź“‘ Collaborative document editing using Markdown
GNU Affero General Public License v3.0
540 stars 87 forks source link

Sync fails if initial state is present in the yjs document #4480

Open max-nextcloud opened 1 year ago

max-nextcloud commented 1 year ago

Describe the bug If there is some initial state in the yjs document our sync mechanism fails and changes from the user in question are not transferred.

If a yjs session starts with initial data in the document the client will not send updates for that initial data. Normally this is not a problem because the initial Sync message 1 by the other party will cause the client to send it's 'backlog'.

Our sync mechanism blocks yjs queries (Sync Message 1).

It's not clear if this can happen in a real life scenario. Tiptap may take longer to load than yjs takes to initialize and send the initial push message.

To Reproduce

Add this test to cypress/e2e/api/SyncServiceProvider.spec.js:


it('syncs even when initial state was present', function() {
        const sourceMap = this.source.getMap()
        const targetMap = this.target.getMap()
        sourceMap.set('unrelated', 'value')
        cy.intercept({ method: 'POST', url: '**/apps/text/session/push' })
            .as('push')
        cy.intercept({ method: 'POST', url: '**/apps/text/session/sync' })
            .as('sync')
        cy.wait('@push')
        cy.then(() => {
            sourceMap.set('keyA', 'valueA')
            expect(targetMap.get('keyB')).to.be.eq(undefined)
        })
        cy.wait('@sync')
        cy.wait('@sync')
        // eslint-disable-next-line cypress/no-unnecessary-waiting
        cy.wait(1000)
        cy.then(() => {
            expect(targetMap.get('keyA')).to.be.eq('valueA')
        })
    })

See it fail.

juliushaertl commented 7 months ago

@max-nextcloud Sounds a bit like your recent fix addressed this, but not sure if the test case is still applicable?

max-nextcloud commented 7 months ago

Uh... interesting. Had forgotten about this one. It's probably still failing as my fix only applies after some document state has been saved. But it could (and probably should) be expended to also address this one. Nice to know there's already a test for this. It should still work. Will add it to my todo list.

mejo- commented 5 months ago

@max-nextcloud should we close this for now, given that we did quite a bit of refactoring of create/sync/push? Or would you like to keep it open?