feat(comments): confirm before discarding an unsaved comment#2861
Open
YousefED wants to merge 2 commits into
Open
feat(comments): confirm before discarding an unsaved comment#2861YousefED wants to merge 2 commits into
YousefED wants to merge 2 commits into
Conversation
When the new-comment composer is dismissed (e.g. by clicking outside or pressing Escape) while it contains unsaved text, show a confirmation prompt before discarding it. Previously the comment was lost silently. The composer editor is now created and owned by FloatingComposerController so the dismiss handler can check whether the user has typed anything, and a translatable `comments.discard_pending_comment` string is added to all locales. Addresses #2742 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthrough
ChangesDiscard Pending Comment Confirmation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react/src/components/Comments/FloatingComposerController.tsx (1)
83-97: ⚡ Quick winAdd regression tests for the new dismiss-confirm flow.
Please add coverage for: (1) empty draft dismisses without prompt, (2) non-empty draft + confirm accepts discard, (3) non-empty draft + confirm cancel keeps composer open with text intact.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/src/components/Comments/FloatingComposerController.tsx` around lines 83 - 97, Add three regression tests for the onOpenChange handler in FloatingComposerController to cover the dismiss-confirm flow: first test that an empty draft (when newCommentEditor.isEmpty is true) dismisses the composer without prompting the user, second test that when a non-empty draft exists and the user confirms the discard prompt (window.confirm returns true), the composer closes and stopPendingComment is called, and third test that when a non-empty draft exists and the user cancels the discard prompt (window.confirm returns false), the composer remains open and editor.focus is called while the draft text is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/src/components/Comments/FloatingComposer.tsx`:
- Around line 29-36: The `newCommentEditor` prop on the `FloatingComposer`
component has been made required, which is a breaking change for external
consumers. Either make the prop optional by adding a `?` to the type definition
and provide fallback behavior when it is not provided, or ensure this change is
documented as a semver-major release with clear migration guidance for users
currently rendering `FloatingComposer` directly.
---
Nitpick comments:
In `@packages/react/src/components/Comments/FloatingComposerController.tsx`:
- Around line 83-97: Add three regression tests for the onOpenChange handler in
FloatingComposerController to cover the dismiss-confirm flow: first test that an
empty draft (when newCommentEditor.isEmpty is true) dismisses the composer
without prompting the user, second test that when a non-empty draft exists and
the user confirms the discard prompt (window.confirm returns true), the composer
closes and stopPendingComment is called, and third test that when a non-empty
draft exists and the user cancels the discard prompt (window.confirm returns
false), the composer remains open and editor.focus is called while the draft
text is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 154a44d5-61f2-4cd3-affc-5f97fed84022
📒 Files selected for processing (25)
packages/core/src/i18n/locales/ar.tspackages/core/src/i18n/locales/de.tspackages/core/src/i18n/locales/en.tspackages/core/src/i18n/locales/es.tspackages/core/src/i18n/locales/fa.tspackages/core/src/i18n/locales/fr.tspackages/core/src/i18n/locales/he.tspackages/core/src/i18n/locales/hr.tspackages/core/src/i18n/locales/is.tspackages/core/src/i18n/locales/it.tspackages/core/src/i18n/locales/ja.tspackages/core/src/i18n/locales/ko.tspackages/core/src/i18n/locales/nl.tspackages/core/src/i18n/locales/no.tspackages/core/src/i18n/locales/pl.tspackages/core/src/i18n/locales/pt.tspackages/core/src/i18n/locales/ru.tspackages/core/src/i18n/locales/sk.tspackages/core/src/i18n/locales/uk.tspackages/core/src/i18n/locales/uz.tspackages/core/src/i18n/locales/vi.tspackages/core/src/i18n/locales/zh-tw.tspackages/core/src/i18n/locales/zh.tspackages/react/src/components/Comments/FloatingComposer.tsxpackages/react/src/components/Comments/FloatingComposerController.tsx
Comment on lines
+29
to
+36
| >(props: { | ||
| /** | ||
| * The (empty) editor used to compose the new comment. Created and owned by | ||
| * the `FloatingComposerController`, so it can check for unsaved text before | ||
| * the composer is dismissed. | ||
| */ | ||
| newCommentEditor: BlockNoteEditor<any, any, any>; | ||
| }) { |
Contributor
There was a problem hiding this comment.
Public API break: newCommentEditor is now required on an exported component.
Making this prop mandatory can break existing external consumers that render FloatingComposer directly. Please either preserve backward compatibility (optional prop with fallback behavior) or explicitly ship this as a semver-major change with migration guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react/src/components/Comments/FloatingComposer.tsx` around lines 29
- 36, The `newCommentEditor` prop on the `FloatingComposer` component has been
made required, which is a breaking change for external consumers. Either make
the prop optional by adding a `?` to the type definition and provide fallback
behavior when it is not provided, or ensure this change is documented as a
semver-major release with clear migration guidance for users currently rendering
`FloatingComposer` directly.
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
Collaborator
Author
|
@virgile-dev See a demo here: https://blocknote-lxlgmdups-typecell.vercel.app/collaboration/comments-with-sidebar Two UX questions:
Internal (BlockNote): decide if we want to make this behavior configurable |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Show a confirmation prompt before discarding an unsaved comment in the new-comment composer, so users don't lose their text by accidentally clicking away.
Rationale
As raised in discussion #2742: when writing a new comment, clicking outside the composer (or pressing Escape) immediately discards it, silently losing any text the user typed. Full draft support was considered too complex; the agreed approach was to warn the user before discarding when text has been entered.
Changes
window.confirm()prompt asks the user before discarding. Cancelling keeps the composer open with the text intact; confirming discards as before. An empty composer closes silently, unchanged.FloatingComposerController(which owns the open/dismiss lifecycle) and passed down toFloatingComposer. This lets the dismiss handler checkisEmptydirectly. A fresh editor is created per pending comment, so it always starts empty.comments.discard_pending_commentstring to all 24 locales.Impact
FloatingComposeris a public export; its props change from{}to a requirednewCommentEditor. In practice it can only be used via the controller (it depends on it for the editor and positioning), so real-world impact is minimal.Testing
tsc -bpasses for@blocknote/coreand@blocknote/react.eslintpasses on the changed files.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit