OU-1195 [DashboardListPage] List OCP namespaces in Create Dashboard ProjectSelector #765
Open
zhuje wants to merge 11 commits intoopenshift:mainfrom
Open
OU-1195 [DashboardListPage] List OCP namespaces in Create Dashboard ProjectSelector #765zhuje wants to merge 11 commits intoopenshift:mainfrom
zhuje wants to merge 11 commits intoopenshift:mainfrom
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zhuje 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 |
openshift-ci
bot
added
the
approved
zhuje
force-pushed
the
ou1195-list-all-projects-main-pr
branch
from
c781f9b to
c191b22
Compare
…oard ProjectSelector
zhuje
force-pushed
the
ou1195-list-all-projects-main-pr
branch
from
6fa563f to
d6f3b6d
Compare
jgbernalp
reviewed
Feb 12, 2026
| ): string[] => { | ||
| const persesProjectNames = Object.keys(persesUserPermissions).filter((name) => name !== '*'); | ||
| const allAvailableProjects = new Set<string>([...persesProjectNames]); | ||
| ocpProjects?.forEach((project) => { |
Contributor
There was a problem hiding this comment.
This seems to be required.
Suggested change
| ocpProjects?.forEach((project) => { | |
| ocpProjects.forEach((project) => { |
Contributor
Author
There was a problem hiding this comment.
Included in the latest commit
jgbernalp
reviewed
Feb 12, 2026
| return ( | ||
| <> | ||
| {disabled ? ( | ||
| <Tooltip content={t('Create button is disabled because you do not have permission')}> |
Contributor
There was a problem hiding this comment.
Suggested change
| <Tooltip content={t('Create button is disabled because you do not have permission')}> | |
| <Tooltip content={t('You don't have permissions to create dashboards')}> |
Contributor
Author
There was a problem hiding this comment.
Included in the latest commit
jgbernalp
reviewed
Feb 12, 2026
| key={selectedProject || 'no-selection'} | ||
| initialOptions={projectOptions} | ||
| placeholder={t('Select a project')} | ||
| noOptionsFoundMessage={(filter) => t(`No project found for "${filter}"`)} |
Contributor
There was a problem hiding this comment.
This won't work for the translation as it does not use interpolation. Either we use interpolation inside the translation or we just translate the static part
Contributor
Author
There was a problem hiding this comment.
Updated in the latest commit and run make i18n-frontend to update translations list in plugins__monitoring-plugin.json
|
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 many dashboard localization keys and implements a permissions-aware project flow: Perses permission fetching, OCP project watching, a unified useEditableProjects hook, TypeaheadSelect UI for project selection, and a create-project API/mutation. Changes
sequenceDiagram
participant User
participant UI as Dashboard UI
participant Hook as useEditableProjects
participant OCP as OCP API (Projects)
participant PermAPI as Perses Permissions API
participant ProjAPI as Perses Project API
participant Cache as Query Cache
User->>UI: Open create/rename dialog
UI->>Hook: initialize (reads user + OCP projects)
Hook->>OCP: watch/list Projects
Hook->>PermAPI: fetchPersesUserPermissions(username)
PermAPI-->>Hook: return permissions per project
Hook-->>UI: editableProjects, allProjects, loading/error
User->>UI: Select or type project name
alt Project does not exist
UI->>ProjAPI: createPersesProject(projectName)
ProjAPI-->>UI: project created
UI->>Cache: invalidate 'projects' & 'dashboards'
UI-->>Hook: refresh projects/permissions
end
User->>UI: Submit dashboard form
UI->>ProjAPI: create dashboard (with project)
ProjAPI-->>UI: dashboard created
UI->>Cache: invalidate 'dashboards'
UI-->>User: navigate to new dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/dashboards/perses/dashboard-action-modals.tsx (1)
197-205:⚠️ Potential issue | 🔴 CriticalRuntime crash:
editableProjectsandallProjectscan beundefined.When permissions are loading or errored,
useEditableProjectsreturnsundefinedfor botheditableProjectsandallProjects. Line 200 callseditableProjects.includes(...)and line 204 accessesallProjects[0], both of which will throwTypeErroronundefined.Suggested fix — add optional chaining / fallbacks
const defaultProject = useMemo(() => { if (!dashboard) return ''; - if (dashboard.metadata.project && editableProjects.includes(dashboard.metadata.project)) { + if (dashboard.metadata.project && editableProjects?.includes(dashboard.metadata.project)) { return dashboard.metadata.project; } - return allProjects[0] || ''; + return allProjects?.[0] || ''; }, [dashboard, editableProjects, allProjects]);
🤖 Fix all issues with AI agents
In `@web/locales/en/plugin__monitoring-plugin.json`:
- Around line 169-229: The code in dashboard-action-modals.tsx calls t(`No
namespace found for "${filter}"`) but that exact key is missing from
plugin__monitoring-plugin.json; either change the call to reuse the existing
interpolated key t('No project found for "{{filter}}"') (replace string
interpolation with the i18n {{filter}} syntax) or add a new JSON entry "No
namespace found for \"{{filter}}\"": "No namespace found for \"{{filter}}\"" to
the locales file so t('No namespace found for \"{{filter}}\"') will resolve;
update the call site to use the same {{filter}} placeholder rather than JS
string interpolation.
- Line 200: Update the translation value for the key "You don't have permissions
to dashboard actions" to a grammatically correct phrase; locate the JSON entry
with that exact key and replace the value string with a clearer alternative such
as "You don't have permission to perform dashboard actions" (or "You don't have
permission for dashboard actions") so the key/value reads naturally in the
locales file.
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Line 307: The modal title is hardcoded in the Duplicate Dashboard
modal—replace the literal "Duplicate Dashboard" passed to ModalHeader with a
translated string (e.g. title={t('Duplicate Dashboard')}) and ensure the
translation function is available in this component (import/use the existing
useTranslation hook or the component's t prop as used in RenameActionModal and
other modals); update the ModalHeader call in this duplicate-dashboard modal to
use t('Duplicate Dashboard') instead of the hardcoded string.
- Around line 375-384: When handling onClearSelection for the TypeaheadSelect,
after calling setSelectedProject(null) also clear the form's projectName so the
form state and UI stay consistent; update the onClearSelection handler in
dashboard-action-modals.tsx to call the form API (e.g.
setValue('projectName','') or resetField('projectName')) and clear validation
(clearErrors('projectName')) in addition to setSelectedProject(null) so the form
value, errors, and selectedProject are all cleared together.
- Around line 371-384: The translation call inside the TypeaheadSelect's
noOptionsFoundMessage currently uses a JS template literal (t(`No namespace
found for "${filter}"`)) which prevents i18next interpolation; change it to use
i18next interpolation syntax by passing a translation key string and an
interpolation object (e.g., t('No namespace found for "{{filter}}"', { filter
})) in the noOptionsFoundMessage prop so i18next can match the locale entry and
substitute the runtime filter value; update the TypeaheadSelect usage where
noOptionsFoundMessage is defined to use t('No namespace found for "{{filter}}"',
{ filter }) instead of the template literal.
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 143-149: The modal toggle handler handleModalToggle currently
flips isModalOpen and resets dashboardName and formErrors only when closing;
extend it to also reset selectedProject when the modal is being closed so the
selection isn't stale on next open. Update handleModalToggle to call
setSelectedProject(undefined or '' depending on the selectedProject type)
alongside setDashboardName('') and setFormErrors({}) in the branch where
isModalOpen is true.
- Around line 159-170: The inline CreateBtn component is recreated each render;
instead, stop defining function CreateBtn inside the component—either extract it
to a top-level component that accepts props (e.g., handleModalToggle, disabled,
permissionsLoading, t, persesDashboardDataTestIDs.createDashboardButtonToolbar)
or convert it to a stable JSX variable (e.g., const createBtn = (<Button
...>{permissionsLoading ? t('Loading...') : t('Create')}</Button>)) and replace
<CreateBtn /> with {createBtn} to avoid unmount/remount and preserve DOM state.
In `@web/src/components/dashboards/perses/hooks/useEditableProjects.ts`:
- Around line 32-60: The getEditableProjects function (useEditableProjects) can
produce duplicate project names when a wildcard project '*' and specific project
entries coexist; update the logic to deduplicate results (e.g., collect names
into a Set or filter uniques before returning) and simplify the conditional by
replacing the redundant "else if (projectName !== '*')" with just "else" so
wildcard expansion and specific project additions don't double-add the same
project names.
In `@web/src/components/dashboards/perses/perses-client.ts`:
- Around line 30-33: The PersesPermission interface declares scopes as a string
but the code treats it like an array (using .includes) and actions is correctly
string[]; update the PersesPermission declaration so scopes: string[] to match
usage and allow array membership checks, then audit usages of PersesPermission
(e.g., any logic reading PersesPermission.scopes and calling .includes or
iterating) to ensure they handle an array; alternatively, if scopes should be a
single value instead, change call sites that use .includes('Dashboard') to an
explicit equality check (===) — prefer changing PersesPermission.scopes to
string[] to align with actions: string[] and existing .includes usage.
🧹 Nitpick comments (3)
web/src/components/dashboards/perses/hooks/useOcpProjects.ts (1)
5-10:useK8sWatchResourceerror return value is discarded.
useK8sWatchResourcereturns[data, loaded, error], but only the first two values are destructured. If the watch fails (e.g., RBAC denial for non-optional resources, network issues), the error is silently swallowed, and consumers will see an empty project list with no indication of failure.Consider capturing and exposing the error so
useEditableProjectscan surface it:Suggested change
- const [ocpProjects, ocpProjectsLoaded] = useK8sWatchResource<K8sResourceKind[]>({ + const [ocpProjects, ocpProjectsLoaded, ocpProjectsError] = useK8sWatchResource<K8sResourceKind[]>({ isList: true, kind: ProjectModel.kind, optional: true, });web/src/components/dashboards/perses/dashboard-create-dialog.tsx (1)
86-98: Dashboard name duplicate check may miss concurrently created dashboards.The duplicate check on lines 86-98 relies on
dashboardsfetched fromusePerses(selectedProject), which is populated only when the modal is open and a project is selected. If the data hasn't loaded yet or is stale, a duplicate name could slip through. Consider also handling a 409 Conflict response from the server as a fallback.web/src/components/dashboards/perses/hooks/useEditableProjects.ts (1)
12-16:useUsernamedirectly accesses internal Redux state (state.sdkCore?.user) — fragile coupling to SDK internals.This pattern relies on the internal state structure of
@openshift-console/dynamic-plugin-sdk. While no public hooks currently exist in SDK v4.19.0 to replace this, the SDK team has acknowledged this limitation with a TODO noting that public user access utilities will be available in future SDK versions. Revisit this when the SDK provides public alternatives (e.g.,useActiveUser).
web/src/components/dashboards/perses/dashboard-action-modals.tsx
Outdated
Show resolved
Hide resolved
web/src/components/dashboards/perses/dashboard-create-dialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/components/dashboards/perses/dashboard-action-modals.tsx (1)
197-205:⚠️ Potential issue | 🔴 Critical
editableProjectsandallProjectscan beundefined, causing a crash.Per the
useEditableProjectshook, botheditableProjectsandallProjectsareundefinedwhile permissions are loading or if an error occurs. Line 200 callseditableProjects.includes(...)and line 204 accessesallProjects[0]without null guards, which will throw aTypeErrorduring the initial render before permissions have loaded.Suggested fix
const defaultProject = useMemo(() => { if (!dashboard) return ''; - if (dashboard.metadata.project && editableProjects.includes(dashboard.metadata.project)) { + if (dashboard.metadata.project && editableProjects?.includes(dashboard.metadata.project)) { return dashboard.metadata.project; } - return allProjects[0] || ''; + return allProjects?.[0] || ''; }, [dashboard, editableProjects, allProjects]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx` around lines 197 - 205, The computed defaultProject in the useMemo (symbol: defaultProject) assumes editableProjects and allProjects are arrays and calls editableProjects.includes(...) and allProjects[0], which can be undefined; update the logic to null-guard those values (e.g., treat undefined as empty arrays) before using includes or indexing and return '' if neither yields a valid project; specifically modify the useMemo that references dashboard.metadata.project, editableProjects and allProjects to first coerce editableProjects = editableProjects || [] and allProjects = allProjects || [] (or use Array.isArray checks) so includes and [0] are safe during permission loading/errors.web/src/components/dashboards/perses/dashboard-create-dialog.tsx (1)
86-98:⚠️ Potential issue | 🟡 MinorDuplicate-name error message is not translatable.
Line 95 uses a hardcoded English string instead of
t(). The locale file already has a key for this:"Dashboard name '{{dashboardName}}' already exists in '{{projectName}}' project!".Suggested fix
setFormErrors({ - dashboardName: `Dashboard name "${dashboardName}" already exists in this project`, + dashboardName: t("Dashboard name '{{dashboardName}}' already exists in '{{projectName}}' project!", { + dashboardName, + projectName: selectedProject, + }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx` around lines 86 - 98, Replace the hardcoded English error string with a translatable message using the i18n key and interpolation; when setting form errors in the duplicate-name check inside the component (the block that references dashboards, selectedProject, dashboardName and calls setFormErrors), call t() with the key "Dashboard name '{{dashboardName}}' already exists in '{{projectName}}' project!" (or the mapped lookup key from your locale) and pass { dashboardName: dashboardName.trim(), projectName: selectedProject } so the error becomes translatable and includes both the dashboard and project names.
🧹 Nitpick comments (2)
web/src/components/dashboards/perses/dashboard-action-modals.tsx (1)
371-372: Samekeyprop remount pattern as the create dialog.
key={selectedProject || 'no-selection'}forces a full remount ofTypeaheadSelecton every selection change. See the related comment ondashboard-create-dialog.tsxfor details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx` around lines 371 - 372, The TypeaheadSelect is being force-remounted by using key={selectedProject || 'no-selection'} (seen in dashboard-action-modals.tsx), which causes loss of internal state on each selection; remove the key prop or replace it with a stable identifier and instead make the component controlled via props (pass selectedProject, onChange, and any needed value/initialValue) so it updates without remounting; ensure TypeaheadSelect's value/selected prop is used for controlled updates and only use a key when you explicitly need to reset the component state.web/locales/en/plugin__monitoring-plugin.json (1)
231-231: Developer error message in translation file is unnecessary.
"useToast must be used within ToastProvider"is a developer-facing invariant error, not a user-facing string. Translating it adds noise to the locale file and offers no UX benefit. Consider removing it from the translations and using a plain string in the source code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/locales/en/plugin__monitoring-plugin.json` at line 231, Remove the developer-facing invariant string from the locale JSON and stop looking it up via i18n: delete the key/value `"useToast must be used within ToastProvider": "useToast must be used within ToastProvider"` from plugin__monitoring-plugin.json, then update the code that throws this error (the useToast hook/function and any callsites that reference the i18n key) to throw or log a plain hard-coded message like "useToast must be used within ToastProvider" directly in the source (e.g., inside useToast and/or ToastProvider implementations) so there are no runtime i18n lookups for a developer-only error. Ensure no remaining references to that translation key remain to avoid missing-key warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 222-231: projectOptions currently derives its selected flag from
form.watch('projectName') (selectedProjectName) while the TypeaheadSelect
component is keyed by the local state selectedProject, causing potential
mismatch when form.reset() updates the form but selectedProject isn't updated;
pick a single source of truth: either (A) remove local selectedProject state and
key TypeaheadSelect on selectedProjectName and update projectOptions to use
selectedProjectName everywhere (update any handlers to set
form.setValue('projectName') instead of setSelectedProject), or (B) keep local
selectedProject as the canonical value and change projectOptions to compute
selected based on selectedProject (and ensure form values are synced when
submitting/closing, and update handleClose to reset selectedProject); update all
references to selectedProjectName/selectedProject, TypeaheadSelect key, and
handleClose/form.reset accordingly so they use the same source.
- Around line 294-297: The local state selectedProject can become out-of-sync
with form.watch('projectName') because handleClose only calls form.reset(); fix
by clearing or syncing selectedProject when the form is reset: update
handleClose to also call setSelectedProject(null) (or derive selectedProject
from form.watch('projectName') instead of keeping separate state) so that
onProjectSelect, TypeaheadSelect key, and projectOptions.selected stay
consistent with the form reset.
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 100-116: The toast and alert messages around project creation use
JS template literals and bypass i18next; update the calls to addAlert and
setFormErrors to use t() with interpolation (e.g., t('dashboard.projectCreated',
{ project: selectedProject }) and t('dashboard.projectCreateError', { project:
selectedProject })) instead of string templates, add corresponding keys to the
locale files, and ensure createProjectMutation.mutateAsync, selectedProject,
persesProjects, addAlert, and setFormErrors are updated to use those i18n keys
so the success and error messages are translatable.
- Around line 271-280: The button logic mistakenly uses the v5-only mutation
property `isPending` so loading/disabled states never trigger; update all
occurrences of `createDashboardMutation.isPending` and
`createProjectMutation.isPending` to use `isLoading` instead (e.g., in the
disabled expression, the `isLoading` prop, and the conditional text block) so
the button properly reflects mutation state when `createDashboardMutation` or
`createProjectMutation` are in-flight.
---
Outside diff comments:
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 197-205: The computed defaultProject in the useMemo (symbol:
defaultProject) assumes editableProjects and allProjects are arrays and calls
editableProjects.includes(...) and allProjects[0], which can be undefined;
update the logic to null-guard those values (e.g., treat undefined as empty
arrays) before using includes or indexing and return '' if neither yields a
valid project; specifically modify the useMemo that references
dashboard.metadata.project, editableProjects and allProjects to first coerce
editableProjects = editableProjects || [] and allProjects = allProjects || []
(or use Array.isArray checks) so includes and [0] are safe during permission
loading/errors.
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 86-98: Replace the hardcoded English error string with a
translatable message using the i18n key and interpolation; when setting form
errors in the duplicate-name check inside the component (the block that
references dashboards, selectedProject, dashboardName and calls setFormErrors),
call t() with the key "Dashboard name '{{dashboardName}}' already exists in
'{{projectName}}' project!" (or the mapped lookup key from your locale) and pass
{ dashboardName: dashboardName.trim(), projectName: selectedProject } so the
error becomes translatable and includes both the dashboard and project names.
---
Duplicate comments:
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Line 307: The previous concern about an untranslated modal title has been
addressed by using the translation function t in the ModalHeader call; verify
that the translation key passed to t ('Duplicate Dashboard') exists in the
locale files and that ModalHeader (used here) receives the title prop and
labelId prop correctly, then approve/remove the duplicate review comment; no
code changes required in ModalHeader usage itself (confirm presence of t,
ModalHeader, and labelId).
- Around line 371-385: The PR comment includes duplicate review markers and the
previous concerns are already fixed: keep the TypeaheadSelect change as-is
(retain the i18n interpolation t('No namespace found for "{{filter}}"', { filter
})) and the onClearSelection handler should continue resetting both local state
and the form via setSelectedProject(null) and form.setValue('projectName', '');
remove the duplicate review tag/marker ([duplicate_comment] / duplicated
approval note) from the PR/comment metadata so the review is clean and only one
approval comment remains.
---
Nitpick comments:
In `@web/locales/en/plugin__monitoring-plugin.json`:
- Line 231: Remove the developer-facing invariant string from the locale JSON
and stop looking it up via i18n: delete the key/value `"useToast must be used
within ToastProvider": "useToast must be used within ToastProvider"` from
plugin__monitoring-plugin.json, then update the code that throws this error (the
useToast hook/function and any callsites that reference the i18n key) to throw
or log a plain hard-coded message like "useToast must be used within
ToastProvider" directly in the source (e.g., inside useToast and/or
ToastProvider implementations) so there are no runtime i18n lookups for a
developer-only error. Ensure no remaining references to that translation key
remain to avoid missing-key warnings.
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 371-372: The TypeaheadSelect is being force-remounted by using
key={selectedProject || 'no-selection'} (seen in dashboard-action-modals.tsx),
which causes loss of internal state on each selection; remove the key prop or
replace it with a stable identifier and instead make the component controlled
via props (pass selectedProject, onChange, and any needed value/initialValue) so
it updates without remounting; ensure TypeaheadSelect's value/selected prop is
used for controlled updates and only use a key when you explicitly need to reset
the component state.
web/src/components/dashboards/perses/dashboard-create-dialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/components/dashboards/perses/project/ProjectDropdown.tsx (2)
115-115:useEditableProjects()is called twice — once inProjectMenuand once inProjectDropdown.
ProjectMenuis rendered as a direct child ofProjectDropdown(line 215), so both invocations execute on every render. While the underlying data-fetching hooks may deduplicate network requests, theuseMemocomputations insideuseEditableProjects(includingcombinePersesAndOcpProjects,getEditableProjects, and the sort operations) still run twice.ProjectMenuonly needsallProjects, whichProjectDropdownalready holds.♻️ Proposed refactor — pass `allProjects` as a prop
const ProjectMenu: React.FC<{ setOpen: (isOpen: boolean) => void; onSelect: (event: React.MouseEvent, itemId: string) => void; selected?: string; menuRef: React.MutableRefObject<HTMLDivElement>; + allProjects: string[] | undefined; -}> = ({ setOpen, onSelect, selected, menuRef }) => { +}> = ({ setOpen, onSelect, selected, menuRef, allProjects }) => { const filterRef = useRef(null); const [filterText, setFilterText] = useState(''); - const { allProjects } = useEditableProjects(); ...const menuProps = { setOpen, onSelect, selected, menuRef, + allProjects, };Also applies to: 194-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx` at line 115, ProjectDropdown and ProjectMenu both call useEditableProjects(), causing duplicate expensive memo work; remove the extra call by having ProjectDropdown (which already calls useEditableProjects and holds allProjects) pass its allProjects down to ProjectMenu as a prop, update ProjectMenu to accept an allProjects prop and stop calling useEditableProjects itself (remove or replace const { allProjects } = useEditableProjects() in ProjectMenu), and update any references inside ProjectMenu that consumed its own allProjects to use the prop instead.
205-207:permissionsErrorcauses a silentnullreturn with no user feedback.When the permissions fetch fails, the entire dropdown disappears without any indication to the user. Depending on where
ProjectDropdownis rendered this could appear as a layout shift or missing UI element with no explanation. Consider rendering a disabled toggle or an inline error state instead ofnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx` around lines 205 - 207, The component currently returns null when permissionsError is true which hides the dropdown silently; update the ProjectDropdown component to handle permissionsError explicitly instead of the combined null-return check: keep the existing check for permissionsLoading or empty allProjects to return null, but when permissionsError is true render a disabled dropdown/toggle or an inline error state (e.g., a disabled ProjectDropdown control with an error tooltip or small error text) so users see feedback; locate the conditional using permissionsLoading, permissionsError, and allProjects in ProjectDropdown and replace the combined early return with an explicit branch that renders the disabled/error UI when permissionsError is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx`:
- Line 127: ProjectMenu currently inserts a hard-coded "All Projects" string
into items (items.unshift({ title: 'All Projects', key: '' })) which bypasses
i18n; fix by using the translation function—either call useTranslation() inside
ProjectMenu and replace the literal with t('All Projects'), or accept a
translated prop from ProjectDropdown (which already calls t('All Projects')) and
use that prop when unshifting the menu item; update the items.unshift call to
use the translated value so the menu item matches the toggle button.
---
Nitpick comments:
In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx`:
- Line 115: ProjectDropdown and ProjectMenu both call useEditableProjects(),
causing duplicate expensive memo work; remove the extra call by having
ProjectDropdown (which already calls useEditableProjects and holds allProjects)
pass its allProjects down to ProjectMenu as a prop, update ProjectMenu to accept
an allProjects prop and stop calling useEditableProjects itself (remove or
replace const { allProjects } = useEditableProjects() in ProjectMenu), and
update any references inside ProjectMenu that consumed its own allProjects to
use the prop instead.
- Around line 205-207: The component currently returns null when
permissionsError is true which hides the dropdown silently; update the
ProjectDropdown component to handle permissionsError explicitly instead of the
combined null-return check: keep the existing check for permissionsLoading or
empty allProjects to return null, but when permissionsError is true render a
disabled dropdown/toggle or an inline error state (e.g., a disabled
ProjectDropdown control with an error tooltip or small error text) so users see
feedback; locate the conditional using permissionsLoading, permissionsError, and
allProjects in ProjectDropdown and replace the combined early return with an
explicit branch that renders the disabled/error UI when permissionsError is set.
web/src/components/dashboards/perses/project/ProjectDropdown.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/components/dashboards/perses/project/ProjectDropdown.tsx (2)
114-114:useEditableProjectsis called twice in the same component tree.
ProjectMenu(line 114) andProjectDropdown(line 193) both invokeuseEditableProjects(). SinceProjectMenuis always rendered insideProjectDropdown, this runs the hook — and all its internaluseMemowork — twice per render. While React Query's cache prevents duplicate network requests, it is cleaner and cheaper to call the hook once inProjectDropdownand passallProjectsdown as a prop.♻️ Proposed refactor
const ProjectMenu: React.FC<{ setOpen: (isOpen: boolean) => void; onSelect: (event: React.MouseEvent, itemId: string) => void; selected?: string; menuRef: React.MutableRefObject<HTMLDivElement>; + allProjects: string[] | undefined; -}> = ({ setOpen, onSelect, selected, menuRef }) => { +}> = ({ setOpen, onSelect, selected, menuRef, allProjects }) => { const filterRef = useRef(null); const [filterText, setFilterText] = useState(''); - const { allProjects } = useEditableProjects(); ... }; const ProjectDropdown: React.FC<ProjectDropdownProps> = ({ ... }) => { const { allProjects, permissionsLoading, permissionsError } = useEditableProjects(); ... const menuProps = { setOpen, onSelect, selected, menuRef, + allProjects, }; ... };Also applies to: 193-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx` at line 114, ProjectDropdown currently calls useEditableProjects() and so does ProjectMenu, causing duplicate hook work; change ProjectDropdown to call useEditableProjects() once (retain const { allProjects } = useEditableProjects() in ProjectDropdown) and pass allProjects as a prop to ProjectMenu (e.g., <ProjectMenu allProjects={allProjects} .../>), then remove the useEditableProjects() call from ProjectMenu and update ProjectMenu’s props/type to accept allProjects (with the same shape it expects) and use that prop instead; ensure any default/fallback handling that ProjectMenu relied on from the hook is preserved when using the passed-in allProjects.
204-206:permissionsErrorsilently removes the dropdown with no user feedback.When
permissionsErroris truthy,ProjectDropdownreturnsnull, causing the project selector to silently vanish. Consider rendering a disabled toggle or an inline error state so users know why the selector disappeared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx` around lines 204 - 206, ProjectDropdown currently returns null when permissionsError is truthy (alongside permissionsLoading/allProjects checks), which silently hides the selector; update the render logic in ProjectDropdown to detect permissionsError and render a visible disabled/readonly toggle or inline error state instead of returning null—use the existing flags (permissionsError, permissionsLoading, allProjects) to conditionally render either a disabled dropdown or an error message component when permissionsError is set, while keeping the existing loading behavior when permissionsLoading is true and the normal dropdown when permissionsError is falsy and allProjects is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 173-181: The current conditional uses disabled to decide which
Tooltip to show, but disabled is true while permissions are still loading so
users see the "contact your cluster administrator" message during the "Checking
permissions…" state; update the rendering logic around createBtn/Tooltip to
check permissionsLoading first and, while permissionsLoading is true, show a
Tooltip (or no Tooltip) that indicates "Checking permissions…" (or disable the
Tooltip) instead of the admin contact message; locate the conditional that
references disabled, permissionsLoading, createBtn and Tooltip in
dashboard-create-dialog.tsx and adjust the branches so the permissionsLoading
case is handled before the disabled case.
---
Duplicate comments:
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 272-282: The button logic is using the non-existent .isPending on
React Query v4 which prevents loading/disabled states from working; update all
uses of createDashboardMutation.isPending and createProjectMutation.isPending to
the v4 property .isLoading (including the isDisabled check, the isLoading prop,
and the ternary that shows t('Creating...') vs t('Create')) so the button
reflects mutation loading state correctly.
- Around line 107-112: Wrap the toast messages in i18next translation calls and
use interpolation rather than template literals: replace the success and error
addAlert calls to use t('dashboardCreate.projectCreated', { project:
selectedProject }) and t('dashboardCreate.errorCreatingProject', { project:
selectedProject, error: errorMessage }) (or similar keys), ensure errorMessage
is the plain string passed into t, and import/use the useTranslation hook (t) in
the component so addAlert receives translated text; also add the corresponding
locale keys (dashboardCreate.projectCreated and
dashboardCreate.errorCreatingProject) to your i18n resource files.
In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx`:
- Line 126: ProjectMenu currently injects a hardcoded English label with
items.unshift({ title: 'All Projects', key: '' }), causing untranslated UI;
update ProjectMenu to use the translation hook or accept a translated label from
its parent — e.g., import and call useTranslation() inside ProjectMenu and
replace the literal with t('All Projects'), or add a prop (e.g.,
allProjectsLabel) and have ProjectDropdown pass t('All Projects') into
ProjectMenu to ensure consistent translated text; touch the ProjectMenu
function/component and the items.unshift call (and ProjectDropdown props where
t('All Projects') is already used) to apply the change.
---
Nitpick comments:
In `@web/src/components/dashboards/perses/project/ProjectDropdown.tsx`:
- Line 114: ProjectDropdown currently calls useEditableProjects() and so does
ProjectMenu, causing duplicate hook work; change ProjectDropdown to call
useEditableProjects() once (retain const { allProjects } = useEditableProjects()
in ProjectDropdown) and pass allProjects as a prop to ProjectMenu (e.g.,
<ProjectMenu allProjects={allProjects} .../>), then remove the
useEditableProjects() call from ProjectMenu and update ProjectMenu’s props/type
to accept allProjects (with the same shape it expects) and use that prop
instead; ensure any default/fallback handling that ProjectMenu relied on from
the hook is preserved when using the passed-in allProjects.
- Around line 204-206: ProjectDropdown currently returns null when
permissionsError is truthy (alongside permissionsLoading/allProjects checks),
which silently hides the selector; update the render logic in ProjectDropdown to
detect permissionsError and render a visible disabled/readonly toggle or inline
error state instead of returning null—use the existing flags (permissionsError,
permissionsLoading, allProjects) to conditionally render either a disabled
dropdown or an error message component when permissionsError is set, while
keeping the existing loading behavior when permissionsLoading is true and the
normal dropdown when permissionsError is falsy and allProjects is available.
web/src/components/dashboards/perses/dashboard-create-dialog.tsx
Outdated
Show resolved
Hide resolved
…e is used and update dashboard-create-dialog to use editableProjects array to check if the project already exists
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/dashboards/perses/dashboard-action-modals.tsx (1)
196-204:⚠️ Potential issue | 🔴 Critical
editableProjectsandallProjectscan beundefined— runtime TypeError.Per the
useEditableProjectshook, botheditableProjectsandallProjectsareundefinedwhen permissions are loading, missing, or errored. Line 199 callseditableProjects.includes(...)and line 203 accessesallProjects[0]without null checks, which will throw aTypeError.Proposed fix
const defaultProject = useMemo(() => { if (!dashboard) return ''; - if (dashboard.metadata.project && editableProjects.includes(dashboard.metadata.project)) { + if (dashboard.metadata.project && editableProjects?.includes(dashboard.metadata.project)) { return dashboard.metadata.project; } - return allProjects[0] || ''; + return allProjects?.[0] || ''; }, [dashboard, editableProjects, allProjects]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx` around lines 196 - 204, defaultProject's useMemo assumes editableProjects and allProjects are arrays but they can be undefined from useEditableProjects; update the logic in the useMemo (the defaultProject constant) to guard against undefined by treating editableProjects and allProjects as empty arrays when missing (e.g., fallback via nullable checks or defaulting to []), and use safe checks (e.g., Array.isArray or optional chaining + nullish coalescing) for the includes(...) and [0] access so no TypeError occurs when permissions are loading or errored.
🧹 Nitpick comments (2)
web/src/components/dashboards/perses/dashboard-action-modals.tsx (2)
369-382:TypeaheadSelectre-keys on every selection, causing full remount.Same issue as in
dashboard-create-dialog.tsx:key={selectedProjectName || 'no-selection'}(line 370) changes with each selection, forcing React to unmount/remount theTypeaheadSelect, losing internal state (focus, input text, scroll).Consider removing the dynamic key or using a stable one.
Proposed fix
<TypeaheadSelect - key={selectedProjectName || 'no-selection'} initialOptions={projectOptions} placeholder={t('Select namespace')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx` around lines 369 - 382, The TypeaheadSelect is being forcibly remounted because the dynamic key prop uses selectedProjectName (key={selectedProjectName || 'no-selection'}), which changes on each selection; remove this dynamic key or replace it with a stable constant key so the TypeaheadSelect component (in dashboard-action-modals.tsx) is not unmounted on selection—update the JSX for TypeaheadSelect to omit the changing key or use a fixed string (mirroring the fix applied in dashboard-create-dialog.tsx) so internal state (focus, input text, scroll) is preserved while keeping onSelect={onProjectSelect} and other props unchanged.
234-241:useEffectdependency oneditableProjects?.lengthis fragile — consider the full reference.Using
editableProjects?.lengthas a dependency means the effect won't re-run if the array contents change but the length stays the same (e.g., if a project is renamed or swapped). Also,formis included as a dependency which may cause unnecessary re-runs sinceuseFormreturns a stable reference only if configured correctly.This is a minor concern — the current usage (resetting on modal open with a new projects list) is likely fine in practice.
Suggested improvement
React.useEffect(() => { - if (isOpen && dashboard && editableProjects?.length > 0 && defaultProject) { + if (isOpen && dashboard && editableProjects && editableProjects.length > 0 && defaultProject) { form.reset({ projectName: defaultProject, dashboardName: '', }); } - }, [isOpen, dashboard, defaultProject, editableProjects?.length, form]); + }, [isOpen, dashboard, defaultProject, editableProjects, form]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx` around lines 234 - 241, The effect's dependency on editableProjects?.length is brittle; update the dependency list to watch the full array and the specific form method instead of the whole form object — e.g. use [isOpen, dashboard, defaultProject, editableProjects, form.reset] — so the effect re-runs when editableProjects content changes (renames/swaps) and avoid unnecessary re-runs caused by the unstable form reference; keep the effect body calling form.reset({ projectName: defaultProject, dashboardName: '' }) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 99-113: editableProjects can be undefined, so change the
projectExists check to guard/null-coalesce before calling .some — e.g. compute
projectExists using Array.isArray(editableProjects) && editableProjects.some(p
=> p === selectedProject) or (editableProjects ?? []).some(...) so you never
call .some on undefined; keep the rest of the create flow
(createProjectMutation.mutateAsync, addAlert, setFormErrors) unchanged.
- Around line 50-60: The TypeaheadSelect usage in dashboard-create-dialog.tsx is
passing an unsupported prop maxMenuHeight to the `@patternfly/react-templates`
TypeaheadSelect; remove the maxMenuHeight prop from the TypeaheadSelect JSX (or
replace the TypeaheadSelect with the `@patternfly/react-core` Select/MenuContent
pair if you need to control menu height) so that the component only uses
supported props; update any related props/state handling in the same component
to match the chosen approach (keep TypeaheadSelect without maxMenuHeight or swap
to Select and apply menu height there).
---
Outside diff comments:
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 196-204: defaultProject's useMemo assumes editableProjects and
allProjects are arrays but they can be undefined from useEditableProjects;
update the logic in the useMemo (the defaultProject constant) to guard against
undefined by treating editableProjects and allProjects as empty arrays when
missing (e.g., fallback via nullable checks or defaulting to []), and use safe
checks (e.g., Array.isArray or optional chaining + nullish coalescing) for the
includes(...) and [0] access so no TypeError occurs when permissions are loading
or errored.
---
Duplicate comments:
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 164-169: In RenameActionModal, DuplicateActionModal, and
DeleteActionModal replace all uses of the non-existent update/duplicate/delete
mutation property .isPending with the correct `@tanstack/react-query` v4 property
.isLoading (e.g., updateDashboardMutation.isPending ->
updateDashboardMutation.isLoading, same for the duplicate and delete mutation
objects) so buttons’ isDisabled and isLoading props behave correctly during
mutations.
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 269-279: Button loading/disable logic uses .isPending (v5) instead
of .isLoading (v4), so the submit never shows loading or disables; update
references in the DashboardCreateDialog component to use
createDashboardMutation.isLoading and createProjectMutation.isLoading wherever
.isPending is used (both in the isDisabled/isLoading props and in the
conditional that renders "Creating..." vs "Create") so the button correctly
disables and shows the loading state during mutations.
- Around line 93-96: Replace any user-facing template literals with i18next t()
calls using interpolation; e.g., change the setFormErrors call that currently
sets dashboardName to a template string to use t('Dashboard name
"{{dashboardName}}" already exists in this project', { dashboardName }), and
likewise update the toast.success/toast.error uses in the create project and
create dashboard flows to use t('Project "{{project}}" created successfully', {
project: selectedProject }), t('Failed to create project "{{project}}"', {
project: selectedProject }), t('Error creating project: {{errorMessage}}', {
errorMessage }), t('Dashboard "{{dashboardName}}" created successfully', {
dashboardName }), and t('Error creating dashboard: {{errorMessage}}', {
errorMessage }); ensure you import/use the same i18n t function already used in
this component and preserve existing variable names (dashboardName,
selectedProject, errorMessage) so the translator keys and interpolation objects
match.
- Around line 170-178: The tooltip telling users to contact their admin is shown
while permissionsLoading is true because disabled is set during loading; update
the rendering condition so the Tooltip wraps createBtn only when
permissionsLoading is false and disabled is true (e.g., use if
(!permissionsLoading && disabled) to render <Tooltip ...><span
...>{createBtn}</span></Tooltip>), and when permissionsLoading is true render
the disabled createBtn without that admin-contact tooltip (or alternatively
render a loading indicator), ensuring checks reference the existing disabled,
permissionsLoading, createBtn and Tooltip symbols.
---
Nitpick comments:
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 369-382: The TypeaheadSelect is being forcibly remounted because
the dynamic key prop uses selectedProjectName (key={selectedProjectName ||
'no-selection'}), which changes on each selection; remove this dynamic key or
replace it with a stable constant key so the TypeaheadSelect component (in
dashboard-action-modals.tsx) is not unmounted on selection—update the JSX for
TypeaheadSelect to omit the changing key or use a fixed string (mirroring the
fix applied in dashboard-create-dialog.tsx) so internal state (focus, input
text, scroll) is preserved while keeping onSelect={onProjectSelect} and other
props unchanged.
- Around line 234-241: The effect's dependency on editableProjects?.length is
brittle; update the dependency list to watch the full array and the specific
form method instead of the whole form object — e.g. use [isOpen, dashboard,
defaultProject, editableProjects, form.reset] — so the effect re-runs when
editableProjects content changes (renames/swaps) and avoid unnecessary re-runs
caused by the unstable form reference; keep the effect body calling form.reset({
projectName: defaultProject, dashboardName: '' }) as-is.
web/src/components/dashboards/perses/dashboard-create-dialog.tsx
Outdated
Show resolved
Hide resolved
Contributor
|
@zhuje: 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. |
…ist before creating / duplicating
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA
https://issues.redhat.com/browse/OU-1195
Kubeadmin Demo
https://github.com/user-attachments/assets/29d72d16-3d8a-424e-94c6-a1cebb3317d9
User Demo
https://github.com/user-attachments/assets/a026d4d4-8f48-4643-af96-7d781873ffaa
Perses Image (containing API updates to K8s auth)
quay.io/rh-ee-pyurkovi/perses:1.4.5based on perses/perses#3887
Monitoring-Console-Plugin Image
quay.io/jezhu/monitoring-console-plugin:ou1195-feb17-1009Summary by CodeRabbit
New Features
Bug Fixes
Chores