OCPBUGS-77113: feat: re enable dev console views#831
OCPBUGS-77113: feat: re enable dev console views#831jgbernalp wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jgbernalp: This pull request references Jira Issue OCPBUGS-77113, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/cherry-pick release-4.21 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp 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
|
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:
WalkthroughReplace dev-monitoring redirect routes with direct page components and new console.tab entries; remove DevRedirects; add MonitoringContext flag for namespace selector; implement route/query-aware namespace handling and tenancy-aware filtering across alerts/silences; make Metrics and Legacy Dashboards pages optionally hide namespace UI. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/alerting/AlertingPage.tsx (1)
98-105:⚠️ Potential issue | 🟠 MajorNamespace selector is now visible on tabs that still ignore it.
This condition also enables the selector for
RuleResource.url, butweb/src/components/alerting/AlertRulesPage.tsxstill consumesrulesdirectly and does not apply the selected namespace in the non-tenancy/admin path. For cluster admins, changing the namespace here will update the control without actually narrowing the rules list. Either scope the always-visible selector to the alerts tab for now, or add matching namespace filtering to the other alerting tabs first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/AlertingPage.tsx` around lines 98 - 105, The NamespaceBar is being shown for pages that ignore namespace (e.g., RuleResource.url) causing a mismatch with AlertRulesPage which still consumes unfiltered rules; either restrict the selector to only the alerts tab or add namespace-filtering in the rules consumers. Fix by changing the render condition in AlertingPage.tsx (where NamespaceBar is rendered) to only show for the alerts tab identifier (instead of namespacedPages.includes(pathname)), or alternatively implement namespace-aware filtering inside AlertRulesPage.tsx (and any component that uses rules directly) so they filter the rules by the selected namespace before consumption; keep the existing dispatch(alertingClearSelectorData(prometheus, namespace)) and setNamespace(namespace) behavior.
🤖 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/MetricsPage.tsx`:
- Around line 1406-1417: Summary: The page title is being replaced with
whitespace when displayNamespaceSelector is false, removing DocumentTitle and
causing an empty heading; only NamespaceBar should be conditional. Fix: always
render DocumentTitle and ListPageHeader with t('Metrics') (do not pass ' '), and
move the NamespaceBar conditional so only NamespaceBar is wrapped by
displayNamespaceSelector; keep NamespaceBar's onNamespaceChange handler
(dispatch(queryBrowserDeleteAllQueries()) and setNamespace) unchanged. Update
the JSX around DocumentTitle, NamespaceBar, and ListPageHeader (symbols:
DocumentTitle, NamespaceBar, ListPageHeader, displayNamespaceSelector,
setNamespace, queryBrowserDeleteAllQueries, dispatch) so the title is always
present and only the NamespaceBar is conditional.
---
Outside diff comments:
In `@web/src/components/alerting/AlertingPage.tsx`:
- Around line 98-105: The NamespaceBar is being shown for pages that ignore
namespace (e.g., RuleResource.url) causing a mismatch with AlertRulesPage which
still consumes unfiltered rules; either restrict the selector to only the alerts
tab or add namespace-filtering in the rules consumers. Fix by changing the
render condition in AlertingPage.tsx (where NamespaceBar is rendered) to only
show for the alerts tab identifier (instead of
namespacedPages.includes(pathname)), or alternatively implement namespace-aware
filtering inside AlertRulesPage.tsx (and any component that uses rules directly)
so they filter the rules by the selected namespace before consumption; keep the
existing dispatch(alertingClearSelectorData(prometheus, namespace)) and
setNamespace(namespace) behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9b54a84d-cc9f-4f01-a0b2-f57c86b35803
📒 Files selected for processing (9)
web/console-extensions.jsonweb/package.jsonweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsPage.tsxweb/src/components/metrics/promql-expression-input.tsxweb/src/components/redirects/dev-redirects.tsxweb/src/contexts/MonitoringContext.tsxweb/src/hooks/useMonitoring.ts
💤 Files with no reviewable changes (2)
- web/package.json
- web/src/components/redirects/dev-redirects.tsx
|
/test e2e-aws-ovn |
jgbernalp
force-pushed
the
re-enable-dev-monitoring
branch
from
51e303c to
dbd22be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
web/src/components/alerting/SilencesPage.tsx (1)
219-224: Consider usingnamespacedSilencesforunfilteredDatato maintain consistency.The
unfilteredDataprop is set tosilences?.data(all silences), whiledatauses the namespace-filteredfilteredData. This may cause the table to show a count mismatch (e.g., "Showing 5 of 100") when viewing a specific namespace.💡 Suggested fix
data={filteredData ?? []} loaded={!!silences?.loaded} loadError={silences?.loadError ?? ''} Row={SilenceTableRowWithCheckbox} - unfilteredData={silences?.data ?? []} + unfilteredData={namespacedSilences ?? []}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/SilencesPage.tsx` around lines 219 - 224, The table's unfilteredData currently uses silences?.data while data uses filteredData, causing counts to mismatch; update the unfilteredData prop to use namespacedSilences (or namespacedSilences ?? []) instead of silences?.data so the unfiltered dataset matches the namespace-filtered data source used by filteredData; ensure the change is applied where Row is SilenceTableRowWithCheckbox and loaded/loadError props remain unchanged.web/src/components/alerting/AlertsPage.tsx (1)
52-56: Ensuretriggerreference stability or document the intentional behavior.The
useEffectdeliberately excludestriggerfrom dependencies. This is fine iftriggeris stable or if calling it only on mount is the desired behavior. Consider adding a brief comment explaining why re-triggering ontriggerchanges is not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/AlertsPage.tsx` around lines 52 - 56, The useEffect call invoking trigger omits trigger from its dependency array; either ensure trigger is a stable reference (e.g., memoize it where defined with useCallback or return the same instance from the hook/provider that supplies it) so calling it only on mount is safe, or add a short inline comment next to the useEffect explaining that the omission is intentional and that trigger is expected to be stable/only needed on initial mount; update references to useEffect and trigger accordingly so reviewers understand the chosen approach.
🤖 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/alerting/AlertsPage.tsx`:
- Around line 131-138: The filter for namespacedAlerts can throw if an alert's
labels is undefined; update the filtering logic used to compute namespacedAlerts
so it safely checks labels before accessing namespace (e.g., use optional
chaining or an explicit guard when comparing a.labels?.namespace to namespace)
and ensure the same safe check applies wherever namespacedAlerts is passed into
useListPageFilter (referencing variables/functions namespacedAlerts, alerts,
rowFilters, and useListPageFilter to locate the code).
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 102-115: The current namespacedSilences calculation excludes
silences that lack a 'namespace' matcher when a specific namespace is selected
and also calls filter on silences?.data without a safe default; update the
namespacedSilences logic so that when namespace !== ALL_NAMESPACES_KEY it
returns silences?.data (or an empty array) filtered to include either silences
that have a namespace matcher equal to namespace OR silences that have no
namespace matcher (cluster-wide), and ensure you use a safe default (e.g.,
silences?.data ?? []) before calling filter; this affects the namespacedSilences
constant used by useListPageFilter.
---
Nitpick comments:
In `@web/src/components/alerting/AlertsPage.tsx`:
- Around line 52-56: The useEffect call invoking trigger omits trigger from its
dependency array; either ensure trigger is a stable reference (e.g., memoize it
where defined with useCallback or return the same instance from the
hook/provider that supplies it) so calling it only on mount is safe, or add a
short inline comment next to the useEffect explaining that the omission is
intentional and that trigger is expected to be stable/only needed on initial
mount; update references to useEffect and trigger accordingly so reviewers
understand the chosen approach.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 219-224: The table's unfilteredData currently uses silences?.data
while data uses filteredData, causing counts to mismatch; update the
unfilteredData prop to use namespacedSilences (or namespacedSilences ?? [])
instead of silences?.data so the unfiltered dataset matches the
namespace-filtered data source used by filteredData; ensure the change is
applied where Row is SilenceTableRowWithCheckbox and loaded/loadError props
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3a5ebad6-fefe-48a2-ac6b-4e1bf33fc2c6
📒 Files selected for processing (10)
web/console-extensions.jsonweb/package.jsonweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/metrics/promql-expression-input.tsxweb/src/components/redirects/dev-redirects.tsxweb/src/contexts/MonitoringContext.tsxweb/src/hooks/useMonitoring.ts
💤 Files with no reviewable changes (2)
- web/package.json
- web/src/components/redirects/dev-redirects.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/contexts/MonitoringContext.tsx
- web/src/components/metrics/promql-expression-input.tsx
jgbernalp
force-pushed
the
re-enable-dev-monitoring
branch
2 times, most recently
from
327ff9c to
37d6d41
Compare
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@jgbernalp: This pull request references Jira Issue OCPBUGS-77113, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
There was a problem hiding this comment.
Actionable comments posted: 3
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/legacy/useLegacyDashboards.ts (1)
131-139:⚠️ Potential issue | 🟠 MajorNamespace-only switches can leave the URL on the old scope.
This callback now builds a namespace-scoped
url, but the guard still skipsnavigate()whenever the dashboard name is unchanged. After Line 186 forces a reload on namespace changes, switching namespaces while staying on the same board will update local state without updating the browser URL, so refresh/deep-linking lands back in the previous namespace. Please include namespace/current location in the navigation guard instead of checking onlyQueryParams.Dashboard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboards/legacy/useLegacyDashboards.ts` around lines 131 - 139, The guard that currently prevents navigate() when only the dashboard name is unchanged should also account for namespace/location changes: update the conditional around navigate(url, { replace: true }) (referencing getLegacyDashboardsUrl, newBoard, urlBoard, initialLoad, namespace, perspective, params) to compare the current URL/namespace as well (e.g., check params.get(QueryParams.Namespace) or compare the full constructed url to the current location) so that changing namespace triggers navigate even when QueryParams.Dashboard equals newBoard; keep the existing initialLoad behavior.
♻️ Duplicate comments (1)
web/src/components/alerting/SilencesPage.tsx (1)
107-120:⚠️ Potential issue | 🟠 MajorApply full silence-matcher semantics when scoping by namespace.
This only keeps silences whose
namespacematcher is a literal equality match. Cluster-wide silences and regex/negative namespace matchers will disappear from the namespaced view even though they can still affect that namespace. Please evaluate the namespace matchers the same way the alerting code already handlesisRegex/isEqual, and keep the table's baseunfilteredDataon that same scoped collection.💡 Suggested shape for the namespace filter
+ const matchesNamespace = useCallback((silence: Silence, ns: string) => { + const namespaceMatchers = silence.matchers.filter((m) => m.name === 'namespace'); + if (namespaceMatchers.length === 0) { + return true; + } + return namespaceMatchers.every((matcher) => { + const isMatch = matcher.isRegex + ? new RegExp(`^${matcher.value}$`).test(ns) + : matcher.value === ns; + return matcher.isEqual === false && ns ? !isMatch : isMatch; + }); + }, []); + const namespacedSilences = ALL_NAMESPACES_KEY === namespace ? silences?.data - : silences?.data?.filter((s) => - s.matchers.some((m) => m.name === 'namespace' && m.value === namespace), - ); + : (silences?.data ?? []).filter((s) => matchesNamespace(s, namespace)); - unfilteredData={silences?.data ?? []} + unfilteredData={namespacedSilences ?? []}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/SilencesPage.tsx` around lines 107 - 120, The current namespacedSilences filter only keeps silences with a namespace matcher that equals the namespace literal; change it to reuse the same matcher semantics as the alerting code: when ALL_NAMESPACES_KEY === namespace keep silences?.data (including those without namespace matchers), otherwise keep any silence where either it has no namespace matcher (cluster-wide) or at least one matcher with name === 'namespace' that matches the requested namespace according to matcher.isRegex and matcher.isEqual semantics (if isRegex -> test the regex, if not isRegex -> compare equality and respect isEqual for negation). Update the namespacedSilences computation (referencing namespacedSilences, ALL_NAMESPACES_KEY, useListPageFilter, and s.matchers) to perform that evaluation so the table's unfilteredData is the correctly scoped collection.
🧹 Nitpick comments (3)
web/src/components/hooks/useQueryNamespace.ts (1)
19-24: Make namespace syncing opt-in instead of implicit.
useQueryNamespace()is now doing a globalsetActiveNamespace()from every consumer. In this PR it is also used from leaf components likeweb/src/components/alerting/SilencesUtils.tsxandweb/src/components/console/graphs/promethues-graph.tsx, so a single page can enqueue the same namespace sync many times before the first state update lands. Please split this into a pure read hook plus a page-level sync hook, or otherwise make the sync behavior opt-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/hooks/useQueryNamespace.ts` around lines 19 - 24, useQueryNamespace currently calls setActiveNamespace implicitly in its useEffect (using routeNamespace, queryNamespace, activeNamespace), causing multiple components to trigger global sync; change it to be a pure read hook that returns the resolved namespace (routeNamespace || queryNamespace) and do NOT call setActiveNamespace inside the hook by default, and then provide an opt-in sync mechanism (either add a second export/useSyncQueryNamespace hook or a boolean param like useQueryNamespace({sync: true}) that performs the setActiveNamespace call). Update callers (e.g., places using useQueryNamespace in SilencesUtils.tsx and promethues-graph.tsx) to call the opt-in sync hook or invoke the returned sync function from page-level components only, leaving leaf components to just read the namespace.web/src/components/alerting/AlertRulesPage.tsx (1)
86-102: Avoid running namespace synchronization in every virtualized row.
RuleTableRowis rendered per table row, butuseQueryNamespace()is not a pure read — it also syncs the active namespace. Hoisting the hook toAlertRulesPage_and threadingnamespaceinto the row renderer avoids repeated side effects while the table mounts/re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/AlertRulesPage.tsx` around lines 86 - 102, RuleTableRow currently calls useQueryNamespace() which triggers namespace synchronization on every virtualized row render; hoist the hook into the parent component AlertRulesPage_ (call useQueryNamespace() there) and pass the resolved namespace as a prop into RuleTableRow (remove useQueryNamespace() from RuleTableRow). Update RuleTableRow's signature (RowProps<Rule>) to accept the namespace prop and use that namespace when calling getRuleUrl(perspective, obj, namespace), ensuring no namespace-sync side effects happen per row.web/src/components/alerting/AlertList/AlertTableRow.tsx (1)
31-39: HoistuseQueryNamespace()out of the row renderer.
AlertTableRowis instantiated once per alert, butuseQueryNamespace()also performs active-namespace synchronization. Using it here turns one namespace lookup into N identical effects on large tables. Resolvenamespacein the parent list/page component and pass it intoAlertTableRowinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/AlertList/AlertTableRow.tsx` around lines 31 - 39, AlertTableRow currently calls useQueryNamespace() inside the row renderer which causes duplicate active-namespace sync effects; remove the useQueryNamespace() call from AlertTableRow and instead accept namespace as a prop (update AlertTableRow: FC<{ alert: Alert; namespace: string }>) and use that prop where namespace is needed; resolve namespace once in the parent list/page component by calling useQueryNamespace() there and pass the resulting namespace into each AlertTableRow instance (update all callers/iterators that render AlertTableRow to pass the new namespace prop).
🤖 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/hooks/usePerspective.tsx`:
- Around line 65-70: The dev branches insert raw namespace into URLs; add a
single helper in this module (e.g., normalizeNamespace or encodeNamespace) that
returns encodeURIComponent(namespace) when namespace is truthy and a safe
fallback (e.g., '' or 'all-namespaces' per app convention) when undefined, and
use that helper everywhere you build dev URLs (start with getAlertsUrl and
replace all other dev cases in this file) to avoid emitting unencoded '#' or
'/ns/undefined' fragments—update all dev-case string interpolations to use the
helper instead of directly using the namespace variable.
In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx`:
- Line 4: The dev rule link is missing a concrete namespace causing URLs like
/dev-monitoring/ns/undefined/...; update the link generation to pass a
guaranteed namespace to getRuleUrl (use alertDetails.namespace or another
validated namespace) instead of relying on the unconditional
setNamespace(ALL_NAMESPACES_KEY); ensure you only fallback to ALL_NAMESPACES_KEY
when not in the dev perspective and adjust any other calls around getRuleUrl and
setNamespace (also update the similar occurrences at the other noted locations)
so dev URLs receive a real namespace value.
In `@web/src/components/MetricsPage.tsx`:
- Around line 1326-1327: The namespace change reset is only performed in
NamespaceBar.onNamespaceChange so shell-driven changes (when
displayNamespaceSelector is false) leave old queries intact; update the logic
that handles namespace resolution (the hook/useEffect that reads the resolved
namespace used by useQueryNamespace and displayNamespaceSelector) to call
setNamespace(...) and also clear/reset the query browser state (same reset you
call inside NamespaceBar.onNamespaceChange) whenever the resolved namespace
changes externally; locate the namespace handling around
useQueryNamespace/setNamespace and the NamespaceBar onNamespaceChange code (also
mirror the fix for the similar block around lines referenced 1406-1414) and
ensure both paths invoke the same query-clear routine so predefined dev queries
don’t retain the old namespace literal.
---
Outside diff comments:
In `@web/src/components/dashboards/legacy/useLegacyDashboards.ts`:
- Around line 131-139: The guard that currently prevents navigate() when only
the dashboard name is unchanged should also account for namespace/location
changes: update the conditional around navigate(url, { replace: true })
(referencing getLegacyDashboardsUrl, newBoard, urlBoard, initialLoad, namespace,
perspective, params) to compare the current URL/namespace as well (e.g., check
params.get(QueryParams.Namespace) or compare the full constructed url to the
current location) so that changing namespace triggers navigate even when
QueryParams.Dashboard equals newBoard; keep the existing initialLoad behavior.
---
Duplicate comments:
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 107-120: The current namespacedSilences filter only keeps silences
with a namespace matcher that equals the namespace literal; change it to reuse
the same matcher semantics as the alerting code: when ALL_NAMESPACES_KEY ===
namespace keep silences?.data (including those without namespace matchers),
otherwise keep any silence where either it has no namespace matcher
(cluster-wide) or at least one matcher with name === 'namespace' that matches
the requested namespace according to matcher.isRegex and matcher.isEqual
semantics (if isRegex -> test the regex, if not isRegex -> compare equality and
respect isEqual for negation). Update the namespacedSilences computation
(referencing namespacedSilences, ALL_NAMESPACES_KEY, useListPageFilter, and
s.matchers) to perform that evaluation so the table's unfilteredData is the
correctly scoped collection.
---
Nitpick comments:
In `@web/src/components/alerting/AlertList/AlertTableRow.tsx`:
- Around line 31-39: AlertTableRow currently calls useQueryNamespace() inside
the row renderer which causes duplicate active-namespace sync effects; remove
the useQueryNamespace() call from AlertTableRow and instead accept namespace as
a prop (update AlertTableRow: FC<{ alert: Alert; namespace: string }>) and use
that prop where namespace is needed; resolve namespace once in the parent
list/page component by calling useQueryNamespace() there and pass the resulting
namespace into each AlertTableRow instance (update all callers/iterators that
render AlertTableRow to pass the new namespace prop).
In `@web/src/components/alerting/AlertRulesPage.tsx`:
- Around line 86-102: RuleTableRow currently calls useQueryNamespace() which
triggers namespace synchronization on every virtualized row render; hoist the
hook into the parent component AlertRulesPage_ (call useQueryNamespace() there)
and pass the resolved namespace as a prop into RuleTableRow (remove
useQueryNamespace() from RuleTableRow). Update RuleTableRow's signature
(RowProps<Rule>) to accept the namespace prop and use that namespace when
calling getRuleUrl(perspective, obj, namespace), ensuring no namespace-sync side
effects happen per row.
In `@web/src/components/hooks/useQueryNamespace.ts`:
- Around line 19-24: useQueryNamespace currently calls setActiveNamespace
implicitly in its useEffect (using routeNamespace, queryNamespace,
activeNamespace), causing multiple components to trigger global sync; change it
to be a pure read hook that returns the resolved namespace (routeNamespace ||
queryNamespace) and do NOT call setActiveNamespace inside the hook by default,
and then provide an opt-in sync mechanism (either add a second
export/useSyncQueryNamespace hook or a boolean param like
useQueryNamespace({sync: true}) that performs the setActiveNamespace call).
Update callers (e.g., places using useQueryNamespace in SilencesUtils.tsx and
promethues-graph.tsx) to call the opt-in sync hook or invoke the returned sync
function from page-level components only, leaving leaf components to just read
the namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 048aaaf3-00de-450c-8997-e43acbdcd68b
📒 Files selected for processing (27)
web/console-extensions.jsonweb/package.jsonweb/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsPage.tsxweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertDetail/SilencedByTable.tsxweb/src/components/alerting/AlertList/AggregateAlertTableRow.tsxweb/src/components/alerting/AlertList/AlertTableRow.tsxweb/src/components/alerting/AlertRulesDetailsPage.tsxweb/src/components/alerting/AlertRulesPage.tsxweb/src/components/alerting/AlertUtils.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsDetailsPage.tsxweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilenceForm.tsxweb/src/components/alerting/SilencesDetailsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/SilencesUtils.tsxweb/src/components/console/graphs/promethues-graph.tsxweb/src/components/dashboards/legacy/legacy-dashboard.tsxweb/src/components/dashboards/legacy/useLegacyDashboards.tsweb/src/components/hooks/usePerspective.tsxweb/src/components/hooks/useQueryNamespace.tsweb/src/components/metrics/promql-expression-input.tsxweb/src/components/redirects/dev-redirects.tsxweb/src/contexts/MonitoringContext.tsxweb/src/hooks/useMonitoring.ts
💤 Files with no reviewable changes (2)
- web/src/components/redirects/dev-redirects.tsx
- web/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/hooks/useMonitoring.ts
- web/src/components/alerting/AlertsPage.tsx
- web/src/contexts/MonitoringContext.tsx
| export const getAlertsUrl = (perspective: Perspective, namespace?: string) => { | ||
| switch (perspective) { | ||
| case 'acm': | ||
| return `/multicloud${AlertResource.url}`; | ||
| case 'dev': | ||
| return `/dev-monitoring/ns/${namespace}/alerts`; |
There was a problem hiding this comment.
Encode and guard namespace before composing dev URLs.
Every dev branch here inserts namespace raw into the path or query string. That breaks as soon as the value is the console sentinel #ALL_NS# because # starts a fragment, and the optional parameter also leaves these helpers free to emit /ns/undefined/... while the namespace is still unresolved. Please normalize the namespace once in this module—encodeURIComponent for path/query usage plus a safe fallback when it is missing—and reuse that helper across all dev cases.
Also applies to: 79-84, 93-98, 107-116, 125-130, 139-145, 153-158, 167-176, 185-195, 204-213, 246-251, 266-269, 284-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/hooks/usePerspective.tsx` around lines 65 - 70, The dev
branches insert raw namespace into URLs; add a single helper in this module
(e.g., normalizeNamespace or encodeNamespace) that returns
encodeURIComponent(namespace) when namespace is truthy and a safe fallback
(e.g., '' or 'all-namespaces' per app convention) when undefined, and use that
helper everywhere you build dev URLs (start with getAlertsUrl and replace all
other dev cases in this file) to avoid emitting unencoded '#' or '/ns/undefined'
fragments—update all dev-case string interpolations to use the helper instead of
directly using the namespace variable.
| import { ResourceIcon, Timestamp, useActiveNamespace } from '@openshift-console/dynamic-plugin-sdk'; | ||
| import { Bullseye, Spinner } from '@patternfly/react-core'; | ||
| import { Link } from 'react-router-dom'; | ||
| import { Link } from 'react-router-dom-v5-compat'; |
There was a problem hiding this comment.
Pass a concrete namespace into the dev rule link.
getRuleUrl's dev branch builds /dev-monitoring/ns/${namespace}/rules/${id}, but this call still omits the namespace. After removing the redirect flow, clicking an incident rule from the dev perspective will navigate to /dev-monitoring/ns/undefined/.... The unconditional setNamespace(ALL_NAMESPACES_KEY) is also at odds with namespaced dev URLs. Use alertDetails.namespace (or another guaranteed namespace) here, and only force ALL_NAMESPACES_KEY outside the dev perspective.
Suggested fix
<Link
- to={getRuleUrl(perspective, alertDetails?.rule)}
- onClick={() => setNamespace(ALL_NAMESPACES_KEY)}
+ to={getRuleUrl(perspective, alertDetails?.rule, alertDetails.namespace)}
+ onClick={() => {
+ if (perspective !== 'dev') {
+ setNamespace(ALL_NAMESPACES_KEY);
+ }
+ }}
>Also applies to: 36-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx` at line 4, The dev
rule link is missing a concrete namespace causing URLs like
/dev-monitoring/ns/undefined/...; update the link generation to pass a
guaranteed namespace to getRuleUrl (use alertDetails.namespace or another
validated namespace) instead of relying on the unconditional
setNamespace(ALL_NAMESPACES_KEY); ensure you only fallback to ALL_NAMESPACES_KEY
when not in the dev perspective and adjust any other calls around getRuleUrl and
setNamespace (also update the similar occurrences at the other noted locations)
so dev URLs receive a real namespace value.
| const { setNamespace } = useQueryNamespace(); | ||
| const { displayNamespaceSelector } = useMonitoring(); |
There was a problem hiding this comment.
Reset queries on shell-driven namespace changes.
Right now the namespace reset lives only inside NamespaceBar's onNamespaceChange. When displayNamespaceSelector is false, switching projects from the dev console can leave the previous query set intact, and the predefined dev queries still bake in the old namespace literal. Please clear the query browser off the resolved namespace as well, not only from the local selector callback.
💡 One way to preserve the old reset behavior in the dev tab
-import { useMemo, useCallback, useEffect, useState } from 'react';
+import { useMemo, useCallback, useEffect, useState, useRef } from 'react';
const MetricsPage_: FC = () => {
const { t } = useTranslation(process.env.I18N_NAMESPACE);
const [units, setUnits] = useQueryParam(QueryParams.Units, StringParam);
- const { setNamespace } = useQueryNamespace();
+ const { namespace, setNamespace } = useQueryNamespace();
const { displayNamespaceSelector } = useMonitoring();
const dispatch = useDispatch();
+ const previousNamespace = useRef(namespace);
+
+ useEffect(() => {
+ if (previousNamespace.current !== namespace) {
+ if (!displayNamespaceSelector) {
+ dispatch(queryBrowserDeleteAllQueries());
+ }
+ previousNamespace.current = namespace;
+ }
+ }, [dispatch, displayNamespaceSelector, namespace]);Also applies to: 1406-1414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/MetricsPage.tsx` around lines 1326 - 1327, The namespace
change reset is only performed in NamespaceBar.onNamespaceChange so shell-driven
changes (when displayNamespaceSelector is false) leave old queries intact;
update the logic that handles namespace resolution (the hook/useEffect that
reads the resolved namespace used by useQueryNamespace and
displayNamespaceSelector) to call setNamespace(...) and also clear/reset the
query browser state (same reset you call inside NamespaceBar.onNamespaceChange)
whenever the resolved namespace changes externally; locate the namespace
handling around useQueryNamespace/setNamespace and the NamespaceBar
onNamespaceChange code (also mirror the fix for the similar block around lines
referenced 1406-1414) and ensure both paths invoke the same query-clear routine
so predefined dev queries don’t retain the old namespace literal.
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
re-enable-dev-monitoring
branch
from
37d6d41 to
30340b6
Compare
|
@jgbernalp: This pull request references Jira Issue OCPBUGS-77113, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
There was a problem hiding this comment.
Actionable comments posted: 5
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/alerting/SilenceForm.tsx (1)
132-136:⚠️ Potential issue | 🟠 MajorDon't treat an unresolved namespace as a locked tenant.
Line 135 makes
undefinedlook like a concrete namespace becauseundefined !== ALL_NAMESPACES_KEY. That can send the form down the namespaced submit path later, including anamespace: undefinedmatcher and tenancy URL. Require a real namespace before locking, and keep submit disabled until it resolves.Minimal fix for the predicate
- const isPageNamespaceLocked = isNamespaced && namespace !== ALL_NAMESPACES_KEY; + const isPageNamespaceLocked = isNamespaced && !!namespace && namespace !== ALL_NAMESPACES_KEY;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/SilenceForm.tsx` around lines 132 - 136, The predicate treating an unresolved namespace as a locked tenant is incorrect: update the isPageNamespaceLocked logic (used where useQueryNamespace() -> namespace is read and in the isNamespaced flow) so it only returns true when a real namespace is present and not the ALL_NAMESPACES_KEY (e.g., namespace && namespace !== ALL_NAMESPACES_KEY). Also ensure the form submit remains disabled until namespace is resolved (namespace !== undefined) to avoid submitting a matcher or tenancy URL with namespace: undefined.
♻️ Duplicate comments (2)
web/src/components/MetricsPage.tsx (1)
1326-1327:⚠️ Potential issue | 🟠 MajorReset query state on shell-driven namespace changes.
When
displayNamespaceSelectorisfalse, theNamespaceBarcallback never runs, so switching projects from the dev shell leaves the existing query set intact. That is especially visible for the predefined dev queries, which hard-code the previous namespace into the PromQL text. Please watch the resolved namespace in an effect and reuse the samequeryBrowserDeleteAllQueries()path there.One way to preserve the old reset behavior
-import type { FC, Ref } from 'react'; -import { useMemo, useCallback, useEffect, useState } from 'react'; +import type { FC, Ref } from 'react'; +import { useMemo, useCallback, useEffect, useRef, useState } from 'react'; @@ - const { setNamespace } = useQueryNamespace(); + const { namespace, setNamespace } = useQueryNamespace(); const { displayNamespaceSelector } = useMonitoring(); const dispatch = useDispatch(); + const previousNamespace = useRef(namespace); @@ useEffect(() => { if (!isGraphUnit(units)) { setUnits('short'); } }, [units, setUnits]); + + useEffect(() => { + if (!displayNamespaceSelector && previousNamespace.current !== namespace) { + dispatch(queryBrowserDeleteAllQueries()); + previousNamespace.current = namespace; + } + }, [dispatch, displayNamespaceSelector, namespace]);#!/bin/bash rg -n -C3 'queryBrowserDeleteAllQueries\(' web/src/components/MetricsPage.tsx rg -n -C3 'useQueryNamespace|displayNamespaceSelector|NamespaceBar' web/src/components/MetricsPage.tsxAlso applies to: 1406-1417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/MetricsPage.tsx` around lines 1326 - 1327, When displayNamespaceSelector is false the NamespaceBar callback isn't invoked so shell-driven namespace changes don't trigger a reset; add an effect in MetricsPage that watches the resolved namespace from useQueryNamespace (or the hook that returns setNamespace) and when that resolved namespace changes call queryBrowserDeleteAllQueries() to clear queries (reuse the same deletion path the NamespaceBar uses). Ensure this runs only when displayNamespaceSelector is false (from useMonitoring) to preserve existing behavior, and reference setNamespace, displayNamespaceSelector, NamespaceBar, and queryBrowserDeleteAllQueries when locating where to add the effect.web/src/components/hooks/usePerspective.tsx (1)
65-70:⚠️ Potential issue | 🟠 MajorNormalize
namespacebefore composing dev URLs.
namespaceis optional in these helpers, but everydevbranch interpolates it raw. That can generate/ns/undefined/...or?namespace=undefinedwhile the namespace is still resolving.Also applies to: 79-84, 93-98, 107-116, 125-130, 139-145, 153-158, 167-176, 185-195, 204-213, 233-251, 260-269, 278-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/hooks/usePerspective.tsx` around lines 65 - 70, The dev-branch URL helpers are interpolating namespace directly (e.g., getAlertsUrl) which can produce "undefined" in paths; normalize the incoming namespace at the top of each helper (e.g., const ns = namespace ?? '' or const ns = namespace ? encodeURIComponent(namespace) : '') and use that normalized ns when building the `/dev-monitoring/ns/${ns}/...` URLs so you never insert the literal string "undefined" (apply this change to all helpers with a dev branch such as getAlertsUrl and the other dev-* URL builders referenced in the file).
🤖 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/alerting/AlertsPage.tsx`:
- Around line 43-56: The component-level useEffect in AlertsPage.tsx is causing
a duplicate initial poll because useAlerts() already begins polling on mount;
remove the effect that calls trigger() (the useEffect block referencing trigger)
so the initial fetch is only started by the useAlerts hook, and also drop the
related eslint-disable comment since the effect will be removed.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 47-52: The effect in SilencesPage that calls trigger() causes a
duplicate initial fetch because useAlerts() already starts polling; remove the
useEffect block that calls trigger() (the const { silences,
silenceClusterLabels, trigger } = useAlerts(); useEffect(() => { trigger(); /*
eslint-disable-next-line react-hooks/exhaustive-deps */ }, []);) so the
component relies on useAlerts()'s built-in start-up poll instead of dispatching
trigger twice.
In `@web/src/components/alerting/SilencesUtils.tsx`:
- Around line 284-293: The expireSilence handler calls getFetchSilenceUrl using
namespace from useQueryNamespace() before that hook may be resolved, producing
URLs like /ns/undefined; modify expireSilence (the function named expireSilence)
to guard: return early (or disable the action) if namespace is falsy, or
alternatively change getFetchSilenceUrl to safely handle an undefined namespace
by using a safe fallback; ensure the check uses the same namespace variable from
useQueryNamespace() so the DELETE is only issued when namespace is present and
valid.
In `@web/src/components/console/graphs/promethues-graph.tsx`:
- Line 35: The forEach callback is using an expression body that implicitly
returns params.set(...), which triggers the lint rule; change the callback for
queries.forEach to use a block body and call params.set(`query${index}`, q)
without returning anything (i.e., replace the concise arrow with a
brace-enclosed function body), so the side-effect is preserved but no value is
returned from the callback.
In `@web/src/components/hooks/usePerspective.tsx`:
- Around line 278-287: The dev-case URL in getLegacyDashboardsUrl currently
returns /dev-monitoring/... which navigates away from the new dashboards tab;
change the return for perspective === 'dev' in getLegacyDashboardsUrl to use the
dashboards route (e.g. return
`/dashboards/ns/${namespace}?dashboard=${boardName}`) so switching boards stays
on the registered "dashboards" href; update the string in the case 'dev' branch
inside the getLegacyDashboardsUrl function.
---
Outside diff comments:
In `@web/src/components/alerting/SilenceForm.tsx`:
- Around line 132-136: The predicate treating an unresolved namespace as a
locked tenant is incorrect: update the isPageNamespaceLocked logic (used where
useQueryNamespace() -> namespace is read and in the isNamespaced flow) so it
only returns true when a real namespace is present and not the
ALL_NAMESPACES_KEY (e.g., namespace && namespace !== ALL_NAMESPACES_KEY). Also
ensure the form submit remains disabled until namespace is resolved (namespace
!== undefined) to avoid submitting a matcher or tenancy URL with namespace:
undefined.
---
Duplicate comments:
In `@web/src/components/hooks/usePerspective.tsx`:
- Around line 65-70: The dev-branch URL helpers are interpolating namespace
directly (e.g., getAlertsUrl) which can produce "undefined" in paths; normalize
the incoming namespace at the top of each helper (e.g., const ns = namespace ??
'' or const ns = namespace ? encodeURIComponent(namespace) : '') and use that
normalized ns when building the `/dev-monitoring/ns/${ns}/...` URLs so you never
insert the literal string "undefined" (apply this change to all helpers with a
dev branch such as getAlertsUrl and the other dev-* URL builders referenced in
the file).
In `@web/src/components/MetricsPage.tsx`:
- Around line 1326-1327: When displayNamespaceSelector is false the NamespaceBar
callback isn't invoked so shell-driven namespace changes don't trigger a reset;
add an effect in MetricsPage that watches the resolved namespace from
useQueryNamespace (or the hook that returns setNamespace) and when that resolved
namespace changes call queryBrowserDeleteAllQueries() to clear queries (reuse
the same deletion path the NamespaceBar uses). Ensure this runs only when
displayNamespaceSelector is false (from useMonitoring) to preserve existing
behavior, and reference setNamespace, displayNamespaceSelector, NamespaceBar,
and queryBrowserDeleteAllQueries when locating where to add the effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 996af8ee-019d-4338-8425-450793c6b586
📒 Files selected for processing (28)
web/console-extensions.jsonweb/package.jsonweb/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsPage.tsxweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertDetail/SilencedByTable.tsxweb/src/components/alerting/AlertList/AggregateAlertTableRow.tsxweb/src/components/alerting/AlertList/AlertTableRow.tsxweb/src/components/alerting/AlertRulesDetailsPage.tsxweb/src/components/alerting/AlertRulesPage.tsxweb/src/components/alerting/AlertUtils.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsDetailsPage.tsxweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilenceForm.tsxweb/src/components/alerting/SilencesDetailsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/SilencesUtils.tsxweb/src/components/console/graphs/promethues-graph.tsxweb/src/components/dashboards/legacy/legacy-dashboard-page.tsxweb/src/components/dashboards/legacy/legacy-dashboard.tsxweb/src/components/dashboards/legacy/useLegacyDashboards.tsweb/src/components/hooks/usePerspective.tsxweb/src/components/hooks/useQueryNamespace.tsweb/src/components/metrics/promql-expression-input.tsxweb/src/components/redirects/dev-redirects.tsxweb/src/contexts/MonitoringContext.tsxweb/src/hooks/useMonitoring.ts
💤 Files with no reviewable changes (2)
- web/package.json
- web/src/components/redirects/dev-redirects.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- web/src/components/Incidents/IncidentsPage.tsx
- web/src/components/hooks/useQueryNamespace.ts
- web/src/hooks/useMonitoring.ts
- web/src/components/dashboards/legacy/legacy-dashboard.tsx
- web/src/components/alerting/AlertUtils.tsx
- web/src/components/Incidents/IncidentsDetailsRowTable.tsx
- web/src/components/alerting/AlertsDetailsPage.tsx
- web/src/components/alerting/AlertingPage.tsx
- web/src/components/metrics/promql-expression-input.tsx
- web/src/components/alerting/SilencesDetailsPage.tsx
- web/src/components/dashboards/legacy/useLegacyDashboards.ts
- web/src/components/alerting/AlertList/AlertTableRow.tsx
| const { | ||
| trigger, | ||
| alerts, | ||
| additionalAlertSourceLabels, | ||
| alertClusterLabels, | ||
| rulesAlertLoading, | ||
| silences, | ||
| } = useAlerts(); | ||
|
|
||
| // triggers the alert polling if this view was the first to load | ||
| useEffect(() => { | ||
| trigger(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Remove the extra initial poll.
useAlerts() already starts polling on mount in web/src/hooks/useAlerts.ts, so this effect can dispatch the same alerting fetch twice on first render.
Suggested fix
-import { useEffect, type FC } from 'react';
+import type { FC } from 'react';
…
- const {
- trigger,
- alerts,
- additionalAlertSourceLabels,
- alertClusterLabels,
- rulesAlertLoading,
- silences,
- } = useAlerts();
-
- // triggers the alert polling if this view was the first to load
- useEffect(() => {
- trigger();
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, []);
+ const {
+ alerts,
+ additionalAlertSourceLabels,
+ alertClusterLabels,
+ rulesAlertLoading,
+ silences,
+ } = useAlerts();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/alerting/AlertsPage.tsx` around lines 43 - 56, The
component-level useEffect in AlertsPage.tsx is causing a duplicate initial poll
because useAlerts() already begins polling on mount; remove the effect that
calls trigger() (the useEffect block referencing trigger) so the initial fetch
is only started by the useAlerts hook, and also drop the related eslint-disable
comment since the effect will be removed.
| const { silences, silenceClusterLabels, trigger } = useAlerts(); | ||
|
|
||
| useEffect(() => { | ||
| trigger(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Remove the extra initial poll.
useAlerts() already starts polling on mount, so this effect can dispatch the same silences/alerts fetch twice on first render.
Suggested fix
-import { useContext, useState, useMemo, useCallback, memo, useEffect } from 'react';
+import { useContext, useState, useMemo, useCallback, memo } from 'react';
…
- const { silences, silenceClusterLabels, trigger } = useAlerts();
-
- useEffect(() => {
- trigger();
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, []);
+ const { silences, silenceClusterLabels } = useAlerts();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { silences, silenceClusterLabels, trigger } = useAlerts(); | |
| useEffect(() => { | |
| trigger(); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, []); | |
| const { silences, silenceClusterLabels } = useAlerts(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/alerting/SilencesPage.tsx` around lines 47 - 52, The
effect in SilencesPage that calls trigger() causes a duplicate initial fetch
because useAlerts() already starts polling; remove the useEffect block that
calls trigger() (the const { silences, silenceClusterLabels, trigger } =
useAlerts(); useEffect(() => { trigger(); /* eslint-disable-next-line
react-hooks/exhaustive-deps */ }, []);) so the component relies on useAlerts()'s
built-in start-up poll instead of dispatching trigger twice.
| const { namespace } = useQueryNamespace(); | ||
|
|
||
| const [isInProgress, , setInProgress, setNotInProgress] = useBoolean(false); | ||
| const [success, , setSuccess] = useBoolean(false); | ||
| const [errorMessage, setErrorMessage] = useState(); | ||
|
|
||
| const expireSilence = () => { | ||
| setInProgress(); | ||
| const url = getFetchSilenceUrl(perspective, silenceID); | ||
| const url = getFetchSilenceUrl(perspective, silenceID, namespace); | ||
| consoleFetchJSON |
There was a problem hiding this comment.
Guard the expire request until namespace is resolved.
This modal now builds the fetch URL from useQueryNamespace(), but that hook can still be unresolved on the first render. In dev perspective that produces a /ns/undefined/... DELETE target, so expiring a silence can fail for the wrong reason. Please block the action until namespace is present, or make the URL helper fall back safely.
One straightforward guard
const expireSilence = () => {
+ if (perspective === 'dev' && !namespace) {
+ setErrorMessage(t('Namespace is still loading.'));
+ return;
+ }
setInProgress();
const url = getFetchSilenceUrl(perspective, silenceID, namespace);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { namespace } = useQueryNamespace(); | |
| const [isInProgress, , setInProgress, setNotInProgress] = useBoolean(false); | |
| const [success, , setSuccess] = useBoolean(false); | |
| const [errorMessage, setErrorMessage] = useState(); | |
| const expireSilence = () => { | |
| setInProgress(); | |
| const url = getFetchSilenceUrl(perspective, silenceID); | |
| const url = getFetchSilenceUrl(perspective, silenceID, namespace); | |
| consoleFetchJSON | |
| const { namespace } = useQueryNamespace(); | |
| const [isInProgress, , setInProgress, setNotInProgress] = useBoolean(false); | |
| const [success, , setSuccess] = useBoolean(false); | |
| const [errorMessage, setErrorMessage] = useState(); | |
| const expireSilence = () => { | |
| if (perspective === 'dev' && !namespace) { | |
| setErrorMessage(t('Namespace is still loading.')); | |
| return; | |
| } | |
| setInProgress(); | |
| const url = getFetchSilenceUrl(perspective, silenceID, namespace); | |
| consoleFetchJSON |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/alerting/SilencesUtils.tsx` around lines 284 - 293, The
expireSilence handler calls getFetchSilenceUrl using namespace from
useQueryNamespace() before that hook may be resolved, producing URLs like
/ns/undefined; modify expireSilence (the function named expireSilence) to guard:
return early (or disable the action) if namespace is falsy, or alternatively
change getFetchSilenceUrl to safely handle an undefined namespace by using a
safe fallback; ensure the check uses the same namespace variable from
useQueryNamespace() so the DELETE is only issued when namespace is present and
valid.
| } | ||
|
|
||
| const params = new URLSearchParams(); | ||
| queries.forEach((q, index) => params.set(`query${index}`, q)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
find . -type f -name "promethues-graph.tsx" 2>/dev/null | head -5Repository: openshift/monitoring-plugin
Length of output: 127
🏁 Script executed:
# Also check for variations in filename (prometheus vs promethues)
find . -type f -name "*prometheus*.tsx" 2>/dev/null | head -5
find . -type f -name "*graph*.tsx" 2>/dev/null | grep -i console | head -10Repository: openshift/monitoring-plugin
Length of output: 239
🏁 Script executed:
# Read the file to see line 35 in context
wc -l ./web/src/components/console/graphs/promethues-graph.tsxRepository: openshift/monitoring-plugin
Length of output: 130
🏁 Script executed:
# Read the file, showing line numbers around line 35
sed -n '1,50p' ./web/src/components/console/graphs/promethues-graph.tsx | cat -nRepository: openshift/monitoring-plugin
Length of output: 2055
🏁 Script executed:
# Check for Biome configuration files
find . -maxdepth 2 -type f \( -name "biome.json" -o -name ".biome.json" -o -name "biome.jsonc" \) 2>/dev/nullRepository: openshift/monitoring-plugin
Length of output: 83
🏁 Script executed:
# Check for any linting config files and package.json to understand the project setup
ls -la . | grep -E "biome|eslint|prettier|package"
head -30 ./package.json 2>/dev/null | grep -E "biome|lint"Repository: openshift/monitoring-plugin
Length of output: 132
🏁 Script executed:
# Read the Biome configuration
cat ./biome.jsonRepository: openshift/monitoring-plugin
Length of output: 1845
🌐 Web query:
Biome useIterableCallbackReturn rule suspicious
💡 Result:
Biome’s lint/suspicious/useIterableCallbackReturn rule flags inconsistent or incorrect return usage inside callbacks passed to common iterable/array methods—because it often indicates a logic bug (e.g., a map/filter callback that sometimes returns nothing, producing undefined elements or incorrect filtering). It’s enabled by default (“recommended”) and is equivalent to ESLint’s array-callback-return. [1]
What it checks: [1]
- Must return a value in callbacks for methods like
map,filter,find,every,some,reduce,sort, etc. - Must NOT return a value for
forEachcallbacks (return values are meaningless there), though you can allow them.
Config option:
checkForEach(since Biome v2.4.0) defaults totrue; set it tofalseto stop reportingreturn <value>insideforEachcallbacks. [1]
Sources: [1] [2]
References
- [1] Biome rule docs:
useIterableCallbackReturn - [2] Biome “rules sources” mapping (ESLint
array-callback-return→ BiomeuseIterableCallbackReturn)
Avoid returning from the forEach callback.
Line 35 currently returns params.set(...) from a forEach callback, which violates the lint/suspicious/useIterableCallbackReturn rule (enabled by default in Biome). While forEach ignores return values, this pattern is flagged as it may indicate unintended logic. Convert to a block body to fix.
Suggested fix
- queries.forEach((q, index) => params.set(`query${index}`, q));
+ queries.forEach((q, index) => {
+ params.set(`query${index}`, q);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queries.forEach((q, index) => params.set(`query${index}`, q)); | |
| queries.forEach((q, index) => { | |
| params.set(`query${index}`, q); | |
| }); |
🧰 Tools
🪛 Biome (2.4.6)
[error] 35-35: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/console/graphs/promethues-graph.tsx` at line 35, The
forEach callback is using an expression body that implicitly returns
params.set(...), which triggers the lint rule; change the callback for
queries.forEach to use a block body and call params.set(`query${index}`, q)
without returning anything (i.e., replace the concise arrow with a
brace-enclosed function body), so the side-effect is preserved but no value is
returned from the callback.
| export const getLegacyDashboardsUrl = ( | ||
| perspective: Perspective, | ||
| boardName: string, | ||
| namespace?: string, | ||
| ) => { | ||
| switch (perspective) { | ||
| case 'virtualization-perspective': | ||
| return `/virt-monitoring/dashboards/${boardName}`; | ||
| case 'acm': | ||
| return ''; | ||
| case 'dev': | ||
| return `/dev-monitoring/ns/${namespace}?dashboard=${boardName}`; |
There was a problem hiding this comment.
Keep dev dashboard links on the dashboards route.
The new dev tab is registered with href: "dashboards" in web/console-extensions.json, but this helper returns /dev-monitoring/ns/${namespace}?dashboard=.... Switching boards will navigate off the dashboards tab instead of updating it.
Suggested fix
case 'dev':
- return `/dev-monitoring/ns/${namespace}?dashboard=${boardName}`;
+ return `/dev-monitoring/ns/${namespace}/dashboards?dashboard=${boardName}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getLegacyDashboardsUrl = ( | |
| perspective: Perspective, | |
| boardName: string, | |
| namespace?: string, | |
| ) => { | |
| switch (perspective) { | |
| case 'virtualization-perspective': | |
| return `/virt-monitoring/dashboards/${boardName}`; | |
| case 'acm': | |
| return ''; | |
| case 'dev': | |
| return `/dev-monitoring/ns/${namespace}?dashboard=${boardName}`; | |
| export const getLegacyDashboardsUrl = ( | |
| perspective: Perspective, | |
| boardName: string, | |
| namespace?: string, | |
| ) => { | |
| switch (perspective) { | |
| case 'acm': | |
| return ''; | |
| case 'dev': | |
| return `/dev-monitoring/ns/${namespace}/dashboards?dashboard=${boardName}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/hooks/usePerspective.tsx` around lines 278 - 287, The
dev-case URL in getLegacyDashboardsUrl currently returns /dev-monitoring/...
which navigates away from the new dashboards tab; change the return for
perspective === 'dev' in getLegacyDashboardsUrl to use the dashboards route
(e.g. return `/dashboards/ns/${namespace}?dashboard=${boardName}`) so switching
boards stays on the registered "dashboards" href; update the string in the case
'dev' branch inside the getLegacyDashboardsUrl function.
|
@jgbernalp: This pull request references Jira Issue OCPBUGS-77113, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
tested with PR |
openshift-ci
bot
added
the
qe-approved
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/test e2e-monitoring |
|
@jgbernalp: 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. |
5 participants
This PR
displayNamespaceSelectorinto the monitoring context. This is used in the metrics view to hide the namespace selector that is already present in the outer component scope provided by the dev console. Adds a newMpCmoDevMetricsPagecomponent for the console extensions to consume with this new configurationFixes:
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements