INN 2988 add relative time filter to runs #1320
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
anafilipadealmeida
requested review from
goodoldneon and
djfarrelly
as code owners
…dd-relative-time-filter-to-runs
...(organization-active)/(dashboard)/env/[environmentSlug]/functions/[slug]/runs/TimeFilter.tsx
Show resolved
Hide resolved
...c/app/(organization-active)/(dashboard)/env/[environmentSlug]/functions/[slug]/runs/page.tsx
Outdated
Show resolved
Hide resolved
...(organization-active)/(dashboard)/env/[environmentSlug]/functions/[slug]/runs/TimeFilter.tsx
Show resolved
Hide resolved
| type Props = SelectProps & (MultiProps | SingleProps); | ||
|
|
||
| export function Select({ | ||
| defaultValue, | ||
| label, | ||
| isLabelVisible = true, | ||
| children, | ||
| onChange, | ||
| multiple, | ||
| }: SelectProps & (MultiProps | SingleProps)) { | ||
| className, | ||
| }: Props) { |
There was a problem hiding this comment.
suggestion (non-blocking): This can be for future, but it might be nice to allow Option or it's id to be a generic type that can be passed to the Select component (ex. Select<T>(...). That way there could be less type checking in components that use Select and pass an onChange handler. We might then be able to drop the if (isFunctionTimeField(option.id)) {... checking in the other components.
| // const [untilTime, setUntilTime] = useSearchParam('until'); | ||
|
|
||
| /* TODO: When we have absolute time, the start date will be either coming from the date picker or the relative time */ | ||
| const [startTime, setStartTime] = useState<Date>(new Date()); |
There was a problem hiding this comment.
suggestion (non-blocking): Probably doesn't matter with the TODO here, but will this cause issues b/c new Date() will return a new value every time this component renders? I don't know if it needs to be memo'd or we can just forget it for now until the date picker is there?
There was a problem hiding this comment.
I think startTime should be derived (and memoized, like you're suggesting) from the URL state. If the URL has something like relative=5h then startTime would be 5 hours ago. If the URL has something like startTime=2024-05-07T11:22:33Z then it would be new Date("2024-05-07T11:22:33Z").
Also, should we rename startTime? It's kinda confusing since it reads like it's related to "started at" when it's really the lower time bound for whichever time field is used (queued, started, or ended)
There was a problem hiding this comment.
@goodoldneon was making a good point about always inferring the value from the URL, even if they are the initial defaults that users didn't select. That means changing the behaviour so that the defaults in filters are now also part of the URL, and drop the need for this state. I'll make that change either with pagination or the absolute time work
anafilipadealmeida
deleted the
ana/inn-2988-add-relative-time-filter-to-runs
branch
Description
Screen.Recording.2024-05-07.at.13.21.53.mov
Motivation
Type of change (choose one)
Checklist
Check our Pull Request Guidelines