Repository navigation
feat(bestax-migrate): convert .select and .breadcrumb with the element inside them - #764
Conversation
|
deep-review: fresh Read-only: don't install, build or run tests; CI covers them on this head. Read Skip the MCP and ESLint packages, generated files, fixtures and the markdown. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 38 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 (25)
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 (25)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe migration planner and transformer now support converting a mapped wrapper together with its sole child. The Bulma mappings add ChangesAbsorbed component conversions
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant transform
participant plan
participant JSX_AST
transform->>plan: ElementFacts with soleChild
plan-->>transform: Conversion with absorbed props and attribute changes
transform->>JSX_AST: Replace wrapper with component on child
Merge Risk: ⚪ Minimal · up to The reviewed conversions preserve the investigated accessibility label and keyed component identity. No actionable merge-blocking issue remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The conversion has a meaningful design surface, but the inspected paths preserve or reject sensitive markup rather than silently moving it. No introduced security issue was established. Coverage is incomplete, so this is not a minimal-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 18 files. (7 skipped: 7 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://c8a1e220.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 · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | A non-literal size={expr} renames to multipleSize and SelectBase then drops it, because attributeValue cannot tell size={3} from size={rows} |
bestax-migrate/src/sources/bulma-classes/plan.ts:604 |
| 2 | 🔵 Advisory | Robustness | The top-level drop loop runs before absorbs.renames are applied, and removeAttr(holder, undefined) is a silent no-op — unreachable today only because no absorbs entry declares untypedAttrs |
bestax-migrate/src/sources/bulma-classes/transform.ts:714 |
| 3 | 🔵 Advisory | Robustness | inTags re-attaches tagComments(child) to the name while child.openingElement/closingElement themselves survive the replace, so a comment recast hangs on either node would print twice; the new test asserts toContain, not a count |
bestax-migrate/src/sources/bulma-classes/transform.ts:709 |
| 4 | 🔵 Advisory | Coverage | orList's single-name branch is unreachable — the only entry with absorbs.modifiers has two |
bestax-migrate/src/sources/bulma-classes/plan.ts:470 |
Overall: The design is sound and the guards are unusually thorough — for attributesOn: 'child' the element may carry nothing but a key, for attributesOn: 'element' the child may carry nothing at all, and absorb() runs before the attribute/defaults/adoptsIdFrom checks so those read the attributes the component is actually given (which is what makes adoptsIdFrom correct for the <select>'s id). The riskiest part is the one place a converted value is forwarded conditionally rather than verbatim: SelectBase.tsx:112 writes size only when typeof multipleSize === 'number', and the planner cannot see the difference between size={3} and size={rows} — that is finding 1, and the only blocking one. Look at that first; everything else is on the record rather than in the way.
Residual risk:
- Source lost when the component is written in the child's place — refuted. For
.selectthe planner refuses on any element attribute butkey(plan.ts:568), andkeyrides along on the merged attribute list (transform.ts:731); for.breadcrumbit refuses on any child attribute at all. Checked againstabsorbed.input/output.tsxand the truth sweep's refusal table. - Children or whitespace dropped by
elementPath.replace(child)— refuted.soleChildonly filters JSXText thatreachesReact()rejects, so a comment or text beside the child refuses (children:SelectBase, covered by a new transform test), and the child's own children are never touched. keymoved or duplicated — refuted. Akeyon the child refuses (attr:key, andkeywas added to the rule vocabulary so the id stays informative); akeyon the element stays on the component in its place, which is the same position in the array.- The
ida labelledFieldinjects intoSelectBase— refuted.adoptsIdFrom: ['Field']walks every ancestor throughbestaxAround/componentsAround, andFieldpublishes atargetIdonly when given alabelprop (Field.tsx:230-236), which a same-run-converted.fieldnever gets — so the id-lessSelectBaseinside a convertedFieldinform.output.tsxis markup-identical. - Class divergence between the raw pair and the component — refuted. The main truth sweep now renders both new roots for the root alone, for every modifier and for every helper token;
is-primarystays a class (omits+textColor: null) exactly as it does for.input/.textarea, andbgColor: 'backgroundColor'lands on the wrapper either way. - Other conditionally-forwarded props — checked and clear.
SelectBase.tsx:112is the only runtime type check in either component;Breadcrumb'svalidBreadcrumbAlignments/Separators/Sizesguards only ever see literals the modifier table wrote. - Open (low):
omitsis documentation only — nothing inplan.tsreads it, so anomitsentry drifting frommodifiersfails nothing. Pre-existing and shared with.input/.textarea, not introduced here.
🏄 Clean set, dude — the codemod paddles out, swallows the
<select>whole and comes back up with the same markup it went in with. Just one gnarly little ripple where asize={expr}can get washed off the board in plain JS; patch that and this one's glassy all the way to the beach.
…t inside them SelectBase renders the <select> inside .select itself, and Breadcrumb the <ul> inside .breadcrumb, so each pair is two raw elements that become one component. A new entry field, absorbs, lets a root convert together with its only child: the component is written in the child's place, with the element's props first and the child's attributes after them. - SelectBase gives its <select> every attribute it's given, so the .select may carry none but a key, and is-hovered and is-focused on the <select> become props. multiple pairs with .is-multiple, and size becomes multipleSize beside it. - Breadcrumb renders its <ul> bare, and writes aria-label="breadcrumbs" unless it's given one, so the <ul> carries nothing and the nav needs a label of its own.
lookup_bulma_classes used to call .select and .breadcrumb families converted by hand. It now names SelectBase and Breadcrumb, says each renders the element inside it, and says what moves onto the component and what the <select>'s classes and size become.
The generator reads the codemod's table, so the rule now reports both with the converts message instead of "converted by hand".
1eaeef2 to
a9d8082
Compare
|
On the deep review's advisory rows:
All three are folded into the |
|
deep-review: verify Read-only: don't install, build or run tests; CI covers them on this head. Verify the fix for the open thread and the three advisory rows of the last review: the rename branch of Skip the MCP and ESLint packages, generated files, fixtures and the markdown. |
Preview DeploymentPreview URL: https://33c318e1.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| Thread | Severity | Finding | Outcome |
|---|---|---|---|
plan.ts:610 |
🟡 Minor · Correctness | A non-literal size={expr} renamed to multipleSize and SelectBase then dropped it |
✅ Verified fixed — replied and resolved |
Overall: This pass settled threads and reviewed no commits. childAttributeValue (transform.ts:228) now carries a number literal through as a number, so the rename branch of absorb() can tell size={3} from size={rows} and refuses everything that is neither a number nor a string; given() keeps the literal verbatim so numberAttrs does not touch it, and a non-renderable string (size="04") still refuses downstream. The three advisory rows of the last review were all addressed in the same push and are noted below — nothing new was raised, per verify mode.
Advisories from the previous review, re-checked (no threads were open on these):
- Drop-before-rename ordering (
transform.ts:728-735) — addressed.absorbs.dropandabsorbs.renamesnow run inside theif (child)block before theconversion.droploop, so aconversion.dropnaming a renamed attribute finds it on the child;holderstill falls back tochild, which is non-undefinedwheneverconversion.absorbsis set. - Comment duplication in
inTags(transform.ts:717-724) — addressed.inTagsnow reads onlychild.openingElement.name/child.closingElement?.name, whose nodes are discarded by thechild.openingElement.name = element.openingElement.namesplice, while theopeningElementandclosingElementnodes of the child survive and keep their own comments.renameElementbuilds fresh name nodes, so the tag comments captured from the element are re-attached rather than duplicated. The new "keeps each comment in the tags, once" test counts occurrences (split(...).length - 1) across six comment positions instead of assertingtoContain. orListsingle-name branch (plan.ts:469-476) — addressed. The helper is now also used by thewrapsChildrenrefusal, where theunlesslist oncard-headerholds a single name (Card.Header.Title), so the branch is live. The extraction is behaviour-identical to the inlinelistit replaced.
Residual risk:
- Other value kinds reaching the rename — refuted.
literalValueOfyields exactlystring | number | boolean | expression;childAttributeValuemapsnumberto itself and everything else throughattributeValue, which returnstrueonly for a boolean literal that is true, elsenull. So a baresize,size={true},size={false}and any expression all land onnull/trueand refuse. - A number literal leaking into a name-blind check — refuted. Every downstream consumer of the merged
attributesmap (dangerouslySetInnerHTML,ownProps,defaults/missing,adoptsIdFrom,dropsAttr) testshas(name); the two that read values (untypedAttrsequality,numberAttrs) both treat thenulla number literal collapses to as "not convertible", which refuses or skips rather than mis-writing. pairsseeing a number — refuted.spec.pairsrequireschild.attributes.get(name) === true, somultiple={1}refuses.- Open (low): the
Build and Testcheck was stillin_progressona9d80822when this pass ran, so the three new test blocks were verified by reading, not by execution (the steer is read-only). The React 18/19 matrix is green but coversbulma-uionly.
🏄 Came back out for one last check on that
sizeset, and yeah — the codemod now knows a real number from a mystery wave and lets the sketchy ones roll on by. Threads all closed out, board is waxed, this one is good to paddle in.
|
🎉 This PR is included in version 2.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.5.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, after the wrapper folds (#761).
SelectBaserenders the<select>inside.selectitself, andBreadcrumbrenders the<ul>inside.breadcrumb. So in both, two raw elements become one component, the other way round from a fold: the outer element names the component, and the inner one is the part it renders. This adds that.How it decides
absorbs, names the child's tag and where the component puts the attributes it's given. The planner gets the element's only child as a new fact (soleChild: its tag, classes and attributes), and the transform writes the component in that child's place: the element's props and attributes first, then the child's, and the element goes. Writing it in the child's place keeps the child's children where they were, and comments in the tags that go move onto the component's name.SelectBaseputs every attribute on the<select>, so the.selectmay carry nothing but akey, and the<select>'s attributes becomeSelectBase's.is-hoveredandis-focusedon the<select>become props, and any other class on it keeps the pair as markup.multiplerenders both.is-multipleon the wrapper and the attribute on the<select>, so the two convert only as a pair, andsizebecomesmultipleSizebeside it (a number, or the expression as written). A select's color stays a class, like an input's, sincecolorrendershas-text-<color>too.Breadcrumbrenders its<ul>bare and puts what it's given on the<nav>, so the<ul>carries nothing. It writesaria-label="breadcrumbs"before those attributes, so a.breadcrumbconverts with any label of its own, and without one it getsdefaults:Breadcrumb.children:), an attribute that would move or be lost (attr), a spread or a computed class on the child (spread:,dynamic-class:).The other surfaces
feat(bestax-mcp)):lookup_bulma_classesnamesSelectBaseandBreadcrumbwhere it used to say "by hand", says each renders the element inside it, and says what moves where..is-multiplesays it needs a baremultipleon the<select>.fix(eslint-plugin)): the rule reports both with the converts message. Its "two families" test used.select, so it uses.dropdownand.modalnow.family:selectandfamily:breadcrumbrecipes are gone. Theref:section also loses a sentence naming which components forward refs, which had gone stale.Checked
multiplepair and thesizerename, and every refusal. Mappingis-hoveredto the wrong prop fails it.multipleSize={4}; writing that as a string fails it.absorbedfixture pair and both families in the kitchen sink render the same before and after, and the form fixture's select now converts. A second run changes nothing.pnpm allpasses locally.Summary by CodeRabbit
New Features
.selectmarkup can now be converted toSelectBase, including supported attributes and hover, focus, and multiple-selection settings.Breadcrumb, with alignment, separator, and size settings supported.Documentation