fix(docker): ask for the depends_on confirmation from the stacks list #1
julian
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/docker-stacks-update-confirm"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The per-container update button on the stacks page posted to
/container/:id/update and reported any failure verbatim. When other
services in the stack depend_on the target, that route answers 409
{needsConfirmation, service, dependents} and expects the client to
re-send with ?confirmed=1. ContainerPage does exactly that; StacksPage
never looked at the body, so the row got a bare "update failed: ... HTTP
409" — no detail, since the body carries no
errorfield for apiFetch toappend — and the update was simply unreachable from that page.
Mirror ContainerPage: keep the ApiError body, raise ConfirmDialog naming
the dependents, retry with ?confirmed=1 on ack. The pending confirmation
is keyed by container and the dialog names the target service, because
this page lists many containers at once.
The per-container update button on the stacks page posted to /container/:id/update and reported any failure verbatim. When other services in the stack depend_on the target, that route answers 409 {needsConfirmation, service, dependents} and expects the client to re-send with ?confirmed=1. ContainerPage does exactly that; StacksPage never looked at the body, so the row got a bare "update failed: ... HTTP 409" — no detail, since the body carries no `error` field for apiFetch to append — and the update was simply unreachable from that page. Mirror ContainerPage: keep the ApiError body, raise ConfirmDialog naming the dependents, retry with ?confirmed=1 on ack. The pending confirmation is keyed by container and the dialog names the target service, because this page lists many containers at once.The fix itself is correct — I verified it end-to-end against fake data (dialog appears naming the dependents, confirm retries with
?confirmed=1, the row shows the updating state and settles cleanly). Good commit message, too.Requesting changes on three points before this merges; see the inline comments:
containerNamefallback — the server always sends a non-emptyservicein the 409 body, so this is dead code.@ -62,0 +70,4 @@let confirmUpdate = $state<{ id: string; service: string; dependents: string[] } | null>(null);const containerName = (id: string) =>Required: drop this helper. The server builds the 409 body as
service: target.service || target.name, sobody.serviceis never empty andcontainerName()(plus theid.slice(0, 12)fallback behind it) is unreachable. Usebody.servicedirectly in the handler below — dead defensive code hides the actual contract.@ -69,1 +92,4 @@}: null;updatingC[id] = false;if (body?.needsConfirmation) {Required: guard the overwrite. If two updates are in flight and both answer 409 (slow request, quick clicks on two rows before the first dialog opens), the second assignment replaces the first
confirmUpdate— the first container gets no dialog and no error, it just silently does nothing. Cheapest fix: ifconfirmUpdateis already set, put the confirmation prompt intoactionError[id]instead of replacing the pending dialog, so the user sees why nothing happened and can click again.@ -365,6 +399,22 @@</Modal>{/if}<ConfirmDialogRequired: extract this. The title, message template, labels, and confirm/cancel wiring are now duplicated verbatim from
ContainerPage.svelte— two copies of user-facing wording will drift on the next edit. Move it into a small shared component in the docker frontend (e.g.DependentsConfirm.sveltetakingservice,dependents,onconfirm,oncancel) and use it from both pages.The previous commit's one-at-a-time guard was wrong in three ways. It told the user to answer the open dialog and retry, but for two containers in the same stack that retry cannot succeed: confirming the first one makes the server reserve the project (`updating.add`), so the second POST comes back "update already running". The message it wrote to the row was never cleared by anything, so it outlived the dialog it referred to. And it hand-typed a second copy of the dependents wording, hardcoding the plural, one commit after that wording was centralized. Queue the pending confirmations instead. The dialog shows the head of the queue and each answer shifts it, so a second 409 waits its turn rather than replacing or being turned away — no advice to give, nothing to go stale, and the wording stays in one place. A container already queued is not enqueued twice. A confirmed retry that still comes back needing confirmation now reports that instead of reopening the same dialog, which otherwise loops forever if the query flag never reaches the server. The ApiError body cast also claimed `service` and `dependents` for every failure on the route, including the `{ error }` bodies and the ones with no body at all. Narrow it on `status === 409` first, where those fields are actually guaranteed.Three ways the confirmation could be answered and then mean nothing. A stack update reserves the project server-side for the whole pull, but the Modal showing its progress can be dismissed while it runs, and the per- container buttons underneath stayed enabled. Clicking one raised the depends_on prompt, and "Update anyway" then came back "update already running" — the acknowledgement thrown away, the row left on a raw 409. Disable those buttons while their own stack is locked and say why in the title. Both pages keep refreshing under an open prompt. A queued confirmation names its container by short id, which a recreate changes, so answering it POSTed to an id the server no longer knows — a 404 written to an actionError key no row renders, i.e. nothing on screen at all. Drop queued entries whose container is gone on refresh, and whose image is no longer reported updatable. A null updateAvailable means "not checked yet" and keeps the entry. Same shape on the container page, where the prompt lives outside the `{#if update?.updateAvailable}` block that owns the button: if a refresh retracts the offer while the dialog is up, confirming would pull and recreate an already-current container, restarting its dependents for nothing.All three addressed. Head is now
2039373.1.
containerNamedropped (327984d). Confirmed againstmod.ts:1083— the body is built asservice: target.service || target.name, sobody.serviceis never empty. The helper and itsid.slice(0, 12)fallback are gone; both pages readbody.servicedirectly. I also narrowed theApiErrorbody cast one.status === 409first, since that is the only response on the route where those fields are actually guaranteed.2. Dialog extracted (
327984d). NewDependentsConfirm.sveltetakingservice,dependents,onconfirm,oncancel, used by both StacksPage and ContainerPage; neither importsConfirmDialogany more. One user-visible consequence: ContainerPage's message used to say "depend on this service" and now names it, because the shared component takes the service and ContainerPage was already receiving it in the 409 body without using it.3. Overwrite guarded — but not the way you suggested, because I don't think that way works. Writing the second confirmation into
actionError[id]tells the user to answer the open dialog and then retry. For two containers in the same stack that retry cannot succeed:updating.add(stack.project)(mod.ts:1088) runs as soon as the first confirmation is accepted, so the second POST comes backupdate already runningfrommod.ts:1067. The advice would be wrong precisely when the collision is most likely. It also left a message on the row that nothing ever cleared, so it outlived the dialog it pointed at.Replaced with a queue (
89b6bb7):confirmQueueholds the pending confirmations, the dialog shows the head, and each answer shifts it. A second 409 waits its turn instead of being dropped or turned away — no advice to get wrong, no message to go stale, and the wording stays in the one place point 2 put it. A container already queued is not enqueued twice.959ddb2e65to20393731caAll three points from the previous review are resolved, and resolved well — the queue, the shared
DependentsConfirm, the narrowed 409 cast, plus the stale-prompt pruning and thestackLockedguard I didn't ask for. Re-verified end-to-end on this head: build clean, cancel path, confirm path, and the container page through the shared component all behave.One remaining required change before merge; see the inline comment: consecutive queued prompts reuse the still-mounted dialog, so one Enter can acknowledge two containers back to back.
@ -365,6 +430,19 @@</Modal>{/if}<DependentsConfirmRequired: with two or more prompts queued, confirming shifts the next one into the dialog that is already mounted —
opennever goes false, soConfirmDialog's$effectdoesn't re-run: no remount, no focus reset. Its keydown handler confirms on Enter regardless of focus, so a second Enter acks the next container while its message has barely rendered. Wrap the dialog in{#key confirmUpdate?.id}so each pending confirmation gets a fresh mount (restoring the focus-on-Cancel behavior of the danger variant), or blankopenfor a tick between prompts.The
{#key confirmUpdate?.id}remount is exactly what was asked — focus reset and a visible re-animation per queued prompt. Re-verified on this head: build clean, cancel path, confirm path, and the container page through the shared component all behave.Approving. One note for a separate issue, not this PR:
ConfirmDialogconfirms Enter regardless of focus, so a held key (auto-repeat) could still chain-acknowledge queued prompts across remounts — ane.repeatguard in the component would close that.Thanks for the precise turnaround across all three rounds.