BA-1809 [FE] Simplify CurrentProfileProvider #142
Conversation
|
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/modules/access/useLogin/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
WalkthroughThe pull request introduces significant changes across multiple components and modules, primarily focusing on user profile management. Key modifications include the removal of the Changes
Possibly related PRs
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: 13
🧹 Outside diff range and nitpick comments (26)
packages/utils/types/profile.ts (2)
1-6: Add documentation to improve type clarity and maintainabilitySince this type is crucial for profile management and SSR, please consider adding documentation:
- Add JSDoc comment explaining the type's purpose and usage
- Document when nullable fields can be null
- Clarify the purpose and format of
urlPathExample improvement:
+/** + * Represents essential profile data used for SSR and cookie storage. + * Used as a lightweight alternative to the full profile type. + */ export type MinimalProfile = { + /** Unique identifier for the profile */ id: string + /** Display name of the profile. Null for unset names */ name: string | null + /** URL to the profile image. Null when no image is set */ image: string | null + /** URL-friendly path for the profile page. Null for new profiles */ urlPath: string | null }
4-4: Consider adding URL validation for the image fieldSince
imagepresumably stores a URL, consider using a more specific type or adding validation to ensure it's a valid URL when non-null.You could create a custom type:
type ImageUrl = string & { readonly brand: unique symbol } function validateImageUrl(url: string): ImageUrl { if (!/^https?:\/\/.+/.test(url)) { throw new Error('Invalid image URL format') } return url as ImageUrl }packages/components/modules/profiles/useCurrentProfile/__tests__/__mock__/profiles.ts (1)
3-10: Add TypeScript type definitions for better type safety and documentation.Consider adding explicit type definitions to improve code maintainability and prevent type-related issues in tests.
+import type { MinimalProfile } from '../types' + -export const mockUserProfileFactory = (id: string) => { +export const mockUserProfileFactory = (id: string): MinimalProfile => { return { id, name: faker.person.fullName(), image: faker.image.avatar(), urlPath: faker.internet.url(), } }packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx (1)
4-4: Consider using destructuring for better hook usageThe current implementation could potentially cause multiple hook calls. Consider using destructuring to follow React best practices and ensure optimal performance.
- const profile = useCurrentProfile().currentProfile + const { currentProfile: profile } = useCurrentProfile()This change would:
- Prevent potential multiple hook calls
- Make the code more idiomatic React
- Maintain better consistency with React hooks usage patterns
packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/useAndInitializeCurrentProfile/index.tsx (1)
5-12: Enhance test utility robustnessAs a test utility function, this implementation could be more robust to handle various test scenarios.
Consider these improvements:
- Add validation for the initialProfile parameter
- Handle cases where a profile is already set
- Add options to simulate loading/error states for tests
Here's a suggested enhancement:
export const useAndInitializeCurrentProfile = ( initialProfile: MinimalProfile, { noSSR = true, force = false, // Force update even if profile exists simulateLoading = false, simulateError?: Error }: ServerSideRenderingOption & TestOptions = {}, ) => { const { currentProfile, setCurrentProfile } = useCurrentProfile({ noSSR }) React.useEffect(() => { if (!initialProfile?.id) { throw new Error('Invalid profile provided') } if (simulateError) { throw simulateError } if (!currentProfile || force) { setCurrentProfile(initialProfile) } }, [initialProfile, force, currentProfile, setCurrentProfile]) if (simulateLoading) { return { currentProfile: undefined, setCurrentProfile } } return { currentProfile, setCurrentProfile } }packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx (1)
1-4: Consider adding test-specific profile atom initialization.Since this is a test utility, it might be helpful to initialize Jotai atoms with test-specific values.
Consider adding a wrapper that pre-initializes profile atoms:
import { Provider as JotaiProvider, createStore } from 'jotai' import { profileAtom } from '../../atoms' // adjust path as needed const createTestStore = (initialProfile?: Profile) => { const store = createStore() if (initialProfile) { store.set(profileAtom, initialProfile) } return store } // Then use it in the component: const TestComponentWithProviders = ({ environment, initialProfile }: { environment: Environment, initialProfile?: Profile }) => { const store = createTestStore(initialProfile) return ( <JotaiProvider store={store}> <RelayEnvironmentProvider environment={environment}> <TestComponent /> </RelayEnvironmentProvider> </JotaiProvider> ) }packages/authentication/types/user.ts (1)
19-21: Consider being more specific about avatar string formatThe
avatarfield accepts eitherFileorstring, but it would be helpful to be more explicit about the expected string format (URL vs base64).- avatar?: File | string + avatar?: File | { url: string } | { base64: string }packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx (1)
10-10: Consider destructuring for cleaner codeWhile the current implementation works, you could make it more concise by destructuring
currentProfiledirectly from the hook.- const profile = useCurrentProfile().currentProfile + const { currentProfile: profile } = useCurrentProfile()packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx (2)
10-10: LGTM! Consider memoizing the current profile ID.The transition to using the
useCurrentProfilehook aligns well with the PR objectives and eliminates prop drilling. The implementation is clean and follows React best practices.Consider memoizing the current profile ID if it's used in multiple places or if the component re-renders frequently:
const { currentProfile } = useCurrentProfile() + const currentProfileId = useMemo(() => currentProfile?.id, [currentProfile]) const profile = readInlineData<ProfileItemInlineFragment$key>( ProfileItemInlineFragment, profileRef, ) const profileUrlPath = profile.urlPath?.path - const isActiveProfile = profile.id === currentProfile?.id + const isActiveProfile = profile.id === currentProfileIdAlso applies to: 21-21
Line range hint
33-33: Consider passing minimal profile data in onClick.The
onProfileChangehandler receives the entire profile object, which might include more data than necessary. Consider passing only the required fields to minimize unnecessary data flow.- onClick={() => onProfileChange(profile)} + onClick={() => onProfileChange({ id: profile.id, name: profile.name, urlPath: profile.urlPath })}packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx (2)
36-36: Address TODO comment regarding backend subscriptionThe TODO comment suggests uncertainty about the backend subscription functionality. This should be verified before deployment to ensure reliable real-time updates.
Would you like me to:
- Create a GitHub issue to track this verification task?
- Help generate test cases to verify the subscription behavior?
Line range hint
39-67: Add missing dependency to useMemoThe config uses
profile?.idin the subscription variables, butprofileis not included in the dependency array. This could lead to stale subscriptions if the profile changes.const config = useMemo( () => ({ // ... config }), - [nodeId], + [nodeId, profile?.id], )packages/components/modules/profiles/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx (4)
3-4: Consider consolidating imports from the same moduleThe imports from '@baseapp-frontend/utils' can be combined into a single import statement for better maintainability.
-import { MinimalProfile } from '@baseapp-frontend/utils' -import { LOGOUT_EVENT, eventEmitter, getCookie, setCookie } from '@baseapp-frontend/utils' +import { LOGOUT_EVENT, MinimalProfile, eventEmitter, getCookie, setCookie } from '@baseapp-frontend/utils'
17-19: Add JSDoc documentation and consider renaming the helper functionThe helper function would benefit from documentation and a more specific name since it deals with cookies.
-const loadPreStoredData = (profile: MinimalProfile) => { +/** + * Sets the current profile data in cookies for testing purposes + * @param profile - The minimal profile to store in cookies + */ +const loadProfileIntoCookie = (profile: MinimalProfile) => { setCookie(CURRENT_PROFILE_KEY, profile, { stringfyValue: true }) }
36-54: Improve test clarity and assertionsA few suggestions to enhance the test:
- Use a different profile ID to avoid confusion with the first test
- Add assertions to verify the complete logout flow
- const profile2 = mockUserProfileFactory('user-profile-1') + const profile2 = mockUserProfileFactory('user-profile-2') loadPreStoredData(profile2) const TestComponentWithProviders = require('./__utils__/TestComponentWithProvider').default - render(<TestComponentWithProviders environment={environment} />) + const { getByText } = render(<TestComponentWithProviders environment={environment} />) + + // Verify profile is initially present + await waitFor(() => { + expect(getByText(profile2.id)).toBeInTheDocument() + }) act(() => { eventEmitter.emit(LOGOUT_EVENT) })
11-55: Consider adding more test scenariosThe test suite would benefit from additional coverage:
- Error handling for malformed cookie data
- Profile switching scenarios
- Edge cases around cookie expiration or deletion
Would you like me to help generate these additional test cases?
🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (1)
26-30: Consider adding error handling for profile switchingWhile the implementation of
getMinimalProfileis a good optimization for SSR and cookie storage, consider adding error handling for the profile switch operation.Consider updating the implementation to handle potential errors:
const handleProfileChange = (profile: ProfileItemInlineFragment$data) => { if (currentProfile?.id !== profile.id) { - setCurrentProfile(getMinimalProfile(profile)) - sendToast(`Switched to ${profile.name}`) - handleCloseSubmenu() + try { + setCurrentProfile(getMinimalProfile(profile)) + sendToast(`Switched to ${profile.name}`) + handleCloseSubmenu() + } catch (error) { + sendToast('Failed to switch profile', { variant: 'error' }) + console.error('Profile switch failed:', error) + } } }packages/components/modules/profiles/useCurrentProfile/index.ts (3)
20-29: Consider adding input validation for the profile parameter.While the implementation is correct, it might be good to add validation to ensure the required
idfield exists.export const getMinimalProfile = function ( profile: Omit<ProfileItemInlineFragment$data, ' $fragmentType'>, ): 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, } }
47-47: Improve SSR detection type safety.The current SSR detection could be more type-safe.
- const isSSR = typeof window === typeof undefined + const isSSR = typeof window === 'undefined'
84-96: Consider extracting cookie operations to a separate utility.The SSR handling logic could be simplified by extracting cookie operations to a dedicated utility.
+const getProfileWithSSR = (noSSR: boolean) => { + if (typeof window === 'undefined') { + return getProfileFromCookie({ noSSR }) + } + return null +} if (isSSR) { return { - currentProfile: getProfileFromCookie({ noSSR }), + currentProfile: getProfileWithSSR(noSSR), setCurrentProfile, updateProfileIfActive, } }packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx (1)
Line range hint
41-44: Consider extracting participant filtering logic for better readability.The logic for finding other participants could be extracted into a separate function to improve readability and maintainability.
+ const findOtherParticipant = (participants: any, currentProfileId: string) => + participants.edges.find( + (edge) => edge?.node?.profile?.id && edge?.node?.profile?.id !== currentProfileId + ); if ( room.participants?.totalCount && room.participants?.totalCount < MINIMUM_AMOUNT_OF_PARTICIPANTS_TO_SHOW_ROOM_TITLE ) { - const otherParticipant = room.participants.edges.find( - (edge) => edge?.node?.profile?.id && edge?.node?.profile?.id !== profile?.id, - ) + const otherParticipant = findOtherParticipant(room.participants, profile?.id); roomData.title = otherParticipant?.node?.profile?.name roomData.avatarUrl = otherParticipant?.node?.profile?.image?.url }packages/components/modules/messages/MessagesList/index.tsx (1)
Line range hint
119-133: Consider adding error handling for the mutationThe read message mutation lacks error handling, which could be important for debugging issues in production.
Consider adding error handling:
commitMutation({ variables: { input: { roomId: room.id, profileId: profile?.id as string, }, }, + onError: (error) => { + console.error('Failed to mark messages as read:', error); + }, })packages/components/modules/messages/SendMessage/index.tsx (2)
Line range hint
95-109: Implement proper error handling for message sending failuresThe current implementation uses console.error and TODO comments for error handling. Given this is a user-facing message sending component, proper error handling is essential for:
- Providing feedback to users when messages fail to send
- Handling network errors gracefully
- Maintaining error logging for debugging
Consider implementing:
onCompleted: (response, errors) => { if (errors) { - // TODO: handle errors - console.error(errors) + const errorMessage = errors.map(error => error.message).join(', '); + toast.error(`Failed to send message: ${errorMessage}`); + logger.error('Message send failed', { errors, roomId }); } const mutationErrors = response?.chatRoomSendMessage?.errors setFormRelayErrors(form, mutationErrors) if (!mutationErrors?.length) { form.reset() + } else { + toast.error('Failed to send message. Please try again.'); } }, - // TODO: handle errors - onError: console.error, + onError: (error) => { + toast.error('Network error while sending message'); + logger.error('Message send failed', { error, roomId }); + },
Line range hint
86-109: Consider adding resilience patternsWhile the component is well-structured, consider enhancing its robustness with:
- Retry logic for failed mutations
- Rate limiting to prevent message spam
- Optimistic updates for better UX
Example implementation approach:
const RETRY_COUNT = 3; const RATE_LIMIT_MS = 1000; // Rate limiting utility const useRateLimit = (ms: number) => { const lastCall = useRef<number>(0); return () => { const now = Date.now(); if (now - lastCall.current < ms) { return false; } lastCall.current = now; return true; }; }; // In your component: const checkRateLimit = useRateLimit(RATE_LIMIT_MS); const onSubmit = (data: SocialUpsertForm) => { if (!checkRateLimit()) { toast.warning('Please wait before sending another message'); return; } // ... existing code };packages/components/package.json (1)
Zustand dependency is still actively used and required
The codebase is actively using Zustand for state management in several critical components:
useNotificationhook for toast notifications- Comment reply system (
useCommentReplyandCommentReplyProvider)- Chat room functionality (
useChatRoomandChatRoomProvider)These components haven't been migrated to Jotai yet and are still using Zustand's core features like
createStore,useStore, andcreate. The dependency should be retained until these components are refactored.🔗 Analysis chain
Line range hint
32-41: Verify if Zustand dependency is still required.Since this PR transitions from Zustand to Jotai for state management, verify if the
zustanddependency is still needed.
Let me check the actual usage in these files to determine if they are actively using Zustand.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining Zustand usage in the codebase # Search for Zustand imports and usage echo "Checking for Zustand usage..." rg -l "from ['\"]zustand" || echo "No Zustand imports found" # Search for create store patterns echo -e "\nChecking for Zustand store creation..." rg -l "create\w*Store" || echo "No Zustand store creation found"Length of output: 596
Script:
#!/bin/bash # Check the content of files that have Zustand imports echo "Content of useNotification/index.tsx:" cat packages/utils/hooks/useNotification/index.tsx echo -e "\nContent of useCommentReply/index.tsx:" cat packages/components/modules/comments/context/useCommentReply/index.tsx echo -e "\nContent of CommentReplyProvider/index.tsx:" cat packages/components/modules/comments/context/CommentReplyProvider/index.tsx echo -e "\nContent of ChatRoomProvider/index.tsx:" cat packages/components/modules/messages/context/ChatRoomProvider/index.tsx echo -e "\nContent of useChatRoom/index.tsx:" cat packages/components/modules/messages/context/useChatRoom/index.tsxLength of output: 4721
packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx (1)
Line range hint
30-144: Consider memoizing complex computations for better performance.The component performs several complex computations in the render callbacks. Consider memoizing the following values using
useMemoto optimize performance, especially for chat rooms with many messages:
isOwnMessageflexAlignmentscanShowAvatarcanShowNameExample optimization:
- const isOwnMessage = currentProfile?.id === message?.profile?.id - const flexAlignments = isOwnMessage ? 'flex-end' : 'flex-start' - const canShowAvatar = isFirstGroupedMessage && !isOwnMessage - const canShowName = - canShowAvatar && - roomParticipantsCount && - roomParticipantsCount >= MINIMUM_AMOUNT_OF_PARTICIPANTS_TO_SHOW_ROOM_TITLE + const messageDisplayProperties = useMemo(() => { + const isOwnMessage = currentProfile?.id === message?.profile?.id + const flexAlignments = isOwnMessage ? 'flex-end' : 'flex-start' + const canShowAvatar = isFirstGroupedMessage && !isOwnMessage + const canShowName = + canShowAvatar && + roomParticipantsCount && + roomParticipantsCount >= MINIMUM_AMOUNT_OF_PARTICIPANTS_TO_SHOW_ROOM_TITLE + + return { isOwnMessage, flexAlignments, canShowAvatar, canShowName } + }, [currentProfile?.id, message?.profile?.id, isFirstGroupedMessage, roomParticipantsCount]) + + const { isOwnMessage, flexAlignments, canShowAvatar, canShowName } = messageDisplayProperties
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between ef0aef0 and c63de9ca96cb172f576ac812b23cf75b3eea3e78.
📒 Files selected for processing (34)
packages/authentication/types/user.ts(1 hunks)packages/components/.storybook/decorators/withProviders.tsx(2 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(1 hunks)packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/index.tsx(1 hunks)packages/components/modules/messages/SendMessage/index.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(1 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(1 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/index.tsx(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts(0 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/CurrentProfileProvider.test.tsx(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__mock__/profiles.ts(0 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__/__mock__/profiles.ts(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx(1 hunks)packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/useAndInitializeCurrentProfile/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)packages/utils/index.ts(1 hunks)packages/utils/types/profile.ts(1 hunks)
💤 Files with no reviewable changes (7)
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/CurrentProfileProvider.test.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/mock/profiles.ts
- 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 (1)
- packages/components/modules/profiles/useCurrentProfile/constants.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/modules/profiles/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx
[error] 21-21: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (35)
packages/components/modules/profiles/index.ts (1)
1-1: LGTM! Verify imports across the codebase.
The export changes align with the transition from context-based state management to Jotai atoms. This simplification is a good architectural improvement.
Let's verify that all imports have been updated to the new path:
✅ Verification successful
Let me verify if there are any imports from the old context path that might have been missed by the previous search pattern.
All imports have been correctly updated
The verification shows that:
- No outdated imports of
useCurrentProfilefrom the context path exist - All current imports of
useCurrentProfileare using the correct relative path (../../useCurrentProfileor../../../useCurrentProfile) - The new export
getMinimalProfileis also being imported correctly where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports from the old context path
# and verify all current imports are from the new path
echo "Checking for any outdated imports from context..."
rg -l "import.*useCurrentProfile.*from.*['\"].*context['\"]"
echo "Verifying current imports..."
rg -l "import.*useCurrentProfile.*from.*['\"].*useCurrentProfile['\"]"
Length of output: 562
Script:
#!/bin/bash
# Using a broader search pattern to catch any potential variations of imports from context
echo "Checking for any context-related imports..."
rg -l "from ['\"].*context['\"]"
# Check if useCurrentProfile is imported from the correct relative path
echo -e "\nVerifying correct relative imports..."
rg -C 2 "import.*useCurrentProfile" packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx
Length of output: 3498
packages/utils/types/profile.ts (1)
1-6: Verify the completeness of minimal profile data
Given this type is used for SSR and replaces the previous avatar implementation, we should verify that these fields are sufficient for all use cases.
✅ Verification successful
Let me try different commands to gather the necessary information.
Based on the search results, I can now provide a final response:
MinimalProfile type appears to be complete for its use cases
The MinimalProfile type contains all the essential fields needed for its current usage across the codebase:
id: Used for profile identification and switchingname: Used in profile displays and menu itemsimage: Used for avatar displays in menus and listsurlPath: Used for profile navigation and links
The search results show these fields are consistently used in:
- Profile popover components for switching profiles
- Profile menu items for display
- Current profile display
- Profile lists and navigation
The previous avatar implementation has been fully migrated to use the image field, with no legacy avatar-specific code paths remaining in the TypeScript files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all profile-related data access to ensure we're not missing essential fields
echo "Searching for profile property access patterns..."
rg -t typescript "profile\." --context 2
echo "Searching for avatar usage that might need migration..."
rg -t typescript "avatar\." --context 2
echo "Checking profile-related components..."
fd -e tsx -e ts "Profile" --exec ast-grep --pattern 'interface $interface { $$$ }'
Length of output: 554
Script:
#!/bin/bash
# Search for profile property access patterns
echo "Searching for profile property access patterns..."
rg "profile\." -g "*.{ts,tsx}"
echo -e "\nSearching for avatar usage..."
rg "avatar\." -g "*.{ts,tsx}"
echo -e "\nChecking profile type definitions..."
rg "type.*Profile|interface.*Profile" -g "*.{ts,tsx}" -A 5
Length of output: 38784
packages/components/modules/profiles/useCurrentProfile/__tests__/__mock__/profiles.ts (1)
3-10: Verify alignment with new profile structure and JWT claims.
Since the PR involves deriving the initial profile from JWT claims, we should verify if this mock factory generates all required fields for testing the new functionality.
✅ Verification successful
Mock profile factory matches the MinimalProfile type definition
The mock profile factory in profiles.ts correctly implements all required fields of the MinimalProfile type as seen in packages/utils/types/profile.ts:
id: stringname: string | nullimage: string | nullurlPath: string | null
The implementation uses faker to generate valid test data for each field, which is suitable for testing the profile functionality. The profile is not directly derived from JWT claims but rather stored/retrieved from cookies as seen in the useCurrentProfile implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the mock profile structure matches the actual profile type definition
# and JWT claims usage across the codebase
# Look for MinimalProfile type definition
echo "Checking MinimalProfile type definition:"
ast-grep --pattern 'interface MinimalProfile {
$$$
}'
# Look for JWT claim usage with profile
echo "Checking JWT claim usage with profile:"
rg -A 5 'jwt.*claim.*profile|profile.*jwt.*claim'
Length of output: 287
Script:
#!/bin/bash
# Let's try different approaches to find profile-related types and JWT usage
# Search for any interface or type definition containing "Profile"
echo "Searching for Profile types:"
ast-grep --pattern 'type Profile = {
$$$
}'
ast-grep --pattern 'interface Profile {
$$$
}'
# Search for profile-related types in types or interfaces directories
echo "Searching for profile type files:"
fd -e ts -e tsx profile --type f
# Look for JWT-related code with profile
echo "Checking JWT and profile usage:"
rg -l "profile.*jwt|jwt.*profile"
# Look for profile-related imports and usages
echo "Checking profile imports and usages:"
rg -A 3 "import.*Profile.*from"
Length of output: 19584
Script:
#!/bin/bash
# Let's check the MinimalProfile type definition in utils package and profile-related code
# Check the profile type definition in utils
echo "Checking profile type in utils:"
cat packages/utils/types/profile.ts
# Check the current profile provider implementation
echo "Checking current profile provider implementation:"
cat packages/components/modules/profiles/useCurrentProfile/index.ts
# Look for JWT token related code
echo "Checking JWT token related code:"
rg -A 5 "getTokenClaims|parseJwt"
Length of output: 2994
packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx (1)
1-1: LGTM! Import path simplified
The simplified import path aligns well with the PR's objective of streamlining the current profile management.
packages/components/modules/profiles/useCurrentProfile/__tests__/__utils__/useAndInitializeCurrentProfile/index.tsx (2)
1-3: LGTM! Clean and well-organized imports.
1-12: Verify integration with new state management
Since this PR transitions from Zustand to Jotai and implements cookie-based storage, ensure this test utility properly reflects these architectural changes.
✅ Verification successful
Test utility correctly integrates with new Jotai state management
The test utility useAndInitializeCurrentProfile properly aligns with the new Jotai implementation:
- It correctly uses the
useCurrentProfilehook which now uses Jotai'suseAtom - The utility maintains the same interface while working with the new state management
- It properly handles the SSR option which is used for cookie-based storage
- No remnants of Zustand implementation in the profile management code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the useCurrentProfile implementation uses Jotai
rg -A 5 "from 'jotai'" "$(dirname "$(fd -t f useCurrentProfile/index.ts$ | head -n1)")"
# Check for any remaining Zustand references in profile-related code
rg "zustand" "$(dirname "$(fd -t f useCurrentProfile/index.ts$ | head -n1)")"
Length of output: 4181
Script:
#!/bin/bash
# Check the implementation of useCurrentProfile to verify Jotai integration
ast-grep --pattern 'const useCurrentProfile = $_'
# Check the test file to verify proper test setup with Jotai
fd -t f "useCurrentProfile/__tests__" -x cat {}
Length of output: 4817
packages/authentication/types/user.ts (1)
1-12: Verify the impact of removing direct avatar access
The transition from direct avatar property to MinimalProfile is a breaking change that might affect components directly accessing user.avatar. Let's verify all usages are updated.
Consider adding a migration guide in the PR description to help other developers update their code to use profile.image instead of the old avatar properties.
✅ Verification successful
Let me gather more information about the MinimalProfile type and GraphQL schema to understand the full picture.
Let me try one more search to locate the GraphQL schema and MinimalProfile definition with different patterns:
Let me check the MinimalProfile definition and the schema to understand the profile structure:
Based on the findings, I can now provide a final response:
The avatar property removal is safe to proceed
The transition from direct avatar property to profile.image is safe because:
- The
MinimalProfiletype correctly includes theimagefield as a replacement for avatar - All direct
user.avatarusages found are in generated GraphQL files (__generated__) which will be regenerated during the build process - The schema shows the User type already has a proper
avatarfield that accepts width/height parameters, suggesting the frontend is already using the new pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct avatar access that needs updating
rg -A 2 'user\.avatar\.'
# Verify MinimalProfile contains necessary avatar fields
ast-grep --pattern 'interface MinimalProfile {
$$$
image: $_
$$$
}'
Length of output: 2349
Script:
#!/bin/bash
# Search for MinimalProfile definition
ast-grep --pattern 'interface MinimalProfile {
$$$
}'
# Check GraphQL schema for user type definitions
rg -A 10 'type User' '*.graphql'
# Look for any avatar-related GraphQL fragments
rg -A 5 'fragment.*User' '*.graphql'
Length of output: 231
Script:
#!/bin/bash
# Find GraphQL schema files
fd -e graphql -e gql
# Search for MinimalProfile in typescript files
rg -A 5 'MinimalProfile' --type typescript
# Look for avatar usage in GraphQL related files
fd . -e graphql -e gql -e ts -e tsx --exec rg -l 'avatar|image'
Length of output: 6577
Script:
#!/bin/bash
# Check MinimalProfile definition in utils package
cat packages/utils/types/profile.ts
# Check GraphQL schema for user type
cat packages/components/schema.graphql
Length of output: 31975
packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx (1)
Line range hint 30-42: Verify subscription behavior with nullable profile.
The subscription configuration depends on profile?.id. Since we're changing how the profile is accessed and it's now coming from JWT claims, we should ensure proper handling of the nullable profile during initialization.
Let's verify the error handling for missing profile:
✅ Verification successful
Let me gather more information about the subscription and profile handling.
Subscription behavior with nullable profile is properly handled
The subscription configuration is correctly handling the nullable profile case:
- The subscription query requires a non-null profile ID (
$profileId: ID!), which means the subscription won't be initiated without a valid profile ID - The
useMemodependency onprofile?.idensures the subscription is only configured when a profile ID is available - Basic error handling is in place with
onError: console.error - The subscription will naturally not be active when profile is null since the variables won't be complete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's proper error handling for missing profile cases
# in related message subscription components
# Search for error handling patterns in message-related files
rg -A 5 "onError|catch|error" packages/components/modules/messages
Length of output: 13723
Script:
#!/bin/bash
# Check the subscription component and its usage
ast-grep --pattern 'const useMessageCountUpdateSubscription = () => {
$$$
}'
# Check how the subscription is used in components
rg -l "useMessageCountUpdateSubscription" packages/components/modules/messages
# Check the subscription query definition
rg -A 5 "MessageCountUpdateSubscription =" packages/components/modules/messages
Length of output: 1196
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx (1)
Line range hint 1-31: Verify TypeScript types for useCurrentProfile hook
The hook's return type should be updated to reflect the new interface returning currentProfile directly.
Let's check the hook's type definitions:
✅ Verification successful
TypeScript types for useCurrentProfile hook are correctly implemented
The hook's return type correctly reflects the interface returning currentProfile directly. The implementation shows that the hook returns an object with currentProfile of type MinimalProfile | null, along with setCurrentProfile and updateProfileIfActive functions. This matches the usage in the MessageItem component where currentProfile is destructured from the hook's return value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Locate and examine the TypeScript types for useCurrentProfile
# Expected: Type definitions should show currentProfile in the return type
# Find the hook's implementation and type definitions
rg "useCurrentProfile.*=.*" --type typescript -A 5
Length of output: 87
Script:
#!/bin/bash
# Let's try different approaches to locate the useCurrentProfile hook implementation
# Search for useCurrentProfile definition using ripgrep
rg "export.*useCurrentProfile" -A 10
# Also search for the hook's implementation pattern
rg "const useCurrentProfile.*=" -A 10
# Find files containing useCurrentProfile
fd -e ts -e tsx | xargs rg "useCurrentProfile"
Length of output: 6238
Script:
#!/bin/bash
# Let's examine the type definitions in the useCurrentProfile hook implementation
# Get the full content of the hook implementation file
cat packages/components/modules/profiles/useCurrentProfile/index.ts
# Also check for any type definitions that might be imported
rg "type.*MinimalProfile" -A 5
Length of output: 2888
packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx (3)
Line range hint 1-38: Implementation aligns well with PR objectives
The changes successfully simplify the CurrentProfile component while supporting the transition to cookie-based storage and SSR. The code is clean, focused, and maintains good separation of concerns.
7-7: Verify import path consistency across codebase
The import path for useCurrentProfile has changed as part of the module restructuring.
✅ Verification successful
Import path is consistent with codebase patterns
The import path ../../useCurrentProfile in CurrentProfile/index.tsx is correct and follows the established patterns in the codebase:
- Components within the ProfilePopover directory use relative imports (
../../useCurrentProfile) - Components outside the profiles module import it via the barrel file (
../../profiles) - The hook is properly exported from both
useCurrentProfile/index.tsand the profiles barrel file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent useCurrentProfile imports that might have been missed
# Expected: All imports should use the new path pattern
# Search for all useCurrentProfile imports
rg "import.*useCurrentProfile.*from" --type typescript
Length of output: 91
Script:
#!/bin/bash
# Search for useCurrentProfile imports without file type restriction since rg doesn't recognize typescript
rg "import.*useCurrentProfile.*from"
# Also search for the hook definition to understand its location
rg "export.*useCurrentProfile"
Length of output: 2083
19-19: Verify empty string handling in AvatarWithPlaceholder
The nullish coalescing operator has been removed, now directly passing potentially undefined image value. Ensure AvatarWithPlaceholder properly handles empty strings and undefined values.
✅ Verification successful
Empty string handling is safe in AvatarWithPlaceholder
The component safely handles empty strings and undefined values by:
- Extending MUI's Avatar component which handles empty/invalid src values
- Providing a fallback icon (AvatarUploadFallbackIcon) when no content is provided
- Properly forwarding all props to the underlying Avatar component
The change from profile.image ?? '' to profile.image is safe as both undefined and empty string cases will result in the fallback being displayed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AvatarWithPlaceholder implementation for empty/undefined handling
# Search for AvatarWithPlaceholder component definition
ast-grep --pattern 'export const AvatarWithPlaceholder = $$$'
Length of output: 63
Script:
#!/bin/bash
# Let's try different approaches to find AvatarWithPlaceholder implementation
# Search for AvatarWithPlaceholder using ripgrep
rg "AvatarWithPlaceholder" -A 5
# Also search for any file containing AvatarWithPlaceholder in its name
fd "AvatarWithPlaceholder"
Length of output: 19692
Script:
#!/bin/bash
# Let's examine the AvatarWithPlaceholder implementation
cat packages/design-system/components/avatars/AvatarWithPlaceholder/index.tsx
# Also check its types
cat packages/design-system/components/avatars/AvatarWithPlaceholder/types.ts
Length of output: 861
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx (3)
8-8: LGTM: Jotai import aligns with state management transition
The addition of JotaiProvider import is consistent with the PR's objective of transitioning from Zustand to Jotai for state management.
19-29: LGTM: Provider hierarchy properly restructured
The JotaiProvider is correctly positioned as the outermost provider, maintaining proper provider hierarchy while other providers remain in their logical order.
19-29: Verify test coverage for new state management
Since CurrentProfileProvider has been removed and replaced with JotaiProvider, ensure that:
- Existing tests are updated to use Jotai state management
- Test cases properly mock/provide profile data through Jotai atoms
✅ Verification successful
Let me gather more information to verify the test coverage specifically in the AccountPopover directory and check how the profile data is being handled in tests.
Let me try a different approach to search for the test files and their contents.
Let me check the content of the AccountPopover test files to see how they handle profile data and verify the state management implementation.
Let me check the implementation of the withProviders utility to see how Jotai is being used for state management in tests.
Based on the test files and implementation I've reviewed, I can now provide the final response:
Tests are properly set up with Jotai state management
The test implementation already handles the transition from CurrentProfileProvider to JotaiProvider correctly:
- The
withProvidersutility properly wraps components withJotaiProvider - Tests are using mock profile data through the Relay test environment
- Cypress tests verify profile-related functionality comprehensively:
- Profile switching
- Empty profile states
- Profile list interactions
- Custom profile-related props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential test files that might need updates
# Look for test files that might reference CurrentProfileProvider
rg -l "CurrentProfileProvider" --type ts --type tsx
# Look for existing test files in the AccountPopover directory
fd -e test.tsx -e spec.tsx . "$(dirname "$(dirname "$(dirname "$0")")")"
Length of output: 1454
Script:
#!/bin/bash
# Search for test files specifically in the AccountPopover directory
fd -e test.tsx -e spec.tsx . packages/components/modules/navigations/Header/AccountMenu/AccountPopover/
# Check how profile data is being handled in these tests
rg -l "useCurrentProfile|currentProfile" packages/components/modules/navigations/Header/AccountMenu/AccountPopover/ --type ts --type tsx
# Look for Jotai atom usage in tests
rg -l "atom|useAtom" packages/components/modules/navigations/Header/AccountMenu/AccountPopover/ --type ts --type tsx
Length of output: 419
Script:
#!/bin/bash
# Search for test files in the AccountPopover directory with more generic path
fd . packages/components --type f -e tsx -e ts | grep -i "accountpopover.*test"
# Use ast-grep to find test cases that might be using profile-related data
ast-grep --pattern 'describe($_, $_) {
$$$
test($_, $_) {
$$$
}
$$$
}'
# Look for any profile or atom-related imports in test files
rg "import.*(?:Profile|atom|useAtom)" --type-add 'tsx:*.tsx' --type tsx
Length of output: 6136
Script:
#!/bin/bash
# Check the content of AccountPopover.cy.tsx test file
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx
# Check the mock profiles implementation
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__mocks__/profiles.ts
# Check the test utils implementation
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/AccountPopoverForTesting/index.tsx
Length of output: 8545
Script:
#!/bin/bash
# Check the withProviders implementation
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx
# Look for any Jotai-related atoms in the component directory
rg -l "atom" --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' --type ts --type tsx packages/components/modules/navigations/Header/AccountMenu/AccountPopover/
# Check for any profile-related atoms that might be used
rg "atom.*profile" --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' --type ts --type tsx packages/components/
Length of output: 1474
packages/utils/index.ts (2)
34-34: LGTM! The new type export follows the established pattern.
The addition maintains consistent style with other type exports and is correctly positioned alphabetically.
34-34: Verify the profile types module and its usage.
Let's ensure the referenced module exists and check how these types are being used across the codebase.
✅ Verification successful
Let me gather more information about the profile types usage with a different approach.
Profile types module exists and is actively used across the codebase
The profile types module (packages/utils/types/profile.ts) exists and exports a MinimalProfile type that is properly used throughout the codebase, particularly in:
- Profile management components under
packages/components/modules/profiles - Message-related components for user interactions
- Authentication module as a dependency
The type export in packages/utils/index.ts is valid and serves its purpose as the main entry point for these types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the profile types module exists and analyze its usage
# Check if the profile types file exists
echo "Checking for profile types file..."
fd "profile.ts" "packages/utils/types"
# Look for imports of profile types
echo -e "\nChecking usage of profile types..."
rg -t typescript "import.*from ['\"].*utils/types/profile['\"]" -A 2
# Look for direct usage of MinimalProfile type
echo -e "\nChecking usage of MinimalProfile type..."
ast-grep --pattern 'type $VAR = MinimalProfile'
ast-grep --pattern ': MinimalProfile'
Length of output: 529
Script:
#!/bin/bash
# Check the content of the profile types file
echo "Content of profile.ts:"
cat packages/utils/types/profile.ts
# Look for imports using ripgrep with different pattern
echo -e "\nChecking usage of profile types (alternative pattern)..."
rg "from ['\"].*profile['\"]"
# Look for type references
echo -e "\nChecking type references..."
rg "Profile[\[\]\{\}\(\)<>, ]" -t ts
Length of output: 14821
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx (2)
5-5: LGTM: Import changes align with the new cookie-based storage approach
The new imports properly support the transition from localStorage to cookie-based storage for profile management.
Also applies to: 9-9
Line range hint 13-54: Verify integration with mock resolvers
The story configuration maintains proper testing infrastructure, but we should verify that the mock resolvers align with the new profile management approach.
Let's check the mock resolvers implementation:
✅ Verification successful
Let me check the complete mock resolver implementation to verify its alignment with the profile management approach.
Mock resolvers correctly aligned with profile management
The mock resolvers implementation properly reflects the profile management approach by providing:
- Current profile with id, name, image, and URL path
- List of available profiles with consistent structure
- Matches the cookie data structure set in the story
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check mock resolver implementation for profile-related resolvers
rg -A 5 'mockResolvers' packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/
Length of output: 2644
Script:
#!/bin/bash
# Get the full content of mockResolvers.ts
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/mockResolvers.ts
Length of output: 963
packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx (1)
21-21: Verify integration with cookie-based storage.
Since the PR objectives mention transitioning to cookie-based storage for profiles, let's verify that the useCurrentProfile hook properly integrates with this new storage mechanism.
✅ Verification successful
Integration with cookie-based storage verified
Looking at the implementation of useCurrentProfile, it properly integrates with cookie-based storage through:
- Using
getCookie,setCookie, andremoveCookieutilities for profile data persistence - Storing profile data under
CURRENT_PROFILE_KEYin cookies - Handling both client and server-side rendering scenarios with the
noSSRoption - Properly managing profile updates and removal through cookie operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cookie-related logic in the useCurrentProfile hook implementation
rg -A 10 "useCurrentProfile" | rg -B 5 -A 10 "cookie"
# Search for any cookie utility functions that might be used
ast-grep --pattern 'function $_(cookie$_) { $$$ }'
Length of output: 104
Script:
#!/bin/bash
# Let's first find the implementation of useCurrentProfile hook
ast-grep --pattern 'export function useCurrentProfile() {
$$$
}'
# Also search for any profile-related hooks that might be using cookies
rg -B 5 -A 10 "useProfile.*cookie"
# Search for profile storage implementation
fd "profile.*store" --type f --exec rg -l "cookie"
Length of output: 156
Script:
#!/bin/bash
# Let's try a broader search for the hook implementation
rg -B 2 -A 10 "export.*useCurrentProfile"
# Look for profile-related stores or contexts
fd "profile" --type f --exec rg -l "createContext|useState|useReducer"
# Search for any profile storage utilities
rg -B 5 -A 10 "profile.*storage"
Length of output: 638
Script:
#!/bin/bash
# Let's look at the actual implementation of useCurrentProfile
cat packages/components/modules/profiles/useCurrentProfile/index.ts
# Also check for any profile context providers
rg -B 5 -A 10 "ProfileContext|ProfileProvider"
# Look for profile-related hooks that might be using storage
rg -B 5 -A 10 "useProfile.*storage|getProfile.*storage"
Length of output: 4831
packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx (2)
Line range hint 44-64: LGTM: Well-structured updater logic
The subscription updater implementation is robust with proper null checks and follows Relay best practices for connection handling.
38-38: Consider handling undefined profile case
While the profile access change aligns with the PR objectives, we should ensure proper handling when the profile is undefined, as this could affect the subscription setup.
Consider adding a null check:
- const profile = useCurrentProfile().currentProfile
+ const { currentProfile: profile } = useCurrentProfile()
+ if (!profile) {
+ return null // or handle appropriately
+ }packages/components/.storybook/decorators/withProviders.tsx (2)
9-9: LGTM: Jotai provider import added correctly
The addition of JotaiProvider import aligns with the PR's objective to transition from Zustand to Jotai for state management.
37-49: Provider hierarchy restructured appropriately
The JotaiProvider is correctly positioned as the outermost provider, and the remaining providers maintain a logical order that respects their dependencies.
Let's verify if any stories need updates due to the removal of CurrentProfileProvider:
✅ Verification successful
Provider hierarchy changes are safe to proceed
The removal of CurrentProfileProvider is safe as stories are not directly dependent on it. The only story using profile data (AccountPopover) manages its state through cookies and mock data in its decorators, which will continue to work with the new JotaiProvider hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stories that might have been using CurrentProfileProvider
rg -l "CurrentProfileProvider" "packages/components/src/**/*.stories.tsx"
# Search for stories that might be accessing profile state
rg -l "useCurrentProfile|currentProfile" "packages/components/src/**/*.stories.tsx"
Length of output: 321
Script:
#!/bin/bash
# Let's first find the correct stories directory structure
fd --type f "stories.tsx$" packages/components/
# Then search for CurrentProfileProvider usage in all TypeScript files under components
rg -l "CurrentProfileProvider" "packages/components/"
# Search for profile state usage
rg -l "useCurrentProfile|currentProfile" "packages/components/"
Length of output: 3247
Script:
#!/bin/bash
# Let's check the content of the AccountPopover story since it's the only story that appeared in both searches
cat "packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx"
# Let's also check if there are any direct dependencies on CurrentProfileProvider in the stories
rg "CurrentProfileProvider" --type tsx --type ts packages/components/ -C 2
Length of output: 1936
packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx (1)
25-25: LGTM! The simplified destructuring aligns with the new profile management approach.
The change from { profile: currentProfile } to { currentProfile } correctly reflects the updated structure of the useCurrentProfile hook, aligning with the PR's objective of simplifying profile management.
Let's verify consistent usage of the new destructuring pattern across the codebase:
✅ Verification successful
The destructuring pattern change is consistently applied across the codebase
The verification shows that:
- No instances of the old
profile: currentProfilepattern were found - The new
{ currentProfile }destructuring pattern is consistently used across multiple components - A few components use direct access via
profile = useCurrentProfile().currentProfile
Both patterns are valid ways to access the profile data, and the codebase shows consistent usage of the new destructuring pattern in similar contexts (like message components).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old destructuring pattern
# and verify consistent usage of the new pattern
# Check for old pattern
echo "Checking for old destructuring pattern..."
rg "const.*profile:\s*currentProfile.*useCurrentProfile"
# Check for new pattern usage
echo "Checking new destructuring pattern usage..."
rg "const.*{\s*currentProfile\s*}.*useCurrentProfile"
Length of output: 443
Script:
#!/bin/bash
# Let's fix the regex patterns and try again
# Check for old pattern (profile: currentProfile)
echo "Checking for old destructuring pattern..."
rg "profile: currentProfile.*useCurrentProfile"
# Check for new pattern ({ currentProfile })
echo "Checking for new pattern usage..."
rg "{\s*currentProfile\s*}\s*=\s*useCurrentProfile"
# Let's also check the hook implementation
echo "Checking useCurrentProfile hook implementation..."
ast-grep --pattern 'export function useCurrentProfile() {
$$$
}'
Length of output: 573
Script:
#!/bin/bash
# Let's try with simpler patterns and escape the curly braces
# Check for old pattern (profile: currentProfile)
echo "Checking for old destructuring pattern..."
rg "profile: currentProfile"
# Check for new pattern ({ currentProfile })
echo "Checking for new pattern usage..."
rg "\{ currentProfile \}"
# Let's also find all files that use useCurrentProfile
echo "Finding files using useCurrentProfile..."
rg "useCurrentProfile"
# Find the hook definition
echo "Looking for useCurrentProfile definition..."
fd useCurrentProfile
Length of output: 5783
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (2)
14-14: LGTM: Import changes align with profile simplification strategy
The addition of getMinimalProfile import aligns with the PR's objective to streamline profile management.
23-23: Verify consistent hook usage pattern across components
The simplified destructuring of useCurrentProfile looks good and aligns with the refactoring goals.
Let's verify this pattern is consistently applied across other components:
✅ Verification successful
Hook usage patterns are consistent with the refactoring
Looking at all usages of useCurrentProfile across the codebase, we can see two consistent patterns:
- Direct destructuring:
const { currentProfile, setCurrentProfile } = useCurrentProfile() - Single property access:
const profile = useCurrentProfile().currentProfile
Both patterns are clean and appropriate for their use cases. The changes in the ProfilesList component align with these established patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining nested destructuring patterns of useCurrentProfile
rg "const \{.*profile.*\} = useCurrentProfile\(\)" --type typescript
Length of output: 105
Script:
#!/bin/bash
# Search for useCurrentProfile hook usage patterns
rg "const.*useCurrentProfile" -t tsx -t ts
Length of output: 72
Script:
#!/bin/bash
# Search for useCurrentProfile hook usage patterns across all files
rg "const.*useCurrentProfile"
Length of output: 2118
packages/components/modules/profiles/useCurrentProfile/index.ts (3)
1-19: LGTM! Well-organized imports and correct Next.js directive placement.
The imports are logically grouped and include all necessary dependencies for the implementation.
38-40: LGTM! Proper atom initialization with cookie data.
The atom is correctly typed and initialized with the profile from cookie storage.
77-82: Verify event listener cleanup.
The cleanup function in the useEffect hook looks correct, but let's verify there are no memory leaks.
✅ Verification successful
Event listener cleanup implementation is correct and consistent
The verification shows two event listener implementations in the codebase:
- In
useCurrentProfile: UseseventEmitter.on()with correspondingeventEmitter.off()in cleanup - In
useEventSubscription: UseseventEmitter.on()witheventEmitter.removeListener()in cleanup
Both implementations properly handle cleanup in their respective useEffect hooks. The useCurrentProfile implementation follows the correct pattern of registering the event listener and cleaning it up when the component unmounts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of eventEmitter to ensure consistent cleanup patterns
rg "eventEmitter\.(on|off)" -A 5 -B 5
Length of output: 1860
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx (2)
36-36: LGTM: Profile retrieval supports SSR
The implementation correctly uses useCurrentProfile with noSSR: false to ensure server-side rendering compatibility, aligning with the PR's objective of supporting SSR through cookie-based profile storage.
69-69: Verify fallback behavior for missing profile images
While the change to use profile?.image ?? '' is correct following the removal of the avatar property, ensure that the empty string fallback provides an acceptable visual experience.
✅ Verification successful
Empty string fallback is safe due to built-in placeholder
The empty string fallback ('') is safe to use as the AvatarWithPlaceholder component has a built-in fallback mechanism. When no image source is provided, it automatically renders an AvatarUploadFallbackIcon component, ensuring a consistent visual experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom avatar fallback implementations or default avatar handling
rg -A 3 'ClickableAvatar' --type tsx
Length of output: 64
Script:
#!/bin/bash
# Search for any custom avatar fallback implementations or default avatar handling
rg -A 3 'ClickableAvatar'
Length of output: 4387
Script:
#!/bin/bash
# Search for the Avatar component usage and its props
rg -A 3 'Avatar' --type ts --type tsx --type jsx
Length of output: 76
Script:
#!/bin/bash
# Find all files that might contain avatar-related components
fd -e tsx -e ts -e jsx Avatar
Length of output: 31
Script:
#!/bin/bash
# Search for the base Avatar component implementation
rg -A 5 'AvatarWithPlaceholder' packages/design-system/components/avatars/
Length of output: 6803
Script:
#!/bin/bash
# Look for the actual implementation of ClickableAvatar
cat packages/design-system/components/avatars/ClickableAvatar/index.tsx
Length of output: 858
Script:
#!/bin/bash
# Look for the AvatarWithPlaceholder implementation to understand the fallback behavior
cat packages/design-system/components/avatars/AvatarWithPlaceholder/index.tsx
Length of output: 634
packages/components/modules/messages/SendMessage/index.tsx (1)
78-78: LGTM: Profile access simplification
The simplified destructuring aligns with the new state management approach and maintains consistency across components.
packages/components/package.json (1)
32-32: LGTM! Dependencies align with the PR objectives.
The addition of jotai and presence of js-cookie align perfectly with the PR's objectives of transitioning to Jotai for state management and implementing cookie-based profile storage.
packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx (1)
30-30: LGTM! Property renaming is consistent with the new profile management approach.
The change from profile to currentProfile aligns well with the PR's objective of simplifying profile management.
...ules/profiles/useCurrentProfile/__tests__/__utils__/useAndInitializeCurrentProfile/index.tsx
Outdated
Show resolved
Hide resolved
...s/modules/profiles/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx
Outdated
Show resolved
Hide resolved
...ages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
Outdated
Show resolved
Hide resolved
...ages/components/modules/profiles/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/useCurrentProfile/index.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/profiles/useCurrentProfile/index.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
packages/authentication/modules/access/useLogin/index.ts (1)
Line range hint
51-59: Add error handling for JWT decoding and profile extraction.While the implementation aligns with the PR objectives, consider adding proper error handling:
- Handle potential JWT decode failures
- Validate the decoded user data structure
- Add null check for user.profile
Consider this safer implementation:
async function handleLoginSuccess( response: LoginJWTResponse | LoginChangeExpiredPasswordRedirectResponse, ) { if (isLoginChangeExpiredPasswordRedirectResponse(response)) { return } - const user = decodeJWT<User>(response.access) - if (user) { - setCurrentProfile(user.profile) - } + try { + const user = decodeJWT<User>(response.access) + if (!user) { + throw new Error('Failed to decode user from JWT') + } + if (!user.profile) { + throw new Error('User profile is missing in JWT') + } + setCurrentProfile(user.profile) + } catch (error) { + console.error('Failed to extract profile from JWT:', error) + throw error + } await setTokenAsync(accessKeyName, response.access, {packages/authentication/types/profile.ts (1)
1-6: Add JSDoc documentation for the MinimalProfile type.The type structure looks good and aligns well with the simplified profile management objectives. Consider adding JSDoc documentation to describe the type's purpose and usage in SSR/JWT context.
+/** + * Represents a minimal user profile structure used for SSR and JWT claims. + * Contains essential profile information that can be safely stored in cookies + * and transmitted between client and server. + */ export type MinimalProfile = { id: string name: string | null image: string | null urlPath: string | null }packages/components/modules/profiles/graphql/queries/UserProfile.ts (1)
Line range hint
3-3: Remove outdated TODO comment.The TODO comment about removing inline fragment can be removed as it has been addressed with the transition to named fragments.
-// TODO: remove inline fragment export const UserProfileQuery = graphql`packages/authentication/modules/profile/index.ts (1)
1-5: Add JSDoc documentation for the new exportsWhile the exports are well-organized, adding JSDoc documentation would help developers understand:
- The purpose and usage of useCurrentProfile hook
- The structure and requirements of MinimalProfile
- The role of InitialProfileProvider in testing
Example documentation:
+/** + * Hook for accessing and managing the current user profile + * @returns {Object} Object containing currentProfile and profile management methods + */ export { default as useCurrentProfile } from './useCurrentProfile' +/** + * Minimal representation of a user profile for efficient state management + */ export type { MinimalProfile } from '../../types/profile' +/** + * Provider component for testing with initial profile state + * @component + */ export { InitialProfileProvider } from './useCurrentProfile/__tests__/__utils__/TestComponentWithProvider'packages/components/modules/profiles/graphql/index.ts (1)
1-3: Consider using a type guard for profile dataAdding a type guard would improve type safety when working with profile data from GraphQL.
function isValidProfileData( profile: unknown ): profile is Omit<ProfileItemFragment$data, ' $fragmentType'> { return typeof profile === 'object' && profile !== null && 'id' in profile; }packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx (1)
22-37: Consider improving type safety for props handlingThe use of
Partial<InitialProfileProp>allows undefined values, but the component specifically handles undefined as a special case. Consider making this more explicit in the type system.-const TestComponentWithProviders = (props: Partial<InitialProfileProp>) => { +type TestComponentWithProvidersProps = { + initialProfile?: MinimalProfile | null; +}; + +const TestComponentWithProviders = ({ initialProfile }: TestComponentWithProvidersProps) => { - if (props.initialProfile === undefined) { + if (initialProfile === undefined) {packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__mocks__/profiles.ts (1)
Line range hint
14-46: Consider simplifying the mock profiles list structureThe mockProfilesListFactory maintains a deeply nested structure (
data.me.profiles) while transforming the MinimalProfile into a different shape. This might make tests more complex than necessary.Consider simplifying to match the MinimalProfile structure directly:
export const mockProfilesListFactory = (size: number, userProfile: MinimalProfile) => { - return { - data: { - me: { - profiles: [ - { - id: userProfile.id, - name: userProfile.name, - image: { - url: userProfile.image, - }, - urlPath: { - path: userProfile.urlPath, - }, - }, + return [ + userProfile, ...Array.from({ length: size }).map((_, index) => ({ id: `profile-${index}`, name: faker.person.fullName(), - image: { - url: faker.image.avatar(), - }, - urlPath: { - path: faker.internet.url(), - }, + image: faker.image.avatar(), + urlPath: faker.internet.url(), })), - ], - }, - }, - } + ]packages/authentication/modules/profile/useCurrentProfile/index.ts (1)
51-55: Consider adding validation for newProfileThe
updateProfileIfActivefunction should validate the newProfile object before updating.const updateProfileIfActive = (newProfile: MinimalProfile) => { + if (!newProfile || typeof newProfile !== 'object') { + console.warn('Invalid profile object provided') + return + } if (currentProfile?.id === newProfile.id) { setCurrentProfile(newProfile) } }packages/authentication/modules/user/useUpdateUser/index.ts (1)
17-17: Consider adding retry logic for token refreshSetting the profile to null immediately on token refresh failure might lead to unnecessary logouts. Consider implementing a retry mechanism before clearing the profile.
const { setCurrentProfile } = useCurrentProfile() const mutation = useMutation({ mutationFn: (params: UserUpdateParams<TUser>) => ApiClass.updateUser<TUser>(params), onSettled: async (data, error, variables, context) => { queryClient.invalidateQueries({ queryKey: USER_API_KEY.getUser() }) try { await refreshAccessToken(accessKeyName, refreshKeyName) } catch (e) { - // silently fail - // eslint-disable-next-line no-console - setCurrentProfile(null) - console.error(e) + let retryCount = 3 + while (retryCount > 0) { + try { + await new Promise(resolve => setTimeout(resolve, 1000)) + await refreshAccessToken(accessKeyName, refreshKeyName) + return + } catch (retryError) { + retryCount-- + if (retryCount === 0) { + console.error('Token refresh failed after retries:', retryError) + setCurrentProfile(null) + } + } + } } options?.onSettled?.(data, error, variables, context) },Also applies to: 28-28
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx (1)
61-61: Consider adding test coverage for profile switching during SSRWhile the current tests cover client-side profile switching, consider adding tests that verify the behavior when switching profiles with SSR enabled.
Also applies to: 70-71, 74-74, 103-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between c63de9ca96cb172f576ac812b23cf75b3eea3e78 and 2930dc49b22bebbb15d89f64ab20b8d483e0bfd1.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ProfileItemInlineFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfilesListQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserProfileQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (41)
packages/authentication/index.ts(1 hunks)packages/authentication/modules/access/useLogin/index.ts(4 hunks)packages/authentication/modules/profile/index.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__mock__/profiles.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/constants.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/index.ts(1 hunks)packages/authentication/modules/user/useUpdateUser/index.ts(3 hunks)packages/authentication/package.json(2 hunks)packages/authentication/types/profile.ts(1 hunks)packages/authentication/types/user.ts(1 hunks)packages/components/modules/comments/CommentCreate/index.tsx(3 hunks)packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/index.tsx(0 hunks)packages/components/modules/comments/Comments/index.tsx(1 hunks)packages/components/modules/comments/CommentsList/index.tsx(0 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(2 hunks)packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(2 hunks)packages/components/modules/messages/SendMessage/index.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx(0 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx(7 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__mocks__/profiles.ts(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(3 hunks)packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx(3 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/modules/profiles/graphql/index.ts(1 hunks)packages/components/modules/profiles/graphql/queries/ProfileItemInline.ts(0 hunks)packages/components/modules/profiles/graphql/queries/ProfilesList.ts(1 hunks)packages/components/modules/profiles/graphql/queries/UserProfile.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/comments/CommentItem/index.tsx
- packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/storybook/stories.tsx
- packages/components/modules/profiles/graphql/queries/ProfileItemInline.ts
- packages/components/modules/comments/CommentsList/index.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/authentication/modules/profile/useCurrentProfile/constants.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/components/package.json
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/modules/profiles/index.ts
- packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
- packages/components/modules/messages/MessagesList/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx
- packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/SendMessage/index.tsx
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx
- packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/authentication/types/user.ts
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts
- packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx
🔇 Additional comments (24)
packages/authentication/package.json (1)
18-18: LGTM: Dependencies align with PR objectives
The new dependencies are appropriate:
jotai: Required for the new state management approach@faker-js/faker: Added as a dev dependency, following the project's versioning pattern for test dependencies
Also applies to: 32-32
packages/components/modules/comments/Comments/index.tsx (2)
Line range hint 1-57: LGTM! Changes align with PR objectives
The component has been successfully refactored to:
- Remove direct profile ID passing
- Maintain clean component structure
- Preserve existing functionality
54-54: Verify CommentCreate integration with current profile context
The removal of profileId prop aligns with the PR objectives of simplifying profile management. However, we should verify that the CommentCreate component is properly integrated with the new profile context.
✅ Verification successful
CommentCreate integration with current profile context is properly implemented
The CommentCreate component is correctly integrated with the new profile context:
- It imports and uses the
useCurrentProfilehook - While
profileIdstill exists in the props interface, the component internally usescurrentProfile.idfrom the hook - This implementation allows for backward compatibility while defaulting to the new context-based approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CommentCreate component's integration with current profile context
# Check if CommentCreate uses useCurrentProfile hook
ast-grep --pattern 'import { useCurrentProfile } from "@baseapp-frontend/authentication"' packages/components/modules/comments/CommentCreate/index.tsx
# Verify CommentCreate props type definition doesn't include profileId
ast-grep --pattern 'interface CommentCreateProps {
$$$
profileId?: $_
$$$
}' packages/components/modules/comments/CommentCreate/types.ts
Length of output: 930
Script:
#!/bin/bash
# Let's check the implementation of CommentCreate to see how profileId is used
ast-grep --pattern 'const CommentCreate = ($_) => {
$$$
}' packages/components/modules/comments/CommentCreate/index.tsx
# Also check for any direct usage of profileId prop
rg "profileId" packages/components/modules/comments/CommentCreate/index.tsx -A 2
Length of output: 587
Script:
#!/bin/bash
# Let's verify if CommentCreate is using useCurrentProfile hook
ast-grep --pattern 'const $_ = useCurrentProfile()' packages/components/modules/comments/CommentCreate/index.tsx
# Check for any imports from authentication package
rg "from.*authentication" packages/components/modules/comments/CommentCreate/index.tsx
Length of output: 387
packages/authentication/modules/access/useLogin/index.ts (3)
8-8: LGTM! New imports align with PR objectives.
The added imports support the transition to JWT-based profile management and follow good practices with absolute imports.
Also applies to: 25-25, 31-31
45-45: LGTM! Clean hook initialization.
The destructuring of setCurrentProfile from useCurrentProfile follows React hooks best practices.
Line range hint 117-137: Consider addressing type-related technical debt.
There are multiple TODOs and any type assertions in the form submission handlers. This technical debt could lead to type-safety issues.
Let's check for similar patterns in other files:
Would you like help creating a follow-up issue to track the type refactoring work?
packages/authentication/modules/profile/useCurrentProfile/__tests__/__mock__/profiles.ts (1)
1-10: LGTM! Clean and straightforward mock factory implementation.
The factory function provides a good foundation for generating test data with realistic values using faker.
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx (3)
3-3: LGTM! Import changes align with the transition to Jotai
The new imports correctly introduce Jotai for state management and InitialProfileProvider for profile handling, following the architectural changes described in the PR objectives.
Also applies to: 9-9
19-23: Verify InitialProfileProp type definition
The HOC correctly extends its props with InitialProfileProp. Let's verify the type definition to ensure it matches the JWT claims structure.
✅ Verification successful
Let me gather more information about the MinimalProfile type to ensure it matches the JWT claims structure.
InitialProfileProp type definition is correctly structured
The InitialProfileProp type is well-defined and properly used in the codebase. It contains a single property initialProfile of type MinimalProfile | null, where MinimalProfile includes essential user profile fields (id, name, image, urlPath) that can be derived from JWT claims. The type is correctly imported from the authentication package and used consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the InitialProfileProp type definition and its usage
# Expected: Type should include fields that match JWT profile claims
ast-grep --pattern 'type InitialProfileProp = {
$$$
}'
# Check other usages of InitialProfileProp
rg 'InitialProfileProp' -A 3
Length of output: 3845
Script:
#!/bin/bash
# Check MinimalProfile type definition
ast-grep --pattern 'type MinimalProfile = {
$$$
}'
# Also check for interface definition
ast-grep --pattern 'interface MinimalProfile {
$$$
}'
# Search for any JWT claim type definitions
rg -g '*.ts' -g '*.tsx' 'type.*Claims' -A 5
Length of output: 532
24-36: Provider hierarchy looks good, but verify SSR compatibility
The provider hierarchy is well-structured with Jotai as the outermost provider. Since the PR objectives mention SSR support, we should verify that this hierarchy works correctly with SSR.
✅ Verification successful
Let me gather more information about the SSR setup and Jotai configuration.
Let me try one more search with corrected patterns to find SSR-related configurations:
Based on the search results, I can now provide the final response:
Provider hierarchy is SSR-compatible, no issues found
The current provider hierarchy is SSR-safe because:
- JotaiProvider is correctly positioned as the outermost provider, which is the recommended setup for Jotai's SSR support
- The code shows proper atom hydration patterns in the codebase (via
useHydrateAtoms) - The test utilities demonstrate consistent provider nesting that matches the production setup
- InitialProfileProvider properly hydrates the profile atom state, which is essential for SSR
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for SSR-related configurations and usage patterns
# Expected: Should find SSR-specific handling in related files
# Look for SSR-related configurations in Next.js pages
fd '_app.tsx|_document.tsx' --exec rg -A 5 'JotaiProvider|InitialProfileProvider' {}
# Check for hydration handling
rg 'useHydrateAtoms|getDefaultStore'
Length of output: 452
Script:
#!/bin/bash
# Check for Jotai SSR-specific setup and provider usage patterns
rg -A 5 'Provider.*=.*{' packages/components/
rg -A 5 'createStore|getDefaultStore|useStore' packages/components/
# Look for any SSR-specific configurations or hydration logic
rg -A 5 'getServerSideProps|getInitialProps|hydrateAtoms' packages/components/
# Check if there are any specific SSR configurations in the test utils
fd 'test-utils|__utils__' --exec rg -A 5 'JotaiProvider|Provider.*=.*{' {}
Length of output: 2696
Script:
#!/bin/bash
# Search for Jotai provider usage and SSR configurations
rg -A 5 "JotaiProvider"
# Look for any Provider definitions and SSR setup
rg -A 5 "export.*Provider.*=.*\{"
# Check for hydration or SSR-specific code in the component's directory
rg -A 5 "useHydrateAtoms|getServerSideProps|getInitialProps" packages/components/modules/navigations/Header/AccountMenu/AccountPopover/
# Check for any Jotai store configuration
rg -A 5 "createStore|Provider.*children" packages/components/modules/navigations/Header/
Length of output: 9560
packages/components/modules/profiles/graphql/queries/ProfilesList.ts (1)
7-7: LGTM! Verify fragment implementation.
The transition to named fragments improves code reusability and maintainability.
✅ Verification successful
LGTM! Fragment implementation is properly defined and consistently used.
The ProfileItemFragment is well-defined in packages/components/modules/profiles/graphql/queries/ProfileItem.ts and is correctly used across the codebase:
- Properly defined with fields: id, name, image, and urlPath
- Used consistently in ProfilesList, UserProfile, and AllProfilesList queries
- No remaining inline fragments found that need conversion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ProfileItemFragment implementation and consistent usage
# Check for fragment definition
ast-grep --pattern 'fragment ProfileItemFragment on Profile {
$$$
}'
# Check for any remaining inline fragments that should be converted
rg -l "...ProfileItemInlineFragment"
Length of output: 109
Script:
#!/bin/bash
# Let's try a broader search for the fragment and its usage
# Search for ProfileItemFragment definition and usage
rg "ProfileItemFragment" -A 5
# Search for any file containing Profile-related fragments
fd "Profile.*\.(graphql|ts)" --exec cat {}
Length of output: 48805
packages/components/modules/profiles/graphql/queries/UserProfile.ts (1)
8-8: LGTM! Fragment change is consistent.
The use of ProfileItemFragment aligns with the codebase-wide standardization of profile fragments.
packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx (1)
1-4: LGTM! Verify test coverage for new Jotai implementation
The changes correctly reflect the new hook API structure. The component properly tests the currentProfile access pattern.
✅ Verification successful
The test results show comprehensive test coverage with proper Jotai implementation:
- There's a dedicated test file
CurrentProfileProvider.test.tsx - The tests include proper Jotai setup with
JotaiProviderand customInitialProfileProvider - The test utilities are properly structured with provider wrappers
Test coverage for the new Jotai implementation looks complete and well-structured
The codebase shows proper test coverage with:
- Dedicated provider testing
- Proper Jotai provider setup
- Test utilities with provider wrappers
- Integration tests with profile data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for comprehensive test coverage of the new Jotai implementation
# Look for other test files that should be updated
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | xargs rg "useCurrentProfile"
# Check for Jotai-specific test utilities
rg "Provider|atom" "packages/authentication/modules/profile/useCurrentProfile/__tests__"
Length of output: 2241
packages/components/modules/profiles/graphql/index.ts (1)
1-14: Verify SSR hydration handling
While the code structure looks good, ensure that Jotai's hydration process is properly handled during SSR, especially with the new cookie-based profile storage mentioned in the PR objectives.
✅ Verification successful
Based on the search results, I can now generate the final response:
SSR hydration is properly handled with Jotai
The codebase shows proper implementation of Jotai's SSR hydration:
- JotaiProvider is correctly set up in test utilities, indicating awareness of provider requirements
- The profile state management is using atoms (as seen in the imports referencing profileAtom)
- The app is using Next.js App Router (evidenced by numerous 'use client' directives), which handles hydration automatically
- Cookie handling is properly implemented in the utils package with SSR-aware functions that check for server/client context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper SSR hydration setup
# Look for Jotai hydration configuration
rg -A 5 "hydrate|dehydrate" "packages/authentication"
# Check for cookie handling in SSR context
rg -A 5 "getServerSideProps|getInitialProps" .
Length of output: 102
Script:
#!/bin/bash
# Let's try a broader search for SSR and cookie-related implementations
# Check for cookie handling implementations
rg -A 5 "cookie" .
# Look for Jotai provider setup in app or page components
rg -A 5 "Provider.*jotai|Jotai.*Provider" .
# Check for profile-related SSR implementations
rg -A 5 "SSR|serverSide|getStatic" .
# Look for hydration-related code in the authentication package
fd . packages/authentication -t f -e ts -e tsx --exec cat {} | rg -A 5 "hydrat|initialState"
Length of output: 69128
Script:
#!/bin/bash
# Let's check for Jotai-specific SSR and hydration setup in the codebase
# Look for Jotai provider setup in app/page components
rg -A 5 "Provider.*from.*jotai|from.*jotai.*Provider" .
# Search for Jotai hydration/initialization code
rg -A 5 "hydrateAtoms|createStore|initializeAtom" .
# Check for profile-related atom definitions and their initialization
rg -A 5 "atom\(.*profile|Profile.*atom" .
# Look for Next.js app router setup which might handle hydration
fd . -t f -e tsx -e ts --exec rg -l "use client|export const metadata"
Length of output: 9067
packages/authentication/index.ts (1)
3-3: LGTM! Export aligns with profile management restructuring
The new export follows the established barrel pattern and correctly exposes the new profile module, which is essential for the Jotai-based profile management system.
packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx (1)
1-21: LGTM! Well-structured test utility implementation
The implementation provides a clean way to initialize profile state for testing, properly utilizing Jotai's useHydrateAtoms hook and following React best practices with proper typing.
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__mocks__/profiles.ts (2)
1-11: LGTM! Clean implementation of profile factory
The mockUserProfileFactory correctly implements the MinimalProfile structure, making it consistent with the new profile management approach.
47-47: LGTM! Simplified null profile representation
The change to use direct null value aligns well with the new profile management approach.
packages/authentication/modules/profile/useCurrentProfile/index.ts (1)
59-64: LGTM: Event cleanup implementation
The cleanup of event listeners is properly implemented in the useEffect hook.
packages/components/modules/comments/CommentCreate/index.tsx (2)
5-5: LGTM: Profile management refactor aligns with PR objectives
The changes correctly implement the transition from prop-based profile ID to using the new useCurrentProfile hook, which aligns with the PR's objective of simplifying profile management.
Also applies to: 79-82
109-109: Verify null safety handling for currentProfile
The optional chaining operator is correctly used here, but we should ensure the mutation handles the case where currentProfile is null appropriately.
✅ Verification successful
From the gathered information, I can now provide a final response:
Optional chaining for currentProfile?.id is sufficient here
The null safety handling is appropriate for this case because:
- The
profileIdfield inCommentCreateInputis explicitly marked as optional in the GraphQL schema (profileId?: string | null | undefined) - The component uses proper form validation through
zodResolverandSOCIAL_UPSERT_FORM_VALIDATION_SCHEMA - The mutation handles errors through its
errorsfield in the response type, which can capture any server-side validation issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling related to null profileId in the mutation
ast-grep --pattern 'mutation CommentCreate($input: CommentCreateInput!) {
$$$
commentCreate(input: $input) {
$$$
errors {
$$$
}
$$$
}
}'
Length of output: 166
Script:
#!/bin/bash
# Let's first find the GraphQL mutation definition file
fd "CommentCreate.*graphql" --type f --exec cat {}
# Also search for any validation or error handling in the component
rg -A 5 "profileId.*currentProfile" packages/components/modules/comments/CommentCreate/
Length of output: 17329
Script:
#!/bin/bash
# Let's check the component's validation and error handling
rg -B 10 -A 5 "CommentCreate.*mutation" packages/components/modules/comments/CommentCreate/index.tsx
# Also check if there's any validation before the mutation
rg -B 5 "const.*CommentCreate" packages/components/modules/comments/CommentCreate/index.tsx
Length of output: 2255
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx (3)
31-33: LGTM: Test setup properly handles null profile case
The test correctly validates component behavior when no initial profile is present, which is important for SSR scenarios.
49-51: LGTM: Profile data structure simplified
The test updates correctly reflect the simplified profile data structure, making assertions more straightforward and maintainable.
Also applies to: 55-56
115-115: LGTM: Custom props testing maintained
The test modifications preserve the comprehensive coverage of custom props while adapting to the new profile management system.
Also applies to: 136-136, 153-153
packages/authentication/modules/profile/useCurrentProfile/__tests__/__mock__/profiles.ts
Outdated
Show resolved
Hide resolved
packages/authentication/modules/profile/useCurrentProfile/index.ts
Outdated
Show resolved
Hide resolved
...s/authentication/modules/profile/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx
Outdated
Show resolved
Hide resolved
...s/authentication/modules/profile/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx
Outdated
Show resolved
Hide resolved
tsl-ps2
force-pushed
the
BA-1809-simplify-currentprofileprovider-squash
branch
from
2930dc4 to
03b0268
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx (1)
18-23: Add JSDoc to document the withProviders HOCConsider adding JSDoc documentation to explain the purpose of this HOC and its parameters, especially the new initialProfile parameter.
+/** + * HOC that wraps components with necessary providers for testing + * @param Component - The component to wrap + * @returns A wrapped component with all required providers + */ const withProviders =packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx (2)
31-33: Consider removing unused GraphQL environment setup.Since the profile is now passed directly via props and async operations have been removed, the GraphQL environment setup might be unnecessary for this test case.
- const { environment } = createTestEnvironment() - - cy.mount(<AccountPopoverForTesting environment={environment} initialProfile={null} />) + cy.mount(<AccountPopoverForTesting initialProfile={null} />)
70-71: Consider adding type guards instead of non-null assertions.The use of non-null assertions (
!.) might mask potential runtime issues. Consider adding type guards for better type safety.- cy.contains('li', profile.name!).should('exist') - cy.contains('li', profile.urlPath.path!).should('exist') + if (profile.name && profile.urlPath.path) { + cy.contains('li', profile.name).should('exist') + cy.contains('li', profile.urlPath.path).should('exist') + } - cy.findByLabelText(`Switch to ${profileListData.data.me.profiles[1]!.name}`).click() + const targetProfile = profileListData.data.me.profiles[1] + if (targetProfile?.name) { + cy.findByLabelText(`Switch to ${targetProfile.name}`).click() + }Also applies to: 74-74
packages/authentication/modules/profile/useCurrentProfile/index.ts (1)
59-64: Consider adding cleanup for profile on unmountThe effect cleanup only removes the event listener but doesn't clean up the profile state.
useEffect(() => { eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile) return () => { eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile) + // Clean up profile state when component unmounts + setProfile(null) } }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 2930dc49b22bebbb15d89f64ab20b8d483e0bfd1 and 03b0268186978d73b50c0a34b38e22020fa69185.
⛔ Files ignored due to path filters (3)
packages/components/__generated__/ProfileItemInlineFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfilesListQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserProfileQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (51)
packages/authentication/index.ts(1 hunks)packages/authentication/jest.config.ts(1 hunks)packages/authentication/modules/access/useLogin/index.ts(4 hunks)packages/authentication/modules/profile/index.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__mock__/profiles.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponent/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/TestComponentWithProvider/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/constants.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/index.ts(1 hunks)packages/authentication/modules/user/useUpdateUser/index.ts(3 hunks)packages/authentication/package.json(2 hunks)packages/authentication/types/profile.ts(1 hunks)packages/authentication/types/user.ts(1 hunks)packages/components/.storybook/decorators/withProviders.tsx(2 hunks)packages/components/__tests__/dummy.test.ts(1 hunks)packages/components/modules/comments/CommentCreate/index.tsx(3 hunks)packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/index.tsx(0 hunks)packages/components/modules/comments/Comments/index.tsx(1 hunks)packages/components/modules/comments/CommentsList/index.tsx(0 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(2 hunks)packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(2 hunks)packages/components/modules/messages/SendMessage/index.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx(0 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx(7 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__mocks__/profiles.ts(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(3 hunks)packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx(3 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/CurrentProfileProvider.test.tsx(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__mock__/profiles.ts(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__utils__/TestComponentWithProvider/index.tsx(0 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/graphql/index.ts(1 hunks)packages/components/modules/profiles/graphql/queries/ProfileItemInline.ts(0 hunks)packages/components/modules/profiles/graphql/queries/ProfilesList.ts(1 hunks)packages/components/modules/profiles/graphql/queries/UserProfile.ts(1 hunks)packages/components/modules/profiles/index.ts(1 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (12)
- packages/components/modules/profiles/context/useCurrentProfile/index.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/utils/TestComponentWithProvider/index.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/types.ts
- packages/components/modules/profiles/graphql/queries/ProfileItemInline.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/mock/profiles.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/constants.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/index.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/CurrentProfileProvider.test.tsx
- packages/components/modules/comments/CommentItem/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/storybook/stories.tsx
- packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx
- packages/components/modules/comments/CommentsList/index.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/components/tests/dummy.test.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- packages/authentication/modules/profile/useCurrentProfile/constants.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/mock/profiles.ts
- packages/authentication/types/profile.ts
- packages/authentication/modules/profile/index.ts
- packages/components/modules/profiles/graphql/index.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/utils/TestComponentWithProvider/index.tsx
- packages/authentication/index.ts
- packages/components/package.json
- packages/components/modules/profiles/graphql/queries/UserProfile.ts
- packages/components/modules/profiles/index.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/utils/TestComponent/index.tsx
- packages/components/modules/profiles/graphql/queries/ProfilesList.ts
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/comments/Comments/index.tsx
- packages/components/modules/messages/MessagesList/index.tsx
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
- packages/authentication/package.json
- packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx
- packages/components/modules/messages/SendMessage/index.tsx
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx
- packages/components/.storybook/decorators/withProviders.tsx
- packages/authentication/modules/user/useUpdateUser/index.ts
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx
- packages/components/modules/comments/CommentCreate/index.tsx
- packages/authentication/types/user.ts
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts
- packages/authentication/modules/access/useLogin/index.ts
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/tests/mocks/profiles.ts
- packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/authentication/modules/profile/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (9)
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx (2)
3-9: LGTM! Import statements align with the state management transition
The new imports correctly support the transition from Zustand to Jotai and the introduction of InitialProfileProvider.
18-36: Verify provider hierarchy matches production code
The provider hierarchy looks correct with JotaiProvider at root and InitialProfileProvider replacing CurrentProfileProvider. Let's verify this matches the production setup.
✅ Verification successful
Provider hierarchy matches production code and follows best practices
The test provider hierarchy is correctly implemented:
- JotaiProvider at root is correct as it's needed for the profileAtom (defined in authentication module)
- InitialProfileProvider after JotaiProvider is proper as it uses Jotai's useHydrateAtoms
- Other providers (Query, Relay, Theme, Notification) maintain consistent order with other test utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify provider hierarchy in production code matches test setup
# Expected: Similar provider structure in main App or layout components
# Search for JotaiProvider usage in main app components
rg -A 5 "JotaiProvider" --type tsx
# Search for InitialProfileProvider usage
rg -A 5 "InitialProfileProvider" --type tsx
Length of output: 134
Script:
#!/bin/bash
# Let's try without file type filtering since tsx is not recognized
# Search for provider hierarchy in main app components
rg -A 10 "JotaiProvider"
# Search for InitialProfileProvider usage
rg -A 10 "InitialProfileProvider"
# Also search for potential variations of the file extension
rg -g "*.ts*" -A 10 "JotaiProvider"
Length of output: 22228
Script:
#!/bin/bash
# Let's search for the main app or root component to verify production setup
rg -g "*.ts*" -A 10 "App.*=.*" --multiline
# Also search for any other potential provider wrapper components
rg -g "*.ts*" -A 10 "Providers.*=.*" --multiline
# Search for the actual profile atom implementation
rg -g "*.ts*" "profileAtom.*=" --multiline
Length of output: 20216
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx (3)
49-51: LGTM! Profile assertions are simplified.
The test setup and assertions are now more straightforward, directly accessing profile properties instead of navigating through nested structures.
Also applies to: 55-56
115-115: LGTM! Custom props test coverage is maintained.
The test maintains comprehensive coverage of custom props while adapting to the new profile structure.
Also applies to: 153-153
Line range hint 1-190: Consider adding tests for cookie-based profile storage.
The PR objectives mention saving the current profile to a cookie for SSR support, but this functionality isn't covered in the test suite.
Let's check if cookie-related tests exist elsewhere:
Would you like help creating test cases for cookie-based profile storage?
packages/authentication/jest.config.ts (1)
1-6: LGTM! Jest configuration is well-structured.
The configuration properly extends the base config while safely adding new test patterns.
packages/authentication/modules/profile/useCurrentProfile/__tests__/CurrentProfileProvider.test.tsx (2)
13-35: 🛠️ Refactor suggestion
Enhance test coverage for CurrentProfileProvider
The test suite currently only covers the logout scenario. Consider adding tests for:
- Profile initialization
- Profile updates via
setCurrentProfile - SSR vs client-side rendering scenarios
- Error handling for cookie operations
Would you like me to help generate additional test cases?
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
18-18:
Remove .only from test case
The .only modifier prevents other tests from running. This should be removed before merging.
- it.only('should erase current profile when user logs out', async () => {
+ it('should erase current profile when user logs out', async () => {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
packages/authentication/modules/profile/useCurrentProfile/index.ts (1)
37-49: 🛠️ Refactor suggestion
Add error handling and secure cookie options
The cookie operations should include error handling and secure options.
const setCurrentProfile = (newProfile: MinimalProfile | null) => {
if (newProfile === null) {
setProfile(() => {
- removeCookie(CURRENT_PROFILE_KEY)
+ try {
+ removeCookie(CURRENT_PROFILE_KEY)
+ } catch (error) {
+ console.error('Failed to remove profile cookie:', error)
+ }
return null
})
} else {
setProfile(() => {
- setCookie(CURRENT_PROFILE_KEY, newProfile, { stringfyValue: true })
+ try {
+ setCookie(CURRENT_PROFILE_KEY, newProfile, {
+ stringfyValue: true,
+ httpOnly: true,
+ secure: process.env.NODE_ENV === 'production',
+ sameSite: 'strict'
+ })
+ } catch (error) {
+ console.error('Failed to set profile cookie:', error)
+ }
return newProfile
})
}
}
packages/authentication/modules/profile/useCurrentProfile/index.ts
Outdated
Show resolved
Hide resolved
packages/authentication/modules/profile/useCurrentProfile/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 03b0268186978d73b50c0a34b38e22020fa69185 and d6ad58b4c0968b08d362a3397f8539c1c9501773.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/package.json(3 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/components/package.json
- packages/authentication/package.json
🔇 Additional comments (1)
packages/components/CHANGELOG.md (1)
8-9: LGTM: Dependency update is correctly documented
The dependency update to @baseapp-frontend/authentication@4.0.3 is properly documented and aligns with the changes.
There was a problem hiding this comment.
why do we need that?
There was a problem hiding this comment.
Without, it interpreted all the files in the __test__/__utils__ and __test__/__mocks__ (like profiles.ts) as test suites and errored that test suites need to include at least one test.
There was a problem hiding this comment.
I think we can move this folder inside TestComponentWithProviders, since it's only being imported there, right?
There was a problem hiding this comment.
I am wondering why we would do it on catch
There was a problem hiding this comment.
The reason I put this here, is that if there occurs an error in refreshAccessToken this function removes the access and refresh tokens, but does not fire a logout event. So I guess we should also remove the current profile, too. I would have liked to do that in the error handling of the refreshAccessToken function directly, but since the function is defined in the utils package, it should better not import from authentication. I thought that passing an onError callback was too compicated, so settled for this easier solution.
There was a problem hiding this comment.
I didn't get what this test does tbh :D
couldn't we do a useCurrentProfile test using the renderHook helper? We have a couple of examples on the auth package modules
packages/components/CHANGELOG.md
Outdated
There was a problem hiding this comment.
I think we need to update that
There was a problem hiding this comment.
What do you mean? Update the text? Or make this a minor/major version bump?
packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (2)
packages/authentication/modules/access/useLogin/index.ts (1)
Line range hint
134-134: Track remaining technical debt.There are multiple TODO comments about type refactoring that should be addressed:
- "TODO: refactor types" appears multiple times in the form submission handlers
Would you like me to help create GitHub issues to track these type refactoring tasks?
Also applies to: 142-142, 149-149
packages/graphql/package.json (1)
25-25: Consider using a version range for React peer dependencyUsing an exact version (
18.3.1) for the React peer dependency might be too restrictive. Consider using a version range (e.g.,^18.3.1or>=18.3.1) to allow for compatible React versions and avoid potential conflicts with other packages.- "react": "18.3.1" + "react": "^18.3.1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between d6ad58b4c0968b08d362a3397f8539c1c9501773 and e8c89f0a41130c0c74c3db004128a0b47250210b.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
packages/authentication/modules/access/useLogin/index.ts(4 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx(1 hunks)packages/authentication/package.json(2 hunks)packages/components/modules/comments/CommentCreate/types.ts(0 hunks)packages/components/modules/comments/CommentItem/CommentsReplies/types.ts(0 hunks)packages/components/modules/comments/CommentItem/types.ts(0 hunks)packages/components/modules/comments/Comments/types.ts(0 hunks)packages/components/modules/comments/CommentsList/types.ts(0 hunks)packages/components/package.json(2 hunks)packages/config/package.json(1 hunks)packages/design-system/package.json(1 hunks)packages/eslint-plugin/package.json(1 hunks)packages/graphql/package.json(1 hunks)packages/provider/package.json(1 hunks)packages/test/package.json(1 hunks)packages/utils/package.json(1 hunks)packages/wagtail/package.json(1 hunks)
💤 Files with no reviewable changes (5)
- packages/components/modules/comments/CommentItem/types.ts
- packages/components/modules/comments/CommentsList/types.ts
- packages/components/modules/comments/Comments/types.ts
- packages/components/modules/comments/CommentCreate/types.ts
- packages/components/modules/comments/CommentItem/CommentsReplies/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
packages/authentication/modules/access/useLogin/index.ts
[error] 37-37: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: Lint
packages/authentication/modules/access/useLogin/index.ts
[warning] 42-42:
Unexpected console statement
🔇 Additional comments (23)
packages/provider/package.json (2)
15-16: Dependencies and their type definitions are properly aligned
The package versions and their corresponding TypeScript type definitions are properly aligned:
- lodash@^4.17.21 with @types/lodash@^4.17.7
- @tanstack/react-query@^5.45.1 (types included)
- TypeScript@^5.4.5 is recent and stable
Also applies to: 25-27
15-16: Verify React compatibility with @tanstack/react-query
Let's verify that React 18.3.1 is compatible with @tanstack/react-query 5.45.1
Also applies to: 20-20
✅ Verification successful
React 18.3.1 is compatible with @tanstack/react-query 5.45.1
The peer dependency requirement for @tanstack/react-query 5.45.1 is ^18.0.0, which means it's compatible with React versions from 18.0.0 up to (but not including) 19.0.0. React 18.3.1 falls within this range. None of the recent issues in the TanStack Query repository indicate any compatibility problems with React 18.3.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check peer dependencies of @tanstack/react-query
# Get peer dependencies from npm registry
curl -s https://registry.npmjs.org/@tanstack/react-query/5.45.1 | jq -r '.peerDependencies | to_entries | .[] | select(.key == "react") | .value'
# Check for any reported issues
gh api graphql -f query='
{
search(query: "repo:TanStack/query react 18.3.1", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
state
}
}
}
}'
Length of output: 1011
packages/eslint-plugin/package.json (1)
18-26: LGTM! Development dependencies properly updated
The transition from catalog references to specific versions improves dependency management clarity. The chosen versions are stable and compatible.
packages/authentication/package.json (2)
16-21: LGTM! Dependencies align with state management migration
The addition of jotai and updates to related dependencies support the transition from Zustand to Jotai for state management.
16-46: Verify complete migration from Zustand to Jotai
Previous review comments indicate that Zustand dependencies and usage still exist in other packages. Consider documenting the migration strategy if this is intentional.
#!/bin/bash
# Description: Check for remaining Zustand dependencies and usage
# Look for Zustand in package.json files
fd "package.json" --exec rg -l "zustand" {}
# Look for Zustand imports in TypeScript files
rg -l "zustand" --type ts --type tsxpackages/wagtail/package.json (1)
22-83: LGTM! Development dependencies properly updated
The transition from catalog references to specific versions improves dependency management clarity. All chosen versions (except Next.js) are stable and compatible.
packages/test/package.json (3)
16-21: Verify React Query v5 compatibility
The upgrade to @tanstack/react-query v5.x introduces breaking changes that might affect tests. Ensure that all test utilities are updated to handle the new React Query API.
✅ Verification successful
React Query v5 usage is minimal and contained
The React Query usage in the test package is limited to setting up a test provider in ComponentWithProviders/index.tsx. The package only uses QueryClient for test setup purposes, and there are no direct query or mutation hooks being used. The v5 upgrade should have minimal impact on the testing utilities.
packages/test/providers/ComponentWithProviders/index.tsxonly creates a QueryClient instance for test isolation- No actual query or mutation hooks are used in the test package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for React Query usage in test files
rg -t ts -t tsx "useQuery|useMutation|queryClient" --glob "packages/test/**/*"
Length of output: 108
Script:
#!/bin/bash
# Search for React Query usage in test files with correct file extensions
rg "useQuery|useMutation|queryClient" --type-add 'tsx:*.{tsx,ts}' --type tsx "packages/test/"
# Also check for any test setup files that might configure React Query
rg "react-query|tanstack" --type-add 'tsx:*.{tsx,ts,js,jsx,json}' --type tsx "packages/test/"
Length of output: 738
Line range hint 1-58: Ensure test coverage for Jotai integration
Given that this PR transitions from Zustand to Jotai for state management, verify that the test utilities package includes proper support for testing Jotai atoms and hooks, particularly for the new profile management implementation.
29-42: Verify TypeScript 5.4.5 compatibility
The upgrade to TypeScript 5.4.5 is significant. Ensure that:
- All type definitions in the test utilities are compatible
- No new strict checks are breaking existing tests
✅ Verification successful
This will help us verify the TypeScript usage and potential compatibility concerns in the test utilities and configuration.
TypeScript 5.4.5 upgrade appears safe for test utilities
The test utilities use basic TypeScript features that are stable across versions:
- Simple type definitions using standard Response and RequestInit types
- Basic class implementation with standard type annotations
- No advanced type system features or strict checks that could break
The jest configuration explicitly disables TypeScript diagnostics and uses a separate tsconfig for tests, providing additional safety against version-specific issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for TypeScript-specific test utilities and potential type issues
rg -t ts -t tsx "type|interface|enum" --glob "packages/test/**/*"
# Look for custom type definitions that might need updates
fd -e d.ts -e tsx -e ts . packages/test
Length of output: 769
Script:
#!/bin/bash
# Check the content of type definitions and interfaces in test utilities
cat packages/test/utils/mockFetch/types.ts packages/test/utils/mockFetchError/types.ts
# Check for any TypeScript configuration files
fd tsconfig.json packages/test
# Look for potential TypeScript errors in mock implementations
cat packages/test/__mocks__/*.ts
# Check test setup files for TypeScript usage
cat packages/test/jest.config.ts
Length of output: 2942
packages/components/package.json (2)
4-4: LGTM: Version bump is appropriate
The version increment from 0.0.22 to 0.0.23 aligns with the significant changes in dependencies and state management approach.
24-41: Verify if zustand is still required
Since the PR objectives mention transitioning from Zustand to Jotai for state management, we should verify if zustand is still needed as a dependency.
✅ Verification successful
Let me check the actual usage of zustand in these files to determine if it's actively being used.
Zustand is actively used and required
The dependency is actively used in several critical features:
- Notification system (
useNotification) - Comment reply functionality (
useCommentReply,CommentReplyProvider) - Chat room management (
useChatRoom,ChatRoomProvider)
All these implementations use Zustand's core features (create, useStore) for state management. While there might be a transition to Jotai in progress, Zustand is still an essential dependency that cannot be removed at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining zustand usage in the codebase
echo "Searching for zustand imports..."
rg "from ['\"]zustand" -l
echo "Searching for zustand usage patterns..."
ast-grep --pattern 'import $_ from "zustand"'
Length of output: 574
Script:
#!/bin/bash
# Check the content of files using zustand
echo "Content of useNotification hook:"
cat packages/utils/hooks/useNotification/index.tsx
echo -e "\nContent of comment-related zustand usage:"
cat packages/components/modules/comments/context/useCommentReply/index.tsx
cat packages/components/modules/comments/context/CommentReplyProvider/index.tsx
echo -e "\nContent of chat-related zustand usage:"
cat packages/components/modules/messages/context/useChatRoom/index.tsx
cat packages/components/modules/messages/context/ChatRoomProvider/index.tsx
Length of output: 4535
packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx (1)
18-34: LGTM! Well-structured test implementation
The test case is well-implemented with:
- Proper async/await usage with waitFor()
- Event handling wrapped in act()
- Clear assertions for both UI state and side effects
- Good use of cleanup between tests
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
packages/utils/package.json (2)
16-28: Verify compatibility between core dependencies
Several observations about the core dependencies:
- The PR objective mentions transitioning from Zustand to Jotai, but
zustandis still listed as a dependency. This might indicate incomplete migration. - The Next.js version (
14.3.0-canary.24) is a canary release which may introduce instability.
Consider the following recommendations:
- If the Zustand to Jotai migration is complete, remove the
zustanddependency - Consider using a stable version of Next.js instead of a canary release for production
37-54: Verify testing framework compatibility
The testing dependencies have been significantly updated:
- Jest and related packages to ^29.7.0
- Testing Library React to ^16.0.0
- TypeScript to ^5.4.5
Ensure that:
- All test files are compatible with the new Testing Library version
- The TypeScript upgrade from catalog version to 5.4.5 doesn't introduce breaking changes
- Jest configuration is updated to support these new versions
✅ Verification successful
Let me analyze the test configuration and dependencies to verify compatibility. I'll check the Jest configurations and test files.
Let me check the actual test files to verify compatibility with the updated testing libraries.
Based on the analysis of the test files and dependencies, I can now provide a final response:
Dependencies update is compatible and safe to proceed
The codebase is well-prepared for the updated testing dependencies:
- Test files are using modern Testing Library patterns through
@baseapp-frontend/testpackage - Jest configuration is properly inherited from the base test package
- TypeScript types are compatible with the new versions
- No deprecated API usage found in the test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any type errors with the new TypeScript version
echo "Checking TypeScript files for potential compatibility issues..."
fd -e ts -e tsx --exec head -n 1
# Check Jest configuration
echo "Checking Jest configuration..."
fd "jest.config" -t f
Length of output: 33231
Script:
#!/bin/bash
# Check Jest configurations
echo "Checking Jest configurations..."
cat packages/utils/jest.config.ts
# Check test files for Testing Library usage
echo "Checking Testing Library usage..."
rg "@testing-library/react" -A 2
Length of output: 3818
Script:
#!/bin/bash
# Check test files for current Testing Library usage patterns
echo "Checking test files..."
rg -l "\.test\.(ts|tsx)$" | xargs head -n 20
# Check if there are any TypeScript errors in test files
echo "Checking TypeScript files in tests..."
fd "test.(ts|tsx)$" --exec head -n 20
Length of output: 22767
packages/authentication/modules/access/useLogin/index.ts (2)
8-8: LGTM! Import changes align with PR objectives.
The new imports support the transition to JWT-based profile handling and Jotai state management.
Also applies to: 25-25, 31-31
57-57: 🛠️ Refactor suggestion
Verify JWT decoding error handling.
While the implementation aligns with using JWT claims for the initial profile, there's no error handling for JWT decoding failures. This could lead to silent failures in production.
Let's verify the JWT structure and error handling:
Consider adding error handling:
- const user = decodeJWT<User>(response.access)
- if (user) {
+ try {
+ const user = decodeJWT<User>(response.access)
+ if (!user?.profile) {
+ throw new Error('Invalid JWT: missing profile data')
+ }
makeImagePathAbsolute(user.profile)
setCurrentProfile(user.profile)
- }
+ } catch (error) {
+ // Consider using error boundary or error logging service
+ throw new Error('Failed to process user profile from JWT')
+ }Also applies to: 68-72
packages/design-system/package.json (3)
21-26: Consider stability implications of @mui/lab alpha version
While the @mui packages are consistently on version 5.x.x, using an alpha version (@mui/lab@5.0.0-alpha.170) in production could introduce instability. Consider either:
- Using a stable version of @mui/lab
- Documenting the specific need for this alpha version
✅ Verification successful
@mui/lab dependency is justified and actively used
The alpha version of @mui/lab is being used for specific components that are actively implemented in the codebase:
- LoadingButton: Used in multiple components including ActionsOverlay, MarkAllAsReadButton, and ChatRoomListItem
- TimelineDot: Used in NotificationItem/CommentReply component
- Custom style overrides are also defined for LoadingButton
Since these components are actively used and the alpha version has been stable across the workspace (as seen in pnpm-lock.yaml), the dependency is justified. The version is consistently used across packages and appears to be working as expected in production.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any components are using @mui/lab
rg -l "@mui/lab" --type tsx
Length of output: 55
Script:
#!/bin/bash
# Check if any components are using @mui/lab with correct file extensions
rg -l "@mui/lab" --type ts --type jsx --type tsx --type js
Length of output: 86
Script:
#!/bin/bash
# Search for @mui/lab imports across all files
rg -l "@mui/lab"
Length of output: 536
Script:
#!/bin/bash
# Check what components from @mui/lab are actually being used
rg "@mui/lab" -B1 -A1
Length of output: 3665
39-40:
Verify React version compatibility
The update to React 18.3.1 is very recent. Please ensure all dependencies, especially the @mui packages and jotai, are compatible with this version.
✅ Verification successful
React 18.3.1 version appears to be consistent across the workspace
All packages in the workspace that specify React as a peer dependency are aligned to use version 18.3.1. The main dependencies MUI (v5.15.19) and Jotai (v2.9.3) are recent versions that typically maintain compatibility with the latest React releases. Since all packages are consistently using the same React version, there's no immediate compatibility concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check package.json files in the workspace for React version mismatches
fd package.json --type f --exec jq -r 'select(.peerDependencies != null) | select(.peerDependencies.react != null) | "\(.name): React \(.peerDependencies.react)"'
Length of output: 496
Script:
#!/bin/bash
# Check for MUI and jotai dependencies and their React peer dependency requirements
fd package.json --type f --exec jq -r '
select(.dependencies != null) |
select(.dependencies["@mui/material"] != null or .dependencies["jotai"] != null) |
"\(.name):\n MUI: \(.dependencies["@mui/material"] // "not found")\n Jotai: \(.dependencies["jotai"] // "not found")"
'
echo "=== MUI Peer Dependencies ==="
fd package.json --type f --exec jq -r '
select(.name | contains("@mui/material")) |
.peerDependencies | select(. != null) | to_entries[] |
"\(.key): \(.value)"
'
echo "=== Jotai Peer Dependencies ==="
fd package.json --type f --exec jq -r '
select(.name | contains("jotai")) |
.peerDependencies | select(. != null) | to_entries[] |
"\(.key): \(.value)"
'
Length of output: 1076
Script:
#!/bin/bash
# Let's check the actual installed versions in node_modules
echo "=== Checking @mui/material package.json ==="
curl -s https://registry.npmjs.org/@mui/material/5.15.19/package.json | jq '.peerDependencies'
echo "=== Checking jotai package.json ==="
curl -s https://registry.npmjs.org/jotai/2.9.3/package.json | jq '.peerDependencies'
Length of output: 367
49-57:
Verify breaking changes in major version updates
Several major version updates are included:
- Storybook v8
- TypeScript 5.4
Please ensure:
- All Storybook stories are compatible with v8
- No TypeScript breaking changes affect the codebase
Also applies to: 70-70
packages/graphql/package.json (2)
30-37: Verify @types package compatibility
Please ensure that the @types package versions align with their runtime counterparts:
@types/react^18.3.3 with React 18.3.1@types/react-relay^16.0.6 with react-relay ^16.2.0@types/relay-runtime^17.0.3 with relay-runtime ^16.2.0
Also, TypeScript 5.4.5 is a recent version. Let's verify compatibility across the project.
✅ Verification successful
@types packages and TypeScript versions are compatible
The verification shows that:
- React version (18.3.1) matches @types/react (^18.3.3)
- react-relay version (^16.2.0) matches @types/react-relay (^16.0.6)
- relay-runtime version (^16.2.0) is compatible with @types/relay-runtime (^17.0.3)
- TypeScript version (^5.4.5) is consistently used across all packages in the monorepo
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check TypeScript compatibility with @types packages
echo "Checking @types packages compatibility..."
npm view @types/react@18.3.3 dependencies
npm view @types/react-relay@16.0.6 dependencies
npm view @types/relay-runtime@17.0.3 dependencies
# Verify TypeScript version compatibility
echo "Checking TypeScript compatibility..."
tsc --version
Length of output: 536
Script:
#!/bin/bash
# Check package.json files for React and Relay versions
echo "Checking package dependencies..."
fd package.json --type f --exec grep -l "react\"\\|react-relay\\|relay-runtime" {} \; | xargs cat
# Check TypeScript versions across the project
echo "Checking TypeScript versions..."
fd package.json --type f --exec grep -l "typescript" {} \; | xargs cat
Length of output: 44681
15-21: Verify relay package version compatibility
The relay packages have version mismatches that could potentially cause issues:
react-relayandrelay-runtimeare at ^16.2.0relay-test-utilsis at ^17.0.0
Also, the addition of js-cookie aligns with the PR objective of implementing cookie-based profile storage for SSR.
Let's verify the compatibility and security of these versions:
✅ Verification successful
Version mismatch is not a concern - relay-test-utils is only used in tests
Based on the analysis of the codebase:
relay-test-utilsis only imported in test-related files and test utilities:packages/graphql/providers/RelayTestProvider/packages/graphql/utils/createTestEnvironment/packages/components/__test_utils__/- Test files with
__tests__directories
react-relayandrelay-runtimeat version 16.2.0 are used consistently throughout the application code- The version difference in
relay-test-utils(17.0.0) doesn't impact production code as it's strictly a testing dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for known vulnerabilities and latest versions
echo "Checking npm registry for latest versions and compatibility..."
npm view react-relay@^16.2.0 version peerDependencies
npm view relay-runtime@^16.2.0 version
npm view relay-test-utils@^17.0.0 version
# Check for any security advisories
echo "Checking for security advisories..."
npm audit graphql@16.8.1 graphql-ws@5.16.0 isomorphic-ws@5.0.0 js-cookie@3.0.5 react-relay@16.2.0 relay-runtime@16.2.0 relay-test-utils@17.0.0
Length of output: 943
Script:
#!/bin/bash
# Let's check if there are any direct dependencies on relay-test-utils in the codebase
rg -l "relay-test-utils" --type js --type ts --type jsx --type tsx
# Check if relay-test-utils is only used in test files
fd -e test.js -e test.ts -e test.jsx -e test.tsx -x rg -l "relay-test-utils" {}
# Check package.json files for any other relay dependencies
fd package.json -x rg -l "relay" {}
# Let's also check the actual imports of relay packages
rg "from ['\"]relay|from ['\"]react-relay" -A 1
Length of output: 30822
packages/config/package.json (2)
13-28: Consider using caret ranges consistently
While most dependencies use caret ranges (^), ensure this is intentional for maintenance and updates. This allows for automatic patch and minor version updates while preventing major version updates that might introduce breaking changes.
13-28: Verify security advisories for all dependencies
Let's check for any known security vulnerabilities in the specified versions.
✅ Verification successful
All dependencies are secure with current versions
The security scan shows that all dependencies are using secure versions. The only vulnerability found was for ESLint versions < 4.18.2 (RegExp DoS), but the project uses ESLint ^8.42.0 which is well above the patched version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories for all dependencies
# Check for security advisories using GitHub API
for pkg in "emotion__eslint-plugin" "next__eslint-plugin-next" "trivago__prettier-plugin-sort-imports" "typescript-eslint__eslint-plugin" "typescript-eslint__parser" "eslint" "eslint-config-airbnb" "eslint-config-airbnb-typescript" "eslint-config-next" "eslint-config-prettier" "eslint-plugin-import" "eslint-plugin-jsx-a11y" "eslint-plugin-react" "eslint-plugin-react-hooks" "prettier" "prettier-plugin-tailwindcss"; do
echo "Checking $pkg..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "'$pkg'") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
done
Length of output: 14350
packages/components/package.json
Outdated
There was a problem hiding this comment.
Consider using a stable React version
The peer dependencies specify React 18.3.1, which is a canary release. Consider using a stable version (e.g., 18.2.0) to avoid potential compatibility issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/InitialProfileProvider/index.tsx (2)
1-6: Consider using absolute imports for better maintainability.The deep relative imports (../../../../../../) can be hard to maintain and are prone to breaking when files are moved. Consider configuring and using absolute imports instead.
-import { MinimalProfile } from '../../../../../../types/profile' -import { profileAtom } from '../../../index' +import { MinimalProfile } from '@authentication/types/profile' +import { profileAtom } from '@authentication/modules/profile/useCurrentProfile'
12-18: Fix typo in the usage example comment.There's a grammatical error in the comment.
-// // You're component goes here, it is passed the initialProfile +// // Your component goes here, it is passed the initialProfile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between e8c89f0a41130c0c74c3db004128a0b47250210b and 07b502726478cd214b320f8bbeacdd6ca76e836f.
📒 Files selected for processing (9)
packages/authentication/modules/profile/index.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/InitialProfileProvider/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx(1 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx(3 hunks)packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
- packages/components/modules/messages/MessagesList/index.tsx
- packages/authentication/modules/profile/index.ts
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx
- packages/authentication/modules/profile/useCurrentProfile/tests/useCurrentProfile.test.tsx
🔇 Additional comments (2)
packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/InitialProfileProvider/index.tsx (2)
8-10: LGTM! Clear and well-typed interface.
The type definition is explicit about nullability and follows TypeScript best practices.
19-25: Verify behavior with changing initialProfile prop.
The implementation looks clean and follows best practices. However, we should verify how this behaves if initialProfile changes after initial hydration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/authentication/modules/access/useLogin/index.ts (1)
Technical debt items need attention across the codebase
The
@ts-ignore BA-1206is part of a larger pattern of type-related technical debt in the codebase. Found multiple instances that need attention:
- Several
@ts-ignorecomments across multiple packages- Multiple TODO comments specifically about type improvements:
- Form handling type improvements in authentication modules
- Generic type fixes in
useJWTUser- Import-related type issues in design system components
- DeepPartial type error pending v8 fix in useSignUp
🔗 Analysis chain
Line range hint
121-121: Track and address technical debt itemsThere are several TODO comments about type refactoring and a @ts-ignore that reference BA-1206. These should be tracked and addressed to maintain code quality.
Let's check if there's an existing issue for BA-1206:
Would you like me to create a new issue to track the type refactoring tasks if one doesn't exist?
Also applies to: 134-134
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if BA-1206 issue exists and its status gh issue list --search "BA-1206 in:title,body"Length of output: 48
Script:
#!/bin/bash # Let's check for other instances of BA-1206 in the codebase rg "BA-1206" -A 2 -B 2 # Also check for similar @ts-ignore patterns rg "@ts-ignore" -A 2 -B 2 # Look for TODO comments about type refactoring rg -i "TODO.*typ(e|ing)" -A 2 -B 2Length of output: 14202
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 07b502726478cd214b320f8bbeacdd6ca76e836f and f3396f22e46636583dc604bdd19bde88d2c1e94d.
📒 Files selected for processing (1)
packages/authentication/modules/access/useLogin/index.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/authentication/modules/access/useLogin/index.ts
[error] 37-37: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
packages/authentication/modules/access/useLogin/index.ts (1)
35-43:
Function needs improvements for safety and immutability
The current implementation has several issues:
- Mutates the input parameter directly
- Missing explicit return type
- Lacks proper error handling for environment variables
Consider this safer implementation:
-const makeImagePathAbsolute = (profileArg: MinimalProfile | null) => {
- const profile = profileArg
- if (profile && profile.image) {
- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
- if (baseUrl) {
- profile.image = baseUrl + profile.image
- }
- }
-}
+const makeImagePathAbsolute = (profile: MinimalProfile | null): MinimalProfile | null => {
+ if (!profile?.image || !process.env.NEXT_PUBLIC_API_BASE_URL) {
+ return profile
+ }
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL.replace('/v1', '')
+ return {
+ ...profile,
+ image: baseUrl + profile.image
+ }
+}🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
# Conflicts: # packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx # packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts # packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/CurrentProfileProvider.test.tsx # packages/components/modules/profiles/context/CurrentProfileProvider/index.tsx # packages/components/modules/profiles/context/CurrentProfileProvider/types.ts # packages/components/modules/profiles/index.ts # pnpm-lock.yaml
# Conflicts: # packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
This reverts commit 1071e6e2098c27d40eb28bce840c285805db1b80.
anicioalexandre
force-pushed
the
BA-1809-simplify-currentprofileprovider-squash
branch
from
f3396f2 to
ea106e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
packages/test/jest.config.ts (1)
14-14: LGTM! Configuration updates enhance test coverageThe changes improve the test setup by:
- Including JavaScript files in the transform pattern
- Adding WebSocket mocking support
- Specifying precise test file matching
- Handling mock files appropriately
Consider adding comments to document the purpose of each configuration section for better maintainability.
Also applies to: 16-20, 30-30, 34-34
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between f3396f22e46636583dc604bdd19bde88d2c1e94d and ea106e03d4b34d3550427854de406ec0aec41979.
⛔ Files ignored due to path filters (4)
packages/components/__generated__/ProfileItemInlineFragment.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/ProfilesListQuery.graphql.tsis excluded by!**/__generated__/**packages/components/__generated__/UserProfileQuery.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (61)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/index.ts(1 hunks)packages/authentication/modules/access/useLogin/index.ts(4 hunks)packages/authentication/modules/profile/index.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__mock__/profiles.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/InitialProfileProvider/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/constants.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/index.ts(1 hunks)packages/authentication/modules/user/useUpdateUser/index.ts(3 hunks)packages/authentication/package.json(3 hunks)packages/authentication/types/profile.ts(1 hunks)packages/authentication/types/user.ts(1 hunks)packages/components/.storybook/decorators/withProviders.tsx(2 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/__tests__/dummy.test.ts(1 hunks)packages/components/jest.config.ts(1 hunks)packages/components/modules/comments/CommentCreate/index.tsx(3 hunks)packages/components/modules/comments/CommentCreate/types.ts(0 hunks)packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/CommentsReplies/types.ts(0 hunks)packages/components/modules/comments/CommentItem/index.tsx(0 hunks)packages/components/modules/comments/CommentItem/types.ts(0 hunks)packages/components/modules/comments/Comments/index.tsx(1 hunks)packages/components/modules/comments/Comments/types.ts(0 hunks)packages/components/modules/comments/CommentsList/index.tsx(0 hunks)packages/components/modules/comments/CommentsList/types.ts(0 hunks)packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx(2 hunks)packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx(1 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/index.tsx(2 hunks)packages/components/modules/messages/SendMessage/index.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx(2 hunks)packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx(2 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__storybook__/stories.tsx(0 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx(7 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__mocks__/profiles.ts(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(3 hunks)packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx(3 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts(1 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/CurrentProfileProvider.test.tsx(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__mock__/profiles.ts(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__mock__/user.ts(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__utils__/TestComponent/index.tsx(0 hunks)packages/components/modules/profiles/context/CurrentProfileProvider/__tests__/__utils__/TestComponentWithProvider/index.tsx(0 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/graphql/queries/ProfileItemInline.ts(0 hunks)packages/components/modules/profiles/graphql/queries/ProfilesList.ts(1 hunks)packages/components/modules/profiles/graphql/queries/UserProfile.ts(1 hunks)packages/components/modules/profiles/index.ts(0 hunks)packages/components/package.json(2 hunks)packages/test/CHANGELOG.md(1 hunks)packages/test/jest.config.ts(2 hunks)packages/test/package.json(1 hunks)
💤 Files with no reviewable changes (20)
- packages/components/modules/comments/CommentItem/CommentsReplies/types.ts
- packages/components/modules/comments/CommentCreate/types.ts
- packages/components/modules/comments/CommentItem/types.ts
- packages/components/modules/comments/CommentsList/types.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/utils/TestComponent/index.tsx
- packages/components/modules/comments/CommentItem/index.tsx
- packages/components/modules/comments/Comments/types.ts
- packages/components/modules/profiles/index.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/utils/TestComponentWithProvider/index.tsx
- packages/components/modules/comments/CommentItem/CommentsReplies/index.tsx
- packages/components/modules/comments/CommentsList/index.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/constants.ts
- packages/components/modules/profiles/context/useCurrentProfile/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/storybook/stories.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/types.ts
- packages/components/modules/profiles/graphql/queries/ProfileItemInline.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/mock/user.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/mock/profiles.ts
- packages/components/modules/profiles/context/CurrentProfileProvider/index.tsx
- packages/components/modules/profiles/context/CurrentProfileProvider/tests/CurrentProfileProvider.test.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/components/jest.config.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- packages/authentication/index.ts
- packages/authentication/modules/profile/useCurrentProfile/constants.ts
- packages/components/modules/messages/MessagesList/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/tests/utils/withProviders/index.tsx
- packages/components/tests/dummy.test.ts
- packages/components/CHANGELOG.md
- packages/authentication/types/profile.ts
- packages/components/modules/profiles/graphql/queries/ProfilesList.ts
- packages/components/modules/messages/ChatRoomsList/ChatRoomItem/index.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/MessageItem/index.tsx
- packages/components/modules/messages/graphql/subscriptions/useMessageCountUpdateSubscription.tsx
- packages/components/modules/profiles/graphql/queries/UserProfile.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/mock/profiles.ts
- packages/components/modules/messages/SendMessage/index.tsx
- packages/components/package.json
- packages/authentication/modules/profile/index.ts
- packages/components/.storybook/decorators/withProviders.tsx
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/index.tsx
- packages/test/package.json
- packages/components/modules/messages/graphql/subscriptions/useRoomListSubscription.tsx
- packages/components/modules/profiles/ProfilePopover/CurrentProfile/index.tsx
- packages/authentication/package.json
- packages/components/modules/comments/Comments/index.tsx
- packages/components/modules/messages/CreateChatRoomList/ChatRoomListItem/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/tests/AccountPopover.cy.tsx
- packages/components/modules/comments/CommentCreate/index.tsx
- packages/components/modules/messages/MessagesList/MessagesGroup/index.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/index.tsx
- packages/authentication/modules/user/useUpdateUser/index.ts
- packages/authentication/types/user.ts
- packages/components/modules/profiles/ProfilePopover/ProfilesList/ProfileMenuItem/types.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/useCurrentProfile.test.tsx
- packages/components/modules/navigations/Header/AccountMenu/AccountPopover/tests/mocks/profiles.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/utils/InitialProfileProvider/index.tsx
🧰 Additional context used
🪛 GitHub Check: Lint
packages/authentication/modules/profile/useCurrentProfile/index.ts
[warning] 57-57:
React Hook useCallback has a missing dependency: 'setCurrentProfile'. Either include it or remove the dependency array
[warning] 64-64:
React Hook useEffect has a missing dependency: 'removeCurrentProfile'. Either include it or remove the dependency array
🔇 Additional comments (12)
packages/authentication/modules/access/useLogin/index.ts (3)
8-8: LGTM: New imports align with PR objectives
The added imports support the transition to JWT-based profile handling and new profile management approach.
Also applies to: 25-25, 31-31
45-45: LGTM: Profile management hook properly initialized
The useCurrentProfile hook integration aligns with the PR's objective to simplify profile management.
56-65:
Enhance error handling and type safety in JWT processing
The JWT decoding and profile setup needs better error handling and type safety.
Apply this improved implementation:
- const user = decodeJWT<User>(response.access)
- if (user) {
- // TODO: handle the absolute image path on the backend
- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
- const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
- setCurrentProfile({
- ...user.profile,
- image: absoluteImagePath,
- })
- }
+ try {
+ const user = decodeJWT<User>(response.access)
+ if (!user?.profile) {
+ throw new Error('Invalid user profile in JWT')
+ }
+
+ if (!process.env.NEXT_PUBLIC_API_BASE_URL) {
+ throw new Error('API base URL not configured')
+ }
+
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL.replace('/v1', '')
+ const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
+
+ setCurrentProfile({
+ ...user.profile,
+ image: absoluteImagePath,
+ })
+ } catch (error) {
+ console.error('Failed to process user profile:', error)
+ throw error
+ }packages/authentication/modules/profile/useCurrentProfile/index.ts (3)
19-24: Add type validation for profile data from cookies
Cookie data should be validated before use as it comes from an external source.
37-49: Add error handling for cookie operations
The setCurrentProfile function should handle potential cookie operation failures.
35-35:
Fix SSR detection logic
The current SSR detection has a bug that will always evaluate to false.
- const isSSR = typeof window === typeof undefined
+ const isSSR = typeof window === 'undefined'Likely invalid or redundant comment.
packages/authentication/CHANGELOG.md (2)
7-7: Add missing changelog entries for key changes
The changelog should document:
- Transition from Zustand to Jotai
- New behavior of deriving initial profile from JWT claims
- SSR improvements with cookie-based storage
3-7:
Consider bumping to version 5.0.0 and improving documentation
The removal of CurrentProfileProvider and introduction of Jotai represents a breaking change that warrants:
- A major version bump (5.0.0)
- Documentation under "Breaking Changes" section
- Migration guide with examples
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (3)
5-5: LGTM! Import changes align with architectural improvements.
The transition to absolute imports from @baseapp-frontend/authentication and the updated fragment type improve modularity and maintainability.
Also applies to: 12-12
23-23: LGTM! Hook usage simplified.
The simplified destructuring of useCurrentProfile improves code readability and aligns with the transition to Jotai for state management.
31-36:
Add error handling for environment variable.
The code assumes NEXT_PUBLIC_API_BASE_URL is always available. Consider adding error handling or a default value.
Apply this diff to improve error handling:
- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL
+ if (!baseUrl) {
+ console.error('NEXT_PUBLIC_API_BASE_URL is not defined')
+ return
+ }
+ const apiBaseUrl = baseUrl.replace('/v1', '')
- const absoluteImagePath = profile.image ? `${baseUrl}${profile.image?.url}` : null
+ const absoluteImagePath = profile.image ? `${apiBaseUrl}${profile.image?.url}` : nullLikely invalid or redundant comment.
packages/test/CHANGELOG.md (1)
3-4:
Consider a minor/major version bump instead of patch
The changes in this PR represent significant architectural modifications (Zustand to Jotai, profile management changes). According to semantic versioning:
- PATCH versions (2.0.x) should only contain backwards compatible bug fixes
- MINOR versions (2.x.0) should contain backwards compatible new features
- MAJOR versions (x.0.0) should contain breaking changes
Given the scope of changes, this should likely be version 2.1.0 (minor) or 3.0.0 (major) if there are breaking changes in the testing utilities.
Let's check for potential breaking changes:
There was a problem hiding this comment.
Validate image path construction
The current image path construction could be vulnerable to path traversal if the image path from the profile contains ../ or similar sequences.
Add path validation:
- const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
+ const absoluteImagePath = user.profile.image
+ ? `${baseUrl}${user.profile.image.replace(/\.{2,}/g, '')}`
+ : 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.
| const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '') | |
| const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null | |
| const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '') | |
| const absoluteImagePath = user.profile.image | |
| ? `${baseUrl}${user.profile.image.replace(/\.{2,}/g, '')}` | |
| : null |
There was a problem hiding this comment.
Add missing dependencies to React hooks
The useCallback and useEffect hooks are missing dependencies.
- const removeCurrentProfile = useCallback(() => setCurrentProfile(null), [])
+ const removeCurrentProfile = useCallback(() => setCurrentProfile(null), [setCurrentProfile])
useEffect(() => {
eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile)
return () => {
eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile)
}
- }, [])
+ }, [removeCurrentProfile])Also applies to: 64-64
🧰 Tools
🪛 GitHub Check: Lint
[warning] 57-57:
React Hook useCallback has a missing dependency: 'setCurrentProfile'. Either include it or remove the dependency array
There was a problem hiding this comment.
💡 Codebase verification
Move image URL construction to backend - Multiple instances found
The manual construction of absolute image paths is indeed a systemic issue, found in multiple locations:
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsxpackages/authentication/modules/access/useLogin/index.ts
Both files:
- Contain identical TODO comments about handling image paths on the backend
- Manually construct image URLs using environment variables
- Use the same fragile approach of manipulating the API base URL
This confirms that the image URL construction needs to be moved to the backend as a unified solution.
🔗 Analysis chain
Move image URL construction to backend.
The manual construction of absolute image paths in the frontend is fragile and could break if the backend URL structure changes. This should be handled by the backend as indicated by the TODO comment.
Let's verify if this is being handled consistently across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar image URL constructions that might need to be updated
rg -l "NEXT_PUBLIC_API_BASE_URL.*image.*url" --type typescript
Length of output: 99
Script:
#!/bin/bash
# Search for similar image URL constructions using ripgrep without file type restriction
rg -l "NEXT_PUBLIC_API_BASE_URL.*image.*url"
# Also search for any image URL construction patterns
rg -l "image.*url.*NEXT_PUBLIC"
Length of output: 80
Script:
#!/bin/bash
# Search for TODO comments related to image paths
rg "TODO.*image.*path"
Length of output: 278
Script:
#!/bin/bash
# Search for any environment variable usage with image URLs
rg "process\.env.*image.*url" -A 2
Length of output: 36
Script:
#!/bin/bash
# Let's check the implementation in the login module as well
cat packages/authentication/modules/access/useLogin/index.ts
Length of output: 4720
Script:
#!/bin/bash
# Search for any URL construction patterns with baseUrl
rg "baseUrl.*image" -A 2
Length of output: 686
packages/test/CHANGELOG.md
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changelog description with more details
The current description "Add additional configuration to jest.config file" is too vague. Given that this PR involves significant architectural changes (Zustand to Jotai transition), please provide more specific details about:
- What configurations were added
- Why they were needed
- How they support testing the new architecture
Example of a more detailed description:
-Add additional configuration to `jest.config` file.
+Add Jest configurations to support Jotai state management testing:
+- Configure custom test environment for Jotai
+- Add transformers for cookie-based profile management
+- Update mock implementations for SSR testing📝 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.
| - Add additional configuration to `jest.config` file. | |
| Add Jest configurations to support Jotai state management testing: | |
| - Configure custom test environment for Jotai | |
| - Add transformers for cookie-based profile management | |
| - Update mock implementations for SSR testing |
anicioalexandre
force-pushed
the
BA-1809-simplify-currentprofileprovider-squash
branch
from
ea106e0 to
a1e147e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx (2)
8-12: Consider adding mocks for JWT and SSR-related functionalityGiven that the PR objectives mention using JWT claims for initial profile and SSR support, consider adding mocks for:
- JWT token parsing/validation
- SSR-specific utilities or flags
jest.mock('@baseapp-frontend/utils', () => ({ ...jest.requireActual('@baseapp-frontend/utils'), removeCookie: jest.fn(), setCookie: jest.fn(), + parseJwt: jest.fn(), + isSSR: jest.fn(() => false), }))
15-17: Add beforeEach setup for consistent initial stateConsider adding a beforeEach block to reset the initial state, especially important when testing cookie-based initialization:
+ beforeEach(() => { + // Reset initial state + jest.resetModules() + // Clear any stored cookies + removeCookie(CURRENT_PROFILE_KEY) + }) afterEach(() => { jest.clearAllMocks() })packages/components/modules/profiles/Members/index.tsx (1)
Line range hint
24-27: Consider optimizing the query execution for undefined profile.Currently, the query executes with an empty string when
currentProfileis undefined. Consider preventing the query execution until a valid profile is available.- const data = useLazyLoadQuery<IUserMembersListPaginationQuery>(UserMembersListPaginationQuery, { - profileId: currentProfile?.id || '', - count: NUMBER_OF_MEMBERS_ON_FIRST_LOAD, - orderBy: 'status', - }) + if (!currentProfile?.id) return null; + + const data = useLazyLoadQuery<IUserMembersListPaginationQuery>(UserMembersListPaginationQuery, { + profileId: currentProfile.id, + count: NUMBER_OF_MEMBERS_ON_FIRST_LOAD, + orderBy: 'status', + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between ea106e03d4b34d3550427854de406ec0aec41979 and a1e147ea27fbf8a5f1216833e81e916614ffaa92.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/authentication/modules/access/useLogin/index.ts(4 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx(1 hunks)packages/authentication/package.json(3 hunks)packages/components/jest.config.ts(1 hunks)packages/components/modules/profiles/Members/index.tsx(2 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/package.json(2 hunks)packages/test/CHANGELOG.md(1 hunks)packages/test/jest.config.ts(2 hunks)packages/test/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/package.json
- packages/test/package.json
- packages/authentication/package.json
- packages/test/CHANGELOG.md
- packages/test/jest.config.ts
🔇 Additional comments (11)
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (4)
5-5: LGTM! Import changes align with the refactoring objectives.
The transition to absolute imports and named fragments improves maintainability and follows better practices.
Also applies to: 12-12
23-23: LGTM! Hook usage simplified.
The simplified destructuring of useCurrentProfile aligns with the transition to Jotai and improves code readability.
27-30: Move image URL construction to backend.
The manual construction of absolute image paths in the frontend is fragile and could break if the backend URL structure changes. This should be handled by the backend as indicated by the TODO comment.
31-36: LGTM! Profile update structure follows the new MinimalProfile type.
The setCurrentProfile implementation correctly uses the simplified profile structure with essential fields (id, name, image, urlPath).
packages/components/modules/profiles/Members/index.tsx (2)
3-3: LGTM! Import change aligns with centralized profile management.
The import change correctly utilizes the centralized authentication package, which is consistent with the architectural changes being made.
22-22: Verify SSR behavior with the new profile management.
The hook usage has been simplified, which is good. However, since this component is using Suspense and the PR mentions cookie-based profiles for SSR, we should verify that the profile is properly hydrated during server-side rendering.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the profile management implementation:
The profile management is properly handled for SSR scenarios
The useCurrentProfile hook implementation shows that:
- It uses Jotai for global state management combined with cookie-based persistence
- It has explicit SSR handling with a
noSSRoption that defaults totrue - The Members component uses the default behavior which is safe for SSR as the profile is hydrated from cookies
The PR's changes to simplify the hook usage are consistent with the documented behavior in the authentication package's changelog, which explicitly mentions the cookie-based profile tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the component or its parents have any getServerSideProps or similar SSR setup
rg -A 5 "getServerSideProps|getInitialProps" $(fd -e tsx -e ts Members)
# Check if there are any SSR-specific conditions around useCurrentProfile
ast-grep --pattern 'if (typeof window === "undefined") { $$$ }'
Length of output: 438
Script:
#!/bin/bash
# Let's check the implementation of useCurrentProfile hook
rg -l "useCurrentProfile" --type ts --type tsx
# Check for any SSR-specific code in the profile context or hooks
rg -A 5 "useCurrentProfile|CurrentProfileProvider" --type ts --type tsx
# Look for any Suspense boundaries around the Members component
ast-grep --pattern 'Suspense $$$>
$$$
<Members $$$/>
$$$
</Suspense>'
# Check if there are any hydration-specific code patterns
rg -A 5 "useEffect.*profile|hydrat" --type ts --type tsx
Length of output: 341
Script:
#!/bin/bash
# Let's try without explicit type flags since they're not recognized
# Check the implementation of useCurrentProfile hook
rg -l "useCurrentProfile"
# Check for any SSR-specific code in the profile context or hooks
rg -A 5 "useCurrentProfile|CurrentProfileProvider"
# Check for any hydration-specific code patterns
rg -A 5 "useEffect.*profile|hydrat"
# Look for any cookie-related profile handling
rg -A 5 "cookie.*profile|profile.*cookie"
Length of output: 30548
packages/authentication/modules/access/useLogin/index.ts (5)
8-8: LGTM: New imports align with architecture changes
The added imports support the transition to JWT-based profile initialization and new profile state management.
Also applies to: 25-25, 31-31
45-45: LGTM: Profile state management initialization
The destructured setCurrentProfile from useCurrentProfile aligns with the transition to Jotai for state management.
59-60:
Improve environment variable and path handling
The current implementation has several security concerns:
- Missing environment variable validation
- Potential path traversal vulnerability in image path construction
Apply this diff to improve security:
- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
- const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL
+ if (!baseUrl) {
+ throw new Error('API base URL is not configured')
+ }
+ const sanitizedBaseUrl = baseUrl.replace('/v1', '')
+ const absoluteImagePath = user.profile.image
+ ? `${sanitizedBaseUrl}${user.profile.image.replace(/\.{2,}/g, '')}`
+ : nullLikely invalid or redundant comment.
56-65:
Enhance JWT decoding error handling
The current implementation lacks proper error handling for JWT decoding and profile extraction.
Apply this diff to improve error handling:
- const user = decodeJWT<User>(response.access)
- if (user) {
- // TODO: handle the absolute image path on the backend
- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
- const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
- setCurrentProfile({
- ...user.profile,
- image: absoluteImagePath,
- })
- }
+ try {
+ const user = decodeJWT<User>(response.access)
+ if (!user?.profile) {
+ throw new Error('Invalid user profile in JWT')
+ }
+
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL
+ if (!baseUrl) {
+ throw new Error('API base URL is not configured')
+ }
+
+ const sanitizedBaseUrl = baseUrl.replace('/v1', '')
+ const absoluteImagePath = user.profile.image
+ ? `${sanitizedBaseUrl}${user.profile.image.replace(/\.{2,}/g, '')}`
+ : null
+
+ setCurrentProfile({
+ ...user.profile,
+ image: absoluteImagePath,
+ })
+ } catch (error) {
+ console.error('Failed to process user profile:', error)
+ throw error
+ }Likely invalid or redundant comment.
58-58: Address TODO comment regarding image path handling
The TODO comment indicates that image path handling should be moved to the backend. This is a security best practice that needs to be addressed.
Consider creating a dedicated API endpoint for serving profile images that handles:
- Path sanitization
- Access control
- CDN integration
- Image optimization
packages/components/jest.config.ts
Outdated
There was a problem hiding this comment.
💡 Codebase verification
Revert needed: Jest config simplification removes critical test configurations
The simplified Jest configuration removes several important customizations that were present in the previous version:
- Custom test matching patterns for spec/test files
- Mock setup file for GraphQL WebSocket
- Specific Babel transform configuration
- Custom module path ignore patterns for mocks
These configurations appear to be package-specific and necessary for the components package. The base configuration file (@baseapp-frontend/test/jest.config.ts) is also not found in the expected location, which raises concerns about the validity of this change.
packages/components/jest.config.ts: Revert the simplification and maintain the package-specific configurations- Ensure the GraphQL WebSocket mock setup at
jest/__mocks__/graphqlWsMock.tsis properly configured - Verify the Babel transform settings for JS/TS files are correctly maintained
🔗 Analysis chain
LGTM! Verify test coverage remains intact.
The simplification of the Jest configuration by directly requiring the base config is a good practice that reduces duplication and centralizes test settings. This change aligns well with the DRY principle.
Let's verify that all tests still run correctly and no package-specific configurations were accidentally removed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage and configurations
# Check for any package-specific test setup files that might need migration
fd -e ts -e js "setup" packages/components/
# Look for test-specific patterns that might need special handling
rg -g "*.{test,spec}.{ts,tsx}" "testMatch|setupFilesAfterEnv|transform" packages/components/
Length of output: 138
Script:
#!/bin/bash
# Let's try a different approach to verify the test setup:
# 1. Check the content of the previous jest config to understand what we might have lost
git show HEAD^:packages/components/jest.config.ts || true
# 2. Look for any test-related files and patterns
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . packages/components/
# 3. Check the base configuration we're now importing
cat packages/baseapp-frontend/test/jest.config.ts || true
Length of output: 971
There was a problem hiding this comment.
Add test coverage for critical scenarios
The current test suite is missing coverage for several key scenarios introduced in this PR:
- Initial profile loading from JWT claims:
it('should initialize profile from JWT claims', () => {
const mockClaims = { profileId: 'jwt-profile-id' }
parseJwt.mockReturnValue(mockClaims)
const { result } = renderHook(() => useCurrentProfile())
expect(result.current.currentProfile).toEqual(expect.objectContaining({ id: 'jwt-profile-id' }))
})- SSR behavior with cookie:
it('should handle SSR profile initialization', () => {
isSSR.mockReturnValue(true)
const profile = mockUserProfileFactory('ssr-profile')
// Mock cookie retrieval for SSR
const { result } = renderHook(() => useCurrentProfile({ noSSR: false }))
expect(result.current.currentProfile).toEqual(profile)
})- Profile updates with active check:
it('should only update if profile is active', () => {
const profile = mockUserProfileFactory('active-profile')
const { result } = renderHook(() => useCurrentProfile())
act(() => result.current.updateProfileIfActive(profile))
expect(result.current.currentProfile).toEqual(profile)
})There was a problem hiding this comment.
Fix incorrect profile in cookie assertion
There's a bug in the test case where the wrong profile is being used in the cookie assertion after setting profile2:
act(() => result.current.setCurrentProfile(profile2))
expect(result.current.currentProfile!.id).toEqual('profile-id-2')
- expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true })
+ expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile2, { stringfyValue: 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.
| it('changes current profile state and sets cookie', () => { | |
| const profile1 = mockUserProfileFactory('profile-id-1') | |
| const profile2 = mockUserProfileFactory('profile-id-2') | |
| const { result } = renderHook(() => useCurrentProfile()) | |
| act(() => result.current.setCurrentProfile(profile1)) | |
| expect(result.current.currentProfile!.id).toEqual('profile-id-1') | |
| expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true }) | |
| act(() => result.current.setCurrentProfile(profile2)) | |
| expect(result.current.currentProfile!.id).toEqual('profile-id-2') | |
| expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true }) | |
| }) | |
| it('changes current profile state and sets cookie', () => { | |
| const profile1 = mockUserProfileFactory('profile-id-1') | |
| const profile2 = mockUserProfileFactory('profile-id-2') | |
| const { result } = renderHook(() => useCurrentProfile()) | |
| act(() => result.current.setCurrentProfile(profile1)) | |
| expect(result.current.currentProfile!.id).toEqual('profile-id-1') | |
| expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile1, { stringfyValue: true }) | |
| act(() => result.current.setCurrentProfile(profile2)) | |
| expect(result.current.currentProfile!.id).toEqual('profile-id-2') | |
| expect(setCookie).toHaveBeenCalledWith(CURRENT_PROFILE_KEY, profile2, { stringfyValue: true }) | |
| }) |
anicioalexandre
force-pushed
the
BA-1809-simplify-currentprofileprovider-squash
branch
from
a1e147e to
25fd287
Compare
anicioalexandre
force-pushed
the
BA-1809-simplify-currentprofileprovider-squash
branch
from
25fd287 to
e69248c
Compare
|
anicioalexandre
deleted the
BA-1809-simplify-currentprofileprovider-squash
branch
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/authentication/modules/access/useLogin/index.ts (1)
61-64: Consider adding type safety for profile state updates.The profile state update could benefit from stronger type checking.
+ type ProfileUpdate = Parameters<typeof setCurrentProfile>[0] setCurrentProfile({ ...user.profile, image: absoluteImagePath, - }) + } satisfies ProfileUpdate)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 25fd287502adccd2ef1b710c363ab492dd8ddd66 and e69248c.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
packages/authentication/modules/access/useLogin/index.ts(4 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/useCurrentProfile.test.tsx(1 hunks)packages/authentication/package.json(3 hunks)packages/components/.storybook/__mocks__/nextFontMock.ts(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/__mocks__/nextFontMock.ts(1 hunks)packages/components/cypress/__mocks__/nextFontMock.ts(1 hunks)packages/components/jest.config.ts(0 hunks)packages/components/modules/profiles/Members/index.tsx(2 hunks)packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx(2 hunks)packages/components/package.json(2 hunks)packages/design-system/.storybook/__mocks__/nextFontMock.ts(1 hunks)packages/test/CHANGELOG.md(1 hunks)packages/test/jest.config.ts(2 hunks)packages/test/package.json(1 hunks)packages/wagtail/.storybook/__mocks__/nextFontMock.ts(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)
💤 Files with no reviewable changes (1)
- packages/components/jest.config.ts
✅ Files skipped from review due to trivial changes (6)
- packages/components/cypress/mocks/nextFontMock.ts
- packages/components/.storybook/mocks/nextFontMock.ts
- packages/wagtail/.storybook/mocks/nextFontMock.ts
- packages/design-system/.storybook/mocks/nextFontMock.ts
- packages/wagtail/CHANGELOG.md
- packages/components/mocks/nextFontMock.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/components/package.json
- packages/test/CHANGELOG.md
- packages/components/CHANGELOG.md
- packages/test/package.json
- packages/authentication/package.json
- packages/components/modules/profiles/Members/index.tsx
- packages/test/jest.config.ts
- packages/authentication/modules/profile/useCurrentProfile/tests/useCurrentProfile.test.tsx
🔇 Additional comments (8)
packages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx (4)
5-5: LGTM! Import changes align with the refactoring goals.
The changes to use absolute imports and named fragments improve maintainability.
Also applies to: 12-12
23-23: LGTM! Hook usage simplified.
The simplified destructuring of useCurrentProfile aligns with the refactoring goals.
31-36: LGTM! Well-structured profile update.
The explicit field structure with proper null handling improves type safety and maintainability.
27-30:
Move image URL construction to backend.
The manual construction of absolute image paths in the frontend is fragile and could break if the backend URL structure changes. This should be handled by the backend as indicated by the TODO comment.
packages/authentication/modules/access/useLogin/index.ts (4)
8-8: LGTM: New imports align with PR objectives.
The added imports support the transition to JWT-based profile initialization and new profile state management.
Also applies to: 25-25, 31-31
45-45: LGTM: Clean hook initialization.
The useCurrentProfile hook integration aligns with the transition to Jotai for state management.
59-60:
Add validation for environment variable and image path.
The current implementation has two issues:
- No validation for NEXT_PUBLIC_API_BASE_URL
- Potential path traversal vulnerability in image path construction
- const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL?.replace('/v1', '')
- const absoluteImagePath = user.profile.image ? `${baseUrl}${user.profile.image}` : null
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL
+ if (!baseUrl) {
+ throw new Error('API base URL is not configured')
+ }
+ const sanitizedBaseUrl = baseUrl.replace('/v1', '')
+ const absoluteImagePath = user.profile.image
+ ? `${sanitizedBaseUrl}${user.profile.image.replace(/\.{2,}/g, '')}`
+ : nullLikely invalid or redundant comment.
56-57:
Add error handling for JWT decoding.
The JWT decoding lacks proper error handling which could lead to silent failures.
- const user = decodeJWT<User>(response.access)
- if (user) {
+ try {
+ const user = decodeJWT<User>(response.access)
+ if (!user) {
+ throw new Error('Failed to decode JWT token')
+ }Likely invalid or redundant comment.
| } | ||
| const user = decodeJWT<User>(response.access) | ||
| if (user) { | ||
| // TODO: handle the absolute image path on the backend |
There was a problem hiding this comment.
💡 Codebase verification
Based on the evidence gathered, I can now provide the final response:
Image path handling needs to be moved to the backend as planned
The TODO comment is valid as the current implementation in multiple files shows client-side image path construction:
packages/authentication/modules/access/useLogin/index.tspackages/components/modules/profiles/ProfilePopover/ProfilesList/index.tsx
Both files use NEXT_PUBLIC_API_BASE_URL to construct image URLs on the client side. While there are image URL implementations in the Wagtail module, they appear to be for a different part of the system and already handle complete URLs properly.
🔗 Analysis chain
Backend should handle image path construction.
The TODO comment indicates that image path handling should be moved to the backend. This is still being handled on the client side, which is not aligned with the security best practices.
Let's check if this is being addressed in other parts of the codebase:
Consider creating a dedicated API endpoint for profile data that returns fully qualified image URLs.
Let me verify if there are any backend implementations or ongoing work related to this image path handling.
Let me check one more thing to see if there are any backend API implementations or configurations related to image handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar image path handling patterns
rg -A 3 "NEXT_PUBLIC_API_BASE_URL.*image"
# Check for any backend-related changes
rg -A 3 "TODO.*handle.*backend"
Length of output: 1098
Script:
#!/bin/bash
# Check backend-related files for image URL handling
ast-grep --pattern 'function $FUNC($$$) {
$$$
url: $$$
$$$
}'
# Look for API endpoints related to profile or image handling
rg -l "router.(get|post).*profile"
# Check if there are any recent changes or PRs addressing this
rg -l "image.*url"
Length of output: 1433
Script:
#!/bin/bash
# Look for backend API configurations or implementations
rg -A 5 "createUrl|getImageUrl|imageUrl"
# Check for any image-related configurations in the backend
fd -t f "config|settings" -x rg -l "image.*path|image.*url" {}
Length of output: 6251
* BA-1809 squash all previous commits * BA-1809 Replacing catalogs temporarily, revert later * BA-1809 Fix rebase mistakes * Revert "BA-1809 Replacing catalogs temporarily, revert later" This reverts commit 1071e6e2098c27d40eb28bce840c285805db1b80. * BA-1809 Do not pass currentProfile as props * BA-1809 Fix component tests * BA-1809 Remove duplicated try ... catch * BA-1809 Move useCurrentProfile into authentication package * BA-1809 Remove profileId from Comment props and use useCurrentProfile instead * BA-1809 set current profile in useLogin hook * BA-1809 Fix tests * BA-1809 changesets * BA-1809 pnpm-lock.yaml * BA-1809 Replace catalogs, revert later! * BA-1809 Remove profile id from comment props * BA-1809 Make image paths absolute * BA-1809 Implement remaining gitlab suggestions * BA-1809 Test whether env variable is undefined before using it * tweaks --------- Co-authored-by: Alexandre Anicio <aa@tsl.io>
* BA-1809 [FE] Simplify CurrentProfileProvider (#142) * BA-1809 squash all previous commits * BA-1809 Replacing catalogs temporarily, revert later * BA-1809 Fix rebase mistakes * Revert "BA-1809 Replacing catalogs temporarily, revert later" This reverts commit 1071e6e2098c27d40eb28bce840c285805db1b80. * BA-1809 Do not pass currentProfile as props * BA-1809 Fix component tests * BA-1809 Remove duplicated try ... catch * BA-1809 Move useCurrentProfile into authentication package * BA-1809 Remove profileId from Comment props and use useCurrentProfile instead * BA-1809 set current profile in useLogin hook * BA-1809 Fix tests * BA-1809 changesets * BA-1809 pnpm-lock.yaml * BA-1809 Replace catalogs, revert later! * BA-1809 Remove profile id from comment props * BA-1809 Make image paths absolute * BA-1809 Implement remaining gitlab suggestions * BA-1809 Test whether env variable is undefined before using it * tweaks --------- Co-authored-by: Alexandre Anicio <aa@tsl.io> * feat: render newly created chats in chatrooms list * chore: versioning --------- Co-authored-by: tsl-ps2 <ps2@tsl.io> Co-authored-by: Alexandre Anicio <aa@tsl.io> Co-authored-by: Pedro Tibúrcio <pedrotpoliveira@gmail.com>
* BA-1809 squash all previous commits * BA-1809 Replacing catalogs temporarily, revert later * BA-1809 Fix rebase mistakes * Revert "BA-1809 Replacing catalogs temporarily, revert later" This reverts commit 1071e6e2098c27d40eb28bce840c285805db1b80. * BA-1809 Do not pass currentProfile as props * BA-1809 Fix component tests * BA-1809 Remove duplicated try ... catch * BA-1809 Move useCurrentProfile into authentication package * BA-1809 Remove profileId from Comment props and use useCurrentProfile instead * BA-1809 set current profile in useLogin hook * BA-1809 Fix tests * BA-1809 changesets * BA-1809 pnpm-lock.yaml * BA-1809 Replace catalogs, revert later! * BA-1809 Remove profile id from comment props * BA-1809 Make image paths absolute * BA-1809 Implement remaining gitlab suggestions * BA-1809 Test whether env variable is undefined before using it * tweaks --------- Co-authored-by: Alexandre Anicio <aa@tsl.io>
* BA-1809 [FE] Simplify CurrentProfileProvider (#142) * BA-1809 squash all previous commits * BA-1809 Replacing catalogs temporarily, revert later * BA-1809 Fix rebase mistakes * Revert "BA-1809 Replacing catalogs temporarily, revert later" This reverts commit 1071e6e2098c27d40eb28bce840c285805db1b80. * BA-1809 Do not pass currentProfile as props * BA-1809 Fix component tests * BA-1809 Remove duplicated try ... catch * BA-1809 Move useCurrentProfile into authentication package * BA-1809 Remove profileId from Comment props and use useCurrentProfile instead * BA-1809 set current profile in useLogin hook * BA-1809 Fix tests * BA-1809 changesets * BA-1809 pnpm-lock.yaml * BA-1809 Replace catalogs, revert later! * BA-1809 Remove profile id from comment props * BA-1809 Make image paths absolute * BA-1809 Implement remaining gitlab suggestions * BA-1809 Test whether env variable is undefined before using it * tweaks --------- Co-authored-by: Alexandre Anicio <aa@tsl.io> * chore: versioning * feat: archived room mutation and filter * chore: versioning * fix: PR fixes * chore: fix changelog * feat: unarchive chat * chore: versioning --------- Co-authored-by: tsl-ps2 <ps2@tsl.io> Co-authored-by: Alexandre Anicio <aa@tsl.io> Co-authored-by: Pedro Tibúrcio <pedrotpoliveira@gmail.com>
This refactors the way in which the current profile is provided, switching from Zustand to Jotai for handling the state.
The current profile is now saved to a cookie, which can be sent to the server for SSR.
When logging in, the initial current profile is obtained from JWT claims instead of by querying the server.
The corresponding changes on the backend were implemented in silverlogic/baseapp-backend#189.
Summary by CodeRabbit
Release Notes
New Features
useCurrentProfilehook for managing user profile state with Jotai, enhancing profile management capabilities.InitialProfileProviderfor setting initial profile state in components.MinimalProfiletype for simplified user profile representation.Improvements
CommentCreateand other components by removing theprofileIdprop, transitioning to context-based profile retrieval.AccountPopoverand related components to simplify profile image handling and state management.Memberscomponent to directly utilizecurrentProfilefor improved clarity.Bug Fixes
Chores