test: Flutter test harness + critical notifier/filter unit tests - #181
Merged
Conversation
- BackupReminderNotifier: badge visibility, snooze/dismiss/complete flows - BackupCompletedNotifier: load legacy fallback, mark/reset persistence - FirstRunNotifier: async load, default false, markFirstRunComplete - filteredOrdersProvider: tab/currency/payment/rating/premium filters, sort - filteredTradesWithOrderStateProvider: status filter, terminal collapse, sort - fake_orders/fake_trades test helpers with controllable timestamps
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Flutter and Riverpod tests for account notifiers, first-run persistence, order filtering, and trade filtering, along with deterministic fixtures and provider-container cleanup utilities. ChangesAccount notifier and first-run coverage
Order filtering coverage
Trade filtering coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
Member
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/features/trades/filtered_trades_provider_test.dart (1)
26-33: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
unorderedEqualsinstead ofcontainsAllto ensure exact match.
containsAllverifies that the specified items are present but does not fail if unexpected items are also present. Since the goal is to assert the exact set of returned trades (and order isn't strictly important for this specific test),unorderedEqualsis a stronger assertion.♻️ Proposed fix
test('"All" returns every trade', () async { final container = _tradesWith([ fakeTrade(id: 'a', status: OrderStatus.active), fakeTrade(id: 'b', status: OrderStatus.pending), ]); - expect(await _orderIds(container), containsAll(['order-a', 'order-b'])); + expect(await _orderIds(container), unorderedEquals(['order-a', 'order-b'])); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/trades/filtered_trades_provider_test.dart` around lines 26 - 33, Update the “All” test’s assertion in `_orderIds` to use `unorderedEquals` with `['order-a', 'order-b']`, ensuring the result contains exactly those trade IDs while remaining order-independent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/features/trades/filtered_trades_provider_test.dart`:
- Around line 47-59: Update the terminal-status test’s expectation in
`test('terminal protocol statuses collapse into the success filter')` to use
`unorderedEquals` with exactly `['order-success', 'order-settled']`, ensuring
`order-canceled` is excluded while preserving order-independent matching.
---
Nitpick comments:
In `@test/features/trades/filtered_trades_provider_test.dart`:
- Around line 26-33: Update the “All” test’s assertion in `_orderIds` to use
`unorderedEquals` with `['order-a', 'order-b']`, ensuring the result contains
exactly those trade IDs while remaining order-independent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4397656-d06b-43cc-970a-a771b706a1cc
📒 Files selected for processing (7)
test/features/account/backup_reminder_provider_test.darttest/features/home/filtered_orders_provider_test.darttest/features/trades/filtered_trades_provider_test.darttest/features/walkthrough/first_run_provider_test.darttest/support/fake_orders.darttest/support/fake_trades.darttest/support/provider_harness.dart
grunch
reviewed
Jul 16, 2026
grunch
left a comment
grunch
left a comment
Member
There was a problem hiding this comment.
Strict Review
Actionable comments: 9 (1 high, 4 medium, 4 low) · Verdict: Approve with observations — no blocking issues; the HIGH is a one-line matcher fix worth doing before merge.
Verification performed
- ✅ Every assertion cross-checked against the actual provider sources (
backup_reminder_provider.dart,home_order_providers.dart,trades_providers.dart,first_run_provider.dart) — they reflect implemented behavior (prefs keys, snooze semantics, terminal-status collapse, default-range guards). - ✅
flutter analyzerun locally on this branch in an isolated worktree: clean. - ✅ CI green (Flutter analyze/test, Rust, CodeRabbit); the 29-test count in the PR body checks out (12+9+4+4).
What's solid
createContainer+addTearDown(container.dispose)is the canonical Riverpod test harness; zero state leakage between tests (setMockInitialValuesresets prefs per test).- Rust-backed providers correctly isolated via
overrideWith— no native bridge involved. kFakeNowanchored timestamps make sort assertions deterministic.- Behavior-descriptive test names, consistent AAA structure, legacy fallback covered (
BackupCompletedNotifier→ dismissed flag), and the non-obviousinitialValue+ live-snooze reconciliation case is tested. - Fits existing CI with no workflow change; accurate PR description.
Summary of findings (details inline)
| Severity | File | Issue |
|---|---|---|
| 🔴 High | filtered_trades_provider_test.dart |
Terminal-collapse test uses containsAll — cannot fail for the exact regression it targets |
| 🟠 Medium | filtered_trades_provider_test.dart |
"All" test: containsAll doesn't assert length |
| 🟠 Medium | filtered_trades_provider_test.dart |
orderStatusToFilter has 15 arms; only the success bucket is partially covered |
| 🟠 Medium | filtered_orders_provider_test.dart |
Default-range guard semantics (!= defaultRatingRange) unpinned |
| 🟠 Medium | fake_orders.dart |
Builder cannot produce range-shaped orders |
| 🟡 Low | filtered_orders_provider_test.dart |
Latent autoDispose fragility in _bookWith |
| 🟡 Low | filtered_orders_provider_test.dart |
ProviderContainerHelper could live in test/support/ |
| 🟡 Low | fake_trades.dart |
Fixture pairs status: active with TradeStep.disputed() |
| 🟡 Low | backup_reminder_provider_test.dart |
Wall-clock dependence in snooze fixtures (noted, not blocking) |
- add tests for unrated orders and range orders in filteredOrdersProvider - add orderStatusToFilter exhaustiveness test in filtered_trades_provider_test - extract bookWith/OrderBookHarness from filtered_orders_provider_test to support/order_book_harness.dart with autoDispose keepAlive fix - fakeOrder now supports range orders via fiatAmountMin/fiatAmountMax params - replace containsAll with unorderedEquals in filtered_trades
grunch
approved these changes
Jul 17, 2026
Member
There was a problem hiding this comment.
All High/Medium comments resolved and verified against the actual code:
- 🔴 High (success test did not verify exclusion) →
unorderedEquals+ the fixture keeps thecanceledtrade, so it now fails if one leaks into the success bucket. - 🟠
containsAll→unorderedEquals, exhaustiveorderStatusToFiltertest with a length guard, coverage for unrated/range orders, andfakeOrdernow supports range orders. - 🟡 autoDispose keepAlive + helper extracted to
test/support/order_book_harness.dart.
CI green (Flutter, Rust, CodeRabbit). The two remaining Low nits (active+disputed fixture in fake_trades, wall-clock in backup_reminder) are optional follow-ups. LGTM ✅
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
2 participants
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.
Refs #152 (PR1 of 2 — phased).
First slice of the Flutter test base:
ProviderContainerharness with auto-dispose + fake data builders (test/support/).BackupReminderNotifier/BackupCompletedNotifier(snooze, dismiss, legacy migration) andFirstRunNotifier(loading→data, first-launch default).filteredOrdersProvider(tabs, ownership, currency/payment/rating/premium, sort) andfilteredTradesWithOrderStateProvider(status filter, terminal-status collapse, sort) — Rust-backed providers are overridden with fakes.29 tests passing;
flutter analyzeclean. Suite runs in the existing CIflutter teststep (no workflow change needed).Deferred to PR2: DESIGN_SYSTEM golden tests.
Summary by CodeRabbit