Conversation
`#collectFields` wrote `fields[name] = field.checked` for every checkbox, a
boolean under the control's own name. Three boxes named `features[]` with two
ticked therefore left the browser as a single `false` — the last box's checked
state — and same-named boxes overwrote each other before any schema could see
them. Measured against a `[:string]` schema the action received `{}`, against a
flat `:string` schema `"false"`; both silent. A native submission of the same
three boxes sends `features[]=news&features[]=events`.
A name ending in `[]` is now collected as an array of the chosen values: a
ticked box contributes its `value`, an unticked one nothing, a `<select
multiple>` its selected options, any other control its value. The suffix is the
only trigger. An implicit "two controls share a name" rule was tried and
dropped: it also catches Rails' hidden companions and radio groups, and every
neighbouring path that reads the same DOM — the draft snapshot, the conditional
confirm — would have to reproduce the same guesswork.
Two shapes keep their meaning: a lone checkbox without `[]` stays the documented
yes/no boolean, and a radio group keeps its single checked value, `[]` or not.
That makes the suffix a migration point for existing apps, and the CHANGELOG
says so: ANY `[]`-named control now contributes to an array, including a single
one. A lone `<input type="text" name="tags[]">` used to post `"abc"` and now
posts `["abc"]`; measured against the real ParamSchema, a flat
`params: { tags: :string }` coerces that to the literal `"[\"abc\"]"` —
silently, with a 200. Such a param has to be declared as an array type, or the
suffix dropped from a name that never meant a list.
A hidden input sharing a name with a checkbox is that box's COMPANION and
contributes nothing. The value cannot be the test, because Rails renders three
different ones, measured from the helpers:
check_box(:u, :sub) hidden value="0" + box value="1"
check_box(:u, :ids, {multiple: true}, v) hidden value="0" + box value=v, name ends in []
collection_check_boxes / unchecked_value nil hidden value="" — or none at all
What identifies a companion is that a checkbox shares its name. Reading the
second shape by value put every companion into the group: three boxes with the
third ticked collected as `["0","0","0","3"]`, and against `[:integer]` the
action would have written tag id 0 — worse than the bug, which dropped the param
and let the keyword default stand. A hidden input WITHOUT a same-named checkbox
is an ordinary value, the usual shape for a list JS maintains.
Over a form body a group's values are written as `params[name][]`, the shape
Rack parses back into an array. The indexed form `#appendField` writes for a
plain array (`params[name][0]`, `params[name][1]`) arrives as a hash of index
keys — an array param type normalizes that back, but only an array type does,
so the two encodings would stop coercing identically for the same fields.
An EMPTY group has no form-body spelling: a repeated key with no values is
nothing, and `[""]` means something else per element type (`[:string]` keeps it,
`[:integer]` coerces `[0]`, `[:date]` and `[:file]` drop the key). Over the JSON
path a cleared group arrives as `[]`; over a form body — which the client uses
when a file input carries a file — its key is absent and the action's keyword
default applies. The README caveat says so.
`reactive_persist` drafts such a group as the list of ticked values and restores
exactly those boxes; before, the draft held one boolean and the restore ticked
every box of the group. Its slot is an array for every control that can
contribute a value — checkboxes, selects, text inputs, editors and
contenteditables — with a radio group the single exception, as in the collector.
A non-checkbox sharing the group's name left a string there, and `.push` on it
threw inside the draft write; that write is swallowed, so the root persisted
nothing at all, silently. Editors are collected after the native controls, so
they always landed last and always clobbered the array.
On restore, a control that cannot pick its own entry out of the list keeps what
the server rendered — as long as the group has two or more contributors. The
draft records values in document order with nothing saying which control each
one came from, so replaying it would paste "freeform,news" into a text field. A
group of ONE contributor has no such ambiguity, and refusing it would silently
drop the draft of a plain field whose name merely ends in `[]`, the usual shape
for a list JS maintains: measured, `<input type="text" name="tags[]">` used to
round-trip its draft and stopped doing so once the group slot existed. Both
entry points resolve the list through the same rule, including the deferred
editor path, which applies an editor that upgraded after connect without
passing the branch chain at all.
Whether the server already rendered a box of a group ticked is decided ONCE
before the restore walks the controls. The walk writes `checked` as it goes, so
asking from inside it reads the restore's own work: the first box it ticks makes
every later box of the same group look server-rendered. Measured, a draft of
["news","maps"] came back as ["news"] alone, and the single-value case the tests
covered is exactly the one where that is invisible. The radio branch asks the
same question inline and stays correct only because a radio group holds a single
value.
A draft written before this change survives the upgrade, but its group key no
longer reaches the controls that read a list. It holds one boolean — or, in a
mixed group, whatever control wrote last — and applying it kept causing damage
for as long as the draft lived, seven days by default: a checkbox group came
back fully ticked, while a `<select multiple>` under the same name lost its
rendered selection, because under `restore: "always"` that branch matches
`Set{"true"}` against its options and nothing matches. The next snapshot
replaces the key with the list. Only that one key changed meaning, which is why
`PERSIST_VERSION` stays where it is: bumping it would also discard the drafted
prose of every form that has no checkbox group at all.
The predicate contract in `confirm_predicate.js` says what a group's value is
now, including that an empty group is PRESENT as `[]` and that `[]` is truthy.
CHANGELOG, README and the actions guide carry the rule, and the README's
multipart caveat is corrected: a `[]`-named group's values ride along fine, an
emptied one has no spelling there.
…#258) The unit tests in the previous commit observe the POST body. These drive the same shape through the two layers a component author actually meets. `CheckboxGroupComponent` renders what the issue describes: three boxes sharing `features[]` (two ticked), a lone yes/no box, and a `<select multiple>`. Its `save` declares the group as `[:string]` and reflects the coerced result, the way NestedParamsComponent does for bracketed keys. The request spec pins the server half from both encodings: the array arrives as an array, an empty group stays `[]` rather than collapsing to nil, a group that never rendered stays nil, the lone checkbox keeps its boolean, a `<select multiple>` group coerces the same way, and a repeated `params[features][]` over a form-encoded body reaches the declared array. Nothing on the server changed, so these pass before the fix as well — they pin what an action receives once the client sends what a group means. The system spec is the one that could not pass before: it unticks one box, ticks another, adds a second option to the select, saves, and reads what the action received — in a real browser, through the minified client the browser suite serves. Against the old client it reported `"features":null`, the exact symptom of the collector writing a boolean under the group's name.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
Seven review comments, all in this branch's own new lines. Two were defects,
both measured before and after; the rest are prose, naming and test shape.
**A `[]`-named editor posted a scalar.** `#collectFields` reads named rich
editors and bare [contenteditable] in a SECOND pass, which assigned
`fields[name]`. A lone `notes[]` contenteditable therefore left the browser as
`{"notes[]": "typed"}` while `persistSnapshot` pushed the same control into an
array: the wire and the draft disagreed about one field, and a declared array
type saw a string. The pass appends to the group slot now, as the first one
does — companion rule included.
That companion rule had to grow for it. A hidden input sharing a name with a
NAMED editor is that editor's twin (an editor mirroring its serialized value
into a hidden is the shape this pass exists for), so under a `[]` name the
hidden contributes nothing and the editor speaks for both; otherwise the value
rode the wire twice while the draft, which never sees hidden inputs, held one.
The canonical Rails Trix pair is untouched: there the name sits on the hidden
and the editor points at it with `input=`, so it has no name in this query.
Both halves ask whether the editor is READY first. Trix defines its elements in
a setTimeout after load, so a save can run while the editor is still an
unupgraded tag with nothing to read — suppressing its hidden twin then and
posting the editor's "" would overwrite the real value with an empty group,
which is issue zoolutions#8 under a `[]` name. Measured: `{"notes[]": [""]}` where
`["<p>real</p>"]` was owed.
A scalar already under a `[]` name is left alone, because only a RADIO can have
put it there: a radio means "pick one" and keeps its single value with or
without the suffix, which is why `#arrayFieldNames` excepts it. Converting that
to a group discarded the chosen value — measured, `{"pick[]": "a"}` became
`{"pick[]": ["typed"]}`.
**A multi-select restored from another control's values.** A `<select
multiple>` reads a list by matching option values, which is only sound when the
list is its own. Sharing a group with a second contributor it treated every
entry as one of its options: measured, a draft of ["blue","freitext"] from a
select plus a text field selected both, one of them the text field's. It keeps
the server's selection in that case now. Neither defect is reachable on main —
for that DOM main drafts a scalar (last writer wins), so the array draft these
changes introduced is what exposed it.
The rest: the docs sentence said "the client sends one" where the referent was
a form body, not an empty array; a test comment described a throw its own
fixture cannot produce and called a contenteditable an editor; two tests
re-typed the `seedDraft` helper and the PAYLOAD literal instead of using them;
the predicate contract's example addressed `fields.tags` where the key keeps
its suffix and is `fields["tags[]"]`; and a comment block in `#collectFields`
sat at the wrong indentation inside the checkbox branch.
Test shape, from the same round: the collect example named "posts an ARRAY, not
a scalar" carried a checkbox in its fixture, so the pre-fix failure it measured
was the editor's value being swallowed, not a scalar being posted — it is named
for what it does now and the scalar case has its own example. The multi-select
example asserted an empty selection against a fixture that rendered none, so it
would have held for a draft that never arrived; it renders one selected option,
asserts it, and carries an unrelated field that proves the draft came.
Author
|
Pushed a commit for the review above, plus two small follow-ups: one corrects a CHANGELOG sentence (an editor under a Two of the points were real, both in the code this PR adds. An editor or contenteditable under a Reviewing that commit on our side turned up two more cases on the same path, fixed in it as well: an editor saved before Trix had upgraded posted an empty string instead of its hidden value, and a radio next to an editor under one name lost its value. A third change from that pass, treating a hidden field that mirrors an editor as its companion, is taken back in the follow-up commit: none of the editors the gem supports renders that shape, and the rule dropped a hidden field's own value. The other five were wording in our own comments, docs and tests, and are fixed as suggested. |
) The CHANGELOG said an editor sharing a `[]` name "ADDS its value to the group rather than standing down behind it". That is the wrong way round beside a RADIO, which keeps its single value with or without the suffix: there the editor does stand down, measured — `{"pick[]": "a"}`, not `["typed"]`. The sentence names the exception now. The editor selector was spelled twice, once as `PERSIST_EDITOR_SELECTOR` and once inline in `#collectFields`. They are identical today, and the companion rule reads one while the second pass reads the other — drift would be silent in both directions. One constant.
Author
|
@mhenrixon Sorry about the noise last night. This PR is the smaller, initial version to address #258. Your call on how to proceed here, I'll work with #259 in my app for now. Of course closing the PR(s) is also completely valid, no hard feelings. |
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Fix all with cubic | Re-trigger cubic
…utions#258) Three comments, all in this branch's own new lines, plus one rollback the round turned up. A test added as a counterweight was a verbatim duplicate of the hidden-input test beside it: same fixture, same assertion, and no editor in the root at all, so it measured nothing about the rule it was meant to bound. It drives the actual boundary now. The CHANGELOG packed five behaviours into one sentence. Each has its own sentence. The editor DOM was walked twice, once for the companion names and once for the second pass. The elements are collected once and handed to both. The rollback: a hidden input sharing a `[]` name with a NAMED editor was read as that editor's twin and suppressed, so the value would not ride the wire twice. The shape that rule was built for does not exist. The dummy app renders the three real ones and none of them matches: Lexxy carries the name on the element with no hidden, Trix carries it on the HIDDEN and the editor has none, and a bare contenteditable has no hidden either. Nothing in the DOM tells a mirroring hidden from a list JS maintains, and the rule cost a measured silent loss: a radio, a hidden and a contenteditable under one `[]` name posted ["a","typed"] where the hidden's own value belonged. A doubled value is visible on the wire; a swallowed one is not. The hidden keeps its say. What stays from that round is the readiness check: an editor that has not upgraded contributes nothing to a group. Trix defines its elements in a setTimeout after load, its "" is an absent value rather than an empty one, and persistSnapshot omits such an editor for the same reason.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Fix all with cubic | Re-trigger cubic
…utions#258) The head of the collector's second pass still said a hidden twin is suppressed "so the value still rides the wire exactly once". It is not: nothing in the DOM tells a hidden that mirrors an editor from one that is a list JS maintains, and reading it as a twin cost a measured silent loss, so that rule came out again. The guard for that decision quoted a measurement from a DIFFERENT fixture — `{"pick[]": ["a","typed"]}`, which belongs to the radio case three tests down. On its own fixture the suppressing rule produced `["typed"]`, with the hidden's value gone. The comment says that now. No behaviour changes. The sourcemap carries the source text, so the client is rebuilt and the vendored copies re-synced; `rake build:js_check` fails otherwise.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
This branch has not been deployed
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.
Fixes #258.
Checkbox groups didn't survive
#collectFields. Every checkbox was collected asfields[name] = field.checked, so three boxes namedfeatures[]overwrote each other and the action got the checked state of the last one instead of the chosen values.Now a name ending in
[]is collected as an array of the ticked values; a<select multiple>works the same way. Over a form body the values go out asparams[name][], the same as a native form would send them.Unchanged: a single checkbox without
[]is still a boolean, radio groups still post one value, and the hidden field Rails renders in front of a checkbox is ignored as before.One thing to know when upgrading: a single field whose name ends in
[]is now sent as an array too, so its param has to be declared as an array type.Not covered: a group with nothing ticked can't be expressed in a form body, so over multipart the key is just missing. #259 has a full solution for that, but it got too big, so I'm keeping it as a draft and sending this one first.
Tests: JS unit tests for the collector and for draft persistence, a request spec, and two browser examples in the dummy app. 27 of them fail on main.
Summary by cubic
Fixes #258 so checkbox groups in
#collectFieldscollect as arrays of the ticked values instead of resolving to the last box's checked state — a group offeatures[]boxes now postsparams[features][]the way a native submit would, and a cleared group arrives as[]over the JSON path. Named editors and contenteditables append to a[]group too, and a hidden input sharing the group's name keeps contributing its value — nothing can tell one that mirrors an editor from a list JS maintains, and a doubled value is visible where a swallowed one is not — except beside a radio, whose single value the editor leaves alone.reactive_persistdrafts a group as its value list and restores exactly the ticked boxes; a control that can't pick its own entries out of the list (a<select multiple>or text field sharing a group with another contributor) keeps what the server rendered. A draft written before this change has a boolean under the group key and is no longer applied to controls that read a list.Migration
[]now contributes to an array, including a lone one:<input name="tags[]">sends["abc"]where it used to send"abc"— declare such params as array types or drop the suffix.[].Unchanged: a lone checkbox without
[]stays a boolean, a radio group posts its single checked value with or without the suffix, a hidden companion next to a checkbox contributes nothing, and a lone[]-named text field still restores its draft.Written for commit ce9ce15. Summary will update on new commits.