fix: ui in consistencies and linter issues#3979
Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors rollback dialog UI into modular components, introduces optional withSignal prop to StatusIndicator, updates ActiveDeploymentCard to use it, and adjusts DeploymentsList column memo dependencies and formatting. Public API change: StatusIndicator now accepts an optional withSignal boolean. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
please add a screenshot for these type of PRs, I have no idea what this even does |
|
@chronark 
this is the figma version
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (1)
408-416: Broken row keys: usingrow.idinstead ofrow.deployment.id.
keyExtractorreceives the row shape{ deployment, environment }. Usingdeployment.id(the variable name here is the row) returnsundefined, causing unstable/missing React keys.Apply:
- keyExtractor={(deployment) => deployment.id} + keyExtractor={(row) => row.deployment.id}
🧹 Nitpick comments (5)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
14-35: Ping keyframes reliability + a11y and affordance nits.
- Ensure
@keyframes pingis emitted by your build. Prefer Tailwind’sanimate-pingplus per-dotanimationDelayto avoid missing keyframes in CSS extraction.- The cloud wrapper shows a pointer cursor but is not interactive; drop the pointer to avoid misleading affordance.
- Mark the decorative signal group
aria-hidden.Apply inside the signal dots:
- className={cn( + className={cn( "absolute inset-0 size-2 rounded-full", + "animate-ping", index === 0 && "bg-successA-9 opacity-75", index === 1 && "bg-successA-10 opacity-60", index === 2 && "bg-successA-11 opacity-40", index === 3 && "bg-successA-12 opacity-25", )} style={{ - animation: "ping 2s cubic-bezier(0, 0, 0.2, 1) infinite", - animationDelay: `${delay}s`, + animationDelay: `${delay}s`, + animationDuration: "2s", }}Also mark the signal group decorative:
- <div className="absolute -top-0.5 -right-0.5"> + <div className="absolute -top-0.5 -right-0.5" aria-hidden="true">Outside this hunk, remove the pointer cursor on the icon container:
- <div className="size-5 rounded flex items-center justify-center cursor-pointer border border-grayA-3 transition-all duration-100 bg-grayA-3"> + <div className="size-5 rounded flex items-center justify-center border border-grayA-3 transition-all duration-100 bg-grayA-3">apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (1)
103-104: Good use of withSignal; tighten surrounding UI details.
- Badge text color is forced to success even for non-success variants; let the Badge style drive color.
- Replace “TODO” placeholders with real data.
- Use author avatar from data instead of a placeholder asset.
Suggested minimal fixes outside this hunk:
- <Badge variant={statusConfig.variant} className="text-successA-11 font-medium"> + <Badge variant={statusConfig.variant} className="font-medium">- <div className="text-gray-9 text-xs">TODO</div> + <div className="text-gray-9 text-xs"> + {deployment.gitCommitMessage ?? "—"} + </div>- <img src="TODO" alt="TODO" className="rounded-full size-5" /> + <img + src={deployment.gitCommitAuthorAvatarUrl ?? ""} + alt={deployment.gitCommitAuthorName ?? "Author"} + className="rounded-full size-5" + referrerPolicy="no-referrer" + loading="lazy" + />apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (3)
125-159: UseshortenIdconsistently for deployment IDs (avoid manual slicing).Keeps formatting consistent with commit SHA usage and centralizes rules.
- <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: 4, endChars: 4 })} + </span>
44-56: Avoid brittlecollection.projects.utils.refetch()hack.
utils.invalidate()should be sufficient. The extra refetch is undocumented and error‑prone.toast.success("Rollback completed", { description: `Successfully rolled back to deployment ${deployment.id}`, }); - // hack to revalidate - try { - // @ts-expect-error Their docs say it's here - collection.projects.utils.refetch(); - } catch (error) { - console.error("Refetch error:", error); - } onOpenChange(false);
10-25: Prop naming consistency: preferwithSignal(matchesStatusIndicator).Local
showSignalworks but diverges from the shared component prop, increasing cognitive overhead.-type DeploymentSectionProps = { +type DeploymentSectionProps = { title: string; deployment: Deployment; isActive: boolean; - showSignal?: boolean; + withSignal?: boolean; }; -const DeploymentSection = ({ title, deployment, isActive, showSignal }: DeploymentSectionProps) => ( +const DeploymentSection = ({ title, deployment, isActive, withSignal }: DeploymentSectionProps) => ( <div className="space-y-2"> @@ - <DeploymentCard deployment={deployment} isActive={isActive} showSignal={showSignal} /> + <DeploymentCard deployment={deployment} isActive={isActive} withSignal={withSignal} /> </div> );And in usages:
-<DeploymentSection +<DeploymentSection title="Current active deployment" deployment={currentDeployment} isActive={true} - showSignal={true} + withSignal />Prop threading:
-type DeploymentCardProps = { +type DeploymentCardProps = { deployment: Deployment; isActive: boolean; - showSignal?: boolean; + withSignal?: boolean; }; -const DeploymentCard = ({ deployment, isActive, showSignal }: DeploymentCardProps) => ( +const DeploymentCard = ({ deployment, isActive, withSignal }: DeploymentCardProps) => ( @@ - <StatusIndicator withSignal={showSignal} /> + <StatusIndicator withSignal={withSignal} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx(3 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mcstepp
PR: unkeyed/unkey#3952
File: apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx:70-79
Timestamp: 2025-09-12T17:57:18.305Z
Learning: In the deployment rollback functionality, self-rollback scenarios are prevented at the UI level in the actions menu through the `canRollback` condition which includes `!("active" in deployment && deployment.active)`, making additional checks in the dialog component redundant.
📚 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)/projects/[projectId]/deployments/components/table/deployments-list.tsx
📚 Learning: 2025-09-12T17:57:18.305Z
Learnt from: mcstepp
PR: unkeyed/unkey#3952
File: apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx:70-79
Timestamp: 2025-09-12T17:57:18.305Z
Learning: In the deployment rollback functionality, self-rollback scenarios are prevented at the UI level in the actions menu through the `canRollback` condition which includes `!("active" in deployment && deployment.active)`, making additional checks in the dialog component redundant.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx
🧬 Code graph analysis (4)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
internal/icons/src/icons/cloud.tsx (1)
Cloud(15-38)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (3)
apps/dashboard/components/virtual-table/types.ts (1)
Column(13-24)apps/dashboard/lib/collections/deployments.ts (1)
Deployment(38-38)apps/dashboard/lib/collections/environments.ts (1)
Environment(13-13)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (2)
internal/icons/src/icons/circle-warning.tsx (1)
CircleWarning(15-42)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
StatusIndicator(4-38)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (3)
go/gen/proto/ctrl/v1/deployment.pb.go (3)
Deployment(420-455)Deployment(468-468)Deployment(483-485)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
StatusIndicator(4-38)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). (3)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (7)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (1)
34-56: Status mapping refactor is a no-op.Pure reformat; semantics unchanged.
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (4)
163-166: Types on Column data look good.Explicit row shape improves readability in render fns.
389-395: Param typing in Action column: good.Matches the table row shape and ensures props safety.
406-406: useMemo deps:deploymentsintentionally omitted — verified.Columns useMemo (lines ~163–172) contains no references to
deployments;deployments.datais passed to VirtualTable at line ~410. No change required now — adddeploymentsto the deps if you later readdeployments(e.g., counts) inside any column.
229-241: runtimeConfig is required by schema — defensive checks unnecessary. runtimeConfig is defined as a required z.object in apps/dashboard/lib/collections/deployments.ts and is returned by the trpc router (apps/dashboard/lib/trpc/routers/deployment/list.ts); DB schema maps runtime_config as JSON (internal/db/src/schema/deployments.ts). Current direct accesses (regions.reduce, cpus, memory) align with the schema.Likely an incorrect or invalid review comment.
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx (1)
106-116: Modular sections/card: solid cleanup and parity maintained.The dialog reads cleaner and composes well. No extra self‑rollback checks added—aligned with prior guidance.
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
4-8: Public prop added — verified: no non‑JSX invocations foundSearch found only the definition and JSX usages in:
- apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (definition)
- apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx ()
- apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/skeleton.tsx ()
- apps/dashboard/app/(app)/projects/[projectId]/deployments/components/rollback-dialog.tsx ()
No plain function calls (StatusIndicator(...)) or createElement/h(StatusIndicator) usages were found.
2 participants
What does this PR do?
Fixes UI inconsistencies in rollback dialog and linter issues.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit