Slack/reform #1852
Slack/reform #1852
Conversation
|
@AshishViradiya153 is attempting to deploy a commit to the mftsio Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (33)
Tip CodeRabbit can generate a title for your PR based on the changes.Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the WalkthroughIntroduces Slack integration across backend and UI: OAuth install flow, DB models/migration, client and event templating, team APIs to manage integration and channels, email notification on install, settings UI (connect, disconnect, channel selection, toggles), and background Slack notifications on document views, dataroom access, and downloads. Adds token encryption utilities and nodemailer dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Settings as Settings UI (/settings/slack)
participant API as /api/teams/:teamId/slack/oauth/authorize
participant Slack as Slack OAuth
participant Callback as /api/slack/oauth/callback
participant DB as Prisma (SlackIntegration, Team)
participant Email as sendSlackIntegrationNotification
User->>Settings: Click "Connect Slack"
Settings->>API: GET authorize (teamId)
API->>API: Validate session + team membership
API->>Slack: Build OAuth URL
API-->>Settings: { oauthUrl, state }
Settings->>Slack: Redirect to install URL
Slack->>Callback: Redirect with code,state
Callback->>Callback: Validate session, state, team access
Callback->>Slack: Exchange code for token
Callback->>Slack: team.info, auth.test
Callback->>DB: Create SlackIntegration (encrypt token)
Callback->>Email: Notify team users (best-effort)
Callback-->>User: Redirect /settings/slack?success=1
sequenceDiagram
autonumber
participant API as View/Download APIs
participant Events as slackEventManager.processEvent
participant DB as Prisma (SlackIntegration, config)
participant Templates as createSlackMessage
participant Slack as Slack API (chat.postMessage)
API->>API: Record event (view/access/download)
API->>Events: notify*({...}) (background)
Events->>DB: Fetch SlackIntegration by teamId
alt Integration enabled and event type enabled
Events->>Templates: createSlackMessage(eventData)
alt Message created
loop Enabled channels
Events->>Slack: chat.postMessage (channel, blocks)
Slack-->>Events: ok/error
end
else Unknown/invalid
Events-->>API: Skip
end
else No integration or disabled
Events-->>API: Skip
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pages/api/teams/[teamId]/index.ts (1)
215-239: Fix Promise.all misuse and ensure domain deletions are awaited.
domainPromisesis built viamapbut returnsvoid[](noreturn), so deletions start but aren’t tracked.team.domains && domainPromisespasses a nested array intoPromise.all, which is treated as a plain value and not awaited.- Net effect: domain removals may not finish before team deletion.
Apply:
- // prepare a list of promises to delete domains - let domainPromises: void[] = []; - if (team.domains) { - domainPromises = team.domains.map((domain) => { - removeDomainFromVercelProject(domain.slug); - }); - } + // prepare a list of promises to delete domains + const domainPromises: Promise<void>[] = + team.domains?.map((domain) => + removeDomainFromVercelProject(domain.slug), + ) ?? []; await Promise.all([ - // delete domains, if exists on team - team.domains && domainPromises, + // delete domains, if exists on team + ...domainPromises, // delete subscription, if exists on team team.stripeId && cancelSubscription(team.stripeId, isOldAccount(team.plan)), // delete user from contact book unsubscribe((session.user as CustomUser).email ?? ""), // delete user, if no other teams - userTeams.length === 1 && - prisma.user.delete({ - where: { - id: (session.user as CustomUser).id, - }, - }), + ...(userTeams.length === 1 + ? [ + prisma.user.delete({ + where: { id: (session.user as CustomUser).id }, + }), + ] + : []), // delete team branding from redis redis.del(`brand:logo:${teamId}`), // delete team prisma.team.delete({ where: { id: teamId, }, }), ]);pages/api/links/download/dataroom-document.ts (2)
149-152: Guard against empty permitted document set to prevent runtime crashes.After permission filtering,
downloadDocumentscan be empty; subsequentdownloadDocuments[0]...will throw. Return 403/404 early.Apply this diff after the filtering block:
downloadDocuments = downloadDocuments.filter((doc) => permittedDocumentIds.includes(doc.id), ); } + // Ensure the requested document is still permitted/available + if (!downloadDocuments.length) { + return res.status(403).json({ error: "Error downloading" }); + }Also applies to: 187-193
219-224: Possible “Invalid Date” when viewedAt is null.
new Date(view.viewedAt)can throw on.toLocale*ifviewedAtis null. Fallback tonew Date().Apply this diff:
- date: new Date(view.viewedAt).toLocaleDateString(), + date: (view.viewedAt ? new Date(view.viewedAt) : new Date()).toLocaleDateString(), ipAddress: getIpAddress(req.headers), link: view.link.name, - time: new Date(view.viewedAt).toLocaleTimeString(), + time: (view.viewedAt ? new Date(view.viewedAt) : new Date()).toLocaleTimeString(),pages/api/links/download/index.ts (1)
152-157: Fix potential crash: viewedAt can be null when building watermark viewerData.Guard against null to avoid RangeError from
.toLocale*.Apply this diff:
- date: new Date(view.viewedAt).toLocaleDateString(), + date: (view.viewedAt ? new Date(view.viewedAt) : new Date()).toLocaleDateString(), ipAddress: getIpAddress(req.headers), link: view.link.name, - time: new Date(view.viewedAt).toLocaleTimeString(), + time: (view.viewedAt ? new Date(view.viewedAt) : new Date()).toLocaleTimeString(),app/api/views-dataroom/route.ts (1)
681-688: Duplicate identifier:dataroomViewIdredeclared in the same scope.You destructure
dataroomViewIdfrom the request body earlier, then redeclareconst dataroomViewId = ..., which is a compile-time error. Rename the local variable.- const dataroomViewId = + const computedDataroomViewId = newDataroomView?.id ?? dataroomSession?.viewId ?? undefined; @@ - viewId: dataroomViewId, + viewId: computedDataroomViewId,
🧹 Nitpick comments (61)
components/upload-zone.tsx (4)
270-279: Make extension checks case-insensitive.Handles
.DWG,.DXF,.KML,.KMZ.- if ( - uploadResult.fileName.endsWith(".dwg") || - uploadResult.fileName.endsWith(".dxf") - ) { + const fileNameLower = uploadResult.fileName.toLowerCase(); + if (fileNameLower.endsWith(".dwg") || fileNameLower.endsWith(".dxf")) { supportedFileType = "cad"; - contentType = `image/vnd.${uploadResult.fileName.split(".").pop()}`; + contentType = `image/vnd.${fileNameLower.split(".").pop()}`; } @@ - if ( - uploadResult.fileName.endsWith(".kml") || - uploadResult.fileName.endsWith(".kmz") - ) { + if (fileNameLower.endsWith(".kml") || fileNameLower.endsWith(".kmz")) { supportedFileType = "map"; - contentType = `application/vnd.google-earth.${uploadResult.fileName.endsWith(".kml") ? "kml+xml" : "kmz"}`; + contentType = `application/vnd.google-earth.${fileNameLower.endsWith(".kml") ? "kml+xml" : "kmz"}`; }Also applies to: 283-288
676-676: Trim unused dependency fromgetFilesFromEventhook.
endpointTargetTypeisn’t referenced; avoid needless re-creations.- [folderPathName, endpointTargetType, teamInfo], + [folderPathName, teamInfo],
70-76: BroadendataroomDocumentIdtype foronUploadSuccesspayload.You pass
nullwhen dataroom add fails; reflect that in the type.onUploadSuccess?: ( files: { fileName: string; documentId: string; - dataroomDocumentId: string; + dataroomDocumentId: string | null; }[], ) => void;
699-723: Keep the hint text in sync with accepted types.Paid plan supports .msg (Outlook). Consider adding it to the displayed list.
- : `Only *.pdf, *.pptx, *.docx, *.xlsx, *.xls, *.csv, *.ods, *.ppt, *.odp, *.doc, *.odt, *.dwg, *.dxf, *.png, *.jpg, *.jpeg, *.mp4, *.mov, *.avi, *.webm, *.ogg`} + : `Only *.pdf, *.pptx, *.docx, *.xlsx, *.xls, *.csv, *.ods, *.ppt, *.odp, *.doc, *.odt, *.dwg, *.dxf, *.png, *.jpg, *.jpeg, *.mp4, *.mov, *.avi, *.webm, *.ogg, *.msg`}package.json (1)
106-106: Remove unused nodemailer dependency (package.json: line 106)
nodemailer isn’t imported or referenced anywhere in the codebase; remove it to reduce bundle weight and avoid potential cold-start impact.components/sidebar/app-sidebar.tsx (1)
153-157: Nice addition of Slack to Settings menu.Optional: if Slack is feature-flagged or plan-gated later, mirror the gating used for other items.
pages/api/record_reaction.ts (1)
30-41: Avoid unnecessary include to reduce DB work.The created
reactionisn’t used; including the relatedviewadds extra query cost without benefit here.Apply to inline-remove the include:
- include: { - view: { - select: { - documentId: true, - dataroomId: true, - linkId: true, - viewerEmail: true, - viewerId: true, - teamId: true, - }, - }, - },components/settings/settings-header.tsx (1)
70-74: Slack tab added — consistent with sidebar and breadcrumbs.If you later introduce feature flags, consider
disabled: !features?.slackfor parity with other tabs.components/shared/icons/slack-icon.tsx (1)
8-16: Add accessible title support and hide decorative icon from AT by default.Expose an optional
titleprop and setaria-hiddenwhen no title is provided. This keeps it decorative by default and accessible when a label is supplied.-interface SlackIconProps { - className?: string; - size?: number; -} +interface SlackIconProps { + className?: string; + size?: number; + title?: string; +} -export function SlackIcon({ className = "", size = 24 }: SlackIconProps) { +export function SlackIcon({ className = "", size = 24, title }: SlackIconProps) { return ( <svg width={size} height={size} viewBox="0 0 24 24" fill="none" className={className} + role={title ? "img" : undefined} + aria-hidden={title ? undefined : true} + focusable="false" > + {title ? <title>{title}</title> : null}components/ui/multi-select-v2.tsx (2)
234-236: Hide trigger icon from assistive tech to avoid redundant labels.The icon is purely decorative in the trigger; hide it from AT.
- {triggerIcon || ( - <TagIcon className="!size-4 shrink-0 text-muted-foreground" /> - )} + <span aria-hidden="true"> + {triggerIcon || ( + <TagIcon className="!size-4 shrink-0 text-muted-foreground" /> + )} + </span>
259-260: Typographic choice acknowledged.Switch to “+{n} more” (no space) is fine; just flagging in case of consistency with other counters.
components/emails/slack-integration-notification.tsx (1)
16-24: Remove unused userEmail prop (or use it).
userEmailis accepted but unused. Either remove it from props or incorporate it into the copy.-export default function SlackIntegrationNotification({ - teamName, - userEmail, - settingsUrl, -}: { - teamName: string; - userEmail: string; - settingsUrl: string; -}) { +export default function SlackIntegrationNotification({ + teamName, + settingsUrl, +}: { + teamName: string; + settingsUrl: string; +}) {lib/utils.ts (1)
592-616: Token encryption works; consider minor clarity and future-proofing.
- Error messages mention only
NEXT_PRIVATE_SLACK_TOKEN_KEYthough a fallback key is allowed; adjust for accuracy.- Add a small prefix marker (e.g.,
slk:) to reduce false positives when checking “already encrypted”.- const encryptionKey = process.env.NEXT_PRIVATE_SLACK_TOKEN_KEY || process.env.NEXT_PRIVATE_DOCUMENT_PASSWORD_KEY; + const encryptionKey = + process.env.NEXT_PRIVATE_SLACK_TOKEN_KEY || + process.env.NEXT_PRIVATE_DOCUMENT_PASSWORD_KEY; if (!encryptionKey) { - throw new Error("NEXT_PRIVATE_SLACK_TOKEN_KEY environment variable is required for token encryption"); + throw new Error( + "Missing encryption key: set NEXT_PRIVATE_SLACK_TOKEN_KEY (preferred) or NEXT_PRIVATE_DOCUMENT_PASSWORD_KEY.", + ); } @@ - const textParts: string[] = token.split(":"); - if (textParts.length === 2) { + const textParts: string[] = token.split(":"); + if (textParts.length === 2) { return token; }prisma/schema/schema.prisma (1)
71-81: Brand model looks good; consider color validation upstreamOne-per-team via teamId @unique is correct. Minor: enforce hex/rgb via app-layer schema (Zod/Yup) to prevent invalid brandColor/accentColor.
lib/slack/types.ts (3)
44-51: Make channel optional to match existing usageExisting code builds messages without setting channel when a default is applied downstream. Keep the property optional to avoid forcing callers to pass it prematurely.
-export interface SlackMessage { - channel: string; +export interface SlackMessage { + channel?: string; text?: string; blocks?: any[]; thread_ts?: string; unfurl_links?: boolean; unfurl_media?: boolean; }
53-57: Unify notificationTypes representationTop-level config uses a keyed object (SlackNotificationConfig), while per-channel uses string[]. Prefer a single event type definition (union) used everywhere to reduce mapping/validation complexity.
+export type SlackEventType = "document_view" | "dataroom_access" | "document_download"; export interface SlackNotificationConfig { - document_view: boolean; - dataroom_access: boolean; - document_download: boolean; + [k in SlackEventType]: boolean; }And adjust SlackChannelConfig below (see next comment).
59-64: Align SlackChannelConfig.notificationTypes to typed eventsUse the union to ensure only supported events are configured.
export interface SlackChannelConfig { id: string; name: string; enabled: boolean; - notificationTypes: string[]; + notificationTypes: SlackEventType[]; }app/api/views/route.ts (1)
640-652: Gate Slack notifications and keep them non-blocking
- Respect link-level notification toggle if applicable.
- You're already using waitUntil; keep it but avoid firing when notifications are disabled.
- if (!isPreview) { + if (!isPreview && link.enableNotification) { waitUntil( notifyDocumentView({ teamId: link.teamId!, documentId, linkId, viewerEmail: email ?? undefined, viewerId: viewer?.id ?? undefined, }).catch((error) => { console.error("Error sending Slack notification:", error); }) ); }Question: Is PII (viewerEmail) intended to be sent to Slack by default, or should this be controlled via a privacy flag in SlackIntegration settings?
pages/api/links/download/dataroom-folder.ts (1)
299-317: Avoid blocking the download path with Slack notificationThis awaits Slack dispatch on the hot path. Fire-and-forget to reduce latency, and optionally gate by a link/team-level flag.
- if (view.link.teamId) { - try { - await notifyDocumentDownload({ + if (view.link.teamId) { + try { + // fire-and-forget; log failures + // eslint-disable-next-line @typescript-eslint/no-floating-promises + notifyDocumentDownload({ teamId: view.link.teamId, documentId: undefined, dataroomId, linkId, viewerEmail: view.viewerEmail ?? undefined, viewerId: undefined, metadata: { folderName: rootFolder.name, documentCount: allDocuments.length, isFolderDownload: true, }, - }); + }).catch((error) => { + console.error("Error sending Slack notification:", error); + }); } catch (error) { console.error("Error sending Slack notification:", error); } }Additionally (outside this hunk), to gate and enrich:
- Include enableNotification in view.link select and guard the send.
- Select and pass viewerId if available.
Example changes elsewhere in this file:
// in the view select - link: { + link: { select: { teamId: true, allowDownload: true, + enableNotification: true, expiresAt: true, isArchived: true, enableWatermark: true, watermarkConfig: true, name: true, permissionGroupId: true, }, }, + viewerId: true,Then conditionally send:
if (view.link.teamId && view.link.enableNotification) { /* send */ }pages/api/links/download/dataroom-document.ts (1)
170-186: Fire-and-forget Slack notify + minimize PII exposure.
- Consider not awaiting the event to avoid adding tail latency to downloads.
- Confirm product/legal stance before sending
viewerEmailinto Slack.Apply this diff to make it non-blocking while preserving error logging:
- try { - await notifyDocumentDownload({ + try { + void notifyDocumentDownload({ teamId: view.link.teamId, documentId, dataroomId: view.dataroom.id, linkId, viewerEmail: view.viewerEmail ?? undefined, viewerId: view.viewerId ?? undefined, - }); + }).catch((error) => + console.error("Error sending Slack notification:", error), + ); } catch (error) { console.error("Error sending Slack notification:", error); }pages/api/links/download/bulk.ts (2)
306-323: Include viewerId (if available) and consider fire-and-forget; validate optional documentId.
- Pass
viewerIdto keep parity with single-document flow.- Consider non-blocking notify to avoid delaying bulk ZIP generation.
- Verify
documentIdis optional inSlackEventData.Apply these diffs:
@@ - await notifyDocumentDownload({ + void notifyDocumentDownload({ teamId: view.dataroom.teamId, - documentId: undefined, // Bulk download, no specific document + documentId: undefined, // Bulk download, no specific document dataroomId: view.dataroom.id, linkId, viewerEmail: view.viewerEmail ?? undefined, - viewerId: undefined, + viewerId: view.viewerId ?? undefined, metadata: { documentCount: downloadDocuments.length, isBulkDownload: true, }, - }); + }).catch((error) => + console.error("Error sending Slack notification:", error), + );And select
viewerIdfrom theview:select: { id: true, viewedAt: true, viewerEmail: true, + viewerId: true,
243-245: Avoid enqueueing empty file keys for ZIP generation.When adding empty folders (
addFileToStructure(folder.path, "", "")),fileKeyis""and gets pushed, potentially breaking the Lambda.Apply this diff:
- fileKeys.push(fileKey); + if (fileKey) { + fileKeys.push(fileKey); + }pages/api/links/download/index.ts (1)
104-117: Add viewerId and consider non-blocking notify; confirm PII policy.
- Include
viewerId(align with dataroom-document flow).- Fire-and-forget to reduce latency.
- Ensure sending
viewerEmailto Slack is intended.Apply these diffs:
@@ - await prisma.view.update({ + await prisma.view.update({ where: { id: viewId }, data: { downloadedAt: new Date() }, }); - if (view.document?.teamId) { + if (view.document?.teamId) { try { - await notifyDocumentDownload({ + void notifyDocumentDownload({ teamId: view.document.teamId, documentId: view.document.id, dataroomId: undefined, linkId, viewerEmail: view.viewerEmail ?? undefined, - viewerId: undefined, - }); + viewerId: view.viewerId ?? undefined, + }).catch((error) => + console.error("Error sending Slack notification:", error), + ); } catch (error) { console.error("Error sending Slack notification:", error); } }And select
viewerIdfrom the view:select: { id: true, viewedAt: true, viewerEmail: true, + viewerId: true,lib/swr/use-slack-integration.ts (1)
25-31: Minor naming nit: returnisLoadingto mirror SWR, or documentloading.Not required, but aligning with SWR’s
isLoadingcan reduce confusion across hooks.- loading: isLoading && !data, + isLoading: isLoading && !data,components/settings/slack-settings-skeleton.tsx (1)
5-5: Add basic a11y attributes to the loading container.Helps assistive tech announce loading state.
- <div className="space-y-6"> + <div className="space-y-6" role="status" aria-busy aria-live="polite" aria-label="Loading Slack settings">lib/emails/send-slack-integration-notification.ts (1)
23-23: Gate real sends across non-production envs, not only NODE_ENV=developmentAvoid sending real emails on Preview/Stage (e.g., Vercel preview).
- test: process.env.NODE_ENV === "development", + test: (process.env.VERCEL_ENV ?? process.env.NODE_ENV) !== "production",lib/swr/use-slack-channels.ts (1)
16-22: Reduce noisy retries and flickerOptional: don’t retry on 4xx and keep previous data during refreshes.
{ revalidateOnFocus: false, revalidateOnReconnect: false, dedupingInterval: 30000, revalidateIfStale: false, errorRetryCount: 2, errorRetryInterval: 5000, + shouldRetryOnError: (err) => !err?.status || err.status >= 500, + keepPreviousData: true, }pages/api/teams/[teamId]/slack/oauth/authorize.ts (2)
51-53: Make redirectUri robust when NEXTAUTH_URL is unsetFallback to request host/proto to avoid broken OAuth in local/preview environments.
- const redirectUri = `${process.env.NEXTAUTH_URL}/api/slack/oauth/callback`; + const proto = (req.headers["x-forwarded-proto"] as string) || "https"; + const host = req.headers.host; + const baseUrl = process.env.NEXTAUTH_URL ?? `${proto}://${host}`; + const redirectUri = `${baseUrl}/api/slack/oauth/callback`;
49-49: Optionally sign thestateto prevent tamperingBase64-encoded JSON is easy to forge. Add an HMAC and verify it in the callback.
Example (outside this hunk):
import crypto from "crypto"; const hmac = (s:string) => crypto.createHmac("sha256", process.env.STATE_SECRET!).update(s).digest("hex"); // here: const payload = JSON.stringify({ teamId, userId, ts: Date.now() }); const state = Buffer.from(payload).toString('base64'); const sig = hmac(state); // send both: { oauthUrl, state, sig } // in callback: recompute hmac(state) === sig and check ts freshnesspages/api/teams/[teamId]/slack/channels.ts (2)
85-98: Return 404 when integration is missing on update (handle Prisma P2025).If the team has no Slack integration,
updatethrows and you return 500. Surface a 404 instead.- await prisma.slackIntegration.update({ + try { + await prisma.slackIntegration.update({ where: { teamId }, data: { enabledChannels }, - }); + }); + } catch (e: any) { + if (e?.code === 'P2025') { + return res.status(404).json({ error: 'Slack integration not found' }); + } + throw e; + }
57-70: Avoid masking Slack API failures with 200 + empty list.Returning
{ channels: [] }on unexpected Slack errors hides issues and complicates debugging. Prefer a 502/500 with a user-friendly message.- return res.status(200).json({ channels: [] }); + return res.status(502).json({ error: 'Failed to fetch channels from Slack. Please try again.' });app/api/views-dataroom/route.ts (1)
666-676: Avoid noisy error logs; add concise context.Logging raw errors repeatedly can be noisy. Add minimal context and avoid duplicate stack traces (the event manager may already log).
- } catch (error) { - console.error("Error sending Slack notification:", error); + } catch (error) { + console.error("Slack notification error (dataroom_access)", { linkId, dataroomId, teamId: link.teamId, error: (error as Error).message });and similarly for the document_view block.
Also applies to: 795-805
pages/api/slack/oauth/callback.ts (3)
126-149: Don’t block the OAuth redirect on email sends.Awaiting
Promise.allSettled(emailPromises)can slow or time out the OAuth flow for large teams. Fire-and-forget (with logging), or queue these emails out-of-band.- await Promise.allSettled(emailPromises) - .then(results => { - const rejected = results.filter(r => r.status === 'rejected'); - if (rejected.length) { - console.error('Some Slack integration notification emails failed:', rejected); - } - }); + // Fire-and-forget to keep OAuth redirect snappy + void Promise.allSettled(emailPromises).then(results => { + const rejected = results.filter(r => r.status === 'rejected'); + if (rejected.length) { + console.error('Some Slack integration notification emails failed:', rejected); + } + }).catch((e) => { + console.error('Slack integration notification fanout failed:', e); + });
24-35: Coerce query params to strings to avoidstring[]edge cases.Next.js can provide
string | string[]. Coerce defensively before use.- const { code, state, error } = req.query as { code: string; state: string; error?: string }; + const qp = req.query as { code?: string | string[]; state?: string | string[]; error?: string | string[] }; + const code = Array.isArray(qp.code) ? qp.code[0] : qp.code; + const state = Array.isArray(qp.state) ? qp.state[0] : qp.state; + const error = Array.isArray(qp.error) ? qp.error[0] : qp.error;
66-74: Handle race on duplicate installations (unique constraint).Simultaneous installs can slip past the existence check and hit a unique constraint on create. Catch Prisma P2002 and redirect gracefully.
Would you like me to add a guarded create with a try/catch for P2002 (and import
Prismato checkinstanceof Prisma.PrismaClientKnownRequestError)?Also applies to: 84-103
components/ui/alert-dialog.tsx (1)
176-177: Trim extra whitespace inside title element.Minor readability/spacing nit.
- <AlertDialogTitle> {title} </AlertDialogTitle> + <AlertDialogTitle>{title}</AlertDialogTitle>prisma/migrations/20250904040013_slack_integration/migration.sql (3)
21-26: Remove redundant non-unique index on teamId.The unique index on teamId already provides an index. Keeping both adds write overhead without benefit.
--- CreateIndex -CREATE INDEX "SlackIntegration_teamId_idx" ON "SlackIntegration"("teamId");
12-16: Add safe defaults for JSONB columns.Defaults simplify inserts and reduce chances of NULL/shape errors.
- "enabledChannels" JSONB NOT NULL, + "enabledChannels" JSONB NOT NULL DEFAULT '{}'::jsonb, "enabled" BOOLEAN NOT NULL DEFAULT true, - "notificationTypes" JSONB NOT NULL, + "notificationTypes" JSONB NOT NULL DEFAULT '{"document_view":true,"dataroom_access":true,"document_download":true}'::jsonb,
8-8: Confirm token-at-rest strategy.accessToken is sensitive. Ensure application-level encryption is enforced and KMS rotation is documented; consider renaming to accessTokenEnc to signal ciphertext.
pages/settings/slack.tsx (4)
221-226: Avoid console noise in production.Gate performance logs or remove.
- console.log(`Channel update API call took ${endTime - startTime}ms`); + if (process.env.NODE_ENV !== "production") { + console.log(`Channel update API call took ${endTime - startTime}ms`); + }
277-282: Function name is misleading; no actual debouncing.Either implement debouncing or rename to reflect behavior.
Example (basic debounce without dependencies creep):
-const debouncedChannelsUpdate = (selectedChannelIds: string[]) => { - setPendingChannelUpdate(true); - handleChannelsUpdate(selectedChannelIds).finally(() => { - setPendingChannelUpdate(false); - }); -}; +const debouncedChannelsUpdate = (() => { + let t: ReturnType<typeof setTimeout> | null = null; + return (selectedChannelIds: string[]) => { + setPendingChannelUpdate(true); + if (t) clearTimeout(t); + t = setTimeout(() => { + handleChannelsUpdate(selectedChannelIds).finally(() => { + setPendingChannelUpdate(false); + }); + }, 300); + }; +})();
94-97: Remove unused helper.handleIntegrationUpdate is defined but never used.
- const handleIntegrationUpdate = (updatedIntegration: SlackIntegration) => { - mutateIntegration(updatedIntegration, false); - };
500-520: Reflect pending updates in control state.Optional: pass pendingChannelUpdate to loading for better UX feedback while saving.
- <MultiSelect - loading={false} + <MultiSelect + loading={pendingChannelUpdate}lib/types/slack.ts (4)
1-6: Avoid type name collision with lib/slack/types.tsThere’s another SlackChannel declared in lib/slack/types.ts with a different shape (adds is_member). Having two interfaces with the same name but different fields is a footgun for accidental imports. Prefer a distinct public API name (e.g., ApiSlackChannel) or re-export the shared type from a single module.
Apply:
-export interface SlackChannel { +export interface ApiSlackChannel { id: string; name: string; is_archived: boolean; is_private: boolean; }And update references in this file (and consumers) accordingly.
15-32: Align server vs API shapes and reuse types
- createdAt/updatedAt as string is fine for API responses, but ensure server-side uses Date and converts explicitly at the boundary.
- enabledChannels: Record<string, SlackChannelConfig> is good, but consider typing the key as channel ID: Record<ApiSlackChannel['id'], SlackChannelConfig>.
Would you like a follow-up patch to add zod/io-ts schemas to validate API payloads?
34-40: Reuse the union for request typingTie UpdateSlackIntegrationRequest.notificationTypes and enabledChannels to the new union to prevent invalid values.
Apply:
export interface UpdateSlackIntegrationRequest { enabled?: boolean; - notificationTypes?: SlackIntegration['notificationTypes']; + notificationTypes?: Record<SlackNotificationType, boolean>; defaultChannel?: string; - enabledChannels?: Record<string, SlackChannelConfig>; + enabledChannels?: Record<string, SlackChannelConfig>; }
42-47: Minor: Export consistent API response typesIf you rename SlackChannel to ApiSlackChannel above, reflect it here to avoid drift.
Apply:
export interface SlackChannelsResponse { - channels: SlackChannel[]; + channels: ApiSlackChannel[]; } export interface SlackIntegrationResponse extends SlackIntegration { }lib/slack/client.ts (1)
166-194: Handle rate limiting and join-on-demand
- Consider handling HTTP 429 (Retry-After) with backoff.
- For errors like not_in_channel, optionally call conversations.join before retrying.
I can provide a small Slack API wrapper with retry/backoff if you want it in this PR.
lib/slack/events.ts (7)
2-3: Import and reuse the client singletonAvoid multiple SlackClient instances. Import the exported singleton.
Apply:
-import { SlackClient } from "./client"; +import { slackClient, SlackClient } from "./client";
21-23: Use singleton instead of new SlackClient()This removes redundant env checks and centralizes HTTP behavior.
Apply:
- constructor() { - this.client = new SlackClient(); - } + constructor() { + this.client = slackClient; + }
6-16: Narrow eventType to a unionUse a union type to prevent invalid event types at compile-time.
Apply (after adding SlackNotificationType in lib/types/slack.ts):
-export interface SlackEventData { +export interface SlackEventData { teamId: string; - eventType: string; + eventType: import('@/lib/types/slack').SlackNotificationType; documentId?: string; dataroomId?: string; linkId?: string; viewerEmail?: string; viewerId?: string; userId?: string; metadata?: Record<string, any>; }
61-74: Avoid rebuilding the same message per channelcreateSlackMessage doesn’t vary by channel; build once and reuse inside the loop.
Apply:
- for (const channel of channels) { - try { - const message = await createSlackMessage(eventData); - if (message) { - const slackMessage = { - ...message, - channel: channel.id - }; - await this.client.sendMessage(integration.accessToken, slackMessage); - } - } catch (channelError) { + const baseMessage = await createSlackMessage(eventData); + if (!baseMessage) return; + for (const channel of channels) { + try { + const slackMessage = { ...baseMessage, channel: channel.id }; + await this.client.sendMessage(integration.accessToken, slackMessage); + } catch (channelError) { console.error(`Error sending to channel ${channel.name || channel.id}:`, channelError); } }
89-95: Type the integration parameter; avoid anyGive the compiler help to catch typos in notification type keys.
Apply:
- private isEventTypeEnabled( - eventType: string, - integration: any - ): boolean { + private isEventTypeEnabled( + eventType: string, + integration: { notificationTypes?: Record<string, boolean> } + ): boolean { const notificationTypes = integration.notificationTypes || {}; return notificationTypes[eventType] || false; }
97-105: Return typed channel configsTie channel configs to a concrete type from your public types module to prevent shape drift.
Apply:
- private async getNotificationChannels( + private async getNotificationChannels( eventData: SlackEventData, integration: SlackIntegration - ): Promise<any[]> { + ): Promise<import('@/lib/types/slack').SlackChannelConfig[]> { const enabledChannels = integration.enabledChannels || {}; return Object.values(enabledChannels) - .filter((channel: any) => channel.enabled) - .filter((channel: any) => channel.notificationTypes && channel.notificationTypes.includes(eventData.eventType)); + .filter((channel: any) => channel.enabled) + .filter((channel: any) => channel.notificationTypes?.includes(eventData.eventType as any)); }
56-58: Optional: fallback to defaultChannelIf no per-channel config matches, consider falling back to integration.defaultChannel when set.
I can draft a small change that builds a single-target list from defaultChannel if you want it here.
lib/slack/templates.ts (6)
45-47: Fetch related entities in parallel to reduce latency.Three sequential DB calls are unnecessary and slow under load.
@@ - const document = eventData.documentId ? await getDocumentInfo(eventData.documentId) : null; - const dataroom = eventData.dataroomId ? await getDataroomInfo(eventData.dataroomId) : null; - const link = eventData.linkId ? await getLinkInfo(eventData.linkId) : null; + const [document, dataroom, link] = await Promise.all([ + eventData.documentId ? getDocumentInfo(eventData.documentId) : Promise.resolve(null), + eventData.dataroomId ? getDataroomInfo(eventData.dataroomId) : Promise.resolve(null), + eventData.linkId ? getLinkInfo(eventData.linkId) : Promise.resolve(null), + ]); @@ - const dataroom = eventData.dataroomId ? await getDataroomInfo(eventData.dataroomId) : null; - const link = eventData.linkId ? await getLinkInfo(eventData.linkId) : null; + const [dataroom, link] = await Promise.all([ + eventData.dataroomId ? getDataroomInfo(eventData.dataroomId) : Promise.resolve(null), + eventData.linkId ? getLinkInfo(eventData.linkId) : Promise.resolve(null), + ]); @@ - const document = eventData.documentId ? await getDocumentInfo(eventData.documentId) : null; - const dataroom = eventData.dataroomId ? await getDataroomInfo(eventData.dataroomId) : null; - const link = eventData.linkId ? await getLinkInfo(eventData.linkId) : null; + const [document, dataroom, link] = await Promise.all([ + eventData.documentId ? getDocumentInfo(eventData.documentId) : Promise.resolve(null), + eventData.dataroomId ? getDataroomInfo(eventData.dataroomId) : Promise.resolve(null), + eventData.linkId ? getLinkInfo(eventData.linkId) : Promise.resolve(null), + ]);Also applies to: 140-142, 235-237
97-99: Prefer event timestamp over “now” for message time.If the pipeline is delayed/retried,
new Date()is misleading.@@ + const ts = eventData.metadata?.timestamp ? new Date(eventData.metadata.timestamp) : new Date(); @@ - text: `*Time:*\n${new Date().toLocaleString()}` + text: `*Time:*\n${ts.toLocaleString()}` @@ + const ts = eventData.metadata?.timestamp ? new Date(eventData.metadata.timestamp) : new Date(); @@ - text: `*Time:*\n${new Date().toLocaleString()}` + text: `*Time:*\n${ts.toLocaleString()}` @@ + const ts = eventData.metadata?.timestamp ? new Date(eventData.metadata.timestamp) : new Date(); @@ - text: `*Time:*\n${new Date().toLocaleString()}` + text: `*Time:*\n${ts.toLocaleString()}`Also applies to: 184-185, 304-305
11-18: Type Slack blocks with the official SDK types.Avoid
any[]and catch shape errors at compile time.+import type { KnownBlock } from "@slack/web-api"; @@ -export interface SlackMessage { +export interface SlackMessage { channel?: string; text?: string; - blocks?: any[]; + blocks?: KnownBlock[]; thread_ts?: string; unfurl_links?: boolean; unfurl_media?: boolean; }
356-371: Consider scoping lookups by team/workspace to prevent cross-tenant leakage.If models carry
teamId/workspaceId, add it to thewhereclause.Would you like a follow-up patch once you confirm the schema field name?
Also applies to: 375-397, 400-414
126-129: Guard against missing NEXTAUTH_URL to avoid broken Slack buttons.If the env var is unset, URLs will start with “undefined…”.
Minimal guard example:
- url: eventData.documentId - ? `${process.env.NEXTAUTH_URL}/documents/${eventData.documentId}` - : `${process.env.NEXTAUTH_URL}/dashboard` + url: (() => { + const base = process.env.NEXTAUTH_URL; + if (!base) return "https://papermark.app/dashboard"; + return eventData.documentId ? `${base}/documents/${eventData.documentId}` : `${base}/dashboard`; + })() @@ - url: eventData.dataroomId - ? `${process.env.NEXTAUTH_URL}/datarooms/${eventData.dataroomId}` - : `${process.env.NEXTAUTH_URL}/dashboard` + url: (() => { + const base = process.env.NEXTAUTH_URL; + if (!base) return "https://papermark.app/dashboard"; + return eventData.dataroomId ? `${base}/datarooms/${eventData.dataroomId}` : `${base}/dashboard`; + })() @@ - url: eventData.dataroomId - ? `${process.env.NEXTAUTH_URL}/datarooms/${eventData.dataroomId}` - : eventData.documentId - ? `${process.env.NEXTAUTH_URL}/documents/${eventData.documentId}` - : `${process.env.NEXTAUTH_URL}/dashboard` + url: (() => { + const base = process.env.NEXTAUTH_URL; + if (!base) return "https://papermark.app/dashboard"; + if (eventData.dataroomId) return `${base}/datarooms/${eventData.dataroomId}`; + if (eventData.documentId) return `${base}/documents/${eventData.documentId}`; + return `${base}/dashboard`; + })()Also applies to: 219-222, 333-339
49-51: DRY up viewer/link formatting.The viewer mention and link label logic is duplicated.
I can factor tiny helpers like:
const mention = (email?: string) => email ? `<mailto:${email}|${email}>` : "Anonymous"; const linkLabelOf = (link?: { id?: string; name?: string } | null) => link?.name ?? (link?.id ? `Link ${link.id.slice(0, 5)}` : null);Also applies to: 143-145, 239-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
app/api/views-dataroom/route.ts(4 hunks)app/api/views/route.ts(2 hunks)components/emails/slack-integration-notification.tsx(1 hunks)components/layouts/breadcrumb.tsx(1 hunks)components/settings/settings-header.tsx(1 hunks)components/settings/slack-settings-skeleton.tsx(1 hunks)components/shared/icons/slack-icon.tsx(1 hunks)components/sidebar/app-sidebar.tsx(1 hunks)components/ui/alert-dialog.tsx(2 hunks)components/ui/multi-select-v2.tsx(4 hunks)components/upload-zone.tsx(1 hunks)lib/emails/send-slack-integration-notification.ts(1 hunks)lib/slack/client.ts(1 hunks)lib/slack/events.ts(1 hunks)lib/slack/templates.ts(1 hunks)lib/slack/types.ts(1 hunks)lib/swr/use-slack-channels.ts(1 hunks)lib/swr/use-slack-integration.ts(1 hunks)lib/types/slack.ts(1 hunks)lib/utils.ts(1 hunks)package.json(2 hunks)pages/api/links/download/bulk.ts(5 hunks)pages/api/links/download/dataroom-document.ts(3 hunks)pages/api/links/download/dataroom-folder.ts(2 hunks)pages/api/links/download/index.ts(3 hunks)pages/api/record_reaction.ts(1 hunks)pages/api/slack/oauth/callback.ts(1 hunks)pages/api/teams/[teamId]/datarooms/[id]/documents/index.ts(0 hunks)pages/api/teams/[teamId]/index.ts(2 hunks)pages/api/teams/[teamId]/slack/channels.ts(1 hunks)pages/api/teams/[teamId]/slack/index.ts(1 hunks)pages/api/teams/[teamId]/slack/oauth/authorize.ts(1 hunks)pages/settings/slack.tsx(1 hunks)prisma/migrations/20250904040013_slack_integration/migration.sql(1 hunks)prisma/schema/schema.prisma(2 hunks)prisma/schema/team.prisma(1 hunks)
💤 Files with no reviewable changes (1)
- pages/api/teams/[teamId]/datarooms/[id]/documents/index.ts
🧰 Additional context used
🧬 Code graph analysis (22)
lib/slack/types.ts (2)
lib/types/slack.ts (2)
SlackChannel(1-6)SlackChannelConfig(8-13)lib/slack/templates.ts (2)
SlackMessage(11-18)SlackChannelConfig(4-9)
lib/slack/client.ts (2)
lib/utils.ts (1)
decryptSlackToken(619-645)lib/slack/types.ts (4)
SlackOAuthResponse(22-42)SlackWorkspaceInfo(1-6)SlackChannel(14-20)SlackMessage(44-51)
lib/emails/send-slack-integration-notification.ts (2)
lib/resend.ts (1)
sendEmail(12-95)components/emails/slack-integration-notification.tsx (1)
SlackIntegrationNotification(16-84)
lib/swr/use-slack-channels.ts (2)
lib/types/slack.ts (1)
SlackChannelsResponse(43-45)lib/utils.ts (1)
fetcher(48-62)
pages/api/teams/[teamId]/slack/oauth/authorize.ts (6)
pages/api/slack/oauth/callback.ts (1)
handler(11-165)pages/api/teams/[teamId]/slack/index.ts (1)
handler(8-43)pages/api/teams/[teamId]/slack/channels.ts (1)
handler(8-102)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/slack/client.ts (1)
slackClient(198-198)
app/api/views/route.ts (1)
lib/slack/events.ts (1)
notifyDocumentView(111-113)
pages/api/links/download/dataroom-document.ts (1)
lib/slack/events.ts (1)
notifyDocumentDownload(120-122)
pages/api/teams/[teamId]/slack/index.ts (5)
pages/api/slack/oauth/callback.ts (1)
handler(11-165)pages/api/teams/[teamId]/slack/channels.ts (1)
handler(8-102)pages/api/teams/[teamId]/slack/oauth/authorize.ts (1)
handler(8-64)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)
lib/types/slack.ts (2)
lib/slack/types.ts (2)
SlackChannel(14-20)SlackChannelConfig(59-64)lib/slack/templates.ts (1)
SlackChannelConfig(4-9)
pages/api/teams/[teamId]/slack/channels.ts (4)
pages/api/teams/[teamId]/slack/index.ts (1)
handler(8-43)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/slack/client.ts (1)
slackClient(198-198)
lib/swr/use-slack-integration.ts (2)
lib/types/slack.ts (1)
SlackIntegrationResponse(47-47)lib/utils.ts (1)
fetcher(48-62)
pages/api/slack/oauth/callback.ts (5)
pages/api/teams/[teamId]/slack/channels.ts (1)
handler(8-102)pages/api/teams/[teamId]/slack/oauth/authorize.ts (1)
handler(8-64)lib/slack/client.ts (1)
slackClient(198-198)lib/utils.ts (1)
encryptSlackToken(593-616)lib/emails/send-slack-integration-notification.ts (1)
sendSlackIntegrationNotification(5-29)
lib/slack/templates.ts (1)
lib/slack/events.ts (1)
SlackEventData(6-16)
components/upload-zone.tsx (9)
lib/constants.ts (3)
FREE_PLAN_ACCEPTED_FILE_TYPES(101-110)FULL_PLAN_ACCEPTED_FILE_TYPES(112-144)SUPPORTED_DOCUMENT_MIME_TYPES(67-98)lib/swr/use-billing.ts (1)
usePlan(86-136)context/team-context.tsx (1)
useTeam(85-85)ee/limits/swr-handler.ts (1)
useLimits(19-49)lib/utils/get-file-size-limits.ts (2)
getFileSizeLimits(10-43)getFileSizeLimit(46-66)lib/utils/get-page-number-count.ts (1)
getPagesCount(8-40)lib/utils/get-content-type.ts (1)
getSupportedContentType(1-46)lib/documents/create-document.ts (2)
DocumentData(4-13)createDocument(15-59)lib/folders/create-folder.ts (4)
isSystemFile(5-10)createFolderInMainDocs(51-79)determineFolderPaths(81-97)createFolderInBoth(99-162)
pages/api/links/download/index.ts (1)
lib/slack/events.ts (1)
notifyDocumentDownload(120-122)
pages/api/links/download/dataroom-folder.ts (1)
lib/slack/events.ts (1)
notifyDocumentDownload(120-122)
pages/api/teams/[teamId]/index.ts (4)
ee/stripe/index.ts (1)
cancelSubscription(33-56)ee/stripe/utils.ts (1)
isOldAccount(252-254)lib/types.ts (1)
CustomUser(17-17)lib/redis.ts (1)
redis(4-7)
lib/slack/events.ts (3)
lib/slack/client.ts (1)
SlackClient(4-195)lib/types/slack.ts (1)
SlackIntegration(15-32)lib/slack/templates.ts (1)
createSlackMessage(20-39)
components/ui/alert-dialog.tsx (1)
components/ui/button.tsx (1)
Button(71-71)
app/api/views-dataroom/route.ts (1)
lib/slack/events.ts (2)
notifyDataroomAccess(115-117)notifyDocumentView(111-113)
pages/settings/slack.tsx (9)
context/team-context.tsx (1)
useTeam(85-85)lib/swr/use-slack-integration.ts (1)
useSlackIntegration(11-31)lib/swr/use-slack-channels.ts (1)
useSlackChannels(11-31)lib/types/slack.ts (4)
SlackIntegration(15-32)SlackChannelConfig(8-13)UpdateSlackIntegrationRequest(35-40)SlackIntegrationResponse(47-47)components/layouts/app.tsx (1)
AppLayout(16-38)components/settings/settings-header.tsx (1)
SettingsHeader(8-96)components/settings/slack-settings-skeleton.tsx (1)
SlackSettingsSkeleton(3-82)components/ui/tooltip.tsx (1)
BadgeTooltip(53-107)components/ui/multi-select-v2.tsx (1)
MultiSelect(134-378)
pages/api/links/download/bulk.ts (2)
lib/slack/events.ts (1)
notifyDocumentDownload(120-122)lib/utils/ip.ts (1)
getIpAddress(1-8)
🪛 Biome (2.1.2)
components/emails/slack-integration-notification.tsx
[error] 69-69: Avoid using target="_blank" without rel="noopener" or rel="noreferrer".
Opening external links in new tabs without rel="noopener" is a security risk. See the explanation for more details.
Safe fix: Add the rel="noopener" attribute.
(lint/security/noBlankTarget)
🔇 Additional comments (28)
components/upload-zone.tsx (2)
258-261: VerifyownerIdandteamIdare always defined at drop time.If session/team context isn’t loaded, uploads may fail.
Would you like a guard that blocks the drop until both are available and shows a toast?
37-43: Nice: extracting accept maps to top scope.This avoids per-render object identity changes and fixes folder-drop MIME inference.
components/layouts/breadcrumb.tsx (1)
226-227: Slack breadcrumb entry looks correct and consistent.pages/api/teams/[teamId]/index.ts (3)
13-13: No action needed – whitespace.
220-221: Confirm plan field presence for isOldAccount().
isOldAccount(team.plan)assumesteam.planis loaded. Withincludeused above, scalar fields should be present, but please double-check the Team Prisma model to ensureplanis notnullfor deleted/legacy teams. If nullable, guard it.
232-232: No action needed – whitespace.components/ui/multi-select-v2.tsx (1)
121-122: Prop addition LGTM.
triggerIconmakes the control more flexible for branded use (e.g., Slack).lib/utils.ts (1)
592-645: Helpers confirmed server-side only:encryptSlackTokenanddecryptSlackTokenare used exclusively inpages/api/slack/oauth/callback.tsandlib/slack/client.ts, with no imports in any client-side code.prisma/schema/schema.prisma (2)
70-70: No-op line changeNothing to review here.
578-598: Verify SlackIntegration encryption and indexing
- accessToken: annotate with
@db.Textfor ciphertext storage; confirm it’s encrypted before persistence and, if so, rename toaccessTokenEncryptedfor clarity.- enabledChannels & notificationTypes: add
@default(dbgenerated("'{}'::jsonb"))to ensure non-null defaults.- Add
@@index([workspaceId]); if a workspace truly maps one-to-one with a team, replace with@@unique([workspaceId]).prisma/schema/team.prisma (1)
25-25: 1:1 relation wired correctlyOptional one-to-one via SlackIntegration unique(teamId) matches this side. Looks good.
app/api/views/route.ts (1)
21-21: Import is fineNo concerns.
pages/api/links/download/dataroom-folder.ts (1)
11-11: Import looks goodConsistent with other Slack event imports.
pages/api/links/download/dataroom-document.ts (2)
9-9: Slack event import is appropriate and isolated.Importing from the central Slack events module keeps concerns separated and avoids coupling API code to transport details.
41-50: Selecting link.teamId is required for downstream Slack routing.Good addition; it avoids an extra round-trip to resolve team context during notification.
pages/api/links/download/bulk.ts (3)
10-10: Centralized Slack event import looks good.
50-52: Selecting dataroom.id enables proper Slack payloads and later storage ops.
282-282: Formatting-only change.No functional impact.
pages/api/links/download/index.ts (2)
9-9: Slack event import added—good modularity.
42-43: Selecting document.id is necessary for notification payloads.lib/swr/use-slack-integration.ts (1)
11-23: SWR setup and key gating look solid.The key nulling with
enabled && teamIdand conservative revalidation config are appropriate for settings data.components/settings/slack-settings-skeleton.tsx (1)
3-14: Skeleton structure LGTM.Clean, composable placeholders that match likely UI density.
lib/swr/use-slack-channels.ts (1)
25-30: Remove SWR v1 compatibility fallback
package.json shows SWR ^2.3.6, soisLoadingis always defined and boolean—no need for the v1 fallback.pages/api/teams/[teamId]/slack/index.ts (1)
50-67: Good: excludes sensitive fields in GETThe selection omits the Slack access token and limits exposure to non-sensitive fields.
pages/api/teams/[teamId]/slack/channels.ts (1)
1-9: Manual decryption redundant
slackClient.getChannels already calls its internal decryptToken on the passed token, so you can continue passing integration.accessToken directly.Likely an incorrect or invalid review comment.
pages/settings/slack.tsx (1)
98-117: Verify toast.warning availability in sonner.Some versions don’t expose toast.warning; fallback to toast or toast.message to avoid runtime errors.
If unsupported, change:
- toast.warning(`Warning: ${router.query.warning}`); + toast(`Warning: ${router.query.warning}`);lib/slack/client.ts (2)
26-37: Verify Slack OAuth scopes; separate bot scopes from user scopesusers:read.email typically requires user_scope in OAuth v2. Either add user_scope or drop the user email scope if not needed.
Apply (if you need user emails):
getOAuthUrl(state: string, redirectUri: string): string { const params = new URLSearchParams({ client_id: this.clientId, - scope: 'channels:read,groups:read,mpim:read,im:read,chat:write,chat:write.public,team:read,users:read,users:read.email', + scope: 'channels:read,groups:read,mpim:read,im:read,chat:write,chat:write.public,team:read,users:read', + user_scope: 'users:read.email', redirect_uri: redirectUri, state: state, });If you prefer, I can also add a small helper to build scopes from arrays to avoid typos.
197-198: Reuse singleton in consumersGood exporting of a singleton. Ensure downstream (events.ts) uses this instead of constructing new instances.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
pages/api/links/download/bulk.ts (1)
341-350: Null-safe watermark timestamps — addressed.Fallback to current time avoids “Invalid time value.” Thanks for closing out the earlier note.
lib/types/slack.ts (1)
81-88: Good: strong typing for notification types is in placeUsing SlackNotificationType[] for channel-level configs addresses the earlier safety concern.
lib/slack/client.ts (2)
137-147: Paginate with limit and add per-page timeoutMinor efficiency and resiliency improvement.
do { const url = new URL(`${this.baseUrl}/conversations.list`); url.searchParams.set('types', 'public_channel,private_channel'); url.searchParams.set('exclude_archived', 'true'); + url.searchParams.set('limit', '200'); if (cursor) url.searchParams.set('cursor', cursor); - const resp = await fetch(url.toString(), { - headers: { - 'Authorization': `Bearer ${decryptedToken}`, - 'Content-Type': 'application/json', - }, - }); + const ac = new AbortController(); + const t = setTimeout(() => ac.abort(), 10000); + const resp = await fetch(url.toString(), { + headers: { + 'Authorization': `Bearer ${decryptedToken}`, + 'Content-Type': 'application/json', + }, + signal: ac.signal, + }).finally(() => clearTimeout(t));Also applies to: 148-161
19-21: Harden decryptToken: tolerate plaintext and missing keyWrap decryptSlackToken to avoid crashes when NEXT_PRIVATE_SLACK_TOKEN_KEY is absent (common in dev) or token is already plaintext.
- private decryptToken(accessToken: string): string { - return decryptSlackToken(accessToken); - } + private decryptToken(accessToken: string): string { + try { + return decryptSlackToken(accessToken); + } catch { + return accessToken; + } + }lib/slack/templates.ts (1)
37-44: Fix unsafe link?.id.slice(...) and add “direct access” fallbacklink?.id.slice(...) can throw when id is undefined; unify to a safe linkLabel and fall back to “direct access.”
@@ - let accessContext = ""; - if (eventData.dataroomId && dataroom) { - accessContext = `in dataroom "${dataroom.name}"`; - } else if (link?.name) { - accessContext = `via shared link "${link.name}"`; - } else { - accessContext = `via shared link "Link ${link?.id.slice(0, 5)}"`; - } + const linkLabel = link?.name ?? (typeof link?.id === 'string' ? `Link ${link.id.slice(0, 5)}` : null); + const accessContext = + dataroom?.name ? `in dataroom "${dataroom.name}"` + : linkLabel ? `via shared link "${linkLabel}"` + : "via direct access"; @@ - text: eventData.dataroomId - ? `Viewed document in dataroom "${dataroom?.name || 'Unknown'}"` - : link?.name - ? `Viewed document via shared link "${link.name}"` - : `Viewed document via shared link "Link ${link?.id.slice(0, 5)}"` + text: eventData.dataroomId + ? `Viewed document in dataroom "${dataroom?.name || 'Unknown'}"` + : linkLabel + ? `Viewed document via shared link "${linkLabel}"` + : "Viewed document via direct access" @@ - const accessContext = link?.name ? `via shared link "${link.name}"` : `via shared link "Link ${link?.id.slice(0, 5)}"`; + const linkLabel = link?.name ?? (typeof link?.id === 'string' ? `Link ${link.id.slice(0, 5)}` : null); + const accessContext = linkLabel ? `via shared link "${linkLabel}"` : "via direct access"; @@ - text: link?.name - ? `Dataroom accessed via shared link "${link.name}"` - : `Dataroom accessed via shared link "Link ${link?.id.slice(0, 5)}"` + text: linkLabel + ? `Dataroom accessed via shared link "${linkLabel}"` + : "Dataroom accessed via direct access" @@ - } else if (link?.name) { - downloadContext = `via shared link "${link.name}"`; - } else { - downloadContext = `via shared link "Link ${link?.id.slice(0, 5)}"`; - } + } else if (linkLabel) { + downloadContext = `via shared link "${linkLabel}"`; + } else { + downloadContext = "via direct access"; + } @@ - : link?.name - ? `Document download via shared link "${link.name}"` - : `Document download via shared link "Link ${link?.id.slice(0, 5)}"` + : linkLabel + ? `Document download via shared link "${linkLabel}"` + : "Document download via direct access"Also applies to: 92-97, 132-134, 187-190, 243-248, 301-305
🧹 Nitpick comments (9)
pages/api/links/download/bulk.ts (1)
306-324: Don’t block bulk download on Slack latency; optionally send after success.Awaiting Slack here can slow user downloads and the event name suggests a successful download. Two options:
- Non-blocking: fire-and-forget with local error handling.
- Semantics: emit after the Lambda returns a URL (i.e., confirmed success), or include a status field.
Apply for non-blocking (minimal change):
- if (view.dataroom?.teamId) { - try { - await notifyDocumentDownload({ + if (view.dataroom?.teamId) { + notifyDocumentDownload({ teamId: view.dataroom.teamId, documentId: undefined, // Bulk download, no specific document dataroomId: view.dataroom.id, linkId, viewerEmail: view.viewerEmail ?? undefined, viewerId: undefined, metadata: { documentCount: downloadDocuments.length, isBulkDownload: true, }, - }); - } catch (error) { - console.error("Error sending Slack notification:", error); - } + }).catch((error) => { + console.error("Error sending Slack notification:", error); + }); }Would you prefer we emit only after the download URL is generated? I can provide a follow-up diff to move this block into the 200-response branch with a metadata.status = "success".
lib/types/slack.ts (2)
17-21: Unify notification types shape across integration and channelsSlackIntegration.notificationTypes is an object of booleans while channels use a string union array. Standardize via a reusable map type.
@@ - notificationTypes: { - document_view: boolean; - dataroom_access: boolean; - document_download: boolean; - }; + notificationTypes: SlackNotificationSettings; @@ -export type SlackNotificationType = 'document_view' | 'dataroom_access' | 'document_download'; +export type SlackNotificationType = 'document_view' | 'dataroom_access' | 'document_download'; +export type SlackNotificationSettings = Record<SlackNotificationType, boolean>;Also applies to: 81-83
72-79: Type Slack blocks more strictly (optional)Swap any[] for official Slack block types to catch template issues at compile time.
+import type { KnownBlock, Block } from '@slack/types'; @@ -export interface SlackMessage { +export interface SlackMessage { channel?: string; text?: string; - blocks?: any[]; + blocks?: Array<KnownBlock | Block>; thread_ts?: string; unfurl_links?: boolean; unfurl_media?: boolean; }lib/slack/client.ts (4)
62-64: Surface Slack error body on OAuth failureInclude response body for faster debugging.
- if (!response.ok) { - throw new Error(`Slack OAuth failed: ${response.statusText}`); - } + if (!response.ok) { + const text = await response.text().catch(() => ''); + throw new Error(`Slack OAuth failed: ${response.status} ${response.statusText}${text ? ` - ${text}` : ''}`); + } @@ - if (!data.ok) { - throw new Error(`Slack OAuth error: ${data.error}`); - } + if (!data.ok) { + throw new Error(`Slack OAuth error: ${data.error}`); + }Also applies to: 66-71
79-86: Add timeout to team.info and drop unnecessary Content-TypeConsistency with OAuth call; GET doesn’t need Content-Type.
async getWorkspaceInfo(accessToken: string): Promise<SlackWorkspaceInfo> { const decryptedToken = this.decryptToken(accessToken); - const response = await fetch(`${this.baseUrl}/team.info`, { - headers: { - 'Authorization': `Bearer ${decryptedToken}`, - 'Content-Type': 'application/x-www-form-urlencoded', - }, - }); + const ac = new AbortController(); + const t = setTimeout(() => ac.abort(), 10000); + const response = await fetch(`${this.baseUrl}/team.info`, { + headers: { + 'Authorization': `Bearer ${decryptedToken}`, + }, + signal: ac.signal, + }).finally(() => clearTimeout(t));Also applies to: 87-90
109-116: Add timeout to auth.test and simplify headersSame rationale as above.
- const response = await fetch(`${this.baseUrl}/auth.test`, { - headers: { - 'Authorization': `Bearer ${decryptedToken}`, - 'Content-Type': 'application/json', - }, - }); + const ac = new AbortController(); + const t = setTimeout(() => ac.abort(), 10000); + const response = await fetch(`${this.baseUrl}/auth.test`, { + headers: { + 'Authorization': `Bearer ${decryptedToken}`, + }, + signal: ac.signal, + }).finally(() => clearTimeout(t));Also applies to: 117-121
169-178: Timeout + basic 429 retry for chat.postMessageAvoid hanging calls; respect Retry-After for transient rate limits.
- const response = await fetch(`${this.baseUrl}/chat.postMessage`, { - method: 'POST', - headers: { - 'Authorization': `Bearer ${decryptedToken}`, - 'Content-Type': 'application/json', - }, - body: JSON.stringify(message), - }); + let response: Response; + for (let attempt = 0; attempt < 2; attempt++) { + const ac = new AbortController(); + const t = setTimeout(() => ac.abort(), 10000); + response = await fetch(`${this.baseUrl}/chat.postMessage`, { + method: 'POST', + headers: { + 'Authorization': `Bearer ${decryptedToken}`, + 'Content-Type': 'application/json', + }, + body: JSON.stringify(message), + signal: ac.signal, + }).finally(() => clearTimeout(t)); + if (response.status !== 429) break; + const retryAfter = parseInt(response.headers.get('retry-after') || '1', 10); + await new Promise(r => setTimeout(r, Math.min(retryAfter, 5) * 1000)); + }Also applies to: 180-183
lib/slack/templates.ts (2)
30-33: Parallelize DB fetches to cut latencyFetch document/dataroom/link in parallel.
- const document = eventData.documentId ? await getDocumentInfo(eventData.documentId) : null; - const dataroom = eventData.dataroomId ? await getDataroomInfo(eventData.dataroomId) : null; - const link = eventData.linkId ? await getLinkInfo(eventData.linkId) : null; + const [document, dataroom, link] = await Promise.all([ + eventData.documentId ? getDocumentInfo(eventData.documentId) : Promise.resolve(null), + eventData.dataroomId ? getDataroomInfo(eventData.dataroomId) : Promise.resolve(null), + eventData.linkId ? getLinkInfo(eventData.linkId) : Promise.resolve(null), + ]); @@ - const document = eventData.documentId ? await getDocumentInfo(eventData.documentId) : null; - const dataroom = eventData.dataroomId ? await getDataroomInfo(eventData.dataroomId) : null; - const link = eventData.linkId ? await getLinkInfo(eventData.linkId) : null; + const [document, dataroom, link] = await Promise.all([ + eventData.documentId ? getDocumentInfo(eventData.documentId) : Promise.resolve(null), + eventData.dataroomId ? getDataroomInfo(eventData.dataroomId) : Promise.resolve(null), + eventData.linkId ? getLinkInfo(eventData.linkId) : Promise.resolve(null), + ]);Also applies to: 220-223
111-114: Ensure NEXTAUTH_URL is set (avoid broken action links)If unset in some envs, buttons will point to “undefined/...”.
Also applies to: 204-207, 319-324
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
components/emails/slack-integration-notification.tsx(1 hunks)components/ui/alert-dialog.tsx(2 hunks)lib/slack/client.ts(1 hunks)lib/slack/templates.ts(1 hunks)lib/types/slack.ts(1 hunks)pages/api/links/download/bulk.ts(5 hunks)pages/api/links/download/dataroom-document.ts(4 hunks)pages/api/links/download/dataroom-folder.ts(3 hunks)pages/api/links/download/index.ts(4 hunks)pages/api/teams/[teamId]/slack/index.ts(1 hunks)pages/settings/slack.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pages/api/teams/[teamId]/slack/index.ts
- components/ui/alert-dialog.tsx
- pages/settings/slack.tsx
- components/emails/slack-integration-notification.tsx
- pages/api/links/download/index.ts
- pages/api/links/download/dataroom-document.ts
🧰 Additional context used
🧬 Code graph analysis (4)
lib/slack/client.ts (2)
lib/utils.ts (1)
decryptSlackToken(619-645)lib/types/slack.ts (4)
SlackOAuthResponse(50-70)SlackWorkspaceInfo(43-48)SlackChannel(1-7)SlackMessage(72-79)
pages/api/links/download/dataroom-folder.ts (1)
lib/slack/events.ts (1)
notifyDocumentDownload(120-122)
lib/slack/templates.ts (2)
lib/slack/events.ts (1)
SlackEventData(6-16)lib/types/slack.ts (1)
SlackMessage(72-79)
pages/api/links/download/bulk.ts (2)
lib/slack/events.ts (1)
notifyDocumentDownload(120-122)lib/utils/ip.ts (1)
getIpAddress(1-8)
🔇 Additional comments (5)
pages/api/links/download/bulk.ts (3)
10-10: Importing Slack notifier is correct and scoped well.No concerns here.
50-50: Selectingdataroom.idis necessary for Slack payload.Good catch—this unblocks composing the event.
282-282: Formatting-only change.No functional impact.
pages/api/links/download/dataroom-folder.ts (1)
11-11: LGTM: Slack events import is correct.Import path and symbol match lib/slack/events.ts. No issues.
lib/slack/client.ts (1)
27-33: Verify scopes; trim to least privilegeDo you truly need mpim:read and im:read and chat:write.public? If not required for your UX, drop them.
| if (view.link.teamId) { | ||
| try { | ||
| await notifyDocumentDownload({ | ||
| teamId: view.link.teamId, | ||
| documentId: undefined, | ||
| dataroomId, | ||
| linkId, | ||
| viewerEmail: view.viewerEmail ?? undefined, | ||
| viewerId: undefined, | ||
| metadata: { | ||
| folderName: rootFolder.name, | ||
| documentCount: allDocuments.length, | ||
| isFolderDownload: true, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Error sending Slack notification:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t block the download API on Slack; also avoid false-positive “download” notifications.
- The awaited Slack call adds latency to a hot path. Make it fire-and-forget with a caught promise.
- Consider moving the notification to after the Lambda succeeds (i.e., after downloadUrl is obtained) to avoid notifying on failed/aborted downloads. If business intent is “download requested,” keep here but clarify in Slack copy.
- Improve error log context (teamId, dataroomId, linkId) for triage.
Apply this diff to make the call non-blocking and improve logging:
- if (view.link.teamId) {
- try {
- await notifyDocumentDownload({
- teamId: view.link.teamId,
- documentId: undefined,
- dataroomId,
- linkId,
- viewerEmail: view.viewerEmail ?? undefined,
- viewerId: undefined,
- metadata: {
- folderName: rootFolder.name,
- documentCount: allDocuments.length,
- isFolderDownload: true,
- },
- });
- } catch (error) {
- console.error("Error sending Slack notification:", error);
- }
- }
+ if (view.link.teamId) {
+ void notifyDocumentDownload({
+ teamId: view.link.teamId,
+ documentId: undefined,
+ dataroomId,
+ linkId,
+ viewerEmail: view.viewerEmail ?? undefined,
+ viewerId: undefined,
+ metadata: {
+ folderName: rootFolder.name,
+ documentCount: allDocuments.length,
+ isFolderDownload: true,
+ },
+ }).catch((error) => {
+ console.error("Error sending Slack notification", {
+ teamId: view.link.teamId,
+ dataroomId,
+ linkId,
+ error,
+ });
+ });
+ }Option (if you want “only on success” semantics): move the non-blocking call to just after you compute downloadUrl and before sending the 200 response.
Run to check other usages and ensure consistent event semantics across the codebase:
🏁 Script executed:
#!/bin/bash
rg -n --no-heading "notifyDocumentDownload\(|eventType:\s*['\"]document_download['\"]" -C2Length of output: 1972
Make notifyDocumentDownload calls non-blocking in all download APIs
Apply the fire-and-forget pattern (void notifyDocumentDownload(...).catch(...)) instead of await in:
- pages/api/links/download/index.ts
- pages/api/links/download/dataroom-folder.ts
- pages/api/links/download/dataroom-document.ts
- pages/api/links/download/bulk.ts
Include{teamId, dataroomId?, linkId?, documentId?}in error logs for triage. Optionally move calls to after downloadUrl generation for true “download succeeded” notifications.
🤖 Prompt for AI Agents
In pages/api/links/download/dataroom-folder.ts around lines 299 to 317, the
notifyDocumentDownload call is awaited which blocks the response; change it to a
fire-and-forget pattern by calling notifyDocumentDownload without await and
attach .catch(...) to log errors, and ensure the catch log includes { teamId:
view.link.teamId, dataroomId, linkId, documentId: undefined } for triage;
optionally move the non-blocking call to after downloadUrl generation if you
want to guarantee the notification only when the download URL is created.
| date: new Date(view.viewedAt ? view.viewedAt : new Date()).toLocaleDateString(), | ||
| time: new Date(view.viewedAt ? view.viewedAt : new Date()).toLocaleTimeString(), | ||
| link: view.link.name, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compute once to prevent date/time skew; use a stable timezone.
Two new Date() evaluations can diverge across midnight boundaries and are locale/host-TZ dependent. Compute once and format with an explicit timezone.
Apply these changes:
@@
- const params = {
+ const viewedAtForWatermark = view.viewedAt ?? new Date();
+ const params = {
@@
- email: view.viewerEmail,
- date: new Date(view.viewedAt ? view.viewedAt : new Date()).toLocaleDateString(),
- time: new Date(view.viewedAt ? view.viewedAt : new Date()).toLocaleTimeString(),
+ email: view.viewerEmail,
+ date: viewedAtForWatermark.toLocaleDateString('en-US', { timeZone: 'UTC' }),
+ time: viewedAtForWatermark.toLocaleTimeString('en-US', { timeZone: 'UTC', hour12: false }),
link: view.link.name,
ipAddress: getIpAddress(req.headers),If the watermark service supports it, consider also passing an ISO timestamp (e.g., timestamp: viewedAtForWatermark.toISOString()) for unambiguous auditing.
📝 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.
| date: new Date(view.viewedAt ? view.viewedAt : new Date()).toLocaleDateString(), | |
| time: new Date(view.viewedAt ? view.viewedAt : new Date()).toLocaleTimeString(), | |
| link: view.link.name, | |
| // Compute once to avoid skew and use a stable timezone | |
| const viewedAtForWatermark = view.viewedAt ?? new Date(); | |
| const params = { | |
| email: view.viewerEmail, | |
| date: viewedAtForWatermark.toLocaleDateString('en-US', { timeZone: 'UTC' }), | |
| time: viewedAtForWatermark.toLocaleTimeString('en-US', { timeZone: 'UTC', hour12: false }), | |
| link: view.link.name, | |
| ipAddress: getIpAddress(req.headers), | |
| // Optional for unambiguous auditing: | |
| // timestamp: viewedAtForWatermark.toISOString(), | |
| }; |
🤖 Prompt for AI Agents
In pages/api/links/download/dataroom-folder.ts around lines 338 to 340, the code
instantiates new Date() twice causing potential date/time skew and uses host
locale/ TZ; compute viewedAtForWatermark once (const viewedAtForWatermark =
view.viewedAt ? new Date(view.viewedAt) : new Date()), use that single Date
instance to derive date and time strings formatted with an explicit
timezone/locale (e.g., toLocaleDateString('en-US', { timeZone: 'UTC' }) and
toLocaleTimeString('en-US', { timeZone: 'UTC' })), and also pass an ISO
timestamp (e.g., timestamp: viewedAtForWatermark.toISOString()) to the watermark
service for unambiguous auditing.
|
coderabbit full review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/featureFlags/index.ts (1)
31-36: Avoid enabling Slack by default when EDGE_CONFIG is missingAuto-enabling all features in dev also turns on Slack, which can surface broken UI/API paths if Slack env isn’t configured. Keep dev ergonomics for other flags but default Slack to false here.
- if (!process.env.EDGE_CONFIG) { - return Object.fromEntries( - Object.entries(teamFeatures).map(([key, _v]) => [key, true]), - ); - } else if (!teamId) { + if (!process.env.EDGE_CONFIG) { + // Enable all for local/dev except Slack which requires external config + return Object.fromEntries( + Object.entries(teamFeatures).map(([key]) => [ + key, + key === "slack" ? false : true, + ]), + ); + } else if (!teamId) {
🧹 Nitpick comments (25)
lib/featureFlags/index.ts (1)
48-54: Validate edge-config payload keys to known BetaFeaturesRuntime casts allow unknown keys to toggle flags silently. Add a schema (z.enum of BetaFeatures) to validate and ignore unknown entries.
Would you like a small zod schema snippet to validate
betaFeaturesfrom Edge Config?lib/integrations/slack/env.ts (2)
14-15: Add explicit return type to lock contractHelps prevent accidental changes and improves DX.
-export const getSlackEnv = () => { +export const getSlackEnv = (): SlackEnv => {
21-25: Surface which env vars are missing without leaking secretsPrinting the paths of failing keys speeds up setup while keeping values private.
- if (!parsed.success) { - throw new Error( - "Slack app environment variables are not configured properly.", - ); - } + if (!parsed.success) { + const missing = parsed.error.issues.map((i) => i.path.join(".")).join(", "); + throw new Error( + `Slack app environment variables are not configured properly. Missing/invalid: ${missing}`, + ); + }prisma/schema/conversation.prisma (1)
204-213: Consider composite indexes for primary read pathsPer-item indexes are good. If you commonly list FAQs by team/status/recency or by dataroom/status, add composites like:
- @@index([teamId, status, createdAt])
- @@index([dataroomId, status, createdAt])
This can materially reduce planning time and I/O on list endpoints.
If you share typical WHERE/ORDER BY patterns, I can propose targeted indexes.
components/emails/installed-integration-notification.tsx (1)
59-66: Guard settings link against missing base URLIf
NEXT_PUBLIC_BASE_URLisn’t set in the email-rendering environment, the link becomes invalid. Add a fallback or fail early.- href={`${process.env.NEXT_PUBLIC_BASE_URL}/settings/integrations/${integration.slug}`} + href={`${process.env.NEXT_PUBLIC_BASE_URL ?? "https://app.papermark.io"}/settings/integrations/${integration.slug}`}prisma/schema/integration.prisma (1)
21-40: Operational indexes and token hygiene metadata
- Add
@@index([teamId, enabled])to speed up listing enabled integrations per team.- Consider
encryptionVersion(Int) andtokenLastRotatedAt(DateTime?) to support crypto migrations/rotation without out-of-band knowledge.- If categories are finite, switch
categoryto an enum for data quality.Share your common read patterns and I’ll propose exact composite indexes.
lib/integrations/slack/uninstall.ts (1)
14-24: Add timeout and handle non-2xx responses explicitlyAvoid hanging requests and surface HTTP errors cleanly.
- const response = await fetch("https://slack.com/api/apps.uninstall", { - method: "POST", - headers: { - "Content-Type": "application/x-www-form-urlencoded", - }, - body: new URLSearchParams({ - token, - client_id: env.SLACK_CLIENT_ID, - client_secret: env.SLACK_CLIENT_SECRET, - }), - }); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 10_000); + const response = await fetch("https://slack.com/api/apps.uninstall", { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + token, + client_id: env.SLACK_CLIENT_ID, + client_secret: env.SLACK_CLIENT_SECRET, + }), + signal: controller.signal, + }).finally(() => clearTimeout(timeout)); @@ - const data = await response.json(); + if (!response.ok) { + throw new Error(`Slack uninstall HTTP error: ${response.status}`); + } + const data = await response.json();Also applies to: 26-32
lib/swr/use-slack-integration.ts (1)
14-20: Extract shared SWR options to avoid duplication across hooks.These options are duplicated in use-slack-channels. Centralize them (e.g., lib/swr/options.ts) and import here.
Apply locally:
- { - revalidateOnFocus: false, - revalidateOnReconnect: false, - dedupingInterval: 30000, - revalidateIfStale: false, - errorRetryCount: 2, - errorRetryInterval: 5000, - }, + SLACK_SWR_OPTS,New file:
// lib/swr/options.ts export const SLACK_SWR_OPTS = { revalidateOnFocus: false, revalidateOnReconnect: false, dedupingInterval: 30000, revalidateIfStale: false, errorRetryCount: 2, errorRetryInterval: 5000, } as const;app/api/integrations/slack/oauth/callback/route.ts (1)
27-28: Remove unused workspace variable.It’s never read.
- let workspace: Pick<Team, "id" | "plan"> | null = null; + // no-op- workspace = await prisma.team.findUniqueOrThrow({ + await prisma.team.findUniqueOrThrow({lib/integrations/install.ts (1)
1-1: Prefer Next’s unstable_after over @vercel/functions waitUntil for portability.unstable_after works across runtimes in Next.js App Router.
-import { waitUntil } from "@vercel/functions"; +import { unstable_after as after } from "next/server";- waitUntil( - (async () => { + after(async () => { const team = await prisma.team.findUniqueOrThrow({ where: { id: teamId, }, select: { name: true, users: { where: { userId }, select: { user: { select: { email: true }, }, }, }, installedIntegrations: { where: { integrationId }, select: { integration: { select: { name: true, slug: true, }, }, }, }, }, }); // ... - })(), - ); + });lib/swr/use-slack-channels.ts (2)
17-23: Extract shared SWR options to a constant (same as useSlackIntegration).Avoid duplication and ensure consistency of retry/backoff.
- { - revalidateOnFocus: false, - revalidateOnReconnect: false, - dedupingInterval: 30000, - revalidateIfStale: false, - errorRetryCount: 2, - errorRetryInterval: 5000, - }, + SLACK_SWR_OPTS,Additional change outside this hunk:
// import from your new options module import { SLACK_SWR_OPTS } from "@/lib/swr/options";
27-27: Use nullish coalescing for array default.Safer when empty arrays are valid values.
- channels: data?.channels || [], + channels: data?.channels ?? [],pages/api/teams/[teamId]/integrations/slack/index.ts (1)
82-86: Prefer structured logging with context.Add
teamIdanduserIdto the log to aid debugging.pages/api/teams/[teamId]/integrations/slack/channels.ts (1)
73-94: Consider surfacing Slack API failures instead of a 200 with empty list.Returning 200 on unknown Slack errors hides outages. Prefer 5xx (e.g., 502) with a minimal error to allow the UI to display a retry.
lib/integrations/slack/client.ts (2)
19-21: Don’t hard-fail when OAuth env vars are missing.Channel listing and messaging only need the bot token; throwing here breaks non-OAuth code paths and import-time.
Apply:
- if (!this.clientId || !this.clientSecret) { - throw new Error("SLACK_CLIENT_ID and SLACK_CLIENT_SECRET must be set"); - } + if (!this.clientId || !this.clientSecret) { + console.warn( + "Slack OAuth env not set; OAuth features disabled. Token-based APIs remain available.", + ); + }
224-224: Singleton instantiation can still throw at import-time.If you keep strict env checks in the future, consider lazy instantiation (factory) to avoid import-time failures.
lib/integrations/slack/events.ts (1)
61-80: Optional: parallelize per-channel posts with bounded concurrency.If many channels are enabled, consider
Promise.allSettledor a small pool to reduce latency while preserving isolation.lib/integrations/slack/types.ts (1)
17-17: Remove stale comment.The inline comment adds noise.
-// from lib/types/slack.tsprisma/migrations/20250913000000_add_integrations/migration.sql (1)
11-11: Default configuration to an empty object to avoid null handling.Set a NOT NULL default for configuration to simplify reads and avoid null checks in resolvers/UI.
- "configuration" JSONB, + "configuration" JSONB NOT NULL DEFAULT '{}'::jsonb,pages/settings/slack.tsx (2)
223-233: Remove unused timing variables.startTime/endTime aren’t used; keep code lean.
- const startTime = performance.now(); const response = await fetch( `/api/teams/${teamId}/integrations/slack/channels`, { method: "PUT", headers: { "Content-Type": "application/json" }, body: JSON.stringify(requestBody), }, ); - const endTime = performance.now();
286-291: “debouncedChannelsUpdate” isn’t debounced; implement actual debounce.Frequent selection changes will fire many PUTs. Debounce client-side.
-import { useCallback, useEffect, useMemo, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react";- const debouncedChannelsUpdate = (selectedChannelIds: string[]) => { - setPendingChannelUpdate(true); - handleChannelsUpdate(selectedChannelIds).finally(() => { - setPendingChannelUpdate(false); - }); - }; + const channelsUpdateTimer = useRef<number | null>(null); + const debouncedChannelsUpdate = (selectedChannelIds: string[]) => { + setPendingChannelUpdate(true); + if (channelsUpdateTimer.current) window.clearTimeout(channelsUpdateTimer.current); + channelsUpdateTimer.current = window.setTimeout(() => { + handleChannelsUpdate(selectedChannelIds).finally(() => { + setPendingChannelUpdate(false); + }); + }, 400); + };Also applies to: 3-3
lib/integrations/slack/templates.ts (4)
119-123: Ensure absolute URLs exist; NEXTAUTH_URL may be unset.If NEXTAUTH_URL isn’t configured, Slack links may be invalid. Either validate and throw early, or fall back to a known public base URL.
- ? `${process.env.NEXTAUTH_URL}/documents/${eventData.documentId}` - : `${process.env.NEXTAUTH_URL}/dashboard`, + ? `${process.env.NEXTAUTH_URL ?? ""}/documents/${eventData.documentId}` + : `${process.env.NEXTAUTH_URL ?? ""}/dashboard`,Consider failing fast if the base URL is empty.
Also applies to: 219-223, 339-344
91-93: Use a consistent, explicit time format.Server-local toLocaleString leads to inconsistent timestamps. Prefer ISO 8601 or add timezone.
- text: `*Time:*\n${new Date().toLocaleString()}`, + text: `*Time:*\n${new Date().toISOString()}`,Also applies to: 184-186, 309-311
32-39: Parallelize independent fetches (minor).Fetch document/dataroom/link in parallel to reduce latency.
Example:
const [document, dataroom, link] = await Promise.all([ eventData.documentId ? getDocumentInfo(eventData.documentId, eventData.teamId) : Promise.resolve(null), eventData.dataroomId ? getDataroomInfo(eventData.dataroomId, eventData.teamId) : Promise.resolve(null), eventData.linkId ? getLinkInfo(eventData.linkId, eventData.teamId) : Promise.resolve(null), ]);Also applies to: 135-139, 235-242
415-433: Unused helper.getViewInfo isn’t used; remove or integrate to source metadata from viewId when present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
app/api/integrations/slack/oauth/authorize/route.ts(1 hunks)app/api/integrations/slack/oauth/callback/route.ts(1 hunks)app/api/views-dataroom/route.ts(3 hunks)app/api/views/route.ts(2 hunks)components/emails/installed-integration-notification.tsx(1 hunks)components/settings/settings-header.tsx(2 hunks)lib/featureFlags/index.ts(2 hunks)lib/integrations/install.ts(1 hunks)lib/integrations/slack/client.ts(1 hunks)lib/integrations/slack/env.ts(1 hunks)lib/integrations/slack/events.ts(1 hunks)lib/integrations/slack/install.ts(1 hunks)lib/integrations/slack/templates.ts(1 hunks)lib/integrations/slack/types.ts(1 hunks)lib/integrations/slack/uninstall.ts(1 hunks)lib/integrations/slack/utils.ts(1 hunks)lib/swr/use-slack-channels.ts(1 hunks)lib/swr/use-slack-integration.ts(1 hunks)pages/api/links/download/bulk.ts(5 hunks)pages/api/links/download/dataroom-document.ts(4 hunks)pages/api/links/download/dataroom-folder.ts(3 hunks)pages/api/links/download/index.ts(4 hunks)pages/api/teams/[teamId]/datarooms/[id]/documents/index.ts(0 hunks)pages/api/teams/[teamId]/integrations/slack/channels.ts(1 hunks)pages/api/teams/[teamId]/integrations/slack/index.ts(1 hunks)pages/settings/slack.tsx(1 hunks)prisma/migrations/20250913000000_add_integrations/migration.sql(1 hunks)prisma/schema/conversation.prisma(2 hunks)prisma/schema/dataroom.prisma(2 hunks)prisma/schema/document.prisma(1 hunks)prisma/schema/integration.prisma(1 hunks)prisma/schema/schema.prisma(2 hunks)prisma/schema/team.prisma(1 hunks)
💤 Files with no reviewable changes (1)
- pages/api/teams/[teamId]/datarooms/[id]/documents/index.ts
✅ Files skipped from review due to trivial changes (2)
- prisma/schema/document.prisma
- prisma/schema/dataroom.prisma
🚧 Files skipped from review as they are similar to previous changes (9)
- prisma/schema/team.prisma
- pages/api/links/download/dataroom-folder.ts
- app/api/views-dataroom/route.ts
- prisma/schema/schema.prisma
- app/api/views/route.ts
- pages/api/links/download/bulk.ts
- pages/api/links/download/dataroom-document.ts
- pages/api/links/download/index.ts
- components/settings/settings-header.tsx
🧰 Additional context used
🧬 Code graph analysis (14)
lib/integrations/slack/uninstall.ts (2)
lib/integrations/slack/env.ts (1)
getSlackEnv(14-30)lib/integrations/slack/types.ts (1)
SlackCredential(3-11)
app/api/integrations/slack/oauth/authorize/route.ts (5)
app/api/integrations/slack/oauth/callback/route.ts (1)
GET(24-95)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/utils/get-search-params.ts (1)
getSearchParams(1-10)lib/types.ts (1)
CustomUser(17-17)lib/integrations/slack/install.ts (1)
getSlackInstallationUrl(8-26)
lib/integrations/slack/install.ts (2)
lib/integrations/slack/env.ts (1)
getSlackEnv(14-30)lib/redis.ts (1)
redis(4-7)
pages/api/teams/[teamId]/integrations/slack/index.ts (4)
pages/api/teams/[teamId]/integrations/slack/channels.ts (1)
handler(12-131)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/integrations/slack/env.ts (1)
getSlackEnv(14-30)
pages/api/teams/[teamId]/integrations/slack/channels.ts (6)
pages/api/teams/[teamId]/integrations/slack/index.ts (1)
handler(16-51)pages/api/auth/[...nextauth].ts (1)
authOptions(35-204)lib/types.ts (1)
CustomUser(17-17)lib/integrations/slack/env.ts (1)
getSlackEnv(14-30)lib/integrations/slack/client.ts (1)
slackClient(224-224)lib/integrations/slack/types.ts (1)
SlackCredential(3-11)
lib/integrations/install.ts (1)
lib/resend.ts (1)
sendEmail(12-96)
app/api/integrations/slack/oauth/callback/route.ts (7)
lib/integrations/slack/env.ts (1)
getSlackEnv(14-30)lib/types.ts (2)
Team(304-311)CustomUser(17-17)lib/utils/get-search-params.ts (1)
getSearchParams(1-10)lib/redis.ts (1)
redis(4-7)lib/integrations/slack/types.ts (1)
SlackCredential(3-11)lib/integrations/slack/utils.ts (1)
encryptSlackToken(3-28)lib/integrations/install.ts (1)
installIntegration(16-98)
pages/settings/slack.tsx (11)
context/team-context.tsx (1)
useTeam(85-85)lib/hooks/use-feature-flags.ts (1)
useFeatureFlags(10-31)lib/swr/use-slack-integration.ts (1)
useSlackIntegration(8-29)lib/swr/use-slack-channels.ts (1)
useSlackChannels(7-32)lib/integrations/slack/types.ts (2)
SlackChannelConfig(61-66)SlackIntegration(26-32)components/layouts/app.tsx (1)
AppLayout(16-38)components/settings/settings-header.tsx (1)
SettingsHeader(8-98)components/settings/slack-settings-skeleton.tsx (1)
SlackSettingsSkeleton(3-82)components/shared/icons/slack-icon.tsx (1)
SlackIcon(8-36)components/ui/tooltip.tsx (1)
BadgeTooltip(53-107)components/ui/multi-select-v2.tsx (1)
MultiSelect(134-378)
lib/swr/use-slack-channels.ts (3)
context/team-context.tsx (1)
useTeam(85-85)lib/integrations/slack/types.ts (1)
SlackChannel(18-24)lib/utils.ts (1)
fetcher(48-62)
lib/integrations/slack/events.ts (4)
lib/integrations/slack/client.ts (1)
SlackClient(9-221)lib/integrations/slack/types.ts (2)
SlackEventData(43-54)SlackIntegration(26-32)lib/integrations/slack/env.ts (1)
getSlackEnv(14-30)lib/integrations/slack/templates.ts (1)
createSlackMessage(5-24)
lib/integrations/slack/templates.ts (1)
lib/integrations/slack/types.ts (2)
SlackEventData(43-54)SlackMessage(34-41)
components/emails/installed-integration-notification.tsx (1)
components/emails/shared/footer.tsx (1)
Footer(3-27)
lib/swr/use-slack-integration.ts (3)
context/team-context.tsx (1)
useTeam(85-85)lib/integrations/slack/types.ts (1)
SlackIntegration(26-32)lib/utils.ts (1)
fetcher(48-62)
lib/integrations/slack/client.ts (2)
lib/integrations/slack/types.ts (2)
SlackChannel(18-24)SlackMessage(34-41)lib/integrations/slack/utils.ts (1)
decryptSlackToken(30-58)
🔇 Additional comments (8)
lib/integrations/slack/utils.ts (1)
1-1: Confirm Node.js runtime usageThis relies on Node’s
crypto. Ensure these helpers aren’t called in the Edge runtime (Web Crypto API differs).pages/api/teams/[teamId]/integrations/slack/index.ts (1)
1-51: Auth and team-guarding look good.The session check, composite key membership check, and method switch are clean.
pages/api/teams/[teamId]/integrations/slack/channels.ts (1)
62-71: Channel mapping LGTM.Filters archived, normalizes
is_member, and sorts by name.lib/integrations/slack/types.ts (1)
34-41: Types for SlackMessage are fine for now.Given variability of Block Kit payloads,
any[]is acceptable.prisma/migrations/20250913000000_add_integrations/migration.sql (2)
44-44: Indexes/uniques look good.Composite uniqueness on (teamId, integrationId) and unique slug are appropriate for preventing duplicates.
Also applies to: 47-47
10-10: Confirm: updatedAt is managed by Prisma (@updatedat). Integration.updatedAt (prisma/schema/integration.prisma:16) and InstalledIntegration.updatedAt (prisma/schema/integration.prisma:35) are declared DateTime @updatedat — DB default not required.pages/settings/slack.tsx (1)
46-55: SWR usage and gating look solid.Nice job gating channel fetches on integration availability and disabling revalidate-on-focus to reduce churn.
Also applies to: 56-64
lib/integrations/slack/templates.ts (1)
5-24: Top-level API looks good.Graceful error handling with null returns is appropriate for the dispatcher.
| const userTeam = await prisma.userTeam.findUnique({ | ||
| where: { | ||
| userId_teamId: { | ||
| userId, | ||
| teamId, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm composite unique (userId, teamId) exists for userTeam.
findUnique on userId_teamId requires a matching @@unique/@@id. If absent, use findFirst.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Model UserTeam definition:"
rg -nP -C3 'model\s+UserTeam\b' prisma
echo
echo "Looking for composite unique on [userId, teamId]:"
rg -nP '@@unique\(\s*\[userId\s*,\s*teamId\]\s*\)' prisma || echo "Composite unique NOT found"Length of output: 420
Fix findUnique usage — add composite unique or use findFirst
Verified: prisma/schema/team.prisma model UserTeam does NOT define @@unique([userId, teamId]) (composite unique NOT found). findUnique({ where: { userId_teamId: { userId, teamId } } }) is invalid.
- Option A (preferred): add @@unique([userId, teamId]) to model UserTeam in prisma/schema/team.prisma so the existing findUnique call is correct.
- Option B: change app/api/integrations/slack/oauth/authorize/route.ts (lines ~27–34) to use prisma.userTeam.findFirst({ where: { userId, teamId } }) or query by a true unique field.
🤖 Prompt for AI Agents
In app/api/integrations/slack/oauth/authorize/route.ts around lines 27 to 34,
the code uses prisma.userTeam.findUnique with a composite key userId_teamId but
the UserTeam model in prisma/schema/team.prisma does not define
@@unique([userId, teamId]), so update either the schema or the query: preferred
fix — add @@unique([userId, teamId]) to the UserTeam model in
prisma/schema/team.prisma and run prisma migrate to make the composite unique
valid; alternative quick fix — change the call in route.ts to
prisma.userTeam.findFirst({ where: { userId, teamId } }) (or query by an actual
unique field) so the query matches the schema.
| } | ||
|
|
||
| const oauthUrl = await getSlackInstallationUrl(teamId); | ||
| console.log("oauthUrl", oauthUrl); |
There was a problem hiding this comment.
Do not log the OAuth URL (leaks state).
oauthUrl includes the state param. Avoid logging it.
- console.log("oauthUrl", oauthUrl);
+ // Avoid logging oauthUrl; it contains sensitive `state`.📝 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.
| console.log("oauthUrl", oauthUrl); | |
| // Avoid logging oauthUrl; it contains sensitive `state`. |
🤖 Prompt for AI Agents
In app/api/integrations/slack/oauth/authorize/route.ts around line 41, remove
the console.log("oauthUrl", oauthUrl) which prints the OAuth URL containing the
state param; instead either remove the logging entirely or log a non-sensitive
placeholder (e.g., "oauthUrl generated" or only log safe parts like the provider
name) and if you must inspect the URL in development use a secure debug flag
that strips or redacts the state and query params before logging.
| } catch (error) { | ||
| console.error("Slack OAuth authorization error:", error); | ||
| return NextResponse.json( | ||
| { error: "Internal server error" }, | ||
| { status: 500 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return 400 on validation errors instead of 500.
Handle Zod parsing failures explicitly.
- } catch (error) {
+ } catch (error) {
+ if (error instanceof z.ZodError) {
+ return NextResponse.json({ error: "Invalid request" }, { status: 400 });
+ }
console.error("Slack OAuth authorization error:", error);
return NextResponse.json(
{ error: "Internal server error" },
{ status: 500 },
);
}📝 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.
| } catch (error) { | |
| console.error("Slack OAuth authorization error:", error); | |
| return NextResponse.json( | |
| { error: "Internal server error" }, | |
| { status: 500 }, | |
| ); | |
| } | |
| } catch (error) { | |
| if (error instanceof z.ZodError) { | |
| return NextResponse.json({ error: "Invalid request" }, { status: 400 }); | |
| } | |
| console.error("Slack OAuth authorization error:", error); | |
| return NextResponse.json( | |
| { error: "Internal server error" }, | |
| { status: 500 }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In app/api/integrations/slack/oauth/authorize/route.ts around lines 46 to 52,
the catch block currently returns a 500 for all errors; update it to detect Zod
validation failures and return a 400 instead. Import ZodError from "zod" (or use
error.name === "ZodError"), and in the catch: if the error is a ZodError return
NextResponse.json({ error: "Invalid request", details: /* error issues or
message */ }, { status: 400 }); otherwise log the error and return the existing
500 response; ensure logs distinguish validation vs internal errors.
| import { getSession } from "next-auth/react"; | ||
| import z from "zod"; |
There was a problem hiding this comment.
Use getServerSession(authOptions) in App Router route.
getSession from next-auth/react is for client; it will not work reliably in a server route.
-import { getSession } from "next-auth/react";
+import { getServerSession } from "next-auth/next";
+import { authOptions } from "@/pages/api/auth/[...nextauth]";- const session = await getSession();
+ const session = await getServerSession(authOptions);📝 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.
| import { getSession } from "next-auth/react"; | |
| import z from "zod"; | |
| import { getServerSession } from "next-auth/next"; | |
| import { authOptions } from "@/pages/api/auth/[...nextauth]"; | |
| import z from "zod"; |
🤖 Prompt for AI Agents
In app/api/integrations/slack/oauth/callback/route.ts around lines 5-6, the code
imports getSession from next-auth/react which is client-side; replace it with
getServerSession from next-auth and call getServerSession(authOptions) in this
server route. Import or reference your authOptions (e.g., from your
[...nextauth] config) and use await getServerSession(req, res, authOptions) or
the appropriate signature for your Next.js version, remove the getSession
import/usages, and ensure the route handler runs on the server so the
server-side session is retrieved reliably.
| const data = await response.json(); | ||
|
|
||
| console.log("data", data); | ||
|
|
||
| const credentials: SlackCredential = { | ||
| appId: data.app_id, | ||
| botUserId: data.bot_user_id, | ||
| scope: data.scope, | ||
| accessToken: encryptSlackToken(data.access_token), | ||
| tokenType: data.token_type, | ||
| authUser: data.authed_user, | ||
| team: data.team, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate Slack response and avoid logging secrets.
Check data.ok, surface Slack errors, and stop logging the full payload which contains tokens.
- const data = await response.json();
-
- console.log("data", data);
-
- const credentials: SlackCredential = {
- appId: data.app_id,
- botUserId: data.bot_user_id,
- scope: data.scope,
- accessToken: encryptSlackToken(data.access_token),
- tokenType: data.token_type,
- authUser: data.authed_user,
- team: data.team,
- };
+ const data = await response.json();
+ if (!data?.ok) {
+ throw new Error(`Slack OAuth failed: ${data?.error ?? "unknown_error"}`);
+ }
+ const {
+ app_id,
+ bot_user_id,
+ scope,
+ access_token,
+ token_type,
+ authed_user,
+ team,
+ } = data;
+ const credentials: SlackCredential = {
+ appId: app_id,
+ botUserId: bot_user_id,
+ scope,
+ accessToken: encryptSlackToken(access_token),
+ tokenType: token_type,
+ authUser: authed_user,
+ team,
+ };📝 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 data = await response.json(); | |
| console.log("data", data); | |
| const credentials: SlackCredential = { | |
| appId: data.app_id, | |
| botUserId: data.bot_user_id, | |
| scope: data.scope, | |
| accessToken: encryptSlackToken(data.access_token), | |
| tokenType: data.token_type, | |
| authUser: data.authed_user, | |
| team: data.team, | |
| }; | |
| const data = await response.json(); | |
| if (!data?.ok) { | |
| throw new Error(`Slack OAuth failed: ${data?.error ?? "unknown_error"}`); | |
| } | |
| const { | |
| app_id, | |
| bot_user_id, | |
| scope, | |
| access_token, | |
| token_type, | |
| authed_user, | |
| team, | |
| } = data; | |
| const credentials: SlackCredential = { | |
| appId: app_id, | |
| botUserId: bot_user_id, | |
| scope, | |
| accessToken: encryptSlackToken(access_token), | |
| tokenType: token_type, | |
| authUser: authed_user, | |
| team, | |
| }; |
🤖 Prompt for AI Agents
In app/api/integrations/slack/oauth/callback/route.ts around lines 70 to 82, the
code currently parses and logs the full Slack response and assumes success;
instead check if data.ok is true and if not throw or return an error that
surfaces data.error (and any useful error_description) to the caller; remove the
console.log("data", data) call so tokens and secrets are not printed; validate
required fields (access_token, app_id, team, etc.) before building
SlackCredential and return a clear error if any are missing; keep
encryptSlackToken for the token but never log the raw or encrypted token.
| const validationResult = slackIntegrationUpdateSchema.safeParse(req.body); | ||
|
|
||
| if (!validationResult.success) { | ||
| return res.status(400).json({ | ||
| error: "Invalid request payload", | ||
| details: validationResult.error.errors, | ||
| }); | ||
| } | ||
|
|
||
| const { enabled, enabledChannels } = validationResult.data; | ||
|
|
||
| if (enabledChannels && Object.keys(validationResult.data).length === 1) { | ||
| await prisma.installedIntegration.update({ | ||
| where: { | ||
| teamId_integrationId: { | ||
| teamId, | ||
| integrationId: env.SLACK_INTEGRATION_ID, | ||
| }, | ||
| }, | ||
| data: { configuration: { enabledChannels } }, | ||
| }); | ||
|
|
||
| return res.status(200).json({ | ||
| success: true, | ||
| enabledChannels, | ||
| updatedAt: new Date().toISOString(), | ||
| }); | ||
| } | ||
|
|
||
| const updateData: { | ||
| enabled?: boolean; | ||
| enabledChannels?: Record<string, boolean>; | ||
| } = {}; | ||
|
|
||
| if (enabled !== undefined) updateData.enabled = enabled; | ||
|
|
||
| if (enabledChannels) updateData.enabledChannels = enabledChannels; | ||
|
|
||
| if (Object.keys(updateData).length === 0) { | ||
| return res.status(400).json({ error: "No fields to update" }); | ||
| } | ||
|
|
||
| const updatedIntegration = await prisma.installedIntegration.update({ | ||
| where: { | ||
| teamId_integrationId: { | ||
| teamId, | ||
| integrationId: env.SLACK_INTEGRATION_ID, | ||
| }, | ||
| }, | ||
| data: updateData, | ||
| select: { | ||
| id: true, | ||
| credentials: true, | ||
| configuration: true, | ||
| enabled: true, | ||
| createdAt: true, | ||
| updatedAt: true, | ||
| }, | ||
| }); | ||
|
|
||
| return res.status(200).json(updatedIntegration); |
There was a problem hiding this comment.
Fix update logic: writes to a non-existent column and can clobber configuration; also no 404 when missing.
- Bug:
updateData.enabledChannelstargets a top-level column that doesn't exist.enabledChannelslives underconfiguration. - Inconsistent branches: special-case path updates JSON, generic path writes invalid top-level field.
- No existence check: update will throw, returning 500 instead of 404.
Replace the whole update section with a merge-into-JSON update and proper 404:
const env = getSlackEnv();
try {
- const validationResult = slackIntegrationUpdateSchema.safeParse(req.body);
+ const validationResult = slackIntegrationUpdateSchema.safeParse(req.body);
if (!validationResult.success) {
return res.status(400).json({
error: "Invalid request payload",
details: validationResult.error.errors,
});
}
- const { enabled, enabledChannels } = validationResult.data;
-
- if (enabledChannels && Object.keys(validationResult.data).length === 1) {
- await prisma.installedIntegration.update({
- where: {
- teamId_integrationId: {
- teamId,
- integrationId: env.SLACK_INTEGRATION_ID,
- },
- },
- data: { configuration: { enabledChannels } },
- });
-
- return res.status(200).json({
- success: true,
- enabledChannels,
- updatedAt: new Date().toISOString(),
- });
- }
-
- const updateData: {
- enabled?: boolean;
- enabledChannels?: Record<string, boolean>;
- } = {};
-
- if (enabled !== undefined) updateData.enabled = enabled;
-
- if (enabledChannels) updateData.enabledChannels = enabledChannels;
-
- if (Object.keys(updateData).length === 0) {
- return res.status(400).json({ error: "No fields to update" });
- }
-
- const updatedIntegration = await prisma.installedIntegration.update({
+ const { enabled, enabledChannels } = validationResult.data;
+
+ const existing = await prisma.installedIntegration.findUnique({
+ where: {
+ teamId_integrationId: {
+ teamId,
+ integrationId: env.SLACK_INTEGRATION_ID,
+ },
+ },
+ select: { configuration: true },
+ });
+ if (!existing) {
+ return res.status(404).json({ error: "Slack integration not found" });
+ }
+
+ const nextConfig =
+ enabledChannels != null
+ ? { ...(existing.configuration || {}), enabledChannels }
+ : undefined;
+
+ if (enabled === undefined && nextConfig === undefined) {
+ return res.status(400).json({ error: "No fields to update" });
+ }
+
+ const updatedIntegration = await prisma.installedIntegration.update({
where: {
teamId_integrationId: {
teamId,
integrationId: env.SLACK_INTEGRATION_ID,
},
},
- data: updateData,
+ data: {
+ ...(enabled !== undefined ? { enabled } : {}),
+ ...(nextConfig ? { configuration: { set: nextConfig } } : {}),
+ },
select: {
id: true,
- credentials: true,
configuration: true,
enabled: true,
createdAt: true,
updatedAt: true,
},
});
return res.status(200).json(updatedIntegration);📝 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 validationResult = slackIntegrationUpdateSchema.safeParse(req.body); | |
| if (!validationResult.success) { | |
| return res.status(400).json({ | |
| error: "Invalid request payload", | |
| details: validationResult.error.errors, | |
| }); | |
| } | |
| const { enabled, enabledChannels } = validationResult.data; | |
| if (enabledChannels && Object.keys(validationResult.data).length === 1) { | |
| await prisma.installedIntegration.update({ | |
| where: { | |
| teamId_integrationId: { | |
| teamId, | |
| integrationId: env.SLACK_INTEGRATION_ID, | |
| }, | |
| }, | |
| data: { configuration: { enabledChannels } }, | |
| }); | |
| return res.status(200).json({ | |
| success: true, | |
| enabledChannels, | |
| updatedAt: new Date().toISOString(), | |
| }); | |
| } | |
| const updateData: { | |
| enabled?: boolean; | |
| enabledChannels?: Record<string, boolean>; | |
| } = {}; | |
| if (enabled !== undefined) updateData.enabled = enabled; | |
| if (enabledChannels) updateData.enabledChannels = enabledChannels; | |
| if (Object.keys(updateData).length === 0) { | |
| return res.status(400).json({ error: "No fields to update" }); | |
| } | |
| const updatedIntegration = await prisma.installedIntegration.update({ | |
| where: { | |
| teamId_integrationId: { | |
| teamId, | |
| integrationId: env.SLACK_INTEGRATION_ID, | |
| }, | |
| }, | |
| data: updateData, | |
| select: { | |
| id: true, | |
| credentials: true, | |
| configuration: true, | |
| enabled: true, | |
| createdAt: true, | |
| updatedAt: true, | |
| }, | |
| }); | |
| return res.status(200).json(updatedIntegration); | |
| const validationResult = slackIntegrationUpdateSchema.safeParse(req.body); | |
| if (!validationResult.success) { | |
| return res.status(400).json({ | |
| error: "Invalid request payload", | |
| details: validationResult.error.errors, | |
| }); | |
| } | |
| const { enabled, enabledChannels } = validationResult.data; | |
| const existing = await prisma.installedIntegration.findUnique({ | |
| where: { | |
| teamId_integrationId: { | |
| teamId, | |
| integrationId: env.SLACK_INTEGRATION_ID, | |
| }, | |
| }, | |
| select: { configuration: true }, | |
| }); | |
| if (!existing) { | |
| return res.status(404).json({ error: "Slack integration not found" }); | |
| } | |
| const nextConfig = | |
| enabledChannels != null | |
| ? { ...(existing.configuration || {}), enabledChannels } | |
| : undefined; | |
| if (enabled === undefined && nextConfig === undefined) { | |
| return res.status(400).json({ error: "No fields to update" }); | |
| } | |
| const updatedIntegration = await prisma.installedIntegration.update({ | |
| where: { | |
| teamId_integrationId: { | |
| teamId, | |
| integrationId: env.SLACK_INTEGRATION_ID, | |
| }, | |
| }, | |
| data: { | |
| ...(enabled !== undefined ? { enabled } : {}), | |
| ...(nextConfig ? { configuration: { set: nextConfig } } : {}), | |
| }, | |
| select: { | |
| id: true, | |
| configuration: true, | |
| enabled: true, | |
| createdAt: true, | |
| updatedAt: true, | |
| }, | |
| }); | |
| return res.status(200).json(updatedIntegration); |
| import { useCallback, useEffect, useMemo, useState } from "react"; | ||
|
|
||
| import { useTeam } from "@/context/team-context"; | ||
| import { CircleHelpIcon, Hash, Settings, XCircleIcon } from "lucide-react"; |
There was a problem hiding this comment.
Fix lucide-react imports and components; current names don’t exist.
lucide-react exports icons without the “Icon” suffix. Use CircleHelp and XCircle.
-import { CircleHelpIcon, Hash, Settings, XCircleIcon } from "lucide-react";
+import { CircleHelp, Hash, Settings, XCircle } from "lucide-react";- <CircleHelpIcon className="h-4 w-4 shrink-0 text-muted-foreground hover:text-foreground" />
+ <CircleHelp className="h-4 w-4 shrink-0 text-muted-foreground hover:text-foreground" />- <XCircleIcon className="h-4 w-4" />
+ <XCircle className="h-4 w-4" />Also applies to: 466-467, 483-483
🤖 Prompt for AI Agents
In pages/settings/slack.tsx around line 6 (and also check lines 466-467 and
483), the code imports and uses lucide-react icons with an "Icon" suffix that
doesn't exist; update the import to use the correct exported names (e.g., import
CircleHelp and XCircle instead of CircleHelpIcon and XCircleIcon) and replace
any JSX usages of <CircleHelpIcon /> and <XCircleIcon /> with <CircleHelp /> and
<XCircle /> respectively, ensuring any props passed remain unchanged.
| // Redirect if Slack feature is not enabled | ||
| useEffect(() => { | ||
| if (!isFeatureEnabled("slack")) { | ||
| router.push("/settings/general"); | ||
| toast.error("This feature is not available for your team"); | ||
| } | ||
| }, [isFeatureEnabled, router]); |
There was a problem hiding this comment.
Avoid false redirects: gate feature-flag redirect on loading state.
isFeatureEnabled("slack") returns false while flags load, causing unwanted redirect. Wait for flags to load and use replace to avoid back-button loops.
- const { isFeatureEnabled } = useFeatureFlags();
+ const { isFeatureEnabled, isLoading } = useFeatureFlags();
...
- useEffect(() => {
- if (!isFeatureEnabled("slack")) {
- router.push("/settings/general");
- toast.error("This feature is not available for your team");
- }
- }, [isFeatureEnabled, router]);
+ useEffect(() => {
+ if (isLoading) return;
+ if (!isFeatureEnabled("slack")) {
+ toast.error("This feature is not available for your team");
+ router.replace("/settings/general");
+ }
+ }, [isLoading, isFeatureEnabled, router]);Also applies to: 9-13
🤖 Prompt for AI Agents
In pages/settings/slack.tsx around lines 121-127, the redirect runs while
feature flags are still loading causing false redirects; update the effect to
wait for the feature-flag loading state (e.g., isLoading or flagsLoaded) and
only redirect when flags have finished loading and isFeatureEnabled("slack") is
false, use router.replace("/settings/general") instead of push to avoid
back-button loops, and include the loading state in the dependency array (e.g.,
[isFeatureEnabled, router, isFeatureFlagsLoaded]).
| const handleDisconnect = async () => { | ||
| const disconnectPromise = async () => { | ||
| const response = await fetch(`/api/teams/${teamId}/integrations/slack`, { | ||
| method: "DELETE", | ||
| }); | ||
|
|
||
| if (response.ok) { | ||
| mutateIntegration(undefined, false); | ||
| } else { | ||
| const data = await response.json(); | ||
| throw new Error(data.error || "Failed to disconnect Slack"); | ||
| } | ||
| }; | ||
|
|
||
| toast.promise(disconnectPromise(), { | ||
| loading: "Disconnecting Slack integration...", | ||
| success: "Slack integration disconnected successfully", | ||
| error: "Failed to disconnect Slack integration. Please try again.", | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing teamId in disconnect flow.
Prevent “undefined” API path and provide user feedback.
- const handleDisconnect = async () => {
+ const handleDisconnect = async () => {
+ if (!teamId) {
+ toast.error("No team selected");
+ return;
+ }
const disconnectPromise = async () => {
const response = await fetch(`/api/teams/${teamId}/integrations/slack`, {
method: "DELETE",
});
if (response.ok) {
mutateIntegration(undefined, false);
+ // Optional: clear channels cache as well
+ // mutateChannels({ channels: [] }, false);
} else {
const data = await response.json();
throw new Error(data.error || "Failed to disconnect Slack");
}
};
toast.promise(disconnectPromise(), {📝 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 handleDisconnect = async () => { | |
| const disconnectPromise = async () => { | |
| const response = await fetch(`/api/teams/${teamId}/integrations/slack`, { | |
| method: "DELETE", | |
| }); | |
| if (response.ok) { | |
| mutateIntegration(undefined, false); | |
| } else { | |
| const data = await response.json(); | |
| throw new Error(data.error || "Failed to disconnect Slack"); | |
| } | |
| }; | |
| toast.promise(disconnectPromise(), { | |
| loading: "Disconnecting Slack integration...", | |
| success: "Slack integration disconnected successfully", | |
| error: "Failed to disconnect Slack integration. Please try again.", | |
| }); | |
| }; | |
| const handleDisconnect = async () => { | |
| if (!teamId) { | |
| toast.error("No team selected"); | |
| return; | |
| } | |
| const disconnectPromise = async () => { | |
| const response = await fetch(`/api/teams/${teamId}/integrations/slack`, { | |
| method: "DELETE", | |
| }); | |
| if (response.ok) { | |
| mutateIntegration(undefined, false); | |
| // Optional: clear channels cache as well | |
| // mutateChannels({ channels: [] }, false); | |
| } else { | |
| const data = await response.json(); | |
| throw new Error(data.error || "Failed to disconnect Slack"); | |
| } | |
| }; | |
| toast.promise(disconnectPromise(), { | |
| loading: "Disconnecting Slack integration...", | |
| success: "Slack integration disconnected successfully", | |
| error: "Failed to disconnect Slack integration. Please try again.", | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In pages/settings/slack.tsx around lines 153 to 172, the disconnect flow can
call the API with an undefined teamId which produces an invalid URL and poor UX;
guard by checking teamId before making the request and aborting with user
feedback. Implement an early check that if (!teamId) then call
toast.error("Unable to disconnect: missing team ID") (or similar) and return
without invoking disconnectPromise; alternatively incorporate the check inside
disconnectPromise to throw a descriptive error before calling fetch, and ensure
toast.promise receives that rejection so the user sees the message instead of
attempting a DELETE to /api/teams/undefined/....
| -- CreateTable | ||
| CREATE TABLE "InstalledIntegration" ( | ||
| "id" TEXT NOT NULL, | ||
| "credentials" JSONB, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not store OAuth credentials in plaintext JSON. Encrypt or segregate secrets.
Storing tokens in JSONB is a high-impact security risk. Prefer application-level envelope encryption (KMS/HSM) and store ciphertext+nonce, or move credentials to a dedicated secrets table with stricter access. At minimum, ensure tokens are encrypted before persisting.
I can draft a follow-up migration and helper to write/read encrypted blobs if helpful.
Also applies to: 11-11
2 participants
Summary by CodeRabbit