fix: readd dropdown menu to cells and add more options #545
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis set of changes refactors and modularizes the deployment approval and policy evaluation UI components in the webservice application. Several previously inlined or local components (such as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant ApprovalDialog
participant API
User->>UI: Click "Approve" or "Force deploy"
UI->>ApprovalDialog: Open dialog with version/environment info
User->>ApprovalDialog: Confirm approval/rejection (optional reason)
ApprovalDialog->>API: Submit approval record (status, reason)
API-->>ApprovalDialog: Respond with success
ApprovalDialog->>UI: Close dialog, refresh state
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/policy-evaluation.ts (1)
1-9: Consider adding documentation comments to the type definitionThe
PolicyEvaluationResulttype is well-structured, but adding JSDoc comments would improve maintainability by explaining what each property represents, especially for the various approval record types.+/** + * Represents the result of a policy evaluation process + */ export type PolicyEvaluationResult = { + /** The policies being evaluated */ policies: { id: string; name: string }[]; rules: { + /** Maps policy IDs to arrays of required approvals */ anyApprovals: Record<string, string[]>; + /** Maps policy IDs to arrays of required role approvals */ roleApprovals: Record<string, string[]>; + /** Maps policy IDs to arrays of required user approvals */ userApprovals: Record<string, string[]>; + /** Maps policy IDs to boolean flags indicating if they're blocked by version selectors */ versionSelector: Record<string, boolean>; }; };apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/BlockedByVersionSelectorCell.tsx (1)
29-37: Minor: givegetPoliciesBlockingByVersionSelectoran explicit return typeAdding an explicit type (e.g.
Array<{ id: string; name: string }>orPolicy[]) makes the helper self-documenting and guards against accidental API changes.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ApprovalRequiredCell.tsx (1)
32-70: Helper could be simplified & de-duplicated
getPoliciesWithApprovalRequiredperforms three almost identical passes over the evaluation object and thenuniqBy.
Consider extracting the repeated logic or iterating once over the rules object to improve readability and reduce work.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx (1)
34-47: Reason field never resets after dialog closesIf the user opens the dialog, types a reason, approves/rejects, and later re-opens the dialog, the previous reason remains.
Resetting the state on close prevents accidental reuse of stale input.- .then(() => setOpen(false)) + .then(() => { + setOpen(false); + setReason(""); + })Alternatively, add a
useEffectthat clearsreasonwheneveropenbecomesfalse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/ApprovalCheck.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/FlowNode.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/FlowPolicyNode.tsx(0 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/Approval.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/ResourceReleaseTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentPageContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ActiveJobsCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ApprovalRequiredCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/BlockedByVersionSelectorCell.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx(6 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/policy-evaluation.ts(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(sidebar)/systems/_components/system-deployment-table/TableDeployments.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/DeploymentVersionDropdownMenu.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx(3 hunks)
💤 Files with no reviewable changes (3)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/FlowNode.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/ApprovalCheck.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/FlowPolicyNode.tsx
🧰 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)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/ResourceReleaseTable.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(sidebar)/systems/_components/system-deployment-table/TableDeployments.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentPageContent.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/Approval.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/policy-evaluation.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ApprovalRequiredCell.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/ApprovalDialog.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ActiveJobsCell.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/DeploymentVersionDropdownMenu.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/BlockedByVersionSelectorCell.tsxapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx
🧬 Code Graph Analysis (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/ResourceReleaseTable.tsx (1)
packages/db/src/schema/environment.ts (1)
environment(59-84)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (25)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(sidebar)/systems/_components/system-deployment-table/TableDeployments.tsx (1)
214-214: Standardized system data passing.The change from passing
systemSlugto passing the entiresystemobject ensures consistency across deployment components and allows downstream components to access additional system properties if needed in the future.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/ResourceReleaseTable.tsx (1)
198-198: Added explicit environmentId for better approval context.Adding the
environmentIdprop to theEnvironmentApprovalRowcomponent explicitly passes the environment context to the approval flow, which helps in making environment-specific approval operations more clear and maintainable.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(sidebar)/deployments/TableDeployments.tsx (1)
233-233: Standardized system data using object structure.Converting the
systemSlugstring prop to asystemobject prop with aslugproperty aligns with the broader refactoring effort and creates consistency with other deployment-related components.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/DeploymentPageContent.tsx (1)
360-360: Standardized system data passing in environment cell.The change from passing a
systemSlugstring to passing asystemobject maintains consistency with other related components and supports the modularization of deployment environment cells.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/jobs/release-table/EnvironmentApprovalRow.tsx (3)
12-12: Props interface extended correctlyAdded the necessary
environmentIdstring property to theEnvironmentApprovalRowPropstype, which is required for the updatedApprovalDialogcomponent API.
18-18: Prop destructuring updated correctlyThe component destructuring now includes the new
environmentIdprop, which matches the updated props interface.
23-25: Improved API usage with explicit propsThe refactored
ApprovalDialogimplementation is now using explicit properties instead of passing the entiredeploymentVersionobject. This change:
- Makes the interface more explicit and easier to understand
- Provides better type safety
- Follows React best practices for prop drilling
This is a positive improvement to the codebase.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployments/environment-cell/DeploymentEnvironmentCell.tsx (3)
27-27: Improved prop structure with object instead of stringChanging from
systemSlug: stringtosystem: { slug: string }is a good improvement that:
- Makes the component more extensible for future additions
- Follows the pattern of structured data objects rather than primitive values
- Creates a more consistent API across components
This structural change aligns with React best practices.
33-33: Prop usage updated correctly throughout componentAll instances of
systemSlughave been correctly updated to usesystem.sluginstead, ensuring the component works as expected with the new API.Also applies to: 44-44
67-67: System object now properly passed to child componentThe
systemprop is now correctly passed to theLazyDeploymentVersionEnvironmentCellcomponent, which ensures proper propagation of the system data through the component hierarchy. This is important for maintaining context consistency.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/Approval.tsx (3)
9-9: Good modularization with shared component importAdding the import for the shared
ApprovalDialogcomponent is a good step toward code reuse and maintainability.
53-57: Improved code modularity by using shared componentReplacing the inline approval dialog implementation with the shared
ApprovalDialogcomponent:
- Reduces code duplication
- Centralizes the dialog logic for easier maintenance
- Ensures consistent behavior across the application
- Simplifies this component's implementation
The implementation correctly passes all required props and the
onSubmitcallback for cache invalidation.
78-82: Consistent usage of shared componentThe shared
ApprovalDialogcomponent is consistently used in all relevant sections of the component, ensuring uniform behavior throughout the UI.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/_components/deployment-version/DeploymentVersionDropdownMenu.tsx (5)
49-94: Export reusable component to improve code modularity.The
RedeployVersionDialogcomponent is now exported to facilitate reuse in other parts of the application. Additionally, thehandleRedeployfunction has been simplified while maintaining the same functionality.
96-140: Export ForceDeployVersionDialog component for reuse.Making this dialog component exportable improves code modularity and allows it to be reused in other components, which aligns with the overall refactoring approach seen in this PR.
150-168: Simplify DropdownAction component by removing disabled state.The component no longer supports disabled states (
disabledanddisabledMessageprops have been removed), which simplifies the implementation. Menu items will now always be enabled and trigger dialogs instead of showing disabled states with tooltips.This change alters the UX by always showing enabled actions - please verify this behavior is intentional and doesn't lead to user confusion in cases where actions would have previously been disabled with explanatory messages.
177-177: Remove conditional disabling from DeploymentVersionDropdownMenu.The
isVersionBeingDeployedprop has been removed from the component signature, which means the redeploy action will no longer be conditionally disabled. This aligns with the changes to theDropdownActioncomponent.
183-183: Add shrink-0 class to prevent button from collapsing.Adding the
shrink-0utility class prevents the dropdown trigger button from shrinking when space is limited, maintaining consistent button dimensions.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/DeploymentVersionEnvironmentCell.tsx (7)
7-7: Simplify imports to include only necessary icons.Keeping only the required icon imports helps maintain clean code and reduces bundle size.
14-22: Import modularized components for better code organization.The file now imports components and helper functions from separate modules instead of defining them inline. This improves code modularity, reusability, and maintainability.
51-56: Update NoJobAgentCell props to use object pattern.The component now accepts
systemanddeploymentobjects instead of individual properties, standardizing prop handling across components. TheworkspaceSlugis now retrieved usinguseParamsinside the component, making it more self-contained.
59-61: Simplify URL construction with object properties.URL construction now uses properties from the passed objects, which is more maintainable and less error-prone.
82-82: Update props type to include system object.The props type now includes a
systemobject, aligning with the standardization of passing objects rather than individual strings throughout the codebase.
90-91: Improve props handling with simplified pattern.The component now accepts a single
propsobject and destructures only what's needed at the top level, making the code more maintainable.
116-116: Standardize props passing with spread operator.Props are now passed wholesale to child components using the spread operator, reducing code duplication and standardizing prop handling across the components.
Also applies to: 134-135, 148-149, 154-154
...oyments/[deploymentSlug]/(sidebar)/_components/release-cell/BlockedByVersionSelectorCell.tsx
Show resolved
Hide resolved
...ug]/(raw)/deployments/[deploymentSlug]/(sidebar)/_components/release-cell/ActiveJobsCell.tsx
Show resolved
Hide resolved
| import { JobStatus } from "@ctrlplane/validators/jobs"; | ||
|
|
||
| import { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enum mismatch can cause false negatives when detecting active jobs
statuses is typed as SCHEMA.JobStatus[], yet the comparison is done against JobStatus imported from the validator package.
Although the underlying literal values may coincide today, nothing enforces that they will stay in sync.
-import { JobStatus } from "@ctrlplane/validators/jobs";
+// Re-use the enum that the array is already typed with.
+import { JobStatus as DbJobStatus } from "@ctrlplane/db/schema";and
-const hasActiveJobs = statuses.some((s) => s === JobStatus.InProgress);
+const hasActiveJobs = statuses.includes(DbJobStatus.InProgress);Aligning the source of truth prevents drift and removes an unnecessary extra import.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { JobStatus } from "@ctrlplane/validators/jobs"; | |
| import { | |
| -import { JobStatus } from "@ctrlplane/validators/jobs"; | |
| +// Re-use the enum that the array is already typed with. | |
| +import { JobStatus as DbJobStatus } from "@ctrlplane/db/schema"; | |
| import { | |
| /* … */ |
| import { JobStatus } from "@ctrlplane/validators/jobs"; | |
| import { | |
| -const hasActiveJobs = statuses.some((s) => s === JobStatus.InProgress); | |
| +const hasActiveJobs = statuses.includes(DbJobStatus.InProgress); |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores