From 2cc377147619fed9838bd016ccff6e4766dd4a44 Mon Sep 17 00:00:00 2001
From: skjnldsv <skjnldsv@protonmail.com>
Date: Wed, 23 Oct 2024 08:15:40 +0200
Subject: [PATCH] feat(systemtags): emit tags changes and optimise tag updates
 performances

Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
---
 .../src/components/SystemTagPicker.vue        |  81 ++++++++++----
 apps/systemtags/src/event-bus.d.ts            |   9 ++
 .../inlineSystemTagsAction.spec.ts            |  62 +++++++++--
 .../files_actions/inlineSystemTagsAction.ts   | 101 ++++++++++--------
 apps/systemtags/src/services/api.ts           |   4 +
 apps/systemtags/src/utils.ts                  |   7 ++
 6 files changed, 192 insertions(+), 72 deletions(-)
 create mode 100644 apps/systemtags/src/event-bus.d.ts

diff --git a/apps/systemtags/src/components/SystemTagPicker.vue b/apps/systemtags/src/components/SystemTagPicker.vue
index 55b664e3c9a..43e7e8fa682 100644
--- a/apps/systemtags/src/components/SystemTagPicker.vue
+++ b/apps/systemtags/src/components/SystemTagPicker.vue
@@ -11,7 +11,7 @@
 		close-on-click-outside
 		out-transition
 		@update:open="onCancel">
-		<NcEmptyContent v-if="loading || done" :name="t('systemtags', 'Applying changes…')">
+		<NcEmptyContent v-if="loading || done" :name="t('systemtags', 'Applying tags changes…')">
 			<template #icon>
 				<NcLoadingIcon v-if="!done" />
 				<CheckIcon v-else fill-color="var(--color-success)" />
@@ -86,8 +86,8 @@ import type { TagWithId } from '../types'
 import { defineComponent } from 'vue'
 import { emit } from '@nextcloud/event-bus'
 import { sanitize } from 'dompurify'
-import { showInfo } from '@nextcloud/dialogs'
-import { t } from '@nextcloud/l10n'
+import { showError, showInfo } from '@nextcloud/dialogs'
+import { getLanguage, t } from '@nextcloud/l10n'
 import escapeHTML from 'escape-html'
 
 import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
@@ -101,7 +101,7 @@ import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
 import TagIcon from 'vue-material-design-icons/Tag.vue'
 import CheckIcon from 'vue-material-design-icons/CheckCircle.vue'
 
-import { getNodeSystemTags } from '../utils'
+import { getNodeSystemTags, setNodeSystemTags } from '../utils'
 import { getTagObjects, setTagObjects } from '../services/api'
 import logger from '../services/logger'
 
@@ -303,23 +303,66 @@ export default defineComponent({
 				toRemove: this.toRemove,
 			})
 
-			// Add tags
-			for (const tag of this.toAdd) {
-				const { etag, objects } = await getTagObjects(tag, 'files')
-				let ids = [...objects.map(obj => obj.id), ...this.nodes.map(node => node.fileid)] as number[]
-				// Remove duplicates and empty ids
-				ids = [...new Set(ids.filter(id => !!id))]
-				await setTagObjects(tag, 'files', ids.map(id => ({ id, type: 'files' })), etag)
+			try {
+				// Add tags
+				for (const tag of this.toAdd) {
+					const { etag, objects } = await getTagObjects(tag, 'files')
+
+					// Create a new list of ids in one pass
+					const ids = [...new Set([
+						...objects.map(obj => obj.id).filter(Boolean),
+						...this.nodes.map(node => node.fileid).filter(Boolean),
+					])] as number[]
+
+					// Set tags
+					await setTagObjects(tag, 'files', ids.map(id => ({ id, type: 'files' })), etag)
+				}
+
+				// Remove tags
+				for (const tag of this.toRemove) {
+					const { etag, objects } = await getTagObjects(tag, 'files')
+
+					// Get file IDs from the nodes array just once
+					const nodeFileIds = new Set(this.nodes.map(node => node.fileid))
+
+					// Create a filtered and deduplicated list of ids in one pass
+					const ids = objects
+						.map(obj => obj.id)
+						.filter((id, index, self) => !nodeFileIds.has(id) && self.indexOf(id) === index)
+
+					// Set tags
+					await setTagObjects(tag, 'files', ids.map(id => ({ id, type: 'files' })), etag)
+				}
+			} catch (error) {
+				logger.error('Failed to apply tags', { error })
+				showError(t('systemtags', 'Failed to apply tags changes'))
+				this.loading = false
+				return
 			}
 
-			// Remove tags
-			for (const tag of this.toRemove) {
-				const { etag, objects } = await getTagObjects(tag, 'files')
-				let ids = objects.map(obj => obj.id) as number[]
-				// Remove the ids of the nodes and remove duplicates
-				ids = [...new Set(ids.filter(id => !this.nodes.map(node => node.fileid).includes(id)))]
-				await setTagObjects(tag, 'files', ids.map(id => ({ id, type: 'files' })), etag)
-			}
+			const nodes = [] as Node[]
+
+			// Update nodes
+			this.toAdd.forEach(tag => {
+				this.nodes.forEach(node => {
+					const tags = [...(getNodeSystemTags(node) || []), tag.displayName]
+						.sort((a, b) => a.localeCompare(b, getLanguage(), { ignorePunctuation: true }))
+					setNodeSystemTags(node, tags)
+					nodes.push(node)
+				})
+			})
+
+			this.toRemove.forEach(tag => {
+				this.nodes.forEach(node => {
+					const tags = [...(getNodeSystemTags(node) || [])].filter(t => t !== tag.displayName)
+						.sort((a, b) => a.localeCompare(b, getLanguage(), { ignorePunctuation: true }))
+					setNodeSystemTags(node, tags)
+					nodes.push(node)
+				})
+			})
+
+			// trigger update event
+			nodes.forEach(node => emit('systemtags:node:updated', node))
 
 			this.done = true
 			this.loading = false
diff --git a/apps/systemtags/src/event-bus.d.ts b/apps/systemtags/src/event-bus.d.ts
new file mode 100644
index 00000000000..368d0acaf0f
--- /dev/null
+++ b/apps/systemtags/src/event-bus.d.ts
@@ -0,0 +1,9 @@
+import type { Node } from '@nextcloud/files'
+
+declare module '@nextcloud/event-bus' {
+	interface NextcloudEvents {
+		'systemtags:node:updated': Node
+	}
+}
+
+export {}
diff --git a/apps/systemtags/src/files_actions/inlineSystemTagsAction.spec.ts b/apps/systemtags/src/files_actions/inlineSystemTagsAction.spec.ts
index 9d25bfcd2ad..6861b5015a1 100644
--- a/apps/systemtags/src/files_actions/inlineSystemTagsAction.spec.ts
+++ b/apps/systemtags/src/files_actions/inlineSystemTagsAction.spec.ts
@@ -5,6 +5,8 @@
 import { action } from './inlineSystemTagsAction'
 import { describe, expect, test } from 'vitest'
 import { File, Permission, View, FileAction } from '@nextcloud/files'
+import { emit, subscribe } from '@nextcloud/event-bus'
+import { setNodeSystemTags } from '../utils'
 
 const view = {
 	id: 'files',
@@ -28,7 +30,8 @@ describe('Inline system tags action conditions tests', () => {
 		expect(action.default).toBeUndefined()
 		expect(action.enabled).toBeDefined()
 		expect(action.order).toBe(0)
-		expect(action.enabled!([file], view)).toBe(false)
+		// Always enabled
+		expect(action.enabled!([file], view)).toBe(true)
 	})
 
 	test('Enabled with valid system tags', () => {
@@ -50,7 +53,7 @@ describe('Inline system tags action conditions tests', () => {
 })
 
 describe('Inline system tags action render tests', () => {
-	test('Render nothing when Node does not have system tags', async () => {
+	test('Render something even when Node does not have system tags', async () => {
 		const file = new File({
 			id: 1,
 			source: 'http://localhost/remote.php/dav/files/admin/foobar.txt',
@@ -60,7 +63,10 @@ describe('Inline system tags action render tests', () => {
 		})
 
 		const result = await action.renderInline!(file, view)
-		expect(result).toBeNull()
+		expect(result).toBeInstanceOf(HTMLElement)
+		expect(result!.outerHTML).toMatchInlineSnapshot(
+			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags" data-systemtags-fileid="1"></ul>"',
+		)
 	})
 
 	test('Render a single system tag', async () => {
@@ -80,7 +86,7 @@ describe('Inline system tags action render tests', () => {
 		const result = await action.renderInline!(file, view)
 		expect(result).toBeInstanceOf(HTMLElement)
 		expect(result!.outerHTML).toMatchInlineSnapshot(
-			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags"><li class="files-list__system-tag">Confidential</li></ul>"',
+			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags" data-systemtags-fileid="1"><li class="files-list__system-tag">Confidential</li></ul>"',
 		)
 	})
 
@@ -101,7 +107,7 @@ describe('Inline system tags action render tests', () => {
 		const result = await action.renderInline!(file, view)
 		expect(result).toBeInstanceOf(HTMLElement)
 		expect(result!.outerHTML).toMatchInlineSnapshot(
-			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags"><li class="files-list__system-tag">Important</li><li class="files-list__system-tag">Confidential</li></ul>"',
+			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags" data-systemtags-fileid="1"><li class="files-list__system-tag">Important</li><li class="files-list__system-tag">Confidential</li></ul>"',
 		)
 	})
 
@@ -127,7 +133,51 @@ describe('Inline system tags action render tests', () => {
 		const result = await action.renderInline!(file, view)
 		expect(result).toBeInstanceOf(HTMLElement)
 		expect(result!.outerHTML).toMatchInlineSnapshot(
-			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags"><li class="files-list__system-tag">Important</li><li class="files-list__system-tag files-list__system-tag--more" title="Confidential, Secret, Classified" aria-hidden="true" role="presentation">+3</li><li class="files-list__system-tag hidden-visually">Confidential</li><li class="files-list__system-tag hidden-visually">Secret</li><li class="files-list__system-tag hidden-visually">Classified</li></ul>"',
+			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags" data-systemtags-fileid="1"><li class="files-list__system-tag">Important</li><li class="files-list__system-tag files-list__system-tag--more" title="Confidential, Secret, Classified" aria-hidden="true" role="presentation">+3</li><li class="files-list__system-tag hidden-visually">Confidential</li><li class="files-list__system-tag hidden-visually">Secret</li><li class="files-list__system-tag hidden-visually">Classified</li></ul>"',
+		)
+	})
+
+	test('Render gets updated on system tag change', async () => {
+		const file = new File({
+			id: 1,
+			source: 'http://localhost/remote.php/dav/files/admin/foobar.txt',
+			owner: 'admin',
+			mime: 'text/plain',
+			permissions: Permission.ALL,
+			attributes: {
+				'system-tags': {
+					'system-tag': [
+						'Important',
+						'Confidential',
+						'Secret',
+						'Classified',
+					],
+				},
+			},
+		})
+
+		const result = await action.renderInline!(file, view) as HTMLElement
+		document.body.appendChild(result)
+		expect(result).toBeInstanceOf(HTMLElement)
+		expect(document.body.innerHTML).toMatchInlineSnapshot(
+			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags" data-systemtags-fileid="1"><li class="files-list__system-tag">Important</li><li class="files-list__system-tag files-list__system-tag--more" title="Confidential, Secret, Classified" aria-hidden="true" role="presentation">+3</li><li class="files-list__system-tag hidden-visually">Confidential</li><li class="files-list__system-tag hidden-visually">Secret</li><li class="files-list__system-tag hidden-visually">Classified</li></ul>"',
+		)
+
+		// Subscribe to the event
+		const eventPromise = new Promise((resolve) => {
+			subscribe('systemtags:node:updated', resolve)
+		})
+
+		// Change tags
+		setNodeSystemTags(file, ['Public'])
+		emit('systemtags:node:updated', file)
+		expect(file.attributes!['system-tags']!['system-tag']).toEqual(['Public'])
+
+		// Wait for the event to be processed
+		await eventPromise
+
+		expect(document.body.innerHTML).toMatchInlineSnapshot(
+			'"<ul class="files-list__system-tags" aria-label="Assigned collaborative tags" data-systemtags-fileid="1"><li class="files-list__system-tag">Public</li></ul>"',
 		)
 	})
 })
diff --git a/apps/systemtags/src/files_actions/inlineSystemTagsAction.ts b/apps/systemtags/src/files_actions/inlineSystemTagsAction.ts
index 2a7ee3e7eb2..afd54a4f7de 100644
--- a/apps/systemtags/src/files_actions/inlineSystemTagsAction.ts
+++ b/apps/systemtags/src/files_actions/inlineSystemTagsAction.ts
@@ -4,6 +4,7 @@
  */
 import type { Node } from '@nextcloud/files'
 import { FileAction } from '@nextcloud/files'
+import { subscribe } from '@nextcloud/event-bus'
 import { t } from '@nextcloud/l10n'
 
 import '../css/fileEntryInlineSystemTags.scss'
@@ -21,6 +22,46 @@ const renderTag = function(tag: string, isMore = false): HTMLElement {
 	return tagElement
 }
 
+const renderInline = async function(node: Node): Promise<HTMLElement> {
+	// Ensure we have the system tags as an array
+	const tags = getNodeSystemTags(node)
+
+	const systemTagsElement = document.createElement('ul')
+	systemTagsElement.classList.add('files-list__system-tags')
+	systemTagsElement.setAttribute('aria-label', t('files', 'Assigned collaborative tags'))
+	systemTagsElement.setAttribute('data-systemtags-fileid', node.fileid?.toString() || '')
+
+	if (tags.length === 0) {
+		return systemTagsElement
+	}
+
+	systemTagsElement.append(renderTag(tags[0]))
+	if (tags.length === 2) {
+		// Special case only two tags:
+		// the overflow fake tag would take the same space as this, so render it
+		systemTagsElement.append(renderTag(tags[1]))
+	} else if (tags.length > 1) {
+		// More tags than the one we're showing
+		// So we add a overflow element indicating there are more tags
+		const moreTagElement = renderTag('+' + (tags.length - 1), true)
+		moreTagElement.setAttribute('title', tags.slice(1).join(', '))
+		// because the title is not accessible we hide this element for screen readers (see alternative below)
+		moreTagElement.setAttribute('aria-hidden', 'true')
+		moreTagElement.setAttribute('role', 'presentation')
+		systemTagsElement.append(moreTagElement)
+
+		// For accessibility the tags are listed, as the title is not accessible
+		// but those tags are visually hidden
+		for (const tag of tags.slice(1)) {
+			const tagElement = renderTag(tag)
+			tagElement.classList.add('hidden-visually')
+			systemTagsElement.append(tagElement)
+		}
+	}
+
+	return systemTagsElement
+}
+
 export const action = new FileAction({
 	id: 'system-tags',
 	displayName: () => '',
@@ -32,57 +73,23 @@ export const action = new FileAction({
 			return false
 		}
 
-		const node = nodes[0]
-		const tags = getNodeSystemTags(node)
-
-		// Only show the action if the node has system tags
-		if (tags.length === 0) {
-			return false
-		}
-
+		// Always show the action, even if there are no tags
+		// This will render an empty tag list and allow events to update it
 		return true
 	},
 
 	exec: async () => null,
-
-	async renderInline(node: Node) {
-		// Ensure we have the system tags as an array
-		const tags = getNodeSystemTags(node)
-
-		if (tags.length === 0) {
-			return null
-		}
-
-		const systemTagsElement = document.createElement('ul')
-		systemTagsElement.classList.add('files-list__system-tags')
-		systemTagsElement.setAttribute('aria-label', t('files', 'Assigned collaborative tags'))
-
-		systemTagsElement.append(renderTag(tags[0]))
-		if (tags.length === 2) {
-			// Special case only two tags:
-			// the overflow fake tag would take the same space as this, so render it
-			systemTagsElement.append(renderTag(tags[1]))
-		} else if (tags.length > 1) {
-			// More tags than the one we're showing
-			// So we add a overflow element indicating there are more tags
-			const moreTagElement = renderTag('+' + (tags.length - 1), true)
-			moreTagElement.setAttribute('title', tags.slice(1).join(', '))
-			// because the title is not accessible we hide this element for screen readers (see alternative below)
-			moreTagElement.setAttribute('aria-hidden', 'true')
-			moreTagElement.setAttribute('role', 'presentation')
-			systemTagsElement.append(moreTagElement)
-
-			// For accessibility the tags are listed, as the title is not accessible
-			// but those tags are visually hidden
-			for (const tag of tags.slice(1)) {
-				const tagElement = renderTag(tag)
-				tagElement.classList.add('hidden-visually')
-				systemTagsElement.append(tagElement)
-			}
-		}
-
-		return systemTagsElement
-	},
+	renderInline,
 
 	order: 0,
 })
+
+const updateSystemTagsHtml = function(node: Node) {
+	renderInline(node).then((systemTagsHtml) => {
+		document.querySelectorAll(`[data-systemtags-fileid="${node.fileid}"]`).forEach((element) => {
+			element.replaceWith(systemTagsHtml)
+		})
+	})
+}
+
+subscribe('systemtags:node:updated', updateSystemTagsHtml)
diff --git a/apps/systemtags/src/services/api.ts b/apps/systemtags/src/services/api.ts
index c37b6e51c20..39059d18347 100644
--- a/apps/systemtags/src/services/api.ts
+++ b/apps/systemtags/src/services/api.ts
@@ -143,6 +143,10 @@ export const getTagObjects = async function(tag: TagWithId, type: string): Promi
 /**
  * Set the objects for a tag.
  * Warning: This will overwrite the existing objects.
+ * @param tag The tag to set the objects for
+ * @param type The type of the objects
+ * @param objectIds The objects to set
+ * @param etag Strongly recommended to avoid conflict and data loss.
  */
 export const setTagObjects = async function(tag: TagWithId, type: string, objectIds: TagObject[], etag: string = ''): Promise<void> {
 	const path = `/systemtags/${tag.id}/${type}`
diff --git a/apps/systemtags/src/utils.ts b/apps/systemtags/src/utils.ts
index 456dc52ca84..35fed6026f3 100644
--- a/apps/systemtags/src/utils.ts
+++ b/apps/systemtags/src/utils.ts
@@ -9,6 +9,7 @@ import type { DAVResultResponseProps } from 'webdav'
 
 import type { BaseTag, ServerTag, Tag, TagWithId } from './types.js'
 import type { Node } from '@nextcloud/files'
+import Vue from 'vue'
 
 export const defaultBaseTag: BaseTag = {
 	userVisible: true,
@@ -66,3 +67,9 @@ export const getNodeSystemTags = function(node: Node): string[] {
 
 	return [tags].flat()
 }
+
+export const setNodeSystemTags = function(node: Node, tags: string[]): void {
+	Vue.set(node.attributes, 'system-tags', {
+		'system-tag': tags,
+	})
+}