Conversation
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 "4.22.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
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@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 "4.22.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. |
|
@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 "4.22.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. |
|
/test e2e-incidents |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-incidents |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@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 "4.22.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. |
- add incidentsTimestamp in redux store - add two extra calls to retrieve min_over_time and last_over_time for incidents - enrich incident with absolute datapoints timestamps - update IncidentTooltip labels with absolute Start and End times
…entsTable on page refresh
- Fix initial state type mismatch for incidentsTimestamps and alertsTimestamps
(was [] instead of { minOverTime: [], lastOverTime: [] })
- Add defensive check in matchTimestampMetricForIncident for undefined timestamps
- Refactor incidents useEffect to fetch timestamps and incidents in parallel,
then use fetched values directly instead of stale closure values
- Refactor alerts useEffect with same pattern and add guards for empty
incidentForAlertProcessing and timeRanges
- Add timeRanges and rules to alerts useEffect dependency array
- Remove unused alertsTimestamps selector
Assisted-By: Claude Opus 4.5
rioloc
force-pushed
the
feat/absolute-start-dates
branch
from
14440d2 to
b8f4ed7
Compare
|
/test e2e-incidents |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich, rioloc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/hold |
openshift-ci
bot
added
do-not-merge/hold
|
New changes are detected. LGTM label has been removed. |
|
/retest |
rioloc
force-pushed
the
feat/absolute-start-dates
branch
from
3f05003 to
8b40410
Compare
rioloc
force-pushed
the
feat/absolute-start-dates
branch
from
8b40410 to
a5864d2
Compare
|
/test okd-scos-images |
|
linter error, please take a look |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@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 "4.22.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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR adds time-windowed (15-day) processing, firstTimestamp propagation, gap-aware alert splitting, severity-based incident splitting, chart-bar generation over grouped items with startDate, new/updated utilities (DAY_MS, rounding/matching, padding removal), and multiple test updates to cover the behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as IncidentsPage
participant Fetch as Fetch(prometheus)
participant ProcA as processAlerts
participant ProcI as processIncidents
participant Chart as Chart Components
participant Table as Table Components
Page->>Page: compute daysSpanMs = 15 * DAY_MS
Page->>Fetch: request alerts (15-day window)
Page->>Fetch: request incidents (15-day window)
Fetch-->>Page: alertsResults, incidentsResults
Page->>ProcA: convertToAlerts(alertsResults, now, daysSpanMs)
Note over ProcA: group by identity<br/>dedupe + split on gaps (>5min)<br/>compute firstTimestamp, clamp to window
ProcA-->>Page: alerts[] (with firstTimestamp)
Page->>ProcI: convertToIncidents(incidentsResults, now, daysSpanMs)
Note over ProcI: group by group_id<br/>split by severity runs<br/>compute firstTimestamp, clamp/pad to window
ProcI-->>Page: incidents[] (with firstTimestamp)
Page->>Page: dispatch setAlertsData & setAlertsTableData<br/>dispatch setFilteredIncidentsData
Page->>Chart: pass grouped alerts/incidents
Chart->>Chart: group rows, remove trailing padding, create bars with startDate
Chart-->>Page: rendered charts
Page->>Table: pass incidents with firstTimestamp
Table->>Table: sort by firstTimestamp, render composite timestamps
Table-->>Page: rendered tables
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/components/Incidents/IncidentsTable.tsx (1)
94-99:⚠️ Potential issue | 🟠 MajorMissing fallback when
firstTimestampis0.When no timestamp match is found,
firstTimestampdefaults to0, causinggetMinStartDateto return0— which renders as Jan 1, 1970 in the<Timestamp>component (Line 183) and breaks sort order.IncidentsDetailsRowTable.tsx(Lines 28-29) guards against this withfirstTimestamp > 0 ? firstTimestamp : alertsStartFiring, but this file lacks the same fallback.Proposed fix: add fallback to alertsStartFiring
return Math.min( - ...alert.alertsExpandedRowData.map((alertData) => alertData.firstTimestamp), + ...alert.alertsExpandedRowData.map((alertData) => + alertData.firstTimestamp > 0 ? alertData.firstTimestamp : alertData.alertsStartFiring, + ), );web/src/components/Incidents/IncidentsPage.tsx (1)
400-403:⚠️ Potential issue | 🟡 MinorInconsistent error logging:
console.loghere vsconsole.errorin the alerts fetch (line 313).The alerts-fetching
.catchon line 313 usesconsole.errorbut the incidents-fetching.catchhere still usesconsole.log.Proposed fix
.catch((err) => { // eslint-disable-next-line no-console - console.log(err); + console.error(err); });web/src/components/Incidents/utils.ts (1)
406-450:⚠️ Potential issue | 🟠 MajorUpdate
AlertsChartBartype definition to includestartDatefield, or remove it from the function if unused.The
createAlertsChartBarsfunction (line 430) addsstartDate: new Date(intervalFirstTimestamp * 1000)to each returned object, but theAlertsChartBartype definition inmodel.ts(lines 137-150) does not include this field. The function has an explicit return type annotation ofAlertsChartBar[], creating a type mismatch. Additionally,startDateis not accessed anywhere in the codebase, suggesting it may be dead code. Either addstartDate: Dateto theAlertsChartBartype or remove it from the function.
🤖 Fix all issues with AI agents
In `@web/src/components/Incidents/api.ts`:
- Around line 180-187: The code should call consoleFetchJSON(url) directly
(remove the redundant Promise.resolve) and defensively access the result to
avoid a TypeError: capture the response from consoleFetchJSON and use
response.data?.result || [] when constructing the returned PrometheusResponse
(keep the same shape with resultType: 'matrix'), ensuring you still return the
object as PrometheusResponse; reference the variables/functions response,
consoleFetchJSON and the PrometheusResponse return shape and mirror the optional
chaining used in fetchDataForIncidentsAndAlerts.
In `@web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx`:
- Around line 86-99: enrichedIncidentsData currently sets firstTimestamp to 0
when matchTimestampMetricForIncident returns undefined, causing "Jan 1, 1970" in
tooltips; change the fallback in the useMemo mapping so firstTimestamp uses the
incident's earliest value timestamp (e.g., the first element in incident.values)
when matchedMinTimestamp is undefined, parsing that timestamp with parseInt
(instead of '0'), and keep the rest of the mapping unchanged; this touches
enrichedIncidentsData, matchTimestampMetricForIncident usage, and will prevent
createIncidentsChartBars from receiving epoch-zero values for firstTimestamp.
In `@web/src/components/Incidents/processAlerts.spec.ts`:
- Around line 570-582: The test fixtures for AlertsTimestamps are using key
"value" (singular) but splitAbsoluteTimestampsByGap expects entries shaped like
production (entries have "values" as an array of [timestamp,string] pairs);
update the test objects in alertsTimestamps.minOverTime (and the three other
timestamp-matching tests) to use values: [[matchedMinTimestamp,
matchedMinTimestamp.toString()]] (i.e., change "value: [...]" to "values:
[[...]]") so they match the shape consumed by splitAbsoluteTimestampsByGap and
the AlertsTimestamps type.
In `@web/src/components/Incidents/processAlerts.ts`:
- Line 344: The code assumes incidents[0] exists when computing
incidentFirstTimestamp in processAlerts (after calling mergeIncidentsByKey),
which can throw if the merged incidents array is empty; update the public
function in processAlerts.ts to defensively handle an empty array by checking
incidents.length (or the result of mergeIncidentsByKey) before accessing
incidents[0].firstTimestamp and either return early/skip processing, or set
incidentFirstTimestamp to a safe default (e.g., undefined) and propagate that
state so callers won’t dereference a missing element; reference the variable
incidentFirstTimestamp and the mergeIncidentsByKey call to find where to add the
guard.
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 194-225: processIncidentsForAlerts currently falls back to '0'
when matchTimestampMetricForIncident returns no match, producing epoch
timestamps; change the fallback for firstTimestamp to use the incident's
earliest value timestamp (from incident.values) before finally defaulting to
'0'. Specifically, when computing firstTimestamp in processIncidentsForAlerts,
pick matchedMinTimestamp?.value?.[1] if present, otherwise use the timestamp
from incident.values (the earliest entry), and then parseInt that result to set
firstTimestamp; update any parseInt usage accordingly to avoid defaulting
straight to epoch.
In `@web/src/components/Incidents/utils.spec.ts`:
- Around line 156-179: The test setup and assertion for
matchTimestampMetricForIncident are placed directly in the describe block,
causing them to run during collection and orphaning subsequent it blocks; wrap
the variable declarations (incident, timestamps, result) and the
expect(result).toBe(...) assertion inside a new it('returns matching timestamp
metric', () => { ... }) block, and move the describe closing token (the final
"});") to after the existing it blocks so all it tests (including those on lines
181–270) are nested inside describe('matchTimestampMetricForIncident'); keep
references to matchTimestampMetricForIncident, incident, timestamps, and result
when relocating code.
In `@web/src/components/Incidents/utils.ts`:
- Around line 325-326: The code constructs startDate from
incident.firstTimestamp using new Date(incident.firstTimestamp * 1000) which
yields 1970-01-01 for a 0 timestamp; change the construction so that when
incident.firstTimestamp is missing/0 you set startDate to null (or undefined)
instead of a Date(0), update the startDate type to Date | null in the relevant
interface/shape (the startDate property in the object/type), and adjust
consumers (e.g., IncidentsChart.tsx and any rendering logic) to handle null
startDate appropriately; specifically locate the new Date(...) call and replace
it with a guarded expression that only creates a Date when
incident.firstTimestamp > 0 and otherwise assigns null, and update the type
annotations for startDate to match.
🧹 Nitpick comments (9)
web/src/components/Incidents/model.ts (1)
26-34: Consider replacingArray<any>with a proper type for Prometheus instant query results.Both
IncidentsTimestampsandAlertsTimestampsuseArray<any>forminOverTimeandlastOverTime. The data shape from Prometheus instant queries is well-known (an object withmetric: Record<string, string>andvalue: [number, string]). Typing these properly would catch misuse at compile time and improve discoverability.Additionally, the two types are structurally identical — a single shared type (or type alias) would reduce duplication.
Proposed typing improvement
+export type TimestampResult = { + metric: Record<string, string>; + value: [number, string]; +}; + -export type IncidentsTimestamps = { - minOverTime: Array<any>; - lastOverTime: Array<any>; -}; - -export type AlertsTimestamps = { - minOverTime: Array<any>; - lastOverTime: Array<any>; -}; +export type TimestampData = { + minOverTime: Array<TimestampResult>; + lastOverTime: Array<TimestampResult>; +}; + +export type IncidentsTimestamps = TimestampData; +export type AlertsTimestamps = TimestampData;web/src/components/Incidents/api.ts (1)
157-160:fetchparameter is accepted but unused.The
fetchparameter is never referenced in the function body —consoleFetchJSONis called directly on Line 180. This matches the pattern infetchDataForIncidentsAndAlerts(which also accepts but ignoresfetch), but it's misleading. Consider removing it or actually using it for consistency and testability.web/src/components/Incidents/processIncidents.spec.ts (1)
1037-1065: RedundantemptyTimestampsre-declarations in edge case tests.The
emptyTimestampsfixture at Lines 1039-1042 and 1055-1058 duplicates the one already declared in the parentdescribescope (Lines 672-675). The locally scoped copies can be removed since they're identical and the parent-scope variable is accessible.Proposed cleanup
describe('edge cases', () => { it('should handle empty array', () => { - const emptyTimestamps: IncidentsTimestamps = { - minOverTime: [], - lastOverTime: [], - }; const result = processIncidentsForAlerts([], emptyTimestamps); expect(result).toEqual([]); }); it('should handle single incident', () => { const incidents: PrometheusResult[] = [ { metric: { group_id: 'incident1', silenced: 'true' }, values: [[1000, '2']], }, ]; - const emptyTimestamps: IncidentsTimestamps = { - minOverTime: [], - lastOverTime: [], - }; - const result = processIncidentsForAlerts(incidents, emptyTimestamps);web/src/store/actions.ts (1)
193-197: Consider adding parameter types to the new action creators.Both
setIncidentsTimestampsandsetAlertsTimestampsaccept untyped parameters. While this is consistent with existing action creators in this file (e.g.,setIncidents,setAlertsData), addingIncidentsTimestamps/AlertsTimestampstypes would improve type safety and discoverability.web/src/components/Incidents/processAlerts.ts (2)
76-107: Gap-splitting logic is duplicated betweendeduplicateAlertsandsplitAbsoluteTimestampsByGap.Both iterate sorted values, detect gaps >
PROMETHEUS_QUERY_INTERVAL_SECONDS, and split into sub-intervals. Consider extracting a shared helper likesplitValuesByGap(sortedValues, threshold)to reduce duplication.Also applies to: 116-141
393-408: VariablefirstTimestampon line 395 shadows the outer scope and serves a different meaning.Line 395 assigns
firstTimestampfrom paddedValues (chart start position), while line 402 usesalert.firstTimestamp(absolute start). Both are named similarly but mean different things. Renaming the local tochartFirstTimestamporpaddedFirstTimestampwould improve readability.web/src/components/Incidents/IncidentsPage.tsx (2)
265-274: Alert timestamp values are reshaped to single-element arrays — verify downstream consumers expect this.The mapping on lines 271-273 transforms each alert value from
[queryTimestamp, alertTimestamp]to[roundedTimestamp](single-element array). Downstream code insplitAbsoluteTimestampsByGapandassignAbsoluteFirstTimestampsaccessesvalues[x][0], so this works — but the non-standard shape is fragile and undocumented. A comment clarifying whyvalue[1]is the actual alert firing timestamp (from thetimestamp()PromQL function) and whyvalue[0](the query evaluation time) is dropped would help future maintainers.
338-349: Hardcoded15dlookback for min_over_time query — should ideally align with the maximum selectable days filter.The Prometheus query
min_over_time(timestamp(cluster_health_components_map)[15d:5m])hardcodes a 15-day lookback. This happens to match the current max selectable range ("15 days"), but if the max range is ever extended, these will silently diverge. Consider extracting this as a constant.web/src/components/Incidents/utils.ts (1)
955-966: All parameters and return typed asany— limits type safety and IDE support.While defensively coded, this function could benefit from minimal typing. Even a simple interface for the
timestampmetric shape would catch mismatches (e.g., thevaluevsvaluesissue in tests).
web/src/components/Incidents/api.ts
Outdated
| const response = await Promise.resolve(consoleFetchJSON(url)); | ||
| return { | ||
| status: 'success', | ||
| data: { | ||
| resultType: 'matrix', | ||
| result: response.data.result, | ||
| }, | ||
| } as PrometheusResponse; |
There was a problem hiding this comment.
Add defensive access on response.data.result to avoid potential TypeError.
If the API returns an unexpected shape (e.g., an error response without a data field), response.data.result at Line 185 will throw. The existing fetchDataForIncidentsAndAlerts (Line 145) uses optional chaining (r.data?.result || []) for this reason.
Also, Promise.resolve(consoleFetchJSON(url)) is redundant — consoleFetchJSON already returns a Promise.
Proposed fix
- const response = await Promise.resolve(consoleFetchJSON(url));
+ const response = await consoleFetchJSON(url);
return {
status: 'success',
data: {
resultType: 'matrix',
- result: response.data.result,
+ result: response?.data?.result || [],
},
} as PrometheusResponse;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await Promise.resolve(consoleFetchJSON(url)); | |
| return { | |
| status: 'success', | |
| data: { | |
| resultType: 'matrix', | |
| result: response.data.result, | |
| }, | |
| } as PrometheusResponse; | |
| const response = await consoleFetchJSON(url); | |
| return { | |
| status: 'success', | |
| data: { | |
| resultType: 'matrix', | |
| result: response?.data?.result || [], | |
| }, | |
| } as PrometheusResponse; |
🤖 Prompt for AI Agents
In `@web/src/components/Incidents/api.ts` around lines 180 - 187, The code should
call consoleFetchJSON(url) directly (remove the redundant Promise.resolve) and
defensively access the result to avoid a TypeError: capture the response from
consoleFetchJSON and use response.data?.result || [] when constructing the
returned PrometheusResponse (keep the same shape with resultType: 'matrix'),
ensuring you still return the object as PrometheusResponse; reference the
variables/functions response, consoleFetchJSON and the PrometheusResponse return
shape and mirror the optional chaining used in fetchDataForIncidentsAndAlerts.
| ); | ||
|
|
||
| const incidentFirstTimestamp = Math.min(...timestamps); | ||
| const incidentFirstTimestamp = incidents[0].firstTimestamp; |
There was a problem hiding this comment.
Potential TypeError if incidents array is empty after merging.
incidents[0].firstTimestamp will throw if mergeIncidentsByKey returns an empty array. While the caller in IncidentsPage.tsx guards against an empty incidentForAlertProcessing, this function is public and should be defensive.
Proposed fix
- const incidentFirstTimestamp = incidents[0].firstTimestamp;
+ const incidentFirstTimestamp = incidents[0]?.firstTimestamp ?? 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const incidentFirstTimestamp = incidents[0].firstTimestamp; | |
| const incidentFirstTimestamp = incidents[0]?.firstTimestamp ?? 0; |
🤖 Prompt for AI Agents
In `@web/src/components/Incidents/processAlerts.ts` at line 344, The code assumes
incidents[0] exists when computing incidentFirstTimestamp in processAlerts
(after calling mergeIncidentsByKey), which can throw if the merged incidents
array is empty; update the public function in processAlerts.ts to defensively
handle an empty array by checking incidents.length (or the result of
mergeIncidentsByKey) before accessing incidents[0].firstTimestamp and either
return early/skip processing, or set incidentFirstTimestamp to a safe default
(e.g., undefined) and propagate that state so callers won’t dereference a
missing element; reference the variable incidentFirstTimestamp and the
mergeIncidentsByKey call to find where to add the guard.
| export const processIncidentsForAlerts = ( | ||
| incidents: Array<PrometheusResult>, | ||
| ): Array<Partial<Incident>> => { | ||
| return incidents.map((incident, index) => { | ||
| incidentsTimestamps: IncidentsTimestamps, | ||
| ) => { | ||
| const matchedIncidents = incidents.map((incident) => { | ||
| // expand matchTimestampMetricForIncident here | ||
| const matchedMinTimestamp = matchTimestampMetricForIncident( | ||
| incident.metric, | ||
| incidentsTimestamps.minOverTime, | ||
| ); | ||
|
|
||
| return { | ||
| ...incident, | ||
| firstTimestamp: parseInt(matchedMinTimestamp?.value?.[1] ?? '0'), | ||
| }; | ||
| }); | ||
|
|
||
| return matchedIncidents.map((incident, index) => { | ||
| // Read silenced value from cluster_health_components_map metric label | ||
| // If missing, default to false | ||
| const silenced = incident.metric.silenced === 'true'; | ||
|
|
||
| // Return the processed incident | ||
| return { | ||
| const retval = { | ||
| ...incident.metric, | ||
| values: incident.values, | ||
| x: incidents.length - index, | ||
| silenced, | ||
| firstTimestamp: incident.firstTimestamp, | ||
| }; | ||
| return retval; | ||
| }); |
There was a problem hiding this comment.
Same epoch-fallback issue: firstTimestamp defaults to 0 when no timestamp match is found.
Line 207 uses the same parseInt(matchedMinTimestamp?.value?.[1] ?? '0') pattern already flagged in IncidentsChart.tsx. When no match exists, firstTimestamp becomes 0 (epoch), which propagates misleading dates into downstream alert processing. Consider using the incident's earliest value timestamp as a fallback here too.
🤖 Prompt for AI Agents
In `@web/src/components/Incidents/processIncidents.ts` around lines 194 - 225,
processIncidentsForAlerts currently falls back to '0' when
matchTimestampMetricForIncident returns no match, producing epoch timestamps;
change the fallback for firstTimestamp to use the incident's earliest value
timestamp (from incident.values) before finally defaulting to '0'. Specifically,
when computing firstTimestamp in processIncidentsForAlerts, pick
matchedMinTimestamp?.value?.[1] if present, otherwise use the timestamp from
incident.values (the earliest entry), and then parseInt that result to set
firstTimestamp; update any parseInt usage accordingly to avoid defaulting
straight to epoch.
| const incident = { | ||
| group_id: 'group1', | ||
| src_alertname: 'Alert1', | ||
| src_namespace: 'ns1', | ||
| component: 'comp1', | ||
| src_severity: 'warning', | ||
| }; | ||
|
|
||
| const timestamps = [ | ||
| { | ||
| metric: { | ||
| group_id: 'group1', | ||
| src_alertname: 'Alert1', | ||
| src_namespace: 'ns1', | ||
| component: 'comp1', | ||
| src_severity: 'warning', | ||
| }, | ||
| value: [1704067200, '1704067200'], | ||
| }, | ||
| ]; | ||
|
|
||
| const result = matchTimestampMetricForIncident(incident, timestamps); | ||
| expect(result).toBe(timestamps[0]); | ||
| }); |
There was a problem hiding this comment.
Test assertions and it blocks are incorrectly scoped — assertions at describe level, subsequent tests orphaned.
Lines 156–178 contain variable declarations and expect assertions directly inside the describe('matchTimestampMetricForIncident') body (not wrapped in an it block). These will execute during Jest's collection phase, not as a proper test. Additionally, the describe closes at line 179, so the it blocks on lines 181–270 become top-level tests, detached from their intended describe group.
This looks like a missing it(...) wrapper around lines 156–178.
Proposed fix
- const incident = {
+ it('should match when all fields match exactly', () => {
+ const incident = {
group_id: 'group1',
src_alertname: 'Alert1',
src_namespace: 'ns1',
component: 'comp1',
src_severity: 'warning',
};
const timestamps = [
{
metric: {
group_id: 'group1',
src_alertname: 'Alert1',
src_namespace: 'ns1',
component: 'comp1',
src_severity: 'warning',
},
value: [1704067200, '1704067200'],
},
];
const result = matchTimestampMetricForIncident(incident, timestamps);
expect(result).toBe(timestamps[0]);
-});
+ });
-it('should not match when group_id differs', () => {
+ it('should not match when group_id differs', () => {And similarly re-indent the remaining it blocks (lines 181–270) to be inside the describe by moving the closing }); of the describe to after line 270.
🤖 Prompt for AI Agents
In `@web/src/components/Incidents/utils.spec.ts` around lines 156 - 179, The test
setup and assertion for matchTimestampMetricForIncident are placed directly in
the describe block, causing them to run during collection and orphaning
subsequent it blocks; wrap the variable declarations (incident, timestamps,
result) and the expect(result).toBe(...) assertion inside a new it('returns
matching timestamp metric', () => { ... }) block, and move the describe closing
token (the final "});") to after the existing it blocks so all it tests
(including those on lines 181–270) are nested inside
describe('matchTimestampMetricForIncident'); keep references to
matchTimestampMetricForIncident, incident, timestamps, and result when
relocating code.
| startDate: Date; | ||
| fill: string; |
There was a problem hiding this comment.
Same concern: startDate derived from firstTimestamp which may be epoch (0) for unmatched incidents.
new Date(incident.firstTimestamp * 1000) on line 350 will produce "Jan 1, 1970" if firstTimestamp is 0 (the default when no timestamp match is found). This is the display-side of the issue flagged in IncidentsChart.tsx.
Also applies to: 350-350
🤖 Prompt for AI Agents
In `@web/src/components/Incidents/utils.ts` around lines 325 - 326, The code
constructs startDate from incident.firstTimestamp using new
Date(incident.firstTimestamp * 1000) which yields 1970-01-01 for a 0 timestamp;
change the construction so that when incident.firstTimestamp is missing/0 you
set startDate to null (or undefined) instead of a Date(0), update the startDate
type to Date | null in the relevant interface/shape (the startDate property in
the object/type), and adjust consumers (e.g., IncidentsChart.tsx and any
rendering logic) to handle null startDate appropriately; specifically locate the
new Date(...) call and replace it with a guarded expression that only creates a
Date when incident.firstTimestamp > 0 and otherwise assigns null, and update the
type annotations for startDate to match.
ceb6fcd to
bb1ffe8
Compare
|
@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 "4.22.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. |
rioloc
force-pushed
the
feat/absolute-start-dates
branch
from
bb1ffe8 to
7ce5c6a
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 (3)
web/src/components/Incidents/IncidentsPage.tsx (1)
340-344:⚠️ Potential issue | 🟡 MinorInconsistent error logging:
console.logvsconsole.error.Line 343 uses
console.log(err)while the analogous catch at line 281 usesconsole.error(err). Useconsole.errorconsistently for error paths.Proposed fix
- console.log(err); + console.error(err);🤖 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 340 - 344, In IncidentsPage component's promise catch block that currently reads .catch((err) => { console.log(err); }) replace console.log with console.error to match the other error path and use consistent error logging (keep the eslint-disable-next-line no-console if needed); target the .catch arrow function where console logging is used and change it to console.error(err).web/src/components/Incidents/IncidentsTable.tsx (1)
94-99:⚠️ Potential issue | 🟡 Minor
getMinStartDatecan surface epoch whenfirstTimestampis0.If any
alertData.firstTimestampis still0(the current default inprocessIncidentsForAlerts),Math.min(...)will return0and the<Timestamp>at line 183 will display "Jan 1, 1970". The root cause is thefirstTimestamp: 0placeholder inprocessIncidents.ts(line 313), but this function should also guard:Proposed defensive fix
const getMinStartDate = (alert: GroupedAlert): number => { if (!alert.alertsExpandedRowData || alert.alertsExpandedRowData.length === 0) { return 0; } - return Math.min(...alert.alertsExpandedRowData.map((alertData) => alertData.firstTimestamp)); + const timestamps = alert.alertsExpandedRowData + .map((alertData) => alertData.firstTimestamp) + .filter((ts) => ts > 0); + return timestamps.length > 0 ? Math.min(...timestamps) : 0; };🤖 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 alerts list is empty but will also return 0 if any firstTimestamp values include the placeholder 0, causing a "Jan 1, 1970" display; update getMinStartDate to filter out non-positive/zero firstTimestamp values before taking Math.min (e.g., collect alert.alertsExpandedRowData.map(a => a.firstTimestamp).filter(ts => ts > 0) and if that filtered array is empty return 0, otherwise return Math.min(...filtered)), referencing getMinStartDate and alert.alertsExpandedRowData/firstTimestamp to locate the change.web/src/components/Incidents/processAlerts.ts (1)
286-298:⚠️ Potential issue | 🟠 MajorIncident matching should include
componentto align with alert deduplication logicThe
matchingIncidentlookup omitscomponent(lines 286-298), but alerts are explicitly deduplicated by(alertname, namespace, component, severity)(seededuplicateAlertsin the same file). Additionally,matchTimestampMetricForIncidentinutils.tsincludescomponentin its matching criteria. If two alerts share the same alertname, namespace, and severity but differ in component, this could return an incorrect match. Consider addingcomponentto the matching condition to align with how alerts are grouped and how other timestamp matching is performed.🤖 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 286 - 298, The incident lookup in the matchingIncident find call omits component and can return wrong matches; update the predicate used in incidents.find (the matchingIncident logic) to also compare incident.component === alert.metric.component so it aligns with deduplicateAlerts and matchTimestampMetricForIncident in utils.ts; ensure you update the predicate that currently checks src_alertname, src_namespace, and src_severity to include src_component/component comparison as well.
🧹 Nitpick comments (5)
web/src/components/Incidents/utils.ts (3)
312-335: Orphaned JSDoc block forcreateIncidentsChartBars.Lines 312–321 contain a JSDoc comment describing
createIncidentsChartBars, but it's immediately followed by a second JSDoc block (lines 322–335) forremoveTrailingPaddingFromSeveritySegments. The first JSDoc is detached from its target function (which starts at line 349) and will not be associated by tooling.Either move the first JSDoc directly above line 349 or remove the duplicate since the function is self-descriptive from context.
🤖 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 312 - 335, The first JSDoc for createIncidentsChartBars is orphaned and separated from the function declaration (createIncidentsChartBars) by another JSDoc for removeTrailingPaddingFromSeveritySegments; move the JSDoc block currently at lines 312–321 so it immediately precedes the createIncidentsChartBars function declaration (or delete that duplicate comment if you prefer) to ensure tooling associates the correct documentation with createIncidentsChartBars and keep the removeTrailingPaddingFromSeveritySegments JSDoc directly above its function.
994-1005: Prefer typed parameters overanyinmatchTimestampMetricForIncident.The function uses
anyfor bothincidentandtimestampsparameters. Since theIncidenttype already exists inmodel.tsand the timestamps have a known Prometheus result shape, using proper types would catch label mismatches at compile time and improve discoverability.🤖 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 994 - 1005, The function matchTimestampMetricForIncident currently uses `any` for `incident` and `timestamps`; replace these with concrete types by importing the existing `Incident` type from model.ts and defining/using a typed shape for the timestamp entries (e.g., a Prometheus result or an interface with `metric: { group_id, src_alertname, src_namespace, component, src_severity }` and any needed value fields). Update the signature to `incident: Incident` and `timestamps: Array<YourTimestampType>` (or a named alias), adjust the runtime null/array guard to use the typed parameter, and ensure the code references the typed `timestamp.metric` fields so TypeScript will validate label names at compile time.
349-352: Untypedvalueparameter ingetSeverityName.The inner function
getSeverityNameon line 350 accepts an untypedvalueparameter. Adding an explicit type (e.g.,string) improves clarity and catches misuse at compile time.Proposed fix
- const getSeverityName = (value) => { + const getSeverityName = (value: string) => {🤖 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 349 - 352, The inner helper getSeverityName inside createIncidentsChartBars currently accepts an untyped parameter; update its signature to explicitly type the parameter (e.g., value: string) and optionally the return type (e.g., : string) so TypeScript can enforce correct usage and catch misuse of non-string severity values; locate getSeverityName in createIncidentsChartBars and add the parameter and return type annotations.web/src/components/Incidents/IncidentsPage.tsx (1)
240-241: Always fetching 15 days of data regardless of user's selection.Both fetch effects unconditionally use
getIncidentsTimeRanges(15 * DAY_MS, currentTime). When the user selects "Last 1 day", this still fetches 15 daily chunks. This is intentional for absolute timestamp computation, but consider whether the performance cost is acceptable — especially for clusters with dense alert histories.Also applies to: 305-306
🤖 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 240 - 241, The code unconditionally computes fetchTimeRanges via getIncidentsTimeRanges(15 * DAY_MS, currentTime) (using DAY_MS, currentTime and fetchTimeRanges) which causes both fetch effects to fetch 15 days of chunks even when the user selected a shorter range; change the logic so you only request the full 15-day history when you actually need it to compute firstTimestamp (e.g., compute firstTimestamp lazily or call getIncidentsTimeRanges(15 * DAY_MS, currentTime) only when selectedRange !== '1d' is false and firstTimestamp is unresolved), otherwise compute fetchTimeRanges from the user-selected range (e.g., getIncidentsTimeRanges(selectedRangeMs, currentTime)) and avoid the expensive 15-day fetch; update both places where fetchTimeRanges is used (around where fetch effects are defined) to use this conditional behavior.web/src/components/Incidents/processIncidents.spec.ts (1)
779-779:as any[]casts indicate return type mismatch.The casts at lines 779 and 843 work around a typing gap between
processIncidentsForAlerts's return type and the assertions. Consider updating the function's return type or adding a proper type to avoid silent type erosion.🤖 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 779, The test is using "as any[]" to silence a return-type mismatch from processIncidentsForAlerts; update the function's declared return type (or export a precise type alias) so callers/tests can rely on a concrete shape instead of casting. Specifically, adjust the signature of processIncidentsForAlerts to return the correct array type (e.g., IncidentAlert[] or a named type matching the objects used in the spec) or export a ReturnType alias and use that in the spec assertions to remove the "as any[]" casts and retain strong typing.
🤖 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 232-283: The effect calls convertToAlerts(..., daysSpan) but
daysSpan is not in the useEffect dependency array, causing a stale closure;
update the dependency array for the useEffect that contains convertToAlerts (the
effect that maps fetchTimeRanges and dispatches setAlertsData /
setAlertsTableData) to include daysSpan so the effect re-runs when the days
filter changes.
- Around line 273-277: The loading state is being set using a potentially stale
filteredData; update the logic so the effect that calls
dispatch(setAlertsAreLoading(...)) bases its decision on fresh data: either
include filteredData in that effect's dependency array (so the effect re-runs
when filteredData changes) or, better, compute loading from the newly fetched
alerts response (e.g., use the fetchedAlerts length/result instead of
filteredData) and then dispatch setAlertsAreLoading accordingly; locate the code
around filteredData, dispatch, and setAlertsAreLoading in IncidentsPage.tsx (the
effect that currently checks isEmpty(filteredData)) and change it to use the
latest fetched alerts or add filteredData to the effect dependencies to avoid
the race.
In `@web/src/components/Incidents/processAlerts.ts`:
- Around line 302-304: The calculation of alertFirstTimestamp can produce NaN
when alert.values is empty; defensively guard against undefined by checking
alert.values and alert.values[0] before subtracting
PROMETHEUS_QUERY_INTERVAL_SECONDS (e.g., treat missing values as null/0 or skip
the alert), so update the code around alertFirstTimestamp in processAlerts.ts to
compute a safe fallback, and ensure downstream uses (firstTimestamp and
alertsStartFiring) handle the fallback consistently; reference the
alertFirstTimestamp variable, PROMETHEUS_QUERY_INTERVAL_SECONDS constant, and
the deduplicateAlerts-derived alert.values to locate and fix the logic.
In `@web/src/components/Incidents/processIncidents.spec.ts`:
- Around line 806-827: The test currently asserts the placeholder firstTimestamp
of 0 returned by processIncidentsForAlerts; once you implement the TODO in
processIncidentsForAlerts so that firstTimestamp is derived like convertToAlerts
does, update this spec to compute the expected firstTimestamp from the provided
Prometheus values and assert that result[0].firstTimestamp equals that derived
value (replace the hardcoded 0 check). Refer to processIncidentsForAlerts and
convertToAlerts to locate the conversion logic and ensure the test mirrors the
same timestamp derivation.
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 184-191: The current logic only records the first severity in
severityMetrics for a result, which misses later severity changes; update the
block that handles a result (the code that checks result.values.length and
result.metric.src_alertname !== 'Watchdog') to iterate over all entries in
result.values and record each distinct severity value (e.g., value[1]) into
severityMetrics.get(groupId) mapping to result.metric; keep the existing guard
that initializes severityMetrics.set(groupId, new Map()) when missing and ensure
you deduplicate so each severity value for that groupId maps to the metric used
later by groupSeverityMetrics.get(segmentSeverity).
---
Outside diff comments:
In `@web/src/components/Incidents/IncidentsPage.tsx`:
- Around line 340-344: In IncidentsPage component's promise catch block that
currently reads .catch((err) => { console.log(err); }) replace console.log with
console.error to match the other error path and use consistent error logging
(keep the eslint-disable-next-line no-console if needed); target the .catch
arrow function where console logging is used and change it to
console.error(err).
In `@web/src/components/Incidents/IncidentsTable.tsx`:
- Around line 94-99: getMinStartDate currently returns 0 when alerts list is
empty but will also return 0 if any firstTimestamp values include the
placeholder 0, causing a "Jan 1, 1970" display; update getMinStartDate to filter
out non-positive/zero firstTimestamp values before taking Math.min (e.g.,
collect alert.alertsExpandedRowData.map(a => a.firstTimestamp).filter(ts => ts >
0) and if that filtered array is empty return 0, otherwise return
Math.min(...filtered)), referencing getMinStartDate and
alert.alertsExpandedRowData/firstTimestamp to locate the change.
In `@web/src/components/Incidents/processAlerts.ts`:
- Around line 286-298: The incident lookup in the matchingIncident find call
omits component and can return wrong matches; update the predicate used in
incidents.find (the matchingIncident logic) to also compare incident.component
=== alert.metric.component so it aligns with deduplicateAlerts and
matchTimestampMetricForIncident in utils.ts; ensure you update the predicate
that currently checks src_alertname, src_namespace, and src_severity to include
src_component/component comparison as well.
---
Duplicate comments:
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 300-316: processIncidentsForAlerts currently returns
firstTimestamp: 0 which yields epoch dates; replace that placeholder by
computing the incident's earliest value timestamp from incident.values and
assign it to firstTimestamp. Locate processIncidentsForAlerts and use the
incident.values array (each value entry contains a timestamp) to derive the
minimum/earliest timestamp (convert entries to Number if they are strings) and
set firstTimestamp to that value (keep previous behaviour if values is empty by
using undefined or null as appropriate for downstream components).
In `@web/src/components/Incidents/utils.spec.ts`:
- Around line 158-181: The test declarations and assertions for
matchTimestampMetricForIncident are placed directly inside the describe block
(variables incident, timestamps and the expect call) rather than inside an it
block, causing orphaned tests; wrap the block that declares incident, timestamps
and calls expect(matchTimestampMetricForIncident(...)) in a proper it(...) test
(e.g., it('matches timestamp metric for incident', () => { ... })) and move the
describe(...) closing brace so it comes after the final it block (so all
subsequent it tests remain inside the describe).
In `@web/src/components/Incidents/utils.ts`:
- Around line 386-388: The startDate is set to new Date(incident.firstTimestamp
* 1000) which yields the epoch when firstTimestamp is 0; change the assignment
in the object (where group_id, nodata, startDate are set) to guard against a
zero timestamp (from processIncidentsForAlerts) by using a conditional: if
incident.firstTimestamp is a positive number create new
Date(incident.firstTimestamp * 1000) otherwise set startDate to null (or
undefined) so tooltips won't show 1970-01-01; update any downstream consumers
expecting a Date to handle the nullable value accordingly.
---
Nitpick comments:
In `@web/src/components/Incidents/IncidentsPage.tsx`:
- Around line 240-241: The code unconditionally computes fetchTimeRanges via
getIncidentsTimeRanges(15 * DAY_MS, currentTime) (using DAY_MS, currentTime and
fetchTimeRanges) which causes both fetch effects to fetch 15 days of chunks even
when the user selected a shorter range; change the logic so you only request the
full 15-day history when you actually need it to compute firstTimestamp (e.g.,
compute firstTimestamp lazily or call getIncidentsTimeRanges(15 * DAY_MS,
currentTime) only when selectedRange !== '1d' is false and firstTimestamp is
unresolved), otherwise compute fetchTimeRanges from the user-selected range
(e.g., getIncidentsTimeRanges(selectedRangeMs, currentTime)) and avoid the
expensive 15-day fetch; update both places where fetchTimeRanges is used (around
where fetch effects are defined) to use this conditional behavior.
In `@web/src/components/Incidents/processIncidents.spec.ts`:
- Line 779: The test is using "as any[]" to silence a return-type mismatch from
processIncidentsForAlerts; update the function's declared return type (or export
a precise type alias) so callers/tests can rely on a concrete shape instead of
casting. Specifically, adjust the signature of processIncidentsForAlerts to
return the correct array type (e.g., IncidentAlert[] or a named type matching
the objects used in the spec) or export a ReturnType alias and use that in the
spec assertions to remove the "as any[]" casts and retain strong typing.
In `@web/src/components/Incidents/utils.ts`:
- Around line 312-335: The first JSDoc for createIncidentsChartBars is orphaned
and separated from the function declaration (createIncidentsChartBars) by
another JSDoc for removeTrailingPaddingFromSeveritySegments; move the JSDoc
block currently at lines 312–321 so it immediately precedes the
createIncidentsChartBars function declaration (or delete that duplicate comment
if you prefer) to ensure tooling associates the correct documentation with
createIncidentsChartBars and keep the removeTrailingPaddingFromSeveritySegments
JSDoc directly above its function.
- Around line 994-1005: The function matchTimestampMetricForIncident currently
uses `any` for `incident` and `timestamps`; replace these with concrete types by
importing the existing `Incident` type from model.ts and defining/using a typed
shape for the timestamp entries (e.g., a Prometheus result or an interface with
`metric: { group_id, src_alertname, src_namespace, component, src_severity }`
and any needed value fields). Update the signature to `incident: Incident` and
`timestamps: Array<YourTimestampType>` (or a named alias), adjust the runtime
null/array guard to use the typed parameter, and ensure the code references the
typed `timestamp.metric` fields so TypeScript will validate label names at
compile time.
- Around line 349-352: The inner helper getSeverityName inside
createIncidentsChartBars currently accepts an untyped parameter; update its
signature to explicitly type the parameter (e.g., value: string) and optionally
the return type (e.g., : string) so TypeScript can enforce correct usage and
catch misuse of non-string severity values; locate getSeverityName in
createIncidentsChartBars and add the parameter and return type annotations.
| useEffect(() => { | ||
| (async () => { | ||
| const currentTime = incidentsLastRefreshTime; | ||
| Promise.all( | ||
| timeRanges.map(async (range) => { | ||
| const response = await fetchDataForIncidentsAndAlerts( | ||
| safeFetch, | ||
| range, | ||
| createAlertsQuery(incidentForAlertProcessing), | ||
| ); | ||
| return response.data.result; | ||
| }), | ||
| ) | ||
| .then((results) => { | ||
| const prometheusResults = results.flat(); | ||
| const alerts = convertToAlerts( | ||
| prometheusResults, | ||
| incidentForAlertProcessing, | ||
| currentTime, | ||
| ); | ||
| // Guard: don't process if no incidents selected or timeRanges not ready | ||
| if (incidentForAlertProcessing.length === 0 || timeRanges.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const currentTime = incidentsLastRefreshTime; | ||
|
|
||
| // Always fetch 15 days of alert data so firstTimestamp is computed from full history | ||
| const fetchTimeRanges = getIncidentsTimeRanges(15 * DAY_MS, currentTime); | ||
|
|
||
| Promise.all( | ||
| fetchTimeRanges.map(async (range) => { | ||
| const response = await fetchDataForIncidentsAndAlerts( | ||
| safeFetch, | ||
| range, | ||
| createAlertsQuery(incidentForAlertProcessing), | ||
| ); | ||
| return response.data.result; | ||
| }), | ||
| ) | ||
| .then((alertsResults) => { | ||
| const prometheusResults = alertsResults.flat(); | ||
| const alerts = convertToAlerts( | ||
| prometheusResults, | ||
| incidentForAlertProcessing, | ||
| currentTime, | ||
| daysSpan, | ||
| ); | ||
| dispatch( | ||
| setAlertsData({ | ||
| alertsData: alerts, | ||
| }), | ||
| ); | ||
| if (rules && alerts) { | ||
| dispatch( | ||
| setAlertsData({ | ||
| alertsData: alerts, | ||
| setAlertsTableData({ | ||
| alertsTableData: groupAlertsForTable(alerts, rules), | ||
| }), | ||
| ); | ||
| if (rules && alerts) { | ||
| dispatch( | ||
| setAlertsTableData({ | ||
| alertsTableData: groupAlertsForTable(alerts, rules), | ||
| }), | ||
| ); | ||
| } | ||
| if (!isEmpty(filteredData)) { | ||
| dispatch(setAlertsAreLoading({ alertsAreLoading: false })); | ||
| } else { | ||
| dispatch(setAlertsAreLoading({ alertsAreLoading: true })); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(err); | ||
| }); | ||
| })(); | ||
| }, [incidentForAlertProcessing]); | ||
| } | ||
| if (!isEmpty(filteredData)) { | ||
| dispatch(setAlertsAreLoading({ alertsAreLoading: false })); | ||
| } else { | ||
| dispatch(setAlertsAreLoading({ alertsAreLoading: true })); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error(err); | ||
| }); | ||
| }, [incidentForAlertProcessing, timeRanges, rules]); |
There was a problem hiding this comment.
daysSpan used inside effect but missing from dependency array — stale closure risk.
Line 259 passes daysSpan to convertToAlerts, but daysSpan is not listed in the dependency array at line 283 ([incidentForAlertProcessing, timeRanges, rules]). When the user changes the days filter, daysSpan updates via a separate useEffect (line 222), but this effect won't re-run for that change alone. The captured daysSpan in the closure may be stale, causing alerts to be filtered against the wrong time window.
Proposed fix
- }, [incidentForAlertProcessing, timeRanges, rules]);
+ }, [incidentForAlertProcessing, timeRanges, rules, daysSpan]);🤖 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 232 - 283, The
effect calls convertToAlerts(..., daysSpan) but daysSpan is not in the useEffect
dependency array, causing a stale closure; update the dependency array for the
useEffect that contains convertToAlerts (the effect that maps fetchTimeRanges
and dispatches setAlertsData / setAlertsTableData) to include daysSpan so the
effect re-runs when the days filter changes.
| // firstTimestamp is absolute over the full 15-day data (before N-day filtering), | ||
| // with the padding offset applied to match chart rendering | ||
| const alertFirstTimestamp = alert.values[0]?.[0] - PROMETHEUS_QUERY_INTERVAL_SECONDS; |
There was a problem hiding this comment.
alertFirstTimestamp may be NaN if alert.values is unexpectedly empty.
While the current flow should guarantee non-empty alert.values from deduplicateAlerts, the optional chaining alert.values[0]?.[0] returns undefined when empty, making undefined - 300 = NaN. This propagates silently into firstTimestamp and alertsStartFiring.
Consider a defensive fallback:
Proposed fix
- const alertFirstTimestamp = alert.values[0]?.[0] - PROMETHEUS_QUERY_INTERVAL_SECONDS;
+ const alertFirstTimestamp = (alert.values[0]?.[0] ?? sortedValues[0][0]) - PROMETHEUS_QUERY_INTERVAL_SECONDS;🤖 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 302 - 304, The
calculation of alertFirstTimestamp can produce NaN when alert.values is empty;
defensively guard against undefined by checking alert.values and alert.values[0]
before subtracting PROMETHEUS_QUERY_INTERVAL_SECONDS (e.g., treat missing values
as null/0 or skip the alert), so update the code around alertFirstTimestamp in
processAlerts.ts to compute a safe fallback, and ensure downstream uses
(firstTimestamp and alertsStartFiring) handle the fallback consistently;
reference the alertFirstTimestamp variable, PROMETHEUS_QUERY_INTERVAL_SECONDS
constant, and the deduplicateAlerts-derived alert.values to locate and fix the
logic.
| describe('firstTimestamp', () => { | ||
| it('should default firstTimestamp to 0', () => { | ||
| const incidents: PrometheusResult[] = [ | ||
| { | ||
| metric: { | ||
| group_id: 'incident1', | ||
| src_alertname: 'TestAlert', | ||
| src_namespace: 'test-namespace', | ||
| component: 'test-component', | ||
| src_severity: 'critical', | ||
| }, | ||
| values: [[1704067300, '2']], | ||
| }, | ||
| ]; | ||
|
|
||
| const result = processIncidentsForAlerts(incidents); | ||
| expect(result).toHaveLength(1); | ||
| // processIncidentsForAlerts sets firstTimestamp to 0; | ||
| // the real firstTimestamp is computed in convertToAlerts from alert data | ||
| expect(result[0].firstTimestamp).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test documents the firstTimestamp: 0 placeholder.
This test confirms the unfinished work in processIncidentsForAlerts. Once the TODO at processIncidents.ts:312 is resolved (as suggested in that file's review), this test should be updated to assert the correct derived value rather than 0.
🤖 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 806 -
827, The test currently asserts the placeholder firstTimestamp of 0 returned by
processIncidentsForAlerts; once you implement the TODO in
processIncidentsForAlerts so that firstTimestamp is derived like convertToAlerts
does, update this spec to compute the expected firstTimestamp from the provided
Prometheus values and assert that result[0].firstTimestamp equals that derived
value (replace the hardcoded 0 check). Refer to processIncidentsForAlerts and
convertToAlerts to locate the conversion logic and ensure the test mirrors the
same timestamp derivation.
|
@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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx (1)
112-117:⚠️ Potential issue | 🟡 Minor
a[0].xwill throw ifcreateIncidentsChartBarsreturns an empty array for any group.If a group's incidents all produce empty
consolidateAndMergeIntervalsresults,createIncidentsChartBarsreturns[]. Accessinga[0].xon an empty array yieldsundefined, andundefined - undefinedproducesNaN, which silently corrupts sort order. Worse,bars.map(...)on an empty inner array followed bybar[0].group_idat line 272 will throw.Proposed defensive fix
- 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, createIncidentsChartBars can return [] for some groups which makes chartBars.sort((a,b)=>a[0].x - b[0].x) and later accesses like bars[0].group_id unsafe; filter out empty bars immediately after creating chartBars (e.g., const chartBars = adjustedGroups.map(...).filter(b => b.length > 0)) so the sort and subsequent map can safely use a[0].x and bars[0].group_id, then proceed to sort by a[0].x and reassign consecutive x values as before; also ensure any other downstream code that assumes non-empty bars (references to bars[0]) only runs after this filtering.web/src/components/Incidents/IncidentsPage.tsx (1)
341-344:⚠️ Potential issue | 🟡 MinorInconsistent error logging:
console.loghere vsconsole.errorat line 281.The incidents fetch catch block uses
console.logwhile the alerts fetch usesconsole.error. Both should useconsole.errorfor consistency and proper log-level semantics.Proposed fix
.catch((err) => { // eslint-disable-next-line no-console - console.log(err); + console.error(err); });🤖 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 341 - 344, The catch block handling the incidents fetch currently uses console.log((err)) which is inconsistent with the alerts fetch that uses console.error; update the incidents fetch catch (the .catch((err) => { console.log(err); }) block) to call console.error(err) instead (and remove the eslint-disable-next-line no-console if no longer needed) so error logging is consistent and uses the proper log level.web/src/components/Incidents/model.ts (1)
95-124:⚠️ Potential issue | 🟠 MajorAdd
lastTimestampfield toIncidentsDetailsAlerttype definition.
IncidentsDetailsRowTable.tsx(line 63–64) accessesalertDetails.lastTimestamp, but theIncidentsDetailsAlerttype does not declare this field. This causes a TypeScript type error.Fix
export type IncidentsDetailsAlert = { alertname: string; alertsStartFiring: number; alertsEndFiring: number; alertstate: string; component: string; layer: string; name: string; namespace: string; resolved: boolean; severity: Severity; x: number; firstTimestamp: number; + lastTimestamp: number; values: Array<Timestamps>; silenced: boolean; rule: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Incidents/model.ts` around lines 95 - 124, The IncidentsDetailsAlert type is missing the lastTimestamp field used elsewhere; update the IncidentsDetailsAlert type (in model.ts) to include lastTimestamp: number (or number | undefined if it can be absent) so references like alertDetails.lastTimestamp in IncidentsDetailsRowTable.tsx type-check correctly.
🧹 Nitpick comments (8)
web/src/components/Incidents/model.ts (1)
18-18:severity: anyweakens type safety.A
Severitytype is already defined at line 63 of this file. Usinganyhere bypasses compile-time checks. If this needs to accommodate the numeric string values ('0','1','2') in addition to the named severity type, consider a union likeSeverity | stringinstead.🤖 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 property declaration severity: any weakens type safety—change the property on the incident model (the severity field) to use the existing Severity type instead of any, and if you must accept numeric string values include them in a union (e.g., Severity | '0' | '1' | '2' or Severity | string); update the severity declaration where it appears in this file to reference the Severity type defined later (Severity) rather than any so the compiler enforces valid values.web/src/components/Incidents/IncidentsPage.tsx (1)
240-241: Always fetching 15 days of data regardless of the selected time filter.Both the alerts fetch (line 241) and incidents fetch (line 306) use
15 * DAY_MSfor the fetch window. When the user selects "1 day", this fetches 15× more data than displayed. This is an intentional trade-off to compute absolutefirstTimestampfrom the full history, but it could impact performance for clusters with many incidents/alerts.Consider documenting this trade-off with a comment or exploring whether a targeted instant query for just the
firstTimestampwould be more efficient.Also applies to: 305-306
🤖 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 240 - 241, Currently both the alerts and incidents fetch call getIncidentsTimeRanges(15 * DAY_MS, currentTime) regardless of the selected time filter, which unnecessarily fetches 15× more data; change the fetch so it uses the user-selected duration (e.g., pass the selected time filter duration instead of 15 * DAY_MS when computing fetchTimeRanges for alerts and incidents) and compute firstTimestamp with a targeted instant query helper (create a new helper like fetchFirstTimestampForAlerts or fetchFirstTimestampForIncidents that runs a single instant Prometheus/DB query for the earliest timestamp matching the alert/incident selector) and use that value instead of pulling full 15-day history; if you choose not to implement the instant query now, at minimum add a clear comment next to getIncidentsTimeRanges/const fetchTimeRanges explaining the deliberate 15-day trade-off and the potential performance impact so future reviewers see why we fetch extra history.web/src/components/Incidents/processAlerts.ts (1)
247-256:Math.min(...[])returnsInfinitywhen all incidents have empty values — safe but fragile.If
mergeIncidentsByKeyreturns incidents that all have emptyvalues,timestampswill be[].Math.min(...[])→InfinityandMath.max(...[])→-Infinity. This works by accident (all alerts get filtered out), but a largetimestampsarray could also hit the call-stack limit for spread inMath.min/Math.max.Consider using a
reduceor explicit check:Proposed hardening
const timestamps = incidents.flatMap( (incident) => incident.values?.map((value) => value[0]) ?? [], ); + if (timestamps.length === 0) { + return []; + } + const incidentFirstTimestamp = Math.min(...timestamps); const incidentLastTimestamp = Math.max(...timestamps);🤖 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 247 - 256, The current use of Math.min(...timestamps)/Math.max(...timestamps) is fragile for empty or very large timestamp arrays; replace the spread calls with array reduces (e.g., reduce to compute min and max with Number.POSITIVE_INFINITY/NEGATIVE_INFINITY as initial values) and then add an early guard: if there were no timestamps (min === Infinity or max === -Infinity) handle that case (e.g., return/skip this incident group or set effectiveFirstTimestamp to nDaysStartSeconds) before computing effectiveFirstTimestamp; update the logic around timestamps, incidentFirstTimestamp, incidentLastTimestamp, and effectiveFirstTimestamp accordingly.web/src/components/Incidents/processIncidents.spec.ts (1)
806-827: Test documents thefirstTimestamp: 0placeholder — consider tracking the TODO.The test correctly asserts the current behavior where
processIncidentsForAlertssetsfirstTimestamp: 0. The comment at line 823-824 explains the intent. However, the corresponding// TODO SET MEatprocessIncidents.ts:312should be tracked to avoid shipping permanently.Would you like me to open an issue to track resolving the
TODO SET MEforfirstTimestampinprocessIncidentsForAlerts?🤖 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 806 - 827, The test highlights that processIncidentsForAlerts currently sets firstTimestamp to a placeholder 0 (see TODO SET ME in the processIncidentsForAlerts implementation) and the TODO must be tracked; either compute the real firstTimestamp from the incident values (e.g., deriving the earliest timestamp from the Prometheus values array) in processIncidentsForAlerts or create a tracking issue and replace the inline "// TODO SET ME" with a clear TODO that references that issue (include the issue ID and a short note that convertToAlerts currently computes the real value), and update the test comment accordingly so the placeholder behavior is explicitly tied to the tracking ticket.web/src/components/Incidents/utils.ts (3)
349-352: Untypedvalueparameter ingetSeverityName.Line 350's
(value)lacks a type annotation — will cause lint/TS issues if strict settings are enabled. Should be(value: string).Fix
- const getSeverityName = (value) => { + const getSeverityName = (value: string) => {🤖 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 349 - 352, The helper getSeverityName inside createIncidentsChartBars is missing a type for its parameter; update the function signature getSeverityName to accept a typed parameter (e.g., change (value) to (value: string)) and optionally add an explicit return type (string) to satisfy strict TypeScript/lint rules while keeping the same conditional logic.
988-1006:matchTimestampMetricForIncidentusesanythroughout — loses type safety.The function signature
(incident: any, timestamps: Array<any>): anydefeats TypeScript checks entirely. At minimum, type the parameters to the shapes actually used (e.g.,Pick<Incident, 'group_id' | 'src_alertname' | 'src_namespace' | 'component' | 'src_severity'>forincident, andArray<PrometheusResult>or a narrower type fortimestamps).Suggested typed signature
-export const matchTimestampMetricForIncident = (incident: any, timestamps: Array<any>): any => { +type IncidentMatchKey = Pick<Incident, 'group_id' | 'src_alertname' | 'src_namespace' | 'component' | 'src_severity'>; + +export const matchTimestampMetricForIncident = ( + incident: IncidentMatchKey, + timestamps: Array<PrometheusResult>, +): PrometheusResult | 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 988 - 1006, matchTimestampMetricForIncident currently uses any for both parameters and return which removes type safety; replace these anys by a precise typed signature: use Pick<Incident,'group_id'|'src_alertname'|'src_namespace'|'component'|'src_severity'> for the incident param (or the existing Incident type if available) and type timestamps as Array<PrometheusResult> or a narrower interface that exposes metric: { group_id: string; src_alertname: string; src_namespace: string; component?: string; src_severity: string } and make the return type the matching PrometheusResult | undefined; update the function signature and imports so TypeScript can validate the timestamp.metric property access inside matchTimestampMetricForIncident.
188-193:Math.min(...a.values.map(...))insortByEarliestTimestamphas the same spread-overflow risk.Same concern as flagged in
processIncidents.ts— spreading a largevaluesarray intoMath.mincan hit the call-stack limit.Safer alternative
- const earliestA = Math.min(...a.values.map((value) => value[0])); - const earliestB = Math.min(...b.values.map((value) => value[0])); + const earliestA = a.values.reduce((min, v) => Math.min(min, v[0]), Infinity); + const earliestB = b.values.reduce((min, v) => Math.min(min, v[0]), Infinity);🤖 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 188 - 193, The sortByEarliestTimestamp function currently computes earliestA/earliestB by using Math.min with a spread over a.values.map(...) which can overflow the call stack for large arrays; replace that spread usage with a safe min computation (e.g., iterate or use Array.prototype.reduce) to find the minimum timestamp for a.values and b.values, keeping the rest of the comparator logic in items.sort intact and updating references to earliestA and earliestB within that comparator.web/src/components/Incidents/processIncidents.ts (1)
214-216:Math.max(...array)on potentially large value arrays risks stack overflow.
Math.maxandMath.minwith spread syntax are limited by the JS engine's maximum call-stack size (typically ~65 K–125 K args). If a long-running incident accumulates many data points, this could throw aRangeError.Suggested safer alternative
- const resultLatestTimestamp = Math.max(...result.values.map((v) => v[0])); - const existingLatestTimestamp = Math.max(...existingIncident.values.map((v) => v[0])); + const resultLatestTimestamp = result.values.reduce((max, v) => Math.max(max, v[0]), -Infinity); + const existingLatestTimestamp = existingIncident.values.reduce((max, v) => Math.max(max, v[0]), -Infinity);🤖 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 214 - 216, The current computation of resultLatestTimestamp and existingLatestTimestamp uses Math.max(...array) which can cause a RangeError for very large arrays; replace those spread calls in the processIncidents logic with an O(n) safe reduction (e.g., iterate over result.values and existingIncident.values or use Array.prototype.reduce to compute the max timestamp using Number.NEGATIVE_INFINITY as the initial value) and assign the results to resultLatestTimestamp and existingLatestTimestamp, preserving the existing guard that both arrays are non-empty; ensure you reference the variables result.values, existingIncident.values, resultLatestTimestamp and existingLatestTimestamp in the update.
🤖 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 336-347: The exported function
removeTrailingPaddingFromSeveritySegments can throw when sorting because the
comparator assumes a.values[0][0] exists; update the comparator to defensively
handle incidents with missing or empty values (e.g., treat missing as +Infinity
or use 0 as a fallback) and ensure downstream code in the map also guards
against incident.values being undefined before slicing (e.g., default to [] or
check Array.isArray(incident.values) and length > 1). Modify the comparator used
in sorted = [...group].sort(...) and the mapping branch that slices
incident.values so the function never dereferences values[0][0] on
empty/undefined arrays.
---
Outside diff comments:
In `@web/src/components/Incidents/IncidentsChart/IncidentsChart.tsx`:
- Around line 112-117: createIncidentsChartBars can return [] for some groups
which makes chartBars.sort((a,b)=>a[0].x - b[0].x) and later accesses like
bars[0].group_id unsafe; filter out empty bars immediately after creating
chartBars (e.g., const chartBars = adjustedGroups.map(...).filter(b => b.length
> 0)) so the sort and subsequent map can safely use a[0].x and bars[0].group_id,
then proceed to sort by a[0].x and reassign consecutive x values as before; also
ensure any other downstream code that assumes non-empty bars (references to
bars[0]) only runs after this filtering.
In `@web/src/components/Incidents/IncidentsPage.tsx`:
- Around line 341-344: The catch block handling the incidents fetch currently
uses console.log((err)) which is inconsistent with the alerts fetch that uses
console.error; update the incidents fetch catch (the .catch((err) => {
console.log(err); }) block) to call console.error(err) instead (and remove the
eslint-disable-next-line no-console if no longer needed) so error logging is
consistent and uses the proper log level.
In `@web/src/components/Incidents/model.ts`:
- Around line 95-124: The IncidentsDetailsAlert type is missing the
lastTimestamp field used elsewhere; update the IncidentsDetailsAlert type (in
model.ts) to include lastTimestamp: number (or number | undefined if it can be
absent) so references like alertDetails.lastTimestamp in
IncidentsDetailsRowTable.tsx type-check correctly.
---
Duplicate comments:
In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx`:
- Around line 61-67: The TypeScript error is caused by the missing lastTimestamp
field on the IncidentsDetailsAlert type used by alertDetails; update the
IncidentsDetailsAlert interface/type in model.ts to include lastTimestamp
(number) so usages like the Timestamp call in IncidentsDetailsRowTable
(referencing alertDetails.lastTimestamp) compile; ensure the type aligns with
alertsEndFiring and other timestamp fields.
In `@web/src/components/Incidents/IncidentsPage.tsx`:
- Around line 232-283: The effect in useEffect that fetches alerts captures
daysSpan and incidentsLastRefreshTime but they are not listed in the dependency
array, causing a stale closure; update the dependency array for the effect (the
one using incidentForAlertProcessing, timeRanges, rules) to include daysSpan and
incidentsLastRefreshTime (and any other stable values you rely on), and remove
the eslint-disable react-hooks/exhaustive-deps so the linter can help catch
future omissions; ensure the effect body still uses incidentsLastRefreshTime as
currentTime and daysSpan when calling convertToAlerts after adding them to the
deps.
In `@web/src/components/Incidents/processAlerts.ts`:
- Around line 302-304: alertFirstTimestamp can become NaN when alert.values is
empty; change the calculation inside processAlerts (the alertFirstTimestamp
assignment) to defensively handle missing values by falling back to a safe
timestamp (e.g., use alert.values?.[0]?.[0] ?? alert.startsAt ?? Date.now() /
1000) before subtracting PROMETHEUS_QUERY_INTERVAL_SECONDS, or skip this alert
when values is empty; update the alertFirstTimestamp expression accordingly and
preserve the subtraction of PROMETHEUS_QUERY_INTERVAL_SECONDS only after using
the fallback; reference the alertFirstTimestamp variable and deduplicateAlerts
flow to ensure compatibility.
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 307-315: The returned object currently sets firstTimestamp: 0
(TODO) which yields epoch dates; update the construction inside
processIncidentsForAlerts (the block creating retval) to compute firstTimestamp
from the incident data instead of hardcoding 0 — e.g. derive the earliest
timestamp from incident.values (or fallback to an existing incident.metric.start
if available) by finding the minimum timestamp field on the values array and
using that as firstTimestamp, with a safe fallback (null or undefined) if no
timestamps exist so downstream code doesn't get Date(0).
- Around line 184-191: The code stores only one metric per severity in
severityMetrics (using severityMetrics.get(groupId).set(severityValue,
result.metric)), which causes later results with the same severityValue but
different src_* labels (e.g., different src_alertname) to overwrite earlier
ones; change the storage to preserve all contributing metrics for a severity:
when handling a result in processIncidents (variables: severityMetrics, groupId,
result, result.metric, severityValue), append result.metric to a list/array for
that severity (create the array if missing) instead of blindly calling set(), or
alternatively only set if the key is absent depending on desired semantics,
ensuring multiple metrics per severity are retained.
In `@web/src/components/Incidents/utils.spec.ts`:
- Around line 158-181: The test file has setup variables and an expect call
executing at describe-collection time for the suite named
"matchTimestampMetricForIncident", causing subsequent it blocks to be orphaned;
fix by moving the closing of the describe('matchTimestampMetricForIncident')
block so that the variable declarations, the expect(call to
matchTimestampMetricForIncident), and all the it(...) tests are nested inside
that describe, or alternatively wrap those declarations/assertion into a proper
it(...) test; adjust only the describe closure so the suite encloses everything
related to matchTimestampMetricForIncident.
In `@web/src/components/Incidents/utils.ts`:
- Around line 386-388: The startDate is being set to new
Date(incident.firstTimestamp * 1000) which yields the Unix epoch when
incident.firstTimestamp is 0; update the logic around startDate (where it's set
from incident.firstTimestamp in the mapping that produces group_id, nodata,
startDate) to only create a Date when incident.firstTimestamp is a positive
number (e.g., incident.firstTimestamp > 0) and otherwise set startDate to
null/undefined so the UI doesn't render "Jan 1, 1970"; reference the
incident.firstTimestamp and the startDate assignment in this utility to
implement the conditional.
---
Nitpick comments:
In `@web/src/components/Incidents/IncidentsPage.tsx`:
- Around line 240-241: Currently both the alerts and incidents fetch call
getIncidentsTimeRanges(15 * DAY_MS, currentTime) regardless of the selected time
filter, which unnecessarily fetches 15× more data; change the fetch so it uses
the user-selected duration (e.g., pass the selected time filter duration instead
of 15 * DAY_MS when computing fetchTimeRanges for alerts and incidents) and
compute firstTimestamp with a targeted instant query helper (create a new helper
like fetchFirstTimestampForAlerts or fetchFirstTimestampForIncidents that runs a
single instant Prometheus/DB query for the earliest timestamp matching the
alert/incident selector) and use that value instead of pulling full 15-day
history; if you choose not to implement the instant query now, at minimum add a
clear comment next to getIncidentsTimeRanges/const fetchTimeRanges explaining
the deliberate 15-day trade-off and the potential performance impact so future
reviewers see why we fetch extra history.
In `@web/src/components/Incidents/model.ts`:
- Line 18: The property declaration severity: any weakens type safety—change the
property on the incident model (the severity field) to use the existing Severity
type instead of any, and if you must accept numeric string values include them
in a union (e.g., Severity | '0' | '1' | '2' or Severity | string); update the
severity declaration where it appears in this file to reference the Severity
type defined later (Severity) rather than any so the compiler enforces valid
values.
In `@web/src/components/Incidents/processAlerts.ts`:
- Around line 247-256: The current use of
Math.min(...timestamps)/Math.max(...timestamps) is fragile for empty or very
large timestamp arrays; replace the spread calls with array reduces (e.g.,
reduce to compute min and max with Number.POSITIVE_INFINITY/NEGATIVE_INFINITY as
initial values) and then add an early guard: if there were no timestamps (min
=== Infinity or max === -Infinity) handle that case (e.g., return/skip this
incident group or set effectiveFirstTimestamp to nDaysStartSeconds) before
computing effectiveFirstTimestamp; update the logic around timestamps,
incidentFirstTimestamp, incidentLastTimestamp, and effectiveFirstTimestamp
accordingly.
In `@web/src/components/Incidents/processIncidents.spec.ts`:
- Around line 806-827: The test highlights that processIncidentsForAlerts
currently sets firstTimestamp to a placeholder 0 (see TODO SET ME in the
processIncidentsForAlerts implementation) and the TODO must be tracked; either
compute the real firstTimestamp from the incident values (e.g., deriving the
earliest timestamp from the Prometheus values array) in
processIncidentsForAlerts or create a tracking issue and replace the inline "//
TODO SET ME" with a clear TODO that references that issue (include the issue ID
and a short note that convertToAlerts currently computes the real value), and
update the test comment accordingly so the placeholder behavior is explicitly
tied to the tracking ticket.
In `@web/src/components/Incidents/processIncidents.ts`:
- Around line 214-216: The current computation of resultLatestTimestamp and
existingLatestTimestamp uses Math.max(...array) which can cause a RangeError for
very large arrays; replace those spread calls in the processIncidents logic with
an O(n) safe reduction (e.g., iterate over result.values and
existingIncident.values or use Array.prototype.reduce to compute the max
timestamp using Number.NEGATIVE_INFINITY as the initial value) and assign the
results to resultLatestTimestamp and existingLatestTimestamp, preserving the
existing guard that both arrays are non-empty; ensure you reference the
variables result.values, existingIncident.values, resultLatestTimestamp and
existingLatestTimestamp in the update.
In `@web/src/components/Incidents/utils.ts`:
- Around line 349-352: The helper getSeverityName inside
createIncidentsChartBars is missing a type for its parameter; update the
function signature getSeverityName to accept a typed parameter (e.g., change
(value) to (value: string)) and optionally add an explicit return type (string)
to satisfy strict TypeScript/lint rules while keeping the same conditional
logic.
- Around line 988-1006: matchTimestampMetricForIncident currently uses any for
both parameters and return which removes type safety; replace these anys by a
precise typed signature: use
Pick<Incident,'group_id'|'src_alertname'|'src_namespace'|'component'|'src_severity'>
for the incident param (or the existing Incident type if available) and type
timestamps as Array<PrometheusResult> or a narrower interface that exposes
metric: { group_id: string; src_alertname: string; src_namespace: string;
component?: string; src_severity: string } and make the return type the matching
PrometheusResult | undefined; update the function signature and imports so
TypeScript can validate the timestamp.metric property access inside
matchTimestampMetricForIncident.
- Around line 188-193: The sortByEarliestTimestamp function currently computes
earliestA/earliestB by using Math.min with a spread over a.values.map(...) which
can overflow the call stack for large arrays; replace that spread usage with a
safe min computation (e.g., iterate or use Array.prototype.reduce) to find the
minimum timestamp for a.values and b.values, keeping the rest of the comparator
logic in items.sort intact and updating references to earliestA and earliestB
within that comparator.
| 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]); | ||
|
|
||
| return sorted.map((incident, idx) => { | ||
| if (idx < sorted.length - 1 && incident.values.length > 1) { | ||
| return { ...incident, values: incident.values.slice(0, -1) }; | ||
| } | ||
| return incident; | ||
| }); | ||
| } |
There was a problem hiding this comment.
removeTrailingPaddingFromSeveritySegments sorts by first value but doesn't guard against empty values.
If any incident in the group has an empty values array, a.values[0][0] on line 339 will throw a TypeError. While current callers likely filter these out, this is a public export that could be called with unexpected data.
Defensive sort comparator
- const sorted = [...group].sort((a, b) => a.values[0][0] - b.values[0][0]);
+ const sorted = [...group].sort((a, b) => (a.values[0]?.[0] ?? 0) - (b.values[0]?.[0] ?? 0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export 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]); | |
| return sorted.map((incident, idx) => { | |
| if (idx < sorted.length - 1 && incident.values.length > 1) { | |
| return { ...incident, values: incident.values.slice(0, -1) }; | |
| } | |
| return incident; | |
| }); | |
| } | |
| export function removeTrailingPaddingFromSeveritySegments(group: Incident[]): Incident[] { | |
| if (group.length <= 1) return group; | |
| const sorted = [...group].sort((a, b) => (a.values[0]?.[0] ?? 0) - (b.values[0]?.[0] ?? 0)); | |
| return sorted.map((incident, idx) => { | |
| if (idx < sorted.length - 1 && incident.values.length > 1) { | |
| return { ...incident, values: incident.values.slice(0, -1) }; | |
| } | |
| return incident; | |
| }); | |
| } |
🤖 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 336 - 347, The exported
function removeTrailingPaddingFromSeveritySegments can throw when sorting
because the comparator assumes a.values[0][0] exists; update the comparator to
defensively handle incidents with missing or empty values (e.g., treat missing
as +Infinity or use 0 as a fallback) and ensure downstream code in the map also
guards against incident.values being undefined before slicing (e.g., default to
[] or check Array.isArray(incident.values) and length > 1). Modify the
comparator used in sorted = [...group].sort(...) and the mapping branch that
slices incident.values so the function never dereferences values[0][0] on
empty/undefined arrays.
7ce5c6a to
12db5ce
Compare
rioloc
force-pushed
the
feat/absolute-start-dates
branch
from
12db5ce to
04c8326
Compare
|
@rioloc: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Problem
Startdates 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
Startdate will be displayed as:Fix
The absolute start date of an incident/alert is always displayed, and it is not related to the number of selected days.
Solution
Absolute timestamps for
cluster_health_components_map{}, for incidents, andALERTS{}for alerts are retrieved by performing an instant query call to Prometheus in order to get themin_over_time(timestamp(cluster_health_components_map{})), which return the timestamp of the first datapoint for that metric. (Same for ALERTS).The result is saved into redux store and then used to match related incident/alert in order to update the Start date displayed in the tooltip.
Before
main.webm
After
feat.absolute-start-dates.webm
Summary by CodeRabbit
New Features
Improvements