Fix single perspective icon and text in NavHeader#16663
Open
vojtechszocs wants to merge 1 commit into
Open
Conversation
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:
WalkthroughPerspective override handling was centralized into a shared utility and consumed by frontend hooks, the perspective state provider, and NavHeader. Related tests now mock the shared override data directly, and server logging reports parsed perspective overrides. ChangesPerspective override flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/core
![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: 1
🤖 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 `@frontend/packages/console-shared/src/hooks/usePerspectives.ts`:
- Around line 42-52: The `overridePerspectives` export in the IIFE is casting
the parsed JSON to `Perspective[]` without validating that the parsed result is
actually an array. This means valid JSON like `{}` will pass parsing and the
type assertion, but crash downstream when array methods like `.find()` are
called. Add runtime validation to check that the parsed result from
`JSON.parse(window.SERVER_FLAGS.perspectives)` is actually an array (using
`Array.isArray()`) before returning it, and return undefined if the validation
fails, following the allow-list validation approach at this trust boundary.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ded9900b-8754-4070-b09a-624a162dfe60
📒 Files selected for processing (6)
frontend/@types/console/window.d.tsfrontend/packages/console-app/src/actions/providers/perspective-state-provider.tsfrontend/packages/console-app/src/components/nav/NavHeader.tsxfrontend/packages/console-shared/src/hooks/usePerspectives.tsfrontend/packages/console-shared/src/hooks/usePinnedResources.tspkg/server/server.go
vojtechszocs
commented
Jun 22, 2026
| () => | ||
| perspectiveExtensions.find((p) => p?.properties?.id === activePerspective)?.properties ?? | ||
| perspectiveExtensions[0]?.properties ?? { icon: null, name: null }, | ||
| perspectiveExtensions[0]?.properties ?? { icon: null, name: '-' }, |
Contributor
Author
There was a problem hiding this comment.
Note that name is mandatory within console.perspective extension's properties.
The fallback "-" text could be replaced with null to not render any text, if we have such preference.
vojtechszocs
force-pushed
the
fix-single-perspective
branch
from
9ca7d94 to
f1298a6
Compare
openshift-ci
Bot
added
needs-rebase
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
frontend/packages/console-app/src/components/detect-context/PerspectiveConfiguration.tsx (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a concrete SDK module path over root
srcimport.For new code, use a direct module file path for this type import instead of
@console/dynamic-plugin-sdk/src.As per coding guidelines, "
frontend/**/*.{ts,tsx,js,jsx}: Never import from package index files ... import from specific file paths instead."🤖 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 `@frontend/packages/console-app/src/components/detect-context/PerspectiveConfiguration.tsx` at line 17, The import statement for the Perspective type in PerspectiveConfiguration.tsx is importing from the root `@console/dynamic-plugin-sdk/src` path. Replace this with a direct module file path instead of the `src` root directory. Determine the specific file that exports the Perspective type and update the import statement to reference that concrete module path directly.Source: Coding guidelines
🤖 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
`@frontend/packages/console-app/src/components/detect-context/__tests__/PerspectiveDetector.spec.tsx`:
- Line 10: The mockOverridePerspectives variable in PerspectiveDetector.spec.tsx
has an incorrect type annotation that does not allow undefined values. Update
the type annotation of mockOverridePerspectives from PerspectiveType[] to
PerspectiveType[] | undefined to match the actual return type of the
overridePerspectives export and to support test scenarios where undefined is
explicitly assigned to this variable.
In
`@frontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsx`:
- Line 9: The variable mockOverridePerspectives is declared with type
Perspective[] but is reset to undefined in the afterEach cleanup blocks, so the
type declaration does not match the actual possible values. Update the type
annotation of mockOverridePerspectives from Perspective[] to Perspective[] |
undefined to accurately reflect that it can be either an array of Perspective
objects or undefined.
In
`@frontend/packages/console-shared/src/hooks/__tests__/usePerspectives.spec.ts`:
- Line 10: The type annotation for the variable mockOverridePerspectives is
declared as Perspective[] but the tests assign undefined to it in multiple
places, causing a type mismatch. Update the type declaration of
mockOverridePerspectives to be Perspective[] | undefined to correctly reflect
that it can hold either an array of Perspective objects or an undefined value.
In
`@frontend/packages/console-shared/src/hooks/__tests__/usePinnedResources.spec.ts`:
- Line 18: The variable mockOverridePerspectives is currently typed as
Perspective[] but should be typed as Perspective[] | undefined to accurately
reflect that it can be assigned undefined (as shown on line 77 and matching the
behavior of the actual overridePerspectives export). Update the type annotation
of mockOverridePerspectives from Perspective[] to Perspective[] | undefined.
In `@frontend/packages/console-shared/src/utils/override-perspectives.ts`:
- Around line 36-53: The function getOverridePerspectives declares a return type
of Perspective[] but its control flow shows it can return undefined in two
scenarios: when window.SERVER_FLAGS.perspectives is not set and when the
try-catch block fails to parse the perspectives. Update the return type
annotation of the getOverridePerspectives function from Perspective[] to
Perspective[] | undefined to accurately reflect the actual return possibilities
and align the TypeScript signature with the JSDoc documentation.
- Line 1: In the import statement at the top of override-perspectives.ts, change
the import source for AccessReviewResourceAttributes from the package root
import `@console/dynamic-plugin-sdk` to the direct file path
`@console/dynamic-plugin-sdk/src/extensions/console-types`. This is a direct
modification to the single import line that imports the
AccessReviewResourceAttributes type, replacing the short import path with the
full file path to the concrete module.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/detect-context/PerspectiveConfiguration.tsx`:
- Line 17: The import statement for the Perspective type in
PerspectiveConfiguration.tsx is importing from the root
`@console/dynamic-plugin-sdk/src` path. Replace this with a direct module file
path instead of the `src` root directory. Determine the specific file that
exports the Perspective type and update the import statement to reference that
concrete module path directly.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1fe580db-b85b-4ce5-bdde-8450f7a379a9
📒 Files selected for processing (13)
frontend/@types/console/window.d.tsfrontend/packages/console-app/src/actions/providers/perspective-state-provider.tsfrontend/packages/console-app/src/components/detect-context/PerspectiveConfiguration.tsxfrontend/packages/console-app/src/components/detect-context/__tests__/PerspectiveDetector.spec.tsxfrontend/packages/console-app/src/components/nav/NavHeader.tsxfrontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsxfrontend/packages/console-shared/src/hooks/__tests__/usePerspectives.spec.tsfrontend/packages/console-shared/src/hooks/__tests__/usePinnedResources.spec.tsfrontend/packages/console-shared/src/hooks/usePerspectives.tsfrontend/packages/console-shared/src/hooks/usePinnedResources.tsfrontend/packages/console-shared/src/utils/override-perspectives.tsfrontend/packages/dev-console/src/components/catalog/PinnedResourcesConfiguration.tsxpkg/server/server.go
✅ Files skipped from review due to trivial changes (1)
- frontend/@types/console/window.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/server/server.go
- frontend/packages/console-app/src/actions/providers/perspective-state-provider.ts
- frontend/packages/console-shared/src/hooks/usePinnedResources.ts
- frontend/packages/console-app/src/components/nav/NavHeader.tsx
| import NavHeader from '../NavHeader'; | ||
| import { renderWithPerspective } from './navTestUtils'; | ||
|
|
||
| let mockOverridePerspectives: Perspective[]; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the type of mockOverridePerspectives to allow undefined.
The variable is typed as Perspective[] but should be Perspective[] | undefined to match the actual return type of overridePerspectives. The afterEach blocks at lines 110 and 130 explicitly reset this variable to undefined.
🔧 Proposed fix
-let mockOverridePerspectives: Perspective[];
+let mockOverridePerspectives: Perspective[] | undefined;📝 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
| let mockOverridePerspectives: Perspective[]; | |
| let mockOverridePerspectives: Perspective[] | undefined; |
🤖 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
`@frontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsx`
at line 9, The variable mockOverridePerspectives is declared with type
Perspective[] but is reset to undefined in the afterEach cleanup blocks, so the
type declaration does not match the actual possible values. Update the type
annotation of mockOverridePerspectives from Perspective[] to Perspective[] |
undefined to accurately reflect that it can be either an array of Perspective
objects or undefined.
|
|
||
| const useExtensionsMock = useExtensions as jest.Mock; | ||
|
|
||
| let mockOverridePerspectives: Perspective[]; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the type of mockOverridePerspectives to allow undefined.
The variable is typed as Perspective[] but tests explicitly assign undefined to it (lines 27, 58, 90). The type should be Perspective[] | undefined to match the actual return type of overridePerspectives.
🔧 Proposed fix
-let mockOverridePerspectives: Perspective[];
+let mockOverridePerspectives: Perspective[] | undefined;🤖 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 `@frontend/packages/console-shared/src/hooks/__tests__/usePerspectives.spec.ts`
at line 10, The type annotation for the variable mockOverridePerspectives is
declared as Perspective[] but the tests assign undefined to it in multiple
places, causing a type mismatch. Update the type declaration of
mockOverridePerspectives to be Perspective[] | undefined to correctly reflect
that it can hold either an array of Perspective objects or an undefined value.
| const useModelFinderMock = useModelFinder as jest.Mock; | ||
| const setPinnedResourcesMock = jest.fn(); | ||
|
|
||
| let mockOverridePerspectives: Perspective[]; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the type of mockOverridePerspectives to allow undefined.
The variable is typed as Perspective[] but should be Perspective[] | undefined. Line 77 explicitly assigns undefined to this variable, and the actual overridePerspectives export can return undefined when no overrides are configured.
🔧 Proposed fix
-let mockOverridePerspectives: Perspective[];
+let mockOverridePerspectives: Perspective[] | undefined;📝 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
| let mockOverridePerspectives: Perspective[]; | |
| let mockOverridePerspectives: Perspective[] | undefined; |
🤖 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
`@frontend/packages/console-shared/src/hooks/__tests__/usePinnedResources.spec.ts`
at line 18, The variable mockOverridePerspectives is currently typed as
Perspective[] but should be typed as Perspective[] | undefined to accurately
reflect that it can be assigned undefined (as shown on line 77 and matching the
behavior of the actual overridePerspectives export). Update the type annotation
of mockOverridePerspectives from Perspective[] to Perspective[] | undefined.
| @@ -0,0 +1,56 @@ | |||
| import type { AccessReviewResourceAttributes } from '@console/dynamic-plugin-sdk'; | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the concrete export location for AccessReviewResourceAttributes
rg -n "export (type|interface) AccessReviewResourceAttributes" frontend/packages/console-dynamic-plugin-sdk/srcRepository: openshift/console
Length of output: 282
🏁 Script executed:
#!/bin/bash
# Check if there's an index file at the SDK root that re-exports AccessReviewResourceAttributes
fd -t f "^index\.(ts|js)$" frontend/packages/console-dynamic-plugin-sdk/src
# Check the main SDK index for re-exports
head -50 frontend/packages/console-dynamic-plugin-sdk/src/index.ts | grep -E "(export|AccessReviewResourceAttributes)"
# Verify the console-types.ts file location and confirm the export
cat -n frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts | sed -n '60,70p'Repository: openshift/console
Length of output: 1376
🏁 Script executed:
#!/bin/bash
# Check the extensions/index.ts to see if it re-exports AccessReviewResourceAttributes
cat frontend/packages/console-dynamic-plugin-sdk/src/extensions/index.ts | grep -E "(AccessReviewResourceAttributes|export.*from)"
# Also verify the import in the file under review
cat -n frontend/packages/console-shared/src/utils/override-perspectives.ts | head -5Repository: openshift/console
Length of output: 1516
🏁 Script executed:
#!/bin/bash
# Check how other files import from console-dynamic-plugin-sdk to see the pattern
rg -n "import.*AccessReviewResourceAttributes" frontend/packages/console-shared/src --type ts
# Also check what import patterns are used in the console codebase for SDK imports
rg "import.*from '`@console/dynamic-plugin-sdk/src`" frontend/packages/console-shared/src --type ts -A 0 | head -10Repository: openshift/console
Length of output: 1800
Use a direct SDK file import instead of a package-root import.
Import AccessReviewResourceAttributes from the concrete file path @console/dynamic-plugin-sdk/src/extensions/console-types instead of the package root. This avoids potential circular dependencies and aligns with the codebase pattern already established in other files within console-shared.
Change:
import type { AccessReviewResourceAttributes } from '`@console/dynamic-plugin-sdk`';To:
import type { AccessReviewResourceAttributes } from '`@console/dynamic-plugin-sdk/src/extensions/console-types`';🤖 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 `@frontend/packages/console-shared/src/utils/override-perspectives.ts` at line
1, In the import statement at the top of override-perspectives.ts, change the
import source for AccessReviewResourceAttributes from the package root import
`@console/dynamic-plugin-sdk` to the direct file path
`@console/dynamic-plugin-sdk/src/extensions/console-types`. This is a direct
modification to the single import line that imports the
AccessReviewResourceAttributes type, replacing the short import path with the
full file path to the concrete module.
Source: Coding guidelines
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
vojtechszocs
force-pushed
the
fix-single-perspective
branch
from
f1298a6 to
c73c57a
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/packages/console-app/src/components/nav/NavHeader.tsx`:
- Around line 74-80: The NavHeader fallback currently returns null name/icon
when usePerspectives() has no available entries, which makes the header blank.
Update the fallback logic in NavHeader’s useMemo so that if no matching
perspective is found and perspectiveExtensions[0] is missing, it still returns
the explicit built-in label “Core platform” instead of { icon: null, name: null
}, while preserving the existing search for the active perspective.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dbf0eb16-d071-4e00-a4ea-15955d3c4a55
📒 Files selected for processing (13)
frontend/@types/console/window.d.tsfrontend/packages/console-app/src/actions/providers/perspective-state-provider.tsfrontend/packages/console-app/src/components/detect-context/PerspectiveConfiguration.tsxfrontend/packages/console-app/src/components/detect-context/__tests__/PerspectiveDetector.spec.tsxfrontend/packages/console-app/src/components/nav/NavHeader.tsxfrontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsxfrontend/packages/console-shared/src/hooks/__tests__/usePerspectives.spec.tsfrontend/packages/console-shared/src/hooks/__tests__/usePinnedResources.spec.tsfrontend/packages/console-shared/src/hooks/usePerspectives.tsfrontend/packages/console-shared/src/hooks/usePinnedResources.tsfrontend/packages/console-shared/src/utils/override-perspectives.tsfrontend/packages/dev-console/src/components/catalog/PinnedResourcesConfiguration.tsxpkg/server/server.go
✅ Files skipped from review due to trivial changes (1)
- frontend/@types/console/window.d.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/server/server.go
- frontend/packages/console-app/src/components/nav/tests/NavHeader.spec.tsx
- frontend/packages/console-shared/src/utils/override-perspectives.ts
- frontend/packages/console-app/src/components/detect-context/tests/PerspectiveDetector.spec.tsx
- frontend/packages/console-shared/src/hooks/usePinnedResources.ts
- frontend/packages/console-shared/src/hooks/tests/usePerspectives.spec.ts
- frontend/packages/dev-console/src/components/catalog/PinnedResourcesConfiguration.tsx
- frontend/packages/console-app/src/actions/providers/perspective-state-provider.ts
- frontend/packages/console-app/src/components/detect-context/PerspectiveConfiguration.tsx
- frontend/packages/console-shared/src/hooks/usePerspectives.ts
- frontend/packages/console-shared/src/hooks/tests/usePinnedResources.spec.ts
vojtechszocs
force-pushed
the
fix-single-perspective
branch
from
c73c57a to
9c3a425
Compare
vojtechszocs
force-pushed
the
fix-single-perspective
branch
from
9c3a425 to
1c1a862
Compare
Contributor
|
@vojtechszocs: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Analysis / Root cause
Assuming there is just one perspective available
NavHeaderrenders "Core platform" perspective icon and text (regardless of the available perspective)NavHeaderrenders the actual available perspective icon and text (using "Core platform" as a fallback)Solution description
window.SERVER_FLAGS.perspectivesNavHeaderto render actual available perspective icon and textpkg/server/server.goto log perspective overrides (if provided)Screenshots
Test setup
localhost:9001npm install && npm run devlocalhost:9000using following parameters./bin/bridge -plugins kubevirt-plugin=http://localhost:9001 -i18n-namespaces=plugin__kubevirt-plugin \ -perspectives '[{ "id" : "admin", "visibility": {"state" : "Disabled" }}, { "id" : "dev", "visibility": {"state" : "Disabled" }}]' \ -branding openshiftIn this setup, both "Core platform" (
admin) and "Developer" (dev) perspectives are disabled.Test cases
Open
localhost:9000in web browser and check the nav headerconsole.perspectiveextension comes into effect]console.perspectiveextension comes into effect]Summary by CodeRabbit