Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughWalkthroughAdds flattened deployment metadata to Project schema and insertion payload. Simplifies project list UI to consume these fields directly. Replaces ORM-based listing with a raw SQL LEFT JOIN query, mapping results (with defaults) into the expanded Project type. Minor prop typing adjustment to allow null commit timestamps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Dashboard ProjectsList
participant API as trpc.deploy.project.list
participant DB as Database
UI->>API: listProjects(workspaceId)
API->>DB: SQL SELECT projects LEFT JOIN deployments, domains
DB-->>API: rows (project + deployment + domain)
rect rgba(200,200,255,0.15)
note right of API: Map rows → Project type<br/>Apply defaults for missing fields
end
API-->>UI: Project[]
UI->>UI: Render ProjectCard with domain, commitTitle,<br/>commitTimestamp, branch, author, regions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
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: 1
🧹 Nitpick comments (8)
apps/dashboard/lib/collections/deploy/projects.ts (1)
14-21: Prefer nullable/empty defaults over “will‑be‑replace‑by‑server” plumbingMake these UI‑flattened fields safe-by-default in the Zod schema so inserts don’t need placeholder strings and new projects render “no deployments” correctly.
- // Flattened deployment fields for UI - commitTitle: z.string(), - branch: z.string(), - author: z.string(), - commitTimestamp: z.number().int().nullable(), - regions: z.array(z.string()), - // Domain field - domain: z.string(), + // Flattened deployment fields for UI + commitTitle: z.string().default(""), + branch: z.string().default(""), + author: z.string().default(""), + commitTimestamp: z.number().int().nullable().default(null), + regions: z.array(z.string()).default([]), + // Domain field + domain: z.string().default(""),Also scan other writers to this collection to ensure they don’t rely on the old required-field behavior.
apps/dashboard/app/(app)/projects/_components/create-project/create-project-dialog.tsx (1)
45-51: Don’t seed fake commit/domain metadata on insertUsing Date.now() and placeholder strings briefly renders a bogus “last commit” on brand‑new projects. Send null/empty so the card shows “No deployments.”
- author: "will-be-replace-by-server", - branch: "will-be-replace-by-server", - commitTimestamp: Date.now(), - commitTitle: "will-be-replace-by-server", - domain: "will-be-replace-by-server", - regions: [], + author: "", + branch: "", + commitTimestamp: null, + commitTitle: "", + domain: "", + regions: [],apps/dashboard/app/(app)/projects/_components/list/projects-card.tsx (2)
93-97: Truthiness check can hide timestamp value 0; compare to null insteadGuard against edge cases and align with prop typing (number | null).
- {commitTimestamp ? ( + {commitTimestamp != null ? ( <TimestampInfo value={commitTimestamp} className="hover:underline whitespace-pre" /> ) : ( <span className="text-xs text-gray-12 truncate max-w-[70px]">No deployments</span> )}
85-91: Avoid dummy “#” linkThis Link goes nowhere and is confusing for keyboard users/AT. Render plain text or a button until a real URL is available.
- <Link - href="#" - className="text-[13px] font-medium text-accent-12 leading-5 min-w-0 truncate cursor-pointer hover:underline" - > - {commitTitle} - </Link> + <span className="text-[13px] font-medium text-accent-12 leading-5 min-w-0 truncate"> + {commitTitle || "No commits"} + </span>apps/dashboard/lib/trpc/routers/deploy/project/list.ts (2)
27-41: Select defaultBranch and createdAt to improve fallbacks and orderingUse project.defaultBranch as a better branch fallback, and order by a non‑null timestamp.
SELECT ${projects.id}, ${projects.name}, ${projects.slug}, ${projects.updatedAt}, + ${projects.createdAt}, ${projects.gitRepositoryUrl}, ${projects.liveDeploymentId}, + ${projects.defaultBranch}, ${deployments.gitCommitMessage}, ${deployments.gitBranch}, ${deployments.gitCommitAuthorName}, ${deployments.gitCommitTimestamp}, ${deployments.runtimeConfig}, ${domains.domain}And update types/mapping accordingly:
type ProjectRow = { id: string; name: string; slug: string; - updated_at: number | null; + updated_at: number | null; + created_at: number | null; git_repository_url: string | null; live_deployment_id: string | null; + default_branch: string | null; git_commit_message: string | null; git_branch: string | null; git_commit_author_name: string | null; git_commit_timestamp: number | null; runtime_config: Deployment["runtimeConfig"] | null; domain: string | null; };
49-49: Order by last activity even when updatedAt is nullSome rows may have null updatedAt. Use COALESCE(updatedAt, createdAt) for stable ordering.
- ORDER BY ${projects.updatedAt} DESC + ORDER BY COALESCE(${projects.updatedAt}, ${projects.createdAt}) DESCapps/dashboard/app/(app)/projects/_components/list/index.tsx (2)
24-40: Skeletons: good UX; consider reflecting expected page sizeIf server later paginates, drive skeleton count from the page size to avoid layout shifts.
49-51: Minor: escape search term visuallyNo XSS risk here, but wrapping the echoed term in
or similar improves scannability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 132f477dc6a11a014a7a63e90f5cba30ffd516be and c25a6bf0c81be754c8daab7f51ba4a619472356d.
📒 Files selected for processing (5)
apps/dashboard/app/(app)/projects/_components/create-project/create-project-dialog.tsx (1 hunks)apps/dashboard/app/(app)/projects/_components/list/index.tsx (4 hunks)apps/dashboard/app/(app)/projects/_components/list/projects-card.tsx (1 hunks)apps/dashboard/lib/collections/deploy/projects.ts (1 hunks)apps/dashboard/lib/trpc/routers/deploy/project/list.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/project/list.tsapps/dashboard/app/(app)/projects/_components/list/index.tsx
📚 Learning: 2025-08-31T19:30:38.171Z
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.
Applied to files:
apps/dashboard/lib/trpc/routers/deploy/project/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)/projects/_components/list/index.tsx
📚 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/_components/list/index.tsx
🧬 Code graph analysis (2)
apps/dashboard/lib/trpc/routers/deploy/project/list.ts (5)
apps/dashboard/lib/collections/deploy/deployments.ts (1)
Deployment (39-39)
apps/dashboard/lib/collections/deploy/projects.ts (2)
projects (41-106)Project (38-38)
internal/db/src/schema/projects.ts (1)
projects (8-31)
internal/db/src/schema/deployments.ts (1)
deployments (9-52)
internal/db/src/schema/domains.ts (1)
domains (4-20)
apps/dashboard/app/(app)/projects/_components/list/index.tsx (3)
apps/dashboard/lib/collections/deploy/projects.ts (1)
projects (41-106)
internal/db/src/schema/projects.ts (1)
projects (8-31)
apps/dashboard/app/(app)/projects/_components/list/projects-card.tsx (1)
ProjectCard (21-115)
⏰ 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 (3)
apps/dashboard/lib/trpc/routers/deploy/project/list.ts (2)
22-26: Nice: server‑side composition removes the client hook violation
Moving the join to TRPC fixes the earlier hook/data‑dependency issue cleanly. Parameterization via drizzle’s sql template is safe.
If you expect many projects, consider pagination (LIMIT/OFFSET or cursor) to bound result sizes. I can sketch a paginated version.
61-66: Don’t fabricate commit metadata; use null/empty defaults
Fake values (“[DUMMY]…”, yesterday’s timestamp, “us-east-1”) mislead the UI. Return null/empty and let the card render “No deployments.”
- commitTitle: row.git_commit_message ?? "[DUMMY] Initial commit",
- branch: row.git_branch ?? "main",
- author: row.git_commit_author_name ?? "[DUMMY] Unknown Author",
- commitTimestamp: row.git_commit_timestamp ?? Date.now() - 86400000,
- regions: row.runtime_config?.regions?.map((r) => r.region) ?? ["us-east-1"],
- domain: row.domain ?? "project-temp.unkey.app",
+ commitTitle: row.git_commit_message ?? "",
+ branch: row.git_branch ?? row.default_branch ?? "main",
+ author: row.git_commit_author_name ?? "",
+ commitTimestamp: row.git_commit_timestamp ?? null,
+ regions: row.runtime_config?.regions?.map((r) => r.region) ?? [],
+ domain: row.domain ?? "",
⛔ Skipped due to learnings
Learnt from: mcstepp
PR: unkeyed/unkey#3662
File: apps/dashboard/lib/trpc/routers/branch/getByName.ts:0-0
Timestamp: 2025-07-28T20:36:36.865Z
Learning: In apps/dashboard/lib/trpc/routers/branch/getByName.ts, mcstepp prefers to keep mock data (gitCommitMessage, buildDuration, lastCommitAuthor, etc.) in the branch procedure during POC phases to demonstrate what the UI would look like with proper schema changes, rather than returning null/undefined values.
apps/dashboard/app/(app)/projects/_components/list/index.tsx (1)
71-105: Card props mapping looks consistent with the new Project shape
Once server returns null/empty defaults (see list.ts), this renders the intended “No deployments”/empty regions states.
Please verify the list doesn’t show duplicate cards when a project has multiple domains. If it does, it’s coming from the LEFT JOIN on domains in list.ts.
| LEFT JOIN ${domains} | ||
| ON ${projects.id} = ${domains.projectId} | ||
| AND ${domains.workspaceId} = ${ctx.workspace.id} |
There was a problem hiding this comment.
Domain join can duplicate rows and pick a non‑active domain
Joining domains by projectId returns 1+ rows per project (multiple domains) and selects an arbitrary domain. Join by live deployment to get the “active” domain; consider a deterministic tie‑break (e.g., prefer custom over wildcard, newest first).
- LEFT JOIN ${domains}
- ON ${projects.id} = ${domains.projectId}
- AND ${domains.workspaceId} = ${ctx.workspace.id}
+ LEFT JOIN ${domains}
+ ON ${deployments.id} IS NOT NULL
+ AND ${domains.deploymentId} = ${deployments.id}
+ AND ${domains.workspaceId} = ${ctx.workspace.id}If you must support project‑level domains as a fallback, pull one deterministically via a subquery (ORDER BY type='custom' DESC, updated_at DESC LIMIT 1). I can draft that if desired.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/dashboard/lib/trpc/routers/deploy/project/list.ts around lines 45-47,
the current LEFT JOIN to domains by projectId can produce multiple rows per
project and return an arbitrary domain; change the query so it joins the
project's active/live deployment's domain (i.e., join through the
live_deployments/live_deployment_id) and use a deterministic tie-break when
multiple domains exist (prefer custom over wildcard, then newest), or, if
keeping a project-level fallback, replace the direct join with a subquery that
selects a single domain per project (ORDER BY (type='custom') DESC, updated_at
DESC LIMIT 1) and join that result instead so each project yields at most one
deterministic domain row.
| branch: row.git_branch ?? "main", | ||
| author: row.git_commit_author_name ?? "[DUMMY] Unknown Author", | ||
| commitTimestamp: row.git_commit_timestamp ?? Date.now() - 86400000, | ||
| regions: row.runtime_config?.regions?.map((r) => r.region) ?? ["us-east-1"], |
There was a problem hiding this comment.
this is bad, the default should absolutely not be some random region. The control plane already create a proper runtimeConfig
There was a problem hiding this comment.
That region and other [DUMMY ] data is just for nicer UI so we can find the stylistic issues on the frontend. Everything starts with [DUMMY ] or hardcoded stuff will be removed before demo.
There was a problem hiding this comment.
the control plane sets the region correctly though, there is no need for default here
| author: row.git_commit_author_name ?? "[DUMMY] Unknown Author", | ||
| commitTimestamp: row.git_commit_timestamp ?? Date.now() - 86400000, | ||
| regions: row.runtime_config?.regions?.map((r) => r.region) ?? ["us-east-1"], | ||
| domain: row.domain ?? "project-temp.unkey.app", |
There was a problem hiding this comment.
this is flat out broken and no longer supports multiple domains per deployment
There was a problem hiding this comment.
For projects listing we only require 1 domain similar to Vercel. If you are saying we should return all the associated data and filter it on the frontend that we can do.
There was a problem hiding this comment.
for projects listing maybe, but then you need a different trpc route to get more details about the deployment or whatever
if we just return dumb lists and join on the frontend, we can reuse the data on many pages
| "Failed to retrieve projects due to an error. If this issue persists, please contact support.", | ||
| }); | ||
| }); | ||
| const result = await db.execute(sql` |
There was a problem hiding this comment.
what's the benefit of doing a raw sql, rather than a nice-to-read drizzle query?
There was a problem hiding this comment.
Domains were lacking in the connections for Project thats probably why I fallback to this and tbh this felt easier, but ifs a problem we can extend drizzle connections and Domains then do a drizzle query
|
We can do better I think. The whole idea of tanstack db is that you can join as you need on the frontend, rather than pulling prejoined data. Also this transforms the data multiple times, setting weird defaults and omitting other fields entirely, making it harder to follow the trail. |
|
I think we should just revert to unpartitioned collections and move on for now |
Since they are now scoped to
I agree but returning fully unaware data(returning data as is) makes UI joins a bit harder to read. Tbh, once it gets big it becomes harder to read than SQL to me eyes at least. So, returning a bit of scoped data then filtering, transforming on the frontend feels easier. And, probably performance wise this could be much faster once we introduce indexes in our SQL like
This shit is 100% temporary. I'll get rid of weird defaults, omits before deploy. Thanks to those I found some issues yesterday. Thats why most of them are marked as TLDR; I'll get rid of |
2 participants
What does this PR do?
This PR fixes hook violation for projects page. Issue was to get the latest commit and active domain for that project we were joining everything on the client side, but then I moved some of the stuff like deployments, domains and environments to projectId scoped structure so joining became impossible. This PR just gets everything from db by joining them properly.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
UI
Performance
Chores