Fix Access Management path in settings dropdown#3443
Open
jjaquish wants to merge 3 commits intoRedHatInsights:masterfrom
Open
Fix Access Management path in settings dropdown#3443jjaquish wants to merge 3 commits intoRedHatInsights:masterfrom
jjaquish wants to merge 3 commits intoRedHatInsights:masterfrom
Conversation
jjaquish
requested review from
Hyperkid123,
florkbr and
karelhala
as code owners
|
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:
WalkthroughOrg-admin IAM routing in Tools now chooses between Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Header/HeaderTests/Tools.test.js (1)
123-138: Consider extracting a shared auth-context fixture for these tests.The three
mockAuthContextblocks are mostly identical; centralizing them will reduce maintenance cost and keep route-specific intent clearer.Also applies to: 158-173, 193-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Header/HeaderTests/Tools.test.js` around lines 123 - 138, Multiple tests define nearly identical mockAuthContext objects; extract a shared fixture (e.g., export const mockAuthContext or function createMockAuthContext) at the top of Tools.test.js or in a test-utils file and import/use it in each test to avoid duplication; update the three locations that currently declare mockAuthContext to instead call the shared factory or reference the exported constant, and adjust any test-specific overrides by cloning or spreading the shared object (e.g., { ...mockAuthContext, token: 'override' }) so route-specific intent remains clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Header/HeaderTests/Tools.test.js`:
- Around line 80-98: Replace all uses of container.querySelector in the Tools
tests with React Testing Library queries (use screen.getByTestId for the
provided test IDs) to make assertions more robust: in the renderTools helper
keep returning the rendered container but update each assertion that selects
elements by ID/attributes to use screen.getByTestId('settings-toggle') for the
settings button and screen.getByTestId('UserAccess') for the IAM link (also
update the similar selectors at the other occurrences noted around the other
test blocks), and remove direct DOM query usage in favor of screen.* queries so
tests target the elements by their data-testid attributes when interacting with
the Tools component.
---
Nitpick comments:
In `@src/components/Header/HeaderTests/Tools.test.js`:
- Around line 123-138: Multiple tests define nearly identical mockAuthContext
objects; extract a shared fixture (e.g., export const mockAuthContext or
function createMockAuthContext) at the top of Tools.test.js or in a test-utils
file and import/use it in each test to avoid duplication; update the three
locations that currently declare mockAuthContext to instead call the shared
factory or reference the exported constant, and adjust any test-specific
overrides by cloning or spreading the shared object (e.g., { ...mockAuthContext,
token: 'override' }) so route-specific intent remains clear.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5f742cb-6e81-47ef-bcaa-b14edb56ea20
⛔ Files ignored due to path filters (1)
src/components/Header/HeaderTests/__snapshots__/Tools.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/components/Header/HeaderTests/Tools.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/Header/HeaderTests/Tools.test.js (1)
20-27: Make theSettingsTogglemock respectisHiddenitems.Rendering hidden items in the mock can mask real-world behavior and produce false-positive assertions.
Mock fidelity improvement
- {props.dropdownItems.map((group, groupIndex) => ( + {props.dropdownItems.map((group, groupIndex) => ( <div key={groupIndex}> - {group.items.map((item, itemIndex) => ( + {group.items + .filter((item) => !item.isHidden) + .map((item, itemIndex) => ( <a key={itemIndex} href={item.url} data-testid={item.ouiaId}> {item.title} </a> - ))} + ))} </div> ))}As per coding guidelines, "
**/*.test.{ts,tsx,js}: Check for proper mocking and test setup."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Header/HeaderTests/Tools.test.js` around lines 20 - 27, The SettingsToggle test mock currently renders every item from props.dropdownItems and ignores the isHidden flag, causing hidden items to appear in tests; update the mock used in Tools.test.js (the SettingsToggle mock) so that when rendering group.items it first filters out items with item.isHidden (e.g. replace direct group.items.map(...) with group.items.filter(item => !item.isHidden).map(...)) so the mock behavior matches real component behavior and prevents false-positive assertions.src/hooks/useBreadcrumbsLinks.test.tsx (1)
74-108: Tighten fallback-case assertions to catch structural regressions.This case currently allows extra/incorrect segments while still passing. If the intended behavior is bundle + active item only, assert the exact array shape.
Suggested test hardening
- expect(result.current.length).toBeGreaterThanOrEqual(1); - expect(result.current.some((s) => s.title === 'Advisor')).toBe(true); + expect(result.current).toEqual([ + { + title: 'RHEL', + href: '/insights', + }, + { + title: 'Advisor', + href: '/insights/advisor', + active: true, + }, + ]);As per coding guidelines, "
**/*.test.{ts,tsx,js}: Verify test quality and coverage."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useBreadcrumbsLinks.test.tsx` around lines 74 - 108, Tighten the test "should fall back to bundle href when no route match fragments exist" to assert the exact breadcrumb array shape instead of loose checks: when using createWrapper('/insights/advisor', [], navigation) and rendering useBreadcrumbsLinks(), expect result.current to strictly equal an array of two segments — first the bundle segment ({ title: 'RHEL', href: '/insights' }) and second the active item segment ({ title: 'Advisor', href: '/insights/advisor' }) — and assert length === 2; update the assertions to compare the full array (or deepEqual) rather than only presence/length checks to catch structural regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/Header/HeaderTests/Tools.test.js`:
- Around line 20-27: The SettingsToggle test mock currently renders every item
from props.dropdownItems and ignores the isHidden flag, causing hidden items to
appear in tests; update the mock used in Tools.test.js (the SettingsToggle mock)
so that when rendering group.items it first filters out items with item.isHidden
(e.g. replace direct group.items.map(...) with group.items.filter(item =>
!item.isHidden).map(...)) so the mock behavior matches real component behavior
and prevents false-positive assertions.
In `@src/hooks/useBreadcrumbsLinks.test.tsx`:
- Around line 74-108: Tighten the test "should fall back to bundle href when no
route match fragments exist" to assert the exact breadcrumb array shape instead
of loose checks: when using createWrapper('/insights/advisor', [], navigation)
and rendering useBreadcrumbsLinks(), expect result.current to strictly equal an
array of two segments — first the bundle segment ({ title: 'RHEL', href:
'/insights' }) and second the active item segment ({ title: 'Advisor', href:
'/insights/advisor' }) — and assert length === 2; update the assertions to
compare the full array (or deepEqual) rather than only presence/length checks to
catch structural regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11ddb705-1723-4432-b614-4e7bafdc423a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/components/Header/HeaderTests/Tools.test.jssrc/hooks/useBreadcrumbsLinks.test.tsxsrc/hooks/useBreadcrumbsLinks.ts
karelhala
approved these changes
Apr 10, 2026
Contributor
karelhala
left a comment
karelhala
left a comment
There was a problem hiding this comment.
Looking good, can you please resolve the merge conflicts? Thank you.
jjaquish
force-pushed
the
jjaquish/RHCLOUD-46650
branch
from
7d888d3 to
76848a5
Compare
Author
|
@karelhala Thanks for looking, should be good now! |
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
2 participants
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.
The current path is to /iam/access-management/overview for the Access Management item in the settings dropdown for a user with workspaces enabled. In RBAC V2 this path doesn't exist, and directs the user to their my access page instead. It should be /iam/overview for workspaces org admins
Summary by CodeRabbit
Bug Fixes
Tests
Chores