fix(toolbar): give each toolbar separator its own item - #3932
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt Previous reviewsReview updated until commit 6b8d876 Results up to commit fb6e2e0
|
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@PopaSerban, thanks for the PR—this was a precise diagnosis and a clean fix. I added a focused regression test to the branch; it fails with the previous shared-separator implementation and passes with your change. The full SuperDoc package suite also passes locally with 2,493 tests.
Approving—thanks again for the contribution!
`ButtonGroup.vue` keys its v-for on item.id.value. default-items.js created a single `separator` item and reused that same object six times in the toolbar list; since separators default to the "center" group, all six landed in one ButtonGroup sharing one id, so a re-render for that group patched a list with six duplicate keys (Vue's "Duplicate keys found during update" warning). Each separator occurrence now gets its own item via makeSeparator().
fb6e2e0 to
6b8d876
Compare
|
@cubic-dev-ai review this PR after the rebase. |
@caio-pizzol I have started the AI code review. It will take a few minutes to complete. |
3 participants
Summary
ButtonGroupby givingeach separator in the built-in toolbar its own item, instead of reusing one
shared item six times (
packages/superdoc/src/internal/toolbar/built-in/default-items.js).UI layer only — no pipeline/document-data involvement.
Context
ButtonGroup.vuekeys itsv-foronitem.id.value.default-items.jscreated asingle
separatortoolbar item and spliced that same object into the toolbar listsix times; since separators default to the
centergroup, all six ended up in oneButtonGroupsharing oneid, so Vue re-render passes for that group patched alist with six duplicate keys.
The warning only surfaces on a re-render after the initial mount (Vue's duplicate-key
check runs in
patchKeyedChildren, not on first mount), which is why it doesn't showup right after page load — it needs a
toolbar-state-changeevent (e.g. clickinginto the document, changing selection, typing) to trigger the re-render.
Fix:
default-items.jsnow callsuseToolbarItem(...)fresh for each separatoroccurrence, giving each its own
id.packages/superdoc/src/internal/toolbar/built-in/default-items.jsseparatoritem →makeSeparator()factory, called at each of its 6 use sites