fix: allow updating status of deployment versions with no jobs - #989
adityachoudhari26 wants to merge 1 commit into
Conversation
- Lift VersionStatusDialog state outside DropdownMenu in VersionDropdown so the status dialog doesn't unmount when the dropdown closes (Radix DismissableLayer was firing onOpenChange(false) on the DropdownMenu when the Dialog overlay appeared, unmounting the Dialog along with the DropdownMenuContent) - Add VersionDropdown to NoActiveDeployments so versions with no active jobs also expose the "Update Status" action Fixes #988 Co-authored-by: Aditya Choudhari <adityachoudhari26@users.noreply.github.com>
|
|
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes UI/UX issues preventing users from updating a deployment version’s status when the version has no jobs, primarily by preventing Radix dropdown unmounting from immediately closing the status dialog and by surfacing the status action for zero-active-deployment versions.
Changes:
- Refactors
VersionStatusDialogto a controlledDialogwith open state lifted out ofDropdownMenuContentto avoid immediate dismissal/unmounting. - Adds
VersionDropdownto the “no active deployments” version card state so “Update Status” remains reachable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx | Converts the dialog to controlled state and decouples it from the dropdown content to prevent auto-close/unmount behavior. |
| apps/web/app/routes/ws/deployments/_components/versioncard/VersionCard.tsx | Renders the version actions dropdown even when there are no active deployments for the version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [status, setStatus] = useState<VersionStatus>(version.status); | ||
| const onClick = () => updateStatus(status).then(onClose); | ||
| const onClick = () => updateStatus(status).then(() => onOpenChange(false)); |
| const { updateStatus, isPending } = useUpdateVersionStatus(version.id); | ||
| const [status, setStatus] = useState<VersionStatus>(version.status); | ||
| const onClick = () => updateStatus(status).then(onClose); | ||
| const onClick = () => updateStatus(status).then(() => onOpenChange(false)); |
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-7 w-7 shrink-0 text-muted-foreground" | ||
| onClick={(e) => e.stopPropagation()} | ||
| > | ||
| <EllipsisIcon className="size-3" /> | ||
| </Button> |
| const { updateStatus, isPending } = useUpdateVersionStatus(version.id); | ||
| const [status, setStatus] = useState<VersionStatus>(version.status); | ||
| const onClick = () => updateStatus(status).then(onClose); | ||
| const onClick = () => updateStatus(status).then(() => onOpenChange(false)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx (1)
48-52:⚠️ Potential issue | 🟡 MinorAdd error handling for failed status updates.
If the mutation fails, the user receives no feedback. Consider adding a
.catch()handler to show an error toast.🛡️ Proposed fix to add error handling
const updateStatus = (status: VersionStatus) => updateVersionStatus .mutateAsync({ workspaceId, versionId, status }) .then(() => toast.success("Status update queued successfully")) - .then(() => invalidateVersions()); + .then(() => invalidateVersions()) + .catch((error) => { + toast.error("Failed to update status"); + throw error; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx` around lines 48 - 52, The updateStatus function currently chains updateVersionStatus.mutateAsync(...).then(...).then(...) without handling failures; add a .catch() to that promise chain (after invalidateVersions()) to call toast.error with a helpful message and optionally log the error so the user is notified when the mutation fails; reference updateStatus and updateVersionStatus.mutateAsync and ensure invalidateVersions() still runs only on success while the .catch handles errors from mutateAsync and the subsequent thens.
🧹 Nitpick comments (1)
apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx (1)
67-68: Consider resettingstatusstate when the dialog opens.The
statusstate is initialized fromversion.statusonly on mount. If the version's status is updated externally (e.g., by another user) and the dialog is reopened without the component remounting, the Select will show the stale initial value rather than the currentversion.status.♻️ Proposed fix using useEffect to sync state
+import { useState, useEffect } from "react"; ... function VersionStatusDialog({ version, open, onOpenChange, }: { version: Version; open: boolean; onOpenChange: (open: boolean) => void; }) { const { updateStatus, isPending } = useUpdateVersionStatus(version.id); const [status, setStatus] = useState<VersionStatus>(version.status); + + useEffect(() => { + if (open) { + setStatus(version.status); + } + }, [open, version.status]); + const onClick = () => updateStatus(status).then(() => onOpenChange(false));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx` around lines 67 - 68, The local status state (status, setStatus typed as VersionStatus) is only initialized from version.status on mount and can become stale if the dialog is reopened; add a useEffect that resets setStatus(version.status) whenever the dialog is opened (watch version.status and the dialog open flag/prop) so the Select shows the current version.status when the dialog opens, leaving onClick (updateStatus(...).then(()=>onOpenChange(false))) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx`:
- Around line 48-52: The updateStatus function currently chains
updateVersionStatus.mutateAsync(...).then(...).then(...) without handling
failures; add a .catch() to that promise chain (after invalidateVersions()) to
call toast.error with a helpful message and optionally log the error so the user
is notified when the mutation fails; reference updateStatus and
updateVersionStatus.mutateAsync and ensure invalidateVersions() still runs only
on success while the .catch handles errors from mutateAsync and the subsequent
thens.
---
Nitpick comments:
In
`@apps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx`:
- Around line 67-68: The local status state (status, setStatus typed as
VersionStatus) is only initialized from version.status on mount and can become
stale if the dialog is reopened; add a useEffect that resets
setStatus(version.status) whenever the dialog is opened (watch version.status
and the dialog open flag/prop) so the Select shows the current version.status
when the dialog opens, leaving onClick
(updateStatus(...).then(()=>onOpenChange(false))) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2cfa337-07f6-451a-9495-3ddf6048b206
📒 Files selected for processing (2)
apps/web/app/routes/ws/deployments/_components/versioncard/VersionCard.tsxapps/web/app/routes/ws/deployments/_components/versioncard/VersionDropdown.tsx
Fixes two related bugs where users could not update the status of a deployment version that has no jobs:
Dialog closes immediately:
VersionStatusDialogwas nested insideDropdownMenuContentas an uncontrolled RadixDialog. When the Dialog overlay appeared, Radix'sDismissableLayerfiredonOpenChange(false)on the parentDropdownMenu, unmountingDropdownMenuContentand the Dialog with it. Fixed by converting to a controlled Dialog with state lifted outside the dropdown.No Update Status option for zero-job versions:
NoActiveDeploymentsdidn't renderVersionDropdown, so there was no way to trigger "Update Status" for versions with no active deployments. Fixed by addingVersionDropdownto the component.Closes #988
Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactor