feat(ee): granular permissions #1699
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a new granular permission system for dataroom links, centered around "Permission Groups" and their associated access controls. It adds new database tables, API endpoints, React components, and backend logic to support assigning, editing, and enforcing file-level permissions for dataroom links, alongside legacy group-based permissions. The UI and API are updated to handle both permission group and legacy group models. Changes
Sequence Diagram(s)Dataroom Link Creation and Permission Group AssignmentsequenceDiagram
participant User
participant DataroomLinkSheet (FE)
participant API /teams/[teamId]/datarooms/[id]/permission-groups/index.ts
participant DB
User->>DataroomLinkSheet (FE): Fill link form, set permissions
DataroomLinkSheet (FE)->>API /teams/.../permission-groups/index.ts: POST create permission group (with permissions)
API /teams/.../permission-groups/index.ts->>DB: Create PermissionGroup, PermissionGroupAccessControls
API /teams/.../permission-groups/index.ts-->>DataroomLinkSheet (FE): Return created group and controls
DataroomLinkSheet (FE)->>API /links/[id]/index.ts: PUT update link with permissionGroupId
API /links/[id]/index.ts->>DB: Update Link.permissionGroupId
API /links/[id]/index.ts-->>DataroomLinkSheet (FE): Return updated link
DataroomLinkSheet (FE)-->>User: Show success sheet with permissions summary
Dataroom Link Access and Permission ChecksequenceDiagram
participant Viewer
participant Frontend
participant API /links/domains/[...domainSlug].ts
participant lib/api/links/link-data.ts
participant DB
Viewer->>Frontend: Access dataroom link
Frontend->>API /links/domains/[...domainSlug].ts: GET link data
API /links/domains/[...domainSlug].ts->>lib/api/links/link-data.ts: fetchDataroomLinkData({ groupId, permissionGroupId })
lib/api/links/link-data.ts->>DB: Query Link, PermissionGroup, AccessControls
lib/api/links/link-data.ts-->>API /links/domains/[...domainSlug].ts: Return link data with accessControls
API /links/domains/[...domainSlug].ts-->>Frontend: Return link data
Frontend-->>Viewer: Render dataroom view with permissions enforced
Editing File Permissions for a Dataroom LinksequenceDiagram
participant User
participant LinksTable (FE)
participant PermissionsSheet (FE)
participant API /teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts
participant DB
User->>LinksTable (FE): Click "Edit File Permissions"
LinksTable (FE)->>PermissionsSheet (FE): Open permission sheet for link
User->>PermissionsSheet (FE): Modify permissions, save
PermissionsSheet (FE)->>API /teams/.../permission-groups/[permissionGroupId].ts: PUT update access controls
API /teams/.../permission-groups/[permissionGroupId].ts->>DB: Update PermissionGroupAccessControls (batch)
API /teams/.../permission-groups/[permissionGroupId].ts-->>PermissionsSheet (FE): Return updated controls
PermissionsSheet (FE)-->>LinksTable (FE): Notify permissions updated
LinksTable (FE)-->>User: Show success toast, refresh link list
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (4)
pages/api/links/[id]/documents/[documentId].ts (1)
94-103: 🛠️ Refactor suggestionPotential precedence clash between
groupIdandpermissionGroupId
fetchDataroomDocumentLinkDatamay now receive bothgroupIdandpermissionGroupIdsimultaneously (if the link was migrated and still has both ids).
Ensure that function defines a clear precedence rule; otherwise ambiguous access-control evaluation is possible.app/api/views-dataroom/route.ts (1)
858-909:⚠️ Potential issueComposite key supplied incorrectly – may throw Prisma validation error
findUniquemust receive exactly the unique/composite key object – extra fields are not allowed.
If the model has
@@unique([groupId, itemId, itemType]), the correct call is:-await prisma.viewerGroupAccessControls.findUnique({ - where: { - groupId_itemId: { - groupId: link.groupId, - itemId: dataroomDocument.id, - }, - itemType: ItemType.DATAROOM_DOCUMENT, - }, +await prisma.viewerGroupAccessControls.findUnique({ + where: { + groupId_itemId_itemType: { + groupId: link.groupId, + itemId: dataroomDocument.id, + itemType: ItemType.DATAROOM_DOCUMENT, + }, }, select: { canDownload: true }, });The same applies to
permissionGroupAccessControls. As written, Prisma will throw
Unknown arg 'itemType'at runtime.pages/api/links/[id]/index.ts (1)
328-337:⚠️ Potential issueValidate
permissionGroupIdbefore persisting – potential privilege-escalation
permissionGroupIdcoming from the client is written straight to the DB without confirming that
- the group exists,
- it belongs to the same team and dataroom as the link, and
- the current user has access to it.
An attacker could point a link to an arbitrary permission group and silently gain (or revoke) access to items outside their scope.
+// ✱ Before calling tx.link.update +if (linkData.permissionGroupId) { + const pg = await tx.permissionGroup.findUnique({ + where: { id: linkData.permissionGroupId, teamId }, + select: { dataroomId: true }, + }); + if (!pg || (!dataroomLink || pg.dataroomId !== targetId)) { + throw new Error("Invalid permissionGroupId for this team/dataroom"); + } +}pages/api/links/download/dataroom-document.ts (1)
118-149:⚠️ Potential issueGuard against empty
downloadDocumentsAfter filtering by group permissions the array may be empty, but later code dereferences
downloadDocuments[0], causing a 500. Return 403 when no permitted docs remain.
♻️ Duplicate comments (1)
lib/webhook/triggers/link-created.ts (1)
96-99: Same compatibility caveat applies here
permissionGroupIdis added to thelink.createdpayload as well – be sure to keep the payload contract in sync with documentation / consumers (see comment above).
🧹 Nitpick comments (19)
components/links/link-sheet/upload-section/index.tsx (1)
199-205: Hard-coded plan strings – easy to drift
requiredPlan="data rooms plus"(lower-case) andplan: "Data Rooms Plus"(title-case) are now duplicated literals.
Consider centralising plan identifiers/labels in a shared constant enum to avoid typo-driven bugs and simplify future renames:-import { PLAN_DATA_ROOMS_PLUS_ID, PLAN_DATA_ROOMS_PLUS_LABEL } from "@/lib/plans"; +import { PLAN_DR_PLUS_ID, PLAN_DR_PLUS_LABEL } from "@/lib/plans"; - requiredPlan="data rooms plus" + requiredPlan={PLAN_DR_PLUS_ID} - plan: "Data Rooms Plus", + plan: PLAN_DR_PLUS_LABEL,components/links/link-sheet/dataroom-link-sheet.tsx (1)
1-7: Avoidanyand the extra wrapper – re-export with correct typingThe wrapper introduces an
anyleak and an extra render layer. You can preserve type-safety and tree-shaking by re-exporting the EE component directly (or at least typing the props).-"use client"; - -import { DataroomLinkSheet as DataroomLinkSheetEE } from "@/ee/features/permissions/components/dataroom-link-sheet"; - -export function DataroomLinkSheet(props: any) { - return <DataroomLinkSheetEE {...props} />; -} +// “use client” (keep if required) +import { DataroomLinkSheet as DataroomLinkSheetEE } from "@/ee/features/permissions/components/dataroom-link-sheet"; +import { ComponentProps } from "react"; + +export type DataroomLinkSheetProps = ComponentProps<typeof DataroomLinkSheetEE>; + +export function DataroomLinkSheet(props: DataroomLinkSheetProps) { + return <DataroomLinkSheetEE {...props} />; +} + +// or simply: +// export { DataroomLinkSheetEE as DataroomLinkSheet };lib/types.ts (1)
156-159: Prefer a union inside the array for easier consumption
ViewerGroupAccessControls[] | PermissionGroupAccessControls[]forces callers to narrow
the whole array first.
Using an element-level union makes common operations (e.g..map) type-safe without extra guards.- accessControls?: - | ViewerGroupAccessControls[] - | PermissionGroupAccessControls[]; + accessControls?: ( + | ViewerGroupAccessControls + | PermissionGroupAccessControls + )[];components/datarooms/dataroom-header.tsx (1)
9-17: Minor typings / API nits
linkType={"DATAROOM_LINK"}– iflinkTypeis a string-literal union consider exporting a constant/enum to avoid typos.- The surrounding
<Button key={1}>doesn’t need akeyprop; keys are only meaningful in arrays.No blockers, just polish.
Also applies to: 72-77
components/links/link-sheet/permissions-sheet.tsx (1)
5-7: Avoidany; preserve the strong typing the EE component already exposesLeaking
anyhere drops all compile-time safety for callers and defeats the purpose of the thin wrapper. Re-export the existing prop type instead:-import { PermissionsSheet as PermissionsSheetEE } from "@/ee/features/permissions/components/permissions-sheet"; - -export function PermissionsSheet(props: any) { - return <PermissionsSheetEE {...props} />; -} +import { PermissionsSheet as PermissionsSheetEE } from "@/ee/features/permissions/components/permissions-sheet"; + +type PermissionsSheetProps = React.ComponentProps<typeof PermissionsSheetEE>; + +export function PermissionsSheet(props: PermissionsSheetProps) { + return <PermissionsSheetEE {...props} />; +}This keeps the public API of the wrapper in lock-step with the enterprise component and gives consumers full IntelliSense.
components/view/viewer/dataroom-viewer.tsx (1)
105-106: Use an array of union instead of a union of arrays
ViewerGroupAccessControls[] | PermissionGroupAccessControls[]forbids mixed elements and forces downstream code to perform narrowing.
If mixed controls are ever returned (e.g. future migration period) you’ll get
Property 'itemId' does not exist on type ….-accessControls: ViewerGroupAccessControls[] | PermissionGroupAccessControls[]; +accessControls: (ViewerGroupAccessControls | PermissionGroupAccessControls)[];Even if today only one flavour is sent, the broader type costs nothing and removes the need for casts later.
pages/api/links/domains/[...domainSlug].ts (1)
176-178: Avoid mutatinglinkDatain-place
linkData.accessControls = …breaks immutability assumptions and makes it harder to reason about the provenance of fields.- linkData = data.linkData; - brand = data.brand; - // Include access controls in the link data for the frontend - linkData.accessControls = data.accessControls; + ({ linkData, brand } = data); + linkData = { ...linkData, accessControls: data.accessControls };A fresh object keeps side-effects local and avoids accidental bleed-through if
data.linkDatais reused elsewhere.app/api/views-dataroom/route.ts (1)
858-866: Duplicate logic – extract a helperThe if/else blocks perform identical work against two tables.
A small helper reduces branching and future drift:const canDownloadForGroup = async ( table: "viewer" | "permission", groupId: string, itemId: string, ) => { const repo = table === "viewer" ? prisma.viewerGroupAccessControls : prisma.permissionGroupAccessControls; const rec = await repo.findUnique({ where: { groupId_itemId_itemType: { groupId, itemId, itemType: ItemType.DATAROOM_DOCUMENT, }, }, select: { canDownload: true }, }); return rec?.canDownload ?? false; };Then:
canDownload = await canDownloadForGroup( link.groupId ? "viewer" : "permission", effectiveGroupId, dataroomDocument.id, );pages/api/links/[id]/index.ts (1)
127-129:linkData.accessControlsmutates fetched data in-place
fetchDataroomLinkDatamost likely returns an object reused elsewhere; adding a new property in-place (linkData.accessControls = …) risks side-effects or React state-mutation warnings on the client.Safer: create a fresh object.
- linkData.accessControls = data.accessControls; + linkData = { ...linkData, accessControls: data.accessControls };pages/api/links/download/dataroom-folder.ts (1)
134-150: Duplicate branching & unusedeffectiveGroupId
effectiveGroupIdis computed but never consumed.
At the same time the two separate queries differ only by table name.
You can remove the duplication and the dead variable:-const effectiveGroupId = view.groupId || view.link.permissionGroupId; - -if (effectiveGroupId) { - let groupPermissions: any[] = []; - if (view.groupId) { - groupPermissions = await prisma.viewerGroupAccessControls.findMany({ … }); - } else if (view.link.permissionGroupId) { - groupPermissions = await prisma.permissionGroupAccessControls.findMany({ … }); - } +const groupPermissions = + !view.groupId && !view.link.permissionGroupId + ? [] + : await (view.groupId + ? prisma.viewerGroupAccessControls + : prisma.permissionGroupAccessControls).findMany({ + where: { + groupId: view.groupId ?? view.link.permissionGroupId, + canDownload: true, + }, + });Cleaner and easier to keep in sync.
pages/api/links/download/bulk.ts (1)
119-139: Same branching duplication as indataroom-folder.tsConsider extracting the common “fetch permissions by group” logic to avoid drift between the two endpoints.
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts (1)
211-229: N ×updateManycan be replaced with a single bulk callRunning one
updateManyper item scales poorly. Use a singlecreateMany({ skipDuplicates:… })+deleteManyor Prismaupsertto cut query count.pages/api/links/download/dataroom-document.ts (2)
126-138: Duplicate code path for legacy vs new groupsThe two queries only differ by table name. Consider a helper that selects the correct model based on group type to keep this block DRY.
140-144: Lose theany
groupPermissionsandpermissionare typed asany. Add proper Prisma‐generated types to catch enum/value mismatches at compile time.components/links/link-sheet/link-success-sheet.tsx (1)
50-54:domainSlugmay be undefined
link.domainSlugisn’t guaranteed onLinkWithViews. Fallback tolink.domain?.slugor guard to avoidundefinedin URL.components/links/link-sheet/link-options.tsx (1)
60-73: Minor UX: keyboard accessibility for collapsible headers
CollapsibleTriggerlacksrole="button"/aria-expandedattributes, making it less accessible to screen-reader users.ee/features/permissions/components/permissions-sheet.tsx (2)
190-190: Simplify the boolean expression.The ternary operator is unnecessary here.
Apply this diff to simplify the expression:
- view: permission ? permission.canView : hasPermissionData ? false : true, + view: permission ? permission.canView : !hasPermissionData,🧰 Tools
🪛 Biome (1.9.4)
[error] 190-190: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
256-261: Simplify the document filtering condition.The current condition is redundant and can be simplified for better readability.
Apply this diff to simplify the logic:
- items - .filter( - (item) => - (item.parentId === parentId && item.document) || - (parentId === null && item.folderId === null && item.document), - ) + items + .filter((item) => { + if (item.document) { + return parentId === null ? item.folderId === null : item.parentId === parentId; + } + return false; + })ee/features/permissions/components/dataroom-link-sheet.tsx (1)
243-263: Use optional chaining for cleaner code.Replace logical AND operators with optional chaining for better readability.
Apply this diff to use optional chaining:
- let blobUrl: string | null = - linkData.metaImage && linkData.metaImage.startsWith("data:") - ? null - : linkData.metaImage; - if (linkData.metaImage && linkData.metaImage.startsWith("data:")) { + let blobUrl: string | null = + linkData.metaImage?.startsWith("data:") + ? null + : linkData.metaImage; + if (linkData.metaImage?.startsWith("data:")) { // Convert the data URL to a blob const blob = convertDataUrlToFile({ dataUrl: linkData.metaImage }); // Upload the blob to vercel storage blobUrl = await uploadImage(blob); } // Upload meta favicon if it's a data URL - let blobUrlFavicon: string | null = - linkData.metaFavicon && linkData.metaFavicon.startsWith("data:") - ? null - : linkData.metaFavicon; - if (linkData.metaFavicon && linkData.metaFavicon.startsWith("data:")) { + let blobUrlFavicon: string | null = + linkData.metaFavicon?.startsWith("data:") + ? null + : linkData.metaFavicon; + if (linkData.metaFavicon?.startsWith("data:")) {🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
app/api/views-dataroom/route.ts(3 hunks)components/datarooms/dataroom-header.tsx(2 hunks)components/links/link-sheet/dataroom-link-sheet.tsx(1 hunks)components/links/link-sheet/index.tsx(4 hunks)components/links/link-sheet/link-options.tsx(8 hunks)components/links/link-sheet/link-success-sheet.tsx(1 hunks)components/links/link-sheet/og-section.tsx(1 hunks)components/links/link-sheet/permissions-sheet.tsx(1 hunks)components/links/link-sheet/upload-section/index.tsx(1 hunks)components/links/links-table.tsx(8 hunks)components/view/dataroom/dataroom-view.tsx(1 hunks)components/view/viewer/dataroom-viewer.tsx(2 hunks)ee/features/permissions/components/dataroom-link-sheet.tsx(1 hunks)ee/features/permissions/components/permissions-sheet.tsx(1 hunks)lib/api/links/link-data.ts(6 hunks)lib/api/views/send-webhook-event.ts(1 hunks)lib/types.ts(2 hunks)lib/webhook/triggers/link-created.ts(1 hunks)pages/api/links/[id]/documents/[documentId].ts(2 hunks)pages/api/links/[id]/index.ts(3 hunks)pages/api/links/domains/[...domainSlug].ts(3 hunks)pages/api/links/download/bulk.ts(2 hunks)pages/api/links/download/dataroom-document.ts(2 hunks)pages/api/links/download/dataroom-folder.ts(2 hunks)pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts(1 hunks)pages/api/teams/[teamId]/datarooms/[id]/permission-groups/index.ts(1 hunks)prisma/migrations/20250612000000_add_permissions_to_link/migration.sql(1 hunks)prisma/schema/dataroom.prisma(2 hunks)prisma/schema/link.prisma(1 hunks)prisma/schema/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
components/datarooms/dataroom-header.tsx (2)
ee/features/permissions/components/dataroom-link-sheet.tsx (1)
DataroomLinkSheet(76-900)components/links/link-sheet/dataroom-link-sheet.tsx (1)
DataroomLinkSheet(5-7)
components/links/link-sheet/permissions-sheet.tsx (1)
ee/features/permissions/components/permissions-sheet.tsx (1)
PermissionsSheet(336-1028)
🪛 Biome (1.9.4)
ee/features/permissions/components/permissions-sheet.tsx
[error] 190-190: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
ee/features/permissions/components/dataroom-link-sheet.tsx
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
lib/api/views/send-webhook-event.ts (1)
97-100: Adding a new field in a public webhook payload can be breaking – consider versioning / announcing
permissionGroupIdis now always included in thelinkobject that is delivered to third-party webhooks.
For consumers performing strict schema validation this is a breaking change unless they explicitly ignore unknown properties.At minimum:
- Announce the change in the changelog / developer docs.
- If you provide webhook “versions”, bump the version and gate the new field behind it.
No code change required if you’re fine with a silent rollout, but please verify downstream impact.
prisma/schema/schema.prisma (1)
82-83: Relation looks good – double-check onDelete behaviour
Team.permissionGroups PermissionGroup[]creates a one-to-many link but the correspondingPermissionGroup.teamrelation (defined in the permission-group model file) should specify anonDelete: Cascade(or the desired rule).
If that optic is missing, dangling permission groups could remain after a team is deleted.team Team @relation(fields: [teamId], references: [id], -onDelete: Cascade?)Please confirm the setting matches your data-retention policy.
pages/api/links/[id]/documents/[documentId].ts (1)
55-57: 👍 Permission-group field correctly included in the selectSelecting
permissionGroupIdearly avoids an extra query later – change looks good.components/view/dataroom/dataroom-view.tsx (1)
268-270:⚠️ Potential issue
||will treat an intentionally empty array as “falsy” – use nullish-coalescing insteadIf
link.accessControlsis an empty array (meaning “no explicit per-link overrides”), the current expression falls back togroup?.accessControls, which is probably not what we want.
Switching to??preserves an empty array while still falling back on the group or[]when the value is trulynull/undefined.- accessControls={link.accessControls || group?.accessControls || []} + accessControls={ + link.accessControls ?? group?.accessControls ?? [] + }Likely an incorrect or invalid review comment.
components/links/link-sheet/og-section.tsx (1)
140-142: Text tweak looks goodTooltip/title wording change is clear and user-friendly; no functional impact.
components/view/viewer/dataroom-viewer.tsx (1)
5-9: Import ordering looks fine
No concerns here.components/links/link-sheet/index.tsx (2)
258-259: Preset application unintentionally resets notification flag tofalse
enableNotificationis now set with!!preset.enableNotification, meaning:
undefined→false(overrides existing user choice)null→falsePreviously it kept the previous value. To restore that behaviour:
- enableNotification: !!preset.enableNotification, + enableNotification: + preset.enableNotification === undefined + ? prev.enableNotification + : preset.enableNotification,
448-448: Sheet width bump – check mobile break-pointsIncreasing the small-screen width from 600 px to 800 px will overflow many phones (< 414 px).
Confirm via responsive testing or addmax-w-fullatsmbreakpoint.prisma/schema/dataroom.prisma (1)
177-195: Duplicate item IDs across types not allowed
@@unique([groupId,itemId])prevents storing separate rules for a document and a folder sharing the same ID (IDs are globally unique only by convention). If you ever support hierarchical reuse, includeitemTypein the compound key.ee/features/permissions/components/permissions-sheet.tsx (2)
745-746: Good use of ref pattern to avoid stale closures.The empty dependency array combined with
dataRefis a good pattern here to prevent excessive re-renders while maintaining access to current data.
336-1028: Well-structured permissions management component.The component demonstrates excellent architecture with:
- Clear separation of tree building, permission calculation, and UI logic
- Proper handling of hierarchical permissions with partial states
- Good UX with the "Share Entire Dataroom" toggle
- Appropriate use of React patterns and performance optimizations
components/links/links-table.tsx (1)
796-828: Clean integration of permission management components.Good separation of concerns with conditional rendering of
DataroomLinkSheetfor datarooms and standardLinkSheetfor documents. ThePermissionsSheetintegration is well-handled with proper state management and callbacks.ee/features/permissions/components/dataroom-link-sheet.tsx (1)
76-900: Well-architected dataroom link management component.The component demonstrates excellent patterns:
- Clear separation of link creation/update logic from permission management
- Proper handling of async operations with loading states
- Good error handling and user feedback
- Clean integration with the PermissionsSheet component
🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
prisma/migrations/20250612000000_add_permissions_to_link/migration.sql (1)
1-55: Well-designed database migration for granular permissions.The migration demonstrates good practices:
- Nullable
permissionGroupIdensures backward compatibility- Composite unique constraint prevents duplicate access controls
- CASCADE deletes maintain referential integrity
- Appropriate indexes for query performance
lib/api/links/link-data.ts (1)
169-174: Clean handling of dual permission models.Good approach to extract access controls from either ViewerGroup or PermissionGroup, maintaining backward compatibility while supporting the new permission system.
| // granular permissions | ||
| permissionGroupId String? | ||
| permissionGroup PermissionGroup? @relation(fields: [permissionGroupId], references: [id], onDelete: SetNull) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add an index & enforce exclusivity for permissionGroupId
- Queries on
Link.permissionGroupId(present in multiple API routes) will scan the whole table.@@ model Link permissionGroupId String? permissionGroup PermissionGroup? @relation(fields: [permissionGroupId], references: [id], onDelete: SetNull)
- @@index([permissionGroupId])
- Only one of
groupIdorpermissionGroupIdshould be set. Prisma can’t express a
CHECKconstraint yet, but adding a runtime guard in link-creation/update code would prevent inconsistent rows.
📝 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.
| // granular permissions | |
| permissionGroupId String? | |
| permissionGroup PermissionGroup? @relation(fields: [permissionGroupId], references: [id], onDelete: SetNull) | |
| // granular permissions | |
| permissionGroupId String? | |
| permissionGroup PermissionGroup? @relation(fields: [permissionGroupId], references: [id], onDelete: SetNull) | |
| @@index([permissionGroupId]) |
🤖 Prompt for AI Agents
In prisma/schema/link.prisma around lines 53 to 56, add an index on the
permissionGroupId field to optimize queries filtering by this column.
Additionally, enforce exclusivity between groupId and permissionGroupId by
implementing a runtime check in the link creation and update logic to ensure
only one of these fields is set, since Prisma does not support CHECK constraints
for this yet.
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts
Outdated
Show resolved
Hide resolved
| if (permissions && typeof permissions === "object") { | ||
| // Get existing access controls to determine what needs to be updated vs created | ||
| const existingControls = | ||
| await tx.permissionGroupAccessControls.findMany({ | ||
| where: { | ||
| groupId: permissionGroupId, | ||
| }, | ||
| select: { | ||
| itemId: true, | ||
| itemType: true, | ||
| canView: true, | ||
| canDownload: true, | ||
| canDownloadOriginal: true, | ||
| }, | ||
| }); | ||
|
|
||
| const existingMap = new Map( | ||
| existingControls.map((control) => [ | ||
| `${control.itemId}-${control.itemType}`, | ||
| control, | ||
| ]), | ||
| ); | ||
|
|
||
| const toUpdate: Array<{ | ||
| itemId: string; | ||
| itemType: any; | ||
| canView: boolean; | ||
| canDownload: boolean; | ||
| canDownloadOriginal: boolean; | ||
| }> = []; | ||
|
|
||
| const toCreate: Array<{ | ||
| groupId: string; | ||
| itemId: string; | ||
| itemType: any; | ||
| canView: boolean; | ||
| canDownload: boolean; | ||
| canDownloadOriginal: boolean; | ||
| }> = []; | ||
|
|
||
| // Categorize permissions into updates vs creates |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate & sanitize incoming permissions payload
typeof permissions === "object" is too loose—malformed keys or unexpected booleans will silently pass through and write to the DB.
Add schema validation (e.g. Zod) and reject items that do not belong to the current dataroom to prevent privilege-escalation via arbitrary itemIds.
🤖 Prompt for AI Agents
In
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts
around lines 140 to 180, the current check for permissions using typeof
permissions === "object" is too broad and allows malformed or unauthorized data
to pass through. To fix this, implement schema validation using a library like
Zod to strictly validate the shape and types of the permissions payload.
Additionally, add logic to verify that each permission item belongs to the
current dataroom by checking itemIds against the dataroom context, and reject
any items that do not match to prevent privilege escalation.
| const handlePermissionsSave = async (permissions: any) => { | ||
| if (!editPermissionLink) return; | ||
|
|
||
| try { | ||
| // Update the permissions for the existing link | ||
| const res = await fetch( | ||
| `/api/teams/${teamInfo?.currentTeam?.id}/datarooms/${targetId}/permission-groups/${editPermissionLink.permissionGroupId}`, | ||
| { | ||
| method: "PUT", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| permissions: permissions, | ||
| linkId: editPermissionLink.id, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| if (!res.ok) { | ||
| const { error } = await res.json(); | ||
| throw new Error(error ?? "Failed to update permissions"); | ||
| } | ||
|
|
||
| // Refresh the links cache | ||
| const endpointTargetType = `${targetType.toLowerCase()}s`; | ||
| mutate( | ||
| `/api/teams/${teamInfo?.currentTeam?.id}/${endpointTargetType}/${encodeURIComponent( | ||
| targetId, | ||
| )}/links`, | ||
| ); | ||
|
|
||
| setShowPermissionsSheet(false); | ||
| setEditPermissionLink(null); | ||
| toast.success("File permissions updated successfully"); | ||
| } catch (error) { | ||
| console.error("Error updating file permissions:", error); | ||
| toast.error("Failed to update file permissions"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add validation for team context before API call.
The function should validate that teamInfo?.currentTeam?.id exists before making the API call to prevent potential runtime errors.
Apply this diff to add proper validation:
const handlePermissionsSave = async (permissions: any) => {
if (!editPermissionLink) return;
+ if (!teamInfo?.currentTeam?.id) {
+ toast.error("Team information not available");
+ return;
+ }
try {
// Update the permissions for the existing link
const res = await fetch(📝 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 handlePermissionsSave = async (permissions: any) => { | |
| if (!editPermissionLink) return; | |
| try { | |
| // Update the permissions for the existing link | |
| const res = await fetch( | |
| `/api/teams/${teamInfo?.currentTeam?.id}/datarooms/${targetId}/permission-groups/${editPermissionLink.permissionGroupId}`, | |
| { | |
| method: "PUT", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ | |
| permissions: permissions, | |
| linkId: editPermissionLink.id, | |
| }), | |
| }, | |
| ); | |
| if (!res.ok) { | |
| const { error } = await res.json(); | |
| throw new Error(error ?? "Failed to update permissions"); | |
| } | |
| // Refresh the links cache | |
| const endpointTargetType = `${targetType.toLowerCase()}s`; | |
| mutate( | |
| `/api/teams/${teamInfo?.currentTeam?.id}/${endpointTargetType}/${encodeURIComponent( | |
| targetId, | |
| )}/links`, | |
| ); | |
| setShowPermissionsSheet(false); | |
| setEditPermissionLink(null); | |
| toast.success("File permissions updated successfully"); | |
| } catch (error) { | |
| console.error("Error updating file permissions:", error); | |
| toast.error("Failed to update file permissions"); | |
| } | |
| }; | |
| const handlePermissionsSave = async (permissions: any) => { | |
| if (!editPermissionLink) return; | |
| if (!teamInfo?.currentTeam?.id) { | |
| toast.error("Team information not available"); | |
| return; | |
| } | |
| try { | |
| // Update the permissions for the existing link | |
| const res = await fetch( | |
| `/api/teams/${teamInfo?.currentTeam?.id}/datarooms/${targetId}/permission-groups/${editPermissionLink.permissionGroupId}`, | |
| { | |
| method: "PUT", | |
| headers: { | |
| "Content-Type": "application/json", | |
| }, | |
| body: JSON.stringify({ | |
| permissions: permissions, | |
| linkId: editPermissionLink.id, | |
| }), | |
| }, | |
| ); | |
| if (!res.ok) { | |
| const { error } = await res.json(); | |
| throw new Error(error ?? "Failed to update permissions"); | |
| } | |
| // Refresh the links cache | |
| const endpointTargetType = `${targetType.toLowerCase()}s`; | |
| mutate( | |
| `/api/teams/${teamInfo?.currentTeam?.id}/${endpointTargetType}/${encodeURIComponent( | |
| targetId, | |
| )}/links`, | |
| ); | |
| setShowPermissionsSheet(false); | |
| setEditPermissionLink(null); | |
| toast.success("File permissions updated successfully"); | |
| } catch (error) { | |
| console.error("Error updating file permissions:", error); | |
| toast.error("Failed to update file permissions"); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In components/links/links-table.tsx around lines 302 to 341, the function
handlePermissionsSave does not validate the existence of
teamInfo?.currentTeam?.id before making the API call, which can cause runtime
errors. Add a check at the start of the function to ensure
teamInfo?.currentTeam?.id is defined; if not, exit early or handle the error
appropriately to prevent the API call from proceeding without a valid team
context.
| const effectiveGroupId = groupId || permissionGroupId; | ||
|
|
||
| if (effectiveGroupId) { | ||
| // Check if this is a ViewerGroup (legacy) or PermissionGroup | ||
| // First try to find ViewerGroup permissions (for backwards compatibility) | ||
| if (groupId) { | ||
| // This is a ViewerGroup (legacy behavior) | ||
| groupPermissions = await prisma.viewerGroupAccessControls.findMany({ | ||
| where: { | ||
| groupId: groupId, | ||
| OR: [{ canView: true }, { canDownload: true }], | ||
| }, | ||
| }); | ||
| } else if (permissionGroupId) { | ||
| // This is a PermissionGroup (new behavior) | ||
| groupPermissions = await prisma.permissionGroupAccessControls.findMany({ | ||
| where: { | ||
| groupId: permissionGroupId, | ||
| OR: [{ canView: true }, { canDownload: true }], | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated permission fetching logic into a helper function.
The permission fetching logic is duplicated between fetchDataroomLinkData and fetchDataroomDocumentLinkData. Consider extracting this into a reusable helper function.
Create a helper function to reduce duplication:
async function fetchGroupPermissions(
groupId?: string,
permissionGroupId?: string,
itemId?: string,
itemType?: ItemType
): Promise<ViewerGroupAccessControls[] | PermissionGroupAccessControls[]> {
const effectiveGroupId = groupId || permissionGroupId;
if (!effectiveGroupId) return [];
const whereClause: any = {
groupId: effectiveGroupId,
OR: [{ canView: true }, { canDownload: true }],
};
if (itemId && itemType) {
whereClause.itemId = itemId;
whereClause.itemType = itemType;
}
if (groupId) {
return await prisma.viewerGroupAccessControls.findMany({ where: whereClause });
} else if (permissionGroupId) {
return await prisma.permissionGroupAccessControls.findMany({ where: whereClause });
}
return [];
}Also applies to: 194-221
🤖 Prompt for AI Agents
In lib/api/links/link-data.ts around lines 27 to 48 and also lines 194 to 221,
the permission fetching logic is duplicated in fetchDataroomLinkData and
fetchDataroomDocumentLinkData. Extract this logic into a single helper function
named fetchGroupPermissions that accepts groupId, permissionGroupId, itemId, and
itemType as parameters. This function should build a where clause with groupId
or permissionGroupId, include canView or canDownload conditions, and optionally
filter by itemId and itemType. Then it should query the appropriate prisma table
(viewerGroupAccessControls or permissionGroupAccessControls) based on which
groupId is provided, returning the results or an empty array if no effective
groupId is given. Replace the duplicated code in both functions with calls to
this new helper function.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/links/links-table.tsx (1)
304-327:⚠️ Potential issueTeam / permission-group validation still missing before PUT request
This is the same issue flagged in a previous review: the
handlePermissionsSaveAPI call proceeds even ifteamInfo?.currentTeam?.idoreditPermissionLink.permissionGroupIdareundefined, resulting in a malformed route and 404/500s.const handlePermissionsSave = async (permissions: any) => { - if (!editPermissionLink) return; + if ( + !editPermissionLink || + !teamInfo?.currentTeam?.id || + !editPermissionLink.permissionGroupId + ) { + toast.error("Missing team or permission-group context"); + return; + }
🧹 Nitpick comments (2)
components/links/links-table.tsx (2)
283-291: Remove redundant double negationStatic-analysis warning:
if (!!groupId)can be simplified toif (groupId).- if (!!groupId) { + if (groupId) {🧰 Tools
🪛 Biome (1.9.4)
[error] 283-283: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
698-709: Disable handler while the switch is loadingThe switch is visually disabled via the
disabledprop, yetonCheckedChangewill still fire if the user toggles it before the previous request finishes (depends on implementation of the customSwitch).
Safest option is to early-return in the handler whenloadingLinks.has(link.id)istrue.onCheckedChange={(checked) => { - handleArchiveLink( + if (loadingLinks.has(link.id)) return; + handleArchiveLink( link.id, link.documentId ?? link.dataroomId ?? "", checked, ); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/links/links-table.tsx(12 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/links/links-table.tsx
[error] 396-396: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
| const handleEditPermissions = (link: LinkWithViews) => { | ||
| setEditPermissionLink(link); | ||
| setShowPermissionsSheet(true); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Guard against missing permissionGroupId before opening the sheet
handleEditPermissions opens the PermissionsSheet even when the link has no associated permissionGroupId, which will immediately break the sheet’s API calls. Add a defensive check so the user gets a toast instead of a runtime error.
const handleEditPermissions = (link: LinkWithViews) => {
- setEditPermissionLink(link);
- setShowPermissionsSheet(true);
+ if (!link.permissionGroupId) {
+ toast.error("This link is not associated with a permission group.");
+ return;
+ }
+ setEditPermissionLink(link);
+ setShowPermissionsSheet(true);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleEditPermissions = (link: LinkWithViews) => { | |
| setEditPermissionLink(link); | |
| setShowPermissionsSheet(true); | |
| }; | |
| const handleEditPermissions = (link: LinkWithViews) => { | |
| if (!link.permissionGroupId) { | |
| toast.error("This link is not associated with a permission group."); | |
| return; | |
| } | |
| setEditPermissionLink(link); | |
| setShowPermissionsSheet(true); | |
| }; |
🤖 Prompt for AI Agents
In components/links/links-table.tsx around lines 299 to 303, the function
handleEditPermissions opens the PermissionsSheet without checking if the link
has a permissionGroupId, causing runtime errors. Add a conditional check to
verify that link.permissionGroupId exists before calling
setShowPermissionsSheet(true). If it does not exist, show a toast notification
to inform the user instead of opening the sheet.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts (2)
155-161:⚠️ Potential issueStill no authorization / ownership check on
itemIds
Validation now ensures shape & types, but any arbitraryitemIdthat belongs to a different dataroom happily passes and is written to the DB → privilege-escalation vector.
Add a check that everyitemIdbelongs todataroomId(or at least toteamId) before proceeding.
211-217:⚠️ Potential issue
canDownloadOriginalhard-coded tofalseagainEvery create/update forces
canDownloadOriginaltofalse, silently stripping the permission.
Either (a) include it in the payload & validation, or (b) leave it untouched when not provided.- canDownloadOriginal: false, + canDownloadOriginal: permission.original ?? existing?.canDownloadOriginal ?? false,
🧹 Nitpick comments (3)
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts (3)
190-205: Avoidany; leverage the generatedItemTypeenum
itemType: anyloses type safety and defeats the purpose of the earlier Zod validation.- itemType: any; + itemType: ItemType;Same for the
toCreatestructure.
236-254: N+1 update/delete queries – batch themLooping
updateMany/deleteManyper item spawns O(N) round-trips.
Consider:
- A single
updateManywithORinwhere, or- Use a raw SQL
INSERT … ON CONFLICTfor upserts, or- Replace with
createMany+skipDuplicatesand then a singledeleteManyfor the leftovers.Improves latency noticeably on large permission sets.
Also applies to: 275-286
312-314: Comment contradicts the code
// We only allow GET requestsis outdated – you now allow GET and PUT.
Update the comment to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/links/link-sheet/index.tsx(4 hunks)pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts(1 hunks)pages/api/teams/[teamId]/datarooms/[id]/permission-groups/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/links/link-sheet/index.tsx
- pages/api/teams/[teamId]/datarooms/[id]/permission-groups/index.ts
| const team = await prisma.team.findUnique({ | ||
| where: { | ||
| id: teamId, | ||
| users: { | ||
| some: { userId }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
findUnique will throw – relational filters are not allowed
findUnique only accepts the fields that make up a unique or composite-unique constraint.
Passing users: { some … } (or any relational filter) violates this rule and will crash at runtime.
Use findFirst (or findUnique on a dedicated composite key) instead.
-const team = await prisma.team.findUnique({
+const team = await prisma.team.findFirst({
where: {
id: teamId,
users: { some: { userId } },
},
});Same pattern appears for dataroom & permissionGroup look-ups below – fix them too.
Also applies to: 111-118
🤖 Prompt for AI Agents
In
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts
around lines 46 to 53 and also lines 111 to 118, replace all uses of prisma's
findUnique that include relational filters like users: { some: { userId } } with
findFirst, since findUnique only accepts unique fields and will throw an error
with relational filters. Update the dataroom and permissionGroup look-ups
similarly to use findFirst when filtering by relations.
| if ( | ||
| existing.canView !== permission.view || | ||
| existing.canDownload !== permission.download | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Change-detection ignores canDownloadOriginal
Even if you expose canDownloadOriginal, the diff only checks view/download.
Include the field in the comparison or you'll miss updates and accidentally overwrite on unrelated changes.
- existing.canDownload !== permission.download
+ existing.canDownload !== permission.download ||
+ existing.canDownloadOriginal !== permission.originalAlso applies to: 246-249
🤖 Prompt for AI Agents
In
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts
around lines 221-224 and 246-249, the change detection logic only compares
canView and canDownload fields but ignores canDownloadOriginal. Update the
conditional checks to also compare existing.canDownloadOriginal with
permission.canDownloadOriginal to ensure all relevant permission changes are
detected and handled correctly.
| const permissionGroup = await prisma.permissionGroup.findUnique({ | ||
| where: { | ||
| id: permissionGroupId, | ||
| dataroomId: dataroomId, | ||
| teamId: teamId, | ||
| }, | ||
| include: { |
There was a problem hiding this comment.
Compound filters with findUnique are invalid
Each of these findUnique calls mixes the primary key id with extra fields (dataroomId, teamId).
Unless you created a composite unique on all of those columns, Prisma validation will fail.
Switch to findFirst or reduce the where clause to the unique key only.
Also applies to: 124-131, 137-143
🤖 Prompt for AI Agents
In
pages/api/teams/[teamId]/datarooms/[id]/permission-groups/[permissionGroupId].ts
around lines 60 to 66, the code uses prisma.permissionGroup.findUnique with a
where clause containing multiple fields (id, dataroomId, teamId), which is
invalid unless a composite unique constraint exists on all those fields. To fix
this, replace findUnique with findFirst to allow filtering by multiple fields,
or modify the where clause to only include the unique key field if no composite
unique constraint exists. Apply the same fix to similar calls around lines
124-131 and 137-143.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
ee/features/permissions/components/dataroom-link-sheet.tsx (1)
702-706: Implement the “add group” flow instead of early-returning.When the user chooses
add_groupthe handler just returns, leaving them stuck. Either navigate to the group-creation screen or open the group sheet as originally intended.This was pointed out in a previous review; please address or justify.
components/links/links-table.tsx (2)
298-301: Guard against missingpermissionGroupIdbefore opening the sheet.Opening
PermissionsSheetwith a link that lackspermissionGroupIdwill break internal fetches. Add:if (!link.permissionGroupId) { toast.error("This link is not associated with a permission group."); return; }Same feedback was given earlier – please address.
303-344: ValidateteamInfo.currentTeam.idbefore hitting the API.
handlePermissionsSaveconstructs URLs with${teamInfo?.currentTeam?.id}but never checks that the value exists. If the team context hasn’t loaded yet you’ll firefetch /api/teams/undefined/..., returning 404 and showing a generic error to the user.Add an early guard:
if (!teamInfo?.currentTeam?.id) { toast.error("Team context not available."); return; }
🧹 Nitpick comments (2)
ee/features/permissions/components/dataroom-link-sheet.tsx (1)
242-263: Deduplicate data-URL checks & favour optional chaining.The four-way
linkData.metaImage && linkData.metaImage.startsWith("data:")pattern is verbose. Consider:const isDataUrl = (u?: string | null) => u?.startsWith("data:"); const metaImageUrl = isDataUrl(linkData.metaImage) ? await uploadImage(convertDataUrlToFile({ dataUrl: linkData.metaImage! })) : linkData.metaImage;Same for favicon. Improves readability and matches the linter’s “useOptionalChain” suggestion.
🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/links/links-table.tsx (1)
436-447: Minor: drop redundant double negation.
if (!!groupId)→if (groupId). Keeps codebase consistent and removes the linter warning.🧰 Tools
🪛 Biome (1.9.4)
[error] 436-436: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/links/link-sheet/dataroom-link-sheet.tsx(1 hunks)components/links/link-sheet/index.tsx(5 hunks)components/links/links-table.tsx(12 hunks)ee/features/permissions/components/dataroom-link-sheet.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/links/link-sheet/dataroom-link-sheet.tsx
- components/links/link-sheet/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/links/links-table.tsx
[error] 436-436: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
ee/features/permissions/components/dataroom-link-sheet.tsx
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| link: any, | ||
| permissions: ItemPermission | null, | ||
| isUpdating: boolean, | ||
| ) => { | ||
| // If permissions are null, user wants full dataroom access - no permission group needed | ||
| if (permissions === null) { | ||
| // If updating and link had a permission group, we might want to remove it | ||
| // For now, we'll leave existing permission groups as they are | ||
| return; | ||
| } | ||
|
|
||
| // If permissions is an empty object, user wants no access - still create/update permission group | ||
| // If permissions has items, create/update permission group with those permissions | ||
| if (permissions && typeof permissions === "object") { | ||
| if (isUpdating && currentLink?.permissionGroupId) { | ||
| // Update existing permission group | ||
| await fetch( | ||
| `/api/teams/${teamInfo?.currentTeam?.id}/datarooms/${targetId}/permission-groups/${currentLink.permissionGroupId}`, | ||
| { | ||
| method: "PUT", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| permissions: permissions, | ||
| }), | ||
| }, | ||
| ); | ||
| } else { | ||
| // Create new permission group | ||
| await fetch( | ||
| `/api/teams/${teamInfo?.currentTeam?.id}/datarooms/${targetId}/permission-groups`, | ||
| { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| permissions: permissions, | ||
| linkId: link.id, | ||
| }), | ||
| }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale permission-group possible when permissions are cleared.
If a link previously had a permissionGroupId and the user now sets permissions to “full access” (null), the old permission group is left intact, so the DB contains an unused record and the link still references it. Persist either a DELETE or PUT with empty permissions to keep data in sync.
🤖 Prompt for AI Agents
In ee/features/permissions/components/dataroom-link-sheet.tsx around lines 320
to 364, when permissions are set to null (full access) but the link previously
had a permissionGroupId, the old permission group is not removed or cleared,
causing stale data. To fix this, add logic to detect this case and either send a
DELETE request to remove the old permission group or a PUT request with empty
permissions to clear it, ensuring the database and link state stay synchronized.
| const createOrUpdateLinkWithPermissions = async ( | ||
| linkData: DEFAULT_LINK_TYPE, | ||
| permissions: ItemPermission | null, | ||
| shouldPreview: boolean = false, | ||
| showSuccess: boolean = false, | ||
| ) => { | ||
| // Upload the image if it's a data URL | ||
| let blobUrl: string | null = | ||
| linkData.metaImage && linkData.metaImage.startsWith("data:") | ||
| ? null | ||
| : linkData.metaImage; | ||
| if (linkData.metaImage && linkData.metaImage.startsWith("data:")) { | ||
| // Convert the data URL to a blob | ||
| const blob = convertDataUrlToFile({ dataUrl: linkData.metaImage }); | ||
| // Upload the blob to vercel storage | ||
| blobUrl = await uploadImage(blob); | ||
| } | ||
|
|
||
| // Upload meta favicon if it's a data URL | ||
| let blobUrlFavicon: string | null = | ||
| linkData.metaFavicon && linkData.metaFavicon.startsWith("data:") | ||
| ? null | ||
| : linkData.metaFavicon; | ||
| if (linkData.metaFavicon && linkData.metaFavicon.startsWith("data:")) { | ||
| const blobFavicon = convertDataUrlToFile({ | ||
| dataUrl: linkData.metaFavicon, | ||
| }); | ||
| blobUrlFavicon = await uploadImage(blobFavicon); | ||
| } | ||
|
|
||
| let endpoint = "/api/links"; | ||
| let method = "POST"; | ||
| const isUpdating = !!currentLink?.id; | ||
|
|
||
| if (isUpdating) { | ||
| endpoint = `/api/links/${currentLink.id}`; | ||
| method = "PUT"; | ||
| } | ||
|
|
||
| const response = await fetch(endpoint, { | ||
| method: method, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| ...linkData, | ||
| metaImage: blobUrl, | ||
| metaFavicon: blobUrlFavicon, | ||
| targetId: targetId, | ||
| linkType: linkType, | ||
| teamId: teamInfo?.currentTeam?.id, | ||
| }), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| // handle error with toast message | ||
| const { error } = await response.json(); | ||
| toast.error(error); | ||
| setIsSaving(false); | ||
| return; | ||
| } | ||
|
|
||
| const returnedLink = await response.json(); | ||
|
|
||
| // Handle permissions | ||
| await handlePermissionGroupOperations( | ||
| returnedLink, | ||
| permissions, | ||
| isUpdating, | ||
| ); | ||
|
|
||
| // Handle UI updates and notifications | ||
| await handlePostSaveOperations( | ||
| returnedLink, | ||
| isUpdating, | ||
| showSuccess, | ||
| shouldPreview, | ||
| permissions, | ||
| ); | ||
|
|
||
| setData(DEFAULT_LINK_PROPS(linkType, groupId)); | ||
| setIsSaving(false); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break up createOrUpdateLinkWithPermissions for maintainability.
At ~180 LoC the method mixes upload logic, API branching, permission-group handling and post-save UI work. Extract helpers (e.g. uploadMetaAssets, persistLink, syncPermissionGroup, updateCaches) to keep each concern < 50 LoC and easier to test.
🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In ee/features/permissions/components/dataroom-link-sheet.tsx around lines 235
to 317, the createOrUpdateLinkWithPermissions function is too long and mixes
multiple concerns like image uploading, API calls, permission handling, and UI
updates. Refactor by extracting separate helper functions such as
uploadMetaAssets for handling image uploads, persistLink for the API request
logic, syncPermissionGroup for permission-related operations, and updateCaches
or handlePostSaveOperations for UI updates and notifications. This will keep
each function under 50 lines and improve maintainability and testability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ee/features/permissions/components/dataroom-link-sheet.tsx (1)
320-329: Stale permission group persists when permissions reset to “Full access”
Same issue raised in a previous review – the earlyreturnleaves the oldpermissionGroupIdhanging around and the DB record orphaned.
Please implement a DELETE or a PUT with empty permissions whenpermissions === null.components/links/links-table.tsx (2)
305-336: Still no team-context null-check before API callPrevious review already highlighted the missing guard for
teamInfo?.currentTeam?.id.
If the context is undefined (e.g. immediately after a hard refresh) these two fetches will hit/api/teams/undefined/..., returning 404.
299-303: 🛠️ Refactor suggestionOpening PermissionsSheet without a group may 404
handleEditPermissionsshows the sheet even whenlink.permissionGroupIdisnull.
<PermissionsSheet>receivespermissionGroupId={undefined}which will likely trigger a GET/…/permission-groups/undefinedunless the sheet guards internally.Recommended quick fix:
const handleEditPermissions = (link: LinkWithViews) => { - setEditPermissionLink(link); - setShowPermissionsSheet(true); + if (!link.permissionGroupId) { + toast.error("Link does not have custom permissions yet."); + return; + } + setEditPermissionLink(link); + setShowPermissionsSheet(true); };
🧹 Nitpick comments (2)
ee/features/permissions/components/dataroom-link-sheet.tsx (1)
241-264: Factor out duplicated image-upload blockThe favicon/upload logic is a copy-paste of the meta-image branch. Extracting a small helper (e.g.
await maybeUploadDataUrl(dataUrl)) removes ~20 LoC, reduces cyclomatic complexity and makes the function easier to test.🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
components/links/links-table.tsx (1)
740-752: Redundant double-negation & misleading param naming
checked={!link.isArchived}and latertoast.success(!isArchived ? …)make it hard to reason about the current vs. desired state.
You can drop the!on the toast line and rename the parameter toisActivefor clarity:- toast.success( - !isArchived ? "Link successfully archived" : "Link successfully reactivated", - ); + toast.success( + isActive ? "Link successfully reactivated" : "Link successfully archived", + );(Static analysis flagged the redundant double negation.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/links/link-sheet/index.tsx(9 hunks)components/links/links-table.tsx(13 hunks)ee/features/permissions/components/dataroom-link-sheet.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/links/link-sheet/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
components/links/links-table.tsx
[error] 437-437: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
ee/features/permissions/components/dataroom-link-sheet.tsx
[error] 243-243: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 255-255: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 258-258: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
ee/features/permissions/components/dataroom-link-sheet.tsx (1)
139-142:useEffectmisses reactive deps – sheet can show stale defaults
linkType,groupId, andisDataroomsinfluence the computed default props, but onlycurrentLinkis listed in the dependency array.
Navigating to another dataroom (or toggling the product tier) while the sheet is open will leave the form in an inconsistent state.- useEffect(() => { - setData(currentLink || DEFAULT_LINK_PROPS(linkType, groupId, !isDatarooms)); - }, [currentLink]); + useEffect(() => { + setData( + currentLink || DEFAULT_LINK_PROPS(linkType, groupId, !isDatarooms), + ); + }, [currentLink, linkType, groupId, isDatarooms]);
Summary by CodeRabbit
New Features
Improvements
Bug Fixes