feat:updates for conversations #1955
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR extends conversation functionality to document links. It refactors viewer creation to support anonymous viewers in the views API, modifies default link settings to conditionally enable conversations, adds conversation sections in link management, creates new API routes for document-scoped conversations, and introduces pages for viewing document conversations. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
ee/limits/server.ts (2)
110-118: Inconsistent null handling forconversationsInDataroombetween plan types.The free plan path spreads
parsedDatadirectly, meaning ifparsedData.conversationsInDataroomis explicitlynull, it will override the default limits value. However, the paid plan path (lines 120-140) treatsnullas "not explicitly set" and falls back to the default. This inconsistency could lead to unexpected behavior.Consider applying the same null-handling logic to the free plan path:
if (isFreePlan(team.plan)) { + const conversationsInDataroom = + parsedData.conversationsInDataroom !== null && + parsedData.conversationsInDataroom !== undefined + ? parsedData.conversationsInDataroom + : (defaultLimits as any).conversationsInDataroom; + return { ...defaultLimits, ...parsedData, + ...(conversationsInDataroom !== undefined && { + conversationsInDataroom, + }), usage: { documents: documentCount, links: linkCount, users: userCount }, ...(isTrial && { users: 3, }), };
142-157: Inconsistent property presence between success and error paths.In the paid plan success path (lines 132-134),
conversationsInDataroomis only included if defined. However, in this error path, it's always present—either fromdefaultLimitsor defaulting tofalse. This means the return object shape differs depending on whether parsing succeeds or fails.Consider matching the conditional inclusion pattern from the success path:
const defaultLimits = planLimitsMap[basePlan] || FREE_PLAN_LIMITS; + const conversationsInDataroom = (defaultLimits as any).conversationsInDataroom ?? false; + return { ...defaultLimits, - // Preserve conversationsInDataroom from default limits if it exists - ...((defaultLimits as any).conversationsInDataroom === undefined && { - conversationsInDataroom: false, - }), + ...(conversationsInDataroom !== undefined && { + conversationsInDataroom, + }), usage: { documents: documentCount, links: linkCount, users: userCount },components/view/document-view.tsx (1)
31-49: Align DEFAULT_DOCUMENT_VIEW_TYPE and analytics with viewerId usageYou’re now destructuring and storing
viewerId, but the exported type and analytics payload haven’t fully caught up:
- Type definition is missing viewerId
DEFAULT_DOCUMENT_VIEW_TYPEdoes not currently declareviewerId, but you passviewerIdintosetViewData. This is a type mismatch.Consider updating the type:
export type DEFAULT_DOCUMENT_VIEW_TYPE = { viewId?: string; + viewerId?: string; file?: string | null; pages?: | {
- Analytics payload still uses viewId under viewerId key
Now that you have a true
viewerId, it’s more accurate to send that (and optionally fall back toviewId):- viewerId: viewId, + viewerId: viewerId ?? viewId,That keeps existing behavior but starts emitting a semantically correct viewer identifier when available.
Also applies to: 154-165, 191-201
ee/features/conversations/components/viewer/conversation-view-sidebar.tsx (1)
83-106: SWR cache keys for conversations are inconsistent when viewerId is missingThere’s a subtle but real mismatch between the fetch key and the mutate keys:
- Fetch key (documents)
WhendataroomIdis falsy andviewerIdis falsy,useSWRuses:`/api/conversations?linkId=${linkId}`- Mutate key (documents)
handleSendMessagealways mutates:which becomes`/api/conversations?linkId=${linkId}&viewerId=${viewerId}`viewerId=undefinedwhenviewerIdis falsy and does not match the fetch key.- Mutate key (create)
handleCreateConversationbuilds acacheKeythat always includes an&viewerId=${effectiveViewerId}suffix. For datarooms with no viewerId yet, the hook’s key isnull, so you’re mutating a key this component isn’t subscribed to.In scenarios where
viewerIdis not populated (e.g., preview views or any future flow where you deliberately omit viewerId), list views will not update in response to new messages or new conversations, because you’re mutating a different key than the one used byuseSWR.Consider centralizing key construction and reusing it everywhere:
- } = useSWR<Conversation[]>( - dataroomId - ? (viewerId - ? `/api/conversations?dataroomId=${dataroomId}&viewerId=${viewerId}` - : null) - : `/api/conversations?linkId=${linkId}${viewerId ? `&viewerId=${viewerId}` : ""}`, + const conversationsKey = + dataroomId + ? viewerId + ? `/api/conversations?dataroomId=${dataroomId}&viewerId=${viewerId}` + : null + : `/api/conversations?linkId=${linkId}${ + viewerId ? `&viewerId=${viewerId}` : "" + }`; + + } = useSWR<Conversation[]>( + conversationsKey, fetcher, { /* ... */ }, );Then in
handleCreateConversationandhandleSendMessage, mutate the same key (guarding againstnull):- const cacheKey = dataroomId - ? `/api/conversations?dataroomId=${dataroomId}&viewerId=${effectiveViewerId}` - : `/api/conversations?linkId=${linkId}&viewerId=${effectiveViewerId}`; - mutate( - cacheKey, + const cacheKey = dataroomId + ? effectiveViewerId + ? `/api/conversations?dataroomId=${dataroomId}&viewerId=${effectiveViewerId}` + : null + : `/api/conversations?linkId=${linkId}${ + effectiveViewerId ? `&viewerId=${effectiveViewerId}` : "" + }`; + if (cacheKey) { + mutate( + cacheKey, [newConversation, ...(conversations || [])], false, - ); + ); + }- mutate( - dataroomId - ? `/api/conversations?dataroomId=${dataroomId}&viewerId=${viewerId}` - : `/api/conversations?linkId=${linkId}&viewerId=${viewerId}`, + if (conversationsKey) { + mutate( + conversationsKey, conversations?.map((conv) => conv.id === activeConversation.id ? { /* ... */ } : conv, ), false, - ); + ); + }This keeps the cache coherent for both dataroom and document links and avoids divergent keys when
viewerIdis absent.Also applies to: 109-152, 192-206
components/links/link-sheet/index.tsx (1)
51-89: Default conversation state for document links may conflict with email auth expectationsWith
enableConversationnow defaulting totrueforDOCUMENT_LINK, new document links will start with:
enableConversation: trueemailProtected: trueemailAuthenticated: falseHowever,
ConversationSectionexplicitly turns both email flags on only when the user enables conversations via the toggle. Because the toggle isn’t invoked on creation, document links can end up with conversations enabled but email authentication still off.If the product intent is “Q&A conversations require authenticated viewers”, consider aligning the defaults, e.g.:
- emailAuthenticated: false, + emailAuthenticated: + linkType === LinkType.DOCUMENT_LINK ? true : false, ... - enableConversation: linkType === LinkType.DOCUMENT_LINK ? true : false, + enableConversation: linkType === LinkType.DOCUMENT_LINK ? true : false,or otherwise ensuring that whenever
enableConversationistruefor document links,emailAuthenticatedis alsotrue.
🧹 Nitpick comments (13)
ee/limits/server.ts (2)
120-134: Consider simplifying the type check and improving type safety.The condition
parsedData.conversationsInDataroom !== null && parsedData.conversationsInDataroom !== undefinedcan be simplified since the schema only allowsboolean | null | undefined. Additionally, usingas anybypasses type safety.Consider this refactor:
- const conversationsInDataroom = - parsedData.conversationsInDataroom !== null && - parsedData.conversationsInDataroom !== undefined - ? parsedData.conversationsInDataroom - : (defaultLimits as any).conversationsInDataroom; + const conversationsInDataroom = + typeof parsedData.conversationsInDataroom === 'boolean' + ? parsedData.conversationsInDataroom + : defaultLimits.conversationsInDataroom;Note: This assumes
TPlanLimitstype includesconversationsInDataroom?: boolean. If not, you may need to update the type definition instead of usingas any.
120-126: Improve type safety by updatingTPlanLimitstype definition.The repeated use of
(defaultLimits as any).conversationsInDataroomsuggests that theTPlanLimitstype doesn't include theconversationsInDataroomproperty, requiring type assertion to bypass checks. This reduces type safety and maintainability.Update the
TPlanLimitstype definition in the constants file to include:conversationsInDataroom?: boolean;This will eliminate the need for
as anycasting and provide proper type checking throughout the codebase.Also applies to: 150-152
pages/api/links/[id]/index.ts (1)
66-66: enableConversation wiring looks correctIncluding
enableConversationin the GET select and persisting it fromlinkDataon PUT aligns this route with the new conversations feature; the|| falsedefault keeps the field explicitly opt‑in.If you ever evolve this to a tri‑state flag, consider switching to
enableConversation: linkData.enableConversation ?? falseto avoid treating non‑boolean truthy values astrue.Also applies to: 401-405
lib/api/links/link-data.ts (1)
368-369: Additional fields in document linkData are fine, but watch duplicationSelecting
idandenableConversationon the link here is consistent with the new conversations feature and safe, though these fields may already be available from the base link query in/api/links/[id]. If duplication becomes confusing, you could narrow this select to just what consumers truly need.components/view/nav.tsx (1)
204-228: Conversation toggle behavior and gating look consistentThe updated effect dependencies ensure the keyboard handler respects the latest
conversationsEnabled/showConversationsvalues, and the Q&A button + sidebar now correctly key offconversationsEnabledrather thanisDataroom.Note: the
'c'shortcut only opens the sidebar (it doesn’t toggle it closed; Escape does), which matches the inline comment but is worth keeping in mind for UX.Also applies to: 312-321, 476-487
ee/features/conversations/components/dashboard/conversation-list-item.tsx (1)
20-38: Fallback toconversation.titleis now redundantWith
formatDocumentReferencealways returning a non-empty string (falls back to"Untitled conversation"), the|| conversation.titleat Line 67 will never be used. Consider inlining the fallback there or dropping the|| conversation.titlefor clarity.pages/documents/[id]/settings.tsx (1)
77-95: Confirm NavMenu active state and intended tab set on Settings pageWrapping
DocumentHeader+NavMenuin a<header>looks good. One thing to double‑check:
- On
/documents/[id]/settings,NavMenunow only renders an “Overview” item withsegment: \${document.id}`andhref: /documents/${document.id}`.- If
NavItemdetermines the active tab using the last URL segment (e.g.,"settings"), this page might show no tab as active.If the intent is for Settings to be an un-tabbed subpage (only showing a link back to Overview), this is fine; otherwise, consider adding a Settings nav item with a
segmentthat matches whatNavItemexpects for this route.ee/features/conversations/components/dashboard/link-option-conversation-section.tsx (1)
3-72: Behavior is good; normalizeData Rooms Pluslabel for consistencyThe new
linkTypeprop andisDocumentLinklogic nicely let this section adapt tooltip copy and upgrade plan between document and dataroom links.Minor nit:
requiredPlanuses"data rooms plus"while the upgradeplanuses"Data Rooms Plus", and other plan strings appear to use this capitalized form. For consistency (and to avoid potential string‑union mismatches), consider:- requiredPlan={isDocumentLink ? "Pro" : "data rooms plus"} + requiredPlan={isDocumentLink ? "Pro" : "Data Rooms Plus"} ... - plan: isDocumentLink ? "Pro" : "Data Rooms Plus", + plan: isDocumentLink ? "Pro" : "Data Rooms Plus",pages/documents/[id]/index.tsx (1)
187-195: Unnecessary fragment wrapper.The empty fragment
<>...</>wrapping the conditionalAnnotationSheetis redundant since the conditional expression already handles the rendering logic.- <> - {featureFlags?.annotations && ( - <AnnotationSheet - documentId={prismaDocument.id} - teamId={teamId} - numPages={primaryVersion.numPages || 1} - /> - )} - </>, + featureFlags?.annotations ? ( + <AnnotationSheet + key="annotations" + documentId={prismaDocument.id} + teamId={teamId} + numPages={primaryVersion.numPages || 1} + /> + ) : null,pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/index.ts (2)
10-10: Unused import:messageServiceThe
messageServiceis imported but not used in this file.-import { messageService } from "@/ee/features/conversations/lib/api/messages";
80-92: Redundant database query for link data.The link data is already included in the conversation query (lines 64-72), but a separate query is made to verify ownership. Consider reusing the included data.
// Verify conversation belongs to a link of this document if (conversation.linkId) { - const link = await prisma.link.findUnique({ - where: { id: conversation.linkId }, - select: { documentId: true }, - }); - - if (!link || link.documentId !== documentId) { + // Use the already-included link data from conversation query + // Need to adjust the include to also select documentId + const linkDocumentId = conversation.link?.document ? documentId : null; + // Or fetch document relationship - for now verify via link existence + if (!conversation.link) { return res.status(403).json({ error: "Conversation does not belong to this document", }); } }Alternatively, add
documentIdto the link select in the original query:link: { select: { + documentId: true, document: { select: { name: true, }, }, }, },Then verify with
conversation.link.documentId !== documentId.ee/features/conversations/pages/document-conversation-detail.tsx (1)
28-28: Unused import:BadgeThe
Badgecomponent is imported but not used anywhere in this file.-import { Badge } from "@/components/ui/badge";pages/api/teams/[teamId]/documents/[id]/conversations/index.ts (1)
117-121: Avoid usinganytype for better type safety.Multiple places use explicit
anytype casts which reduces TypeScript's ability to catch errors. Consider defining proper types or inferring from Prisma.You can leverage Prisma's generated types:
import { Prisma } from "@prisma/client"; type ConversationWithRelations = Prisma.ConversationGetPayload<{ include: { participants: true; messages: { take: 1; select: { content: true; createdAt: true } }; link: { select: { id: true; document: { select: { id: true; name: true } } } }; _count: { select: { messages: true } }; }; }>;Then use this type instead of
anyin the map/filter callbacks.Also applies to: 137-141, 143-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
app/api/views/route.ts(2 hunks)components/links/link-sheet/index.tsx(1 hunks)components/links/link-sheet/link-options.tsx(1 hunks)components/navigation-menu.tsx(4 hunks)components/view/document-view.tsx(2 hunks)components/view/nav.tsx(3 hunks)components/view/view-data.tsx(1 hunks)ee/features/conversations/api/conversations-route.ts(6 hunks)ee/features/conversations/components/dashboard/conversation-list-item.tsx(1 hunks)ee/features/conversations/components/dashboard/link-option-conversation-section.tsx(4 hunks)ee/features/conversations/components/viewer/conversation-view-sidebar.tsx(5 hunks)ee/features/conversations/lib/api/conversations/index.ts(3 hunks)ee/features/conversations/pages/document-conversation-detail.tsx(1 hunks)ee/features/conversations/pages/document-conversation-overview.tsx(1 hunks)ee/limits/server.ts(2 hunks)lib/api/links/link-data.ts(1 hunks)pages/api/links/[id]/index.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/index.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/read/index.ts(1 hunks)pages/api/teams/[teamId]/documents/[id]/conversations/index.ts(1 hunks)pages/documents/[id]/conversations/[conversationId]/index.tsx(1 hunks)pages/documents/[id]/conversations/index.tsx(1 hunks)pages/documents/[id]/index.tsx(4 hunks)pages/documents/[id]/settings.tsx(1 hunks)prisma/schema/conversation.prisma(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
pages/documents/[id]/settings.tsx (2)
components/documents/document-header.tsx (1)
DocumentHeader(68-987)components/navigation-menu.tsx (1)
NavMenu(30-59)
ee/features/conversations/pages/document-conversation-overview.tsx (9)
pages/documents/[id]/conversations/index.tsx (1)
DocumentConversationOverviewPage(3-5)lib/swr/use-document.ts (1)
useDocument(11-55)lib/utils.ts (1)
fetcher(48-62)components/documents/document-header.tsx (1)
DocumentHeader(68-987)components/navigation-menu.tsx (1)
NavMenu(30-59)components/ui/badge.tsx (1)
Badge(50-50)ee/features/conversations/components/dashboard/conversation-list-item.tsx (1)
ConversationListItem(7-82)components/ui/dialog.tsx (6)
Dialog(149-149)DialogContent(151-151)DialogHeader(152-152)DialogTitle(154-154)DialogDescription(155-155)DialogFooter(153-153)components/ui/button.tsx (1)
Button(71-71)
pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts (2)
lib/types.ts (1)
CustomUser(17-17)ee/features/conversations/lib/api/messages/index.ts (1)
messageService(6-160)
pages/documents/[id]/conversations/index.tsx (1)
ee/features/conversations/pages/document-conversation-overview.tsx (1)
DocumentConversationOverviewPage(55-316)
pages/documents/[id]/conversations/[conversationId]/index.tsx (1)
ee/features/conversations/pages/document-conversation-detail.tsx (1)
DocumentConversationDetailPage(83-489)
pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/index.ts (4)
pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts (1)
handler(13-103)pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/read/index.ts (1)
handler(11-82)pages/api/teams/[teamId]/documents/[id]/conversations/index.ts (1)
handler(14-235)lib/types.ts (1)
CustomUser(17-17)
components/navigation-menu.tsx (1)
components/sidebar/nav-main.tsx (1)
NavItem(29-44)
components/links/link-sheet/link-options.tsx (1)
ee/features/conversations/components/dashboard/link-option-conversation-section.tsx (1)
ConversationSection(9-77)
⏰ 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: Analyze (javascript-typescript)
🔇 Additional comments (12)
components/navigation-menu.tsx (1)
17-26: Count badge addition is clean and backwards‑compatibleThe new optional
countprop is threaded correctly throughNavMenu→NavItem, and the rendering logic (positive count → numeric badge, else fall back totag) preserves existing behavior for callers that don’t provide a count.Also applies to: 40-49, 61-69, 127-135
components/links/link-sheet/link-options.tsx (1)
275-301: Link-type-aware conversations wiring looks correctPassing
linkTypeintoConversationSectionfor dataroom links and adding a dedicated “Q&A Conversations” section forDOCUMENT_LINKcleanly aligns the UI with the new link-type‑aware behavior and plan gating. TheisAllowedexpression also matches the intended set of paid plans that can toggle this feature.pages/documents/[id]/conversations/[conversationId]/index.tsx (1)
1-5: Thin wrapper page is appropriateThis page cleanly re-exports the existing
DocumentConversationDetailcomponent under the document/conversation route without adding extra logic. Looks good.prisma/schema/conversation.prisma (1)
10-12: Makingdataroomrelation optional is consistent with document-scoped conversationsAllowing
dataroomId/dataroomto be nullable fits the new use cases where conversations are anchored to documents or links without a dataroom. KeepingonDelete: Cascadeand the index is appropriate.pages/documents/[id]/conversations/index.tsx (1)
1-5: Wrapper page cleanly exposes the document conversations overviewThis correctly wires the shared
DocumentConversationOverviewfeature into the/documents/[id]/conversationsroute without extra complexity.ee/features/conversations/lib/api/conversations/index.ts (1)
54-54: LGTM! Clean implementation for optional dataroomId.The conditional spread pattern correctly handles the optional
dataroomIdfor both the data payload and the include clause, enabling document-only conversations without dataroom association.Also applies to: 73-73, 111-111
pages/documents/[id]/index.tsx (2)
91-110: LGTM! Well-configured SWR hook for conversations.The SWR configuration with
revalidateOnFocus,dedupingInterval, andkeepPreviousDatais appropriate for this use case. The conditional fetching based onprismaDocument?.id && teamIdprevents unnecessary requests.
209-225: LGTM! Conditional NavMenu rendering with unread badge.The NavMenu is correctly rendered only when conversations exist, and the unread count badge logic (
unreadCount > 0 ? unreadCount : undefined) properly hides the badge when there are no unread messages.pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/read/index.ts (1)
11-29: LGTM! Proper authentication and parameter extraction.The session-based authentication and query parameter extraction are correctly implemented.
ee/features/conversations/pages/document-conversation-overview.tsx (1)
70-85: LGTM! Well-configured SWR hook with error handling.The SWR configuration includes appropriate error handling with toast notification, and the
keepPreviousDataoption provides a smooth UX during refetches.pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts (1)
86-93: LGTM! Correct async notification pattern with waitUntil.Using
waitUntilto trigger the notification task asynchronously is the correct pattern for Vercel serverless functions, allowing the response to return immediately while the notification is processed in the background.ee/features/conversations/api/conversations-route.ts (1)
276-297: Notifications are only sent for dataroom conversations.Document link conversations (where
dataroomIdis absent) will not trigger team member notifications. Verify this is the intended behavior, or add notification support for link-based conversations.Also applies to: 334-355
| try { | ||
| let viewer: { id: string; verified: boolean } | null = null; | ||
| if (email && !isPreview) { | ||
| // find or create a viewer | ||
| console.time("find-viewer"); | ||
| viewer = await prisma.viewer.findUnique({ | ||
| where: { | ||
| teamId_email: { | ||
| teamId: link.teamId!, | ||
| email: email, | ||
| if (!isPreview) { | ||
| if (email) { | ||
| // find or create a viewer with email | ||
| console.time("find-viewer"); | ||
| viewer = await prisma.viewer.findUnique({ | ||
| where: { | ||
| teamId_email: { | ||
| teamId: link.teamId!, | ||
| email: email, | ||
| }, | ||
| }, | ||
| }, | ||
| select: { id: true, verified: true }, | ||
| }); | ||
| console.timeEnd("find-viewer"); | ||
|
|
||
| if (!viewer) { | ||
| console.time("create-viewer"); | ||
| select: { id: true, verified: true }, | ||
| }); | ||
| console.timeEnd("find-viewer"); | ||
|
|
||
| if (!viewer) { | ||
| console.time("create-viewer"); | ||
| viewer = await prisma.viewer.create({ | ||
| data: { | ||
| email: email, | ||
| verified: isEmailVerified, | ||
| teamId: link.teamId!, | ||
| }, | ||
| select: { id: true, verified: true }, | ||
| }); | ||
| console.timeEnd("create-viewer"); | ||
| } else if (!viewer.verified && isEmailVerified) { | ||
| await prisma.viewer.update({ | ||
| where: { id: viewer.id }, | ||
| data: { verified: isEmailVerified }, | ||
| }); | ||
| } | ||
| } else { | ||
| // For anonymous viewers, create with a generated email based on IP and user agent | ||
| // This allows anonymous users to participate in conversations | ||
| const anonymousEmail = `anonymous-${ipAddress?.replace(/\./g, '-') || 'unknown'}-${Date.now()}@anonymous.papermark.com`; | ||
| console.time("create-anonymous-viewer"); | ||
| viewer = await prisma.viewer.create({ | ||
| data: { | ||
| email: email, | ||
| verified: isEmailVerified, | ||
| email: anonymousEmail, | ||
| verified: false, | ||
| teamId: link.teamId!, | ||
| }, | ||
| select: { id: true, verified: true }, | ||
| }); | ||
| console.timeEnd("create-viewer"); | ||
| } else if (!viewer.verified && isEmailVerified) { | ||
| await prisma.viewer.update({ | ||
| where: { id: viewer.id }, | ||
| data: { verified: isEmailVerified }, | ||
| }); | ||
| console.timeEnd("create-anonymous-viewer"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Anonymous viewer email construction will throw at runtime
In the anonymous viewer path you’re treating the ipAddress import as if it were a string:
const anonymousEmail = `anonymous-${ipAddress?.replace(/\./g, '-') || 'unknown'}-${Date.now()}@anonymous.papermark.com`;But ipAddress is a function from @vercel/functions (as you correctly use elsewhere via ipAddress(request)). Here, ipAddress is never called, so ipAddress?.replace attempts to invoke .replace on the function object, which doesn’t exist and will throw when this branch is hit.
This will break any non‑preview, non‑email‑protected view where email is absent (the new anonymous viewer flow).
A safe version would be:
- } else {
- // For anonymous viewers, create with a generated email based on IP and user agent
- // This allows anonymous users to participate in conversations
- const anonymousEmail = `anonymous-${ipAddress?.replace(/\./g, '-') || 'unknown'}-${Date.now()}@anonymous.papermark.com`;
- console.time("create-anonymous-viewer");
- viewer = await prisma.viewer.create({
- data: {
- email: anonymousEmail,
- verified: false,
- teamId: link.teamId!,
- },
- select: { id: true, verified: true },
- });
- console.timeEnd("create-anonymous-viewer");
- }
+ } else {
+ // For anonymous viewers, create with a generated email based on IP
+ // This allows anonymous users to participate in conversations
+ const ipAddressValue = ipAddress(request);
+ const sanitizedIp =
+ typeof ipAddressValue === "string"
+ ? ipAddressValue.replace(/\./g, "-")
+ : "unknown";
+ const anonymousEmail = `anonymous-${sanitizedIp}-${Date.now()}@anonymous.papermark.com`;
+
+ console.time("create-anonymous-viewer");
+ viewer = await prisma.viewer.create({
+ data: {
+ email: anonymousEmail,
+ verified: false,
+ teamId: link.teamId!,
+ },
+ select: { id: true, verified: true },
+ });
+ console.timeEnd("create-anonymous-viewer");
+ }The viewerId being returned in the response (viewerId: viewer?.id || undefined) is otherwise consistent with the new viewer flow and will be useful for conversations.
Also applies to: 679-683
🤖 Prompt for AI Agents
In app/api/views/route.ts around lines 470-519 (and the similar block at
679-683), the anonymous email construction treats ipAddress as a string but
ipAddress is a function; call ipAddress(request) to get the actual IP string,
guard that result before calling .replace, and fall back to 'unknown' if
undefined — e.g. compute const ip = ipAddress(request); const ipPart = ip ?
ip.replace(/\./g, '-') : 'unknown'; then use ipPart in the anonymous email
construction so no runtime error occurs when email is absent.
| viewerId: viewData.viewerId || ("viewerId" in viewData ? viewData.viewerId : undefined), | ||
| isMobile: isMobile, |
There was a problem hiding this comment.
Simplify viewerId assignment; conversationsEnabled gating looks good
The viewerId expression is effectively a no‑op and bypasses the "viewerId" in viewData type guard:
viewerId: viewData.viewerId || ("viewerId" in viewData ? viewData.viewerId : undefined),This is equivalent to just viewData.viewerId. If you want the type guard to matter, you can simplify and tighten it to:
- viewerId: viewData.viewerId || ("viewerId" in viewData ? viewData.viewerId : undefined),
+ viewerId: "viewerId" in viewData ? viewData.viewerId : undefined,The updated conversationsEnabled expression correctly enables:
- dataroom conversations when the dataroom view data flags them, and
- document conversations when
link.enableConversation === trueand there is nodataroomId.
Also applies to: 93-98
🤖 Prompt for AI Agents
In components/view/view-data.tsx around lines 87-88 (and also apply the same fix
to lines 93-98), the current viewerId expression is redundant and bypasses the
type guard; replace the long expression with a single clear assignment using
either viewData.viewerId or, if you need the type guard to distinguish undefined
vs missing, use a conditional: "viewerId" in viewData ? viewData.viewerId :
undefined; update the other occurrences at 93-98 the same way so the
type-narrowing behaves as intended.
| team = await prisma.team.findUnique({ | ||
| where: { id: link.teamId || link.document?.teamId }, | ||
| select: { | ||
| id: true, | ||
| }, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Potential undefined value in findUnique where clause.
If both link.teamId and link.document?.teamId are undefined/null, the findUnique query will use undefined as the id, which may cause unexpected behavior.
+ const teamId = link.teamId || link.document?.teamId;
+ if (!teamId) {
+ return res.status(400).json({ error: "Team not found for link" });
+ }
+
team = await prisma.team.findUnique({
- where: { id: link.teamId || link.document?.teamId },
+ where: { id: teamId },
select: {
id: 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.
| team = await prisma.team.findUnique({ | |
| where: { id: link.teamId || link.document?.teamId }, | |
| select: { | |
| id: true, | |
| }, | |
| }, | |
| select: { | |
| id: true, | |
| }, | |
| }); | |
| }); | |
| const teamId = link.teamId || link.document?.teamId; | |
| if (!teamId) { | |
| return res.status(400).json({ error: "Team not found for link" }); | |
| } | |
| team = await prisma.team.findUnique({ | |
| where: { id: teamId }, | |
| select: { | |
| id: true, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In ee/features/conversations/api/conversations-route.ts around lines 158-163,
the call to prisma.team.findUnique uses link.teamId || link.document?.teamId
which can be undefined; before calling findUnique validate that a concrete
teamId exists and handle the missing case (e.g., return an error/early-exit or
throw with a clear message), or alternatively change the lookup to a safer query
that doesn't receive undefined (e.g., guard the code path and only call
findUnique when teamId is truthy). Ensure you do not pass undefined into the
where clause and add a clear error/handling path for missing teamId.
| // For document links, create anonymous viewer if viewerId is not provided | ||
| if (!viewerId && !dataroomId) { | ||
| // Get IP address from request for anonymous viewer creation | ||
| const ipAddress = req.headers["x-forwarded-for"] || req.headers["x-real-ip"] || req.socket.remoteAddress || "unknown"; | ||
| const anonymousEmail = `anonymous-${String(ipAddress).replace(/\./g, '-')}-${Date.now()}@anonymous.papermark.com`; | ||
|
|
||
| // Create anonymous viewer | ||
| const anonymousViewer = await prisma.viewer.create({ | ||
| data: { | ||
| email: anonymousEmail, | ||
| verified: false, | ||
| teamId: team.id, | ||
| }, | ||
| select: { id: true }, | ||
| }); | ||
|
|
||
| // Use the anonymous viewer ID | ||
| viewerId = anonymousViewer.id; | ||
| } else if (!viewerId && dataroomId) { | ||
| return res.status(400).json({ error: "Viewer is required" }); | ||
| } |
There was a problem hiding this comment.
Consider privacy implications of storing IP in email field.
Storing the client IP address in the email field could raise privacy/compliance concerns (GDPR, CCPA). Consider using a hash or random identifier instead.
- const ipAddress = req.headers["x-forwarded-for"] || req.headers["x-real-ip"] || req.socket.remoteAddress || "unknown";
- const anonymousEmail = `anonymous-${String(ipAddress).replace(/\./g, '-')}-${Date.now()}@anonymous.papermark.com`;
+ // Use a random identifier instead of IP to avoid storing PII
+ const randomId = crypto.randomUUID().slice(0, 8);
+ const anonymousEmail = `anonymous-${randomId}-${Date.now()}@anonymous.papermark.com`;You'll need to import crypto from Node.js:
import crypto from "crypto";🤖 Prompt for AI Agents
In ee/features/conversations/api/conversations-route.ts around lines 170 to 190,
avoid embedding the raw client IP into the anonymous email (privacy risk);
instead generate a non-identifying identifier by hashing the IP (or using a
random UUID) and use that hash/ID in the anonymous email or as a separate
identifier; import Node's crypto module (import crypto from "crypto") and use
crypto.createHash('sha256').update(String(ipAddress)).digest('hex') (or
crypto.randomUUID()) to produce the opaque token, then construct the anonymous
email with that token rather than the raw IP and continue creating the viewer
with the token-based email.
| const ipAddress = req.headers["x-forwarded-for"] || req.headers["x-real-ip"] || req.socket.remoteAddress || "unknown"; | ||
| const anonymousEmail = `anonymous-${String(ipAddress).replace(/\./g, '-')}-${Date.now()}@anonymous.papermark.com`; |
There was a problem hiding this comment.
x-forwarded-for header may contain multiple IP addresses.
The x-forwarded-for header can contain a comma-separated list of IPs. Extract only the client IP (first in the list) for consistent behavior.
- const ipAddress = req.headers["x-forwarded-for"] || req.headers["x-real-ip"] || req.socket.remoteAddress || "unknown";
- const anonymousEmail = `anonymous-${String(ipAddress).replace(/\./g, '-')}-${Date.now()}@anonymous.papermark.com`;
+ const forwardedFor = req.headers["x-forwarded-for"];
+ const clientIp = (typeof forwardedFor === 'string' ? forwardedFor.split(',')[0].trim() : forwardedFor?.[0])
+ || req.headers["x-real-ip"]
+ || req.socket.remoteAddress
+ || "unknown";
+ const anonymousEmail = `anonymous-${String(clientIp).replace(/[.:]/g, '-')}-${Date.now()}@anonymous.papermark.com`;Note: Also handle IPv6 colons in the replacement pattern.
📝 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 ipAddress = req.headers["x-forwarded-for"] || req.headers["x-real-ip"] || req.socket.remoteAddress || "unknown"; | |
| const anonymousEmail = `anonymous-${String(ipAddress).replace(/\./g, '-')}-${Date.now()}@anonymous.papermark.com`; | |
| const forwardedFor = req.headers["x-forwarded-for"]; | |
| const clientIp = (typeof forwardedFor === 'string' ? forwardedFor.split(',')[0].trim() : forwardedFor?.[0]) | |
| || req.headers["x-real-ip"] | |
| || req.socket.remoteAddress | |
| || "unknown"; | |
| const anonymousEmail = `anonymous-${String(clientIp).replace(/[.:]/g, '-')}-${Date.now()}@anonymous.papermark.com`; |
🤖 Prompt for AI Agents
In ee/features/conversations/api/conversations-route.ts around lines 173-174,
the code currently reads the x-forwarded-for header as a whole and uses it in
the anonymous email; update it to parse x-forwarded-for as a comma-separated
list, take the first entry (trim whitespace) as the client IP, fall back to
x-real-ip / req.socket.remoteAddress / "unknown" if missing, and then sanitize
that single IP for the email by replacing dots and colons (e.g. use a regex to
replace /[.:]/g with '-') so IPv4 and IPv6 addresses are normalized
consistently.
| if (conversation.linkId) { | ||
| const link = await prisma.link.findUnique({ | ||
| where: { id: conversation.linkId }, | ||
| select: { documentId: true }, | ||
| }); | ||
|
|
||
| if (!link || link.documentId !== documentId) { | ||
| return res.status(403).json({ | ||
| error: "Conversation does not belong to this document", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Prisma schema files
fd -t f "schema.prisma"Repository: mfts/papermark
Length of output: 85
🏁 Script executed:
# Search for the Conversation model definition
rg -A 20 "model Conversation"Repository: mfts/papermark
Length of output: 4583
🏁 Script executed:
# Read the file to see the full context of this endpoint
cat -n pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/index.ts | head -100Repository: mfts/papermark
Length of output: 3463
🏁 Script executed:
# Search for conversation creation to understand if linkId should always be set
rg -A 5 "conversation.*create" --type ts -i | head -80Repository: mfts/papermark
Length of output: 6820
🏁 Script executed:
# Check if there are other endpoints or code that validates conversation-document relationship differently
rg "conversation.*linkId\|dataroomDocumentId" --type ts -B 2 -A 2 | head -100Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
# Search for other conversation endpoints to see how they validate access
rg "conversation.*findUnique\|conversation.*findMany" --type ts -B 3 -A 10 | grep -A 10 "documentId\|linkId\|dataroomDocumentId" | head -120Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
# Check the conversations list endpoint to see how it filters conversations
cat -n pages/api/teams/[teamId]/documents/[id]/conversations/index.ts | head -80Repository: mfts/papermark
Length of output: 2805
🏁 Script executed:
# Verify the relationship between conversations and dataroomDocument in the context
rg "dataroomDocumentId" prisma/schema/conversation.prisma -B 2 -A 2Repository: mfts/papermark
Length of output: 948
Conversations without a linkId bypass document access verification.
The endpoint filters conversations by verifying conversation.linkId belongs to the requested document only when linkId is present. If linkId is null, any conversation can be accessed regardless of whether it belongs to this document. Since conversations can be linked via linkId (document link), dataroomDocumentId (dataroom document), or neither (team-level), the verification must either reject conversations without a linkId or validate the appropriate relationship. This is also inconsistent with the list endpoint, which only returns conversations linked to the document's links.
| // Verify conversation belongs to a link of this document | ||
| if (conversation.linkId) { | ||
| const link = await prisma.link.findUnique({ | ||
| where: { id: conversation.linkId }, | ||
| select: { documentId: true }, | ||
| }); | ||
|
|
||
| if (!link || link.documentId !== documentId) { | ||
| return res.status(403).json({ | ||
| error: "Conversation does not belong to this document", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same authorization gap as read endpoint when conversation has no linkId.
Similar to the read endpoint, when conversation.linkId is null, no validation ensures the conversation belongs to this document. This could allow creating messages in conversations through unrelated document endpoints.
Apply the same fix as suggested for the read endpoint to ensure consistent authorization.
🤖 Prompt for AI Agents
In
pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts
around lines 65 to 77, the current check only validates the conversation when
conversation.linkId is present and leaves an authorization gap when linkId is
null; update the logic to also validate that the conversation belongs to the
document by comparing conversation.documentId to documentId when linkId is falsy
(or always fetch conversation.documentId and use it when linkId is absent), and
return 403 if the conversation.documentId does not match the documentId so the
same ownership check used in the read endpoint is applied here.
| // Create the message | ||
| const message = await messageService.createMessage({ | ||
| conversationId, | ||
| content: content.trim(), | ||
| userId, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for messageService exports and method definitions in the features directory
rg -n "export.*messageService|createMessage|addMessage" --type ts ee/features/conversations/lib/api/messages/Repository: mfts/papermark
Length of output: 218
🏁 Script executed:
# Also check the actual file being reviewed to see how messageService is imported and used
rg -n "messageService\." pages/api/teams/\[teamId\]/documents/\[id\]/conversations/\[conversationId\]/messages/index.tsRepository: mfts/papermark
Length of output: 119
Change createMessage to addMessage at line 80.
The code calls messageService.createMessage, but the messageService only exports addMessage. This will cause a runtime error. Update the method call to match the actual export.
🤖 Prompt for AI Agents
In
pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts
around lines 79 to 84, the call uses messageService.createMessage but the
service exports addMessage; replace the call to messageService.addMessage so it
matches the actual export and pass the same payload (conversationId, content:
content.trim(), userId); verify any surrounding references or tests expecting
createMessage are updated to use addMessage as well.
| // Verify conversation belongs to a link of this document | ||
| if (conversation.linkId) { | ||
| const link = await prisma.link.findUnique({ | ||
| where: { id: conversation.linkId }, | ||
| select: { documentId: true }, | ||
| }); | ||
|
|
||
| if (!link || link.documentId !== documentId) { | ||
| return res.status(403).json({ | ||
| error: "Conversation does not belong to this document", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the full file to understand the complete context
wc -l "pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/read/index.ts"Repository: mfts/papermark
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Read the file to see full context
cat -n "pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/read/index.ts"Repository: mfts/papermark
Length of output: 2940
🏁 Script executed:
#!/bin/bash
# Search for Conversation model/type definition
rg -n "type Conversation|interface Conversation" --type tsRepository: mfts/papermark
Length of output: 1210
🏁 Script executed:
#!/bin/bash
# Search for how conversations are fetched and validated
rg -n "prisma\.conversation" --type ts -A 3 -B 1Repository: mfts/papermark
Length of output: 12543
🏁 Script executed:
#!/bin/bash
# Check if there are other relationship checks for conversations and documents
rg -n "linkId" --type ts -B 2 -A 2Repository: mfts/papermark
Length of output: 50371
🏁 Script executed:
#!/bin/bash
# Check the conversation creation logic to see if linkId is optional
rg -n "prisma.conversation.create" --type ts -A 10 -B 2 | head -100Repository: mfts/papermark
Length of output: 1370
🏁 Script executed:
#!/bin/bash
# Check the Conversation model definition in schema
find . -name "schema.prisma" -o -name "*.prisma" | xargs cat 2>/dev/null | grep -A 20 "model Conversation"Repository: mfts/papermark
Length of output: 2441
🏁 Script executed:
#!/bin/bash
# Check if there are other validation checks for dataroomId conversations
rg -n "dataroomId" "pages/api/teams/\[teamId\]/documents/\[id\]/conversations/\[conversationId\]/read/index.ts"Repository: mfts/papermark
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Check similar endpoints to see if they have the same issue
cat -n "pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/messages/index.ts" | sed -n '55,75p'Repository: mfts/papermark
Length of output: 890
🏁 Script executed:
#!/bin/bash
# Verify the issue exists in the DELETE endpoint too
cat -n "pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/index.ts" | sed -n '155,180p'Repository: mfts/papermark
Length of output: 1024
Add validation for conversations without a linkId to prevent authorization bypass.
Conversations can legitimately have linkId as null (for dataroom-scoped conversations). When a conversation lacks a linkId, the current code skips validation entirely and proceeds to mark it as read, which bypasses authorization. This allows users with access to a document to modify conversations from other datarooms in the same team.
The fix should explicitly reject conversations without a linkId:
// Verify conversation belongs to a link of this document
if (conversation.linkId) {
const link = await prisma.link.findUnique({
where: { id: conversation.linkId },
select: { documentId: true },
});
if (!link || link.documentId !== documentId) {
return res.status(403).json({
error: "Conversation does not belong to this document",
});
}
+ } else {
+ // Conversations without linkId should not be accessible from document endpoints
+ return res.status(403).json({
+ error: "Conversation does not belong to this document",
+ });
}This same pattern exists in the GET detail, GET messages, and DELETE endpoints for conversations—all need the same fix.
🤖 Prompt for AI Agents
pages/api/teams/[teamId]/documents/[id]/conversations/[conversationId]/read/index.ts
around lines 57-69: the current branch only validates conversations that have a
linkId, allowing conversations with null linkId to skip authorization and be
marked read (authorization bypass). Update the logic to explicitly reject
conversations with a null linkId (return 403 or equivalent error) when the
endpoint expects a document-scoped conversation; keep the existing
link.documentId check for non-null linkId. Apply the same explicit null-linkId
rejection in the corresponding GET detail, GET messages, and DELETE conversation
endpoints to prevent the same bypass.
| // Create the conversation | ||
| const conversation = await conversationService.createConversation({ | ||
| viewId, | ||
| userId, | ||
| data: { | ||
| ...data, | ||
| linkId, | ||
| }, | ||
| teamId, | ||
| }); | ||
|
|
||
| return res.status(201).json(conversation); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "*.ts" -o -name "*.js" | xargs rg -l "conversationService|createConversation" | head -20Repository: mfts/papermark
Length of output: 641
🏁 Script executed:
ast-grep --pattern 'createConversation($_) {
$$$
}'Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
rg "createConversation" -A 15 --type ts --type jsRepository: mfts/papermark
Length of output: 5430
🏁 Script executed:
cat -n pages/api/teams/[teamId]/documents/[id]/conversations/index.ts | sed -n '180,230p'Repository: mfts/papermark
Length of output: 1741
🏁 Script executed:
cat -n ee/features/conversations/lib/api/conversations/index.ts | sed -n '1,80p'Repository: mfts/papermark
Length of output: 2795
🏁 Script executed:
cat -n pages/api/teams/[teamId]/documents/[id]/conversations/index.ts | sed -n '1,180p'Repository: mfts/papermark
Length of output: 6394
🏁 Script executed:
rg "userId" ee/features/conversations/api/conversations-route.ts -B 5 -A 5 | head -60Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
cat -n ee/features/conversations/api/conversations-route.ts | sed -n '1,100p'Repository: mfts/papermark
Length of output: 3669
🏁 Script executed:
rg "createConversation" -B 10 ee/features/conversations/api/ -A 3Repository: mfts/papermark
Length of output: 2392
🏁 Script executed:
cat -n ee/features/conversations/api/team-conversations-route.ts | sed -n '1,150p'Repository: mfts/papermark
Length of output: 5490
🏁 Script executed:
rg "viewerId" pages/api/teams/[teamId]/documents/[id]/conversations/index.ts -B 3 -A 3Repository: mfts/papermark
Length of output: 1476
Remove unused viewerId parameter from request body handling.
The viewerId is extracted from the request body at line 192 but never passed to conversationService.createConversation. Since this is a team member endpoint (authenticated via session), conversations are created with userId instead. Either remove the viewerId destructuring to clarify the endpoint only accepts team member conversations, or pass viewerId if the endpoint should support viewer-based conversations.
🤖 Prompt for AI Agents
In pages/api/teams/[teamId]/documents/[id]/conversations/index.ts around lines
216-227 (viewerId was destructured from req.body at line ~192 but never used),
remove the unused viewerId destructuring from the request body and any related
references so the handler only uses the authenticated userId when calling
conversationService.createConversation; if instead you intended to support
viewer-based conversations, pass viewerId into the createConversation payload
(or validate it) — but default fix: delete the viewerId extraction to avoid
unused variable and clarify the endpoint only creates team-member conversations.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.