Repository navigation
feat(bestax-migrate): convert Pagination's page links and ellipsis in their li's place - #819
Conversation
… their li's place Pagination.Link and Pagination.Ellipsis render the bare <li> around the element too, so a .pagination-link or .pagination-ellipsis converts only as the only child of a bare <li>, and takes that <li>'s place, key and all. A new entry field, parent, says so, and the transform reports the element around each one (soleChildOf) to check it. A link writes tabIndex before the props it's given, so it's a default. active also renders aria-current="page", so a new modifier option, needsAttr, makes is-current a prop only beside an aria-current of the element's own; without one it stays a class. is-disabled stays a class. Pagination.Ellipsis renders its own ellipsis and writes a className it's given in place of its class, so two more fields, rendersText and classNameReplaces, have it convert only holding exactly that and carrying no other class. Refs #809
…ent, or its own text A lookup of .pagination-link or .pagination-ellipsis says the component renders the bare <li> around it, to write in that <li>'s place; that the ellipsis renders its own text and takes no other class; and that is-current becomes active only beside an aria-current of the element's own. The index carries parent, rendersText and classNameReplaces, and the agree test gives the planner the <li>, the text and the aria-current the lookup states as conditions.
…es the codemod converts
|
Warning Review limit reachedNext included review available in 1 minute. 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)
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 |
|
deep-review: fresh The core is the |
Preview DeploymentPreview URL: https://08deccf3.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 — 2 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | The only-child refusal is computed from the element's own parent, so it can never fire for a parent entry — a .pagination-link/.pagination-ellipsis whose bare <li> is the only child of a component converts silently, moving any cloneElement-injected props/ref from the <li> onto the <a>/<span> |
bestax-migrate/src/sources/bulma-classes/plan.ts:324 |
| 2 | 🟡 Minor | Coverage | neededFor is threaded only into the fixed-token sweep, so the new active={cond} conversion (is-current under a clsx condition beside aria-current) is decided by plan.test.ts but never rendered and compared against the raw markup |
bestax-migrate/e2e/bulma-classes-truth.test.ts:288 |
| 3 | 🔵 Advisory | Correctness | A key on the <a> alone (the <li> unkeyed) is not refused: it moves from a position React ignores to the array element itself, turning an index-keyed list into a page-keyed one. Markup is identical and the "before" was a React key warning, so this is an improvement rather than a defect — but it is a reconciliation change the attr:key refusal does not cover |
bestax-migrate/src/sources/bulma-classes/plan.ts:338 |
| 4 | 🔵 Advisory | Correctness | needsAttr.value is never read by the planner — only attributes.has(name) is checked — so .is-current beside aria-current="step" also converts. That is sound (the element's own aria-current reaches the <a> through {...rest} and wins over the 'page' the prop writes), but the field is documentation/messaging only, not a guard |
bestax-migrate/src/sources/bulma-classes/class-map.ts:46 |
| 5 | 🔵 Advisory | Robustness | .pagination-link inside a .menu-list is planned twice: the <li> refuses as Menu.Item and leaves a context:Menu.Item TODO, while the <a> converts and deletes that same <li>. The resulting markup is correct; the leftover TODO describes an element that is gone. Contrived markup, no test covers it |
bestax-migrate/src/sources/bulma-classes/transform.ts:1396 |
Overall: The change is sound and unusually well fenced — the new parent / rendersText / classNameReplaces / needsAttr fields each encode a real property of Pagination.Link and Pagination.Ellipsis as written (bulma-ui/src/components/Pagination.tsx:309-347), and I checked each against the component: the <li> is bare, tabIndex and aria-current="page" are written before {...rest} so an element's own value wins, the ellipsis ignores children and lets a given className replace its own, and is-disabled correctly stays a class because disabled also writes aria-disabled and tabIndex="-1". The riskiest part is the new "the component lands in the holder's position" shape, because every existing refusal is phrased about the element — finding 1 is the one place that asymmetry still bites. Look at that first, then at finding 2, since the truth suite is what the table's safety claim rests on.
Residual risk:
- Markup equality for the shipped conversions — refuted empirically.
pnpm exec turbo run test --filter=bestax-migratepasses (2071 tests), includinge2e/bulma-classes-fixtures-render.test.ts, which rendersPagesandPageRefusalsbefore and after and compares normalised HTML — so the<li>, the<li>'skeythroughpages.map,aria-current="page",tabIndex={0}, thehas-text-dangerhelper and the self-closing ellipsis are confirmed identical, not merely reasoned.bestax-mcp(243 tests) and the eslint plugin pass;gen:mcp:check,gen:eslint-meta:check,gen:catalog:check,check:conformance(22/22) andformat:checkare all clean. - Something other than a bare
<li>slipping through — refuted.holderOfyields a holder only whensoleChild(holder) === element, andsoleChildfilters only whitespace React drops, so a comment, a sibling text node, a{expr}wrapper, a fragment or{cond && <a/>}all leavesoleChildOfunset and refuse; a computedclassNameon the<li>givestokens: nulland an empty onetokens: [], both of which failtokens === undefined. The fixture covers the class-on-<li>, sibling-content and no-<li>variants. - The ellipsis converting on text it does not render — refuted.
textOfcomparesjsxTextRendersof the single text child against…, so..., a{expr}, a self-closing span and a text-plus-comment child all refuse;…decodes to the same character, including across the multi-line form. - Class-attribute order differing (
pagination-link is-current my-app→pagination-link my-app is-current) — accepted, not a risk.normalizeHtmlsorts classes by design, and class order in the attribute has no CSS effect. - Open: the two findings above, plus advisories 3–5. Nothing in the MCP lookup or the skills/docs prose reads false to me: the
<li>, ellipsis andaria-currentsentences inbulma-classes.tsandunmappables.mdeach match what the components do.
🏄 Clean little set on this one — the codemod paddles out, grabs the
<li>on the way past, and lands the whole page link in one piece. Just two soft spots in the lineup: nobody's checking who's holding the<li>when a component wraps it, and the truth suite quietly skips theactive={cond}wave. Patch those and it's good to go.
…onent's only child The only-child refusal reads the element's own parent, which for a page link is the <li> it takes the place of, so a <li> that was a component's only child converted: whatever that component handed the <li> through cloneElement would land on the <a>. A new fact, holderOnlyChildOf, carries that component for the element around, and the planner refuses on it. The truth test now renders is-current under a clsx condition too, beside the aria-current it needs, and asserts each such flag was rendered.
|
On the three advisories from the deep review:
|
|
deep-review: verify Check the fixes for the two open threads in 0e72683: the |
Preview DeploymentPreview URL: https://08e18069.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 2 resolved · 0 open
| # | Thread | Verdict | Location |
|---|---|---|---|
| 1 | only-child refusal never reached the <li> the component replaces |
✅ Verified fixed — resolved | bestax-migrate/src/sources/bulma-classes/plan.ts:343 |
| 2 | needsAttr not threaded into the conditional pass, so active={cond} was never rendered |
✅ Verified fixed — resolved | bestax-migrate/e2e/bulma-classes-truth.test.ts:783 |
Thread 1 — holderOnlyChildOf. The fact is set from onlyChildOf(elementPath.parent, bestaxLocals) (transform.ts:1094) inside the same holderOf(elementPath) guard that produces soleChildOf, so it reads the <li>'s own parent — the wrapper this finding was about — and PASS_THROUGH_PARENTS / bestaxLocals keep a fragment or a real Pagination.List from tripping it. The refusal sits inside if (parent) at plan.ts:343, after the bare holder check, so it covers both parent entries in class-map.ts (pagination-link, pagination-ellipsis) and nothing else. The transform test drives it end to end: a .pagination-link in a bare <li> inside a local Tooltip stays markup and reports only-child:Pagination.Link.
Thread 2 — conditional pass. onTag is now defaultsFor(entry, tag) plus neededFor(entry, [token]), so plan() keeps is-current in conditional and renderBoth writes active={when['is-current']} against clsx-shaped raw markup across every CONDITIONS value. The new needing.filter(token => !flagged.has(token)) assertion is non-vacuous by construction: plan.ts:734 admits a needsAttr modifier as a flag only when attributes.has(modifier.needsAttr.name), so dropping the attribute again empties conversion.conditional, leaves flagged without is-current, and fails the assertion.
Overall: This pass settled the two threads the prior deep review left open and reviewed no commits — no new findings were sought or raised. Both fixes are real and empirically checked: the conditional truth suites pass at 503 tests and the whole package is green at 2073 tests across 52 suites, with no fixture churn, so nothing that used to convert stopped. Nothing is left for the human here beyond a normal read of the diff.
Residual risk: none newly identified in this pass — verify mode raises no new findings. Carried over from the first review and unchanged: needsAttr has exactly one entry in class-map.ts (is-current → aria-current), so the second conditional test's "beside each other modifier" pass still has no case to exercise for it; that becomes real only if pagination-link gains a second modifier.
🏄 Both waves got ridden clean, dude — the
<li>keeps its wrapper's vibes and the truth suite actually renders theactiveflag now. 2073 tests green, nothing left bobbing out back. Good to go.
Preview DeploymentPreview URL: https://e2559e9e.bestax.pages.dev |
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
…gination-links # Conflicts: # bestax-mcp/data/skills.json # bestax-migrate/src/sources/bulma-classes/__tests__/plan.test.ts # bestax-migrate/src/sources/bulma-classes/plan.ts
Preview DeploymentPreview URL: https://c493f672.bestax.pages.dev |
Preview DeploymentPreview URL: https://b2f64cc5.bestax.pages.dev |
|
🎉 This PR is included in version 2.22.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.18.1 🎉 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
Refs #809: its first item, Pagination's page links and ellipsis.
Pagination.LinkandPagination.Ellipsisrender the bare<li>around the element too, so each converts in that<li>'s place:How it decides
<li>goes with it. A new entry field,parent, says the target renders a bare<tag>around the element. The element converts only as the only child of a bare one (no class, no attributes but akey, nothing beside it), and takes its place. The<li>'skeymoves onto the component, so a.map()of<li key>s converts. The transform reports the element around each one as a new fact,soleChildOf. This is the reverse of how.selecttakes in its<select>.is-current.activerendersaria-current="page"before the props it's given, so a new modifier option,needsAttr, makesis-currentthe prop only beside anaria-currentof the element's own. Without one it stays a class, which renders the same, and the same holds under aclsxcondition.is-disabledstays a class, sincedisabledalso writesaria-disabledandtabIndex={-1}.tabIndex. A link writestabIndex={0}before the props it's given, so it's adefaultsvalue, as on Previous and Next.Pagination.Ellipsisrenders its own…and writes aclassNameit's given in place of its own class. Two more fields cover that:rendersTextmeans it converts only holding exactly that text (…or the character) and closes itself, andclassNameReplacesmeans it converts only with no other class.The other surfaces
feat(bestax-mcp)): the lookup says the component renders the<li>around the element, what the ellipsis takes, and thearia-currentcondition onis-current. The agree test gives the planner the<li>, the text and thearia-current, and it agrees across the sweep, including a lookup check for the ellipsis that runs after the tag check, as the planner's does.fix(eslint-plugin)): regenerated.is-current, the ellipsis in the prop map's color and no-helper lists, thecontext,childrenanddefaultsexplanations in unmappables in place of the family recipe, and the guide's list.bestax-migrate/CLAUDE.mdnamesparentamong the planner's shapes.Checked
<li>, with each modifier (given what it needs) and each helper, and the fuzz covers them. The props e2e typechecks them,activeincluded.<li>and each way it isn't, akeyon both,is-currentwith and withoutaria-current(and under a condition), and the ellipsis's text and class.paginationfixture adds the links, the ellipsis and each refusal, and renders the same before and after.pnpm allpasses locally.The rest of #809 (the tabs' inner elements,
.icon-text,.file) stays open there.