test(routing): pin queryParameter, which a report claimed was broken - #147
Merged
Conversation
A consumer put `?scale=5000` on a route, could not read it back, and filed it
against this package. The evidence was `dusk:get_routes` reporting
`location: /` while the browser sat on `#/?scale=5000`.
That evidence measures something else. Dusk's `location` is
`route.settings.name` off the Navigator (`ext_navigation.dart:442`), which
under `MaterialApp.router` is the declared path pattern, not the resolved URI.
It was always going to read `/` and says nothing about what this router saw.
These six tests settle it, and all six passed on the first run: the router
reads a query parameter correctly, both when it is passed through the
`queryParameters` argument and when it is written inline in the path, which is
the shape `to('/?scale=5000')` produces and the one the report used. So there
is nothing to fix here, and the report is withdrawn.
They stay because the API had no regression test at all, and because the next
person who doubts it should be able to settle it in one command rather than by
reasoning about GoRouter. The fourth one is the interesting case: a later
navigation with no query has to CLEAR the value rather than leave a stale one a
caller would read as current.
Tests only, no `lib/` change, so the post-change sync does not apply and there
is no CHANGELOG entry: nothing about the behaviour moved.
anilcancakir
added a commit
to anilcancakir/watchools
that referenced
this pull request
Sep 7, 2026
The comment blamed `MagicRouter.queryParameter` for not seeing the query, on the evidence that `dusk:get_routes` reported `location: /` while the browser sat on `#/?scale=5000`. That field is `route.settings.name` off the Navigator (`ext_navigation.dart:442`), which under `MaterialApp.router` is the declared path pattern; it was always going to read `/` and said nothing about the router. Six tests in `magic` now pin `queryParameter`, inline-query form included, and all six passed on the first run (`fluttersdk/magic#147`). The report is withdrawn. `Uri.base` stays, for the reason that was actually true: this is read from a controller's field initialiser, which can run before any route has resolved, and the router only records a location inside `pageBuilder`.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Test-only addition that does what it claims: the six cases pass locally, the accessors genuinely had no prior coverage, and the file follows the conventions of its sibling I verified the "no existing coverage" claim: I also confirmed the Minor
TestsThis is the tests. Six cases covering both No Checks I ran
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ation Three points from review. One test name read backwards from what it asserts. "survives a navigation that carries no query at all" says the value persists; the body, the inline comment and the pull request description all say it is cleared, which is the behaviour worth pinning. Renamed to say so. The pre-resolution case pumped nothing and took a `tester` it never used. `.claude/rules/tests.md:18` asks for `test()` for pure logic, and the sibling file already does exactly that for its own no-route-yet case. Now a `test()`. `Request.query` and `Request.queryParams` (`request.dart:72` and `:83`) are one-line delegations to the two accessors this file covers, and nothing tested them either. They are the surface a consumer actually reads, and reading through them is what the original report was doing, so the pairing is the part worth having under test rather than the one-liners themselves.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Both minors from the last pass are fixed and the Since TestsSeven cases now: six on the router accessors, one on the Checks I ran
|
anilcancakir
added a commit
to anilcancakir/watchools
that referenced
this pull request
Sep 7, 2026
…rove (#5) * fix(devtools): install the frame-perf bridge and open the gates to profile `dusk:perf_end` refused every session with `liveness advanced 0` and blamed a backgrounded page. The app rendered, screenshots came back full, and activating the Chrome target changed nothing: the counter the refusal is computed from had never been wired. Dusk cannot depend on telescope, magic or wind (`perf_readers.dart` states that frozen contract), so it declares a `framePerfReader` defaulting to an empty frame list and a zero liveness counter. `magic_devtools` is the one place all four are visible and is therefore the only place that pointer can be assigned, which `MagicPerfIntegration.install()` does. This app hand-rolled five install calls and that was not among them. Replaced with `MagicDevtools.installPre()` and `installPost()`, the package's own halves. The split is not cosmetic: the perf integration registers a `NavigatorObserver` and `MagicRouter.addObserver` throws a `StateError` once the router is built, so it has to run before `Magic.init`. The gates move from `kDebugMode` to `!kReleaseMode` so a profile build carries the tooling. A debug build's numbers rank causes; they do not describe a device, and the profile build is the only one worth quoting. Release is unchanged. * test(perf): add a provider-scale fixture and a frame-measurement harness The hand-written fixtures are 23 channels and 15 titles. That is the right size for judging a design and useless for judging a frame: every lazy-list question this app has is invisible below about a thousand rows, and a real Xtream playlist is hundreds of `group-title` values and five figures of channels. `ScaleFixture` generates a line-up and a catalogue of any size, deterministically (every value is a pure function of the item's index, so two runs a week apart produce byte-identical data) and with the missing-data shares that decide the layout rather than a full house: two in five channels carry no EPG, one in three no logo, three in ten titles no poster. No network URLs in it. The first version pointed at `picsum.photos`, so every measurement raced hundreds of live fetches and decodes against the frames it was timing, which is the most likely single cause of the run-to-run variance that swamped the effect size of every change. The cost is that this fixture cannot measure image memory; that needs its own harness. `FixtureScale` reads `?scale=N` off `Uri.base` and clamps it. Not through `MagicRouter.queryParameter`, which reported the location as `/` while the browser sat on `#/?scale=5000`. Not a `--dart-define`, because `fsa start` assembles a fixed argv with no passthrough. Not an `.env` key, because `.env` is a real asset during `flutter test` and a scale left there would silently generate five thousand channels under every widget test. `tool/dusk/perf.sh` drives the sessions. Its header says which of its own numbers to believe and why, and the short version is that the build COUNTS are the instrument and the milliseconds are not: the same code measured 50 ms and 132 ms on one session with byte-identical build counts on both sides. The block attribution's micros are not printed at all, because they are nested (one frame's blocks summed to twenty one times the frame's own build time), so ranking by them ranks by tree depth. * perf(catalogue): memoise the two getters Vitrin asks for three times a build `continueWatching` walked the whole catalogue, and for a series its episode list, on every read. `Vitrin` reads it three times in one build: to decide whether the resume rail exists, for the hero's fallback, and for the rail's own contents. `noArtworkNote` walked it again for the toolbar's count. Both join the frame-cache set the controller already keeps for `matches` and `sections`, and both are dropped by the same `_invalidate`. Unmeasured, and the harness could not have measured it: the cost is Dart list walking, which does not appear in the build counts that are the only reliable signal here. It is sound by construction rather than by evidence, which is the honest description. Also picks up the generated fixture through `FixtureScale`. * perf(ui): give the rails a fixed extent and stop building keep-alives Three changes to `Rail`, measured on the catalogue's vertical session at scale 5000 against the same session before them. `itemExtent` from a new required `itemWidth`. Every caller already had the number: it is the same width it hands the tile. A `SliverFixedExtentList` does not lay a child out to learn its extent; a plain `SliverList` does, on every child, on every scroll. The inter-card gap moves from a `ListView.separated` separator onto a `Padding` on the item. A separator is a full delegate child slot, mounted and bounded like a card: `KeyedSubtree` 234 to 150 and `RepaintBoundary` 234 to 150, over a run that drew more frames than the one before it. `addAutomaticKeepAlives: false`. No card in this app keeps itself alive, so the wrapper is a widget per card that can never do anything, and it is the one mechanism that could pin a whole rail and its scroll position alive after it scrolled out of the vertical list. `AutomaticKeepAlive` and `_SelectionKeepAlive` both 234 to zero. The cast rail is the one that had to be handled by hand. It was the only rail whose item was not already wrapped in a `SizedBox`, and the tidy-looking fix was to expose a width constant on `PersonCircle` beside its existing height. The only constant that existed was the CIRCLE's 88 pixel diameter, while the cell renders at 104. A fixed-extent sliver hands its child a TIGHT main-axis constraint, so that would have squeezed every cast cell by sixteen pixels with `shrink-0` unable to argue. It now writes 104 in a `SizedBox` exactly as the other five rails do, and the constant is not exposed. Not visible to any gate: `perf.sh` never opens a title, and the widget tests ignore overflow because the test font is square. Verified from a screenshot, where the cells sit 112 apart, which is 104 plus the 8 pixel gap. * perf(ui): build the category strips lazily Both strips were `ListView(children: [for ...])`. The reason to change that is narrower than it first looks, and the first version of this commit message had it wrong: `SliverChildListDelegate.build` is `children[index]`, so only mounted indices are ever BUILT and the measured `WDiv` build count was identical either way. What the list form does is allocate a widget object per group every time the method runs, and it runs on every controller notify, which means every keystroke. At two hundred groups that is two hundred allocations per character for the thirteen chips anyone can see. Below the harness's noise floor at this scale, and kept anyway: it is the shape that does not degrade when a provider sends eight hundred groups, which several do. * docs: record the scale harness and which of its numbers to believe * docs: correct the reason this reads the URL rather than the router The comment blamed `MagicRouter.queryParameter` for not seeing the query, on the evidence that `dusk:get_routes` reported `location: /` while the browser sat on `#/?scale=5000`. That field is `route.settings.name` off the Navigator (`ext_navigation.dart:442`), which under `MaterialApp.router` is the declared path pattern; it was always going to read `/` and said nothing about the router. Six tests in `magic` now pin `queryParameter`, inline-query form included, and all six passed on the first run (`fluttersdk/magic#147`). The report is withdrawn. `Uri.base` stays, for the reason that was actually true: this is read from a controller's field initialiser, which can run before any route has resolved, and the router only records a location inside `pageBuilder`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The report, and why it was wrong
A consumer put
?scale=5000on a route, could not read it back, and filed it here. The evidence wasdusk:get_routesprintinglocation: /while the browser sat onhttp://localhost:3210/#/?scale=5000.That evidence measures something else. Dusk's
locationisroute.settings.nametaken off the Navigator (fluttersdk_dusk,lib/src/extensions/ext_navigation.dart:442), which underMaterialApp.routeris the declared path pattern rather than the resolved URI. It was always going to read/, whatever this router had seen.All six tests below passed on the first run.
MagicRouter.queryParameterreads the query correctly, including from the inline formto('/?scale=5000')that the report used. There is nothing to fix and the report is withdrawn.Why keep the tests
queryParameterandqueryParametershad no regression test anywhere intest/. The next person who doubts them should be able to settle it with one command instead of reasoning about how GoRouter parses a location.Six cases:
queryParametersargumentto('/?scale=5000'))queryParametersexposes every pairqueryParametersis empty rather than null before any route resolvesGates
dart format .: 334 files, 0 changed.dart analyzeon the new file: no issues.flutter test: 1426 passed, 6 of them new.Tests only, no change under
lib/, so the post-change sync does not apply and there is no CHANGELOG entry: nothing about the behaviour moved.