Fix the Filter & Group editor: add-after-result, Reset, row identity, validation - #1111
Open
abefarris wants to merge 3 commits into
Open
Fix the Filter & Group editor: add-after-result, Reset, row identity, validation#1111abefarris wants to merge 3 commits into
abefarris wants to merge 3 commits into
Conversation
The Add button appended to the end of the list, but the validator requires result to be last, so adding a level to the default benchmark/control/result grouping always produced an invalid config and a disabled Apply button.
Two defects with one visible symptom - rows surviving a Reset that should
have removed them.
Row keys were derived from content (`${type}-${value}`). A newly added row
is {type: ""}, so adding two produced two children keyed "-undefined".
React's child map for the list is then ambiguous and later updates strand
rows on screen: after Reset a discarded row remained, and a second Reset
could not clear it, even though the saved grouping and the rendered tree
were both correct - only a remount fixed it. Keying by index instead is
worse: the react-select at a given position keeps its displayed value, so
inserting rows made the new ones render as the previous occupant. Each row
now carries an id generated when the row is created, preserved across edits
and reordering, and stripped before saving so it never reaches the URL.
Reset called onApply([]) but never touched local state. That clears the
saved grouping so the panel falls back to its default, but when the panel is
already on the default the search params do not change, nothing re-renders,
and the pending rows stay on screen. Reset now sets the editor's rows to the
default returned by useGroupingConfig - which knows the control and detection
defaults - before applying.
…bled
The validator's switch fell through to `default: return true`, so a row with
no type counted as valid and Apply saved {"type":""} into the URL - a level
that groups nothing. Repeated levels were likewise accepted, though a second
copy cannot subdivide anything its twin has not already split.
Both are now rejected. Duplicates are keyed on type and value, so
control_tag:domain and control_tag:label remain distinct levels.
isValid.reason previously existed only as the Apply button's title
attribute, so a blocked save looked like a dead button. It is now rendered
next to the button, which also surfaces the pre-existing "Result must be the
last grouping" message - reachable by dragging the result level off the end.
|
|
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.
Fix the Filter & Group editor: add-after-result, Reset, row identity, validation
Branch:
fix-grouping-editor→develop· 3 commits · Filter & Group editor components +useGroupingConfigProblems
Five bugs in the benchmark Filter & Group editor, all reproducible on
develop:after the trailing
resultlevel, producing an invalid config that cannot besaved. (
resultmust be the last level.)discarded, so the editor state never actually reset.
type, so two rows of the same type (or positional keys after a drag) made
react-select values jump between rows.
was written into the URL.
misbehaves.
Fixes (one commit each)
aa7d5d5— Insert a new grouping level before the trailing result level.45123b9— Give grouping rows a stable identity (__rowId, stripped beforesave), and make Reset restore the default grouping (
defaultGroupingis nowexported from
useGroupingConfigas the single source of that default).02ef124— Reject empty and duplicate grouping levels, and say why Apply isdisabled instead of failing silently.
Testing
Manually exercised across ~60 editor scenarios: add/remove/reorder at every
position, Reset before and after Apply, drag reordering (framer-motion
Reorderidentity follows the row object, not the index), duplicate/empty rejection
messages, and round-trips through the
?grouping=URL parameter. Also verifiedgrouping selections are reflected in the rendered tree (grouping types,
ordering, nesting depth, leaf content) — not just in the URL.
Notes
Searched existing issues; none of these appear to have been reported. The
closest in the area is the long-closed #90, but that was a tree-rendering bug
(results lumped under one benchmark when it appears under several parent
groups), unrelated to the editor.