SRVKP-12328: Add date range filter for PipelineRuns list page#1133
SRVKP-12328: Add date range filter for PipelineRuns list page#1133adityavshinde wants to merge 1 commit into
Conversation
Code Review by Qodo
1. Missing i18n key Time range
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adityavshinde The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
b44e712 to
24a8a9b
Compare
anwesha-palit-redhat
left a comment
There was a problem hiding this comment.
- Disable tekton-results from tektonconfig and check the behave and attach the screen recording
- The screen recording you currently added is not clear - unable to understand the flow of what you are trying to show
- Add some AI generated unit test cases for this change please
1. Screen recording after disabling tekton-results from tektonconfigRFE_without_tkn-results.mp42. Steps followed in the screen recording (description)
3. Unit testsAlready added in |
|
| const trOptions: typeof optionsMemo = useMemo(() => { | ||
| if (optionsMemo?.name) { | ||
| const { name, ...rest } = optionsMemo; | ||
| const { name, filter, ...rest } = optionsMemo; |
There was a problem hiding this comment.
this needs to be rebased with master branch
There was a problem hiding this comment.
is this rebased yet ?
There was a problem hiding this comment.
Yes, I have rebased this. Now making changes accordingly.
| placeholder?: string; | ||
| options: FilterOption[]; | ||
| defaultValues?: string[]; | ||
| singleSelect?: boolean; |
There was a problem hiding this comment.
what is the purpose of single select
There was a problem hiding this comment.
The existing DataViewFilterToolbar only supported checkbox-style filters where users can select multiple values (e.g., selecting both "Succeeded" and "Failed" statuses). But for the date range filter, selecting multiple time ranges simultaneously doesn't make sense.
|
@adityavshinde: This pull request references SRVKP-12328 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. |
| limit?: number; | ||
| name?: string; | ||
| skipFetch?: boolean; | ||
| filter?: string; |
There was a problem hiding this comment.
- Server-side filtering via CEL expression for Tekton Results data
- Client-side filtering for Kubernetes/etcd data
can you capture screen recording with network tab to verify if server side filtering is working for tekton-results api
There was a problem hiding this comment.
Server side filtering is working for tekton-results api but for K8s api we do not have server side filtering.
Screencast.From.2026-06-19.13-39-30.mp4
There was a problem hiding this comment.
also add a single line comment for what the filter means here
24a8a9b to
e698165
Compare
Signed-off-by: Aditya Shinde <adishind@redhat.com>
e698165 to
aa972d4
Compare
|
Code review by qodo was updated up to the latest commit aa972d4 |
| return { | ||
| ...rest, | ||
| filter: EQ('data.metadata.name', name), | ||
| filter: AND(EQ('data.metadata.name', name), filter), |
There was a problem hiding this comment.
what if filter is null ?
There was a problem hiding this comment.
what is the expected format for the filter
There was a problem hiding this comment.
what if
filteris null ?
AND() is a utility function in tekton-results.ts that joins CEL expressions with &&. It skips falsy values (null, undefined, empty string). So if filter is null:
AND(EQ('data.metadata.name', name), null)just returnsEQ('data.metadata.name', name)- It behaves exactly like the original code before the change:
filter: EQ('data.metadata.name', name)
There was a problem hiding this comment.
what is the expected format for the
filter
It's a CEL string for the Tekton Results API. The format looks like:
data.status.startTime > timestamp("2026-06-22T10:00:00.000Z")
This is generated by useDateRangeFilter in dateFilterCEL. It's the same format used everywhere else in the codebase for Tekton Results queries (e.g., EQ('data.metadata.name', name) produces data.metadata.name == "xyz").
There was a problem hiding this comment.
please add verification
| () => ({ | ||
| ...(selector && { selector }), | ||
| ...(options?.filter && { filter: options.filter }), | ||
| }), |
There was a problem hiding this comment.
can you break this down to if-else so that it is easier to read ?
Also describe how the flow happens from pipelinerun list page to useTaskRuns.ts - try and think about the edge cases, also how it might change in future for taskruns
| const isTektonResultEnabled = useFlag(FLAG_PIPELINE_TEKTON_RESULT_INSTALLED); | ||
| const [timespan, setTimespan, preferenceLoaded] = useUserPreference<number>( | ||
| SETTINGS_KEY, | ||
| parsePrometheusDuration('1d'), |
There was a problem hiding this comment.
are we setting 1d as default ?
There was a problem hiding this comment.
are we setting 1d as default ?
Yes
| const currentKey = formatPrometheusDuration(timespan); | ||
| const timeRangeOptions = isTektonResultEnabled | ||
| ? TimeRangeOptions() | ||
| : TimeRangeOptionsK8s(); |
There was a problem hiding this comment.
I do not think we need two separate options here as k8 anyway does not support server side rendering, the one in pipeline overview is added because of prometheus
@arvindk-softwaredev wdyt ?
| ? TimeRangeOptions() | ||
| : TimeRangeOptionsK8s(); | ||
|
|
||
| const filterValues = useMemo( |
There was a problem hiding this comment.
how much are we really saving in terms of performance with this useMemo ?
There was a problem hiding this comment.
how much are we really saving in terms of performance with this useMemo ?
Without useMemo, React creates a new filterValues object on every render, even if nothing changed. DataViewFilterToolbar sees a "new" object each time and re-renders unnecessarily.
With useMemo, React reuses the same object until baseFilterValues or currentKey actually changes. This avoids those extra re-renders. Removing it wouldn't cause any visible performance issue as the object is small.
There was a problem hiding this comment.
please explain how would this change if the values in dependency arrays were props
There was a problem hiding this comment.
I think it will work same because useMemo only compares current values with the previous one. If the parent passes new object every render then it will recompute again and again.
| return ( | ||
| <ListPageBody> | ||
| {!hideTextFilter && ( | ||
| {!hideTextFilter && preferenceLoaded && ( |
There was a problem hiding this comment.
what happens when preferenceLoaded is false ? the user sees nothing in this implementation, we should add necessary error banners / loaders
There was a problem hiding this comment.
I will try this case practically and update
There was a problem hiding this comment.
bubble the error state and then try please
There was a problem hiding this comment.
|
|
||
| export const useDateRangeFilter = (): DateRangeFilterResult => { | ||
| const isTektonResultEnabled = useFlag(FLAG_PIPELINE_TEKTON_RESULT_INSTALLED); | ||
| const [timespan, setTimespan, preferenceLoaded] = useUserPreference<number>( |
There was a problem hiding this comment.
what if this api fails ?
can we bubble the error please
There was a problem hiding this comment.
what if this api fails ? can we bubble the error please
If this api fails, then it falls back to the default value which is "Last day". This will not throw an error as it is defined in SDK
| useUserPreference, | ||
| } from '@openshift-console/dynamic-plugin-sdk'; | ||
| import { FLAG_PIPELINE_TEKTON_RESULT_INSTALLED } from '../../consts'; | ||
| import { parsePrometheusDuration } from '../pipelines-overview/dateTime'; |
There was a problem hiding this comment.
why are we using this parsePrometheusDuration ?
we are going to query tektonresults and this is very unreadable
try { const parts = duration .trim() .split(/\s+/) .map((p) => p.match(/^(\d+)([wdhms])$/)); return _.sumBy(parts, (p) => parseInt(p[1], 10) * units[p[2]]); } catch (ignored) { // Invalid duration format return 0; }
There was a problem hiding this comment.
why are we using this
parsePrometheusDuration? we are going to query tektonresults and this is very unreadabletry { const parts = duration .trim() .split(/\s+/) .map((p) => p.match(/^(\d+)([wdhms])$/)); return _.sumBy(parts, (p) => parseInt(p[1], 10) * units[p[2]]); } catch (ignored) { // Invalid duration format return 0; }
parsePrometheusDuration simply converts a human-readable duration string like '1d' into milliseconds (86400000). It's not related to Prometheus or Tekton Results, it's just a utility for parsing duration strings. We use it because:
- The time range dropdown options are stored as duration keys ('1d', '1w', '1m', etc.)
- We need milliseconds for
Date.now() - timespancalculation - The Overview page already uses this same function for the same purpose
- It's already in the codebase, so no new dependency
The name is misleading (it's borrowed from Prometheus-style duration format), but the function itself is just string to milliseconds conversion.
There was a problem hiding this comment.
@adityavshinde
This is a request
We do not want AI responses to review comment, write based on what you understand and can verify else there will be no learning from this
cc : @arvindk-softwaredev


Summary
Adds a time range filter to the PipelineRuns list page, allowing users to filter pipeline runs
by time period (Last day, Last week, Last month, Last quarter, Last year).
useUserPreference(stored in ConfigMap per user)singleSelectsupport toCheckboxFilterConfigfor reuse by other filtersChanged files
useDateRangeFilter.ts(new) - Hook managing time range state, persistence, and CEL generationuseTektonResult.ts- Accepts and forwardsfilterparameter to downstream hooksuseTaskRuns.ts- Threadsfilterto Tekton Results API, combines with name filter viaAND()DataViewFilterToolbar.tsx- AddedsingleSelectmode toCheckboxFilterInputPipelineRunsList.tsx- Integrates time range into the filter systemScreen Recording
RFE-7628.mp4
Test plan
startTime(pending) are preserved