Skip to content

Commit 32ccc8f

Browse files
committed
fix(versioning): don't attribute lost content to whoever removed it (experimental)
Behind `experimental.lostContentAttribution` in the collaboration options; it only changes what a version diff shows. With it, versions are decoded keeping deleted content, so content lost with a concurrently deleted block, or with a moved block's lost copy, shows as deleted without an author instead of being credited to the deleter or the mover. Content inserted and deleted between the two versions stays hidden. Also, on by default: a change without a known author shows "Deleted" or "Inserted" when hovered, and a change to only a block's id isn't shown.
1 parent b2ceae6 commit 32ccc8f

13 files changed

Lines changed: 408 additions & 72 deletions

File tree

‎examples/07-collaboration/14-suggestion-gallery/src/App.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,8 @@ function VersionMerge({
415415
fragment: setup.afterDoc.get("doc"),
416416
provider: { awareness: setup.diffAwareness },
417417
user: USER_A,
418+
// The gallery shows the experimental diff refinements.
419+
experimental: { lostContentAttribution: true },
418420
},
419421
}),
420422
);

‎examples/07-collaboration/14-suggestion-gallery/src/scenarios.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1103,8 +1103,8 @@ export const scenarios: SuggestionScenario[] = [
11031103
id: "concurrent-move-into-deleted-block",
11041104
feedback: [
11051105
{
1106-
severity: "high",
1107-
note: "X is lost: B's move inserts a copy into Parent, which A deletes. Versioning shows X as deleted by B, though B only moved it, and doesn't show Child at all. To be fixed by #3166.",
1106+
severity: "info",
1107+
note: "X is lost: B's move inserts a copy into Parent, which A deletes. Versioning shows X deleted without an author: B only moved it, and A never saw it there.",
11081108
},
11091109
],
11101110
title: "Move a block into a block that is deleted",
@@ -1133,8 +1133,8 @@ export const scenarios: SuggestionScenario[] = [
11331133
id: "concurrent-delete-parent-vs-child-type",
11341134
feedback: [
11351135
{
1136-
severity: "high",
1137-
note: "B's type change is lost with Parent. Versioning shows Parent as deleted by A, but doesn't show Child at all. To be fixed by #3166.",
1136+
severity: "info",
1137+
note: "B's type change is lost with Parent. Versioning shows Parent deleted by A, and Child deleted without an author: B's change replaced Child with a copy that was lost, and the diff can't tell who to credit.",
11381138
},
11391139
],
11401140
title: "Delete a parent vs change its child's type",

‎packages/core/src/y/extensions/AttributionExtension.test.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,7 @@ describe("AttributionExtension user loading", () => {
297297
});
298298
});
299299

300-
// To be fixed by #3166.
301-
it.fails("does not show a change to a block's id", () => {
300+
it("does not show a change to a block's id", () => {
302301
const { editor } = createEditor();
303302
editor.replaceBlocks(editor.document, [
304303
{ type: "paragraph", content: "hello" },

‎packages/core/src/y/extensions/AttributionExtension.ts‎

Lines changed: 15 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -230,19 +230,16 @@ export const AttributionExtension = createExtension(
230230
.map((id) => userStore.getUser(id)?.username ?? id)
231231
.filter((username) => username !== "");
232232

233-
// A stable identity string for a wrapper (empty if unattributed), used to
234-
// (a) test whether a mark is attributed and (b) group adjacent marks with
235-
// the *same* attribution under one tooltip. It's an internal grouping key,
236-
// not the displayed text — that's composed in the view from `users` and
237-
// the format label — so it's built from raw `data-*` (ids + format keys)
238-
// and stays free of i18n/username resolution.
233+
// A stable identity string for a mark wrapper, used to group nested marks
234+
// with the *same* change under one tooltip. Every wrapper is a change;
235+
// its author may be unknown (e.g. content removed along with a
236+
// concurrently deleted block). It's an internal grouping key, not the
237+
// displayed text, so it's built from raw `data-*` and stays free of
238+
// i18n/username resolution.
239239
const attributionIdentity = (wrapper: HTMLElement) => {
240240
const ids = parseUserIds(wrapper.dataset["userIds"]);
241-
if (ids.length === 0 && wrapper.dataset["attributes"] === undefined) {
242-
return "";
243-
}
244241
const format = parseFormatKeys(wrapper.dataset["format"]);
245-
return `${wrapper.dataset["attributes"] ?? ""}:${format.join(",")}:${ids.join(",")}`;
242+
return `${wrapper.tagName}:${wrapper.dataset["attributes"] ?? ""}:${format.join(",")}:${ids.join(",")}`;
246243
};
247244

248245
// Build the tooltip state from a wrapper's `data-*` attributes. A
@@ -302,24 +299,6 @@ export const AttributionExtension = createExtension(
302299
store.setState(undefined);
303300
};
304301

305-
// The innermost attributed mark at or above `el`, skipping unattributed
306-
// wrappers so an attributed ancestor still wins.
307-
const innermostAttributed = (
308-
el: Element | null,
309-
): HTMLElement | undefined => {
310-
while (el) {
311-
const wrapper = el.closest<HTMLElement>(ATTRIBUTION_MARK_SELECTOR);
312-
if (!wrapper) {
313-
return undefined;
314-
}
315-
if (attributionIdentity(wrapper)) {
316-
return wrapper;
317-
}
318-
el = wrapper.parentElement;
319-
}
320-
return undefined;
321-
};
322-
323302
const nodeAttribution = (
324303
target: Element,
325304
): { mark: HTMLElement; preview: Element } | undefined => {
@@ -356,17 +335,18 @@ export const AttributionExtension = createExtension(
356335
) {
357336
return undefined;
358337
}
359-
const mark = Array.from(
360-
owner.querySelectorAll<HTMLElement>(ATTRIBUTION_MARK_SELECTOR),
361-
).find(attributionIdentity);
338+
const mark = owner.querySelector<HTMLElement>(
339+
ATTRIBUTION_MARK_SELECTOR,
340+
);
362341
return mark ? { mark, preview: owner } : undefined;
363342
};
364343

365344
const onPointerOver = (event: Event) => {
366345
const target = event.target instanceof Element ? event.target : null;
367346
const hoveredMark =
368347
target && dom.contains(target)
369-
? innermostAttributed(target)
348+
? (target.closest<HTMLElement>(ATTRIBUTION_MARK_SELECTOR) ??
349+
undefined)
370350
: undefined;
371351
const fallback =
372352
target && !hoveredMark ? nodeAttribution(target) : undefined;
@@ -379,21 +359,18 @@ export const AttributionExtension = createExtension(
379359

380360
const identity = attributionIdentity(innermost);
381361
// Anchor on the outermost ancestor with the *same* attribution so one
382-
// tooltip covers the whole region; a differently-attributed ancestor
383-
// breaks the chain, and unattributed ones are climbed past.
362+
// tooltip covers the whole region; a different ancestor breaks the chain.
384363
let anchor = innermost;
385364
let el: Element | null = innermost.parentElement;
386365
while (el) {
387366
const ancestor = el.closest<HTMLElement>(ATTRIBUTION_MARK_SELECTOR);
388367
if (!ancestor) {
389368
break;
390369
}
391-
const ancestorIdentity = attributionIdentity(ancestor);
392-
if (ancestorIdentity === identity) {
393-
anchor = ancestor;
394-
} else if (ancestorIdentity) {
370+
if (attributionIdentity(ancestor) !== identity) {
395371
break;
396372
}
373+
anchor = ancestor;
397374
el = ancestor.parentElement;
398375
}
399376

‎packages/core/src/y/extensions/Versioning.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
StyleSchema,
99
} from "../../schema/index.js";
1010
import { ForkYDocExtension } from "./ForkYDoc.js";
11+
import { CollaborationExtension } from "./index.js";
1112
import { serializeFragment } from "./snapshotCodec.js";
1213
import { showSnapshotPreview } from "./snapshotPreview.js";
1314

@@ -59,6 +60,7 @@ export function createYVersionView<
5960
content,
6061
comparison?.content,
6162
comparison?.attributions,
63+
editor.getExtension(CollaborationExtension)?.experimental,
6264
);
6365
},
6466
close() {

‎packages/core/src/y/extensions/YAttributionMarks.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,16 @@ const createAttributionMarkView =
112112
) =>
113113
({ mark, inline }: { mark: PMMark; inline: boolean }) => {
114114
const editor = options?.editor;
115+
// A block's id isn't content users edit, so a change to it isn't shown
116+
// (e.g. a diff pairing a deleted block with the editor's empty one).
117+
// Inline content may have an `id` prop of its own, which is.
118+
const { id: _id, ...blockChanges } = getAttributeChanges(mark);
119+
const changes = inline ? getAttributeChanges(mark) : blockChanges;
120+
if (type === "attrs" && Object.keys(changes).length === 0) {
121+
const dom = document.createElement(inline ? "span" : "div");
122+
dom.style.display = "contents";
123+
return { dom, contentDOM: dom };
124+
}
115125
// `<ins>`/`<del>` are semantic elements. The modification mark has no
116126
// dedicated element, so it renders as a `<span>` inline or a `<div>` over a
117127
// block, matching its `parseDOM` rules.
@@ -131,7 +141,7 @@ const createAttributionMarkView =
131141
});
132142
if (type === "attrs") {
133143
dom.dataset["type"] = "attributes";
134-
dom.dataset["attributes"] = JSON.stringify(getAttributeChanges(mark));
144+
dom.dataset["attributes"] = JSON.stringify(changes);
135145
}
136146
if (type === "modification") {
137147
dom.dataset["type"] = "modification";

‎packages/core/src/y/extensions/index.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { normalizeToUserStore, UserStoreOrResolver } from "../../user/index.js";
99
import { AttributionExtension } from "./AttributionExtension.js";
1010
import { ForkYDocExtension } from "./ForkYDoc.js";
1111
import { RelativePositionMappingExtension } from "./RelativePositionMapping.js";
12+
import type { ExperimentalVersionDiffs } from "./snapshotPreview.js";
1213
import { SuggestionsExtension } from "./Suggestions.js";
1314
import { CollaborationUser, YCursorExtension } from "./YCursorPlugin.js";
1415
import type { GetAttributionMarkClassName } from "./YAttributionMarks.js";
@@ -66,6 +67,12 @@ export type CollaborationOptions = {
6667
* dropped for that mark. See {@link GetAttributionMarkClassName}.
6768
*/
6869
getAttributionMarkClassName?: GetAttributionMarkClassName;
70+
71+
/**
72+
* Experimental refinements of how version history diffs are shown. They only
73+
* change what a diff shows, never what is stored.
74+
*/
75+
experimental?: ExperimentalVersionDiffs;
6976
};
7077

7178
export const CollaborationExtension = createExtension(
@@ -81,6 +88,7 @@ export const CollaborationExtension = createExtension(
8188
return {
8289
key: "collaboration",
8390
fragment: options.fragment,
91+
experimental: options.experimental ?? {},
8492
userStore,
8593
blockNoteExtensions: [
8694
options.suggestionDoc
@@ -129,6 +137,7 @@ export function withCollaboration<
129137
};
130138
}
131139

140+
export type { ExperimentalVersionDiffs } from "./snapshotPreview.js";
132141
export * from "./AttributionExtension.js";
133142
export * from "./DiffVersioningExtension.js";
134143
export * from "./ForkYDoc.js";

‎packages/core/src/y/extensions/snapshotCodec.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ export type DecodedFragment = {
1616
export function decodeFragmentUpdate(
1717
fragment: Y.Node,
1818
content: Uint8Array,
19-
opts?: { suggestionDoc?: boolean },
19+
opts?: { suggestionDoc?: boolean; keepDeleted?: boolean },
2020
): DecodedFragment {
21-
const doc = new Y.Doc(
22-
opts?.suggestionDoc ? { isSuggestionDoc: true } : undefined,
23-
);
21+
const doc = new Y.Doc({
22+
...(opts?.suggestionDoc ? { isSuggestionDoc: true } : {}),
23+
...(opts?.keepDeleted ? { gc: false } : {}),
24+
});
2425
try {
2526
Y.applyUpdateV2(doc, content);
2627
return { doc, fragment: findTypeInOtherYdoc(fragment, doc) };

0 commit comments

Comments
 (0)