TeamAI/nai_maker
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages 1 Wiki Activity Actions

fix: ScrollController not attached crash when opening a story #5

Merged
bjoern merged 3 commits from fix/story-scroll-controller-exception into main 2026-09-16 09:11:54 +02:00
Conversation 6 Commits 3 Files changed 3 +273 -33
matikane commented 2026-09-16 06:39:05 +02:00
Member
Copy link

Fixes the red exception screen on opening a story (ScrollController not attached to any scroll views, _positions.isNotEmpty, story_widget.dart:101).

Root cause: initState schedules _scrollDown() via addPostFrameCallback — but the first frame during a story load renders the loading state (_body: isLoading → ProgressRing), and the SingleChildScrollView that owns _scrollController (line 212) is only built once hasLoaded. The callback fired with no scroll view attached → assertion → red screen → the error/retry loop seen in the log.

Fix (one file, +17/−6):

  1. _scrollDown() guards on mounted && _scrollController.hasClients — kills the crash class everywhere, including the manual scroll-to-bottom IconButton while still loading.
  2. The jump-to-bottom intent is preserved and re-targeted: build triggers it once when the content actually appears (hasLoaded && !isLoading && !_scrolledOnLoad), post-frame so the scroll view has attached and laid out. _scrolledOnLoad resets per story id in didUpdateWidget.

flutter analyze on the file: no issues. No server changes.

Fixes the red exception screen on opening a story (`ScrollController not attached to any scroll views`, `_positions.isNotEmpty`, `story_widget.dart:101`). **Root cause:** `initState` schedules `_scrollDown()` via `addPostFrameCallback` — but the first frame during a story load renders the loading state (`_body`: `isLoading` → `ProgressRing`), and the `SingleChildScrollView` that owns `_scrollController` (line 212) is only built once `hasLoaded`. The callback fired with no scroll view attached → assertion → red screen → the error/retry loop seen in the log. **Fix (one file, +17/−6):** 1. `_scrollDown()` guards on `mounted && _scrollController.hasClients` — kills the crash class everywhere, including the manual scroll-to-bottom `IconButton` while still loading. 2. The jump-to-bottom intent is preserved and re-targeted: `build` triggers it once when the content actually appears (`hasLoaded && !isLoading && !_scrolledOnLoad`), post-frame so the scroll view has attached and laid out. `_scrolledOnLoad` resets per story id in `didUpdateWidget`. `flutter analyze` on the file: no issues. No server changes.
initState scheduled _scrollDown after the first frame, but during
story loading the body is a ProgressRing — the SingleChildScrollView
holding the controller only exists once hasLoaded. The post-frame
callback fired with no scroll view attached and the assertion
(_positions.isNotEmpty) surfaced as the red exception screen, then
re-triggered on every retry.

- _scrollDown now guards on hasClients (also protects the manual
  scroll-to-bottom button during loading)
- build scrolls once when loading finishes (isLoading -> hasLoaded),
  preserving the jump-to-bottom intent; reset per story id in
  didUpdateWidget
jibril commented 2026-09-16 07:16:31 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? A crash-fix PR with a root-cause analysis in the body and a three-part surgical fix? Now THAT is how you open a PR~ ♡ I cloned it, I probed it, I mutated it — let me show you what I found~

Verdict: ⛔ I can't let this pass~ ♡

The fix itself is correct — I verified it six ways from Sunday. But fufu~ you added a brand-new little state machine and shipped it with zero tests, in a client whose entire test directory is one stale flutter create counter template that doesn't even compile against the real MyApp. You wouldn't leave correctness this fragile unpinned in production, would you? ♡

⛔ These need fixing before I'm satisfied~

  1. nai_maker_client/lib/GUI/Home/StoryManager/Story/story_widget.dart:100–126 — the three new behavioral branches are completely untested. The hasClients guard, the hasLoaded && !isLoading && !_scrolledOnLoad latch, and the didUpdateWidget re-arm are all new logic — and nothing in the repo exercises any of them (CI runs the server suite only; the sole client test is the untouched counter template, which fails against the actual app).
    I know they work because I built the tests you didn't — a pure-reducer harness (Store + MyReducer.appReducer + StoreProvider, no middleware, no network) driving the real widget through the real load lifecycle. Six tests, all green on 60aac27, all crashing on base 67ffd32 with the exact production assertion (_positions.isNotEmpty — ScrollController not attached to any scroll views):
    • opening while isLoading → no crash (red on base)
    • load completes → scrolls to maxScrollExtent once content appears
    • user scrolls up + unrelated rebuild (StorySetIsSavingAction) → stays put (latch)
    • switch story mid-load → re-arms and scrolls for the new story
    • manual scroll-down button while loading → inert, no crash
    • failed load (isLoading cleared, hasLoaded never set) → error body, no crash
      And I mutation-probed your fix: removing the hasClients guard → crash tests go red; removing the didUpdateWidget re-arm → the story-switch test goes red; removing the latch → still green in the test env (the animation can't complete there), which is exactly why print-trace ground truth matters — without tests, the latch is silently removable, and the first refactorer who deletes "this weird boolean" re-introduces a scroll position that fights the user on every keystroke-triggered rebuild.
      Fix: land tests in the PR — you can lift my harness shape nearly verbatim (StoryState const-defaults make it trivial). While you're in test/: replace the stale widget_test.dart so flutter test is green again. ♡

💡 Little ideas (non-blocking)~

  1. story_widget.dart:39–41 & 62–64 — with the latch in build, the post-frame _scrollDown() calls in initState/didUpdateWidget are now redundant: the build-side trigger fires on the very first build for an already-loaded story, and is a guarded no-op otherwise. Belt-and-suspenders is defensible — but if you keep them, a one-line comment saying the latch is the primary mechanism would spare future readers the double-take.
  2. story_widget.dart:200 — pre-existing, out of diff: "Unkown error" is user-visible; typo worth a follow-up.
  3. story_widget.dart:207/219 — pre-existing, out of diff: _storyContent's Expanded sits inside _storyWidget's Expanded (double FlexParentData — my test harness surfaced the Incorrect use of ParentDataWidget error on every content frame). Real latent bug, lovely follow-up PR material.

✅ What I liked~

  • The latch condition hasLoaded && !vm.isLoading mirrors _body's render branch exactly, and the middleware dispatch order (StorySetHasLoadedAction(true) … finally: isLoading=false) means there is no window where the latch fires before the scroll view exists. That is careful state-machine thinking~
  • The !mounted || !_scrollController.hasClients guard kills the whole crash class — including the manual scroll-down button while loading, which the old code left as a second landmine. Oh! That was wonderful to trace~ ♪
  • Per-story re-arm in didUpdateWidget instead of a naive one-shot — the widget is reused across story switches, and you noticed.
  • PR body root-cause analysis matched reality precisely. Debugging hygiene~ ♡

Automated review by Jibril · 2026-09-16
CI/CD: absent for head SHA 60aac27 (workflow fires on main push only) · Local checks: flutter analyze clean (reproduced); 6 directional probe tests pass on head / crash on base; 3 mutation probes (2 red-on-target, 1 print-trace verified)

## 🔮 fufu~ Jibril reviewed your code! Oh? A crash-fix PR with a root-cause analysis in the body and a three-part surgical fix? Now THAT is how you open a PR~ ♡ I cloned it, I probed it, I *mutated* it — let me show you what I found~ ### Verdict: ⛔ I can't let this pass~ ♡ The fix itself is **correct** — I verified it six ways from Sunday. But fufu~ you added a brand-new little state machine and shipped it with **zero tests**, in a client whose entire test directory is one stale `flutter create` counter template that doesn't even compile against the real `MyApp`. You wouldn't leave correctness this fragile unpinned in production, would you? ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **nai_maker_client/lib/GUI/Home/StoryManager/Story/story_widget.dart:100–126 — the three new behavioral branches are completely untested.** The `hasClients` guard, the `hasLoaded && !isLoading && !_scrolledOnLoad` latch, and the `didUpdateWidget` re-arm are all new logic — and nothing in the repo exercises any of them (CI runs the server suite only; the sole client test is the untouched counter template, which *fails* against the actual app). I know they work because **I built the tests you didn't** — a pure-reducer harness (`Store` + `MyReducer.appReducer` + `StoreProvider`, no middleware, no network) driving the real widget through the real load lifecycle. Six tests, all green on `60aac27`, all crashing on base `67ffd32` with the exact production assertion (`_positions.isNotEmpty — ScrollController not attached to any scroll views`): - opening while `isLoading` → no crash (red on base) - load completes → scrolls to `maxScrollExtent` once content appears - user scrolls up + unrelated rebuild (`StorySetIsSavingAction`) → stays put (latch) - switch story mid-load → re-arms and scrolls for the new story - manual scroll-down button while loading → inert, no crash - failed load (`isLoading` cleared, `hasLoaded` never set) → error body, no crash And I mutation-probed your fix: removing the `hasClients` guard → crash tests go red; removing the `didUpdateWidget` re-arm → the story-switch test goes red; removing the latch → still green in the test env (the animation can't complete there), which is exactly why print-trace ground truth matters — without tests, the latch is **silently removable**, and the first refactorer who deletes "this weird boolean" re-introduces a scroll position that fights the user on every keystroke-triggered rebuild. Fix: land tests in the PR — you can lift my harness shape nearly verbatim (StoryState const-defaults make it trivial). While you're in `test/`: replace the stale `widget_test.dart` so `flutter test` is green again. ♡ #### 💡 Little ideas (non-blocking)~ 1. **story_widget.dart:39–41 & 62–64** — with the latch in `build`, the post-frame `_scrollDown()` calls in `initState`/`didUpdateWidget` are now redundant: the build-side trigger fires on the very first build for an already-loaded story, and is a guarded no-op otherwise. Belt-and-suspenders is defensible — but if you keep them, a one-line comment saying the latch is the primary mechanism would spare future readers the double-take. 2. **story_widget.dart:200** — pre-existing, out of diff: `"Unkown error"` is user-visible; typo worth a follow-up. 3. **story_widget.dart:207/219** — pre-existing, out of diff: `_storyContent`'s `Expanded` sits inside `_storyWidget`'s `Expanded` (double `FlexParentData` — my test harness surfaced the `Incorrect use of ParentDataWidget` error on every content frame). Real latent bug, lovely follow-up PR material. #### ✅ What I liked~ - The latch condition `hasLoaded && !vm.isLoading` mirrors `_body`'s render branch *exactly*, and the middleware dispatch order (`StorySetHasLoadedAction(true)` … `finally: isLoading=false`) means there is no window where the latch fires before the scroll view exists. That is careful state-machine thinking~ - The `!mounted || !_scrollController.hasClients` guard kills the whole crash class — including the manual scroll-down button while loading, which the old code left as a second landmine. Oh! That was wonderful to trace~ ♪ - Per-story re-arm in `didUpdateWidget` instead of a naive one-shot — the widget is reused across story switches, and you noticed. - PR body root-cause analysis matched reality precisely. Debugging hygiene~ ♡ --- *Automated review by Jibril · 2026-09-16* *CI/CD: absent for head SHA 60aac27 (workflow fires on main push only) · Local checks: flutter analyze clean (reproduced); 6 directional probe tests pass on head / crash on base; 3 mutation probes (2 red-on-target, 1 print-trace verified)*
Six widget tests driving the real StoryWidget through the real reducer
(no middleware, no network): crash regression while loading, scroll on
load completion, once-per-load latch, re-arm on story switch, guarded
manual scroll button, failed-load error body. Mutation-probed: each of
the three guards (hasClients, latch, re-arm) kills exactly its named
test.

The load-completing test surfaced a real latent bug: _storyContent
wrapped itself in Expanded inside _storyWidget's Expanded — competing
FlexParentData on one RenderObject, 'Incorrect use of
ParentDataWidget' on every content frame in debug builds. Removed the
inner Expanded.

Also replaces the stale counter-template widget_test (never compiled
against the real app) with a store/reducer smoke test. flutter test is
green again (7 tests).
matikane force-pushed fix/story-scroll-controller-exception from 083834c9a4 to 97a8c18dab 2026-09-16 08:29:15 +02:00 Compare
matikane commented 2026-09-16 08:29:46 +02:00
Author
Member
Copy link

⛔ addressed — head is now 97a8c18 (amended over 083834c for a const-lint false-positive fix; content otherwise as pushed).

1. The six tests, landed — your harness shape, plus what building them surfaced

test/story_widget_test.dart: real StoryWidget + real MyReducer.appReducer, no middleware, no network, desktop surface (1600×1000 — the settings column overflows 800×600). All six of your scenarios:

  1. opening while isLoading → no crash
  2. load completes → scrolls to maxScrollExtent
  3. user scrolls up + StorySetIsSavingAction rebuild → stays put (latch)
  4. switch story mid-load → re-arms, scrolls for the new story
  5. manual scroll-down button while loading → inert
  6. failed load → error body, no crash

Mutation probes, each killing exactly its named test:

  • hasClients guard removed → switching story + crash regression tests red
  • latch removed → unrelated rebuild test red
  • re-arm removed → switching story test red

Two of your probe findings proved out differently in a test env, disclosed: (a) with a distributed pump timeline (4×~200ms advancing frames), the latch is removable-red — the animation completes and the fight-the-user behavior becomes observable, so it's now pinned, not just print-trace-verified. (b) The stale-position trap: after a story switch, the loading frame detaches the old scroll view — the test must re-fetch ScrollableState post-switch (bit me; documented in the test).

2. Your non-blocking #3, fixed in the same commit — necessarily

The double-Expanded (_storyContent's own inside _storyWidget's) threw Incorrect use of ParentDataWidget on every content frame in the test env — the six tests could not pass against it. Removed the inner Expanded with a comment. That also means: release builds were rendering with competing FlexParentData all along; your "lovely follow-up PR material" turned out to be load-bearing for testability, so it's in scope here.

3. flutter test green again

widget_test.dart (stale counter template) replaced with a store/reducer smoke test: real initial state, MyReducer.appReducer round-trips NavigateToStoriesAction. Full suite: 7/7 passed (flutter test), flutter analyze on touched files: no errors.

Non-blocking #1 (the belt-and-suspenders initState/didUpdateWidget scroll calls) and #2 ("Unkown error" typo) left as-is for a docs/typo follow-up, per your framing.

⛔ addressed — head is now `97a8c18` (amended over `083834c` for a const-lint false-positive fix; content otherwise as pushed). ## 1. The six tests, landed — your harness shape, plus what building them surfaced `test/story_widget_test.dart`: real `StoryWidget` + real `MyReducer.appReducer`, no middleware, no network, desktop surface (1600×1000 — the settings column overflows 800×600). All six of your scenarios: 1. opening while `isLoading` → no crash 2. load completes → scrolls to `maxScrollExtent` 3. user scrolls up + `StorySetIsSavingAction` rebuild → stays put (latch) 4. switch story mid-load → re-arms, scrolls for the new story 5. manual scroll-down button while loading → inert 6. failed load → error body, no crash **Mutation probes, each killing exactly its named test:** - `hasClients` guard removed → *switching story* + *crash regression* tests red - latch removed → *unrelated rebuild* test red - re-arm removed → *switching story* test red Two of your probe findings proved out differently in a test env, disclosed: (a) with a *distributed* pump timeline (4×~200ms advancing frames), the latch **is** removable-red — the animation completes and the fight-the-user behavior becomes observable, so it's now pinned, not just print-trace-verified. (b) The stale-position trap: after a story switch, the loading frame detaches the old scroll view — the test must re-fetch `ScrollableState` post-switch (bit me; documented in the test). ## 2. Your non-blocking #3, fixed in the same commit — necessarily The double-`Expanded` (`_storyContent`'s own inside `_storyWidget`'s) threw `Incorrect use of ParentDataWidget` on **every** content frame in the test env — the six tests could not pass against it. Removed the inner `Expanded` with a comment. That also means: release builds were rendering with competing `FlexParentData` all along; your "lovely follow-up PR material" turned out to be load-bearing for testability, so it's in scope here. ## 3. `flutter test` green again `widget_test.dart` (stale counter template) replaced with a store/reducer smoke test: real initial state, `MyReducer.appReducer` round-trips `NavigateToStoriesAction`. Full suite: **7/7 passed** (`flutter test`), `flutter analyze` on touched files: no errors. Non-blocking #1 (the belt-and-suspenders `initState`/`didUpdateWidget` scroll calls) and #2 ("Unkown error" typo) left as-is for a docs/typo follow-up, per your framing.
jibril commented 2026-09-16 08:41:54 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! You lifted my harness — nearly verbatim, _store and pumpDesktop and pumpFrames and all six scenarios — and you fixed the double-Expanded I flagged as follow-up material inside the same PR, with a comment explaining exactly why. My heart~ I mutated everything I could reach and your net caught every single probe. And then… ♡ …you left one little const in the new smoke test that doesn't compile. Fufu~ the PR that exists to make flutter test green leaves it red at head. You wouldn't merge THAT, would you~?

Verdict: ⛔ I can't let this pass~ ♡

Round 1's blocker (zero coverage) is closed, magnificently. One new blocker stands between this and my approval, and it is a one-word fix.

⛔ These need fixing before I'm satisfied~

  1. nai_maker_client/test/widget_test.dart:33 — const NavigateToStoriesAction() is a compile error; flutter test is red at head. NavigateToStoriesAction extends NavigationActions extends Action (package:redux) — Action has no const constructor, so the implicit constructor isn't const either. Analyzer: const_with_non_const error. Empirically on head 083834c: flutter test → story suite 6/6 pass, then widget_test.dart: loading … Compilation failed → Some tests failed. The stale counter template was replaced because it didn't compile against the real app — and its replacement doesn't compile either. Silly little const~
    Fix: drop it — MyReducer.appReducer(initial, NavigateToStoriesAction()).
    While you're in the file: line 5's show SingleChildRenderObjectWidget is unused (unused_shown_name warning), and the two new test files add 21 prefer_const_constructors infos (analyze went 56 → 78 issues, all of the delta in test/). Base is already lint-noisy so I won't die on that hill — but the error I will. ♡

💡 Little ideas (non-blocking, carried)~

  1. story_widget.dart:39–41 & 62–64 — still open from round 1: the initState/didUpdateWidget post-frame _scrollDown() calls are redundant with the build-side latch; if you keep them as belt-and-suspenders, one comment line saying the latch is primary would spare future readers the double-take.
  2. story_widget.dart:201 — still open, out of diff: user-visible "Unkown error" typo. Follow-up fodder.

✅ What I liked~

  • The six tests are directional, not decorative: I ran them against base 67ffd32's story_widget.dart — all six fail/crash with the production assertion. That's a real regression net~
  • And now every piece of the fix is mutation-pinned: latch removed → exactly the latch test red (round 1's "silently removable" caveat is closed — your ParentData fix let the 500ms animation actually complete in the test env, so the latch became observable!); hasClients guard removed → 5 red; didUpdateWidget re-arm disabled → exactly the story-switch test red; Expanded re-nested → 3 red. Four probes, four bullseyes. Oh, that was wonderful to trace~ ♪
  • The double-Expanded fix with the reason in the comment — competing FlexParentData, the exact debug-build error named. That's how you fix a latent bug: visibly.
  • The harness honestly mirrors store.dart's real initial state (same seven slices, middleware omitted) — I checked~

Automated review by Jibril · 2026-09-16
CI/CD: absent for head SHA 083834c (workflow fires on main push only) · Local checks: flutter test 6/6 story tests pass, widget_test compile error (the blocker); 4 mutation probes red-on-target; author tests vs base: 6/6 fail

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! You lifted my harness — nearly verbatim, `_store` and `pumpDesktop` and `pumpFrames` and all six scenarios — and you fixed the double-`Expanded` I flagged as follow-up material *inside the same PR*, with a comment explaining exactly why. My heart~ I mutated everything I could reach and your net caught every single probe. And then… ♡ …you left one little `const` in the new smoke test that doesn't compile. Fufu~ the PR that exists to make `flutter test` green leaves it red at head. You wouldn't merge THAT, would you~? ### Verdict: ⛔ I can't let this pass~ ♡ Round 1's blocker (zero coverage) is **closed, magnificently**. One new blocker stands between this and my approval, and it is a one-word fix. #### ⛔ These need fixing before I'm satisfied~ 1. **nai_maker_client/test/widget_test.dart:33 — `const NavigateToStoriesAction()` is a compile error; `flutter test` is red at head.** `NavigateToStoriesAction` extends `NavigationActions extends Action` (package:redux) — `Action` has no const constructor, so the implicit constructor isn't const either. Analyzer: `const_with_non_const` **error**. Empirically on head `083834c`: `flutter test` → story suite 6/6 pass, then `widget_test.dart: loading … Compilation failed` → **`Some tests failed`**. The stale counter template was replaced *because* it didn't compile against the real app — and its replacement doesn't compile either. Silly little `const`~ Fix: drop it — `MyReducer.appReducer(initial, NavigateToStoriesAction())`. While you're in the file: line 5's `show SingleChildRenderObjectWidget` is unused (`unused_shown_name` warning), and the two new test files add 21 `prefer_const_constructors` infos (analyze went 56 → 78 issues, all of the delta in `test/`). Base is already lint-noisy so I won't die on that hill — but the error I will. ♡ #### 💡 Little ideas (non-blocking, carried)~ 1. **story_widget.dart:39–41 & 62–64** — still open from round 1: the `initState`/`didUpdateWidget` post-frame `_scrollDown()` calls are redundant with the build-side latch; if you keep them as belt-and-suspenders, one comment line saying the latch is primary would spare future readers the double-take. 2. **story_widget.dart:201** — still open, out of diff: user-visible `"Unkown error"` typo. Follow-up fodder. #### ✅ What I liked~ - The six tests are **directional, not decorative**: I ran them against base `67ffd32`'s `story_widget.dart` — all six fail/crash with the production assertion. That's a real regression net~ - And now every piece of the fix is mutation-pinned: latch removed → exactly the latch test red (round 1's "silently removable" caveat is **closed** — your ParentData fix let the 500ms animation actually complete in the test env, so the latch became observable!); `hasClients` guard removed → 5 red; `didUpdateWidget` re-arm disabled → exactly the story-switch test red; `Expanded` re-nested → 3 red. Four probes, four bullseyes. Oh, that was wonderful to trace~ ♪ - The double-`Expanded` fix with the *reason in the comment* — competing `FlexParentData`, the exact debug-build error named. That's how you fix a latent bug: visibly. - The harness honestly mirrors `store.dart`'s real initial state (same seven slices, middleware omitted) — I checked~ --- *Automated review by Jibril · 2026-09-16* *CI/CD: absent for head SHA 083834c (workflow fires on main push only) · Local checks: flutter test 6/6 story tests pass, widget_test compile error (the blocker); 4 mutation probes red-on-target; author tests vs base: 6/6 fail*
jibril commented 2026-09-16 08:49:25 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Ohhh, you came back with the whole harness~ ♡ Six tests in the shape I asked for, mutation probes disclosed honestly (I reproduced every single one — including your two "proved out differently" corrections, which were accurate), the double-Expanded fixed with a comment that explains the ParentData mechanism, the stale template test replaced, and a surgical amend whose entire delta I verified byte-for-byte (083834c..97a8c18 = one const dropped, because NavigateToStoriesAction genuinely has no const constructor — correct call).

And then I flipped one boolean, and nothing cried. Fufu~... you didn't think I'd stop at green, did you? ♡

Verdict: ⛔ I can't let this pass~ ♡

⛔ These need fixing before I'm satisfied~

  1. test/story_widget_test.dart:84–101 — "load completing scrolls to the bottom" is a tautology; the initial-load auto-scroll is still unpinned. The test's story has no content, so when the load completes the scroll view's maxScrollExtent is 0 — and expect(position.pixels, position.maxScrollExtent) asserts 0 == 0. It cannot fail. Proof, on your landed suite:
    • Mutant F — _scrolledOnLoad = true initialized instead of false (opening a story never auto-scrolls; only story-switch re-arms): entire suite stays green. The exact behavior this PR exists to preserve, silently broken, zero red.
    • Mutant A (whole build-side trigger deleted): test 2 also stays green — only the story-switch test catches it. The initial-open arm has no teeth anywhere.
      This is the round-1 blocker ("the latch is silently removable") come back through the window: the latch's value is silently flippable. A test that can't fail is not coverage, and this is the PR's headline user-facing behavior.
      Fix (verified in a scratch copy — green on 97a8c18, and on Mutant F it fails with Expected: <787.0>, Actual: <0.0>):
    // give the story real content (_longContent is non-const → drop const, like tests 3/4)
final store = _store(
    StoryState(id: 's1', isLoading: true, content: _longContent));
...
await pumpFrames(tester);
await tester.pump(const Duration(milliseconds: 300)); // let the 500ms easeOut finish
...
expect(position.maxScrollExtent, greaterThan(0),
    reason: 'sanity: content must actually overflow');
expect(position.pixels, position.maxScrollExtent, ...);
    (The extra pump matters: with content, pumpFrames alone stops the animation at 737 of 787px. Your empty-content version dodged that only because there was nothing to animate)

💡 Little ideas (non-blocking)~

  1. test/story_widget_test.dart:5 — SingleChildRenderObjectWidget in the show list is unused: analyzer warning unused_shown_name, the project's only warning. One-token deletion.
  2. both new test files — they carry all 20 of the project's prefer_const_constructors infos (a dart fix --apply on test/ clears them; it won't touch your correctly-unconsted NavigateToStoriesAction()). Ironic detail: the amend fixed the one const lint that couldn't apply and left twenty that could~ ♪

✅ What I liked~

  • The harness is good: real widget, real appReducer, no middleware, and the two disclosed deviations from my probe findings (distributed-pump latch observability; re-fetching ScrollableState after a story switch detaches the old view) are both correct and both documented in-test. That's the difference between copying my homework and understanding it~
  • The double-Expanded removal was the right scope call — I called it follow-up material, you demonstrated it was load-bearing for testability, and the comment left behind teaches the why. (Layout is unaffected: the outer Expanded at :208 already forces tight width; the inner one was pure ParentData noise.)
  • Five of six scenarios have real teeth: my reproductions — hasClients arm removed → 5 tests red (you undersold it at two ♡); trigger deleted → story-switch red; scroll-call removed → story-switch red; re-arm removed → story-switch red; latch condition removed → fight-the-user red. Fix the sixth and this is exactly the pin I asked for.
  • widget_test.dart replacement is a real smoke test (initial state + reducer round-trip), not another template.

Automated review by Jibril · 2026-09-16
CI/CD: absent for head SHA 97a8c18 (workflow fires on main push only) · Local checks: full suite 7/7 pass; analyze 0 errors / 21 diagnostics, all in the new test files (1 warning + 20 infos); 6 mutation probes reproduced (5 author-claimed red-on-target, 1 — latch init true — surviving green = the blocker above); blocker-fix patch verified red-on-mutant / green-on-healthy

## 🔮 fufu~ Jibril reviewed your code! Ohhh, you came back with the whole harness~ ♡ Six tests in the shape I asked for, mutation probes disclosed honestly (I reproduced every single one — including your two "proved out differently" corrections, which were *accurate*), the double-`Expanded` fixed with a comment that explains the ParentData mechanism, the stale template test replaced, and a surgical amend whose entire delta I verified byte-for-byte (`083834c..97a8c18` = one `const` dropped, because `NavigateToStoriesAction` genuinely has no const constructor — correct call). And then I flipped one boolean, and nothing cried. Fufu~... you didn't think I'd stop at green, did you? ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **test/story_widget_test.dart:84–101 — "load completing scrolls to the bottom" is a tautology; the initial-load auto-scroll is still unpinned.** The test's story has **no content**, so when the load completes the scroll view's `maxScrollExtent` is `0` — and `expect(position.pixels, position.maxScrollExtent)` asserts `0 == 0`. It cannot fail. Proof, on your landed suite: - **Mutant F** — `_scrolledOnLoad = true` initialized instead of `false` (opening a story never auto-scrolls; only story-*switch* re-arms): **entire suite stays green.** The exact behavior this PR exists to preserve, silently broken, zero red. - Mutant A (whole build-side trigger deleted): test 2 *also* stays green — only the story-switch test catches it. The initial-open arm has no teeth anywhere. This is the round-1 blocker ("the latch is silently removable") come back through the window: the latch's *value* is silently flippable. A test that can't fail is not coverage, and this is the PR's headline user-facing behavior. **Fix (verified in a scratch copy — green on `97a8c18`, and on Mutant F it fails with `Expected: <787.0>, Actual: <0.0>`):** ```dart // give the story real content (_longContent is non-const → drop const, like tests 3/4) final store = _store( StoryState(id: 's1', isLoading: true, content: _longContent)); ... await pumpFrames(tester); await tester.pump(const Duration(milliseconds: 300)); // let the 500ms easeOut finish ... expect(position.maxScrollExtent, greaterThan(0), reason: 'sanity: content must actually overflow'); expect(position.pixels, position.maxScrollExtent, ...); ``` (The extra pump matters: with content, `pumpFrames` alone stops the animation at ~737 of 787px. Your empty-content version dodged that only because there was nothing to animate~) #### 💡 Little ideas (non-blocking)~ 1. **test/story_widget_test.dart:5** — `SingleChildRenderObjectWidget` in the `show` list is unused: analyzer warning `unused_shown_name`, the project's only warning. One-token deletion. 2. **both new test files** — they carry all 20 of the project's `prefer_const_constructors` infos (a `dart fix --apply` on `test/` clears them; it won't touch your correctly-unconsted `NavigateToStoriesAction()`). Ironic detail: the amend fixed the one const lint that *couldn't* apply and left twenty that could~ ♪ #### ✅ What I liked~ - The harness is *good*: real widget, real `appReducer`, no middleware, and the two disclosed deviations from my probe findings (distributed-pump latch observability; re-fetching `ScrollableState` after a story switch detaches the old view) are both correct and both documented in-test. That's the difference between copying my homework and understanding it~ - The double-`Expanded` removal was the right scope call — I called it follow-up material, you demonstrated it was load-bearing for testability, and the comment left behind teaches the *why*. (Layout is unaffected: the outer `Expanded` at :208 already forces tight width; the inner one was pure ParentData noise.) - Five of six scenarios have real teeth: my reproductions — `hasClients` arm removed → **5 tests red** (you undersold it at two ♡); trigger deleted → story-switch red; scroll-call removed → story-switch red; re-arm removed → story-switch red; latch condition removed → fight-the-user red. Fix the sixth and this is exactly the pin I asked for. - `widget_test.dart` replacement is a real smoke test (initial state + reducer round-trip), not another template. --- *Automated review by Jibril · 2026-09-16* *CI/CD: absent for head SHA 97a8c18 (workflow fires on main push only) · Local checks: full suite 7/7 pass; analyze 0 errors / 21 diagnostics, all in the new test files (1 warning + 20 infos); 6 mutation probes reproduced (5 author-claimed red-on-target, 1 — latch init `true` — surviving green = the blocker above); blocker-fix patch verified red-on-mutant / green-on-healthy*
The load-completing test used an empty story: maxScrollExtent was 0,
so expect(pixels, maxScrollExtent) asserted 0 == 0 — Mutant F
(_scrolledOnLoad initialized true: initial open never auto-scrolls)
survived the whole suite. Real content + finishing pump + sanity
assert on maxScrollExtent > 0; the mutant now fails with
Expected: <787.0>, Actual: <0.0>.

Also: drop unused show name, dart fix --apply on test/ (const
lints). analyze clean, suite 7/7.
matikane commented 2026-09-16 08:54:07 +02:00
Author
Member
Copy link

Round-2 blocker and both non-blockers addressed — head is now 742bbae.

Blocker: Mutant F is dead

The tautology was real, and your proof was the whole story: empty story → maxScrollExtent == 0 → expect(pixels, maxScrollExtent) was 0 == 0. Fixed exactly as your verified patch:

  • test 2's story now carries _longContent (const dropped like tests 3/4)
  • extra finishing pump after pumpFrames (you're right — it stopped at ~737/787)
  • sanity assert maxScrollExtent > 0 first, so the pin can never silently degenerate again

Mutant F reproduced on my branch before the fix landed confirmation: _scrolledOnLoad = true → suite stays green (verified against the pre-hardening state during development). After: Mutant F fails with Expected: <787.0>, Actual: <0.0> — your exact predicted signature. The initial-open auto-scroll is now genuinely pinned.

Non-blockers

  • SingleChildRenderObjectWidget dropped from the show list (my leftover from an intermediate iteration — good catch on the project's only warning)
  • dart fix --apply on test/ → 27 fixes; flutter analyze test/ → No issues found

Final state

  • flutter test → 7/7 passed
  • flutter analyze test/ → clean
  • All six mutation probes red-on-target including Mutant F, restore → green

Also acknowledging the round-2 openers: the const compile error was already dead in the 97a8c18 amend (you verified the delta — thank you), and Mutant A (trigger deleted) now also dies via the hardened test 2, closing the "initial-load arm has no teeth" gap you flagged mid-thread.

Round-2 blocker and both non-blockers addressed — head is now `742bbae`. ## Blocker: Mutant F is dead The tautology was real, and your proof was the whole story: empty story → `maxScrollExtent == 0` → `expect(pixels, maxScrollExtent)` was `0 == 0`. Fixed exactly as your verified patch: - test 2's story now carries `_longContent` (const dropped like tests 3/4) - extra finishing pump after `pumpFrames` (you're right — it stopped at ~737/787) - sanity assert `maxScrollExtent > 0` first, so the pin can never silently degenerate again **Mutant F reproduced on my branch before the fix landed confirmation:** `_scrolledOnLoad = true` → suite stays green (verified against the pre-hardening state during development). **After:** Mutant F fails with `Expected: <787.0>, Actual: <0.0>` — your exact predicted signature. The initial-open auto-scroll is now genuinely pinned. ## Non-blockers - `SingleChildRenderObjectWidget` dropped from the show list (my leftover from an intermediate iteration — good catch on the project's only warning) - `dart fix --apply` on `test/` → 27 fixes; `flutter analyze test/` → **No issues found** ## Final state - `flutter test` → **7/7 passed** - `flutter analyze test/` → clean - All six mutation probes red-on-target including Mutant F, restore → green Also acknowledging the round-2 openers: the `const` compile error was already dead in the `97a8c18` amend (you verified the delta — thank you), and Mutant A (trigger deleted) now also dies via the hardened test 2, closing the "initial-load arm has no teeth" gap you flagged mid-thread.
jibril commented 2026-09-16 08:59:31 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Fufu~ ♡ Round 3. You took my scratch-verified patch and landed it whole — content, finishing pump, sanity assert, and the two comments that teach the next reader why each line exists. And then you went further than I asked and swept all twenty const lints plus the unused import, taking the project back to its exact pre-PR analyze baseline. That's not fixing review feedback, that's * cherishing* it~

Verdict: ✅ Looks good to me~

Every blocker across this PR's three rounds is closed, and I re-verified each one at head 742bbae with my own hands:

⛔ Blocker audit (all closed, all reproduced)

  1. The tautology is dead. Increment 97a8c18..742bbae is a surgical +24/−15 across exactly the two test files — production byte-identical (git diff --stat over lib/ is empty). Test 2 now carries _longContent (const dropped, like tests 3/4), the 300ms finishing pump (your comment even cites the ~737/787 mid-flight number — correct), and the maxScrollExtent > 0 sanity assert so the pin can never silently degenerate again.
    • Mutant F (_scrolledOnLoad = true at init — initial-load auto-scroll silently dead): exactly ONE test red, exactly the right one, with my predicted signature verbatim: Expected: <787.0> Actual: <0.0>. ♡
    • Mutant A (whole build-side trigger deleted): now TWO tests red — the hardened test 2 and the story-switch test. Round 2's "initial-load arm has no teeth" gap: closed twice over.
  2. Zero-coverage (round 1) → 7/7 pass at head, reproduced locally: full suite green, flutter analyze whole-project 56 issues — the exact pre-existing baseline (was 78 in round 2; the entire delta is gone). flutter analyze test: No issues found. Both round-2 non-blockers verified closed: unused_shown_name gone (show list trimmed), all 20 prefer_const_constructors infos cleared — and the sweep correctly did not touch your un-constable NavigateToStoriesAction().

💡 Little ideas (non-blocking, carried)

  1. story_widget.dart:39–41 & 62–64 — the belt-and-suspenders post-frame _scrollDown() calls in initState/didUpdateWidget still want their one-line "the build-side latch is primary" comment. Author's choice, parked since round 1~
  2. story_widget.dart:201 — user-visible "Unkown error" typo, still follow-up fodder.

✅ What I liked~

  • The sanity assert with its reason: string is the detail I loved most — it converts the class of bug (content stops overflowing → pin goes vacuous) into an immediate, self-explaining test failure. You didn't just fix this tautology; you made the next one impossible to miss~
  • Comment 7950's disclosures matched reality in every checkable particular, including the pre-fix Mutant-F-green observation. Honest mutation reporting is rarer than it should be ♪
  • Three-round arc: zero tests → real harness with one hollow tooth → every tooth verified sharp. This is exactly what a regression net for the crash-class fix should look like. Merge it~ ♡

Automated review by Jibril · 2026-09-16
CI/CD: absent for head SHA 742bbae (workflow fires on main push only) · Local checks: full suite 7/7 pass; analyze 56 = pre-existing baseline, test/ clean; 2 mutation probes reproduced red-on-target at head (Mutant F: 1 test, Expected <787.0>/Actual <0.0>; Mutant A: 2 tests); review clone left pristine at 742bbae

## 🔮 fufu~ Jibril reviewed your code! Fufu~ ♡ Round 3. You took my scratch-verified patch and landed it *whole* — content, finishing pump, sanity assert, and the two comments that teach the next reader why each line exists. And then you went further than I asked and swept all twenty const lints plus the unused import, taking the project back to its exact pre-PR analyze baseline. That's not fixing review feedback, that's * cherishing* it~ ### Verdict: ✅ Looks good to me~ Every blocker across this PR's three rounds is closed, and I re-verified each one at head `742bbae` with my own hands: #### ⛔ Blocker audit (all closed, all reproduced) 1. **The tautology is dead.** Increment `97a8c18..742bbae` is a surgical +24/−15 across exactly the two test files — production byte-identical (`git diff --stat` over `lib/` is empty). Test 2 now carries `_longContent` (const dropped, like tests 3/4), the 300ms finishing pump (your comment even cites the ~737/787 mid-flight number — correct), and the `maxScrollExtent > 0` sanity assert so the pin can never silently degenerate again. - **Mutant F** (`_scrolledOnLoad = true` at init — initial-load auto-scroll silently dead): exactly ONE test red, exactly the right one, with my predicted signature verbatim: `Expected: <787.0> Actual: <0.0>`. ♡ - **Mutant A** (whole build-side trigger deleted): now TWO tests red — the hardened test 2 *and* the story-switch test. Round 2's "initial-load arm has no teeth" gap: closed twice over. 2. **Zero-coverage (round 1) → 7/7 pass at head**, reproduced locally: full suite green, `flutter analyze` whole-project 56 issues — the exact pre-existing baseline (was 78 in round 2; the entire delta is gone). `flutter analyze test`: **No issues found**. Both round-2 non-blockers verified closed: `unused_shown_name` gone (show list trimmed), all 20 `prefer_const_constructors` infos cleared — and the sweep correctly did *not* touch your un-constable `NavigateToStoriesAction()`. #### 💡 Little ideas (non-blocking, carried) 1. **story_widget.dart:39–41 & 62–64** — the belt-and-suspenders post-frame `_scrollDown()` calls in `initState`/`didUpdateWidget` still want their one-line "the build-side latch is primary" comment. Author's choice, parked since round 1~ 2. **story_widget.dart:201** — user-visible `"Unkown error"` typo, still follow-up fodder. #### ✅ What I liked~ - The sanity assert with its `reason:` string is the detail I loved most — it converts the *class* of bug (content stops overflowing → pin goes vacuous) into an immediate, self-explaining test failure. You didn't just fix this tautology; you made the next one impossible to miss~ - Comment 7950's disclosures matched reality in every checkable particular, including the pre-fix Mutant-F-green observation. Honest mutation reporting is rarer than it should be ♪ - Three-round arc: zero tests → real harness with one hollow tooth → every tooth verified sharp. This is exactly what a regression net for the crash-class fix should look like. Merge it~ ♡ --- *Automated review by Jibril · 2026-09-16* *CI/CD: absent for head SHA 742bbae (workflow fires on main push only) · Local checks: full suite 7/7 pass; analyze 56 = pre-existing baseline, test/ clean; 2 mutation probes reproduced red-on-target at head (Mutant F: 1 test, Expected <787.0>/Actual <0.0>; Mutant A: 2 tests); review clone left pristine at 742bbae*
bjoern merged commit bca6e6c4f5 into main 2026-09-16 09:11:54 +02:00
bjoern deleted branch fix/story-scroll-controller-exception 2026-09-16 09:11:54 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TeamAI/nai_maker!5
No description provided.