fix: domains count and date time selection text#4001
Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughReplaces the hardcoded "Last 12 hours" default in the deployment time-range UI with an internal Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as DeploymentListDatetime UI
participant Filters as Filters Store/Props
participant State as Local Title State
Note over UI,Filters: On mount / when filters change
Filters->>UI: emit filters[]
UI->>State: evaluate filters for time-related entries
alt time filter(s) present
UI->>State: build timeValues (startTime/endTime/since) and set title accordingly
else no time filter present
UI->>State: set title to "Select Time Range"
end
State-->>UI: updated title -> UI re-renders button label/style
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
...ts/[projectId]/deployments/components/controls/components/deployment-list-datetime/index.tsx
Show resolved
Hide resolved
![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 (1)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-datetime/index.tsx (1)
79-82: Replace remaining "Last 12 hours" literals with the shared constant and stop using string-equality checks.Update usages to reference the constant in apps/dashboard/components/logs/datetime/constants.ts (instead of comparing against the display string).
- apps/dashboard/components/logs/datetime/constants.ts:49
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/index.tsx:14, 77
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/controls/components/logs-datetime/index.tsx:14, 77
- apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx:14, 77
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/controls/components/logs-datetime/index.tsx:15, 81
- apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/index.tsx:14, 77
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/controls/components/logs-datetime/index.tsx:15, 81
- apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-datetime/index.tsx:81
- apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx:14, 77
- apps/dashboard/app/(app)/apis/_components/controls/components/logs-datetime/index.tsx:14, 77
🧹 Nitpick comments (6)
apps/dashboard/lib/trpc/routers/deploy/domains/list.ts (2)
22-24: Deterministic “last 3” with tie‑breakerIf multiple domains share the same createdAt, ordering can be unstable. Add a secondary sort (e.g., id) to keep results deterministic.
- orderBy: (table, { desc }) => desc(table.createdAt), + orderBy: (table, { desc }) => [desc(table.createdAt), desc(table.id)],
22-24: Consider composite index for this query patternFilter by (workspaceId, projectId) and order by createdAt DESC is a perfect fit for a composite index to keep this LIMIT 3 query fast at scale.
Example (SQL; adjust names to your schema/migration tool):
CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_domains_ws_proj_createdat ON domains (workspace_id, project_id, created_at DESC);apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-datetime/index.tsx (4)
79-82: Update styling logic to new default labelThe class toggle still references "Last 12 hours", so the button appears active even on the new default. Tie styles to TITLE_EMPTY_DEFAULT.
- "group-data-[state=open]:bg-gray-4 px-2 rounded-lg", - title ? "" : "opacity-50", - title !== "Last 12 hours" ? "bg-gray-4" : "", + "group-data-[state=open]:bg-gray-4 px-2 rounded-lg", + title === TITLE_EMPTY_DEFAULT ? "opacity-50" : "", + title && title !== TITLE_EMPTY_DEFAULT ? "bg-gray-4" : "",
10-10: Narrow state type; title no longer nullSince the initial value is non-null and you always set strings, simplify the state type.
- const [title, setTitle] = useState<string | null>(TITLE_EMPTY_DEFAULT); + const [title, setTitle] = useState<string>(TITLE_EMPTY_DEFAULT);
14-20: Simplify effect and avoid early returnsUse Array.some for clarity and to make intent explicit.
- for (const filter of filters) { - if (["startTime", "endTime", "since"].includes(filter.field)) { - return; - } - } - setTitle(TITLE_EMPTY_DEFAULT); + const hasTime = filters.some((f) => + ["startTime", "endTime", "since"].includes(f.field), + ); + if (!hasTime) setTitle(TITLE_EMPTY_DEFAULT);
71-71: Remove unreachable fallbackWith non-null title, the nullish coalesce is unnecessary.
- initialTitle={title ?? ""} + initialTitle={title}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-datetime/index.tsx(1 hunks)apps/dashboard/lib/trpc/routers/deploy/domains/list.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/domains/list.ts
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-datetime/index.tsx (1)
apps/dashboard/app/(app)/projects/[projectId]/deployments/hooks/use-filters.ts (1)
useFilters(33-127)
⏰ 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). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Vade Review
- GitHub Check: Analyze (javascript-typescript)
|
don’t revert it those are 2 separate things |
|
show me all domains, where domain.deploymentId == project.liveDeploymentId |
|
shouldn't we suppose to show all the domains related to that deployment anyway which includes liveDeployentId? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/dashboard/lib/trpc/routers/deploy/domains/list.ts (3)
16-22: Return shape LGTM; consider server-side filtering by live deployment.Selecting
deploymentIdis good. To reduce payload and align with the “show domains for the live deployment” guidance, accept an optionaldeploymentIdin the input and include it in thewhereclause when provided.Apply this diff within the shown range to add the conditional filter:
columns: { id: true, domain: true, projectId: true, - deploymentId: true, + deploymentId: true, type: true, },And outside this hunk, update the input and where (example):
// change input .input(z.object({ projectId: z.string(), deploymentId: z.string().optional() })) // change where .where((table, { eq, and }) => input.deploymentId ? and( eq(table.workspaceId, ctx.workspace.id), eq(table.projectId, input.projectId), eq(table.deploymentId, input.deploymentId), ) : and(eq(table.workspaceId, ctx.workspace.id), eq(table.projectId, input.projectId)), )
24-31: Prefer structured error context.Add minimal context (workspaceId, projectId) to the log to aid debugging; keep user-facing message unchanged.
- .catch((error) => { - console.error("Error querying domains:", error); + .catch((error) => { + console.error("Error querying domains", { + error, + workspaceId: ctx.workspace.id, + projectId: input.projectId, + });
16-22: Add DB index on deployment_id for anticipated filter.If we start filtering by
deploymentId, add an index to avoid full scans on large domain sets.apps/dashboard/app/(app)/projects/[projectId]/page.tsx (2)
21-27: Guard against undefined in equality filter.When
project?.liveDeploymentIdis undefined, passing it toeq()may yield no matches or throw depending on the query builder. Safer to short‑circuit to an empty result until the ID is known.- const domains = useLiveQuery( - (q) => - q - .from({ domain: collections.domains }) - .where(({ domain }) => eq(domain.deploymentId, project?.liveDeploymentId)), - [project?.liveDeploymentId], - ); + const liveDeploymentId = project?.liveDeploymentId; + const domains = useLiveQuery( + (q) => + liveDeploymentId + ? q.from({ domain: collections.domains }).where(({ domain }) => eq(domain.deploymentId, liveDeploymentId)) + : q.from({ domain: collections.domains }).where(() => false), + [liveDeploymentId], + );If
where(() => false)isn’t supported in your query builder, render an empty state when!liveDeploymentIdand skip the query.
63-66: Optional: empty state for no domains.If there’s no live deployment or no matching domains, show an Empty state instead of a blank list for better UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/app/(app)/projects/[projectId]/page.tsx(1 hunks)apps/dashboard/lib/collections/deploy/domains.ts(1 hunks)apps/dashboard/lib/trpc/routers/deploy/domains/list.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/domains/list.ts
🧬 Code graph analysis (1)
apps/dashboard/app/(app)/projects/[projectId]/page.tsx (1)
internal/db/src/schema/domains.ts (1)
domains(4-20)
⏰ 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). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Vade Review
🔇 Additional comments (1)
apps/dashboard/lib/collections/deploy/domains.ts (1)
7-13: Schema update LGTM; aligns with DB.
deploymentId: z.string().nullable()matches the DB column being nullable. No further changes needed here.
* feat: add init lgos * feat: fix prefixes and filters * fix: add animated logdetails * refactor: get rid of duplicated components * fix: colors * refactor: get rid of unsued * refactor: use common component * fix: live switch * fix: table read source * refactor: use common schema for logs * refactor: use same filters * refactor: make logs denser * fix: make it denser * fix: add missing hostname to table * refactor: table columns and text contrasts * fix: memo issue and add gateway to logs to deployments * fix: color inconsistency * fix: test and add tooltip * fix: border-color * fix: missing params * fix: truncate long req and resp body * fix: comments * fix: props * fix: conflict * fix: small ui issue * fix: make rollback and promote consistent * fix: remove all animated props --------- Co-authored-by: Andreas Thomas <dev@chronark.com>
2 participants
What does this PR do?
This PR only fetches last 3 domain from domains table and reverts dateTime component's selection text.
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
Summary by CodeRabbit