Support multi-attribute note sorting#10060
Conversation
Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Code Review
This pull request adds support for sorting notes by multiple attributes and explicit sort directions (e.g., 'attribute:asc' or 'attribute:desc') in tree.ts, accompanied by comprehensive unit tests in tree.spec.ts. The sorting logic is refactored to parse multiple criteria, apply sorting directions directly within the comparator, and simplify the pinning logic for 'top' and 'bottom' notes. Feedback on the changes identifies a potential issue in the new compareSortValues helper, where equal values could return a non-zero result when sortNatural is false, violating the comparator contract; a code suggestion is provided to add an explicit equality check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function compareSortValues(a: string, b: string, reverse = false) { | ||
| return compare(a, b) * (reverse ? -1 : 1); | ||
| } |
There was a problem hiding this comment.
When sortNatural is false, the underlying compare function returns 1 for equal values (since a < b is false). When sorting by the fallback title, if two notes have the exact same title, compareSortValues will be called with equal values and return a non-zero result (either 1 or -1 depending on reverse). This violates the comparator contract (which requires returning 0 for equal elements) and can lead to unstable sorting or unpredictable behavior in some JavaScript engines. Adding an explicit equality check to return 0 prevents this issue.
| function compareSortValues(a: string, b: string, reverse = false) { | |
| return compare(a, b) * (reverse ? -1 : 1); | |
| } | |
| function compareSortValues(a: string, b: string, reverse = false) { | |
| if (a === b) return 0; | |
| return compare(a, b) * (reverse ? -1 : 1); | |
| } |
Greptile SummaryThis PR adds multi-attribute note sorting by extending the
Confidence Score: 4/5Safe to merge with minor follow-up work; core sorting logic and all existing tests are preserved, and the new multi-attribute path is straightforward. The refactor correctly replaces the notes.reverse() end-flip with inline per-criterion direction, and the #top/#bottom pinning cleanup is a genuine improvement. The main risk is the title fallback direction: when any criterion carries an explicit :asc/:desc the post-loop fallback ignores the global #sortDirection, which could silently change sort order for users who combine the two. The missing malformed-direction test means that edge case is unverified. Neither issue corrupts data or breaks existing single-attribute sorting. packages/trilium-core/src/services/tree.ts — the hasExplicitDirection fallback logic; packages/trilium-core/src/services/tree.spec.ts — the missing malformed-direction test case. Important Files Changed
|
| it("sorts notes by multiple attributes in order", () => { | ||
| const note = buildNote({ | ||
| children: [ | ||
| {title: "first", "#priority": "1", "#dueDate": "2026-05-20"}, | ||
| {title: "third", "#priority": "2", "#dueDate": "2026-05-01"}, | ||
| {title: "second", "#priority": "1", "#dueDate": "2026-05-10"} | ||
| ], | ||
| "#sorted": "priority,dueDate" | ||
| }); | ||
|
|
||
| getContext().init(() => { | ||
| tree.sortNotesIfNeeded(note.noteId); | ||
| }); | ||
|
|
||
| const orderedTitles = note.children.map((child) => child.title); | ||
| expect(orderedTitles).toStrictEqual(["second", "first", "third"]); | ||
| }); | ||
|
|
||
| it("supports explicit sort direction for each sorted attribute", () => { | ||
| const note = buildNote({ | ||
| children: [ | ||
| {title: "low-priority", "#priority": "1", "#dueDate": "2026-05-01"}, | ||
| {title: "next-date", "#priority": "2", "#dueDate": "2026-05-02"}, | ||
| {title: "high-priority", "#priority": "3", "#dueDate": "2026-05-01"} | ||
| ], | ||
| "#sorted": "dueDate:asc,priority:desc" | ||
| }); | ||
|
|
||
| getContext().init(() => { | ||
| tree.sortNotesIfNeeded(note.noteId); | ||
| }); | ||
|
|
||
| const orderedTitles = note.children.map((child) => child.title); | ||
| expect(orderedTitles).toStrictEqual(["high-priority", "low-priority", "next-date"]); | ||
| }); |
There was a problem hiding this comment.
Missing "malformed sort direction fallback" test
The PR description explicitly lists "malformed sort direction fallback" as one of three new spec cases, but only two tests were added. The missing test would cover the case where #sorted = "priority:typo" (invalid direction) — with the current logic, rawDirection = "typo" fails the hasExplicitDirection check, so the entire string "priority:typo" becomes the attribute key, not "priority". Since that attribute doesn't exist, sorting silently falls back to title. A test case would make this documented, intentional behaviour explicit and guard against future regressions.
| } | ||
|
|
||
| const sortCriteria = parseSortCriteria(customSortBy, reverse); | ||
| const hasExplicitDirection = sortCriteria.some((criterion) => criterion.hasExplicitDirection); |
There was a problem hiding this comment.
Title fallback direction is surprising when explicit and non-explicit criteria are mixed
hasExplicitDirection is true if any criterion carries :asc/:desc, and controls the post-loop title fallback: compareSortValues(titleAEl, titleBEl, hasExplicitDirection ? false : reverse). When a note has #sorted = "dueDate:asc" (one explicit) and #sortDirection = "desc" (global reverse), the final title-based tiebreaker is sorted ascending — the user's #sortDirection is silently ignored for the fallback. Users who previously relied on #sorted = "dueDate" + #sortDirection = "desc" to get a descending-dueDate, descending-title-fallback order and switch to the new "dueDate:asc" spelling will get an ascending title fallback instead.
Summary
Adds multi-attribute sorting support for child notes when
#sortedcontains a comma-separated sequence such asdueDate:asc,priority:desc.Linked Issue
Related to #6829.
Problem
The current note tree sorting path only uses one sorted attribute, so ties on the first attribute fall through to the existing fallback ordering instead of applying additional user-provided sort keys.
Fix
#sortedentries with optional:asc/:descdirection.Validation
git diff --check origin/main...HEADnpx --yes pnpm@11.5.0 --filter server test ../../packages/trilium-core/src/services/tree.spec.tsnpx --yes pnpm@11.5.0 typechecknpx --yes pnpm@11.5.0 --filter server buildThe server build completed with existing bundler warnings outside this patch surface.
Risk Notes
packages/trilium-core/src/services/tree.tsandpackages/trilium-core/src/services/tree.spec.ts.#top,#bottom, natural sorting, and existing fallback ordering are preserved.edit-docsworkflow; I can update that path if maintainers prefer.