fix: address Task 4 review findings

This commit is contained in:
lda
2026-08-04 20:41:24 +07:00 Verified
parent ad3090e5be
commit 6a5af51813
11 changed files with 162 additions and 7 deletions
@@ -1,4 +1,5 @@
import { render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { afterEach, describe, expect, it } from "vitest";
import { cleanup } from "@testing-library/react";
import { MemoryRouter } from "react-router-dom";
@@ -57,4 +58,25 @@ describe("ConsoleShell", () => {
expect(screen.getByRole("link", { name: label })).toHaveAttribute("href", href);
}
});
it("provides a keyboard skip link to the workspace main region", async () => {
render(
<MemoryRouter>
<ConsoleShell
connection={initialState()}
onConnect={() => undefined}
onDraftChange={() => undefined}
>
<p>Content</p>
</ConsoleShell>
</MemoryRouter>,
);
const skipLink = screen.getByRole("link", { name: "Skip to main content" });
expect(skipLink).toHaveAttribute("href", "#console-workspace-main");
await userEvent.tab();
expect(skipLink).toHaveFocus();
});
});
@@ -27,6 +27,9 @@ export const ConsoleShell = ({
children,
}: Props) => (
<div className="console-workspace">
<a className="console-skip-link" href="#console-workspace-main">
Skip to main content
</a>
<header className="console-workspace__header">
<ConnectionHeader
state={connection}
@@ -4,7 +4,7 @@ import { useRef } from "react";
import { MemoryRouter, Outlet, Route, Routes, useNavigate } from "react-router-dom";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { connectToServer, callOperation } from "../connection/api.js";
import type { ConnectResponse } from "../connection/contracts.js";
import type { ConnectResponse, RpcResponse } from "../connection/contracts.js";
import { useConsoleWorkspace } from "./context.js";
import { ConsoleWorkspace } from "./ConsoleWorkspace.js";
@@ -29,6 +29,16 @@ const successfulConnection = (target: string): ConnectResponse => ({
equivalentCli: "uv run wf status",
});
const successfulRead = (): RpcResponse => ({
ok: true,
operation: "workflow.capabilities.list",
label: "List capabilities",
interpreted: { items: [], nextCursor: null, total: 0 },
exchange: { request: {}, response: { status: 200 } },
equivalentCli: "uv run wf cap list",
durationMs: 4,
});
const OutletProbe = () => {
const workspace = useConsoleWorkspace();
const executorIdentity = useRef<NonNullable<typeof workspace.readExecutor> | null>(null);
@@ -43,7 +53,20 @@ const OutletProbe = () => {
<output data-testid="executor-stable">
{workspace.readExecutor === executorIdentity.current ? "yes" : "no"}
</output>
<output data-testid="sources-loading">{String(workspace.connection.sourcesLoading)}</output>
<output data-testid="evidence-ids">
{workspace.connection.evidence.map((record) => record.id).join("|")}
</output>
<button type="button" onClick={() => navigate("/console/drafts")}>Navigate to drafts</button>
<button
type="button"
disabled={workspace.readExecutor === null}
onClick={() => {
void workspace.readExecutor?.run("workflow.capabilities.list", {}, (value) => value);
}}
>
Read capabilities
</button>
<Outlet />
</>
);
@@ -91,6 +114,7 @@ describe("ConsoleWorkspace", () => {
);
expect(screen.getByTestId("executor-state")).toHaveTextContent("available");
expect(screen.getByTestId("executor-stable")).toHaveTextContent("yes");
expect(screen.getByTestId("sources-loading")).toHaveTextContent("false");
expect(screen.getAllByText("Health check")).toHaveLength(1);
expect(mockedCallOperation).not.toHaveBeenCalled();
@@ -101,6 +125,32 @@ describe("ConsoleWorkspace", () => {
expect(screen.getByTestId("connected-target")).toHaveTextContent("http://one.example/rpc");
});
it("keeps health and read evidence ids unique across reconnects", async () => {
mockedConnectToServer.mockResolvedValue(successfulConnection("http://one.example/rpc"));
mockedCallOperation.mockResolvedValue(successfulRead());
renderWorkspace();
const user = userEvent.setup();
await user.click(screen.getByRole("button", { name: "Connect" }));
await user.click(await screen.findByRole("button", { name: "Read capabilities" }));
await waitFor(() => {
expect(screen.getByTestId("evidence-ids")).toHaveTextContent(
"workflow.health-0|workflow.capabilities.list-1",
);
});
await user.click(screen.getByRole("button", { name: "Reconnect" }));
await waitFor(() => {
expect(screen.getByRole("button", { name: "Read capabilities" })).toBeEnabled();
});
await user.click(screen.getByRole("button", { name: "Read capabilities" }));
await waitFor(() => {
const ids = screen.getByTestId("evidence-ids").textContent?.split("|") ?? [];
expect(ids).toHaveLength(4);
expect(new Set(ids).size).toBe(ids.length);
});
});
it("ignores a stale health response after a newer target connects", async () => {
let resolveFirst!: (response: ConnectResponse) => void;
const first = new Promise<ConnectResponse>((resolve) => {
@@ -13,6 +13,7 @@ import type { ConsoleWorkspaceContextValue } from "./context.js";
export const ConsoleWorkspace = () => {
const [state, dispatch] = useReducer(connectionReducer, null, initialState);
const connectGeneration = useRef(0);
const evidenceSequence = useRef(0);
const connectedTarget = state.phase === "connected" ? state.connectedTarget : null;
const recordEvidence = useCallback(
@@ -20,12 +21,21 @@ export const ConsoleWorkspace = () => {
[],
);
const allocateEvidenceId = useCallback(
(operation: string): string => `${operation}-${evidenceSequence.current++}`,
[],
);
const readExecutor = useMemo(
() =>
connectedTarget
? createConsoleReadExecutor({ target: connectedTarget, recordEvidence })
? createConsoleReadExecutor({
target: connectedTarget,
recordEvidence,
allocateEvidenceId,
})
: null,
[connectedTarget, recordEvidence],
[allocateEvidenceId, connectedTarget, recordEvidence],
);
const onDraftChange = useCallback(
@@ -45,7 +55,7 @@ export const ConsoleWorkspace = () => {
dispatch({
type: "evidence_recorded",
record: {
id: `health-${Date.now()}`,
id: allocateEvidenceId("workflow.health"),
operation: "workflow.health",
label: "Health check",
equivalentCli: response.equivalentCli,
@@ -71,7 +81,7 @@ export const ConsoleWorkspace = () => {
});
},
);
}, []);
}, [allocateEvidenceId]);
const workspaceContext = useMemo<ConsoleWorkspaceContextValue>(
() => ({
@@ -140,4 +140,22 @@ describe("ConsoleReadExecutor", () => {
const ids = recordEvidence.mock.calls.map(([record]) => record.id);
expect(new Set(ids).size).toBe(2);
});
it("uses a caller-owned allocator when evidence spans executor lifetimes", async () => {
const recordEvidence = vi.fn();
const allocateEvidenceId = vi.fn((operation: string) => `workspace-${operation}`);
const executor = createConsoleReadExecutor({
target: "http://console.test/rpc",
recordEvidence,
allocateEvidenceId,
invoke: vi.fn(async () => success(null)),
});
await executor.run("workflow.capabilities.list", {}, (value) => value);
expect(allocateEvidenceId).toHaveBeenCalledWith("workflow.capabilities.list");
expect(recordEvidence.mock.calls[0]?.[0]?.id).toBe(
"workspace-workflow.capabilities.list",
);
});
});
@@ -20,6 +20,8 @@ type InvokeOperation = (
params: unknown,
) => Promise<RpcResponse>;
type EvidenceIdAllocator = (operation: string) => string;
const errorKindForCode = (code: string): ConsoleClientErrorKind => {
switch (code) {
case "invalid_target":
@@ -47,10 +49,14 @@ const clientErrorKindForInvocation = (
export const createConsoleReadExecutor = (options: {
readonly target: string;
readonly recordEvidence: (record: EvidenceRecord) => void;
readonly allocateEvidenceId?: EvidenceIdAllocator;
readonly invoke?: InvokeOperation;
}): ConsoleReadExecutor => {
let evidenceSequence = 0;
const invoke = options.invoke ?? callOperation;
const allocateEvidenceId =
options.allocateEvidenceId ??
((operation: string): string => `${operation}-${evidenceSequence++}`);
const record = (
operation: OperationName,
@@ -61,7 +67,7 @@ export const createConsoleReadExecutor = (options: {
durationMs: number,
): void => {
options.recordEvidence({
id: `${operation}-${evidenceSequence++}`,
id: allocateEvidenceId(operation),
operation,
label,
equivalentCli,