π Note details tab in the sidebar - #1974
Conversation
86ae56b to
55c2179
Compare
55c2179 to
84f9b9a
Compare
67be3df to
651ea2a
Compare
dafa4af to
6b22799
Compare
There was a problem hiding this comment.
As discussed with @AndyScherzinger β if "Details" moves to be the last (rightmost) tab.
Most of the info is already existing in other places, but yeah we will introduce tabs for versions anyway, so good to keep it consistent. :)
bea2946 to
5b66fe2
Compare
23dc96a to
75d0fff
Compare
Codecov Reportβ Patch coverage is
π’ Thoughts on this report? Let us know! |
cb2fd76 to
48f959b
Compare
dfa45a5 to
762bada
Compare
|
please resolve the merge conflicts now that #1972 has been merged |
762bada to
41ff98b
Compare
41ff98b to
f21c026
Compare
| .replaceAll(/^~~~[\s\S]*?^~~~/gm, ' ') | ||
| // images, which have no words to keep, then links, which keep their label | ||
| .replaceAll(/!\[[^\]]*\]\([^)]*\)/g, ' ') | ||
| .replaceAll(/\[([^\]]*)\]\([^)]*\)/g, '$1') |
There was a problem hiding this comment.
Claude suggests the following changes here:
// images, which have no words to keep, then links, which keep their label
- .replaceAll(/!\[[^\]]*\]\([^)]*\)/g, ' ')
- .replaceAll(/\[([^\]]*)\]\([^)]*\)/g, '$1')
+ .replaceAll(/!\[[^\]\n]*\]\([^)\n]*\)/g, ' ')
+ .replaceAll(/\[([^\]\n]*)\]\([^)\n]*\)/g, '$1')
...to avoid a performance regression. Without the \n, an opening [ with no ] after it makes the engine scan to the end of the note before the match fails. It then resumes at the next [ and scans to the end again β one full scan of the note per stray bracket. A 120 KB note full of them takes ~2.4 s here, and the tab is frozen for all of it. Bounding the class to a line makes each failed attempt cost one line instead of the whole note: same note, ~2 ms.
Nothing that sits on a single line strips differently, so stray brackets are counted as words exactly as before. The one behaviour change is a link or image label deliberately wrapped across two lines: it stops being recognised as a link, so its brackets stay in the text and count as words. That's the same trade the emphasis pattern below already makes.
silverkszlo
left a comment
There was a problem hiding this comment.
cool, sidebar looks good and complete now, thanks! :)
|
Thanks @silverkszlo π I am running another review via an agent, let's see if it returns with findings, else I agree - good to go |
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>
e35ec1f to
82da15d
Compare
|
@silverkszlo I am done and CI is green π |
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)