Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughExtracts a reusable PulseIndicator from status-indicator. Introduces a new DeploymentSelect component under openapi-diff/components, removes the old one, and updates page import. Adds hasOpenApiSpec to deployment schema and computes it in TRPC by deriving from openapiSpec, which is now selected but stripped from the response. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant DS as DeploymentSelect (UI)
participant TRPC as deployments.list (TRPC)
participant DB as DB
U->>DS: Open OpenAPI diff page
DS->>TRPC: Fetch deployments
TRPC->>DB: SELECT ..., openapiSpec
DB-->>TRPC: Rows (incl. openapiSpec)
TRPC-->>DS: Deployments with hasOpenApiSpec (openapiSpec removed)
note over DS: Disable items lacking spec or matching disabledDeploymentId<br/>Highlight liveDeploymentId with PulseIndicator
U->>DS: Choose deployment
DS-->>U: Update selection
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)
✨ 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! 🙏 |
![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
🧹 Nitpick comments (1)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx (1)
58-58: Standardize date formatting across the dashboard
Observed multipleformat()patterns in the dashboard code:
- apps/.../openapi-diff/components/deployment-select.tsx:
format(deployment.createdAt, "MMM d, h:mm a")- apps/.../_components/create-key/expiration-setup.tsx:
format(date, "MMM d, yyyy 'at' h:mm a")- apps/.../logs/queries/utils.ts:
format(Number(value), "MMM d HH:mm:ss")- apps/.../chart/utils/format-timestamp.ts:
format(localDate, "MMM d, HH:mm"),"HH:mm:ss", etc.Pick one canonical pattern (for example,
"MMM d, h:mm a") and centralize it via a shared date-format helper for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/status-indicator.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/deployment-select.tsx(0 hunks)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx(1 hunks)apps/dashboard/lib/collections/deploy/deployments.ts(1 hunks)apps/dashboard/lib/trpc/routers/deploy/deployment/list.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/deployment-select.tsx
🧰 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/lib/trpc/routers/deploy/deployment/list.ts
📚 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/lib/trpc/routers/deploy/deployment/list.ts
📚 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/page.tsx
🧬 Code graph analysis (2)
apps/dashboard/lib/trpc/routers/deploy/deployment/list.ts (1)
internal/db/src/schema/deployments.ts (1)
deployments(9-54)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx (4)
apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/layout-provider.tsx (1)
useProjectLayout(16-22)internal/db/src/schema/deployments.ts (1)
deployments(9-54)apps/dashboard/lib/shorten-id.ts (1)
shortenId(5-54)apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
PulseIndicator(77-105)
⏰ 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 Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
🔇 Additional comments (7)
apps/dashboard/lib/collections/deploy/deployments.ts (1)
17-18: LGTM! Schema addition aligns with backend data.The
hasOpenApiSpecfield correctly extends the Deployment schema to support the new deployment selection UX. This flag is computed by the TRPC endpoint inlist.ts(line 38) from the presence ofopenapiSpec.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/details/active-deployment-card/status-indicator.tsx (1)
71-105: LGTM! Clean extraction of reusable component.The
PulseIndicatorcomponent properly encapsulates the pulsing animation logic with sensible defaults and flexible props for different status colors. The original animation timing and visual behavior are preserved.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/page.tsx (1)
13-13: LGTM! Import path correctly updated.The import path properly reflects the component relocation to the
componentssubdirectory.apps/dashboard/lib/trpc/routers/deploy/deployment/list.ts (2)
27-27: LGTM! Efficient data retrieval.Adding
openapiSpecto the query columns enables computing thehasOpenApiSpecflag while keeping the actual spec content out of the response payload.
33-38: LGTM! Clean computed field pattern.The destructuring and mapping correctly computes
hasOpenApiSpecfrom the presence ofopenapiSpecwhile excluding the potentially large spec text from the response. This keeps the API response lean while providing the UI with the information it needs.apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/openapi-diff/components/deployment-select.tsx (2)
29-107: LGTM! Well-structured component with clear UX patterns.The
DeploymentSelectcomponent correctly implements the PR objectives:
- Disables deployments without OpenAPI specs using
hasOpenApiSpec- Prevents selecting the same deployment twice via
disabledDeploymentId- Provides contextual tooltips for disabled states
- Indicates live deployments with
PulseIndicator- Handles loading and empty states appropriately
The component logic is sound and the data flow from backend through TRPC to this UI component is correct.
62-78: Consider potential DOM nesting issue with InfoTooltip + SelectItem.The
InfoTooltipcomponent withasChildprop is wrapping aSelectItem. This pattern relies on proper slot composition in Radix UI primitives. Verify that this doesn't cause DOM nesting issues or accessibility warnings, especially sinceSelectItemitself may render as a complex interactive element.If you encounter any console warnings or accessibility issues during testing, consider restructuring the tooltip to wrap only the content div (lines 79-84) rather than the entire SelectItem.
2 participants
What does this PR do?
This PR;
deploymentIdand timestamp for diffing selecting items.Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated