Shared Multi Step Form for Object Storage Connectors

[lovincyrus]

This PR implements a shared, config-driven multi-step form architecture for object storage connectors (GCS, S3,
Azure). It replaces a GCS-specific implementation with a generalized system that splits connector configuration
into two steps:

1. Connector step: Authentication configuration
2. Source step: Resource path and model creation

• Azure explicit credentials
• S3 explicit credentials
• Remove Skip and add a contextual messaging in right panel that acts the same
• Add public option that changes button to Continue
• Pin Help text to bottom of right panel, update messaging on Source model page
• Change Test and Import Data to, Just Import Data or Create Model
• https://github.com/rilldata/rill/pull/8395/files
• Connector validation for authentication method
• ⚠️ Centralize auth method state in connectorStepStore to avoid initialization races
• ⚠️ Extract isSubmitDisabled logic to testable functions
• ⚠️ Add constants for magic strings ("public", etc.)

Checklist:

• Covered by tests
• Ran it and it works as intended
• Reviewed the diff before requesting a review
• Checked for unhandled edge cases
• Linked the issues it closes
• Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
• Intend to cherry-pick into the release branch
• I'm proud of this work!

[lovincyrus]

Code Review: Shared Multi Step Form for Object Storage Connectors

Overview

This PR implements a shared, config-driven multi-step form architecture for object storage connectors (GCS, S3, Azure). It replaces a GCS-specific implementation with a generalized system that splits connector configuration into two steps:

1. Connector step: Authentication configuration
2. Source step: Resource path and model creation

The refactoring demonstrates good abstraction design by extracting authentication patterns into reusable configurations.

---

Strengths

1. Excellent Abstraction & Reusability ✅

The introduction of multi-step-auth-configs.ts is a strong design decision. This configuration-driven approach allows adding new connectors without duplicating form logic. The config structure is well-designed with:

• authOptions: Defines available authentication methods
• authFieldGroups: Maps each auth method to its required fields
• clearFieldsByMethod: Ensures form state hygiene when switching auth methods
• excludedKeys: Prevents duplicate rendering of auth fields

2. Strong Type Safety ✅

The type definitions in types.ts are well-structured with discriminated unions for AuthField ensuring type-safe rendering based on field type.

3. Good Component Boundaries ✅

The component hierarchy is well-organized:

AddDataForm.svelte (orchestrator)
  ↓
MultiStepFormRenderer.svelte (config → component adapter)
  ↓
MultiStepAuthForm.svelte (auth UI logic)

Each component has a clear responsibility.

---

Areas for Improvement

1. State Management Complexity ⚠️

Issue: The selectedAuthMethod state is managed across multiple layers with initialization logic scattered in AddDataForm.svelte:313-327. Multiple reactive statements compete to set the value, passed through multiple components with bind:authMethod.

Problems:

• Initialization race conditions: Multiple reactive statements compete to set selectedAuthMethod
• Unclear ownership: Who owns this state? Parent or child?
• Difficult debugging: State changes happen reactively across files

Recommendation: Centralize auth method state in connectorStepStore:

// connectorStepStore.ts
export const connectorStepStore = writable<{
  step: ConnectorStep;
  connectorConfig: Record<string, unknown> | null;
  selectedAuthMethod: string | null;  // Add this
}>({
  step: "connector",
  connectorConfig: null,
  selectedAuthMethod: null,
});

export function setAuthMethod(method: string) {
  connectorStepStore.update((state) => ({ ...state, selectedAuthMethod: method }));
}

2. Dynamic Validation Logic Needs Improvement ⚠️

The validation in FormValidation.ts:50-86 uses test-based conditional validation with getter dependencies.

Problems:

• Fragile getter dependency: Relies on authMethodGetter function passed through constructor
• Validation state not obvious: Hard to understand which fields are validated when

Recommendation: Use yup.when() with explicit dependencies:

fieldValidations[field.id] = yup.string().when('$authMethod', {
  is: (authMethod: string) => authMethod === method,
  then: (schema) => schema.required(`${label} is required`),
  otherwise: (schema) => schema.optional(),
});

3. Submit Button Logic Too Complex ⚠️

The isSubmitDisabled calculation in AddDataForm.svelte:137-210 is ~74 lines with nested conditionals.

Problems:

• Hard to test: Giant reactive statement with multiple branches
• Hard to maintain: Adding new conditions requires understanding all existing logic
• Duplicates validation logic: Re-implements field requirement checks

Recommendation: Extract to well-named functions:

function isMultiStepConnectorDisabled(config, selectedAuthMethod, paramsForm, paramsErrors) {
  const method = selectedAuthMethod || config.defaultAuthMethod || config.authOptions?.[0]?.value;
  if (!method) return true;
  
  const fields = config.authFieldGroups?.[method] || [];
  if (!fields.length && method === 'public') return false;
  
  return !fields.every(field => {
    if (field.optional ?? false) return true;
    const value = paramsForm[field.id];
    return !isEmpty(value) && !paramsErrors[field.id]?.length;
  });
}

$: isSubmitDisabled = (() => {
  if (isMultiStepConnector && stepState.step === "connector") {
    return isMultiStepConnectorDisabled(activeMultiStepConfig, selectedAuthMethod, $paramsForm, $paramsErrors);
  }
  // ... other cases
})();

4. Form State Clearing Logic Scattered ⚠️

Field clearing happens reactively in MultiStepAuthForm.svelte:28-39. If someone forgets to update clearFieldsByMethod, stale data persists.

Recommendation: Make clearing explicit and comprehensive:

function clearUnusedAuthFields(
  form: Record<string, any>,
  config: MultiStepFormConfig,
  selectedMethod: string
): Record<string, any> {
  const allAuthFields = new Set(
    Object.values(config.authFieldGroups).flat().map(f => f.id)
  );
  const activeFields = new Set(
    config.authFieldGroups[selectedMethod]?.map(f => f.id) || []
  );
  
  const clearedForm = { ...form };
  for (const field of allAuthFields) {
    if (!activeFields.has(field)) {
      clearedForm[field] = "";
    }
  }
  return clearedForm;
}

This approach clears all non-active auth fields automatically and doesn't require maintaining clearFieldsByMethod separately.

5. Submit Flow Has Hidden Public Auth Shortcut ⚠️

In AddDataFormManager.ts:317-321, public auth skips connection testing:

if (selectedAuthMethod === "public") {
  setConnectorConfig(values);
  setStep("source");
  return;
}

Problems:

• Hidden logic: User clicks "Continue" but validation still runs
• Hard to discover: This shortcut is buried in the submit handler

Recommendation: Extract to named functions:

private handlePublicAuthSubmit(values: Record<string, unknown>) {
  // Public auth doesn't require connection testing
  setConnectorConfig(values);
  setStep("source");
}

private async handlePrivateAuthSubmit(queryClient: any, values: Record<string, unknown>) {
  await submitAddConnectorForm(queryClient, this.connector, values, false);
  setConnectorConfig(values);
  setStep("source");
}

6. "Save Anyway" Logic is Confusing ⚠️

The conditions for showing "Save Anyway" button in AddDataFormManager.ts:287-307 have 6 conditions ANDed together.

Recommendation: Extract to a well-named predicate:

private shouldShowSaveAnywayButton(args: {
  isConnectorForm: boolean;
  event: any;
  stepState: ConnectorStepState;
  selectedAuthMethod?: string;
}): boolean {
  const { isConnectorForm, event, stepState, selectedAuthMethod } = args;
  
  // Only show for connector forms (not sources)
  if (!isConnectorForm) return false;
  
  // ClickHouse has its own error handling
  if (this.connector.name === "clickhouse") return false;
  
  // Need a submission result to show the button
  if (!event?.result) return false;
  
  // Multi-step connectors: don't show on source step (final step)
  if (stepState?.step === "source") return false;
  
  // Public auth bypasses connection test, so no "Save Anyway" needed
  if (stepState?.step === "connector" && selectedAuthMethod === "public") return false;
  
  return true;
}

7. Backend Connector Property Changes Not Well Explained ⚠️

The backend changes in runtime/drivers/s3/s3.go remove role assumption properties (aws_role_arn, aws_role_session_name, aws_external_id).

Problems:

• Breaking change: Existing connectors using role assumption will break
• No migration path: PR description doesn't mention how to handle existing configs

Recommendation:

• Add migration guide in PR description
• Consider deprecation path
• Add comment explaining why removed

8. Magic String "public" Used Throughout ⚠️

The string "public" appears in multiple places without constants.

Recommendation: Define constants:

// constants.ts
export const AUTH_METHOD_PUBLIC = "public" as const;
export const AUTH_METHOD_CREDENTIALS = "credentials" as const;

// Usage:
if (selectedAuthMethod === AUTH_METHOD_PUBLIC) { ... }

9. UI Copy Should Be Extracted to Constants ⚠️

Button labels are hardcoded in AddDataFormManager.ts:229-247.

Recommendation: Extract to a config:

export const BUTTON_LABELS = {
  public: { idle: "Continue", submitting: "Continuing..." },
  connector: { idle: "Test and Connect", submitting: "Testing connection..." },
  source: { idle: "Import Data", submitting: "Importing data..." },
} as const;

10. Adding New Connectors Requires Updates in Multiple Files ⚠️

To add a new multi-step connector, you need to update 4 different files.

Recommendation: Enforce via types:

// Ensure every MULTI_STEP_CONNECTOR has a config
type MultiStepConnectorName = typeof MULTI_STEP_CONNECTORS[number];
const _typeCheck: Record<MultiStepConnectorName, MultiStepFormConfig> = multiStepFormConfigs;

This creates a compile-time error if someone adds a connector without a config.

---

Testing Concerns

11. Complex Logic Needs More Tests ⚠️

Key areas needing tests:

1. Auth method switching: Does field clearing work correctly?
2. Validation logic: Are required fields enforced per auth method?
3. Public auth shortcut: Does it skip connection testing correctly?
4. Multi-step navigation: Can users go back? Is state preserved?

---

Summary & Recommendations

Priority Fixes (Before Merge)

1. ⚠️ Centralize auth method state in connectorStepStore to avoid initialization races
2. ⚠️ Extract isSubmitDisabled logic to testable functions
3. ⚠️ Add constants for magic strings ("public", etc.)
4. ⚠️ Document backend breaking changes and provide migration path

Medium Priority (Can Be Follow-up)

1. Improve validation with yup.when() instead of test-based validation
2. Simplify "Save Anyway" logic with named predicates
3. Extract field clearing logic to be config-driven
4. Add comprehensive tests for auth switching and validation

Low Priority (Nice to Have)

1. Extract button labels to constants
2. Standardize error handling utilities
3. Add type-level enforcement for multi-step connector configs

---

Final Verdict

Overall Assessment: Good abstraction work with some state management complexity

• ✅ Readability: Good component boundaries, but some overly complex reactive statements
• ✅ Maintainability: Config-driven design is excellent, but state management needs simplification
• ⚠️ Abstractions: Strong config system, but validation and state management could be cleaner
• ⚠️ Function Boundaries: Good separation, but some functions are too large (isSubmitDisabled)
• ⚠️ State Management: Needs improvement - auth method state scattered across components

The PR successfully generalizes the multi-step form pattern, making it reusable for all object storage connectors. The main concern is state management complexity around selectedAuthMethod, which should be centralized in the store before merging.

[royendo]

GCS: https://jam.dev/c/74536bbf-e99d-4381-b4ad-f9afd6b007a4

• when switching between JSON and HMAC, there's a split sec where HMAC shows missing creds in red
• when skipping using "already connected?" I cannot add a model, the button flickers but does nothing (see end of recording)

S3: https://jam.dev/c/1f2b5a0d-2ba3-46c4-95fb-73ce3d93b459

• connectors are iterating s3, s4, s5 intead of s3, s3_1, s3_2
• same as above: when skipping using "already connected?" I cannot add a model, the button flickers but does nothing

Azure: https://jam.dev/c/a9dfe375-4ed9-4c8a-9168-0451e2894ab2

• same as above: when skipping using "already connected?" I cannot add a model, the button flickers but does nothing
  Overall good! Thanks for adding the additional changes that I messaged you with!

[lovincyrus]

GCS: https://jam.dev/c/74536bbf-e99d-4381-b4ad-f9afd6b007a4

• when switching between JSON and HMAC, there's a split sec where HMAC shows missing creds in red
• when skipping using "already connected?" I cannot add a model, the button flickers but does nothing (see end of recording)

S3: https://jam.dev/c/1f2b5a0d-2ba3-46c4-95fb-73ce3d93b459

• connectors are iterating s3, s4, s5 intead of s3, s3_1, s3_2
• same as above: when skipping using "already connected?" I cannot add a model, the button flickers but does nothing

Azure: https://jam.dev/c/a9dfe375-4ed9-4c8a-9168-0451e2894ab2

• same as above: when skipping using "already connected?" I cannot add a model, the button flickers but does nothing
  Overall good! Thanks for adding the additional changes that I messaged you with!

Fixed! @royendo

Resolved

[royendo]

still creating s3, s4 file names

[lovincyrus]

still creating s3, s4 file names

Have migrated the fix from #8354

[royendo]

Looks good from Product QA.

[ericpgreen2]

Hey Cyrus,

A while back we wrote a design doc for connector forms that aligned on using JSON Schema as the form definition format: https://www.notion.so/rilldata/Snippets-API-design-doc-1f5ba33c8f5780798a0cfc819a406b42

The idea is that these schemas eventually live on the backend and can be used by both UI and AI/CLI.

Defining a config-driven form system that works across multiple connectors is definitely the right direction! But the custom TypeScript format (authFieldGroups, clearFieldsByMethod, etc.) diverges from that design. Instead, let's define these as JSON Schema in the frontend. This gets us closer to the design doc without requiring backend work now, and when we move the schemas to the backend later, it's just moving files.

Here's a rough sketch of what S3 might look like:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "auth_method": {
      "type": "string",
      "title": "Authentication method",
      "enum": ["access_keys", "public"],
      "default": "access_keys",
      "x-display": "radio"
    },
    "aws_access_key_id": {
      "type": "string",
      "title": "Access Key ID",
      "description": "AWS access key ID for the bucket",
      "x-secret": true,
      "x-step": "connector",
      "x-visible-if": { "auth_method": "access_keys" }
    },
    "aws_secret_access_key": {
      "type": "string",
      "title": "Secret Access Key",
      "description": "AWS secret access key for the bucket",
      "x-secret": true,
      "x-step": "connector",
      "x-visible-if": { "auth_method": "access_keys" }
    },
    "path": {
      "type": "string",
      "title": "S3 URI",
      "pattern": "^s3://",
      "x-step": "source"
    },
    "name": {
      "type": "string",
      "title": "Model name",
      "pattern": "^[a-zA-Z_][a-zA-Z0-9_]*$",
      "x-step": "source"
    }
  },
  "allOf": [
    {
      "if": { "properties": { "auth_method": { "const": "access_keys" } } },
      "then": { "required": ["aws_access_key_id", "aws_secret_access_key"] }
    }
  ]
}

This is illustrative—you'll need to think through the right JSON Schema properties. Try to stay as close to native JSON Schema as possible before reaching for custom x-* extensions. The ones I used above:

• x-secret: mask the input field
• x-step: which step of the multi-step flow this field belongs to
• x-display: UI hint for how to render (radio, select, etc.)
• x-visible-if: conditional visibility based on other field values

There may be better ways to model some of these. Let me know if you want to talk through it.

---

Developed in collaboration with Claude Code

[ericpgreen2]

See the comment above. A declarative config language to define these forms is definitely the right direction! It's so close to the JSON Schema approach we had discussed previously. This seems like a good time to take your code here one step closer to that design doc.

[lovincyrus]

Hey Cyrus,

A while back we wrote a design doc for connector forms that aligned on using JSON Schema as the form definition format: https://www.notion.so/rilldata/Snippets-API-design-doc-1f5ba33c8f5780798a0cfc819a406b42

The idea is that these schemas eventually live on the backend and can be used by both UI and AI/CLI.

Defining a config-driven form system that works across multiple connectors is definitely the right direction! But the custom TypeScript format (authFieldGroups, clearFieldsByMethod, etc.) diverges from that design. Instead, let's define these as JSON Schema in the frontend. This gets us closer to the design doc without requiring backend work now, and when we move the schemas to the backend later, it's just moving files.

Here's a rough sketch of what S3 might look like:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "auth_method": {
      "type": "string",
      "title": "Authentication method",
      "enum": ["access_keys", "public"],
      "default": "access_keys",
      "x-display": "radio"
    },
    "aws_access_key_id": {
      "type": "string",
      "title": "Access Key ID",
      "description": "AWS access key ID for the bucket",
      "x-secret": true,
      "x-step": "connector",
      "x-visible-if": { "auth_method": "access_keys" }
    },
    "aws_secret_access_key": {
      "type": "string",
      "title": "Secret Access Key",
      "description": "AWS secret access key for the bucket",
      "x-secret": true,
      "x-step": "connector",
      "x-visible-if": { "auth_method": "access_keys" }
    },
    "path": {
      "type": "string",
      "title": "S3 URI",
      "pattern": "^s3://",
      "x-step": "source"
    },
    "name": {
      "type": "string",
      "title": "Model name",
      "pattern": "^[a-zA-Z_][a-zA-Z0-9_]*$",
      "x-step": "source"
    }
  },
  "allOf": [
    {
      "if": { "properties": { "auth_method": { "const": "access_keys" } } },
      "then": { "required": ["aws_access_key_id", "aws_secret_access_key"] }
    }
  ]
}

This is illustrative—you'll need to think through the right JSON Schema properties. Try to stay as close to native JSON Schema as possible before reaching for custom x-* extensions. The ones I used above:

• x-secret: mask the input field
• x-step: which step of the multi-step flow this field belongs to
• x-display: UI hint for how to render (radio, select, etc.)
• x-visible-if: conditional visibility based on other field values

There may be better ways to model some of these. Let me know if you want to talk through it.

Developed in collaboration with Claude Code

Added the json schema changes in 83bfcdf

resolved

[ericpgreen2]

Nice work adopting JSON Schema—this is the right direction.

One structural concern: the current flow is JSON Schema → getMultiStepFormConfig() → custom config → MultiStepFormRenderer. The ~200-line getMultiStepFormConfig() function transforms the schema into a different format that the renderer consumes. This defeats the purpose of using JSON Schema—the value is that consumers can interpret the schema directly, without needing a translation layer that duplicates the structure in a different format.

A cleaner approach would be a JSONSchemaFormRenderer that consumes the schema directly—interpreting properties, x-visible-if, allOf/if/then, etc. without the intermediate transformation. This is how libraries like react-jsonschema-form work. The renderer would emit form values via a callback (e.g., onSubmit={(values) => ...}), which the parent can use to call the existing submitAddSourceForm/submitAddConnectorForm functions.

Keep the renderer generic—use slots for context-specific UI outside the form fields themselves: the "Already connected? Import your data" link, connector-specific error messages, etc. That way the same renderer can be reused for non-connector templates later.

For incremental migration, isolate the new pattern in its own top-level feature directory to align with the design doc:

web-common/src/features/templates/
  schemas/
    s3.ts
    gcs.ts
    azure.ts
  JSONSchemaFormRenderer.svelte
  types.ts
  utils.ts

This keeps the new approach cleanly separated and positions it as a general pattern for template-based code generation, not just source connectors.

Optionally, you could also extract the file generation logic from submitAddDataForm.ts into this directory with an interface that mirrors the future backend API:

// Today: implemented client-side
// Tomorrow: calls RuntimeService.GenerateFile
async function generateFile(args: {
  template: string;
  properties: Record<string, unknown>;
  preview?: boolean;
}): Promise<{
  addedPaths: string[];
  modifiedPaths: string[];
  previews: Record<string, string>; // path → YAML
}>

This would make the backend migration a simple swap—same interface, different implementation.

Let me know if you want to talk through any of this.

---

Developed in collaboration with Claude Code

[ericpgreen2]

See above feedback on the JSON Schema implementation

resolved

[ericpgreen2]

Good progress on the JSONSchemaFormRenderer—it's much better now that the intermediate transformation layer is gone.

One more structural issue: the renderer still has connector-specific logic baked in:

• Hardcoded step: "connector" | "source" types
• Auth-specific handling (authMethod, authInfo, authMethodKey)
• Special "Authentication method" label
• Logic that assumes the auth-method-with-nested-fields pattern

This makes it a "connector form renderer" rather than a generic "JSON Schema form renderer."

The principle: The renderer should be a pure function of the schema. It shouldn't know about connectors, sources, or auth methods. It should just:

1. Render fields based on schema properties
2. Filter by x-step if a step prop is passed (but step is just a string, not specifically "connector"/"source")
3. Show/hide fields based on x-visible-if evaluated against current form values
4. Handle x-display: radio for any enum field—not just auth methods

The auth method pattern (radio with nested fields) is just one instance of a general pattern: "enum field with conditionally visible related fields." The renderer should handle that generically.

Connector-specific logic belongs in a parent component. Some naming options:

• ObjectStorageConnectorFlow.svelte - describes what it's for
• MultiStepConnectorForm.svelte - describes the pattern
• ConnectorFormWizard.svelte - wizard pattern

This parent component handles:

• The multi-step flow (connector → source)
• Button labels that change based on auth method
• "Already connected? Import your data" skip link
• Connection testing before advancing steps

The renderer just renders fields; the parent orchestrates the workflow.

Directory structure: The sources/modal/ directory is getting unwieldy. I'd suggest splitting things up:

web-common/src/features/templates/
  JSONSchemaFormRenderer.svelte   # Generic, reusable
  types.ts
  utils.ts
  schemas/
    s3.ts
    gcs.ts
    azure.ts

web-common/src/features/sources/modal/
  ObjectStorageConnectorFlow.svelte   # Connector-specific workflow
  AddDataForm.svelte                   # Existing, uses the flow component
  ...

The generic rendering infrastructure lives in templates/. The connector-specific workflow stays in sources/modal/ and imports from templates/. The schemas live in templates/ since they'll eventually be served from the backend per the design doc.

Let me know if you want to talk through any of this.

---

Developed in collaboration with Claude Code

[lovincyrus]

Validation Architecture

The current implementation reads the JSON Schema and translates it into Yup validation via makeAuthOptionValidationSchema(). This adds an unnecessary layer of indirection. SuperForms has a built-in JSON Schema adapter that can validate directly against the schema.

See the SuperForms JSON Schema documentation for details.

Recommended changes:

1. Install @exodus/schemasafe:

   npm i -D @exodus/schemasafe

2. If using client-side validation, update vite.config.ts to include it in optimizeDeps:

   optimizeDeps: {
     include: ['@exodus/schemasafe']
   }

3. In AddDataFormManager.ts, use the schemasafe adapter for multi-step connectors:

   import { schemasafe } from "sveltekit-superforms/adapters";
   
   // For multi-step connectors with JSON Schema
   const paramsAdapter = schemasafe(jsonSchema);

4. Remove makeAuthOptionValidationSchema() from FormValidation.ts - the JSON Schema's allOf conditionals handle conditional required fields directly.
5. Remove the multi-step connector entries from yupSchemas.ts (s3_connector, gcs_connector, azure_connector) since validation will come from the JSON Schema.

This makes the JSON Schema the single source of truth for both form structure and validation.

MultiStepConnectorFlow: Remove Dead Code 8222f10

The {#if activeSchema} conditional and FormRenderer fallback are unnecessary. MultiStepConnectorFlow is only rendered for multi-step connectors, and all multi-step connectors have JSON Schemas defined. The {:else} branch is dead code.

Simplify to:

<AddDataFormSection
  id={paramsFormId}
  enhance={paramsEnhance}
  onSubmit={paramsSubmit}
>
  <JSONSchemaFormRenderer
    schema={activeSchema}
    step={stepState.step}
    form={paramsForm}
    errors={$paramsErrors}
    {onStringInputChange}
    {handleFileUpload}
  />
</AddDataFormSection>

JSONSchemaFieldControl Location and Naming 3e27775

This component is generic and should live in features/templates/ alongside JSONSchemaFormRenderer.svelte.

The name "JSONSchemaFieldControl" is also verbose. Consider renaming to SchemaField.svelte - it's concise and clear in the context of the templates directory.

Import Direction 1aa737b

JSONSchemaFormRenderer.svelte imports isStepMatch from connector-schemas.ts:

import { isStepMatch } from "../sources/modal/connector-schemas";

This creates a dependency from generic templates/ code back to connector-specific sources/modal/ code. The isStepMatch function is generic (it just checks x-step) and should live in schema-utils.ts alongside the other schema utilities.

Minor: findRadioEnumKey Fallback 3d87f28

In schema-utils.ts line 46:

return schema.properties.auth_method ? "auth_method" : null;

This fallback to auth_method is connector-specific. The function should just return null if no radio enum with x-display: "radio" is found - the loop above already handles the generic case.

Developed in collaboration with Claude Code

Address this round of feedback. The multi-step connectors now only depend on the schema-driven design, with the validation pattern strictly enforced. It is not trivial to migrate to schemasafe in this branch. We should perform this migration in a follow-up to align the current json schema with schemasafe.

resolved

[ericpgreen2]

Thanks for addressing the feedback. The cleanup looks good - the dead code removal, SchemaField rename/relocation, and isStepMatch move all look correct.

Regarding the validation architecture: I'd prefer we implement schemasafe before merging rather than deferring to a follow-up. The concern is that the translation layer in FormValidation.ts (~150 lines manually reimplementing what schemasafe handles) becomes tech debt that tends to linger. Your comment about needing to "align the current json schema with schemasafe" also suggests there may be schema adjustments required - those are easier to make now than after this is on main.

Suggestion: Create a new branch off this one and implement the schemasafe migration there. We can review both together and merge once the architecture is clean. If you hit genuine blockers with schemasafe, we can reassess at that point.

One additional item: SchemaField.svelte still imports from sources/modal:

import { normalizeErrors } from "../sources/modal/utils";

This creates a dependency from generic templates/ code back to connector-specific code. normalizeErrors is a generic utility and should be moved within templates/.

---

Developed in collaboration with Claude Code

[lovincyrus]

Thanks for addressing the feedback. The cleanup looks good - the dead code removal, SchemaField rename/relocation, and isStepMatch move all look correct.

Regarding the validation architecture: I'd prefer we implement schemasafe before merging rather than deferring to a follow-up. The concern is that the translation layer in FormValidation.ts (~150 lines manually reimplementing what schemasafe handles) becomes tech debt that tends to linger. Your comment about needing to "align the current json schema with schemasafe" also suggests there may be schema adjustments required - those are easier to make now than after this is on main.

Suggestion: Create a new branch off this one and implement the schemasafe migration there. We can review both together and merge once the architecture is clean. If you hit genuine blockers with schemasafe, we can reassess at that point.

One additional item: SchemaField.svelte still imports from sources/modal:

import { normalizeErrors } from "../sources/modal/utils";

This creates a dependency from generic templates/ code back to connector-specific code. normalizeErrors is a generic utility and should be moved within templates/.

Developed in collaboration with Claude Code

Okay, addressed both! I managed to add a schemasafe-based helper in jsonSchemaValidator and removed the ~150 lines custom code tech debt.

resolved