Standalone mobile nav - #9525
Conversation
|
📚 Website preview is ready! 🔗 Preview URL: https://pr-9525.trilium-homepage.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
|
🖥️ App preview is ready! 🔗 Preview URL: https://pr-9525.trilium-app.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
There was a problem hiding this comment.
Code Review
This pull request introduces a touch-native MobileNoteNavigator for mobile devices, replacing the previous Fancytree-based implementation. To support this, the tree context menu logic and several command executors were refactored into shared, UI-agnostic modules. The PR also includes dependency updates for Node.js and TypeScript, along with CSS refinements for rendered content. Feedback was provided regarding the navigation stack construction logic under hoisting constraints and the need for a null check when accessing parent nodes in tree commands.
| let start = parentSegments.indexOf(hoistedId); | ||
| if (start < 0) start = 0; | ||
| const clamped = parentSegments.slice(start); | ||
| if (clamped.length === 0) { | ||
| return [hoistedId]; | ||
| } | ||
|
|
||
| const stack: string[] = []; | ||
| for (let i = 0; i < clamped.length; i++) { | ||
| stack.push(clamped.slice(0, i + 1).join("/")); | ||
| } |
There was a problem hiding this comment.
The logic for building the navigation stack has two issues when hoisting is active:
- If the active note is not a descendant of the hoisted note (
start < 0), it incorrectly falls back to showing the path from the real root (start = 0). This violates the hoisting constraint where only the hoisted subtree should be visible. It should instead return a stack containing only thehoistedId. - The stack construction joins segments of the
clampedarray but omits the path prefix before the hoisted note (e.g., "root/"). This results in relative paths that may not be correctly resolved bynoteContext.setNoteor other services expecting absolute paths.
Preserving the prefix and correctly handling the non-descendant case ensures consistent navigation behavior.
| let start = parentSegments.indexOf(hoistedId); | |
| if (start < 0) start = 0; | |
| const clamped = parentSegments.slice(start); | |
| if (clamped.length === 0) { | |
| return [hoistedId]; | |
| } | |
| const stack: string[] = []; | |
| for (let i = 0; i < clamped.length; i++) { | |
| stack.push(clamped.slice(0, i + 1).join("/")); | |
| } | |
| let start = parentSegments.indexOf(hoistedId); | |
| if (start < 0) { | |
| return [hoistedId]; | |
| } | |
| const clamped = parentSegments.slice(start); | |
| const prefix = parentSegments.slice(0, start).join("/"); | |
| const stack: string[] = []; | |
| for (let i = 0; i < clamped.length; i++) { | |
| const path = clamped.slice(0, i + 1).join("/"); | |
| stack.push(prefix ? `${prefix}/${path}` : path); | |
| } |
| selectAllNotesInParentCommand({ node }: CommandListenerData<"selectAllNotesInParent">) { | ||
| if (!node) return; | ||
| for (const child of node.getParent().getChildren()) { | ||
| child.setSelected(true); | ||
| } | ||
| } |
There was a problem hiding this comment.
selectAllNotesInParentCommand should verify that node.getParent() returns a valid parent node before attempting to call getChildren(). While node is guaranteed to be present if the command is triggered, getParent() could theoretically return null in certain tree states or if called on a virtual root, leading to a runtime error.
| selectAllNotesInParentCommand({ node }: CommandListenerData<"selectAllNotesInParent">) { | |
| if (!node) return; | |
| for (const child of node.getParent().getChildren()) { | |
| child.setSelected(true); | |
| } | |
| } | |
| selectAllNotesInParentCommand({ node }: CommandListenerData<"selectAllNotesInParent">) { | |
| const parent = node?.getParent(); | |
| if (!parent) return; | |
| for (const child of parent.getChildren()) { | |
| child.setSelected(true); | |
| } | |
| } |
No description provided.