fix(drinks): move provider mutations out of setState - #538
Conversation
Both search-dismissal callbacks called provider.setSearchQuery('') from
inside a setState closure. setSearchQuery calls notifyListeners(), which
synchronously marks every watching element dirty, so the callback mixed
two rebuild-scheduling mechanisms in one block — the shape that produces
"setState() or markNeedsBuild() called during build" as soon as anything
upstream reorders. setState now mutates widget-local fields only and the
provider call runs after it.
The toggle only clears when collapsing, so that condition is hoisted to
a local rather than dropped.
Adds test/drinks_screen_search_dismiss_test.dart: neither dismissal path
had coverage. The tests assert the user-visible outcome — the bar closes
and the unfiltered list returns — and pass against both the old and new
implementations, pinning the refactor as behaviour-preserving.
Fixes #526
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #526 by ensuring setState in DrinksScreen only mutates widget-local state, while BeerProvider.setSearchQuery('') (which calls notifyListeners()) runs outside the setState closure to avoid mixing rebuild-scheduling mechanisms.
Changes:
- Moved
provider.setSearchQuery('')calls out ofsetStatein both search-dismissal callbacks (clear button and search toggle). - Preserved the “clear only on collapse” behavior by hoisting the collapse condition into a local
isCollapsing. - Added a new widget test suite covering both dismissal paths and the expand-vs-collapse asymmetry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/screens/drinks_screen.dart | Refactors search dismissal callbacks to keep provider mutations out of setState while preserving behavior. |
| test/drinks_screen_search_dismiss_test.dart | Adds regression coverage for search dismissal (clear button + collapse toggle) and verifies expanding does not clear an existing query. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-526-setstate-provider.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
Fixes #526
What changed
lib/screens/drinks_screen.dart— both search-dismissal callbacks (the search-bar clear button and the search toggle) calledprovider.setSearchQuery('')from inside asetStateclosure.setSearchQuerycallsnotifyListeners(), which synchronously marks every watching element dirty. Doing that insidesetStatemixes two rebuild-scheduling mechanisms in one synchronous block — the shape that producessetState() or markNeedsBuild() called during buildas soon as anything upstream reorders.setStatenow mutates widget-local fields only, and the provider call runs after it.The toggle only clears when collapsing, so that condition is hoisted to a local (
isCollapsing) rather than dropped.Tests
Adds
test/drinks_screen_search_dismiss_test.dart. Neither dismissal path had coverage — the existing debounce and style-filter suites open the search bar but never close it.Three tests, asserting the user-visible outcome rather than just provider state: the bar closes, the query clears, and the unfiltered list returns. A third pins that expanding the bar leaves an existing query intact, which is the asymmetry the hoisted condition preserves.
They pass against both the old and the new implementation, which is the point — this is a latent-shape fix, so the tests exist to pin the refactor as behaviour-preserving rather than to reproduce a live failure.
./bin/mise run checkpasses: 1312 tests (1309 baseline + 3 new), analyzer clean.Generated by Claude Code