Repository navigation
fix(create-bestax): validate at submit in the bestax-form signup example - #474
Conversation
The canonical SignupForm submitted on the stale render-scope error after setTouched(true), so an invalid email that was never blurred passed the if (!error && email) guard on first submit. Compute the validity once and check it in the handler, matching the corrected pattern the Form Library Agnostic post teaches, and wire the label to the input (id plus labelProps htmlFor) the way the skill's own accessibility note prescribes. The Storybook skill-example showcase uses a different, submit-gated pattern and is unaffected.
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
There was a problem hiding this comment.
Pull request overview
Fixes stale validation in the bundled bestax-form signup example.
Changes:
- Computes and checks email validity directly on submission.
- Associates the email label with its input for accessibility.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| id="signup-email" | ||
| labelProps={{ htmlFor: 'signup-email' }} |
Preview DeploymentPreview URL: https://758459ed.bestax.pages.dev |
allxsmith
added
documentation
AI triage — issues this PR may resolve
If this PR resolves one of these, add the line below to the PR description |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 1 advisory
| No | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Accessibility | The Storybook skill-example Signup story (which showcases agent output of this skill) does not model the id + labelProps={{ htmlFor }} wiring that the canonical example and the skill's own a11y note now teach; a pre-existing gap, not a regression from this PR. | bulma-ui/src/skill-examples/Form.stories.tsx:83 |
Overall: The change is sound and correctly scoped. The original example had a genuine stale-closure bug: the submit handler read the render-scope error, which is undefined before the first setTouched(true) takes effect, so 'if (!error && email)' submitted an invalid-but-non-empty email on the first submit. The fix computes 'valid' unconditionally and gates the submit on it ('if (valid)'), and the dropped '&& email' guard is correctly subsumed because an empty string fails the regex. The id + labelProps={{ htmlFor }} addition is a valid API (Input forwards labelProps to Field and forwards id to InputBase via rest props) and matches the skill's own accessibility note. The fix(create-bestax) scope is right, since the skill is bundled into create-bestax at its build time. Nothing here needs a human to block on; the reviewer can glance at the advisory and decide whether the Storybook showcase is worth syncing in a follow-up.
Residual risk:
- Same stale-read shape elsewhere — refuted: grepping for the buggy guard across skills/, docs/, and bulma-ui/src surfaces only the intended file, the already-correct blog demo (from #471), and the onBlur handlers. No other copy teaches the buggy pattern.
- The skill-examples Signup story carrying the same bug — refuted: it uses a submit-gated 'submitted ? errors[k] : undefined' pattern where errors is computed unconditionally from current state, so there is no stale-closure read. The PR's unaffected claim is accurate.
- An invalid API being taught — refuted: reading Input.tsx and Field.tsx confirms labelProps is typed React.LabelHTMLAttributes (which includes htmlFor) and id lands in the rest props spread onto InputBase.
🏄 Total gnarly little closure riptide, dude: the old code let a bogus email slip past the break on the first paddle-out, but this patch reads the wave right off 'valid' and the a11y leash is finally clipped in. Clean set, no blockers, go ahead and drop in.
Per review: skills/CLAUDE.md requires the Storybook showcases to track the canonical example, and the signup story's Name, Email, and Country controls rendered labels without the id plus labelProps htmlFor wiring the skill now teaches. Scoped to the signup story, which is the showcase of the canonical example this PR changes.
Preview DeploymentPreview URL: https://d0646a81.bestax.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
skills/bestax-form/SKILL.md:156
- The input is still
type="email", so browsers can block the submit event for values that fail native email constraint validation. For example, pressing Enter while an unblurred field containsfoonever reaches this handler,setTouched(true)does not run, and the canonical userland-validation example still does not show its inline error. Disable native validation withnoValidateon this form (and the corresponding shipped demos) so the regex-based submit path handles every invalid value consistently.
if (valid) {
|
🎉 This PR is included in version 4.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Found while writing the Form Library Agnostic post (#471): the
bestax-formskill's canonicalSignupFormreads the stale render-scopeerrorright aftersetTouched(true), so an invalid email that was never blurred passesif (!error && email)on first submit. The blog demo deliberately ships the corrected pattern, which left the two shipped surfaces teaching different code.const valid = …regex.test(email)once; gate the error ontouched && !valid; checkvalidin the submit handler (the&& emailguard is subsumed, an empty string fails the regex).id+labelProps={{ htmlFor }}to the example, which the skill's own accessibility note (label does not wirehtmlFor) prescribes but the example did not model.bulma-ui/src/skill-examples/Form.stories.tsx: its signup story uses a submit-gatedsubmitted ? errors[k] : undefinedpattern without the stale read, so it is unaffected.fix(create-bestax)scope so the bundled skill copy actually releases.pnpm format:checkandpnpm check:conformancepass.