perf: only load initial resources once #669
Conversation
WalkthroughThe in-memory resource initialization was refactored to accept externally provided FullResource[] data. Repository factory creation was removed. Selector create methods now require initialEntities. Workspace now fetches initial resources once, then passes them into selector and repository constructors during load. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Workspace
participant DB
participant SelectorMgr
participant DepSel as InMemoryDeploymentResourceSelector
participant EnvSel as InMemoryEnvironmentResourceSelector
participant Repo as InMemoryResourceRepository
App->>Workspace: load(workspaceId)
Workspace->>DB: getInitialResources(workspaceId)
DB-->>Workspace: FullResource[] initialResources
Workspace->>SelectorMgr: createSelectorManager(initialResources)
activate SelectorMgr
SelectorMgr->>DepSel: create(workspaceId, initialResources)
DepSel-->>SelectorMgr: instance (seeded)
SelectorMgr->>EnvSel: create(workspaceId, initialResources)
EnvSel-->>SelectorMgr: instance (seeded)
deactivate SelectorMgr
Workspace->>Repo: new InMemoryResourceRepository({ initialEntities: initialResources })
Repo-->>Workspace: repository instance
Note over DepSel,EnvSel: Initial match computation runs against provided initialEntities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/event-queue/src/selector/in-memory/environment-resource.ts (1)
62-77: Guard against stale metadata in seeded resources
initialEntitiesare now the only source of resource data when seeding this selector. Before this change we always pulled fresh rows (including metadata) from the DB insidecreate(), but now the selector trusts whatever array the caller supplies. If a consumer accidentally hands us entities that weren’t derived from the latest DB state, we’ll insert outdated entries intocomputedEnvironmentResourceand start returning inconsistent matches. Either add a defensive refresh (e.g., fetch metadata here when the caller doesn’t supply it) or assert that every entity includes current metadata so we fail fast instead of persisting incorrect rows.apps/event-queue/src/repository/in-memory/resource.ts (1)
19-24: Avoid persisting mismatched in-memory stateWe now accept
initialEntitiesfrom the outside and immediately mirror them to the DB on writes. If the caller seeds the repository with stale or partially loaded resources, our mutations will happily overwrite canonical rows with that stale data. Consider enforcing thatinitialEntitiesoriginate from a trusted loader (or refreshing from the DB during construction) so we can guarantee the in-memory cache is consistent before we start writing back. At minimum, validate the shape/version of each entity before storing it.apps/event-queue/src/selector/in-memory/deployment-resource.ts (1)
62-77: Guard against stale metadata in seeded resourcesSame concern as the environment selector: we no longer hit the DB for resource state when seeding. If
initialEntitiesare stale or missing metadata, we’ll recomputecomputedDeploymentResourcerows with incorrect data and return wrong matches. Please add a validation/refresh step so we can trust the data we persist.
🧹 Nitpick comments (1)
apps/event-queue/src/workspace/workspace.ts (1)
50-81: Ensure initial resource snapshot stays in syncWe’re now building the single source of truth for
initialResourceshere. If other startup paths callWorkspace.loadbefore metadata or resource tables settle, we risk propagating incomplete data to both selectors and the in-memory repository. Consider wrapping this fetch in a transaction (or version check) to guarantee a consistent snapshot before we pass it around.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/event-queue/src/repository/in-memory/resource.ts(1 hunks)apps/event-queue/src/selector/in-memory/deployment-resource.ts(3 hunks)apps/event-queue/src/selector/in-memory/environment-resource.ts(3 hunks)apps/event-queue/src/workspace/workspace.ts(4 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/in-memory/deployment-resource.tsapps/event-queue/src/workspace/workspace.tsapps/event-queue/src/selector/in-memory/environment-resource.tsapps/event-queue/src/repository/in-memory/resource.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/in-memory/deployment-resource.tsapps/event-queue/src/workspace/workspace.tsapps/event-queue/src/selector/in-memory/environment-resource.tsapps/event-queue/src/repository/in-memory/resource.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/in-memory/deployment-resource.tsapps/event-queue/src/workspace/workspace.tsapps/event-queue/src/selector/in-memory/environment-resource.tsapps/event-queue/src/repository/in-memory/resource.ts
🧠 Learnings (1)
📚 Learning: 2025-06-24T23:56:54.799Z
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.
Applied to files:
apps/event-queue/src/repository/in-memory/resource.ts
🧬 Code graph analysis (3)
apps/event-queue/src/selector/in-memory/deployment-resource.ts (1)
packages/events/src/kafka/events.ts (1)
FullResource(62-64)
apps/event-queue/src/workspace/workspace.ts (5)
packages/events/src/kafka/events.ts (1)
FullResource(62-64)apps/event-queue/src/selector/in-memory/deployment-resource.ts (1)
InMemoryDeploymentResourceSelector(30-257)apps/event-queue/src/selector/in-memory/environment-resource.ts (1)
InMemoryEnvironmentResourceSelector(30-260)apps/event-queue/src/repository/repository.ts (1)
WorkspaceRepository(55-133)apps/event-queue/src/repository/in-memory/resource.ts (1)
InMemoryResourceRepository(15-60)
apps/event-queue/src/selector/in-memory/environment-resource.ts (1)
packages/events/src/kafka/events.ts (1)
FullResource(62-64)
⏰ 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: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: Typecheck
| .groupBy((row) => row.resource.id) | ||
| .map((group) => { | ||
| const [first] = group; | ||
| if (first == null) return null; | ||
| const { resource } = first; | ||
| const metadata = Object.fromEntries( | ||
| group | ||
| .map((r) => r.resource_metadata) | ||
| .filter(isPresent) | ||
| .map((m) => [m.key, m.value]), | ||
| ); | ||
| return { ...resource, metadata }; | ||
| }) | ||
| .value() | ||
| .filter(isPresent); |
There was a problem hiding this comment.
Bug: metadata lookup uses wrong property name
row.resource_metadata will always be undefined—Drizzle returns camelCase keys (row.resourceMetadata). As written, we drop every metadata row and seed selectors/repositories with empty metadata. That breaks any selector predicates relying on metadata and risks wiping metadata when the in-memory repository writes back. Please swap this to row.resourceMetadata.
Apply the following diff:
- .map((r) => r.resource_metadata)
+ .map((r) => r.resourceMetadata)📝 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.
| .groupBy((row) => row.resource.id) | |
| .map((group) => { | |
| const [first] = group; | |
| if (first == null) return null; | |
| const { resource } = first; | |
| const metadata = Object.fromEntries( | |
| group | |
| .map((r) => r.resource_metadata) | |
| .filter(isPresent) | |
| .map((m) => [m.key, m.value]), | |
| ); | |
| return { ...resource, metadata }; | |
| }) | |
| .value() | |
| .filter(isPresent); | |
| .groupBy((row) => row.resource.id) | |
| .map((group) => { | |
| const [first] = group; | |
| if (first == null) return null; | |
| const { resource } = first; | |
| const metadata = Object.fromEntries( | |
| group | |
| .map((r) => r.resourceMetadata) | |
| .filter(isPresent) | |
| .map((m) => [m.key, m.value]), | |
| ); | |
| return { ...resource, metadata }; | |
| }) | |
| .value() | |
| .filter(isPresent); |
🤖 Prompt for AI Agents
In apps/event-queue/src/workspace/workspace.ts around lines 66 to 80, the code
reads row.resource_metadata which is always undefined because Drizzle returns
camelCase keys; replace usages of row.resource_metadata with
row.resourceMetadata so metadata entries are correctly collected (update the
.map and any related filters to use row.resourceMetadata and ensure
isPresent/type checks still apply).
There was a problem hiding this comment.
this is straight up wrong lol
Summary by CodeRabbit
No changes to user-facing APIs or behavior.