fix: harden workspace manager drafts
This commit is contained in:
@@ -38,3 +38,36 @@ exit 0
|
||||
|
||||
`git diff --check` passed before commit. No workspace secret value, secret-file path, raw
|
||||
diagnostic body, publish call, import flow, or export flow was introduced.
|
||||
|
||||
## Fix round 1
|
||||
|
||||
### Root causes and fixes
|
||||
|
||||
- The original duplicate proposal appended `-copy` and then truncated at 63 characters. For an
|
||||
already-maximal ID, truncation could remove the suffix and reproduce the immutable source ID.
|
||||
The proposal now reserves suffix space and falls back to a distinct `-2` suffix when a maximal
|
||||
source already ends in `-copy`.
|
||||
- `dwh.timeout_ms` was rendered as a positive numeric field but was absent from the client
|
||||
validation map. It now has the same immediate accessible error treatment as other numeric
|
||||
fields, so a rejected save never reaches the manager’s saved-draft toast.
|
||||
- Registry status, workspace list, and selected-detail React Query failures were rendered as
|
||||
loading, empty, or unselected states. Each now has a named alert and a retry control, distinct
|
||||
from its corresponding loading and empty state.
|
||||
|
||||
### TDD evidence
|
||||
|
||||
- RED: max-length duplication retained the original 63-character ID; the timeout field produced
|
||||
no alert; and each of the three failed queries had no accessible retry control.
|
||||
- GREEN: the focused manager/editor tests passed **12/12**, covering a valid changed duplicate
|
||||
proposal, rejected zero timeout with no save toast, and status/list/detail retry recovery.
|
||||
|
||||
### Verification
|
||||
|
||||
Executed from `frontend/`:
|
||||
|
||||
```text
|
||||
npx vitest run
|
||||
50 test files passed, 364 tests passed
|
||||
npx tsc -b
|
||||
exit 0
|
||||
```
|
||||
|
||||
@@ -70,3 +70,17 @@ test("uses native closed selects for each workspace enum and embedding provider"
|
||||
expect(screen.getByRole("listbox", { name: "DWH transport" })).toHaveProperty("multiple", true);
|
||||
expect(screen.getByRole("listbox", { name: "Vector transport" })).toHaveProperty("multiple", true);
|
||||
});
|
||||
|
||||
test("rejects a non-positive DWH timeout without saving a draft", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onSaveDraft = vi.fn();
|
||||
render(<WorkspaceEditor draft={draft} onSaveDraft={onSaveDraft} onPublish={vi.fn()} />);
|
||||
|
||||
await user.clear(screen.getByLabelText("DWH timeout (ms)"));
|
||||
await user.type(screen.getByLabelText("DWH timeout (ms)"), "0");
|
||||
await user.click(screen.getByRole("button", { name: "Save draft" }));
|
||||
|
||||
expect(screen.getByRole("alert")).toHaveTextContent("DWH timeout must be a positive whole number");
|
||||
expect(screen.getByLabelText("DWH timeout (ms)")).toHaveAttribute("aria-invalid", "true");
|
||||
expect(onSaveDraft).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -49,6 +49,8 @@ function validate(workspace: CanonicalWorkspace): FieldErrors {
|
||||
if (!workspace.dwh.supported_transports.length) errors["dwh.transport"] = "Choose at least one DWH transport";
|
||||
const dwhPort = positiveInteger(workspace.dwh.port, "DWH port", 65_535);
|
||||
if (dwhPort) errors["dwh.port"] = dwhPort;
|
||||
const dwhTimeout = positiveInteger(workspace.dwh.timeout_ms, "DWH timeout");
|
||||
if (dwhTimeout) errors["dwh.timeout"] = dwhTimeout;
|
||||
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(workspace.semantic_index.vector_store.database)) errors["vector.database"] = "Use a database identifier";
|
||||
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(workspace.semantic_index.vector_store.schema)) errors["vector.schema"] = "Use a schema identifier";
|
||||
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(workspace.semantic_index.vector_store.collection)) errors["vector.collection"] = "Use a collection identifier";
|
||||
@@ -175,7 +177,7 @@ export function WorkspaceEditor({ draft, onSaveDraft, onPublish: _onPublish, idL
|
||||
<Field label="DWH port" error={errors["dwh.port"]}>
|
||||
{({ id, describedBy, invalid }) => <input id={id} type="number" min="1" max="65535" aria-label="DWH port" aria-describedby={describedBy} aria-invalid={invalid} className={fieldClass} value={workspace.dwh.port ?? ""} onChange={(event) => update((value) => ({ ...value, dwh: { ...value.dwh, port: numberOrUndefined(event.target.value) } }))} />}
|
||||
</Field>
|
||||
<Field label="DWH timeout (ms)">
|
||||
<Field label="DWH timeout (ms)" error={errors["dwh.timeout"]}>
|
||||
{({ id, describedBy, invalid }) => <input id={id} type="number" min="1" aria-label="DWH timeout (ms)" aria-describedby={describedBy} aria-invalid={invalid} className={fieldClass} value={workspace.dwh.timeout_ms ?? ""} onChange={(event) => update((value) => ({ ...value, dwh: { ...value.dwh, timeout_ms: numberOrUndefined(event.target.value) } }))} />}
|
||||
</Field>
|
||||
</Section>
|
||||
|
||||
@@ -72,6 +72,43 @@ test("stages duplicate and delete operations without publishing", async () => {
|
||||
expect(published).toBe(false);
|
||||
});
|
||||
|
||||
test("proposes a different valid ID when duplicating a 63-character workspace ID", async () => {
|
||||
const user = userEvent.setup();
|
||||
const maxId = `w${"a".repeat(62)}`;
|
||||
const maxWorkspace = { ...workspace, workspace: { ...workspace.workspace, id: maxId } };
|
||||
server.use(
|
||||
http.get("http://localhost:8787/workspaces", () => HttpResponse.json([{
|
||||
id: maxId, name: "Maximum", displayName: "Maximum", language: "en", file: `workspaces/${maxId}.yaml`,
|
||||
revision: { id: maxId, commit: "a".repeat(40), blob: "b".repeat(40), snapshotPath: "/tmp/maximum", state: "operational" },
|
||||
}])),
|
||||
http.get(`http://localhost:8787/workspaces/${maxId}`, () => HttpResponse.json({
|
||||
workspace: maxWorkspace,
|
||||
revision: { id: maxId, commit: "a".repeat(40), blob: "b".repeat(40), snapshotPath: "/tmp/maximum", state: "operational" },
|
||||
})),
|
||||
);
|
||||
renderManager();
|
||||
|
||||
await user.click(await screen.findByRole("button", { name: "Maximum" }));
|
||||
await user.click(screen.getByRole("button", { name: "Duplicate workspace" }));
|
||||
|
||||
const proposed = screen.getByLabelText("Workspace ID") as HTMLInputElement;
|
||||
expect(proposed.value).toMatch(/^[a-z][a-z0-9-]{2,62}$/);
|
||||
expect(proposed).not.toHaveValue(maxId);
|
||||
});
|
||||
|
||||
test("does not show a saved-draft toast when manager validation rejects a DWH timeout", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderManager();
|
||||
|
||||
await user.click(await screen.findByRole("button", { name: "PSD Clinical" }));
|
||||
await user.clear(screen.getByLabelText("DWH timeout (ms)"));
|
||||
await user.type(screen.getByLabelText("DWH timeout (ms)"), "0");
|
||||
await user.click(screen.getByRole("button", { name: "Save draft" }));
|
||||
|
||||
expect(screen.getByRole("alert")).toHaveTextContent("DWH timeout must be a positive whole number");
|
||||
expect(screen.queryByText("Draft saved in this browser.")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test("runs validation and installation test with only sanitized messages", async () => {
|
||||
const user = userEvent.setup();
|
||||
server.use(
|
||||
@@ -90,3 +127,53 @@ test("runs validation and installation test with only sanitized messages", async
|
||||
expect(await screen.findByText("binding_missing: DWH binding is not configured")).toBeVisible();
|
||||
expect(within(screen.getByTestId("workspace-diagnostics")).queryByText(/password|token|secret/i)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test("shows an accessible retry instead of a loading status when the registry status query fails", async () => {
|
||||
const user = userEvent.setup();
|
||||
let calls = 0;
|
||||
server.use(http.get("http://localhost:8787/workspace-registry/status", () => {
|
||||
calls += 1;
|
||||
return calls === 1 ? new HttpResponse(null, { status: 503 }) : HttpResponse.json({ branch: "main", ahead: 0, behind: 0, degraded: false });
|
||||
}));
|
||||
renderManager();
|
||||
|
||||
expect(await screen.findByRole("alert", { name: "Workspace registry status failed" })).toHaveTextContent("Could not load registry status.");
|
||||
await user.click(screen.getByRole("button", { name: "Retry registry status" }));
|
||||
expect(await screen.findByText("main")).toBeVisible();
|
||||
expect(calls).toBe(2);
|
||||
});
|
||||
|
||||
test("shows an accessible retry instead of an empty list when the workspace list query fails", async () => {
|
||||
const user = userEvent.setup();
|
||||
let calls = 0;
|
||||
server.use(http.get("http://localhost:8787/workspaces", () => {
|
||||
calls += 1;
|
||||
return calls === 1 ? new HttpResponse(null, { status: 503 }) : HttpResponse.json([]);
|
||||
}));
|
||||
renderManager();
|
||||
|
||||
expect(await screen.findByRole("alert", { name: "Workspace list failed" })).toHaveTextContent("Could not load workspaces.");
|
||||
expect(screen.queryByText("No published workspaces.")).not.toBeInTheDocument();
|
||||
await user.click(screen.getByRole("button", { name: "Retry workspace list" }));
|
||||
expect(await screen.findByText("No published workspaces.")).toBeVisible();
|
||||
expect(calls).toBe(2);
|
||||
});
|
||||
|
||||
test("shows an accessible retry when the selected workspace detail query fails", async () => {
|
||||
const user = userEvent.setup();
|
||||
let calls = 0;
|
||||
server.use(http.get("http://localhost:8787/workspaces/psd-clinical", () => {
|
||||
calls += 1;
|
||||
return calls === 1 ? new HttpResponse(null, { status: 503 }) : HttpResponse.json({
|
||||
workspace,
|
||||
revision: { id: "psd-clinical", commit: "a".repeat(40), blob: "b".repeat(40), snapshotPath: "/tmp/psd", state: "operational" },
|
||||
});
|
||||
}));
|
||||
renderManager();
|
||||
|
||||
await user.click(await screen.findByRole("button", { name: "PSD Clinical" }));
|
||||
expect(await screen.findByRole("alert", { name: "Workspace details failed" })).toHaveTextContent("Could not load workspace details.");
|
||||
await user.click(screen.getByRole("button", { name: "Retry workspace details" }));
|
||||
expect(await screen.findByRole("heading", { name: "PSD Clinical" })).toBeVisible();
|
||||
expect(calls).toBe(2);
|
||||
});
|
||||
|
||||
@@ -35,7 +35,18 @@ function draftFromRecord(record: WorkspaceRecord): WorkspaceDraft {
|
||||
}
|
||||
|
||||
function proposedId(id: string): string {
|
||||
return `${id}-copy`.slice(0, 63);
|
||||
const copy = `${id.slice(0, 58)}-copy`;
|
||||
// A max-length source ending in "-copy" would otherwise reproduce itself.
|
||||
return copy === id ? `${id.slice(0, 61)}-2` : copy;
|
||||
}
|
||||
|
||||
function QueryError({ name, message, retryLabel, onRetry }: {
|
||||
name: string;
|
||||
message: string;
|
||||
retryLabel: string;
|
||||
onRetry: () => void;
|
||||
}) {
|
||||
return <div role="alert" aria-label={name} className="grid gap-2 rounded-md border border-destructive/30 bg-destructive/5 p-3 text-sm"><p>{message}</p><div><Button size="sm" variant="outline" onClick={onRetry}>{retryLabel}</Button></div></div>;
|
||||
}
|
||||
|
||||
export function WorkspaceManager({ open, onClose }: { open: boolean; onClose: () => void }) {
|
||||
@@ -44,13 +55,16 @@ export function WorkspaceManager({ open, onClose }: { open: boolean; onClose: ()
|
||||
const [notice, setNotice] = useState<string>();
|
||||
const [diagnostics, setDiagnostics] = useState<string[]>([]);
|
||||
const [deletionDraft, setDeletionDraft] = useState<WorkspaceDeletionDraft>();
|
||||
const { data: status } = useQuery({ queryKey: ["workspace-registry-status"], queryFn: getWorkspaceRegistryStatus, enabled: open });
|
||||
const { data: workspaces = [], isLoading: workspacesLoading } = useQuery({ queryKey: ["workspaces"], queryFn: listWorkspaces, enabled: open });
|
||||
const { data: record, isLoading: recordLoading } = useQuery({
|
||||
const statusQuery = useQuery({ queryKey: ["workspace-registry-status"], queryFn: getWorkspaceRegistryStatus, enabled: open });
|
||||
const workspacesQuery = useQuery({ queryKey: ["workspaces"], queryFn: listWorkspaces, enabled: open });
|
||||
const detailQuery = useQuery({
|
||||
queryKey: ["workspace", selectedId],
|
||||
queryFn: () => getWorkspace(selectedId!),
|
||||
enabled: Boolean(open && selectedId && !localDraft),
|
||||
});
|
||||
const status = statusQuery.data;
|
||||
const workspaces = workspacesQuery.data ?? [];
|
||||
const record = detailQuery.data;
|
||||
|
||||
const savedDraft = selectedId && !localDraft ? workspaceDrafts.load(selectedId) : undefined;
|
||||
const savedDeletionDraft = selectedId && !deletionDraft ? workspaceDeletionDrafts.load(selectedId) : undefined;
|
||||
@@ -147,26 +161,25 @@ export function WorkspaceManager({ open, onClose }: { open: boolean; onClose: ()
|
||||
<nav aria-label="Workspaces" className="flex min-h-0 flex-col border-r border-border/70 bg-muted/30 p-3">
|
||||
<Button size="sm" className="mb-3 w-full" onClick={createWorkspace}><Plus />New workspace</Button>
|
||||
<div className="min-h-0 flex-1 overflow-y-auto">
|
||||
{workspacesLoading ? <p className="p-2 text-xs text-muted-foreground">Loading workspaces…</p> : workspaces.map((workspace) => (
|
||||
{workspacesQuery.isLoading ? <p className="p-2 text-xs text-muted-foreground">Loading workspaces…</p> : workspacesQuery.isError ? <QueryError name="Workspace list failed" message="Could not load workspaces." retryLabel="Retry workspace list" onRetry={() => { void workspacesQuery.refetch(); }} /> : workspaces.map((workspace) => (
|
||||
<button key={workspace.id} type="button" aria-label={workspace.displayName} aria-current={selectedId === workspace.id ? "page" : undefined} onClick={() => selectWorkspace(workspace.id)} className="mb-1 w-full rounded-md px-2.5 py-2 text-left text-sm hover:bg-muted aria-[current=page]:bg-primary/10 aria-[current=page]:font-semibold">
|
||||
<span className="block truncate">{workspace.displayName}</span>
|
||||
<span className="block truncate text-xs text-muted-foreground">{workspace.id}</span>
|
||||
</button>
|
||||
))}
|
||||
{!workspacesLoading && workspaces.length === 0 && <p className="p-2 text-xs text-muted-foreground">No published workspaces.</p>}
|
||||
{!workspacesQuery.isLoading && !workspacesQuery.isError && workspaces.length === 0 && <p className="p-2 text-xs text-muted-foreground">No published workspaces.</p>}
|
||||
</div>
|
||||
<div className="border-t border-border/70 pt-3 text-xs text-muted-foreground">
|
||||
<p className="font-semibold text-foreground">Git status</p>
|
||||
<p>{status?.branch ?? "Loading…"}</p>
|
||||
{status && <p>{status.degraded ? "Degraded" : "Current"} · ↑{status.ahead} ↓{status.behind}</p>}
|
||||
{statusQuery.isError ? <QueryError name="Workspace registry status failed" message="Could not load registry status." retryLabel="Retry registry status" onRetry={() => { void statusQuery.refetch(); }} /> : statusQuery.isLoading ? <p>Loading…</p> : status && <><p>{status.branch}</p><p>{status.degraded ? "Degraded" : "Current"} · ↑{status.ahead} ↓{status.behind}</p></>}
|
||||
</div>
|
||||
</nav>
|
||||
|
||||
<div className="min-w-0 overflow-y-auto px-5 py-5">
|
||||
{!currentDraft && !recordLoading && <div className="grid min-h-64 place-items-center text-center"><div><h3 className="font-heading font-semibold">Select a workspace</h3><p className="mt-1 text-sm text-muted-foreground">Review an existing definition or start a browser-only draft.</p></div></div>}
|
||||
{(currentDraft || recordLoading) && (
|
||||
{detailQuery.isError && selectedId && !localDraft ? <QueryError name="Workspace details failed" message="Could not load workspace details." retryLabel="Retry workspace details" onRetry={() => { void detailQuery.refetch(); }} /> : !currentDraft && !detailQuery.isLoading && <div className="grid min-h-64 place-items-center text-center"><div><h3 className="font-heading font-semibold">Select a workspace</h3><p className="mt-1 text-sm text-muted-foreground">Review an existing definition or start a browser-only draft.</p></div></div>}
|
||||
{!detailQuery.isError && (currentDraft || detailQuery.isLoading) && (
|
||||
<>
|
||||
{recordLoading && !currentDraft ? <p className="text-sm text-muted-foreground">Loading workspace definition…</p> : currentDraft && <>
|
||||
{detailQuery.isLoading && !currentDraft ? <p className="text-sm text-muted-foreground">Loading workspace definition…</p> : currentDraft && <>
|
||||
<div className="mb-5 flex flex-wrap items-start justify-between gap-3 border-b border-border/70 pb-4">
|
||||
<div>
|
||||
<p className="thot-label">Workspace definition</p>
|
||||
|
||||
Reference in New Issue
Block a user