Conversation
ogzhanolguncu
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughRemoves legacy promotion/rollback dialog files and reintroduces them under a new actions path; adds shared DeploymentCard/DeploymentSection/DomainsSection UI, ProximityPrefetch, and collection preload; replaces Project.updatedAt with latestDeploymentId across schema and queries; simplifies Go deploy polling by removing per-step updates; several UI/shortcut tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as PromotionDialog (client)
participant TRPC as TRPC API
participant Cache as TRPC utils
participant Coll as Collections (projects/deployments/domains)
User->>UI: clicks "Promote"
UI->>TRPC: mutate(promote, { targetDeploymentId, liveDeploymentId })
TRPC-->>UI: success
UI->>Cache: invalidate()
UI->>Coll: try refetch() (projects, deployments, domains)
UI-->>User: show success toast, close dialog
TRPC-->>UI: error
UI-->>User: show error toast
sequenceDiagram
autonumber
participant CLI as deploy command
participant CP as ControlPlaneClient
CLI->>CP: PollDeploymentStatus(deploymentID, onStatusChange)
loop polling
CP->>CP: fetch deployment status
alt status changed
CP->>CLI: onStatusChange(DeploymentStatusEvent)
end
end
Note over CP: per-step updates removed — no step events emitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/index.tsx (1)
170-186: Duplicate onClick handlers break the chevron toggle.The outer
buttonelement (line 172) and the innerButtoncomponent (line 178) both haveonClick={() => setExpanded(!isExpanded)}. When clicking the inner chevron button, both handlers fire in sequence, toggling the state twice and canceling out the effect—leaving the expansion state unchanged.Apply this diff to remove the duplicate handler from the inner Button:
- <button - className="flex items-center gap-1.5" - onClick={() => setExpanded(!isExpanded)} - type="button" - > + <button + className="flex items-center gap-1.5" + onClick={() => setExpanded(!isExpanded)} + type="button" + > <div className="text-grayA-9 text-xs"> {showBuildSteps ? "Build logs" : "Gateway logs"} </div> - <Button size="icon" variant="ghost" onClick={() => setExpanded(!isExpanded)}> + <Button size="icon" variant="ghost" asChild> + <div> <ChevronDown className={cn( "text-grayA-9 !size-3 transition-transform duration-200", isExpanded && "rotate-180", )} /> + </div> </Button> </button>Alternatively, if you want the chevron to be a distinct clickable area, remove the outer button's onClick and keep only the inner Button's handler.
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/project-actions.tsx (1)
65-73: Address placeholder deployment ID in project settings navigation.Lines 69-71 contain placeholder code with a hardcoded fake deployment ID (
"idk"), which will result in broken navigation for the "Project settings" menu item.The comment indicates this will change soon. To prevent this from being forgotten:
- Either implement the correct navigation now (if the proper route is known), or
- Temporarily disable this menu item until the correct implementation is ready.
Would you like me to open a new issue to track completing this implementation?
🧹 Nitpick comments (11)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/filter-button.tsx (1)
28-28: Visual distinction added for inactive state icons.The change reduces the icon size for inactive filter buttons using
!size-3, creating a visual hierarchy where active buttons have larger icons. However, the!importantflag suggests a specificity conflict.Consider whether the important flag is necessary or if the underlying size conflict can be resolved at the component/theme level for cleaner CSS.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/index.tsx (1)
211-218: Consider aligning internal filter state with UI labels.The UI labels were updated to "5XX" and "4XX" (lines 211, 218), but the internal
logFilterstate still checks for"errors"and"warnings"(lines 207, 214). This mismatch between user-facing terminology and internal state can reduce code clarity.For consistency, consider updating the filter state values to match the UI labels (e.g.,
"5xx"and"4xx"), or alternatively keep the generic terms in the UI if the filters apply beyond HTTP status codes. This change would touch theuseDeploymentLogshook and related filter logic.apps/dashboard/lib/trpc/routers/deploy/project/list.ts (1)
21-21: LGTM! Backend provides latestDeploymentId via subquery.The new subquery correctly selects the most recent deployment ID for each project, and the mapping to
latestDeploymentIdaligns with the updated project schema. The workspace filter in the subquery ensures proper multi-tenancy.For future optimization, consider whether the subquery could be moved to a materialized column or index if this query becomes a performance bottleneck. However, for current usage, the implementation is correct and performant enough.
Also applies to: 44-52, 79-79
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/layout.tsx (1)
43-47: Avoid @ts-expect-error by exposing a typed invalidate/refetch method.Instead of calling internal utils.refetch with a type error, add a typed method on collectionManager (e.g., collections.domains.invalidate() or collectionManager.invalidate("domains")) and use that here.
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/index.tsx (1)
80-112: Leverage ProximityPrefetch route prefetch (optional).If you can compute the project route here, pass it via the route prop to also prefetch the page for faster nav. Alternatively, move ProximityPrefetch inside ProjectCard where projectPath is known and provide route there.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/domains-section.tsx (1)
20-26: LGTM; minor style consistency nit.Implementation is sound. Optional: align border color classes with DomainRow’s defaults to avoid mixed gray tokens.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-card.tsx (1)
20-22: Prefer shortenId for consistency; add branch fallback.Use shortenId for deployment.id formatting (already used for commit SHA) and guard against missing gitBranch.
- <span className="text-xs text-accent-12 font-mono"> - {`${deployment.id.slice(0, 3)}...${deployment.id.slice(-4)}`} - </span> + <span className="text-xs text-accent-12 font-mono"> + {shortenId(deployment.id, { startChars: 3, endChars: 4 })} + </span> @@ - <span className="truncate">{deployment.gitBranch}</span> + <span className="truncate">{deployment.gitBranch ?? "-"}</span>Also applies to: 36-43
apps/dashboard/components/proximity-prefetch.tsx (1)
95-103: Reduce rerenders and duplicate global listeners.Each instance adds a window mousemove listener and updates state on every event, causing frequent rerenders and N listeners in lists. Prefer a single global pointermove subscription and store position in a ref, or throttle via rAF.
Options:
- Replace mousemove with pointermove for broader device support.
- Track mouse position in a ref and run proximity logic inside the listener with a small debounce, avoiding React state updates per event.
- Share a singleton listener (module scope) that notifies all instances.
Also applies to: 105-116
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/rollback-dialog.tsx (1)
41-51: Remove brittle refetch “hack” and rely on proper invalidation.
collection.projects/deployments/domains.utils.refetch()is suppressed with@ts-expect-errorand likely undefined at runtime (caught and logged). This is noisy and fragile. You already callutils.invalidate(); that should be the source of truth. If additional refresh is needed, expose typed refetch APIs viacollectionManageror context instead of reaching into non-existent fields.Apply this diff to drop the hack:
- // hack to revalidate - try { - // @ts-expect-error Their docs say it's here - collection.projects.utils.refetch(); - // @ts-expect-error Their docs say it's here - collection.deployments.utils.refetch(); - // @ts-expect-error Their docs say it's here - collection.domains.utils.refetch(); - } catch (error) { - console.error("Refetch error:", error); - }apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/promotion-dialog.tsx (2)
40-50: Remove brittle refetch “hack” and rely on proper invalidation.Same issue as in RollbackDialog. Suppressed, likely undefined, and noisy. Prefer
utils.invalidate()and/or a typed refetch viacollectionManager.Apply this diff:
- // hack to revalidate - try { - // @ts-expect-error Their docs say it's here - collection.projects.utils.refetch(); - // @ts-expect-error Their docs say it's here - collection.deployments.utils.refetch(); - // @ts-expect-error Their docs say it's here - collection.domains.utils.refetch(); - } catch (error) { - console.error("Refetch error:", error); - }
71-91: Minor: Button label polish (optional).Consider “Promote to” + shortened SHA/id on the same line might read better with a non-breaking space or colon. Not required.
Example:
- “Promote to: {shortenId(...)}”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/promotion-dialog.tsx(0 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/rollback-dialog.tsx(0 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-card.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-section.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/domains-section.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx(2 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/promotion-dialog.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/rollback-dialog.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/filter-button.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/index.tsx(3 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/domain-row.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/layout.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/client.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx(3 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/index.tsx(2 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/project-actions.tsx(3 hunks)apps/dashboard/components/logs/live-switch-button/index.tsx(2 hunks)apps/dashboard/components/proximity-prefetch.tsx(1 hunks)apps/dashboard/lib/collections/deploy/projects.ts(1 hunks)apps/dashboard/lib/collections/index.ts(1 hunks)apps/dashboard/lib/trpc/routers/deploy/project/list.ts(3 hunks)go/cmd/deploy/control_plane.go(0 hunks)go/cmd/deploy/main.go(5 hunks)internal/ui/src/components/buttons/refresh-button.tsx(2 hunks)internal/ui/src/components/logs/control-cloud/index.tsx(3 hunks)
💤 Files with no reviewable changes (3)
- apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/rollback-dialog.tsx
- apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/promotion-dialog.tsx
- go/cmd/deploy/control_plane.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsxapps/dashboard/lib/trpc/routers/deploy/project/list.ts
📚 Learning: 2025-04-24T14:34:30.621Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3115
File: apps/dashboard/components/logs/checkbox/filters-popover.tsx:33-55
Timestamp: 2025-04-24T14:34:30.621Z
Learning: In the ShortcutActivator component within filters-popover.tsx, the purpose is to track keys separately for each filter item, providing a registration mechanism for shortcuts passed to it rather than enforcing specific key combinations like option+shift+key.
Applied to files:
internal/ui/src/components/logs/control-cloud/index.tsx
📚 Learning: 2025-07-28T20:38:53.244Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx:322-341
Timestamp: 2025-07-28T20:38:53.244Z
Learning: In apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx, mcstepp prefers to keep hardcoded endpoint logic in the getDiffType function during POC phases for demonstrating diff functionality, rather than implementing a generic diff algorithm. This follows the pattern of keeping simplified implementations for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/client.tsxapps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx
🧬 Code graph analysis (14)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/layout.tsx (1)
apps/dashboard/lib/collections/deploy/projects.ts (1)
projects(43-109)
apps/dashboard/components/logs/live-switch-button/index.tsx (1)
apps/dashboard/hooks/use-keyboard-shortcut.tsx (1)
useKeyboardShortcut(226-304)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/index.tsx (2)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/filter-button.tsx (1)
FilterButton(13-34)internal/icons/src/icons/triangle-warning-2.tsx (1)
TriangleWarning2(16-45)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-card.tsx (4)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
StatusIndicator(26-69)apps/dashboard/lib/shorten-id.ts (1)
shortenId(5-54)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/promotion-dialog.tsx (1)
DeploymentCardProps(146-180)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/rollback-dialog.tsx (1)
DeploymentCardProps(145-179)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/promotion-dialog.tsx (6)
apps/dashboard/lib/collections/index.ts (2)
collectionManager(67-67)collection(70-74)internal/db/src/schema/domains.ts (1)
domains(4-36)apps/dashboard/lib/trpc/routers/deploy/deployment/promote.ts (1)
promote(14-122)apps/dashboard/lib/shorten-id.ts (1)
shortenId(5-54)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-section.tsx (1)
DeploymentSection(12-25)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/domains-section.tsx (1)
DomainsSection(8-30)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/domains-section.tsx (2)
internal/db/src/schema/domains.ts (1)
domains(4-36)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/domain-row.tsx (1)
DomainRow(12-38)
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/project-actions.tsx (1)
apps/dashboard/components/logs/table-action.popover.tsx (2)
TableActionPopover(37-156)MenuItem(20-30)
go/cmd/deploy/main.go (1)
go/gen/proto/ctrl/v1/deployment.pb.go (3)
Deployment(412-446)Deployment(459-459)Deployment(474-476)
internal/ui/src/components/logs/control-cloud/index.tsx (1)
internal/ui/src/hooks/use-keyboard-shortcut.tsx (1)
useKeyboardShortcut(221-298)
apps/dashboard/lib/trpc/routers/deploy/project/list.ts (3)
internal/db/src/schema/domains.ts (1)
domains(4-36)internal/db/src/schema/deployments.ts (1)
deployments(9-54)internal/db/src/schema/projects.ts (1)
projects(8-31)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/rollback-dialog.tsx (6)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/layout-provider.tsx (1)
useProject(16-22)internal/db/src/schema/domains.ts (1)
domains(4-36)apps/dashboard/lib/trpc/routers/deploy/deployment/rollback.ts (1)
rollback(14-127)apps/dashboard/lib/collections/index.ts (1)
collection(70-74)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-section.tsx (1)
DeploymentSection(12-25)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/domains-section.tsx (1)
DomainsSection(8-30)
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/index.tsx (4)
apps/dashboard/components/proximity-prefetch.tsx (1)
ProximityPrefetch(52-123)apps/dashboard/lib/collections/index.ts (1)
collectionManager(67-67)apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/projects-card.tsx (1)
ProjectCard(24-135)apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/project-actions.tsx (1)
ProjectActions(16-23)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx (1)
apps/dashboard/lib/shorten-id.ts (1)
shortenId(5-54)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-section.tsx (3)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-card.tsx (1)
DeploymentCard(13-47)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/promotion-dialog.tsx (1)
DeploymentSectionProps(18-26)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/rollback-dialog.tsx (1)
DeploymentSectionProps(19-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
- GitHub Check: Test API / API Test Local
🔇 Additional comments (25)
internal/ui/src/components/buttons/refresh-button.tsx (1)
47-72: LGTM! Keyboard shortcut update is consistent.The keyboard shortcut change from
option+shift+rtooption+shift+wis correctly applied across all three locations:
- Hook binding (line 47)
- Tooltip text (line 65)
- Visual display (line 72)
This aligns with the PR's objective to improve keyboard shortcuts for better accessibility.
apps/dashboard/components/logs/live-switch-button/index.tsx (3)
12-12: LGTM! Improved keyboard shortcut ergonomics.The shortcut change from L to Q places the key closer to the modifier keys (Option+Shift), making it more accessible for single-handed use.
19-19: Consistent tooltip text.The button title correctly reflects the new keyboard shortcut, ensuring users see the updated shortcut when hovering.
34-34: Visual display updated correctly.The KeyboardButton component now displays the new shortcut, maintaining consistency across all UI elements (hook registration, tooltip, and visual display).
go/cmd/deploy/main.go (5)
12-13: LGTM!The added imports support the new title-casing functionality for environment names, which is used in the
printCompletionInfofunction.
285-291: LGTM!The rename from
deploymentIdtodeploymentIDfollows Go naming conventions where acronyms should be capitalized (ID, not Id).
309-309: LGTM!The simplified
PollDeploymentStatuscall removes per-step update callbacks, streamlining the deployment polling logic to focus on major status transitions.
366-366: LGTM!The updated signature adds the
envparameter to display environment information in the completion summary, called correctly at line 320.
372-378: Title-casing improves output readability.The implementation correctly uses
cases.Title(language.English)to format environment names for display. This works well for simple slugs like "preview" or "production."Note: If environment slugs can contain hyphens or special formatting (e.g., "staging-v2"),
cases.Titlewill capitalize each word segment, which may or may not be desired. For most common cases, this should be fine.internal/ui/src/components/logs/control-cloud/index.tsx (1)
30-30: LGTM! Keyboard shortcut remapping fromoption+shift+d/ctooption+shift+a/simproves ergonomics, UI labels are updated accordingly, and no conflicts with existing shortcuts were found.apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/project-actions.tsx (3)
25-29: LGTM!The function signature correctly accepts
workspaceSlugto support workspace-scoped routing, aligning with the broader navigation pattern improvements in this PR.
58-58: LGTM!The label change from "View logs" to "View gateway logs" improves specificity and user clarity.
61-61: LGTM!The navigation path correctly constructs a workspace-scoped route using the provided
workspaceSlugparameter.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/client.tsx (1)
115-133: LGTM! Enhanced empty state UI.The new animated empty state provides a more polished user experience with a gradient accent, pulsing icon, and clearer messaging. The use of semantic HTML (h3, p) and appropriate color tokens (grayA-12, grayA-9) maintains accessibility.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx (2)
3-3: LGTM! Consistent deployment ID presentation.The use of
shortenIdfor deployment labels improves readability and aligns with the broader UI overhaul for deployment-related components across the PR.Also applies to: 88-88
174-174: LGTM! Enhanced loading and error states.The updated padding and richer error UI (with gradient, icon, and clear messaging) provide a more polished user experience consistent with other UI improvements in this PR.
Also applies to: 182-202
apps/dashboard/lib/collections/index.ts (2)
41-45: LGTM! Useful preloading capability added.The new
preloadProjectmethod enables efficient parallel preloading of all collections for a given project, which aligns with the PR's goal of improving data loading (e.g., proximity prefetch mentioned in the AI summary).
47-54: LGTM! Cleanup refactored for consistency.The refactored
cleanupmethod now uses the same parallel pattern aspreloadProject, improving maintainability and consistency across the collection manager.apps/dashboard/lib/collections/deploy/projects.ts (1)
12-12: LGTM! Schema updated to include latestDeploymentId.The addition of
latestDeploymentIdto the project schema aligns with the backend changes (seen inlist.ts) and enables UI components to key on the latest deployment for refetch logic and rendering.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/deployments-list.tsx (1)
119-119: LGTM! DomainList keying updated for latestDeploymentId.The expanded composite key now includes
project?.latestDeploymentId, ensuring theDomainListre-mounts when the latest deployment changes. This prevents stale domain displays and aligns with the new project schema.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx (2)
10-11: LGTM! Import paths updated for relocated dialogs.The updated import paths reflect the restructuring of
PromotionDialogandRollbackDialogfrom sibling directories to subdirectories within the actions folder, as noted in the AI summary.
83-83: LGTM! URL path corrected with leading slash.Adding the leading slash ensures the gateway logs URL is constructed as an absolute path from the workspace root, preventing potential routing issues.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/domain-row.tsx (1)
3-3: LGTM! DomainRow now accepts external styling.The addition of an optional
classNameprop with proper composition viacnenables flexible styling while preserving default classes. This aligns with the PR's UI improvements and supports usage in components likeDomainsSection.Also applies to: 9-9, 12-19
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/components/deployment-section.tsx (1)
18-24: LGTM.Simple, cohesive wrapper around DeploymentCard with a clear header.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/deployments/components/table/components/actions/rollback-dialog.tsx (1)
29-33: Validate domain filter logic (inconsistent with PromotionDialog).Here you filter only on
sticky ∈ ["environment","live"]. In PromotionDialog you also addeq(domain.deploymentId, liveDeployment.id). Confirm which is intended and align both dialogs.
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/list/project-actions.tsx
Show resolved
Hide resolved
...g]/projects/[projectId]/deployments/components/table/components/actions/promotion-dialog.tsx
Show resolved
Hide resolved
...ug]/projects/[projectId]/deployments/components/table/components/actions/rollback-dialog.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/layout.tsx
Show resolved
Hide resolved
|
that screenshot makes me wonder whether we should show shortcuts on hover only |
if you ask my opinion I don't think anyone using them but me/us? 😄 So we can hide them yeah |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/create-project/create-project-dialog.tsx (1)
65-65: Consider removing or upgrading to structured logging.Since the collection's
onInserthandler already shows error toasts (line 66 comment), thisconsole.errormay be redundant. If error logging is needed for debugging, consider using a structured logging solution instead.internal/ui/src/components/logs/control-cloud/index.tsx (1)
151-153: Consider usingmax-w-fitfor flexible content sizing.The hover transition uses a fixed
max-w-[100px], which could clip content or leave excess space depending on the actual width ofKeyboardButton. Usingmax-w-fitwould accommodate varying content widths more gracefully.-<div className="max-w-0 opacity-0 group-hover:max-w-[100px] group-hover:opacity-100 transition-all duration-300 ease-in-out overflow-hidden"> +<div className="max-w-0 opacity-0 group-hover:max-w-fit group-hover:opacity-100 transition-all duration-300 ease-in-out overflow-hidden"> <KeyboardButton shortcut="⌥+⇧+A" /> </div>Note: This suggestion assumes the critical KeyboardButton API issue in the previous comment is addressed first.
Also applies to: 158-160
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/create-project/create-project-dialog.tsx(1 hunks)apps/dashboard/components/logs/live-switch-button/index.tsx(2 hunks)internal/ui/src/components/buttons/keyboard-button.tsx(1 hunks)internal/ui/src/components/buttons/refresh-button.tsx(2 hunks)internal/ui/src/components/logs/control-cloud/index.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/logs/live-switch-button/index.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T14:34:30.621Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3115
File: apps/dashboard/components/logs/checkbox/filters-popover.tsx:33-55
Timestamp: 2025-04-24T14:34:30.621Z
Learning: In the ShortcutActivator component within filters-popover.tsx, the purpose is to track keys separately for each filter item, providing a registration mechanism for shortcuts passed to it rather than enforcing specific key combinations like option+shift+key.
Applied to files:
internal/ui/src/components/logs/control-cloud/index.tsx
🧬 Code graph analysis (2)
internal/ui/src/components/logs/control-cloud/index.tsx (1)
internal/ui/src/hooks/use-keyboard-shortcut.tsx (1)
useKeyboardShortcut(221-298)
internal/ui/src/components/buttons/refresh-button.tsx (1)
internal/icons/src/icons/refresh-3.tsx (1)
Refresh3(15-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
internal/ui/src/components/buttons/refresh-button.tsx (3)
47-47: LGTM! Keyboard shortcut updated for better accessibility.The change from
option+shift+rtooption+shift+waligns with the PR objective to improve shortcut ergonomics by using keys that are easier to reach (Q-W pattern).
65-74: LGTM! Clean implementation of hover-reveal pattern.The changes effectively implement the "show on hover" behavior discussed in the PR comments:
- Title tooltip correctly reflects the new shortcut (line 65)
groupandoverflow-hiddenclasses enable the parent hover state (line 68)- Smooth transition animation reveals the keyboard shortcut on hover using
max-w-0→max-w-[100px]and opacity changes (lines 72-74)This addresses the concern about shortcuts taking up excessive screen space while keeping them discoverable.
83-83: LGTM! Proper public API export.The export statement correctly exposes both the component and its props type, following standard React/TypeScript conventions.
apps/dashboard/app/(app)/[workspaceSlug]/projects/_components/create-project/create-project-dialog.tsx (2)
45-45: LGTM!The change from
updatedAttolatestDeploymentIdcorrectly aligns with the schema update described in the PR summary. Initializing it asnullfor a new project is appropriate.
44-52: Verify server-side placeholder replacement.The insert payload relies on the server to replace several placeholder values (id, author, authorAvatar, branch, commitTitle, domain). Ensure the server-side logic consistently replaces these fields to prevent placeholder strings from appearing in the UI.
internal/ui/src/components/buttons/keyboard-button.tsx (1)
29-29: LGTM! Clean simplification.Passing
classNamedirectly tocn()is clearer than wrapping it in an object. Thecnutility handles string arguments natively, so this change maintains functionality while improving readability.internal/ui/src/components/logs/control-cloud/index.tsx (3)
30-46: LGTM! Shortcut update aligns with PR objectives.The keyboard shortcut change from
option+shift+dtooption+shift+aplaces the key in a more accessible position, as noted in the PR description. The string format is correctly parsed byuseKeyboardShortcut.
48-50: LGTM! Consistent shortcut repositioning.The keyboard shortcut change from
option+shift+ctooption+shift+smaintains consistency with the previous change and improves accessibility.
133-162: Verify keyboard-only accessibility for hover-revealed shortcuts.The hover-reveal pattern addresses the space concern raised in PR comments, but shortcuts only appear on mouse hover (
:hover). Keyboard-only users navigating with Tab won't see these hints.Consider extending the reveal trigger to include focus states:
-<div className="max-w-0 opacity-0 group-hover:max-w-[100px] group-hover:opacity-100 transition-all duration-300 ease-in-out overflow-hidden"> +<div className="max-w-0 opacity-0 group-hover:max-w-[100px] group-hover:opacity-100 group-focus-within:max-w-[100px] group-focus-within:opacity-100 transition-all duration-300 ease-in-out overflow-hidden"> <KeyboardButton shortcut="⌥+⇧+A" /> </div>This ensures the shortcuts are visible when any element within the control cloud receives keyboard focus, improving accessibility for keyboard-only navigation.
|
nitpick, showing the openapi diff without a 2nd deployment makes no sense, it's not valuable to the user and you only encounter it on the first deploy, which is likely from a new users. We should try to minimize the noise for new users especially 
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create an issue for this improvement. 🏁 Script executed: Length of output: 44 @chronark I've created issue #4074 to track hiding the OpenAPI diff in the sidebar when there's only one deployment to compare. |
this is wrong
we said we'd have the left one be the live deployment, but here it's not.
edit: actually we didn't say that, we said the right one should be live. But now that I thinka bout it, that makes no sense |
I agree, but I don't really like experience of disappearing and appearing stuff. So I think current experience is good, but if it annoys you so much we can do something else 😄 |
|
Tip For best results, initiate chat on the files or code changes. @ogzhanolguncu, I understand your concern about the UX implications of elements appearing and disappearing. That's a valid consideration for maintaining a consistent interface experience. Since the issue has been created per chronark's request (#4074), the team can discuss the best implementation approach when you get to it. Perhaps there's a middle ground—like showing a disabled or grayed-out state instead of completely hiding it, or using a subtle placeholder message that explains why the diff isn't available yet. Feel free to adjust the approach in the issue description or close it if you decide the current behavior is preferable after all! 😊 |
so you basically wanna swap the order for both this details section and OpenAPI spec tab. got it. |
not just swapping but showing the correct one from = live deployment |
|
Okay, left one will always point to latest "production" and right one will always target the latest "preview". I also found 2 minor stuff and fixed them as well |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/cmd/deploy/main.go (1)
360-372: Environment title-casing improves presentation.The title-casing transformation correctly formats environment names for display (e.g., "preview" → "Preview"). The implementation is straightforward and appropriate for a CLI tool.
Consider extracting the caser to a package-level variable to avoid recreating it on every invocation:
+var envCaser = cases.Title(language.English) + func printCompletionInfo(deployment *ctrlv1.Deployment, env string) { if deployment == nil || deployment.GetId() == "" { fmt.Printf("✓ Deployment completed\n") return } - caser := cases.Title(language.English) - fmt.Println() fmt.Println(CompletionTitle) fmt.Printf(" %s: %s\n", CompletionDeploymentID, deployment.GetId()) fmt.Printf(" %s: %s\n", CompletionStatus, CompletionReady) - fmt.Printf(" %s: %s\n", CompletionEnvironment, caser.String(env)) + fmt.Printf(" %s: %s\n", CompletionEnvironment, envCaser.String(env))This is a micro-optimization and not critical for a CLI tool's performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/project-details-expandables/sections/open-api-diff.tsx(2 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx(4 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx(4 hunks)apps/dashboard/components/virtual-table/index.tsx(1 hunks)go/cmd/deploy/control_plane.go(0 hunks)go/cmd/deploy/main.go(5 hunks)
💤 Files with no reviewable changes (1)
- go/cmd/deploy/control_plane.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-28T20:38:53.244Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx:322-341
Timestamp: 2025-07-28T20:38:53.244Z
Learning: In apps/dashboard/app/(app)/projects/[projectId]/diff/[...compare]/components/client.tsx, mcstepp prefers to keep hardcoded endpoint logic in the getDiffType function during POC phases for demonstrating diff functionality, rather than implementing a generic diff algorithm. This follows the pattern of keeping simplified implementations for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/project-details-expandables/sections/open-api-diff.tsxapps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx
📚 Learning: 2025-07-25T19:11:00.208Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts:110-147
Timestamp: 2025-07-25T19:11:00.208Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/getOpenApiDiff.ts, the user mcstepp prefers to keep mock data fallbacks in POC/demonstration code for simplicity, even if it wouldn't be production-ready. This aligns with the PR being work-in-progress for demonstration purposes.
Applied to files:
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/project-details-expandables/sections/open-api-diff.tsx
🧬 Code graph analysis (4)
apps/dashboard/components/virtual-table/index.tsx (1)
apps/dashboard/components/virtual-table/components/loading-indicator.tsx (1)
LoadMoreFooter(19-210)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/project-details-expandables/sections/open-api-diff.tsx (2)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
StatusIndicator(26-69)apps/dashboard/lib/shorten-id.ts (1)
shortenId(5-54)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx (1)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
PulseIndicator(77-105)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx (1)
apps/dashboard/lib/shorten-id.ts (1)
shortenId(5-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
🔇 Additional comments (19)
apps/dashboard/components/virtual-table/index.tsx (1)
393-402: LGTM! Conditional rendering prevents unnecessary component mounting.The conditional check correctly prevents the
LoadMoreFooterfrom rendering whenloadMoreFooterPropsis not provided, which fixes the blinking/disappearing UI issue mentioned in the comment. This approach is more efficient than always mounting the component and relying on internal visibility logic or thehideprop.The pattern of spreading
loadMoreFooterProps(for optional configuration) while explicitly passing dynamic values (onLoadMore,isFetchingNextPage,totalVisible,totalCount) is appropriate and allows parent components to customize the footer behavior while theVirtualTablecontrols the calculated state.go/cmd/deploy/main.go (3)
12-13: LGTM!The golang.org/x/text packages are the standard library approach for Unicode-aware text transformations. Appropriate for title-casing environment display names.
279-279: LGTM!The renaming from
deploymentIdtodeploymentIDfollows Go naming conventions where initialisms should be fully uppercase. The change is applied consistently across the function.Also applies to: 285-285, 303-303
314-314: LGTM!Passing
opts.Environmentparameterizes the completion display, making it correctly reflect the deployed environment rather than using a default value.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/project-details-expandables/sections/open-api-diff.tsx (6)
44-44: LGTM! Correct deployment selection.The logic correctly identifies the latest preview deployment by excluding the live deployment from the query results. This aligns with the PR objective to show "from=live" and "to=latest preview".
48-48: LGTM! Correct old deployment assignment.Setting
oldDeploymentIdtoliveDeploymentIdis correct for the "from" side of the diff comparison.
54-68: LGTM! Proper handling of single deployment case.The logic correctly handles the edge case where there's a deployment but no live deployment set, displaying a simplified "current" view instead of a diff comparison.
74-74: LGTM! Correct URL parameter order.The diff URL construction correctly uses
from=${liveDeploymentId}&to=${newDeployment.id}, which aligns with the PR objective to compare from live deployment to latest preview.
80-86: LGTM! Appropriate status indicators.The left side (from/live deployment) correctly shows the diff status with signal indicator, which provides visual feedback about breaking changes or warnings.
92-92: StatusIndicator default props confirmed
StatusIndicator without explicit props defaults tostatus="safe"andwithSignal=false, rendering a safe icon without pulse. Please confirm this matches the intended UX for the “to/new deployment” indicator.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx (4)
62-62: Excellent fix! Deployment order now correct.This change correctly sets the "from" deployment to the live deployment when no URL parameters are present, addressing the issue raised by chronark in the PR comments. The deployment comparison now properly shows: from=live deployment, to=latest preview.
88-88: LGTM! Simplified deployment labeling.Using
shortenIdfor deployment labels provides consistency across the UI and simplifies the label generation logic.
174-178: LGTM! Improved loading state spacing.The padding adjustment from
mx-3 mb-3topx-8provides better visual consistency with other states.
182-201: LGTM! Enhanced error state UI.The improved error state with gradient background, icon, and better typography significantly improves the user experience when deployment comparison fails.
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx (5)
40-42: LGTM! Clear latest deployment identification.The logic correctly identifies the latest preview deployment by finding the first deployment that's not the live deployment. This assumes deployments are sorted by
createdAtdesc, which is confirmed in the query at line 29 of openapi-diff/page.tsx.
44-62: LGTM! Well-structured tooltip logic.The
getTooltipContenthelper provides clear, contextual tooltips for different deployment states (disabled, live, latest), improving the user experience.
64-72: LGTM! Helpful trigger title.The
getTriggerTitlehelper provides accessible title attributes for the select trigger, indicating whether the selected deployment is live or latest preview.
111-117: LGTM! Clear visual indicators.The dual PulseIndicator approach effectively distinguishes between live deployments (green pulse) and latest preview deployments (gray pulse), making it easy for users to identify deployment status at a glance.
131-131: LGTM! Accessible trigger title.Using
getTriggerTitle()provides a helpful title attribute that improves accessibility by indicating the deployment type on hover.
|
@coderabbitai create an issue to change the display of keyboardshortcuts to be in a tooltip instead |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 44 @chronark I've created issue #4086 to track changing the keyboard shortcuts display to use tooltips instead of the current hover-revealed containers, and assigned it to @ogzhanolguncu. |
Maybe not hide it completely, but a message about "not enough deployments to show spec diffs" so that the user is aware that the feature is there, they just don't meet the criteria to use it yet. |
|
|
|
Tip For best results, initiate chat on the files or code changes. @mcstepp That's a great middle-ground solution! Showing a message like "Not enough deployments to show spec diffs" would:
I'd recommend adding this suggestion as a comment on issue #4074, so the team can discuss the best implementation approach when they tackle it. This feels like a better UX than completely hiding the section! |
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/08/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
3 participants
What does this PR do?
This PR addresses issues on this linear ticket: https://linear.app/unkey/issue/ENG-2098/small-ui-tweaks.
I also updated some keyboard shortcuts. Two of them were broken and they had quite strange positions. So its on
Q-WandA-Sso its always easy to access.Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated