fix(home): the order book grid survives an order coming back

[Catrya]

Closes #453

With the order book in its grid layout (window 600 px or wider), every order that left the book and came back made SliverMasonryGrid throw in performLayout (Null check operator used on a null value) on every frame. The grid went red in debug and blank for the user for a few seconds, until a later update laid it out again. An order leaves and comes back whenever a take is cancelled or times out, so anyone watching the grid saw it, not only the user involved.

Cause

OrderBookList passed the list's findChildIndexCallback (#357) to its masonry grid (#424), so that cards moved instead of being rebuilt. flutter_staggered_grid_view 0.7.0 (its latest release, July 2023) cannot lay out a child moved that way.

Change

• Grid (2 or 3 columns): no index callback, and no ValueKey on its cards. As OrderBookList notes, a key without the callback is worse than none.
• List (1 column): unchanged. It keeps both, and indexOfKey now lives in its branch.

The cost: in the grid, a card shifted by an arriving order or a re-sort is rebuilt in place instead of moved.

Tests

• order_book_list_reorder_test.dart: the "keeps a row with its order across a re-sort" case now covers the list only.
• New regression group: with 2 and 3 columns, the grid goes through an order in the middle leaving and coming back, the first order leaving and coming back, and an older order arriving in the middle. It lays out without an exception every time.
• With the callback restored in the grid, 5 of the 6 new cases fail. The sixth, 2 columns with an older order arriving in the middle, did not fail on main either: that case only breaks with 3 columns.
• flutter analyze is clean and flutter test passes (1029).

Manual test

With a wide window, took and cancelled orders so each came back to the book, three times. No exceptions and no blank grid; before this change each of those threw on every frame.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: cf704c70-f8cb-4415-a22a-97a6c80fac8f

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[grunch]

Actionable comments posted: 3

Root cause and fix direction are right: the crash is childScrollOffset(child)! in RenderSliverMasonryGrid.performLayout (flutter_staggered_grid_view-0.7.0/lib/src/rendering/sliver_masonry_grid.dart:380), reached once findChildIndexCallback moves a child the masonry layout never positioned, and dropping the callback in the grid removes it. The single-column path is unchanged. One part of the change is not neutral, though: removing the ValueKey from grid cards lets an in-flight press open a different order than the one pressed. Verified below, and the fix is a one-word change the PR's own regression tests already accept.

🧹 Verified-OK notes (4)

• Crash site matches #453's trace (sliver_masonry_grid.dart:380:54, the childScrollOffset(child)! in the earliest-scroll-offset scan). ✅
• No per-order state can bleed on an in-place rebuild: OrderListItem and every sub-widget in order_list_item.dart (_HeaderRow, _CurrencyChip, _Chip, _AmountRow, _FixedSatsCaption, _PaymentMethodsRow, _ReputationStrip, _Stat) are StatelessWidgets, so the only state that crosses a shift is the InkWell's gesture/splash — see the inline comment. ✅
• List path (columns == 1) keeps ValueKey + the doubled index callback; the "keeps a row with its order across a re-sort" test still guards it. ✅
• CI green on the merge ref (Flutter, Rust, Web smoke); mergeable: MERGEABLE. ✅

📜 Review details

Reviewed commit: 7255ac6
Files reviewed: 2 — lib/features/home/widgets/order_book_list.dart, test/features/home/order_book_list_reorder_test.dart
Profile: strict

How the finding below was verified (throwaway worktree on 7255ac6, not pushed): a probe widget test pumps the grid (2 columns) with orders [10, 20, 30, 40], starts a press on the second card (order-20), pumps a book where a newer order arrives on top ([5, 10, 20, 30, 40]), releases, and records onOrderTap.

Grid cards	Index callback	onOrderTap after the press	PR's 6 "grid lays out an order that comes back" tests
unkeyed (this PR)	none	order-10 — a different order than the one pressed	pass
ValueKey(order.id)	none	none — the press is cancelled with the torn-down element	pass

[grunch]

🎯 Functional Correctness | 🟠 Major

Unkeyed grid cards let a press open the wrong order when the book shifts mid-gesture.

Without a key, the element at grid index i is reused for whatever order lands at i after an update. Its InkWell keeps the in-progress gesture, and on release it calls the new widget's onTap, which is () => onOrderTap(<the order now at i>). Book updates arrive live from relays, and the grid is exactly the layout used on desktop, web and tablets, where press-and-hold (and a slow click) is common.

Reproduced on this commit: press order-20 (index 1), a newer order arrives and every card shifts one slot, release → onOrderTap('order-10'). The user lands on an order they never selected, possibly with a Take button in front of them. The same reuse moves hover and keyboard focus to a different order on web and desktop.

The PR removes the key on the premise that "without the callback a key would only make that worse". That holds for rebuild cost, not correctness. A keyed card whose key no longer matches the widget at its index fails Widget.canUpdate, so the element is torn down, which cancels the gesture. It does not reintroduce the crash, because nothing is moved by key. With keyed: true here and still no findChildIndexCallback:

• the same probe records no tap (the press is cancelled);
• all 6 new "grid lays out an order that comes back" tests still pass.

Suggested fix: keep the key in the grid, not the callback, and reword the comment above so the next reader doesn't "optimise" it away again.

-              (context, index) => _card(orders[index], keyed: false),
+              (context, index) => _card(orders[index], keyed: true),

    // Unlike the list, the grid does not move cards by key:
    // `SliverMasonryGrid` cannot lay out a child moved by
    // `findChildIndexCallback` (#453). Its cards stay keyed anyway. A card
    // whose order moved is torn down and re-inflated rather than reused for
    // another order, so a press, hover or focus in progress never lands on
    // an order the user did not pick. The cost is a re-inflate instead of a
    // rebuild for each shifted card.

🤖 Prompt for AI Agents

In lib/features/home/widgets/order_book_list.dart, the SliverMasonryGrid builder (around line 106):
pass keyed: true to _card so grid cards keep ValueKey(order.id), while still NOT passing
findChildIndexCallback to the grid's SliverChildBuilderDelegate. Update the comment block above
the grid (around lines 81-89) to explain the key is kept for gesture/focus safety and that only
the index callback is omitted (#453). Then add the regression test described on
test/features/home/order_book_list_reorder_test.dart.

[grunch]

📐 Maintainability | 🔵 Trivial

This comment will rot, and the dependency isn't pinned to the version it describes.

"flutter_staggered_grid_view 0.7.0, its latest release" is true today, but pubspec.yaml declares ^0.7.0, so a 0.7.x release that changes RenderSliverMasonryGrid would land on the next pub upgrade with nothing prompting anyone to revisit this workaround.

Consider referencing the issue instead of a point-in-time claim (… cannot lay out a child moved by findChildIndexCallback (#453, flutter_staggered_grid_view 0.7.0)). Either pin the dependency exactly, or leave a TODO(#453) to re-enable the callback in the grid once upstream handles moved children. The regression group below would then show whether a future version fixed it.

[grunch]

🧪 Test Coverage | 🟡 Minor

The new group proves "no exception", but nothing guards what the user taps after a shift.

Every case asserts takeException() == null and four cards. That is the right regression for #453, but it is equally green whether grid cards are keyed or not, so the wrong-order press from the other comment isn't covered, and would silently come back if someone later drops the key "for performance".

Suggested addition next to this group (verified: it fails on this commit and passes with keyed: true):

testWidgets('a press that spans a book shift never opens another order', (
  tester,
) async {
  tester.view.physicalSize = const Size(1000, 2400);
  tester.view.devicePixelRatio = 1.0;
  addTearDown(tester.view.reset);
  await withClock(Clock.fixed(kFakeNow), () async {
    final taps = <String>[];
    Future<void> pumpWith(List<int> book) => tester.pumpWidget(
      MaterialApp(
        theme: buildDarkTheme(),
        localizationsDelegates: AppLocalizations.localizationsDelegates,
        supportedLocales: AppLocalizations.supportedLocales,
        home: Scaffold(
          body: OrderBookList(
            orders: _book(book),
            currencyFlags: const {'USD': '🇺🇸'},
            reasons: const {},
            columns: 2,
            onOrderTap: taps.add,
          ),
        ),
      ),
    );

    await pumpWith([10, 20, 30, 40]);
    final gesture = await tester.startGesture(
      tester.getCenter(find.byType(OrderListItem).at(1)), // order-20
    );
    await tester.pump(const Duration(milliseconds: 50));
    await pumpWith([5, 10, 20, 30, 40]); // every card shifts one slot
    await gesture.up();
    await tester.pumpAndSettle();

    expect(taps, isNot(contains('order-10')));
  });
});