Address mgmt/separate translation #2323
Conversation
- move addressManagement translation resources (profile.json -> editProfile.json).
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several enhancements across multiple files, focusing on type safety, component organization, and address management functionalities. Notable changes include the addition of new components for address handling, updates to existing components for improved type imports, and modifications to styling files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (24)
src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressFormButtons.tsx (4)
1-3: Consider restructuring the common components directory.The
WebappButtoncomponent is accessed through a deep relative path (../../../../../common/WebappButton). Since it's a common component, consider moving it to a more accessible location (e.g.,@components/commonor@common) and using absolute imports to improve maintainability and reduce the complexity of import paths.
5-9: Add JSDoc documentation to the Props interface.While the interface is well-typed, adding JSDoc documentation would improve developer experience by providing clear descriptions of each prop's purpose and usage.
+/** + * Props for the AddressFormButtons component + */ interface Props { + /** Text to display on the submit button */ text: string; + /** Handler function for form submission */ handleSubmit: () => void; + /** Handler function for cancellation */ handleCancel: () => void; }
20-21: Consider using more generic class names for better reusability.The button class names (
cancelButtonandaddAddressButton) are specific to address management. Since this is a reusable micro-component, consider using more generic class names likesecondaryButtonandprimaryButton.- buttonClasses={styles.cancelButton} + buttonClasses={styles.secondaryButton} - buttonClasses={styles.addAddressButton} + buttonClasses={styles.primaryButton}Also applies to: 27-28
11-31: Consider adding error handling and prop validation.As this is a reusable component, consider:
- Wrapping the component with an error boundary to gracefully handle rendering errors
- Adding prop validation to ensure required props are provided with appropriate values
Example implementation:
import { ErrorBoundary } from 'react-error-boundary'; const AddressFormButtons = ({ text, handleSubmit, handleCancel }: Props) => { // Validate props if (!text) { throw new Error('AddressFormButtons: text prop is required'); } // ... rest of the component }; // Usage with error boundary export default function AddressFormButtonsWithErrorBoundary(props: Props) { return ( <ErrorBoundary fallback={<div>Something went wrong</div>}> <AddressFormButtons {...props} /> </ErrorBoundary> ); }src/features/user/Settings/EditProfile/AddressManagment/UpdateAddressType.tsx (2)
17-24: Add JSDoc comments to document the Props interface.While the props are well-typed, adding JSDoc comments would improve documentation and provide better IDE hints.
+/** + * Props for the UpdateAddressType component + */ interface Props { + /** Type of address to be set */ addressType: 'primary' | 'mailing'; + /** Function to control modal visibility */ setIsModalOpen: SetState<boolean>; + /** Current user address if it exists */ userAddress: Address | undefined; + /** Address selected for the update action */ selectedAddressForAction: Address | null; + /** Callback to refresh user addresses */ updateUserAddresses: () => Promise<void>; + /** Function to set the current address action */ setAddressAction: SetState<AddressAction | null>; }
68-108: Enhance accessibility with ARIA labels and roles.While the UI structure is clean, adding ARIA attributes would improve accessibility.
- <div className={styles.addressActionContainer}> + <div + className={styles.addressActionContainer} + role="dialog" + aria-labelledby="address-type-header" + aria-describedby="address-type-description" + > - <h2 className={styles.header}> + <h2 id="address-type-header" className={styles.header}> {tAddressManagement(`addressType.${addressType}`)} </h2> - <p> + <p id="address-type-description"> {tAddressManagement('addressPrompts.setAddressConfirmation', { addressType: tAddressManagement(`addressType.${addressType}`), })}src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressForm.tsx (3)
79-80: Remove unused dependencies inuseCallbackhooksIn the dependencies array of your
useCallbackhooks forhandleSuggestAddressandhandleGetAddress,geocoderis included but not used within the functions. Including unused dependencies can lead to unnecessary re-creations of the functions.Apply this diff to remove
geocoderfrom the dependencies:), - [geocoder, country] + [country] );), - [geocoder, setValue] + [setValue] );Also applies to: 101-102
136-140: Remove unnecessary 'required' validation message for optional fieldThe
address2field is optional, but the validation messages include a 'required' message. This could cause confusion or display unnecessary error messages.Apply this diff to remove the 'required' validation message:
validationMessages={{ - required: t('validationErrors.addressRequired'), invalid: t('validationErrors.addressInvalid'), }}
95-98: Include 'state' when setting address details from fetched dataWhen updating form values with fetched address details in
handleGetAddress, you set theaddress,city, andzipCodefields but omit thestate. If thestateinformation is available, it should be included to provide a complete address.Modify the code to set the
statevalue:async (value: string) => { const details = await fetchAddressDetails(value); if (details) { setValue('address', details.address, { shouldValidate: true }); setValue('city', details.city, { shouldValidate: true }); setValue('zipCode', details.zipCode, { shouldValidate: true }); + setValue('state', details.state, { shouldValidate: true }); }src/features/user/Settings/EditProfile/AddressManagment/index.tsx (1)
172-172: Remove unnecessary Fragment wrapper around 'renderModalContent'The
Fragmentcomponent wrappingrenderModalContentis unnecessary since it contains only a single child. Removing it simplifies the code without impacting functionality.Apply this diff to remove the unnecessary
Fragment:- <>{renderModalContent}</> + {renderModalContent}🧰 Tools
🪛 Biome (1.9.4)
[error] 172-172: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
src/features/user/Settings/EditProfile/AddressManagment/microComponents/FormattedAddressBlock.tsx (1)
21-26: Consider adding semantic HTML improvementsWhile using the
addresstag is semantically correct, consider adding ARIA labels for better accessibility.- <address> + <address aria-label="User address details"> <p>{address}</p> {address2 !== null && <p>{address2}</p>} <p>{cityStatePostalString}</p> </address>src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressList.tsx (3)
21-21: Consider using optional chaining consistentlyWhile using nullish coalescing is good, the optional chaining on
addresses?.lengthseems unnecessary sinceaddressesis required by the Props interface.-const addressCount = addresses?.length ?? 0; +const addressCount = addresses.length;
24-35: Add empty state handling and error boundaryThe component should handle empty states and potential rendering errors gracefully.
+ if (addressCount === 0) { + return ( + <div className={styles.addressListContainer}> + <p>No addresses found</p> + </div> + ); + } + return ( <div className={styles.addressListContainer}> {addresses.map((address) => ( + address && ( <SingleAddress key={address.id} userAddress={address} addressCount={addressCount} setAddressAction={setAddressAction} setIsModalOpen={setIsModalOpen} setSelectedAddressForAction={setSelectedAddressForAction} /> + ) ))} </div> );
8-13: Consider adding JSDoc comments for better documentationThe Props interface would benefit from JSDoc comments explaining the purpose of each prop.
+/** + * Props for the AddressList component + * @interface Props + * @property {Address[]} addresses - List of user addresses + * @property {SetState<AddressAction | null>} setAddressAction - Callback to set the current address action + * @property {SetState<Address | null>} setSelectedAddressForAction - Callback to set the selected address + * @property {SetState<boolean>} setIsModalOpen - Callback to control modal visibility + */ interface Props { addresses: Address[]; setAddressAction: SetState<AddressAction | null>; setSelectedAddressForAction: SetState<Address | null>; setIsModalOpen: SetState<boolean>; }src/features/user/Settings/EditProfile/AddressManagment/microComponents/SingleAddress.tsx (2)
1-15: Fix typo in directory nameThe directory name contains a typo: "AddressManagment" should be "AddressManagement". This affects import paths and could cause confusion.
9-15: Add validation for addressCount propConsider adding runtime validation to ensure
addressCountis a positive number, as negative values wouldn't make sense in this context.Example validation:
interface Props { userAddress: Address; - addressCount: number; + addressCount: number & { __brand: 'PositiveNumber' }; setIsModalOpen: SetState<boolean>; setAddressAction: SetState<AddressAction | null>; setSelectedAddressForAction: SetState<Address | null>; } + +// Type guard for positive numbers +const isPositiveNumber = (n: number): n is number & { __brand: 'PositiveNumber' } => n > 0;src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressInput.tsx (1)
47-52: Add error handling for invalid suggestion objectsThe
getOptionLabelfunction could throw if an invalid object is passed. Consider adding error handling.const getOptionLabel = (option: string | AddressOption): string => { if (typeof option === 'string') { return option; } - return 'text' in option ? option.text : ''; + try { + if (!option || typeof option !== 'object') { + console.warn('Invalid option object received:', option); + return ''; + } + return 'text' in option ? option.text : ''; + } catch (error) { + console.error('Error processing option:', error); + return ''; + } };src/features/user/Settings/EditProfile/AddressManagment/DeleteAddress.tsx (2)
16-21: Props interface looks good but could be more strictThe Props interface is well-defined, but
addressIdbeing optional (undefined) could lead to runtime issues if not properly handled.Consider making
addressIdrequired and handling the undefined case at a higher level:interface Props { setIsModalOpen: SetState<boolean>; - addressId: string | undefined; + addressId: string; updateUserAddresses: () => Promise<void>; setAddressAction: SetState<AddressAction | null>; }
67-81: Improve accessibility of action buttonsThe button container needs ARIA labels for better screen reader support.
-<div className={styles.buttonContainer}> +<div className={styles.buttonContainer} role="group" aria-label="Address deletion actions"> <WebappButton text={tCommon('cancel')} elementType="button" variant="secondary" onClick={handleCancel} + aria-label={tCommon('cancel')} /> <WebappButton text={tAddressManagement('deleteAction.delete')} elementType="button" variant="primary" onClick={deleteAddress} + aria-label={tAddressManagement('deleteAction.delete')} /> </div>src/features/user/Settings/EditProfile/AddressManagment/EditAddress.tsx (1)
47-88: Optimize useCallback dependenciesThe useCallback hook includes unnecessary dependencies that could cause unnecessary re-renders:
handleErrorandputAuthenticatedRequestare stable and don't need to be dependencies- Some dependencies could be memoized at a higher level
useCallback( async (data: FormData) => { // ... function implementation ... }, [ contextLoaded, user, token, country, selectedAddressForAction?.type, selectedAddressForAction?.id, tenantConfig.id, logoutUser, updateUserAddresses, - handleError, - putAuthenticatedRequest, ] );src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressActionMenu.tsx (1)
90-104: Improve Popover accessibility and positioningThe Popover component needs additional ARIA attributes and better positioning logic.
<Popover id={id} open={open} anchorEl={popoverAnchor} onClose={closePopover} + role="menu" + aria-label={tAddressManagement('actions.menuLabel')} anchorOrigin={{ vertical: 'bottom', horizontal: 'left', }} + transformOrigin={{ + vertical: 'top', + horizontal: 'left', + }} sx={{ '& .MuiPaper-root': { borderRadius: '12px', + marginTop: '4px', }, }} >src/features/user/Settings/EditProfile/AddressManagment/AddAddress.tsx (1)
18-38: Consider improving type safety in FormData interface.The type definitions could be more precise to prevent potential runtime issues.
Consider this improvement:
export type FormData = { - address: string | undefined; - address2: string | null; - city: string | undefined; - zipCode: string | undefined; - state: string | null; + address: string; + address2: string | undefined; + city: string; + zipCode: string; + state: string | undefined; };This change:
- Makes required fields non-optional
- Standardizes optional fields to use
undefinedinstead of mixing withnullsrc/features/common/InputTypes/AutoCompleteCountry.tsx (1)
71-72: Consider using a type-safe translation key approach.The current implementation uses type assertions for country codes, but a more robust solution would be to ensure type safety at the translation key level.
Consider creating a type-safe translation utility:
type CountryTranslationKey = `country.${Lowercase<CountryCode>}`; function getCountryTranslationKey(code: CountryCode): CountryTranslationKey { return `country.${code.toLowerCase()}` as CountryTranslationKey; } // Usage: const label = t(getCountryTranslationKey(countryCode));This approach would provide compile-time safety for translation keys without needing type assertions.
Also applies to: 96-97, 107-107
src/features/user/Settings/EditProfile/AddressManagment/AddressManagement.module.scss (1)
177-200: Consider improving mobile responsiveness implementation.The mobile styles could be more maintainable and accessible.
- Consider using CSS variables for breakpoints:
:root { --breakpoint-xs: 576px; --min-modal-width: 320px; } @media (max-width: var(--breakpoint-xs)) { .addressFormLayout { min-width: var(--min-modal-width); // ... } }
- Add touch target sizes for mobile:
.buttonContainer { @include flex-container(column); button { width: 100%; + min-height: 44px; // iOS minimum touch target size } }
🛑 Comments failed to post (17)
src/features/user/Settings/EditProfile/AddressManagment/UpdateAddressType.tsx (1)
41-63: 🛠️ Refactor suggestion
Add validation and improve error handling in updateAddress function.
While the function handles the happy path well, it could be improved in terms of validation and error handling.
const updateAddress = async (addressType: 'primary' | 'mailing') => { if (!contextLoaded || !user || !token) return; + if (!selectedAddressForAction?.id) { + setErrors([{ message: 'No address selected for update' }]); + return; + } setIsUploadingData(true); const bodyToSend = { type: addressType, }; try { const res = await putAuthenticatedRequest<Address>( tenantConfig.id, `/app/addresses/${selectedAddressForAction?.id}`, bodyToSend, token, logoutUser ); - if (res) updateUserAddresses(); + if (res) { + await updateUserAddresses(); + } else { + setErrors([{ message: 'Failed to update address' }]); + } } catch (error) { setErrors(handleError(error as APIError)); } finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const updateAddress = async (addressType: 'primary' | 'mailing') => { if (!contextLoaded || !user || !token) return; if (!selectedAddressForAction?.id) { setErrors([{ message: 'No address selected for update' }]); return; } setIsUploadingData(true); const bodyToSend = { type: addressType, }; try { const res = await putAuthenticatedRequest<Address>( tenantConfig.id, `/app/addresses/${selectedAddressForAction?.id}`, bodyToSend, token, logoutUser ); if (res) { await updateUserAddresses(); } else { setErrors([{ message: 'Failed to update address' }]); } } catch (error) { setErrors(handleError(error as APIError)); } finally { setIsUploadingData(false); setIsModalOpen(false); setAddressAction(null); } };src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressForm.tsx (1)
209-215: 🛠️ Refactor suggestion
Integrate 'country' field with form control for consistency
The
CountrySelectcomponent is handling its own state separately fromreact-hook-form, which may lead to inconsistencies in validation and form submission behavior compared to other fields.Consider using the
Controllercomponent to integrate thecountryfield withreact-hook-form:- <CountrySelect - countries={allCountries} - label={t('fieldLabels.country')} - name="country" - defaultValue={country} - onChange={setCountry} - /> + <Controller + name="country" + control={control} + defaultValue={country} + rules={{ + required: t('validationErrors.countryRequired'), + }} + render={({ field: { onChange, value, onBlur } }) => ( + <CountrySelect + countries={allCountries} + label={t('fieldLabels.country')} + value={value} + onChange={(newValue) => { + onChange(newValue); + setCountry(newValue); + }} + onBlur={onBlur} + /> + )} + />This integrates
countryinto the form's control flow, ensuring consistent validation and state management.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<Controller name="country" control={control} defaultValue={country} rules={{ required: t('validationErrors.countryRequired'), }} render={({ field: { onChange, value, onBlur } }) => ( <CountrySelect countries={allCountries} label={t('fieldLabels.country')} value={value} onChange={(newValue) => { onChange(newValue); setCountry(newValue); }} onBlur={onBlur} /> )} />src/utils/addressManagement.ts (1)
8-8:
⚠️ Potential issueCorrect the class name 'GeocoderArcGIs' to 'GeocoderArcGIS'
The imported class name
GeocoderArcGIsis misspelled. It should beGeocoderArcGIS, with 'GIS' in uppercase letters.Apply this diff to correct the class name:
-import GeocoderArcGIs from 'geocoder-arcgis'; +import GeocoderArcGIS from 'geocoder-arcgis';And update the class instantiation:
-export const geocoder = new GeocoderArcGIs( +export const geocoder = new GeocoderArcGIS(Also applies to: 59-59
src/features/user/Settings/EditProfile/AddressManagment/index.tsx (3)
43-49:
⚠️ Potential issueAvoid mutating state directly when sorting 'userAddresses'
The
sortmethod mutates the array in place. SinceuserAddressesis a state variable, mutating it directly can lead to unexpected behavior. Create a copy of the array before sorting to prevent direct mutation of state.Apply this diff to create a new array before sorting:
- return userAddresses.sort((a, b) => { + return [...userAddresses].sort((a, b) => { return ( addressTypeOrder.indexOf(a.type) - addressTypeOrder.indexOf(b.type) ); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const sortedAddresses = useMemo(() => { return [...userAddresses].sort((a, b) => { return ( addressTypeOrder.indexOf(a.type) - addressTypeOrder.indexOf(b.type) ); }); }, [userAddresses]);
51-64:
⚠️ Potential issueInclude 'setErrors' in the dependency array of 'updateUserAddresses'
The
setErrorsfunction is used withinupdateUserAddressesbut is not included in the dependency array of theuseCallbackhook. To ensure consistent behavior and prevent potential issues with stale closures, includesetErrorsin the dependency array.Apply this diff to add
setErrorsto the dependency array:}, [ user, token, contextLoaded, tenantConfig.id, logoutUser, + setErrors, ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const updateUserAddresses = useCallback(async () => { if (!user || !token || !contextLoaded) return; try { const res = await getAuthenticatedRequest<Address[]>( tenantConfig.id, '/app/addresses', token, logoutUser ); if (res) setUserAddresses(res); } catch (error) { setErrors(handleError(error as APIError)); } }, [user, token, contextLoaded, tenantConfig.id, logoutUser, setErrors]);
79-138:
⚠️ Potential issueAdd 'setAddressAction' to the dependency array of 'renderModalContent'
The
setAddressActionfunction is used within therenderModalContentuseMemobut is not included in the dependency array. This could lead to inconsistencies ifsetAddressActionchanges. Include it in the dependency array to ensure proper memoization.Apply this diff to add
setAddressActionto the dependency array:}, [ setIsModalOpen, setUserAddresses, selectedAddressForAction, updateUserAddresses, primaryAddress, billingAddress, addressAction, + setAddressAction, ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const renderModalContent = useMemo(() => { switch (addressAction) { case ADDRESS_ACTIONS.ADD: return ( <AddAddress setIsModalOpen={setIsModalOpen} setUserAddresses={setUserAddresses} setAddressAction={setAddressAction} /> ); case ADDRESS_ACTIONS.EDIT: return ( <EditAddress setIsModalOpen={setIsModalOpen} selectedAddressForAction={selectedAddressForAction} updateUserAddresses={updateUserAddresses} setAddressAction={setAddressAction} /> ); case ADDRESS_ACTIONS.DELETE: return ( <DeleteAddress addressId={selectedAddressForAction?.id} setIsModalOpen={setIsModalOpen} updateUserAddresses={updateUserAddresses} setAddressAction={setAddressAction} /> ); case ADDRESS_ACTIONS.SET_PRIMARY: return ( <UpdateAddressType addressType={ADDRESS_TYPE.PRIMARY} userAddress={primaryAddress} setAddressAction={setAddressAction} setIsModalOpen={setIsModalOpen} selectedAddressForAction={selectedAddressForAction} updateUserAddresses={updateUserAddresses} /> ); case ADDRESS_ACTIONS.SET_BILLING: return ( <UpdateAddressType addressType={ADDRESS_TYPE.MAILING} userAddress={billingAddress} setAddressAction={setAddressAction} setIsModalOpen={setIsModalOpen} selectedAddressForAction={selectedAddressForAction} updateUserAddresses={updateUserAddresses} /> ); } }, [ setIsModalOpen, setUserAddresses, selectedAddressForAction, updateUserAddresses, primaryAddress, billingAddress, addressAction, setAddressAction, ]);src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressFormLayout.tsx (1)
1-7: 🛠️ Refactor suggestion
Type 'children' prop as 'ReactNode' instead of 'JSX.Element'
The
childrenprop should be typed asReactNodeto accommodate all possible types of React children, including strings, numbers, null, or arrays of elements.Apply this diff to update the import and type definition:
-import type { JSX } from 'react'; +import type { ReactNode } from 'react'; interface Props { - children: JSX.Element; + children: ReactNode; label: string; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import type { ReactNode } from 'react'; import styles from '../AddressManagement.module.scss'; interface Props { children: ReactNode; label: string; }src/features/user/Settings/EditProfile/index.tsx (1)
6-6: 🛠️ Refactor suggestion
Correct the directory name 'AddressManagment' to 'AddressManagement'
The import path includes a misspelled directory name
AddressManagment. It should beAddressManagementto reflect correct spelling and maintain consistency across the project.Update the import statement and rename the directory accordingly:
-import AddressManagement from './AddressManagment'; +import AddressManagement from './AddressManagement';Ensure all references and import paths are updated to match the corrected directory name.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import AddressManagement from './AddressManagement';src/features/user/Settings/EditProfile/AddressManagment/microComponents/FormattedAddressBlock.tsx (1)
15-15: 🛠️ Refactor suggestion
Consider improving type safety of country code conversion
The type assertion
as Lowercase<CountryCode>might be unsafe. Consider validating the country code before conversion.-const countryName = tCountry(country.toLowerCase() as Lowercase<CountryCode>); +const lowerCountry = country.toLowerCase(); +const isValidCountry = (code: string): code is Lowercase<CountryCode> => { + return /^[a-z]{2}$/.test(code); +}; +const countryName = isValidCountry(lowerCountry) + ? tCountry(lowerCountry) + : country;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const lowerCountry = country.toLowerCase(); const isValidCountry = (code: string): code is Lowercase<CountryCode> => { return /^[a-z]{2}$/.test(code); }; const countryName = isValidCountry(lowerCountry) ? tCountry(lowerCountry) : country;src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressInput.tsx (3)
13-13: 🛠️ Refactor suggestion
Improve type safety of form Control
The
Control<any>type reduces type safety. Consider using a generic type parameter for the form values.-interface AddressInputProps { +interface AddressInputProps<T extends Record<string, any>> { name: string; - control: Control<any>; + control: Control<T>; // ... } -const AddressInput: React.FC<AddressInputProps> = ({ +const AddressInput = <T extends Record<string, any>>({ // ...props -}: AddressInputProps) => { +}: AddressInputProps<T>) => {Committable suggestion skipped: line range outside the PR's diff.
6-9: 🛠️ Refactor suggestion
Improve type safety of AddressOption interface
The interface uses
[key: string]: anywhich reduces type safety. Consider defining specific optional properties instead.interface AddressOption { text: string; - [key: string]: any; + id?: string; + details?: { + street?: string; + city?: string; + country?: string; + }; }Committable suggestion skipped: line range outside the PR's diff.
63-66: 🛠️ Refactor suggestion
Add debouncing to onInputChange callback
The
onInputChangecallback is called on every input change, which could lead to excessive API calls if used for address suggestions. Consider adding debouncing.+import { useCallback } from 'react'; +import debounce from 'lodash/debounce'; const AddressInput: React.FC<AddressInputProps> = ({ // ...props }) => { + const debouncedInputChange = useCallback( + debounce((value: string) => { + onInputChange?.(value); + }, 300), + [onInputChange] + ); + return ( <Controller // ... render={({ field, fieldState: { error } }) => ( <Autocomplete // ... onInputChange={(_, newValue) => { field.onChange(newValue); - onInputChange?.(newValue); + debouncedInputChange(newValue); }}Committable suggestion skipped: line range outside the PR's diff.
src/features/user/Settings/EditProfile/AddressManagment/DeleteAddress.tsx (1)
36-54: 🛠️ Refactor suggestion
Enhance error handling and user feedback
The deleteAddress function could benefit from additional error handling and user feedback:
- No validation of addressId before API call
- No specific error message for different failure scenarios
- No success feedback to user
Consider implementing these improvements:
const deleteAddress = async () => { - if (!contextLoaded || !user || !token) return; + if (!contextLoaded || !user || !token || !addressId) { + setErrors([{ message: 'Unable to delete address: missing required data' }]); + return; + } try { setIsLoading(true); await deleteAuthenticatedRequest( tenantConfig.id, `/app/addresses/${addressId}`, token, logoutUser ); updateUserAddresses(); + // Consider showing success toast/notification } catch (error) { - setErrors(handleError(error as APIError)); + const apiError = error as APIError; + setErrors(handleError(apiError)); + // Log error for monitoring + console.error('Failed to delete address:', apiError); } finally {Committable suggestion skipped: line range outside the PR's diff.
src/features/user/Settings/EditProfile/AddressManagment/EditAddress.tsx (1)
64-66: 🛠️ Refactor suggestion
Improve type safety of API response handling
The response handling could be more robust with type checking.
- if (res && updateUserAddresses) { + if (res?.id && updateUserAddresses) { updateUserAddresses(); }Committable suggestion skipped: line range outside the PR's diff.
src/features/user/Settings/EditProfile/AddressManagment/microComponents/AddressActionMenu.tsx (1)
106-118: 🛠️ Refactor suggestion
Enhance accessibility and keyboard navigation
The menu items need improved accessibility support:
- Missing keyboard navigation
- Incomplete ARIA attributes
- Improper use of role="button" on list items
{addressActionConfig.map((item, key) => { if (!item.shouldRender) return; return ( <li key={key} className={styles.action} - onClick={() => handleActionClick(item.action)} - role="button" > + <button + onClick={() => handleActionClick(item.action)} + className={styles.actionButton} + role="menuitem" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + handleActionClick(item.action); + } + }} + > {item.label} + </button> </li> ); })}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{addressActionConfig.map((item, key) => { if (!item.shouldRender) return; return ( <li key={key} className={styles.action} > <button onClick={() => handleActionClick(item.action)} className={styles.actionButton} role="menuitem" tabIndex={0} onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { handleActionClick(item.action); } }} > {item.label} </button> </li> ); })}src/features/user/Settings/EditProfile/AddressManagment/AddAddress.tsx (1)
56-96: 🛠️ Refactor suggestion
Optimize useCallback dependencies and add validation.
Two improvements could be made to this implementation:
- Remove unnecessary dependencies:
[ contextLoaded, user, token, country, logoutUser, setUserAddresses, - handleError, setIsLoading, setIsModalOpen, - postAuthenticatedRequest, ]
- Add country validation:
async (data: FormData) => { if (!contextLoaded || !user || !token) return; + if (!country) { + setErrors([{ message: 'Please select a country' }]); + return; + } setIsLoading(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const addAddress = useCallback( async (data: FormData) => { if (!contextLoaded || !user || !token) return; if (!country) { setErrors([{ message: 'Please select a country' }]); return; } setIsLoading(true); const bodyToSend = { ...data, country, type: ADDRESS_TYPE.OTHER, }; try { const res = await postAuthenticatedRequest<Address>( tenantConfig.id, '/app/addresses', bodyToSend, token, logoutUser ); if (res && setUserAddresses) { setUserAddresses((prevAddresses) => [...prevAddresses, res]); } } catch (error) { setErrors(handleError(error as APIError)); } finally { setIsLoading(false); setIsModalOpen(false); setAddressAction(null); } }, [ contextLoaded, user, token, country, logoutUser, setUserAddresses, setIsLoading, setIsModalOpen, ] );src/features/user/Settings/EditProfile/AddressManagment/AddressManagement.module.scss (1)
51-69: 🛠️ Refactor suggestion
Add accessibility improvements for interactive elements.
The kebab menu button could be more accessible.
.kebabMenuButton { display: flex; justify-content: center; align-items: center; height: 34px; width: 34px; border-radius: 50%; background-color: transparent; cursor: pointer; + // Add focus styles for keyboard navigation + &:focus-visible { + outline: 2px solid $primaryColor; + outline-offset: 2px; + } svg { height: 20px; width: 12px; } }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@sunilsabatp Made some minor changes to the translation keys, LGTM now.
Tasks covered under this PR :
profile.json -> editProfile.json).SCSS variabledeclarations.