Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 133 additions & 1 deletion app/api/views-dataroom/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
import { verifyDataroomSession } from "@/lib/auth/dataroom-auth";
import { PreviewSession, verifyPreviewSession } from "@/lib/auth/preview-auth";
import { sendOtpVerificationEmail } from "@/lib/emails/send-email-otp-verification";
import { sendSignedNDAEmail } from "@/lib/emails/send-signed-nda";
import { getFile } from "@/lib/files/get-file";
import { newId } from "@/lib/id-helper";
import {
Expand All @@ -29,7 +30,8 @@ import { CustomUser, WatermarkConfigSchema } from "@/lib/types";
import { checkPassword, decryptEncrpytedPassword, log } from "@/lib/utils";
import { extractEmailDomain, isEmailMatched } from "@/lib/utils/email-domain";
import { generateOTP } from "@/lib/utils/generate-otp";
import { LOCALHOST_IP } from "@/lib/utils/geo";
import { geolocation } from "@vercel/functions";
import { LOCALHOST_GEO_DATA, LOCALHOST_IP } from "@/lib/utils/geo";
import { checkGlobalBlockList } from "@/lib/utils/global-block-list";
import { validateEmail } from "@/lib/utils/validate-email";

Expand Down Expand Up @@ -132,6 +134,21 @@ export async function POST(request: NextRequest) {
select: {
plan: true,
globalBlockList: true,
users: {
select: {
role: true,
user: {
select: {
email: true,
},
},
},
},
},
},
agreement: {
select: {
name: true,
},
},
customFields: {
Expand Down Expand Up @@ -687,6 +704,63 @@ export async function POST(request: NextRequest) {
})(),
);
}

// 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",
});
}
})(),
);
}
Comment on lines +708 to +763
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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:

  1. The geo lookup + locationString construction and adminEmail / teamMembers derivation 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.
  2. If no ADMIN user exists, adminEmail will be null and still passed as ownerEmail. If sendSignedNDAEmail assumes a non‑null owner address, this will just log errors on every call. Either short‑circuit when !adminEmail or make sendSignedNDAEmail explicitly 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

}

const dataroomViewId =
Expand Down Expand Up @@ -817,6 +891,64 @@ export async function POST(request: NextRequest) {
})(),
);
}

// Send NDA completion email if agreement was signed and notifications are enabled
if (
newView &&
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 (for document view within dataroom, link to dataroom)
await sendSignedNDAEmail({
ownerEmail: adminEmail,
viewId: newView.id,
dataroomId: dataroomId, // Link to dataroom even for document views
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 document view: ${newView.id}. \n\n ${error}`,
type: "error",
});
}
})(),
);
}
}

// if document version has pages, then return pages
Expand Down
77 changes: 76 additions & 1 deletion app/api/views/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { hashToken } from "@/lib/api/auth/token";
import { verifyPreviewSession } from "@/lib/auth/preview-auth";
import { PreviewSession } from "@/lib/auth/preview-auth";
import { sendOtpVerificationEmail } from "@/lib/emails/send-email-otp-verification";
import { sendSignedNDAEmail } from "@/lib/emails/send-signed-nda";
import { getFeatureFlags } from "@/lib/featureFlags";
import { getFile } from "@/lib/files/get-file";
import { newId } from "@/lib/id-helper";
Expand All @@ -23,7 +24,8 @@ import { CustomUser, WatermarkConfigSchema } from "@/lib/types";
import { checkPassword, decryptEncrpytedPassword, log } from "@/lib/utils";
import { isEmailMatched } from "@/lib/utils/email-domain";
import { generateOTP } from "@/lib/utils/generate-otp";
import { LOCALHOST_IP } from "@/lib/utils/geo";
import { geolocation } from "@vercel/functions";
import { LOCALHOST_GEO_DATA, LOCALHOST_IP } from "@/lib/utils/geo";
import { checkGlobalBlockList } from "@/lib/utils/global-block-list";
import { validateEmail } from "@/lib/utils/validate-email";

Expand Down Expand Up @@ -108,6 +110,21 @@ export async function POST(request: NextRequest) {
select: {
plan: true,
globalBlockList: true,
users: {
select: {
role: true,
user: {
select: {
email: true,
},
},
},
},
},
},
agreement: {
select: {
name: true,
},
},
customFields: {
Expand Down Expand Up @@ -658,6 +675,64 @@ export async function POST(request: NextRequest) {
}),
);
}

// 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",
});
}
})(),
);
}
Comment on lines +679 to +735
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Find the sendSignedNDAEmail function definition to verify its return type
rg -n "sendSignedNDAEmail\s*=" -A 15 --type=ts

Repository: 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=ts

Repository: 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=ts

Repository: mfts/papermark

Length of output: 14233


🏁 Script executed:

# Read the full sendSignedNDAEmail implementation
cat lib/emails/send-signed-nda.ts

Repository: mfts/papermark

Length of output: 1438


🏁 Script executed:

# Check the sendEmail function to understand error handling
rg -n "export const sendEmail" -A 20 --type=ts

Repository: 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 -250

Repository: 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:

  1. sendSignedNDAEmail return type: Confirmed it returns { success: true, data } or { success: false, error } (lib/emails/send-signed-nda.ts)

  2. 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, meaning success: false cases are silently ignored.

  3. De-duplication via agreementResponse: Confirmed valid. The agreementResponse is created during view creation (lines 537-540) before email sending (lines 679-735). It can be gated on: if multiple POSTs are sent with hasConfirmedAgreement=true for the same viewer/link, multiple views + multiple emails would be created. Gating on newView.agreementResponse existence 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: sendSignedNDAEmail returns { 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.success would 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 with hasConfirmedAgreement=true for the same viewer/link, you'll create multiple views and send multiple NDA emails. Gate on newView.agreementResponse existence 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.

}

const returnObject = {
Expand Down
49 changes: 43 additions & 6 deletions components/analytics/views-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
Download,
DownloadCloudIcon,
FileBadgeIcon,
FileTextIcon,
ServerIcon,
ThumbsDownIcon,
ThumbsUpIcon,
Expand Down Expand Up @@ -101,12 +102,48 @@ const columns: ColumnDef<View>[] = [
</BadgeTooltip>
)}
{row.original.agreementResponse && (
<BadgeTooltip
content={`Agreed to ${row.original.agreementResponse.agreement.name}`}
key="agreement"
>
<FileBadgeIcon className="h-4 w-4 text-emerald-500 hover:text-emerald-600" />
</BadgeTooltip>
<>
<BadgeTooltip
content={`Agreed to ${row.original.agreementResponse.agreement.name}`}
key="agreement"
>
<FileBadgeIcon className="h-4 w-4 text-emerald-500 hover:text-emerald-600" />
</BadgeTooltip>
<BadgeTooltip
content="Download NDA Certificate"
key="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.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");
}
Comment on lines +116 to +139
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Inconsistent behavior compared to other download implementations.

Two inconsistencies with visitors-table.tsx and dataroom-visitors-table.tsx:

  1. Line 132: Missing viewerName in filename - other files use viewerName || viewerEmail
  2. 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.

Suggested change
<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.

}}
className="h-4 w-4 text-emerald-500 hover:text-emerald-600 cursor-pointer"
>
<FileTextIcon className="h-4 w-4" />
</button>
</BadgeTooltip>
</>
)}
{row.original.downloadedAt && (
<BadgeTooltip
Expand Down
Loading