fix: close final draft authoring review gaps
This commit is contained in:
@@ -33,6 +33,7 @@ export type CapabilityNodeFormProps = {
|
||||
readonly onValueChange?: (value: CapabilityNodeFormValue) => void;
|
||||
readonly onDirtyChange?: (dirty: boolean) => void;
|
||||
readonly submitLabel?: string;
|
||||
readonly stepIdReadOnly?: boolean;
|
||||
readonly hidden?: boolean;
|
||||
};
|
||||
|
||||
@@ -48,6 +49,7 @@ export const CapabilityNodeForm = ({
|
||||
onValueChange,
|
||||
onDirtyChange,
|
||||
submitLabel = "Add node",
|
||||
stepIdReadOnly = false,
|
||||
hidden = false,
|
||||
}: CapabilityNodeFormProps) => {
|
||||
const stepIdRef = useRef<HTMLInputElement>(null);
|
||||
@@ -125,8 +127,9 @@ export const CapabilityNodeForm = ({
|
||||
<input
|
||||
aria-label="Step id"
|
||||
defaultValue={initialValue?.stepId ?? ""}
|
||||
readOnly={stepIdReadOnly}
|
||||
ref={stepIdRef}
|
||||
onChange={(event) => {
|
||||
onChange={() => {
|
||||
notifyMetadataChange();
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -1,11 +1,15 @@
|
||||
import { cleanup, render, screen } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import type { CapabilityDetail } from "../domain/capability-models.js";
|
||||
import type { DraftWorkspace } from "../domain/draft-workspace-models.js";
|
||||
import type { DraftAuthoringController } from "./useDraftAuthoring.js";
|
||||
import { ContextInspector } from "./ContextInspector.js";
|
||||
|
||||
afterEach(() => cleanup());
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
const draft: DraftWorkspace = {
|
||||
workspaceId: "draft-report",
|
||||
@@ -92,6 +96,7 @@ describe("ContextInspector", () => {
|
||||
);
|
||||
|
||||
expect(screen.getByRole("textbox", { name: "Description" })).toHaveValue("Read the report");
|
||||
expect(screen.getByRole("textbox", { name: "Step id" })).toHaveAttribute("readonly");
|
||||
expect(screen.getByRole("spinbutton", { name: "Retry" })).toHaveValue(2);
|
||||
expect(screen.getByRole("spinbutton", { name: "Timeout seconds" })).toHaveValue(45);
|
||||
expect(screen.getByRole("textbox", { name: "Title" })).toHaveValue("Existing title");
|
||||
@@ -99,6 +104,39 @@ describe("ContextInspector", () => {
|
||||
expect(screen.getAllByText("Bind")).not.toHaveLength(0);
|
||||
});
|
||||
|
||||
it("keeps deferred actions focusable while keyboard activation does not dispatch", async () => {
|
||||
const user = userEvent.setup();
|
||||
render(
|
||||
<ContextInspector
|
||||
capabilities={[]}
|
||||
capabilityDetail={detail}
|
||||
capabilityDetailMessage={null}
|
||||
capabilityDetailPhase="ready"
|
||||
controller={controller}
|
||||
draft={draft}
|
||||
selection={{ kind: "node", nodeId: "read" }}
|
||||
/>,
|
||||
);
|
||||
|
||||
const deferred = screen.getAllByRole("button", { name: /Later/ });
|
||||
expect(screen.getByText("These actions are not available in this workbench yet.")).toBeInTheDocument();
|
||||
expect(deferred).toHaveLength(6);
|
||||
deferred[0]?.focus();
|
||||
for (const [index, action] of deferred.entries()) {
|
||||
if (index > 0) await user.tab();
|
||||
expect(document.activeElement).toBe(action);
|
||||
expect(action).toHaveAttribute("aria-disabled", "true");
|
||||
expect(action).not.toBeDisabled();
|
||||
await user.keyboard("{Enter}");
|
||||
await user.keyboard(" ");
|
||||
}
|
||||
|
||||
expect(controller.addCapability).not.toHaveBeenCalled();
|
||||
expect(controller.updateCapability).not.toHaveBeenCalled();
|
||||
expect(controller.setRoute).not.toHaveBeenCalled();
|
||||
expect(controller.validate).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("maps server diagnostics to the selected node form", () => {
|
||||
const diagnosticDraft: DraftWorkspace = {
|
||||
...draft,
|
||||
|
||||
@@ -211,8 +211,15 @@ const RawDraft = ({ draft }: { readonly draft: DraftWorkspace["draft"] }) => (
|
||||
);
|
||||
|
||||
const DeferredActions = () => (
|
||||
<section className="authoring-inspector__deferred" aria-labelledby="deferred-actions-heading">
|
||||
<section
|
||||
aria-describedby="deferred-actions-description"
|
||||
aria-labelledby="deferred-actions-heading"
|
||||
className="authoring-inspector__deferred"
|
||||
>
|
||||
<h3 id="deferred-actions-heading">Deferred actions</h3>
|
||||
<p id="deferred-actions-description">
|
||||
These actions are not available in this workbench yet.
|
||||
</p>
|
||||
<div className="authoring-inspector__deferred-actions">
|
||||
{[
|
||||
"Undo — Later",
|
||||
@@ -221,7 +228,17 @@ const DeferredActions = () => (
|
||||
"Delete route — Later",
|
||||
"Add other step — Later",
|
||||
"Create artifact — Later",
|
||||
].map((label) => <button disabled key={label} type="button">{label}</button>)}
|
||||
].map((label) => (
|
||||
<button
|
||||
aria-describedby="deferred-actions-description"
|
||||
aria-disabled="true"
|
||||
key={label}
|
||||
onClick={(event) => event.preventDefault()}
|
||||
type="button"
|
||||
>
|
||||
{label}
|
||||
</button>
|
||||
))}
|
||||
</div>
|
||||
</section>
|
||||
);
|
||||
@@ -342,6 +359,7 @@ export const ContextInspector = ({
|
||||
onDirtyChange={controller.markDirty}
|
||||
onSubmit={controller.updateCapability}
|
||||
onValueChange={(value) => controller.rememberCapabilityForm("update", value)}
|
||||
stepIdReadOnly
|
||||
submitLabel="Apply changes"
|
||||
/>
|
||||
);
|
||||
|
||||
@@ -116,8 +116,11 @@ describe("DraftWorkbench", () => {
|
||||
"Add other step — Later",
|
||||
"Create artifact — Later",
|
||||
]) {
|
||||
expect(screen.getByRole("button", { name: label })).toBeDisabled();
|
||||
const action = screen.getByRole("button", { name: label });
|
||||
expect(action).toHaveAttribute("aria-disabled", "true");
|
||||
expect(action).not.toBeDisabled();
|
||||
}
|
||||
expect(screen.getByText("These actions are not available in this workbench yet.")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("uses named mobile sheets, keeps selection persistent, and returns focus on close", async () => {
|
||||
|
||||
@@ -213,6 +213,26 @@ describe("useDraftAuthoring", () => {
|
||||
expect(result.current.draft.status).toBe("valid");
|
||||
});
|
||||
|
||||
it("targets the selected node when submitted metadata contains a different step id", async () => {
|
||||
const initial = workspace({
|
||||
draft: { steps: { read: { use: "demo.read" } }, routes: {} },
|
||||
summary: { name: "report", start: "read", stepCount: 1, routeCount: 0, steps: ["read"] },
|
||||
});
|
||||
updateCapabilityStep.mockResolvedValue(workspace({ revision: 4 }));
|
||||
const { result } = renderHook(() =>
|
||||
useDraftAuthoring({
|
||||
draft: initial,
|
||||
initialSelection: { kind: "node", nodeId: "read" },
|
||||
}),
|
||||
);
|
||||
|
||||
await act(async () => result.current.updateCapability({ ...capabilityInput, stepId: "other" }));
|
||||
|
||||
expect(updateCapabilityStep).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ stepId: "read" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves dirty form ownership on ordinary failures and revision conflicts", async () => {
|
||||
const initial = workspace();
|
||||
authoringClient.addCapabilityStep.mockRejectedValueOnce(new Error("server unavailable"));
|
||||
|
||||
@@ -168,11 +168,13 @@ export const useDraftAuthoring = ({
|
||||
readExecutor,
|
||||
}), [connectedTarget, draft.workspaceId, readExecutor, writeExecutor]);
|
||||
const currentDraftRef = useRef(draft);
|
||||
const currentSelectionRef = useRef(selection);
|
||||
const currentInsertionContextRef = useRef(insertionContext);
|
||||
const currentProvenanceRef = useRef(currentProvenance);
|
||||
|
||||
useEffect(() => {
|
||||
currentDraftRef.current = draft;
|
||||
currentSelectionRef.current = selection;
|
||||
currentInsertionContextRef.current = insertionContext;
|
||||
currentProvenanceRef.current = currentProvenance;
|
||||
}, [currentProvenance, draft, insertionContext, selection]);
|
||||
@@ -332,6 +334,12 @@ export const useDraftAuthoring = ({
|
||||
const updateCapability = useCallback(
|
||||
(input: CapabilityNodeFormValue): Promise<void> => {
|
||||
lastSubmissionRef.current = { kind: "update", input };
|
||||
// The form's Step id is display-only for updates; mutation targets stay
|
||||
// bound to the node selected when the operator opened the inspector.
|
||||
const selectedNodeId =
|
||||
currentSelectionRef.current.kind === "node"
|
||||
? currentSelectionRef.current.nodeId
|
||||
: input.stepId;
|
||||
return runMutation(
|
||||
"update",
|
||||
input,
|
||||
@@ -339,7 +347,7 @@ export const useDraftAuthoring = ({
|
||||
client.updateCapabilityStep({
|
||||
workspaceId: requestDraft.workspaceId,
|
||||
revision: requestDraft.revision,
|
||||
stepId: input.stepId,
|
||||
stepId: selectedNodeId,
|
||||
update: {
|
||||
description: input.description,
|
||||
input: input.inputBindings,
|
||||
|
||||
@@ -126,6 +126,29 @@ const LeafControl = ({
|
||||
id,
|
||||
};
|
||||
if (field.kind === "boolean") {
|
||||
if (field.required && !field.hasDefault) {
|
||||
const selectedValue = value === true ? "true" : value === false ? "false" : "";
|
||||
return (
|
||||
<select
|
||||
{...common}
|
||||
aria-label={label}
|
||||
onChange={(event) => {
|
||||
onValueChange(
|
||||
event.target.value === "true"
|
||||
? true
|
||||
: event.target.value === "false"
|
||||
? false
|
||||
: undefined,
|
||||
);
|
||||
}}
|
||||
value={selectedValue}
|
||||
>
|
||||
<option value="">Choose true or false</option>
|
||||
<option value="true">true</option>
|
||||
<option value="false">false</option>
|
||||
</select>
|
||||
);
|
||||
}
|
||||
return (
|
||||
<input
|
||||
{...common}
|
||||
|
||||
@@ -160,6 +160,49 @@ describe("SchemaForm", () => {
|
||||
expect(explicit[0]?.value).toEqual({ enabled: false });
|
||||
});
|
||||
|
||||
it("offers an unset, true, and false choice for a required boolean without a default", async () => {
|
||||
const user = userEvent.setup();
|
||||
const submissions: SchemaSerializationResult[] = [];
|
||||
render(
|
||||
<SchemaForm
|
||||
onSubmit={(result) => submissions.push(result)}
|
||||
schema={{
|
||||
type: "object",
|
||||
properties: { enabled: { type: "boolean" } },
|
||||
required: ["enabled"],
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
const enabled = screen.getByRole("combobox", { name: "Enabled" });
|
||||
expect(enabled).toHaveValue("");
|
||||
expect(screen.getByRole("option", { name: "Choose true or false" })).toBeInTheDocument();
|
||||
await user.selectOptions(enabled, "false");
|
||||
await user.click(screen.getByRole("button", { name: "Save form" }));
|
||||
expect(submissions[0]?.value).toEqual({ enabled: false });
|
||||
expect(submissions[0]?.issues).toEqual([]);
|
||||
});
|
||||
|
||||
it("serializes the true choice for a required boolean without a default", async () => {
|
||||
const user = userEvent.setup();
|
||||
const submissions: SchemaSerializationResult[] = [];
|
||||
render(
|
||||
<SchemaForm
|
||||
onSubmit={(result) => submissions.push(result)}
|
||||
schema={{
|
||||
type: "object",
|
||||
properties: { enabled: { type: "boolean" } },
|
||||
required: ["enabled"],
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
await user.selectOptions(screen.getByRole("combobox", { name: "Enabled" }), "true");
|
||||
await user.click(screen.getByRole("button", { name: "Save form" }));
|
||||
expect(submissions[0]?.value).toEqual({ enabled: true });
|
||||
expect(submissions[0]?.issues).toEqual([]);
|
||||
});
|
||||
|
||||
it("routes nested diagnostics only to their owning field", () => {
|
||||
render(
|
||||
<SchemaForm
|
||||
|
||||
@@ -235,6 +235,26 @@ describe("serializeSchemaValues", () => {
|
||||
expect(serializeSchemaValues(field, { enabled: false }).value).toEqual({ enabled: false });
|
||||
});
|
||||
|
||||
it("requires an explicit true or false value for a required boolean without a default", () => {
|
||||
const field = normalizeSchema({
|
||||
type: "object",
|
||||
properties: { enabled: { type: "boolean" } },
|
||||
required: ["enabled"],
|
||||
});
|
||||
|
||||
expect(serializeSchemaValues(field, { enabled: undefined }).issues).toEqual([
|
||||
{ path: ["enabled"], message: "Choose true or false." },
|
||||
]);
|
||||
expect(serializeSchemaValues(field, { enabled: true })).toMatchObject({
|
||||
value: { enabled: true },
|
||||
issues: [],
|
||||
});
|
||||
expect(serializeSchemaValues(field, { enabled: false })).toMatchObject({
|
||||
value: { enabled: false },
|
||||
issues: [],
|
||||
});
|
||||
});
|
||||
|
||||
it("reports and preserves missing required string and JSON values", () => {
|
||||
const field = normalizeSchema({
|
||||
type: "object",
|
||||
|
||||
Reference in New Issue
Block a user