fix: resource search supports direct CEL expressions#981
fix: resource search supports direct CEL expressions#981adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 54 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change introduces CEL (Common Expression Language) query parsing to enhance resource searching. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Pull request overview
Enables the workspace Resources page search box to accept full CEL selector expressions (when the input appears to be CEL), while preserving the existing “plain text” search behavior by wrapping it into a name/identifier contains() selector.
Changes:
- Added client-side CEL parsing to detect valid CEL expressions in the search field.
- Introduced a helper to convert the search string into a selector (
truefor empty, passthrough for valid CEL, otherwise wrap intocontains()). - Updated the
trpc.resource.listquery to use the derived selector.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getCleanedSearch(search: string): string { | ||
| if (search === "") return "true"; |
There was a problem hiding this comment.
Function name getCleanedSearch is a bit misleading since it returns a full CEL selector expression (and can return a constant like true), not a cleaned search string. Renaming to something like getResourceSelector/buildResourceSelector would make the intent clearer.
| const escaped = search.replace(/'/g, "\\'"); | ||
| return `resource.name.contains('${escaped}') || resource.identifier.contains('${escaped}')`; |
There was a problem hiding this comment.
The fallback selector interpolation only escapes single quotes. If the user types a backslash (or other escape-sensitive characters), the generated CEL string literal can become invalid and cause the resource list query to fail. Consider escaping backslashes first (as done in apps/web/app/routes/ws/jobs/_components/ResourceFilter.tsx) and ideally handling other control characters (e.g., newlines) to ensure the selector is always valid CEL.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/app/routes/ws/resources.tsx`:
- Around line 57-64: The current conditional restricts raw CEL parsing to
strings containing "resource.", causing valid CEL expressions with other
prefixes (e.g., "deployment.") to be wrapped incorrectly; change the logic in
the block that uses parse(search) so you attempt CEL parse more broadly—either
remove the search.includes("resource.") guard and try parse(search) for any
input (returning search when result.isSuccess), or switch to an explicit
CEL-mode flag using the existing selectorToCel conversion path; update the code
around the parse(search) call and any related branching so successful CEL parses
(parse, result.isSuccess) for other entity prefixes are accepted instead of
being rewritten.
- Around line 66-67: The current build of the CEL selector only escapes single
quotes but not backslashes, so update the construction of the escaped string
(the assignment to the variable escaped that uses search) to first escape
backslashes in search (replace '\' with '\\') and then escape single quotes,
then continue returning the same selector string
(`resource.name.contains('${escaped}') ||
resource.identifier.contains('${escaped}')`) so CEL receives properly-escaped
backslashes and quotes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c623bdb-3213-4e10-ae3d-c5a5dde3acc5
📒 Files selected for processing (1)
apps/web/app/routes/ws/resources.tsx
| if (search.includes("resource.")) { | ||
| try { | ||
| const result = parse(search); | ||
| if (result.isSuccess) return search; | ||
| } catch { | ||
| // not valid CEL, fall through to wrapping | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't restrict direct CEL mode to resource. queries only.
Line 57 makes CEL parsing conditional on resource., so valid raw selectors using other supported prefixes will be rewritten into the fallback .contains(...) query instead of being executed as CEL. That makes the new feature partial and silently changes query semantics.
Broaden the CEL-path detection beyond a single prefix, or make CEL mode explicit instead of inferring it from resource.. Based on learnings: selectorToCel handles entity-type prefixes like resource. and deployment. in raw CEL expressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/app/routes/ws/resources.tsx` around lines 57 - 64, The current
conditional restricts raw CEL parsing to strings containing "resource.", causing
valid CEL expressions with other prefixes (e.g., "deployment.") to be wrapped
incorrectly; change the logic in the block that uses parse(search) so you
attempt CEL parse more broadly—either remove the search.includes("resource.")
guard and try parse(search) for any input (returning search when
result.isSuccess), or switch to an explicit CEL-mode flag using the existing
selectorToCel conversion path; update the code around the parse(search) call and
any related branching so successful CEL parses (parse, result.isSuccess) for
other entity prefixes are accepted instead of being rewritten.
There was a problem hiding this comment.
this is restricted to the resource page so this is fine
There was a problem hiding this comment.
@adityachoudhari26, that's a fair point — since this is the resource-specific page, only resource. prefixed CEL expressions are semantically meaningful here. The restriction is intentional and correct. I'll withdraw the concern.
✏️ Learnings added
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 981
File: apps/web/app/routes/ws/resources.tsx:57-64
Timestamp: 2026-04-13T18:27:02.578Z
Learning: In `apps/web/app/routes/ws/resources.tsx`, the `getCleanedSearch` function intentionally restricts direct CEL parsing to strings containing `"resource."`. This is by design because the page is the Resources page and only `resource.*` prefixed CEL expressions are relevant there. Do not flag the missing support for other prefixes (e.g., `deployment.`) as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 845
File: apps/workspace-engine/pkg/workspace/store/relationship_indexes.go:180-180
Timestamp: 2026-03-12T22:11:19.490Z
Learning: In `apps/workspace-engine/pkg/workspace/store/relationship_indexes.go`, the `selectorToCel` function uses `strings.ReplaceAll` to replace entity-type prefixes (e.g., "resource.", "deployment.") in raw CEL expressions. This behavior was present pre-refactor and is intentionally preserved. Do not flag it as a new issue in this file.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 237
File: apps/webservice/src/app/[workspaceSlug]/(app)/_components/deployment-resource-drawer/DeploymentResourceDrawer.tsx:43-50
Timestamp: 2024-11-27T23:16:35.580Z
Learning: In `DeploymentResourceDrawer.tsx`, the `isOpen` variable already checks whether `deploymentId`, `environmentId`, and `resourceId` are non-null, so additional null checks in query `enabled` conditions are not necessary.
Learnt from: CR
Repo: ctrlplanedev/ctrlplane PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-09T16:35:48.799Z
Learning: Policies are CEL-based declarative rules with a `selector` field (CEL expression) matching resources/environments
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 637
File: packages/events/src/kafka/client.ts:10-16
Timestamp: 2025-08-01T04:41:41.345Z
Learning: User adityachoudhari26 prefers not to add null safety checks for required environment variables when they are guaranteed to be present in their deployment configuration, similar to their preference for simplicity over defensive programming in test code.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 601
File: e2e/tests/api/policies/retry-policy.spec.ts:23-24
Timestamp: 2025-06-24T23:52:50.732Z
Learning: The user adityachoudhari26 prefers not to add null safety checks or defensive programming in test code, particularly in e2e tests, as they prioritize simplicity and focus on the main functionality being tested rather than comprehensive error handling within the test itself.
Learnt from: adityachoudhari26
Repo: ctrlplanedev/ctrlplane PR: 818
File: apps/workspace-engine/pkg/db/queries/schema.sql:252-282
Timestamp: 2026-02-26T23:01:30.641Z
Learning: User adityachoudhari26 prefers to defer adding database indexes on foreign keys until there's evidence they're needed, considering such additions premature optimization at the initial schema design stage.
fixes #978
Summary by CodeRabbit