From 78943da92ba5a8946cc42edb1d90b55bcef88fc7 Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 8 Jun 2026 20:41:13 +0100 Subject: [PATCH] fix(editor): tag inserts with clientVars author until userAuthor propagates (Firefox authorship flake) (#7910) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(editor): tag inserts with clientVars author until userAuthor propagates Fixes the intermittent Firefox `clear_authorship_color` flake ("clear authorship colors can be undone to restore author colors"). Root cause: the inner editor's `thisAuthor` starts '' and is only populated when collab_client's `setProperty('userAuthor', userId)` reaches the inner frame — that call is queued by the outer ace wrapper via pendingInit until the iframe loads, so it is applied asynchronously. Under Firefox timing the first keystrokes can beat it, so freshly typed text is tagged author=''. An empty author canonicalizes to no-author, producing an unattributed insert (`Z:1>5+5$Hello`) that the server's pad-corruption guard rejects ("submitted an insert without an author attribute"), dropping the whole USER_CHANGES and losing authorship. Undo then cannot restore an author color and the test sees ``. Fix: a `getLocalAuthor()` helper that falls back to `clientVars.userId` (the same author id, available synchronously in the inner frame — already used as `myId` elsewhere) whenever `thisAuthor` is still empty, applied at the three new-text insert sites. The intentional clear-authorship path (`['author','']`) and the server-side guard are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(editor): also seed line-attribute author to close the same race The first commit fixed text inserts, but line-attribute changes (lists, headings, alignment) emit a line-marker insert tagged with AttributeManager.author, which likewise starts '' and is only set when the async setProperty('userAuthor') lands. An early list/heading could therefore emit an unattributed line-marker insert and hit the same server-side rejection. Seed documentAttributeManager.author from getLocalAuthor() (clientVars fallback) right after it is constructed; the userauthor handler keeps it in sync after. Audited the other author-tagging paths: all ace2_inner text-insert sites go through getLocalAuthor()/authorizer now, and contentcollector (paste/import) derives authorship from the pasted DOM's author- classes via className2Author — a different mechanism, not this async race — and self-guards on `if (state.author)`. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(editor): read the local author from the TOP pad window, not the inner frame v1 of this fix fell back to `window.clientVars?.userId`, but that did NOT resolve the flake — CI reproduced the identical failure and the server log still showed the unattributed-insert rejection. Ground truth via direct DOM measurement of a running pad: the inner editor iframe's `window.clientVars` is NEVER populated (undefined at t=0/1/3/6s for the life of the pad), so the v1 fallback always returned ''. The author id lives on the TOP pad window (`window.top.clientVars.userId`), set by pad.ts when the CLIENT_VARS message arrives — which necessarily precedes editor creation, so it is reliably available from the inner frame the moment the editor can accept input. getLocalAuthor() now reads window.top.clientVars.userId (guarded with try/catch for cross-origin embedded pads). thisAuthor still takes precedence once the queued setProperty('userAuthor') lands. All text-insert sites and the line-attribute seed already route through getLocalAuthor(), so both paths are covered. Verification: ts-check passes; fix source confirmed by direct measurement (the top window reliably holds the author at editor-init time). Local end-to-end repro was not possible — the dev server's require-kernel bundle did not reflect working-tree edits — so this is validated against the measured source + CI. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(editor): walk ancestor frames for the author instead of window.top Addresses a correctness gap flagged in review: reading window.top.clientVars fails for same-origin embeds, where window.top is the host page (accessible, so no exception is thrown) and has no Etherpad clientVars — getLocalAuthor() then returns '' and the unattributed-insert bug recurs. Walk up the ancestor chain (inner -> ace_outer -> pad window) and return the first frame that has clientVars.userId. This stops at the pad window and never depends on window.top, so it is correct for normal pads, same-origin embeds, and cross-origin embeds (the try/catch ends the walk if an ancestor is cross-origin, though the pad window — always same-origin with the editor frames — is reached first). Behavior in the non-embed case is unchanged (pad window is 2 hops up). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(collab): stamp the author onto outgoing inserts in collab_client Replaces the earlier getLocalAuthor attempts (v1-v3, reverted from ace2_inner.ts), which all failed because they tried to *read* an author that genuinely does not exist yet: the local author id (clientVars.userId) only arrives with the CLIENT_VARS socket message, and under load (Firefox + plugins) the editor can become editable and the user can type before that message lands. At that instant there is no author anywhere client-side, so the editor emits an unattributed insert that the server's pad-corruption guard rejects — dropping the change and losing authorship (the clear_authorship_color flake). Fix where the author IS reliably available: collab_client only exists after CLIENT_VARS has arrived, so its `userId` is always populated. A new pure helper stampAuthorOnInserts() rewrites the outgoing wire changeset so every '+' op carries an author, stamping userId onto any that lack one, just before it goes on the wire (both the USER_CHANGES send and the reconnect/further-changeset path). Independent of editor-init timing. Verification: - Unit test (stampAuthorOnInserts.test.ts, 5 cases): stamps the exact flake changeset `Z:1>5+5$Hello` -> authored + checkRep-valid + text preserved; leaves already-attributed inserts and keep/remove-only changesets unchanged; never invents an author when userId is empty; preserves other attributes. - ace2_inner.ts reverted to develop (the editor is unchanged; the fix is fully localized to the send path). - Local run of the clear_authorship_color spec with the fix: all pass, no server-side unattributed-insert rejections; full backend-new vitest green (the unrelated backend-tests-glob env-only failure aside). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- src/static/js/collab_client.ts | 22 +++++- src/static/js/stampAuthorOnInserts.ts | 58 ++++++++++++++ .../specs/stampAuthorOnInserts.test.ts | 76 +++++++++++++++++++ 3 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 src/static/js/stampAuthorOnInserts.ts create mode 100644 src/tests/backend-new/specs/stampAuthorOnInserts.test.ts diff --git a/src/static/js/collab_client.ts b/src/static/js/collab_client.ts index c90f92e80..bc5a533bb 100644 --- a/src/static/js/collab_client.ts +++ b/src/static/js/collab_client.ts @@ -26,6 +26,7 @@ const chat = require('./chat').chat; const hooks = require('./pluginfw/hooks'); const browser = require('./vendors/browser'); +import {stampAuthorOnInserts} from './stampAuthorOnInserts'; // Dependency fill on init. This exists for `pad.socket` only. // TODO: bind directly to the socket. @@ -48,6 +49,23 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad) let commitDelay = 500; const userId = initialUserInfo.userId; + + // Build the outgoing changeset and guarantee every insert carries an author. + // collab_client only exists after CLIENT_VARS has arrived, so `userId` is always + // populated here — whereas the editor can build a changeset with an empty author + // if the user typed before CLIENT_VARS landed (the early-typing race that the + // server's pad-corruption guard rejects). Stamping here is the last line of + // defense before the changeset goes on the wire. See stampAuthorOnInserts. + const prepareUserChangeset = () => { + const data = editor.prepareUserChangeset(); + if (data.changeset) { + const stamped = stampAuthorOnInserts(data.changeset, data.apool, userId); + data.changeset = stamped.changeset; + data.apool = stamped.apool; + } + return data; + }; + // var socket; const userSet = {}; // userId -> userInfo userSet[userId] = initialUserInfo; @@ -114,7 +132,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad) // Check if there are any pending revisions to be received from server. // Allow only if there are no pending revisions to be received from server if (!isPendingRevision) { - const userChangesData = editor.prepareUserChangeset(); + const userChangesData = prepareUserChangeset(); if (userChangesData.changeset) { lastCommitTime = now; committing = true; @@ -416,7 +434,7 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad) obj.committedChangesetAPool = stateMessage.apool; editor.applyPreparedChangesetToBase(); } - const userChangesData = editor.prepareUserChangeset(); + const userChangesData = prepareUserChangeset(); if (userChangesData.changeset) { obj.furtherChangeset = userChangesData.changeset; obj.furtherChangesetAPool = userChangesData.apool; diff --git a/src/static/js/stampAuthorOnInserts.ts b/src/static/js/stampAuthorOnInserts.ts new file mode 100644 index 000000000..a8f8afbf7 --- /dev/null +++ b/src/static/js/stampAuthorOnInserts.ts @@ -0,0 +1,58 @@ +'use strict'; + +import {deserializeOps, pack, unpack} from './Changeset'; +import AttributeMap from './AttributeMap'; +import AttributePool from './AttributePool'; +import {SmartOpAssembler} from './SmartOpAssembler'; + +/** + * Ensure every insert (`+`) op in a wire changeset carries an `author` attribute. + * + * Why this exists: the local author id (`clientVars.userId`) only becomes available + * once the CLIENT_VARS socket message arrives. Under load (notably Firefox with + * plugins) the editor can become editable and the user can type *before* that + * message lands, so the editor tags the insert with an empty author. An empty + * author canonicalizes to "no author", producing an unattributed insert that the + * server's pad-corruption guard rejects — dropping the whole USER_CHANGES and + * silently losing the typed text's authorship (the clear_authorship_color flake). + * + * This runs in collab_client, which only exists after CLIENT_VARS has arrived, so + * the author id passed here is always populated. Stamping any author-less insert + * just before the changeset is sent guarantees the server never sees an + * unattributed insert, independent of editor-init timing. + * + * @param changeset - The wire changeset string from prepareUserChangeset(). + * @param apoolJsonable - The jsonable wire attribute pool that accompanies it. + * @param authorId - The local author id (collab_client's userId). If falsy, the + * inputs are returned unchanged (nothing better to stamp with). + * @returns The (possibly) rewritten changeset + jsonable pool. Returns the inputs + * unchanged when no insert needed an author, so the common path is a no-op. + */ +export const stampAuthorOnInserts = ( + changeset: string, + apoolJsonable: any, + authorId: string, +): {changeset: string, apool: any} => { + if (!authorId) return {changeset, apool: apoolJsonable}; + const pool = (new AttributePool()).fromJsonable(apoolJsonable); + const unpacked = unpack(changeset); + const assem = new SmartOpAssembler(); + let modified = false; + for (const op of deserializeOps(unpacked.ops)) { + if (op.opcode === '+') { + const attribs = AttributeMap.fromString(op.attribs, pool); + if (!attribs.get('author')) { + attribs.set('author', authorId); + op.attribs = attribs.toString(); + modified = true; + } + } + assem.append(op); + } + if (!modified) return {changeset, apool: apoolJsonable}; + assem.endDocument(); + return { + changeset: pack(unpacked.oldLen, unpacked.newLen, assem.toString(), unpacked.charBank), + apool: pool.toJsonable(), + }; +}; diff --git a/src/tests/backend-new/specs/stampAuthorOnInserts.test.ts b/src/tests/backend-new/specs/stampAuthorOnInserts.test.ts new file mode 100644 index 000000000..449814a2a --- /dev/null +++ b/src/tests/backend-new/specs/stampAuthorOnInserts.test.ts @@ -0,0 +1,76 @@ +'use strict'; + +import {describe, it, expect} from 'vitest'; +import {stampAuthorOnInserts} from '../../../static/js/stampAuthorOnInserts'; +import {checkRep, deserializeOps, unpack} from '../../../static/js/Changeset'; +import AttributeMap from '../../../static/js/AttributeMap'; +import AttributePool from '../../../static/js/AttributePool'; + +const AUTHOR = 'a.test1234567890'; +const EMPTY_POOL = () => (new AttributePool()).toJsonable(); + +// Read the author attribute off the first '+' op of a (changeset, jsonable pool). +const firstInsertAuthor = (changeset: string, apoolJsonable: any): string | undefined => { + const pool = (new AttributePool()).fromJsonable(apoolJsonable); + for (const op of deserializeOps(unpack(changeset).ops)) { + if (op.opcode === '+') return AttributeMap.fromString(op.attribs, pool).get('author'); + } + return undefined; +}; + +describe('stampAuthorOnInserts', () => { + it('stamps the author onto an unattributed insert (the flake changeset)', () => { + // `Z:1>5+5$Hello` — insert "Hello" with NO author, exactly what the editor + // emits during the early-typing race and what the server rejects. + const input = 'Z:1>5+5$Hello'; + const {changeset, apool} = stampAuthorOnInserts(input, EMPTY_POOL(), AUTHOR); + // The insert now carries the author... + expect(firstInsertAuthor(changeset, apool)).toBe(AUTHOR); + // ...the result is a valid canonical changeset... + expect(() => checkRep(changeset)).not.toThrow(); + // ...and the text is preserved. + expect(unpack(changeset).charBank).toBe('Hello'); + // It actually changed (was unattributed before). + expect(changeset).not.toBe(input); + }); + + it('leaves an already-attributed insert unchanged', () => { + // Build `Z:1>5*0+5$Hello` with author already in the pool. + const pool = new AttributePool(); + const n = pool.putAttrib(['author', AUTHOR]); // index 0 + const attributed = `Z:1>5*${n}+5$Hello`; + const jsonable = pool.toJsonable(); + const {changeset, apool} = stampAuthorOnInserts(attributed, jsonable, AUTHOR); + expect(changeset).toBe(attributed); // unchanged (no-op path) + expect(firstInsertAuthor(changeset, apool)).toBe(AUTHOR); + }); + + it('does not invent an author when authorId is empty', () => { + const input = 'Z:1>5+5$Hello'; + const {changeset} = stampAuthorOnInserts(input, EMPTY_POOL(), ''); + expect(changeset).toBe(input); // nothing to stamp with → unchanged + }); + + it('does not touch keep/remove-only changesets', () => { + // `Z:6<1=5-1$` — keep 5, remove 1; no insert ops. + const input = 'Z:6<1=5-1$x'; + const {changeset} = stampAuthorOnInserts(input, EMPTY_POOL(), AUTHOR); + expect(changeset).toBe(input); + }); + + it('preserves a non-author attribute already on the insert while adding author', () => { + // Insert with a bold attribute but no author. + const pool = new AttributePool(); + const b = pool.putAttrib(['bold', 'true']); // index 0 + const input = `Z:1>5*${b}+5$Hello`; + const {changeset, apool} = stampAuthorOnInserts(input, pool.toJsonable(), AUTHOR); + const outPool = (new AttributePool()).fromJsonable(apool); + let amap: AttributeMap | null = null; + for (const op of deserializeOps(unpack(changeset).ops)) { + if (op.opcode === '+') { amap = AttributeMap.fromString(op.attribs, outPool); break; } + } + expect(amap!.get('author')).toBe(AUTHOR); + expect(amap!.get('bold')).toBe('true'); + expect(() => checkRep(changeset)).not.toThrow(); + }); +});