feat(editor): add block button for hovering blocks#14879
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds an "add block" widget: a new Lit component and constants, registers the element, shows/positions the widget from drag-handle pointer logic, inserts and focuses a new ChangesAdd Block Widget and Drag-handle Integration
Minor UI Tweak — Paragraph Heading Icon
Sequence DiagramsequenceDiagram
participant User
participant PointerWatcher as Pointer Event\nWatcher
participant DragHandle as Drag Handle
participant AddBlockWidget as Add Block\nWidget
participant BlockEditor as Block\nEditor
User->>PointerWatcher: hover over block
PointerWatcher->>DragHandle: reveal drag handle
PointerWatcher->>AddBlockWidget: set visible = true
PointerWatcher->>DragHandle: position add-block container
AddBlockWidget-->>User: render plus button
User->>AddBlockWidget: click plus button
AddBlockWidget->>DragHandle: dispatch "add-block" event
DragHandle->>DragHandle: _handleAddBlock() — insert paragraph via store
DragHandle->>BlockEditor: focus text model in new paragraph
DragHandle->>AddBlockWidget: hide widget (visible = false)
BlockEditor-->>User: new block focused and ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 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. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
blocksuite/affine/widgets/drag-handle/src/drag-handle.ts (1)
74-91: Avoid leaving an empty undo step whenaddBlockreturns falsy.
store.captureSync()is called unconditionally beforestore.addBlock(...). IfaddBlockfails or returns falsy (unlikely but handled by the early return), an empty history entry has already been captured andhide()is also skipped, so the drag-handle stays visible after a no-op. Either gatecaptureSync()on success or always callhide()to keep UI state consistent.♻️ Suggested adjustment
- store.captureSync(); const newBlockId = store.addBlock( 'affine:paragraph', {}, parent, index + 1 ); - if (!newBlockId) return; + if (!newBlockId) { + this.hide(); + return; + } + store.captureSync(); this.host.updateComplete .then(() => { focusTextModel(this.std, newBlockId); }) .catch(console.error); this.hide();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/widgets/drag-handle/src/drag-handle.ts` around lines 74 - 91, The current code calls store.captureSync() before store.addBlock(...), creating an empty undo step if addBlock returns falsy and leaving the drag handle visible; fix by calling store.addBlock first and if it returns falsy call this.hide() and return, otherwise call store.captureSync() and then proceed with this.host.updateComplete.then(()=> focusTextModel(this.std, newBlockId)).catch(console.error) so undo history is only captured for successful adds and UI is always hidden on failure.blocksuite/affine/widgets/drag-handle/src/components/add-block-widget.ts (1)
66-75: Use the shared icon from@blocksuite/icons/litinstead of an inline SVG.
@blocksuite/iconsis already a dependency of this package. ReusingPlusIcon(imported from@blocksuite/icons/lit) keeps icon styling/sizing consistent with the rest of the editor and avoids drift if the design system changes the glyph.♻️ Example refactor
-import { css, html, LitElement } from 'lit'; +import { PlusIcon } from '@blocksuite/icons/lit'; +import { css, html, LitElement } from 'lit'; @@ - <svg - xmlns="http://www.w3.org/2000/svg" - viewBox="0 0 12 12" - fill="currentColor" - aria-hidden="true" - > - <path - d="M6 1a.75.75 0 0 1 .75.75v3.5h3.5a.75.75 0 0 1 0 1.5h-3.5v3.5a.75.75 0 0 1-1.5 0v-3.5h-3.5a.75.75 0 0 1 0-1.5h3.5v-3.5A.75.75 0 0 1 6 1Z" - /> - </svg> + ${PlusIcon({ width: '12', height: '12' })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/widgets/drag-handle/src/components/add-block-widget.ts` around lines 66 - 75, Replace the inline SVG in add-block-widget's render with the shared PlusIcon from `@blocksuite/icons/lit`: remove the <svg> block and import PlusIcon from "@blocksuite/icons/lit", then render <PlusIcon /> where the SVG was, preserving accessibility attributes (aria-hidden or aria-label) and any sizing/className/styles used by the surrounding component (e.g., in AddBlockWidget or the component that contains the SVG). Ensure the import statement is added at the top and that existing CSS/props that control color/size still apply to PlusIcon so visual parity is maintained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/widgets/drag-handle/src/watchers/pointer-event-watcher.ts`:
- Around line 343-349: The add-block container
(.affine-add-block-widget-container / addBlockWidgetContainer) is sized to
draggingAreaRect.height and left displayed, so it continues to capture pointer
events even when the button isn't rendered; update the logic that
positions/updates addBlockWidgetContainer and AffineDragHandleWidget.hide() to
either set addBlockWidgetContainer.style.display = 'none' (and clear its inline
left/top/height) when showAddBlockWidget is false or when hide() is called (or
mode !== 'page'), or alternatively size the container to the button's intrinsic
height instead of draggingAreaRect.height (use the actual button element's
offsetHeight) so the container no longer spans the full block height; ensure any
inline styles left from previous shows are cleared when hiding to prevent a
lingering, pointer-capturing element.
---
Nitpick comments:
In `@blocksuite/affine/widgets/drag-handle/src/components/add-block-widget.ts`:
- Around line 66-75: Replace the inline SVG in add-block-widget's render with
the shared PlusIcon from `@blocksuite/icons/lit`: remove the <svg> block and
import PlusIcon from "@blocksuite/icons/lit", then render <PlusIcon /> where the
SVG was, preserving accessibility attributes (aria-hidden or aria-label) and any
sizing/className/styles used by the surrounding component (e.g., in
AddBlockWidget or the component that contains the SVG). Ensure the import
statement is added at the top and that existing CSS/props that control
color/size still apply to PlusIcon so visual parity is maintained.
In `@blocksuite/affine/widgets/drag-handle/src/drag-handle.ts`:
- Around line 74-91: The current code calls store.captureSync() before
store.addBlock(...), creating an empty undo step if addBlock returns falsy and
leaving the drag handle visible; fix by calling store.addBlock first and if it
returns falsy call this.hide() and return, otherwise call store.captureSync()
and then proceed with this.host.updateComplete.then(()=>
focusTextModel(this.std, newBlockId)).catch(console.error) so undo history is
only captured for successful adds and UI is always hidden on failure.
🪄 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: 3d76dae8-274e-4be3-94b0-a74143155583
📒 Files selected for processing (9)
blocksuite/affine/blocks/paragraph/src/heading-icon.tsblocksuite/affine/widgets/drag-handle/package.jsonblocksuite/affine/widgets/drag-handle/src/components/add-block-widget.tsblocksuite/affine/widgets/drag-handle/src/config.tsblocksuite/affine/widgets/drag-handle/src/consts.tsblocksuite/affine/widgets/drag-handle/src/drag-handle.tsblocksuite/affine/widgets/drag-handle/src/effects.tsblocksuite/affine/widgets/drag-handle/src/styles.tsblocksuite/affine/widgets/drag-handle/src/watchers/pointer-event-watcher.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## canary #14879 +/- ##
==========================================
- Coverage 58.61% 58.20% -0.42%
==========================================
Files 3190 3191 +1
Lines 175351 175394 +43
Branches 26020 25953 -67
==========================================
- Hits 102787 102091 -696
- Misses 69150 69860 +710
- Partials 3414 3443 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ate import path for focusTextModel
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/widgets/drag-handle/src/drag-handle.ts`:
- Around line 72-80: The code assumes parent.children.indexOf(block.model)
yields a valid position but that can be -1 if the hovered block moved or was
deleted; before calling store.addBlock (and after store.captureSync()), compute
index = parent.children.indexOf(block.model) and if index === -1 set a safe
fallback (e.g. insertIndex = parent.children.length to append after the current
children) or bail out; then call store.addBlock with insertIndex (or index + 1)
so you don't accidentally insert at the start when the anchor is stale.
🪄 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: c9f258d8-39fe-439d-8217-d3ff3e25aeba
📒 Files selected for processing (1)
blocksuite/affine/widgets/drag-handle/src/drag-handle.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
blocksuite/affine/widgets/drag-handle/src/drag-handle.ts (1)
261-266: Minor: Dual visibility mechanism for the add-block widget.The widget's visibility is controlled both via the
.visibleproperty (reactive) and by toggling the container'sdisplaystyle inhide()(imperative). This layered approach is defensive but ensure they stay synchronized—ifshowAddBlockWidgetbecomestruewhile the container hasdisplay: nonefrom a previoushide()call, the widget won't appear.The current flow looks correct since
hide()resets both, but worth noting for future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@blocksuite/affine/widgets/drag-handle/src/drag-handle.ts` around lines 261 - 266, The add-block widget visibility is controlled both by the .visible property (showAddBlockWidget) and by imperatively toggling the container's display in hide(); ensure these stay synchronized by updating the code path that shows the widget to also clear the container's display style (or remove the imperative toggle entirely). Concretely, when setting showAddBlockWidget = true (or in the method that makes the widget visible), also set the container element's style.display = '' (or appropriate visible value) so affine-add-block-widget's .visible and the container DOM style cannot diverge; alternatively consolidate to a single mechanism by removing the display toggle in hide() and relying solely on showAddBlockWidget/.visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocksuite/affine/widgets/drag-handle/src/drag-handle.ts`:
- Line 20: The file drag-handle.ts currently imports focusTextModel via a
relative internal path; update the import to use the package public API by
replacing the relative import with an import from '@blocksuite/affine-rich-text'
so that focusTextModel is imported from the public package export instead of
'../../../rich-text/src/dom.js'.
---
Nitpick comments:
In `@blocksuite/affine/widgets/drag-handle/src/drag-handle.ts`:
- Around line 261-266: The add-block widget visibility is controlled both by the
.visible property (showAddBlockWidget) and by imperatively toggling the
container's display in hide(); ensure these stay synchronized by updating the
code path that shows the widget to also clear the container's display style (or
remove the imperative toggle entirely). Concretely, when setting
showAddBlockWidget = true (or in the method that makes the widget visible), also
set the container element's style.display = '' (or appropriate visible value) so
affine-add-block-widget's .visible and the container DOM style cannot diverge;
alternatively consolidate to a single mechanism by removing the display toggle
in hide() and relying solely on showAddBlockWidget/.visible.
🪄 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: 517111c6-3eef-4853-9c12-6281cfafbd27
📒 Files selected for processing (1)
blocksuite/affine/widgets/drag-handle/src/drag-handle.ts
|
@darkskygit please review. |
darkskygit
force-pushed
the
canary
branch
5 times, most recently
from
3403237 to
78a9942
Compare
darkskygit
changed the title
add block button for hovering blocks
|
need to |
|
@darkskygit do i need to change something here or the PR is ready to merge? |
this pr break some test, please fix them |
Ok, will do. |
2 participants
This PR implements [feature request] #14845
Summary by CodeRabbit
New Features
Style