Repository navigation
feat(bestax-migrate): convert Bulma form fields, controls, inputs and textareas - #759
Conversation
|
deep-review: fresh Read-only: don't install, build or run tests; CI covers them on this head. Read the form entries in Skip the MCP and ESLint packages, generated files, fixtures, |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughBulma form classes now map to Bestax field, control, input, and textarea components. The migration planner considers existing Bestax components and generated IDs when planning conversions. Tests, fixtures, and migration references cover the mappings and conversion conditions. ChangesBulma form migration
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Transform
participant Plan
participant RootEntry
participant MigrationResult
Transform->>Plan: Pass element facts and Bestax context
Plan->>RootEntry: Read target and conversion metadata
RootEntry-->>Plan: Return mapping and context rules
Plan-->>Transform: Return conversion plan or refusal
Transform-->>MigrationResult: Emit converted components or retained markup with TODO
Merge Risk: ⚪ Minimal · up to The form conversions appear ready to merge after normal checks; no actionable issue was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The migration can now replace more form markup with components. The reviewed safeguards preserve markup where component context, generated IDs, or child wrapping could change its behavior. No introduced security issue was established, but the assessment does not cover every downstream use. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 12 files. (8 skipped: 8 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://c8fd29fd.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 1 blocking · 4 advisory
| No. | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | A labelled Field reached through a wrapper component is invisible to bestaxAround, so the input converts and adopts the generated id of that Field — and two bare controls under one wrapper get the same id |
bestax-migrate/src/sources/bulma-classes/plan.ts:242-251 |
| 2 | 🔵 Advisory | Robustness | The mirror blind spot: providesContext only sees bestax elements written in this file, so a .field div holding a user component that renders a bestax Input still converts, and that Input then drops the .field it would have rendered. Lower likelihood — the pre-migration markup is already double-wrapped |
bestax-migrate/src/sources/bulma-classes/plan.ts:235 |
| 3 | 🔵 Advisory | Correctness | The adoptsIdFrom TODO asserts "inside a bestax Field with a label", but the guard reads only the name of the ancestor. It also fires for a Field with no label and for a grouped or has-addons labelled Field, where targetId is undefined (Field.tsx:231-234) and no id is ever handed down — a safe over-refusal, but the message describes a situation that is not the one that occurred |
bestax-migrate/src/sources/bulma-classes/plan.ts:249 |
| 4 | 🔵 Advisory | Coverage | Adding textarea to VOID means the render-equality sweep never renders a textarea with children through both paths. Verified by inspection that it is fine: TextAreaBase does not destructure children, so it flows through rest onto the element unchanged |
bestax-migrate/e2e/bulma-classes-truth.test.ts:49 |
| 5 | 🔵 Advisory | Robustness | The old one-line doc comment above VOID ("Elements that must stay empty in HTML") was left in place above the new one; textarea is in the set for a different reason |
bestax-migrate/e2e/bulma-classes-truth.test.ts:44 |
Overall: The table work is careful and the modifier mapping is exact — I cross-checked every class Field, FieldLabel, FieldBody, Control, InputBase and TextAreaBase can emit from their usePrefixedClassNames maps against the six new entries, and each one is either a mapped modifier or deliberately left in className for a reason that holds (component colors, has-addons-*, is-grouped-*, textarea is-loading). The riskiest part is the pair of new context guards: both resolve context by JSX name in this file, while React resolves it by render tree, and the two diverge the moment a user component sits in between. Focus there first — finding 1 reaches an idiomatic composition, a small Labelled component that forwards its children into a labelled Field, and can produce duplicate DOM ids.
Residual risk:
- Context reaching a component the codemod itself writes — refuted. Of the six new targets only
InputBaseandTextAreaBaseread form context, and onlyuseFieldLabelId. A codemod-writtenFieldnever gets alabelprop, sotargetIdisundefined(Field.tsx:231-234) and the provider hands down nothing. No other mapped target inROOTScallsuseInsideField/useInsideControl/useFieldLabelId— checked every consumer underbulma-ui/src/form/. - A bestax control written inside a JSX expression container, such as one returned from a
.mapcallback — refuted. The ancestor walk behindbestaxInsidecallscontinueon non-JSXElement paths rather than stopping, so the enclosing.fieldor.controlis still reached and still refuses. - An indirect bestax control inside a converting
.fieldor.control— open, row 2 above. - Helper props reaching the DOM as attributes — refuted.
FieldLabelandFieldBodyspread their raw props onto the element, and so doesControl, and all three carrynoHelpers: true;Fieldspreads only therestfromuseBulmaClasses, and keeps helpers. That matches the components as written. wrapsChildren.whenchanging behaviour for the existingCardentry — refuted. The negatedwhencheck short-circuits, so every entry without awhenplans exactly as before.- The
refclaims — refuted.FORWARDS_REFgains exactlyControl,InputBaseandTextAreaBase, all threeReact.forwardRef;Field,Field.LabelandField.Bodyare plain FCs and are absent. The truth test asserts that set exactly, andprops-extract.mjsemits arefrow for forwardRef components, so the newpassThroughref entries line up with the index — confirmed againstbestax-mcp/data/components/Control.json. - Numeric-typed attributes such as
rowsandmaxLengthneedingnumberAttrs— refuted. The React type forrowson a textarea is alreadynumber, so a stringrowsnever typechecks on the raw element either;numberAttrsis only needed where bestax narrows a DOM type that was wider, as with a progressvalue.
🏄 Clean set of waves here — six form classes carved one-for-one onto real components, every modifier checked against the source, and the whole thing lands on its feet. Just watch that one reef: the context guards read the file, but React reads the tree, and a little wrapper component in between is where the id sneaks through. Patch that and it is all glass.
… textareas The bulma-classes source left form markup as a family, because bestax's Input and TextArea render .field and .control wrappers of their own. But the pieces map one to one: Field and Control render exactly their Bulma elements, and InputBase and TextAreaBase are the controls without wrappers. - .field, .field-label, .field-body, .control, .input and .textarea are mapped entries now. The modifiers with a prop of their own convert; an input's color stays a class, because color sets has-text-<color> too, and so do has-addons-centered and the is-grouped-* variants. - A horizontal field converts once its label and body do, through a when condition on wrapsChildren. Field.Label, Field.Body and Control spread their raw props onto the element, so helper classes stay on them. - Two context guards, reported as context:<Target>. A .field or .control that already holds a bestax component stays markup, since Field and Control tell bestax's form controls to skip their own wrappers. An input or textarea with no id inside a bestax Field stays markup, since it would take the Field's generated id. - The truth test reads InputBase's and TextAreaBase's props with the API extractor, since neither has an API page of its own, and renders a textarea without children. - A form fixture pair from Bulma's own form markup, and a form in the kitchen sink. Part of #743
…zontal lookup_bulma_classes now covers .field, .control, .input and .textarea from the codemod's table. Field wraps its children in a .field-body of its own only when it's horizontal, so the lookup states that condition for field is-horizontal and not for a plain field.
…onverts no-bulma-component-class reported .field, .input and .textarea as a family converted by hand, and skipped .control, .field-label and .field-body as parts. bestax-migrate converts all six now, so the regenerated metadata reports them with the converts message.
The codemod converts the contact form's fields, controls, input and textarea now, leaving the select and the checkbox. The addendum names it as the third partial family, and the contact row accepts the Control and base-control composition the codemod writes.
512c749 to
7de870d
Compare
|
On the deep review's advisory rows, folded into the
|
|
deep-review: verify Read-only: don't install, build or run tests; CI covers them on this head. Settle the open wrapper-component thread against the current |
Preview DeploymentPreview URL: https://34e39d78.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| No | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | Resolved — labelled Field reached through a wrapper component handed the input its generated id |
bestax-migrate/src/sources/bulma-classes/plan.ts:247 |
Thread settled
- A labelled
Fieldreached through a wrapper component still hands the input its id — verified fixed. The guard no longer keys offbestaxAroundalone:plan.ts:247-260fires whenever anadoptsIdFromentry has noidattribute and picksfacts.bestaxAround?.find(...) ?? facts.componentsAround?.[0], and thearound()helper intransform.ts(410-429) fillsotherwith every JSX ancestor that is not a bestax import, not an intrinsic tag, not a custom element and not inPASS_THROUGH_PARENTS. TheLabelledrepro from that thread is now a test (__tests__/transform.test.ts:479-495): the bare input refuses withcontext:InputBase, theid="email"variant still converts to<InputBase id="email" type="email" />, and aReact.Fragmentwrapper raises nothing — so the fragment exemption did not get swept up with the fix.plan.test.ts:515-527covers thecomponentsAroundfallback branch on its own, and both theinputandtextareaentries setadoptsIdFromto Field (class-map.ts:831,:867), so the sibling target is covered too. The duplicate-idvariant of the repro (two bare controls under one wrapper) is subsumed: neither converts. The one case the walk still cannot see — a wrapper defined in another file — is stated inskills/bestax-migrate/references/bulma-classes/unmappables.md:99-101with the advice to give inputs ids first, which is what the thread asked for on the record.
Overall: This pass settled the one open thread from the earlier review and reviewed no commits — no fresh review was performed, and no new findings were raised. The fix is the narrow, conservative shape that thread suggested (refuse on any unaccounted-for component ancestor rather than reach across files), so the residual cost is lost conversions on inputs under app wrappers, not incorrect output. A maintainer wanting the commits since the original review judged should ask for a deep-review: fresh run.
Residual risk:
- Cross-file wrapper rendering a labelled
Field: still out of reach by construction (the codemod reads one file at a time) — documented rather than guarded, which is the accepted trade-off recorded inunmappables.md. - Over-refusal: any missing-
id.input/.textareaunder a non-bestax component ancestor now stays markup even when that ancestor renders noField. Conservative by design — a skipped conversion, never wrong markup. - Not re-examined this pass: everything outside the settled thread. Per VERIFY mode, no other path was reviewed, so absence of findings here is not a clean bill for the rest of the diff.
🏄 Caught the one leaky spot in the lineup, patched it, and the tests prove the wave still breaks the same way it did before. Cleanly settled — paddle it in.
|
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.16.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Part of #743: the next structural family after Navbar (#754). The
bulma-classessource left form markup as a family (family:field,family:input,family:textarea), because bestax'sInputandTextArearender a.fieldand a.controlof their own. The pieces map one to one, though:FieldandControlrender exactly their Bulma elements, andInputBaseandTextAreaBaseare the controls without wrappers, which is howField's own docs compose them.What converts
.field→Field(is-horizontal,is-grouped,has-addons,is-narrow),.field-label→Field.Label(its size),.field-body→Field.Body..control→Controlon a<div>or<p>(has-icons-left/-right,is-loading,is-expanded, its size)..input→InputBaseand.textarea→TextAreaBase, with their sizes and states.On Bulma's own form markup (the new
formfixture pair) every field, control, input and textarea converts. The.select,.checkboxandspan.iconkeep their family TODOs, and.labeland.helpstay as they are.What stays, and why
is-danger):colorrendershas-text-<color>on it too, so the class stays.has-addons-centered,has-addons-right,is-grouped-*: thehasAddonsandgroupedvalues that render them renderhas-addonsoris-groupedas well.Fieldwraps anything else in a.field-bodyof its own. That's Card'swrapsChildrenrule with a newwhencondition, since a plain field doesn't wrap.Field.Label,Field.BodyandControl: they spread their raw props onto the element as well asrest, so a helper prop would reach the DOM as an attribute too.Context
FieldandControltell bestax's form controls inside them to skip their own wrappers, and bestax's wrapped inputs read that context. So there are two guards, both reported ascontext:<Target>:.fieldor.controlthat already holds a bestax component (anInputfrom an earlier migration by hand) stays markup, because converting it would change how that component renders.idinside a bestaxFieldstays markup, because it would adopt the Field's generated id."Already holds" and "inside" are resolved by binding, like the Card children, so a local that shadows a bestax name doesn't count.
The other surfaces
feat(bestax-mcp)): the lookup states the field's wrap rule only whenis-horizontalis there.fix(eslint-plugin)): the regenerated metadata reports the six classes as converted.context:<Target>, and newfamily:selectandfamily:filerecipes) and the migration guide.test(eval)): the codemod converts the contact form except its select and checkbox, so the addendum names it as the third partial family, and its contact row accepts theControl+ base-control composition. I ran the codemod over the fixture to check.Checked
InputBaseandTextAreaBasehave no API page, so the prop-classification check reads their props with the same extractor the index uses. The harness renders a textarea without children, since React refuses text beside avalue.formfixture pair and the kitchen sink's form render the same before and after, and a second run changes nothing.pnpm allpasses locally.For you
feat(bestax-mcp)andfix(eslint-plugin)commits will ship with each package's next release instead. That's fine either way. Just calling it out.Summary by CodeRabbit