OCPBUGS-77113: feat: re enable dev console views#831
OCPBUGS-77113: feat: re enable dev console views#831jgbernalp wants to merge 2 commits 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 use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![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
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
|
@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
force-pushed
the
re-enable-dev-monitoring
branch
from
30340b6 to
dd75f96
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. |
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
re-enable-dev-monitoring
branch
from
dd75f96 to
0649bd3
Compare
|
/test e2e-monitoring |
|
/test ? |
|
/test e2e-monitoring-dev |
|
@jgbernalp: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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