Skip to content

feat: rework validation modes - #54

Open
mateusz-pietras wants to merge 4 commits into
refactor/forms-reworkfrom
feat/LMG-399-validation-modes
Open

feat: rework validation modes#54
mateusz-pietras wants to merge 4 commits into
refactor/forms-reworkfrom
feat/LMG-399-validation-modes

Conversation

@mateusz-pietras

Copy link
Copy Markdown
Member

No description provided.

@mateusz-pietras
mateusz-pietras force-pushed the feat/LMG-399-validation-modes branch from 037f412 to 6bcb4e7 Compare August 13, 2026 21:59
@KamilSztandur
KamilSztandur force-pushed the fix/validation-issues branch from 0226b80 to 797f304 Compare August 14, 2026 07:43
One named mode on the form says what the form does: `disabled` (the
default), `onUserInteraction`, or `onUnfocus`. It is broadcast to every
field and subform, including ones registered or attached later, so a
section added after the first submit behaves like one added at build
time.

The decision lives in one new file, `lib/src/validation_mode.dart`: the
enum, the internal event enum, and `validatesOn` — a pure function of
the event, the mode and the interaction flag. It reads no field status,
which is what makes it safe on a pipeline that overwrites the status
with `pending` and clears both error slots on every write.

Above every mode: a field the user has never edited validates nothing on
its own, and a subtree with `validationEnabled` off validates nothing at
all. The switch is folded into the mode the form broadcasts, so a field
has one input to obey rather than two, and a switched-off subtree is
excluded from `validate()` and from every derived aggregate alike.

`validate()` neither consults the mode nor changes it — escalation is
deleted, which is what makes a single broadcast sufficient.

Also:

- `prefill(value)` writes a value the user did not type, without arming
  the guarantee.
- `focusNode` and `focus()` move up to `AdvancedFieldController`, and
  `handleUnfocus()` runs the blur path — flushing a debounce in any
  mode, reusing a settled verdict, retrying a failed round, and
  reporting a throwing validator instead of leaking it into the zone.
- A field or subform may claim its own mode and manage it from then on;
  a parent's later change reaches only the children that have not.
- `validateWithAutovalidate()` is renamed `revalidateSync()`.
- `SharedCall.invalidate()` drops a pass answered under settings that no
  longer hold.
- The error report tag says `leancode_forms`, not the stale
  `advanced_forms`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mateusz-pietras
mateusz-pietras changed the base branch from fix/validation-issues to refactor/forms-rework August 14, 2026 14:37
Cut the comment volume the validation-modes feature added, without
changing behaviour: facts that were repeated across three to five
doc-comments now live in one place and are linked from the rest.

Rename three private members to stop them colliding with their
neighbours: `_applyMode` -> `_publishValidationMode` (mirrors the form's
peer), `_formEnabled` -> `_parentEnabled` (same concept as the form's
field of that name), `_statusKeepingFailure` -> `_statusAfterAbort`.

Add `AdvancedFieldController._validatesOn`, replacing three five-line
`validatesOn(...)` calls, and inline the single-call-site
`_flushDebounce` into `handleUnfocus`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mateusz-pietras
mateusz-pietras force-pushed the feat/LMG-399-validation-modes branch from 6bcb4e7 to d0d9e7f Compare August 14, 2026 14:41
mateusz-pietras and others added 2 commits August 14, 2026 17:07
Both controllers interleaved several concerns in one file. Each concern
now lives in its own part file of the same library, so private state
stays reachable and no cross-file contract is needed.

Field controller: focus handling and the async validation round machinery.
Form controller: child wiring, relations and the state class.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@mateusz-pietras
mateusz-pietras marked this pull request as ready for review August 14, 2026 21:18
part of 'advanced_form_controller.dart';

// Listeners on child fields and subforms, and what to update when they change.
mixin _ChildWiring on ChangeNotifier {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a good idea of extracting it here? I'm worried that the public API will be harder to lookup - wdyt?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in the other comment, as long as it's mixin, not extension it's findable, then it's fine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting this logic into few mixins was a very good idea. Much more readable when logic is grouped

exception: error,
stack: stack,
library: 'advanced_forms',
library: 'leancode_forms',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why revert the name :D

Comment on lines -31 to -48
FocusNode? _focusNode;

/// The [FocusNode] bound to this field, created on first use.
///
/// Throws a [StateError] if controller is disposed.
FocusNode get focusNode {
if (isDisposed) {
throw StateError(
'Cannot use the focusNode of a disposed AdvancedTextFieldController.',
);
}

return _focusNode ??= FocusNode(
debugLabel:
'AdvancedTextFieldController${name?.isNotEmpty ?? false ? '($name)' : ''}',
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's removed because AdvancedFieldController already provides it, but now I think that we should be able to pass our own focus node. Add it to constructor

this.validationError,
this.asyncError,
this.autovalidate = false,
this.mode = ValidationMode.disabled,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we wanted to change the name from disabled to manual

// Runs async validation: starts a pass, waits for the result, and writes it
// only if still current. Round tracking lives on the controller (`_currentRound`,
// `_hasVerdict`, `_lastFailure`).
extension _ValidationRounds<T, E extends Object>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it as mixin if possible, so it's explicitly declared where these methods are coming from when reading advanced_field_controller.dart code

part of 'advanced_form_controller.dart';

// Listeners on child fields and subforms, and what to update when they change.
mixin _ChildWiring on ChangeNotifier {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in the other comment, as long as it's mixin, not extension it's findable, then it's fine

part of 'advanced_form_controller.dart';

// Listeners on child fields and subforms, and what to update when they change.
mixin _ChildWiring on ChangeNotifier {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting this logic into few mixins was a very good idea. Much more readable when logic is grouped

@@ -0,0 +1,101 @@
part of 'advanced_form_controller.dart';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doesn't have to be part of this file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's public API, just export it separately

Comment on lines 35 to 40
AdvancedFieldController({
required T initialValue,
Validator<T, E>? validator,
AsyncValidation<T, E>? asyncValidation,
this.name,
}) : _value = AdvancedFieldState<T, E>(value: initialValue),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof I didn't notice it earlier. We have to allow developers to pass their own FocusNodes

final bool autovalidate;
/// When this field validates itself. The **effective** mode: a field whose
/// form has validation switched off reports [ValidationMode.disabled].
final ValidationMode mode;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd name this field validationMode

/// Not used for identity — fields are identified by reference.
String? get name;

/// Tells the field the user has left it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the user has left it

IDK, but sounds weird to me


// Listeners on fields this form does not own — for external callbacks only.
// Does not update the form's own state.
mixin _Relations on ChangeNotifier {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's private, maybe we can tighten the on type to the controller type, instead of allowing any ChangeNotifier?

The similar thing goes for other mixins introduced in this PR

Comment on lines +167 to +172
unawaited(
validate().catchError((Object error, StackTrace stackTrace) {
_report(name, 'validating after focus loss', error, stackTrace);
return false;
}),
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd override handleUnfocus as async and use regular await + try/catch


void _setState(AdvancedFormState newValue);

final _onValuesChanged = ChangeNotifier();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this notifier (and _onStatusChanged) disposed of properly? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now at 1763 lines, tl;dr — split up into multiple files mayhaps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants