feat(Editor): support external editor instances#6678
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds external TipTap editor support to Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🧹 Nitpick comments (2)
test/components/Editor.spec.ts (1)
78-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the dev warning is actually emitted.
The spy silences
console.warnbut never verifies it was called, so the "inert props" warning behavior documented inEditor.vueisn't actually covered by this test.✅ Suggested assertion
warn.mockRestore() wrapper.unmount() + expect(warn).toHaveBeenCalledWith(expect.stringContaining('ignores'))🤖 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 `@test/components/Editor.spec.ts` around lines 78 - 92, The `Editor.spec.ts` test currently silences `console.warn` but never checks that the inert-props warning is emitted. In the `it('ignores engine props...')` case, keep the `vi.spyOn(console, 'warn')` on `warn`, then add an assertion that it was called when mounting `Editor` with `modelValue` and `contentType` alongside an external `editor`. Use the `Editor` component and the `warn` spy to verify the dev-only warning behavior documented in `Editor.vue`, then restore the spy as before.src/runtime/composables/useComponentProps.ts (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep composable exports on the
use*naming convention.
isPropBoundis a setup-context helper exported fromsrc/runtime/composables; rename it touseIsPropBound, or move it to a non-composable utils module if it should remain a predicate helper. As per coding guidelines,src/runtime/composables/**/*.ts: “Composables must useuse*naming convention with camelCase (e.g.,useFormField.ts).”Proposed rename
-export function isPropBound(name: string): boolean { +export function useIsPropBound(name: string): boolean { const vnode = getCurrentInstance()?.vnode if (!vnode || !vnode.props) return false return camelCase(name) in vnode.props || kebabCase(name) in vnode.props }🤖 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 `@src/runtime/composables/useComponentProps.ts` around lines 55 - 58, The exported setup helper currently violates the composables naming rule because `isPropBound` is exposed from `src/runtime/composables`; rename the exported function in `useComponentProps.ts` to `useIsPropBound` and update any internal references to match, or move it out of the composables folder if you want to keep the predicate-style name. Make sure the exported symbol follows the `use*` camelCase convention alongside the existing `getCurrentInstance`, `camelCase`, and `kebabCase` usage in that module.Source: Coding guidelines
🤖 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 `@docs/content/docs/2.components/editor.md`:
- Around line 180-183: Clarify the v-model ownership note in the editor
documentation: the current sentence in the note about the external editor owning
content/schema and v-model control sounds awkward. Rewrite the guidance around
the editor mode description to state plainly that in this mode the engine props
are ignored and the external editor instance must manage its own v-model,
instead of saying “not UEditor component.”
In `@src/runtime/components/Editor.vue`:
- Around line 302-305: The watchEffect in Editor.vue exits early when
externalEditor.value is set and props.editorProps becomes false, which leaves
previously injected Nuxt UI props attached. Update the editorProps handling in
this effect so that when props.editorProps is false after having been enabled,
you restore the captured original editor props on the external editor instance
before returning. Use the existing externalEditor and props.editorProps logic in
watchEffect to locate the change.
- Around line 150-162: The base editor class is being lost when `editorProps`
are merged because `defu()` preserves the first `attributes.class` it
encounters, so a user-supplied class overrides the Nuxt UI theme class. Update
`baseEditorProps` and the `editorProps` computed merge in `Editor.vue` to
combine `class` values before calling `defu`, ensuring both the default styling
from `ui.value.base` and any consumer-provided `editorProps.attributes.class`
are preserved.
---
Nitpick comments:
In `@src/runtime/composables/useComponentProps.ts`:
- Around line 55-58: The exported setup helper currently violates the
composables naming rule because `isPropBound` is exposed from
`src/runtime/composables`; rename the exported function in
`useComponentProps.ts` to `useIsPropBound` and update any internal references to
match, or move it out of the composables folder if you want to keep the
predicate-style name. Make sure the exported symbol follows the `use*` camelCase
convention alongside the existing `getCurrentInstance`, `camelCase`, and
`kebabCase` usage in that module.
In `@test/components/Editor.spec.ts`:
- Around line 78-92: The `Editor.spec.ts` test currently silences `console.warn`
but never checks that the inert-props warning is emitted. In the `it('ignores
engine props...')` case, keep the `vi.spyOn(console, 'warn')` on `warn`, then
add an assertion that it was called when mounting `Editor` with `modelValue` and
`contentType` alongside an external `editor`. Use the `Editor` component and the
`warn` spy to verify the dev-only warning behavior documented in `Editor.vue`,
then restore the spy as before.
🪄 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: f37f8e32-3d25-49f8-baef-49aebc2779aa
📒 Files selected for processing (4)
docs/content/docs/2.components/editor.mdsrc/runtime/components/Editor.vuesrc/runtime/composables/useComponentProps.tstest/components/Editor.spec.ts
commit: |
…ances Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
For transparency: both the last commit (2067be3) and this comment were drafted with Fable's (Claude) assistance, but reviewed by me. The diff looks bigger than it is — for normal What actually changed:
The Trade-offs to be aware of:
Big picture, this unlocks bringing your own TipTap instance (custom starter kits/schemas like comark-tiptap) while Happy to adjust any of this — WDYT @benjamincanac |
| data-slot="content" | ||
| :class="ui.content({ class: props.ui?.content })" | ||
| :class="[ui.content({ class: props.ui?.content }), ui.prose({ class: props.ui?.prose })]" | ||
| v-bind="isExternalEditor ? $attrs : undefined" |
There was a problem hiding this comment.
this is the only thing I'm not fully sure about. I added it because I do tend to use it when in a pinch, but feel free to change @benjamincanac (the v-bind="isExternalEditor ? $attrs : undefined" part)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/components/Editor.spec.ts (1)
65-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winShallow clone can mask nested mutations.
{ ...editor.options }only clones one level deep, sooriginalOptions.editorPropsand the post-mountcurrentOptions.editorPropsare the same object reference (editorProps). If the component mutateseditorProps.attributes.classin place (rather than replacing the object), this test won't catch it —toEqualwill trivially pass since both snapshots point to the identical mutated object.Use a deep clone for the "before" snapshot to make the mutation check meaningful.
♻️ Proposed fix using deep clone
const editorProps = { attributes: { class: 'custom-class' } } const editor = createEditor('<p>External content</p>', { editorProps }) // `options.element` is set by `EditorContent` when mounting the editor. - const { element: _, ...originalOptions } = { ...editor.options } + const { element: _, ...originalOptions } = structuredClone(editor.options) const wrapper = await mountSuspended(Editor, { props: { editor } }) expect(editor.options.editorProps).toBe(editorProps) const { element: __, ...currentOptions } = { ...editor.options } expect(currentOptions).toEqual(originalOptions)🤖 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 `@test/components/Editor.spec.ts` around lines 65 - 77, The mutation check in Editor.spec.ts is currently using a shallow copy of editor.options, which lets nested changes inside editorProps go unnoticed. Update the test around createEditor and mountSuspended(Editor) to take a deep snapshot of the pre-mount options instead of spreading editor.options, so the comparison against currentOptions will catch in-place mutations to nested fields like editorProps.attributes. Keep the existing assertions around editor.options.editorProps and the unmount flow, but make the “before” snapshot fully independent from the live options object.
🤖 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.
Nitpick comments:
In `@test/components/Editor.spec.ts`:
- Around line 65-77: The mutation check in Editor.spec.ts is currently using a
shallow copy of editor.options, which lets nested changes inside editorProps go
unnoticed. Update the test around createEditor and mountSuspended(Editor) to
take a deep snapshot of the pre-mount options instead of spreading
editor.options, so the comparison against currentOptions will catch in-place
mutations to nested fields like editorProps.attributes. Keep the existing
assertions around editor.options.editorProps and the unmount flow, but make the
“before” snapshot fully independent from the live options object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4b2f90e-d358-4e6f-86ba-c8ac41f9abf3
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Editor-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Editor.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/content/docs/2.components/editor.mdsrc/runtime/components/Editor.vuesrc/runtime/composables/useComponentProps.tssrc/theme/editor.tstest/components/Editor.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/runtime/composables/useComponentProps.ts
- docs/content/docs/2.components/editor.md
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
🔗 Linked issue
N/A, discussed privately on discord and surfaced in comarkdown/comark#164
❓ Type of change
📚 Description
<UEditor>builds its ownStarterKit, so there was no way to bring an extended one. This adds aneditorprop: pass your own TipTap instance and<UEditor>becomes a shell — theme, handlers and the child components (toolbar, menus) wire to it, while your editor owns the schema, content andv-model. Opt-in and non-breaking, the built-in path is untouched.The motivating case is editors backed by a custom kit like comark-tiptap (lossless markdown ↔ AST ↔ ProseMirror), which can't coexist with the built-in
StarterKit. Mode is detected on prop presence, so an editor created asynchronously (undefinedon first render) still works, with a#fallbackslot for that time window.One thing worth a second look before merge: in external mode we auto-inject the theme's base (prose) classes onto the editable via
editorProps. Editors that ship their own styling — comark included — could collide or double-up there, so the rendered result is worth an eyeball. There's an:editor-props="false"escape hatch for full control.📝 Checklist