feat(api-client): 20565 Implement robust authentication and error handling, improve test coverage #954
Conversation
…d mocking and error handling
…handling scenarios
…ling and update proxy configuration access
… undefined expiration dates
…d setup logout endpoint
…roved authentication testing
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces widespread refactoring across tests, factories, API clients, and feature modules. Changes include the removal of boolean flags in API calls in favor of structured authentication settings (via an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ApiClient
participant TokenStore
participant AuthServer
Client->>ApiClient: API request (with authRequirement)
ApiClient->>TokenStore: Check token validity
alt Valid Token
TokenStore-->>ApiClient: Return token
ApiClient->>AuthServer: Forward request with token
AuthServer-->>ApiClient: Return response
else Missing/Invalid Token (MUST)
ApiClient-->>Client: Return authentication error
else Missing Token (OPTIONAL)
ApiClient->>AuthServer: Forward request without token
AuthServer-->>ApiClient: Return response (with warning)
end
ApiClient-->>Client: Return API response
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (55)
✨ 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 ignore |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.eslintignore(0 hunks).eslintrc.identical.keys.cjs(0 hunks).eslintrc.json(0 hunks).gitignore(1 hunks)eslint.config.mjs(1 hunks)eslintrc.identical.keys.mjs(1 hunks)package.json(5 hunks)src/core/api_client/apiClient.ts(1 hunks)src/core/api_client/apiClientError.ts(1 hunks)src/core/api_client/tests/authorization.test.ts(1 hunks)src/core/api_client/tests/errorHandling.test.ts(1 hunks)src/core/api_client/tests/eventHandling.test.ts(1 hunks)src/core/api_client/tests/factories/auth.factory.ts(3 hunks)src/core/api_client/tests/factories/mock.factory.ts(4 hunks)src/core/api_client/tests/factories/token.factory.ts(1 hunks)src/core/api_client/tests/httpHandling.test.ts(1 hunks)src/core/api_client/tests/mocks/replaceUrlWithProxy.mock.ts(1 hunks)src/core/api_client/tests/urlHandling.test.ts(1 hunks)src/core/api_client/types.ts(0 hunks)src/core/api_client/utils.ts(0 hunks)src/core/auth/OidcSimpleClient.test.ts(1 hunks)src/core/auth/OidcSimpleClient.ts(6 hunks)src/core/metrics/app-metrics.ts(0 hunks)src/utils/atoms/createStateMap.test.tsx(1 hunks)src/utils/axios/replaceUrlWithProxy.ts(1 hunks)src/utils/storage/index.ts(1 hunks)vite.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- .eslintrc.identical.keys.cjs
- src/core/metrics/app-metrics.ts
- .eslintignore
- src/core/api_client/types.ts
- .eslintrc.json
- src/core/api_client/utils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/core/api_client/tests/eventHandling.test.ts
[error] 130-130: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
src/utils/atoms/createStateMap.test.tsx
[error] 57-63: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
src/core/api_client/tests/factories/token.factory.ts
[error] 17-17: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 18-18: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 19-19: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 32-32: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/core/api_client/tests/factories/mock.factory.ts
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 246-246: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 252-252: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/core/api_client/tests/factories/auth.factory.ts
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 75-75: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (29)
src/core/api_client/tests/authorization.test.ts (4)
5-6: Import statements are appropriate.The imported modules
fetchMock,ApiClientError, andgetApiErrorKindare correctly included for the test setup.
9-10: Factory imports are correctly added.The
MockFactoryandAuthFactoryare appropriately imported for mocking and authentication purposes in the tests.
22-57: Comprehensive test for password grant authentication.The test case for handling password grant authentication is well-structured. It correctly resets mocks, sets up successful authentication, and verifies both the user's login status and the stored token. Additionally, it checks the token request format to ensure the correct parameters are sent.
60-117: Effective error handling tests cover multiple scenarios.The test for handling authentication failures efficiently covers various error cases, including unauthorized access, bad requests, and network errors. By iterating over the
errorCasesarray, it ensures that each scenario is tested, and the expected error kinds are correctly asserted.src/core/api_client/tests/mocks/replaceUrlWithProxy.mock.ts (1)
1-7: Mock functionreplaceUrlWithProxyMockis correctly implemented.The mock effectively replaces the real
replaceUrlWithProxyfunction, ensuring that URL handling can be tested without invoking the actual implementation, which is essential for isolating test cases.src/core/api_client/apiClientError.ts (2)
16-17: Great improvement in type safety and error checking!The change from
anytounknownand usinginstanceoffor type checking is more type-safe and reliable.
20-21: Consistent error handling improvements across helper functions.The changes maintain consistency in type safety and error handling patterns across all utility functions.
Also applies to: 24-25
src/core/api_client/tests/factories/token.factory.ts (2)
8-11: Good improvement using dynamic timestamps!Using current time-based calculations makes the token generation more realistic and maintainable.
23-26: Consistent time-based token generation.The implementation maintains consistency in how expiration times are calculated across different token types.
Also applies to: 31-35
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/core/api_client/tests/factories/auth.factory.ts (3)
12-17: Well-designed TokenConfig interface.The interface provides a clean and focused set of properties for token configuration.
45-51: Good token creation implementation.The createToken method handles defaults well and correctly implements the expired token scenario.
53-58: Comprehensive mock implementations for different scenarios.The mock methods cover success, error, and logout scenarios, providing good test coverage.
Also applies to: 60-65, 67-72
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/core/api_client/apiClient.ts (1)
154-154: Verify token refresh behavior.The removal of the token assignment simplifies the code, but we should ensure that:
- Token refresh still works correctly
- No race conditions occur during concurrent requests
- Error handling remains consistent
Run this script to check for potential race conditions in token refresh:
✅ Verification successful
Token refresh implementation verified as safe and correct.
The removed token assignment was redundant as the
getAccessToken()call already handles token refresh with proper concurrency protection. The implementation inOidcSimpleClientprevents race conditions during concurrent requests, and error cases are properly handled.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper handling of concurrent token refresh # Look for other token refresh implementations that might need similar updates # Search for token refresh patterns ast-grep --pattern 'await $_.getAccessToken()' # Search for token usage patterns rg -A 5 'getAccessToken'Length of output: 4799
src/core/api_client/tests/eventHandling.test.ts (3)
15-74: LGTM! Well-structured event listener tests.The test suite thoroughly covers error event listener functionality, including registration, triggering, multiple listeners, and unsubscription. The assertions are comprehensive and verify both the error events and the error types.
113-136: LGTM! Comprehensive pool management tests.The test effectively verifies concurrent request tracking and pool updates. Good use of
poolUpdatesarray to track state changes.🧰 Tools
🪛 Biome (1.9.4)
[error] 130-130: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
158-194: LGTM! Thorough error handling tests.The tests effectively verify pool cleanup for both regular and network errors. Good use of MockFactory for simulating different error scenarios.
src/core/api_client/tests/factories/mock.factory.ts (2)
21-30: LGTM! Improved token endpoint mocking.Good improvement using a function for dynamic response generation and tracking call count. This provides more flexibility and better tracking of mock usage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
48-53: LGTM! Consistent pattern for logout endpoint.The logout endpoint mock follows the same pattern as the token endpoint, maintaining consistency in the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/core/auth/OidcSimpleClient.test.ts (3)
27-67: LGTM! Thorough token management tests.Good coverage of both successful and error scenarios in token management. The tests effectively verify token storage, retrieval, and validation.
70-178: LGTM! Comprehensive token refresh tests.Excellent coverage of concurrent refresh scenarios and error handling. Good use of mocks to simulate various token states and server responses.
477-662: LGTM! Thorough token refresh flow tests.Excellent coverage of the token refresh flow, including:
- Preemptive refresh
- Network errors
- Concurrent refresh requests
- Various token states
eslint.config.mjs (2)
145-145: Ensure the correct path forlocalesPathin i18n rulesThe
localesPathspecified in thei18n-checker/json-key-existsrule on line 144 may need to point to a specific JSON file rather than a directory.Please verify that
localesPathis correctly configured. If a specific JSON file is required, adjust the path accordingly.
53-54: Verify the usage offixupPluginRuleswitheslint-plugin-react-hooksThe use of
fixupPluginRuleson line 53 may not be necessary foreslint-plugin-react-hooks. This function is typically used to adjust plugin rules for compatibility, buteslint-plugin-react-hooksshould work without it.Please verify if
fixupPluginRulesis required here. If not, consider simplifying the plugin configuration:- 'react-hooks': fixupPluginRules(reactHooks), + 'react-hooks': reactHooks,Run the following script to check if any errors occur without
fixupPluginRules:eslintrc.identical.keys.mjs (2)
17-17: EnsurefilePathini18n-json/identical-keysrule is correctThe
filePathon line 17 points tocommon.json. Confirm that this is the intended reference file for key validation across all locales.Please verify that
common.jsonis the correct base file. If a different file should be used, update thefilePathaccordingly.
9-12: Confirm the processor configuration for.jsonfilesThe processor configuration on lines 9-12 spreads the
.jsonprocessor fromeslint-plugin-i18n-json. Ensure that this setup correctly processes JSON files.Please verify that the processor is correctly applied. If issues arise, consider adjusting the configuration as follows:
- processor: { - meta: { name: '.json' }, - ...i18nJsonPlugin.processors['.json'], - }, + processor: i18nJsonPlugin.processors['.json'],Run the following script to verify the ESLint configuration:
✅ Verification successful
The processor configuration is correctly implemented
The current implementation that spreads the
.jsonprocessor and adds metadata is valid. While no translation files exist yet, the configuration is properly set up for processing JSON files when they are added. The suggested change to use direct assignment offers no advantages over the current implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ESLint processor configuration for JSON files # Test ESLint with the current configuration npx eslint 'src/core/localization/translations/**/*.json' --config eslintrc.identical.keys.mjsLength of output: 190
Script:
#!/bin/bash # Check the plugin import and processor configuration echo "=== Checking plugin import ===" rg "import.*eslint-plugin-i18n-json" -A 2 echo -e "\n=== Looking for similar processor configurations ===" rg "processor.*json" -A 3 echo -e "\n=== Finding JSON files in translations directory ===" fd -e json . src/core/localization/translationsLength of output: 1018
Script:
#!/bin/bash # Check for localization or translation directories echo "=== Finding all JSON files in the project ===" fd -e json echo -e "\n=== Looking for translation or localization directories ===" fd -t d "translations\|localization\|locale"Length of output: 531
.gitignore (1)
21-21: LGTM! More maintainable ignore patterns.The consolidation of specific patterns into more general ones (
.env.*.local,*.log,configs/*.local.*) improves maintainability while maintaining the same functionality.Also applies to: 30-30, 35-35
package.json (3)
24-24: LGTM! New coverage script for improved reporting.The new
coverage:lcovscript with specific coverage reporting options will help with test coverage tracking and reporting.
126-126: Review the impact of dependency updates.Several dependencies have been updated. Notable changes:
- ESLint major version update (8.x → 9.x)
- Testing-related packages updated
- React ecosystem packages updated
Please ensure:
- The major ESLint version upgrade is compatible
- All tests pass with the updated dependencies
Also applies to: 128-128, 130-130, 140-140, 153-156, 160-160, 165-166, 172-172, 175-175, 178-178, 180-183, 187-187, 196-196
26-26: Verify the impact of ESLint configuration changes.The changes to lint scripts and ESLint configuration might affect existing code. Please ensure:
- The new configuration doesn't introduce breaking changes
- The team is aware of any new linting rules
Also applies to: 28-28
✅ Verification successful
ESLint configuration changes are safe and well-structured ✅
The changes to the lint scripts and ESLint configuration are using standard patterns and up-to-date dependencies. The setup includes proper TypeScript support, React-specific rules, and i18n validation. No breaking changes detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential linting issues with the new configuration # Test: Run ESLint with the new configuration and check for errors eslint --quiet src/**/*.{ts,tsx} > lint_results.txt || true echo "Number of files with potential issues:" wc -l lint_results.txtLength of output: 462
Script:
#!/bin/bash # Find ESLint config files echo "=== ESLint Config Files ===" fd -e json -e js -e yaml -e yml . -E node_modules | grep -i eslint # Show content of ESLint configs if found echo -e "\n=== ESLint Config Contents ===" fd -e json -e js -e yaml -e yml . -E node_modules -x cat {} \; | grep -i eslint -A 10 -B 10 # Check package.json for ESLint-related entries echo -e "\n=== Package.json ESLint Dependencies ===" cat package.json | grep -i eslint -A 2 -B 2Length of output: 29521
✅ Actions performedReviews paused. |
|
|
@coderabbitai resolve |
Bundle size diff
|
✅ Actions performedComments resolved. |
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (16.66%) is below the target coverage (30.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
==========================================
+ Coverage 10.04% 10.69% +0.65%
==========================================
Files 669 670 +1
Lines 29420 29610 +190
Branches 1236 1317 +81
==========================================
+ Hits 2954 3168 +214
+ Misses 25936 25911 -25
- Partials 530 531 +1
☔ View full report in Codecov by Sentry. |
|
Preview environments for this PR:
These previews are automatically updated with each commit. Note: After a new deployment, it may take a few minutes for the changes to propagate and for caches to update. During this time, you might experience temporary loading issues or see an older version of the app. If the app fails to load, please wait a few minutes and try again. |
Bundle ReportChanges will increase total bundle size by 7.51kB (0.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: konturio/disaster-ninja-fe-esmAssets Changed:
Files in
Files in
Files in
Files in
Files in
Files in
Files in
Files in
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
…, and update documentation
… logic, and update documentation
…error handling scenarios
…, introducing new levels (SHOULD, NEVER) and enhancing documentation
…ogic and improve consistency across API calls and tests
…uction; remove unused authentication response types
…ensure proper typing and chaining for wretch requests
…error handling scenarios in authorization tests
…cessary checks for AUTH_REQUIREMENT.NEVER; update tests to reflect new behavior for optional and mandatory authentication scenarios
…licit error messages; update MockApiErrorResponse type to make message optional and enhance mock factory for conditional message inclusion
…t.AUTH_REQUIREMENT in API calls for consistency and maintainability
src/features/bivariate_color_manager/atoms/bivariateColorManagerResource.ts
Show resolved
Hide resolved
vkozio
changed the title
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
https://kontur.fibery.io/Tasks/Task/Improve-coverage-for-auth-and-api-clients-20565
Summary by CodeRabbit