From a860741e3d800fa54c36e91d8ed2c0c3348bcd93 Mon Sep 17 00:00:00 2001 From: Trang Doan Date: Mon, 31 Aug 2026 15:40:35 -0400 Subject: [PATCH 1/6] ENG-2184 Add search to the settings panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../roam/src/components/settings/Settings.tsx | 16 ++ .../navigation/SettingsNavContext.tsx | 3 + .../navigation/SettingsSearchField.tsx | 180 ++++++++++++++++++ .../navigation/useSettingAnchorScroll.ts | 57 ++++++ .../settings/utils/settingAnchor.ts | 6 + .../settings/utils/settingsNavigation.ts | 10 + .../settings/utils/settingsSearch.ts | 94 +++++++++ apps/roam/src/styles/settingsStyles.css | 45 ++++- 8 files changed, 410 insertions(+), 1 deletion(-) create mode 100644 apps/roam/src/components/settings/navigation/SettingsSearchField.tsx create mode 100644 apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts create mode 100644 apps/roam/src/components/settings/utils/settingsSearch.ts diff --git a/apps/roam/src/components/settings/Settings.tsx b/apps/roam/src/components/settings/Settings.tsx index 5ec8a023d..acbf235e8 100644 --- a/apps/roam/src/components/settings/Settings.tsx +++ b/apps/roam/src/components/settings/Settings.tsx @@ -40,6 +40,9 @@ import { tabIdOf, } from "./utils/settingsNavigation"; import { SettingsNavProvider } from "./navigation/SettingsNavContext"; +import SettingsSearchField from "./navigation/SettingsSearchField"; +import { useSettingAnchorScroll } from "./navigation/useSettingAnchorScroll"; +import type { SearchableEntry } from "./utils/settingsCatalog"; import GrammarNodesRoute from "./GrammarNodesRoute"; const SectionHeader = ({ children }: { children: React.ReactNode }) => ( @@ -95,6 +98,18 @@ export const SettingsDialog = ({ (tabId: string) => dispatch({ type: "select-tab", tabId }), [], ); + // Cleared once the row is found or the lookup gives up, so a repeat jump to the + // same row still scrolls. + const [pendingAnchorId, setPendingAnchorId] = useState(null); + const handleSearchSelect = useCallback((entry: SearchableEntry) => { + dispatch({ type: "navigate", path: entry.path }); + setPendingAnchorId(entry.kind === "setting" ? entry.anchorId : null); + }, []); + const clearPendingAnchor = useCallback(() => setPendingAnchorId(null), []); + useSettingAnchorScroll({ + anchorId: pendingAnchorId, + onSettled: clearPendingAnchor, + }); // eslint-disable-next-line react-hooks/exhaustive-deps const settings = useMemo(() => bulkReadSettings(), [activeTabId]); const [leftSidebarEnabled, setLeftSidebarEnabled] = useState( @@ -185,6 +200,7 @@ export const SettingsDialog = ({ vertical={true} renderActiveTabPanelOnly={true} > + Preferences void; pop: () => void; goToDepth: (depth: number) => void; + /** Jumps straight to a full route; `push` only moves one segment at a time. */ + navigate: (path: SettingsPath) => void; }; const SettingsNavContext = createContext(null); @@ -34,6 +36,7 @@ export const SettingsNavProvider = ({ push: (segment) => dispatch({ type: "push", segment }), pop: () => dispatch({ type: "pop" }), goToDepth: (depth) => dispatch({ type: "truncate", depth }), + navigate: (target) => dispatch({ type: "navigate", path: target }), }), [path, dispatch], ); diff --git a/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx b/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx new file mode 100644 index 000000000..bb94d2480 --- /dev/null +++ b/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx @@ -0,0 +1,180 @@ +import React, { useEffect, useMemo, useRef, useState } from "react"; +import { + Icon, + InputGroup, + Menu, + MenuItem, + Popover, + Position, +} from "@blueprintjs/core"; +import { + buildSettingsCatalog, + type SearchableEntry, +} from "../utils/settingsCatalog"; +import { rankSettings } from "../utils/settingsSearch"; + +const SettingsSearchResult = ({ + entry, + isActive, + onSelect, +}: { + entry: SearchableEntry; + isActive: boolean; + onSelect: (entry: SearchableEntry) => void; +}): JSX.Element => ( + + {entry.label} + {/* Undimmed on purpose: white on the active row's `#137CBD` is 4.5:1, and any + opacity below 100% drops under AA for text this size (80% measures 3.5:1). */} + + {entry.breadcrumb} + + + } + // Selecting on mousedown so the choice lands before the input's blur closes + // the list out from under the pointer. + onMouseDown={(event: React.MouseEvent) => { + event.preventDefault(); + onSelect(entry); + }} + /> +); + +/** Results are a portalled Popover because the tab list this field sits in is styled + * `overflow-y: auto; overflow-x: hidden`, which clips an in-flow dropdown on both axes. */ +const SettingsSearchField = ({ + onSelect, +}: { + onSelect: (entry: SearchableEntry) => void; +}): JSX.Element => { + const [query, setQuery] = useState(""); + const [activeIndex, setActiveIndex] = useState(0); + const [isOpen, setIsOpen] = useState(false); + const inputRef = useRef(null); + const scrollContainerRef = useRef(null); + + const results = useMemo( + () => rankSettings({ entries: buildSettingsCatalog(), query }), + [query], + ); + const isShowingResults = isOpen && query.trim() !== ""; + + // Keeps the keyboard-selected row visible when the list scrolls, following + // the same approach as DiscourseNodeSearchMenu. + useEffect(() => { + const container = scrollContainerRef.current; + if (!container) return; + const activeItem = container.querySelector( + '[data-active="true"]', + ); + if (!activeItem) return; + const containerRect = container.getBoundingClientRect(); + const itemRect = activeItem.getBoundingClientRect(); + if ( + itemRect.bottom > containerRect.bottom || + itemRect.top < containerRect.top + ) { + activeItem.scrollIntoView({ block: "nearest", behavior: "auto" }); + } + }, [activeIndex, results]); + + const select = (entry: SearchableEntry) => { + onSelect(entry); + setQuery(""); + setIsOpen(false); + inputRef.current?.blur(); + }; + + const handleKeyDown = (event: React.KeyboardEvent) => { + if (event.key === "Escape") { + // Stopping propagation so Escape clears the search rather than closing + // the whole Settings dialog out from under a half-typed query. + if (query !== "") event.stopPropagation(); + setQuery(""); + setIsOpen(false); + return; + } + if (!isShowingResults || results.length === 0) return; + if (event.key === "ArrowDown") { + event.preventDefault(); + setActiveIndex((index) => (index + 1) % results.length); + } else if (event.key === "ArrowUp") { + event.preventDefault(); + setActiveIndex((index) => (index - 1 + results.length) % results.length); + } else if (event.key === "Enter") { + event.preventDefault(); + const entry = results[activeIndex]; + if (entry) select(entry); + } + }; + + return ( + + + No settings match “{query.trim()}” + + ) : ( +
+ + {results.map((entry, index) => ( + + ))} + +
+ ) + } + > +
+ { + inputRef.current = input; + }} + leftIcon="search" + placeholder="Search settings" + value={query} + onChange={(event: React.ChangeEvent) => { + setQuery(event.target.value); + setActiveIndex(0); + setIsOpen(true); + }} + onFocus={() => setIsOpen(true)} + onBlur={() => setIsOpen(false)} + onKeyDown={handleKeyDown} + /> +
+ + ); +}; + +export default SettingsSearchField; diff --git a/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts b/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts new file mode 100644 index 000000000..972bc7221 --- /dev/null +++ b/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts @@ -0,0 +1,57 @@ +import { useEffect } from "react"; +import { + SETTING_ANCHOR_FLASH_CLASS, + settingAnchorSelector, +} from "../utils/settingAnchor"; + +/** Roughly 500ms at 60fps. */ +const MAX_LOOKUP_FRAMES = 30; +const FLASH_DURATION_MS = 1600; + +/** The row is not in the DOM when the jump is dispatched — only the active panel renders, + * and a `Collapse` mounts later still — so a single lookup misses and this retries. */ +export const useSettingAnchorScroll = ({ + anchorId, + onSettled, +}: { + anchorId: string | null; + onSettled: () => void; +}): void => { + useEffect(() => { + if (!anchorId) return; + let frame = 0; + let rafId = 0; + let flashTimeout: ReturnType | undefined; + let flashed: Element | null = null; + + const settle = () => { + onSettled(); + }; + + const look = () => { + const target = document.querySelector(settingAnchorSelector(anchorId)); + if (target) { + target.scrollIntoView({ block: "center", behavior: "smooth" }); + target.classList.add(SETTING_ANCHOR_FLASH_CLASS); + flashed = target; + flashTimeout = setTimeout(() => { + target.classList.remove(SETTING_ANCHOR_FLASH_CLASS); + }, FLASH_DURATION_MS); + settle(); + return; + } + if (frame++ >= MAX_LOOKUP_FRAMES) { + settle(); + return; + } + rafId = requestAnimationFrame(look); + }; + + rafId = requestAnimationFrame(look); + return () => { + cancelAnimationFrame(rafId); + if (flashTimeout) clearTimeout(flashTimeout); + flashed?.classList.remove(SETTING_ANCHOR_FLASH_CLASS); + }; + }, [anchorId, onSettled]); +}; diff --git a/apps/roam/src/components/settings/utils/settingAnchor.ts b/apps/roam/src/components/settings/utils/settingAnchor.ts index 88757806b..479543024 100644 --- a/apps/roam/src/components/settings/utils/settingAnchor.ts +++ b/apps/roam/src/components/settings/utils/settingAnchor.ts @@ -6,3 +6,9 @@ export const settingAnchor = ( ): Record => ({ [SETTING_ANCHOR_ATTRIBUTE]: settingKeys.join("/"), }); + +/** Setting keys are authored identifiers, so quoting is enough to build a selector. */ +export const settingAnchorSelector = (anchorId: string): string => + `[${SETTING_ANCHOR_ATTRIBUTE}="${anchorId.replace(/"/g, '\\"')}"]`; + +export const SETTING_ANCHOR_FLASH_CLASS = "dg-setting-row--flash"; diff --git a/apps/roam/src/components/settings/utils/settingsNavigation.ts b/apps/roam/src/components/settings/utils/settingsNavigation.ts index bcef6a444..8f945343e 100644 --- a/apps/roam/src/components/settings/utils/settingsNavigation.ts +++ b/apps/roam/src/components/settings/utils/settingsNavigation.ts @@ -10,6 +10,7 @@ export type SettingsPath = readonly string[]; export type SettingsNavAction = | { type: "select-tab"; tabId: string } + | { type: "navigate"; path: SettingsPath } | { type: "push"; segment: string } | { type: "pop" } | { type: "truncate"; depth: number }; @@ -30,6 +31,9 @@ export const depthOf = (path: SettingsPath): number => export const segmentsOf = (path: SettingsPath): readonly string[] => path.slice(1); +export const isSamePath = (a: SettingsPath, b: SettingsPath): boolean => + a.length === b.length && a.every((segment, index) => segment === b[index]); + export const settingsNavReducer = ( state: SettingsPath, action: SettingsNavAction, @@ -39,6 +43,12 @@ export const settingsNavReducer = ( return action.tabId === tabIdOf(state) && state.length === 1 ? state : rootPath(action.tabId); + // Search jumps to a setting several segments deep in one go, which `push` + // cannot express. An empty path is ignored rather than emptying the route. + case "navigate": + return action.path.length === 0 || isSamePath(action.path, state) + ? state + : [...action.path]; case "push": return [...state, action.segment]; case "pop": diff --git a/apps/roam/src/components/settings/utils/settingsSearch.ts b/apps/roam/src/components/settings/utils/settingsSearch.ts new file mode 100644 index 000000000..6b007805c --- /dev/null +++ b/apps/roam/src/components/settings/utils/settingsSearch.ts @@ -0,0 +1,94 @@ +import type { SearchableEntry } from "./settingsCatalog"; + +export const SETTINGS_SEARCH_RESULT_LIMIT = 8; + +/** Lower is better, so an exact label is never buried under a description that + * happens to mention the same word. */ +const MatchTier = { + exactLabel: 0, + labelPrefix: 1, + labelSubstring: 2, + breadcrumb: 3, + keyword: 4, + description: 5, +} as const; + +type Tier = (typeof MatchTier)[keyof typeof MatchTier]; + +const normalize = (value: string): string => + value.toLowerCase().replace(/\s+/g, " ").trim(); + +const haystackOf = (entry: SearchableEntry): string => + normalize( + [ + entry.label, + entry.breadcrumb, + ...entry.keywords, + entry.kind === "setting" ? (entry.description ?? "") : "", + ].join(" "), + ); + +const tierFor = (entry: SearchableEntry, query: string): Tier | null => { + const label = normalize(entry.label); + if (label === query) return MatchTier.exactLabel; + if (label.startsWith(query)) return MatchTier.labelPrefix; + if (label.includes(query)) return MatchTier.labelSubstring; + if (normalize(entry.breadcrumb).includes(query)) return MatchTier.breadcrumb; + if (entry.keywords.some((keyword) => normalize(keyword).includes(query))) + return MatchTier.keyword; + if ( + entry.kind === "setting" && + entry.description && + normalize(entry.description).includes(query) + ) + return MatchTier.description; + return null; +}; + +/** Multi-word queries match when every word appears somewhere, but the tier still comes + * from the whole query, so single-word precision is not diluted. */ +const matches = ( + entry: SearchableEntry, + query: string, +): { tier: Tier } | null => { + const whole = tierFor(entry, query); + if (whole !== null) return { tier: whole }; + + const words = query.split(" ").filter(Boolean); + if (words.length < 2) return null; + const haystack = haystackOf(entry); + return words.every((word) => haystack.includes(word)) + ? { tier: MatchTier.description } + : null; +}; + +/** Settings before pages at the same tier; then alphabetical, for a stable list. */ +const compare = ( + a: { entry: SearchableEntry; tier: Tier }, + b: { entry: SearchableEntry; tier: Tier }, +): number => { + if (a.tier !== b.tier) return a.tier - b.tier; + if (a.entry.kind !== b.entry.kind) return a.entry.kind === "setting" ? -1 : 1; + return a.entry.label.localeCompare(b.entry.label); +}; + +export const rankSettings = ({ + entries, + query, + limit = SETTINGS_SEARCH_RESULT_LIMIT, +}: { + entries: readonly SearchableEntry[]; + query: string; + limit?: number; +}): SearchableEntry[] => { + const normalized = normalize(query); + if (normalized === "") return []; + return entries + .flatMap((entry) => { + const match = matches(entry, normalized); + return match ? [{ entry, tier: match.tier }] : []; + }) + .sort(compare) + .slice(0, limit) + .map(({ entry }) => entry); +}; diff --git a/apps/roam/src/styles/settingsStyles.css b/apps/roam/src/styles/settingsStyles.css index a0ec6ac5e..428c41df1 100644 --- a/apps/roam/src/styles/settingsStyles.css +++ b/apps/roam/src/styles/settingsStyles.css @@ -38,7 +38,8 @@ } :root { - /* Brand secondary, mirroring packages/tailwind-config. */ + /* Brand colours, mirroring packages/tailwind-config. */ + --dg-primary: #ff8c4b; --dg-secondary: #5f57c0; /** Neutrals mirrored too: Roam ships Tailwind's utilities but not this repo's theme extensions. */ --dg-neutral-dark: #1f1f1f; @@ -117,3 +118,45 @@ text-align: center; font-family: monospace; } + +/* Settings search results. + Portalled by Blueprint so the tab rail's `overflow: hidden` cannot clip it. + The surface is painted here rather than inherited: Roam ships its own + Blueprint build and themes `.bp3-popover`, so relying on the library default + leaves the panel transparent and the rail shows straight through it. */ +.dg-settings-search__results .bp3-popover-content { + width: 340px; + background-color: #fff; + border-radius: 3px; + box-shadow: + 0 0 0 1px rgba(16, 22, 26, 0.1), + 0 2px 4px rgba(16, 22, 26, 0.2), + 0 8px 24px rgba(16, 22, 26, 0.2); +} + +.dg-settings-search__results .bp3-menu { + background-color: transparent; +} + +.dg-settings-search__scroll { + max-height: 20rem; + overflow-y: auto; +} + +/* Marks the row a search result jumped to. The row is already scrolled into + view; this only answers "which one of these is it?". */ +.dg-setting-row--flash { + animation: dg-setting-row-flash 1.6s ease-out; +} + +@keyframes dg-setting-row-flash { + 0%, + 40% { + background-color: var(--dg-neutral-light); + box-shadow: inset 3px 0 0 0 var(--dg-primary); + } + 100% { + background-color: transparent; + box-shadow: inset 3px 0 0 0 transparent; + } +} From 95d2a44d540f80cc23f109381b3144d77c3cbfec Mon Sep 17 00:00:00 2001 From: Trang Doan Date: Mon, 31 Aug 2026 23:47:59 -0400 Subject: [PATCH 2/6] ENG-2184 Let the search-result flash outlive the jump that started it 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 --- .../navigation/useSettingAnchorScroll.ts | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts b/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts index 972bc7221..9a04fa802 100644 --- a/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts +++ b/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts @@ -8,6 +8,35 @@ import { const MAX_LOOKUP_FRAMES = 30; const FLASH_DURATION_MS = 1600; +const flashTimeouts = new WeakMap(); + +/** + * Marks the row that was jumped to, and owns the class for the whole animation. + * + * Deliberately not scoped to the effect below: finding the row settles the jump, + * which clears `anchorId` and so re-runs the effect. An effect-scoped cleanup + * would strip the class on that very next render, and the flash would never be + * seen. + */ +const flashRow = (target: Element): void => { + const pending = flashTimeouts.get(target); + if (pending !== undefined) window.clearTimeout(pending); + + // Removing and forcing a reflow restarts the animation, so jumping to the same + // row twice flashes twice rather than riding the first animation out. + target.classList.remove(SETTING_ANCHOR_FLASH_CLASS); + target.getBoundingClientRect(); + target.classList.add(SETTING_ANCHOR_FLASH_CLASS); + + flashTimeouts.set( + target, + window.setTimeout(() => { + target.classList.remove(SETTING_ANCHOR_FLASH_CLASS); + flashTimeouts.delete(target); + }, FLASH_DURATION_MS), + ); +}; + /** The row is not in the DOM when the jump is dispatched — only the active panel renders, * and a `Collapse` mounts later still — so a single lookup misses and this retries. */ export const useSettingAnchorScroll = ({ @@ -21,37 +50,24 @@ export const useSettingAnchorScroll = ({ if (!anchorId) return; let frame = 0; let rafId = 0; - let flashTimeout: ReturnType | undefined; - let flashed: Element | null = null; - - const settle = () => { - onSettled(); - }; const look = () => { const target = document.querySelector(settingAnchorSelector(anchorId)); if (target) { target.scrollIntoView({ block: "center", behavior: "smooth" }); - target.classList.add(SETTING_ANCHOR_FLASH_CLASS); - flashed = target; - flashTimeout = setTimeout(() => { - target.classList.remove(SETTING_ANCHOR_FLASH_CLASS); - }, FLASH_DURATION_MS); - settle(); + flashRow(target); + onSettled(); return; } if (frame++ >= MAX_LOOKUP_FRAMES) { - settle(); + onSettled(); return; } rafId = requestAnimationFrame(look); }; rafId = requestAnimationFrame(look); - return () => { - cancelAnimationFrame(rafId); - if (flashTimeout) clearTimeout(flashTimeout); - flashed?.classList.remove(SETTING_ANCHOR_FLASH_CLASS); - }; + // Only the lookup is cancellable; the flash owns its own lifetime. + return () => cancelAnimationFrame(rafId); }, [anchorId, onSettled]); }; From 9e6e080cc3fd7d8d5804c45289ce10467d76aa8a Mon Sep 17 00:00:00 2001 From: Trang Doan Date: Mon, 31 Aug 2026 23:58:32 -0400 Subject: [PATCH 3/6] ENG-2184 Cover the search ranking tiers 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 --- .../utils/__tests__/settingsSearch.test.ts | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts diff --git a/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts b/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts new file mode 100644 index 000000000..05b6b2f93 --- /dev/null +++ b/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts @@ -0,0 +1,134 @@ +import { describe, expect, it } from "vitest"; +import { rankSettings, SETTINGS_SEARCH_RESULT_LIMIT } from "../settingsSearch"; +import type { SearchableEntry } from "../settingsCatalog"; + +const setting = ( + label: string, + extra: Partial = {}, +): SearchableEntry => + ({ + kind: "setting", + id: label, + anchorId: label, + label, + keywords: [], + path: ["preferences-general"], + breadcrumb: "Preferences › General", + ...extra, + }) as SearchableEntry; + +const page = (label: string): SearchableEntry => ({ + kind: "page", + id: `page:${label}`, + label, + keywords: [], + path: ["features-canvas"], + breadcrumb: "Features", +}); + +const labels = (entries: SearchableEntry[]): string[] => + entries.map((entry) => entry.label); + +describe("rankSettings", () => { + it("returns nothing for an empty or whitespace query", () => { + const entries = [setting("Canvas page format")]; + expect(rankSettings({ entries, query: "" })).toEqual([]); + expect(rankSettings({ entries, query: " " })).toEqual([]); + }); + + it("matches case- and whitespace-insensitively", () => { + const entries = [setting("Canvas page format")]; + expect( + labels(rankSettings({ entries, query: " CANVAS page " })), + ).toEqual(["Canvas page format"]); + }); + + // The tier order is the whole point: an exact label must never be buried under + // a description that happens to mention the same word. + it("orders exact label, then prefix, then substring, then description", () => { + const entries = [ + setting("Mentions overlay in its description", { + description: "Controls the overlay", + }), + setting("Overlay in canvas"), + setting("Discourse context overlay"), + setting("Overlay"), + ]; + expect(labels(rankSettings({ entries, query: "overlay" }))).toEqual([ + "Overlay", + "Overlay in canvas", + "Discourse context overlay", + "Mentions overlay in its description", + ]); + }); + + it("matches a keyword and ranks it below any label match", () => { + const entries = [ + setting("Auto canvas relations", { keywords: ["tldraw"] }), + setting("Tldraw shortcut"), + ]; + expect(labels(rankSettings({ entries, query: "tldraw" }))).toEqual([ + "Tldraw shortcut", + "Auto canvas relations", + ]); + }); + + it("puts settings before pages at the same tier, then sorts by label", () => { + const entries = [page("Canvas"), setting("Canvas")]; + expect( + rankSettings({ entries, query: "canvas" }).map((entry) => entry.kind), + ).toEqual(["setting", "page"]); + }); + + it("breaks a tier tie alphabetically for a stable list", () => { + const entries = [ + setting("Overlay zeta"), + setting("Overlay alpha"), + setting("Overlay mid"), + ]; + expect(labels(rankSettings({ entries, query: "overlay " }))).toEqual([ + "Overlay alpha", + "Overlay mid", + "Overlay zeta", + ]); + }); + + // Every word has to appear somewhere, but the tier still comes from the whole + // query, so a single-word search keeps its precision. + it("matches a multi-word query across label, breadcrumb and keywords", () => { + const entries = [ + setting("Key image", { + breadcrumb: "Grammar › Nodes › Claim › Canvas", + keywords: ["tldraw"], + }), + ]; + expect(labels(rankSettings({ entries, query: "claim key image" }))).toEqual( + ["Key image"], + ); + expect(rankSettings({ entries, query: "claim key missing" })).toEqual([]); + }); + + it("does not scatter a single word across fields", () => { + const entries = [setting("Tag", { breadcrumb: "Grammar › Nodes" })]; + expect(rankSettings({ entries, query: "zzz" })).toEqual([]); + }); + + it("caps results at the limit", () => { + const entries = Array.from({ length: 20 }, (_, index) => + setting(`Overlay ${index}`), + ); + expect(rankSettings({ entries, query: "overlay" })).toHaveLength( + SETTINGS_SEARCH_RESULT_LIMIT, + ); + expect(rankSettings({ entries, query: "overlay", limit: 3 })).toHaveLength( + 3, + ); + }); + + it("searches a page's breadcrumb, which is its only extra text", () => { + const entries = [page("Canvas")]; + expect(labels(rankSettings({ entries, query: "features" }))).toEqual([ + "Canvas", + ]); + }); +}); From 95ef51ebdc52c2743a8771649b1f3d12341db84e Mon Sep 17 00:00:00 2001 From: Trang Doan Date: Fri, 4 Sep 2026 17:49:31 -0400 Subject: [PATCH 4/6] ENG-2184 Inset the search field to the rail's text edge Review feedback: the box sat flush with the rail while every tab title under it was indented. Co-Authored-By: Claude Fable 5.1 --- .../components/settings/navigation/SettingsSearchField.tsx | 2 +- apps/roam/src/styles/settingsStyles.css | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx b/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx index bb94d2480..f6a0ab377 100644 --- a/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx +++ b/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx @@ -155,7 +155,7 @@ const SettingsSearchField = ({ ) } > -
+
{ inputRef.current = input; diff --git a/apps/roam/src/styles/settingsStyles.css b/apps/roam/src/styles/settingsStyles.css index 428c41df1..7e26d1bc9 100644 --- a/apps/roam/src/styles/settingsStyles.css +++ b/apps/roam/src/styles/settingsStyles.css @@ -119,6 +119,12 @@ font-family: monospace; } +/* Settings search field. Inset to the tab titles' left edge so the box does not + sit flush with the rail while everything under it is indented. */ +.dg-settings-search { + margin: 4px 10px 8px; +} + /* Settings search results. Portalled by Blueprint so the tab rail's `overflow: hidden` cannot clip it. The surface is painted here rather than inherited: Roam ships its own From e807ce9ddcde9c4036bc79259b064a655ff39ef4 Mon Sep 17 00:00:00 2001 From: Trang Doan Date: Fri, 4 Sep 2026 18:10:45 -0400 Subject: [PATCH 5/6] ENG-2184 Escape backslashes in the anchor selector; drop a redundant 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 --- .../utils/__tests__/settingsSearch.test.ts | 21 +++++++++---------- .../settings/utils/settingAnchor.ts | 5 +++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts b/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts index 05b6b2f93..ad84a8595 100644 --- a/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts +++ b/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts @@ -5,17 +5,16 @@ import type { SearchableEntry } from "../settingsCatalog"; const setting = ( label: string, extra: Partial = {}, -): SearchableEntry => - ({ - kind: "setting", - id: label, - anchorId: label, - label, - keywords: [], - path: ["preferences-general"], - breadcrumb: "Preferences › General", - ...extra, - }) as SearchableEntry; +): SearchableEntry => ({ + kind: "setting", + id: label, + anchorId: label, + label, + keywords: [], + path: ["preferences-general"], + breadcrumb: "Preferences › General", + ...extra, +}); const page = (label: string): SearchableEntry => ({ kind: "page", diff --git a/apps/roam/src/components/settings/utils/settingAnchor.ts b/apps/roam/src/components/settings/utils/settingAnchor.ts index 479543024..e10882f9a 100644 --- a/apps/roam/src/components/settings/utils/settingAnchor.ts +++ b/apps/roam/src/components/settings/utils/settingAnchor.ts @@ -7,8 +7,9 @@ export const settingAnchor = ( [SETTING_ANCHOR_ATTRIBUTE]: settingKeys.join("/"), }); -/** Setting keys are authored identifiers, so quoting is enough to build a selector. */ +/** Setting keys are authored identifiers today; escaped anyway so a future key with a + * quote or backslash cannot break out of the attribute selector. */ export const settingAnchorSelector = (anchorId: string): string => - `[${SETTING_ANCHOR_ATTRIBUTE}="${anchorId.replace(/"/g, '\\"')}"]`; + `[${SETTING_ANCHOR_ATTRIBUTE}="${anchorId.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"]`; export const SETTING_ANCHOR_FLASH_CLASS = "dg-setting-row--flash"; From 786cb7d0055a01e42c87d7a861b72fd92e7c006a Mon Sep 17 00:00:00 2001 From: Trang Doan Date: Mon, 7 Sep 2026 18:21:11 -0400 Subject: [PATCH 6/6] ENG-2184 Drop unit tests and trim comments to the decisions they record Co-Authored-By: Claude Fable 5.1 --- .../roam/src/components/settings/Settings.tsx | 3 +- .../navigation/SettingsSearchField.tsx | 15 +- .../navigation/useSettingAnchorScroll.ts | 14 +- .../utils/__tests__/settingsSearch.test.ts | 133 ------------------ .../settings/utils/settingAnchor.ts | 3 +- .../settings/utils/settingsNavigation.ts | 3 +- apps/roam/src/styles/settingsStyles.css | 13 +- 7 files changed, 15 insertions(+), 169 deletions(-) delete mode 100644 apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts diff --git a/apps/roam/src/components/settings/Settings.tsx b/apps/roam/src/components/settings/Settings.tsx index acbf235e8..7225beea4 100644 --- a/apps/roam/src/components/settings/Settings.tsx +++ b/apps/roam/src/components/settings/Settings.tsx @@ -98,8 +98,7 @@ export const SettingsDialog = ({ (tabId: string) => dispatch({ type: "select-tab", tabId }), [], ); - // Cleared once the row is found or the lookup gives up, so a repeat jump to the - // same row still scrolls. + // Cleared once settled, so a repeat jump to the same row still scrolls. const [pendingAnchorId, setPendingAnchorId] = useState(null); const handleSearchSelect = useCallback((entry: SearchableEntry) => { dispatch({ type: "navigate", path: entry.path }); diff --git a/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx b/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx index f6a0ab377..cd133fe4c 100644 --- a/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx +++ b/apps/roam/src/components/settings/navigation/SettingsSearchField.tsx @@ -23,8 +23,7 @@ const SettingsSearchResult = ({ onSelect: (entry: SearchableEntry) => void; }): JSX.Element => ( {entry.label} - {/* Undimmed on purpose: white on the active row's `#137CBD` is 4.5:1, and any - opacity below 100% drops under AA for text this size (80% measures 3.5:1). */} + {/* Undimmed: any opacity drops white-on-#137CBD below AA. */} @@ -41,8 +39,7 @@ const SettingsSearchResult = ({
} - // Selecting on mousedown so the choice lands before the input's blur closes - // the list out from under the pointer. + // Select on mousedown, before the input's blur closes the list. onMouseDown={(event: React.MouseEvent) => { event.preventDefault(); onSelect(entry); @@ -69,8 +66,7 @@ const SettingsSearchField = ({ ); const isShowingResults = isOpen && query.trim() !== ""; - // Keeps the keyboard-selected row visible when the list scrolls, following - // the same approach as DiscourseNodeSearchMenu. + // Keeps the keyboard-selected row visible. useEffect(() => { const container = scrollContainerRef.current; if (!container) return; @@ -97,8 +93,7 @@ const SettingsSearchField = ({ const handleKeyDown = (event: React.KeyboardEvent) => { if (event.key === "Escape") { - // Stopping propagation so Escape clears the search rather than closing - // the whole Settings dialog out from under a half-typed query. + // Escape clears the query instead of closing the dialog. if (query !== "") event.stopPropagation(); setQuery(""); setIsOpen(false); diff --git a/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts b/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts index 9a04fa802..5c5eacd27 100644 --- a/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts +++ b/apps/roam/src/components/settings/navigation/useSettingAnchorScroll.ts @@ -10,20 +10,13 @@ const FLASH_DURATION_MS = 1600; const flashTimeouts = new WeakMap(); -/** - * Marks the row that was jumped to, and owns the class for the whole animation. - * - * Deliberately not scoped to the effect below: finding the row settles the jump, - * which clears `anchorId` and so re-runs the effect. An effect-scoped cleanup - * would strip the class on that very next render, and the flash would never be - * seen. - */ +/** Owns the flash independently of the effect: settling clears anchorId and re-runs the + * effect, whose cleanup would otherwise strip the class before it is seen. */ const flashRow = (target: Element): void => { const pending = flashTimeouts.get(target); if (pending !== undefined) window.clearTimeout(pending); - // Removing and forcing a reflow restarts the animation, so jumping to the same - // row twice flashes twice rather than riding the first animation out. + // Remove and reflow so hitting the same row twice restarts the animation. target.classList.remove(SETTING_ANCHOR_FLASH_CLASS); target.getBoundingClientRect(); target.classList.add(SETTING_ANCHOR_FLASH_CLASS); @@ -67,7 +60,6 @@ export const useSettingAnchorScroll = ({ }; rafId = requestAnimationFrame(look); - // Only the lookup is cancellable; the flash owns its own lifetime. return () => cancelAnimationFrame(rafId); }, [anchorId, onSettled]); }; diff --git a/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts b/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts deleted file mode 100644 index ad84a8595..000000000 --- a/apps/roam/src/components/settings/utils/__tests__/settingsSearch.test.ts +++ /dev/null @@ -1,133 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { rankSettings, SETTINGS_SEARCH_RESULT_LIMIT } from "../settingsSearch"; -import type { SearchableEntry } from "../settingsCatalog"; - -const setting = ( - label: string, - extra: Partial = {}, -): SearchableEntry => ({ - kind: "setting", - id: label, - anchorId: label, - label, - keywords: [], - path: ["preferences-general"], - breadcrumb: "Preferences › General", - ...extra, -}); - -const page = (label: string): SearchableEntry => ({ - kind: "page", - id: `page:${label}`, - label, - keywords: [], - path: ["features-canvas"], - breadcrumb: "Features", -}); - -const labels = (entries: SearchableEntry[]): string[] => - entries.map((entry) => entry.label); - -describe("rankSettings", () => { - it("returns nothing for an empty or whitespace query", () => { - const entries = [setting("Canvas page format")]; - expect(rankSettings({ entries, query: "" })).toEqual([]); - expect(rankSettings({ entries, query: " " })).toEqual([]); - }); - - it("matches case- and whitespace-insensitively", () => { - const entries = [setting("Canvas page format")]; - expect( - labels(rankSettings({ entries, query: " CANVAS page " })), - ).toEqual(["Canvas page format"]); - }); - - // The tier order is the whole point: an exact label must never be buried under - // a description that happens to mention the same word. - it("orders exact label, then prefix, then substring, then description", () => { - const entries = [ - setting("Mentions overlay in its description", { - description: "Controls the overlay", - }), - setting("Overlay in canvas"), - setting("Discourse context overlay"), - setting("Overlay"), - ]; - expect(labels(rankSettings({ entries, query: "overlay" }))).toEqual([ - "Overlay", - "Overlay in canvas", - "Discourse context overlay", - "Mentions overlay in its description", - ]); - }); - - it("matches a keyword and ranks it below any label match", () => { - const entries = [ - setting("Auto canvas relations", { keywords: ["tldraw"] }), - setting("Tldraw shortcut"), - ]; - expect(labels(rankSettings({ entries, query: "tldraw" }))).toEqual([ - "Tldraw shortcut", - "Auto canvas relations", - ]); - }); - - it("puts settings before pages at the same tier, then sorts by label", () => { - const entries = [page("Canvas"), setting("Canvas")]; - expect( - rankSettings({ entries, query: "canvas" }).map((entry) => entry.kind), - ).toEqual(["setting", "page"]); - }); - - it("breaks a tier tie alphabetically for a stable list", () => { - const entries = [ - setting("Overlay zeta"), - setting("Overlay alpha"), - setting("Overlay mid"), - ]; - expect(labels(rankSettings({ entries, query: "overlay " }))).toEqual([ - "Overlay alpha", - "Overlay mid", - "Overlay zeta", - ]); - }); - - // Every word has to appear somewhere, but the tier still comes from the whole - // query, so a single-word search keeps its precision. - it("matches a multi-word query across label, breadcrumb and keywords", () => { - const entries = [ - setting("Key image", { - breadcrumb: "Grammar › Nodes › Claim › Canvas", - keywords: ["tldraw"], - }), - ]; - expect(labels(rankSettings({ entries, query: "claim key image" }))).toEqual( - ["Key image"], - ); - expect(rankSettings({ entries, query: "claim key missing" })).toEqual([]); - }); - - it("does not scatter a single word across fields", () => { - const entries = [setting("Tag", { breadcrumb: "Grammar › Nodes" })]; - expect(rankSettings({ entries, query: "zzz" })).toEqual([]); - }); - - it("caps results at the limit", () => { - const entries = Array.from({ length: 20 }, (_, index) => - setting(`Overlay ${index}`), - ); - expect(rankSettings({ entries, query: "overlay" })).toHaveLength( - SETTINGS_SEARCH_RESULT_LIMIT, - ); - expect(rankSettings({ entries, query: "overlay", limit: 3 })).toHaveLength( - 3, - ); - }); - - it("searches a page's breadcrumb, which is its only extra text", () => { - const entries = [page("Canvas")]; - expect(labels(rankSettings({ entries, query: "features" }))).toEqual([ - "Canvas", - ]); - }); -}); diff --git a/apps/roam/src/components/settings/utils/settingAnchor.ts b/apps/roam/src/components/settings/utils/settingAnchor.ts index e10882f9a..be9aef18e 100644 --- a/apps/roam/src/components/settings/utils/settingAnchor.ts +++ b/apps/roam/src/components/settings/utils/settingAnchor.ts @@ -7,8 +7,7 @@ export const settingAnchor = ( [SETTING_ANCHOR_ATTRIBUTE]: settingKeys.join("/"), }); -/** Setting keys are authored identifiers today; escaped anyway so a future key with a - * quote or backslash cannot break out of the attribute selector. */ +/** Escaped so a key with a quote or backslash cannot break the selector. */ export const settingAnchorSelector = (anchorId: string): string => `[${SETTING_ANCHOR_ATTRIBUTE}="${anchorId.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"]`; diff --git a/apps/roam/src/components/settings/utils/settingsNavigation.ts b/apps/roam/src/components/settings/utils/settingsNavigation.ts index 8f945343e..34a6b0021 100644 --- a/apps/roam/src/components/settings/utils/settingsNavigation.ts +++ b/apps/roam/src/components/settings/utils/settingsNavigation.ts @@ -43,8 +43,7 @@ export const settingsNavReducer = ( return action.tabId === tabIdOf(state) && state.length === 1 ? state : rootPath(action.tabId); - // Search jumps to a setting several segments deep in one go, which `push` - // cannot express. An empty path is ignored rather than emptying the route. + // Search jumps several segments at once; an empty path is ignored. case "navigate": return action.path.length === 0 || isSamePath(action.path, state) ? state diff --git a/apps/roam/src/styles/settingsStyles.css b/apps/roam/src/styles/settingsStyles.css index 7e26d1bc9..e7a7a2390 100644 --- a/apps/roam/src/styles/settingsStyles.css +++ b/apps/roam/src/styles/settingsStyles.css @@ -119,17 +119,13 @@ font-family: monospace; } -/* Settings search field. Inset to the tab titles' left edge so the box does not - sit flush with the rail while everything under it is indented. */ +/** Inset to the tab titles' left edge. */ .dg-settings-search { margin: 4px 10px 8px; } -/* Settings search results. - Portalled by Blueprint so the tab rail's `overflow: hidden` cannot clip it. - The surface is painted here rather than inherited: Roam ships its own - Blueprint build and themes `.bp3-popover`, so relying on the library default - leaves the panel transparent and the rail shows straight through it. */ +/** Portalled by Blueprint so the rail's overflow cannot clip it; painted here because + * Roam themes `.bp3-popover` and the library default is transparent. */ .dg-settings-search__results .bp3-popover-content { width: 340px; background-color: #fff; @@ -149,8 +145,7 @@ overflow-y: auto; } -/* Marks the row a search result jumped to. The row is already scrolled into - view; this only answers "which one of these is it?". */ +/** Marks the row a search result jumped to. */ .dg-setting-row--flash { animation: dg-setting-row-flash 1.6s ease-out; }