Conversation
diegoimbert
requested review from
alpetric,
hugocasa and
rubenfiszel
as code owners
|
Claude finished @diegoimbert's task in 5m 11s —— View job Review: PR #7873 — Runs page redesign
SummaryThis is a substantial UX improvement to the Runs page. The PR consolidates the date picker into a unified What looks good
Issues foundBugs:
Code quality: Performance: UX considerations: No security issues foundThe changes are purely frontend UI/UX with no new API calls, auth changes, or user input handling that would introduce security concerns. Testing instructionsNavigate to the Runs page in the Windmill app. Verify the new layout renders correctly with the runs table, duration chart, and filters visible. Test the TimeframeSelect component by clicking the calendar icon next to the refresh button — select preset timeframes (e.g., "Within last 5 minutes") and set custom from/to dates using the manual date pickers in the dropdown. Verify the URL updates with Test multi-select interactions: click a job row to select it, then Shift+click another row to select a range, Ctrl/Cmd+click to toggle individual rows, and Ctrl+A (after clicking inside the table) to select all jobs. Right-click on a selected job to see the context menu with options like "Show run details", "Go to script/flow page", "Run again", and "Cancel". Verify these actions work correctly. Test multi-select right-click: select multiple jobs, right-click, and verify the "Run again" and "Cancel" options show counts of eligible jobs. Use the Batch actions dropdown at the bottom of the table to enter cancel/rerun selection modes — verify the checkbox column appears, the "select all" checkbox works, and the exit button returns to normal mode. Test the bottom bar toggles (Schedules, Planned later, Auto-refresh) and the per-page selector. Also verify the Service Logs page ( |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| selectedIds = [] | ||
| loadingSelectedIds = true | ||
|
|
||
| const loadingToast = sendUserToast('Loading job ids', 'info') |
There was a problem hiding this comment.
Typographical note: Consider capitalizing 'IDs' in the toast message ('Loading job IDs') for consistency.
| } | ||
|
|
||
| async function onReRunSelectedJobs() { | ||
| async function onReRunSelectedJobs(batchReRunOptions: BatchReRunOptions) { |
There was a problem hiding this comment.
Lexicographical note: The function names 'onRerunAllJobsMatchingFilters' (line 401) and 'onReRunSelectedJobs' (line 420) use inconsistent capitalization for 'rerun'. Consider standardizing the naming convention.
| import Select from './select/Select.svelte' | ||
| import { goto } from '$lib/navigation' | ||
| import { page } from '$app/stores' | ||
| import { watch } from 'runed' |
There was a problem hiding this comment.
There is a potential typographical error in the new import statement. Please verify that 'runed' is the correct module name. If it's incorrect (e.g., if you intended to import from something like 'routed' or another module), please update it accordingly.
| return | ||
| } | ||
| return sendUserToast('Only scripts can be used as a input schema', true) | ||
| sendUserToast('Only scripts can be used as a input schema', true) |
There was a problem hiding this comment.
Typographical/grammatical suggestion: Consider changing the toast message from "Only scripts can be used as a input schema" to "Only scripts can be used as an input schema".
| <Button | ||
| variant="accent" | ||
| onClick={() => onConfirm(options)} | ||
| endIcon={{ icon: RefreshCwIcon }}>Run again</Button |
There was a problem hiding this comment.
There's a potential typo at the closing Button tag. It appears that the closing tag is split across two lines ("</Button" followed by a standalone ">"). Please merge these into a single properly closed tag (e.g., "") to ensure correct rendering.
| <ToggleButton | ||
| value="dependencies" | ||
| label="Deps" | ||
| showTooltipIcon |
There was a problem hiding this comment.
Typographical error in tooltip: In 'Deploying a script, flow or an app launch a dependency job that create and then attach the lockfile to the deployed item. This mechanism ensure that logic is always executed with the exact same direct and indirect dependencies.', consider updating to something like 'Deploying a script, flow or an app launches a dependency job that creates and then attaches the lockfile to the deployed item. This mechanism ensures that logic is always executed with the exact same direct and indirect dependencies.'
| } | ||
| { | ||
| label: 'Sync', | ||
| value: 'deploymentcallbacks', |
There was a problem hiding this comment.
Typographical error in tooltip: The phrase 'configured in the the workspace settings' has a duplicate 'the'. Please remove one instance.
| > | ||
| Load next {perPage} jobs | ||
| </button> | ||
| {/if}</div |
There was a problem hiding this comment.
There's a minor typographical formatting issue in the snippet footer block. The closing tag is split over two lines (line 543 shows </div and line 544 contains >), which can be confusing. It might be cleaner to merge it into a single line as </div>.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } else { | ||
| if (batchRerunOptionsIsOpen) batchRerunOptionsIsOpen = false | ||
| if ( | ||
| JSON.stringify(selectedIds) !== JSON.stringify([jobOrDate.job.id]) || |
There was a problem hiding this comment.
Using JSON.stringify for array comparison is fragile and slower than necessary. Consider a simpler check:
if (selectedIds.length !== 1 || selectedIds[0] !== jobOrDate.job.id || selectedWorkspace !== jobOrDate.job.workspace_id) {
| let selectedIdsPossibleActions = $derived.by(() => { | ||
| const cancellableJobIds: string[] = [] | ||
| const rerunnableJobIds: string[] = [] | ||
| for (const jobId of selectedIds) { | ||
| const job = flatJobs?.find( |
There was a problem hiding this comment.
Performance concern: selectedIdsPossibleActions iterates through all selectedIds and for each does a linear flatJobs?.find(...). With large selection sets (e.g., Ctrl+A on 1000+ jobs), this is O(n*m). Consider building a Map from flatJobs for O(1) lookups, or caching the lookup map when flatJobs changes.
| return true | ||
| } | ||
|
|
||
| let selectableJobs = $derived(jobs?.filter(jobIsSelectable) ?? []) |
There was a problem hiding this comment.
Bug: selectableJobs filters from jobs (the raw list), but jobIsSelectable checks rightClickPopover?.isOpen() and hoveredDropdownAction, which are transient UI states. Since this is a $derived, it re-computes reactively — but hoveredDropdownAction changes as the user hovers over menu items, which will cause the "select all" checkbox to toggle different sets of jobs while the user is just hovering. This could lead to confusing selection behavior where hovering over "Cancel" in the context menu silently changes which jobs would be selected by the "select all" checkbox in the header.
| class={twMerge( | ||
| 'flex flex-row items-center h-full w-full select-none transition-opacity', | ||
| nonSelectable || (rightClickPopover?.isOpen() && !selected) |
There was a problem hiding this comment.
The opacity dimming when the context menu is open (rightClickPopover?.isOpen() && !selected) creates a nice visual effect, but rightClickPopover?.isOpen() is a function call inside a reactive template. Since _isOpen is a $state variable in RightClickPopover, this should still trigger reactivity correctly through the component method — but it's worth verifying this works as expected in practice since method calls on component refs don't always trigger Svelte 5 fine-grained reactivity.
| function computeMinMaxInc(inc: number) { | ||
| let minTs = new Date(new Date().getTime() - inc).toISOString() | ||
| let maxTs = new Date().toISOString() | ||
| return { minTs, maxTs } |
There was a problem hiding this comment.
Minor: computeMinMaxInc computes maxTs as "now" at the time of invocation, but the dynamic timeframes call this lazily via computeMinMax(). This is correct behavior — just noting that consecutive calls within the same frame will produce slightly different maxTs values. If precision matters, consider caching now for a single computation cycle.
| url.searchParams.set('timeframe', timeframe.label) | ||
| else url.searchParams.delete('timeframe') | ||
|
|
||
| history.replaceState(null, '', url) |
There was a problem hiding this comment.
Calling history.replaceState directly bypasses SvelteKit's router. This is intentional to avoid full navigation, but be aware it won't trigger SvelteKit's beforeNavigate/afterNavigate lifecycle hooks. This seems acceptable for query-param-only updates.
| export const runsTimeframes: Timeframe[] = [ | ||
| { label: 'Latest runs', computeMinMax: () => ({ minTs: null, maxTs: null }) }, |
There was a problem hiding this comment.
Nit: The runsTimeframes and serviceLogsTimeframes arrays share most of their items. Consider extracting shared timeframe entries and composing these arrays to avoid duplication:
const sharedTimeframes = [
{ label: 'Within last 5 minutes', computeMinMax: () => computeMinMaxInc(5 * 60 * 1000) },
// ...
]
export const runsTimeframes = [
{ label: 'Latest runs', computeMinMax: () => ({ minTs: null, maxTs: null }) },
{ label: 'Within 30 seconds', computeMinMax: () => computeMinMaxInc(30 * 1000) },
{ label: 'Within last minute', computeMinMax: () => computeMinMaxInc(60 * 1000) },
...sharedTimeframes,
].map(item => ({ ...item, type: 'dynamic' }))
| if (key === 'A' && (keysPressed.Control || keysPressed.Meta)) { | ||
| if (batchRerunOptionsIsOpen) return | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| selectedIds = flatJobs |
There was a problem hiding this comment.
Ctrl+A selects all jobs in the virtual list, but flatJobs includes date separators. The filter is correct here, but note that selectedIds could end up containing IDs of jobs not visible due to virtualization. This is fine for the batch action use case, but worth documenting.
| onClick?: () => void | ||
| onHover?: (hover: boolean) => void | ||
| } | ||
| export type Props = { |
There was a problem hiding this comment.
The icon type is any — consider typing this more specifically (e.g., typeof import('lucide-svelte').Icon or Component) for better type safety and IDE support.
| } | ||
| }} | ||
| class="absolute left-0 top-0 z-[9999] w-fit" | ||
| style="transform: translate({mousePos.x + 2}px, {mousePos.y + 2}px)" |
There was a problem hiding this comment.
The popover is positioned using transform: translate(...) from the top-left corner. If the user right-clicks near the bottom or right edge of the viewport, the menu could overflow off-screen. Consider adding viewport boundary clamping logic (e.g., checking if mousePos.x + menuWidth > window.innerWidth).
| onClickOutside: (e) => { | ||
| _isOpen = false | ||
| e.preventDefault() | ||
| e.stopPropagation() |
There was a problem hiding this comment.
Calling e.preventDefault() and e.stopPropagation() on click-outside events prevents normal interactions (like clicking buttons elsewhere on the page) when the context menu is open. The user would need to click twice — once to dismiss the menu, once to actually interact. This is a common UX tradeoff but can feel unintuitive. Consider only preventing default if the click target is within the runs table area.
frontend/src/lib/utils.ts
Outdated
| export function formatDateRange( | ||
| before: string | Date | undefined, | ||
| after: string | Date | undefined |
There was a problem hiding this comment.
Bug: The parameter names before and after are semantically inverted from how the function is used. The caller passes (minTs, maxTs) — where minTs is the "from" date and maxTs is the "to" date. But the function labels the first parameter before and the second after, which reads as "before = end of range, after = start of range" — the opposite convention.
The internal logic shows this confusion: when only the first parameter is set, it renders "Before {date}" (implying an upper bound), but the caller passes minTs (a lower bound) as the first argument.
Consider renaming to from/to or minDate/maxDate to match the calling convention.
| end: string | Date | undefined | ||
| ): string { | ||
| const now = new Date() | ||
| const startDate = start ? new Date(start) : undefined |
There was a problem hiding this comment.
Consider validating the Date conversions for 'start' and 'end'. Without a check for invalid date strings, the function may output 'Invalid Date'.
| } | ||
|
|
||
| // Both dates provided | ||
| if (startDate && endDate) { |
There was a problem hiding this comment.
Add a guard for the case where the start date is later than the end date. Handling this (by swapping or throwing an error) could avoid confusing outputs.
| const startDate = start ? new Date(start) : undefined | ||
| const endDate = end ? new Date(end) : undefined | ||
|
|
||
| // Helper to format time only |
There was a problem hiding this comment.
The helper functions (formatTime, formatDateNoYear, formatDateWithYear, isSameDay, isSameYear) are redefined on each call. Consider moving them outside the function for improved performance if this function is called frequently.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
| } else { | ||
| if (batchRerunOptionsIsOpen) batchRerunOptionsIsOpen = false | ||
| if ( | ||
| selectedIds.length !== 1 || |
There was a problem hiding this comment.
Good improvement: the new checks replace JSON.stringify() with direct array length and element comparison for a single selection. This is clearer and more efficient. Note that this assumes that when a job is selected, selectedIds is always a single-element array. If multi-select logic changes later, the condition may need to be revisited.
| jobKindsCat = v | ||
| } | ||
| { | ||
| label: 'Sync', |
There was a problem hiding this comment.
Typo fix: Removed duplicate 'the' from the tooltip string.
| @@ -957,7 +868,7 @@ | |||
| value="deploymentcallbacks" | |||
There was a problem hiding this comment.
Typo fix: Removed duplicate 'the' from the tooltip for 'deploymentcallbacks'.
| label: 'Sync', | ||
| value: 'deploymentcallbacks', | ||
| tooltip: | ||
| 'Sync jobs that are triggered on every script deployment to sync the workspace with the Git repository configured in the workspace settings' |
There was a problem hiding this comment.
Typographical error: The duplicate 'the' in the tooltip text has been removed. Please double-check all similar strings for consistency.
1 participant
In future PR: Replace top filters with standard searchbar filter component
Screen.Recording.2026-02-10.at.12.13.05.mov