diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d8c704ac..df5b3a209 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,9 +28,14 @@ and this project adheres to become one, no version spans more than a minute, and changes are merged whoever made them, because a version is a moment in the document rather than a moment in one person's editing. Selecting one previews the document exactly as - it stood then. Note that a document whose history has not yet been replayed - into the collaboration server (`manage.py migrate_documents`) lists only the - edits made since it moved there + it stood then. A migrated document's imported history is the exception to the + grouping and is listed save by save: those entries are a record of saves that + already happened rather than an editing session to summarise, and they arrive + spaced exactly the width of the grouping window, so grouping them would have + dropped about a third of them depending on how fast the network was years ago. + Note that a document whose history has not yet been replayed into the + collaboration server (`manage.py migrate_documents`) lists only the edits made + since it moved there - ✨(frontend) make restoring a version work again. It has done nothing since the migration, while still promising that the document would be replaced. The diff --git a/documentation/collaboration.md b/documentation/collaboration.md index 0e40a2bed..1fb260113 100644 --- a/documentation/collaboration.md +++ b/documentation/collaboration.md @@ -144,6 +144,14 @@ moment in one person's editing, so two people typing in the same minute produce two interleaved ones. The collaboration server only ever groups changes by the same author, so this last step happens in the browser, on top of its grouping. +With one exception: **a migrated document's imported history is never grouped.** Before the +migration the editor saved the whole document once a minute, and the migration replays each of +those saves at its original timestamp, attributed to `system`. Grouped like ordinary editing they +would land on the granularity itself — a chain of entries a minute apart, merging or not depending +on how fast the network was on the day each was written, losing about a third of the chain. There +was no editing session to summarise here, only a record of saves that already happened, so they are +shown one for one. Both the collaboration server and the browser are told to leave them alone. + ## Restoring a previous state Selecting a version and restoring it asks the collaboration server to undo everything that happened diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts b/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts index 99402b5a9..350895db2 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/__tests__/utils.test.ts @@ -86,6 +86,47 @@ describe('mergeActivityEntries', () => { expect(version.to).toBe(900); }); + it('keeps a migrated history intact, save by save', () => { + /** + * The case `UNGROUPED_AUTHORS` exists for. The backend used to save the + * whole document every 60s, each save becoming one version of the legacy + * file, and the migration replays them at their original timestamps, all + * attributed to `system` — so an imported history is a chain of entries + * spaced almost exactly the granularity apart, with nothing to tell them + * apart by author. + * + * Jitter is applied in both directions on purpose: it is the negative side + * that would merge, and a perfectly fixed interval would pass either way. + */ + let t = 0; + const legacy: ActivityEntry[] = []; + [0, -900, 700, -1500, 400, -300, 1200, -80].forEach((jitter) => { + legacy.push(entry(t, t, 'system')); + t += 60_000 + jitter; + }); + + expect(mergeActivityEntries(legacy)).toHaveLength(legacy.length); + }); + + it('never folds an edit into the imported history beside it', () => { + // stricter than the collaboration server's own test, which would have + // refused this on the author comparison it makes and this one does not: a + // document edited moments after it was migrated must not absorb the + // migration entry, nor be absorbed by it + const versions = mergeActivityEntries([ + entry(0, 0, 'system'), + entry(500, 500, 'alice'), + entry(1_000, 1_000, 'system'), + ]); + + expect(versions).toHaveLength(3); + expect(versions.map((v) => v.by)).toEqual([ + ['system'], + ['alice'], + ['system'], + ]); + }); + it('keeps an unattributed change without inventing an author', () => { const [version] = mergeActivityEntries([entry(0, 0, null)]); diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx index d784171fc..294c2f254 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/api/useDocActivity.tsx @@ -9,7 +9,11 @@ import { import { useCollaborationTarget } from '@/core/config/hooks/useCollaborationUrl'; import { APIActivity, DocVersion } from '../types'; -import { VERSION_GRANULARITY_MS, mergeActivityEntries } from '../utils'; +import { + UNGROUPED_AUTHORS, + VERSION_GRANULARITY_MS, + mergeActivityEntries, +} from '../utils'; export type DocActivityParam = { docId: string; @@ -41,6 +45,9 @@ const getDocActivity = async ( group: true, groupMaxGap: VERSION_GRANULARITY_MS, groupMaxDuration: VERSION_GRANULARITY_MS, + // the imported history is shown save by save — see UNGROUPED_AUTHORS. + // The panel applies this again, on both sides of an entry. + groupExclude: UNGROUPED_AUTHORS.join(','), }, }, ); diff --git a/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts b/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts index 202f6476a..192c69e00 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts +++ b/src/frontend/apps/impress/src/features/docs/doc-versioning/utils.ts @@ -11,6 +11,24 @@ import { ActivityEntry, DocVersion } from './types'; */ export const VERSION_GRANULARITY_MS = 60_000; +/** + * Authors whose changes are never merged into a version with anything else. + * + * `system` is not a person who edited for a long time; it is what a document's + * imported past is attributed to. The backend used to save the whole document + * once a minute, each save becoming one version of the legacy file, and the + * migration replays those saves at their original timestamps — so an imported + * history arrives as a chain of `system` entries spaced almost exactly the + * granularity apart. Grouped like ordinary editing, whether two of those + * versions survive as two would come down to how fast the network was on the + * day they were written, and about a third of the chain would collapse. + * + * Grouping is a judgement about someone's editing, and there was no editing + * here: these entries are the record of saves that already happened, and the + * only honest thing to do with them is to show them one for one. + */ +export const UNGROUPED_AUTHORS = ['system']; + /** * Merge an ascending activity timeline into the versions the panel lists. * @@ -28,6 +46,13 @@ export const VERSION_GRANULARITY_MS = 60_000; * * Both comparisons are `<`, matching the server's, so entries exactly a minute * apart start a new version rather than joining the old one. + * + * `UNGROUPED_AUTHORS` is honoured on both sides, which is stricter than the + * server's own test and has to be: the server only refuses to merge an excluded + * entry into the one before it, because it would have refused anyway on the + * author test it makes and this does not. Here an excluded entry both starts a + * version and closes it, so an edit made moments after a document was migrated + * cannot be folded into the imported history it happens to sit next to. */ export const mergeActivityEntries = ( activity: ActivityEntry[], @@ -35,12 +60,17 @@ export const mergeActivityEntries = ( ): DocVersion[] => { const versions: DocVersion[] = []; const authors: Set[] = []; + const isUngrouped = (by: string | null) => + by !== null && UNGROUPED_AUTHORS.includes(by); activity.forEach((entry) => { const last = versions[versions.length - 1]; + const lastAuthors = authors[authors.length - 1]; if ( last && + !isUngrouped(entry.by) && + !Array.from(lastAuthors ?? []).some(isUngrouped) && entry.from - last.to < granularityMs && entry.to - last.from < granularityMs ) { diff --git a/src/yhub-server/package-lock.json b/src/yhub-server/package-lock.json index 62822fae9..97b7f647c 100644 --- a/src/yhub-server/package-lock.json +++ b/src/yhub-server/package-lock.json @@ -7,7 +7,7 @@ "name": "yhub-server", "dependencies": { "@aws-sdk/client-s3": "3.1110.0", - "@y/hub": "0.8.0", + "@y/hub": "0.8.2", "@y/y": "14.0.0-rc.24", "jose": "6.2.8" }, @@ -504,9 +504,9 @@ "license": "ISC" }, "node_modules/@y/hub": { - "version": "0.8.0", - "resolved": "https://registry.npmjs.org/@y/hub/-/hub-0.8.0.tgz", - "integrity": "sha512-omBq00oLl7NVmB2qPMlKmTL/6ZTxP8hYZZHOX0RMsSKL+53JlbLJn9WVX04xtXYaM+Q81aLsy1/BRUwBYf4yOw==", + "version": "0.8.2", + "resolved": "https://registry.npmjs.org/@y/hub/-/hub-0.8.2.tgz", + "integrity": "sha512-JMwHMwmnUAp6N+qz2MKaIRp/eDGOYIqGTRnW+aNyFg4nypMnnsjPzx7rs6/uG4RR1IDEKiqr4A5Khta44Q2NPQ==", "license": "AGPL-3.0 OR PROPRIETARY", "dependencies": { "@y-crdt/yn": "^0.1.4", diff --git a/src/yhub-server/package.json b/src/yhub-server/package.json index bb86fcd65..5196228b8 100644 --- a/src/yhub-server/package.json +++ b/src/yhub-server/package.json @@ -10,7 +10,7 @@ }, "dependencies": { "@aws-sdk/client-s3": "3.1110.0", - "@y/hub": "0.8.0", + "@y/hub": "0.8.2", "@y/y": "14.0.0-rc.24", "jose": "6.2.8" },