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

Signed-off-by: Max <[email protected]>

The `Session` extension provides the `setSession` and `clearSession` commands. Session data is made available via `editor.storage.session`. This way session data can be provided after initializing the editor. Signed-off-by: Max <[email protected]>

Signed-off-by: Max <[email protected]>

Allow initializing the editor before knowing the session. Signed-off-by: Max <[email protected]>

Only used once. Signed-off-by: Max <[email protected]>

initialize editor before lowlight has been loaded Wait with loading the actual content to ensure syntax highlighting is available. Signed-off-by: Max <[email protected]>

Cannot load it in setup yet as it requires the yjs provider, which in turn requires the sync service, which is only initialized in mounted. Signed-off-by: Max <[email protected]>

Signed-off-by: Max <[email protected]>

The `character-count` extensions storage is a singleton and might be initialized by other editors loaded at the same time. See ueberdosis/tiptap#6060 Signed-off-by: Max <[email protected]>

The default slot somehow is not reactive. Signed-off-by: Max <[email protected]>

Signed-off-by: Max <[email protected]>

The editor storage of tiptap v2 is a singleton. We do not want to share the connection between multiple editors on the same page. Signed-off-by: Max <[email protected]>

The tiptap-text-direction extension only adds `dir` attributes when the document is updated. Use the `setContent` command in `setInitialYjsState` so it gets triggered and sets the editor up with the final state. This avoids another transaction after loading the yjs that gets recorded in the undo history. Signed-off-by: Max <[email protected]>

Signed-off-by: Max <[email protected]>

Use fixtures to initialize wrapper and destroy after the test Signed-off-by: Max <[email protected]>

mejo-

Really nice work ❤️

max-nextcloud force-pushed the migrate/mixins-to-composables branch 3 times, most recently from 4a8e79d to 4fc23ad Compare June 16, 2025 13:22

max-nextcloud force-pushed the migrate/mixins-to-composables branch 12 times, most recently from d51ccbd to 8e33aff Compare June 18, 2025 08:59

max-nextcloud marked this pull request as ready for review June 18, 2025 09:49

max-nextcloud requested a review from mejo- as a code owner June 18, 2025 09:49

max-nextcloud requested review from juliusknorr June 18, 2025 09:49

mejo- reviewed Jun 18, 2025

View reviewed changes

max-nextcloud force-pushed the migrate/mixins-to-composables branch 4 times, most recently from 410df10 to 6f0db14 Compare June 20, 2025 09:49

mejo- reviewed Jun 20, 2025

View reviewed changes

src/extensions/Session.ts Outdated Show resolved Hide resolved

max-nextcloud force-pushed the migrate/mixins-to-composables branch 3 times, most recently from 63b0b54 to 1cb8f09 Compare June 21, 2025 05:42

This comment was marked as resolved.

Sign in to view

max-nextcloud added 15 commits June 28, 2025 14:26

chore(migrate): use.find instead of deprecated .contains

b0790dc

Signed-off-by: Max <[email protected]>

chore(refactor): configure mention in rich text extension

9d3cc87

Signed-off-by: Max <[email protected]>

chore(types): collaborationCursor extension to typescript

c4f863f

Signed-off-by: Max <[email protected]>

chore(simplify): rely on updateUser command

354ad41

Allow initializing the editor before knowing the session. Signed-off-by: Max <[email protected]>

chore(simplify): replace computed fileExtension with temp

022e483

Only used once. Signed-off-by: Max <[email protected]>

enh(code): start to load syntax highlighting during setup

0809c8e

initialize editor before lowlight has been loaded Wait with loading the actual content to ensure syntax highlighting is available. Signed-off-by: Max <[email protected]>

chore(refactor): load editor in mounted

b326875

Cannot load it in setup yet as it requires the yjs provider, which in turn requires the sync service, which is only initialized in mounted. Signed-off-by: Max <[email protected]>

chore(refactor): create editor in created instead of mounted

3e5cfd8

Signed-off-by: Max <[email protected]>

chore(refactor): create ydoc in setup

ab26032

Signed-off-by: Max <[email protected]>

fix(character-count): always provide the current editors doc

2831462

The `character-count` extensions storage is a singleton and might be initialized by other editors loaded at the same time. See ueberdosis/tiptap#6060 Signed-off-by: Max <[email protected]>

fix(character-count): use the NcActionTexts name prop

d7df4c0

The default slot somehow is not reactive. Signed-off-by: Max <[email protected]>

fix(loading): only show main container when content loaded

e24d90a

Signed-off-by: Max <[email protected]>

fix(mention): use shallowRef for connection

0d5f4f7

The editor storage of tiptap v2 is a singleton. We do not want to share the connection between multiple editors on the same page. Signed-off-by: Max <[email protected]>

max-nextcloud force-pushed the migrate/mixins-to-composables branch from 40133be to f186837 Compare June 28, 2025 12:26

chore(refactor): extract useEditor into its own file

fae62fc

Signed-off-by: Max <[email protected]>

max-nextcloud force-pushed the migrate/mixins-to-composables branch 2 times, most recently from 43ade53 to ff4a5a9 Compare June 28, 2025 12:30

chore(refactor): extract useEditorFlags into its own file

17f0d62

Signed-off-by: Max <[email protected]>

max-nextcloud force-pushed the migrate/mixins-to-composables branch 2 times, most recently from 335d30a to f7f5d0d Compare June 28, 2025 17:15

test(RichTextReader): basic test

90b1d84

Signed-off-by: Max <[email protected]>

max-nextcloud force-pushed the migrate/mixins-to-composables branch from f7f5d0d to 90b1d84 Compare June 28, 2025 18:12

test(RichTextReader): update content

fabc79e

Use fixtures to initialize wrapper and destroy after the test Signed-off-by: Max <[email protected]>

max-nextcloud mentioned this pull request Jun 29, 2025

Migrate editor instantiation to setup function #7371

Merged

mejo- approved these changes Jun 30, 2025

View reviewed changes

mejo- merged commit 64280d9 into main Jun 30, 2025
66 checks passed

mejo- deleted the migrate/mixins-to-composables branch June 30, 2025 08:23

skjnldsv mentioned this pull request Aug 19, 2025

32.0.0 beta 1 nextcloud/server#54502

Merged

chore(migrate): useEditorMixin to useEditor composable #7313

chore(migrate): useEditorMixin to useEditor composable #7313

Uh oh!

Conversation

max-nextcloud commented Jun 16, 2025

Uh oh!

codecov bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mejo- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-nextcloud Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

mejo- left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jun 16, 2025 •

edited

Loading

max-nextcloud Jun 18, 2025 •

edited

Loading