Skip to content
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

wip: feat: comments #1376

Draft
wants to merge 103 commits into
base: main
Choose a base branch
from
Draft

wip: feat: comments #1376

wants to merge 103 commits into from

Conversation

YousefED
Copy link
Collaborator

@YousefED YousefED commented Jan 16, 2025

This PR adds comments to BlockNote!

  • We implement several Comment / Thread related components that can be customized similar to other BlockNote components.
  • The implementation is completely backend agnostic as the storage interface (ThreadStore) is pluggable. This means you can "bring your own provider" if you like.

See docs

ThreadStores

YjsThreadStore

The YjsThreadStore provides direct Yjs-based storage for comments, storing thread data directly in the Yjs document. This implementation is ideal for simple collaborative setups where all users have write access to the document.
The downside of this is that comments require a more advanced permission model than text documents, and Yjs doesn't provide a straightforward way to secure document updates.

RESTYjsThreadStore

The RESTYjsThreadStore combines Yjs storage with a REST API backend, providing secure comment management while maintaining real-time collaboration. This implementation is ideal when you have strong authentication requirements, but is a little more work to set up.

In this implementation, data is written to the Yjs document via a REST API which can handle access control. Data is still retrieved from the Yjs document directly (after it's been updated by the REST API), this way all comment information automatically syncs between clients using the existing collaboration provider.

We created a separate repository with an example server.

TipTap

We integrate directly with the TiptapCollabProvider with TiptapThreadStore.

LiveBlocks (later)

The UI Components are roughly based on the LiveBlocks open source components. LiveBlocks users will have two different ways to use LiveBlocks:

  • Use LiveBlocks UI components: this can be done with Feature/liveblocks v2 #1259. This would support all liveblocks functionality (read status, attachments, etc), but the style is a bit different from the other BlockNote components.
  • Use BlockNote UI components (matching your existing mantine / shadcn / ... version of BlockNote). These are "first-class" supported in BlockNote but miss some features compared to liveblocks (i.e.: read status, attachments, etc). We want to implement a LiveBlocksThreadStore for this, but this work is currently blocked (we can't store BlockNote documents describing comments in LiveBlocks)

TODO

TBD:

  • table cell, move block, and API operations
  • accessibility

Yjs next steps

  • overlapping marks (Yjs)
  • security of rest store
  • security of yjs store

To fix later

Important:

  • prevent outside modifications to the thread Y Map BlockNote-demo-nextjs-hocuspocus#1
  • comments on images and other block nodes
  • reposition on window resize
  • copy comment when cut+paste (optionally also with copy / paste?)
  • resolving newer comments deletes older already resolved ones at the same place. Probably a larger issue (yjs doesn't deal well with overlapping marks?)
  • show loading / error states in UI for interacting with comments
  • TBD: should the thread be deleted when the main (first) comment is removed?
  • TBD: add server side validation for YjsThreadStore (make sure users can't edit other users comments, etc). To make this work, we'd need to monitor updates in HocusPocus for example

Nice to haves:

  • cmd+enter to save?
  • Implement LiveBlocksThreadStore (and possibly move to separate package)
  • hide emojis when editing
  • badge ux (hover background / border)
  • close emoji popup (action bar) when adding reaction
  • better mouse over area for showing action toolbar
  • disable children (indentation) for comments content
  • implement anchored threads
  • positions of floating elements after window resize
  • expose an API for accessing comments (currently, comments don't appear in the BlockNote API)
  • when another user adds a reaction, your picker gets closed
  • when closing more actions popover by clicking the trigger button, the action toolbar stays visible when hovering something else

closes #695
closes #1109
closes #1148
closes #703
closes #1200

Copy link

vercel bot commented Jan 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Feb 26, 2025 5:07pm
blocknote-website ✅ Ready (Inspect) Visit Preview Feb 26, 2025 5:07pm

}

/* https://ariakit.org/components/hovercard */
/*.bn-ak-button {*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed?

.bn-toolbar.bn-ak-toolbar {
overflow-x: auto;
max-width: 100vw;
.bn-ak-toolbar {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that we don't care about the max-width anymore? That would cause it from overflowing on small screens.

* Store the positions of all threads in the document.
* this can be used later to implement a floating sidebar
*/
threadPositions: Map<string, { from: number; to: number }>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on some bugs in the suggestion plugin that will be related to this. It isn't safe to store positions like this. Would likely also need to keep around a relative position as well. But, we can figure this out later

Comment on lines +111 to +119
tr.removeMark(trimmedFrom, trimmedTo, markType);
tr.addMark(
trimmedFrom,
trimmedTo,
markType.create({
...mark.attrs,
orphan: isOrphan,
})
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be that you update the mark attr instead of removing & re-applying it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, I see there isn't an easy API for that. I just remember a step specifically for updating markattrs but isn't exposed on transactions for some reason


editor.onCreate(() => {
this.updateMarksFromThreads(this.threadStore.getThreads());
editor.onSelectionChange(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird to register this after the create hook, why not just register in the constructor separately?

Comment on lines +145 to +147
// Note: Plugins are currently not destroyed when the editor is destroyed.
// We should unsubscribe from the threadStore when the editor is destroyed.
this.threadStore.subscribe(this.updateMarksFromThreads);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we need an API for this

@@ -105,6 +105,7 @@
"@types/emoji-mart": "^3.0.14",
"@types/hast": "^3.0.0",
"@types/uuid": "^8.3.4",
"@hocuspocus/provider": "^2.15.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is peerOptional no?
https://stackoverflow.com/questions/62047806/how-do-i-set-a-peer-dependency-optional#66228639

TS might care for someone who imports it

Comment on lines +42 to +44
useEffect(() => {
editor.ForceSelectionVisible = !!state?.pendingComment;
}, [editor, state?.pendingComment]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird. Can we describe why this is needed, it also feels like it should not be handled by React, because it is being derived from the editor state?

Comment on lines +60 to +74
// Positioning with [data-bn-thread-id] attribute is a bit hacky,
// we could probably also use the thread position from the plugin state?
// for now, this works ok
const updateRef = useCallback(() => {
if (!state?.selectedThreadId) {
return;
}

const el = editor.domElement?.querySelector(
`[data-bn-thread-id="${state?.selectedThreadId}"]`
);
if (el) {
setReference(el);
}
}, [setReference, editor, state?.selectedThreadId]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely strange, I don't quite understand how this all works

Comment on lines +34 to +63
const handleFocusIn = (event: FocusEvent) => {
if (!focusedRef.current) {
_setFocused(true);
onFocus?.(event);
}
};

// eslint-disable-next-line react-hooks/exhaustive-deps
const handleFocusOut = (event: FocusEvent) => {
if (focusedRef.current && !containsRelatedTarget(event)) {
_setFocused(false);
onBlur?.(event);
}
};

useEffect(() => {
const node = ref.current;

if (node) {
node.addEventListener("focusin", handleFocusIn);
node.addEventListener("focusout", handleFocusOut);

return () => {
node?.removeEventListener("focusin", handleFocusIn);
node?.removeEventListener("focusout", handleFocusOut);
};
}

return undefined;
}, [handleFocusIn, handleFocusOut]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was copied over, but just wanted to say that this is strangely wasteful, it constantly adds and removes the event listeners when it doesn't need to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants