feat: New Profile Screen (read-only) #1519
Conversation
WalkthroughThis pull request introduces comprehensive changes across multiple files, focusing on enhancing user profile management, navigation, internationalization, and design system components. The modifications span several key areas: profile screens, navigation configurations, translation files, design system updates, and notification permission handling. The changes aim to improve user experience by streamlining UI components, adding new features like profile editing, and introducing more flexible styling options. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProfileScreen
participant ContactCard
participant EditMode
participant Navigation
User->>ProfileScreen: Open Profile
ProfileScreen->>ContactCard: Render Contact Info
User->>ContactCard: Tap Edit
ContactCard->>EditMode: Toggle Edit Mode
EditMode->>ProfileScreen: Update Profile State
User->>ProfileScreen: Save Changes
ProfileScreen->>Navigation: Navigate Back/Update
Possibly related PRs
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
screens/UserProfileScreen.tsx (1)
Line range hint
108-117: Fix display name length inconsistency and add username validation.
Display name has conflicting length limits:
trimmedDisplayNameallows 50 charactersmaxLengthprop restricts to 30 charactersUsername field lacks essential validations:
- No minimum length check
- No special character validation
Here's a suggested fix for the display name inconsistency:
- const trimmedDisplayName = text.slice(0, 50); + const trimmedDisplayName = text.slice(0, 30);Consider adding a username validation helper:
const validateUsername = (username: string): boolean => { const MIN_LENGTH = 3; const VALID_PATTERN = /^[a-zA-Z0-9_]+$/; return username.length >= MIN_LENGTH && VALID_PATTERN.test(username); };Also applies to: 134-146
🧹 Nitpick comments (4)
screens/Profile.tsx (2)
832-858: SimplifyhandleChatPressby removing redundantnavigation.popToTop()callsIn the
handleChatPressfunction (lines 832-858),navigation.popToTop();is called in both branches of theif...elsestatement. This is redundant and can be moved before theifstatement to simplify the code.Apply this diff to refactor the function:
const handleChatPress = useCallback(() => { + navigation.popToTop(); const isPreviouslyInNavStack = navigation .getState() .routes.some((route) => { if (route.name !== "Conversation") { return false; } const params = route.params as ConversationNavParams; return params?.peer === peerAddress.toLowerCase(); }); if (isPreviouslyInNavStack) { - navigation.popToTop(); navigation.navigate({ name: "Conversation", params: { peer: peerAddress, }, }); } else { - navigation.popToTop(); navigation.dispatch( StackActions.push("Conversation", { peer: peerAddress, }) ); } }, [navigation, peerAddress]);
104-138: Consolidate styling approach for consistencyThe
useStylesfunction (lines 104-138) defines styles usingStyleSheet.create, while other components, such asContactCard, use inline styles. For better maintainability and consistency, consider adopting a single styling approach throughout the file.features/conversation/conversation.nav.tsx (1)
8-8: Remove unused importPlatformThe
Platformmodule is imported on line 8 but not used in this file. Removing unused imports keeps the code clean and avoids potential confusion.screens/UserProfileScreen.tsx (1)
Line range hint
41-48: Enhance error handling for better user experience.The current error handling could be improved:
- No retry mechanism for failed operations
- No cleanup of error state
- Generic error tracking without specific error context
Consider implementing:
- A retry mechanism
- Error state cleanup
- More detailed error tracking
const handleSave = useCallback(async () => { + // Clear previous error state + setErrorMessage(''); try { const { success } = await createOrUpdateProfile({ profile }); if (success) { navigation.popToTop(); } } catch (error) { - sentryTrackError(error); + sentryTrackError(error, { + context: 'UserProfileScreen.handleSave', + profile: { ...profile, avatar: 'REDACTED' }, + }); + // Set user-friendly error message + setErrorMessage(translate('userProfile.errors.saveFailed')); } }, [createOrUpdateProfile, profile, navigation]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/conversation/conversation.nav.tsx(2 hunks)screens/Navigation/ProfileNav.tsx(2 hunks)screens/Navigation/UserProfileNav.tsx(0 hunks)screens/Profile.tsx(5 hunks)screens/UserProfileScreen.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- screens/Navigation/UserProfileNav.tsx
🔇 Additional comments (4)
screens/Navigation/ProfileNav.tsx (1)
20-22: Verify navigation behavior after simplifyingoptionsThe
optionsobject inProfileNavhas been simplified to a static title on lines 20-22. Ensure that this change does not affect necessary navigation features, such as dynamic titles or header buttons needed for user navigation.screens/UserProfileScreen.tsx (3)
Line range hint
18-20: Well-implemented performance optimizations!Good use of React performance best practices:
- Component memoization
- Proper usage of useCallback
- Efficient ref handling for input focus management
Also applies to: 41-48, 52-62
Line range hint
108-117: Excellent attention to UX details!Great implementation of user-friendly features:
- Proper auto-capitalization settings
- Optimized keyboard behavior
- Internationalized placeholder texts
Also applies to: 134-146
44-44: Verify navigation UX with removed back button.The change from
goBack()topopToTop()combined with the removal of the header back button (mentioned in the summary) might create a suboptimal user experience:
- Users can't go back to review changes
- Users might get stuck if save fails
- Navigation history is completely cleared
Let's verify the navigation setup:
✅ Verification successful
Navigation implementation is properly safeguarded
The
popToTop()navigation only occurs after a successful profile update, with proper error handling in place:
- Failed operations don't trigger navigation
- Users receive visible error feedback
- Errors are tracked via Sentry
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any alternative navigation options or error handling rg -A 5 "UserProfile.*Screen" screens/Navigation/Length of output: 3512
Script:
#!/bin/bash # Check UserProfileScreen for error handling and navigation rg -A 5 "catch|error|navigation\." screens/UserProfileScreen.tsxLength of output: 683
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
screens/Profile.tsx (3)
162-176: Improve shadow implementation for cross-platform compatibility.The current shadow implementation might not work consistently across platforms. Android requires
elevationwhile iOS usesshadowXproperties.Consider using a platform-specific shadow implementation:
const baseStyle = { backgroundColor: theme.colors.fill.primary, borderRadius: theme.borderRadius.xs, padding: theme.spacing.xl, marginTop: theme.spacing.xs, marginBottom: theme.spacing.lg, - shadowColor: theme.colors.fill.primary, - shadowOpacity: 0.25, - shadowRadius: 12, - shadowOffset: { - width: 0, - height: 6, - }, - elevation: 5, + ...Platform.select({ + ios: { + shadowColor: theme.colors.fill.primary, + shadowOpacity: 0.25, + shadowRadius: 12, + shadowOffset: { + width: 0, + height: 6, + }, + }, + android: { + elevation: 5, + }, + }), };
845-846: Implement bio from profile backend.The TODO comment indicates missing bio implementation from the Convos backend/local db.
Would you like me to help implement the bio functionality or create a GitHub issue to track this task?
805-831: Refactor duplicate navigation logic.The navigation logic for handling chat press is duplicated between
handleChatPressfunction and the message action inactionsTableViewItems.Consider reusing the
handleChatPressfunction in the message action:{ id: "message", title: translate("send_a_message"), titleColor: primaryColor(colorScheme), - action: () => { - navigation.popToTop(); - navigation.dispatch( - StackActions.push("Conversation", { - peer: route.params.address, - }) - ); - }, + action: handleChatPress, },Also applies to: 938-943
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
screens/Profile.tsx(10 hunks)
🧰 Additional context used
🪛 GitHub Check: tsc
screens/Profile.tsx
[failure] 184-184:
'shadowOffset' is specified more than once, so this usage will be overwritten.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: performance-test
🔇 Additional comments (1)
screens/Profile.tsx (1)
312-316: Replace hardcoded Alert title and message with localized strings.The Alert dialog uses hardcoded strings instead of localized translations.
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
screens/Profile.tsx
Outdated
| const colorScheme = useColorScheme(); | ||
|
|
||
| useHeader( | ||
| { |
There was a problem hiding this comment.
let's pull this out into features/profile/useProfileHeader - it's pretty beefy!
|
sorry, just realized this is draft! ping when ready! |
lourou
force-pushed
the
lr/profile-screen
branch
from
387bfc4 to
d890add
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
screens/Profile.tsx (1)
145-250: Consider moving ContactCard to design system.The
ContactCardcomponent is a well-structured, reusable UI component with animations. As suggested in past review comments, it would be beneficial to move it to the design system for better maintainability and reusability across the app.🧰 Tools
🪛 GitHub Check: tsc
[failure] 184-184:
'shadowOffset' is specified more than once, so this usage will be overwritten.🪛 GitHub Actions: Typescript CI
[error] 184-184: 'shadowOffset' is specified more than once, so this usage will be overwritten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/conversation/conversation.nav.tsx(2 hunks)screens/Navigation/ProfileNav.tsx(2 hunks)screens/Navigation/UserProfileNav.tsx(0 hunks)screens/Profile.tsx(10 hunks)screens/UserProfileScreen.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- screens/Navigation/UserProfileNav.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/UserProfileScreen.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
screens/Profile.tsx
[failure] 184-184:
'shadowOffset' is specified more than once, so this usage will be overwritten.
🪛 GitHub Actions: Typescript CI
screens/Profile.tsx
[error] 184-184: 'shadowOffset' is specified more than once, so this usage will be overwritten.
🔇 Additional comments (6)
screens/Navigation/ProfileNav.tsx (1)
20-22: LGTM! Navigation options have been simplified.The navigation options have been streamlined to use a static title, which aligns with the PR's objective of simplifying navigation controls.
features/conversation/conversation.nav.tsx (2)
51-56: Implement action for the 'more_vert' icon in header.The
HeaderActionwith the "more_vert" icon has aTODOcomment and lacks an implementation.
35-59: LGTM! Header configuration is well-structured.The header configuration:
- Uses
HStackfor proper layout of header actions- Includes QR code icon with navigation to "ShareProfile"
- Maintains consistent spacing using theme values
screens/Profile.tsx (3)
312-316: Replace hardcoded Alert title and message with localized strings.The
Alert.alertfunction uses hardcoded strings"Title"and"Message"instead of the localizedtranslatefunctions.
843-848: Implement bio from profile backend.The TODO comment indicates that bio implementation is pending. This should be implemented to provide complete user information.
Would you like me to help implement the bio integration with the Convos backend/local db?
178-189:⚠️ Potential issueFix TypeScript error in animated style.
The
shadowOffsetproperty is being specified twice in the style object, which will cause the second declaration to overwrite the first one.Apply this diff to fix the TypeScript error:
const animatedStyle = useAnimatedStyle(() => ({ transform: [ { perspective: 800 }, { rotateX: `${rotateX.value}deg` }, { rotateY: `${rotateY.value}deg` }, ], - shadowOffset: { - width: shadowOffsetX.value, - height: shadowOffsetY.value, - }, ...baseStyle, + shadowOffset: { + width: shadowOffsetX.value, + height: shadowOffsetY.value, + }, }));Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 184-184:
'shadowOffset' is specified more than once, so this usage will be overwritten.🪛 GitHub Actions: Typescript CI
[error] 184-184: 'shadowOffset' is specified more than once, so this usage will be overwritten.
lourou
force-pushed
the
lr/profile-screen
branch
from
d890add to
4672434
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
features/conversation/conversation.nav.tsx (1)
Line range hint
938-943: Reuse handleChatPress function to avoid code duplication.The navigation logic is duplicated. Consider reusing the
handleChatPressfunction to maintain DRY principles.Apply this diff:
- navigation.popToTop(); - navigation.dispatch( - StackActions.push("Conversation", { - peer: route.params.address, - }) - ); + handleChatPress();screens/Profile.tsx (2)
845-846: Implement bio from profile data.The TODO comment indicates that bio implementation is pending. Consider implementing this feature to enhance the user profile display.
Would you like me to help implement the bio feature or create a GitHub issue to track this task?
139-144: Consider extracting ContactCard to design system.The
ContactCardcomponent with its 3D tilt effect could be a reusable UI component. Consider extracting it to the design system for broader use across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
features/conversation/conversation.nav.tsx(2 hunks)screens/Navigation/ProfileNav.tsx(2 hunks)screens/Navigation/UserProfileNav.tsx(0 hunks)screens/Profile.tsx(10 hunks)screens/UserProfileScreen.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- screens/Navigation/UserProfileNav.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/UserProfileScreen.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
screens/Profile.tsx
[failure] 184-184:
'shadowOffset' is specified more than once, so this usage will be overwritten.
🪛 GitHub Actions: Typescript CI
screens/Profile.tsx
[error] 184-184: 'shadowOffset' is specified more than once, so this usage will be overwritten.
🔇 Additional comments (4)
screens/Navigation/ProfileNav.tsx (1)
20-22: LGTM! Navigation options have been simplified.The navigation configuration has been streamlined to use a static title, which aligns with the PR's goal of simplifying navigation.
features/conversation/conversation.nav.tsx (1)
51-56: Implement action for the 'more_vert' icon in header.The
HeaderActionwith the "more_vert" icon has aTODOcomment and lacks an implementation.screens/Profile.tsx (2)
312-316:⚠️ Potential issueReplace hardcoded Alert title and message with localized strings.
The
Alert.alertfunction uses hardcoded strings"Title"and"Message"instead of the localizedtranslatefunctions.Apply this diff to use the
translatefunctions:Alert.alert( - "Title", //translate("profile.block.title"), - /*translate("profile.block.message", { - name: preferredUserName, - })*/ - "Message", + translate("profile.block.title"), + translate("profile.block.message", { + name: preferredUserName, + }),Likely invalid or redundant comment.
178-189:⚠️ Potential issueFix TypeScript error in animated style.
The
shadowOffsetproperty is being specified twice in the style object, which will cause the second declaration to overwrite the first one.Apply this diff to fix the TypeScript error:
const animatedStyle = useAnimatedStyle(() => ({ transform: [ { perspective: 800 }, { rotateX: `${rotateX.value}deg` }, { rotateY: `${rotateY.value}deg` }, ], - shadowOffset: { - width: shadowOffsetX.value, - height: shadowOffsetY.value, - }, ...baseStyle, + shadowOffset: { + width: shadowOffsetX.value, + height: shadowOffsetY.value, + }, }));Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 184-184:
'shadowOffset' is specified more than once, so this usage will be overwritten.🪛 GitHub Actions: Typescript CI
[error] 184-184: 'shadowOffset' is specified more than once, so this usage will be overwritten.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
screens/Profile.tsx (1)
Line range hint
1074-1075: Implement error feedback in revokeOtherInstallations.The error handler includes a TODO comment but currently only tracks the error in Sentry. Consider implementing user feedback for a better experience.
} catch (error) { - // TODO: Show error feedback to user sentryTrackError(error); + Alert.alert( + translate("revoke_error_title"), + translate("revoke_error_description") + ); }
🧹 Nitpick comments (2)
screens/Profile.tsx (2)
139-246: Optimize ContactCard component performance.Consider these performance improvements:
- Move
baseStyleoutside the component or memoize it withuseMemo.- Memoize the
panGesturewithuseMemoto prevent recreating the gesture handler on every render.+ const baseStyle = { + backgroundColor: theme.colors.fill.primary, + borderRadius: theme.borderRadius.xs, + padding: theme.spacing.xl, + marginTop: theme.spacing.xs, + marginBottom: theme.spacing.lg, + shadowColor: theme.colors.fill.primary, + shadowOpacity: 0.25, + shadowRadius: 12, + elevation: 5, + }; const ContactCard = memo(function ContactCard({...}) { const { theme } = useAppTheme(); const colorScheme = useColorScheme(); const rotateX = useSharedValue(0); const rotateY = useSharedValue(0); const shadowOffsetX = useSharedValue(0); const shadowOffsetY = useSharedValue(6); - const baseStyle = {...} const animatedStyle = useAnimatedStyle(() => ({ transform: [ { perspective: 800 }, { rotateX: `${rotateX.value}deg` }, { rotateY: `${rotateY.value}deg` }, ], shadowOffset: { width: shadowOffsetX.value, height: shadowOffsetY.value, }, ...baseStyle, })); - const panGesture = Gesture.Pan()... + const panGesture = useMemo( + () => + Gesture.Pan() + .onBegin(() => { + rotateX.value = withSpring(0); + rotateY.value = withSpring(0); + shadowOffsetX.value = withSpring(0); + shadowOffsetY.value = withSpring(0); + }) + .onUpdate((event) => { + rotateX.value = event.translationY / 10; + rotateY.value = event.translationX / 10; + shadowOffsetX.value = -event.translationX / 20; + shadowOffsetY.value = event.translationY / 20; + }) + .onEnd(() => { + rotateX.value = withSpring(0); + rotateY.value = withSpring(0); + shadowOffsetX.value = withSpring(0); + shadowOffsetY.value = withSpring(0); + }), + [rotateX, rotateY, shadowOffsetX, shadowOffsetY] + );
841-842: Track the TODO for bio implementation.The TODO comment indicates that bio implementation from the Convos backend/local db is pending.
Would you like me to create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
screens/Profile.tsx(12 hunks)
🔇 Additional comments (2)
screens/Profile.tsx (2)
308-312: Replace hardcoded Alert title and message with localized strings.The Alert dialog still uses hardcoded strings instead of translations.
801-827: 🛠️ Refactor suggestionReuse handleChatPress to avoid code duplication.
The chat navigation logic is duplicated. Consider reusing the
handleChatPressfunction inactionsTableViewItems.items.push({ id: "message", title: translate("send_a_message"), titleColor: primaryColor(colorScheme), - action: () => { - setTimeout(() => { - const isPreviouslyInNavStack = navigation - .getState() - .routes.some((route) => { - if (route.name !== "Conversation") { - return false; - } - const params = route.params as ConversationNavParams; - return params?.peer === peerAddress.toLowerCase(); - }); - if (isPreviouslyInNavStack) { - navigation.popToTop(); - navigation.navigate({ - name: "Conversation", - params: { - peer: peerAddress, - }, - }); - } else { - navigation.popToTop(); - navigation.dispatch( - StackActions.push("Conversation", { - peer: peerAddress, - }) - ); - } - }, 300); - }, + action: () => { + setTimeout(handleChatPress, 300); + },Likely invalid or redundant comment.
lourou
force-pushed
the
lr/profile-screen
branch
2 times, most recently
from
99fe3b6 to
25f268a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
screens/Profile.tsx (2)
292-407: 🛠️ Refactor suggestionExtract header configuration to a custom hook.
The header configuration is quite complex and should be extracted to improve maintainability.
Create a new file
features/profile/useProfileHeader.tsand move the header configuration logic there:- useHeader( - { - safeAreaEdges: ["top"], - titleComponent: (...), - LeftActionComponent: (...), - RightActionComponent: (...), - }, - [router, theme, peerAddress, preferredUserName, setPeersStatus, colorScheme] - ); + useProfileHeader({ + router, + theme, + peerAddress, + preferredUserName, + setPeersStatus, + colorScheme, + isMyProfile, + handleChatPress + });
347-352:⚠️ Potential issueReplace hardcoded Alert strings with translations.
The Alert dialog still uses hardcoded strings instead of the localized translate functions.
Apply this diff to use translations:
- "Title", //translate("profile.block.title"), - /*translate("profile.block.message", { - name: preferredUserName, - })*/ - "Message", + translate("profile.block.title"), + translate("profile.block.message", { + name: preferredUserName, + }),
🧹 Nitpick comments (8)
design-system/Button/Button.styles.ts (2)
48-52: Consider extracting common styles.The base styles could be extracted into a constant to avoid recreation on each call.
+const BASE_BUTTON_STYLE: ViewStyle = { + flexDirection: "row", + alignItems: "center", + justifyContent: "center", + gap: spacing.xxs, +}; export const getButtonViewStyle = ... const baseStyle: ViewStyle = { - flexDirection: "row", - alignItems: "center", - justifyContent: "center", - gap: spacing.xxs, + ...BASE_BUTTON_STYLE, borderRadius: borderRadius.sm, overflow: "hidden", // ... rest of the styles };
74-79: Consider using a state-to-style mapping for better maintainability.The switch-case with nested conditionals could be refactored to use a mapping object for better maintainability.
const variantStyles: Record<IButtonVariant, (props: { pressed: boolean, disabled: boolean, colors: Theme['colors'] }) => Partial<ViewStyle>> = { 'fill': ({ pressed, disabled, colors }) => ({ backgroundColor: disabled ? colors.fill.tertiary : pressed ? colors.fill.secondary : colors.fill.primary }), 'outline': ({ pressed, colors }) => ({ borderWidth: 1, borderColor: colors.border.secondary, backgroundColor: pressed ? colors.fill.minimal : 'transparent' }), // ... other variants }; // Usage in getButtonViewStyle const variantStyle = variantStyles[variant]({ pressed, disabled, colors }); return { ...baseStyle, ...variantStyle };Also applies to: 84-89, 99-102
design-system/Button/Button.tsx (1)
126-129: Consider extracting style computation logic.The style computation logic in both Pressable and Text could be extracted into memoized helpers for better reusability.
const getComponentStyle = useMemo(() => { return (pressed: boolean, disabled: boolean) => [ pressed ? $pressedViewStyle : $viewStyle, disabled && $disabledViewStyle, ]; }, [$pressedViewStyle, $viewStyle, $disabledViewStyle]); // Usage <Pressable style={({ pressed }) => getComponentStyle(pressed, !!disabled)} ... >Also applies to: 168-171
features/profiles/utils/formatUsername.ts (1)
18-21: Consider optimizing the domain replacement logic.The current implementation uses two separate
.replace()calls. Consider using a single regex pattern with alternation for better performance.- const cleanUsername = username - .replace(/\.conversedev\.eth$/, "") - .replace(/\.converse\.xyz$/, ""); + const cleanUsername = username.replace(/\.(conversedev\.eth|converse\.xyz)$/, "");features/profiles/utils/__tests__/formatUsername.test.ts (1)
3-29: Consider adding more edge cases to the test suite.The test suite is well-structured but could benefit from additional edge cases:
- Multiple dots in username (e.g., "user.name.conversedev.eth")
- Special characters in username
- Case sensitivity
it("should handle complex usernames correctly", () => { expect(formatUsername("user.name.conversedev.eth")).toBe("@user.name"); expect(formatUsername("User-Name.converse.xyz")).toBe("@User-Name"); expect(formatUsername("USER.conversedev.eth")).toBe("@USER"); });screens/Navigation/ProfileNav.tsx (1)
15-24: Add JSDoc documentation for the ProfileNav component.Consider adding component-level documentation to explain the purpose and usage of this navigation screen.
+/** + * Navigation wrapper for the Profile screen. + * Handles profile viewing with optional group conversation context. + * @param {ProfileNavParams} params.address - The address of the profile to view + * @param {ProfileNavParams} params.fromGroupTopic - Optional group conversation context + */ export default function ProfileNav() {features/conversation/conversation.nav.tsx (1)
39-57: Consider extracting header actions to a separate component.The header actions could be moved to a dedicated component for better maintainability and reusability.
type ConversationHeaderActionsProps = { onSharePress: () => void; onMenuPress: () => void; }; const ConversationHeaderActions: React.FC<ConversationHeaderActionsProps> = ({ onSharePress, onMenuPress, }) => { const { theme } = useAppTheme(); return ( <HStack style={{ alignItems: "center", columnGap: theme.spacing.xxs, }} > <HeaderAction icon="qrcode" onPress={onSharePress} /> <HeaderAction icon="more_vert" onPress={onMenuPress} /> </HStack> ); };screens/Profile.tsx (1)
Line range hint
813-1093: Consider extracting profile sections into separate components.The profile sections (usernames, addresses, social items, actions) are tightly coupled within the ProfileScreenImpl component. Consider extracting these into separate components for better maintainability.
Example structure:
// components/ProfileSections/UsernamesSection.tsx export const UsernamesSection = ({ items }: { items: TableViewItemType[] }) => ( <TableView items={items} title={`USERNAME${items.length > 1 ? "S" : ""}`} style={themed($sectionContainer)} /> ); // Usage in ProfileScreenImpl {usernamesItems.length > 0 && ( <UsernamesSection items={usernamesItems} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
design-system/Button/Button.props.ts(1 hunks)design-system/Button/Button.styles.ts(3 hunks)design-system/Button/Button.tsx(3 hunks)features/conversation/conversation.nav.tsx(2 hunks)features/profiles/utils/__tests__/formatUsername.test.ts(1 hunks)features/profiles/utils/formatUsername.ts(1 hunks)screens/Navigation/ProfileNav.tsx(2 hunks)screens/Navigation/UserProfileNav.tsx(0 hunks)screens/Profile.tsx(13 hunks)screens/UserProfileScreen.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- screens/Navigation/UserProfileNav.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/UserProfileScreen.tsx
🔇 Additional comments (8)
design-system/Button/Button.props.ts (1)
17-17: LGTM! Verify styling implementation for the new variant.The addition of "link.bare" variant is clean and non-breaking. Let's verify its styling implementation.
✅ Verification successful
Styling for "link.bare" variant is properly implemented
The variant has appropriate minimal styling focusing on content alignment without decorative elements, matching its intended bare design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify styling implementation for "link.bare" variant # Check if the new variant has corresponding style handling rg -l "link\.bare" .Length of output: 123
Script:
#!/bin/bash # Check styling implementation in Button.styles.ts rg "link\.bare" ./design-system/Button/Button.styles.ts -B 2 -A 2Length of output: 229
Script:
#!/bin/bash # Get complete styling implementation for link.bare variant ast-grep --pattern 'if (variant === "link.bare") { $$$ }'Length of output: 574
design-system/Button/Button.styles.ts (2)
38-46: LGTM! Function signature improvement.Making the
pressedparameter required and using explicit type definition improves type safety and makes the API contract clearer.
61-69: LGTM! Clean implementation of link.bare variant.The special case for "link.bare" correctly implements a minimal button style without decorations.
design-system/Button/Button.tsx (2)
55-63: LGTM! Improved style memoization.Good use of
useMemofor style computation with correct dependencies.
75-78: LGTM! Clean style organization.Good separation of concerns with individual memoized style objects and clear dependencies.
Also applies to: 80-88, 90-98, 100-102
features/conversation/conversation.nav.tsx (2)
51-56: Implement the menu action for the more_vert icon.The TODO comment for opening the menu needs to be implemented.
47-49: Add error handling for navigation.Consider adding error handling for the navigation action to handle potential navigation failures.
onPress={() => { + try { navigation.navigate("ShareProfile"); + } catch (error) { + console.error("Failed to navigate to ShareProfile:", error); + // TODO: Show error toast/alert to user + } }}screens/Profile.tsx (1)
831-831: Implement bio functionality from backend.The TODO comment indicates missing bio implementation. This should be addressed to complete the profile functionality.
Would you like me to help implement the bio integration with the Convos backend?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
screens/Profile.tsx (1)
351-355:⚠️ Potential issueReplace hardcoded Alert title and message with localized strings.
The Alert dialog uses hardcoded strings "Title" and "Message" instead of the localized translate functions.
🧹 Nitpick comments (1)
screens/Profile.tsx (1)
834-834: Implement the bio feature from backend/local db.The TODO comment indicates that the bio feature needs to be implemented. This should be connected to the Convos backend or local database.
Would you like me to help create a GitHub issue to track this task or generate the implementation code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
screens/Profile.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: performance-test
- GitHub Check: lint
🔇 Additional comments (3)
screens/Profile.tsx (3)
137-142: Great documentation!The component documentation clearly explains the purpose and functionality of the ContactCard component.
286-293: Reuse handleChatPress to avoid code duplication.The chat navigation logic is duplicated in the TableView items. Consider reusing this function to maintain DRY principles.
235-237: Implement the Edit button functionality.The Edit button currently shows a "Coming soon" alert. Since the codebase has profile editing functionality (
useCreateOrUpdateProfileInfo), this should be properly implemented.Run the following script to verify the profile editing infrastructure:
✅ Verification successful
Implement Edit functionality using existing
useCreateOrUpdateProfileInfohookThe Edit button should use the existing profile update functionality. Reference implementation can be found in
UserProfileScreen.tsxwhich demonstrates proper usage of the hook including loading and error handling.
- Import and use
useCreateOrUpdateProfileInfofromscreens/Onboarding/OnboardingUserProfileScreen- Follow the implementation pattern from
UserProfileScreen.tsx- Handle loading and error states appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for profile editing functionality ast-grep --pattern 'useCreateOrUpdateProfileInfo($$$)' # Look for profile editing navigation patterns rg "navigation.*edit.*profile" -t ts -t js -iLength of output: 355
Script:
#!/bin/bash # Check hook implementation ast-grep --pattern 'const useCreateOrUpdateProfileInfo = $$$' # Check usage context in files rg -B 2 -A 5 "useCreateOrUpdateProfileInfo" -t ts -t tsx # Look for edit-related components or patterns rg "edit.*profile|profile.*edit" -i -t ts -t tsxLength of output: 227
Script:
#!/bin/bash # Find hook implementation file fd -e ts -e tsx | xargs rg "export.*useCreateOrUpdateProfileInfo" # Check for profile-related files and edit functionality fd -e ts -e tsx | xargs rg -l "profile|edit" -i # Look for the hook's implementation and usage fd -e ts -e tsx | xargs rg -B 3 -A 6 "useCreateOrUpdateProfileInfo"Length of output: 10365
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
screens/Profile.tsx (1)
59-163: 🛠️ Refactor suggestionExtract header configuration to a custom hook.
The header configuration is complex and should be extracted to
features/profile/useProfileHeaderfor better maintainability.Would you like me to help extract this logic into a custom hook?
🧹 Nitpick comments (7)
design-system/table/FullWidthTable.tsx (2)
8-14: Consider making the row type more flexible.The
IFullWidthTableRowtype could be more reusable by:
- Supporting custom render functions for complex values
- Allowing additional styling props
- Supporting accessibility attributes
type IFullWidthTableRow = { label: string; value: string; + renderValue?: (value: string) => React.ReactNode; + accessibilityLabel?: string; + testID?: string; onValueChange?: (value: boolean) => void; isSwitch?: boolean; isEnabled?: boolean; + style?: ViewStyle; };
57-67: Consider consolidating style objects for better maintainability.The current style implementation has multiple separate style objects. Consider consolidating them into a single styles object for better organization.
-const $container: ThemedStyle<ViewStyle> = ({ +const styles = { + container: ({ colors, spacing, borderWidth }: ThemeType) => ({ colors, spacing, borderWidth, }) => ({ backgroundColor: colors.background.surface, borderRadius: spacing.xs, borderWidth: borderWidth.sm, borderColor: colors.border.subtle, overflow: "hidden", - }); + }), -const $sectionContainer: ThemedStyle<ViewStyle> = () => ({ + sectionContainer: () => ({ width: "100%", - }); + }), // ... similar changes for other style objectsAlso applies to: 69-71, 73-78, 80-82, 84-87
features/profiles/components/ContactCard.tsx (1)
73-91: Optimize gesture handler performance and add haptic feedback.The gesture handler could benefit from performance optimizations and enhanced user feedback.
const panGesture = Gesture.Pan() .onBegin(() => { + 'worklet'; rotateX.value = withSpring(0); rotateY.value = withSpring(0); shadowOffsetX.value = withSpring(0); shadowOffsetY.value = withSpring(0); + runOnJS(Haptics.selectionAsync)(); }) .onUpdate((event) => { + 'worklet'; rotateX.value = event.translationY / 10; rotateY.value = event.translationX / 10; shadowOffsetX.value = -event.translationX / 20; shadowOffsetY.value = event.translationY / 20; }) .onEnd(() => { + 'worklet'; rotateX.value = withSpring(0); rotateY.value = withSpring(0); shadowOffsetX.value = withSpring(0); shadowOffsetY.value = withSpring(0); + runOnJS(Haptics.selectionAsync)(); });i18n/translations/en.ts (1)
366-376: Ensure consistent naming convention for nested translation keys.The translation keys under
profile.settingsfollow a different naming pattern compared to other sections. Consider standardizing the naming convention.settings: { - notifications: "Notifications", - archive: "Archive", - keep_messages: "Keep messages", - blocked: "Blocked", + title: { + notifications: "Notifications", + archive: "Archive", + keep_messages: "Keep messages", + blocked: "Blocked", + }, },i18n/translations/fr.ts (1)
362-373: Verify translation consistency between English and French versions.The French translations should maintain consistent structure and meaning with their English counterparts. Consider the following adjustments:
block: { title: "Bloquer l'utilisateur", - message: "Êtes-vous sûr de vouloir bloquer {{name}} ?", + message: "Voulez-vous vraiment bloquer {{name}} ?", }, save: "Enregistrer", settings: { notifications: "Notifications", archive: "Archive", - keep_messages: "Conserver les messages", + keep_messages: "Garder les messages", blocked: "Bloqué", },The suggested changes:
- Use "Voulez-vous vraiment" instead of "Êtes-vous sûr" for better French localization
- Simplify "Conserver les messages" to "Garder les messages" for consistency with the English version's brevity
screens/Profile.tsx (2)
94-158: Extract menu configuration to a separate constant.The menu configuration is quite large and could be extracted to improve readability.
+const PROFILE_MENU_CONFIG = { + menuTitle: "", + menuItems: [ + { + actionKey: "share", + actionTitle: translate("share"), + icon: { + iconType: "SYSTEM", + iconValue: "square.and.arrow.up", + }, + }, + // ... other menu items + ], +}; <ContextMenuButton style={themed($contextMenu)} isMenuPrimaryAction onPressMenuItem={({ nativeEvent }) => { // ... handler code }} - menuConfig={{ - menuTitle: "", - menuItems: [ - // ... menu items - ], - }} + menuConfig={PROFILE_MENU_CONFIG} >
215-248: Remove unnecessary Fragment wrapper.The Fragment wrapper is redundant as it contains only one child.
- {!isMyProfile && ( - <> <VStack style={[themed($section), { paddingVertical: theme.spacing.lg }]} > <FullWidthTable rows={PROFILE_TABLE_ROWS} /> </VStack> - </> - )} + {!isMyProfile && ( + <VStack + style={[themed($section), { paddingVertical: theme.spacing.lg }]} + > + <FullWidthTable rows={PROFILE_TABLE_ROWS} /> + </VStack> + )}🧰 Tools
🪛 Biome (1.9.4)
[error] 216-247: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
design-system/table/FullWidthTable.tsx(1 hunks)features/profiles/components/ContactCard.tsx(1 hunks)i18n/translations/en.ts(1 hunks)i18n/translations/fr.ts(1 hunks)screens/Profile.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
screens/Profile.tsx
[error] 216-247: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (4)
features/profiles/components/ContactCard.tsx (1)
41-44: 🛠️ Refactor suggestionConsider using worklet functions for better animation performance.
The shared values should be initialized using worklet functions to optimize animation performance.
- const rotateX = useSharedValue(0); - const rotateY = useSharedValue(0); - const shadowOffsetX = useSharedValue(0); - const shadowOffsetY = useSharedValue(6); + const rotateX = useSharedValue(0, "worklet"); + const rotateY = useSharedValue(0, "worklet"); + const shadowOffsetX = useSharedValue(0, "worklet"); + const shadowOffsetY = useSharedValue(6, "worklet");Likely invalid or redundant comment.
screens/Profile.tsx (3)
1-29: LGTM! Well-organized imports.The imports are cleanly structured and all dependencies are necessary for the component's functionality.
55-57: Implement profile editing functionality.The
handleEditProfilefunction only toggles theeditModestate but doesn't implement actual editing functionality.Would you like me to help implement the profile editing functionality using the existing
useCreateOrUpdateProfileInfohook?
254-284: LGTM! Well-structured themed styles.The styles are well-organized, properly themed, and make good use of design tokens.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
screens/Profile.tsx (2)
105-106: Provide user feedback after copying to clipboard.Consider showing a toast or notification to inform the user that the address has been copied to the clipboard.
227-243: Remove unnecessary Fragment to simplify JSX structure.The
Fragmentwrapping theVStackcomponent is redundant since it contains only one child.Apply this diff to remove the unnecessary
Fragment:{!isMyProfile && ( - <> <VStack style={[themed($section), { paddingVertical: theme.spacing.lg }]} > <FullWidthTable rows={[ { label: translate("profile.settings.notifications"), value: "On", isSwitch: true, isEnabled: true, onValueChange: () => {}, }, ]} /> </VStack> - </> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 227-243: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
i18n/translations/fr.ts (3)
350-353: Verify user blocking confirmation message format.The blocking confirmation message uses a placeholder
{{name}}which is good for localization. However, consider adding a period at the end of the confirmation message for consistency with other dialog messages in the UI.- message: "Êtes-vous sûr de vouloir bloquer {{name}} ?", + message: "Êtes-vous sûr de vouloir bloquer {{name}} ?.",
364-368: Review the account disconnection message formatting.The account disconnection description is a complex message that might need formatting improvements:
- Consider breaking it into two separate messages for better readability
- Add proper spacing after periods
disconnect_account_description: - "Vos discussions de groupe seront chiffrées et sauvegardées sur votre appareil jusqu'à ce que vous supprimiez Converse. Vos messages privés seront sauvegardés par le réseau XMTP.", + "Vos discussions de groupe seront chiffrées et sauvegardées sur votre appareil jusqu'à ce que vous supprimiez Converse.\n\nVos messages privés seront sauvegardés par le réseau XMTP.",
372-373: Ensure consistent error message formatting.The attachment download error message should follow the same format as other error messages in the file (ending with a period).
attachment_message_error_download: - "Impossible de télécharger la pièce jointe", + "Impossible de télécharger la pièce jointe.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
design-system/table/FullWidthTable.tsx(1 hunks)features/profiles/components/ContactCard.tsx(1 hunks)hooks/useDisconnectActionSheet.ts(1 hunks)i18n/translations/en.ts(1 hunks)i18n/translations/fr.ts(1 hunks)screens/Profile.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- design-system/table/FullWidthTable.tsx
- i18n/translations/en.ts
🧰 Additional context used
🪛 Biome (1.9.4)
screens/Profile.tsx
[error] 227-243: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (4)
features/profiles/components/ContactCard.tsx (1)
105-109: Add error handling for avatar loading failures.The
Avatarcomponent should handle loading failures gracefully to enhance user experience.hooks/useDisconnectActionSheet.ts (1)
27-27: Ensure all translation keys are correctly defined and used.Verify that the
log_outtranslation key exists in all localization files and accurately reflects the intended action.screens/Profile.tsx (1)
62-166: Extract header configuration into a separate hook for better maintainability.The header configuration logic is complex and can be moved to
features/profile/useProfileHeaderto improve code readability.i18n/translations/fr.ts (1)
356-361: LGTM: Settings menu translations are clear and concise.The translations for settings menu items follow good practices:
- Consistent capitalization
- Clear and concise terms
- Common French UI terminology
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
i18n/translations/fr.ts (1)
365-370: Consider revising disconnect-related translations for clarity.The terms "log out" and "disconnect" are used interchangeably, which might confuse users.
Consider standardizing on either "déconnexion" or "déconnexion du compte" consistently across these strings:
- log_out: "Se déconnecter", - disconnect_delete_local_data: "Se déconnecter et supprimer les données locales", - disconnect_this_account: "Se déconnecter de ce compte", + log_out: "Déconnexion", + disconnect_delete_local_data: "Déconnexion et suppression des données locales", + disconnect_this_account: "Déconnexion du compte"features/profiles/components/ContactCard.tsx (1)
110-122: Track the pending Names menu feature.The TODO comment indicates a pending feature dependent on SCW completion. This should be tracked to ensure implementation once the dependency is ready.
Would you like me to create a GitHub issue to track this pending feature implementation?
screens/Profile.tsx (1)
271-288: Remove unnecessary Fragment.The Fragment wrapper is redundant as it contains only one child element.
- <> <VStack style={[themed($section), { paddingVertical: theme.spacing.lg }]} > <FullWidthTable rows={[ { label: translate("profile.settings.notifications"), value: "On", isSwitch: true, isEnabled: true, onValueChange: () => {}, }, ]} /> </VStack> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 271-287: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
features/conversation-list/conversation-list.screen-header.tsx(1 hunks)features/profiles/components/ContactCard.tsx(1 hunks)i18n/translations/en.ts(2 hunks)i18n/translations/fr.ts(1 hunks)screens/Profile.tsx(1 hunks)screens/ShareProfile.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- screens/ShareProfile.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- i18n/translations/en.ts
🧰 Additional context used
🪛 Biome (1.9.4)
screens/Profile.tsx
[error] 271-287: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (12)
features/conversation-list/conversation-list.screen-header.tsx (1)
151-151: LGTM! Translation key update is consistent.The change from
"App settings"to"app_settings"aligns with the translation keys defined in the localization files.i18n/translations/fr.ts (4)
350-353: LGTM! Block functionality translations are clear and user-friendly.The block user confirmation dialog provides clear messaging and includes placeholder for user name.
355-357: LGTM! Profile action translations are consistent.The edit/save/done actions follow a clear progression for profile modifications.
358-363: LGTM! Settings section is well-organized.The settings translations cover essential profile features: notifications, archive, message retention, and blocked users.
371-375: LGTM! Profile features and error messages are well-defined.The translations cover essential profile features and error states appropriately.
features/profiles/components/ContactCard.tsx (3)
1-22: Well-structured imports and type definitions!The component is properly optimized with React.memo, and the props interface is well-defined with appropriate optional flags.
41-91: Excellent animation implementation!The 3D tilt effect is well-implemented using shared values and proper gesture handling, with smooth transitions and proper cleanup.
105-109: Add error handling for avatar loading failures.The Avatar component should handle loading failures gracefully, as previously suggested.
screens/Profile.tsx (4)
61-210: Extract profile header logic to a separate file.As previously suggested, the header configuration logic is complex and should be extracted to
features/profile/useProfileHeaderfor better maintainability.
166-170: Implement profile edit functionality.The edit action currently shows a placeholder alert. This needs to be implemented to provide the actual editing capability.
Would you like me to help implement the edit functionality using the existing
useCreateOrUpdateProfileInfohook?
231-265: Reduce code duplication in table configurations.As previously suggested, the table configurations should be extracted to constants to avoid duplication.
Also applies to: 275-285
294-319: Well-structured themed styles!The styled components are well-organized, follow consistent naming conventions, and properly utilize theme variables.
lourou
force-pushed
the
lr/profile-screen
branch
from
64ca06b to
8e0cba6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
screens/Profile.tsx (1)
Line range hint
67-124: Extract context menu action handler into a custom hook.The
handleContextMenuActionfunction is complex and handles multiple responsibilities.Extract this logic into a custom hook like
useProfileContextMenuActionsfor better maintainability and reusability.🧰 Tools
🪛 Biome (1.9.4)
[error] 290-306: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
🧹 Nitpick comments (11)
design-system/Icon/Icon.types.ts (1)
15-15: LGTM! Consider adding JSDoc comments for new icons.The new icon additions (
pencilandperson.crop.circle.badge.xmark) align well with the profile screen requirements. Consider adding JSDoc comments to document their intended usage contexts.export type IIconName = | "xmark" | "xmark.circle.fill" | "plus" ... + /** Used for edit/modify actions */ | "pencil" ... + /** Used for removing/disconnecting user profiles */ | "person.crop.circle.badge.xmark" ...Also applies to: 56-56
design-system/Button/Button.styles.ts (3)
Line range hint
48-60: Consider simplifying the size-based spacing logic.The current implementation repeats the size condition check. Consider using a mapping object to make the code more maintainable:
+ const sizeToSpacing = { + sm: { vertical: spacing.xxs, horizontal: spacing.xs }, + md: { vertical: spacing.xxs, horizontal: spacing.xs }, + lg: { vertical: spacing.xs, horizontal: spacing.sm }, + } as const; + const baseStyle: ViewStyle = { flexDirection: "row", alignItems: "center", justifyContent: "center", gap: spacing.xxs, borderRadius: borderRadius.sm, overflow: "hidden", - paddingVertical: - size === "md" || size === "sm" ? spacing.xxs : spacing.xs, - paddingHorizontal: - size === "md" || size === "sm" ? spacing.xs : spacing.sm, + paddingVertical: sizeToSpacing[size].vertical, + paddingHorizontal: sizeToSpacing[size].horizontal, };
61-69: Avoid style duplication for "link.bare" variant.The "link.bare" variant duplicates some properties from the base style. Consider reusing the common properties:
if (variant === "link.bare") { + const { flexDirection, alignItems, justifyContent, gap } = baseStyle; return { - flexDirection: "row", - alignItems: "center", - justifyContent: "center", - gap: spacing.xxs, + flexDirection, + alignItems, + justifyContent, + gap, }; }
Line range hint
71-110: Add exhaustive type checking for variants.The switch statement for variants could benefit from exhaustive type checking to ensure all variants are handled:
+ const assertNever = (x: never): never => { + throw new Error(`Unexpected variant: ${x}`); + }; if (action === "primary") { switch (variant) { case "fill": // ... existing code ... break; case "outline": // ... existing code ... break; case "link": case "text": // ... existing code ... break; case "link.bare": // ... existing code ... break; default: - break; + assertNever(variant); } }screens/Navigation/ProfileNav.tsx (1)
3-3: Consider platform-specific navigation patterns.With the switch to NativeStack and simplified options, ensure that platform-specific navigation patterns and accessibility features are still maintained.
Consider:
- iOS back gesture navigation
- Android hardware back button handling
- Screen reader accessibility for the navigation header
- Dynamic header styling based on the platform
Would you like me to provide a more robust implementation that handles these platform-specific concerns?
Also applies to: 20-22
features/profiles/components/ContactCard.tsx (2)
110-122: Remove commented-out code.The commented-out code for the "Names" menu should be removed and tracked in a separate issue if needed.
Would you like me to create a GitHub issue to track the implementation of the "Names" menu feature for Smart Contract Wallet integration?
73-91: Add debounce to pan gesture updates.The pan gesture updates might cause performance issues on lower-end devices due to frequent animation updates.
+import { debounce } from 'lodash'; const panGesture = Gesture.Pan() .onBegin(() => { rotateX.value = withSpring(0); rotateY.value = withSpring(0); shadowOffsetX.value = withSpring(0); shadowOffsetY.value = withSpring(0); }) - .onUpdate((event) => { + .onUpdate(debounce((event) => { rotateX.value = event.translationY / 10; rotateY.value = event.translationX / 10; shadowOffsetX.value = -event.translationX / 20; shadowOffsetY.value = event.translationY / 20; - }) + }, 16))design-system/table/FullWidthTable.tsx (1)
Line range hint
250-284: Extract table row configurations into a separate constant.The table row configurations are hardcoded and should be extracted for better maintainability.
+const MY_PROFILE_TABLE_ROWS: IFullWidthTableRow[] = [ + { + label: translate("profile.settings.notifications"), + value: "On", + isSwitch: true, + isEnabled: true, + onValueChange: () => {}, + }, + { + label: translate("profile.settings.archive"), + value: "Forever", + onValueChange: () => {}, + }, + // ... other rows +]; <FullWidthTable editMode={editMode} - rows={[...]} + rows={MY_PROFILE_TABLE_ROWS} />i18n/translations/fr.ts (3)
355-357: Consider adding validation feedback translations.While the basic profile editing actions are covered, consider adding translations for validation feedback messages that users might see when editing their profile (e.g., character limits, invalid inputs).
365-370: Ensure consistent terminology for disconnect/logout actions.The translations use both "Se déconnecter" and "Déconnecter". Consider using consistent terminology throughout:
- Line 365: "log_out" → "Se déconnecter"
- Line 368: "disconnect_this_account" → "Se déconnecter de ce compte"
374-375: Consider adding more specific error messages for attachments.The current error message is generic. Consider adding more specific error messages for different attachment download failure scenarios (e.g., network issues, file not found, insufficient permissions).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
design-system/Button/Button.props.ts(1 hunks)design-system/Button/Button.styles.ts(3 hunks)design-system/Button/Button.tsx(3 hunks)design-system/Icon/Icon.tsx(3 hunks)design-system/Icon/Icon.types.ts(2 hunks)design-system/table/FullWidthTable.tsx(1 hunks)features/conversation-list/conversation-list.screen-header.tsx(1 hunks)features/conversation/conversation.nav.tsx(2 hunks)features/profiles/components/ContactCard.tsx(1 hunks)features/profiles/utils/__tests__/formatUsername.test.ts(1 hunks)features/profiles/utils/formatUsername.ts(1 hunks)hooks/useDisconnectActionSheet.ts(1 hunks)i18n/translations/en.ts(2 hunks)i18n/translations/fr.ts(1 hunks)screens/Navigation/ProfileNav.tsx(2 hunks)screens/Navigation/UserProfileNav.tsx(0 hunks)screens/Profile.tsx(2 hunks)screens/ShareProfile.tsx(0 hunks)screens/UserProfileScreen.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- screens/ShareProfile.tsx
- screens/Navigation/UserProfileNav.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- features/conversation-list/conversation-list.screen-header.tsx
- features/profiles/utils/formatUsername.ts
- hooks/useDisconnectActionSheet.ts
- design-system/Button/Button.props.ts
- screens/UserProfileScreen.tsx
- design-system/Button/Button.tsx
- features/conversation/conversation.nav.tsx
- features/profiles/utils/tests/formatUsername.test.ts
- i18n/translations/en.ts
🧰 Additional context used
🪛 Biome (1.9.4)
screens/Profile.tsx
[error] 290-306: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (11)
design-system/Icon/Icon.tsx (2)
18-18: LGTM! Icon registry entries are consistent.The new icon mappings follow the established 1:1 convention and correctly correspond to their SF Symbols names.
Also applies to: 59-59
103-104: LGTM! Improved code formatting.The conditional expression formatting enhances readability while maintaining the same logic.
design-system/Button/Button.styles.ts (2)
38-46: LGTM! Improved type safety in function signature.The changes improve type safety by making the
pressedparameter required and using explicit type definitions. This forces consumers to handle the pressed state explicitly and provides better IDE support.
94-98: Remove or address the commented code.The commented code suggests incomplete refactoring for the "text" variant. Either remove the commented code or create a tracking issue to complete the refactoring.
Would you like me to create a GitHub issue to track this refactoring task?
screens/Navigation/ProfileNav.tsx (1)
20-22: Verify the impact of simplified navigation options.The navigation options have been significantly simplified from a dynamic configuration to a static title. This might have removed important functionality such as:
- Dynamic header buttons
- Color scheme adaptations
- Platform-specific behaviors
Let's check for any usage of the removed header functionality:
✅ Verification successful
Simplified navigation options are correct and align with codebase patterns
The navigation configuration has been appropriately simplified as header customizations are handled at the screen level rather than in the navigation configuration. The search results show that other profile-related screens manage their own header buttons and styling independently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to header buttons or navigation options in profile-related files rg -t typescript -t tsx "headerLeft|headerRight|navigationOptions.*Profile" # Check for color scheme usage in navigation rg -t typescript -t tsx "useColorScheme.*navigation" # Look for potential accessibility features in navigation headers rg -t typescript -t tsx "accessibilit.*header|a11y.*header"Length of output: 299
Script:
#!/bin/bash # Search for header-related configurations in TS/TSX files rg "headerLeft|headerRight|navigationOptions.*Profile" --glob "*.{ts,tsx}" # Look for color scheme usage in navigation rg "useColorScheme.*navigation" --glob "*.{ts,tsx}" # Check for header customization patterns rg "options\s*=\s*\{[\s\S]*?title.*?Profile" --glob "*.{ts,tsx}" -A 5 # Search for accessibility attributes in headers rg "accessibility.*header|a11y.*header" --glob "*.{ts,tsx}"Length of output: 1792
features/profiles/components/ContactCard.tsx (1)
105-109: Add error handling for avatar loading failures.The Avatar component should handle loading failures gracefully.
<Avatar uri={avatarUri} name={displayName} size={theme.avatarSize.lg} + onError={() => console.warn('Failed to load avatar')} + fallbackComponent={() => ( + <View style={{ + width: theme.avatarSize.lg, + height: theme.avatarSize.lg, + backgroundColor: theme.colors.background.subtle, + borderRadius: theme.avatarSize.lg / 2 + }} /> + )} />design-system/table/FullWidthTable.tsx (1)
8-16: 🛠️ Refactor suggestionAdd missing accessibility props to IFullWidthTableRow.
The row interface should include accessibility-related props for better screen reader support.
type IFullWidthTableRow = { label: string; value?: string; onValueChange?: (value: boolean) => void; isSwitch?: boolean; isEnabled?: boolean; isWarning?: boolean; onPress?: () => void; + accessibilityLabel?: string; + accessibilityHint?: string; + testID?: string; };Likely invalid or redundant comment.
screens/Profile.tsx (1)
132-134: Add safety check for route history access.Accessing
router.getState().routes[router.getState().routes.length - 2].namewithout ensuring there are enough routes may cause an error.- {router.canGoBack() && router.getState().routes.length >= 2 + {router.canGoBack() + && router.getState().routes.length >= 2 ? router.getState().routes[router.getState().routes.length - 2].name : ""}i18n/translations/fr.ts (3)
350-353: LGTM: Block user translations are clear and consistent.The block user translations are well-structured and follow the established pattern for confirmation dialogs.
371-373: LGTM: Profile page translations are clear and actionable.The translations for profile-related actions are clear and follow good UX writing practices.
358-363: Verify the settings menu structure.The settings translations look good, but we should verify that all possible settings options are covered.
✅ Verification successful
Settings menu structure is complete and accurate.
The French translations for the settings menu perfectly match all settings options implemented in the codebase, with no missing translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any settings-related components or configurations that might need translations rg -g '!i18n/*' -i 'settings|notifications|archive|blocked' -A 3Length of output: 68293
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
design-system/Button/Button.styles.ts (1)
61-69: Consider extracting common flex styles.The flex-related styles are duplicated between the base styles and the link.bare variant. Consider extracting these into a shared constant.
+ const flexStyles: ViewStyle = { + flexDirection: "row", + alignItems: "center", + justifyContent: "center", + gap: spacing.xxs, + }; const baseStyle: ViewStyle = { - flexDirection: "row", - alignItems: "center", - justifyContent: "center", - gap: spacing.xxs, + ...flexStyles, borderRadius: borderRadius.sm, // ... rest of base styles }; if (variant === "link.bare") { return { - flexDirection: "row", - alignItems: "center", - justifyContent: "center", - gap: spacing.xxs, + ...flexStyles, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
design-system/Button/Button.styles.ts(3 hunks)design-system/Button/Button.tsx(3 hunks)features/conversation/conversation.nav.tsx(2 hunks)features/profiles/utils/formatUsername.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- features/conversation/conversation.nav.tsx
- features/profiles/utils/formatUsername.ts
- design-system/Button/Button.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: performance-test
🔇 Additional comments (4)
design-system/Button/Button.styles.ts (4)
40-46: Well-structured type definitions!The explicit type definition improves type safety and makes the API contract clearer. Making
pressedrequired is a good practice as it ensures proper handling of button states.
47-52: Clean base style implementation!Good use of design tokens and consistent spacing logic. The
baseStyleapproach reduces code duplication and maintains consistency.
Line range hint
1-110: Verify styling implementation against requirements.The implementation correctly follows all styling instructions from the cursor query for primary action buttons. However, as noted in the query, styling for
action="danger"is still pending.Would you like me to help implement the danger action styles following the same patterns?
Line range hint
74-110: Clean up commented code and temporary solutions.
- There's commented code for the text variant that should be addressed:
// Put back when we're done refactoring all the variant="text" button // if (pressed) { // style.backgroundColor = colors.fill.minimal; // }
- The opacity change for the text variant is marked as temporary.
Please create tickets to track:
- Completing the text variant refactoring
- Replacing the temporary opacity solution with the final implementation
Would you like me to help create GitHub issues to track these tasks?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
i18n/translations/fr.ts (1)
359-363: Consider adding descriptions for settings options.The settings translations are concise, but consider adding descriptions to clarify what each setting does, especially for "Archive" and "Keep messages" which might not be self-explanatory to users.
settings: { notifications: "Notifications", + notifications_description: "Recevez des notifications pour les nouveaux messages", archive: "Archive", + archive_description: "Archivez les conversations pour les masquer de la liste principale", keep_messages: "Conserver les messages", + keep_messages_description: "Conservez l'historique des messages sur votre appareil", },design-system/Icon/Icon.android.tsx (1)
Line range hint
97-100: Consider enhancing icon validation.The current error handling logs a warning and returns null for invalid icons. Consider throwing an error in development but gracefully handling it in production for better developer experience.
if (!iconName) { + if (__DEV__) { + throw new Error(`Invalid icon name: ${icon || picto}`); + } logger.warn(`Invalid icon name ${icon || picto}`); return null; }design-system/Table/Table.types.ts (1)
1-9: Consider splitting the interface into more focused types.The current interface combines multiple concerns (display, interactivity, form controls). Consider splitting it into more focused types for better maintainability:
-export type ITableRow = { +type BaseTableRow = { label: string; value?: string | number; +}; + +type InteractiveTableRow = BaseTableRow & { onPress?: () => void; +}; + +type SwitchTableRow = BaseTableRow & { onValueChange?: (value: boolean) => void; isSwitch: true; disabled?: boolean; +}; + +export type TableRow = BaseTableRow | InteractiveTableRow | SwitchTableRow & { isWarning?: boolean; - onValueChange?: (value: boolean) => void; - isSwitch?: boolean; - disabled?: boolean; - onPress?: () => void; };Also, consider removing the "I" prefix from interface names as it's not a common TypeScript convention.
design-system/Table/Table.tsx (1)
8-11: Document the purpose of the editMode prop.The
editModeprop's purpose is not clear from the implementation. Consider adding JSDoc comments to explain its usage and impact on the component's behavior.features/profiles/components/ContactCard.tsx (1)
111-122: Uncomment and implement edit functionality.The edit functionality is currently commented out with a TODO. This should be implemented to enable profile editing.
Would you like me to help create a GitHub issue to track the implementation of the edit functionality?
screens/Profile.tsx (1)
320-350: Extract notification permission logic to a custom hook.The notification permission handling logic is complex and could be extracted to improve readability and reusability.
Consider creating a custom hook like
useNotificationPermissionsthat encapsulates this logic:const useNotificationPermissions = () => { const { notificationsPermissionStatus, setNotificationsPermissionStatus } = useAppStore(s => ({ notificationsPermissionStatus: s.notificationsPermissionStatus, setNotificationsPermissionStatus: s.setNotificationsPermissionStatus, })); const requestPermissions = async () => { if (notificationsPermissionStatus === "denied") { if (Platform.OS === "android") { const newStatus = await requestPushNotificationsPermissions(); if (newStatus === "denied") { Linking.openSettings(); } else if (newStatus) { setNotificationsPermissionStatus(newStatus); } } else { Linking.openSettings(); } } else if (notificationsPermissionStatus === "undetermined") { const newStatus = await requestPushNotificationsPermissions(); if (!newStatus) return; setNotificationsPermissionStatus(newStatus); } }; return { notificationsPermissionStatus, requestPermissions }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
components/TransactionPreview/TransactionResult.tsx(1 hunks)components/TransactionPreview/TransactionSimulationFailure.tsx(1 hunks)design-system/Button/Button.styles.ts(1 hunks)design-system/Icon/Icon.android.tsx(3 hunks)design-system/Table/Table.tsx(1 hunks)design-system/Table/Table.types.ts(1 hunks)design-system/Table/TableRow.tsx(1 hunks)design-system/TextField/TextField.tsx(1 hunks)design-system/chip.tsx(2 hunks)features/ExternalWalletPicker/ExternalWalletPicker.tsx(1 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(3 hunks)features/profiles/components/ContactCard.tsx(1 hunks)i18n/translations/en.ts(2 hunks)i18n/translations/fr.ts(1 hunks)screens/Profile.tsx(2 hunks)theme/border-radius.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- components/TransactionPreview/TransactionSimulationFailure.tsx
- features/ExternalWalletPicker/ExternalWalletPicker.tsx
- features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- i18n/translations/en.ts
🧰 Additional context used
🪛 GitHub Check: tsc
screens/Profile.tsx
[failure] 278-278:
Argument of type '"profile.username_copied"' is not assignable to parameter of type 'TxKeyPath'.
[failure] 290-290:
Argument of type '"profile.ens_copied"' is not assignable to parameter of type 'TxKeyPath'.
[failure] 304-304:
Argument of type '"profile.domain_copied"' is not assignable to parameter of type 'TxKeyPath'.
design-system/TextField/TextField.tsx
[failure] 182-182:
Property 'xss' does not exist on type '{ readonly xxs: 8; readonly xs: 12; readonly sm: 16; readonly md: 24; readonly message: { readonly bubble: 16; readonly attachment: 16; }; }'.
🪛 GitHub Actions: Typescript CI
design-system/TextField/TextField.tsx
[error] 182-182: Property 'xss' does not exist on type '{ readonly xxs: 8; readonly xs: 12; readonly sm: 16; readonly md: 24; readonly message: { readonly bubble: 16; readonly attachment: 16; }; }'
🔇 Additional comments (14)
i18n/translations/fr.ts (4)
350-353: LGTM: Block user translations are clear and consistent.The block user translations are well-structured and follow the established pattern for confirmation dialogs, using a placeholder for the user's name.
355-358: Verify translation consistency for action buttons.The translations for "edit", "save", and "done" are appropriate, but note that "done" appears twice in the file (line 357 and line 376). This could lead to inconsistent translations if they represent different contexts.
Run this script to check for potential inconsistencies:
✅ Verification successful
Translations are consistent and contextually appropriate
The standalone "done" key is consistently translated as "Valider" in both occurrences. The different translation "Terminé" is used appropriately in the session management context (revoke_done_*).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for duplicate keys in translation files rg -U "done.*?:.*?[\"'].*?[\"']" --type tsLength of output: 460
371-376: Review formatting of notification-related strings.The translations for UI actions and error messages are clear, but verify that the notification text ("Activer les notifications") aligns with the system's notification permission dialogs on French devices.
Run this script to check notification-related strings:
✅ Verification successful
Notification string translation is appropriate and follows platform conventions
The French translation "Activer les notifications" is correctly formatted and aligns with standard system notification dialogs on French devices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for notification-related translations rg -U "notification.*?:.*?[\"'].*?[\"']" --type tsLength of output: 503
365-370: Ensure consistent terminology for disconnect/logout actions.The translations use both "Se déconnecter" and "Déconnecter" in various contexts. While grammatically correct, this might confuse users about the difference between these actions.
Run this script to analyze the usage patterns:
design-system/Icon/Icon.android.tsx (2)
94-95: LGTM! Improved code formatting.The indentation changes in the ternary expression enhance code readability while maintaining the same logic.
21-21: Verify if duplicate mapping is necessary.The
pencilicon maps to the same glyph "edit" assquare.and.pencil. While this might be intentional for API compatibility, please verify if both mappings are needed.Run this script to check for usage patterns:
✅ Verification successful
Both icon mappings are necessary and correctly implemented.
The duplicate mapping is intentional and required as both
pencilandsquare.and.pencilare actively used in different parts of the application:
square.and.pencilin header actions (Profile, Conversation List)pencilin profile menu items🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of both icon names to verify if both mappings are necessary echo "Searching for 'pencil' icon usage:" rg -l "icon=[\"\']pencil[\"\']|picto=[\"\']pencil[\"\']" echo -e "\nSearching for 'square.and.pencil' icon usage:" rg -l "icon=[\"\']square\.and\.pencil[\"\']|picto=[\"\']square\.and\.pencil[\"\']"Length of output: 619
Script:
#!/bin/bash # Search for both icon names with proper regex and context echo "Searching for 'pencil' icon usage:" rg -B 2 -A 2 '"pencil"' echo -e "\nSearching for 'square.and.pencil' icon usage:" rg -B 2 -A 2 '"square\.and\.pencil"' # Also search with single quotes echo -e "\nSearching with single quotes:" rg -B 2 -A 2 "'pencil'" rg -B 2 -A 2 "'square\.and\.pencil'"Length of output: 3570
theme/border-radius.ts (1)
2-3: Consider documenting the border radius scale rationale.The introduction of
xxs: 8and change ofxsfrom 8 to 12 creates a more granular scale. However, this change might affect existing components usingxs. Consider:
- Documenting the rationale behind the border radius scale
- Verifying all components using
borderRadius.xshave been updatedcomponents/TransactionPreview/TransactionResult.tsx (1)
35-35: LGTM! Consistent with new border radius scale.The update from
xstoxxsaligns with the new border radius scale.design-system/Table/Table.tsx (1)
18-22: Using label as key might cause issues with duplicate labels.Using
row.labelas key could lead to React reconciliation issues if labels are not unique. Consider adding a unique identifier to the row type or generating a stable key.-{rows.map((row) => ( +{rows.map((row, index) => ( <View - key={row.label} + key={`${row.label}-${index}`} // temporary solution, prefer unique IDs style={themed($sectionContainer)} >Let's check if there are any duplicate labels in the codebase:
✅ Verification successful
Using label as key might cause reconciliation issues with duplicate labels
The review comment is valid. While current usage in the codebase shows labels coming from translations (likely unique), there's no structural enforcement of label uniqueness in the
ITableRowinterface. Using${row.label}-${index}as suggested would prevent potential reconciliation issues, especially since the component might be reused in contexts where labels could be duplicate.
- The current implementation in
Profile.tsxshows dynamic row construction, which could potentially lead to duplicate labels depending on the conditions- The
Tablecomponent's interface (ITableRow[]) doesn't enforce unique labels- React's reconciliation requires stable, unique keys for optimal performance and correct behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TableRow usage with potential duplicate labels rg -A 5 "TableRow" | rg "label:"Length of output: 82
Script:
#!/bin/bash # Find TableRow implementation and related files echo "=== TableRow Implementation ===" ast-grep --pattern 'const TableRow = $_' echo -e "\n=== Table Tests ===" fd "Table.test" --exec cat {} echo -e "\n=== Table Usage Examples ===" rg -l "TableRow" --type typescript --type typescriptreactLength of output: 3162
Script:
#!/bin/bash # Search for Table usage and tests with correct extensions echo "=== Table Tests ===" fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . | grep -i table | xargs cat echo -e "\n=== Table Usage ===" rg "rows.*\[" -A 5 --type-add 'ts:*.{ts,tsx}' --type tsLength of output: 962
design-system/chip.tsx (1)
58-61: LGTM! Visual refinements improve the component's appearance.The changes to text preset, padding, and height create a more polished look while maintaining the component's functionality.
Also applies to: 79-81
features/profiles/components/ContactCard.tsx (2)
105-109: Add error handling for avatar loading failures.The Avatar component should handle loading failures gracefully to prevent blank spaces in the UI.
<Avatar uri={avatarUri} name={displayName} size={theme.avatarSize.lg} + onError={() => console.warn('Failed to load avatar')} + fallbackComponent={() => ( + <View style={{ + width: theme.avatarSize.lg, + height: theme.avatarSize.lg, + backgroundColor: theme.colors.background.subtle, + borderRadius: theme.avatarSize.lg / 2 + }} /> + )} />
15-22: Add TypeScript validation foronToggleEditprop.The
onToggleEditprop should be required whenisMyProfileis true.type IContactCardProps = { displayName: string; userName?: string; avatarUri?: string; isMyProfile?: boolean; editMode?: boolean; - onToggleEdit?: () => void; + onToggleEdit: isMyProfile extends true ? () => void : undefined; };design-system/Button/Button.styles.ts (1)
56-64: LGTM! Clean implementation of the bare link variant.The implementation of the "link.bare" variant is clean and follows the existing pattern, providing a minimal styling option for text-only buttons.
screens/Profile.tsx (1)
100-104: Implement profile edit functionality.The edit functionality is currently blocked with a TODO comment and Alert message.
The profile edit functionality should be implemented instead of showing an alert. Would you like me to help create a GitHub issue to track this implementation?
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
screens/Profile.tsx (2)
144-247: 🛠️ Refactor suggestionExtract header configuration to a custom hook.
The header configuration is complex and contains multiple conditional renders. Consider extracting it to a custom hook like
useProfileHeaderfor better maintainability and reusability.
277-278:⚠️ Potential issueUse specific translation keys for different copy operations.
The same translation key "profile.copied" is used for different types of copied content (username, ENS, domain). Consider using specific keys for better clarity:
- "profile.copied.username"
- "profile.copied.ens"
- "profile.copied.domain"
Also applies to: 289-290, 303-304
🧹 Nitpick comments (5)
i18n/translations/en.ts (1)
356-379: LGTM! Consider adding a confirmation message for the "Save" action.The new translations are well-organized and cover all necessary user interactions. However, consider adding a confirmation message when users save their profile changes, similar to the block confirmation message.
Add a confirmation message for profile changes:
profile: { ... save: "Save", + save_confirmation: "Your profile changes have been saved.", ... },screens/Profile.tsx (4)
1-44: Consider organizing imports by category.Group imports into categories for better maintainability:
- React and React Native
- Design system components
- Navigation
- Hooks
- Utils
- External libraries
Line range hint
85-141: Extract context menu actions into separate handlers.The
handleContextMenuActionfunction handles multiple responsibilities. Consider splitting it into separate handlers for better maintainability:
handleShareActionhandleEditActionhandleBlockAction
264-310: Extract names section to a separate component.The names section has complex conditional rendering logic. Consider extracting it to a
ProfileNamescomponent for better maintainability.
397-399: Replace magic number with theme spacing value.The magic number
4in$editIconstyle should be replaced with a theme spacing value for consistency.- marginBottom: 4, // Centers the square.and.pencil icon + marginBottom: spacing.xxs, // Centers the square.and.pencil icon
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
design-system/TextField/TextField.tsx(1 hunks)i18n/translations/en.ts(2 hunks)i18n/translations/fr.ts(1 hunks)screens/Profile.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- design-system/TextField/TextField.tsx
🔇 Additional comments (1)
i18n/translations/en.ts (1)
179-179: LGTM! Key name follows consistent naming convention.The change from
App settingstoapp_settingsaligns with the snake_case naming convention used throughout the translations.
|
|
||
| function ProfileScreenImpl() { | ||
| const navigation = useRouter(); | ||
| const [editMode, setEditMode] = useState(false); |
There was a problem hiding this comment.
💡 Codebase verification
Uncomment or remove the edit mode implementation
The edit mode functionality is partially implemented:
- The state management and UI components are in place
handleEditProfileis properly connected to UI elements- However, there's a commented out
handleEditProfilecall that needs attention
screens/Profile.tsx: Uncomment// handleEditProfile();or remove it if not needed
🔗 Analysis chain
Implement or remove the edit mode functionality.
The editMode state and handleEditProfile function are set up but blocked by a TODO comment. This could lead to confusion as the UI suggests edit functionality that isn't available.
Also applies to: 75-77, 100-104
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if edit functionality is implemented elsewhere
rg "handleEditProfile|editMode" --type ts --type tsx
Length of output: 82
Script:
#!/bin/bash
# Search for edit-related functionality in TypeScript files
rg -t typescript "handleEditProfile|editMode|edit.*profile"
# Use ast-grep to find state declarations related to edit mode
ast-grep --pattern 'const [$_] = useState($$$)'
# Look for Profile component definition to understand the full context
ast-grep --pattern 'function Profile($$$) {
$$$
}'
Length of output: 201
Script:
#!/bin/bash
# Search for edit-related functionality without file type specification
rg "editMode|handleEditProfile"
# Search for Profile component and surrounding context
rg "function Profile" -A 10
# Find any edit-related UI elements
rg "edit.*profile" -i
Length of output: 4801
| if (variant === "link.bare") { | ||
| return { | ||
| flexDirection: "row", | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| gap: spacing.xxs, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Not 100% sure about this variant and style 🤔
Wonder if we should just have around a for now?
There was a problem hiding this comment.
It's a button with no extra padding/margin, which helps align things. I used i in the ContactCard, but design has changed. Although this will be needed in the future!
…ur profile or someone else's
…e combination - Add text.bare variant to IButtonVariant type - Add special case in getButtonViewStyle for text.bare that removes padding and decorations - Keep existing button variants and their styling intact - Use text.bare variant in Profile screen's Edit button This allows for buttons without padding while maintaining the existing button system.
- Add Done button in header when in edit mode - Fix edit mode toggle functionality - Update context menu items with proper type safety - Remove copy address option from other users' profiles - Remove 0x wallet address from share own profile view
- Rename FullWidthTable to Table for better naming - Extract TableRow into separate component - Move shared types to Table.types.ts - Remove theme prop drilling by using useAppTheme hook directly in TableRow
lourou
force-pushed
the
lr/profile-screen
branch
from
2f64bf6 to
f9bdac9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
features/profiles/components/contact-card.tsx (1)
111-122:⚠️ Potential issueImplement or remove the edit button.
The edit button implementation is commented out with no clear indication of when it will be implemented.
screens/Profile.tsx (1)
99-103:⚠️ Potential issueTODO comment blocks critical functionality.
The edit profile functionality is blocked by a TODO comment and shows an alert instead of implementing the feature.
🧹 Nitpick comments (10)
screens/Navigation/ProfileNav.tsx (1)
16-16: Consider restoring navigation options for consistent UX.The simplified implementation removes all navigation options. Consider adding basic header configuration for consistent navigation experience:
- return <NativeStack.Screen name="Profile" component={ProfileScreen} />; + return ( + <NativeStack.Screen + name="Profile" + component={ProfileScreen} + options={{ + headerShown: true, + headerBackTitleVisible: false, + }} + /> + );features/profiles/utils/format-converse-username.ts (2)
14-17: Add input validation for empty strings.The function returns undefined for falsy values but should explicitly handle empty strings at the start.
export function formatConverseUsername( username: string | undefined ): IUsernameResult | undefined { - if (!username) return undefined; + if (!username?.trim()) return undefined;
19-27: Consider caching the config value.
getConfig()is called on every function execution. Consider caching the usernameSuffix value if this function is called frequently.+const usernameSuffix = getConfig().usernameSuffix; + export function formatConverseUsername( username: string | undefined ): IUsernameResult | undefined { if (!username?.trim()) return undefined; // Check if it's a Converse username (either domain) - if (username.endsWith(getConfig().usernameSuffix)) { + if (username.endsWith(usernameSuffix)) { // Extract everything before the domain - const cleanUsername = username.replace(getConfig().usernameSuffix, ""); + const cleanUsername = username.replace(usernameSuffix, ""); return { isConverseUsername: true, username: `@${cleanUsername}`, }; }features/profiles/utils/__tests__/format-converse-username.test.ts (1)
4-39: Add tests for additional edge cases.Consider adding test cases for:
- Usernames with multiple occurrences of the suffix
- Usernames with special characters
- Very long usernames
Example additional test cases:
it("should handle usernames with multiple suffixes", () => { const suffix = getConfig().usernameSuffix; expect(formatConverseUsername(`test${suffix}${suffix}`)).toEqual({ isConverseUsername: true, username: "@test", }); }); it("should handle usernames with special characters", () => { const suffix = getConfig().usernameSuffix; expect(formatConverseUsername(`test.123-_${suffix}`)).toEqual({ isConverseUsername: true, username: "@test.123-_", }); });design-system/settings-list/settings-list-row.tsx (1)
15-61: Consider adding accessibility labels.The component handles user interaction well, but could benefit from accessibility improvements.
<TouchableOpacity style={themed($rowContainer)} onPress={row.onPress} + accessibilityRole="button" + accessibilityLabel={row.label} >features/profiles/components/social-names.tsx (1)
36-39: Consider using a custom hook for clipboard operations.The clipboard operation with alert feedback is used in multiple places. Consider extracting it to a reusable hook.
// useClipboard.ts export const useClipboard = () => { const copyToClipboard = (text: string, alertMessage: string) => { Clipboard.setString(text); Alert.alert(translate(alertMessage)); }; return { copyToClipboard }; };screens/Profile.tsx (2)
280-287: Consider adding loading state for permissions.The notifications permission UI should handle loading state while permission is being requested.
{ label: translate("turn_on_notifications"), + disabled: isRequestingPermission, onPress: requestPermission, }
294-298: Remove or implement commented code.The commented out settings row for message retention should either be implemented or removed.
i18n/translations/fr.ts (1)
375-376: Consider adding error details to the download error message.The download error message is quite generic. Consider adding more context or possible solutions.
- "Impossible de télécharger la pièce jointe", + "Impossible de télécharger la pièce jointe. Vérifiez votre connexion internet ou réessayez plus tard.",features/notifications/hooks/use-notifications-permission.ts (1)
25-44: Consider adding error handling and return type for requestPermission.The implementation looks good, but a few improvements could be made:
- Add error handling for
requestPushNotificationsPermissionscalls- Add explicit return type for
requestPermission- Consider adding a comment explaining why Android 13 requires special handling
- const requestPermission = async () => { + const requestPermission = async (): Promise<void> => { if (notificationsPermissionStatus === "denied") { if (Platform.OS === "android") { - // Android 13 is always denied first so let's try to show + // On Android 13+, the initial permission check always returns "denied" + // so we need to explicitly request permissions const newStatus = await requestPushNotificationsPermissions(); - if (newStatus === "denied") { - Linking.openSettings(); - } else if (newStatus) { - setNotificationsPermissionStatus(newStatus); + try { + if (newStatus === "denied") { + Linking.openSettings(); + } else if (newStatus) { + setNotificationsPermissionStatus(newStatus); + } + } catch (error) { + console.error('Failed to handle notification permission:', error); + throw error; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
components/TransactionPreview/TransactionResult.tsx(1 hunks)components/TransactionPreview/TransactionSimulationFailure.tsx(1 hunks)design-system/Button/Button.props.ts(1 hunks)design-system/Button/Button.styles.ts(1 hunks)design-system/Icon/Icon.android.tsx(3 hunks)design-system/Icon/Icon.tsx(3 hunks)design-system/Icon/Icon.types.ts(2 hunks)design-system/TextField/TextField.tsx(1 hunks)design-system/chip.tsx(2 hunks)design-system/settings-list/settings-list-row.tsx(1 hunks)design-system/settings-list/settings-list.tsx(1 hunks)design-system/settings-list/settings-list.types.ts(1 hunks)features/ExternalWalletPicker/ExternalWalletPicker.tsx(1 hunks)features/conversation-list/conversation-list.screen-header.tsx(1 hunks)features/conversation/conversation-composer/conversation-composer-reply-preview.tsx(2 hunks)features/conversation/conversation.nav.tsx(2 hunks)features/notifications/hooks/use-notifications-permission.ts(1 hunks)features/profiles/components/contact-card.tsx(1 hunks)features/profiles/components/social-names.tsx(1 hunks)features/profiles/utils/__tests__/format-converse-username.test.ts(1 hunks)features/profiles/utils/format-converse-username.ts(1 hunks)hooks/useDisconnectActionSheet.ts(1 hunks)i18n/translations/en.ts(2 hunks)i18n/translations/fr.ts(1 hunks)screens/Navigation/ProfileNav.tsx(2 hunks)screens/Navigation/UserProfileNav.tsx(0 hunks)screens/NotificationsScreen.tsx(2 hunks)screens/Onboarding/OnboardingNotificationsScreen.tsx(2 hunks)screens/Profile.tsx(2 hunks)screens/ShareProfile.tsx(0 hunks)screens/UserProfileScreen.tsx(1 hunks)theme/border-radius.ts(1 hunks)
💤 Files with no reviewable changes (2)
- screens/Navigation/UserProfileNav.tsx
- screens/ShareProfile.tsx
🚧 Files skipped from review as they are similar to previous changes (17)
- design-system/Button/Button.props.ts
- design-system/TextField/TextField.tsx
- features/ExternalWalletPicker/ExternalWalletPicker.tsx
- components/TransactionPreview/TransactionSimulationFailure.tsx
- hooks/useDisconnectActionSheet.ts
- screens/UserProfileScreen.tsx
- features/conversation-list/conversation-list.screen-header.tsx
- design-system/Icon/Icon.types.ts
- theme/border-radius.ts
- features/conversation/conversation-composer/conversation-composer-reply-preview.tsx
- components/TransactionPreview/TransactionResult.tsx
- design-system/Icon/Icon.android.tsx
- design-system/chip.tsx
- design-system/Button/Button.styles.ts
- design-system/Icon/Icon.tsx
- features/conversation/conversation.nav.tsx
- i18n/translations/en.ts
🔇 Additional comments (17)
screens/Navigation/ProfileNav.tsx (2)
16-16: Consider using useHeader hook as suggested.As mentioned in a previous review, consider using the
useHeaderhook for consistent header styling across the app.
16-16: Verify the read-only implementation.Since this PR implements a read-only profile screen, ensure that the navigation setup properly reflects this requirement and prevents any editing capabilities.
design-system/settings-list/settings-list.types.ts (1)
1-9: Consider adding validation for value type.The
valueproperty accepts bothstringandboolean, which could lead to type mismatches when used withonValueChangethat only accepts boolean. Consider splitting into separate properties or adding runtime validation.export type ISettingsListRow = { label: string; - value?: string | boolean; + stringValue?: string; + booleanValue?: boolean; onPress?: () => void; onValueChange?: (value: boolean) => void; isWarning?: boolean; isSwitch?: boolean; disabled?: boolean; };design-system/settings-list/settings-list-row.tsx (2)
10-13: LGTM! Props interface is well-defined.The
ISettingsListRowPropstype is clear and focused, with good use of optional properties.
63-80: LGTM! Well-structured themed styles.The styles are properly typed with ThemedStyle and follow consistent spacing patterns.
features/profiles/components/social-names.tsx (2)
11-14: LGTM! Clear type definition.The
ISocialNametype is well-structured and handles optional domain property appropriately.
63-67: LGTM! Proper filtering of .eth domains.The filtering of .eth domains from unstoppableDomains is correct as these are already displayed through ensNames.
features/profiles/components/contact-card.tsx (2)
15-22: LGTM! Well-defined props interface.The
IContactCardPropstype clearly defines all required and optional properties.
73-91: LGTM! Smooth animation implementation.The pan gesture handler is well-implemented with proper spring animations for reset.
screens/Profile.tsx (1)
149-151: LGTM! Safe route access.The code properly checks for route existence before accessing the route name.
i18n/translations/fr.ts (3)
350-364: LGTM! Profile block and settings translations are well-structured.The translations for blocking users and profile settings are properly organized and accurately translated to French.
357-357: Remove duplicate translation keydone.The translation key
doneappears twice:
- Within the
profileobject asdone: "Valider"(line 357)- At the root level as
done: "Valider"(appears in the diff but was removed as shown in the past review)Also applies to: 376-376
366-374: Verify consistency of disconnect-related translations.The translations use both "Se déconnecter" and "Déconnecter" for logout/disconnect actions. Consider using consistent terminology throughout the application.
Run this script to check for consistency:
features/notifications/hooks/use-notifications-permission.ts (1)
6-12: LGTM! Well-defined type for the hook return value.The type definition is clear and properly encapsulates all necessary functionality.
screens/NotificationsScreen.tsx (1)
11-12: LGTM! Clean migration to the new notifications permission hook.The implementation is cleaner and more maintainable with the new hook.
Also applies to: 29-30
screens/Onboarding/OnboardingNotificationsScreen.tsx (2)
20-21: LGTM! Good error handling with Sentry integration.The implementation properly handles errors and maintains the existing functionality.
Also applies to: 61-67
1-1: Verify PR scope and title alignment.The PR is titled "New Profile Screen (read-only)" but the changes are focused on notification permission handling. This seems to be a discrepancy that needs clarification.
| {rows.map((row, index) => ( | ||
| <SettingsListRow | ||
| key={row.label + index} | ||
| row={row} | ||
| editMode={editMode} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using array index in key prop.
Using array index as part of the key prop can lead to issues with component reordering and state preservation. Consider using a unique identifier from the row data instead.
{rows.map((row, index) => (
<SettingsListRow
- key={row.label + index}
+ key={row.id || row.label} // Add an id property to ISettingsListRow
row={row}
editMode={editMode}
/>
))}Committable suggestion skipped: line range outside the PR's diff.
| import { Platform, Linking } from "react-native"; | ||
| import { useAppStore } from "@/data/store/appStore"; | ||
| import { requestPushNotificationsPermissions } from "../utils/requestPushNotificationsPermissions"; | ||
| import { useSettingsStore } from "@/data/store/accountsStore"; | ||
|
|
||
| type UseNotificationsPermissionReturn = { | ||
| notificationsPermissionStatus: "granted" | "undetermined" | "denied"; | ||
| requestPermission: () => Promise<void>; | ||
| setNotificationsSettings: (settings: { | ||
| showNotificationScreen: boolean; | ||
| }) => void; | ||
| }; | ||
|
|
||
| export function useNotificationsPermission(): UseNotificationsPermissionReturn { | ||
| const notificationsPermissionStatus = useAppStore( | ||
| (s) => s.notificationsPermissionStatus | ||
| ); | ||
| const setNotificationsPermissionStatus = useAppStore( | ||
| (s) => s.setNotificationsPermissionStatus | ||
| ); | ||
| const setNotificationsSettings = useSettingsStore( | ||
| (s) => s.setNotificationsSettings | ||
| ); | ||
|
|
||
| const requestPermission = async () => { | ||
| if (notificationsPermissionStatus === "denied") { | ||
| if (Platform.OS === "android") { | ||
| // Android 13 is always denied first so let's try to show | ||
| const newStatus = await requestPushNotificationsPermissions(); | ||
| if (newStatus === "denied") { | ||
| Linking.openSettings(); | ||
| } else if (newStatus) { | ||
| setNotificationsPermissionStatus(newStatus); | ||
| } | ||
| } else { | ||
| Linking.openSettings(); | ||
| } | ||
| } else if (notificationsPermissionStatus === "undetermined") { | ||
| // Open popup | ||
| const newStatus = await requestPushNotificationsPermissions(); | ||
| if (!newStatus) return; | ||
| setNotificationsPermissionStatus(newStatus); | ||
| } | ||
| }; | ||
|
|
||
| return { | ||
| notificationsPermissionStatus, | ||
| requestPermission, | ||
| setNotificationsSettings, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Migration needed for notification permission handling
The new hook is properly used in UI components (Profile, NotificationsScreen, OnboardingNotificationsScreen) but several files still use direct permission management:
components/AccountSettingsButton.tsx: Direct usage of permission request and status managementfeatures/notifications/utils/subscribeToNotifications.ts: Direct permission requestfeatures/notifications/utils/saveNotificationsStatus.ts: Direct status management
These files should be refactored to use the new hook for consistent permission handling across the codebase.
🔗 Analysis chain
Verify the impact of this new hook across the codebase.
Let's ensure all notification permission-related code is migrated to use this new hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of notification permission-related functions
rg "requestPushNotificationsPermissions|setNotificationsPermissionStatus|setNotificationsSettings" --type ts
Length of output: 3684
Script:
#!/bin/bash
# Check Profile.tsx implementation
rg "useNotificationsPermission|setNotificationsSettings" -B 2 -A 2 screens/Profile.tsx
Length of output: 586
| marginBottom: theme.spacing.lg, | ||
| shadowColor: theme.colors.fill.primary, | ||
| shadowOpacity: 0.25, | ||
| shadowRadius: 12, |
There was a problem hiding this comment.
Should this be put into theme/borderradius theme?
There was a problem hiding this comment.
Yes! I'll add this in the follow up profile refactor PR
Linked to #1500
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the release notes:
New Features
Improvements
Design System Updates
Localization
Performance
Screenshot of the read-only implementation