-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Edit policy UI #472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Edit policy UI #472
Conversation
WalkthroughThis pull request introduces several new components and pages to support policy management and editing within the workspace application. The changes add navigation capabilities (using Next.js’s Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "Policy UI"
participant NextJS as "Next.js App"
participant API as "Policy API"
User->>UI: Click "Edit" on a policy
UI->>NextJS: Navigate using Link (with workspaceSlug & policyId)
NextJS->>API: Request policy data
API-->>NextJS: Return policy details
NextJS->>UI: Render policy edit components (tabs, configuration, etc.)
sequenceDiagram
participant Client
participant Router as "PolicyRouter"
participant Tx as "Transaction System"
participant DB as "Database"
Client->>Router: Send update policy request
Router->>Tx: Begin transaction (updatePolicyInTx)
Tx->>DB: Update core policy data and related fields
DB-->>Tx: Return update results
Tx-->>Router: Commit transaction, return updated policy
Router-->>Client: Respond with updated policy data
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (1)
✨ 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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (17)
packages/api/src/router/policy/time-util.ts (1)
3-15: Consider adding error handling and JSDoc commentsThe function lacks input validation and documentation. Adding proper error handling and documentation would improve code maintainability.
+/** + * Converts a date from local timezone to UTC + * @param date The date to convert + * @param timeZone The timezone string (e.g., 'America/New_York') + * @returns A new Date object representing the same time in UTC + */ export const getLocalDateAsUTC = (date: Date, timeZone: string) => { + if (!date || !(date instanceof Date)) { + throw new Error('Invalid date parameter'); + } + if (!timeZone || typeof timeZone !== 'string') { + throw new Error('Invalid timeZone parameter'); + } const parts = getDatePartsInTimeZone(date, timeZone); return new Date( Date.UTC( parts.year, parts.month - 1, // Adjusted for 0-based month indexing parts.day, parts.hour, parts.minute, parts.second, ), ); };apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsx (1)
8-11: Add error handling for redirection processThe code doesn't handle potential errors during URL construction or redirection. Consider adding try/catch blocks to handle errors gracefully.
export default async function PolicyPage(props: { params: Promise<{ workspaceSlug: string; policyId: string }>; }) { + try { const { workspaceSlug, policyId } = await props.params; return redirect( urls.workspace(workspaceSlug).policies().edit(policyId).baseUrl(), ); + } catch (error) { + console.error("Failed to redirect to policy edit page:", error); + // Handle the error appropriately, e.g., redirect to an error page + throw new Error("Failed to redirect to policy edit page"); + } }apps/webservice/src/app/urls.ts (1)
50-58: Add JSDoc comments to workspacePolicyEdit functionThe function follows the naming convention of other URL builder functions, but lacks documentation. Consider adding JSDoc comments to explain the purpose of this function and its parameters.
+/** + * Generates URLs for policy editing pages + * @param slug The workspace slug + * @param policyId The ID of the policy to edit + * @returns Object with methods to generate URLs for different policy edit pages + */ const workspacePolicyEdit = (slug: string, policyId: string) => { const base = [slug, "policies", policyId, "edit"]; return { baseUrl: () => buildUrl(...base), configuration: () => buildUrl(...base, "configuration"), timeWindows: () => buildUrl(...base, "time-windows"), deploymentFlow: () => buildUrl(...base, "deployment-flow"), qualitySecurity: () => buildUrl(...base, "quality-security"), }; };packages/api/src/router/policy/update.ts (6)
9-25: Potential partial update consideration for targetsThis function removes all existing policy targets and reinserts fresh ones. This is valid if a complete replacement is intended. However, if partial updates or versioning of targets are desired, consider an upsert or more granular update approach.
27-57: Review deny window replacement logicSimilar to targets, you fully clear deny windows before re-inserting. If you anticipate future partial modifications or auditing of existing windows, an incremental update might be beneficial.
88-104: Handle missing existing record in any-approval updatesWhen
versionAnyApprovalsis not null, you callupdate(...)on the table. If there's no existing row, the update will affect zero rows. Depending on your schema and logic, consider an insert-or-update pattern if you want to ensure a record is always present.
105-123: Incremental updates vs. bulk removalThis pattern deletes all user approvals and inserts new ones. If you need revision history or partial updates, explore a more granular approach. For a replace-all strategy, this code is fine.
125-143: Role approvals replaced in entiretyLikewise, the complete removal and re-insertion of role approvals is straightforward but lacks partial tracking. Ensure you confirm this is the intended design.
145-198: Return specialized errors or statusesWhen the policy is not found, an unhandled error is thrown. Consider throwing a custom exception or returning an error type that your calling code can distinguish from other failures. Otherwise, this transactional update logic is well-structured and clear.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx (1)
59-93: Accessible navigation with visual feedbackThe UI implementation provides clear visual feedback for the active tab and has proper spacing for readability. Consider adding ARIA attributes for better accessibility.
<Link href={tab.href} key={tab.id} className={cn( "flex w-full cursor-pointer justify-start gap-3 p-3 text-muted-foreground", pathname === tab.href ? "bg-purple-500/10 text-purple-300" : "hover:bg-purple-500/5 hover:text-purple-300", )} + aria-current={pathname === tab.href ? "page" : undefined} >apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (1)
66-73: Consider adding explicit error handling to the mutation chainWhile the current promise chain works for handling successful updates, consider adding a
.catch()handler to explicitly manage and display errors to the user.const onSubmit = form.handleSubmit((data) => updatePolicy .mutateAsync({ id: policyId, data }) .then((res) => form.reset(res)) .then(() => router.refresh()) + .catch((error) => { + console.error("Failed to update policy:", error); + // Optionally show an error toast/notification to the user + }), );apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx (2)
52-79: Consider dynamic environment and deployment listsThe hard-coded environment and deployment lists might limit flexibility as the application grows. Consider fetching these values dynamically from an API or configuration.
- const ENVIRONMENTS = ["production", "staging", "development"] as const; - const DEPLOYMENTS = ["web-app", "api-service", "worker"] as const; + // Fetch these values from API or configuration + const { data: environments = ["production", "staging", "development"] } = + api.environment.list.useQuery(); + const { data: deployments = ["web-app", "api-service", "worker"] } = + api.deployment.list.useQuery();
347-348: Consider showing a message for null deploymentSelectorWhen deploymentSelector is null, the component renders an empty fragment. Consider showing an informative message like the one for environmentSelector.
- <></> + <FormItem> + <FormLabel>Deployment</FormLabel> + <div className="text-sm text-muted-foreground"> + Applies to all deployments in the selected environments. + </div> + </FormItem>apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (3)
69-74: Avoid repeated lookups in the deployment array.
Currently, the code invokesdeployments.find(...)multiple times when rendering the selected deployment. Storing the found item in a variable would reduce repetition and improve clarity.- {selectedId - ? deployments.find((d) => d.id === selectedId) - ? `${deployments.find((d) => d.id === selectedId)?.system.name} / ${deployments.find((d) => d.id === selectedId)?.name}` - : "Select deployment..." - : "Select deployment..."} + const selectedDeployment = deployments.find((d) => d.id === selectedId); + {selectedDeployment + ? `${selectedDeployment.system.name} / ${selectedDeployment.name}` + : "Select deployment..."}
159-159: Clarify usage ofLabelimported from Recharts.
It may cause confusion if your codebase uses multiple label components from different libraries. Consider renaming it or confirming that you want Recharts'Labelin this context.
245-245: Consider removing or gating debug logs.
console.log(errors)can clutter the console in production. Consider removing it or wrapping it with a development check.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (1)
423-437: Consider enabling role approval functionality or remove commented code.
The block of code that enables role approvals is currently commented out. If this functionality is planned, uncomment and finish the implementation; otherwise, remove the dead code to maintain clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/PolicyTable.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/approval-gates/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/page.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/DeploymentFlow.tsx(1 hunks)apps/webservice/src/app/urls.ts(1 hunks)packages/api/src/router/policy/create.ts(4 hunks)packages/api/src/router/policy/router.ts(3 hunks)packages/api/src/router/policy/time-util.ts(1 hunks)packages/api/src/router/policy/update.ts(1 hunks)packages/db/src/schema/policy.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/DeploymentFlow.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsxpackages/api/src/router/policy/update.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsxapps/webservice/src/app/urls.tspackages/db/src/schema/policy.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/PolicyTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsxpackages/api/src/router/policy/create.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsxpackages/api/src/router/policy/time-util.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/approval-gates/page.tsxapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsxpackages/api/src/router/policy/router.tsapps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx
🧠 Learnings (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#470
File: apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/TimeWindows.tsx:149-150
Timestamp: 2025-04-10T23:00:12.090Z
Learning: In the policy creation form's TimeWindows component, the start time is intentionally stored at `denyWindows.${index}.rrule.dtstart` while the end time is at `denyWindows.${index}.dtend` (outside the rrule object) as this aligns with the RRule library's structure for handling recurring time windows.
🧬 Code Graph Analysis (18)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/DeploymentFlow.tsx (1)
packages/db/src/schema/workspace.ts (1)
workspace(18-27)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/page.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (1)
EditTimeWindow(47-264)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsx (2)
packages/db/src/schema/policy.ts (1)
policy(28-44)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (1)
EditDeploymentFlow(185-308)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsx (2)
packages/db/src/schema/policy.ts (1)
policy(28-44)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx (1)
EditConfiguration(81-454)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx (2)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/PolicyContext.tsx (1)
PolicyTab(16-20)apps/webservice/src/app/urls.ts (1)
urls(223-223)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx (3)
packages/db/src/schema/policy.ts (1)
policy(28-44)apps/webservice/src/app/urls.ts (1)
urls(223-223)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx (1)
PolicyEditTabs(19-95)
packages/api/src/router/policy/update.ts (4)
packages/db/src/common.ts (3)
Tx(22-22)buildConflictUpdateColumns(30-46)takeFirst(9-13)packages/db/src/schema/policy.ts (2)
UpdatePolicy(122-122)policy(28-44)packages/api/src/router/policy/time-util.ts (1)
getLocalDateAsUTC(3-15)packages/validators/src/releases/conditions/release-condition.ts (1)
DeploymentVersionCondition(15-20)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (3)
packages/validators/src/releases/conditions/release-condition.ts (1)
deploymentVersionCondition(22-28)packages/db/src/schema/policy.ts (4)
Policy(131-131)PolicyDeploymentVersionSelector(133-135)policy(28-44)updatePolicy(100-121)apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/version/condition/DeploymentVersionConditionRender.tsx (1)
DeploymentVersionConditionRender(21-69)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsx (3)
packages/db/src/schema/workspace.ts (1)
workspace(18-27)packages/db/src/schema/policy.ts (1)
policy(28-44)apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (1)
EditQualitySecurity(271-452)
packages/db/src/schema/policy.ts (5)
packages/db/src/schema/rules/deny-window.ts (1)
createPolicyRuleDenyWindow(130-130)packages/db/src/schema/rules/deployment-selector.ts (1)
createPolicyDeploymentVersionSelector(29-32)packages/db/src/schema/rules/approval-any.ts (1)
createPolicyRuleAnyApproval(52-52)packages/db/src/schema/rules/approval-user.ts (1)
createPolicyRuleUserApproval(57-57)packages/db/src/schema/rules/approval-role.ts (1)
createPolicyRuleRoleApproval(63-63)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/page.tsx (2)
packages/db/src/schema/policy.ts (1)
policy(28-44)apps/webservice/src/app/urls.ts (1)
urls(223-223)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsx (1)
apps/webservice/src/app/urls.ts (1)
urls(223-223)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (2)
packages/db/src/schema/rules/deny-window.ts (1)
createPolicyRuleDenyWindow(130-130)packages/db/src/schema/policy.ts (2)
policy(28-44)updatePolicy(100-121)
packages/api/src/router/policy/time-util.ts (1)
packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
getDatePartsInTimeZone(20-43)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/approval-gates/page.tsx (2)
apps/webservice/src/app/urls.ts (1)
urls(223-223)packages/db/src/schema/policy.ts (1)
policy(28-44)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (6)
packages/ui/src/popover.tsx (3)
Popover(35-35)PopoverTrigger(35-35)PopoverContent(35-35)packages/ui/src/button.tsx (1)
Button(80-80)packages/db/src/schema/rules/approval-any.ts (1)
PolicyRuleAnyApproval(77-79)packages/db/src/schema/rules/approval-user.ts (1)
PolicyRuleUserApproval(82-84)packages/db/src/schema/rules/approval-role.ts (1)
PolicyRuleRoleApproval(88-90)packages/db/src/schema/policy.ts (2)
Policy(131-131)policy(28-44)
packages/api/src/router/policy/router.ts (2)
packages/db/src/schema/policy.ts (1)
policy(28-44)packages/api/src/router/policy/update.ts (1)
updatePolicyInTx(145-197)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx (3)
packages/db/src/schema/policy.ts (2)
Policy(131-131)policy(28-44)apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment/condition/EnvironmentConditionRender.tsx (1)
EnvironmentConditionRender(18-72)apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployments/condition/DeploymentConditionRender.tsx (1)
DeploymentConditionRender(18-72)
🔇 Additional comments (41)
apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/approval-gates/page.tsx (1)
123-127: URL construction updated for policy editing.The change adds a
.baseUrl()call to the URL construction chain, which aligns with the new URL structure for the policy editing interface. This is a good improvement that maintains consistency with the updated navigation system.apps/webservice/src/app/[workspaceSlug]/(app)/policies/create/_components/DeploymentFlow.tsx (1)
181-181: Code simplification for query options.The query options for
api.deployment.byWorkspaceId.useQueryhave been condensed to a single line, improving readability while maintaining the same functionality.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/page.tsx (1)
1-8: LGTM! Clean implementation of the TimeWindows page component.This component follows Next.js conventions for route parameters and async components, correctly extracting the policyId from params and passing it to the EditTimeWindow component.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/page.tsx (1)
1-13: LGTM! Well-implemented page component with proper error handling.The component correctly:
- Extracts the policyId from route parameters
- Fetches the policy data
- Handles the not-found case appropriately
- Renders the EditDeploymentFlow component with the policy data
This follows best practices for Next.js page components with server-side data fetching.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/page.tsx (1)
6-8:✅ Verification successful
Verify Next.js route parameter type
The
paramsproperty is typed as a Promise, which is unusual for Next.js route parameters - they are typically directly available without being wrapped in a Promise. Verify if this is the correct type or if it should be{ params: { policyId: string } }instead.
🏁 Script executed:
#!/bin/bash # Check other Next.js page components to verify the pattern echo "Checking other Next.js pages for params pattern..." fd --type file --extension tsx "page.tsx" --exec grep -l "params:" {} \; | head -n 5 | xargs catLength of output: 15785
The async route parameter typing in this file is correct. After verifying several other Next.js pages in the codebase (e.g., JoinPage, WorkspaceSettingsPage, GoogleIntegrationPage, etc.), it's clear that wrapping
paramsin a Promise is an intentional design choice to support asynchronous server component behavior. No changes are required.apps/webservice/src/app/urls.ts (1)
46-48: LGTM! The edit function is correctly updated to use the new workspacePolicyEdit function.The modification correctly delegates URL construction to the dedicated helper function, improving code organization.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/page.tsx (1)
6-16: Consider avoiding Promise-based params and confirm usageYou're passing a Promise for
paramsand then awaiting it in line 9. While it works, this is slightly unconventional for Next.js route parameters. Typically,paramswould be a direct object, not a Promise. Ensure there's a deliberate reason for this approach or refactor for clarity.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/page.tsx (1)
15-19: Confirm user authorization before redirectingThe code immediately redirects to the configuration page if the policy is found. Verify that all necessary authorization checks are performed beforehand to ensure only eligible users can edit the policy.
packages/api/src/router/policy/update.ts (1)
59-86: Check for conflicting insertsUsing
onConflictDoUpdateeffectively handles collisions for thedeploymentVersionSelector. Confirm that side effects, such as logs or concurrency handling in other areas, are consistent with the replaced data. Otherwise, this logic looks concise.packages/api/src/router/policy/create.ts (6)
8-8: Improved time handling with utility functionThe code now uses the
getLocalDateAsUTCutility function from a dedicated time-util module, which improves code organization and reusability.
10-10: Type declaration moved for better readabilityThe CreatePolicyInput type declaration has been relocated while maintaining its functionality.
32-32: LGTM: Proper object spreading in rrule handlingThe code correctly spreads the rrule object and adds the dtstart value, maintaining the object's structure while updating specific properties.
59-61: Code formatting improved for clarityThe policy target insertion has been condensed from multiline to single-line format, improving readability while maintaining the same functionality.
79-83: Improved code structure for user approval insertionThe insertion operation for policy rule user approval has been refactored to a more concise and readable format.
86-90: Consistent formatting applied to role approval insertionThe role approval insertion formatting matches the pattern used elsewhere in the file, maintaining code consistency.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/_components/PolicyEditTabs.tsx (4)
1-11: Well-structured component with proper importsThe component follows best practices with client-side declaration at the top and organized imports. Good use of Next.js features for navigation.
12-18: Clean TypeScript interface definitionThe TabConfig interface is well-defined with appropriate types for each property.
19-31: Effective URL parameter handlingThe component correctly extracts workspaceSlug and policyId using useParams, and builds policy edit URLs using the url utility.
32-57: Well-organized tab configurationThe tabs array is clearly structured with descriptive labels and IDs that match the PolicyTab type. Each tab has a clear purpose described in its description property.
packages/db/src/schema/policy.ts (1)
100-121: Comprehensive schema extension for flexible policy updatesThe updatePolicy schema has been extended to include multiple optional fields, allowing for more granular policy updates. The structure is consistent with the createPolicy schema, making the API more intuitive to use.
The extension properly handles complicated nested structures and follows the pattern of omitting policyId from the input, as it should be provided separately in the update function.
packages/api/src/router/policy/router.ts (5)
22-22: Added import for transaction-based policy updateThe new import of updatePolicyInTx enables a transaction-based approach to updating policies, which improves data consistency.
179-182: Updated authorization check to use new input structureThe authorization check now correctly references input.policyId instead of input directly, aligning with the updated input structure.
183-189: Enhanced input schema with time zone supportThe byId procedure now accepts a structured input with policyId and timeZone properties, making the API more flexible. The timeZone defaults to "UTC" which is a good practice for backward compatibility.
191-199: Comprehensive policy data retrievalThe query now includes related entities like targets, denyWindows, and various approval configurations, providing a complete view of the policy for the edit UI.
223-225: Transaction-based policy update implementationThe update mutation now uses a transaction to ensure data consistency across multiple tables. This is particularly important for complex updates involving related entities.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/layout.tsx (1)
19-64: Well-structured layout implementation with proper error handlingThe layout component effectively creates a consistent editing interface with breadcrumb navigation, policy edit tabs, and a scrollable content area. The async/await pattern is correctly implemented for fetching policy data, with appropriate error handling using
notFound().apps/webservice/src/app/[workspaceSlug]/(app)/policies/(sidebar)/_components/PolicyTable.tsx (4)
4-5: Correctly added Next.js navigation importsThe addition of Link and useParams imports sets up the foundation for client-side navigation.
27-27: Proper import of urls utilityThe urls import is correctly added to enable route generation.
54-57: Well-implemented URL construction functionThe editUrl function correctly uses the workspaceSlug from params and the provided policyId to construct edit URLs.
171-178: Successfully implemented client-side navigation for Edit actionThe Link component is properly integrated with the dropdown menu item, enabling seamless navigation to policy edit pages while maintaining styling.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/time-windows/EditTimeWindow.tsx (6)
26-28: Well-defined schema using zodThe schema correctly omits the policyId field from the denyWindows array items as expected.
47-64: Proper component setup with data fetching and form initializationThe component correctly initializes the form with policy data and updates it when the data changes.
108-137: Well-implemented day selection with toggle functionalityThe day selection buttons are well implemented with proper state management for toggling days on and off.
139-175: Time input handling matches expected RRule structureThe start time is correctly stored at
denyWindows.${index}.rrule.dtstartwhich aligns with the RRule library's structure as noted in the retrieved learning.
178-214: End time is correctly handled outside the RRule objectFollowing the learned pattern, the end time is properly stored at
denyWindows.${index}.dtendoutside the rrule object.
230-249: Well-implemented functionality for adding new time windowsThe append functionality correctly initializes new time windows with the expected structure, including empty byweekday array, WEEKLY frequency, and appropriate timezone.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/configuration/EditConfiguration.tsx (3)
39-50: Well-structured schema definition using zodThe schema correctly defines the structure for policy configuration including targets with deployment and environment selectors.
81-109: Solid form setup with proper mutation handlingThe component correctly initializes the form with policy data and handles the update mutation with proper promise chaining.
445-450: Good submit button implementation with proper disablingThe submit button is correctly disabled when the form is not dirty or when an update is pending.
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/deployment-flow/EditDeploymentFlow.tsx (1)
260-281: Verify null-setting logic for the version selector.
When a user removes all conditions (!newValue.conditions.length), the form setsdeploymentVersionSelectorto null. Verify that any dependent UI or logic (e.g., saving partial changes) still functions as intended when the version selector is fully removed.apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/edit/quality-security/EditQualitySecurity.tsx (1)
294-306: Review default values for newly appended user or role approvals.
When callingappendUseror a futureappendRole, ensure that the default fields align with the schema requirements and any upcoming changes in the associated API (e.g., ensuringid,createdAt,policyIdare appropriately set).
| export const getLocalDateAsUTC = (date: Date, timeZone: string) => { | ||
| const parts = getDatePartsInTimeZone(date, timeZone); | ||
| return new Date( | ||
| Date.UTC( | ||
| parts.year, | ||
| parts.month, | ||
| parts.day, | ||
| parts.hour, | ||
| parts.minute, | ||
| parts.second, | ||
| ), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix potential month indexing issue in getLocalDateAsUTC
The function correctly converts a local date to UTC, but there's a potential issue with month indexing. JavaScript's Date.UTC() expects month values from 0-11, but the getDatePartsInTimeZone function (as seen in the provided snippet) returns month values from 1-12. This could lead to an off-by-one error.
Apply this fix:
export const getLocalDateAsUTC = (date: Date, timeZone: string) => {
const parts = getDatePartsInTimeZone(date, timeZone);
return new Date(
Date.UTC(
parts.year,
- parts.month,
+ parts.month - 1, // Adjust for 0-based month indexing in Date.UTC
parts.day,
parts.hour,
parts.minute,
parts.second,
),
);
};📝 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.
| export const getLocalDateAsUTC = (date: Date, timeZone: string) => { | |
| const parts = getDatePartsInTimeZone(date, timeZone); | |
| return new Date( | |
| Date.UTC( | |
| parts.year, | |
| parts.month, | |
| parts.day, | |
| parts.hour, | |
| parts.minute, | |
| parts.second, | |
| ), | |
| ); | |
| }; | |
| export const getLocalDateAsUTC = (date: Date, timeZone: string) => { | |
| const parts = getDatePartsInTimeZone(date, timeZone); | |
| return new Date( | |
| Date.UTC( | |
| parts.year, | |
| parts.month - 1, // Adjust for 0-based month indexing in Date.UTC | |
| parts.day, | |
| parts.hour, | |
| parts.minute, | |
| parts.second, | |
| ), | |
| ); | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see what you are seeing
const formatter = new Intl.DateTimeFormat("en-US", {
year: "numeric",
month: "2-digit",
day: "2-digit",
hour: "2-digit",
minute: "2-digit",
second: "2-digit",
hour12: false,
timeZone,
});
const parts = formatter.formatToParts(date);
const get = (type: string) =>
parts.find((p) => p.type === type)?.value ?? "0";
return {
year: parseInt(get("year"), 10),
month: parseInt(get("month"), 10),
day: parseInt(get("day"), 10),
hour: parseInt(get("hour"), 10),
minute: parseInt(get("minute"), 10),
second: parseInt(get("second"), 10),
};
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adityachoudhari26 Thanks for sharing the implementation of getDatePartsInTimeZone. Looking at the code you provided, I can confirm my original assessment is correct.
The Intl.DateTimeFormat with "month" in "2-digit" format generates values like "01" through "12" (for January through December). After parsing with parseInt(), this produces numbers 1-12.
However, JavaScript's Date.UTC() method expects months in 0-11 format (where 0 = January, 1 = February, etc.). Without adjusting the month value by subtracting 1, the date would be set one month ahead of the intended date.
For example, if trying to convert January 15, 2023, the current code would actually create February 15, 2023 in UTC.
The adjustment is still needed:
parts.month - 1 // Adjust for 0-based month indexing in Date.UTC| const policy = await api.policy.byId({ policyId }); | ||
| if (policy == null) return notFound(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for API call failures
The code only handles the case when policy is null, but not when the API call throws an exception. Consider adding try/catch blocks to handle potential API errors gracefully.
export default async function ConfigurationPage(props: {
params: Promise<{ policyId: string }>;
}) {
const { policyId } = await props.params;
+ try {
const policy = await api.policy.byId({ policyId });
if (policy == null) return notFound();
return <EditConfiguration policy={policy} />;
+ } catch (error) {
+ console.error("Failed to fetch policy:", error);
+ // Consider using a custom error component or redirecting to an error page
+ throw new Error("Failed to load policy configuration");
+ }
}📝 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 policy = await api.policy.byId({ policyId }); | |
| if (policy == null) return notFound(); | |
| export default async function ConfigurationPage(props: { | |
| params: Promise<{ policyId: string }>; | |
| }) { | |
| const { policyId } = await props.params; | |
| try { | |
| const policy = await api.policy.byId({ policyId }); | |
| if (policy == null) return notFound(); | |
| return <EditConfiguration policy={policy} />; | |
| } catch (error) { | |
| console.error("Failed to fetch policy:", error); | |
| // Consider using a custom error component or redirecting to an error page | |
| throw new Error("Failed to load policy configuration"); | |
| } | |
| } |
| export default async function PolicyPage(props: { | ||
| params: Promise<{ workspaceSlug: string; policyId: string }>; | ||
| }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify Next.js route parameter type
Similar to the configuration page, the params property is typed as a Promise, which is unusual for Next.js route parameters. Verify if this is the correct type for Next.js page components.
🌐 Web query:
What is the correct type for route parameters in Next.js App Router?
💡 Result:
In Next.js App Router, the correct type for route parameters depends on whether you're working with server components or client components:
Server Components
For server components (pages, layouts), the params prop is typed based on your dynamic route segments:
// app/blog/[slug]/page.tsx
export default function Page({
params,
}: {
params: { slug: string } // Explicit type for dynamic segment
}) {
return <h1>{params.slug}</h1>
}For catch-all routes:
// app/shop/[...slug]/page.tsx
export default function Page({
params,
}: {
params: { slug: string[] } // Array type for catch-all
}) {
// ...
}Client Components
Use the useParams hook with generics:
'use client'
import { useParams } from 'next/navigation'
export default function ClientComponent() {
const params = useParams<{
category: string
item: string
}>()
return <div>{params.category} - {params.item}</div>
}Type Validation Patterns
For advanced type safety:
- Zod Validation (recommended):
import { z } from 'zod'
const ParamsSchema = z.object({
id: z.string().uuid()
})
export default function Page({ params }: { params: unknown }) {
const parsed = ParamsSchema.safeParse(params)
if (!parsed.success) return <div>Invalid params</div>
return <div>Valid ID: {parsed.data.id}</div>
}- Type Assertion (basic validation):
interface Params {
id: string
variant: 'a' | 'b' | 'c'
}
export default function Page({ params }: { params: Params }) {
// ...
}Special Cases
- Optional catch-all:
params: { slug?: string[] } - Multiple segments:
params: { category: string product: string }
- Search Params (separate from route params):
export default function Page({ searchParams, }: { searchParams: { [key: string]: string | string[] | undefined } }) { // ... }
The App Router automatically infers types from your route structure, but explicit typing is recommended for TypeScript safety[9][4]. For complex applications, consider using Zod for runtime validation alongside TypeScript types[1][9].
Citations:
- 1: https://www.flightcontrol.dev/blog/fix-nextjs-routing-to-have-full-type-safety
- 2: What is the Typescript type for the parameters in a server component? vercel/next.js#46131
- 3: https://nextjs.org/docs/app/building-your-application/routing/route-handlers
- 4: https://nextjs.org/docs/app/api-reference/functions/use-params
- 5: https://nextjs.org/docs/pages/building-your-application/routing/dynamic-routes
- 6: How to get dynamic route params from Server Components? vercel/next.js#51818
- 7: https://www.youtube.com/watch?v=vAfq5l1tYNY
- 8: https://nextjs.org/docs/app/building-your-application/routing
- 9: https://nextjs.org/docs/app/building-your-application/routing/dynamic-routes
- 10: https://nextjs.org/docs/app
Correct the Next.js Route Parameter Type
It appears that the params prop in your PolicyPage component is currently typed as a Promise, whereas Next.js App Router expects route parameters to be a plain object. Please update the code accordingly:
- File:
apps/webservice/src/app/[workspaceSlug]/(app)/policies/[policyId]/page.tsx(Lines 5-7) - Action: Remove the
Promise<>wrapper and declareparamsas a plain object.
For example, update your snippet from:
export default async function PolicyPage(props: {
params: Promise<{ workspaceSlug: string; policyId: string }>;
}) {to:
export default async function PolicyPage({ params }: { params: { workspaceSlug: string; policyId: string } }) {This change aligns with Next.js conventions for server components as per the latest documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
@adityachoudhari26 You're right, and I apologize for the incorrect suggestion. Let me check how route parameters are handled in this codebase to understand the correct pattern.
🏁 Script executed:
#!/bin/bash
# Look for other Next.js page components to see how they handle params
rg "function.*Page.*\(props: \{.*params: Promise<" --type ts -A 3 --context 3
Length of output: 79
Let me search for other Next.js page components in the codebase to understand how they handle route parameters.
🏁 Script executed:
#!/bin/bash
# Look for other Next.js page components that use params in this codebase
rg "export default (async )?function \w+Page\(props:" --type ts -A 5
Length of output: 50177
@adityachoudhari26 You're absolutely right, and I apologize for the incorrect suggestion. After searching through the codebase, I can see that your implementation follows the consistent pattern used throughout the project.
All page components in this codebase use params as a Promise that needs to be awaited:
params: Promise
<{ workspaceSlug: string; policyId: string }>;This appears to be a deliberate architectural pattern in your project that differs from the standard Next.js convention. Thank you for the correction.
🧬 Code Graph Analysis Results
Relevant Code Snippets
Filename: apps/webservice/src/app/urls.ts
- Line 224
export const urls = { workspace };- Description: This line exports an object
urlsthat contains a reference toworkspace. Theworkspaceobject likely includes methods for constructing URLs related to workspaces, which may be relevant for understanding how thePolicyPagefunction constructs the redirect URL.
| <div className="max-w-xl rounded-lg border p-4"> | ||
| <h4 className="mb-4 font-medium">Target Coverage Preview</h4> | ||
| <div className="relative overflow-hidden"> | ||
| <table className="w-full border-collapse text-sm"> | ||
| <thead> | ||
| <tr> | ||
| <th className="border-b p-2 text-left font-medium"> | ||
| Environment | ||
| </th> | ||
| {DEPLOYMENTS.map((dep) => ( | ||
| <th | ||
| key={dep} | ||
| className="border-b p-2 text-left font-medium" | ||
| > | ||
| {dep | ||
| .split("-") | ||
| .map( | ||
| (word) => | ||
| word.charAt(0).toUpperCase() + word.slice(1), | ||
| ) | ||
| .join(" ")} | ||
| </th> | ||
| ))} | ||
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {ENVIRONMENTS.map((env) => ( | ||
| <tr key={env}> | ||
| <td className="border-b p-2 font-medium"> | ||
| {env.charAt(0).toUpperCase() + env.slice(1)} | ||
| </td> | ||
| {DEPLOYMENTS.map((dep) => ( | ||
| <td key={`${env}-${dep}`} className="border-b p-2"> | ||
| {/* {getCoverage(env, dep) ? ( | ||
| <Badge | ||
| variant="secondary" | ||
| className="bg-purple-500/10 text-purple-300" | ||
| > | ||
| Included | ||
| </Badge> | ||
| ) : ( | ||
| <Badge | ||
| variant="outline" | ||
| className="text-muted-foreground" | ||
| > | ||
| Not Included | ||
| </Badge> | ||
| )} */} | ||
| </td> | ||
| ))} | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Complete the target coverage preview functionality
The target coverage preview table has commented-out logic that needs to be implemented to provide value to users.
Consider implementing the getCoverage function and uncommenting the badge rendering logic to make this table functional:
+ // Determine if a target covers the given environment and deployment
+ const getCoverage = (env: string, dep: string) => {
+ return form.getValues().targets.some(target => {
+ // Logic to check if the target's selectors match env and dep
+ // This is a simplified example - actual implementation would need to evaluate conditions
+ const envMatch = !target.environmentSelector || /* evaluate env conditions */;
+ const depMatch = !target.deploymentSelector || /* evaluate dep conditions */;
+ return envMatch && depMatch;
+ });
+ };
// Then uncomment the badge rendering code at lines 420-434📝 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.
| <div className="max-w-xl rounded-lg border p-4"> | |
| <h4 className="mb-4 font-medium">Target Coverage Preview</h4> | |
| <div className="relative overflow-hidden"> | |
| <table className="w-full border-collapse text-sm"> | |
| <thead> | |
| <tr> | |
| <th className="border-b p-2 text-left font-medium"> | |
| Environment | |
| </th> | |
| {DEPLOYMENTS.map((dep) => ( | |
| <th | |
| key={dep} | |
| className="border-b p-2 text-left font-medium" | |
| > | |
| {dep | |
| .split("-") | |
| .map( | |
| (word) => | |
| word.charAt(0).toUpperCase() + word.slice(1), | |
| ) | |
| .join(" ")} | |
| </th> | |
| ))} | |
| </tr> | |
| </thead> | |
| <tbody> | |
| {ENVIRONMENTS.map((env) => ( | |
| <tr key={env}> | |
| <td className="border-b p-2 font-medium"> | |
| {env.charAt(0).toUpperCase() + env.slice(1)} | |
| </td> | |
| {DEPLOYMENTS.map((dep) => ( | |
| <td key={`${env}-${dep}`} className="border-b p-2"> | |
| {/* {getCoverage(env, dep) ? ( | |
| <Badge | |
| variant="secondary" | |
| className="bg-purple-500/10 text-purple-300" | |
| > | |
| Included | |
| </Badge> | |
| ) : ( | |
| <Badge | |
| variant="outline" | |
| className="text-muted-foreground" | |
| > | |
| Not Included | |
| </Badge> | |
| )} */} | |
| </td> | |
| ))} | |
| </tr> | |
| ))} | |
| </tbody> | |
| </table> | |
| </div> | |
| </div> | |
| // Determine if a target covers the given environment and deployment | |
| const getCoverage = (env: string, dep: string) => { | |
| return form.getValues().targets.some(target => { | |
| // Logic to check if the target's selectors match env and dep | |
| // This is a simplified example - actual implementation would need to evaluate conditions | |
| const envMatch = !target.environmentSelector || /* evaluate env conditions */; | |
| const depMatch = !target.deploymentSelector || /* evaluate dep conditions */; | |
| return envMatch && depMatch; | |
| }); | |
| }; | |
| <div className="max-w-xl rounded-lg border p-4"> | |
| <h4 className="mb-4 font-medium">Target Coverage Preview</h4> | |
| <div className="relative overflow-hidden"> | |
| <table className="w-full border-collapse text-sm"> | |
| <thead> | |
| <tr> | |
| <th className="border-b p-2 text-left font-medium"> | |
| Environment | |
| </th> | |
| {DEPLOYMENTS.map((dep) => ( | |
| <th | |
| key={dep} | |
| className="border-b p-2 text-left font-medium" | |
| > | |
| {dep | |
| .split("-") | |
| .map( | |
| (word) => | |
| word.charAt(0).toUpperCase() + word.slice(1) | |
| ) | |
| .join(" ")} | |
| </th> | |
| ))} | |
| </tr> | |
| </thead> | |
| <tbody> | |
| {ENVIRONMENTS.map((env) => ( | |
| <tr key={env}> | |
| <td className="border-b p-2 font-medium"> | |
| {env.charAt(0).toUpperCase() + env.slice(1)} | |
| </td> | |
| {DEPLOYMENTS.map((dep) => ( | |
| <td key={`${env}-${dep}`} className="border-b p-2"> | |
| {getCoverage(env, dep) ? ( | |
| <Badge | |
| variant="secondary" | |
| className="bg-purple-500/10 text-purple-300" | |
| > | |
| Included | |
| </Badge> | |
| ) : ( | |
| <Badge | |
| variant="outline" | |
| className="text-muted-foreground" | |
| > | |
| Not Included | |
| </Badge> | |
| )} | |
| </td> | |
| ))} | |
| </tr> | |
| ))} | |
| </tbody> | |
| </table> | |
| </div> | |
| </div> |
| const editQualitySecuritySchema = z.object({ | ||
| versionAnyApprovals: z | ||
| .object({ | ||
| id: z.string(), | ||
| createdAt: z.date(), | ||
| requiredApprovalsCount: z.number(), | ||
| policyId: z.string(), | ||
| }) | ||
| .nullable(), | ||
| versionUserApprovals: z.array( | ||
| z.object({ | ||
| id: z.string(), | ||
| createdAt: z.date(), | ||
| policyId: z.string(), | ||
| userId: z.string(), | ||
| }), | ||
| ), | ||
| versionRoleApprovals: z.array( | ||
| z.object({ | ||
| id: z.string(), | ||
| createdAt: z.date(), | ||
| policyId: z.string(), | ||
| roleId: z.string(), | ||
| requiredApprovalsCount: z.number(), | ||
| }), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure versionAnyApprovals fields match the schema requirements.
When enabling or disabling general approvals via the switch, only requiredApprovalsCount is set. The schema, however, requires additional non-optional fields (id, createdAt, policyId). Consider providing default values for these fields or making them optional in the schema to avoid validation errors.
- onCheckedChange={(checked) =>
- field.onChange(
- checked
- ? { requiredApprovalsCount: 1 }
- : null,
- )
- }
+ onCheckedChange={(checked) => {
+ if (checked) {
+ field.onChange({
+ id: "", // Provide defaults or handle properly
+ createdAt: new Date(),
+ policyId: policy.id,
+ requiredApprovalsCount: 1,
+ });
+ } else {
+ field.onChange(null);
+ }
+ }}📝 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 editQualitySecuritySchema = z.object({ | |
| versionAnyApprovals: z | |
| .object({ | |
| id: z.string(), | |
| createdAt: z.date(), | |
| requiredApprovalsCount: z.number(), | |
| policyId: z.string(), | |
| }) | |
| .nullable(), | |
| versionUserApprovals: z.array( | |
| z.object({ | |
| id: z.string(), | |
| createdAt: z.date(), | |
| policyId: z.string(), | |
| userId: z.string(), | |
| }), | |
| ), | |
| versionRoleApprovals: z.array( | |
| z.object({ | |
| id: z.string(), | |
| createdAt: z.date(), | |
| policyId: z.string(), | |
| roleId: z.string(), | |
| requiredApprovalsCount: z.number(), | |
| }), | |
| ), | |
| }); | |
| onCheckedChange={(checked) => { | |
| if (checked) { | |
| field.onChange({ | |
| id: "", // Provide defaults or handle properly | |
| createdAt: new Date(), | |
| policyId: policy.id, | |
| requiredApprovalsCount: 1, | |
| }); | |
| } else { | |
| field.onChange(null); | |
| } | |
| }} |
Summary by CodeRabbit
New Features
Refactor