BA-2267: generate edit username #221
Conversation
pt-tsl
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis pull request implements a series of changes across multiple packages. It updates version numbers, moves and reorganizes components (such as the ProfileSettingsComponent and PhoneNumberInput/UsernameIcon), and refines GraphQL fragments and queries. Validation schemas and default value functions are enhanced, and new TypeScript interfaces and components are introduced. Storybook documentation and decorators—along with mock resolvers for mutation testing—have also been added to support the new features and improvements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PSComponent as ProfileSettingsComponent
participant Relay as Relay Query
participant DefaultValues as getProfileDefaultValues
User->>PSComponent: Load Profile Settings
PSComponent->>Relay: Execute ProfileSettingsRelayTestQuery
Relay-->>PSComponent: Return profile data
PSComponent->>DefaultValues: Initialize default form values (remove slashes if needed)
User->>PSComponent: Submit updated profile data
PSComponent->>Relay: Commit profile update mutation
Relay-->>PSComponent: Mutation response (success/error)
sequenceDiagram
participant Story as Storybook Story
participant HOC as withMutationResolver
participant Relay as Relay Environment
participant Mock as Mock Resolver
Story->>HOC: Render story with mutation handling
HOC->>Relay: Override executeMutation method
Relay->>Mock: Invoke mock resolver (or simulate error)
Mock-->>Relay: Return mocked mutation response
HOC-->>Story: Render final story output
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
ERR_PNPM_OPTIONAL_DEPS_REQUIRE_PROD_DEPS Optional dependencies cannot be installed without production dependencies ✨ Finishing Touches
🪧 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.
Actionable comments posted: 5
🧹 Nitpick comments (10)
packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx (1)
3-11: Consider more descriptive prop naming.While the component implementation is solid, the
enableRemoveprop name could be more descriptive of its purpose in controlling the grid layout.Consider renaming to something like
hasTwoColumnsorshowRemoveButtonto better reflect its impact on the layout:-export const BannerButtonsContainer = styled('div')<{ enableRemove: boolean }>( - ({ theme, enableRemove = false }) => ({ +export const BannerButtonsContainer = styled('div')<{ hasTwoColumns: boolean }>( + ({ theme, hasTwoColumns = false }) => ({ display: 'grid', - gridTemplateColumns: enableRemove ? '1fr 1fr' : '1fr', + gridTemplateColumns: hasTwoColumns ? '1fr 1fr' : '1fr', gridTemplateRows: '1fr', gap: theme.spacing(2), - placeSelf: enableRemove ? 'inherit' : 'center', + placeSelf: hasTwoColumns ? 'inherit' : 'center', }), )packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx (1)
26-30: Fix typo in component name.The component name has a typo.
-export const ContryTitle = styled((props) => <Typography {...props} />)<TypographyProps>( +export const CountryTitle = styled((props) => <Typography {...props} />)<TypographyProps>(packages/components/modules/profiles/common/types.ts (1)
22-25: LGTM! Interface supports username editing requirements.The new interface properly types the profile data and username slash removal functionality. Consider documenting the purpose of
removeSlashInUsernamewith JSDoc comments for better maintainability.Add JSDoc comments to explain the interface:
+/** + * Interface for getting profile form default values + * @property profile - The profile data + * @property removeSlashInUsername - Flag to control removal of slashes from username + */ export interface ProfileGetDefaultFormValues { profile?: ProfileComponentFragment$data | null removeSlashInUsername?: boolean }packages/components/modules/profiles/common/utils.ts (2)
9-12: Consider documenting the default behavior.The default value of
trueforremoveSlashInUsernameshould be documented as it affects username formatting.Add JSDoc comments to explain the function parameters and default behavior:
+/** + * Gets default values for profile form + * @param profile - The profile data + * @param removeSlashInUsername - Whether to remove slashes from username (defaults to true) + * @returns Formatted profile data with default values + */ export const getProfileDefaultValues = ({ profile, removeSlashInUsername = true, }: ProfileGetDefaultFormValues) => {
21-23: Handle multiple slashes in username.The current implementation only replaces the first slash. Consider using a global replace for consistent handling of multiple slashes.
- formattedProfile.urlPath = formattedProfile.urlPath?.replace('/', '') + formattedProfile.urlPath = formattedProfile.urlPath?.replace(/\//g, '')packages/design-system/components/web/icons/UsernameIcon/index.tsx (1)
5-16: LGTM! Icon implementation follows Material-UI patterns.The implementation is clean and follows Material-UI conventions. Consider adding aria-label for accessibility.
Add aria-label for better accessibility:
-const UsernameIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( +const UsernameIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( + <SvgIcon + sx={{ fontSize: 24, ...sx }} + aria-label="username" + {...props} + >packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
19-25: Consider making menu dimensions responsive.Hard-coded dimensions might not work well across different screen sizes and resolutions.
- style: { - height: '300px', - width: '360px', - top: '10px', - left: '-34px', - }, + style: { + maxHeight: '300px', + width: 'max(360px, 100%)', + top: '10px', + left: '-34px', + },packages/design-system/components/web/inputs/PhoneNumberField/index.tsx (1)
44-66: Enhance accessibility and performance.Consider the following improvements:
- Add proper ARIA labels for screen readers
- Memoize callback functions to prevent unnecessary rerenders
return ( <PureTextField value={inputValue} onChange={handlePhoneValueChange} type="tel" inputRef={inputRef} + aria-label="Phone number input" InputProps={{ startAdornment: ( - <InputAdornment position="start"> + <InputAdornment position="start" aria-label="Country selection"> <CountrySelect setCountry={setCountry} country={country} selectProps={selectProps} countryNameProps={countryNameProps} countryDialCodeProps={countryDialCodeProps} optionProps={optionProps} /> </InputAdornment> ), }} {...props} /> )packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (2)
35-42: Enhance example code with validation and error handling.The example code could be more comprehensive by showing:
- Form validation integration
- Error handling
- Loading states
```javascript -import PhoneNumberField from '../PhoneNumberField' +import { useForm } from 'react-hook-form' +import { PhoneNumberField } from '../PhoneNumberField' -const MyComponent = () => ( - <PhoneNumberField value="" onChange={(phone) => console.log(phone)} defaultCountry="US" /> -) +const MyComponent = () => { + const { control } = useForm() + return ( + <PhoneNumberField + control={control} + name="phoneNumber" + defaultCountry="US" + label="Phone Number" + /> + ) +} export default MyComponent--- `17-26`: **Add validation rules documentation.** The props section should include validation rules and requirements for phone numbers: - Required format - Allowed/disallowed characters - Length restrictions </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f207740142402f4b1526616be1e4ce80db998360 and dc44668ce0e65e1e04dbc5f15e8dcdb70201255d. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (34)</summary> * `packages/components/CHANGELOG.md` (1 hunks) * `packages/components/modules/profiles/common/constants.ts` (1 hunks) * `packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts` (1 hunks) * `packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts` (1 hunks) * `packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts` (0 hunks) * `packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts` (0 hunks) * `packages/components/modules/profiles/common/index.ts` (1 hunks) * `packages/components/modules/profiles/common/types.ts` (1 hunks) * `packages/components/modules/profiles/common/utils.ts` (2 hunks) * `packages/components/modules/profiles/common/zod.ts` (1 hunks) * `packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx` (2 hunks) * `packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx` (1 hunks) * `packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx` (1 hunks) * `packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts` (1 hunks) * `packages/components/modules/profiles/web/index.ts` (1 hunks) * `packages/components/package.json` (1 hunks) * `packages/design-system/CHANGELOG.md` (1 hunks) * `packages/design-system/components/web/icons/UsernameIcon/index.tsx` (1 hunks) * `packages/design-system/components/web/icons/index.ts` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/constants.ts` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/index.tsx` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx` (1 hunks) * `packages/design-system/components/web/inputs/PhoneNumberField/types.ts` (1 hunks) * `packages/design-system/components/web/inputs/index.ts` (1 hunks) * `packages/design-system/package.json` (2 hunks) * `packages/eslint-plugin/CHANGELOG.md` (1 hunks) * `packages/eslint-plugin/index.ts` (1 hunks) * `packages/eslint-plugin/package.json` (1 hunks) * `packages/wagtail/CHANGELOG.md` (1 hunks) * `packages/wagtail/package.json` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts * packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts </details> <details> <summary>✅ Files skipped from review due to trivial changes (8)</summary> * packages/design-system/components/web/inputs/PhoneNumberField/constants.ts * packages/wagtail/package.json * packages/eslint-plugin/CHANGELOG.md * packages/components/package.json * packages/wagtail/CHANGELOG.md * packages/eslint-plugin/package.json * packages/eslint-plugin/index.ts * packages/components/CHANGELOG.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Biome (1.9.4)</summary> <details> <summary>packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx</summary> [error] 82-82: Avoid the delete operator which can impact performance. Unsafe fix: Use an undefined assignment instead. (lint/performance/noDelete) --- [error] 86-86: Avoid the delete operator which can impact performance. Unsafe fix: Use an undefined assignment instead. (lint/performance/noDelete) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx</summary> [uncategorized] ~31-~31: Loose punctuation mark. Context: ...ional functionality. - `CountrySelect`: Used to display the country select drop... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (1)</summary> * GitHub Check: Analyze (javascript) </details> <details> <summary>🔇 Additional comments (19)</summary><blockquote> <details> <summary>packages/design-system/package.json (2)</summary> `4-4`: **Version Update Consistency.** The package version has been bumped from "1.0.4" to "1.0.5", which appears consistent with the new design system enhancements. Please ensure that the changelog and release notes reflect this update and that downstream packages (e.g., `@baseapp-frontend/wagtail`) are synchronized accordingly. --- `81-81`: **New Dependency Addition Verification.** The new dependency `"react-international-phone": "^4.5.0"` has been added, likely to support new components (e.g., `PhoneNumberInput`) within the design system. Confirm that this dependency is the best choice for the target environments and consider adding documentation on its usage or any configuration details if necessary. </details> <details> <summary>packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts (1)</summary> `1-5`: **LGTM! Clean type definitions.** The interface is well-defined with appropriate typing for the profile prop, allowing for loading states. </details> <details> <summary>packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx (1)</summary> `4-9`: **Consider layout impact of negative margins.** While the styling works for the current use case, the negative left margin could cause layout issues if the component is used in different contexts. Consider testing the component in various container widths and padding configurations to ensure the negative margin doesn't cause unintended overflow or alignment issues. </details> <details> <summary>packages/components/modules/profiles/common/zod.ts (1)</summary> `18-21`: **Add uniqueness validation for usernames.** The current validation correctly implements the character length and alphanumeric requirements from the PR objectives. However, the uniqueness validation mentioned in the requirements ("If the generated username already exists, the system will append a number") is not implemented here. Consider adding a custom validation rule or async validator to check username uniqueness against the database. This could be implemented as: ```typescript export const checkUsernameUniqueness = async (username: string): Promise<boolean> => { // API call to check if username exists return true // or false } // Usage in validation schema: urlPath: z.string() .min(8, { message: PROFILE_FORM_VALIDATION.urlPath.empty }) .regex(/^[a-zA-Z0-9]+$/, { message: PROFILE_FORM_VALIDATION.urlPath.invalid }) .refine(async (val) => await checkUsernameUniqueness(val), { message: "This username is already taken" })packages/design-system/components/web/inputs/index.ts (1)
3-4: LGTM! But verify alignment with PR objectives.The exports are well-organized and follow a consistent pattern. However, these changes seem unrelated to the PR objectives which focus on username generation and editing functionality.
Could you please clarify how the PhoneNumberField component relates to the username functionality described in the PR objectives?
Also applies to: 10-10
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx (1)
1-22: LGTM! Well-structured Storybook configuration.The story configuration follows best practices with:
- Proper type definitions using WithControllerProps
- Clear component title
- Sensible default props for testing
packages/design-system/components/web/inputs/PhoneNumberField/types.ts (3)
6-13: LGTM! Well-defined CountrySelect interface.The interface provides good customization options through optional props while maintaining required core functionality.
15-19: LGTM! Clean PhoneNumber interface.Good use of Omit to avoid prop conflicts while extending CountrySelectProps.
21-21: LGTM! Comprehensive field props type.Smart composition of TextFieldProps with PhoneNumberProps.
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx (1)
4-24: LGTM! Well-implemented Select styling.The Select component styling:
- Handles focus states appropriately
- Implements responsive design using breakpoints
- Uses theme spacing consistently
packages/components/modules/profiles/web/index.ts (1)
19-21: LGTM! Export statements follow consistent pattern.The new exports for
ProfileSettingsComponentmaintain the established pattern of exporting both the component and its types.packages/design-system/components/web/icons/index.ts (1)
31-31: LGTM!The UsernameIcon export follows the established pattern and is correctly placed alphabetically.
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
188-196:❓ Verification inconclusive
Verify username validation rules.
The username field implementation should enforce the business rules mentioned in the PR objectives:
- No spaces allowed
- Minimum length of 8 characters (with random digits added if shorter)
- No special characters allowed
🏁 Script executed:
#!/bin/bash # Description: Check the validation schema for username field. # Search for validation schema ast-grep --pattern $'const DEFAULT_PROFILE_FORM_VALIDATION = z.object({ $$$ urlPath: $$$, $$$ })'Length of output: 104
Action Required: Manual Verification of Username Validation Rules
The initial automated search for the
DEFAULT_PROFILE_FORM_VALIDATIONschema did not return any results. Please manually verify that the username field (referenced asurlPath) enforces the business rules defined in the PR objectives:
- No spaces allowed
- Minimum length of 8 characters (with random digits appended if shorter)
- No special characters allowed
Double-check wherever the validation for
urlPathis implemented—whether within a validation schema, inline in the component, or elsewhere—to ensure it aligns with these requirements.packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx (1)
294-298:❓ Verification inconclusive
Verify username validation consistency.
Ensure that the username validation rules are consistent between web and native implementations:
- No spaces allowed
- Minimum length of 8 characters
- No special characters allowed
🏁 Script executed:
#!/bin/bash # Description: Compare validation schemas between web and native. # Search for validation schema in native echo "Native validation:" ast-grep --pattern $'const DEFAULT_PROFILE_FORM_VALIDATION = z.object({ $$$ urlPath: $$$, $$$ })' # Search for validation schema in web echo "Web validation:" fd --type f "constants.ts" --exec rg -A 10 "DEFAULT_PROFILE_FORM_VALIDATION"Length of output: 832
Action Required: Confirm Username Validation Consistency in Native Implementation
The web validation for the username (
urlPath) enforces:
- A minimum length of 8 characters
- Input matching an alphanumeric regex (
/^[a-zA-Z0-9]+$/), which disallows spaces and special charactersOur initial automated search did not reveal a corresponding native validation schema for
urlPathin the file. Please manually verify that the native implementation in:
- File:
packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(around lines 294-298)properly enforces these same rules. If the native validation is missing or inconsistent, update it to match the web configuration.
packages/design-system/CHANGELOG.md (1)
3-8: LGTM!The changelog entry follows the correct format and clearly documents the component moves.
packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts (1)
11-14:❓ Verification inconclusive
Verify username field inclusion in profile data.
Given that this PR implements username generation and editing functionality, consider including the
usernamefield in the node selection to support username uniqueness checks and display.
🏁 Script executed:
#!/bin/bash # Check if ProfileItemFragment includes username field ast-grep --pattern 'fragment ProfileItemFragment on Profile { $$$ username $$$ }'Length of output: 89
Action Required: Verify
usernameField in ProfileItemFragmentThe automated search did not yield any indication of a
usernamefield within the ProfileItemFragment. Since this PR implements username generation and editing functionality, it is important to ensure that the ProfileItemFragment includes theusernamefield to support uniqueness checks and display requirements. Please verify manually that the fragment definition indeed contains theusernamefield, or update it accordingly.packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts (1)
18-20:❓ Verification inconclusive
Verify username search and display capabilities.
Since this PR implements username functionality:
- Ensure
MemberItemFragmentincludes theusernamefield for display- Verify that the search parameter
qsupports username searches
🏁 Script executed:
#!/bin/bash # Check if MemberItemFragment includes username field ast-grep --pattern 'fragment MemberItemFragment on ProfileUserRole { $$$ username $$$ }' # Check if search parameter supports username rg -A 5 "username.*\$q" --type tsLength of output: 131
Action Required: Verify Username Search and Display Functionality
The automated script did not produce any output. Please manually verify the following:
- MemberItemFragment: Confirm that its GraphQL fragment definition correctly includes the
usernamefield.- Search Functionality: Ensure that the search parameter (
q) properly supports filtering by username in the relevant TypeScript files.packages/components/modules/profiles/common/index.ts (1)
3-10:❓ Verification inconclusive
Verify exports for username-related functionality.
The reorganization of exports looks good. However, given that this PR implements username generation and editing:
- Ensure any new username-related mutations are exported
- Verify that username validation utilities are exported through
./utilsor./zod
🏁 Script executed:
#!/bin/bash # Check for username-related mutations rg -l "mutation.*username" --type ts packages/components/modules/profiles/common/graphql/mutations/ # Check for username validation utilities rg -l "username.*validation|validateUsername" --type ts packages/components/modules/profiles/common/{utils,zod}/Length of output: 439
I'll need to gather a bit more evidence before finalizing the rewritten comment. The initial commands yielded no results for username‐related mutations and produced errors when checking for username validation utilities in the expected directories. It seems the directories for validations might have changed (or moved) and the mutations may not exist in the expected location.
Let's run an updated search across the entire common folder to verify both aspects:
Once these commands run, please review the output to confirm whether:
- New mutations involving username functionality exist.
- Any username validation helpers are implemented (and where they reside).
After you verify these outputs, we can finalize the review comment.
🏁 Script executed:
#!/bin/bash # Check for username-related mutations anywhere under the common folder rg -l "mutation.*username" packages/components/modules/profiles/common/ # Check for username validation utilities anywhere under the common folder rg -l "(validateUsername|usernameValidation)" packages/components/modules/profiles/common/Length of output: 166
Verify username-related functionality exports
The reorganization appears sound overall; however, our automated searches did not locate any username-related mutations or username validation utilities within the common folder. This may mean:
- The new username mutations and validation utilities are implemented elsewhere or named differently.
- The intended exports for username functionality might be missing or not yet integrated.
Please manually verify that:
- All new mutations handling username generation/editing are correctly exported.
- Username validation tools are either included in the expected directories (
./utilsor./zod) or are available elsewhere and properly referenced.
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
from
dc44668 to
805d390
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
82-82: Optimize performance by avoiding the delete operator.Replace
deleteoperations with undefined assignments for better performance.- delete dirtyValues.image + dirtyValues.image = undefined - delete dirtyValues.bannerImage + dirtyValues.bannerImage = undefinedAlso applies to: 86-86
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
17-34: Add ARIA labels for accessibility.The select component should have proper ARIA labels to improve accessibility for screen readers.
<StyledSelect variant="outlined" + aria-label="Select country" MenuProps={{ style: { height: '300px', width: '360px', top: '10px', left: '-34px', }, transformOrigin: { vertical: 'top', horizontal: 'left', }, }} {...selectProps} value={country.iso2} onChange={(event) => setCountry(event.target.value as CountryIso2)} renderValue={(value) => <FlagImage iso2={value as CountryIso2} />} >packages/design-system/components/web/inputs/PhoneNumberField/index.tsx (1)
44-66: Add validation feedback for phone numbers.The component should provide visual feedback for invalid phone numbers and expose validation state to parent components.
<PureTextField value={inputValue} onChange={handlePhoneValueChange} type="tel" inputRef={inputRef} + error={!phone.isValid} + helperText={!phone.isValid ? "Please enter a valid phone number" : ""} InputProps={{ startAdornment: ( <InputAdornment position="start"> <CountrySelect setCountry={setCountry} country={country} selectProps={selectProps} countryNameProps={countryNameProps} countryDialCodeProps={countryDialCodeProps} optionProps={optionProps} /> </InputAdornment> ), }} {...props} />packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (2)
9-10: Improve documentation clarity and grammar.Add "the" before "user experience" and include validation state documentation.
- **Purpose**: The `PhoneNumberField` component provides a text field that supports phone number formatting and validation. It enhances user experience by allowing users to select their country and enter a phone number in the correct format. + **Purpose**: The `PhoneNumberField` component provides a text field that supports phone number formatting and validation. It enhances the user experience by allowing users to select their country and enter a phone number in the correct format. - **Expected Behavior**: The component should display a text field with a country select dropdown. The phone number input should be formatted according to the selected country. It should handle various states such as loading, loaded, and error. + **Expected Behavior**: The component should display a text field with a country select dropdown. The phone number input should be formatted according to the selected country. It should handle various states such as loading, loaded, error, and validation, providing visual feedback for invalid phone numbers.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ... formatting and validation. It enhances user experience by allowing users to select ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
27-32: Add accessibility documentation.Include information about ARIA labels and keyboard navigation.
## Notes - **Related Components**: - `PureTextField`: Used to create the base input field with additional functionality. - `CountrySelect`: Used to display the country select dropdown. + +## Accessibility + +- The component includes ARIA labels for screen readers +- Supports keyboard navigation for country selection +- Provides validation feedback through both visual and screen reader announcements🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. -CountrySelect: Used to display the country select drop...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/profiles/common/constants.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts(0 hunks)packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts(0 hunks)packages/components/modules/profiles/common/index.ts(1 hunks)packages/components/modules/profiles/common/types.ts(1 hunks)packages/components/modules/profiles/common/utils.ts(2 hunks)packages/components/modules/profiles/common/zod.ts(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/index.ts(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/web/icons/UsernameIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/constants.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/types.ts(1 hunks)packages/design-system/components/web/inputs/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts
- packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/profiles/web/index.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts
- packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/constants.ts
- packages/components/modules/profiles/common/types.ts
- packages/design-system/CHANGELOG.md
- packages/design-system/package.json
- packages/components/modules/profiles/common/zod.ts
- packages/design-system/components/web/inputs/PhoneNumberField/storybook/stories.tsx
- packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
- packages/wagtail/CHANGELOG.md
- packages/design-system/components/web/inputs/index.ts
- packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx
- packages/components/modules/profiles/common/constants.ts
- packages/components/package.json
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts
- packages/components/CHANGELOG.md
- packages/design-system/components/web/icons/UsernameIcon/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/types.ts
- packages/wagtail/package.json
🧰 Additional context used
🪛 LanguageTool
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ... formatting and validation. It enhances user experience by allowing users to select ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. - CountrySelect: Used to display the country select drop...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome (1.9.4)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (4)
packages/components/modules/profiles/common/index.ts (1)
3-10: LGTM! Well-organized GraphQL exports.The reorganization of exports to use fragments instead of queries aligns with GraphQL best practices, promoting code reuse and maintainability.
packages/components/modules/profiles/common/utils.ts (1)
9-23:⚠️ Potential issueImplement username generation logic.
While the function handles slash removal, it's missing the core username generation requirements:
- Generate username from user's name
- Handle duplicate usernames by appending numbers
- Add random digits for names shorter than 8 characters
Would you like me to help implement the username generation logic that meets these requirements?
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
36-36: Replaceanytype with proper typing.Using
anytype reduces type safety. Consider using the proper type fromreact-international-phonelibrary.packages/design-system/components/web/inputs/PhoneNumberField/index.tsx (1)
39-41: Improve type safety in onChange handler.The
anytype in the onChange callback should be replaced with proper typing from the library.
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
from
805d390 to
913e4e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
188-199: 🛠️ Refactor suggestionEnhance username field with real-time validation.
The username field needs additional features to meet PR objectives:
- Real-time validation feedback
- Alternative username suggestions when taken
- Minimum length indicator (8 characters)
- Special character validation feedback
Would you like me to help implement these enhancements?
76-104:⚠️ Potential issueAdd username-specific handling in form submission.
The onSubmit function needs to:
- Check for duplicate usernames
- Generate alternative usernames when duplicates are found
- Validate minimum length and add random digits if needed
Additionally, consider replacing the
deleteoperator with undefined assignments for better performance:- delete dirtyValues.image + dirtyValues.image = undefined - delete dirtyValues.bannerImage + dirtyValues.bannerImage = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🧹 Nitpick comments (2)
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (2)
19-25: Add missing props to documentation.The props section is missing important form field props that users might need:
error(boolean): Whether the field has an errorhelperText(string): Helper text to display below the fieldrequired(boolean): Whether the field is requireddisabled(boolean): Whether the field is disabled
35-42: Improve example usage.The example has incorrect import path and doesn't demonstrate error handling.
-import PhoneNumberField from '../PhoneNumberField' +import { PhoneNumberField } from '@baseapp-frontend/design-system' const MyComponent = () => ( <PhoneNumberField value="" - onChange={(phone) => console.log(phone)} + onChange={(phone) => { + console.log(phone) + // Validate phone number format + const isValid = /^\+\d{1,3}\s\d{1,14}$/.test(phone) + setError(!isValid) + setHelperText(isValid ? '' : 'Invalid phone number format') + }} defaultCountry="US" + error={error} + helperText={helperText} + required /> )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx(1 hunks)packages/components/modules/profiles/common/constants.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts(0 hunks)packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts(0 hunks)packages/components/modules/profiles/common/index.ts(1 hunks)packages/components/modules/profiles/common/types.ts(1 hunks)packages/components/modules/profiles/common/utils.ts(2 hunks)packages/components/modules/profiles/common/zod.ts(1 hunks)packages/components/modules/profiles/native/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/index.ts(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/web/icons/UsernameIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/constants.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/types.ts(1 hunks)packages/design-system/components/web/inputs/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts
- packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/design-system/components/web/inputs/PhoneNumberField/constants.ts
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/profiles/common/zod.ts
- packages/wagtail/package.json
- packages/design-system/components/web/icons/UsernameIcon/index.tsx
- packages/components/modules/profiles/common/types.ts
- packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
- packages/design-system/package.json
- packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/storybook/stories.tsx
- packages/components/modules/profiles/common/constants.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx
- packages/wagtail/CHANGELOG.md
- packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts
- packages/design-system/components/web/inputs/PhoneNumberField/types.ts
- packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts
- packages/components/package.json
- packages/design-system/components/web/inputs/PhoneNumberField/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/components/CHANGELOG.md
- packages/components/modules/profiles/web/index.ts
- packages/design-system/components/web/inputs/index.ts
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/common/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. - CountrySelect: Used to display the country select drop...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (3)
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
36-36: Replaceanytype with proper typing.Using
anytype reduces type safety. Consider using the proper type fromreact-international-phonelibrary.- {defaultCountries.map((c: any) => { + {defaultCountries.map((c: CountryIso2) => {packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx (1)
34-34: LGTM!The implementation correctly handles optional chaining and is consistent with the slash removal pattern across other components.
packages/components/modules/profiles/native/ProfileComponent/index.tsx (1)
36-36:❓ Verification inconclusive
Verify username validation requirements.
The implementation correctly handles optional chaining and is consistent with other components. However, based on the PR objectives, we need to ensure that the username validation requirements are implemented:
- Minimum length of 8 characters
- No special characters allowed
- Handling of duplicate usernames
Run this script to check for username validation implementation:
🏁 Script executed:
#!/bin/bash # Description: Search for username validation logic in the codebase. # Check for validation schema or rules rg -A 5 "username.*validation|username.*schema|urlPath.*validation|urlPath.*schema" # Check for duplicate username handling rg -A 5 "username.*exists|username.*taken|urlPath.*exists|urlPath.*taken" # Check for length and special character validation rg -A 5 "username.*length|username.*special|urlPath.*length|urlPath.*special"Length of output: 241
Action Required: Verify and Implement Username Validation Requirements
While the component correctly uses optional chaining and maintains consistency with similar components, the current code does not reveal any implementation for the username validation requirements outlined in the PR objectives. Specifically, please confirm or implement the following checks:
- Minimum Length: Username must be at least 8 characters.
- Allowed Characters: Special characters should be disallowed.
- Duplicate Usernames: Proper handling to prevent duplicate usernames is needed.
Since the automated search for validation logic did not yield any results, I recommend a manual verification of the entire codebase to ensure these requirements are addressed, either in this component or a shared validation module.
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx
Outdated
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
from
913e4e8 to
8d6c45b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (3)
7-7:⚠️ Potential issueFix typo in styled component import.
The styled component import has a typo:
ContryTitleshould beCountryTitle.-import { ContryTitle, Select as StyledSelect } from './styled' +import { CountryTitle, Select as StyledSelect } from './styled'
36-36: 🛠️ Refactor suggestionReplace
anytype with proper typing.Using
anytype reduces type safety. Consider using the proper type fromreact-international-phonelibrary.- {defaultCountries.map((c: any) => { + {defaultCountries.map((c: CountryIso2) => {
41-41:⚠️ Potential issueFix typo in styled component name.
The styled component name has a typo:
ContryTitleshould beCountryTitle.- <ContryTitle {...countryNameProps}>{defaultCountry.name}</ContryTitle> + <CountryTitle {...countryNameProps}>{defaultCountry.name}</CountryTitle>packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
76-104:⚠️ Potential issueUsername validation and generation logic is missing from the onSubmit function.
Based on the PR objectives, the system should generate usernames based on user name, handle duplicates, ensure minimum length, and validate against special characters. However, the current implementation doesn't include this logic.
Add username validation and generation logic to the
onSubmitfunction to:
- Check for duplicate usernames
- Generate alternative usernames when duplicates are found
- Validate minimum length and add random digits if needed
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
188-199: 🛠️ Refactor suggestionAdd real-time username validation feedback.
The username field needs to provide immediate feedback to users about validation issues including availability, length requirements, and character restrictions.
Enhance the TextField component with:
- Real-time validation to show if a username is already taken
- Suggestions for alternative usernames when duplicates are found
- Visual indicators for minimum length requirements
- Feedback about invalid characters
Consider adding debounced validation with a backend check for username availability.
🧹 Nitpick comments (4)
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
17-30: Consider extracting MenuProps to a constantThe MenuProps configuration is quite detailed and might be reused elsewhere. Consider extracting it to a separate constant or styled component to improve maintainability.
+const COUNTRY_SELECT_MENU_PROPS = { + style: { + height: '300px', + width: '360px', + top: '10px', + left: '-34px', + }, + transformOrigin: { + vertical: 'top', + horizontal: 'left', + }, +}; <StyledSelect variant="outlined" - MenuProps={{ - style: { - height: '300px', - width: '360px', - top: '10px', - left: '-34px', - }, - transformOrigin: { - vertical: 'top', - horizontal: 'left', - }, - }} + MenuProps={COUNTRY_SELECT_MENU_PROPS}packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (2)
31-31: Fix loose punctuation markThere's an extra space in the documentation that should be removed for consistency.
- - `PureTextField`: Used to create the base input field with additional functionality. - `CountrySelect`: Used to display the country select dropdown. + - `PureTextField`: Used to create the base input field with additional functionality. + - `CountrySelect`: Used to display the country select dropdown.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. -CountrySelect: Used to display the country select drop...(UNLIKELY_OPENING_PUNCTUATION)
35-42: Update example import to match actual implementationThe import statement in the example doesn't match how the component will be exported and used based on the changes in
inputs/index.ts.-import PhoneNumberField from '../PhoneNumberField' +import { PhoneNumberField } from '@baseapp-frontend/design-system'packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
82-82: Replace delete operator with undefined assignment for better performance.The
deleteoperator can negatively impact JavaScript performance as it causes objects to be deoptimized by the JavaScript engine.- delete dirtyValues.image; + dirtyValues.image = undefined; - delete dirtyValues.bannerImage; + dirtyValues.bannerImage = undefined;Also applies to: 86-86
🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx(1 hunks)packages/components/modules/profiles/common/constants.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts(0 hunks)packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts(0 hunks)packages/components/modules/profiles/common/index.ts(1 hunks)packages/components/modules/profiles/common/types.ts(1 hunks)packages/components/modules/profiles/common/utils.ts(2 hunks)packages/components/modules/profiles/common/zod.ts(1 hunks)packages/components/modules/profiles/native/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/index.ts(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/web/icons/UsernameIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/constants.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/types.ts(1 hunks)packages/design-system/components/web/inputs/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts
- packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- packages/design-system/components/web/icons/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/constants.ts
- packages/components/modules/profiles/native/ProfileComponent/index.tsx
- packages/components/modules/profiles/common/zod.ts
- packages/wagtail/CHANGELOG.md
- packages/components/package.json
- packages/design-system/package.json
- packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx
- packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts
- packages/wagtail/package.json
- packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx
- packages/components/CHANGELOG.md
- packages/components/modules/profiles/common/types.ts
- packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/storybook/stories.tsx
- packages/components/modules/profiles/common/constants.ts
- packages/design-system/components/web/icons/UsernameIcon/index.tsx
- packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts
- packages/design-system/components/web/inputs/PhoneNumberField/types.ts
- packages/components/modules/profiles/web/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/index.tsx
- packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
- packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/common/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. - CountrySelect: Used to display the country select drop...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (7)
packages/components/modules/profiles/common/utils.ts (4)
9-12: Enhanced function signature with more explicit namingThe function renaming from
getDefaultValuestogetProfileDefaultValuesimproves clarity and specificity. The new parameter structure using object destructuring is more maintainable and allows for better extension in the future.
21-23: Good implementation for slash handling in usernamesThe logic to remove slashes from the username (urlPath) aligns with the PR objectives of parsing slashes on usernames to simplify user input.
6-7: Centralize Shared Utilities for Phone Number and Image URLThe TODO comments indicate shared functionality that should be moved to common locations.
Per previous reviews, consider moving
DEFAULT_PHONE_NUMBER_COUNTRY_CODEandgetImageUrlto a common shared module to improve maintainability and avoid potential inconsistencies between duplicated definitions.Also applies to: 32-33
4-30: Implement username validation and generation logicThe code handles formatting the profile data and removing slashes, but it does not address the automatic username generation functionality mentioned in the PR objectives.
Based on the PR objectives, the function should include logic for:
- Generating usernames from user's name
- Ensuring minimum length of 8 characters
- Appending random digits for uniqueness
- Validating against special characters
Would you like me to help implement this functionality?
packages/design-system/components/web/inputs/index.ts (1)
3-4: Good component export structureThe exports for
PhoneNumberFieldand its types follow the established pattern in the codebase, making the components accessible to other parts of the application. The addition ofTextareaFieldtypes exports also improves consistency.Also applies to: 10-10
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (1)
5-42: Well-structured component documentationThe documentation is comprehensive, covering purpose, behavior, use cases, props, and example usage. This makes it easy for other developers to understand and use the component correctly.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. -CountrySelect: Used to display the country select drop...(UNLIKELY_OPENING_PUNCTUATION)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
47-47:❓ Verification inconclusive
Confirm the removeSlashInUsername logic is properly implemented.
The form uses
getProfileDefaultValueswithremoveSlashInUsername: true, which seems to address the requirement to parse slashes in usernames, but we should verify the implementation.
🏁 Script executed:
#!/bin/bash # Check the implementation of getProfileDefaultValues rg -A 15 "getProfileDefaultValues.*removeSlashInUsername" packages/components/modules/profiles/common/Length of output: 104
Action Required: Verify the
removeSlashInUsernameLogic ImplementationOur initial automated search in the
packages/components/modules/profiles/common/directory for evidence of handling theremoveSlashInUsernameflag did not return any results. This absence means we haven't yet confirmed that the logic to parse slashes from usernames is implemented withingetProfileDefaultValues.Next Steps:
- Manual Verification: Please manually review the definition of
getProfileDefaultValues(likely in the common profile utilities) to ensure that theremoveSlashInUsernameflag is used to modify the username as intended.- Search the Entire Codebase: Consider searching the entire repository for occurrences of the string
removeSlashInUsernamein case the implementation resides outside the initially expected directory.Once you confirm that the flag is properly handled, or update the logic if necessary, we can mark this improvement as complete.
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx (1)
9-9: Consider updating the component title pathThe title path is quite lengthy which can create deep nesting in the Storybook navigation. Since this component appears to be moved from
baseapp-frontend-templateto@baseapp-frontend/componentsaccording to the summary, consider updating the title to reflect the new package location.- title: '@baseapp-frontend-template / Pages/User/Settings/AccountSettings/AccountProfile', + title: '@baseapp-frontend/components / Profiles / ProfileSettings',packages/components/.storybook/decorators/withMutationResolver.tsx (2)
21-62: Well-implemented mutation resolver with minor improvements possibleThe useEffect hook correctly overrides and restores the environment's executeMutation method, handling both static and dynamic mock resolvers appropriately.
Consider these minor improvements:
- Extract timeout values to constants for better maintainability
- Add error handling for the original observable subscription
- Remove unused
queueOperationResolverfrom the dependency array+ const MUTATION_TIMEOUT = 100; // ms useEffect(() => { const originalExecuteMutation = environment.executeMutation environment.executeMutation = (request) => { // ...existing code... return Observable.create((sink) => { if (mutationError) { setTimeout(() => { sink.error(new Error(mutationError)) - }, 100) + }, MUTATION_TIMEOUT) } else { observable.subscribe({ complete: () => { setTimeout(() => { sink.complete() - }, 100) + }, MUTATION_TIMEOUT) }, + error: (error) => { + sink.error(error) + } }) } }) } return () => { environment.executeMutation = originalExecuteMutation } - }, [environment, queueOperationResolver]) + }, [environment])
42-56: Handle username validation related to PR objectivesThe implementation of the mock mutation resolver is generic, but consider adding specific handling for username validation errors to match the PR objectives which mention handling username validation and providing alternative suggestions when a username is already taken.
You could enhance this decorator to handle specific username validation scenarios as defined in the PR objectives:
- Detecting duplicate usernames
- Suggesting alternative usernames
- Validating username rules (no spaces, min length, no special characters)
This would make the Storybook stories more aligned with the actual business requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/components/.storybook/decorators/index.ts(1 hunks)packages/components/.storybook/decorators/withMutationResolver.tsx(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/components/schema.graphql(0 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/schema.graphql
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
🔇 Additional comments (10)
packages/components/.storybook/decorators/index.ts (1)
3-3: Clean addition of withMutationResolver exportThis addition exports the withMutationResolver decorator, which will be useful for mocking GraphQL mutations in Storybook stories, particularly for testing username-related operations.
packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts (1)
1-9: Well-structured Relay test queryThe query is properly defined with the
@relay_test_operationdirective and includes the necessary fragment for profile component testing.packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/index.tsx (1)
1-14: Well-implemented query wrapper componentThis component follows the standard pattern for using a component with GraphQL data in Storybook, properly utilizing
useLazyLoadQueryand passing the fetched profile data to the ProfileSettingsComponent.packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts (2)
11-13: Verify URL path format for usernames with slashesThe PR mentions parsing slashes in usernames for profile pages, but the mock uses a standard path format. Ensure this matches the expected format for profile URLs with username inclusion.
25-30: Good error case handlingThe mockResolversWithMutationError correctly extends the base resolvers while overriding just the mutation behavior for error testing.
packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx (3)
8-29: Well-structured Storybook configurationThe Storybook configuration is clean and follows best practices with appropriate decorators, args, and argTypes defined. The profile prop is correctly documented with a clear description of its purpose.
35-39: LGTM for Default story implementationThe Default story is correctly implemented with appropriate name and mock resolvers.
41-45: LGTM for Error story implementationGood implementation of an error state story that uses the specialized mock resolvers with mutation errors. This helps in testing error handling within the component.
packages/components/.storybook/decorators/withMutationResolver.tsx (2)
10-10: Well-defined type for dynamic mock resolversThe type definition for
DynamicMockResolversis clear and properly typed to accept an operation descriptor and return mock resolvers.
12-20: Good parameter extraction from Storybook contextThe code correctly extracts the necessary parameters from the Storybook context, including environment, mock resolvers, and mutation error flags.
packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
from
575a353 to
df7f8ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
25-36: Consider extracting menu styles to a constant or theme configuration.The hardcoded values for MenuProps styling could make the component less maintainable and responsive. Consider extracting these values to constants or theme variables to improve adaptability to different screen sizes and contexts.
+const MENU_STYLES = { + style: { + height: '300px', + width: '360px', + top: '10px', + left: '-34px', + }, + transformOrigin: { + vertical: 'top', + horizontal: 'left', + }, +}; export const CountrySelect: FC<CountrySelectProps> = ({ country, setCountry, selectProps, countryNameProps, countryDialCodeProps, optionProps, }) => ( <StyledSelect variant="outlined" - MenuProps={{ - style: { - height: '300px', - width: '360px', - top: '10px', - left: '-34px', - }, - transformOrigin: { - vertical: 'top', - horizontal: 'left', - }, - }} + MenuProps={MENU_STYLES} {...selectProps}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/components/.storybook/decorators/index.ts(1 hunks)packages/components/.storybook/decorators/withMutationResolver.tsx(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts
- packages/components/.storybook/decorators/index.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/components/.storybook/decorators/withMutationResolver.tsx
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/mockResolver.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/stories.tsx
- packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (1)
packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx (1)
15-53: LGTM! The component is well-structured with appropriate TypeScript types.The CountrySelect component is cleanly implemented with proper type annotations, clear props interface, and efficient rendering of country options. Good job using the react-international-phone library's types and components.
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
2 times, most recently
from
f574067 to
91a36bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (1)
35-42: Optional: Demonstrate error handling in examples.
While the usage snippet clearly shows how to set up a basic phone number field, consider adding an example that demonstrates how to handle phone number formatting errors or validation messages for an even more complete reference.packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
118-118: Prefer explicit types overany.
Replacingtype: anywith a more descriptive type (e.g., a string literal union or a dedicated enum) will improve type safety and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
packages/components/.storybook/decorators/index.ts(1 hunks)packages/components/.storybook/decorators/withMutationResolver.tsx(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx(1 hunks)packages/components/modules/profiles/common/constants.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts(0 hunks)packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts(0 hunks)packages/components/modules/profiles/common/index.ts(1 hunks)packages/components/modules/profiles/common/types.ts(1 hunks)packages/components/modules/profiles/common/utils.ts(2 hunks)packages/components/modules/profiles/common/zod.ts(1 hunks)packages/components/modules/profiles/native/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/index.ts(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/web/icons/UsernameIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/constants.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/types.ts(1 hunks)packages/design-system/components/web/inputs/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts
- packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- packages/components/.storybook/decorators/index.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts
- packages/components/modules/profiles/common/zod.ts
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/wagtail/package.json
- packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts
- packages/components/modules/profiles/common/types.ts
- packages/components/package.json
- packages/design-system/components/web/inputs/PhoneNumberField/constants.ts
- packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx
- packages/components/modules/profiles/native/ProfileComponent/index.tsx
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/mockResolver.ts
- packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
- packages/design-system/components/web/icons/UsernameIcon/index.tsx
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx
- packages/components/CHANGELOG.md
- packages/components/.storybook/decorators/withMutationResolver.tsx
- packages/design-system/components/web/inputs/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/components/modules/profiles/web/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/types.ts
- packages/components/modules/profiles/common/constants.ts
- packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts
- packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts
- packages/design-system/components/web/inputs/PhoneNumberField/storybook/stories.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
- packages/wagtail/CHANGELOG.md
- packages/components/modules/profiles/common/index.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/index.tsx
- packages/design-system/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. - CountrySelect: Used to display the country select drop...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit Test Packages
- GitHub Check: Build and Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx (1)
29-29: Updated urlPath rendering to remove slashes.The change adds optional chaining and removes slashes from the urlPath, which aligns with the PR's requirement to parse slashes in usernames and profile pages.
packages/components/modules/profiles/common/utils.ts (4)
4-4: Added new type import for function parameters.Good practice importing the structured type definition for the function parameters.
9-12: Improved function signature for profile default values.Renaming to
getProfileDefaultValuesand adding structured parameters improves clarity and flexibility.
6-7: Address TODO comments for shared utilities.These TODO comments indicate functionality that should be moved to shared locations. Moving these constants to a common location would improve maintainability.
21-23:❓ Verification inconclusive
Added conditional slash removal from urlPath.
This change aligns with the PR objective to parse slashes in usernames. However, the full username generation requirements (min 8 chars, uniqueness check, special character validation) don't appear to be implemented here.
Is username generation and validation implemented elsewhere? The PR objectives mention automatically generating usernames, ensuring uniqueness, and adding random digits when necessary.
🏁 Script executed:
#!/bin/bash # Search for username generation/validation logic rg -t ts -t tsx "username|urlPath" --context=5 | grep -E "generate|valid|unique"Length of output: 110
Below is a revised review comment. Note that the initial search command failed due to an unrecognized tsx file type, so I’ve provided an updated script to re-run the search. Please run the following command to help manually verify if username generation/validation logic (e.g., enforcing an 8‑character minimum, uniqueness checks, or special character validation) is implemented elsewhere in the codebase:
Rewritten Review Comment:
The updated conditional logic to remove a slash from
urlPathmeets part of the PR objective, but the full username generation requirements (minimum 8 characters, uniqueness check, special character validation with random digit supplementation) are not evident in this file. Please confirm if these requirements are addressed in another module or service. If not, we need to implement the missing validations as per the objectives.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for username generation and validation logic in TypeScript/TSX files. rg -g "*.ts" -g "*.tsx" -e "generateUsername" -e "validateUsername" -e "username generation" -e "unique" --context=5Length of output: 234
Username Generation/Validation Verification
The conditional logic for removing a slash from
urlPathhas been added correctly. However, based on our searches, it remains unclear where the full username generation and validation logic (enforcing an 8-character minimum, ensuring uniqueness, and appending random digits when needed) is implemented. Please verify manually if these functionalities reside in another module/service or if further implementation is required.packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx (2)
8-29: Well-structured Storybook configuration.The Storybook setup follows best practices with clear component metadata, proper decorators, and well-defined props documentation.
35-45: Comprehensive story coverage with error handling.Excellent approach creating both default and error states with appropriate mock resolvers. This ensures the component is tested in various scenarios.
packages/design-system/components/web/inputs/PhoneNumberField/index.tsx (3)
15-24: Clear documentation with BaseApp guidelines.The JSDoc comment provides excellent context about the component's purpose and modification guidelines.
39-41: Improve type safety in onChange handler.The
anytype in the onChange callback should be replaced with proper typing from the library.- onChange: (data: any) => { + onChange: (data: { phone: string; country: CountryIso2 }) => { onChange?.(data.phone) },
25-67: Well-implemented PhoneNumberField component.The component correctly implements international phone number input functionality with country selection. It's designed to be reusable and follows the project's component patterns.
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (2)
1-3: Well-structured Storybook metadata.
TheMetatag is configured correctly to help Storybook organize this documentation at the specified location.
9-10: Comprehensive documentation overview.
The explanation of the component's purpose, use cases, and props is clear and concise, making it easy for consumers to quickly understand and integrate thePhoneNumberFieldinto various contexts.Also applies to: 14-16, 19-26
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
2 times, most recently
from
35d2337 to
6635ae7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
76-105:⚠️ Potential issueMissing logic for generating unique usernames and appending digits.
The PR objectives specify automatically generating a username, ensuring uniqueness, and appending digits if the username is too short or duplicated. Currently, theonSubmitfunction doesn’t implement any logic to handle username collisions or automatic username generation.🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
80-87: 🛠️ Refactor suggestionAvoid using the
deleteoperator for performance reasons.
Assignundefinedto properties instead of deleting them. This helps maintain predictable object shapes and can improve performance.if ('image' in dirtyValues && image && typeof image !== 'string') { uploadables.image = image - delete dirtyValues.image + dirtyValues.image = undefined } if ('bannerImage' in dirtyValues && bannerImage && typeof bannerImage !== 'string') { uploadables.bannerImage = bannerImage - delete dirtyValues.bannerImage + dirtyValues.bannerImage = undefined }🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🧹 Nitpick comments (8)
packages/components/.storybook/decorators/withMutationResolver.tsx (3)
43-56: Improve error handling in Observable subscriptionThe current implementation only handles the 'complete' event when subscribing to the observable, but doesn't handle potential errors from the original mutation.
observable.subscribe({ complete: () => { setTimeout(() => { sink.complete() }, 100) }, + error: (error) => { + setTimeout(() => { + sink.error(error) + }, 100) + }, + next: (value) => { + sink.next(value) + } })
44-52: Potential memory leak with timeoutsThe timeouts created for handling mutation completion and errors aren't being cleared if the component unmounts during their execution period.
if (mutationError) { - setTimeout(() => { + const errorTimeout = setTimeout(() => { sink.error(new Error(mutationError)) }, 100) + return () => clearTimeout(errorTimeout) } else { observable.subscribe({ complete: () => { - setTimeout(() => { + const completeTimeout = setTimeout(() => { sink.complete() }, 100) + return () => clearTimeout(completeTimeout) },
45-46: Consider making timeouts configurableThe hardcoded timeout values (100ms) could be made configurable through Storybook parameters to accommodate different testing scenarios.
+ const timeout: number = context.parameters.mutationTimeout || 100 // Then use 'timeout' instead of hardcoded 100: setTimeout(() => { sink.error(new Error(mutationError)) - }, 100) + }, timeout) setTimeout(() => { sink.complete() - }, 100) + }, timeout)Also applies to: 50-52
packages/components/modules/profiles/common/utils.ts (2)
9-30: Implement complete username generation logic.The current changes only handle slash removal in usernames, but the PR objectives include more comprehensive username generation requirements:
- Generating usernames from user's name (removing spaces)
- Ensuring minimum length of 8 characters
- Appending random digits for uniqueness
- Validating against special characters
Consider implementing the complete username generation logic according to the acceptance criteria.
Would you like me to help implement the username generation functionality according to the requirements?
6-7: Centralize shared utilities.The TODO comment about moving
DEFAULT_PHONE_NUMBER_COUNTRY_CODEto a shared location still exists. Both this file and the PhoneNumberField component use this constant independently.Consider extracting this constant to a common shared module that both web and native components can import from to avoid duplication and potential inconsistencies.
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (1)
9-9: Enhance documentation with clear article.Minor grammatical improvement:
- The `PhoneNumberField` component provides a text field that supports phone number formatting and validation. It enhances user experience by allowing users to select their country and enter a phone number in the correct format. + The `PhoneNumberField` component provides a text field that supports phone number formatting and validation. It enhances the user experience by allowing users to select their country and enter a phone number in the correct format.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ... formatting and validation. It enhances user experience by allowing users to select ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
1-2: Add unit test coverage for this new component.
To ensure confidence in future changes, please add or expand test cases covering the various scenarios (image upload, banner upload, username changes, etc.).
118-118: Use a more specific type instead ofany.
DeclaringhandleRemoveImage(type: any)may complicate maintenance and readability. Restrict the parameter to the relevant string keys or an enum-like type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
packages/components/.storybook/decorators/index.ts(1 hunks)packages/components/.storybook/decorators/withMutationResolver.tsx(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx(1 hunks)packages/components/modules/profiles/common/constants.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts(0 hunks)packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts(0 hunks)packages/components/modules/profiles/common/index.ts(1 hunks)packages/components/modules/profiles/common/types.ts(1 hunks)packages/components/modules/profiles/common/utils.ts(2 hunks)packages/components/modules/profiles/common/zod.ts(1 hunks)packages/components/modules/profiles/native/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/index.ts(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/web/icons/UsernameIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/constants.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/types.ts(1 hunks)packages/design-system/components/web/inputs/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts
- packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- packages/components/.storybook/decorators/index.ts
- packages/wagtail/package.json
- packages/wagtail/CHANGELOG.md
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/index.tsx
- packages/design-system/components/web/icons/index.ts
- packages/components/modules/profiles/common/types.ts
- packages/design-system/components/web/inputs/PhoneNumberField/constants.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts
- packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx
- packages/components/package.json
- packages/components/modules/profiles/native/ProfileComponent/index.tsx
- packages/design-system/package.json
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/common/zod.ts
- packages/components/CHANGELOG.md
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/stories.tsx
- packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts
- packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/mockResolver.ts
- packages/design-system/components/web/icons/UsernameIcon/index.tsx
- packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx
- packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts
- packages/design-system/components/web/inputs/PhoneNumberField/storybook/stories.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/types.ts
- packages/components/modules/profiles/web/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts
- packages/design-system/components/web/inputs/index.ts
- packages/components/modules/profiles/common/constants.ts
- packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/components/modules/profiles/common/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ... formatting and validation. It enhances user experience by allowing users to select ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. - CountrySelect: Used to display the country select drop...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (10)
packages/components/.storybook/decorators/withMutationResolver.tsx (3)
1-10: Good imports and type definitionThe imports are well organized with proper spacing. The DynamicMockResolvers type is clearly defined for use in the component.
12-19: Well-structured parameter extractionGood job extracting the necessary parameters from the Storybook context. The code is clear and follows best practices by providing default values.
64-67: The component structure follows best practicesThe decorator correctly returns the wrapped Story component and is properly exported as the default export.
packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx (1)
29-29: Added optional chaining for saferurlPathaccess.The addition of optional chaining for both
urlPathand itsreplacemethod is a good safety improvement, preventing potential runtime errors ifurlPathis null or undefined.packages/components/modules/profiles/common/utils.ts (2)
9-12: Renamed and enhanced function signature to support more flexible username handling.Good refactoring to rename the function to the more specific
getProfileDefaultValuesand add parameters including the option to control slash handling.
21-23: Added slash removal for usernames.This implementation addresses the PR objective of parsing slashes in usernames, making it more user-friendly. The default behavior removes slashes while allowing the option to preserve them when needed.
packages/design-system/components/web/inputs/PhoneNumberField/index.tsx (2)
39-41: Improve type safety in onChange handler.The
anytype in the onChange callback should be replaced with proper typing from the library.- onChange: (data: any) => { + onChange: (data: { phone: string; country: CountryIso2 }) => { onChange?.(data.phone) },
1-69: Phone input component looks well structured.This new component provides a good foundation for phone number input with international support, which will be useful for user profile management.
A few suggestions for further improvements:
- Consider adding validation for phone numbers
- Add error states and error messages
- Add tests to ensure proper functioning
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (1)
1-42: Comprehensive component documentation.The documentation provides a clear overview of the component's purpose, behavior, use cases, props, and example usage. This will be valuable for other developers using the component.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: You might be missing the article “the” here.
Context: ... formatting and validation. It enhances user experience by allowing users to select ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. -CountrySelect: Used to display the country select drop...(UNLIKELY_OPENING_PUNCTUATION)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (1)
189-199: Consider real-time or client-side username availability checks.
This matches the PR objectives of ensuring uniqueness. You could proactively notify users of duplicates and suggest alternative usernames before they submit changes.
pt-tsl
force-pushed
the
BA-2267-generate-edit-username
branch
from
6635ae7 to
59111d6
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (1)
29-31: Consider revising punctuation for clarity.A minor punctuation or spacing issue has been flagged by static analysis in the bullet list near line 31. Ensure there's consistent spacing or punctuation to avoid confusion.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. -CountrySelect: Used to display the country select drop...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx(1 hunks)packages/components/modules/profiles/common/constants.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts(1 hunks)packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts(0 hunks)packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts(0 hunks)packages/components/modules/profiles/common/index.ts(1 hunks)packages/components/modules/profiles/common/types.ts(1 hunks)packages/components/modules/profiles/common/utils.ts(2 hunks)packages/components/modules/profiles/common/zod.ts(1 hunks)packages/components/modules/profiles/native/ProfileComponent/index.tsx(1 hunks)packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx(2 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts(1 hunks)packages/components/modules/profiles/web/index.ts(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/design-system/components/web/icons/UsernameIcon/index.tsx(1 hunks)packages/design-system/components/web/icons/index.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/stories.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/constants.ts(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/types.ts(1 hunks)packages/design-system/components/web/inputs/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)packages/components/.storybook/decorators/index.ts(1 hunks)packages/components/.storybook/decorators/withMutationResolver.tsx(1 hunks)packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/index.tsx(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/mockResolver.ts(1 hunks)packages/components/modules/profiles/web/ProfileSettingsComponent/__storybook__/stories.tsx(1 hunks)packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx(1 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx(2 hunks)packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- packages/components/modules/profiles/common/graphql/queries/ProfilesList.ts
- packages/components/modules/profiles/common/graphql/queries/UserMembersList.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- packages/design-system/components/web/icons/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/styled.tsx
- packages/design-system/package.json
- packages/components/CHANGELOG.md
- packages/wagtail/package.json
- packages/components/.storybook/decorators/index.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/types.ts
- packages/components/package.json
- packages/components/modules/profiles/native/ProfileComponent/index.tsx
- packages/components/modules/profiles/common/zod.ts
- packages/wagtail/CHANGELOG.md
- packages/components/modules/profiles/web/ProfileSettingsComponent/styled.tsx
- packages/components/modules/profiles/common/types.ts
- packages/components/modules/profiles/native/ProfileSettingsComponent/index.tsx
- packages/design-system/components/web/icons/UsernameIcon/index.tsx
- packages/components/modules/profiles/web/index.ts
- packages/components/modules/messages/web/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/storybook/stories.tsx
- packages/components/modules/profiles/common/graphql/queries/ProfileSettingsRelayTest.ts
- packages/components/modules/profiles/common/graphql/fragments/ProfilesList.ts
- packages/design-system/components/web/inputs/PhoneNumberField/types.ts
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/mockResolver.ts
- packages/components/modules/profiles/common/constants.ts
- packages/design-system/components/web/inputs/index.ts
- packages/components/modules/profiles/common/graphql/fragments/UserMembersList.ts
- packages/components/.storybook/decorators/withMutationResolver.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
- packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/index.tsx
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/stories.tsx
- packages/design-system/CHANGELOG.md
- packages/components/modules/profiles/web/ProfileSettingsComponent/storybook/index.tsx
- packages/components/modules/profiles/common/index.ts
- packages/design-system/components/web/inputs/PhoneNumberField/constants.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx
[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ional functionality. - CountrySelect: Used to display the country select drop...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (14)
packages/components/modules/profiles/web/profile-popover/CurrentProfile/index.tsx (1)
29-29: Excellent addition of optional chaining for safer property access.The addition of optional chaining to both
urlPathand thereplacemethod improves code robustness by preventing potential runtime errors. This change aligns with the PR objective of better handling usernames in the UI and implements the exact fix suggested in a previous code review.packages/design-system/package.json (1)
81-81:❓ Verification inconclusive
New dependency added for phone number input functionality.
The addition of the
react-international-phonepackage aligns with the implementation of the newPhoneNumberFieldcomponent. This library provides internationalized phone number input with formatting and validation capabilities.
🏁 Script executed:
#!/bin/bash # Verify the package is actively maintained and check for any security issues npm view react-international-phone homepage maintainers time.modified # Check for known vulnerabilities npm audit --package react-international-phone@4.5.0Length of output: 631
Manual Security Verification Required
The new dependency for international phone input,
react-international-phone@^4.5.0, appears to be actively maintained (last modified on 2025-01-18) and is correctly aligned with the newPhoneNumberFieldcomponent. However, the automatic vulnerability audit did not complete due to the absence of a lockfile. Please ensure a package lock file is generated (e.g., vianpm i --package-lock-only) and manually re-runnpm audit --package react-international-phone@4.5.0to verify there are no security issues.
- Confirm lockfile creation for a complete audit.
- Re-run the vulnerability check post lockfile generation.
packages/components/modules/profiles/common/utils.ts (4)
4-4: Added import for new type definition.Good addition of the
ProfileGetDefaultFormValuestype to provide proper typing for the updated function parameters.
9-12: Renamed and modified function to support username formatting rules.The function has been renamed from
getDefaultValuestogetProfileDefaultValuesand now accepts an object with profile data and a flag to control slash removal in usernames. This change supports the PR objective for username editing functionality.
21-23: Added logic to handle slash removal in usernames.This implementation aligns with the PR objectives for parsing slashes in usernames. The conditional logic ensures that slashes are only removed when the
removeSlashInUsernameparameter is true.
6-7: Address TODO comments that were previously flagged.There are still unaddressed TODO comments about moving shared functionality to common locations. This was previously flagged in code reviews.
#!/bin/bash # Search for phone number country code usage rg "DEFAULT_PHONE_NUMBER_COUNTRY_CODE" -A 2 # Search for image URL utility usage rg "getImageUrl" -A 2packages/design-system/components/web/inputs/PhoneNumberField/CountrySelect/styled.tsx (2)
4-24: Well-implemented styled Select component.The
Selectcomponent is properly styled to have a clean appearance with appropriate responsive behavior. The CSS selectors are used effectively to control the visibility of the fieldset element based on focus state.
26-30:⚠️ Potential issueFix typo in component name.
There's a typo in the component name:
ContryTitleshould beCountryTitle.-export const ContryTitle = styled((props) => <Typography {...props} />)<TypographyProps>( +export const CountryTitle = styled((props) => <Typography {...props} />)<TypographyProps>( ({ theme }) => ({ margin: theme.spacing(0, 1), }), )Likely an incorrect or invalid review comment.
packages/design-system/components/web/inputs/PhoneNumberField/index.tsx (3)
1-34: Well-structured phone number input component.The component is well-organized with clear JSDoc comments and appropriate prop handling. The component leverages the react-international-phone library effectively to create a phone input with country selection capabilities.
39-41: Improve type safety in onChange handler.The
anytype in the onChange callback should be replaced with proper typing from the library.- onChange: (data: any) => { + onChange: (data: { phone: string; country: CountryIso2 }) => { onChange?.(data.phone) },
44-66: Well-implemented component rendering.The component correctly renders a
PureTextFieldwith appropriate props and the country selector as a start adornment. The implementation follows good practices for customizing input components.packages/design-system/components/web/inputs/PhoneNumberField/__storybook__/PhoneNumberField.mdx (1)
1-3: Use of Storybook "Meta" tag is correct.This MDX file correctly declares the Storybook meta configuration with the
titleproperty. This is in line with best practices for organizing component documentation in Storybook.packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx (2)
46-50: Leverage updated default values logic.Using
getProfileDefaultValues({ profile, removeSlashInUsername: true })correctly aligns with the PR objective of removing slashes from usernames. Confirm that all user-facing paths have the slashes removed where appropriate to avoid accidental conflicts.
80-87: Avoid using thedeleteoperator ondirtyValues.This is a repeat of an existing recommendation from past reviews. Deleting properties can degrade performance. Assigning
undefinedor refactoring the approach is generally preferred.Apply this diff to replace
deletewith a safer pattern:- delete dirtyValues.image + dirtyValues.image = undefined ... - delete dirtyValues.bannerImage + dirtyValues.bannerImage = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/components/modules/profiles/web/ProfileSettingsComponent/index.tsx
Show resolved
Hide resolved
anicioalexandre
added
the
approved
Acceptance Criteria
Context
Even though the username field exists, right now we don't have any way for users to setup their username in BA. And they are important for the different social feature that we have.
Business Rules - Automatic Username Creation
Business Rules - Edit Username
Functional Requirements
Approvd
https://app.approvd.io/silverlogic/BA/stories/39057
Summary by CodeRabbit
New Features
ProfileSettingsComponentfor managing user profile settings.UsernameIconcomponent for consistent branding.CountrySelectcomponent to facilitate country selection in phone number input.Bug Fixes
Documentation