chore: init basic ts repo #664
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant R as DbResourceRepository
participant TX as Tx (optional)
participant DB as db (default)
rect rgba(220,230,241,0.8)
note right of R: Workspace-scoped CRUD & exists
C->>R: create/get/getAll/update/delete/exists
alt tx provided
R->>TX: execute query (scoped by workspaceId)
TX-->>R: row(s) / boolean
else no tx
R->>DB: execute query (scoped by workspaceId)
DB-->>R: row(s) / boolean
end
R-->>C: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 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)
✨ 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: 4
🧹 Nitpick comments (6)
apps/event-queue/src/repository/repository.ts (4)
3-10: Unify return types with a MaybePromise alias (or standardize on Promises).Unioning Promise | T across all methods complicates consumption. Introduce a MaybePromise alias to improve readability (or consider making the interface fully async).
Apply this diff:
-type Entity = { id: string }; +export type Entity = { id: string }; +export type MaybePromise<T> = T | Promise<T>; export interface Repository<T extends Entity> { - get(id: string): Promise<T | null> | T | null; - getAll(): Promise<T[]> | T[]; - create(entity: T): Promise<T> | T; - update(entity: T): Promise<T> | T; - delete(id: string): Promise<T | null> | T | null; - exists(id: string): Promise<boolean> | boolean; + get(id: string): MaybePromise<T | null>; + getAll(): MaybePromise<T[]>; + create(entity: T): MaybePromise<T>; + update(entity: T): MaybePromise<T>; + delete(id: string): MaybePromise<T | null>; + exists(id: string): MaybePromise<boolean>; }
1-2: Export Entity for reuse.External modules may benefit from reusing the Entity constraint; export it explicitly.
Apply the first hunk of the previous diff (exporting Entity).
25-29: Avoid non-null assertion and improve typing in update().Return the value you just assigned to eliminate the non-null assertion and keep typing precise.
Apply this diff:
- update(entity: T): T { - const existing = this.entities[entity.id] ?? {}; - this.entities[entity.id] = { ...existing, ...entity }; - return this.entities[entity.id]!; - } + update(entity: T): T { + const existing = (this.entities[entity.id] ?? {}) as Partial<T>; + const updated = { ...existing, ...entity } as T; + this.entities[entity.id] = updated; + return updated; + }
12-21: Optional: prefer Map over Record for the in-memory store.Map avoids prototype pitfalls, offers clearer intent for key/value stores, and provides better iteration semantics.
If you decide to switch later, I can provide a targeted diff to convert Record<string, T> usage to Map<string, T>.
apps/event-queue/src/repository/db-resource-repository.ts (2)
9-15: Optional: clarify transaction usage in the constructor docstring.The tx param is great. Consider noting that when omitted it uses the shared db client; callers should pass tx for atomic multi-step operations.
Happy to add a brief JSDoc explaining behavior and example usage with a Tx.
9-64: Schema/indexing note: ensure uniqueness expectations match scoping.If resource.id is only unique per-workspace (not globally), enforce a composite unique index on (workspaceId, id). If id is globally unique, the additional workspace filter remains a defense-in-depth guard.
Would you like me to draft a migration for the appropriate unique index based on your schema expectations?
📜 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/repository/db-resource-repository.ts(1 hunks)apps/event-queue/src/repository/repository.ts(1 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/repository/db-resource-repository.tsapps/event-queue/src/repository/repository.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/repository/db-resource-repository.tsapps/event-queue/src/repository/repository.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/repository/db-resource-repository.tsapps/event-queue/src/repository/repository.ts
🧬 Code graph analysis (2)
apps/event-queue/src/repository/db-resource-repository.ts (3)
apps/event-queue/src/repository/repository.ts (1)
Repository(3-10)packages/db/src/common.ts (3)
Tx(22-22)takeFirstOrNull(15-20)takeFirst(9-13)packages/db/src/client.ts (1)
db(15-15)
apps/event-queue/src/repository/repository.ts (1)
apps/workspace-engine/pkg/model/types.go (1)
Entity(5-7)
⏰ 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 (1)
apps/event-queue/src/repository/db-resource-repository.ts (1)
17-34: LGTM: read operations are correctly scoped to workspace.get() and getAll() constrain by workspaceId. Good guardrails for multi-tenant safety.
| async update(entity: schema.Resource) { | ||
| return this.db | ||
| .update(schema.resource) | ||
| .set(entity) | ||
| .where(eq(schema.resource.id, entity.id)) | ||
| .returning() | ||
| .then(takeFirst); | ||
| } |
There was a problem hiding this comment.
Scope update() to workspace and prevent workspaceId/id drift.
- Current WHERE only matches by id, which can update a record in a different workspace.
- .set(entity) may overwrite workspaceId (or attempt to update id).
Constrain by workspaceId and force workspaceId in the SET payload; avoid updating id.
Apply this diff:
- async update(entity: schema.Resource) {
- return this.db
+ async update(entity: schema.Resource) {
+ const { id, workspaceId: _ignored, ...rest } = entity;
+ return this.db
.update(schema.resource)
- .set(entity)
- .where(eq(schema.resource.id, entity.id))
+ .set({ ...rest, workspaceId: this.workspaceId })
+ .where(
+ and(
+ eq(schema.resource.id, id),
+ eq(schema.resource.workspaceId, this.workspaceId),
+ ),
+ )
.returning()
.then(takeFirst);
}📝 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 update(entity: schema.Resource) { | |
| return this.db | |
| .update(schema.resource) | |
| .set(entity) | |
| .where(eq(schema.resource.id, entity.id)) | |
| .returning() | |
| .then(takeFirst); | |
| } | |
| async update(entity: schema.Resource) { | |
| const { id, workspaceId: _ignored, ...rest } = entity; | |
| return this.db | |
| .update(schema.resource) | |
| .set({ ...rest, workspaceId: this.workspaceId }) | |
| .where( | |
| and( | |
| eq(schema.resource.id, id), | |
| eq(schema.resource.workspaceId, this.workspaceId), | |
| ), | |
| ) | |
| .returning() | |
| .then(takeFirst); | |
| } |
🤖 Prompt for AI Agents
In apps/event-queue/src/repository/db-resource-repository.ts around lines 42 to
49, the update() currently matches only by id and blindly .set(entity), which
can modify/attempt to modify id or update a row in another workspace; change the
WHERE to include workspaceId (e.g., where id = entity.id AND workspaceId =
entity.workspaceId), ensure the SET payload never contains id (omit id from the
object being set) and explicitly force workspaceId in the SET payload to
entity.workspaceId so workspaceId cannot drift.
There was a problem hiding this comment.
ID is globally unique in our postgres db
There was a problem hiding this comment.
@adityachoudhari26 Thank you for that clarification! If IDs are globally unique in your PostgreSQL database, then you're absolutely right - there's no risk of updating records across different workspaces when matching by ID alone.
My suggestion to add workspace scoping to the WHERE clause is unnecessary overhead in your case. The current approach with eq(schema.resource.id, entity.id) is perfectly safe.
The only remaining consideration would be preventing accidental workspaceId changes in the SET payload, but given your database design, this is more of a defensive programming practice than a critical issue.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#664
File: apps/event-queue/src/repository/db-resource-repository.ts:42-49
Timestamp: 2025-08-26T20:10:10.042Z
Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.
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.
| async delete(id: string) { | ||
| return this.db | ||
| .delete(schema.resource) | ||
| .where(eq(schema.resource.id, id)) | ||
| .returning() | ||
| .then(takeFirstOrNull); | ||
| } |
There was a problem hiding this comment.
Scope delete() to workspace.
Deleting by id without workspace scoping can remove records across tenants.
Apply this diff:
async delete(id: string) {
return this.db
.delete(schema.resource)
- .where(eq(schema.resource.id, id))
+ .where(
+ and(
+ eq(schema.resource.id, id),
+ eq(schema.resource.workspaceId, this.workspaceId),
+ ),
+ )
.returning()
.then(takeFirstOrNull);
}📝 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 delete(id: string) { | |
| return this.db | |
| .delete(schema.resource) | |
| .where(eq(schema.resource.id, id)) | |
| .returning() | |
| .then(takeFirstOrNull); | |
| } | |
| async delete(id: string) { | |
| return this.db | |
| .delete(schema.resource) | |
| .where( | |
| and( | |
| eq(schema.resource.id, id), | |
| eq(schema.resource.workspaceId, this.workspaceId), | |
| ), | |
| ) | |
| .returning() | |
| .then(takeFirstOrNull); | |
| } |
There was a problem hiding this comment.
ID is globally unique
There was a problem hiding this comment.
@adityachoudhari26 Thank you for the clarification! You're absolutely right. Since resource IDs are globally unique across the entire database, workspace scoping in the WHERE clause is indeed unnecessary for the delete() method. My suggestion was based on the incorrect assumption that IDs might only be unique within workspace boundaries.
The current implementation is correct as-is - querying by a globally unique ID will never accidentally affect resources from other workspaces.
I appreciate the correction and will keep this architectural decision in mind for future reviews of this codebase.
🐰
✨ Ah, learning something new!
( ◡ ◡ )
\ ▽ /
-----
🧠 Learnings used
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#664
File: apps/event-queue/src/repository/db-resource-repository.ts:42-49
Timestamp: 2025-08-26T20:10:10.042Z
Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.
| async exists(id: string) { | ||
| return this.db | ||
| .select() | ||
| .from(schema.resource) | ||
| .where(eq(schema.resource.id, id)) | ||
| .then(takeFirstOrNull) | ||
| .then((r) => r != null); | ||
| } |
There was a problem hiding this comment.
Scope exists() to workspace and avoid selecting all columns.
exists() should be tenant-safe; also prefer selecting a minimal column.
Apply this diff:
async exists(id: string) {
return this.db
- .select()
+ .select({ id: schema.resource.id })
.from(schema.resource)
- .where(eq(schema.resource.id, id))
+ .where(
+ and(
+ eq(schema.resource.id, id),
+ eq(schema.resource.workspaceId, this.workspaceId),
+ ),
+ )
.then(takeFirstOrNull)
.then((r) => r != null);
}📝 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 exists(id: string) { | |
| return this.db | |
| .select() | |
| .from(schema.resource) | |
| .where(eq(schema.resource.id, id)) | |
| .then(takeFirstOrNull) | |
| .then((r) => r != null); | |
| } | |
| async exists(id: string) { | |
| return this.db | |
| .select({ id: schema.resource.id }) | |
| .from(schema.resource) | |
| .where( | |
| and( | |
| eq(schema.resource.id, id), | |
| eq(schema.resource.workspaceId, this.workspaceId), | |
| ), | |
| ) | |
| .then(takeFirstOrNull) | |
| .then((r) => r != null); | |
| } |
🤖 Prompt for AI Agents
In apps/event-queue/src/repository/db-resource-repository.ts around lines 57 to
64, the exists() query currently selects all columns and is not scoped to the
workspace/tenant; update it to only select the minimal column (resource id) and
add a where clause restricting results to the current workspace (e.g., where
eq(schema.resource.workspaceId, this.workspaceId) or the appropriate workspace
id field on the resource), then return whether a row exists — this makes the
check tenant-safe and avoids fetching unnecessary columns.
There was a problem hiding this comment.
ID is globally unique
There was a problem hiding this comment.
@adityachoudhari26, thank you for the clarification! You're absolutely right - I have this noted in my knowledge base from our previous interaction that resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping is not necessary when querying by ID.
My suggestion to add workspace scoping to the exists() method was incorrect. The current implementation is already correct in terms of scoping. The only potential optimization would be selecting minimal columns (select({ id: schema.resource.id }) instead of select()), but that's a minor performance consideration and not a correctness issue.
(つ˶ᵔ ᵕ ᵔ˶)つ curious rabbit learning
🧠 Learnings used
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#601
File: packages/job-dispatch/src/job-update.ts:264-270
Timestamp: 2025-06-24T23:56:54.799Z
Learning: In this codebase, the `Tx` type is just an alias for the database client type (`Omit<typeof db, "$client">`) and does not necessarily indicate an active transaction context. Functions like `createReleaseJob` need to be called within a transaction, which is why they are wrapped with `db.transaction()` even when the parameter is typed as `Tx`. Drizzle supports nested transactions via breakpoints, so additional transaction wrappers are safe even if already within a transaction.
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#664
File: apps/event-queue/src/repository/db-resource-repository.ts:42-49
Timestamp: 2025-08-26T20:10:10.042Z
Learning: In the ctrlplane codebase, resource IDs are globally unique across the entire PostgreSQL database, so workspace scoping in WHERE clauses is not necessary when querying by ID.
Summary by CodeRabbit