Skip to content
Open
Show file tree
Hide file tree
Changes from 10 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
19 changes: 18 additions & 1 deletion app/(editor)/create/[[...paramsArr]]/_client.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ const Create = ({ session }: { session: Session | null }) => {
Sentry.captureException(error);
},
});

const {
mutate: create,
data: createData,
Expand Down Expand Up @@ -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
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 +231,7 @@ const Create = ({ session }: { session: Session | null }) => {
await create({ ...formData });
} else {
await save({ ...formData, id: postId });

setSavedTime(
new Date().toLocaleString(undefined, {
dateStyle: "medium",
Expand Down Expand Up @@ -564,10 +567,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
18 changes: 18 additions & 0 deletions drizzle/0011_add_series_update_post.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
-- Create Series table
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
);
-- 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 INDEX idx_series_name ON "Series"("name");
ALTER TABLE series ADD CONSTRAINT unique_series_name_per_user UNIQUE (name, userId);
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

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.

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

2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions schema/post.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ 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()
});

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.

});
2 changes: 1 addition & 1 deletion server/api/router/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const appRouter = createTRPCRouter({
notification: notificationRouter,
admin: adminRouter,
report: reportRouter,
tag: tagRouter,
tag: tagRouter
});

// export type definition of API
Expand Down
217 changes: 163 additions & 54 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 @@ -52,10 +52,13 @@ export const postRouter = createTRPCRouter({
update: protectedProcedure
.input(SavePostSchema)
.mutation(async ({ input, ctx }) => {
const { id, body, title, excerpt, canonicalUrl, tags = [] } = input;
const { id, body, title, excerpt, canonicalUrl, tags = [], seriesName } = input;

const currentPost = await ctx.db.query.post.findFirst({
where: (posts, { eq }) => eq(posts.id, id),
with: {
series: true
},
});

if (currentPost?.userId !== ctx.session.user.id) {
Expand All @@ -64,6 +67,94 @@ export const postRouter = createTRPCRouter({
});
}

// series
const postId = currentPost.id;

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

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

}
Comment on lines +77 to +110
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 error handling and fix potential race condition in createNewSeries.

The function has two issues:

  1. Missing error handling for the transaction
  2. 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.

Suggested change
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 unlinkSeries = async (seriesId: number) => {
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 (!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) {
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);
}
}



// if user doesnt link any tags to the article no point in doing the tag operations
// This also makes autosave during writing faster
if (tags.length > 0) {
Expand Down Expand Up @@ -105,7 +196,7 @@ export const postRouter = createTRPCRouter({
return excerpt && excerpt.length > 0
? excerpt
: // @Todo why is body string | null ?
removeMarkdown(currentPost.body as string, {}).substring(0, 156);
removeMarkdown(currentPost.body as string, {}).substring(0, 156);
}
return excerpt;
};
Expand Down Expand Up @@ -187,10 +278,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;
});
Comment on lines +281 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.

🛠️ Refactor suggestion

Improve series cleanup in delete mutation.

The series cleanup logic has similar issues to those in the update mutation:

  1. Missing error handling
  2. Unnecessary non-null assertion
  3. 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.

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


return deletedPost;
}),
Expand All @@ -203,33 +311,33 @@ export const postRouter = createTRPCRouter({

setLiked
? await ctx.db.transaction(async (tx) => {
res = await tx.insert(like).values({ postId, userId }).returning();
res = await tx.insert(like).values({ postId, userId }).returning();
await tx
.update(post)
.set({
likes: increment(post.likes),
})
.where(eq(post.id, postId));
})
: await ctx.db.transaction(async (tx) => {
res = await tx
.delete(like)
.where(
and(
eq(like.postId, postId),
eq(like.userId, ctx.session?.user?.id),
),
)
.returning();
if (res.length !== 0) {
await tx
.update(post)
.set({
likes: increment(post.likes),
likes: decrement(post.likes),
})
.where(eq(post.id, postId));
})
: await ctx.db.transaction(async (tx) => {
res = await tx
.delete(like)
.where(
and(
eq(like.postId, postId),
eq(like.userId, ctx.session?.user?.id),
),
)
.returning();
if (res.length !== 0) {
await tx
.update(post)
.set({
likes: decrement(post.likes),
})
.where(eq(post.id, postId));
}
});
}
});

return res;
}),
Expand All @@ -241,16 +349,16 @@ export const postRouter = createTRPCRouter({

setBookmarked
? await ctx.db
.insert(bookmark)
.values({ postId, userId: ctx.session?.user?.id })
.insert(bookmark)
.values({ postId, userId: ctx.session?.user?.id })
: await ctx.db
.delete(bookmark)
.where(
and(
eq(bookmark.postId, postId),
eq(bookmark.userId, ctx.session?.user?.id),
),
);
.delete(bookmark)
.where(
and(
eq(bookmark.postId, postId),
eq(bookmark.userId, ctx.session?.user?.id),
),
);
return res;
}),
sidebarData: publicProcedure
Expand All @@ -267,26 +375,26 @@ export const postRouter = createTRPCRouter({
// if user not logged in and they wont have any liked posts so default to a count of 0
ctx.session?.user?.id
? ctx.db
.selectDistinct()
.from(like)
.where(
and(
eq(like.postId, id),
eq(like.userId, ctx.session.user.id),
),
)
.selectDistinct()
.from(like)
.where(
and(
eq(like.postId, id),
eq(like.userId, ctx.session.user.id),
),
)
: [false],
// if user not logged in and they wont have any bookmarked posts so default to a count of 0
ctx.session?.user?.id
? ctx.db
.selectDistinct()
.from(bookmark)
.where(
and(
eq(bookmark.postId, id),
eq(bookmark.userId, ctx.session.user.id),
),
)
.selectDistinct()
.from(bookmark)
.where(
and(
eq(bookmark.postId, id),
eq(bookmark.userId, ctx.session.user.id),
),
)
: [false],
]);
return {
Expand Down Expand Up @@ -428,6 +536,7 @@ export const postRouter = createTRPCRouter({
where: (posts, { eq }) => eq(posts.id, id),
with: {
tags: { with: { tag: true } },
series: true
},
});

Expand Down
Loading
Loading