Add breadcrumbs to navigation #7995
Conversation
Summary of ChangesHello @eliandoran, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new breadcrumb navigation feature to enhance user experience by providing clear hierarchical context within the application. It involves the creation of a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new breadcrumb component, which is a great addition for improving navigation. The implementation is solid, and it's well-integrated into the desktop layout. I've identified a few areas for improvement, including a potential issue with an incorrect file extension in an import and a couple of opportunities to refactor the new Breadcrumb.tsx component for better clarity and maintainability. Overall, this is a well-executed feature.
| import PromotedAttributes from "../widgets/PromotedAttributes.jsx"; | ||
| import SpacerWidget from "../widgets/launch_bar/SpacerWidget.jsx"; | ||
| import LauncherContainer from "../widgets/launch_bar/LauncherContainer.jsx"; | ||
| import Breadcrumb from "../widgets/Breadcrumb.jsx"; |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| function buildNotePaths(notePathArray: string[] | undefined) { | ||
| if (!notePathArray) return []; | ||
|
|
||
| let prefix = ""; | ||
| const output: string[] = []; | ||
| for (const notePath of notePathArray) { | ||
| output.push(`${prefix}${notePath}`); | ||
| prefix += `${notePath}/`; | ||
| } | ||
| return output; | ||
| } |
There was a problem hiding this comment.
This function can be written more declaratively and concisely by using the map method. This would improve readability and maintainability.
function buildNotePaths(notePathArray: string[] | undefined) {
if (!notePathArray) {
return [];
}
return notePathArray.map((_, i) => notePathArray.slice(0, i + 1).join('/'));
}
dosubot
bot
added
the
size:L
There was a problem hiding this comment.
Pull request overview
This PR adds a breadcrumb navigation component to the Trilium Notes desktop layout, replacing the title row with a breadcrumb that shows the note's hierarchical path. The implementation includes floating header behavior and adjusts the positioning of ribbon and floating buttons to work with the new layout.
Key Changes:
- Implements a new breadcrumb component with collapsible paths and dropdown navigation for child notes
- Refactors content header to float on scroll with dynamic positioning via CSS custom properties
- Adjusts floating buttons positioning to account for dynamic header heights
- Moves the title row from top-level to inside the scrolling content area
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
apps/client/src/widgets/Breadcrumb.tsx |
New breadcrumb component with path navigation and dropdown menus |
apps/client/src/widgets/Breadcrumb.css |
Styling for the breadcrumb component with flexible layout |
apps/client/src/layouts/desktop_layout.tsx |
Replaces title row with breadcrumb, moves title inside scrolling container |
apps/client/src/widgets/containers/content_header.ts |
Adds floating behavior on scroll with CSS custom property updates |
apps/client/src/widgets/containers/content_header.css |
Styles for floating header with sticky positioning |
apps/client/src/widgets/FloatingButtons.tsx |
Removes event-based positioning in favor of CSS variables |
apps/client/src/widgets/FloatingButtons.css |
Updates positioning to use CSS custom properties for dynamic height |
apps/client/src/widgets/ribbon/Ribbon.tsx |
Adds height tracking via ResizeObserver and CSS custom properties |
apps/client/src/widgets/ribbon/style.css |
Makes ribbon sticky when content header floats |
apps/client/src/widgets/react/hooks.tsx |
Updates useChildNotes to handle undefined parent IDs |
apps/client/src/widgets/react/react_utils.tsx |
Makes separator parameter in joinElements accept ComponentChild |
apps/client/src/services/link.ts |
Fixes title handling to allow empty strings (was treating falsy as undefined) |
apps/client/src/components/app_context.ts |
Removes deprecated contentSafeMarginChanged event |
eslint.config.mjs |
Adds .tsx files to ESLint configuration |
| Various CSS files | Minor whitespace and styling fixes for layout consistency |
| if (this.isFloating || this.parentElement!.scrollTop === 0) { | ||
| parentEl!.style.setProperty("--content-header-height", `${this.currentHeight}px`); | ||
| } else { | ||
| parentEl!.style.removeProperty("--content-header-height"); |
There was a problem hiding this comment.
Potential null pointer exception: parentEl could be null if .closest() doesn't find a matching ancestor, but the code uses parentEl! (non-null assertion) at lines 73 and 75. This will throw a runtime error if the note-split container is not found.
Add a null check before accessing parentEl:
const parentEl = this.parentElement?.closest<HTMLDivElement>(".note-split");
if (!parentEl) return;| if (this.isFloating || this.parentElement!.scrollTop === 0) { | |
| parentEl!.style.setProperty("--content-header-height", `${this.currentHeight}px`); | |
| } else { | |
| parentEl!.style.removeProperty("--content-header-height"); | |
| if (!parentEl) return; | |
| if (this.isFloating || this.parentElement!.scrollTop === 0) { | |
| parentEl.style.setProperty("--content-header-height", `${this.currentHeight}px`); | |
| } else { | |
| parentEl.style.removeProperty("--content-header-height"); |
| let prefix = ""; | ||
| const output: string[] = []; | ||
| for (const notePath of notePathArray) { | ||
| output.push(`${prefix}${notePath}`); | ||
| prefix += `${notePath}/`; | ||
| } | ||
| return output; |
There was a problem hiding this comment.
The function buildNotePaths mutates the prefix variable on each iteration, which could be simplified using map with slice:
function buildNotePaths(notePathArray: string[] | undefined) {
if (!notePathArray) return [];
return notePathArray.map((_, index) =>
notePathArray.slice(0, index + 1).join("/")
);
}This approach is more functional and easier to understand.
| let prefix = ""; | |
| const output: string[] = []; | |
| for (const notePath of notePathArray) { | |
| output.push(`${prefix}${notePath}`); | |
| prefix += `${notePath}/`; | |
| } | |
| return output; | |
| return notePathArray.map((_, index) => | |
| notePathArray.slice(0, index + 1).join("/") | |
| ); |
| export default function Breadcrumb() { | ||
| const { note, noteContext } = useNoteContext(); | ||
| const notePath = buildNotePaths(noteContext?.notePathArray); | ||
|
|
||
| return ( | ||
| <div className="breadcrumb"> | ||
| {notePath.length > COLLAPSE_THRESHOLD ? ( | ||
| <> | ||
| {notePath.slice(0, INITIAL_ITEMS).map((item, index) => ( | ||
| <Fragment key={item}> | ||
| {index === 0 | ||
| ? <BreadcrumbRoot noteContext={noteContext} /> | ||
| : <BreadcrumbItem notePath={item} activeNotePath={noteContext?.notePath ?? ""} /> | ||
| } | ||
| <BreadcrumbSeparator notePath={item} activeNotePath={notePath[index + 1]} noteContext={noteContext} /> | ||
| </Fragment> | ||
| ))} | ||
| <BreadcrumbCollapsed items={notePath.slice(INITIAL_ITEMS, -FINAL_ITEMS)} noteContext={noteContext} /> | ||
| {notePath.slice(-FINAL_ITEMS).map((item, index) => ( | ||
| <Fragment key={item}> | ||
| <BreadcrumbSeparator notePath={notePath[notePath.length - FINAL_ITEMS - (1 - index)]} activeNotePath={item} noteContext={noteContext} /> | ||
| <BreadcrumbItem notePath={item} activeNotePath={noteContext?.notePath ?? ""} /> | ||
| </Fragment> | ||
| ))} | ||
| </> | ||
| ) : ( | ||
| notePath.map((item, index) => ( | ||
| <Fragment key={item}> | ||
| {index === 0 | ||
| ? <BreadcrumbRoot noteContext={noteContext} /> | ||
| : <BreadcrumbItem notePath={item} activeNotePath={noteContext?.notePath ?? ""} /> | ||
| } | ||
| {(index < notePath.length - 1 || note?.hasChildren()) && | ||
| <BreadcrumbSeparator notePath={item} activeNotePath={notePath[index + 1]} noteContext={noteContext} />} | ||
| </Fragment> | ||
| )) | ||
| )} | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new Breadcrumb component lacks test coverage. Given that the codebase has comprehensive tests for other widgets (e.g., apps/client/src/widgets/collections/ components have .spec.ts files), this component should have tests covering:
- Path building logic with various note path lengths
- Collapsed breadcrumb behavior when exceeding COLLAPSE_THRESHOLD
- Dropdown content rendering for child notes
- Edge cases like empty paths or missing notes
Closes #3754.