feat(ui): add MagicSelector, which scopes a rebuild to one controller field - #149
Conversation
… field refreshUI() notifies every listener and MagicStatefulViewState answers with setState on the whole view. That is the right default, and it stops being cheap on a screen where one field changes often and most of the screen does not care: a consumer measured one keystroke in a search field rebuilding 220 styled containers. MagicBuilder could not help, because it needs a ValueListenable and a controller is a ChangeNotifier. The mechanism is the cache, not the listener. MagicSelector keeps the widget its builder returned and, while the selected value compares equal, returns that same INSTANCE, so Element.updateChild short circuits on child.widget == newWidget and never descends. A widget that merely skipped its own setState would still be rebuilt from above, which is the situation inside every MagicStatefulView. A changed selector or builder deliberately does not invalidate the cache. Both are written inline in a parent's build, so both are a fresh closure every time and comparing them by identity would drop the cache on exactly the rebuild this exists to survive. A changed selector still takes effect the moment it returns a different value; a changed builder that would render differently from the same value is what the purity contract in the class doc rules out. Equality is plain ==. A selector returning a freshly built List never matches its own cache, which is pinned in a test rather than fixed: deep comparison of a ten thousand element list on every notification costs more than the rebuild it prevents.
Covers what it is for, why returning an identical instance is the mechanism, the purity contract the caching forces, and why equality is plain == rather than a deep comparison. Ends on when to reach for which: MagicBuilder when the source already is a ValueListenable, MagicSelector when it is the controller.
Bumps the skill to 0.1.13. Rule 6 now names which of the two section builders fits which source, because the distinction is the thing an agent gets wrong: MagicBuilder needs a ValueListenable and a controller is not one.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The widget is correct and the caching mechanism it rests on holds up against the framework source; what I found is three small sync/DX items, none of them blocking. I verified the central claim rather than taking it on trust. Minor
One doc line worth tightening
Tests11 new tests in Checks I ran
|
Hot reload marks descendants dirty, so an edit INSIDE the cached subtree showed
up on its own. An edit to the builder did not: the cached instance is what those
descendants rebuild against, so changing builder: (n) => Text('$n items') to
Text('$n rows') kept showing items until the selected value happened to move.
reassemble only runs in debug, so dropping the cache there is free.
Two tests, one per contract hole review named. The second pins the hole rather
than closing it: a changed builder is not seen while the value holds, which is
what the purity contract exists to rule out, written down so the next reader
meets it as a decision.
Both docs said reading an InheritedWidget inside the cached subtree needs no selection, which is true and invites the reading that goes stale: a WindTheme.of(context) written in the enclosing build captures the view's context, so a theme change rebuilds the view, the cache is served, and the subtree keeps the old theme. Same class as the captured-total hole, and a dark-mode toggle is a likelier way to meet it. The stamp comment in SKILL.md still read v0.1.12 while the frontmatter had moved to 0.1.13, so the file shipping downstream disagreed with itself. And the changelog entry opened a second ### Added under [Unreleased]; publish.yml builds the release body from that section, so it would have shipped with two.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The three commits since my last review resolve every item I raised, and each fix is the right shape; nothing new to flag. What changed since The Tests13 tests now (up from 11). The two additions cover the hot-reload cache drop and the changed-builder contract, both under the new Checks I ran
|
1 participant
The problem
refreshUI()notifies every listener, andMagicStatefulViewState._onControllerChangedanswers by callingsetState(() {})on the whole view (lib/src/ui/magic_view.dart:177). That is the right default: a controller does not know which of its fields a screen reads, and a view that rebuilds is always correct.It stops being cheap on a screen where one field changes often and most of the screen does not care. Measured in a consumer with Flutter's own build profiling, at a realistic data scale: one keystroke in a search field rebuilt 220 styled containers, and the seven-keystroke session was the most expensive interaction on the screen by a wide margin (p90 207 ms against 77 to 80 ms for every scroll session in the same run).
MagicBuildercould not help. It takes aValueListenable<T>(lib/src/ui/magic_builder.dart:115), and aMagicControlleris aChangeNotifier. The only workaround available today is a hand-maintainedValueNotifierper field, which means two notification mechanisms in one controller and a second thing to keep in sync withrefreshUI().The mechanism, which is the cache rather than the listener
This is the part worth reviewing carefully, because the obvious implementation does not work.
A
BlocSelector-shaped widget gates its ownsetStateon an equality check. That is enough when notifications arrive through anInheritedWidget, and useless here: the parent view rebuilds unconditionally from above, so the child is rebuilt whether or not it wanted to be, and its own gate is never consulted.MagicSelectorcaches the widget its builder returned and, while the selected value compares equal, returns that same instance.Element.updateChildshort circuits onhasSameSuperclass && child.widget == newWidget(packages/flutter/lib/src/widgets/framework.dart:4027on 3.47.0), and an identical instance satisfies that, so the descent ends there and the subtree is never visited.Two decisions that will look wrong at first
A changed
selectororbuilderdoes not invalidate the cache. Both are written inline in a parent'sbuild, so both are a fresh closure on every parent rebuild; comparing them by identity drops the cache on exactly the rebuild this widget exists to survive. I wrote it the other way first and two tests failed for precisely that reason. A changed selector still takes effect the moment it returns a different value, becausebuildre-reads it. A changed builder that would render differently from the same value is the one case this cannot see, which is why the class doc makes purity a contract rather than a suggestion.Equality is plain
==, not a deep comparison. provider'sSelectordefaults toDeepCollectionEquality; flutter_bloc'sBlocSelectoruses!=. This follows bloc. Walking a ten thousand element list on every keystroke costs more than the rebuild it prevents, and the consumer that motivated this has exactly that list. The consequence, that a selector returning a freshly builtListnever matches its own cache, is pinned in a test and documented rather than hidden.Tests
11 tests in
test/ui/magic_selector_test.dart, written before the implementation. The first group is the headline case: a probe inside the selector and a probe outside it, under a realMagicStatefulView, so the sibling's rising count proves the full-view rebuild happened while the scoped count stays at 1 across seven unrelated notifications.The rest cover standing alone with no view above it, a notification that does not move the value, swapping the controller instance (with listener counts on both sides), a changed selector, detaching on unmount, a record selecting two fields, and the documented
Listbehaviour.ProfileControllercounts its own listeners rather than readingChangeNotifier.hasListeners, which is@protectedand produces aninvalid_use_of_protected_memberwarning outside a subclass instance member.Gates
dart analyzeclean,dart format .no diff, 1438 tests green (no new skips),lib/src/ui/magic_selector.dartat LF:30 LH:30,dart pub publish --dry-rununchanged from master.Post-change sync:
CHANGELOG.md(Unreleased / Added),doc/basics/ui-helpers.md(a section beside MagicBuilder, plus its ToC entry),skills/magic-framework/SKILL.md(rule 6 now says which builder fits which source, version 0.1.12 to 0.1.13),skills/magic-framework/references/controllers-views.md(full section plus ToC).example/is deliberately untouched. It is regenerated per release bymagic:install(CLAUDE.md), so a hand-written demo there is a liability, and none of the existing UI helpers (MagicBuilder,MagicTitle,MagicCan) appear in it either. The usage examples live in the doc.Found while writing the tests, not fixed here
import 'package:magic/magic.dart'shadowsdart:ui'sTextDirectionenum withpackage:intl'sTextDirectionclass, becauselib/magic.dart:7blanket-exports intl. SoDirectionality(textDirection: TextDirection.ltr)does not compile in any consumer file that imports the barrel, and the error ("Member not found: 'ltr'") does not name the cause. The test file works around it withhide TextDirectionand a comment. The fix has a precedent five lines above the offending export:file_pickeris already exported by name for the same class of collision. Separate PR.