[ACM-29130] RBAC UI - Creation wizard - project validation#5748
Open
Ginxo wants to merge 27 commits intostolostron:mainfrom
Open
[ACM-29130] RBAC UI - Creation wizard - project validation#5748Ginxo wants to merge 27 commits intostolostron:mainfrom
Ginxo wants to merge 27 commits intostolostron:mainfrom
Conversation
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ginxo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
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:
📝 WalkthroughWalkthroughAdds i18n keys for missing-project workflows; narrows RoleAssignmentStatus.reason to specific literals; introduces useClusterNamespaceMap and useRoleAssignmentsStatusHook to detect/create missing projects with progress alerts; refactors status UI to accept roleAssignment with per-reason callbacks; extends alert/toast API to support id-returning addAlert and modifyAlert. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as RoleAssignments UI
participant Hook as useRoleAssignmentsStatusHook
participant ClusterMap as useClusterNamespaceMap
participant Toast as AcmToast
participant MCA as fireManagedClusterActionCreate
UI->>Hook: mount / request callbacksPerReasonMap
Hook->>ClusterMap: request clusterNamespaceMap
ClusterMap-->>Hook: return clusterNamespaceMap
alt MissingNamespaces detected
Hook->>Toast: addAlert("Creating missing projects", progress=0)
loop per cluster/namespace
Hook->>MCA: fireManagedClusterActionCreate(ProjectRequest)
MCA-->>Hook: success / failure
Hook->>Toast: modifyAlert(update progress)
end
Hook->>Toast: modifyAlert(completion summary)
else No missing namespaces
Hook->>Toast: addAlert("No missing projects")
end
Hook-->>UI: expose callbacksPerReasonMap, isProcessingMap
UI->>UI: render RoleAssignmentStatusComponent(roleAssignment, callbacks, processing)
sequenceDiagram
participant Component as RoleAssignmentStatusComponent
participant Props as Props (roleAssignment, callbacksPerReasonMap)
participant Callback as callbacksPerReasonMap[reason]
participant Toast as AcmToast
Component->>Props: inspect roleAssignment.status.reason
alt reason ∈ {MissingNamespaces, ApplicationFailed}
Component->>Callback: invoke handler(roleAssignment)
Callback->>Toast: addAlert / modifyAlert (returns AcmAlertInfoWithId)
Toast-->>Callback: updated alert info
Component->>Component: show spinner if isCallbackProcessing
else other reasons
Component->>Component: render reason string/footer
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Ginxo
commented
Feb 25, 2026
| return newAlertInfos | ||
| }) | ||
| }, []) | ||
| const modifyAlert = useCallback<(alertInfo: AcmAlertInfoWithId) => AcmAlertInfoWithId>( |
Contributor
Author
There was a problem hiding this comment.
there is no way I can have a kind of progress component alert thing, so I had to adapt AcmToastContext in order to be able to rerender specific alert based on progress information.
The rest of the code remains the same and the whole application is not affected by this apart from different mocking values from 4 different test files
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/wizards/RoleAssignment/CommonProjectCreateProgressBar.tsx (1)
37-37:⚠️ Potential issue | 🟡 MinorDivision by zero when
totalCountis 0.If
totalCountis0,progressValueevaluates toNaN. While callers currently guard against this, the component itself should be defensive.🛡️ Proposed fix
- const progressValue = ((successCount + errorCount) / totalCount) * 100 + const progressValue = totalCount > 0 ? ((successCount + errorCount) / totalCount) * 100 : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/wizards/RoleAssignment/CommonProjectCreateProgressBar.tsx` at line 37, The computation of progressValue in CommonProjectCreateProgressBar can produce NaN when totalCount is 0; update the progressValue calculation (using successCount, errorCount, totalCount) to defensively handle totalCount === 0 (e.g., set progressValue to 0 when totalCount is 0) and clamp the result to the 0–100 range so the component never renders NaN or out-of-range values.
♻️ Duplicate comments (2)
frontend/src/routes/UserManagement/RoleAssignments/hook/RoleAssignmentDataHook.test.tsx (1)
2-2: Same@testing-library/react-hooks/ React 18 incompatibility.Migrate to
import { renderHook } from '@testing-library/react'as noted for the other hook test files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignments/hook/RoleAssignmentDataHook.test.tsx` at line 2, The test imports renderHook from the deprecated package causing React 18 incompatibility; update the import to use renderHook from '@testing-library/react' instead of '@testing-library/react-hooks' in RoleAssignmentDataHook.test.tsx and ensure any references to renderHook in this file (e.g., calls that wrap hook invocation for RoleAssignmentDataHook) continue to work with the new import (adjust usage to the RTL renderHook API if necessary).frontend/src/utils/useClusterNamespaceMap.test.ts (1)
3-3: Same@testing-library/react-hooks/ React 18 incompatibility as the other hook test files.See comment in
useRoleAssignmentsStatusHook.test.tsx(Line 4) — migrate toimport { renderHook } from '@testing-library/react'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/useClusterNamespaceMap.test.ts` at line 3, Replace the incompatible import of renderHook from '@testing-library/react-hooks' with the React 18 compatible import from '@testing-library/react' (update the import statement that currently reads "import { renderHook } from '@testing-library/react-hooks'" to "import { renderHook } from '@testing-library/react'"); ensure existing usages of renderHook in the test remain unchanged and run under the new import.
🧹 Nitpick comments (6)
frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx (1)
50-50:handleMissingNamespacescaptures staleclusterNamespaceMapwithoutuseCallback.Since this async function closes over
clusterNamespaceMapand is stored incallbacksPerReasonMap(returned to consumers), it will always reference the latest render's map but also creates a new function identity every render. Consider wrapping inuseCallbackwith appropriate dependencies to prevent unnecessary re-renders in consuming components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx` at line 50, The async function handleMissingNamespaces closes over clusterNamespaceMap and is recreated every render (causing new identity in callbacksPerReasonMap); wrap handleMissingNamespaces in React.useCallback and include the minimal necessary dependencies (e.g., clusterNamespaceMap and any other referenced variables) so its identity is stable across renders and it still captures up-to-date clusterNamespaceMap; update where callbacksPerReasonMap is built to use the memoized handleMissingNamespaces.frontend/src/routes/UserManagement/RoleAssignment/RoleAssignments.test.tsx (1)
243-243:modifyAlertmock correctly returns the input alert.This matches the production
modifyAlertbehavior (returning the modified alert with its id), unlike other test files that use a barejest.fn(). Consider aligning the mock pattern across test files for consistency, though this is a minor nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignments.test.tsx` at line 243, Tests use a concrete mock for modifyAlert (jest.fn((alert: any) => alert)) while other tests use bare jest.fn(); update the other test files to match this behavior or change this one to a bare mock for consistency—locate the modifyAlert mock in RoleAssignments.test.tsx (the modifyAlert jest.fn) and replace or align it so all tests consistently either return the passed alert (jest.fn((alert)=>alert)) or use a plain jest.fn() across the suite.frontend/src/utils/useClusterNamespaceMap.test.ts (1)
155-177: Missing test case forundefinedns.cluster.The existing test covers
undefinednamespace names but not undefined cluster names. After fixing the production guard inuseClusterNamespaceMap.ts, a test like the following would validate that items with a falsyclusterfield are silently skipped rather than polluting the map with a garbage key.🧪 Suggested additional test
+ it('should skip items with undefined or empty cluster name', () => { + mockUseSearchResultItemsQuery.mockReturnValue({ + data: { + searchResult: [ + { + items: [ + { cluster: 'cluster-1', name: 'valid-ns' }, + { cluster: undefined, name: 'orphan-ns' }, + { cluster: '', name: 'empty-cluster-ns' }, + ], + }, + ], + }, + loading: false, + error: undefined, + } as any) + + const { result } = renderHook(() => useClusterNamespaceMap()) + + expect(result.current.clusterNamespaceMap).toEqual({ + 'cluster-1': ['valid-ns'], + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/useClusterNamespaceMap.test.ts` around lines 155 - 177, Add a test to validate that useClusterNamespaceMap skips items with a falsy cluster value: update the test using mockUseSearchResultItemsQuery to return items where one entry has cluster: undefined (or ''/null) and a valid name, then renderHook(() => useClusterNamespaceMap()) and assert that result.current.clusterNamespaceMap does not contain an undefined/empty key and only includes namespaces grouped under real cluster keys (reference useClusterNamespaceMap and mockUseSearchResultItemsQuery to locate the code to modify).frontend/src/utils/useClusterNamespaceMap.ts (1)
29-30: Redundant optional chaining in the truthy branch.Line 29 already guards that
allNamespacesQuery?.searchResult?.[0]?.items?.lengthis truthy, soitemsis guaranteed non-null inside theifblock. The optional chaining and|| []fallback on line 30 are unreachable.♻️ Proposed simplification
- const items = allNamespacesQuery?.searchResult?.[0]?.items || [] + const items = allNamespacesQuery!.searchResult![0]!.items!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/useClusterNamespaceMap.ts` around lines 29 - 30, Inside useClusterNamespaceMap, the if guard using isLoading and allNamespacesQuery?.searchResult?.[0]?.items?.length guarantees items is non-null, so replace the redundant optional chaining and fallback in the assignment for items: change const items = allNamespacesQuery?.searchResult?.[0]?.items || [] to a direct (non-optional) access such as const items = allNamespacesQuery!.searchResult![0]!.items or const items = allNamespacesQuery.searchResult[0].items (whichever matches your TS strictness), referencing the existing variables isLoading, allNamespacesQuery and items to remove the unreachable || [] fallback.frontend/public/locales/en/translation.json (1)
1487-1490: Consider plural placeholder naming for plural text.Line 1490 uses plural wording (
projects,clusters) with singular placeholder names ({{project}},{{cluster}}). Using plural placeholder names would make intent clearer and reduce interpolation mismatch risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/locales/en/translation.json` around lines 1487 - 1490, The plural translation key "Error creating missing projects {{project}} for clusters {{cluster}}." uses singular placeholders; update the placeholders to plural (e.g., change to "Error creating missing projects {{projects}} for clusters {{clusters}}.") and then update any code that calls this translation key to pass the corresponding plural variables (projects, clusters) instead of project/cluster so interpolation matches the plural intent.frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx (1)
134-142: JSDoc is stale and no longer matches the exported props type.The block still documents
status/callbackProgress, which are not present inRoleAssignmentStatusComponentProps. Please update to avoid API confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 134 - 142, The JSDoc above RoleAssignmentStatusComponent is stale: update the comment to match the exported RoleAssignmentStatusComponentProps by removing references to the nonexistent status and callbackProgress fields (or replace them with the actual prop names/types used in RoleAssignmentStatusComponentProps), ensuring the doc correctly describes callbacksPerReasonMap and any real props; reference the RoleAssignmentStatusComponent and RoleAssignmentStatusComponentProps symbols when editing the JSDoc so the comment and exported type stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/public/locales/en/translation.json`:
- Around line 2118-2120: Replace the awkward message value for the translation
key that currently reads "No missing projects found for {{name}} RoleAssignment.
MultiClusterRoleAssignment resource is reconciling information." with a clearer
phrase such as "No missing projects found for {{name}} RoleAssignment.
MultiClusterRoleAssignment reconciliation is in progress." — edit the value in
frontend/public/locales/en/translation.json for that exact string so the UI
shows the clearer wording.
In `@frontend/src/resources/multicluster-role-assignment.ts`:
- Around line 40-46: The union type for reason in
multicluster-role-assignment.ts is too narrow and excludes actual backend values
seen in mock data (e.g., "permissions don't applied" and "invalid cluster set");
update the type definition (the `reason` union) to include those exact backend
string literals or relax it to a plain string type, and ensure any code paths
using RoleAssignmentStatusComponent / ReasonString continue to handle unknown
values gracefully.
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 173-174: The Spinner components in RoleAssignmentStatusComponent
currently use hardcoded English aria-label strings; replace them with localized
strings by wrapping the aria-label values in the translation function (t(...))
and using appropriate i18n keys (e.g., reuse the key used for label "Creating
common projects" or add new keys like 'creatingCommonProjects.ariaLabel') for
both occurrences (the Spinner with label={t('Creating common projects')} and the
other Spinner at the later occurrence referenced in the review). Ensure
aria-label now reads aria-label={t('...')} so screen-reader text is localized.
- Line 126: The Label component currently hardcodes aria-disabled to true which
incorrectly exposes disabled state to assistive tech; update the Label usage
(Label) to either remove the aria-disabled attribute and rely on
isDisabled={isCallbackProcessing} or bind it to the actual state
(aria-disabled={isCallbackProcessing}) so it reflects the same boolean as
isDisabled, ensuring accessibility state matches isCallbackProcessing.
- Around line 42-47: The onClick handler in RoleAssignmentStatusComponent
currently invokes callback immediately in the true branch and returns a function
in the false branch, so clicks become a no-op when callback is missing and the
button remains enabled; change the handler to always provide a function (e.g.,
onClick={() => callback ? callback(roleAssignment) : console.error(... )}) and
also disable the button when no callback is provided by updating isDisabled to
combine areActionButtonsDisabled with !callback so the button is disabled if
there is no callback.
In
`@frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.test.tsx`:
- Around line 127-150: The test uses a synchronous act to invoke handleMissing
from useRoleAssignmentsStatusHook and then immediately reads
isProcessingRoleAssignmentMap/isAnyRoleAssignmentProcessing, which can be flaky
under React 18 concurrent mode; update the test to invoke handleMissing inside
an awaited act (e.g., await act(async () => { handleMissing!(baseRoleAssignment)
})) and/or follow it with a waitFor that asserts the expected state (checking
isProcessingRoleAssignmentMap['ra-1'] and isAnyRoleAssignmentProcessing) so
state updates from fireManagedClusterActionCreate (mocked by
fireManagedClusterActionCreateMock) are fully committed before assertions run.
- Line 4: Tests import utilities from the incompatible package
'@testing-library/react-hooks'; update the imports in the three test files so
renderHook and act are imported from '@testing-library/react' (and ensure your
project dependency `@testing-library/react` is upgraded to v13+ to support React
18), i.e., replace any "import { renderHook, act } from
'@testing-library/react-hooks'" with the equivalent import from
'@testing-library/react' in
frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.test.tsx,
frontend/src/utils/useClusterNamespaceMap.test.ts, and
frontend/src/routes/UserManagement/RoleAssignments/hook/RoleAssignmentDataHook.test.tsx.
In
`@frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx`:
- Around line 195-205: The translation call in useRoleAssignmentsStatusHook is
passing plural keys (projects, clusters) but the template uses singular
placeholders ({{project}}, {{cluster}}), so update the interpolation to match
the template or (better) update the template placeholders to plural — e.g.,
change the t(...) template to use {{projects}} and {{clusters}} to match
Object.values(callbackProgress.errorClusterNamespacesMap).flat() and
Object.keys(callbackProgress.errorClusterNamespacesMap), or alternatively rename
the interpolation object keys to project and cluster if you prefer singular
placeholders; ensure the keys used in the t(...) call exactly match the
placeholders so the toast message is interpolated correctly.
- Around line 82-141: The Promise.all call is receiving nested arrays because
Object.entries(...).map returns an array of arrays, so the inner
fireManagedClusterActionCreate promises aren't awaited; update the outer
iteration to produce a flat Promise[] (e.g., replace the outer .map with
.flatMap over Object.entries(missingNamespacesPerCluster)) so Promise.all
receives a flat list of promises, keeping the existing handlers that update
counter, errorClusterNamespacesMap, toastContext.addAlert, and
setCallbackProgress intact; ensure you reference missingNamespacesPerCluster,
fireManagedClusterActionCreate, counter, and setCallbackProgress when applying
the change.
In `@frontend/src/ui-components/AcmAlert/AcmAlert.tsx`:
- Around line 70-81: modifyAlert currently updates only visibleAlerts causing
activeAlerts to become stale; update activeAlerts in the same callback path as
visibleAlerts (or in a synchronized state update) so both arrays remain
consistent: in the modifyAlert implementation (the function named modifyAlert
that calls setVisibleAlerts) also call the corresponding state updater for
activeAlerts (e.g., setActiveAlerts) to find the same alert by id, replace it,
and return the new array, being careful to handle index === -1 (no-op) and
preserve the function’s return value of alertInfo; mirror the same replacement
logic used in addAlert to keep behavior consistent with removeAlert/clearAlerts.
- Around line 70-81: modifyAlert currently sets newAlertInfos[index] without
checking if index === -1 which silently corrupts the array; update the
modifyAlert callback to mirror removeAlert/removeVisibleAlert by checking the
result of alerts.findIndex((ai) => ai.id === alertInfo.id) and only assign
newAlertInfos[index] = alertInfo and return the updated array when index !== -1,
otherwise return the original alerts unchanged (and still return alertInfo from
the outer function).
- Around line 46-51: The addAlert function currently mutates the caller's
alertInfo by assigning alert.id directly; instead create a new object (e.g.,
newAlert = { ...alert, id: generatedId }) and use that when calling
setActiveAlerts and setVisibleAlerts and when returning the value so the
original alert parameter is not modified; update references to alert.id in
addAlert to use the newAlert variable and keep the generated id logic and return
shape the same (AcmAlertInfoWithId).
In `@frontend/src/ui-components/AcmAlert/AcmToast.tsx`:
- Around line 25-30: The addAlert callback currently mutates the caller's alert
object by assigning alert.id; instead create a new object (e.g., alertWithId)
that spreads the incoming AcmAlertInfo and adds the generated id, then pass
alertWithId to setActiveAlerts and setVisibleAlerts and return it as
AcmAlertInfoWithId; update references in the addAlert implementation to avoid
mutating the original object.
- Around line 49-60: modifyAlert currently assumes the alert id exists and
writes into newAlertInfos[index] without checking for -1; update the modifyAlert
callback (the function passed to setVisibleAlerts referenced as modifyAlert and
the state setter setVisibleAlerts operating on AcmAlertInfoWithId) to first
compute index with alerts.findIndex(...), then only replace newAlertInfos[index]
and return the updated array if index !== -1; if index === -1 simply return the
original alerts array (and still return alertInfo from the outer callback) to
avoid silent writes to newAlertInfos[-1].
In `@frontend/src/utils/useClusterNamespaceMap.ts`:
- Around line 31-44: The reducer in useClusterNamespaceMap.ts is using
ns.cluster as a key without validating it, which can create "undefined" or
empty-string keys; update the reduce callback in the function (the items.reduce
handler) to skip entries where clusterName is falsy or not a non-empty string
(e.g., if (!clusterName || typeof clusterName !== 'string' || clusterName.trim()
=== '') return acc), so only valid clusterName values are used to initialize and
push namespaceName into acc[clusterName].
---
Outside diff comments:
In `@frontend/src/wizards/RoleAssignment/CommonProjectCreateProgressBar.tsx`:
- Line 37: The computation of progressValue in CommonProjectCreateProgressBar
can produce NaN when totalCount is 0; update the progressValue calculation
(using successCount, errorCount, totalCount) to defensively handle totalCount
=== 0 (e.g., set progressValue to 0 when totalCount is 0) and clamp the result
to the 0–100 range so the component never renders NaN or out-of-range values.
---
Duplicate comments:
In
`@frontend/src/routes/UserManagement/RoleAssignments/hook/RoleAssignmentDataHook.test.tsx`:
- Line 2: The test imports renderHook from the deprecated package causing React
18 incompatibility; update the import to use renderHook from
'@testing-library/react' instead of '@testing-library/react-hooks' in
RoleAssignmentDataHook.test.tsx and ensure any references to renderHook in this
file (e.g., calls that wrap hook invocation for RoleAssignmentDataHook) continue
to work with the new import (adjust usage to the RTL renderHook API if
necessary).
In `@frontend/src/utils/useClusterNamespaceMap.test.ts`:
- Line 3: Replace the incompatible import of renderHook from
'@testing-library/react-hooks' with the React 18 compatible import from
'@testing-library/react' (update the import statement that currently reads
"import { renderHook } from '@testing-library/react-hooks'" to "import {
renderHook } from '@testing-library/react'"); ensure existing usages of
renderHook in the test remain unchanged and run under the new import.
---
Nitpick comments:
In `@frontend/public/locales/en/translation.json`:
- Around line 1487-1490: The plural translation key "Error creating missing
projects {{project}} for clusters {{cluster}}." uses singular placeholders;
update the placeholders to plural (e.g., change to "Error creating missing
projects {{projects}} for clusters {{clusters}}.") and then update any code that
calls this translation key to pass the corresponding plural variables (projects,
clusters) instead of project/cluster so interpolation matches the plural intent.
In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignments.test.tsx`:
- Line 243: Tests use a concrete mock for modifyAlert (jest.fn((alert: any) =>
alert)) while other tests use bare jest.fn(); update the other test files to
match this behavior or change this one to a bare mock for consistency—locate the
modifyAlert mock in RoleAssignments.test.tsx (the modifyAlert jest.fn) and
replace or align it so all tests consistently either return the passed alert
(jest.fn((alert)=>alert)) or use a plain jest.fn() across the suite.
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 134-142: The JSDoc above RoleAssignmentStatusComponent is stale:
update the comment to match the exported RoleAssignmentStatusComponentProps by
removing references to the nonexistent status and callbackProgress fields (or
replace them with the actual prop names/types used in
RoleAssignmentStatusComponentProps), ensuring the doc correctly describes
callbacksPerReasonMap and any real props; reference the
RoleAssignmentStatusComponent and RoleAssignmentStatusComponentProps symbols
when editing the JSDoc so the comment and exported type stay in sync.
In
`@frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx`:
- Line 50: The async function handleMissingNamespaces closes over
clusterNamespaceMap and is recreated every render (causing new identity in
callbacksPerReasonMap); wrap handleMissingNamespaces in React.useCallback and
include the minimal necessary dependencies (e.g., clusterNamespaceMap and any
other referenced variables) so its identity is stable across renders and it
still captures up-to-date clusterNamespaceMap; update where
callbacksPerReasonMap is built to use the memoized handleMissingNamespaces.
In `@frontend/src/utils/useClusterNamespaceMap.test.ts`:
- Around line 155-177: Add a test to validate that useClusterNamespaceMap skips
items with a falsy cluster value: update the test using
mockUseSearchResultItemsQuery to return items where one entry has cluster:
undefined (or ''/null) and a valid name, then renderHook(() =>
useClusterNamespaceMap()) and assert that result.current.clusterNamespaceMap
does not contain an undefined/empty key and only includes namespaces grouped
under real cluster keys (reference useClusterNamespaceMap and
mockUseSearchResultItemsQuery to locate the code to modify).
In `@frontend/src/utils/useClusterNamespaceMap.ts`:
- Around line 29-30: Inside useClusterNamespaceMap, the if guard using isLoading
and allNamespacesQuery?.searchResult?.[0]?.items?.length guarantees items is
non-null, so replace the redundant optional chaining and fallback in the
assignment for items: change const items =
allNamespacesQuery?.searchResult?.[0]?.items || [] to a direct (non-optional)
access such as const items = allNamespacesQuery!.searchResult![0]!.items or
const items = allNamespacesQuery.searchResult[0].items (whichever matches your
TS strictness), referencing the existing variables isLoading, allNamespacesQuery
and items to remove the unreachable || [] fallback.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (23)
frontend/public/locales/en/translation.jsonfrontend/src/resources/multicluster-role-assignment.tsfrontend/src/routes/Governance/policies/CreatePolicySubmit.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignments.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignments.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentsHelper.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentsHelper.tsxfrontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsxfrontend/src/routes/UserManagement/RoleAssignments/RoleAssignmentWizardModalWrapper.test.tsxfrontend/src/routes/UserManagement/RoleAssignments/hook/RoleAssignmentDataHook.test.tsxfrontend/src/routes/UserManagement/RoleAssignments/hook/RoleAssignmentDataHook.tsfrontend/src/routes/UserManagement/RoleAssignments/roleAssignmentModalHelper.test.tsfrontend/src/ui-components/AcmAlert/AcmAlert.test.tsxfrontend/src/ui-components/AcmAlert/AcmAlert.tsxfrontend/src/ui-components/AcmAlert/AcmToast.tsxfrontend/src/utils/useClusterNamespaceMap.test.tsfrontend/src/utils/useClusterNamespaceMap.tsfrontend/src/wizards/Argo/CreateArgoResources.test.tsxfrontend/src/wizards/RoleAssignment/CommonProjectCreateProgressBar.test.tsxfrontend/src/wizards/RoleAssignment/CommonProjectCreateProgressBar.tsx
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx
Outdated
Show resolved
Hide resolved
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx
Outdated
Show resolved
Hide resolved
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx (3)
124-124:⚠️ Potential issue | 🟡 Minor
aria-disabledis hardcoded and does not reflect the real state.This always announces disabled state to assistive technologies, even when the label is interactive.
💡 Proposed fix
- <Label variant="outline" isDisabled={isCallbackProcessing} aria-disabled={true}> + <Label variant="outline" isDisabled={isCallbackProcessing} aria-disabled={isCallbackProcessing}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` at line 124, The Label component currently hardcodes aria-disabled to true which incorrectly announces it as disabled; update the Label usage in RoleAssignmentStatusComponent so aria-disabled reflects the real state (e.g., aria-disabled={isCallbackProcessing}) or remove the aria-disabled prop entirely and rely on isDisabled for semantics, ensuring the component's accessible state matches the isCallbackProcessing flag.
171-172:⚠️ Potential issue | 🟡 MinorLocalize spinner
aria-labelstrings.Both spinner
aria-labelvalues are hardcoded in English and should uset(...)for localized accessibility text.Also applies to: 211-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 171 - 172, The Spinner aria-labels in RoleAssignmentStatusComponent are hardcoded English strings; update both occurrences where icon={<Spinner isInline aria-label="..."/>} to use the translation function (e.g. aria-label={t('Creating common projects for the role assignment clusters')}) and add corresponding i18n keys used by label props (same keys or appropriately named ones) so the accessibility strings are localized — check both the occurrence around the icon prop with label={t('Creating common projects')} and the duplicate occurrence later (the other icon/label pair around lines ~211-212) and ensure keys are added to the translation files.
43-49:⚠️ Potential issue | 🟠 MajorButton fallback path is still a no-op; disable action when callback is unavailable.
When
callbackis missing, the current handler returns a function instead of executing logic, and the button remains clickable. This leaves a dead action in the UI.💡 Proposed fix
onClick={() => - callback - ? callback(roleAssignment) - : () => console.error('No callback method implemented for reason', roleAssignment.status?.reason) + callback ? callback(roleAssignment) : undefined } - isDisabled={areActionButtonsDisabled} + isDisabled={areActionButtonsDisabled || !callback} isLoading={isCallbackProcessing}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 43 - 49, The onClick currently returns a function when callback is missing which leaves the button clickable; update RoleAssignmentStatusComponent so onClick directly calls callback(roleAssignment) when callback exists and otherwise set onClick to undefined (or null), and ensure the button's disabled prop uses areActionButtonsDisabled || !callback (i.e. set isDisabled={areActionButtonsDisabled || !callback}) so actions are truly disabled when callback is unavailable; keep isLoading as is (isCallbackProcessing).frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx (1)
91-95:⚠️ Potential issue | 🟠 MajorFix toast interpolation key mismatch in the error summary message.
The string uses
{{project}}/{{cluster}}, but the interpolation object providesprojects/clusters. This prevents replacement and shows raw placeholders.💡 Proposed fix
- message: t('Error creating missing projects {{project}} for clusters {{cluster}}.', { + message: t('Error creating missing projects {{projects}} for clusters {{clusters}}.', { clusters: Object.keys(callbackProgress.errorClusterNamespacesMap).join(', '), projects: Object.values(callbackProgress.errorClusterNamespacesMap).flat().join(', '), errorCount: callbackProgress.errorCount, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx` around lines 91 - 95, In useRoleAssignmentsStatusHook.tsx update the toast message template to match the interpolation object: replace the placeholders {{project}} and {{cluster}} with the plural forms {{projects}} and {{clusters}} (the message composed from callbackProgress and its keys like callbackProgress.errorClusterNamespacesMap and callbackProgress.errorCount) so the values provided in the interpolation object are correctly substituted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx`:
- Around line 20-27: The current reduction only iterates
Object.keys(clusterNamespaceMap) so any cluster present in clusterNamesSet but
missing from clusterNamespaceMap is skipped; change the iteration to cover all
clusters in clusterNamesSet (e.g., iterate Array.from(clusterNamesSet) or the
union of Object.keys(clusterNamespaceMap) and clusterNamesSet) and then use
const existingNamespaces = clusterNamespaceMap[cluster] ?? [] to compute missing
namespaces against targetNamespaces and populate the accumulator as before
(references: clusterNamespaceMap, clusterNamesSet, targetNamespaces, the reduce
block).
---
Duplicate comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Line 124: The Label component currently hardcodes aria-disabled to true which
incorrectly announces it as disabled; update the Label usage in
RoleAssignmentStatusComponent so aria-disabled reflects the real state (e.g.,
aria-disabled={isCallbackProcessing}) or remove the aria-disabled prop entirely
and rely on isDisabled for semantics, ensuring the component's accessible state
matches the isCallbackProcessing flag.
- Around line 171-172: The Spinner aria-labels in RoleAssignmentStatusComponent
are hardcoded English strings; update both occurrences where icon={<Spinner
isInline aria-label="..."/>} to use the translation function (e.g.
aria-label={t('Creating common projects for the role assignment clusters')}) and
add corresponding i18n keys used by label props (same keys or appropriately
named ones) so the accessibility strings are localized — check both the
occurrence around the icon prop with label={t('Creating common projects')} and
the duplicate occurrence later (the other icon/label pair around lines ~211-212)
and ensure keys are added to the translation files.
- Around line 43-49: The onClick currently returns a function when callback is
missing which leaves the button clickable; update RoleAssignmentStatusComponent
so onClick directly calls callback(roleAssignment) when callback exists and
otherwise set onClick to undefined (or null), and ensure the button's disabled
prop uses areActionButtonsDisabled || !callback (i.e. set
isDisabled={areActionButtonsDisabled || !callback}) so actions are truly
disabled when callback is unavailable; keep isLoading as is
(isCallbackProcessing).
In
`@frontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx`:
- Around line 91-95: In useRoleAssignmentsStatusHook.tsx update the toast
message template to match the interpolation object: replace the placeholders
{{project}} and {{cluster}} with the plural forms {{projects}} and {{clusters}}
(the message composed from callbackProgress and its keys like
callbackProgress.errorClusterNamespacesMap and callbackProgress.errorCount) so
the values provided in the interpolation object are correctly substituted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsxfrontend/src/routes/UserManagement/RoleAssignment/useRoleAssignmentsStatusHook.tsx
frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Contributor
Author
|
/test pr-image-mirror-mce |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx (1)
213-214:⚠️ Potential issue | 🟡 MinorLocalize pending spinner
aria-label.The pending-state spinner still uses a hardcoded English
aria-label. This should go throught(...)like the other spinner label.Proposed fix
- icon={<Spinner isInline aria-label="Role Assignment being applied" />} + icon={<Spinner isInline aria-label={t('Role Assignment being applied')} />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 213 - 214, The Spinner's aria-label is hardcoded; update the Spinner in RoleAssignmentStatusComponent so its aria-label is localized via the i18n translator (use the existing t(...) call pattern), e.g. replace the inline string in the Spinner prop with a call to t(...) (same key used for label or a specific key like 'RoleAssignment.pending'/'RoleAssignment.pendingAria') so both label and aria-label come from t(...) and remain consistent.
🧹 Nitpick comments (3)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx (2)
36-39: Make missing-project detection more robust to message casing/wording.The message heuristic is case-sensitive and tightly tied to one phrasing. Normalizing case (and matching
namespace/namespaces) reduces false negatives for showing the recovery action.Proposed hardening
- const isMissingNamespaces = - roleAssignment.status?.reason === 'MissingNamespaces' || - (roleAssignment.status?.message?.includes('namespaces') && roleAssignment.status?.message?.includes('not found')) + const normalizedMessage = roleAssignment.status?.message?.toLowerCase() ?? '' + const isMissingNamespaces = + roleAssignment.status?.reason === 'MissingNamespaces' || + (normalizedMessage.includes('namespace') && normalizedMessage.includes('not found'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 36 - 39, The current isMissingNamespaces check relies on a case-sensitive, exact message phrase and misses variants; update the heuristic in RoleAssignmentStatusComponent by normalizing roleAssignment.status?.message to a lowercase string (guarding for undefined) and then testing includes for 'namespace' OR 'namespaces' and 'not found' (all lowercase), while still preserving the existing explicit reason === 'MissingNamespaces' check; reference the isMissingNamespaces variable and roleAssignment.status?.message to implement this more robust matching.
134-142: JSDoc is stale relative to current props.The comment still documents removed fields (
status,callbackProgress.*). Updating this block to current props will avoid API confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 134 - 142, The JSDoc above RoleAssignmentStatusComponent is stale: remove references to the removed props (status and callbackProgress.*) and update the doc to match the component's current props interface; specifically, ensure callbacksPerReasonMap (and any other actual props defined in the RoleAssignmentStatusComponent props/type) are documented with accurate types and descriptions, and remove or replace outdated entries so the JSDoc mirrors the actual Prop type used by RoleAssignmentStatusComponent.frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsx (1)
235-304: Add regression coverage for missing-callback disabled state.This suite validates
areActionButtonsDisabled, but it doesn’t assert the!callbackdisable path. A small test here would lock in the fix and prevent button re-enablement regressions.Suggested test addition
+ it('disables Create missing projects button when callback is not provided', () => { + render( + <RoleAssignmentStatusComponent + roleAssignment={createBaseRoleAssignment({ + ...baseStatus, + status: 'Error', + reason: 'MissingNamespaces', + message: 'Missing', + })} + /> + ) + expect(screen.getByRole('button', { name: 'Create missing projects' })).toBeDisabled() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsx` around lines 235 - 304, Add a test in RoleAssignmentStatusComponent.test.tsx that asserts the "Create missing projects" button is disabled when the component lacks a callback for MissingNamespaces (i.e., callbacksPerReasonMap is undefined or does not include MissingNamespaces): render RoleAssignmentStatusComponent with status 'Error' and reason 'MissingNamespaces' but without callbacksPerReasonMap (or with an empty map), then assert the button is disabled and that clicking it does not call any callback; reference RoleAssignmentStatusComponent, prop callbacksPerReasonMap, and prop areActionButtonsDisabled to ensure you cover the "no-callback" disable path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx`:
- Around line 70-126: The current creation loop blows up into an unbounded
fan-out by calling fireManagedClusterActionCreate for every namespace at once;
change it to use bounded concurrency (e.g., a concurrency limit constant like
MAX_CONCURRENCY = 5) and process missingNamespacesPerCluster with a
concurrency-controlled runner (per-cluster or global) that schedules at most
MAX_CONCURRENCY outstanding fireManagedClusterActionCreate promises at a time,
preserving the existing .then/.catch/.finally handling (including counter
updates, errorClusterNamespacesMap, addAlertCallback and onProgressCallback) so
progress and error reporting remain identical; locate the logic around
missingNamespacesPerCluster and fireManagedClusterActionCreate in
roleAssignmentErrorHandlingFunctions.tsx and replace the
Promise.all(flatMap(...)) fan-out with a bounded queue/pool or a small
p-limit-like helper to throttle concurrent calls.
- Around line 20-23: Deduplicate targetNamespaces before doing the diff to avoid
scheduling duplicates: inside the reduce that iterates
Array.from(clusterNamesSet) (the block referencing clusterNamespaceMap and
targetNamespaces), replace uses of targetNamespaces with a deduplicated version
(e.g., create const uniqueTargetNamespaces = Array.from(new
Set(targetNamespaces)) prior to the reduce or at the top of the reducer) and use
uniqueTargetNamespaces when computing missing = uniqueTargetNamespaces.filter(ns
=> !existingNamespaces.includes(ns)) so progress/errors/counts aren't inflated
by duplicates.
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 191-203: The ExpandableSection in RoleAssignmentStatusComponent
currently renders roleAssignment.status?.message directly, which can be empty
and overrides the StatusTooltip fallback; update the bodyContent so the
ExpandableSection displays roleAssignment.status?.message if present, otherwise
render the same fallback string used by StatusTooltip (reuse the same
translation/key or constant used elsewhere in this component for missing
details) and keep existing props (isErrorExpanded, onErrorToggle) intact.
---
Duplicate comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 213-214: The Spinner's aria-label is hardcoded; update the Spinner
in RoleAssignmentStatusComponent so its aria-label is localized via the i18n
translator (use the existing t(...) call pattern), e.g. replace the inline
string in the Spinner prop with a call to t(...) (same key used for label or a
specific key like 'RoleAssignment.pending'/'RoleAssignment.pendingAria') so both
label and aria-label come from t(...) and remain consistent.
---
Nitpick comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsx`:
- Around line 235-304: Add a test in RoleAssignmentStatusComponent.test.tsx that
asserts the "Create missing projects" button is disabled when the component
lacks a callback for MissingNamespaces (i.e., callbacksPerReasonMap is undefined
or does not include MissingNamespaces): render RoleAssignmentStatusComponent
with status 'Error' and reason 'MissingNamespaces' but without
callbacksPerReasonMap (or with an empty map), then assert the button is disabled
and that clicking it does not call any callback; reference
RoleAssignmentStatusComponent, prop callbacksPerReasonMap, and prop
areActionButtonsDisabled to ensure you cover the "no-callback" disable path.
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 36-39: The current isMissingNamespaces check relies on a
case-sensitive, exact message phrase and misses variants; update the heuristic
in RoleAssignmentStatusComponent by normalizing roleAssignment.status?.message
to a lowercase string (guarding for undefined) and then testing includes for
'namespace' OR 'namespaces' and 'not found' (all lowercase), while still
preserving the existing explicit reason === 'MissingNamespaces' check; reference
the isMissingNamespaces variable and roleAssignment.status?.message to implement
this more robust matching.
- Around line 134-142: The JSDoc above RoleAssignmentStatusComponent is stale:
remove references to the removed props (status and callbackProgress.*) and
update the doc to match the component's current props interface; specifically,
ensure callbacksPerReasonMap (and any other actual props defined in the
RoleAssignmentStatusComponent props/type) are documented with accurate types and
descriptions, and remove or replace outdated entries so the JSDoc mirrors the
actual Prop type used by RoleAssignmentStatusComponent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
frontend/public/locales/en/translation.jsonfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsxfrontend/src/ui-components/AcmAlert/AcmAlert.test.tsxfrontend/src/ui-components/AcmAlert/AcmAlert.tsxfrontend/src/ui-components/AcmAlert/AcmToast.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.test.tsx
frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx
Outdated
Show resolved
Hide resolved
frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx
Outdated
Show resolved
Hide resolved
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx
Show resolved
Hide resolved
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx (1)
210-217:⚠️ Potential issue | 🟡 MinorLocalize the Spinner
aria-labelfor the Pending state.The Spinner at line 213 uses a hardcoded English
aria-label("Role Assignment being applied"), while the Spinner at line 173 correctly uses the localized version viat(). This should be consistent for accessibility in non-English locales.🔧 Proposed fix
case roleAssignment.status?.status === 'Pending': return ( <StatusTooltip - icon={<Spinner isInline aria-label="Role Assignment being applied" />} + icon={<Spinner isInline aria-label={t('Role Assignment being applied')} />} label={t('Pending')} {...commonStatusTooltipProps} /> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx` around lines 210 - 217, In RoleAssignmentStatusComponent.tsx update the Pending-state Spinner to use the localized aria-label instead of the hardcoded English string: replace the literal "Role Assignment being applied" passed to Spinner's aria-label with the result of the translation function t(...) (matching the pattern used by the other Spinner in this component), so the aria-label is localized for non-English locales.
🧹 Nitpick comments (1)
frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx (1)
78-114: Consider extracting duplicate error handling logic.The error handling in the
.then()branch (lines 82-96) and.catch()branch (lines 100-114) contains nearly identical code for incrementingcounter.error, updatingerrorClusterNamespacesMap, and callingaddAlertCallback. This could be extracted to a helper function.♻️ Suggested refactor
+ const handleError = (clusterName: string, namespace: string, errorMessage: string) => { + counter.error++ + counter.errorClusterNamespacesMap[clusterName] = [ + ...(counter.errorClusterNamespacesMap[clusterName] || []), + namespace, + ] + addAlertCallback({ + title: t('Error creating missing project'), + message: t('Error creating missing project {{project}} for cluster {{cluster}}. Error: {{error}}.', { + project: namespace, + cluster: clusterName, + error: errorMessage, + }), + type: 'danger', + autoClose: true, + }) + } + const createMissingProject = (clusterName: string, namespace: string) => fireManagedClusterActionCreate(clusterName, { apiVersion: ProjectRequestApiVersion, kind: ProjectRequestKind, metadata: { name: namespace }, }) .then((actionResponse) => { if (actionResponse.actionDone === 'ActionDone') { counter.success++ } else { - counter.error++ - counter.errorClusterNamespacesMap[clusterName] = [ - ...(counter.errorClusterNamespacesMap[clusterName] || []), - namespace, - ] - addAlertCallback({ - title: t('Error creating missing project'), - message: t('Error creating missing project {{project}} for cluster {{cluster}}. Error: {{error}}.', { - project: namespace, - cluster: clusterName, - error: actionResponse.message, - }), - type: 'danger', - autoClose: true, - }) + handleError(clusterName, namespace, actionResponse.message ?? 'Unknown error') } }) - .catch((err: Error) => { - counter.error++ - counter.errorClusterNamespacesMap[clusterName] = [ - ...(counter.errorClusterNamespacesMap[clusterName] || []), - namespace, - ] - addAlertCallback({ - title: t('Error creating missing project'), - message: t('Error creating missing project {{project}} for cluster {{cluster}}. Error: {{error}}.', { - project: namespace, - cluster: clusterName, - error: err.message, - }), - type: 'danger', - autoClose: true, - }) - }) + .catch((err: Error) => handleError(clusterName, namespace, err.message))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx` around lines 78 - 114, Extract the duplicate error handling into a helper (e.g., handleActionError) that accepts (clusterName: string, namespace: string, errorMessage: string) and performs the shared work: increment counter.error, update counter.errorClusterNamespacesMap[clusterName] by appending namespace, and call addAlertCallback with the t(...) translated title and message using errorMessage; then replace the duplicated blocks inside the .then() branch (where actionResponse.actionDone !== 'ActionDone') and the .catch((err) => ...) to call handleActionError(clusterName, namespace, actionResponse.message) or handleActionError(clusterName, namespace, err.message) respectively so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 210-217: In RoleAssignmentStatusComponent.tsx update the
Pending-state Spinner to use the localized aria-label instead of the hardcoded
English string: replace the literal "Role Assignment being applied" passed to
Spinner's aria-label with the result of the translation function t(...)
(matching the pattern used by the other Spinner in this component), so the
aria-label is localized for non-English locales.
---
Nitpick comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx`:
- Around line 78-114: Extract the duplicate error handling into a helper (e.g.,
handleActionError) that accepts (clusterName: string, namespace: string,
errorMessage: string) and performs the shared work: increment counter.error,
update counter.errorClusterNamespacesMap[clusterName] by appending namespace,
and call addAlertCallback with the t(...) translated title and message using
errorMessage; then replace the duplicated blocks inside the .then() branch
(where actionResponse.actionDone !== 'ActionDone') and the .catch((err) => ...)
to call handleActionError(clusterName, namespace, actionResponse.message) or
handleActionError(clusterName, namespace, err.message) respectively so behavior
remains identical.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsx (1)
248-260: Add callback-assertion coverage for the message-based fallback path.Line 248 currently verifies only button visibility when
reasonisApplicationFailedand message contains missing-namespace text. Add a click assertion for this branch too, so regressions don’t slip through when callback routing changes.💡 Suggested test addition
+ it('invokes MissingNamespaces callback when reason is ApplicationFailed but message indicates missing namespaces', () => { + const onMissingNamespaces = jest.fn() + render( + <RoleAssignmentStatusComponent + roleAssignment={createBaseRoleAssignment({ + ...baseStatus, + status: 'Error', + reason: 'ApplicationFailed', + message: 'Required namespaces not found on cluster', + })} + callbacksPerReasonMap={{ + MissingNamespaces: onMissingNamespaces, + }} + /> + ) + screen.getByRole('button', { name: 'Create missing projects' }).click() + expect(onMissingNamespaces).toHaveBeenCalledTimes(1) + })Also applies to: 278-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsx` around lines 248 - 260, The test only checks visibility of the "Create missing projects" button but not that clicking it calls the component callback; update the RoleAssignmentStatusComponent tests (the case at lines around 248 and the similar case around 278-301) to pass a jest.fn() handler (e.g. const onCreateMissing = jest.fn()) into the component, render the component with that prop, find the button by role/name, simulate a click (userEvent.click or fireEvent.click) and assert the mock was called (expect(onCreateMissing).toHaveBeenCalled()). Ensure you import and use userEvent or fireEvent and replace the existing visibility-only assertions with both visibility and click-callback assertions for RoleAssignmentStatusComponent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`:
- Around line 134-142: Update the stale JSDoc for RoleAssignmentStatusComponent
to match the exported prop names and shapes: replace references to "status" and
"callbackProgress.*" with the actual props "roleAssignment" (describe its
expected shape or type), "callbacksPerReasonMap" (map of reason -> callback),
"isCallbackProcessing" (boolean) and "areActionButtonsDisabled" (boolean);
ensure param tags match exact prop identifiers used in the component and remove
or adjust any outdated nested fields like
callbackProgress.successCount/errorCount/totalCount so the docblock accurately
reflects the current component API (RoleAssignmentStatusComponent and its prop
names).
---
Nitpick comments:
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsx`:
- Around line 248-260: The test only checks visibility of the "Create missing
projects" button but not that clicking it calls the component callback; update
the RoleAssignmentStatusComponent tests (the case at lines around 248 and the
similar case around 278-301) to pass a jest.fn() handler (e.g. const
onCreateMissing = jest.fn()) into the component, render the component with that
prop, find the button by role/name, simulate a click (userEvent.click or
fireEvent.click) and assert the mock was called
(expect(onCreateMissing).toHaveBeenCalled()). Ensure you import and use
userEvent or fireEvent and replace the existing visibility-only assertions with
both visibility and click-callback assertions for RoleAssignmentStatusComponent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.test.tsxfrontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsxfrontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/routes/UserManagement/RoleAssignment/roleAssignmentErrorHandlingFunctions.tsx
Comment on lines
134
to
142
| /** Props for the RoleAssignmentStatusComponent | ||
| * @param status - The status of the role assignment | ||
| * @param callbacksPerReasonMap - A map of callbacks per reason. The key is the reason and the value is the callback function. This is used to display the callback button in the status tooltip. | ||
| * @param callbackProgress - The status of the callback processing | ||
| * @param callbackProgress.isProcessing - Whether the callback processing is in progress | ||
| * @param callbackProgress.successCount - The number of successful callbacks | ||
| * @param callbackProgress.errorCount - The number of error callbacks | ||
| * @param callbackProgress.totalCount - The total number of callbacks | ||
| */ |
There was a problem hiding this comment.
Props docblock is stale and mismatched with the exported type.
Line 135+ still documents status and callbackProgress.*, but the actual props are roleAssignment, callbacksPerReasonMap, isCallbackProcessing, and areActionButtonsDisabled. This will mislead consumers and reviewers.
📝 Suggested doc update
-/** Props for the RoleAssignmentStatusComponent
- * `@param` status - The status of the role assignment
- * `@param` callbacksPerReasonMap - A map of callbacks per reason. The key is the reason and the value is the callback function. This is used to display the callback button in the status tooltip.
- * `@param` callbackProgress - The status of the callback processing
- * `@param` callbackProgress.isProcessing - Whether the callback processing is in progress
- * `@param` callbackProgress.successCount - The number of successful callbacks
- * `@param` callbackProgress.errorCount - The number of error callbacks
- * `@param` callbackProgress.totalCount - The total number of callbacks
- */
+/** Props for the RoleAssignmentStatusComponent
+ * `@param` roleAssignment - Flattened role assignment row object used for status rendering
+ * `@param` callbacksPerReasonMap - Optional callback per status reason for footer actions
+ * `@param` isCallbackProcessing - Whether a reason callback action is currently running
+ * `@param` areActionButtonsDisabled - Disables action buttons in tooltip footer
+ */📝 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
| /** Props for the RoleAssignmentStatusComponent | |
| * @param status - The status of the role assignment | |
| * @param callbacksPerReasonMap - A map of callbacks per reason. The key is the reason and the value is the callback function. This is used to display the callback button in the status tooltip. | |
| * @param callbackProgress - The status of the callback processing | |
| * @param callbackProgress.isProcessing - Whether the callback processing is in progress | |
| * @param callbackProgress.successCount - The number of successful callbacks | |
| * @param callbackProgress.errorCount - The number of error callbacks | |
| * @param callbackProgress.totalCount - The total number of callbacks | |
| */ | |
| /** Props for the RoleAssignmentStatusComponent | |
| * `@param` roleAssignment - Flattened role assignment row object used for status rendering | |
| * `@param` callbacksPerReasonMap - Optional callback per status reason for footer actions | |
| * `@param` isCallbackProcessing - Whether a reason callback action is currently running | |
| * `@param` areActionButtonsDisabled - Disables action buttons in tooltip footer | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/src/routes/UserManagement/RoleAssignment/RoleAssignmentStatusComponent.tsx`
around lines 134 - 142, Update the stale JSDoc for RoleAssignmentStatusComponent
to match the exported prop names and shapes: replace references to "status" and
"callbackProgress.*" with the actual props "roleAssignment" (describe its
expected shape or type), "callbacksPerReasonMap" (map of reason -> callback),
"isCallbackProcessing" (boolean) and "areActionButtonsDisabled" (boolean);
ensure param tags match exact prop identifiers used in the component and remove
or adjust any outdated nested fields like
callbackProgress.successCount/errorCount/totalCount so the docblock accurately
reflects the current component API (RoleAssignmentStatusComponent and its prop
names).
Ginxo
commented
Feb 26, 2026
| const { t } = useTranslation() | ||
| const callback = roleAssignment.status?.reason ? callbacksPerReasonMap?.[roleAssignment.status.reason] : undefined | ||
|
|
||
| const isMissingNamespaces = |
Contributor
Author
There was a problem hiding this comment.
temporary solution while backend is offering more error granularity
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
Signed-off-by: Enrique Mingorance Cano <emingora@redhat.com>
|
@Ginxo: The following test 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.
📝 Summary
it depends on backend, so holding it for now
Ticket Summary (Title):
RBAC UI - Creation wizard - project validation
Ticket Link:
https://issues.redhat.com/browse/ACM-29130
Evidences
Video_2026-02-25_17-50-12.mp4
Testing instructions
Expected results:
the projects are created on the different clusters and the error disappears as soon as the resources are reconciled.
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests