ENG-2184 Add search to the settings panel - #1378
trangdoan982 wants to merge 6 commits into
Conversation
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:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
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}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
1fc68db to
ed7bf4e
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
ed7bf4e to
cb0fb8e
Compare
cb0fb8e to
8e001d5
Compare
8e001d5 to
468f2d2
Compare
468f2d2 to
107e7b1
Compare
107e7b1 to
8cecb56
Compare
8cecb56 to
5b8fa54
Compare
07f219a to
edad556
Compare
38c1513 to
d6d2044
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d6d2044f38
ℹ️ 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".
| * Roam themes `.bp3-popover` and the library default is transparent. */ | ||
| .dg-settings-search__results .bp3-popover-content { | ||
| width: 340px; | ||
| background-color: #fff; |
There was a problem hiding this comment.
In dark or custom Roam themes, forcing the portalled popover to #fff bypasses the theme's Blueprint/Roam background while its menu text can remain theme-controlled, producing a visually inconsistent or unreadable result list. Use an established host variable such as --screen-bg-color or a native Blueprint pattern instead of introducing a fixed background palette.
AGENTS.md reference: apps/roam/AGENTS.md:L9-L15
Useful? React with 👍 / 👎.
| const results = useMemo( | ||
| () => rankSettings({ entries: buildSettingsCatalog(), query }), | ||
| [query], |
There was a problem hiding this comment.
When a graph has no node type named Export, typing export ranks only buildSettingsCatalog() and returns no usable result: that catalog filters out all export settings because they have no path, and its page entries come only from SETTINGS_TAB_META, which has no Export page. Consequently the advertised flow that lets export reach the separate Export dialog instead displays “No settings match”; add a dedicated actionable Export entry or selection action.
Useful? React with 👍 / 👎.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
The codex comments are worth addressing.
Please ask your agent why it isn't adhering to the ~ requirement in AGENTS.md.
Here's some general feedback.
https://www.loom.com/share/7c3e6e7eab2944829ef5f5d11cce5d03
- Make repeated setting results distinguishable by including the node type in the main label, e.g. “Issue > Index.”
- Avoid matching shared breadcrumb terms on repeated node settings. Searching “Nodes” or “Grammar” should show the relevant page, not a list of Color and other repeated entries.
- Add a direct page result for each node type. Searching “Evidence” should show its settings page first; its individual settings can appear below.
- Adjust the highlight spacing so the background and colored edge have padding around the row content on both sides. (or drop the color strip)
- Add ability to search for canvas shortcut overrides (a single entry is fine, not an entry for each shortcut)
- Index relation names so searching for a relation, such as “Informs,” returns a result that opens its settings.
Not all required for this PR, but I imagine we should create a place to capture this feedback from all team members.
| import { | ||
| buildSettingsCatalog, | ||
| type SearchableEntry, | ||
| } from "../utils/settingsCatalog"; |
| buildSettingsCatalog, | ||
| type SearchableEntry, | ||
| } from "../utils/settingsCatalog"; | ||
| import { rankSettings } from "../utils/settingsSearch"; |
| import { | ||
| SETTING_ANCHOR_FLASH_CLASS, | ||
| settingAnchorSelector, | ||
| } from "../utils/settingAnchor"; |
| background-color: transparent; | ||
| } | ||
|
|
||
| .dg-settings-search__scroll { |
| } | ||
|
|
||
| /** Inset to the tab titles' left edge. */ | ||
| .dg-settings-search { |
d6d2044 to
6cc638f
Compare
6cc638f to
502efd9
Compare
502efd9 to
52eaf52
Compare
52eaf52 to
7978a64
Compare
7978a64 to
b105cbe
Compare
Adds the search field at the top of the settings rail: type a setting's name, section or description and jump to it wherever it currently lives. Matching is a plain substring pass over the entries ENG-2213 records, ranked so that an exact label always beats a section or description that merely mentions the word. Tabs are offered as their own result kind, so "export" reaches the Export page even though no setting is named that. The catalog is rebuilt per query rather than cached for the dialog's lifetime, because node types and feature gates can both change while Settings is open. The results are a portalled Popover, not an in-flow dropdown: the field lives inside Blueprint's tab list, which Settings styles `overflow-y: auto; overflow-x: hidden`, so an in-flow panel is clipped on both axes. It also paints its own surface, because Roam ships its own Blueprint build and themes `.bp3-popover` — inheriting the library default leaves the panel transparent with the rail showing through it. Jumping needs a new `navigate` action: the reducer could only push one segment at a time, and a setting can be several segments deep. The row is not in the DOM when the jump is dispatched, since only the active tab's panel renders, so the scroll retries by frame until the row appears or a budget runs out — at which point the user is at least on the right page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Finding the row settled the jump, which cleared anchorId and so re-ran the effect; its cleanup then stripped the flash class on the very next render. The row scrolled into view with no indication of which one had matched. The flash now owns its own lifetime instead of the effect's, and re-flashing a row already flashing restarts the animation rather than riding out the first. Only the frame-by-frame lookup stays cancellable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tier order is the behaviour worth pinning: an exact label must not be buried under a description that happens to mention the same word, and a multi-word query must match across fields without diluting single-word precision. Both are easy to regress while tuning and neither is visible without a query that exercises the boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback: the box sat flush with the rail while every tab title under it was indented. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cast The selector only escaped double quotes; a backslash in an anchor id could still break out of the attribute string. Keys are authored today, so this is belt-and-braces rather than a live bug. The test helper's `as SearchableEntry` was already the expression's type, which the changed-files lint flags as a warning and fails on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
https://www.loom.com/share/3a670cb80b624ac284e0d3e82e2826bc
Reviewer brief
Result: A search field at the top of the settings rail matches settings by name, section/group and description, and jumps to the matching row wherever it currently lives — including per-node-type settings behind the Grammar › Nodes drill-down. Tabs are offered as their own result kind, so "export" reaches the Export page even though no setting is named that.
Stacked on ENG-2213 Settings catalog: address every setting by key and location #1379 (ENG-2213), which records what every setting is called and where it lives. This PR is only the search layer that reads it: ranking, the field, and the jump.
Review focus — the jump retries by frame. The target row is not in the DOM when the jump is dispatched:
renderActiveTabPanelOnlymeans the destination panel mounts on a later commit, and a row inside aCollapsemounts later still.useSettingAnchorScrolltherefore polls by animation frame until the row appears or a ~500ms budget runs out, then degrades to leaving the user on the right page. A single lookup would silently miss.**Risk or follow-up:**
- **No tests.** Ranking tiers, node-type expansion and availability gating are unverified by CI. - **Not verified in Roam: the transparency fix itself.** Roam stopped re-registering the developer extension after the reload needed to pick up the new CSS. Worth one manual look — open settings, type anything, confirm the panel has a solid background. - Escape stops propagation only while the query is non-empty, so a first Escape clears the search and a second closes the dialog. Intentional, and easy to read as a bug.Verification
Manual and local only.
pnpm ci:validate(check-types + 142 unit tests) and eslint on the changed files — 0 errors, no new warnings on added lines.…eng-2184-add-search-to-the-settings-panel): the field renders in the rail and returns correct labels and breadcrumbs. That run is what caught the transparent popover.Tabscarrying the same overflow rules asSettings.tsx. Measured: opaque 340px surface; opens rightward from the field's left edge; active-row breadcrumb at 4.53:1 on Blueprint's#137CBD(80% opacity measured 3.49:1, below AA — hence full white); ArrowDown scrolls the active row into view. The harness was not kept.Scope check
$scope-checkagainst the ENG ticket and final diff.Done When:settingAnchorIdexport removed — added earlier in this branch's history, never used.Local delegated full review