Conversation
keithwillcode
added
core
WalkthroughAdds an organization watchlist/blocklist feature and renames the Settings tab/labels from "privacy" to "privacy_and_security". Introduces TRPC schemas and handlers (list/create/get/delete), a Prisma-backed Watchlist repository with audit logging and migration linking Watchlist ↔ WatchlistAudit, server-side permission checks (PBAC Resource.Watchlist and CRUD permissions), new UI components (BlocklistTable, CreateBlocklistEntryModal, BlocklistEntryDetailsSheet) integrated into the org privacy page, locale keys, and a domain-with-@ regex. Also updates settings layout tab keys and defers translation initialization in the org privacy page. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (21)
packages/features/data-table/hooks/useDebouncedWidth.ts (1)
1-3: Directive correctly added.The
"use client"directive is necessary and correctly placed for this hook, which uses client-only React features (useState,useEffect,ResizeObserver).Optionally, the extra blank line (line 3) could be removed to follow the more common convention of a single blank line between the directive and imports:
"use client"; - import debounce from "lodash/debounce";apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
173-180: Remove legacy “privacy” key
The"privacy"entry inorganizationAdminKeysno longer corresponds to any settings route—remove it to avoid confusion.packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.ts (1)
8-8: Consider adding max length validation for searchTerm.While the searchTerm is correctly marked as optional, it lacks a maximum length constraint. Adding a reasonable max length (e.g.,
.max(255)) can help prevent potential performance issues from extremely long search strings.Apply this diff to add a max length:
- searchTerm: z.string().optional(), + searchTerm: z.string().max(255).optional(),packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.test.ts (1)
153-193: Consider adding test coverage for error handling.The test suite comprehensively covers access control, validation, and success scenarios. However, there's no test for when
deleteEntrythrows an error, which should result in anINTERNAL_SERVER_ERRORresponse according to the handler's try-catch block.Consider adding this test case to the "successful deletion" describe block:
it("should throw INTERNAL_SERVER_ERROR when deleteEntry fails", async () => { mockWatchlistRepo.findEntryWithAudit.mockResolvedValue({ entry: mockEntry, auditHistory: [], }); mockWatchlistRepo.deleteEntry.mockRejectedValue(new Error("Database error")); await expect( deleteWatchlistEntryHandler({ ctx: { user: mockUser }, input: { id: "entry-123", }, }) ).rejects.toMatchObject({ code: "INTERNAL_SERVER_ERROR", message: "Failed to delete blocklist entry", }); });packages/features/ee/organizations/pages/settings/privacy.tsx (1)
41-51: Pass a stable tableIdentifier to DataTableProviderAvoid state collisions across navigations and ensure stable saved filters by providing an explicit tableIdentifier.
- <DataTableProvider useSegments={useSegments} defaultPageSize={25}> + <DataTableProvider + tableIdentifier="org-watchlist" + useSegments={useSegments} + defaultPageSize={25} + >packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.test.ts (1)
33-52: Align mock audit shape with repository contract (optional)Mocks use timestamp/previousValue/newValue; repository returns changedAt/value. Not breaking here, but aligning reduces confusion and future drift.
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx (3)
109-111: Localize date formattingFormat pattern isn’t localized. Prefer passing the user’s locale (or a shared date util) to date-fns format.
If useLocale exposes a dateFns locale, do:
const { t, dateLocale } = useLocale(); // ... format(new Date(audit.changedAt), "PPP, p", { locale: dateLocale })Otherwise, use the project’s date formatting utility, if available. Based on learnings
30-41: Type the action discriminator for safer variantsNarrow action to the actual union type to catch mismatches and keep Badge variants consistent.
- const getActionVariant = (action: string) => { + const getActionVariant = (action: BlocklistEntry["action"]) => { switch (action) { case "BLOCK": return "red"; case "REPORT": return "orange"; case "ALERT": return "blue"; default: return "gray"; } };
86-89: Localize fallback placeholdersReplace hardcoded "—" with a localized placeholder (e.g., t("not_available")) for consistency with i18n guidelines.
Confirm the appropriate key (e.g., "not_available"). If none exists, I can add it across locales.
Also applies to: 106-108, 120-127
packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.ts (1)
89-89: Prefer named export only (avoid default exports)Project guideline: favor named exports for better tree-shaking and refactors. Drop the default export.
-export default listWatchlistEntriesHandler; +// Prefer named export onlyAs per coding guidelines
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.ts (1)
85-85: Prefer named export only (avoid default exports)Follow project convention to drop default exports in TS files.
-export default getWatchlistEntryDetailsHandler; +// Prefer named export onlyAs per coding guidelines
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.ts (3)
47-59: Trim and normalize value before validation and persistenceAvoid false negatives and duplicate variants by trimming first and validating/saving the trimmed value (lowercased on save).
Apply this diff:
const watchlistRepo = new WatchlistRepository(prisma); - if (input.type === "EMAIL" && !emailRegex.test(input.value)) { + const trimmedValue = input.value.trim(); + + if (input.type === "EMAIL" && !emailRegex.test(trimmedValue)) { throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid email address format", }); } - if (input.type === "DOMAIN" && !domainWithAtRegex.test(input.value)) { + if (input.type === "DOMAIN" && !domainWithAtRegex.test(trimmedValue)) { throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid domain format. Domain must start with @ (e.g., @example.com)", }); } try { const entry = await watchlistRepo.createEntry({ type: input.type, - value: input.value.toLowerCase(), + value: trimmedValue.toLowerCase(), organizationId, action: WatchlistAction.BLOCK, description: input.description, userId: user.id, });Also applies to: 61-69
76-83: Use typed/structured duplicate error instead of string matchingString contains("already exists") is brittle. Prefer a custom DuplicateWatchlistEntryError (or Prisma P2002 check) and handle by instanceof (or code).
I can draft a small custom error class and update repository + handler catch if you want.
86-86: Drop default export; keep named export onlyProject guideline discourages default exports in TS. Remove the default export and use the named export everywhere.
Apply this diff:
-export default createWatchlistEntryHandler;As per coding guidelines
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.test.ts (1)
192-295: Add a test for duplicate mapping to BAD_REQUESTCover the branch that maps repository “already exists” errors to a BAD_REQUEST TRPC error.
Apply this addition near the end of the suite:
+ it("should map duplicate repository error to BAD_REQUEST", async () => { + mockPermissionCheckService.checkPermission.mockResolvedValue(true); + mockWatchlistRepo.createEntry.mockRejectedValue( + new Error("Watchlist entry already exists for this organization") + ); + + await expect( + createWatchlistEntryHandler({ + ctx: { user: mockUser }, + input: { type: WatchlistType.EMAIL, value: "dup@example.com" }, + }) + ).rejects.toMatchObject({ code: "BAD_REQUEST" }); + });apps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsx (2)
66-80: Trim input for validation and submissionImprove DX and consistency by trimming before validate and submit; optionally trim description.
Apply:
- const onSubmit = (data: FormData) => { + const onSubmit = (data: FormData) => { createWatchlistEntry.mutate({ type: data.type, - value: data.value, - description: data.description, + value: data.value.trim(), + description: data.description?.trim() || undefined, }); }; - const validateValue = (value: string) => { - if (!value) return t("required"); + const validateValue = (raw: string) => { + const value = raw.trim(); + if (!value) return t("field_required"); if (watchType === WatchlistType.EMAIL) { if (!emailRegex.test(value)) { return t("invalid_email_address"); } } else if (watchType === WatchlistType.DOMAIN) { if (!domainWithAtRegex.test(value)) { return t("invalid_domain_format"); } } return true; };Also applies to: 58-64
101-114: Wire labels to inputs; localize placeholders
- Connect labels for a11y (use id/inputId).
- Localize placeholders per .tsx i18n guideline.
Apply:
render={({ field }) => ( - <Select + <Select {...field} options={typeOptions} + inputId="type" onChange={(option) => { if (option) { field.onChange(option.value); setValue("value", ""); } }} value={typeOptions.find((opt) => opt.value === field.value)} />render={({ field }) => ( - <Input + <Input {...field} - placeholder={watchType === WatchlistType.EMAIL ? "user@example.com" : "@spammer.com"} + id="value" + placeholder={ + watchType === WatchlistType.EMAIL + ? t("email_placeholder_example") + : t("domain_placeholder_example") + } /> )}render={({ field }) => ( - <TextArea {...field} placeholder={t("reason_for_adding_to_blocklist")} rows={3} /> + <TextArea + {...field} + id="description" + placeholder={t("reason_for_adding_to_blocklist")} + rows={3} + /> )}Please ensure the i18n keys email_placeholder_example and domain_placeholder_example exist in locales. As per coding guidelines
Also applies to: 130-136, 144-150
apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (2)
46-49: Gate query by read permissionAvoid unnecessary fetch when user can’t read.
Apply:
const { data, isPending } = trpc.viewer.organizations.listWatchlistEntries.useQuery( { limit, offset, searchTerm, }, { placeholderData: keepPreviousData, + enabled: permissions?.canRead ?? true, } );
135-143: Improve delete UX: disable while pending; clear selection on close
- Disable delete button during mutation.
- Clear entryToDelete when dialog closes via overlay/Escape.
Apply:
{permissions?.canDelete && ( <Button color="destructive" variant="icon" StartIcon="trash" onClick={() => handleDelete(entry)} + disabled={deleteWatchlistEntry.isPending} tooltip={t("delete")} /> )}- <Dialog open={showDeleteDialog} onOpenChange={setShowDeleteDialog}> + <Dialog + open={showDeleteDialog} + onOpenChange={(open) => { + setShowDeleteDialog(open); + if (!open) setEntryToDelete(null); + }}> <ConfirmationDialogContent variety="danger" title={t("delete_blocklist_entry")} confirmBtnText={t("delete")} onConfirm={confirmDelete}> {t("delete_blocklist_entry_confirmation", { value: entryToDelete?.value })} </ConfirmationDialogContent> </Dialog>Also applies to: 193-201
packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.ts (2)
46-60: Avoid heavy fetch for delete pre-check; select only needed fieldsUsing findEntryWithAudit pulls audit history unnecessarily. Add a lightweight repository method (e.g., findEntryById with select: { id, organizationId, isGlobal }) and use that here.
Example:
// repo async findEntryById(id: string) { return this.prismaClient.watchlist.findUnique({ where: { id }, select: { id: true, organizationId: true, isGlobal: true }, }); } // handler const entry = await watchlistRepo.findEntryById(input.id);As per coding guidelines
77-77: Remove default export; use named export onlyAlign with project’s no-default-exports rule for TS.
Apply:
-export default deleteWatchlistEntryHandler;As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/(org-admin-only)/privacy/page.tsx(2 hunks)apps/web/modules/settings/organizations/privacy/blocklist-table.tsx(1 hunks)apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx(1 hunks)apps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsx(1 hunks)apps/web/public/static/locales/en/common.json(3 hunks)packages/features/data-table/hooks/useDebouncedWidth.ts(1 hunks)packages/features/ee/organizations/pages/settings/privacy.tsx(2 hunks)packages/features/pbac/domain/types/permission-registry.ts(4 hunks)packages/features/users/repositories/UserRepository.ts(1 hunks)packages/lib/emailSchema.ts(1 hunks)packages/lib/server/repository/watchlist.interface.ts(1 hunks)packages/lib/server/repository/watchlist.repository.ts(1 hunks)packages/prisma/migrations/20251014143620_add_watchlist_audit_relation/migration.sql(1 hunks)packages/prisma/schema.prisma(2 hunks)packages/trpc/server/routers/viewer/organizations/_router.tsx(2 hunks)packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.test.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.test.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.test.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.test.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.tspackages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.test.tspackages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.tspackages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.tspackages/features/data-table/hooks/useDebouncedWidth.tspackages/lib/emailSchema.tspackages/lib/server/repository/watchlist.interface.tspackages/lib/server/repository/watchlist.repository.tspackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.tsapps/web/modules/settings/organizations/privacy/blocklist-table.tsxpackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.test.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.tspackages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.test.tspackages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.tspackages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.tsapps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/(org-admin-only)/privacy/page.tsxpackages/features/data-table/hooks/useDebouncedWidth.tspackages/lib/emailSchema.tspackages/features/ee/organizations/pages/settings/privacy.tsxpackages/lib/server/repository/watchlist.interface.tspackages/lib/server/repository/watchlist.repository.tspackages/trpc/server/routers/viewer/organizations/_router.tsxapps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsxpackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.test.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.tsapps/web/modules/settings/organizations/privacy/blocklist-table.tsxpackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.tspackages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.test.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.tspackages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.test.tspackages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.tspackages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.tsapps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/(org-admin-only)/privacy/page.tsxpackages/features/data-table/hooks/useDebouncedWidth.tspackages/lib/emailSchema.tspackages/features/ee/organizations/pages/settings/privacy.tsxpackages/lib/server/repository/watchlist.interface.tspackages/lib/server/repository/watchlist.repository.tspackages/trpc/server/routers/viewer/organizations/_router.tsxapps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsxpackages/features/pbac/domain/types/permission-registry.tspackages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.test.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
**/*Repository.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/features/users/repositories/UserRepository.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/settings/organizations/privacy/blocklist-table.tsxapps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/(org-admin-only)/privacy/page.tsxpackages/features/ee/organizations/pages/settings/privacy.tsxpackages/trpc/server/routers/viewer/organizations/_router.tsxapps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
packages/lib/server/repository/watchlist.repository.ts
🧠 Learnings (1)
📚 Learning: 2025-10-03T18:51:52.952Z
Learnt from: alishaz-polymath
PR: calcom/cal.com#24246
File: packages/prisma/migrations/20251003103832_upsert_watchlist_audit/migration.sql:55-88
Timestamp: 2025-10-03T18:51:52.952Z
Learning: In the cal.com codebase, the team avoids Prisma-level relations for watchlist audit tables (WatchlistAudit, WatchlistEventAudit) to prevent nested selects and maintain explicit query control, even if it means handling referential integrity at the application layer or through database-level constraints only.
Applied to files:
packages/prisma/migrations/20251014143620_add_watchlist_audit_relation/migration.sqlpackages/prisma/schema.prisma
🧬 Code graph analysis (15)
packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.ts (4)
packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.ts (1)
TListWatchlistEntriesInputSchema(16-16)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-338)hasPermission(215-233)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)packages/features/users/repositories/UserRepository.ts (1)
UserRepository(122-1114)
apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (5)
packages/trpc/react/trpc.ts (2)
RouterOutputs(143-143)trpc(54-138)packages/ui/components/toast/showToast.tsx (1)
showToast(77-101)apps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsx (1)
CreateBlocklistEntryModal(25-173)apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx (1)
BlocklistEntryDetailsSheet(20-131)packages/ui/components/dialog/Dialog.tsx (1)
Dialog(35-38)
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.ts (1)
packages/features/watchlist/lib/types.ts (1)
WatchlistType(3-3)
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.ts (4)
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.ts (1)
TCreateWatchlistEntryInputSchema(11-11)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-338)hasPermission(215-233)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)packages/lib/emailSchema.ts (2)
emailRegex(4-5)domainWithAtRegex(12-13)
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.test.ts (3)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-338)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.handler.ts (1)
createWatchlistEntryHandler(19-84)
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.test.ts (4)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-338)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)packages/features/users/repositories/UserRepository.ts (1)
UserRepository(122-1114)packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.ts (1)
getWatchlistEntryDetailsHandler(19-83)
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.ts (4)
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.ts (1)
TGetWatchlistEntryDetailsInputSchema(7-7)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-338)hasPermission(215-233)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)packages/features/users/repositories/UserRepository.ts (1)
UserRepository(122-1114)
packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.ts (3)
packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.ts (1)
TDeleteWatchlistEntryInputSchema(7-7)packages/features/pbac/services/permission-check.service.ts (2)
PermissionCheckService(19-338)hasPermission(215-233)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx (1)
packages/trpc/react/trpc.ts (2)
RouterOutputs(143-143)trpc(54-138)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/(org-admin-only)/privacy/page.tsx (3)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/actions/validateUserHasOrg.tsx (1)
validateUserHasOrg(19-30)apps/web/app/_utils.tsx (1)
getTranslate(15-19)packages/features/pbac/lib/resource-permissions.ts (1)
getResourcePermissions(50-87)
packages/features/ee/organizations/pages/settings/privacy.tsx (3)
packages/features/data-table/DataTableProvider.tsx (1)
DataTableProvider(93-426)packages/features/data-table/hooks/useSegments.ts (1)
useSegments(9-66)apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (1)
BlocklistTable(30-204)
packages/lib/server/repository/watchlist.repository.ts (2)
packages/lib/server/repository/watchlist.interface.ts (6)
IWatchlistRepository(53-67)CreateWatchlistInput(28-35)WatchlistEntry(3-14)CheckWatchlistInput(37-41)FindAllEntriesInput(43-51)WatchlistAuditEntry(16-26)packages/features/watchlist/lib/repository/PrismaWatchlistAuditRepository.ts (1)
PrismaWatchlistAuditRepository(11-93)
packages/trpc/server/routers/viewer/organizations/_router.tsx (4)
packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.schema.ts (1)
ZListWatchlistEntriesInputSchema(5-14)packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.ts (1)
ZCreateWatchlistEntryInputSchema(5-9)packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.ts (1)
ZDeleteWatchlistEntryInputSchema(3-5)packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.ts (1)
ZGetWatchlistEntryDetailsInputSchema(3-5)
apps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsx (5)
packages/ui/components/toast/showToast.tsx (1)
showToast(77-101)packages/lib/emailSchema.ts (2)
emailRegex(4-5)domainWithAtRegex(12-13)packages/ui/components/dialog/Dialog.tsx (4)
Dialog(35-38)DialogContent(56-117)DialogHeader(124-142)DialogFooter(150-164)packages/ui/components/form/select/Select.tsx (1)
Select(14-125)packages/ui/components/form/inputs/TextField.tsx (1)
Input(55-66)
packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.test.ts (3)
packages/features/pbac/services/permission-check.service.ts (1)
PermissionCheckService(19-338)packages/lib/server/repository/watchlist.repository.ts (1)
WatchlistRepository(13-180)packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.handler.ts (1)
deleteWatchlistEntryHandler(18-75)
🔇 Additional comments (14)
packages/lib/emailSchema.ts (1)
7-13: LGTM! Well-documented domain validation regex.The regex pattern correctly enforces the @ prefix and supports international domain characters, which aligns with the watchlist domain validation requirements.
packages/trpc/server/routers/viewer/organizations/createWatchlistEntry.schema.ts (1)
1-11: LGTM! Clean and appropriate schema definition.The schema correctly uses
z.nativeEnumfor the Prisma enum, includes reasonable string length constraints, and follows Zod best practices.packages/features/pbac/domain/types/permission-registry.ts (2)
15-15: LGTM! Watchlist resource added to enum.The new
Watchlistresource follows the established naming convention and fits well into the existing resource structure.
677-712: LGTM! Complete and consistent permission registry for Watchlist.The permission definitions follow the established pattern for other resources:
- Proper Organization scope
- Correct dependency chain (CRUD operations depend on Read)
- Consistent i18n key naming
- Appropriate metadata structure
packages/prisma/migrations/20251014143620_add_watchlist_audit_relation/migration.sql (1)
1-2: Verify: Prisma relation contradicts previous team decision.The migration adds a database-level foreign key, which is fine. However, the corresponding schema.prisma changes (lines 2324 and 2341-2342 in schema.prisma) introduce Prisma-level relations between
WatchlistandWatchlistAudit. This appears to contradict a previous team decision to avoid Prisma-level relations for audit tables to prevent nested selects and maintain explicit query control.Based on learnings
Please confirm whether this design change is intentional and the previous constraint no longer applies, or if the Prisma relations should be removed while keeping only the database-level foreign key constraint.
packages/features/users/repositories/UserRepository.ts (1)
1045-1056: LGTM! Clean user lookup method for enriching audit data.The implementation correctly fetches multiple users by IDs with minimal field selection, which is appropriate for enriching watchlist audit records with user details.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/SettingsLayoutAppDirClient.tsx (1)
101-101: LGTM! Tab label updated to reflect expanded scope.The tab name change from "privacy" to "privacy_and_security" aligns with the broader watchlist/blocklist feature set being introduced.
packages/trpc/server/routers/viewer/organizations/deleteWatchlistEntry.schema.ts (1)
1-7: LGTM! Appropriate schema for delete operation.The schema correctly validates the UUID format for the watchlist entry ID and follows the established pattern for delete operation schemas.
packages/prisma/schema.prisma (1)
2324-2324: Verify: Prisma relation contradicts previous team decision.The Prisma-level relation between
WatchlistandWatchlistAuditcontradicts the retrieved learning that indicates the team avoids Prisma-level relations for audit tables to prevent nested selects and maintain explicit query control.Based on learnings
This is the same concern raised for the migration file. Please confirm whether:
- This design change is intentional and the previous constraint no longer applies
- The relations should be removed, keeping only the database-level foreign key constraint
If the relations are intended, ensure that repository and handler code explicitly uses
select(per coding guidelines) to avoid accidentally including nested audit records.Also applies to: 2341-2342
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/(org-admin-only)/privacy/page.tsx (1)
44-60: LGTM! Watchlist permissions setup is consistent.The watchlist permissions fetch follows the same pattern as the Organization permissions and uses appropriate fallback roles (ADMIN and OWNER) that align with the TRPC endpoint authorization. The permission set (read, create, delete) is well-suited for watchlist entries.
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.schema.ts (1)
3-5: LGTM! Input schema is well-defined.The schema correctly validates the entry ID as a UUID, which is appropriate for database identifiers.
packages/trpc/server/routers/viewer/organizations/_router.tsx (1)
170-193: LGTM! Watchlist endpoints are properly configured.All four watchlist endpoints correctly use
authedOrgAdminProcedurefor access control, appropriate operation types (query for reads, mutation for writes), and follow the established pattern of dynamic handler imports. The implementation is consistent with existing endpoints in this router.packages/trpc/server/routers/viewer/organizations/listWatchlistEntries.handler.ts (1)
19-43: Auth and org checks look solidOrg resolution + permission check path matches PBAC service usage; error codes/messages consistent with details handler.
packages/trpc/server/routers/viewer/organizations/getWatchlistEntryDetails.handler.ts (1)
19-43: Handler flow LGTMOrg check, permission check, not-found, cross-org check, and enrichment are correct.
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx
Outdated
Show resolved
Hide resolved
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx (1)
86-89: "Created by" still uses latest audit entry instead of earliest.This issue was flagged in a previous review but remains unfixed. The code uses
auditHistory[0]which is the latest change, not the original creator. Since audit history is returned in descending order, you need the last element.Apply this diff to fix the issue:
<div> <label className="text-default mb-1 block text-sm font-medium">{t("created_by")}</label> - <p className="text-default">{data.auditHistory[0]?.changedByUser?.email || "—"}</p> + <p className="text-default"> + {data.auditHistory[data.auditHistory.length - 1]?.changedByUser?.email || "—"} + </p> </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx(1 hunks)packages/lib/server/repository/watchlist.repository.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{service,repository}.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Avoid dot-suffixes like
.service.tsor.repository.tsfor new files; reserve.test.ts,.spec.ts,.types.tsfor their specific purposes
Files:
packages/lib/server/repository/watchlist.repository.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/server/repository/watchlist.repository.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/server/repository/watchlist.repository.tsapps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/server/repository/watchlist.repository.tsapps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx
🧠 Learnings (2)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`
Applied to files:
packages/lib/server/repository/watchlist.repository.ts
📚 Learning: 2025-08-07T18:42:34.081Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/lib/server/repository/PrismaPhoneNumberRepository.ts:412-417
Timestamp: 2025-08-07T18:42:34.081Z
Learning: In Cal.com codebase, the coding guideline requiring explicit `select` clauses instead of `include` for Prisma queries applies to read operations but not to update operations. Update operations don't need explicit select clauses.
Applied to files:
packages/lib/server/repository/watchlist.repository.ts
🧬 Code graph analysis (2)
packages/lib/server/repository/watchlist.repository.ts (2)
packages/lib/server/repository/watchlist.interface.ts (6)
IWatchlistRepository(53-67)CreateWatchlistInput(28-35)WatchlistEntry(3-14)CheckWatchlistInput(37-41)FindAllEntriesInput(43-51)WatchlistAuditEntry(16-26)packages/features/watchlist/lib/repository/PrismaWatchlistAuditRepository.ts (1)
PrismaWatchlistAuditRepository(11-93)
apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx (1)
packages/trpc/react/trpc.ts (2)
RouterOutputs(143-143)trpc(54-138)
🔇 Additional comments (4)
packages/lib/server/repository/watchlist.repository.ts (4)
27-52: Transaction pattern correctly ensures atomicity.The use of
$transactionto wrap both the watchlist creation and audit logging ensures that both operations succeed or fail together, preventing inconsistent state. This addresses the atomicity concern from previous reviews.
88-116: Excellent use of explicitselectclauses.The query properly uses explicit
selectfor both the watchlist fields and the nested audits relation, adhering to the coding guidelines. TheorderBy: { changedAt: "desc" }correctly retrieves the most recent audit entry.
130-158: Proper use of explicitselectthroughout.The method correctly uses explicit
selectclauses for both the entry and its audit history, avoiding the prohibitedincludepattern. The explicit field selection keeps payloads lean and follows repository conventions.
187-202: Transaction pattern correctly ensures atomicity.The use of
$transactionto wrap both the audit creation and deletion ensures that both operations succeed or fail together, preventing orphaned audit records or incomplete deletions. This addresses the atomicity concern from previous reviews.
| export const domainWithAtRegex = | ||
| /^@[a-zA-Z0-9\u00a1-\uffff]([a-zA-Z0-9\u00a1-\uffff-]*[a-zA-Z0-9\u00a1-\uffff])?(\.[a-zA-Z0-9\u00a1-\uffff]([a-zA-Z0-9\u00a1-\uffff-]*[a-zA-Z0-9\u00a1-\uffff])?)*$/; | ||
|
|
There was a problem hiding this comment.
Probably worth looking into packages/features/watchlist/lib/utils/normalization.ts
dosubot
bot
added
platform
|
I just need to test it, looks good overall, will approve once I test it in the morning 🙏 |
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/features/ee/organizations/pages/settings/other-team-members-view.tsx (1)
195-195: Consider clarifying the margin utilities.The combination of
my-8andmt-6is confusing becausemt-6overrides the top margin frommy-8, resulting in asymmetric spacing (top: 1.5rem, bottom: 2rem).Apply this diff for clarity:
- <hr className="border-subtle my-8 mt-6" /> + <hr className="border-subtle mb-8 mt-6" />apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (2)
53-63: Consider adding loading state to delete button.The delete mutation properly invalidates the query and handles errors. However, the UI provides no visual feedback during the deletion process (e.g., button disabled state, spinner).
To improve UX, you could disable the delete button while the mutation is pending:
const deleteWatchlistEntry = trpc.viewer.organizations.deleteWatchlistEntry.useMutation({ onSuccess: async () => { await utils.viewer.organizations.listWatchlistEntries.invalidate(); showToast(t("blocklist_entry_deleted"), "success"); setShowDeleteDialog(false); setEntryToDelete(null); }, onError: (error) => { showToast(error.message, "error"); }, });Then in the ConfirmationDialogContent (line 197-203), pass
isPending={deleteWatchlistEntry.isPending}to disable the confirm button during deletion.
107-116: Simplify the complex audit type narrowing.The type guard logic for extracting the email from audit data is overly complex and brittle. The nested conditional type checking with inline type assertions makes this code hard to maintain.
Consider simplifying the type extraction:
cell: ({ row }) => { - const audit = row.original.audits?.[0] as - | { changedByUserId: number | null } - | { - changedByUser?: { id: number; email: string; name: string | null } | undefined; - changedByUserId: number | null; - } - | undefined; - const email = - (audit && "changedByUser" in audit ? audit.changedByUser?.email : undefined) ?? undefined; + const audit = row.original.audits?.[0]; + const email = audit && "changedByUser" in audit + ? audit.changedByUser?.email + : undefined; return <span className="text-default">{email ?? "—"}</span>; },Or extract this logic into a helper function for reusability and testability:
function getCreatorEmail(audits?: BlocklistEntry["audits"]): string | undefined { const audit = audits?.[0]; if (!audit || !("changedByUser" in audit)) return undefined; return audit.changedByUser?.email; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/web/modules/settings/organizations/privacy/blocklist-table.tsx(1 hunks)packages/features/ee/organizations/pages/settings/other-team-members-view.tsx(2 hunks)packages/features/ee/organizations/pages/settings/privacy.tsx(2 hunks)packages/features/ee/teams/components/MakeTeamPrivateSwitch.tsx(0 hunks)packages/features/ee/teams/pages/team-settings-view.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/features/ee/teams/components/MakeTeamPrivateSwitch.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/ee/organizations/pages/settings/other-team-members-view.tsxpackages/features/ee/teams/pages/team-settings-view.tsxapps/web/modules/settings/organizations/privacy/blocklist-table.tsxpackages/features/ee/organizations/pages/settings/privacy.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/organizations/pages/settings/other-team-members-view.tsxpackages/features/ee/teams/pages/team-settings-view.tsxapps/web/modules/settings/organizations/privacy/blocklist-table.tsxpackages/features/ee/organizations/pages/settings/privacy.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/organizations/pages/settings/other-team-members-view.tsxpackages/features/ee/teams/pages/team-settings-view.tsxapps/web/modules/settings/organizations/privacy/blocklist-table.tsxpackages/features/ee/organizations/pages/settings/privacy.tsx
🧬 Code graph analysis (2)
apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (5)
packages/trpc/react/trpc.ts (2)
RouterOutputs(143-143)trpc(54-138)packages/ui/components/toast/showToast.tsx (1)
showToast(77-101)apps/web/modules/settings/organizations/privacy/components/create-blocklist-entry-modal.tsx (1)
CreateBlocklistEntryModal(25-173)apps/web/modules/settings/organizations/privacy/components/blocklist-entry-details-sheet.tsx (1)
BlocklistEntryDetailsSheet(20-131)packages/ui/components/dialog/Dialog.tsx (1)
Dialog(35-38)
packages/features/ee/organizations/pages/settings/privacy.tsx (3)
packages/features/data-table/DataTableProvider.tsx (1)
DataTableProvider(93-426)packages/features/data-table/hooks/useSegments.ts (1)
useSegments(9-66)apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (1)
BlocklistTable(30-207)
🔇 Additional comments (8)
packages/features/ee/organizations/pages/settings/other-team-members-view.tsx (1)
41-41: LGTM! Minor formatting cleanup.Removing the trailing space improves code consistency.
packages/features/ee/teams/pages/team-settings-view.tsx (1)
175-182: LGTM! Good cleanup and layout refinement.The changes improve code quality:
- Removing the unused
useLocalehook fromPrivacySettingsView(noted in the summary) eliminates dead code since this component delegates all text rendering to its children.- Wrapping
MakeTeamPrivateSwitchin adivwithmt-6provides consistent vertical spacing in the layout.Both changes are appropriate and maintain the component's functionality.
apps/web/modules/settings/organizations/privacy/blocklist-table.tsx (3)
40-49: LGTM: Correct React Query v5 pattern.The use of
keepPreviousDatawithplaceholderDatais the correct pattern for React Query v5, preventing jarring UI updates during pagination/filtering.
156-163: LGTM: Manual pagination correctly configured.The table setup properly uses
manualPaginationmode with correctpageCountcalculation. This delegates pagination to the server-side query, which is the right approach for potentially large datasets.
165-206: LGTM: Well-structured table with proper permission guards.The component correctly:
- Wraps table in DataTableWrapper with loading states
- Conditionally renders create/delete actions based on permissions
- Separates concerns with dedicated modal/sheet components
- Provides consistent user feedback via toasts
packages/features/ee/organizations/pages/settings/privacy.tsx (3)
3-10: LGTM: Clean integration of required dependencies.All necessary imports are added for the blocklist feature integration. The imports follow the project's conventions with proper aliasing (
@calcompaths).
12-22: LGTM: Props interface cleanly extended.The addition of optional
watchlistPermissionsprop maintains backward compatibility while enabling the new blocklist feature. The permission structure (canRead,canCreate,canDelete) provides proper granular access control.
41-53: LGTM: Proper conditional rendering with data table integration.The blocklist section is correctly:
- Gated behind
watchlistPermissions?.canReadpermission check- Wrapped in
DataTableProviderwithuseSegmentshook for state management- Configured with a reasonable
defaultPageSizeof 25- Using localized strings for heading and description
The BlocklistTable component handles its own create/delete permission checks internally, so only the read permission needs to be verified at this level.
emrysal
left a comment
emrysal
left a comment
There was a problem hiding this comment.
Re-approving Volnei's earlier approval and adding my own stamp.
6 participants
What does this PR do?
Moved Booking Report Table from #24396 to this PR
Screen.Recording.2025-10-14.at.9.12.04.PM.mov
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?