Conversation
ogzhanolguncu
requested review from
Flo4604,
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds empty and skeleton UI states for active deployments and domains, makes deploymentId nullable across hooks and components, introduces loading control via useLiveQuery, tweaks layout/navigation behavior, adds a subtle-bounce keyframe, and adds an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Page
participant LiveQuery as useLiveQuery(projects)
participant ActiveCard as ActiveDeploymentCard
participant LogsHook as useDeploymentLogs
Page->>LiveQuery: subscribe(projects)
LiveQuery-->>Page: { project, isLoading }
Page->>ActiveCard: deploymentId = project?.liveDeploymentId ?? null, isLoading
alt isLoading
ActiveCard->>ActiveCard: render Skeleton
else if !deploymentId
ActiveCard->>ActiveCard: render ActiveDeploymentCardEmpty
else
ActiveCard->>LogsHook: useDeploymentLogs(deploymentId)
LogsHook-->>ActiveCard: logs stream
ActiveCard->>ActiveCard: render active deployment UI
end
sequenceDiagram
autonumber
participant Page
participant DomainsQuery as useDomainsQuery
participant DomainsUI as DomainsSection
Page->>DomainsQuery: fetch domains
DomainsQuery-->>Page: { domains, isLoading }
alt isLoading
Page->>DomainsUI: render DomainRowSkeleton (n rows)
else if domains.length > 0
Page->>DomainsUI: render DomainRow list
else
Page->>DomainsUI: render DomainRowEmpty
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
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! 🙏 |
ogzhanolguncu
changed the title
![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 (2)
apps/dashboard/app/(app)/projects/[projectId]/layout.tsx (1)
61-77: Tooltip won’t show on a disabled button. Wrap with a non-disabled element.Disabled elements don’t receive pointer events; the tooltip won’t trigger. Wrap the Button to preserve the tooltip while keeping the button disabled.
Apply this diff:
- <InfoTooltip - asChild - content={getTooltipContent()} - position={{ - side: "bottom", - align: "end", - }} - > - <Button - variant="ghost" - className="size-7" - disabled={!liveDeploymentId} - onClick={() => setIsDetailsOpen(!isDetailsOpen)} - > - <DoubleChevronLeft size="lg-medium" className="text-gray-13" /> - </Button> - </InfoTooltip> + <InfoTooltip + asChild + content={getTooltipContent()} + position={{ side: "bottom", align: "end" }} + > + <span className="inline-flex" aria-disabled={!liveDeploymentId}> + <Button + variant="ghost" + className="size-7" + disabled={!liveDeploymentId} + onClick={() => setIsDetailsOpen(!isDetailsOpen)} + > + <DoubleChevronLeft size="lg-medium" className="text-gray-13" /> + </Button> + </span> + </InfoTooltip>apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/hooks/use-deployment-logs.tsx (1)
103-109: Fix runtime error: optionallevelcan cause.includeson undefined.
log.level?.toLowerCase().includes(...)will call.includesonundefined. Coalesce or chain to avoid exceptions.Apply this diff:
- filtered = filtered.filter( - (log) => - log.message.toLowerCase().includes(searchTerm.toLowerCase()) || - log.timestamp.includes(searchTerm) || - log.level?.toLowerCase().includes(searchTerm.toLowerCase()), - ); + const term = searchTerm.toLowerCase(); + filtered = filtered.filter( + (log) => + log.message.toLowerCase().includes(term) || + log.timestamp.includes(searchTerm) || + (log.level ?? "").toLowerCase().includes(term), + );
🧹 Nitpick comments (8)
apps/dashboard/styles/tailwind/tailwind.css (1)
116-124: Expose the keyframes via a utility class (currently unused).Define a utility so components can use
animate-subtle-bouncewithout inline styles or Tailwind config changes.Apply this diff to add a utility:
@keyframes subtle-bounce { 0%, 100% { transform: translateY(0px); } 50% { transform: translateY(-8px); } } +@layer utilities { + .animate-subtle-bounce { + animation: subtle-bounce 2s ease-in-out infinite; + } +}apps/dashboard/app/(app)/projects/[projectId]/details/domain-row.tsx (1)
52-55: Prefer using the new subtle-bounce animation utility over inlinestyle.Switch to a class for consistency and to avoid inline styles.
Apply this diff:
- <Link4 - className="text-gray-9 size-6 group-hover:text-gray-11 transition-all duration-200 animate-pulse" - style={{ animationDuration: "2s" }} - /> + <Link4 + className="text-gray-9 size-6 group-hover:text-gray-11 transition-all duration-200 animate-subtle-bounce" + />apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (2)
75-80: Guard the equality whendeploymentIdis null to avoid invalid comparisons.Coalesce to a non-matching string to keep the query valid until an ID exists.
Apply this diff:
- const { data, isLoading } = useLiveQuery( - (q) => - q - .from({ deployment: collections.deployments }) - .where(({ deployment }) => eq(deployment.id, deploymentId)), - [deploymentId], - ); + const { data, isLoading } = useLiveQuery( + (q) => + q + .from({ deployment: collections.deployments }) + .where(({ deployment }) => eq(deployment.id, deploymentId ?? "")), + [deploymentId], + );If
eqsupportsnullsafely, feel free to keep it as-is; otherwise, this avoids throwing and yields an empty result.
118-123: Avoid forcing success text color on all statuses.
className="text-successA-11"conflicts with error/warning variants.Apply this diff:
- <Badge variant={statusConfig.variant} className="text-successA-11 font-medium"> + <Badge variant={statusConfig.variant} className="font-medium">apps/dashboard/app/(app)/projects/[projectId]/page.tsx (3)
2-3: Usecollections.projectsfrom context for consistency.Avoid mixing
collectionandcollections; keeps a single query client/source of truth.Apply this diff:
-import { collection } from "@/lib/collections"; import { eq, useLiveQuery } from "@tanstack/react-db";
15-17: Align projects query with context collections.Use the same
collectionssource you already consume below.Apply this diff:
- const projects = useLiveQuery((q) => - q.from({ project: collection.projects }).where(({ project }) => eq(project.id, projectId)), - ); + const projects = useLiveQuery((q) => + q.from({ project: collections.projects }).where(({ project }) => eq(project.id, projectId)), + );
21-26: Coalesce nullableliveDeploymentIdin the domains filter to avoid invalid comparisons.Prevents passing
undefinedtoeq, which may be unsupported.Apply this diff:
const { data: domains, isLoading: isDomainsLoading } = useLiveQuery( (q) => q .from({ domain: collections.domains }) - .where(({ domain }) => eq(domain.deploymentId, project?.liveDeploymentId)), + .where(({ domain }) => eq(domain.deploymentId, project?.liveDeploymentId ?? "")), [project?.liveDeploymentId], );If
eqgracefully handlesundefined/null, this is optional; otherwise, this yields a safe empty result until an ID exists.apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/active-deployment-card-empty.tsx (1)
23-55: Hide decorative icons from assistive tech.All three icons here (Cloud, Plus, CodeBranch) are purely ornamental; right now they’ll surface to screen readers as nameless graphics, which adds noise to the empty state. Mark them as hidden so the meaningful copy is what gets announced.
Apply:
- <Cloud + <Cloud + aria-hidden="true" className="text-gray-9 size-6 group-hover:text-gray-11 transition-all duration-200 animate-pulse" style={{ animationDuration: "2s" }} /> ... - <Button onClick={onCreateDeployment} size="sm" className="mt-2 group/button"> - <Plus className="size-4 mr-2 group-hover/button:rotate-90 transition-transform duration-200" /> + <Button onClick={onCreateDeployment} size="sm" className="mt-2 group/button"> + <Plus + aria-hidden="true" + className="size-4 mr-2 group-hover/button:rotate-90 transition-transform duration-200" + /> Create deployment </Button> ... - <CodeBranch className="text-gray-7 size-3" /> + <CodeBranch aria-hidden="true" className="text-gray-7 size-3" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/active-deployment-card-empty.tsx(1 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/hooks/use-deployment-logs.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx(3 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/domain-row.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/layout.tsx(5 hunks)apps/dashboard/app/(app)/projects/[projectId]/navigations/project-navigation.tsx(1 hunks)apps/dashboard/app/(app)/projects/[projectId]/page.tsx(3 hunks)apps/dashboard/styles/tailwind/tailwind.css(1 hunks)internal/icons/src/props.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-28T19:42:37.047Z
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/app/(app)/projects/page.tsx:74-81
Timestamp: 2025-07-28T19:42:37.047Z
Learning: In apps/dashboard/app/(app)/projects/page.tsx, the user mcstepp prefers to keep placeholder functions like generateSlug inline during POC/demonstration phases rather than extracting them to utility modules, with plans to refactor later when the feature matures beyond the proof-of-concept stage.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/page.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)/projects/[projectId]/layout.tsx
📚 Learning: 2025-09-19T12:43:46.746Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#4000
File: apps/dashboard/lib/collections/index.ts:92-99
Timestamp: 2025-09-19T12:43:46.746Z
Learning: In the dashboard codebase, useLiveQuery hooks have mixed invalidation behavior - some automatically invalidate when collections change, while others are memoized and require explicit collection properties in their deps arrays. Not all useLiveQuery calls need collection dependencies added.
Applied to files:
apps/dashboard/app/(app)/projects/[projectId]/layout.tsx
🧬 Code graph analysis (7)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/active-deployment-card-empty.tsx (3)
internal/icons/src/icons/cloud.tsx (1)
Cloud(15-38)internal/icons/src/icons/plus.tsx (1)
Plus(16-52)internal/icons/src/icons/code-branch.tsx (1)
CodeBranch(15-41)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/hooks/use-deployment-logs.tsx (1)
apps/dashboard/lib/trpc/server.ts (1)
trpc(7-14)
apps/dashboard/app/(app)/projects/[projectId]/navigations/project-navigation.tsx (2)
internal/icons/src/icons/refresh-3.tsx (1)
Refresh3(15-46)apps/dashboard/app/(app)/projects/_components/list/repo-display.tsx (1)
RepoDisplay(12-34)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (3)
apps/dashboard/app/(app)/projects/[projectId]/layout-provider.tsx (1)
useProjectLayout(13-19)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/skeleton.tsx (1)
ActiveDeploymentCardSkeleton(6-66)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/active-deployment-card-empty.tsx (1)
ActiveDeploymentCardEmpty(10-60)
apps/dashboard/app/(app)/projects/[projectId]/details/domain-row.tsx (2)
internal/icons/src/icons/link-4.tsx (1)
Link4(16-45)internal/icons/src/icons/share-up-right.tsx (1)
ShareUpRight(15-56)
apps/dashboard/app/(app)/projects/[projectId]/page.tsx (3)
internal/db/src/schema/domains.ts (1)
domains(4-36)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (1)
ActiveDeploymentCard(72-278)apps/dashboard/app/(app)/projects/[projectId]/details/domain-row.tsx (3)
DomainRowSkeleton(25-39)DomainRow(9-23)DomainRowEmpty(41-67)
apps/dashboard/app/(app)/projects/[projectId]/layout.tsx (2)
apps/dashboard/lib/collections/deploy/projects.ts (1)
projects(42-108)internal/db/src/schema/projects.ts (1)
projects(8-31)
⏰ 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). (6)
- GitHub Check: Test Packages / Test
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Vercel Agent Review
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/dashboard/app/(app)/projects/[projectId]/navigations/project-navigation.tsx (1)
78-87: Nice fragment swap to accommodate the separator.Returning a fragment keeps the flex layout clean while letting the repo pill and separator sit side‑by‑side without an extra wrapper. The updated styling on
RepoDisplayreads well too.internal/icons/src/props.ts (1)
36-37: LGTM: Addingstyleto IconProps is appropriate.Enables inline styles to flow to the SVG via props spreading. No concerns.
apps/dashboard/app/(app)/projects/[projectId]/page.tsx (2)
36-42: Nice: active deployment always rendered with empty/loading states.Clear UX progression: skeleton → empty → data.
49-58: Good domain list state handling.Skeletons while loading, rows when present, and an empty state fallback are well-structured.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
...p/(app)/projects/[projectId]/details/active-deployment-card/active-deployment-card-empty.tsx
Outdated
Show resolved
Hide resolved
ogzhanolguncu
changed the title
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]/details/active-deployment-card/index.tsx (1)
127-133: Badge text color is hard-coded to successThe text color remains success for warning/error variants.
- <Badge variant={statusConfig.variant} className="text-successA-11 font-medium"> + <Badge + variant={statusConfig.variant} + className={cn( + "font-medium", + statusConfig.variant === "success" + ? "text-successA-11" + : statusConfig.variant === "warning" + ? "text-warningA-11" + : "text-errorA-11", + )} + >
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/hooks/use-deployment-logs.tsx (1)
65-71: Avoid passing a sentinel "" to the TRPC query inputRely on enabled + a type assertion instead of sending an empty string; also disable refetch-on-focus for parity with gateway logs.
- // without this check TS yells at us - deploymentId: deploymentId ?? "", + // safe: guarded by `enabled` + deploymentId: deploymentId as string, }, { - enabled: showBuildSteps && isExpanded && Boolean(deploymentId), + enabled: showBuildSteps && isExpanded && Boolean(deploymentId), refetchInterval: BUILD_STEPS_REFETCH_INTERVAL, + refetchOnWindowFocus: false, },apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (2)
77-83: Null passed to eq(): verify library semanticsPassing a possible null into eq(...) can be unsafe depending on @tanstack/react-db’s typing/behavior (it may generate
= nullor error). If eq doesn’t accept null, coerce to a sentinel or gate the comparison.If eq rejects null, minimally:
- .where(({ deployment }) => eq(deployment.id, deploymentId)), + .where(({ deployment }) => eq(deployment.id, deploymentId ?? "__none__")),Or, if the API supports it, prefer an “always false” predicate when deploymentId is null.
255-258: Use a stable key for log rowsIndex-based keys cause unnecessary re-renders on filter/sort. Use log.id.
- {filteredLogs.map((log, index) => ( + {filteredLogs.map((log) => ( <div - key={`${log.message}-${index}`} + key={log.id}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/hooks/use-deployment-logs.tsx(2 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx(3 hunks)apps/dashboard/app/(app)/projects/[projectId]/details/domain-row.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/app/(app)/projects/[projectId]/details/domain-row.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (3)
apps/dashboard/app/(app)/projects/[projectId]/layout-provider.tsx (1)
useProjectLayout(13-19)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/skeleton.tsx (1)
ActiveDeploymentCardSkeleton(6-66)apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/active-deployment-card-empty.tsx (1)
ActiveDeploymentCardEmpty(10-48)
🔇 Additional comments (4)
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/hooks/use-deployment-logs.tsx (1)
25-28: Prop now allows null deploymentId — good changeThis aligns the hook with the new empty-state flow.
apps/dashboard/app/(app)/projects/[projectId]/details/active-deployment-card/index.tsx (3)
21-21: Empty-state import looks goodImport location and usage are consistent.
72-73: Prop accepts null deploymentId — goodMatches upstream nullable flow.
107-113: Loading → Empty fallback flow is correctSkeleton while live query loads, then empty UI when no deployment.
|
@Flo4604 @perkinsjr @mcstepp we need one more please |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (09/25/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
What does this PR do?
This PR adds proper empty state for project details. Project Details UI now properly transition between states.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated