OU-1399: pass ISO string to Timestamp component for valid date rendering#968
Conversation
String(epochMs) produces a numeric string that new Date() cannot parse, resulting in Invalid Date and "-" displayed in the Start/End columns. Convert to ISO 8601 format via toISOString() instead. Signed-off-by: Tomáš Remeš <tremes@redhat.com> Assisted-by: Claude Code:claude-opus-4-6
openshift-ci-robot
added
the
jira/valid-reference
|
@tremes: This pull request references OU-1399 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. |
WalkthroughStart and End timestamp cells in incident details and the Start cell in the incidents table now conditionally render '---' when missing and otherwise pass ISO-8601 strings (via new Date(...).toISOString()) to the Timestamp component. ChangesTimestamp Format Standardization
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Incidents/IncidentsTable.tsx (1)
94-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider UX for zero-timestamp edge case.
When
alert.alertsExpandedRowDatais empty or missing,getMinStartDatereturns0, which renders as"1970-01-01T00:00:00.000Z"(epoch start). This might be confusing for users.Consider displaying a fallback like
'---'when there's no valid start date, as already done for the resolved End timestamp in IncidentsDetailsRowTable.📅 Proposed UX improvement
const getMinStartDate = (alert: GroupedAlert): number => { if (!alert.alertsExpandedRowData || alert.alertsExpandedRowData.length === 0) { - return 0; + return -1; // Use -1 to signal "no data" instead of 0 } return Math.min(...alert.alertsExpandedRowData.map((alertData) => alertData.alertsStartFiring)); };Then update the rendering logic (lines 183-185) to check for
-1:<Td dataLabel={columnNames.startDate}> - <Timestamp - timestamp={new Date(getMinStartDate(alert) * 1000).toISOString()} - /> + {(() => { + const minStart = getMinStartDate(alert); + return minStart >= 0 ? ( + <Timestamp + timestamp={new Date(minStart * 1000).toISOString()} + /> + ) : ( + '---' + ); + })()} </Td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/Incidents/IncidentsTable.tsx` around lines 94 - 99, getMinStartDate currently returns 0 for missing/empty alert.alertsExpandedRowData which renders as the epoch; change getMinStartDate to return a sentinel (e.g., -1) for "no start date" and update the component rendering that consumes getMinStartDate (where you render the start timestamp for a GroupedAlert) to check for that sentinel and display the same fallback string '---' used for resolved End timestamps in IncidentsDetailsRowTable; adjust any types/locals (GroupedAlert, alertsExpandedRowData mapping) accordingly so callers treat -1 as "no date" instead of a real timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx`:
- Around line 56-58: The End timestamp rendering using <Timestamp timestamp={new
Date(alertDetails.alertsEndFiring * 1000).toISOString()} /> can throw if
alertsEndFiring is null/undefined/NaN; apply the same defensive check used for
the Start timestamp: only render <Timestamp> (or pass a computed ISO string)
when alertDetails.resolved is true AND alertDetails.alertsEndFiring is
non-null/undefined and a valid number (e.g., check alertDetails.alertsEndFiring
!= null && !isNaN(Number(alertDetails.alertsEndFiring))); otherwise omit the
<Timestamp> or pass undefined so toISOString() is never called. Ensure you
update the IncidentsDetailsRowTable component where Timestamp and
alertDetails.alertsEndFiring are referenced.
In `@web/src/components/Incidents/IncidentsTable.tsx`:
- Around line 183-185: getMinStartDate(alert) may return null/undefined/NaN
causing new Date(...).toISOString() to throw; before calling toISOString() in
the Timestamp component use defensive validation: call getMinStartDate(alert),
ensure it is a finite number (or create a Date and check
!isNaN(date.getTime())), and only call toISOString() when valid; otherwise pass
a safe fallback (null/undefined/empty string) into <Timestamp /> so it can
render gracefully. Reference: getMinStartDate, alert, and the Timestamp
component usage in IncidentsTable.tsx.
---
Outside diff comments:
In `@web/src/components/Incidents/IncidentsTable.tsx`:
- Around line 94-99: getMinStartDate currently returns 0 for missing/empty
alert.alertsExpandedRowData which renders as the epoch; change getMinStartDate
to return a sentinel (e.g., -1) for "no start date" and update the component
rendering that consumes getMinStartDate (where you render the start timestamp
for a GroupedAlert) to check for that sentinel and display the same fallback
string '---' used for resolved End timestamps in IncidentsDetailsRowTable;
adjust any types/locals (GroupedAlert, alertsExpandedRowData mapping)
accordingly so callers treat -1 as "no date" instead of a real timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 080984eb-e33f-47d8-b431-90169aa15923
📒 Files selected for processing (2)
web/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsTable.tsx
|
/lgtm |
openshift-ci
Bot
added
lgtm
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
/cherry-pick release-coo-ocp-4.22 |
|
@PeterYurkovich: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
QE to be completed within COO |
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
|
/lgtm |
|
/unhold |
openshift-ci
Bot
added
lgtm
Guard against null/undefined/NaN/zero timestamp values that would cause toISOString() to throw or render epoch (1970) dates. Show '---' fallback instead, consistent with unresolved End timestamps. Signed-off-by: Tomáš Remeš <tremes@redhat.com> Assisted-by: Claude Code:claude-opus-4-6
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich, tremes 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/components/Incidents/IncidentsDetailsRowTable.tsx (1)
48-54: ⚡ Quick winConsider adding finite value check for consistency.
While the truthy check catches
null,undefined,NaN, and0, it doesn't guard againstInfinityor-Infinity, which would causetoISOString()to throw aRangeError. The table file'sgetMinStartDatefunction usesNumber.isFinite()to prevent this. For consistency and robustness, consider applying the same guard here.🛡️ Proposed defensive enhancement
<Td dataLabel="expanded-details-firingstart"> - {alertDetails.alertsStartFiring ? ( + {alertDetails.alertsStartFiring && Number.isFinite(alertDetails.alertsStartFiring) ? ( <Timestamp timestamp={new Date(alertDetails.alertsStartFiring * 1000).toISOString()} /> ) : ( '---' )} </Td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx` around lines 48 - 54, The current rendering uses a truthy check on alertDetails.alertsStartFiring before calling new Date(...).toISOString(), but that can still allow Infinity/-Infinity and cause a RangeError; update the condition to mirror getMinStartDate by using Number.isFinite(alertDetails.alertsStartFiring) (or similar finite check) before passing alertDetails.alertsStartFiring to Timestamp so the Timestamp(...) call only happens for finite numeric values.web/src/components/Incidents/IncidentsTable.tsx (1)
186-192: 💤 Low valueConsider caching the computed minimum start date.
The current implementation calls
getMinStartDate(alert)twice: once for the condition check and once to compute the timestamp. While not a performance issue for small datasets, you could eliminate the duplicate call by computing it once.♻️ Optional refactor to eliminate duplicate call
<Td dataLabel={columnNames.startDate}> - {getMinStartDate(alert) > 0 ? ( + {(() => { + const minStart = getMinStartDate(alert); + return minStart > 0 ? ( <Timestamp - timestamp={new Date(getMinStartDate(alert) * 1000).toISOString()} + timestamp={new Date(minStart * 1000).toISOString()} /> - ) : ( + ) : ( '---' - )} + ); + })()} </Td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/Incidents/IncidentsTable.tsx` around lines 186 - 192, Compute getMinStartDate(alert) once and reuse it: before rendering the JSX for this row, assign const minStart = getMinStartDate(alert) (or inside the map callback/functional component scope) and then use minStart for the conditional and for creating the Timestamp (new Date(minStart * 1000).toISOString()). Update references in the IncidentsTable component so Timestamp and the '---' fallback both use this cached minStart instead of calling getMinStartDate(alert) twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/src/components/Incidents/IncidentsDetailsRowTable.tsx`:
- Around line 48-54: The current rendering uses a truthy check on
alertDetails.alertsStartFiring before calling new Date(...).toISOString(), but
that can still allow Infinity/-Infinity and cause a RangeError; update the
condition to mirror getMinStartDate by using
Number.isFinite(alertDetails.alertsStartFiring) (or similar finite check) before
passing alertDetails.alertsStartFiring to Timestamp so the Timestamp(...) call
only happens for finite numeric values.
In `@web/src/components/Incidents/IncidentsTable.tsx`:
- Around line 186-192: Compute getMinStartDate(alert) once and reuse it: before
rendering the JSX for this row, assign const minStart = getMinStartDate(alert)
(or inside the map callback/functional component scope) and then use minStart
for the conditional and for creating the Timestamp (new Date(minStart *
1000).toISOString()). Update references in the IncidentsTable component so
Timestamp and the '---' fallback both use this cached minStart instead of
calling getMinStartDate(alert) twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 805acd08-11b9-410e-bf7f-daad632d8c8e
📒 Files selected for processing (2)
web/src/components/Incidents/IncidentsDetailsRowTable.tsxweb/src/components/Incidents/IncidentsTable.tsx
|
/override ci/prow/e2e-aws-ovn |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/e2e-aws-ovn DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@PeterYurkovich: new pull request created: #969 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@tremes: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
String(epochMs) produces a numeric string that new Date() cannot parse, resulting in Invalid Date and "-" displayed in the Start/End columns. Convert to ISO 8601 format via toISOString() instead.
Assisted-by: Claude Code:claude-opus-4-6
Summary by CodeRabbit