π Note details tab in the sidebar - #1974
Merged
Merged
Conversation
karlitschek
requested review from
enjeck and
silverkszlo
and
a lite review from Copilot
and removed request for
enjeck and
silverkszlo
August 6, 2026 18:32
AndyScherzinger
marked this pull request as draft
August 16, 2026 22:48
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
from
August 16, 2026 23:12
86ae56b to
55c2179
Compare
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
from
August 17, 2026 06:27
55c2179 to
84f9b9a
Compare
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
4 times, most recently
from
August 17, 2026 21:00
67be3df to
651ea2a
Compare
1 task
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
2 times, most recently
from
August 24, 2026 16:48
bea2946 to
5b66fe2
Compare
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
2 times, most recently
from
August 30, 2026 10:04
23dc96a to
75d0fff
Compare
Codecov Reportβ Patch coverage is
π’ Thoughts on this report? Let us know! |
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
2 times, most recently
from
August 31, 2026 08:06
cb2fd76 to
48f959b
Compare
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
3 times, most recently
from
September 12, 2026 10:57
dfa45a5 to
762bada
Compare
Contributor
|
please resolve the merge conflicts now that #1972 has been merged |
silverkszlo
reviewed
Sep 14, 2026
silverkszlo
reviewed
Sep 14, 2026
silverkszlo
reviewed
Sep 14, 2026
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
from
September 14, 2026 14:01
762bada to
41ff98b
Compare
silverkszlo
reviewed
Sep 14, 2026
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
from
September 14, 2026 15:02
41ff98b to
f21c026
Compare
silverkszlo
reviewed
Sep 14, 2026
silverkszlo
approved these changes
Sep 14, 2026
silverkszlo
left a comment
Contributor
There was a problem hiding this comment.
cool, sidebar looks good and complete now, thanks! :)
Member
|
Thanks @silverkszlo π I am running another review via an agent, let's see if it returns with findings, else I agree - good to go |
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
from
September 14, 2026 15:45
f21c026 to
e35ec1f
Compare
The sidebar hosts the tabs the Files sidebar contributes but says nothing about the note itself. This adds a tab of Notes' own with the note's category, a reading estimate, its path, and a read-only marker when the note cannot be written. Size, modification date and owner are already in the sidebar header and are not repeated here. Most of it is free: everything but the reading estimate is on the note in the store. The estimate is not free, and that shapes the design. The note list payload excludes `content`, so a note that has never been opened has none client-side. Rather than fetching every body up front, the tab pulls the one note it needs and only once its tab is actually selected β opening the sidebar to share a note does not drag its body down with it. Until then the row shows a placeholder, and a body that cannot be read shows a dash rather than disappearing. fetchNote() only rejects on a missing note and reports everything else itself, so the content is what says whether the fetch worked. The body is fetched for one note at a time, and which note that is guards the result: the sidebar can be sent to another note while a fetch is still on its way β from a row of the list, or by the list navigating β and the answer to the first request must then neither be reported for the second note nor keep its own fetch from starting. Estimating a reading time means counting words without the markup, otherwise '#' and '**' inflate the number. noteStats.js strips the obvious things β fenced code, image syntax, link targets while keeping labels, heading, quote and list markers, setext underlines, emphasis β and leaves the rest alone. A full parse would be much more code for a number nobody checks to the decimal. Emphasis is matched within a line, both because it cannot span a blank line and so an unclosed marker does not scan the rest of the note for a partner. Deliberately no word or character count. The Text app shows both for the note open in rich mode, computed from its parsed document rather than from the markdown, so a second pair of numbers here would differ from those and invite the question of which is right. A reading estimate is the one figure Text does not offer. The tab is not one from the Files registry, so it renders outside the loop over the allow-listed tabs, with an order that puts it after them: Sharing stays the first and default tab. Its icon is outlined until its tab is active, following what the sharing icon already does, and its rows are inset to the same 8px the Files tabs put their own content at. resolvedTab() clamps over both kinds of tab now, so the fallback can land on this one when it is the only tab there is. "Details" is offered in the note's action menu next to Share and Versions. The sidebar also follows the note the list navigates to. It was opened for one note and then stayed on it, so picking another note left it showing the previous note's sharing, versions and details, which reads as stale data rather than as a sidebar that belongs to a different note. It now watches the note id in the route and re-opens itself for whatever the list navigates to, keeping the tab that is in view, the way the Files sidebar does. Note selection is read from the route rather than from the store: setSelectedNote() is never called anywhere, so store.notes.selectedNote is always null. Reusing onSidebarOpen() means the switch goes through the same path as opening the sidebar in the first place, so the DAV node, the parent folder and the note body are all reloaded for the new note rather than half-updated. Opening no longer skips the node context when no Files tab is registered, since the Details tab needs it. Two of the values the tab shows would otherwise go stale. updateNote() copies the note attributes only, so internalPath and readonly β reported by the server but not among them β were never refreshed. And the category endpoint answers with the category string alone, so nothing carried the new path back after a move; setCategory() refetches the note now, and a refetch that does not arrive is not reported as the category update having failed. Assisted-by: Claude Code:claude-opus-5[1m] Assisted-by: Claude Code:claude-fable-5 Co-Authored-By: Andy Scherzinger <info@andy-scherzinger.de> Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com> Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
AndyScherzinger
force-pushed
the
feat/noid/note-info
branch
from
September 14, 2026 16:50
e35ec1f to
82da15d
Compare
Member
|
@silverkszlo I am done and CI is green π |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The note sidebar hosts Sharing and Versions but says nothing about the note itself. This adds a tab of Notes' own β Details β with the note's category, a reading estimate, its path, and a read-only marker when the note cannot be written.
Everything but the reading estimate is already on the note in the store, so the tab costs one fetch at most. Size, modification date and owner are not repeated here: #1972 put them in the sidebar header, where they describe the note as a whole rather than belonging to one of its tabs.
The reading estimate shapes the design
The note list payload excludes
content, so a note that has never been opened has none client-side. Rather than fetching every body up front, the tab pulls the one note it needs, and only once its tab is actually selected β opening the sidebar to share a note does not drag its body down with it. Until then the row shows a placeholder, and a body that cannot be read shows a dash rather than disappearing.NcAppSidebarTabkeeps an inactive tab mounted and hides it in CSS, so the tab's content is rendered only while it is the active one. Otherwise the estimate would recount the whole body on every keystroke of someone editing the note with the sidebar open on Sharing.The body is fetched for one note at a time, and which note that is guards the result: the sidebar can be sent to another note while a fetch is still on its way β from a row of the list, or by the list navigating β and the answer to the first request must then neither be reported for the second note nor keep its own fetch from starting.
Counting words means ignoring the markup, otherwise
#and**inflate the number.noteStats.jsstrips the obvious things β fenced code, image syntax, link targets while keeping labels, heading, quote and list markers, setext underlines, emphasis β and leaves the rest alone. A full parse would be much more code for a number nobody checks to the decimal. Emphasis is matched within a line, both because it cannot span a blank line and so an unclosed marker does not scan the rest of the note looking for a partner.Deliberately no word or character count
The Text app already shows both for the note open in rich mode, computed from its parsed document rather than from the markdown. The two definitions disagree β for
**bold** [label](https://example.com)Text counts 10 characters and a markdown-based count sees 37 β so a second pair of numbers here would invite the question of which is right. A reading estimate is the one figure Text does not offer.Fitting in next to the registered tabs
The tab is not one from the Files registry, so it renders outside the loop over the allow-listed ones, with an order that puts it after them: Sharing stays the first and default tab and Versions keeps its place. Its icon is outlined until its tab is active, following what the sharing icon already does, and its rows are inset to the same 8px the Files tabs put their own content at.
resolvedTab()clamps over both kinds of tab now rather than the registered ones alone, so the fallback can land on this tab when it is the only one there is β and the "not available right now" empty state is keyed on that same list, so it cannot render alongside a tab that is on screen. Opening the sidebar no longer skips loading the DAV node when no Files tab is registered, since the header and this tab both need it."Details" is offered in the note's action menu next to Share and Versions.
Keeping up with the note
The sidebar is opened for one note, but the list keeps navigating. An open sidebar now follows whichever note is being looked at, the way the Files one does, keeping the tab in view. It watches the note id in the route rather than the store's
selectedNote, which nothing ever sets.Within the tab, the category was reactive but the path was not, and needed a full reload. Two separate causes, both required:
updateNote()copies the note attributes only.internalPathis not one of them, and neither isreadonly, so both were left behind on every update β a refetch could not have refreshed them either.setCategory()refetches the note now. A refetch that does not arrive is not the category update having failed, so its failure is not reported as one.Measured on a dev instance: neither fix, the path never updates; the store fix alone, 18.9s (the next background sync); both, 415ms.
Layout
Rows are stacked β label above value β rather than laid out in two columns. A path is long enough that a two-column row either wraps inside a narrow value column or has to break the rhythm by spanning the full width on its own; stacking every row removes the special case and gives each value the whole sidebar. It also leaves room for translated labels, which run considerably longer than the English ones.
core/css/apps.scssstylesdl/dt/ddfor prose in a wide content area: 12px of padding on every side, and a fixed 130px label column aligned to its end. In a sidebar that doubled the height of every row and left the labels ragged, so the component resets all three. Worth knowing for anyone else reaching for a description list.Tests
playwright/e2e/note-sidebar.spec.tsgains cases that open the details tab from the actions menu, fill its icon only while it is active, estimate the reading time from the note body, load a body for a note that has never been opened, load the body of the note the sidebar moves to while another is still on its way, follow the note the list navigates to, and mark the reading time unavailable when the body cannot be loaded. Its existing tab-count case now expects three tabs rather than two.src/tests/noteStats.spec.jscoversnoteStats.jsβ every markup rulestripMarkup()implements, including the non-breaking space a task checkbox may contain, plus a body that is absent or not a string and a reading estimate that neither rounds an existing note down to nothing nor misrounds at the boundaries.src/tests/notesStore.spec.jscoversupdateNote()propagatinginternalPathandreadonly. Those are unit tests rather than e2e ones on purpose: the path refreshes within seconds in the e2e container whether the fix is there or not, so no browser test can fail for it.π€ AI (if applicable)