chore: selector methods assume single entity or selector #665
Conversation
WalkthroughImplements per-item selector change-tracking (MatchChange), converts selector APIs from batch to single-item operations, adds a DB-backed environment↔resource Selector, updates SelectorManager and workspace pipeline to use singular resource/environment/deployment fields and call single-item manager methods, and adds a new dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Pipeline as OperationPipeline
participant Manager as SelectorManager
participant Selector as Selector
Client->>Pipeline: dispatch(opts with resource/environment/deployment)
Pipeline->>Manager: updateResource(resource)?\nupdateEnvironment(environment)?\nupdateDeployment(deployment)?
alt field present
Manager->>Selector: upsertEntity(entity)
Selector-->>Manager: Promise<MatchChange[]> (added/removed)
Manager-->>Pipeline: aggregated MatchChange[]
else missing
Manager-->>Pipeline: Promise.resolve() (no-op)
end
Pipeline-->>Client: completion
sequenceDiagram
autonumber
actor Client
participant Pipeline as OperationPipeline
participant Manager as SelectorManager
participant Selector as Selector
Client->>Pipeline: dispatch(delete with optional fields)
Pipeline->>Manager: removeResource(resource)?\nremoveEnvironment(environment)?\nremoveDeployment(deployment)?
alt field present
Manager->>Selector: removeEntity(entity)
Selector-->>Manager: Promise<MatchChange[]> (removed)
else missing
Manager-->>Pipeline: Promise.resolve() (no-op)
end
Pipeline-->>Client: completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
apps/event-queue/src/workspace/pipeline.ts (2)
10-16: Avoid introducing unused option fields (or add corresponding builders/dispatch).
deploymentVersion,policy,job, andjobAgentare added toWorkspaceOptionsbut never consumed in builders ordispatch. This is likely dead code and may trigger lint warnings. Either remove for now or add matching builder methods and dispatch handling.
57-59: Prefer building the ops array over Promise.resolve() no-ops; also verify cross-op ordering needs.Using
Promise.resolve()placeholders is a bit noisy. Build an array of ops instead. Also, if change-tracking depends on deterministic ordering between entity and selector upserts, consider sequentialization or documenting concurrency guarantees.- case "update": - await Promise.all([ - resource ? manager.updateResource(resource) : Promise.resolve(), - environment - ? manager.updateEnvironment(environment) - : Promise.resolve(), - deployment ? manager.updateDeployment(deployment) : Promise.resolve(), - ]); + case "update": { + const ops: Promise<unknown>[] = []; + if (resource) ops.push(manager.updateResource(resource)); + if (environment) ops.push(manager.updateEnvironment(environment)); + if (deployment) ops.push(manager.updateDeployment(deployment)); + await Promise.all(ops); break; - case "delete": - await Promise.all([ - resource ? manager.removeResource(resource) : Promise.resolve(), - environment - ? manager.removeEnvironment(environment) - : Promise.resolve(), - deployment ? manager.removeDeployment(deployment) : Promise.resolve(), - ]); + } + case "delete": { + const ops: Promise<unknown>[] = []; + if (resource) ops.push(manager.removeResource(resource)); + if (environment) ops.push(manager.removeEnvironment(environment)); + if (deployment) ops.push(manager.removeDeployment(deployment)); + await Promise.all(ops); break; + }Also applies to: 65-70, 74-79
apps/event-queue/src/selector/selector.ts (9)
9-14: Export MatchChangeType for consumers and cross-language parity.You already export
MatchChange; exportingMatchChangeTypeimproves ergonomics and mirrors the Go side.-type MatchChangeType = "added" | "removed"; +export type MatchChangeType = "added" | "removed";
43-49: Propagate aggregated changes from updateResource.Underlying selectors return
MatchChange[]but results are discarded. Aggregate and return to callers that need diffs; existing callers can still ignore the value.- async updateResource(resource: Resource) { - await Promise.all([ - this.environmentResources.upsertEntity(resource), - this.deploymentResources.upsertEntity(resource), - this.policyTargetResources.upsertEntity(resource), - ]); - } + async updateResource(resource: Resource) { + const [envRes, depRes, polRes] = await Promise.all([ + this.environmentResources.upsertEntity(resource), + this.deploymentResources.upsertEntity(resource), + this.policyTargetResources.upsertEntity(resource), + ]); + return [...envRes, ...depRes, ...polRes]; + }
51-55: Return changes from updateEnvironment.Maintain symmetry with
updateResourceand surface diffs.- async updateEnvironment(environment: Environment) { - await this.policyTargetEnvironments.upsertEntity(environment); - await this.environmentResources.upsertSelector(environment); - } + async updateEnvironment(environment: Environment) { + const changesA = await this.policyTargetEnvironments.upsertEntity(environment); + const changesB = await this.environmentResources.upsertSelector(environment); + return [...changesA, ...changesB]; + }
56-60: Return changes from updateDeployment.- async updateDeployment(deployment: Deployment) { - await this.policyTargetDeployments.upsertEntity(deployment); - await this.deploymentResources.upsertSelector(deployment); - } + async updateDeployment(deployment: Deployment) { + const changesA = await this.policyTargetDeployments.upsertEntity(deployment); + const changesB = await this.deploymentResources.upsertSelector(deployment); + return [...changesA, ...changesB]; + }
61-65: Return changes from removeResource.- async removeResource(resource: Resource) { - await this.environmentResources.removeEntity(resource); - await this.deploymentResources.removeEntity(resource); - await this.policyTargetResources.removeEntity(resource); - } + async removeResource(resource: Resource) { + const [envRes, depRes, polRes] = await Promise.all([ + this.environmentResources.removeEntity(resource), + this.deploymentResources.removeEntity(resource), + this.policyTargetResources.removeEntity(resource), + ]); + return [...envRes, ...depRes, ...polRes]; + }
67-71: Return changes from removeEnvironment.- async removeEnvironment(environment: Environment) { - await this.policyTargetEnvironments.removeEntity(environment); - await this.environmentResources.removeSelector(environment); - } + async removeEnvironment(environment: Environment) { + const changesA = await this.policyTargetEnvironments.removeEntity(environment); + const changesB = await this.environmentResources.removeSelector(environment); + return [...changesA, ...changesB]; + }
72-76: Return changes from removeDeployment.- async removeDeployment(deployment: Deployment) { - await this.policyTargetDeployments.removeEntity(deployment); - await this.deploymentResources.removeSelector(deployment); - } + async removeDeployment(deployment: Deployment) { + const changesA = await this.policyTargetDeployments.removeEntity(deployment); + const changesB = await this.deploymentResources.removeSelector(deployment); + return [...changesA, ...changesB]; + }
77-81: Method name is plural but takes a single item; also return aggregated changes.Keep API consistent with singular operations. Renaming is optional; returning changes is recommended.
- async upsertPolicyTargets(policyTarget: PolicyTarget) { - await this.policyTargetResources.upsertSelector(policyTarget); - await this.policyTargetEnvironments.upsertSelector(policyTarget); - await this.policyTargetDeployments.upsertSelector(policyTarget); - } + async upsertPolicyTargets(policyTarget: PolicyTarget) { + const [res, env, dep] = await Promise.all([ + this.policyTargetResources.upsertSelector(policyTarget), + this.policyTargetEnvironments.upsertSelector(policyTarget), + this.policyTargetDeployments.upsertSelector(policyTarget), + ]); + return [...res, ...env, ...dep]; + }Optional rename (will require updating call sites):
- async upsertPolicyTargets(policyTarget: PolicyTarget) { + async upsertPolicyTarget(policyTarget: PolicyTarget) {
83-87: Same here: singular naming and change aggregation for removals.- async removePolicyTargets(policyTarget: PolicyTarget) { - await this.policyTargetResources.removeSelector(policyTarget); - await this.policyTargetEnvironments.removeSelector(policyTarget); - await this.policyTargetDeployments.removeSelector(policyTarget); - } + async removePolicyTargets(policyTarget: PolicyTarget) { + const [res, env, dep] = await Promise.all([ + this.policyTargetResources.removeSelector(policyTarget), + this.policyTargetEnvironments.removeSelector(policyTarget), + this.policyTargetDeployments.removeSelector(policyTarget), + ]); + return [...res, ...env, ...dep]; + }Optional rename:
- async removePolicyTargets(policyTarget: PolicyTarget) { + async removePolicyTarget(policyTarget: PolicyTarget) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
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.
📒 Files selected for processing (2)
apps/event-queue/src/selector/selector.ts(2 hunks)apps/event-queue/src/workspace/pipeline.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/event-queue/src/selector/selector.tsapps/event-queue/src/workspace/pipeline.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/event-queue/src/selector/selector.tsapps/event-queue/src/workspace/pipeline.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/src/selector/selector.tsapps/event-queue/src/workspace/pipeline.ts
🧬 Code graph analysis (1)
apps/event-queue/src/selector/selector.ts (2)
apps/workspace-engine/pkg/engine/selector/types.go (1)
MatchChangeType(14-14)apps/event-queue/src/workspace/pipeline.ts (3)
resource(39-42)environment(44-47)deployment(49-52)
⏰ 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). (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (2)
apps/event-queue/src/workspace/pipeline.ts (1)
39-52: Builder methods look good and align with the singular API.Chaining and assignment are straightforward; no issues spotted.
apps/event-queue/src/selector/selector.ts (1)
17-22: Interface return types for change-tracking are a solid improvement.Signatures are clear and future-proof for callers that want diffs.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/event-queue/src/selector/db-environment-resource.ts (3)
306-318: Filter out soft-deleted resources when listing entities for a selector.Keeps behavior consistent with getAllEntities().
Apply:
- .where(eq(schema.computedEnvironmentResource.environmentId, selector.id)) + .where( + and( + eq(schema.computedEnvironmentResource.environmentId, selector.id), + isNull(schema.resource.deletedAt), + ), + )
109-119: Potential N+1 query pattern.Per-environment probes are O(N). If this becomes hot, consider batching via EXISTS over environments with a lateral resource predicate, or caching parsed selectors.
I can prototype a batched EXISTS query if helpful.
Also applies to: 90-107
121-166: Consider wrapping multi-step reconciliation in a transaction.Upsert + compute diffs + mapping writes are not atomic; concurrent updates can race.
If a Tx isn’t provided, start one and reuse it across the whole method.
Also applies to: 241-285, 168-197
apps/event-queue/src/selector/selector.ts (2)
81-85: Rename to singular for consistency.Method name accepts a single PolicyTarget. Consider upsertPolicyTarget.
- async upsertPolicyTargets(policyTarget: PolicyTarget) { + async upsertPolicyTarget(policyTarget: PolicyTarget) {
87-91: Same naming nit.- async removePolicyTargets(policyTarget: PolicyTarget) { + async removePolicyTarget(policyTarget: PolicyTarget) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
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.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/event-queue/package.json(1 hunks)apps/event-queue/src/selector/db-environment-resource.ts(1 hunks)apps/event-queue/src/selector/selector.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/package.jsonapps/event-queue/src/selector/db-environment-resource.tsapps/event-queue/src/selector/selector.ts
**/*.{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/event-queue/src/selector/db-environment-resource.tsapps/event-queue/src/selector/selector.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/event-queue/src/selector/db-environment-resource.tsapps/event-queue/src/selector/selector.ts
🧬 Code graph analysis (2)
apps/event-queue/src/selector/db-environment-resource.ts (3)
packages/db/src/common.ts (3)
Tx(22-22)buildConflictUpdateColumns(30-46)takeFirstOrNull(15-20)apps/event-queue/src/selector/selector.ts (2)
Selector(20-34)MatchChange(14-18)packages/db/src/selectors/index.ts (1)
selector(13-13)
apps/event-queue/src/selector/selector.ts (3)
apps/workspace-engine/pkg/engine/selector/types.go (1)
MatchChangeType(14-14)apps/event-queue/src/workspace/pipeline.ts (3)
resource(39-42)environment(44-47)deployment(49-52)packages/db/src/schema/policy.ts (1)
policyTarget(65-79)
⏰ 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: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
apps/event-queue/package.json (1)
21-21: Ensurets-is-presentcatalog mapping is correctly defined and resolvesI confirmed that your workspace catalog declares a mapping for
ts-is-present:
- In
pnpm-workspace.yaml(around line 27):
ts-is-present: ^1.2.2However, running
pnpm -w why ts-is-presentproduced no output, so it’s not clear whether the mapping is being applied during install. Please install or bootstrap the workspace and re-run the resolution check to be certain.• Locations to verify:
apps/event-queue/package.json(line 21):"ts-is-present": "catalog:"pnpm-workspace.yaml(line 27): mapping forts-is-presentRewritten review comment:
Confirm catalog mapping for ts-is-present.
We see
"ts-is-present": "catalog:"inapps/event-queue/package.json, and yourpnpm-workspace.yamldefinests-is-present: ^1.2.2undercatalog.After installing the workspace dependencies, please run:
pnpm -w why ts-is-presentto ensure that the catalog mapping resolves to version 1.2.2 (or another locked version) as expected. Installs will fail if the mapping isn’t applied.
apps/event-queue/src/selector/db-environment-resource.ts (1)
287-304: Removing selector deletes the Environment row—confirm FK cascade on computedEnvironmentResource.If FK isn’t ON DELETE CASCADE, delete will fail or leave orphans. Either confirm cascade, or explicitly clear mappings first.
If cascade is absent, use:
- await this.db - .delete(schema.environment) - .where(eq(schema.environment.id, selector.id)); + await this.db + .delete(schema.computedEnvironmentResource) + .where(eq(schema.computedEnvironmentResource.environmentId, selector.id)); + await this.db.delete(schema.environment).where(eq(schema.environment.id, selector.id));apps/event-queue/src/selector/selector.ts (8)
9-18: Change-tracking types look good.Enums + payload shape are clear and minimal.
21-26: Interface shift to single-item + change lists—confirm call-site updates.This is a breaking change; ensure all implementers/consumers are updated.
I can scan the repo for outdated variadic calls if needed.
47-53: You’re dropping returned MatchChange[]. Intentional?If these changes should drive downstream events, consider aggregating and returning them from updateResource.
I can wire aggregation and a typed return if desired.
55-58: Same note on ignored changes.Potentially aggregate and bubble up MatchChange[].
60-63: Same note on ignored changes.
65-69: Same note on ignored changes.
71-74: Same note on ignored changes.
76-79: Same note on ignored changes.
Summary by CodeRabbit
New Features
Refactor
Chore