Implement comments feature for articles #134
Conversation
WalkthroughThe changes introduce new database schemas for managing articles, favorites, and comments. New tables and relationships are defined, along with comprehensive schema definitions and types for inserting, updating, and returning article and comment data. The articles module integrates a CommentsRepository and CommentsService, while the articles plugin defines several RESTful routes for article and comment operations. Additionally, updates to the profiles repository and service add methods for retrieving profiles by user ID, enhancing overall article management and comment functionalities. Changes
Sequence Diagram(s)Comment Creation FlowsequenceDiagram
participant U as User
participant AP as Articles Plugin
participant CS as Comments Service
participant CR as Comments Repository
participant DB as Database
U->>AP: POST /articles/:slug/comments (with comment data)
AP->>CS: createComment(articleSlug, commentBody, userId)
CS->>CR: create(commentData)
CR->>DB: INSERT comment record
DB-->>CR: Insertion confirmation
CR-->>CS: New comment data
CS-->>AP: ReturnedComment with author details
AP-->>U: Response with created comment
Comment Deletion FlowsequenceDiagram
participant U as User
participant AP as Articles Plugin
participant CS as Comments Service
participant CR as Comments Repository
participant DB as Database
U->>AP: DELETE /articles/:slug/comments/:id
AP->>CS: deleteComment(articleSlug, commentId, userId)
CS->>CR: delete(commentId, userId)
CR->>DB: DELETE comment WHERE id = commentId AND authorId = userId
DB-->>CR: Deletion confirmation
CR-->>CS: Deletion status
CS-->>AP: Success response
AP-->>U: Comment deletion response
Poem
✨ Finishing Touches
🪧 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 review |
✅ Actions performedReview triggered.
|
![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 (7)
src/articles/comments/comments.schema.ts (1)
32-32:DeleteCommentResponsestub is ready.
You may consider including a success/message field if the client needs explicit confirmation beyond HTTP status codes.src/articles/comments/comments.service.ts (1)
114-120:getAuthorUsernameis a clear helper.
ThrowingBadRequestErrorwhen the user is missing might be better expressed asNotFoundErrorfor clarity, if your error semantics differentiate. Otherwise, it is acceptable.- throw new BadRequestError(\`User with id \${userId} not found\`); + throw new NotFoundError(\`User with id \${userId} not found\`);src/articles/articles.plugin.ts (2)
145-166: Consider handling non-existent article case.The GET route implementation is correctly fetching comments with proper parameter typing. Similar to the POST route, consider adding error handling for cases where the article doesn't exist.
Also, the ternary expression to handle the userId could be simplified using optional chaining:
return { comments: await store.commentsService.getComments( params.slug, - userId === null ? undefined : userId, + userId ?? undefined, ), };
168-188: Add validation for numeric ID.While
Number.parseInthandles conversion from string to number, it doesn't validate that the result is a valid number. Consider adding explicit validation to handle cases where the ID cannot be properly parsed.async ({ params, store, request }) => { + const commentId = Number.parseInt(params.id, 10); + if (Number.isNaN(commentId)) { + throw new Error('Invalid comment ID'); + } await store.commentsService.deleteComment( params.slug, - Number.parseInt(params.id, 10), + commentId, await store.authService.getUserIdFromHeader(request.headers), ); return {}; },src/articles/articles.schema.ts (1)
32-49: Consider using a more specific type for author.The schema uses
Type.Any()for the author field, which reduces type safety. Consider using a more specific type definition for better validation and IDE support.Type.Object({ - author: Type.Any(), + author: Type.Object({ + username: Type.String(), + bio: Type.Optional(Type.String()), + image: Type.Optional(Type.String()), + following: Type.Boolean(), + }), }),src/articles/comments/comments.repository.ts (2)
10-17: Add error handling to createComment method.The method looks good but doesn't handle potential insertion errors. Consider adding try/catch to provide more meaningful error messages.
async createComment(commentData: CommentToCreate) { + try { const [comment] = await this.db .insert(comments) .values(commentData) .returning(); return comment; + } catch (error) { + console.error('Failed to create comment:', error); + throw new Error('Failed to create comment'); + } }
38-45: Consider moving getArticleBySlug to an articles repository.This method seems out of place in a comments repository. Consider moving it to an articles repository and injecting that dependency instead, to maintain separation of concerns.
Also, add handling for when the article is not found:
async getArticleBySlug(slug: string) { const [article] = await this.db .select() .from(articles) .where(eq(articles.slug, slug)); + if (!article) { + throw new Error(`Article with slug ${slug} not found`); + } + return article; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/articles/articles.model.ts(2 hunks)src/articles/articles.module.ts(3 hunks)src/articles/articles.plugin.ts(3 hunks)src/articles/articles.schema.ts(3 hunks)src/articles/comments/comments.repository.ts(1 hunks)src/articles/comments/comments.schema.ts(1 hunks)src/articles/comments/comments.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
src/articles/articles.module.ts (2)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(7:52)src/articles/comments/comments.service.ts (1) (1)
CommentsService(7:121)
src/articles/articles.model.ts (1)
src/users/users.model.ts (1) (1)
users(12:23)
src/articles/comments/comments.repository.ts (2)
src/articles/comments/comments.schema.ts (1) (1)
CommentToCreate(9:12)src/articles/articles.model.ts (2) (2)
comments(73:84)articles(14:26)
src/articles/articles.schema.ts (1)
src/articles/articles.model.ts (1) (1)
favoriteArticles(42:55)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(7:52)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(22:22)CommentToCreate(9:12)src/articles/articles.model.ts (1) (1)
comments(73:84)
src/articles/articles.plugin.ts (1)
src/articles/comments/comments.schema.ts (4) (4)
AddCommentSchema(3:7)ReturnedCommentResponse(24:26)ReturnedCommentsResponse(28:30)DeleteCommentResponse(32:32)
🔇 Additional comments (31)
src/articles/articles.module.ts (4)
1-2: Imports look good.
The new imports ensure theCommentsRepositoryandCommentsServiceare available. Make sure that the path aliases (@/articles/...) are properly configured in yourtsconfig.json.
25-29: Service instantiation is consistent.
CommentsServicecorrectly depends oncommentsRepository,profilesService, andusersRepository. This ensures it can manage comments and retrieve author profiles as needed.
31-35: New comments service state is integrated correctly.
ProvidingcommentsServicein the Elysia state ensures it’s accessible throughout the module. This aligns with the existing pattern forarticlesServiceandauthService.
14-14:❓ Verification inconclusive
Repository instantiation is correct.
Using the existingdbinstance to createcommentsRepositoryis consistent with the pattern used for other repositories. Confirm that the migrations for the comments table are already applied.
🏁 Script executed:
#!/bin/bash # Verify that the 'comments' table exists in the database schema # using a relevant DB introspection query or migration check. # (Assuming there's a CLI to run migrations or introspect the schema.) npm run db:migrate:statusLength of output: 261
Attention: Manual Verification Needed for Migration Check
The instantiation of
commentsRepositoryusing the existingdbinstance is correct and follows the repository pattern used elsewhere. However, the automated check for the comments table migration failed because the scriptdb:migrate:statusisn’t defined in the package scripts. Please manually verify that the migrations for the comments table have been applied or update the migration command in the repository’s configuration.
- Confirm manually (or with an alternative script) that the comments table exists.
- Consider adding or correcting the migration status script in the package.json.
src/articles/articles.model.ts (4)
37-39: Newcommentsrelation aligns with the table definition.
Declaringcomments: many(comments)underarticleRelationsestablishes a clear one-to-many relationship between articles and comments. Ensure that therelationName: 'articleComments'is referenced consistently in the codebase.
72-72: No substantive change.
This is a blank line insertion.
73-84: Definition of thecommentstable appears accurate.
The schema matches standard patterns of referencingarticles.idandusers.id. Timestamps defaulting toCURRENT_TIMESTAMPis consistent with the existing approach in other tables.
86-97:commentRelationsis well-defined.
The one-to-many relationships toarticlesandusersare appropriately declared usingfieldsandreferences. This ensures that model relationships for comments are properly maintained.src/articles/comments/comments.schema.ts (7)
1-2: Import statements are consistent with TypeBox usage.
Havingtype Static, Typefrom@sinclair/typeboxis standard for defining schemas.
3-7: Structured “comment” property inAddCommentSchema.
Nesting thebodyfield undercommentmatches typical RealWorld style payloads. No issues noted.
9-12:CommentToCreatemerges schema props with DB fields.
Combining the user-suppliedbodywithauthorIdandarticleIdis clear and straightforward.
14-20:CommentSchemadefinition is comprehensive.
Capturingid,body, timestamps, plus anauthorfield of typeType.Any()provides flexibility for nested author data.
22-22:ReturnedCommentcorrectness.
ExportingReturnedCommentbased onCommentSchemaenables type-safe contract for returned data.
24-26:ReturnedCommentResponsealigns with typical REST responses.
Wrapping the comment in a top-levelcommentobject is consistent with standard patterns.
28-30:ReturnedCommentsResponsefor multiple comments.
The array structure is appropriate for listing multiple comments in a consistent format.src/articles/comments/comments.service.ts (6)
1-3: Imports for errors, profiles, and users are correct.
These references align with the constructor dependencies.
4-5:comments.repositoryandcomments.schemausage.
Properly typed references ensure stronger type safety when creating and returning comments.
7-12: Constructor is logically consistent.
AcceptingcommentsRepository,profilesService, andusersRepositorycentralizes all needed integrations.
14-45:createCommenthandles validation and creation well.
- Checks whether the article exists.
- Correctly merges user input with
authorIdandarticleId.- Fetches and attaches the author’s profile.
- Returns the comment data in the expected format.
47-80:getCommentsreturns an enriched list of comments.
The function retrieves comments, fetches each author’s profile, and formats date fields to ISO strings. This approach is solid and meets typical RealWorld-style expectations.
82-112:deleteCommentboundaries are well-enforced.
- Ensures the article exists.
- Validates the target comment and ownership via
comment.authorId.- Invokes the repository deletion upon confirmation.
src/articles/articles.plugin.ts (2)
11-17: Good job with the proper imports.The necessary schema imports for the new comment functionality have been correctly set up.
75-75: Fixed typo in summary.Good catch on fixing "Artifle Feed" to "Article Feed".
src/articles/articles.schema.ts (5)
1-1: Good use of absolute import path.Using absolute import paths improves maintainability and makes refactoring easier.
30-30: Updated type definition matches schema changes.The simplified type definition correctly aligns with the changes to UpdateArticleSchema.
55-55: Good addition of DeleteArticleResponse.Adding a specific response type for delete operations is good practice, even if it's just an empty object.
67-67: Good export of ArticleFavoritedBy type.Making this type exported improves reusability across the codebase.
26-26:✅ Verification successful
Verify the removal of tagList from UpdateArticleSchema.
The schema now only allows updating title, description, and body, but no longer includes tagList. Make sure this is intentional and aligns with your business requirements.
🏁 Script executed:
#!/bin/bash # Check if tagList updates are handled elsewhere or if this is an intentional change # Look for any tagList updates in article services rg -A 5 -B 5 "tagList.*update" --type tsLength of output: 42
🏁 Script executed:
#!/bin/bash # Look for any occurrence of "tagList" in the articles.schema.ts file to verify its complete removal rg "tagList" src/articles/articles.schema.ts # Additionally, search for "tagList" across all TypeScript files in the repository rg "tagList" --type tsLength of output: 666
Update Schema Clarification:
The removal oftagListfrom theUpdateArticleSchemais confirmed as intentional. The update schema is defined usingType.Pick(insertArticleSchemaRaw, ['title', 'description', 'body']), meaning that only the three fields oftitle,description, andbodycan be updated. WhiletagListremains defined in other parts of the schema (used for article creation and stored in the model), its exclusion from the update schema aligns with the business requirements.src/articles/comments/comments.repository.ts (3)
1-6: Good imports and organization.The imports are well organized with clear separation between external libraries and internal modules.
7-9: Good repository pattern implementation.The class follows the repository pattern with dependency injection, which makes testing easier.
28-36: Good implementation of getCommentsByArticleId.The method correctly retrieves comments for an article and sorts them by creation date in descending order.
…atabase operation
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/articles/comments/comments.service.ts (2)
14-45: 🛠️ Refactor suggestionOptimize database queries when retrieving author information
The current implementation makes two separate database queries to get author information:
- First fetching the username with
getAuthorUsername()- Then fetching the profile with
profilesService.findByUsername()This could be optimized to reduce database load.
Consider refactoring the ProfilesService to include a
findByIdmethod that directly accepts a user ID:- const authorUsername = await this.getAuthorUsername(comment.authorId); - const authorProfile = await this.profilesService.findByUsername( - userId, - authorUsername, - ); + const authorProfile = await this.profilesService.findById(userId, comment.authorId);
47-80: 🛠️ Refactor suggestionPerformance concern: N+1 query problem in comment retrieval
For each comment, you're making two separate database queries:
- Getting the username for each comment author
- Getting the profile information for each author
This results in 2N+1 queries for N comments, which can create significant performance issues as the number of comments grows.
Consider refactoring to batch these operations:
const comments = await this.commentsRepository.findManyByArticleId(article.id); + + // Get unique author IDs + const authorIds = [...new Set(comments.map(comment => comment.authorId))]; + + // Fetch all required profiles in one or two queries + const authorProfiles = await this.profilesService.findManyByIds(currentUserId || 0, authorIds); const returnedComments = await Promise.all( comments.map(async (comment) => { - const authorUsername = await this.getAuthorUsername(comment.authorId); - const authorProfile = await this.profilesService.findByUsername( - currentUserId || 0, - authorUsername, - ); + const authorProfile = authorProfiles.find(profile => profile.user.id === comment.authorId); return { id: comment.id, body: comment.body, createdAt: comment.createdAt, updatedAt: comment.updatedAt, author: authorProfile.profile, }; }), );
🧹 Nitpick comments (1)
src/articles/comments/comments.service.ts (1)
114-120: Consider caching user information for repeated lookupsThe
getAuthorUsernamemethod is called multiple times throughout the service. If user information rarely changes during a request lifecycle, consider implementing a simple in-memory cache to avoid redundant database queries.+ private userCache: Map<number, string> = new Map(); private async getAuthorUsername(userId: number): Promise<string> { + if (this.userCache.has(userId)) { + return this.userCache.get(userId)!; + } const user = await this.usersRepository.findById(userId); if (!user) { throw new BadRequestError(`User with id ${userId} not found`); } + this.userCache.set(userId, user.username); return user.username; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/articles/comments/comments.schema.ts(1 hunks)src/articles/comments/comments.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/articles/comments/comments.schema.ts (1)
src/articles/articles.model.ts (1) (1)
comments(73-84)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(7-46)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(38-38)CommentToCreate(15-18)src/articles/articles.model.ts (1) (1)
comments(73-84)
🔇 Additional comments (6)
src/articles/comments/comments.service.ts (2)
1-12: Well-structured class design with appropriate dependenciesThe service class is well-organized with proper dependency injection through the constructor. All dependencies are correctly typed and marked as private readonly, following good TypeScript practices.
82-112: Robust validation in the delete operationThe delete operation includes comprehensive validation:
- Verifies the article exists
- Verifies the comment exists
- Confirms the comment belongs to the specified article
- Ensures the user is authorized (is the comment author)
This thorough approach provides proper security and data integrity.
src/articles/comments/comments.schema.ts (4)
1-8: Good use of schema generation utilitiesThe code properly leverages TypeBox and Drizzle-TypeBox to generate base schemas from the database model, which ensures type consistency between the database and API layers.
9-18: Well-designed input schema and database typeThe
AddCommentSchemacorrectly models the client input (just the comment body), whileCommentToCreateextends it with server-side information needed for storage. This separation of concerns is a good practice that keeps the API clean and the implementation flexible.
20-38: Security-conscious response schema designThe
ReturnedCommentSchemaproperly omits sensitive internal fields (articleId,authorId) and carefully selects which user information to expose, excluding sensitive data like passwords and emails. The inclusion of thefollowingproperty is useful for the UI to determine relationship status.
40-48: Consistent response schema patternThe response schemas follow a consistent pattern of wrapping data in named properties (
commentorcomments), which is good for API consistency and future extensibility. The emptyDeleteCommentResponseis appropriate for operations that only need to signal success.
- Updated CommentsService to use authorId directly when fetching author profile. - Introduced findByUserId method in ProfilesRepository to retrieve user profiles by their ID. - Enhanced ProfilesService with findByUserId method for better error handling and profile response generation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/articles/comments/comments.service.ts (1)
56-76: 🛠️ Refactor suggestionConsider optimizing comment retrieval to reduce database queries.
The current implementation makes a separate database query for each comment to retrieve the author username and profile. This could become inefficient with a large number of comments.
Consider refactoring this to use a more efficient query approach:
- Fetch all comments with author IDs
- Get all unique author IDs
- Batch retrieve author profiles in a single query
- Map the profiles back to comments
This would reduce the number of database queries from O(n) to O(1) where n is the number of comments.
🧹 Nitpick comments (4)
src/profiles/profiles.repository.ts (1)
19-28: Optimization suggestion: Consider usingfindFirstinstead offindManywhen querying by primary key.The implementation is correct, but since you're querying by the primary key (
users.id), which should return at most one result, usingfindFirstwould be more semantically appropriate and potentially more efficient than usingfindManyand then extracting the first element.async findByUserId(targetUserId: number) { - const result = await this.db.query.users.findMany({ + const result = await this.db.query.users.findFirst({ where: eq(users.id, targetUserId), with: { followers: true }, }); - if (result.length === 0) { + if (!result) { return null; } - return result[0]; + return result; }src/articles/comments/comments.service.ts (3)
31-43: Simplify author profile retrieval.When creating a comment, you're passing the same user ID twice to
findByUserId, which is redundant. Since the comment author is the current user, you can simplify this code.const comment = await this.commentsRepository.create(commentData); const authorProfile = await this.profilesService.findByUserId( userId, - comment.authorId, + userId, );
21-23: Standardize error types for consistency.The service uses a mix of error types -
BadRequestErrorfor resources not found andAuthorizationErrorfor permission issues. It would be more consistent and in line with REST API conventions to use more specific error types:
NotFoundErrorfor resources not foundBadRequestErrorfor invalid requestsAuthorizationErrorfor permission issuesReplace
BadRequestErrorwithNotFoundErrorwhen resources are not found:if (!article) { - throw new BadRequestError(`Article with slug ${articleSlug} not found`); + throw new NotFoundError(`Article with slug ${articleSlug} not found`); } // And similar changes for other not found errorsAlso applies to: 52-54, 88-90, 94-96, 98-102, 104-108
1-5: Import NotFoundError for consistent error handling.Since you're already importing errors, make sure to import
NotFoundErrorfor use with resource not found scenarios.-import { AuthorizationError, BadRequestError } from '@errors'; +import { AuthorizationError, BadRequestError, NotFoundError } from '@errors';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/articles/comments/comments.service.ts(1 hunks)src/profiles/profiles.repository.ts(1 hunks)src/profiles/profiles.service.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/profiles/profiles.repository.ts (1)
src/users/users.model.ts (1) (1)
users(12-23)
src/profiles/profiles.service.ts (1)
src/profiles/profiles.schema.ts (1) (1)
ParsedProfileSchema(27-27)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(7-46)src/profiles/profiles.service.ts (1) (1)
ProfilesService(6-81)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(38-38)CommentToCreate(15-18)
🔇 Additional comments (5)
src/profiles/profiles.service.ts (2)
20-26: LGTM! Implementation offindByUserIdmethod.The method follows the same pattern as the existing
findByUsernamemethod, maintaining consistency in the codebase. It properly handles error cases by throwing aNotFoundErrorwhen a user is not found.
63-77: Well-handled nullable parameter type.The updated implementation correctly handles the case where
currentUserIdis null by returningfalsefor thefollowingfield, which is a sensible default. This enhances the robustness of the method.src/articles/comments/comments.service.ts (3)
14-44: LGTM: Comment creation implementation.The implementation is thorough in validating the article's existence, creating the comment with appropriate metadata, and retrieving author profile information for the response.
81-111: LGTM: Comment deletion implementation.The implementation is comprehensive in its error handling, verifying:
- Article existence
- Comment existence
- Comment belongs to the specified article
- User has permission to delete the comment
This ensures robust validation before performing the deletion operation.
113-119: LGTM: Helper method for retrieving author username.The private helper method is well-implemented and properly handles the case when a user is not found.
- Updated the findById method in CommentsRepository to include author information such as id, username, bio, and image. - Added the ability to fetch the author's followers alongside their details.
- Updated CommentsService to directly use authorId when fetching author profiles, improving clarity and efficiency. - Replaced the getAuthorUsername method with a direct call to findByUserId in ProfilesService.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/articles/comments/comments.service.ts (2)
46-78: 🛠️ Refactor suggestionOptimize database queries for fetching comments.
The
getCommentsmethod currently makes a separate database query for each comment to fetch the author's profile. For articles with many comments, this could lead to a significant number of database queries (N+1 problem).Consider optimizing this by:
- Fetching all unique author IDs from the comments
- Making a single query to get all profiles at once
- Mapping the profiles to their respective comments
async getComments( articleSlug: string, currentUserId?: number, ): Promise<ReturnedComment[]> { const article = await this.commentsRepository.findBySlug(articleSlug); if (!article) { throw new BadRequestError(`Article with slug ${articleSlug} not found`); } const comments = await this.commentsRepository.findManyByArticleId( article.id, ); + // Get unique author IDs + const authorIds = [...new Set(comments.map(comment => comment.authorId))]; + + // Fetch all profiles at once + const authorProfiles = await Promise.all( + authorIds.map(authorId => + this.profilesService.findByUserId( + currentUserId === undefined ? null : currentUserId, + authorId + ) + ) + ); + + // Create a map for quick profile lookup + const profileMap = new Map(); + authorIds.forEach((authorId, index) => { + profileMap.set(authorId, authorProfiles[index].profile); + }); const returnedComments = await Promise.all( comments.map(async (comment) => { - const authorProfile = await this.profilesService.findByUserId( - currentUserId || 0, - comment.authorId, - ); return { id: comment.id, body: comment.body, createdAt: comment.createdAt, updatedAt: comment.updatedAt, - author: authorProfile.profile, + author: profileMap.get(comment.authorId), }; }), ); return returnedComments; }
63-63:⚠️ Potential issueImprove handling of undefined currentUserId.
Using
currentUserId || 0is problematic because 0 could be a valid user ID, which would be treated the same as if currentUserId were undefined. This could lead to incorrect following status.- const authorProfile = await this.profilesService.findByUserId( - currentUserId || 0, - comment.authorId, - ); + const authorProfile = await this.profilesService.findByUserId( + currentUserId === undefined ? null : currentUserId, + comment.authorId, + );
🧹 Nitpick comments (4)
src/articles/comments/comments.service.ts (4)
112-118: Remove unused private method.The
getAuthorUsernamemethod is defined but not used anywhere in this class. Consider removing unused methods to improve code maintainability.- private async getAuthorUsername(userId: number): Promise<string> { - const user = await this.usersRepository.findById(userId); - if (!user) { - throw new BadRequestError(`User with id ${userId} not found`); - } - return user.username; - }If this method is intended for future use, consider adding a TODO comment explaining its purpose.
19-23: Duplicate article validation logic across methods.The same article validation logic is repeated in all three public methods. Consider extracting this common functionality into a private helper method.
+ private async getArticleBySlug(articleSlug: string) { + const article = await this.commentsRepository.findBySlug(articleSlug); + if (!article) { + throw new BadRequestError(`Article with slug ${articleSlug} not found`); + } + return article; + } async createComment( articleSlug: string, commentBody: { body: string }, userId: number, ): Promise<ReturnedComment> { - const article = await this.commentsRepository.findBySlug(articleSlug); - - if (!article) { - throw new BadRequestError(`Article with slug ${articleSlug} not found`); - } + const article = await this.getArticleBySlug(articleSlug); // Rest of the method... } // Apply similar changes to getComments and deleteComment methodsAlso applies to: 50-54, 85-89
38-43: Duplicate comment response structure.The same structure for formatting a returned comment is duplicated in both
createCommentandgetCommentsmethods. Consider extracting this into a private helper method.+ private formatComment(comment: any, authorProfile: any): ReturnedComment { + return { + id: comment.id, + body: comment.body, + createdAt: comment.createdAt, + updatedAt: comment.updatedAt, + author: authorProfile.profile, + }; + } async createComment( // ... ) { // ... - return { - id: comment.id, - body: comment.body, - createdAt: comment.createdAt, - updatedAt: comment.updatedAt, - author: authorProfile.profile, - }; + return this.formatComment(comment, authorProfile); } // Apply similar changes to the comments.map callback in getCommentsAlso applies to: 67-73
109-109: Consider handling delete operation result.The
deleteCommentmethod doesn't check if the delete operation was successful. The repository'sdeletemethod likely returns information about the number of rows affected.- await this.commentsRepository.delete(commentId, userId); + const result = await this.commentsRepository.delete(commentId, userId); + if (!result || result.count === 0) { + throw new BadRequestError('Failed to delete the comment'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/articles/comments/comments.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(7-59)src/profiles/profiles.service.ts (1) (1)
ProfilesService(6-81)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(38-38)CommentToCreate(15-18)
🔇 Additional comments (1)
src/articles/comments/comments.service.ts (1)
91-107: Great security checks to verify comment ownership.The implementation properly validates the comment's existence, association with the article, and user authorization before deletion. This is a good security practice that prevents unauthorized deletion of comments.
…ng and documentation - Added JSDoc comments to the getComments method in CommentsService for better clarity on parameters and return values. - Updated the findByUserId method in ProfilesService to accept a nullable currentUserId, improving type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/articles/comments/comments.service.ts (2)
21-22: Use more specific error types for missing resources.Throughout the file, missing resources (article or comment) are reported as
BadRequestError. Consider usingNotFoundError(or a similar 404-focused error) to better reflect the error cause when these resources are not present or do not belong to the specified article.Example changes you might apply:
- throw new BadRequestError(`Article with slug ${articleSlug} not found`); + throw new NotFoundError(`Article with slug ${articleSlug} not found`); - throw new BadRequestError(`Comment with id ${commentId} not found`); + throw new NotFoundError(`Comment with id ${commentId} not found`); - throw new BadRequestError( - `Comment with id ${commentId} does not belong to article ${articleSlug}`, - ); + throw new NotFoundError( + `Comment with id ${commentId} does not belong to article ${articleSlug}`, + );Also applies to: 59-60, 99-101, 103-107
66-81: Be mindful of potential N+1 database queries.For each comment, you're making a separate call to retrieve the author profile. If the number of comments is large, this can degrade performance significantly. Consider fetching all authors in one go, or using a join in the repository to avoid multiple queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/articles/comments/comments.service.ts(1 hunks)src/profiles/profiles.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/profiles/profiles.service.ts
🧰 Additional context used
🧠 Learnings (1)
src/articles/comments/comments.service.ts (1)
Learnt from: yamcodes
PR: agnyz/bedstack#134
File: src/articles/comments/comments.service.ts:32-35
Timestamp: 2025-03-20T20:01:10.732Z
Learning: In the comment creation flow in src/articles/comments/comments.service.ts, the code correctly uses the newly created comment's authorId when fetching the author profile, even though this is set from the userId parameter. This provides better semantic clarity than directly using userId again.
🧬 Code Definitions (1)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(7-59)src/profiles/profiles.service.ts (1) (1)
ProfilesService(6-81)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(38-38)CommentToCreate(15-18)
🔇 Additional comments (2)
src/articles/comments/comments.service.ts (2)
1-12: Implementation looks solid.No issues found in the imports and constructor. Dependency injection is clear and follows standard best practices.
32-35: Good semantic clarity when retrieving the author profile.Using
comment.authorId(derived fromuserId) to fetch the author’s profile is consistent with your previous learnings. This approach is fine and emphasizes the logic that the author of the comment is the user making the request.
Hajbo
left a comment
Hajbo
left a comment
There was a problem hiding this comment.
LGTM! We are almost there!🚀
…retrieval - Add a new method in CommentsRepository to find comments by article ID, optimized to include author details and their followers. - Simplify comment mapping in CommentsService to directly include author information, greatly improving performance from O(n*(k+t)) to O(n).
Doing this since we no longer need the extra fetching (no use case). - Remove unnecessary author details fetching in the findById method. - Add JSDoc.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/articles/comments/comments.service.ts (2)
58-60: Prefer a consistent error type for missing article.As with the createComment method, you may want to throw a
NotFoundErrorhere to make it clear that the article does not exist.
89-91: Inconsistent error usage for missing comment.Similar to missing article, consider throwing a
NotFoundErrorinstead of aBadRequestErrorwhen the comment is not found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/articles/comments/comments.repository.ts(1 hunks)src/articles/comments/comments.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/articles/comments/comments.repository.ts
🧰 Additional context used
🧠 Learnings (1)
src/articles/comments/comments.service.ts (1)
Learnt from: yamcodes
PR: agnyz/bedstack#134
File: src/articles/comments/comments.service.ts:32-35
Timestamp: 2025-03-20T20:01:10.732Z
Learning: In the comment creation flow in src/articles/comments/comments.service.ts, the code correctly uses the newly created comment's authorId when fetching the author profile, even though this is set from the userId parameter. This provides better semantic clarity than directly using userId again.
🧬 Code Definitions (1)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(6-79)src/profiles/profiles.service.ts (1) (1)
ProfilesService(6-81)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(38-38)CommentToCreate(15-18)
🔇 Additional comments (4)
src/articles/comments/comments.service.ts (4)
1-6: Module imports and constructors look good.No correctness issues. The dependencies (ProfilesService, UsersRepository, CommentsRepository) are appropriately injected and stored for later use.
32-35: Fetching author profile using the newly created comment’sauthorIdis correct.This implementation is consistent with the logic that
comment.authorIdshould match the user who created the comment, and provides clarity.
66-79: Verify thatcomment.authoris always present.Given the repository's
with: { author: ... }clause,comment.authorshould exist. However, if the DB query fails to return author details (e.g., orphaned records), this could cause runtime errors. Consider safety checks or confirming that the repository enforces referential integrity.
105-106: Authorization check is accurate.Throwing an
AuthorizationErrorif the user attempts to delete another author’s comment is correct.
- Replaced BadRequestError with NotFoundError when an article is not found by slug, improving clarity in error reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/articles/comments/comments.service.ts (2)
59-61: 🛠️ Refactor suggestionUse NotFoundError for consistency with the createComment method.
In the createComment method you correctly use NotFoundError for a missing article, but here you're using BadRequestError instead. For consistency and better HTTP semantics, use NotFoundError in both places.
if (!article) { - throw new BadRequestError(`Article with slug ${articleSlug} not found`); + throw new NotFoundError(`Article with slug ${articleSlug} not found`); }
90-92: 🛠️ Refactor suggestionUse NotFoundError for consistency with the createComment method.
Similar to the earlier comment, use NotFoundError here to maintain consistency throughout the service.
if (!article) { - throw new BadRequestError(`Article with slug ${articleSlug} not found`); + throw new NotFoundError(`Article with slug ${articleSlug} not found`); }
🧹 Nitpick comments (2)
src/articles/comments/comments.service.ts (2)
76-79: Consider using null coalescing operator for currentUserId check.The current implementation for determining "following" status works correctly, but could be more concise using the null coalescing operator.
following: currentUserId - ? comment.author.followers.some((f) => f.followerId === currentUserId) - : false, + ? comment.author.followers.some((f) => f.followerId === currentUserId) + : false,
112-113: Consider returning the deleted comment ID for confirmation.Currently, deleteComment returns void. Consider returning the deleted comment ID or a success message to provide confirmation to the caller.
- async deleteComment( + async deleteComment( articleSlug: string, commentId: number, userId: number, -): Promise<void> { +): Promise<{ id: number }> { // ...existing code... await this.commentsRepository.delete(commentId, userId); + return { id: commentId }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/articles/comments/comments.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/articles/comments/comments.service.ts (1)
Learnt from: yamcodes
PR: agnyz/bedstack#134
File: src/articles/comments/comments.service.ts:32-35
Timestamp: 2025-03-20T20:01:10.732Z
Learning: In the comment creation flow in src/articles/comments/comments.service.ts, the code correctly uses the newly created comment's authorId when fetching the author profile, even though this is set from the userId parameter. This provides better semantic clarity than directly using userId again.
🔇 Additional comments (6)
src/articles/comments/comments.service.ts (6)
1-7: Well-organized import structure with appropriate error types.Your imports are well-organized, including specific error types for different scenarios. I like that you're importing
NotFoundErrorfrom Elysia for resource not found situations.
8-14: Good service structure with proper dependency injection.The service follows good practices with explicit dependencies injected through the constructor and marked as private readonly.
15-45: The comment creation implementation looks solid.The flow for creating a comment is well-structured, checking for article existence, properly constructing the comment data, and returning the formatted response with author information.
47-81: Well-structured comments retrieval with proper documentation.The getComments method is well-documented with JSDoc and effectively processes the comments to include author information with following status. The mapping logic is clean and straightforward.
100-104: Good validation to ensure comment belongs to the specified article.This check helps maintain data integrity by ensuring the comment belongs to the correct article before proceeding with deletion.
106-110: Good authorization check to prevent unauthorized deletions.The code correctly verifies that only the author of a comment can delete it, with a clear error message explaining the restriction.
…issing articles - Replaced BadRequestError with NotFoundError when an article is not found by slug, enhancing clarity in error reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/articles/comments/comments.service.ts (2)
88-90:⚠️ Potential issueUse NotFoundError for a non-existent article.
For consistency with the error handling in other methods (like lines 20-22), use NotFoundError when an article doesn't exist.
if (!article) { - throw new BadRequestError(`Article with slug ${articleSlug} not found`); + throw new NotFoundError(`Article with slug ${articleSlug} not found`); }
94-96:⚠️ Potential issueUse NotFoundError for a non-existent comment.
For consistency with resource not found errors, use NotFoundError when a comment doesn't exist rather than BadRequestError.
if (!comment) { - throw new BadRequestError(`Comment with id ${commentId} not found`); + throw new NotFoundError(`Comment with id ${commentId} not found`); }
🧹 Nitpick comments (2)
src/articles/comments/comments.service.ts (2)
98-102: Consider using NotFoundError for comment-article mismatch.When a comment doesn't belong to the specified article, it's essentially a resource not found situation in the context of that article.
if (comment.articleId !== article.id) { - throw new BadRequestError( + throw new NotFoundError( `Comment with id ${commentId} does not belong to article ${articleSlug}`, ); }
65-78: Consider refactoring the comment mapping logic for better maintenance.The current mapping logic in getComments is inline. Consider extracting it to a helper method for better readability and maintainability, especially since you might need to reuse this logic elsewhere.
- return comments.map((comment) => ({ - id: comment.id, - body: comment.body, - createdAt: comment.createdAt, - updatedAt: comment.updatedAt, - author: { - username: comment.author.username, - bio: comment.author.bio, - image: comment.author.image, - following: currentUserId - ? comment.author.followers.some((f) => f.followerId === currentUserId) - : false, - }, - })); + return comments.map((comment) => this.formatCommentResponse(comment, currentUserId)); // Add this helper method to the class + private formatCommentResponse(comment: any, currentUserId?: number): ReturnedComment { + return { + id: comment.id, + body: comment.body, + createdAt: comment.createdAt, + updatedAt: comment.updatedAt, + author: { + username: comment.author.username, + bio: comment.author.bio, + image: comment.author.image, + following: currentUserId + ? comment.author.followers.some((f) => f.followerId === currentUserId) + : false, + }, + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/articles/comments/comments.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/articles/comments/comments.service.ts (1)
Learnt from: yamcodes
PR: agnyz/bedstack#134
File: src/articles/comments/comments.service.ts:32-35
Timestamp: 2025-03-20T20:01:10.732Z
Learning: In the comment creation flow in src/articles/comments/comments.service.ts, the code correctly uses the newly created comment's authorId when fetching the author profile, even though this is set from the userId parameter. This provides better semantic clarity than directly using userId again.
🧬 Code Definitions (1)
src/articles/comments/comments.service.ts (3)
src/articles/comments/comments.repository.ts (1) (1)
CommentsRepository(6-71)src/profiles/profiles.service.ts (1) (1)
ProfilesService(6-81)src/articles/comments/comments.schema.ts (2) (2)
ReturnedComment(38-38)CommentToCreate(15-18)
🔇 Additional comments (2)
src/articles/comments/comments.service.ts (2)
31-34: Clean approach for retrieving author profile.The code correctly uses the newly created comment's authorId when fetching the author profile, which provides good semantic clarity even though it's set from the userId parameter.
74-76: Correct handling of following status for comments.The implementation properly handles the following status by checking if currentUserId exists and setting it to false when not provided. This aligns with the behavior in ProfilesService and discussions about proper handling of the following field.
Description
Implement comment feature for articles.
Builds on #129 so it's better to merge that first.
PR Checklist
bun docsSummary by CodeRabbit