chore: update deployment cel ui#699
Conversation
|
Warning Rate limit exceeded@adityachoudhari26 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR implements deployment resource selection functionality by introducing a new route, page component, and TRPC mutation. It adds UI for managing CEL-based resource selectors and persists configurations with validation. The Deployment type is extended with an optional resourceSelector field, and navigation is updated with a Resources tab. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Resource Selector Page
participant API as TRPC/wsEngine
participant Storage as Backend
User->>UI: Navigate to Resources tab
UI->>API: Fetch deployment & resources (TRPC)
API-->>UI: Current selector & resources
UI->>UI: Display CEL expression input
User->>UI: Enter/modify CEL expression
UI->>UI: Debounce input (1s)
UI->>API: Fetch matching resources (TRPC)
API-->>UI: Filtered resources
UI->>UI: Display resource preview
User->>UI: Click Save
UI->>API: Validate selector (POST /v1/validate/resource-selector)
alt Validation Success
API->>Storage: Persist resourceSelectorCel
Storage-->>API: Deployment updated
API-->>UI: Success
UI->>UI: Show success toast
else Validation Failed
API-->>UI: Aggregated errors
UI->>UI: Display error messages
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple layers (routing, UI components, type definitions, backend mutation) with moderate logic density in the resource selector page (debouncing, state management, TRPC hooks). The validation flow in the TRPC mutation adds complexity, but the overall pattern is cohesive and follows existing conventions. Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsx (1)
20-33: Optimize version check query.The
useNoVersionshook has two potential optimizations:
Reduce polling frequency or remove it: Polling every 5 seconds seems excessive for checking version existence. Versions are unlikely to appear/disappear frequently during a single page session. Consider removing
refetchIntervalor increasing it significantly.Use
limit: 1instead oflimit: 1000: Since you only need to know if any versions exist, fetching a single version is sufficient.Apply this diff to optimize the query:
const versionsQuery = trpc.deployment.versions.useQuery( { workspaceId: workspace.id, deploymentId: deployment.id, - limit: 1000, + limit: 1, offset: 0, }, - { refetchInterval: 5000 }, );apps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsx (1)
32-79: Consider displaying the fetched resources.The
SelectorWithResourcescomponent fetches resources based on the debounced selector (line 42), but these resources are never displayed in the UI. This might be intentional if the feature is incomplete, but consider:
- Showing a preview of matching resources to help users verify their selector
- Displaying the resource count
- Removing the unused
resourcesvariable if display isn't planned
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/web/app/routes.ts(1 hunks)apps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsx(1 hunks)apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx(1 hunks)apps/web/app/routes/ws/deployments/_components/DeploymentsNavbarTabs.tsx(1 hunks)apps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsx(1 hunks)packages/trpc/src/routes/deployments.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsxapps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsxapps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsxpackages/trpc/src/routes/deployments.tsapps/web/app/routes/ws/deployments/_components/DeploymentsNavbarTabs.tsxapps/web/app/routes.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsxapps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsxapps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsxpackages/trpc/src/routes/deployments.tsapps/web/app/routes/ws/deployments/_components/DeploymentsNavbarTabs.tsxapps/web/app/routes.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsxapps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsxapps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsxpackages/trpc/src/routes/deployments.tsapps/web/app/routes/ws/deployments/_components/DeploymentsNavbarTabs.tsxapps/web/app/routes.ts
🧬 Code graph analysis (3)
apps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsx (5)
apps/web/app/components/WorkspaceProvider.tsx (1)
useWorkspace(33-38)apps/web/app/api/trpc.tsx (1)
trpc(15-15)apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx (1)
useDeployment(36-42)apps/web/app/routes/ws/_components/CelExpiressionInput.tsx (1)
CelExpressionInput(920-1040)apps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsx (1)
DeploymentPageHeader(35-74)
apps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsx (5)
apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx (1)
useDeployment(36-42)apps/web/app/components/WorkspaceProvider.tsx (1)
useWorkspace(33-38)apps/web/app/api/trpc.tsx (1)
trpc(15-15)apps/web/app/routes/ws/deployments/_components/CreateVersionDialog.tsx (1)
CreateVersionDialog(67-299)apps/web/app/routes/ws/deployments/_components/DeploymentsNavbarTabs.tsx (1)
DeploymentsNavbarTabs(7-44)
packages/trpc/src/routes/deployments.ts (3)
packages/trpc/src/trpc.ts (1)
protectedProcedure(97-97)packages/trpc/src/ws-engine.ts (1)
wsEngine(3-3)packages/events/src/kafka/client.ts (1)
sendGoEvent(35-65)
🔇 Additional comments (5)
apps/web/app/routes.ts (1)
11-14: LGTM!The new resources route follows the established pattern for deployment sub-routes and aligns with the Resources tab added to the navigation.
apps/web/app/routes/ws/deployments/_components/DeploymentsNavbarTabs.tsx (2)
15-21: LGTM!The active tab selection logic correctly identifies the resources path and follows the established cascading ternary pattern.
29-31: LGTM!The Resources tab trigger is correctly configured and positioned appropriately in the navigation.
apps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsx (1)
62-69: Verify the Create Version button visibility logic.The button is shown when versions exist (
!noVersions) and hidden when there are no versions. This seems counterintuitive—typically, a "Create" button would be more prominent when the list is empty. Please verify this is the intended behavior (e.g., if the first version must be created elsewhere).apps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsx (1)
19-30: RemoveuseResourceshook or implement resource display.The
useResourceshook fetches resources but they're never displayed in the component. TheSelectorWithResourcescomponent only renders a CEL expression editor and save button—no resource list. Either remove the unused hook or implement resource display with proper truncation handling.Likely an incorrect or invalid review comment.
| <Breadcrumb> | ||
| <BreadcrumbList> | ||
| <BreadcrumbItem> | ||
| <BreadcrumbItem> | ||
| <Link to={`/${workspace.slug}/deployments`}>Deployments</Link> | ||
| </BreadcrumbItem> | ||
| <BreadcrumbSeparator /> | ||
| <BreadcrumbPage>{deployment.name}</BreadcrumbPage> | ||
| </BreadcrumbItem> | ||
| </BreadcrumbList> | ||
| </Breadcrumb> |
There was a problem hiding this comment.
Fix incorrect breadcrumb nesting.
The breadcrumb structure has redundant nested BreadcrumbItem elements. The outer BreadcrumbItem (Line 50) wraps another BreadcrumbItem (Line 51), which is incorrect.
Apply this diff to fix the structure:
<Breadcrumb>
<BreadcrumbList>
- <BreadcrumbItem>
- <BreadcrumbItem>
- <Link to={`/${workspace.slug}/deployments`}>Deployments</Link>
- </BreadcrumbItem>
- <BreadcrumbSeparator />
- <BreadcrumbPage>{deployment.name}</BreadcrumbPage>
+ <BreadcrumbItem>
+ <Link to={`/${workspace.slug}/deployments`}>Deployments</Link>
</BreadcrumbItem>
+ <BreadcrumbSeparator />
+ <BreadcrumbItem>
+ <BreadcrumbPage>{deployment.name}</BreadcrumbPage>
+ </BreadcrumbItem>
</BreadcrumbList>
</Breadcrumb>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Breadcrumb> | |
| <BreadcrumbList> | |
| <BreadcrumbItem> | |
| <BreadcrumbItem> | |
| <Link to={`/${workspace.slug}/deployments`}>Deployments</Link> | |
| </BreadcrumbItem> | |
| <BreadcrumbSeparator /> | |
| <BreadcrumbPage>{deployment.name}</BreadcrumbPage> | |
| </BreadcrumbItem> | |
| </BreadcrumbList> | |
| </Breadcrumb> | |
| <Breadcrumb> | |
| <BreadcrumbList> | |
| <BreadcrumbItem> | |
| <Link to={`/${workspace.slug}/deployments`}>Deployments</Link> | |
| </BreadcrumbItem> | |
| <BreadcrumbSeparator /> | |
| <BreadcrumbItem> | |
| <BreadcrumbPage>{deployment.name}</BreadcrumbPage> | |
| </BreadcrumbItem> | |
| </BreadcrumbList> | |
| </Breadcrumb> |
🤖 Prompt for AI Agents
In apps/web/app/routes/ws/deployments/_components/DeploymentPageHeader.tsx
around lines 48 to 58, the breadcrumb contains a redundant nested
BreadcrumbItem; remove the outer BreadcrumbItem so the BreadcrumbList contains
sibling items instead of one wrapping another. Replace the nested structure with
two sibling BreadcrumbItem entries: the first should hold the Link to
`/${workspace.slug}/deployments`, followed by BreadcrumbSeparator, and the
second should contain BreadcrumbPage with deployment.name, ensuring no
BreadcrumbItem is nested inside another.
| description?: string; | ||
| jobAgentId?: string; | ||
| systemId: string; | ||
| resourceSelector?: { cel?: string; json?: Record<string, unknown> }; |
There was a problem hiding this comment.
Clarify the purpose of the unused json field.
The resourceSelector type includes both cel and json fields, but the current implementation (TRPC mutation and UI) only uses the cel field. Consider removing the json field if it's not needed, or document its intended use if it's for future functionality.
If the json field should be removed:
- resourceSelector?: { cel?: string; json?: Record<string, unknown> };
+ resourceSelector?: { cel?: string };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resourceSelector?: { cel?: string; json?: Record<string, unknown> }; | |
| resourceSelector?: { cel?: string }; |
🤖 Prompt for AI Agents
In apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx around
line 11 the resourceSelector type declares a json field that is never used by
the TRPC mutation or UI; either remove the json?: Record<string, unknown>
property from the type and update any related type definitions and TRPC
input/schema to drop it, or if it’s intended for future use, add a concise
inline comment documenting its purpose and expected shape and ensure the TRPC
mutation/input types explicitly accept and validate it; make the change
consistently where the resourceSelector type is referenced.
| const onResourceSelectorChange = async (selector: string) => { | ||
| setSelector(selector); | ||
| console.log("selector", selector); | ||
| await updateDeployment.mutateAsync({ | ||
| workspaceId: workspace.id, | ||
| deploymentId: deployment.id, | ||
| data: { resourceSelectorCel: selector }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Remove debug statement and add error handling.
Two issues in the onResourceSelectorChange function:
- Debug statement: Line 47 contains a
console.logthat should be removed. - Missing error handling: The mutation has no try-catch block, and the calling code in the onClick handler (lines 67-71) will show a success toast even if the mutation fails.
Apply this diff to fix both issues:
const onResourceSelectorChange = async (selector: string) => {
setSelector(selector);
- console.log("selector", selector);
- await updateDeployment.mutateAsync({
- workspaceId: workspace.id,
- deploymentId: deployment.id,
- data: { resourceSelectorCel: selector },
- });
+ try {
+ await updateDeployment.mutateAsync({
+ workspaceId: workspace.id,
+ deploymentId: deployment.id,
+ data: { resourceSelectorCel: selector },
+ });
+ } catch (error) {
+ const message = error instanceof Error ? error.message : "Failed to update resource selector";
+ toast.error(message);
+ throw error;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onResourceSelectorChange = async (selector: string) => { | |
| setSelector(selector); | |
| console.log("selector", selector); | |
| await updateDeployment.mutateAsync({ | |
| workspaceId: workspace.id, | |
| deploymentId: deployment.id, | |
| data: { resourceSelectorCel: selector }, | |
| }); | |
| }; | |
| const onResourceSelectorChange = async (selector: string) => { | |
| setSelector(selector); | |
| try { | |
| await updateDeployment.mutateAsync({ | |
| workspaceId: workspace.id, | |
| deploymentId: deployment.id, | |
| data: { resourceSelectorCel: selector }, | |
| }); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : "Failed to update resource selector"; | |
| toast.error(message); | |
| throw error; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsx around
lines 45 to 53, remove the debug console.log and add proper error handling for
the mutation: wrap the call to updateDeployment.mutateAsync in a try/catch,
await the mutation inside try, and in catch handle the error (e.g., log it and
rethrow or return a failure indicator) so calling code can decide whether to
show a success toast; ensure setSelector remains correct on success and consider
reverting it on failure.
| <Button | ||
| className="h-[2.5rem]" | ||
| onClick={() => | ||
| onResourceSelectorChange(selector).then(() => | ||
| toast.success( | ||
| "Deployment resource selector updated successfully.", | ||
| ), | ||
| ) | ||
| } | ||
| > | ||
| Save | ||
| </Button> |
There was a problem hiding this comment.
Add loading state and error handling to the Save button.
The Save button is missing:
- Loading state during the mutation (use
updateDeployment.isPending) - Error handling for the promise chain
- Button should be disabled while saving
Apply this diff to improve the button implementation:
<Button
className="h-[2.5rem]"
+ disabled={updateDeployment.isPending}
onClick={() =>
onResourceSelectorChange(selector).then(() =>
toast.success(
"Deployment resource selector updated successfully.",
),
- )
+ ).catch(() => {
+ // Error already handled in onResourceSelectorChange
+ })
}
>
+ {updateDeployment.isPending && "Saving..."}
+ {!updateDeployment.isPending && "Save"}
- Save
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| className="h-[2.5rem]" | |
| onClick={() => | |
| onResourceSelectorChange(selector).then(() => | |
| toast.success( | |
| "Deployment resource selector updated successfully.", | |
| ), | |
| ) | |
| } | |
| > | |
| Save | |
| </Button> | |
| <Button | |
| className="h-[2.5rem]" | |
| disabled={updateDeployment.isPending} | |
| onClick={() => | |
| onResourceSelectorChange(selector).then(() => | |
| toast.success( | |
| "Deployment resource selector updated successfully.", | |
| ), | |
| ).catch(() => { | |
| // Error already handled in onResourceSelectorChange | |
| }) | |
| } | |
| > | |
| {updateDeployment.isPending && "Saving..."} | |
| {!updateDeployment.isPending && "Save"} | |
| </Button> |
🤖 Prompt for AI Agents
In apps/web/app/routes/ws/deployments/page.$deploymentId.resources.tsx around
lines 64 to 75, the Save button currently fires
onResourceSelectorChange(selector) without handling pending state, errors, or
disabling the button; update the Button to use updateDeployment.isPending to set
a loading/visual state and disabled=true while pending, call
onResourceSelectorChange(selector).then(() => toast.success("Deployment resource
selector updated successfully.")).catch(err => toast.error(err?.message ||
"Failed to update deployment resource selector.")), and ensure the promise is
returned/awaited when appropriate so the UI reflects completion or failure.
| update: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| workspaceId: z.uuid(), | ||
| deploymentId: z.string(), | ||
| data: z.object({ | ||
| resourceSelectorCel: z.string().min(1).max(255), | ||
| }), | ||
| }), | ||
| ) | ||
| .mutation(async ({ input }) => { | ||
| const { workspaceId, deploymentId, data } = input; | ||
| const validate = await wsEngine.POST("/v1/validate/resource-selector", { | ||
| body: { resourceSelector: { cel: data.resourceSelectorCel } }, | ||
| }); | ||
|
|
||
| if (!validate.data?.valid) | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: | ||
| Array.isArray(validate.data?.errors) && | ||
| validate.data.errors.length > 0 | ||
| ? validate.data.errors.join(", ") | ||
| : "Invalid resource selector", | ||
| }); | ||
|
|
||
| const deployment = await wsEngine.GET( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId, deploymentId } } }, | ||
| ); | ||
|
|
||
| if (!deployment.data) | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Deployment not found", | ||
| }); | ||
|
|
||
| const resourceSelector = { cel: data.resourceSelectorCel }; | ||
| const updateData = { ...deployment.data, resourceSelector }; | ||
|
|
||
| await sendGoEvent({ | ||
| workspaceId, | ||
| eventType: Event.DeploymentUpdated, | ||
| timestamp: Date.now(), | ||
| data: updateData, | ||
| }); | ||
|
|
||
| return deployment.data; | ||
| }), |
There was a problem hiding this comment.
Add authorization check to the update mutation.
The update mutation is missing an authorization check. All other mutations in this router (get, list, create, etc.) include .meta({ authorizationCheck }) to verify permissions. This is a critical security issue as it allows any authenticated user to update any deployment.
Add the authorization check after the input definition:
update: protectedProcedure
.input(
z.object({
workspaceId: z.uuid(),
deploymentId: z.string(),
data: z.object({
resourceSelectorCel: z.string().min(1).max(255),
}),
}),
)
+ .meta({
+ authorizationCheck: ({ canUser, input }) =>
+ canUser
+ .perform(Permission.DeploymentUpdate)
+ .on({ type: "workspace", id: input.workspaceId }),
+ })
.mutation(async ({ input }) => {🤖 Prompt for AI Agents
In packages/trpc/src/routes/deployments.ts around lines 147 to 195, the update
mutation is missing the required authorization check; add the same authorization
metadata used by other mutations by inserting .meta({ authorizationCheck })
immediately after the .input(...) call for the update protectedProcedure so the
request permissions are validated before .mutation executes, ensuring only
authorized users can update deployments.
Summary by CodeRabbit