Repository navigation
feat(bestax-migrate): convert .file inside a Field, building its tree from props - #830
Conversation
… from props
File renders the whole .file tree from props, and a .field of its own
around it outside a Field, so the family stayed markup. It now converts
when its tree is exactly the one File renders and it sits inside a Field:
a bestax one already in the file, or a .field converting in the same run,
which a pass after planning checks.
The <input>'s attributes become File's own, since File puts the ones it
doesn't read there, and its other classes become inputClassName. The
button text becomes buttonLabel (left out when it's File's own default),
each .file-icon's content iconLeft or iconRight, and the .file-name's text
fileName beside hasName. The .file itself carries no attribute but a key,
since that would move to the <input> too. A color stays a class, since
color renders has-text-<color> as well, and so does is-centered beside
isRight. A has-name under a condition stays in the call: File renders
{hasName && ...}, and the truth test caught a 0 condition rendering as
text.
The render truth test wraps the markup in a .field and the component in a
Field for each tree shape; the props test typechecks the <input>
attributes File takes.
Closes #809
…rom props lookup_bulma_classes now tells an agent that File is written from the <input>'s attributes, the button text, the icons and the file name, and that outside a Field it renders a .field of its own. The index carries the table's new buildsFile, and the agree test hands the planner the tree and the .field around it, as the codemod sees them.
The regenerated metadata moves file from the families the codemod leaves as markup to the classes it converts, so no-bulma-component-class points at bestax-migrate for it.
|
Warning Review limit reachedNext included review available in 15 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 (21)
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://93a22a59.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 · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | textValue guards ", & and newlines but not \; recast's StringLiteral printer doubles a backslash, and a JSX attribute string has no escapes — so fileName/inputClassName/buttonLabel can render text the markup didn't |
bestax-migrate/src/sources/bulma-classes/transform.ts:878 |
| 2 | 🔵 Advisory | Performance | readChild now recurses the whole subtree, and childElementsOf(element, readChild) runs once per element, so fact-gathering is O(Σ subtree size) rather than one level deep |
bestax-migrate/src/sources/bulma-classes/transform.ts:1181 |
| 3 | 🔵 Advisory | Coverage | The has-name-under-a-condition path (onlyTrue, the 0-renders-as-text case the PR body cites) has no test pairing a conditional has-name with a .file-name in the tree |
bestax-migrate/src/sources/bulma-classes/__tests__/plan.test.ts:1050 |
Overall: Sound, and unusually well fenced for a whole-tree conversion — the shape check is exact-match on every node, the <input>-attribute hand-off refuses anything File reads as a prop of its own, and the truth test renders markup-in-.field against component-in-Field for each tree shape, which is the only thing that could really prove this. I verified it empirically: pnpm --filter bestax-migrate test is 53 suites / 2123 tests green (the e2e suites need bulma-ui built first), and bestax-mcp and the ESLint plugin are green too. The riskiest part is the two-phase Field gate — the planner sees only classesAround, and the post-planning pass at transform.ts:1367 is what actually decides — so read that pass first; I traced the demotion orders and it is safe (the field entry has no folds, and the file-level block() nulls every conversion together, so there is no way to land a File whose Field went away). The one blocking item is a narrow printing bug in new code that the repo's own jsxStringValue helper already solves.
Residual risk:
- A
Fileconverted outside a runtimeField, rendering a nested.field. Refuted. The post-pass walks syntactic ancestors for a bestaxFieldor a.fieldplanned to become one;Field.tsx:286returns<FieldProvider value={true}>wrapping everything it renders, includingrenderedLabeland thehorizontalField.Bodypath, so any syntacticFieldancestor — children,labelprop,messageprop — really does setuseInsideField(). The other direction (a runtimeFieldthe walk cannot see, e.g.<Field>{renderFile()}</Field>) only refuses, which is the safe side. Confirmed in the fixture: the.filewith no.fieldaround it stays markup withcontext:File. - Index drift between the planner's
parts/childrenand the transform'sinside(...). Refuted.childElementsOfreturnsundefinedunless every non-dropped child is aJSXElement, andbuildFilerefuses onundefinedat each level, so by the timetransform.ts:1553re-derives the same lists with!dropped, the two filters are provably the same set. - Comments lost with the tree. Refuted by reading
moved(transform.ts:1577) against every node it deletes: tags, tag names, all parts' attributes, the<input>'s droppedclassName/type, and the comment children ofelement/label/cta. Comments inside a part that becomes a prop ride along incontentValue's fragment — and a comment child makestextOfreturnundefined, so thebuttonLabel-equals-default omission can never silently swallow one. hasNamerendering a falsy number as text. Refuted:has-namecarriesonlyTrue, so a conditional stays in the joiner andhasNameis only ever written bare. The sibling flags (isBoxed,isFullwidth,isRight) do not need it —Fileconsumes them in aclassNamesobject or anif, never as a child. Open, untested: the conditional-has-name-with-a-.file-namecombination (advisory 3).- Attribute-type drift moving the
<input>'s attributes ontoFile. Refuted for the two props whose types actually differ (size,colorareOmited and are inownProps, giving anattr:refusal, test-covered) and fortype(skipped, hardcoded).numberAttrsOf(File)is justtabIndex, and the truth test feeds it.refcorrectly passes through —Fileis inFORWARDS_REFandref-roster.test.tsholds that to the real component. textValue's other escape hazards beyond finding 1.<,>,{,}are literal inside a JSX attribute string and need no guard;'never triggers an escape because recast's"auto"quote picks the shorter rendering, which is always the double-quoted one here. Control characters cannot reach it —jsxTextRendersmaps tabs to spaces and the regex already rejects\r\n. So\is the whole gap.- Generated-artefact staleness (MCP index, ESLint metadata, skill rosters). Not re-run locally: the
gen:*:checkscripts need approval in this sandbox. CI's own steps cover it; thebuildsFilefield is present in all three outputs and bestax-mcp's agree test passes against the committed index.
🏄 Whole gnarly
.filetree collapsing into one clean<File />— that's a proper barrel, dude, and the truth test riding markup and component side by side is exactly the wax job this deserved. Just one grain of sand on the board: a backslash slips pasttextValueand comes out doubled. Sand that off and paddle out. 🤙
A JSX attribute string has no escapes, and recast prints a string literal with its backslash doubled, so a fileName, inputClassName or buttonLabel holding one rendered two. It now goes in as an expression, as a quote or an & already did. Each element's tree is read once now: every element around it read it again, which made gathering the facts grow with the depth of the tree. A plan that changes after it's been read forgets the reading. And a test pins that a has-name under a condition doesn't count for a .file-name.
|
On the advisories: both are done in 212831e. Each element's tree is now read once and cached, and a plan that changes after it's read forgets the reading. A planner test now pins that a |
|
deep-review: fresh 212831e answers the first review: the backslash fix on its thread, plus both advisories, a cache so each element's tree is read once and a test for a conditional has-name. The cache is new code this review hasn't seen. This review is of the whole PR as it now stands. |
Preview DeploymentPreview URL: https://f865cd58.bestax.pages.dev |
|
deep-review: verify the open thread 212831e fixes the backslash thread and adds the two advisories' changes (a reading cache and a conditional has-name test). Three fresh runs of this PR were cut short by the runner before posting, and the first full review already covered the rest of the diff. |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | textValue let a backslash through and the printer doubled it — verified fixed, resolved |
bestax-migrate/src/sources/bulma-classes/transform.ts:880 |
What I checked
textValuenow reads/^[^"&\\\r\n]*$/, so a text holding a backslash takes the
jsxExpressionContainerbranch — the same branch"and&already took. All three
call sites share the helper:buttonLabel(:913),inputClassName(:1575),
fileName(:1582).- The regression test at
__tests__/transform.test.ts:1312feedsC:\Uploadsas a
.file-labeland asserts the printedbuttonLabel={"C:\\Uploads"}. It passes. With the
old character class that text matched, so it printed as a JSX attribute string and the
assertion could not hold — the test is a real guard, not a restatement. - Keeping the local helper rather than
_shared/jsx-utils.ts'sjsxStringValueis right:
the local one also guards&, whichjsxStringValuedoes not. - Suite state: the
bulma-classesunit set is green, 269 passed. The one red suite,
__tests__/value-unions.test.ts, fails onCannot find module '@allxsmith/bestax-bulma/constants'— an unbuilt sibling in this checkout, unrelated to
the diff.
Overall: This pass settled threads and reviewed no commits. The backslash fix is
correct at the single choke point every affected prop flows through, and it ships with a
test that fails against the old guard. Nothing is left open from the first review; the two
advisory-driven changes in 212831e2 (the reading cache and the conditional has-name test)
were not re-reviewed here — a fresh run is what covers those.
Residual risk: Out of scope for a verify pass — this pass judged only the single open
thread, not the failure class or the rest of the diff.
🏄 Paddled back out for just the one wave and it closed out clean — backslash bug's
handled at the one spot all three props funnel through, with a test that actually wipes
out on the old code. All threads cleared, nothing left bobbing in the lineup.
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| No. | Severity | Area | Finding | Location | Status |
|---|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | textValue let a backslash through and the printer doubled it | bestax-migrate/src/sources/bulma-classes/transform.ts:880 | ✅ Resolved |
Overall: This pass settled threads and reviewed no commits. The single open thread was already answered by 212831e, and had been replied to and resolved at this same SHA by the previous verify run; this pass re-confirmed the fix independently rather than taking that on trust, and found nothing still wrong. Nothing is left open, so a human can go straight to the change itself.
What I checked on the one thread
-
Guard: the negated class in textValue now covers a backslash as well as the quote, the ampersand and the line breaks (transform.ts:880), so a text holding a backslash falls to the jsxExpressionContainer branch. Keeping the local helper over the shared jsxStringValue is the right call: it also guards the ampersand, which jsxStringValue does not, and an attribute holding an entity would otherwise be re-read as the character it names.
-
Blast radius: all three consumers share the one helper — buttonLabel (913), inputClassName (1575), fileName (1582) — so the fix covers every site the finding named.
-
Printer behaviour, exercised directly against the repo copy of jscodeshift/recast rather than reasoned about:
bare literal : <File buttonLabel="C:\\Uploads" /> old branch: a JSX attribute string has no escapes, so this renders TWO backslashes expr container : <File buttonLabel={"C:\\Uploads"} /> new branch: a JS string, so ONE backslash -
The regression test bites, and is not tautological: the new case at transform.test.ts:1312 feeds a Windows path as a .file-label and asserts the printed buttonLabel takes the braced, JS-string form. Under the old class, which had no backslash in it, that text matches, so it prints as the bare attribute form above, which lacks the brace the assertion requires. The test could not pass before the fix.
-
Suite: the bulma-classes transform suite is green, 151 passed. The one red suite in the wider package run is the render harness failing to resolve an unbuilt bestax-bulma dist, which is pre-existing and unrelated.
Residual risk: none identified for the addressed class.
- The sibling emitter textChild (transform.ts:867) needs no matching change: it emits j.jsxText, which recast prints raw, and JSX text has no backslash escapes, as the original finding said, and that code is unchanged.
- The ampersand case that the local helper is kept for lives in the same character class, so hardening for the backslash did not trade away the entity-decoding guard.
🏄 Paddled back out to check the one wave that was still breaking, and yeah, that backslash fix is clean, the test behind it actually bites, and the lineup is empty. All green, go surf it.
allxsmith
added
the
needs-human-review
📸 Story screenshots at handoff —
|
|
🎉 This PR is included in version 5.18.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.24.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.13.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 #809. This is its last item,
.file..pagination-link/.pagination-ellipsislanded in #819 and.icon-textin #823. Theul/liinside.tabsis dropped, as I proposed on the issue: the only part that could match a bare<li>there is the deprecatedTabs.Item, and converting to it would just hand people another migration.Filerenders the whole.filetree from props, and a.fieldof its own around it outside aField, so until now the codemod left.fileas markup. It now converts when the tree is exactly the oneFilerenders and it sits inside aField:Fieldaround it is a bestax one already in the file, or a.fieldthat converts in the same run. The planner can't see the second case, since children are planned first, so a pass after planning checks it. When the.fieldstays markup, the.filestays too, with acontext:FileTODO.File, which puts the ones it doesn't read back on the<input>. Its other classes becomeinputClassName. An attributeFilereads as a prop of its own (size,color,label, a helper name) refuses.buttonLabel, left out when it'sFile's own defaultChoose a file…. Each.file-icon's content becomesiconLeftoriconRight, and the.file-name's text becomesfileNamebesidehasName. Mixed content is written as a fragment, with each text as the string it renders.colorrendershas-text-<color>too, andis-centered, which renders nothing besideisRight. Ahas-nameunder a condition also stays in the joiner call.Filerenders{hasName && fileName && …}, and the truth test caught a0condition rendering as text..filebutkey, sinceFilewould move it to the<input>;Tests:
.fieldagainst the component inside aField: icons, default text, input attributes and classes, a name, and the modifiers together. Swapping the icons in the planner on purpose fails it.Fileis given..fieldwith a.filein the kitchen sink.classNameattributes being lost. They now move to the name that stays.Also in this PR:
lookup_bulma_classesdescribes whatFileis written from.no-bulma-component-classnow reports.fileas a class the codemod converts.pnpm allpasses locally.