chore: init basic event sending and receiving#644
Conversation
WalkthroughThis change introduces structured event dispatching for resource lifecycle events in the API, adds new Kafka event dispatch utilities, and improves logging and graceful shutdown handling in the workspace engine. New configuration and scripts for development and build workflows are also included, along with Docker Compose and Kafka connectivity adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant API as API Route
participant Events as @ctrlplane/events
participant Kafka as Kafka Broker
API->>Events: sendResourceEvent(previous, current)
alt previous is null
Events->>Kafka: dispatchResourceCreated(current, source="api")
else previous exists
Events->>Kafka: dispatchResourceUpdated(previous, current, source="api")
end
Kafka-->>Events: Ack/Result
Events-->>API: Completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/workspace-engine/package.json (1)
1-8: Missingprivate: trueand devDependency forair
- Prevent accidental npm publish of a Go project:
{ + "private": true,
- The
devscript assumesairis on the PATH. Declare it sopnpm i/npm igives a reproducible setup:"scripts": { "dev": "air", @@ }, + "devDependencies": { + "air": "npm:@cosmtrek/air" + }(or document that
airis installed viago install).packages/events/src/kafka/event-dispatch/index.ts (1)
1-1: Avoid hard-coding.jsextension unless TS config opts into itWith default
moduleResolution: nodeTypeScript cannot resolve./resource.js. If the repo isn’t using the newer bundler/Node-16 resolution, drop the extension:-export * from "./resource.js"; +export * from "./resource";packages/events/src/kafka/index.ts (1)
3-3: Consistent path styleFor the same reason as above, prefer extension-less import:
-export * from "./event-dispatch/index.js"; +export * from "./event-dispatch";apps/webservice/src/app/api/v1/resources/route.ts (1)
118-118: Consider error handling for event dispatching.The event dispatching is not wrapped in error handling. While the
sendEventfunction handles errors internally by logging them, you might want to consider whether failed event dispatching should affect the API response or be handled differently.If you want to ensure event dispatching doesn't affect the API response, you could wrap it in a try/catch:
- await sendResourceEvent(existingResource, insertedResource); + try { + await sendResourceEvent(existingResource, insertedResource); + } catch (error) { + log.error("Failed to dispatch resource event", { error }); + }However, since the underlying
sendEventfunction already handles errors internally, this may not be necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webservice/src/app/api/v1/resources/route.ts(3 hunks)apps/workspace-engine/.air.toml(1 hunks)apps/workspace-engine/main.go(3 hunks)apps/workspace-engine/package.json(1 hunks)apps/workspace-engine/pkg/kafka/consumer.go(1 hunks)docker-compose.dev.yaml(2 hunks)packages/events/src/kafka/client.ts(2 hunks)packages/events/src/kafka/event-dispatch/index.ts(1 hunks)packages/events/src/kafka/event-dispatch/resource.ts(1 hunks)packages/events/src/kafka/index.ts(1 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:
packages/events/src/kafka/event-dispatch/index.tspackages/events/src/kafka/client.tspackages/events/src/kafka/index.tspackages/events/src/kafka/event-dispatch/resource.tsapps/webservice/src/app/api/v1/resources/route.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:
packages/events/src/kafka/event-dispatch/index.tspackages/events/src/kafka/client.tspackages/events/src/kafka/index.tspackages/events/src/kafka/event-dispatch/resource.tsapps/webservice/src/app/api/v1/resources/route.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
packages/events/src/kafka/event-dispatch/index.tsdocker-compose.dev.yamlapps/workspace-engine/package.jsonpackages/events/src/kafka/client.tspackages/events/src/kafka/index.tspackages/events/src/kafka/event-dispatch/resource.tsapps/webservice/src/app/api/v1/resources/route.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#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
PR: ctrlplanedev/ctrlplane#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
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
📚 Learning: applies to **/*.{ts,tsx} : import styles: use named imports, group imports by source (std lib > exte...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T22:34:20.754Z
Learning: Applies to **/*.{ts,tsx} : Import styles: Use named imports, group imports by source (std lib > external > internal)
Applied to files:
packages/events/src/kafka/event-dispatch/index.ts
📚 Learning: in the `docker-compose.yaml` file and associated services, sample aes encryption keys (`variables_ae...
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#189
File: docker-compose.yaml:59-59
Timestamp: 2024-11-09T21:03:55.544Z
Learning: In the `docker-compose.yaml` file and associated services, sample AES encryption keys (`VARIABLES_AES_256_KEY`) may be included for demonstration purposes.
Applied to files:
docker-compose.dev.yaml
📚 Learning: applies to **/*.{js,jsx,ts,tsx,json,md,yml,yaml} : formatting: prettier is used with `@ctrlplane/pre...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T22:34:20.754Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,md,yml,yaml} : Formatting: Prettier is used with `@ctrlplane/prettier-config`
Applied to files:
apps/workspace-engine/package.json
📚 Learning: applies to **/*.{ts,tsx} : handle errors explicitly (use try/catch and typed error responses)...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T22:34:20.754Z
Learning: Applies to **/*.{ts,tsx} : Handle errors explicitly (use try/catch and typed error responses)
Applied to files:
packages/events/src/kafka/client.ts
📚 Learning: in apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts, when implementing error...
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#627
File: apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts:0-0
Timestamp: 2025-07-23T22:42:20.933Z
Learning: In apps/event-worker/src/workers/job-update/trigger-dependendent-targets.ts, when implementing error handling for the triggerDependentTargets function, only a try-catch wrapper is needed around the entire function body. Additional null checks after takeFirst() calls are unnecessary since takeFirst throws an error if no record is found.
Applied to files:
packages/events/src/kafka/client.ts
📚 Learning: applies to **/*.{ts,tsx} : consistent type imports: `import type { type } from "module"`...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T22:34:20.754Z
Learning: Applies to **/*.{ts,tsx} : Consistent type imports: `import type { Type } from "module"`
Applied to files:
packages/events/src/kafka/index.ts
📚 Learning: in the ctrlplane codebase, when defining database schemas with drizzle orm, it's an intentional patt...
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#579
File: packages/db/src/schema/rules/concurrency.ts:0-0
Timestamp: 2025-06-01T19:10:11.535Z
Learning: In the ctrlplane codebase, when defining database schemas with Drizzle ORM, it's an intentional pattern to spread base fields (like `basePolicyRuleFields`) and then redefine specific fields to add additional constraints (like unique constraints or foreign key references). The TypeScript field overwriting behavior is deliberately used to override base field definitions with more specific requirements.
Applied to files:
apps/webservice/src/app/api/v1/resources/route.ts
📚 Learning: the `releasechannel` scope type is included in the `scopetype` enum in `packages/db/src/schema/rbac....
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-10-29T02:05:46.185Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
Applied to files:
apps/webservice/src/app/api/v1/resources/route.ts
📚 Learning: run all tests using 'go test ./...'....
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/selector-engine/CLAUDE.md:0-0
Timestamp: 2025-08-04T17:39:09.167Z
Learning: Run all tests using 'go test ./...'.
Applied to files:
apps/workspace-engine/.air.toml
📚 Learning: applies to apps/selector-engine/**/*_test.go : follow the existing test structure in '*_test.go' fil...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/selector-engine/CLAUDE.md:0-0
Timestamp: 2025-08-04T17:39:09.167Z
Learning: Applies to apps/selector-engine/**/*_test.go : Follow the existing test structure in '*_test.go' files.
Applied to files:
apps/workspace-engine/.air.toml
📚 Learning: when running tests, always check for and run any lint/typecheck commands if they exist, such as 'npm...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/selector-engine/CLAUDE.md:0-0
Timestamp: 2025-08-04T17:39:09.167Z
Learning: When running tests, always check for and run any lint/typecheck commands if they exist, such as 'npm run lint', 'npm run typecheck', 'golangci-lint', 'go vet', and 'go fmt'.
Applied to files:
apps/workspace-engine/.air.toml
🧬 Code Graph Analysis (2)
packages/events/src/kafka/client.ts (1)
packages/logger/src/index.ts (1)
logger(48-48)
packages/events/src/kafka/event-dispatch/resource.ts (3)
packages/db/src/schema/resource.ts (1)
resource(59-87)packages/events/src/kafka/client.ts (1)
sendEvent(28-46)apps/workspace-engine/pkg/kafka/consumer.go (1)
Event(20-27)
⏰ 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). (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (17)
docker-compose.dev.yaml (1)
68-70: Double-check auto-topic and delete flags before mirroring to non-dev stacksEnabling
KAFKA_AUTO_CREATE_TOPICS_ENABLEandKAFKA_DELETE_TOPIC_ENABLEis fine for local-dev but dangerous in shared or prod clusters (hard-to-trace topic churn, accidental data loss).
Ensure the compose file used for staging/prod doesn’t inherit these flags.apps/workspace-engine/.air.toml (1)
5-9: Binary name diverges fromnpm run buildoutput
.air.tomlbuilds./tmp/mainwhile thebuildscript outputsworkspace-engine. Divergent names can confuse deployment scripts and health checks.Align the two (e.g.,
-o workspace-engineeverywhere) or document why they differ.apps/workspace-engine/pkg/kafka/consumer.go (1)
37-37: LGTM!Adding broker logging improves observability and debugging capabilities. The refactoring to move signal handling to the main application is also a good architectural decision.
packages/events/src/kafka/client.ts (3)
4-4: LGTM!Good addition of structured logging with proper component scoping.
9-9: LGTM!Creating a child logger with component context is a best practice for structured logging.
31-46: LGTM!Excellent addition of error handling and logging. The try/catch wrapper satisfies the coding guidelines requirement for explicit error handling, and the approach of logging errors without throwing is appropriate for event sending operations to prevent application crashes.
apps/workspace-engine/main.go (5)
9-12: LGTM!Good addition of required imports for signal handling and graceful shutdown.
47-48: LGTM!Creating a cancellable context with proper cleanup is essential for graceful shutdown.
61-61: LGTM!Adding deferred cleanup for the TCP listener ensures proper resource management.
63-63: LGTM!Using structured logging instead of print statements is a good improvement for consistency.
65-71: LGTM!Excellent implementation of graceful shutdown handling. The signal channel setup properly captures SIGINT/SIGTERM signals, and the context cancellation will propagate shutdown to all goroutines including the Kafka consumer.
apps/webservice/src/app/api/v1/resources/route.ts (2)
8-13: LGTM!Good addition of the new event dispatch functions to the imports.
66-76: LGTM!Excellent helper function that properly determines whether to dispatch a resource created or updated event based on the presence of a previous resource. The "api" source parameter correctly identifies the origin of these events.
packages/events/src/kafka/event-dispatch/resource.ts (4)
1-4: LGTM!Good imports and proper type usage from the database schema and Kafka client.
6-17: LGTM!Well-structured resource creation event dispatcher. The event structure is consistent with the Go consumer expectations, and using the resource ID as the event ID provides good correlation capabilities.
19-31: LGTM!Excellent implementation of the resource updated event dispatcher. Including both previous and current resource states in the payload provides comprehensive context for consumers to understand what changed.
33-44: LGTM!Consistent implementation of the resource deletion event dispatcher that follows the same pattern as the other dispatch functions.
| KAFKA_LISTENERS: PLAINTEXT://0.0.0.0:9092,CONTROLLER://0.0.0.0:9093 | ||
| KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://localhost:9092,CONTROLLER://localhost:9093 |
There was a problem hiding this comment.
Advertised listeners will break inter-container connectivity
KAFKA_ADVERTISED_LISTENERS points to localhost, but containers on the Docker network will resolve that to themselves, not to ctrlplane-broker. Producers/consumers in other services therefore cannot reach the broker.
- KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://localhost:9092,CONTROLLER://localhost:9093
+ # Expose the broker’s container hostname so other services can connect
+ KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://ctrlplane-broker:9092,CONTROLLER://ctrlplane-broker:9093📝 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.
| KAFKA_LISTENERS: PLAINTEXT://0.0.0.0:9092,CONTROLLER://0.0.0.0:9093 | |
| KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://localhost:9092,CONTROLLER://localhost:9093 | |
| KAFKA_LISTENERS: PLAINTEXT://0.0.0.0:9092,CONTROLLER://0.0.0.0:9093 | |
| # Expose the broker’s container hostname so other services can connect | |
| KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://ctrlplane-broker:9092,CONTROLLER://ctrlplane-broker:9093 |
🤖 Prompt for AI Agents
In docker-compose.dev.yaml around lines 58 to 59, the KAFKA_ADVERTISED_LISTENERS
environment variable is set to use localhost, which breaks inter-container
communication because other containers resolve localhost to themselves. To fix
this, replace localhost with the actual service name or network alias of the
Kafka broker container (e.g., ctrlplane-broker) so that other containers can
correctly reach the broker.
There was a problem hiding this comment.
The feedback is incorrect for our hybrid setup - we run infrastructure services (Kafka, Redis, DB) in Docker containers but run our applications directly on the host via pnpm dev. Since our host applications connect to Kafka via the exposed port mapping, localhost:9092 in KAFKA_ADVERTISED_LISTENERS is actually the correct configuration.
There was a problem hiding this comment.
this is specifically for our development environment
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the clarification! You're absolutely right - I misunderstood your development architecture. In your hybrid setup where infrastructure services run in Docker containers but applications run directly on the host via pnpm dev, localhost:9092 in KAFKA_ADVERTISED_LISTENERS is indeed the correct configuration.
The host applications connect through the exposed port mapping (9092:9092), so from their perspective, the Kafka broker is accessible at localhost:9092. My initial feedback incorrectly assumed a fully containerized setup where all services communicate via the Docker network.
Your configuration is appropriate for this development pattern where you want the convenience of containerized infrastructure with the flexibility of running applications natively on the host.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#644
File: docker-compose.dev.yaml:58-59
Timestamp: 2025-08-05T20:11:46.624Z
Learning: In the ctrlplane project, the development setup uses a hybrid architecture where infrastructure services (Kafka, Redis, PostgreSQL) run in Docker containers via docker-compose.dev.yaml, but applications run directly on the host using `pnpm dev`. This means KAFKA_ADVERTISED_LISTENERS should use `localhost` addresses since host applications connect through port mappings rather than Docker network communication.
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: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#189
File: docker-compose.yaml:59-59
Timestamp: 2024-11-09T21:03:55.544Z
Learning: In the `docker-compose.yaml` file and associated services, sample AES encryption keys (`VARIABLES_AES_256_KEY`) may be included for demonstration purposes.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#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
PR: ctrlplanedev/ctrlplane#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
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the clarification! You're absolutely right - my feedback was incorrect for your hybrid development setup. Since your applications run directly on the host via pnpm dev and connect to the containerized Kafka broker through the exposed port mapping (9092:9092), localhost:9092 in KAFKA_ADVERTISED_LISTENERS is indeed the correct configuration.
I apologize for the confusion. Your setup makes perfect sense for development where you want the flexibility of running apps on the host while keeping infrastructure services containerized.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#644
File: docker-compose.dev.yaml:58-59
Timestamp: 2025-08-05T20:11:59.855Z
Learning: In ctrlplanedev/ctrlplane, the development environment uses a hybrid setup where infrastructure services (Kafka, Redis, DB) run in Docker containers but applications run directly on the host via pnpm dev. For this setup, KAFKA_ADVERTISED_LISTENERS should use localhost addresses because host applications connect to Kafka via exposed port mapping, not inter-container communication.
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: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#189
File: docker-compose.yaml:59-59
Timestamp: 2024-11-09T21:03:55.544Z
Learning: In the `docker-compose.yaml` file and associated services, sample AES encryption keys (`VARIABLES_AES_256_KEY`) may be included for demonstration purposes.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#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
PR: ctrlplanedev/ctrlplane#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
PR: ctrlplanedev/ctrlplane#601
File: e2e/tests/api/policies/retry-policy.spec.ts:117-130
Timestamp: 2025-06-24T23:53:25.398Z
Learning: User adityachoudhari26 prefers to keep non-null assertions in e2e test code without extensive null safety checks, reasoning that test failures serve the same purpose of catching issues and the extra validation doesn't add much value in test contexts.
Summary by CodeRabbit
New Features
Improvements
Chores