From b67183d2122f054f79dc4646ea3c4c97b10cd1c4 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 22:27:09 +0200 Subject: [PATCH] refactor: own the acting-user resolution in one composable Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YLxwAsiXJ1Au8CBDBmH7iY --- frontend/src/lib/actingUser.svelte.ts | 51 ++++++++ .../src/lib/components/ResourceEditor.svelte | 120 ++++++++---------- .../components/ResourceEditorDrawer.svelte | 24 +--- .../src/lib/components/ResourceForm.svelte | 17 ++- .../src/lib/components/VariableEditor.svelte | 53 +++----- .../src/lib/components/VariableForm.svelte | 11 +- .../schedules/ScheduleEditorInner.svelte | 22 +--- 7 files changed, 144 insertions(+), 154 deletions(-) create mode 100644 frontend/src/lib/actingUser.svelte.ts diff --git a/frontend/src/lib/actingUser.svelte.ts b/frontend/src/lib/actingUser.svelte.ts new file mode 100644 index 0000000000..52a3ee0108 --- /dev/null +++ b/frontend/src/lib/actingUser.svelte.ts @@ -0,0 +1,51 @@ +import { untrack } from 'svelte' +import { fromStore } from 'svelte/store' +import { userStore, workspaceStore, type UserExt } from '$lib/stores' +import { getUserExt } from '$lib/user' + +/** + * The user acting in a workspace that is not necessarily the one the top nav points at — an AI + * session or a workspace-specific variant acts on a workspace the nav deliberately is not on. + * + * `$userStore` is loaded for the navigation workspace and answers only for that one, so it is + * returned as-is there and costs no request. Any other workspace is asked once and cached under + * its own id; keying the cache by workspace is what makes a superseded lookup harmless, since a + * late answer can only ever land under the question it was asked. + * + * An unresolved user is `undefined`, and must never fall back to the navigation user, whose + * rights belong to another workspace — `canWrite`/`isOwner` refuse for an unknown user, which is + * the only safe answer. Callers that must not show that refusal as a denial ask `resolved` first. + */ +export function useActingUser(workspace: () => string | undefined) { + const navWorkspace = fromStore(workspaceStore) + const navUser = fromStore(userStore) + // A failed lookup is cached as `undefined` under its key, so it refuses rather than + // retrying on every read. + let others: Record = $state({}) + + $effect(() => { + const ws = workspace() + if (!ws || ws === navWorkspace.current) return + if (ws in others) return + untrack(() => { + getUserExt(ws).then((u) => (others[ws] = u)) + }) + }) + + function userIn(ws: string | undefined): UserExt | undefined { + if (!ws) return undefined + return ws === navWorkspace.current ? navUser.current : others[ws] + } + + return { + /** The acting user in `ws`, or `undefined` when it is not known. Only workspaces this + * hook has been pointed at are looked up; the rest read as unknown. */ + in: userIn, + /** Whether `ws` has an answer at all — a resolved user, or a lookup that failed. */ + resolved: (ws: string | undefined): boolean => + !!ws && (ws === navWorkspace.current || ws in others), + get current(): UserExt | undefined { + return userIn(workspace()) + } + } +} diff --git a/frontend/src/lib/components/ResourceEditor.svelte b/frontend/src/lib/components/ResourceEditor.svelte index bec8b28e4b..73315664fe 100644 --- a/frontend/src/lib/components/ResourceEditor.svelte +++ b/frontend/src/lib/components/ResourceEditor.svelte @@ -9,7 +9,7 @@ } from '$lib/gen' import { canWrite } from '$lib/utils' import { createEventDispatcher, onDestroy, untrack } from 'svelte' - import { userStore, workspaceStore } from '$lib/stores' + import { workspaceStore } from '$lib/stores' import { sendUserToast } from '$lib/toast' import { clearJsonSchemaResourceCache } from './schema/jsonSchemaResource.svelte' import ResourceForm from './ResourceForm.svelte' @@ -17,8 +17,7 @@ import { invalidateWorkspacePaths } from './PathNameAutocomplete.svelte' import Alert from './common/alert/Alert.svelte' import { resource } from 'runed' - import { getUserExt } from '$lib/user' - import type { UserExt } from '$lib/stores' + import { useActingUser } from '$lib/actingUser.svelte' import { UserDraft, draftValuesEqual, type UserDraftHandle } from '$lib/userDraft.svelte' import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte' import { setLocalDraftHint } from '$lib/localDraftHints.svelte' @@ -91,18 +90,7 @@ let initialStates: Record = $state({}) let existedInitially: Record = $state({}) let fetchedResources: Record = $state({}) - // The user acting in each loaded workspace other than the navigation one, fetched - // alongside the resource. `undefined` stands for "we don't know" — a lookup still in - // flight or one that failed. Read through `actingUserIn`, never directly. - let perWsUser: Record = $state({}) - - /** The user acting in `ws`. `$userStore` is loaded for the navigation workspace and - * answers only for that one; anywhere else the lookup above answers, and `undefined` - * must never borrow the navigation user's rights — `canWrite` refuses for it. */ - function actingUserIn(ws: string | undefined): UserExt | undefined { - if (!ws) return undefined - return ws === $workspaceStore ? $userStore : perWsUser[ws] - } + const acting = useActingUser(() => selected) const handlesArray = UserDraft.useMany(() => workspaceSpecs.map((s) => ({ @@ -237,7 +225,7 @@ () => deployedPath, () => deployedUrl, () => resource_type, - () => actingUserIn(selected)?.is_admin + () => acting.in(selected)?.is_admin ], async ([ws, path, _url, type, admin]) => ws && path && type === 'git_repository' && admin @@ -254,14 +242,15 @@ let resourceToEdit: Resource | undefined = $derived( selected ? fetchedResources[selected] : undefined ) - let can_write = $derived.by(() => { - // A resource that does not exist yet has nobody's permissions on it. In edit mode the - // resource and the acting user land together, so a missing one is also a missing - // other, and neither may read as writable. + // `undefined` until both the resource and the acting user have landed — a pending verdict + // is neither a grant nor the denial the read-only alert announces, so the two must stay + // distinguishable. + let can_write: boolean | undefined = $derived.by(() => { + // A resource that does not exist yet has nobody's permissions on it. if (!initialPath || !selected) return true const r = fetchedResources[selected] - if (!r) return false - return canWrite(current?.path ?? initialPath, r.extra_perms ?? {}, actingUserIn(selected)) + if (!r || !acting.resolved(selected)) return undefined + return canWrite(current?.path ?? initialPath, r.extra_perms ?? {}, acting.in(selected)) }) const dirtyWorkspaces = $derived( @@ -298,8 +287,7 @@ dirtyWorkspaces.every((ws) => { const r = fetchedResources[ws] return ( - !r || - canWrite(states[ws]?.draft?.path ?? initialPath, r.extra_perms ?? {}, actingUserIn(ws)) + !r || canWrite(states[ws]?.draft?.path ?? initialPath, r.extra_perms ?? {}, acting.in(ws)) ) }) ) @@ -322,11 +310,6 @@ ensureHandle(ws, s) initialStates[ws] = structuredClone(s) existedInitially[ws] = false - // A resource being created runs no fetch for the acting user to ride along with, - // so a workspace the navigation store cannot answer for is asked here. - if (ws !== $workspaceStore && !(ws in perWsUser)) { - getUserExt(ws).then((u) => (perWsUser[ws] = u)) - } }) }) @@ -336,45 +319,40 @@ if (!ws || !initialPath) return if (ws in states) return untrack(() => { - // `actingUserIn` answers from `$userStore` for the navigation workspace, so only - // another one is worth asking. - const needsUser = ws !== $workspaceStore - Promise.all([ - ResourceService.getResource({ workspace: ws, path: initialPath, getDraft: true }), - needsUser ? getUserExt(ws) : undefined - ]).then(([r, user]) => { - // `.draft` already holds the editor's `ResourceState` shape. - const savedDraftState = (r as any).draft as ResourceState | undefined - fetchedResources[ws] = r - // Deployed baseline as the dirty-check reference, so the banner - // compares draft-vs-deployed and fires immediately when a draft exists. - const deployedState: ResourceState = { - path: r.path, - description: r.description ?? '', - args: (r.value ?? {}) as any, - labels: r.labels ?? undefined, - wsSpecific: r.ws_specific ?? false + ResourceService.getResource({ workspace: ws, path: initialPath, getDraft: true }).then( + (r) => { + // `.draft` already holds the editor's `ResourceState` shape. + const savedDraftState = (r as any).draft as ResourceState | undefined + fetchedResources[ws] = r + // Deployed baseline as the dirty-check reference, so the banner + // compares draft-vs-deployed and fires immediately when a draft exists. + const deployedState: ResourceState = { + path: r.path, + description: r.description ?? '', + args: (r.value ?? {}) as any, + labels: r.labels ?? undefined, + wsSpecific: r.ws_specific ?? false + } + // Open with the saved draft if present, else the deployed. + const s: ResourceState = savedDraftState ?? deployedState + openedOnDraft[ws] = !!savedDraftState + // Gate BEFORE the handle is acquired: `stopSync` queues on a + // not-yet-live entry, and the form can settle before the effect + // above gets a chance to run. Only worth doing when no draft exists + // yet — where one does, there is no phantom to prevent and + // suspending could only drop a write. + if (!savedDraftState) setGated(ws, true) + ensureHandle(ws, s) + initialStates[ws] = structuredClone(deployedState) + // Draft-only paths (`no_deployed`) have no row — saving must + // CREATE, not update (update 404s). + existedInitially[ws] = !(r as any).no_deployed + // Keep resource_type in sync for the base workspace (controls the schema) + if (ws === effectiveWorkspace) { + resource_type = r.resource_type + } } - // Open with the saved draft if present, else the deployed. - const s: ResourceState = savedDraftState ?? deployedState - openedOnDraft[ws] = !!savedDraftState - // Gate BEFORE the handle is acquired: `stopSync` queues on a - // not-yet-live entry, and the form can settle before the effect - // above gets a chance to run. Only worth doing when no draft exists - // yet — where one does, there is no phantom to prevent and - // suspending could only drop a write. - if (!savedDraftState) setGated(ws, true) - ensureHandle(ws, s) - initialStates[ws] = structuredClone(deployedState) - // Draft-only paths (`no_deployed`) have no row — saving must - // CREATE, not update (update 404s). - existedInitially[ws] = !(r as any).no_deployed - if (needsUser) perWsUser[ws] = user - // Keep resource_type in sync for the base workspace (controls the schema) - if (ws === effectiveWorkspace) { - resource_type = r.resource_type - } - }) + ) }) }) @@ -441,7 +419,7 @@ onDraftStateChange?.(!!initialPath && selectedDirty) }) $effect(() => { - onCanWriteChange?.(can_write) + onCanWriteChange?.(can_write === true) }) export function localDraftDeployed(): ResourceState | undefined { @@ -575,7 +553,9 @@ {/if} - {#if current} + + {#if current && can_write !== undefined} {#key current} current!.path, setPath} @@ -597,7 +577,7 @@ {resourceToEdit} onLoadResourceType={() => resourceTypeResource.refetch()} workspace={selected} - actingUser={actingUserIn(selected)} + actingUser={acting.in(selected) ?? null} /> {/key} {/if} diff --git a/frontend/src/lib/components/ResourceEditorDrawer.svelte b/frontend/src/lib/components/ResourceEditorDrawer.svelte index ea3bd1407c..15c14f4251 100644 --- a/frontend/src/lib/components/ResourceEditorDrawer.svelte +++ b/frontend/src/lib/components/ResourceEditorDrawer.svelte @@ -5,10 +5,9 @@ import { History, Loader2, Save } from 'lucide-svelte' import WsSpecificVersions from './WsSpecificVersions.svelte' - import { userStore, workspaceStore } from '$lib/stores' + import { workspaceStore } from '$lib/stores' import { isOwner } from '$lib/utils' - import { getUserExt } from '$lib/user' - import { resource } from 'runed' + import { useActingUser } from '$lib/actingUser.svelte' import LocalDraftBanner from './LocalDraftBanner.svelte' import OpenInSessionButton from './sessions/OpenInSessionButton.svelte' import { @@ -62,24 +61,13 @@ // The editor renders whichever workspace-specific variant `selected` points at, so history has // to follow it too — otherwise a restore would write over the variant the user is not looking at. let historyWorkspace = $derived(selected ?? effectiveWorkspace) - // `$userStore` is loaded for the navigation workspace and answers only for that one, so - // only history pointed elsewhere costs a lookup — and only once a resource is open, since - // this drawer outlives every resource it opens. - const otherWsUser = resource( - () => (path && historyWorkspace !== $workspaceStore ? historyWorkspace : undefined), - async (ws) => (ws ? await getUserExt(ws) : undefined) - ) - const historyUser = $derived.by(() => { - if (historyWorkspace === $workspaceStore) return $userStore - const u = otherWsUser.current - // `resource` keeps the previous result across a refetch, and a superseded lookup can - // still land last, so a user only answers for the workspace they were fetched for. - return u?.workspace_id === historyWorkspace ? u : undefined - }) + // Gated on `path`: this drawer outlives every resource it opens, so there is nothing to + // answer about until one is open. + const historyUser = useActingUser(() => (path ? historyWorkspace : undefined)) // Clearing is irreversible and the backend gates it on ownership, not write access, so the // verdict has to come from the membership `historyWorkspace` knows about. An unresolved // user gets no Clear button rather than one computed from another workspace's rights. - let canClearSelected = $derived(isOwner(path ?? '', historyUser, historyWorkspace)) + let canClearSelected = $derived(isOwner(path ?? '', historyUser.current, historyWorkspace)) // A close reaches `on:close` on a later flush, by which point a caller that closed this drawer to // open another editor has already anchored the new one. Clearing then would strip that anchor. diff --git a/frontend/src/lib/components/ResourceForm.svelte b/frontend/src/lib/components/ResourceForm.svelte index 629d403bb2..df1aa5bd99 100644 --- a/frontend/src/lib/components/ResourceForm.svelte +++ b/frontend/src/lib/components/ResourceForm.svelte @@ -38,7 +38,9 @@ viewJsonSchema: boolean jsonError: string deployTo: string | undefined - can_write: boolean + /** `undefined` while the acting user or the resource is still being resolved: neither a + * grant nor the denial the read-only alert announces. */ + can_write: boolean | undefined resource_type: string | undefined resourceTypeInfo: ResourceType | undefined resourceSchema: Schema | undefined @@ -49,9 +51,10 @@ * defaults to the nav workspace. */ workspace?: string | undefined /** The user acting in `workspace`, resolved by the editor above. `undefined` while - * that lookup is pending or after it failed: every check below then refuses, rather - * than answering with the navigation user's rights in another workspace. */ - actingUser: UserExt | undefined + * `null` while that lookup is pending or after it failed: every check below then + * refuses, rather than answering with the navigation user's rights in another + * workspace. */ + actingUser: UserExt | null /** Fired once the GitLab picker has stored the picked project's token, so a * form that would otherwise file the URL as a secret knows it holds none. */ onCredentialStored?: () => void @@ -158,7 +161,7 @@ {#if !hidePath}
- {#if !can_write} + {#if can_write === false}
You only have read access to this resource and cannot edit it @@ -168,13 +171,13 @@
diff --git a/frontend/src/lib/components/VariableEditor.svelte b/frontend/src/lib/components/VariableEditor.svelte index d1cc858fc4..bbec4b2cb9 100644 --- a/frontend/src/lib/components/VariableEditor.svelte +++ b/frontend/src/lib/components/VariableEditor.svelte @@ -1,7 +1,7 @@