OU-1091: swap monitoring-plugin to use DataView#911
Merged
openshift-merge-bot[bot] merged 10 commits intoopenshift:mainfrom Apr 30, 2026
Merged
OU-1091: swap monitoring-plugin to use DataView#911openshift-merge-bot[bot] merged 10 commits intoopenshift:mainfrom
openshift-merge-bot[bot] merged 10 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@PeterYurkovich: This pull request references OU-1091 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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
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:
WalkthroughReplaces legacy VirtualizedTable/ListPageFilter with PatternFly DataView tables; adds URL-synced filter, pagination, and sort hooks; introduces reusable toolbar/filter/table primitives and new filter/sort utilities; migrates alerts/ silences/targets/pages to DataView; updates Cypress tests and adds Jest suites and supporting console UX components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "TableToolbar / TableFilters"
participant Hook as "useTableFilters / useTablePagination"
participant Data as "DataViewTable"
participant URL as "Browser URL"
User->>UI: change filter / sort / page
UI->>Hook: onSetFilters / onPerPageSelect / onSetPage / onSort
Hook->>URL: update query params
Hook->>Data: provide filtered, sorted, paginated rows
Data->>UI: render rows and update controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/package.json (1)
104-112:⚠️ Potential issue | 🟠 MajorMove
@patternfly/react-component-groupstodependencies(currently indevDependencies).This package is imported by runtime UI code (e.g.,
web/src/components/alerting/SilencesPage.tsxusesBulkSelectandBulkSelectValue). Runtime dependencies should be listed independenciesto follow npm conventions and ensure the build is maintainable if the installation process changes (e.g., ifnpm ci --productionis used in the future).Suggested change
{ "dependencies": { + "@patternfly/react-component-groups": "^6.4.0", "@codemirror/autocomplete": "^6.0.4", "@codemirror/commands": "^6.0.1", "@codemirror/language": "^6.2.0", ... "@patternfly/react-data-view": "^6.4.0", ... }, "devDependencies": { "@cypress/grep": "^4.1.0", ... - "@patternfly/react-component-groups": "^6.4.0", "@swc/core": "^1.15.3", ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/package.json` around lines 104 - 112, The package "@patternfly/react-component-groups" is used at runtime (e.g., imported by web/src/components/alerting/SilencesPage.tsx for BulkSelect and BulkSelectValue) but is currently listed under devDependencies; move it to dependencies in web/package.json by removing the entry from "devDependencies" and adding the same version string under "dependencies", then update the lockfile (run npm/yarn install) so the runtime dependency is recorded correctly.web/src/components/alerting/SilencesPage.tsx (1)
459-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn real comparators from
sortSilences.These branches never return
0for ties, and the tuple-based branches compare arrays with>, which coerces them to strings.silenceStateOrderalso includes thesilenceStateOrderfunction itself in the tuple, so state ordering can degrade to comparing function source text. The result is unstable and can sort the state/firing-alert/cluster columns incorrectly.🤖 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 459 - 505, The sorting helpers return invalid comparators (arrays coerced to strings, omitted tie=0, and silenceStateOrder mistakenly includes the function itself), causing unstable sorts; fix sortSilences and its helpers so each branch returns a proper numeric comparator: change silenceStateOrder to return a tuple of numbers (stateIndex and timestamp as milliseconds) or expose a helper that returns those numbers and compare them lexicographically to produce -1/0/1, change silenceClusterOrder/clusterSort to return a numeric index (not an array) and compare indices directly, use localeCompare for name/createdBy (which returns -1/0/1), use numeric comparison for silenceFiringAlertsOrder, and apply direction by multiplying the comparison result by 1 or -1 so ties return 0 and comparisons are stable; reference functions: sortSilences, silenceStateOrder, silenceClusterOrder (clusterSort), silenceFiringAlertsOrder, and rowFilter to locate and update the branches.
🟡 Minor comments (5)
web/src/components/targets-page.tsx-555-562 (1)
555-562:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPass the current label filter value into the LABEL filter item.
TableLabelFilterbuilds its toolbar chips fromvalue, but this config never suppliesfilters[TargetsFilterOptions.LABEL]. The label filter still applies, but the active chip state is invisible and can't be deleted individually from the toolbar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/targets-page.tsx` around lines 555 - 562, The LABEL filter config for TableLabelFilter does not pass the current filter value, so the toolbar chips (built from value) don't show active label chips; update the filter item where filterId: TargetsFilterOptions.LABEL (the TableFilterOption.LABEL entry) to include value: filters[TargetsFilterOptions.LABEL] (or the appropriate filters state variable) so TableLabelFilter receives its current value; keep existing props like onChange: onFiltersChange(TargetsFilterOptions.LABEL) and labelPath: 'labels' unchanged.web/src/components/alerting/AlertRulesPage.tsx-106-112 (1)
106-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe source column header is mislabeled.
This column renders the rule source (
User/Platform), but the header isTotal. That makes the new table misleading.🤖 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 106 - 112, The columnKeys array builds table headers but the entry for AlertRulesFilterOptions.SOURCE is incorrectly labeled t('Total'); update the label for the { label: t('Total'), key: rowFilter(AlertRulesFilterOptions.SOURCE) } entry to a correct translation like t('Source') (or t('Rule source') if preferred) so the column that shows rule source (User/Platform) is labeled appropriately.web/src/components/alerting/SilencesPage.tsx-400-404 (1)
400-404:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a silence-specific table label.
aria-label="Repositories table"is a copy/paste label, so assistive tech announces the wrong content on this page.Suggested fix
- <DataViewTable - aria-label="Repositories table" + <DataViewTable + aria-label="Silences table" columns={columns} rows={selectedPageOfSilences} />🤖 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 400 - 404, The DataViewTable in SilencesPage is using a copy/paste aria-label "Repositories table" which mislabels the content for assistive tech; update the DataViewTable component (in SilencesPage) to use a silence-specific aria-label such as "Silences table" or "Alert silences table" so the rows/selectedPageOfSilences and columns relate to silences are correctly announced by screen readers; locate the DataViewTable invocation in SilencesPage.tsx and replace the aria-label value accordingly.web/cypress/views/list-page.ts-286-289 (1)
286-289:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert that "Clear filters" is absent here, not merely hidden.
DataViewToolbar-clear-all-filtersis conditionally rendered. In the empty state it is typically not mounted, sonot.be.visiblewill fail before the assertion can run.Suggested fix
- cy.byOUIAID('DataViewToolbar-clear-all-filters').should('not.be.visible'); + cy.byOUIAID('DataViewToolbar-clear-all-filters').should('not.exist');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/list-page.ts` around lines 286 - 289, The test currently checks cy.byOUIAID('DataViewToolbar-clear-all-filters').should('not.be.visible') which fails when the element is not mounted; change the assertion to verify the element is not present instead (use .should('not.exist') on the same cy.byOUIAID('DataViewToolbar-clear-all-filters') call) so the test correctly asserts the "Clear filters" control is absent in the empty state.web/cypress/views/list-page.ts-152-157 (1)
152-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape tag text before building the exact-match regex.
This helper is matching literal chip text, but
new RegExp(\^${tagName}$`)treats characters like.,+,(, and)` as regex syntax. Tags containing metacharacters will remove the wrong chip or fail to match at all.Suggested fix
cy.get(Classes.IndividualTag) - .contains(new RegExp(`^${tagName}$`)) + .contains(new RegExp(`^${Cypress._.escapeRegExp(tagName)}$`)) .parent() .next('span') .children('button') .click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/list-page.ts` around lines 152 - 157, The removeIndividualTag helper builds an unescaped regex with new RegExp(`^${tagName}$`) which treats characters like .+()[] as regex metacharacters and breaks matching; update removeIndividualTag to escape tagName before constructing the regex (or avoid RegExp entirely by using a literal text match such as contains(tagName, { matchCase: true }) if supported) so Classes.IndividualTag.contains(...) matches the exact chip text; ensure you implement or import a safe escape function (e.g., escapeRegExp) and use that escaped string when calling new RegExp or switch to a direct text-contains API to perform an exact-match.
🤖 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/AlertRulesPage.tsx`:
- Around line 358-385: The comparator in sortRules is invalid because it returns
0/1 (or 1/0) instead of the required -1/0/1, causing unstable/incorrect sorts;
update each branch in sortRules (the NAME, SEVERITY, STATE, SOURCE cases
identified by rowFilter(AlertRulesFilterOptions.*)) to use proper comparators:
use string.localeCompare for name/source (handling undefined with fallback to
empty string), numeric or explicit -1/0/1 comparisons for severity and for
values returned by alertingRuleStateOrder/alertingRuleSource (or subtract for
numeric results), and ensure ties return 0 so the sort is stable and correct.
- Around line 145-148: The effect currently resets pagination on mount because
it always calls pagination.onSetPage when filters changes; change it to skip the
initial mount by adding an isInitialMount ref (e.g., useRef(true)) and only call
pagination.onSetPage(undefined, 1) when isInitialMount.current is false, then
set isInitialMount.current = false after the first render; additionally guard by
checking the current pagination.page and only call onSetPage if it's not already
1 to avoid unnecessary URL rewrites. Target the useEffect, the filters
dependency, and the pagination.onSetPage call to implement this.
In `@web/src/components/alerting/AlertsPage.tsx`:
- Around line 331-336: The map key uses only aggregatedAlert.name which is not
unique; update the rendering of AggregateAlertTableRow in
selectedPageOfAggregatedAlerts.map to use a deterministic composite key that
matches the full aggregation identity (the same fields used to build the
aggregate) instead of just aggregatedAlert.name — e.g., combine
aggregatedAlert.name with the aggregate grouping fields (such as
aggregatedAlert.groupingKeys, aggregatedAlert.id, or the aggregationKey used
upstream) into a single string (e.g. by joining or JSON.stringify) and pass that
as the key so React can reliably distinguish expandable rows.
- Around line 162-165: The effect that resets pagination runs on mount and thus
clears deep-linked pages; update the useEffect in AlertsPage that currently
calls pagination.onSetPage(undefined, 1) to skip its first run: add a ref like
isFirstRender (useRef(true)) and inside the effect if (isFirstRender.current) {
isFirstRender.current = false; return; } so the reset only runs on subsequent
filter changes (using filters as the dependency), ensuring deep-linked pages are
preserved on initial mount.
- Around line 364-400: The comparator in sortAggregatedAlerts is returning 0/1
instead of -1/0/1; update each sort branch (name, severity, alert-total, state)
to return -1 when a < b, 1 when a > b, and 0 when equal, respecting direction
('asc' vs 'desc'). For name use localeCompare on toLocaleLowerCase(); for
severity compute numeric order via the AlertSeverity map and subtract bOrder -
aOrder (or invert for desc); for alert-total compare a.alerts.length and
b.alerts.length to return -1/0/1; for state use string comparison
(localeCompare) with direction applied. Ensure you reference the existing
symbols sortAggregatedAlerts, rowFilter, AlertFilterOptions, and AlertSeverity
when making changes.
In `@web/src/components/alerting/filter-rules.ts`:
- Around line 53-54: The function ruleHasAlertState uses _.isEmpty and _.some
without importing lodash; replace the implicit lodash usage with native checks:
for the NotFiring branch use a null/empty check like (rule.alerts == null ||
rule.alerts.length === 0) and for the other branch use the array some method
(rule.alerts?.some(a => a.state === state)); update ruleHasAlertState signature
accordingly and remove any reference to _ so no lodash import is required.
In `@web/src/components/alerting/filter-silences.ts`:
- Around line 24-29: The branch is incorrectly negating silences.filter(...) (an
array) and checking the whole collection instead of the current silence; replace
that check with a boolean test against the current silence's matchers, e.g. use
!silence?.matchers?.some(m => m.name === 'namespace' && m.value === namespace)
(or silence?.matchers?.some(...) without !) depending on surrounding logic, so
shouldFilterNamespace correctly excludes the row when the current silence
contains a matching namespace matcher.
- Around line 35-37: The name filter is comparing against silence?.name which
doesn't match the actual silence display name; in the filtering logic around
selectedFilters[SilenceFilterOptions.NAME] replace the target used in
fuzzyCaseInsensitive with a display name derived from the silence.matchers
(e.g., join or format matcher key/value pairs into the same string used
elsewhere for display) and then call
fuzzyCaseInsensitive(selectedFilters[SilenceFilterOptions.NAME],
displayNameFromMatchers) instead of using silence?.name; update any
helper/variable names near fuzzyCaseInsensitive and matchers to make intent
clear.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 147-149: The selection currently matches rows by recomputed
`index`, causing selections to move after `namespacedSilences` is rebuilt;
change the `useDataViewSelection` `matchOption` to compare a stable silence
identity field (e.g., `id`, `silenceID`, or `uid`) instead of `index` so
selections track the same silence across sorts/filters, e.g., update the
`matchOption` used in `useDataViewSelection` to return a?.<stableId> ===
b?.<stableId> and ensure the selection payloads (the items passed into the hook
and the bulk-expire action consumer) include that same stable id so the
bulk-expire operation targets the intended silences.
In `@web/src/components/console/public/components/autocomplete.tsx`:
- Around line 107-117: The effect in the autocomplete hook calls
labelParser(data, labelPath) unconditionally which can throw when data is
undefined or when some resources lack the target label path; guard the call and
downstream filtering by verifying data is present and labelParser returns an
array (e.g., default to [] when data is falsy), and ensure items passed to
fuzzyCaseInsensitive are strings (filter out null/undefined non-string items)
before slicing and calling setSuggestions in the useEffect that references
visible, textValue, showSuggestions, data, and labelPath.
In `@web/src/components/filter-targets.ts`:
- Around line 62-63: The function ruleHasAlertState uses lodash methods
(_.isEmpty and _.some) but lodash is not imported; add an import for lodash
(commonly: import _ from 'lodash') at the top of the file so ruleHasAlertState
can call _.isEmpty and _.some without type/lint/build errors, or alternatively
replace the lodash calls with native equivalents (e.g., rule.alerts?.length ===
0 and Array.prototype.some) if you prefer to avoid adding a dependency; update
any types/imports accordingly to satisfy the TypeScript compiler.
In `@web/src/components/table-pagination.tsx`:
- Around line 52-63: The fallback handlers for onPerPageSelect and onSetPage
call optional setters unguarded (setPage, setPerPage), which causes runtime
errors when those setters are undefined; update the handlers so they check for
existence of the setters before calling them (e.g., if (setPerPage) {
setPerPage(v); } and if (setPage) { setPage(Math.floor(firstRow / v) + 1); } in
the onPerPageSelect fallback, and if (setPage) setPage(v) in the onSetPage
fallback), or alternatively make the props required when the corresponding
callback is not provided; locate the logic around onPerPageSelect, onSetPage,
setPage, setPerPage, page and perPage and add the guards accordingly.
In `@web/src/components/table/TableTextFilter.tsx`:
- Around line 16-31: The handler in useEffect (handleKeyDown) must bail out when
a modal/dialog is open so pressing "/" doesn't focus the background filter; add
a guard at the top of handleKeyDown that checks for an open modal/dialog (for
example: document.querySelector('[role="dialog"]:not([aria-hidden="true"])') or
another app-wide modal indicator) and return early if found, keeping the rest of
the logic (showToolbarItem, filterId lookup and focusing the input) unchanged;
reference the same pattern used in useDocumentListener to ensure consistent
behavior.
In `@web/src/components/table/TableToolbar.tsx`:
- Around line 35-49: The toolbar is freezing the Clear filters button by storing
JSX in defaultClearFilters ref which captures a stale clearAllFilters; remove
the useRef/defaultClearFilters pattern in TableToolbar and render the
<ToolbarItem> with the <Button> inline (pass it directly as
customLabelGroupContent on each render) so the button always uses the current
clearAllFilters callback, and also add clearAllFilters to the
DataViewToolbarProps type (and any places that destructure props) so the prop is
declared and typed.
In `@web/src/components/table/useTableFilters.ts`:
- Around line 60-63: The effect that initializes filters should resync when
URL/search params or provided initialFilters change instead of running only on
mount; update the useEffect that calls onSetFilters(getInitialFilters()) to
include the relevant dependencies (e.g., getInitialFilters or the router
search/location value and/or initialFilters) in its dependency array so it
re-runs on navigation or prop updates, remove the eslint-disable comment, and
ensure you still call onSetFilters(getInitialFilters()) inside that effect
(optionally guard to avoid resetting when the computed filters equal current
filters).
In `@web/src/components/table/useTablePagination.ts`:
- Around line 16-19: The initial state in useTablePagination currently does
parseInt directly from searchParams for page and perPage which can produce NaN;
sanitize these values before storing by validating parseInt results and falling
back to the provided defaults (page and perPage) when NaN or out-of-range;
ensure page is coerced to at least 1 and perPage to a sensible positive integer
(or the default) before setState and before any effect writes them back to the
URL so you never write "NaN" for page/perPage.
- Around line 21-29: The callback updateSearchParams captures a stale
searchParams snapshot and may overwrite other hooks' params on mount; change it
to use the functional updater form of setSearchParams so you read the latest
params when updating: inside updateSearchParams call setSearchParams(prev => {
const params = new URLSearchParams(prev); params.set(pageParam, `${page}`);
params.set(perPageParam, `${perPage}`); return params; }); and apply the same
pattern to the other pagination-related callbacks referenced (the similar
updater at lines 32-36) so they all build on the current search params instead
of a captured searchParams value.
In `@web/src/components/targets-page.tsx`:
- Around line 466-469: The current useEffect with
pagination.onSetPage(undefined, 1) runs on mount and resets deep-linked ?page=N;
change it to only reset when filters actually change after the first render by
skipping the initial run—e.g., add a mounted/ref flag or store previousFilters
in a useRef and in the effect only call pagination.onSetPage(undefined, 1) when
mountedRef.current is true and filters differ (or set mountedRef.current = true
after first render); target the useEffect and pagination.onSetPage usage and
ensure the dependency remains [filters] while preventing the first-mount
invocation.
- Around line 676-703: The sortTargets comparator currently returns 0/1 rather
than negative/zero/positive; update each branch in sortTargets to produce proper
three-way comparisons: compute a multiplier (const dir = direction === 'asc' ? 1
: -1) and return dir * comparisonResult, using localeCompare for string fields
(e.g. (a.scrapeUrl ?? '').toLocaleLowerCase().localeCompare((b.scrapeUrl ??
'').toLocaleLowerCase())), numeric subtraction for numeric fields (e.g.
(a.lastScrapeDuration ?? 0) - (b.lastScrapeDuration ?? 0)), and safe comparisons
for optional nested fields like labels.namespace and lastScrape; also update the
health/status branches to return dir * (a.health - b.health) or appropriate
mapping so sorting is stable and correct when using rowFilter('scrapeUrl'),
rowFilter('health'), rowFilter('namespace'), rowFilter('lastScrape'),
rowFilter('lastScrapeDuration') and rowFilter(TargetsFilterOptions.STATUS').
---
Outside diff comments:
In `@web/package.json`:
- Around line 104-112: The package "@patternfly/react-component-groups" is used
at runtime (e.g., imported by web/src/components/alerting/SilencesPage.tsx for
BulkSelect and BulkSelectValue) but is currently listed under devDependencies;
move it to dependencies in web/package.json by removing the entry from
"devDependencies" and adding the same version string under "dependencies", then
update the lockfile (run npm/yarn install) so the runtime dependency is recorded
correctly.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 459-505: The sorting helpers return invalid comparators (arrays
coerced to strings, omitted tie=0, and silenceStateOrder mistakenly includes the
function itself), causing unstable sorts; fix sortSilences and its helpers so
each branch returns a proper numeric comparator: change silenceStateOrder to
return a tuple of numbers (stateIndex and timestamp as milliseconds) or expose a
helper that returns those numbers and compare them lexicographically to produce
-1/0/1, change silenceClusterOrder/clusterSort to return a numeric index (not an
array) and compare indices directly, use localeCompare for name/createdBy (which
returns -1/0/1), use numeric comparison for silenceFiringAlertsOrder, and apply
direction by multiplying the comparison result by 1 or -1 so ties return 0 and
comparisons are stable; reference functions: sortSilences, silenceStateOrder,
silenceClusterOrder (clusterSort), silenceFiringAlertsOrder, and rowFilter to
locate and update the branches.
---
Minor comments:
In `@web/cypress/views/list-page.ts`:
- Around line 286-289: The test currently checks
cy.byOUIAID('DataViewToolbar-clear-all-filters').should('not.be.visible') which
fails when the element is not mounted; change the assertion to verify the
element is not present instead (use .should('not.exist') on the same
cy.byOUIAID('DataViewToolbar-clear-all-filters') call) so the test correctly
asserts the "Clear filters" control is absent in the empty state.
- Around line 152-157: The removeIndividualTag helper builds an unescaped regex
with new RegExp(`^${tagName}$`) which treats characters like .+()[] as regex
metacharacters and breaks matching; update removeIndividualTag to escape tagName
before constructing the regex (or avoid RegExp entirely by using a literal text
match such as contains(tagName, { matchCase: true }) if supported) so
Classes.IndividualTag.contains(...) matches the exact chip text; ensure you
implement or import a safe escape function (e.g., escapeRegExp) and use that
escaped string when calling new RegExp or switch to a direct text-contains API
to perform an exact-match.
In `@web/src/components/alerting/AlertRulesPage.tsx`:
- Around line 106-112: The columnKeys array builds table headers but the entry
for AlertRulesFilterOptions.SOURCE is incorrectly labeled t('Total'); update the
label for the { label: t('Total'), key:
rowFilter(AlertRulesFilterOptions.SOURCE) } entry to a correct translation like
t('Source') (or t('Rule source') if preferred) so the column that shows rule
source (User/Platform) is labeled appropriately.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 400-404: The DataViewTable in SilencesPage is using a copy/paste
aria-label "Repositories table" which mislabels the content for assistive tech;
update the DataViewTable component (in SilencesPage) to use a silence-specific
aria-label such as "Silences table" or "Alert silences table" so the
rows/selectedPageOfSilences and columns relate to silences are correctly
announced by screen readers; locate the DataViewTable invocation in
SilencesPage.tsx and replace the aria-label value accordingly.
In `@web/src/components/targets-page.tsx`:
- Around line 555-562: The LABEL filter config for TableLabelFilter does not
pass the current filter value, so the toolbar chips (built from value) don't
show active label chips; update the filter item where filterId:
TargetsFilterOptions.LABEL (the TableFilterOption.LABEL entry) to include value:
filters[TargetsFilterOptions.LABEL] (or the appropriate filters state variable)
so TableLabelFilter receives its current value; keep existing props like
onChange: onFiltersChange(TargetsFilterOptions.LABEL) and labelPath: 'labels'
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b3afb611-620a-43db-ae36-548418515fdd
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.gitignoreweb/cypress/README.mdweb/cypress/support/commands/traces-logging-commands.tsweb/cypress/support/monitoring/00.bvt_monitoring.cy.tsweb/cypress/support/monitoring/00.bvt_monitoring_namespace.cy.tsweb/cypress/support/monitoring/01.reg_alerts.cy.tsweb/cypress/support/monitoring/04.reg_alerts_namespace.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/list-page.tsweb/cypress/views/silences-list-page.tsweb/eslint.config.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertList/AggregateAlertTableRow.tsxweb/src/components/alerting/AlertList/DownloadCSVButton.tsxweb/src/components/alerting/AlertList/LabelFilter.tsxweb/src/components/alerting/AlertList/filter-alerts.tsweb/src/components/alerting/AlertList/hooks/useAggregateAlertColumns.tsweb/src/components/alerting/AlertList/hooks/utils.tsweb/src/components/alerting/AlertRulesPage.tsxweb/src/components/alerting/AlertUtils.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsAggregates.tsweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/SilencesUtils.tsxweb/src/components/alerting/filter-rules.tsweb/src/components/alerting/filter-silences.tsweb/src/components/alerting/useSelectedFilters.tsweb/src/components/alerting/useSortAlerts.tsweb/src/components/console/console-shared/constants/common.tsweb/src/components/console/console-shared/hooks/useDocumentListener.tsweb/src/components/console/models/index.tsweb/src/components/console/public/components/autocomplete.tsxweb/src/components/console/public/components/factory/text-filter.tsxweb/src/components/dashboards/legacy/table.tsxweb/src/components/dashboards/perses/dashboard-list.tsxweb/src/components/dashboards/perses/hooks/useDashboardsData.tsweb/src/components/data-test.tsweb/src/components/filter-targets.tsweb/src/components/query-browser.tsxweb/src/components/table-pagination.tsxweb/src/components/table/TableCheckboxFilter.tsxweb/src/components/table/TableFilters.tsxweb/src/components/table/TableLabelFilter.tsxweb/src/components/table/TableTextFilter.tsxweb/src/components/table/TableToolbar.tsxweb/src/components/table/useTableFilters.tsweb/src/components/table/useTablePagination.tsweb/src/components/targets-page.tsx
💤 Files with no reviewable changes (7)
- web/cypress/support/commands/traces-logging-commands.ts
- web/src/components/dashboards/perses/hooks/useDashboardsData.ts
- web/src/components/alerting/AlertList/hooks/utils.ts
- web/src/components/alerting/useSortAlerts.ts
- web/src/components/alerting/AlertList/hooks/useAggregateAlertColumns.ts
- web/src/components/query-browser.tsx
- web/src/components/alerting/useSelectedFilters.ts
PeterYurkovich
force-pushed
the
ou-1091-aggregated-alerts
branch
from
94b0729 to
713ab3c
Compare
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/SilencesPage.tsx (1)
462-468:⚠️ Potential issue | 🟠 Major
sortSilencesuses invalid comparator logic and includes a self-reference insilenceStateOrder.Line 467 includes a self-reference to the
silenceStateOrderfunction itself within the tuple, polluting array comparisons. Additionally, comparing arrays with the>operator (lines 499, 505) relies on string conversion rather than element-wise comparison, producing unstable sorting results. Arrays should be compared element-by-element using a proper comparator function that returns -1, 0, or 1, not by stringification.Suggested fix
Remove the self-reference at line 467 and implement proper array comparison:
const silenceStateOrder = (silence: Silence) => [ [SilenceStates.Active, SilenceStates.Pending, SilenceStates.Expired].indexOf( silenceState(silence), ), _.get(silence, silenceState(silence) === SilenceStates.Pending ? 'startsAt' : 'endsAt'), - silenceStateOrder, ];Then replace the array comparisons at lines 499 and 505 with a proper comparator function.
🤖 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 462 - 468, silenceStateOrder currently builds a tuple that mistakenly includes itself and the sorting uses array-to-string comparisons; remove the self-reference from silenceStateOrder so it returns [stateIndex, timestamp] (use silenceState(silence) and SilenceStates to compute stateIndex and pick startsAt vs endsAt), then implement a dedicated comparator function (used by sortSilences) that compares those two tuples element-by-element and returns -1/0/1 (compare stateIndex numerically, then timestamps with Date or string compare, then a stable tiebreaker like silence.id); replace any usages that do array1 > array2 or array comparisons with this comparator so sorting is stable and correct.
♻️ Duplicate comments (11)
web/src/components/alerting/AlertRulesPage.tsx (2)
145-148:⚠️ Potential issue | 🟠 MajorPage reset effect still runs on initial mount.
This rewrites deep-linked pagination to page 1 on first load.
Suggested fix
+const isInitialMount = useRef(true); useEffect(() => { - pagination.onSetPage(undefined, 1); + if (isInitialMount.current) { + isInitialMount.current = false; + return; + } + if (pagination.page !== 1) { + pagination.onSetPage(undefined, 1); + } }, [filters]); // eslint-disable-line react-hooks/exhaustive-deps🤖 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 145 - 148, The useEffect that resets pagination runs on initial mount and overwrites deep-linked page state; modify the effect to skip the first render by adding an isInitialMount ref (e.g., const isInitialMount = useRef(true)) and in the useEffect for filters return early if isInitialMount.current is true (set it false afterward), otherwise call pagination.onSetPage(undefined, 1); this ensures useEffect (and pagination.onSetPage) only runs when filters actually change after mount.
358-385:⚠️ Potential issue | 🟠 Major
sortRulesstill uses an invalid comparator pattern.Several branches return only two outcomes and don’t handle equals, which can produce unstable/incomplete ordering.
Suggested fix
if (sortBy === rowFilter(AlertRulesFilterOptions.NAME)) { - return [...data].sort((a, b) => - a.name?.toLocaleLowerCase() < b.name?.toLocaleLowerCase() ? lower : upper, - ); + return [...data].sort((a, b) => { + const cmp = (a.name ?? '').localeCompare(b.name ?? '', undefined, { sensitivity: 'base' }); + return direction === 'asc' ? cmp : -cmp; + }); }🤖 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 358 - 385, The sortRules comparator currently returns only two outcomes and doesn't handle equality, causing unstable ordering; update each sort callback inside sortRules (branches for rowFilter(AlertRulesFilterOptions.NAME), SEVERITY, STATE, SOURCE) to return -1, 0, or 1: use localeCompare for string comparisons (Rule.name and alertingRuleSource results) and numeric comparison for severity/alertingRuleStateOrder, explicitly return 0 when equal, and optionally fall back to a stable secondary key (e.g., rule id or name) to guarantee deterministic ordering; keep references to rowFilter, AlertRulesFilterOptions, alertingRuleStateOrder, alertingRuleSource, and Rule.labels.severity when making the changes.web/src/components/alerting/filter-silences.ts (2)
35-37:⚠️ Potential issue | 🟠 MajorName filter target is likely incorrect (
silence.name).Filtering against
silence.namecan miss matches; use a searchable value derived from matchers.Suggested fix
+const searchableName = + silence.matchers?.map((m) => `${m.name}=${m.value}`).join(',') ?? ''; if ( selectedFilters[SilenceFilterOptions.NAME] && - !fuzzyCaseInsensitive(selectedFilters[SilenceFilterOptions.NAME], silence?.name) + !fuzzyCaseInsensitive(selectedFilters[SilenceFilterOptions.NAME], searchableName) ) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/filter-silences.ts` around lines 35 - 37, The NAME filter is using silence?.name which can miss matches; instead compute a searchable string from the silence matchers and filter against that. Update the fuzzyCaseInsensitive call (where selectedFilters[SilenceFilterOptions.NAME] is checked) to pass a derived searchable value built from the silence matchers (e.g., join matcher.name/matcher.value pairs or call an existing helper like getSilenceSearchableValue) rather than silence?.name so the Name filter matches against searchable matcher content.
24-29:⚠️ Potential issue | 🟠 MajorNamespace filtering logic is still effectively a no-op.
This branch negates an array result and checks the whole collection instead of the current
silence, so row-level namespace filtering does not work.Suggested fix
if ( shouldFilterNamespace && - !silences?.filter((s) => - s.matchers.some((m) => m.name === 'namespace' && m.value === namespace), - ) + !silence.matchers?.some((m) => m.name === 'namespace' && m.value === namespace) ) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/filter-silences.ts` around lines 24 - 29, The namespace check is applied to the whole silences array instead of the current silence, making row-level filtering a no-op; in the conditional that uses shouldFilterNamespace, replace the global silences?.filter(...) check with a matcher check against the current silence (use silence.matchers.some(m => m.name === 'namespace' && m.value === namespace) or its boolean inverse as intended) so each silence row is individually tested; update the conditional around shouldFilterNamespace to reference silence.matchers rather than silences to perform correct per-row namespace filtering.web/src/components/table-pagination.tsx (1)
52-63:⚠️ Potential issue | 🟠 MajorFallback handlers still dereference optional setters.
When
onPerPageSelect/onSetPagearen’t supplied, the fallback branches can call undefinedsetPage/setPerPage.Suggested fix
: (e, v) => { const firstRow = (page - 1) * perPage; - setPage(Math.floor(firstRow / v) + 1); - setPerPage(v); + setPage?.(Math.floor(firstRow / v) + 1); + setPerPage?.(v); } -onSetPage={onSetPage ? onSetPage : (e, v) => setPage(v)} +onSetPage={onSetPage ? onSetPage : (_e, v) => setPage?.(v)}Also applies to: 99-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table-pagination.tsx` around lines 52 - 63, The fallback handlers for onPerPageSelect and onSetPage dereference setPage/setPerPage even when those setters may be undefined; update the fallback implementations for onPerPageSelect and onSetPage to guard calls to setPage and setPerPage (e.g., check that setPage and setPerPage are defined before invoking them) or replace them with no-op fallbacks when the setters are unavailable, and apply the same guard to the duplicate block around lines 99-103 so neither fallback calls undefined setters (refer to the onPerPageSelect, onSetPage, setPage, setPerPage, page, and perPage symbols).web/src/components/table/TableTextFilter.tsx (1)
16-31:⚠️ Potential issue | 🟠 MajorGuard
/shortcut when a modal/dialog is open.The key handler still does not bail out for open dialogs, so
/can move focus behind an active modal.Suggested fix
const handleKeyDown = (event: KeyboardEvent) => { + const hasOpenDialog = document.querySelector('[role="dialog"]:not([aria-hidden="true"])'); + if (hasOpenDialog) { + return; + } if (event.key === '/' && !event.ctrlKey && !event.metaKey && !event.altKey) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/TableTextFilter.tsx` around lines 16 - 31, The key handler in useEffect (handleKeyDown) needs to bail out when a modal/dialog is open so pressing "/" doesn't focus the table filter behind an active modal; add an early guard in handleKeyDown that checks for open dialogs/modals (e.g., document.querySelector('dialog[open], [role="dialog"][open], [data-modal-open], .modal.show') or any project-specific modal attribute) and return if any are found, before checking showToolbarItem or focusing the inputElement identified by filterId.web/src/components/table/useTableFilters.ts (1)
60-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResync local filters when URL params change.
Line 60 mounts once only, so navigation updates can leave
filtersstale.Suggested fix
- // Initialize filters from URL parameters on mount only - useEffect(() => { - onSetFilters(getInitialFilters()); - }, []); // eslint-disable-line react-hooks/exhaustive-deps + useEffect(() => { + setFilters(getInitialFilters()); + }, [getInitialFilters]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/useTableFilters.ts` around lines 60 - 63, The useEffect in useTableFilters currently only runs on mount so onSetFilters(getInitialFilters()) doesn't resync when URL params change; update the effect to depend on the URL/search changes (or the getInitialFilters result) so it re-runs when navigation updates occur — modify the useEffect that calls onSetFilters to include the relevant dependency (e.g., location.search or getInitialFilters) so filters stay in sync with URL params.web/src/components/table/TableToolbar.tsx (1)
12-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare
clearAllFiltersin props and stop freezing the clear-button callback.Line 31 destructures
clearAllFiltersbutDataViewToolbarPropsdoesn’t declare it, and Lines 35-48 freeze the initial button node in a ref, so handler updates won’t propagate.Suggested fix
-import { FC, PropsWithChildren, useRef } from 'react'; +import { FC, PropsWithChildren } from 'react'; ... export interface DataViewToolbarProps extends Omit<PropsWithChildren<ToolbarProps>, 'ref'> { ... /** React node to display filters */ filters?: React.ReactNode; + /** Callback to clear all filters */ + clearAllFilters?: () => void; } ... - const defaultClearFilters = useRef( + const clearFiltersContent = ( <ToolbarItem> <Button ouiaId={`${ouiaId}-clear-all-filters`} variant="link" onClick={clearAllFilters} isInline > Clear filters </Button> </ToolbarItem>, - ); + ); return ( - <Toolbar ouiaId={ouiaId} customLabelGroupContent={defaultClearFilters.current} {...props}> + <Toolbar ouiaId={ouiaId} customLabelGroupContent={clearFiltersContent} {...props}>#!/bin/bash rg -n "interface DataViewToolbarProps|clearAllFilters|useRef|customLabelGroupContent" web/src/components/table/TableToolbar.tsxAlso applies to: 31-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/TableToolbar.tsx` around lines 12 - 23, Add clearAllFilters to the props interface and stop "freezing" the button node so updates to its handler propagate: update DataViewToolbarProps to include clearAllFilters?: () => void, remove the pattern that stores the initial button DOM/node in a ref (the frozenRef used around customLabelGroupContent / clear button), and instead render the clear button directly (or use a callback ref that sets the latest node) so the onClick handler passed from props (clearAllFilters) is invoked correctly when it changes; ensure the TableToolbar component uses the clearAllFilters prop rather than a stale frozen handler.web/src/components/alerting/SilencesPage.tsx (1)
150-152:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMatch selected rows by stable silence identity, not
index.Line 151 rebinds selection when rows are rebuilt/sorted, so bulk expire can hit the wrong records.
Suggested fix
const selection = useDataViewSelection({ - matchOption: (a, b) => a?.index === b?.index, + matchOption: (a, b) => a?.silence?.id === b?.silence?.id, });🤖 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 150 - 152, The selection currently matches rows by a transient index (useDataViewSelection({ matchOption: (a, b) => a?.index === b?.index })), which breaks when rows are rebuilt/sorted; change matchOption to compare a stable silence identifier (e.g. a?.id or a?.silenceID === b?.id or b?.silenceID) so selection tracks the actual silence record across reorders/refreshes — update the matchOption passed to useDataViewSelection to use that stable id field used on your row objects.web/src/components/targets-page.tsx (2)
466-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t reset pagination on initial render.
Line 466 currently forces deep-linked pages back to
1on first mount before user interaction.Suggested fix
+ const didMountRef = useRef(false); useEffect(() => { - // When changing filters change back to being on page 1 - pagination.onSetPage(undefined, 1); + if (didMountRef.current) { + pagination.onSetPage(undefined, 1); + } else { + didMountRef.current = true; + } }, [filters]); // eslint-disable-line react-hooks/exhaustive-deps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/targets-page.tsx` around lines 466 - 469, The useEffect that calls pagination.onSetPage(undefined, 1) is resetting deep-linked pages on initial mount; update the effect (the useEffect watching filters) to skip the call on first render (e.g., use an isFirstRender ref or a previous-filters check) so pagination.onSetPage only runs when filters change after mount, leaving deep-linked page state intact; modify the effect around pagination.onSetPage to return early on the initial render.
676-703:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
sortTargetscomparator must return negative/zero/positive values.Current branches return
0/1and don’t handle equality, so sort order is unreliable.Suggested fix
- const lower = direction === 'asc' ? 0 : 1; - const upper = direction === 'asc' ? 1 : 0; + const dir = direction === 'asc' ? 1 : -1; ... - return [...data].sort((a, b) => - a.scrapeUrl?.toLocaleLowerCase() < b.scrapeUrl?.toLocaleLowerCase() ? lower : upper, - ); + return [...data].sort( + (a, b) => + dir * + (a.scrapeUrl ?? '') + .toLocaleLowerCase() + .localeCompare((b.scrapeUrl ?? '').toLocaleLowerCase()), + );#!/bin/bash rg -n "const sortTargets|\\.sort\\(\\(a, b\\).*\\? lower : upper|const lower =|const upper =" web/src/components/targets-page.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/targets-page.tsx` around lines 676 - 703, The comparator in sortTargets returns 0/1 and ignores equality; change it to return negative/zero/positive values and handle undefined/equality properly: replace lower/upper constants with -1 and 1 (or directly return numeric differences), use localeCompare for string fields (scrapeUrl and labels?.namespace) with toLocaleLowerCase or handle undefined, use numeric subtraction or timestamp difference for numeric/date fields (health, lastScrapeDuration, lastScrape converted to millis), and ensure the branch for TargetsFilterOptions.STATUS uses the same numeric comparison; update the sort callbacks in sortTargets (rows using rowFilter('scrapeUrl'), rowFilter('health'), rowFilter('namespace'), rowFilter('lastScrape'), rowFilter('lastScrapeDuration'), and rowFilter(TargetsFilterOptions.STATUS)) to return -1/0/1 via these comparisons and handle null/undefined consistently.
🧹 Nitpick comments (1)
web/src/components/alerting/AlertList/AggregateAlertTableRow.tsx (1)
13-20: Use a type-only import forAggregatedAlertFilters.
AlertsPage.tsximportsAggregateAlertTableRow, which importsAggregatedAlertFiltersfromAlertsPage.tsx, creating a circular dependency. Since this symbol is only referenced in a type annotation,import typebreaks the circular dependency at runtime while preserving type safety.♻️ Proposed change
-import { AggregatedAlertFilters } from '../AlertsPage'; +import type { AggregatedAlertFilters } from '../AlertsPage';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/alerting/AlertList/AggregateAlertTableRow.tsx` around lines 13 - 20, The import of AggregatedAlertFilters in AggregateAlertTableRow.tsx creates a runtime circular dependency because it's only used as a type; change that import to a type-only import (i.e., use import type { AggregatedAlertFilters } from '...') so the symbol is erased at runtime and the circular dependency is broken while preserving type information for the AggregateAlertTableRow props.
🤖 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/cypress/views/list-page.ts`:
- Around line 155-157: The current contains(new RegExp(`^${tagName}$`)) call can
break when tagName contains regex metacharacters; fix by escaping tagName before
constructing the RegExp (e.g., add or use an escapeRegExp helper and call
contains(new RegExp(`^${escapeRegExp(tagName)}$`))). Update the code around
Classes.IndividualTag and the RegExp construction to use the escaped value so
tag matching is exact and not treated as a regex pattern.
In `@web/src/components/alerting/AlertRulesPage.tsx`:
- Around line 111-112: In AlertRulesPage.tsx the column header label for the
source column is wrong: the column defined with key
rowFilter(AlertRulesFilterOptions.SOURCE) is labeled t('Total'); change that
label to a meaningful source header (e.g., t('Source') or t('Alert source')) so
the header matches the key and UI. Update the array entry where { label:
t('Total'), key: rowFilter(AlertRulesFilterOptions.SOURCE) } is defined to use
the corrected t(...) value.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 197-200: The effect currently forces pagination to page 1 on
mount, breaking deep links; modify the useEffect that references filters so it
skips the initial render (e.g., add a firstRender ref checked inside the
useEffect) and only calls pagination.onSetPage(undefined, 1) on subsequent
filter changes; specifically, update the useEffect that uses filters to guard
the call to pagination.onSetPage (or alternatively only reset when
pagination.page !== 1) so deep-linked ?page=N is preserved on first render.
In `@web/src/components/table/TableCheckboxFilter.tsx`:
- Around line 83-86: The labels mapping in TableCheckboxFilter.tsx can produce
undefined keys/nodes when URL params include stale values; update the labels
generation to first filter value to only include items present in options (e.g.,
value.filter(item => options.some(option => option.value === item))) and then
map to { key: activeOption.value, node: activeOption.label } using the found
activeOption so you never create chips with undefined key/node; reference the
labels prop mapping, value, options and activeOption in your fix.
In `@web/src/components/table/TableLabelFilter.tsx`:
- Around line 45-48: The deleteLabel handler uses _.difference on objects so it
fails because ToolbarFilter passes a new object instance; update deleteLabel to
remove by the label key instead: call setLabelInputText('') then compute newKeys
from labelSelection (e.g., labelSelection.map(l => l.key).filter(k => k !==
label.key)) and pass that array to applyLabelFilters; replace the _.difference
usage with this key-based filter so deletion works reliably (references:
deleteLabel, setLabelInputText, applyLabelFilters, labelSelection,
ToolbarLabel).
---
Outside diff comments:
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 462-468: silenceStateOrder currently builds a tuple that
mistakenly includes itself and the sorting uses array-to-string comparisons;
remove the self-reference from silenceStateOrder so it returns [stateIndex,
timestamp] (use silenceState(silence) and SilenceStates to compute stateIndex
and pick startsAt vs endsAt), then implement a dedicated comparator function
(used by sortSilences) that compares those two tuples element-by-element and
returns -1/0/1 (compare stateIndex numerically, then timestamps with Date or
string compare, then a stable tiebreaker like silence.id); replace any usages
that do array1 > array2 or array comparisons with this comparator so sorting is
stable and correct.
---
Duplicate comments:
In `@web/src/components/alerting/AlertRulesPage.tsx`:
- Around line 145-148: The useEffect that resets pagination runs on initial
mount and overwrites deep-linked page state; modify the effect to skip the first
render by adding an isInitialMount ref (e.g., const isInitialMount =
useRef(true)) and in the useEffect for filters return early if
isInitialMount.current is true (set it false afterward), otherwise call
pagination.onSetPage(undefined, 1); this ensures useEffect (and
pagination.onSetPage) only runs when filters actually change after mount.
- Around line 358-385: The sortRules comparator currently returns only two
outcomes and doesn't handle equality, causing unstable ordering; update each
sort callback inside sortRules (branches for
rowFilter(AlertRulesFilterOptions.NAME), SEVERITY, STATE, SOURCE) to return -1,
0, or 1: use localeCompare for string comparisons (Rule.name and
alertingRuleSource results) and numeric comparison for
severity/alertingRuleStateOrder, explicitly return 0 when equal, and optionally
fall back to a stable secondary key (e.g., rule id or name) to guarantee
deterministic ordering; keep references to rowFilter, AlertRulesFilterOptions,
alertingRuleStateOrder, alertingRuleSource, and Rule.labels.severity when making
the changes.
In `@web/src/components/alerting/filter-silences.ts`:
- Around line 35-37: The NAME filter is using silence?.name which can miss
matches; instead compute a searchable string from the silence matchers and
filter against that. Update the fuzzyCaseInsensitive call (where
selectedFilters[SilenceFilterOptions.NAME] is checked) to pass a derived
searchable value built from the silence matchers (e.g., join
matcher.name/matcher.value pairs or call an existing helper like
getSilenceSearchableValue) rather than silence?.name so the Name filter matches
against searchable matcher content.
- Around line 24-29: The namespace check is applied to the whole silences array
instead of the current silence, making row-level filtering a no-op; in the
conditional that uses shouldFilterNamespace, replace the global
silences?.filter(...) check with a matcher check against the current silence
(use silence.matchers.some(m => m.name === 'namespace' && m.value === namespace)
or its boolean inverse as intended) so each silence row is individually tested;
update the conditional around shouldFilterNamespace to reference
silence.matchers rather than silences to perform correct per-row namespace
filtering.
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 150-152: The selection currently matches rows by a transient index
(useDataViewSelection({ matchOption: (a, b) => a?.index === b?.index })), which
breaks when rows are rebuilt/sorted; change matchOption to compare a stable
silence identifier (e.g. a?.id or a?.silenceID === b?.id or b?.silenceID) so
selection tracks the actual silence record across reorders/refreshes — update
the matchOption passed to useDataViewSelection to use that stable id field used
on your row objects.
In `@web/src/components/table-pagination.tsx`:
- Around line 52-63: The fallback handlers for onPerPageSelect and onSetPage
dereference setPage/setPerPage even when those setters may be undefined; update
the fallback implementations for onPerPageSelect and onSetPage to guard calls to
setPage and setPerPage (e.g., check that setPage and setPerPage are defined
before invoking them) or replace them with no-op fallbacks when the setters are
unavailable, and apply the same guard to the duplicate block around lines 99-103
so neither fallback calls undefined setters (refer to the onPerPageSelect,
onSetPage, setPage, setPerPage, page, and perPage symbols).
In `@web/src/components/table/TableTextFilter.tsx`:
- Around line 16-31: The key handler in useEffect (handleKeyDown) needs to bail
out when a modal/dialog is open so pressing "/" doesn't focus the table filter
behind an active modal; add an early guard in handleKeyDown that checks for open
dialogs/modals (e.g., document.querySelector('dialog[open],
[role="dialog"][open], [data-modal-open], .modal.show') or any project-specific
modal attribute) and return if any are found, before checking showToolbarItem or
focusing the inputElement identified by filterId.
In `@web/src/components/table/TableToolbar.tsx`:
- Around line 12-23: Add clearAllFilters to the props interface and stop
"freezing" the button node so updates to its handler propagate: update
DataViewToolbarProps to include clearAllFilters?: () => void, remove the pattern
that stores the initial button DOM/node in a ref (the frozenRef used around
customLabelGroupContent / clear button), and instead render the clear button
directly (or use a callback ref that sets the latest node) so the onClick
handler passed from props (clearAllFilters) is invoked correctly when it
changes; ensure the TableToolbar component uses the clearAllFilters prop rather
than a stale frozen handler.
In `@web/src/components/table/useTableFilters.ts`:
- Around line 60-63: The useEffect in useTableFilters currently only runs on
mount so onSetFilters(getInitialFilters()) doesn't resync when URL params
change; update the effect to depend on the URL/search changes (or the
getInitialFilters result) so it re-runs when navigation updates occur — modify
the useEffect that calls onSetFilters to include the relevant dependency (e.g.,
location.search or getInitialFilters) so filters stay in sync with URL params.
In `@web/src/components/targets-page.tsx`:
- Around line 466-469: The useEffect that calls pagination.onSetPage(undefined,
1) is resetting deep-linked pages on initial mount; update the effect (the
useEffect watching filters) to skip the call on first render (e.g., use an
isFirstRender ref or a previous-filters check) so pagination.onSetPage only runs
when filters change after mount, leaving deep-linked page state intact; modify
the effect around pagination.onSetPage to return early on the initial render.
- Around line 676-703: The comparator in sortTargets returns 0/1 and ignores
equality; change it to return negative/zero/positive values and handle
undefined/equality properly: replace lower/upper constants with -1 and 1 (or
directly return numeric differences), use localeCompare for string fields
(scrapeUrl and labels?.namespace) with toLocaleLowerCase or handle undefined,
use numeric subtraction or timestamp difference for numeric/date fields (health,
lastScrapeDuration, lastScrape converted to millis), and ensure the branch for
TargetsFilterOptions.STATUS uses the same numeric comparison; update the sort
callbacks in sortTargets (rows using rowFilter('scrapeUrl'),
rowFilter('health'), rowFilter('namespace'), rowFilter('lastScrape'),
rowFilter('lastScrapeDuration'), and rowFilter(TargetsFilterOptions.STATUS)) to
return -1/0/1 via these comparisons and handle null/undefined consistently.
---
Nitpick comments:
In `@web/src/components/alerting/AlertList/AggregateAlertTableRow.tsx`:
- Around line 13-20: The import of AggregatedAlertFilters in
AggregateAlertTableRow.tsx creates a runtime circular dependency because it's
only used as a type; change that import to a type-only import (i.e., use import
type { AggregatedAlertFilters } from '...') so the symbol is erased at runtime
and the circular dependency is broken while preserving type information for the
AggregateAlertTableRow props.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 470936ab-ff53-4895-bcc3-7cadf9581b42
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.gitignoreweb/cypress/README.mdweb/cypress/support/commands/traces-logging-commands.tsweb/cypress/support/monitoring/00.bvt_monitoring.cy.tsweb/cypress/support/monitoring/00.bvt_monitoring_namespace.cy.tsweb/cypress/support/monitoring/01.reg_alerts.cy.tsweb/cypress/support/monitoring/04.reg_alerts_namespace.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/list-page.tsweb/cypress/views/silences-list-page.tsweb/eslint.config.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/components/MetricsPage.tsxweb/src/components/alerting/AlertList/AggregateAlertTableRow.tsxweb/src/components/alerting/AlertList/DownloadCSVButton.tsxweb/src/components/alerting/AlertList/LabelFilter.tsxweb/src/components/alerting/AlertList/filter-alerts.tsweb/src/components/alerting/AlertList/hooks/useAggregateAlertColumns.tsweb/src/components/alerting/AlertList/hooks/utils.tsweb/src/components/alerting/AlertRulesPage.tsxweb/src/components/alerting/AlertUtils.tsxweb/src/components/alerting/AlertingPage.tsxweb/src/components/alerting/AlertsAggregates.tsweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/SilencesUtils.tsxweb/src/components/alerting/filter-rules.tsweb/src/components/alerting/filter-silences.tsweb/src/components/alerting/useSelectedFilters.tsweb/src/components/alerting/useSortAlerts.tsweb/src/components/console/console-shared/constants/common.tsweb/src/components/console/console-shared/hooks/useDocumentListener.tsweb/src/components/console/models/index.tsweb/src/components/console/public/components/autocomplete.tsxweb/src/components/console/public/components/factory/text-filter.tsxweb/src/components/dashboards/legacy/table.tsxweb/src/components/dashboards/perses/dashboard-list.tsxweb/src/components/dashboards/perses/hooks/useDashboardsData.tsweb/src/components/data-test.tsweb/src/components/filter-targets.tsweb/src/components/query-browser.tsxweb/src/components/table-pagination.tsxweb/src/components/table/TableCheckboxFilter.tsxweb/src/components/table/TableFilters.tsxweb/src/components/table/TableLabelFilter.tsxweb/src/components/table/TableTextFilter.tsxweb/src/components/table/TableToolbar.tsxweb/src/components/table/useTableFilters.tsweb/src/components/table/useTablePagination.tsweb/src/components/targets-page.tsx
💤 Files with no reviewable changes (7)
- web/src/components/dashboards/perses/hooks/useDashboardsData.ts
- web/src/components/query-browser.tsx
- web/src/components/alerting/AlertList/hooks/useAggregateAlertColumns.ts
- web/cypress/support/commands/traces-logging-commands.ts
- web/src/components/alerting/AlertList/hooks/utils.ts
- web/src/components/alerting/useSelectedFilters.ts
- web/src/components/alerting/useSortAlerts.ts
✅ Files skipped from review due to trivial changes (8)
- web/package.json
- web/src/components/alerting/AlertingPage.tsx
- web/src/components/console/console-shared/constants/common.ts
- .gitignore
- web/cypress/README.md
- web/src/components/MetricsPage.tsx
- web/src/components/console/models/index.ts
- web/eslint.config.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- web/src/components/alerting/AlertList/DownloadCSVButton.tsx
- web/src/components/dashboards/legacy/table.tsx
- web/src/components/alerting/filter-rules.ts
- web/src/components/alerting/AlertList/LabelFilter.tsx
- web/src/components/dashboards/perses/dashboard-list.tsx
- web/cypress/support/monitoring/00.bvt_monitoring_namespace.cy.ts
- web/src/components/filter-targets.ts
- web/src/components/alerting/AlertsAggregates.ts
- web/src/components/table/useTablePagination.ts
- web/src/components/console/public/components/autocomplete.tsx
- web/src/components/alerting/AlertsPage.tsx
- web/cypress/views/alerting-rule-list-page.ts
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
web/src/components/table-pagination.tsx (1)
34-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback handlers still call optional setters without guards.
When
onPerPageSelectoronSetPageare not provided, the fallback handlers callsetPageandsetPerPageunconditionally (Lines 41-42 and 45), but these are now optional props (Lines 73-74). If a caller provides neither the callback overrides nor the setters, this will cause a runtime error.🔧 Suggested fix: Add guards before calling optional setters
onPerPageSelect={ onPerPageSelect ? onPerPageSelect : (e, v) => { // When changing the number of results per page, // keep the start row approximately the same const firstRow = (page - 1) * perPage; - setPage(Math.floor(firstRow / v) + 1); - setPerPage(v); + setPage?.(Math.floor(firstRow / v) + 1); + setPerPage?.(v); } } - onSetPage={onSetPage ? onSetPage : (e, v) => setPage(v)} + onSetPage={onSetPage ? onSetPage : (e, v) => setPage?.(v)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table-pagination.tsx` around lines 34 - 45, The fallback handlers for onPerPageSelect and onSetPage call optional setters unguarded (onPerPageSelect, onSetPage, setPage, setPerPage), so add guards before invoking them: in the inline fallback for onPerPageSelect compute firstRow and newPage as now, then only call setPage(newPage) and setPerPage(v) if those functions are defined (or use optional chaining/setPage?.(newPage), setPerPage?.(v)); similarly, in the onSetPage fallback only call setPage(v) when setPage is provided. Ensure you keep page and perPage usage for calculating firstRow/newPage but avoid any unconditional calls to the optional setters.
🧹 Nitpick comments (2)
web/src/components/filter-targets.ts (1)
10-15: 💤 Low valueDocumentation refers to "alerts" but this function filters "targets".
The JSDoc comment mentions "alerts" (lines 11-14) but this function filters
Target[]objects. This appears to be a copy-paste fromfilter-alerts.ts.📝 Suggested documentation fix
/** - * Filters alerts based on tenancy: - * - with tenancy: alerts are automatically pre-filtered. + * Filters targets based on tenancy: + * - with tenancy: targets are automatically pre-filtered. * - without tenancy (admin): filters by selected namespace for UX consistency. - * - "All Projects": returns all alerts, including those without a namespace label. + * - "All Projects": returns all targets, including those without a namespace label. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/filter-targets.ts` around lines 10 - 15, The JSDoc incorrectly references "alerts" while this module deals with Target[]; update the comment to refer to "targets" (and "Target[]") and adjust wording accordingly so it accurately describes filterTargets' behavior: pre-filtering when tenancy exists, admin behavior filtering by selected namespace, and "All Projects" returning all targets (including those without a namespace label); ensure any mention of related filenames or functions references filter-targets.ts and the filterTargets function/Target type for clarity.web/src/components/table/TableCheckboxFilter.tsx (1)
41-41: 💤 Low valueUnused
containerRef-appendTowill always receiveundefined.
containerRefis created but never attached to any DOM element via arefprop. At Line 142,appendTo={containerRef.current || undefined}will always resolve toundefined. Either remove the ref or attach it to the intended container element.🔧 Suggested fix: Remove unused ref
export const TableCheckboxFilter: FC<CustomDataViewCheckboxFilterProps> = ({ ... }: CustomDataViewCheckboxFilterProps) => { const [isOpen, setIsOpen] = useState(false); const toggleRef = useRef<HTMLButtonElement>(null); const menuRef = useRef<HTMLDivElement>(null); - const containerRef = useRef<HTMLDivElement>(null); ... popperRef={menuRef} - appendTo={containerRef.current || undefined} + appendTo={undefined} aria-label={`${title ?? filterId} filter`}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/TableCheckboxFilter.tsx` at line 41, In TableCheckboxFilter, containerRef is created but never attached, so appendTo={containerRef.current || undefined} always yields undefined; fix by either removing the unused containerRef and passing undefined (or omitting appendTo) or by attaching containerRef to the intended DOM node (e.g., add ref={containerRef} to the wrapper div or the element meant to host the popup) so containerRef.current is populated before it’s used in the component that receives appendTo; update the code that references containerRef (the appendTo prop) accordingly.
🤖 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 279-281: The CSV download currently uses the paginated slice
selectedPageOfAggregatedAlerts in AlertsPage when rendering <DownloadCSVButton
loaded={loaded} filteredData={selectedPageOfAggregatedAlerts} />, causing
exports to include only the current page; to restore prior behavior, pass the
full filtered set aggregatedAlerts instead (i.e., use
filteredData={aggregatedAlerts}), or if the intention is to keep page-only
exports, update the button label/tooltip in DownloadCSVButton to indicate
“Download current page” so users aren’t surprised; update the prop usage and any
corresponding tests or docs referencing DownloadCSVButton accordingly.
In `@web/src/components/alerting/filter-rules.ts`:
- Around line 42-48: selectedFilters[AlertRulesFilterOptions.SOURCE] is optional
and may be undefined, but the code in the filtering branch uses .length and
.includes directly; update the filter logic in the function that uses
selectedFilters, guarding the SOURCE access (e.g., check for existence with
selectedFilters[AlertRulesFilterOptions.SOURCE] != null or use a safe default
array) before checking length and includes so alertingRuleSource(rule) is only
compared when SOURCE is present; adjust the condition that currently references
AlertRulesFilterOptions.SOURCE, selectedFilters, and alertingRuleSource(rule)
accordingly to avoid a TypeError.
In `@web/src/components/console/console-shared/hooks/useDocumentListener.ts`:
- Around line 41-53: In useDocumentListener, guard access to ref.current before
calling DOM methods: inside the KeyEventModes.HIDE case check that ref.current
is truthy before calling ref.current.blur() (and still call setVisible(false)),
and inside KeyEventModes.FOCUS verify ref.current exists (and optionally is an
HTMLElement) before checking document.activeElement !== ref.current and before
calling ref.current.focus() and e.preventDefault(); update the conditional
branches around ref.current to avoid dereferencing a null ref.
In `@web/src/components/console/public/components/autocomplete.tsx`:
- Around line 59-63: The prop suggestionCount on AutocompleteInputProps is
declared but never used — replace hard-coded uses of MAX_SUGGESTIONS inside the
AutocompleteInput component (and any helper functions like
getFilteredSuggestions/getVisibleSuggestions) with a computed limit that prefers
the caller-provided suggestionCount (e.g. const limit = suggestionCount ??
MAX_SUGGESTIONS) and use limit when slicing or rendering suggestions; also
ensure suggestionCount is pulled from props in the component signature and
passed through to any internal functions that currently assume MAX_SUGGESTIONS.
In `@web/src/components/table/useTableFilters.ts`:
- Around line 75-81: deleteFilter currently removes the filter key entirely from
the filters object using the delete operator which differs from onDeleteFilters'
approach of resetting filter values to empty strings/arrays and can break
consumers that expect all keys to exist; update deleteFilter (the useCallback
that calls setFilters) to mirror onDeleteFilters' reset logic by replacing the
value for filterToDelete with the appropriate empty value (string or array)
instead of deleting the property, using the same detection/normalization logic
you use in onDeleteFilters so Object.keys(filters) and downstream sync code
continue to see the key.
In `@web/src/components/targets-page.tsx`:
- Around line 409-411: The initialSort is using
rowFilter(TargetsFilterOptions.NAME) ('name') but the columns use
rowFilter('scrapeUrl') for the Endpoint column, causing the initial sort key to
mismatch; update the initialSort passed to useDataViewSort so sortBy uses the
same rowFilter value as the column you want to sort by (e.g., replace
rowFilter(TargetsFilterOptions.NAME) with rowFilter('scrapeUrl')) or
alternatively change the Endpoint column key to use
rowFilter(TargetsFilterOptions.NAME) so both sides reference the same key
(ensure the change is applied where useDataViewSort is called and where the
column keys are defined).
In `@web/src/components/utils.ts`:
- Around line 139-155: severitySort must implement a strict total-order
comparator: derive a concrete severity string for a and b (preserving the
existing logic using 'severity' property or labels?.severity), map known
severities to numeric ranks (e.g., Critical highest, Warning next, None lowest)
and assign a stable rank for unknown/empty severities, then compute rank
difference (return rankB - rankA so higher severity sorts first) and only when
ranks are equal fall back to a deterministic localeCompare on the severity
strings to break ties; update the severitySort function to use this
ranking+fallback approach so reversed inputs cannot both return positive values.
---
Duplicate comments:
In `@web/src/components/table-pagination.tsx`:
- Around line 34-45: The fallback handlers for onPerPageSelect and onSetPage
call optional setters unguarded (onPerPageSelect, onSetPage, setPage,
setPerPage), so add guards before invoking them: in the inline fallback for
onPerPageSelect compute firstRow and newPage as now, then only call
setPage(newPage) and setPerPage(v) if those functions are defined (or use
optional chaining/setPage?.(newPage), setPerPage?.(v)); similarly, in the
onSetPage fallback only call setPage(v) when setPage is provided. Ensure you
keep page and perPage usage for calculating firstRow/newPage but avoid any
unconditional calls to the optional setters.
---
Nitpick comments:
In `@web/src/components/filter-targets.ts`:
- Around line 10-15: The JSDoc incorrectly references "alerts" while this module
deals with Target[]; update the comment to refer to "targets" (and "Target[]")
and adjust wording accordingly so it accurately describes filterTargets'
behavior: pre-filtering when tenancy exists, admin behavior filtering by
selected namespace, and "All Projects" returning all targets (including those
without a namespace label); ensure any mention of related filenames or functions
references filter-targets.ts and the filterTargets function/Target type for
clarity.
In `@web/src/components/table/TableCheckboxFilter.tsx`:
- Line 41: In TableCheckboxFilter, containerRef is created but never attached,
so appendTo={containerRef.current || undefined} always yields undefined; fix by
either removing the unused containerRef and passing undefined (or omitting
appendTo) or by attaching containerRef to the intended DOM node (e.g., add
ref={containerRef} to the wrapper div or the element meant to host the popup) so
containerRef.current is populated before it’s used in the component that
receives appendTo; update the code that references containerRef (the appendTo
prop) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1cc5bd5e-b77c-41e2-92fb-d600aa533a3f
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
scripts/start-console.shweb/cypress/support/monitoring/01.reg_alerts.cy.tsweb/cypress/support/monitoring/04.reg_alerts_namespace.cy.tsweb/cypress/views/alerting-rule-list-page.tsweb/cypress/views/list-page.tsweb/jest.config.jsweb/jest.setup.tsweb/locales/en/plugin__monitoring-plugin.jsonweb/package.jsonweb/src/components/Incidents/api.spec.tsweb/src/components/alerting/AlertList/DownloadCSVButton.tsxweb/src/components/alerting/AlertList/filter-alerts.spec.tsweb/src/components/alerting/AlertRulesPage.tsxweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/filter-rules.spec.tsweb/src/components/alerting/filter-rules.tsweb/src/components/alerting/filter-silences.spec.tsweb/src/components/alerting/filter-silences.tsweb/src/components/console/console-shared/hooks/useDocumentListener.tsweb/src/components/console/public/components/autocomplete.tsxweb/src/components/filter-targets.spec.tsweb/src/components/filter-targets.tsweb/src/components/table-pagination.tsxweb/src/components/table/TableCheckboxFilter.tsxweb/src/components/table/TableLabelFilter.tsxweb/src/components/table/TableToolbar.tsxweb/src/components/table/sort-utils.spec.tsweb/src/components/table/sort-utils.tsweb/src/components/table/useTableColumns.tsweb/src/components/table/useTableFilters.tsweb/src/components/table/useTablePagination.tsweb/src/components/targets-page.tsxweb/src/components/utils.spec.tsweb/src/components/utils.ts
💤 Files with no reviewable changes (1)
- web/src/components/Incidents/api.spec.ts
✅ Files skipped from review due to trivial changes (3)
- web/jest.setup.ts
- web/package.json
- web/src/components/table/sort-utils.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- web/src/components/alerting/AlertList/DownloadCSVButton.tsx
- web/src/components/table/TableLabelFilter.tsx
- web/src/components/alerting/filter-silences.ts
- web/src/components/table/TableToolbar.tsx
- web/cypress/views/alerting-rule-list-page.ts
- web/locales/en/plugin__monitoring-plugin.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/console/public/components/autocomplete.tsx`:
- Around line 20-23: getLabelsAsString currently calls mapLabelsToStrings on any
truthy result from _.get(obj, path), which can crash if the value is a
non-object (string/array). Change the guard in getLabelsAsString to ensure the
retrieved labels is an object map before calling mapLabelsToStrings (e.g., check
_.isPlainObject(labels) or (typeof labels === 'object' && labels !== null &&
!Array.isArray(labels))); if the check fails return an empty array. This targets
the getLabelsAsString function and prevents mapLabelsToStrings from receiving
invalid types.
In `@web/src/components/utils.ts`:
- Around line 153-157: severitySort currently orders severities ascending (uses
rankA - rankB) which conflicts with call sites that expect index 0 to be highest
severity; in the severitySort comparator (variables rankA, rankB, severityA,
severityB) invert the numeric comparison to return rankB - rankA so
higher-ranked severities come first, leaving the localeCompare fallback
unchanged for equal ranks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8f2e53f7-1a7c-4805-acb2-9be4223d044e
📒 Files selected for processing (7)
web/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/filter-rules.tsweb/src/components/console/console-shared/hooks/useDocumentListener.tsweb/src/components/console/public/components/autocomplete.tsxweb/src/components/table/useTableFilters.tsweb/src/components/targets-page.tsxweb/src/components/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/targets-page.tsx
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
jgbernalp
force-pushed
the
ou-1091-aggregated-alerts
branch
from
71104d9 to
9eb88ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
web/src/components/alerting/SilencesPage.tsx (2)
143-145:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMatch selected rows by
silence.id, notsilence.name.
nameis not a stable identity. If two silences share a name, selection can collapse onto the wrong row and the bulk expire action will operate on the wrong silence.🤖 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 143 - 145, The selection comparator currently matches rows by silence.name which is not stable; update the matchOption in the useDataViewSelection call to compare silence IDs instead (e.g., change the comparator used in selection/matchOption to compare a?.silence?.id === b?.silence?.id), ensuring you guard for undefined/null silences on both sides so selection and bulk-expire operate on the correct silence entities.
171-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t reset deep-linked pagination on first render.
Like the targets page, this effect runs immediately on mount and overwrites
?page=Nwith page 1 even when the user has not changed any filter yet.🤖 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 171 - 174, The useEffect in SilencesPage.tsx currently calls pagination.onSetPage(undefined, 1) on mount because it reacts to filters immediately; change it to skip the first render by introducing a local ref flag (e.g., isInitialMount) and only call pagination.onSetPage when isInitialMount.current is false (set it to true on first render then to false afterwards), so filter changes after mount still reset to page 1 but deep-linked ?page=N is preserved on initial load; update the useEffect that references filters and pagination.onSetPage to use this initial-mount guard.web/src/components/table/useTableFilters.ts (1)
14-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResync local filters when the URL or
initialFilterschange.
filtersis seeded fromsearchParamsonly once, and after that this hook only writes back to the URL. Back/forward navigation or a changed filter shape will leave local state stale and out of sync with the query string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/table/useTableFilters.ts` around lines 14 - 45, The local filters state in useTableFilters is only initialized once and never updated when searchParams or initialFilters change, causing it to get out of sync; add a useEffect inside useTableFilters that listens to changes in searchParams and initialFilters and re-computes/sets filters using the same logic used in the initial useState initializer (respecting Array.isArray(initialFilters[key]), using searchParams.get/getAll, treating empty arrays as null, and falling back to initialFilters[key]) so that setFilters is updated whenever the URL or initialFilters shape changes.web/src/components/targets-page.tsx (1)
427-430:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip the initial filter effect before resetting pagination.
This fires on mount, so deep-linked targets pages opened with
?page=Nget forced back to page 1 before the user changes any filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/targets-page.tsx` around lines 427 - 430, The effect that calls pagination.onSetPage(undefined, 1) runs on mount and resets deep-linked pages; modify the useEffect that currently depends on filters so it skips its first invocation: add a mounted/ref guard (e.g., mountedRef or prevFiltersRef) and in the effect return early when the component is mounting (or when previous filters are undefined) so pagination.onSetPage is only called on subsequent filter changes; update the useEffect surrounding pagination.onSetPage and references to filters accordingly (useEffect, pagination.onSetPage, filters).
🧹 Nitpick comments (2)
web/src/components/utils.ts (1)
139-157: ⚡ Quick winAvoid re-allocating
rankinside comparator hot path.
severitySortis called repeatedly duringArray.sort(including nested sorts inweb/src/components/alerting/SilencesPage.tsx), so constructingnew Map(...)per comparison adds avoidable overhead. Hoist the rank map to module scope.♻️ Proposed change
+const severityRank = new Map<string, number>([ + [AlertSeverity.None, 0], + [AlertSeverity.Info, 1], + [AlertSeverity.Warning, 2], + [AlertSeverity.Critical, 3], +]); export const severitySort = ( a: Alert | Rule | AggregatedAlert, b: Alert | Rule | AggregatedAlert, ): number => { const severityA = (('severity' in a ? a.severity : a.labels?.severity) ?? '').toLowerCase(); const severityB = (('severity' in b ? b.severity : b.labels?.severity) ?? '').toLowerCase(); - - const rank = new Map<string, number>([ - [AlertSeverity.None, 0], - [AlertSeverity.Info, 1], - [AlertSeverity.Warning, 2], - [AlertSeverity.Critical, 3], - ]); - - const rankA = rank.get(severityA) ?? -1; - const rankB = rank.get(severityB) ?? -1; + const rankA = severityRank.get(severityA) ?? -1; + const rankB = severityRank.get(severityB) ?? -1; if (rankA !== rankB) return rankB - rankA; return severityA.localeCompare(severityB, undefined, { sensitivity: 'base' }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/utils.ts` around lines 139 - 157, The comparator severitySort is recreating the rank Map on every comparison; hoist that map to module scope (e.g., const SEVERITY_RANK = new Map<string, number>(...)) and have severitySort reference that shared map instead of constructing it inside the function; update any references to rank (rankA, rankB) to use SEVERITY_RANK.get(...) so the comparator reuses the same allocation and avoids per-call overhead.web/src/components/console/public/components/autocomplete.tsx (1)
108-119: ⚡ Quick winReset suggestions when the effect preconditions fail.
When the condition is false,
suggestionsremains stale. IftextValueis externally cleared whilevisiblestays true, old suggestions can linger.Suggested fix
useEffect(() => { if (textValue && visible && showSuggestions) { const processed = labelParser(data, labelPath); // User input without whitespace const processedText = textValue.trim().replace(/\s*=\s*/, '='); const maxSuggestions = suggestionCount ?? MAX_SUGGESTIONS; const filtered = [...processed] .filter((item) => fuzzyCaseInsensitive(processedText, item)) .slice(0, maxSuggestions); setSuggestions(filtered); + } else { + setSuggestions([]); } }, [visible, textValue, showSuggestions, data, labelPath, suggestionCount]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/console/public/components/autocomplete.tsx` around lines 108 - 119, The effect that builds suggestions inside useEffect (in the Autocomplete component) only updates suggestions when the if condition passes, leaving stale suggestions otherwise; update the cleanup branch so when !(textValue && visible && showSuggestions) you call setSuggestions([]) to clear stale results (reference useEffect, setSuggestions, textValue, visible, showSuggestions, labelParser and fuzzyCaseInsensitive) so external clears of textValue immediately remove suggestions.
🤖 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/filter-rules.ts`:
- Around line 50-58: The label parsing currently uses raw split(',') and
split('=') which leaves surrounding whitespace in tokens; update the block that
iterates selectedFilters[AlertRulesFilterOptions.LABEL] and labelMatchers so you
first trim each labelMatcher (e.g., labelMatcher.trim()) before splitting, and
after split assign const key = keyValue[0].trim() and const value =
keyValue[1].trim() prior to comparing with rule.labels?.[key]; this ensures
inputs like "severity = critical" match correctly and avoids false negatives.
In `@web/src/components/console/public/components/autocomplete.tsx`:
- Around line 27-31: The labelParser function assumes its first arg is an array
and calls reduce, which will crash for null/non-array inputs; update labelParser
to guard/coerce its resources parameter (e.g., use Array.isArray(resources) ?
resources : []) before reducing and then iterate calling
getLabelsAsString(resource, labelPath) as before so it safely returns an empty
Set for non-array inputs; ensure you only change the input normalization inside
labelParser so callers (and getLabelsAsString) are unaffected.
---
Duplicate comments:
In `@web/src/components/alerting/SilencesPage.tsx`:
- Around line 143-145: The selection comparator currently matches rows by
silence.name which is not stable; update the matchOption in the
useDataViewSelection call to compare silence IDs instead (e.g., change the
comparator used in selection/matchOption to compare a?.silence?.id ===
b?.silence?.id), ensuring you guard for undefined/null silences on both sides so
selection and bulk-expire operate on the correct silence entities.
- Around line 171-174: The useEffect in SilencesPage.tsx currently calls
pagination.onSetPage(undefined, 1) on mount because it reacts to filters
immediately; change it to skip the first render by introducing a local ref flag
(e.g., isInitialMount) and only call pagination.onSetPage when
isInitialMount.current is false (set it to true on first render then to false
afterwards), so filter changes after mount still reset to page 1 but deep-linked
?page=N is preserved on initial load; update the useEffect that references
filters and pagination.onSetPage to use this initial-mount guard.
In `@web/src/components/table/useTableFilters.ts`:
- Around line 14-45: The local filters state in useTableFilters is only
initialized once and never updated when searchParams or initialFilters change,
causing it to get out of sync; add a useEffect inside useTableFilters that
listens to changes in searchParams and initialFilters and re-computes/sets
filters using the same logic used in the initial useState initializer
(respecting Array.isArray(initialFilters[key]), using searchParams.get/getAll,
treating empty arrays as null, and falling back to initialFilters[key]) so that
setFilters is updated whenever the URL or initialFilters shape changes.
In `@web/src/components/targets-page.tsx`:
- Around line 427-430: The effect that calls pagination.onSetPage(undefined, 1)
runs on mount and resets deep-linked pages; modify the useEffect that currently
depends on filters so it skips its first invocation: add a mounted/ref guard
(e.g., mountedRef or prevFiltersRef) and in the effect return early when the
component is mounting (or when previous filters are undefined) so
pagination.onSetPage is only called on subsequent filter changes; update the
useEffect surrounding pagination.onSetPage and references to filters accordingly
(useEffect, pagination.onSetPage, filters).
---
Nitpick comments:
In `@web/src/components/console/public/components/autocomplete.tsx`:
- Around line 108-119: The effect that builds suggestions inside useEffect (in
the Autocomplete component) only updates suggestions when the if condition
passes, leaving stale suggestions otherwise; update the cleanup branch so when
!(textValue && visible && showSuggestions) you call setSuggestions([]) to clear
stale results (reference useEffect, setSuggestions, textValue, visible,
showSuggestions, labelParser and fuzzyCaseInsensitive) so external clears of
textValue immediately remove suggestions.
In `@web/src/components/utils.ts`:
- Around line 139-157: The comparator severitySort is recreating the rank Map on
every comparison; hoist that map to module scope (e.g., const SEVERITY_RANK =
new Map<string, number>(...)) and have severitySort reference that shared map
instead of constructing it inside the function; update any references to rank
(rankA, rankB) to use SEVERITY_RANK.get(...) so the comparator reuses the same
allocation and avoids per-call overhead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1e6b2695-c1bc-499e-82e3-f6301e5ca34a
📒 Files selected for processing (10)
Dockerfile.devDockerfile.dev-mcpweb/src/components/alerting/AlertsPage.tsxweb/src/components/alerting/SilencesPage.tsxweb/src/components/alerting/filter-rules.tsweb/src/components/console/console-shared/hooks/useDocumentListener.tsweb/src/components/console/public/components/autocomplete.tsxweb/src/components/table/useTableFilters.tsweb/src/components/targets-page.tsxweb/src/components/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/console/console-shared/hooks/useDocumentListener.ts
- web/src/components/alerting/AlertsPage.tsx
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
Contributor
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, PeterYurkovich The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
Contributor
|
@PeterYurkovich: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores