-
Notifications
You must be signed in to change notification settings - Fork 4
feat: canary release select #319
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
Conversation
WalkthroughAdds canary-by-device-ids support and client UI for three canary modes (automatic, labels, devices). Backend validates mutual exclusivity, selects devices via IDs, labels, or default logic, and inserts deployment associations. Frontend provides a two-step deploy modal to preview and submit canary selections. Changes
Sequence DiagramsequenceDiagram
actor User as User
participant UI as Deployment Modal
participant API as Backend API
participant DB as Database
User->>UI: Open deploy modal → choose mode
alt Select by Device IDs
UI->>API: POST /releases/{id}/deployment (canary_device_ids=[...])
else Select by Labels
UI->>API: POST /releases/{id}/deployment (canary_device_labels=[...])
else Automatic
UI->>API: POST /releases/{id}/deployment ()
end
Note over API: Validate not both IDs and labels
alt IDs provided
API->>DB: SELECT devices WHERE id IN ($3) AND release/target match AND recent_ping
else Labels provided
API->>DB: SELECT DISTINCT devices WHERE labels match AND release alignment
else Automatic
API->>DB: SELECT top N devices by recency & score
end
DB-->>API: device list
API->>DB: INSERT INTO deployment_devices (deployment_id, device_id) SELECT ...
DB-->>API: insert result (rows affected)
API-->>UI: 200 OK / Deployment started or 400 if no devices
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
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: 2
🤖 Fix all issues with AI agents
In `@api/src/deployment/mod.rs`:
- Around line 75-90: The current logic treats canary_device_labels: [] or
canary_device_ids: [] as None (falling back to auto-selection); change
validation to explicitly reject empty selectors: add checks that detect when
request.as_ref().and_then(|r| r.canary_device_labels.as_ref()) is Some and empty
(and similarly for canary_device_ids) and if either is explicitly empty call
tx.rollback().await? and return Err(ApiError::bad_request("canary_device_labels
cannot be empty") or similar); keep the existing has_labels/has_ids behavior for
None so only explicit Some([]) is rejected and existing mutual-exclusion check
between canary_device_labels and canary_device_ids (using has_labels/has_ids)
remains unchanged.
In `@dashboard/app/api-client.ts`:
- Around line 89-92: The DeploymentRequest currently allows both
canary_device_labels and canary_device_ids simultaneously; update the Rust model
in models/src/deployment.rs to make these mutually exclusive by replacing the
flat struct with a discriminated enum (use #[serde(untagged)] and add
#[schema(discriminator = "kind")] on the enum) so utoipa emits a oneOf schema;
define two variants (e.g., Labels { kind: "labels", canary_device_labels:
Vec<String> } and Ids { kind: "ids", canary_device_ids: Vec<i32> })
corresponding to the existing fields and keep any shared fields in a common
variant or wrapper as needed so Orval will generate a TypeScript union instead
of allowing both fields at once.
🧹 Nitpick comments (3)
dashboard/app/(private)/releases/[id]/page.tsx (3)
145-162: Verify:labelMatchDevicesincludes offline devices whileeligibleDevicesfilters them out.The
eligibleDevicesquery (line 132) usesonline: true, butlabelMatchDevicesdoes not include this filter. This means the labels preview may show devices that are offline and won't actually receive the canary update, potentially misleading users about which devices will be targeted.Consider adding
online: trueto maintain consistency:const { data: labelMatchDevices = [], isLoading: labelDevicesLoading } = useGetDevices( { distribution_id: release?.distribution_id, + online: true, outdated: false, labels: canaryLabels, },Alternatively, if showing offline devices is intentional for preview purposes, consider adding a visual indicator for offline devices in the preview list.
823-840: Accessibility: Device selection lacks keyboard support.The device selection items use
onClickondivelements without keyboard accessibility. Users cannot navigate or select devices using keyboard.Consider using semantic elements or adding proper ARIA attributes:
♿ Suggested accessibility improvements
- <div - key={device.id} - onClick={() => - setCanaryDeviceIds((prev) => { - const next = new Set(prev); - if (next.has(device.id)) { - next.delete(device.id); - } else { - next.add(device.id); - } - return next; - }) - } - className={`flex items-center gap-3 px-3 py-2.5 border-b border-gray-100 last:border-b-0 transition-colors cursor-pointer ${ - isSelected - ? "bg-blue-50" - : "hover:bg-gray-50" - }`} - > + <div + key={device.id} + role="checkbox" + aria-checked={isSelected} + tabIndex={0} + onClick={() => + setCanaryDeviceIds((prev) => { + const next = new Set(prev); + if (next.has(device.id)) { + next.delete(device.id); + } else { + next.add(device.id); + } + return next; + }) + } + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + setCanaryDeviceIds((prev) => { + const next = new Set(prev); + if (next.has(device.id)) { + next.delete(device.id); + } else { + next.add(device.id); + } + return next; + }); + } + }} + className={`flex items-center gap-3 px-3 py-2.5 border-b border-gray-100 last:border-b-0 transition-colors cursor-pointer ${ + isSelected + ? "bg-blue-50" + : "hover:bg-gray-50" + }`} + >You could also extract the toggle logic into a handler to avoid duplication.
979-1020: Selected devices may disappear from preview if they go offline.The preview filters
eligibleDevices(which only includes online devices) bycanaryDeviceIds. If a device goes offline after selection, it will disappear from the preview while its ID remains incanaryDeviceIds. The deployment would still attempt to target that device ID.Consider either:
- Filtering out stale selections when eligibleDevices changes
- Storing selected device objects (not just IDs) to preserve preview state
- Adding a visual indicator or message when selected devices are no longer eligible
| volumes: | ||
| # Mount source code for hot reloading | ||
| - ./api:/app/api | ||
| - ./cli:/app/cli |
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 volumen mount for local dev on cli
No description provided.