This PR cleans up the addConfiguration flag. These changes were
automatically generated by AI and should be reviewed carefully.
Fixes#10627
## 🧹 AI Flag Cleanup Summary
This change removes the `addConfiguration` feature flag. The feature was
discarded, so this cleanup reverts the UI to its state before the
`addConfiguration` flag was introduced. The primary change is in the
strategy
menu, where the single "Add configuration" button is removed and the
original
"Use template," "Add strategy," and "More" buttons are restored.
### 🚮 Removed
- **Flag Definitions**
- `addConfiguration` flag from `experimental.ts` on the backend.
- `addConfiguration` flag from `uiConfig.ts` on the frontend.
- `addConfiguration: true` from the `server-dev.ts` config.
- **UI Components & Logic**
- The conditional rendering in `FeatureStrategyMenu.tsx` that showed an
"Add
configuration" button.
- The `useUiFlag('addConfiguration')` hook call and its import from
`FeatureStrategyMenu.tsx`.
### 🛠 Kept
- **UI Components & Logic**
- The original set of buttons in `FeatureStrategyMenu.tsx`: "Use
template",
"Add strategy", and a "More strategies" icon button. This was the code
path for
when the flag was disabled.
### 📝 Why
The `addConfiguration` feature flag was marked as completed with the
outcome
"discarded". This cleanup removes the flag and all related code,
preserving only
the intended code path, which is the UI behavior from before the flag
was
introduced.
---------
Co-authored-by: unleash-bot <194219037+unleash-bot[bot]@users.noreply.github.com>
Co-authored-by: Nuno Góis <github@nunogois.com>
This PR cleans up the releasePlans flag. These changes were
automatically generated by AI and should be reviewed carefully.
Fixes#10536
## 🧹 AI Flag Cleanup Summary
The `releasePlans` feature flag has been removed, making the feature
permanently
available for Enterprise customers. All conditional logic and checks
related to
this flag have been removed from the codebase.
This change ensures that Release Plans are an integral part of the
Unleash
Enterprise offering.
### 🚮 Removed
- **Flag Definitions**
- `releasePlans` flag from `experimental.ts` in the backend.
- `releasePlans` flag from `uiConfig.ts` in the frontend.
- `releasePlans` flag from `server-dev.ts` development config.
- **Conditional Logic**
- Removed checks for `releasePlansEnabled` in components and hooks,
including
`ReleaseManagement.tsx`, `FeatureStrategyMenu.tsx`, and
`NewInUnleash.tsx`.
- Removed `useUiFlag('releasePlans')` calls from all frontend files.
- Removed the `flag: 'releasePlans'` property from route definitions in
`routes.ts`.
### 🛠 Kept
- **Feature Functionality**
- All UI and logic related to Release Plans and Release Templates are
now
unconditionally enabled for Enterprise users.
### 📝 Why
The `releasePlans` feature has been successfully rolled out and is now a
stable
part of the product. This cleanup removes the artık feature flag to
simplify the
codebase and reduce complexity.
---------
Co-authored-by: unleash-bot <194219037+unleash-bot[bot]@users.noreply.github.com>
Co-authored-by: Nuno Góis <github@nunogois.com>
Doesn't clear the value from the constraint input value popover if you
close it and then re-open. In other words, if you accidentally click
out, you don't lose your progress. Instead, the popover will open again,
with the value you had when you closed it highlighted (so that it's easy
to type over if you want to):
<img width="452" alt="image"
src="https://github.com/user-attachments/assets/d86aa00e-4956-40a8-8fea-e75be5d5425b"
/>
The reason I'm changing this now is because I noticed that the error
wasn't cleared correctly when the popover was closed. If we do it this
way instead, then that makes sense, because you can still see the value.
This is also how the single-value popover has worked forever.
From some quick testing, the single value popover still works as
expected:
<img width="562" alt="image"
src="https://github.com/user-attachments/assets/9041a922-b055-4310-ab60-93ad219981a4"
/>
As a side note: I'm adding a comment to anyone coming after as to why
focus handling on escape doesn't work correctly on the single value
button. I was about to go down a rabbit hole on that before I read my
own comment on the previous PR. So I thought I'd put that here too.
Makes it so that the constraint value input gives you an error if you
try to add one or more values that **all** exist in the set of values
already. E.g. if you have `a` and `b`, and try to add `a`, it'll tell
you that "`a` has already been added". Likewise, if you try to add
`a,b`, it'll tell you that all these values already exist. However, if
at least one of the values does not exist, then it will allow you to
submit the values (we already do deduplication before storing anyway).
The background for this is that a user was confused thinking that just
one specific value didn't get added to their constraints. As it turns
out, they'd already added the value previously, so when it didn't show
up at the end of the list, they thought it didn't work at all.
<img width="863" alt="image"
src="https://github.com/user-attachments/assets/12195e0a-04bc-4b41-bd44-432120c768a6"
/>
<img width="816" alt="image"
src="https://github.com/user-attachments/assets/433a64d7-aec0-482d-8544-574656c266ce"
/>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Follow-up to: https://github.com/Unleash/unleash/pull/10213
This makes our `AddValuesPopover` backdrop click-through. This means you
can interact with any element in the "background" right away and it will
work, while closing the popover at the same time.
If this works well it may be worth extracting to a reusable
ClickThroughPopover or similar.
This PR takes two steps towards better constraint handling:
## New type: `IConstraintWithId`
Introduces a new type, `IConstraintWithId`. This is the same as an
`IConstraint`, except the constraint id property is required. The idea
is that the list of editable constraints should move towards using this
instead of just `IConstraint`. That should prevent us (on a type-level)
from seeing more of the same kind of errors we saw with the segment
constraints yesterday.
I don't want to go ahead and update all the upstream uses of this to
IConstraintWithId in this PR, so I'll look at that separately.
## API payload constraint replacer
Introduces an api payload constraint "replacer", which we can use for
[JSON.stringify's `replacer`
parameter](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#the_replacer_parameter).
The current implementation works both for strategies and for segments
and has been added to edit + create forms for both of these resources.
This has a couple benefits:
1. We can clearly state exactly how we want them to be rendered,
including property order. I've decided to go with context -> operator ->
value(s) as the main one (check the screenie), as I believe this is the
most logical reading order.
2. We can exclude value/values (whichever one doesn't work with the
operator)
3. It doesn't matter how we treat constraints internally, we can still
present the payload how we want
4. Importantly: this only affects the stringification for the
user-facing API payload, so it's very low risk. It does not affect
anything that we actually send to the api.
Here's what it can look like with ordered properties:
<img width="392" alt="image"
src="https://github.com/user-attachments/assets/f46f77c8-0b5a-4ded-b13a-bb567df60bd3"
/>
Prevents the property order from changing when constraints are set from
the editable constraint component. When we render out the API command,
we don't specify the order of properties in the objects, which means
that it can change dramatically, which can be a little jarring.
As it is right now, it first renders in one order when you open the
strategy form:
<img width="299" alt="image"
src="https://github.com/user-attachments/assets/52cf2445-d9eb-402c-b5bc-0fece5fbe822"
/>
And when you navigate to the targeting section of the strategy form, it
changes to:
<img width="299" alt="image"
src="https://github.com/user-attachments/assets/e4cb7006-dcf4-4e88-befb-ccba5b647ddd"
/>
This also applies to constraints in segments.
With this change, the order will remain the same before and after.
Additionally, there's some extra tests around constraint ids being kept
the same and being set if it doesn't exist.
## Further work
This came about as part of the issue we had with constraint editing in
segments being broken as of now. As part of that, I would like to make
some further improvements (such as making the ID required when you use a
list of editable constraints), but that will be in a follow-up. There
are some complications that might not make it a viable option, sadly.
We could also try to stabilize the property order in the API rendering
methods (which I think might be a good idea), but because there's
multiple different ones, this seems to be a faster solution.
This PR addresses and removes the last comment related to the
addEditStrategy flag. In doing so, I have also removed the remaining
dangling files from the new constraint accordion directory. I believe
that everything that's left in there now is currently in use.
This PR removes more constraint inputs and validators that are not in
use anymore. Additionally, the old constraint components that are still
being used by the project action filter item, have been moved to that
directory. This also goes for ResolveInput which has been simplified to
only the inputs and operators used by actions filter item.
I've done a manual side-by-side comparison of the old and newly
refactored filter item, and it appears to be working the exact same.
This PR continues the cleanup after removing the addEditStrategy flag
(part 2 of ???). The primary purpose of this PR is to delete and remove
all references to the LegacyConstraintAccordion.
I've gone and updated all references to the legacy files in external
components and verified manually that they still work.
Most of the files in this PR are changing references. I've extracted two
bits into more general constants/utils:
1. Constraint IDs are a symbol. it was exported as a const from the
previous createEmptyConstraint file. I've moved it into constants.
2. formatOperatorDescription was similarly used all over the place, so
I've placed it in the shared utils directory.
In reviewing this, you can ignore any changes in the legacy constraint
accordion folder, because that's all been deleted. Instead, focus on the
changes in the other files. It's primarily just import updates, but
would be good to get a second set of eyes, anyway.
Fixes a console log about invalid dom nesting:
<img width="1256" alt="image"
src="https://github.com/user-attachments/assets/0849103c-6901-4b64-a124-00eaf8cc7dde"
/>
I've changed the offending div to a span. We set `display: flex` on it,
anyway, so it shouldn't make a difference.
I've also removed some redundant functions and braces that we don't
need.
Removes all usages of flag addEditStrategy and refactors code where
necessary.
This is only the first step of the cleanup. After this, there's still
lots of code to be removed. I've got a different PR that removes ~5k
lines of code (https://github.com/Unleash/unleash/pull/10105) that I
want to reach in pieces to make sure that everythnig works on the way
there.
This removes a strategy that was already deprecated, but only for new
installations.
I tested starting with an installation with this strategy being used and
then updating, and I was still able to edit the strategy, so this should
not impact current users.
On a fresh install the strategy is no longer available.
---------
Co-authored-by: Nuno Góis <github@nunogois.com>
Updates how we handle deleted legal values for the constraint reducer.
The previous iteration used useState and took the deleted legal values
as a third argument. This isn't possible anymore because a reducer can
take only two args. The simplest way forward for this was to move the
deleted legal values into the state itself, so that it's available in
the reducer. Because deleted legal values can be updated whenever (when
we get a response for that specific context field), we'll update it via
`useEffect`.
I'm not crazy about this approach, so if you have better suggestions,
I'm listening.
I've changed the signature of the reducer, so I've also updated the
tests. In doing so, I thought it now makes more sense to have the base
objects be objects instead of functions, so the changes there are
primarily updating that.
Improves handling of constraints in use that have been deleted.
This change implments a few small changes on both the front and the back
end on how we deal with constraints that have been deleted.
The most important change is on the back end, in the
`/constraints/validate` endpoint. We used to throw here if the
constraint couldn't be found, but the only reason we wanted to look for
the constraint in the db was to check for legal values. Now, instead,
we'll allow you to pass a constraint field that doesn't exist in the
database. We'll still check the values against the operator for
validity, we just don't control legal values anymore (because there
aren't any).
On the front end, we improve the handling by showing the deleted context
filed in the dropdown, both when the selector dropdown is closed and
when it is open. However, if you change the context field, we remove the
deleted field from the list. This seems like a sensible tradeoff. Means
you can't select it if you've deselected it.
- Deletes an unused file
- Fixes a console.error due to a prop being passed on to the HTML
component
- Puts all editable constraint files within a separate directory.
We're migrating to ESM, which will allow us to import the latest
versions of our dependencies.
Co-Authored-By: Christopher Kolstad <chriswk@getunleash.io>
Uses a purple color for the hover underline. Also, sets it to be
transparent when not-hovered, so that you get a nice fade in effect.
Focus (top) and hover (bottom) now have the same visual style, but
different ways to get to that state (expansion vs fade-in):
<img width="979" alt="image"
src="https://github.com/user-attachments/assets/e342ea4e-4821-4e4c-bb5d-6b9d3a672e26"
/>
There can be any number of these on a page, so setting autofocus here is
a Bad Idea (TM). It's probably a holdover from when the input was an
accordion and we wanted to give you focus when you opened it (we do a
similar thing for the popover, for instance).
This property will cause you to focus (and potentially scroll) to the
last constraint on the page.
This PR implements a number of strategies to make the app perform better
when you have large lists. For instance, we have a constraint field that
has ~600 legal values. Currently in main, it is pretty slow and sloggy
to use (about on par with what we see in hosted).
With this PR, it becomes pretty snappy, as shown in this video (should
hopefully be even better in production mode?):
https://www.loom.com/share/2e882bee25a3454a85bec7752e8252dc?sid=7786b22d-6c60-47e8-bd71-cc5f347c4e0f
The steps taken are:
1. Change the `useState` hook to instead use `useReducer`. The reason is
that its dispatch function is guaranteed to have a stable identity. This
lets us use it in memoized functions and components.
2. Because useReducer doesn't update the state variable until the next
render, we need to use `useEffect` to update the constraint when it has
actually updated instead of just calling it after the reducer.
3. Add a `toggle value` action and use that instead of checking whether
the value is equal or not inside an onChange function. If we were to
check the state of the value outside the reducer, the memoized function
would be re-evaluated every time value or values change, which would
result in more renders than necessary. By instead doing this kind of
checking inside the reducer, we can cache more aggressively.
4. Because the onChange function can now be memoized, we can memoize all
the legal value selector labels too. This is the real goal here, because
we don't need to re-render 600 components, because one of them was
checked.
One side effect of using useEffect to call `onUpdate` is that it will
also be called immediately when the hook is invoked the first time, but
it will be called with the same value as the constraint that was passed
in, so I don't think that's an issue.
Second: the `useEffect` call uses `localConstraint` directly as a dep
instead of stringifying it. I'm not sure why, but stringifying it makes
it not update correctly for legal values.
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Adds a test suite for the useEditableConstraint hook, attempting to test
all the parts of it that we can't test in isolation.
Plus: a few, small refactorings:
- Renames `onAutoSave` on `onUpdate` to better match `onDelete` (and
because autosave doesn't really mean anything anymore).
- Simplifies and collapses some types
Adds a fairly comprehensive test suite for the constraint reducer. I put
in all cases I thought were relevant.
As part of this, I discovered one bug, and changed two actions.
The bug was toggling on the wrong property when you tried to invert case
sensitivity.
The action changes are:
- rename "remove value from list" to "remove value"
- remove "set value" in favor of instead letting "add value(s)" work in
single-value constraints too.
Extracts and tests the implementation of the functions to get deleted
and invalid legal values.
This is pretty straightforward stuff and arguably not crucial, but it
was an easy place to start and I don't think it hurts to have these in
place anyway.
Adds an input form for single legal values (i.e. for number and semver
operators).
- Uses the `validator` for the constraint to check the list of legal
values and provides a list of "invalid legal values" for values that
don't pass validation.
- Values in the "invalid legal values" set are "disabled" when rendered
in the UI. Additionally, there's an extra bit of text that tells you
that values that aren't valid are disabled.
- Makes the legal values selector more generic and makes it adapt to
multi- or single-value selection based on input props. The external
interface is two separate components (to make it clearer that they are
different things and because their props don't line up perfectly).
Rendered:
<img width="957" alt="image"
src="https://github.com/user-attachments/assets/cd8d2f32-057d-4e31-8fd3-174676eeb65e"
/>
Still todo: testing deleted/invalid legal value detection. I'll do that
as part of the big testathon in a followup.
Fixes an issue where you would lose keyboard focus when interacting with
one of the input fields in the new editable constraint.
The fix was spinning out the inputs into their own separate component.
This prevents them from being re-rendered every time (or something idk)
which allows us to keep focus.
It also stops the popover for multi-value constraint operators from
closing after you've entered a value; allowing for faster entry of more
values (as was intended and as was how it functioned previously).
This (admittedly pretty big) PR removes a component layer, moves all
logic for updating constraint values into a single module, and dumbs
down other components.
The main changes are:
- EditableConstraintWrapper is gone. All the logic in there has been
moved into the new `useEditableConstraint` hook. Previously it was split
between the wrapper, editableConstraint itself, the legalValues
component.
- the `useEditableConstraint` hook accepts a constraint and a save
function and returns an editable version of that constraint, the
validator for input values, a function that accepts update commands,
and, when relevant, existing and deleted legal values.
- All the logic for updating a constraint now exists in the
`constraint-reducer` file. As a pure function, it'll be easy to unit
test pretty thoroughly to make sure all commands work as they should
(tests will come later)
- The legal values selector has been dumbed down consiberably as it no
longer needs to create its own internal weak map. The internal
representation of selected values is now a set, so any kind of lookup is
now constant time, which should remove the need for the extra layer of
abstraction.
## Discussion points
I know the reducer pattern isn't one we use a *lot* in Unleash, but I
found a couple examples of it in the front end and it's also quite
similar to how we handle state updates to change request states. I'd be
happy to find a different way to represent it if we can keep it in a
single, testable interface.
Semi-relatedly: I've exposed the actions to submit for the updates at
the moment, but we could map these to functions instead. It'd make
invocations a little easier (you wouldn't need to specify the action
yourself; only use the payload as a function arg if there is one), but
we'd end up doing more mapping to create them. I'm not sure it's worth
it, but I also don't mind if we do 💁🏼
