Conversation
WalkthroughThis change introduces a new "workspace-engine" application with its own Dockerfile, ESLint and TypeScript configurations, package manifest, environment configuration, and core source files implementing a Kafka consumer and a workspace engine class. It refactors Kafka client initialization in the events package for lazy loading and expands the events package to re-export Kafka-related modules, enhancing modularity and public API surface. Changes
Sequence Diagram(s)sequenceDiagram
participant Kafka as Kafka Broker
participant Consumer as WorkspaceEngine Consumer
participant EngineMap as WorkspaceEngine Map
participant Engine as WorkspaceEngine Instance
Kafka->>Consumer: Push message (key, value)
Consumer->>Consumer: Validate key as UUID
alt Key valid
Consumer->>EngineMap: Get or create WorkspaceEngine by ID
Consumer->>Engine: Pass message value to readMessage()
Engine->>Engine: Parse and log message
else Key invalid
Consumer->>Consumer: Log error, skip message
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/events/src/kafka/index.ts (1)
1-2: Import extension choice may hinder IDE toolingUsing the
.jsextension in files that live undersrc/sometimes confuses TypeScript language-services (jump-to-definition, auto-imports) unlessallowImportingTsExtensionsis enabled in every consumer tsconfig.If runtime requires a file extension, consider the pattern:
export * from "./client"; // no extension – resolves to .ts during dev, .js after buildor document in
README.mdthatallowImportingTsExtensions: trueis mandatory for downstream packages.packages/events/src/index.ts (1)
40-40: Star-re-export could mask duplicate symbol names
export * from "./kafka/index.js";indiscriminately re-exports everything which can silently overwrite exports coming from lines 36-39 if any names collide later.
If the public surface is small, prefer explicit named exports orexport * as kafka.apps/workspace-engine/src/config.ts (1)
12-15: Trim & de-dupe broker list aftersplitEnvironment strings often contain spaces:
"host1:9092, host2:9092 ,host1:9092".
A quick sanitize avoids subtle connection issues:- .transform((val) => val.split(",")), + .transform((val) => + [...new Set(val.split(",").map((s) => s.trim()).filter(Boolean))] + ),apps/workspace-engine/tsconfig.json (1)
7-8:tsBuildInfoFileis ignored withoutincremental: trueTypeScript only writes the build-info file when
incremental(orcomposite) is enabled.
Either add"incremental": trueor droptsBuildInfoFileto avoid dead config.+ "incremental": true,apps/workspace-engine/src/workspace-engine.ts (1)
21-21: Implement actual message processing logic.The
await Promise.resolve()is a placeholder. Consider implementing the actual message processing logic or adding a TODO comment to track this requirement.Would you like me to help implement the message processing logic or create an issue to track this TODO?
apps/workspace-engine/src/index.ts (1)
39-43: Consider consistent error logging.The error logging for invalid keys uses a different logger instance than the invalid message logging above. Consider using the component-specific logger for consistency.
+const log = logger.child({ component: "WorkspaceEngine" }); + - logger.error("Invalid key", { key }); + log.error("Invalid key", { key });Also apply the same change to line 35:
- logger.error("Invalid message", { message }); + log.error("Invalid message", { message });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/workspace-engine/Dockerfile(1 hunks)apps/workspace-engine/eslint.config.js(1 hunks)apps/workspace-engine/package.json(1 hunks)apps/workspace-engine/src/config.ts(1 hunks)apps/workspace-engine/src/index.ts(1 hunks)apps/workspace-engine/src/workspace-engine.ts(1 hunks)apps/workspace-engine/tsconfig.json(1 hunks)packages/events/src/index.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/index.tspackages/events/src/kafka/index.tsapps/workspace-engine/src/index.tsapps/workspace-engine/src/config.tsapps/workspace-engine/src/workspace-engine.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/index.tspackages/events/src/kafka/index.tsapps/workspace-engine/src/index.tsapps/workspace-engine/src/config.tsapps/workspace-engine/src/workspace-engine.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/index.tspackages/events/src/kafka/index.tsapps/workspace-engine/eslint.config.jsapps/workspace-engine/src/index.tsapps/workspace-engine/package.jsonapps/workspace-engine/src/config.tsapps/workspace-engine/tsconfig.jsonapps/workspace-engine/src/workspace-engine.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:07.352Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
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} : 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/index.tspackages/events/src/kafka/index.tsapps/workspace-engine/eslint.config.jsapps/workspace-engine/tsconfig.json
📚 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/index.tsapps/workspace-engine/eslint.config.jsapps/workspace-engine/tsconfig.json
📚 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/eslint.config.jsapps/workspace-engine/package.jsonapps/workspace-engine/tsconfig.json
📚 Learning: in the ctrlplane project, they use next.js standalone output mode, which means they intentionally ex...
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#363
File: apps/webservice/tsconfig.json:14-15
Timestamp: 2025-03-09T08:56:53.603Z
Learning: In the ctrlplane project, they use Next.js standalone output mode, which means they intentionally exclude `.next/types` in their tsconfig.json despite including `.next/types/**/*.ts` in their include pattern.
Applied to files:
apps/workspace-engine/eslint.config.jsapps/workspace-engine/package.jsonapps/workspace-engine/tsconfig.json
📚 Learning: applies to **/*.{ts,tsx} : use typescript with explicit types (prefer interfaces for public apis)...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T22:34:20.754Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript with explicit types (prefer interfaces for public APIs)
Applied to files:
apps/workspace-engine/eslint.config.jsapps/workspace-engine/tsconfig.json
📚 Learning: applies to **/*.{ts,tsx} : prefer async/await over raw promises...
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T22:34:20.754Z
Learning: Applies to **/*.{ts,tsx} : Prefer async/await over raw promises
Applied to files:
apps/workspace-engine/eslint.config.js
📚 Learning: in the codebase, deployments are decoupled from environments. when deleting environments (e.g., in `...
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#187
File: apps/jobs/src/ephemeral-env-checker/index.ts:57-0
Timestamp: 2024-10-30T23:10:58.869Z
Learning: In the codebase, deployments are decoupled from environments. When deleting environments (e.g., in `apps/jobs/src/ephemeral-env-checker/index.ts`), associated deployments should not be deleted.
Applied to files:
apps/workspace-engine/src/config.ts
📚 Learning: in `apps/webservice/src/app/[workspaceslug]/_components/release-channel-drawer/usage.tsx`, within th...
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:07.352Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
Applied to files:
apps/workspace-engine/src/config.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/workspace-engine/tsconfig.json
🪛 Hadolint (2.12.0)
apps/workspace-engine/Dockerfile
[error] 48-48: Use absolute WORKDIR
(DL3000)
⏰ 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: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (5)
apps/workspace-engine/src/index.ts (3)
9-14: LGTM: Well-structured Kafka consumer setup.The Kafka client and consumer configuration is properly set up with appropriate client ID and consumer group. The use of environment variables for broker configuration provides good flexibility.
16-25: LGTM: Efficient workspace engine caching pattern.The Map-based caching of WorkspaceEngine instances is an efficient approach that avoids creating duplicate instances for the same workspace. The factory function pattern is clean and well-implemented.
27-51: LGTM: Robust message processing with proper validation.The consumer implementation properly validates message keys as UUIDs and handles invalid messages gracefully. The integration with the WorkspaceEngine instances is well-structured.
apps/workspace-engine/package.json (1)
1-33: LGTM: Well-configured package manifest.The package.json is properly configured with:
- Appropriate workspace package setup with private flag
- ES modules configuration matching the source code
- Comprehensive scripts for development, building, and code quality
- Dependencies that align with the source code usage (kafkajs, zod, events, logger)
- Proper development tooling and Prettier configuration per coding guidelines
apps/workspace-engine/Dockerfile (1)
1-50: LGTM: Well-structured multi-stage Docker build.The Dockerfile follows good practices with:
- Appropriate Alpine base image with necessary build dependencies
- Efficient layer caching through strategic file copying
- Proper pnpm and turbo setup for monorepo builds
- Security-conscious non-root user creation
- Production environment configuration
| export class WorkspaceEngine { | ||
| constructor(private workspaceId: string) {} | ||
|
|
||
| async readMessage(message: Buffer<ArrayBufferLike>) { |
There was a problem hiding this comment.
Fix the Buffer type definition.
The Buffer<ArrayBufferLike> type is incorrect. Buffer doesn't take a generic type parameter.
- async readMessage(message: Buffer<ArrayBufferLike>) {
+ async readMessage(message: Buffer) {📝 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.
| async readMessage(message: Buffer<ArrayBufferLike>) { | |
| async readMessage(message: Buffer) { |
🤖 Prompt for AI Agents
In apps/workspace-engine/src/workspace-engine.ts at line 10, the method
readMessage incorrectly uses Buffer with a generic type parameter
Buffer<ArrayBufferLike>. Remove the generic parameter so that the type is simply
Buffer. Update the method signature to use Buffer without any type arguments to
fix the type definition.
| const messageData: Message<keyof EventPayload> = JSON.parse( | ||
| message.toString(), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add runtime validation for message parsing.
The type assertion Message<keyof EventPayload> assumes the parsed JSON matches the expected structure without validation. Consider using a schema validator like zod to ensure type safety at runtime.
+import { z } from "zod";
+
+const MessageSchema = z.object({
+ // Define your Message schema here based on EventPayload structure
+});
+
- const messageData: Message<keyof EventPayload> = JSON.parse(
- message.toString(),
- );
+ const parsed = JSON.parse(message.toString());
+ const messageData = MessageSchema.parse(parsed);🤖 Prompt for AI Agents
In apps/workspace-engine/src/workspace-engine.ts around lines 12 to 14, the code
parses a JSON message and asserts its type without runtime validation, which
risks runtime errors if the data structure is incorrect. To fix this, define a
schema for Message using a validation library like zod, then parse and validate
the JSON message against this schema. Replace the direct JSON.parse and type
assertion with schema parsing that throws or handles errors if the data does not
conform to the expected structure.
|
#36 207.9 @ctrlplane/webservice:build: ✓ Compiled successfully something in this PR is causing this error with building the webservice package, can you help trace what it is |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/events/src/kafka/client.ts (1)
18-24: Consider improving error handling for producer connection.The current implementation doesn't handle connection failures properly. If
producer.connect()fails, the producer remains in the global variable but disconnected, and subsequent calls won't retry connection.Consider this improvement:
const getProducer = async () => { if (producer == null) { - producer = getKafka().producer(); - await producer.connect(); + try { + producer = getKafka().producer(); + await producer.connect(); + } catch (error) { + producer = null; // Reset on failure to allow retry + throw error; + } } return producer; };This ensures connection failures don't leave the producer in an invalid state.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/events/src/kafka/client.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/client.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/client.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/client.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#188
File: apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx:42-44
Timestamp: 2024-11-01T02:35:07.352Z
Learning: In `apps/webservice/src/app/[workspaceSlug]/_components/release-channel-drawer/Usage.tsx`, within the `Usage` component, the arrays used for filtering inherited environments are expected to remain small. Therefore, performance optimizations for this filtering logic are not necessary.
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.
⏰ 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)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/events/src/kafka/client.ts (2)
1-1: LGTM! Proper setup for lazy loading pattern.The type import and nullable global variables correctly establish the foundation for lazy initialization, addressing the import-time execution issue mentioned in the PR objectives.
Also applies to: 7-8
30-30: LGTM! Correct implementation of lazy producer usage.The change to use
await getProducer()properly implements the lazy loading pattern, ensuring the producer is initialized and connected only when needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Aditya Choudhari <48932219+adityachoudhari26@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Aditya Choudhari <48932219+adityachoudhari26@users.noreply.github.com>
packages/events/src/kafka/client.ts
Outdated
| const getKafka = () => { | ||
| kafka ??= new Kafka({ | ||
| clientId: "ctrlplane-events", | ||
| brokers: env.KAFKA_BROKERS.split(","), | ||
| }); | ||
| return kafka; | ||
| }; |
There was a problem hiding this comment.
| const getKafka = () => { | |
| kafka ??= new Kafka({ | |
| clientId: "ctrlplane-events", | |
| brokers: env.KAFKA_BROKERS.split(","), | |
| }); | |
| return kafka; | |
| }; | |
| const getKafka = () => | |
| (kafka ??= new Kafka({ | |
| clientId: "ctrlplane-events", | |
| brokers: env.KAFKA_BROKERS.split(","), | |
| })); | |
apps/workspace-engine/src/config.ts
Outdated
| import dotenv from "dotenv"; | ||
| import { z } from "zod"; | ||
|
|
||
| dotenv.config(); |
Summary by CodeRabbit
New Features
Chores