Conversation
ogzhanolguncu
requested review from
Flo4604,
chronark,
imeyer,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughFrontend updates the deployments table to derive production status from the active deployment and remove the environment prop from the render function. Backend shifts environment lookup to include project ID and extends DeployRequest with EnvironmentID. SQLC queries and generated code are updated accordingly, replacing workspace+slug lookup with workspace+project+slug. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Dashboard UI
participant API as Ctrl Service (create_deployment.go)
participant DB as DB (sqlc queries)
participant WF as Deploy Workflow
UI->>API: POST /deploy (workspace_id, project_id, env_slug, ...)
API->>DB: FindEnvironmentByProjectIdAndSlug(workspace_id, project_id, slug)
DB-->>API: Environment { id, ... }
API->>WF: DeployRequest{ WorkspaceID, ProjectID, DeploymentID, EnvironmentID, DockerImage, KeyspaceID }
WF-->>API: Ack / workflow started
API-->>UI: 202 Accepted
note over API,WF: New: EnvironmentID included in DeployRequest
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (5)
go/pkg/db/queries/environment_find_by_project_id_and_slug.sql (1)
1-6: Scope fix is correct; add LIMIT and a composite unique index.Query now correctly scopes by workspace + project + slug. To guard against accidental duplicates and improve planner choices, add LIMIT 1 here, and enforce a composite unique index on (workspace_id, project_id, slug). Given your past note that environments isn’t in prod yet, adding the index should be low risk.
Apply LIMIT:
WHERE workspace_id = sqlc.arg(workspace_id) AND project_id = sqlc.arg(project_id) - AND slug = sqlc.arg(slug); + AND slug = sqlc.arg(slug) +LIMIT 1;Recommended DDL (separate migration):
ALTER TABLE environments ADD UNIQUE KEY ux_env_ws_proj_slug (workspace_id, project_id, slug);go/apps/ctrl/services/deployment/deploy_workflow.go (1)
68-74: EnvironmentID added but unused in workflow.You accept environment_id on the request but don’t use it downstream (steps, logs, partition/gateway). If intentional, fine; otherwise consider threading it into logs/steps to aid traceability.
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (2)
61-63: Brittle production detection via array head.activeDeployment.data.at(0) assumes ordering and presence. If the hook doesn’t guarantee index 0 is the active deployment, this can mislabel rows. Prefer an explicit activeDeployment.current (if exposed) or a predicate keyed by deployment.id.
89-93: Label “Production” may be misleading; align with “Current”.You show a “Current” badge but label the env as “Production”. If “current” ≠ prod (e.g., multiple envs), this is confusing. Either keep showing the actual environment, or align the label with the badge.
- {isProduction ? "Production" : "Preview"} + {isProduction ? "Current" : "Preview"}go/apps/ctrl/services/deployment/create_deployment.go (1)
62-69: Improve NotFound message to include project context.Error mentions only workspace; include project ID (and optionally slug) to aid debugging.
- return nil, connect.NewError(connect.CodeNotFound, - fmt.Errorf("environment '%s' not found in workspace '%s'", - req.Msg.GetEnvironmentSlug(), req.Msg.GetWorkspaceId())) + return nil, connect.NewError(connect.CodeNotFound, + fmt.Errorf("environment '%s' not found in workspace '%s' and project '%s'", + req.Msg.GetEnvironmentSlug(), req.Msg.GetWorkspaceId(), req.Msg.GetProjectId()))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx(2 hunks)go/apps/ctrl/services/deployment/create_deployment.go(2 hunks)go/apps/ctrl/services/deployment/deploy_workflow.go(1 hunks)go/pkg/db/environment_find_by_project_id_and_slug.sql_generated.go(1 hunks)go/pkg/db/environment_find_by_workspace_and_slug.sql_generated.go(0 hunks)go/pkg/db/querier_generated.go(1 hunks)go/pkg/db/queries/environment_find_by_project_id_and_slug.sql(1 hunks)go/pkg/db/queries/environment_find_by_workspace_and_slug.sql(0 hunks)
💤 Files with no reviewable changes (2)
- go/pkg/db/queries/environment_find_by_workspace_and_slug.sql
- go/pkg/db/environment_find_by_workspace_and_slug.sql_generated.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3853
File: apps/dashboard/lib/trpc/routers/deploy/project/envs/list.ts:75-87
Timestamp: 2025-08-31T19:30:38.171Z
Learning: In apps/dashboard/lib/trpc/routers/deploy/project/envs/list.ts, the getEnvs procedure currently uses mock data (VARIABLES) and ignores the projectId input parameter. This is intentional temporary behavior - the user ogzhanolguncu indicated they plan to hook this up to the database later.
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.
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3973
File: go/pkg/db/schema.sql:307-308
Timestamp: 2025-09-15T17:40:51.527Z
Learning: The environments table in the Unkey codebase is not being used in production yet, so schema changes to it don't require complex migration sequences to handle existing data or concurrent usage.
📚 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
🧬 Code graph analysis (3)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (1)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/env-status-badge.tsx (1)
EnvStatusBadge(28-41)
go/apps/ctrl/services/deployment/create_deployment.go (1)
go/pkg/db/environment_find_by_project_id_and_slug.sql_generated.go (1)
FindEnvironmentByProjectIdAndSlugParams(21-25)
go/pkg/db/querier_generated.go (2)
go/pkg/partition/db/database.go (1)
DBTX(10-10)go/pkg/db/environment_find_by_project_id_and_slug.sql_generated.go (2)
FindEnvironmentByProjectIdAndSlugParams(21-25)FindEnvironmentByProjectIdAndSlugRow(27-33)
⏰ 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: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (4)
go/apps/ctrl/services/deployment/create_deployment.go (2)
56-60: Good: environment lookup now scoped by project.Using FindEnvironmentByProjectIdAndSlug fixes the cross‑project leakage.
Do you want a small unit test to assert that an environment slug from another project yields NotFound?
150-156: DeployRequest now carries EnvironmentID; confirm orchestration consumers accept the new shape.If this struct is serialized across boundaries (Hydra inputs, audits), confirm no strict schema validators reject the added field.
go/pkg/db/environment_find_by_project_id_and_slug.sql_generated.go (1)
42-53: LGTM: generated method matches SQL and types.Params/Row mapping and scan order are correct; Description as sql.NullString is appropriate.
go/pkg/db/querier_generated.go (1)
192-199: Rename verified — no remaining references to FindEnvironmentByWorkspaceAndSlugNo occurrences of FindEnvironmentByWorkspaceAndSlug were found; only FindEnvironmentByProjectIdAndSlug appears in generated DB files (go/pkg/db/...) and in go/apps/ctrl/services/deployment/create_deployment.go:56.
2 participants
What does this PR do?
This PR fixes env assignment when creating new deploy requests. Previously it was always getting the first environment from the DB regardless of
projectId, so now we also useprojectIdwhen querying environments table.Every deployment is being treated as
previewonly thing that decides whats production isliveDeploymentIdon projects table.Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores