Repository navigation
fix(create-bestax): stop the vite-ts build shadowing its config, and fix the starter page - #986
Merged
Merged
Conversation
…fix the starter page The vite-ts template's tsconfig.node.json was composite with nothing to stop it emitting, so `tsc -b` wrote vite.config.js next to vite.config.ts and Vite loaded that stale copy from then on. The template now splits its TypeScript config the way Vite's react-ts template does, a solution tsconfig.json referencing tsconfig.app.json and tsconfig.node.json, both noEmit. The starter page wraps its content in main > Section > Container, so the columns' negative margins sit inside the section's padding instead of widening a phone screen. The success Notification gets the hasDelete its onDelete was waiting for. Headings run h1, h2, h3, card headers sit inside the main landmark rather than reading as banners, the docs link is underlined, the subtitle drops the grey that failed contrast in dark mode, and card icons are decorative IconText instead of an Icon announced as "icon". main imports the bestax stylesheet ahead of App, so a class in App.css wins over a Bulma rule of the same weight. Under the no-helpers flavors every helper prop renders nothing. The generated CLAUDE.md now says so and sends inline styles to named classes in src/App.css, and the scaffolder swaps the starter's helper props for such classes, so the title stays centered and the cards even. IconText spaces icons with Bulma's own rule, so that gap holds there too. Material Symbols imports only outlined.css, the style Icon uses, instead of all three fonts, and pins ^0.46.0, the newest minor bestax-bulma's peer range admits. The generated README names the package manager that ran the CLI and the icon library picked. The skills follow: the icons reference gives the outlined import, and the optimize worked example lists the starter's new inventory, re-measured pixel-equal to the prebuilt flavor.
…ter-template # Conflicts: # bestax-mcp/data/skills.json
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (94)
📒 Files selected for processing (23)
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 |
Contributor
Preview DeploymentPreview URL: https://3f3a95a8.bestax.pages.dev |
Owner
Author
|
deep-review: fresh The visual baselines for the new starter page are committed, so this review posts every finding, advisory included, as its own thread. It reviews 14a81f6. |
Contributor
Preview DeploymentPreview URL: https://e60858c2.bestax.pages.dev |
…ter-template # Conflicts: # bestax-mcp/data/skills.json
Owner
Author
Contributor
Preview DeploymentPreview URL: https://43c2a699.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Owner
Author
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 597c392. |
Contributor
Preview DeploymentPreview URL: https://f03fa670.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 3 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Accessibility | Reset dropped focus to the body when it disabled itself — verified fixed, resolved | create-bestax/templates/vite-ts/src/App.tsx:126 |
| 2 | 🔵 Advisory | Accessibility | Neither notification was a live region — verified fixed, resolved | create-bestax/templates/vite-ts/src/App.tsx:153 |
| 3 | 🔵 Advisory | Coverage | HELPER_PROP missed every viewport-suffixed helper — verified fixed, resolved |
create-bestax/src/__tests__/templates.test.ts:42 |
Overall: This pass settled threads and reviewed no commits. All three remaining advisory threads are genuinely fixed in the current tree, and each fix carries its own regression test: countRef.current?.focus() in the Reset handler plus the e2e test keeps focus on the counter when Reset disables itself; role="status" on the shared notification Column, mounted empty so only its content changes, plus announces both notifications through a status region; and HELPER_PROP now derived from the TypeScript AST of useBulmaClasses.tsx rather than a hand-written list, with a guard test that pins the suffixed variants and asserts that displayTablet="flex" matches while color="info" does not. Both templates, App.tsx and App.jsx, carry the two accessibility fixes identically, and turbo.json declares the newly-read bulma-ui files as inputs to create-bestax test tasks so the cache cannot go stale. Nothing here is left for a human to arbitrate.
Residual risk:
- The helper-prop gate could still be blind to a new helper shape — refuted for the known set: the AST read takes every name the hook destructures off
props, so the viewport variants,overflowX/overflowYand anything added to the library later are covered, and the guard test fails if that read ever comes back short, for example if the hook split its destructuring. A helper reaching the DOM without passing throughuseBulmaClasseswould still slip, but no such path exists today. role="status"impliesaria-atomic, so a count change re-reads the success notification too — on the record as an acknowledged trade-off, chosen over two regions to preserve the spacing between the notifications. Not a defect.- Focus after a mouse click on Reset — the Count button does receive focus but renders no
:focus-visiblering, which is why the06-counter-resetscreenshot stayed pixel-identical at zero tolerance. Correct behaviour, not a gap. - No new findings were raised, per VERIFY mode.
pnpm --filter create-bestax run testran green locally: 11 suites, 422 tests.
🏄 Dropped in on the three leftover threads and every one was already handled clean — that AST-driven helper gate is honestly a nicer line than the one I paddled out with. Zero open, board is back on the beach, this one is good to go.
Owner
Author
|
deep-review: fresh The threads from the last run are settled, and main is merged in. Since the review on f4591e0 this adds the Reset focus fix, the status region around the starter's notifications, the no-helpers gate that now reads its helper prop list from |
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Coverage | The rewritten no-helpers gate requires a =, so bare boolean helper attributes (relative, clearfix, fullHeight, gapless) still pass it |
create-bestax/src/__tests__/templates.test.ts:80 |
| 2 | 🔵 Advisory | API | "and the rest render nothing" over-claims — skeleton works under the no-helpers flavors, which forward bulma/sass/base/skeleton |
create-bestax/src/constants.ts:183 |
| 3 | 🔵 Advisory | Accessibility | role="status" is implicitly aria-atomic="true", so every click past 10 re-announces the whole column, open success notification included |
create-bestax/templates/vite-ts/src/App.tsx:137 |
Overall: The change is sound and unusually well evidenced — both issues reproduce as described, and each fix is held by a test that reads the real template rather than a stand-in. The riskiest part is now the gate, not the starter: templates.test.ts is the only thing standing between a future starter edit and a silent repeat of #948, and finding 1 is the one hole left in it. Look at that first; findings 2 and 3 are polish on text and markup that a reader will copy.
Residual risk:
- Boolean helper props escape the no-helpers gate. Open — posted as finding 1.
is-relative/is-clearfix/is-gaplesslive inbulma/sass/helpers/{position,float,gap}.scss, whichbestax-no-helpers.scssnever forwards, andis-full-heighthas no Bulma rule at all. - The gate only reads
src/App.{tsx,jsx}. Open.replaceStarterHelperPropsand both scans are hard-wired to that one file, so a starter that grows a second component carrying helper props is outside the check. No current template does. - A helper prop on an element that already has
className. Open. The swap issplit(prop).join('className="…"'), which would leave twoclassNameattributes on such an element — later-wins in JSX, so a quiet layout loss. No starter element carries both today, and nothing asserts that stays true. - A helper prop spelled so the literal swap misses it (
textAlign={'centered'}, single quotes). Refuted:HELPER_PROPstill matchestextAlign=, so the no-helpers test fails loudly instead of shipping an unstyled starter. - Baselines predate the last five commits (regenerated at
14a81f6; HEAD is597c3923). Refuted. The only post-baseline change that could move pixels is Reset's programmatic.focus(), caught by06-counter-reset.png— but Bulma 1 styles.button:focus-visible, not:focus(bulma/sass/elements/button.scss:274), and that screenshot follows a mouse click, so Chromium's focus-visible heuristic does not fire.material-icons/iconfont/filled.cssdeclares.material-iconsexactly as the bare import does, sovite-complete-material-iconsrenders the same glyphs.role="status", theCSS_ORDER_COMMENTand the turbo/test changes emit no markup or class differences. - The solution-style
tsconfig.jsonerroring on non-compositereferences (TS6306) ornoEmitones (TS6310). Refuted against the pinned compiler:verifyProjectReferencesraises both only when the parent'sinputs.lengthis non-zero (typescript/lib/typescript.js:130158-130161), and the solution config is"files": []. Sotsc -bbuildstsconfig.app.jsonandtsconfig.node.json, bothnoEmit, and writes novite.config.js— which is the #947 fix, and the walk intemplates.test.tspins that both projects are still reached and emit nothing. - The new turbo inputs missing a read. Refuted: the only out-of-package reads in
templates.test.tsarebulma-ui/package.json(viacreateRequire) andbulma-ui/src/helpers/useBulmaClasses.tsx, both declared; and the basetest/test:coveragetasks carry nodependsOn, so the per-package overrides drop nothing (test:coveragekeeps itscoverage/**outputs). - Coverage of the new a11y/layout behaviour in the PR gate. Open, by design. The eight new
app.spec.tsassertions run only invisual-regression.yml, notci.yml, so the Reset-focus, status-region, landmark and underline guarantees are not enforced on a PR — the jest side covers the scaffolder edits, not the rendered page.
🏄 Dude, this one paddled out for two gnarly issues and came back with the whole reef mapped — tsconfig no longer shadowing itself, phone-width overflow gone, focus actually landing somewhere, and a gate that reads the library instead of guessing at it. Three little advisories bobbing in the lineup, nothing that will drop you. Send it.
…rs gate The gate matched helper props by a pattern that required an `=`, so a boolean helper written bare, such as `<Box relative>`, passed it. It now parses the App and reads the JSX attribute names, which finds a prop in any form and ignores the same word in text or a comment.
…o-helpers flavors The generated CLAUDE.md said every helper prop renders nothing in a no-helpers app, but those flavors keep Bulma's skeleton styles, so `skeleton` works there as it does in complete. The text now claims only the props whose class the flavor leaves out, names `skeleton` as one that still works, and a test holds both claims to what the flavor's stylesheet forwards.
… its own A status region reads out everything in it when any of it changes, and the starter's one region held both notifications, so every click past ten read the success notification out again with the new count. Each notification now sits in a region of its own, mounted empty before it appears, so a screen reader hears the one that appeared or changed. Bulma spaces a notification only from what follows it in the same parent, and each notification is now alone in its region, so App.css puts that space between two regions that both hold one. The page renders the same as before in every state, both notifications at once included.
Owner
Author
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on ad6f567. |
Contributor
Preview DeploymentPreview URL: https://62a3d631.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 3 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Coverage | Gate could not see a bare boolean helper prop — now parses the TSX AST for JSX attribute names | create-bestax/src/__tests__/templates.test.ts:84 |
| 2 | 🔵 Advisory | API | "and the rest" over-claimed; skeleton does render under the no-helpers flavors — claim narrowed and skeleton named |
create-bestax/src/constants.ts:183 |
| 3 | 🔵 Advisory | Accessibility | One atomic role="status" re-read the whole column on every click past 10 — each notification now has a region of its own |
create-bestax/templates/vite-ts/src/App.tsx:139 |
Overall: This pass settled threads and reviewed no commits. All three open advisories are fixed, each by a better route than the one I suggested: the gate parses the App with the TypeScript compiler instead of a regex, which closes the bare-attribute hole without the m/p/gap false alarms my pattern would have caused; the CLAUDE.md claim is narrowed to "helper props that add those classes" and names skeleton, so it stays true as the hook gains props; and each notification gets an atomic status region of its own instead of aria-atomic="false", which avoids trading the re-read for a bare "12". I checked each against the current code rather than the commit messages — both no-helpers stylesheets do @forward "bulma/sass/base/skeleton" and neither forwards bulma/sass/helpers (bestax-no-helpers.scss:17, bestax-no-helpers-prefixed.scss:16); the complete case still asserts helperPropsIn(app) is non-empty, so the new reader is wired to the real App and not silently returning an empty list; and replaceStarterHelperProps appends to the same App.css, so the no-helpers flavors inherit the new .status-region rule. pnpm --filter create-bestax run test: 11 suites, 427 tests, all passing. Nothing on these three is left for a human.
Residual risk:
- Thread 1 — a helper prop the AST walk still misses. A JSX spread attribute carries no
JsxAttributenode, so the gate would not see a helper prop arriving that way. Refuted as a practical risk:replaceStarterHelperPropsswaps props by literal string match (project-creator.ts:317-319), so a spread in the starter would already defeat the swap itself, which the complete-versus-no-helpers assertions in that same test catch. Not a gap this fix introduced. - Thread 2 — the sentence drifting from the CSS. Refuted. The new test resolves the stylesheet of each flavor from its own
importStatementand asserts both halves (skeleton forwarded, helpers not), so a flavor that starts forwardingbulma/sass/helpersor drops skeleton fails there rather than shipping a false claim. - Thread 3 — the spacing rule silently not applying. The new e2e derives the expected gap from the box computed
margin-bottomrather than hardcoding a value, and the author reports the comparison failing once the rule is taken out of the built CSS, so a missing--bulma-block-spacingwould surface as a failure and not a silent 0. I could not re-run the Playwright suite here (no browsers, and the sandbox blocked the file mutation I wanted as a negative control), so this one rests on the unit suite plus reading the spec — named here as reasoned, not executed. - Beyond the three threads: nothing. This is a verify pass, so no new finding was sought and none is reported.
🏄 Three mellow advisories rolled in, three paddled right back out — and the author did not just take my lines, he found better ones on all three. Tests green, code reads clean. Good to go, dude.
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.4 🎉 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 📦🚀 |
This was referenced Oct 10, 2026
Merged
|
🎉 This PR is included in version 1.14.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The vite-ts template's
tsconfig.node.jsonwascompositewith nothing to stop it emitting, so the build'stsc -bwrotevite.config.jsandvite.config.d.tsinto the project root. Vite prefers the.jsfile, so every edit tovite.config.tsafter a first build was silently ignored. The template now lays out its TypeScript config the way Vite's own react-ts template does:tsconfig.jsonis a solution file that referencestsconfig.app.json(the old app options, unchanged) andtsconfig.node.json, and neither emits. A scaffold built withtsc -bnow leaves novite.config.jsbehind, and both projects are still type-checked.The starter page had several problems, all in both templates.
onDeletewithouthasDelete, so it never showed a close button. It does now.Columnsstraight insideContainer, which has no side padding below desktop width, so the columns' negative margins made a phone screen scroll sideways, with boxes flush to the edges. The page is nowmain > Section > Container, Bulma's own nesting, and the section's padding holds the gutter.no-helpersflavors every helper prop renders a class that stylesheet doesn't have. The generated CLAUDE.md now says so in the setup section, and its house style sends an inline style to a named class insrc/App.cssinstead of to a helper-prop table that would render nothing. The scaffolder swaps the starter's helper props (the title's centering, and the flex pair that evens the card heights) for named classes with their rules inApp.css, so the starter keeps its layout and shows that flavor's way of styling. Card icons now useIconText, whose spacing is Bulma's own rule, so they no longer touch their titles in any flavor.mainlandmark, which also stops each card's<header>from being exposed as a page banner; headings run h1, h2, h3 in order (card titles are now h3); the bestax.io link is underlined; the subtitle drops the grey text color that failed contrast in dark mode; and the card icons arearia-hiddeninstead of each adding "icon" to its heading. axe reports nothing on the new starter, light or dark, in the complete, no-helpers and no-helpers-prefixed flavors. On the old no-helpers starter it reported the duplicate banners, the missingmain, the heading order, the link and the icons' labels.material-symbols/outlined.css, the styleIconrenders by default, instead of the bare import that puts all three fonts into the build. The generated CLAUDE.md says which style is loaded and how to add the others. Its pin moves to^0.46.0, the newest minor bestax-bulma's peer range admits:^0.47.6makesnpm installfail with ERESOLVE against the published library, so going further needs bulma-ui to admit 0.47 first. A test now holds every icon pin to the newest arm of the library's peer range for it.mainnow imports the stylesheets ahead ofApp, so a named class inApp.csswins over a Bulma rule of the same weight, which the no-helpers classes depend on. The icons in those flavors also rely on the #946 fix, which loads the icon stylesheet after every flavor's stylesheet, not only afterbestax.css.The generated README is part of #949, but it needed CLI code, so it's here: it now gives its commands for the package manager that ran the CLI (the one the success message already names) and lists the icon library picked, instead of always saying pnpm and Font Awesome.
The
gapline #948 lists is already right on main (#877 corrected the CLAUDE.md template and the skills); this create-bestax release is what carries it to scaffolded apps.The skills follow the template: the icons reference gives the outlined Material Symbols import, and the bestax-optimize worked example lists the starter's new helper inventory and Bulma's
deletemodule for the close button, re-measured pixel-equal to the prebuiltcompleteflavor.Tests:
templates.test.tsruns the scaffolder's edits against the real templates (TypeScript projects that emit nothing, no helper prop left under a no-helpers flavor,IconTextcard titles for every icon library, the README per package manager, the icon pins against the peer range), and each of those fails on main. It also runs the #946 import order end to end on disk, after the no-helpers rewrite, as a check on the templates rather than a second copy of that fix's tests. New e2e assertions without screenshots check the phone-width overflow, centering and even cards, icon spacing, the close button, landmarks and heading order, and the underline; each fails against the old no-helpers Font Awesome starter and passes on the new one.The visual-regression baselines change with the layout, so they need regenerating on this branch (
visual-regression.ymlwithupdate_snapshots=true) before that workflow goes green.Listings: cursor.directory and ClawHub hold copies of each skill's text, so their bestax-icons and bestax-optimize copies keep the old Material Symbols import and worked example until someone refreshes them.
Left out: Bulma v1's notification padding doesn't reserve room for the close button, so on a narrow column the button overlaps the end of the text. That's library CSS, not the template. Material Symbols is a variable font of about 4 MB even for the one style; getting the download smaller means subsetting it (Google Fonts'
icon_names, say), which changes how the font is delivered, so it isn't done here.pnpm allpasses locally.Fixes #947
Fixes #948