[WIP] TRT-2364: Fix timestamp and date type inconsistencies - #3716
[WIP] TRT-2364: Fix timestamp and date type inconsistencies#3716mstaeble wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@mstaeble: This pull request references TRT-2364 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 story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Tip For best results, initiate chat on the files or code changes.
The actionable item here is for the linked story |
bb354da to
0ce2e75
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe PR standardizes temporal data handling across Go APIs, database queries, frontend grids, filters, release dates, and job reports. Epoch values and timezone-ambiguous dates now use typed UTC timestamps, ISO strings, ChangesTemporal data handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (17 passed)
✨ 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 (3)
sippy-ng/src/component_readiness/TriagedRegressionTestList.js (1)
215-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
type: 'date'on theLast Failurecolumn.The
valueGetternow returns aDateobject, but the column lackstype: 'date', so DataGrid sorting/filtering will not treat it as a date.♻️ Proposed change
{ field: 'last_failure', headerName: 'Last Failure', flex: 12, filterable: false, + type: 'date', valueGetter: (params) => {As per coding guidelines: "For MUI DataGrid timestamp columns, use
type: 'date'with avalueGetterthat returns aDateobject."🤖 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 `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js` around lines 215 - 235, The Last Failure column in TriagedRegressionTestList is returning a Date from its valueGetter but is missing the DataGrid date type. Update the column definition for the last_failure field to include type: 'date' so MUI DataGrid sorts and filters it as a date; keep the existing valueGetter and renderCell behavior unchanged.Source: Coding guidelines
sippy-ng/src/component_readiness/RegressedTestsPanel.js (1)
215-265: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet
type: 'date'on these timestamp columns.Both
Regressed SinceandLast Failurenow returnDateobjects fromvalueGetter, but neither column declarestype: 'date'. Per the frontend guideline, MUI DataGrid timestamp columns should usetype: 'date'with avalueGetterreturning aDateso sorting and any date filtering behave correctly.♻️ Proposed change
{ field: 'regression', headerName: 'Regressed Since', flex: 12, filterable: false, + type: 'date', valueGetter: (params) => {{ field: 'last_failure', headerName: 'Last Failure', flex: 12, filterable: false, + type: 'date', valueGetter: (params) => {As per coding guidelines: "For MUI DataGrid timestamp columns, use
type: 'date'with avalueGetterthat returns aDateobject."🤖 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 `@sippy-ng/src/component_readiness/RegressedTestsPanel.js` around lines 215 - 265, The RegressedTestsPanel DataGrid columns for Regessed Since and Last Failure return Date objects from their valueGetter functions but do not declare the column type, so update both column definitions to use type: 'date' while keeping the existing valueGetter logic in place. This applies to the column objects that define field 'regression' and field 'last_failure', so sorting and date-aware behavior work consistently with the Date values being returned.Source: Coding guidelines
pkg/api/test_analysis.go (1)
25-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAnchor the overall analysis window to
reportEnd.The grouped queries use the caller-provided
reportEnd, but the overall query still uses wall-clocktime.Now()and has no upper bound. Historical report requests can return anoverallseries for a different 14-day window than the job or variant series.Proposed fix
- Where("date >= ?", time.Now().Add(-24*14*time.Hour)). + Where("date <= ?", reportEnd). + Where("date >= ?", reportEnd.Add(-24*14*time.Hour)).🤖 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 `@pkg/api/test_analysis.go` around lines 25 - 43, The overall query in GetTestAnalysisOverallFromDB is using wall-clock time instead of the caller-provided reportEnd, so its date window can drift from the other series. Update the time filter in GetTestAnalysisOverallFromDB to anchor the 14-day lookback on reportEnd, and add an upper bound at reportEnd so the overall series matches the grouped queries’ window; use the existing query builder chain in GetTestAnalysisOverallFromDB to make the change.
🤖 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 `@sippy-ng/src/datagrid/GridToolbarFilterItem.js`:
- Around line 107-112: The date filter update in GridToolbarFilterItem’s
onChange handler is rebuilding the filter model without carrying over
props.filterModel.not, which causes negated date filters to lose their negation
state after a picker change. Update the props.setFilterModel call in this date
value change path to preserve the existing not flag from props.filterModel
alongside columnField, operatorValue, and value so the filter behavior stays
consistent.
In `@sippy-ng/src/jobs/JobsDetail.js`:
- Around line 57-58: The job details flow in JobsDetail.js uses
Temporal.PlainDate.from directly, but the frontend does not guarantee native
Temporal support. Import the Temporal polyfill in this module (or a shared entry
point used before JobsDetail runs) and ensure the existing
setStartDate/setEndDate logic uses that polyfilled Temporal reference so opening
job details works in browsers without native support.
---
Outside diff comments:
In `@pkg/api/test_analysis.go`:
- Around line 25-43: The overall query in GetTestAnalysisOverallFromDB is using
wall-clock time instead of the caller-provided reportEnd, so its date window can
drift from the other series. Update the time filter in
GetTestAnalysisOverallFromDB to anchor the 14-day lookback on reportEnd, and add
an upper bound at reportEnd so the overall series matches the grouped queries’
window; use the existing query builder chain in GetTestAnalysisOverallFromDB to
make the change.
In `@sippy-ng/src/component_readiness/RegressedTestsPanel.js`:
- Around line 215-265: The RegressedTestsPanel DataGrid columns for Regessed
Since and Last Failure return Date objects from their valueGetter functions but
do not declare the column type, so update both column definitions to use type:
'date' while keeping the existing valueGetter logic in place. This applies to
the column objects that define field 'regression' and field 'last_failure', so
sorting and date-aware behavior work consistently with the Date values being
returned.
In `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js`:
- Around line 215-235: The Last Failure column in TriagedRegressionTestList is
returning a Date from its valueGetter but is missing the DataGrid date type.
Update the column definition for the last_failure field to include type: 'date'
so MUI DataGrid sorts and filters it as a date; keep the existing valueGetter
and renderCell behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2edbb857-8aaf-4fc5-8ae1-8d17c2410c15
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (35)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/util/utils.gopkg/util/utils_test.gosippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobsDetail.js
💤 Files with no reviewable changes (1)
- sippy-ng/src/App.js
0ce2e75 to
4c44aed
Compare
openshift-ci
Bot
added
the
ready-for-human-review
4c44aed to
fddef49
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sippy-ng/src/tests/FeatureGates.js (1)
213-225: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPotential in-place mutation of the memoized default.
When the
filtersparam is absent,filterModelis the memoizeddefaultFilterModel.requestSearchmutatescurrentFilters.itemsin place (filterModel.items.filter(...)assigned back), andbookmarks[0].modelalso points atdefaultFilterModel.items(Line 127). Mutating the shared default can corrupt the bookmark/default state. Consider cloning before mutating.🤖 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 `@sippy-ng/src/tests/FeatureGates.js` around lines 213 - 225, The requestSearch helper in FeatureGates is mutating the shared memoized defaultFilterModel through filterModel/currentFilters, which can corrupt bookmark/default state because bookmarks[0].model also points at that same items array. Update requestSearch to work on a cloned filter model and cloned items before filtering/pushing the feature_gate criterion, then pass the new object to setFilterModel so defaultFilterModel is never modified in place.pkg/testidentification/ocp_variants.go (1)
110-114: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBug: existence check uses the wrong map key, dropping accumulated variant values.
variantKeyValuesis keyed by variant name (e.g.AllPlatforms()readsvariantValues["Platform"]), and the insert/create branches both key byv.VariantName. Changing the lookup tovariantKeyValues[v.VariantValue]means theokcheck almost never matches the key actually written. As a result, theelsebranch runs on every value for a given name and overwrites the set instead of inserting, so only the last value per variant name survives.The check must use the same key as the writes (
v.VariantName):🐛 Proposed fix
- if _, ok := variantKeyValues[v.VariantValue]; ok { + if _, ok := variantKeyValues[v.VariantName]; ok { variantKeyValues[v.VariantName].Insert(v.VariantValue) } else { variantKeyValues[v.VariantName] = sets.NewString(v.VariantValue) }🤖 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 `@pkg/testidentification/ocp_variants.go` around lines 110 - 114, The existence check in the variant accumulation logic uses the wrong map key, causing previously collected values to be lost. In the code that updates variantKeyValues, make sure the lookup matches the same key used for writes, namely v.VariantName, so the insert branch is taken when that variant name already exists and values are accumulated instead of overwritten.
🧹 Nitpick comments (3)
sippy-ng/src/App.js (1)
498-502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the
[isLoaded]dependency array.Dropping the dependency array makes this effect run after every render. The
if (!isLoaded)guard prevents repeated fetches, so behavior is effectively unchanged, but running the callback on every render is unnecessary and diverges from the prior intent (run whenisLoadedtransitions). Re-adding[isLoaded]is clearer and keepsreact-hooks/exhaustive-depshappy.♻️ Suggested change
useEffect(() => { if (!isLoaded) { fetchData() } - }) + }, [isLoaded])🤖 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 `@sippy-ng/src/App.js` around lines 498 - 502, Restore the missing dependency array on the useEffect in App.js so the fetch logic only runs when isLoaded changes, not after every render. Update the effect that calls fetchData to include [isLoaded], keeping the existing !isLoaded guard intact and preserving the intended transition-based behavior while satisfying react-hooks/exhaustive-deps.sippy-ng/src/tests/TestAnalysis.js (1)
63-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInline default recreates
filterModelevery render.When the
filtersparam is absent, this destructure default produces a brand-new object on each render (unlike the prior stable-reference helper). It feeds the dependency array of the page-context effect (Lines 162-170), sosetPageContextForChatwill rerun on every render. Consider wrapping the default inuseMemokeyed ontestName.🤖 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 `@sippy-ng/src/tests/TestAnalysis.js` around lines 63 - 72, The inline fallback for `filterModel` in `TestAnalysis` creates a new object on every render when `filters` is missing, which makes the `useEffect` that calls `setPageContextForChat` keep retriggering. Move that default construction into a `useMemo` in `TestAnalysis`, keyed on `testName`, and use the memoized value in the `useQueryParam('filters', SafeJSONParam)` destructure so the `filterModel` reference stays stable across renders.pkg/filter/filterable.go (1)
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
sets.StringforjobRunFields. This is a hand-rolled set; switch tok8s.io/apimachinery/pkg/util/setsandHas()for consistency with the Go guidelines.🤖 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 `@pkg/filter/filterable.go` around lines 58 - 68, The `jobRunFields` lookup in `StripJobRunFilters` is a hand-rolled set and should be converted to `k8s.io/apimachinery/pkg/util/sets.String` for consistency. Update the `jobRunFields` declaration to use a `sets.String` value, then change the membership check inside `StripJobRunFilters` to use `Has()` instead of map indexing. Keep the rest of the filter-copy logic unchanged.Source: Coding guidelines
🤖 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 `@pkg/filter/filterable.go`:
- Line 670: Update Filter.Filter in filterable.go to handle
apitype.ColumnTypeTimestamp in-memory alongside the existing string, numerical,
and array branches, since JobRun.GetFieldType("timestamp") now resolves to a
timestamp type. Add a dedicated timestamp branch that parses the filter value as
RFC3339, compares it against the item's timestamp field, and returns the same
style of match/non-match result as the other field-type handlers. Also add
regression coverage around the Filter.Filter path to verify timestamp filtering
works and no longer falls through to the “unknown field or field type” case.
In `@sippy-ng/src/component_readiness/TriagedRegressionTestList.js`:
- Around line 221-226: The `valueGetter` in `TriagedRegressionTestList` should
guard against `params.row.last_failure` being null or undefined before accessing
`.Valid`. Update the existing `valueGetter` logic to first check that
`last_failure` exists, then return null when it is missing or invalid, and only
construct the `Date` from `params.row.last_failure.Time` when the object is
present and valid.
In `@sippy-ng/src/jobs/JobAnalysis.js`:
- Around line 160-162: The fetch effect in JobAnalysis is missing props.release
in its dependency list, so navigation between releases can reuse stale data.
Update the useEffect that calls fetchData() to depend on props.release as well
as filterModel and period, and ensure fetchData uses the current release value
from props.release when building the request.
In `@sippy-ng/src/jobs/JobRunsTable.js`:
- Around line 165-172: The timestamp handling in JobRunsTable is missing a
null/invalid guard, so `valueGetter` and `renderCell` can produce `Invalid Date`
when `params.value` is absent or bad. Update the column logic in `JobRunsTable`
to match the defensive pattern used in `RegressedTestsPanel`: return `null` from
`valueGetter` when the timestamp is missing/invalid, and short-circuit
`renderCell` so it renders nothing instead of calling `toLocaleString()` or
`relativeTime()` on an invalid date.
In `@sippy-ng/src/jobs/JobTable.js`:
- Around line 549-551: JobTable does not refetch data when props.release
changes, so stale rows can remain in release-scoped views. Update the useEffect
that calls fetchData() in JobTable to include props.release in its dependency
list, ensuring the effect reruns whenever the release context changes. Keep the
fix localized to the fetchData-triggering effect and preserve the existing
period, filterModel, sort, and sortField dependencies.
In `@sippy-ng/src/pull_requests/PullRequestsTable.js`:
- Around line 371-373: The fetch effect in PullRequestsTable is missing
props.release from its dependency list, so release changes can leave stale pull
request data on screen. Update the useEffect that calls fetchData to include
props.release alongside filterModel, sort, sortField, and view so the request
reruns whenever the active release changes, especially when rendered from
RepositoryDetails.
In `@sippy-ng/src/releases/PayloadStreamTestFailures.js`:
- Around line 198-200: The fetch effect is missing the release stream query
inputs it actually depends on, so updates to release, arch, or stream can leave
PayloadStreamTestFailures showing stale results. Update the useEffect in
PayloadStreamTestFailures so its dependency array includes the values read by
fetchData(), alongside the existing filterModel, sort, and sortField
dependencies, ensuring the table refetches whenever those query params change.
In `@sippy-ng/src/releases/ReleasePayloadPullRequests.js`:
- Around line 159-161: The effect in ReleasePayloadPullRequests is missing the
server-side sort dependencies, so changing sort no longer triggers a refetch.
Update the useEffect that calls fetchData to depend on filterModel, sort, and
sortField, matching the other table components and ensuring the query rebuilt by
fetchData stays in sync with sorting changes. Keep the logic in
ReleasePayloadPullRequests aligned with PayloadStreamsTable,
ReleasePayloadTable, and ReleasePayloadJobRuns so server-side sorting continues
to work.
---
Outside diff comments:
In `@pkg/testidentification/ocp_variants.go`:
- Around line 110-114: The existence check in the variant accumulation logic
uses the wrong map key, causing previously collected values to be lost. In the
code that updates variantKeyValues, make sure the lookup matches the same key
used for writes, namely v.VariantName, so the insert branch is taken when that
variant name already exists and values are accumulated instead of overwritten.
In `@sippy-ng/src/tests/FeatureGates.js`:
- Around line 213-225: The requestSearch helper in FeatureGates is mutating the
shared memoized defaultFilterModel through filterModel/currentFilters, which can
corrupt bookmark/default state because bookmarks[0].model also points at that
same items array. Update requestSearch to work on a cloned filter model and
cloned items before filtering/pushing the feature_gate criterion, then pass the
new object to setFilterModel so defaultFilterModel is never modified in place.
---
Nitpick comments:
In `@pkg/filter/filterable.go`:
- Around line 58-68: The `jobRunFields` lookup in `StripJobRunFilters` is a
hand-rolled set and should be converted to
`k8s.io/apimachinery/pkg/util/sets.String` for consistency. Update the
`jobRunFields` declaration to use a `sets.String` value, then change the
membership check inside `StripJobRunFilters` to use `Has()` instead of map
indexing. Keep the rest of the filter-copy logic unchanged.
In `@sippy-ng/src/App.js`:
- Around line 498-502: Restore the missing dependency array on the useEffect in
App.js so the fetch logic only runs when isLoaded changes, not after every
render. Update the effect that calls fetchData to include [isLoaded], keeping
the existing !isLoaded guard intact and preserving the intended transition-based
behavior while satisfying react-hooks/exhaustive-deps.
In `@sippy-ng/src/tests/TestAnalysis.js`:
- Around line 63-72: The inline fallback for `filterModel` in `TestAnalysis`
creates a new object on every render when `filters` is missing, which makes the
`useEffect` that calls `setPageContextForChat` keep retriggering. Move that
default construction into a `useMemo` in `TestAnalysis`, keyed on `testName`,
and use the memoized value in the `useQueryParam('filters', SafeJSONParam)`
destructure so the `filterModel` reference stays stable across renders.
🪄 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: d990cb2f-d180-42be-82df-c4e55ba8c139
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (51)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlconfig/openshift.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/testidentification/ocp_variants.gopkg/util/utils.gopkg/util/utils_test.gopkg/variantregistry/snapshot.yamlsippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobAnalysis.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobTable.jssippy-ng/src/jobs/JobsDetail.jssippy-ng/src/pull_requests/PullRequestsTable.jssippy-ng/src/releases/PayloadStreamTestFailures.jssippy-ng/src/releases/PayloadStreamsTable.jssippy-ng/src/releases/PayloadTestFailures.jssippy-ng/src/releases/ReleasePayloadJobRuns.jssippy-ng/src/releases/ReleasePayloadPullRequests.jssippy-ng/src/releases/ReleasePayloadTable.jssippy-ng/src/repositories/RepositoriesTable.jssippy-ng/src/tests/FeatureGates.jssippy-ng/src/tests/TestAnalysis.jssippy-ng/src/tests/TestTable.js
|
Scheduling required tests: |
fddef49 to
2937c21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/variantregistry/snapshot.yaml`:
- Around line 26693-26695: The perfscale `multi-ns` jobs are being recorded with
`Architecture: multi`, which causes x86-only jobs to be treated as multi-arch in
downstream variant consumers. Update the snapshot generation in
`pkg/variantregistry/snapshot.go` so these `*-x86-*` entries keep `Architecture`
as `amd64`, and move the `multi-ns` distinction into a separate variant field or
the job key instead. Then regenerate the affected `snapshot.yaml` entries for
the
`periodic-ci-openshift-eng-ocp-qe-perfscale-ci-main-aws-4.22-nightly-x86-cudn-density-multi-ns-500-24nodes`
family.
🪄 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: 634f7723-be81-4a5e-b5b0-3b1a6059594c
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (38)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlconfig/openshift.yamlpkg/api/README.mdpkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/test_analysis.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/db/db.gopkg/db/views.gopkg/filter/filterable.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/testidentification/ocp_variants.gopkg/util/utils.gopkg/util/utils_test.gopkg/variantregistry/snapshot.yamlsippy-ng/src/App.jssippy-ng/src/build_clusters/BuildClusterDetails.jssippy-ng/src/component_readiness/RegressedTestsPanel.jssippy-ng/src/component_readiness/TriagedRegressionTestList.jssippy-ng/src/datagrid/GridToolbarFilterItem.jssippy-ng/src/datagrid/utils.jssippy-ng/src/helpers.jssippy-ng/src/jobs/JobRunsTable.jssippy-ng/src/jobs/JobStackedChart.jssippy-ng/src/jobs/JobsDetail.js
✅ Files skipped from review due to trivial changes (6)
- pkg/db/db.go
- pkg/api/componentreadiness/queryparamparser_test.go
- .apm/instructions/frontend.instructions.md
- .apm/instructions/backend.instructions.md
- apm.lock.yaml
- pkg/api/README.md
🚧 Files skipped from review as they are similar to previous changes (31)
- pkg/api/componentreadiness/dataprovider/bigquery/releasedates.go
- pkg/sippyserver/chat_conversations.go
- pkg/api/componentreadiness/utils/utils_test.go
- pkg/util/utils_test.go
- pkg/api/job_runs.go
- sippy-ng/src/jobs/JobStackedChart.js
- sippy-ng/src/datagrid/utils.js
- pkg/apis/sippy/v1/types.go
- sippy-ng/src/component_readiness/RegressedTestsPanel.js
- pkg/db/views.go
- sippy-ng/src/build_clusters/BuildClusterDetails.js
- sippy-ng/src/App.js
- sippy-ng/src/component_readiness/TriagedRegressionTestList.js
- pkg/testidentification/ocp_variants.go
- sippy-ng/src/jobs/JobRunsTable.js
- sippy-ng/src/datagrid/GridToolbarFilterItem.js
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
- sippy-ng/src/jobs/JobsDetail.js
- pkg/api/releases_test.go
- pkg/util/utils.go
- pkg/api/componentreadiness/triage_test.go
- pkg/sippyserver/server.go
- pkg/apis/sippyprocessing/v1/types.go
- sippy-ng/src/helpers.js
- pkg/api/test_analysis.go
- pkg/api/releases.go
- pkg/sippyserver/parameters.go
- pkg/filter/filterable.go
- pkg/apis/api/types.go
- pkg/api/jobs.go
- config/openshift.yaml
bc4cd1a to
c07f3b7
Compare
|
Scheduling required tests: |
44481e4 to
81427de
Compare
|
Scheduling required tests: |
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
mstaeble
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
81427de to
669ae14
Compare
openshift-ci
Bot
added
the
needs-rebase
669ae14 to
4a0f382
Compare
openshift-ci
Bot
removed
the
needs-rebase
openshift-ci
Bot
added
the
needs-rebase
4a0f382 to
cad8568
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@coderabbitai resume |
|
✅ Action performedReviews resumed. Full review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 44 minutes. |
cad8568 to
52cbc17
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble 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
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/backend_disruption_test.go (1)
12-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert timestamp round-trip values.
The test does not check
JobRunStartTimeorJobRunEndTimeafter unmarshalling. It passes if either field is omitted or decoded asnil. Assert the first row has the expected values. Leave the second row timestamps unset and assert that they remainnil.Proposed test update
{ BackendName: "kube-api-reused-connections", DisruptionSeconds: 0, JobName: "periodic-ci-openshift-release-master-ci-5.0-e2e-gcp-ovn-upgrade", JobRunName: "2084247445587365888", - JobRunStartTime: &startTime, - JobRunEndTime: &endTime, Cluster: "build01", ReleaseTag: "5.0.0-0.ci-2026-08-01-142300", }, @@ row := decoded.Rows[0] + if row.JobRunStartTime == nil || !row.JobRunStartTime.Equal(startTime) { + t.Errorf("JobRunStartTime = %v, want %v", row.JobRunStartTime, startTime) + } + if row.JobRunEndTime == nil || !row.JobRunEndTime.Equal(endTime) { + t.Errorf("JobRunEndTime = %v, want %v", row.JobRunEndTime, endTime) + } @@ emptyRow := decoded.Rows[1] + if emptyRow.JobRunStartTime != nil || emptyRow.JobRunEndTime != nil { + t.Errorf("expected nil job-run timestamps, got start=%v end=%v", emptyRow.JobRunStartTime, emptyRow.JobRunEndTime) + }As per coding guidelines, “New or modified functionality should include test coverage.”
🤖 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 `@pkg/api/backend_disruption_test.go` around lines 12 - 34, Update the unmarshalling assertions in the backend disruption test to verify the first row’s JobRunStartTime and JobRunEndTime match startTime and endTime, respectively. Also assert the second row’s corresponding timestamp fields remain nil, preserving its intentionally unset values.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/db/query/test_queries.go (1)
460-493: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the major SQL sections in
TestDurations.Add concise comments before the test lookup and time-window filters, variant filters, and UTC date aggregation. This query is under
pkg/db/query/, and the path instructions require comments that explain each major SQL section.Based on path instructions, SQL query-building code under
pkg/**/query/**should have inline comments explaining major query sections, including JOINs and WHERE clauses.Proposed comments
func TestDurations(dbc *db.DB, release, test string, includedVariants, excludedVariants []string) (map[civil.Date]float64, error) { type testDuration struct { Period civil.Date `json:"period"` AverageDuration float64 `json:"average_duration"` } rows := make([]testDuration, 0) results := make(map[civil.Date]float64) + // Resolve the test and restrict results to the release and 14-day lookback. testQuery := dbc.DB.Table("tests").Where("name = ?", test).Select("id") q := dbc.DB.Table("prow_job_run_tests"). Joins("JOIN tests ON prow_job_run_tests.test_id = tests.id"). Joins("JOIN prow_jobs ON prow_jobs.id = prow_job_run_tests.prow_job_id"). Where("prow_job_run_tests.prow_job_run_timestamp > current_date - interval '14' day"). Where("prow_job_run_tests.test_id = (?)", testQuery). Where("prow_job_run_tests.prow_job_run_release = ?", release) + // Apply variant inclusion and exclusion filters. for _, variant := range includedVariants { q = q.Where("prow_jobs.variant_combination_id IN (SELECT id FROM variant_combinations WHERE ? = any(variants))", variant) } for _, variant := range excludedVariants { q = q.Where("NOT EXISTS (SELECT 1 FROM variant_combinations WHERE ? = any(variants) AND id = prow_jobs.variant_combination_id)", variant) } + // Aggregate durations by UTC calendar date. res := q.🤖 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 `@pkg/db/query/test_queries.go` around lines 460 - 493, Add concise inline comments in TestDurations before the test lookup and time-window WHERE clauses, the variant inclusion and exclusion filters, and the UTC date aggregation. Include a comment identifying the JOIN section as required for this SQL query-building code, without changing query behavior.Source: Path instructions
pkg/db/db.go (1)
96-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd a regression test for the UTC session setting.
The new runtime parameter changes every PostgreSQL session created by
New. Add or verify a test that opens the connection throughNewand checkscurrent_setting('timezone')isUTC. Check one timestamp round trip as well.🤖 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 `@pkg/db/db.go` at line 96, Add a regression test for New that opens a PostgreSQL connection, verifies current_setting('timezone') returns UTC, and performs one timestamp round trip to confirm session behavior.Source: Coding guidelines
🤖 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 `@cmd/sippy/seed_data.go`:
- Line 1433: In the relationship processing flow, validate rel.GADate for nil
before the gaDate dereference; return a contextual error when it is nil, and
retain the existing date handling for non-nil values.
In `@pkg/api/releases.go`:
- Line 636: Update the release response construction around gaDateMap so the
legacy DeprecatedGADates/ga_dates field retains its existing timestamp string
representation when serialized, or omit that legacy field entirely. Do not
populate it with civil.Date values; expose date-only values only through the new
dates or release_attrs fields.
In `@pkg/apis/api/recent_test_failures.go`:
- Around line 78-80: Update RecentTestFailure.GetFieldType to classify
first_failure, last_failure, and last_pass as ColumnTypeTimestamp; implement
RecentTestFailure.GetTimestampValue to return the corresponding time.Time
values, handling nil LastPass and unknown fields appropriately. Remove these
fields’ Unix conversions from GetNumericalValue, and add unit tests covering all
three timestamps, nil LastPass, and an unknown parameter.
In `@pkg/cache/bigquerycache/bigquery.go`:
- Around line 182-186: Remove the TIMESTAMP(...) wrappers around native
timestamp parameters in the cache read queries: use `@expByNowTime` and `@expTime`
in pkg/cache/bigquerycache/bigquery.go lines 182-186, and use `@tsLower` and
`@tsUpper` in lines 219-223. Keep the existing parameter values and query behavior
unchanged.
In `@pkg/dataloader/gateststatus/loader.go`:
- Line 65: Update the release-processing logic around rel.GADate to check for
nil before dereferencing it. For a missing date, record the invalid release row
using the existing reporting mechanism and continue processing later releases;
only assign gaDate after the guard confirms rel.GADate is non-nil.
In `@pkg/db/functions.go`:
- Line 36: Update the migration or function-sync logic around the public
job_results and test_results definitions to explicitly drop the legacy overloads
job_results(text, timestamp, timestamp, timestamp) and test_results(timestamp,
timestamp, timestamp) before creating the timestamptz signatures. Retain the
existing untyped cleanup if needed, but ensure both typed drops execute before
the CREATE FUNCTION statements.
In `@pkg/filter/filterable.go`:
- Around line 211-225: Update FilterFieldToSQL’s shared numeric
timestamp-operator handling to parse f.Value with time.RFC3339Nano before
constructing provider-specific SQL, returning the existing invalid-timestamp
parameter/error path when parsing fails. Reuse the validated timestamp value for
timestamp arithmetic parameters, while preserving normal numeric handling for
non-timestamp fields.
- Around line 629-632: Update filterTimestamp so it reads the item timestamp and
evaluates OperatorIsEmpty and OperatorIsNotEmpty using value.IsZero() before
applying the blank filter.Value shortcut. Preserve the existing shortcut for
other operators, and add regression tests covering both zero and non-zero
timestamps for the empty operators.
In `@pkg/sippyserver/server.go`:
- Line 1199: Restore error handling for splitJobAndJobRunFilters in
pkg/sippyserver/server.go at lines 1199-1199 and 1716-1716: capture both return
values, return HTTP 400 when filter parsing fails, and only call
query.ListFilteredJobIDs or api.PrintJobAnalysisJSONFromDB after successful
splitting.
In `@sippy-ng/src/jobs/JobTable.jsx`:
- Around line 91-93: In sippy-ng/src/jobs/JobTable.jsx at lines 91-93 and
sippy-ng/src/releases/ReleasePayloadTable.jsx at lines 194-195, update each
related valueGetter to preserve and return the raw date string rather than
converting it to a Date object, while keeping the displayed time formatting
correct through the associated formatter. Ensure Tooltip title receives the
original string in both tables.
In `@test/integration/component_readiness_test.go`:
- Line 1299: Update the test setup around gaDate to remove the invalid gaCivil
conversion and pass gaDate directly to utils.GAWindowEnd, which accepts
civil.Date.
---
Outside diff comments:
In `@pkg/api/backend_disruption_test.go`:
- Around line 12-34: Update the unmarshalling assertions in the backend
disruption test to verify the first row’s JobRunStartTime and JobRunEndTime
match startTime and endTime, respectively. Also assert the second row’s
corresponding timestamp fields remain nil, preserving its intentionally unset
values.
---
Nitpick comments:
In `@pkg/db/db.go`:
- Line 96: Add a regression test for New that opens a PostgreSQL connection,
verifies current_setting('timezone') returns UTC, and performs one timestamp
round trip to confirm session behavior.
In `@pkg/db/query/test_queries.go`:
- Around line 460-493: Add concise inline comments in TestDurations before the
test lookup and time-window WHERE clauses, the variant inclusion and exclusion
filters, and the UTC date aggregation. Include a comment identifying the JOIN
section as required for this SQL query-building code, without changing query
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d3076157-84ef-4cb7-921d-0cc372b07dbd
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (57)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlcmd/sippy/seed_data.gopkg/api/README.mdpkg/api/backend_disruption.gopkg/api/backend_disruption_test.gopkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/jira.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/tests.gopkg/apis/api/recent_test_failures.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/apis/workloadmetrics/v1/types.gopkg/cache/bigquerycache/bigquery.gopkg/dataloader/gateststatus/loader.gopkg/dataloader/prowloader/pgwriter/pgwriter.gopkg/dataloader/prowloader/prow.gopkg/dataloader/releasedefloader/releasedefloader.gopkg/dataloader/releasedefloader/releasedefloader_test.gopkg/db/db.gopkg/db/functions.gopkg/db/models/releases.gopkg/db/query/test_queries.gopkg/filter/filterable.gopkg/flags/postgres_benchmarking_test.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/util/utils.gopkg/util/utils_test.gosippy-ng/src/App.jsxsippy-ng/src/build_clusters/BuildClusterDetails.jsxsippy-ng/src/component_readiness/RegressedTestsPanel.jsxsippy-ng/src/component_readiness/TriagePotentialMatches.jsxsippy-ng/src/component_readiness/TriagedRegressionTestList.jsxsippy-ng/src/component_readiness/TriagedRegressions.jsxsippy-ng/src/datagrid/GridToolbarFilterItem.jsxsippy-ng/src/datagrid/utils.jsxsippy-ng/src/helpers.jsxsippy-ng/src/jobs/JobRunsTable.jsxsippy-ng/src/jobs/JobStackedChart.jsxsippy-ng/src/jobs/JobTable.jsxsippy-ng/src/jobs/JobsDetail.jsxsippy-ng/src/pull_requests/PullRequestsTable.jsxsippy-ng/src/releases/ReleasePayloadTable.jsxtest/integration/component_readiness_test.go
💤 Files with no reviewable changes (2)
- pkg/dataloader/releasedefloader/releasedefloader_test.go
- sippy-ng/src/App.jsx
| valueGetter: (params) => { | ||
| return params.value ? new Date(params.value) : null | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | rg '(^|/)JobTable\.jsx$|(^|/)ReleasePayloadTable\.jsx$|package.json$|package-lock.json$|yarn.lock$|pnpm-lock.yaml$' || true
echo
echo "== JobTable relevant lines =="
sed -n '70,115p' sippy-ng/src/jobs/JobTable.jsx 2>/dev/null || true
echo
echo "== ReleasePayloadTable relevant lines =="
sed -n '180,210p' sippy-ng/src/releases/ReleasePayloadTable.jsx 2>/dev/null || true
echo
echo "== dependency versions =="
for f in package.json sippy-ng/package.json package-lock.json sippy-ng/package-lock.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
node -e "const f='$f'; const fs=require('fs'); const j=JSON.parse(fs.readFileSync(f,'utf8')||'{}'); console.log((j.dependencies||{})['`@mui/material`'], 'deps', (j.devDependencies||{})['`@mui/material`']);" || true
rg '"`@mui/material`"|"`@mui/x-data-grid`"|"`@emotion/react`"|"react-dom"|\"react-dom"' "$f" -n || true
fi
doneRepository: openshift/sippy
Length of output: 3930
🌐 Web query:
MUI Tooltip component title prop children prop React error object title
💡 Result:
The MUI Tooltip component's title prop is designed to accept a React node (React.ReactNode), which allows for strings, numbers, elements, or arrays of these types [1][2][3]. If you are encountering an error when passing an object (or any React node) to the title prop, it is often due to one of the following reasons: 1. TypeScript Type Mismatch: Historically, some versions of MUI's TypeScript definitions caused conflicts because the Tooltip's title prop name clashed with the standard HTML global title attribute (which only accepts a string) [4][5]. If you are using TypeScript, ensure you are on a recent version of MUI, as this is generally resolved by correctly extending the component's interface [4]. 2. Incorrect Property Overwriting: If you are spreading props onto the Tooltip, you might be accidentally overwriting the title prop with another value [6]. Ensure that the title prop is applied after any spread operations (e.g., <Tooltip {...props} title={myTitle} />) [6]. 3. Child Element Requirements: The Tooltip component requires its children to be valid React elements that can hold a ref [1][7][8]. If you are passing a custom component as a child, it must use React.forwardRef and correctly spread props to the underlying DOM element [9][8]. While this usually affects the child prop, failing to do so can sometimes lead to cryptic errors that may be misattributed to other props [9][8]. 4. Invalid Node Types: While title accepts most React nodes, zero-length strings, undefined, null, and false are explicitly ignored and will not render the tooltip [1][2][3]. If you are passing an object, ensure it is a valid renderable React node. To debug, verify the structure of the object you are passing. If it is a complex JavaScript object that is not a valid React node, you must transform it into a string or a React element (like
Citations:
- 1: https://next.mui.com/material-ui/api/tooltip/
- 2: https://github.com/mui-org/material-ui/blob/master/packages/mui-material/src/Tooltip/Tooltip.d.ts
- 3: https://mui.com/material-ui/api/tooltip/
- 4: Title property of Tooltip is clashing with HTML attributes API [API issue] mui/material-ui#9113
- 5: [Tooltip] Improve title node support mui/material-ui#14149
- 6: https://stackoverflow.com/questions/71645570/how-to-pass-a-custom-component-to-title-props-in-mui-tooltip
- 7: https://v7.mui.com/material-ui/api/tooltip/
- 8: https://v7.mui.com/material-ui/react-tooltip/
- 9: https://mui.com/material-ui/react-tooltip/
🌐 Web query:
React warning: Objects are not valid as a React child Date object title prop Tooltip
💡 Result:
The React error "Objects are not valid as a React child" occurs because React components can only render primitives (like strings, numbers, or booleans) or other React elements [1][2]. A JavaScript Date object is a complex object type, which React cannot render directly as text [3][1]. To resolve this when using a Date object in a tooltip title or any other component, you must convert the date into a string format [3][4]. Common solutions include: 1. Using built-in Date methods: {date.toLocaleDateString} or {date.toString} [2][4] 2. Using custom formatting: For more control, you can use Intl.DateTimeFormat or libraries like date-fns or dayjs to format the date string before passing it to the title prop [5][6]. Example: // Incorrect ... // Correct ... If you are encountering this error specifically within a library like Material UI (MUI) or Recharts, ensure that you are not passing an object to a prop that expects a string [7][8]. If you need to render complex content (like a custom component) within a tooltip title, most libraries support passing a React node (e.g., a
Citations:
- 1: Variable refers to new Date() can not be a child element. react/react#7988
- 2: https://stackoverflow.com/questions/45224856/unable-to-display-date-in-jsx-in-reactjs
- 3: https://stackoverflow.com/questions/73267394/uncaught-error-objects-are-not-valid-as-a-react-child-found-object-date-i
- 4: https://stackoverflow.com/questions/55035202/react-console-error-with-regards-to-date-format-objects-are-not-valid-as-a
- 5: https://stackoverflow.com/questions/63447723/objects-are-not-valid-as-a-react-child-found-object-with-keys-seconds-nanose
- 6: https://www.shadcn.io/examples/chart-tooltip-label-formatter
- 7: https://stackoverflow.com/questions/71645570/how-to-pass-a-custom-component-to-title-props-in-mui-tooltip
- 8: https://recharts-recharts.mintlify.app/api/components/tooltip
- 9: https://v7.mui.com/material-ui/react-tooltip/
🌐 Web query:
MUI v5 Tooltip component title Date object renders toString
💡 Result:
In Material UI (MUI) v5, the Tooltip component's title prop accepts a React Node [1][2]. When you pass a JavaScript Date object directly as the title, React attempts to render it [2]. However, because a Date object is not a valid React element on its own, React (and by extension the browser environment it runs in) will often default to calling the object's.toString method [3]. This results in the Tooltip displaying the standard, unformatted date string (e.g., "Fri Aug 07 2026 00:00:00 GMT+0000 (Coordinated Universal Time)") rather than a custom formatted date. To display a formatted date in an MUI Tooltip, you should format the Date object into a string before passing it to the title prop. You can do this using standard JavaScript methods, such as: Hover me Alternatively, if you are using a library like date-fns, dayjs, or Luxon, you should use their respective formatting functions to convert the date object into a human-readable string before providing it to the title prop. This ensures the Tooltip displays exactly the text you intend.
Citations:
- 1: https://next.mui.com/material-ui/api/tooltip/
- 2: https://github.com/mui-org/material-ui/blob/master/packages/mui-material/src/Tooltip/Tooltip.d.ts
- 3: Date filter variant's tooltip showing incorrect date to what is being filtered KevinVandy/material-react-table#1312
Keep tooltip titles as strings; pass the raw date string back to valueGetter.
Tooltip title={params.value} turns the Date object into toString() output such as Fri Aug 07 2026 00:00:00 GMT..., not the intended formatted date.
sippy-ng/src/jobs/JobTable.jsx: keep value as string and update the related time formatting if needed.sippy-ng/src/releases/ReleasePayloadTable.jsx: keep value as string and update the related time formatting if needed.
[low_effort_and_medium_reward]
📍 Affects 2 files
sippy-ng/src/jobs/JobTable.jsx#L91-L93(this comment)sippy-ng/src/releases/ReleasePayloadTable.jsx#L194-L195
🤖 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 `@sippy-ng/src/jobs/JobTable.jsx` around lines 91 - 93, In
sippy-ng/src/jobs/JobTable.jsx at lines 91-93 and
sippy-ng/src/releases/ReleasePayloadTable.jsx at lines 194-195, update each
related valueGetter to preserve and return the raw date string rather than
converting it to a Date object, while keeping the displayed time formatting
correct through the associated formatter. Ensure Tooltip title receives the
original string in both tables.
Source: Coding guidelines
Replace epoch millisecond integers with proper timestamp and date types across the database schema, API layer, and frontend. Backend: - Enforce UTC timezone on all PostgreSQL connections via pgx RuntimeParams - Change matview timestamp from bigint epoch to TIMESTAMP WITH TIME ZONE - Use civil.Date for date-only fields (GA dates, development start dates, CountByDate, jobDetailAPIResult start/end, ReleaseDefinition) - Change CalendarEvent.Start/End from string to time.Time - Change ChatConversationResponse.CreatedAt from string to time.Time - Change JobRun.Timestamp from int to time.Time (RFC 3339 in JSON) - Remove epoch extraction from SQL filters; compare timestamptz directly - Handle ColumnTypeTimestamp in Compare() via GetNumericalValue - Fix PrintJobsReportFromDB to strip job-run filters (timestamp, cluster) before querying the prow_jobs table (pre-existing bug) - Add filter.StripJobRunFilters for reusable job-run filter removal - Fix GetTestAnalysisOverallFromDB to use reportEnd instead of time.Now() for consistent date windowing with pinned time (pre-existing bug) Frontend: - Use ISO 8601 strings for timestamp filter values throughout - Remove dead ga_dates timezone hack in App.js - Use valueGetter returning Date objects for DataGrid timestamp columns - Add type: 'date' to all date/timestamp DataGrid columns - Preserve 'not' flag when updating date filter values - Rewrite JobsDetail day bucketing with Temporal.PlainDate - Update DateTimePicker and filter display to use ISO strings Documentation: - Add timestamp/date type guidelines to backend and frontend instructions - Update API docs to show RFC 3339 and YYYY-MM-DD formats Note: production databases should also have their default timezone set to UTC via ALTER DATABASE <name> SET timezone = 'UTC'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
52cbc17 to
c192dbd
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/dataloader/gateststatus/loader.go (1)
65-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both
GADatebranches.Add table-driven tests for the nil
GADatepath that recordsrelease <release> has no GA dateand continues, and for a validcivil.Datethat reachesloadRelease. Cover this in the same package before merging, as required for Go functionality changes.🤖 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 `@pkg/dataloader/gateststatus/loader.go` around lines 65 - 69, Add table-driven tests in the gateststatus package covering both branches around the GADate check: verify a nil GADate records the exact release-specific error and continues processing, and verify a valid civil.Date proceeds to loadRelease. Reuse existing test helpers or fixtures and assert the resulting errors and load behavior.Source: Coding guidelines
cmd/sippy/main.go (1)
10-16: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueKeep the
time.Local = time.UTCsetting and cover the startup contract.
time.Localis process-visible, and this project’s direct imports are unlikely to initialize beforecmd/sippy’s init. Add coverage for the global-local-time contract under a non-UTCTZso future changes do not break the UTC-only behavior.🤖 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 `@cmd/sippy/main.go` around lines 10 - 16, Keep the time.Local = time.UTC assignment in init and add a test covering the startup contract under a non-UTC TZ environment. Verify that initialization forces time.Local to UTC, preserving the UTC-only behavior even when the host timezone differs.Source: Coding guidelines
🤖 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 `@cmd/sippy/main.go`:
- Around line 10-16: Keep the time.Local = time.UTC assignment in init and add a
test covering the startup contract under a non-UTC TZ environment. Verify that
initialization forces time.Local to UTC, preserving the UTC-only behavior even
when the host timezone differs.
In `@pkg/dataloader/gateststatus/loader.go`:
- Around line 65-69: Add table-driven tests in the gateststatus package covering
both branches around the GADate check: verify a nil GADate records the exact
release-specific error and continues processing, and verify a valid civil.Date
proceeds to loadRelease. Reuse existing test helpers or fixtures and assert the
resulting errors and load behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0515e737-b227-4103-b6f6-8fc3f39dccc0
⛔ Files ignored due to path filters (8)
.claude/rules/backend.mdis excluded by!.claude/**.claude/rules/frontend.mdis excluded by!.claude/**.cursor/rules/backend.mdcis excluded by!.cursor/**.cursor/rules/frontend.mdcis excluded by!.cursor/**AGENTS.mdis excluded by!AGENTS.mdCLAUDE.mdis excluded by!CLAUDE.mdsippy-ng/AGENTS.mdis excluded by!sippy-ng/AGENTS.mdsippy-ng/CLAUDE.mdis excluded by!sippy-ng/CLAUDE.md
📒 Files selected for processing (63)
.apm/instructions/backend.instructions.md.apm/instructions/frontend.instructions.mdapm.lock.yamlcmd/sippy/main.gocmd/sippy/seed_data.gopkg/api/README.mdpkg/api/backend_disruption.gopkg/api/backend_disruption_test.gopkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/triage_test.gopkg/api/componentreadiness/utils/utils_test.gopkg/api/jira.gopkg/api/job_runs.gopkg/api/jobs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/tests.gopkg/apis/api/recent_test_failures.gopkg/apis/api/types.gopkg/apis/sippy/v1/types.gopkg/apis/sippyprocessing/v1/types.gopkg/apis/workloadmetrics/v1/types.gopkg/cache/bigquerycache/bigquery.gopkg/dataloader/gateststatus/loader.gopkg/dataloader/prowloader/pgwriter/pgwriter.gopkg/dataloader/prowloader/prow.gopkg/dataloader/releasedefloader/releasedefloader.gopkg/dataloader/releasedefloader/releasedefloader_test.gopkg/db/db.gopkg/db/functions.gopkg/db/models/releases.gopkg/db/query/test_queries.gopkg/filter/filterable.gopkg/flags/postgres_benchmarking_test.gopkg/sippyserver/chat_conversations.gopkg/sippyserver/parameters.gopkg/sippyserver/server.gopkg/util/utils.gopkg/util/utils_test.gosippy-ng/src/App.jsxsippy-ng/src/build_clusters/BuildClusterDetails.jsxsippy-ng/src/component_readiness/RegressedTestsPanel.jsxsippy-ng/src/component_readiness/TriagePotentialMatches.jsxsippy-ng/src/component_readiness/TriagedRegressionTestList.jsxsippy-ng/src/component_readiness/TriagedRegressions.jsxsippy-ng/src/datagrid/GridToolbarFilterItem.jsxsippy-ng/src/datagrid/utils.jsxsippy-ng/src/helpers.jsxsippy-ng/src/jobs/JobRunsTable.jsxsippy-ng/src/jobs/JobStackedChart.jsxsippy-ng/src/jobs/JobTable.jsxsippy-ng/src/jobs/JobsDetail.jsxsippy-ng/src/pull_requests/PullRequestsTable.jsxsippy-ng/src/releases/ReleasePayloadTable.jsxtest/integration/backfill_test.gotest/integration/component_readiness_test.gotest/integration/job_runs_report_test.gotest/integration/jobs_test.gotest/integration/pgwriter_test.gotest/integration/recent_test_failures_test.go
💤 Files with no reviewable changes (2)
- sippy-ng/src/App.jsx
- pkg/dataloader/releasedefloader/releasedefloader_test.go
🚧 Files skipped from review as they are similar to previous changes (54)
- pkg/db/db.go
- pkg/util/utils_test.go
- sippy-ng/src/jobs/JobStackedChart.jsx
- pkg/sippyserver/server.go
- pkg/api/releases_test.go
- pkg/api/componentreadiness/test_details.go
- sippy-ng/src/component_readiness/RegressedTestsPanel.jsx
- pkg/apis/workloadmetrics/v1/types.go
- sippy-ng/src/jobs/JobRunsTable.jsx
- pkg/db/models/releases.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
- pkg/sippyserver/chat_conversations.go
- sippy-ng/src/jobs/JobTable.jsx
- pkg/apis/sippy/v1/types.go
- pkg/api/componentreadiness/utils/utils_test.go
- apm.lock.yaml
- pkg/api/jira.go
- pkg/dataloader/releasedefloader/releasedefloader.go
- pkg/flags/postgres_benchmarking_test.go
- pkg/db/functions.go
- .apm/instructions/frontend.instructions.md
- sippy-ng/src/component_readiness/TriagePotentialMatches.jsx
- pkg/api/README.md
- pkg/dataloader/prowloader/prow.go
- sippy-ng/src/build_clusters/BuildClusterDetails.jsx
- pkg/api/job_runs.go
- sippy-ng/src/datagrid/GridToolbarFilterItem.jsx
- pkg/api/backend_disruption.go
- pkg/api/componentreadiness/queryparamparser_test.go
- pkg/api/backend_disruption_test.go
- pkg/api/componentreadiness/dataprovider/bigquery/releasedates.go
- pkg/sippyserver/parameters.go
- sippy-ng/src/datagrid/utils.jsx
- pkg/util/utils.go
- pkg/apis/sippyprocessing/v1/types.go
- sippy-ng/src/component_readiness/TriagedRegressionTestList.jsx
- pkg/dataloader/prowloader/pgwriter/pgwriter.go
- sippy-ng/src/helpers.jsx
- sippy-ng/src/jobs/JobsDetail.jsx
- sippy-ng/src/component_readiness/TriagedRegressions.jsx
- .apm/instructions/backend.instructions.md
- pkg/api/jobs.go
- pkg/apis/api/recent_test_failures.go
- pkg/api/componentreadiness/triage_test.go
- pkg/api/releases.go
- pkg/filter/filterable.go
- pkg/db/query/test_queries.go
- pkg/cache/bigquerycache/bigquery.go
- test/integration/component_readiness_test.go
- pkg/api/tests.go
- pkg/apis/api/types.go
- sippy-ng/src/pull_requests/PullRequestsTable.jsx
- cmd/sippy/seed_data.go
- sippy-ng/src/releases/ReleasePayloadTable.jsx
2 participants
Summary
TIMESTAMP WITH TIME ZONEandDATEtypes across the database schema, API layer, and frontendcivil.Datefor date-only fields (GA dates, development start dates, CountByDate, CalendarEvent)CalendarEvent.Start/Endfromstringtotime.TimeChatConversationResponse.CreatedAtfromstringtotime.TimeJobRun.Timestampfrominttotime.Time(RFC 3339 in JSON)/api/jobsfailed when timestamp filters were present (addedfilter.StripJobRunFilters)JobsDetailday bucketing withTemporal.PlainDateNote: Production databases should also have their default timezone set to UTC via
ALTER DATABASE <name> SET timezone = 'UTC'.Depends on (will need conflict resolution):
ReleaseDefinitionmodel date fields will needcivil.Datetest_analysis.gofunctionsTest plan
make lintpassesmake testpasses (Go + JS + MCP)make verify-apmpassesManual verification against staging-2 (prod database clone)
Ran
sippy migrateandsippy refreshagainst a clone of the production database, then served locally with--data-provider postgres.API responses verified:
/api/jobs/runs- timestamp is RFC 3339 string (was epoch ms integer)/api/releases- ga_dates are YYYY-MM-DD, dates.ga are YYYY-MM-DD, last_updated is RFC 3339/api/jobs/details- start/end are YYYY-MM-DD (civil.Date), JobRunResult.timestamp is RFC 3339/api/tests/analysis/overall- CountByDate.date is YYYY-MM-DD (civil.Date)/api/releases/tags/events- CalendarEvent.start is RFC 3339/api/incidents- CalendarEvent.start/end are RFC 3339UI pages verified:
Suitevariant error with postgres data provider, unrelated to this PR)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
YYYY-MM-DD.Bug Fixes
Documentation