feat(ui): allow copying flags across environments#5772
Conversation
Signed-off-by: AnouarMohamed <m.anouar@mundiapolis.ma>
Signed-off-by: AnouarMohamed <m.anouar@mundiapolis.ma>
|
This is great we should also add the ability to populate segments as well |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2 #5772 +/- ##
=======================================
Coverage 61.75% 61.75%
=======================================
Files 142 142
Lines 14442 14442
=======================================
Hits 8919 8919
Misses 4784 4784
Partials 739 739
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Verdict: comment
The cross-environment flag copy is well-scoped and the new test coverage is solid. However, three UX/consistency issues raised in the previous automated review are still unfixed in this revision: a stale error_state bug when switching environments, a missing in-flight guard on the Copy button, and an unused dependency in a React effect.
ui/src/components/panels/CopyFlagPanel.tsx
- minor (L154): The remote-namespace fetch calls
setError(err)on failure but never clears the error on success. If a user selects an environment where the fetch previously failed, then switches to one that succeeds, the stale error persists in the UI. CallclearError()in the.thenhandler beforesetRemoteNamespaces(data.items ?? []). - minor (L283): The Copy button is not disabled while a copy is in-flight. A double-click can start two concurrent
handleCopypromises, which may race on the backend and produce a misleading conflict error even though one copy succeeds. Add a localcopyingstate, include it in thedisabledcondition, and reset it in both the.thenand.catchhandlers of theonClickpromise chain. - nit (L175):
namespace.keyis listed in the remote-namespace fetch effect's dependency array but is never referenced inside the effect body. Remove it to avoid unnecessary refetches if the current namespace changes while the dialog is open.
🤖 Automated review by the Flipt PR review agent.
Signed-off-by: AnouarMohamed <m.anouar@mundiapolis.ma>
Signed-off-by: AnouarMohamed <m.anouar@mundiapolis.ma>
d3d8830 to
ef3d381
Compare
Signed-off-by: AnouarMohamed <m.anouar@mundiapolis.ma>
| (candidate) => candidate.key === initialEnvironmentKey | ||
| ) || environmentOptions[0]; | ||
|
|
||
| if (selectedEnvironment?.key !== initialEnvironment.key) { |
There was a problem hiding this comment.
I think this effect can accidentally undo the user's selection. Since selectedEnvironment?.key is in the dependency list, picking another environment reruns the effect and recomputes the initial environment. When the current env has another namespace, that initial value is the current env, so the cross-env selection gets snapped back. Could we make this initialization run only when the dialog opens / when there's no valid selected environment, instead of whenever the selected env changes?
| return; | ||
| } | ||
|
|
||
| setRemoteNamespaces([]); |
There was a problem hiding this comment.
If this request fails, we clear the remote namespaces and the dialog falls through to the same empty-state message as a valid environment with no namespaces. That makes a network/API error look like 'No namespaces are available', and there's no obvious retry path. Could we keep a local load error state and show an inline error/retry here, only showing the empty-state copy after a successful empty response?
| </DialogHeader> | ||
| <div className="my-2 space-y-4"> | ||
| <div className="space-y-2"> | ||
| <div className="text-sm font-medium">Environment</div> |
There was a problem hiding this comment.
Small accessibility issue: this text looks like a label visually, but it isn't associated with the select trigger. Same for Namespace below. Screen readers won't get a reliable accessible name for these controls, and tests can't query them by role/name. Could we wire these up with a real label or aria-labelledby on the Listbox trigger?
Summary
Adds support for copying a flag to a namespace in another environment from the flag details page.
Fixes #5382.
Changes
Listboxcontrol fix as a prerequisite for dependent selectionsBackward Compatibility
Existing same-environment namespace copy behavior still works.