feat: Confirm flag changes modal #31031
Conversation
📸 UI snapshots have been updated19 snapshot changes in total. 0 added, 19 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added a feature flag confirmation system to prevent accidental changes to feature flags, with a modal that displays pending changes before they're applied.
- Added
FeatureFlagConfirmationModal.tsxthat shows specific changes being made to flags (status, variants, conditions) before saving - Added
FeatureFlagRolloutConfirmationModal.tsxspecifically for confirming rollout condition changes - Added
flags_require_confirmationboolean field to the Team model with a migration (0708) - Added team setting in
FeatureFlagSettings.tsxto toggle confirmation requirement - Implemented
changesRequireConfirmation()infeatureFlagLogic.tsto detect significant changes requiring confirmation
12 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| // Wrapper for any function that modifies rollout conditions | ||
| const withConfirmation = (callback: () => void): (() => void) => { | ||
| // Don't require confirmation for rollout changes - these are relatively minor adjustments | ||
| return callback | ||
| } |
There was a problem hiding this comment.
logic: The withConfirmation function doesn't actually show the confirmation modal - it just returns the callback directly. This makes the confirmation feature non-functional. The function should check flagsRequireConfirmation and either show the modal or execute the callback directly.
| // Wrapper for any function that modifies rollout conditions | |
| const withConfirmation = (callback: () => void): (() => void) => { | |
| // Don't require confirmation for rollout changes - these are relatively minor adjustments | |
| return callback | |
| } | |
| // Wrapper for any function that modifies rollout conditions | |
| const withConfirmation = (callback: () => void): (() => void) => { | |
| // Don't require confirmation for rollout changes - these are relatively minor adjustments | |
| return () => { | |
| if (flagsRequireConfirmation && id && id !== 'new') { | |
| setConfirmRolloutModalVisible(true) | |
| setPendingRolloutUpdate({ callback }) | |
| } else { | |
| callback() | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
^ this is good feedback IMO
| document | ||
| .getElementById('feature-flag') | ||
| ?.dispatchEvent(new Event('submit', { cancelable: true, bubbles: true })) |
There was a problem hiding this comment.
style: Consider adding error handling in case the form element isn't found. If the form ID changes or the element isn't in the DOM, this would silently fail.
📸 UI snapshots have been updated16 snapshot changes in total. 0 added, 16 modified, 0 deleted:
Triggered by this commit. |
| // State for feature flag confirmation modal | ||
| const [flagChanges, setFlagChanges] = useState<string[]>([]) | ||
| const [showSaveConfirmModal, setShowSaveConfirmModal] = useState(false) |
There was a problem hiding this comment.
we try to avoid mixing react state management (i.e. useState, useEffect) with our existing state management via Kea. see https://posthog.com/handbook/engineering/conventions/frontend-coding#dos-and-donts.
Try making these into a kea logic instead! I think it would be appropriate to define them in featureFlagLogic.ts.
| // Wrapper for any function that modifies rollout conditions | ||
| const withConfirmation = (callback: () => void): (() => void) => { | ||
| // Don't require confirmation for rollout changes - these are relatively minor adjustments | ||
| return callback | ||
| } |
There was a problem hiding this comment.
^ this is good feedback IMO
| // State for rollout condition confirmation modal | ||
| const [confirmRolloutModalVisible, setConfirmRolloutModalVisible] = useState(false) | ||
| const [pendingRolloutUpdate, setPendingRolloutUpdate] = useState<{ | ||
| callback: () => void | ||
| } | null>(null) |
There was a problem hiding this comment.
same as above; these should be logics in FeatureFlagReleaseConditionLogic.ts
| @@ -0,0 +1,47 @@ | |||
| import { LemonButton, LemonModal } from '@posthog/lemon-ui' | |||
There was a problem hiding this comment.
this modal is super similar to the FeatureFlagConfirmationModal – why not combine them both into a ConfirmationModal.tsx that lives in /scenes/feature-flags/ and serves both purposes? That feels more in the spirit of DRY (Don't Repeat Yourself) to me.
| @@ -211,20 +229,20 @@ export function FeatureFlagReleaseConditions({ | |||
| icon={<IconCopy />} | |||
| noPadding | |||
| tooltip="Duplicate condition set" | |||
| onClick={() => duplicateConditionSet(index)} | |||
| onClick={withConfirmation(() => duplicateConditionSet(index))} | |||
There was a problem hiding this comment.
why are these methods all wrapped in withConfirmation() if that method just returns the callback?
| } | ||
|
|
||
| return ( | ||
| <LemonModal |
There was a problem hiding this comment.
given that this is a component with which we expect users to interact, this should be LemonDialog over LemonModal.
| } | ||
|
|
||
| // Check for release condition changes | ||
| if (JSON.stringify(originalFlag.filters.groups) !== JSON.stringify(updatedFlag.filters.groups)) { |
There was a problem hiding this comment.
can use lodash.deepEqual here instead of stringifying
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("posthog", "0707_file_system_shortcut"), |
There was a problem hiding this comment.
migrations need to be updated – this should be generated and not hand-rolled.
|
@ordehi This is a great start; but there's a few things we need to clean up here to make this production-ready! As discussed in Slack, I'm happy to run with them, and call out on the subsequent PR what I did differently than your original approach. Thanks for getting the ball rolling on this! |
Problem
Users often report issues with others changing flags when they shouldn't, which has an impact in their app. They'd like a way to prevent or make it harder to make such changes. A quick win is to have an optional modal that asks for confirmation (listing the changes made) before changing things like flag status, variants and release conditions.
Changes
This adds a modal that shows when the user makes changes to:
The modal lists the changes the user is trying to save to the flag:
The user has to click "Save changes" before the changes are applied.
The modal is optional, they need to enable it in settings for the project:
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Tested locally.