mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-07-26 11:49:16 +00:00
fix(stacks): close dialog and toast on Empty create (F-2) (#1168)
* fix(stacks): close dialog and toast on Empty create (F-2)
The Empty branch of the Create Stack dialog left no visual confirmation
on a successful POST: no success toast, no busy state on the Create
button, and no double-click guard. The first thing a new operator does
is create a stack, so a click that looks like a no-op is the day-one
impression killer.
What changed
- Empty handler now mirrors the Git and docker-run patterns: a
creatingEmpty busy guard, Loader2 spinner on the Create button,
disabled Cancel mid-flight, and a "Stack <name> created." success
toast fired synchronously before the editor-load handoff.
- The Empty panel is wrapped in a form so the Enter key submits the
same path as clicking Create.
- Mapped 403 to a clearer permission message; fall back to the
backend's error string for other non-OK statuses.
- Capture the active node id at handler entry and pass it through
onStackCreated. The parent compares against the live node ref and
skips the editor load if the user switched nodes mid-create, with
an info toast pointing at the prior node.
- Tightened the create-a-new-stack E2E (no more silent .catch on the
dialog-close assertion, asserts the toast, no longer relies on a
page reload to see the sidebar entry) and added a regression spec
that the busy guard collapses double-clicks into a single POST.
* fix(stacks): close double-click race in create handler (F-2)
The setState-based busy guard could race a second click that landed
before React committed the disabled state, so two POSTs got dispatched
when a user double-clicked Create. The new useRef-based check is
read and written synchronously inside the handler so a re-entrant
invocation bails before issuing a second fetch.
The accompanying E2E (`create dialog: double-clicking Create fires
only one POST`) was also hanging to its 30s test timeout: the second
click's locator resolution could outlive the dialog when the first
POST resolved quickly, so Playwright waited for `[role="dialog"]` to
reappear. Both clicks now fire in the same microtask via Promise.all
with a 1s timeout on the second click so the locator-resolution path
fails fast instead of hanging the test.
* fix(stacks): keep busy guard active across modal close (F-2)
Two issues from independent review.
1. Important: resetting creatingEmpty / creatingEmptyRef inside the modal
close handler created an Escape/backdrop race. A user could close the
dialog mid-flight (clearing both flags), reopen, click Create again,
and slip past the guard before the first POST settled. The finally
block already owns that lifecycle, so the close handler no longer
touches either flag.
2. Nit: bring 403 messaging to parity with the Git branch. The Empty
handler now reads the backend's error field first and falls back to
the original hardcoded copy if it is missing, mirroring how
handleCreateStackFromGit surfaces backend permission errors.
* test(stacks): rebuild double-click spec around observable disabled-state (F-2)
The Promise.all racing approach to "double-click fires only one POST"
hung to the 30s test timeout on CI. The page snapshot at timeout confirms
the product fix works correctly (dialog closed, stack created, editor
navigated); the test itself was the flake. Race-based assertions on a
single React-state commit boundary are inherently timing-sensitive.
The rewrite turns the test inside-out:
- The route handler holds POST responses for 400ms so the in-flight
window is large enough to observe deterministically.
- await route.continue() (instead of void fire-and-forget) closes a
subtle stall pattern Playwright's docs flag under load.
- After the first click the test asserts the button reaches a disabled
state within the in-flight window, which proves the busy guard
activated at the React-state layer.
- A best-effort force-click against the now-disabled button exercises
the user-mash path; the synchronous useRef guard inside the handler
catches it whether or not the browser dispatches the click.
- expect(postCount).toBe(1) still owns the actual regression assertion.
No product code change. The synchronous creatingEmptyRef guard already
landed in 61e79861.
This commit is contained in:
Anso
+60
-7
@@ -35,18 +35,71 @@ test.describe('Stack management', () => {
|
||||
await page.locator('#create-stack-name').fill(TEST_STACK);
|
||||
await page.locator('[role="dialog"]').getByRole('button', { name: 'Create' }).click();
|
||||
|
||||
// Wait for dialog to close (success) or error message to appear (failure)
|
||||
await Promise.race([
|
||||
page.getByRole('dialog').waitFor({ state: 'hidden', timeout: 8_000 }),
|
||||
page.getByText(/already exists/i).waitFor({ state: 'visible', timeout: 8_000 }),
|
||||
]).catch(() => { });
|
||||
// F-2: the dialog MUST close on a successful create. No silent .catch swallow.
|
||||
await expect(page.getByRole('dialog')).toBeHidden({ timeout: 8_000 });
|
||||
|
||||
// The stack should now exist - refresh and verify via the sidebar
|
||||
// F-2: a success toast MUST appear so the click does not feel like a no-op.
|
||||
await expect(page.getByText(`Stack "${TEST_STACK}" created.`)).toBeVisible({ timeout: 5_000 });
|
||||
|
||||
// F-2: the new stack appears in the sidebar without a manual reload.
|
||||
await expect(
|
||||
page.locator('[role="listbox"]').getByText(TEST_STACK, { exact: true })
|
||||
).toBeVisible({ timeout: 5_000 });
|
||||
});
|
||||
|
||||
test('create dialog: double-clicking Create fires only one POST', async ({ page }) => {
|
||||
const stackName = 'e2e-double-click-stack';
|
||||
|
||||
// Clean any prior leftover and reload so we start from a known sidebar state.
|
||||
await page.evaluate(async (name) => {
|
||||
await fetch(`/api/stacks/${name}`, { method: 'DELETE', credentials: 'include' }).catch(() => { });
|
||||
}, stackName);
|
||||
await page.reload();
|
||||
await loginAs(page);
|
||||
await waitForStacksLoaded(page);
|
||||
|
||||
await expect(page.getByText(TEST_STACK).first()).toBeVisible({ timeout: 5_000 });
|
||||
// Count POST /api/stacks calls and hold the response briefly so the
|
||||
// disabled-button window is observable from the test. Awaiting
|
||||
// route.continue() avoids the fire-and-forget pattern that can subtly
|
||||
// stall a request on a busy CI runner.
|
||||
let postCount = 0;
|
||||
await page.route('**/api/stacks', async (route) => {
|
||||
if (route.request().method() === 'POST') {
|
||||
postCount += 1;
|
||||
await new Promise((resolve) => setTimeout(resolve, 400));
|
||||
}
|
||||
await route.continue();
|
||||
});
|
||||
|
||||
try {
|
||||
await page.getByRole('button', { name: 'Create Stack' }).click();
|
||||
await expect(page.getByRole('dialog')).toBeVisible({ timeout: 5_000 });
|
||||
await page.locator('#create-stack-name').fill(stackName);
|
||||
|
||||
const createBtn = page.locator('[role="dialog"]').getByRole('button', { name: /^Create/ });
|
||||
await createBtn.click();
|
||||
|
||||
// The synchronous useRef guard plus React's disabled re-render must
|
||||
// surface a disabled Create button while the POST is in flight. If this
|
||||
// assertion ever times out, the busy-state path has regressed.
|
||||
await expect(createBtn).toBeDisabled({ timeout: 1_500 });
|
||||
|
||||
// Best-effort second click against the now-disabled native button. A real
|
||||
// user mash translates to a no-op at the browser level on a disabled
|
||||
// submit button; the synchronous ref guard catches it even if Playwright
|
||||
// managed to dispatch the click anyway.
|
||||
await createBtn.click({ force: true, timeout: 500 }).catch(() => undefined);
|
||||
|
||||
await expect(page.getByRole('dialog')).toBeHidden({ timeout: 8_000 });
|
||||
await expect(page.getByText(`Stack "${stackName}" created.`)).toBeVisible({ timeout: 5_000 });
|
||||
expect(postCount).toBe(1);
|
||||
} finally {
|
||||
await page.unroute('**/api/stacks');
|
||||
// Cleanup
|
||||
await page.evaluate(async (name) => {
|
||||
await fetch(`/api/stacks/${name}`, { method: 'DELETE', credentials: 'include' }).catch(() => { });
|
||||
}, stackName);
|
||||
}
|
||||
});
|
||||
|
||||
test('delete the test stack', async ({ page }) => {
|
||||
|
||||
@@ -33,6 +33,7 @@ import { useTrivyStatus } from '@/hooks/useTrivyStatus';
|
||||
import { StackSidebar } from '@/components/sidebar/StackSidebar';
|
||||
import type { StackRowStatus } from '@/components/sidebar/stack-status-utils';
|
||||
import { useComposeDiffPreviewEnabled } from '@/hooks/use-compose-diff-preview-enabled';
|
||||
import { toast } from '@/components/ui/toast-store';
|
||||
|
||||
export default function EditorLayout() {
|
||||
const { isAdmin, can } = useAuth();
|
||||
@@ -91,6 +92,15 @@ export default function EditorLayout() {
|
||||
|
||||
const { nodes, activeNode, setActiveNode } = useNodes();
|
||||
|
||||
// Mirror activeNode.id in a ref so async handlers (e.g. CreateStackDialog's
|
||||
// post-create handoff) can detect a node switch that happened mid-flight.
|
||||
// Closure capture of activeNode would always match the value at handler-creation
|
||||
// time and miss the switch.
|
||||
const activeNodeIdRef = useRef<number | null>(activeNode?.id ?? null);
|
||||
useEffect(() => {
|
||||
activeNodeIdRef.current = activeNode?.id ?? null;
|
||||
}, [activeNode?.id]);
|
||||
|
||||
const overlayState = useOverlayState();
|
||||
const {
|
||||
createDialogOpen, setCreateDialogOpen,
|
||||
@@ -227,8 +237,16 @@ export default function EditorLayout() {
|
||||
<CreateStackDialog
|
||||
open={createDialogOpen}
|
||||
onOpenChange={setCreateDialogOpen}
|
||||
onStackCreated={async (sName) => {
|
||||
onStackCreated={async (sName, sourceNodeId) => {
|
||||
await refreshStacks();
|
||||
// loadFile keeps its own unsaved-changes overlay (intentional safety,
|
||||
// shared with every other "switch to a different stack" code path).
|
||||
// Skip the load if the user switched nodes mid-create so we do not
|
||||
// 404 against a stack name that lives on the previous node.
|
||||
if (sourceNodeId != null && activeNodeIdRef.current !== sourceNodeId) {
|
||||
toast.info(`Stack "${sName}" created on the previous node.`);
|
||||
return;
|
||||
}
|
||||
await stackActions.loadFile(sName);
|
||||
}}
|
||||
onStacksChanged={async () => { await refreshStacks(); }}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useRef, useState, type KeyboardEvent } from 'react';
|
||||
import { useRef, useState, type FormEvent, type KeyboardEvent } from 'react';
|
||||
import { Plus, GitBranch, FileCode2, Loader2, type LucideIcon } from 'lucide-react';
|
||||
import { Modal, ModalHeader, ModalBody, ModalFooter } from '../ui/modal';
|
||||
import { Button } from '../ui/button';
|
||||
@@ -9,12 +9,16 @@ import { Checkbox } from '../ui/checkbox';
|
||||
import { GitSourceFields, type ApplyMode } from '../stack/GitSourceFields';
|
||||
import { apiFetch } from '@/lib/api';
|
||||
import { toast } from '@/components/ui/toast-store';
|
||||
import { useNodes } from '@/context/NodeContext';
|
||||
import { cn } from '@/lib/utils';
|
||||
|
||||
export interface CreateStackDialogProps {
|
||||
open: boolean;
|
||||
onOpenChange: (open: boolean) => void;
|
||||
onStackCreated: (stackName: string) => void | Promise<void>;
|
||||
// sourceNodeId is the active node ID captured at the moment the user clicked
|
||||
// Create. Parent compares against the current active node before navigating
|
||||
// so a mid-flight node switch does not land the user on a 404.
|
||||
onStackCreated: (stackName: string, sourceNodeId: number | null | undefined) => void | Promise<void>;
|
||||
onStacksChanged: () => void | Promise<void>;
|
||||
}
|
||||
|
||||
@@ -30,8 +34,16 @@ const tabId = (m: CreateMode) => `create-stack-tab-${m}`;
|
||||
const panelId = (m: CreateMode) => `create-stack-panel-${m}`;
|
||||
|
||||
export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacksChanged }: CreateStackDialogProps) {
|
||||
const { activeNode } = useNodes();
|
||||
const [createMode, setCreateMode] = useState<CreateMode>('empty');
|
||||
const [newStackName, setNewStackName] = useState('');
|
||||
// Synchronous guard. The disabled-button + setState pair can race a rapid
|
||||
// second click that lands before React has committed the disabled state,
|
||||
// re-entering the handler with a stale closure value of creatingEmpty and
|
||||
// firing a second POST. The ref check is read/written synchronously inside
|
||||
// the handler so the second invocation bails before issuing another POST.
|
||||
const creatingEmptyRef = useRef(false);
|
||||
const [creatingEmpty, setCreatingEmpty] = useState(false);
|
||||
const [dockerRunInput, setDockerRunInput] = useState('');
|
||||
const [convertedYaml, setConvertedYaml] = useState<string | null>(null);
|
||||
const [isConverting, setIsConverting] = useState(false);
|
||||
@@ -66,8 +78,12 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
};
|
||||
|
||||
const handleCreateStack = async () => {
|
||||
if (creatingEmptyRef.current) return;
|
||||
if (!newStackName.trim()) return;
|
||||
const stackName = newStackName.trim();
|
||||
const sourceNodeId = activeNode?.id;
|
||||
creatingEmptyRef.current = true;
|
||||
setCreatingEmpty(true);
|
||||
try {
|
||||
const response = await apiFetch('/stacks', {
|
||||
method: 'POST',
|
||||
@@ -75,21 +91,37 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
});
|
||||
if (!response.ok) {
|
||||
if (response.status === 409) {
|
||||
throw new Error('Stack already exists');
|
||||
} else if (response.status === 400) {
|
||||
throw new Error('Invalid stack name (use alphanumeric characters and hyphens only)');
|
||||
throw new Error('Stack already exists.');
|
||||
}
|
||||
throw new Error('Failed to create stack');
|
||||
if (response.status === 400) {
|
||||
throw new Error('Invalid stack name (use alphanumeric characters and hyphens only).');
|
||||
}
|
||||
const body = await response.json().catch(() => ({}));
|
||||
const backendError = (body as { error?: string })?.error;
|
||||
if (response.status === 403) {
|
||||
throw new Error(backendError || 'You do not have permission to create stacks.');
|
||||
}
|
||||
throw new Error(backendError || 'Failed to create stack.');
|
||||
}
|
||||
onOpenChange(false);
|
||||
setNewStackName('');
|
||||
await onStackCreated(stackName);
|
||||
setCreateMode('empty');
|
||||
toast.success(`Stack "${stackName}" created.`);
|
||||
await onStackCreated(stackName, sourceNodeId);
|
||||
} catch (error) {
|
||||
console.error('Failed to create stack:', error);
|
||||
toast.error((error as Error).message || 'Failed to create stack');
|
||||
toast.error((error as Error).message || 'Failed to create stack.');
|
||||
} finally {
|
||||
creatingEmptyRef.current = false;
|
||||
setCreatingEmpty(false);
|
||||
}
|
||||
};
|
||||
|
||||
const handleEmptyFormSubmit = (e: FormEvent<HTMLFormElement>) => {
|
||||
e.preventDefault();
|
||||
void handleCreateStack();
|
||||
};
|
||||
|
||||
const handleCreateStackFromGit = async () => {
|
||||
const stackName = newStackName.trim();
|
||||
if (!stackName) {
|
||||
@@ -104,6 +136,7 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
toast.error('Only HTTPS repository URLs are supported.');
|
||||
return;
|
||||
}
|
||||
const sourceNodeId = activeNode?.id;
|
||||
setCreatingFromGit(true);
|
||||
const loadingId = toast.loading(gitDeployNow ? 'Fetching, creating, and deploying...' : 'Fetching and creating stack...');
|
||||
try {
|
||||
@@ -154,7 +187,7 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
}
|
||||
onOpenChange(false);
|
||||
resetCreateFromGitForm();
|
||||
await onStackCreated(stackName);
|
||||
await onStackCreated(stackName, sourceNodeId);
|
||||
} catch (error) {
|
||||
console.error('Failed to create stack from Git:', error);
|
||||
toast.error((error as Error)?.message || 'Failed to create stack from Git.');
|
||||
@@ -209,6 +242,7 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
toast.error('Convert the command before creating the stack.');
|
||||
return;
|
||||
}
|
||||
const sourceNodeId = activeNode?.id;
|
||||
setCreatingFromDockerRun(true);
|
||||
const loadingId = toast.loading('Creating stack from converted YAML...');
|
||||
let createdStack = false;
|
||||
@@ -245,7 +279,7 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
onOpenChange(false);
|
||||
resetCreateFromDockerRunForm();
|
||||
setNewStackName('');
|
||||
await onStackCreated(stackName);
|
||||
await onStackCreated(stackName, sourceNodeId);
|
||||
} catch (error) {
|
||||
console.error('Failed to create stack from docker run:', error);
|
||||
const err = error as { message?: string; error?: string; data?: { error?: string } };
|
||||
@@ -265,7 +299,7 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
}
|
||||
};
|
||||
|
||||
const busy = creatingFromGit || creatingFromDockerRun;
|
||||
const busy = creatingEmpty || creatingFromGit || creatingFromDockerRun;
|
||||
|
||||
return (
|
||||
<Modal
|
||||
@@ -277,6 +311,11 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
setCreateMode('empty');
|
||||
resetCreateFromGitForm();
|
||||
resetCreateFromDockerRunForm();
|
||||
// Intentionally NOT resetting creatingEmptyRef / creatingEmpty
|
||||
// here: the in-flight POST owns its own lifecycle and clears
|
||||
// both flags in finally. Resetting on close would let an
|
||||
// Escape-then-reopen sequence slip past the guard and fire a
|
||||
// second POST before the first request settled.
|
||||
}
|
||||
}}
|
||||
>
|
||||
@@ -289,31 +328,43 @@ export function CreateStackDialog({ open, onOpenChange, onStackCreated, onStacks
|
||||
|
||||
{createMode === 'empty' && (
|
||||
<div role="tabpanel" id={panelId('empty')} aria-labelledby={tabId('empty')}>
|
||||
<ModalBody>
|
||||
<div className="space-y-2">
|
||||
<Label htmlFor="create-stack-name">Stack Name</Label>
|
||||
<Input
|
||||
id="create-stack-name"
|
||||
placeholder="Stack name (e.g., myapp)"
|
||||
value={newStackName}
|
||||
onChange={(e) => setNewStackName(e.target.value)}
|
||||
autoFocus
|
||||
/>
|
||||
</div>
|
||||
</ModalBody>
|
||||
<ModalFooter
|
||||
hint="ALPHANUMERIC · HYPHENS"
|
||||
secondary={
|
||||
<Button type="button" variant="ghost" onClick={() => onOpenChange(false)}>
|
||||
Cancel
|
||||
</Button>
|
||||
}
|
||||
primary={
|
||||
<Button onClick={handleCreateStack} disabled={!newStackName.trim()}>
|
||||
Create
|
||||
</Button>
|
||||
}
|
||||
/>
|
||||
<form onSubmit={handleEmptyFormSubmit}>
|
||||
<ModalBody>
|
||||
<div className="space-y-2">
|
||||
<Label htmlFor="create-stack-name">Stack Name</Label>
|
||||
<Input
|
||||
id="create-stack-name"
|
||||
placeholder="Stack name (e.g., myapp)"
|
||||
value={newStackName}
|
||||
onChange={(e) => setNewStackName(e.target.value)}
|
||||
disabled={creatingEmpty}
|
||||
autoFocus
|
||||
/>
|
||||
</div>
|
||||
</ModalBody>
|
||||
<ModalFooter
|
||||
hint="ALPHANUMERIC · HYPHENS"
|
||||
secondary={
|
||||
<Button
|
||||
type="button"
|
||||
variant="ghost"
|
||||
onClick={() => onOpenChange(false)}
|
||||
disabled={creatingEmpty}
|
||||
>
|
||||
Cancel
|
||||
</Button>
|
||||
}
|
||||
primary={
|
||||
<Button type="submit" disabled={creatingEmpty || !newStackName.trim()}>
|
||||
{creatingEmpty ? (
|
||||
<><Loader2 className="w-4 h-4 mr-1.5 animate-spin" strokeWidth={1.5} />Creating</>
|
||||
) : (
|
||||
<><Plus className="w-4 h-4 mr-1.5" strokeWidth={1.5} />Create</>
|
||||
)}
|
||||
</Button>
|
||||
}
|
||||
/>
|
||||
</form>
|
||||
</div>
|
||||
)}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user