feat(bestax-migrate): convert .tabs and .icon around their children

[allxsmith]

Closes #808. .tabs and .icon were left as markup whole. With children, Tabs and Icon each render Bulma's element around them as given, so both convert around their children:

<div className="tabs is-toggle is-toggle-rounded is-fullwidth mt-2">
  <ul>
    <li>
      <a>
        <span className="icon is-small" aria-label="Pictures">
          <i className="fas fa-image" aria-hidden="true"></i>
        </span>
        <span>Pictures</span>
      </a>
    </li>
  </ul>
</div>
// becomes
<Tabs toggle rounded isFullwidth mt="2">
  <ul>
    <li>
      <a>
        <Icon size="small" aria-label="Pictures">
          <i className="fas fa-image" aria-hidden="true"></i>
        </Icon>
        <span>Pictures</span>
      </a>
    </li>
  </ul>
</Tabs>

How it decides

• .tabs → Tabs (<div>): is-centered/is-right/is-left → align, is-small/is-medium/is-large → size, and is-fullwidth, is-boxed, is-toggle and is-toggle-rounded → their flags. is-<color> stays a class, since color is deprecated. The <ul>, <li> and <a> inside stay as written.
• Tabs passes its active tab to Tabs.Tab and Tabs.Content.Item, and renders differently around a Tabs.Content. So providesContext becomes { what, readBy }: a .tabs already holding one of those stays markup, and one holding any other bestax component converts. Field and Control name no readers, so they keep refusing around any bestax component.
• .icon → Icon (<span>), with is-small/is-medium/is-large → size. Icon writes aria-label="icon" unless it's given one, so an element without one gets a defaults:Icon TODO.
• Icon picks its render by whether it has children: with none it renders its own <i> from name. A new entry field, needsElementChildren, has it convert only around HTML elements written out, since an expression child could come out empty.

The other surfaces

• MCP (feat(bestax-mcp)) and ESLint (fix(eslint-plugin)): regenerated for the two, and the MCP agree test gives needsElementChildren entries an element child the way the planner needs.
• Skill and docs: component-map rows, the modifier rows in the prop map, the context and defaults sections in unmappables, and the guide's list of families. The form fixture's icon now converts with its defaults TODO.

Checked

• The render truth test and props e2e sweep both entries, each given an <i> to render around.
• A tabs fixture pair from Bulma's own markup renders the same before and after, with an icon missing aria-label kept as markup.
• Plan tests cover readBy, a .tabs around bestax components that don't read it converting, and needsElementChildren refusing an expression child.
• pnpm all passes locally.

[coderabbitai]

Warning

Review limit reached

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: allxsmith/bestax/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 6fdf1213-a33e-4f23-94fa-cb046ed36c47

📥 Commits

Reviewing files that changed from the base of the PR and between 25f274c and 46cff0c.

⛔ Files ignored due to path filters (1)

• eslint-plugin/src/generated/metadata.ts is excluded by !**/generated/**

📒 Files selected for processing (15)

• bestax-mcp/data/bulma-classes.json
• bestax-mcp/data/skills.json
• bestax-mcp/src/__tests__/bulma-classes-agree.test.ts
• bestax-migrate/e2e/bulma-classes-props.test.ts
• bestax-migrate/e2e/bulma-classes-truth.test.ts
• bestax-migrate/src/sources/bulma-classes/__testfixtures__/form.output.tsx
• bestax-migrate/src/sources/bulma-classes/__testfixtures__/tabs.input.tsx
• bestax-migrate/src/sources/bulma-classes/__testfixtures__/tabs.output.tsx
• bestax-migrate/src/sources/bulma-classes/__tests__/plan.test.ts
• bestax-migrate/src/sources/bulma-classes/class-map.ts
• bestax-migrate/src/sources/bulma-classes/plan.ts
• docs/docs/guides/getting-started/migration/bulma-classes.md
• skills/bestax-migrate/references/bulma-classes/component-map.md
• skills/bestax-migrate/references/bulma-classes/prop-map.md
• skills/bestax-migrate/references/bulma-classes/unmappables.md

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[allxsmith]

deep-review: fresh

Runs here get stopped by the runner at 15 to 20 minutes (#800), so post your review by about your 30th tool call, even if short, and list anything you didn't reach under Residual risk. Don't run builds or whole suites; CI runs them on this head.

Review only the core: the tabs and icon entries in bestax-migrate/src/sources/bulma-classes/class-map.ts, against bulma-ui's Tabs.tsx and Icon.tsx, and the providesContext readBy and needsElementChildren checks in plan.ts. Does each render exactly the markup it replaces (modifiers, the class merge, aria-label), and is everything that would differ refused, including a bestax component inside .tabs that reads its context under a name readBy misses?

[github-actions]

Preview Deployment

Preview URL: https://546f9e58.bestax.pages.dev

[claude]

Deep review — 1 blocking · 3 advisory

No.	Severity	Area	Finding	Location
1	🟡 Minor	Correctness	readBy lists three of the four reachable spellings of the tab panel item; <TabsContent.Item> slips through, so .tabs converts and gains a context provider	bestax-migrate/src/sources/bulma-classes/class-map.ts:1745
2	🔵 Advisory	Coverage	The truth/props harness feeds entry.defaults back verbatim (aria-label="icon"), so it cannot distinguish "Icon forwards a given aria-label" from "Icon always writes its default" — only the tabs fixture render test (aria-label="Info") proves the pass-through	bestax-migrate/e2e/bulma-classes-truth.test.ts:76
3	🔵 Advisory	Robustness	providesContext consults only bestaxInside, never app components that could render a reader; a .tabs div holding <MyTabRow /> converts even if MyTabRow renders Tabs.Tab. Pre-existing for Field and Control, now inherited by Tabs	bestax-migrate/src/sources/bulma-classes/plan.ts:363
4	🔵 Advisory	Robustness	needsElementChildren is stricter than Icon needs — a text-only child is refused, and a cond && child yields false or null (still the children branch), never undefined. Conservative in the safe direction, so on the record rather than as a defect	bestax-migrate/src/sources/bulma-classes/plan.ts:376

Overall: The core is sound and the modelling is the right one — I checked both entries against the components, and each renders what it replaces. Tabs with no Tabs.Content child takes the backward-compatible branch and emits classNames(tabsClasses, bulmaHelperClasses, className), so align, size, isFullwidth, boxed, toggle and rounded round-trip exactly, and keeping the omitted is-<color> as a class is right twice over: Tabs.color is deprecated and passing it logs a dev warning. The Icon children branch renders the container with its aria-label BEFORE the rest spread, so a passed aria-label="Info" wins over the "icon" default, and the defaults refusal keeps the no-label case as markup — which the form and tabs fixtures both show. ownProps is complete against both prop types (diffed member by member), and putting style in passThrough is correct, because Icon destructures it and applies it explicitly. The riskiest part is widening providesContext from "any bestax component" to a name list: an allowlist is only as good as its spellings, and finding 1 is the one hole. Look at that first.

Residual risk:

• A reader under a name readBy misses — open, posted as finding 1 (TabsContent.Item). I enumerated the rest: aliases resolve to the imported name in bestaxTarget, so importing Tab as X still records Tab; TabList and TabItem (Tabs.List, Tabs.Item) call no useTabsContext, so they correctly do not refuse; the package has no default export, so the empty-owner path at transform.ts:851-855 is unreachable here.
• A same-run conversion creating a reader inside .tabs — refuted: bestaxInside is built only from pre-existing bestax imports, and no ROOTS entry targets any Tabs.* part, so nothing the codemod converts can become a context reader inside a converted .tabs.
• Icon converting inside .control or .field and blocking those — refuted: bestaxInside is computed from pre-existing bestax elements, not from this run of conversions, so .control still converts; on a re-run the .control is already Control and no longer a className candidate.
• .icon inside .control.has-icons-left — refuted: is-left and is-right are not in modifiers, so they stay in className, which Icon merges last, giving an identical class string.
• Class-merge order — refuted for both: the caller className lands last in classNames(root, modifiers, helpers, className), matching what the truth harness asserts.
• Not reached: e2e/bulma-classes-fixtures-render.test.ts, bulma-classes-truth.test.ts and bulma-classes-props.test.ts all load bulma-ui/dist/index.cjs, and this run was told not to build — CI runs them on this head. plan.test.ts passes (57/57). The regenerated bestax-mcp/data/*.json and eslint-plugin/src/generated/metadata.ts are staleness-checked by CI; I did not diff them by hand.

🏄 Clean set of waves here — Tabs and Icon drop in right where the raw markup was standing, and every refusal paddles in the safe direction. Just one little rip current: the readBy guest list forgot TabsContent.Item. Patch that name in and this one is good to ride.

[allxsmith]

On the three advisories from the deep review:

2. Taken, in 33c1f15. It's true of every entry with defaults, not only Icon: the truth sweep gave each default only its own value. Each default the props type takes now converts with another value and renders the same.
3. Left as it is in code. Refusing around any app component inside a .tabs would keep every .tabs whose links are a router's Link as markup, which is most of them in an app with routes, and Field and Control accept the same limit. What was missing was saying so for tabs: eeb135f adds it to the .tabs paragraph in unmappables, with a note to check tabs built from the app's own components after the run.
4. Agreed that it's stricter than Icon needs, and in the safe direction. Staying as it is.

[allxsmith]

deep-review: verify

Check the fix for the two open threads (the same finding, TabsContent.Item missing from readBy) in 52c13c4: the entry in class-map.ts and the truth test claim that holds readBy to every spelling the library exports.

Keep it short: runs here get stopped by the runner at 15 to 20 minutes (#800). Don't run builds or whole suites; CI runs them on this head.

[github-actions]

Preview Deployment

Preview URL: https://43d7150d.bestax.pages.dev

[claude]

Deep review (verify) — 2 resolved · 0 open

No.	Severity	Area	Finding	Location
1	🟡 Minor	Correctness	readBy missed the TabsContent.Item spelling — resolved, entry present at class-map.ts:1751	bestax-migrate/src/sources/bulma-classes/class-map.ts:1751
2	🟡 Minor	Correctness	Duplicate of the row above, raised by the re-run — resolved	bestax-migrate/src/sources/bulma-classes/class-map.ts:1751

Overall: This pass settled threads and reviewed no commits. Both open threads were the same finding, and both are fixed at this head: readBy now lists all four spellings (class-map.ts:1745-1753), so plan() reaches the context refusal and a .tabs wrapping a TabsContent.Item stays markup. The new truth test at e2e/bulma-classes-truth.test.ts:861-902 is the part worth a human eye — it is a closure check rather than an exhaustiveness one, which is the right call here but bounded (see below).

Residual risk:

• Could the fourth spelling silently drop out again? No — refuted by reading the test. withSubComponents uses Object.assign, so TabContentItem, TabsContent.Item and Tabs.Content.Item are the same object; spellings keys on identity, so every still contains TabsContent.Item via the other two entries. Deleting the line leaves readBy a strict subset of every and the toEqual fails — the entry is load-bearing, not decorative.
• Could a different context-reader be missed entirely? Open, and inherent to the design. The test derives every from the components readBy already names, so it catches a missing spelling of a named component but not a missing component. A future reader added to the library with no readBy entry at all still passes. Not a defect in this diff — naming the boundary, not asking for a change.
• Does the depth === 2 walk cap hide a spelling? No — the deepest real part path in the library is three segments (Tabs.Content.Item), which the walk reaches at depth 2 before stopping.

🏄 Two threads, one bug, and the author did not just patch the line — they built a test that makes the line load-bearing. Both threads closed clean, nothing new washed up on the beach. Send it. 🤙

[github-actions]

📸 Story screenshots at handoff — eeb135f

No Storybook stories map to this PR's changed files — nothing to screenshot. (workflow run)

[github-actions]

Preview Deployment

Preview URL: https://1dcb4cdb.bestax.pages.dev

[github-actions]

Preview Deployment

Preview URL: https://fbbb4d50.bestax.pages.dev

[github-actions]

Preview Deployment

Preview URL: https://8969cce2.bestax.pages.dev

[github-actions]

Preview Deployment

Preview URL: https://967a8c01.bestax.pages.dev

[bestax-release-bot]

🎉 This PR is included in version 2.20.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[bestax-release-bot]

🎉 This PR is included in version 5.18.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[bestax-release-bot]

🎉 This PR is included in version 1.12.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[bestax-release-bot]

🎉 This PR is included in version 4.2.13 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀