feat: fix recomposition bugs and optimise recompositions#2860
Merged
wilsonrivera merged 7 commits intoMay 13, 2026
Merged
Conversation
wilsonrivera
requested review from
Aenimus,
JivusAyrus,
StarpTech and
thisisnithin
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
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 adds FeatureFlagRepository.getFeatureFlagsBySubgraphLabels, uses it from SubgraphRepository.update to collect affected feature flags and federated graphs with split-config-loading gating, and updates integration tests and GraphQL fixtures to assert recomposition behavior for base and mutual subgraph changes. ChangesFeature Flag Recomposition on Subgraph Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. ✨ Finishing Touches📝 Generate docstrings
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![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: 3
🧹 Nitpick comments (1)
controlplane/test/feature-flag/feature-flag-integration.test.ts (1)
34-34: ⚡ Quick winAvoid committing debug mode enabled by default.
isDebugMode = truemakes the suite run in debug timing mode for normal runs; this is easy to forget and slows CI feedback.💡 Suggested fix
-const isDebugMode = true; +const isDebugMode = false;🤖 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/feature-flag/feature-flag-integration.test.ts` at line 34, The test constant isDebugMode is hard-coded to true causing tests to run in debug timing mode; change isDebugMode to default to false (or read from an explicit CI/env flag like process.env.DEBUG_MODE or a test CLI flag) so normal CI runs are not slowed. Update the declaration of isDebugMode and any test setup that references it (search for isDebugMode in feature-flag-integration.test.ts and related helpers) to use the new default or environment-driven value and ensure any local debug override is documented in test README or env variable.
🤖 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/FeatureFlagRepository.ts`:
- Around line 1044-1055: The loop over matchingFeatureFlags populates result
with FeatureFlagWithFeatureSubgraphs even when, after calling
getFeatureSubgraphsByFeatureFlagId and filtering out entries with empty
schemaVersionId, the featureSubgraphs array is empty; change the logic in the
loop (the block around getFeatureSubgraphsByFeatureFlagId and the result.push
that builds the FeatureFlagWithFeatureSubgraphs) to only push a feature flag
into result when the filtered featureSubgraphs.length > 0 so flags without any
publishable feature subgraphs are excluded.
- Around line 1016-1037: The query currently omits any label predicate when
uniqueLabels is empty, causing all feature flags in the namespace to match;
update the logic that builds conditions (the block using uniqueLabels,
arrayOverlaps, featureFlags.labels, and joinLabel) so that when
uniqueLabels.length === 0 you add a predicate that prevents matches (e.g., a
constant false predicate or an explicit check that cannot be true) instead of
omitting the label condition; ensure this change applies before constructing
matchingFeatureFlags (the
.select(...).from(featureFlags).where(and(eq(featureFlags.namespaceId,
namespaceId), eq(featureFlags.organizationId, this.organizationId),
...conditions)) statement).
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Around line 461-465: The feature-flag lookup currently always passes the
pre-update subgraph.labels to featureFlagRepo.getFeatureFlagsBySubgraphLabels;
change it to use the post-update labels when provided (e.g., use data.labels or
the update payload) so flags matching newly applied labels aren't missed.
Concretely, compute labelsToUse = data.labels ?? subgraph.labels (or the
equivalent update field present in this method) and pass labelsToUse to
getFeatureFlagsBySubgraphLabels instead of subgraph.labels.
---
Nitpick comments:
In `@controlplane/test/feature-flag/feature-flag-integration.test.ts`:
- Line 34: The test constant isDebugMode is hard-coded to true causing tests to
run in debug timing mode; change isDebugMode to default to false (or read from
an explicit CI/env flag like process.env.DEBUG_MODE or a test CLI flag) so
normal CI runs are not slowed. Update the declaration of isDebugMode and any
test setup that references it (search for isDebugMode in
feature-flag-integration.test.ts and related helpers) to use the new default or
environment-driven value and ensure any local debug override is documented in
test README or env variable.
🪄 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: a17578e0-977a-4f9e-8f68-c6b1535f87dd
📒 Files selected for processing (6)
controlplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/test/feature-flag/feature-flag-integration.test.tscontrolplane/test/test-data/feature-flags/products-standalone-update.graphqlcontrolplane/test/test-data/feature-flags/products-standalone.graphqlcontrolplane/test/test-data/feature-flags/products-update.graphql
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2860 +/- ##
===========================================
+ Coverage 40.94% 64.55% +23.60%
===========================================
Files 1026 318 -708
Lines 129324 45209 -84115
Branches 5997 4908 -1089
===========================================
- Hits 52956 29184 -23772
+ Misses 74628 16000 -58628
+ Partials 1740 25 -1715
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controlplane/test/feature-flag/feature-flag-integration.test.ts (1)
3157-3161: ⚡ Quick winUse baseline/delta assertions instead of absolute global composition counts.
At Line 3157 and the repeated queries below, asserting absolute lengths on all
isFeatureFlagComposition=truerows can become order-dependent when this TODO is enabled. Prefer capturing an initial count and asserting deltas for this test flow.💡 Suggested pattern
+ const getFeatureFlagCompositionCount = async () => + ( + await server.db + .select({ id: graphCompositions.id }) + .from(graphCompositions) + .where(eq(graphCompositions.isFeatureFlagComposition, true)) + .execute() + ).length; + + const baselineFeatureFlagCompositionCount = await getFeatureFlagCompositionCount(); - - let featureFlagCompositions = await server.db - .select({ id: graphCompositions.id }) - .from(graphCompositions) - .where(eq(graphCompositions.isFeatureFlagComposition, true)) - .execute(); - - expect(featureFlagCompositions).toHaveLength(1); + expect((await getFeatureFlagCompositionCount()) - baselineFeatureFlagCompositionCount).toBe(1); await assertNumberOfCompositions(client, baseGraphName, 1, namespace); // ... - featureFlagCompositions = await server.db - .select({ id: graphCompositions.id }) - .from(graphCompositions) - .where(eq(graphCompositions.isFeatureFlagComposition, true)) - .execute(); - - expect(featureFlagCompositions).toHaveLength(1); + expect((await getFeatureFlagCompositionCount()) - baselineFeatureFlagCompositionCount).toBe(1);Also applies to: 3176-3180, 3195-3199, 3212-3216, 3229-3233
🤖 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/feature-flag/feature-flag-integration.test.ts` around lines 3157 - 3161, The tests currently assert absolute counts by querying graphCompositions for isFeatureFlagComposition=true (variable featureFlagCompositions) multiple times; change these to baseline/delta assertions by first querying and storing an initialCount (e.g., initialFeatureFlagCount = (await server.db.select(...).where(eq(graphCompositions.isFeatureFlagComposition, true)).execute()).length) before the operations, then after each operation re-query to get newCount and assert newCount === initialFeatureFlagCount + expectedDelta (and update initialFeatureFlagCount = newCount when chaining checks); apply this pattern for all occurrences involving graphCompositions/isFeatureFlagComposition (the queries around lines referenced: 3157, 3176-3180, 3195-3199, 3212-3216, 3229-3233).
🤖 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/feature-flag/feature-flag-integration.test.ts`:
- Around line 3157-3161: The tests currently assert absolute counts by querying
graphCompositions for isFeatureFlagComposition=true (variable
featureFlagCompositions) multiple times; change these to baseline/delta
assertions by first querying and storing an initialCount (e.g.,
initialFeatureFlagCount = (await
server.db.select(...).where(eq(graphCompositions.isFeatureFlagComposition,
true)).execute()).length) before the operations, then after each operation
re-query to get newCount and assert newCount === initialFeatureFlagCount +
expectedDelta (and update initialFeatureFlagCount = newCount when chaining
checks); apply this pattern for all occurrences involving
graphCompositions/isFeatureFlagComposition (the queries around lines referenced:
3157, 3176-3180, 3195-3199, 3212-3216, 3229-3233).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62fe18b2-4c7d-425f-ab10-f1c37747394d
📒 Files selected for processing (1)
controlplane/test/feature-flag/feature-flag-integration.test.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 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/SubgraphRepository.ts`:
- Around line 389-392: The predicate logic in the loop over newFeatureFlags is
inverted: change the condition that determines when to add a feature flag to
affectedFeatureFlagIds so it selects flags that depend on the updated base
subgraph rather than those that don’t; specifically, replace the every((fsg) =>
fsg.baseSubgraphId !== subgraph.id) check with a positive membership test (e.g.,
some((fsg) => fsg.baseSubgraphId === subgraph.id) or negate the current
predicate) so featureFlag.id is added to affectedFeatureFlagIds only when
featureFlag.featureSubgraphs includes a baseSubgraphId equal to subgraph.id;
apply the same fix to the similar block handling newFeatureFlags later in the
file.
🪄 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: 2b6a4a33-7dff-4cd0-8f62-06be3b90bfba
📒 Files selected for processing (4)
controlplane/src/core/repositories/FeatureFlagRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/test/feature-flag/feature-flag-integration.test.tscontrolplane/test/test-data/feature-flags/products-standalone-feature.graphql
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/repositories/FeatureFlagRepository.ts
Aenimus
reviewed
May 12, 2026
Aenimus
reviewed
May 12, 2026
Aenimus
reviewed
May 12, 2026
Aenimus
reviewed
May 12, 2026
Aenimus
reviewed
May 12, 2026
Aenimus
reviewed
May 12, 2026
Aenimus
reviewed
May 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/SubgraphRepository.ts`:
- Around line 392-396: The new-label feature-flag lookup
(featureFlagRepo.getFeatureFlagsBySubgraphLabels call) currently includes
disabled flags (excludeDisabled: false), which diverges from the old-label
reconciliation pass that only scans enabled flags; change the new-label call in
SubgraphRepository (the getFeatureFlagsBySubgraphLabels invocation) to use the
same excludeDisabled behavior as the old-label pass (i.e., set excludeDisabled:
true or reuse the same flag/constant) so disabled flags are not enqueued on
label-only updates and both scans remain aligned.
- Line 579: The call to .map on affectedFederatedGraphById.keys() fails because
keys() returns an IterableIterator; convert the iterator to an array before
mapping so Promise.all can call fedGraphRepo.byId for each id and populate
refreshedGraphs. Replace the keys() usage in the refreshedGraphs assignment (the
expression that builds refreshedGraphs and calls fedGraphRepo.byId) with an
array-conversion like Array.from(...) or the spread operator to produce an array
of ids, then map over that array to call fedGraphRepo.byId.
🪄 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: cf9bc4b9-37b5-46cc-9b15-51d0df2cf02b
📒 Files selected for processing (1)
controlplane/src/core/repositories/SubgraphRepository.ts
…-subgraph-does-not-recompose
Aenimus
changed the title
…ating-a-base-subgraph-does-not-recompose' into wilson/eng-9560-controlplane-updating-a-base-subgraph-does-not-recompose
Contributor
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/test/label.test.ts (1)
622-627:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert
updateSubgraphsuccess to avoid false-positive test outcomesLine 622 and Line 684 mutate state but don’t assert success. In the “should not trigger recomposition” case, a failed update can still leave compositions at
1, causing a false pass.Suggested patch
- await client.updateSubgraph({ + const updateRes = await client.updateSubgraph({ name: subgraph1Name, namespace: 'default', labels: [label2], }); + expect(updateRes.response?.code).toBe(EnumStatusCode.OK); @@ - await client.updateSubgraph({ + const updateRes = await client.updateSubgraph({ name: subgraph1Name, namespace: 'default', labels: [label3], }); + expect(updateRes.response?.code).toBe(EnumStatusCode.OK);Also applies to: 684-688
🤖 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/label.test.ts` around lines 622 - 627, The test calls client.updateSubgraph (e.g., updateSubgraph in the test) to mutate state but doesn't assert the call succeeded, which can produce false-positive passes; change the test to capture and assert the result of client.updateSubgraph (or the returned response object/boolean) and/or verify by fetching the subgraph after the update (e.g., call client.getSubgraph or inspect the update response) to ensure the update completed before proceeding with the rest of the test, and apply the same assertion for the other occurrence around lines 684-688 so failures in updateSubgraph fail the test instead of masking them.
🤖 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/test/label.test.ts`:
- Around line 622-627: The test calls client.updateSubgraph (e.g.,
updateSubgraph in the test) to mutate state but doesn't assert the call
succeeded, which can produce false-positive passes; change the test to capture
and assert the result of client.updateSubgraph (or the returned response
object/boolean) and/or verify by fetching the subgraph after the update (e.g.,
call client.getSubgraph or inspect the update response) to ensure the update
completed before proceeding with the rest of the test, and apply the same
assertion for the other occurrence around lines 684-688 so failures in
updateSubgraph fail the test instead of masking them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f46c43-d260-4400-be4b-816ff1eaa76b
📒 Files selected for processing (2)
controlplane/src/core/repositories/SubgraphRepository.tscontrolplane/test/label.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/repositories/SubgraphRepository.ts
wilsonrivera
deleted the
wilson/eng-9560-controlplane-updating-a-base-subgraph-does-not-recompose
branch
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
2 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
New Features
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.