feat(search): merged layout in search index#38
Conversation
OME-157 SearchProvider now uses the merged layout (backend + user override) instead of the raw backend layout. Custom groups appear as "nav-group" category entries with their description, keywords, and icon indexed by Orama. Hidden "more" children remain searchable. Search index rebuilds when the nav layout override changes. AppNavGroup renders custom group icons via dynamic import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds slug-to-name icon conversion, updates search indexing to merge override navigation layouts and resolve custom group icons, and changes search rendering to lazy-load icons, render nested trees recursively, and support a new ChangesSearch icon and indexing flow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
| * Convert a kebab-case icon slug to PascalCase icon component name. | ||
| * E.g. "folder-open" → "FolderOpenIcon", "cog" → "CogIcon" | ||
| */ | ||
| export function iconSlugToName(slug: string): string { |
There was a problem hiding this comment.
I believe this function already exists somewhere. In the nav editor. Can we deduplicate? Are there any duplications introduced?
There was a problem hiding this comment.
Good catch — I searched the codebase and iconSlugToName is actually new (no existing duplicate). The closest related functions are iconNameToFile and iconNameToKeywords in the same file, but they go the opposite direction (PascalCase → kebab/keywords). The nav editor (GroupFormModal, IconGalleryModal) works with PascalCase names directly.
Since the function is already in packages/common/src/pfIconLoader.ts, it's available as a shared utility across all packages.
| setGroupIcon(() => cached); | ||
| return; | ||
| } | ||
| loadPfIcon(iconName).then((comp) => { |
There was a problem hiding this comment.
Can we make the loading function stand alone? Not within a component file. Or even better, could we have a DynamicIcon component that handles the loading internally? And used in all the places that require the custom icons?
There was a problem hiding this comment.
Great suggestion! I've extracted a standalone DynamicPfIcon component in packages/common/src/DynamicPfIcon.tsx that handles async loading internally. AppNavGroup now uses it instead of the inline useState/useEffect pattern.
GroupIconPicker in the settings plugin has a similar loading pattern and could also be migrated to use DynamicPfIcon — happy to do that in a follow-up if you'd like.
| cb: (entry: CommonNavLayoutEntry) => void, | ||
| ): void { | ||
| for (const entry of entries) { | ||
| if (entry.type === "more") { |
There was a problem hiding this comment.
I think we have an enum for this, right? For these magic strings
There was a problem hiding this comment.
The existing `NodeKind` enum covers FlatNode kinds (`Page`, `Group`, `Section`) but not `NavLayoutEntry.type` values — `"more"` specifically isn't in `NodeKind` because the flatten step strips it. The `NavLayoutEntry` discriminated union enforces valid type values at compile time, and the literal string checks are consistent with how `navLayout.ts` uses them throughout (lines 388, 409, 485, 648).
If you'd prefer entry-type constants for readability, that would be a broader refactor across `navLayout.ts` — happy to tackle it as a separate PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gui/src/components/Search/SearchProvider.tsx`:
- Around line 418-428: The async custom icon load in SearchProvider’s result
rendering can still write into iconMapRef after a newer override-triggered index
rebuild, causing stale icons to overwrite the current group icon state. Update
the icon-loading path around loadPfIcon and the index rebuild logic in
SearchProvider to clear per-index icon maps when rebuilding and to ignore late
Promise completions from older layouts, using the existing
entryId/group-${groupId} mapping to verify the completion is still current
before calling iconMapRef.current.set.
- Around line 430-436: Keep injected custom-group parents in the nav-group
bucket instead of letting query() prepend them into the child result’s category
bucket. Update the parent-injection logic in SearchProvider.tsx, around
featureParentRef.current.set and the groupFeatureId handling, so custom-group
parents retain their original nav-group category and still flow through the
dynamic icon map rather than being treated as nav.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: b50141a5-bb2b-4d73-a478-3c6a13b66f6f
📒 Files selected for processing (7)
packages/common/src/__tests__/pfIconLoader.test.tspackages/common/src/index.tspackages/common/src/pfIconLoader.tspackages/gui/src/components/AppNav/AppNavGroup.tsxpackages/gui/src/components/Search/FleetSearch.tsxpackages/gui/src/components/Search/SearchProvider.tsxpackages/gui/src/components/Search/searchIndex.ts
… icons OME-157 - Add DynamicPfIcon component in common for reusable async icon loading - Use DynamicPfIcon in AppNavGroup instead of inline useState/useEffect - Guard SearchProvider icon loads against stale rebuilds (cancelled flag) - Clear component/icon/parent maps on index rebuild - Fix parent injection: use parent.category to keep nav-group parents in correct bucket, attach IconComponent from iconMapRef - Deduplicate parent entries in query results Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OME-157
ComponentType defaults to ComponentType<{}> which rejects className.
Introduce PfIconComponent type alias with { className?: string } and
cast loader results to satisfy strict overload checks.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest |
|
@platex-rehor-bot I have found an issue, the icon, custom icon that is, is not surfaced in the search result. The new structure of custom groups is also not reflected in the search. Meaning the search index does not appear to be rebuilt when a navigation layout changes. In our nav editor. |
OME-157 Fix three issues with search not reflecting nav editor changes: 1. Group parent-child tree rendering broken — toFeatureId stripped "group-" prefix but children use "group." dot notation, so groups were never matched as parents. Fix: map "group-X" → "group.X". 2. Page iconOverride from nav editor ignored — search always showed CubesIcon. Now reads iconOverride from merged layout and loads the overridden PF icon component dynamically. 3. Module extensions in custom groups without their own icon fell back to SearchIcon. Now inherit the parent custom group's icon. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@Hyperkid123 Good catch! I found three bugs in the search indexing that caused these issues:
All three are fixed in commit 19f50a0. Lint clean, 238 tests pass. |
- Custom nav groups now nest pages recursively in search results (Foobar → Clusters → Create cluster → Kind/GCP) instead of showing them as a flat "Custom groups" category. - Renderer supports multi-level parent-child trees via recursive renderTree, replacing the previous single-level parent/children split. - Guide lines use PF TreeView standards: fixed 1.125rem offset instead of 50%, proper last-child height, divider-width tokens. - Remove stray console.log from FleetSearch. - Remove unused "nav-group" category — custom groups stay under "Pages" with deeper nesting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Search results now reflect custom nav group structuresWhat changed:
Files:
|
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/gui/src/components/Search/SearchProvider.tsx (1)
353-361: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the injected parent chain for nested results.
When only a nested child matches, the injected module parent loses
feature: groupFeature, and the insertion loop does not enqueue that parent’s group ancestor. The recursive renderer then shows the module as a root instead of under its custom group.Proposed fix
featureParentRef.current.set(globalFeatureId, { id: entryId, title: label, description: description ?? `Navigate to ${label}`, category: "nav", pathname: basePath, - icon: "", + icon: resolvedIconName, status: moduleStatus, + feature: groupFeature, });- for (const [, items] of Object.entries(results)) { - const resultIds = new Set(items.map((i) => i.id)); - const needed = new Set<string>(); + for (const items of Object.values(results)) { + const needed: string[] = []; + const seen = new Set<string>(); for (const item of items) { - if (!item.feature) continue; - const parent = featureParentRef.current.get(item.feature); - if (parent && !resultIds.has(parent.id)) { - needed.add(item.feature); - } + if (item.feature) needed.push(item.feature); } - for (const featureId of needed) { + while (needed.length > 0) { + const featureId = needed.shift(); + if (!featureId || seen.has(featureId)) continue; + seen.add(featureId); const parent = featureParentRef.current.get(featureId); if (!parent) continue; const targetCat = parent.category; const targetItems = results[targetCat] ?? []; if (targetItems.some((i) => i.id === parent.id)) continue; const parentItem = { ...parent }; const comp = componentMapRef.current.get(parentItem.id); if (comp) parentItem.Component = comp; const icon = iconMapRef.current.get(parentItem.id); if (icon) parentItem.IconComponent = icon; results[targetCat] = [parentItem, ...targetItems]; + if (parentItem.feature) needed.push(parentItem.feature); } }Also applies to: 522-543
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gui/src/components/Search/SearchProvider.tsx` around lines 353 - 361, The nested search result injection is dropping the custom parent chain, so a matched child can lose its `feature: groupFeature` parent and the group ancestor is never enqueued. Update the parent insertion logic in SearchProvider’s nested result handling and the recursive renderer helpers so injected module parents preserve their existing parent metadata and continue to enqueue the custom group ancestor before rendering. Keep the fix focused around the nested result builder and insertion loop that creates the `featureParentRef` entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gui/src/components/Search/FleetSearch.tsx`:
- Around line 47-55: The lazy icon component in FleetSearch is being frozen by
useState, so it does not update when name or fallback changes. Replace the
useState-based initialization of IconC with useMemo so the lazy component is
recomputed whenever name changes, and keep fallback as part of the memo
dependencies. Update the logic around loadPfIcon and the F fallback in
FleetSearch to preserve the same behavior while ensuring overrides render the
current icon.
---
Outside diff comments:
In `@packages/gui/src/components/Search/SearchProvider.tsx`:
- Around line 353-361: The nested search result injection is dropping the custom
parent chain, so a matched child can lose its `feature: groupFeature` parent and
the group ancestor is never enqueued. Update the parent insertion logic in
SearchProvider’s nested result handling and the recursive renderer helpers so
injected module parents preserve their existing parent metadata and continue to
enqueue the custom group ancestor before rendering. Keep the fix focused around
the nested result builder and insertion loop that creates the `featureParentRef`
entries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 3ddb5009-0435-4c05-b5de-857f18048970
📒 Files selected for processing (3)
packages/gui/src/components/Search/FleetSearch.scsspackages/gui/src/components/Search/FleetSearch.tsxpackages/gui/src/components/Search/SearchProvider.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/gui/src/components/Search/FleetSearch.scss
OME-157 Injected module parents now carry feature and resolvedIconName so the recursive parent-injection loop walks up to the group ancestor. Switched from a flat Set iteration to a queue-based walk that enqueues each parent's own feature, ensuring multi-level nesting (e.g. Group → Module → Create cluster) surfaces the full tree in results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OME-157 LazyIcon used useState initializer which freezes the lazy component on first render — icon overrides never update. Switch to useMemo keyed on name+Fallback so the lazy component re-creates when the icon name changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed both CodeRabbit findings in 34249e7 + ee5c3f7:
|
2 participants
Summary
OME-157 — Phase 6: Search + AppNav updates
nav-groupcategory entries in search with their description, keywords, and icon indexed by Orama for better matchingforEachEntryhelper recursively walks layout entries including "more" childrenloadPfIcon+getCachedPfIcon)iconSlugToNameutility converts kebab-case icon slugs (e.g.folder-open) to PascalCase icon component names (e.g.FolderOpenIcon)Changes
packages/common/src/pfIconLoader.tsiconSlugToNamehelperpackages/common/src/index.tsiconSlugToNamepackages/gui/src/components/Search/SearchProvider.tsxpackages/gui/src/components/Search/searchIndex.tsnav-grouptoSearchCategorytypepackages/gui/src/components/Search/FleetSearch.tsxpackages/gui/src/components/AppNav/AppNavGroup.tsxpackages/common/src/__tests__/pfIconLoader.test.tsTest plan
pfIconLoader.test.tstests pass (iconSlugToName roundtrips, edge cases)🤖 Generated with Claude Code