COO-1545: Backport customizable dashboards#775
Open
PeterYurkovich wants to merge 4 commits intoopenshift:release-coo-0.4from
Open
COO-1545: Backport customizable dashboards#775PeterYurkovich wants to merge 4 commits intoopenshift:release-coo-0.4from
PeterYurkovich wants to merge 4 commits intoopenshift:release-coo-0.4from
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references COO-1545 which is a valid jira issue. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a Perses-based dashboards feature with new UI components, APIs, permissions, theming, and DevSpace development artifacts; restructures routing and build tooling, introduces many new React modules, removes legacy skeleton and plugin loader, and updates packaging and webpack configuration. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich 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
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (3)
web/webpack.config.ts (1)
105-128:⚠️ Potential issue | 🟡 MinorConfigure SWC-loader for development builds.
The dev build uses
swc-loaderwithout any inline options and there is no.swcrcfile or swc configuration inpackage.json. SWC-loader requires explicit TypeScript/TSX parsing configuration to function correctly. Either add a.swcrcfile, add swc configuration topackage.json, or specify loader options inline. The production build's esbuild-loader is configured with explicit options and will work as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/webpack.config.ts` around lines 105 - 128, The development branch currently unshifts a rule using 'swc-loader' into config.module.rules without any options, but SWC needs explicit parser config for TypeScript/TSX; update the rule (the object added in the else branch that matches /\.(jsx?|tsx?)$/ and uses loader: 'swc-loader') to include an options object (or point to a .swcrc) that enables jsc.parser.syntax = 'typescript', jsc.parser.tsx = true for TSX support (and jsc.target or transform.react settings as needed), so SWC can correctly parse and transpile TS/TSX during development.web/src/components/dashboards/perses/datasource-api.ts (1)
47-63:⚠️ Potential issue | 🟠 MajorReturn type mismatch with throw behavior.
The return type is
Promise<DatasourceResource | undefined>but the function throws an error when no datasource is found instead of returningundefined. Callers expectingundefinedon no-match will receive an unhandled exception. Either update the return type to remove| undefinedor returnundefinedinstead of throwing.Option A: Update return type (if throwing is intended)
getDatasource = async ( project: string, selector: DatasourceSelector, - ): Promise<DatasourceResource | undefined> => { + ): Promise<DatasourceResource> => {Apply the same change to
getGlobalDatasourceat line 67.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/datasource-api.ts` around lines 47 - 63, The current getDatasource implementation declares Promise<DatasourceResource | undefined> but throws when no match; either make the function consistently throw by removing | undefined from its return type (update the signature of getDatasource and also getGlobalDatasource) or change the control flow to return undefined instead of throwing when fetchDatasourceList returns an empty/non-array result; locate getDatasource and getGlobalDatasource and adjust the return type or replace the throw new Error(...) with return undefined (and ensure callers handle the behavior).web/src/components/dashboards/perses/PersesWrapper.tsx (1)
378-384:⚠️ Potential issue | 🔴 CriticalEnable conditional querying based on dashboardName.
The
useFetchPersesDashboardhook hasenabled: truehardcoded, causing network requests even whendashboardNameis undefined (e.g., in list view). Change theenabledflag toenabled: !!dashboardNameto prevent invalid fetches and avoid blockingLoadingBoxwhen no dashboard is selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/PersesWrapper.tsx` around lines 378 - 384, The useFetchPersesDashboard call in InnerWrapper is triggering requests even when dashboardName is falsy; update the options passed to useFetchPersesDashboard so its enabled flag is conditional (enabled: !!dashboardName) instead of always true. Locate the useFetchPersesDashboard invocation in InnerWrapper and change the hook options to set enabled to !!dashboardName so persesDashboard and persesDashboardLoading only run when a dashboardName exists.
🧹 Nitpick comments (5)
web/src/components/metrics/promql-expression-input.tsx (1)
268-271: Consider restoring type safety for the fetch response.Using
anybypasses TypeScript's type checking for the API response. Since the code expectsresponse.datato be an array of strings (line 270-271), an inline type or interface would help catch API contract mismatches at compile time.♻️ Suggested improvement
- safeFetch<any>(`${PROMETHEUS_BASE_PATH}/${PrometheusEndpoint.LABEL}/__name__/values`) + safeFetch<{ data: string[] }>(`${PROMETHEUS_BASE_PATH}/${PrometheusEndpoint.LABEL}/__name__/values`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/metrics/promql-expression-input.tsx` around lines 268 - 271, The fetch currently uses any which erases type safety; define a response type (e.g. an interface with data: string[]) and use it with safeFetch (safeFetch<YourResponseType>(...)) when calling safeFetch for `${PROMETHEUS_BASE_PATH}/${PrometheusEndpoint.LABEL}/__name__/values`, then read response.data and pass it to setMetricNames; additionally add a minimal runtime guard (Array.isArray(response.data) && response.data.every(item => typeof item === 'string')) before calling setMetricNames to avoid runtime surprises.web/src/components/dashboards/perses/project/useActiveProject.tsx (1)
38-45: Remove unused dependencies from effect.
persesProjectsandperspectiveare in the dependency array but not used in the effect body. This causes unnecessary re-executions when those values change.♻️ Suggested cleanup
}, [ projectFromUrl, activeProject, - perspective, - persesProjects, persesProjectsLoading, setProject, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/project/useActiveProject.tsx` around lines 38 - 45, The effect inside useActiveProject includes unnecessary dependencies causing extra re-runs; remove persesProjects and perspective from the dependency array and leave only the values actually referenced in the effect body (projectFromUrl, activeProject, persesProjectsLoading, setProject). Update the dependency array in the useEffect call inside useActiveProject to match the variables used, or if ESLint complains, add a clear inline // eslint-disable-next-line react-hooks/exhaustive-deps comment with an explanation above the useEffect.web/src/components/dashboards/perses/hooks/useDashboardsData.ts (1)
53-66: Index-based comparison assumes stable API ordering.The
dashboardsChangedcheck compares dashboards by array index. If the API returns dashboards in a different order between calls, this will incorrectly report changes and recreate metadata unnecessarily. Consider comparing by a stable key (e.g.,metadata.name+metadata.project) using a Set or Map for order-independent comparison.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/hooks/useDashboardsData.ts` around lines 53 - 66, The current dashboardsChanged logic compares persesDashboards to prevDashboardsRef.current by index (in dashboardsChanged), which breaks when API ordering changes; instead compute a stable key for each dashboard (e.g., `${dashboard.metadata.name}::${dashboard.metadata.project}`) for both persesDashboards and prevDashboardsRef.current, build Sets (or Maps) of those keys, and compare the Sets for equality (and/or check sizes) to determine real changes; update the dashboardsChanged calculation and subsequent logic that uses dashboardsChanged/prevMetadataRef to be order-independent using these key Sets so metadata is only recreated when keys differ.web/src/components/dashboards/perses/dashboard-permissions.ts (1)
12-48: Consider batching permission checks for better performance.The current implementation makes 3 API calls per project sequentially through the loop. For users with many projects, this could result in noticeable latency. Consider batching all permission checks with a single
Promise.allacross all projects.♻️ Alternative approach for batching
+const checkProjectPermissions = async (projects: any[]): Promise<string[]> => { + if (!projects || projects.length === 0) { + return []; + } + + const permissionPromises = projects + .filter((project) => project?.metadata?.name) + .map(async (project) => { + const projectName = project.metadata.name; + try { + const [createResult, updateResult, deleteResult] = await Promise.all([ + checkAccess({ group: 'perses.dev', resource: 'persesdashboards', verb: 'create', namespace: projectName }), + checkAccess({ group: 'perses.dev', resource: 'persesdashboards', verb: 'update', namespace: projectName }), + checkAccess({ group: 'perses.dev', resource: 'persesdashboards', verb: 'delete', namespace: projectName }), + ]); + const canEdit = createResult?.status?.allowed && updateResult?.status?.allowed && deleteResult?.status?.allowed; + return canEdit ? projectName : null; + } catch { + return null; + } + }); + + const results = await Promise.all(permissionPromises); + return results.filter((name): name is string => name !== null); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-permissions.ts` around lines 12 - 48, The loop currently calls checkAccess three times per project (create/update/delete) causing many API calls; instead build a single array of permission promises for all projects using checkAccess (e.g., for each projectName push promises for verbs 'create','update','delete' into an array), await Promise.all on that array, then iterate the resolved results to reconstruct per-project permission triples and compute canEdit to push into editableProjectNames; preserve the try/catch around the global Promise.all and include projectName context when mapping results back to each project.web/src/components/dashboards/perses/dashboard-list.tsx (1)
378-395: Type mismatch:targetedDashboardcan be undefined.The modal components receive
targetedDashboardwhich has typeDashboardResource | undefined, butActionModalProps.dashboardexpectsDashboardResource. While the modals handleundefinedinternally, this creates a type safety gap.♻️ Suggested type alignment
Either update the
ActionModalPropsinterface to acceptDashboardResource | undefined:interface ActionModalProps { - dashboard: DashboardResource; + dashboard: DashboardResource | undefined; isOpen: boolean; onClose: () => void; handleModalClose: () => void; }Or only render modals when
targetedDashboardis defined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-list.tsx` around lines 378 - 395, The prop passed to RenameActionModal, DuplicateActionModal, and DeleteActionModal, targetedDashboard, is typed as DashboardResource | undefined but ActionModalProps.dashboard expects DashboardResource; fix by aligning types: either broaden ActionModalProps.dashboard to accept DashboardResource | undefined (update the ActionModalProps interface/type used by RenameActionModal, DuplicateActionModal, DeleteActionModal) or only render those modal components when targetedDashboard is non-undefined (wrap the three modal JSX elements in a conditional rendering that checks targetedDashboard). Ensure you update all related prop types or conditional checks consistently so TypeScript no longer reports the mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.devspace`:
- Around line 5-26: The Dockerfile currently sets USER 0 and never switches
back, causing the container to run as root; after the build steps (after RUN
make build-backend and before ENTRYPOINT ["make", "start-devspace-backend"]),
create a non-root user/group (or use an existing unprivileged UID/GID), chown
the app directories (/opt/app-root, /.devspace, and any copied files) to that
user, and switch to it via USER <non-root> so runtime executes as a non-root
user; reference the existing targets and symbols like RUN make install-backend,
RUN make build-backend, and ENTRYPOINT to place the user creation, chown, and
USER change in the correct spot.
In `@web/src/components/dashboards/perses/dashboard-action-modals.tsx`:
- Around line 99-104: The onError callback for the rename dashboard mutation is
re-throwing the error after showing an alert (the anonymous onError function
that builds msg via t(...) and calls addAlert(..., AlertVariant.danger)); remove
the trailing "throw err" so the mutation handler does not produce an unhandled
rejection—keep the t(...) message and addAlert(...) call as-is (optionally
replace throw with a debug log if you want to record the error but do not
re-throw).
- Around line 449-454: In the onError callback inside DeleteActionModal (the
onError: (err) => { ... } handler) remove the final "throw err" so the mutation
error is handled locally via addAlert(AlertVariant.danger) and not re-thrown;
simply construct and log the message with t(`Could not delete dashboard.
${err}`) and call addAlert(msg, AlertVariant.danger) then return/exit the
callback without throwing.
- Around line 82-88: processForm is directly mutating the dashboard prop by
assigning to dashboard.spec.display; instead create a new object copy and update
that before calling the mutation. In the
SubmitHandler<RenameDashboardValidationType> processForm, build a newSpec (e.g.,
{ ...dashboard.spec, display: { ...(dashboard.spec?.display || {}), name:
data.dashboardName } }) or a newDashboard (e.g., { ...dashboard, spec: newSpec
}) and pass that to the mutation; do not write to dashboard.spec.display
in-place so React props remain immutable.
In `@web/src/components/dashboards/perses/dashboard-action-validations.ts`:
- Line 90: The final return currently signals loading is still true; update the
returned object so that when returning refinedSchema the loading flag is false —
change isSchemaLoading: true to isSchemaLoading: false in the return that
includes refinedSchema (the object containing { schema: refinedSchema,
isSchemaLoading: ..., hasSchemaError: ... }) so it correctly reflects that
loading has completed.
- Around line 60-65: The early-return treats an empty dashboards array as still
loading; change the check so only undefined/null dashboards trigger the loading
state (e.g., test dashboards === undefined or dashboards == null) while an empty
array returns isSchemaLoading: false; keep schema produced by
createDashboardDialogValidationSchema(t) and hasSchemaError: false but set
isSchemaLoading to false for empty arrays so loading is only true when
dashboards is truly absent.
In `@web/src/components/dashboards/perses/dashboard-create-dialog.tsx`:
- Around line 106-119: The duplicate-name check in dashboard-create-dialog.tsx
compares existing dashboards' metadata names to the raw user input, causing
mismatches (e.g., "Café Test" vs "Cafe Test"); update the logic in the block
that uses dashboards.some(...) to compute the candidate metadata name by calling
generateMetadataName(dashboardName.trim()) and compare that generated name to
d.metadata.name (and ensure you still check d.metadata.project ===
selectedProject), then keep using setFormErrors to report the duplicate when the
generated names match.
In `@web/src/components/dashboards/perses/dashboard-list-frame.tsx`:
- Line 26: The Helmet invocation is passing raw text instead of a <title>
element, so replace the current usage of Helmet wrapping t('Metrics dashboards')
with a Helmet that contains a <title> child (e.g., Helmet -> title with the
result of t('Metrics dashboards')) so the document title is actually set; update
the Helmet usage in the DashboardListFrame (where Helmet and t('Metrics
dashboards') are used) accordingly.
In `@web/src/components/dashboards/perses/dashboard-page.tsx`:
- Around line 41-44: The component currently calls setActiveProject(urlProject)
during render which causes state updates during render; move this logic into a
useEffect that runs when urlProject or activeProject change: create a useEffect
in the dashboard component that checks if urlProject is truthy and different
from activeProject, then calls setActiveProject(urlProject). Reference the
existing identifiers urlProject, activeProject, and setActiveProject and ensure
the effect has [urlProject, activeProject] as dependencies to avoid
render-looping state updates.
- Around line 102-112: Remove the redundant outer ToastProvider wrapping in the
DashboardPage component: edit the DashboardPage React component to eliminate the
outer <ToastProvider> (the one that wraps <DashboardPage_ />) so only the inner
ToastProvider in DashboardFrame supplies the toast context; ensure
QueryParamProvider and QueryClientProvider remain intact and that DashboardPage_
(or DashboardFrame) continues to render unchanged.
In `@web/src/components/dashboards/perses/dashboard-permissions.ts`:
- Around line 38-39: The calculation of canEdit assumes
createResult.status.allowed (and same for updateResult/deleteResult) always
exist; add defensive validation before accessing nested properties: for
createResult, updateResult, deleteResult and their .status fields (results of
checkAccess), ensure each object is non-null and has a boolean allowed (e.g.,
typeof result?.status?.allowed === 'boolean') and treat missing/invalid shapes
as denied (false); update the canEdit assignment to use these guarded checks or
helper function (e.g., isAllowed(result)) so unexpected response shapes or
network errors won't throw.
In `@web/src/components/dashboards/perses/dashboard-toolbar.tsx`:
- Around line 199-201: The Alert in dashboard-toolbar.tsx contains a hardcoded
English message; wrap that string with the app's i18n translation function
(e.g., t('dashboard.managedViaCodeWarning') or i18n.t(...)) instead of the
literal text so it is localizable; update the JSX Alert (the Alert element
rendering "Dashboard managed via code only. Download JSON and commit changes to
save.") to call the translation key and add the new key to your translation
resource files, keeping the same message as the default value.
In `@web/src/components/dashboards/perses/hooks/usePerses.ts`:
- Around line 52-58: When a project is selected the code should prefer
project-specific query results over potentially stale all-dashboards cache;
update the returned fields so that when project is truthy you use
persesProjectDashboards (not persesDashboards) and likewise prefer
persesProjectDashboardsError and persesProjectDashboardsLoading; specifically
change persesDashboards to use (project ? persesProjectDashboards ?? [] :
persesDashboards ?? []) and change persesDashboardsError and
persesDashboardsLoading to choose
persesProjectDashboardsError/persesProjectDashboardsLoading when project is set
(falling back to the all-dashboards values otherwise).
In `@web/src/components/dashboards/perses/project/ProjectBar.tsx`:
- Around line 20-28: When building the URL in the project change handler, avoid
appending a bare question mark when there are no query params: only append
`?${params.toString()}` if `params.toString()` is non-empty. Update the logic
around `params`, `newProject`, `setActiveProject` and the call to `history.push`
(the block that constructs `const url =
`${getDashboardsListUrl(perspective)}?${params.toString()}`;`) so that it uses
the base `getDashboardsListUrl(perspective)` alone when `params` is empty, and
appends the `?` + params only when `params.toString()` yields a non-empty
string.
In `@web/src/components/dashboards/perses/ToastProvider.tsx`:
- Around line 51-52: The Alert is using a lookup AlertVariant[variant] which can
yield undefined for undefined or unexpected variant values; change the component
to pass the incoming variant prop directly to Alert (use variant rather than
AlertVariant[variant]) so the prop retains its correct AlertProps['variant']
type and avoids runtime errors; update the JSX where Alert is rendered in
ToastProvider.tsx (referencing the Alert component and the variant prop) to pass
variant directly and remove the indexed lookup.
---
Outside diff comments:
In `@web/src/components/dashboards/perses/datasource-api.ts`:
- Around line 47-63: The current getDatasource implementation declares
Promise<DatasourceResource | undefined> but throws when no match; either make
the function consistently throw by removing | undefined from its return type
(update the signature of getDatasource and also getGlobalDatasource) or change
the control flow to return undefined instead of throwing when
fetchDatasourceList returns an empty/non-array result; locate getDatasource and
getGlobalDatasource and adjust the return type or replace the throw new
Error(...) with return undefined (and ensure callers handle the behavior).
In `@web/src/components/dashboards/perses/PersesWrapper.tsx`:
- Around line 378-384: The useFetchPersesDashboard call in InnerWrapper is
triggering requests even when dashboardName is falsy; update the options passed
to useFetchPersesDashboard so its enabled flag is conditional (enabled:
!!dashboardName) instead of always true. Locate the useFetchPersesDashboard
invocation in InnerWrapper and change the hook options to set enabled to
!!dashboardName so persesDashboard and persesDashboardLoading only run when a
dashboardName exists.
In `@web/webpack.config.ts`:
- Around line 105-128: The development branch currently unshifts a rule using
'swc-loader' into config.module.rules without any options, but SWC needs
explicit parser config for TypeScript/TSX; update the rule (the object added in
the else branch that matches /\.(jsx?|tsx?)$/ and uses loader: 'swc-loader') to
include an options object (or point to a .swcrc) that enables jsc.parser.syntax
= 'typescript', jsc.parser.tsx = true for TSX support (and jsc.target or
transform.react settings as needed), so SWC can correctly parse and transpile
TS/TSX during development.
---
Nitpick comments:
In `@web/src/components/dashboards/perses/dashboard-list.tsx`:
- Around line 378-395: The prop passed to RenameActionModal,
DuplicateActionModal, and DeleteActionModal, targetedDashboard, is typed as
DashboardResource | undefined but ActionModalProps.dashboard expects
DashboardResource; fix by aligning types: either broaden
ActionModalProps.dashboard to accept DashboardResource | undefined (update the
ActionModalProps interface/type used by RenameActionModal, DuplicateActionModal,
DeleteActionModal) or only render those modal components when targetedDashboard
is non-undefined (wrap the three modal JSX elements in a conditional rendering
that checks targetedDashboard). Ensure you update all related prop types or
conditional checks consistently so TypeScript no longer reports the mismatch.
In `@web/src/components/dashboards/perses/dashboard-permissions.ts`:
- Around line 12-48: The loop currently calls checkAccess three times per
project (create/update/delete) causing many API calls; instead build a single
array of permission promises for all projects using checkAccess (e.g., for each
projectName push promises for verbs 'create','update','delete' into an array),
await Promise.all on that array, then iterate the resolved results to
reconstruct per-project permission triples and compute canEdit to push into
editableProjectNames; preserve the try/catch around the global Promise.all and
include projectName context when mapping results back to each project.
In `@web/src/components/dashboards/perses/hooks/useDashboardsData.ts`:
- Around line 53-66: The current dashboardsChanged logic compares
persesDashboards to prevDashboardsRef.current by index (in dashboardsChanged),
which breaks when API ordering changes; instead compute a stable key for each
dashboard (e.g., `${dashboard.metadata.name}::${dashboard.metadata.project}`)
for both persesDashboards and prevDashboardsRef.current, build Sets (or Maps) of
those keys, and compare the Sets for equality (and/or check sizes) to determine
real changes; update the dashboardsChanged calculation and subsequent logic that
uses dashboardsChanged/prevMetadataRef to be order-independent using these key
Sets so metadata is only recreated when keys differ.
In `@web/src/components/dashboards/perses/project/useActiveProject.tsx`:
- Around line 38-45: The effect inside useActiveProject includes unnecessary
dependencies causing extra re-runs; remove persesProjects and perspective from
the dependency array and leave only the values actually referenced in the effect
body (projectFromUrl, activeProject, persesProjectsLoading, setProject). Update
the dependency array in the useEffect call inside useActiveProject to match the
variables used, or if ESLint complains, add a clear inline //
eslint-disable-next-line react-hooks/exhaustive-deps comment with an explanation
above the useEffect.
In `@web/src/components/metrics/promql-expression-input.tsx`:
- Around line 268-271: The fetch currently uses any which erases type safety;
define a response type (e.g. an interface with data: string[]) and use it with
safeFetch (safeFetch<YourResponseType>(...)) when calling safeFetch for
`${PROMETHEUS_BASE_PATH}/${PrometheusEndpoint.LABEL}/__name__/values`, then read
response.data and pass it to setMetricNames; additionally add a minimal runtime
guard (Array.isArray(response.data) && response.data.every(item => typeof item
=== 'string')) before calling setMetricNames to avoid runtime surprises.
web/src/components/dashboards/perses/dashboard-action-modals.tsx
Outdated
Show resolved
Hide resolved
web/src/components/dashboards/perses/dashboard-action-validations.ts
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
web/src/components/dashboards/perses/datasource-api.ts (1)
47-63:⚠️ Potential issue | 🔴 CriticalAdd error handling for the new exception behavior.
The
getDatasourcemethod now throws when no datasource is found, butCachedDatasourceAPI.getDatasource(line 137) calls it without a.catch()handler. The promise rejection will propagate uncaught since:
- The cache logic at lines 138-145 explicitly checks for
undefinedreturns (old behavior)- The external
DatasourceStoreProviderlibrary (from@perses-dev/dashboards) was written expecting the original undefined-based contract, not exceptions- No error boundaries exist at call sites
Either add
.catch()error handling at line 137, or reconsider the exception-based approach to align with the existing API contract and consumer expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/datasource-api.ts` around lines 47 - 63, CachedDatasourceAPI.getDatasource currently calls getDatasource (which now throws when no datasource is found) without handling rejections; update CachedDatasourceAPI.getDatasource to call this.getDatasource(...).catch(err => { if (err.message?.includes('No matching datasource') || isNotFoundError(err)) return undefined; throw err; }) so it converts the new exception case back to the original undefined contract while still rethrowing unexpected errors; reference the methods getDatasource and CachedDatasourceAPI.getDatasource when making the change.
🧹 Nitpick comments (1)
web/src/components/dashboards/perses/dashboard-toolbar.tsx (1)
39-97: EditButton’s public props are defined but ignored.
label,disabled,disabledTooltip, andloadingare part ofEditButtonPropsyet not used, which makes the API misleading and harder to maintain. Either wire them through or remove them.♻️ Suggested wiring for the props
-export const EditButton = ({ onClick, activeProject }: EditButtonProps): React.ReactElement => { +export const EditButton = ({ + onClick, + activeProject, + label, + disabled: disabledProp, + disabledTooltip, + loading: loadingProp, +}: EditButtonProps): React.ReactElement => { const { t } = useTranslation(process.env.I18N_NAMESPACE); - const { canEdit, loading } = usePersesEditPermissions(activeProject); - const disabled = !canEdit; + const { canEdit, loading: permissionsLoading } = usePersesEditPermissions(activeProject); + const loading = loadingProp ?? permissionsLoading; + const disabled = disabledProp ?? !canEdit; const button = ( <Button onClick={onClick} startIcon={<PencilIcon />} variant="outlined" color="secondary" disabled={disabled || loading} sx={{ whiteSpace: 'nowrap', minWidth: 'auto' }} > - {loading ? t('Loading...') : t('Edit')} + {loading ? t('Loading...') : label ?? t('Edit')} </Button> ); if (disabled && !loading) { return ( - <Tooltip title={t("You don't have permission to edit this dashboard")} arrow> + <Tooltip title={disabledTooltip ?? t("You don't have permission to edit this dashboard")} arrow> <span>{button}</span> </Tooltip> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/perses/dashboard-toolbar.tsx` around lines 39 - 97, The props declared on EditButtonProps (label, disabled, disabledTooltip, loading, activeProject) are defined but not used; update the EditButton component to respect them: accept all props from EditButtonProps, use the label prop instead of the hard-coded t('Edit'), use the loading prop to override the loading value returned by usePersesEditPermissions (fall back to hook value when prop is undefined), use the disabled prop to override the computed disabled (computedDisabled = typeof disabled === 'boolean' ? disabled : !canEdit), and use disabledTooltip if provided for the Tooltip text (fall back to the current permission message); keep activeProject passed into usePersesEditPermissions and preserve other behavior (startIcon, variant, disabled state combining computedDisabled || loading).
🤖 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 70-113: The form's defaultValues passed to useForm (in
useForm<RenameDashboardValidationType>) are only read on initial render so when
dashboard or isOpen changes the input can be stale; call form.reset(...) with
the current dashboard name when dashboard or isOpen changes (e.g. inside a
useEffect that depends on [dashboard, isOpen]) to set the dashboardName to
getResourceDisplayName(dashboard) (or '' when no dashboard), while keeping
existing handleClose which already calls form.reset().
In `@web/src/components/dashboards/perses/dashboard-action-validations.ts`:
- Around line 35-93: The code currently allows t to be undefined and then calls
it unguarded; fix by ensuring a safe fallback translator is used inside
useDashboardValidationSchema: create a local safeT = t ?? ((key: string, opts?:
any) => { if (!opts) return key; replace placeholders in key with values from
opts or return a simple interpolated string }); then pass safeT into
createDashboardDialogValidationSchema and use safeT in the refine message
construction (replace all uses of t with safeT) so callers can omit t without
runtime errors; reference useDashboardValidationSchema,
createDashboardDialogValidationSchema, and the refine message callback when
applying the change.
In `@web/src/components/dashboards/perses/dashboard-list.tsx`:
- Around line 45-93: DashboardActionsCell currently calls
usePersesEditPermissions(project) per row causing many redundant permission
checks; remove that hook from DashboardActionsCell and instead accept permission
props (e.g., canEdit: boolean and optionally loading: boolean) so the parent
DashboardsTable can call useProjectPermissions once and pass per-row canEdit.
Concretely: update DashboardActionsCell's props to include canEdit (and loading
if needed), delete the usePersesEditPermissions call and the derived disabled
variable, use the incoming canEdit/loading to decide whether to render the
Tooltip + disabled ActionsColumn or the enabled ActionsColumn, and finally
update DashboardsTable to call useProjectPermissions(project) once, derive
canEdit for the table rows, and pass it into each DashboardActionsCell instance.
Ensure rowSpecificActions, ActionsColumn, and Tooltip behavior stay the same.
In `@web/src/components/dashboards/perses/dashboard-toolbar.tsx`:
- Around line 128-201: The component OCPDashboardToolbar uses t(...) but never
defines it; fix by importing useTranslation from 'react-i18next' and calling
const { t } = useTranslation() inside OCPDashboardToolbar (e.g., alongside
useEditMode/useTimeZoneParams hooks) so t is defined before the JSX renders;
ensure the import and the hook call are added to the top of the component file.
---
Outside diff comments:
In `@web/src/components/dashboards/perses/datasource-api.ts`:
- Around line 47-63: CachedDatasourceAPI.getDatasource currently calls
getDatasource (which now throws when no datasource is found) without handling
rejections; update CachedDatasourceAPI.getDatasource to call
this.getDatasource(...).catch(err => { if (err.message?.includes('No matching
datasource') || isNotFoundError(err)) return undefined; throw err; }) so it
converts the new exception case back to the original undefined contract while
still rethrowing unexpected errors; reference the methods getDatasource and
CachedDatasourceAPI.getDatasource when making the change.
---
Duplicate comments:
In `@web/src/components/dashboards/perses/datasource-api.ts`:
- Around line 65-77: getGlobalDatasource currently assumes
fetchGlobalDatasourceList returns a non-empty array; mirror the same
contract/verification used elsewhere by explicitly validating the response from
fetchGlobalDatasourceList(selector.kind, selector.name ? undefined : true,
selector.name) (check Array.isArray(list) and list.length > 0) and, on failure,
throw the same translated Error via this.t('No matching datasource found'); also
ensure the returned value matches the expected GlobalDatasourceResource type by
selecting list[0] and consider adding a guard/error if multiple entries are
returned when selector.name was provided so the function's contract remains
consistent with other datasource lookup helpers (refer to getGlobalDatasource
and fetchGlobalDatasourceList).
In `@web/src/components/dashboards/perses/project/ProjectBar.tsx`:
- Around line 7-9: Update ProjectBarProps so activeProject and its setter are
nullable (use string | null): change setActiveProject to
React.Dispatch<React.SetStateAction<string | null>> and activeProject to string
| null so calling setActiveProject(null) is type-safe; and fix the URL building
where params.toString() is appended (look for code using params.toString() in
ProjectBar or handlers) to prepend a '?' only when the query string is non-empty
(e.g., const qs = params.toString(); const path = base + (qs ? '?' + qs : '')).
---
Nitpick comments:
In `@web/src/components/dashboards/perses/dashboard-toolbar.tsx`:
- Around line 39-97: The props declared on EditButtonProps (label, disabled,
disabledTooltip, loading, activeProject) are defined but not used; update the
EditButton component to respect them: accept all props from EditButtonProps, use
the label prop instead of the hard-coded t('Edit'), use the loading prop to
override the loading value returned by usePersesEditPermissions (fall back to
hook value when prop is undefined), use the disabled prop to override the
computed disabled (computedDisabled = typeof disabled === 'boolean' ? disabled :
!canEdit), and use disabledTooltip if provided for the Tooltip text (fall back
to the current permission message); keep activeProject passed into
usePersesEditPermissions and preserve other behavior (startIcon, variant,
disabled state combining computedDisabled || loading).
PeterYurkovich
force-pushed
the
backport-dashboards-to-coo-0.4
branch
from
3cb2368 to
4dd6f18
Compare
|
@PeterYurkovich: This pull request references COO-1545 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@PeterYurkovich: This pull request references COO-1545 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
@PeterYurkovich: 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
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.
This PR looks to backport the customizable dashboards available on main to the release-coo-0.4 branch, which still uses Patternfly 5. Most of the components have been kept as close as possible to their current state on the main branch, with some changes needed for the translation to PF-5 and how the monitoring plugin codebase is structured on release-coo-0.4.
Demo
Screencast.From.2026-02-18.10-55-14.mp4