BA-2020: mobile internal testing #172
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/__mocks__/expo.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/authentication/.eslintrc.js
WalkthroughThis pull request introduces a comprehensive set of changes focusing on platform-specific handling, particularly for mobile environments. The modifications span multiple packages, with a primary emphasis on improving token management, environment configuration, and platform detection. Key changes include introducing a new Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/graphql/utils/createTestEnvironment/index.ts (1)
55-55: Consider using a centralized or structured logger.Logging with
console.warnhere may be sufficient for testing, but using a dedicated logging library or standardized approach across the codebase usually makes debugging and monitoring more consistent—especially in production. Consider discussing whether to replace or augment this line with a project-standard logger.packages/authentication/modules/access/useLogin/index.ts (1)
58-70: Refactor to handle user image path.You're building an absolute image path on-the-fly. This logic is repeated in some codebases and might be better extracted into a helper function or utility, which would make your code more consistent and testable. Also, confirm that this logic covers use cases where
NEXT_PUBLIC_API_BASE_URLis undefined or missing required segments.pnpm-workspace.yaml (1)
130-131: Add a newline at end of file.The static analysis tool flagged a missing newline at the end of the file. Consider adding one to comply with style guidelines.
react-native-core: expo-secure-store: ~14.0.0 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 131-131: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/authentication/modules/access/useLogin/index.ts(2 hunks)packages/graphql/utils/createTestEnvironment/index.ts(1 hunks)packages/utils/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
pnpm-workspace.yaml
[error] 131-131: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/authentication/modules/access/useLogin/index.ts (3)
56-68: Handle mobile login flow.There's a
TODOat line 57 indicating a need for mobile support. Currently, the code bypasses JWT decoding for mobile. Verify that this is intentional and won't break core authentication features on mobile devices. If you plan on implementing a different flow for mobile, consider adding documentation or clarifying comments reflecting that approach.
99-99: Await asynchronous calls before subsequent operations.Good use of
awaitto ensure tokens are fully set before continuing. This avoids potential race conditions. No issues here.
101-101: Ensure onSuccess callback requirements are met.You are calling
loginOptions?.onSuccessafter awaitinghandleLoginSuccess. Verify that no part ofonSuccessdepends on tokens set inhandleLoginSuccessbefore they are actually set. Currently, it looks correct, but remain mindful if you add more complex logic in the future.packages/utils/package.json (3)
17-29: Confirm version constraints and compatibility with the codebase.You've updated
axios,expo-secure-store,js-cookie,lodash,luxon,next,react-hook-form, andzustandto pinned versions. This can improve reproducibility, but please verify there are no breaking changes that might affect upstream or downstream dependencies.
32-32: Pinned React version.Pinning React to
18.3.1helps ensure consistent behavior. Review any library that depends on React to ensure compatibility with this exact version rather than a^or~range.
38-55: Validate dev dependencies.These test and type definition libraries (e.g., jest-related packages,
@types/*,ts-jest, TypeScript) have been updated or pinned. Confirm your test suites still pass and that your CI/CD pipeline is updated if any of these versions introduce new lint or type-checking behaviors.
anicioalexandre
force-pushed
the
BA-2020-mobile-internal-testing
branch
from
c8871f9 to
da9a073
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/utils/package.json (1)
17-29: Standardize version specifier formatThe dependencies section mixes different version specifier formats:
- Fixed versions:
"axios": "1.7.2"- Caret ranges:
"events": "^3.3.0"- Tilde ranges:
"expo-secure-store": "~14.0.0"Consider standardizing to fixed versions for better reproducibility, especially for a mobile testing environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/authentication/package.json(1 hunks)packages/utils/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (5)
packages/authentication/package.json (3)
26-26: Verify React pre-release version compatibilityThe PR updates React to version 18.3.1 which is a pre-release version. This could potentially introduce instability in a production environment.
Please confirm:
- Was this update intentional for mobile internal testing?
- Have you tested the application thoroughly with this pre-release version?
33-47: Verify test suite compatibilityThe PR updates all testing libraries to very recent versions:
- @testing-library/react: 16.0.0
- jest: 29.7.0
Please confirm:
- All tests pass with the new versions
- No deprecated test APIs are in use
Run this script to check for potential test issues:
✅ Verification successful
Test suite compatibility verified ✅
The codebase is using modern testing patterns and no deprecated APIs were found. The test suite appears fully compatible with the updated versions:
- All test files follow current best practices
- No deprecated cleanup/fireEvent patterns detected
- Proper async/await patterns with waitFor
- Centralized test utilities with proper re-exports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for deprecated testing patterns rg -l 'render\(' | xargs rg -l 'cleanup|fireEvent|waitFor'Length of output: 60
Script:
#!/bin/bash # Find test files echo "=== Test Files ===" fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts # Check testing library imports echo -e "\n=== Testing Library Imports ===" rg "@testing-library/react" -A 2 -B 2 # Check render patterns echo -e "\n=== Render Patterns ===" ast-grep --pattern 'render($$$)' # Check specific testing patterns echo -e "\n=== Testing Patterns ===" rg "fireEvent\.|waitFor\(|cleanup\(" -A 2Length of output: 13429
17-22: Review major version updates in core dependenciesSeveral dependencies have significant version updates that may include breaking changes:
- @tanstack/react-query: 5.x (major version)
- react-hook-form: 7.51.x
Please ensure:
- Breaking changes have been addressed
- The application has been tested with these versions
- These versions are compatible with mobile platforms
Run this script to check for any breaking changes documentation:
✅ Verification successful
Dependencies update verification completed successfully
The codebase shows clear evidence that:
- React Query v5 breaking changes have been properly addressed and documented
- Dependencies are consistently versioned across all packages
- Mobile platform compatibility is maintained through proper cross-platform dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for migration guides or breaking changes in the repository rg -i "breaking|migration|upgrade" --type mdLength of output: 347
Script:
#!/bin/bash # Check the actual changes made for React Query v5 migration echo "=== Checking React Query related changes ===" rg -l "useQuery|useMutation|QueryClient" --type ts --type tsx echo -e "\n=== Checking mobile-specific code ===" rg -l "mobile|platform|ios|android" --type ts --type tsx echo -e "\n=== Checking react-hook-form usage ===" rg -l "useForm|FormProvider" --type ts --type tsxLength of output: 541
Script:
#!/bin/bash # Check React Query migration and usage echo "=== React Query Usage ===" rg -l "useQuery|useMutation|QueryClient" "*.ts" "*.tsx" echo -e "\n=== Mobile Platform Specific Code ===" rg -l "mobile|platform|ios|android" "*.ts" "*.tsx" echo -e "\n=== React Hook Form Usage ===" rg -l "useForm|FormProvider" "*.ts" "*.tsx" echo -e "\n=== Package Dependencies ===" find . -name "package.json" -exec sh -c 'echo "=== {} ==="; cat {}' \;Length of output: 27467
packages/utils/package.json (2)
17-55: Ensure version alignment between packagesThe dependencies in utils and authentication packages should be aligned where they overlap:
- react-hook-form: 7.51.5 ✓
- js-cookie: 3.0.5 ✓
- React: 18.3.1 ✓
Good job maintaining version consistency between packages!
19-19: Verify expo-secure-store mobile compatibilityThe update to expo-secure-store ~14.0.0 is relevant for mobile testing.
Please confirm:
- This version is compatible with your React Native version
- Mobile-specific storage features have been tested
Run this script to check for mobile-specific code patterns:
packages/utils/package.json
Outdated
| "next": "catalog:", | ||
| "lodash": "4.17.21", | ||
| "luxon": "3.4.4", | ||
| "next": "14.3.0-canary.24", |
There was a problem hiding this comment.
Remove next.js canary version from production dependencies
Using a canary version (14.3.0-canary.24) of next.js in production dependencies is risky as these versions are unstable and intended for testing.
Replace with the latest stable version:
- "next": "14.3.0-canary.24",
+ "next": "14.0.4",📝 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.
| "next": "14.3.0-canary.24", | |
| "next": "14.0.4", |
anicioalexandre
force-pushed
the
BA-2020-mobile-internal-testing
branch
from
da9a073 to
033bf42
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/utils/package.json (1)
25-25:⚠️ Potential issueRemove next.js canary version from production dependencies
Using a canary version (
14.3.0-canary.24) of next.js in production dependencies is risky as these versions are unstable and intended for testing.- "next": "14.3.0-canary.24", + "next": "14.0.4",
🧹 Nitpick comments (6)
packages/utils/functions/token/index.ts (2)
9-12: Add JSDoc comments for new exportsThe new token management functions would benefit from JSDoc documentation explaining their purpose and differences.
+/** + * Generic token getter that works with custom storage implementations + */ export * from './getTokenAsyncGeneric' +/** + * Mobile-specific token getter using secure storage + */ export * from './getTokenAsyncMobileOnly' +/** + * Generic token setter that works with custom storage implementations + */ export * from './setTokenAsyncGeneric' +/** + * Mobile-specific token setter using secure storage + */ export * from './setTokenAsyncMobileOnly'
Line range hint
1-1: Consider adding integration tests for token managementThe new token management implementation would benefit from:
- Integration tests covering both mobile and generic implementations
- Documentation explaining when to use each implementation
- Error boundary setup to handle token-related failures gracefully
Would you like me to help create a test plan or documentation template for these implementations?
packages/utils/functions/token/getTokenAsyncGeneric/index.ts (2)
7-9: Add JSDoc documentation for the TokenGetter interface.Consider adding JSDoc documentation to improve code maintainability and IDE support.
+/** + * Generic interface for token getter functions + * @param key - The key to retrieve the token + * @param config - Optional configuration object + * @returns Promise resolving to the token string or null if not found + */ export interface TokenGetter<ConfigType extends object = Record<string, unknown>> { (key: string, config?: ConfigType): Promise<string | null> }
11-20: Add error handling for the token getter function.The current implementation silently passes through any errors from the getter function. Consider adding try-catch block to handle potential errors gracefully.
export const getTokenAsyncGeneric = async < ConfigType extends Record<string, unknown> = DefaultOptions, >( key = ACCESS_KEY_NAME, config?: ConfigType, getter: TokenGetter<ConfigType> = getCookieAsync, ) => { - const token = await getter(key, config) - return token + try { + const token = await getter(key, config) + return token + } catch (error) { + console.error(`Failed to get token for key ${key}:`, error) + return null + } }packages/authentication/package.json (1)
33-33: Consider using exact version for @faker-js/faker.The caret (
^) in the version constraint"^9.0.3"allows minor and patch updates, which could lead to inconsistent test behavior. Consider using an exact version for test dependencies.- "@faker-js/faker": "^9.0.3", + "@faker-js/faker": "9.0.3",pnpm-workspace.yaml (1)
130-131: Add newline at end of file.YAML files should end with a newline character.
react-native-core: - expo-secure-store: ~14.0.0 + expo-secure-store: ~14.0.0 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 131-131: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/authentication/modules/access/useLogin/index.ts(2 hunks)packages/authentication/package.json(1 hunks)packages/graphql/utils/createTestEnvironment/index.ts(1 hunks)packages/utils/functions/token/getTokenAsyncGeneric/index.ts(1 hunks)packages/utils/functions/token/getTokenAsyncMobileOnly/index.ts(1 hunks)packages/utils/functions/token/index.ts(1 hunks)packages/utils/functions/token/setTokenAsyncGeneric/index.ts(1 hunks)packages/utils/functions/token/setTokenAsyncMobileOnly/index.ts(1 hunks)packages/utils/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/graphql/utils/createTestEnvironment/index.ts
- packages/authentication/modules/access/useLogin/index.ts
🧰 Additional context used
🪛 GitHub Check: Lint
packages/utils/functions/token/setTokenAsyncMobileOnly/index.ts
[warning] 7-7:
Unexpected console statement
packages/utils/functions/token/setTokenAsyncGeneric/index.ts
[warning] 19-19:
Unexpected console statement
🪛 yamllint (1.35.1)
pnpm-workspace.yaml
[error] 131-131: no new line character at the end of file
(new-line-at-end-of-file)
| export const getTokenAsyncMobileOnly = async (key = ACCESS_KEY_NAME) => { | ||
| const token = await getItemAsync(key) | ||
| return token | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and explicit return type
The implementation should handle potential errors from getItemAsync and specify the return type for better type safety.
-export const getTokenAsyncMobileOnly = async (key = ACCESS_KEY_NAME) => {
+export const getTokenAsyncMobileOnly = async (key = ACCESS_KEY_NAME): Promise<string | null> => {
+ try {
const token = await getItemAsync(key)
return token
+ } catch (error) {
+ console.error(`Failed to get token for ${key}:`, error)
+ return null
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getTokenAsyncMobileOnly = async (key = ACCESS_KEY_NAME) => { | |
| const token = await getItemAsync(key) | |
| return token | |
| } | |
| export const getTokenAsyncMobileOnly = async (key = ACCESS_KEY_NAME): Promise<string | null> => { | |
| try { | |
| const token = await getItemAsync(key) | |
| return token | |
| } catch (error) { | |
| console.error(`Failed to get token for ${key}:`, error) | |
| return null | |
| } | |
| } |
| export const setTokenAsyncMobileOnly = async (key: string, value: string) => { | ||
| try { | ||
| await setItemAsync(key, value) | ||
| } catch (error) { | ||
| console.error(`Failed to set token for ${key}:`, error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return success status and improve error handling
The function should return a boolean indicating success/failure and use a more appropriate logging approach for mobile environments.
-export const setTokenAsyncMobileOnly = async (key: string, value: string) => {
+export const setTokenAsyncMobileOnly = async (key: string, value: string): Promise<boolean> => {
try {
await setItemAsync(key, value)
+ return true
} catch (error) {
- console.error(`Failed to set token for ${key}:`, error)
+ // TODO: Use appropriate mobile logging service
+ console.error(`Failed to set token for ${key}:`, error instanceof Error ? error.message : String(error))
+ return false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const setTokenAsyncMobileOnly = async (key: string, value: string) => { | |
| try { | |
| await setItemAsync(key, value) | |
| } catch (error) { | |
| console.error(`Failed to set token for ${key}:`, error) | |
| } | |
| } | |
| export const setTokenAsyncMobileOnly = async (key: string, value: string): Promise<boolean> => { | |
| try { | |
| await setItemAsync(key, value) | |
| return true | |
| } catch (error) { | |
| // TODO: Use appropriate mobile logging service | |
| console.error(`Failed to set token for ${key}:`, error instanceof Error ? error.message : String(error)) | |
| return false | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[warning] 7-7:
Unexpected console statement
| export const setTokenAsyncGeneric = async < | ||
| ConfigType extends Record<string, unknown> = SetCookieOptions, | ||
| >( | ||
| key: string, | ||
| value: string, | ||
| config?: ConfigType, | ||
| setter: TokenSetter<ConfigType> = setCookie, | ||
| ) => { | ||
| try { | ||
| await setter(key, value, config) | ||
| } catch (error) { | ||
| console.error(`Failed to set token for ${key}:`, error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align error handling with mobile implementation
For consistency across implementations, this function should also return a boolean status and use improved error handling.
export const setTokenAsyncGeneric = async <
ConfigType extends Record<string, unknown> = SetCookieOptions,
>(
key: string,
value: string,
config?: ConfigType,
setter: TokenSetter<ConfigType> = setCookie,
-) => {
+): Promise<boolean> => {
try {
await setter(key, value, config)
+ return true
} catch (error) {
- console.error(`Failed to set token for ${key}:`, error)
+ console.error(`Failed to set token for ${key}:`, error instanceof Error ? error.message : String(error))
+ return false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const setTokenAsyncGeneric = async < | |
| ConfigType extends Record<string, unknown> = SetCookieOptions, | |
| >( | |
| key: string, | |
| value: string, | |
| config?: ConfigType, | |
| setter: TokenSetter<ConfigType> = setCookie, | |
| ) => { | |
| try { | |
| await setter(key, value, config) | |
| } catch (error) { | |
| console.error(`Failed to set token for ${key}:`, error) | |
| } | |
| } | |
| export const setTokenAsyncGeneric = async < | |
| ConfigType extends Record<string, unknown> = SetCookieOptions, | |
| >( | |
| key: string, | |
| value: string, | |
| config?: ConfigType, | |
| setter: TokenSetter<ConfigType> = setCookie, | |
| ): Promise<boolean> => { | |
| try { | |
| await setter(key, value, config) | |
| return true | |
| } catch (error) { | |
| console.error(`Failed to set token for ${key}:`, error instanceof Error ? error.message : String(error)) | |
| return false | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[warning] 19-19:
Unexpected console statement
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pnpm-workspace.yaml (1)
132-132: Add newline at end of file.Add a newline character at the end of the file to follow YAML conventions and fix the linting error.
react-native-core: expo-secure-store: ~14.0.0 - react-native: 0.76.3 + react-native: 0.76.3 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
packages/utils/functions/token/setTokenAsync/index.ts (2)
3-3: Consider optimizing platform-specific checksThe current implementation has potentially redundant platform checks. If
EXPO_PUBLIC_PLATFORM === 'mobile'is true, checkingPlatform.OSbecomes unnecessary. Consider refactoring to make the logic more maintainable:-import { Platform } from 'react-native' +import { Platform } from 'react-native' +const isMobilePlatform = process.env.EXPO_PUBLIC_PLATFORM === 'mobile' || + Platform.OS === 'ios' || Platform.OS === 'android' export const setTokenAsync = async (key: string, value: string, config?: CookieAttributes) => { try { - if ( - process.env.EXPO_PUBLIC_PLATFORM === 'mobile' || - Platform.OS === 'ios' || - Platform.OS === 'android' - ) { + if (isMobilePlatform) {Also applies to: 9-13
Line range hint
1-20: Enhance error handling specificityThe current error handling catches all errors generically. Consider adding specific error handling for different scenarios (e.g., storage errors vs network errors) and providing more context in the error message.
try { if (isMobilePlatform) { await setItemAsync(key, value) } else { setCookie(key, value, config) } -} catch (error) { - console.error(`Failed to set token for ${key}:`, error) +} catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : String(error) + console.error( + `Failed to set token for ${key} on platform ${ + isMobilePlatform ? 'mobile' : 'web' + }: ${errorMessage}` + ) + throw error // Re-throw to allow handling by caller }packages/utils/functions/token/getTokenAsync/index.ts (1)
Line range hint
8-24: Enhance type safety and error handlingConsider adding stronger typing for the return value and improving error handling:
export const getTokenAsync = async ( key = ACCESS_KEY_NAME, { noSSR = false }: ServerSideRenderingOption = {}, -) => { +): Promise<string | null> => { if (isMobilePlatform) { - const token = await getItemAsync(key) - return token + try { + return await getItemAsync(key) + } catch (error: unknown) { + console.error(`Failed to get token from secure store: ${error}`) + return null + } } - const token = await getCookieAsync<string>(key, { noSSR }) - return token + try { + return await getCookieAsync<string>(key, { noSSR }) + } catch (error: unknown) { + console.error(`Failed to get token from cookie: ${error}`) + return null + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/utils/functions/token/getTokenAsync/index.ts(2 hunks)packages/utils/functions/token/setTokenAsync/index.ts(1 hunks)packages/utils/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
pnpm-workspace.yaml
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Applications and Packages
- GitHub Check: Lint
🔇 Additional comments (7)
pnpm-workspace.yaml (2)
Line range hint
1-132: Well-organized workspace configuration!The workspace configuration maintains a clean and logical organization of dependencies into catalogs, with consistent formatting and version constraints.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
130-132: Verify React Native dependencies versions and security.The new
react-native-corecatalog looks good, but let's verify the versions:Additionally, consider:
- Adding
@types/react-nativeto ensure proper TypeScript support- Including other essential React Native dependencies like
react-native-reanimatedif animations will be used🧰 Tools
🪛 yamllint (1.35.1)
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
packages/utils/functions/token/getTokenAsync/index.ts (1)
Line range hint
1-24: Verify platform-specific token handlingSince this is a WIP PR for mobile internal testing, ensure comprehensive testing of token persistence across app restarts and different mobile scenarios.
✅ Verification successful
Token handling implementation is properly tested ✅
The implementation includes comprehensive test coverage for mobile token persistence:
- Mobile platform using expo-secure-store
- Platform-specific logic separation
- Proper mocking of secure storage
- Token retrieval verification across different scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token-related test files echo "Searching for token-related test files..." rg -l "getTokenAsync|setTokenAsync" -g "*.test.*" -g "*.spec.*" # Look for existing mobile platform tests echo "Checking for mobile platform test coverage..." rg -l "Platform.OS.*test" -g "*.test.*" -g "*.spec.*"Length of output: 647
Script:
#!/bin/bash # Check content of token-related test files echo "Content of getTokenAsync client test:" cat "packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.client.test.ts" echo -e "\nContent of getTokenAsync server test:" cat "packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.server.test.ts" # Look for expo-secure-store mocking echo -e "\nChecking for expo-secure-store mocks:" rg "expo-secure-store" -g "*.test.*" -g "*.spec.*" -g "*mock*"Length of output: 5342
packages/utils/package.json (4)
25-25: Remove next.js canary version from production dependenciesUsing a canary version (
14.3.0-canary.24) of next.js in production dependencies is risky as these versions are unstable and intended for testing.Replace with the latest stable version:
- "next": "14.3.0-canary.24", + "next": "14.0.4",
17-30: Verify compatibility with updated dependency versionsSeveral dependencies have been updated with significant version changes. Please verify compatibility, especially for:
- expo-secure-store (~14.0.0)
- react-hook-form (7.51.5)
- zustand (4.5.2)
Let's check for any breaking changes that might affect the codebase:
✅ Verification successful
Dependencies update verification completed - No compatibility issues found
The codebase uses standard APIs and patterns that are well-supported across these versions. No breaking changes or deprecated feature usage detected that would affect the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of potentially affected APIs rg -A 3 'useForm|create\s*\(|SecureStore' --type tsLength of output: 26311
33-33: Verify React 18.3.1 compatibilityThe React peer dependency has been updated to 18.3.1. This is a significant version update that might affect compatibility with other packages and the application itself.
Please verify:
- Compatibility with other dependencies
- Any required updates to React-specific code
Let's check for potential compatibility issues:
✅ Verification successful
React 18.3.1 version update is consistent
The version update is properly synchronized across all packages in the monorepo, with consistent type definitions (@types/[email protected], @types/[email protected]) and Next.js integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for React version requirements in other package.json files fd package.json | xargs jq -r 'select(.peerDependencies != null) | select(.peerDependencies.react != null) | "\(.name): React \(.peerDependencies.react)"'Length of output: 542
Script:
#!/bin/bash # Check for direct React dependencies and devDependencies in package.json files echo "=== Direct React dependencies ===" fd package.json | xargs jq -r 'select(.dependencies != null) | select(.dependencies.react != null) | "\(.name): React \(.dependencies.react)"' echo -e "\n=== React devDependencies ===" fd package.json | xargs jq -r 'select(.devDependencies != null) | select(.devDependencies.react != null) | "\(.name): React \(.devDependencies.react)"' # Look for documentation about catalog:react18 echo -e "\n=== Searching for catalog:react18 documentation ===" rg -i "catalog:react18" -A 5 -B 5Length of output: 16140
39-56: Verify compatibility with updated dev dependenciesSignificant updates to testing libraries and TypeScript:
- TypeScript 5.4.5 might introduce new type checking rules
- Testing libraries (@testing-library/react 16.0.0) might have breaking changes
Let's check for potential issues:
✅ Verification successful
Dependencies update appears safe due to abstraction layer
The codebase has a well-structured test setup with a custom abstraction layer in
packages/test/index.tsxthat shields components from direct breaking changes. The TypeScript configuration is properly maintained across packages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for TypeScript errors with new version tsc --noEmit # Check for deprecated testing patterns rg -l 'render|fireEvent|waitFor|screen' --type testLength of output: 191
Script:
#!/bin/bash # Find test files fd -e test.tsx -e test.ts -e test.jsx -e test.js # Find TypeScript config fd tsconfig.json # Check testing patterns in found files rg -l '@testing-library/(react|user-event|jest-dom)' # Look for testing library usage patterns ast-grep --pattern 'import { $$$ } from "@testing-library/react"'Length of output: 4093
Script:
#!/bin/bash # Check test setup and patterns cat packages/test/index.tsx # Look for specific testing patterns that might be affected by v16 rg -A 2 'render\(' packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsx rg -A 2 'fireEvent\.' packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsxLength of output: 1155
| @@ -1,4 +1,5 @@ | |||
| import { getItemAsync } from 'expo-secure-store' | |||
| import { Platform } from 'react-native' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency with setTokenAsync implementation
For better maintainability and DRY principles, consider extracting the platform check logic into a shared utility:
+// In a new file: utils/platform.ts
+export const isMobilePlatform =
+ process.env.EXPO_PUBLIC_PLATFORM === 'mobile' ||
+ Platform.OS === 'ios' ||
+ Platform.OS === 'android'
- if (
- process.env.EXPO_PUBLIC_PLATFORM === 'mobile' ||
- Platform.OS === 'ios' ||
- Platform.OS === 'android'
- ) {
+ if (isMobilePlatform) {Also applies to: 12-16
packages/utils/package.json
Outdated
| "qs": "^6.12.1", | ||
| "react-hook-form": "catalog:", | ||
| "react-hook-form": "7.51.5", | ||
| "react-native": "0.76.3", |
There was a problem hiding this comment.
💡 Codebase verification
Move react-native to peer dependencies
The codebase shows intentional cross-platform support with proper platform-specific handling, but react-native should be a peer dependency since:
- It's only used for platform detection
- Allows consumers to control the react-native version in their environment
- Follows best practices for cross-platform packages
🔗 Analysis chain
Verify react-native integration requirements
The addition of react-native as a direct dependency suggests this package now supports mobile platforms. Please verify:
- Is this intentional or should it be a peer dependency?
- Are there any platform-specific code paths that need to be handled?
Let's check the codebase for platform-specific handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for platform-specific imports or conditions
rg -l 'Platform|react-native' --type ts
Length of output: 187
Script:
#!/bin/bash
# Check the content of files with react-native references
echo "=== token/setTokenAsync/index.ts ==="
cat packages/utils/functions/token/setTokenAsync/index.ts
echo -e "\n=== token/getTokenAsync/index.ts ==="
cat packages/utils/functions/token/getTokenAsync/index.ts
echo -e "\n=== package.json peer dependencies ==="
cat packages/utils/package.json | jq '.peerDependencies'
Length of output: 1713
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/utils/functions/token/removeTokenAsync/index.ts (1)
Line range hint
13-13: Correct the error messageIn the catch block, the error message should accurately reflect the operation being performed. It currently says 'Failed to set token' but should be 'Failed to remove token'.
Apply this diff to correct the error message:
- console.error(`Failed to set token for ${key}:`, error) + console.error(`Failed to remove token for ${key}:`, error)
🧹 Nitpick comments (11)
packages/authentication/modules/user/getUser/__tests__/getUser.test.ts (2)
6-8: Consider maintaining consistent semicolon usage in mock setupThe mock setup is clean and follows Jest best practices, but there's an inconsistency in semicolon usage between the opening and closing lines.
jest.mock('@baseapp-frontend/utils/functions/token/getToken', () => ({ - getToken: jest.fn(), + getToken: jest.fn() }))
19-22: Consider adding a mock function call verificationWhile the test correctly verifies the null return, it would be more robust to also verify that the
getTokenfunction was actually called.it('should return null if no token is set', () => { ;(getToken as jest.Mock).mockReturnValue(undefined) const user = getUser() expect(user).toBeNull() + expect(getToken).toHaveBeenCalled() })packages/authentication/modules/access/useLogin/index.ts (2)
58-60: Reminder: Implement mobile login flowThe
handleLoginSuccessfunction currently skips profile setup on mobile platforms due to theif (!isMobilePlatform())condition. The TODO comment suggests adapting this flow for mobile. Consider implementing the necessary logic to handle login success on mobile platforms to ensure a consistent user experience.Do you want assistance in adapting the login flow for mobile platforms or opening a GitHub issue to track this task?
62-64: Suggestion: Handle image path on the backendConstructing the absolute image path on the frontend can lead to maintainability issues and potential errors if the base URL changes. The TODO comment indicates handling this on the backend. Consider updating the backend to provide the absolute image URL to simplify the frontend logic.
packages/utils/functions/os/__tests__/os.test.ts (1)
1-41: Add test case for unexpected Platform.OS values.The test suite comprehensively covers the main platform cases. Consider adding a test for unexpected
Platform.OSvalues to ensure robust error handling.+ it('returns false for unexpected platform values', () => { + jest.doMock('react-native', () => ({ + Platform: { OS: 'unexpected_platform' }, + })) + + const { isMobilePlatform } = require('..') + expect(isMobilePlatform()).toBe(false) + })packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts (1)
31-31: Consider enhancing test descriptions for better clarity.The test descriptions could be more specific about the expected behavior. Consider:
- "should use SecureStore for token retrieval when on mobile platform"
- "should use getCookie for token retrieval when on non-mobile platform"
- "should use getCookie with noSSR flag when client-side retrieval is needed"
Also applies to: 42-42, 53-53
packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts (1)
Line range hint
47-54: Consider adding error logging in error handling tests.While the error handling tests verify that errors don't propagate, it would be valuable to ensure errors are logged for debugging purposes.
it('should not throw error when deleteItemAsync fails on mobile', async () => { ;(isMobilePlatform as jest.Mock).mockReturnValue(true) ;(deleteItemAsync as jest.Mock).mockImplementationOnce(async () => { throw new Error('SecureStore Error') }) + const consoleSpy = jest.spyOn(console, 'error').mockImplementation() await expect(removeTokenAsync(mockKey)).resolves.not.toThrow() + expect(consoleSpy).toHaveBeenCalledWith('Failed to remove token:', expect.any(Error)) + consoleSpy.mockRestore() expect(deleteItemAsync).toHaveBeenCalledWith(mockKey) expect(removeCookie).not.toHaveBeenCalled() })Also applies to: 59-66
packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts (1)
Line range hint
28-36: Consider adding token format validation tests.While the implementation correctly handles token storage, it would be beneficial to add tests verifying the token format (e.g., JWT format if applicable).
it('should validate token format before storage', async () => { ;(isMobilePlatform as jest.Mock).mockReturnValue(true) const invalidToken = 'invalid-token-format' await expect(setTokenAsync(mockKey, invalidToken)) .rejects .toThrow('Invalid token format') })packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts (1)
68-75: Consider adding a specific error messageThe test could be more explicit about the expected error when no refresh token is available. Consider updating the test to verify a specific error message that would help in debugging.
- await expect(refreshAccessToken()).rejects.toThrow() + await expect(refreshAccessToken()).rejects.toThrow('No refresh token available')packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (1)
20-20: Consider using dynamic timestamps in JWT mockThe hardcoded timestamp
1234567890indecodeJWTmock could lead to test failures if time-based logic is added. Consider using a dynamic timestamp relative to the current time.- decodeJWT: jest.fn(() => ({ exp: 1234567890 })), + decodeJWT: jest.fn(() => ({ exp: Math.floor(Date.now() / 1000) + 3600 })),packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts (1)
11-26: Consider adding test cases for token edge cases.While the current tests cover the basic scenarios, consider adding test cases for:
- Invalid token format
- Expired tokens
- Tokens with missing user fields
Example implementation:
it('should handle invalid token format', async () => { ;(getTokenAsync as jest.Mock).mockReturnValue('invalid-token') const user = await getUserAsync() expect(user).toBeNull() }) it('should handle expired token', async () => { ;(getTokenAsync as jest.Mock).mockReturnValue('expired.token.here') const user = await getUserAsync() expect(user).toBeNull() }) it('should handle token with missing user fields', async () => { ;(getTokenAsync as jest.Mock).mockReturnValue('token.with.incomplete.user') const user = await getUserAsync() expect(user?.email).toBeUndefined() expect(user?.firstName).toBeUndefined() expect(user?.lastName).toBeUndefined() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
packages/authentication/__mocks__/react-native.ts(1 hunks)packages/authentication/modules/access/useLogin/index.ts(3 hunks)packages/authentication/modules/user/getUser/__tests__/getUser.test.ts(1 hunks)packages/authentication/modules/user/getUser/index.ts(1 hunks)packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts(1 hunks)packages/authentication/modules/user/getUserAsync/index.ts(1 hunks)packages/components/__mocks__/react-native.ts(1 hunks)packages/test/__mocks__/react-native.ts(1 hunks)packages/test/jest.config.ts(1 hunks)packages/utils/__mocks__/react-native.ts(1 hunks)packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts(1 hunks)packages/utils/functions/axios/createAxiosInstance/index.ts(1 hunks)packages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.ts(4 hunks)packages/utils/functions/fetch/baseAppFetch/index.ts(2 hunks)packages/utils/functions/os/__tests__/os.test.ts(1 hunks)packages/utils/functions/os/index.ts(1 hunks)packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts(5 hunks)packages/utils/functions/token/getToken/__tests__/getToken.server.test.ts(2 hunks)packages/utils/functions/token/getToken/index.ts(1 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.client.test.ts(5 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.server.test.ts(2 hunks)packages/utils/functions/token/getTokenAsync/index.ts(1 hunks)packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts(1 hunks)packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts(6 hunks)packages/utils/functions/token/removeTokenAsync/index.ts(1 hunks)packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts(7 hunks)packages/utils/functions/token/setTokenAsync/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- packages/utils/mocks/react-native.ts
- packages/components/mocks/react-native.ts
- packages/authentication/modules/user/getUser/index.ts
- packages/authentication/modules/user/getUserAsync/index.ts
- packages/test/mocks/react-native.ts
- packages/utils/functions/axios/createAxiosInstance/index.ts
- packages/utils/functions/fetch/baseAppFetch/tests/baseAppFetch.test.ts
- packages/authentication/mocks/react-native.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/utils/functions/token/setTokenAsync/index.ts
- packages/utils/functions/token/getTokenAsync/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (20)
packages/authentication/modules/user/getUser/__tests__/getUser.test.ts (2)
11-17: Well-structured test with comprehensive assertions!The test case effectively verifies multiple user properties and follows good testing practices with clear mocking and assertions.
1-1: Verify consistent token-based approach implementationThe transition from cookies to tokens represents a significant architectural change. Let's verify the consistent implementation across related files.
✅ Verification successful
Token implementation correctly uses cookies for storage
The codebase shows a well-structured implementation where the token-based authentication properly uses cookies as the secure storage mechanism. This is a common and recommended pattern, not an inconsistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining cookie-based authentication implementations rg -l "js-cookie|getCookie|setCookie" --type ts # Search for consistent token usage pattern rg -l "getToken.*@baseapp-frontend/utils/functions/token" --type tsLength of output: 2241
packages/utils/functions/fetch/baseAppFetch/index.ts (2)
93-93: LGTM! Improved Accept header flexibility.The broader Accept header allows for more flexible responses while maintaining JSON as the primary format.
138-138: LGTM! Explicit charset specification.Explicitly specifying UTF-8 charset in Content-Type header is a good practice. However, let's verify that all API endpoints properly handle this charset specification.
✅ Verification successful
Charset specification is safe and properly implemented
The explicit UTF-8 charset specification in baseAppFetch is well-implemented:
- Centralized in the base fetch utility for consistency
- No conflicting charset specifications or handling found in the codebase
- No evidence of API endpoints rejecting or handling charset differently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any API endpoints explicitly reject or handle charset differently # Look for charset-related error handling or content-type parsing in the codebase # Search for content-type header parsing rg -i "content-type.*charset" --type ts # Search for explicit charset handling rg -i "charset=(utf-8|UTF-8)" --type ts # Search for potential charset-related error handling rg -i "UnsupportedMediaType|415|InvalidCharset" --type tsLength of output: 2170
Script:
#!/bin/bash # Search for any other Content-Type header settings rg -i "('|\")Content-Type('|\").*:.*application/json" --type ts # Search for header setting patterns that might override our Content-Type rg -i "headers\s*=|headers:|setHeader" --type tsLength of output: 4163
packages/test/jest.config.ts (1)
23-23: LGTM! Verify mock implementation setupThe addition of react-native mock mapping aligns well with the existing configuration structure and testing setup.
Let's verify the mock setup:
✅ Verification successful
Mock implementation setup verified ✅
The react-native mock is correctly implemented with:
- Central implementation in
packages/test/__mocks__/react-native.ts- Proper re-exports from other packages referencing the test package mock
- Consistent mock architecture across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify react-native mock implementation and its usage # Check if the mock file exists echo "Checking for mock file..." fd -t f "react-native.ts" -p "__mocks__" # Check for consistent mock usage across packages echo "Checking mock usage across packages..." rg -l "from ['|\"]@baseapp-frontend/test/__mocks__/react-native" packages/*/ # Check for any direct react-native imports in test files that might need mocking echo "Checking test files for react-native imports..." rg "from ['|\"]react-native['|\"]" "**/*.(spec|test).(ts|tsx)"Length of output: 808
Script:
#!/bin/bash # Search for mock file in the entire repository echo "Searching for react-native mock file..." fd -t f "react-native.ts" # Check other jest configs for react-native mocks echo "Checking jest configs..." rg "react-native.*mocks?" -t ts "jest.config" # Search for react-native imports in test files with correct pattern echo "Checking test files for react-native imports..." rg "from ['\"]react-native['\"]" -t ts -t tsx -g "*{test,spec}*" # List all __mocks__ directories echo "Listing mock directories..." fd -t d "__mocks__"Length of output: 1154
Script:
#!/bin/bash # Compare contents of react-native mock files echo "Comparing mock file contents..." for file in packages/{authentication,components,test,utils}/__mocks__/react-native.ts; do echo "=== $file ===" cat "$file" echo done # Check if other packages import from test package mock echo "Checking imports from test package mock..." rg "from ['\"]@baseapp-frontend/test/__mocks__/react-native" -t ts -t tsxLength of output: 1700
packages/authentication/modules/access/useLogin/index.ts (1)
100-100: Good practice: Awaiting asynchronous operationsAwaiting
handleLoginSuccess(response)in theonSuccesscallback ensures that all asynchronous token settings complete before proceeding. This change improves the reliability of the authentication flow by preventing potential race conditions.packages/utils/functions/os/index.ts (1)
1-3: Addition ofisMobilePlatformutilityIntroducing the
isMobilePlatformfunction centralizes platform detection logic, enhancing code maintainability and readability across the codebase.packages/utils/functions/token/removeTokenAsync/index.ts (1)
4-8: Improved platform detection usingisMobilePlatformReplacing environment variable checks with
isMobilePlatform()enhances code clarity and ensures consistent platform detection across different modules.packages/utils/functions/token/getToken/index.ts (1)
6-6: LGTM! Platform detection refactor improves maintainability.The change from environment variable to
isMobilePlatform()centralizes platform detection logic, making the code more maintainable.Let's verify that all platform-specific token handling is consistent across the codebase:
Also applies to: 12-12
✅ Verification successful
Platform detection and token handling verified as consistent
All token-related functions properly use the new
isMobilePlatform()check and handle tokens appropriately:
- Mobile: Using expo-secure-store
- Web: Using cookies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct EXPO_PUBLIC_PLATFORM checks rg "process\.env\.EXPO_PUBLIC_PLATFORM" --type ts # Verify consistent usage of isMobilePlatform rg "isMobilePlatform\(\)" --type ts -C 2Length of output: 3614
packages/utils/functions/token/getToken/__tests__/getToken.server.test.ts (1)
9-9: LGTM! Test setup properly mocks platform detection.The test correctly mocks
isMobilePlatformto simulate a server environment, maintaining proper test isolation.Also applies to: 21-23, 30-30
packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.server.test.ts (1)
9-9: LGTM! Consistent with other token-related test updates.The test setup properly mocks platform detection and maintains consistency with other token-related test implementations.
Also applies to: 21-23, 30-30
packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts (1)
19-21: LGTM! Clean mock setup for platform detection.The mock setup for
isMobilePlatformfollows Jest best practices and improves code maintainability by centralizing platform detection logic.packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.client.test.ts (1)
19-21: LGTM! Consistent implementation with synchronous version.The async implementation maintains parity with the synchronous version while properly handling promises and async operations. The platform detection logic is consistently applied across both versions.
Also applies to: 31-31, 42-42, 53-53
packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts (1)
Line range hint
74-83: LGTM! Comprehensive configuration testing.The test cases thoroughly cover the configuration options for non-mobile platforms.
packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts (4)
4-19: Well-structured mock declarations!The mock declarations follow Jest best practices by using
jest.fn()and maintaining clear isolation between different token-related functions.
26-30: Clean mock assignments with clear naming!The mock assignments follow good practices by using descriptive variable names and properly referencing the mocked modules.
55-64: Thorough error handling and cleanup verification!The test properly verifies both the error propagation and the cleanup actions when token refresh fails.
38-49: Verify security implications ofsecure: falseWhile the test logic is sound, setting
secure: falseinmockSetTokenAsynccould have security implications in production. Please verify if this is intentional and if it aligns with the security requirements for different environments.✅ Verification successful
secure: falsein test is correct and safeThe test correctly reflects non-production behavior. The actual implementation in
refreshAccessTokenautomatically setssecure: truein production viasecure: process.env.NODE_ENV === 'production', ensuring proper security in production environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of secure cookie settings rg -A 2 "secure:\s*(true|false)" --type tsLength of output: 720
Script:
#!/bin/bash # Find implementations of token-related functions echo "=== Looking for setTokenAsync implementation ===" ast-grep --pattern 'export const setTokenAsync = $_' echo -e "\n=== Looking for refreshAccessToken implementation ===" ast-grep --pattern 'export const refreshAccessToken = $_' echo -e "\n=== Looking for cookie/token configurations ===" rg -l "cookie|token.*config" --type tsLength of output: 5304
packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (2)
20-24: Good conversion to jest.fn() for better test control!The change from direct return values to
jest.fn()improves test flexibility and allows for better verification of mock calls.
25-26: Well-structured refreshAccessToken mock!The new
refreshAccessTokenmock follows the established pattern and properly handles the async nature of the operation.
| import type { CookiesGetByNameFn } from '@baseapp-frontend/test' | ||
|
|
||
| import Cookies from 'js-cookie' | ||
| import { getTokenAsync } from '@baseapp-frontend/utils/functions/token/getTokenAsync' |
There was a problem hiding this comment.
💡 Codebase verification
Transition to token-based authentication is incomplete
The codebase shows mixed authentication methods:
- Token-based infrastructure is fully implemented
- Several core components still use cookies for authentication:
packages/utils/functions/token/setTokenAsync/index.tspackages/utils/functions/axios/createAxiosInstance/index.tspackages/authentication/modules/user/useUser/index.ts
Note: Cookie usage for language preferences is a separate concern and can be retained.
🔗 Analysis chain
LGTM! Clean transition from cookies to tokens.
The import changes and mock setup correctly reflect the architectural shift from cookie-based to token-based authentication.
Let's verify that this transition is consistent across the codebase:
Also applies to: 6-8
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining cookie-based authentication
# that might have been missed during the transition
# Search for remaining js-cookie imports
echo "Checking for remaining js-cookie imports..."
rg "from 'js-cookie'" -l
# Search for cookie-related function calls
echo "Checking for remaining cookie-related function calls..."
rg "Cookies\.(get|set|remove)" -l
# Search for cookie-related types/interfaces
echo "Checking for cookie-related types..."
rg "CookiesGetByNameFn" -l
Length of output: 2214
Script:
#!/bin/bash
# Check the context of cookie usage in core functionality files
echo "Checking cookie usage in core functionality files..."
echo -e "\nsetTokenAsync implementation:"
rg -A 5 "from 'js-cookie'" packages/utils/functions/token/setTokenAsync/index.ts
echo -e "\naxios instance creation:"
rg -A 5 "from 'js-cookie'" packages/utils/functions/axios/createAxiosInstance/index.ts
echo -e "\nCookie usage in authentication modules:"
rg -A 5 "Cookies\.(get|set|remove)" packages/authentication/modules/user/useUser/index.ts
echo -e "\nLanguage-related cookie usage:"
rg -A 5 "Cookies\.(get|set|remove)" packages/utils/functions/language/getLanguage/__tests__/getLanguage.client.test.ts
Length of output: 1721
Script:
#!/bin/bash
# Check for token-based implementations
echo "Checking token-based implementations..."
echo -e "\nToken storage/retrieval implementations:"
rg -A 5 "getTokenAsync|setTokenAsync" packages/utils/functions/token/ -l
echo -e "\nToken usage in authentication:"
rg -A 5 "getTokenAsync|setTokenAsync" packages/authentication/modules/user/ -l
echo -e "\nCheck if there are parallel implementations:"
ast-grep --pattern 'function $_($$TOKEN$$) {
$$$
Cookies.$_($$$)
$$$
}'
Length of output: 1532
anicioalexandre
force-pushed
the
BA-2020-mobile-internal-testing
branch
from
6bfce08 to
568d2f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (3)
packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx (1)
Line range hint
107-139: Fix potential security issue in custom validation.The custom validation test allows mismatched passwords to pass, which could be a security concern if this validation schema is used in production.
The test should either:
- Use matching passwords in the test data, or
- Include password matching validation in the custom schema
const customValidationSchema = z.object({ newPassword: z.string().min(1), - confirmNewPassword: z.string().min(1), + confirmNewPassword: z.string().min(1).refine( + (data) => data === customDefaultValues.newPassword, + 'Passwords must match' + ), })packages/utils/package.json (1)
Line range hint
1-56: Sync package.json with pnpm-lock.yamlThe pipeline is failing because pnpm-lock.yaml is out of sync with package.json.
Run
pnpm installto update the lockfile with the new dependency specifications.🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] pnpm-lock.yaml is out of sync with package.json. The package specifications in the lockfile do not match the specifications in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
packages/authentication/modules/access/useSignUp/__tests__/useSignUp.test.tsx (1)
Line range hint
1-12: Consider abstracting the common Axios mock setup.The axiosMock setup with @ts-ignore is repeated across multiple test files. Consider creating a shared test utility.
Example implementation:
// test/utils/mockAxios.ts import { MockAdapter } from '@baseapp-frontend/test' import { axios } from '@baseapp-frontend/utils' export const createAxiosMock = () => new MockAdapter(axios)
♻️ Duplicate comments (1)
packages/utils/package.json (1)
17-30:⚠️ Potential issueReview dependencies version updates
The dependency updates look good overall, but there are a few concerns:
- The next.js canary version (14.3.0-canary.24) should be replaced with a stable version
- react-native should be moved to peer dependencies since this is a cross-platform utility package
Apply these changes:
- "next": "14.3.0-canary.24", + "next": "14.0.4", - "react-native": "0.76.3",And add to peerDependencies:
"peerDependencies": { "react": "18.3.1", + "react-native": "0.76.3" },🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] pnpm-lock.yaml is out of sync with package.json. The package specifications in the lockfile do not match the specifications in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
🧹 Nitpick comments (18)
packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts (2)
28-28: LGTM! Comprehensive test coverage with clear platform-specific behavior.The test cases effectively cover:
- Platform-specific storage mechanisms
- Error handling for both platforms
- Configuration options for cookies
Consider adding these test cases to improve coverage:
it('should handle empty token values', async () => { (isMobilePlatform as jest.Mock).mockReturnValue(false) await setTokenAsync(mockKey, '') expect(setCookie).toHaveBeenCalledWith(mockKey, '', undefined) }) it('should validate cookie configuration', async () => { (isMobilePlatform as jest.Mock).mockReturnValue(false) const invalidConfig = { invalid: true } await setTokenAsync(mockKey, mockValue, invalidConfig) // Verify how invalid config is handled })Also applies to: 39-39, 50-50, 62-62, 74-74
Line range hint
32-35: Enhance assertion messages for better test failure debugging.The assertions are well-structured, but their error messages could be more descriptive to help debug test failures.
Consider updating assertions with more specific messages:
- expect(setItemAsync).toHaveBeenCalledWith(mockKey, mockValue) + expect(setItemAsync).toHaveBeenCalledWith( + mockKey, + mockValue, + 'Should store token securely on mobile platform' + ) - expect(setCookie).not.toHaveBeenCalled() + expect(setCookie).not.toHaveBeenCalled( + 'Should not use cookies when on mobile platform' + )Also applies to: 43-46, 57-59, 68-70, 79-82
packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx (2)
1-10: Improve test setup with lifecycle hooks.Consider moving the mock setup and cleanup into test lifecycle hooks for better test isolation and cleanup.
+ beforeEach(() => { + axiosMock.reset() + }) + + afterAll(() => { + axiosMock.restore() + })🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
Line range hint
15-46: Enhance test description and error handling.The success test case could be more specific about what it's testing. Also, consider validating the request payload and headers.
- it('should run onSuccess', async () => { + it('should call onSuccess when password reset request succeeds', async () => { axiosMock.onPost('/forgot-password/reset').reply(200, { newPassword: password, token, }) + let requestPayload: any + axiosMock.onPost('/forgot-password/reset').reply((config) => { + requestPayload = JSON.parse(config.data) + expect(config.headers['Content-Type']).toBe('application/json') + return [200, { newPassword: password, token }] + })🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
pnpm-workspace.yaml (1)
130-132: Add newline at end of fileThe file is missing a newline character at the end.
Add a newline at the end of the file:
react-native-core: expo-secure-store: ~14.0.0 - react-native: 0.76.3 + react-native: 0.76.3 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
packages/authentication/services/user.ts (2)
6-7: Add error handling for getUser methodWhile the transition to axios is good, consider adding error handling for failed requests and type validation for the response.
static getUser<TUser extends Partial<User>>(): Promise<TUser> { - return axios.get(`/users/me`) + return axios.get<TUser>(`/users/me`) + .catch(error => { + if (error.response?.status === 401) { + throw new Error('Unauthorized access') + } + throw error + }) }
10-14: Add response type and error handling for updateUser methodSimilar to getUser, add proper type annotation for axios and handle potential errors.
static updateUser<TUser extends Partial<User>>({ userId, data, }: UserUpdateParams<TUser>): Promise<TUser> { - return axios.patch(`/users/${userId}`, data) + return axios.patch<TUser>(`/users/${userId}`, data) + .catch(error => { + if (error.response?.status === 404) { + throw new Error('User not found') + } + throw error + }) }packages/authentication/modules/user/useUpdateUser/__tests__/useUpdateUser.test.tsx (1)
Line range hint
35-43: Move test fixtures to separate filesThe JWT token and other test data should be moved to separate fixture files for better maintainability.
Create a new file
__fixtures__/tokens.ts:export const mockJWTToken = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...'Then import and use it in the test:
- const token = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...' + const { mockJWTToken } = require('./__fixtures__/tokens')packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsx (1)
Line range hint
17-92: Enhance test coverage with additional assertions.Consider adding the following test cases:
- Verify the request payload format
- Test error scenarios with different status codes
- Validate the token storage mechanism
packages/authentication/modules/user/useJWTUser/__tests__/useJWTUser.test.tsx (1)
Line range hint
50-58: Enhance JWT token validation in tests.The test could be more thorough in validating the JWT token handling:
- Test token expiration scenarios
- Verify token refresh behavior
- Test invalid token format handling
packages/authentication/modules/access/useChangeExpiredPassword/__tests__/useChangeExpiredPassword.test.tsx (1)
Line range hint
113-147: Enhance password validation test cases.The custom validation test could be more comprehensive:
- Test password complexity requirements
- Verify minimum/maximum length constraints
- Test password history validation
Example test case:
test('should validate password complexity', async () => { const complexValidationSchema = z.object({ currentPassword: z.string().min(1), newPassword: z.string() .min(8) .regex(/[A-Z]/, 'Must contain uppercase') .regex(/[a-z]/, 'Must contain lowercase') .regex(/[0-9]/, 'Must contain number'), confirmNewPassword: z.string() }) // ... test implementation })packages/utils/functions/axios/createAxiosInstance/index.ts (3)
26-31: Consider documenting configuration options.The new configuration options provide good flexibility but would benefit from documentation explaining their purpose and impact.
74-99: Consider simplifying request data handling logic.The request data transformation logic is complex and nested. Consider extracting it into separate functions for better maintainability.
Example refactor:
- if (request.data) { - if (!file || !useFormData) { - if (stringifyBody) { - if (decamelizeRequestBodyKeys) { - request.data = JSON.stringify(humps.decamelizeKeys(request.data)) - } else { - request.data = JSON.stringify(request.data) - } - } else if (decamelizeRequestBodyKeys) { - request.data = humps.decamelizeKeys(request.data) - } - } else if (file && useFormData) { - const formData = new FormData() - Object.entries(request.data).forEach(([key, value]) => { - const decamelizedKey = humps.decamelize(key) - if (!value) return - if (value instanceof File) { - formData.append(decamelizedKey, value) - } else if (typeof value === 'object') { - formData.append(decamelizedKey, JSON.stringify(value)) - } else { - formData.append(decamelizedKey, value.toString()) - } - }) - request.data = formData - } - } + if (!request.data) return request; + + const transformData = () => { + if (!file || !useFormData) { + return transformJsonData(request.data); + } + if (file && useFormData) { + return transformFormData(request.data); + } + return request.data; + }; + + request.data = transformData();
111-131: Consider extracting error handling logic.The error handling logic in the response interceptor could be simplified by extracting it into a separate function.
Example refactor:
- if (isJsonError && error.response?.data) { - const newError = { response: { data: {} } } - newError.response.data = camelizeResponseDataKeys - ? humps.camelizeKeys(error.response.data) - : error.response.data - - return Promise.reject(newError) - } + if (isJsonError && error.response?.data) { + return Promise.reject(transformError(error)); + } +const transformError = (error) => ({ + response: { + data: camelizeResponseDataKeys + ? humps.camelizeKeys(error.response.data) + : error.response.data + } +});packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (4)
5-9: Consider strengthening the humps mock implementationThe current mock implementation for
humpsuses simple string replacement which might not handle all edge cases. Consider using the actual humps implementation in tests or adding more comprehensive test cases to verify the behavior.jest.mock('humps', () => { const actual = jest.requireActual('humps'); return { decamelize: jest.fn().mockImplementation(actual.decamelize), decamelizeKeys: jest.fn().mockImplementation(actual.decamelizeKeys), camelizeKeys: jest.fn().mockImplementation(actual.camelizeKeys), }; });
137-168: Add type safety to FormData mockThe current FormData mock implementation uses
@ts-ignoreand lacks proper typing. Consider adding proper TypeScript types to improve type safety.class MockFormData implements FormData { private _store: Record<string, any> = {}; append(name: string, value: string | Blob, fileName?: string): void { this._store[name] = value; } has(name: string): boolean { return Object.prototype.hasOwnProperty.call(this._store, name); } // Implement other required FormData methods delete(name: string): void {} get(name: string): FormDataEntryValue | null { return null; } getAll(name: string): FormDataEntryValue[] { return []; } set(name: string, value: string | Blob, fileName?: string): void {} forEach(callbackfn: (value: FormDataEntryValue, key: string, parent: FormData) => void): void {} entries(): IterableIterator<[string, FormDataEntryValue]> { return [][Symbol.iterator](); } keys(): IterableIterator<string> { return [][Symbol.iterator](); } values(): IterableIterator<FormDataEntryValue> { return [][Symbol.iterator](); } [Symbol.iterator](): IterableIterator<[string, FormDataEntryValue]> { return [][Symbol.iterator](); } }
Line range hint
98-121: Consider adding negative test cases for servicesWithoutTokenWhile the current tests verify that Authorization headers are not added for matching URLs, consider adding tests for:
- URLs that partially match but shouldn't be excluded
- Edge cases with special characters in URLs
// Example additional test cases const request = { headers: { Authorization: undefined }, url: '/someUrlExtra' // Should still add Authorization }; await interceptorFn(request); expect(request.headers.Authorization).toBe('Bearer someAuthToken'); const requestWithSpecialChars = { headers: { Authorization: undefined }, url: '/someUrl?param=value' // Should be excluded }; await interceptorFn(requestWithSpecialChars); expect(requestWithSpecialChars.headers.Authorization).toBeUndefined();
170-188: Consolidate similar test cases using test.eachThe three test cases for request body transformation (stringify, decamelize) follow a similar pattern. Consider using
test.eachto make the tests more maintainable and reduce duplication.it.each([ ['default behavior', {}, true, true], ['with stringifyBody false', { stringifyBody: false }, false, true], ['with decamelizeRequestBodyKeys false', { decamelizeRequestBodyKeys: false }, true, false], ])('should handle request body transformation - %s', async (_, config, shouldStringify, shouldDecamelize) => { const { axios: { interceptors: { request: { use } } } } = createAxiosInstance(config); const [[interceptorFn]] = (use as jest.Mock).mock.calls; const requestBody = { testKey: 'testValue' }; const request = { data: requestBody, method: 'POST', headers: {} }; await interceptorFn(request); if (shouldDecamelize) { expect(humps.decamelizeKeys).toHaveBeenCalledWith(requestBody); } else { expect(humps.decamelizeKeys).not.toHaveBeenCalled(); } expect(request.data).toEqual(shouldStringify ? JSON.stringify(requestBody) : requestBody); });Also applies to: 190-208, 210-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
packages/authentication/__mocks__/react-native.ts(1 hunks)packages/authentication/modules/access/useChangeExpiredPassword/__tests__/useChangeExpiredPassword.test.tsx(4 hunks)packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsx(3 hunks)packages/authentication/modules/access/useLogin/index.ts(3 hunks)packages/authentication/modules/access/useRecoverPassword/__tests__/useRecoverPassword.test.tsx(3 hunks)packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx(4 hunks)packages/authentication/modules/access/useSignUp/__tests__/useSignUp.test.tsx(5 hunks)packages/authentication/modules/user/getUser/__tests__/getUser.test.ts(1 hunks)packages/authentication/modules/user/getUser/index.ts(1 hunks)packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts(1 hunks)packages/authentication/modules/user/getUserAsync/index.ts(1 hunks)packages/authentication/modules/user/useJWTUser/__tests__/useJWTUser.test.tsx(4 hunks)packages/authentication/modules/user/useUpdateUser/__tests__/useUpdateUser.test.tsx(3 hunks)packages/authentication/package.json(1 hunks)packages/authentication/services/auth.ts(2 hunks)packages/authentication/services/user.ts(1 hunks)packages/components/__mocks__/react-native.ts(1 hunks)packages/graphql/utils/createTestEnvironment/index.ts(1 hunks)packages/test/__mocks__/react-native.ts(1 hunks)packages/test/jest.config.ts(1 hunks)packages/utils/__mocks__/react-native.ts(1 hunks)packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts(4 hunks)packages/utils/functions/axios/createAxiosInstance/index.ts(4 hunks)packages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.ts(4 hunks)packages/utils/functions/fetch/baseAppFetch/index.ts(2 hunks)packages/utils/functions/os/__tests__/os.test.ts(1 hunks)packages/utils/functions/os/index.ts(1 hunks)packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts(5 hunks)packages/utils/functions/token/getToken/__tests__/getToken.server.test.ts(2 hunks)packages/utils/functions/token/getToken/index.ts(1 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.client.test.ts(5 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.server.test.ts(2 hunks)packages/utils/functions/token/getTokenAsync/index.ts(1 hunks)packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts(1 hunks)packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts(6 hunks)packages/utils/functions/token/removeTokenAsync/index.ts(1 hunks)packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts(7 hunks)packages/utils/functions/token/setTokenAsync/index.ts(1 hunks)packages/utils/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/test/mocks/react-native.ts
- packages/utils/functions/os/index.ts
- packages/utils/functions/token/getTokenAsync/index.ts
- packages/authentication/mocks/react-native.ts
- packages/utils/functions/token/setTokenAsync/index.ts
- packages/utils/functions/token/removeTokenAsync/index.ts
- packages/utils/mocks/react-native.ts
- packages/test/jest.config.ts
- packages/utils/functions/token/getTokenAsync/tests/getTokenAsync.server.test.ts
- packages/authentication/modules/user/getUser/index.ts
- packages/utils/functions/token/getToken/index.ts
- packages/utils/functions/fetch/baseAppFetch/tests/baseAppFetch.test.ts
- packages/utils/functions/token/getTokenAsync/tests/getTokenAsync.client.test.ts
- packages/utils/functions/fetch/baseAppFetch/index.ts
- packages/authentication/modules/user/getUserAsync/index.ts
- packages/graphql/utils/createTestEnvironment/index.ts
- packages/utils/functions/os/tests/os.test.ts
- packages/authentication/modules/user/getUser/tests/getUser.test.ts
- packages/utils/functions/token/getToken/tests/getToken.server.test.ts
- packages/authentication/modules/access/useLogin/index.ts
- packages/components/mocks/react-native.ts
- packages/authentication/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/authentication/modules/access/useChangeExpiredPassword/__tests__/useChangeExpiredPassword.test.tsx
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/access/useSignUp/__tests__/useSignUp.test.tsx
[error] 11-12: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/user/useJWTUser/__tests__/useJWTUser.test.tsx
[error] 17-18: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/access/useRecoverPassword/__tests__/useRecoverPassword.test.tsx
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/user/useUpdateUser/__tests__/useUpdateUser.test.tsx
[error] 17-18: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsx
[error] 14-15: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🪛 GitHub Actions: Main Workflow
packages/utils/package.json
[error] pnpm-lock.yaml is out of sync with package.json. The package specifications in the lockfile do not match the specifications in package.json. Run 'pnpm install' without the --frozen-lockfile flag to update the lockfile.
🪛 yamllint (1.35.1)
pnpm-workspace.yaml
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (14)
packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts (1)
5-5: LGTM! Clean mock setup for platform detection.The import and mock setup for
isMobilePlatformfollow Jest best practices and are properly typed with TypeScript.Also applies to: 15-17
pnpm-workspace.yaml (1)
130-132: Verify version constraints across workspaceThe react-native-core catalog versions should be consistent with the package dependencies:
- expo-secure-store uses
~14.0.0(allows patch updates)- react-native uses exact version
0.76.3Run this script to check version consistency across the workspace:
✅ Verification successful
Version constraints are consistent across workspace ✅
The dependencies in
packages/utils/package.jsonperfectly match the versions specified in the workspace configuration:
expo-secure-store: ~14.0.0react-native: 0.76.3🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check version consistency for react-native dependencies echo "=== Searching for react-native dependencies ===" rg '"react-native":|"expo-secure-store":' --type json echo -e "\n=== Checking pnpm workspace config ===" cat pnpm-workspace.yaml | grep -A 2 'react-native-core:'Length of output: 509
🧰 Tools
🪛 yamllint (1.35.1)
[error] 132-132: no new line character at the end of file
(new-line-at-end-of-file)
packages/authentication/modules/access/useRecoverPassword/__tests__/useRecoverPassword.test.tsx (1)
8-9: Investigate and fix TypeScript error instead of suppressing itThe TODO comment indicates a known issue with AxiosRequestHeaders that should be investigated and fixed properly.
Let's check if this is a common issue:
✅ Verification successful
Keep @ts-ignore until BA-1081 is resolved
This TypeScript error is a known issue being tracked by BA-1081 and affects all authentication test files consistently. Since this is a test-only typing issue that doesn't affect runtime behavior, it's acceptable to keep the ts-ignore comment until the tracked issue is resolved.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar TypeScript ignores related to AxiosRequestHeaders rg -l "ts-ignore.*AxiosRequestHeaders" .Length of output: 668
Script:
#!/bin/bash # Check package.json for axios and related dependencies cat packages/authentication/package.json | jq '.dependencies,.devDependencies' 2>/dev/null || echo "No package.json found" # Check the mock adapter implementation pattern rg -A 2 "new MockAdapter" packages/authentication/Length of output: 3445
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/user/useJWTUser/__tests__/useJWTUser.test.tsx (1)
17-18: Track the TODO comment in the issue tracker.The TODO comment about investigating the AxiosRequestHeaders error is repeated across files.
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-18: Do not export from a test file.
(lint/suspicious/noExportsInTest)
packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts (2)
1-8: LGTM! Clean transition from cookies to tokens.The import changes and mock setup correctly reflect the architectural shift from cookie-based to token-based authentication.
11-13: LGTM! Test cases properly validate token-based authentication.The test cases effectively verify both successful and unsuccessful token retrieval scenarios.
Also applies to: 21-22
packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts (2)
6-6: LGTM! Platform detection properly mocked.The introduction of the
isMobilePlatformmock improves the test structure by centralizing platform detection logic.Also applies to: 19-21
31-31: LGTM! Comprehensive platform-specific test coverage.Test cases effectively verify token retrieval behavior for both mobile and non-mobile platforms.
Also applies to: 42-42, 53-53
packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts (2)
5-5: LGTM! Platform detection properly mocked.The introduction of the
isMobilePlatformmock improves the test structure.Also applies to: 15-17
27-27: LGTM! Robust error handling and platform-specific tests.Test cases effectively verify:
- Platform-specific token removal
- Error handling for both platforms
- Graceful failure scenarios
Also applies to: 37-37, 47-47, 59-59
packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts (2)
4-19: LGTM! Well-organized mock setup.The mock setup is clean and properly organized, making the test file more maintainable.
Also applies to: 26-29
38-49: LGTM! Comprehensive test coverage for token refresh scenarios.Test cases effectively verify:
- Successful token refresh
- Error handling during refresh
- Empty token scenarios
Also applies to: 54-64, 68-75
packages/utils/functions/axios/createAxiosInstance/index.ts (1)
40-45: LGTM! Improved content type handling.The content type configuration is now properly conditional and supports both JSON and form data.
packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (1)
269-271: Address TODO comments for response interceptor testsThe missing test cases for response interceptor are critical for ensuring complete test coverage. Consider implementing these tests to verify response data transformation behavior.
Would you like me to help implement the missing test cases for:
- Response data decamelization by default
- Conditional response data decamelization based on configuration?
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | ||
| export const axiosMock = new MockAdapter(axios) |
There was a problem hiding this comment.
Remove export and handle type error properly.
The axiosMock should not be exported from a test file as it's an anti-pattern. Additionally, the @ts-ignore with TODO indicates an unresolved type issue that should be addressed.
Apply this diff to fix the issues:
- // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error
- export const axiosMock = new MockAdapter(axios)
+ const axiosMock = new MockAdapter(axios)Consider investigating and fixing the type error instead of ignoring it. The error might be related to incorrect type definitions or version mismatches.
📝 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.
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | |
| export const axiosMock = new MockAdapter(axios) | |
| const axiosMock = new MockAdapter(axios) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
| static recoverPassword({ email }: ForgotPasswordRequest): Promise<void> { | ||
| return baseAppFetch(`/forgot-password`, { method: 'POST', body: { email } }) | ||
| return axios.post(`/forgot-password`, { email }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement consistent error handling across authentication methods
All authentication methods should follow a consistent error handling pattern and provide meaningful error messages.
Consider implementing a centralized error handling utility for authentication errors:
// authErrors.ts
export const handleAuthError = (error: any, context: string) => {
if (error.response?.status === 401) {
throw new Error(`${context}: Unauthorized`)
}
if (error.response?.status === 404) {
throw new Error(`${context}: Not found`)
}
throw new Error(`${context}: Operation failed`)
}Then use it consistently across all methods:
static recoverPassword({ email }: ForgotPasswordRequest): Promise<void> {
- return axios.post(`/forgot-password`, { email })
+ return axios.post(`/forgot-password`, { email })
+ .catch(error => handleAuthError(error, 'Password recovery'))
}Also applies to: 21-22, 25-26, 29-34
| static login({ email, password }: LoginRequest): Promise<LoginResponse> { | ||
| return baseAppFetch(`/auth/login`, { method: 'POST', body: { email, password } }) | ||
| return axios.post(`/auth/login`, { email, password }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security and error handling for login method
The login method handles sensitive credentials and should implement proper error handling and security measures.
static login({ email, password }: LoginRequest): Promise<LoginResponse> {
- return axios.post(`/auth/login`, { email, password })
+ return axios.post<LoginResponse>(`/auth/login`, { email, password })
+ .catch(error => {
+ if (error.response?.status === 401) {
+ throw new Error('Invalid credentials')
+ }
+ // Avoid leaking specific error details
+ throw new Error('Authentication failed')
+ })
}📝 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.
| static login({ email, password }: LoginRequest): Promise<LoginResponse> { | |
| return baseAppFetch(`/auth/login`, { method: 'POST', body: { email, password } }) | |
| return axios.post(`/auth/login`, { email, password }) | |
| static login({ email, password }: LoginRequest): Promise<LoginResponse> { | |
| return axios.post<LoginResponse>(`/auth/login`, { email, password }) | |
| .catch(error => { | |
| if (error.response?.status === 401) { | |
| throw new Error('Invalid credentials') | |
| } | |
| // Avoid leaking specific error details | |
| throw new Error('Authentication failed') | |
| }) |
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | ||
| export const axiosMock = new MockAdapter(axios) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove export from test file
The axiosMock should not be exported from a test file as it violates testing best practices.
-export const axiosMock = new MockAdapter(axios)
+const axiosMock = new MockAdapter(axios)📝 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.
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | |
| export const axiosMock = new MockAdapter(axios) | |
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | |
| const axiosMock = new MockAdapter(axios) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Do not export from a test file.
(lint/suspicious/noExportsInTest)
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | ||
| export const axiosMock = new MockAdapter(axios) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove export and fix TypeScript error
Similar to the previous test file, the axiosMock should not be exported and the TypeScript error should be properly fixed.
-// @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error
-export const axiosMock = new MockAdapter(axios)
+const axiosMock = new MockAdapter(axios)📝 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.
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | |
| export const axiosMock = new MockAdapter(axios) | |
| const axiosMock = new MockAdapter(axios) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-18: Do not export from a test file.
(lint/suspicious/noExportsInTest)
| // @ts-ignore TODO: (BA-1081) investigate AxiosRequestHeaders error | ||
| export const axiosMock = new MockAdapter(axios) |
There was a problem hiding this comment.
💡 Codebase verification
Create an issue to track the AxiosRequestHeaders type definition problem
This TODO appears in 7 different test files within the authentication module, indicating a systemic issue that needs attention. Consider creating a shared mock adapter utility to resolve this across all affected files.
- Found in:
- useLogin.test.tsx
- useSignUp.test.tsx
- useResetPassword.test.tsx
- useRecoverPassword.test.tsx
- useChangeExpiredPassword.test.tsx
- useJWTUser.test.tsx
- useUpdateUser.test.tsx
🔗 Analysis chain
Track the TODO comment in the issue tracker.
The TODO comment about investigating the AxiosRequestHeaders error should be tracked in your issue management system.
Would you like me to create a GitHub issue to track this TODO?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this TODO is already tracked
gh issue list --search "BA-1081 in:title,body" --json number,title,body
Length of output: 76
Script:
#!/bin/bash
# Search for similar @ts-ignore patterns with AxiosRequestHeaders
rg "@ts-ignore.*AxiosRequestHeaders" -B 1 -A 1
# Search for MockAdapter usage patterns
ast-grep --pattern 'new MockAdapter($_)'
Length of output: 3730
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-15: Do not export from a test file.
(lint/suspicious/noExportsInTest)
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/graphql/config/environment.ts (1)
WebSocket endpoint configuration inconsistency confirmed
The codebase shows an inconsistent approach to endpoint configuration:
- HTTP endpoint is hardcoded for both production (
https://api.baseapp.io/graphql) and development (http://localhost:8000/graphql)- WebSocket endpoint uses environment variables in production but is hardcoded in development configs
This mixed approach could lead to configuration mismatches and maintenance difficulties. Consider adopting a consistent strategy for both HTTP and WebSocket endpoints.
🔗 Analysis chain
Line range hint
91-102: Verify WebSocket endpoint configuration.While the HTTP endpoint was hardcoded, the WebSocket endpoint still uses environment variables. This inconsistency could lead to connection issues if the endpoints need to be in sync.
Let's verify the WebSocket configuration across environments:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for WebSocket endpoint configurations rg "WS_RELAY_ENDPOINT" # Check for other WebSocket-related configurations rg -A 5 "createClient|webSocketImpl"Length of output: 2430
Script:
#!/bin/bash # Check for HTTP/API endpoint configurations rg "API_ENDPOINT|GRAPHQL_ENDPOINT|HTTP_ENDPOINT" -A 2 # Search for specific endpoint URLs rg "http.*graphql|https.*graphql"Length of output: 519
🧹 Nitpick comments (4)
packages/graphql/config/environment.ts (1)
Line range hint
1-1: Consider migrating to Axios for consistency.The AI summary indicates a broader transition from
baseAppFetchto Axios across the codebase. This file still usesbaseAppFetch.Consider migrating this file to use Axios for consistency with other changes in this PR. Would you like me to provide a code example for the migration?
packages/utils/functions/axios/createAxiosInstance/index.ts (2)
41-46: Consider adding content type for DELETE method.While the content type setup is good for POST, PATCH, and PUT methods, consider adding it for DELETE as well, as DELETE requests might include a request body.
if (setContentType) { const contentType = file ? 'multipart/form-data' : 'application/json' instance.defaults.headers.post['Content-Type'] = contentType instance.defaults.headers.patch['Content-Type'] = contentType instance.defaults.headers.put['Content-Type'] = contentType + instance.defaults.headers.delete['Content-Type'] = contentType }
122-134: Simplify error handling logic.The error handling code could be simplified and made more maintainable:
- The content type check is duplicated
- The error object creation could be more concise
(error) => { const contentTypeHeader = error.response?.headers?.['content-type'] || '' const isJsonError = contentTypeHeader.includes('application/json') - if (isJsonError && error.response?.data) { - const newError = { response: { data: {} } } - newError.response.data = camelizeResponseDataKeys - ? humps.camelizeKeys(error.response.data) - : error.response.data - - return Promise.reject(newError) - } + if (isJsonError && error.response?.data) { + return Promise.reject({ + response: { + data: camelizeResponseDataKeys + ? humps.camelizeKeys(error.response.data) + : error.response.data + } + }) + } return Promise.reject(error) }packages/utils/functions/test/index.ts (1)
1-1: Consider importing type definitions for better type safety.Since this is a TypeScript file, consider importing the type definition for
Alertto ensure type safety:-import { Alert } from 'react-native' +import { Alert, AlertButton, AlertOptions } from 'react-native'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
packages/graphql/config/environment.ts(1 hunks)packages/graphql/package.json(1 hunks)packages/utils/functions/axios/createAxiosInstance/index.ts(4 hunks)packages/utils/functions/test/index.ts(1 hunks)
🔇 Additional comments (4)
packages/utils/functions/axios/createAxiosInstance/index.ts (2)
27-32: Well-structured configuration with cross-platform support.The new configuration parameters are well-organized with appropriate defaults. The baseUrl fallback chain properly handles both Next.js and Expo environments.
Line range hint
141-147: Well-structured exports for common use cases.The pre-configured axios instances provide convenient defaults for both regular requests and file uploads.
packages/graphql/package.json (2)
16-18: Verify GraphQL dependencies compatibility.The core GraphQL dependencies have been updated to specific versions:
These versions need verification for:
- Compatibility with mobile platforms
- Known security vulnerabilities
✅ Verification successful
GraphQL dependencies are appropriately versioned and secure ✅
The selected versions are stable releases with no known vulnerabilities:
- [email protected] includes the patch for the moderate severity vulnerability
- [email protected] and [email protected] are production-ready versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any security advisories for the GraphQL packages gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "graphql") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check if these versions are the latest stable releases for pkg in "graphql" "graphql-ws" "isomorphic-ws"; do echo "Checking $pkg..." npm view $pkg versions --json | jq -r '.[-1]' doneLength of output: 1141
19-22: Verify React-Relay ecosystem compatibility.The React-Relay related dependencies have been updated:
Note that relay-test-utils is on version 17 while other Relay packages are on 16. This version mismatch needs investigation.
✅ Verification successful
React-Relay ecosystem compatibility verified
The version difference between relay-test-utils (17.0.0) and other Relay packages (16.2.0) is safe. The test utility is used exclusively for testing purposes, primarily for types and mock environments, and has no runtime dependencies on the other Relay packages. This is a common pattern where test utilities can support multiple versions of their main packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these versions are compatible with each other echo "Checking peer dependencies..." for pkg in "react-relay" "relay-runtime" "relay-test-utils"; do npm view $pkg@latest peerDependencies --json doneLength of output: 462
Script:
#!/bin/bash # Check peer dependencies for specific versions echo "Checking peer dependencies for specific versions..." for pkg in "[email protected]" "[email protected]" "[email protected]"; do echo "=== $pkg ===" npm view $pkg peerDependencies --json done # Search for any relay configuration or setup files echo -e "\nSearching for relay configuration files..." fd -e js -e ts -e json relay.*configLength of output: 982
Script:
#!/bin/bash echo "=== Content of relay config files ===" for file in packages/components/relay.config.js packages/config/relay.config.ts; do echo -e "\n--- $file ---" cat "$file" done # Also check for any relay-related imports in test files echo -e "\n=== Checking relay-test-utils usage ===" rg "from ['\"](relay-test-utils|@relay-tools)" -A 2Length of output: 3029
| @@ -65,8 +65,7 @@ export async function httpFetch( | |||
| ): Promise<GraphQLResponse> { | |||
| const fetchOptions = getFetchOptions({ request, variables, uploadables }) | |||
| const response = await baseAppFetch('', { | |||
| baseUrl: (process.env.NEXT_PUBLIC_RELAY_ENDPOINT || | |||
| process.env.EXPO_PUBLIC_RELAY_ENDPOINT) as string, | |||
| baseUrl: 'https://api.baseapp.io/graphql', | |||
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded GraphQL endpoint breaks development and testing configurations
The hardcoded URL conflicts with:
- Local development setup using
localhost:8000/graphql - Schema generation scripts in multiple packages
- Cypress test configurations
Revert to using environment variables as suggested in the review comment to maintain compatibility with development, testing, and cross-platform environments.
🔗 Analysis chain
Avoid hardcoding the GraphQL endpoint URL.
Replacing environment variables with a hardcoded URL introduces several concerns:
- Different environments (dev/staging/prod) can't use different endpoints
- Mobile and web platforms might need different endpoints
- URL changes would require code changes and redeployment
Revert to using environment variables to maintain flexibility:
- baseUrl: 'https://api.baseapp.io/graphql',
+ baseUrl: (process.env.NEXT_PUBLIC_RELAY_ENDPOINT ||
+ process.env.EXPO_PUBLIC_RELAY_ENDPOINT) as string,Let's verify the impact of this change on the mobile testing environment:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the hardcoded URL is used elsewhere in the codebase
rg -l "api.baseapp.io/graphql"
# Check for other environment variable usage patterns
rg "NEXT_PUBLIC_RELAY_ENDPOINT|EXPO_PUBLIC_RELAY_ENDPOINT"
Length of output: 839
| @@ -1,14 +1,17 @@ | |||
| import _axios from 'axios' | |||
| import humps from 'humps' | |||
| import Cookies from 'js-cookie' | |||
| import { Alert } from 'react-native' | |||
There was a problem hiding this comment.
Remove debug alerts and avoid platform-specific imports.
The import of Alert from 'react-native' and subsequent debug alerts create several issues:
- Platform compatibility: Using react-native's
Alertin a shared utility breaks web platform compatibility - Debug code in production: Debug alerts should not be committed to production code
Remove the debug code and platform-specific import:
-import { Alert } from 'react-native'
...
- Alert.alert('baseURL', baseUrl)
- Alert.alert('EXPO_PUBLIC_API_BASE_URL', process.env.EXPO_PUBLIC_API_BASE_URL)Also applies to: 49-50
| Object.entries(request.data).forEach(([key, value]) => { | ||
| const decamelizedKey = humps.decamelize(key) | ||
| if (!value) return | ||
| if (value instanceof File) { | ||
| formData.append(decamelizedKey, value) | ||
| } else if (typeof value === 'object') { | ||
| formData.append(decamelizedKey, JSON.stringify(value)) | ||
| } else { | ||
| formData.append(decamelizedKey, value.toString()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve form data handling robustness.
The form data handling could be improved in several ways:
- The null check
if (!value) returnmight skip validfalseboolean values - The
value.toString()call might fail for null/undefined values that pass the initial check
Object.entries(request.data).forEach(([key, value]) => {
const decamelizedKey = humps.decamelize(key)
- if (!value) return
+ if (value === null || value === undefined) return
if (value instanceof File) {
formData.append(decamelizedKey, value)
} else if (typeof value === 'object') {
formData.append(decamelizedKey, JSON.stringify(value))
} else {
- formData.append(decamelizedKey, value.toString())
+ formData.append(decamelizedKey, String(value))
}
})📝 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.
| Object.entries(request.data).forEach(([key, value]) => { | |
| const decamelizedKey = humps.decamelize(key) | |
| if (!value) return | |
| if (value instanceof File) { | |
| formData.append(decamelizedKey, value) | |
| } else if (typeof value === 'object') { | |
| formData.append(decamelizedKey, JSON.stringify(value)) | |
| } else { | |
| formData.append(decamelizedKey, value.toString()) | |
| } | |
| Object.entries(request.data).forEach(([key, value]) => { | |
| const decamelizedKey = humps.decamelize(key) | |
| if (value === null || value === undefined) return | |
| if (value instanceof File) { | |
| formData.append(decamelizedKey, value) | |
| } else if (typeof value === 'object') { | |
| formData.append(decamelizedKey, JSON.stringify(value)) | |
| } else { | |
| formData.append(decamelizedKey, String(value)) | |
| } |
| export const getExpoEnvironment = () => { | ||
| Alert.alert('EXPO_PUBLIC_API_BASE_URL', process.env.EXPO_PUBLIC_API_BASE_URL) | ||
| return process.env.EXPO_PUBLIC_API_BASE_URL | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider separating concerns and improving UI feedback.
The function currently mixes displaying and returning the value. Additionally, Alert.alert is synchronous and could block the UI.
Consider splitting this into two functions and using a non-blocking approach:
export const getExpoEnvironment = (): string | undefined => {
return process.env.EXPO_PUBLIC_API_BASE_URL
}
export const displayExpoEnvironment = async (): Promise<void> => {
const apiUrl = getExpoEnvironment()
// Use Toast or other non-blocking notification method
// This is just an example, implement based on your UI components
Toast.show({
type: 'info',
text: `API URL: ${apiUrl || 'Not defined'}`,
duration: 3000
})
}Add error handling and type safety for environment variables.
The current implementation has several potential issues:
- No error handling for undefined environment variables
- No type safety for the return value
- Direct access to
process.envmight fail in production builds
Consider implementing this safer approach:
-export const getExpoEnvironment = () => {
+export const getExpoEnvironment = (): string | undefined => {
+ const apiUrl = process.env.EXPO_PUBLIC_API_BASE_URL
+ if (!apiUrl) {
+ console.warn('EXPO_PUBLIC_API_BASE_URL is not defined')
+ }
Alert.alert('EXPO_PUBLIC_API_BASE_URL', process.env.EXPO_PUBLIC_API_BASE_URL)
- return process.env.EXPO_PUBLIC_API_BASE_URL
+ return apiUrl
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getExpoEnvironment = () => { | |
| Alert.alert('EXPO_PUBLIC_API_BASE_URL', process.env.EXPO_PUBLIC_API_BASE_URL) | |
| return process.env.EXPO_PUBLIC_API_BASE_URL | |
| } | |
| export const getExpoEnvironment = (): string | undefined => { | |
| const apiUrl = process.env.EXPO_PUBLIC_API_BASE_URL | |
| if (!apiUrl) { | |
| console.warn('EXPO_PUBLIC_API_BASE_URL is not defined') | |
| } | |
| Alert.alert('EXPO_PUBLIC_API_BASE_URL', process.env.EXPO_PUBLIC_API_BASE_URL) | |
| return apiUrl | |
| } |
💡 Codebase verification
Test utility needs proper isolation and documentation
Several issues need to be addressed:
- The utility should be renamed to
index.test.tsto ensure it's excluded from production builds - Remove the
exportkeyword to prevent unintended usage outside of tests - Document the utility's purpose and usage in the README
- Consider removing
Alert.alertas UI components shouldn't be used in test utilities
🔗 Analysis chain
Verify the testing context of this utility.
Since this appears to be a testing utility (based on the file path and function name), please ensure:
- This code is not included in production builds
- There's documentation about when and how to use this utility
Let's check if there are any production safeguards:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this testing utility is properly isolated from production code
# and verify its usage across the codebase
# Look for imports of this testing utility
rg "import.*getExpoEnvironment.*from.*utils/functions/test" -t ts -t tsx
# Check for any production build configuration that might need to exclude this file
fd "webpack|rollup|vite|tsconfig" -e config.js -e json
Length of output: 927
Script:
#!/bin/bash
# Search for imports with correct extension handling
rg "import.*getExpoEnvironment.*from.*utils/functions/test" -t typescript
# Check utils package tsconfig files
cat packages/utils/tsconfig.build.json packages/utils/tsconfig.json
# Look for documentation
fd "README|docs" -e md -d 2 packages/utils
# Check test directory structure and contents
fd . packages/utils/functions/test/
Length of output: 769
Script:
#!/bin/bash
# Search for imports with correct extension
rg "import.*getExpoEnvironment.*from.*utils/functions/test" -t ts
# Check README content
cat packages/utils/README.md
# Check if the test directory gets included in the build
fd . packages/utils/dist/functions/test
Length of output: 721
packages/graphql/package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "@baseapp-frontend/utils": "workspace:*", | ||
| "react": "catalog:react18" | ||
| "react": "18.3.1" |
There was a problem hiding this comment.
💡 Codebase verification
Invalid React version specified
The React version 18.3.1 appears to be invalid. React Native requires ^18.2.0, and the latest stable React 18.x version is 18.2.x. To ensure compatibility with React Native, please use a valid version within the 18.2.x range.
🔗 Analysis chain
Verify React version compatibility with mobile platforms.
The React peer dependency has been updated to 18.3.1. Since this PR is focused on mobile internal testing, we should verify this version's compatibility with React Native.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check React Native compatibility
echo "Checking React Native compatibility..."
npm view react-native@latest peerDependencies --json
# Check if this React version is the latest stable
npm view react versions --json | jq -r '.[-1]'
Length of output: 276
packages/graphql/package.json
Outdated
| "@types/js-cookie": "3.0.6", | ||
| "@types/react": "18.3.3", | ||
| "@types/react-dom": "18.3.0", | ||
| "@types/react-relay": "16.0.6", | ||
| "@types/relay-runtime": "17.0.3", | ||
| "@types/relay-test-utils": "14.1.4", | ||
| "relay-compiler": "16.2.0", | ||
| "typescript": "5.4.5" |
There was a problem hiding this comment.
💡 Codebase verification
Invalid TypeScript version and type definition mismatches detected
- TypeScript 5.4.5 is an invalid version (latest dev is 5.8.0-dev)
- React type definitions (18.3.3/18.3.0) are slightly ahead of React version (18.3.1)
- Relay type definitions have significant version mismatches:
- [email protected] vs @types/[email protected]
- @types/[email protected] vs @types/[email protected]
🔗 Analysis chain
Review TypeScript and type definitions versions.
The TypeScript and type definitions have been updated to recent versions. Some observations:
- [email protected] is a major version update
- @types/[email protected] and @types/[email protected] versions should be kept in sync
- The Relay type definitions (@types/react-relay, @types/relay-runtime, @types/relay-test-utils) have version mismatches
Consider implementing a version management strategy (e.g., using ^ for minor version updates) to make future updates more maintainable while ensuring type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes in TypeScript 5.4
echo "Checking TypeScript release notes..."
curl -s "https://raw.githubusercontent.com/Microsoft/TypeScript/main/CHANGELOG.md" | grep -A 10 "TypeScript 5.4"
# Verify type definitions compatibility
echo "Checking @types packages compatibility..."
for pkg in "@types/react" "@types/react-dom" "@types/react-relay" "@types/relay-runtime" "@types/relay-test-utils"; do
npm view $pkg dependencies --json
done
Length of output: 1297
Script:
#!/bin/bash
# Check TypeScript releases
echo "Checking latest TypeScript releases..."
npm view typescript versions --json | grep "5\." | tail -n 5
# Check React and Relay versions in package.json files
echo -e "\nChecking React and Relay versions..."
find . -name "package.json" -type f -exec sh -c '
echo "\nFile: {}"
cat {} | grep -E "\"react\"|\"react-dom\"|\"relay-runtime\"|\"relay-compiler\"" || true
' \;
Length of output: 10361
anicioalexandre
force-pushed
the
BA-2020-mobile-internal-testing
branch
from
2f3098f to
0b05224
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/graphql/package.json (2)
26-26:⚠️ Potential issueInvalid React version specified
The React version 18.3.1 appears to be invalid. React Native requires
^18.2.0, and the latest stable React 18.x version is 18.2.x.
31-38:⚠️ Potential issueVersion mismatches in dependencies
Several version mismatches need attention:
- TypeScript 5.4.5 is not a valid version (latest stable is 5.3.x)
- Relay type definitions have significant version mismatches:
- [email protected] vs @types/[email protected]
- @types/[email protected] vs @types/[email protected]
packages/utils/package.json (2)
26-26:⚠️ Potential issueRemove next.js canary version from production dependencies
Using a canary version (
14.3.0-canary.24) of next.js in production dependencies is risky as these versions are unstable.
29-29: 🛠️ Refactor suggestionMove react-native to peer dependencies
react-native should be a peer dependency since:
- It's only used for platform detection
- Allows consumers to control the react-native version
- Follows best practices for cross-platform packages
🧹 Nitpick comments (11)
packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts (1)
Line range hint
45-67: Consider adding test cases for edge scenarios.While the error handling is well tested, consider adding these test cases for better coverage:
- Empty/null key parameter handling
- Undefined platform value handling
- Multiple consecutive calls behavior
Example test case:
it('should handle empty/null key parameter', async () => { (isMobilePlatform as jest.Mock).mockReturnValue(true); await expect(removeTokenAsync('')).resolves.not.toThrow(); await expect(removeTokenAsync(null)).resolves.not.toThrow(); expect(deleteItemAsync).not.toHaveBeenCalled(); expect(removeCookie).not.toHaveBeenCalled(); });packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts (2)
70-70: Consider adding a specific error message expectation.The test could be more specific about the expected error when no refresh token is available.
- await expect(refreshAccessToken()).rejects.toThrow() + await expect(refreshAccessToken()).rejects.toThrow('No refresh token available')
25-76: Consider adding platform-specific test cases.Given that the implementation includes platform-specific handling (as mentioned in the AI summary), consider adding test cases that verify the behavior across different platforms using the
isMobilePlatformmock.Example test structure:
it('should handle token refresh differently on mobile platforms', async () => { // Mock mobile platform mockIsMobilePlatform.mockReturnValue(true) // Test mobile-specific behavior }) it('should handle token refresh differently on web platforms', async () => { // Mock web platform mockIsMobilePlatform.mockReturnValue(false) // Test web-specific behavior })packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts (1)
11-19: Add test cases for invalid token scenariosWhile the test correctly verifies successful token retrieval, consider adding test cases for:
- Invalid/malformed token
- Expired token
- Token with missing user fields
it('should handle invalid token format', async () => { ;(getTokenAsync as jest.Mock).mockReturnValue('invalid-token') const user = await getUserAsync() expect(user).toBeNull() }) it('should handle token with missing user fields', async () => { ;(getTokenAsync as jest.Mock).mockReturnValue('eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.e30.Et9HFtf9R3GEMA0IICOfFMVXY7kkTX1wr4qCyhIf58U') const user = await getUserAsync() expect(user).toBeNull() })packages/utils/types/env.ts (1)
1-5: LGTM! Consider adding JSDoc comments.The type definition is well-structured and follows Expo's naming conventions. Consider adding JSDoc comments to document the purpose of each environment variable.
+/** + * Environment variables for mobile platform configuration + */ export type MobileDefaultEnvironmentVariables = { + /** Base URL for API endpoints */ EXPO_PUBLIC_API_BASE_URL: string + /** GraphQL Relay endpoint */ EXPO_PUBLIC_RELAY_ENDPOINT: string + /** WebSocket endpoint for Relay */ EXPO_PUBLIC_WS_RELAY_ENDPOINT: string }packages/utils/functions/expo/index.ts (1)
5-13: LGTM! Consider enhancing error handling.The implementation is well-typed and safely handles undefined cases. Consider adding more specific error handling for development environments.
export const getExpoConstant = < T extends Record<string, string> = MobileDefaultEnvironmentVariables, >( key: keyof T, ) => { - if (!Constants?.expoConfig?.extra) return undefined + if (!Constants?.expoConfig?.extra) { + if (process.env.NODE_ENV === 'development') { + console.warn(`Expo config extra is not defined when accessing key: ${String(key)}`) + } + return undefined + } return (Constants.expoConfig.extra as T)[key] }packages/utils/functions/token/getAccessToken/index.ts (1)
10-13: LGTM! Consider adding type assertion for URL.The implementation correctly handles both Next.js and Expo environments. Consider adding type assertion to ensure the URL is non-null.
- const EXPO_PUBLIC_API_BASE_URL = getExpoConstant('EXPO_PUBLIC_API_BASE_URL') + const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL ?? getExpoConstant('EXPO_PUBLIC_API_BASE_URL') + if (!baseUrl) { + throw new Error('API base URL is not configured.') + } const response = await fetch( - `${process.env.NEXT_PUBLIC_API_BASE_URL ?? EXPO_PUBLIC_API_BASE_URL}/auth/refresh`, + `${baseUrl}/auth/refresh`,pnpm-workspace.yaml (1)
130-133: Add newline at end of fileAdd a newline character at the end of the file to comply with YAML standards.
react-native-core: expo-constants: ~17.0.3 expo-secure-store: ~14.0.0 - react-native: 0.76.3 + react-native: 0.76.3 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
packages/utils/functions/axios/createAxiosInstance/index.ts (1)
77-87: Consider simplifying the nested conditions.The nested conditions for data transformation could be simplified using early returns or a more declarative approach.
- if (request.data) { - if (!file || !useFormData) { - if (stringifyBody) { - if (decamelizeRequestBodyKeys) { - request.data = JSON.stringify(humps.decamelizeKeys(request.data)) - } else { - request.data = JSON.stringify(request.data) - } - } else if (decamelizeRequestBodyKeys) { - request.data = humps.decamelizeKeys(request.data) - } + if (!request.data) return; + + if (file && useFormData) { + // Handle form data case + return; + } + + if (decamelizeRequestBodyKeys) { + request.data = humps.decamelizeKeys(request.data); + } + + if (stringifyBody) { + request.data = JSON.stringify(request.data); + }packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (2)
139-148: Enhance FormData mock implementation.Consider enhancing the FormData mock to better match actual FormData behavior:
- Add support for multiple values with the same key
- Implement
get,getAll, andentriesmethodsglobal.FormData = class MockFormData { - _store = {} + _entries = [] append(key: any, value: any) { - this._store[key] = value + this._entries.push([key, value]) } has(key: any) { - return Object.prototype.hasOwnProperty.call(this._store, key) + return this._entries.some(([k]) => k === key) } + get(key: string) { + return this._entries.find(([k]) => k === key)?.[1] + } + getAll(key: string) { + return this._entries.filter(([k]) => k === key).map(([, v]) => v) + } + *entries() { + yield* this._entries + } }
269-271: Add missing response interceptor tests.The TODO comments indicate missing test coverage for response interceptor functionality.
Would you like me to help generate the missing test cases for:
- Response data decamelization by default
- Conditional response data decamelization?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
packages/authentication/__mocks__/react-native.ts(1 hunks)packages/authentication/modules/access/preAuthenticateJWT/index.ts(2 hunks)packages/authentication/modules/access/useLogin/index.ts(3 hunks)packages/authentication/modules/user/getUser/__tests__/getUser.test.ts(1 hunks)packages/authentication/modules/user/getUser/index.ts(1 hunks)packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts(1 hunks)packages/authentication/modules/user/getUserAsync/index.ts(1 hunks)packages/authentication/package.json(1 hunks)packages/components/__mocks__/react-native.ts(1 hunks)packages/graphql/config/environment.ts(3 hunks)packages/graphql/package.json(1 hunks)packages/graphql/utils/createTestEnvironment/index.ts(1 hunks)packages/test/__mocks__/react-native.ts(1 hunks)packages/test/jest.config.ts(1 hunks)packages/utils/__mocks__/react-native.ts(1 hunks)packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts(4 hunks)packages/utils/functions/axios/createAxiosInstance/index.ts(4 hunks)packages/utils/functions/expo/index.ts(1 hunks)packages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.ts(4 hunks)packages/utils/functions/fetch/baseAppFetch/index.ts(4 hunks)packages/utils/functions/os/__tests__/os.test.ts(1 hunks)packages/utils/functions/os/index.ts(1 hunks)packages/utils/functions/token/getAccessToken/index.ts(1 hunks)packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts(5 hunks)packages/utils/functions/token/getToken/__tests__/getToken.server.test.ts(2 hunks)packages/utils/functions/token/getToken/index.ts(1 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.client.test.ts(5 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.server.test.ts(2 hunks)packages/utils/functions/token/getTokenAsync/index.ts(1 hunks)packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts(1 hunks)packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts(6 hunks)packages/utils/functions/token/removeTokenAsync/index.ts(1 hunks)packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts(7 hunks)packages/utils/functions/token/setTokenAsync/index.ts(1 hunks)packages/utils/index.ts(2 hunks)packages/utils/package.json(1 hunks)packages/utils/types/env.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/test/mocks/react-native.ts
- packages/utils/functions/os/index.ts
- packages/authentication/mocks/react-native.ts
- packages/authentication/modules/user/getUser/index.ts
- packages/utils/mocks/react-native.ts
- packages/utils/functions/token/getTokenAsync/index.ts
- packages/utils/functions/token/getTokenAsync/tests/getTokenAsync.server.test.ts
- packages/utils/functions/token/removeTokenAsync/index.ts
- packages/test/jest.config.ts
- packages/components/mocks/react-native.ts
- packages/graphql/utils/createTestEnvironment/index.ts
- packages/utils/functions/token/getTokenAsync/tests/getTokenAsync.client.test.ts
- packages/utils/functions/token/getToken/tests/getToken.server.test.ts
- packages/utils/functions/token/getToken/index.ts
- packages/authentication/modules/user/getUserAsync/index.ts
- packages/utils/functions/token/setTokenAsync/index.ts
- packages/utils/functions/token/getToken/tests/getToken.client.test.ts
- packages/authentication/modules/user/getUser/tests/getUser.test.ts
- packages/utils/functions/fetch/baseAppFetch/tests/baseAppFetch.test.ts
- packages/utils/functions/token/setTokenAsync/tests/setTokenAsync.test.ts
- packages/authentication/modules/access/useLogin/index.ts
- packages/utils/functions/os/tests/os.test.ts
- packages/utils/functions/fetch/baseAppFetch/index.ts
- packages/graphql/config/environment.ts
- packages/authentication/package.json
🧰 Additional context used
🪛 yamllint (1.35.1)
pnpm-workspace.yaml
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (18)
packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts (2)
5-5: LGTM! Clean mock setup for platform detection.The mock setup for
isMobilePlatformfollows Jest best practices and maintains consistency with other mocks in the file.Also applies to: 15-17
Line range hint
27-33: LGTM! Comprehensive platform-specific behavior testing.The test cases effectively verify that:
- Mobile platforms use
deleteItemAsync- Non-mobile platforms use
removeCookie- Each platform exclusively uses its designated method
Also applies to: 37-43
packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts (4)
4-18: LGTM! Well-structured mock declarations.The mock declarations are properly isolated and follow a consistent pattern, covering all necessary token operations.
26-29: LGTM! Clear and consistent mock variable assignments.The mock variables follow jest best practices with descriptive naming and proper require statements.
55-64: LGTM! Comprehensive error handling test.The test case properly verifies both the error propagation and cleanup operations when token refresh fails.
46-48: Verify the security implications ofsecure: false.The test sets
secure: falsefor token storage. Please verify if this aligns with the security requirements, especially for production environments.✅ Verification successful
secure: falsesetting is correctly implementedThe test case appropriately mirrors the implementation where
secureis dynamically set based on the environment (process.env.NODE_ENV === 'production'). In production, cookies are secure, while in test/development environments, they are not. This is the expected behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of secure option configuration rg -A 2 "secure:" --type tsLength of output: 1473
packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts (2)
Line range hint
21-27: LGTM!The test case correctly verifies the behavior when no token is present.
1-8: Verify token-based authentication transitionThe import changes and mock setup correctly reflect the architectural shift from cookie-based to token-based authentication.
Let's verify that this transition is consistent across the codebase:
packages/utils/index.ts (1)
16-16: LGTM! New exports support mobile functionality.The addition of expo functions and environment types aligns well with the mobile internal testing objectives.
Also applies to: 33-33
pnpm-workspace.yaml (1)
130-133: LGTM! New react-native-core catalogThe addition of the react-native-core catalog with appropriate version constraints looks good and aligns with the mobile testing objectives.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
packages/utils/functions/axios/createAxiosInstance/index.ts (6)
27-32: LGTM! Well-structured configuration parameters.The new configuration parameters for request/response data transformation are well-organized with sensible defaults, improving the flexibility of the axios instance.
43-48: LGTM! Improved content type configuration.The conditional content type configuration is well-implemented, allowing for more flexibility while maintaining correct headers for different request types.
Line range hint
52-76: LGTM! Robust token management implementation.The token management logic is well-structured with proper async handling, token validation, and refresh flow.
90-99: Form data handling needs improvement.The form data handling could be improved for better robustness:
- The null check
if (!value) returnmight skip validfalseboolean values- The
value.toString()call might fail for null/undefined values that pass the initial check
114-134: LGTM! Improved response handling.The response handling is well-implemented with proper content type checks and error handling.
Line range hint
140-147: LGTM! Well-structured exports.The exported instances provide convenient pre-configured options for common use cases.
packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (2)
5-36: LGTM! Comprehensive test setup.The test setup is well-structured with proper mocks for all dependencies and clear mock implementations.
170-267: LGTM! Comprehensive test coverage.The test cases thoroughly verify the new functionality with clear assertions and good coverage of edge cases.
| const EXPO_PUBLIC_API_BASE_URL = getExpoConstant('EXPO_PUBLIC_API_BASE_URL') | ||
| const response = await fetch( | ||
| `${process.env.NEXT_PUBLIC_API_BASE_URL ?? process.env.EXPO_PUBLIC_API_BASE_URL}/auth/pre-auth/jwt`, | ||
| `${process.env.NEXT_PUBLIC_API_BASE_URL ?? EXPO_PUBLIC_API_BASE_URL}/auth/pre-auth/jwt`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting URL resolution to a shared utility.
This URL resolution pattern is duplicated across multiple files. Consider extracting it to a shared utility function.
+// In packages/utils/functions/url/index.ts
+export const getApiBaseUrl = () => {
+ const baseUrl = process.env.NEXT_PUBLIC_API_BASE_URL ?? getExpoConstant('EXPO_PUBLIC_API_BASE_URL')
+ if (!baseUrl) {
+ throw new Error('API base URL is not configured.')
+ }
+ return baseUrl
+}
- const EXPO_PUBLIC_API_BASE_URL = getExpoConstant('EXPO_PUBLIC_API_BASE_URL')
- const response = await fetch(
- `${process.env.NEXT_PUBLIC_API_BASE_URL ?? EXPO_PUBLIC_API_BASE_URL}/auth/pre-auth/jwt`,
+ const response = await fetch(
+ `${getApiBaseUrl()}/auth/pre-auth/jwt`,Committable suggestion skipped: line range outside the PR's diff.
| const EXPO_PUBLIC_API_BASE_URL = getExpoConstant('EXPO_PUBLIC_API_BASE_URL') | ||
|
|
||
| const instance = _axios.create({ | ||
| baseURL: process.env.NEXT_PUBLIC_API_BASE_URL, | ||
| baseURL: baseUrl ?? EXPO_PUBLIC_API_BASE_URL, |
There was a problem hiding this comment.
💡 Codebase verification
Add validation to ensure base URL is properly configured
The base URL should be validated to prevent runtime issues with empty strings or malformed URLs:
const EXPO_PUBLIC_API_BASE_URL = getExpoConstant('EXPO_PUBLIC_API_BASE_URL')
if (!baseUrl && !EXPO_PUBLIC_API_BASE_URL) {
throw new Error('Base URL must be configured either via baseUrl parameter or EXPO_PUBLIC_API_BASE_URL environment variable')
}
const instance = _axios.create({
baseURL: baseUrl ?? EXPO_PUBLIC_API_BASE_URL,🔗 Analysis chain
Consider adding error handling for base URL configuration.
While the fallback to EXPO_PUBLIC_API_BASE_URL is good, consider adding validation to ensure at least one base URL is available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if base URL configuration exists in other files
rg -l "NEXT_PUBLIC_API_BASE_URL|EXPO_PUBLIC_API_BASE_URL" --type ts
Length of output: 784
Script:
#!/bin/bash
# Check getExpoConstant implementation and env types
echo "=== getExpoConstant implementation ==="
ast-grep --pattern 'function getExpoConstant($_) { $$$ }'
echo -e "\n=== Environment types ==="
cat packages/utils/types/env.ts
echo -e "\n=== Existing validation patterns ==="
rg "(!|===|typeof.*undefined|null.*check)" packages/utils/functions/axios/createAxiosInstance/index.ts -A 2
Length of output: 1447
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/utils/functions/token/getAccessToken/__tests__/getAccessToken.test.ts (1)
24-25: Improve test reliability by properly managing environment variables.The environment variable setup should be moved to test lifecycle hooks for better isolation and maintainability.
Consider applying these changes:
describe('getAccessToken', () => { + const TEST_API_BASE_URL = 'http://localhost:3000' + beforeEach(() => { jest.clearAllMocks() + process.env.NEXT_PUBLIC_API_BASE_URL = TEST_API_BASE_URL + }) + + afterEach(() => { + delete process.env.NEXT_PUBLIC_API_BASE_URL }) it('should call fetch with the correct URL and headers', async () => { - process.env.NEXT_PUBLIC_API_BASE_URL = 'http://localhost:3000' - const refreshToken = 'test-refresh-token'packages/test/__mocks__/expo.ts (3)
1-11: Enhance mock implementation robustnessThe current ExpoSecureStore mock implementation could be improved in several ways:
- Extract 'ACCESS_KEY_NAME' as a constant
- Add error scenarios simulation
- Add TypeScript types
- Implement value storage simulation for setItemAsync
+const ACCESS_KEY_NAME = 'ACCESS_KEY_NAME' + +interface SecureStore { + getItemAsync: jest.Mock<Promise<string | null>> + setItemAsync: jest.Mock<Promise<boolean>> + deleteItemAsync: jest.Mock<Promise<boolean>> +} + +const mockStorage = new Map<string, string>() + -const ExpoSecureStore = { +const ExpoSecureStore: SecureStore = { getItemAsync: jest.fn(async (key: string) => { - return key === 'ACCESS_KEY_NAME' ? 'mocked_value' : null + return key === ACCESS_KEY_NAME ? 'mocked_value' : mockStorage.get(key) ?? null }), setItemAsync: jest.fn(async (key: string, value: string) => { + if (!key || !value) { + throw new Error('Invalid key or value') + } + mockStorage.set(key, value) return true }), deleteItemAsync: jest.fn(async (key: string) => { + return mockStorage.delete(key) - return true }), }
13-21: Improve Constants mock configurationThe current implementation uses undefined values which might not be ideal for testing. Consider:
- Using meaningful default values
- Adding TypeScript types
- Adding JSDoc documentation
+interface ExpoConfig { + extra: { + EXPO_PUBLIC_API_BASE_URL: string | undefined + EXPO_PUBLIC_RELAY_ENDPOINT: string | undefined + EXPO_PUBLIC_WS_RELAY_ENDPOINT: string | undefined + } +} + +/** + * Mock Constants object for testing + * @description Simulates Expo's Constants object with configurable environment variables + */ -const Constants = { +const Constants: { expoConfig: ExpoConfig } = { expoConfig: { extra: { - EXPO_PUBLIC_API_BASE_URL: undefined, - EXPO_PUBLIC_RELAY_ENDPOINT: undefined, - EXPO_PUBLIC_WS_RELAY_ENDPOINT: undefined, + EXPO_PUBLIC_API_BASE_URL: 'http://localhost:3000', + EXPO_PUBLIC_RELAY_ENDPOINT: 'http://localhost:4000/graphql', + EXPO_PUBLIC_WS_RELAY_ENDPOINT: 'ws://localhost:4000/graphql', }, }, }
23-26: Revise export strategyThe current export strategy using spread operator could lead to naming conflicts. Consider:
- Using named exports
- Adding TypeScript module declaration
+// Add at the top of the file +declare module 'expo' { + export { SecureStore, Constants } +} + -module.exports = { - ...ExpoSecureStore, - ...Constants, +module.exports = { + SecureStore: ExpoSecureStore, + Constants, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/authentication/__mocks__/expo.ts(1 hunks)packages/authentication/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/components/__mocks__/expo.ts(1 hunks)packages/components/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/test/__mocks__/expo.ts(1 hunks)packages/test/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/test/jest.config.ts(1 hunks)packages/utils/__mocks__/expo.ts(1 hunks)packages/utils/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/utils/functions/token/getAccessToken/__tests__/getAccessToken.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/components/mocks/expoSecureStoreMock.ts
- packages/utils/mocks/expoSecureStoreMock.ts
- packages/test/mocks/expoSecureStoreMock.ts
- packages/authentication/mocks/expoSecureStoreMock.ts
✅ Files skipped from review due to trivial changes (3)
- packages/utils/mocks/expo.ts
- packages/authentication/mocks/expo.ts
- packages/components/mocks/expo.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/test/jest.config.ts
🔇 Additional comments (2)
packages/utils/functions/token/getAccessToken/__tests__/getAccessToken.test.ts (1)
Line range hint
13-63: Enhance test coverage for platform-specific scenarios.Based on the implementation changes mentioned in the summary, the test suite should be expanded to cover:
- Platform-specific behavior using
isMobilePlatform()- Base URL resolution from both NEXT and EXPO environments
- Server error responses (e.g., 401, 403, 500)
Let me verify the implementation changes that need test coverage:
packages/test/__mocks__/expo.ts (1)
1-26: Verify mock usage across test filesLet's ensure this mock is properly configured and used consistently across all test files.
anicioalexandre
force-pushed
the
BA-2020-mobile-internal-testing
branch
from
23d8454 to
d73be4f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
packages/test/CHANGELOG.md (1)
5-7: Enhance the changelog entry with more detailsSince this PR focuses on mobile internal testing, please expand the changelog entry to include:
- The purpose of the new mock files
- Which mobile features they help test
- Any related configuration changes needed to use these mocks
Example format:
### Patch Changes -Add `expo` and `react-native` mock files. +Add `expo` and `react-native` mock files: +- Added mocks to support mobile platform testing +- Includes mocks for [list specific expo/react-native features] +- See jest.config.js for usage examplespackages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts (2)
27-27: Consider using beforeEach for common mock setup.The platform mock setup is repeated in each test case. Consider moving it to a
beforeEachblock with a shared variable to control the platform type.describe('removeTokenAsync', () => { const mockKey = 'test-key' + let isMobile = false + + beforeEach(() => { + isMobile = false + ;(isMobilePlatform as jest.Mock).mockReturnValue(isMobile) + }) afterEach(() => { jest.clearAllMocks() }) it('should call deleteItemAsync on mobile platform', async () => { - ;(isMobilePlatform as jest.Mock).mockReturnValue(true) + isMobile = true // ... rest of the test })Also applies to: 37-37, 47-47, 59-59
Line range hint
20-67: Consider adding edge cases to improve test coverage.The current test suite covers the main scenarios well. Consider adding these additional test cases:
- Test with empty key
- Test with special characters in the key
- Test concurrent token removals
Example test case:
it('should handle empty key correctly', async () => { (isMobilePlatform as jest.Mock).mockReturnValue(true) await removeTokenAsync('') expect(deleteItemAsync).toHaveBeenCalledWith('') expect(deleteItemAsync).toHaveBeenCalledTimes(1) })packages/utils/CHANGELOG.md (2)
3-4: Consider adding a release date.For better tracking and historical reference, consider adding the release date next to the version number.
-## 3.1.0 +## 3.1.0 (2025-01-XX)
11-11: Document the mock files' purpose and usage.The addition of
expoandreact-nativemock files should include more details about their intended use cases and configuration requirements.-Add `expo` and `react-native` mock files. +Add `expo` and `react-native` mock files for improved testing of platform-specific functionality. These mocks can be used in Jest tests by configuring the `moduleNameMapper` in your Jest config.pnpm-workspace.yaml (2)
130-133: Add newline at end of file.Add a newline character at the end of the file to comply with YAML best practices and fix the linting error.
Apply this diff:
react-native-core: expo-constants: ~17.0.3 expo-secure-store: ~14.0.0 - react-native: 0.76.3 + react-native: 0.76.3 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
130-133: Consider using consistent version constraints.The catalog uses mixed version constraints:
~for Expo packages (allows patch updates)- No prefix for React Native (fixed version)
Consider using consistent version constraints across related packages to make maintenance easier and prevent potential compatibility issues.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
packages/utils/functions/axios/createAxiosInstance/index.ts (2)
43-48: Improve code reusability by extracting common header setting logicThe content type header setting could be more DRY by using an array of methods.
- if (setContentType) { - const contentType = file ? 'multipart/form-data' : 'application/json' - instance.defaults.headers.post['Content-Type'] = contentType - instance.defaults.headers.patch['Content-Type'] = contentType - instance.defaults.headers.put['Content-Type'] = contentType - } + if (setContentType) { + const contentType = file ? 'multipart/form-data' : 'application/json' + const methods = ['post', 'patch', 'put'] + methods.forEach(method => { + instance.defaults.headers[method]['Content-Type'] = contentType + }) + }
Line range hint
141-147: Consider lazy initialization of exported axios instancesCreating axios instances at module level makes it harder to configure them for different environments and complicates testing. Consider using factory functions or lazy initialization.
-export const { axios, requestInterceptorId, responseInterceptorId } = createAxiosInstance() - -export const { - axios: axiosForFiles, - requestInterceptorId: requestInterceptorIdForFiles, - responseInterceptorId: responseInterceptorIdForFiles, -} = createAxiosInstance({ file: true }) +let defaultInstance: ReturnType<typeof createAxiosInstance> | null = null +let fileInstance: ReturnType<typeof createAxiosInstance> | null = null + +export const getDefaultAxios = () => { + if (!defaultInstance) { + defaultInstance = createAxiosInstance() + } + return defaultInstance +} + +export const getFileAxios = () => { + if (!fileInstance) { + fileInstance = createAxiosInstance({ file: true }) + } + return fileInstance +}packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (2)
139-148: Enhance FormData mock implementationThe current FormData mock is basic and might not catch all edge cases. Consider enhancing it with:
- Support for
getAll()to test multiple values with the same key- Support for
entries()to verify all stored data- Type-safe implementation
- global.FormData = class MockFormData { - _store = {} - append(key: any, value: any) { - // @ts-ignore - this._store[key] = value - } - has(key: any) { - return Object.prototype.hasOwnProperty.call(this._store, key) - } - } + global.FormData = class MockFormData { + private store: Map<string, any[]> = new Map() + + append(key: string, value: any): void { + const values = this.store.get(key) || [] + values.push(value) + this.store.set(key, values) + } + + has(key: string): boolean { + return this.store.has(key) + } + + getAll(key: string): any[] { + return this.store.get(key) || [] + } + + *entries(): IterableIterator<[string, any]> { + for (const [key, values] of this.store) { + for (const value of values) { + yield [key, value] + } + } + } + }
170-188: Improve test descriptions for better clarityThe test descriptions could be more specific about the behavior being tested. Consider renaming:
- it('should stringify and decamelize request body by default' + it('should stringify request body and convert camelCase keys to snake_case by default' - it('should not stringify and decamelize the body if `stringifyBody` is false' + it('should only convert camelCase keys to snake_case when stringifyBody is false' - it('should not decamelize the body if `decamelizeRequestBodyKeys` is false' + it('should only stringify the body when decamelizeRequestBodyKeys is false'Also applies to: 190-208, 210-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/__mocks__/expo.ts(1 hunks)packages/authentication/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/authentication/__mocks__/react-native.ts(1 hunks)packages/authentication/modules/access/preAuthenticateJWT/index.ts(2 hunks)packages/authentication/modules/access/useLogin/index.ts(3 hunks)packages/authentication/modules/user/getUser/__tests__/getUser.test.ts(1 hunks)packages/authentication/modules/user/getUser/index.ts(1 hunks)packages/authentication/modules/user/getUserAsync/__tests__/getUserAsync.test.ts(1 hunks)packages/authentication/modules/user/getUserAsync/index.ts(1 hunks)packages/authentication/package.json(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/__mocks__/expo.ts(1 hunks)packages/components/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/components/__mocks__/react-native.ts(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/graphql/CHANGELOG.md(1 hunks)packages/graphql/config/environment.ts(3 hunks)packages/graphql/package.json(1 hunks)packages/graphql/utils/createTestEnvironment/index.ts(1 hunks)packages/provider/CHANGELOG.md(1 hunks)packages/provider/package.json(1 hunks)packages/test/CHANGELOG.md(1 hunks)packages/test/__mocks__/expo.ts(1 hunks)packages/test/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/test/__mocks__/react-native.ts(1 hunks)packages/test/jest.config.ts(1 hunks)packages/test/package.json(1 hunks)packages/utils/CHANGELOG.md(1 hunks)packages/utils/__mocks__/expo.ts(1 hunks)packages/utils/__mocks__/expoSecureStoreMock.ts(0 hunks)packages/utils/__mocks__/react-native.ts(1 hunks)packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts(4 hunks)packages/utils/functions/axios/createAxiosInstance/index.ts(4 hunks)packages/utils/functions/expo/index.ts(1 hunks)packages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.ts(4 hunks)packages/utils/functions/fetch/baseAppFetch/index.ts(4 hunks)packages/utils/functions/os/__tests__/os.test.ts(1 hunks)packages/utils/functions/os/index.ts(1 hunks)packages/utils/functions/token/getAccessToken/__tests__/getAccessToken.test.ts(1 hunks)packages/utils/functions/token/getAccessToken/index.ts(1 hunks)packages/utils/functions/token/getToken/__tests__/getToken.client.test.ts(5 hunks)packages/utils/functions/token/getToken/__tests__/getToken.server.test.ts(2 hunks)packages/utils/functions/token/getToken/index.ts(1 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.client.test.ts(5 hunks)packages/utils/functions/token/getTokenAsync/__tests__/getTokenAsync.server.test.ts(2 hunks)packages/utils/functions/token/getTokenAsync/index.ts(1 hunks)packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts(1 hunks)packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts(6 hunks)packages/utils/functions/token/removeTokenAsync/index.ts(1 hunks)packages/utils/functions/token/setTokenAsync/__tests__/setTokenAsync.test.ts(7 hunks)packages/utils/functions/token/setTokenAsync/index.ts(1 hunks)packages/utils/index.ts(2 hunks)packages/utils/package.json(3 hunks)packages/utils/types/env.ts(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (4)
- packages/components/mocks/expoSecureStoreMock.ts
- packages/utils/mocks/expoSecureStoreMock.ts
- packages/test/mocks/expoSecureStoreMock.ts
- packages/authentication/mocks/expoSecureStoreMock.ts
✅ Files skipped from review due to trivial changes (6)
- packages/test/package.json
- packages/provider/CHANGELOG.md
- packages/wagtail/package.json
- packages/provider/package.json
- packages/components/package.json
- packages/components/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (38)
- packages/authentication/mocks/expo.ts
- packages/test/mocks/react-native.ts
- packages/graphql/utils/createTestEnvironment/index.ts
- packages/utils/functions/expo/index.ts
- packages/graphql/package.json
- packages/utils/functions/token/getTokenAsync/index.ts
- packages/utils/mocks/expo.ts
- packages/utils/functions/token/getTokenAsync/tests/getTokenAsync.server.test.ts
- packages/authentication/mocks/react-native.ts
- packages/utils/functions/token/getAccessToken/tests/getAccessToken.test.ts
- packages/authentication/modules/access/preAuthenticateJWT/index.ts
- packages/utils/types/env.ts
- packages/utils/functions/os/tests/os.test.ts
- packages/utils/functions/token/getToken/tests/getToken.server.test.ts
- packages/utils/functions/token/getToken/index.ts
- packages/components/mocks/react-native.ts
- packages/utils/functions/fetch/baseAppFetch/tests/baseAppFetch.test.ts
- packages/utils/functions/token/getToken/tests/getToken.client.test.ts
- packages/components/mocks/expo.ts
- packages/utils/functions/os/index.ts
- packages/utils/mocks/react-native.ts
- packages/authentication/modules/user/getUser/index.ts
- packages/utils/functions/token/removeTokenAsync/index.ts
- packages/test/jest.config.ts
- packages/authentication/modules/user/getUser/tests/getUser.test.ts
- packages/utils/functions/token/setTokenAsync/index.ts
- packages/graphql/config/environment.ts
- packages/utils/functions/token/getTokenAsync/tests/getTokenAsync.client.test.ts
- packages/authentication/modules/user/getUserAsync/index.ts
- packages/utils/index.ts
- packages/test/mocks/expo.ts
- packages/authentication/modules/user/getUserAsync/tests/getUserAsync.test.ts
- packages/utils/functions/fetch/baseAppFetch/index.ts
- packages/utils/functions/token/getAccessToken/index.ts
- packages/utils/functions/token/setTokenAsync/tests/setTokenAsync.test.ts
- packages/authentication/modules/access/useLogin/index.ts
- packages/utils/package.json
- packages/authentication/package.json
🧰 Additional context used
🪛 yamllint (1.35.1)
pnpm-workspace.yaml
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (15)
packages/test/CHANGELOG.md (1)
3-4: LGTM on version incrementThe bump to version 2.0.6 correctly follows semantic versioning for non-breaking changes.
packages/utils/functions/token/removeTokenAsync/__tests__/removeTokenAsync.test.ts (1)
15-17: LGTM! Mock setup follows best practices.The mock setup for
isMobilePlatformis well-structured and consistent with other mocks in the file.packages/utils/CHANGELOG.md (1)
7-11: Verify the impact of platform detection changes.The switch from environment variables to utility functions (
getExpoConstantandisMobilePlatform) represents a significant architectural change in how platform-specific behavior is handled. This could affect existing implementations.Let's verify the usage of these new utilities:
✅ Verification successful
Platform detection changes have been properly implemented
The migration from environment variables to utility functions has been completed successfully across all packages. The changes are consistently applied and well-tested, with no remaining direct usage of the old environment variables except in type definitions and changelogs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct usage of the old environment variables # and verify that the new utilities are properly imported where needed. echo "Checking for any remaining EXPO_PUBLIC_PLATFORM usage..." rg "EXPO_PUBLIC_PLATFORM" echo "Checking for any remaining EXPO_PUBLIC_API_BASE_URL usage..." rg "EXPO_PUBLIC_API_BASE_URL" echo "Verifying isMobilePlatform imports and usage..." rg "isMobilePlatform" echo "Verifying getExpoConstant imports and usage..." rg "getExpoConstant"Length of output: 10255
pnpm-workspace.yaml (1)
130-133: Review React Native and Expo versions for compatibility.The React Native version (0.76.3) appears to be a pre-release or incorrect version number, as the latest stable version is in the 0.73.x range. Additionally, ensure that the Expo SDK versions (
expo-constantsandexpo-secure-store) are compatible with the chosen React Native version.Run this script to check the latest versions and compatibility:
🧰 Tools
🪛 yamllint (1.35.1)
[error] 133-133: no new line character at the end of file
(new-line-at-end-of-file)
packages/utils/functions/token/refreshAccessToken/__tests__/refreshAccessToken.test.ts (4)
4-19: LGTM! Well-structured mock declarations.The mock declarations follow a consistent pattern and properly isolate the dependencies for testing.
26-30: LGTM! Clear and consistent mock assignments.The mock variable assignments follow a clear naming convention and properly reference the mocked modules.
37-75: LGTM! Comprehensive test coverage.The test cases effectively cover:
- Successful token refresh
- Failed token refresh with proper cleanup
- Missing token scenario with appropriate error handling
46-48: Verify thesecure: falsesetting for mobile platforms.Given that this PR involves mobile platform handling, verify if the
secure: falsesetting is appropriate for both web and mobile contexts.✅ Verification successful
The
secure: falsesetting in the test is correctThe implementation uses platform-specific storage:
- Mobile: Uses
expo-secure-storewhich provides secure storage by default- Web: Uses cookies where
secureis set based on the environment (truein production,falseotherwise)The test correctly reflects the non-production behavior for web platform, while mobile platforms use a separate secure storage mechanism unaffected by this setting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of secure cookie settings to ensure consistency rg -l 'secure:\s*(true|false)' --type ts # Search for platform-specific cookie configurations ast-grep --pattern 'setTokenAsync($_, $_, { secure: $_ })'Length of output: 1165
Script:
#!/bin/bash # Search for platform detection or mobile-specific configurations rg -l '(isMobile|platform|mobile)' --type ts # Search for any cookie or token configuration files fd -e ts -e js config cookie tokenLength of output: 1272
Script:
#!/bin/bash # Check the core token handling implementations cat packages/utils/functions/token/setTokenAsync/index.ts cat packages/utils/functions/os/index.ts # Look for any configuration files that might affect token handling fd -e ts -e js -e json "config|settings" --type fLength of output: 2267
packages/design-system/CHANGELOG.md (2)
12-12: Verify the change category modification.The change category for version 0.0.28 has been modified from "Major Changes" to "Patch Changes". This modification needs verification to ensure it aligns with semantic versioning principles, as it could impact consumers of the package.
✅ Verification successful
Changelog category change is correct
The modification from "Major Changes" to "Patch Changes" for version 0.0.28 is appropriate as it only involves adding MDX documentation files, which aligns with semantic versioning principles where documentation changes warrant a patch version bump.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the actual changes in version 0.0.28 # Look for the commit that introduced version 0.0.28 git log -p --all -S "0.0.28" -- "packages/design-system/CHANGELOG.md" # Check for any breaking changes in the components mentioned in the changelog rg "MDX documentation" --type mdx --type tsxLength of output: 74
Script:
#!/bin/bash # Get the full changelog content cat packages/design-system/CHANGELOG.md # Check package.json for version information cat packages/design-system/package.json # Look for related component files fd . packages/design-system/src -t fLength of output: 9217
7-8: Verify the impact of the utils package update.The update from @baseapp-frontend/[email protected] to @baseapp-frontend/[email protected] is a minor version bump that may introduce new features. Let's verify the changes and their impact.
✅ Verification successful
The utils package update is safe to apply.
The changes in @baseapp-frontend/[email protected] focus on HTTP client functionality and environment variables, which are not used by the design-system package. The design-system package only uses stable utility functions that were not modified in this update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in the utils package update # Check the changelog of utils package for any breaking changes fd -g "CHANGELOG.md" -x rg -A 10 "3\.1\.0" {} # Check for any direct usage of utils package that might be affected rg "@baseapp-frontend/utils" --type ts --type tsxLength of output: 1689
Script:
#!/bin/bash # Search for utils package usage with correct extension patterns rg "@baseapp-frontend/utils" -t ts # Search for specific usage of modified functions rg "createAxiosInstance|baseAppFetch|EXPO_PUBLIC_API_BASE_URL|EXPO_PUBLIC_PLATFORM" -t ts # Look for any imports from utils in design-system package fd . packages/design-system -e ts -e tsx -x rg "@baseapp-frontend/utils" {}Length of output: 23058
packages/wagtail/CHANGELOG.md (1)
3-10: Verify compatibility with updated dependenciesThe changelog shows significant version jumps in dependencies:
@baseapp-frontend/graphqlfrom 1.1.15 to 1.2.0@baseapp-frontend/utilsfrom 3.0.5 to 3.1.0These minor version bumps might introduce breaking changes.
✅ Verification successful
Dependencies update verified as safe
The version bumps follow semantic versioning correctly:
@baseapp-frontend/utils: Adds new token management functions while maintaining backward compatibility@baseapp-frontend/graphql: No breaking changes detected🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential breaking changes in the updated dependencies # Look for major API changes in graphql package rg -B 2 -A 2 "breaking|deprecat|remove|rename" packages/graphql/ # Look for major API changes in utils package rg -B 2 -A 2 "breaking|deprecat|remove|rename" packages/utils/Length of output: 16103
packages/graphql/CHANGELOG.md (1)
3-9: Verify complete migration togetExpoConstantThe change from direct
process.envusage togetExpoConstantis good for mobile compatibility. However, we should ensure all environment variables are properly migrated.✅ Verification successful
Migration to
getExpoConstantis complete ✅The codebase shows proper implementation of
getExpoConstantfor all environment variables, with no remaining directprocess.envusage. Note thatEXPO_PUBLIC_API_BASE_URLappears to have been renamed toEXPO_PUBLIC_RELAY_ENDPOINT.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining direct process.env usage echo "Checking for remaining direct process.env usage..." rg "process\.env\.EXPO_PUBLIC" packages/graphql/ # Check for consistent getExpoConstant usage echo "Verifying getExpoConstant implementation..." rg "getExpoConstant" packages/graphql/Length of output: 932
packages/authentication/CHANGELOG.md (1)
3-12: Cross-package version consistency check neededThe changes across packages show a coordinated effort for mobile support, but we should verify version consistency:
- [email protected] depends on [email protected]
- [email protected] depends on [email protected]
- [email protected] depends on [email protected]
This inconsistency in utils version (3.0.6 vs 3.1.0) could lead to compatibility issues.
Also applies to: 3-9, 3-10
packages/utils/functions/axios/createAxiosInstance/index.ts (2)
34-37: Add validation to ensure base URL is properly configuredThe base URL should be validated to prevent runtime issues with empty strings or malformed URLs.
90-99: Improve form data handling robustness.The form data handling could be improved in several ways:
- The null check
if (!value) returnmight skip validfalseboolean values- The
value.toString()call might fail for null/undefined values that pass the initial check
| ## 4.1.0 | ||
|
|
||
| ### Minor Changes | ||
|
|
||
| - Adapt `useLogin` to use `isMobilePlatform` to omit current profile logic from mobile platforms. | ||
| - Replace `process.env.EXPO_PUBLIC_API_BASE_URL` usage with `getExpoConstant`. | ||
| - Adapt `getUser` and `getUserAsync` tests to mock `getToken` and `getTokenAsync`. | ||
| - Add `expo` and `react-native` mock files. | ||
| - Updated dependencies | ||
| - @baseapp-frontend/[email protected] |
There was a problem hiding this comment.
💡 Codebase verification
Mobile platform test coverage is incomplete
The mobile-specific changes in useLogin lack test coverage:
- No tests verifying the conditional logic using
isMobilePlatform - Missing test cases for mobile vs non-mobile behavior differences
- Platform detection function is not mocked in tests
🔗 Analysis chain
Verify mobile platform test coverage
The changes introduce significant mobile-specific adaptations:
- Mobile-specific logic in
useLogin - Environment variable handling changes
- New mock files for expo and react-native
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for mobile scenarios
echo "Checking mobile-specific test coverage..."
rg -B 2 -A 2 "isMobilePlatform" packages/authentication/
# Verify mock implementations
echo "Verifying mock files..."
fd -e ts -e js "mock" packages/authentication/ --exec cat {}
Length of output: 1978
Script:
#!/bin/bash
# Check useLogin tests for mobile coverage
echo "Checking useLogin tests..."
fd "useLogin" packages/authentication/ -t f | grep -i "test\|spec" | xargs cat
# Check if isMobilePlatform is mocked in tests
echo "Checking for isMobilePlatform mocks..."
rg -B 2 -A 2 "mock.*isMobilePlatform" packages/authentication/
Length of output: 3269
| (error) => { | ||
| const contentTypeHeader = error.response?.headers?.['content-type'] || '' | ||
| const isJsonError = contentTypeHeader.includes('application/json') | ||
|
|
||
| if (isJsonError && error.response?.data) { | ||
| const newError = { response: { data: {} } } | ||
| newError.response.data = humps.camelizeKeys(error.response.data) | ||
| newError.response.data = camelizeResponseDataKeys | ||
| ? humps.camelizeKeys(error.response.data) | ||
| : error.response.data | ||
|
|
||
| return Promise.reject(newError) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve important error properties during error transformation
The current error handling discards potentially important error properties like status, statusText, and headers. This could make error handling more difficult for consumers.
- const newError = { response: { data: {} } }
- newError.response.data = camelizeResponseDataKeys
+ const newError = {
+ ...error,
+ response: {
+ ...error.response,
+ data: camelizeResponseDataKeys
+ ? humps.camelizeKeys(error.response.data)
+ : error.response.data
+ }
+ }
- ? humps.camelizeKeys(error.response.data)
- : error.response.data📝 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.
| (error) => { | |
| const contentTypeHeader = error.response?.headers?.['content-type'] || '' | |
| const isJsonError = contentTypeHeader.includes('application/json') | |
| if (isJsonError && error.response?.data) { | |
| const newError = { response: { data: {} } } | |
| newError.response.data = humps.camelizeKeys(error.response.data) | |
| newError.response.data = camelizeResponseDataKeys | |
| ? humps.camelizeKeys(error.response.data) | |
| : error.response.data | |
| return Promise.reject(newError) | |
| } | |
| (error) => { | |
| const contentTypeHeader = error.response?.headers?.['content-type'] || '' | |
| const isJsonError = contentTypeHeader.includes('application/json') | |
| if (isJsonError && error.response?.data) { | |
| const newError = { | |
| ...error, | |
| response: { | |
| ...error.response, | |
| data: camelizeResponseDataKeys | |
| ? humps.camelizeKeys(error.response.data) | |
| : error.response.data | |
| } | |
| } | |
| return Promise.reject(newError) | |
| } |
| // TODO: add tests for response interceptor | ||
| // - should decamelize response data by default | ||
| // - should not decamelize response data if `decamelizeResponseData` is false |
There was a problem hiding this comment.
💡 Codebase verification
Add missing response interceptor tests
The response interceptor implementation exists but lacks test coverage. Add these test cases:
- Should camelize response data for JSON responses when
camelizeResponseDataKeysis true - Should not camelize response data when
camelizeResponseDataKeysis false - Should handle non-JSON content-type responses correctly
- Should return full response when
returnDatais false - Should return only data when
returnDatais true - Should handle error responses appropriately
🔗 Analysis chain
Add missing response interceptor tests
The response interceptor lacks test coverage. Consider adding these test cases:
- Successful response data transformation
- Error response data transformation
- Content-type header handling
- Edge cases (null/undefined data)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current test coverage for response interceptor
rg -B 2 -A 2 "response.*interceptor" packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts
Length of output: 309
Script:
#!/bin/bash
# Check implementation of response interceptor
ast-grep --pattern 'instance.interceptors.response.use' packages/utils/functions/axios/createAxiosInstance/createAxiosInstance.ts
# Search for any response interceptor tests in other files
rg -l "response.*interceptor.*test"
Length of output: 288
Script:
#!/bin/bash
# Find the implementation file
fd createAxiosInstance.ts
# Once we find the file, let's look for response interceptor implementation
rg -B 2 -A 2 "interceptors.response"
# Search for any response interceptor tests
rg -l "test.*response.*interceptor"
Length of output: 658
Script:
#!/bin/bash
# Get the full response interceptor implementation
rg -B 2 -A 10 "interceptors.response.use" packages/utils/functions/axios/createAxiosInstance/index.ts
Length of output: 576
anicioalexandre
changed the title
__package_name__package update -v __package_version__Summary by CodeRabbit
Release Notes
New Features
isMobilePlatform()to improve cross-platform token and environment handling.getExpoConstant()function for retrieving mobile-specific environment variables.Improvements
Chores
Bug Fixes