feat: Create Policies#138
Conversation
…ment schema validation and integrate with server actions for policy creation.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces a suite of features centered on policy creation. A new action ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as FilterToolbar
participant S as CreatePolicySheet
participant F as CreateNewPolicyForm
participant A as createPolicyAction
participant DB as Database
participant UI as NotificationSystem
U->>T: Click "Create Policy" button
T->>S: Set open state to true
S->>F: Render policy creation form
U->>F: Fill and submit form data
F->>A: Call createPolicyAction with form data
A->>DB: Insert new policy record
DB-->>A: Return success or error response
A-->>F: Pass result (success/error)
F->>UI: Display toast notification (success/error)
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Shipped from 39,000 feet |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/app/src/actions/schema.ts (1)
271-276: Add validation constraints to the createPolicySchema.While the schema structure is appropriate, it lacks validation constraints that are present in other schemas in this file.
Consider adding validation constraints for improved data integrity and consistent user feedback:
export const createPolicySchema = z.object({ - title: z.string(), - description: z.string(), + title: z.string({ + required_error: "Policy title is required", + }).min(1, { + message: "Policy title should be at least 1 character", + }).max(100, { + message: "Policy title should be at most 100 characters", + }), + description: z.string({ + required_error: "Policy description is required", + }).min(1, { + message: "Policy description should be at least 1 character", + }).max(255, { + message: "Policy description should be at most 255 characters", + }), frameworkIds: z.array(z.string()), controlIds: z.array(z.string()), });apps/app/src/components/sheets/create-policy-sheet.tsx (1)
23-45: Fix potentially invalid Tailwind class in desktop view.The component handles desktop view appropriately with a Sheet component, but there's a potential issue with the Tailwind classes used.
The
size-autoclass in line 32 might not be a standard Tailwind class. Consider replacing it with more standard sizing classes:<Button size="icon" variant="ghost" - className="p-0 m-0 size-auto hover:bg-transparent" + className="p-0 m-0 h-auto w-auto hover:bg-transparent" onClick={() => setOpen(null)} >apps/app/src/actions/policies/create-new-policy.ts (3)
21-26: Ensure uniqueness fortitleused as aslug.Currently, the
slugfield is directly set to thetitle, which may lead to potential collisions if different policies share the same title. Consider implementing a slug-generation strategy (e.g., appending random suffixes, or using a library likeslugify) to ensure uniqueness and avoid conflicts.
47-55: Consider customizing defaultdepartmentandfrequency.Hard-coding
Departments.noneandFrequency.monthlymight not always be suitable. If these defaults can vary by organization or context, it might be more flexible to either accept them from user input (with safe defaults) or store them in a configuration file.
65-71: Enhance error handling details.For debugging or user feedback, consider including the exact error message (e.g.,
error.message) rather than returning a generic "Failed to create policy" message. This will simplify troubleshooting issues in both development and production.console.error(error); return { - success: false, - error: "Failed to create policy", + success: false, + error: error instanceof Error ? error.message : "Failed to create policy", };apps/app/src/components/tables/policies/filter-toolbar.tsx (2)
128-132: Unify button label for consistent UX.The label is set to
t("common.actions.addNew"). If you want consistent terminology, consider standardizing across both the mobile and desktop button to either “Create New Policy” or “Add Policy” in alignment witht("policies.create_new").<Button onClick={() => setOpen("true")} variant="action"> <Plus className="h-4 w-4" /> - {t("common.actions.addNew")} + {t("policies.create_new")} </Button>
184-187: Avoid dual button usage for the same action.Users see two separate buttons for creating a policy—one for mobile (
lines 128-132) and one for desktop (lines 184-187). Although it’s intentional for responsive design, ensure messaging remains consistent across devices, potentially reusing the same text or style. This duplication might be simplified by conditionally rendering one component or by reusing the same label for uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/app/languine.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
apps/app/src/actions/policies/create-new-policy.ts(1 hunks)apps/app/src/actions/schema.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/(home)/overview/frameworks/controls/[id]/components/SingleControlSkeleton.tsx(1 hunks)apps/app/src/components/forms/policies/create-new-policy.tsx(1 hunks)apps/app/src/components/sheets/create-policy-sheet.tsx(1 hunks)apps/app/src/components/tables/policies/filter-toolbar.tsx(3 hunks)apps/app/src/locales/en.ts(1 hunks)apps/app/src/locales/es.ts(1 hunks)apps/app/src/locales/fr.ts(1 hunks)apps/app/src/locales/no.ts(1 hunks)apps/app/src/locales/pt.ts(1 hunks)
🔇 Additional comments (12)
apps/app/src/app/[locale]/(app)/(dashboard)/(home)/overview/frameworks/controls/[id]/components/SingleControlSkeleton.tsx (1)
29-29: Minor styling update to remove rounded cornersThe
rounded-mdclass has been removed from this skeleton loader div. This appears to be an intentional UI consistency change for the loading state.apps/app/src/locales/pt.ts (1)
361-367: Successfully added Portuguese translations for policy creationThe new translations for the policy creation feature are appropriately structured and follow the same pattern as other localization entries. These strings will provide proper user feedback during policy creation in Portuguese.
apps/app/src/locales/es.ts (1)
361-367: Successfully added Spanish translations for policy creationThe added translations for the policy creation feature are well-structured and consistent with both the existing Spanish localization patterns and the corresponding translations in other language files.
apps/app/src/locales/no.ts (1)
361-367: Successfully added Norwegian translations for policy creationThe Norwegian translations for the policy creation feature have been properly implemented, maintaining consistent structure with both the existing localization patterns and the equivalent translations in other language files.
apps/app/src/locales/fr.ts (1)
361-367: New translations for policy creation added correctly.The French translations for the policy creation feature have been added appropriately, with clear and concise messages for:
- Success notification
- Error notification
- Section titles and form field labels
These translations align well with the policy creation functionality being implemented.
apps/app/src/locales/en.ts (1)
362-368: Clear and consistent English translations for policy creation.The English translation strings for the policy creation feature are concise and user-friendly. They properly match the structure seen in the French translations, maintaining consistency across languages.
apps/app/src/components/sheets/create-policy-sheet.tsx (2)
1-12: Appropriate imports and setup for the component.The component imports necessary UI components from the Bubba UI library and uses appropriate hooks for internationalization, responsive design, and state management.
13-22: Well-structured component with responsive behavior.The component correctly uses the
useMediaQueryhook to determine viewport size anduseQueryStateto manage the open/closed state through URL parameters, which is a good practice for maintaining state across page refreshes and navigation.apps/app/src/actions/policies/create-new-policy.ts (1)
28-45:Details
❓ Verification inconclusive
Validate user input for collisions or improper references.
While you create references for frameworks (
policyFrameworks) and controls (PolicyControl) using IDs from the request, there’s no check for whether they actually exist in the database. If the IDs are invalid or belong to a resource outside of the user’s organization, your code may create inconsistent records. Consider adding a quick validation step or referencing constraints to ensure the IDs correspond to valid frameworks and controls within the same organization (if applicable).
🏁 Script executed:
#!/bin/bash # This script demonstrates how you might discover non-existent references to frameworks or controls. # We search for the policy creation logic to verify if there's an existing validation. rg -A 10 "map((id) => ({ frameworkId: id }))"Length of output: 170
Below is a revised review comment that asks you to take a closer look at input validation. Since our initial regex search failed (likely due to regex parsing issues) and a fixed-string search for built-in validations hasn’t produced evidence that incoming IDs are verified, please manually verify that any reference IDs (both for frameworks and controls) undergo validation before being used. Consider adding a check that confirms the referenced records exist (and belong to the user’s organization, if applicable) before creating the policy.
Action Required: Validate Policy Reference IDs
• In fileapps/app/src/actions/policies/create-new-policy.ts(lines 28–45), ensure that the IDs provided inframeworkIdsandcontrolIdsare verified—i.e. confirm that each provided ID corresponds to an existing record (and to a record from the correct organization).
• If relying solely on the database’s foreign key constraints isn’t enough (for example, if collision handling or more expressive error messages are desired), add an explicit validation step before the policy is created.apps/app/src/components/forms/policies/create-new-policy.tsx (2)
60-127: Form UX improvements.
- Prevent double submissions: The submit button is disabled while
createPolicy.status === "executing", which is good. However, consider also disabling form inputs or providing a loading indicator to enhance clarity.- Validation errors: You are using Zod with
react-hook-form; this is solid. Ensure that you show relevant field-level error messages if a user fails validation. TheFormMessagecomponents are present, so you may want to confirm they display all validation messages effectively (e.g., iftitleis required but omitted).- Handling additional fields: If you plan to incorporate more advanced policy fields in the future (like
departments,frequency, etc.), it’s straightforward to expand the Zod schema, so this is well-prepared for extension.
36-44:Details
❓ Verification inconclusive
Add handling for edge cases or empty arrays.
When
frameworkIdsorcontrolIdsarrays are empty, it might be a valid scenario or an oversight. If your system requires at least one framework or control, prompt the user or handle that logic gracefully. Otherwise, clarify that empty arrays are acceptable inputs.
🏁 Script executed:
#!/bin/bash # Checking where createPolicyForm is used to confirm that empty arrays are valid scenario rg -A 5 'CreateNewPolicyForm'Length of output: 1998
I'll now run a shell script to search for mentions of
frameworkIdsandcontrolIdsin thecreate-new-policy.tsxfile to verify if there's any handling for empty arrays within this file.
🏁 Script executed:
#!/bin/bash # Search for frameworkIds in the file rg -n "frameworkIds" apps/app/src/components/forms/policies/create-new-policy.tsx # Search for controlIds in the file rg -n "controlIds" apps/app/src/components/forms/policies/create-new-policy.tsxLength of output: 213
Action Required: Clarify Handling of Empty Arrays in CreateNewPolicyForm
The current implementation in
apps/app/src/components/forms/policies/create-new-policy.tsxinitializes bothframeworkIdsandcontrolIdsas empty arrays (see lines 51-52). Please confirm whether empty arrays are valid inputs:
- If a policy should always include at least one framework or control, add validation and prompt the user appropriately.
- If empty arrays are acceptable, please add a clarifying comment or documentation to make this behavior explicit.
apps/app/src/components/tables/policies/filter-toolbar.tsx (1)
190-190: Confirmation of dynamic route state.
<CreatePolicySheet />relies onuseQueryState("create-policy-sheet"). If users navigate away from the page, ensure that the state is reset or remains valid on revisit to avoid confusion (e.g., returning to the page with the "create-policy-sheet" query param set might unexpectedly open the sheet). You can choose to reset or ignore stale query params if that’s undesirable.
| return ( | ||
| <Drawer open={isOpen} onOpenChange={handleOpenChange}> | ||
| <DrawerTitle hidden>{t("policies.create_new")}</DrawerTitle> | ||
| <DrawerContent className="p-6"> | ||
| <CreateNewPolicyForm /> | ||
| </DrawerContent> | ||
| </Drawer> | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure accessibility of drawer title for screen readers.
The mobile view uses a Drawer component with a hidden title, which might affect accessibility.
The hidden prop on DrawerTitle may hide the title from screen readers. Consider alternative approaches to visually hide while keeping it accessible:
- <DrawerTitle hidden>{t("policies.create_new")}</DrawerTitle>
+ <DrawerTitle className="sr-only">{t("policies.create_new")}</DrawerTitle>Or if the Bubba UI library's DrawerTitle handles this accessibility concern internally, you may validate that with:
🏁 Script executed:
#!/bin/bash
# Search for DrawerTitle component implementation to verify accessibility handling
fd --type f --extension tsx --extension jsx --exec grep -l "DrawerTitle" {} \; | xargs catLength of output: 28519
Action Required: Update Drawer Title for Accessibility
Please replace the use of the hidden prop on <DrawerTitle> with a visually hidden class (e.g. sr-only) so that the title remains available to screen readers. The current implementation forwards the hidden prop to the underlying element, which results in the title being hidden from all users—including those using assistive technologies.
For example, in the file apps/app/src/components/sheets/create-policy-sheet.tsx at lines 47–54, change:
- <DrawerTitle hidden>{t("policies.create_new")}</DrawerTitle>
+ <DrawerTitle className="sr-only">{t("policies.create_new")}</DrawerTitle>This approach visually hides the title while ensuring it remains accessible. Please also verify that this change is consistent with the intended behavior of the Bubba UI library and does not conflict with any internal accessibility handling.
📝 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.
| return ( | |
| <Drawer open={isOpen} onOpenChange={handleOpenChange}> | |
| <DrawerTitle hidden>{t("policies.create_new")}</DrawerTitle> | |
| <DrawerContent className="p-6"> | |
| <CreateNewPolicyForm /> | |
| </DrawerContent> | |
| </Drawer> | |
| ) | |
| return ( | |
| <Drawer open={isOpen} onOpenChange={handleOpenChange}> | |
| <DrawerTitle className="sr-only">{t("policies.create_new")}</DrawerTitle> | |
| <DrawerContent className="p-6"> | |
| <CreateNewPolicyForm /> | |
| </DrawerContent> | |
| </Drawer> | |
| ) |
2 participants
Added the ability to create custom policies
Summary by CodeRabbit
New Features
Style