ENG-2112 Add sort button with sort dropdown - #1320
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
trangdoan982
added a commit
that referenced
this pull request
Aug 20, 2026
Escape cannot be intercepted from the DOM. Obsidian registers the Modal's close-on-Escape before any plugin React tree exists, so a listener added later always runs second: preventDefault plus stopImmediatePropagation in a React handler does not work, nor does a capture-phase window listener, nor registering on the Modal's own scope, since Scope resolves in registration order. The previous DOM attempt here closed the whole modal instead of the panel. Pushing a Scope while the panel is open lands above the modal in the stack, so the panel gets Escape first. The dead DOM handlers in both the panel and the modal are gone with it. Diagnosis credit to the SearchDropdown work on ENG-2112 (#1320), which fixes the same bug for the sort dropdown. Migrating this component onto that shared shell would drop ~120 duplicated lines and is best done once both land, since SearchDropdown does not exist on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trangdoan982
force-pushed
the
eng-2112-add-sort-button-with-sort-dropdown
branch
from
August 20, 2026 16:49
5d023c7 to
a255058
Compare
trangdoan982
changed the base branch from
eng-2109-create-node-search-modal-with-ranked-results-and-preview
to
eng-2111-add-keyboard-only-node-type-filtering-with-tag-chips
August 20, 2026 16:49
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
trangdoan982
force-pushed
the
eng-2112-add-sort-button-with-sort-dropdown
branch
from
August 21, 2026 21:51
3cd4dca to
72e8f4b
Compare
trangdoan982
added a commit
that referenced
this pull request
Aug 22, 2026
Escape cannot be intercepted from the DOM. Obsidian registers the Modal's close-on-Escape before any plugin React tree exists, so a listener added later always runs second: preventDefault plus stopImmediatePropagation in a React handler does not work, nor does a capture-phase window listener, nor registering on the Modal's own scope, since Scope resolves in registration order. The previous DOM attempt here closed the whole modal instead of the panel. Pushing a Scope while the panel is open lands above the modal in the stack, so the panel gets Escape first. The dead DOM handlers in both the panel and the modal are gone with it. Diagnosis credit to the SearchDropdown work on ENG-2112 (#1320), which fixes the same bug for the sort dropdown. Migrating this component onto that shared shell would drop ~120 duplicated lines and is best done once both land, since SearchDropdown does not exist on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trangdoan982
force-pushed
the
eng-2112-add-sort-button-with-sort-dropdown
branch
from
August 22, 2026 04:27
19ed572 to
5f21839
Compare
trangdoan982
force-pushed
the
eng-2112-add-sort-button-with-sort-dropdown
branch
from
August 22, 2026 04:34
5f21839 to
a4c0e6b
Compare
trangdoan982
added a commit
that referenced
this pull request
Aug 22, 2026
* ENG-2110 Add node type filter dropdown menu Adds a type filter beside the node search input. Roam's advanced search is the design and behaviour reference; its pure filter semantics are ported, its BlueprintJS structure is not. The search seam already supported this — rankDiscourseNodesByTitle takes nodeTypeIds and filters before scoring — so this is UI and state only. - discourseNodeTypeFilter.ts ports Roam's semantics, including the canonicalisation that makes "none selected" and "all selected" both mean no filter, matching filterCandidatesByNodeTypeIds. - NodeTypeFilterMenu renders an Obsidian-native trigger (clickable-icon + setIcon) with a count badge, over a panel with checkbox rows, colour dots, per-row Only, Select all with indeterminate state, and a type search past 7 types. - selectedNodeTypeIds lives in NodeSearch as the single source of truth so ENG-2111's chips can share it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ENG-2110 Address review: contain panel keystrokes, swap Select all for Clear filter Panel keystrokes no longer reach the modal's key handler. Only Escape was stopped before, so Enter typed in the type search ran the modal's "open the highlighted result" branch — closing the modal and opening an unrelated note — and the arrows moved the result selection. Replaces the "Select all" checkbox with a "Clear filter (n)" button shown only while a filter is active. Because an empty selection and a full one are the same state, the checkbox sat checked and inert whenever nothing was filtered, so clicking it appeared to do nothing. Clearing is the control's only real function, so it now says that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ENG-2110 Drop the unused select-all check state helper getSelectAllCheckState and SelectAllCheckState lost their only consumer when the Select all checkbox became the Clear filter button. Roam keeps its own copy, which its tri-state checkbox still uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ENG-2110 Close the filter panel on Escape via a pushed keymap scope Escape cannot be intercepted from the DOM. Obsidian registers the Modal's close-on-Escape before any plugin React tree exists, so a listener added later always runs second: preventDefault plus stopImmediatePropagation in a React handler does not work, nor does a capture-phase window listener, nor registering on the Modal's own scope, since Scope resolves in registration order. The previous DOM attempt here closed the whole modal instead of the panel. Pushing a Scope while the panel is open lands above the modal in the stack, so the panel gets Escape first. The dead DOM handlers in both the panel and the modal are gone with it. Diagnosis credit to the SearchDropdown work on ENG-2112 (#1320), which fixes the same bug for the sort dropdown. Migrating this component onto that shared shell would drop ~120 duplicated lines and is best done once both land, since SearchDropdown does not exist on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ENG-2110 Address review: tighten filter comments Names ENG-2111 instead of the internal "F6" shorthand, notes the search threshold is tuned for the desktop-only modal, and drops the vague "stored empty set" phrasing. One line each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Base automatically changed from
eng-2111-add-keyboard-only-node-type-filtering-with-tag-chips
to
main
August 22, 2026 04:51
trangdoan982
force-pushed
the
eng-2112-add-sort-button-with-sort-dropdown
branch
from
August 23, 2026 19:38
a4c0e6b to
1921db7
Compare
Member
|
@trangdoan982 I don't see a loom video for this. Did you test it? |
Member
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1921db783a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
mdroidian
approved these changes
Aug 23, 2026
mdroidian
left a comment
mdroidian
left a comment
Member
There was a problem hiding this comment.
Approved, assuming you have tested this yourself.
Let's make sure to add Loom videos showing/describing the change.
Also, be sure to double check/test the codex P2's.
1 task
trangdoan982
added a commit
that referenced
this pull request
Aug 24, 2026
* ENG-2168 Document Obsidian node search Add a core-features page for the advanced node search feature, covering the whole beta rather than only what has merged: ranked search with preview and badges, the type filter dropdown, keyboard tag chips, sort, and the footer actions including insert-link-at-cursor. Sort (ENG-2112, #1320) and insert link at cursor (ENG-2114, #1314) are still open, so this page should land with or after them. Screenshots are of the running plugin, captured against the combined advanced-search-beta build so the sort and insert-link surfaces were present. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * trim down instructions --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Five client-side sort dimensions over the search result list — relevance, alphabetical, date created, date modified, author — with Asc/Desc controls, mirroring Roam's sort menu (ENG-1732). Sorting runs over the full ranked list and the 50-row truncation comes after it; sorting the truncated window would have shown the alphabetically-first 50 of the best-matching 50. Author resolution moves out of NodeSearchModal into discourseNodeAuthor with its behaviour unchanged, and unattributed notes are pinned last in both directions. The trigger-plus-panel shell lives in SearchDropdown rather than inside the sort menu, and NodeTypeFilterMenu now renders through it — dropping its duplicate trigger, badge, Escape scope, outside-click, keystroke containment and panel container, along with the hardcoded shadow that container carried. isTypeFilterOpen becomes a shared openDropdown, so the two panels can no longer be open at once. Escape reaches the panel only through a pushed keymap scope: Obsidian registers the Modal's close-on-Escape before any plugin React tree exists, so a DOM listener added later always runs second, and registering on the Modal's own scope loses to the built-in handler. Rebased from the ENG-2109/2110/2111 stack onto main after all three merged. The eight commits from that stack's lifetime are collapsed into one, because replaying them over the squash-merged base meant re-resolving the same NodeSearchModal regions eight times; the resolved tree is byte-identical to the pre-rebase branch except for one comment main revised on its own. Verified over CDP: 30/30. Two assertions skipped for want of data in the available vault — full-list-vs-truncated-window needs over 50 matches, and unattributed-last needs a note whose authorId does not resolve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The option rows and direction controls are divs, so Enter and Space needed wiring up and the rows needed tabIndex to be reachable at all. Raised in review; the filter panel got this for free from its native checkboxes. Also drops the empty-before-setIcon in SearchDropdown. The claim it rested on — that `setIcon` appends — does not hold on Obsidian 1.13.7: driving the panel through ten direction changes and five option switches leaves exactly one svg in every icon container, with or without the empty. All three call sites now use the same one-liner as the rest of the app. 35/35, with three assertions added: every control carries tabIndex, Enter on a focused option applies that sort, and Space on a focused direction control applies it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trangdoan982
force-pushed
the
eng-2112-add-sort-button-with-sort-dropdown
branch
from
August 24, 2026 15:21
28d4e3c to
17eb56b
Compare
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
2 participants
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.
Closes ENG-2112. Was stacked on #1321 → #1315 → #1285; all three have merged, so this now targets
maindirectly.Five client-side sort dimensions for the node search modal — relevance, alphabetical, date created, date modified, author — with Asc/Desc controls, mirroring Roam's sort menu (ENG-1732).
Changes
utils/discourseNodeSort.tscomponents/NodeSortMenu.tsxcomponents/SearchDropdown.tsxutils/discourseNodeAuthor.tsNodeSearchModalunchangedcomponents/NodeTypeFilterMenu.tsxcomponents/NodeSearchModal.tsxThree things worth a reviewer's attention
Sorting precedes truncation. The modal sliced to
MAX_VISIBLE_RESULTSbefore anything else, so sorting after that slice would have reordered the best-matching 50 rather than all matches. Now: rank (+ type filter) → sort → slice.Escape needs a pushed keymap scope. Obsidian registers the Modal's close-on-Escape before any plugin React tree exists, so anything added later runs second —
stopImmediatePropagationin a React handler, a capture-phasewindowlistener, and registering on the Modal's ownscopeall fail (the last becauseScoperesolves in registration order).Keymap.pushScopelands above the modal. #1315 adopted this from the diagnosis here; with the migration it now lives in one place.The filter migration is the deduplication. The sort panel needs behaviour identical to the type filter's, so the choice was one shared shell or a second copy.
NodeTypeFilterMenunow renders throughSearchDropdown, dropping its duplicate trigger, badge, Escape scope, outside-click, keystroke containment and panel container. Two fixes fall out: its panel had a hardcodedshadow-[0_4px_12px_rgba(0,0,0,0.15)]and now uses--shadow-s, andisTypeFilterOpenbecame a sharedopenDropdown, so both panels can no longer be open at once.Verification
apps/obsidianhas no test runner, so verified by driving the real app over CDP — 30/30 assertions: all five dimensions and both directions, per-dimension direction defaults, Escape sparing the modal, styling (inactive rows match the panel background, shadow matches--shadow-s), the trigger holding one icon across direction changes, mutual exclusion of the two panels, and all five options honouring an active type filter (19 rows, none off-type).Two assertions were skipped for want of data in the vault available at the time — sort-over-full-list-vs-truncated-window needs more than 50 matches, and unattributed-last needs a note whose
authorIddoes not resolve. Both passed on a larger vault before the rebase.check-typesandlintclean.Tailwind note:
shadow-[var(--shadow-s)]is silently inertThe value is ambiguous, so Tailwind resolves it as a shadow colour and emits
--tw-shadow-colorwith nobox-shadow. It needs the type hint,shadow-[shadow:var(--shadow-s)].ModifyNodeModal.tsx:500has the same broken usage on main — tracked separately, not touched here.Scope check
$scope-check— skill unavailable in this worktree; scope assessed by hand againstDone When.Done When: the sort-before-truncate fix (four of five options are wrong without it); the shared shell and filter migration (see above); Escape via a pushed scope; the singleopenDropdownstate. Size justification in this comment.🤖 Generated with Claude Code