Skip to content

🔢 Expose file versions in the sidebar - #1972

Open
karlitschek wants to merge 8 commits into
mainfrom
feat/noid/expose-versions
Open

🔢 Expose file versions in the sidebar#1972
karlitschek wants to merge 8 commits into
mainfrom
feat/noid/expose-versions

Conversation

@karlitschek

@karlitschek karlitschek commented Aug 6, 2026

Copy link
Copy Markdown
Member

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. Exposing it turned the note sidebar into something worth bringing closer to the Files one, so this PR does both.

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.

What else is in here

  • Versions is reachable from the note's action menu, next to "Share". That menu lives in the note list row, so it is present in every editor mode.
  • "Open sidebar" in the editor's own actions menu, using the DockRight icon and the label the Viewer app uses for the same entry. That menu only exists in the markdown editor — the rich editor brings its own menu bar — so the note list row remains the path that works in every mode.
  • The sidebar header carries the file metadata: size, modification date, and the owner as a bubble with avatar and display name. This reimplements what the Files sidebar renders in its subname slot. No new data plumbing was needed — fetchDavNode() already uses the default propfind, which asks for getcontentlength, getlastmodified, owner-id and owner-display-name.
  • The sharing tab icon is outlined until its tab is active, then fills. Versions keeps the icon its app registered, as there is no outlined counterpart of backup-restore, and both icons line up with each other.
  • Restoring a version updates the app right away. The versions list refreshes in place instead of waiting for a reload, and the editor is replaced by a spinner while the restored content loads, so it cannot be typed into halfway through the switch.
Implementation notes
  • 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. A predicate that throws drops that tab, with the failure logged, instead of taking the sidebar down.
  • Tabs initialise independently, so one failing to define its custom element no longer hides the others. A tab whose element never got defined is not offered at all, rather than sitting in the tab bar with an empty pane behind it; it gets another chance on the next open.
  • No tab is waited for indefinitely. customElements.whenDefined() never settles for an element that is never defined, so it is raced against a timeout — otherwise an onInit() that resolves without delivering its element would leave the sidebar spinning, with the pending initialization stuck for every later open as well.
  • Opens that overlap share the in-flight initialization of a custom element instead of assuming it already succeeded.
  • 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.
  • The requested tab is clamped to one the sidebar actually renders — NcAppSidebar falls back to its first tab on its own, but does not report that back.
  • The icon swap happens inside the #icon slot rather than through a dedicated one, since @nextcloud/vue has no #icon-active: NcAppSidebarTab exposes renderIcon() without arguments. The tab button invokes it from its own render function, so reading the resolved tab id there tracks it.
  • Mixing the two icon systems misaligns the tab bar: NcIconSvgWrapper reserves a clickable-area box around its svg, while a material design icon is only as big as itself, which put the versions icon 7px below the sharing one. The wrapper's inline modifier drops that box, leaving both at 20px — the same as the sidebar in 📊 Note details tab in the sidebar #1974.
  • The versions tab 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 sidebar answers that by swapping its current node, the way the Files sidebar store does, matching on source. The subname picks it up as well, so size and modification date do not lag behind a restore either.
  • The restore handlers in both editors were reading a payload files_versions no longer emits — a fileInfo key, and a fileId on the version — so the requested one threw and the restored one always returned early. They take the node from the event now and compare its fileid, which brings back both the loading state and the immediate refresh. NotePlain.refreshNote() returns its promise so that loading state outlasts the refresh, and both editors clear it on files_versions:restore:failed rather than staying behind the spinner for good.
  • 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.

Tests

New playwright/e2e/note-sidebar.spec.ts:

  • opens the versions tab from the actions menu
  • refreshes the versions list once a version is restored
  • keeps the editor behind a spinner while a restored version loads
  • shows the size, the modification date and the owner of the note
  • renders the allow-listed tabs only
  • switches between the sharing and versions tabs
  • fills the sharing icon only while its tab is active
  • lines the tab icons up with each other
  • falls back to the first tab when the requested one is unavailable
  • markdown editor › opens the sidebar from the editor actions menu

openNoteActions() moved from note-actions.spec.ts into support/note.ts so both specs share it. Two helpers were added there as well: setNoteMode() for the markdown-editor case, and createNoteRevisions(), which creates a note and then rewrites it over WebDAV — going around the app on purpose, as writing through it would retitle and thereby rename the note. Both take Playwright's isolated request fixture rather than page.request, whose basic auth would replace the session cookie the browser is logged in with.

src/tests/sidebarTabs.spec.js covers selectNoteSidebarTabs() without a server or a browser: 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. It needs the vitest setup from #1989, which should merge first.

🖼️ Screenshots

🏚️ Before 🏡 After
notes_versioning_before 2026-08-18 10_51_48-Notes - Nextcloud — Mozilla Firefox

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@karlitschek
karlitschek requested review from enjeck and silverkszlo and a lite review from Copilot and removed request for enjeck and silverkszlo August 6, 2026 17:26

This comment was marked as resolved.

@enjeck enjeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add screenshots showing what the UI looks like. Maybe significant enough tp require designer input

@AndyScherzinger AndyScherzinger added 3. to review AI assisted This PR contains AI-assisted commits enhancement New feature or request labels Aug 11, 2026
@AndyScherzinger
AndyScherzinger force-pushed the feat/noid/expose-versions branch 2 times, most recently from 4c30cea to 71fb7b5 Compare August 16, 2026 10:27
@AndyScherzinger AndyScherzinger self-assigned this Aug 16, 2026
@AndyScherzinger
AndyScherzinger force-pushed the feat/noid/expose-versions branch from 71fb7b5 to f1635de Compare August 16, 2026 10:50
@AndyScherzinger
AndyScherzinger requested a lite review from Copilot August 16, 2026 10:58

This comment was marked as outdated.

@AndyScherzinger
AndyScherzinger force-pushed the feat/noid/expose-versions branch 2 times, most recently from ad83197 to ca9796d Compare August 16, 2026 11:46
@AndyScherzinger
AndyScherzinger requested a lite review from Copilot August 16, 2026 11:46
@AndyScherzinger AndyScherzinger added this to the 6.1.0 milestone Aug 16, 2026

This comment was marked as resolved.

This comment was marked as outdated.

Frank Karlitschek and others added 4 commits August 18, 2026 09:42
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 <info@andy-scherzinger.de>
Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com>
The note editor's actions menu had no way to reach the sidebar, so
sharing and versions were only reachable from the note list row.

The entry uses the DockRight icon and the "Open sidebar" label the
Viewer app uses for the same purpose, and sits next to "Full screen"
as it does there. That menu only exists in the markdown editor — the
rich editor brings its own menu bar — so the note list row remains the
path that works in every editor mode.

The e2e helper that switches the editor mode takes the isolated request
fixture rather than page.request: the latter shares the browser cookie
jar, so its basic auth replaces the session cookie and logs the page out
on the next reload.

Assisted-by: Claude Code:claude-opus-5[1m]
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The sidebar header carried only the note title, while the Files sidebar
shows the file metadata right below it.

Reimplements the subname the Files app renders in its sidebar header
(apps/files/src/components/FilesSidebar/FilesSidebarSubname.vue): the
formatted file size, the modification date and the owner as a user
bubble with avatar and display name, rendered through NcAppSidebar's
subname slot.

No new data plumbing was needed — fetchDavNode() already uses the
default propfind, which asks for getcontentlength, getlastmodified,
owner-id and owner-display-name.

The metadata is the state of the file at the moment the sidebar was
opened; nothing re-fetches the node while it stays open.

Assisted-by: Claude Code:claude-opus-5[1m]
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The sidebar kept three collections to answer one question — is this
tab's custom element defined?

* initializedTabs was only ever written once customElements.whenDefined()
  had resolved, which is exactly when customElements.get() starts
  returning the constructor — and that is checked first in the same
  guard. Redundant, so it is gone.
* pendingTabs caches an in-flight side effect on the global custom
  element registry rather than component state, so it moves to module
  scope along with the two functions that use it.
* loadingTab is raised once per run instead of from the per-tab helper.
  Guarding it per tab bought nothing, since loadNodeContext() raises
  loadingContext on every open anyway.

The active tab is now clamped by a computed feeding the `active` prop
instead of a watcher writing back into activeTab, and the reset shared
by onSidebarOpen and onClosed became a single method.

tabError is dropped as well: now that a tab failing to define its
element is no longer offered, it could only be set when no tab was left
at all, where the empty state already carries a message. The failure
itself is still logged with the tab id.

Assisted-by: Claude Code:claude-opus-5[1m]
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

This comment was marked as outdated.

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 <info@andy-scherzinger.de>
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 <info@andy-scherzinger.de>
@AndyScherzinger
AndyScherzinger force-pushed the feat/noid/expose-versions branch from 788f057 to 372e025 Compare August 18, 2026 08:12
@AndyScherzinger
AndyScherzinger requested a lite review from Copilot August 18, 2026 10:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Suppressed comments (6)

src/components/NotePlain.vue:1

  • refreshNote() now awaits and can throw; if it rejects, startRefreshTimer() won’t run and (more importantly) onFileRestored() won’t reach this.loading = false, potentially leaving the editor stuck behind the spinner. Wrap the awaited call(s) in try { ... } finally { ... } so the refresh timer is always restarted and loading is always cleared after restore attempts.
<!--

src/components/NoteSidebarSubname.vue:22

  • When node.owner is present but owner-display-name is missing, NcUserBubble will receive an undefined displayName, which can render as an empty/odd label. Consider falling back to a sensible default (e.g., node.owner) when owner-display-name is not available.
		<template v-if="node.owner">
			<span class="note-sidebar-subname__separator">•</span>
			<NcUserBubble class="note-sidebar-subname__owner"
				:displayName="ownerDisplayName"
				:title="t('notes', 'Owner')"
				:user="node.owner"
			/>
		</template>

src/components/NoteSidebarSubname.vue:52

  • When node.owner is present but owner-display-name is missing, NcUserBubble will receive an undefined displayName, which can render as an empty/odd label. Consider falling back to a sensible default (e.g., node.owner) when owner-display-name is not available.
		ownerDisplayName() {
			return this.node.attributes?.['owner-display-name']
		},

playwright/support/note.ts:53

  • The fixed 3.5s sleep per revision can significantly slow down e2e runs and may still be flaky if server-side thinning behavior changes. Prefer a more deterministic wait (e.g., poll until versions endpoint shows the expected count / mtime change) or centralize the delay as a named constant with a brief rationale and a single place to tune it.
	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()
	}

src/components/NoteItem.vue:50

  • hasVersionsTab() is called during render and queries getSidebarTabs() each time. If the registry lookup is non-trivial, this can add avoidable overhead in list rendering; also, if sidebar tabs are registered asynchronously, this approach may not update without another reactive trigger. Consider caching this as a computed value (optionally refreshed when the sidebar registry changes) rather than recalculating on every render.
			<NcActionButton v-if="hasVersionsTab()" @click="onShowVersions">
				<template #icon>
					<BackupRestoreIcon :size="20" />
				</template>
				{{ t('notes', 'Versions') }}
			</NcActionButton>

src/components/NoteItem.vue:352

  • hasVersionsTab() is called during render and queries getSidebarTabs() each time. If the registry lookup is non-trivial, this can add avoidable overhead in list rendering; also, if sidebar tabs are registered asynchronously, this approach may not update without another reactive trigger. Consider caching this as a computed value (optionally refreshed when the sidebar registry changes) rather than recalculating on every render.
		hasVersionsTab() {
			return getSidebarTabs().some((tab) => tab?.id === 'files_versions')
		},

Comment thread src/components/NoteShareSidebar.vue
Comment thread src/components/NoteShareSidebar.vue
Comment thread src/components/NotePlain.vue Outdated
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 <info@andy-scherzinger.de>

This comment was marked as low quality.

@AndyScherzinger

Copy link
Copy Markdown
Member

restoring versions while using Nextcloud Text leads to unexpected version creations, still investigating, so putting it back to draft state until resolved. Sorry for the GH notification noise.

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 <info@andy-scherzinger.de>
@AndyScherzinger
AndyScherzinger force-pushed the feat/noid/expose-versions branch from 98c52c5 to f5ce13d Compare August 18, 2026 18:13
@AndyScherzinger
AndyScherzinger marked this pull request as ready for review August 18, 2026 19:01
@AndyScherzinger

Copy link
Copy Markdown
Member

Update: versioning works well, but the above is an issue in Text, now reported at nextcloud/text#9077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review AI assisted This PR contains AI-assisted commits enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants