perf(editor-toolbar): reduce rerenders in UEditorToolbar#6294
perf(editor-toolbar): reduce rerenders in UEditorToolbar#6294
Conversation
…mponent and memoize item state
…t to remove unused import
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes optimize the Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/runtime/components/EditorToolbar.vue (1)
208-218: Avoid repeated linear scans in the slot-state helpers.
itemKeyMapalready gives you the stable key, butgetRenderEntry()then walks every group again. A slotted toolbar that callsisActive()/isDisabled()per item turns these lookups into repeated O(n) scans during render, which gives back some of the perf win this refactor is trying to unlock.♻️ Sketch of the `Map`-based lookup
+const renderEntryMap = computed(() => { + const map = new Map<string, ToolbarRenderEntry>() + for (const group of renderGroups.value) { + for (const entry of group) { + map.set(entry.key, entry) + } + } + return map +}) + function getRenderEntry(key: string) { - for (const group of renderGroups.value) { - for (const entry of group) { - if (entry.key === key) { - return entry - } - } - } + return renderEntryMap.value.get(key) }Also applies to: 439-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/EditorToolbar.vue` around lines 208 - 218, The slot-state helpers are performing repeated O(n) scans because getRenderEntry() re-walks groups even though itemKeyMap holds stable keys; change the approach to build a direct Map lookup (e.g., entryByItem or entryByKey) when computing itemKeyMap so getRenderEntry(), isActive(), and isDisabled() can do constant-time lookups; update the computed that builds itemKeyMap (and the related computed at lines 439-465) to also populate a Map from item (or key) to the render entry and have getRenderEntry() read from that Map instead of iterating groups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/EditorToolbar.vue`:
- Line 99: The import statement currently mixes a runtime import with a type
import; change "import { computed, inject, shallowRef, watch, type ShallowRef }
from 'vue'" into two lines so runtime symbols (computed, inject, shallowRef,
watch) remain in the first import and the type ShallowRef is imported using
"import type { ShallowRef } from 'vue'"; update the top of the script where
computed, inject, shallowRef, watch and ShallowRef are referenced to use these
two separate imports.
In `@src/runtime/components/EditorToolbarItemRenderer.vue`:
- Around line 26-53: The template in EditorToolbarItemRenderer.vue renders
UDropdownMenu, UTooltip, and UButton nodes but currently omits the repo-standard
data-slot attributes; update the template to add data-slot markers on each
rendered element so selectors can target them (e.g., add data-slot="dropdown" on
the UDropdownMenu root, data-slot="tooltip" on each UTooltip node, and
data-slot="button" on each UButton), ensuring both the dropdown-branch
(UDropdownMenu with nested UTooltip/ UButton) and the non-dropdown branches
(UTooltip and plain UButton) receive the appropriate data-slot attributes and
keep existing bindings/props like dropdownProps, buttonProps, tooltip and the
`@click` handler unchanged.
---
Nitpick comments:
In `@src/runtime/components/EditorToolbar.vue`:
- Around line 208-218: The slot-state helpers are performing repeated O(n) scans
because getRenderEntry() re-walks groups even though itemKeyMap holds stable
keys; change the approach to build a direct Map lookup (e.g., entryByItem or
entryByKey) when computing itemKeyMap so getRenderEntry(), isActive(), and
isDisabled() can do constant-time lookups; update the computed that builds
itemKeyMap (and the related computed at lines 439-465) to also populate a Map
from item (or key) to the render entry and have getRenderEntry() read from that
Map instead of iterating groups.
🪄 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: 1e709767-314f-4d05-94e6-3197a5fa79be
📒 Files selected for processing (3)
src/runtime/components/EditorToolbar.vuesrc/runtime/components/EditorToolbarItemRenderer.vuetest/components/EditorToolbar.spec.ts
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/runtime/components/EditorToolbar.vue (1)
408-410: Consider the cost of deep watching on large items arrays.The
deep: truewatcher onprops.itemswill traverse the entire items structure on every change. For typical toolbar configurations this is fine, but if items are dynamically generated or frequently mutated, this could become a bottleneck.Since the PR objective focuses on transaction-driven lag (which is correctly addressed), this is acceptable for now. If items mutation becomes a concern, consider using a stable items reference from the parent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/EditorToolbar.vue` around lines 408 - 410, The watcher on props.items uses deep: true which will recursively traverse the entire items structure on every change and can be expensive for large or frequently-mutated arrays; to fix, avoid deep watching by either relying on a stable items reference passed from the parent (so a shallow watcher on () => props.items is sufficient) or replace the deep watcher with a watcher on a computed key that changes only when the items identity or relevant metadata change, and update renderGroups.value by calling buildRenderGroups() inside that watcher (references: props.items, renderGroups, buildRenderGroups).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/EditorToolbar.vue`:
- Around line 408-410: The watcher on props.items uses deep: true which will
recursively traverse the entire items structure on every change and can be
expensive for large or frequently-mutated arrays; to fix, avoid deep watching by
either relying on a stable items reference passed from the parent (so a shallow
watcher on () => props.items is sufficient) or replace the deep watcher with a
watcher on a computed key that changes only when the items identity or relevant
metadata change, and update renderGroups.value by calling buildRenderGroups()
inside that watcher (references: props.items, renderGroups, buildRenderGroups).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee5bc2de-912f-43d6-8311-fbf4e47901bd
📒 Files selected for processing (1)
src/runtime/components/EditorToolbar.vue
Jannchie
changed the title
benjamincanac
left a comment
benjamincanac
left a comment
There was a problem hiding this comment.
Sorry but I will not be merging this, you made way too many changes for no apparent reason it's impossible to review.
Sorry about that, most of the changes were driven by lint fixes and CodeRabbitAI suggestions rather than functional changes. I’m happy to put together a much smaller, more focused PR instead. |
…redundant editor serialization
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/runtime/components/EditorToolbar.vue (1)
247-253: Consider disabling items when their handler is missing.When an item has a
kindbut no corresponding handler is registered, the function returns{ active: false, disabled: false }. This allows the button to appear clickable even thoughonClickat line 285-288 will silently do nothing when no handler exists.Consider returning
disabled: truefor items with unregistered handlers to provide clearer feedback to users.♻️ Suggested change
const handler = handlers?.value?.[item.kind] if (!handler) { return { active: false, - disabled: false + disabled: true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/EditorToolbar.vue` around lines 247 - 253, The code currently returns { active: false, disabled: false } when const handler = handlers?.value?.[item.kind] is missing, leaving buttons appearing enabled while onClick (the handler call around lines 285-288) will do nothing; change the early-return to set disabled: true (e.g., return { active: false, disabled: true }) so items with no registered handler are rendered disabled and provide correct UX feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/EditorToolbar.vue`:
- Around line 247-253: The code currently returns { active: false, disabled:
false } when const handler = handlers?.value?.[item.kind] is missing, leaving
buttons appearing enabled while onClick (the handler call around lines 285-288)
will do nothing; change the early-return to set disabled: true (e.g., return {
active: false, disabled: true }) so items with no registered handler are
rendered disabled and provide correct UX feedback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75c3e927-2281-4aa7-bd86-b8be7e466f46
📒 Files selected for processing (2)
src/runtime/components/EditorToolbar.vuetest/components/EditorToolbar.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test/components/EditorToolbar.spec.ts
Jannchie
force-pushed
the
perf/editor-toolbar-renderer
branch
from
6a97ef4 to
d369f5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/EditorToolbar.vue`:
- Around line 162-165: The toolbar-wide stateVersion causes full-component
rerenders because isActive, isDisabled, and getDropdownItems read stateVersion
during render; change to per-item reactive versions by storing a Ref<number> per
item in itemStateCache (and similar per-item entry for dropdownItemsCache) and
use that per-item ref in isActive(item), isDisabled(item), and
getDropdownItems(item) so only the affected item updates. Replace the global
stateVersion.value++ scheduling with logic that finds/creates the item's version
ref in itemStateCache and increments that ref (or toggles a small per-item
computed) when that item's underlying state changes, ensuring the WeakMap keys
remain the item objects and the render dependencies are per-item rather than
toolbar-wide.
- Around line 384-410: Add a deep watcher for props.items that calls
scheduleStateRefresh to invalidate the versioned caches used by getItemState()
and getDropdownItems() when item objects are mutated in place; place it
alongside the existing watchers for handlers and props.editor so any in-place
changes to item.active, item.disabled, or nested item.items trigger
scheduleStateRefresh (use a deep:true watcher on props.items and call
scheduleStateRefresh in the callback).
🪄 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: 9eb51612-a706-4847-bef6-ffcb4f935126
📒 Files selected for processing (2)
src/runtime/components/EditorToolbar.vuetest/components/EditorToolbar.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/components/EditorToolbar.spec.ts
| watch(() => handlers.value, () => { | ||
| scheduleStateRefresh() | ||
| }, { deep: true }) | ||
|
|
||
| watch(() => props.editor, (editor, _, onCleanup) => { | ||
| scheduleStateRefresh() | ||
|
|
||
| if (typeof (editor as any)?.on !== 'function' || typeof (editor as any)?.off !== 'function') { | ||
| return | ||
| } | ||
|
|
||
| const onStateChange = () => { | ||
| scheduleStateRefresh() | ||
| } | ||
|
|
||
| editor.on('selectionUpdate', onStateChange) | ||
| editor.on('focus', onStateChange) | ||
| editor.on('blur', onStateChange) | ||
| editor.on('transaction', onStateChange) | ||
|
|
||
| onCleanup(() => { | ||
| editor.off('selectionUpdate', onStateChange) | ||
| editor.off('focus', onStateChange) | ||
| editor.off('blur', onStateChange) | ||
| editor.off('transaction', onStateChange) | ||
| }) | ||
| }, { immediate: true }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/runtime/components/EditorToolbar.vue | head -450 | tail -100Repository: nuxt/ui
Length of output: 3790
🏁 Script executed:
wc -l src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 95
🏁 Script executed:
# Search for getItemState and getDropdownItems functions
rg -n "getItemState|getDropdownItems|scheduleStateRefresh" src/runtime/components/EditorToolbar.vue -A 3Repository: nuxt/ui
Length of output: 1833
🏁 Script executed:
# Look at the full structure of watchers and cache-related code
rg -n "watch\(|cache\.|computed\(" src/runtime/components/EditorToolbar.vue | head -30Repository: nuxt/ui
Length of output: 465
🏁 Script executed:
# Look at scheduleStateRefresh and stateVersion
rg -n "scheduleStateRefresh|stateVersion|itemStateCache|dropdownItemsCache" src/runtime/components/EditorToolbar.vue -B 2 -A 5 | head -80Repository: nuxt/ui
Length of output: 2224
🏁 Script executed:
# Check if there are any other watchers or computed properties that depend on props.items
rg -n "props\.items" src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 183
🏁 Script executed:
# Look at the entire getItemState function
sed -n '191,245p' src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 1489
🏁 Script executed:
# Get the full groups computed implementation
sed -n '149,160p' src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 275
🏁 Script executed:
# Look at the full context around resolveItemState to see all dependencies
sed -n '209,270p' src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 1610
🏁 Script executed:
# Check if there are prop definitions and documentation about mutating items
rg -n "props\." src/runtime/components/EditorToolbar.vue | grep -E "items|defineProps" -A 5Repository: nuxt/ui
Length of output: 505
🏁 Script executed:
# Look for prop definitions at the top of the file
sed -n '1,120p' src/runtime/components/EditorToolbar.vue | grep -A 20 "defineProps\|items"Repository: nuxt/ui
Length of output: 938
🏁 Script executed:
# Check the related type definitions to understand item structure
rg -n "EditorToolbarItem|EditorToolbarDropdownItem" src/runtime/components/EditorToolbar.vue | head -20Repository: nuxt/ui
Length of output: 1642
🏁 Script executed:
# Look for any test files or usage examples that might show in-place mutations
fd -e "spec\|test" . --path "*EditorToolbar*"Repository: nuxt/ui
Length of output: 284
🏁 Script executed:
# Search for any documentation about item mutations
rg -n "mutate|mutation" src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 33
🏁 Script executed:
# Get the type definitions for EditorToolbarItem to see what properties can be mutated
sed -n '1,50p' src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 2057
🏁 Script executed:
# Check how groups computed is used in the template to understand reactivity
rg -n "groups" src/runtime/components/EditorToolbar.vueRepository: nuxt/ui
Length of output: 224
🏁 Script executed:
# Look at the ButtonItem and EditorItem types to see what mutable properties exist
rg -n "type ButtonItem|type EditorItem" src/runtime/components/EditorToolbar.vue -A 5Repository: nuxt/ui
Length of output: 188
🏁 Script executed:
# Find EditorItem type definition
rg -n "type EditorItem" src/runtime/types/editor.ts -A 10Repository: nuxt/ui
Length of output: 771
🏁 Script executed:
# Find ButtonProps definition to see all properties
rg -n "type ButtonProps" src/runtime/types/ -A 15Repository: nuxt/ui
Length of output: 33
🏁 Script executed:
# Find ButtonProps definition more broadly
rg -n "export type ButtonProps|export interface ButtonProps" src/runtime/types/ -A 20 | head -50Repository: nuxt/ui
Length of output: 33
🏁 Script executed:
# Check what properties ButtonItem includes from ButtonProps
rg -n "active|disabled" src/runtime/types/index.ts | head -20Repository: nuxt/ui
Length of output: 33
🏁 Script executed:
# Search for ButtonProps in button component
rg -n "ButtonProps" src/runtime/types/index.ts -B 2 -A 10Repository: nuxt/ui
Length of output: 33
🏁 Script executed:
# Check the button component itself for prop definitions
fd -name "Button.vue" src/runtime/components/Repository: nuxt/ui
Length of output: 281
🏁 Script executed:
# Let's look at types directory structure
ls -la src/runtime/types/ | head -20Repository: nuxt/ui
Length of output: 806
🏁 Script executed:
# Look at index.ts for ButtonProps
rg -n "ButtonProps" src/runtime/types/index.ts -A 10Repository: nuxt/ui
Length of output: 33
🏁 Script executed:
# Check if active/disabled are directly on ButtonItem
rg -n "active|disabled" src/runtime/components/EditorToolbar.vue | head -30Repository: nuxt/ui
Length of output: 1489
Add watcher for props.items to invalidate caches on item mutations.
The component caches getItemState() and getDropdownItems() results using version-based invalidation. Currently, only changes to handlers and editor trigger cache invalidation via scheduleStateRefresh(). If a consumer mutates item properties in place—such as toggling item.active, item.disabled, or modifying item.items—the component will rerender while reusing stale cached state, leaving toolbar state out of sync until the next editor event.
🔧 Minimal fix
watch(() => handlers.value, () => {
scheduleStateRefresh()
}, { deep: true })
+watch(() => props.items, () => {
+ scheduleStateRefresh()
+}, { deep: true })
+
watch(() => props.editor, (editor, _, onCleanup) => {
scheduleStateRefresh()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/EditorToolbar.vue` around lines 384 - 410, Add a deep
watcher for props.items that calls scheduleStateRefresh to invalidate the
versioned caches used by getItemState() and getDropdownItems() when item objects
are mutated in place; place it alongside the existing watchers for handlers and
props.editor so any in-place changes to item.active, item.disabled, or nested
item.items trigger scheduleStateRefresh (use a deep:true watcher on props.items
and call scheduleStateRefresh in the callback).
9fbc6e4 to
07fbade
Compare
2 participants
🔗 Linked issue
Closes #6293
❓ Type of change
📚 Description
This change reduces interaction lag in
UEditorToolbarwhen many toolbar items are rendered.It preserves the existing toolbar behavior while narrowing the update boundary, so editor transactions no longer cause the entire toolbar to rerender.
To achieve this, the default toolbar item rendering has been split into an internal
EditorToolbarItemRenderercomponent, and reactive state is now stored per toolbar item instead of rebuilding all item render data in the parent. Toolbar state refreshes remain driven by editortransactionevents.A regression test was also added to ensure unrelated rerenders do not recompute handler state.
📝 Checklist
✅ Verification
pnpm eslint src/runtime/components/EditorToolbar.vue src/runtime/components/EditorToolbarItemRenderer.vue test/components/EditorToolbar.spec.tspnpm vitest test/components/EditorToolbar.spec.tspnpm typecheck