Feature/ Add "series" to link related articles #1202
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 changes in this pull request implement a new feature that allows users to link articles into a series. This includes adding a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
had a problem deploying
to
production
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (4)
schema/series.ts (1)
3-6: Consider enhancing schema validation rules.While the basic structure is good, consider adding these validations to make the schema more robust:
- For
postId: Add format validation if it's a UUID- For
seriesName: Add constraints for:
- Minimum/maximum length
- Allowed characters
- Empty string handling after trim
- Maximum series name size to prevent DB issues
export const UpdateSeriesSchema = z.object({ - postId: z.string(), - seriesName: z.string().trim().optional() + postId: z.string().uuid("Invalid post ID format"), + seriesName: z.string() + .trim() + .min(1, "Series name cannot be empty") + .max(100, "Series name too long") + .regex(/^[\w\s-]+$/, "Series name contains invalid characters") + .optional() });server/db/schema.ts (1)
38-56: Consider additional data integrity constraintsWhile the basic schema structure is in place, consider these architectural improvements:
- Add a check constraint to prevent invalid order sequences within a series
- Consider adding a maximum length constraint for series names
- Add triggers or application-level validation to maintain consecutive order numbers when posts are added/removed from a series
Example constraints:
-- Check for valid order sequence CONSTRAINT valid_series_order CHECK (series_order > 0) -- Maximum length for series names ALTER TABLE series ADD CONSTRAINT series_name_length CHECK (char_length(name) <= 100)Also applies to: 173-173, 300-302
server/api/router/series.ts (2)
16-31: Optimize Data Retrieval incurrentPostQueryYou can optimize the
currentPostquery by selecting only necessary fields and removing redundantwithclause if not needed elsewhere.Apply this diff to refine the query:
const currentPost = await ctx.db.query.post.findFirst({ columns: { id: true, seriesId: true, userId: true }, - with: { - series: { - columns: { - id: true, - name: true - }, - }, - }, where: (post, { eq }) => eq(post.id, postId), });
79-107: RedundantuserIdCheck During Series DeletionBased on previous learnings, the
userIdcheck when deleting a series may be redundant since unauthorized access is already prevented.Apply this diff to remove the redundant check:
await tx.delete(series).where( and( eq(series.id, seriesId), - eq(series.userId, ctx.session.user.id) ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/(editor)/create/[[...paramsArr]]/_client.tsx(4 hunks)drizzle/0011_add_series_update_post.sql(1 hunks)schema/post.ts(1 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
📓 Learnings (3)
server/api/router/index.ts (1)
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/api/router/series.ts:109-135
Timestamp: 2024-10-18T04:18:28.906Z
Learning: In the TypeScript file `server/api/router/series.ts`, when comments are included to aid reviewers, avoid suggesting their removal.
server/api/router/post.ts (1)
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.
server/api/router/series.ts (1)
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.
🔇 Additional comments (11)
schema/series.ts (1)
3-6: Verify schema usage in mutations and error handling.
Let's verify how this schema is being used in the API mutations and ensure proper error handling is in place.
✅ Verification successful
Schema validation and error handling are properly implemented
The schema is correctly used in the series router with proper validation:
- Input validation is handled through tRPC's
.input(UpdateSeriesSchema) - Additional validation for empty series name with appropriate error throwing
- Protected procedure ensures authenticated access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how UpdateSeriesSchema is used in mutations and error handling
# Look for imports of this schema
echo "Checking schema imports:"
rg -l "UpdateSeriesSchema"
# Look for mutation definitions using this schema
echo -e "\nChecking mutation usage:"
ast-grep --pattern 'mutation($$$) {
$$$
series {
update($$$)
$$$
}
}'
# Check for error handling
echo -e "\nChecking error handling:"
rg -A 5 "UpdateSeriesSchema.*validate"
Length of output: 387
Script:
#!/bin/bash
# Let's check how the schema is actually used in the router file
echo "Checking series router implementation:"
rg -A 10 "UpdateSeriesSchema" server/api/router/series.ts
# Let's also check for any error handling patterns in the router
echo -e "\nChecking error handling patterns:"
ast-grep --pattern 'try {
$$$
UpdateSeriesSchema
$$$
} catch (error) {
$$$
}'
# Let's look for any validation or safeParse usage
echo -e "\nChecking validation usage:"
rg -B 2 -A 5 "safeParse|parse|validate" server/api/router/series.ts
Length of output: 1049
drizzle/0011_add_series_update_post.sql (1)
1-14: Verify the chronological ordering requirement implementation.
Based on the PR objectives, articles within a series should be ordered chronologically by publication date. The current schema doesn't explicitly handle this requirement, but it could be managed through the existing Post table's timestamps.
Let's verify if the Post table has the necessary columns for chronological ordering:
Consider whether the chronological ordering should be:
- Implicit using Post's publication date (current approach)
- Explicit using a new column in Post table for manual ordering
- Managed through a junction table for more flexible ordering
Would you like assistance in implementing any of these approaches?
✅ Verification successful
The Post table already has appropriate columns for chronological ordering
The Post table includes both published and createdAt timestamp columns which can effectively handle chronological ordering of articles within a series. The published column is particularly suitable for this purpose as it represents the actual publication date of posts.
This means the current schema can support chronological ordering through:
publishedtimestamp - For ordering by publication datecreatedAttimestamp - As a fallback for ordering by creation date
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Post table has publication date or similar columns
# Expected: Should find columns related to post timing/ordering
rg -A 10 'CREATE TABLE.*"Post"'
Length of output: 803
server/api/router/index.ts (2)
11-11: LGTM! Import statement follows the established pattern.
The import statement for seriesRouter follows the same pattern as other router imports, maintaining consistency in the codebase.
21-21: LGTM! Router configuration is properly integrated.
The seriesRouter is correctly added to the appRouter configuration, following the same structure as other routers. This integration enables the series feature while maintaining backward compatibility.
Let's verify the series router implementation:
✅ Verification successful
Series router implementation is properly integrated and verified
The verification confirms:
- The series router is correctly implemented with proper schema validation using
UpdateSeriesSchema - Database schema includes the series table with correct relations and constraints
- The router handles series operations with appropriate error handling and transaction management
- Schema validation is properly defined with required fields and validations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the series router implementation and its schema
# Test 1: Check if the series router file exists and contains the expected exports
rg -l "export.*seriesRouter" server/api/router/series.ts
# Test 2: Verify the series schema implementation
rg -l "UpdateSeriesSchema" server/api/schema/series.ts
# Test 3: Check for any potential circular dependencies
rg -l "from.*router/index" server/api/router/series.ts
Length of output: 266
Script:
#!/bin/bash
# Let's verify the series router implementation more thoroughly
# Check the actual content of series router
cat server/api/router/series.ts
# Look for series-related schema definitions in any location
fd -e ts -e js -x rg -l "schema.*[Ss]eries"
# Check for any imports in the series router
rg "import.*from" server/api/router/series.ts
# Look for any series-related type definitions
fd -e ts -x rg -l "type.*Series|interface.*Series"
Length of output: 6072
Script:
#!/bin/bash
# Let's verify the schema import and its implementation
# Check if the schema file exists in the correct location
fd -e ts -x rg -l "UpdateSeriesSchema" {}
# Check the content of the schema file if found
fd -e ts -x cat {} -n | rg -A 5 -B 5 "UpdateSeriesSchema"
# Verify the database schema for series table
rg -l "series.*schema" server/db/schema.ts
cat server/db/schema.ts | rg -A 5 -B 5 "series"
Length of output: 2811
schema/post.ts (1)
28-28: LGTM! The schema change aligns with the series feature requirements.
The addition of an optional seriesName field to SavePostSchema correctly supports the ability to associate posts with series while maintaining backward compatibility.
server/api/router/post.ts (3)
17-17: LGTM: Import statement is correctly updated.
The addition of the series import is necessary for the new series-related operations.
448-448: LGTM: Series relationship correctly included.
The addition of series: true to the with clause ensures that series data is properly loaded when editing a draft, which is necessary for the series feature.
Line range hint 1-548: Verify series creation and update implementation.
The current changes handle series deletion and fetching, but I don't see mutations for creating or updating series relationships in this file. According to the PR objectives, these operations should be supported.
Let's verify if these operations are implemented elsewhere:
app/(editor)/create/[[...paramsArr]]/_client.tsx (1)
228-228: LGTM!
The seriesName handling in form data is implemented correctly.
server/api/router/series.ts (2)
36-40: Authorization Check Confirmed
The authorization check ensures only the post owner can update the series, maintaining data security.
83-88:
Correct ne Operator Usage
Ensure the ne (not equal) operator is correctly imported and used to exclude the current post in the series check.
Apply this diff to import the ne operator:
-import { eq, and } from "drizzle-orm";
+import { eq, ne, and } from "drizzle-orm";And verify that ne is functioning as intended in the condition.
Likely invalid or redundant comment.
| -- Update Post table to add seriesId column | ||
| ALTER TABLE "Post" | ||
| ADD COLUMN "seriesId" INTEGER | ||
| ADD CONSTRAINT fk_post_series | ||
| FOREIGN KEY ("seriesId") | ||
| REFERENCES "Series" ("id") | ||
| ON DELETE SET NULL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding an index on the seriesId column.
The foreign key constraint is correctly set up with appropriate ON DELETE behavior. However, consider adding an index on the "seriesId" column to improve the performance of queries that filter or join posts by series.
ALTER TABLE "Post"
ADD COLUMN "seriesId" INTEGER
ADD CONSTRAINT fk_post_series
FOREIGN KEY ("seriesId")
REFERENCES "Series" ("id")
ON DELETE SET NULL;
+CREATE INDEX idx_post_series ON "Post"("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.
| -- Update Post table to add seriesId column | |
| ALTER TABLE "Post" | |
| ADD COLUMN "seriesId" INTEGER | |
| ADD CONSTRAINT fk_post_series | |
| FOREIGN KEY ("seriesId") | |
| REFERENCES "Series" ("id") | |
| ON DELETE SET NULL; | |
| -- Update Post table to add seriesId column | |
| ALTER TABLE "Post" | |
| ADD COLUMN "seriesId" INTEGER | |
| ADD CONSTRAINT fk_post_series | |
| FOREIGN KEY ("seriesId") | |
| REFERENCES "Series" ("id") | |
| ON DELETE SET NULL; | |
| CREATE INDEX idx_post_series ON "Post"("seriesId"); |
| -- Create Series table | ||
| CREATE TABLE IF NOT EXISTS "Series" ( | ||
| "id" SERIAL PRIMARY KEY, | ||
| "name" TEXT NOT NULL, | ||
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | ||
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding constraints and indexes to the Series table.
The table structure looks good, but consider these improvements for better data integrity and performance:
- Add a unique constraint on the "name" column to prevent duplicate series names
- Add an index on the "name" column to improve lookup performance
- Add a column to track the order of articles within a series (e.g., "sequence" or "position")
CREATE TABLE IF NOT EXISTS "Series" (
"id" SERIAL PRIMARY KEY,
- "name" TEXT NOT NULL,
+ "name" TEXT NOT NULL UNIQUE,
"createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
"updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL
);
+CREATE INDEX idx_series_name ON "Series"("name");Committable suggestion was skipped due to low confidence.
schema/post.ts
Outdated
| @@ -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.
🛠️ Refactor suggestion
Consider adding validation rules for series names.
To ensure consistency and prevent potential issues, consider adding validation rules for the series name:
- seriesName: z.string().trim().optional()
+ seriesName: z.string()
+ .trim()
+ .min(1, "Series name cannot be empty")
+ .max(50, "Series name is too long")
+ .regex(/^[\w\s-]+$/, "Series name can only contain letters, numbers, spaces, and hyphens")
+ .optional()This would:
- Prevent empty strings after trimming
- Limit series name length to maintain UI consistency
- Restrict characters to ensure URL-friendly series names and consistent formatting
📝 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.
| seriesName: z.string().trim().optional() | |
| seriesName: z.string() | |
| .trim() | |
| .min(1, "Series name cannot be empty") | |
| .max(50, "Series name is too long") | |
| .regex(/^[\w\s-]+$/, "Series name can only contain letters, numbers, spaces, and hyphens") | |
| .optional() |
There was a problem hiding this comment.
This seems like a sensible addition @RangerCreaky
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
server/api/router/post.ts
Outdated
| const deletedPost = await ctx.db.transaction(async (tx) => { | ||
| const [deletedPost] = await tx | ||
| .delete(post) | ||
| .where(eq(post.id, id)) | ||
| .returning(); | ||
|
|
||
| if(deletedPost.seriesId){ | ||
| // check is there is any other post with the current seriesId | ||
| const anotherPostInThisSeries = await tx.query.post.findFirst({ | ||
| where: (post, { eq }) => | ||
| eq(post.seriesId, deletedPost.seriesId!) | ||
| }) | ||
| // if another post with the same seriesId is present, then do nothing | ||
| // else remove the series from the series table | ||
| if(!anotherPostInThisSeries){ | ||
| await tx.delete(series).where(eq(series.id, deletedPost.seriesId)); | ||
| } | ||
| } | ||
|
|
||
| return deletedPost; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve series cleanup implementation.
While the transaction usage is good for maintaining data consistency, there are several improvements that could be made:
- Optimize the query to use count instead of findFirst:
- const anotherPostInThisSeries = await tx.query.post.findFirst({
- where: (post, { eq }) =>
- eq(post.seriesId, deletedPost.seriesId!)
- })
+ const [result] = await tx
+ .select({ count: sql<number>`count(*)` })
+ .from(post)
+ .where(eq(post.seriesId, deletedPost.seriesId))
+ .limit(1);
+ const hasOtherPosts = result.count > 0;- Remove the non-null assertion and add error handling:
- if(deletedPost.seriesId){
+ if (deletedPost.seriesId) {
// check is there is any other post with the current seriesId
- const anotherPostInThisSeries = await tx.query.post.findFirst({
+ try {
+ const [result] = await tx
+ .select({ count: sql<number>`count(*)` })
+ .from(post)
+ .where(eq(post.seriesId, deletedPost.seriesId))
+ .limit(1);
+ const hasOtherPosts = result.count > 0;
- if(!anotherPostInThisSeries){
+ if (!hasOtherPosts) {
await tx.delete(series).where(eq(series.id, deletedPost.seriesId));
+ }
+ } catch (error) {
+ console.error('Failed to cleanup series:', error);
+ // Transaction will be rolled back automatically
+ throw new TRPCError({
+ code: 'INTERNAL_SERVER_ERROR',
+ message: 'Failed to cleanup series',
+ });
}
}Committable suggestion was skipped due to low confidence.
| @@ -149,6 +170,7 @@ | |||
| .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.
Add index on seriesId for query performance
An index on seriesId will improve performance when querying posts within a series, which is crucial for the series navigation feature.
Add the index in the post table options:
}, (table) => {
return {
idKey: uniqueIndex("Post_id_key").on(table.id),
slugKey: uniqueIndex("Post_slug_key").on(table.slug),
slugIndex: index("Post_slug_index").on(table.slug),
- userIdIndex: index("Post_userId_index").on(table.userId),
+ userIdIndex: index("Post_userId_index").on(table.userId),
+ seriesIdIndex: index("Post_seriesId_index").on(table.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.
| seriesId: integer("seriesId").references(() => series.id, { onDelete: "set null", onUpdate: "cascade" }), | |
| }, (table) => { | |
| return { | |
| idKey: uniqueIndex("Post_id_key").on(table.id), | |
| slugKey: uniqueIndex("Post_slug_key").on(table.slug), | |
| slugIndex: index("Post_slug_index").on(table.slug), | |
| userIdIndex: index("Post_userId_index").on(table.userId), | |
| seriesIdIndex: index("Post_seriesId_index").on(table.seriesId), | |
| }; | |
| }, |
server/api/router/series.ts
Outdated
| if (seriesName && seriesName.trim() === "") { | ||
| throw new TRPCError({ code: 'BAD_REQUEST', message: 'Series name cannot be empty' }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Series Name Validation
Consider simplifying the validation logic for seriesName to enhance readability.
Apply this diff to streamline the validation:
-if (seriesName && seriesName.trim() === "") {
+if (seriesName?.trim() === "") {
throw new TRPCError({ code: 'BAD_REQUEST', message: 'Series name cannot be empty' });
}📝 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 && seriesName.trim() === "") { | |
| throw new TRPCError({ code: 'BAD_REQUEST', message: 'Series name cannot be empty' }); | |
| } | |
| if (seriesName?.trim() === "") { | |
| throw new TRPCError({ code: 'BAD_REQUEST', message: 'Series name cannot be empty' }); | |
| } |
server/api/router/series.ts
Outdated
| eq(series.name, seriesTitle), | ||
| eq(series.userId, ctx.session.user.id) | ||
| ), |
There was a problem hiding this comment.
Avoid Potential Race Conditions
Consider adding a unique constraint at the database level to prevent race conditions when two series with the same name are created simultaneously by the same user.
Apply this diff to add a unique constraint:
+-- Add a unique constraint on (name, userId) in your migration script
+ALTER TABLE series ADD CONSTRAINT unique_series_name_per_user UNIQUE (name, userId);Committable suggestion was skipped due to low confidence.
server/api/router/series.ts
Outdated
| if(!currSeries){ | ||
| const [newSeries] = await tx.insert(series).values({ | ||
| name: seriesTitle, | ||
| userId: ctx.session.user.id, | ||
| updatedAt: new Date() | ||
| }).returning(); | ||
|
|
||
| seriesId = newSeries.id; | ||
| } | ||
| else{ | ||
| seriesId = currSeries.id; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Series Creation Logic
You can simplify the logic by using upsert to handle series creation or retrieval in a single step.
Apply this diff to use upsert:
let seriesId: number;
-const currSeries = await tx.query.series.findFirst({ ... });
-
-if (!currSeries) {
- const [newSeries] = await tx.insert(series).values({ ... }).returning();
- seriesId = newSeries.id;
-} else {
- seriesId = currSeries.id;
-}
+
+const [newSeries] = await tx
+ .insert(series)
+ .values({
+ name: seriesTitle,
+ userId: ctx.session.user.id,
+ updatedAt: new Date(),
+ })
+ .onConflict((oc) => oc
+ .columns(['name', 'userId'])
+ .doUpdateSet({ updatedAt: new Date() })
+ )
+ .returning();
+seriesId = newSeries.id;Committable suggestion was skipped due to low confidence.
server/api/router/series.ts
Outdated
| 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?.name !== 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
Clarify Control Flow for Series Update Logic
Consider refactoring the nested conditions to improve readability and maintainability of the series update logic.
Apply this diff to restructure the control flow:
if (seriesName) {
if (currentPost.seriesId) {
if (currentPost.series?.name !== seriesName) {
await unlinkSeries(currentPost.seriesId);
await createNewSeries(seriesName);
}
- // Else, series name is the same; no action needed.
} else {
await createNewSeries(seriesName);
}
} else if (currentPost.seriesId) {
await unlinkSeries(currentPost.seriesId);
}
-// Else, no series name provided and no existing series; no action needed.📝 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?.name !== 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?.name !== seriesName) { | |
| await unlinkSeries(currentPost.seriesId); | |
| await createNewSeries(seriesName); | |
| } | |
| // Else, series name is the same; no action needed. | |
| } else { | |
| await createNewSeries(seriesName); | |
| } | |
| } else if (currentPost.seriesId) { | |
| await unlinkSeries(currentPost.seriesId); | |
| } | |
| // Else, no series name provided and no existing series; no action needed. |
server/api/router/series.ts
Outdated
| const currSeries = await tx.query.series.findFirst({ | ||
| columns: { | ||
| id: true | ||
| }, | ||
| where: (series, { eq, and }) => and( | ||
| eq(series.name, seriesTitle), | ||
| eq(series.userId, ctx.session.user.id) | ||
| ), | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the schema analysis, I can now provide a final response:
Add Unique Constraint for Series Name Per User
The Series table currently lacks a unique constraint for the combination of name and userId. This could lead to duplicate series names for the same user. Add a unique composite constraint:
ALTER TABLE "Series" ADD CONSTRAINT "unique_series_name_per_user"
UNIQUE ("name", "userId");🔗 Analysis chain
Handle Series Name Uniqueness per User
Ensure that series names are unique per user to prevent conflicts when different users have series with the same name.
Run the following script to check for uniqueness constraints:
Let me try a different approach to verify the uniqueness constraints in the schema definition.
Let me check the schema definition for the Series table specifically.
Let me check if there are any unique constraints defined for the Series table in subsequent migrations.
Let me check for any composite unique constraints or indexes that might have been added to ensure series name uniqueness per user.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if series names are unique per user.
# Test: Search for series with duplicate names under different users.
# Expect: No series have the same name with different userIds.
rg -A 5 $'series' | gawk '/name:/{name=$2} /userId:/{user=$2; print name,user}' | sort | uniq -d
Length of output: 109
Script:
#!/bin/bash
# Search for schema definition of the series table
ast-grep --pattern 'createTable("series"' -A 20
Length of output: 98024
Script:
#!/bin/bash
# Search for Series table schema and any unique constraints
rg -A 10 'CREATE TABLE.*"Series"'
Length of output: 842
Script:
#!/bin/bash
# Search for any unique constraints or indexes on the Series table
rg -i "series.*unique|unique.*series|CREATE.*INDEX.*series"
Length of output: 61
Script:
#!/bin/bash
# Search for any constraints or indexes involving both series and userId
rg -A 5 'ALTER TABLE.*"Series".*ADD CONSTRAINT|CREATE.*INDEX.*series.*user|ADD CONSTRAINT.*series.*user'
Length of output: 106
schema/post.ts
Outdated
| @@ -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.
Previously review comments were raised stating that seriesName would cause issues while joining table as it will not be unique.
However, the savePostSchema is not the schema for the post table. The post table itself has seriesId as a foreign key. Please refer to the link above, where I have included screenshots of the post table schema and how the savePostSchema is currently being used.
RangerCreaky
had a problem deploying
to
production
GitHub Actions
Failure
|
@NiallJoeMaher can you please review, I have worked on the changes you mentioned previously. |
| .delete(post) | ||
| .where(eq(post.id, id)) | ||
| .returning(); | ||
| const deletedPost = await ctx.db.transaction(async (tx) => { |
There was a problem hiding this comment.
Nitpick, naming here should be ctx
There was a problem hiding this comment.
@NiallJoeMaher
I've retained tx to distinguish it from ctx, which is the broader application context. Renaming tx here to ctx would cause a naming conflict and could lead to confusion, as ctx is also used to reference the session context in this block.
for example,
return await ctx.db.transaction(async (tx) => {
let seriesId : number;
const currSeries = await tx.query.series.findFirst({
columns: {
id: true
},
where: (series, { eq, and }) => and(
eq(series.name, seriesTitle),
eq(series.userId, ctx.session.user.id)
),
})
here there is a reference to the ctx inside the callback, renaming tx to ctx would cause issues.
Please let me know if this approach works, or if there’s another way you'd like it handled!
There was a problem hiding this comment.
would txCtx be a good middle ground?
ie. its the transaction context
There was a problem hiding this comment.
@JohnAllenTech This looks good to me.
Let me know if that works.
|
@NiallJoeMaher Can you please check the reply and Please let me know if this approach works, or if there’s another way you'd like it handled! |
|
@NiallJoeMaher Any update on this? |
schema/post.ts
Outdated
| @@ -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.
This seems like a sensible addition @RangerCreaky
schema/series.ts
Outdated
|
|
||
| export const UpdateSeriesSchema = z.object({ | ||
| postId: z.string(), | ||
| seriesName: z.string().trim().optional() |
There was a problem hiding this comment.
Probably can share the schema fix as above here too to ensure the name is valid when provided.
server/api/router/series.ts
Outdated
| // check if the series title is same as the current series name | ||
| // then we do nothing | ||
| if(currentPost?.series?.name !== seriesName){ | ||
| // then the user has updated the series name in this particular edit |
There was a problem hiding this comment.
There is probably too many comments here from the AI generation
There was a problem hiding this comment.
No this comments aren't from the AI generation
I have added them, for the reviewers to understand the logic
Will remove them in the final commit before pushing
server/api/router/index.ts
Outdated
| @@ -16,6 +18,7 @@ export const appRouter = createTRPCRouter({ | |||
| admin: adminRouter, | |||
| report: reportRouter, | |||
| tag: tagRouter, | |||
| series: seriesRouter | |||
There was a problem hiding this comment.
I don't think the logic needs to be separated from the post logic since we create a series from within a post
There was a problem hiding this comment.
Okay, Will merge this logic with the post logic
There was a problem hiding this comment.
@NiallJoeMaher made this update
Merged this logic with the post logic
RangerCreaky
had a problem deploying
to
production
GitHub Actions
Error
RangerCreaky
had a problem deploying
to
production
GitHub Actions
Error
RangerCreaky
had a problem deploying
to
production
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
drizzle/0011_add_series_update_post.sql (2)
1-8: Consider enhancing the Series table definition.While the basic structure is good, consider these improvements:
- Add a maximum length constraint to the "name" column to prevent excessive lengths
- Use consistent casing for "userId" (should be "userId" or "user_id" to match your convention)
- Add documentation comments explaining the table's purpose
-- Create Series table +-- Stores series information for grouping related articles +-- A series belongs to a user and can contain multiple posts CREATE TABLE IF NOT EXISTS "Series" ( "id" SERIAL PRIMARY KEY, - "name" TEXT NOT NULL, + "name" TEXT NOT NULL CHECK (length("name") <= 255), "userId" text NOT NULL, "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL );
1-18: Consider adding a position column for explicit article ordering.While publication dates can be used for chronological ordering, a dedicated position/sequence column in the Post table would provide more flexibility in arranging articles within a series. This would allow:
- Manual reordering of articles
- Insertion of articles between existing ones
- Clear indication of article position (e.g., "Article 3 of 5")
This aligns with the requirement to show article position within a series.
ALTER TABLE "Post" ADD COLUMN "seriesId" INTEGER +ADD COLUMN "seriesPosition" INTEGER, ADD CONSTRAINT fk_post_series FOREIGN KEY ("seriesId") REFERENCES "Series" ("id") ON DELETE SET NULL;server/api/router/post.ts (1)
139-154: Simplify series update logic.The nested conditionals make the code harder to follow. The logic can be simplified by handling each case separately.
Consider this alternative structure:
-if (seriesName) { - if (currentPost?.seriesId) { - if (currentPost?.series?.name !== seriesName) { - await unlinkSeries(currentPost.seriesId); - await createNewSeries(seriesName); - } - } - else { - await createNewSeries(seriesName); - } -} -else { - if (currentPost.seriesId !== null) { - await unlinkSeries(currentPost.seriesId); - } -} +// Case 1: Remove series +if (!seriesName && currentPost.seriesId) { + await unlinkSeries(currentPost.seriesId); + return; +} + +// Case 2: Create or update series +if (seriesName && (!currentPost.seriesId || currentPost.series?.name !== seriesName)) { + if (currentPost.seriesId) { + await unlinkSeries(currentPost.seriesId); + } + await createNewSeries(seriesName); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (6)
app/(editor)/create/[[...paramsArr]]/_client.tsx(4 hunks)drizzle/0011_add_series_update_post.sql(1 hunks)schema/post.ts(1 hunks)server/api/router/index.ts(1 hunks)server/api/router/post.ts(9 hunks)server/db/schema.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/api/router/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- schema/post.ts
🧰 Additional context used
📓 Learnings (1)
server/api/router/post.ts (1)
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.
🔇 Additional comments (8)
drizzle/0011_add_series_update_post.sql (2)
9-15: LGTM! Foreign key constraint is well implemented.
The foreign key setup with ON DELETE SET NULL behavior correctly handles series deletion, matching the requirements.
16-17: LGTM! Indexes have been added as suggested.
The indexes on seriesId and name columns will improve query performance for series-related operations.
server/db/schema.ts (2)
300-302: LGTM! Relations are correctly defined
The one-to-many relationship between series and posts is properly implemented.
173-173:
Add series-related fields and indexes to the post table
To support efficient querying and series navigation features:
Apply these changes:
likes: integer("likes").default(0).notNull(),
seriesId: integer("seriesId").references(() => series.id, { onDelete: "set null", onUpdate: "cascade" }),
+ seriesOrder: integer("seriesOrder"),
},
(table) => {
return {
idKey: uniqueIndex("Post_id_key").on(table.id),
slugKey: uniqueIndex("Post_slug_key").on(table.slug),
slugIndex: index("Post_slug_index").on(table.slug),
- userIdIndex: index("Post_userId_index").on(table.userId),
+ userIdIndex: index("Post_userId_index").on(table.userId),
+ seriesIdIndex: index("Post_seriesId_index").on(table.seriesId),
+ seriesOrderIndex: index("post_series_order_index").on(
+ table.seriesId,
+ table.seriesOrder
+ ),
};
},Likely invalid or redundant comment.
server/api/router/post.ts (3)
539-539: LGTM!
The addition of series to the with clause is correct and necessary for retrieving series information with the post.
112-137: 🛠️ Refactor suggestion
Improve unlinkSeries implementation.
The function has several issues:
- Missing error handling
- Unnecessary non-null assertion on
seriesId - Redundant comment
Note: This comment partially overlaps with a previous review comment about series cleanup. Apply these additional improvements:
const unlinkSeries = async (seriesId: number) => {
return await ctx.db.transaction(async (tx) => {
+ try {
const anotherPostInThisSeries = await tx.query.post.findFirst({
where: (post, { eq, and, ne }) =>
and(
ne(post.id, currentPost.id),
- eq(post.seriesId, seriesId!)
+ eq(post.seriesId, seriesId)
)
});
- // if another post with the same seriesId is present, then do nothing
- // else remove the series from the series table
if (!anotherPostInThisSeries) {
await tx.delete(series).where(
and(
eq(series.id, seriesId),
eq(series.userId, ctx.session.user.id)
)
);
}
- // update that series id in the current post
await tx
.update(post)
.set({
seriesId: null
})
.where(eq(post.id, currentPost.id));
+ } catch (error) {
+ console.error('Failed to unlink series:', error);
+ throw new TRPCError({
+ code: 'INTERNAL_SERVER_ERROR',
+ message: 'Failed to unlink series'
+ });
+ }
});
};73-75: 🛠️ Refactor suggestion
Move empty string validation to schema level.
The empty string validation for seriesName should be handled in the SavePostSchema validation schema instead of runtime validation.
Move this validation to the schema:
// In schema/post.ts
export const SavePostSchema = z.object({
// ... other fields
seriesName: z.string().optional().refine(
(val) => !val || val.trim().length > 0,
"Series name cannot be empty"
),
});⛔ Skipped due to learnings
Learnt from: RangerCreaky
PR: codu-code/codu#1146
File: server/api/router/series.ts:109-135
Timestamp: 2024-10-18T04:18:28.906Z
Learning: In the TypeScript file `server/api/router/series.ts`, when comments are included to aid reviewers, avoid suggesting their removal.
app/(editor)/create/[[...paramsArr]]/_client.tsx (1)
575-587: Enhance series input field UX.
While the implementation is functional, consider these improvements for better user experience:
- Add pattern validation to ensure consistent series names
- Show existing series names as suggestions to prevent typos
- Expand the help text to include format requirements
<input
id="seriesName"
type="text"
placeholder="The name of my series"
defaultValue={data?.series?.name || ""}
+ pattern="^[a-zA-Z0-9\s-]+$"
+ title="Series name can only contain letters, numbers, spaces, and hyphens"
{...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
+ Series names are case-sensitive and must match exactly to connect articles. Use only letters, numbers, spaces, and hyphens.
</p>
+{/* Add datalist for existing series names */}
+<datalist id="series-suggestions">
+ {existingSeries.map(series => (
+ <option key={series.id} value={series.name} />
+ ))}
+</datalist>Let's verify if there's a series API endpoint we can use for suggestions:
| ON DELETE SET NULL; | ||
| CREATE INDEX idx_post_series ON "Post"("seriesId"); | ||
| CREATE INDEX idx_series_name ON "Series"("name"); | ||
| ALTER TABLE series ADD CONSTRAINT unique_series_name_per_user UNIQUE (name, userId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix table name casing in the constraint.
The unique constraint is a good addition that prevents duplicate series names per user. However, there's an inconsistency in the table name casing.
-ALTER TABLE series ADD CONSTRAINT unique_series_name_per_user UNIQUE (name, userId);
+ALTER TABLE "Series" ADD CONSTRAINT unique_series_name_per_user UNIQUE ("name", "userId");📝 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.
| ALTER TABLE series ADD CONSTRAINT unique_series_name_per_user UNIQUE (name, userId); | |
| ALTER TABLE "Series" ADD CONSTRAINT unique_series_name_per_user UNIQUE ("name", "userId"); |
server/db/schema.ts
Outdated
| export const series = pgTable("Series", { | ||
| id: serial("id").primaryKey(), | ||
| name: text("name").notNull(), | ||
| description: text("description"), | ||
| userId: text("userId").notNull().references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }), | ||
| createdAt: timestamp("createdAt", { | ||
| precision: 3, | ||
| mode: "string", | ||
| withTimezone: true, | ||
| }) | ||
| .default(sql`CURRENT_TIMESTAMP`) | ||
| .notNull(), | ||
| updatedAt: timestamp("updatedAt", { | ||
| precision: 3, | ||
| withTimezone: true | ||
| }).notNull() | ||
| .$onUpdate(() => new Date()) | ||
| .default(sql`CURRENT_TIMESTAMP`), | ||
| }) |
There was a problem hiding this comment.
Schema design improvements needed for the series table
Several adjustments are needed to align with the schema design patterns:
Apply these changes:
-export const series = pgTable("Series", {
+export const series = pgTable("series", {
id: serial("id").primaryKey(),
name: text("name").notNull(),
description: text("description"),
userId: text("userId").notNull().references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }),
createdAt: timestamp("createdAt", {
precision: 3,
- mode: "string",
withTimezone: true,
})
.default(sql`CURRENT_TIMESTAMP`)
.notNull(),
updatedAt: timestamp("updatedAt", {
precision: 3,
withTimezone: true
})
.notNull()
- .$onUpdate(() => new Date())
+ .$onUpdate(() => new Date().toISOString())
.default(sql`CURRENT_TIMESTAMP`),
+ }, (table) => ({
+ nameUserIdKey: uniqueIndex("series_name_userId_key").on(
+ table.name,
+ table.userId
+ ),
+ nameIndex: index("series_name_index").on(table.name),
+ userIdIndex: index("series_userId_index").on(table.userId),
}));📝 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.
| export const series = pgTable("Series", { | |
| id: serial("id").primaryKey(), | |
| name: text("name").notNull(), | |
| description: text("description"), | |
| userId: text("userId").notNull().references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }), | |
| createdAt: timestamp("createdAt", { | |
| precision: 3, | |
| mode: "string", | |
| withTimezone: true, | |
| }) | |
| .default(sql`CURRENT_TIMESTAMP`) | |
| .notNull(), | |
| updatedAt: timestamp("updatedAt", { | |
| precision: 3, | |
| withTimezone: true | |
| }).notNull() | |
| .$onUpdate(() => new Date()) | |
| .default(sql`CURRENT_TIMESTAMP`), | |
| }) | |
| export const series = pgTable("series", { | |
| id: serial("id").primaryKey(), | |
| name: text("name").notNull(), | |
| description: text("description"), | |
| userId: text("userId").notNull().references(() => user.id, { onDelete: "cascade", onUpdate: "cascade" }), | |
| createdAt: timestamp("createdAt", { | |
| precision: 3, | |
| withTimezone: true, | |
| }) | |
| .default(sql`CURRENT_TIMESTAMP`) | |
| .notNull(), | |
| updatedAt: timestamp("updatedAt", { | |
| precision: 3, | |
| withTimezone: true | |
| }) | |
| .notNull() | |
| .$onUpdate(() => new Date().toISOString()) | |
| .default(sql`CURRENT_TIMESTAMP`), | |
| }, (table) => ({ | |
| nameUserIdKey: uniqueIndex("series_name_userId_key").on( | |
| table.name, | |
| table.userId | |
| ), | |
| nameIndex: index("series_name_index").on(table.name), | |
| userIdIndex: index("series_userId_index").on(table.userId), | |
| })); |
| const createNewSeries = async (seriesTitle: string) => { | ||
| return await ctx.db.transaction(async (tx) => { | ||
| let seriesId: number; | ||
| const currSeries = await tx.query.series.findFirst({ | ||
| columns: { | ||
| id: true | ||
| }, | ||
| where: (series, { eq, and }) => and( | ||
| eq(series.name, seriesTitle), | ||
| eq(series.userId, ctx.session.user.id) | ||
| ), | ||
| }) | ||
|
|
||
| if (!currSeries) { | ||
| const [newSeries] = await tx.insert(series).values({ | ||
| name: seriesTitle, | ||
| userId: ctx.session.user.id, | ||
| updatedAt: new Date() | ||
| }).returning(); | ||
|
|
||
| seriesId = newSeries.id; | ||
| } | ||
| else { | ||
| seriesId = currSeries.id; | ||
| } | ||
| await tx | ||
| .update(post) | ||
| .set({ | ||
| seriesId: seriesId | ||
| }) | ||
| .where(eq(post.id, currentPost.id)); | ||
| }) | ||
|
|
||
| } |
There was a problem hiding this comment.
Add error handling and fix potential race condition in createNewSeries.
The function has two issues:
- Missing error handling for the transaction
- Potential race condition between checking for existing series and creating a new one
Apply this improvement:
const createNewSeries = async (seriesTitle: string) => {
- return await ctx.db.transaction(async (tx) => {
+ return await ctx.db.transaction(async (tx) => {
+ try {
let seriesId: number;
- const currSeries = await tx.query.series.findFirst({
+ // Use insert with ON CONFLICT to handle race condition
+ const [newSeries] = await tx.insert(series)
+ .values({
+ name: seriesTitle,
+ userId: ctx.session.user.id,
+ updatedAt: new Date()
+ })
+ .onConflictDoUpdate({
+ target: [series.name, series.userId],
+ set: { updatedAt: new Date() }
+ })
+ .returning();
+
+ await tx
+ .update(post)
+ .set({
+ seriesId: newSeries.id
+ })
+ .where(eq(post.id, currentPost.id));
+
+ return newSeries.id;
+ } catch (error) {
+ console.error('Failed to create/update series:', error);
+ throw new TRPCError({
+ code: 'INTERNAL_SERVER_ERROR',
+ message: 'Failed to create/update series'
+ });
+ }
});
};📝 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 createNewSeries = async (seriesTitle: string) => { | |
| return await ctx.db.transaction(async (tx) => { | |
| let seriesId: number; | |
| const currSeries = await tx.query.series.findFirst({ | |
| columns: { | |
| id: true | |
| }, | |
| where: (series, { eq, and }) => and( | |
| eq(series.name, seriesTitle), | |
| eq(series.userId, ctx.session.user.id) | |
| ), | |
| }) | |
| if (!currSeries) { | |
| const [newSeries] = await tx.insert(series).values({ | |
| name: seriesTitle, | |
| userId: ctx.session.user.id, | |
| updatedAt: new Date() | |
| }).returning(); | |
| seriesId = newSeries.id; | |
| } | |
| else { | |
| seriesId = currSeries.id; | |
| } | |
| await tx | |
| .update(post) | |
| .set({ | |
| seriesId: seriesId | |
| }) | |
| .where(eq(post.id, currentPost.id)); | |
| }) | |
| } | |
| const createNewSeries = async (seriesTitle: string) => { | |
| return await ctx.db.transaction(async (tx) => { | |
| try { | |
| let seriesId: number; | |
| // Use insert with ON CONFLICT to handle race condition | |
| const [newSeries] = await tx.insert(series) | |
| .values({ | |
| name: seriesTitle, | |
| userId: ctx.session.user.id, | |
| updatedAt: new Date() | |
| }) | |
| .onConflictDoUpdate({ | |
| target: [series.name, series.userId], | |
| set: { updatedAt: new Date() } | |
| }) | |
| .returning(); | |
| await tx | |
| .update(post) | |
| .set({ | |
| seriesId: newSeries.id | |
| }) | |
| .where(eq(post.id, currentPost.id)); | |
| return newSeries.id; | |
| } catch (error) { | |
| console.error('Failed to create/update series:', error); | |
| throw new TRPCError({ | |
| code: 'INTERNAL_SERVER_ERROR', | |
| message: 'Failed to create/update series' | |
| }); | |
| } | |
| }); | |
| } |
| const deletedPost = await ctx.db.transaction(async (tx) => { | ||
| const [deletedPost] = await tx | ||
| .delete(post) | ||
| .where(eq(post.id, id)) | ||
| .returning(); | ||
|
|
||
| if (deletedPost.seriesId) { | ||
| // check is there is any other post with the current seriesId | ||
| const anotherPostInThisSeries = await tx.query.post.findFirst({ | ||
| where: (post, { eq }) => | ||
| eq(post.seriesId, deletedPost.seriesId!) | ||
| }) | ||
| // if another post with the same seriesId is present, then do nothing | ||
| // else remove the series from the series table | ||
| if (!anotherPostInThisSeries) { | ||
| await tx.delete(series).where(eq(series.id, deletedPost.seriesId)); | ||
| } | ||
| } | ||
|
|
||
| return deletedPost; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve series cleanup in delete mutation.
The series cleanup logic has similar issues to those in the update mutation:
- Missing error handling
- Unnecessary non-null assertion
- Could use count instead of findFirst for efficiency
Apply this improvement:
const deletedPost = await ctx.db.transaction(async (tx) => {
+ try {
const [deletedPost] = await tx
.delete(post)
.where(eq(post.id, id))
.returning();
if (deletedPost.seriesId) {
- // check is there is any other post with the current seriesId
- const anotherPostInThisSeries = await tx.query.post.findFirst({
- where: (post, { eq }) =>
- eq(post.seriesId, deletedPost.seriesId!)
- })
+ const [result] = await tx
+ .select({ count: sql<number>`count(*)` })
+ .from(post)
+ .where(eq(post.seriesId, deletedPost.seriesId))
+ .limit(1);
+
- if (!anotherPostInThisSeries) {
+ if (result.count === 0) {
await tx.delete(series).where(eq(series.id, deletedPost.seriesId));
}
}
return deletedPost;
+ } catch (error) {
+ console.error('Failed to delete post and cleanup series:', error);
+ throw new TRPCError({
+ code: 'INTERNAL_SERVER_ERROR',
+ message: 'Failed to delete post'
+ });
+ }
});📝 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 deletedPost = await ctx.db.transaction(async (tx) => { | |
| const [deletedPost] = await tx | |
| .delete(post) | |
| .where(eq(post.id, id)) | |
| .returning(); | |
| if (deletedPost.seriesId) { | |
| // check is there is any other post with the current seriesId | |
| const anotherPostInThisSeries = await tx.query.post.findFirst({ | |
| where: (post, { eq }) => | |
| eq(post.seriesId, deletedPost.seriesId!) | |
| }) | |
| // if another post with the same seriesId is present, then do nothing | |
| // else remove the series from the series table | |
| if (!anotherPostInThisSeries) { | |
| await tx.delete(series).where(eq(series.id, deletedPost.seriesId)); | |
| } | |
| } | |
| return deletedPost; | |
| }); | |
| const deletedPost = await ctx.db.transaction(async (tx) => { | |
| try { | |
| const [deletedPost] = await tx | |
| .delete(post) | |
| .where(eq(post.id, id)) | |
| .returning(); | |
| if (deletedPost.seriesId) { | |
| const [result] = await tx | |
| .select({ count: sql<number>`count(*)` }) | |
| .from(post) | |
| .where(eq(post.seriesId, deletedPost.seriesId)) | |
| .limit(1); | |
| if (result.count === 0) { | |
| await tx.delete(series).where(eq(series.id, deletedPost.seriesId)); | |
| } | |
| } | |
| return deletedPost; | |
| } catch (error) { | |
| console.error('Failed to delete post and cleanup series:', error); | |
| throw new TRPCError({ | |
| code: 'INTERNAL_SERVER_ERROR', | |
| message: 'Failed to delete post' | |
| }); | |
| } | |
| }); |
| seriesName: data.seriesName || undefined | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and sanitization for series name.
The series name is currently accepted without any validation or sanitization. Since series names need to match exactly for articles to be connected (as mentioned in the UI help text), consider adding validation to prevent issues with whitespace, special characters, or case sensitivity.
const getFormData = () => {
const data = getValues();
+ const sanitizedSeriesName = data.seriesName?.trim();
const formData = {
...data,
tags,
canonicalUrl: data.canonicalUrl || undefined,
excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155),
- seriesName: data.seriesName || undefined
+ seriesName: sanitizedSeriesName || undefined
};
return formData;
};Committable suggestion skipped: line range outside the PR's diff.
RangerCreaky
had a problem deploying
to
production
GitHub Actions
Failure
|
@NiallJoeMaher I have made the mentioned changes |
RangerCreaky
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
server/db/schema.ts (2)
172-172: Consider adding constraints for series managementWhile the foreign key is correctly set up, consider adding:
- A CHECK constraint to ensure published posts in a series have valid publication dates
- An index on (seriesId, published) for efficient series navigation queries
Based on the schema analysis and search results, I can now generate the final response:
Schema needs position field for series articles
The current schema lacks essential features needed for proper series implementation:
- No
positionororderfield in the post table to track article sequence- No built-in navigation between series articles
- Chronological ordering relies solely on
publishedtimestamp which may not reflect intended article sequenceRequired changes:
- Add
positionfield to theposttable to explicitly track article order within a series- Consider adding navigation-related fields or implementing a view/query to determine previous/next articles
🔗 Analysis chain
Line range hint
38-301: Verify schema supports all series featuresLet's verify if the current schema can support all required features from issue #1081:
- Navigation between articles in a series
- Position tracking (e.g., "Article 3 of 5")
- Chronological ordering
Let me analyze the schema and search results to verify if we need additional information.
From the schema and search results, I can see:
- The
seriestable has:
id(primary key)nameuserId(foreign key to user)createdAtupdatedAt
- The
posttable has:
seriesId(foreign key to series)publishedtimestamp for chronological ordering
- The
seriesRelationsdefines:
- One-to-many relationship between series and posts
- No explicit ordering field exists in the schema for position tracking
Let me verify if there are any additional fields or implementations I might have missed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if we have the necessary fields for ordering echo "Checking fields for series navigation and ordering..." rg -A 5 "seriesOrder|published|position" # Check if we have any existing implementation of series navigation echo "Checking for series navigation implementation..." rg -A 5 "previous.*article|next.*article"Length of output: 64312
Script:
#!/bin/bash # Check for any additional series-related fields or implementations echo "Checking for series order implementations..." rg -A 5 "seriesOrder|series.*order|order.*series|position.*series" # Check for any series navigation implementations in components echo "Checking for series navigation components..." fd -e tsx -e ts | xargs rg -l "series.*nav|series.*next|series.*prev"Length of output: 2064
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
drizzle/0000_initial_schema.sql(3 hunks)schema/post.ts(1 hunks)server/db/schema.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- schema/post.ts
🔇 Additional comments (4)
server/db/schema.ts (1)
38-55: 🛠️ Refactor suggestion
Schema design needs improvements
I agree with the past review comments about schema design issues. Additionally:
- Consider adding a
descriptionfield as nullable text to store series descriptions - Add a
slugfield for URL-friendly series names
drizzle/0000_initial_schema.sql (3)
24-25: LGTM! Post table changes are well-implemented.
The nullable seriesId column correctly implements the optional series relationship, and the showComments boolean with a default value follows good practices.
221-226: LGTM! Foreign key constraint is properly implemented.
The ON DELETE SET NULL behavior is appropriate for this optional relationship, and the error handling for duplicate constraints follows best practices.
Line range hint 1-500: Verify the impact on existing data and queries.
Since this is a schema change:
- Ensure there's a rollback plan
- Verify that existing queries handle nullable seriesId
- Consider the performance impact of the new indexes
✅ Verification successful
Schema changes are properly handled with rollback support
The schema changes are well-designed with proper safety measures:
-
Rollback is inherently supported:
seriesIdis nullable withON DELETE SET NULLconstraint- All operations are wrapped in
DO $$ BEGIN/EXCEPTION/END $$blocks to handle duplicates - Foreign key constraints maintain referential integrity
-
Existing queries handle nullable seriesId:
- The codebase already has proper null checks in post router
- Series-related operations (create/unlink) are wrapped in transactions
- Cascading deletes are properly configured
-
Performance considerations:
- No full table scans required as
seriesIdreferences are properly indexed through foreign key - The schema uses appropriate data types and constraints
- No full table scans required as
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing queries that might be affected by the schema changes
rg -l 'SELECT.*FROM.*"Post".*WHERE'
# Look for potential JOIN operations that might need updating
rg -l 'JOIN.*"Post".*ON'
Length of output: 98
Script:
#!/bin/bash
# Check for SQL queries in TypeScript/JavaScript files
rg -t ts -t js 'seriesId|Series' --context 2
# Check for Drizzle schema definitions and migrations
fd -e ts schema
fd -e sql -x cat {} \;
# Look for any Series-related code
rg -l 'Series|seriesId'
Length of output: 49885
| export const seriesRelations = relations(series, ({ one, many }) => ({ | ||
| posts: many(post), | ||
| })); |
There was a problem hiding this comment.
Add user relation to seriesRelations
The series table has a userId field but the relation to the user table is not defined. Add:
export const seriesRelations = relations(series, ({ one, many }) => ({
posts: many(post),
+ user: one(user, { fields: [series.userId], references: [user.id] }),
}));Committable suggestion skipped: line range outside the PR's diff.
drizzle/0000_initial_schema.sql
Outdated
| CREATE TABLE IF NOT EXISTS "Series" ( | ||
| "id" SERIAL PRIMARY KEY, | ||
| "name" TEXT NOT NULL, | ||
| "userId" text NOT NULL, | ||
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | ||
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the Series table with additional columns and indexes.
While the basic structure is correct, consider these improvements for better functionality and performance:
- Add an ordering column for chronological arrangement of posts within a series
- Add a unique index to prevent duplicate series names per user
- Consider adding a soft delete column for better data management
CREATE TABLE IF NOT EXISTS "Series" (
"id" SERIAL PRIMARY KEY,
"name" TEXT NOT NULL,
"userId" text NOT NULL,
+ "orderColumn" INTEGER DEFAULT 0 NOT NULL,
+ "isDeleted" BOOLEAN DEFAULT FALSE NOT NULL,
"createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
"updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL
);
+CREATE UNIQUE INDEX IF NOT EXISTS "Series_name_userId_key" ON "Series" ("name", "userId") WHERE "isDeleted" = 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.
| CREATE TABLE IF NOT EXISTS "Series" ( | |
| "id" SERIAL PRIMARY KEY, | |
| "name" TEXT NOT NULL, | |
| "userId" text NOT NULL, | |
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | |
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | |
| ); | |
| CREATE TABLE IF NOT EXISTS "Series" ( | |
| "id" SERIAL PRIMARY KEY, | |
| "name" TEXT NOT NULL, | |
| "userId" text NOT NULL, | |
| "orderColumn" INTEGER DEFAULT 0 NOT NULL, | |
| "isDeleted" BOOLEAN DEFAULT FALSE NOT NULL, | |
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | |
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | |
| ); | |
| CREATE UNIQUE INDEX IF NOT EXISTS "Series_name_userId_key" ON "Series" ("name", "userId") WHERE "isDeleted" = FALSE; |
|
@NiallJoeMaher any update on this? |
1 similar comment
|
@NiallJoeMaher any update on this? |
|
hey @RangerCreaky sorry we've been busy, I'm taking a look at this |
|
Hey @CarolinaCobo no problem. |
There was a problem hiding this comment.
Hey, sorry for the major delay. I've been crazy busy. A few issues are still here. You can't do migrations in older migrations or they'll never run.
There was a problem hiding this comment.
Ohh.... Sure,
I will push them into a new migration file
| @@ -217,6 +218,7 @@ const Create = ({ session }: { session: Session | null }) => { | |||
| tags, | |||
| canonicalUrl: data.canonicalUrl || undefined, | |||
| excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155), | |||
| seriesName: data.seriesName || undefined | |||
There was a problem hiding this comment.
We should santize this (using trim) on Zod.
|
@NiallJoeMaher, I will work on these changes |
RangerCreaky
had a problem deploying
to
production
GitHub Actions
Failure
|
@NiallJoeMaher I have made the changes, please review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
drizzle/0011_add_series.sql (1)
13-17: Add index on foreign key for better JOIN performanceWhile the foreign key constraint is correctly implemented with appropriate error handling and NULL cascading, consider adding an index on the seriesId column to improve performance when joining Posts with Series.
Add this after the foreign key constraint:
CREATE INDEX "post_seriesid_idx" ON "Post"("seriesId");drizzle/0000_initial_schema.sql (3)
Line range hint
1-186: Critical: Missing Series table implementationThe migration file is missing the required schema changes for the Series feature mentioned in the PR objectives. This includes:
- The "Series" table
- The "seriesId" column in the Post table
- Foreign key constraints between Post and Series tables
Add the following schema changes:
+ CREATE TABLE IF NOT EXISTS "Series" ( + "id" SERIAL PRIMARY KEY NOT NULL, + "name" TEXT NOT NULL, + "userId" TEXT NOT NULL, + "orderColumn" INTEGER DEFAULT 0 NOT NULL, + "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, + "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL + ); + + -- Add series relationship to Post table + ALTER TABLE "Post" ADD COLUMN "seriesId" INTEGER REFERENCES "Series"("id") ON DELETE SET NULL; + + -- Add indexes for performance + CREATE INDEX IF NOT EXISTS "Post_seriesId_idx" ON "Post"("seriesId"); + CREATE UNIQUE INDEX IF NOT EXISTS "Series_name_userId_key" ON "Series"("name", "userId"); + + -- Add foreign key constraints + DO $$ BEGIN + ALTER TABLE "Post" ADD CONSTRAINT "Post_seriesId_fkey" + FOREIGN KEY ("seriesId") REFERENCES "public"."Series"("id") + ON DELETE SET NULL; + EXCEPTION + WHEN duplicate_object THEN null; + END $$; + + DO $$ BEGIN + ALTER TABLE "Series" ADD CONSTRAINT "Series_userId_fkey" + FOREIGN KEY ("userId") REFERENCES "public"."User"("id") + ON DELETE CASCADE; + EXCEPTION + WHEN duplicate_object THEN null; + END $$;
Line range hint
1-186: Consider adding essential columns for series managementTo support the series feature requirements outlined in the PR objectives, consider adding these essential columns:
Add these columns to the Series table:
+ -- Additional columns for Series table + "description" TEXT, -- Series description + "position" INTEGER DEFAULT 0 NOT NULL, -- Position within series + "isComplete" BOOLEAN DEFAULT FALSE, -- Mark series as complete + "publishedAt" TIMESTAMP WITH TIME ZONE -- When series was first published
Line range hint
1-186: Add indexes for performance optimizationTo ensure optimal query performance for series-related operations:
Consider adding these indexes:
-- Index for retrieving posts in a series by position CREATE INDEX IF NOT EXISTS "Post_seriesId_position_idx" ON "Post"("seriesId", "position"); -- Index for finding series by user CREATE INDEX IF NOT EXISTS "Series_userId_idx" ON "Series"("userId"); -- Index for finding published series CREATE INDEX IF NOT EXISTS "Series_publishedAt_idx" ON "Series"("publishedAt");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (4)
app/(editor)/create/[[...paramsArr]]/_client.tsx(5 hunks)drizzle/0000_initial_schema.sql(1 hunks)drizzle/0011_add_series.sql(1 hunks)schema/post.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- schema/post.ts
🔇 Additional comments (4)
drizzle/0011_add_series.sql (2)
11-11: LGTM!
The nullable seriesId column correctly implements the optional relationship between posts and series.
1-17: Verify chronological ordering implementation
The PR objectives mention that articles within a series should be chronologically ordered. Please verify that the Post table has the necessary fields (e.g., publishedAt) to support this requirement.
✅ Verification successful
Chronological ordering is supported through existing timestamp fields
The Post table already has the necessary timestamp fields to support chronological ordering of articles within a series:
published- timestamp with timezone for publication datecreatedAt- timestamp with timezone for creation date (with default value)
These fields provide sufficient support for implementing chronological ordering of articles within a series.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Post table has necessary timestamp fields for chronological ordering
rg -A 10 'CREATE TABLE.*"Post"'
Length of output: 803
drizzle/0000_initial_schema.sql (1)
Line range hint 1-186: Verify the impact on existing data
The schema changes for the Series feature need careful consideration for existing data:
✅ Verification successful
Let me check the content of the series migration file to understand the schema changes better.
No data migration concerns for Series feature
The schema changes for the Series feature are safe as:
- The new
Seriestable is being created fresh without affecting existing tables - The
Posttable is only being altered to add a nullableseriesIdcolumn withON DELETE SET NULLconstraint - No existing columns in the initial schema contain series-related data that needs migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing posts that might be affected by the schema changes
# Check for any existing posts that might be part of a series
rg -l "series|part|chapter" --type sql
# Look for potential series indicators in post titles
rg "Part \d|Chapter \d|Series:" --type sql
# Check for any existing foreign key constraints that might conflict
rg "FOREIGN KEY.*Series" --type sql
Length of output: 316
Script:
#!/bin/bash
# Check the content of the series migration file
cat drizzle/0011_add_series.sql
# Check if there are any posts with series-related columns
rg "seriesId|seriesPosition" drizzle/0000_initial_schema.sql
Length of output: 661
app/(editor)/create/[[...paramsArr]]/_client.tsx (1)
581-593: 🛠️ Refactor suggestion
Enhance series name input validation and user experience.
The current implementation lacks:
- Input validation for series name format
- Maximum length constraint
- Visual feedback for validation errors
Consider these improvements:
<label htmlFor="seriesName">
Series Name
</label>
<input
id="seriesName"
type="text"
placeholder="The name of my series"
defaultValue={data?.series?.name || ""}
+ maxLength={50}
+ pattern="^[a-zA-Z0-9\s-]+$"
{...register("seriesName", {
+ maxLength: {
+ value: 50,
+ message: "Series name cannot exceed 50 characters"
+ },
+ pattern: {
+ value: /^[a-zA-Z0-9\s-]+$/,
+ message: "Series name can only contain letters, numbers, spaces, and hyphens"
+ }
})}
/>
+{errors.seriesName && (
+ <p className="mt-1 text-sm text-red-600">
+ {errors.seriesName.message}
+ </p>
+)}
<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
+ Series names are case-sensitive and can contain letters, numbers, spaces, and hyphens (max 50 characters). Type it exactly as you did in previous articles to ensure they are connected.
</p>Let's verify if there are any existing series names that would be invalid under the new validation rules:
| CREATE TABLE IF NOT EXISTS "Series" ( | ||
| "id" SERIAL PRIMARY KEY, | ||
| "name" TEXT NOT NULL, | ||
| "userId" text NOT NULL, | ||
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | ||
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add necessary indexes and constraints to the Series table
Consider the following improvements for better performance and data integrity:
- Add a composite unique constraint on (name, userId) to prevent duplicate series names per user
- Add an index on userId for faster user-specific queries
- Add an index on createdAt for chronological ordering
- Add a length limit to the name field
Apply this modification:
CREATE TABLE IF NOT EXISTS "Series" (
"id" SERIAL PRIMARY KEY,
- "name" TEXT NOT NULL,
+ "name" VARCHAR(255) NOT NULL,
"userId" text NOT NULL,
"createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL,
"updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL
);
+CREATE UNIQUE INDEX "series_name_userid_unique_idx" ON "Series"("name", "userId");
+CREATE INDEX "series_userid_idx" ON "Series"("userId");
+CREATE INDEX "series_createdat_idx" ON "Series"("createdAt");📝 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.
| CREATE TABLE IF NOT EXISTS "Series" ( | |
| "id" SERIAL PRIMARY KEY, | |
| "name" TEXT NOT NULL, | |
| "userId" text NOT NULL, | |
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | |
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | |
| ); | |
| CREATE TABLE IF NOT EXISTS "Series" ( | |
| "id" SERIAL PRIMARY KEY, | |
| "name" VARCHAR(255) NOT NULL, | |
| "userId" text NOT NULL, | |
| "createdAt" TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP NOT NULL, | |
| "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL | |
| ); | |
| CREATE UNIQUE INDEX "series_name_userid_unique_idx" ON "Series"("name", "userId"); | |
| CREATE INDEX "series_userid_idx" ON "Series"("userId"); | |
| CREATE INDEX "series_createdat_idx" ON "Series"("createdAt"); |
|
|
||
| const sanitizedSeriesName = FormDataSchema.parse({ | ||
| seriesName: data.seriesName, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add error handling for series name sanitization.
The current implementation has two potential issues:
- Missing error handling for the Zod schema parsing
- Using spread operator could accidentally override other fields if FormDataSchema includes more fields
Consider this safer implementation:
- const sanitizedSeriesName = FormDataSchema.parse({
- seriesName: data.seriesName,
- });
-
+ let sanitizedSeriesName;
+ try {
+ const result = FormDataSchema.parse({
+ seriesName: data.seriesName,
+ });
+ sanitizedSeriesName = result.seriesName;
+ } catch (error) {
+ toast.error("Invalid series name format");
+ return;
+ }
+
const formData = {
...data,
tags,
canonicalUrl: data.canonicalUrl || undefined,
excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155),
- ...sanitizedSeriesName
+ seriesName: sanitizedSeriesName
};📝 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 sanitizedSeriesName = FormDataSchema.parse({ | |
| seriesName: data.seriesName, | |
| }); | |
| let sanitizedSeriesName; | |
| try { | |
| const result = FormDataSchema.parse({ | |
| seriesName: data.seriesName, | |
| }); | |
| sanitizedSeriesName = result.seriesName; | |
| } catch (error) { | |
| toast.error("Invalid series name format"); | |
| return; | |
| } | |
| const formData = { | |
| ...data, | |
| tags, | |
| canonicalUrl: data.canonicalUrl || undefined, | |
| excerpt: data.excerpt || removeMarkdown(data.body, {}).substring(0, 155), | |
| seriesName: sanitizedSeriesName | |
| }; |
|
@NiallJoeMaher any update on this? |
|
@RangerCreaky |
|
@John-Paul-Larkin |
✨ Codu Pull Request 💻
Fixes #(issue)
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
Associated Screenshots
WORKING GIF:
