fix: Show pending approval button with dialog in deployment releases page #293
Conversation
WalkthroughThe pull request introduces changes to the deployment and release approval workflow across multiple components. The modifications focus on fetching and handling approval status dynamically, removing static linked environments, and updating the rendering logic to conditionally display release information and approval dialogs. The changes streamline the approval process by relying on API queries to retrieve environment and policy details, enhancing the component's flexibility and data retrieval mechanism. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)Pattern ⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (2)
107-108: Consider using a status enum or union type.
approval?.status === "pending"is valid. For better type safety and maintainability, consider using enums or a discriminated union, rather than string literals.
174-186: Refine user feedback in approval dialog.The
ApprovalDialogis rendered correctly when approval is pending. Consider adding more explanatory text or a tooltip on the button to give users clarity about next steps.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (4)
26-28: Consider adding an error state for the policy query.
Currently, there's no UI handling forpolicyQ.isError. If the API call fails, users may see no information, leading to confusion.} = ({ release, policyId, children }) => { const policyQ = api.environment.policy.byId.useQuery(policyId); + if (policyQ.isError) { + return ( + <AlertDialogDescription> + Failed to load policy info. Please try again later. + </AlertDialogDescription> + ); + }
43-43: Consider using async/await for clarity.
Chaining multiple.then()calls is fine, but an async/await style might improve readability and reduce nesting.
49-49: Apply the same async/await suggestion here for consistency.
Consolidate with the onApprove flow for uniform error handling.
66-83: Include a fallback for empty environments.
WhenpolicyQ.data?.environmentsis empty or undefined, it might be helpful to show a short message.{!policyQ.isLoading && ( <AlertDialogDescription> <div className="flex flex-col gap-2"> Approves this release for the following environments: <div className="flex flex-wrap gap-2"> - {policyQ.data?.environments.map(...)} + {policyQ.data?.environments?.length ? ( + policyQ.data.environments.map(...) + ) : ( + <span>No associated environments found.</span> + )} </div> </div> </AlertDialogDescription> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx(5 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ResourceReleaseTable.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (1)
Pattern **/*.{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.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (5)
21-21: Validate import path.Confirm that
"./[deploymentSlug]/releases/[versionId]/ApprovalCheck"is the correct source forApprovalDialog.
52-57: Conditional fetch looks good.Using
environment.policyId ?? ""as a fallback and enabling the query conditionally prevents unnecessary requests. This approach is in compliance with the guidelines on error handling.
95-97: Aggregated loading state is well-handled.Combining all loading flags into a single
isLoadingvariable maintains clarity. Ensure you handle or display potential error states elsewhere in the flow.
113-114: Verify the desired user flow for pending approvals.Hiding the release when
isPendingApprovalmight be intentional. Confirm that this matches the UX requirements for pending states.
187-187: Ensure coherent fallback for deployment.Rendering the
DeployButtonwhen no other condition applies is straightforward. Verify that this behavior aligns with expected user workflows.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/EnvironmentApprovalRow.tsx (2)
3-3: Removal of unused types aligns with code cleanupThanks for removing the
Environmentimport if it's no longer used. This keeps dependencies minimal and the file more maintainable.
20-20: Consistent removal of thelinkedEnvironmentspropPassing only
releaseandpolicyIdsimplifies this component’s interface, aligning with the shift away from props-based environment data. Confirm that any logic relying onlinkedEnvironmentsnow relies on an appropriate API or other data source.✅ Verification successful
Verified: ApprovalDialog successfully migrated from props to API-based data fetching
The component now consistently uses only
releaseandpolicyIdprops across all usages, with environment data being fetched through API queries. The remaininglinkedEnvironmentsreferences in the codebase are in unrelated Flow components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that there are no remaining references to the removed `linkedEnvironments` prop rg 'linkedEnvironments'Length of output: 603
Script:
#!/bin/bash # Find ApprovalDialog component definition and usages ast-grep --pattern 'function ApprovalDialog' rg -A 5 'ApprovalDialog.*='Length of output: 3079
Script:
#!/bin/bash # Find ApprovalDialog component definition fd ApprovalDialog.tsx rg -A 10 'export.*ApprovalDialog'Length of output: 2007
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/ApprovalCheck.tsx (7)
3-3: No issues with the newly added import.
32-37: Invalidate function looks good.
Clearing the cache after approve/reject is a sensible approach to keep data fresh.
52-52: No content to comment on.
61-65: Loading spinner implementation looks good.
This provides a clear user experience while data is being fetched.
86-90: Approve/Reject actions appear correct.
The footer structure is straightforward and consistent with standard UI patterns.
99-99: Signature changes look consistent with the removal of linkedEnvironments.
This aligns with the new approach that sources environment data from the query.
142-142: No issues in the updated usage of ApprovalDialog.
Passing only the needed props keeps the component concise.
Screen_Recording_2025-01-24_at_1.27.11_PM.mov
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
linkedEnvironmentsprop from multiple components.