Skip to content

fix(ui_firestore): don't setState after dispose when fetching a page - #717

Open
mtallenca wants to merge 2 commits into
firebase:mainfrom
mtallenca:fix/query-builder-setstate-after-dispose
Open

mtallenca wants to merge 2 commits into
firebase:mainfrom
mtallenca:fix/query-builder-setstate-after-dispose

Conversation

@mtallenca

Copy link
Copy Markdown

Description

FirestoreQueryBuilder._listenQuery delays its setState with Future.microtask so that fetchMore can be called from a child's build (the documented pattern, e.g. from a ListView item builder). That build can be in the same frame that removes the FirestoreQueryBuilder — a list item scrolled out of the cache extent, or a route being torn down — so the microtask runs after dispose and throws:

setState() called after dispose(): _FirestoreQueryBuilderState<...>

In release builds this surfaces as Null check operator used on a null value in State.setState. We see it in production on web (Flutter wasm), from query_builder.dart _listenQuery.

This PR checks mounted in the microtask. The snapshots() listener's own setState calls don't need it, since dispose cancels that subscription.

Test: a FirestoreQueryBuilder in a ListView whose builder calls fetchMore; a snapshot arrives and the list is scrolled in the same frame so the item is dropped during layout. It fails without the fix with the error above and passes with it.

Related Issues

None found.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • All unit tests pass (melos run test:unit:all doesn't fail).
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

_listenQuery delays its setState with a microtask so that fetchMore can be
called from a child's build. That build can be in the frame that removes the
FirestoreQueryBuilder (a list item scrolled away, a route torn down), so the
microtask runs after dispose and throws.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request ensures that setState is only called if the widget is still mounted by adding a mounted check inside the delayed Future.microtask callback, and adds a widget test to cover this scenario. The review feedback suggests correcting a typo in the code comment, changing fetchNextpage to fetchMore to align with the public API.

Comment thread packages/firebase_ui_firestore/lib/src/query_builder.dart Outdated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.

1 participant