Skip to content

Release 0.1.1: fix the predictive back, the route keys and the observers - #1

Merged
PlugFox merged 8 commits into
masterfrom
fix/0.1.1
Oct 1, 2026
Merged

PlugFox merged 8 commits into
masterfrom
fix/0.1.1

Conversation

@PlugFox

@PlugFox PlugFox commented Oct 1, 2026

Copy link
Copy Markdown
Owner

Fixes the problems found by a review of the 0.1.0 release and bumps the version to 0.1.1.

Fixed

  • Predictive back on Android. A view with one route reported canHandlePop: false, so on Android 13+ the system closed the app and didPopRoute was never called. That also happened when a NavigationTabsController could return to the previous tab or the view had its own onBackButtonPressed. The view now claims the press in those cases. An inactive tab no longer reports the back button state for the visible one.
  • Route keys. The default key escapes %, & and = and writes a null argument without a value. Before this, {'q': 'a&b=c'} and {'q': 'a', 'b': 'c'} produced the same key, and so did null and 'null'.
  • Observers.
    • When a change happens during the build, observers are now notified at the end of the frame, so setState inside an observer no longer throws.
    • When one observer callback throws, that observer still gets its remaining events.
  • Controller.
    • A guard that returns an empty stack is now reported to FlutterError instead of hitting an assert, which also discarded the queued changes in debug builds.
    • pop and removeKey called from a listener are postponed until the current change is committed.
    • The result of a cancelled pop is dropped right away.
    • A guard can now dispose the controller safely.
  • Page cache. A page built from the view's context (locale, theme, an inherited scope) is rebuilt when that context changes.

Docs

  • A class route's name is minified in obfuscated builds.
  • Routes compare by instance in a plain List.
  • Overridden tags must keep kModalTag.
  • pushForResult completes the previous waiter of the same route with null.
  • controller.stack while the guards run.
  • A guard that keeps a route the user closed brings back a new instance of it.

CI and packaging

  • deploy.yml publishes only from the tag that matches the version, so a manual run from a branch now fails.
  • The pub cache key is computed from the pubspecs. It used the git-ignored pubspec.lock before, so the key never changed.
  • Removed the version rewrite in setup/action.yaml, which never ran.
  • Added .pubignore, so Makefile, .github and .vscode stay out of the package.
  • CHANGELOG: the entries already published with 0.1.0 (pub.dev showed them under "Unreleased") are moved under 0.1.0.

Behaviour changes

  • Default key format for arguments: name?id instead of name?id=null, and special characters are escaped.
  • Observer tests now expect the remaining callbacks to run after one throws.

Checks

  • flutter analyze --fatal-infos --fatal-warnings: no issues in the package or the example.
  • flutter test: package 194/194, example 11/11. Each new test fails on the old code.
  • flutter pub publish --dry-run: the only warning was the uncommitted files before this commit.

The back button: on Android with the predictive back gesture a view tells
the platform that the framework handles the press when its tabs can return
to the previous tab or it has its own onBackButtonPressed, otherwise the
application was closed without reaching didPopRoute. An inactive tab no
longer reports the state of the back button for the visible one.

The controller: observers of a change made during the build are notified
at the end of the frame, a failed observer callback no longer swallows the
remaining events, a guard returning an empty stack is reported instead of
asserting and dropping the queue, pop and removeKey from a listener are
postponed instead of being checked against the stack about to change, the
result of a cancelled pop is dropped right away, a guard may dispose the
controller.

The routes: the default key escapes %, & and = in the arguments and writes
a null argument without a value, so different arguments no longer collide.
A page built from the context of the view is rebuilt when it changes.

The CI publishes only from the tag of the version, the pub cache depends on
the pubspecs and a .pubignore keeps the development files out of the
package. The entries published with 0.1.0 are moved under 0.1.0.
Copilot AI balanced review requested due to automatic review settings October 1, 2026 15:02

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Predictive-back reporting has disabled and nested-view edge cases, and deferred notifications can produce a duplicate listener callback.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Prepares release 0.1.1 with navigation correctness fixes, documentation, tests, and packaging updates.

Changes:

  • Fixes predictive back, observer delivery, controller queuing, route keys, and page caching.
  • Adds regression coverage and API documentation.
  • Updates release metadata, packaging exclusions, and CI publishing safeguards.
File Description
lib/​src/​controller.dart Updates queuing, results, and observer delivery.
lib/​src/​view.dart Updates page caching and predictive-back reporting.
lib/​src/​tabs.dart Reports tab-group back state changes.
lib/​src/​route.dart Escapes default route keys and expands docs.
lib/​src/​guard.dart Documents guard lifecycle behavior.
test/​controller_test.dart Covers controller regressions.
test/​context_test.dart Covers deferred observer delivery.
test/​observer_edge_test.dart Updates callback-failure expectations.
test/​routes_edge_test.dart Covers escaped and null arguments.
test/​tabs_test.dart Covers predictive back with tabs and handlers.
test/​view_test.dart Covers context-dependent page rebuilding.
pubspec.yaml Bumps the package to 0.1.1.
CHANGELOG.md Records release fixes and reorganizes history.
.pubignore Excludes development-only package files.
.github/​workflows/​deploy.yml Requires publishing from a matching tag.
.github/​actions/​setup/​action.yaml Corrects cache inputs and removes version rewriting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/src/controller.dart
Comment thread lib/src/view.dart Outdated
The default key now escapes the name as well as the arguments, and
escapes "?" as %3F, so a separator inside the name can no longer
produce the key of another route: "a?b" with {"c": 1} and "a" with
{"b?c": 1} were both "a?b?c=1". The documentation of the key states
what is guaranteed: values are compared by their toString() and the
order of the arguments matters.

The pub cache key of the CI includes the Flutter version and the week,
restores a warm cache when a pubspec changes, and covers the
dependencies of the example. A comment in the deploy workflow notes
that a manual publish must be enabled on pub.dev.
…er and a dispose from a guard

- Drain the deferred observer events in place and keep the notification
  flag set during the end of frame dispatch, so a change made by an
  observer is reported after the pending events and a change made by a
  listener does not schedule a spurious notification next frame.
- maybePop called from a guard, an observer or a listener decides against
  the stack the pop applies to and falls back to the group.
- Stop running the guards once one of them disposes the controller, and
  do not subscribe a controller disposed during its initial validation to
  the revalidation.
- Do not notify the observers when a listener disposes the controller.
- Document the queued changes from a guard, the deferred observer events
  and the pushForResult waiter replacement.
On Android the predictive back gesture closes the application unless a
NavigationNotification with canHandlePop: true reaches it, so the view
must report exactly what didPopRoute would do.

- Every view swallows the notifications of its navigator, its routes and
  its nested views, and the top level views report one combined state:
  a visible nested view that can go back, a route to close, a PopScope
  blocking the visible route, a handler of the view or a previous tab.
  Two top level views (hand-made tabs) no longer overwrite each other.
- A nested view reports again when its route becomes current again.
- A disabled view (interceptBackButton: false) no longer claims the
  back button, and disabling it is reported.
- The tabs controller reports when it is created around mounted views
  and when it is disposed, so no stale claim of a previous tab remains.
- The pages are built with a context of their own, so a guard reading
  e.g. the size of the window no longer rebuilds every page.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Mounted-view maybePop handling and removal of the last navigation view can leave back behavior incorrect.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Documented route identity semantics conflict with custom equality

lib/​src/​route.dart:13

A plain List calls each element's operator ==; it only compares route instances when the concrete route class has not overridden equality. Since applications can implement == on their route classes, this new documentation currently promises identity semantics that the API cannot enforce. Qualify the statement so consumers are not directed to the controller helpers based on an incorrect premise.

Comment thread lib/src/controller.dart
Comment thread lib/src/view.dart
…raw the back button of the last view

- maybePop called from a guard, an observer or a listener with a mounted
  view waits for the change to be committed and the pages to be rebuilt,
  instead of asking a navigator that still shows the previous stack.
- Removing the last top level view reports the back button of the
  navigator it was shown in, so the platform does not keep the claim of
  a view that no longer exists.
- The documentation of NavigationStack no longer promises identity
  comparison for route classes that override ==.
@PlugFox

PlugFox commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Re the "previously missed" note on lib/src/route.dart:13: fixed in 3332bdd. The NavigationStack docs now say that NavigationRoute does not override ==, so the list operations compare instances only unless a route class overrides it, while the controller always identifies a route by its key.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Predictive-back reporting misses host navigator state, and listener-triggered disposal can leave controllers partially disposed.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Defer disposal until listener notification completes

lib/​src/​controller.dart:560

Disposing from a controller listener still calls ChangeNotifier.dispose() while notifyListeners() is active, which triggers ChangeNotifier's debug assertion. Because NavigationController.dispose() sets _disposed before calling super.dispose(), this return suppresses observers but leaves the controller only partially disposed; the new test also captures the resulting FlutterError without asserting that errors is empty. Defer the superclass disposal until listener notification has unwound, and make the test verify that no error was reported.

Medium severity Include host navigator state when reporting remaining views

lib/​src/​view.dart:381

This aggregate ignores the host navigator while any root view remains. If a one-route NavigationView is on route A of MaterialApp and the host navigator pushes an ordinary route B, the view becomes invisible and this reports false even though the host can pop B; that later report can disable predictive back for B. Include each root's host navigator state, as _reportNoViewLeft already does, and cover a plain host route pushed above a view.

A listener calling dispose made ChangeNotifier.dispose run while
notifyListeners was active, which ChangeNotifier reports as an error and
which left the controller half disposed. The controller is now marked as
disposed at once and ChangeNotifier.dispose runs when the notification
ends. The test now checks that no error is reported.

Also cover a plain route pushed by the host navigator above a view: the
host navigator replaces the false reported by the hidden view with its
own claim, so the press reaches it.
@PlugFox

PlugFox commented Oct 1, 2026

Copy link
Copy Markdown
Owner Author

Re the two "previously missed" notes of the review on 3332bdd:

  • lib/src/controller.dart:560, disposal from a listener: fixed in bd448b1. dispose marks the controller as disposed at once (detaches, completes the waiters, drops the deferred events) and, when called while notifyListeners is active, runs ChangeNotifier.dispose once the outermost notification returns. The test observers are not notified about a change after a listener has disposed the controller now asserts that no error is reported; it failed with the ChangeNotifier error before the fix.
  • lib/src/view.dart:381, host navigator state: not reproducible. A top level view dispatches its report from inside the route of the host navigator, and Navigator already handles a NavigationNotification coming from its routes: when it can pop, it replaces the value with its own canHandlePop: true. So with a plain route B pushed above a one-route view on route A, the platform keeps true even after the hidden view reports again. Added a plain route pushed above a view is reported in test/back_button_test.dart (push B, change the hidden view controller, back press pops B, then false); it passes on 3332bdd as well and stays as a regression test.

@PlugFox
PlugFox merged commit dde4b5d into master Oct 1, 2026
3 checks passed
@PlugFox
PlugFox deleted the fix/0.1.1 branch October 1, 2026 15:56
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