Open
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.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces a GlowIcon component and refactors deployment configuration with modularized steps, enhances environment context with isSaving tracking, adds a PendingRedeployBanner component, and implements save-state monitoring hooks via an external store. Changes
Sequence DiagramsequenceDiagram
participant User
participant PendingRedeployBanner
participant TRPC
participant Cache
participant Router
User->>PendingRedeployBanner: Click Redeploy Button
activate PendingRedeployBanner
PendingRedeployBanner->>TRPC: deployments.redeploy mutation
activate TRPC
TRPC-->>PendingRedeployBanner: onSuccess (newDeploymentId)
deactivate TRPC
PendingRedeployBanner->>Cache: invalidate deployments cache
PendingRedeployBanner->>Router: navigate to new deployment
deactivate PendingRedeployBanner
Router-->>User: Display deployment page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/components/deployment-domains-card.tsx (1)
20-24:⚠️ Potential issue | 🟡 MinorCoerce the optional
glowprop before passing it toGlowIcon.
GlowIcontreatsundefinedastrue, but this component still treats an omittedglowprop as false foriconClassName. That changes the default rendering from a plain icon to a pulsing one whenever callers leaveglowunset.💡 Minimal fix
<GlowIcon icon={<Cube iconSize="md-medium" className="size-[18px]" />} - glow={glow} + glow={Boolean(glow)} className="w-full h-full" />Also applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/components/deployment-domains-card.tsx around lines 20 - 24, The component DeploymentDomainsCard treats an omitted glow prop differently from GlowIcon (GlowIcon treats undefined as true), so coerce the optional glow into an explicit boolean before use (e.g., const showGlow = glow !== false) and use that showGlow when calling GlowIcon and when computing iconClassName so both places have the same default behavior; update all occurrences in the component (including the other block referenced around lines 63-69) to reference showGlow instead of the raw glow prop.
🧹 Nitpick comments (2)
web/apps/dashboard/lib/trpc/routers/deploy/project/create.ts (1)
160-173: Guard the regional settings insert whenregionsis empty.Drizzle ORM throws with
'values() must be called with at least one value'when.values([])is passed. This code will fail project creation if the regions table is unseeded, since[prodEnvId, previewEnvId].flatMap(...)produces an empty array when regions is empty. Add a guard to avoid that failure mode.Suggested minimal patch
const regions = await tx.query.regions.findMany({ columns: { id: true } }); - await tx.insert(schema.appRegionalSettings).values( - [prodEnvId, previewEnvId].flatMap((environmentId) => - regions.map((r) => ({ - workspaceId: ctx.workspace.id, - appId, - environmentId, - regionId: r.id, - replicas: 1, - createdAt: Date.now(), - updatedAt: Date.now(), - })), - ), - ); + const regionalSettings = [prodEnvId, previewEnvId].flatMap((environmentId) => + regions.map((r) => ({ + workspaceId: ctx.workspace.id, + appId, + environmentId, + regionId: r.id, + replicas: 1, + createdAt: Date.now(), + updatedAt: Date.now(), + })), + ); + + if (regionalSettings.length > 0) { + await tx.insert(schema.appRegionalSettings).values(regionalSettings); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/lib/trpc/routers/deploy/project/create.ts` around lines 160 - 173, The insert into appRegionalSettings calls .values(...) with a flattened array from regions and will throw if regions is empty; to fix, build the payload array (e.g., map over regions for each of prodEnvId and previewEnvId) and only call tx.insert(schema.appRegionalSettings).values(payload) when payload.length > 0 (guarding against regions.length === 0), referencing the existing variables regions, prodEnvId, previewEnvId, ctx.workspace.id and appId so no insert is attempted with an empty array.web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/components/glow-icon.tsx (1)
24-30: Minor: Redundant ternary branches.The
glowVisiblelogic has identical outcomes forglow === truein bothtransitionbranches ("animate-pulse opacity-20"). This could be simplified, but the current structure may be intentional for future differentiation.♻️ Optional simplification
const glowVisible = transition - ? glow - ? "animate-pulse opacity-20" - : "opacity-0 transition-opacity duration-300" - : glow - ? "animate-pulse opacity-20" - : "hidden"; + ? glow + ? "animate-pulse opacity-20" + : "opacity-0 transition-opacity duration-300" + : glow + ? "animate-pulse opacity-20" + : "hidden";If transition behavior should differ in the future, the current structure is fine. Otherwise:
- const glowVisible = transition - ? glow - ? "animate-pulse opacity-20" - : "opacity-0 transition-opacity duration-300" - : glow - ? "animate-pulse opacity-20" - : "hidden"; + const glowVisible = glow + ? "animate-pulse opacity-20" + : transition + ? "opacity-0 transition-opacity duration-300" + : "hidden";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/components/glow-icon.tsx around lines 24 - 30, The conditional for glowVisible currently repeats the same branch for glow === true; simplify by checking glow first: if glow is true return "animate-pulse opacity-20", otherwise return transition ? "opacity-0 transition-opacity duration-300" : "hidden". Update the glowVisible assignment (the variable named glowVisible that uses transition and glow) to use this simplified logic so behavior is unchanged but redundant ternary branches are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/pending-redeploy-banner.tsx:
- Around line 35-45: The onSuccess handler for the redeploy mutation (redeploy
in trpc.deploy.deployment.redeploy.useMutation) currently calls
queryClient.invalidateQueries(...).then(() => router.push(...)) but does not
return that Promise, so the mutation becomes non-pending before navigation
starts; fix by returning the Promise from onSuccess (make the onSuccess async
and await/return queryClient.invalidateQueries(...) before calling router.push
or simply return queryClient.invalidateQueries(...).then(...)) and ensure you
reference currentDeploymentRef.current and router.push to perform navigation
after the awaited invalidation.
In `@web/apps/dashboard/lib/collections/deploy/environment-settings.ts`:
- Around line 53-58: _wrap each subscriber invocation in _notifySaveSubscribers
so that callbacks from _saveSubscribers are executed inside a try/catch that
prevents exceptions from bubbling; log or ignore the error but do not rethrow,
ensuring the caller (dispatchSettingsMutations) can always reach its finally
block and decrement _pendingSaves; apply the same defensive try/catch pattern to
the other subscriber loop mentioned (the saved-listener loop around lines
303-314) so listener exceptions cannot convert a successful save into a rejected
update or leave _pendingSaves stuck.
---
Outside diff comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/components/deployment-domains-card.tsx:
- Around line 20-24: The component DeploymentDomainsCard treats an omitted glow
prop differently from GlowIcon (GlowIcon treats undefined as true), so coerce
the optional glow into an explicit boolean before use (e.g., const showGlow =
glow !== false) and use that showGlow when calling GlowIcon and when computing
iconClassName so both places have the same default behavior; update all
occurrences in the component (including the other block referenced around lines
63-69) to reference showGlow instead of the raw glow prop.
---
Nitpick comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/components/glow-icon.tsx:
- Around line 24-30: The conditional for glowVisible currently repeats the same
branch for glow === true; simplify by checking glow first: if glow is true
return "animate-pulse opacity-20", otherwise return transition ? "opacity-0
transition-opacity duration-300" : "hidden". Update the glowVisible assignment
(the variable named glowVisible that uses transition and glow) to use this
simplified logic so behavior is unchanged but redundant ternary branches are
removed.
In `@web/apps/dashboard/lib/trpc/routers/deploy/project/create.ts`:
- Around line 160-173: The insert into appRegionalSettings calls .values(...)
with a flattened array from regions and will throw if regions is empty; to fix,
build the payload array (e.g., map over regions for each of prodEnvId and
previewEnvId) and only call
tx.insert(schema.appRegionalSettings).values(payload) when payload.length > 0
(guarding against regions.length === 0), referencing the existing variables
regions, prodEnvId, previewEnvId, ctx.workspace.id and appId so no insert is
attempted with an empty array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6585f684-9a59-4e41-b519-4b011022b1e3
📒 Files selected for processing (16)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/(deployment-progress)/deployment-step.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/components/table/components/actions/redeploy-dialog.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/environment-provider.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/page.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/pending-redeploy-banner.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/components/deployment-domains-card.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/components/glow-icon.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment/content.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment/environment-inner.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment/environment-provider.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment/fallback.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment/index.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/onboarding-environment-provider.tsxweb/apps/dashboard/lib/collections/deploy/environment-settings.tsweb/apps/dashboard/lib/trpc/routers/deploy/project/create.ts
💤 Files with no reviewable changes (2)
- web/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/configure-deployment.tsx
- web/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/onboarding-environment-provider.tsx
...p/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/pending-redeploy-banner.tsx
Outdated
Show resolved
Hide resolved
web/apps/dashboard/lib/collections/deploy/environment-settings.ts
Outdated
Show resolved
Hide resolved
chronark
approved these changes
Mar 17, 2026
Collaborator
|
@mcstepp is that emoji supposed to mean it's good? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR: