Update to latest @dtdot/eslint-config #150
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several configuration and code improvements across the project. The changes focus on updating linting and formatting configurations, enhancing error tracking with Sentry, refining service implementations, and introducing a new caching mechanism. The modifications span multiple files, including ESLint and Prettier configurations, package dependencies, and various service implementations. The updates aim to improve code quality, maintainability, and performance through more precise configurations and refined service logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UserService
participant UserPersistence
participant Cache
Client->>UserService: getUsers(first, sortedBy)
UserService->>Cache: Check cached results
alt Cache hit
Cache-->>UserService: Return cached users
else Cache miss
UserService->>UserPersistence: Retrieve users
UserPersistence-->>UserService: Return users
UserService->>Cache: Store users in cache
end
UserService-->>Client: Return users
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/auth/authorisation.service.ts (1)
Line range hint
6-11: Enhance error handling and add defensive checksWhile the implementation is functional, consider these improvements for robustness:
- Include the required role in the error message for better debugging
- Add null/undefined checks for context.roles
Consider applying these changes:
requireUserRole(context: QuizlordContext, role: Role) { + if (!context?.roles) { + throw new UnauthorisedError('User roles are not available'); + } if (!context.roles.includes(role)) { - throw new UnauthorisedError('You are not authorised to perform this action'); + throw new UnauthorisedError(`This action requires the ${role} role`); } }
🧹 Nitpick comments (7)
src/quiz/quiz.service.test.ts (2)
Line range hint
14-29: Consider enhancing mock organization.Consider extracting the mock implementations into a separate test helpers file to improve maintainability and reusability.
+ // Move to __test__/helpers/mocks.ts + export const createMockPersistence = () => ({ + createQuizWithImages: jest.fn(), + getCompletionScoreWithQuizTypesForUser: jest.fn(), + getQuizzesWithUserResults: jest.fn(), + getQuizByIdWithResults: jest.fn(), + getRecentQuizCompletions: jest.fn(), + getRecentQuizUploads: jest.fn(), + });
Line range hint
37-41: Consider adding test lifecycle management.Consider adding a common beforeAll setup and afterAll cleanup to manage test lifecycle consistently.
+ beforeAll(() => { + // Common test setup + }); + + afterAll(() => { + // Common test cleanup + });src/service.locator.ts (1)
Line range hint
1-41: Consider dependency injection instead of service locator patternWhile the service locator pattern works, consider migrating to dependency injection for better testability and maintainability. The current pattern makes dependencies implicit and harder to mock in tests.
Benefits of switching to DI:
- Explicit dependencies
- Easier unit testing
- Better compile-time checking
- More maintainable codebase
Would you like assistance in planning this architectural improvement?
src/auth/authorisation.service.ts (1)
Line range hint
6-11: Consider caching role checks for performanceFor high-traffic applications, consider implementing a caching strategy for role checks, especially if roles are fetched from a database. This could be particularly important if this method is called frequently within a request lifecycle.
src/activity/activity.gql.ts (1)
Line range hint
12-15: Consider adding error handling to the resolverThe resolver should handle potential errors from the DataLoader to prevent unhandled promise rejections from propagating to the GraphQL layer.
async function activityFeedUser(parent: RecentActivityItem, _params: Record<string, never>, _context: QuizlordContext) { - return usersForActivityLoader.load(parent); + try { + return await usersForActivityLoader.load(parent); + } catch (error) { + console.error('Failed to load users for activity:', error); + throw new Error('Failed to load users for activity'); + } }src/user/user.service.ts (2)
Line range hint
52-89: Consider simplifying the overloaded method signaturesThe current implementation uses method overloading to handle different sorting scenarios, which adds complexity to the codebase. Consider using a single method with an options object pattern for better maintainability.
interface GetUsersOptions { first: number; sortedBy: UserSortOption; currentUserId?: string; afterId?: string; } async getUsers(options: GetUsersOptions): Promise<GetUsersResult>
Line range hint
155-186: Well-structured batch processing implementationThe implementation of
getUsersForActivitiesis well-structured with clear separation of concerns:
- Efficient filtering of activity items by type
- Parallel processing of quiz completions and uploads
- Proper type transformations
However, consider adding error handling for the persistence layer calls.
async getUsersForActivities(activityItems: readonly RecentActivityItem[]): Promise<Record<string, User[]>> { + try { const quizCompletionActivityItems = activityItems.filter( (activityItem) => activityItem.actionType === 'QUIZ_COMPLETED', ); // ... rest of the implementation ... return { ...quizCompletionUserMap, ...quizUploadUserMap, }; + } catch (error) { + console.error('Failed to fetch users for activities:', error); + throw new Error('Failed to fetch users for activities'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.eslintrc.js(0 hunks).prettierrc.js(0 hunks)eslint.config.js(1 hunks)package.json(2 hunks)prettier.config.js(1 hunks)src/activity/activity.gql.ts(1 hunks)src/auth/authorisation.service.ts(1 hunks)src/index.ts(1 hunks)src/instrument.ts(1 hunks)src/quiz/quiz.gql.ts(1 hunks)src/quiz/quiz.persistence.ts(1 hunks)src/quiz/quiz.service.test.ts(1 hunks)src/quiz/quiz.service.ts(3 hunks)src/service.locator.ts(1 hunks)src/statistics/statistics.service.test.ts(1 hunks)src/user/user.gql.ts(1 hunks)src/user/user.persistence.ts(1 hunks)src/user/user.service.test.ts(1 hunks)src/user/user.service.ts(1 hunks)src/util/cache.ts(0 hunks)
💤 Files with no reviewable changes (3)
- .prettierrc.js
- .eslintrc.js
- src/util/cache.ts
✅ Files skipped from review due to trivial changes (7)
- prettier.config.js
- src/instrument.ts
- eslint.config.js
- src/user/user.gql.ts
- src/quiz/quiz.gql.ts
- src/user/user.persistence.ts
- src/user/user.service.test.ts
🔇 Additional comments (16)
src/quiz/quiz.persistence.ts (1)
6-6: LGTM! Clean integration of QuizFilters type.
The addition of QuizFilters import enhances type safety and maintains proper separation of concerns.
src/quiz/quiz.service.ts (3)
10-10: LGTM! Clean import organization.
The import statements are well-organized and properly grouped.
Also applies to: 14-14
Line range hint 63-73: LGTM! Improved object destructuring pattern.
The destructuring pattern enhances code readability by clearly separating fields that require transformation.
Line range hint 300-309: LGTM! Consistent destructuring pattern.
The destructuring pattern in #quizPersistenceWithMyCompletionsToQuiz matches the pattern used in getQuizDetails, maintaining consistency.
src/quiz/quiz.service.test.ts (1)
1-8: LGTM! Clean import organization and proper mocking setup.
The imports and mock setup provide a solid foundation for testing.
src/service.locator.ts (3)
4-4: LGTM: Config import addition aligns with centralization effort
The addition of the config import aligns with the broader effort to centralize configuration management across the application.
Line range hint 18-19: Verify AUTH0 configuration handling
The authentication service is now using centralized config values, which is good. However, we should verify that these sensitive AUTH0 configurations are properly handled in the config module.
✅ Verification successful
Let me check how these configurations are actually loaded and validated by looking at the config module's implementation.
AUTH0 configuration is properly validated and handled ✅
The config module implements proper validation for AUTH0_DOMAIN and AUTH0_AUDIENCE:
- Both fields are marked as required
- AUTH0_AUDIENCE specifically requires a valid URI format
- The validation is enforced at startup with process termination on failure
- Values are loaded from environment variables and validated against a strict schema
- Unknown fields are allowed but stripped from the final config object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how AUTH0 configs are loaded/handled
rg -A 5 "AUTH0_(DOMAIN|AUDIENCE)" src/config/
Length of output: 909
Script:
#!/bin/bash
# Check the config module implementation for loading and validation
cat src/config/config.ts
Length of output: 1773
Line range hint 24-24: Verify AWS configuration handling
The S3FileService is using multiple AWS-related configurations. Let's verify these are properly handled and that sensitive credentials are managed securely.
✅ Verification successful
Let me gather more information about AWS configuration and credential handling.
Based on the gathered information, I can now provide the final response:
AWS configuration is properly handled with secure practices
The implementation follows secure practices for AWS configuration:
- AWS credentials are managed through the default AWS SDK credential chain (no hardcoded credentials)
- Configuration is properly validated using Joi schema in config.ts
- S3Client is initialized with only the required region parameter
- Sensitive operations use temporary credentials via presigned URLs with a 1-hour expiration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check AWS config handling
rg -A 5 "AWS_(REGION|BUCKET_NAME)" src/config/
# Check for any AWS credential handling
rg -i "aws.*credential" src/
Length of output: 931
Script:
#!/bin/bash
# Check for AWS SDK initialization and credential configuration
rg -A 5 "new AWS\." src/
rg -A 5 "AWS\.config" src/
# Check for environment variable loading related to AWS
rg -A 5 "process\.env\.AWS" src/
# Look for credential configuration in S3FileService
ast-grep --pattern 'class S3FileService {
$$$
}'
Length of output: 2063
src/auth/authorisation.service.ts (2)
1-4: LGTM! Well-organized imports.
The imports follow a clean separation between external dependencies and internal modules.
Line range hint 6-11: Verify consistent usage of role validation
Let's ensure this method is being used consistently across all authenticated endpoints.
✅ Verification successful
Based on the results, I can now generate the final response:
Role validation is consistently implemented across all GraphQL resolvers
The requireUserRole check is properly implemented across all GraphQL resolvers in the codebase:
- User resolvers in
user.gql.ts - Quiz resolvers in
quiz.gql.ts(create, complete, mark illegible, etc.) - Statistics resolver in
statistics.gql.ts - Activity resolver in
activity.gql.ts
All endpoints consistently validate the 'USER' role before proceeding with their operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all GraphQL resolvers and verify role checks
# Look for resolver decorators and check if requireUserRole is used
# Search for GraphQL resolver methods
ast-grep --pattern 'class $_ {
$$$
@Resolver($$$)
async $_($$$) {
$$$
}
$$$
}'
# Search for requireUserRole usage
rg -A 3 'requireUserRole'
Length of output: 3357
package.json (2)
14-14: LGTM: Lint script scope refinement
The lint script now specifically targets the ./src directory instead of the entire project, which is more efficient and aligns with common practices.
52-52: Verify compatibility with the new major versions
The updates include significant version changes:
- @dtdot/eslint-config: ^1.0.1 (major)
- eslint: ^8.57.1 (minor)
- prettier: ^3.4.2 (minor)
Also applies to: 58-58, 61-61
✅ Verification successful
Let me try to gather more specific information about the package dependencies and their usage.
Let me check the actual ESLint and Prettier configurations to understand how they're being used.
ESLint and Prettier configurations are minimal and follow recommended presets
The project uses a simple configuration that extends @dtdot/eslint-config recommended presets without any custom overrides. The version bump should be safe as:
- The ESLint config only uses the recommended preset with no custom rules
- The Prettier config also uses the recommended preset without modifications
- Both tools are only used for development/linting and don't affect runtime behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in the updated packages
# Check the changelog for @dtdot/eslint-config
gh api repos/dtdot/eslint-config/releases | jq '.[0].body'
# Check ESLint migration guide for version 8
rg -A 5 "migration|upgrade|breaking" $(fd -t f . | grep -i changelog | grep -i eslint)
Length of output: 1681
Script:
#!/bin/bash
# Check current ESLint configuration and rules being used
fd -t f "\.eslintrc" -X cat
# Check for ESLint and Prettier related configurations in package.json
rg -A 5 "eslint|prettier" package.json
# Look for any ESLint or Prettier specific configurations or extends
fd -t f "\.js|\.json" -X rg -l "dtdot/eslint-config|eslintConfig|prettier"
Length of output: 827
Script:
#!/bin/bash
# Check the content of ESLint and Prettier config files
echo "=== ESLint Config ==="
cat eslint.config.js
echo -e "\n=== Prettier Config ==="
cat prettier.config.js
# Check for any existing ESLint errors in the codebase
echo -e "\n=== Current ESLint Issues ==="
npx eslint ./src --max-warnings 0 || true
Length of output: 758
src/index.ts (1)
4-4: LGTM: Import organization and Sentry integration
The imports have been reorganized and grouped logically, improving code organization. The addition of Sentry for error tracking is a good practice for production monitoring.
Also applies to: 11-11, 14-14, 16-16, 18-18, 19-19
src/statistics/statistics.service.test.ts (1)
3-5: LGTM: Import organization
The imports have been reordered to follow the new ESLint configuration rules.
src/activity/activity.gql.ts (1)
Line range hint 27-35: LGTM! Excellent performance optimization using DataLoader
The implementation of DataLoader for batching user requests is a great performance improvement. It will help reduce the N+1 query problem by batching multiple individual user requests into a single call.
A few positive observations:
- Proper use of readonly for the input parameter
- Clear mapping of results back to individual requests
- Handling of missing users by returning empty arrays
src/user/user.service.ts (1)
Line range hint 155-186: Verify the performance impact of Object.fromEntries transformations
The implementation uses multiple Object.fromEntries and Object.entries transformations. For large datasets, this could impact performance.
✅ Verification successful
Let me gather more context about the data flow and usage patterns to better assess the performance implications.
Let me try a different approach to understand the usage patterns and data flow.
Based on the gathered context, I can now provide a final response about the performance implications of the Object.fromEntries transformations.
Object transformations are acceptable for this use case
The implementation's use of Object.fromEntries and Object.entries is justified because:
- The data volume is naturally limited as it processes recent activity items
- The persistence layer shows that quiz completions and uploads are fetched with limits (see
getRecentQuizCompletionsandgetRecentQuizUploads) - The transformations are necessary to map persistence models to domain models while preserving the activity item IDs as keys
The performance impact is negligible since:
- The data structure is shallow (single-level object transformation)
- The operation is performed on an already filtered subset of activity items
- The transformation maintains necessary mapping between activity IDs and their associated users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in the codebase to identify potential performance bottlenecks
rg -A 2 "Object\.fromEntries\("
Length of output: 501
Script:
#!/bin/bash
# Check the persistence layer implementation to understand data volume
ast-grep --pattern 'getUsersForQuizCompletions($_) {
$$$
}'
# Check for any test files that might indicate data volume
rg -l "getUsersForActivities.*test"
# Look for any configuration or constants that might indicate expected data volumes
rg "MAX_.*QUIZ|LIMIT.*QUIZ"
Length of output: 131
Script:
#!/bin/bash
# Look for test files related to user service
fd -e test.ts -e spec.ts user.service
# Check for activity-related interfaces or types to understand data structure
rg -A 5 "interface.*Activity|type.*Activity"
# Look for persistence layer implementation
fd persistence.ts -x cat {}
Length of output: 14710
danielemery
force-pushed
the
95-update-to-100-release-of-dtdoteslint-config
branch
from
fc61945 to
08b832a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
=======================================
Coverage 34.16% 34.16%
=======================================
Files 26 26
Lines 600 600
Branches 87 93 +6
=======================================
Hits 205 205
Misses 395 395 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Cacheinterface and aMemoryCacheclass for efficient data storage and retrieval.AuthorisationServicefor role-based user authorization.Bug Fixes
UserService.Documentation
Tests
QuizServiceandUserServiceto validate various scenarios and error handling.Chores
package.jsonfor improved performance and security.