feat: P2.3 collections, optimistic UI, security hardening, landing polish#26
Merged
Conversation
…lish Collections (P2.3 complete): - Full CRUD API: POST/DELETE /api/collections, add/remove bookmark endpoints - DashboardClient: collection pills, active filter banner, inline delete confirm - BookmarkCard: folder button dropdown, collection chips with inline remove Optimistic updates (instant UI, no router.refresh): - Tag add/remove: local state with revert on failure, duplicate guard - Bookmark delete: fires onDelete only on API success - Collection membership: add/remove with revert on failure - Collection delete: snapshot revert on failure - Tag filter pills now live-update via onTagsChange callback Security: - Thumbnail SSRF protection in scraper.ts - Gemini prompt injection fix (truncate + strip newlines) - Auth standardized to getUserFromRequest across all API routes - Prisma P2025 handled in collection bookmark DELETE - supabase-rls.sql added for RLS policies (apply in Supabase SQL editor) - Safety cap (take: 500) on bookmark GET Bug fixes: - Concurrent tag removal uses Set<string> instead of single removingId - loading state now disables tag input during inflight request - Missing try/catch on collections POST/DELETE routes - handleDeleteCollection reverts on API failure Landing page: - Removed "AI-powered bookmarking" badge - Bigger headings and subheadings (clamp values bumped up) - Em dashes replaced with regular hyphens throughout - Section headers: emojis removed, uppercase tracking-wide labels Other: - scroll-behavior smooth warning fixed (data-scroll-behavior on html) - admin page revalidate=300 to prevent rate limiting Supabase listUsers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Collection API Routes client/src/app/api/collections/route.ts, client/src/app/api/collections/[id]/route.ts |
New GET/POST/DELETE endpoints for user-scoped collections with auth, validation, ownership checks, idempotent delete handling, and error responses. |
Collection-Bookmark Membership Routes client/src/app/api/collections/[id]/bookmarks/route.ts, client/src/app/api/collections/[id]/bookmarks/[bookmarkId]/route.ts |
New POST (upsert membership) and DELETE handlers; authenticate via getUserFromRequest, verify collection ownership, treat missing records (P2025) as idempotent 204. |
Bookmarks & Tags APIs client/src/app/api/bookmarks/route.ts, client/src/app/api/bookmarks/[id]/tags/route.ts, client/src/app/api/bookmarks/[id]/tags/[tagId]/route.ts |
Switched auth retrieval to getUserFromRequest(req) across handlers; added take: 500 cap on GET bookmarks; minor error message punctuation tweak; handler param rename _req→req. |
Dashboard & Bookmark UI client/src/components/DashboardClient.tsx, client/src/components/BookmarkCard.tsx, client/src/app/app/page.tsx |
Large client-side state changes: DashboardClient accepts initialCollections, tracks localBookmarks, memberships, and tagOverrides; BookmarkCard API expanded with collection UI, optimistic tag/membership flows, local tag state; loader now fetches collections+bookmarks and serializes collectionIds. |
Admin caching client/src/app/admin/page.tsx |
Added unstable_cache-wrapped getCachedAuthUsers to cache Supabase admin user listing (revalidate: 300) and replaced inline per-request listUsers call. |
URL safety & scraper client/src/lib/url-validation.ts, client/src/lib/scraper.ts |
Added isPrivateIp and isSafeUrl; scraper validates scraped thumbnail URLs and returns null for unsafe/invalid thumbnails. |
Gemini tagging client/src/lib/gemini.ts |
generateTags now strips newlines and truncates combined title+description to 500 chars before prompting the model. |
Supabase RLS SQL client/supabase-rls.sql |
New SQL file adding RLS policies for Bookmark, Tag, Collection, and CollectionBookmark to enforce per-user ownership and scoped access via related rows. |
URL validation & config client/next.config.ts, client/src/app/globals.css, client/src/app/layout.tsx, client/src/app/page.tsx, client/src/app/auth/page.tsx |
Removed HTTP remote image pattern (HTTPS-only); .font-mono-lp uses generic monospace; root <html> adds data-scroll-behavior="smooth"; landing/auth copy/typography and punctuation adjustments. |
Minor utils & types client/src/lib/* |
Other small updates across libs (scraper, gemini) and types used by UI components to include collectionIds and new collection types. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Client as DashboardClient / BookmarkCard
participant API as Next.js API routes
participant Auth as getUserFromRequest
participant DB as Prisma
participant RLS as Supabase RLS
Client->>API: POST /api/collections (create) with user session
API->>Auth: extract user from request
Auth-->>API: user
API->>DB: prisma.collection.create({ userId })
DB-->>API: created collection
API-->>Client: 201 Created
Client->>API: POST /api/collections/:id/bookmarks (add membership)
API->>Auth: extract user from request
Auth-->>API: user
API->>DB: verify collection.userId == user.id
DB-->>API: collection
API->>DB: upsert collectionBookmark (collectionId, bookmarkId)
DB-->>API: upsert result
API-->>Client: 204 No Content
Client->>API: DELETE /api/collections/:id/bookmarks/:bookmarkId (remove)
API->>Auth: extract user from request
Auth-->>API: user
API->>DB: delete collectionBookmark (handle P2025 -> 204)
DB-->>API: deleted / error
API-->>Client: 204 No Content
note over DB,RLS: RLS policies ensure DB operations are scoped to authenticated user ownership
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
- feat: P2.2 admin dashboard #24: Replaces admin user list call with cached
getCachedAuthUsers— directly related to the admin caching change. - feat: P2.1 user dashboard — filter, sort, grid/list view #23: Earlier DashboardClient / BookmarkCard refactor — overlaps with the expanded client-side state and prop/API changes here.
- feat: AI tagging, extension polish, nav/auth UX + security hardening #22: Changes to
client/src/lib/gemini.tsand tagging flow — directly related to the updated tagging prompt/text handling.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 22.58% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main changes: collections implementation, optimistic UI updates, security hardening, and landing page polish. |
| Description check | ✅ Passed | The description covers all required template sections with substantial detail. It includes a comprehensive summary of changes organized by feature area, a clear action-required callout, and addresses all major PR objectives. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
p2-collections
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 help to get the list of available commands and usage tips.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
client/src/app/api/collections/route.ts (2)
6-16: Consider usingNextResponse.json()for consistency with other API routes.This route uses
Response.json()while other routes in this PR (e.g.,bookmarks/route.ts) useNextResponse.json(). Both work in Next.js App Router, but usingNextResponseconsistently enables easier adoption of Next.js-specific features like setting cookies or headers.♻️ Suggested change for consistency
import { prisma } from "@/lib/prisma"; import { getUserFromRequest } from "@/lib/supabase/get-user"; -import { NextRequest } from "next/server"; +import { NextRequest, NextResponse } from "next/server"; // GET /api/collections — list all collections for the user export async function GET(req: NextRequest) { const user = await getUserFromRequest(req); - if (!user) return Response.json({ error: "unauthorized" }, { status: 401 }); + if (!user) return NextResponse.json({ error: "unauthorized" }, { status: 401 }); const collections = await prisma.collection.findMany({ where: { userId: user.id }, orderBy: { createdAt: "asc" }, }); - return Response.json(collections); + return NextResponse.json(collections); }Apply similar changes to POST handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/collections/route.ts` around lines 6 - 16, Replace usages of the global Response.json with NextResponse.json for consistency and to enable Next.js App Router features: update the exported GET (and the POST handler if present) in route.ts to return NextResponse.json(...) instead of Response.json(...), ensure you import NextResponse from "next/server", and keep the same payload and options (e.g., { status: 401 }) when converting the return values.
18-43: Consider adding rate limiting and name length validation.Unlike the bookmark and tag routes, collection creation has no rate limiting. A malicious user could create many collections rapidly.
Additionally, there's no maximum length validation on
name, allowing arbitrarily long strings to be stored.🛡️ Suggested improvements
import { prisma } from "@/lib/prisma"; import { getUserFromRequest } from "@/lib/supabase/get-user"; import { NextRequest, NextResponse } from "next/server"; +import { collectionRatelimit } from "@/lib/ratelimit"; // add this limiter // POST /api/collections — create a new collection export async function POST(req: NextRequest) { const user = await getUserFromRequest(req); if (!user) return NextResponse.json({ error: "unauthorized" }, { status: 401 }); + const { success } = await collectionRatelimit.limit(user.id); + if (!success) + return NextResponse.json( + { error: "too many requests - slow down" }, + { status: 429 }, + ); // ... existing json parsing ... const name = typeof body.name === "string" ? body.name.trim() : ""; - if (!name) return NextResponse.json({ error: "name is required" }, { status: 400 }); + if (!name) return NextResponse.json({ error: "name is required" }, { status: 400 }); + if (name.length > 100) return NextResponse.json({ error: "name too long" }, { status: 400 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/collections/route.ts` around lines 18 - 43, Add rate limiting and a maximum name length check to the POST handler: in the POST function, call the same per-user rate limiter used by the bookmark/tag routes (e.g., rateLimit or limiter middleware used elsewhere) right after getUserFromRequest to early-return 429 when the user is over quota, and validate the name variable before creating the record by enforcing a max length (e.g., MAX_NAME_LENGTH = 100) after trimming (reject with Response.json({ error: "name too long" }, { status: 400 }) if exceeded) and keep the existing empty-name check; ensure the prisma.collection.create call still uses the validated name and user.id.client/src/app/app/page.tsx (1)
13-24: Consider adding a pagination limit for consistency with the API route.The API route at
/api/bookmarkshas atake: 500safety cap, but this server-side query fetches all bookmarks without limit. For users with a large number of bookmarks, this could cause performance degradation.If this is intentional (server-side can handle more), consider documenting the rationale. Otherwise, apply a consistent limit.
🔧 Suggested change to add safety cap
prisma.bookmark.findMany({ where: { userId }, orderBy: { createdAt: "desc" }, + take: 500, // safety cap — matches API route include: { tags: true, collections: { select: { collectionId: true } } }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/app/page.tsx` around lines 13 - 24, The server-side query for bookmarks (prisma.bookmark.findMany used to populate the bookmarks variable) fetches all rows while the API route enforces take: 500; add a consistent safety cap (e.g., include take: 500 or a configurable MAX_BOOKMARKS constant) to the prisma.bookmark.findMany call and keep the same ordering and includes; if the higher/no-limit is intentional, add a brief inline comment documenting the rationale instead of leaving it unbounded.client/src/app/page.tsx (1)
75-90: Move these landing-page style tweaks into Tailwind utilities.These changes keep expanding the inline-style surface in a
client/**/*file instead of using Tailwind classes, which makes this page harder to keep consistent with the rest of the app.As per coding guidelines,
client/**/*.{tsx,jsx,ts,js,css}: Use Tailwind CSS v4 for styling.Also applies to: 281-285, 317-319, 364-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/page.tsx` around lines 75 - 90, The inline style block on the landing headline (the <h1> with three <span> lines) and the paragraph below (the <p> using "fadeUp" animation) should be replaced with Tailwind v4 utility classes: remove the style props on the <h1>, its child <span>s, and the <p>, and map fontSize/clamp, fontWeight, lineHeight, margins, colors, display, and the text-shadow/animation into Tailwind classes or custom utilities; add any missing utilities (e.g., a custom clamp font-size, text-shadow, and the fadeUp keyframes/animation) in tailwind.config.js or your globals CSS and then use those class names in the JSX (refer to the <h1> block, the three inner <span> elements, and the animated <p> to locate all occurrences). Ensure the same adjustments are applied to the other inline-style occurrences noted (around lines 281-285, 317-319, 364-392).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/admin/page.tsx`:
- Around line 9-10: The file sets export const revalidate = 300 but
createClient() reads cookies() (forcing the route to be dynamic), so the
revalidate is ignored; remove the revalidate export and instead wrap the
expensive data fetch with Next.js caching (use unstable_cache) so the auth flow
remains dynamic while supabaseAdmin.auth.admin.listUsers() and related queries
are cached; locate the revalidate export to delete it, find
createClient()/cookies() usage to keep dynamic behavior, and create an
unstable_cache-wrapped helper that calls supabaseAdmin.auth.admin.listUsers()
(and any related data operations) which the page code should call.
In `@client/src/app/api/collections/`[id]/bookmarks/[bookmarkId]/route.ts:
- Around line 27-30: The current catch block treats Prisma's P2025 as a 404
which causes the frontend optimistic rollback to restore a deleted membership;
instead map P2025 to a success (e.g., 204 No Content or 200 OK) so
repeated/de-duped DELETEs are idempotent. In the catch (e: unknown) handling
where you check (e as { code?: string })?.code === "P2025", change the response
to a successful status (204 or 200) rather than Response.json({ error: "not
found" }, { status: 404 }); so DashboardClient.tsx’s optimistic snapshot logic
won't undo a deletion on a retried request.
In `@client/src/app/api/collections/`[id]/route.ts:
- Around line 20-24: The delete handler's catch block treats a missing row as a
server error; update the error handling around prisma.collection.delete so that
if the deletion throws a Prisma known-request error for "Record to delete does
not exist" (PrismaClientKnownRequestError with code 'P2025') you return a
successful response (e.g., Response.json({}, { status: 200 }) or 204) instead of
500, and only return the 500 error for other exceptions; locate the
prisma.collection.delete call and replace the bare catch with logic that
inspects the caught error (error.code === 'P2025' or instanceof
Prisma.PrismaClientKnownRequestError) to treat already-missing collections as
success.
In `@client/src/components/BookmarkCard.tsx`:
- Around line 251-257: The delete button currently only becomes visible on hover
(uses group-hover:opacity-100) which hides it from keyboard users; update the
button element(s) that call deleteBookmark (and the other similar button around
the 313-319 snippet) to also include focus-visible:opacity-100 and add an
accessible focus ring class (e.g., focus-visible:outline-none
focus-visible:ring-2 focus-visible:ring-offset-1 focus-visible:ring-red-400 or
your design-system equivalents) in their className so the control becomes
visible and keyboard-focusable; ensure you apply these classes to the same JSX
button elements that have onClick={deleteBookmark} and the matching counterpart
so tabbing shows a visible focus state.
- Around line 97-119: The failure path currently calls setLocalTags(snapshot)
which restores the entire pre-delete list and can re-add tags deleted
successfully by other in-flight operations; change it to reinsert only the
failed tag: capture the removed tag from snapshot (const failedTag =
snapshot.find(t => t.id === tagId)) and on error use setLocalTags(prev =>
prev.some(t => t.id === tagId) ? prev : (failedTag ? [...prev, failedTag] :
prev)); keep the existing setRemovingIds cleanup in finally and reference
functions/variables removeTag, setLocalTags, localTags, removingIds, and
snapshot when making the change.
In `@client/src/components/DashboardClient.tsx`:
- Around line 120-130: Wrap the fetch calls in DashboardClient.tsx in try/catch
blocks so thrown network or runtime errors are caught; in the catch
setCollectionError with an appropriate message and revert any optimistic state
changes (e.g., undo changes made via setCollections, membership updates
functions used around the create/delete/membership mutation sites). Specifically
update the create-collection block that calls fetch and uses setCollections and
setCollectionError, and mirror this pattern for the other mutation sites (the
blocks around lines referenced for delete and membership changes) so both res.ok
=== false and thrown exceptions consistently update collectionError and restore
optimistic state.
- Around line 211-228: The early return in the DashboardClient when
localBookmarks.length === 0 prevents the collections list and create UI from
rendering; instead of returning the entire component, remove this top-level
return and render the empty-bookmarks panel only in the bookmarks area (e.g.,
conditionally render the "Nothing saved yet" block where bookmarks are shown),
leaving the collections list and collection-creation UI (e.g., renderCollections
/ CreateCollectionButton or equivalent) outside that conditional so they remain
visible even when localBookmarks is empty; update DashboardClient to use
conditional JSX for the bookmarks region rather than an early return.
In `@client/src/lib/scraper.ts`:
- Around line 13-24: isSafeThumbnailUrl uses a narrow regex and misses IPv6,
0.0.0.0, 169.254.x.x, and non-dotted IP forms; replace its logic by reusing the
more comprehensive isPrivateIp validation used in route.ts. Import or extract
the existing isPrivateIp utility and call it from isSafeThumbnailUrl (preserve
the HTTPS check and URL parsing), returning false when
isPrivateIp(parsed.hostname) is true or the URL parse fails; update tests
accordingly to cover IPv4, IPv6, link-local, metadata IPs and unusual encodings.
---
Nitpick comments:
In `@client/src/app/api/collections/route.ts`:
- Around line 6-16: Replace usages of the global Response.json with
NextResponse.json for consistency and to enable Next.js App Router features:
update the exported GET (and the POST handler if present) in route.ts to return
NextResponse.json(...) instead of Response.json(...), ensure you import
NextResponse from "next/server", and keep the same payload and options (e.g., {
status: 401 }) when converting the return values.
- Around line 18-43: Add rate limiting and a maximum name length check to the
POST handler: in the POST function, call the same per-user rate limiter used by
the bookmark/tag routes (e.g., rateLimit or limiter middleware used elsewhere)
right after getUserFromRequest to early-return 429 when the user is over quota,
and validate the name variable before creating the record by enforcing a max
length (e.g., MAX_NAME_LENGTH = 100) after trimming (reject with Response.json({
error: "name too long" }, { status: 400 }) if exceeded) and keep the existing
empty-name check; ensure the prisma.collection.create call still uses the
validated name and user.id.
In `@client/src/app/app/page.tsx`:
- Around line 13-24: The server-side query for bookmarks
(prisma.bookmark.findMany used to populate the bookmarks variable) fetches all
rows while the API route enforces take: 500; add a consistent safety cap (e.g.,
include take: 500 or a configurable MAX_BOOKMARKS constant) to the
prisma.bookmark.findMany call and keep the same ordering and includes; if the
higher/no-limit is intentional, add a brief inline comment documenting the
rationale instead of leaving it unbounded.
In `@client/src/app/page.tsx`:
- Around line 75-90: The inline style block on the landing headline (the <h1>
with three <span> lines) and the paragraph below (the <p> using "fadeUp"
animation) should be replaced with Tailwind v4 utility classes: remove the style
props on the <h1>, its child <span>s, and the <p>, and map fontSize/clamp,
fontWeight, lineHeight, margins, colors, display, and the text-shadow/animation
into Tailwind classes or custom utilities; add any missing utilities (e.g., a
custom clamp font-size, text-shadow, and the fadeUp keyframes/animation) in
tailwind.config.js or your globals CSS and then use those class names in the JSX
(refer to the <h1> block, the three inner <span> elements, and the animated <p>
to locate all occurrences). Ensure the same adjustments are applied to the other
inline-style occurrences noted (around lines 281-285, 317-319, 364-392).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4865fe3-d335-4dfc-8e52-dcb998e54558
📒 Files selected for processing (17)
client/src/app/admin/page.tsxclient/src/app/api/bookmarks/[id]/tags/[tagId]/route.tsclient/src/app/api/bookmarks/[id]/tags/route.tsclient/src/app/api/bookmarks/route.tsclient/src/app/api/collections/[id]/bookmarks/[bookmarkId]/route.tsclient/src/app/api/collections/[id]/bookmarks/route.tsclient/src/app/api/collections/[id]/route.tsclient/src/app/api/collections/route.tsclient/src/app/app/page.tsxclient/src/app/auth/page.tsxclient/src/app/layout.tsxclient/src/app/page.tsxclient/src/components/BookmarkCard.tsxclient/src/components/DashboardClient.tsxclient/src/lib/gemini.tsclient/src/lib/scraper.tsclient/supabase-rls.sql
- admin: replace ineffective revalidate=300 with unstable_cache on listUsers - collections DELETE: return 204 (not 404/500) for P2025 — idempotent deletes - BookmarkCard: removeTag restores only the failed tag, not full snapshot - BookmarkCard: delete buttons visible on focus-visible for keyboard users - DashboardClient: wrap all collection fetch calls in try/catch - DashboardClient: remove early return so collections stay visible after last bookmark deleted - scraper: extract isPrivateIp to shared url-validation utility, covers IPv6/169.254/0.0.0.0 gaps - bookmarks/route: import isPrivateIp from shared utility - collections POST: enforce max name length of 100 chars - page.tsx: add take:500 cap to server-side bookmark query Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/api/bookmarks/route.ts (1)
45-49:⚠️ Potential issue | 🔴 CriticalNote: IPv4-mapped IPv6 addresses also bypass this check.
The
blockedHostsSet provides good defense-in-depth, but::ffff:127.0.0.1bypasses bothblockedHosts.has()(not in set) andisPrivateIp()(the bug flagged in url-validation.ts). OnceisPrivateIpis fixed, this combined check will be robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/api/bookmarks/route.ts` around lines 45 - 49, The host validation in route.ts currently misses IPv4-mapped IPv6 addresses (e.g., ::ffff:127.0.0.1) because blockedHosts.has(host) and isPrivateIp(host) are checked against the raw hostname; update the check to first normalize/detect IPv4-mapped IPv6 addresses (strip the ::ffff: prefix or convert to the mapped IPv4 address) before applying blockedHosts.has() and isPrivateIp(); reference the variables/functions blockedHosts, host, parsedUrl, and isPrivateIp and ensure the normalizedHost is used for both the Set lookup and the private-IP check so mapped IPv4 addresses are correctly blocked.
🧹 Nitpick comments (2)
client/src/app/admin/page.tsx (1)
13-16: Avoid silent failure in cached user fetch.Line 15 swallows Supabase errors by returning
[]without any signal, which makes admin-data outages hard to detect. Add lightweight server logging before fallback.Suggested patch
const getCachedAuthUsers = unstable_cache( async () => { const { data, error } = await supabaseAdmin.auth.admin.listUsers({ perPage: 1000 }); - return error ? [] : data.users; + if (error) { + console.error("AdminPage listUsers failed", { message: error.message }); + return []; + } + return data.users; }, ["admin-auth-users"], { revalidate: 300 }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/admin/page.tsx` around lines 13 - 16, The cached user fetch currently swallows Supabase errors by returning [] in the expression "return error ? [] : data.users" after calling supabaseAdmin.auth.admin.listUsers; change this to log the error to the server logger (e.g., console.error or the app's logger) before returning the fallback so outages are visible. Locate the anonymous async function that calls supabaseAdmin.auth.admin.listUsers and add a lightweight server-side log statement that includes the error object (and a short context message) when error is truthy, then still return the empty array as the safe fallback.client/src/components/DashboardClient.tsx (1)
563-566: Replace these inline styles with Tailwind utilities/classes.
style={{ padding: "48px 32px" }}and the per-cardanimationDelaystyles bypass the repo's Tailwind-only styling rule. The padding can move directly into utilities, and the bounded delays can come from a small class lookup instead of inline styles. As per coding guidelines,client/**/*.{tsx,jsx,ts,js,css}: Use Tailwind CSS v4 for styling.Also applies to: 628-631, 652-655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/DashboardClient.tsx` around lines 563 - 566, The container div in DashboardClient.tsx uses an inline style for padding (style={{ padding: "48px 32px" }}) and several card elements use inline animationDelay styles; replace those inline styles with Tailwind utility classes: convert padding to py-12 px-8 on the container and for per-card delays create a small mapping from index/delay value to Tailwind delay classes (e.g., "delay-75","delay-100","delay-150", etc.) and apply via className instead of style; update the elements that set animationDelay (the per-card render logic referenced around lines 628–631 and 652–655) to use the mapped classnames so all styling uses Tailwind v4 utilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/DashboardClient.tsx`:
- Around line 633-638: The BookmarkCard is still receiving the original
bookmark.tags prop so its localTags (initialized in BookmarkCard from props) can
revert on remount; update both render branches where BookmarkCard is
instantiated (the lines passing bookmark with collectionIds override) to also
override tags using the tagOverrides map (e.g., pass tags:
tagOverrides.get(bookmark.id) ?? bookmark.tags) so BookmarkCard always gets the
current overridden tags; ensure the same change is applied at the other
occurrence noted (lines 658-662).
- Around line 289-303: The input for creating collections limits names to 50
chars while the API accepts up to 100, so update the UI to match the API: change
the input's maxLength from 50 to 100 and ensure any client-side validation
around newCollectionName and inside handleCreateCollection uses the same
100-character limit so users can create names up to the API's 100-char contract.
- Around line 137-150: handleDeleteCollection currently saves the entire
collections array and restores it on failure, which clobbers intervening user
changes; instead, capture the single deleted item and its original index (e.g.,
save deletedCollection = collections.find(c => c.id === id) and deletedIndex),
optimistically remove by filtering, then on failure call setCollections(prev =>
{ if (prev.length === snapshot.length - 1 && !prev.some(c => c.id === id))
return [...prev.slice(0, deletedIndex), deletedCollection,
...prev.slice(deletedIndex)]; return prev; }) so you only reinsert the
failed-deletion item when the collection list hasn't changed otherwise;
likewise, save the previous activeCollection value and only restore
activeCollection to that id if current activeCollection is still null (meaning
the user didn't change selection since the delete was initiated).
- Around line 196-219: The rollback currently restores the entire snapshot for
bookmarkId which can undo concurrent successful removals; change the revert
logic in removeFromCollection to only reinsert the failed collectionId into the
current memberships for that bookmark. Keep the existing snapshot variable for
context, but replace the revert implementation so it calls setMemberships(prev
=> { const next = new Map(prev); const list = new Set(next.get(bookmarkId) ??
[]); list.add(collectionId); next.set(bookmarkId, Array.from(list)); return
next; }) — i.e., merge the failed collectionId into the current list (guarding
against duplicates) instead of resetting to snapshot. Ensure this uses the same
removeFromCollection function, bookmarkId, and collectionId identifiers.
In `@client/src/lib/url-validation.ts`:
- Around line 15-23: The IPv6 branch (where isIP(host) === 6) currently misses
IPv4-mapped IPv6 addresses like "::ffff:127.0.0.1"; update the IPv6 handling in
url-validation.ts to detect when lower.startsWith("::ffff:") (or the "::FFFF:"
variant), extract the trailing IPv4 portion (the substring after "::ffff:"), and
then run the existing IPv4 private/loopback checks on that extracted IPv4 string
(i.e., reuse the same logic used for isIP(host) === 4) instead of only checking
"::1", "fe80:", "fc", "fd" patterns; keep the existing IPv6 checks for native
IPv6 addresses and ensure the mapped address path treats mapped IPv4 as IPv4 for
SSRF protection.
---
Outside diff comments:
In `@client/src/app/api/bookmarks/route.ts`:
- Around line 45-49: The host validation in route.ts currently misses
IPv4-mapped IPv6 addresses (e.g., ::ffff:127.0.0.1) because
blockedHosts.has(host) and isPrivateIp(host) are checked against the raw
hostname; update the check to first normalize/detect IPv4-mapped IPv6 addresses
(strip the ::ffff: prefix or convert to the mapped IPv4 address) before applying
blockedHosts.has() and isPrivateIp(); reference the variables/functions
blockedHosts, host, parsedUrl, and isPrivateIp and ensure the normalizedHost is
used for both the Set lookup and the private-IP check so mapped IPv4 addresses
are correctly blocked.
---
Nitpick comments:
In `@client/src/app/admin/page.tsx`:
- Around line 13-16: The cached user fetch currently swallows Supabase errors by
returning [] in the expression "return error ? [] : data.users" after calling
supabaseAdmin.auth.admin.listUsers; change this to log the error to the server
logger (e.g., console.error or the app's logger) before returning the fallback
so outages are visible. Locate the anonymous async function that calls
supabaseAdmin.auth.admin.listUsers and add a lightweight server-side log
statement that includes the error object (and a short context message) when
error is truthy, then still return the empty array as the safe fallback.
In `@client/src/components/DashboardClient.tsx`:
- Around line 563-566: The container div in DashboardClient.tsx uses an inline
style for padding (style={{ padding: "48px 32px" }}) and several card elements
use inline animationDelay styles; replace those inline styles with Tailwind
utility classes: convert padding to py-12 px-8 on the container and for per-card
delays create a small mapping from index/delay value to Tailwind delay classes
(e.g., "delay-75","delay-100","delay-150", etc.) and apply via className instead
of style; update the elements that set animationDelay (the per-card render logic
referenced around lines 628–631 and 652–655) to use the mapped classnames so all
styling uses Tailwind v4 utilities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a9d8a15-5f40-4f9b-bb43-6dcd874910b3
📒 Files selected for processing (10)
client/src/app/admin/page.tsxclient/src/app/api/bookmarks/route.tsclient/src/app/api/collections/[id]/bookmarks/[bookmarkId]/route.tsclient/src/app/api/collections/[id]/route.tsclient/src/app/api/collections/route.tsclient/src/app/app/page.tsxclient/src/components/BookmarkCard.tsxclient/src/components/DashboardClient.tsxclient/src/lib/scraper.tsclient/src/lib/url-validation.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- client/src/app/app/page.tsx
- client/src/lib/scraper.ts
- client/src/app/api/collections/[id]/route.ts
- client/src/app/api/collections/[id]/bookmarks/[bookmarkId]/route.ts
Mobile performance (FCP/LCP on /app):
- Remove unoptimized from bookmark images — Next.js now serves WebP/AVIF
- Add sizes prop to both image variants (list: 64px, grid: responsive)
- Add priority prop support, pass priority={i === 0} from DashboardClient
- Remove http:// wildcard from remotePatterns (security + optimization)
- Drop JetBrains Mono from global layout (only used on landing page mock)
- Use system monospace on landing URL bar, remove --font-mono dependency
CodeRabbit findings:
- url-validation: handle IPv4-mapped IPv6 (::ffff:127.0.0.1) SSRF bypass
- admin: log error before returning empty fallback from getCachedAuthUsers
- DashboardClient: collection delete reverts only deleted item, not full snapshot
- DashboardClient: removeFromCollection reverts only failed collectionId (concurrent-safe)
- DashboardClient: maxLength 50→100 to match API contract
- DashboardClient: pass tagOverrides to BookmarkCard to prevent stale tag props on remount
- collections/route: switch Response.json → NextResponse.json for consistency
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/src/components/DashboardClient.tsx (1)
137-157:⚠️ Potential issue | 🟠 MajorRestore the active collection filter when delete rollback runs.
If the user deletes the currently selected collection and the request fails, the pill is reinserted but
activeCollectionstays cleared. The banner/result set then reflect a successful unfilter even though the delete did not stick.Targeted rollback fix
async function handleDeleteCollection(id: string) { const deletedIndex = collections.findIndex((c) => c.id === id); const deletedItem = collections[deletedIndex]; if (!deletedItem) return; + const wasActive = activeCollection === id; setCollections((prev) => prev.filter((c) => c.id !== id)); - if (activeCollection === id) setActiveCollection(null); - const revert = () => setCollections((prev) => { - if (prev.some((c) => c.id === id)) return prev; - const next = [...prev]; - next.splice(Math.min(deletedIndex, next.length), 0, deletedItem); - return next; - }); + if (wasActive) setActiveCollection(null); + const revert = () => { + setCollections((prev) => { + if (prev.some((c) => c.id === id)) return prev; + const next = [...prev]; + next.splice(Math.min(deletedIndex, next.length), 0, deletedItem); + return next; + }); + if (wasActive) { + setActiveCollection((prev) => (prev === null ? id : prev)); + } + }; try { const res = await fetch(`/api/collections/${id}`, { method: "DELETE" }); if (!res.ok) { revert();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/DashboardClient.tsx` around lines 137 - 157, handleDeleteCollection does an optimistic removal but doesn't restore the activeCollection when the delete is rolled back; capture whether the deleted item was active (e.g. const wasActive = activeCollection === id) before calling setCollections and setActiveCollection(null), then update the revert closure to not only reinsert the deletedItem via setCollections but also, if wasActive is true, call setActiveCollection(id) to restore the filter when the rollback runs.
🧹 Nitpick comments (2)
client/src/components/DashboardClient.tsx (2)
115-133: Keep the draft name until create succeeds.This closes the inline form and clears
newCollectionNamebefore the POST resolves. On a network or 4xx/5xx failure the user gets the error, but has to reopen the form and retype the collection name.Small UX tweak
async function handleCreateCollection() { const name = newCollectionName.trim(); if (!name) return; - setCreatingCollection(false); - setNewCollectionName(""); setCollectionError(null); try { const res = await fetch("/api/collections", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ name }), }); if (res.ok) { const created: CollectionItem = await res.json(); setCollections((prev) => [...prev, created]); + setCreatingCollection(false); + setNewCollectionName(""); } else { setCollectionError("Failed to create collection"); } } catch { setCollectionError("Failed to create collection"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/DashboardClient.tsx` around lines 115 - 133, The code clears the draft (calls setCreatingCollection(false) and setNewCollectionName("")) before the POST completes, losing the user's input on failure; change the flow so you keep newCollectionName (and keep the form open) until the request succeeds: remove the early calls to setCreatingCollection(false) and setNewCollectionName("") before the fetch, and instead call setCreatingCollection(false) and setNewCollectionName("") inside the success branch where res.ok is true (after setCollections), while leaving setCollectionError(...) and keeping the draft intact in the else and catch branches; update references to newCollectionName, name, setCreatingCollection, setNewCollectionName, setCollectionError and the POST handling accordingly.
572-575: Replace the static padding style with Tailwind classes.These values are fixed, so
px-8 py-12would keep the empty-state panel inside the repo's Tailwind-only styling convention.As per coding guidelines,
client/**/*.{tsx,jsx,ts,js,css}: Use Tailwind CSS v4 for styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/DashboardClient.tsx` around lines 572 - 575, The empty-state container div in DashboardClient.tsx currently uses an inline style for padding (style={{ padding: "48px 32px" }}); replace that inline style with the equivalent Tailwind padding classes by removing the style prop and adding px-8 py-12 to the div's className (the div with className starting "flex flex-col items-center...") so the component uses Tailwind-only styling as required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/app/api/collections/route.ts`:
- Around line 30-38: Check for an existing collection with the same name for the
current user before creating and enforce user-scoped uniqueness in the database
schema: in route.ts, query prisma.collection.findFirst or findUnique with where:
{ userId: user.id, name } and return a 409/400 JSON error if found, and also add
a unique constraint to the Collection model (@@unique([userId, name]) or
equivalent) in the Prisma schema and run a migration; finally, handle Prisma
unique-constraint errors around prisma.collection.create (catch the specific
error code and return the same 409/400) so creating duplicates is prevented both
at the app and DB level.
---
Duplicate comments:
In `@client/src/components/DashboardClient.tsx`:
- Around line 137-157: handleDeleteCollection does an optimistic removal but
doesn't restore the activeCollection when the delete is rolled back; capture
whether the deleted item was active (e.g. const wasActive = activeCollection ===
id) before calling setCollections and setActiveCollection(null), then update the
revert closure to not only reinsert the deletedItem via setCollections but also,
if wasActive is true, call setActiveCollection(id) to restore the filter when
the rollback runs.
---
Nitpick comments:
In `@client/src/components/DashboardClient.tsx`:
- Around line 115-133: The code clears the draft (calls
setCreatingCollection(false) and setNewCollectionName("")) before the POST
completes, losing the user's input on failure; change the flow so you keep
newCollectionName (and keep the form open) until the request succeeds: remove
the early calls to setCreatingCollection(false) and setNewCollectionName("")
before the fetch, and instead call setCreatingCollection(false) and
setNewCollectionName("") inside the success branch where res.ok is true (after
setCollections), while leaving setCollectionError(...) and keeping the draft
intact in the else and catch branches; update references to newCollectionName,
name, setCreatingCollection, setNewCollectionName, setCollectionError and the
POST handling accordingly.
- Around line 572-575: The empty-state container div in DashboardClient.tsx
currently uses an inline style for padding (style={{ padding: "48px 32px" }});
replace that inline style with the equivalent Tailwind padding classes by
removing the style prop and adding px-8 py-12 to the div's className (the div
with className starting "flex flex-col items-center...") so the component uses
Tailwind-only styling as required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 510f3f9e-3aa9-4d45-989e-dc15b8116047
📒 Files selected for processing (9)
client/next.config.tsclient/src/app/admin/page.tsxclient/src/app/api/collections/route.tsclient/src/app/globals.cssclient/src/app/layout.tsxclient/src/app/page.tsxclient/src/components/BookmarkCard.tsxclient/src/components/DashboardClient.tsxclient/src/lib/url-validation.ts
💤 Files with no reviewable changes (1)
- client/next.config.ts
✅ Files skipped from review due to trivial changes (2)
- client/src/app/globals.css
- client/src/lib/url-validation.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/app/admin/page.tsx
- client/src/app/page.tsx
Comment on lines
+30
to
+38
| const name = typeof body.name === "string" ? body.name.trim() : ""; | ||
| if (!name) return NextResponse.json({ error: "name is required" }, { status: 400 }); | ||
| if (name.length > 100) return NextResponse.json({ error: "name too long" }, { status: 400 }); | ||
|
|
||
| let collection; | ||
| try { | ||
| collection = await prisma.collection.create({ | ||
| data: { name, userId: user.id }, | ||
| }); |
There was a problem hiding this comment.
Reject duplicate collection names per user.
This still allows the same account to create two collections with the same visible name. DashboardClient renders only col.name, so duplicate pills and delete confirmations become ambiguous very quickly. Please block duplicates here and back that up with user-scoped uniqueness instead of inserting another row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/app/api/collections/route.ts` around lines 30 - 38, Check for an
existing collection with the same name for the current user before creating and
enforce user-scoped uniqueness in the database schema: in route.ts, query
prisma.collection.findFirst or findUnique with where: { userId: user.id, name }
and return a 409/400 JSON error if found, and also add a unique constraint to
the Collection model (@@unique([userId, name]) or equivalent) in the Prisma
schema and run a migration; finally, handle Prisma unique-constraint errors
around prisma.collection.create (catch the specific error code and return the
same 409/400) so creating duplicates is prevented both at the app and DB level.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
P2.3 Collections (complete)
membership
filter banner
add/remove from collections
Optimistic UI (everything is now instant)
reverts on failure
failure
change on cards
Security
storing)
newlines from user input)
all API routes
bookmark DELETE
SQL editor to enable RLS
Bug fixes
of single string
routes
Landing page
hero)
tracking-wide style
Action required
After merging, run
supabase-rls.sqlin yourSupabase SQL editor to enable Row Level Security
on all tables.
Summary by CodeRabbit
New Features
Bug Fixes
Style & UX
Security & Config
Documentation