fix: harden draft workspace routes
This commit is contained in:
@@ -1,10 +1,16 @@
|
||||
import { cleanup, render, screen } from "@testing-library/react";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { MemoryRouter, Route, Routes } from "react-router-dom";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { DraftWorkspace } from "../domain/draft-workspace-models.js";
|
||||
import type { DraftWorkspaceController } from "./useDraftWorkspace.js";
|
||||
import { useDraftWorkspace } from "./useDraftWorkspace.js";
|
||||
import { DraftDetailRoute } from "./DraftDetailRoute.js";
|
||||
import { DraftDetailRoute, formatBoundedJson } from "./DraftDetailRoute.js";
|
||||
|
||||
const globalStyles = readFileSync(
|
||||
"src/styles/global.css",
|
||||
"utf8",
|
||||
);
|
||||
|
||||
vi.mock("./useDraftWorkspace.js", () => ({
|
||||
useDraftWorkspace: vi.fn(),
|
||||
@@ -101,6 +107,47 @@ describe("DraftDetailRoute", () => {
|
||||
expect(screen.queryByRole("link", { name: /compile|artifact|save|edit|mutate/i })).toBeNull();
|
||||
});
|
||||
|
||||
it("makes the raw JSON region focusable and names its horizontal scrolling behavior", () => {
|
||||
renderRoute();
|
||||
|
||||
const rawJson = screen.getByRole("region", {
|
||||
name: "Raw draft JSON, horizontally scrollable",
|
||||
});
|
||||
expect(rawJson).toHaveAttribute("tabindex", "0");
|
||||
});
|
||||
|
||||
it("bounds JSON traversal without reading remote fields after the budget", () => {
|
||||
const draft = {
|
||||
first: "x".repeat(1_000),
|
||||
get later() {
|
||||
throw new Error("later field should not be read");
|
||||
},
|
||||
};
|
||||
|
||||
expect(() => formatBoundedJson(draft, 80)).not.toThrow();
|
||||
expect(formatBoundedJson(draft, 80)).toHaveLength(80);
|
||||
expect(formatBoundedJson(draft, 80)).toContain("truncated");
|
||||
});
|
||||
|
||||
it("does not render a selected workspace whose identity differs from the URL", () => {
|
||||
mockedUseDraftWorkspace.mockReturnValue(
|
||||
controller({ selected: workspace({ workspaceId: "draft-old" }) }),
|
||||
);
|
||||
renderRoute("draft-new");
|
||||
|
||||
expect(screen.queryByRole("heading", { name: "Quarterly report" })).toBeNull();
|
||||
expect(screen.getByText("draft-new")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("keeps detail panels stackable at the mobile workspace breakpoint", () => {
|
||||
renderRoute();
|
||||
|
||||
expect(document.querySelector(".draft-detail__panels")).toBeInTheDocument();
|
||||
expect(globalStyles).toMatch(
|
||||
/@media \(max-width: 850px\)[\s\S]*?\.draft-detail__panels\s*\{[\s\S]*?grid-template-columns:\s*1fr/,
|
||||
);
|
||||
});
|
||||
|
||||
it("explains when the full draft document was not returned", () => {
|
||||
mockedUseDraftWorkspace.mockReturnValue(
|
||||
controller({ selected: workspace({ draft: null }) }),
|
||||
|
||||
@@ -6,6 +6,7 @@ import type {
|
||||
import { useDraftWorkspace } from "./useDraftWorkspace.js";
|
||||
|
||||
const MAX_RAW_DRAFT_CHARS = 12_000;
|
||||
const TRUNCATION_MARKER = "... truncated ...";
|
||||
|
||||
const titleFor = (workspace: DraftWorkspace): string =>
|
||||
workspace.title?.trim() || workspace.workspaceId;
|
||||
@@ -19,10 +20,77 @@ const formatValue = (value: unknown): string => {
|
||||
return encoded ?? String(value);
|
||||
};
|
||||
|
||||
const boundedJson = (value: Record<string, unknown>): string => {
|
||||
const encoded = JSON.stringify(value, null, 2);
|
||||
if (encoded.length <= MAX_RAW_DRAFT_CHARS) return encoded;
|
||||
return `${encoded.slice(0, MAX_RAW_DRAFT_CHARS)}\n... truncated ...`;
|
||||
// Traverse until the display budget is exhausted so a large remote object is
|
||||
// never fully materialized just to produce a clipped escape-hatch preview.
|
||||
export const formatBoundedJson = (value: unknown, maxChars = MAX_RAW_DRAFT_CHARS): string => {
|
||||
const truncationMarker = TRUNCATION_MARKER.slice(0, Math.max(0, maxChars));
|
||||
const contentLimit = Math.max(0, maxChars - truncationMarker.length);
|
||||
let output = "";
|
||||
let truncated = false;
|
||||
const activeObjects = new WeakSet<object>();
|
||||
|
||||
const append = (chunk: string): void => {
|
||||
if (truncated) return;
|
||||
if (output.length + chunk.length > contentLimit) {
|
||||
output += chunk.slice(0, Math.max(0, contentLimit - output.length));
|
||||
truncated = true;
|
||||
return;
|
||||
}
|
||||
output += chunk;
|
||||
};
|
||||
|
||||
const visit = (current: unknown, depth: number): void => {
|
||||
if (truncated) return;
|
||||
if (current === null || typeof current !== "object") {
|
||||
if (typeof current === "string") {
|
||||
append(JSON.stringify(current));
|
||||
} else if (typeof current === "number") {
|
||||
append(Number.isFinite(current) ? String(current) : "null");
|
||||
} else if (typeof current === "boolean") {
|
||||
append(current ? "true" : "false");
|
||||
} else {
|
||||
append("null");
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
if (activeObjects.has(current)) {
|
||||
append('"[Circular]"');
|
||||
return;
|
||||
}
|
||||
activeObjects.add(current);
|
||||
const indent = " ".repeat(depth);
|
||||
const childIndent = " ".repeat(depth + 1);
|
||||
|
||||
if (Array.isArray(current)) {
|
||||
append("[");
|
||||
let first = true;
|
||||
for (const item of current) {
|
||||
if (truncated) break;
|
||||
append(first ? `\n${childIndent}` : `,\n${childIndent}`);
|
||||
visit(item, depth + 1);
|
||||
first = false;
|
||||
}
|
||||
if (!truncated) append(first ? "]" : `\n${indent}]`);
|
||||
} else {
|
||||
const record = current as Record<string, unknown>;
|
||||
append("{");
|
||||
let first = true;
|
||||
for (const key in record) {
|
||||
if (!Object.prototype.hasOwnProperty.call(current, key) || truncated) continue;
|
||||
append(first ? `\n${childIndent}` : `,\n${childIndent}`);
|
||||
append(JSON.stringify(key));
|
||||
append(": ");
|
||||
visit(record[key], depth + 1);
|
||||
first = false;
|
||||
}
|
||||
if (!truncated) append(first ? "}" : `\n${indent}}`);
|
||||
}
|
||||
activeObjects.delete(current);
|
||||
};
|
||||
|
||||
visit(value, 0);
|
||||
return truncated ? `${output}${truncationMarker}` : output;
|
||||
};
|
||||
|
||||
const Fact = ({ label, value }: { readonly label: string; readonly value: string }) => (
|
||||
@@ -81,7 +149,13 @@ const RawDraft = ({ draft }: { readonly draft: Record<string, unknown> | null })
|
||||
<details className="draft-detail__raw">
|
||||
<summary>Raw draft document</summary>
|
||||
{draft ? (
|
||||
<pre>{boundedJson(draft)}</pre>
|
||||
<pre
|
||||
aria-label="Raw draft JSON, horizontally scrollable"
|
||||
role="region"
|
||||
tabIndex={0}
|
||||
>
|
||||
{formatBoundedJson(draft)}
|
||||
</pre>
|
||||
) : (
|
||||
<p>Full draft document was not returned</p>
|
||||
)}
|
||||
@@ -91,7 +165,8 @@ const RawDraft = ({ draft }: { readonly draft: Record<string, unknown> | null })
|
||||
export const DraftDetailRoute = () => {
|
||||
const { workspaceId = null } = useParams<{ workspaceId: string }>();
|
||||
const drafts = useDraftWorkspace(workspaceId);
|
||||
const draft = drafts.selected;
|
||||
const draft =
|
||||
drafts.selected?.workspaceId === workspaceId ? drafts.selected : null;
|
||||
|
||||
return (
|
||||
<div className="draft-detail">
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { act, renderHook, waitFor } from "@testing-library/react";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { act, cleanup, renderHook, waitFor } from "@testing-library/react";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { ConnectionState } from "../../app/state.js";
|
||||
import { useConsoleWorkspace } from "../context.js";
|
||||
import type { DraftWorkspaceClient } from "../domain/draft-workspace-client.js";
|
||||
@@ -87,6 +87,8 @@ beforeEach(() => {
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => cleanup());
|
||||
|
||||
describe("useDraftWorkspace", () => {
|
||||
it("loads the list and URL-owned detail through the draft client", async () => {
|
||||
client.list.mockResolvedValue(page([workspace("draft-report")]));
|
||||
@@ -127,6 +129,32 @@ describe("useDraftWorkspace", () => {
|
||||
expect(result.current.selected?.workspaceId).not.toBe("draft-first");
|
||||
});
|
||||
|
||||
it("does not expose a loaded detail during URL or target transitions", async () => {
|
||||
client.list.mockResolvedValue(page([]));
|
||||
client.load
|
||||
.mockResolvedValueOnce(workspace("draft-first"))
|
||||
.mockReturnValueOnce(deferred<DraftWorkspace>().promise)
|
||||
.mockReturnValueOnce(deferred<DraftWorkspace>().promise);
|
||||
|
||||
const { result, rerender } = renderHook(
|
||||
({ workspaceId }: { workspaceId: string | null }) => useDraftWorkspace(workspaceId),
|
||||
{ initialProps: { workspaceId: "draft-first" } },
|
||||
);
|
||||
await waitFor(() => expect(result.current.selected?.workspaceId).toBe("draft-first"));
|
||||
|
||||
mockedUseConsoleWorkspace.mockReturnValue({
|
||||
connection: connectedState,
|
||||
connectedTarget: "http://new-workflow.example/rpc",
|
||||
recordEvidence: vi.fn(),
|
||||
readExecutor: {} as ConsoleReadExecutor,
|
||||
});
|
||||
rerender({ workspaceId: "draft-first" });
|
||||
expect(result.current.selected).toBeNull();
|
||||
|
||||
rerender({ workspaceId: "draft-second" });
|
||||
expect(result.current.selected).toBeNull();
|
||||
});
|
||||
|
||||
it("preserves the loaded list while refresh reloads the relevant reads", async () => {
|
||||
client.list
|
||||
.mockResolvedValueOnce(page([workspace("draft-old")]))
|
||||
@@ -138,13 +166,43 @@ describe("useDraftWorkspace", () => {
|
||||
const { result } = renderHook(() => useDraftWorkspace("draft-old"));
|
||||
await waitFor(() => expect(result.current.selected?.workspaceId).toBe("draft-old"));
|
||||
|
||||
act(() => result.current.refresh());
|
||||
act(() => {
|
||||
result.current.refresh();
|
||||
result.current.refresh();
|
||||
});
|
||||
|
||||
expect(result.current.items[0]?.workspaceId).toBe("draft-old");
|
||||
expect(client.list).toHaveBeenCalledTimes(2);
|
||||
expect(client.load).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("coalesces a simultaneous target and URL change into one detail read", async () => {
|
||||
client.list
|
||||
.mockResolvedValueOnce(page([workspace("draft-old")]))
|
||||
.mockResolvedValueOnce(page([workspace("draft-new")]));
|
||||
client.load
|
||||
.mockResolvedValueOnce(workspace("draft-old"))
|
||||
.mockResolvedValueOnce(workspace("draft-new"));
|
||||
|
||||
const { result, rerender } = renderHook(
|
||||
({ workspaceId }: { workspaceId: string | null }) => useDraftWorkspace(workspaceId),
|
||||
{ initialProps: { workspaceId: "draft-old" } },
|
||||
);
|
||||
await waitFor(() => expect(result.current.selected?.workspaceId).toBe("draft-old"));
|
||||
|
||||
mockedUseConsoleWorkspace.mockReturnValue({
|
||||
connection: connectedState,
|
||||
connectedTarget: "http://new-workflow.example/rpc",
|
||||
recordEvidence: vi.fn(),
|
||||
readExecutor: {} as ConsoleReadExecutor,
|
||||
});
|
||||
rerender({ workspaceId: "draft-new" });
|
||||
|
||||
await waitFor(() => expect(result.current.selected?.workspaceId).toBe("draft-new"));
|
||||
expect(client.list).toHaveBeenCalledTimes(2);
|
||||
expect(client.load).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it("clears stale data and reloads both reads after reconnect", async () => {
|
||||
client.list
|
||||
.mockResolvedValueOnce(page([workspace("draft-old")]))
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
type DraftWorkspaceClient,
|
||||
} from "../domain/draft-workspace-client.js";
|
||||
import type { DraftWorkspace } from "../domain/draft-workspace-models.js";
|
||||
import type { ConsoleReadExecutor } from "../domain/read-executor.js";
|
||||
|
||||
export type DraftLoadPhase =
|
||||
| "disconnected"
|
||||
@@ -23,7 +24,16 @@ export type DraftWorkspaceController = {
|
||||
readonly refresh: () => void;
|
||||
};
|
||||
|
||||
type DraftWorkspaceState = Omit<DraftWorkspaceController, "refresh">;
|
||||
type StoredDraftSelection = {
|
||||
readonly workspace: DraftWorkspace;
|
||||
readonly workspaceId: string;
|
||||
readonly connectedTarget: string;
|
||||
readonly connectionGeneration: number;
|
||||
};
|
||||
|
||||
type DraftWorkspaceState = Omit<DraftWorkspaceController, "refresh" | "selected"> & {
|
||||
readonly selected: StoredDraftSelection | null;
|
||||
};
|
||||
|
||||
const initialState: DraftWorkspaceState = {
|
||||
listPhase: "disconnected",
|
||||
@@ -48,13 +58,35 @@ export const useDraftWorkspace = (
|
||||
const [state, setState] = useState<DraftWorkspaceState>(initialState);
|
||||
const listGenerationRef = useRef(0);
|
||||
const detailGenerationRef = useRef(0);
|
||||
const previousWorkspaceIdRef = useRef(workspaceId);
|
||||
const currentWorkspaceIdRef = useRef(workspaceId);
|
||||
currentWorkspaceIdRef.current = workspaceId;
|
||||
const listPendingRef = useRef(false);
|
||||
const detailPendingRef = useRef(false);
|
||||
const observedRequestRef = useRef<{
|
||||
readonly readExecutor: ConsoleReadExecutor | null;
|
||||
readonly connectedTarget: string | null;
|
||||
readonly workspaceId: string | null;
|
||||
} | null>(null);
|
||||
const connectionSignatureRef = useRef<{
|
||||
readonly readExecutor: ConsoleReadExecutor | null;
|
||||
readonly connectedTarget: string | null;
|
||||
} | null>(null);
|
||||
const connectionGenerationRef = useRef(0);
|
||||
const connectionSignature = { readExecutor, connectedTarget };
|
||||
const previousConnection = connectionSignatureRef.current;
|
||||
if (
|
||||
previousConnection === null ||
|
||||
previousConnection.readExecutor !== readExecutor ||
|
||||
previousConnection.connectedTarget !== connectedTarget
|
||||
) {
|
||||
connectionGenerationRef.current++;
|
||||
connectionSignatureRef.current = connectionSignature;
|
||||
}
|
||||
|
||||
const runList = useCallback(() => {
|
||||
const runList = useCallback((force = false) => {
|
||||
if (!client || !connectedTarget) return;
|
||||
if (listPendingRef.current && !force) return;
|
||||
listPendingRef.current = false;
|
||||
const generation = ++listGenerationRef.current;
|
||||
listPendingRef.current = true;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
listPhase: "loading",
|
||||
@@ -79,13 +111,19 @@ export const useDraftWorkspace = (
|
||||
listPhase: "error",
|
||||
listMessage: errorMessage(error),
|
||||
}));
|
||||
})
|
||||
.finally(() => {
|
||||
if (generation === listGenerationRef.current) listPendingRef.current = false;
|
||||
});
|
||||
}, [client, connectedTarget]);
|
||||
|
||||
const runDetail = useCallback(
|
||||
(nextWorkspaceId: string | null) => {
|
||||
(nextWorkspaceId: string | null, force = false) => {
|
||||
if (detailPendingRef.current && !force) return;
|
||||
detailPendingRef.current = false;
|
||||
const generation = ++detailGenerationRef.current;
|
||||
if (!nextWorkspaceId) {
|
||||
detailPendingRef.current = false;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
detailPhase: client && connectedTarget ? "idle" : "disconnected",
|
||||
@@ -95,6 +133,7 @@ export const useDraftWorkspace = (
|
||||
return;
|
||||
}
|
||||
if (!client || !connectedTarget) {
|
||||
detailPendingRef.current = false;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
detailPhase: "disconnected",
|
||||
@@ -104,6 +143,9 @@ export const useDraftWorkspace = (
|
||||
return;
|
||||
}
|
||||
|
||||
const requestTarget = connectedTarget;
|
||||
const requestConnectionGeneration = connectionGenerationRef.current;
|
||||
detailPendingRef.current = true;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
detailPhase: "loading",
|
||||
@@ -114,11 +156,19 @@ export const useDraftWorkspace = (
|
||||
void client
|
||||
.load(nextWorkspaceId)
|
||||
.then((detail) => {
|
||||
if (generation !== detailGenerationRef.current) return;
|
||||
if (
|
||||
generation !== detailGenerationRef.current ||
|
||||
requestConnectionGeneration !== connectionGenerationRef.current
|
||||
) return;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
detailPhase: "ready",
|
||||
selected: detail,
|
||||
selected: {
|
||||
workspace: detail,
|
||||
workspaceId: nextWorkspaceId,
|
||||
connectedTarget: requestTarget,
|
||||
connectionGeneration: requestConnectionGeneration,
|
||||
},
|
||||
detailMessage: null,
|
||||
}));
|
||||
})
|
||||
@@ -130,15 +180,29 @@ export const useDraftWorkspace = (
|
||||
selected: null,
|
||||
detailMessage: errorMessage(error),
|
||||
}));
|
||||
})
|
||||
.finally(() => {
|
||||
if (generation === detailGenerationRef.current) detailPendingRef.current = false;
|
||||
});
|
||||
},
|
||||
[client, connectedTarget],
|
||||
);
|
||||
|
||||
useEffect(() => {
|
||||
const previousRequest = observedRequestRef.current;
|
||||
const connectionChanged =
|
||||
previousRequest === null ||
|
||||
previousRequest.readExecutor !== readExecutor ||
|
||||
previousRequest.connectedTarget !== connectedTarget;
|
||||
const workspaceChanged =
|
||||
previousRequest !== null && previousRequest.workspaceId !== workspaceId;
|
||||
observedRequestRef.current = { readExecutor, connectedTarget, workspaceId };
|
||||
|
||||
if (!client || !connectedTarget) {
|
||||
listGenerationRef.current++;
|
||||
detailGenerationRef.current++;
|
||||
listPendingRef.current = false;
|
||||
detailPendingRef.current = false;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
listPhase: "disconnected",
|
||||
@@ -151,24 +215,26 @@ export const useDraftWorkspace = (
|
||||
return;
|
||||
}
|
||||
|
||||
// A new executor represents a fresh connection. Do not show data from the
|
||||
// old server while the list and URL-owned detail are being reloaded.
|
||||
setState((current) => ({
|
||||
...current,
|
||||
items: [],
|
||||
selected: null,
|
||||
listMessage: null,
|
||||
detailMessage: null,
|
||||
}));
|
||||
runList();
|
||||
runDetail(currentWorkspaceIdRef.current);
|
||||
}, [client, connectedTarget, runDetail, runList]);
|
||||
|
||||
useEffect(() => {
|
||||
if (previousWorkspaceIdRef.current === workspaceId) return;
|
||||
previousWorkspaceIdRef.current = workspaceId;
|
||||
runDetail(workspaceId);
|
||||
}, [runDetail, workspaceId]);
|
||||
if (connectionChanged) {
|
||||
// A new executor represents a fresh connection. Do not show data from
|
||||
// the old server while the combined URL-owned reads are reloading.
|
||||
listGenerationRef.current++;
|
||||
detailGenerationRef.current++;
|
||||
listPendingRef.current = false;
|
||||
detailPendingRef.current = false;
|
||||
setState((current) => ({
|
||||
...current,
|
||||
items: [],
|
||||
selected: null,
|
||||
listMessage: null,
|
||||
detailMessage: null,
|
||||
}));
|
||||
runList(true);
|
||||
runDetail(workspaceId, true);
|
||||
} else if (workspaceChanged) {
|
||||
runDetail(workspaceId, true);
|
||||
}
|
||||
}, [client, connectedTarget, readExecutor, runDetail, runList, workspaceId]);
|
||||
|
||||
const refresh = useCallback(() => {
|
||||
if (!client || !connectedTarget) return;
|
||||
@@ -176,5 +242,16 @@ export const useDraftWorkspace = (
|
||||
runDetail(workspaceId);
|
||||
}, [client, connectedTarget, runDetail, runList, workspaceId]);
|
||||
|
||||
return { ...state, refresh };
|
||||
const storedSelection = state.selected;
|
||||
const selected =
|
||||
storedSelection !== null &&
|
||||
workspaceId !== null &&
|
||||
connectedTarget !== null &&
|
||||
storedSelection.workspaceId === workspaceId &&
|
||||
storedSelection.connectedTarget === connectedTarget &&
|
||||
storedSelection.connectionGeneration === connectionGenerationRef.current
|
||||
? storedSelection.workspace
|
||||
: null;
|
||||
|
||||
return { ...state, selected, refresh };
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user