From db9a0008d3d049f157c327c49104f271c516c4df Mon Sep 17 00:00:00 2001 From: Leonel Rivas Date: Mon, 15 Jun 2026 23:51:26 -0700 Subject: [PATCH 1/2] fix(settings): validate the add-provider wizard step before advancing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Next button in the add-provider-instance dialog advanced the wizard unconditionally, so an empty or invalid Instance ID on the Identity step was only caught at the final Add instance step — where it silently no-ops, leaving the user with a dead button and no feedback. Gate advancing on the current step: the Identity step now blocks Next while the Instance ID is invalid and surfaces the existing inline error, matching the check handleSave already applies before submit. Fixes #2813. --- .../AddProviderInstanceDialog.test.ts | 38 +++++++++++++++++++ .../settings/AddProviderInstanceDialog.tsx | 38 ++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 apps/web/src/components/settings/AddProviderInstanceDialog.test.ts diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts b/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts new file mode 100644 index 00000000000..0ea90a541fe --- /dev/null +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { advanceInstanceWizard } from "./AddProviderInstanceDialog"; + +describe("advanceInstanceWizard", () => { + it("advances from the Driver step even when the instance id is invalid", () => { + // The Driver step (index 0) does not own the Instance ID field, so an + // invalid id must not block leaving it. + expect(advanceInstanceWizard(0, 3, { instanceIdError: "Instance ID is required." })).toEqual({ + kind: "advance", + step: 1, + }); + }); + + it("blocks leaving the Identity step while the instance id is invalid", () => { + // Regression for #2813: clicking Next on the Identity step used to advance + // unconditionally, so the user reached the final step only for "Add + // instance" to silently no-op. + expect(advanceInstanceWizard(1, 3, { instanceIdError: "Instance ID is required." })).toEqual({ + kind: "blocked", + error: "Instance ID is required.", + }); + }); + + it("advances from the Identity step once the instance id is valid", () => { + expect(advanceInstanceWizard(1, 3, { instanceIdError: null })).toEqual({ + kind: "advance", + step: 2, + }); + }); + + it("never advances past the last step", () => { + expect(advanceInstanceWizard(2, 3, { instanceIdError: null })).toEqual({ + kind: "advance", + step: 2, + }); + }); +}); diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx index 77c1813f110..b37eb2ec3ac 100644 --- a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx @@ -108,6 +108,30 @@ function validateInstanceId(id: string, existing: ReadonlySet): string | return null; } +export type WizardAdvance = + | { readonly kind: "advance"; readonly step: number } + | { readonly kind: "blocked"; readonly error: string }; + +/** + * Decide what clicking "Next" should do in the add-instance wizard. The + * Identity step (index 1) owns the Instance ID, so the wizard must not advance + * past it while the id is invalid — otherwise the user reaches the final step + * only for "Add instance" to silently no-op. Returns either the next step to + * move to, or the error that blocks advancing (mirrors the gate `handleSave` + * applies before submit). + */ +export function advanceInstanceWizard( + currentStep: number, + stepCount: number, + validation: { readonly instanceIdError: string | null }, +): WizardAdvance { + const blockingError = currentStep === 1 ? validation.instanceIdError : null; + if (blockingError !== null) { + return { kind: "blocked", error: blockingError }; + } + return { kind: "advance", step: Math.min(stepCount - 1, currentStep + 1) }; +} + interface AddProviderInstanceDialogProps { open: boolean; onOpenChange: (open: boolean) => void; @@ -453,7 +477,19 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns {wizardStep === 0 ? "Cancel" : "Back"} {wizardStep < wizardSteps.length - 1 ? ( - ) : ( From 9f399ae6713e38fece4098b3db9783551556afe6 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Wed, 22 Jul 2026 14:31:10 +0200 Subject: [PATCH 2/2] Complete validation paths for PR #3100 Gate both footer and forward step-header navigation through Identity validation while preserving backward navigation. Keep the navigation rule independently testable and surface the existing inline error on blocked skips. Co-authored-by: codex --- .../AddProviderInstanceDialog.logic.ts | 36 +++++ .../AddProviderInstanceDialog.test.ts | 52 +++--- .../settings/AddProviderInstanceDialog.tsx | 151 +++++------------- .../AddProviderInstanceWizardSteps.test.tsx | 61 +++++++ .../AddProviderInstanceWizardSteps.tsx | 70 ++++++++ 5 files changed, 239 insertions(+), 131 deletions(-) create mode 100644 apps/web/src/components/settings/AddProviderInstanceDialog.logic.ts create mode 100644 apps/web/src/components/settings/AddProviderInstanceWizardSteps.test.tsx create mode 100644 apps/web/src/components/settings/AddProviderInstanceWizardSteps.tsx diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.logic.ts b/apps/web/src/components/settings/AddProviderInstanceDialog.logic.ts new file mode 100644 index 00000000000..fdffa9a190e --- /dev/null +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.logic.ts @@ -0,0 +1,36 @@ +export type WizardNavigation = + | { readonly kind: "navigate"; readonly step: number } + | { readonly kind: "blocked"; readonly step: number; readonly error: string }; + +const IDENTITY_STEP = 1; + +export const ADD_PROVIDER_WIZARD_STEPS = ["Driver", "Identity", "Config"] as const; + +/** + * Resolve navigation within the add-provider wizard. + * + * Moving forward past Identity requires a valid instance id, whether the user + * advances one step at a time or skips directly to Config from a step header. + * A blocked skip lands on Identity so its existing inline validation is + * visible. Backward navigation is always preserved. + */ +export function resolveWizardNavigation( + currentStep: number, + requestedStep: number, + stepCount: number, + validation: { readonly instanceIdError: string | null }, +): WizardNavigation { + const lastStep = Math.max(0, stepCount - 1); + const targetStep = Math.max(0, Math.min(lastStep, requestedStep)); + const movesForwardPastIdentity = currentStep <= IDENTITY_STEP && targetStep > IDENTITY_STEP; + + if (movesForwardPastIdentity && validation.instanceIdError !== null) { + return { + kind: "blocked", + step: Math.min(IDENTITY_STEP, lastStep), + error: validation.instanceIdError, + }; + } + + return { kind: "navigate", step: targetStep }; +} diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts b/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts index 0ea90a541fe..594d2e4537e 100644 --- a/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.test.ts @@ -1,38 +1,44 @@ import { describe, expect, it } from "vite-plus/test"; -import { advanceInstanceWizard } from "./AddProviderInstanceDialog"; - -describe("advanceInstanceWizard", () => { - it("advances from the Driver step even when the instance id is invalid", () => { - // The Driver step (index 0) does not own the Instance ID field, so an - // invalid id must not block leaving it. - expect(advanceInstanceWizard(0, 3, { instanceIdError: "Instance ID is required." })).toEqual({ - kind: "advance", +import { resolveWizardNavigation } from "./AddProviderInstanceDialog.logic"; + +describe("resolveWizardNavigation", () => { + const invalidId = { instanceIdError: "Instance ID is required." }; + const validId = { instanceIdError: null }; + + it("allows moving from Driver to Identity before the instance id is valid", () => { + expect(resolveWizardNavigation(0, 1, 3, invalidId)).toEqual({ kind: "navigate", step: 1 }); + }); + + it("blocks Next from Identity to Config while the instance id is invalid", () => { + expect(resolveWizardNavigation(1, 2, 3, invalidId)).toEqual({ + kind: "blocked", step: 1, + error: "Instance ID is required.", }); }); - it("blocks leaving the Identity step while the instance id is invalid", () => { - // Regression for #2813: clicking Next on the Identity step used to advance - // unconditionally, so the user reached the final step only for "Add - // instance" to silently no-op. - expect(advanceInstanceWizard(1, 3, { instanceIdError: "Instance ID is required." })).toEqual({ + it("stops a direct Driver-to-Config skip at Identity and surfaces its error", () => { + expect(resolveWizardNavigation(0, 2, 3, invalidId)).toEqual({ kind: "blocked", + step: 1, error: "Instance ID is required.", }); }); - it("advances from the Identity step once the instance id is valid", () => { - expect(advanceInstanceWizard(1, 3, { instanceIdError: null })).toEqual({ - kind: "advance", - step: 2, - }); + it("allows advancing and skipping forward once the instance id is valid", () => { + expect(resolveWizardNavigation(1, 2, 3, validId)).toEqual({ kind: "navigate", step: 2 }); + expect(resolveWizardNavigation(0, 2, 3, validId)).toEqual({ kind: "navigate", step: 2 }); }); - it("never advances past the last step", () => { - expect(advanceInstanceWizard(2, 3, { instanceIdError: null })).toEqual({ - kind: "advance", - step: 2, - }); + it("always preserves backward Driver and Identity navigation", () => { + expect(resolveWizardNavigation(2, 1, 3, invalidId)).toEqual({ kind: "navigate", step: 1 }); + expect(resolveWizardNavigation(2, 0, 3, invalidId)).toEqual({ kind: "navigate", step: 0 }); + expect(resolveWizardNavigation(1, 0, 3, invalidId)).toEqual({ kind: "navigate", step: 0 }); + }); + + it("clamps requested steps to the wizard bounds", () => { + expect(resolveWizardNavigation(2, 8, 3, validId)).toEqual({ kind: "navigate", step: 2 }); + expect(resolveWizardNavigation(0, -1, 3, invalidId)).toEqual({ kind: "navigate", step: 0 }); }); }); diff --git a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx index b37eb2ec3ac..ed808e30b0c 100644 --- a/apps/web/src/components/settings/AddProviderInstanceDialog.tsx +++ b/apps/web/src/components/settings/AddProviderInstanceDialog.tsx @@ -1,8 +1,7 @@ "use client"; -import { CheckIcon } from "lucide-react"; import { Radio as RadioPrimitive } from "@base-ui/react/radio"; -import { useCallback, useMemo, useState } from "react"; +import { useMemo, useState } from "react"; import { ProviderInstanceId, ProviderDriverKind, @@ -29,6 +28,12 @@ import { toastManager } from "../ui/toast"; import { DRIVER_OPTION_BY_VALUE, DRIVER_OPTIONS } from "./providerDriverMeta"; import { ProviderSettingsForm, deriveProviderSettingsFields } from "./ProviderSettingsForm"; import { AnimatedHeight } from "../AnimatedHeight"; +import { + ADD_PROVIDER_WIZARD_STEPS, + resolveWizardNavigation, + type WizardNavigation, +} from "./AddProviderInstanceDialog.logic"; +import { AddProviderInstanceWizardSteps } from "./AddProviderInstanceWizardSteps"; const PROVIDER_ACCENT_SWATCHES = [ "#2563eb", @@ -108,30 +113,6 @@ function validateInstanceId(id: string, existing: ReadonlySet): string | return null; } -export type WizardAdvance = - | { readonly kind: "advance"; readonly step: number } - | { readonly kind: "blocked"; readonly error: string }; - -/** - * Decide what clicking "Next" should do in the add-instance wizard. The - * Identity step (index 1) owns the Instance ID, so the wizard must not advance - * past it while the id is invalid — otherwise the user reaches the final step - * only for "Add instance" to silently no-op. Returns either the next step to - * move to, or the error that blocks advancing (mirrors the gate `handleSave` - * applies before submit). - */ -export function advanceInstanceWizard( - currentStep: number, - stepCount: number, - validation: { readonly instanceIdError: string | null }, -): WizardAdvance { - const blockingError = currentStep === 1 ? validation.instanceIdError : null; - if (blockingError !== null) { - return { kind: "blocked", error: blockingError }; - } - return { kind: "advance", step: Math.min(stepCount - 1, currentStep + 1) }; -} - interface AddProviderInstanceDialogProps { open: boolean; onOpenChange: (open: boolean) => void; @@ -167,26 +148,37 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns const instanceIdError = validateInstanceId(instanceId, existingIds); const showInstanceIdError = hasAttemptedSubmit && instanceIdError !== null; const previewLabel = label.trim() || `${driverOption.label} Workspace`; - const wizardSteps = ["Driver", "Identity", "Config"] as const; const wizardStepSummaries = [driverOption.label, previewLabel, null] as const; const configDraft = configByDriver[driver] ?? EMPTY_CONFIG_DRAFT; - const setConfigDraft = useCallback( - (config: Record | undefined) => { - setConfigByDriver((existing) => { - const next = { ...existing }; - if (config === undefined || Object.keys(config).length === 0) { - delete next[driver]; - } else { - next[driver] = config; - } - return next; - }); - }, - [driver], - ); + const setConfigDraft = (config: Record | undefined) => { + setConfigByDriver((existing) => { + const next = { ...existing }; + if (config === undefined || Object.keys(config).length === 0) { + delete next[driver]; + } else { + next[driver] = config; + } + return next; + }); + }; + + const applyWizardNavigation = (navigation: WizardNavigation) => { + if (navigation.kind === "blocked") { + setHasAttemptedSubmit(true); + } + setWizardStep(navigation.step); + }; + + const navigateToStep = (requestedStep: number) => { + applyWizardNavigation( + resolveWizardNavigation(wizardStep, requestedStep, ADD_PROVIDER_WIZARD_STEPS.length, { + instanceIdError, + }), + ); + }; - const handleSave = useCallback(() => { + const handleSave = () => { setHasAttemptedSubmit(true); if (instanceIdError !== null) return; @@ -225,18 +217,7 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns description: error instanceof Error ? error.message : "Update failed.", }); } - }, [ - driver, - driverOption, - configByDriver, - instanceId, - instanceIdError, - label, - accentColor, - onOpenChange, - settings.providerInstances, - updateSettings, - ]); + }; return ( @@ -248,46 +229,12 @@ export function AddProviderInstanceDialog({ open, onOpenChange }: AddProviderIns Configure an additional provider instance — for example, a second Codex install pointed at a different workspace. -
- {wizardSteps.map((step, index) => ( - - ))} -
+
{wizardStep === 0 ? "Cancel" : "Back"} - {wizardStep < wizardSteps.length - 1 ? ( - ) : ( diff --git a/apps/web/src/components/settings/AddProviderInstanceWizardSteps.test.tsx b/apps/web/src/components/settings/AddProviderInstanceWizardSteps.test.tsx new file mode 100644 index 00000000000..964779f4ec9 --- /dev/null +++ b/apps/web/src/components/settings/AddProviderInstanceWizardSteps.test.tsx @@ -0,0 +1,61 @@ +import { Children, isValidElement, type ReactElement } from "react"; +import { describe, expect, it, vi } from "vite-plus/test"; + +import { ADD_PROVIDER_WIZARD_STEPS } from "./AddProviderInstanceDialog.logic"; +import { AddProviderInstanceWizardSteps } from "./AddProviderInstanceWizardSteps"; + +interface StepButtonProps { + readonly "aria-current"?: string; + readonly onClick: () => void; +} + +function renderStepButtons( + currentStep: number, + instanceIdError: string | null, + onNavigation: Parameters[0]["onNavigation"], +): ReactElement[] { + const header = AddProviderInstanceWizardSteps({ + currentStep, + summaries: ["Codex", "Codex Workspace", null], + instanceIdError, + onNavigation, + }); + + return Children.toArray(header.props.children).filter( + (child): child is ReactElement => isValidElement(child), + ); +} + +describe("AddProviderInstanceWizardSteps", () => { + it("gates the actual Config header click through Identity validation", () => { + const onNavigation = vi.fn(); + const buttons = renderStepButtons(0, "Instance ID is required.", onNavigation); + + expect(buttons).toHaveLength(ADD_PROVIDER_WIZARD_STEPS.length); + buttons[2]!.props.onClick(); + + expect(onNavigation).toHaveBeenCalledOnce(); + expect(onNavigation).toHaveBeenCalledWith({ + kind: "blocked", + step: 1, + error: "Instance ID is required.", + }); + }); + + it("marks the wizard step separately from the clicked button focus", () => { + const buttons = renderStepButtons(1, "Instance ID is required.", vi.fn()); + + expect(buttons[0]!.props["aria-current"]).toBeUndefined(); + expect(buttons[1]!.props["aria-current"]).toBe("step"); + expect(buttons[2]!.props["aria-current"]).toBeUndefined(); + }); + + it("preserves the actual backward header click", () => { + const onNavigation = vi.fn(); + const buttons = renderStepButtons(2, "Instance ID is required.", onNavigation); + + buttons[0]!.props.onClick(); + + expect(onNavigation).toHaveBeenCalledWith({ kind: "navigate", step: 0 }); + }); +}); diff --git a/apps/web/src/components/settings/AddProviderInstanceWizardSteps.tsx b/apps/web/src/components/settings/AddProviderInstanceWizardSteps.tsx new file mode 100644 index 00000000000..4747557471d --- /dev/null +++ b/apps/web/src/components/settings/AddProviderInstanceWizardSteps.tsx @@ -0,0 +1,70 @@ +import { CheckIcon } from "lucide-react"; + +import { cn } from "../../lib/utils"; +import { + ADD_PROVIDER_WIZARD_STEPS, + resolveWizardNavigation, + type WizardNavigation, +} from "./AddProviderInstanceDialog.logic"; + +interface AddProviderInstanceWizardStepsProps { + readonly currentStep: number; + readonly summaries: readonly (string | null)[]; + readonly instanceIdError: string | null; + readonly onNavigation: (navigation: WizardNavigation) => void; +} + +export function AddProviderInstanceWizardSteps({ + currentStep, + summaries, + instanceIdError, + onNavigation, +}: AddProviderInstanceWizardStepsProps) { + return ( +
+ {ADD_PROVIDER_WIZARD_STEPS.map((step, index) => ( + + ))} +
+ ); +}