Feature/ Add series to link related articles #1146
Conversation
|
@RangerCreaky is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces a "Series" feature to the application, allowing users to link related articles. It adds a new input field for Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RangerCreaky
temporarily deployed
to
production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
schema/series.ts (1)
3-6: LGTM: UpdateSeriesSchema is well-defined, with minor suggestions.The
UpdateSeriesSchemacorrectly defines the structure for updating series information, aligning with the PR objectives. The optionalseriesNameallows for removing a post from a series, and trimming prevents whitespace issues.Consider the following suggestions for potential improvements:
- Add a minimum length constraint to
postIdto ensure it's not empty.- Consider adding a maximum length constraint to
seriesNameto prevent excessively long series names.- You might want to add a regex pattern to
postIdif it follows a specific format.Example implementation:
export const UpdateSeriesSchema = z.object({ postId: z.string().min(1, "Post ID cannot be empty"), seriesName: z.string().trim().max(100, "Series name cannot exceed 100 characters").optional() });drizzle/0011_add_series_update_post.sql (3)
1-9: LGTM! Consider adding a trigger for "updatedAt".The "Series" table structure is well-designed and appropriate for the feature. Good use of constraints and data types.
Consider adding a trigger to automatically update the "updatedAt" column when a row is modified. This ensures the column is always up-to-date without relying on application logic. Here's an example:
CREATE OR REPLACE FUNCTION update_updated_at_column() RETURNS TRIGGER AS $$ BEGIN NEW."updatedAt" = NOW(); RETURN NEW; END; $$ language 'plpgsql'; CREATE TRIGGER update_series_updated_at BEFORE UPDATE ON "Series" FOR EACH ROW EXECUTE FUNCTION update_updated_at_column();
10-16: LGTM! Consider adding an index on "seriesId".The alteration to the "Post" table is well-structured and correctly implements the relationship between posts and series.
Consider adding an index on the "seriesId" column to improve query performance when filtering or joining posts by series. Here's an example:
CREATE INDEX idx_post_series_id ON "Post" ("seriesId");This index will be particularly useful if you frequently query posts by their series or join the "Post" and "Series" tables.
1-16: Overall implementation aligns well with PR objectives.The SQL script successfully implements the database schema changes required for the series feature. It creates a new "Series" table and modifies the "Post" table to establish the relationship between posts and series. This implementation supports the CREATE, EDIT, and DELETE cases described in the PR objectives.
The changes align with the requirements outlined in the linked issue #1081, providing the necessary database structure to:
- Allow specifying a series for articles
- Enable navigation between related articles
- Support indexing articles within a series
As the feature develops, consider the following:
- Implement appropriate indexing strategies as the data grows.
- Ensure that application logic handles the ordering of articles within a series, possibly by adding an "order" column to the "Post" table in the future.
- Plan for efficient querying of series and their associated posts to support the navigation features described in the issue.
server/api/router/index.ts (1)
Line range hint
11-21: Summary: Series feature successfully integrated into the main router.The changes in this file effectively integrate the new series feature into the application's routing system. The addition of the
seriesRouterimport and its inclusion in theappRouterare done consistently with other features, minimizing the risk of integration issues. These changes provide the necessary foundation for the series functionality to be accessible throughout the application, aligning well with the PR objectives.A few points to consider:
- Ensure that the
series.tsfile is properly implemented and tested.- Verify that any necessary middleware or authentication for the series routes are correctly set up in the
seriesRouter.- Update any relevant documentation to reflect the new series feature and its routing.
schema/post.ts (2)
28-28: LGTM! Consider adding a maximum length constraint.The addition of the
seriesNamefield toSavePostSchemais well-implemented and aligns with the PR objectives. It's correctly defined as an optional trimmed string.Consider adding a maximum length constraint to prevent excessively long series names:
- seriesName: z.string().trim().optional() + seriesName: z.string().trim().max(100, "Max series name length is 100 characters.").optional()
54-54: LGTM! Consider adding a maximum length constraint.The addition of the
seriesNamefield toConfirmPostSchemais consistent with the change inSavePostSchemaand appropriate for the post confirmation process.For consistency with the suggested improvement in
SavePostSchema, consider adding a maximum length constraint:- seriesName: z.string().trim().optional() + seriesName: z.string().trim().max(100, "Max series name length is 100 characters.").optional()server/api/router/series.ts (1)
54-58: IncludecreatedAtField When Inserting a New SeriesWhen creating a new series, setting the
createdAttimestamp ensures accurate record-keeping and consistency.Apply this diff to add the
createdAtfield:await tx.insert(series).values({ title: seriesTitle, userId: ctx.session.user.id, + createdAt: new Date(), updatedAt: new Date() }).returning();server/api/router/post.ts (1)
190-210: Rename inner 'deletedPost' variable to avoid shadowing and improve clarityInside the transaction, the variable
deletedPostis redeclared. This shadows the outerdeletedPostvariable and can lead to confusion. Consider renaming the inner variable to something likedeletedPostDatato enhance readability.server/db/schema.ts (1)
40-42: Add a unique constraint on 'title' per userTo prevent users from creating multiple series with the same title, consider adding a unique index on
(userId, title). This ensures each user has uniquely titled series, enhancing data integrity.Apply this change:
export const series = pgTable("Series", { id: serial("id").primaryKey(), title: text("title").notNull(), description: text("description"), userId: text("userId").notNull().references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }), +}, (table) => { + return { + userTitleUniqueIndex: uniqueIndex("Series_userId_title_unique").on(table.userId, table.title), + }; +});app/(app)/create/[[...paramsArr]]/_client.tsx (1)
581-586: Add validation for series name inputTo prevent users from entering series names that are too long or contain invalid characters, consider adding validation to the
seriesNameinput field.Apply this diff to add a maximum length attribute and basic pattern validation:
<input id="seriesName" type="text" placeholder="The name of my series" + maxLength={100} + pattern="^[\w\s\-]+$" defaultValue={data?.series?.title || ""} {...register("seriesName")} />This adds a maximum length of 100 characters and restricts input to letters, numbers, spaces, underscores, and hyphens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- app/(app)/create/[[...paramsArr]]/_client.tsx (4 hunks)
- drizzle/0011_add_series_update_post.sql (1 hunks)
- schema/post.ts (2 hunks)
- schema/series.ts (1 hunks)
- server/api/router/index.ts (2 hunks)
- server/api/router/post.ts (3 hunks)
- server/api/router/series.ts (1 hunks)
- server/db/schema.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (11)
schema/series.ts (2)
1-1: LGTM: Zod import is correct.The import statement correctly imports the Zod library, which is necessary for defining the schema in this file.
1-6: Overall assessment: Well-implemented schema for series updates.The
schema/series.tsfile successfully introduces theUpdateSeriesSchema, which aligns well with the PR objectives for implementing a "Series" feature. The schema correctly handles the core requirements:
- It allows updating a post's series information.
- It supports removing a post from a series (by making
seriesNameoptional).- It prevents whitespace issues in series names.
The implementation provides a solid foundation for validating series update operations. Consider the minor suggestions provided earlier to further enhance the robustness of the schema.
server/api/router/index.ts (2)
21-21: LGTM! Series router successfully integrated.The
seriesRouteris correctly added to theappRouterobject, following the established pattern in this file. This integration makes the new series functionality available through the main application router, which aligns with the PR objectives.
11-11: LGTM! Verify the existence of theseries.tsfile.The import statement for
seriesRouteris correctly added and follows the established pattern in this file. This change aligns with the PR objectives of implementing a new series feature.To ensure the
series.tsfile exists, run the following script:✅ Verification successful
Verification Successful!
The
series.tsfile exists in theserver/api/router/directory as required. The import statement forseriesRouteris correct and aligns with the PR objectives of implementing the new series feature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the series.ts file in the same directory # Test: Check if the series.ts file exists if fd -p "server/api/router/series.ts"; then echo "series.ts file found" else echo "series.ts file not found" fiLength of output: 114
schema/post.ts (1)
28-28: Verify the impact ofseriesNameaddition on related components and API endpoints.The addition of the
seriesNamefield toSavePostSchemaandConfirmPostSchemais well-implemented and consistent. To ensure full integration of this new feature:Please run the following script to check for any components or API endpoints that might need updating to handle the new
seriesNamefield:Also applies to: 54-54
server/api/router/series.ts (1)
1-129: Overall Code Structure and ImplementationThe implementation of the
seriesRouterand theupdatemethod effectively handles series management for posts. The use of transactions ensures data integrity during database operations.server/api/router/post.ts (3)
17-17: Importing 'series' model is necessary for series functionalityThe addition of
seriesto the import statement ensures that the series model is available in this file, enabling series-related operations.
448-448: Including 'series' in the query fetches associated series dataAdding
series: truein thewithclause of theeditDraftquery ensures that the series data associated with the post is fetched. This is essential for editing drafts that are part of a series.
190-210:⚠️ Potential issueEnsure correct usage of transaction methods
Within the transaction, verify that using
tx.query.post.findFirstis appropriate according to your ORM's API. Depending on the ORM, the transaction objecttxmight not have aqueryproperty, and you may need to usetx.select,tx.query, or another method.To confirm the correct methods available on the transaction object, run the following script:
server/db/schema.ts (2)
172-172: Verify 'onDelete' behavior for 'seriesId' foreign key in 'post' tableThe foreign key for
seriesIdin theposttable usesonDelete: "set null". This means that when a series is deleted, theseriesIdin associated posts will be set tonull. Ensure this aligns with the desired application behavior. If you prefer posts to be deleted when their associated series is deleted, consider changingonDeleteto"cascade".
298-304: LGTM!The
seriesRelationsare correctly defined, establishing the appropriate relationships betweenseries,posts, anduser.
server/api/router/series.ts
Outdated
| await tx.delete(series).where(eq(series.id, seriesId)); | ||
| } |
There was a problem hiding this comment.
Ensure Only the User's Series is Deleted
When deleting a series, the current code removes any series matching the seriesId, regardless of ownership. This could inadvertently delete another user's series with the same ID.
Modify the where clause to include userId, ensuring only the current user's series is deleted:
- await tx.delete(series).where(eq(series.id, seriesId));
+ await tx.delete(series).where(
+ and(
+ eq(series.id, seriesId),
+ eq(series.userId, ctx.session.user.id)
+ )
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await tx.delete(series).where(eq(series.id, seriesId)); | |
| } | |
| await tx.delete(series).where( | |
| and( | |
| eq(series.id, seriesId), | |
| eq(series.userId, ctx.session.user.id) | |
| ) | |
| ); | |
| } |
There was a problem hiding this comment.
Since the seriesId is being picked from the currentPost , so this comparison is not necessary in my opinion. Had the comparison been done based on series title, then comparing the user id would have been important.
However, this check is alse added in the latest commit.
There was a problem hiding this comment.
@RangerCreaky, thank you for addressing the issue and adding the check in your latest commit. I appreciate your attention to detail.
✏️ Learnings added
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/api/router/series.ts:0-0
Timestamp: 2024-10-18T04:13:45.764Z
Learning: In the `seriesRouter` in `server/api/router/series.ts`, when deleting a series using the `seriesId` from `currentPost`, additional `userId` checks may not be necessary because unauthorized access to `currentPost` is already prevented.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
server/api/router/series.ts
Outdated
| where: (post, { eq, and, ne }) => | ||
| and ( | ||
| ne(post.id, currentPost.id), | ||
| eq(post.seriesId, currentPost.seriesId!) | ||
| ) |
There was a problem hiding this comment.
Fix Incorrect Reference to currentPost.seriesId in unlinkSeries Function
In the unlinkSeries function, the condition uses currentPost.seriesId!, which may not align with the seriesId parameter passed to the function. This could lead to unexpected behavior if currentPost.seriesId has changed.
Apply this diff to use the seriesId parameter for accurate matching:
- eq(post.seriesId, currentPost.seriesId!)
+ eq(post.seriesId, seriesId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| where: (post, { eq, and, ne }) => | |
| and ( | |
| ne(post.id, currentPost.id), | |
| eq(post.seriesId, currentPost.seriesId!) | |
| ) | |
| where: (post, { eq, and, ne }) => | |
| and ( | |
| ne(post.id, currentPost.id), | |
| eq(post.seriesId, seriesId) | |
| ) |
There was a problem hiding this comment.
Done in the latest commit
There was a problem hiding this comment.
@RangerCreaky, thank you for fixing the issue! 😊
(_/)
(•_•)
/ 🐰
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
server/api/router/series.ts
Outdated
| const currSeries = await tx.query.series.findFirst({ | ||
| columns: { | ||
| id: true | ||
| }, | ||
| where: (series, { eq }) => eq(series.title, seriesTitle), | ||
| }) |
There was a problem hiding this comment.
Prevent Cross-User Series Linking by Including userId in Series Query
Currently, when checking for an existing series, the query filters only by series.title. This could allow a user to link their post to a series owned by another user if the series titles match.
Apply this diff to include userId in the where clause to ensure the series belongs to the current user:
- where: (series, { eq }) => eq(series.title, seriesTitle),
+ where: (series, { eq, and }) => and(
+ eq(series.title, seriesTitle),
+ eq(series.userId, ctx.session.user.id)
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currSeries = await tx.query.series.findFirst({ | |
| columns: { | |
| id: true | |
| }, | |
| where: (series, { eq }) => eq(series.title, seriesTitle), | |
| }) | |
| const currSeries = await tx.query.series.findFirst({ | |
| columns: { | |
| id: true | |
| }, | |
| where: (series, { eq, and }) => and( | |
| eq(series.title, seriesTitle), | |
| eq(series.userId, ctx.session.user.id) | |
| ), | |
| }) |
There was a problem hiding this comment.
done in the latest commit
There was a problem hiding this comment.
@RangerCreaky, glad to hear that the issue has been resolved! 🎉
(^o^/)
(='.'=)
(")_(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| if(seriesName){ | ||
| // check if the current post is already linked to a series | ||
| if(currentPost?.seriesId){ | ||
| // check if the series title is same as the current series name | ||
| // then we do nothing | ||
| if(currentPost?.series?.title !== seriesName){ | ||
| // then the user has updated the series name in this particular edit | ||
| // Check if there is another post with the same title, else delete the series | ||
| // and create a new post with the new series name | ||
| // and update that new series id in the post | ||
| await unlinkSeries(currentPost.seriesId); | ||
| await createNewSeries(seriesName); | ||
| } | ||
| } | ||
| else{ | ||
| // the current post is not yet linked to a seriesId | ||
| // so create a new series and put that Id in the post | ||
| await createNewSeries(seriesName); | ||
| } | ||
| } | ||
| else{ | ||
| // either the user has not added the series Name (We do nothing) | ||
| // or while editing the post, the user has removed the series name | ||
| if(currentPost.seriesId !== null){ | ||
| await unlinkSeries(currentPost.seriesId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Conditional Logic for Better Readability
The nested if-else statements can be refactored for clarity and maintainability. Simplifying the conditions will make the code easier to understand and reduce potential errors.
Consider restructuring the logic as follows:
if (seriesName) {
if (currentPost.seriesId) {
if (currentPost.series.title !== seriesName) {
await unlinkSeries(currentPost.seriesId);
await createNewSeries(seriesName);
}
// No action needed if titles match
} else {
await createNewSeries(seriesName);
}
} else if (currentPost.seriesId !== null) {
await unlinkSeries(currentPost.seriesId);
}
// No action needed if seriesName is not provided and no seriesId is linked📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(seriesName){ | |
| // check if the current post is already linked to a series | |
| if(currentPost?.seriesId){ | |
| // check if the series title is same as the current series name | |
| // then we do nothing | |
| if(currentPost?.series?.title !== seriesName){ | |
| // then the user has updated the series name in this particular edit | |
| // Check if there is another post with the same title, else delete the series | |
| // and create a new post with the new series name | |
| // and update that new series id in the post | |
| await unlinkSeries(currentPost.seriesId); | |
| await createNewSeries(seriesName); | |
| } | |
| } | |
| else{ | |
| // the current post is not yet linked to a seriesId | |
| // so create a new series and put that Id in the post | |
| await createNewSeries(seriesName); | |
| } | |
| } | |
| else{ | |
| // either the user has not added the series Name (We do nothing) | |
| // or while editing the post, the user has removed the series name | |
| if(currentPost.seriesId !== null){ | |
| await unlinkSeries(currentPost.seriesId); | |
| } | |
| } | |
| if (seriesName) { | |
| if (currentPost.seriesId) { | |
| if (currentPost.series.title !== seriesName) { | |
| await unlinkSeries(currentPost.seriesId); | |
| await createNewSeries(seriesName); | |
| } | |
| // No action needed if titles match | |
| } else { | |
| await createNewSeries(seriesName); | |
| } | |
| } else if (currentPost.seriesId !== null) { | |
| await unlinkSeries(currentPost.seriesId); | |
| } | |
| // No action needed if seriesName is not provided and no seriesId is linked |
There was a problem hiding this comment.
The changes suggested are to remove the comments
However, keeping it for now, for the ease of the reviewers
There was a problem hiding this comment.
@RangerCreaky, understood. Keeping the comments for the ease of reviewers makes sense.
(_/)
(•︵•)
('')('')
✏️ Learnings added
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/api/router/series.ts:109-135
Timestamp: 2024-10-18T04:18:28.426Z
Learning: In the TypeScript file `server/api/router/series.ts`, when comments are included to aid reviewers, avoid suggesting their removal.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const currentPost = await ctx.db.query.post.findFirst({ | ||
| columns: { | ||
| id: true, | ||
| seriesId: true, | ||
| userId: true | ||
| }, | ||
| with: { | ||
| series: { | ||
| columns: { | ||
| id: true, | ||
| title: true | ||
| }, | ||
| }, | ||
| }, | ||
| where: (post, { eq }) => eq(post.id, postId), | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Enhance postId Validation with Specific Constraints
Currently, postId is validated as a string. To further mitigate security risks, please implement more specific validation rules (e.g., ensure postId follows a UUID format if applicable).
🔗 Analysis chain
Validate postId and Handle Potential Security Risks
While fetching currentPost, ensure that postId is validated to prevent SQL injection or unauthorized access. Although TRPC and the ORM handle parameterization, additional validation enhances security.
Run the following script to check the validation of postId:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `UpdateSeriesSchema` properly validates `postId`
# Test: Check the schema definition for `postId` validation
rg --type typescript "UpdateSeriesSchema"
Length of output: 76
Script:
#!/bin/bash
# Description: Verify that `UpdateSeriesSchema` properly validates `postId`
# Test: Check the schema definition for `postId` validation without specifying file type
rg "UpdateSeriesSchema"
# Alternatively, restrict the search to TypeScript files using a glob pattern
rg "UpdateSeriesSchema" --glob "*.ts" --glob "*.tsx"
Length of output: 490
Script:
#!/bin/bash
# Description: Extract the definition of `UpdateSeriesSchema` to verify `postId` validation
# Extract lines surrounding the `UpdateSeriesSchema` definition in schema/series.ts
rg "export const UpdateSeriesSchema" schema/series.ts -A 10 -B 5
Length of output: 206
| updatedAt: timestamp("updatedAt", { | ||
| precision: 3, | ||
| withTimezone: true | ||
| }).notNull() | ||
| .$onUpdate(() => new Date()) | ||
| .default(sql`CURRENT_TIMESTAMP`), |
There was a problem hiding this comment.
Ensure consistency in 'updatedAt' timestamp configuration
In the series table, the updatedAt field is missing the mode: "string" option and uses .$onUpdate(() => new Date()), whereas other tables like post use mode: "string" and .$onUpdate(() => new Date().toISOString()). For consistency and to prevent potential type mismatches, consider adding mode: "string" and using .$onUpdate(() => new Date().toISOString()).
Apply this diff to ensure consistency:
updatedAt: timestamp("updatedAt", {
precision: 3,
+ mode: "string",
withTimezone: true
}).notNull()
- .$onUpdate(() => new Date())
+ .$onUpdate(() => new Date().toISOString())
.default(sql`CURRENT_TIMESTAMP`),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updatedAt: timestamp("updatedAt", { | |
| precision: 3, | |
| withTimezone: true | |
| }).notNull() | |
| .$onUpdate(() => new Date()) | |
| .default(sql`CURRENT_TIMESTAMP`), | |
| updatedAt: timestamp("updatedAt", { | |
| precision: 3, | |
| mode: "string", | |
| withTimezone: true | |
| }).notNull() | |
| .$onUpdate(() => new Date().toISOString()) | |
| .default(sql`CURRENT_TIMESTAMP`), |
There was a problem hiding this comment.
Keeping it as in the date time format, as mentioned in the issue description
There was a problem hiding this comment.
@RangerCreaky, understood. We'll keep the datetime format as per the issue description.
(_/)
( •_•)
/
✏️ Learnings added
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/db/schema.ts:50-55
Timestamp: 2024-10-18T04:16:22.358Z
Learning: In `server/db/schema.ts`, for the `series` table, the `updatedAt` field uses `.$onUpdate(() => new Date())` without `mode: "string"`, as per the issue description. This is intentional, and we should not suggest changing it for consistency with other tables.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| let saveSuccess = false; | ||
| try { | ||
| await save({ ...formData, id: postId }); | ||
| saveSuccess = true; | ||
| } catch (error) { | ||
| toast.error("Error saving post."); | ||
| Sentry.captureException(error); | ||
| } | ||
|
|
||
| let seriesUpdateSuccess = false; | ||
| try { | ||
| if(formData?.seriesName){ | ||
| await seriesUpdate({ postId, seriesName: formData.seriesName }); | ||
| } | ||
| seriesUpdateSuccess = true; | ||
| } catch (error) { | ||
| toast.error("Error updating series."); | ||
| Sentry.captureException(error); | ||
| } | ||
|
|
||
| if(saveSuccess && seriesUpdateSuccess){ | ||
| toast.success("Saved"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix logical error in series update success handling
There is a logical issue in the way seriesUpdateSuccess is initialized and evaluated. If formData.seriesName is undefined, the seriesUpdate function is not called, but seriesUpdateSuccess is still set to true. This may prevent the success toast from displaying even when the post is saved successfully.
To ensure that the success toast appears appropriately, initialize seriesUpdateSuccess to true before the try block, and only set it to false if an error occurs during the series update. This way, if seriesUpdate is not called due to the absence of seriesName, seriesUpdateSuccess remains true.
Apply this diff to fix the issue:
- let seriesUpdateSuccess = false;
+ let seriesUpdateSuccess = true;
try {
if(formData?.seriesName){
await seriesUpdate({ postId, seriesName: formData.seriesName });
}
- seriesUpdateSuccess = true;
} catch (error) {
toast.error("Error updating series.");
Sentry.captureException(error);
+ seriesUpdateSuccess = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let saveSuccess = false; | |
| try { | |
| await save({ ...formData, id: postId }); | |
| saveSuccess = true; | |
| } catch (error) { | |
| toast.error("Error saving post."); | |
| Sentry.captureException(error); | |
| } | |
| let seriesUpdateSuccess = false; | |
| try { | |
| if(formData?.seriesName){ | |
| await seriesUpdate({ postId, seriesName: formData.seriesName }); | |
| } | |
| seriesUpdateSuccess = true; | |
| } catch (error) { | |
| toast.error("Error updating series."); | |
| Sentry.captureException(error); | |
| } | |
| if(saveSuccess && seriesUpdateSuccess){ | |
| toast.success("Saved"); | |
| } | |
| let saveSuccess = false; | |
| try { | |
| await save({ ...formData, id: postId }); | |
| saveSuccess = true; | |
| } catch (error) { | |
| toast.error("Error saving post."); | |
| Sentry.captureException(error); | |
| } | |
| let seriesUpdateSuccess = true; | |
| try { | |
| if(formData?.seriesName){ | |
| await seriesUpdate({ postId, seriesName: formData.seriesName }); | |
| } | |
| } catch (error) { | |
| toast.error("Error updating series."); | |
| Sentry.captureException(error); | |
| seriesUpdateSuccess = false; | |
| } | |
| if(saveSuccess && seriesUpdateSuccess){ | |
| toast.success("Saved"); | |
| } |
| <p className="mt-2 mb-2 text-sm text-neutral-600 dark:text-neutral-400"> | ||
| Share this link with others to preview your | ||
| draft. Anyone with the link can view your draft. | ||
| </p> | ||
|
|
||
| <label htmlFor="seriesName"> | ||
| Series Name | ||
| </label> | ||
| <input | ||
| id="seriesName" | ||
| type="text" | ||
| placeholder="The name of my series" | ||
| defaultValue={data?.series?.title || ""} | ||
| {...register("seriesName")} | ||
| /> | ||
| <p className="mt-2 text-sm text-neutral-600 dark:text-neutral-400"> | ||
| This text is case-sensitive so make sure you type it exactly as you did in previous articles to ensure they are connected | ||
| </p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize series names to be case-insensitive
The series name field is currently case-sensitive, which may lead to user errors if the series name is not typed exactly the same in each article. This could result in related articles not being linked correctly.
Consider normalizing series names by converting them to lowercase (or a consistent case) when storing and comparing them in the database. This will enhance the user experience by ensuring that articles are correctly linked in a series, regardless of the case used when entering the series name.
Apply this diff to handle case normalization on the client side:
const formData = {
...data,
tags,
canonicalUrl: data.canonicalUrl || undefined,
excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155),
- seriesName: data.seriesName || undefined
+ seriesName: data.seriesName ? data.seriesName.toLowerCase() : undefined
};Ensure that the server-side logic also handles series names in a case-insensitive manner.
Committable suggestion was skipped due to low confidence.
RangerCreaky
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/api/router/series.ts (2)
36-36: Remove unnecessary optional chainingSince
currentPosthas been confirmed to exist earlier in the code, the optional chaining incurrentPost?.userIdis unnecessary. AccesscurrentPost.userIddirectly for clarity and consistency.Apply this diff:
- if (currentPost?.userId !== ctx.session.user.id) { + if (currentPost.userId !== ctx.session.user.id) {
57-61: Include 'createdAt' timestamp when creating a new seriesIn the
createNewSeriesfunction, only theupdatedAtfield is set when inserting a new series. If yourseriestable includes acreatedAtfield, it's advisable to set it during insertion to maintain accurate timestamps.Apply this diff:
await tx.insert(series).values({ title: seriesTitle, userId: ctx.session.user.id, + createdAt: new Date(), updatedAt: new Date() }).returning();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/api/router/series.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
server/api/router/series.ts (1)
83-88:⚠️ Potential issueEnsure 'unlinkSeries' considers only the current user's posts
The query in the
unlinkSeriesfunction checks for other posts linked to the same series but does not filter byuserId. This could lead to incorrect behavior if other users have posts linked to the same series. IncludeuserIdin the query to ensure that only the current user's posts are considered.Apply this diff to modify the query:
const anotherPostInThisSeries = await tx.query.post.findFirst({ where: (post, { eq, and, ne }) => and ( ne(post.id, currentPost.id), eq(post.seriesId, seriesId), + eq(post.userId, ctx.session.user.id) ) })⛔ Skipped due to learnings
Learnt from: RangerCreaky PR: codu-code/codu#1146 File: server/api/router/series.ts:0-0 Timestamp: 2024-10-18T04:13:45.950Z Learning: In the `seriesRouter` in `server/api/router/series.ts`, when deleting a series using the `seriesId` from `currentPost`, additional `userId` checks may not be necessary because unauthorized access to `currentPost` is already prevented.
|
@NiallJoeMaher please review and let me know the changes I need to make |
|
I'm out of the office at the moment so will try get to this soon. Need to do some testing on it. 🦾 |
|
Sure, I am up for it |
| @@ -25,6 +25,7 @@ export const SavePostSchema = z.object({ | |||
| canonicalUrl: z.optional(z.string().trim().url()), | |||
| tags: z.string().array().max(5).optional(), | |||
| published: z.string().datetime().optional(), | |||
| seriesName: z.string().trim().optional() | |||
There was a problem hiding this comment.
Should this be seriesId ?
In the drizzle migration you added a FK constraint for seriesId but I only see seriesName on the post schema
There was a problem hiding this comment.
Hey @JohnAllenTech , Sorry I was away for the past 2 days, so couldn't reply
You are right, the foreign key is seriesId. However, the as of my knowledge, this schema (savePostSchema) is the input for the update Procedure in the post router
` update: protectedProcedure
.input(SavePostSchema)
.mutation(async ({ input, ctx }) => {
const { id, body, title, excerpt, canonicalUrl, tags = [] } = input;
const currentPost = await ctx.db.query.post.findFirst({
where: (posts, { eq }) => eq(posts.id, id),
});
`
This method takes the seriesName as input, and creates a new record in the series. and updates the seriesId in the posts table.
The code below has the schema for the post table which has the seriesId
(db/schema.ts)
.references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }), showComments: boolean("showComments").default(true).notNull(), likes: integer("likes").default(0).notNull(), seriesId: integer("seriesId").references(() => series.id, { onDelete: "set null", onUpdate: "cascade" }),
There was a problem hiding this comment.
I also had the same question as @JohnAllenTech since the series name may not be unique and we could have collisions while trying to find the relevant series. I think this needs to be seriesId.
|
@NiallJoeMaher @JohnAllenTech any update on this? |
| @@ -25,6 +25,7 @@ export const SavePostSchema = z.object({ | |||
| canonicalUrl: z.optional(z.string().trim().url()), | |||
| tags: z.string().array().max(5).optional(), | |||
| published: z.string().datetime().optional(), | |||
| seriesName: z.string().trim().optional() | |||
There was a problem hiding this comment.
I also had the same question as @JohnAllenTech since the series name may not be unique and we could have collisions while trying to find the relevant series. I think this needs to be seriesId.
| -- Create Series table | ||
| CREATE TABLE IF NOT EXISTS "Series" ( | ||
| "id" SERIAL PRIMARY KEY, | ||
| "title" TEXT NOT NULL, |
There was a problem hiding this comment.
Could we change this to "name"
|
|
||
| export const UpdateSeriesSchema = z.object({ | ||
| postId: z.string(), | ||
| seriesName: z.string().trim().optional() |
There was a problem hiding this comment.
This isn't unique so could cause issues when we join.
| @@ -50,6 +51,7 @@ export const ConfirmPostSchema = z.object({ | |||
| .optional(), | |||
| canonicalUrl: z.string().trim().url().optional(), | |||
| tags: z.string().array().max(5).optional(), | |||
| seriesName: z.string().trim().optional() | |||
| CREATE TABLE IF NOT EXISTS "Series" ( | ||
| "id" SERIAL PRIMARY KEY, | ||
| "title" TEXT NOT NULL, | ||
| "description" TEXT, |
There was a problem hiding this comment.
I don't think this is used yet so we can delete it for the moment until we have a UI to update this
| "id" SERIAL PRIMARY KEY, | ||
| "title" TEXT NOT NULL, | ||
| "description" TEXT, | ||
| "userId" TEXT NOT NULL, |
There was a problem hiding this comment.
This can be inferred from the post itself
|
Let me know if this is still WIP so I can pick it up if you don't have the time. |
✨ Codu Pull Request 💻
Fixes #1081
Pull Request details
Cases considered
CREATE
When the user creates a new record will be created in the series table
EDIT
If the user deletes the post which had a series name then the series will be removed from the series table, if no other post has that series.
Any Breaking changes
None
Note to Reviewers
Please let me know,
Associated Screenshots
UI
Working video
