Repository navigation
feat(bestax-migrate): convert an .image around anything but a bare <img> - #804
Conversation
An .image converted only together with a bare <img>, so an <iframe class="has-ratio"> or an <img> with loading or width kept both as markup. Image renders any children it's given as they are, in place of its own <img>, so the absorb spec gains elseWraps: when the child can't be absorbed, the element converts around its children instead. The bare-<img> form still comes first, and an empty .image still refuses, with a message saying Image would render an <img> of its own. Closes #801
…img> lookup_bulma_classes now adds that Image renders other children as given, so they go inside it as they are.
|
Warning Review limit reachedNext included review available in 30 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 selected for processing (13)
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 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:
|
Preview DeploymentPreview URL: https://bc365815.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 · 1 advisory
| No | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟠 Major | Correctness | elseWraps converts an .image whose sole rendered child is a JSX expression; Image picks its content with a truthiness test on children, so a falsy expression makes it render an <img> of its own where the <figure> rendered nothing |
bestax-migrate/src/sources/bulma-classes/plan.ts:293 |
| 2 | 🔵 Advisory | Coverage | The truth test elseWraps branch renders only truthy children, and its two class-shaped refusal rows (empty child class, computed child class) render the child with no className in both halves, since renderBoth sets one only when sole.tokens?.length — so those rows prove the container, not the child |
bestax-migrate/e2e/bulma-classes-truth.test.ts:628 |
Overall: The mechanism is the right one, and the container half is genuinely equal: Image renders image + is-<size> + has-ratio (ratio sizes only) + helpers + className, the ratio tokens stay in omits so they land in className, and the wrap path leaves the child JSX completely untouched — which is why dropping the <img> from the spread, attr and dynamic-class unmappables is correct rather than optimistic. The riskiest part is the one fact the planner reads statically: hasChildren means "there is JSX inside", while Image decides with children ? … : <img/>, and the two disagree for exactly the single-expression child that formatted JSX produces. That is finding 1, and where the human should look first.
Residual risk:
- A discarded refusal leaking a TODO — refuted:
refusebuilds a new array (plan.ts:256) and never mutates the outer one, so throwing theabsorboutcome away leaves no phantom entry; the new plan test pins the todos to exactlychildren:Imagefor the empty case. - The child losing something in the wrap path — refuted:
absorbedstaysundefined, so noabsorbs.propsare emitted and the codemod rewrites only the element. A childloading,width,keyorref, a child spread, an empty or computed childclassName, andis-roundedon the child all survive verbatim, which is what makes the unmappables edits accurate. - Container class drift — refuted: the new
plan.test.tscase pins the leftoverclassNametois-16by9for a ratio, andImageaddshas-ratioonly from asizecontainingby, so a ratio kept as a class rendersimage is-16by9exactly. - Empty
.image— refuted:hasChildren: falserefuses withchildren:Image, asserted in bothplan.test.tsand the newelseWrapstail of the truth test. - Open: the empty-element gate now sits ahead of the
dangerouslySetInnerHTMLgate, so an.imagecarryingdangerouslySetInnerHTMLand no JSX children refuses with thechildren:Imagemessage about an empty element rather than theattr:dangerouslySetInnerHTMLone. It still refuses, so no markup is at risk — only which rule and wording the report shows. - Open (not reached): I did not run the suites or the
bulma-classescorpus script on this head (runner time cap). CI covers the suites; the corpus is deliberately out of CI, so a breadth pass over real.imagemarkup is still worth doing before release.
🏄 Clean little swell, this one —
Imagealready rides whatever children you hand it, so letting the.imagewrap instead of bail is the right line. Just watch that one rip current: a falsy expression child, andImagepaddles out an empty<img>of its own. Patch that and it is all glass.
…they're elements
Image tests its children for truth, so an .image around an expression
({src && <img />}) that came out falsy would render an <img> of its own
where the markup rendered nothing. The fallback now converts only around
HTML elements written out, and refuses anything else with a
children:Image TODO.
The truth test also renders a child's empty or computed className in both
halves, so those rows test the child as well as the container.
…not an expression
|
deep-review: verify Check the fix for the open thread (an expression child) in 570c6fd: the new condition on the Keep it short: runs here get stopped by the runner at 15 to 20 minutes (#800), so read just that commit and don't run builds or whole suites (CI runs them on this head). |
Preview DeploymentPreview URL: https://2544f9d7.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| # | Severity | Area | Thread | Location | Outcome |
|---|---|---|---|---|---|
| 1 | 🟠 Major | Correctness | elseWraps converted an .image whose sole child is an expression, and Image renders its own <img> when that expression is falsy |
bestax-migrate/src/sources/bulma-classes/plan.ts:293 |
✅ Verified fixed — resolved |
Overall: This pass settled the one open thread from the earlier review and reviewed no commits. The fallback in plan() now converts only when the children are HTML elements written out — soleChild, or a non-empty childElements — both of which make props.children a React element or an array, so Image can never fall through to rendering its own <img>; anything else refuses with a children:Image TODO. Nothing here needs a human beyond the usual squash-merge read; CI owns the builds and the e2e truth suite.
Residual risk:
- A fact path that still admits a falsy child — refuted:
transform.tsis the only producer of these facts, andsoleChild(872) requires a single non-droppedJSXElementwhilechildElementsOf(712) bails toundefinedon any non-JSXElementor non-intrinsic child, so no expression, text, or component child reaches the convert path. - Regression in what still converts — refuted empirically:
plan.test.ts58/58 andtransform.test.ts130/130 pass on this head, theabsorbedfixture pinning the new refusal alongside the existing.imageconversions. - Over-refusal — open, and harmless: an
.imageholding an element plus a comment sibling, or one wrapping only text, now refuses where it would have converted safely. That direction only leaves markup as markup with a TODO. - The e2e truth suite needs
bulma-ui/dist, so its new plan-level expression assertion was read rather than run; CI executes it on this head.
🏄 Clean little set on this one, dude — the codemod stopped dropping a phantom
<img>into the falsy branch and just leaves the markup chillin where it cannot read the wave. Only thread paddled in, good to go.
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
|
🎉 This PR is included in version 2.16.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.18.0 🎉 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
Closes #801. An
.imageconverted only together with a bare<img>(#797), so these kept both as markup:How it decides
Imagerenders any children it's given as they are, in place of its own<img>. So the absorb spec gainselseWraps: when the child can't be absorbed, the planner converts the element around its children like any other element, and they stay as written.<img>form still comes first, since it gives<Image src alt />..imagestill refuses, now with achildren:Imagemessage sayingImagewould render an<img>of its own there.The other surfaces
feat(bestax-mcp)): the lookup adds that other children go insideImageas they are..imagebullet and the migration guide say so. The unmappables drop the<img>from thespread,attranddynamic-classcases it no longer blocks, and say an.imagegetschildren:Imageonly when it's empty.Checked
elseWrapsentry to convert around every child it can't absorb, rendering the same markup, instead of refusing, and still to refuse with nothing inside. Its render helper gives a non-absorbing conversion the raw child as its children.absorbedfixture adds an<iframe>and an empty.image, and its<img loading>case now converts. All render the same before and after.pnpm allpasses locally.