Fix broken dashboard and implement missing features#22
Closed
Conversation
…tations - Fix collections page querying wrong table (userPosts → collections) - Fix inverted filter validation in user post display - Fix collections API returning undefined (arrow fn body) - Fix missing offset validation throw in public_data API - Fix Zod validation (z.email() → z.string().email()) in login - Fix Content-Type header typos (appilcation → application) - Fix admin settings toggles sending wrong values and overwriting state - Fix post edit system (remove invalid metadata export, add tag editing) - Remove debug console.logs and fix typos (instence, geint-sans) - Implement collections list & create pages with API - Implement share link / URL shortener API (was returning 403) - Fix user account page (name save, password reset) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis pull request introduces collection management features with new API endpoints and UI components, refactors share link creation, updates slug-based routing for collections, and applies numerous bug fixes including typo corrections, improved error logging, removed console statements, and CSS class name corrections across the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/app/user/[userid]/postDisplay.tsx (1)
25-32:⚠️ Potential issue | 🔴 CriticalThis type guard rejects every valid filter array.
apps/web/src/components/publicPostsAndVideos.tsx:197-200definesFilterFormatas an object shape, butisValidFilterObject()recursively re-checks each item as if it were another array. With the new!isValidFilterObject(parsed)branch, valid filters now always throw and the page silently drops filtering.💡 Proposed fix
- const isValidFilterObject = (arr: any): arr is FilterFormat[] => { - return Array.isArray(arr) && arr.every((item) => isValidFilterObject(item)); - }; + const isValidFilterObject = (arr: unknown): arr is FilterFormat[] => { + return ( + Array.isArray(arr) && + arr.every( + (item): item is FilterFormat => + typeof item === "object" && + item !== null && + "by" in item && + "filter" in item && + item.by === "tag" && + typeof item.filter === "string", + ) + ); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/user/`[userid]/postDisplay.tsx around lines 25 - 32, The type guard isValidFilterObject incorrectly recurses into items as arrays causing all valid FilterFormat[] to be rejected; update isValidFilterObject to verify arr is an array and every item satisfies the FilterFormat shape (e.g., implement or call a helper isFilterFormat that checks required keys/types on each item) instead of calling isValidFilterObject(item) recursively; apply this to the parsed variable after JSON.parse(filters) so the !isValidFilterObject(parsed) branch only throws for truly invalid filter objects.apps/web/src/app/api/data/settings/route.ts (1)
324-339:⚠️ Potential issue | 🟠 MajorReturn success from
change_perms.
auth.api.adminUpdateUser()can succeed here, but this branch never returns a response, so control falls through to the generic failure payload at Lines 367-375. The permission change is applied while the caller sees{ success: false }.💡 Proposed fix
await auth.api.adminUpdateUser({ body: { userId: body.user, data: { role: body.role }, }, headers: await headers(), }); + return Response.json({ success: true, msg: "" }, { status: 200 }); } catch (e: any) { console.error(e); throw new Error(e.message || "ERR_GENERIC"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/data/settings/route.ts` around lines 324 - 339, The change_perms branch fails to return a success response after calling auth.api.adminUpdateUser, causing callers to receive the generic failure; update the branch handling body.action === "change_perms" (in route.ts) so that after await auth.api.adminUpdateUser(...) you set statusCode = 200 and return the success payload (e.g., { success: true }) or otherwise early-return the proper response instead of falling through to the generic failure handler; keep the existing self-change check (body.user === session.user.id) and error catch as-is but ensure successful path returns immediately.apps/web/src/app/api/data/public_data/route.ts (1)
15-23:⚠️ Potential issue | 🟡 MinorReturn 400 for invalid
offsetvalues.These branches throw client-input errors, but the catch block still hard-codes HTTP 500. Invalid
offsetvalues will keep showing up as server failures, so the newERR_OFFSET_PARAM_NOT_A_NUMBERpath does not actually fix the API contract.💡 Proposed fix
export const GET = async (request: NextRequest) => { + let statusCode = 200; try { const { searchParams } = new URL(request.url); const offset = searchParams.get("offset"); @@ if (offset === null) { + statusCode = 400; throw new Error("ERR_NO_PARAMS_TO_USE"); } if (!/^\d+$/.test(offset)) { + statusCode = 400; throw new Error("ERR_OFFSET_PARAM_NOT_A_NUMBER"); } if (!Number.isSafeInteger(Number(offset))) { + statusCode = 400; throw new Error("ERR_OFFSET_PARAM_NOT_A_SAFE_INTEGER"); } @@ return Response.json( { success: false, msg: e.message, result: [] }, { - status: 500, + status: statusCode || 500, }, ); } };Also applies to: 64-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/data/public_data/route.ts` around lines 15 - 23, The offset validation currently throws generic Errors (ERR_NO_PARAMS_TO_USE, ERR_OFFSET_PARAM_NOT_A_NUMBER, ERR_OFFSET_PARAM_NOT_A_SAFE_INTEGER) but the catch block always returns HTTP 500; change the validation to surface client errors and make the catch mapper return 400 for these cases: either throw a Response with status 400 or throw an Error object with a status property (status = 400) and update the catch in the same route handler to check for those specific error codes/status and return a 400 response, falling back to 500 for other errors; apply the same change for the second validation block mentioned (the similar checks later in the file).
🧹 Nitpick comments (5)
apps/web/src/app/dashboard/posts/edit/[slug]/client.tsx (4)
103-110: Consider adding an accessible label for the tag delete button.The button lacks an
aria-label, which may make it unclear to screen reader users that clicking removes the tag.Accessibility improvement
-<button key={it} onClick={() => deleteTag(it)}> +<button key={it} onClick={() => deleteTag(it)} aria-label={`Remove tag ${it}`}> <Badge🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx around lines 103 - 110, The delete button wrapping the Badge (rendered in the map that calls deleteTag) lacks an accessible label; update the button in the client component so it includes an aria-label (e.g., aria-label={`Remove tag ${it}`} or aria-label={`Remove ${it} tag`}) and/or a visually-hidden text node to make its purpose clear to screen readers while keeping the visual Badge unchanged; ensure this change is applied to the button element that invokes deleteTag({it}) so screen readers announce the removal action.
4-4: Consider using a path alias instead of deep relative imports.The 8-level relative path
../../../../../../../../packages/db/srcis fragile and will break if the file moves. If your monorepo has path aliases configured (e.g.,@repo/db), prefer using them for cross-package imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx at line 4, The import for main_schema uses a fragile deep relative path ("import { main_schema } from '../../../../../../../../packages/db/src'"); replace this with your monorepo path alias (e.g., "import { main_schema } from '@repo/db'") or update tsconfig/webpack path mappings and use that alias throughout; update the import in apps/web/src/app/dashboard/posts/edit/[slug]/client.tsx to reference the alias so future file moves won't break the cross-package import and ensure the alias points to the package's exported entry (packages/db/src or its build output).
52-64:replaceAll(" ", "")removes all spaces, including internal ones.Using
replaceAll(" ", "")will convert a tag like"my tag"into"mytag". If you only intend to remove leading/trailing whitespace, use.trim()instead. If this is intentional for hashtag-style tags (no spaces allowed), consider adding a brief comment to clarify.If trimming is the intent
const addTag = () => { - const trimmed = tagInput.replaceAll(" ", ""); + const trimmed = tagInput.trim(); if (trimmed.length === 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx around lines 52 - 64, In addTag, using tagInput.replaceAll(" ", "") removes internal spaces (e.g., "my tag" → "mytag"); change this to use tagInput.trim() to only strip leading/trailing whitespace and preserve internal spaces, or alternatively enforce/no-allow internal spaces by validating tagInput.includes(" ") and showing an error; update the logic around trimmed (from tagInput) and keep the existing checks that use tags, setPost, setTagInput, and toast for consistent behavior.
74-76: Add type assertion forstatusto ensure type safety.The
onValueChangecallback provides astring, butpost.statusexpects a specific union type. Adding a type assertion prevents potential TypeScript errors and documents the expected values.Proposed fix
onValueChange={(vl) => { - setPost({ ...post, status: vl }); + setPost({ ...post, status: vl as Post["status"] }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx around lines 74 - 76, The onValueChange handler is assigning a string (vl) to post.status which expects a narrower union type; update the handler to assert vl to the expected status union before setting state (e.g., use a type assertion when calling setPost so status: vl as PostStatus) and/or use the functional setForm pattern (setPost(prev => ({ ...prev, status: vl as PostStatus }))) to ensure type safety; locate the onValueChange callback and change the assignment to include the assertion referring to setPost, post, status, and onValueChange.apps/web/src/app/api/data/create_share_link/route.ts (1)
14-20: Consider using crypto for slug generation.
Math.random()is not cryptographically secure. While this is acceptable for URL slugs where predictability isn't a critical security concern, usingcrypto.randomBytesorcrypto.getRandomValueswould be more robust against potential enumeration attacks.♻️ Optional improvement using crypto
+import { randomBytes } from "crypto"; + const characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; function generateSlug(length = 8) { - let slug = ""; - for (let i = 0; i < length; i++) { - slug += characters.charAt(Math.floor(Math.random() * characters.length)); - } - return slug; + const bytes = randomBytes(length); + return Array.from(bytes) + .map((b) => characters[b % characters.length]) + .join(""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/data/create_share_link/route.ts` around lines 14 - 20, The generateSlug function currently builds the slug using Math.random() (see function generateSlug and the characters variable); replace the non-cryptographic RNG with a crypto-backed source—e.g., use Node's crypto.randomBytes or the Web Crypto API's crypto.getRandomValues—to generate unbiased random indices into characters, ensuring you map bytes to the characters length without modulo bias (reject out-of-range bytes or scale correctly). Update generateSlug to use the chosen crypto API (with a small wrapper to support both environments if needed) and keep the same function signature and behavior otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/api/data/create_collection/route.ts`:
- Around line 37-45: The current pre-check using
db.select(...).where(dorm.eq(main_schema.collections.slug, slug)) is race-prone;
instead let the insert be authoritative and catch unique-constraint violations
from the insert into main_schema.collections, translating that specific
duplicate-key error into a 409 with the message "A collection with this slug
already exists". Remove or treat the select pre-check as advisory, perform the
insert directly, and update the existing catch/error-handling around the insert
to detect the DB unique-constraint error for main_schema.collections.slug (and
return statusCode = 409) while allowing other errors to bubble as before; apply
the same change to the second similar block referenced (lines 49-55).
In `@apps/web/src/app/api/data/create_share_link/route.ts`:
- Around line 90-96: The catch block in create_share_link route returns
e.message to the client which can leak internals; instead, log the full error
server-side (using console.error or process logger) and return a generic
Response.json payload like { success: false, msg: "Internal server error" } with
status 500; update the catch in route.ts (the catch surrounding the
createShareLink handler / Response.json call) to remove e.message from the
response while preserving detailed logging.
- Around line 53-62: Validate that targetUrl is a well-formed URL and
sanitize/validate customSlug before using it: parse/validate targetUrl (e.g.,
via the URL constructor or a strict regex) and return a 400 response if invalid;
for customSlug, trim it, enforce a max length (e.g., 64 chars), allow only safe
characters (a-z0-9 and hyphens/underscores) and reject or normalize anything
else (or fall back to generateSlug()), and ensure generateSlug() is still used
when customSlug is absent or invalid; apply these checks where targetUrl and
customSlug are read in the create_share_link route handler.
- Around line 64-82: The check-then-insert for main_schema.urlShorter with
urlSlug can race; wrap the db.insert(main_schema.urlShorter).values(...) in a
try/catch and handle unique-constraint failures by returning the same 409
Response.json({ success: false, msg: "Slug already taken" }, { status: 409 });
detect the constraint violation by inspecting the DB error (e.g., Postgres code
'23505' or the error.constraint/name/message) so legitimate unique-violation
errors map to 409 while other errors rethrow or return a 500.
In `@apps/web/src/app/dashboard/collections/client.tsx`:
- Around line 25-27: The component is calling toast.error(error.message) during
render which will trigger on every re-render while error is truthy; move this
logic into a useEffect so the toast fires only once when error changes: add
import of useEffect (if missing), create a useEffect hook that depends on
[error] and inside it check if (error) then call toast.error(error.message);
keep the original error variable and toast.error call but remove the direct call
from the component body to prevent duplicate toasts.
In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx:
- Around line 93-98: The onKeyDown handler in the tag input currently checks for
e.key === "Space", which never matches — change the condition in the onKeyDown
callback (where addTag() is invoked) to detect space correctly by using e.key
=== " " (a literal space) or e.code === "Space" (to be robust across layouts),
e.g. check (e.key === "Enter" || e.key === " " || e.code === "Space") before
calling addTag() and e.preventDefault().
In `@apps/web/src/app/dashboard/user/account/client.tsx`:
- Line 224: The password dialog currently only toggles visibility via
onOpenChange={setPasswordDialogOpen}, so when it’s dismissed (Escape/outside
click) sensitive fields currentPassword, newPassword, and confirmPassword remain
populated; update the handler to a custom function (e.g.,
handlePasswordDialogChange) that calls setPasswordDialogOpen(open) and, when
open is false, also clears all three state variables (setCurrentPassword(''),
setNewPassword(''), setConfirmPassword('')) so the fields are reset whenever the
Dialog component (Dialog / passwordDialogOpen / setPasswordDialogOpen) is
closed.
- Around line 111-126: handleSaveProfile currently calls authClient.updateUser
but does not refresh the session, leaving session-dependent UI stale; after a
successful update (inside handleSaveProfile after authClient.updateUser resolves
and before toast.success), call the session refetch provided by
useSession().refetch() (or alternatively call router.refresh()) to revalidate
the client session so components relying on authClient.useSession() (e.g.,
user-menu.tsx) show the updated name immediately.
---
Outside diff comments:
In `@apps/web/src/app/api/data/public_data/route.ts`:
- Around line 15-23: The offset validation currently throws generic Errors
(ERR_NO_PARAMS_TO_USE, ERR_OFFSET_PARAM_NOT_A_NUMBER,
ERR_OFFSET_PARAM_NOT_A_SAFE_INTEGER) but the catch block always returns HTTP
500; change the validation to surface client errors and make the catch mapper
return 400 for these cases: either throw a Response with status 400 or throw an
Error object with a status property (status = 400) and update the catch in the
same route handler to check for those specific error codes/status and return a
400 response, falling back to 500 for other errors; apply the same change for
the second validation block mentioned (the similar checks later in the file).
In `@apps/web/src/app/api/data/settings/route.ts`:
- Around line 324-339: The change_perms branch fails to return a success
response after calling auth.api.adminUpdateUser, causing callers to receive the
generic failure; update the branch handling body.action === "change_perms" (in
route.ts) so that after await auth.api.adminUpdateUser(...) you set statusCode =
200 and return the success payload (e.g., { success: true }) or otherwise
early-return the proper response instead of falling through to the generic
failure handler; keep the existing self-change check (body.user ===
session.user.id) and error catch as-is but ensure successful path returns
immediately.
In `@apps/web/src/app/user/`[userid]/postDisplay.tsx:
- Around line 25-32: The type guard isValidFilterObject incorrectly recurses
into items as arrays causing all valid FilterFormat[] to be rejected; update
isValidFilterObject to verify arr is an array and every item satisfies the
FilterFormat shape (e.g., implement or call a helper isFilterFormat that checks
required keys/types on each item) instead of calling isValidFilterObject(item)
recursively; apply this to the parsed variable after JSON.parse(filters) so the
!isValidFilterObject(parsed) branch only throws for truly invalid filter
objects.
---
Nitpick comments:
In `@apps/web/src/app/api/data/create_share_link/route.ts`:
- Around line 14-20: The generateSlug function currently builds the slug using
Math.random() (see function generateSlug and the characters variable); replace
the non-cryptographic RNG with a crypto-backed source—e.g., use Node's
crypto.randomBytes or the Web Crypto API's crypto.getRandomValues—to generate
unbiased random indices into characters, ensuring you map bytes to the
characters length without modulo bias (reject out-of-range bytes or scale
correctly). Update generateSlug to use the chosen crypto API (with a small
wrapper to support both environments if needed) and keep the same function
signature and behavior otherwise.
In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx:
- Around line 103-110: The delete button wrapping the Badge (rendered in the map
that calls deleteTag) lacks an accessible label; update the button in the client
component so it includes an aria-label (e.g., aria-label={`Remove tag ${it}`} or
aria-label={`Remove ${it} tag`}) and/or a visually-hidden text node to make its
purpose clear to screen readers while keeping the visual Badge unchanged; ensure
this change is applied to the button element that invokes deleteTag({it}) so
screen readers announce the removal action.
- Line 4: The import for main_schema uses a fragile deep relative path ("import
{ main_schema } from '../../../../../../../../packages/db/src'"); replace this
with your monorepo path alias (e.g., "import { main_schema } from '@repo/db'")
or update tsconfig/webpack path mappings and use that alias throughout; update
the import in apps/web/src/app/dashboard/posts/edit/[slug]/client.tsx to
reference the alias so future file moves won't break the cross-package import
and ensure the alias points to the package's exported entry (packages/db/src or
its build output).
- Around line 52-64: In addTag, using tagInput.replaceAll(" ", "") removes
internal spaces (e.g., "my tag" → "mytag"); change this to use tagInput.trim()
to only strip leading/trailing whitespace and preserve internal spaces, or
alternatively enforce/no-allow internal spaces by validating tagInput.includes("
") and showing an error; update the logic around trimmed (from tagInput) and
keep the existing checks that use tags, setPost, setTagInput, and toast for
consistent behavior.
- Around line 74-76: The onValueChange handler is assigning a string (vl) to
post.status which expects a narrower union type; update the handler to assert vl
to the expected status union before setting state (e.g., use a type assertion
when calling setPost so status: vl as PostStatus) and/or use the functional
setForm pattern (setPost(prev => ({ ...prev, status: vl as PostStatus }))) to
ensure type safety; locate the onValueChange callback and change the assignment
to include the assertion referring to setPost, post, status, and onValueChange.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5579ba80-2a16-4b27-9fc7-760ecaefd165
📒 Files selected for processing (25)
apps/web/src/app/api/data/create_collection/route.tsapps/web/src/app/api/data/create_share_link/route.tsapps/web/src/app/api/data/get_all_collections/route.tsapps/web/src/app/api/data/public_data/route.tsapps/web/src/app/api/data/publish/file/route.tsapps/web/src/app/api/data/publish/route.tsapps/web/src/app/api/data/settings/route.tsapps/web/src/app/c/[slug]/page.tsxapps/web/src/app/dashboard/collections/client.tsxapps/web/src/app/dashboard/collections/create/client.tsxapps/web/src/app/dashboard/collections/create/page.tsxapps/web/src/app/dashboard/collections/page.tsxapps/web/src/app/dashboard/posts/create/client.tsxapps/web/src/app/dashboard/posts/edit/[slug]/client.tsxapps/web/src/app/dashboard/posts/manage/client.tsxapps/web/src/app/dashboard/settings/clientComponents.tsxapps/web/src/app/dashboard/user/account/client.tsxapps/web/src/app/dashboard/user/manage_all/client.tsxapps/web/src/app/login/client.tsxapps/web/src/app/search/client.tsxapps/web/src/app/search/page.tsxapps/web/src/app/user/[userid]/postDisplay.tsxapps/web/src/components/publicPostsAndVideos.tsxapps/web/src/index.csspackages/auth/src/index.ts
💤 Files with no reviewable changes (4)
- apps/web/src/app/dashboard/posts/create/client.tsx
- apps/web/src/components/publicPostsAndVideos.tsx
- apps/web/src/app/api/data/publish/file/route.ts
- packages/auth/src/index.ts
Comment on lines
+37
to
+45
| const existing = await db | ||
| .select() | ||
| .from(main_schema.collections) | ||
| .where(dorm.eq(main_schema.collections.slug, slug)); | ||
|
|
||
| if (existing.length > 0) { | ||
| statusCode = 409; | ||
| throw new Error("A collection with this slug already exists"); | ||
| } |
Contributor
There was a problem hiding this comment.
The slug pre-check is still race-prone.
packages/db/src/schema/main.ts:69-80 already enforces collections.slug as UNIQUE, so two concurrent requests can both pass the select at Lines 37-40 and the loser then turns into a generic 500 from the catch block. Please make the insert the source of truth and translate duplicate-key failures back to 409.
Also applies to: 49-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/data/create_collection/route.ts` around lines 37 - 45,
The current pre-check using
db.select(...).where(dorm.eq(main_schema.collections.slug, slug)) is race-prone;
instead let the insert be authoritative and catch unique-constraint violations
from the insert into main_schema.collections, translating that specific
duplicate-key error into a 409 with the message "A collection with this slug
already exists". Remove or treat the select pre-check as advisory, perform the
insert directly, and update the existing catch/error-handling around the insert
to detect the DB unique-constraint error for main_schema.collections.slug (and
return statusCode = 409) while allowing other errors to bubble as before; apply
the same change to the second similar block referenced (lines 49-55).
Comment on lines
+53
to
+62
| const { targetUrl, customSlug } = body; | ||
|
|
||
| if (!targetUrl || typeof targetUrl !== "string") { | ||
| return Response.json( | ||
| { success: false, msg: "targetUrl is required" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| const urlSlug = customSlug || generateSlug(); |
Contributor
There was a problem hiding this comment.
Missing validation for targetUrl format and customSlug sanitization.
targetUrlis only checked for existence but not validated as a proper URL. Invalid URLs could be stored.customSlugis used directly without sanitization - special characters, spaces, or excessively long values could cause issues.
🛡️ Proposed validation improvements
const { targetUrl, customSlug } = body;
if (!targetUrl || typeof targetUrl !== "string") {
return Response.json(
{ success: false, msg: "targetUrl is required" },
{ status: 400 },
);
}
+ // Validate URL format
+ try {
+ new URL(targetUrl);
+ } catch {
+ return Response.json(
+ { success: false, msg: "Invalid URL format" },
+ { status: 400 },
+ );
+ }
+
+ // Validate customSlug if provided
+ if (customSlug) {
+ if (typeof customSlug !== "string" || !/^[a-zA-Z0-9_-]{1,50}$/.test(customSlug)) {
+ return Response.json(
+ { success: false, msg: "Invalid slug format. Use only letters, numbers, hyphens, and underscores (max 50 chars)" },
+ { status: 400 },
+ );
+ }
+ }
+
const urlSlug = customSlug || generateSlug();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/data/create_share_link/route.ts` around lines 53 - 62,
Validate that targetUrl is a well-formed URL and sanitize/validate customSlug
before using it: parse/validate targetUrl (e.g., via the URL constructor or a
strict regex) and return a 400 response if invalid; for customSlug, trim it,
enforce a max length (e.g., 64 chars), allow only safe characters (a-z0-9 and
hyphens/underscores) and reject or normalize anything else (or fall back to
generateSlug()), and ensure generateSlug() is still used when customSlug is
absent or invalid; apply these checks where targetUrl and customSlug are read in
the create_share_link route handler.
Comment on lines
+64
to
+82
| // Check if slug already exists | ||
| const existing = await db | ||
| .select() | ||
| .from(main_schema.urlShorter) | ||
| .where(dorm.eq(main_schema.urlShorter.urlSlug, urlSlug)) | ||
| .limit(1); | ||
|
|
||
| if (existing.length > 0) { | ||
| return Response.json( | ||
| { success: false, msg: "Slug already taken" }, | ||
| { status: 409 }, | ||
| ); | ||
| } | ||
|
|
||
| await db.insert(main_schema.urlShorter).values({ | ||
| urlSlug, | ||
| targetUrl, | ||
| byUser: userId, | ||
| }); |
Contributor
There was a problem hiding this comment.
Race condition: check-then-insert pattern may cause confusing errors.
Two concurrent requests with the same customSlug can both pass the existence check before either inserts. The DB's UNIQUE constraint (per schema) will reject the second insert, but the error will bubble up as a 500 with a database error message rather than a clean 409 conflict response.
Consider catching the unique constraint violation and returning a proper 409:
🛡️ Proposed fix to handle constraint violation
- await db.insert(main_schema.urlShorter).values({
- urlSlug,
- targetUrl,
- byUser: userId,
- });
+ try {
+ await db.insert(main_schema.urlShorter).values({
+ urlSlug,
+ targetUrl,
+ byUser: userId,
+ });
+ } catch (insertError: any) {
+ // Handle unique constraint violation (concurrent insert race)
+ if (insertError.code === "23505") {
+ return Response.json(
+ { success: false, msg: "Slug already taken" },
+ { status: 409 },
+ );
+ }
+ throw insertError;
+ }📝 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
| // Check if slug already exists | |
| const existing = await db | |
| .select() | |
| .from(main_schema.urlShorter) | |
| .where(dorm.eq(main_schema.urlShorter.urlSlug, urlSlug)) | |
| .limit(1); | |
| if (existing.length > 0) { | |
| return Response.json( | |
| { success: false, msg: "Slug already taken" }, | |
| { status: 409 }, | |
| ); | |
| } | |
| await db.insert(main_schema.urlShorter).values({ | |
| urlSlug, | |
| targetUrl, | |
| byUser: userId, | |
| }); | |
| // Check if slug already exists | |
| const existing = await db | |
| .select() | |
| .from(main_schema.urlShorter) | |
| .where(dorm.eq(main_schema.urlShorter.urlSlug, urlSlug)) | |
| .limit(1); | |
| if (existing.length > 0) { | |
| return Response.json( | |
| { success: false, msg: "Slug already taken" }, | |
| { status: 409 }, | |
| ); | |
| } | |
| try { | |
| await db.insert(main_schema.urlShorter).values({ | |
| urlSlug, | |
| targetUrl, | |
| byUser: userId, | |
| }); | |
| } catch (insertError: any) { | |
| // Handle unique constraint violation (concurrent insert race) | |
| if (insertError.code === "23505") { | |
| return Response.json( | |
| { success: false, msg: "Slug already taken" }, | |
| { status: 409 }, | |
| ); | |
| } | |
| throw insertError; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/data/create_share_link/route.ts` around lines 64 - 82,
The check-then-insert for main_schema.urlShorter with urlSlug can race; wrap the
db.insert(main_schema.urlShorter).values(...) in a try/catch and handle
unique-constraint failures by returning the same 409 Response.json({ success:
false, msg: "Slug already taken" }, { status: 409 }); detect the constraint
violation by inspecting the DB error (e.g., Postgres code '23505' or the
error.constraint/name/message) so legitimate unique-violation errors map to 409
while other errors rethrow or return a 500.
Comment on lines
+90
to
+96
| } catch (e: any) { | ||
| console.error(e); | ||
| return Response.json( | ||
| { success: false, msg: e.message }, | ||
| { status: 500 }, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid exposing internal error messages to clients.
Returning e.message directly (line 93) could leak database errors, stack traces, or internal implementation details. Return a generic message to the client while keeping the detailed logging server-side.
🛡️ Proposed fix
} catch (e: any) {
console.error(e);
return Response.json(
- { success: false, msg: e.message },
+ { success: false, msg: "An unexpected error occurred" },
{ status: 500 },
);
}📝 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
| } catch (e: any) { | |
| console.error(e); | |
| return Response.json( | |
| { success: false, msg: e.message }, | |
| { status: 500 }, | |
| ); | |
| } | |
| } catch (e: any) { | |
| console.error(e); | |
| return Response.json( | |
| { success: false, msg: "An unexpected error occurred" }, | |
| { status: 500 }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/data/create_share_link/route.ts` around lines 90 - 96,
The catch block in create_share_link route returns e.message to the client which
can leak internals; instead, log the full error server-side (using console.error
or process logger) and return a generic Response.json payload like { success:
false, msg: "Internal server error" } with status 500; update the catch in
route.ts (the catch surrounding the createShareLink handler / Response.json
call) to remove e.message from the response while preserving detailed logging.
Comment on lines
+25
to
+27
| if (error) { | ||
| toast.error(error.message); | ||
| } |
Contributor
There was a problem hiding this comment.
Toast called during render will fire repeatedly.
Calling toast.error() directly in the component body means it will execute on every re-render while error is truthy, potentially showing duplicate toasts. Move this to a useEffect to fire only once when the error state changes.
🐛 Proposed fix using useEffect
+"use client";
+import { useEffect } from "react";
import { useQuery } from "@tanstack/react-query";
import { toast } from "sonner";
import Link from "next/link";
import type { Route } from "next";
import { Button } from "@/components/ui/button";
export default function CollectionsClient() {
const { data, isLoading, error } = useQuery({
queryKey: ["collections"],
queryFn: async () => {
const req = await fetch("/api/data/get_all_collections");
const res = await req.json();
if (!req.ok) {
throw new Error(res.message || "Failed to fetch collections");
}
return res.data as {
collectionId: string;
slug: string;
title: string;
}[];
},
});
- if (error) {
- toast.error(error.message);
- }
+ useEffect(() => {
+ if (error) {
+ toast.error(error.message);
+ }
+ }, [error]);
return (📝 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 (error) { | |
| toast.error(error.message); | |
| } | |
| "use client"; | |
| import { useEffect } from "react"; | |
| import { useQuery } from "@tanstack/react-query"; | |
| import { toast } from "sonner"; | |
| import Link from "next/link"; | |
| import type { Route } from "next"; | |
| import { Button } from "@/components/ui/button"; | |
| export default function CollectionsClient() { | |
| const { data, isLoading, error } = useQuery({ | |
| queryKey: ["collections"], | |
| queryFn: async () => { | |
| const req = await fetch("/api/data/get_all_collections"); | |
| const res = await req.json(); | |
| if (!req.ok) { | |
| throw new Error(res.message || "Failed to fetch collections"); | |
| } | |
| return res.data as { | |
| collectionId: string; | |
| slug: string; | |
| title: string; | |
| }[]; | |
| }, | |
| }); | |
| useEffect(() => { | |
| if (error) { | |
| toast.error(error.message); | |
| } | |
| }, [error]); | |
| return ( | |
| // Rest of the JSX... | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/collections/client.tsx` around lines 25 - 27, The
component is calling toast.error(error.message) during render which will trigger
on every re-render while error is truthy; move this logic into a useEffect so
the toast fires only once when error changes: add import of useEffect (if
missing), create a useEffect hook that depends on [error] and inside it check if
(error) then call toast.error(error.message); keep the original error variable
and toast.error call but remove the direct call from the component body to
prevent duplicate toasts.
Comment on lines
+93
to
+98
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === "Space") { | ||
| e.preventDefault(); | ||
| addTag(); | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
Bug: e.key === "Space" will never match; the spacebar key is " ".
The keydown event for spacebar sets e.key to a literal space character " ", not the string "Space". This means users cannot add tags by pressing Space as intended.
🐛 Proposed fix
onKeyDown={(e) => {
- if (e.key === "Enter" || e.key === "Space") {
+ if (e.key === "Enter" || e.key === " ") {
e.preventDefault();
addTag();
}
}}📝 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
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === "Space") { | |
| e.preventDefault(); | |
| addTag(); | |
| } | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| addTag(); | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/posts/edit/`[slug]/client.tsx around lines 93 -
98, The onKeyDown handler in the tag input currently checks for e.key ===
"Space", which never matches — change the condition in the onKeyDown callback
(where addTag() is invoked) to detect space correctly by using e.key === " " (a
literal space) or e.code === "Space" (to be robust across layouts), e.g. check
(e.key === "Enter" || e.key === " " || e.code === "Space") before calling
addTag() and e.preventDefault().
Comment on lines
+111
to
+126
| const handleSaveProfile = async () => { | ||
| setSaving(true); | ||
| try { | ||
| const { error } = await authClient.updateUser({ | ||
| name, | ||
| }); | ||
| if (error) { | ||
| throw new Error(error.message || "Failed to update profile"); | ||
| } | ||
| toast.success("Profile updated successfully!"); | ||
| } catch (e: any) { | ||
| toast.error(e.message || "Failed to update profile"); | ||
| } finally { | ||
| setSaving(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the codebase structure and locate relevant files
git ls-files | grep -E "(better-auth|auth)" | head -20Repository: hpware/log
Length of output: 298
🏁 Script executed:
# Let's examine the specific file mentioned in the review
cat -n apps/web/src/app/dashboard/user/account/client.tsx | sed -n '111,126p'Repository: hpware/log
Length of output: 601
🏁 Script executed:
# Check the user-menu.tsx file referenced in the review
cat -n apps/web/src/components/user-menu.tsx | sed -n '15,25p'Repository: hpware/log
Length of output: 382
🏁 Script executed:
# Search for how better-auth is imported and configured
rg "better-auth" --type ts --type tsx | head -30Repository: hpware/log
Length of output: 81
🏁 Script executed:
# Search for other updateUser calls to see patterns
rg "updateUser" --type ts --type tsx -A 3 -B 1Repository: hpware/log
Length of output: 81
🏁 Script executed:
# Check the auth-client setup
cat -n apps/web/src/lib/auth-client.tsRepository: hpware/log
Length of output: 285
🏁 Script executed:
# Search for updateUser and router patterns without type restrictions
rg "updateUser" -A 5 -B 2Repository: hpware/log
Length of output: 667
🏁 Script executed:
# Search for router.refresh patterns
rg "router.refresh" -B 2 -A 2Repository: hpware/log
Length of output: 36
🏁 Script executed:
# Check if better-auth package config exists
cat packages/auth/src/index.tsRepository: hpware/log
Length of output: 2491
🏁 Script executed:
# Look for session/invalidation patterns
rg "session" -i | grep -i "refetch\|invalidat\|refresh" | head -20Repository: hpware/log
Length of output: 36
🏁 Script executed:
# Check if there's any server action or mutation handling for profile updates
rg "useTransition\|useAction\|server" apps/web/src/app/dashboard/user/account/Repository: hpware/log
Length of output: 36
🏁 Script executed:
# See the full client.tsx file to understand the component structure
cat apps/web/src/app/dashboard/user/account/client.tsxRepository: hpware/log
Length of output: 8603
🏁 Script executed:
# Check how useSession is actually used in other places
rg "useSession" -B 2 -A 5 | head -50Repository: hpware/log
Length of output: 2000
🏁 Script executed:
# Search for better-auth documentation or examples in package.json
cat packages/auth/package.jsonRepository: hpware/log
Length of output: 594
🏁 Script executed:
# Look for any revalidateTag or revalidatePath patterns which are Next.js specific
rg "revalidateTag|revalidatePath" -B 2 -A 2Repository: hpware/log
Length of output: 36
🏁 Script executed:
# Check how the session is initially loaded in the page (server-side)
fd "page.tsx" | xargs grep -l "account" | head -5Repository: hpware/log
Length of output: 36
🏁 Script executed:
# Look for the layout or page structure around the account page
cat -n apps/web/src/app/dashboard/user/account/page.tsx 2>/dev/null || cat -n apps/web/src/app/dashboard/user/page.tsx 2>/dev/null || echo "File not found, searching..."Repository: hpware/log
Length of output: 579
🏁 Script executed:
# Search for all files in the account directory
fd "." apps/web/src/app/dashboard/user/account/Repository: hpware/log
Length of output: 153
🌐 Web query:
better-auth updateUser session refresh invalidate useSession reactive
💡 Result:
- Update user fields (name/image/etc.): call
authClient.updateUser({ ... }). This updates the user record. [1] - Update session fields (only custom “additional fields”, not core fields like
token,userId,expiresAt, etc.): callauthClient.updateSession({ ... }). [2]
“Refresh” session data (so UI picks up changes)
useSession()is intended to be reactive. [2]- If you’re using cookie cache, session reads may stay stale until cache expiry; to force a fresh read (and refresh the cookie cache), fetch with
disableCookieCache: true:authClient.getSession({ query: { disableCookieCache: true } })(or the server equivalent). [3]
- A commonly recommended pattern after changing session/user state is: update on the server (e.g.
/update-user) then trigger a client refresh viauseSession().refetch(). [4]
Invalidate / revoke sessions (log out a device or all devices)
- Revoke current/any specific session by token:
authClient.revokeSession({ token }). [2] - Revoke all other sessions:
authClient.revokeOtherSessions(). [2] - Revoke all sessions:
authClient.revokeSessions(). [2]
If useSession() is “not reactive”
- This has been reported (e.g., stuck
isPending, not updating until a rerender). [5] - In those cases, using an explicit refresh (above) is the practical workaround:
disableCookieCache: trueongetSessionand/oruseSession().refetch()(where available). [3][4]
Sources: Better Auth docs on Users/Accounts and Session Management, plus community Q&A threads. [1][2][3][4][5]
Call useSession().refetch() or router.refresh() after successful updateUser() to refresh session-dependent UI.
The updateUser() call only updates the user record and does not automatically invalidate or refresh the session state. Components like user-menu.tsx that read from authClient.useSession() will display stale data (outdated name) until the session is explicitly refetched or the page reloads. better-auth's useSession() hook is intended to be reactive, but with cookie caching enabled, session reads can remain stale. The recommended pattern is to call useSession().refetch() on the client or router.refresh() on success to ensure dependent UI updates immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/user/account/client.tsx` around lines 111 - 126,
handleSaveProfile currently calls authClient.updateUser but does not refresh the
session, leaving session-dependent UI stale; after a successful update (inside
handleSaveProfile after authClient.updateUser resolves and before
toast.success), call the session refetch provided by useSession().refetch() (or
alternatively call router.refresh()) to revalidate the client session so
components relying on authClient.useSession() (e.g., user-menu.tsx) show the
updated name immediately.
| </div> | ||
| <div className="flex flex-row space-x-1 mt-3"> | ||
| <Dialog> | ||
| <Dialog open={passwordDialogOpen} onOpenChange={setPasswordDialogOpen}> |
Contributor
There was a problem hiding this comment.
Clear password fields when the dialog is dismissed.
At Line 224, onOpenChange={setPasswordDialogOpen} only toggles visibility. If the user closes the modal with Escape or an outside click, currentPassword, newPassword, and confirmPassword stay in memory and reappear on reopen. That is a poor default for sensitive fields.
🔐 Proposed fix
- <Dialog open={passwordDialogOpen} onOpenChange={setPasswordDialogOpen}>
+ <Dialog
+ open={passwordDialogOpen}
+ onOpenChange={(open) => {
+ setPasswordDialogOpen(open);
+ if (!open) {
+ setCurrentPassword("");
+ setNewPassword("");
+ setConfirmPassword("");
+ }
+ }}
+ >📝 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
| <Dialog open={passwordDialogOpen} onOpenChange={setPasswordDialogOpen}> | |
| <Dialog | |
| open={passwordDialogOpen} | |
| onOpenChange={(open) => { | |
| setPasswordDialogOpen(open); | |
| if (!open) { | |
| setCurrentPassword(""); | |
| setNewPassword(""); | |
| setConfirmPassword(""); | |
| } | |
| }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/dashboard/user/account/client.tsx` at line 224, The password
dialog currently only toggles visibility via
onOpenChange={setPasswordDialogOpen}, so when it’s dismissed (Escape/outside
click) sensitive fields currentPassword, newPassword, and confirmPassword remain
populated; update the handler to a custom function (e.g.,
handlePasswordDialogChange) that calls setPasswordDialogOpen(open) and, when
open is false, also clears all three state variables (setCurrentPassword(''),
setNewPassword(''), setConfirmPassword('')) so the fields are reset whenever the
Dialog component (Dialog / passwordDialogOpen / setPasswordDialogOpen) is
closed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
bun run buildpasses cleanlytsc --noEmit)console.logstatements remaining🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes