chore(migrate): useEditorMixin to useEditor composable #7313

max-nextcloud · 2025-06-16T12:25:48Z

No description provided.

codecov · 2025-06-16T13:26:32Z

Codecov Report

Attention: Patch coverage is 86.07595% with 77 lines in your changes missing coverage. Please review.

Project coverage is 59.66%. Comparing base (1ba51c5) to head (fabc79e).
Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/Suggestion/Mention/suggestions.js	16.66%	20 Missing ⚠️
src/extensions/CollaborationCursor.ts	13.33%	13 Missing ⚠️
src/composables/useEditorMethods.ts	50.00%	12 Missing ⚠️
src/editor.js	0.00%	9 Missing ⚠️
src/components/Menu/BaseActionEntry.js	33.33%	8 Missing ⚠️
src/components/Menu/ActionInsertLink.vue	14.28%	6 Missing ⚠️
src/components/Menu/ActionAttachmentUpload.vue	20.00%	4 Missing ⚠️
src/components/Menu/utils.js	42.85%	4 Missing ⚠️
src/components/Menu/EmojiPickerAction.vue	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7313      +/-   ##
==========================================
+ Coverage   59.22%   59.66%   +0.44%     
==========================================
  Files         481      486       +5     
  Lines       37149    37285     +136     
  Branches     1048     1097      +49     
==========================================
+ Hits        22000    22245     +245     
+ Misses      15047    14934     -113     
- Partials      102      106       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mejo-

Really nice work! I didn't properly test it but read through the code changes and have some questions and remarks.

mejo- · 2025-06-18T13:49:59Z

src/components/Assistant.vue

 		}

-		this.$editor.on('selectionUpdate', this.onSelection)
+		this.editor?.on('selectionUpdate', this.onSelection)


You now check in many places whether editor exists. This means that we cannot be sure that useEditor returns an editor object and always have to deal with editor being undefined? It looks a bit like an anti-pattern to me 🤔

If I get it right, then editor can be set by all the base components that call provideEditor() in setup() and then set this.editor in created() (i.e. BaseReader, MarkdownContentEditor and Editor). Since all consumers of useEditor() should be descendants (i.e child/granchild components) of the base editor components, editor should always already be set when used there, no?

There are two things that come into play here right now:

inject returns an optional value - i.e. can always be undefined. It's possible to cast it to always be defined (see last example in the docs for typing provide/inject ). That implies that the component will behave in unpredicted ways - most likely throwing exceptions when being used without provideing a value.

editor starts as undefined in the providing component such as Editor.vue. We only initialize it once we loaded the content.

2 is maybe artifact from how the content used to be loaded. I think we could be able to create the editor itself in the components setup routine these days. I'm a bit hesitant to changing the loading order right now as it feels fragile - but initializing the editor in the setup function rather than in loaded seems like it might even be a win for load times.

Thus far we hid these issues by only rendering subcomponents once the $editor was loaded. Whenever we removed the v-if='hasEditor' somewhere we'd get a lot of errors about this.$editor being undefined where it was not expected.

In addition the file that is rendered in an editor component never changes. If we switch between files in collective I believe the Editor component is unmounted and mounted again with a new value for the file related props. This is not how other components work, where one can just update a prop. This also means that for example the entire Menubar is recreated - with basically the same content. I was thinking that we might be able to keep the Editor component and change the props instead at some point. So far my assumption was that this would lead to editor being undefined while the new file is loaded. The ydoc that tracks the editing would also need to be replaced and it's part of the collaboration plugin config. But maybe we can even keep the editor around and "just" replace its content, the ydoc and the file related parts of the sync.

Anyway... I will take a look and see if I can initialize the editor in the setup function already. Then we might be able to do away with the ? everywhere.

src/components/CollisionResolveDialog.vue

mejo- · 2025-06-18T14:50:15Z

src/components/Editor.vue

-			if (this.$editor.isEditable !== editable) {
-				this.$editor.setEditable(editable)
-			}
+			this.setEditable(this.editMode && !this.requireReconnect)


Not directly related to this PR, but I just realized that we have a watcher for requireReconnect that calls setEditable(!this.requireReconnect), which smells a lot like conflicting with this one.

If this.editMode === false and this.requireReconnect switches to false, the editor is set to editable by the watcher, but any subsequent change will switch it back to read-only here in the onChange handler. Or this is purely theoretical? 🤔

It definitely seems messy. I think we should have a single computed that determines if the editor is editable and that we hand to the editor composable where it's watched and acted upon. That way the tiptap commands would live inside the composable only and the components would only deal with vue data structures.

src/components/Editor.vue

mejo- · 2025-06-18T14:53:07Z

src/components/Editor.vue

-					this.$editor = null
-					this.hasEditor = false
+					this.editor.destroy()
+					this.editor = undefined


I see that this is a moment when this.editor can become undefined. But since it's only called at the very end of beforeDestroy() it should not be a problem, no?

I agree. We can also just leave it and not set it to undefined. I was trying to replace this.hasEditor = false here. It hides a bunch of children. I wonder if they caused exceptions before because $editor was destroyed but they still existed.

mejo- · 2025-06-18T14:55:47Z

src/components/Editor.vue

 			this.translateModal = false
 		},
+		applyCommand(fn) {
+			this.editor?.commands?.command(fn)


This again has a little smell for me. We might expect applyCommand() to actually do what it's called to do and rely on it later. Just silently ignoring when this.editor.commands is not set looks potentially dangerous to me.

Thinking a bit more about it, I wonder whether such silent handling of undefined objects could contribute to the problem that we still have out-of-sync scenarios where steps didn't get applied.

I'll see if i can initialize the editor right in the setup function to avoid the whole problem all together.

src/composables/useEditorMethods.ts

mejo- · 2025-06-18T15:09:58Z

src/components/Editor/MarkdownContentEditor.vue

+	setup() {
+		const { editor } = provideEditor()
+		const { setEditable } = useEditorMethods(editor)
+		provide(editorFlagsKey, {


I don't understand this. Why not use providEditorFlags() from Editor.provider.ts here as well?

I'm trying to replace the inject that existed previously

[IS_RICH_EDITOR]: { get: () => true, },

But now the flags come in one object so I cannot set just one of them.

If we'd just use provideEditorFlags(props) isRichEditor might end up being false in particular if rich_editing_enabled is set to false.

But I will move this to a helper function in Editor.provider.js. I think that might be cleaner / more clear. Something like provideRichEditorFlag()

src/extensions/Session.ts