feat: add new endpoints for fetching feature flags and feature subgraphs by federated graph#2729
Merged
JivusAyrus merged 22 commits intomainfrom Apr 16, 2026
Conversation
…phs by federated graph
JivusAyrus
requested review from
StarpTech,
endigma,
thisisnithin and
wilsonrivera
as code owners
Contributor
|
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:
WalkthroughThis PR refactors feature flags and feature subgraphs out of Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
controlplane/src/core/repositories/FeatureFlagRepository.ts (1)
495-512: Avoid 2N+1 lookups after paginating.This path paginates in SQL, but each returned row still does
byTargetId()andbyId()serially. With the current 50-item cap, one page can still trigger 100+ extra DB queries. Folding the missing fields into the main query, or batch-loading them by ID, would keep the new endpoint’s latency predictable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/FeatureFlagRepository.ts` around lines 495 - 512, In FeatureFlagRepository where featureSubgraphTargets is iterated, avoid N+1 by collecting all needed IDs (all targetId values and all baseSubgraphId values from featureSubgraphTargets) and batch-loading them once (e.g., using subgraphRepo.byIds or a new subgraphRepo.byTargetIds/byIds method) before the loop; then map the returned subgraph rows by id/targetId and populate featureSubgraphs from those maps (or alternatively fold the missing fields into the primary SQL with joins) instead of calling subgraphRepo.byTargetId and subgraphRepo.byId for each item.controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts (1)
65-66: Assert the exact result set here.This setup creates one eligible flag, but
toBeGreaterThanOrEqual(1)plussome(...)still allows duplicates or unrelated extras. Tightening this to the exact returned IDs/names—or adding a second ineligible flag to the fixture—would actually lock down the “only flags in the latest composition” contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts` around lines 65 - 66, The test currently uses a loose assertion (expect(resp.featureFlags.length).toBeGreaterThanOrEqual(1) plus some(...)), so tighten it to assert the exact returned set: either change the length assertion to expect(resp.featureFlags.length).toBe(1) and assert the single entry matches flagName (e.g., resp.featureFlags[0].name === flagName) or replace both assertions with a deterministic comparison of names/ids (e.g., compare resp.featureFlags.map(f=>f.name) to [flagName]); alternatively, add a second ineligible flag to the fixture and assert the response contains only the eligible flag(s) to lock down the “only flags in the latest composition” contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.ts`:
- Around line 65-84: The loop discards the composed schema-version id from
getFeatureFlagSchemaVersionsByBaseSchemaVersion (ffsInLatestValidComposition)
and re-reads the mutable current flag via featureFlagRepo.getFeatureFlagById,
causing drift; update the loop to load the exact composed flag version using the
schema-version identifier present on each ff (e.g., ff.schemaVersionId or
ff.featureFlagSchemaVersionId) and call the repository method that returns that
specific schema-version record (instead of getFeatureFlagById), then push that
version into featureFlags so the response reflects the flags as they were in the
latest composition.
In
`@controlplane/src/core/bufservices/feature-flag/getFeatureSubgraphsByFederatedGraph.ts`:
- Around line 70-87: The featureSubgraphs mapping in
getFeatureSubgraphsByFederatedGraph (inside the featureSubgraphs:
featureSubgraphs.map(...) block) omits createdUserId, causing inconsistent
payloads vs getFeatureSubgraphs; add createdUserId: fs.createdUserId to the
object returned by the map, and update any related type/interface (feature
subgraph DTO or return type) if necessary so createdUserId is included
throughout; keep other fields (e.g., routingURL, subscriptionProtocol,
websocketSubprotocol, type via convertToSubgraphType) unchanged.
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 437-439: getFeatureSubgraphs currently restricts the query filter
to like(targets.name, `%${query}%`) which breaks "Search by ID or Name"; modify
the query handling so the conditions array includes an OR branch that also
checks subgraphs.id for an exact match when query is a UUID (or always include
equality on subgraphs.id alongside the name like). Locate the code that pushes
like(targets.name, `%${query}%`) and add a complementary condition that checks
subgraphs.id (use equals/subgraphs.id === query or the repository's equality
helper) so searches by feature-subgraph ID continue to return results.
In
`@controlplane/test/feature-flag/get-feature-subgraphs-by-federated-graph.test.ts`:
- Around line 313-323: Replace the loose .some() check with a strict cardinality
assertion: for the call to client.getFeatureSubgraphsByFederatedGraph in this
test, verify that resp.featureSubgraphs contains exactly one entry named
"users-feature" (e.g., count/filter the entries with s.name === 'users-feature'
and expect the count to equal 1) and optionally assert that
resp.featureSubgraphs has no duplicate names by comparing its length to the
length of the set/unique list of names; update the test that references
resp.featureSubgraphs to enforce these exact-unique expectations.
In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/playground.tsx:
- Around line 745-763: Extract the feature-flag name resolution into a new
constant (e.g., selectedFeatureFlagName) by reading
compositionFlagsData?.featureFlags and finding the flag with id ===
loadSchemaGraphId, and then modify the useQuery call for
getFederatedGraphSDLByName to include an enabled condition so it only runs when
either type !== 'featureFlag' or (type === 'featureFlag' && compositionFlagsData
has resolved and selectedFeatureFlagName is defined); update the query's
featureFlagName to use selectedFeatureFlagName so the SDL query does not fire
with undefined and avoids the unnecessary re-fetch/flicker.
---
Nitpick comments:
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 495-512: In FeatureFlagRepository where featureSubgraphTargets is
iterated, avoid N+1 by collecting all needed IDs (all targetId values and all
baseSubgraphId values from featureSubgraphTargets) and batch-loading them once
(e.g., using subgraphRepo.byIds or a new subgraphRepo.byTargetIds/byIds method)
before the loop; then map the returned subgraph rows by id/targetId and populate
featureSubgraphs from those maps (or alternatively fold the missing fields into
the primary SQL with joins) instead of calling subgraphRepo.byTargetId and
subgraphRepo.byId for each item.
In
`@controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts`:
- Around line 65-66: The test currently uses a loose assertion
(expect(resp.featureFlags.length).toBeGreaterThanOrEqual(1) plus some(...)), so
tighten it to assert the exact returned set: either change the length assertion
to expect(resp.featureFlags.length).toBe(1) and assert the single entry matches
flagName (e.g., resp.featureFlags[0].name === flagName) or replace both
assertions with a deterministic comparison of names/ids (e.g., compare
resp.featureFlags.map(f=>f.name) to [flagName]); alternatively, add a second
ineligible flag to the fixture and assert the response contains only the
eligible flag(s) to lock down the “only flags in the latest composition”
contract.
🪄 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: a8ab9dd9-6e5d-4ab3-ab4c-328fa99f892d
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (16)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.tscontrolplane/src/core/bufservices/feature-flag/getFeatureSubgraphsByFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/getFederatedGraphByName.tscontrolplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.tscontrolplane/test/feature-flag/get-feature-subgraphs-by-federated-graph.test.tsproto/wg/cosmo/platform/v1/platform.protostudio/src/components/layout/graph-layout.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/playground.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/index.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/schema/sdl.tsxstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/subgraphs.tsx
💤 Files with no reviewable changes (2)
- studio/src/components/layout/graph-layout.tsx
- controlplane/src/core/bufservices/federated-graph/getFederatedGraphByName.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/playground.tsx (1)
745-763:⚠️ Potential issue | 🟠 MajorPrevent feature-flag mode from loading SDL before flag resolution.
Line 756 still executes the SDL query before the selected flag name exists (Line 761), so feature-flag mode can fetch base schema first and re-fetch after flags load. Also, the flag lookup is duplicated in three places (Line 761, Line 1012, Line 1067), which risks drift.
💡 Proposed fix
const { data: compositionFlagsData } = useQuery( getFeatureFlagsInLatestCompositionByFederatedGraph, @@ }, ); + const selectedFeatureFlagName = + type === 'featureFlag' + ? (compositionFlagsData?.featureFlags ?? []).find((f) => f.id === loadSchemaGraphId)?.name + : undefined; - const { data, isLoading: isLoadingGraphSchema } = useQuery(getFederatedGraphSDLByName, { - name: graphContext?.graph?.name, - namespace: graphContext?.graph?.namespace, - featureFlagName: - type === 'featureFlag' - ? (compositionFlagsData?.featureFlags ?? []).find((f) => f.id === loadSchemaGraphId)?.name - : undefined, - }); + const { data, isLoading: isLoadingGraphSchema } = useQuery( + getFederatedGraphSDLByName, + { + name: graphContext?.graph?.name, + namespace: graphContext?.graph?.namespace, + featureFlagName: selectedFeatureFlagName, + }, + { + enabled: !!graphContext?.graph?.name && (type !== 'featureFlag' || !!selectedFeatureFlagName), + }, + ); @@ graphContext?.graph?.id || '', - type === 'featureFlag' - ? (compositionFlagsData?.featureFlags ?? []).find((f) => f.id === loadSchemaGraphId)?.name - : undefined, + selectedFeatureFlagName, ), @@ - if (type === 'featureFlag') { - const featureFlag = (compositionFlagsData?.featureFlags ?? []).find((f) => f.id === loadSchemaGraphId); - if (featureFlag) { - requestHeaders['X-Feature-Flag'] = featureFlag.name; - } + if (selectedFeatureFlagName) { + requestHeaders['X-Feature-Flag'] = selectedFeatureFlagName; } @@ - compositionFlagsData?.featureFlags, + selectedFeatureFlagName, @@ - compositionFlagsData?.featureFlags, + selectedFeatureFlagName,Also applies to: 1011-1013, 1066-1070
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/playground.tsx around lines 745 - 763, Prevent the SDL query from running in feature-flag mode until the flags are loaded and the selected flag is resolved: compute a single selectedFlagName variable by deriving it once from compositionFlagsData, loadSchemaGraphId and use that name wherever needed (replace the repeated .find usages at the three spots), and pass featureFlagName: selectedFlagName to the getFederatedGraphSDLByName useQuery; also set the useQuery enabled flag to only run when either type !== 'featureFlag' or selectedFlagName is truthy so the base SDL is not fetched prematurely.controlplane/src/core/repositories/FeatureFlagRepository.ts (1)
437-439:⚠️ Potential issue | 🟡 MinorKeep feature-subgraph ID search working.
This new path only filters on
targets.name, so UUID searches that already work ingetFeatureSubgraphs()stop returning results here.💡 Proposed fix
if (query) { - conditions.push(like(targets.name, `%${query}%`)); + conditions.push(isValidUuid(query) ? eq(subgraphs.id, query) : like(targets.name, `%${query}%`)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/FeatureFlagRepository.ts` around lines 437 - 439, The current query filter only matches targets.name so UUID searches stop returning results; in the method that builds conditions (where you have if (query) { conditions.push(like(targets.name, `%${query}%`)); }) update it to also check the feature-subgraph id—e.g., add a second condition on targets.id (exact match) or OR a like on targets.id when query looks like a UUID—so getFeatureSubgraphs()/the same query path will return results for ID searches as well; modify the conditions.push call(s) to include targets.id alongside targets.name (and optionally detect UUID format to prefer exact match).controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.ts (1)
69-83:⚠️ Potential issue | 🟠 MajorThis returns current flag state, not the latest composed snapshot.
getFeatureFlagSchemaVersionsByBaseSchemaVersion()gives you the persisted composition membership, but this loop throws that record away and reloads the mutable live flag byfeatureFlagId. If a flag is disabled, relabeled, or deleted after that composition, this RPC will drift from what the latest composition actually contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.ts` around lines 69 - 83, The code currently discards the composition membership returned by getFeatureFlagSchemaVersionsByBaseSchemaVersion (ffsInLatestValidComposition) and reloads live flags via featureFlagRepo.getFeatureFlagById, which returns the current mutable state instead of the snapshot that was part of the composition; change the lookup to fetch the versioned/snapshot data tied to the composition record (e.g., use the schema-version-scoped accessor instead of getFeatureFlagById) — for example call a repository method that returns the feature-flag snapshot by featureFlagId + base/schema version (or use the data already present on the ff record) so you populate featureFlags from the composed snapshot (reference symbols: getFeatureFlagSchemaVersionsByBaseSchemaVersion, ffsInLatestValidComposition, ff.featureFlagId, featureFlagRepo.getFeatureFlagById, featureFlags).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 467-484: The ORDER BY in FeatureFlagRepository's query uses only
targets.createdAt (baseQuery.orderBy(asc(targets.createdAt))) which is
non-deterministic for identical timestamps; update the ordering to include a
stable tiebreaker (e.g., secondary sort by targets.id or
subgraphsToFederatedGraph.id) so pagination with limit/offset is deterministic,
and keep the count query as-is; locate where baseQuery.orderBy(...) is called
and add the secondary asc(...) tiebreaker to the same orderBy chain.
In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/playground.tsx:
- Around line 619-629: The query for composition flags is being enabled only by
graphContext?.graph?.name but reads router.query.namespace into the namespace
const without guarding for its availability, which can fire a request with a
missing namespace during router hydration; update how namespace is derived
(allow undefined) and change the useQuery enabled option for
getFeatureFlagsInLatestCompositionByFederatedGraph to require both
graphContext?.graph?.name and namespace (e.g., enabled:
!!graphContext?.graph?.name && !!namespace) so the query only runs when
namespace is present.
---
Duplicate comments:
In
`@controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.ts`:
- Around line 69-83: The code currently discards the composition membership
returned by getFeatureFlagSchemaVersionsByBaseSchemaVersion
(ffsInLatestValidComposition) and reloads live flags via
featureFlagRepo.getFeatureFlagById, which returns the current mutable state
instead of the snapshot that was part of the composition; change the lookup to
fetch the versioned/snapshot data tied to the composition record (e.g., use the
schema-version-scoped accessor instead of getFeatureFlagById) — for example call
a repository method that returns the feature-flag snapshot by featureFlagId +
base/schema version (or use the data already present on the ff record) so you
populate featureFlags from the composed snapshot (reference symbols:
getFeatureFlagSchemaVersionsByBaseSchemaVersion, ffsInLatestValidComposition,
ff.featureFlagId, featureFlagRepo.getFeatureFlagById, featureFlags).
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 437-439: The current query filter only matches targets.name so
UUID searches stop returning results; in the method that builds conditions
(where you have if (query) { conditions.push(like(targets.name, `%${query}%`));
}) update it to also check the feature-subgraph id—e.g., add a second condition
on targets.id (exact match) or OR a like on targets.id when query looks like a
UUID—so getFeatureSubgraphs()/the same query path will return results for ID
searches as well; modify the conditions.push call(s) to include targets.id
alongside targets.name (and optionally detect UUID format to prefer exact
match).
In `@studio/src/pages/`[organizationSlug]/[namespace]/graph/[slug]/playground.tsx:
- Around line 745-763: Prevent the SDL query from running in feature-flag mode
until the flags are loaded and the selected flag is resolved: compute a single
selectedFlagName variable by deriving it once from compositionFlagsData,
loadSchemaGraphId and use that name wherever needed (replace the repeated .find
usages at the three spots), and pass featureFlagName: selectedFlagName to the
getFederatedGraphSDLByName useQuery; also set the useQuery enabled flag to only
run when either type !== 'featureFlag' or selectedFlagName is truthy so the base
SDL is not fetched prematurely.
🪄 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: 244afbfd-1f02-4c3d-bc07-467895044470
📒 Files selected for processing (5)
controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.tscontrolplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.tscontrolplane/test/feature-flag/get-feature-subgraphs-by-federated-graph.test.tsstudio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/playground.tsx
✅ Files skipped from review due to trivial changes (2)
- controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts
- controlplane/test/feature-flag/get-feature-subgraphs-by-federated-graph.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
487-490:⚠️ Potential issue | 🟠 MajorUse
schema.targets.namespaceIdin the mixed RBAC branch.Line 489 compares namespace IDs against
schema.targets.id, which will not match. Actors with both namespace-scoped and resource-scoped graph access lose the namespace portion of their permissions, causinglist()to under-return graphs they should access.Suggested change
conditions.push( - or(inArray(schema.targets.id, [...new Set(namespaces)]), inArray(schema.targets.id, [...new Set(resources)])), + or( + inArray(schema.targets.namespaceId, [...new Set(namespaces)]), + inArray(schema.targets.id, [...new Set(resources)]), + ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/FederatedGraphRepository.ts` around lines 487 - 490, In FederatedGraphRepository (the mixed RBAC branch inside the list/filter construction), change the left-hand field used for namespace checks from schema.targets.id to schema.targets.namespaceId so namespace-scoped permissions are matched correctly; specifically, update the or(...) call that currently does or(inArray(schema.targets.id, [...new Set(namespaces)]), inArray(schema.targets.id, [...new Set(resources)])) to use schema.targets.namespaceId for the namespaces side while keeping schema.targets.id for resource-scoped checks and the existing de-duplication with new Set.
♻️ Duplicate comments (1)
controlplane/src/core/repositories/FeatureFlagRepository.ts (1)
467-468: 🛠️ Refactor suggestion | 🟠 MajorAdd a stable tiebreaker to the paginated ordering.
ORDER BY targets.createdAtalone is not deterministic when multiple rows share the same timestamp, sooffset-based paging can duplicate or skip feature subgraphs between pages.💡 Proposed fix
.innerJoin(namespaces, eq(namespaces.id, targets.namespaceId)) .where(and(...conditions, eq(subgraphsToFederatedGraph.federatedGraphId, federatedGraphId))) - .orderBy(asc(targets.createdAt)); + .orderBy(asc(targets.createdAt), asc(subgraphs.id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/FeatureFlagRepository.ts` around lines 467 - 468, The ORDER BY in FeatureFlagRepository using .orderBy(asc(targets.createdAt)) is not deterministic and can cause duplicates/skips during offset paging; update the query that builds the ordering (the call chaining ending with .orderBy(...)) to add a stable tiebreaker such as the primary key (e.g., targets.id or targets.pk) and include it with the same sort direction (e.g., .orderBy(asc(targets.createdAt), asc(targets.id))) so rows with identical createdAt values are consistently ordered.
🧹 Nitpick comments (3)
controlplane/src/core/repositories/FeatureFlagRepository.ts (2)
251-253: Redundant condition can be simplified.Since
isOrganizationVieweris defined asisOrganizationAdminOrDeveloper || this.roles.includes('organization-viewer')(perRBACEvaluator.ts:80), checkingisOrganizationAdminOrDeveloperseparately is redundant.💡 Proposed simplification
private applyRbacConditionsToQuery(rbac: RBACEvaluator | undefined, conditions: (SQL<unknown> | undefined)[]) { - if (!rbac || rbac.isOrganizationAdminOrDeveloper || rbac.isOrganizationViewer) { + if (!rbac || rbac.isOrganizationViewer) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/FeatureFlagRepository.ts` around lines 251 - 253, The condition in FeatureFlagRepository (variable rbac) redundantly checks rbac.isOrganizationAdminOrDeveloper and rbac.isOrganizationViewer; since RBACEvaluator's isOrganizationViewer already includes isOrganizationAdminOrDeveloper, simplify the guard to only check for absence of rbac or rbac.isOrganizationViewer (i.e., replace the current if (!rbac || rbac.isOrganizationAdminOrDeveloper || rbac.isOrganizationViewer) return true; with if (!rbac || rbac.isOrganizationViewer) return true;), referencing the RBACEvaluator properties to justify the change.
489-505: N+1 query pattern could be optimized with batch fetching.Each iteration executes two separate DB queries (
byTargetIdandbyId). With a page of N results, this generates 2N additional queries. Consider collecting alltargetIds andbaseSubgraphIds upfront and fetching them in bulk.💡 Proposed batch approach
const featureSubgraphTargets = await baseQuery.execute(); + // Batch fetch all required data + const targetIds = featureSubgraphTargets.map((f) => f.targetId); + const baseSubgraphIds = [...new Set(featureSubgraphTargets.map((f) => f.baseSubgraphId))]; + + const [subgraphsByTarget, baseSubgraphsById] = await Promise.all([ + subgraphRepo.byTargetIds(targetIds), // Would need to add this batch method + subgraphRepo.byIds(baseSubgraphIds), // Would need to add this batch method + ]); + + const targetMap = new Map(subgraphsByTarget.map((s) => [s.targetId, s])); + const baseMap = new Map(baseSubgraphsById.map((s) => [s.id, s])); + const featureSubgraphs: FeatureSubgraphDTO[] = []; for (const f of featureSubgraphTargets) { - const fs = await subgraphRepo.byTargetId(f.targetId); + const fs = targetMap.get(f.targetId); if (!fs) { continue; } - const baseSubgraph = await subgraphRepo.byId(f.baseSubgraphId); + const baseSubgraph = baseMap.get(f.baseSubgraphId); if (!baseSubgraph) { continue; }Note: This would require adding batch methods to
SubgraphRepositoryif they don't exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/FeatureFlagRepository.ts` around lines 489 - 505, The loop over featureSubgraphTargets causes an N+1 query pattern by calling subgraphRepo.byTargetId and subgraphRepo.byId per item; collect all targetIds and baseSubgraphIds from featureSubgraphTargets first, add or use batch methods on SubgraphRepository (e.g., byTargetIds(targetIds[]) and byIds(baseIds[])) to fetch maps of Subgraph objects in two bulk queries, then iterate featureSubgraphTargets and look up fs and baseSubgraph from those maps when building featureSubgraphs (replace the per-item calls to byTargetId and byId).controlplane/src/core/repositories/SubgraphRepository.ts (1)
685-685: Add an explicit return type to the new static helper.Callers branch on this value as a boolean, so
: booleanshould be part of the signature instead of being inferred.Suggested change
- static applyRbacConditionsToQuery(rbac: RBACEvaluator | undefined, conditions: (SQL<unknown> | undefined)[]) { + static applyRbacConditionsToQuery( + rbac: RBACEvaluator | undefined, + conditions: (SQL<unknown> | undefined)[], + ): boolean {As per coding guidelines,
**/*.{ts,tsx}: Use explicit type annotations for function parameters and return types in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/repositories/SubgraphRepository.ts` at line 685, The static helper applyRbacConditionsToQuery currently relies on an inferred return type but callers branch on it as a boolean—update its signature to explicitly declare a boolean return type by adding ": boolean" to the method declaration (static applyRbacConditionsToQuery(rbac: RBACEvaluator | undefined, conditions: (SQL<unknown> | undefined)[]): boolean) and ensure the implementation returns a boolean value in all code paths to satisfy the explicit annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@controlplane/src/core/bufservices/federated-graph/getFederatedGraphByName.ts`:
- Around line 55-61: Remove the two raw console.log separator calls around the
subgraphRepo.listByFederatedGraph call in getFederatedGraphByName; either delete
them or replace with a single structured logger.debug (using the module's
logger) if you need trace info, and keep the call to
subgraphRepo.listByFederatedGraph({ federatedGraphTargetId:
federatedGraph.targetId, published: false, rbac: authContext.rbac }) unchanged.
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Around line 985-1023: The mapper in listByFederatedGraph is creating a
ProtoSubgraph for every gRPC subgraph even when schemaVersionId is missing and
it fails to populate plugin metadata for grpc_plugin; update the mapping so that
proto is only created when sg.schemaVersionId (or svSchemaSDL equivalent) exists
(matching getSubgraph/byTargetId behavior) and, when sg.type === 'grpc_plugin',
populate proto.pluginData with the plugin image/metadata fields (the same
properties getSubgraph used previously — e.g., plugin image/name/version fields
from the sg record) so the SubgraphDTO/ProtoSubgraph contract is preserved
between listByFederatedGraph and getSubgraph.
- Around line 975-981: The join on schema.protobufSchemaVersions currently
switches between innerJoin and leftJoin based on data.published which causes
non-gRPC subgraphs to be dropped when published is true; change the dynamic join
to always use leftJoin for the protobufSchemaVersions join (the call that
currently uses [data.published ? 'innerJoin' : 'leftJoin'] with
schema.protobufSchemaVersions and the ON clause containing
inArray(schema.subgraphs.type, ['grpc_plugin','grpc_service']) and
eq(schema.subgraphs.schemaVersionId,
schema.protobufSchemaVersions.schemaVersionId)), and make the same change in the
analogous joins in FederatedGraphRepository (the recomposition logic) and
createFederatedGraph so protobuf fields remain nullable and non-gRPC subgraphs
are preserved.
---
Outside diff comments:
In `@controlplane/src/core/repositories/FederatedGraphRepository.ts`:
- Around line 487-490: In FederatedGraphRepository (the mixed RBAC branch inside
the list/filter construction), change the left-hand field used for namespace
checks from schema.targets.id to schema.targets.namespaceId so namespace-scoped
permissions are matched correctly; specifically, update the or(...) call that
currently does or(inArray(schema.targets.id, [...new Set(namespaces)]),
inArray(schema.targets.id, [...new Set(resources)])) to use
schema.targets.namespaceId for the namespaces side while keeping
schema.targets.id for resource-scoped checks and the existing de-duplication
with new Set.
---
Duplicate comments:
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 467-468: The ORDER BY in FeatureFlagRepository using
.orderBy(asc(targets.createdAt)) is not deterministic and can cause
duplicates/skips during offset paging; update the query that builds the ordering
(the call chaining ending with .orderBy(...)) to add a stable tiebreaker such as
the primary key (e.g., targets.id or targets.pk) and include it with the same
sort direction (e.g., .orderBy(asc(targets.createdAt), asc(targets.id))) so rows
with identical createdAt values are consistently ordered.
---
Nitpick comments:
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 251-253: The condition in FeatureFlagRepository (variable rbac)
redundantly checks rbac.isOrganizationAdminOrDeveloper and
rbac.isOrganizationViewer; since RBACEvaluator's isOrganizationViewer already
includes isOrganizationAdminOrDeveloper, simplify the guard to only check for
absence of rbac or rbac.isOrganizationViewer (i.e., replace the current if
(!rbac || rbac.isOrganizationAdminOrDeveloper || rbac.isOrganizationViewer)
return true; with if (!rbac || rbac.isOrganizationViewer) return true;),
referencing the RBACEvaluator properties to justify the change.
- Around line 489-505: The loop over featureSubgraphTargets causes an N+1 query
pattern by calling subgraphRepo.byTargetId and subgraphRepo.byId per item;
collect all targetIds and baseSubgraphIds from featureSubgraphTargets first, add
or use batch methods on SubgraphRepository (e.g., byTargetIds(targetIds[]) and
byIds(baseIds[])) to fetch maps of Subgraph objects in two bulk queries, then
iterate featureSubgraphTargets and look up fs and baseSubgraph from those maps
when building featureSubgraphs (replace the per-item calls to byTargetId and
byId).
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Line 685: The static helper applyRbacConditionsToQuery currently relies on an
inferred return type but callers branch on it as a boolean—update its signature
to explicitly declare a boolean return type by adding ": boolean" to the method
declaration (static applyRbacConditionsToQuery(rbac: RBACEvaluator | undefined,
conditions: (SQL<unknown> | undefined)[]): boolean) and ensure the
implementation returns a boolean value in all code paths to satisfy the explicit
annotation.
🪄 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: 45080fc1-6313-4c83-86f4-56a00831d643
📒 Files selected for processing (6)
controlplane/src/core/bufservices/federated-graph/getFederatedGraphByName.tscontrolplane/src/core/bufservices/workspace/getWorkspace.tscontrolplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/services/WorkspaceService.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/test/test-util.ts`:
- Line 119: The fallback default for Keycloak API URL in variable apiUrl was
changed to use port 8090; revert this so that const apiUrl =
process.env.KC_API_URL || 'http://localhost:8080' (restore port 8080) in
controlplane/test/test-util.ts so tests use the same Keycloak port as the test
environment when KC_API_URL is not set; update the literal fallback value only
(leave process.env.KC_API_URL usage intact).
🪄 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: 2e01267e-afb0-4c4b-8381-32d9f97b94bd
📒 Files selected for processing (3)
controlplane/src/core/bufservices/federated-graph/getFederatedGraphByName.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/test/test-util.ts
💤 Files with no reviewable changes (1)
- controlplane/src/core/bufservices/federated-graph/getFederatedGraphByName.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/repositories/SubgraphRepository.ts
thisisnithin
approved these changes
Apr 13, 2026
This comment has been minimized.
This comment has been minimized.
…t-feature-flag-data-out-of' into suvij/eng-9327-controlplane-split-feature-flag-data-out-of
…27-controlplane-split-feature-flag-data-out-of
wilsonrivera
approved these changes
Apr 15, 2026
JivusAyrus
deleted the
suvij/eng-9327-controlplane-split-feature-flag-data-out-of
branch
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
GetFeatureFlagsInLatestCompositionByFederatedGraphretrieves feature flags in the latest composition;GetFeatureSubgraphsByFederatedGraphretrieves feature subgraphs with pagination and search support.Checklist