feat: controlplane improve composition queries - #2903
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFour controlplane repositories refactor DB access: SubgraphRepository adds a batched subgraph-name helper and normalizes proto field usage; FeatureFlagRepository bulk-joins schema/proto/plugin metadata and removes transaction-scoped writes; FederatedGraphRepository and GraphCompositionRepository move composition/schema-version writes off transaction callbacks and return a slimmer result for schema-version writes. ChangesRepository Database Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
controlplane/src/core/repositories/FeatureFlagRepository.ts (1)
974-1035: 💤 Low valueMinor inconsistency in
schemaVersionIdfield mapping.The query selects both
schemaVersionId: subgraphs.schemaVersionId(line 925) andsvId: schemaVersion.id(line 932). In the DTO construction,schemaVersionIdis set fromgraph.svId(line 1027), not from the originally selectedschemaVersionId. While functionally equivalent due to the FK constraint, this differs fromSubgraphRepository.getSubgraphsMatchingwhich consistently usessg.schemaVersionId.Consider using the same source field for consistency:
- schemaVersionId: graph.svId ?? '', + schemaVersionId: graph.schemaVersionId ?? '',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/repositories/FeatureFlagRepository.ts` around lines 974 - 1035, The DTO maps schemaVersionId from graph.svId while the query also returns graph.schemaVersionId (and SubgraphRepository uses sg.schemaVersionId); change the mapping in FeatureFlagRepository where featureGraphsByFlag is built (the push that spreads ...graph) to use graph.schemaVersionId (or normalize earlier so a single field is used) instead of graph.svId to keep source consistency with SubgraphRepository.getSubgraphsMatching and the selected query fields.controlplane/src/core/repositories/GraphCompositionRepository.ts (1)
124-137: 💤 Low valueConsider pre-computing schema version lookup map for clarity.
The repeated
composedSubgraphs.indexOf(subgraph)lookups to accesssubgraphSchemaVersionIdsare O(n) per call. While composition subgraph counts are typically small, building aMap<subgraphId, schemaVersionId>upfront would be clearer and more efficient.♻️ Suggested improvement
+ const subgraphSchemaVersionMap = new Map( + composedSubgraphs.map((sg) => [sg.id, sg.schemaVersionId]) + ); + const updatedSubgraphs = composedSubgraphs.filter((subgraph) => { const prevSubgraph = prevCompositionSubgraphs.find((prevSubgraph) => prevSubgraph.id === subgraph.id); return ( - prevSubgraph && prevSubgraph.schemaVersionId !== subgraphSchemaVersionIds[composedSubgraphs.indexOf(subgraph)] + prevSubgraph && prevSubgraph.schemaVersionId !== subgraphSchemaVersionMap.get(subgraph.id) ); }); const unchangedSubgraphs = composedSubgraphs.filter((subgraph) => prevCompositionSubgraphs.some( (prevSubgraph) => prevSubgraph.id === subgraph.id && - prevSubgraph.schemaVersionId === subgraphSchemaVersionIds[composedSubgraphs.indexOf(subgraph)], + prevSubgraph.schemaVersionId === subgraphSchemaVersionMap.get(subgraph.id), ), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/repositories/GraphCompositionRepository.ts` around lines 124 - 137, In GraphCompositionRepository, the filters that compute updatedSubgraphs and unchangedSubgraphs repeatedly call composedSubgraphs.indexOf(subgraph) to index into subgraphSchemaVersionIds (O(n) per lookup); precompute a Map from subgraph id to schemaVersionId (e.g., build subgraphSchemaVersionById from composedSubgraphs and subgraphSchemaVersionIds before the filters) and then use that map inside the updatedSubgraphs and unchangedSubgraphs predicates to compare prevSubgraph.schemaVersionId against the mapped schemaVersionId, removing the indexOf calls and improving clarity and performance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/src/core/repositories/FederatedGraphRepository.ts`:
- Around line 785-790: In FederatedGraphRepository.ts (around the insert that
writes to federatedGraphsToFeatureFlagSchemaVersions), remove the empty-string
fallback for baseCompositionSchemaVersionId and instead ensure a real UUID is
provided: check federatedGraph.composedSchemaVersionId before calling
this.db.insert in the method performing the insert (the block using
federatedGraphsToFeatureFlagSchemaVersions and variables schemaVersionId,
federatedGraph, featureFlagId); if composedSchemaVersionId is null/undefined
either skip this insert for that federatedGraph or throw/return a clear error so
baseCompositionSchemaVersionId is always assigned the actual
composedSchemaVersionId UUID (no '').
- Around line 736-810: The feature-flag branch is inserting an empty string into
federatedGraphsToFeatureFlagSchemaVersions.baseCompositionSchemaVersionId which
violates the FK (column is notNull); in addSchemaVersion change the value
written for baseCompositionSchemaVersionId to a valid UUID when
federatedGraph.composedSchemaVersionId is missing (e.g. use
federatedGraph.composedSchemaVersionId ?? insertedVersion[0].insertedId) or
conditionally omit/adjust the insert so only a valid schemaVersion id is stored;
update the insertion in the isFeatureFlagComposition branch where
federatedGraphsToFeatureFlagSchemaVersions is written to reference
baseCompositionSchemaVersionId and ensure insertedVersion is used as fallback.
---
Nitpick comments:
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 974-1035: The DTO maps schemaVersionId from graph.svId while the
query also returns graph.schemaVersionId (and SubgraphRepository uses
sg.schemaVersionId); change the mapping in FeatureFlagRepository where
featureGraphsByFlag is built (the push that spreads ...graph) to use
graph.schemaVersionId (or normalize earlier so a single field is used) instead
of graph.svId to keep source consistency with
SubgraphRepository.getSubgraphsMatching and the selected query fields.
In `@controlplane/src/core/repositories/GraphCompositionRepository.ts`:
- Around line 124-137: In GraphCompositionRepository, the filters that compute
updatedSubgraphs and unchangedSubgraphs repeatedly call
composedSubgraphs.indexOf(subgraph) to index into subgraphSchemaVersionIds (O(n)
per lookup); precompute a Map from subgraph id to schemaVersionId (e.g., build
subgraphSchemaVersionById from composedSubgraphs and subgraphSchemaVersionIds
before the filters) and then use that map inside the updatedSubgraphs and
unchangedSubgraphs predicates to compare prevSubgraph.schemaVersionId against
the mapped schemaVersionId, removing the indexOf calls and improving clarity and
performance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5414b1e4-bddc-45f1-b0ae-3ecd2a06539a
📒 Files selected for processing (4)
controlplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/GraphCompositionRepository.tscontrolplane/src/core/repositories/SubgraphRepository.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2903 +/- ##
==========================================
+ Coverage 65.13% 65.54% +0.41%
==========================================
Files 327 327
Lines 47138 46882 -256
Branches 5241 5242 +1
==========================================
+ Hits 30703 30731 +28
+ Misses 16411 16127 -284
Partials 24 24
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.