From 04b47bf359fc67ef3a6cd4ca915fd4b67c19ca9c Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Thu, 21 May 2026 09:57:49 +0200 Subject: [PATCH] fix(sessions): bypass UserDraft inside session panes + sessionUnread crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After merging main's UserDraft PR (#9121) into the sessions branch, two integration issues surfaced: 1. AppEditor.svelte calls `UserDraft.use('app', path)` at the component level — keyed by ($workspaceStore, 'app', path). Sessions that haven't materialized a fork yet stay at the user's main workspace, so a session targeting an app at the same path as a regular /apps/edit tab shared the same LS key. The session would read the regular tab's autosave and write its fork-edits back over it. Gate UserDraft.use on `!getContext('aiChatManager')` — sessions inject the manager via setContext, so inside a session pane the handle is `undefined`, stateApp falls through to the `app` prop the session loaded, and the auto-save $effect bails. Same gate on the four UserDraft.remove call sites in AppEditorHeader and RawAppEditorHeader so save/deploy from a session pane doesn't wipe the LS draft of a non-session tab at the same path. 2. sessionUnread.svelte.ts called useLocalStorageValue at module scope. Main's PR added a deep-mutation $effect inside that helper, which now requires component-initialization context — every page crashed at import time with `Svelte error: effect_orphan`. Replaced with a plain module-level $state + manual localStorage persist; same reactivity contract for callers. 3. ScriptEditorView.svelte was passing a `replaceStateFn` prop that ScriptBuilder dropped on main. Removed. Verified end-to-end with Playwright: - /flows/edit/{path} regression: UserDraft handle still created, no console errors - /sessions loads, sessionUnread doesn't crash - Session targeting non-raw app `u/admin/userdraft_collision_test` displays the fork content (FORK_ONLY_MARKER) even with an LS poison at `userdraft/w/local/app/{path}` containing a POISONED_BY_REGULAR_TAB_AUTOSAVE marker; poison remains untouched after the session loads and renders Co-Authored-By: Claude Opus 4.7 --- .../components/apps/editor/AppEditor.svelte | 15 ++++-- .../apps/editor/AppEditorHeader.svelte | 13 +++-- .../raw_apps/RawAppEditorHeader.svelte | 8 ++-- .../sessions/ScriptEditorView.svelte | 1 - .../sessions/sessionUnread.svelte.ts | 48 +++++++++++++------ 5 files changed, 59 insertions(+), 26 deletions(-) diff --git a/frontend/src/lib/components/apps/editor/AppEditor.svelte b/frontend/src/lib/components/apps/editor/AppEditor.svelte index e7b55a1d72..373fb0b283 100644 --- a/frontend/src/lib/components/apps/editor/AppEditor.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditor.svelte @@ -3,7 +3,7 @@ const bubble = createBubbler() import SplitPanesWrapper from '$lib/components/splitPanes/SplitPanesWrapper.svelte' - import { onMount, setContext, untrack } from 'svelte' + import { getContext, onMount, setContext, untrack } from 'svelte' import { twMerge } from 'tailwind-merge' import { Pane, Splitpanes } from 'svelte-splitpanes' @@ -85,15 +85,23 @@ migrateApp(untrack(() => app)) + // Inside a session pane the AIChatManager is injected via context. Sessions + // have their own state machinery (sessionRuntime + per-fork backend), and + // the user-facing $workspaceStore stays on the main workspace even when + // the session is editing in a fork — so a UserDraft handle here would + // share its LS key with the regular /apps/edit route and clobber both + // sides' autosaves. Skip UserDraft entirely in that case. + const inSessionPane = !!getContext('aiChatManager') + const appDraftPath = newApp ? '' : (path ?? '') - const appDraftHandle = UserDraft.use('app', appDraftPath) + const appDraftHandle = inSessionPane ? undefined : UserDraft.use('app', appDraftPath) // Prefer the persisted autosave over the prop when both exist (e.g. // /apps/add reload: the route always initializes `app` to an empty // template, but the user's last session is sitting in LS under the // empty-path entry). The route is responsible for wiping the entry // (`UserDraft.remove`) when it wants to force a fresh start — // `?nodraft=true`, template/hub loads, etc. - const stateApp = $state(untrack(() => appDraftHandle.draft ?? app)) + const stateApp = $state(untrack(() => appDraftHandle?.draft ?? app)) const appStore = writable(stateApp) // Captured once on mount: the load-time revs are only used as the // seed meta on the very first persist of this entry. After that the @@ -113,6 +121,7 @@ let firstMirror = true $effect(() => { readFieldsRecursively(stateApp) + if (!appDraftHandle) return untrack(() => { // Resolve the meta to attach BEFORE the wipe — the wipe clears // in-memory meta and would otherwise force-seed `initialRevs` diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte index a8278778d0..8807666414 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeader.svelte @@ -172,6 +172,11 @@ const { history, jobsDrawerOpen, refreshComponents } = getContext('AppEditorContext') + // Sessions inject an AIChatManager via context; AppEditor skips its + // UserDraft handle in that case, so the cleanup calls here must skip too + // (otherwise we'd wipe a non-session tab's autosave at the same path). + const inSessionPane = !!getContext('aiChatManager') + const loading = $state({ publish: false, save: false, @@ -231,7 +236,7 @@ } closeSaveDrawer() sendUserToast('App deployed successfully') - UserDraft.remove('app', path) + if (!inSessionPane) UserDraft.remove('app', path) onSavedNewAppPath?.(path) } catch (e) { sendUserToast('Error creating app', e) @@ -331,7 +336,7 @@ closeSaveDrawer() sendUserToast('App deployed successfully') - UserDraft.remove('app', $appPath) + if (!inSessionPane) UserDraft.remove('app', $appPath) if ($appPath !== npath) { onSavedNewAppPath?.(npath) } @@ -407,7 +412,7 @@ // The initial draft was promoted to a real path on the backend — // drop the autosave keyed on the prior (possibly empty) path so // a future "+ App" click opens on a clean slate. - UserDraft.remove('app', $appPath) + if (!inSessionPane) UserDraft.remove('app', $appPath) onSavedNewAppPath?.(newEditedPath) } catch (e) { sendUserToast('Error saving initial draft', e) @@ -498,7 +503,7 @@ } sendUserToast('Draft saved') - UserDraft.remove('app', path) + if (!inSessionPane) UserDraft.remove('app', path) loading.saveDraft = false if (newApp || savedApp.draft_only) { onSavedNewAppPath?.(newEditedPath || path) diff --git a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte index 8e77e8dcda..c32b08f3c7 100644 --- a/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppEditorHeader.svelte @@ -269,7 +269,7 @@ } closeSaveDrawer() sendUserToast('App deployed successfully') - UserDraft.remove('raw_app', path) + if (!inSessionPane) UserDraft.remove('raw_app', path) dispatch('savedNewAppPath', path) } catch (e) { sendUserToast(`Error creating app: ${e.body ?? e.message}`, true) @@ -380,7 +380,7 @@ closeSaveDrawer() sendUserToast('App deployed successfully') - UserDraft.remove('raw_app', appPath) + if (!inSessionPane) UserDraft.remove('raw_app', appPath) if (appPath !== npath) { dispatch('savedNewAppPath', npath) } @@ -462,7 +462,7 @@ // The initial draft was promoted to a real path on the backend — // drop the autosave keyed on the prior (possibly empty) path so // a future "+ App" click opens on a clean slate. - UserDraft.remove('raw_app', appPath) + if (!inSessionPane) UserDraft.remove('raw_app', appPath) dispatch('savedNewAppPath', newEditedPath) } catch (e) { sendUserToast(`Error saving initial draft: ${e.body ?? e.message}`, true) @@ -563,7 +563,7 @@ } sendUserToast('Draft saved') - UserDraft.remove('raw_app', path) + if (!inSessionPane) UserDraft.remove('raw_app', path) loading.saveDraft = false if (newApp || savedApp.draft_only) { dispatch('savedNewAppPath', newEditedPath || path) diff --git a/frontend/src/lib/components/sessions/ScriptEditorView.svelte b/frontend/src/lib/components/sessions/ScriptEditorView.svelte index 942dbbb306..c39e7549ca 100644 --- a/frontend/src/lib/components/sessions/ScriptEditorView.svelte +++ b/frontend/src/lib/components/sessions/ScriptEditorView.svelte @@ -112,7 +112,6 @@ initialPath={path} fullyLoaded={!runtime.loadingScript} disableHistoryChange={true} - replaceStateFn={() => {}} {diffDrawer} {onNavigate} {initialTestPanelCollapsed} diff --git a/frontend/src/lib/components/sessions/sessionUnread.svelte.ts b/frontend/src/lib/components/sessions/sessionUnread.svelte.ts index 302103c808..aec1f9e47a 100644 --- a/frontend/src/lib/components/sessions/sessionUnread.svelte.ts +++ b/frontend/src/lib/components/sessions/sessionUnread.svelte.ts @@ -1,40 +1,60 @@ -import { useLocalStorageValue } from '$lib/svelte5Utils.svelte' import type { SessionRuntime } from './sessionRuntime.svelte' // Per-user, per-session "last seen" marker — count of displayMessages the // last time the user was actually on that session's page. Compared against // the runtime's current message count to derive an unread badge. // -// Stored as a single localStorage entry holding Record -// to avoid scattering keys across the namespace and to make -// useLocalStorageValue's reactivity cover all sessions at once. -const lastSeenStore = useLocalStorageValue>( - 'windmill_sessions_last_seen_counts', - {} -) +// Stored as a single localStorage entry holding Record. +// Module-level $state for cross-session reactivity; can't use +// `useLocalStorageValue` here because its internal $effect requires a +// component-initialization context (we run at import time). +const LS_KEY = 'windmill_sessions_last_seen_counts' + +function readInitial(): Record { + if (typeof window === 'undefined') return {} + try { + const raw = localStorage.getItem(LS_KEY) + return raw ? JSON.parse(raw) : {} + } catch { + return {} + } +} + +const lastSeen = $state<{ val: Record }>({ val: readInitial() }) + +function persist(): void { + if (typeof window === 'undefined') return + try { + localStorage.setItem(LS_KEY, JSON.stringify(lastSeen.val)) + } catch (e) { + console.error('sessionUnread: localStorage write failed', e) + } +} // Mark the session as seen up to `count` messages. No-op when already at // or past that count (idempotent — call freely from $effects). export function markSessionSeen(sessionId: string, count: number) { - const current = lastSeenStore.val[sessionId] ?? 0 + const current = lastSeen.val[sessionId] ?? 0 if (current >= count) return - lastSeenStore.val = { ...lastSeenStore.val, [sessionId]: count } + lastSeen.val = { ...lastSeen.val, [sessionId]: count } + persist() } // Drop the session entry entirely (used on delete so we don't leak // stale ids into localStorage indefinitely). export function forgetSessionSeen(sessionId: string) { - if (!(sessionId in lastSeenStore.val)) return - const next = { ...lastSeenStore.val } + if (!(sessionId in lastSeen.val)) return + const next = { ...lastSeen.val } delete next[sessionId] - lastSeenStore.val = next + lastSeen.val = next + persist() } // Number of unread messages for a session. Undefined / unloaded runtime // returns 0 — until messages are hydrated we don't know what's new. export function unreadCountFor(sessionId: string, runtime: SessionRuntime | undefined): number { if (!runtime) return 0 - const seen = lastSeenStore.val[sessionId] ?? 0 + const seen = lastSeen.val[sessionId] ?? 0 const total = runtime.manager.displayMessages.length return Math.max(0, total - seen) }