From eae1a62fb23d5b9f253ad0ae528f82384a16a57a Mon Sep 17 00:00:00 2001 From: lda Date: Fri, 14 Aug 2026 18:34:44 +0700 Subject: [PATCH] fix: close Task 5 contract graph review findings --- .../authoring/AuthoringPathPicker.test.tsx | 53 ++++++++++++ .../authoring/AuthoringPathPicker.tsx | 20 +++-- .../authoring/useAuthoringContract.test.tsx | 13 +++ .../authoring/useAuthoringContract.ts | 31 +++++-- .../domain/authoring-contract-client.test.ts | 12 ++- .../domain/authoring-contract-client.ts | 3 +- .../domain/authoring-contract-models.test.ts | 14 +++- .../domain/authoring-contract-models.ts | 82 +++++++++---------- 8 files changed, 165 insertions(+), 63 deletions(-) diff --git a/web/apps/console/src/workspace/authoring/AuthoringPathPicker.test.tsx b/web/apps/console/src/workspace/authoring/AuthoringPathPicker.test.tsx index ec97f9be..bca60d6b 100644 --- a/web/apps/console/src/workspace/authoring/AuthoringPathPicker.test.tsx +++ b/web/apps/console/src/workspace/authoring/AuthoringPathPicker.test.tsx @@ -41,6 +41,10 @@ const options: ReadonlyArray = [ availability: "conditional", reason: "Available when the selected step runs in a viewer frame.", }), + option("context.viewer_email", "Viewer email", "runtime_context", ["workflow_output"], { + availability: "conditional", + reason: "Available only in viewer frames.", + }), option("output.report", "Final report", "workflow_output", ["workflow_output"]), option("input.customer.name", "Customer name", "workflow_input", ["step_input"]), ]; @@ -133,6 +137,55 @@ describe("AuthoringPathPicker", () => { expect(onChange).toHaveBeenCalledWith("input.title"); }); + it("gives a conditional incompatible option one composed description node", () => { + render( + , + ); + + const optionButton = screen.getByRole("button", { name: /Viewer email/ }); + const describedBy = optionButton.getAttribute("aria-describedby"); + + expect(optionButton).toBeDisabled(); + expect(describedBy).not.toBeNull(); + expect(describedBy?.trim().split(/\s+/)).toHaveLength(1); + expect(document.querySelectorAll(`#${describedBy}`).length).toBe(1); + expect(document.getElementById(describedBy ?? "")?.textContent).toContain( + "Available only in viewer frames.", + ); + expect(document.getElementById(describedBy ?? "")?.textContent).toContain( + "Not available for this field.", + ); + }); + + it("renders a supplied non-conditional reason before describing the option", () => { + render( + , + ); + + const optionButton = screen.getByRole("button", { name: /Reasoned/ }); + const describedBy = optionButton.getAttribute("aria-describedby"); + expect(describedBy).not.toBeNull(); + expect(document.getElementById(describedBy ?? "")?.textContent).toBe( + "Retained for repair context.", + ); + }); + it("keeps nested choices keyboard reachable and preserves normal choices in Advanced mode", async () => { const user = userEvent.setup(); const onChange = vi.fn(); diff --git a/web/apps/console/src/workspace/authoring/AuthoringPathPicker.tsx b/web/apps/console/src/workspace/authoring/AuthoringPathPicker.tsx index 0b9adf54..fc548f2e 100644 --- a/web/apps/console/src/workspace/authoring/AuthoringPathPicker.tsx +++ b/web/apps/console/src/workspace/authoring/AuthoringPathPicker.tsx @@ -80,14 +80,19 @@ export const AuthoringPathPicker = ({
{group.label}
- {groupOptions.map((option) => { + {groupOptions.map((option, optionIndex) => { const compatible = option.uses.some((use) => requestedUses.has(use)); - const reasonId = `${id}-${safeId(option.path)}-reason`; + const reasonId = `${id}-${safeId(group.origin)}-${optionIndex}-description`; + const availabilityReason = option.availability === "conditional" + ? option.reason ?? "Conditionally available; verify this path at runtime." + : option.reason; + const description = [ + availabilityReason, + ...(compatible ? [] : ["Not available for this field."]), + ].filter((part): part is string => part !== undefined).join(" "); return ( ); })} diff --git a/web/apps/console/src/workspace/authoring/useAuthoringContract.test.tsx b/web/apps/console/src/workspace/authoring/useAuthoringContract.test.tsx index 50d3353e..f40e446f 100644 --- a/web/apps/console/src/workspace/authoring/useAuthoringContract.test.tsx +++ b/web/apps/console/src/workspace/authoring/useAuthoringContract.test.tsx @@ -125,6 +125,19 @@ describe("useAuthoringContract", () => { expect(result.current.inventory?.selectedStepId).toBeNull(); }); + it("surfaces an initial inspection failure instead of staying loading", async () => { + client.inspect.mockRejectedValue(new Error("initial inspection failed")); + + const { result } = renderHook(() => + useAuthoringContract({ workspaceId: "draft-report", revision: 7, selectedStepId: null }), + ); + + await waitFor(() => expect(result.current.phase).toBe("error")); + + expect(result.current.inventory).toBeNull(); + expect(result.current.message).toBe("initial inspection failed"); + }); + it("passes the executable step id and ignores a stale selection response", async () => { const first = deferred(); const second = deferred(); diff --git a/web/apps/console/src/workspace/authoring/useAuthoringContract.ts b/web/apps/console/src/workspace/authoring/useAuthoringContract.ts index 938c08a6..37e0c28d 100644 --- a/web/apps/console/src/workspace/authoring/useAuthoringContract.ts +++ b/web/apps/console/src/workspace/authoring/useAuthoringContract.ts @@ -42,12 +42,14 @@ type StoredInventory = { type AuthoringContractState = { readonly phase: AuthoringContractPhase; + readonly attempted: RequestIdentity | null; readonly stored: StoredInventory | null; readonly message: string | null; }; const initialState: AuthoringContractState = { phase: "disconnected", + attempted: null, stored: null, message: null, }; @@ -98,6 +100,7 @@ export const useAuthoringContract = ({ const requested = request; setState((current) => ({ phase: "loading", + attempted: requested, stored: current.stored !== null && sameRequest(current.stored.request, requested) ? current.stored @@ -115,6 +118,7 @@ export const useAuthoringContract = ({ if (generation !== generationRef.current) return; setState({ phase: "ready", + attempted: requested, stored: { request: requested, inventory }, message: null, }); @@ -134,6 +138,7 @@ export const useAuthoringContract = ({ generationRef.current++; setState({ phase: client === null || connectedTarget === null ? "disconnected" : "idle", + attempted: null, stored: null, message: null, }); @@ -150,20 +155,28 @@ export const useAuthoringContract = ({ request !== null && state.stored !== null && sameRequest(state.stored.request, request) ? state.stored.inventory : null; - const phase = + const hasCurrentAttempt = request !== null && - (state.stored === null || !sameRequest(state.stored.request, request)) - ? "loading" - : request === null - ? client === null || connectedTarget === null - ? "disconnected" - : "idle" - : state.phase; + state.attempted !== null && + sameRequest(state.attempted, request); + const phase = request === null + ? client === null || connectedTarget === null + ? "disconnected" + : "idle" + : hasCurrentAttempt && state.phase === "error" + ? "error" + : state.stored !== null && sameRequest(state.stored.request, request) + ? state.phase + : "loading"; return { phase, inventory: currentInventory, - message: currentInventory === null && phase === "loading" ? null : state.message, + message: hasCurrentAttempt && state.phase === "error" + ? state.message + : currentInventory === null && phase === "loading" + ? null + : state.message, refresh, }; }; diff --git a/web/apps/console/src/workspace/domain/authoring-contract-client.test.ts b/web/apps/console/src/workspace/domain/authoring-contract-client.test.ts index ed4a955c..89fc3ca3 100644 --- a/web/apps/console/src/workspace/domain/authoring-contract-client.test.ts +++ b/web/apps/console/src/workspace/domain/authoring-contract-client.test.ts @@ -43,7 +43,7 @@ const runWith = ( describe("AuthoringContractClient", () => { it("sends the exact inspection params and decodes the response", async () => { - const { executor, calls } = runWith(wireInventory); + const { executor, calls } = runWith({ ...wireInventory, selected_step_id: "render" }); const client = createAuthoringContractClient(executor); const result = await client.inspect({ @@ -80,6 +80,7 @@ describe("AuthoringContractClient", () => { it.each([ ["workspaceId", { workspace_id: "other" }], ["revision", { revision: 8 }], + ["selectedStepId", { selected_step_id: "render" }], ])("rejects a response with a mismatched %s", async (_field, replacement) => { const { executor } = runWith({ ...wireInventory, ...replacement }); const client = createAuthoringContractClient(executor); @@ -88,4 +89,13 @@ describe("AuthoringContractClient", () => { client.inspect({ workspaceId: "draft-report", revision: 7, selectedStepId: null }), ).rejects.toThrow("does not match inspection request"); }); + + it("rejects a response for a different executable step", async () => { + const { executor } = runWith({ ...wireInventory, selected_step_id: "other" }); + const client = createAuthoringContractClient(executor); + + await expect( + client.inspect({ workspaceId: "draft-report", revision: 7, selectedStepId: "render" }), + ).rejects.toThrow("does not match inspection request"); + }); }); diff --git a/web/apps/console/src/workspace/domain/authoring-contract-client.ts b/web/apps/console/src/workspace/domain/authoring-contract-client.ts index 988b0506..6c1042eb 100644 --- a/web/apps/console/src/workspace/domain/authoring-contract-client.ts +++ b/web/apps/console/src/workspace/domain/authoring-contract-client.ts @@ -42,7 +42,8 @@ export const createAuthoringContractClient = ( ); if ( inventory.workspaceId !== workspaceId || - inventory.revision !== input.revision + inventory.revision !== input.revision || + (inventory.selectedStepId?.trim() || null) !== selectedStepId ) { throw new Error("authoring contract response does not match inspection request"); } diff --git a/web/apps/console/src/workspace/domain/authoring-contract-models.test.ts b/web/apps/console/src/workspace/domain/authoring-contract-models.test.ts index 9a670ead..9ff440da 100644 --- a/web/apps/console/src/workspace/domain/authoring-contract-models.test.ts +++ b/web/apps/console/src/workspace/domain/authoring-contract-models.test.ts @@ -96,11 +96,23 @@ describe("authoring contract models", () => { }); it("rejects a malformed inventory envelope", () => { - expect(() => decodeAuthoringContractInventory({ ...inventory, revision: "7" })).toThrow( + for (const revision of [0, -1, 1.5, "7"]) { + expect(() => decodeAuthoringContractInventory({ ...inventory, revision })).toThrow( + "AuthoringContractInventory is malformed", + ); + } + expect(() => decodeAuthoringContractInventory({ ...inventory, selected_step_id: "" })).toThrow( "AuthoringContractInventory is malformed", ); }); + it("requires a reason for conditional options", () => { + expect(() => decodeAuthoringContractInventory({ + ...inventory, + readable_sources: [{ ...pathOption, availability: "conditional", reason: undefined }], + })).toThrow("AuthoringContractInventory is malformed"); + }); + const _typeCheck: AuthoringContractInventory | null = null; void _typeCheck; }); diff --git a/web/apps/console/src/workspace/domain/authoring-contract-models.ts b/web/apps/console/src/workspace/domain/authoring-contract-models.ts index 13b07ffa..22f135a5 100644 --- a/web/apps/console/src/workspace/domain/authoring-contract-models.ts +++ b/web/apps/console/src/workspace/domain/authoring-contract-models.ts @@ -52,6 +52,8 @@ export type AuthoringContractInventory = { }; const JsonObjectSchema = v.record(v.string(), v.unknown()); +const PositiveRevisionSchema = v.pipe(v.number(), v.integer(), v.minValue(1)); +const SelectedStepIdSchema = v.nullable(v.pipe(v.string(), v.minLength(1))); const AuthoringPathOriginSchema = v.union([ v.literal("workflow_input"), @@ -74,77 +76,73 @@ const AuthoringPathUseSchema = v.union([ v.literal("workflow_output"), ]); -const AuthoringPathOptionWireSchema = v.object({ - path: v.string(), - label: v.string(), - origin: AuthoringPathOriginSchema, - schema: JsonObjectSchema, - required: v.boolean(), - availability: AuthoringPathAvailabilitySchema, - uses: v.array(AuthoringPathUseSchema), - description: v.optional(v.string()), - reason: v.optional(v.string()), -}); +const AuthoringPathOptionSchema = v.pipe( + v.object({ + path: v.string(), + label: v.string(), + origin: AuthoringPathOriginSchema, + schema: JsonObjectSchema, + required: v.boolean(), + availability: AuthoringPathAvailabilitySchema, + uses: v.array(AuthoringPathUseSchema), + description: v.optional(v.string()), + reason: v.optional(v.string()), + }), + v.check( + (option) => + option.availability !== "conditional" || + (option.reason !== undefined && option.reason.trim().length > 0), + "conditional authoring options require a non-empty reason", + ), +); const AuthoringStepContractWireSchema = v.object({ step_id: v.string(), label: v.string(), description: v.optional(v.string()), - input_targets: v.optional(v.array(AuthoringPathOptionWireSchema)), - output_sources: v.optional(v.array(AuthoringPathOptionWireSchema)), + input_targets: v.optional(v.array(AuthoringPathOptionSchema)), + output_sources: v.optional(v.array(AuthoringPathOptionSchema)), outcomes: v.optional(v.array(v.string())), }); const AuthoringContractInventoryWireSchema = v.object({ workspace_id: v.string(), - revision: v.number(), - selected_step_id: v.nullable(v.string()), - readable_sources: v.array(AuthoringPathOptionWireSchema), - step_input_targets: v.array(AuthoringPathOptionWireSchema), - step_output_sources: v.array(AuthoringPathOptionWireSchema), - state_targets: v.array(AuthoringPathOptionWireSchema), - workflow_output_targets: v.array(AuthoringPathOptionWireSchema), + revision: PositiveRevisionSchema, + selected_step_id: SelectedStepIdSchema, + readable_sources: v.array(AuthoringPathOptionSchema), + step_input_targets: v.array(AuthoringPathOptionSchema), + step_output_sources: v.array(AuthoringPathOptionSchema), + state_targets: v.array(AuthoringPathOptionSchema), + workflow_output_targets: v.array(AuthoringPathOptionSchema), entry_steps: v.array(AuthoringStepContractWireSchema), workflow_outcomes: v.array(v.string()), warnings: v.array(v.string()), }); -const AuthoringPathOptionBrowserSchema = v.object({ - path: v.string(), - label: v.string(), - origin: AuthoringPathOriginSchema, - schema: JsonObjectSchema, - required: v.boolean(), - availability: AuthoringPathAvailabilitySchema, - uses: v.array(AuthoringPathUseSchema), - description: v.optional(v.string()), - reason: v.optional(v.string()), -}); - const AuthoringStepContractBrowserSchema = v.object({ stepId: v.string(), label: v.string(), description: v.optional(v.string()), - inputTargets: v.optional(v.array(AuthoringPathOptionBrowserSchema)), - outputSources: v.optional(v.array(AuthoringPathOptionBrowserSchema)), + inputTargets: v.optional(v.array(AuthoringPathOptionSchema)), + outputSources: v.optional(v.array(AuthoringPathOptionSchema)), outcomes: v.optional(v.array(v.string())), }); const AuthoringContractInventoryBrowserSchema = v.object({ workspaceId: v.string(), - revision: v.number(), - selectedStepId: v.nullable(v.string()), - readableSources: v.array(AuthoringPathOptionBrowserSchema), - stepInputTargets: v.array(AuthoringPathOptionBrowserSchema), - stepOutputSources: v.array(AuthoringPathOptionBrowserSchema), - stateTargets: v.array(AuthoringPathOptionBrowserSchema), - workflowOutputTargets: v.array(AuthoringPathOptionBrowserSchema), + revision: PositiveRevisionSchema, + selectedStepId: SelectedStepIdSchema, + readableSources: v.array(AuthoringPathOptionSchema), + stepInputTargets: v.array(AuthoringPathOptionSchema), + stepOutputSources: v.array(AuthoringPathOptionSchema), + stateTargets: v.array(AuthoringPathOptionSchema), + workflowOutputTargets: v.array(AuthoringPathOptionSchema), entrySteps: v.array(AuthoringStepContractBrowserSchema), workflowOutcomes: v.array(v.string()), warnings: v.array(v.string()), }); -type AuthoringPathOptionWire = v.InferOutput; +type AuthoringPathOptionWire = v.InferOutput; type AuthoringStepContractWire = v.InferOutput; type AuthoringContractInventoryWire = v.InferOutput< typeof AuthoringContractInventoryWireSchema