feat(native-app): Add barcode session error #17603
Conversation
… into feat/barcode-session
move contants to env and refactor barcodeSession check to function
WalkthroughThis pull request introduces comprehensive changes to barcode and session management across multiple services and components. The modifications include adding new environment variables for barcode expiration times, enhancing error handling for barcode sessions, and updating configuration schemas to support dynamic expiration settings. The changes span multiple files and services, focusing on improving the reliability and flexibility of barcode-related functionalities. Changes
Suggested Labels
Suggested Reviewers
✨ 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
libs/auth-nest-tools/src/lib/jwt.payload.ts (1)
13-13: LGTM! Consider adding JSDoc comment.The addition of the optional
sidproperty is consistent with the Auth interface changes. Consider adding a JSDoc comment to document the purpose of this session identifier.+ /** Session identifier used for tracking user sessions across devices */ sid?: string,libs/shared/problem/src/ProblemType.ts (1)
12-12: LGTM! Consider documenting the error case.The addition of
BAD_SESSIONfollows the established pattern for problem types. Consider adding documentation that describes this error case and its resolution steps.Add a comment describing the error case:
+ /** Indicates that the user's session is invalid due to access from another device */ BAD_SESSION = 'https://docs.devland.is/reference/problems/bad-session',libs/services/license/src/lib/license.config.ts (1)
8-9: LGTM! Consider adding min/max constraintsThe schema properly defines the new expiration time properties. Consider adding min/max constraints to prevent invalid values.
- barcodeExpireTimeInSec: z.number(), - barcodeSessionExpireTimeInSec: z.number(), + barcodeExpireTimeInSec: z.number().min(1).max(3600), + barcodeSessionExpireTimeInSec: z.number().min(60).max(86400),libs/services/license/src/lib/barcode.service.ts (1)
13-14: Consider using an enum for cache keysWhile the constant is properly defined, consider using an enum if there are or will be multiple cache keys.
-export const BARCODE_ACTIVE_SESSION_KEY = 'activeSession' +export enum BarcodeKeys { + ACTIVE_SESSION = 'activeSession', + // Future keys can be added here +}apps/services/license-api/infra/license-api.ts (1)
46-47: Consider making barcode expiration times environment-specific.The barcode expiration times are currently hardcoded. Consider making them environment-specific like other configuration values to allow different timeouts across environments.
Apply this diff to make the values environment-specific:
- BARCODE_EXPIRE_TIME_IN_SEC: '60', - BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800', + BARCODE_EXPIRE_TIME_IN_SEC: { + dev: '60', + staging: '60', + prod: '60', + }, + BARCODE_SESSION_EXPIRE_TIME_IN_SEC: { + dev: '1800', + staging: '1800', + prod: '1800', + },apps/api/infra/api.ts (1)
282-283: Consider making expiration times configurable per environment.The hardcoded values for barcode and session expiration times could be made configurable based on the environment (dev/staging/prod) for better flexibility.
- BARCODE_EXPIRE_TIME_IN_SEC: '60', - BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800', + BARCODE_EXPIRE_TIME_IN_SEC: { + dev: '300', // 5 minutes for development + staging: '120', // 2 minutes for staging + prod: '60', // 1 minute for production + }, + BARCODE_SESSION_EXPIRE_TIME_IN_SEC: { + dev: '3600', // 1 hour for development + staging: '1800', // 30 minutes for staging + prod: '1800', // 30 minutes for production + },libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
474-476: Add input validation for session key generation.The method should validate that both parameters are non-empty strings to prevent generating invalid keys.
getBarcodeSessionKey(licenseType: LicenseType, sub: string) { + if (!licenseType || !sub) { + throw new Error('License type and sub are required for session key generation') + } return `${licenseType}-${sub}` }libs/shared/problem/src/problems.ts (1)
34-36: LGTM! Consider adding JSDoc comments.The
BadSessionProbleminterface is well-structured and follows the established pattern. Consider adding JSDoc comments to document the use case for this problem type, especially since it's in a shared library.+/** + * Represents a problem that occurs when a user's license is accessed on another device, + * invalidating the current session. + */ export interface BadSessionProblem extends BaseProblem { type: ProblemType.BAD_SESSION }charts/islandis-services/api/values.dev.yaml (1)
38-39: LGTM! Configuration is consistent with license-api service.The barcode expiration times are properly synchronized between the API and license-api services, ensuring consistent behavior across the system.
Consider using a shared configuration management system or documentation to maintain these values in sync across services.
charts/islandis-services/api/values.prod.yaml (1)
38-39: Environment variables look good, consider adding comments.The timeout values are reasonable and consistent with security best practices:
- 60 seconds for barcode expiration provides good security while maintaining usability
- 30 minutes for session expiration aligns with standard session management practices
Consider adding comments to document the purpose of these timeouts, for example:
+ # Time in seconds before a generated barcode expires BARCODE_EXPIRE_TIME_IN_SEC: '60' + # Time in seconds before a barcode session expires (e.g., when accessed from another device) BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800'charts/islandis-services/license-api/values.dev.yaml (1)
22-23: Consider moving timeout values to secrets managementWhile the values are appropriate for development, consider moving these timeout configurations to the secrets management system for better flexibility and consistency across environments. This would allow easier adjustment of these values without requiring code changes.
Example modification:
- BARCODE_EXPIRE_TIME_IN_SEC: '60' - BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800' + BARCODE_EXPIRE_TIME_IN_SEC: ${BARCODE_EXPIRE_TIME_IN_SEC} + BARCODE_SESSION_EXPIRE_TIME_IN_SEC: ${BARCODE_SESSION_EXPIRE_TIME_IN_SEC}Then add to the secrets section:
secrets: BARCODE_EXPIRE_TIME_IN_SEC: '/k8s/api/BARCODE_EXPIRE_TIME_IN_SEC' BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '/k8s/api/BARCODE_SESSION_EXPIRE_TIME_IN_SEC'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
apps/api/infra/api.ts(1 hunks)apps/native/app/src/messages/en.ts(1 hunks)apps/native/app/src/messages/is.ts(1 hunks)apps/native/app/src/screens/wallet-pass/wallet-pass.tsx(1 hunks)apps/native/app/src/ui/lib/card/license-card.tsx(5 hunks)apps/services/license-api/infra/license-api.ts(1 hunks)apps/services/license-api/src/app/modules/license/test/license.service.spec.ts(3 hunks)charts/islandis-services/api/values.dev.yaml(1 hunks)charts/islandis-services/api/values.prod.yaml(1 hunks)charts/islandis-services/api/values.staging.yaml(1 hunks)charts/islandis-services/license-api/values.dev.yaml(1 hunks)charts/islandis-services/license-api/values.prod.yaml(1 hunks)charts/islandis-services/license-api/values.staging.yaml(1 hunks)charts/islandis/values.dev.yaml(2 hunks)charts/islandis/values.prod.yaml(2 hunks)charts/islandis/values.staging.yaml(2 hunks)libs/api/domains/license-service/src/lib/licenseService.service.ts(4 hunks)libs/auth-nest-tools/src/lib/auth.ts(1 hunks)libs/auth-nest-tools/src/lib/current-actor.decorator.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.payload.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts(1 hunks)libs/auth-nest-tools/src/lib/jwt.strategy.ts(1 hunks)libs/services/license/src/index.ts(0 hunks)libs/services/license/src/lib/barcode.service.ts(4 hunks)libs/services/license/src/lib/license.config.ts(2 hunks)libs/shared/problem/src/Problem.ts(2 hunks)libs/shared/problem/src/ProblemType.ts(1 hunks)libs/shared/problem/src/problems.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/services/license/src/index.ts
✅ Files skipped from review due to trivial changes (1)
- libs/auth-nest-tools/src/lib/jwt.strategy.spec.ts
🧰 Additional context used
📓 Path-based instructions (17)
libs/shared/problem/src/ProblemType.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/jwt.payload.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/shared/problem/src/problems.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/auth.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/jwt.strategy.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/shared/problem/src/Problem.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/auth-nest-tools/src/lib/current-actor.decorator.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/native/app/src/messages/en.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/license-api/infra/license-api.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/messages/is.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/services/license-api/src/app/modules/license/test/license.service.spec.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/services/license/src/lib/license.config.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/api/infra/api.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/services/license/src/lib/barcode.service.ts (2)
Pattern libs/services/**/*: "Confirm that the code adheres to the following:
- Shared services and modules that can be utilized across NestJS apps.
- Proper implementation of NestJS providers, including custom providers.
- Adherence to NestJS module encapsulation and isolation principles."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/native/app/src/ui/lib/card/license-card.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (25)
libs/shared/problem/src/Problem.ts (1)
8-8: LGTM! Verify documentation exists.The addition of
BadSessionProblemto the union type follows TypeScript best practices and maintains type safety.Let's verify the documentation exists at the specified URL:
Also applies to: 18-18
✅ Verification successful
Documentation verified and accessible
The documentation for
BadSessionProblemis available at the expected URL, completing the verification process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the documentation for BAD_SESSION exists curl -s -I https://docs.devland.is/reference/problems/bad-session | head -n 1Length of output: 91
libs/auth-nest-tools/src/lib/auth.ts (1)
10-10: LGTM! Clean interface extension.The addition of the optional
sidproperty follows TypeScript best practices and maintains backward compatibility.libs/auth-nest-tools/src/lib/current-actor.decorator.ts (1)
28-28: LGTM! Verify type definition forsidThe addition of the
sidproperty is consistent with the authentication flow. However, ensure that thesidproperty is properly defined in theUsertype.✅ Verification successful
Type definition for
sidproperty is verifiedThe
sidproperty is properly typed as an optional string in the Auth interface which User extends. The implementation in current-actor.decorator.ts is type-safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if sid is properly typed in User interface ast-grep --pattern 'interface User { $$$ sid?: string $$$ }'Length of output: 68
Script:
#!/bin/bash # Find User interface/type definition ast-grep --pattern 'interface User { $$$ }' # Also check for type alias ast-grep --pattern 'type User = { $$$ }' # Backup search using ripgrep rg "interface User|type User" -A 10Length of output: 66973
Script:
#!/bin/bash # Search for User interface/type in auth-related files rg "interface User|type User" -A 5 "libs/auth" # Search for files importing User from auth-nest-tools rg "import.*User.*from.*auth-nest-tools"Length of output: 62332
Script:
#!/bin/bash # Search for User interface/type definition in auth-nest-tools rg "interface User|type User" -A 10 "libs/auth-nest-tools" # Search for files containing sid property definition rg "sid.*:.*string" "libs/auth-nest-tools"Length of output: 383
libs/services/license/src/lib/license.config.ts (1)
22-26: LGTM! Configuration values are properly definedThe default values are reasonable and follow the pattern of other configurations.
libs/auth-nest-tools/src/lib/jwt.strategy.ts (1)
58-58: LGTM! JWT payload properly extendedThe addition of
sidto the validated payload follows the established pattern and maintains type safety.libs/services/license/src/lib/barcode.service.ts (2)
7-7: LGTM! Import includes required typeThe addition of
Millisecondstype improves type safety for cache durations.
Line range hint
69-87: LGTM! Properly using configuration for expirationThe replacement of hardcoded values with configuration improves flexibility and maintainability.
apps/native/app/src/ui/lib/card/license-card.tsx (2)
11-11: LGTM! Improved error handling with Apollo error types.The change from boolean to ApolloError type enables more granular error handling.
Also applies to: 26-29
174-176: LGTM! Well-structured error message handling.The implementation correctly identifies BAD_SESSION errors and displays appropriate user feedback.
Also applies to: 309-311
apps/services/license-api/src/app/modules/license/test/license.service.spec.ts (2)
228-228: LGTM! Improved test maintainability.Using the configuration values instead of hardcoded constants makes the tests more maintainable and resilient to configuration changes.
Also applies to: 297-297
380-380: LGTM! Test correctly uses configuration value.The test now correctly uses the dynamic barcode expiration time from the configuration.
apps/native/app/src/screens/wallet-pass/wallet-pass.tsx (1)
404-404: LGTM! Improved error handling.Passing the full error object instead of a boolean enables more detailed error handling in the LicenseCard component.
libs/api/domains/license-service/src/lib/licenseService.service.ts (3)
478-500: LGTM! Well-structured session validation.The implementation includes proper error handling with ProblemError and informative logging.
511-515: LGTM! Proper session management integration.The changes correctly integrate session management into the barcode creation process.
557-559: LGTM! Atomic session operations.The implementation correctly uses Promise.all for atomic operations.
apps/native/app/src/messages/en.ts (1)
273-274: LGTM! Clear and user-friendly error message.The message effectively communicates the session issue to users without technical jargon.
apps/native/app/src/messages/is.ts (1)
409-410: LGTM! Accurate Icelandic translation.The translation maintains consistent messaging with the English version while being culturally appropriate.
charts/islandis-services/license-api/values.prod.yaml (1)
22-23: Document the expiration time values and verify their appropriateness.The expiration times are consistent across environments (prod/dev/staging):
BARCODE_EXPIRE_TIME_IN_SEC: '60'(1 minute)BARCODE_SESSION_EXPIRE_TIME_IN_SEC: '1800'(30 minutes)Please verify:
- Are these durations appropriate for your use case?
- Should the values differ between environments?
Consider documenting these values in a README or configuration guide to explain:
- Why these specific durations were chosen
- The impact of these timeouts on the user experience
- How to adjust these values if needed
charts/islandis-services/api/values.staging.yaml (1)
38-39: Values are correctly synchronized with production.The timeout values match the production configuration, ensuring consistent behavior across environments.
charts/islandis/values.staging.yaml (2)
289-290: LGTM! Reasonable timeout values for barcode handling.The barcode expiration (60s) and session expiration (1800s) values are appropriate for security while maintaining good user experience.
1440-1441: Consistent configuration across services.The barcode expiration settings are correctly mirrored in the license-api service, maintaining consistency with the api service configuration.
charts/islandis/values.prod.yaml (2)
277-278: LGTM! Reasonable timeout values for barcode management.The barcode expiration (60s) and session timeout (1800s) values provide a good balance between security and user experience:
- Short barcode lifetime prevents misuse
- 30-minute session gives users enough time to complete their tasks
1564-1565: Verify the timeout values match across services.The timeout values are correctly synchronized with the main API service, which is essential for consistent behavior.
Run this script to verify the values are consistent across all services:
✅ Verification successful
Timeout values are synchronized across all services and environments ✅
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any inconsistencies in barcode timeout values across services echo "Checking barcode timeout values across services..." rg -A1 "BARCODE_EXPIRE_TIME_IN_SEC:|BARCODE_SESSION_EXPIRE_TIME_IN_SEC:"Length of output: 3985
charts/islandis/values.dev.yaml (1)
1698-1699: LGTM! Added barcode expiration configuration.The new environment variables for barcode and session expiration times have been added correctly.
Please confirm if these timeout values are appropriate for production use:
- Barcode expires in 60 seconds (1 minute)
- Session expires in 1800 seconds (30 minutes)
These values seem reasonable, but it would be good to verify against any business requirements or user experience considerations.
charts/islandis-services/license-api/values.staging.yaml (1)
22-23: Review timeout values for staging environmentThe timeout values are currently identical to the development environment. Consider if the staging environment, being closer to production, should have different (potentially stricter) timeout values to better reflect production conditions.
Run this script to check timeout values across all environments:
| async getSessionCache(key: string): Promise<string | undefined> { | ||
| return this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for get operations
Consistent with the set operations, add error handling for cache retrieval.
async getSessionCache(key: string): Promise<string | undefined> {
- return this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`)
+ try {
+ return await this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`)
+ } catch (error) {
+ console.error('Failed to get session cache:', error)
+ return undefined
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getSessionCache(key: string): Promise<string | undefined> { | |
| return this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`) | |
| async getSessionCache(key: string): Promise<string | undefined> { | |
| try { | |
| return await this.cacheManager.get(`${BARCODE_ACTIVE_SESSION_KEY}:${key}`) | |
| } catch (error) { | |
| console.error('Failed to get session cache:', error) | |
| return undefined | |
| } | |
| } |
| async setSessionCache(key: string, value: string) { | ||
| return this.cacheManager.set( | ||
| `${BARCODE_ACTIVE_SESSION_KEY}:${key}`, | ||
| value, | ||
| this.config.barcodeSessionExpireTimeInSec * 1000, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for session cache operations
Similar to the previous comment, add error handling for session cache operations.
async setSessionCache(key: string, value: string) {
- return this.cacheManager.set(
- `${BARCODE_ACTIVE_SESSION_KEY}:${key}`,
- value,
- this.config.barcodeSessionExpireTimeInSec * 1000,
- )
+ try {
+ return await this.cacheManager.set(
+ `${BARCODE_ACTIVE_SESSION_KEY}:${key}`,
+ value,
+ this.config.barcodeSessionExpireTimeInSec * 1000,
+ )
+ } catch (error) {
+ console.error('Failed to set session cache:', error)
+ }
}📝 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.
| async setSessionCache(key: string, value: string) { | |
| return this.cacheManager.set( | |
| `${BARCODE_ACTIVE_SESSION_KEY}:${key}`, | |
| value, | |
| this.config.barcodeSessionExpireTimeInSec * 1000, | |
| ) | |
| } | |
| async setSessionCache(key: string, value: string) { | |
| try { | |
| return await this.cacheManager.set( | |
| `${BARCODE_ACTIVE_SESSION_KEY}:${key}`, | |
| value, | |
| this.config.barcodeSessionExpireTimeInSec * 1000, | |
| ) | |
| } catch (error) { | |
| console.error('Failed to set session cache:', error) | |
| } | |
| } |
| return this.cacheManager.set( | ||
| key, | ||
| value, | ||
| this.config.barcodeExpireTimeInSec * 1000, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for cache operations
The cache operation should handle potential errors from the cache manager.
async setCache<Type extends LicenseType>(
key: string,
value: BarcodeData<Type>,
) {
- return this.cacheManager.set(
- key,
- value,
- this.config.barcodeExpireTimeInSec * 1000,
- )
+ try {
+ return await this.cacheManager.set(
+ key,
+ value,
+ this.config.barcodeExpireTimeInSec * 1000,
+ )
+ } catch (error) {
+ // Log the error but don't throw to prevent breaking the flow
+ console.error('Failed to set cache:', error)
+ }
}📝 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.
| return this.cacheManager.set( | |
| key, | |
| value, | |
| this.config.barcodeExpireTimeInSec * 1000, | |
| ) | |
| } | |
| async setCache<Type extends LicenseType>( | |
| key: string, | |
| value: BarcodeData<Type>, | |
| ) { | |
| try { | |
| return await this.cacheManager.set( | |
| key, | |
| value, | |
| this.config.barcodeExpireTimeInSec * 1000, | |
| ) | |
| } catch (error) { | |
| // Log the error but don't throw to prevent breaking the flow | |
| console.error('Failed to set cache:', error) | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17603 +/- ##
==========================================
- Coverage 35.57% 35.56% -0.02%
==========================================
Files 7030 7030
Lines 150466 150492 +26
Branches 42959 42979 +20
==========================================
- Hits 53534 53528 -6
- Misses 96932 96964 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ❌ 8 Total Test Services: 1 Failed, 5 Passed Test Services
❌ Failed Tests (8)
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
libs/shared/problem/src/utils/findProblemInApolloError.ts (1)
Line range hint
28-29: Fix inconsistency in problem resolution.The final return statement doesn't handle the nested exception case, which could lead to inconsistent behavior.
- return graphQLError.extensions!.problem as Problem + const extensions = graphQLError.extensions as ProblemExtensions + return extensions?.problem || extensions?.exception?.problem
🧹 Nitpick comments (2)
libs/shared/problem/src/utils/findProblemInApolloError.ts (1)
19-20: Consider extracting problem resolution logic.The problem resolution logic could be extracted into a separate function for better maintainability and reusability.
+ const resolveProblem = (extensions: ProblemExtensions | undefined): Problem | undefined => { + return extensions?.problem || extensions?.exception?.problem + } + const graphQLError = error.graphQLErrors.find((value) => { const extensions = value.extensions as ProblemExtensions | undefined - const problem = extensions?.problem || extensions?.exception?.problem + const problem = resolveProblem(extensions) return problem && (!types || types.includes(problem.type)) })libs/shared/problem/src/utils/findProblemInApolloError.spec.ts (1)
110-114: Consider adding edge case tests.While the current test coverage is good, consider adding tests for edge cases:
- Null/undefined exception object
- Empty exception object
- Multiple levels of nesting
Example test case:
it('handles null/undefined exception objects', () => { const error = new ApolloError({ graphQLErrors: [ new GraphQLError('Error', null, null, null, null, null, { exception: null, }), ], }) expect(findProblemInApolloError(error)).toBeUndefined() })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/shared/problem/src/utils/findProblemInApolloError.spec.ts(1 hunks)libs/shared/problem/src/utils/findProblemInApolloError.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/shared/problem/src/utils/findProblemInApolloError.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/shared/problem/src/utils/findProblemInApolloError.spec.ts (2)
Pattern libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/shared/problem/src/utils/findProblemInApolloError.ts (1)
4-9: Well-structured interface definition!The
ProblemExtensionsinterface effectively models the API response structure, allowing for both direct problem access and nested problems within exceptions. This aligns well with TypeScript best practices and the PR objectives.libs/shared/problem/src/utils/findProblemInApolloError.spec.ts (1)
77-101: Excellent test coverage for nested problems!The test case is well-structured, following the AAA pattern and effectively verifying the handling of problems nested within exception objects.
|
Closing since I extracted only my commits to a separate PR to make it more readable for now since the other one has not been merged to main. See #17623 |
What
Based on top of #17541.
If I get BAD_SESSION error when fetching barcode I show an error message that states that it is too short since license was accessed on another device.
There is an error that we need to fix somehow. @eirikurn
The error I get from the api is on the form:
but the
findProblemInApolloErrorfunction wants it to be on the following form (excluding the exception nesting):So currently I never match the error because of this.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Localization
The changes focus on improving security and user experience when accessing digital licenses and wallet passes.