feat: add annotations #1868
feat: add annotations #1868
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds end-to-end document annotations: DB models + migration, Prisma schema relations, server APIs for team CRUD and link/view reads, SWR hooks, TipTap rich-text editor and form UI, admin AnnotationSheet, viewer AnnotationPanel/Toggle, resizable viewer integration, feature-flagging, and page wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Author
participant Sheet as AnnotationSheet
participant Form as AnnotationForm
participant API as /api/teams
participant DB as Database
Author->>Sheet: Open manager (Sheet/Dialog)
Sheet->>Form: Render form (create/edit)
Author->>Form: Submit title/pages/content/isVisible + images
Form->>API: POST/PUT /api/teams/:teamId/documents/:id/annotations[/:annotationId]
API->>DB: Create/Update annotation and images
DB-->>API: Saved annotation
API-->>Form: 201/200 OK
Form-->>Sheet: onSuccess -> close
Sheet->>API: GET /api/teams/:teamId/documents/:id/annotations (mutate)
API-->>Sheet: annotations list
sequenceDiagram
autonumber
actor Viewer
participant Nav as Nav (AnnotationToggle)
participant ViewerComp as Pages Viewer
participant SWR as useViewerAnnotations
participant API as /api/links
participant Panel as AnnotationPanel
Viewer->>Nav: Toggle annotations
Nav-->>ViewerComp: onToggle(enabled)
ViewerComp->>SWR: fetch annotations (if enabled)
SWR->>API: GET /api/links/:id[/documents/:docId]/annotations?viewId=...
API-->>SWR: visible annotations (sanitized)
SWR-->>ViewerComp: annotations
ViewerComp->>Panel: render annotations for currentPage
Panel-->>Viewer: show per-page cards/images
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (42)
components/view/viewer/pdf-default-viewer.tsx (1)
314-314: Unify tracking options; avoid calling getTrackingOptions() in render.Use the same (memoized) options object you pass to the tracker to prevent drift if overrides are introduced and to avoid repeated calls during render.
Apply this diff for the changed line:
- inactivityThreshold={getTrackingOptions().inactivityThreshold} + inactivityThreshold={trackingOptions.inactivityThreshold}Add and reuse a memoized options object elsewhere in the component:
// inside PDFViewer const trackingOptions = useMemo(() => getTrackingOptions(), []); const { trackPageViewSafely, resetTrackingState, startIntervalTracking, stopIntervalTracking, getActiveDuration, isInactive, updateActivity, } = useSafePageViewTracker({ ...trackingOptions, externalStartTimeRef: startTimeRef, });Please confirm TRACKING_CONFIG doesn’t change at runtime; if it can, we can memoize based on a version/flag.
lib/featureFlags/index.ts (1)
12-14: Guard against unknown flags from edge-config.Avoid polluting
teamFeaturesifbetaFeaturescontains unexpected keys.Apply:
- if (betaFeatures) { - for (const [featureFlag, teamIds] of Object.entries(betaFeatures)) { - if (teamIds.includes(teamId)) { - teamFeatures[featureFlag as BetaFeatures] = true; - } - } - } + if (betaFeatures) { + for (const [featureFlag, teamIds] of Object.entries(betaFeatures)) { + if ((featureFlag as string) in teamFeatures && teamIds.includes(teamId)) { + teamFeatures[featureFlag as BetaFeatures] = true; + } + } + }Also applies to: 29-31, 50-56
components/ui/resizable.tsx (3)
30-36: Missingafter:content-[""]prevents the pseudo-element line from rendering; add an accessible label.Without
content, the::afterpseudo doesn’t render. Also add a defaultaria-labelfor the handle.Apply:
<ResizablePrimitive.PanelResizeHandle - className={cn( - "relative flex w-px items-center justify-center bg-border after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-1 data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:-translate-y-1/2 data-[panel-group-direction=vertical]:after:translate-x-0 [&[data-panel-group-direction=vertical]>div]:rotate-90", - className, - )} - {...props} + className={cn( + "relative flex w-px items-center justify-center bg-border after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 after:content-[''] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring focus-visible:ring-offset-1 data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:-translate-y-1/2 data-[panel-group-direction=vertical]:after:translate-x-0 [&[data-panel-group-direction=vertical]>div]:rotate-90", + className, + )} + aria-label="Resize" + {...props} >
8-19: Consider forwardRef for PanelGroup/Handle for imperative APIs.If consumers need programmatic control, expose refs.
I can provide a ref-friendly wrapper if desired.
Also applies to: 21-23, 45-45
37-41: Touch target is quite small for mouse/touch.Optionally increase hit area (e.g., add
p-1on the handle ormin-w-[8px]/min-h-[8px]with transparent background) to improve usability without changing the visual line.pages/api/links/[id]/documents/[documentId]/annotations.ts (2)
66-92: Dataroom path LGTM; consider narrowing selected fields to minimize payload.Limit included fields on nested
document.annotations.imagesto only what the client needs (e.g.,id,url,width,height) to reduce overexposure.
100-108: Sanitization is good; tighten image shape explicitly.If
AnnotationImagecontains internal fields (team/user IDs, storage keys), map to a public DTO shape.Example:
- images: annotation.images, + images: annotation.images.map(({ id, url, width, height }) => ({ id, url, width, height })),pages/documents/[id]/index.tsx (1)
96-104: Fix potential React key warning and remove unnecessary Fragment.Give the conditional element a stable key and drop the wrapper fragment inside the actions array.
Apply:
- <> - {featureFlags?.annotations && ( - <AnnotationSheet - documentId={prismaDocument.id} - teamId={teamInfo?.currentTeam?.id!} - numPages={primaryVersion.numPages || 1} - /> - )} - </>, + {featureFlags?.annotations && ( + <AnnotationSheet + key={"annotation-sheet"} + documentId={prismaDocument.id} + teamId={teamInfo?.currentTeam?.id!} + numPages={primaryVersion.numPages || 1} + /> + )},components/view/annotations/annotation-toggle.tsx (1)
29-36: Add basic a11y: aria-pressed and aria-label on the toggle button.Improves screen-reader UX without behavioral changes.
<Button variant="ghost" size="sm" onClick={() => onToggle(!enabled)} className={`${ enabled ? "bg-muted text-foreground" : "text-muted-foreground" } ${!hasAnnotations ? "cursor-not-allowed opacity-50" : ""}`} disabled={!hasAnnotations} + aria-pressed={enabled} + aria-label={ + !hasAnnotations + ? "No annotations available" + : enabled + ? "Hide annotations" + : "Show annotations" + } >pages/api/links/[id]/annotations.ts (2)
61-70: Avoid logic drift with the dataroom-specific endpoint.This endpoint partially duplicates pages/api/links/[id]/documents/[documentId]/annotations.ts. Extract a shared helper (e.g., getSanitizedAnnotations({ link, documentId? })) to keep behavior consistent and tested once.
72-80: Sanitize image fields before returning to viewers.Returning full AnnotationImage objects may leak internal fields. Map to a minimal shape (id, url, filename).
- const sanitizedAnnotations = annotations.map((annotation) => ({ + const sanitizedAnnotations = annotations.map((annotation) => ({ id: annotation.id, title: annotation.title, content: annotation.content, pages: annotation.pages, - images: annotation.images, + images: annotation.images.map((img) => ({ + id: img.id, + url: img.url, + filename: img.filename, + })), createdAt: annotation.createdAt, }));If AnnotationImage has other safe-to-expose fields (e.g., width/height), include them explicitly.
prisma/schema/schema.prisma (1)
68-68: Confirm relation name & delete behavior for User ↔ DocumentAnnotation
- DocumentAnnotation.createdBy already has onDelete: Cascade (prisma/schema/annotation.prisma:10-11).
- User.createdAnnotations is an implicit back-relation (prisma/schema/schema.prisma:68). If you want to retain annotations when a user is deleted, make createdById nullable (String?) and use onDelete: SetNull; otherwise Cascade is correct. Add a named @relation on both sides to avoid ambiguity if you later add other User relations (e.g., lastEditedBy).
components/documents/annotations/annotation-form.tsx (4)
28-36: Tighten validation and normalize payload before submit.
- content: z.any() disables validation; define a minimal TipTap JSON shape or at least unknown/object.
- pages: enforce bounds [1..numPages] client-side before sending to prevent bad payloads if UI gets out of sync.
Apply this diff:
-const formSchema = z.object({ - title: z - .string() - .min(1, "Title is required") - .max(100, "Title must be less than 100 characters"), - content: z.any().optional(), - pages: z.array(z.number()).min(1, "At least one page must be selected"), - isVisible: z.boolean().default(true), -}); +const TipTapJson = z + .object({ + type: z.string().optional(), + content: z.array(z.any()).optional(), + }) + .passthrough(); // be permissive but not 'any' + +const formSchema = z.object({ + title: z.string().min(1, "Title is required").max(100, "Title must be less than 100 characters"), + content: TipTapJson.nullable().optional(), + pages: z.array(z.number()).min(1, "At least one page must be selected"), + isVisible: z.boolean().default(true), +});- const response = await fetch(url, { + const payload = { + ...values, + // defense-in-depth: dedupe and bound-check pages + pages: Array.from(new Set(values.pages)).filter((p) => p >= 1 && p <= numPages), + content: editorContent, + }; + const response = await fetch(url, { method, headers: { "Content-Type": "application/json", }, - body: JSON.stringify({ - ...values, - content: editorContent, - }), + body: JSON.stringify(payload), });Also applies to: 96-106
160-170: Guard Checkbox state to boolean to avoid radix “indeterminate”.Radix’s onCheckedChange can deliver "indeterminate". Coerce to boolean before branching.
- onCheckedChange={(checked) => { - if (checked) { + onCheckedChange={(checked) => { + const isChecked = checked === true; + if (isChecked) { field.onChange([...field.value, page]); } else { field.onChange(field.value.filter((p) => p !== page)); } }}
72-85: Image uploads bypass the new annotation images API. Clarify expected persistence.Right now images are uploaded to storage and embedded in content; the /images API (pages/api/teams/[...]/images.ts) isn’t used, so annotation.images will stay empty. If downstream UI (e.g., AnnotationPanel’s image grid) depends on annotation.images, consider POSTing metadata there after upload, or confirm server will extract images from content.
I can wire a small helper to POST {filename,url,size,mimeType} to the images endpoint after upload if desired.
3-3: Remove unused import.useEffect is imported but not used.
-import { useEffect, useState } from "react"; +import { useState } from "react";pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId]/images.ts (1)
1-11: Validate request body with zod and enforce safe constraints.Replace ad-hoc checks with a schema: require https URL, image/* MIME, and reasonable size limits. This reduces attack surface and bad data.
import { NextApiRequest, NextApiResponse } from "next"; import { authOptions } from "@/pages/api/auth/[...nextauth]"; import { getServerSession } from "next-auth/next"; import { errorhandler } from "@/lib/errorHandler"; import prisma from "@/lib/prisma"; import { getTeamWithUsersAndDocument } from "@/lib/team/helper"; import { CustomUser } from "@/lib/types"; import { log } from "@/lib/utils"; +import { z } from "zod"; @@ - const { filename, url, size, mimeType } = req.body; - - if (!filename || !url || !mimeType) { - return res.status(400).json({ - error: "Missing required fields: filename, url, mimeType", - }); - } - - // Validate file type - if (!mimeType.startsWith("image/")) { - return res.status(400).json({ error: "Only image files are allowed" }); - } + const BodySchema = z.object({ + filename: z.string().min(1), + url: z + .string() + .url() + .refine((u) => u.startsWith("https://"), "URL must be https"), + size: z.number().int().nonnegative().max(10 * 1024 * 1024).optional(), // <=10MB + mimeType: z.string().startsWith("image/"), + }); + const { filename, url, size, mimeType } = BodySchema.parse(req.body);Optional: restrict allowed URL host to your blob/CDN domain(s).
Also applies to: 59-71
components/ui/rich-text-editor.tsx (1)
39-46: Disable base64 images to prevent massive payloads.allowBase64: true can let large inline images bloat your JSON/content and DB. Since you already upload to storage, disallow base64.
- Image.configure({ + Image.configure({ inline: true, - allowBase64: true, + allowBase64: false, HTMLAttributes: { class: "rounded-lg max-w-full h-auto", }, }),components/view/nav.tsx (1)
311-317: Good feature gating; consider UX when no annotations exist.The toggle is hidden unless both onToggleAnnotations and annotationsFeatureEnabled are set—nice. When hasAnnotations is undefined, the button is disabled via default false in the toggle; if annotations may load later, consider passing a tri-state (loading vs none) to avoid a disabled control flicker.
components/view/annotations/annotation-panel.tsx (3)
85-107: Fix combined marks rendering (bold+italic loses one).The current loop overwrites text when multiple marks exist. Wrap progressively so multiple marks compose.
- {node.content?.map((textNode: any, textIndex: number) => { - if (textNode.type === "text") { - let text = textNode.text; - // Apply formatting if marks exist - if (textNode.marks) { - textNode.marks.forEach((mark: any) => { - if (mark.type === "bold") { - text = ( - <strong key={textIndex} className="font-semibold"> - {text} - </strong> - ); - } else if (mark.type === "italic") { - text = ( - <em key={textIndex} className="italic"> - {text} - </em> - ); - } - }); - } - return text; + {node.content?.map((textNode: any, textIndex: number) => { + if (textNode.type === "text") { + let el: React.ReactNode = textNode.text; + for (const mark of textNode.marks ?? []) { + if (mark.type === "bold") { + el = <strong className="font-semibold">{el}</strong>; + } else if (mark.type === "italic") { + el = <em className="italic">{el}</em>; + } + } + return <span key={textIndex}>{el}</span>;
121-146: List rendering is brittle.Accessing list item text via nested indices will drop marks and break on deeper structures. Prefer rendering child nodes recursively or using TipTap’s generateHTML with your extensions to produce safe HTML.
I can refactor renderContent to a small recursive renderer or switch to generateHTML with a sanitization step.
260-266: Privacy: consider no-referrer on external images.Loading third‑party images leaks viewer referrers. Add referrerPolicy="no-referrer" to
tags (both here and in paragraph images).
- <img + <img src={image.url} alt={image.filename} className="h-auto max-h-32 w-full object-contain" loading="lazy" + referrerPolicy="no-referrer" />pages/view/[linkId]/index.tsx (1)
197-201: annotationsEnabled computed for dataroom but not passed to DataroomView.This makes the flag ineffective for dataroom links.
<DataroomView link={link} userEmail={verifiedEmail ?? storedEmail ?? userEmail} verifiedEmail={verifiedEmail} userId={userId} isProtected={!!(emailProtected || linkPassword || enableAgreement)} brand={brand} disableEditEmail={!!disableEditEmail} useCustomAccessForm={useCustomAccessForm} logoOnAccessForm={logoOnAccessForm} token={storedToken} previewToken={previewToken} preview={!!preview} dataroomIndexEnabled={dataroomIndexEnabled} + annotationsEnabled={annotationsEnabled} />Also applies to: 468-482
prisma/schema/annotation.prisma (2)
6-13: Consider removing denormalized teamId or enforce consistency.
DocumentAnnotationstoresteamIdeven thoughDocumentalready implies a team. This can drift. Either:
- Drop
teamIdand join viadocument.teamId, or- Keep it for query ergonomics but add invariants in application code (write-time check that
annotation.teamId === document.teamId) and consistently scope queries by both.Want a small Prisma middleware to enforce this invariant on create/update?
17-19: Indexing is good; consider a composite index for common filters.If your hot queries filter by
{ documentId, isVisible }or{ teamId, documentId }, a composite index may help.Also applies to: 32-33
components/view/viewer/pages-horizontal-viewer.tsx (2)
607-615: Ensure resizable layout reflows when toggling annotations.
defaultSizeis only used on mount; toggling the panel may not resize as intended. Force remount on toggle so defaults reapply.- <ResizablePanelGroup direction="horizontal"> + <ResizablePanelGroup + direction="horizontal" + key={`${Number(annotationsEnabled)}-${Number(!!hasAnnotations)}`} + >Also applies to: 879-897
823-834: Add accessible labels to navigation buttons.Screen readers currently get icon-only buttons.
- <button + <button + aria-label="Previous page" onClick={!isMobile ? goToPreviousPage : undefined} className={cn( "rounded-full bg-gray-950/50 p-1 transition-opacity duration-200 hover:bg-gray-950/75", "opacity-50 group-hover:opacity-100", )} > ... - <button + <button + aria-label="Next page" onClick={!isMobile ? goToNextPage : undefined} className={cn( "rounded-full bg-gray-950/50 p-1 transition-opacity duration-200 hover:bg-gray-950/75", "opacity-50 group-hover:opacity-100", )} >Also applies to: 846-857
components/documents/annotations/annotation-sheet.tsx (2)
59-60: Type the editing state to avoidany.Use the shared
Annotationtype to improve safety.- const [editingAnnotation, setEditingAnnotation] = useState<any>(null); + import type { Annotation } from "@/lib/swr/use-annotations"; + const [editingAnnotation, setEditingAnnotation] = useState<Annotation | null>(null);
82-112: Optional: optimistic visibility toggles.For a snappier UX, optimistically flip
isVisibleand rollback on failure.If you want, I can provide a
mutatepattern with rollback using SWR’s optimistic updates.pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (4)
13-23: Tighten validation: enforce integer pages and safer content shape.
- Use
.int().positive()for page numbers (disallow floats/0/negatives).- Deduplicate/sort pages on input to avoid duplicate storage.
- Prefer
z.unknown()orz.record(z.unknown())overz.any()to keep JSON-safe typing.Apply:
-const createAnnotationSchema = z.object({ - title: z - .string() - .min(1, "Title is required") - .max(100, "Title must be less than 100 characters"), - content: z.record(z.any()), // Rich text content as JSON - pages: z - .array(z.number().min(1)) - .min(1, "At least one page must be selected"), - isVisible: z.boolean().default(true), -}); +const createAnnotationSchema = z.object({ + title: z.string().min(1, "Title is required").max(100, "Title must be less than 100 characters"), + // Rich text content as JSON (opaque to server) + content: z.record(z.unknown()), + pages: z + .array(z.number().int().positive()) + .min(1, "At least one page must be selected") + .transform((arr) => Array.from(new Set(arr)).sort((a, b) => a - b)), + isVisible: z.boolean().default(true), +});
69-74: Don’t log PII verbatim to Slack.The log includes
userIdandteamId. Prefer hashing/truncation or omit unless strictly required.Example:
- message: `Failed to get annotations for document: _${docId}_. \n\n ${error} \n\n*Metadata*: \`{teamId: ${teamId}, userId: ${userId}}\``, + message: `Failed to get annotations for document: _${docId}_.\n\n${String(error)}\n\n*Metadata*: \`{teamId: ${teamId.slice(-6)}, userId: ${userId.slice(-6)}}\``,
73-74: Return after handling error to avoid fall-through.Ensures no further processing after error response.
- errorhandler(error, res); + return errorhandler(error, res);Also applies to: 127-128
25-25: Remove unusedupdateAnnotationSchemaor wire PATCH/PUT.Currently unused; keep dead code out or implement update route.
lib/swr/use-annotations.ts (4)
5-26: Narrow types forcontentand dates.Prefer
Record<string, unknown>forcontent. If API returns ISO strings, keepstring; if Dates, align consistently.- content: any; + content: Record<string, unknown>;
28-36: Avoid requests with missing IDs by using a null SWR key.Prevents hitting
/annotationswith “undefined” segments.-export function useAnnotations(documentId: string, teamId: string) { - const { data, error, mutate } = useSWR<Annotation[]>( - `/api/teams/${teamId}/documents/${documentId}/annotations`, +export function useAnnotations(documentId?: string, teamId?: string) { + const key = + teamId && documentId + ? `/api/teams/${teamId}/documents/${documentId}/annotations` + : null; + const { data, error, mutate } = useSWR<Annotation[]>( + key, fetcher, { revalidateOnFocus: false, revalidateOnReconnect: false, }, );
53-56: Build viewer endpoint withoutviewIdwhen it’s undefined.Prevents
?viewId=undefinedin the URL.- const endpoint = documentId - ? `/api/links/${linkId}/documents/${documentId}/annotations?viewId=${viewId}` - : `/api/links/${linkId}/annotations?viewId=${viewId}`; + const search = viewId ? `?viewId=${encodeURIComponent(viewId)}` : ""; + const endpoint = documentId + ? `/api/links/${linkId}/documents/${documentId}/annotations${search}` + : `/api/links/${linkId}/annotations${search}`;
57-64: Also null-key viewer hook iflinkIdis missing.Avoids accidental requests before nav data loads.
- const { data, error, mutate } = useSWR<Omit<Annotation, "createdBy">[]>( - endpoint, + const key = linkId ? endpoint : null; + const { data, error, mutate } = useSWR<Omit<Annotation, "createdBy">[]>( + key, fetcher, { revalidateOnFocus: false, revalidateOnReconnect: false, }, );components/view/viewer/pages-vertical-viewer.tsx (3)
120-125: Guard annotation fetching behind the feature flag to avoid unnecessary requests.Currently fetches even when annotations are disabled.
- // Fetch annotations for this link - const { annotations } = useViewerAnnotations(linkId, documentId, viewId); + // Fetch annotations only when feature enabled + const shouldFetchAnnotations = !!navData.annotationsFeatureEnabled; + const { annotations } = useViewerAnnotations( + shouldFetchAnnotations ? linkId : undefined, + shouldFetchAnnotations ? documentId : undefined, + shouldFetchAnnotations ? viewId : undefined, + );
778-783: Make the resizable layout reinitialize when the right panel mounts/unmounts.
defaultSizeapplies only on initial mount. Without reinit, toggling annotations may not adjust the left panel’s size consistently.- <ResizablePanelGroup direction="horizontal"> + <ResizablePanelGroup + key={`${annotationsEnabled}-${!!hasAnnotations}`} + direction="horizontal" + > {/* Document Content */} - <ResizablePanel - defaultSize={annotationsEnabled && hasAnnotations ? 75 : 100} - > + <ResizablePanel defaultSize={annotationsEnabled && hasAnnotations ? 75 : 100}>Please confirm whether your
ResizablePanelGroupresets state on child structure changes; if it already does, this can be skipped.
1052-1069: Render gate is solid; consider preloading annotations on toggle to reduce first-open latency.Optional: call
mutate()when the toggle flips on.- const handleToggleAnnotations = (enabled: boolean) => { - setAnnotationsEnabled(enabled); - }; + const handleToggleAnnotations = (enabled: boolean) => { + setAnnotationsEnabled(enabled); + if (enabled && typeof (mutate as any) === "function") { + // @ts-ignore mutate from hook scope if needed + mutate?.(); + } + };prisma/migrations/20250915000000_add_annotations/migration.sql (2)
19-20: Enforce non-null, non-emptypagesat DB level.Aligns DB constraints with API validation; prevents null/empty arrays.
- "pages" INTEGER[], + "pages" INTEGER[] NOT NULL,And add a check constraint:
+-- Ensure at least one page +ALTER TABLE "DocumentAnnotation" + ADD CONSTRAINT "DocumentAnnotation_pages_nonempty_chk" + CHECK (array_length("pages", 1) IS NOT NULL AND array_length("pages", 1) >= 1);
37-41: Add a covering index for viewer/admin queries.Likely filters by
documentIdandisVisible, sorts bycreatedAt.+-- Composite index for common access pattern +CREATE INDEX "DocumentAnnotation_doc_visible_createdAt_idx" + ON "DocumentAnnotation"("documentId" ASC, "isVisible" ASC, "createdAt" DESC);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
components/documents/annotations/annotation-form.tsx(1 hunks)components/documents/annotations/annotation-sheet.tsx(1 hunks)components/ui/form-hook.tsx(1 hunks)components/ui/resizable.tsx(1 hunks)components/ui/rich-text-editor.tsx(1 hunks)components/view/annotations/annotation-panel.tsx(1 hunks)components/view/annotations/annotation-toggle.tsx(1 hunks)components/view/document-view.tsx(3 hunks)components/view/nav.tsx(4 hunks)components/view/view-data.tsx(3 hunks)components/view/viewer/pages-horizontal-viewer.tsx(4 hunks)components/view/viewer/pages-vertical-viewer.tsx(4 hunks)components/view/viewer/pdf-default-viewer.tsx(1 hunks)lib/featureFlags/index.ts(2 hunks)lib/swr/use-annotations.ts(1 hunks)package.json(3 hunks)pages/api/links/[id]/annotations.ts(1 hunks)pages/api/links/[id]/documents/[documentId]/annotations.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId].ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId]/images.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/annotations/index.ts(1 hunks)pages/documents/[id]/index.tsx(3 hunks)pages/view/[linkId]/index.tsx(7 hunks)prisma/migrations/20250915000000_add_annotations/migration.sql(1 hunks)prisma/schema/annotation.prisma(1 hunks)prisma/schema/document.prisma(1 hunks)prisma/schema/schema.prisma(1 hunks)prisma/schema/team.prisma(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
components/view/viewer/pdf-default-viewer.tsx (1)
lib/tracking/tracking-config.ts (1)
getTrackingOptions(17-35)
pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId].ts (5)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (1)
handle(27-133)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/team/helper.ts (1)
getTeamWithUsersAndDocument(30-95)lib/utils.ts (1)
log(64-124)
components/view/annotations/annotation-toggle.tsx (2)
components/ui/tooltip.tsx (4)
TooltipProvider(160-160)Tooltip(155-155)TooltipTrigger(156-156)TooltipContent(159-159)components/ui/button.tsx (1)
Button(71-71)
pages/api/links/[id]/documents/[documentId]/annotations.ts (3)
pages/api/links/[id]/annotations.ts (1)
handle(10-90)lib/featureFlags/index.ts (1)
getFeatureFlags(18-59)lib/utils.ts (1)
log(64-124)
components/documents/annotations/annotation-sheet.tsx (2)
lib/swr/use-annotations.ts (1)
useAnnotations(28-44)components/documents/annotations/annotation-form.tsx (1)
AnnotationForm(48-230)
components/view/annotations/annotation-panel.tsx (2)
lib/swr/use-annotations.ts (1)
useViewerAnnotations(46-72)lib/utils/determine-text-color.ts (1)
determineTextColor(24-28)
pages/api/links/[id]/annotations.ts (3)
pages/api/links/[id]/documents/[documentId]/annotations.ts (1)
handle(8-118)lib/featureFlags/index.ts (1)
getFeatureFlags(18-59)lib/utils.ts (1)
log(64-124)
components/ui/resizable.tsx (1)
lib/utils.ts (1)
cn(18-20)
components/view/viewer/pages-horizontal-viewer.tsx (2)
lib/swr/use-annotations.ts (1)
useViewerAnnotations(46-72)components/view/annotations/annotation-panel.tsx (1)
AnnotationPanel(35-280)
components/view/viewer/pages-vertical-viewer.tsx (3)
lib/swr/use-annotations.ts (1)
useViewerAnnotations(46-72)components/ui/resizable.tsx (3)
ResizablePanelGroup(45-45)ResizablePanel(45-45)ResizableHandle(45-45)components/view/annotations/annotation-panel.tsx (1)
AnnotationPanel(35-280)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (4)
pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/team/helper.ts (1)
getTeamWithUsersAndDocument(30-95)lib/utils.ts (1)
log(64-124)
components/view/nav.tsx (1)
components/view/annotations/annotation-toggle.tsx (1)
AnnotationToggle(19-56)
pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId]/images.ts (6)
pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId].ts (1)
handle(27-131)pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (1)
handle(27-133)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/team/helper.ts (1)
getTeamWithUsersAndDocument(30-95)lib/utils.ts (1)
log(64-124)
lib/swr/use-annotations.ts (1)
lib/utils.ts (1)
fetcher(48-62)
pages/documents/[id]/index.tsx (2)
lib/utils.ts (1)
fetcher(48-62)components/documents/annotations/annotation-sheet.tsx (1)
AnnotationSheet(50-340)
components/ui/form-hook.tsx (1)
lib/utils.ts (1)
cn(18-20)
pages/view/[linkId]/index.tsx (1)
lib/featureFlags/index.ts (1)
getFeatureFlags(18-59)
components/documents/annotations/annotation-form.tsx (2)
lib/utils.ts (1)
uploadImage(519-529)components/ui/rich-text-editor.tsx (1)
RichTextEditor(29-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (15)
package.json (1)
34-34: Deps additions look good.Versions align with React 18/Next 14 and match usage introduced in this PR.
Also applies to: 68-71, 126-126, 132-132
pages/api/links/[id]/documents/[documentId]/annotations.ts (1)
37-43: Feature-flag gate is correct.Early-returning an empty array when disabled matches expected viewer behavior.
prisma/schema/document.prisma (2)
30-31: Relation addition LGTM.
annotations DocumentAnnotation[]is the right side of the one-to-many and matches the usage in queries.
1-39: Cascading deletes verified for annotations. DocumentAnnotation.document and AnnotationImage.annotation both use onDelete: Cascade in prisma/schema/annotation.prisma.pages/documents/[id]/index.tsx (1)
41-47: SWR gating looks good.Key is null until team is known, avoiding unnecessary requests. No action needed.
prisma/schema/team.prisma (1)
25-25: Team.annotations backref — verified. DocumentAnnotation declares teamId and a Team relation with onDelete: Cascade; Team.annotations is the implicit back-relation and no other Team↔DocumentAnnotation relations were found.components/view/view-data.tsx (1)
61-62: Nav wiring LGTM — annotationsFeatureEnabled propagated and used.
components/view/view-data.tsx sets annotationsFeatureEnabled (line 102); components/view/nav.tsx defines/destructures/uses it (lines 66, 109, 311); viewers gate panels on navData.annotationsFeatureEnabled (pages-vertical-viewer.tsx:1053, pages-horizontal-viewer.tsx:880).components/view/document-view.tsx (1)
68-69: Prop plumbing LGTM — confirm all callers pass annotationsEnabled.annotationsEnabled is correctly typed and forwarded in components/view/document-view.tsx (lines 68–69; also 90–91 and 304–305). Sandbox ripgrep returned no files, so I couldn't locate usages — verify all callers pass annotationsEnabled or confirm leaving it undefined is intentional.
pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId]/images.ts (1)
83-90: Return createdAt/updatedAt consistently or include include: {} if needed.If clients expect timestamps or additional fields on create (similar to GET), consider aligning response shape via include/select.
pages/view/[linkId]/index.tsx (1)
119-122: LGTM on wiring the annotations feature flag into document links.Flag retrieval and prop threading to DocumentView look consistent with getFeatureFlags.
Please confirm DocumentView propagates
annotationsEnabledinto itsnavData.annotationsFeatureEnabledso the viewer can render the toggle.Also applies to: 163-164, 272-273
components/view/viewer/pages-horizontal-viewer.tsx (1)
580-590: Nice integration of viewer annotations + toggle.Data fetch, toggle plumbed through Nav, and conditional right panel rendering look solid.
Please confirm Nav surfaces
onToggleAnnotationsandannotationsEnabledas expected and that the toggle is hidden when!navData.annotationsFeatureEnabled.Also applies to: 593-601, 879-897
components/documents/annotations/annotation-sheet.tsx (1)
120-129: Sheet UX and CRUD flows look good.Trigger, create dialog, list/empty-state, and SWR mutate usage are clean.
Also applies to: 168-185
prisma/migrations/20250915000000_add_annotations/migration.sql (1)
24-26: Confirm Prisma model uses@updatedAt.No DB default exists; ensure application sets
updatedAt.Reuse the script provided in the API review to verify
@updatedAtonupdatedAtin the model.components/ui/form-hook.tsx (1)
91-105: Label association: minor nit.Using
htmlFor={formItemId}is fine asFormControlassigns the sameid. Keep as-is.pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (1)
95-112: Resolved — DocumentAnnotation includes @updatedatVerified prisma/schema/annotation.prisma defines
updatedAt DateTime @updatedAt(prisma/schema/annotation.prisma:15); Prisma will populate this on create, so the migration's NOT NULL column is satisfied.
| const useFormField = () => { | ||
| const fieldContext = React.useContext(FormFieldContext); | ||
| const itemContext = React.useContext(FormItemContext); | ||
| const { getFieldState, formState } = useFormContext(); | ||
|
|
||
| const fieldState = getFieldState(fieldContext.name, formState); | ||
|
|
||
| if (!fieldContext) { | ||
| throw new Error("useFormField should be used within <FormField>"); | ||
| } | ||
|
|
||
| const { id } = itemContext; | ||
|
|
||
| return { | ||
| id, | ||
| name: fieldContext.name, | ||
| formItemId: `${id}-form-item`, | ||
| formDescriptionId: `${id}-form-item-description`, | ||
| formMessageId: `${id}-form-item-message`, | ||
| ...fieldState, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Fix useFormField provider checks and call order to avoid runtime errors.
getFieldState(fieldContext.name, ...) is called before verifying context presence; using the hook outside <FormField> can pass undefined to RHF. Also guard against missing <FormItem>.
Apply:
-const useFormField = () => {
- const fieldContext = React.useContext(FormFieldContext);
- const itemContext = React.useContext(FormItemContext);
- const { getFieldState, formState } = useFormContext();
-
- const fieldState = getFieldState(fieldContext.name, formState);
-
- if (!fieldContext) {
- throw new Error("useFormField should be used within <FormField>");
- }
-
- const { id } = itemContext;
-
- return {
- id,
- name: fieldContext.name,
- formItemId: `${id}-form-item`,
- formDescriptionId: `${id}-form-item-description`,
- formMessageId: `${id}-form-item-message`,
- ...fieldState,
- };
-};
+const useFormField = () => {
+ const fieldContext = React.useContext(FormFieldContext);
+ const itemContext = React.useContext(FormItemContext);
+ if (!fieldContext?.name) {
+ throw new Error("useFormField must be used within <FormField>");
+ }
+ if (!itemContext?.id) {
+ throw new Error("useFormField must be used within <FormItem>");
+ }
+ const { getFieldState, formState } = useFormContext();
+ const fieldState = getFieldState(fieldContext.name, formState);
+ const { id } = itemContext;
+ return {
+ id,
+ name: fieldContext.name,
+ formItemId: `${id}-form-item`,
+ formDescriptionId: `${id}-form-item-description`,
+ formMessageId: `${id}-form-item-message`,
+ ...fieldState,
+ };
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const useFormField = () => { | |
| const fieldContext = React.useContext(FormFieldContext); | |
| const itemContext = React.useContext(FormItemContext); | |
| const { getFieldState, formState } = useFormContext(); | |
| const fieldState = getFieldState(fieldContext.name, formState); | |
| if (!fieldContext) { | |
| throw new Error("useFormField should be used within <FormField>"); | |
| } | |
| const { id } = itemContext; | |
| return { | |
| id, | |
| name: fieldContext.name, | |
| formItemId: `${id}-form-item`, | |
| formDescriptionId: `${id}-form-item-description`, | |
| formMessageId: `${id}-form-item-message`, | |
| ...fieldState, | |
| }; | |
| }; | |
| const useFormField = () => { | |
| const fieldContext = React.useContext(FormFieldContext); | |
| const itemContext = React.useContext(FormItemContext); | |
| if (!fieldContext?.name) { | |
| throw new Error("useFormField must be used within <FormField>"); | |
| } | |
| if (!itemContext?.id) { | |
| throw new Error("useFormField must be used within <FormItem>"); | |
| } | |
| const { getFieldState, formState } = useFormContext(); | |
| const fieldState = getFieldState(fieldContext.name, formState); | |
| const { id } = itemContext; | |
| return { | |
| id, | |
| name: fieldContext.name, | |
| formItemId: `${id}-form-item`, | |
| formDescriptionId: `${id}-form-item-description`, | |
| formMessageId: `${id}-form-item-message`, | |
| ...fieldState, | |
| }; | |
| }; |
| const currentPageAnnotations = useMemo(() => { | ||
| if (!annotations) return []; | ||
| return annotations.filter((annotation) => | ||
| annotation.pages.includes(currentPage), | ||
| ); | ||
| }, [annotations, currentPage]); | ||
|
|
There was a problem hiding this comment.
Honor annotation.isVisible (defense-in-depth).
Filter out annotations marked non-visible in addition to page filtering. Even if the API already strips them, UI should defensively hide.
- const currentPageAnnotations = useMemo(() => {
+ const currentPageAnnotations = useMemo(() => {
if (!annotations) return [];
- return annotations.filter((annotation) =>
- annotation.pages.includes(currentPage),
- );
+ return annotations.filter(
+ (a) => a.isVisible && a.pages.includes(currentPage),
+ );
}, [annotations, currentPage]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currentPageAnnotations = useMemo(() => { | |
| if (!annotations) return []; | |
| return annotations.filter((annotation) => | |
| annotation.pages.includes(currentPage), | |
| ); | |
| }, [annotations, currentPage]); | |
| const currentPageAnnotations = useMemo(() => { | |
| if (!annotations) return []; | |
| return annotations.filter( | |
| (a) => a.isVisible && a.pages.includes(currentPage), | |
| ); | |
| }, [annotations, currentPage]); |
🤖 Prompt for AI Agents
components/view/annotations/annotation-panel.tsx around lines 54 to 60: the
currentPageAnnotations memo only filters by page but should also defensively
exclude annotations marked not visible; update the filter to only include
annotations where annotation.pages.includes(currentPage) AND
annotation.isVisible !== false (or Boolean(annotation.isVisible)) so any
annotation explicitly marked non-visible is filtered out in the UI.
| const updateAnnotationSchema = z.object({ | ||
| title: z | ||
| .string() | ||
| .min(1, "Title is required") | ||
| .max(100, "Title must be less than 100 characters") | ||
| .optional(), | ||
| content: z.record(z.any()).optional(), // Rich text content as JSON | ||
| pages: z | ||
| .array(z.number().min(1)) | ||
| .min(1, "At least one page must be selected") | ||
| .optional(), | ||
| isVisible: z.boolean().optional(), | ||
| }); |
There was a problem hiding this comment.
Accept null content and enforce integer pages in schema.
UpdateAnnotationSchema rejects content: null (UI sends null when editor empty) and allows non-integer page numbers. Fix to prevent 400s and sanitize pages.
Apply:
-const updateAnnotationSchema = z.object({
- title: z
- .string()
- .min(1, "Title is required")
- .max(100, "Title must be less than 100 characters")
- .optional(),
- content: z.record(z.any()).optional(), // Rich text content as JSON
- pages: z
- .array(z.number().min(1))
- .min(1, "At least one page must be selected")
- .optional(),
- isVisible: z.boolean().optional(),
-});
+const updateAnnotationSchema = z.object({
+ title: z.string().min(1, "Title is required").max(100, "Title must be less than 100 characters").optional(),
+ // Allow any JSON, including null
+ content: z.any().nullable().optional(),
+ // Only positive integers; at least one page if provided
+ pages: z.array(z.number().int().positive()).min(1, "At least one page must be selected").optional(),
+ isVisible: z.boolean().optional(),
+});Optionally normalize pages on write:
- const validatedData = updateAnnotationSchema.parse(req.body);
+ const validatedData = updateAnnotationSchema.parse(req.body);
+ if (validatedData.pages) {
+ validatedData.pages = Array.from(new Set(validatedData.pages)).sort((a, b) => a - b);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateAnnotationSchema = z.object({ | |
| title: z | |
| .string() | |
| .min(1, "Title is required") | |
| .max(100, "Title must be less than 100 characters") | |
| .optional(), | |
| content: z.record(z.any()).optional(), // Rich text content as JSON | |
| pages: z | |
| .array(z.number().min(1)) | |
| .min(1, "At least one page must be selected") | |
| .optional(), | |
| isVisible: z.boolean().optional(), | |
| }); | |
| const updateAnnotationSchema = z.object({ | |
| title: z.string().min(1, "Title is required").max(100, "Title must be less than 100 characters").optional(), | |
| // Allow any JSON, including null | |
| content: z.any().nullable().optional(), | |
| // Only positive integers; at least one page if provided | |
| pages: z.array(z.number().int().positive()).min(1, "At least one page must be selected").optional(), | |
| isVisible: z.boolean().optional(), | |
| }); |
| const updateAnnotationSchema = z.object({ | |
| title: z | |
| .string() | |
| .min(1, "Title is required") | |
| .max(100, "Title must be less than 100 characters") | |
| .optional(), | |
| content: z.record(z.any()).optional(), // Rich text content as JSON | |
| pages: z | |
| .array(z.number().min(1)) | |
| .min(1, "At least one page must be selected") | |
| .optional(), | |
| isVisible: z.boolean().optional(), | |
| }); | |
| const validatedData = updateAnnotationSchema.parse(req.body); | |
| if (validatedData.pages) { | |
| validatedData.pages = Array.from(new Set(validatedData.pages)).sort((a, b) => a - b); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (1)
41-65: Avoid team‑wide overfetch; query only this document’s annotations.Current call loads all team documents (and their annotations), then picks one; wasteful and forces a
// @ts-ignore. Fetch annotations directly after access check.- const { document } = await getTeamWithUsersAndDocument({ - teamId, - userId, - docId, - options: { - include: { - annotations: { - include: { - images: true, - createdBy: { - select: { - id: true, - name: true, - email: true, - }, - }, - }, - orderBy: { - createdAt: "desc", - }, - }, - }, - }, - }); - - // @ts-ignore - return res.status(200).json(document?.annotations || []); + // Validate access; avoid heavy includes + await getTeamWithUsersAndDocument({ teamId, userId, docId }); + const annotations = await prisma.documentAnnotation.findMany({ + where: { documentId: docId, teamId }, + include: { + images: true, + createdBy: { select: { id: true, name: true, email: true } }, + }, + orderBy: { createdAt: "desc" }, + }); + return res.status(200).json(annotations);Also applies to: 67-68
🧹 Nitpick comments (9)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (3)
25-26: Remove unusedupdateAnnotationSchemahere or extract shared schemas.It’s not used in this route and risks drift with the per‑annotation route. Either delete it here or move both create/update schemas to a shared module (e.g.,
lib/validation/annotations.ts) and import in both API files.-const updateAnnotationSchema = createAnnotationSchema.partial();
49-56: Minimize PII in responses unless required.Do you need
- createdBy: { - select: { - id: true, - name: true, - email: true, - }, - }, + createdBy: { select: { id: true, name: true } },
31-76: Consider pagination for large annotation sets.Add
limit/cursorto prevent unbounded payloads and client render cost.components/documents/annotations/annotation-form.tsx (6)
28-36: Align client schema: dropcontentfrom form schema and ensure integer pages.
contentisn’t a controlled RHF field; keep it out of the form schema to avoid confusion. Also mark pages as integers.-const formSchema = z.object({ +const formSchema = z.object({ title: z .string() .min(1, "Title is required") .max(100, "Title must be less than 100 characters"), - content: z.any().optional(), - pages: z.array(z.number()).min(1, "At least one page must be selected"), + pages: z.array(z.number().int()).min(1, "At least one page must be selected"), isVisible: z.boolean(), });
60-69: Remove unusedcontentdefault from form values.It isn’t bound to any field and can drift from
editorContent.const form = useForm<FormValues>({ resolver: zodResolver(formSchema), defaultValues: { title: annotation?.title || "", - content: annotation?.content || null, pages: annotation?.pages || [], isVisible: annotation?.isVisible !== undefined ? annotation.isVisible : true, }, });
108-112: Bubble up Zod field errors from the API for better UX.Surface server‑side validation details when available.
- if (!response.ok) { - const errorData = await response.json(); - throw new Error(errorData.error || "Failed to save annotation"); - } + if (!response.ok) { + const errorData = await response.json(); + const detailMsg = + Array.isArray(errorData?.details) && errorData.details.length + ? errorData.details.map((d: any) => d.message).join("; ") + : undefined; + throw new Error(detailMsg || errorData.error || "Failed to save annotation"); + }
133-147: Enforce 100‑char max client‑side.- <Input placeholder="Enter annotation title" {...field} /> + <Input + placeholder="Enter annotation title" + maxLength={100} + {...field} + />
40-46: Typeannotationto the server DTO to avoidany.Use a shared type (e.g.,
AnnotationDTO or PrismaDocumentAnnotationpick) so refactors are safer.
73-86: Optional: persist image metadata via the images API.If you intend to populate
annotation.images, callPOST /api/teams/:teamId/documents/:id/annotations/:annotationId/imagesafter create/update by extracting image URLs from the editor JSON.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/documents/annotations/annotation-form.tsx(1 hunks)components/ui/rich-text-editor.tsx(1 hunks)pages/api/teams/[teamId]/documents/[id]/annotations/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/ui/rich-text-editor.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (6)
pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId]/images.ts (1)
handle(12-91)pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId].ts (1)
handle(27-131)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/team/helper.ts (1)
getTeamWithUsersAndDocument(30-95)lib/utils.ts (1)
log(64-124)
components/documents/annotations/annotation-form.tsx (2)
lib/utils.ts (1)
uploadImage(519-529)components/ui/rich-text-editor.tsx (1)
RichTextEditor(29-246)
🔇 Additional comments (1)
components/documents/annotations/annotation-form.tsx (1)
165-170: Guard against indeterminate state from Checkbox.Some Checkbox libs can pass "indeterminate". Be explicit.
- onCheckedChange={(checked) => { - if (checked) { + onCheckedChange={(checked) => { + if (checked === true) { field.onChange([...field.value, page]); } else { field.onChange(field.value.filter((p) => p !== page)); } }}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
components/documents/annotations/annotation-form.tsx (1)
55-58: Good default for TipTap content; add JSONContent typing.Initialize with a valid doc is correct. Type it to avoid
anydrift.- const [editorContent, setEditorContent] = useState( - annotation?.content || { type: "doc", content: [] }, - ); + const [editorContent, setEditorContent] = useState<JSONContent>( + (annotation?.content as JSONContent) ?? { type: "doc", content: [] }, + );Add the type import:
+import type { JSONContent } from "@tiptap/core";pages/api/links/[id]/annotations.ts (1)
22-42: Use findFirst or a compound-unique selector; current findUnique shape is invalidfindUnique requires a unique selector.
{ id, linkId }isn’t valid unless you use the compound alias (e.g.,id_linkId). Use findFirst or the compound key.Apply this diff:
- const view = await prisma.view.findUnique({ - where: { id: viewId, linkId: linkId }, + const view = await prisma.view.findFirst({ + where: { id: viewId, linkId }, include: { - link: true, + link: true, document: { include: { annotations: { where: { isVisible: true, // Only return visible annotations for viewers }, include: { images: true, }, orderBy: { createdAt: "desc", }, }, }, }, }, });If you have a compound unique, prefer:
prisma.view.findUnique({ where: { id_linkId: { id: viewId, linkId } }, include: { link: true, document: { /* … */ } } })pages/api/links/[id]/documents/[documentId]/annotations.ts (1)
28-31: Use findFirst or compound-unique alias; current findUnique selector is invalidSame issue:
{ id, linkId }isn’t a valid unique selector unless using the compound alias.- const view = await prisma.view.findUnique({ - where: { id: viewId, linkId: linkId }, + const view = await prisma.view.findFirst({ + where: { id: viewId, linkId }, select: { id: true, viewedAt: true, link: { select: { id: true, linkType: true, teamId: true, documentId: true, dataroomId: true, }, }, }, });Or, if defined, use the compound key (e.g.,
where: { id_linkId: { id: viewId, linkId } }).
🧹 Nitpick comments (13)
components/documents/annotations/annotation-form.tsx (6)
28-36: Dropcontentfrom the form schema (it’s sourced from editor state anyway).Avoid schema/default duplication and potential drift;
contentis not bound to RHF fields.const formSchema = z.object({ title: z .string() .min(1, "Title is required") .max(100, "Title must be less than 100 characters"), - content: z.any().optional(), - pages: z.array(z.number()).min(1, "At least one page must be selected"), + pages: z + .array(z.number().int().gte(1)) + .min(1, "At least one page must be selected"), isVisible: z.boolean(), });
62-68: RemovecontentfromdefaultValuesto stay consistent with schema and submit payload.defaultValues: { title: annotation?.title || "", - content: annotation?.content || null, pages: annotation?.pages || [], isVisible: annotation?.isVisible !== undefined ? annotation.isVisible : true, },
69-71: Reset form whenannotationprop changes (edit/create switch).Prevents stale fields when editing different annotations.
}); + useEffect(() => { + form.reset({ + title: annotation?.title || "", + pages: annotation?.pages || [], + isVisible: annotation?.isVisible ?? true, + }); + setEditorContent( + (annotation?.content as JSONContent) ?? { type: "doc", content: [] }, + ); + }, [annotation, form]);If
AnnotationSheetreuses this component across items, please verify this behaves as expected.
108-111: Harden error handling for non‑JSON error responses.Avoids
.json()throwing on HTML/plain-text errors.- if (!response.ok) { - const errorData = await response.json(); - throw new Error(errorData.error || "Failed to save annotation"); - } + if (!response.ok) { + let message = "Failed to save annotation"; + try { + const ct = response.headers.get("content-type") || ""; + if (ct.includes("application/json")) { + const data = await response.json(); + message = data.error || data.message || message; + } else { + const text = await response.text(); + if (text) message = text; + } + } catch {} + throw new Error(message); + }
160-171: Coerce checkbox state strictly to boolean to avoid “indeterminate” truthiness.Prevents accidental adds if the component ever emits
"indeterminate".- onCheckedChange={(checked) => { - if (checked) { + onCheckedChange={(checked) => { + if (checked === true) { field.onChange([...field.value, page]); } else { field.onChange(field.value.filter((p) => p !== page)); } }}
223-226: Optional: rely on RHFisSubmittinginstead of manualisLoading.Simplifies state and prevents double toggles.
Example:
- const [isLoading, setIsLoading] = useState(false); + // remove local loading state; use form.formState.isSubmitting - setIsLoading(true); // ... submit logic ... - setIsLoading(false); - <Button type="submit" disabled={isLoading}> + <Button type="submit" disabled={form.formState.isSubmitting}>pages/api/links/[id]/annotations.ts (5)
48-51: Align TTL violation status with sibling endpointThis returns 404; the document-level endpoint returns 403. Use 403 for policy denials.
- if (view.viewedAt < new Date(Date.now() - 1000 * 60 * 60 * 23)) { - // if view is older than 23 hours, we should not allow the annotations to be accessed - return res.status(404).json({ error: "Annotation not found" }); - } + const cutoff = new Date(Date.now() - 23 * 60 * 60 * 1000); + if (view.viewedAt < cutoff) { + // if view is older than 23 hours, we should not allow the annotations to be accessed + return res.status(403).json({ error: "Access denied" }); + }
24-26: Avoid over-fetching: select only needed link fieldsYou only use
linkTypeandteamId. Select them to reduce payload.- include: { - link: true, + include: { + link: { + select: { linkType: true, teamId: true }, + }, document: {Also applies to: 41-41
44-46: Clarify 404 reasonReturn “View not found” for missing view to distinguish from “no annotations.”
- return res.status(404).json({ error: "Annotation not found" }); + return res.status(404).json({ error: "View not found" });
19-20: Optional: validate required query params earlyGuard missing
viewId/idwith a 400 before DB calls.const { id: linkId, viewId } = req.query as { id: string; viewId: string }; + if (!linkId || !viewId) { + return res.status(400).json({ error: "Missing linkId or viewId" }); + }
21-42: Add no-store for viewer responsesThese are TTL-gated and should not be cached by intermediaries.
try { + res.setHeader("Cache-Control", "private, no-store");Also applies to: 53-60, 82-83
pages/api/links/[id]/documents/[documentId]/annotations.ts (2)
64-69: Avoid non-null assertion on documentIdAdd a guard to prevent runtime errors if data is inconsistent.
- document = await prisma.document.findUnique({ - where: { id: view.link.documentId! }, + if (!view.link.documentId) { + return res.status(404).json({ error: "Document not found" }); + } + document = await prisma.document.findUnique({ + where: { id: view.link.documentId },
127-128: Set no-store caching for viewer payloadsPrevents stale/TTL-expired data from caching layers.
- return res.status(200).json(sanitizedAnnotations); + res.setHeader("Cache-Control", "private, no-store"); + return res.status(200).json(sanitizedAnnotations);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/documents/annotations/annotation-form.tsx(1 hunks)pages/api/links/[id]/annotations.ts(1 hunks)pages/api/links/[id]/documents/[documentId]/annotations.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId].ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/annotations/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pages/api/teams/[teamId]/documents/[id]/annotations/index.ts
- pages/api/teams/[teamId]/documents/[id]/annotations/[annotationId].ts
🧰 Additional context used
🧬 Code graph analysis (3)
pages/api/links/[id]/documents/[documentId]/annotations.ts (3)
pages/api/links/[id]/annotations.ts (1)
handle(10-90)lib/featureFlags/index.ts (1)
getFeatureFlags(18-59)lib/utils.ts (1)
log(64-124)
components/documents/annotations/annotation-form.tsx (2)
lib/utils.ts (1)
uploadImage(519-529)components/ui/rich-text-editor.tsx (1)
RichTextEditor(29-246)
pages/api/links/[id]/annotations.ts (3)
pages/api/links/[id]/documents/[documentId]/annotations.ts (1)
handle(8-135)lib/featureFlags/index.ts (1)
getFeatureFlags(18-59)lib/utils.ts (1)
log(64-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
pages/api/links/[id]/documents/[documentId]/annotations.ts (1)
49-52: LGTM: TTL gating present and correctBlocks views older than 23h with 403, matching policy. Keep this as the canonical behavior and align the sibling route.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (7)
14-24: Tighten validation: trim title and disallow unknown keys.Prevents whitespace-only titles and mass-assignment via unexpected fields.
-const createAnnotationSchema = z.object({ - title: z - .string() - .min(1, "Title is required") - .max(100, "Title must be less than 100 characters"), +const createAnnotationSchema = z + .object({ + title: z + .string() + .trim() + .min(1, "Title is required") + .max(100, "Title must be less than 100 characters"), content: z.record(z.any()).nullable().optional(), // Rich text content as JSON - allow null/omitted pages: z .array(z.number().int().min(1)) .min(1, "At least one page must be selected"), isVisible: z.boolean().default(true), -}); +}) + .strict();
26-26: Unused schema.
updateAnnotationSchemais defined but unused. Remove it or wire it up in the PUT/PATCH route to avoid drift.-const updateAnnotationSchema = createAnnotationSchema.partial(); +// TODO: add PUT/PATCH route and use this, or remove for now +// const updateAnnotationSchema = createAnnotationSchema.partial();
45-55: Minimize PII in responses: avoid leaking emails unless strictly needed.Returning
createdBy.emailto all team members may be unnecessary. Prefer id+name; add email only if the UI requires it.- createdBy: { select: { id: true, name: true, email: true } }, + createdBy: { select: { id: true, name: true } },And for POST include:
- createdBy: { - select: { - id: true, - name: true, - email: true, - }, - }, + createdBy: { select: { id: true, name: true } },Also applies to: 90-99
72-75: Normalize pages: de-duplicate and sort before persisting.Prevents redundant rows and stabilizes ordering.
- const validatedData = createAnnotationSchema.parse(req.body); + const validatedData = createAnnotationSchema.parse(req.body); + const normalizedPages = Array.from(new Set(validatedData.pages)).sort( + (a, b) => a - b, + );- ...validatedData, + ...validatedData, + pages: normalizedPages,Also applies to: 82-90
45-52: Consider pagination for large annotation sets.Add
take/cursor(orlimit/offset) query params to avoid unbounded responses for long-lived docs.
32-38: Reduce repetition: factor session retrieval.Minor DRY: extract a small
requireSession(req,res)helper returning{ userId }(403/401 inside).Also applies to: 62-68
56-61: Improve error log fidelity.Log
error.stackwhen available to aid triage; avoid embedding rich objects into Slack text.- message: `Failed to get annotations for document: _${docId}_. \n\n ${error} \n\n*Metadata*: \`{teamId: ${teamId}, userId: ${userId}}\``, + message: `Failed to get annotations for document: _${docId}_.\n\n${ + error instanceof Error ? error.stack : String(error) + }\n\n*Metadata*: \`{teamId: ${teamId}, userId: ${userId}}\``,Apply the same pattern in the POST path.
Also applies to: 111-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (4)
pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/team/helper.ts (1)
getTeamWithUsersAndDocument(30-95)lib/utils.ts (1)
log(64-124)
🔇 Additional comments (1)
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts (1)
85-85: Good handling of JSON nulls.Converting
undefined/nulltoPrisma.JsonNullis correct for JSON columns and preventsDbNullmix-ups.If the Prisma model allows both JSON null and DB null, ensure the column is JSON and not nullable at the DB level (or consistently use
JsonNull).
| const { teamId, id: docId } = req.query as { teamId: string; id: string }; | ||
| const userId = (session.user as CustomUser).id; | ||
|
|
||
| try { | ||
| // Validate access; avoid heavy includes | ||
| await getTeamWithUsersAndDocument({ teamId, userId, docId }); | ||
| const annotations = await prisma.documentAnnotation.findMany({ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: getTeamWithUsersAndDocument may throw when options are omitted (object spread of undefined).
lib/team/helper.ts spreads options into { ...options }. If options is undefined (as in both calls here), this can throw at runtime and 500 the route. Fix at the caller or make the helper resilient.
Quick caller-side fix (safe and local to this PR):
- await getTeamWithUsersAndDocument({ teamId, userId, docId });
+ await getTeamWithUsersAndDocument({
+ teamId,
+ userId,
+ docId,
+ // Minimal selection to validate existence without heavy includes
+ options: { select: { id: true, links: true } },
+ });Apply the same change to the POST path:
- await getTeamWithUsersAndDocument({
- teamId,
- userId,
- docId,
- });
+ await getTeamWithUsersAndDocument({
+ teamId,
+ userId,
+ docId,
+ options: { select: { id: true, links: true } },
+ });Repo-wide helper-side hardening (preferred, prevents future footguns):
--- a/lib/team/helper.ts
+++ b/lib/team/helper.ts
@@
const team = await prisma.team.findUnique({
where: { id: teamId },
include: {
users: { select: { userId: true } },
- documents: {
- ...options,
- },
+ documents: options
+ ? options
+ : { select: { id: true, links: true, ownerId: true } },
},
});Verification script:
Also applies to: 76-81
🏁 Script executed:
#!/bin/bash
# Confirm whether helper currently spreads possibly-undefined options
rg -n -C2 'getTeamWithUsersAndDocument|documents:\s*{\s*\.\.\.options' lib/team/helper.ts
# Find all call sites omitting options (to assess blast radius)
rg -nP 'getTeamWithUsersAndDocument\(\{\s*teamId:\s*|userId:\s*|docId:' -A4 | rg -n 'options:' -B4 -C0Length of output: 266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "---- show lib/team/helper.ts (if present) ----"
if [ -f "lib/team/helper.ts" ]; then
sed -n '1,400p' lib/team/helper.ts || true
else
echo "lib/team/helper.ts not found"
fi
echo
echo "---- literal '...options' occurrences ----"
rg -n -F '...options' || true
echo
echo "---- literal 'getTeamWithUsersAndDocument(' occurrences ----"
rg -n -F 'getTeamWithUsersAndDocument(' -S || true
echo
echo "---- getTeamWithUsersAndDocument matches with context ----"
rg -n -F -C5 'getTeamWithUsersAndDocument(' || true
echo
echo "---- show target annotations API file (if present) ----"
if [ -f 'pages/api/teams/[teamId]/documents/[id]/annotations/index.ts' ]; then
sed -n '1,240p' 'pages/api/teams/[teamId]/documents/[id]/annotations/index.ts' || true
else
echo "file not found at pages/api/teams/[teamId]/documents/[id]/annotations/index.ts"
rg -n --hidden --no-ignore 'annotations/index.ts' || true
fiLength of output: 21131
Critical: guard against spreading undefined options in getTeamWithUsersAndDocument
Verified: lib/team/helper.ts uses documents: { ...options } (and similar patterns) and multiple call sites omit options (e.g. pages/api/teams/[teamId]/documents/[id]/annotations/index.ts GET/POST). Spreading a nullish value can cause a runtime TypeError — fix at callers or harden the helper. Prefer hardening the helper to prevent future footguns.
Quick caller-side fix (safe and local to this PR):
- await getTeamWithUsersAndDocument({ teamId, userId, docId });
+ await getTeamWithUsersAndDocument({
+ teamId,
+ userId,
+ docId,
+ // Minimal selection to validate existence without heavy includes
+ options: { select: { id: true, links: true } },
+ });Apply the same change to the POST path:
- await getTeamWithUsersAndDocument({
- teamId,
- userId,
- docId,
- });
+ await getTeamWithUsersAndDocument({
+ teamId,
+ userId,
+ docId,
+ options: { select: { id: true, links: true } },
+ });Repo-wide helper-side hardening (preferred):
--- a/lib/team/helper.ts
+++ b/lib/team/helper.ts
@@
const team = await prisma.team.findUnique({
where: { id: teamId },
include: {
users: { select: { userId: true } },
- documents: {
- ...options,
- },
+ documents: options
+ ? options
+ : { select: { id: true, links: true, ownerId: true } },
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { teamId, id: docId } = req.query as { teamId: string; id: string }; | |
| const userId = (session.user as CustomUser).id; | |
| try { | |
| // Validate access; avoid heavy includes | |
| await getTeamWithUsersAndDocument({ teamId, userId, docId }); | |
| const annotations = await prisma.documentAnnotation.findMany({ | |
| const { teamId, id: docId } = req.query as { teamId: string; id: string }; | |
| const userId = (session.user as CustomUser).id; | |
| try { | |
| // Validate access; avoid heavy includes | |
| await getTeamWithUsersAndDocument({ | |
| teamId, | |
| userId, | |
| docId, | |
| // Minimal selection to validate existence without heavy includes | |
| options: { select: { id: true, links: true } }, | |
| }); | |
| const annotations = await prisma.documentAnnotation.findMany({ |
🤖 Prompt for AI Agents
pages/api/teams/[teamId]/documents/[id]/annotations/index.ts lines 39-45: the
call to getTeamWithUsersAndDocument may cause a runtime TypeError because the
helper spreads a possibly undefined/null options object; fix by hardening the
helper to default options to an empty object and only spread when non-null (e.g.
set the function param default to {} and use documents: { ...(options || {}) }),
and as a quick local fix for this file ensure the call includes an explicit
empty options object (pass options: {}) so nothing undefined gets spread.
Summary by CodeRabbit