Conversation
|
|
Warning Rate limit exceeded@ogzhanolguncu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a className prop and props type to StatusIndicator, makes DetailRow icon/label nullable with conditional rendering, adds live domains retrieval via useLiveQuery to project details and renders dynamic domain links and tooltip, and adds a new “OpenAPI changes” detail section leveraging StatusIndicator and layout adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Project Details UI
participant Hook as useLiveQuery(domains)
participant API as Domains Store
User->>UI: Navigate to Project Details
UI->>Hook: subscribe(project?.liveDeploymentId)
Hook->>API: query domains by liveDeploymentId
API-->>Hook: domains[]
Hook-->>UI: { project, domains }
UI->>UI: Render project name
UI->>UI: Render first domain link (domains[0])
alt Additional domains
UI->>UI: Show tooltip with domains[1..]
else No additional domains
UI->>UI: No tooltip
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (2)
9-17: Clarify which elementclassNametargets (container vs indicator).
classNameis applied to the inner indicator, not the root container (relative). This is easy to misread and can surprise consumers who try to move/position the whole component. Consider renaming toindicatorClassNameand optionally addingcontainerClassNamefor the outer div.
12-17: Remove “cursor-pointer” or wire up interactivity and accessibility.There’s no click handler; the cursor suggests interactivity. Either remove it or add
onClick,role="button", and keyboard handling.- "size-5 rounded flex items-center justify-center cursor-pointer border border-grayA-3 transition-all duration-100 bg-grayA-3", + "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/project-details-expandables/detail-section.tsx (1)
53-61: Avoid unstable keys whenlabelcan be null.
key={${item.label}-${index}}becomes"null-<index>"for unlabeled rows and may collide. Fallback to index (or add a stableidon items).- <DetailRow - key={`${item.label}-${index}`} + <DetailRow + key={item.label ? `${item.label}-${index}` : `row-${index}`} icon={item.icon} label={item.label} alignment={item.alignment} >apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx (1)
128-150: Defensive rendering and UX polish for additional domains.
- With the default
domains = [], this is safe, but consider hiding the badge when there are no extra domains.- <InfoTooltip + {domains.length > 1 && ( + <InfoTooltip position={{ side: "bottom", }} content={ <div className="space-y-1 z-40"> {domains.slice(1).map((d) => ( <div key={d.domain} className="text-xs font-medium flex items-center gap-1.5" > <div className="w-1 h-1 bg-gray-8 rounded-full" /> <a href={`https://${d.domain}`} target="_blank" rel="noopener noreferrer" className="hover:underline truncate max-w-[270px]" > {d.domain} </a> </div> ))} </div> } > <div className="rounded-full px-1.5 py-0.5 bg-grayA-3 text-gray-12 text-xs leading-[18px] font-mono tabular-nums"> +{domains.slice(1).length} </div> - </InfoTooltip> + </InfoTooltip> + )}Ensure design expects the badge to hide when there are no additional domains.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx(1 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/detail-section.tsx(1 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/sections.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T12:06:14.204Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3833
File: apps/dashboard/components/navigation/sidebar/app-sidebar/components/nav-items/nested-nav-item.tsx:56-65
Timestamp: 2025-08-22T12:06:14.204Z
Learning: In the navigation sidebar components, ReactNode labels are only used for "More" labels (load more buttons), while regular navigation items use string labels. This means type guards like `typeof item.label === "string"` are still valid for most navigation logic since ReactNode usage is limited to specific load-more functionality.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/detail-section.tsx
🧬 Code graph analysis (2)
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx (1)
internal/db/src/schema/domains.ts (1)
domains(4-36)
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/sections.tsx (2)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
StatusIndicator(9-43)internal/icons/src/icons/arrow-right.tsx (1)
ArrowRight(15-48)
⏰ 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: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (5)
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/detail-section.tsx (3)
5-7: Nullableicon/labelprops: LGTM.Allows richer rows and matches new OpenAPI changes section.
14-21: Early return for icon/label-less rows: LGTM.Good layout handling when only children are provided.
26-31: Conditional rendering reads cleanly.This avoids empty containers when icon/label are absent.
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/sections.tsx (1)
24-26: DetailItem type nullability: LGTM.Matches the new usage patterns.
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx (1)
38-44: Verifyeq(..., project?.liveDeploymentId)withundefined.If
eqserializesundefined, it might produce an invalid condition. If supported, gate the query until the id exists; otherwise, thedomains = []default mitigates UI errors.
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/index.tsx
Show resolved
Hide resolved
apps/dashboard/app/(app)/projects/[projectId]/details/project-details-expandables/sections.tsx
Show resolved
Hide resolved
can we fix this edge case? |
|
@chronark don't mind that part. I have another PR which replaces all the dummy data including git and that domain part, and in that PR that issue is already handled. I'll make it ready by tomorrow. |
|
I'll merge this after this. #4034, because of the issues we had here addressed in that PR |
|
so do we merge this now? @ogzhanolguncu |
|
this one is dead I move code to another PR |
2 participants
What does this PR do?
This PR adds OpenAPI changes indicator to active deployment details to project details UI.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated