feat(UI): cypher saved queries category and subcategory cleanup - BED-7541#2648
Open
feat(UI): cypher saved queries category and subcategory cleanup - BED-7541#2648
Conversation
catsiller
added
enhancement
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces imperative auto-scroll (useRef + useEffect) with a callback ref on the selected Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
catsiller
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (2)
102-102: Duplicate Tailwind class.
rounded rounded-smappliesroundedthen overrides it withrounded-sm; drop the redundantrounded.- className={`p-2 rounded rounded-sm flex items-center w-full cursor-pointer hover:bg-neutral-light-3 dark:hover:bg-neutral-dark-3 justify-between pl-4 list-none ${ + className={`p-2 rounded-sm flex items-center w-full cursor-pointer hover:bg-neutral-light-3 dark:hover:bg-neutral-dark-3 justify-between pl-4 list-none ${🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` at line 102, In PrebuiltSearchList component update the className on the list item that currently contains "rounded rounded-sm ..." (inside PrebuiltSearchList.tsx) by removing the redundant "rounded" token so only "rounded-sm" remains; this eliminates the duplicate Tailwind rule without changing visual intent and keeps the rest of the classes (p-2, flex, items-center, etc.) intact.
93-97: Sticky offsettop-9is coupled to the exact height of the category header.The subcategory sticks at
top-9(36px), which assumes the category row (py-2+font-boldsingle line) always renders at 36px. A font-size/line-height change, zoom, or localized wider text that wraps will cause the subheader to overlap with or leave a gap under the category header. Consider measuring the category header height (e.g., via a ref + CSS variable) or stacking them in a single sticky container so the offset doesn't have to be hard-coded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 93 - 97, The subheader in PrebuiltSearchList uses a hard-coded sticky offset class top-9 which assumes the category header height; instead measure the actual category header height and drive the offset dynamically or make both header and subheader share a single sticky container. Add a ref to the category row (e.g., categoryRef) and on mount/update set a CSS variable (e.g., --category-height) to its clientHeight, then replace top-9 with a style using var(--category-height) on the element using styles.subheader; alternatively wrap the category header and subheader in one element and apply position: sticky to that container so no fixed top value is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Around line 71-76: The ref callback selectedItem is only using its parameter
e, so remove unused dependencies to silence the eslint rule and avoid
unnecessary re-creations: update the useCallback call for selectedItem to have
an empty dependency array (i.e., useCallback(..., [])) so its identity stays
stable; leave the ref usage (ref={isSelectedQuery(...) ? selectedItem : null})
unchanged so React still invokes the callback on selection changes.
- Around line 82-88: The category header visibility currently uses a truthiness
check on only the first subsection (!!queryData[0].queries.length) which hides
the category when the first subsection is empty; update the condition in
PrebuiltSearchList (where groupedQueries is mapped and queryData is used) to
test all subsections instead (e.g., use queryData.some(q => q.queries.length))
so the category header renders when any subsection under that category has
queries.
- Around line 92-97: The subheader <div> is being rendered directly inside a
<ul> in PrebuiltSearchList, causing invalid DOM nesting; update the JSX so the
sticky subheader is not a direct child of the <ul> — either move the {subheader
&& !!queries.length && <div className={styles.subheader}>…</div>} to be a
sibling above the <ul> or wrap it in an <li> (e.g., <li role="presentation">)
before rendering list items; ensure you update the conditional that uses
subheader, queries and styles.subheader so the list semantics remain correct and
accessibility attributes (role/aria) are applied if you choose the <li>
approach.
---
Nitpick comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Line 102: In PrebuiltSearchList component update the className on the list
item that currently contains "rounded rounded-sm ..." (inside
PrebuiltSearchList.tsx) by removing the redundant "rounded" token so only
"rounded-sm" remains; this eliminates the duplicate Tailwind rule without
changing visual intent and keeps the rest of the classes (p-2, flex,
items-center, etc.) intact.
- Around line 93-97: The subheader in PrebuiltSearchList uses a hard-coded
sticky offset class top-9 which assumes the category header height; instead
measure the actual category header height and drive the offset dynamically or
make both header and subheader share a single sticky container. Add a ref to the
category row (e.g., categoryRef) and on mount/update set a CSS variable (e.g.,
--category-height) to its clientHeight, then replace top-9 with a style using
var(--category-height) on the element using styles.subheader; alternatively wrap
the category header and subheader in one element and apply position: sticky to
that container so no fixed top value is required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b5f1b05b-2fc3-4f9d-9f1d-c395f60160b5
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx
Comment on lines
+71
to
+76
| const selectedItem = useCallback( | ||
| (e: HTMLLIElement | null) => { | ||
| if (e) e.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); | ||
| }, | ||
| [selectedQuery, showCommonQueries] | ||
| ); |
Contributor
There was a problem hiding this comment.
Remove unnecessary useCallback dependencies to clear the build warning.
The callback body only references its e parameter, so selectedQuery and showCommonQueries are unused deps and are breaking the Build UI pipeline (react-hooks/exhaustive-deps). Re-selection still works correctly without them: because ref={isSelectedQuery(...) ? selectedItem : null} changes per-element, React invokes the ref callback on the newly-selected <li> as it mounts/updates, which triggers the scrollIntoView. Keeping these deps also needlessly changes the callback identity and re-fires the ref on unrelated renders.
♻️ Proposed fix
- const selectedItem = useCallback(
- (e: HTMLLIElement | null) => {
- if (e) e.scrollIntoView({ behavior: 'smooth', block: 'nearest' });
- },
- [selectedQuery, showCommonQueries]
- );
+ const selectedItem = useCallback((e: HTMLLIElement | null) => {
+ if (e) e.scrollIntoView({ behavior: 'smooth', block: 'nearest' });
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const selectedItem = useCallback( | |
| (e: HTMLLIElement | null) => { | |
| if (e) e.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); | |
| }, | |
| [selectedQuery, showCommonQueries] | |
| ); | |
| const selectedItem = useCallback((e: HTMLLIElement | null) => { | |
| if (e) e.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); | |
| }, []); |
🧰 Tools
🪛 GitHub Actions: Build UI
[warning] 75-75: ESLint (react-hooks/exhaustive-deps): React Hook useCallback has unnecessary dependencies: 'selectedQuery' and 'showCommonQueries'. Either exclude them or remove the dependency array.
🪛 GitHub Check: build-ui
[warning] 75-75:
React Hook useCallback has unnecessary dependencies: 'selectedQuery' and 'showCommonQueries'. Either exclude them or remove the dependency array
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`
around lines 71 - 76, The ref callback selectedItem is only using its parameter
e, so remove unused dependencies to silence the eslint rule and avoid
unnecessary re-creations: update the useCallback call for selectedItem to have
an empty dependency array (i.e., useCallback(..., [])) so its identity stays
stable; leave the ref usage (ref={isSelectedQuery(...) ? selectedItem : null})
unchanged so React still invokes the callback on selection changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
71-77:⚠️ Potential issue | 🟠 MajorRemove the failing hook deps workaround.
The build is failing because the disable directive is unused, and
selectedQuery/showCommonQueriesare still reported as unnecessary dependencies. The ref callback only usese, so keep it stable and remove the suppressor.Proposed fix
- // eslint-disable-next-line react-hooks/exhaustive-deps -- deps intentionally trigger ref re-execution on selection change - const selectedItem = useCallback( - (e: HTMLLIElement | null) => { - if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); - }, - [selectedQuery, showCommonQueries] - ); + const selectedItem = useCallback((e: HTMLLIElement | null) => { + if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); + }, []);Verify the current hook/deps block if needed:
#!/bin/bash file="$(fd -a PrebuiltSearchList.tsx | head -n 1)" sed -n '68,78p' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 71 - 77, The current useCallback for selectedItem includes an unnecessary eslint-disable and lists unused deps (selectedQuery, showCommonQueries); remove the "// eslint-disable-next-line react-hooks/exhaustive-deps ..." comment and simplify the hook to a stable ref callback by keeping selectedItem as useCallback((e: HTMLLIElement | null) => { if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); }, []); so the callback only depends on its event parameter and no longer triggers the lint warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Around line 114-123: The static aria-label on each result makes every row
announce the same text; update the interactive div in PrebuiltSearchList so it
exposes a unique accessible name by including the query's label (or removing the
aria-label entirely). Locate the element using the identifiers clickHandler and
adaptClickHandlerToKeyDown and replace the fixed aria-label='Run pre-built
search query' with a dynamic value that incorporates the query identifier (for
example include query.name or query text) so each row is announced distinctly
while keeping the existing onClick/onKeyDown handlers.
---
Duplicate comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Around line 71-77: The current useCallback for selectedItem includes an
unnecessary eslint-disable and lists unused deps (selectedQuery,
showCommonQueries); remove the "// eslint-disable-next-line
react-hooks/exhaustive-deps ..." comment and simplify the hook to a stable ref
callback by keeping selectedItem as useCallback((e: HTMLLIElement | null) => {
if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); }, []); so the
callback only depends on its event parameter and no longer triggers the lint
warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5222abaf-c807-400c-a980-75a523bdf7a6
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
50-50:⚠️ Potential issue | 🔴 CriticalRemove the unused
showCommonQueriesdestructure to unblock CI.The
Build UIpipeline is failing becauseshowCommonQueriesis destructured but never used. Keep the prop type if callers still pass it, but stop binding it locally unless this component needs it again.Proposed fix
const PrebuiltSearchList: FC<PrebuiltSearchListProps> = ({ listSections, - showCommonQueries, clickHandler, deleteHandler, clearFiltersHandler, }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` at line 50, The component PrebuiltSearchList is destructuring showCommonQueries but never uses it; remove showCommonQueries from the local destructure in PrebuiltSearchList (in the function/component signature where props are unpacked) to stop the unused variable error, while keeping the showCommonQueries property in the public prop type/interface (e.g., PrebuiltSearchListProps) if callers still supply it; ensure no other references to showCommonQueries exist in methods like the component body or helper functions before committing.
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
20-20:⚠️ Potential issue | 🟡 MinorStabilize the callback ref to avoid repeated auto-scrolls.
selectedItemis recreated on every render, so the selected<li>receives a new callback ref and can callscrollIntoViewagain on unrelated rerenders. Memoize it withuseCallback.Proposed fix
-import { FC } from 'react'; +import { FC, useCallback } from 'react';- const selectedItem = (e: HTMLLIElement | null) => { + const selectedItem = useCallback((e: HTMLLIElement | null) => { if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); - }; + }, []);In React 18, are callback refs invoked again when the ref callback function identity changes between renders?Also applies to: 71-73, 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` at line 20, The ref callback used for the selected <li> (the selectedItem callback/ref) is recreated each render causing repeated scrollIntoView calls; memoize that callback with React.useCallback so its identity only changes when the selection actually changes (e.g., depend on selectedId or the relevant selected index/props) and replace the inline ref callbacks at the three places (the selectedItem ref and the similar refs around the list items at lines referenced) with the memoized callback to stabilize behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Line 50: The component PrebuiltSearchList is destructuring showCommonQueries
but never uses it; remove showCommonQueries from the local destructure in
PrebuiltSearchList (in the function/component signature where props are
unpacked) to stop the unused variable error, while keeping the showCommonQueries
property in the public prop type/interface (e.g., PrebuiltSearchListProps) if
callers still supply it; ensure no other references to showCommonQueries exist
in methods like the component body or helper functions before committing.
---
Duplicate comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Line 20: The ref callback used for the selected <li> (the selectedItem
callback/ref) is recreated each render causing repeated scrollIntoView calls;
memoize that callback with React.useCallback so its identity only changes when
the selection actually changes (e.g., depend on selectedId or the relevant
selected index/props) and replace the inline ref callbacks at the three places
(the selectedItem ref and the similar refs around the list items at lines
referenced) with the memoized callback to stabilize behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d548f1f0-6d26-41dc-9a0f-33ee962b09ea
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
25-30:⚠️ Potential issue | 🔴 CriticalBreaking change:
showCommonQueriesprop removed but still passed by callers — TypeScript compilation will fail.
PrebuiltSearchListPropsno longer declaresshowCommonQueries, butCommonSearches.tsx(line 208) andPrebuiltSearchList.test.tsx(line 58) still pass it. This will cause TypeScript strict mode to reject excess propertyshowCommonQueries.Update callers to remove the prop:
Fix in CommonSearches.tsx
<PrebuiltSearchList listSections={isFiltered ? filteredList : queryList} clickHandler={handleClick} deleteHandler={handleDeleteQuery} clearFiltersHandler={handleClearFilters} - showCommonQueries={showCommonQueries} />Fix in PrebuiltSearchList.test.tsx
<PrebuiltSearchList listSections={testListSections} - showCommonQueries={true} clickHandler={testClickHandler} deleteHandler={testDeleteHandler} clearFiltersHandler={testClearFiltersHandler} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 25 - 30, The PrebuiltSearchListProps interface no longer includes showCommonQueries, but callers still pass it causing TypeScript excess-property errors; remove the showCommonQueries prop and any conditional code that references it where PrebuiltSearchList is instantiated (notably in CommonSearches.tsx and PrebuiltSearchList.test.tsx), update those JSX usages to simply call <PrebuiltSearchList ... /> without showCommonQueries, and adjust the test expectations/fixtures to reflect the component's behavior without that prop (remove assertions or branching tied to showCommonQueries).
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (1)
69-71:⚠️ Potential issue | 🟠 MajorInline ref callback re-fires
scrollIntoViewon every render — expect scroll jitter withbehavior: 'smooth'.
selectedItemis now a fresh function on each render. Because therefprop identity changes every render, React calls the previous ref withnulland the new ref with the element on every re-render of the selected<li>, which triggersscrollIntoView({ behavior: 'smooth', block: 'start' })repeatedly. Any unrelated state change in an ancestor (filter input, hover state, query fetch refresh, etc.) will cause the saved-queries list to smooth-scroll again. The prior review suggested wrapping inuseCallback(..., [])specifically to stabilize identity — please restore that.♻️ Proposed fix
- const selectedItem = (e: HTMLLIElement | null) => { - if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); - }; + const selectedItem = useCallback((e: HTMLLIElement | null) => { + if (e) e.scrollIntoView({ behavior: 'smooth', block: 'start' }); + }, []);And add
useCallbackto the React import:-import { FC } from 'react'; +import { FC, useCallback } from 'react';Also applies to: 97-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 69 - 71, The inline ref callback selectedItem is recreated every render which causes scrollIntoView({ behavior: 'smooth', block: 'start' }) to fire repeatedly; fix it by importing useCallback from React and wrapping the ref callback in useCallback with an empty dependency array (e.g., const selectedItem = useCallback((e: HTMLLIElement | null) => { if (e) e.scrollIntoView({...}); }, [])) so its identity is stable across renders; also apply the same change to the other inline ref callbacks in this file (the similar ref defined later) to prevent repeated smooth scrolling.
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx (3)
87-92:key={i}on the subheader group and duplicate innerkeyon the button div.Two small nits in the new markup:
- Line 87: using the array index
ias the React key for eachqueryItemis fine only ifqueryDataordering and length are stable. If filter changes can reorder subsections, preferkey={subheader ?? i}to avoid reconciling state to the wrong row.- Line 112: the inner
<div role='button'>also carrieskey={${id}-${idx}}, which has no effect since it's not inside a list render — safe to remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 87 - 92, The list item key uses the unstable array index `i` and there's a redundant inner key on the button div; in PrebuiltSearchList replace key={i} on the outer wrapping div (the one rendering each subheader group) with key={subheader ?? i} to prefer a stable subheader identifier, and remove the unnecessary key={`${id}-${idx}`} from the inner <div role='button'> (the clickable item container) since it is not part of a sibling list and the key has no effect.
97-107: Minor:isSelectedQuery(name, id)is evaluated three times per row.Inside each
<li>,isSelectedQuery(name, id)is called for the className, the inline style, and the ref. Consider computing it once per row for readability and a tiny perf win.♻️ Suggested refactor
{queries?.map((lineItem, idx) => { const { id, name, description, query, canEdit = false } = lineItem; + const isSelected = isSelectedQuery(name, id); return ( <li className={`p-2 rounded rounded-sm flex items-center w-full cursor-pointer hover:bg-neutral-light-3 dark:hover:bg-neutral-dark-3 justify-between pl-4 list-none ${ - isSelectedQuery(name, id) ? styles.selected : '' + isSelected ? styles.selected : '' }`} - style={ - isSelectedQuery(name, id) - ? { scrollMarginTop: '72px' } - : undefined - } + style={isSelected ? { scrollMarginTop: '72px' } : undefined} key={`${id}-${idx}`} - ref={isSelectedQuery(name, id) ? selectedItem : null}> + ref={isSelected ? selectedItem : null}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 97 - 107, Compute the selection boolean once per row and reuse it: call isSelectedQuery(name, id) once (e.g., const selected = isSelectedQuery(name, id)) inside the render for the list item in PrebuiltSearchList, then use selected for the className check (styles.selected), for the style conditional (scrollMarginTop), and for the ref assignment (selected ? selectedItem : null) to avoid re-evaluating the function three times.
79-92: Sticky header offsets are hardcoded and coupled.Category uses
sticky top-0withpy-2, and the subheader usessticky top-9assuming the category header is exactly 36px tall. The<li>scroll target usesscrollMarginTop: '72px'. If theme typography or padding ever changes, all three will drift out of sync (either overlapping content or leaving gaps). Consider extracting these into named constants or CSS variables and/or measuring the category header height, so the offsets stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx` around lines 79 - 92, The sticky header offsets in PrebuiltSearchList are hardcoded and coupled (category uses sticky top-0 with py-2, subheader uses sticky top-9, and list items use scrollMarginTop: '72px'), so extract and centralize the offset into a single source of truth: introduce a CSS variable (e.g., --prebuilt-header-offset) or a React state value and apply it to styles.subheader top values and the li scrollMarginTop; alternatively measure the actual category header height in useEffect (querying the category header element in PrebuiltSearchList) and set the CSS variable on the component root so both the category (styles.subheader) and subheader (styles.subheader) top positions and the scroll target use the same computed offset to avoid drift when typography/padding changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Line 113: The aria-label in PrebuiltSearchList.tsx currently uses a template
with `${name || description}` which can yield the literal "undefined"; update
the logic that builds the aria-label for the element (the attribute currently
set as aria-label={`Run pre-built search query: ${name || description}`}) to
guard against undefined/empty values—use a safe fallback (e.g., coalesce to an
empty string or conditionally omit the suffix) so the rendered attribute never
contains "undefined" (ensure you reference the name and description variables in
your change and keep the prefix "Run pre-built search query:" only when a
non-empty label exists).
---
Outside diff comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Around line 25-30: The PrebuiltSearchListProps interface no longer includes
showCommonQueries, but callers still pass it causing TypeScript excess-property
errors; remove the showCommonQueries prop and any conditional code that
references it where PrebuiltSearchList is instantiated (notably in
CommonSearches.tsx and PrebuiltSearchList.test.tsx), update those JSX usages to
simply call <PrebuiltSearchList ... /> without showCommonQueries, and adjust the
test expectations/fixtures to reflect the component's behavior without that prop
(remove assertions or branching tied to showCommonQueries).
---
Duplicate comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Around line 69-71: The inline ref callback selectedItem is recreated every
render which causes scrollIntoView({ behavior: 'smooth', block: 'start' }) to
fire repeatedly; fix it by importing useCallback from React and wrapping the ref
callback in useCallback with an empty dependency array (e.g., const selectedItem
= useCallback((e: HTMLLIElement | null) => { if (e) e.scrollIntoView({...}); },
[])) so its identity is stable across renders; also apply the same change to the
other inline ref callbacks in this file (the similar ref defined later) to
prevent repeated smooth scrolling.
---
Nitpick comments:
In
`@packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx`:
- Around line 87-92: The list item key uses the unstable array index `i` and
there's a redundant inner key on the button div; in PrebuiltSearchList replace
key={i} on the outer wrapping div (the one rendering each subheader group) with
key={subheader ?? i} to prefer a stable subheader identifier, and remove the
unnecessary key={`${id}-${idx}`} from the inner <div role='button'> (the
clickable item container) since it is not part of a sibling list and the key has
no effect.
- Around line 97-107: Compute the selection boolean once per row and reuse it:
call isSelectedQuery(name, id) once (e.g., const selected =
isSelectedQuery(name, id)) inside the render for the list item in
PrebuiltSearchList, then use selected for the className check (styles.selected),
for the style conditional (scrollMarginTop), and for the ref assignment
(selected ? selectedItem : null) to avoid re-evaluating the function three
times.
- Around line 79-92: The sticky header offsets in PrebuiltSearchList are
hardcoded and coupled (category uses sticky top-0 with py-2, subheader uses
sticky top-9, and list items use scrollMarginTop: '72px'), so extract and
centralize the offset into a single source of truth: introduce a CSS variable
(e.g., --prebuilt-header-offset) or a React state value and apply it to
styles.subheader top values and the li scrollMarginTop; alternatively measure
the actual category header height in useEffect (querying the category header
element in PrebuiltSearchList) and set the CSS variable on the component root so
both the category (styles.subheader) and subheader (styles.subheader) top
positions and the scroll target use the same computed offset to avoid drift when
typography/padding changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 99c1b804-47ef-4130-86a9-586cfd687a69
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.tsx
catsiller
commented
Apr 20, 2026
| const groupedQueries = groupBy(listSections, 'category'); | ||
|
|
||
| const testMatch = (name: string, id?: number) => { | ||
| const isSelectedQuery = (name: string, id?: number) => { |
Contributor
Author
There was a problem hiding this comment.
Updated the const name to be a little bit more descriptive of what its doing
catsiller
commented
Apr 20, 2026
| if (itemRef.current) { | ||
| itemRef.current.scrollIntoView({ behavior: 'smooth', block: 'start' }); | ||
| } | ||
| const scrollSelectedItemIntoView = (e: HTMLLIElement | null) => { |
Contributor
Author
There was a problem hiding this comment.
I know we have been trying not to use useEffect so was able to achieve same goals with const scrollSelectedItemIntoView
please test locally that when item (query) is clicked, item automatically scrolls to the top of list (with offset pixels)
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
1 participant
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.
Description
In Saved Queries under Cypher tab on Explore page, category and sub-category name are not repeated within each individual query entry
The sub-category is displayed once as a header and all queries belonging to that sub-category are grouped beneath it
Given queries are grouped under category or sub-category headers, queries are clearly associated with their parent category
On scroll down, category and subcategory are displayed at top of each section
On query click (if query is not top of the section), selected query scrolls to top of list (under category and subcategory)
Before:
After:
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-7541
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Manually tested in both CE and Enterprise
Please test locally that:
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Improvements
Accessibility