Repository navigation
fix(bulma-ui): keep a hasName File's CTA corners rounded while there is no name - #990
Conversation
…is no name With hasName and nothing to show yet (before a pick, or an empty fileName), File rendered .file.has-name with no .file-name. Bulma squares the CTA's inner corners under has-name to meet the name area, so the button sat alone with two flat corners. - The root takes Bulma's is-empty while hasName has no name to show, which rounds the CTA again in the default and isRight layouts. The markup with a name is unchanged. - Bulma's is-boxed rules come after its is-empty rule and square a boxed CTA's bottom corners again, so _file.scss gives them back with a selector that outranks Bulma's in either order. - The conformance check's rule that a partial registers its own variables asked _file.scss to register --bulma-file-radius, which Bulma registers itself. It now skips a key Bulma registers, and is a pure function held by fixture tests.
…is-empty File now renders Bulma's is-empty on a has-name .file with no .file-name, to keep its button's corners rounded, so the bulma-classes source moves with it and every conversion still renders the markup it replaces. - A has-name .file with no .file-name converts when is-empty is written on it, and the class goes with the rest of the tree, since File renders it there itself. - Without is-empty it stays markup with a defaults:File TODO that names the class to add. A condition on is-empty can't stand in, since File renders it whatever the condition says. - Beside a .file-name, is-empty stays in className, since File renders none there. - The skill's component map and its defaults recipe say so. The truth tests give a has-name .file a name unless a case is about having none, and so does the MCP agreement test, which runs this planner beside lookup_bulma_classes.
…no name lookup_bulma_classes says how a .file's tree becomes File's props. It now adds that with hasName and no .file-name, File renders Bulma's is-empty itself, so that class isn't needed there.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughFile now applies Bulma’s ChangesFile empty-name behavior
SCSS variable conformance
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The empty-name behavior and matching conversion appear ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 16 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
bestax-mcp/src/__tests__/server.test.tsParsing error: "parserOptions.project" has been provided for 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://1cd8f60f.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
deep-review: fresh The last run on 3186d9a was cancelled at the runner limit after it posted one thread (on |
…und corners The hasName TSDoc, the docs page and the File story said Bulma's is-empty keeps the CTA's corners rounded. It does in the default and isRight layouts, but a boxed File gets its bottom corners back from _file.scss, which ships only in the bestax extras, so on Bulma's CSS alone they stay flat. The clause is gone from each, and from the codemod's defaults:File TODO and the two migrate skill references that repeated it, where a bulma-classes app usually has Bulma's CSS alone.
|
deep-review: fresh Runs on 3186d9a were cancelled at the runner limit before posting a summary; between them they posted one thread (on |
|
deep-review: fresh Runs on 3186d9a were cancelled at the runner limit before posting a summary; between them they posted two threads (on |
…t kept as a class lookup_bulma_classes said in its File note that is-empty isn't needed beside hasName with no .file-name, while the is-empty row in the same answer said the class stays in className, which is what the codemod's planner no longer does there. The row now reads rendered by File, with no .file-name in the tree, and says that beside one it stays in className. The agreement test had stopped seeing the case, since it gave every has-name .file a name. It now gives one the tree the lookup's answer describes, with no name when is-empty marks the slot empty, and checks that case directly.
There was a problem hiding this comment.
Deep review — 1 blocking · 1 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | lookup_bulma_classes' is-empty row still says "stays in className" where the planner absorbs it, and the agreement test no longer reaches the nameless tree |
bestax-mcp/src/bulma-classes.ts:839 |
| 2 | 🔵 Advisory | Correctness | the hasName prose credits the rounding to Bulma's is-empty; in the isBoxed layout it comes from _file.scss, which ships only in the bestax extras |
bulma-ui/src/form/File.tsx:68 |
Both rows are the threads the two cancelled runs on this commit managed to post before the runner limit; I re-checked each against the current code and both still stand (leftoverReason → passthroughReason at bulma-classes.ts:255 still feeds the generic row rendered at :907, and plan.ts:775 still puts is-empty in converted). This pass raised nothing new.
Overall: The change is sound, and the hard part — the cascade — is right. I read Bulma's sass/form/file.scss directly: .file.has-name.is-empty .file-cta (0,4,0) outranks .file.is-right .file-cta (0,3,0), so the default and isRight layouts round from Bulma alone; .file.is-boxed.has-name .file-cta ties at (0,4,0) and comes later, which is exactly what the new (0,5,0) extras selector is for, and it wins in either sheet order and in isBoxed+isRight. 'is-empty': hasName && !shownName is the same predicate that gates the .file-name render, so the class and the element cannot disagree. The riskiest piece is the bespoke cascade resolver in File.styles.test.tsx — it stands in for a browser, so it is worth knowing what it does and does not model (below); I confirmed empirically that it is not vacuous, since squares the corners beside a name only passes if jsdom enumerates and ranks Bulma's logical radius longhands, which it does. Focus first on row 1: it is the one place where two surfaces of the shipped product now give an agent opposite instructions for the same input.
Notes from the areas the steer asked me to carry on with, all of which came back clean:
- Conformance rule.
unregisteredVarViolationsis a faithful extraction — same comment stripping, same${rel}:${i + 1}anchoring, only the violation ordering within a partial changes.pnpm exec node scripts/check-conformance.mjs --only=scss-conformancepasses, which is itself the proof thatbulmaKeys()findsfile-radius(_file.scssregisters nothing, so only the Bulma set can be exempting it).bulmais a root devDependency, so thecreateRequireresolve is not phantom. The new exemption cannot change any existing partial's verdict —!registered.has(key)is evaluated first — and because the gate is alreadykey.startsWith(partialName + '-'), a cross-namespace Bulma key can never exempt anything. All 11 cases inscripts/scss-conformance.test.mjspass. - bestax-migrate. The four tree/token combinations all round-trip:
has-name+is-empty+no name absorbs the class (className: null);has-name+is-empty+.file-namekeeps it (className: 'is-empty', and the class order happens to match because the component appendsclassNamelast);has-name+no name+nois-emptyrefuses withdefaults:File; a conditionalis-emptyrefuses too. I specifically chased the case the guard cannot see — a conditionalhas-name— and it is safe by accident of the map rather than by the guard:'has-name'carriesonlyTrue: true(class-map.ts:1715), soflagis false in the conditional loop, the class stays in the joiner,writes.has('hasName')is false, and markup and component both render.file.has-namewith nois-empty. The e2e conditional sweep atbulma-classes-truth.test.ts:917exercises exactly that against rendered HTML. - Fixtures and TODOs.
addTododedupes on comment text, not on the rule key, so the newdefaults:Filemessage cannot swallow or be swallowed by anotherdefaults:Fileone. The refusal is idempotent: addis-empty, re-run, it converts; leave it, re-run, same TODO. - Skill docs. The
component-map.mdworked example at:298still carries a.file-name, so it converts as written and the new paragraph does not contradict it.prop-map.md's.filerow is a class→prop table andis-emptyhas no prop, so leaving it out is right.
Residual risk:
- A Bulma radius rule moving under an at-rule.
cornersOfwalks only top-levelCSSStyleRules and ignores!important, so a.file-ctaradius declaration inside@media/@supports/@layerwould be invisible and the test would stay green while the bug returned. Refuted for today's Bulma: I scannedbulma/css/bulma.cssfor at-rule blocks containing a.file … radiusdeclaration (0 found) and for!importanton one (none). The comment above the helper already scopes itself to "the top-level rules ofcss". - Other layouts squaring the CTA. Refuted by reading
sass/form/file.scssend to end: the only radius declarations touching.file-ctaare thehas-name,has-name.is-empty,is-boxed.has-nameandis-rightrules plus the (0,1,0).file-cta, .file-namebase.is-small/medium/large,is-fullwidth,is-centeredand the colour modifiers set no radius, so no untested modifier combination can re-square it. - RTL and vertical writing modes. Refuted: the new declaration is the physical
border-radiusshorthand with a single value, so all four corners get the same radius whatever the inline direction — unlike Bulma's logical longhands, it has no side to get wrong. - A consumer on stock Bulma CSS with no bestax extras. Open, and inherent to the fix: the boxed layout's rounding lives in
_file.scss, which ships only inextras.css/bestax.css. Row 2 is the documentation half of this; there is no code change that would close it without duplicating the rule into Bulma. className="is-empty"beside a real name. Refuted as a hazard: Bulma's ownis-emptyblock sets.file-name { display: none }, so in that state no name area is visible and rounding all four CTA corners is the correct drawing — the extras rule is more right there than Bulma's cascade, not less.
🏄 Tiny class, gnarly cascade — but they paddled out, read the whole set in Bulma's sheet, and landed a specificity bump that holds in either stylesheet order and in a prefixed build. One stale sentence and one MCP row still telling agents the opposite of what the codemod does; patch those two and this one is clean all the way to the beach.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on e53ee6c. |
Preview DeploymentPreview URL: https://a7ebf48a.bestax.pages.dev |
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on df6f32b. |
Preview DeploymentPreview URL: https://917fafd8.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 2 resolved · 0 open
| ID | Severity | Area | Thread | Outcome |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | defaults:File TODO read as already satisfied when is-empty was conditional — bestax-migrate/src/sources/bulma-classes/plan.ts:805 |
✅ Verified fixed (53306087) |
| 2 | 🔵 Advisory | Robustness | Rule 4 exemption set came from an untested reader — scripts/check-conformance.mjs:631 |
✅ Verified fixed (3d82419d) |
Overall: This pass settled threads and reviewed no commits. Both open advisories are genuinely closed rather than reworded away: the defaults:File refusal now states the condition the guard actually checks, and plan.test.ts:1202-1211 plans the conditional shape and asserts that wording, so the gap can fail again; bulmaKeys() is exported and held to the real Bulma sass/ tree, with a negative (tabs-vertical-min-width) that the namespace gate cannot cover — a sharper choice than the collapse-duration I had suggested. Nothing is left for a human on my threads.
Residual risk:
fileName=""drifting out of the two prose surfaces that teach it — refuted for now: the lookup note (bestax-mcp/src/bulma-classes.ts:856-859) is pinned byserver.test.ts, the CLI TODO byplan.test.ts:1199and__testfixtures__/file.output.tsx:5; the skill prose is pinned to neither, which is the standing gap the earlier thread already recorded.- Rule 4 failing open on a wider
registerVarsEntriesread — refuted: the new case walks the real Bulma tree and pins both directions (file-radiusin,tabs-vertical-min-widthout), so a wider read now redspnpm test:scripts. - Stale generated index — refuted: the byte counts
skills.jsonrecords for the two edited reference files (23915 / 24617) match the files exactly. - Verification note:
pnpm --filter bestax-migrate run test -- src/sources/bulma-classesis 398/398 green andscripts/scss-conformance.test.mjsis 12/12.pnpm run test:scriptshas one failure in this sandbox —eslint-plugin/dist is absent; run its build first(scripts/constants-subpath.test.mjs:272) — an unbuilt sibling package, not this change.
🏄 Paddled back out just to close the last two sets, and both were already clean — the author even picked a gnarlier test key than I asked for. Nothing left bobbing out here, this one is good to go.
There was a problem hiding this comment.
Deep review — 0 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | bulmaKeys() assigns its cache before filling it, so a concurrent caller reads a half-built set (unreachable today, fails loud) |
scripts/check-conformance.mjs:637 |
| 2 | 🔵 Advisory | Coverage | the two skill references are the only surfaces teaching the is-empty / fileName="" recipe that no test pins to the planner |
skills/bestax-migrate/references/bulma-classes/component-map.md:324 |
Overall: Sound, and the hard part — the cascade — is right. I read Bulma 1.0.4's sass/form/file.scss and the compiled bulma.css directly: the only rules that set a radius on .file-cta are the base %control/.file-cta pair (0,1,0), .file.has-name (0,3,0, bulma.css:6033), .file.has-name.is-empty (0,4,0, :6041), .file.is-boxed.has-name (0,4,0, :6074) and .file.is-right (0,3,0, :6100) — so the default and isRight layouts round from Bulma alone, while .file.is-boxed.has-name ties at (0,4,0) and comes later, which is exactly what the new (0,5,0) extras selector is for, and it wins in either sheet order. 'is-empty': hasName && !shownName is the same predicate that gates the .file-name render, so the class and the element cannot disagree, and the shorthand border-radius in _file.scss sets all four corners, so nothing new is direction-sensitive. The riskiest piece is the bespoke cascade resolver in File.styles.test.tsx, but it fails loud rather than vacuous (below) and I confirmed it is live. Neither advisory blocks: the first is latent and noisy-not-quiet, the second is a drift guard on prose that is accurate today.
Empirical pass, all on df6f32b6:
| What | Result |
|---|---|
jest src/form/__tests__/File.test.tsx File.styles.test.tsx |
79 pass |
pnpm --filter bestax-migrate run test (whole package) |
54 suites, 2309 pass |
jest e2e/bulma-classes-truth.test.ts e2e/bulma-classes-props.test.ts |
526 pass |
pnpm --filter bestax-mcp run test |
7 suites, 316 pass |
node --test scripts/scss-conformance.test.mjs |
12 pass |
node scripts/check-conformance.mjs --only=scss-conformance |
✓ |
| build, then grep the flavors | the rule ships in dist/extras.css and dist/versions/bestax-prefixed.css |
Residual risk:
- A layout combination the styles test misses — refuted.
is-centered, the ten colors and the three sizes set no radius anywhere in Bulma's sheet (and--bulma-file-radiusisrem-based, so afont-sizemodifier cannot scale it either), sohas-name×is-boxed×is-rightis the whole space andLAYOUTScovers all four cells. - A conditional
has-namemakingFileadd anis-emptythe markup never had — refuted.class-map.ts:1731markshas-nameonlyTrue, andplan.ts's conditional loop requires!modifier.onlyTruebefore a condition becomes a prop, so a conditionalhas-namestays in theclassNamejoiner andFile's ownhasNameisfalse— the converted element renders nois-emptyeither. The mirror case (statichas-name, conditionalis-empty) refuses, pinned atplan.test.ts:1199-1203. - A static
is-emptybeside a.file-name— refuted.file.fileNameis defined, so the new block is skipped andis-emptystays inclassName(plan.test.ts:1213-1216); the raw markup carried the same class over the same Bulma-hidden.file-name, andnormalizeHtmlsorts class tokens, so the truth test'swith a name, and is-empty beside itcase really does compare the two. - The cascade resolver going vacuous — refuted. It reads only top-level
CSSStyleRules, so a radius rule moved into an@media/@supports/@layerblock leaves that corner absent from the resolved map and bothtoEqual(ALL_ROUND)andtoBe('0')fail rather than pass. It is also live today:squares the corners beside a namepasses only because jsdom enumerates and ranks Bulma's logical longhands (border-start-end-radius,border-end-end-radius). - Rule 4's exemption quietly widening — refuted. I checked every
<partialName>-*key consumed viacv.getVarinscss/{components,elements,form}against Bulma'sregister-varskeys:file-radiusis the only collision, andtabs-*,checkbox-*,radio-*,steps-*,toast-*andtooltip-*have no Bulma counterpart at all. A future Bulma key in one of those namespaces would also register it, so the exemption stays semantically correct, and the new negative assertion reds the suite if Bulma ever registerstabs-vertical-min-width. - Boxed rounding needs the bestax stylesheet — open and accepted; the over-claim that said otherwise was settled in an earlier pass. Only
isBoxedis fixed from_file.scss, so a consumer on plainbulma.csswith nobestax.css/extras.cssstill sees the two squared bottom corners. The docs previews are safe:docs/src/components/ShadowPreview/shadowStyles.js:19,25adoptsextras.cssbesidebulma.min.css, soBoxed with NameandSizes with Boxed and Namedo get the rule. --bulma-file-radiusunresolvable — open, exotic, not posted as a finding. This is the first rule in_file.scsswhose unresolvedvar()makes things worse than no rule: aborder-radiusinvalid at computed-value time falls back to0, squaring all four corners instead of two. It needs an app that compiles Bulma with a non-default$cssvars-prefixand links the prebuiltextras.cssrather than buildingsrc/scss.- Unmeasured gates — this sandbox denied
pnpm test:scripts, the threegen:*:checkstaleness checks, both typechecks andformat:check, so treat those as no signal rather than a pass. By hand I confirmed that only two files carry thehasNamesentence (bestax-mcp/data/components/File.jsonanddocs/docs/api/form/file.md) and both were regenerated with the TSDoc's wording, and thatskills.json's byte counts match the two edited references exactly (24617 / 23915).
🏄 Clean barrel, dude — all four corners rounding the way Bulma meant, one tiny extras rule holding the boxed line, and a hand-rolled cascade resolver that actually paddles out instead of posing. Nothing blocking in the lineup; both advisories are driftwood on the beach. Good to go.
bulmaKeys() assigned its cache to an empty set before the walk over Bulma's sass tree filled it, so a caller arriving during one of the awaits got a partial set, and a read that threw left one cached for good. Nothing calls it that way today, but it is exported now. The cache is now the promise of the finished set, through a small onceAsync helper: a caller mid-read waits for the whole set, and a read that throws is forgotten so the next call reads it again. The scss conformance test pins both halves on the helper, and that bulmaKeys hands a later caller the same set.
…eferences to the planner component-map.md and unmappables.md both teach what a has-name .file with no .file-name needs to convert: is-empty written as a static class, a condition on it not standing in, and fileName="" on the File it becomes. The planner's own TODO, the fixture and the MCP lookup were already pinned to that rule, but no test read either paragraph, so a change to the guard in plan.ts would have left both teaching the old one. component-map-doc.test.ts now runs the planner on the three cases, the class written, missing and conditional, and checks each claim in both paragraphs against what it decides. The unmappables paragraph is looked up under the section the refusal's rule names, which is where the TODO sends an agent.
…empty # Conflicts: # docs/docs/api/form/file.md
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 111d9e0. |
Preview DeploymentPreview URL: https://c3bb0b20.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 2 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | bulmaKeys() published its cache before it was filled — verified fixed, resolved |
scripts/check-conformance.mjs:645 |
| 2 | 🔵 Advisory | Coverage | The two skill references teaching the is-empty recipe were pinned by nothing — verified fixed, resolved |
skills/bestax-migrate/references/bulma-classes/component-map.md:324 |
Overall: This pass settled open threads and reviewed no commits. Both remaining advisories had author replies claiming fixes, and both hold up: the cache is now a memoized promise (onceAsync, with a rejected build dropped so a failed read does not stick), and both skill paragraphs are held to the planner's own decisions rather than to literals. Nothing of mine is left open — a maintainer wanting the current code reviewed should ask for a fresh pass.
Residual risk:
- Cache publication — refuted.
onceAsyncatcheck-conformance.mjs:631assigns the promise, not theSet, so a caller arriving mid-walkawaits the first build;scss-conformance.test.mjs:191proves it by parking a build on a gate and asserting the second caller has not settled, and:219proves a throwing build is forgotten and rebuilt. Both pass. - Prose drift in the two references — refuted.
component-map-doc.test.ts:107-223derives the rule string and the unmappables heading fromplan's own output, so a change to theplan.tsguard moves the lookup and fails the test rather than leaving the paragraphs teaching the old recipe; theparagraph()helper returns the empty string on a missing heading, so there is no silent-pass shape. - Not re-examined by design: everything outside these two threads. A verify pass reads no commits.
- For the record:
pnpm test:scriptsalso reports failures readingeslint-plugin/dist is absent— a build-order artefact of this sandbox, unrelated to this PR. The bestax-migrate suite ran green (54 suites, 2311 tests).
🏄 Both leftover ripples got cleaned up proper, dude — the cache now waits for the whole set, and the skill docs are leashed to the planner instead of floating free. Zero threads open, this one's good to paddle in.
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.15 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
With
hasNameand nothing to show yet (before a pick, orfileName=""),Filerendered.file.has-namewith no.file-name. Underhas-name, Bulma squares the button's inner corners to meet the name area, so the button sat alone with two flat corners. Since #963 that is the first thing thehasNamedocs example and the picked-file stories show.In bulma-ui, the root now takes Bulma's
is-emptywhilehasNamehas no name to show. Bulma's ownis-emptyrule rounds the button again in the default andisRightlayouts. Itsis-boxedrules come later in its sheet and square a boxed button's bottom corners again, so_file.scssgives them back with a selector that outranks Bulma's whichever order the two sheets load in. Markup with a name is unchanged. jsdom computes none of this cascade, so a styles test works out each corner's radius across Bulma's sheet and the partial, in both orders and in a prefixed build. ANoNameYetstory shows the layouts side by side.The conformance rule that a partial registers the variables in its own namespace asked
_file.scssto register--bulma-file-radius, which Bulma registers itself. The rule now skips a key Bulma registers, and it is a pure function with fixture tests for both cases.In bestax-migrate, the bulma-classes source converts a
.fileonly whenFilerenders the same markup, so it moves with the component. Ahas-name.filewith no.file-nameconverts whenis-emptyis written on it, and the class is absorbed into the component. Without it, the.filestays markup with adefaults:FileTODO naming the class to add. A condition onis-emptycan't stand in, sinceFilerenders it whatever the condition says. Next to a.file-name,is-emptystays a class, as before. The skill's component map and itsdefaultsrecipe say so, and the fixture pair covers both cases.In bestax-mcp,
lookup_bulma_classesnow says thatFilerendersis-emptyitself whenhasNamehas no.file-name. Its agreement test, which runs the codemod's planner beside the lookup, gives ahas-name.filea name, as the codemod's own truth tests now do.I checked it in Chromium through Storybook in light and dark mode. Before a pick, all four corners are rounded in the default,
isRightandisBoxedlayouts, and after one the button meets the name the way Bulma draws it.Merge note: merge this with a merge commit, not a squash. The commits carry the
bulma-ui,bestax-migrateandbestax-mcpscopes, and a merge commit releases each of those packages from its own commit. A squash would release only the package its title names.pnpm allpasses locally.Fixes #978
Summary by CodeRabbit
is-emptyand keeps the filename hidden when converting matching markup.