From 7da3b55213f51f2c20706b340c6a223b1533e70d Mon Sep 17 00:00:00 2001 From: Frank Karlitschek Date: Thu, 6 Aug 2026 19:18:16 +0200 Subject: [PATCH 1/8] feat(notes): expose file versions in the note sidebar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Notes have been versioned all along — they are ordinary files, so files_versions keeps history for them without the app doing anything. There was just no way to see it from Notes. Most of the wiring already existed: * PageController dispatches OCA\Files\Event\LoadSidebar, and files_versions registers a listener on that event which adds its sidebar-tab script. The Versions tab has therefore been registered on every Notes page already, simply never rendered. * NotePlain and NoteRich both already subscribe to files_versions:restore:requested and :restored, showing a loading state and refreshing the note afterwards. The restore path was built and unreachable. * NoteShareSidebar already knew how to mount a registered Files sidebar tab as a custom element with the node/folder/view props it expects. The only thing missing was that the sidebar hard-filtered the tab registry down to `id === 'sharing'`. It now renders every tab from an allow-list, so Sharing and Versions sit side by side. Details: * Tab selection moved to a pure function in sidebarTabs.js. It is an allow-list rather than "everything registered", because LoadSidebar brings in whatever every installed app registers and a note sidebar should not grow new tabs when an unrelated app is installed. A tab's own enabled() predicate still has the final say — the versions tab hides itself on public shares and for non-files — but it needs a node to judge, so while the node is still loading tabs are kept and filtered again once it arrives, and a predicate that throws drops that tab instead of taking the sidebar down. * Tabs initialise independently, so one failing to define its custom element no longer hides the others; only a total failure is reported. * New event notes:sidebar:open carries a tab id. notes:share:open is kept as a thin wrapper so anything already emitting it keeps working. * "Versions" action added to the note's action menu, next to "Share". That menu lives in the note list row, so it is present in every editor mode rather than only the non-default one. * Sidebar copy no longer says "sharing" now that it hosts two tabs. The data-cy-notes-share-sidebar hook is deliberately unchanged, since playwright/e2e/basic.spec.ts asserts on it. Assisted-by: Claude Code:claude-opus-5[1m] Co-Authored-By: Andy Scherzinger Signed-off-by: Frank Karlitschek --- playwright/e2e/note-actions.spec.ts | 11 +- playwright/e2e/note-sidebar.spec.ts | 89 +++++++++++++ playwright/support/note.ts | 7 + src/components/NoteItem.vue | 19 +++ src/components/NoteShareSidebar.vue | 197 ++++++++++++++++++++-------- src/sidebarTabs.js | 51 +++++++ 6 files changed, 311 insertions(+), 63 deletions(-) create mode 100644 playwright/e2e/note-sidebar.spec.ts create mode 100644 src/sidebarTabs.js diff --git a/playwright/e2e/note-actions.spec.ts b/playwright/e2e/note-actions.spec.ts index c39796919..8c3d5ffee 100644 --- a/playwright/e2e/note-actions.spec.ts +++ b/playwright/e2e/note-actions.spec.ts @@ -3,18 +3,11 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import type { Locator, Page, TestInfo } from '@playwright/test' +import type { TestInfo } from '@playwright/test' import { expect, test } from '@playwright/test' import { login } from '../support/login.ts' -import { createNote, newNoteButton, noteRow, uniqueTitle } from '../support/note.ts' - -async function openNoteActions(page: Page, noteId: number): Promise { - const row = noteRow(page, noteId) - await row.hover() - await row.locator('.action-item__menutoggle').click() - return row -} +import { createNote, newNoteButton, noteRow, openNoteActions, uniqueTitle } from '../support/note.ts' test.describe('Note actions', () => { test.beforeEach(async ({ page }) => { diff --git a/playwright/e2e/note-sidebar.spec.ts b/playwright/e2e/note-sidebar.spec.ts new file mode 100644 index 000000000..426dabfa1 --- /dev/null +++ b/playwright/e2e/note-sidebar.spec.ts @@ -0,0 +1,89 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { Locator, Page, TestInfo } from '@playwright/test' + +import { expect, test } from '@playwright/test' +import { login } from '../support/login.ts' +import { createNote, newNoteButton, openNoteActions, uniqueTitle } from '../support/note.ts' + +interface EventBusWindow extends Window { + _nc_event_bus: { + emit: (name: string, payload: unknown) => void + } +} + +function sidebar(page: Page): Locator { + return page.locator('[data-cy-notes-share-sidebar]') +} + +function tabButton(page: Page, tabId: string): Locator { + return sidebar(page).locator(`#tab-button-${tabId}`) +} + +function versionsList(page: Page): Locator { + return sidebar(page).locator('[data-files-versions-versions-list]') +} + +async function openSidebarFromActions(page: Page, noteId: number, action: string): Promise { + await openNoteActions(page, noteId) + await page.getByRole('menuitem', { name: action, exact: true }).click() + await expect(sidebar(page)).toBeVisible({ timeout: 15000 }) +} + +test.describe('Note sidebar', () => { + test.beforeEach(async ({ page }) => { + await login(page) + await page.goto('/index.php/apps/notes/') + await expect(newNoteButton(page)).toBeVisible() + }) + + test('opens the versions tab from the actions menu', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('versions', testInfo)) + + await openSidebarFromActions(page, noteId, 'Versions') + + await expect(tabButton(page, 'files_versions')).toHaveAttribute('aria-selected', 'true') + await expect(versionsList(page)).toBeAttached({ timeout: 15000 }) + }) + + test('renders the allow-listed tabs only', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('sidebar-tabs', testInfo)) + + await openSidebarFromActions(page, noteId, 'Share') + + await expect(tabButton(page, 'sharing')).toBeVisible() + await expect(tabButton(page, 'files_versions')).toBeVisible() + await expect(sidebar(page).getByRole('tab')).toHaveCount(2) + }) + + test('switches between the sharing and versions tabs', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('sidebar-switch', testInfo)) + + await openSidebarFromActions(page, noteId, 'Share') + await expect(page.getByText('Internal shares')).toBeVisible({ timeout: 15000 }) + + await tabButton(page, 'files_versions').click() + await expect(tabButton(page, 'files_versions')).toHaveAttribute('aria-selected', 'true') + await expect(versionsList(page)).toBeAttached({ timeout: 15000 }) + + await tabButton(page, 'sharing').click() + await expect(tabButton(page, 'sharing')).toHaveAttribute('aria-selected', 'true') + await expect(page.getByText('Internal shares')).toBeVisible() + }) + + test('falls back to the first tab when the requested one is unavailable', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('sidebar-fallback', testInfo)) + + await page.evaluate((id) => { + (window as unknown as EventBusWindow)._nc_event_bus + .emit('notes:sidebar:open', { noteId: id, tab: 'not-a-note-sidebar-tab' }) + }, noteId) + + await expect(sidebar(page)).toBeVisible({ timeout: 15000 }) + await expect(tabButton(page, 'sharing')).toHaveAttribute('aria-selected', 'true') + await expect(page.getByText('Internal shares')).toBeVisible({ timeout: 15000 }) + }) +}) diff --git a/playwright/support/note.ts b/playwright/support/note.ts index 83eb612a6..ae0ef1faf 100644 --- a/playwright/support/note.ts +++ b/playwright/support/note.ts @@ -26,6 +26,13 @@ export function noteRow(page: Page, noteId: number): Locator { .locator('xpath=ancestor::li[1]') } +export async function openNoteActions(page: Page, noteId: number): Promise { + const row = noteRow(page, noteId) + await row.hover() + await row.locator('.action-item__menutoggle').click() + return row +} + export async function waitForNoteRoute(page: Page, previousNoteId: number | null): Promise { await expect.poll(() => currentNoteId(page)).not.toBe(previousNoteId) diff --git a/src/components/NoteItem.vue b/src/components/NoteItem.vue index 535bfd6e0..4f3aba371 100644 --- a/src/components/NoteItem.vue +++ b/src/components/NoteItem.vue @@ -42,6 +42,13 @@ {{ t('notes', 'Share') }} + + + {{ t('notes', 'Versions') }} + + - {{ tabError || t('notes', 'Sharing and versions are not available right now.') }} + {{ t('notes', 'Sharing and versions are not available right now.') }} @@ -76,11 +76,57 @@ import { selectNoteSidebarTabs } from '../sidebarTabs.js' import store from '../store.js' import { fetchDavNode } from '../WebdavService.js' -// customElements.whenDefined() never settles for an element that is never -// defined, so a tab whose onInit() does not deliver one must not be waited for -// forever const TAB_DEFINITION_TIMEOUT = 10000 +const pendingTabs = new Map() + +/** + * @param {object} tab a registered Files sidebar tab + * @return {Promise} whether its custom element got defined + */ +async function defineTabElement(tab) { + let timeout + try { + await Promise.race([ + (async () => { + await tab.onInit?.() + await window.customElements.whenDefined(tab.tagName) + })(), + new Promise((resolve, reject) => { + timeout = setTimeout( + () => reject(new Error(`${tab.tagName} was not defined in time`)), + TAB_DEFINITION_TIMEOUT, + ) + }), + ]) + return true + } catch (error) { + logger.error('Failed to initialize a sidebar tab in Notes', { error, tab: tab.id }) + return false + } finally { + clearTimeout(timeout) + } +} + +/** + * @param {object} tab a registered Files sidebar tab + * @return {Promise} whether the tab is usable + */ +function initializeTab(tab) { + if (window.customElements.get(tab.tagName)) { + return Promise.resolve(true) + } + + if (!pendingTabs.has(tab.tagName)) { + pendingTabs.set( + tab.tagName, + defineTabElement(tab).finally(() => pendingTabs.delete(tab.tagName)), + ) + } + + return pendingTabs.get(tab.tagName) +} + export default { name: 'NoteShareSidebar', @@ -101,14 +147,11 @@ export default { contextRequestToken: 0, currentFolder: null, currentNode: null, - pendingTabs: new Map(), - initializedTabs: new Set(), failedTabs: new Set(), isOpen: false, loadingContext: false, loadingTab: false, noteId: null, - tabError: '', } }, @@ -136,6 +179,18 @@ export default { return this.availableTabs.filter((tab) => !this.failedTabs.has(tab.tagName)) }, + /** + * NcAppSidebar falls back to its first tab when the active one is not + * among them, but does not report that back, so the tab id has to be + * clamped here as well for `active` to reach the right custom element. + */ + resolvedTab() { + if (this.tabs.some(({ id }) => id === this.activeTab)) { + return this.activeTab + } + return this.tabs[0]?.id ?? this.activeTab + }, + currentView() { return { id: 'notes', @@ -144,14 +199,6 @@ export default { }, }, - watch: { - // the versions tab drops out once the node says it is not applicable, - // so what was requested is not necessarily still renderable - tabs(tabs) { - this.activeTab = this.resolveTab(this.activeTab, tabs) - }, - }, - mounted() { // the share event is kept so anything already emitting it keeps working subscribe('notes:share:open', this.onShareOpen) @@ -172,78 +219,30 @@ export default { } const requestToken = this.contextRequestToken + this.loadingTab = true - // One tab failing to define its element must not hide the others, so - // they are initialised independently and only a total failure is - // reported as an error. - const results = await Promise.all(tabs.map((tab) => this.initializeTab(tab))) + const results = await Promise.all(tabs.map(initializeTab)) if (requestToken !== this.contextRequestToken) { return } - this.loadingTab = false - this.tabError = results.includes(true) - ? '' - : this.t('notes', 'Failed to load the note sidebar.') - }, - - /** - * @param {object} tab a registered Files sidebar tab - * @return {Promise} whether the tab is usable - */ - async initializeTab(tab) { - if (window.customElements.get(tab.tagName) || this.initializedTabs.has(tab.tagName)) { - return true - } - - this.loadingTab = true - - // an open while another one is still initializing the same element - // has to await that initialization, not assume it succeeded - const pending = this.pendingTabs.get(tab.tagName) - if (pending) { - return pending - } - - const initialization = this.defineTabElement(tab) - this.pendingTabs.set(tab.tagName, initialization) + tabs.forEach((tab, index) => { + if (!results[index]) { + this.failedTabs.add(tab.tagName) + } + }) - try { - return await initialization - } finally { - this.pendingTabs.delete(tab.tagName) - } + this.loadingTab = false }, - /** - * @param {object} tab a registered Files sidebar tab - * @return {Promise} whether its custom element got defined - */ - async defineTabElement(tab) { - let timeout - try { - await Promise.race([ - (async () => { - await tab.onInit?.() - await window.customElements.whenDefined(tab.tagName) - })(), - new Promise((resolve, reject) => { - timeout = setTimeout( - () => reject(new Error(`${tab.tagName} was not defined in time`)), - TAB_DEFINITION_TIMEOUT, - ) - }), - ]) - this.initializedTabs.add(tab.tagName) - return true - } catch (error) { - logger.error('Failed to initialize a sidebar tab in Notes', { error, tab: tab.id }) - this.failedTabs.add(tab.tagName) - return false - } finally { - clearTimeout(timeout) - } + resetContext() { + this.contextRequestToken += 1 + this.contextError = '' + this.currentNode = null + this.currentFolder = null + this.loadingContext = false + this.loadingTab = false }, async loadNodeContext() { @@ -292,38 +291,16 @@ export default { } }, - /** - * NcAppSidebar falls back to its first tab when the active one is not - * among them, but does not report that back, so the tab id here has to - * be clamped as well for `active` to reach the right custom element. - * - * @param {string} tab the requested tab id - * @param {Array} tabs the tabs currently rendered - * @return {string} the requested tab if renderable, the first one otherwise - */ - resolveTab(tab, tabs) { - if (tabs.length === 0 || tabs.some(({ id }) => id === tab)) { - return tab - } - return tabs[0].id - }, - onShareOpen({ noteId }) { return this.onSidebarOpen({ noteId, tab: 'sharing' }) }, async onSidebarOpen({ noteId, tab = 'sharing' }) { - this.contextRequestToken += 1 + this.resetContext() this.noteId = Number(noteId) this.isOpen = true - this.contextError = '' - this.tabError = '' - this.currentNode = null - this.currentFolder = null - this.loadingContext = false - this.loadingTab = false this.failedTabs.clear() - this.activeTab = this.resolveTab(tab, this.tabs) + this.activeTab = tab if (this.availableTabs.length === 0) { await this.initializeTabs() @@ -347,14 +324,8 @@ export default { return } - this.contextRequestToken += 1 + this.resetContext() this.noteId = null - this.contextError = '' - this.currentNode = null - this.currentFolder = null - this.loadingContext = false - this.loadingTab = false - this.tabError = '' }, }, } From 8da253814a8d49712e81b550983e8c7f731e2bd5 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 16 Aug 2026 17:49:12 +0200 Subject: [PATCH 5/8] feat(notes): outline the sharing tab icon until its tab is active MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sidebar tabs should carry outlined icons that fill once the tab is active. The sharing tab now renders ShareVariantOutline while inactive and ShareVariant while active, following the pattern from nextcloud/tables#2672. The switch happens inside the #icon slot rather than through a dedicated slot, as @nextcloud/vue has no #icon-active yet: NcAppSidebarTab exposes renderIcon() without arguments. That is enough here, because the tab button invokes renderIcon() from its own render function, so reading the resolved tab id there tracks it. Only the sharing tab is overridden. Every other tab keeps the icon its app registered, versions included — there is no outlined counterpart of the backup-restore icon to fill in. Mixing the two icon systems misaligns the nav: NcIconSvgWrapper reserves a clickable-area box around its svg while a material design icon is only as big as itself, which left the versions icon 7px below the sharing one. The wrapper's inline modifier drops that box. Assisted-by: Claude Code:claude-opus-5[1m] Signed-off-by: Andy Scherzinger --- playwright/e2e/note-sidebar.spec.ts | 33 +++++++++++++++++++++++++++++ src/components/NoteShareSidebar.vue | 10 ++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/playwright/e2e/note-sidebar.spec.ts b/playwright/e2e/note-sidebar.spec.ts index cb899035f..69761417a 100644 --- a/playwright/e2e/note-sidebar.spec.ts +++ b/playwright/e2e/note-sidebar.spec.ts @@ -89,6 +89,39 @@ test.describe('Note sidebar', () => { await expect(page.getByText('Internal shares')).toBeVisible() }) + test('fills the sharing icon only while its tab is active', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('sidebar-icons', testInfo)) + + await openSidebarFromActions(page, noteId, 'Share') + + await expect(tabButton(page, 'sharing').locator('.share-variant-icon')).toBeVisible() + await expect(tabButton(page, 'sharing').locator('.share-variant-outline-icon')).toHaveCount(0) + + await tabButton(page, 'files_versions').click() + + await expect(tabButton(page, 'sharing').locator('.share-variant-outline-icon')).toBeVisible() + await expect(tabButton(page, 'sharing').locator('.share-variant-icon')).toHaveCount(0) + }) + + test('lines the tab icons up with each other', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('sidebar-align', testInfo)) + + await openSidebarFromActions(page, noteId, 'Share') + await expect(tabButton(page, 'files_versions')).toBeVisible() + + const icons = await page.evaluate(() => { + const box = (id: string) => { + const selector = `#tab-button-${id} :is(.icon-vue, .material-design-icon)` + const { y, height } = document.querySelector(selector)!.getBoundingClientRect() + return { y, height } + } + return { sharing: box('sharing'), versions: box('files_versions') } + }) + + expect(icons.versions.y).toBeCloseTo(icons.sharing.y, 0) + expect(icons.versions.height).toBeCloseTo(icons.sharing.height, 0) + }) + test('falls back to the first tab when the requested one is unavailable', async ({ page }, testInfo: TestInfo) => { const noteId = await createNote(page, uniqueTitle('sidebar-fallback', testInfo)) diff --git a/src/components/NoteShareSidebar.vue b/src/components/NoteShareSidebar.vue index 237836f67..d88c22364 100644 --- a/src/components/NoteShareSidebar.vue +++ b/src/components/NoteShareSidebar.vue @@ -26,7 +26,11 @@ :order="tab.order" > @@ -70,6 +74,8 @@ import NcEmptyContent from '@nextcloud/vue/components/NcEmptyContent' import NcIconSvgWrapper from '@nextcloud/vue/components/NcIconSvgWrapper' import NcLoadingIcon from '@nextcloud/vue/components/NcLoadingIcon' import FileOutlineIcon from 'vue-material-design-icons/FileOutline.vue' +import ShareVariantIcon from 'vue-material-design-icons/ShareVariant.vue' +import ShareVariantOutlineIcon from 'vue-material-design-icons/ShareVariantOutline.vue' import NoteSidebarSubname from './NoteSidebarSubname.vue' import logger from '../Logger.js' import { selectNoteSidebarTabs } from '../sidebarTabs.js' @@ -138,6 +144,8 @@ export default { NcLoadingIcon, FileOutlineIcon, NoteSidebarSubname, + ShareVariantIcon, + ShareVariantOutlineIcon, }, data() { From b4c26820ef312d3144ad0afd15fe27f36d7829ec Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 16 Aug 2026 21:35:38 +0200 Subject: [PATCH 6/8] fix(notes): refresh the sidebar versions list after a restore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restoring an older version left the list showing the state from when the sidebar was opened. It took a reload or reopening the sidebar to see the restored version as the current one. The versions tab already reloads itself when the mtime of the node it was handed changes, and emits files:node:updated with a node carrying the restored etag, size and mtime. The Files sidebar closes that loop by swapping its current node whenever such an event names it, which is what the note sidebar now does too — matching on source, as the Files sidebar store does. The subname in the sidebar header picks the update up as well, so size and modification date no longer lag behind a restore either. The test emits the event files_versions sends out after a restore and watches for the reload it triggers, rather than restoring for real: what the sidebar has to do is the same either way, and the outcome then does not hinge on how a server stamps a rollback. Assisted-by: Claude Code:claude-opus-5[1m] Signed-off-by: Andy Scherzinger --- playwright/e2e/note-sidebar.spec.ts | 35 +++++++++++++++++++++++++++++ src/components/NoteShareSidebar.vue | 15 +++++++++++++ 2 files changed, 50 insertions(+) diff --git a/playwright/e2e/note-sidebar.spec.ts b/playwright/e2e/note-sidebar.spec.ts index 69761417a..9caaeb739 100644 --- a/playwright/e2e/note-sidebar.spec.ts +++ b/playwright/e2e/note-sidebar.spec.ts @@ -15,6 +15,11 @@ interface EventBusWindow extends Window { } } +interface NodeLike { + mtime: Date + clone: () => NodeLike +} + function sidebar(page: Page): Locator { return page.locator('[data-cy-notes-share-sidebar]') } @@ -27,6 +32,12 @@ function versionsList(page: Page): Locator { return sidebar(page).locator('[data-files-versions-versions-list]') } +// scoped to the list rather than the sidebar: the sharing tab's element reports +// itself as its own shadow root, which sends a piercing query into a loop +function versionEntries(page: Page): Locator { + return page.locator('[data-files-versions-versions-list] [data-files-versions-version]') +} + function subname(page: Page): Locator { return sidebar(page).locator('.app-sidebar-header__subname') } @@ -53,6 +64,30 @@ test.describe('Note sidebar', () => { await expect(versionsList(page)).toBeAttached({ timeout: 15000 }) }) + test('reloads the versions list when the note is updated', async ({ page }, testInfo: TestInfo) => { + const noteId = await createNote(page, uniqueTitle('sidebar-reload', testInfo)) + + await openSidebarFromActions(page, noteId, 'Versions') + await expect(versionEntries(page).first()).toBeVisible({ timeout: 15000 }) + + let reloads = 0 + page.on('request', (request) => { + if (request.method() === 'PROPFIND' && request.url().includes('/remote.php/dav/versions/')) { + reloads += 1 + } + }) + + // what files_versions hands out once it has restored a version + await page.evaluate(() => { + const tab = document.querySelector('files-versions_sidebar-tab') as unknown as { node: NodeLike } + const node = tab.node.clone() + node.mtime = new Date(node.mtime.getTime() - 60000) + ;(window as unknown as EventBusWindow)._nc_event_bus.emit('files:node:updated', node) + }) + + await expect.poll(() => reloads, { timeout: 15000 }).toBeGreaterThan(0) + }) + test('shows the size, the modification date and the owner of the note', async ({ page }, testInfo: TestInfo) => { const noteId = await createNote(page, uniqueTitle('sidebar-subname', testInfo)) diff --git a/src/components/NoteShareSidebar.vue b/src/components/NoteShareSidebar.vue index d88c22364..9a38ed973 100644 --- a/src/components/NoteShareSidebar.vue +++ b/src/components/NoteShareSidebar.vue @@ -211,11 +211,13 @@ export default { // the share event is kept so anything already emitting it keeps working subscribe('notes:share:open', this.onShareOpen) subscribe('notes:sidebar:open', this.onSidebarOpen) + subscribe('files:node:updated', this.onNodeUpdated) }, unmounted() { unsubscribe('notes:share:open', this.onShareOpen) unsubscribe('notes:sidebar:open', this.onSidebarOpen) + unsubscribe('files:node:updated', this.onNodeUpdated) }, methods: { @@ -299,6 +301,19 @@ export default { } }, + /** + * Tabs report what they changed about the note through this event — a + * restored version for instance — and hand out a node of their own, + * which they in turn watch for changes. + * + * @param {object} node the updated node + */ + onNodeUpdated(node) { + if (node?.source && node.source === this.currentNode?.source) { + this.currentNode = node + } + }, + onShareOpen({ noteId }) { return this.onSidebarOpen({ noteId, tab: 'sharing' }) }, From 4029bf8b176b8a0665e9d1b7ce4c7a8bb1727c96 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sun, 16 Aug 2026 21:36:04 +0200 Subject: [PATCH 7/8] fix(notes): keep the editor behind a spinner while a version is restored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restoring a version left the editor showing the old content until its periodic refresh came around, with nothing indicating that anything was going on — and that stale content could be typed into meanwhile. Both editors already handled this, but were never reached: they read a payload files_versions no longer emits — a fileInfo key, and a fileId on the version — so the requested handler threw on the missing key and the restored one always returned early. They take the node from the event now and compare its fileid. That brings back the loading state, which replaces the editor with a spinner and thereby keeps it from being typed into while the content is swapped, along with the immediate refresh once the restore lands. Two things were needed for that state to mean anything: NotePlain's refreshNote() returns its promise now, as it would otherwise be cleared before the new content arrived, and both editors clear it on files_versions:restore:failed, which would leave the editor stuck behind the spinner for good. The test delays the restore request so the window it asserts on is not a race, and opens the note explicitly, as a reload would leave the editor on whichever note was open before. Its revisions are written over WebDAV rather than through the app, which would retitle — and thereby rename — the note from its changed content, and they are spaced out because recent versions are thinned to one per two seconds. Version entries are located from the list rather than from the sidebar: the sharing tab's element reports itself as its own shadow root, which sends a piercing query into a loop. The poll that waited for a conflict button to auto-click goes as well. It looked for data-cy="resolveServerVersion", which exists neither in Notes nor in Text — Text's collision dialog offers useEditorVersion and useReaderVersion — so it never hit and never stopped, and fixing the guard above would have turned it into a timer per restore that runs for as long as the page is open. Pressing that button for the user would mean discarding whatever they had typed but not yet saved, which is the very thing the dialog asks about, so the dialog is left to them. Assisted-by: Claude Code:claude-opus-5[1m] Signed-off-by: Andy Scherzinger --- playwright/e2e/note-sidebar.spec.ts | 74 ++++++++++++++++++++++++++++- playwright/support/note.ts | 41 +++++++++++++++- src/components/NotePlain.vue | 51 +++++++++++++------- src/components/NoteRich.vue | 37 +++++++++------ 4 files changed, 168 insertions(+), 35 deletions(-) diff --git a/playwright/e2e/note-sidebar.spec.ts b/playwright/e2e/note-sidebar.spec.ts index 9caaeb739..ef0621e83 100644 --- a/playwright/e2e/note-sidebar.spec.ts +++ b/playwright/e2e/note-sidebar.spec.ts @@ -7,7 +7,8 @@ import type { Locator, Page, TestInfo } from '@playwright/test' import { expect, test } from '@playwright/test' import { login } from '../support/login.ts' -import { createNote, newNoteButton, openNoteActions, setNoteMode, uniqueTitle } from '../support/note.ts' +import { createNote, createNoteRevisions, newNoteButton, openNoteActions, setNoteMode, uniqueTitle } from '../support/note.ts' +import { NoteEditor } from '../support/sections/NoteEditor.ts' interface EventBusWindow extends Window { _nc_event_bus: { @@ -88,6 +89,77 @@ test.describe('Note sidebar', () => { await expect.poll(() => reloads, { timeout: 15000 }).toBeGreaterThan(0) }) + test('keeps the editor behind a spinner while a restored version loads', async ({ page, request }) => { + const noteId = await createNoteRevisions(request, [ + 'Restore spinner\n\nrevision one', + 'Restore spinner\n\nrevision two', + ]) + // the editor has to hold this note, not whichever one was open before + await page.goto(`/index.php/apps/notes/note/${noteId}`) + + // hold the restore long enough to observe what the editor does meanwhile + await page.route('**/remote.php/dav/versions/**', async (route) => { + if (route.request().method() === 'MOVE') { + await new Promise((resolve) => setTimeout(resolve, 3000)) + } + await route.continue() + }) + + await openSidebarFromActions(page, noteId, 'Versions') + + const entries = versionEntries(page) + await expect(entries.nth(1)).toBeVisible({ timeout: 15000 }) + + const editor = page.locator('.text-editor, .note-editor') + const spinner = page.locator('#app-content-vue.loading, .text-editor-wrapper.loading') + await expect(editor).toBeVisible() + + await entries.last().hover() + await entries.last().locator('.action-item__menutoggle').first().click() + await page.getByRole('menuitem', { name: 'Restore version' }).click() + + // the editor is gone while the restore runs, so it cannot be typed into + await expect(spinner).toBeVisible() + await expect(editor).toBeHidden() + + await expect(editor).toBeVisible({ timeout: 20000 }) + await new NoteEditor(page).expectText('Restore spinner\n\nrevision one') + }) + + test('gives the editor back when a restore fails', async ({ page, request }) => { + const noteId = await createNoteRevisions(request, [ + 'Restore failure\n\nrevision one', + 'Restore failure\n\nrevision two', + ]) + await page.goto(`/index.php/apps/notes/note/${noteId}`) + + await page.route('**/remote.php/dav/versions/**', async (route) => { + if (route.request().method() === 'MOVE') { + await new Promise((resolve) => setTimeout(resolve, 1000)) + await route.fulfill({ status: 500 }) + return + } + await route.continue() + }) + + await openSidebarFromActions(page, noteId, 'Versions') + + const entries = versionEntries(page) + await expect(entries.nth(1)).toBeVisible({ timeout: 15000 }) + + const editor = page.locator('.text-editor, .note-editor') + await expect(editor).toBeVisible() + + await entries.last().hover() + await entries.last().locator('.action-item__menutoggle').first().click() + await page.getByRole('menuitem', { name: 'Restore version' }).click() + + await expect(page.locator('#app-content-vue.loading, .text-editor-wrapper.loading')).toBeVisible() + + // a failed restore must not leave the editor behind the spinner + await expect(editor).toBeVisible({ timeout: 15000 }) + }) + test('shows the size, the modification date and the owner of the note', async ({ page }, testInfo: TestInfo) => { const noteId = await createNote(page, uniqueTitle('sidebar-subname', testInfo)) diff --git a/playwright/support/note.ts b/playwright/support/note.ts index f72c39c2a..351672c2e 100644 --- a/playwright/support/note.ts +++ b/playwright/support/note.ts @@ -12,10 +12,47 @@ export function uniqueTitle(prefix: string, testInfo: TestInfo): string { return `Playwright ${prefix} ${testInfo.parallelIndex}-${Date.now()}` } +function apiUser(): string { + return process.env.NC_USER ?? 'admin' +} + function apiHeaders(): Record { - const user = process.env.NC_USER ?? 'admin' const password = process.env.NC_PASS ?? 'admin' - return { Authorization: `Basic ${Buffer.from(`${user}:${password}`).toString('base64')}` } + return { Authorization: `Basic ${Buffer.from(`${apiUser()}:${password}`).toString('base64')}` } +} + +/** + * Create a note and rewrite it through WebDAV until it has one version per + * given revision. Writing goes around the app on purpose, so the note keeps its + * file name instead of being retitled from the changed content. + * + * @param request The request fixture to use + * @param revisions The contents to write, oldest first + * @return The id of the created note + */ +export async function createNoteRevisions(request: APIRequestContext, revisions: string[]): Promise { + expect(revisions.length, 'revisions to write').toBeGreaterThan(0) + + const created = await request.post('/index.php/apps/notes/api/v1/notes', { + headers: apiHeaders(), + data: { content: revisions[0] }, + }) + expect(created.ok(), 'creating the note').toBeTruthy() + + const note = await created.json() + const path = note.internalPath.split('/').map(encodeURIComponent).join('/') + + for (const content of revisions.slice(1)) { + // recent versions are thinned out to one per two seconds + await new Promise((resolve) => setTimeout(resolve, 3500)) + const written = await request.put(`/remote.php/dav/files/${apiUser()}${path}`, { + headers: apiHeaders(), + data: content, + }) + expect(written.ok(), 'writing a revision').toBeTruthy() + } + + return note.id } /** diff --git a/src/components/NotePlain.vue b/src/components/NotePlain.vue index 16d298696..236d001f6 100644 --- a/src/components/NotePlain.vue +++ b/src/components/NotePlain.vue @@ -221,6 +221,7 @@ export default { document.addEventListener('visibilitychange', this.onVisibilityChange) subscribe('files_versions:restore:requested', this.onFileRestoreRequested) subscribe('files_versions:restore:restored', this.onFileRestored) + subscribe('files_versions:restore:failed', this.onFileRestoreFailed) }, unmounted() { @@ -233,6 +234,7 @@ export default { this.onUpdateTitle(null) unsubscribe('files_versions:restore:requested', this.onFileRestoreRequested) unsubscribe('files_versions:restore:restored', this.onFileRestored) + unsubscribe('files_versions:restore:failed', this.onFileRestoreFailed) }, methods: { @@ -339,22 +341,23 @@ export default { }, interval * 1000) }, - refreshNote() { - if (!this.note) { - this.startRefreshTimer() - return - } - if (this.note.unsaved && !this.note.conflict) { - this.startRefreshTimer() - return - } - refreshNote(parseInt(this.noteId), this.etag).then((etag) => { + async refreshNote() { + try { + if (!this.note) { + return + } + if (this.note.unsaved && !this.note.conflict) { + return + } + + const etag = await refreshNote(parseInt(this.noteId), this.etag) if (etag) { this.etag = etag this.$forceUpdate() } + } finally { this.startRefreshTimer() - }) + } }, onEdit(newContent) { @@ -432,24 +435,38 @@ export default { this.showConflict = false }, - async onFileRestoreRequested(event) { - const { fileInfo } = event + // the node of a restore carries a numeric fileid, a version a string fileId + isCurrentNote(fileId) { + return this.note && Number(fileId) === this.note.id + }, - if (!this.note || fileInfo.id !== this.note.id) { + onFileRestoreRequested({ node }) { + if (!this.isCurrentNote(node?.fileid)) { return } this.loading = true }, - async onFileRestored(version) { - if (!this.note || version.fileId !== this.note.id) { + onFileRestoreFailed(version) { + if (!this.isCurrentNote(version?.fileId)) { return } - this.refreshNote() this.loading = false }, + + async onFileRestored({ node }) { + if (!this.isCurrentNote(node?.fileid)) { + return + } + + try { + await this.refreshNote() + } finally { + this.loading = false + } + }, }, } diff --git a/src/components/NoteRich.vue b/src/components/NoteRich.vue index 4afd281ae..724bbd109 100644 --- a/src/components/NoteRich.vue +++ b/src/components/NoteRich.vue @@ -66,6 +66,7 @@ export default { subscribe('files:node:updated', this.fileUpdated) subscribe('files_versions:restore:requested', this.onFileRestoreRequested) subscribe('files_versions:restore:restored', this.onFileRestored) + subscribe('files_versions:restore:failed', this.onFileRestoreFailed) }, unmounted() { @@ -73,6 +74,7 @@ export default { unsubscribe('files:node:updated', this.fileUpdated) unsubscribe('files_versions:restore:requested', this.onFileRestoreRequested) unsubscribe('files_versions:restore:restored', this.onFileRestored) + unsubscribe('files_versions:restore:failed', this.onFileRestoreFailed) }, methods: { @@ -158,36 +160,41 @@ export default { return title.length > 0 ? title : t('notes', 'New note') }, - async onFileRestoreRequested(event) { - const { fileInfo } = event + // the node of a restore carries a numeric fileid, a version a string fileId + isCurrentNote(fileId) { + return this.note && Number(fileId) === this.note.id + }, - if (!this.note || fileInfo.id !== this.note.id) { + onFileRestoreRequested({ node }) { + if (!this.isCurrentNote(node?.fileid)) { return } this.loading = true }, - async onFileRestored(version) { - if (!this.note || version.fileId !== this.note.id) { + onFileRestoreFailed(version) { + if (!this.isCurrentNote(version?.fileId)) { return } - const etag = await refreshNote(parseInt(this.noteId), this.etag) + this.loading = false + }, - if (etag) { - this.etag = etag + async onFileRestored({ node }) { + if (!this.isCurrentNote(node?.fileid)) { + return } - const autoResolve = setInterval(() => { - const el = document.querySelector('[data-cy="resolveServerVersion"]') + try { + const etag = await refreshNote(parseInt(this.noteId), this.etag) - if (el) { - el.click() - clearInterval(autoResolve) + if (etag) { + this.etag = etag } - }, 200) - this.loading = false + } finally { + this.loading = false + } }, }, } From f5ce13d46e4f235ecfecb2c3a0a5e912bf551714 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Mon, 17 Aug 2026 22:51:33 +0200 Subject: [PATCH 8/8] test(notes): cover the sidebar tab selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit selectNoteSidebarTabs() decides which of the registered Files tabs a note sidebar ends up with, and the only way it was exercised was through Playwright — a Docker container, a login and a browser for a pure function. Covered: the allow-list, the ordering including a missing order, that a tab is kept while the node it would judge is still loading, that the predicate is asked with the node once there is one, that a predicate throwing drops only that tab, and that the registry it is handed is not reordered in place. Needs the vitest setup, which is added separately so it can land without waiting on this branch. Assisted-by: Claude Code:claude-opus-5[1m] Signed-off-by: Andy Scherzinger --- src/tests/sidebarTabs.spec.js | 98 +++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 src/tests/sidebarTabs.spec.js diff --git a/src/tests/sidebarTabs.spec.js b/src/tests/sidebarTabs.spec.js new file mode 100644 index 000000000..a451e320c --- /dev/null +++ b/src/tests/sidebarTabs.spec.js @@ -0,0 +1,98 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { describe, expect, it, vi } from 'vitest' +import { NOTE_SIDEBAR_TAB_IDS, selectNoteSidebarTabs } from '../sidebarTabs.js' + +const ids = (tabs, context) => selectNoteSidebarTabs(tabs, context).map((tab) => tab.id) + +const node = { basename: 'A note.md' } + +describe('selectNoteSidebarTabs', () => { + it('keeps the tabs a note sidebar hosts', () => { + expect(NOTE_SIDEBAR_TAB_IDS).toEqual(['sharing', 'files_versions']) + }) + + it('drops every tab that is not on the allow-list', () => { + const tabs = [ + { id: 'sharing' }, + { id: 'activity' }, + { id: 'files_versions' }, + { id: 'comments' }, + ] + + expect(ids(tabs)).toEqual(['sharing', 'files_versions']) + }) + + it.each([ + ['nothing registered', []], + ['a registry that is not there yet', null], + ['entries without an id', [{}, null, undefined]], + ])('returns no tabs for %s', (_label, tabs) => { + expect(ids(tabs)).toEqual([]) + }) + + it('sorts by the order the registering apps asked for', () => { + const tabs = [ + { id: 'files_versions', order: 5 }, + { id: 'sharing', order: 1 }, + ] + + expect(ids(tabs)).toEqual(['sharing', 'files_versions']) + }) + + it('treats a missing order as zero', () => { + const tabs = [ + { id: 'files_versions', order: 1 }, + { id: 'sharing' }, + ] + + expect(ids(tabs)).toEqual(['sharing', 'files_versions']) + }) + + it('keeps a tab while the node it would judge is still loading', () => { + const tabs = [{ id: 'files_versions', enabled: () => false }] + + expect(ids(tabs, { node: null })).toEqual(['files_versions']) + }) + + it('asks the tab once the node is there', () => { + const enabled = vi.fn(() => true) + const folder = { basename: 'Notes' } + const view = { id: 'notes' } + + expect(ids([{ id: 'sharing', enabled }], { node, folder, view })).toEqual(['sharing']) + expect(enabled).toHaveBeenCalledWith({ node, folder, view }) + }) + + it('drops a tab that says it does not apply to the node', () => { + const tabs = [ + { id: 'sharing' }, + { id: 'files_versions', enabled: () => false }, + ] + + expect(ids(tabs, { node })).toEqual(['sharing']) + }) + + it('drops only the tab whose predicate throws', () => { + const tabs = [ + { id: 'sharing' }, + { id: 'files_versions', enabled: () => { throw new Error('no node for you') } }, + ] + + expect(ids(tabs, { node })).toEqual(['sharing']) + }) + + it('leaves the registry it was given alone', () => { + const tabs = [ + { id: 'files_versions', order: 5 }, + { id: 'sharing', order: 1 }, + ] + + selectNoteSidebarTabs(tabs, { node }) + + expect(tabs.map((tab) => tab.id)).toEqual(['files_versions', 'sharing']) + }) +})