feat: link as a file #1950
feat: link as a file #1950
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request introduces support for "link" type documents throughout the codebase. It adds link URL handling in API routes, creates a link preview component, implements web link upload functionality in the modal, adds comprehensive URL validation, and includes analytics tracking for link opening events. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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: 7
🧹 Nitpick comments (11)
pages/api/teams/[teamId]/documents/index.ts (1)
340-348: Simplify redundant ternary expression.The ternary
fileType === "link" ? "VERCEL_BLOB" : "VERCEL_BLOB"always evaluates to"VERCEL_BLOB", making the condition unnecessary.- // For link documents, storageType is optional but processDocument requires it - // Use VERCEL_BLOB as a placeholder (not actually used for links) - const finalStorageType = storageType || (fileType === "link" ? "VERCEL_BLOB" : "VERCEL_BLOB"); + // For link documents, storageType is optional but processDocument requires it + // Use VERCEL_BLOB as default (not actually used for links) + const finalStorageType = storageType || "VERCEL_BLOB";app/api/views-dataroom/route.ts (1)
873-884: LGTM! Consider moving the comment for clarity.The link type handling is correct—link documents bypass
getFile()since they store URLs directly. However, the comment at line 883 would be clearer if placed as a case comment within the if-block or as an else-if for the link type.if ( documentVersion.type === "pdf" || documentVersion.type === "image" || documentVersion.type === "video" ) { documentVersion.file = await getFile({ data: documentVersion.file, type: documentVersion.storageType, }); } - // For link documents, the file is already a URL, no processing needed + // Note: For "link" documents, the file is already a URL - no getFile() call needed + if (documentVersion.type === "sheet") {pages/api/record_link_open.ts (2)
77-77: Click ID generation is predictable.Using
Date.now()andMath.random().toString(36).substr(2, 9)produces semi-predictable IDs. For analytics purposes this is likely acceptable, but if uniqueness is critical, consider using the existingnewIdhelper from@/lib/id-helperfor consistency with other ID generation in the codebase.+import { newId } from "@/lib/id-helper"; + - const clickId = `click_${Date.now()}_${Math.random().toString(36).substr(2, 9)}`; + const clickId = newId("linkOpen");
114-125: Error handling silently swallows failures.While returning success even on Tinybird errors prevents blocking the redirect (as the comment notes), consider tracking the error rate or implementing a retry mechanism for reliability. The current approach may mask persistent issues.
components/view/view-data.tsx (1)
116-173: Significant code duplication withdocument-view.tsx.The
onContinuehandler logic (analytics tracking, popup handling) is nearly identical tohandleLinkContinueindocument-view.tsx(lines 294-359). This duplication creates maintenance burden and risk of divergence.Consider extracting a shared utility function:
// lib/utils/link-tracking.ts export async function trackLinkOpen(params: { linkId: string; documentId: string; viewId: string; versionNumber: number; linkUrl: string; viewerEmail?: string; }): Promise<void> { // ... shared tracking logic } export function openLinkWithFallback(url: string): void { const newWindow = window.open(url, "_blank", "noopener,noreferrer"); if (!newWindow) { window.location.href = url; } }components/view/document-view.tsx (1)
294-300: Redundant fallback logic.The
linkUrlvariable (line 127-129) already includesdocument.filein its fallback chain, so the additional fallback on line 296 is redundant.const handleLinkContinue = async () => { - // Use viewData.file or fallback to document.file - const finalLinkUrl = linkUrl || document.file; - if (!finalLinkUrl) { - console.error("Link URL not available", { linkUrl, documentFile: document.file, viewDataFile: viewData.file }); + if (!linkUrl) { + console.error("Link URL not available", { viewDataFile: viewData.file, documentFile: document.file }); return; }Then use
linkUrldirectly instead offinalLinkUrlthroughout the function.components/documents/add-document-modal.tsx (3)
529-660: Code duplication withhandleNotionUpload.The
handleWebLinkUploadfunction shares significant structure withhandleNotionUpload(lines 384-527), including error handling, dataroom integration, analytics capture, and navigation logic. Consider extracting a shared handler:const handleDocumentCreation = async ( documentPayload: { name: string; url: string; type: string; ... }, successMessage: string, ) => { // Common creation, dataroom, analytics, and navigation logic };This would reduce duplication and ensure consistent behavior across document types.
545-552: Consider enforcing HTTPS for web links on client side.The server-side validation in
url-validation.tsenforces HTTPS for link types, but the client-side validation only checks for a valid URL format. Adding an HTTPS check here provides earlier feedback to users:// Validate URL format with Zod - const urlSchema = z.string().url(); + const urlSchema = z.string().url().refine( + (url) => url.startsWith("https://"), + { message: "Please enter a secure URL starting with https://" } + ); const urlValidation = urlSchema.safeParse(webLink); if (!urlValidation.success) { - toast.error("Please enter a valid URL format."); + toast.error(urlValidation.error.errors[0]?.message || "Please enter a valid URL format."); return; }
824-826: Remove commented-out code.Dead/commented code should be removed to maintain code cleanliness:
- {/* <small className="text-xs text-muted-foreground"> - The page will be captured and converted to a document format. - </small> */}components/view/link-preview.tsx (2)
28-46: Domain extraction logic duplicated.This domain extraction pattern is also present in
add-document-modal.tsx(lines 559-564). Consider extracting to a shared utility:// lib/utils/url-helpers.ts export function extractDomainFromUrl(url: string): string { try { const urlObj = new URL(url); return urlObj.hostname.replace("www.", ""); } catch { const match = url.match(/https?:\/\/([^\/]+)/); if (match) return match[1].replace("www.", ""); return url.length > 50 ? url.substring(0, 50) + "..." : url; } }
66-78: Anchor element accessibility concern.Using
onClickwithe.preventDefault()on an anchor makes it behave like a button but retains anchor semantics. Screen readers will announce it as a link, but it won't navigate traditionally. This is acceptable given the security interstitial pattern, but consider addingrole="button"or using a styled button instead for clearer semantics:- {linkUrl ? ( - <a - href={linkUrl} - target="_blank" - rel="noopener noreferrer" - className="block cursor-pointer break-all text-sm font-medium text-blue-600 hover:text-blue-700 underline transition-colors dark:text-blue-400 dark:hover:text-blue-300" - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - onContinue(); - }} - > - {linkUrl} - </a> + {linkUrl ? ( + <button + type="button" + className="block cursor-pointer break-all text-sm font-medium text-blue-600 hover:text-blue-700 underline transition-colors dark:text-blue-400 dark:hover:text-blue-300" + onClick={onContinue} + > + {linkUrl} + </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/api/views-dataroom/route.ts(2 hunks)app/api/views/route.ts(1 hunks)components/documents/add-document-modal.tsx(3 hunks)components/view/document-view.tsx(4 hunks)components/view/link-preview.tsx(1 hunks)components/view/view-data.tsx(2 hunks)lib/api/documents/process-document.ts(1 hunks)lib/constants.ts(1 hunks)lib/utils/get-file-icon.tsx(2 hunks)lib/zod/url-validation.ts(4 hunks)pages/api/record_link_open.ts(1 hunks)pages/api/teams/[teamId]/documents/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pages/api/teams/[teamId]/documents/index.ts (1)
lib/api/documents/process-document.ts (1)
processDocument(30-302)
components/view/document-view.tsx (2)
components/view/nav.tsx (1)
TNavData(51-69)components/view/link-preview.tsx (1)
LinkPreview(19-97)
components/documents/add-document-modal.tsx (1)
components/document-upload.tsx (1)
DocumentUpload(25-202)
components/view/view-data.tsx (1)
components/view/link-preview.tsx (1)
LinkPreview(19-97)
🔇 Additional comments (7)
lib/utils/get-file-icon.tsx (1)
52-53: LGTM! New link icon case added correctly.The
LinkIconfrom lucide-react doesn't use theisLightprop, which is acceptable since lucide icons don't have light/dark variants like the custom icons. This is consistent with howNotionIconandMailIconare handled.lib/constants.ts (1)
171-184: LGTM!The
"link"type is correctly added toSUPPORTED_DOCUMENT_SIMPLE_TYPES, enabling type-safe handling of link documents throughout the codebase.app/api/views/route.ts (1)
601-610: LGTM! Link type correctly handled in file return logic.The
"link"type is appropriately added to the condition for returning the file directly. Since link documents store a URL in thefilefield (not a storage path), they correctly bypass thegetFile()processing at lines 601-610 and are included in the direct file return at lines 672-673.Also applies to: 667-676
app/api/views-dataroom/route.ts (1)
975-984: LGTM!The
"link"type is correctly added to the file return condition, consistent with the changes inapp/api/views/route.ts.lib/zod/url-validation.ts (2)
261-282: Good security validation for link URLs.The validation correctly:
- Enforces HTTPS protocol
- Validates URL format with the URL constructor
- Applies SSRF protection via
validateUrlSecurityHowever, the error message could be more specific to help users understand what went wrong:
.refine( async (data) => { // For link type, validate URL format and security if (data.type === "link") { if (!data.url.startsWith("https://")) { return false; } try { new URL(data.url); // Validate URL format return validateUrlSecurity(data.url); } catch { return false; } } // For other types, use the file path validator return await filePathSchema.safeParseAsync(data.url).then((r) => r.success); }, { - message: "URL must be a valid HTTPS URL for link type, or a valid file path for other types", + message: "URL must be a valid, secure HTTPS URL that does not target internal resources", path: ["url"], }, )
226-232: storageType optional change is backward compatible.The codebase already handles optional
storageTypewith a default fallback to"VERCEL_BLOB"(seepages/api/teams/[teamId]/documents/index.tslines 340-342). All document creation paths, including non-link documents, properly default to this value whenstorageTypeis not provided. Making it optional in the schema formalizes existing behavior and does not break compatibility.components/documents/add-document-modal.tsx (1)
630-648: Web link versioning is supported but handling is incomplete.Links can have new versions created (the modal opens with
newVersion=truefor link documents; see document-header.tsx line 569 which excludes only "notion" type). However, thehandleWebLinkUploadfunction has a!newVersioncheck at line 630 that only redirects and shows success messaging for new documents, with no handling for thenewVersion=truecase. This differs from file uploads, which properly handle both paths. Either links should be excluded from versioning like notion pages, or the link handler should have proper UX feedback for version updates (currently it silently succeeds without redirect).
| ); | ||
| const [code, setCode] = useState<string | null>(null); | ||
| const [isInvalidCode, setIsInvalidCode] = useState<boolean>(false); | ||
| const [linkOpened, setLinkOpened] = useState<boolean>(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unused state variable linkOpened.
The linkOpened state is set on line 353 but never read anywhere in the component. This appears to be dead code.
- const [linkOpened, setLinkOpened] = useState<boolean>(false);And remove line 353:
- setLinkOpened(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [linkOpened, setLinkOpened] = useState<boolean>(false); |
🤖 Prompt for AI Agents
In components/view/document-view.tsx around line 122, the useState declaration
const [linkOpened, setLinkOpened] = useState<boolean>(false); is unused and
should be removed; also remove the corresponding setLinkOpened call at line 353.
Delete the state variable declaration and the lone setter invocation, then
ensure no other references rely on this state (if needed, replace calls to
setLinkOpened with side-effect-free logic or proper state/prop handling).
| const navData: TNavData = { | ||
| linkId: link.id, | ||
| documentId: document.id, | ||
| viewId: viewData.viewId, | ||
| isPreview: viewData.isPreview || false, | ||
| allowDownload: link.allowDownload || false, | ||
| brand: brand, | ||
| annotationsFeatureEnabled: annotationsEnabled || false, | ||
| }; |
There was a problem hiding this comment.
Incomplete navData construction compared to ViewData component.
The navData object here is missing several properties that are included in the ViewData component's navData (lines 83-104 in view-data.tsx), such as viewerId, isMobile, isDataroom, dataroomId, conversationsEnabled, assistantEnabled, and isTeamMember.
This inconsistency may cause issues if the Nav component or other consumers rely on these properties for link documents.
Consider aligning the navData construction or extracting a shared builder function.
🤖 Prompt for AI Agents
In components/view/document-view.tsx around lines 371-379, the navData object is
missing several properties present in view-data.tsx (viewerId, isMobile,
isDataroom, dataroomId, conversationsEnabled, assistantEnabled, isTeamMember),
which can break consumers expecting the full shape; update navData to include
those fields (sourcing values from the same variables/props/viewData/document
where view-data.tsx gets them) or refactor by extracting a shared
buildNavData(viewData, document, link, brand, flags) helper used by both
components so both produce the identical navData shape.
| export default function LinkPreview({ | ||
| linkUrl, | ||
| linkName, | ||
| brand, | ||
| onContinue, | ||
| navData, | ||
| }: LinkPreviewProps) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unused brand prop.
The brand prop is defined in the interface and passed to the component but never used. Either remove it or implement brand-specific styling.
export default function LinkPreview({
linkUrl,
linkName,
- brand,
onContinue,
navData,
}: LinkPreviewProps) {And update the interface accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/view/link-preview.tsx around lines 19 to 25, the prop "brand" is
declared on the component but never used; remove it from the function signature
and from any places passing it, or if brand-specific styling is required, apply
the prop to the component (e.g., map brand to a CSS class or inline style) and
use it in the returned JSX, and then update the LinkPreviewProps interface to
reflect the change (remove the brand field if unused or keep it with the correct
type if implemented). Ensure all imports/parents that pass brand are updated to
match the new interface.
| /> | ||
| ) : viewData.fileType === "link" ? ( | ||
| <LinkPreview | ||
| linkUrl={viewData.file || document.versions[0]?.file || ""} |
There was a problem hiding this comment.
Empty string fallback may cause issues.
Passing an empty string as linkUrl when both viewData.file and document.versions[0]?.file are undefined will result in the LinkPreview component displaying "Link URL not available" but the button will be enabled (since !"" is truthy for the disabled check).
- linkUrl={viewData.file || document.versions[0]?.file || ""}
+ linkUrl={viewData.file || document.versions[0]?.file || null}Then update the LinkPreview component's prop type to accept string | null and adjust the disabled logic accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In components/view/view-data.tsx around line 118, the current fallback uses an
empty string for linkUrl which causes the LinkPreview to show "Link URL not
available" while the button remains enabled; change the expression to return
null when no file is available (e.g., use viewData.file ??
document.versions[0]?.file ?? null) and update the LinkPreview prop type to
accept string | null, then update its internal disabled logic to check for
falsy/null specifically (e.g., disabled = !linkUrl) so the button is disabled
when linkUrl is null.
| // For link type, validate URL format | ||
| if (type === "link") { | ||
| try { | ||
| new URL(key); | ||
| } catch (error) { | ||
| throw new Error("Invalid URL format for link document."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and examine URL validation patterns
fd url-validation.ts --exec cat -n {} \;Repository: mfts/papermark
Length of output: 15350
🏁 Script executed:
#!/bin/bash
# Search for protocol/HTTPS validation across the codebase
rg -i 'https.*refine|protocol.*restrict|allowed.*protocol' -t ts -t js lib/Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Also check for any URL validation or sanitization in the documents API
rg -n -B 3 -A 5 'documentUploadSchema|link.*schema' lib/zod/ lib/api/documents/Repository: mfts/papermark
Length of output: 1124
🏁 Script executed:
#!/bin/bash
# Check the full context around lines 78-85 in process-document.ts
head -100 lib/api/documents/process-document.ts | tail -40Repository: mfts/papermark
Length of output: 1040
🏁 Script executed:
#!/bin/bash
# Also check if documentUploadSchema is used before reaching this code
rg -n -B 10 'process.*link|type.*link' lib/api/documents/process-document.ts | head -50Repository: mfts/papermark
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Find where process-document.ts is called and how it's invoked
rg -n 'processDocument|process-document' lib/api/ --type ts --type js -B 2 -A 2Repository: mfts/papermark
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Search for documentUploadSchema usage
rg -n 'documentUploadSchema' lib/api/ --type ts --type js -B 3 -A 3Repository: mfts/papermark
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Broader search for documentUploadSchema across entire codebase
rg -n 'documentUploadSchema' --type ts --type js -B 2 -A 2Repository: mfts/papermark
Length of output: 3598
🏁 Script executed:
#!/bin/bash
# Find routes/endpoints that handle document uploads
rg -n 'documents.*POST|upload.*document' app/ pages/ --type ts --type js -B 2 -A 2 2>/dev/null | head -60Repository: mfts/papermark
Length of output: 1835
🏁 Script executed:
#!/bin/bash
# Search for where processDocument is actually called
rg -n 'processDocument' --type ts --type js -B 3 -A 3Repository: mfts/papermark
Length of output: 3102
🏁 Script executed:
#!/bin/bash
# Check the app/(ee)/api/links/[id]/upload/route.ts file for validation
cat -n app/\(ee\)/api/links/\[id\]/upload/route.ts | sed -n '1,120p'Repository: mfts/papermark
Length of output: 4214
🏁 Script executed:
#!/bin/bash
# Get more lines to see validation logic
sed -n '1,150p' 'app/(ee)/api/links/[id]/upload/route.ts'Repository: mfts/papermark
Length of output: 4192
🏁 Script executed:
#!/bin/bash
# Find DocumentData type definition
rg -n 'type DocumentData|interface DocumentData' --type ts -A 10Repository: mfts/papermark
Length of output: 810
Ensure comprehensive URL validation is applied consistently for link documents.
The new URL() check at this location validates format only. While documentUploadSchema in lib/zod/url-validation.ts does enforce HTTPS-only URLs and SSRF protection (lines 264-274), this schema validation is not consistently applied across all code paths:
pages/api/teams/[teamId]/documents/index.tsvalidates withdocumentUploadSchemabefore callingprocessDocumentapp/(ee)/api/links/[id]/upload/route.tscallsprocessDocumentwithout schema validation
For the external upload endpoint, add validation to ensure link-type documents enforce HTTPS and pass validateUrlSecurity() checks, matching the comprehensive validation already defined in the schema.
🤖 Prompt for AI Agents
In lib/api/documents/process-document.ts around lines 78 to 85, the current
check using new URL(key) only validates URL format; replace it with the
comprehensive validation used by documentUploadSchema: import and call
validateUrlSecurity(key) and ensure the parsed URL uses the https: protocol
(throw an error if not). Make sure to catch validation errors and rethrow a
clear error for invalid/unsafe link documents so link-type uploads from the
external endpoint are subject to the same HTTPS and SSRF protections as the
schema.
| export default async function handler( | ||
| req: NextApiRequest, | ||
| res: NextApiResponse, | ||
| ) { | ||
| if (req.method !== "POST") { | ||
| res.status(405).json({ message: "Method Not Allowed" }); | ||
| return; | ||
| } | ||
|
|
||
| const { | ||
| linkId, | ||
| documentId, | ||
| viewId, | ||
| linkUrl, | ||
| viewerEmail, | ||
| } = req.body as { | ||
| linkId: string; | ||
| documentId: string; | ||
| viewId: string; | ||
| linkUrl: string; | ||
| viewerEmail?: string; | ||
| }; | ||
|
|
||
| if (!linkId || !documentId || !viewId || !linkUrl) { | ||
| res.status(400).json({ message: "Missing required fields" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Consider adding rate limiting to prevent analytics abuse.
This endpoint has no authentication or rate limiting, making it susceptible to analytics spam. While this may be intentional for client-side tracking, consider adding IP-based rate limiting similar to other endpoints in the codebase.
+import { ratelimit } from "@/lib/redis";
+import { ipAddress } from "@vercel/functions";
export default async function handler(
req: NextApiRequest,
res: NextApiResponse,
) {
if (req.method !== "POST") {
res.status(405).json({ message: "Method Not Allowed" });
return;
}
+ // Rate limit to prevent abuse
+ const ip = req.headers["x-forwarded-for"] || req.headers["x-real-ip"] || "unknown";
+ const { success } = await ratelimit(100, "1 m").limit(`record-link-open:${ip}`);
+ if (!success) {
+ return res.status(429).json({ message: "Too many requests" });
+ }
+
const {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
pages/api/record_link_open.ts around lines 17-43: this public POST analytics
endpoint lacks any rate limiting, making it vulnerable to spam; add an IP-based
rate limiter at the top of the handler (before processing body) modeled after
other endpoints in the repo: import and initialize the same rate-limiter util
(or Redis-backed limiter used elsewhere), call it with req and res and return
429 when the limit is exceeded, and only proceed to parse/validate the body when
the request is allowed; ensure the limiter key uses the requester IP, set a
sensible short window (e.g. N requests per minute) consistent with other
endpoints, and include tests/handling to gracefully return 429 JSON { message:
"Too Many Requests" } so analytics callers receive a clear response.
| const { | ||
| linkId, | ||
| documentId, | ||
| viewId, | ||
| linkUrl, | ||
| viewerEmail, | ||
| } = req.body as { | ||
| linkId: string; | ||
| documentId: string; | ||
| viewId: string; | ||
| linkUrl: string; | ||
| viewerEmail?: string; | ||
| }; |
There was a problem hiding this comment.
linkUrl is extracted but never used.
The linkUrl field is destructured from the request body and validated as required, but it's not included in the clickData payload sent to Tinybird. Either add it to the payload or remove it from the required fields.
const clickData = {
timestamp: new Date(Date.now()).toISOString(),
click_id: clickId,
view_id: viewId,
link_id: linkId,
document_id: documentId,
dataroom_id: null,
+ link_url: linkUrl, // Add if needed for analytics
continent: continent || "",Or remove from validation if not needed:
- if (!linkId || !documentId || !viewId || !linkUrl) {
+ if (!linkId || !documentId || !viewId) {
res.status(400).json({ message: "Missing required fields" });
return;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pages/api/record_link_open.ts around lines 26–38, the request body
destructures linkUrl but the value is never included in the clickData sent to
Tinybird; either include linkUrl in the payload or remove it from the required
fields. Fix by adding linkUrl to the clickData object that you send to Tinybird
(alongside linkId, documentId, viewId, viewerEmail) so the field is recorded, or
if the URL is not needed remove linkUrl from the destructured type and any
validation so it is not treated as required.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.