Conversation
|
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:
📝 WalkthroughWalkthroughThis PR extends runtime logs with region and instance filtering capabilities. Changes include refactoring container log display from three separate columns to one composite column, adding region and instance_id fields throughout data schemas and queries, introducing UI filters for region and instance selection, and implementing Kubernetes pod name-to-instance ID resolution on the backend. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 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)
Comment |
ogzhanolguncu
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
mcstepp and
perkinsjr
as code owners
![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
🧹 Nitpick comments (2)
web/internal/clickhouse/src/runtime-logs.ts (1)
16-18: Minor inconsistency in nullability.
regionis nullable (z.array(z.string()).nullable()) whilek8sPodNamesis not (z.array(z.string())). This works since callers pass empty arrays, but consider making them consistent for API clarity:- k8sPodNames: z.array(z.string()), + k8sPodNames: z.array(z.string()).nullable(),Then update the query condition to handle
null:WHEN {k8sPodNames: Nullable(Array(String))} IS NOT NULL AND length({k8sPodNames: ...}) > 0 THEN ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/internal/clickhouse/src/runtime-logs.ts` around lines 16 - 18, The schema has inconsistent nullability: region is defined as z.array(z.string()).nullable() while k8sPodNames is z.array(z.string())—make them consistent by making k8sPodNames nullable as well (z.array(z.string()).nullable()) and update any query logic that checks k8sPodNames to first test for IS NOT NULL before checking length (e.g., change the WHEN clause to check Nullable(Array(String)) IS NOT NULL AND length(...) > 0) so null and empty-array cases are handled uniformly; update references to k8sPodNames and the query condition in runtime-logs.ts accordingly.web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/details/active-deployment-card-logs/hooks/use-runtime-logs.tsx (1)
43-43: Type assertion violates coding guidelines.The
as React.MutableRefObject<HTMLDivElement>cast bypasses type safety. Consider using a generic type parameter or handling the null case explicitly.♻️ Proposed fix
- const scrollRef = useRef<HTMLDivElement>(null) as React.MutableRefObject<HTMLDivElement>; + const scrollRef = useRef<HTMLDivElement | null>(null);Then update usages to handle the nullable ref, or use a callback ref pattern if non-null is required.
As per coding guidelines: "Never compromise type safety: No
any, no!(non-null assertion), noas Type"🤖 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]/(overview)/details/active-deployment-card-logs/hooks/use-runtime-logs.tsx at line 43, The current declaration `scrollRef` uses a type assertion that breaks type safety; replace it with a properly typed nullable ref (e.g., useRef<HTMLDivElement | null>(null)) or switch to a callback ref, and then update all usages of `scrollRef` in use-runtime-logs (e.g., where `scrollRef.current` is accessed) to guard against null (or accept the callback ref signature) so you no longer need the `as React.MutableRefObject<HTMLDivElement>` cast.
🤖 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)/deployments/[deploymentId]/(deployment-progress)/container-logs-table/deployment-container-logs-table.tsx:
- Around line 19-28: The skeleton is being rendered once per column because
renderSkeletonRow maps columns and returns <ContainerLogRowSkeleton /> for every
cell; update renderSkeletonRow (used by the VirtualTable) so it only renders the
full ContainerLogRowSkeleton in the first column (check for column index or
column.key) and return an empty <td> (or a simple placeholder cell) for all
other columns, ensuring className and style (height) are still applied to
preserve layout; target the renderSkeletonRow function and the
ContainerLogRowSkeleton usage to implement this single-cell skeleton rendering.
In `@web/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/query.ts`:
- Around line 46-60: When instance filters are provided but no matching DB rows
are found, k8sPodNames becomes an empty array which makes the ClickHouse CASE
treat the filter as TRUE and return all logs; fix by short-circuiting: after
fetching instances in the block that populates instanceIds, k8sPodNames and
knownK8sToInstanceId, check if instanceIds.length > 0 and instances.length === 0
and immediately return an empty result (same shape as the resolver’s normal
return) or otherwise mark the query to yield no rows; update the resolver around
the code that sets k8sPodNames / knownK8sToInstanceId so the ClickHouse query
never runs with an empty k8sPodNames.
---
Nitpick comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/(overview)/details/active-deployment-card-logs/hooks/use-runtime-logs.tsx:
- Line 43: The current declaration `scrollRef` uses a type assertion that breaks
type safety; replace it with a properly typed nullable ref (e.g.,
useRef<HTMLDivElement | null>(null)) or switch to a callback ref, and then
update all usages of `scrollRef` in use-runtime-logs (e.g., where
`scrollRef.current` is accessed) to guard against null (or accept the callback
ref signature) so you no longer need the `as
React.MutableRefObject<HTMLDivElement>` cast.
In `@web/internal/clickhouse/src/runtime-logs.ts`:
- Around line 16-18: The schema has inconsistent nullability: region is defined
as z.array(z.string()).nullable() while k8sPodNames is z.array(z.string())—make
them consistent by making k8sPodNames nullable as well
(z.array(z.string()).nullable()) and update any query logic that checks
k8sPodNames to first test for IS NOT NULL before checking length (e.g., change
the WHEN clause to check Nullable(Array(String)) IS NOT NULL AND length(...) >
0) so null and empty-array cases are handled uniformly; update references to
k8sPodNames and the query condition in runtime-logs.ts accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ec5cc50-f546-489e-9346-daeb2b5216f6
📒 Files selected for processing (22)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/(deployment-progress)/container-logs-table/columns.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/(deployment-progress)/container-logs-table/deployment-container-logs-table.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/(deployment-progress)/container-logs-table/skeletons.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/details/active-deployment-card-logs/hooks/use-runtime-logs.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/index.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/runtime-logs-deployment-filter.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/runtime-logs-instance-filter.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/runtime-logs-region-filter.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-search/index.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/table/hooks/use-runtime-logs-query.tsweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/table/runtime-log-details/index.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/table/runtime-logs-table.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/hooks/use-runtime-logs-filters.tsweb/apps/dashboard/lib/schemas/runtime-logs.filter.schema.tsweb/apps/dashboard/lib/schemas/runtime-logs.schema.tsweb/apps/dashboard/lib/trpc/routers/deploy/deployment/runtime-logs.tsweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/list-instances.tsweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/llm-search/utils.tsweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/query.tsweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/utils.tsweb/apps/dashboard/lib/trpc/routers/index.tsweb/internal/clickhouse/src/runtime-logs.ts
chronark
reviewed
Mar 17, 2026
Collaborator
chronark
left a comment
chronark
left a comment
There was a problem hiding this comment.
couple of small things + rabbit
Contributor
Author
|
@chronark fixed now |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/query.ts (2)
95-101: Keep unresolvedinstance_idvalues nullable in the API response.Using
"—"here bakes a display placeholder into the TRPC contract, so downstream code can't distinguish “unresolved instance” from a real ID. Prefer returningnull/undefinedhere and let the UI render the fallback. As per coding guidelines, "Make illegal states unrepresentable by modeling domain with discriminated unions and parsing inputs at boundaries into typed structures".🤖 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/runtime-logs/query.ts` around lines 95 - 101, The mapped logs currently set unresolved instance_id to the display placeholder "—" inside the chLogs.map creating logs; instead return a nullable value so the TRPC response preserves the unresolved state. In the mapping where you use k8sNameToInstanceId.get(log.k8s_pod_name) for instance_id, change the fallback from the string "—" to null (or undefined based on existing typing) so downstream UI can render its own placeholder; ensure the logs variable (created by chLogs.map) and any related return type/signature reflect the nullable instance_id in the TRPC contract.
87-93: Skip the fallback lookup when there are no unknown pod names.
resolveK8sNamesToInstanceIdsis called even whenunknownK8sNamesis empty. A cheap guard keeps empty pages and fully-resolved pages from doing extra work.♻️ Proposed change
- const resolvedMapping = await resolveK8sNamesToInstanceIds(unknownK8sNames); + const resolvedMapping = + unknownK8sNames.length > 0 + ? await resolveK8sNamesToInstanceIds(unknownK8sNames) + : new Map<string, string>();🤖 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/runtime-logs/query.ts` around lines 87 - 93, The code always calls resolveK8sNamesToInstanceIds even when unknownK8sNames is empty; add a cheap guard to skip the call and avoid work: check if unknownK8sNames.length === 0 and if so set resolvedMapping to an empty Map (or simply construct k8sNameToInstanceId from knownK8sToInstanceId) instead of awaiting resolveK8sNamesToInstanceIds, otherwise call resolveK8sNamesToInstanceIds as before and build k8sNameToInstanceId from [...knownK8sToInstanceId, ...resolvedMapping]; reference unknownK8sNames, resolveK8sNamesToInstanceIds, knownK8sToInstanceId, and k8sNameToInstanceId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/query.ts`:
- Around line 95-101: The mapped logs currently set unresolved instance_id to
the display placeholder "—" inside the chLogs.map creating logs; instead return
a nullable value so the TRPC response preserves the unresolved state. In the
mapping where you use k8sNameToInstanceId.get(log.k8s_pod_name) for instance_id,
change the fallback from the string "—" to null (or undefined based on existing
typing) so downstream UI can render its own placeholder; ensure the logs
variable (created by chLogs.map) and any related return type/signature reflect
the nullable instance_id in the TRPC contract.
- Around line 87-93: The code always calls resolveK8sNamesToInstanceIds even
when unknownK8sNames is empty; add a cheap guard to skip the call and avoid
work: check if unknownK8sNames.length === 0 and if so set resolvedMapping to an
empty Map (or simply construct k8sNameToInstanceId from knownK8sToInstanceId)
instead of awaiting resolveK8sNamesToInstanceIds, otherwise call
resolveK8sNamesToInstanceIds as before and build k8sNameToInstanceId from
[...knownK8sToInstanceId, ...resolvedMapping]; reference unknownK8sNames,
resolveK8sNamesToInstanceIds, knownK8sToInstanceId, and k8sNameToInstanceId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a076f748-7dc8-4073-a929-58d8027e38e8
📒 Files selected for processing (3)
web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/index.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/runtime-logs-region-filter.tsxweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/query.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/logs/components/controls/components/runtime-logs-filters/runtime-logs-region-filter.tsx
Contributor
Author
|
@mcstepp You are right being extra careful doesn't hurt. It should be good to go now. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/trpc/routers/deploy/runtime-logs/query.ts`:
- Around line 50-58: The code is mapping instances by instances.k8sName which is
only unique per region and can be overwritten across regions; update the mapping
in the block that builds knownK8sToInstanceId (and other similar mappings used
later) to use a region-aware composite key (for example `${k8sName}::${region}`
or the actual region field) instead of k8sName alone, and update the companion
function resolveK8sNamesToInstanceIds in
web/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/utils.ts to return and
accept these region-aware keys so callers (including the places that build
knownK8sToInstanceId and the code paths referenced by
resolveK8sNamesToInstanceIds) can correctly look up instance_id by the composite
key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d053dd8c-7cd5-4333-9867-ce9f5a6fee33
📒 Files selected for processing (3)
web/apps/dashboard/lib/trpc/routers/deploy/deployment/runtime-logs.tsweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/query.tsweb/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/apps/dashboard/lib/trpc/routers/deploy/runtime-logs/utils.ts
chronark
approved these changes
Mar 18, 2026
perkinsjr
approved these changes
Apr 2, 2026
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 adds instanceId, region filtering and instanceId and region fields to runtime logs.