Skip to content

Conversation

@katinthehatsite
Copy link
Contributor

Related issues

Closes STU-1099

Proposed Changes

This PR refactors CreateSiteForm to take features prop and an onSubmit prop instead of all the value and setter props that it currently does.

Testing Instructions

TBD

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@katinthehatsite katinthehatsite self-assigned this Dec 23, 2025
@katinthehatsite katinthehatsite marked this pull request as draft December 23, 2025 10:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the CreateSiteForm component to manage its own state internally instead of receiving numerous value and setter props, reducing props drilling. The form now accepts defaultValues for initialization and an onSubmit callback that receives all form values, simplifying the component hierarchy and data flow.

Key Changes

  • The CreateSiteForm now owns its form state internally (site name, path, PHP/WP versions, custom domain settings) instead of relying on lifted state
  • The useAddSite hook has been restructured to expose path validation functions (selectPath, generateProposedPath) and a site creation function (handleCreateSite) instead of managing form state
  • The form submission mechanism now uses native form submission with a form ID, allowing the stepper's "Add site" button to submit the form via the form attribute

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/modules/add-site/index.tsx Refactored to pass defaultValues and callback functions to CreateSite instead of managing form state; added logic to build default values with blueprint preferred versions
src/modules/add-site/components/create-site.tsx Updated props interface to accept defaultValues and callbacks instead of individual state values and setters
src/modules/add-site/components/create-site-form.tsx Major refactor to internalize form state management; now accepts defaultValues for initialization and onSubmit callback that receives complete form values
src/modules/add-site/components/stepper.tsx Modified action button to use native form submission with createFormId for create paths instead of onClick handler
src/modules/add-site/hooks/use-stepper.ts Added isCreatePath boolean to identify when the form is on a create path
src/modules/add-site/components/index.ts Added exports for new types: CreateSiteFormValues, CreateSiteFormProps, and PathValidationResult
src/hooks/use-add-site.ts Significantly refactored to remove form state management and expose path validation and site creation functions instead; added type definitions for CreateSiteFormValues and PathValidationResult
src/hooks/tests/use-add-site.test.tsx Updated tests to work with new API that uses handleCreateSite with form values instead of individual setters

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +434 to 450
{ existingDomainNames.length >= 0 && (
<>
<div className="flex items-center gap-2 mt-4">
<input
type="checkbox"
id="use-custom-domain"
checked={ useCustomDomain }
onChange={ ( e ) => setUseCustomDomain( e.target.checked ) }
/>
<label htmlFor="use-custom-domain">{ __( 'Use custom domain' ) }</label>
</div>

{ setUseCustomDomain && setCustomDomain && (
<div className="text-a8c-gray-50 text-xs mt-2">
{ __( 'Your system password will be required to set up the domain.' ) }
</div>
<div className="text-a8c-gray-50 text-xs mt-2">
{ __( 'Your system password will be required to set up the domain.' ) }
</div>
</>
) }
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition existingDomainNames.length >= 0 will always be true since array lengths are never negative. This means the custom domain checkbox and related UI will always be shown. If the intent was to conditionally show this based on whether domain names are available, the condition should likely check if existingDomainNames is defined or has been loaded (e.g., checking for undefined or checking a loading state).

Copilot uses AI. Check for mistakes.
},
[ useCustomDomain, existingDomainNames ]
);

Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom domain error is only validated when the user changes the custom domain input, but not when useCustomDomain changes. If a user enables the custom domain checkbox without typing anything, the validation error won't be set until they interact with the domain input field. This could allow form submission with an invalid (empty) custom domain when the checkbox is enabled.

Consider adding validation when useCustomDomain changes to ensure errors are displayed immediately.

Suggested change
// Keep custom domain error in sync when useCustomDomain or customDomain changes
useEffect( () => {
setCustomDomainError(
getDomainNameValidationError(
useCustomDomain,
customDomain ?? '',
existingDomainNames
)
);
}, [ useCustomDomain, customDomain, existingDomainNames ] );

Copilot uses AI. Check for mistakes.
Comment on lines +402 to +421
const defaultValues = useMemo(
() => ( {
siteName: defaultSiteName,
sitePath: defaultSitePath,
phpVersion: isDeeplinkFlow ? deeplinkPhpVersion : defaultPhpVersion,
wpVersion: isDeeplinkFlow
? deeplinkWpVersion
: latestStableVersion?.value ?? defaultWpVersion,
} ),
[
defaultSiteName,
defaultSitePath,
defaultPhpVersion,
defaultWpVersion,
deeplinkPhpVersion,
deeplinkWpVersion,
isDeeplinkFlow,
latestStableVersion,
]
);
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaultValues memo recalculates every time its dependencies change, including when latestStableVersion changes. However, the form component only uses these values during initialization via useState. This means changes to defaultValues after the form has mounted won't take effect. This creates an inconsistency where the memoized values update but the actual form state doesn't reflect these updates.

This is particularly problematic for the /blueprint/select/create and /blueprint/deeplink/create paths where getDefaultValuesWithBlueprint() is called, as blueprint preferred versions may be applied after the form has already been initialized with the default values.

Copilot uses AI. Check for mistakes.
Comment on lines 22 to +40

export interface CreateSiteFormValues {
siteName: string;
sitePath: string;
phpVersion: AllowedPHPVersion;
wpVersion: string;
useCustomDomain: boolean;
customDomain: string | null;
enableHttps: boolean;
}

export interface PathValidationResult {
path: string;
name?: string;
isEmpty: boolean;
isWordPress: boolean;
error?: string;
}

Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CreateSiteFormValues and PathValidationResult interfaces are defined in both src/hooks/use-add-site.ts and src/modules/add-site/components/create-site-form.tsx. While they appear identical now, having duplicate type definitions can lead to maintenance issues and potential type mismatches if one is updated without updating the other.

Consider defining these types in a single shared location (such as in the hook file) and importing them in both places to ensure type consistency.

Suggested change
export interface CreateSiteFormValues {
siteName: string;
sitePath: string;
phpVersion: AllowedPHPVersion;
wpVersion: string;
useCustomDomain: boolean;
customDomain: string | null;
enableHttps: boolean;
}
export interface PathValidationResult {
path: string;
name?: string;
isEmpty: boolean;
isWordPress: boolean;
error?: string;
}
import type { CreateSiteFormValues, PathValidationResult } from 'src/hooks/use-add-site';

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +48
/** Default values for form initialization (only used once on mount) */
defaultValues?: {
siteName?: string;
sitePath?: string;
phpVersion?: AllowedPHPVersion;
wpVersion?: string;
};
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc comment states "Default values for form initialization (only used once on mount)", but this behavior isn't enforced. The form initializes state from defaultValues in useState calls, which only run once on mount. However, if the parent re-renders with different defaultValues after mount, those changes will be ignored. This could lead to confusion where developers expect the form to update when they change defaultValues.

Consider either adding a useEffect to sync the form state with defaultValues changes, or adding a key prop to the form component so it remounts when values change, or documenting this limitation more prominently.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants