BA-hotfix: current profile fixes #152
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 updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 0
🧹 Outside diff range and nitpick comments (4)
packages/authentication/modules/profile/useCurrentProfile/InitialProfileProviderForTesting/types.ts (1)
5-7: LGTM! Consider adding JSDoc comments.The interface is well-defined and properly extends React's PropsWithChildren. The explicit null union type is good for testing edge cases.
Consider adding JSDoc comments to document the purpose of this testing utility:
+/** + * Props for InitialProfileProviderForTesting component used in test scenarios + * to initialize profile state. + */ export interface InitialProfileProviderForTestingProps extends PropsWithChildren { initialProfile: MinimalProfile | null }packages/authentication/modules/profile/useCurrentProfile/InitialProfileProviderForTesting/index.tsx (1)
24-30: Consider memoizing the children prop.The implementation is clean and follows React best practices. However, since this is a testing utility that might wrap large component trees, consider memoizing the children to optimize re-renders.
-const InitialProfileProviderForTesting: FC<InitialProfileProviderForTestingProps> = ({ +const InitialProfileProviderForTesting: FC<InitialProfileProviderForTestingProps> = React.memo(({ initialProfile, children, }) => { useHydrateAtoms([[profileAtom, initialProfile]]) return children -} +})packages/authentication/modules/access/useLogin/index.ts (1)
Line range hint
132-134: Technical Debt: Type-related TODOs need attentionThere are multiple TODOs related to type refactoring and a
@ts-ignorecomment. These should be addressed to improve type safety.Would you like me to help create a GitHub issue to track the type refactoring work? I can propose solutions for:
- Proper typing for the form submission handlers
- Removing the
@ts-ignorecomment by fixing the type mismatchAlso applies to: 145-147
packages/authentication/CHANGELOG.md (1)
7-7: Minor: Maintain consistent hyphenationFor consistency with common technical writing practices, consider using "log-out" when used as a noun or modifier.
-Make sure the log out listener is loaded only once on `useCurrentProfile`. +Make sure the log-out listener is loaded only once on `useCurrentProfile`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: When ‘log-out’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...0.4 ### Patch Changes - Make sure the log out listener is loaded only once on `useCur...(VERB_NOUN_CONFUSION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/modules/access/useLogin/index.ts(1 hunks)packages/authentication/modules/profile/index.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/InitialProfileProviderForTesting/index.tsx(1 hunks)packages/authentication/modules/profile/useCurrentProfile/InitialProfileProviderForTesting/types.ts(1 hunks)packages/authentication/modules/profile/useCurrentProfile/__tests__/__utils__/InitialProfileProvider/index.tsx(0 hunks)packages/authentication/modules/profile/useCurrentProfile/index.ts(1 hunks)packages/authentication/package.json(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/CurrentUser/index.tsx(1 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx(3 hunks)packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/types.ts(1 hunks)packages/components/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/authentication/modules/profile/useCurrentProfile/tests/utils/InitialProfileProvider/index.tsx
✅ Files skipped from review due to trivial changes (3)
- packages/authentication/package.json
- packages/components/package.json
- packages/components/CHANGELOG.md
🧰 Additional context used
🪛 LanguageTool
packages/authentication/CHANGELOG.md
[uncategorized] ~7-~7: When ‘log-out’ is used as a noun or modifier, it needs to be hyphenated.
Context: ...0.4 ### Patch Changes - Make sure the log out listener is loaded only once on `useCur...
(VERB_NOUN_CONFUSION)
🔇 Additional comments (8)
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/types.ts (1)
1-1: LGTM! Type definitions are consistent.
The addition of initialProfile to WithProvidersOptions aligns well with the new testing infrastructure. The import path and type usage are correct.
Let's verify the package dependency is declared:
Also applies to: 7-7
✅ Verification successful
Let me verify if this package is declared in the workspace dependencies or if there are any other relevant package.json files.
Let me check the dependencies in the authentication package and verify if it's properly set up as a workspace dependency.
Let me check the components package's package.json to verify if it has the proper workspace dependency configuration.
The dependency is properly configured
The @baseapp-frontend/authentication package is correctly declared as a peer dependency in the components package's package.json with "workspace:*" version specifier, which is the correct way to handle workspace dependencies in this monorepo setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if @baseapp-frontend/authentication is properly declared in package.json
jq '.dependencies["@baseapp-frontend/authentication"] // .devDependencies["@baseapp-frontend/authentication"]' packages/components/package.json
Length of output: 150
Script:
#!/bin/bash
# Check workspace dependencies in root package.json
echo "Root package.json dependencies:"
jq '.dependencies["@baseapp-frontend/authentication"] // .devDependencies["@baseapp-frontend/authentication"]' package.json
# Find all package.json files
echo -e "\nAll package.json files:"
fd package.json
# Check if authentication package exists in the monorepo
echo -e "\nAuthentication package location:"
fd -t d authentication packages
Length of output: 785
Script:
#!/bin/bash
# Check the authentication package's package.json
echo "Authentication package.json:"
cat packages/authentication/package.json
# Check workspace configuration in root package.json
echo -e "\nWorkspace configuration:"
jq '.workspaces' package.json
Length of output: 2247
Script:
#!/bin/bash
# Check the components package's package.json
cat packages/components/package.json
Length of output: 5227
packages/authentication/modules/profile/index.ts (1)
2-3: Verify the impact of exporting all constants.
The changes look good, but the wildcard export of constants (export * from './useCurrentProfile/constants') needs verification to ensure we're not exposing unintended internal constants.
Let's check the constants being exported:
✅ Verification successful
Wildcard export of constants is safe - only exposes CURRENT_PROFILE_KEY
The verification shows that only one constant CURRENT_PROFILE_KEY is exported from the constants file, and there are no internal/private constants (prefixed with underscore). This is a clean, intentional export that aligns with the module's public API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check what constants are being exposed
ast-grep --pattern 'export const $_ = $_' packages/authentication/modules/profile/useCurrentProfile/constants.ts
# Check for any internal/private constants (prefixed with _)
ast-grep --pattern 'export const _$_ = $_' packages/authentication/modules/profile/useCurrentProfile/constants.ts
Length of output: 355
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/CurrentUser/index.tsx (2)
3-4: LGTM! Clean import declarations.
The removal of the User type import and reliance on the built-in types from useJWTUser simplifies the code while maintaining type safety.
9-9: Verify type inference for user properties.
The removal of generic type parameters from useJWTUser() relies on default type inference. While this simplifies the code, we should ensure the inferred type includes all required user properties (firstName, lastName, email).
✅ Verification successful
Type inference for useJWTUser is correctly implemented and safe
The implementation is type-safe as useJWTUser properly extends both Partial<User> & JWTContent, where:
Userinterface includes all required properties (firstName, lastName, email)- Type inference works correctly through the generic parameter which enforces these types
- The hook's implementation includes proper type guards and error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other useJWTUser implementations to verify consistent typing
ast-grep --pattern 'useJWTUser<$_>($_)'
Length of output: 41
Script:
#!/bin/bash
# Let's first find the useJWTUser hook definition
ast-grep --pattern 'export function useJWTUser$_() {
$$$
}'
# Also search for any type definitions related to JWTUser
rg "type.*JWTUser" -A 5
# And look for actual usage of useJWTUser
rg "useJWTUser" -A 2
Length of output: 6682
Script:
#!/bin/bash
# Let's look at the User type definition and JWTContent
rg "interface User" -A 10
rg "interface JWTContent" -A 10
# And let's check the actual implementation of useJWTUser
cat packages/authentication/modules/user/useJWTUser/index.ts
Length of output: 4900
packages/authentication/modules/profile/useCurrentProfile/InitialProfileProviderForTesting/index.tsx (1)
8-23: Excellent documentation and example usage!
The JSDoc comment provides clear documentation with a practical example, making it easy for other developers to understand and use the component.
packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/withProviders/index.tsx (1)
Line range hint 19-31: LGTM! Clean integration of InitialProfileProviderForTesting.
The changes properly integrate the new testing provider while maintaining the correct provider hierarchy. The props handling is type-safe with proper destructuring of initialProfile.
However, verify that all test files using this utility have been updated with the new initialProfile prop.
✅ Verification successful
Let me search for all test files using withProviders to ensure we have complete coverage.
Let me try a different approach to find the test files.
Let me check the content of AccountPopoverForTesting to see how it uses withProviders.
Let me check the actual test file to see how AccountPopoverForTesting is being used.
All test files have been properly updated with the initialProfile prop
Based on the verification results:
- The
AccountPopoverForTestingcomponent correctly wraps the withProviders HOC - All test cases in
AccountPopover.cy.tsxproperly provide theinitialProfileprop:- Test for no profile:
initialProfile={null} - Test with profile:
initialProfile={mockUserProfileData} - Test with custom props:
initialProfile={mockUserProfileData}
- Test for no profile:
There are no other test files using this withProviders utility, so all usages have been properly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using withProviders to ensure they're updated
rg -l 'withProviders\(' --type ts --type tsx | xargs rg 'initialProfile'
Length of output: 2224
Script:
#!/bin/bash
# Find all test files using withProviders
rg -l 'withProviders\(' --type ts --type tsx
Length of output: 74
Script:
#!/bin/bash
# Find all test files in the AccountPopover directory
fd -e ts -e tsx . packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/
Length of output: 885
Script:
#!/bin/bash
# Check the implementation of AccountPopoverForTesting
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/__utils__/AccountPopoverForTesting/index.tsx
Length of output: 444
Script:
#!/bin/bash
# Check the content of the test file
cat packages/components/modules/navigations/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsx
Length of output: 6408
packages/authentication/modules/profile/useCurrentProfile/index.ts (1)
30-39: 🛠️ Refactor suggestion
Consider consolidating duplicate logout handling logic
There appears to be duplicate logout handling between the profileAtom.onMount handler and the useEffect hook in useCurrentProfile. Both are listening to the same LOGOUT_EVENT and performing similar cleanup actions.
Consider consolidating the logout handling logic to avoid potential race conditions or inconsistencies. Since the atom's onMount handler is guaranteed to run only once per atom instance, prefer using it over the useEffect:
- useEffect(() => {
- eventEmitter.on(LOGOUT_EVENT, removeCurrentProfile)
- return () => {
- eventEmitter.off(LOGOUT_EVENT, removeCurrentProfile)
- }
- }, [])Let's verify if there are any other components relying on the useEffect logout handler:
Also applies to: 67-72
✅ Verification successful
Let me gather more context about the useCurrentProfile hook implementation to understand the complete picture.
Let me try a different approach to get the implementation details.
Remove duplicate logout event handler in useCurrentProfile
The code review is correct. There is indeed a duplicate logout handling:
- In
profileAtom.onMount(lines 30-39) which sets the atom to null and removes the cookie - In
useCurrentProfilehook'suseEffect(lines 67-72) which callsremoveCurrentProfilethat does the same operations
The atom's onMount handler is the better place for this logic because:
- It's guaranteed to run only once per atom instance
- It's closer to the data source (the atom itself)
- It avoids potential race conditions from multiple components using the hook
- Remove the duplicate
useEffecthandler inuseCurrentProfile(lines 67-72) - Keep the
profileAtom.onMountimplementation as it correctly handles both the atom state and cookie cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using LOGOUT_EVENT
rg "LOGOUT_EVENT" --type ts
Length of output: 2332
Script:
#!/bin/bash
# Get the complete implementation of useCurrentProfile
ast-grep --pattern 'export function useCurrentProfile() {
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Get the content of the useCurrentProfile file
cat packages/authentication/modules/profile/useCurrentProfile/index.ts
Length of output: 2451
packages/authentication/modules/access/useLogin/index.ts (1)
60-60: LGTM: Safe property access with optional chaining
The addition of optional chaining operators for accessing nested user profile properties is a good defensive programming practice.
authenticationpackage update -v 4.0.4useCurrentProfile.useLoginadding optional chaining to a problematic expression.InitialProfileProviderForTestingaround.componentspackage update -v 0.0.25InitialProfileProviderForTesting.Summary by CodeRabbit
Release Notes
New Features
InitialProfileProviderForTestingfor testing scenarios.Bug Fixes
useLoginhook with optional chaining for user profile images.Updates
@baseapp-frontend/authenticationpackage to version 4.0.4.@baseapp-frontend/componentspackage to version 0.0.25.Chores