OU-1040: feat(incidents): add support absolute start dates in Incidents component#892
Open
rioloc wants to merge 2 commits intoopenshift:mainfrom
Open
OU-1040: feat(incidents): add support absolute start dates in Incidents component#892rioloc wants to merge 2 commits intoopenshift:mainfrom
rioloc wants to merge 2 commits intoopenshift:mainfrom
Conversation
rioloc
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rioloc: This pull request references OU-1040 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 bug 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rioloc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
WalkthroughAlerts and incidents processing now deduplicate and split time series on gaps and severity changes, compute and carry a per-entity firstTimestamp, apply N‑day window clipping, adjust padding trimming, and update charts/tables to group by identity/group_id, share x positions across segments, and use ISO startDate/firstTimestamp for displays. Changes
Sequence DiagramsequenceDiagram
participant Prom as Prometheus
participant Proc as Processor
participant Utils as Utils
participant Chart as Charts
participant Table as UI
Prom->>Proc: return raw Prometheus results
activate Proc
Proc->>Proc: merge/deduplicate by identity / group_id
Proc->>Proc: sort values, split on gaps (> PROMETHEUS_QUERY_INTERVAL_SECONDS)
Proc->>Proc: split on severity changes (incidents)
Proc->>Utils: round timestamps (roundTimestampToFiveMinutes) & compute firstTimestamp
Proc->>Proc: clip to N‑day window (daysSpanMs) and insert padding
Proc-->>Chart: grouped entries with firstTimestamp and startDate
Proc-->>Table: processed entries with firstTimestamp
deactivate Proc
activate Chart
Chart->>Chart: group rows by identity / group_id
Chart->>Utils: remove trailing padding from non-final segments
Chart->>Chart: generate bars using datum.startDate
Chart-->>Table: visualized data (tooltips use ISO startDate)
deactivate Chart
activate Table
Table->>Table: sort/display using firstTimestamp and ISO timestamps
deactivate Table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx (1)
112-117:⚠️ Potential issue | 🟡 MinorPotential runtime error if
createIncidentsChartBarsreturns an empty array.The sort on line 114 accesses
a[0].xandb[0].x, which would throw if any group produces an empty bar array. WhilecreateIncidentsChartBars(from context snippet 2) only returns empty for empty input, andadjustedGroupsshould not contain empty groups, adding a defensive filter would prevent potential edge case failures.Suggested fix
// Create chart bars per group and sort by original x values - const chartBars = adjustedGroups.map((group) => createIncidentsChartBars(group, dateValues)); + const chartBars = adjustedGroups + .map((group) => createIncidentsChartBars(group, dateValues)) + .filter((bars) => bars.length > 0); chartBars.sort((a, b) => a[0].x - b[0].x);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx` around lines 112 - 117, The sort currently assumes every element from adjustedGroups.map(createIncidentsChartBars) is non-empty and accessing a[0].x can throw; filter out empty bar arrays before sorting and remapping to reassign x values. Concretely, after creating chartBars (from createIncidentsChartBars and adjustedGroups), apply a defensive filter like chartBars = chartBars.filter(bars => bars.length > 0), then perform the existing sort and the final map that reassigns consecutive x values; reference chartBars, createIncidentsChartBars, and adjustedGroups to locate where to add the filter.web/src/components/Incidents/IncidentsTable.tsx (1)
94-99:⚠️ Potential issue | 🟡 MinorEdge case:
getMinStartDatereturns 0 when no expanded data, leading to epoch timestamp display.When
alertsExpandedRowDatais empty or missing, the function returns0, which would display as January 1, 1970 in the Timestamp component. Consider returningInfinityor handling this case in the rendering logic to show a placeholder instead.Suggested fix
const getMinStartDate = (alert: GroupedAlert): number => { if (!alert.alertsExpandedRowData || alert.alertsExpandedRowData.length === 0) { - return 0; + return Infinity; // Or handle in render with a fallback display } return Math.min(...alert.alertsExpandedRowData.map((alertData) => alertData.firstTimestamp)); };Then in the render (lines 182-186), add a check:
<Td dataLabel={columnNames.startDate}> {getMinStartDate(alert) === Infinity ? ( '---' ) : ( <Timestamp timestamp={new Date(getMinStartDate(alert) * 1000).toISOString()} /> )} </Td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/IncidentsTable.tsx` around lines 94 - 99, getMinStartDate currently returns 0 when alertsExpandedRowData is missing/empty which yields an epoch timestamp; change getMinStartDate (used by the table row render) to return Infinity for the "no data" case and update the render code that uses getMinStartDate(alert) (the <Td> that wraps the Timestamp component) to check for Infinity and render a placeholder like '---' instead of creating a Timestamp; keep the existing timestamp conversion logic (new Date(getMinStartDate(alert) * 1000).toISOString()) for the valid numeric case so existing Timestamp behavior is preserved.
🧹 Nitpick comments (6)
web/src/components/Incidents/IncidentsDetailsRowTable.tsx (1)
27-31: Consider extracting the timestamp fallback logic to reduce duplication.The same fallback pattern
firstTimestamp > 0 ? firstTimestamp : alertsStartFiringappears in both the sorting logic and the rendering logic. Consider extracting this to a helper function for maintainability.Suggested refactor
+const getEffectiveStartTime = (alert: IncidentsDetailsAlert): number => + alert.firstTimestamp > 0 ? alert.firstTimestamp : alert.alertsStartFiring; + const sortedAndMappedAlerts = useMemo(() => { if (alerts && alerts.length > 0) { return [...alerts] .sort((a: IncidentsDetailsAlert, b: IncidentsDetailsAlert) => { - const aStart = a.firstTimestamp > 0 ? a.firstTimestamp : a.alertsStartFiring; - const bStart = b.firstTimestamp > 0 ? b.firstTimestamp : b.alertsStartFiring; + const aStart = getEffectiveStartTime(a); + const bStart = getEffectiveStartTime(b); return aStart - bStart; }) .map((alertDetails: IncidentsDetailsAlert, rowIndex) => { // ... <Td dataLabel="expanded-details-firingstart"> <Timestamp - timestamp={new Date( - (alertDetails.firstTimestamp > 0 - ? alertDetails.firstTimestamp - : alertDetails.alertsStartFiring) * 1000, - ).toISOString()} + timestamp={new Date(getEffectiveStartTime(alertDetails) * 1000).toISOString()} /> </Td>Also applies to: 49-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx` around lines 27 - 31, Extract the duplicate timestamp fallback logic into a small helper like getEffectiveStart(alert: IncidentsDetailsAlert): number and use it in both the .sort callback and the rendering code in IncidentsDetailsRowTable (replace occurrences of `firstTimestamp > 0 ? firstTimestamp : alertsStartFiring`); update the sort comparator to call getEffectiveStart(a) and getEffectiveStart(b) and replace any other direct fallbacks in render (e.g., where the effective start is displayed) to call the same helper so the logic is centralized and maintainable.web/src/components/Incidents/IncidentsPage.tsx (1)
241-242: Consider extracting the 15-day fetch window to a named constant.The
15 * DAY_MSvalue is duplicated for both alerts and incidents fetching. Extract to a constant to make the intent clearer and ensure consistency.Suggested fix
+// Always fetch 15 days of data so firstTimestamp is computed from full history +const FETCH_WINDOW_MS = 15 * DAY_MS; // In alerts useEffect: - const fetchTimeRanges = getIncidentsTimeRanges(15 * DAY_MS, currentTime); + const fetchTimeRanges = getIncidentsTimeRanges(FETCH_WINDOW_MS, currentTime); // In incidents useEffect: - const fetchTimeRanges = getIncidentsTimeRanges(15 * DAY_MS, currentTime); + const fetchTimeRanges = getIncidentsTimeRanges(FETCH_WINDOW_MS, currentTime);Also applies to: 309-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/IncidentsPage.tsx` around lines 241 - 242, Extract the repeated literal 15 * DAY_MS into a named constant (e.g., const INCIDENTS_FETCH_WINDOW = 15 * DAY_MS) and use that constant wherever getIncidentsTimeRanges(...) is called (references: fetchTimeRanges and the other call around lines 309-310) so both alerts and incidents use the same named value; update the two call sites to pass INCIDENTS_FETCH_WINDOW instead of 15 * DAY_MS to ensure consistency and clarify intent.web/src/components/Incidents/model.ts (1)
18-18:severity: anyweakens type safety.The
severityfield usesanytype. Consider using the existingSeveritytype ('critical' | 'warning' | 'info') or a more specific type for better type safety.Suggested fix
- severity: any; + severity: Severity;Or if it needs to handle additional values:
- severity: any; + severity: Severity | string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/model.ts` at line 18, The field declaration "severity: any" weakens type safety—replace it with the concrete Severity union type by changing the property to use Severity (or Severity | string if you must accept extra values) and import or reference the existing Severity type; update the Incidents model's interface/class where "severity" is declared and any places constructing or assigning to this field (e.g., constructors, factories, deserializers) to satisfy the new type.web/src/components/Incidents/processAlerts.ts (1)
303-307: The assumption thatalert.values[0][0]is the earliest timestamp is valid, but consider clarifying it.The code correctly relies on
deduplicateAlertssorting all values by timestamp (line 80–82). However, this assumption is implicit. Add a comment explaining that values are guaranteed to be sorted bydeduplicateAlerts, or consider usingMath.min()for explicit clarity:// Values are sorted by deduplicateAlerts; use Math.min() for clarity if preferred: const alertFirstTimestamp = roundTimestampToFiveMinutes( Math.min(...alert.values.map(v => v[0])) - PROMETHEUS_QUERY_INTERVAL_SECONDS, );This makes the intention clearer and is more defensive against future changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/processAlerts.ts` around lines 303 - 307, The code assumes alert.values[0][0] is the earliest timestamp because deduplicateAlerts sorts values; make that explicit or defensive: add a brief comment referencing deduplicateAlerts to state values are sorted, or replace the access with an explicit min over alert.values timestamps (e.g., using Math.min on alert.values.map(v => v[0])) before calling roundTimestampToFiveMinutes and subtracting PROMETHEUS_QUERY_INTERVAL_SECONDS; update the alertFirstTimestamp calculation (and keep roundTimestampToFiveMinutes and PROMETHEUS_QUERY_INTERVAL_SECONDS) accordingly.web/src/components/Incidents/utils.ts (1)
381-421: Potential issue:nodataintervals don't havefirstTimestampset.In
consolidateAndMergeAlertIntervals, when creatingnodataintervals (line 403), nofirstTimestampis included in the tuple:intervals.push([previousTimestamp + 1, sorted[i].timestamp - 1, 'nodata']);This creates inconsistency in the
AlertsIntervalsArraytype wherenodataintervals have only 3 elements whiledataintervals have 4. The consumer at line 442 handles this with a fallback (?? firstAlert.firstTimestamp), but this could be error-prone.Consider explicitly setting the 4th element to
undefinedfor clarity:-intervals.push([previousTimestamp + 1, sorted[i].timestamp - 1, 'nodata']); +intervals.push([previousTimestamp + 1, sorted[i].timestamp - 1, 'nodata', undefined]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/utils.ts` around lines 381 - 421, In consolidateAndMergeAlertIntervals, the 'nodata' interval pushed into intervals lacks the fourth element (firstTimestamp), producing an inconsistent AlertsIntervalsArray shape; update the nodata push (the one that currently does intervals.push([previousTimestamp + 1, sorted[i].timestamp - 1, 'nodata'])) to include the fourth slot (e.g., undefined or null) so every tuple has four elements ([start, end, 'nodata', undefined]) to match the data intervals and avoid relying on downstream fallbacks like firstAlert.firstTimestamp.web/src/components/Incidents/processIncidents.ts (1)
176-191: Add test case or clarify intent: multiple Prometheus results with same severity valueThe logic at lines 188-190 maps each severity value to a single metric per
group_id. If multiple results contribute the same severity value with differentsrc_alertnameorcomponent, only the last processed result's metric is retained.This appears intentional (implicit "latest wins" design based on the comment at 247-248), but the expected behavior should be explicitly tested or documented in a code comment to prevent future misunderstandings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/processIncidents.ts` around lines 176 - 191, The code in processIncidents.ts builds severityMetrics (Map severityMetrics) by overwriting any existing mapping for a severity value when multiple prometheusResults for the same groupId share the same severity (sv), which is implicit "latest wins" behavior; make this explicit by either adding a unit test that creates multiple prometheusResults with the same severity but different metric fields (e.g., different metric.src_alertname or metric.component) asserting the intended selection behavior, or add a clarifying code comment next to the severityMetrics population loop (referencing severityMetrics, prometheusResults, groupId, sv, and result.metric) that documents that later results override earlier ones and why that is acceptable.
🤖 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/Incidents/IncidentsPage.tsx`:
- Around line 274-278: The current conditional that dispatches
setAlertsAreLoading uses alerts.length to set loading true when alerts is empty,
which causes an empty successful result to stay in a loading state; update the
logic in IncidentsPage (the block using alerts and
dispatch(setAlertsAreLoading)) so that after the fetch completes you always set
alertsAreLoading: false (e.g., dispatch setAlertsAreLoading({ alertsAreLoading:
false }) regardless of alerts.length) or explicitly set it false when
alerts.length === 0; alternatively, if the intended UX is to keep loading for
empty filtered results, add a comment documenting that decision next to the
setAlertsAreLoading call and keep the current logic.
In `@web/src/components/Incidents/utils.spec.ts`:
- Around line 10-19: The test for getCurrentTime is flaky because it calls
getCurrentTime() and Date.now() separately; mock Date.now() for the test so both
calls use the same timestamp. Update the spec to spy/mock Date.now() (so
getCurrentTime() and your expected calculation use the same fixed value),
compute expected using that mocked timestamp and restore the mock after the test
to avoid side effects on other tests.
---
Outside diff comments:
In `@web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx`:
- Around line 112-117: The sort currently assumes every element from
adjustedGroups.map(createIncidentsChartBars) is non-empty and accessing a[0].x
can throw; filter out empty bar arrays before sorting and remapping to reassign
x values. Concretely, after creating chartBars (from createIncidentsChartBars
and adjustedGroups), apply a defensive filter like chartBars =
chartBars.filter(bars => bars.length > 0), then perform the existing sort and
the final map that reassigns consecutive x values; reference chartBars,
createIncidentsChartBars, and adjustedGroups to locate where to add the filter.
In `@web/src/components/Incidents/IncidentsTable.tsx`:
- Around line 94-99: getMinStartDate currently returns 0 when
alertsExpandedRowData is missing/empty which yields an epoch timestamp; change
getMinStartDate (used by the table row render) to return Infinity for the "no
data" case and update the render code that uses getMinStartDate(alert) (the <Td>
that wraps the Timestamp component) to check for Infinity and render a
placeholder like '---' instead of creating a Timestamp; keep the existing
timestamp conversion logic (new Date(getMinStartDate(alert) *
1000).toISOString()) for the valid numeric case so existing Timestamp behavior
is preserved.
---
Nitpick comments:
In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx`:
- Around line 27-31: Extract the duplicate timestamp fallback logic into a small
helper like getEffectiveStart(alert: IncidentsDetailsAlert): number and use it
in both the .sort callback and the rendering code in IncidentsDetailsRowTable
(replace occurrences of `firstTimestamp > 0 ? firstTimestamp :
alertsStartFiring`); update the sort comparator to call getEffectiveStart(a) and
getEffectiveStart(b) and replace any other direct fallbacks in render (e.g.,
where the effective start is displayed) to call the same helper so the logic is
centralized and maintainable.
In `@web/src/components/Incidents/IncidentsPage.tsx`:
- Around line 241-242: Extract the repeated literal 15 * DAY_MS into a named
constant (e.g., const INCIDENTS_FETCH_WINDOW = 15 * DAY_MS) and use that
constant wherever getIncidentsTimeRanges(...) is called (references:
fetchTimeRanges and the other call around lines 309-310) so both alerts and
incidents use the same named value; update the two call sites to pass
INCIDENTS_FETCH_WINDOW instead of 15 * DAY_MS to ensure consistency and clarify
intent.
In `@web/src/components/Incidents/model.ts`:
- Line 18: The field declaration "severity: any" weakens type safety—replace it
with the concrete Severity union type by changing the property to use Severity
(or Severity | string if you must accept extra values) and import or reference
the existing Severity type; update the Incidents model's interface/class where
"severity" is declared and any places constructing or assigning to this field
(e.g., constructors, factories, deserializers) to satisfy the new type.
In `@web/src/components/Incidents/processAlerts.ts`:
- Around line 303-307: The code assumes alert.values[0][0] is the earliest
timestamp because deduplicateAlerts sorts values; make that explicit or
defensive: add a brief comment referencing deduplicateAlerts to state values are
sorted, or replace the access with an explicit min over alert.values timestamps
(e.g., using Math.min on alert.values.map(v => v[0])) before calling
roundTimestampToFiveMinutes and subtracting PROMETHEUS_QUERY_INTERVAL_SECONDS;
update the alertFirstTimestamp calculation (and keep roundTimestampToFiveMinutes
and PROMETHEUS_QUERY_INTERVAL_SECONDS) accordingly.
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 176-191: The code in processIncidents.ts builds severityMetrics
(Map severityMetrics) by overwriting any existing mapping for a severity value
when multiple prometheusResults for the same groupId share the same severity
(sv), which is implicit "latest wins" behavior; make this explicit by either
adding a unit test that creates multiple prometheusResults with the same
severity but different metric fields (e.g., different metric.src_alertname or
metric.component) asserting the intended selection behavior, or add a clarifying
code comment next to the severityMetrics population loop (referencing
severityMetrics, prometheusResults, groupId, sv, and result.metric) that
documents that later results override earlier ones and why that is acceptable.
In `@web/src/components/Incidents/utils.ts`:
- Around line 381-421: In consolidateAndMergeAlertIntervals, the 'nodata'
interval pushed into intervals lacks the fourth element (firstTimestamp),
producing an inconsistent AlertsIntervalsArray shape; update the nodata push
(the one that currently does intervals.push([previousTimestamp + 1,
sorted[i].timestamp - 1, 'nodata'])) to include the fourth slot (e.g., undefined
or null) so every tuple has four elements ([start, end, 'nodata', undefined]) to
match the data intervals and avoid relying on downstream fallbacks like
firstAlert.firstTimestamp.
🪄 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: Pro Plus
Run ID: 06c6f2ad-ff9d-4386-be25-fd20077e5b29
📒 Files selected for processing (12)
web/src/components/Incidents/AlertsChart/AlertsChart.tsxweb/src/components/Incidents/IncidentsChart/IncidentsChart.tsxweb/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsPage.tsxweb/src/components/Incidents/IncidentsTable.tsxweb/src/components/Incidents/model.tsweb/src/components/Incidents/processAlerts.spec.tsweb/src/components/Incidents/processAlerts.tsweb/src/components/Incidents/processIncidents.spec.tsweb/src/components/Incidents/processIncidents.tsweb/src/components/Incidents/utils.spec.tsweb/src/components/Incidents/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/src/components/Incidents/processIncidents.spec.ts (1)
1245-1245: Consider updating the return type instead of usingas any[]cast.The
as any[]cast here and on line 1309 bypasses type checking. IfprocessIncidentsForAlertsis expected to return objects with these properties, consider updating its return type annotation to properly reflect the actual return shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/processIncidents.spec.ts` at line 1245, The test is using an unsafe as any[] cast for the result of processIncidentsForAlerts; update processIncidentsForAlerts's return type to a precise typed array (or export a dedicated interface/type for the alert object shape) so the test can consume a correctly typed value instead of casting; change the function signature (or its exported type alias) to reflect the actual properties the tests expect and update the test variable declaration to use that type (e.g., AlertResult[] or the named interface) to restore compile-time checking.
🤖 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/Incidents/IncidentsChart/IncidentsChart.tsx`:
- Around line 113-114: createIncidentsChartBars can return an empty array which
makes chartBars.sort((a, b) => a[0].x - b[0].x) throw when accessing a[0]; fix
by filtering out empty arrays after mapping (e.g., chartBars =
adjustedGroups.map(g => createIncidentsChartBars(g, dateValues)).filter(b =>
b.length > 0)) and/or make the sort comparator defensive (check b.length and
a[0] exists) so only non-empty bar arrays are sorted; update references to
chartBars and createIncidentsChartBars accordingly.
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 64-66: The access to sortedValues[0][0] when computing
firstTimestamp can throw if sortedValues is empty; in processIncidents.ts update
the logic around the firstTimestamp calculation (the call to
roundTimestampToFiveMinutes and use of PROMETHEUS_QUERY_INTERVAL_SECONDS) to
defensively check that sortedValues.length > 0 before indexing, and if empty
either compute firstTimestamp from a safe fallback (e.g., the window start or
Date.now() minus PROMETHEUS_QUERY_INTERVAL_SECONDS) or return/skip processing
the incident early; ensure the guard is added adjacent to where firstTimestamp
is defined so the code never dereferences sortedValues[0].
---
Nitpick comments:
In `@web/src/components/Incidents/processIncidents.spec.ts`:
- Line 1245: The test is using an unsafe as any[] cast for the result of
processIncidentsForAlerts; update processIncidentsForAlerts's return type to a
precise typed array (or export a dedicated interface/type for the alert object
shape) so the test can consume a correctly typed value instead of casting;
change the function signature (or its exported type alias) to reflect the actual
properties the tests expect and update the test variable declaration to use that
type (e.g., AlertResult[] or the named interface) to restore compile-time
checking.
🪄 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: Pro Plus
Run ID: 1388cf3e-6db6-465c-8424-31c9bdb9f11c
📒 Files selected for processing (12)
web/src/components/Incidents/AlertsChart/AlertsChart.tsxweb/src/components/Incidents/IncidentsChart/IncidentsChart.tsxweb/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsPage.tsxweb/src/components/Incidents/IncidentsTable.tsxweb/src/components/Incidents/model.tsweb/src/components/Incidents/processAlerts.spec.tsweb/src/components/Incidents/processAlerts.tsweb/src/components/Incidents/processIncidents.spec.tsweb/src/components/Incidents/processIncidents.tsweb/src/components/Incidents/utils.spec.tsweb/src/components/Incidents/utils.ts
✅ Files skipped from review due to trivial changes (1)
- web/src/components/Incidents/IncidentsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- web/src/components/Incidents/IncidentsTable.tsx
- web/src/components/Incidents/model.ts
- web/src/components/Incidents/AlertsChart/AlertsChart.tsx
- web/src/components/Incidents/processAlerts.ts
- web/src/components/Incidents/IncidentsDetailsRowTable.tsx
…ert gap detection Decouple the fetch window (always 15 days) from the display window (user-selected N days) so that firstTimestamp reflects the true incident/alert start time regardless of the currently visible time range. Split incidents when severity changes between consecutive timestamps and split alerts when gaps exceed the 300s Prometheus scrape interval. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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/Incidents/processIncidents.ts`:
- Around line 180-194: severityMetrics currently stores only one
PrometheusLabels per (group_id, severity) which causes earlier segments to reuse
the wrong source labels when a severity repeats; change severityMetrics from
Map<string, Map<string, PrometheusLabels>> to Map<string, Map<string,
PrometheusLabels[]>> (or equivalent per-segment storage) in the
prometheusResults loop (where groupId, severityValues and result.metric are
used) so you push result.metric onto an array for each severity instead of
overwriting, and then when emitting segments later (the other block noted around
the second occurrence) consume/pop metrics from that array in FIFO order so each
emitted segment gets the exact PrometheusLabels instance that corresponds to its
occurrence.
In `@web/src/components/Incidents/utils.ts`:
- Around line 381-415: consolidateAndMergeAlertIntervals is merging intervals
across the ±300s padded boundary because you flatten padded timestamps
(taggedValues/sorted) and compute timeDifference > 5 on those padded points;
instead detect gaps using unpadded timestamps or strip boundary padding before
merging: either use each alert's original unpadded value timestamps (from
alert.values without the ±300s padding) or mark/trim the first/last padded
entries per alert before building taggedValues so the timeDifference calculation
(previousTimestamp vs sorted[i].timestamp) reflects real gaps and preserves each
alert's firstTimestamp when emitting intervals (intervals push and
currentFirstTimestamp logic).
🪄 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: Pro Plus
Run ID: 1bbe8681-9f4a-4331-a6eb-8310efd904b2
📒 Files selected for processing (12)
web/src/components/Incidents/AlertsChart/AlertsChart.tsxweb/src/components/Incidents/IncidentsChart/IncidentsChart.tsxweb/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsPage.tsxweb/src/components/Incidents/IncidentsTable.tsxweb/src/components/Incidents/model.tsweb/src/components/Incidents/processAlerts.spec.tsweb/src/components/Incidents/processAlerts.tsweb/src/components/Incidents/processIncidents.spec.tsweb/src/components/Incidents/processIncidents.tsweb/src/components/Incidents/utils.spec.tsweb/src/components/Incidents/utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/Incidents/IncidentsTable.tsx
- web/src/components/Incidents/IncidentsDetailsRowTable.tsx
- web/src/components/Incidents/AlertsChart/AlertsChart.tsx
- web/src/components/Incidents/utils.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/src/components/Incidents/processIncidents.spec.ts (1)
365-399: Consider adding existence assertions before property access.Using
result.find()and immediately accessing properties likeincidentA.firstTimestampcould produce unclearTypeError: Cannot read property of undefinederrors if the test fails unexpectedly. Adding explicit assertions improves test failure diagnostics.💡 Optional improvement for clearer test failures
const incidentA = result.find((i) => i.group_id === 'incidentA'); const incidentB = result.find((i) => i.group_id === 'incidentB'); +expect(incidentA).toBeDefined(); +expect(incidentB).toBeDefined(); expect(incidentA.firstTimestamp).toBe(startA - 300); expect(incidentB.firstTimestamp).toBe(startB - 300);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/processIncidents.spec.ts` around lines 365 - 399, The test accesses incidentA.firstTimestamp and incidentB.firstTimestamp after using result.find(); add explicit existence assertions so failures are clearer: after computing result with convertToIncidents(...) assert result length and then assert incidentA and incidentB are defined (e.g., expect(incidentA).toBeDefined() and expect(incidentB).toBeDefined()) before accessing their firstTimestamp properties to avoid ambiguous TypeError failures.web/src/components/Incidents/utils.spec.ts (1)
631-771: Good test coverage for padding boundary gap detection.The tests cover key scenarios well. Consider adding coverage for the edge case where an alert has 3 values in the pattern
[pad_before, real1, real2](no after-padding), to verify the slicing behavior when after-padding is absent.💡 Optional: Add test for 3-value alert without after-padding
it('should handle alerts with 3 values (pad + 2 real points, no after-padding)', () => { // Simulates very recent alert where currentTime < lastPoint + 300 // Pattern: [pad_before, real1, real2] - no after-padding const alert = makeAlert({ values: [ [700, '2'], // padding before [1000, '2'], // real point 1 [1100, '2'], // real point 2 (continuous, no after-padding) ], firstTimestamp: 700, }); const bars = createAlertsChartBars([alert]); // Verify both real data points are represented // (This would catch the slice(1, -1) issue) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/utils.spec.ts` around lines 631 - 771, Add a unit test that covers the 3-value pattern [pad_before, real1, real2] to catch the slice(1, -1) bug: create a test (e.g., "should handle alerts with 3 values (pad + 2 real points, no after-padding)") that uses makeAlert with values [[700,'2'],[1000,'2'],[1100,'2']] and firstTimestamp 700, call createAlertsChartBars([alert]) and assert that both real points are preserved in the output (e.g., data bars include both real timestamps / no nodata inserted and the interval covers the real points) so the function createAlertsChartBars does not drop the second real point when after-padding is absent.
🤖 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/Incidents/utils.ts`:
- Around line 386-391: The slice logic in alerts.flatMap that builds
taggedValues (currently using values.length <= 2 ? values.slice(-1) :
values.slice(1, -1)) drops the last real data point when after-padding wasn't
added; update the extraction so it preserves all real data points regardless of
padding: use insertPaddingPointsForChart-aware selection (or detect padding
presence) when computing coreValues in the taggedValues mapping inside the
function building taggedValues, ensuring you keep the final real timestamp so
consolidateAndMergeAlertIntervals and createAlertsChartBars see complete
timestamps (refer to taggedValues, alerts.flatMap, insertPaddingPointsForChart
and the failing test in utils.spec.ts).
---
Nitpick comments:
In `@web/src/components/Incidents/processIncidents.spec.ts`:
- Around line 365-399: The test accesses incidentA.firstTimestamp and
incidentB.firstTimestamp after using result.find(); add explicit existence
assertions so failures are clearer: after computing result with
convertToIncidents(...) assert result length and then assert incidentA and
incidentB are defined (e.g., expect(incidentA).toBeDefined() and
expect(incidentB).toBeDefined()) before accessing their firstTimestamp
properties to avoid ambiguous TypeError failures.
In `@web/src/components/Incidents/utils.spec.ts`:
- Around line 631-771: Add a unit test that covers the 3-value pattern
[pad_before, real1, real2] to catch the slice(1, -1) bug: create a test (e.g.,
"should handle alerts with 3 values (pad + 2 real points, no after-padding)")
that uses makeAlert with values [[700,'2'],[1000,'2'],[1100,'2']] and
firstTimestamp 700, call createAlertsChartBars([alert]) and assert that both
real points are preserved in the output (e.g., data bars include both real
timestamps / no nodata inserted and the interval covers the real points) so the
function createAlertsChartBars does not drop the second real point when
after-padding is absent.
🪄 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: Pro Plus
Run ID: 4d8cc424-34e9-4f16-9867-8cb92bb1ab1c
📒 Files selected for processing (4)
web/src/components/Incidents/processIncidents.spec.tsweb/src/components/Incidents/processIncidents.tsweb/src/components/Incidents/utils.spec.tsweb/src/components/Incidents/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/Incidents/processIncidents.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/components/Incidents/processIncidents.spec.ts (1)
1410-1410: Consider defining a proper return type instead of casting toany[].The
as any[]cast is used to access properties likegroup_idandsrc_alertname. Consider adding a typed interface for the return value ofprocessIncidentsForAlertsto improve type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/processIncidents.spec.ts` at line 1410, Replace the unsafe cast "as any[]" by introducing a proper return type for processIncidentsForAlerts and using it in the spec: define a typed interface (e.g., IncidentAlertGroup or ProcessedIncident) that includes the fields you access (group_id, src_alertname, plus any other properties returned), update the processIncidentsForAlerts function signature to return that interface[] (or a named type), and change the test line to use that type instead of "any[]" so you can access group_id and src_alertname with full type safety.web/src/components/Incidents/utils.ts (1)
315-326: Add defensive check for emptyvaluesarray in sort comparator.The sort comparator at line 318 accesses
a.values[0][0]andb.values[0][0]without checking ifvaluesis non-empty. While incidents should have values in practice, this could throw if an unexpected empty array slips through.🛡️ Suggested defensive fix
export function removeTrailingPaddingFromSeveritySegments(group: Incident[]): Incident[] { if (group.length <= 1) return group; - const sorted = [...group].sort((a, b) => a.values[0][0] - b.values[0][0]); + const sorted = [...group].sort((a, b) => { + const aFirst = a.values[0]?.[0] ?? 0; + const bFirst = b.values[0]?.[0] ?? 0; + return aFirst - bFirst; + }); return sorted.map((incident, idx) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/utils.ts` around lines 315 - 326, The sort comparator in removeTrailingPaddingFromSeveritySegments reads a.values[0][0] and b.values[0][0] without guarding against empty values arrays; update the comparator used when creating sorted (the [...group].sort callback) to defensively handle missing/empty values by falling back to a safe default (e.g., Number.POSITIVE_INFINITY or 0) when a.values or b.values are empty or undefined, so the sort never throws and existing slice logic on incident.values remains valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/Incidents/processIncidents.spec.ts`:
- Line 1410: Replace the unsafe cast "as any[]" by introducing a proper return
type for processIncidentsForAlerts and using it in the spec: define a typed
interface (e.g., IncidentAlertGroup or ProcessedIncident) that includes the
fields you access (group_id, src_alertname, plus any other properties returned),
update the processIncidentsForAlerts function signature to return that
interface[] (or a named type), and change the test line to use that type instead
of "any[]" so you can access group_id and src_alertname with full type safety.
In `@web/src/components/Incidents/utils.ts`:
- Around line 315-326: The sort comparator in
removeTrailingPaddingFromSeveritySegments reads a.values[0][0] and
b.values[0][0] without guarding against empty values arrays; update the
comparator used when creating sorted (the [...group].sort callback) to
defensively handle missing/empty values by falling back to a safe default (e.g.,
Number.POSITIVE_INFINITY or 0) when a.values or b.values are empty or undefined,
so the sort never throws and existing slice logic on incident.values remains
valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f8f60da-f3cf-4f1a-ba1b-e75ed4d1b1d1
📒 Files selected for processing (4)
web/src/components/Incidents/processIncidents.spec.tsweb/src/components/Incidents/processIncidents.tsweb/src/components/Incidents/utils.spec.tsweb/src/components/Incidents/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/Incidents/utils.spec.ts
Contributor
|
@rioloc: 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Start dates in tooltips are relatives to the numer of selected days as time span.
For example, if today is 30 Jan 2026 and an incident started on 22 Jan 2026, the Start date will be displayed as:
29 Jan 2026 if "Last 1 Day" is selected
27 Jan 2026 if "Last 3 Days" is selected
23 Jan 2026 if "Last 7 Days" is selected
22 Jan 2026 if "Last 15 Days" is selected. (The only correct one)
Solution
The absolute start date of an incident/alert is always displayed, and it is not related to the number of selected days.
Before
main.webm
After
recording.25.webm
Fixes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests