History and rollback for notebooks

[kasbah]

This adds an undo/redo side-bar and keyboard shortcuts (Ctrl-Z and Ctrl-Shift-Z).

An undo-able change-set is a set of changes made within 500ms of the last change: if you don't change anything for 500ms then the snapshot gets saved.

ui-component demo

Some things to consider:

• The DB migration takes a long time (3 minutes on the staging DB dump on my laptop)
• The undo history is global per-document meaning you will undo someone else's changes if you are working on the same doc.
• The undo history is stored as a tree but we present a flat history of one branch to the user.
• Due to how we handle the new cell popups we do have a strange interaction where undoing/re-doing to an empty cell creation will show the menu unexpectedly. We will need to fix this in the future, we should probably also not create the cell empty before the user picks what it's going to be.
• How this relates to the Automerge history is a little un-intuitive due to not being able to set an existing Automerge doc to particular "heads". I drew a picture:

[epatters]

The undo history is global per-document meaning you will undo someone else's changes if you are working on the same doc.

That, along with the user interface, makes me think we should describe this feature as "history" a la Google docs instead of undo-redo. What do you think?

[kasbah]

It is called that in the code already: "history" and "snapshots". Nothing is really calling this undo/redo except me and the undo & redo buttons on their tooltips (which are that and should definitely keep saying that).

To me the most important aspect of this feature is actually that I can Ctrl-Z when I mess up so that's why I describe it as undo/redo. I don't mind "history" or "undo history", I think they all describe what this does.

[jmoggr]

The asynchronous dance we do around debouncing changes and snapshots likely has at least one race condition and is otherwise really hard to reason about. The use of flags for asynchronous coordination is usually something that should be avoided in favor of primitives that are easier to follow and have better guarantees, this is not how asynchronous rust is meant to be written.

I admit I don't know a better way of writing this off the top of my head, debouncing makes this quite hard.

I could be wrong about the race conditions, but my inability to tell if there is a race condition should be enough of a concern.

---

The undo history is global per-document meaning you will undo someone else's changes if you are working on the same doc.

This is unfortunate. This can degenerate very easily: one user hits undo, but sees no change so hits undo a few more times, another user sees their change undone and either thinks the app is broken or starts hitting undo themselves. Either way both users report the app as broken.

I think that global undo/redo is better than no undo/redo, but I just want to flag that this will likely lead to occasional bug reports.

---

The intention has been to get rid of the JSON snapshots, we were fairly close to being able to remove them, but this turns them into a hard dependency. I think it's probably reasonable to keep them if they are a requirement for this feature, but I want to flag that we will no longer have the option of removing them.

This is an improvement on snapshotting, since we're only creating snapshots every 500ms instead of every change.

---

I'll take a couple hours this afternoon to experiment with alternative ways of handling the asynchronous parts and will follow up here. It could be the case that there is no better solution, but that would feel fairly bad.

[epatters]

Thanks for reviewing, Jason.

The intention has been to get rid of the JSON snapshots, we were fairly close to being able to remove them, but this turns them into a hard dependency. I think it's probably reasonable to keep them if they are a requirement for this feature, but I want to flag that we will no longer have the option of removing them.

I'm not yet tracking the technical details in this PR, but I'm surprised that this feature would depend on JSON snapshotting (even if we might want JSON snapshots around for other reasons, mainly to allow DB queries that reach into documents). Is the undo/rollback performed using JSON snapshots rather than the Automerge doc's own history?

[jmoggr]

Is the undo/rollback performed using JSON snapshots rather than the Automerge doc's own history

Yes. iirc Kaspar mentioned something about having difficulty getting the API to do what was needed.

[kasbah]

Please do check the code before you make assertions like that! I probably did say something like that before but I am using Automerge heads not the JSON snapshot now.

CatColab/packages/backend/src/document.rs

Lines 226 to 234 in 4af3b12

	doc_handle.with_document(|doc| -> Result<(), AppError> {
	doc.transact::<_, _, automerge::AutomergeError>(|tx| {
	copy_doc_at_heads(tx, &target_heads)?;
	Ok(())
	})
	.map_err(|e| AppError::Invalid(format!("Failed to update document: {e:?}")))?;
	Ok(())
	})?;

CatColab/packages/notebook-types/src/automerge_util.rs

Lines 5 to 36 in 4af3b12

	/// Overwrite the document root with the state at `target_heads`.
	///
	/// Unlike the `hydrate_to_json` + `populate_automerge_from_json` round-trip,
	/// this preserves rich-text marks and block markers on Text objects by using
	/// `marks_at` / `mark` instead of going through a plain-string intermediary.
	pub fn copy_doc_at_heads<'a>(
	tx: &mut automerge::transaction::Transaction<'a>,
	target_heads: &[automerge::ChangeHash],
	) -> Result<(), automerge::AutomergeError> {
	use automerge::ReadDoc;
	let current_keys: Vec<String> = tx.keys(automerge::ROOT).collect();
	for key in &current_keys {
	tx.delete(automerge::ROOT, key.as_str())?;
	}
	let target_entries: Vec<_> = {
	let keys: Vec<String> = tx.keys_at(automerge::ROOT, target_heads).collect();
	keys.into_iter()
	.filter_map(|key| {
	tx.get_at(automerge::ROOT, key.as_str(), target_heads)
	.ok()
	.flatten()
	.map(|(v, id)| (key, v.to_owned(), id))
	})
	.collect()
	};
	for (key, value, source_id) in &target_entries {
	put_value_into_map(tx, &automerge::ROOT, key, value, source_id, target_heads)?;
	}
	Ok(())
	}

[epatters]

That's a relief! And yeah, echoing the doc comment, if we went through JSON, rich text would be busted.