Skip to content

ENG-2184 Add search to the settings panel - #1378

Open
trangdoan982 wants to merge 5 commits into
eng-2213-settings-catalog-address-every-setting-by-key-and-locationfrom
eng-2184-add-search-to-the-settings-panel
Open

ENG-2184 Add search to the settings panel#1378
trangdoan982 wants to merge 5 commits into
eng-2213-settings-catalog-address-every-setting-by-key-and-locationfrom
eng-2184-add-search-to-the-settings-panel

Conversation

@trangdoan982

@trangdoan982 trangdoan982 commented Aug 31, 2026

Copy link
Copy Markdown
Member

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 — two Blueprint behaviours the code works around, both load-bearing.

    • The results are a portalled Popover rather than an in-flow dropdown, because the field renders inside Blueprint's tab list, which Settings.tsx styles overflow-y: auto; overflow-x: hidden — an in-flow panel is clipped on both axes.
    • The popover paints its own background, radius and shadow instead of inheriting Blueprint's. Roam ships its own Blueprint build and themes .bp3-popover; inheriting the default renders the panel fully transparent with the rail showing through it. This was only visible in Roam itself, not in isolation.
    • flip and preventOverflow are disabled deliberately: the rail sits at the window edge, so Blueprint's overflow handling pushed the panel off the left of the dialog.
  • Review focus — the jump retries by frame. The target row is not in the DOM when the jump is dispatched: renderActiveTabPanelOnly means the destination panel mounts on a later commit, and a row inside a Collapse mounts later still. useSettingAnchorScroll therefore 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.
  • Driven in a real Roam graph on this build (version stamp …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.
  • Position, contrast and keyboard scrolling were measured in a throwaway browser harness that mounted the real component inside a real vertical Blueprint Tabs carrying the same overflow rules as Settings.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

  • Ran $scope-check against the ENG ticket and final diff.
  • Scope beyond Done When:
    • Description matching. The ticket scoped this out; requested during implementation. The text lives on the ENG-2213 entry, so search and the row read the same string.
    • Dead settingAnchorId export removed — added earlier in this branch's history, never used.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context.

@linear-code

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown

ENG-2184

@graphite-app

graphite-app Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

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:

  • What single problem this PR solves
  • Why the files/changes are coupled

@vercel

vercel Bot commented Aug 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 5, 2026 11:12pm UTC

Request Review

@supabase

supabase Bot commented Aug 31, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment thread apps/roam/src/components/settings/utils/settingsCatalog.ts Outdated
Comment thread apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts Outdated
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from 1fc68db to ed7bf4e Compare August 31, 2026 19:48
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from ed7bf4e to cb0fb8e Compare August 31, 2026 19:55
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from cb0fb8e to 8e001d5 Compare August 31, 2026 19:58
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from 8e001d5 to 468f2d2 Compare August 31, 2026 21:02
@trangdoan982
trangdoan982 changed the base branch from eng-2183-build-the-new-setting-item-row-component to eng-2213-settings-catalog-address-every-setting-by-key-and-location August 31, 2026 21:02
Comment thread apps/roam/src/components/settings/utils/settingAnchor.ts Outdated
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from bc865f3 to 81f750e Compare August 31, 2026 23:08
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from 468f2d2 to 107e7b1 Compare August 31, 2026 23:09
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 81f750e to fd2b364 Compare September 3, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from 107e7b1 to 8cecb56 Compare September 3, 2026 20:30
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from fd2b364 to 8f6c3bd Compare September 4, 2026 21:57
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from 8cecb56 to 5b8fa54 Compare September 4, 2026 21:57
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 8f6c3bd to 0a82e0e Compare September 4, 2026 22:46
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from 07f219a to edad556 Compare September 4, 2026 22:46
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from edad556 to efd92c7 Compare September 5, 2026 22:55
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 0a82e0e to 2867339 Compare September 5, 2026 22:55
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from efd92c7 to ef6ab91 Compare September 5, 2026 22:59
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 2867339 to 369c7f8 Compare September 5, 2026 22:59
trangdoan982 and others added 5 commits September 5, 2026 19:12
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>
@trangdoan982
trangdoan982 force-pushed the eng-2213-settings-catalog-address-every-setting-by-key-and-location branch from 369c7f8 to c3c5884 Compare September 5, 2026 23:12
@trangdoan982
trangdoan982 force-pushed the eng-2184-add-search-to-the-settings-panel branch from ef6ab91 to 9be3ff8 Compare September 5, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant