Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion app/(editor)/create/[[...paramsArr]]/_client.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,14 @@ const Create = ({ session }: { session: Session | null }) => {
Sentry.captureException(error);
},
});

const { mutate: seriesUpdate, status: seriesStatus } = api.series.update.useMutation({
onError(error) {
toast.error("Error updating series");
Sentry.captureException(error);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider enhancing the series mutation implementation.

The mutation could benefit from success handling and optimistic updates to improve user experience.

Consider adding:

 const { mutate: seriesUpdate, status: seriesStatus } = api.series.update.useMutation({
+  onSuccess: () => {
+    toast.success("Series updated successfully");
+  },
   onError(error) {
     toast.error("Error updating series");
     Sentry.captureException(error);
   }
 });
📝 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.

Suggested change
const { mutate: seriesUpdate, status: seriesStatus } = api.series.update.useMutation({
onError(error) {
toast.error("Error updating series");
Sentry.captureException(error);
}
});
const { mutate: seriesUpdate, status: seriesStatus } = api.series.update.useMutation({
onSuccess: () => {
toast.success("Series updated successfully");
},
onError(error) {
toast.error("Error updating series");
Sentry.captureException(error);
}
});


const {
mutate: create,
data: createData,
Expand Down Expand Up @@ -217,6 +225,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should santize this (using trim) on Zod.

};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

return formData;
};
Expand All @@ -229,6 +238,8 @@ const Create = ({ session }: { session: Session | null }) => {
await create({ ...formData });
} else {
await save({ ...formData, id: postId });
await seriesUpdate({ postId, seriesName: formData.seriesName });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error handling for series update.

The series update occurs after post save, but there's no handling for the case where post save succeeds but series update fails. This could lead to inconsistent state.

Consider wrapping both operations in a try-catch:

-      await save({ ...formData, id: postId });
-      await seriesUpdate({ postId, seriesName: formData.seriesName });
+      try {
+        await save({ ...formData, id: postId });
+        await seriesUpdate({ postId, seriesName: formData.seriesName });
+      } catch (error) {
+        toast.error("Failed to save all changes. Please try again.");
+        Sentry.captureException(error);
+        return;
+      }

Committable suggestion was skipped due to low confidence.


setSavedTime(
new Date().toLocaleString(undefined, {
dateStyle: "medium",
Expand Down Expand Up @@ -564,10 +575,24 @@ const Create = ({ session }: { session: Session | null }) => {
{copied ? "Copied" : "Copy Link"}
</div>
</button>
<p className="mt-2 text-sm text-neutral-600 dark:text-neutral-400">
<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?.name || ""}
{...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>
Comment on lines +581 to +593
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation for series name.

While the UI implementation is good, consider adding validation to ensure consistent series names.

Consider adding validation:

 <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", {
+    validate: {
+      format: (value) => 
+        !value || /^[a-zA-Z0-9\s-]+$/.test(value) || 
+        "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>
+)}

Committable suggestion was skipped due to low confidence.

</DisclosurePanel>
</>
)}
Expand Down
14 changes: 14 additions & 0 deletions drizzle/0011_add_series_update_post.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
-- 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
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Add a unique constraint on the "name" column to prevent duplicate series names
  2. Add an index on the "name" column to improve lookup performance
  3. 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.

-- 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
-- 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");

1 change: 1 addition & 0 deletions schema/post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Prevent empty strings after trimming
  2. Limit series name length to maintain UI consistency
  3. 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.

Suggested change
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()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a sensible addition @RangerCreaky

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

savePostSchema reference

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.

});

export const PublishPostSchema = z.object({
Expand Down
6 changes: 6 additions & 0 deletions schema/series.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import z from "zod";

export const UpdateSeriesSchema = z.object({
postId: z.string(),
seriesName: z.string().trim().optional()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably can share the schema fix as above here too to ensure the name is valid when provided.

});
3 changes: 3 additions & 0 deletions server/api/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { adminRouter } from "./admin";
import { reportRouter } from "./report";
import { tagRouter } from "./tag";

import { seriesRouter } from "./series";

export const appRouter = createTRPCRouter({
post: postRouter,
profile: profileRouter,
Expand All @@ -16,6 +18,7 @@ export const appRouter = createTRPCRouter({
admin: adminRouter,
report: reportRouter,
tag: tagRouter,
series: seriesRouter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the logic needs to be separated from the post logic since we create a series from within a post

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, Will merge this logic with the post logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NiallJoeMaher made this update
Merged this logic with the post logic

});

// export type definition of API
Expand Down
28 changes: 23 additions & 5 deletions server/api/router/post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
GetLimitSidePosts,
} from "../../../schema/post";
import { removeMarkdown } from "../../../utils/removeMarkdown";
import { bookmark, like, post, post_tag, tag, user } from "@/server/db/schema";
import { bookmark, like, post, post_tag, tag, user, series } from "@/server/db/schema";
import {
and,
eq,
Expand Down Expand Up @@ -187,10 +187,27 @@ export const postRouter = createTRPCRouter({
});
}

const [deletedPost] = await ctx.db
.delete(post)
.where(eq(post.id, id))
.returning();
const deletedPost = await ctx.db.transaction(async (tx) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, naming here should be ctx

Copy link
Contributor Author

@RangerCreaky RangerCreaky Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would txCtx be a good middle ground?

ie. its the transaction context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnAllenTech This looks good to me.
Let me know if that works.

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;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve series cleanup implementation.

While the transaction usage is good for maintaining data consistency, there are several improvements that could be made:

  1. 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;
  1. 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.


return deletedPost;
}),
Expand Down Expand Up @@ -428,6 +445,7 @@ export const postRouter = createTRPCRouter({
where: (posts, { eq }) => eq(posts.id, id),
with: {
tags: { with: { tag: true } },
series: true
},
});

Expand Down
137 changes: 137 additions & 0 deletions server/api/router/series.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
import { TRPCError } from "@trpc/server";
import { createTRPCRouter, protectedProcedure } from "../trpc";
import { series, post } from "@/server/db/schema";
import { UpdateSeriesSchema } from "@/schema/series";
import {eq, and} from "drizzle-orm";
export const seriesRouter = createTRPCRouter({
update: protectedProcedure
.input(UpdateSeriesSchema)
.mutation(async ({input, ctx}) => {
const {postId, seriesName} = input;

if (seriesName && seriesName.trim() === "") {
throw new TRPCError({ code: 'BAD_REQUEST', message: 'Series name cannot be empty' });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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' });
}


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),
});

if (!currentPost) {
throw new TRPCError({ code: 'NOT_FOUND' });
}
if (currentPost?.userId !== ctx.session.user.id) {
throw new TRPCError({
code: "FORBIDDEN",
});
}
const createNewSeries = async (seriesTitle: string) => {
// check if a series with that name already exists
// or else create a new one
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)
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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


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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

// update that series id in the current post
await tx
.update(post)
.set({
seriesId: seriesId
})
.where(eq(post.id, currentPost.id));
})

}

const unlinkSeries = async (seriesId: number) => {
// Check if the user has added a another post with the same series id previously
return await ctx.db.transaction(async (tx) =>{
const anotherPostInThisSeries = await tx.query.post.findFirst({
where: (post, { eq, and, ne }) =>
and (
ne(post.id, currentPost.id),
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));
})
}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is probably too many comments here from the AI generation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure Mutation Returns a Response

Currently, the update mutation does not return any response to the client. Consider returning a success message or the updated post for client-side confirmation.

Apply this diff to return a success status:

            })
+           return { status: 'success' };
        })
📝 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.

Suggested change
})
})
})
return { status: 'success' };
})

28 changes: 28 additions & 0 deletions server/db/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,27 @@ export const sessionRelations = relations(session, ({ one }) => ({
}),
}));

export const series = pgTable("Series", {
id: serial("id").primaryKey(),
name: text("title").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`),
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Multiple schema design issues need attention

  1. Table naming should follow snake_case convention like other tables
  2. Column "title" should be "name" for consistency with schema design
  3. Add index on name/title column for query performance
  4. Add unique constraint on name per user to prevent duplicates
  5. Align timestamp configurations with other tables

Apply these changes:

-export const series = pgTable("Series", {
+export const series = pgTable("series", {
   id: serial("id").primaryKey(),
-  name: text("title").notNull(),
+  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())
+  })
+  .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),
 }));
📝 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.

Suggested change
export const series = pgTable("Series", {
id: serial("id").primaryKey(),
name: text("title").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),
}));



export const account = pgTable(
"account",
{
Expand Down Expand Up @@ -149,6 +170,7 @@ export const post = pgTable(
.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" }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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),
};
},

},
(table) => {
return {
Expand All @@ -168,6 +190,7 @@ export const postRelations = relations(post, ({ one, many }) => ({
notifications: many(notification),
user: one(user, { fields: [post.userId], references: [user.id] }),
tags: many(post_tag),
series: one(series,{ fields: [post.seriesId], references: [series.id] }),
}));

export const user = pgTable(
Expand Down Expand Up @@ -273,6 +296,11 @@ export const bookmark = pgTable(
},
);


export const seriesRelations = relations(series, ({ one, many }) => ({
posts: many(post),
}));
Comment on lines +299 to +301
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


export const bookmarkRelations = relations(bookmark, ({ one, many }) => ({
post: one(post, { fields: [bookmark.postId], references: [post.id] }),
user: one(user, { fields: [bookmark.userId], references: [user.id] }),
Expand Down