feat(Editor): support external editor instances - #6678
Conversation
|
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:
📝 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.
Ok, I've found what was not satisfying me about this change earlier, but fixed in 87b90b6.
If you, @benjamincanac, would prefer to drop the $attrs altogether I'm fine with that too
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>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Adds an “external editor” mode to <UEditor>, allowing consumers to provide their own TipTap Editor instance so the component becomes a shell that applies Nuxt UI theming and wires toolbars/menus without owning schema/content/lifecycle.
Changes:
- Added an
editorprop (supports refs) to use a provided TipTap editor instance, plus a#fallbackslot for SSR/async editor creation windows. - Introduced
prosestyling support for external editors by scoping theme typography/placeholder classes onto the content wrapper (with:where()to keep specificity low). - Expanded test coverage and updated snapshots/docs to validate external-editor behavior and styling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/components/Editor.spec.ts | Adds new assertions for built-in vs external editor behavior, styling, fallthrough attrs, and lifecycle expectations. |
| test/components/snapshots/Editor-vue.spec.ts.snap | Updates snapshots for the editor DOM/classes after styling/scoping changes. |
| src/theme/editor.ts | Refactors editor theme classes to support re-scoped “prose” styling on the content wrapper. |
| src/runtime/composables/useComponentProps.ts | Adds isPropBound helper to detect prop presence even when bound to undefined. |
| src/runtime/components/Editor.vue | Implements external-editor mode, fallback slot, inert-prop dev warnings, and wrapper-scoped prose styling. |
| docs/content/docs/2.components/editor.md | Documents external editor usage, fallback slot, and the prose opt-out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`d09668d75` only refreshed the nuxt snapshots, leaving the vue ones frozen at the intermediate state where prose classes lived on `content`. Now byte-identical to `v4`, confirming default rendering is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/runtime/components/Editor.vue:386
- In external-editor mode,
v-bind="$attrs"forwards a caller-supplieddata-slotonto the innerEditorContent, which can overwritedata-slot="content". The codebase convention is that a caller-supplieddata-slotshould only win on the component root; inner elements should keep their owndata-slotvalues.
:editor="editor"
data-slot="content"
:class="ui.content({ class: props.ui?.content })"
v-bind="isExternalEditor ? $attrs : undefined"
/>
docs/content/docs/2.components/editor.md:214
- This note says the external editor instance is "never modified", but mounting TipTap's
<EditorContent>necessarily assignseditor.options.elementwhile it is mounted (as also noted in the tests). Consider clarifying this to avoid surprising consumers who diff editor options.
The editor instance is never modified: the theme's [prose classes](#prose) style your content from a wrapper element, so an external editor inherits the default typography automatically. Attributes of the editable element itself remain yours — for parity with the built-in editor, set the theme's `base` slot classes at creation:
`v-bind="$attrs"` in external mode let a caller-supplied `data-slot` clobber `data-slot="content"` (Vue 3 merge order). Omit it to match the built-in path, which already strips it before injecting `editorProps` attributes — a fallthrough `data-slot` only renames the root. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Small disclaimer as some might see my latest commits with claude co-author: I'm on vacation without my laptop, but ended up needing a working pkg.pr.new release so I guided claude in what to edit exactly via mobile 🥲 EDIT: |
🔗 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