Add article CRUD endpoints and some others #129
Conversation
WalkthroughThis pull request makes several codebase updates across configuration, migration, and feature modules. The changes include reformatting VS Code settings, adding a migrations configuration and drop logic in the database, introducing new migration files and SQL scripts, and integrating a comprehensive articles feature (model, module, plugin, repository, schema, and service). Additional enhancements include improved Swagger bearer token support in the app module, error handling in authentication, schema export adjustments, and the inclusion of a slugify utility. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ArticlesPlugin
participant ArticlesService
participant ArticlesRepository
Client->>ArticlesPlugin: POST /articles (InsertArticleSchema)
ArticlesPlugin->>ArticlesService: createArticle(requestData)
ArticlesService->>ArticlesRepository: insertArticle(articleData)
ArticlesRepository-->>ArticlesService: newArticle
ArticlesService-->>ArticlesPlugin: formatArticle(newArticle)
ArticlesPlugin-->>Client: Return Created Article (ReturnedArticleResponseSchema)
Assessment against linked issues
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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 (
|
|
@yamcodes I'm really not happy with the naming of things, any suggestion would be appreciated. Generally I don't like how the schemas and types are called across the codebase, maybe you have a better idea about it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/utils/slug.ts (1)
31-37: Consider adding a test for the slugify function.The logic is sound, but slug generation is a critical component for article functionality since slugs are typically used in URLs. Adding unit tests would help ensure the function behaves as expected across different inputs, including edge cases like strings with multiple spaces, special characters, or non-Latin characters.
src/articles/articles.service.ts (2)
46-58: Consider uniqueness checks for slugs.
Becauseslugis derived from the title, collisions can occur if multiple articles share similar titles. Ensure that the repository throws or handles any uniqueness constraint violation.
60-82: Ensure concurrency safety for slug updates.
When updating an article’s title, the slug changes. If multiple updates happen concurrently, consider whether you need an optimistic concurrency check or versioning to avoid race conditions.src/articles/articles.plugin.ts (1)
55-72: Typographical error in summary.
Line 69 uses “Artifle Feed” instead of “Article Feed.” Update to maintain consistency.- summary: 'Artifle Feed', + summary: 'Article Feed',db/migrations/meta/0007_snapshot.json (1)
1-308: Align date/time column types for consistency.
Thearticlestable usestimestampforcreated_atandupdated_at, while other tables (e.g.,favorite_articles,user_follows,users) usedate. Storing only the date may discard valuable time information and lead to inconsistencies. Consider usingtimestampthroughout if you rely on specific time details.src/articles/articles.repository.ts (3)
110-123: Confirm return value handling for non-existent slug.The method returns
nullif no article is found for the given slug. Consider throwing an error or returning a more explicit message for a missing article to streamline error handling in the service layer.
146-168: Check if update has taken effect.After performing the update query, consider verifying whether the article was actually updated (e.g., checking affected rows) and possibly return the updated article or a bespoke response if no rows were updated (meaning the article might not belong to the user or didn’t exist).
169-175: Return the deleted article or a custom message.As with updates, providing immediate feedback about whether the article was deleted or not will help keep the API consumer informed. A response indicating that no records were affected can be useful if the article does not exist or the user is not the owner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.vscode/settings.json(1 hunks)db/config.ts(1 hunks)db/drop.ts(2 hunks)db/migrations/0007_elite_patriot.sql(1 hunks)db/migrations/meta/0007_snapshot.json(1 hunks)db/migrations/meta/_journal.json(1 hunks)src/app.module.ts(2 hunks)src/articles/articles.model.ts(1 hunks)src/articles/articles.module.ts(1 hunks)src/articles/articles.plugin.ts(1 hunks)src/articles/articles.repository.ts(1 hunks)src/articles/articles.schema.ts(1 hunks)src/articles/articles.service.ts(1 hunks)src/auth/auth.service.ts(1 hunks)src/database.providers.ts(2 hunks)src/profiles/profiles.schema.ts(2 hunks)src/profiles/profiles.service.ts(1 hunks)src/users/users.model.ts(2 hunks)src/users/users.schema.ts(1 hunks)src/utils/slug.ts(1 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (45)
.vscode/settings.json (1)
18-18: Format change for cSpell.words looks good.The consolidation of the spell checker words to a single line is a simple formatting change with no functional impact.
tsconfig.json (1)
40-40: Path mapping for articles module properly configured.This addition follows the project's established pattern for path aliases and will help maintain clean imports for the new articles module.
db/migrations/meta/_journal.json (1)
54-59: Migration entry correctly added to journal.This new migration entry for "0007_elite_patriot" follows the sequential indexing pattern of previous migrations and appears to be the database changes supporting the article functionality mentioned in the PR summary.
src/utils/slug.ts (1)
26-39: Slug generation utility looks good with proper attribution.The slugify function appears well-implemented with appropriate character normalization, accent removal, and formatting for URL-friendly slugs. The MIT license and reference links to the original sources are properly included.
db/config.ts (1)
22-26: Explicitly defined migration configuration improves clarity.The addition of explicit migration configuration enhances code readability by clearly defining where migration metadata is stored, rather than relying on default settings.
src/database.providers.ts (2)
1-1: Import for the new articles schema added.This import correctly brings in the articles schema definitions needed for database setup.
12-12: Schema configuration updated to include articles.The database schema configuration has been properly extended to include both users and articles schemas, which is essential for enabling the article-related database operations.
src/articles/articles.module.ts (2)
1-9: Imports for article module dependencies established.All necessary dependencies for the article module are properly imported, including repositories, services, and the Elysia framework.
10-24: Well-structured setup function using dependency injection.The
setupArticlesfunction follows good software architecture practices:
- Creates repository instances first
- Injects repositories into services, establishing proper dependency chains
- Returns an Elysia instance with appropriate state for route handlers
This approach maintains clean separation of concerns and facilitates testing.
db/drop.ts (3)
3-4: Required imports for article tables and database configuration added.The imports for article-related tables and database configuration have been correctly added to support the enhanced drop functionality.
8-8: Table drop list updated to include article tables.The table array has been properly updated to include the new article-related tables, ensuring they're dropped when the database is reset.
22-28: Migration table drop logic added.Good addition to handle the migrations table cleanup. This addresses the issue mentioned in the PR objectives where the migrations table needed to be dropped as well.
Note that dropping the migrations table will cause all migrations to be re-applied if the system is restarted, as the migration history will be lost. Ensure this is the intended behavior.
src/users/users.model.ts (2)
1-1: Good addition of imports for article-related models.The import of
articlesandfavoriteArticlesis correctly added from the articles model, which will be needed for the new relationships defined in this file.
28-29: Well-structured relationship modeling for user-article connections.These new relationships appropriately establish the connections between users and articles:
publishedArticleslinks users to the articles they've authoredfavoriteArticlesconnects users to the articles they've favoritedThe relationship naming is clear and follows the established pattern in the codebase. This implementation aligns perfectly with the PR objective of adding article functionality.
src/profiles/profiles.schema.ts (3)
5-5: Good update to import the proper schema.Switching from
insertUserSchemaRawtoselectUserSchemaRawis appropriate since profiles are retrieved data rather than data being inserted.
14-14: Correctly uses the imported select schema.Using
selectUserSchemaRawinstead ofinsertUserSchemaRawin theType.Omitis the right approach for defining returned profile data structure.
21-23: Minor formatting improvement.The addition of trailing commas improves code consistency and makes future additions to this object cleaner (preventing git diff issues when adding new properties).
src/users/users.schema.ts (1)
49-49: Good export of the select schema.Making
selectUserSchemaRawavailable for import in other modules is necessary for its usage in the profiles schema. This change correctly supports the modifications made insrc/profiles/profiles.schema.ts.src/auth/auth.service.ts (1)
95-102: Well-implemented optional user ID retrieval.This new method elegantly handles the case where authentication is optional, returning the user ID when available and null otherwise. It properly reuses the existing
getUserFromHeadersmethod while adding appropriate error handling for the optional case.This approach is valuable for features like viewing articles where behavior might differ between authenticated and anonymous users without requiring authentication.
src/profiles/profiles.service.ts (1)
55-69: Good update to handle optional user IDsThe modification to accept
nullforcurrentUserIdand the conditional logic to handle this case is a good improvement. This change makes the profile service more flexible, allowing it to generate profile responses for both authenticated and unauthenticated users.async generateProfileResponse( user: Profile, - currentUserId: number, + currentUserId: number | null, ): Promise<ParsedProfileSchema> { return { profile: { bio: user.bio, image: user.image, username: user.username, - following: !!user.followers.find( - (follower) => follower.followerId === currentUserId, - ), + following: + currentUserId == null + ? false + : !!user.followers.find( + (follower) => follower.followerId === currentUserId, + ), }, }; }This change properly handles cases where profile information needs to be retrieved without a logged-in user context (returning
following: false).src/app.module.ts (4)
1-1: Adding the articles plugin importGood addition to support the new articles functionality.
40-54: Proper Swagger authentication configurationThe addition of security schemes for token authentication in Swagger is well-implemented. The description correctly instructs users to prefix tokens with "Token" rather than "Bearer", which aligns with the PR objectives mentioning fixing the token authentication format.
57-59: Good UX improvement with persistAuthorizationSetting
persistAuthorization: trueimproves the developer experience by maintaining the authorization state across Swagger UI requests.
62-64: Adding the articles plugin to the API groupThe articles plugin is correctly integrated into the API group alongside the existing plugins.
db/migrations/0007_elite_patriot.sql (3)
1-12: Well-structured articles tableThe articles table has all necessary fields with appropriate constraints:
- Primary key on id
- Unique constraint on slug (important for URL-friendly identifiers)
- Required fields for content (title, description, body)
- Array type for tags with default empty array
- Proper timestamp fields for created_at and updated_at
- Foreign key to connect articles to their authors
This structure aligns well with standard article management requirements.
14-20: Well-designed favorites relationship tableThe favorite_articles table correctly implements a many-to-many relationship between users and articles with:
- Composite primary key on (article_id, user_id)
- Appropriate timestamp fields
This design efficiently tracks which users have favorited which articles.
22-24: Appropriate cascade delete constraintsThe foreign key constraints with cascade delete behavior are correctly implemented:
- Deleting a user will delete their authored articles and favorites
- Deleting an article will delete its favorites
This ensures referential integrity while preventing orphaned records.
src/articles/articles.model.ts (4)
14-26: Well-structured articles table modelThe Drizzle ORM model for articles is properly defined with all necessary fields and constraints matching the SQL migration. The model includes appropriate references to the users table for the author relationship.
28-37: Proper article relations configurationThe article relations are well-configured:
- One-to-one relation with users for the author
- Many-to-many relation with users through the favoriteArticles table
This provides a good foundation for querying article data with related entities.
39-52: Correct implementation of favorite_articles junction tableThe favoriteArticles table properly implements a many-to-many relationship between users and articles with:
- References to both articles and users tables
- Composite primary key on (articleId, userId)
- Appropriate timestamp fields
- Cascade delete constraints
This matches the SQL migration and follows Drizzle ORM best practices.
54-68: Well-defined favorite article relationsThe relations for the favoriteArticles table are correctly set up:
- One-to-one relation with the articles table
- One-to-one relation with the users table
- Appropriate relation names for bidirectional navigation
This allows for efficient querying of favorite articles with their related articles and users.
src/articles/articles.service.ts (5)
1-14: Imports look good.
No issues found with the import statements.
21-36: Verify the accuracy ofarticlesCount.
Currently,articlesCountis set toresults.length, which only reflects the size of the fetched subset (limited byoffsetandlimit). If you want to provide the total number of matching articles in the database, consider having the repository return the total instead.
38-44: Well-structured error handling.
Throwing aNotFoundErrorhere is appropriate.
84-97: Deletion logic appears sound.
The authorization check is clear, and the final JSON response is straightforward.
99-124: ConfirmfavoritedByalways returns an array.
IffavoritedBycould be undefined, the code would throw an error. Make sure the repository or mapping layer consistently returns an empty array instead ofnullorundefined.src/articles/articles.plugin.ts (6)
1-12: No issues with initial imports.
All imports are appropriately scoped.
13-38: Plugin setup is coherent.
Establishing the/articlesgroup with relevant tags is clear and well-documented.
39-54: Create Article endpoint looks good.
The route enforces authentication and validates the body schema properly.
73-83: Slug route retrieval is well-structured.
The path parameter is retrieved and handled properly.
84-100: Update endpoint logic is consistent.
Authentication, request body validation, and pass-through to the service are handled neatly.
101-116: Delete endpoint approach is clear.
The route ensures authentication and calls the corresponding service method.src/articles/articles.repository.ts (1)
1-9: Ensure consistent usage of path aliases.Lines 6-7 use
@/users/users.modelvs.@articles/articles.model; please verify all import aliases to maintain consistency or confirm that these aliases are intentionally configured per the build system.#!/bin/bash # Description: Verify that all import paths follow a consistent alias pattern. rg --sort=file "from '@" srcsrc/articles/articles.schema.ts (2)
19-23: Clarify slug creation in ArticleToCreate.The schema includes
slug: string, which may be set externally or computed upon insertion. Verify who owns slug generation logic, or consider omitting it if the logic is purely back-end-driven.
43-55: Validate author’s field types in ReturnedArticleSchema.Within the composite schema, you are explicitly returning strings for
author.bio,author.image, andauthor.username. Confirm that these fields are always present and non-null in the database. If they can benull, consider making them optional or providing a fallback value.
First, I hate hearing that you're "really not happy" 😭 so I'd do anything I can to change that! Second, this is another thing I learned in the past year and half and formed an opinion on - and it pretty much aligns perfectly with colinhacks' here, I think he makes a great case: colinhacks/zod#3285 (comment) it's not something we really need to tackle in this PR, but we can do another one to align the namings with this convention. |
|
Small thing for your next PR, to make the checkpoints go from this: 
to this: 
You need to use I've also edited the PR template to reflect that, in bed086e Edit: Holy shit, I cannot believe this SHA starts with "bed", what are the odds??? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
db/migrations/0007_thin_eternals.sql (2)
1-12: Consider adding indexes for improved query performance.The articles table looks well-structured with appropriate constraints. To optimize query performance, consider adding indexes for fields that will be frequently used in WHERE clauses or JOINs, particularly the
author_idfield since you'll likely want to filter articles by author.CREATE TABLE "articles" ( "id" serial PRIMARY KEY NOT NULL, "slug" text NOT NULL, "title" text NOT NULL, "description" text NOT NULL, "body" text NOT NULL, "tag_list" text[] DEFAULT '{}' NOT NULL, "created_at" timestamp DEFAULT now() NOT NULL, "updated_at" timestamp DEFAULT now() NOT NULL, "author_id" integer NOT NULL, CONSTRAINT "articles_slug_unique" UNIQUE("slug") ); +CREATE INDEX "articles_author_id_idx" ON "articles" ("author_id"); +CREATE INDEX "articles_created_at_idx" ON "articles" ("created_at" DESC);
14-20: Consider adding an index for user_id to optimize user-specific queries.The composite primary key on article_id and user_id is good. For queries that filter favorites by user, an additional index on user_id alone would improve performance.
CREATE TABLE "favorite_articles" ( "article_id" integer NOT NULL, "user_id" integer NOT NULL, "created_at" timestamp DEFAULT now() NOT NULL, "updated_at" timestamp DEFAULT now() NOT NULL, CONSTRAINT "favorite_articles_article_id_user_id_pk" PRIMARY KEY("article_id","user_id") ); +CREATE INDEX "favorite_articles_user_id_idx" ON "favorite_articles" ("user_id");src/articles/articles.service.ts (2)
46-58: Potential slug collision not handled.The slugify function may generate the same slug for similar titles. Consider adding logic to handle slug collisions by appending a unique identifier when a slug already exists.
async createArticle(article: ArticleToCreateData, currentUserId: number) { + let baseSlug = slugify(article.title); + let slug = baseSlug; + let counter = 0; + + // Check if slug already exists and generate a unique one if needed + while (await this.repository.slugExists(slug)) { + counter++; + slug = `${baseSlug}-${counter}`; + } + const articleToCreate: ArticleToCreate = { ...article, tagList: article.tagList?.sort() || [], authorId: currentUserId, - slug: slugify(article.title), + slug, }; const createdArticle = await this.repository.createArticle(articleToCreate); if (!createdArticle) { throw new BadRequestError('Article was not created'); } return await this.generateArticleResponse(createdArticle, currentUserId); }This would require adding a
slugExistsmethod to the repository.
72-82: Same slug collision issue during update.Similar to the creation method, the update method doesn't handle potential slug collisions when updating the title.
const newSlug = article.title - ? slugify(article.title) + ? await this.generateUniqueSlug(slugify(article.title), existingArticle.id) : existingArticle.slug; await this.repository.updateArticle( existingArticle.id, { ...article, slug: newSlug }, currentUserId, ); return this.findBySlug(newSlug);You would need to add a helper method:
private async generateUniqueSlug(baseSlug: string, excludeArticleId?: number): Promise<string> { let slug = baseSlug; let counter = 0; while (await this.repository.slugExistsExcludingArticle(slug, excludeArticleId)) { counter++; slug = `${baseSlug}-${counter}`; } return slug; }src/articles/articles.plugin.ts (1)
69-70: Typo in feed endpoint summary.There's a spelling error in the summary for the article feed endpoint.
detail: { - summary: 'Artifle Feed', + summary: 'Article Feed', },db/migrations/meta/0007_snapshot.json (2)
189-202: Consider using timestamp instead of date for user_follows timestamps.I noticed that
user_followstable uses thedatetype for timestamps, while thearticlesandfavorite_articlestables usetimestamp. This inconsistency could lead to precision issues when tracking follow relationships.- "type": "date", + "type": "timestamp",Also consider changing the default from
CURRENT_DATEtonow()for consistency with the articles tables.
289-302: Consider using timestamp instead of date for user timestamps.Similar to the user_follows table, the users table also uses
datetype for timestamps instead oftimestamp. For consistency and to capture time information:- "type": "date", + "type": "timestamp",And consider changing the default from
CURRENT_DATEtonow().src/articles/articles.repository.ts (2)
171-177: Add return type to deleteArticle method.The
deleteArticlemethod should have an explicit return type for better code clarity:- async deleteArticle(slug: string, currentUserId: number) { + async deleteArticle(slug: string, currentUserId: number): Promise<void> {
142-146: Add error handling for article creation.The
createArticlemethod doesn't handle the case where insertion might fail. Consider adding error handling:async createArticle(article: ArticleToCreate) { - const results = await this.db.insert(articles).values(article).returning(); - const newArticle = results[0]; - return this.findById(newArticle.id); + try { + const results = await this.db.insert(articles).values(article).returning(); + const newArticle = results[0]; + return await this.findById(newArticle.id); + } catch (error) { + // Log the error or handle specific database errors + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
db/drop.ts(2 hunks)db/migrations/0007_thin_eternals.sql(1 hunks)db/migrations/meta/0007_snapshot.json(1 hunks)db/migrations/meta/_journal.json(1 hunks)src/articles/articles.model.ts(1 hunks)src/articles/articles.plugin.ts(1 hunks)src/articles/articles.repository.ts(1 hunks)src/articles/articles.service.ts(1 hunks)src/utils/slugify.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- db/drop.ts
- db/migrations/meta/_journal.json
- src/articles/articles.model.ts
🧰 Additional context used
🧠 Learnings (1)
src/articles/articles.repository.ts (1)
Learnt from: Hajbo
PR: agnyz/bedstack#129
File: src/articles/articles.repository.ts:13-108
Timestamp: 2025-03-17T17:22:55.819Z
Learning: When using Drizzle ORM with nullable parameters in SQL expressions, handling null checks in TypeScript before constructing the SQL query can help avoid type inference issues, rather than using SQL CASE WHEN statements.
🧬 Code Definitions (2)
src/articles/articles.plugin.ts (2)
src/articles/articles.module.ts (1) (1)
setupArticles(10-24)src/articles/articles.schema.ts (7) (7)
ListArticlesQuerySchema(80-87)ReturnedArticleListSchema(89-92)InsertArticleSchema(10-17)ReturnedArticleResponseSchema(57-59)ArticleFeedQuerySchema(76-79)UpdateArticleSchema(25-34)DeleteArticleResponse(96-99)
src/articles/articles.repository.ts (4)
src/database.providers.ts (2) (2)
db(11-14)Database(15-15)src/users/users.model.ts (2) (2)
users(12-23)userFollows(32-45)src/articles/articles.model.ts (2) (2)
articles(14-26)favoriteArticles(39-52)src/articles/articles.schema.ts (2) (2)
ArticleToCreate(20-23)ArticleToUpdate(39-41)
🔇 Additional comments (10)
src/utils/slugify.ts (2)
26-39: Nice implementation of the slugify utility.The slugify function looks well-implemented with good handling of accents and special characters. I particularly like the use of Unicode normalization to separate base characters from diacritical marks.
A couple of suggestions for future enhancement:
- You might want to add collision handling if multiple articles end up with the same slug
- Consider adding a maximum length parameter to prevent overly long slugs
32-33: The biome-ignore comment explains the false positive.Good practice to include the reason for the linter directive. This helps future maintainers understand why the exception was necessary.
db/migrations/0007_thin_eternals.sql (1)
22-24: Foreign key constraints look good.The cascade delete behavior is appropriate here - when an article or user is deleted, related favorites should also be removed. This prevents orphaned records.
src/articles/articles.plugin.ts (3)
23-38: Consistent route paths for cleaner API.In previous review discussions, there was a comment about trailing slashes. For consistency, it would be better to choose one approach (with or without trailing slashes) and apply it throughout the API.
27-30: Auth service API could be simplified.As noted in a previous review comment, the way authentication is handled could be improved. Consider refactoring the auth service to accept the full request object or implement a more idiomatic approach using Elysia's features.
13-19: Well-organized plugin structure.The use of Elysia's group feature to organize routes under the '/articles' path is well-implemented. The tagging for documentation purposes is also a good practice.
db/migrations/meta/0007_snapshot.json (1)
1-339: LGTM: Database schema snapshot looks well-structured.The schema defines four tables with appropriate relationships and constraints:
articleswith a unique slug constraint and foreign key to usersfavorite_articleswith a composite primary key and proper cascade behaviorsuser_followswith appropriate relationship constraintsuserswith unique constraints on email and usernameGood job setting up the relationships and constraints to maintain data integrity.
src/articles/articles.repository.ts (3)
40-43: Handle possible null currentUserId before constructing SQL expression.When
currentUserIdisnull, this could lead to type inference issues with Drizzle ORM.Based on the previous discussion and learnings, it's better to handle the null check in TypeScript:
- authorFollowing: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( - 'authorFollowing', - ), + authorFollowing: + (currentUserId === null + ? sql<boolean>`false` + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)` + ).as('authorFollowing'),
63-66: Handle possible null currentUserId before constructing SQL expression.Similar to the previous instance, the null check for
currentUserIdshould be handled in TypeScript:- favorited: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(favorite_articles.user_id)), false)`.as( - 'favorited', - ), + favorited: + (currentUserId === null + ? sql<boolean>`false` + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(favorite_articles.user_id)), false)` + ).as('favorited'),
153-161: Simplify object transformation with a utility function.The
reduceoperation is harder to read and understand at a glance. Consider using a utility function likeshaketo filter out undefined values.- const valuesToSet = Object.entries(article).reduce( - (acc: { [key: string]: string | string[] | Date }, [key, value]) => { - if (value !== undefined) { - acc[key] = value; - } - return acc; - }, - {}, - ); + const valuesToSet = { ...shake(article) };Where
shakecan be imported from a utility library like Radashi.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/articles/articles.repository.ts (1)
33-49: 🛠️ Refactor suggestionHandle null currentUserId in TypeScript rather than SQL.
Based on the learnings from previous discussions, you should handle the null check for
currentUserIdin TypeScript before constructing the SQL to avoid type inference issues with Drizzle ORM.const authorsWithFollowersCTE = this.db.$with('authorsWithFollowers').as( this.db .select({ authorId: users.id, authorUsername: users.username, authorBio: users.bio, authorImage: users.image, - authorFollowing: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( - 'authorFollowing', - ), + authorFollowing: currentUserId === null + ? sql<boolean>`false`.as('authorFollowing') + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( + 'authorFollowing', + ), }) .from(users) .leftJoin(userFollows, eq(users.id, userFollows.followedId)) .where(and(...authorFilters)) .groupBy(users.id), );
🧹 Nitpick comments (3)
src/articles/articles.service.ts (2)
39-39: Consider passing the currentUserId to the generateArticleResponse method.When finding an article by slug, you're passing
nullfor thecurrentUserId. If the endpoint might be called by authenticated users, consider extracting the user ID from the request headers.
56-78: The updateArticle implementation looks solid but return value could be improved.The implementation correctly checks for article existence and author permissions before updating. However, consider adding error handling and updating the method to return the updated article directly from the repository.
async updateArticle( slug: string, article: ArticleToUpdateRequest, currentUserId: number, ) { const existingArticle = await this.repository.findBySlug(slug); if (!existingArticle) { throw new NotFoundError('Article not found'); } if (existingArticle.authorId !== currentUserId) { throw new AuthorizationError('Only the author can update the article'); } const newSlug = article.title ? slugify(article.title) : existingArticle.slug; - await this.repository.updateArticle( - existingArticle.id, - { ...article, slug: newSlug }, - currentUserId, - ); - return this.findBySlug(newSlug); + try { + const updatedArticle = await this.repository.updateArticle( + existingArticle.id, + { ...article, slug: newSlug }, + currentUserId, + ); + return await this.generateArticleResponse(updatedArticle, currentUserId); + } catch (error) { + console.error('Error updating article:', error); + throw error; + } }src/articles/articles.repository.ts (1)
177-183: Add error handling to deleteArticle.Consider adding error handling to the
deleteArticlemethod to catch and properly handle errors from the database operation.async deleteArticle(slug: string, currentUserId: number) { - return await this.db - .delete(articles) - .where( - and(eq(articles.slug, slug), eq(articles.authorId, currentUserId)), - ); + try { + return await this.db + .delete(articles) + .where( + and(eq(articles.slug, slug), eq(articles.authorId, currentUserId)), + ); + } catch (error) { + console.error('Error deleting article:', error); + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/articles/articles.repository.ts(1 hunks)src/articles/articles.service.ts(1 hunks)src/auth/auth.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/articles/articles.repository.ts (1)
Learnt from: Hajbo
PR: agnyz/bedstack#129
File: src/articles/articles.repository.ts:13-108
Timestamp: 2025-03-17T17:22:55.819Z
Learning: When using Drizzle ORM with nullable parameters in SQL expressions, handling null checks in TypeScript before constructing the SQL query can help avoid type inference issues, rather than using SQL CASE WHEN statements.
🧬 Code Definitions (3)
src/auth/auth.service.ts (1)
src/errors.ts (1) (1)
AuthenticationError(3-9)
src/articles/articles.service.ts (4)
src/articles/articles.repository.ts (1) (1)
ArticlesRepository(10-184)src/profiles/profiles.service.ts (1) (1)
ProfilesService(6-73)src/articles/articles.schema.ts (6) (6)
ReturnedArticleList(94-94)ArticleToCreateData(19-19)ArticleToCreate(20-23)ArticleToUpdateRequest(36-38)ArticleInDb(66-72)ReturnedArticleResponse(62-64)src/utils/slugify.ts (1) (1)
slugify(26-39)
src/articles/articles.repository.ts (4)
src/database.providers.ts (2) (2)
db(11-14)Database(15-15)src/users/users.model.ts (2) (2)
users(12-23)userFollows(32-45)src/articles/articles.model.ts (2) (2)
articles(14-26)favoriteArticles(39-52)src/articles/articles.schema.ts (2) (2)
ArticleToCreate(20-23)ArticleToUpdate(39-41)
🔇 Additional comments (6)
src/auth/auth.service.ts (1)
95-103: The implementation of optional user authentication looks good.The method properly handles authentication errors by returning null when an
AuthenticationErroroccurs, while still propagating other types of errors. This approach allows for optional authentication in endpoints that can work with both authenticated and unauthenticated users.src/articles/articles.service.ts (1)
113-116: No need for the null check in thefavoritedcheck.While adding a null check for
currentUserIdin thefavoritedcondition would make the code more explicit, it's functionally unnecessary since comparing withnullusing===already returnsfalsecorrectly.src/articles/articles.repository.ts (4)
106-113: Good implementation of pagination with count.The implementation correctly separates the limited result query from the count query, ensuring accurate pagination. This is a robust approach that will work correctly even with large datasets.
116-130: Add explicit column selections to findBySlug.For clarity and to ensure all necessary fields are included, add explicit column selections to the
findBySlugmethod.async findBySlug(slug: string) { const result = await this.db.query.articles.findFirst({ where: eq(articles.slug, slug), + columns: { + id: true, + slug: true, + title: true, + description: true, + body: true, + tagList: true, + createdAt: true, + updatedAt: true, + authorId: true, + }, with: { author: { with: { followers: true, }, }, favoritedBy: true, }, }); if (!result) return null; return result; }
132-146: Add explicit column selections to findById.Similarly, add explicit column selections to the
findByIdmethod for consistency and clarity.async findById(id: number) { const result = await this.db.query.articles.findFirst({ where: eq(articles.id, id), + columns: { + id: true, + slug: true, + title: true, + description: true, + body: true, + tagList: true, + createdAt: true, + updatedAt: true, + authorId: true, + }, with: { author: { with: { followers: true, }, }, favoritedBy: true, }, }); if (!result) return null; return result; }
154-175: Simplify the updateArticle method.The current approach using
reduceto filter out undefined values works, but a more straightforward approach would be clearer and easier to maintain.async updateArticle( articleId: number, article: ArticleToUpdate, currentUserId: number, ) { - const valuesToSet = Object.entries(article).reduce( - (acc: { [key: string]: string | string[] | Date }, [key, value]) => { - if (value !== undefined) { - acc[key] = value; - } - return acc; - }, - {}, - ); - valuesToSet.updatedAt = new Date(); + // Create a new object without undefined values + const valuesToSet = Object.fromEntries( + Object.entries(article).filter(([_, value]) => value !== undefined) + ); + valuesToSet.updatedAt = new Date(); await this.db .update(articles) .set(valuesToSet) .where( and(eq(articles.id, articleId), eq(articles.authorId, currentUserId)), ); + // Return the updated article + return this.findById(articleId); }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/articles/articles.repository.ts (4)
53-56:⚠️ Potential issueHandle possible null currentUserId in authorFollowing expression.
When
currentUserIdisnull, the expression could produce unexpected behavior. The SQL expression isn't handling the null case properly.Handle the null check in TypeScript rather than SQL to avoid type inference issues with Drizzle:
-authorFollowing: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( - 'authorFollowing', - ), +authorFollowing: currentUserId === null + ? sql<boolean>`false`.as('authorFollowing') + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( + 'authorFollowing', + ),
76-79:⚠️ Potential issueHandle possible null currentUserId in favorited expression.
Similar to the authorFollowing expression, the favorited expression should handle null currentUserId in TypeScript.
Apply the same pattern as suggested for authorFollowing:
-favorited: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(favorite_articles.user_id)), false)`.as( - 'favorited', - ), +favorited: currentUserId === null + ? sql<boolean>`false`.as('favorited') + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(favorite_articles.user_id)), false)`.as( + 'favorited', + ),
132-146: 🛠️ Refactor suggestionInclude explicit column selection in findBySlug.
The
findBySlugmethod doesn't explicitly select columns, which could lead to missing fields in the returned data.Add explicit column selection to ensure all necessary fields are included:
async findBySlug(slug: string) { const result = await this.db.query.articles.findFirst({ where: eq(articles.slug, slug), + columns: { + id: true, + slug: true, + title: true, + description: true, + body: true, + tagList: true, + createdAt: true, + updatedAt: true, + }, with: { author: { with: { followers: true, }, }, favoritedBy: true, }, }); if (!result) return null; return result; }
148-162: 🛠️ Refactor suggestionInclude explicit column selection in findById.
Similar to findBySlug, the findById method should explicitly select all needed columns.
Add explicit column selection:
async findById(id: number) { const result = await this.db.query.articles.findFirst({ where: eq(articles.id, id), + columns: { + id: true, + slug: true, + title: true, + description: true, + body: true, + tagList: true, + createdAt: true, + updatedAt: true, + }, with: { author: { with: { followers: true, }, }, favoritedBy: true, }, }); if (!result) return null; return result; }
🧹 Nitpick comments (4)
db/migrations/meta/0007_snapshot.json (2)
1-307: Database schema structure looks well-designed, but has timestamp vs date inconsistency.The database schema is well-structured with appropriate relationships and constraints. However, there's an inconsistency in date/time handling:
articlesandfavorite_articlestables usetimestamptype (lines 50, 57, 111, 118)user_followsanduserstables usedatetype (lines 174, 181, 263, 270)This inconsistency could lead to subtle bugs where some entities track time with precision while others only track dates.
Consider standardizing on
timestampfor all temporal columns to ensure consistency and retain time precision across all tables.
69-70: Consider adding indexes on foreign key columns for performance.The schema doesn't define any indexes on foreign key columns that would be frequently used in JOIN operations or WHERE clauses. This could impact query performance, especially as the database grows.
Consider adding indexes on foreign key columns and other frequently queried columns:
articles.author_idfavorite_articles.article_idandfavorite_articles.user_iduser_follows.followed_idanduser_follows.follower_idAlso applies to: 276-277
src/articles/articles.repository.ts (2)
193-199: Add error handling and check for deletion success.The deleteArticle method doesn't check if the deletion was successful and has unnecessary await in the return statement.
Improve error handling and check for deletion success:
async deleteArticle(slug: string, currentUserId: number) { - return await this.db + const result = await this.db .delete(articles) .where( and(eq(articles.slug, slug), eq(articles.authorId, currentUserId)), ); + return result.rowsAffected > 0; }
6-7: Ensure consistent import paths.Import statements mix
@/and@articles/path aliases, which can be confusing.Standardize import path aliases for consistency:
-import { userFollows, users } from '@/users/users.model'; -import { articles, favoriteArticles } from '@articles/articles.model'; +import { articles, favoriteArticles } from '@/articles/articles.model'; +import { userFollows, users } from '@/users/users.model';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
db/migrations/meta/0007_snapshot.json(1 hunks)db/migrations/meta/_journal.json(1 hunks)src/articles/articles.plugin.ts(1 hunks)src/articles/articles.repository.ts(1 hunks)src/articles/articles.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- db/migrations/meta/_journal.json
- src/articles/articles.service.ts
- src/articles/articles.plugin.ts
🧰 Additional context used
🧠 Learnings (1)
src/articles/articles.repository.ts (1)
Learnt from: Hajbo
PR: agnyz/bedstack#129
File: src/articles/articles.repository.ts:13-108
Timestamp: 2025-03-17T17:22:55.819Z
Learning: When using Drizzle ORM with nullable parameters in SQL expressions, handling null checks in TypeScript before constructing the SQL query can help avoid type inference issues, rather than using SQL CASE WHEN statements.
🧬 Code Definitions (1)
src/articles/articles.repository.ts (4)
src/database.providers.ts (2) (2)
db(11-14)Database(15-15)src/users/users.model.ts (2) (2)
users(12-23)userFollows(32-45)src/articles/articles.model.ts (2) (2)
articles(14-26)favoriteArticles(39-52)src/articles/articles.schema.ts (2) (2)
ArticleToCreate(20-23)ArticleToUpdate(39-41)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/articles/articles.repository.ts (1)
170-187: 🛠️ Refactor suggestionReturn the updated article from updateArticle method.
The
updateArticlemethod doesn't return the updated article, making it difficult for callers to access the updated data.Return the updated article after updating:
async updateArticle( articleId: number, article: ArticleToUpdate, currentUserId: number, ) { const filteredArticle = Object.fromEntries( Object.entries(article).filter(([_, value]) => value !== undefined), ); await this.db .update(articles) .set({ ...filteredArticle, updatedAt: new Date(), }) .where( and(eq(articles.id, articleId), eq(articles.authorId, currentUserId)), ); + + // Return the updated article + return this.findById(articleId); }
🧹 Nitpick comments (2)
src/articles/articles.repository.ts (2)
189-195: Improve error handling for deleteArticle method.The
deleteArticlemethod doesn't check if the article exists or if the delete operation was successful.Consider adding error handling and returning a more informative result:
async deleteArticle(slug: string, currentUserId: number) { - return await this.db - .delete(articles) - .where( - and(eq(articles.slug, slug), eq(articles.authorId, currentUserId)), - ); + // First check if the article exists and belongs to the user + const article = await this.db.query.articles.findFirst({ + where: and(eq(articles.slug, slug), eq(articles.authorId, currentUserId)), + columns: { id: true }, + }); + + if (!article) { + return { deleted: false, reason: 'Article not found or not owned by the user' }; + } + + await this.db + .delete(articles) + .where(eq(articles.id, article.id)); + + return { deleted: true }; }
1-196: Ensure proper transaction handling for database operations.The repository methods perform individual database operations, but there's no transaction management for operations that might require atomicity.
Consider adding transaction support for operations that modify data:
// Example for createArticle async createArticle(article: ArticleToCreate) { return await this.db.transaction(async (tx) => { const results = await tx.insert(articles).values(article).returning(); const newArticle = results[0]; // Perform any additional operations within the transaction return this.findById(newArticle.id); }); }This ensures that all operations within the method either complete successfully or are rolled back in case of an error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/articles/articles.repository.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/articles/articles.repository.ts (1)
Learnt from: Hajbo
PR: agnyz/bedstack#129
File: src/articles/articles.repository.ts:13-108
Timestamp: 2025-03-17T17:22:55.819Z
Learning: When using Drizzle ORM with nullable parameters in SQL expressions, handling null checks in TypeScript before constructing the SQL query can help avoid type inference issues, rather than using SQL CASE WHEN statements.
🧬 Code Definitions (1)
src/articles/articles.repository.ts (4)
src/database.providers.ts (2) (2)
db(11-14)Database(15-15)src/users/users.model.ts (2) (2)
users(12-23)userFollows(32-45)src/articles/articles.model.ts (2) (2)
articles(14-26)favoriteArticles(39-52)src/articles/articles.schema.ts (2) (2)
ArticleToCreate(20-23)ArticleToUpdate(39-41)
🔇 Additional comments (3)
src/articles/articles.repository.ts (3)
13-130: Handle null checks for currentUserId in SQL expressions.The SQL expressions for authorsWithFollowers and articlesWithLikes might have unexpected behavior when
currentUserIdis null. Based on the retrieved learning, handling null checks in TypeScript is preferred over SQL CASE WHEN statements.Apply TypeScript null checks before constructing the SQL expressions:
- authorFollowing: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( - 'authorFollowing', - ), + authorFollowing: currentUserId === null + ? sql<boolean>`false`.as('authorFollowing') + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(user_follows.follower_id)), false)`.as( + 'authorFollowing', + ),And similarly for the articlesWithLikes CTE:
- favorited: - sql<boolean>`coalesce(${currentUserId} = any(array_agg(favorite_articles.user_id)), false)`.as( - 'favorited', - ), + favorited: currentUserId === null + ? sql<boolean>`false`.as('favorited') + : sql<boolean>`coalesce(${currentUserId} = any(array_agg(favorite_articles.user_id)), false)`.as( + 'favorited', + ),
132-146: Add missing fields to findBySlug return data.The
findBySlugmethod doesn't explicitly include all necessary article fields which could lead to missing data.Explicitly include all required fields:
async findBySlug(slug: string) { const result = await this.db.query.articles.findFirst({ where: eq(articles.slug, slug), + columns: { + id: true, + slug: true, + title: true, + description: true, + body: true, + tagList: true, + createdAt: true, + updatedAt: true, + }, with: { author: { with: { followers: true, }, }, favoritedBy: true, }, }); if (!result) return null; return result; }
148-162: Add missing fields to findById return data.Similar to the
findBySlugmethod, thefindByIdmethod doesn't explicitly include all necessary article fields.Explicitly include all required fields:
async findById(id: number) { const result = await this.db.query.articles.findFirst({ where: eq(articles.id, id), + columns: { + id: true, + slug: true, + title: true, + description: true, + body: true, + tagList: true, + createdAt: true, + updatedAt: true, + }, with: { author: { with: { followers: true, }, }, favoritedBy: true, }, }); if (!result) return null; return result; }
- Article models and related migrations - Article endpoints - create - update - delete - get by slug - list - feed - Fix drop utility by dropping migrations table as well - Swagger id token auth. This closes #107 as well. That PR didn't work out of the box (it uses an invalid token format, Bearer instead of Token as per specified in the [docs](https://realworld-docs.netlify.app/specifications/backend/endpoints/)), but it gave the inspiration. - [x] Read the [CONTRIBUTING]( https://github.com/agnyz/bedstack/blob/main/CONTRIBUTING.md) guide - [x] Title this PR according to the `type(scope): description` or `type: description` format - [x] Provide description sufficient to understand the changes introduced in this PR, and, if necessary, some screenshots - [x] Reference an issue or discussion where the feature or changes have been previously discussed - [x] Add a failing test that passes with the changes introduced in this PR, or explain why it's not feasible - [x] Add documentation for the feature or changes introduced in this PR to the docs; you can run them with `bun docs` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced full article management capabilities, including endpoints for listing, creating, updating, deleting articles, and fetching personalized feeds. - Enhanced API security with token authentication and persistent authorization. - Expanded the database to support new content types and user interactions, such as published and favorited articles. - Improved profile response handling to better reflect user following status. - Added a utility function for generating URL-friendly slugs. - Added a new configuration for database migrations and expanded the database schema to include articles and favorite articles. - **Bug Fixes** - Adjusted error handling in profile response generation to accommodate cases with no current user ID. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
### Description <!-- Provide a comprehensive description here about what your PR aims to solve. --> Implement comment feature for articles. <!-- You may also add additional context --> Builds on #129 so it's better to merge that first. --- ### PR Checklist <!-- Please do not remove this section --> <!-- Mark each item with an "x" ([ ] becomes [x]) --> - [x] Read the Developer's Guide in [CONTRIBUTING.md](https://github.com/agnyz/bedstack/blob/main/CONTRIBUTING.md) - [x] Use a concise title to represent the changes introduced in this PR - [x] Provide a detailed description of the changes introduced in this PR, and, if necessary, some screenshots - [x] Reference an issue or discussion where the feature or changes have been previously discussed - [x] Add a failing test that passes with the changes introduced in this PR, or explain why it's not feasible - [x] Add documentation for the feature or changes introduced in this PR to the docs; you can run them with `bun docs` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Expanded article capabilities to include creation, updates, deletions, and personalized feeds. - Introduced full comment management, allowing users to add, view, and delete comments on articles. - Improved profile retrieval for a more dynamic and accurate display of user information. - Added functionality to manage favorited articles and their associated comments. - Enhanced schema definitions for articles and comments, providing a robust framework for management. - New methods for retrieving user profiles based on user ID. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Hajbo <hajbodev@gmail.com> Co-authored-by: Hajbo <35660161+Hajbo@users.noreply.github.com>
Description
PR Checklist (Please do not remove)
type(scope): descriptionortype: descriptionformatbun docsSummary by CodeRabbit
New Features
Bug Fixes