Skip to content

feat: new deployments list UI#3797

Merged
chronark merged 43 commits intomainfrom
new-deployments-list-ui
Aug 25, 2025
Merged

feat: new deployments list UI#3797
chronark merged 43 commits intomainfrom
new-deployments-list-ui

Conversation

@ogzhanolguncu
Copy link
Contributor

@ogzhanolguncu ogzhanolguncu commented Aug 15, 2025

What does this PR do?

This UI allows us to see latest deployment list belongs to a certain project.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Test A
  • Test B

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

Summary by CodeRabbit

  • New Features

    • Deployments dashboard: virtualized deployments table with pagination, row actions, and status/environment badges.
    • Rich filters (status, environment, branch, time range/relative) with quick controls and cloud-style field formatting.
    • Natural-language search that translates queries into filters with user-facing feedback.
    • Project breadcrumbs with quick project switcher and repository display.
  • UI

    • Loading skeletons for deployments table columns.
    • New icons and updated info color tokens.
    • Project cards show a navigation loading indicator.
  • Refactor

    • Project page simplified to compose the new deployments components.
image image image image

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 18

♻️ Duplicate comments (21)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx (3)

7-11: Simplify StatusOption: use status as id and drop checked from the type

  • Numeric ids add no value; status is the stable identifier.
  • checked belongs to view state; FilterCheckbox derives it from filters, so it shouldn’t live in options’ type.

Apply this diff:

 type StatusOption = {
-  id: number;
+  id: GroupedDeploymentStatus;
   status: GroupedDeploymentStatus;
   display: string;
-  checked: boolean;
 };

15-37: Stop hardcoding checked and numeric ids in options

Remove checked: false (derived from filters) and use the status as the stable id.

Apply this diff:

 const baseOptions: StatusOption[] = [
   {
-    id: 1,
+    id: "pending",
     status: "pending",
     display: "Pending",
-    checked: false,
   },
   {
-    id: 2,
+    id: "building",
     status: "building",
     display: "Building",
-    checked: false,
   },
   {
-    id: 3,
+    id: "completed",
     status: "completed",
-    display: "Ready",
-    checked: false,
+    display: "Ready",
   },
   {
-    id: 4,
+    id: "failed",
     status: "failed",
     display: "Failed",
-    checked: false,
   },
 ];

51-51: Remove optional chaining on getColorClass

getColorClass is defined in the schema; optional chaining weakens type-safety and is unnecessary.

Apply these diffs:

-          <div className={`size-2 ${getColorClass?.(checkbox.status)} rounded-[2px]`} />
+          <div className={`size-2 ${getColorClass(checkbox.status)} rounded-[2px]`} />
-          colorClass: getColorClass?.(option.status),
+          colorClass: getColorClass(option.status),

Also applies to: 58-58

apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts (5)

18-26: Make the OpenAI model configurable via env (follow UNKEY_ naming).*

Hardcoding the model makes rollouts brittle and violates the project’s env guideline. Use an env var with the mandated prefix and keep the current default.

Apply this diff:

-      // Don't change the model only a few models allow structured outputs
-      model: "gpt-4o-mini",
+      // Configurable; default stays structured-output capable
+      model: process.env.UNKEY_OPENAI_MODEL ?? "gpt-4o-mini",

54-58: Stop logging raw user queries; gate/redact to avoid PII leakage.

This logs the full user input to server logs. Given prior incidents and masking only at the Clickhouse layer, redact and/or log only in non-production.

Apply this diff:

-    console.error(
-      `Something went wrong when querying OpenAI. Input: ${JSON.stringify(
-        userSearchMsg,
-      )}\n Output ${(error as Error).message}`,
-    );
+    if (process.env.NODE_ENV !== "production") {
+      console.error("LLM search failed", {
+        inputPreview: String(userSearchMsg).slice(0, 200),
+        error: (error as Error).message,
+      });
+    } else {
+      console.error("LLM search failed", { error: (error as Error).message });
+    }

64-69: Fix 429 detection for OpenAI APIError (use error.status, not only response.status).

OpenAI’s client typically throws APIError with status on the error object. Your check may miss rate-limit errors and misclassify them as 500s.

Apply this diff:

-    if ((error as { response: { status: number } }).response?.status === 429) {
+    const maybeStatus =
+      (error as any)?.status ?? (error as any)?.response?.status;
+    if (maybeStatus === 429) {
       throw new TRPCError({
         code: "TOO_MANY_REQUESTS",
         message: "Search rate limit exceeded. Please try again in a few minutes.",
       });
     }

79-87: Cache the long system prompt once per process.

This avoids rebuilding a large string on every request.

Apply this diff:

-export const getSystemPrompt = () => {
-  const operatorsByField = Object.entries(deploymentListFilterFieldConfig)
-    .map(([field, config]) => {
-      const operators = config.operators.join(", ");
-      return `- ${field} accepts ${operators} operator${config.operators.length > 1 ? "s" : ""}`;
-    })
-    .join("\n");
-
-  return `You are an expert at converting natural language queries into deployment filters, understanding context and inferring filter types from natural expressions. Handle complex, ambiguous queries by breaking them down into clear filters for deployment management.
+let SYSTEM_PROMPT: string | undefined;
+export const getSystemPrompt = () => {
+  if (SYSTEM_PROMPT) return SYSTEM_PROMPT;
+  const operatorsByField = Object.entries(deploymentListFilterFieldConfig)
+    .map(([field, config]) => {
+      const operators = config.operators.join(", ");
+      return `- ${field} accepts ${operators} operator${config.operators.length > 1 ? "s" : ""}`;
+    })
+    .join("\n");
+
+  SYSTEM_PROMPT = `You are an expert at converting natural language queries into deployment filters, understanding context and inferring filter types from natural expressions. Handle complex, ambiguous queries by breaking them down into clear filters for deployment management.

And at the end of the template (just before the closing backtick in this function):

-]`;
-};
+]`;
+  return SYSTEM_PROMPT;
+};

505-514: Fix contradiction: “ready/active” should map to completed (not pending/building).

Above you state “active/ready ALWAYS means completed.” This example contradicts that.

Apply this diff:

-Query: "show me all ready or active deployments"
-Result: [
-  {
-    field: "status",
-    filters: [
-      { operator: "is", value: "pending" },
-      { operator: "is", value: "building" }
-    ]
-  }
-]
+Query: "show me all ready or active deployments"
+Result: [
+  {
+    field: "status",
+    filters: [
+      { operator: "is", value: "completed" }
+    ]
+  }
+]
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/deployment-status-badge.tsx (2)

13-15: Avoid deep imports; use package public surface.

Importing from internal src paths couples you to private structure and risks breaking on reorganizations. Prefer top-level exports if available.

-import type { IconProps } from "@unkey/icons/src/props";
-import { cn } from "@unkey/ui/src/lib/utils";
+import type { IconProps } from "@unkey/icons";
+import { cn } from "@unkey/ui";

If these aren’t exported at the package root yet, consider re-exporting them there and updating these imports in a follow-up. Would you like me to open a small PR to add the re-exports?


114-121: Add role for better accessibility of status messages.

Expose the status semantics to assistive technology, and announce changes politely for animated/in-progress states.

-    <div
+    <div
+      role="status"
+      aria-live="polite"
+      aria-busy={animated}
       className={cn(
         "items-center flex gap-2 p-1.5 rounded-md w-fit relative",
         animated && "overflow-hidden",
         bgColor,
         className,
       )}
     >
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (2)

99-105: Use EnvStatusBadge for consistency with other chips.

The Environment cell renders a custom gray chip; reuse the shared EnvStatusBadge for consistent visuals and theming.

-          return (
-            <div className="bg-grayA-3 text-xs items-center flex gap-2 p-1.5 rounded-md relative w-fit capitalize">
-              {deployment.environment}
-            </div>
-          );
+          return (
+            <EnvStatusBadge
+              variant={deployment.environment === "production" ? "prod" : "preview"}
+              text={deployment.environment}
+            />
+          );

195-196: Shorten commit SHA to avoid overflow and add hover title.

Use the existing shortenId utility and preserve the full SHA on hover.

-                    {deployment.source.gitSha}
+                    <span title={deployment.source.gitSha}>
+                      {shortenId(deployment.source.gitSha, { startChars: 7, endChars: 0, separator: "" })}
+                    </span>
apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts (9)

8-8: Avoid cross-app import of getTimestampFromRelative (backend depends on UI code)

Importing from @unkey/ui ties server code to a UI package. Move/duplicate this helper into a backend-safe shared module or re-export it from one.

Run this to confirm where the function is actually defined and ensure it’s available in CI/prod:

#!/bin/bash
# Find definition(s) of getTimestampFromRelative
rg -n -C2 -g '!**/node_modules/**' -P '\b(export\s+(function|const|let|var)\s+getTimestampFromRelative\b|\bgetTimestampFromRelative\s*=\s*\()'

# Check all imports of it across the repo
rg -n -C2 -g '!**/node_modules/**' -P 'getTimestampFromRelative'

29-41: Make pullRequest nullable—many deployments won’t have one

Marking pullRequest as required forces fake data and complicates real wiring.

Apply:

 const BaseDeploymentResponse = z.object({
   id: z.string(),
   status: Status,
   instances: z.number(),
   runtime: z.string().nullable(),
   size: z.string().nullable(),
   source: SourceResponse,
   createdAt: z.number(),
   author: AuthorResponse,
   description: z.string().nullable(),
-  pullRequest: PullRequestResponse,
+  pullRequest: PullRequestResponse.nullable(),
 });

58-63: Use a composite cursor to prevent skips when multiple items share createdAt

A single numeric cursor (createdAt) can drop items with identical timestamps. Prefer a composite cursor { createdAt, id }.

Apply here:

 const deploymentsOutputSchema = z.object({
   deployments: z.array(DeploymentResponse),
   hasMore: z.boolean(),
   total: z.number(),
-  nextCursor: z.number().int().nullish(),
+  nextCursor: z
+    .object({
+      createdAt: z.number().int(),
+      id: z.string(),
+    })
+    .nullable(),
 });

And update the response construction:

-        nextCursor:
-          paginatedDeployments.length > 0
-            ? paginatedDeployments[paginatedDeployments.length - 1].createdAt
-            : null,
+        nextCursor:
+          paginatedDeployments.length > 0
+            ? {
+                createdAt: paginatedDeployments[paginatedDeployments.length - 1].createdAt,
+                id: paginatedDeployments[paginatedDeployments.length - 1].id,
+              }
+            : null,

Additionally, update the input schema in apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments.schema.ts:

// apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments.schema.ts
export const deploymentListInputSchema = z.object({
  // ...
- cursor: z.number().nullish(),
+ cursor: z
+   .object({ createdAt: z.number(), id: z.string() })
+   .nullish(),
});

And modify the filter (see cursor block further below) to compare both fields:

filteredDeployments = filteredDeployments.filter(
  d =>
    d.createdAt < cursor.createdAt ||
    (d.createdAt === cursor.createdAt && d.id < cursor.id),
);

87-101: Validate time range (startTime <= endTime) before filtering

Fail fast on inverted ranges to avoid confusing empty results.

       } else {
         startTime = input.startTime;
         endTime = input.endTime;
       }
 
+      // Validate time range
+      if (startTime != null && endTime != null && startTime > endTime) {
+        throw new TRPCError({
+          code: "BAD_REQUEST",
+          message: "startTime must be less than or equal to endTime",
+        });
+      }
+
       // Apply time filters

Also applies to: 102-114


196-202: Truthiness guard drops cursor=0—check type only

Zero is a valid timestamp but is skipped with the current condition.

-      if (input.cursor && typeof input.cursor === "number") {
-        const cursor = input.cursor;
+      if (typeof input.cursor === "number") {
+        const cursor = input.cursor;
         filteredDeployments = filteredDeployments.filter(
           (deployment) => deployment.createdAt < cursor,
         );
       }

239-242: If you adopt a composite cursor, update nextCursor accordingly

Follow-up to the composite cursor suggestion: return both createdAt and id to ensure stable pagination.

-        nextCursor:
-          paginatedDeployments.length > 0
-            ? paginatedDeployments[paginatedDeployments.length - 1].createdAt
-            : null,
+        nextCursor:
+          paginatedDeployments.length > 0
+            ? {
+                createdAt:
+                  paginatedDeployments[paginatedDeployments.length - 1].createdAt,
+                id: paginatedDeployments[paginatedDeployments.length - 1].id,
+              }
+            : null,

246-252: Preserve original error as cause for observability

Attach the original error to the TRPCError; it helps logs and tracing.

       console.error("Error querying deployments:", error);
       throw new TRPCError({
         code: "INTERNAL_SERVER_ERROR",
         message:
           "Failed to retrieve deployments due to an error. If this issue persists, please contact support.",
+        cause: error instanceof Error ? error : undefined,
       });

342-350: Use modern string APIs; avoid deprecated substr()

Replace substr with slice.

-      id: `deployment_${Math.random().toString(36).substr(2, 16)}`,
+      id: `deployment_${Math.random().toString(36).slice(2, 18)}`,
       status,
       instances: Math.floor(Math.random() * 5) + 1,
       runtime: status === "completed" ? runtimes[i % runtimes.length] : null,
       size: sizes[i % sizes.length],
       source: {
         branch: branches[i % branches.length],
-        gitSha: Math.random().toString(36).substr(2, 7),
+        gitSha: Math.random().toString(36).slice(2, 9),
       },

351-351: Stabilize createdAt generation to avoid pagination edge cases

Deterministic spacing (plus tiny deterministic jitter) lowers risk of identical timestamps and flaky pagination.

-      createdAt: baseTime - i * 1000 * 60 * Math.floor(Math.random() * 60),
+      // Deterministic spacing with small per-item jitter to avoid timestamp collisions
+      createdAt: baseTime - i * 60_000 - (i % 1000),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e622e48 and f45db3b.

📒 Files selected for processing (6)
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx (1 hunks)
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/deployment-status-badge.tsx (1 hunks)
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (1 hunks)
  • apps/dashboard/hooks/use-mobile.tsx (1 hunks)
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts (1 hunks)
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages

Files:

  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts
  • apps/dashboard/hooks/use-mobile.tsx
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/deployment-status-badge.tsx
**/*.{ts,tsx}

📄 CodeRabbit Inference Engine (CLAUDE.md)

**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects

Files:

  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts
  • apps/dashboard/hooks/use-mobile.tsx
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts
  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/deployment-status-badge.tsx
**/*.{env,js,ts,go}

📄 CodeRabbit Inference Engine (CLAUDE.md)

All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME

Files:

  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts
🧠 Learnings (5)
📓 Common learnings
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.
📚 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
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.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/project/deployment/list.ts
  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts
📚 Learning: 2025-06-24T13:29:10.129Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3401
File: apps/dashboard/app/(app)/logs/filters.query-params.ts:10-0
Timestamp: 2025-06-24T13:29:10.129Z
Learning: The `buildQueryParams` function in `apps/dashboard/app/(app)/logs/filters.query-params.ts` calls `useFilters()` hook inside it, but this is valid because the function is only called from within other React hooks, maintaining the Rules of Hooks compliance.

Applied to files:

  • apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx
📚 Learning: 2025-01-31T13:50:45.004Z
Learnt from: chronark
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.

Applied to files:

  • apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts
🧬 Code Graph Analysis (5)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (7)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/actions/deployment-list-table-action.popover.constants.tsx (1)
  • DeploymentListTableActions (12-16)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/skeletons.tsx (9)
  • ActionColumnSkeleton (78-89)
  • DeploymentIdColumnSkeleton (4-16)
  • EnvColumnSkeleton (18-22)
  • StatusColumnSkeleton (24-29)
  • InstancesColumnSkeleton (31-39)
  • SizeColumnSkeleton (41-49)
  • SourceColumnSkeleton (51-65)
  • CreatedAtColumnSkeleton (67-69)
  • AuthorColumnSkeleton (71-76)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/hooks/use-deployments-list-query.ts (1)
  • useDeploymentsListQuery (11-123)
apps/dashboard/hooks/use-mobile.tsx (1)
  • useIsMobile (7-22)
apps/dashboard/components/virtual-table/types.ts (1)
  • Column (13-24)
apps/dashboard/lib/shorten-id.ts (1)
  • shortenId (5-48)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/utils/get-row-class.ts (1)
  • getRowClassName (37-50)
apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts (3)
apps/dashboard/app/(app)/projects/[projectId]/deployments/filters.schema.ts (2)
  • DEPLOYMENT_STATUSES (10-20)
  • DeploymentStatus (32-32)
apps/dashboard/lib/trpc/trpc.ts (4)
  • t (8-8)
  • requireUser (10-21)
  • requireWorkspace (23-36)
  • withRatelimit (113-129)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments.schema.ts (1)
  • deploymentListInputSchema (9-17)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx (3)
apps/dashboard/app/(app)/projects/[projectId]/deployments/filters.schema.ts (2)
  • GroupedDeploymentStatus (33-33)
  • deploymentListFilterFieldConfig (50-89)
apps/dashboard/app/(app)/projects/[projectId]/deployments/hooks/use-filters.ts (1)
  • useFilters (33-127)
apps/dashboard/components/logs/checkbox/filter-checkbox.tsx (1)
  • FilterCheckbox (55-259)
apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts (1)
apps/dashboard/app/(app)/projects/[projectId]/deployments/filters.schema.ts (2)
  • deploymentListFilterOutputSchema (129-133)
  • deploymentListFilterFieldConfig (50-89)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/components/deployment-status-badge.tsx (2)
internal/icons/src/props.ts (1)
  • IconProps (30-35)
apps/dashboard/app/(app)/projects/[projectId]/deployments/filters.schema.ts (1)
  • DeploymentStatus (32-32)
⏰ 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 Packages / Test
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Build / Build
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/dashboard/app/(app)/projects/[projectId]/deployments/components/controls/components/deployment-list-filters/components/deployment-status-filter.tsx (1)

4-4: Correct hook import for deployments — good fix

You’re importing the deployments-specific useFilters hook. This avoids the URL/schema desync flagged earlier. LGTM.

apps/dashboard/lib/trpc/routers/deploy/project/deployment/llm-search/utils.ts (1)

39-50: Good fallback UX for unprocessable queries.

Helpful, deployment-relevant examples and consistent support contact.

apps/dashboard/app/(app)/projects/[projectId]/deployments/components/table/deployments-list.tsx (3)

28-37: Good dynamic import with skeleton and SSR disabled.

This addresses hydration pitfalls for client-only popovers and reuses the existing skeleton.


86-87: Nice null guard for optional PR title.

Safe access with a clear fallback avoids runtime errors when no PR is attached.


118-134: VM pluralization looks good.

The conditional label prevents “1 VMs” and reads well across counts.

@mcstepp
Copy link
Collaborator

mcstepp commented Aug 21, 2025

The columns are still moving, and there's a lot of extra padding on the Source column

Screen.Recording.2025-08-21.at.11.10.27.AM.mov

@ogzhanolguncu
Copy link
Contributor Author

@mcstepp sadly some of things you mentioned are caused by react-virtual and custom table. I don't think they are addressable here. The reason for react-virtual is we are actually rendering what you are seeing on the screen not the entire let say 10k elements. Thats why as soon as smaller text appears table will slightly move. But, I think we can shrink the Source column 👍🏻

@chronark
Copy link
Collaborator

@coderabbitai create issues for all things @mcstepp or @chronark mentioned in this PR, create one issue per comment and assign them to @ogzhanolguncu

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 25, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll help you create issues for the concerns mentioned by @mcstepp and @chronark in this PR. Let me first search for any additional comments and then create the issues.

Now I'll create issues for the identified concerns:

@chronark I've created an issue for @mcstepp's comment about the table column movement and excessive padding in the Source column. Based on my review of the comment chain and PR content, this appears to be the main concern that needs to be tracked as a separate issue.

The issue has been assigned to @ogzhanolguncu as requested and includes:

  • Clear description of the problems (column movement and Source column padding)
  • Context from the PR discussion
  • Reference back to the original PR and comment
  • Notes about the technical constraints mentioned by @ogzhanolguncu

If there are any other specific comments from you or @mcstepp in this PR that you'd like me to create separate issues for, please let me know and I'll be happy to create them as well.


🏁 Script executed:

#!/bin/bash
# Search for any additional comments from chronark in the codebase or PR content
echo "Searching for chronark mentions in the codebase..."
rg -i "chronark" --type=md --type=txt -C 2 || true

# Check if there are any TODO or FIXME comments that might be related
echo -e "\nSearching for TODO/FIXME comments that might be related to chronark..."
rg -i "(todo|fixme).*chronark" -C 2 || true

Length of output: 1050


🏁 Script executed:

#!/bin/bash
# Create issue for mcstepp's comment about columns moving and Source column padding
gh issue create \
  --title "Fix table column movement and excessive padding in Source column" \
  --body "**Issue reported by:** @mcstepp
**Original PR:** https://github.com/unkeyed/unkey/pull/3797
**Comment reference:** https://github.com/unkeyed/unkey/pull/3797#issuecomment

## Problem
The deployments table has two issues:
1. Columns are still moving during rendering/scrolling
2. There's excessive padding on the Source column

## Context
This issue was identified during the review of the new deployments list UI. The column movement is likely related to react-virtual rendering behavior, and the Source column padding needs to be reduced for better visual balance.

## Expected Behavior
- Table columns should remain stable during rendering and scrolling
- Source column should have appropriate padding consistent with other columns

## Additional Notes
- This issue is related to the react-virtual implementation used for performance optimization
- @ogzhanolguncu mentioned that some column movement issues might be inherent to react-virtual but the Source column padding can be addressed" \
  --assignee ogzhanolguncu

Length of output: 1165


@chronark I've successfully created the issue for the concerns mentioned by @mcstepp in this PR:

Issue Created:

The issue includes all the context from @mcstepp's comment, references back to this PR, and incorporates @ogzhanolguncu's technical explanation about the react-virtual implementation constraints.

Based on my search through the PR content and comment chain, this appears to be the main actionable issue that needs separate tracking. If there are any other specific comments or concerns from you or @mcstepp that you'd like me to create additional issues for, please point them out and I'll create them right away.


🧠 Learnings used
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2825
File: apps/dashboard/app/(app)/logs-v2/hooks/use-bookmarked-filters.ts:0-0
Timestamp: 2025-01-30T20:51:44.359Z
Learning: The user (ogzhanolguncu) prefers to handle refactoring suggestions in separate PRs to maintain focus in the current PR.

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.

@chronark
Copy link
Collaborator

@coderabbitai also create an issue for my comment

I think we should rename the status "active" to "ready"
it captures the meaning better, cause a vm does not need to be running for us to be ready to handle a request. and it's less confusing with billing cause users rightfully might wonder if they're paying for active instances

@chronark
Copy link
Collaborator

@coderabbitai create an issue for my previous comment #3797 (review)

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 25, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll create an issue for your comment about renaming the "active" status to "ready".

@chronark I've created an issue for your comment about renaming the deployment status from "active" to "ready":

Issue Created:

The issue captures your concern about the current "active" status being potentially confusing for users, especially regarding billing expectations, and includes your proposed solution with the full rationale.


🏁 Script executed:

#!/bin/bash
# Create issue for chronark's comment about renaming status from "active" to "ready"
gh issue create \
  --title "Rename deployment status from 'active' to 'ready' for better clarity" \
  --body "**Issue reported by:** @chronark
**Original PR:** https://github.com/unkeyed/unkey/pull/3797
**Comment reference:** https://github.com/unkeyed/unkey/pull/3797

## Problem
The current deployment status \"active\" can be confusing and misleading for users, particularly around billing expectations.

## Proposed Solution
Rename the status from \"active\" to \"ready\" because:

1. **Better semantic meaning**: A VM doesn't need to be running for the system to be ready to handle requests
2. **Reduces billing confusion**: Users might rightfully wonder if they're paying for \"active\" instances, when the actual billing model may differ
3. **Clearer user experience**: \"Ready\" better captures the state where the deployment can serve traffic

## Original Comment
> I think we should rename the status \"active\" to \"ready\"
> it captures the meaning better, cause a vm does not need to be running for us to be ready to handle a request. and it's less confusing with billing cause users rightfully might wonder if they're paying for active instances

## Expected Changes
- Update status value from \"active\" to \"ready\" in the deployment system
- Update UI components and status displays
- Update any related documentation or user-facing text

## Additional Context
This change would improve user understanding and reduce potential confusion around billing and deployment states." \
  --assignee ogzhanolguncu

Length of output: 1563


Perfect! I've successfully created the issue for your comment about renaming the deployment status:

Issue Created:

The issue includes:

  • Your complete rationale about better semantic meaning
  • The billing confusion concern you raised
  • Clear expected changes needed in the codebase
  • Reference back to this PR and your original comment

Both issues are now tracked separately and assigned to @ogzhanolguncu for follow-up work. This ensures these important UX improvements won't get lost and can be addressed in future iterations.


🧠 Learnings used
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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 25, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Now I'll create the GitHub issue:

@chronark I've created an issue for your comment about renaming the deployment status from "active" to "ready":

Issue Created:

The issue includes:

  • Your reasoning about better semantic meaning and reduced billing confusion
  • Comprehensive scope of changes needed (schema, UI, backend logic)
  • Clear acceptance criteria for implementation
  • Reference back to the original PR comment

The issue captures your point that "ready" better represents that a deployment can handle requests regardless of VM running state, and eliminates potential confusion around billing for "active" instances.


🏁 Script executed:

#!/bin/bash
# Search for "active" status references in the deployment-related code
echo "Searching for 'active' status references in deployment code..."
rg -n "active" apps/dashboard/app/\(app\)/projects/\[projectId\]/deployments/ -C 2

echo -e "\n\nSearching for status-related constants or enums..."
rg -n "(status|Status)" apps/dashboard/app/\(app\)/projects/\[projectId\]/deployments/filters.schema.ts -C 3

echo -e "\n\nSearching for ProductionDeploymentResponse which might contain active field..."
rg -n "ProductionDeploymentResponse\|active.*deployment" apps/dashboard/lib/trpc/routers/deploy/project/deployment/list.ts -C 3

Length of output: 15690


🏁 Script executed:

#!/bin/bash
# Create issue for chronark's comment about renaming "active" status to "ready"
gh issue create \
  --title "Rename deployment status from 'active' to 'ready' for better clarity" \
  --body "**Issue reported by:** @chronark
**Original PR:** https://github.com/unkeyed/unkey/pull/3797
**Comment reference:** https://github.com/unkeyed/unkey/pull/3797#pullrequestreview-3135510436

## Problem
The current deployment status uses 'active' which can be confusing and doesn't accurately capture the intended meaning.

## Proposed Solution
Rename the status from 'active' to 'ready' for the following reasons:

1. **Better semantic meaning**: 'Ready' captures that the deployment is prepared to handle requests, regardless of whether the VM is actively running
2. **Reduces billing confusion**: Users might incorrectly assume they're being charged for 'active' instances, when the billing model may be different
3. **Clearer user experience**: 'Ready' is more intuitive for users to understand the deployment state

## Scope
This change affects:
- Status definitions in the deployment schema
- UI components that display deployment status
- Any backend logic that references the 'active' status
- Documentation and user-facing text

## Acceptance Criteria
- [ ] Update status constants/enums from 'active' to 'ready'
- [ ] Update UI components and badges to display 'ready' instead of 'active'
- [ ] Update any backend logic that checks for 'active' status
- [ ] Ensure no breaking changes to existing deployments
- [ ] Update any related documentation

## Additional Context
This change improves the user experience by providing clearer, less ambiguous status terminology." \
  --assignee ogzhanolguncu

Length of output: 1741


@chronark I've successfully created the issue for your comment about renaming "active" to "ready":

Issue Created:

Based on my search of the codebase, I found that the active field is currently used as a boolean property on production deployments. Specifically, in the deployments list UI (line 81 of deployments-list.tsx), there's a check:

{deployment.environment === "production" && deployment.active && (
  <EnvStatusBadge variant="current" text="Current" />
)}

This shows a "Current" badge for active production deployments. The issue I created includes:

  • Your reasoning about better semantic meaning and reduced billing confusion
  • Comprehensive scope covering schema, UI components, and backend logic
  • Clear acceptance criteria for implementation
  • Reference back to your original comment

The issue captures the need to rename this concept from "active" to "ready" to better represent that the deployment is prepared to handle requests, regardless of whether the VM is actively running.


🧠 Learnings used
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: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.

Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.

Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.

Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: For repo unkeyed/unkey and PR review workflows: When imeyer comments "issue" on a thread, automatically create a thorough GitHub issue (sections: Summary, Impact, Where, Repro/Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and the specific comment, and assign the issue to imeyer.

Learnt from: MichaelUnkey
PR: unkeyed/unkey#3103
File: apps/dashboard/app/(app)/settings/general/page.tsx:36-36
Timestamp: 2025-04-14T13:39:22.635Z
Learning: The Unkey team uses Linear for issue tracking rather than GitHub Issues.

@chronark
Copy link
Collaborator

Let's get this merged, as we will iterate on it over the next weeks

@ogzhanolguncu please close or create issues from the existing rabbit comments, then we can merge

@chronark chronark enabled auto-merge August 25, 2025 14:46
Copy link
Collaborator

@mcstepp mcstepp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issues are in separate tickets, 👍

@chronark chronark added this pull request to the merge queue Aug 25, 2025
@graphite-app
Copy link

graphite-app bot commented Aug 25, 2025

TV gif. We look up at Rowan Atkinson as Mr. Bean wearing medical scrubs. He pulls down a surgical mask, gives a gloved thumbs up, and smiles maniacally. (Added via Giphy)

@graphite-app
Copy link

graphite-app bot commented Aug 25, 2025

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (08/25/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

Merged via the queue into main with commit 40d21cd Aug 25, 2025
18 checks passed
@chronark chronark deleted the new-deployments-list-ui branch August 25, 2025 14:56
@coderabbitai coderabbitai bot mentioned this pull request Aug 29, 2025
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants