Conversation
There was a problem hiding this comment.
All reported issues were addressed across 10 files
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
2578fbe to
9f63913
Compare
There was a problem hiding this comment.
All reported issues were addressed across 9 reviewed files. 4 files intentionally excluded from review.
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
a33d472 to
18f9456
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 reviewed files. 4 files intentionally excluded from review.
Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.
Re-trigger cubic
18f9456 to
a374deb
Compare
There was a problem hiding this comment.
1 issue found across 8 reviewed files. 3 files intentionally excluded from review.
Confidence score: 5/5
spec/requests/checkbox_group_bounds_spec.rbduplicates thereceivedhelper,payloadsetup, and custom empty-groups POST fromspec/requests/checkbox_group_params_spec.rb, increasing test maintenance and inconsistency risk without indicating a product regression — consolidate the shared setup or clarify the distinct coverage.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/requests/checkbox_group_bounds_spec.rb">
<violation number="1" location="spec/requests/checkbox_group_bounds_spec.rb:9">
P3: This spec duplicates the `received` helper, `payload` let, and custom empty-groups POST from `spec/requests/checkbox_group_params_spec.rb` — all three files test the same issue-#258 feature against the same dummy component, and the extraction regex/unescape/parse logic is byte-identical. Extract a shared helper (e.g. `spec/support` module: `received_json(response)`, a default `payload`, and a `post_announced(component, act, params, empty_groups:)`) so the announcement/bounds/scope specs stop maintaining three copies of the same wire format.</violation>
</file>
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
| # array. Without that, the field reaches every array param the schema has, from | ||
| # anywhere params come from, and an invented name writes a junk key. | ||
| RSpec.describe "Announcement is bounded by the schema (issue #258)", type: :request do | ||
| def received(response) |
There was a problem hiding this comment.
P3: This spec duplicates the received helper, payload let, and custom empty-groups POST from spec/requests/checkbox_group_params_spec.rb — all three files test the same issue-#258 feature against the same dummy component, and the extraction regex/unescape/parse logic is byte-identical. Extract a shared helper (e.g. spec/support module: received_json(response), a default payload, and a post_announced(component, act, params, empty_groups:)) so the announcement/bounds/scope specs stop maintaining three copies of the same wire format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/requests/checkbox_group_bounds_spec.rb, line 9:
<comment>This spec duplicates the `received` helper, `payload` let, and custom empty-groups POST from `spec/requests/checkbox_group_params_spec.rb` — all three files test the same issue-#258 feature against the same dummy component, and the extraction regex/unescape/parse logic is byte-identical. Extract a shared helper (e.g. `spec/support` module: `received_json(response)`, a default `payload`, and a `post_announced(component, act, params, empty_groups:)`) so the announcement/bounds/scope specs stop maintaining three copies of the same wire format.</comment>
<file context>
@@ -0,0 +1,57 @@
+# array. Without that, the field reaches every array param the schema has, from
+# anywhere params come from, and an invented name writes a junk key.
+RSpec.describe "Announcement is bounded by the schema (issue #258)", type: :request do
+ def received(response)
+ json = response.body[%r{data-testid="received">(.*?)</pre>}m, 1]
+ JSON.parse(CGI.unescapeHTML(json))
</file context>
There was a problem hiding this comment.
Valid, and taken up the way you sketched it: received_json(response, testid:) and
post_announced(...) live in spec/requests/support/action_request_helpers.rb, which
is where this suite already keeps its shared request helpers.
Two notes on scope. The duplication is older than this PR — six copies of the extraction
at the previous head, three of them predating it (bracket_params_spec,
nested_params_spec, multipart_nested_params_spec). This change removes the three it
created and leaves the three it did not: converting them is a convention call for the
repo, not part of a bug fix. Say the word and it is three times three lines.
The POST helper went one step further than a spec-local extraction. That support file
exists to dogfood the PUBLIC TestHelpers (#110), and a private copy of the wire shape
is exactly what it is meant to prevent — the real gap was that this PR introduced a wire
field without extending the shipped helper, so any user testing a cleared group of their
own component had to hand-roll the body. post_reactive_multipart takes empty_groups:
now; omit it and the request is byte-for-byte what it was (measured on rack 3.2.7: an
empty array disappears from a multipart body and params[:empty_groups] is nil).
a374deb to
49dbb39
Compare
There was a problem hiding this comment.
1 issue found across 14 reviewed files. 3 files intentionally excluded from review.
Confidence score: 3/5
- In
lib/phlex/reactive/param_schema.rb, nested array declarations such asmatrix: [[:string]]can ignore a clearedmatrix[0][]group, potentially leaving stale or incorrectly accepted parameters; updatenested_attributes_step?/declares_array?handling and add coverage for cleared nested arrays.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/phlex/reactive/param_schema.rb">
<violation number="1" location="lib/phlex/reactive/param_schema.rb:218">
P2: For nested array declarations such as `matrix: [[:string]]`, a cleared `matrix[0][]` group is ignored. `nested_attributes_step?` only accepts array-of-hash nodes, so `declares_array?` returns false after reaching the outer array; traverse numeric indices for nested array elements and recognize the element array at the leaf.</violation>
</file>
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
49dbb39 to
c35828d
Compare
There was a problem hiding this comment.
0 issues found across 1 file (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.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
`#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, the nested rows — 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.
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.
With nothing ticked the group stays an empty array rather than vanishing, so an
action can tell a cleared group from one that never rendered.
A form body cannot carry an empty array, so a cleared group is ANNOUNCED: its
key stays absent from `params` and its name rides in `empty_groups[]`, a field
of its own beside token/act/params, which `coerce_params` fills with `[]`. The
field is additive — a request without it behaves exactly as before, so an old
client against a new server and a new client against an old server both keep
today's behaviour — values sent for a group always win over an announcement,
and a bracketed name resolves to the depth `params` itself nests at.
A blank entry (`params[name][]=""`) was the obvious alternative and is
ambiguous. Rails leaves `[""]` to the caller — `collection_check_boxes` ships
exactly that marker and the app filters it — and the schema reads it per
element type, measured: `[:string]` keeps `[""]`, `[:integer]` coerces `[0]`,
`[:date]` and `[:file]` drop the key so the keyword default stands. Reading it
as "cleared" would have changed all four, and for a `[:file]` param backing a
`has_many_attached` that is the difference between "the field did not come in"
and purging the attachments. Those four readings are pinned in
`spec/phlex/reactive/param_schema_spec.rb`.
The announcement is applied BEFORE `unwrap_scope`, because the client sends the
raw DOM name: a `reactive_scope :todo` component announces `todo[tags]`, and the
group has to be placed at that depth first so that peeling the scope afterwards
finds it — the same way it finds a value the client actually sent. The other way
round, a scoped component's cleared group reached the action as nil over a form
body while the JSON path gave it `[]`, which is the disagreement this change
exists to remove. `ActionController::Parameters` converts a hash on assignment,
so a parent node created for a bracketed name is read back before it is written
into; otherwise the write lands in a copy.
An announcement only fills what the action DECLARED as an array: the name is
resolved against the compiled schema, so the field cannot reach an array param
the component never renders as a group, cannot fabricate `[]` for a scalar, and
cannot build nesting of its own — an invented `a[b][c][d]` resolves against the
declared shape or not at all. Names arrive through the same bracket resolution
`params` uses; `ParamSchema.bracket_path` is now public so there is one parser
for the format rather than a copy per reader.
That walk is `ParamSchema#declares_array?` rather than a private method of the
endpoint, for the reason zoolutions#109 gave when it moved the coerce family: reading a
declaration is schema work, and there the walk's rules are reachable from a
unit spec. Through the endpoint they are not — coercion silently discards what
a wrong answer would write, so a request spec answers the same for a rule that
holds and one that does not. The walk steps over the ROW INDEX of a
nested-attributes declaration, which describes its element once while the wire
names a row: without that, `rows_attributes[0][features]` resolved against an
Array, the walk gave up, and an emptied group inside a row stayed
indistinguishable from one that never rendered. Measured over a request that
carried one row and announced its cleared group, the action received the row
without the group where `features: []` was owed.
Stepping over anything OTHER than an index would resolve names the declaration
does not describe, which is why the step asks for a row index and not merely
for a next segment.
A row index is FOLLOWED, never CREATED. The endpoint creates a missing parent
so that a group which is the only thing its container carried can still be
announced — right for a scope or a nested object, and a fabricated child record
for a nested-attributes row: measured, an announcement of
`rows_attributes[0][features]` on a request carrying NO params at all handed
the action `[{"features" => []}]`, and three announcements handed it three
rows. The refusal covers every segment still to be created rather than the row
alone, because bailing at the index after the container above it was created
leaves that container behind, and an empty collection there reads as "the
caller cleared every row" — the same fabrication one level up. All three
answers are distinguishable at the action, which is what the two new examples
pin.
`post_reactive_multipart` takes `empty_groups:`, so a request spec can
reproduce a cleared group the way the client sends it; omit it and the body is
byte-for-byte what it was. The field arrived without the helper, which left the
gem's own suite hand-rolling the POST that the shipped helper exists to spare a
caller.
The schema lookup reads both key forms, because `ParamSchema.compile` keeps the
keys it was given and a declaration written with strings is as valid as one
written with symbols.
`reactive_persist` drafts such a group as the list of ticked values and restores
exactly those boxes; a generic control in a MIXED group keeps what the server
rendered, because the draft holds ticked values and generic values in document
order with nothing saying which came from where — restoring the list into a text
field would paste "freeform,news" into it; 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. The rule that a
generic control keeps the server's value sits ABOVE the restore's branch chain,
and is repeated at the top of the deferred editor path: below the editor
branch it would have missed exactly the controls that land last in the
snapshot, and the deferred path applies an editor that upgraded after connect
without passing the chain at all.
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 that ticked every box of the group for as
long as the draft lived, seven days by default; 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. A multi-select is
covered with the checkboxes, because 0.13.2 wrote one value per NAME with the
last writer winning: a checkbox in a mixed group could leave its boolean under
the select's name, and under `restore: "always"` the select branch would then
match `Set{"true"}` against its options and deselect everything the server had
rendered. 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 is announced, every
other empty array param is still omitted next to a file.
…#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. Over JSON: the array arrives as an array, an empty group stays `[]` rather than collapsing to nil, a group that never rendered stays nil, and an indexed hash coerces into the declared array — named for what it is, since `post_action` sends JSON and that shape is what a `fields_for` collection produces, not what a checkbox group sends. Over a real multipart body via `post_reactive_multipart`: a repeated `params[features][]` group coerces to the array, a lone blank entry reads back as the empty selection, and a blank that arrives alongside real values stays a value. 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. The two bounds examples that read the component's reflected arguments could not see what they claimed to: the component reflects its own keywords, so a junk key an unbounded announcement writes into the raw params never reaches them — both stayed green with the schema bound deleted. They read the dropped-param line instead, which does see the raw params, with a genuinely undeclared param in the same request as the positive control, so a silent log cannot satisfy a negative expectation for free. With the bound deleted, three of the six examples fail where one did. `save_rows` declares a `[]` group inside a nested-attributes row, the shape the README documents, so the announcement's resolution through a row index is covered end to end rather than only in the schema unit spec — both that it fills a row the request carried and that it never brings a row into being. The negative side is readable through the keywords alone: the unbounded answer is `[{"features" => []}]`, a half-fix that leaves the container behind gives `[]`, and only nil means the raw params were not touched. `received_json` and `post_announced` moved into the request suite's shared support file next to the legacy aliases, and `post_announced` is now a thin call on the public `post_reactive_multipart` — these specs exercise the shipped helper rather than a private copy of the wire shape. A miss on the `<pre>` says so instead of surfacing as a TypeError from inside the helper.
c35828d to
c9a46a7
Compare
There was a problem hiding this comment.
0 issues found across 9 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.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
sorry, that escalated a lot between cubic and my agent while I was sleeping, I only expected the first 5 issues to be fixed. I‘ll review it and clean it up first before bothering you. |
…oolutions#258) The minimal branch of this fix went through several review rounds; this brings the corrections here so both branches run the same collector and persist logic. Only the persist and collector region is ported: the wire field (`empty_groups[]`) and everything around `#buildFormData` are this branch's own and stay untouched. What the rounds found, each measured before and after: * The restore decided "did the server already tick a box of this group?" per control, inside the loop that writes `checked` as it goes. The first box it ticked made every later box look server-rendered, so a draft of two values came back as one. The decision is taken once before the walk now. * A `[]`-named control with no second contributor lost its draft: a lone `<input type="text" name="tags[]">` used to round-trip and stopped doing so once groups drafted as lists. A group of one is unambiguous and reads it again; two or more keep what the server rendered. * A `<select multiple>` sharing a group with another contributor treated every entry as one of its options, so a text value that happened to equal an option selected it. It keeps the server's selection in that case. * `#collectFields` read named editors and bare contenteditables in a second pass that ASSIGNED, so a `[]`-named editor posted a scalar while the draft held an array. It appends to the group slot now. * An editor that has not upgraded contributes nothing to a group. Trix defines its elements in a setTimeout after load, and its "" is an absent value rather than an empty one; persistSnapshot omits such an editor for the same reason. * A scalar already under a `[]` name can only be a radio's, and the editor leaves it alone: converting it discarded the chosen value. A hidden input sharing a `[]` name with a named editor keeps contributing. Reading it as that editor's twin was tried and dropped: nothing in the DOM tells a mirroring hidden from a list JS maintains, the shape it was built for does not occur (Lexxy names the element, Trix names the HIDDEN and leaves the editor unnamed, a bare contenteditable has no hidden), and it cost a measured silent loss where a radio, a hidden and a contenteditable shared one name. A doubled value is visible on the wire; a swallowed one is not. The editor DOM is walked once for both passes, and the CHANGELOG paragraph gives each of these behaviours its own sentence.
…lutions#258) Both described the editor-companion rule this branch took out again, and both sat next to the code that contradicts them. 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 suppressed: nothing in the DOM tells a hidden that mirrors an editor from one that is a list JS maintains, and the rule cost a measured silent loss. The CHANGELOG said an editor "stands down beside a radio". That holds only when the radio is the only native control under that name, since a second control is what makes the name a group. Measured: a radio, a hidden and an editor sharing one name post all three values. No behaviour changes. The sourcemap moves with the comment, so the client is rebuilt and the vendored copies re-synced — `rake build:js_check` fails otherwise.
…oolutions#258) The guard for "a same-named hidden keeps contributing" cited `{"pick[]": ["a","typed"]}` — a measurement from the radio case three tests down, on a fixture this one does not use. On its own fixture the suppressing rule produced `["typed"]`, with the hidden's value gone. A number that reads like evidence and belongs to another setup is worse than none. Comment only; no client source changed, so the bundle and its sourcemap stay as they are.
What
#collectFieldswrotefields[name] = field.checkedfor every checkbox — a boolean under thecontrol's own name. Three boxes sharing
features[]with two ticked therefore left the browser as asingle
false, the last box's checked state, and same-named boxes overwrote each other before anyschema could see them. Against a
[:string]schema the action received{}; against a flat:stringschema it received"false". Both silent. A native submission of the same three boxessends
features[]=news&features[]=events.A name is now collected as an array when it carries the
[]suffix or when a second non-radiocontrol shares it: a checked box contributes its
value, an unchecked one nothing, a<select multiple>its selected options, any other control its value. With nothing ticked the groupstays an empty array rather than vanishing, so an action can tell a cleared group from one that never
rendered —
ParamSchema#bracket_pathdrops the empty trailing segment and#array_valuespasses areal array through, so an
[:string]schema coerces[]to[].Two shapes keep their behaviour deliberately: a lone checkbox without
[]stays the documentedyes/no boolean, and a radio group — which shares one name by design to mean "pick one" — keeps
posting its single checked value.
The multipart path writes such a group as repeated
params[name][]entries. The generic appender'sindexed form (
params[name][0]) arrives as a hash of index keys; an array type normalizes that back,but only an array type does, so the JSON and multipart bodies would otherwise stop coercing
identically for the same fields.
Fixes #258.
Tests
spec/javascript/reactive_collect_checkbox_groups.test.js— seven cases against the POST body:the group's values, an empty group, a lone checkbox, a
<select multiple>, two controls sharing aname without
[], a radio group, and the multipart wire shape. Four fail onmainbefore thechange; the multipart one fails if only the
#buildFormDatahalf is missing. The lone-checkbox andradio cases pass on both sides — they are regression guards for the two shapes that must not move.
spec/requests/checkbox_group_params_spec.rb— six examples for the server half: the arrayarrives as an array, an empty group stays
[], a group that never rendered stays nil, the lonecheckbox stays boolean, a
<select multiple>group coerces the same way, and the indexed hash amultipart body produces coerces to the same array. These pass on both sides — the server was never
the broken half; they pin the shape the client now sends.
spec/system/checkbox_group_spec.rb— two examples in a real browser against a real server,through the vendored minified client: untick one box, tick another, add a second option to the
select, save, and read what the action received. Against the old client this reported
"features":null— the exact symptom.spec/dummy/app/components/checkbox_group_component.rb— the component both specs drive, in theshape of
NestedParamsComponent.Full suites on this branch: 653 JS tests and 1741 RSpec examples, 0 failures.
Why a cleared group is announced instead of marked
A form body cannot express an empty array, so a cleared checkbox group needs
somewhere to live. Three options, and why the announcement won:
group over JSON and leaves it untouched over a form body, and the difference
only shows when a form happens to carry a file. Transport-dependent behaviour
for identical input.
params[name][]="". Ambiguous: Rails ships exactly thatmarker from
collection_check_boxesand leaves it to the application, and theschema reads it per element type —
[:string]keeps[""],[:integer]coerces
[0],[:date]and[:file]drop the key. Claiming it means"cleared" changes all four.
empty_groups[], a field of its own besidetoken/act/params. Unambiguous, additive (old client/new server and newclient/old server both behave as they do today), and it touches nothing but
the groups the client names.
One implementation note worth keeping:
ActionController::Parametersconverts ahash on assignment, so the object handed to
[]=is not the one stored. Aparent node created for a bracketed name has to be read back before writing into
it, or the write lands in a copy and the action sees an empty hash.
Out of scope, noticed while here
#collectFieldsnever filtersdisabledcontrols, although the helpersdocument that a disabled trigger "never submits" — pre-existing, and now more
visible because a disabled ticked box contributes a real value to a group.
#showFieldValueresolves a name first-wins from the DOM while the nested-rowhelpers resolve last-wins; both differ from the collector.
[]name today —reactive_field(:features, multiple: true)would let the DSL say it, which isthe maintainer's call.
Summary by cubic
#collectFieldsused to writefields[name] = field.checkedfor every checkbox, so boxes sharing afeatures[]name overwrote one another and the action received the last box's boolean instead of the ticked values, or nothing at all. A name ending in[]is now collected as an array of the chosen values; a lone checkbox stays a boolean, a radio group its single checked value, and<select multiple>its selected options.[]over JSON; over a form body the client announces it inempty_groups[], which the endpoint fills with[].reactive_scopecomponent and a group inside a nested-attributes row resolve correctly; announced names can never fabricate rows or write junk keys.reactive_persistdrafts and restores a group as the ticked values; old drafts still restore, and the confirm-predicate docs describe the array shape."features":null.Review-round corrections to the collector and persist logic:
[]-named control keeps its draft; mixed groups keep the server's values.<select multiple>in a mixed group keeps its server selection instead of matching text values against options.Fixes #258.
Written for commit 70cc7f2. Summary will update on new commits.