-
-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: suggestions architecture WIP #1515
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
||
Another approach, would be to model what is currently being represented as attributes, but instead use marks similar to [`@handlewithcare/prosemirror-suggest-changes`'s schema](https://github.com/handlewithcarecollective/prosemirror-suggest-changes?tab=readme-ov-file#schema) where the content changes are represented as marks. This would allow flexibility in display (because of support for [markviews](https://prosemirror.net/docs/ref/#view.MarkView) which can decouple content from presentation). | ||
|
||
Though, I question whether it _should_ be the responsibility of `y-prosemirror` to be both finding the difference and modifying the content of the document to display those differences. It feels like `y-prosemirror` should be scoped to the integration of yjs & prosemirror, and this is somewhat out of scope for it. I would be okay with `y-prosemirror` providing utilities to find the `y-diff` between documents in the y-prosemirror format, but the responsibility of how that integrates with prosemirror should be the responsibility of the caller of that function, rather than, built into the `y-sync` plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this paragraph is key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this diff be represented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think the Delta format is a good starting point, but would need additional metadata.
Or, we can go with something more specific to Prosemirror since this would be implemented for y-prosemirror
.
We can talk about it 😄
|
||
This is a collection of scattered thoughts on the _diff_-erent aspects of diffing (sorry, had to), and their complications. | ||
|
||
### Keeping user intent, while diffing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was thinking about another edge-case earlier today - writing it down here to share and so I don't forget :)
- when a user is in "suggestion" mode, deleting a word, and adding the same word again should probably not result in a deletion and insertion
Kevin has already implemented a preliminary version of the `y-diff` between two documents. Specifically, by using `ychange` attributes to add metadata onto elements to indicate whether they were modified between the two versions. This approach works for simple documents at the moment, with some limitations around: | ||
|
||
- Certain features are missing (due to this being preliminary work) like | ||
- XYZ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be super interested in feedback here. I remember that the approach - albeit buggy in certain scenarios - is solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ychange does not:
- capture changes in attributes (e.g. heading level 1 to level 3)
- capture changes in marks on nodes (limitation in y-prosemirror as a whole)
I agree, that it is not a fundamental problem with the approach, which is why I tried to frame it as missing features
- Controlling the display of the diff: | ||
- y-prosemirror is controlling how the diffs are rendered so doing something like a stacked diff would not be possible | ||
- Trying to simplify changes to a more human understandable version as described in the HTML diffing section would not be possible, since y-prosemirror directly writes the changes it finds to elements | ||
- Inability to edit content while displaying diffs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an inherent limitation
|
||
### Changes as _attributes_ | ||
|
||
Kevin has already implemented a preliminary version of the `y-diff` between two documents. Specifically, by using `ychange` attributes to add metadata onto elements to indicate whether they were modified between the two versions. This approach works for simple documents at the moment, with some limitations around: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this is basically what you are looking for. You could compute a y-diff based on the ychange attributes. That might make sense, so you can compute a more optimized diff (e.g. filtering out non-edits like delete x • insert x
) and to get more control over how the diff is rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check whether tracked changes within a document also is limited to this or not
adr/2025_03_10-suggestions/README.md
Outdated
|
||
Versioning has already been implemented for Y.js & y-prosemirror, but to be explicit about the sort of thing we are looking for it is good to explicitly state the feature set versioning should have: | ||
|
||
- A user can take _snapshots_ of the current document state, and store that in a separate data store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These snapshots can be taken at regular intervals, or when user explicitly chooses to save a specific "save"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to the doc, the options on how to create these snapshot (from a user-perspective)
- The content that was changed (i.e. additions or deletions) | ||
- Updates to content (i.e. changing an attribute) | ||
- Which user identifiers modified the content | ||
- Ideally, a timestamp of when the content was modified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flesh out timestamp means and at what granularity (typed first keystroke, or when snapshot was taken, or when user assumed a clientID)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be derived from snapshotted version?
### Cons | ||
|
||
- This stores the change in a separate data structure, which would need to be either maintained by mapping new positions through, or using relative positions. | ||
- Being in a different data structure presents risk for being invalidated by changes to the document (which may not have been intentional). Imagine moving a paragraph that had a suggestion on it, it could result in the suggestion pointing into an element which no longer exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
significantly different from versioning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And from existing mark-based approaches. relative positions have some edges cases to be figured out.
- This stores only the minimal representation of a change | ||
- Since it is stored separately | ||
- it is not possible to be "lost" due to changes in the editor content. | ||
- it can be permission-ed separately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to edit these changes? Store them completely separately in a separate data store
They ultimately result in the same in the final document but, the difference is in their semantics and user expectations. There are probably ways to simplify these changes in such a way that it is closer to what the user actually intended with the change (e.g. Prosemirror's [simplifyChanges](https://github.com/ProseMirror/prosemirror-changeset)), but it gets complicated rather quickly and we have to face that it may not always be easy to represent/display. | ||
|
||
Whatever the approach, we need to make sure that the changes are displayed in a way that is easy to understand and aligns with the user's intent. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make clear: this is a tradeoff of a nested element approach, but we are purposefully not choosing to implement a flat YText.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider other limitations of the diff-based approach.
- Possibly, undo-redo may change the result of the diff
- moving content shows large deletes & insertions
- Complicated to setup | ||
- Storage of the branched versions need to be kept up to date (i.e. must apply new updates since the branch) and need to store that document separately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, we could have a hybrid approach, where suggestions are stored into a different document (as a different history), but when in Prosemirror, we lie to Prosemirror & represent them as marks, this would allow us to re-use the approaches around tracked changes at the Prosemirror level, keep all of the benefits of marks just naturallly being transformed, and defer two sources of truth to a separate layer like y-prosemirror to sync the suggestions document and the root document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason being that it is quite unclear how to have 2 sources of truth for editing a single Prosemirror document, who is responsible for synchronizing the change? The problem with the editor having to be aware of this is that it potentially complicates all of the editor content to have to become "suggestion-aware", like if you are editing in a paragraph it has to choose where to save that change into.
To implement this, each user who suggests changes is given a separate document which they would have access to modify the contents of. This allows the user to own that document, while guaranteeing the original document stays unmodified. | ||
|
||
 | ||
|
||
When viewing what suggestions are available on a document, the frontend would have to "collect" the suggestions from each suggestion document, apply an updates those documents may not have seen yet, diff the documents to see what changes are in the suggestion document compared to the original document and finally display those changes to the user in the original document somehow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kevin suggested that we instead have a server-maintained "suggestions doc" where all suggestions would be recorded into. Rather than having to have something which collects these suggestions into one document.
## Cons | ||
|
||
- Adds complexity to displaying content in the editor, may need to rely on things like decorations to hide content we do not want to display | ||
- Since it is not a separate document, the permissions have to be the same as having write permissions to the document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main trade-off of this approach, is around having a suggestions-only user (without having to resort to some server undoing logic, inspecting the changes).
Theoretically, to get around this, you could use cryptography to sign the content of these changes to ensure that they have not been tampered with across editors, but a suggestion user would still have enough permissions to write into the document when they should not be able to.
Perhaps you can offload this to the server so that it is the only entity able to write these marks into the document.
- [Storing document _patches_ and later applying that patch to a document](./suggestions-as-patches.md) | ||
- [Suggestions as _document content_ which stores the changes within the document](./suggestions-as-content.md) | ||
|
||
### AttributionManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kevin will write more about the attribution manager, but to give a summary of how it will work:
- The AttributionManager will sit on the server and be able to attribute updates & elements in a trusted store (the server), to specific users. Those users will be atrtributed to specific clientIDs & clocks.
- Technically, a user can re-use another's, clientID, so the attribution manager must take care to actually known at what clock the change was made, to correctly identify the user.
- The attribution manager will likely be implemented in a network-agnostic way similar to the awareness protocol, so that clients can receive the attribution of users from the server.
Related to versioning, I think Lix is doing a really great job at this: https://lix.opral.com/ |
This is an Architecture Decision Record (ADR) that describes how & why we are choosing to implement suggestions and versioning for BlockNote.