fix: show feature flags for a federated graph under split config - #3129
fix: show feature flags for a federated graph under split config#3129JivusAyrus wants to merge 20 commits into
Conversation
|
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:
WalkthroughFeature-flag resolution now uses the latest valid composition for a federated graph and target. Split-config-loading retains flags from previous valid compositions and reports failed latest compositions. API contracts, integration tests, and Studio stale-composition indicators were updated. ChangesFeature-flag composition scoping
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
controlplane/test/composition/getCompositionDetails.test.ts (1)
597-600: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest federated-graph isolation, not only positive retrieval. Each test uses one graph, so removing
federatedGraphIdfiltering would still pass.
controlplane/test/composition/getCompositionDetails.test.ts#L597-L600: add another graph and assert its flag composition is excluded.controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts#L121-L135: assert only the requested graph’s flags are returned.controlplane/test/federated-graph/get-federated-graph-by-id.test.ts#L243-L245: assert the response excludes a flag composed for another graph.🤖 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/test/composition/getCompositionDetails.test.ts` around lines 597 - 600, Add federated-graph isolation assertions at all three sites: in controlplane/test/composition/getCompositionDetails.test.ts:597-600, create a flag composition for another graph and assert it is excluded; in controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts:121-135, assert only flags from the requested graph are returned; and in controlplane/test/federated-graph/get-federated-graph-by-id.test.ts:243-245, assert the response excludes a flag composed for another graph.controlplane/test/federated-graph/get-federated-graph-by-id.test.ts (1)
195-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit callback annotations.
The newly added test, cleanup, and
labels.mapcallbacks rely on inferred parameter and return types. Annotate them to follow the TypeScript guideline.🤖 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/test/federated-graph/get-federated-graph-by-id.test.ts` around lines 195 - 224, Add explicit parameter and return type annotations to the callbacks introduced in the test, including the test callback, testContext.onTestFinished cleanup callback, and labels.map callback. Keep the existing callback behavior unchanged while following the repository’s TypeScript typing guideline.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@controlplane/test/composition/getCompositionDetails.test.ts`:
- Around line 597-600: Add federated-graph isolation assertions at all three
sites: in controlplane/test/composition/getCompositionDetails.test.ts:597-600,
create a flag composition for another graph and assert it is excluded; in
controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts:121-135,
assert only flags from the requested graph are returned; and in
controlplane/test/federated-graph/get-federated-graph-by-id.test.ts:243-245,
assert the response excludes a flag composed for another graph.
In `@controlplane/test/federated-graph/get-federated-graph-by-id.test.ts`:
- Around line 195-224: Add explicit parameter and return type annotations to the
callbacks introduced in the test, including the test callback,
testContext.onTestFinished cleanup callback, and labels.map callback. Keep the
existing callback behavior unchanged while following the repository’s TypeScript
typing guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b82d47b6-fa3c-4d4e-ac52-1899e0edd73a
📒 Files selected for processing (7)
controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/getCompositionDetails.tscontrolplane/src/core/bufservices/federated-graph/getFederatedGraphById.tscontrolplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/test/composition/getCompositionDetails.test.tscontrolplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.tscontrolplane/test/federated-graph/get-federated-graph-by-id.test.ts
…ompositionDetails
There was a problem hiding this comment.
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/FeatureFlagRepository.ts (1)
1488-1494: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDeclare the explicit return type as
Promise<Array<{ id: string; featureFlagId: string | null }> | undefined>. The database column permitsnull.🤖 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 1488 - 1494, Add an explicit return type annotation to the getFeatureFlagSchemaVersionsByBaseSchemaVersion method. The return type should be Promise<Array<{ id: string; featureFlagId: string | null }> | undefined> to properly account for the fact that the database column permits null values for the featureFlagId field.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@controlplane/src/core/repositories/FeatureFlagRepository.ts`:
- Around line 1488-1494: Add an explicit return type annotation to the
getFeatureFlagSchemaVersionsByBaseSchemaVersion method. The return type should
be Promise<Array<{ id: string; featureFlagId: string | null }> | undefined> to
properly account for the fact that the database column permits null values for
the featureFlagId field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36fed3a3-7f8b-4340-bd59-771e59bab8db
📒 Files selected for processing (3)
controlplane/src/core/bufservices/federated-graph/getCompositionDetails.tscontrolplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/test/composition/getCompositionDetails.test.ts
There was a problem hiding this comment.
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/bufservices/federated-graph/getFederatedGraphById.ts (1)
76-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter out disabled feature flags from
featureFlagsInLatestValidComposition.This loop pushes any flag whose ID appears in
ffsInLatestValidComposition, without checkingflag.isEnabled.featureFlagsincludes disabled flags becausegetFeatureFlagsByFederatedGraphcallsgetFeatureFlagsByBaseSubgraphIdAndLabelMatcherswithexcludeDisabled: false.The sibling handler
getFeatureFlagsInLatestCompositionByFederatedGraph.tsfilters the same repository result byflag.isEnabledand documents why: a disabled flag's router config is removed without recomposing, so its schema version rows still exist but should not be reported as part of the latest composition. Before this PR's fix, split-config mode likely returned no rows here, so this gap stayed hidden. Now that the fix makes the repository method return real rows under split config, disabled flags can appear infeatureFlagsInLatestValidCompositionhere, inconsistent with the sibling handler.Apply the same
isEnabledcheck in this loop.🐛 Proposed fix to exclude disabled flags
if (ffsInLatestValidComposition) { for (const ff of ffsInLatestValidComposition) { const flag = featureFlags.find((f) => f.id === ff.featureFlagId); - if (flag) { + // A disabled feature flag is no longer served in the latest composition (its router config is + // removed without recomposing), so exclude it even though its schema version rows still exist. + if (flag && flag.isEnabled) { featureFlagsInLatestValidComposition.push(flag); } } }🤖 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/bufservices/federated-graph/getFederatedGraphById.ts` around lines 76 - 89, Update the loop in getFederatedGraphById that builds featureFlagsInLatestValidComposition to append a matched flag only when it exists and flag.isEnabled is true. Preserve the existing featureFlags lookup and skip disabled flags consistently with getFeatureFlagsInLatestCompositionByFederatedGraph.
🤖 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.
Outside diff comments:
In `@controlplane/src/core/bufservices/federated-graph/getFederatedGraphById.ts`:
- Around line 76-89: Update the loop in getFederatedGraphById that builds
featureFlagsInLatestValidComposition to append a matched flag only when it
exists and flag.isEnabled is true. Preserve the existing featureFlags lookup and
skip disabled flags consistently with
getFeatureFlagsInLatestCompositionByFederatedGraph.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6921879b-ba0f-445d-92ed-3df3a8fe331b
📒 Files selected for processing (3)
controlplane/src/core/bufservices/feature-flag/getFeatureFlagsInLatestCompositionByFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/getFederatedGraphById.tscontrolplane/src/core/repositories/FeatureFlagRepository.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts`:
- Around line 340-342: Update the assertion to use the same comparator on both
sides of the sort. The resp.featureFlags array is sorted using the default
comparator, while the expected array is sorted using localeCompare, which can
produce different orders for mixed-case alphanumeric names. Either apply
localeCompare to both arrays or use an order-independent comparison approach
like Set equality to ensure consistent results.
🪄 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: 6adac01c-c789-4af2-9bbe-5925e09c4718
📒 Files selected for processing (1)
controlplane/test/feature-flag/get-feature-flags-in-latest-composition-by-federated-graph.test.ts
…for-a-federated-graph' and 'suvij/cosmo-315-controlplane-feature-flags-not-shown-for-a-federated-graph' of github.com:wundergraph/cosmo into suvij/cosmo-315-controlplane-feature-flags-not-shown-for-a-federated-graph
…ot-shown-for-a-federated-graph
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Router-nonroot image scan failed❌ Security vulnerabilities found in image: Please check the security vulnerabilities found in the PR. If you believe this is a false positive, please add the vulnerability to the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ompositions link and update usage in schema pages
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ot-shown-for-a-federated-graph
…r-a-federated-graph' of github.com:wundergraph/cosmo into suvij/cosmo-315-controlplane-feature-flags-not-shown-for-a-federated-graph
…315-controlplane-feature-flags-not-shown-for-a-federated-graph
…315-controlplane-feature-flags-not-shown-for-a-federated-graph
…ot-shown-for-a-federated-graph
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist
In case the ff composition fails
