Fix multi user undo model - #4639
TrueDoctor wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
3 issues found across 9 files
Confidence score: 2/5
crdt.rschanges the serialized shape of everyDelta, but format v1 still reads history frames as the old shape. Existing documents may not reopen reliably; preserve compatibility when loading v1 history.document.rsremoves a peer’sRegisterPeermapping when undoing its first interaction, so later edits lose that peer’s user identity and same-user undo grouping breaks. Keep the mapping available for subsequent edits.session.rsattributes historical deltas using the peer’s current identity, so reassignment can make the new user appear to own the old user’s edits and undo the wrong work. Use the identity recorded for each historical interaction.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="document/graph-storage/src/crdt.rs">
<violation number="1" location="document/graph-storage/src/crdt.rs:20">
P1: Changing `reverse` changes the serialized shape of every `Delta`, but format v1 still loads history frames directly as `Delta`. Existing documents written by the previous build therefore cannot reliably reopen; add a versioned migration or backward-compatible decoder before changing this field.</violation>
</file>
<file name="document/graph-storage/src/document.rs">
<violation number="1" location="document/graph-storage/src/document.rs:82">
P2: Undoing a peer’s first interaction also removes its `RegisterPeer` mapping, but that peer will not re-register on later edits. Those edits then have no user identity here, breaking same-user undo grouping; keep peer registrations append-only when restoring delta priors.</violation>
</file>
<file name="document/graph-storage/src/session.rs">
<violation number="1" location="document/graph-storage/src/session.rs:401">
P2: This classifies historical deltas using the peer's current identity, so reassignment retroactively makes the new user appear to own the old user's interactions. After undoing their registration/edit, the new user can then undo the previous user's next interaction; retain authorship as of each delta or stop undo at peer-identity changes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| pub kind: RegistryDelta, | ||
| pub reverse: RegistryDelta, | ||
| /// What every slot `kind` wrote held before it, stamps included, in write order. See [`Prior`]. | ||
| pub reverse: Vec<Prior>, |
There was a problem hiding this comment.
P1: Changing reverse changes the serialized shape of every Delta, but format v1 still loads history frames directly as Delta. Existing documents written by the previous build therefore cannot reliably reopen; add a versioned migration or backward-compatible decoder before changing this field.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/graph-storage/src/crdt.rs, line 20:
<comment>Changing `reverse` changes the serialized shape of every `Delta`, but format v1 still loads history frames directly as `Delta`. Existing documents written by the previous build therefore cannot reliably reopen; add a versioned migration or backward-compatible decoder before changing this field.</comment>
<file context>
@@ -16,7 +16,8 @@ pub struct Delta {
pub kind: RegistryDelta,
- pub reverse: RegistryDelta,
+ /// What every slot `kind` wrote held before it, stamps included, in write order. See [`Prior`].
+ pub reverse: Vec<Prior>,
/// Local, mutable annotations on this commit (interaction-end marker, future commit messages / labels).
/// Deliberately excluded from `compute_rev`: relabeling a commit must not change its content-addressed
</file context>
| } | ||
| /// Undo `delta` by putting back what it overwrote: in the snapshot, and in the working registry when nothing is hot. | ||
| pub(crate) fn revert_delta(&mut self, delta: &Delta) { | ||
| crate::prior::restore(&mut self.retired_snapshot, &delta.reverse); |
There was a problem hiding this comment.
P2: Undoing a peer’s first interaction also removes its RegisterPeer mapping, but that peer will not re-register on later edits. Those edits then have no user identity here, breaking same-user undo grouping; keep peer registrations append-only when restoring delta priors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/graph-storage/src/document.rs, line 82:
<comment>Undoing a peer’s first interaction also removes its `RegisterPeer` mapping, but that peer will not re-register on later edits. Those edits then have no user identity here, breaking same-user undo grouping; keep peer registrations append-only when restoring delta priors.</comment>
<file context>
@@ -78,17 +77,12 @@ impl Document {
- }
+ /// Undo `delta` by putting back what it overwrote: in the snapshot, and in the working registry when nothing is hot.
+ pub(crate) fn revert_delta(&mut self, delta: &Delta) {
+ crate::prior::restore(&mut self.retired_snapshot, &delta.reverse);
+ if self.hot_log.is_empty() {
+ crate::prior::restore(&mut self.working_registry, &delta.reverse);
</file context>
| fn interaction_start_parent(&self, end: Rev, silent: bool) -> Option<Rev> { | ||
| let history = &self.document.history; | ||
| let mut current = history.get(end)?; | ||
| let user = self.user_of(current.author); |
There was a problem hiding this comment.
P2: This classifies historical deltas using the peer's current identity, so reassignment retroactively makes the new user appear to own the old user's interactions. After undoing their registration/edit, the new user can then undo the previous user's next interaction; retain authorship as of each delta or stop undo at peer-identity changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At document/graph-storage/src/session.rs, line 401:
<comment>This classifies historical deltas using the peer's current identity, so reassignment retroactively makes the new user appear to own the old user's interactions. After undoing their registration/edit, the new user can then undo the previous user's next interaction; retain authorship as of each delta or stop undo at peer-identity changes.</comment>
<file context>
@@ -387,33 +387,29 @@ impl Session {
+ fn interaction_start_parent(&self, end: Rev, silent: bool) -> Option<Rev> {
+ let history = &self.document.history;
+ let mut current = history.get(end)?;
+ let user = self.user_of(current.author);
+ if matches!(current.kind, RegistryDelta::Merge { .. }) || (silent && user != Some(self.document.user)) {
+ return None;
</file context>
edbac74 to
7f1228b
Compare
7f1228b to
057cf3e
Compare
057cf3e to
208c86a
Compare
No description provided.