Nda certificate #1943
Nda certificate #1943
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds NDA completion emails, a SignedNDA email template and sender, NDA certificate PDF generation endpoint, and UI actions to download certificates; updates view creation flows to collect team/admin emails and conditionally trigger NDA notification; makes link creation explicitly set agreement fields. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 3
🧹 Nitpick comments (7)
components/visitors/visitors-table.tsx (1)
299-350: Extract duplicate NDA certificate download logic into a shared utility.This download handler is duplicated across
visitors-table.tsx,views-table.tsx, anddataroom-visitors-table.tsx. Consider extracting it into a reusable utility function to improve maintainability.// lib/utils/download-nda-certificate.ts export async function downloadNdaCertificate( viewId: string, viewerName?: string | null, viewerEmail?: string | null, ): Promise<void> { const response = await fetch(`/api/views/${viewId}/nda-certificate`); if (!response.ok) { throw new Error("Failed to generate certificate"); } const blob = await response.blob(); const url = window.URL.createObjectURL(blob); const a = document.createElement("a"); a.href = url; a.download = `NDA-Certificate-${viewerName || viewerEmail || "Anonymous"}-${Date.now()}.pdf`; document.body.appendChild(a); a.click(); window.URL.revokeObjectURL(url); document.body.removeChild(a); }components/emails/signed-nda.tsx (1)
37-41: Consider using an environment variable for the base URL.The base URL
https://app.papermark.comis hardcoded. For flexibility across environments (staging, self-hosted instances), consider using an environment variable.+ const baseUrl = process.env.NEXT_PUBLIC_BASE_URL || "https://app.papermark.com"; const resourceUrl = dataroomId - ? `https://app.papermark.com/datarooms/${dataroomId}` + ? `${baseUrl}/datarooms/${dataroomId}` : documentId - ? `https://app.papermark.com/documents/${documentId}` - : `https://app.papermark.com`; + ? `${baseUrl}/documents/${documentId}` + : baseUrl;pages/api/views/[viewId]/nda-certificate.ts (1)
241-248: Add timeout to external PDF fetch requests.These external HTTP requests have no timeout, which could cause the API to hang if the remote server is slow or unresponsive.
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout const response = await fetch(agreement.content, { headers: { Accept: "application/pdf" }, + signal: controller.signal, }); + clearTimeout(timeoutId); if (!response.ok) { throw new Error("Failed to fetch agreement document"); }Apply similarly to the fetch at line 297.
app/api/views-dataroom/route.ts (2)
707-763: Approve, but consider extracting duplicated NDA email logic.The NDA email sending logic is correct and properly handles the async context. However, this block is nearly identical to lines 894-951 in the DOCUMENT_VIEW path. Consider extracting a helper function to reduce duplication.
// Example helper (could be placed outside POST or in a separate module) async function sendNDACompletionEmail({ request, viewId, dataroomId, documentId, link, email, name, }: { request: NextRequest; viewId: string; dataroomId?: string; documentId?: string; link: /* appropriate type */; email: string | null; name: string | null; }) { const geo = process.env.VERCEL === "1" ? geolocation(request) : LOCALHOST_GEO_DATA; const locationString = geo.city && geo.country ? geo.region && geo.region !== geo.city ? `${geo.city}, ${geo.region}, ${geo.country}` : `${geo.city}, ${geo.country}` : undefined; const adminUser = link.team.users.find((u) => u.role === "ADMIN"); const adminEmail = adminUser?.user.email || null; const teamMembers = link.team.users .map((u) => u.user.email) .filter((e): e is string => !!e && e !== adminEmail); await sendSignedNDAEmail({ ownerEmail: adminEmail, viewId, dataroomId, documentId, agreementName: link.agreement.name, linkName: link.name || `Link #${link.id.slice(-5)}`, viewerEmail: email, viewerName: name, teamMembers: teamMembers.length > 0 ? teamMembers : undefined, locationString, }); }
894-951: Duplicate NDA email logic – see earlier refactor suggestion.This block duplicates the DATAROOM_VIEW path logic. The same helper extraction suggested above would eliminate this duplication.
lib/emails/send-signed-nda.ts (2)
28-41: Move validation before template creation for efficiency.The
ownerEmailvalidation happens after template creation. While harmless, validating first avoids unnecessary work when the check fails.+}) => { + if (!ownerEmail) { + return { success: false, error: new Error("Document Owner not found") }; + } + const emailTemplate = SignedNDA({ viewId, documentId, dataroomId, agreementName, linkName, viewerEmail, viewerName, locationString, }); try { - if (!ownerEmail) { - throw new Error("Document Owner not found"); - }
43-47: Subject line logic can be simplified.The conditional reassignment can be expressed more concisely.
- let subjectLine: string = `NDA Agreement Signed: ${agreementName}`; const displayName = viewerName || viewerEmail || "Someone"; - if (displayName !== "Someone") { - subjectLine = `${displayName} signed the NDA: ${agreementName}`; - } + const subjectLine = displayName !== "Someone" + ? `${displayName} signed the NDA: ${agreementName}` + : `NDA Agreement Signed: ${agreementName}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/api/views-dataroom/route.ts(5 hunks)app/api/views/route.ts(4 hunks)components/analytics/views-table.tsx(2 hunks)components/emails/signed-nda.tsx(1 hunks)components/visitors/dataroom-visitors-table.tsx(2 hunks)components/visitors/visitors-table.tsx(2 hunks)lib/emails/send-signed-nda.ts(1 hunks)pages/api/links/index.ts(1 hunks)pages/api/views/[viewId]/nda-certificate.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
components/emails/signed-nda.tsx (1)
components/emails/shared/footer.tsx (1)
Footer(3-27)
components/visitors/dataroom-visitors-table.tsx (1)
components/ui/tooltip.tsx (1)
BadgeTooltip(53-107)
app/api/views-dataroom/route.ts (2)
lib/utils/geo.ts (1)
LOCALHOST_GEO_DATA(25-32)lib/emails/send-signed-nda.ts (1)
sendSignedNDAEmail(5-62)
components/analytics/views-table.tsx (1)
components/ui/tooltip.tsx (1)
BadgeTooltip(53-107)
lib/emails/send-signed-nda.ts (2)
components/emails/signed-nda.tsx (1)
SignedNDA(17-96)lib/resend.ts (1)
sendEmail(12-96)
app/api/views/route.ts (3)
lib/utils/geo.ts (1)
LOCALHOST_GEO_DATA(25-32)lib/emails/send-signed-nda.ts (1)
sendSignedNDAEmail(5-62)lib/utils.ts (1)
log(64-124)
components/visitors/visitors-table.tsx (1)
components/ui/tooltip.tsx (1)
BadgeTooltip(53-107)
⏰ 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 (6)
pages/api/links/index.ts (1)
196-200: LGTM!The explicit field assignment is cleaner than a conditional spread. The logic correctly ensures
agreementIdisnullwhen agreement is disabled, which aligns with the validation at lines 111-115.app/api/views/route.ts (3)
691-700: Well-handled geolocation with smart region deduplication.The logic correctly falls back to localhost data in development and avoids redundant region display when it matches the city name. The
undefinedreturn for missing data is appropriate.
680-686: LGTM - comprehensive condition checks.The conditions correctly verify all required data is present before attempting to send the NDA email: agreement must be enabled with a valid ID, notifications must be enabled, and the agreement data must exist.
703-712: Multiple admins are consistently handled using first admin as primary owner—this is intentional, not a bug.The codebase design treats the first admin as the document owner (primary recipient) and includes other team members (including additional admins) on CC. This same pattern is used in
pages/api/jobs/send-notification.tsline 192, showing it's a consistent architectural choice. The data model does allow multiple admins per team, so the scenario you identified is valid. However, the current implementation addresses this by including all team members on CC, meaning other admins receive the notification. If the intent is to notify all admins as primary recipients, that would require refactoring thesendSignedNDAEmailAPI to accept an array of owner emails.app/api/views-dataroom/route.ts (1)
137-152: LGTM – Necessary data expansion for NDA notifications.The additional
usersandagreementselections appropriately support the new NDA email feature by providing team member emails for CC and the agreement name for the subject line.lib/emails/send-signed-nda.ts (1)
5-62: LGTM – Clean email sending abstraction.The function provides a well-structured wrapper around
sendEmailwith proper error handling. The typed parameter object and consistent return shape make it easy to use from callers.
| <button | ||
| onClick={async (e) => { | ||
| e.stopPropagation(); | ||
| try { | ||
| const response = await fetch( | ||
| `/api/views/${row.original.id}/nda-certificate`, | ||
| ); | ||
| if (!response.ok) { | ||
| throw new Error( | ||
| "Failed to generate certificate", | ||
| ); | ||
| } | ||
| const blob = await response.blob(); | ||
| const url = window.URL.createObjectURL(blob); | ||
| const a = document.createElement("a"); | ||
| a.href = url; | ||
| a.download = `NDA-Certificate-${row.original.viewerEmail || "Anonymous"}-${Date.now()}.pdf`; | ||
| document.body.appendChild(a); | ||
| a.click(); | ||
| window.URL.revokeObjectURL(url); | ||
| document.body.removeChild(a); | ||
| } catch (error) { | ||
| toast.error("Failed to download certificate"); | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior compared to other download implementations.
Two inconsistencies with visitors-table.tsx and dataroom-visitors-table.tsx:
- Line 132: Missing
viewerNamein filename - other files useviewerName || viewerEmail - Missing success toast after download (other files show "NDA certificate downloaded successfully!")
a.href = url;
- a.download = `NDA-Certificate-${row.original.viewerEmail || "Anonymous"}-${Date.now()}.pdf`;
+ a.download = `NDA-Certificate-${row.original.viewerName || row.original.viewerEmail || "Anonymous"}-${Date.now()}.pdf`;
document.body.appendChild(a);
a.click();
window.URL.revokeObjectURL(url);
document.body.removeChild(a);
+ toast.success("NDA certificate downloaded successfully!");
} catch (error) {Note: The View interface at line 61-79 may need a viewerName field added if not already present.
📝 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.
| <button | |
| onClick={async (e) => { | |
| e.stopPropagation(); | |
| try { | |
| const response = await fetch( | |
| `/api/views/${row.original.id}/nda-certificate`, | |
| ); | |
| if (!response.ok) { | |
| throw new Error( | |
| "Failed to generate certificate", | |
| ); | |
| } | |
| const blob = await response.blob(); | |
| const url = window.URL.createObjectURL(blob); | |
| const a = document.createElement("a"); | |
| a.href = url; | |
| a.download = `NDA-Certificate-${row.original.viewerEmail || "Anonymous"}-${Date.now()}.pdf`; | |
| document.body.appendChild(a); | |
| a.click(); | |
| window.URL.revokeObjectURL(url); | |
| document.body.removeChild(a); | |
| } catch (error) { | |
| toast.error("Failed to download certificate"); | |
| } | |
| <button | |
| onClick={async (e) => { | |
| e.stopPropagation(); | |
| try { | |
| const response = await fetch( | |
| `/api/views/${row.original.id}/nda-certificate`, | |
| ); | |
| if (!response.ok) { | |
| throw new Error( | |
| "Failed to generate certificate", | |
| ); | |
| } | |
| const blob = await response.blob(); | |
| const url = window.URL.createObjectURL(blob); | |
| const a = document.createElement("a"); | |
| a.href = url; | |
| a.download = `NDA-Certificate-${row.original.viewerName || row.original.viewerEmail || "Anonymous"}-${Date.now()}.pdf`; | |
| document.body.appendChild(a); | |
| a.click(); | |
| window.URL.revokeObjectURL(url); | |
| document.body.removeChild(a); | |
| toast.success("NDA certificate downloaded successfully!"); | |
| } catch (error) { | |
| toast.error("Failed to download certificate"); | |
| } |
🤖 Prompt for AI Agents
In components/analytics/views-table.tsx around lines 116 to 139, the download
handler is inconsistent with other tables: update the generated filename to
prefer row.original.viewerName and fall back to viewerEmail (i.e. use viewerName
|| viewerEmail) and add a success toast after the download completes (e.g.,
toast.success("NDA certificate downloaded successfully!")); if the View
interface (lines ~61-79) does not include viewerName, add that optional field to
the interface so TypeScript knows the property exists.
| <button | ||
| onClick={async (e) => { | ||
| e.stopPropagation(); | ||
| try { | ||
| const response = await fetch( | ||
| `/api/views/${view.id}/nda-certificate`, | ||
| ); | ||
| if (!response.ok) { | ||
| throw new Error( | ||
| "Failed to generate certificate", | ||
| ); | ||
| } | ||
| const blob = await response.blob(); | ||
| const url = window.URL.createObjectURL( | ||
| blob, | ||
| ); | ||
| const a = document.createElement( | ||
| "a", | ||
| ); | ||
| a.href = url; | ||
| a.download = `NDA-Certificate-${view.viewerName || view.viewerEmail || "Anonymous"}-${Date.now()}.pdf`; | ||
| document.body.appendChild(a); | ||
| a.click(); | ||
| window.URL.revokeObjectURL(url); | ||
| document.body.removeChild(a); | ||
| } catch (error) { | ||
| toast.error( | ||
| "Failed to download certificate", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing success toast for consistency.
Unlike visitors-table.tsx, this implementation doesn't show a success toast after download. Consider adding it for consistent UX.
window.URL.revokeObjectURL(url);
document.body.removeChild(a);
+ toast.success("NDA certificate downloaded successfully!");
} catch (error) {📝 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.
| <button | |
| onClick={async (e) => { | |
| e.stopPropagation(); | |
| try { | |
| const response = await fetch( | |
| `/api/views/${view.id}/nda-certificate`, | |
| ); | |
| if (!response.ok) { | |
| throw new Error( | |
| "Failed to generate certificate", | |
| ); | |
| } | |
| const blob = await response.blob(); | |
| const url = window.URL.createObjectURL( | |
| blob, | |
| ); | |
| const a = document.createElement( | |
| "a", | |
| ); | |
| a.href = url; | |
| a.download = `NDA-Certificate-${view.viewerName || view.viewerEmail || "Anonymous"}-${Date.now()}.pdf`; | |
| document.body.appendChild(a); | |
| a.click(); | |
| window.URL.revokeObjectURL(url); | |
| document.body.removeChild(a); | |
| } catch (error) { | |
| toast.error( | |
| "Failed to download certificate", | |
| ); | |
| } | |
| <button | |
| onClick={async (e) => { | |
| e.stopPropagation(); | |
| try { | |
| const response = await fetch( | |
| `/api/views/${view.id}/nda-certificate`, | |
| ); | |
| if (!response.ok) { | |
| throw new Error( | |
| "Failed to generate certificate", | |
| ); | |
| } | |
| const blob = await response.blob(); | |
| const url = window.URL.createObjectURL( | |
| blob, | |
| ); | |
| const a = document.createElement( | |
| "a", | |
| ); | |
| a.href = url; | |
| a.download = `NDA-Certificate-${view.viewerName || view.viewerEmail || "Anonymous"}-${Date.now()}.pdf`; | |
| document.body.appendChild(a); | |
| a.click(); | |
| window.URL.revokeObjectURL(url); | |
| document.body.removeChild(a); | |
| toast.success("NDA certificate downloaded successfully!"); | |
| } catch (error) { | |
| toast.error( | |
| "Failed to download certificate", | |
| ); | |
| } |
🤖 Prompt for AI Agents
In components/visitors/dataroom-visitors-table.tsx around lines 144 to 173, the
download flow lacks a success toast for consistency with visitors-table.tsx;
after the download is successfully prepared and triggered (i.e., after a.click()
and before/after revoking the object URL), call toast.success with a brief
message like "Certificate downloaded" (optionally include
viewerName/viewerEmail), and ensure the toast utility is imported in this file
if not already; keep the try/catch structure so the success toast only fires on
successful response/blob creation.
| const viewedDate = view.viewedAt | ||
| ? format(new Date(view.viewedAt), "yyyy-MM-dd h:mm a '(UTC)'") | ||
| : "Not available"; |
There was a problem hiding this comment.
Date format label "(UTC)" is misleading.
The format string includes '(UTC)' as a literal suffix, but format() from date-fns doesn't convert the date to UTC—it uses the local timezone. This could display incorrect information.
Either format in UTC or remove the misleading suffix:
- const viewedDate = view.viewedAt
- ? format(new Date(view.viewedAt), "yyyy-MM-dd h:mm a '(UTC)'")
- : "Not available";
+ const viewedDate = view.viewedAt
+ ? format(new Date(view.viewedAt), "yyyy-MM-dd h:mm a")
+ : "Not available";Or use formatInTimeZone from date-fns-tz for actual UTC formatting:
import { formatInTimeZone } from 'date-fns-tz';
// ...
const viewedDate = view.viewedAt
? formatInTimeZone(new Date(view.viewedAt), 'UTC', "yyyy-MM-dd h:mm a '(UTC)'")
: "Not available";🤖 Prompt for AI Agents
In pages/api/views/[viewId]/nda-certificate.ts around lines 537-539, the
formatted viewedDate appends the literal "(UTC)" but uses date-fns format which
formats in local time; either remove the misleading "(UTC)" literal or perform a
true UTC format by switching to formatInTimeZone from date-fns-tz. To fix:
choose one — (A) remove " '(UTC)'" from the format string so it reflects local
time correctly, or (B) import formatInTimeZone from 'date-fns-tz', replace
format(...) with formatInTimeZone(new Date(view.viewedAt), 'UTC', "yyyy-MM-dd
h:mm a '(UTC)'"), and update imports accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pages/api/views/[viewId]/nda-certificate.ts (1)
558-583: “(UTC)” suffix is still misleading; format is in local server time (also forcompletedDate).
date-fns’sformatdoes not convert to UTC; it formats theDatein the server’s local timezone. Appending"(UTC)"to bothviewedDateandcompletedDatetherefore mislabels the time shown on the certificate.Either:
- Drop the literal
"(UTC)"from the format strings so they reflect server/local time, or- Switch to a true UTC formatter (e.g.,
formatInTimeZonefromdate-fns-tz) and actually format in UTC.Also apply the same treatment to
completedDate, not justviewedDate.Does `date-fns` `format()` use the local timezone by default, and is `formatInTimeZone` from `date-fns-tz` the recommended way to format a `Date` in UTC while appending a literal "(UTC)" label?
🧹 Nitpick comments (3)
app/api/views/route.ts (1)
113-128: Based on my verification of the codebase, the review comment identifies valid concerns. Here's what I confirmed:
Role semantics verified: The Prisma schema (prisma/schema/team.prisma) defines
enum Role { ADMIN, MANAGER, MEMBER }, confirming "ADMIN" is the correct discriminator.Null handling confirmed: When no ADMIN user exists or the admin lacks an email,
adminEmailbecomesnull. ThesendSignedNDAEmailfunction does check for this (if (!ownerEmail) { throw new Error(...) }) and catches the error, but the call site wraps it inwaitUntil()for background execution. The error IS logged via thelog()function, but since it's async/background, it may appear silent to the user.Multiple admins issue verified: The code uses
find()to get the first admin with no explicit "primary" admin concept in the schema, making the selection arbitrary.
Admin email fallback needed for robustness in NDA notifications
When a team has no ADMIN user or the admin lacks an email,
adminEmailbecomesnullandsendSignedNDAEmailthrows "Document Owner not found". The error is logged but executes asynchronously viawaitUntil(), so notification failures may go unnoticed. Consider implementing a fallback (e.g., first team user, document owner, or a designated fallback email) to ensure NDA completion notifications reach someone. If multiple admins exist, the arbitraryfind()selection could be replaced with an explicit "primary admin" field to ensure consistent owner assignment.app/api/views-dataroom/route.ts (1)
134-147: Team users selection is correct but might be heavier than needed.Selecting all
team.users(and nesteduser.email) per view works for deriving admin + CCs, but it’s now part of every link fetch, even when NDA notifications are off. If teams can be large, consider narrowing this to just what you need (e.g., only admins, or querying users lazily inside the NDA email block) to avoid unnecessary load on the view creation hot path.Also applies to: 149-153
pages/api/views/[viewId]/nda-certificate.ts (1)
173-375: Agreement PDF creation is generally robust; consider making the contentType handling explicit.The TEXT branch’s simple word‑wrap and multipage logic, and the LINK branch’s attempts to (a) resolve Papermark links, then (b) fall back to direct fetch, then (c) fall back to a minimal reference PDF, give you a nice progressive‑enhancement ladder.
If
Agreement.contentTypemight ever gain new values beyond"TEXT"/"LINK", it’d be safer to make this conditional exhaustive (e.g., aswitchthat throws or falls back explicitly for unknown types) so future schema changes don’t silently end up in the LINK code path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/api/views-dataroom/route.ts(5 hunks)app/api/views/route.ts(4 hunks)pages/api/views/[viewId]/nda-certificate.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pages/api/views/[viewId]/nda-certificate.ts (5)
pages/api/auth/[...nextauth].ts (1)
authOptions(38-194)lib/types.ts (1)
CustomUser(17-17)lib/utils.ts (1)
log(64-124)lib/tinybird/pipes.ts (2)
getViewUserAgent(95-107)getViewUserAgent_v2(79-93)lib/files/get-file.ts (1)
getFile(11-28)
app/api/views/route.ts (2)
lib/utils/geo.ts (1)
LOCALHOST_GEO_DATA(25-32)lib/emails/send-signed-nda.ts (1)
sendSignedNDAEmail(5-62)
⏰ 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 (3)
app/api/views/route.ts (1)
14-29: Based on my verification, I can now provide the final rewritten review comment:
Imports and geo constants verified; no action needed
The imports for
sendSignedNDAEmail,geolocation, andLOCALHOST_GEO_DATA/LOCALHOST_IPare correctly implemented:
LOCALHOST_IPis properly exported fromlib/utils/geo.ts(line 34)geolocation(request)usage is safely guarded: (1) only called whenprocess.env.VERCEL === "1", falling back toLOCALHOST_GEO_DATAotherwise, and (2) properties are accessed only after explicit null checks (geo.city && geo.country)No changes required.
pages/api/views/[viewId]/nda-certificate.ts (2)
35-81: Access control and agreement scoping look correct.You correctly:
- Require an authenticated session.
- Restrict access to users belonging to the view’s
team.- Base the certificate on
agreementResponse.agreement, ensuring you tie it to the actual agreement version the user accepted.This keeps the route appropriately locked down and avoids coupling to any current link agreement settings.
87-103: Good handling of agreement version drift and optional user‑agent/location metadata.Logging when
agreement.updatedAt > agreementResponse.createdAtmakes the known “no snapshot” limitation explicit and debuggable without breaking certificate generation, and the user‑agent fetching logic degrades gracefully to sensible defaults. ThelocationText/deviceTypederivations give useful context while staying robust to missing Tinybird data.Also applies to: 156-168
| // Send NDA completion email if agreement was signed and notifications are enabled | ||
| if ( | ||
| hasConfirmedAgreement && | ||
| link.enableAgreement && | ||
| link.agreementId && | ||
| link.enableNotification && | ||
| link.agreement | ||
| ) { | ||
| waitUntil( | ||
| (async () => { | ||
| try { | ||
| // Get location data | ||
| const geo = | ||
| process.env.VERCEL === "1" | ||
| ? geolocation(request) | ||
| : LOCALHOST_GEO_DATA; | ||
| const locationString = | ||
| geo.city && geo.country | ||
| ? geo.region && geo.region !== geo.city | ||
| ? `${geo.city}, ${geo.region}, ${geo.country}` | ||
| : `${geo.city}, ${geo.country}` | ||
| : undefined; | ||
|
|
||
| // Get team members for CC | ||
| const adminUser = link.team?.users.find( | ||
| (u) => u.role === "ADMIN", | ||
| ); | ||
| const adminEmail = adminUser?.user.email || null; | ||
| const teamMembers = link.team?.users | ||
| .map((u) => u.user.email) | ||
| .filter( | ||
| (email): email is string => | ||
| !!email && email !== adminEmail, | ||
| ) || []; | ||
|
|
||
| // Send NDA completion email | ||
| await sendSignedNDAEmail({ | ||
| ownerEmail: adminEmail, | ||
| viewId: newDataroomView.id, | ||
| dataroomId: dataroomId, | ||
| agreementName: link.agreement?.name || "NDA", | ||
| linkName: link.name || `Link #${linkId.slice(-5)}`, | ||
| viewerEmail: email ?? null, | ||
| viewerName: name ?? null, | ||
| teamMembers: teamMembers.length > 0 ? teamMembers : undefined, | ||
| locationString, | ||
| }); | ||
| } catch (error) { | ||
| log({ | ||
| message: `Failed to send NDA completion email for dataroom view: ${newDataroomView.id}. \n\n ${error}`, | ||
| type: "error", | ||
| }); | ||
| } | ||
| })(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
NDA completion email logic is sound; consider DRYing geo/admin lookup and guarding on missing admin email.
The conditions around hasConfirmedAgreement, enableAgreement, agreementId, enableNotification, and agreement look correct, and running sendSignedNDAEmail via waitUntil keeps the main response non‑blocking. Two follow‑ups:
- The geo lookup +
locationStringconstruction andadminEmail/teamMembersderivation are duplicated across DATAROOM and DOCUMENT views. A small helper (e.g.,buildNdaEmailContext({ link, request, viewId, dataroomId, viewer })) would reduce copy‑paste and keep future changes in sync. - If no
ADMINuser exists,adminEmailwill benulland still passed asownerEmail. IfsendSignedNDAEmailassumes a non‑null owner address, this will just log errors on every call. Either short‑circuit when!adminEmailor makesendSignedNDAEmailexplicitly tolerate a missing owner (e.g., skip sending or fall back to another role).
Given the error handling and waitUntil, this is non‑blocking but worth tightening up.
Also applies to: 895-951
| // Send NDA completion email if agreement was signed and notifications are enabled | ||
| if ( | ||
| hasConfirmedAgreement && | ||
| link.enableAgreement && | ||
| link.agreementId && | ||
| link.enableNotification && | ||
| link.agreement | ||
| ) { | ||
| waitUntil( | ||
| (async () => { | ||
| try { | ||
| // Get location data | ||
| const geo = | ||
| process.env.VERCEL === "1" | ||
| ? geolocation(request) | ||
| : LOCALHOST_GEO_DATA; | ||
| const locationString = | ||
| geo.city && geo.country | ||
| ? geo.region && geo.region !== geo.city | ||
| ? `${geo.city}, ${geo.region}, ${geo.country}` | ||
| : `${geo.city}, ${geo.country}` | ||
| : undefined; | ||
|
|
||
| // Get team members for CC | ||
| const adminUser = link.team?.users.find( | ||
| (u) => u.role === "ADMIN", | ||
| ); | ||
| const adminEmail = adminUser?.user.email || null; | ||
| const teamMembers = link.team?.users | ||
| .map((u) => u.user.email) | ||
| .filter( | ||
| (email): email is string => | ||
| !!email && email !== adminEmail, | ||
| ) || []; | ||
|
|
||
| // Send NDA completion email | ||
| await sendSignedNDAEmail({ | ||
| ownerEmail: adminEmail, | ||
| viewId: newView.id, | ||
| documentId: documentId, | ||
| dataroomId: undefined, // Document view, not dataroom | ||
| agreementName: link.agreement?.name || "NDA", | ||
| linkName: link.name || `Link #${linkId.slice(-5)}`, | ||
| viewerEmail: email ?? null, | ||
| viewerName: name ?? null, | ||
| teamMembers: teamMembers.length > 0 ? teamMembers : undefined, | ||
| locationString, | ||
| }); | ||
| } catch (error) { | ||
| log({ | ||
| message: `Failed to send NDA completion email for view: ${newView.id}. \n\n ${error}`, | ||
| type: "error", | ||
| }); | ||
| } | ||
| })(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Find the sendSignedNDAEmail function definition to verify its return type
rg -n "sendSignedNDAEmail\s*=" -A 15 --type=tsRepository: mfts/papermark
Length of output: 864
🏁 Script executed:
# Search for all usages of sendSignedNDAEmail to understand error handling patterns
rg -n "sendSignedNDAEmail" -B 3 -A 5 --type=tsRepository: mfts/papermark
Length of output: 4161
🏁 Script executed:
# Check how agreementResponse is used/stored in the route
rg -n "agreementResponse" -B 3 -A 3 --type=tsRepository: mfts/papermark
Length of output: 14233
🏁 Script executed:
# Read the full sendSignedNDAEmail implementation
cat lib/emails/send-signed-nda.tsRepository: mfts/papermark
Length of output: 1438
🏁 Script executed:
# Check the sendEmail function to understand error handling
rg -n "export const sendEmail" -A 20 --type=tsRepository: mfts/papermark
Length of output: 4414
🏁 Script executed:
# Look at the context around the NDA email sending in app/api/views/route.ts
sed -n '500,740p' app/api/views/route.ts | head -250Repository: mfts/papermark
Length of output: 8081
Perfect. I've now verified all the claims in the review comment. Let me confirm the findings:
Verification Results:
-
✅
sendSignedNDAEmailreturn type: Confirmed it returns{ success: true, data }or{ success: false, error }(lib/emails/send-signed-nda.ts) -
✅ Silent failures: Confirmed all three call sites (app/api/views/route.ts:715, app/api/views-dataroom/route.ts:744 & 932) do
await sendSignedNDAEmail(...)without capturing the return value, meaningsuccess: falsecases are silently ignored. -
✅ De-duplication via
agreementResponse: Confirmed valid. TheagreementResponseis created during view creation (lines 537-540) before email sending (lines 679-735). It can be gated on: if multiple POSTs are sent withhasConfirmedAgreement=truefor the same viewer/link, multiple views + multiple emails would be created. Gating onnewView.agreementResponseexistence would prevent this.
Both suggestions are valid, actionable, and practical. The review comment is accurate.
NDA completion email block is solid; capture email send failures and consider de‑duplication
The NDA completion email logic (agreement + notifications gate, geo enrichment, admin + CC derivation, and waitUntil wrapping) looks good and non‑blocking. Two refinements you should consider:
-
Log on email send failure:
sendSignedNDAEmailreturns{ success, error }and internally catches errors. Since you're not inspecting the return value, non‑throwing failures will be completely silent. Capturing and logging!result.successwould help observability. -
await sendSignedNDAEmail({
-
const result = await sendSignedNDAEmail({ ownerEmail: adminEmail, viewId: newView.id, documentId: documentId, dataroomId: undefined, // Document view, not dataroom agreementName: link.agreement?.name || "NDA", linkName: link.name || `Link #${linkId.slice(-5)}`, viewerEmail: email ?? null, viewerName: name ?? null, teamMembers: teamMembers.length > 0 ? teamMembers : undefined, locationString,
-
});
-
}); -
if (!result?.success) { -
log({ -
message: `sendSignedNDAEmail failed for view ${newView.id}: ${String( -
result?.error ?? "unknown error", -
)}`, -
type: "error", -
}); -
}
-
De‑duplicate via
agreementResponse: If the frontend sends multiple POSTs withhasConfirmedAgreement=truefor the same viewer/link, you'll create multiple views and send multiple NDA emails. Gate onnewView.agreementResponseexistence instead to ensure one email per agreement response: -
if ( -
hasConfirmedAgreement && -
link.enableAgreement && -
link.agreementId && -
link.enableNotification && -
link.agreement -
) {
-
if ( -
newView.agreementResponse && -
link.enableNotification && -
link.agreement -
) {
🤖 Prompt for AI Agents
In app/api/views/route.ts around lines 679 to 735, the NDA email send is
currently fire-and-forget and ignores the sendSignedNDAEmail return value and
can be invoked multiple times for duplicate POSTs; capture the result of
sendSignedNDAEmail, check if result.success is false and call log(...) with the
returned error details inside the try block (or in a small if after the await)
so non-throwing failures are recorded, and before entering the
waitUntil/email-send block add a guard that only proceeds if
newView.agreementResponse exists (or was just created) to de-duplicate emails
for repeated agreement confirmations.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.