fix: various fixes #1934
fix: various fixes #1934
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR introduces plan-based gating for Data Rooms Plus features, adds invitation status indicators to dataroom group members, updates workflow slug validation to disallow underscores, and refactors dataroom invitation modal identifier logic. Component imports are reorganized and API endpoints are updated to fetch invitation data. Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/datarooms/groups/group-member-table.tsx (1)
307-307: Add accessibility label for invitation status icon.The
MailCheckIconlacks an accessible label, making it unclear to screen reader users what the icon represents.Apply this diff to improve accessibility:
- <MailCheckIcon className="h-4 w-4 text-blue-500 hover:text-blue-600" /> + <MailCheckIcon className="h-4 w-4 text-blue-500 hover:text-blue-600" aria-label="Invitation sent" />components/datarooms/actions/rebuild-index-button.tsx (1)
75-75: Consider SWR mutation instead of full page reload.While the current
window.location.reload()ensures all data is refreshed, consider using SWR'smutatefunction for a more refined update. This would provide a smoother user experience by avoiding a full page reload.Example refactor:
// Import mutate from SWR hooks import { useDataroom, useDataroomItems } from "@/lib/swr/use-dataroom"; // Inside component const { mutate: mutateDataroom } = useDataroom(); const { mutate: mutateItems } = useDataroomItems({ root: true }); // In handleRebuildIndex, replace window.location.reload() with: await Promise.all([mutateDataroom(), mutateItems()]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
components/datarooms/actions/rebuild-index-button.tsx(3 hunks)components/datarooms/groups/group-member-table.tsx(5 hunks)components/links/links-table.tsx(2 hunks)ee/features/dataroom-invitations/components/invite-viewers-modal.tsx(1 hunks)ee/features/workflows/README.md(0 hunks)ee/features/workflows/index.ts(0 hunks)ee/features/workflows/lib/types.ts(1 hunks)lib/swr/use-dataroom-groups.ts(2 hunks)pages/api/teams/[teamId]/datarooms/[id]/groups/[groupId]/index.ts(1 hunks)pages/datarooms/[id]/documents/[...name].tsx(1 hunks)pages/datarooms/[id]/documents/index.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- ee/features/workflows/README.md
- ee/features/workflows/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
components/datarooms/actions/rebuild-index-button.tsx (1)
lib/swr/use-billing.ts (1)
usePlan(86-136)
components/datarooms/groups/group-member-table.tsx (2)
components/visitors/visitor-avatar.tsx (1)
VisitorAvatar(10-95)components/ui/timestamp-tooltip.tsx (1)
TimestampTooltip(45-78)
🔇 Additional comments (14)
components/links/links-table.tsx (1)
6-7: LGTM! Import reorganization looks clean.The import reorganization properly groups EE feature imports together and places the hook import in the appropriate section. All three imports (
InviteViewersModal,invitationEmailSchema, anduseFeatureFlags) are actively used in the component, confirming this is a valid cleanup of duplicate imports.Also applies to: 27-27
ee/features/dataroom-invitations/components/invite-viewers-modal.tsx (1)
91-91: Refactoring is correct and aligns with codebase patterns.The change to extract
last5 = link.id.slice(-5)is a good refactoring that follows the established pattern throughout the codebase. Theid.slice(-5)approach for generating fallback link display names (e.g., "Link #abc12") is used consistently in 15+ locations across the codebase, including pages/api/analytics/index.ts, components/view/view-data.tsx, components/links/links-table.tsx, and others. The slug property is reserved for URL construction (domain/slug patterns), which serves a different purpose. This separation of concerns is intentional and correct.pages/api/teams/[teamId]/datarooms/[id]/groups/[groupId]/index.ts (1)
60-72: LGTM! Efficient query for latest invitation.The nested include correctly fetches only the most recent invitation for the current group using proper filtering, ordering, and limit. This is an efficient approach to augment group member data with invitation status.
lib/swr/use-dataroom-groups.ts (1)
10-10: LGTM! Type augmentation aligns with API changes.The addition of the
ViewerInvitationimport and the extension of the viewer type to include an optionalinvitationsarray correctly reflects the API changes and maintains type safety.Also applies to: 84-84
components/datarooms/groups/group-member-table.tsx (2)
301-309: Verify feature flag gating for invitation status indicator.The invitation status indicator (lines 301-309) is displayed without checking the
dataroomInvitationsfeature flag, while the "Share invite" button (line 189) is gated behind this flag. This creates an inconsistency where users might see invitation indicators even when the feature is disabled.Please confirm whether the invitation status indicator should also be gated by the feature flag. If it should show historical data regardless of feature state, the current implementation is correct. Otherwise, wrap the indicator in a feature flag check:
<p className="flex items-center gap-x-2 overflow-visible text-sm font-medium text-gray-800 dark:text-gray-200"> {viewer.viewer.email} - {latestInvitation && ( + {isFeatureEnabled("dataroomInvitations") && latestInvitation && ( <TimestampTooltip timestamp={latestInvitation.sentAt} side="right" rows={["local", "utc"]} > <MailCheckIcon className="h-4 w-4 text-blue-500 hover:text-blue-600" /> </TimestampTooltip> )} </p>
287-300: LGTM! Member rendering logic is sound.The extraction of
latestInvitationfrom the first element of the invitations array is correct, as the API returns invitations ordered bysentAtdescending. The optional chaining ensures safe access.Also applies to: 310-314
ee/features/workflows/lib/types.ts (2)
76-76: No issues found with the .nullish() addition on slug.The change is well-designed and properly handled throughout the codebase:
- Client-side: Workflow creation explicitly enforces slug requirements:
if (domain !== "papermark.com" && !slug.trim())rejects submissions without a slug for custom domains, then sends slug asundefinedfor papermark.com domains.- Server-side: All URL generation code checks both
domainSlug && slugbefore building custom domain URLs, falling back to ID-based URLs when either is missing (workflow-detail.tsx, workflow-list.tsx, engine.ts, workflow-access-view.tsx).- Validation alignment: The schema correctly allows
null/undefinedonly when using papermark.com, matching the business logic across the application.
70-73: Clarify the breaking change and provide correct verification approach.The regex change is a breaking change in the Zod validation layer (not the database schema), which means existing workflows with underscores in their slugs would fail validation on update. However, the provided script won't verify this—it searches the codebase, not the database.
To properly verify this change:
Query the database directly for existing workflows/links with underscores in slugs:
SELECT id, slug FROM "Link" WHERE slug LIKE '%_%' AND "linkType" = 'WORKFLOW_LINK';If underscores exist, decide whether to:
- Accept the breaking change and document it
- Migrate existing slugs (remove underscores)
- Add a data migration to handle the transition
The
.nullish()addition is appropriate since slugs are optional for standard links.pages/datarooms/[id]/documents/index.tsx (1)
10-10: LGTM! Import path updated correctly.The import path correctly reflects the relocation of
RebuildIndexButtonto theactionssubfolder.pages/datarooms/[id]/documents/[...name].tsx (1)
12-12: LGTM! Import path updated correctly.Consistent with the component relocation to the
actionssubfolder, matching the change in the sibling index file.components/datarooms/actions/rebuild-index-button.tsx (4)
3-9: LGTM! Imports updated for plan-based gating.The new imports correctly support the transition from feature-flag gating to plan-based gating with upgrade flow.
43-46: LGTM! Good defensive check.This guard prevents unauthorized execution if
handleRebuildIndexis called directly or state changes unexpectedly. With the currentDialogFooterimplementation, users without DataRooms Plus won't see the "Rebuild Index" button, so this check serves as a defensive safeguard.
130-149: LGTM! Effective upgrade flow implementation.The conditional rendering creates a good user experience: users without DataRooms Plus can open the dialog to learn about the feature, then are presented with an upgrade prompt instead of the action buttons. The
UpgradePlanModalprops correctly target the DataRooms Plus plan and highlight the indexing feature.
33-40: Trial user access behavior needs clarification.The visibility logic allows any trial user to see the button (via the
isTrialcheck), but only DataRoomsPlus users can actually use it (line 43 checksif (!isDataroomsPlus)). This means users on non-DataRooms trials (e.g.,free+drtrial,pro+drtrial) will see the button but encounter an error when clicking it.Is this intentional—to give trial users a preview of the feature as an incentive—or should the button only be visible to users on DataRooms-related trials (
datarooms+drtrial,datarooms-plus+drtrial)?
| import { VisitorAvatar } from "@/components/visitors/visitor-avatar"; | ||
|
|
||
| import { InviteViewersModal } from "../invite-viewers-modal"; | ||
| import { InviteViewersModal } from "../../../ee/features/dataroom-invitations/components/invite-viewers-modal"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use absolute import path for consistency and maintainability.
The relative import path with multiple parent directory traversals is fragile and inconsistent with the rest of the file, which uses absolute imports with the @/ prefix.
Apply this diff:
-import { InviteViewersModal } from "../../../ee/features/dataroom-invitations/components/invite-viewers-modal";
+import { InviteViewersModal } from "@/ee/features/dataroom-invitations/components/invite-viewers-modal";📝 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 { InviteViewersModal } from "../../../ee/features/dataroom-invitations/components/invite-viewers-modal"; | |
| import { InviteViewersModal } from "@/ee/features/dataroom-invitations/components/invite-viewers-modal"; |
🤖 Prompt for AI Agents
In components/datarooms/groups/group-member-table.tsx around line 43, replace
the fragile relative import "import { InviteViewersModal } from
'../../../ee/features/dataroom-invitations/components/invite-viewers-modal';"
with the absolute import used elsewhere (e.g. "import { InviteViewersModal }
from '@/ee/features/dataroom-invitations/components/invite-viewers-modal';") to
ensure consistency and maintainability across the codebase.
Summary by CodeRabbit
New Features
Changes