feature: Add Form Validation support to uni-dom #408
Conversation
Composable, reactive form validation that integrates with the existing value.bind() two-way binding system. Purely Rx-based with no DomNode or DomRenderer changes needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
doc
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust and flexible form validation framework to Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed, reactive form validation system. The core logic is sound and the use of Rx throughout is excellent. The built-in validators cover common use cases, and the extension points for custom rules are flexible. The tests are comprehensive and cover many scenarios.
My review focuses on a few areas for improvement:
- Enhancing the usability of the imperative
validateNow()andvalidateAll()methods, which currently have a subtle dependency on the reactive stream being active. - Refactoring the implementation of
FormValidationto be more concise. - Improving the design documentation to highlight a potential pitfall for users.
Overall, this is a great addition to uni-dom.
| def validateNow(): ValidationState = | ||
| lastValue match | ||
| case Some(v) => | ||
| runRules(v) | ||
| case None => | ||
| ValidationState.Invalid(Seq("No value available")) |
There was a problem hiding this comment.
The validateNow() method can be tricky to use correctly. It relies on lastValue, which is only populated if the source Rx has been subscribed to and has emitted a value. If validateNow() is called before this, it will incorrectly return Invalid(Seq("No value available")). This is a potential footgun for users of the library.
To improve usability, especially for the common case where the source is an RxVar, you could try to get the value from the RxVar directly if lastValue is None.
def validateNow(): ValidationState = {
val v = lastValue.orElse(source match {
case rxv: RxVar[A] => Some(rxv.get)
case _ => None
})
v.map(runRules).getOrElse(ValidationState.Invalid(Seq("No value available")))
}
plans/2026-02-05-form-validation.md
Outdated
| - Purely Rx-based: validation state is plain `Rx` values consumed by existing reactive DOM binding | ||
| - Validation runs on every change by default (reacts to all updates from `value.bind()`) | ||
| - FormValidation uses chained pairwise `Rx.join` via `foldLeft` to support any number of fields | ||
| - Tracks last value internally for imperative `validateNow()`/`validateAll()` |
There was a problem hiding this comment.
The design decision to track the last value for imperative validation is good, but it has a subtle requirement: the reactive stream must be active (subscribed to) for validateNow()/validateAll() to work correctly. This is a potential pitfall for users. It would be beneficial to document this requirement here, for example by adding a note:
- Tracks last value internally for imperative `validateNow()`/`validateAll()`
- **Note**: These imperative methods require the reactive chain to be active (i.e., subscribed to) to have access to the latest value.| val isValid: Rx[Boolean] = | ||
| fields.size match | ||
| case 0 => | ||
| Rx.variable(true) | ||
| case 1 => | ||
| fields(0).isValid | ||
| case n => | ||
| // Chain pairwise joins for any number of fields | ||
| val first: Rx[Boolean] = fields(0).isValid | ||
| fields | ||
| .tail | ||
| .foldLeft(first) { (acc, field) => | ||
| acc | ||
| .join(field.isValid) | ||
| .map { case (a, b) => | ||
| a && b | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of isValid can be simplified by using foldLeft with an initial value, which removes the need for the fields.size match and special casing for 0 or 1 fields. This makes the code more concise.
val isValid: Rx[Boolean] =
fields.map(_.isValid).foldLeft(Rx.variable(true)) { (acc, fieldIsValid) =>
acc.join(fieldIsValid).map { case (a, b) => a && b }
}| val errors: Rx[Seq[String]] = | ||
| fields.size match | ||
| case 0 => | ||
| Rx.variable(Seq.empty) | ||
| case 1 => | ||
| fields(0).errors | ||
| case n => | ||
| val first: Rx[Seq[String]] = fields(0).errors | ||
| fields | ||
| .tail | ||
| .foldLeft(first) { (acc, field) => | ||
| acc | ||
| .join(field.errors) | ||
| .map { case (a, b) => | ||
| a ++ b | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to isValid, the implementation of errors can be simplified by using foldLeft with an initial value. This makes the code more concise and consistent.
val errors: Rx[Seq[String]] =
fields.map(_.errors).foldLeft(Rx.variable(Seq.empty[String])) { (acc, fieldErrors) =>
acc.join(fieldErrors).map { case (a, b) => a ++ b }
}Remove unused imports, drop @volatile (single-threaded Scala.js), add scaladoc for pattern's full-match behavior, and add test for validateNow() before subscription. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- validateNow() reads RxVar directly for reliable imperative use - Simplify FormValidation.isValid/errors with foldLeft from initial value - Update plan doc with subscription requirement note Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust and reactive form validation system to uni-dom, leveraging Rx for composable field and form validations. The design is clean, integrating well with existing two-way binding, and includes a good set of built-in validators. The accompanying test suite is comprehensive, covering various scenarios and edge cases, which is excellent for ensuring reliability. The documentation in the plans file is also clear and well-structured.
| def email(message: String = "Invalid email address"): ValidationRule[String] = pattern( | ||
| "^[^@\\s]+@[^@\\s]+\\.[^@\\s]+$", | ||
| message |
There was a problem hiding this comment.
The current email regex ^[^@\s]+@[^@\s]+\.[^@\s]+$ is quite basic and might not cover all valid email formats or might allow some invalid ones. While the comment mentions it's a "simple practical regex", for a validation library, a more robust regex or integration with a dedicated email validation utility could enhance its reliability and user experience. Consider using a more comprehensive regex or a library if strict email validation is critical.
Merge main (Geolocation feature) and document email regex's intentional simplicity per Gemini review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep validateNow() simple with lastValue tracking, restore explicit FormValidation size matching, and trim verbose scaladoc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
value.bind()/checked.bind()two-way bindingrequired,minLength,maxLength,pattern,email, plus customrule/ruleWithDesign
ValidationStateenum (Valid/Invalid) withisValid/errorsaccessorsFieldValidation[A]derives reactivestate/isValid/errorsviasource.map()FormValidationaggregates fields using chainedRx.join(supports any number of fields)Validateobject provides factory methods and built-in validatorsStorage.scalaTest plan
./sbt "domTest/testOnly *ValidateTest"passes./sbt scalafmtAllclean🤖 Generated with Claude Code