fix: restore legacy path compatibility regressions#96
Conversation
There was a problem hiding this comment.
2 issues found across 6 files
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="packages/core/src/index.ts">
<violation number="1" location="packages/core/src/index.ts:94">
P1: Consecutive indexed segments are merged into one token, which breaks nested indexed path parsing (e.g. `foo[0][1][bar]`).</violation>
</file>
<file name="packages/js2form/src/index.ts">
<violation number="1" location="packages/js2form/src/index.ts:105">
P2: The new bracket normalization drops literal text outside bracket groups (e.g. suffixes), which can generate incorrect field names and break form field matching.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
cubic found two valid issues in the previous patch, and both are fixed in
Added regression coverage for both cases and re-ran @cubic-dev-ai re-run review |
@maxatwork I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac2848f90b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isArraySegment = bracketContent === "" || /^\d+$/.test(bracketContent); | ||
|
|
||
| if (isArraySegment) { | ||
| currentChunk = `${currentChunk}[${bracketContent}]`; |
There was a problem hiding this comment.
Keep sibling
[] Rails fields on the same array item
normalizeName() now routes Rails names like items[][title] and items[][description] through the empty-index array path, but each occurrence gets a fresh synthetic index. In mapFieldsByName() this produces items[0].title and items[1].description, so objectToForm() only fills one field for { items: [{ title, description }] } and leaves the other blank. This breaks a common Rails nested-form shape whenever array object fields omit explicit numeric indexes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid issue. Fixed in 95e4d1a by keeping sibling Rails empty-index fields on the same synthetic array item until a sibling path repeats, so items[][title] and items[][description] map to the same object slot. Added a regression test for that shape in packages/js2form/test/js2form.test.ts and re-ran npm test.
Summary
Fixed Issues
Fixes #74
Fixes #68
Fixes #41
Fixes #39
Fixes #22
Fixes #19
Test Plan
Summary by cubic
Restore legacy Rails-style bracket parsing and name normalization in
packages/coreandpackages/js2form, fix radio value coercion inpackages/dom, and keep sibling Rails array fields on the same synthetic index. Resolves nested object/array regressions and adds tests to lock behavior.packages/core: Parses Rails-style keys with underscores, one-character names, root[prop], and mixed/consecutive indexed arrays.packages/dom: Radio inputs now coerce to true/false/value/null correctly; an empty checked radio isn’t forced to false when true/false siblings exist.packages/js2form: Normalizes Rails-style field names so inputs likea[b][c]populate, keepsitems[][title]anditems[][description]on the same item index, and preserves literal suffixes around bracketed indexes (e.g.,items[5]_id).Written for commit 95e4d1a. Summary will update on new commits.