From cebb47ea2067a69e250651535028fc7c449f6ce0 Mon Sep 17 00:00:00 2001 From: Diego Imbert <70353967+diegoimbert@users.noreply.github.com> Date: Thu, 15 Jan 2026 08:20:04 +0100 Subject: [PATCH] Refactor runs page search params state logic (#7543) * start refactoring runs page query params state * more refactor * more migration * more refactor * per_page migrate * refactor * char consistency layout * runs filter wrong logic * fix autosetFilter * nit remove Default Email filter * Revert "nit remove Default Email filter" This reverts commit b46b3872a9cb38873f5d6dae7007afa4489f7aa4. * nit * arg nits * fix filters.arg reactivity * RunsFilter pass 1 * Refactor JobsLoader into useJobsLoader * fix lastFetchWentToEnd * update claude.md * debounce to avoid flicker * debounce * callback mistake * missing return * change to null * dead code file (SavedInputs was replaced with V2) * arg and result filter nits * better flicker prevention * avoid duplicates when loading more jobs * fix broken type * nit type * improve date filter state mgmt + encode URI component on all params --- frontend/CLAUDE.md | 69 ++ .../lib/components/ConcurrentJobsChart.svelte | 15 +- .../src/lib/components/HistoricInputs.svelte | 59 +- frontend/src/lib/components/RunChart.svelte | 29 +- frontend/src/lib/components/RunsPage.svelte | 764 ++++-------------- .../src/lib/components/SavedInputs.svelte | 460 ----------- .../components/SchemaFormWithArgPicker.svelte | 7 +- .../lib/components/ServiceLogsInner.svelte | 8 +- .../calendarPicker/CalendarPicker.svelte | 2 +- .../components/runs/ManuelDatePicker.svelte | 14 +- .../src/lib/components/runs/RunsFilter.svelte | 177 ++-- .../src/lib/components/runs/RunsQueue.svelte | 2 +- .../src/lib/components/runs/RunsTable.svelte | 2 +- ...sLoader.svelte => useJobsLoader.svelte.ts} | 384 +++++---- frontend/src/lib/svelte5Utils.svelte.ts | 55 +- frontend/src/lib/utils.ts | 2 +- .../(logged)/runs/[...path]/+page.svelte | 10 +- 17 files changed, 646 insertions(+), 1413 deletions(-) delete mode 100644 frontend/src/lib/components/SavedInputs.svelte rename frontend/src/lib/components/runs/{JobsLoader.svelte => useJobsLoader.svelte.ts} (65%) diff --git a/frontend/CLAUDE.md b/frontend/CLAUDE.md index da207ec62a..6788ac10be 100644 --- a/frontend/CLAUDE.md +++ b/frontend/CLAUDE.md @@ -7,6 +7,75 @@ - Keep components small and focused - Always use keys in {#each} blocks +## Data Flow and State Management + +### Prefer Unidirectional Data Flow with Composable State + +When you can use unidirectional data flow with composable state, prefer that over two-way binding between components. Two-way binding between components creates confusing data flow - as the codebase grows, nothing guarantees that bound state won't be updated from multiple locations, leading to bugs and maintenance issues. + +**❌ AVOID: Two-way binding when composable state would work** + +```svelte + +``` + +**✅ PREFER: Unidirectional data flow with composables** + +```typescript +// loader.svelte.ts +function useLoader(argsGetter: () => Args) { + let args = $derived(argsGetter()) + let items = $state([]) + let loading = $state(false) + + $effect(() => { + // Logic reactive to args changes + }) + + return { + get loading() { return loading }, + get items() { return items } + } +} + +// Component.svelte + +``` + +This pattern ensures: +- State responsibility is clearly owned by `useLoader` +- Data flows in one direction (parent → child) +- No ambiguity about where state can be modified +- Better maintainability as the codebase scales + +### Async Data Fetching with Runed + +For async requests, **always use `resource()` from the Runed library** instead of manual state management: + +```typescript +import { resource } from 'runed' + +let items = resource(() => args, (args) => YourService.route(args)) + +// Access loading state +items.loading + +// Access data +items.current +``` + +The `resource()` utility: +- Automatically handles loading states +- Manages async lifecycle +- Provides reactive updates when dependencies change +- Eliminates boilerplate for common async patterns + +**Key Takeaway**: Prefer unidirectional data flow with composables over two-way binding between components. Two-way binding is acceptable for simple form inputs, but avoid it when composable state patterns can provide clearer state ownership. + ## UI Guidelines ### Styling Guidelines diff --git a/frontend/src/lib/components/ConcurrentJobsChart.svelte b/frontend/src/lib/components/ConcurrentJobsChart.svelte index c401df4be6..d9d976bdb5 100644 --- a/frontend/src/lib/components/ConcurrentJobsChart.svelte +++ b/frontend/src/lib/components/ConcurrentJobsChart.svelte @@ -19,16 +19,16 @@ interface Props { extendedJobs?: ExtendedJobs | undefined maxIsNow?: boolean - minTimeSet?: string | undefined - maxTimeSet?: string | undefined + minTimeSet?: string | null + maxTimeSet?: string | null onZoom: (zoom: { min: Date; max: Date }) => void } let { extendedJobs = undefined, maxIsNow = false, - minTimeSet = undefined, - maxTimeSet = undefined, + minTimeSet = null, + maxTimeSet = null, onZoom }: Props = $props() @@ -157,8 +157,8 @@ } function computeMinMaxTime( intervals: AggregatedInterval[] | undefined, - minTimeSet: string | undefined, - maxTimeSet: string | undefined + minTimeSet: string | null, + maxTimeSet: string | null ) { let minTimeSetDate = minTimeSet ? new Date(minTimeSet) : undefined let maxTimeSetDate = maxTimeSet ? new Date(maxTimeSet) : undefined @@ -235,7 +235,8 @@ display: false }, min: minMaxTimes.min, - max: minMaxTimes.max + max: minMaxTimes.max, + ticks: { maxRotation: 0, minRotation: 0 } }, y: { grid: { diff --git a/frontend/src/lib/components/HistoricInputs.svelte b/frontend/src/lib/components/HistoricInputs.svelte index 2f3ac70672..e141400c4d 100644 --- a/frontend/src/lib/components/HistoricInputs.svelte +++ b/frontend/src/lib/components/HistoricInputs.svelte @@ -1,17 +1,17 @@ -{#if runnableId} - -{/if} -
- {#if loading} + {#if jobsLoader?.loading}
{/if} - {#if jobs?.length > 0} + {#if jobs.length > 0} @@ -144,7 +145,7 @@ on:select={(e) => handleSelected(e.detail)} /> {/each} - {#if jobs?.length == 5} + {#if jobs.length == 5} limited to 5 runs diff --git a/frontend/src/lib/components/RunChart.svelte b/frontend/src/lib/components/RunChart.svelte index 78873a3537..73216c8284 100644 --- a/frontend/src/lib/components/RunChart.svelte +++ b/frontend/src/lib/components/RunChart.svelte @@ -12,7 +12,8 @@ LinearScale, PointElement, TimeScale, - LogarithmicScale + LogarithmicScale, + type ChartOptions } from 'chart.js' import type { CompletedJob } from '$lib/gen' import { getDbClockNow } from '$lib/forLater' @@ -23,8 +24,8 @@ interface Props { jobs?: CompletedJob[] | undefined maxIsNow?: boolean - minTimeSet?: string | undefined - maxTimeSet?: string | undefined + minTimeSet?: string | null + maxTimeSet?: string | null selectedIds?: string[] canSelect?: boolean lastFetchWentToEnd?: boolean @@ -37,8 +38,8 @@ let { jobs = [], maxIsNow = false, - minTimeSet = undefined, - maxTimeSet = undefined, + minTimeSet = null, + maxTimeSet = null, selectedIds = $bindable([]), canSelect = true, lastFetchWentToEnd = false, @@ -163,8 +164,8 @@ function computeMinMaxTime( jobs: CompletedJob[] | undefined, - minTimeSet: string | undefined, - maxTimeSet: string | undefined + minTimeSet: string | null, + maxTimeSet: string | null ) { let minTimeSetDate = minTimeSet ? new Date(minTimeSet) : undefined let maxTimeSetDate = maxTimeSet ? new Date(maxTimeSet) : undefined @@ -244,7 +245,7 @@ const minMaxTime = $derived.by(() => computeMinMaxTime(jobs, minTimeSet, maxTimeSet)) - let scatterOptions = $derived({ + let scatterOptions: ChartOptions<'scatter'> = $derived({ responsive: true, maintainAspectRatio: false, plugins: { @@ -274,8 +275,9 @@ grid: { display: false }, - min: minMaxTime.minTime, - max: minMaxTime.maxTime + min: minMaxTime.minTime.getTime(), + max: minMaxTime.maxTime.getTime(), + ticks: { maxRotation: 0, minRotation: 0 } }, y: { grid: { @@ -285,11 +287,14 @@ display: true, text: 'job duration (ms)' }, - type: 'logarithmic' + type: 'logarithmic', + afterFit: function (axis) { + axis.width = Math.max(axis.width, 65) // min width to prevent layout flickering + } } }, animation: false - } as any) + }) diff --git a/frontend/src/lib/components/RunsPage.svelte b/frontend/src/lib/components/RunsPage.svelte index 5ad50f5eb0..fd9487ba1c 100644 --- a/frontend/src/lib/components/RunsPage.svelte +++ b/frontend/src/lib/components/RunsPage.svelte @@ -1,19 +1,12 @@ - - - - { reset() - loadFromQuery() }} /> @@ -971,7 +575,7 @@ { @@ -988,70 +592,52 @@
- {#if minTs || maxTs} + {#if filters.min_ts || filters.max_ts} {/if} (filters.min_ts = null)} label="From" - class={minTs || maxTs ? '' : 'relative top-0 bottom-0 left-0 right-0 h-[34px]'} - on:change={async ({ detail }) => { - minTs = new Date(detail).toISOString() - calendarChangeTimeout && clearTimeout(calendarChangeTimeout) - calendarChangeTimeout = setTimeout(() => { - jobsLoader?.loadJobs(minTs, maxTs, true) - }, 1000) - }} - on:clear={async () => { - minTs = undefined - calendarChangeTimeout && clearTimeout(calendarChangeTimeout) - calendarChangeTimeout = setTimeout(() => { - jobsLoader?.loadJobs(minTs, maxTs, true) - }, 1000) - }} + class={filters.min_ts || filters.max_ts + ? '' + : 'relative top-0 bottom-0 left-0 right-0 h-[34px]'} /> - {#if maxTs || minTs} + {#if filters.max_ts || filters.min_ts} {/if} (filters.max_ts = null)} + bind:date={filters.max_ts} label="To" - class={minTs || maxTs ? '' : 'relative top-0 bottom-0 left-0 right-0 h-[34px]'} - on:change={async ({ detail }) => { - maxTs = new Date(detail).toISOString() - calendarChangeTimeout && clearTimeout(calendarChangeTimeout) - calendarChangeTimeout = setTimeout(() => { - jobsLoader?.loadJobs(minTs, maxTs, true) - }, 1000) - }} - on:clear={async () => { - maxTs = undefined - calendarChangeTimeout && clearTimeout(calendarChangeTimeout) - calendarChangeTimeout = setTimeout(() => { - jobsLoader?.loadJobs(minTs, maxTs, true) - }, 1000) - }} + class={filters.min_ts || filters.max_ts + ? '' + : 'relative top-0 bottom-0 left-0 right-0 h-[34px]'} /> - {#if minTs || maxTs} + {#if filters.min_ts || filters.max_ts} @@ -1061,28 +647,28 @@
{ - if (e.detail == 'running' && maxTs != undefined) { - maxTs = undefined + if (e.detail == 'running' && filters.max_ts != undefined) { + filters.max_ts = null } }} {usernames} @@ -1090,7 +676,7 @@ {paths} mobile={innerWidth < verySmallScreenWidth} small={innerWidth < smallScreenWidth} - calendarSmall={!minTs && !maxTs} + calendarSmall={!filters.min_ts && !filters.max_ts} />
@@ -1164,17 +750,17 @@ {lastFetchWentToEnd} bind:selectedIds canSelect={!selectionMode} - minTimeSet={minTs} - maxTimeSet={maxTs} + minTimeSet={filters.min_ts} + maxTimeSet={filters.max_ts} totalRowsFetched={jobs?.length ?? 0} - maxIsNow={maxTs == undefined} + maxIsNow={filters.max_ts == undefined} onLoadExtra={loadExtra} jobs={completedJobs} onZoom={async (zoom) => { - minTs = zoom.min.toISOString() - maxTs = zoom.max.toISOString() + filters.min_ts = zoom.min.toISOString() + filters.max_ts = zoom.max.toISOString() manualDatePicker?.resetChoice() - jobsLoader?.loadJobs(minTs, maxTs, true) + jobsLoader?.loadJobs(true) }} onPointClicked={(ids) => { runsTable?.scrollToRun(ids) @@ -1182,14 +768,14 @@ /> {:else if graph === 'ConcurrencyChart'} { - minTs = zoom.min.toISOString() - maxTs = zoom.max.toISOString() - jobsLoader?.loadJobs(minTs, maxTs, true) + filters.min_ts = zoom.min.toISOString() + filters.max_ts = zoom.max.toISOString() + jobsLoader?.loadJobs(true) }} /> {/if} @@ -1242,17 +828,11 @@
- {#if !jobTriggerKind} + {#if !filters.job_trigger_kind}
{ - localStorage.setItem( - 'show_schedules_in_run', - showSchedules ? 'true' : 'false' - ) - }} + bind:checked={filters.show_schedules} options={tableTopBarWidth < 800 || selectionMode ? {} : { right: 'Schedules' }} @@ -1266,10 +846,7 @@
{ - localStorage.setItem('show_future_jobs', showFutureJobs ? 'true' : 'false') - }} + bind:checked={filters.show_future_jobs} id="planned-later" options={tableTopBarWidth < 800 || selectionMode ? {} @@ -1282,15 +859,14 @@
{ - lastFetchWentToEnd = false - jobsLoader?.loadJobs(minTs, maxTs, true) + jobsLoader?.loadJobs(true) }} - bind:minTs - bind:maxTs + bind:minTs={filters.min_ts} + bind:maxTs={filters.max_ts} bind:selectedManualDate {loading} bind:this={manualDatePicker} - numberOfLastJobsToFetch={perPage} + numberOfLastJobsToFetch={filters.per_page} /> {:else}
@@ -1345,13 +921,13 @@ - - {:else} - - {i.name} - - {/if} - {#if i.created_by == $userStore?.username || $userStore?.is_admin || $userStore?.is_super_admin} -
- {#if !i.isEditing} - - {/if} - - - -
- {:else} - By {i.created_by} - {/if} -
- - {/each} - {:else} -
No saved Inputs
- {/if} -
-
- - - -
- Previous runs - -
- {#if loading && (jobs == undefined || jobs?.length == 0)} -
Loading current runs...
- {:else if jobs?.length > 0} - {#each jobs as i (i.id)} - - {/each} - {#if jobs?.length == 5} -
... there may be more runs not displayed here as the limit is 5
- {/if} - {:else} -
No job currently running
- {/if} -
- -
- {#if previousInputs === undefined} - - {:else if previousInputs?.length > 0} - {#each previousInputs as i (i.id)} - - {/each} - {:else} -
No previous Runs
- {/if} -
-
- - - -
-
-
- -
-
- {#if typeof previewArgs == 'string' && previewArgs == 'WINDMILL_TOO_BIG'} -
- Payload too big to preview but can still be loaded
- {:else if Object.keys(previewArgs || {}).length > 0} -
- -
- {:else} -
- Select an Input to preview scripts arguments -
- {/if} -
-
-
- -
diff --git a/frontend/src/lib/components/SchemaFormWithArgPicker.svelte b/frontend/src/lib/components/SchemaFormWithArgPicker.svelte index 4764a26e74..14d84b5fcb 100644 --- a/frontend/src/lib/components/SchemaFormWithArgPicker.svelte +++ b/frontend/src/lib/components/SchemaFormWithArgPicker.svelte @@ -83,7 +83,6 @@ let rightPanelOpen = false let savedInputsPicker: SavedInputsPicker | undefined = undefined - let loading = false let captureTable: CaptureTable | undefined = undefined let historicInputs: HistoricInputs | undefined = undefined $: (selectedTab, (dropdownItems = getDropdownItems())) @@ -111,11 +110,13 @@
- historicInputs?.refresh()} /> + historicInputs?.refresh()} + />
minTsManual ?? null, (v) => (minTsManual = v ?? undefined)} + bind:maxTs={() => maxTsManual ?? null, (v) => (maxTsManual = v ?? undefined)} bind:this={manualPicker} {loading} on:loadJobs={() => { diff --git a/frontend/src/lib/components/common/calendarPicker/CalendarPicker.svelte b/frontend/src/lib/components/common/calendarPicker/CalendarPicker.svelte index a228e207f1..0a3a89a3a7 100644 --- a/frontend/src/lib/components/common/calendarPicker/CalendarPicker.svelte +++ b/frontend/src/lib/components/common/calendarPicker/CalendarPicker.svelte @@ -6,7 +6,7 @@ import DateTimeInput from '$lib/components/DateTimeInput.svelte' import { twMerge } from 'tailwind-merge' - export let date: string | undefined + export let date: string | null | undefined export let label: string export let useDropdown: boolean = false export let clearable: boolean = false diff --git a/frontend/src/lib/components/runs/ManuelDatePicker.svelte b/frontend/src/lib/components/runs/ManuelDatePicker.svelte index aaf0d60063..ceb393799b 100644 --- a/frontend/src/lib/components/runs/ManuelDatePicker.svelte +++ b/frontend/src/lib/components/runs/ManuelDatePicker.svelte @@ -4,8 +4,8 @@ import { createEventDispatcher } from 'svelte' interface Props { - minTs: string | undefined - maxTs: string | undefined + minTs: string | null + maxTs: string | null loading?: boolean selectedManualDate?: number loadText?: string | undefined @@ -23,7 +23,7 @@ numberOfLastJobsToFetch = 1000 }: Props = $props() - export function computeMinMax(): { minTs: string; maxTs: string | undefined } | undefined { + export function computeMinMax(): { minTs: string; maxTs: string | null } | undefined { return manualDates[selectedManualDate].computeMinMax() } @@ -33,13 +33,13 @@ function computeMinMaxInc(inc: number) { let minTs = new Date(new Date().getTime() - inc).toISOString() - let maxTs = undefined + let maxTs = null return { minTs, maxTs } } const fixedManualDates: { label: string - computeMinMax: () => { minTs: string; maxTs: string | undefined } | undefined + computeMinMax: () => { minTs: string; maxTs: string | null } | undefined }[] = [ ...(!serviceLogsChoices ? [ @@ -110,8 +110,8 @@ minTs = ts.minTs maxTs = ts.maxTs } else { - minTs = undefined - maxTs = undefined + minTs = null + maxTs = null } dispatch('loadJobs') } diff --git a/frontend/src/lib/components/runs/RunsFilter.svelte b/frontend/src/lib/components/runs/RunsFilter.svelte index 2c7ea5251b..89fc83422b 100644 --- a/frontend/src/lib/components/runs/RunsFilter.svelte +++ b/frontend/src/lib/components/runs/RunsFilter.svelte @@ -1,3 +1,37 @@ + + @@ -407,7 +450,7 @@ - - -
+ + {/snippet} diff --git a/frontend/src/lib/components/runs/RunsQueue.svelte b/frontend/src/lib/components/runs/RunsQueue.svelte index 1a7f7ff410..b0c5ed5adb 100644 --- a/frontend/src/lib/components/runs/RunsQueue.svelte +++ b/frontend/src/lib/components/runs/RunsQueue.svelte @@ -8,7 +8,7 @@ interface Props { queue_count?: Tweened | undefined suspended_count?: Tweened | undefined - success: string | undefined + success: string | null small?: boolean onJobsWaiting?: () => void onJobsSuspended?: () => void diff --git a/frontend/src/lib/components/runs/RunsTable.svelte b/frontend/src/lib/components/runs/RunsTable.svelte index acb484ee90..f9051caa0e 100644 --- a/frontend/src/lib/components/runs/RunsTable.svelte +++ b/frontend/src/lib/components/runs/RunsTable.svelte @@ -34,7 +34,7 @@ selectedIds = $bindable([]), selectedWorkspace = $bindable(undefined), activeLabel = null, - lastFetchWentToEnd = $bindable(false), + lastFetchWentToEnd = false, perPage = 1000 }: Props = $props() diff --git a/frontend/src/lib/components/runs/JobsLoader.svelte b/frontend/src/lib/components/runs/useJobsLoader.svelte.ts similarity index 65% rename from frontend/src/lib/components/runs/JobsLoader.svelte rename to frontend/src/lib/components/runs/useJobsLoader.svelte.ts index 2868390c9f..f719e2732e 100644 --- a/frontend/src/lib/components/runs/JobsLoader.svelte +++ b/frontend/src/lib/components/runs/useJobsLoader.svelte.ts @@ -1,102 +1,114 @@ - + + return { + loadExtraJobs, + loadJobs, + get queue_count() { + return queue_count + }, + get suspended_count() { + return suspended_count + }, + get loading() { + return loading + }, + get completedJobs() { + return completedJobs + }, + get externalJobs() { + return externalJobs + }, + get extendedJobs() { + return extendedJobs + }, + get jobs() { + return jobs + }, + get lastFetchWentToEnd() { + return lastFetchWentToEnd + } + } +} diff --git a/frontend/src/lib/svelte5Utils.svelte.ts b/frontend/src/lib/svelte5Utils.svelte.ts index 1b3300530f..8388a54fce 100644 --- a/frontend/src/lib/svelte5Utils.svelte.ts +++ b/frontend/src/lib/svelte5Utils.svelte.ts @@ -3,7 +3,9 @@ import { untrack } from 'svelte' import { deepEqual } from 'fast-equals' import { type StateStore } from './utils' -import { resource, type ResourceReturn } from 'runed' +import { resource, watch, type ResourceReturn } from 'runed' +import * as runed from 'runed/kit' +import type z from 'zod' export function withProps(component: Component, props: Props) { const ret = $state({ @@ -187,3 +189,54 @@ export class ChangeOnDeepInequality { return this._cached! } } + +// The original from runed has a weird behavior with dedup reads causing duplicate effect runs +// (Every field has to be derived to avoid it : https://runed.dev/docs/utilities/use-search-params) +export function useSearchParams( + schema: S, + options?: runed.SearchParamsOptions +): runed.ReturnUseSearchParams { + let params = runed.useSearchParams(schema, options) + let keys = Object.keys((schema as any).shape ?? {}) + let obj = { ...params } + for (const key of keys) { + // Somehow using $derived does not trigger reactivity sometimes ... + // (e.g: filters.arg in RunsPage.svelte updates in the URL but does not trigger reactivity) + let derivedVal = $state(params[key]) + Object.defineProperty(obj, key, { + get: () => { + if (typeof derivedVal === 'string') return decodeURIComponent(derivedVal) + return derivedVal + }, + set: (v) => { + const val = typeof v === 'string' ? encodeURIComponent(v) : v + params[key] = val + derivedVal = val + }, + enumerable: true, + configurable: true + }) + } + return obj +} + +// Prevents flickering when data is unloaded (undefined) then reloaded quickly +// But still becomes undefined if data is not reloaded within the timeout +// so the user has feedback that the data is not available anymore. +export class StaleWhileLoading { + private _current: T | undefined = $state() + private _currentTimeout: ReturnType | undefined + constructor(getter: () => T, timeout = 400) { + watch(getter, (value) => { + if (this._currentTimeout) clearTimeout(this._currentTimeout) + if (value === undefined) { + this._currentTimeout = setTimeout(() => (this._current = undefined), timeout) + } else { + this._current = value + } + }) + } + get current(): T | undefined { + return this._current + } +} diff --git a/frontend/src/lib/utils.ts b/frontend/src/lib/utils.ts index d548772fd0..b6587cdc70 100644 --- a/frontend/src/lib/utils.ts +++ b/frontend/src/lib/utils.ts @@ -146,7 +146,7 @@ export function retrieveCommonWorkerPrefix(workerName: string): string { } export function subtractDaysFromDateString( - dateString: string | undefined, + dateString: string | null, days: number ): string | undefined { if (dateString == undefined) { diff --git a/frontend/src/routes/(root)/(logged)/runs/[...path]/+page.svelte b/frontend/src/routes/(root)/(logged)/runs/[...path]/+page.svelte index 85c1d8367e..6a3bd00e27 100644 --- a/frontend/src/routes/(root)/(logged)/runs/[...path]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/runs/[...path]/+page.svelte @@ -3,13 +3,9 @@ -{#key perPage} - -{/key} +