fix: close Task 6 review findings

This commit is contained in:
lda
2026-08-14 19:29:39 +07:00 Verified
parent a511740c85
commit 50a7fe3207
11 changed files with 288 additions and 75 deletions
@@ -1,41 +0,0 @@
# Task 6 Report
## Status
Complete. The implementation is based on Task 5 commit `eae1a62f`.
Implementation commit: `92a32df8` (`refactor: use canonical authoring choices`).
## Implementation
- Threaded `useAuthoringContract` inventory data through `SelectedCapabilityInspector`.
- Filtered inventory choices by `step_input`, `step_output_source`, and `state_target` use.
- Replaced input target/source datalists, recursive expression path datalists, and output schema-local controls with `AuthoringPathPicker`.
- Preserved canonical input and output binding serializers. Picker values translate canonical inventory paths to the existing local paths before submission.
- Kept unsupported rows and custom or malformed persisted paths editable. Non-catalog values initialize the picker Advanced section without replacing the persisted value.
- Runtime context choices are rendered only when supplied by the inventory; no runtime context field names are hardcoded in production authoring files.
- Preserved existing row diagnostic accessibility through `aria-invalid` and `aria-describedby` on Advanced custom inputs.
- Kept schema helpers required by validation and output preview; removed `workflowSourceSuggestions` as a production source of choices.
## TDD Evidence
The required RED command was run before production changes:
```powershell
pnpm --dir web --filter @lda/console test -- src/workspace/authoring/StepInputBindingsForm.test.tsx src/workspace/authoring/InputExpressionControl.test.tsx src/workspace/authoring/StepOutputBindingsForm.test.tsx src/workspace/authoring/SelectedCapabilityInspector.test.tsx
```
RED result: 4 test files failed, 4 tests failed, and 60 tests passed because the forms still rendered the old datalists/selects.
The tests then covered inventory-backed source/target choices, recursive expression paths, conditional context visibility, canonical submission, custom-path repair, whole-payload path round-tripping, and the selected inspector inventory seam.
## Verification
- `pnpm --dir web --filter @lda/console test -- src/workspace/authoring`: 19 files passed, 197 tests passed.
- `pnpm --dir web --filter @lda/console typecheck`: passed.
- `npx react-doctor@latest --verbose --scope changed`: score 87/100; six non-blocking warnings were reported in existing picker/component patterns.
- `git diff --check`: passed before commit.
## Concerns
React Doctor reports six non-blocking warnings: mirrored picker state, array lookups, the existing large input form, and empty default props. These patterns were present in the existing authoring seams or are equivalent to the prior suggestion-array defaults; no new blocking diagnostic was reported.
@@ -134,7 +134,7 @@ describe("AuthoringPathPicker", () => {
await user.click(screen.getByRole("button", { name: /Title/ }));
expect(onChange).toHaveBeenCalledTimes(1);
expect(onChange).toHaveBeenCalledWith("input.title");
expect(onChange).toHaveBeenCalledWith("input.title", "catalog");
});
it("gives a conditional incompatible option one composed description node", () => {
@@ -203,7 +203,7 @@ describe("AuthoringPathPicker", () => {
const nested = screen.getByRole("button", { name: /Customer name/ });
nested.focus();
await user.keyboard("{Enter}");
expect(onChange).toHaveBeenCalledWith("input.customer.name");
expect(onChange).toHaveBeenCalledWith("input.customer.name", "catalog");
await user.click(screen.getByText("Advanced"));
expect(screen.getByRole("textbox", { name: "Custom Source path" })).toBeInTheDocument();
@@ -211,6 +211,6 @@ describe("AuthoringPathPicker", () => {
await user.clear(screen.getByRole("textbox", { name: "Custom Source path" }));
await user.type(screen.getByRole("textbox", { name: "Custom Source path" }), "context.future");
expect(onChange).toHaveBeenLastCalledWith("context.future");
expect(onChange).toHaveBeenLastCalledWith("context.future", "custom");
});
});
@@ -5,11 +5,13 @@ import type {
AuthoringPathUse,
} from "../domain/authoring-contract-models.js";
export type AuthoringPathSelection = "catalog" | "custom";
export type AuthoringPathPickerProps = {
readonly options: ReadonlyArray<AuthoringPathOption>;
readonly uses: AuthoringPathUse | ReadonlyArray<AuthoringPathUse>;
readonly value: string;
readonly onChange: (value: string) => void;
readonly onChange: (value: string, selection: AuthoringPathSelection) => void;
readonly label: string;
readonly allowCustom?: boolean;
readonly describedBy?: string | undefined;
@@ -106,7 +108,7 @@ export const AuthoringPathPicker = ({
key={option.path}
onClick={() => {
setCustomValue(option.path);
onChange(option.path);
onChange(option.path, "catalog");
}}
type="button"
>
@@ -138,7 +140,7 @@ export const AuthoringPathPicker = ({
id={customId}
onChange={(event) => {
setCustomValue(event.target.value);
onChange(event.target.value);
onChange(event.target.value, "custom");
}}
type="text"
value={customValue}
@@ -1,16 +1,46 @@
import { cleanup, fireEvent, render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { afterEach, describe, expect, it, vi } from "vitest";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import type { CapabilityDetail } from "../domain/capability-models.js";
import type { AuthoringContractInventory } from "../domain/authoring-contract-models.js";
import type { DraftWorkspace } from "../domain/draft-workspace-models.js";
import type { DraftAuthoringController } from "./useDraftAuthoring.js";
import { ContextInspector } from "./ContextInspector.js";
vi.mock("./useAuthoringContract.js", () => ({ useAuthoringContract: vi.fn() }));
import { useAuthoringContract } from "./useAuthoringContract.js";
const mockedUseAuthoringContract = vi.mocked(useAuthoringContract);
const emptyInventory: AuthoringContractInventory = {
workspaceId: "draft-report",
revision: 3,
selectedStepId: "read",
readableSources: [],
stepInputTargets: [],
stepOutputSources: [],
stateTargets: [],
workflowOutputTargets: [],
entrySteps: [],
workflowOutcomes: [],
warnings: [],
};
afterEach(() => {
cleanup();
vi.clearAllMocks();
});
beforeEach(() => {
mockedUseAuthoringContract.mockReturnValue({
phase: "ready",
inventory: emptyInventory,
message: null,
refresh: vi.fn(),
});
});
const draft: DraftWorkspace = {
workspaceId: "draft-report",
revision: 3,
@@ -2,6 +2,7 @@ import { act, cleanup, fireEvent, render, screen, within } from "@testing-librar
import userEvent from "@testing-library/user-event";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import type { CapabilityDetail } from "../domain/capability-models.js";
import type { AuthoringContractInventory } from "../domain/authoring-contract-models.js";
import type { DraftWorkspace } from "../domain/draft-workspace-models.js";
import { DraftWorkbench } from "./DraftWorkbench.js";
@@ -9,9 +10,27 @@ vi.mock("./useAuthoringCapabilityDetail.js", () => ({
useAuthoringCapabilityDetail: vi.fn(),
}));
vi.mock("./useAuthoringContract.js", () => ({ useAuthoringContract: vi.fn() }));
import { useAuthoringCapabilityDetail } from "./useAuthoringCapabilityDetail.js";
import { useAuthoringContract } from "./useAuthoringContract.js";
const mockedUseAuthoringCapabilityDetail = vi.mocked(useAuthoringCapabilityDetail);
const mockedUseAuthoringContract = vi.mocked(useAuthoringContract);
const emptyInventory: AuthoringContractInventory = {
workspaceId: "draft-review",
revision: 2,
selectedStepId: "collect",
readableSources: [],
stepInputTargets: [],
stepOutputSources: [],
stateTargets: [],
workflowOutputTargets: [],
entrySteps: [],
workflowOutcomes: [],
warnings: [],
};
const workspace: DraftWorkspace = {
workspaceId: "draft-review",
@@ -86,6 +105,12 @@ beforeEach(() => {
detail: null,
message: null,
});
mockedUseAuthoringContract.mockReturnValue({
phase: "ready",
inventory: emptyInventory,
message: null,
refresh: vi.fn(),
});
});
describe("DraftWorkbench", () => {
@@ -60,10 +60,18 @@ const inventory: AuthoringContractInventory = {
warnings: [],
};
const emptyInventory: AuthoringContractInventory = {
...inventory,
readableSources: [],
stepInputTargets: [],
stepOutputSources: [],
stateTargets: [],
};
beforeEach(() => {
mockedUseAuthoringContract.mockReturnValue({
phase: "disconnected",
inventory: null,
phase: "ready",
inventory: emptyInventory,
message: null,
refresh: vi.fn(),
});
@@ -147,6 +155,98 @@ const controllerFor = (workspace: DraftWorkspace): DraftAuthoringController => (
});
describe("SelectedCapabilityInspector", () => {
it("shows loading feedback and gates binding editors without current inventory", async () => {
const user = userEvent.setup();
mockedUseAuthoringContract.mockReturnValue({
phase: "loading",
inventory: null,
message: null,
refresh: vi.fn(),
});
const workspace = draft("read", [], []);
render(
<SelectedCapabilityInspector
capabilityDetail={detail}
capabilityDetailMessage={null}
capabilityDetailPhase="ready"
controller={controllerFor(workspace)}
draft={workspace}
nodeKind="use"
nodeRef="demo.read"
stepId="read"
/>,
);
expect(screen.getByRole("status")).toHaveTextContent("Loading canonical authoring choices...");
await user.click(screen.getByRole("tab", { name: "Inputs" }));
expect(screen.getByText("Input bindings are unavailable until canonical authoring choices are ready.")).toBeInTheDocument();
expect(screen.queryByRole("button", { name: "Save inputs" })).not.toBeInTheDocument();
});
it("shows initial inventory errors with a retry action and gates binding editors", async () => {
const user = userEvent.setup();
const refresh = vi.fn();
mockedUseAuthoringContract.mockReturnValue({
phase: "error",
inventory: null,
message: "Catalog request failed.",
refresh,
});
const workspace = draft("read", [], []);
render(
<SelectedCapabilityInspector
capabilityDetail={detail}
capabilityDetailMessage={null}
capabilityDetailPhase="ready"
controller={controllerFor(workspace)}
draft={workspace}
nodeKind="use"
nodeRef="demo.read"
stepId="read"
/>,
);
expect(screen.getByRole("alert")).toHaveTextContent("Catalog request failed.");
await user.click(screen.getByRole("button", { name: "Retry canonical authoring choices" }));
expect(refresh).toHaveBeenCalledOnce();
await user.click(screen.getByRole("tab", { name: "Outputs" }));
expect(screen.getByText("Output bindings are unavailable until canonical authoring choices are ready.")).toBeInTheDocument();
expect(screen.queryByRole("button", { name: "Save outputs" })).not.toBeInTheDocument();
});
it("retains binding editors and exposes retry feedback when inventory refresh fails", async () => {
const user = userEvent.setup();
const refresh = vi.fn();
mockedUseAuthoringContract.mockReturnValue({
phase: "error",
inventory,
message: "Refresh failed.",
refresh,
});
const workspace = draft("read", [], []);
render(
<SelectedCapabilityInspector
capabilityDetail={detail}
capabilityDetailMessage={null}
controller={controllerFor(workspace)}
draft={workspace}
nodeKind="use"
nodeRef="demo.read"
stepId="read"
capabilityDetailPhase="ready"
/>,
);
expect(screen.getByRole("alert")).toHaveTextContent("Refresh failed.");
await user.click(screen.getByRole("button", { name: "Retry canonical authoring choices" }));
expect(refresh).toHaveBeenCalledOnce();
await user.click(screen.getByRole("tab", { name: "Inputs" }));
expect(screen.getByRole("button", { name: "Save inputs" })).toBeInTheDocument();
});
it("threads inventory-backed pickers into the selected step editors", async () => {
const user = userEvent.setup();
mockedUseAuthoringContract.mockReturnValue({
@@ -70,6 +70,41 @@ const inspectorTabs = Object.keys(tabLabels) as InspectorTab[];
const tabPanelId = (tab: InspectorTab): string => `selected-step-panel-${tab}`;
const tabId = (tab: InspectorTab): string => `selected-step-tab-${tab}`;
const InventoryStatus = ({
inventoryAvailable,
message,
phase,
refresh,
}: {
readonly inventoryAvailable: boolean;
readonly message: string | null;
readonly phase: "disconnected" | "idle" | "loading" | "ready" | "error";
readonly refresh: () => void;
}) => {
if (phase === "loading") {
return <p role="status">
{inventoryAvailable
? "Refreshing canonical authoring choices..."
: "Loading canonical authoring choices..."}
</p>;
}
if (phase === "error") {
return (
<div role="alert">
<p>{message ?? "Canonical authoring choices failed to load."}</p>
<button onClick={refresh} type="button">Retry canonical authoring choices</button>
</div>
);
}
if (!inventoryAvailable && phase === "disconnected") {
return <p role="status">Canonical authoring choices are unavailable while disconnected.</p>;
}
if (!inventoryAvailable) {
return <p role="status">Waiting for canonical authoring choices...</p>;
}
return null;
};
const TabPanel = ({
activeTab,
children,
@@ -153,6 +188,7 @@ export const SelectedCapabilityInspector = ({
};
const detailReady = capabilityDetailPhase === "ready" && capabilityDetail !== null;
const isUnsupported = nodeKind !== undefined && nodeKind !== "use";
const inventoryAvailable = inventory !== null;
return (
<section className="selected-capability-inspector" aria-label="Selected step editor">
@@ -198,6 +234,12 @@ export const SelectedCapabilityInspector = ({
{!detailReady && capabilityDetailPhase === "disconnected" && (
<p role="status">Connect to inspect the capability schema.</p>
)}
<InventoryStatus
inventoryAvailable={inventoryAvailable}
message={authoringContract.message}
phase={authoringContract.phase}
refresh={authoringContract.refresh}
/>
{detailReady && (
<>
<TabPanel activeTab={activeTab} tab="setup">
@@ -210,29 +252,37 @@ export const SelectedCapabilityInspector = ({
/>
</TabPanel>
<TabPanel activeTab={activeTab} tab="inputs">
<StepInputBindingsForm
canonicalVersion={`${stepId}:${controller.resetGeneration}`}
initialRows={inputRows}
inputSchema={capabilityDetail.inputSchema}
sourceOptions={inputSourceOptions}
targetOptions={inputTargetOptions}
onDirtyChange={controller.markDirty}
onSubmit={controller.setStepInputs}
rowDiagnostics={inputDiagnostics.rowIssues}
/>
{inventoryAvailable ? (
<StepInputBindingsForm
canonicalVersion={`${stepId}:${controller.resetGeneration}`}
initialRows={inputRows}
inputSchema={capabilityDetail.inputSchema}
sourceOptions={inputSourceOptions}
targetOptions={inputTargetOptions}
onDirtyChange={controller.markDirty}
onSubmit={controller.setStepInputs}
rowDiagnostics={inputDiagnostics.rowIssues}
/>
) : (
<p role="status">Input bindings are unavailable until canonical authoring choices are ready.</p>
)}
</TabPanel>
<TabPanel activeTab={activeTab} tab="outputs">
<StepOutputBindingsForm
key={`outputs:${stepId}:${controller.resetGeneration}`}
initialRows={outputRows}
onDirtyChange={controller.markDirty}
onSubmit={controller.setStepOutputs}
outputSchema={capabilityDetail.outputSchema}
sourceOptions={outputSourceOptions}
targetOptions={outputTargetOptions}
rowDiagnostics={outputDiagnostics.rowIssues}
stateSchema={(isRecord(draft.draft) ? draft.draft.state_schema : null) ?? emptyStateSchema}
/>
{inventoryAvailable ? (
<StepOutputBindingsForm
key={`outputs:${stepId}:${controller.resetGeneration}`}
initialRows={outputRows}
onDirtyChange={controller.markDirty}
onSubmit={controller.setStepOutputs}
outputSchema={capabilityDetail.outputSchema}
sourceOptions={outputSourceOptions}
targetOptions={outputTargetOptions}
rowDiagnostics={outputDiagnostics.rowIssues}
stateSchema={(isRecord(draft.draft) ? draft.draft.state_schema : null) ?? emptyStateSchema}
/>
) : (
<p role="status">Output bindings are unavailable until canonical authoring choices are ready.</p>
)}
</TabPanel>
</>
)}
@@ -85,6 +85,27 @@ describe("StepInputBindingsForm", () => {
expect(submissions).toEqual([[{ path: "input.request.id", target: "profile.name" }]]);
});
it("preserves a custom target that collides with the canonical step-input namespace", async () => {
const user = userEvent.setup();
const submissions: ReadonlyArray<StepInputBinding>[] = [];
render(
<StepInputBindingsForm
inputSchema={schema}
sourceOptions={[authoringOption("input.title", "Title", "workflow_input", ["step_input"])]}
targetOptions={[authoringOption("step_input.text", "Text target", "step_input", ["step_input"])]}
initialBindings={[{ path: "input.title", target: "step_input.text" }]}
onSubmit={(value) => { submissions.push(value); }}
/>,
);
const target = await editableCustomInput(user, "Target for row 1");
await user.clear(target);
await user.type(target, "step_input.text");
await user.click(screen.getByRole("button", { name: "Save inputs" }));
expect(submissions).toEqual([[{ path: "input.title", target: "step_input.text" }]]);
});
it("formats local and graph path objects at whole and nested paths", () => {
expect(displayLocalInputPath({ root: "local", parts: [] })).toBe(".");
expect(displayLocalInputPath({ root: "local", parts: ["payload", "item"] })).toBe("payload.item");
@@ -481,9 +481,11 @@ const StepInputBindingsFormContent = ({
describedBy={hasIssues ? errorId : undefined}
invalid={hasIssues}
label={`Target for row ${rowNumber}`}
onChange={(target) => editRow(row.id, (current) => ({
onChange={(target, selection) => editRow(row.id, (current) => ({
...current,
target: localPathFromPickerValue(target, targetOptions, "step_input"),
target: selection === "catalog"
? localPathFromPickerValue(target, targetOptions, "step_input")
: target,
}))}
options={targetOptions}
uses="step_input"
@@ -123,6 +123,28 @@ describe("StepOutputBindingsForm", () => {
expect(submissions).toEqual([[{ source: "text", target: "state.existing" }]]);
});
it("preserves a custom source that collides with the canonical step-output namespace", async () => {
const user = userEvent.setup();
const submissions: ReadonlyArray<OutputBinding>[] = [];
render(
<StepOutputBindingsForm
outputSchema={outputSchema}
stateSchema={stateSchema}
sourceOptions={[authoringOption("step_output.text", "Text output", "step_output", ["step_output_source"])]}
targetOptions={[authoringOption("state.existing", "Existing state", "workflow_state", ["state_target"])]}
initialBindings={[{ source: "step_output.text", target: "state.existing" }]}
onSubmit={(value) => { submissions.push(value); }}
/>,
);
const source = await editableCustomInput(user, "Source path for output row 1");
await user.clear(source);
await user.type(source, "step_output.text");
await user.click(screen.getByRole("button", { name: "Save outputs" }));
expect(submissions).toEqual([[{ source: "step_output.text", target: "state.existing" }]]);
});
it("offers capability output sources and existing state targets", () => {
render(
<StepOutputBindingsForm
@@ -201,9 +201,11 @@ const OutputRowEditor = ({
describedBy={hasIssues ? errorId : undefined}
invalid={hasIssues}
label={`Source path for output row ${rowNumber}`}
onChange={(source) => onEdit(row.id, (current) => ({
onChange={(source, selection) => onEdit(row.id, (current) => ({
...current,
sourcePath: localPathFromPickerValue(source, sourceOptions, "step_output"),
sourcePath: selection === "catalog"
? localPathFromPickerValue(source, sourceOptions, "step_output")
: source,
}))}
options={sourceOptions}
uses="step_output_source"