BA-1809 [FE] Simplify CurrentProfileProvider #140
Conversation
WalkthroughThe changes in this pull request primarily involve restructuring user profile data management across various components and hooks. The Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/authentication/types/user.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
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: 8
🧹 Outside diff range and nitpick comments (10)
packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx (1)
Line range hint
1-17: Consider adding test cases for edge scenarios.While the component correctly tests the happy path, consider adding test cases for:
- Initial loading state
- Error scenarios
- Profile update scenarios
Example test cases to consider:
// Loading state it('should handle loading state', () => { // Mock hook to return undefined initially const { container } = render(<TestComponent />) expect(container).toBeEmptyDOMElement() }) // Error scenario it('should handle error states', () => { // Mock hook to simulate error // Verify error handling behavior }) // Profile updates it('should reflect profile updates', async () => { // Mock hook with initial profile // Trigger profile update // Verify new profile is rendered })packages/authentication/types/user.ts (1)
13-28: Consider adding type guard utilities for nullable fieldsThe
MinimalProfileinterface has multiple nullable fields (name,image,urlPath) which could lead to runtime errors if not properly handled.Consider adding type guard utilities:
export const hasProfileImage = (profile: MinimalProfile): profile is MinimalProfile & { image: { url: string } } => { return !!profile.image?.url; }; export const hasProfileUrlPath = (profile: MinimalProfile): profile is MinimalProfile & { urlPath: { path: string } } => { return !!profile.urlPath?.path; };packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx (1)
10-10: Consider using object destructuring for cleaner codeThe current pattern of accessing
currentProfilecould be simplified using object destructuring.- const profile = useCurrentProfile().currentProfile + const { currentProfile: profile } = useCurrentProfile()packages/components/modules/profiles/useCurrentProfile/index.ts (3)
19-24: Consider improving type safety and function naming.The function could benefit from explicit return type annotation and a more specific name to indicate it's profile-specific.
-const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { +const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}): MinimalProfile | undefined => {
55-60: Consider adding error boundary for event handling.The event listener cleanup might not run if there's an error during profile removal.
useEffect(() => { + const handleLogout = () => { + try { + removeCurrentProfile(); + } catch (error) { + console.error('Error during profile removal:', error); + } + }; - eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile) + eventEmitter.on(LOGOUT_EVENT, handleLogout) return () => { - eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile) + eventEmitter.off(LOGOUT_EVENT, handleLogout) } }, [])
1-76: Consider adding integration tests and migration guide.As this is part of a larger migration from Zustand to Jotai:
- Add integration tests covering both SSR and CSR scenarios
- Document the migration pattern for other teams
- Consider adding feature flags for gradual rollout
This will help ensure a smooth transition and maintain backward compatibility during the migration.
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (1)
25-30: Consider adding error handling for profile switching.While the implementation includes good UX features (validation, feedback, and UI updates), it would be beneficial to add error handling for the profile switch operation.
Consider wrapping the operation in a try-catch block:
const handleProfileChange = (profile: ProfileItemFragment$data) => { if (currentProfile?.id !== profile.id) { - setCurrentProfile(profile) - sendToast(`Switched to ${profile.name}`) - handleCloseSubmenu() + try { + setCurrentProfile(profile) + sendToast(`Switched to ${profile.name}`) + handleCloseSubmenu() + } catch (error) { + sendToast('Failed to switch profile. Please try again.', { type: 'error' }) + } } }packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx (1)
69-69: Consider adding a default avatar for empty stateThe current implementation defaults to an empty string when no profile image is available. Consider providing a default avatar image for better user experience.
- src={profile?.image?.url ?? ''} + src={profile?.image?.url ?? '/assets/default-avatar.png'}packages/components/modules/messages/SendMessage/index.tsx (2)
Line range hint
97-107: Improve error handling implementation.The TODO comments indicate incomplete error handling. Consider implementing proper error handling to improve user experience:
- Surface errors to the user through a toast or error message component
- Implement retry logic for failed mutations
- Log errors to an error tracking service instead of console
Here's a suggested implementation:
onCompleted: (response, errors) => { if (errors) { - // TODO: handle errors - console.error(errors) + toast.error('Failed to send message. Please try again.') + captureException(errors) } const mutationErrors = response?.chatRoomSendMessage?.errors setFormRelayErrors(form, mutationErrors) if (!mutationErrors?.length) { form.reset() } }, - // TODO: handle errors - onError: console.error, + onError: (error) => { + toast.error('Failed to send message. Please try again.') + captureException(error) + },
Line range hint
85-94: Strengthen null checks for currentProfile.While there's an early return for null
currentProfile, the code still uses optional chaining (currentProfile?.id) in the mutation variables. Consider adding stronger type guards:const onSubmit = (data: SocialUpsertForm) => { - if (isMutationInFlight || !currentProfile) return + if (isMutationInFlight) return + if (!currentProfile?.id) { + toast.error('Profile not found. Please try again.') + return + } nextClientMutationId += 1 const clientMutationId = nextClientMutationId.toString() commitMutation({ variables: { input: { content: data.body, - profileId: currentProfile?.id, + profileId: currentProfile.id, roomId, clientMutationId, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
packages/authentication/types/user.ts(1 hunks)packages/components/modules/messages/MessageItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesGroup/index.tsx(1 hunks)packages/components/modules/messages/SendMessage/index.tsx(1 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx(2 hunks)packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx(2 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts(2 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/constants.ts(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/index.tsx(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/types.ts(0 hunks)packages/components/modules/profiles/context/useCurrentProfile/index.tsx(0 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx(1 hunks)packages/components/modules/profiles/useCurrentProfile/constants.ts(1 hunks)packages/components/modules/profiles/useCurrentProfile/index.ts(1 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (4)
- packages/components/modules/profiles/context/CurrentProfileProvider/constants.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/index.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/types.ts
- packages/components/modules/profiles/context/useCurrentProfile/index.tsx
✅ Files skipped from review due to trivial changes (3)
- packages/components/modules/profiles/index.ts
- packages/components/modules/profiles/useCurrentProfile/tests/CurrentProfileProvider.test.tsx
- packages/components/modules/profiles/useCurrentProfile/constants.ts
🔇 Additional comments (15)
packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx (1)
1-1: LGTM! Verify test coverage for the updated hook API.
The changes correctly reflect the new useCurrentProfile hook API. The component maintains its testing utility while adapting to the new structure.
Let's verify the test coverage for the new hook API:
Also applies to: 4-4
✅ Verification successful
Test coverage looks comprehensive for the updated hook API
The test suite demonstrates thorough coverage including:
- Profile retrieval from local storage
- User authentication scenarios
- Profile clearing on logout
- Error handling for failed profile fetches
- Cross-user profile isolation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for comprehensive test coverage of the useCurrentProfile hook
# Look for test cases covering different scenarios
# Search for test files related to useCurrentProfile
echo "Searching for related test files..."
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | rg -i "currentprofile"
# Look for test cases covering edge cases
echo "Checking test coverage for edge cases..."
rg -A 5 "describe|it|test" $(fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | rg -i "currentprofile")
Length of output: 3628
packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts (1)
13-14: Consider type alignment between onProfileChange and currentProfile.
There's a potential type mismatch between:
onProfileChangecallback expectingProfileItemFragment$datacurrentProfileusingMinimalProfile
This could lead to type conversion issues when handling profile changes.
Let's verify if there are any type conversion handlers in place:
packages/components/modules/messages/MessageItem/index.tsx (2)
12-12: LGTM! Verify consistent usage across message components.
The simplified destructuring of useCurrentProfile hook aligns well with the broader refactoring goals.
Let's verify the consistent usage of useCurrentProfile across other message-related components:
✅ Verification successful
Consistent usage of useCurrentProfile hook verified across message components
The verification shows that all message-related components (MessageItem, SendMessage, and MessagesGroup) consistently use the same destructuring pattern const { currentProfile } = useCurrentProfile(), which aligns with the refactoring goals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of useCurrentProfile hook in message components
# Expect: All message components should use the same destructuring pattern
ast-grep --pattern 'const { currentProfile } = useCurrentProfile()' packages/components/modules/messages/
Length of output: 444
12-14: Verify SSR functionality with new profile management
Since the PR introduces cookie-based profile storage for SSR, we should ensure the profile comparison logic works correctly during server-side rendering.
Let's check for proper SSR handling:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the SSR functionality with profile management:
SSR handling is properly implemented in the profile management system
The codebase shows that:
- The
useCurrentProfilehook has built-in SSR support with anoSSRoption that controls SSR behavior - It properly detects SSR environment with
typeof window === typeof undefinedcheck - The hook is used with
noSSR: falsein critical navigation components (AccountPopover) - Profile data is managed through cookies (via
setCookie(PROFILE_KEY, newProfile)) which works in both client and server environments
The implementation in MessageItem is using the default SSR-safe configuration of the hook, so no changes are needed for proper SSR functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SSR-related code for profile management
# Expect: Proper handling of profile data during SSR
# Check for SSR-specific profile handling
rg -A 5 'getServerSideProps|getInitialProps' packages/components/modules/messages/
# Check for cookie-related profile management
rg -A 5 'cookie.*profile|profile.*cookie' packages/components/modules/messages/
Length of output: 166
Script:
#!/bin/bash
# Let's check the implementation of useCurrentProfile hook and its SSR handling
ast-grep --pattern 'export function useCurrentProfile'
# Check for any SSR-related code in the messages module
rg -l 'useEffect|useState|useMemo' packages/components/modules/messages/
# Look for cookie handling in the profile management
rg -l 'cookie|localStorage' --type typescript packages/components/
Length of output: 447
Script:
#!/bin/bash
# Let's check the implementation of useCurrentProfile hook in the codebase
rg -A 10 "useCurrentProfile" --type ts --type tsx
# Check for any cookie or profile related utilities
fd -e ts -e tsx | xargs rg "cookie.*profile|profile.*cookie"
# Look for SSR-specific code in the profile management
fd -e ts -e tsx | xargs rg "typeof window.*profile|profile.*typeof window"
Length of output: 217
Script:
#!/bin/bash
# Search for useCurrentProfile implementation and usage
rg -A 10 "useCurrentProfile"
# Look for profile-related hooks or context providers
rg -A 5 "ProfileProvider|ProfileContext"
# Check for any SSR-specific code in profile management
rg -A 5 "typeof window !== 'undefined'.*profile"
Length of output: 21747
packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx (1)
7-7: Verify consistent hook import paths across the codebase
The import path change aligns with the transition from context-based to Jotai-based state management.
✅ Verification successful
Import paths for useCurrentProfile are consistent across the codebase
All files are using the new import path consistently:
packages/components/modules/profiles/index.tsexports from './useCurrentProfile'ProfilesList/index.tsximports from '../../useCurrentProfile'CurrentProfile/index.tsximports from '../../useCurrentProfile'
No instances of the old context-based import path remain, and the hook is properly exported from its new location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all imports of useCurrentProfile are consistent
# and no old context-based imports remain
# Check for any remaining context-based imports
rg -l "from.*context/useCurrentProfile"
# Check current usage pattern
rg -l "from.*useCurrentProfile"
Length of output: 273
Script:
#!/bin/bash
# Let's check the actual import statements in these files to verify consistency
echo "=== Checking imports in modules/profiles/index.ts ==="
rg "from.*useCurrentProfile" packages/components/modules/profiles/index.ts -A 1 -B 1
echo -e "\n=== Checking imports in ProfilesList/index.tsx ==="
rg "from.*useCurrentProfile" packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx -A 1 -B 1
echo -e "\n=== Checking imports in CurrentProfile/index.tsx ==="
rg "from.*useCurrentProfile" packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx -A 1 -B 1
# Also check if useCurrentProfile is exported from the new location
echo -e "\n=== Checking useCurrentProfile exports ==="
rg "export.*useCurrentProfile" packages/components/modules/profiles/
Length of output: 1376
packages/components/modules/profiles/useCurrentProfile/index.ts (2)
1-18: LGTM! Imports and directives are properly organized.
The 'use client' directive and imports are correctly structured for a Next.js client component.
62-74: Verify SSR behavior across different environments.
The conditional return based on SSR needs thorough testing across different Next.js environments and deployment scenarios.
✅ Verification successful
Let me gather more information about the SSR implementation and profile management.
Based on the comprehensive analysis of the codebase, I can now provide a final response about the SSR behavior verification:
SSR implementation is properly handled and tested across environments
The SSR behavior in useCurrentProfile is well-implemented and follows established patterns:
- Uses a consistent SSR detection method (
typeof window === 'undefined') - Properly handles cookie access in both server and client environments through the
getCookieutility - Has comprehensive test coverage for both SSR and client-side scenarios
- Integrates with Next.js's cookie handling through
next/headers - Follows the same pattern as other SSR-aware hooks in the codebase (e.g.,
useJWTUser,useSSR)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other SSR-sensitive hooks and components
rg -l "typeof window === 'undefined'" --type ts
# Look for potential SSR-related issues in other profile management code
ast-grep --pattern 'useEffect($$$)' | rg -A 5 'profile|cookie'
Length of output: 1100
Script:
#!/bin/bash
# Check the implementation of getProfileFromCookie and related functions
ast-grep --pattern 'function getProfileFromCookie' -A 20
# Look for other SSR checks in the codebase
rg "isSSR|noSSR" --type ts -A 3
# Check for cookie-related implementations
rg "cookie" --type ts -A 3 -B 3
Length of output: 65782
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (1)
23-23: LGTM! Clean and intuitive hook usage.
The simplified destructuring of useCurrentProfile makes the API more straightforward and matches the new Jotai-based implementation.
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx (1)
36-36: Verify SSR behavior with noSSR flag
The noSSR: false flag indicates that this component supports server-side rendering. Ensure that the profile cookie is properly accessible during SSR.
✅ Verification successful
SSR behavior is correctly implemented
The noSSR: false flag in AccountPopover is correctly used. The hook's implementation shows:
- Default
noSSRis true, preventing SSR by default - When
noSSR: false, it enables dynamic rendering as documented - Proper cookie handling during SSR via
getProfileFromCookie - Consistent SSR detection using
typeof window === typeof undefined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using useCurrentProfile to verify consistent SSR handling
rg "useCurrentProfile.*noSSR" --type typescript
Length of output: 84
Script:
#!/bin/bash
# Search for useCurrentProfile usage patterns with proper file type
rg "useCurrentProfile.*noSSR"
# Also search for the hook definition to understand the SSR behavior
ast-grep --pattern 'export function useCurrentProfile($_) {
$$$
}'
Length of output: 632
Script:
#!/bin/bash
# Let's check the implementation details of useCurrentProfile hook
cat packages/components/modules/profiles/useCurrentProfile/index.ts
# Also search for other components that might be using this hook
rg "useCurrentProfile" -B2 -A2
Length of output: 10492
packages/components/modules/messages/SendMessage/index.tsx (1)
78-78: LGTM! Profile hook usage aligns with the new pattern.
The updated destructuring of useCurrentProfile hook aligns with the broader refactoring effort to simplify profile data management.
packages/components/modules/messages/MessagesGroup/index.tsx (2)
30-30: LGTM! Verify consistent hook usage across the codebase.
The change from destructuring profile to currentProfile aligns with the PR's objective of simplifying profile management.
Let's verify that this change is consistent across other components:
✅ Verification successful
Hook usage is consistent across the codebase
The verification shows that all components using useCurrentProfile hook consistently destructure currentProfile. The pattern is used in:
- MessageItem component
- MessagesGroup component
- SendMessage component
No instances of the old profile destructuring pattern were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old useCurrentProfile hook usage patterns
# that might have been missed during the refactor
# Search for old pattern
echo "Checking for old profile destructuring pattern..."
rg "const \{ profile \} = useCurrentProfile\(\)"
# Search for new pattern to confirm consistent usage
echo "Checking new currentProfile destructuring pattern..."
rg "const \{ currentProfile \} = useCurrentProfile\(\)"
Length of output: 660
30-30: Verify profile type compatibility.
The component's usage of profile properties (id, image.url, name) should be compatible with the new MinimalProfile type. The optional chaining provides good safety.
Let's verify the profile type definition:
Also applies to: 106-112, 119-121
✅ Verification successful
Let me try a different approach to find these type definitions.
Let me check how the profile is being used in the MessagesGroup component to compare with the MinimalProfile interface we found.
Profile type usage is compatible with MinimalProfile interface
The component only uses profile.id which is present in the MinimalProfile interface found in packages/authentication/types/user.ts. The optional chaining in message?.profile?.id provides safe access, and the field matches the id: string definition in MinimalProfile.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the MinimalProfile type definition to ensure
# it includes all required fields used in this component
# Search for MinimalProfile type definition
echo "Checking MinimalProfile type definition..."
ast-grep --pattern 'type MinimalProfile = {
$$$
}'
# Search for Profile type definition
echo "Checking Profile type definition..."
ast-grep --pattern 'type Profile = {
$$$
}'
Length of output: 278
Script:
#!/bin/bash
# Search for files containing MinimalProfile or Profile type definitions
echo "Searching for files with profile type definitions..."
rg -l "type (MinimalProfile|Profile) = "
# Once we find the files, let's examine their content
echo -e "\nSearching for profile-related type definitions..."
rg "type (MinimalProfile|Profile) = " -A 10
# Also search for interface definitions
echo -e "\nSearching for profile interfaces..."
rg "interface (MinimalProfile|Profile) " -A 10
Length of output: 1044
Script:
#!/bin/bash
# Check how profile properties are accessed in MessagesGroup
echo "Checking profile usage in MessagesGroup..."
rg "profile\." "packages/components/modules/messages/MessagesGroup/index.tsx" -B 2 -A 2
# Also check for any profile-related props or types in the component
echo -e "\nChecking profile-related types in MessagesGroup..."
rg "Profile" "packages/components/modules/messages/MessagesGroup/index.tsx" -B 2 -A 2
Length of output: 859
packages/components/package.json (3)
32-32: LGTM on Jotai addition with version management.
The addition of Jotai using the "catalog:" version specifier maintains consistency with other dependencies.
32-32: Verify the coexistence of Jotai and Zustand.
Both state management libraries (Jotai and Zustand) are present in the dependencies. If this is part of a gradual migration strategy, this is fine. However, if the migration is complete, consider removing Zustand.
Also applies to: 40-40
✅ Verification successful
Let me check the actual usage of both libraries to better understand if they serve different purposes or if there's an ongoing migration.
Both libraries serve different purposes in the codebase - coexistence is justified
The investigation shows that Zustand and Jotai are used for different purposes:
- Zustand is specifically used for managing notification state and comment reply functionality
- Jotai is used for different concerns: logo overrides with storage persistence and profile management
Since both libraries are actively used for distinct features and there's no sign of an ongoing migration between them, their coexistence is justified and intentional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Zustand is still being used in the codebase
# Search for Zustand imports and usage
echo "Checking for Zustand usage..."
rg -l "from ['\"]zustand"
# Search for Jotai usage to confirm migration progress
echo "Checking for Jotai usage..."
rg -l "from ['\"]jotai"
Length of output: 504
Script:
#!/bin/bash
# Check the specific usage of Zustand
echo "Zustand usage details:"
rg "from ['\"]zustand" -A 5
echo "\nJotai usage details:"
rg "from ['\"]jotai" -A 5
Length of output: 3270
32-32: Verify cookie implementation dependencies.
The presence of both jotai and js-cookie aligns with the PR objective of implementing cookie storage for SSR support of the current profile.
Also applies to: 33-33
✅ Verification successful
Cookie implementation for profile storage is properly implemented
The codebase shows a well-structured implementation of profile storage using cookies with proper SSR support:
- Uses
jotaifor state management withprofileAtomanduseAtomhook - Implements cookie storage through the
js-cookiepackage with proper get/set/remove operations - Has SSR-compatible cookie operations with
noSSRoption - Includes JSON serialization support for complex profile objects
- Defines a clear constant
PROFILE_KEY = 'myCurrentProfile'for cookie naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the cookie implementation
# Search for cookie usage in profile-related code
echo "Checking cookie usage in profile-related code..."
rg -A 5 "js-cookie.*profile"
# Look for SSR-related cookie handling
echo "Checking SSR-related cookie handling..."
fd -e ts -e tsx | xargs rg -l "getServerSideProps.*cookie"
Length of output: 287
Script:
#!/bin/bash
# Let's try a broader search for cookie-related implementations
# Search for js-cookie imports and usage
echo "Checking js-cookie imports and usage..."
rg "import.*js-cookie"
rg "Cookies\.(get|set|remove)"
# Search for cookie-related code in the codebase
echo -e "\nChecking general cookie handling..."
rg -i "cookie" -g "*.{ts,tsx,js,jsx}"
# Search for profile-related state management with jotai
echo -e "\nChecking profile state management..."
rg "atom.*profile" -g "*.{ts,tsx}"
Length of output: 37268
Script:
#!/bin/bash
# Let's check for profile-related state management and cookie implementations
# Search for profile-related atoms or state management
echo "Checking profile state management..."
rg "atom.*profile|useAtom.*profile" -g "*.{ts,tsx}"
# Search for profile cookie constants and implementations
echo -e "\nChecking profile cookie implementations..."
rg "PROFILE_KEY|profile.*cookie" -g "*.{ts,tsx}"
# Check for SSR-related profile handling
echo -e "\nChecking SSR profile handling..."
rg "getServerSideProps.*profile|getStaticProps.*profile" -g "*.{ts,tsx}"
Length of output: 1169
packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/useCurrentProfile/index.ts
Outdated
Show resolved
Hide resolved
| */ | ||
| const useCurrentProfile = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | ||
| const [currentProfile, setProfile] = useAtom(profileAtom) | ||
| const isSSR = typeof window === typeof undefined |
There was a problem hiding this comment.
Fix incorrect SSR detection.
The current SSR detection is incorrect. typeof undefined will always be "undefined" as a string.
-const isSSR = typeof window === typeof undefined
+const isSSR = typeof window === 'undefined'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isSSR = typeof window === typeof undefined | |
| const isSSR = typeof window === 'undefined' |
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/authentication/types/user.ts (1)
13-28: Consider moving and simplifying the MinimalProfile interface.Regarding the location question: Moving this to
utils/typesmakes sense as it's a shared type that other packages might need. Theauthenticationpackage should focus on auth-specific types.The interface could be simplified to improve readability:
export interface MinimalProfile { id: string name: string | null | undefined - image: - | { - url: string - } - | null - | undefined + image: { url: string } | null | undefined - urlPath: - | { - path: string - } - | null - | undefined + urlPath: { path: string } | null | undefined }
| data: Partial<Omit<TUser, 'id'>> & { | ||
| avatar?: File | string | ||
| } |
There was a problem hiding this comment.
Resolve inconsistency between User and UserUpdateParams interfaces.
The avatar field in UserUpdateParams.data seems inconsistent with the new User interface structure where avatar-related data is now part of the profile.image. Consider updating this to align with the new structure:
export interface UserUpdateParams<TUser extends Partial<User>> {
userId: TUser['id']
data: Partial<Omit<TUser, 'id'>> & {
- avatar?: File | string
+ profile?: Partial<MinimalProfile> & {
+ image?: File | string
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data: Partial<Omit<TUser, 'id'>> & { | |
| avatar?: File | string | |
| } | |
| data: Partial<Omit<TUser, 'id'>> & { | |
| profile?: Partial<MinimalProfile> & { | |
| image?: File | string | |
| } | |
| } |
| } | ||
| firstName: string | ||
| lastName: string | ||
| profile: MinimalProfile |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Breaking change requires GraphQL schema updates and component modifications
The transition from avatar to profile.image affects GraphQL-related code:
- GraphQL queries and fragments need to be updated to use
profile.imageinstead ofavatar - Generated types in
packages/components/__generated__/need to be regenerated - GraphQL schema already has
profilefield definitions in place
Please ensure:
- Update all GraphQL queries/fragments that reference
user.avatar - Regenerate GraphQL types after query updates
- Update any component code that accesses avatar through destructuring
🔗 Analysis chain
Verify the impact of this breaking change across the application.
The transition from direct avatar property to profile.image is a significant architectural change that affects how profile data is accessed throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in the codebase
# Check for direct avatar property access that needs updating
rg "user\.avatar\b" --type ts --type tsx -l
# Check for profile property access patterns
rg "user\.profile\.(image|name|urlPath)" --type ts --type tsx -l
# Check for destructuring patterns that might break
rg "const \{.*avatar.*\} = user" --type ts --type tsx -l
Length of output: 255
Script:
#!/bin/bash
# Description: Check for potential breaking changes in the codebase using correct file type flags
# Check for direct avatar property access that needs updating
rg "user\.avatar\b" -t ts -t typescript -t js -t jsx
# Check for profile property access patterns
rg "user\.profile\.(image|name|urlPath)" -t ts -t typescript -t js -t jsx
# Check for destructuring patterns that might break
rg "const \{.*avatar.*\} = user" -t ts -t typescript -t js -t jsx
# Additional check for avatar usage in test files
rg "avatar.*:" -t ts -t typescript -t test
Length of output: 383
Script:
#!/bin/bash
# Description: Check for potential breaking changes using simpler ripgrep commands
# Find all TypeScript/JavaScript files first
fd "\.(ts|js)$" > files.txt
# Check for direct avatar property access that needs updating
rg "user\.avatar\b" $(cat files.txt)
# Check for profile property access patterns
rg "user\.profile\." $(cat files.txt)
# Check for destructuring patterns that might break
rg "const.*\{.*avatar.*\}.*=.*user" $(cat files.txt)
# Clean up
rm files.txt
Length of output: 77631
Script:
#!/bin/bash
# Description: Check for GraphQL schema definitions and queries that might need updates
# Look for GraphQL type definitions containing avatar field
rg "type.*\{[^}]*avatar.*:[^}]*\}" -g "*.graphql"
# Look for GraphQL queries/fragments referencing avatar
rg "fragment.*\{[^}]*avatar" -g "*.graphql"
rg "query.*\{[^}]*avatar" -g "*.graphql"
# Check for avatar in GraphQL generated types
rg "avatar.*:" -g "*generated*.ts"
# Check for profile-related GraphQL fields
rg "profile.*:" -g "*.graphql"
Length of output: 2652
Rename PROFILE_KEY, introduce getMinimalProfile to read out minimal profile data from graphql query results
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx (1)
6-13: Consider adding initial profile state setup for testsSince this is a test utility for profile-related components, it might be helpful to accept and set up initial profile state through Jotai.
Consider updating the component to accept initial profile state:
-const TestComponentWithProviders = ({ environment }: { environment: Environment }) => { +interface TestComponentWithProvidersProps { + environment: Environment; + initialProfile?: MinimalProfile; +} + +const TestComponentWithProviders = ({ + environment, + initialProfile +}: TestComponentWithProvidersProps) => { return ( - <JotaiProvider> + <JotaiProvider initialValues={initialProfile ? [[currentProfileAtom, initialProfile]] : undefined}> <RelayEnvironmentProvider environment={environment}> <TestComponent /> </RelayEnvironmentProvider> </JotaiProvider> ) }packages/components/modules/profiles/useCurrentProfile/index.ts (1)
45-96: Consider splitting the hook into smaller, focused hooks.The current hook handles multiple concerns: profile state management, cookie synchronization, and event handling. Consider splitting these responsibilities into separate hooks for better maintainability:
useProfileState: Handle basic state managementuseProfileCookie: Handle cookie synchronizationuseProfileEvents: Handle event subscriptionsThis separation would make the code more maintainable and easier to test. Would you like me to provide an example implementation of this split?
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx (1)
69-71: Enhance avatar image handlingWhile the simplified image source logic is cleaner, consider these improvements:
- Use a default avatar image instead of an empty string to prevent unnecessary requests
- Add dynamic alt text using the profile name
-src={profile?.image ?? ''} -alt="User avatar" +src={profile?.image ?? '/assets/default-avatar.png'} +alt={`${profile?.name ?? 'User'}'s avatar`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
packages/authentication/types/user.ts(1 hunks)packages/components/.storybook/decorators/withProviders.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx(2 hunks)packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx(3 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts(2 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx(1 hunks)packages/components/modules/profiles/useCurrentProfile/constants.ts(1 hunks)packages/components/modules/profiles/useCurrentProfile/index.ts(1 hunks)packages/utils/index.ts(1 hunks)packages/utils/types/profile.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/authentication/types/user.ts
- packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts
- packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx
- packages/components/modules/profiles/index.ts
- packages/components/modules/profiles/useCurrentProfile/constants.ts
🔇 Additional comments (14)
packages/utils/types/profile.ts (2)
1-6: LGTM! Well-structured minimal type definition.
The MinimalProfile type is well-designed with:
- Essential fields for profile identification and display
- Appropriate nullability for SSR compatibility
- Clear and consistent naming
1-6: Verify if MinimalProfile fields are sufficient for all use cases.
Let's ensure this minimal structure satisfies all requirements across the codebase.
✅ Verification successful
Let me try a different approach to gather the necessary information.
Based on the search results, I can now provide a final response:
MinimalProfile fields are sufficient for current use cases
The MinimalProfile type is used appropriately across the codebase, primarily in:
- Profile switching functionality in ProfilePopover component
- Current profile management through useCurrentProfile hook
- User authentication context
All usages only require the defined fields:
id: For profile identificationname: For display in UIimage: For avatar displayurlPath: For profile navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for profile-related type usage and field access patterns
# to verify the completeness of MinimalProfile
# Check for any additional profile fields being accessed
echo "Checking profile field access patterns..."
rg -t typescript -t javascript "profile\." --context 2
# Look for Profile type definitions to compare structure
echo "Checking other Profile type definitions..."
ast-grep --pattern 'type $name = {
$$$
}'
# Search for profile-related props in components
echo "Checking profile props in components..."
ast-grep --pattern 'interface $name Props {
$$$
profile: $type
$$$
}'
Length of output: 492
Script:
#!/bin/bash
# Search for profile-related type usage with ripgrep
echo "Searching for profile type usage..."
rg --type-add 'ts:*.{ts,tsx}' --type ts "profile\." -A 2
# Look for Profile type definitions
echo "Searching for Profile type definitions..."
rg --type-add 'ts:*.{ts,tsx}' --type ts "type.*Profile.*=.*{" -A 5
# Search for profile-related props and interfaces
echo "Searching for profile interfaces..."
rg --type-add 'ts:*.{ts,tsx}' --type ts "(interface|type).*Profile" -A 5
# Search specifically for MinimalProfile usage
echo "Searching for MinimalProfile usage..."
rg --type-add 'ts:*.{ts,tsx}' --type ts "MinimalProfile"
Length of output: 34258
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx (2)
8-8: LGTM! Clean import of JotaiProvider
The addition of JotaiProvider import aligns with the PR's objective of transitioning from Zustand to Jotai for state management.
19-29: LGTM! Well-structured provider hierarchy
The provider hierarchy is correctly implemented with:
- JotaiProvider as the outermost provider for global state
- Proper nesting of other essential providers
- Clean removal of CurrentProfileProvider aligning with the simplified profile management approach
packages/utils/index.ts (2)
34-34: LGTM! Export follows established patterns
The new type export follows the file's established pattern of organizing exports by category (constants, functions, hooks, types) and maintains consistency with other type exports.
34-34: Verify MinimalProfile type usage
Let's verify that the MinimalProfile type is being used consistently across components that handle profile data, especially given the transition from the previous avatar structure.
✅ Verification successful
MinimalProfile type is consistently used across the codebase
The MinimalProfile type is being used correctly throughout the codebase:
- Properly exported from
packages/utils/types/profile.ts - Correctly imported and used in authentication and components packages
- Consistently used in profile management components, especially in
useCurrentProfilehook - Avatar structure appears only in test mocks and material UI overrides, not in actual profile implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of MinimalProfile type across the codebase
# Search for MinimalProfile type usage
echo "=== MinimalProfile Usage ==="
rg "MinimalProfile" -t typescript -A 2
# Search for potential missed refactoring of avatar properties
echo -e "\n=== Potential Missed Refactoring ==="
rg "avatar\s*:\s*{" -t typescript
# Search for useCurrentProfile hook implementations
echo -e "\n=== Current Profile Hook Usage ==="
rg "useCurrentProfile" -t typescript -A 3
Length of output: 519
Script:
#!/bin/bash
# Let's try without typescript file type specification and fix the regex
# Search for MinimalProfile type usage
echo "=== MinimalProfile Usage ==="
rg "MinimalProfile"
# Search for potential missed refactoring of avatar properties
echo -e "\n=== Potential Missed Refactoring ==="
rg "avatar:\s*\{"
# Search for useCurrentProfile hook implementations
echo -e "\n=== Current Profile Hook Usage ==="
rg "useCurrentProfile"
# Search for profile type definitions
echo -e "\n=== Profile Type Definitions ==="
rg "type.*Profile"
Length of output: 15179
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx (1)
5-5: LGTM: Necessary imports for cookie-based profile storage
The new imports support the transition from local storage to cookies for profile management.
Also applies to: 9-9
packages/components/.storybook/decorators/withProviders.tsx (3)
9-9: LGTM: Clean import of Jotai Provider
The import aligns with the PR objective of transitioning to Jotai for state management.
37-49: LGTM: Clean provider hierarchy restructuring
The provider ordering is logical:
- JotaiProvider as outermost layer for global state
- Data providers (Query, Relay) properly positioned
- UI-related providers correctly nested inside
37-49: Verify story compatibility with new profile management
Since CurrentProfileProvider was removed, stories that relied on it might need updates to work with the new Jotai-based profile management.
✅ Verification successful
No impact found on stories from profile management changes
Based on the verification results, no stories were found that use CurrentProfileProvider or access profile data through useCurrentProfile or currentProfile. This suggests that the stories were not dependent on the previous profile management system, and therefore the migration to Jotai-based profile management should not affect them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stories that might need updates
echo "Searching for stories potentially using CurrentProfileProvider..."
rg -l "CurrentProfileProvider" "packages/components/src/**/*.stories.tsx"
echo "Searching for stories accessing profile data..."
rg -l "useCurrentProfile|currentProfile" "packages/components/src/**/*.stories.tsx"
Length of output: 567
Script:
#!/bin/bash
# Search for stories using fd to find correct paths
echo "Finding story files..."
fd -e stories.tsx . packages/components/src
echo "Searching for stories potentially using CurrentProfileProvider..."
fd -e stories.tsx . packages/components/src -x rg -l "CurrentProfileProvider" {}
echo "Searching for stories accessing profile data..."
fd -e stories.tsx . packages/components/src -x rg -l "useCurrentProfile|currentProfile" {}
Length of output: 857
Script:
#!/bin/bash
# First, let's check the correct directory structure
echo "Checking directory structure..."
fd -t d components
# Then search for stories in the correct path
echo "Finding story files..."
fd -e stories.tsx .
echo "Searching for stories potentially using CurrentProfileProvider..."
fd -e stories.tsx . -x rg -l "CurrentProfileProvider" {}
echo "Searching for stories accessing profile data..."
fd -e stories.tsx . -x rg -l "useCurrentProfile|currentProfile" {}
Length of output: 663
packages/components/modules/profiles/useCurrentProfile/index.ts (2)
47-47: Fix incorrect SSR detection.
The current SSR detection is incorrect.
-const isSSR = typeof window === typeof undefined
+const isSSR = typeof window === 'undefined'49-58: 🛠️ Refactor suggestion
Improve error handling in setCurrentProfile.
The error handling could be more informative and include validation.
const setCurrentProfile = (newProfile: MinimalProfile) => {
+ if (!newProfile?.id) {
+ console.error('Invalid profile data provided to setCurrentProfile');
+ return;
+ }
setProfile(() => {
try {
setCookie(CURRENT_PROFILE_KEY, newProfile, { stringfyValue: true })
} catch (error) {
- console.log(error)
+ console.error('Failed to set profile cookie:', error)
}
return newProfile
})
}Likely invalid or redundant comment.
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx (2)
Line range hint 1-124: Overall implementation looks good
The changes successfully transition to the new profile management system while maintaining component modularity and proper cleanup. The implementation aligns well with the PR objectives of simplifying profile management and enabling SSR support.
36-36: Verify SSR behavior and enhance type safety
The noSSR: false option enables server-side rendering, which aligns with the PR objectives. However, let's ensure proper hydration and consider type safety improvements.
Consider adding type safety:
-const profile = useCurrentProfile({ noSSR: false }).currentProfile
+const { currentProfile: profile } = useCurrentProfile({ noSSR: false })✅ Verification successful
Let me gather more information about the useCurrentProfile hook implementation and its usage pattern.
Let me try one more search to find other usages of useCurrentProfile with proper file type.
Let me try one final search without file type restrictions.
Based on the gathered information, I can now provide the final response:
The current profile access pattern is inconsistent with other components
The current destructuring pattern useCurrentProfile({ noSSR: false }).currentProfile differs from the established pattern used across other components. Most components use object destructuring:
const { currentProfile } = useCurrentProfile()const { currentProfile, setCurrentProfile } = useCurrentProfile()
The SSR behavior is properly handled in the hook's implementation, with appropriate cookie handling and state management. For consistency, update to:
-const profile = useCurrentProfile({ noSSR: false }).currentProfile
+const { currentProfile: profile } = useCurrentProfile({ noSSR: false })🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential hydration issues by looking for useEffect hooks handling profile data
rg -A 5 "useEffect.*profile"
# Verify consistent SSR option usage across components
rg "useCurrentProfile.*noSSR"
Length of output: 592
Script:
#!/bin/bash
# Check the implementation of useCurrentProfile
ast-grep --pattern 'const useCurrentProfile = $_'
# Look for other components using useCurrentProfile to verify consistent usage patterns
rg -A 3 "useCurrentProfile" --type typescript
Length of output: 4848
Script:
#!/bin/bash
# Look for other components using useCurrentProfile
rg -A 3 "useCurrentProfile" --type ts --type tsx
Length of output: 76
Script:
#!/bin/bash
# Look for other components using useCurrentProfile without file type restrictions
rg "const.*useCurrentProfile" -A 2
# Also check for direct object destructuring patterns
rg "const.*\{.*currentProfile.*\}.*=.*useCurrentProfile"
Length of output: 3223
| @@ -1,15 +1,15 @@ | |||
| import { Provider as JotaiProvider } from 'jotai' | |||
There was a problem hiding this comment.
💡 Codebase verification
The test setup needs profile initialization via Jotai
The current test setup is missing the initialization of the profile atom that was previously handled by CurrentProfileProvider. Based on the test file and implementation:
- The tests rely on profile data being available through cookies (
loadPreStoredData) - The Jotai atom is initialized with data from
CURRENT_PROFILE_KEYcookie - TestComponentWithProvider should initialize the atom with the test profile data
Suggested changes:
- Add profile atom initialization in TestComponentWithProvider using Jotai's
initialValuesprop - Update the provider setup:
<JotaiProvider initialValues={[[profileAtom, testProfile]]}>🔗 Analysis chain
LGTM! Provider setup aligns with the migration to Jotai.
The transition from CurrentProfileProvider to JotaiProvider is correctly implemented, maintaining the necessary provider hierarchy for testing components.
Let's verify if any profile-related test setup is needed in the test components:
Also applies to: 8-12
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for profile-related test setup patterns in other test files
# Look for test setup patterns that might need to be replicated here
# Search for profile-related test setup in test files
rg -t typescript -t javascript "beforeEach|beforeAll|setup" "packages/components/modules/profiles" -A 5
# Search for Jotai atom setup in test files
rg -t typescript -t javascript "atom|useAtom" "packages/components/modules/profiles/__tests__" -A 5
Length of output: 273
Script:
#!/bin/bash
# Let's try with ripgrep without file type filters
# Search for profile-related test setup patterns
rg "beforeEach|beforeAll|setup" "packages/components/modules/profiles" -A 5
# Search for Jotai atom setup in test files
rg "atom|useAtom" "packages/components/modules/profiles" -A 5
# Also check the specific test component implementation
fd "TestComponentWithProvider" "packages/components/modules/profiles" --exec cat {}
Length of output: 3006
Script:
#!/bin/bash
# Let's check the actual test component implementation file
cat packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx
# Also check for any test setup in the current profile tests
cat packages/components/modules/profiles/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx
Length of output: 2961
| Cookies.set( | ||
| PROFILE_KEY, | ||
| JSON.stringify({ | ||
| id: '1234', | ||
| name: 'test', | ||
| image: null, | ||
| url: { path: 'path' }, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Mock profile structure needs to be updated to match MinimalProfile type
The mock profile structure in the story doesn't match the MinimalProfile type definition:
url: { path: 'path' }should beurlPath: string | null
Update the mock data to match the type:
Cookies.set(
PROFILE_KEY,
JSON.stringify({
id: '1234',
name: 'test',
image: null,
- url: { path: 'path' },
+ urlPath: '/path',
}),
COOKIE_OPTIONS
)🔗 Analysis chain
Consider enhancing cookie security and mock data management
While the implementation works, consider these improvements:
- Add cookie security options (httpOnly, secure, sameSite)
- Move mock profile data to a separate constants file
- Verify that the mock profile structure matches the
MinimalProfiletype
Consider updating the code like this:
+ import { MOCK_PROFILE_DATA } from './mockData'
+ const COOKIE_OPTIONS = {
+ secure: true,
+ sameSite: 'strict',
+ path: '/'
+ }
Cookies.set(
PROFILE_KEY,
- JSON.stringify({
- id: '1234',
- name: 'test',
- image: null,
- url: { path: 'path' },
- }),
+ JSON.stringify(MOCK_PROFILE_DATA),
+ COOKIE_OPTIONS
)Let's verify the profile structure matches the type definition:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MinimalProfile type definition
ast-grep --pattern 'type MinimalProfile = {
$$$
}'
Length of output: 374
| useEffect(() => { | ||
| eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile) | ||
| return () => { | ||
| eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile) | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Add removeCurrentProfile to useEffect dependencies.
The effect's dependency array should include removeCurrentProfile to prevent potential stale closures.
useEffect(() => {
eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile)
return () => {
eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile)
}
-}, [])
+}, [removeCurrentProfile])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile) | |
| return () => { | |
| eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile) | |
| } | |
| }, []) | |
| useEffect(() => { | |
| eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile) | |
| return () => { | |
| eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile) | |
| } | |
| }, [removeCurrentProfile]) |
| export const getMinimalProfile = function <T extends ProfileItemFragment$data>( | ||
| profile: T, | ||
| ): MinimalProfile { | ||
| return { | ||
| id: profile.id, | ||
| name: profile.name ?? null, | ||
| image: profile.image?.url ?? null, | ||
| urlPath: profile.urlPath?.path ?? null, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required fields in getMinimalProfile.
The function should validate that required fields exist before transformation.
export const getMinimalProfile = function <T extends ProfileItemFragment$data>(
profile: T,
): MinimalProfile {
+ if (!profile?.id) {
+ throw new Error('Profile must have an id');
+ }
return {
id: profile.id,
name: profile.name ?? null,
image: profile.image?.url ?? null,
urlPath: profile.urlPath?.path ?? null,
} as const
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getMinimalProfile = function <T extends ProfileItemFragment$data>( | |
| profile: T, | |
| ): MinimalProfile { | |
| return { | |
| id: profile.id, | |
| name: profile.name ?? null, | |
| image: profile.image?.url ?? null, | |
| urlPath: profile.urlPath?.path ?? null, | |
| } | |
| } | |
| export const getMinimalProfile = function <T extends ProfileItemFragment$data>( | |
| profile: T, | |
| ): MinimalProfile { | |
| if (!profile?.id) { | |
| throw new Error('Profile must have an id'); | |
| } | |
| return { | |
| id: profile.id, | |
| name: profile.name ?? null, | |
| image: profile.image?.url ?? null, | |
| urlPath: profile.urlPath?.path ?? null, | |
| } as const | |
| } |
| export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | ||
| const settings = | ||
| getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | ||
|
|
||
| return settings | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type validation for cookie parsing.
The function should handle JSON parsing errors and validate the shape of the parsed data.
export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => {
+ try {
const settings =
getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null
+ if (settings && (!settings.id || typeof settings.id !== 'string')) {
+ console.error('Invalid profile data in cookie');
+ return null;
+ }
+
return settings
+ } catch (error) {
+ console.error('Error parsing profile cookie:', error);
+ return null;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | |
| const settings = | |
| getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | |
| return settings | |
| } | |
| export const getProfileFromCookie = ({ noSSR = true }: ServerSideRenderingOption = {}) => { | |
| try { | |
| const settings = | |
| getCookie<MinimalProfile | undefined>(CURRENT_PROFILE_KEY, { noSSR, parseJSON: true }) ?? null | |
| if (settings && (!settings.id || typeof settings.id !== 'string')) { | |
| console.error('Invalid profile data in cookie'); | |
| return null; | |
| } | |
| return settings | |
| } catch (error) { | |
| console.error('Error parsing profile cookie:', error); | |
| return null; | |
| } | |
| } |
| const loadPreStoredData = (customUserProfileMockData: any) => { | ||
| const profile = customUserProfileMockData.data.me.profile as MinimalProfile | ||
|
|
||
| setCookie(CURRENT_PROFILE_KEY, profile, { stringfyValue: true }) |
There was a problem hiding this comment.
Fix typo: Correct 'stringfyValue' to 'stringifyValue'
The option key stringfyValue is misspelled. It should be stringifyValue. This typo may prevent the cookie from being set correctly.
Apply this diff to fix the typo:
- setCookie(CURRENT_PROFILE_KEY, profile, { stringfyValue: true })
+ setCookie(CURRENT_PROFILE_KEY, profile, { stringifyValue: true })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setCookie(CURRENT_PROFILE_KEY, profile, { stringfyValue: true }) | |
| setCookie(CURRENT_PROFILE_KEY, profile, { stringifyValue: true }) |
| }) | ||
| }) | ||
|
|
||
| const loadPreStoredData = (customUserProfileMockData: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with a specific type for better type safety
Using any undermines TypeScript's type checking. Replace any with a more specific type that matches the structure of customUserProfileMockData.
Update the function signature:
- const loadPreStoredData = (customUserProfileMockData: any) => {
+ const loadPreStoredData = (customUserProfileMockData: ReturnType<typeof mockUserProfileFactory>) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const loadPreStoredData = (customUserProfileMockData: any) => { | |
| const loadPreStoredData = (customUserProfileMockData: ReturnType<typeof mockUserProfileFactory>) => { |
| profileRef: ProfileItemFragment$key | ||
| onProfileChange: (newProfile: ProfileItemFragment$data) => void | ||
| currentProfile?: ProfileItemFragment$data | ||
| currentProfile?: MinimalProfile | null |
There was a problem hiding this comment.
let's not pass currentProfile as props, instead ProfileMenuItem could get it from useCurrentProfile
| import AccountPopover from '..' | ||
| import { withTokenSetup } from '../../../../../../.storybook/decorators' | ||
| import { CURRENT_PROFILE_STORAGE_KEY } from '../../../../../profiles/context/CurrentProfileProvider/constants' | ||
| import { PROFILE_KEY } from '../../../../../profiles/useCurrentProfile/constants' |
This refactors the way in which the current profile is provided, switching from
ZustandtoJotaifor handling the state.The corresponding changes on the backend were implemented in BA-1808 Add basic profile information to JWT claims baseapp-backend#189.
Summary by CodeRabbit
Release Notes
New Features
useCurrentProfilehook for managing user profile data, enhancing state management and cookie synchronization.CURRENT_PROFILE_KEYfor profile management.Improvements
MinimalProfileinterface, simplifying data access across components.currentProfilefrom theuseCurrentProfilehook, improving code clarity.AccountPopovercomponent.Bug Fixes
Chores