Feature flag improvements#550
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extends feature flags with three interconnected capabilities: scheduled enable/rollout changes (future-dated state overrides), kill-switch overrides (forcing a flag to a specific value), and staleness detection (flagging outdated flags based on evaluation age, targeting changes, and experiment completion). Changes span backend (cloud and community apps) and frontend with schema extensions, evaluation logic, response enrichment, service methods, and UI controls. ChangesFeature flag scheduling, kill switches, and staleness
Sequence DiagramsequenceDiagram
participant Client as Client/UI
participant Controller as Controller
participant Service as Service
participant DB as Database
participant Evaluation as Evaluation
Client->>Controller: GET/PUT /feature-flag
Controller->>Service: applyDueScheduledChanges(projectId)
Service->>DB: SELECT flags WHERE scheduledChange.applyAt <= now
DB-->>Service: flags with due changes
Service->>DB: UPDATE enabled/rolloutPercentage, clear scheduledChange
Controller->>Service: create/update flag
Service->>DB: INSERT/UPDATE with scheduledChange, killSwitch fields
DB-->>Service: persisted flag
Controller->>Evaluation: evaluateFlag(flag)
alt killSwitchActive
Evaluation-->>Controller: killSwitchValue
else
Evaluation->>Evaluation: applyDueScheduledChange if needed
Evaluation->>Evaluation: check enabled and targeting rules
Evaluation-->>Controller: boolean or rollout result
end
Controller->>Service: getLastEvaluatedAtByFlagIds(flagIds)
Service->>DB: SELECT max(created) per flagId from evaluations
DB-->>Service: Map<flagId, timestamp>
Controller->>Controller: decorateFlags: compute staleReasons, derive status
Controller-->>Client: enriched flags with status, staleness, lastEvaluatedAt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/community/src/feature-flag/feature-flag.controller.ts (1)
671-702:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply overdue scheduled changes before updating the flag.
This write path can preserve a past-due
scheduledChangewhen the user edits some other field. The next read/evaluate request will then apply that stale schedule and overwrite the newer manual state. Please runapplyDueScheduledChanges(flag.projectId)before building the update payload, like the other mutation/read paths already do.Suggested fix
this.validateScheduledChange(flagDto.scheduledChange) + await this.featureFlagService.applyDueScheduledChanges(flag.projectId) const updatePayload: Partial<FeatureFlag> = { ..._pick(flagDto, [🤖 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 `@backend/apps/community/src/feature-flag/feature-flag.controller.ts` around lines 671 - 702, The update path can preserve a past-due scheduledChange; call applyDueScheduledChanges(flag.projectId) before validating/building the update payload to ensure overdue schedules are applied first. Specifically, in the controller method that currently calls validateScheduledChange(flagDto.scheduledChange) and builds updatePayload (look for validateScheduledChange, hasTargetingRulesChanged, featureFlagService.update, decorateFeatureFlags), insert an awaited call to applyDueScheduledChanges(flag.projectId) immediately before validateScheduledChange and constructing updatePayload so the payload and subsequent featureFlagService.update operate on the latest applied state.backend/apps/cloud/src/feature-flag/feature-flag.controller.ts (1)
723-755:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply due schedules before mutating an existing flag.
If this flag already has a past-due
scheduledChange, updating another field here leaves that stale schedule in storage, and the next read/evaluate path will apply it and overwrite this manual edit. The get/kill/release flows already guard against that; this update path should do the same.Suggested fix
this.validateScheduledChange(flagDto.scheduledChange) + await this.featureFlagService.applyDueScheduledChanges(flag.project.id) const updatePayload: Partial<FeatureFlag> = { ..._pick(flagDto, [🤖 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 `@backend/apps/cloud/src/feature-flag/feature-flag.controller.ts` around lines 723 - 755, Before persisting updates, ensure any past-due scheduled change for this flag is applied/cleared just like the get/kill/release flows do: load the current flag (the existing flag variable used here), call the shared routine that applies due schedules (or implement one on featureFlagService, e.g., applyDueScheduledChange(flag.id) if it doesn't exist), wait for it to complete, then proceed to build updatePayload and call this.featureFlagService.update(id, updatePayload); this prevents a stale scheduledChange from being applied after the manual update. Reference symbols: validateScheduledChange, hasTargetingRulesChanged, featureFlagService.update, decorateFeatureFlags, findOne.
🧹 Nitpick comments (2)
web/app/components/ToolsNav.tsx (2)
323-323: 💤 Low valueNavigation spacing changes appear unrelated to feature flag improvements.
These padding adjustments reduce spacing in the tools navigation but don't seem connected to the PR's stated purpose of adding feature flag scheduling, kill switches, and staleness tracking. Consider whether this styling change should be in a separate PR for clearer change tracking.
Also applies to: 380-380
🤖 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 `@web/app/components/ToolsNav.tsx` at line 323, The padding/classname changes in ToolsNav.tsx ('flex items-start gap-3 px-3 py-2 transition-colors' and the similar occurrence around line ~380) are unrelated to the feature-flag scheduling work; revert those spacing edits in the ToolsNav component back to their original className values (undo the px-3/py-2/gap-3 change) or remove them from this PR and put the styling tweak into a separate PR for clearer history, making sure to update both occurrences within the ToolsNav component so only the feature-flag logic remains in this branch.
380-380: 💤 Low valueConsider consistent padding across mobile navigation elements.
The mobile button (line 380) now uses
px-3 py-2, but the dropdown items (line 414) still usepx-4 py-3. If the goal is tighter spacing throughout, the dropdown items could also be updated for visual consistency.Suggested consistency fix
<Link key={tool.id} to={tool.href} className={cn( - 'flex items-start gap-3 px-4 py-3 transition-colors hover:bg-gray-50 dark:hover:bg-slate-700/30', + 'flex items-start gap-3 px-3 py-2 transition-colors hover:bg-gray-50 dark:hover:bg-slate-700/30', index !== 0 && 'border-t border-gray-100 dark:border-slate-700', )}Also applies to: 414-414
🤖 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 `@web/app/components/ToolsNav.tsx` at line 380, The mobile button in ToolsNav uses tighter padding ('px-3 py-2') while the dropdown items use larger padding ('px-4 py-3'), causing inconsistent spacing; pick one spacing scheme and update the other to match (either change the mobile button className to 'px-4 py-3' or change the dropdown item className to 'px-3 py-2') so visual spacing is consistent across elements—locate the mobile button className in ToolsNav (the element with "flex w-full items-center justify-between gap-3 px-3 py-2") and the dropdown item className around the dropdown rendering (the element using "px-4 py-3") and make the padding values equal.
🤖 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 `@backend/apps/cloud/src/feature-flag/feature-flag.service.ts`:
- Around line 116-155: The current getLastEvaluatedAtByFlagIds method swallows
ClickHouse errors (catch block returns an empty Map) which is indistinguishable
from "never evaluated"; change this so failures are surfaced: remove the catch
that returns result and instead catch the error from clickhouse.query (or let it
bubble) and either log full error context (include projectId and flagIds) using
your service logger, then rethrow the error so the caller can skip
evaluation-based staleness, or return a Map populated with a clear sentinel
value (e.g. "__CLICKHOUSE_ERROR__") for each requested flagId; make the change
in getLastEvaluatedAtByFlagIds around the clickhouse.query/resultSet.json call
so failures are not collapsed into an empty Map.
In
`@backend/migrations/clickhouse/selfhosted_2026_05_24_feature_flag_operations.js`:
- Around line 4-10: The first ALTER that adds the scheduledChange column is
anchored to a non-guaranteed column (`experimentId`) which breaks upgrades;
change the statement that adds `scheduledChange` to remove the "AFTER
experimentId" clause (i.e. use `ADD COLUMN IF NOT EXISTS scheduledChange
Nullable(String)`), and ensure subsequent column additions either don't rely on
that anchor or explicitly use safe anchors (refer to the ALTER statements adding
`killSwitchActive`, `killSwitchValue`, `killedAt`, `targetingUpdatedAt`, and
`updated` and the `UPDATE targetingUpdatedAt = created` line) so the migration
will succeed even when `experimentId` is absent.
In `@web/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsx`:
- Around line 745-746: The datetime input's min currently uses
dayjs().format('YYYY-MM-DDTHH:mm') which allows selecting the current minute
while your submit validation requires a strictly future time; update the min to
match validation by using dayjs().add(1, 'minute').format('YYYY-MM-DDTHH:mm')
(or the same helper/format used by your validation) so the picker disallows the
currently-invalid value — change the min prop near the input in
FeatureFlagSettingsModal (the line with min={...} and the adjacent onChange
handler) to use the +1 minute value.
In `@web/app/routes/projects`.$id.tsx:
- Around line 1738-1741: Wrap the JSON.parse of scheduledChange in a try/catch
to prevent a thrown SyntaxError from bubbling as a 500; specifically, when
reading scheduledChangeRaw from formData.get and when parsing the other
occurrence around lines 1784-1788, attempt JSON.parse and on failure return or
throw a controlled 400 response (e.g., throw new Response or return a
badRequest-like response) with a clear error message so the action responds with
400 instead of 500.
- Around line 1819-1825: The code currently coerces a missing or invalid
killSwitchValue to false before calling serverFetch; update the form handling to
require an explicit value: read the raw value via
formData.get('killSwitchValue') into a variable (e.g. rawKillSwitch), if
rawKillSwitch is null or not equal to 'true' or 'false' return a
bad-request/error response (do not call serverFetch), otherwise set
killSwitchValue = rawKillSwitch === 'true' and pass that to serverFetch for
feature-flag/{flagId}/kill; use the existing flagId and serverFetch identifiers
to locate where to implement this validation.
---
Outside diff comments:
In `@backend/apps/cloud/src/feature-flag/feature-flag.controller.ts`:
- Around line 723-755: Before persisting updates, ensure any past-due scheduled
change for this flag is applied/cleared just like the get/kill/release flows do:
load the current flag (the existing flag variable used here), call the shared
routine that applies due schedules (or implement one on featureFlagService,
e.g., applyDueScheduledChange(flag.id) if it doesn't exist), wait for it to
complete, then proceed to build updatePayload and call
this.featureFlagService.update(id, updatePayload); this prevents a stale
scheduledChange from being applied after the manual update. Reference symbols:
validateScheduledChange, hasTargetingRulesChanged, featureFlagService.update,
decorateFeatureFlags, findOne.
In `@backend/apps/community/src/feature-flag/feature-flag.controller.ts`:
- Around line 671-702: The update path can preserve a past-due scheduledChange;
call applyDueScheduledChanges(flag.projectId) before validating/building the
update payload to ensure overdue schedules are applied first. Specifically, in
the controller method that currently calls
validateScheduledChange(flagDto.scheduledChange) and builds updatePayload (look
for validateScheduledChange, hasTargetingRulesChanged,
featureFlagService.update, decorateFeatureFlags), insert an awaited call to
applyDueScheduledChanges(flag.projectId) immediately before
validateScheduledChange and constructing updatePayload so the payload and
subsequent featureFlagService.update operate on the latest applied state.
---
Nitpick comments:
In `@web/app/components/ToolsNav.tsx`:
- Line 323: The padding/classname changes in ToolsNav.tsx ('flex items-start
gap-3 px-3 py-2 transition-colors' and the similar occurrence around line ~380)
are unrelated to the feature-flag scheduling work; revert those spacing edits in
the ToolsNav component back to their original className values (undo the
px-3/py-2/gap-3 change) or remove them from this PR and put the styling tweak
into a separate PR for clearer history, making sure to update both occurrences
within the ToolsNav component so only the feature-flag logic remains in this
branch.
- Line 380: The mobile button in ToolsNav uses tighter padding ('px-3 py-2')
while the dropdown items use larger padding ('px-4 py-3'), causing inconsistent
spacing; pick one spacing scheme and update the other to match (either change
the mobile button className to 'px-4 py-3' or change the dropdown item className
to 'px-3 py-2') so visual spacing is consistent across elements—locate the
mobile button className in ToolsNav (the element with "flex w-full items-center
justify-between gap-3 px-3 py-2") and the dropdown item className around the
dropdown rendering (the element using "px-4 py-3") and make the padding values
equal.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bc9c5f0-efad-4289-b607-5e358bd7907f
📒 Files selected for processing (20)
backend/apps/cloud/src/feature-flag/dto/feature-flag.dto.tsbackend/apps/cloud/src/feature-flag/entity/feature-flag.entity.tsbackend/apps/cloud/src/feature-flag/evaluation.tsbackend/apps/cloud/src/feature-flag/feature-flag.controller.tsbackend/apps/cloud/src/feature-flag/feature-flag.service.tsbackend/apps/community/src/feature-flag/dto/feature-flag.dto.tsbackend/apps/community/src/feature-flag/entity/feature-flag.entity.tsbackend/apps/community/src/feature-flag/evaluation.tsbackend/apps/community/src/feature-flag/feature-flag.controller.tsbackend/apps/community/src/feature-flag/feature-flag.service.tsbackend/migrations/clickhouse/initialise_selfhosted.jsbackend/migrations/clickhouse/selfhosted_2025_12_10_feature_flags.jsbackend/migrations/clickhouse/selfhosted_2026_05_24_feature_flag_operations.jsbackend/migrations/mysql/2026_05_24_feature_flag_operations.sqlweb/app/api/api.server.tsweb/app/components/ToolsNav.tsxweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsxweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagsView.tsxweb/app/routes/projects.$id.tsxweb/public/locales/en.json
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 `@backend/apps/community/src/feature-flag/feature-flag.controller.ts`:
- Around line 671-673: The controller currently calls
applyDueScheduledChanges(flag.projectId) before
validateScheduledChange(flagDto.scheduledChange), which can persist project-wide
changes even if the incoming payload is invalid; move the validation so
validateScheduledChange(flagDto.scheduledChange) runs before calling
this.featureFlagService.applyDueScheduledChanges(flag.projectId) (i.e., validate
the scheduledChange payload up front and return the 400 on failure), ensuring no
side-effectful writes occur prior to successful validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c56b0c8e-0a85-4b43-a3f9-fcbc635f3a07
📒 Files selected for processing (6)
backend/apps/cloud/src/feature-flag/feature-flag.controller.tsbackend/apps/cloud/src/feature-flag/feature-flag.service.tsbackend/apps/community/src/feature-flag/feature-flag.controller.tsbackend/migrations/clickhouse/selfhosted_2026_05_24_feature_flag_operations.jsweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsxweb/app/routes/projects.$id.tsx
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
1 participant
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.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Chores