mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-24 00:00:46 +00:00
fix(picker): stop self-feeding fetch effect that OOM'd the tab
The drill picker's $effect watched `scope` and called `ensureLoaded` on every change. `ensureLoaded` reads `loaded[kind]` synchronously (to decide whether to show a spinner), so the effect ended up subscribed to the very signal it fills. Each fetch result wrote `loaded[kind] = items`; Svelte 5's $state proxy notifies on every property set even when the reference is unchanged from cache, which refired the effect, which called `ensureLoaded` again, which awaited the cached fetch, which wrote `loaded[kind]` again... runaway loop. In `/scripts/edit/...` the picker's lifecycle stabilised quickly enough to mask the loop, but in a session pane (multiple warm sessions, picker kept alive by the surrounding state) the cycle spun freely — 29.8 million iterations in <100 ms during testing, enough to OOM Firefox / kill the Chromium tab. Two changes: - Replace the scope-watching $effect with an explicit `setScope()` helper called from `drill()`, `goUp()`, and `onMount`. Fetch is now a callback reaction to user navigation, never a reactive consequence of one. No closed feedback cycle is possible. - Untrack the `loaded[kind]` read inside `ensureLoaded`. The search $effect (which loads every kind on first keystroke) is still a reactive caller; the untrack stops it from subscribing to the signal `ensureLoaded` fills, so the same loop can't form there.
This commit is contained in:
@@ -74,8 +74,16 @@ Clicking a row drills *down*; the chevron-left in the header walks one level
|
||||
|
||||
// Sibling-popover open: melt-ui's `openFocus` runs once during the close→open
|
||||
// transition; the picker may not be mounted yet. Retry after settle.
|
||||
// Also kicks off the initial scope's fetch — drill/goUp do the same from
|
||||
// their respective branches, so `ensureLoaded` is always a callback
|
||||
// reaction to user navigation, never a reactive consequence.
|
||||
onMount(() => {
|
||||
const t = setTimeout(focus, 50)
|
||||
const initial = untrack(() => scope)
|
||||
if (initial) {
|
||||
if (initial.kind === 'all') for (const k of kinds) ensureLoaded(k)
|
||||
else ensureLoaded(initial.kind)
|
||||
}
|
||||
return () => clearTimeout(t)
|
||||
})
|
||||
|
||||
@@ -84,6 +92,22 @@ Clicking a row drills *down*; the chevron-left in the header walks one level
|
||||
let scope = $state<Scope>(untrack(() => initialScope))
|
||||
let filter = $state('')
|
||||
|
||||
/**
|
||||
* Canonical entry point for changing the picker's scope. Triggers the
|
||||
* fetch for the kind(s) the new scope needs at the same point in time.
|
||||
* Replaces the older "react to `scope` change via `$effect`" wiring,
|
||||
* which had a subtle bug: `ensureLoaded` reads `loaded[kind]`, so the
|
||||
* effect ended up subscribed to the signal it fills — every fetch
|
||||
* result re-fired it. With explicit callbacks the fetch is tied to
|
||||
* the user's action, never to a reactive consequence of that action.
|
||||
*/
|
||||
function setScope(next: Scope) {
|
||||
scope = next
|
||||
if (!next) return
|
||||
if (next.kind === 'all') for (const k of kinds) ensureLoaded(k)
|
||||
else ensureLoaded(next.kind)
|
||||
}
|
||||
|
||||
/** Tracks whether the last user action was mouse movement (true) or
|
||||
* keyboard nav (false). When false, row `mouseenter` events are ignored
|
||||
* — prevents the cursor from stealing the keyboard-driven highlight as
|
||||
@@ -114,7 +138,13 @@ Clicking a row drills *down*; the chevron-left in the header walks one level
|
||||
if (!$workspaceStore) return
|
||||
// Always re-fetch. If we have nothing cached, show a spinner; if we do,
|
||||
// keep displaying it and quietly swap to fresh data when it lands.
|
||||
if (!loaded[kind]) loadingKind[kind] = true
|
||||
// `loaded[kind]` is read inside `untrack(...)` because this function is
|
||||
// reachable from the search `$effect` below — without the untrack,
|
||||
// that effect would subscribe to the signal `ensureLoaded` fills, and
|
||||
// each `loaded[kind] = items` (proxy `set` notifies even when the ref
|
||||
// is unchanged from cache) would refire it → runaway loop. Drill
|
||||
// navigation goes through `setScope` directly so it isn't affected.
|
||||
if (!untrack(() => loaded[kind])) loadingKind[kind] = true
|
||||
try {
|
||||
const items = await loadKind($workspaceStore, kind)
|
||||
loaded[kind] = items
|
||||
@@ -143,14 +173,6 @@ Clicking a row drills *down*; the chevron-left in the header walks one level
|
||||
}))
|
||||
}
|
||||
|
||||
// Fetch the scope's kind on entry to a non-root level. The `'all'` scope
|
||||
// needs every kind loaded since it merges items across them.
|
||||
$effect(() => {
|
||||
if (!scope) return
|
||||
if (scope.kind === 'all') for (const k of kinds) ensureLoaded(k)
|
||||
else ensureLoaded(scope.kind)
|
||||
})
|
||||
|
||||
// Searching is global → load every kind.
|
||||
$effect(() => {
|
||||
if (filter.trim() !== '') for (const k of kinds) ensureLoaded(k)
|
||||
@@ -421,9 +443,9 @@ Clicking a row drills *down*; the chevron-left in the header walks one level
|
||||
|
||||
function drill(entry: Entry) {
|
||||
if (entry.type === 'kind') {
|
||||
scope = { kind: entry.kind }
|
||||
setScope({ kind: entry.kind })
|
||||
} else if (entry.type === 'dir') {
|
||||
scope = { kind: entry.kind, dir: entry.node.fullPath }
|
||||
setScope({ kind: entry.kind, dir: entry.node.fullPath })
|
||||
} else {
|
||||
pick(entry.item)
|
||||
}
|
||||
@@ -435,13 +457,13 @@ Clicking a row drills *down*; the chevron-left in the header walks one level
|
||||
// just left, so the user sees where they came from.
|
||||
if (!scope.dir) {
|
||||
const leaving = kindKey(scope.kind)
|
||||
scope = undefined
|
||||
setScope(undefined)
|
||||
highlightedKey = leaving
|
||||
return
|
||||
}
|
||||
const leaving = dirKey(scope.kind, scope.dir)
|
||||
const parent = parentDirPath(scope.dir)
|
||||
scope = parent ? { kind: scope.kind, dir: parent } : { kind: scope.kind }
|
||||
setScope(parent ? { kind: scope.kind, dir: parent } : { kind: scope.kind })
|
||||
highlightedKey = leaving
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user