Policy Archiving Should not be a Status it should be its own value, in db, and button with confirmation#222
Conversation
…n db, and button with confirmation
WalkthroughThis PR introduces a set of changes that add and refine archiving capabilities for organization policies. A new archive action is implemented with input validation and authorization checks, and the UI is updated to reflect policy archive states. The changes further extend filtering and state management in policy tables, update compliance calculations, and adjust styling in various components. Additionally, the employee onboarding process is reworked to send portal invitations instead of creating default tasks. Database schemas and migrations have been updated accordingly, and a new email template for portal invites has been added. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PO as PolicyOverview
participant PAS as PolicyArchiveSheet
participant API as ArchivePolicy API
participant DB as Database
U->>PO: Click archive/restore button
PO->>PAS: Open archive sheet
PAS->>API: Submit archive/restore request
API->>DB: Verify user authorization and fetch policy
DB-->>API: Return policy data/error
API->>DB: Update `isArchived` and `archivedAt` fields
DB-->>API: Return update result
API-->>PAS: Respond with success/failure
PAS->>PO: Notify user and update UI
sequenceDiagram
participant U as User
participant PT as PoliciesTable
participant Ctx as PoliciesTableContext
participant Hook as usePolicies Hook
participant API as GetPolicies API
U->>PT: Toggle archived filter
PT->>Ctx: Update `isArchived` state
Ctx->>Hook: Trigger data fetch with new filter
Hook->>API: Request policies with `isArchived` parameter
API-->>Hook: Return filtered policies
Hook-->>Ctx: Update context state
Ctx-->>PT: Render updated policies list
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/hooks/useEvidenceTasks.ts (1)
54-54: Consider removing console.log statementThere's a console.log statement that outputs evidence task results. While useful during development, consider removing it in production code.
- console.log("Evidence tasks result:", result);apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/types/index.ts (1)
21-21: Type inconsistency between schema and input.The
isArchivedfield is defined as a boolean in thepolicySchemabut as a string in thepoliciesInputSchema. While this is likely because inputs often come as strings from query parameters, consider adding a transformation to ensure consistent typing.- isArchived: z.string().optional(), + isArchived: z.string().optional().transform((val) => + val ? val === 'true' : undefined + ),This would transform string values like "true" to actual boolean values, making the types more consistent across your application.
packages/email/emails/invite-portal.tsx (2)
72-86: Consider adding more context to the invitation linkThe link is presented with minimal context. For better usability, you might want to include information about link expiration time (if applicable) or further instructions.
- or copy and paste this URL into your browser{" "} + or copy and paste this URL into your browser if the button doesn't work. This link will expire in 24 hours.{" "}
90-94: Security enhancement for email presentationIncluding the email in the template is good for clarity, but consider obfuscating part of the email for added security in case the email is forwarded.
- <span className="text-[#121212] ">{email}</span>.{" "} + <span className="text-[#121212] ">{email.substring(0, 2)}...{email.substring(email.indexOf('@'))}</span>.{" "}apps/app/src/actions/policies/archive-policy.ts (2)
48-50: Optimize state change determinationThe current implementation doesn't check if the policy is already in the desired state, which could lead to unnecessary database operations.
- const shouldArchive = - action === "archive" || (action === undefined && !policy.isArchived); + // Determine if we should change the current state + const shouldArchive = + action === "archive" || (action === undefined && !policy.isArchived); + + // Skip update if policy is already in the desired state + if (shouldArchive === policy.isArchived) { + return { + success: true, + isArchived: shouldArchive, + }; + }
69-75: Enhance error logging and handlingWhile the error handling is good, consider including more specific error details in the log and potentially differentiating between different types of errors.
- console.error(error); + console.error("Failed to update policy archive status:", error); return { success: false, - error: "Failed to update policy archive status", + error: error instanceof Error + ? `Failed to update policy archive status: ${error.message}` + : "Failed to update policy archive status", };apps/app/src/components/policies/policy-overview.tsx (1)
33-60: Add null check for archivedAtWhile you do check if archivedAt exists before formatting the date, the conditional expression could be simplified for better readability.
- {policy.archivedAt && ( - <> - {t("policies.archive.archived_on")}{" "} - {format(new Date(policy.archivedAt), "PPP")} - </> - )} + {policy.archivedAt + ? `${t("policies.archive.archived_on")} ${format(new Date(policy.archivedAt), "PPP")}` + : t("policies.archive.archived_no_date")}apps/app/src/components/forms/policies/policy-overview.tsx (1)
26-26: Unused import detectedThe FormDescription component is imported but not used in this file.
- FormMessage, - FormDescription, + FormMessage, } from "@bubba/ui/form";apps/app/src/lib/db/employee.ts (1)
3-8: Improve environment variable validation handling.The environment variable check is performed at the module level which will cause the app to crash during initialization if
NEXT_PUBLIC_PORTAL_URLis missing. This approach prevents the application from starting without the variable, which might be appropriate, but it's worth considering if this check should be moved into the relevant function instead.Consider moving this check inside the
inviteEmployeeToPortalfunction instead of at module level, which would provide better error handling and allow the rest of the application to function even if this specific feature is unavailable:-if (!process.env.NEXT_PUBLIC_PORTAL_URL) { - throw new Error("NEXT_PUBLIC_PORTAL_URL is not set"); -} const inviteEmployeeToPortal = async (params: { email: string; organizationName: string; inviteLink: string; }) => { + if (!process.env.NEXT_PUBLIC_PORTAL_URL) { + throw new Error("NEXT_PUBLIC_PORTAL_URL is not set"); + } const { email, organizationName, inviteLink } = params;apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/hooks/usePoliciesTableContext.tsx (1)
1-170: LGTM - Well implemented context provider for policies table.This implementation of the PoliciesTableContext is well structured with proper separation of concerns, good state management, and appropriate error handling when the hook is used outside of its provider. The addition of the archive state is integrated consistently throughout the component.
A few minor improvements to consider:
- The
policiestype is currentlyany[]- consider using a more specific type to improve type safety:- policies: any[] | undefined; + policies: PolicyType[] | undefined;
- Consider using AbortController for cleanup in useEffect instead of setTimeout which could lead to race conditions:
useEffect(() => { if (policies && isSearching) { - const timer = setTimeout(() => { - setIsSearching(false); - }, 100); - return () => clearTimeout(timer); + const controller = new AbortController(); + const timeoutId = setTimeout(() => { + if (!controller.signal.aborted) { + setIsSearching(false); + } + }, 100); + return () => { + controller.abort(); + clearTimeout(timeoutId); + }; } }, [policies, isSearching]);apps/app/src/jobs/tasks/organization/create-organization.ts (1)
223-231: Handle potential null organization safely.The code retrieves the organization for placeholder replacement but doesn't handle the case where the organization may not be found.
Add error handling to ensure organization exists before proceeding:
const organization = await db.organization.findUnique({ where: { id: organizationId, }, }); + if (!organization) { + logger.warn("Organization not found when creating organization policy", { + organizationId, + }); + } const currentDate = new Date(); const formattedDate = currentDate.toISOString().split("T")[0]; // YYYY-MM-DD formatapps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/[policyId]/editor/actions/update-policy.ts (2)
49-49: Consider removing console.log statements for production.There are several console.log statements throughout the code that appear to be for debugging purposes. These should generally be removed before pushing to production or replaced with proper logging utilities.
- console.log("CONTENT TYPE:", typeof content); + // Remove or replace with proper loggingSimilarly for lines 59, 63, and 101.
Also applies to: 59-59, 63-63, 101-101
46-46: Consider using a more specific type for updateData.Using
Record<string, any>reduces type safety. Consider creating a more specific type that reflects the possible fields of the update operation.- const updateData: Record<string, any> = {}; + const updateData: Partial<{ + content: any; + status: string; + // Add other possible fields + }> = {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
apps/app/src/actions/policies/archive-policy.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/components/table/EmployeesTable.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/hooks/useEvidenceTasks.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/overview/components/RequirementStatusChart.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/overview/frameworks/[frameworkId]/components/FrameworkOverview.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/actions/get-policies.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/PoliciesList.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/PoliciesTable.tsx(4 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/columns.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/components/filterCategories.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/components/filterConfigs.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx(6 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/hooks/usePolicies.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/hooks/usePoliciesTableContext.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/types/index.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/[policyId]/editor/actions/update-policy.ts(1 hunks)apps/app/src/components/forms/policies/policy-overview.tsx(4 hunks)apps/app/src/components/header.tsx(1 hunks)apps/app/src/components/policies/policy-overview.tsx(3 hunks)apps/app/src/components/policies/sheets/policy-archive-sheet.tsx(1 hunks)apps/app/src/components/tables/people/empty-states.tsx(1 hunks)apps/app/src/components/tables/people/filter-toolbar.tsx(1 hunks)apps/app/src/jobs/tasks/organization/create-organization.ts(5 hunks)apps/app/src/lib/db/employee.ts(3 hunks)apps/app/src/locales/features/people.ts(1 hunks)apps/app/src/locales/features/policies.ts(2 hunks)packages/db/prisma/migrations/20250328192535_move_archive_to_its_own_column_in_policies/migration.sql(1 hunks)packages/db/prisma/schema/policy.prisma(1 hunks)packages/db/prisma/schema/schema.prisma(0 hunks)packages/email/emails/invite-portal.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/db/prisma/schema/schema.prisma
🧰 Additional context used
🧬 Code Definitions (4)
apps/app/src/components/policies/policy-overview.tsx (1)
apps/app/src/components/policies/sheets/policy-archive-sheet.tsx (1)
PolicyArchiveSheet(22-160)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/components/filterCategories.tsx (1)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/components/filterConfigs.tsx (1)
ARCHIVED_FILTER(21-25)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/hooks/usePoliciesTableContext.tsx (2)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx (2)
PoliciesTableProvider(49-157)usePoliciesTable(159-169)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/hooks/usePolicies.ts (1)
usePolicies(29-68)
apps/app/src/lib/db/employee.ts (1)
packages/email/emails/invite-portal.tsx (1)
InvitePortalEmail(23-104)
🔇 Additional comments (68)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/evidence/hooks/useEvidenceTasks.ts (1)
6-10: Improved import formatting for better readabilityThe multi-line import style for types from
@bubba/db/typesimproves code readability, especially when importing multiple items.apps/app/src/locales/features/people.ts (1)
59-60: Improved success message clarityThe updated success message now properly informs users that an email invitation will be sent when adding an employee, which aligns with the new portal invitation workflow.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/PoliciesList.tsx (2)
19-19: Added import for CreatePolicySheet componentThe new import adds the policy creation sheet component which will be used in the policies list interface.
37-37: Integrated CreatePolicySheet into the PoliciesList componentThe CreatePolicySheet component has been added to the PoliciesList UI, enabling users to create new policies within the context of the policies list view.
apps/app/src/components/tables/people/filter-toolbar.tsx (1)
3-3:Details
✅ Verification successful
Updated import path for EmployeeInviteSheet
The import path has been updated to reflect the new location/name of the employee invite component. This change aligns with the reworked employee onboarding process mentioned in the PR summary.
Make sure this import path change has been consistently applied across all components that use the EmployeeInviteSheet.
🏁 Script executed:
#!/bin/bash # Verify that all imports of the EmployeeInviteSheet component use the new path rg -l "invite-user-sheet" --type ts --type tsxLength of output: 74
Updated Verification Script Needed for EmployeeInviteSheet Import Path
The initial script failed because the specified tsx file type wasn’t recognized by rg. Please run the following updated shell script—which omits explicit file type filters—to verify that the new import path ("invite-user-sheet") is used consistently across the codebase.
Once you verify the output, please confirm that the new import path has been applied to all relevant components.
🏁 Script executed:
#!/bin/bash # Verify updated import path usage for EmployeeInviteSheet across the codebase. rg -l "invite-user-sheet"Length of output: 415
Import path update verified across the codebase
The updated import path, now using the keyword "invite-user-sheet", was found in the following files:
- apps/app/src/components/tables/people/empty-states.tsx
- apps/app/src/components/sheets/add-employee-sheet.tsx
- apps/app/src/components/tables/people/filter-toolbar.tsx
- apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/components/table/EmployeesTable.tsx
This confirms that the import path change for EmployeeInviteSheet has been applied consistently. Please ensure that any future references follow this updated pattern.
apps/app/src/components/tables/people/empty-states.tsx (1)
9-9: Component source has been updated.The import path for
EmployeeInviteSheethas been updated from "invite-user-sheet" to "add-employee-sheet", which seems to align with a refactoring effort across the application. This change is consistent with similar updates in other files.apps/app/src/components/header.tsx (1)
51-51: Is disabling the NotificationCenter intentional in this PR?The NotificationCenter component has been commented out, which seems unrelated to the main purpose of this PR (policy archiving). Please verify if this change was intentional or if it was accidentally included in this PR.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/employees/all/components/table/EmployeesTable.tsx (1)
3-3: Component source has been updated.The import path for
EmployeeInviteSheethas been updated from "invite-user-sheet" to "add-employee-sheet", consistent with similar changes in other files. This appears to be part of a broader refactoring effort.packages/db/prisma/schema/policy.prisma (1)
13-14: Added fields for policy archiving.These new fields properly implement the PR objective of treating policy archiving as its own value rather than a status. The
isArchivedboolean field with a default value offalseensures existing policies aren't automatically marked as archived, and the nullablearchivedAttimestamp will track when policies were archived.After this change is merged, make sure to run the appropriate Prisma migration commands to update your database schema.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/types/index.ts (1)
14-14: Added isArchived field to policySchema.This addition correctly aligns with the Prisma schema changes and allows for representing the archive state of policies in the application.
apps/app/src/locales/features/policies.ts (3)
70-70: Improved code style consistency with trailing commaGood practice adding the trailing comma after "not-relevant" property. This makes future additions cleaner and reduces diffs when new items are added to this object.
84-99: Well-structured archive functionality stringsThe archive object with comprehensive message strings covers all UI aspects of the archiving flow including tooltips, confirmation dialogs, success/error messages, and status indicators. This provides a complete localization solution for the new archiving functionality.
100-102: Appropriate tooltip for edit actionAdding the edit tooltip string complements the archive action and provides a consistent UI experience for policy management actions.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/actions/get-policies.ts (4)
20-20: Added isArchived parameter for policy filteringGood addition of the
isArchivedparameter to the destructured parsedInput, which aligns with the PR objective of treating archiving as a separate property.
54-57: Clear implementation of archive filtering logicThe
isArchivedFilterimplementation correctly handles the boolean conversion from the string parameter and builds the appropriate filter object. The comment also clearly explains the logic.
63-63: Consistent application of archive filterThe archive filter is correctly applied to both the findMany and count queries, ensuring consistent results between the policy list and total count.
Also applies to: 106-106
88-89: Added archived-related fields to query selectionProperly including the
isArchivedandarchivedAtfields in the database query selection ensures the UI has all necessary data to show archive status.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/overview/frameworks/[frameworkId]/components/FrameworkOverview.tsx (1)
66-70: Improved compliance calculation based on controlsThe revised calculation methodology properly shifts focus from requirements to controls for determining compliance percentage. The null check for
totalControls > 0prevents division by zero errors, and the Math.round function ensures a clean integer percentage value.This change makes the compliance metrics more meaningful as it now measures fully compliant controls rather than just counting individual requirements.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/PoliciesTable.tsx (6)
10-10: Added useQueryState for policy sheet managementGood addition of the useQueryState hook for managing the create policy sheet state in the URL query string, making the UI state shareable via URL.
15-15: Updated create policy sheet state managementThe underscore variable naming convention clearly indicates the unused first return value from useQueryState, following good React practices.
29-30: Added isArchived state managementProperly destructuring the isArchived state and its setter from the context ensures consistent archive filtering across components.
41-43: Updated filter count to include archive statusThe activeFilterCount calculation now correctly includes the isArchived filter, ensuring the UI accurately reflects all active filters.
50-51: Passed archive state to filter categoriesProperly passing isArchived state and setter to the filter categories ensures the archive filter can be managed through the UI.
90-94: Updated CTA button to use sheet instead of redirectThe implementation now correctly uses setCreatePolicySheet instead of direct navigation, providing a more modern UX with an in-page sheet instead of a full page navigation.
The history: "push" option ensures proper navigation history handling when opening the sheet.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/components/filterConfigs.tsx (1)
21-25: Good implementation of archiving as a separate filterThe new
ARCHIVED_FILTERconstant aligns well with the PR objective of making policy archiving its own value rather than a status. Moving this from theSTATUS_FILTERSarray to a standalone constant properly separates the archived state from policy statuses, and using theArchivecomponent for the icon provides clear visual semantics.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/hooks/usePolicies.ts (2)
33-33: Appropriate addition of isArchived parameterAdding the
isArchivedparameter with an empty string default value is consistent with how other filter parameters are handled in this function.
44-54: Correctly integrates isArchived parameter in SWR hook and API callThe implementation properly includes the
isArchivedparameter in both the SWR cache key and the parameters passed tofetchPolicies, ensuring consistent caching and proper filtering of archived policies.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/overview/components/RequirementStatusChart.tsx (2)
37-37: Good transition to semantic color classesReplacing hardcoded color values with semantic class names (
bg-muted,text-muted-foreground,bg-secondary,bg-primary) improves theme consistency and maintainability.Also applies to: 39-40, 53-53
70-70: Card styling simplificationSimplifying the Card className to just
select-nonereduces redundancy, assuming the default Card component already provides the necessary styling.packages/email/emails/invite-portal.tsx (4)
1-16: Well-structured imports with appropriate componentsThe imports are organized neatly, separating the external react-email components from internal components. This makes the file more readable and maintainable.
17-21: Well-defined Props interface with appropriate typesThe Props interface clearly defines the required properties (
inviteLink) and the optional property (organizationName) with proper typing.
23-71: Good conditional rendering based on organizationName presenceThe component effectively handles both cases where organization name is present or absent, providing a personalized message to the recipient.
106-107: Default export matches named exportHaving both named and default exports of the same component is a good practice for module flexibility.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/components/filterCategories.tsx (4)
3-3: Appropriate import for ARCHIVED_FILTERThe import statement now correctly includes the ARCHIVED_FILTER constant, which is used in the new filter category.
11-12: Well-defined props for archived filteringThe interface now includes the necessary properties for managing archived filter state, following the same pattern as other filter states.
22-23: Consistent parameter destructuringThe function parameter destructuring has been updated to include the new props, maintaining consistency with the existing code structure.
39-51: Well-implemented archived filter categoryThe new filter category follows the same pattern as existing categories, making it consistent and easy to understand. The implementation correctly handles state updates and pagination reset.
apps/app/src/actions/policies/archive-policy.ts (5)
8-11: Well-defined schema with appropriate validationThe zod schema correctly defines the required
idproperty and an optionalactionproperty with enum constraints to ensure only valid values are provided.
26-31: Proper authorization checkThe function correctly verifies that the user is authorized by checking for the presence of user ID and organization ID before proceeding.
33-46: Good policy existence verificationThe function appropriately checks if the policy exists and belongs to the user's organization before attempting to modify it.
52-58: Good data integrity with archivedAt fieldThe update operation correctly sets both the
isArchivedflag and thearchivedAttimestamp, maintaining data integrity.
60-63: Comprehensive path revalidationThe function revalidates all relevant paths to ensure that the UI is updated appropriately after the change.
apps/app/src/components/policies/policy-overview.tsx (5)
9-9: Consistent icon importsThe icon imports are now properly organized, importing the needed archive and restore icons from Lucide.
13-15: Essential imports for new functionalityThe new imports for the PolicyArchiveSheet, date formatting, and routing are essential for the archive functionality.
29-29: Well-integrated state managementThe archiveOpen state is managed using useQueryState, consistent with the existing state management pattern for the edit functionality.
67-94: Well-implemented conditional rendering for archive/restore actionsThe button group effectively toggles between archive and restore functionality based on the policy's current state, with appropriate tooltips and icons.
116-116: Properly integrated archive sheet componentThe PolicyArchiveSheet component is correctly included in the render output, receiving the necessary props.
packages/db/prisma/migrations/20250328192535_move_archive_to_its_own_column_in_policies/migration.sql (1)
1-20: Well-structured migration for archive functionalityThe migration properly moves the "archived" concept from being a value in the PolicyStatus enum to dedicated fields (isArchived and archivedAt) in the OrganizationPolicy table. The transaction ensures atomicity, and the approach of creating a new enum, switching columns, and then renaming follows best practices.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/columns.tsx (4)
6-8: LGTM! Required imports for new archive functionalityThe imports for Archive icon, Badge component, and the cn utility function are appropriate for implementing the new archive-related UI elements.
15-16: LGTM! Interface updates for archive propertiesThe PolicyRow interface correctly adds isArchived and archivedAt properties to support the new archiving functionality.
35-56: Good UI implementation for archived policiesThe policy name cell now properly indicates archived status with visual cues - muted text and an "Archived" badge with an icon. This provides clear differentiation for archived policies in the table.
81-97: Good display of archive date informationThe implementation correctly shows "Archived on [date]" for archived policies instead of the last updated date, which provides better context to users about when the policy was archived.
apps/app/src/components/forms/policies/policy-overview.tsx (4)
44-44: LGTM! Import for new UI componentAdding the Switch component import is appropriate for the enhanced form field implementation.
46-50: LGTM! Updated policy statusesThe removal of "archived" from policyStatuses is consistent with the database migration changes, properly implementing the move from status-based archiving to field-based archiving.
89-91: LGTM! Simplified defaultValues logicThe defaultValues implementation for isRequiredToSign is now cleaner and directly uses the organizationPolicy property.
293-306: Improved UX with Switch componentReplacing the Select dropdown with a Switch component for the isRequiredToSign boolean property provides a better user experience. The implementation correctly handles the value transformation between UI state and form values.
apps/app/src/components/policies/sheets/policy-archive-sheet.tsx (5)
1-26: LGTM! Well-structured component setupThe imports, props definition, and component structure follow best practices for a React component. The component properly accepts a policy object combining OrganizationPolicy and Policy types.
27-55: Good implementation of hooks and state managementThe component effectively uses hooks for internationalization, media queries, and query state. The open state management and router navigation are well-implemented. The useAction hook provides proper handling of success and error cases with appropriate user feedback.
56-62: LGTM! Properly implemented archive/restore actionThe handleAction function correctly sends the appropriate action based on the current archive status and passes the required policy ID.
63-108: Well-designed UI content with appropriate feedbackThe content section properly handles both archive and restore scenarios with appropriate text and icons. The loading state is well-implemented, and buttons are disabled during execution to prevent duplicate submissions.
110-160: Good responsive implementationThe component correctly renders either a Sheet (desktop) or Drawer (mobile) based on screen size, providing a consistent user experience across devices. Both implementations share the same content while adapting the container to the device type.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/(overview)/components/table/hooks/usePoliciesTableContext.tsx (5)
15-43: LGTM - Well structured policy table context interface.The interface definition is well-organized with clear sections for state, setters, data, and actions. The addition of the
isArchivedproperty and related setter follows the same pattern as the other filter properties.
53-77: LGTM - Good implementation of the archived filter state.The implementation properly integrates the
isArchivedstate usinguseQueryStateand passes it to theusePolicieshook for data fetching. This approach is consistent with how other filters are managed.
79-111: LGTM - Properly updated effect dependencies and filter checks.You've correctly updated the dependencies array for the search parameter tracking effect and the active filter check to include the new
isArchivedstate.
113-120: LGTM - Properly updated filter clearing.The
clearFiltersfunction now correctly resets theisArchivedstate to null when clearing all filters.
122-150: LGTM - Context value properly updated.The context value provided to consumers now includes the
isArchivedstate and its setter, making them available to all components that use theusePoliciesTablehook.apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/all/[policyId]/editor/actions/update-policy.ts (2)
87-89: Verify the impact of clearing signedBy array on policy updates.The code now resets the
signedByarray to empty on every policy update. This could potentially erase important signature records when updating other aspects of a policy.Is this intentional behavior? This change seems related to the PR objective of separating policy archiving from status, but please ensure it aligns with the expected business logic for policy signatures.
10-18: Code readability improvement looks good.The reformatting of method chaining and function logic has improved the code's readability while maintaining the same functionality. The clear separation of concerns makes the code easier to understand and maintain.
Also applies to: 19-29, 31-44, 46-80, 82-94, 96-107
| const inviteEmployeeToPortal = async (params: { | ||
| email: string; | ||
| organizationName: string; | ||
| inviteLink: string; | ||
| }) => { | ||
| const { email, organizationName, inviteLink } = params; | ||
|
|
||
| await sendEmail({ | ||
| to: email, | ||
| subject: `You've been invited to join ${organizationName || "an organization"} on Comp AI`, | ||
| react: InvitePortalEmail({ | ||
| email, | ||
| organizationName, | ||
| inviteLink, | ||
| }), | ||
| }); | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "Employee invited to portal", | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and export the invitation function.
The inviteEmployeeToPortal function lacks error handling for the email sending process, and it's defined as a private constant despite being a core utility.
Add error handling and export the function to improve its robustness and reusability:
-const inviteEmployeeToPortal = async (params: {
+export async function inviteEmployeeToPortal(params: {
email: string;
organizationName: string;
inviteLink: string;
}) => {
const { email, organizationName, inviteLink } = params;
+ try {
await sendEmail({
to: email,
subject: `You've been invited to join ${organizationName || "an organization"} on Comp AI`,
react: InvitePortalEmail({
email,
organizationName,
inviteLink,
}),
});
return {
success: true,
message: "Employee invited to portal",
};
+ } catch (error) {
+ console.error("Failed to send invitation email:", error);
+ return {
+ success: false,
+ message: "Failed to send invitation email",
+ };
+ }
-};
+}📝 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 inviteEmployeeToPortal = async (params: { | |
| email: string; | |
| organizationName: string; | |
| inviteLink: string; | |
| }) => { | |
| const { email, organizationName, inviteLink } = params; | |
| await sendEmail({ | |
| to: email, | |
| subject: `You've been invited to join ${organizationName || "an organization"} on Comp AI`, | |
| react: InvitePortalEmail({ | |
| email, | |
| organizationName, | |
| inviteLink, | |
| }), | |
| }); | |
| return { | |
| success: true, | |
| message: "Employee invited to portal", | |
| }; | |
| }; | |
| export async function inviteEmployeeToPortal(params: { | |
| email: string; | |
| organizationName: string; | |
| inviteLink: string; | |
| }) { | |
| const { email, organizationName, inviteLink } = params; | |
| try { | |
| await sendEmail({ | |
| to: email, | |
| subject: `You've been invited to join ${organizationName || "an organization"} on Comp AI`, | |
| react: InvitePortalEmail({ | |
| email, | |
| organizationName, | |
| inviteLink, | |
| }), | |
| }); | |
| return { | |
| success: true, | |
| message: "Employee invited to portal", | |
| }; | |
| } catch (error) { | |
| console.error("Failed to send invitation email:", error); | |
| return { | |
| success: false, | |
| message: "Failed to send invitation email", | |
| }; | |
| } | |
| } |
| await inviteEmployeeToPortal({ | ||
| email: params.email, | ||
| organizationName: organization?.name || "an organization", | ||
| inviteLink: `${process.env.NEXT_PUBLIC_PORTAL_URL}`, | ||
| }); |
There was a problem hiding this comment.
Add a unique token to the invitation link.
The current invitation link doesn't include any employee-specific identification, which is required for a secure and personalized invitation process.
Update the inviteLink to include an employee identifier or token:
await inviteEmployeeToPortal({
email: params.email,
organizationName: organization?.name || "an organization",
- inviteLink: `${process.env.NEXT_PUBLIC_PORTAL_URL}`,
+ inviteLink: `${process.env.NEXT_PUBLIC_PORTAL_URL}/invite?email=${encodeURIComponent(params.email)}&orgId=${params.organizationId}`,
});Additionally, consider creating a separate token with expiration for improved security.
📝 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.
| await inviteEmployeeToPortal({ | |
| email: params.email, | |
| organizationName: organization?.name || "an organization", | |
| inviteLink: `${process.env.NEXT_PUBLIC_PORTAL_URL}`, | |
| }); | |
| await inviteEmployeeToPortal({ | |
| email: params.email, | |
| organizationName: organization?.name || "an organization", | |
| inviteLink: `${process.env.NEXT_PUBLIC_PORTAL_URL}/invite?email=${encodeURIComponent(params.email)}&orgId=${params.organizationId}`, | |
| }); |
| /** | ||
| * Processes content placeholders by converting to string, replacing values, and converting back | ||
| */ | ||
| function processContentPlaceholders( | ||
| content: InputJsonValue[], | ||
| organizationName: string, | ||
| formattedDate: string | ||
| ): InputJsonValue[] { | ||
| // Convert the content to a string | ||
| const contentString = JSON.stringify(content); | ||
|
|
||
| // Replace all occurrences of the placeholders | ||
| const processedString = contentString | ||
| .replace(/{{organization}}/g, organizationName) | ||
| .replace(/{{date}}/g, formattedDate); | ||
|
|
||
| // Convert back to an object | ||
| return JSON.parse(processedString); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to content placeholder processing.
The current implementation of processContentPlaceholders lacks error handling for potential JSON parsing errors, which could occur if the replacements introduce invalid JSON.
Add try-catch block to handle potential JSON parsing errors:
function processContentPlaceholders(
content: InputJsonValue[],
organizationName: string,
formattedDate: string
): InputJsonValue[] {
+ try {
// Convert the content to a string
const contentString = JSON.stringify(content);
// Replace all occurrences of the placeholders
const processedString = contentString
.replace(/{{organization}}/g, organizationName)
.replace(/{{date}}/g, formattedDate);
// Convert back to an object
return JSON.parse(processedString);
+ } catch (error) {
+ logger.error("Error processing content placeholders", {
+ error,
+ organizationName,
+ });
+ // Return the original content if parsing fails
+ return content;
+ }
}Additionally, consider a more robust approach for complex replacements:
function safeReplace(str, placeholder, value) {
// Escape any special characters that could break JSON
const safeValue = value.replace(/[\\"\u0000-\u001F\u007F-\u009F]/g,
char => "\\u" + ("0000" + char.charCodeAt(0).toString(16)).slice(-4));
return str.replace(new RegExp(`{{${placeholder}}}`, 'g'), safeValue);
}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx (2)
129-138:⚠️ Potential issueStatus handling for archived policies needs updating.
While the query for counting archived policies has been updated to use
isArchived: true, the status handling logic still looks forstatus === "archived". This inconsistency could lead to inaccurate counts in the assignee chart.Apply this fix:
const status = policy.status as | "published" | "draft" - | "archived" | "needs_review"; if (status === "published") ownerData.published += 1; else if (status === "draft") ownerData.draft += 1; -else if (status === "archived") ownerData.archived += 1; else if (status === "needs_review") ownerData.needs_review += 1; +// Check isArchived property separately from status +if (policy.isArchived) ownerData.archived += 1;
109-111: 💡 Verification agent🧩 Analysis chain
Update policy object type to include isArchived property.
The
policiesByAssigneeStatusitems need to include theisArchivedproperty to properly count archived policies in the status chart.
🏁 Script executed:
#!/bin/bash # Check if the query for policiesByAssigneeStatus includes isArchived field rg "isArchived.*select:" -A 20 -g "*.tsx" | grep -A 20 "policiesByAssigneeStatus"Length of output: 81
I'm not seeing any evidence that the
isArchivedfield is being selected with the current query. To better verify whether thePolicytype already includes (or is missing) theisArchivedproperty, please run an additional search. For example, you could check for thePolicyinterface definitions across the codebase:This will help confirm whether the
isArchivedproperty is defined. Once you review the output, we can determine if the type update is still required.
🏁 Script executed:
#!/bin/bash # Search for 'interface Policy' definitions and check if the isArchived property is present rg "interface Policy" -n -A 30 .Length of output: 45774
Action Required: Update the Policy Type Definition
The UI relies on the
isArchivedproperty (as seen in the table columns and status chart components) to correctly handle archived policies. However, our current Policy interface inpackages/db/prisma/seedTypes.tsdoes not include this property. Please update the Policy object type to add theisArchived(and if needed, a correspondingarchivedAt) field and ensure that any queries populatingpoliciesByAssigneeStatusinclude it.
- File to update:
packages/db/prisma/seedTypes.ts(and any related types where Policy is defined)- Action: Add the
isArchived: booleanproperty and update related queries/mutations accordingly.apps/app/src/components/policies/charts/policies-by-assignee.tsx (1)
23-28: 💡 Verification agent🧩 Analysis chain
Consider updating policyStatus object.
The
policyStatusobject still contains "archived" as a status type, but according to the PR objective, archiving is now a separate property rather than a status. You might want to update this object to reflect the new architecture.
🏁 Script executed:
#!/bin/bash # Check if "archived" is still used as a status elsewhere in the codebase rg "status\s*===\s*['\"]archived['\"]" --type=ts --type=tsxLength of output: 93
The previous command errored out due to an unrecognized file type. Please run the above shell script to verify whether any instances of
status === 'archived'still exist anywhere in the codebase. If the search confirms thatarchivedisn’t used as a status (i.e. no matches returned), then thepolicyStatusobject should be updated by removing thearchivedkey to align with the new architecture where archiving is handled as a separate property.Once you verify that there are no other dependencies on "archived" as a status, proceed with updating the code accordingly.
🏁 Script executed:
#!/bin/bash # Re-run the search for occurrences of "archived" used as a status check # Applying a glob pattern to cover both .ts and .tsx files. rg "status\s*===\s*['\"]archived['\"]" --glob "*.ts*"Length of output: 461
Action Required: Review and Align Archived Policy Handling Across Modules
- The
policyStatusobject inapps/app/src/components/policies/charts/policies-by-assignee.tsxstill defines an"archived"key.- The PR objective indicates that archiving should be managed as a separate property rather than as part of the status.
- Our search reveals that some parts of the codebase—specifically in
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx—are still checking forstatus === "archived".- Please verify the intended design for handling archived policies and update the code consistently. This may involve:
- Removing the
"archived"key from thepolicyStatusobject in the policies charts.- Updating any other modules (e.g., the policies overview page) that still use
"archived"as a status to reflect the new architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/vendors/[vendorId]/actions/update-vendor-inherent-risk.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/vendors/[vendorId]/actions/update-vendor-residual-risk.ts(2 hunks)apps/app/src/components/mobile-menu.tsx(1 hunks)apps/app/src/components/policies/charts/policies-by-assignee.tsx(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/app/src/components/mobile-menu.tsx (1)
apps/app/src/components/main-menu.tsx (1)
MainMenu(105-247)
🔇 Additional comments (7)
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/vendors/[vendorId]/actions/update-vendor-inherent-risk.ts (2)
3-3: Import path updated from local to package reference.The import statement has been changed from
@/dbto@bubba/db, which appears to be part of a broader restructuring to use a more centralized database module.
29-30: Improved error handling readability.Breaking the error handling ternary operator across multiple lines enhances code readability.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/vendors/[vendorId]/actions/update-vendor-residual-risk.ts (2)
7-7: Import path updated from local to package reference.The import statement has been changed from
@/dbto@bubba/db, consistent with the same change in other files.
28-29: Improved error handling readability.Breaking the error handling ternary operator across multiple lines enhances code readability.
apps/app/src/app/[locale]/(app)/(dashboard)/[orgId]/policies/(overview)/page.tsx (1)
68-73: Update query for archived policies to match new data model.The query now correctly uses
isArchived: trueinstead ofstatus: "archived"to identify archived policies, which aligns with the PR's objective of moving archive state from status to its own field.apps/app/src/components/policies/charts/policies-by-assignee.tsx (2)
48-48: Filtering condition updated correctly.The change from
policy.status === "archived"topolicy.isArchivedcorrectly implements the PR objective of making archiving a separate property rather than a status.
224-224: Database selection properly updated.Adding
isArchivedto the select statement ensures the property is available for the updated filtering logic.
| organizationId={organizationId} | ||
| userIsAdmin={isAdmin} | ||
| /> | ||
| <MainMenu organizationId={organizationId} userIsAdmin={isAdmin} /> |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Potential UX regression: mobile menu might not close on item selection.
The removal of the onSelect prop (previously onSelect={() => setOpen(false)}) means the mobile menu may no longer automatically close when a user selects a menu item. This could create a suboptimal user experience where users must manually close the menu after navigation.
Consider restoring the auto-close behavior:
-<MainMenu organizationId={organizationId} userIsAdmin={isAdmin} />
+<MainMenu organizationId={organizationId} userIsAdmin={isAdmin} onSelect={() => setOpen(false)} />🏁 Script executed:
#!/bin/bash
# Check if MainMenu component still accepts onSelect prop
ast-grep --pattern $'export function MainMenu({ $props }) { $$$1 }' apps/app/src/components/main-menu.tsxLength of output: 106
I'll run an additional check to see if the MainMenu component still references or destructures an onSelect prop. This additional verification will help determine whether the auto-close behavior was removed accidentally. For example, we can search for "onSelect" within the MainMenu file:
Please run this script manually (or share its output) to confirm if the MainMenu component still accepts an onSelect prop. If no references to onSelect are found, it’s likely that the mobile menu auto-close behavior no longer exists, and you should restore the behavior as specified:
-<MainMenu organizationId={organizationId} userIsAdmin={isAdmin} />
+<MainMenu organizationId={organizationId} userIsAdmin={isAdmin} onSelect={() => setOpen(false)} />Action Required:
- Verify manually whether the MainMenu component in
apps/app/src/components/main-menu.tsxhandles theonSelectprop. - If it does not, please restore the auto-close behavior by adding the
onSelect={() => setOpen(false)}prop.
🏁 Script executed:
#!/bin/bash
# Search for any usage of "onSelect" within MainMenu to verify if it supports the onSelect prop.
rg "onSelect" apps/app/src/components/main-menu.tsxLength of output: 173
Action Required: Restore Mobile Menu Auto-Close Functionality
After verifying via the absence of any "onSelect" references in the MainMenu component, it appears that the component no longer supports an auto-close behavior. To ensure the mobile menu closes on item selection, please restore the previously removed behavior by reintroducing the onSelect prop.
- Affected File:
apps/app/src/components/mobile-menu.tsxat line 35 - Required Change:
-<MainMenu organizationId={organizationId} userIsAdmin={isAdmin} />
+<MainMenu organizationId={organizationId} userIsAdmin={isAdmin} onSelect={() => setOpen(false)} />Please verify manually in the MainMenu component if any handling for onSelect exists. If not, applying this change will restore the intended UX behavior.
📝 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.
| <MainMenu organizationId={organizationId} userIsAdmin={isAdmin} /> | |
| <MainMenu organizationId={organizationId} userIsAdmin={isAdmin} onSelect={() => setOpen(false)} /> |
1 participant
Summary by CodeRabbit
New Features
Style
Chores