[realmeylisdev]

Description

Four test files that navigate the app's bottom tabs were switched off with skip: true and stayed off. This turns them back on and fixes the two underlying causes that kept them off.

The baseline annotations named the wrong blocker. skip_test_ceilings.txt described things like "explore tabs renamed" and "shell nav bar is a column, bottomnavigationbar cast dies" — real problems, but no test ever reached them. Every one of these files died first on:

UnimplementedError: sharedPreferencesProvider must be overridden in ProviderScope
type 'Null' is not a subtype of type 'Map<String, RelayConnectionStatus>'

createMockNostrService() never stubbed the relay-status surface, so anything pumping the app shell threw inside AppShellSideEffects before a widget rendered. That one helper is why four rows carried annotations describing blockers nobody had observed. The fix follows the shape already used directly above it — a non-null default, with the error it prevents named in the comment.

These rows disabled far more than they appeared to. Three of the four skip: true directives in app_shell_integration_test sat on a group(), so 4 directives were disabling 9 tests. Across this PR, 7 directives were holding 12 tests off.

What each restored file now covers:

• Explore tab tap — tapping Explore goes to /explore (grid), not /explore/0 (feed). Taps through find.bySemanticsIdentifier per .claude/rules/accessibility.md; the shell renders VineBottomNav inside a Column, so there is no BottomNavigationBar.onTap to invoke.
• Explore grid mode — grid mode shows the tab bar, not the feed's empty state. The old assertions pinned three tab names that no longer exist (New Vines, Trending, Editor's Pick; the tabs are Classics / New / Popular). The tab list is server-driven, so this asserts on ExploreTabBar instead, and the empty-state string resolves through AppLocalizations rather than being hardcoded.
• App shell navigation — all four groups, 8 tests: shell rendering, route normalization, deep links landing in the right tab, tab-state preservation, and canonical tab paths.
• Fresh-first feed ordering — deleted rather than repaired; see below.

Three assertions were wrong about current behaviour

Un-skipping surfaced expectations that no longer describe the app:

• "/hashtag/rust selects Tags tab" expected index 2. There is no Tags tab — index 2 is the inbox — and a hashtag feed is registered on the root navigator (search_routes.dart: "standalone screen (no bottom nav)"), so it is presented above the shell rather than selecting a tab.
• Tapping Explore was expected to land on /explore/0. That is the exact bug explore_tab_navigation_test exists to prevent.
• "/profile/me/5" was expected to survive; the profile route resolves it to the signed-in npub. Asserted against a literal rather than through the encoder, so it cannot pass for a broken encoder.

"can navigate back within tab stack" is deleted. go() replaces the location inside a shell branch, so two go() calls leave nothing on the stack and pop() throws "There is nothing to pop" — it asserted a mechanism the app does not use, and back behaviour has live coverage in navigation/back_navigation_policy_test.

Why one file was deleted instead of fixed

video_events_provider_fresh_test.dart was annotated # quarantine: real relay ... 40s timeout. It never reached a relay — it died at 00:00 on the missing provider override.

Its ordering claim sits inside if (unseenIndices.isNotEmpty && seenIndices.isNotEmpty), so an empty or all-seen response passes it without checking anything. And the behaviour it asserted is not implemented: rebuilt hermetically against the harness in video_events_provider_listener_test, fed [seen1, unseen1, seen2, unseen2] with seen1/seen2 marked seen, the provider emits that order straight back. Nothing under lib/ orders the discovery feed by seen state.

Whether the feed should order unseen-first is a product question this PR does not answer. It is raised on #4836 instead.

Related Issue: Relates to #4836

Unrequested change — please look at this separately

fix(hashtags): cancel the hashtag service timer when its provider disposes is a production change, not a test fix, and it is the only lib/ change here.

HashtagService's constructor starts a 1-minute periodic timer and registers a listener on VideoEventService. It has a dispose() that undoes both, and nothing called it. The provider watches two other providers, so it rebuilds whenever either changes, and every rebuild stranded a live timer and a listener on the discarded instance. ref.onDispose(service.dispose) is the fix.

It surfaced because the restored app-shell suite kept failing on "A Timer is still pending even after the widget tree was disposed", traced to that constructor, and disposing the ProviderContainer did not stop it. Flagging it on its own because it changes shipped behaviour and was not part of the requested work — the effect is that the timer and listener now stop when the provider goes away.

Out of Scope

• The remaining 5 baseline rows keep their annotations. Six of the original nine are wrong or stale; re-annotating is tracked on test(skips): triage frozen skipped tests (#4337 WS-1) #4836.
• Fresh-first feed ordering, as above.

Verification

• flutter analyze lib test integration_test — clean.
• Full flutter test test --exclude-tags "integration || golden" — 20000 passed, 41 skipped, 0 failed on the rebased branch. The shared helper is imported by 154 test files, so the whole suite was run rather than the touched ones.
• Each restored file was mutation-checked, since test(skips): triage frozen skipped tests (#4337 WS-1) #4836 exists to stop banking tests that cannot fail:
  • Explore tab tap: pointing VineBottomNav._handleTabTap's explore branch at ExploreScreen.pathForIndex(lastIndex ?? 0) turns it red.
  • Explore grid mode: hiding ExploreTabBar turns it red.
  • App shell: pinning ShellChrome's currentIndex to 0 turns 4 of the 8 red.
• bash scripts/check_skip_ceiling.sh passes. This PR removes 4 files / 7 directives / 12 disabled tests; combined with test(profile): restore current-user redirect coverage #8885 landing, the baseline is now 5 files / 10 directives, down from 11 / 19.

Type of Change

• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ✅ Build configuration change

[rabble]

Reviewed at 8980088. No correctness findings in the test changes: the restored test taps the real Explore navigation control, the grid-mode assertion checks the rendered tab bar, and the removed fresh-first tests did not establish the behavior they claimed.

Local verification: all 9 focused router/provider tests passed; flutter analyze --no-pub lib test integration_test reported no issues.

Merge prerequisite: this head conflicts with current main in mobile/scripts/baseline/skip_test_ceilings.txt. The skip-ceiling check also fails against current origin/main because the branch still contains the two profile-redirect entries that main removed. Rebase and preserve both sets of reductions: this PR's fresh-first/Explore entries and main's profile-redirect entries. Rerun the guard and tests after resolution. Leaving this as a comment review pending that update; no branch changes were made.

[NotThatKindOfDrLiz]

Approved after takeover cleanup at 8980088. The restored navigation tests now have failure-safe cleanup, use the production semantic anchor, and rely on the shell-owned retap cubit. Misleading provider tests were removed and insertion-order emission is asserted directly. All applicable checks are green.

[NotThatKindOfDrLiz]

@realmeylisdev I took over the final review cleanup in 8980088654 and rechecked the full PR after CI completed.

Fixed:

• kept inline container disposal for Flutter's pending-timer check and restored teardown disposal for failure paths;
• removed the shadowed HomeFeedRetapCubit test wiring;
• switched the Explore anchor to SemanticIds.exploreTab;
• removed the misleading fresh-first test and the neighboring assertion-free gate test;
• strengthened provider-emission coverage to assert insertion order;
• corrected the PR title and description to match the final scope.

Escalated: whether discovery should prioritize unseen videos remains a product decision; this PR accurately preserves and tests today's insertion-order behavior.

Declined: none.

Verification is green: 9 targeted tests, full flutter analyze lib test integration_test, the skip ceiling guard, pre-push checks, all four CI test shards, Generated Files, Format, Analyze, Goldens, preview build, semantic title, linkage, and aggregate Mobile CI. Service integration jobs were skipped by the expected scope gate.

Please review the takeover commit and merge when you're ready. rabble remains requested for the normal human review path.

[NotThatKindOfDrLiz]

Review: Four findings (non-blocking)

All changes are technically sound. These are nits and one mandatory follow-up for the backlog.

Finding 1: Container disposal not guarded in addTearDown (lines 100, 134)

The periodic relay-status timer (3s interval, provider has keepAlive: true) is disposed inline after the test assertion. If the test fails before reaching that dispose call, the timer leaks into the rest of the merged VGV test bundle.

Fix: Wrap the disposal in addTearDown:

addTearDown(container.dispose);
// ... assertions ...
// inline dispose() removed

This is higher priority — a failure in these tests silently degrades the whole bundle.

---

Finding 2: Dead mock wiring (lines 45-46, 56)

HomeFeedRetapCubit is mocked in the test setup, but the app shell's own BlocProvider<HomeFeedRetapCubit> provides a real instance. Nothing in the test reads the mocked version.

This is a one-line cleanup; the wiring can be removed without loss. Non-blocking.

---

Finding 3: Hardcoded semantic identifier (line 87)

find.bySemanticsIdentifier('explore_tab') should use the constant SemanticIds.exploreTab to prevent drift if the value is renamed.

One-line cleanup. Non-blocking.

---

Finding 4: Vacuous live test assertion (video_events_provider_listener_test.dart:271)

The test named "should reorder videos to show unseen first" asserts only that discoveryVideos was accessed — never validates the actual reordering. Both the seen and unseen lists must be non-empty for the ordering assertion to run, so empty/all-seen responses pass silently.

This is mandatory follow-up work (fits #4836 → #7705 backlog). The test can stay, but the assertion must pin the actual ordering before the video is considered covered. File an issue if this isn't already tracked.

---

Recommendation: Approve with findings 1 and 4 as higher priority (1 is a safety net; 4 is a code-quality backlog item). Findings 2 and 3 can fold in alongside finding 1's fix.

[NotThatKindOfDrLiz]

Approved on rebased head 2165c91. The takeover cleanup remains intact, the skip-baseline conflict was resolved by preserving both independent reductions, and all applicable checks are green, including all four test shards and Mobile CI.

[rabble]

Re-reviewed rebased head 2165c91. The prerequisite in my previous review is resolved: both sets of skip-baseline reductions are preserved, and GitHub reports the branch mergeable.

Verified again on this head: 9 focused router/provider tests passed, flutter analyze --no-pub lib test integration_test reported no issues, and check_skip_ceiling.sh passed with 6 tracked keys and no growth. GitHub checks are green. No additional findings.

[NotThatKindOfDrLiz]

@realmeylisdev takeover follow-up: I rebased the PR onto current main and resolved the skip-baseline conflict by preserving both independent reductions—the Explore entries removed by this PR and the profile-redirect entries already removed on main.

Fixed in the takeover commit:

• added failure-safe ProviderContainer teardown while retaining the inline disposal needed for pending-timer checks;
• removed the shadowed HomeFeedRetapCubit test wiring;
• replaced the hardcoded Explore semantics identifier with SemanticIds.exploreTab;
• removed the misleading assertion-free provider tests and strengthened the retained test to assert insertion-order emission directly;
• corrected the PR title and description to match the resulting coverage.

The separate product decision about whether discovery should order unseen videos first remains intentionally outside this test-only PR; there is no implementation of that behavior to preserve here. No findings were declined.

Verification on current head 2165c915c:

• 9 focused Flutter tests passed locally;
• flutter analyze lib test integration_test passed;
• the skip ceiling passed with 6 tracked keys and no growth;
• every applicable GitHub check is green, including all four test shards, Generated Files, Goldens, Analyze, Format, preview, and aggregate Mobile CI.

I approved the rebased head and re-requested @rabble. Please review the takeover changes and merge when you’re happy with them.

[NotThatKindOfDrLiz]

Update: @rabble has now re-reviewed and approved rebased head 2165c915c. The PR is cleanly mergeable with both approvals and all checks green; merge remains with @realmeylisdev.

[realmeylisdev]

Pushed two commits on top of @NotThatKindOfDrLiz's 2165c915c — thanks for the rebase and the hardening. I adopted both of your improvements into the new file: SemanticIds.exploreTab instead of the magic string I had used, and container teardown.

app_shell_integration_test is now in scope after all. I had left it out and said it should wait for #8885; both of my stated reasons were wrong:

• I said one test asserted /profile/me/5 stays unresolved and so was coupled to test(profile): restore current-user redirect coverage #8885. The npub == 'me' redirect already lived on main independent of that PR, which only extracted the destination calculation into a pure function. The expectation was simply stale against main.
• I said the "Tags tab" case needed a product decision. It does not: tabIndexFromRouteType maps RouteType.hashtag to tab 1, and search_routes.dart registers the hashtag feed on the root navigator as a "standalone screen (no bottom nav)". The old expectation of index 2 was just wrong — index 2 is the inbox.

So the file is restored here: 4 directives that were disabling 9 tests, since three of them sat on a group() rather than a test. 8 are now green and one is deleted (it asserted go()/pop() back navigation inside a shell branch, which throws "There is nothing to pop" because go() replaces; back behaviour has live coverage in navigation/back_navigation_policy_test).

One thing genuinely needs separate eyes: the branch now carries a production change, fix(hashtags). HashtagService starts a 1-minute periodic timer and a VideoEventService listener in its constructor, has a dispose() for both, and the provider never called it — so every rebuild stranded a live timer. It surfaced as a pending-timer failure in the restored suite that disposing the container did not stop. It is one line plus the regenerated .g.dart, in its own commit, and called out in the description.

Also noticed you dropped the 'should reorder videos to show unseen first' test from video_events_provider_listener_test — agreed, that was the one asserting only that discoveryVideos was accessed. The underlying gap is still open: nothing under lib/ orders the discovery feed by seen state. Raised on #4836 rather than implemented here.

Full suite on the rebased branch: 20000 passed, 41 skipped, 0 failed. Watching CI now.

[realmeylisdev]

CI went red on my previous push and is green again on 78ff70ea3. Recording what happened, since the cause is worth knowing.

Tests (shard 2/4) failed with two 10-minute timeouts in cache_recovery_service_test.dart. That file has no reference to anything in this branch and passes in 8 seconds on its own — it was collateral from the merged isolate, not a defect in itself.

The cause was reach. My first commit stubbed the relay-status surface inside createMockNostrService, which roughly 150 suites inherit through getStandardTestOverrides. Once those reads stop throwing, relayStatisticsBridge opens a subscription and starts a 3-second periodic timer — so a change meant to unblock three shell tests was quietly adding work to every suite in the bundle, and on a runner slow enough it pushed later tests past their deadline.

78ff70ea3 moves the stubs to createMockNostrServiceWithRelayStatus(), which the three shell-pumping tests ask for explicitly. createMockNostrService is byte-for-byte back to its origin/main behaviour, so test_provider_overrides.dart is now purely additive.

Verified by reproducing CI's shard locally rather than guessing: scripts/ci/select_test_shard.sh --total 4 --index 2 in a throwaway worktree, then the same very_good test --optimization --concurrency=4 --exclude-tags integration --dart-define=DIVINE_STRICT_CHANNELS=true. Both the failing file and my restored app_shell_integration_test land in that shard together. Result: 4780 passed, 0 failed.

One honest caveat: my local shard took 5m56s against CI's 24m54s, so a 10-minute timeout is something this machine may simply be too fast to reproduce. The local green is supporting evidence, not proof on its own — the green CI run on this head is the actual verdict.

[realmeylisdev]

Both approvals here predate every commit currently on the branch, so please do not merge on them as they stand.

@NotThatKindOfDrLiz approved at 00:37Z and 00:53Z, @rabble at 01:02Z. The three commits now on the head landed at 01:55Z (643ced246, dbecb584b) and 03:01Z (78ff70ea3). GitHub still shows this as approved because stale-review dismissal is off in this repo.

That matters more than usual here, because what landed after the approvals includes:

• fix(hashtags) — a production change to hashtagServiceProvider, the only lib/ change in the PR.
• The whole of the restored app_shell_integration_test (8 tests, plus one deleted).
• The helper narrowing that fixed the red shard.

I raised the same pattern on #8885 a few hours ago and it would be inconsistent not to raise it on my own PR. A fresh look at 643ced246 in particular would be worth having before this goes in.

[rabble]

Reviewed commit 78ff70e.

No blocking findings. Re-reviewed the expanded real-shell navigation coverage, relay-status fixture isolation, and hashtag service disposal added since my previous approval. The provider now cancels the service timer and removes its listener on disposal. Validation: all 20 focused router, provider-listener, and hashtag tests passed; full app analysis and the skip-ceiling guard passed.

[github-actions]

Mobile PR Preview

Preview refreshed for 321a601

Last refresh: 321a601 at 2026-09-10 03:21:34 UTC (preview run)

Property	Value
Preview URL	https://16ddedb7.openvine-app.pages.dev
Pages project	openvine-app
Preview branch	pr-8950
PR branch	test/4836-restore-router-harness-coverage
Commit	321a601

[realmeylisdev]

Rebased onto current main to clear the conflict, now at 321a60107f. All 23 checks pass.

The approved content is unchanged. @rabble's approval is recorded against 78ff70ea3d and @NotThatKindOfDrLiz's against 2165c915c1, so both technically predate this head — but unlike the last time I flagged staleness here, nothing new was added. git range-diff over the two versions of the branch:

1:  d29e01bd04 =  1:  896633e128  test(helpers): stub the relay-status surface on the shared Nostr mock
2:  b25bfa5007 !  2:  365d45c502  test(router): restore explore tab tap coverage
3:  4a4a02f0e0 =  3:  91e0d86cf7  test(explore): restore grid-mode tab bar coverage
4:  982573a562 =  4:  ae94c14f40  test(feed): delete the unrunnable fresh-first relay test
5:  3c783aef6a =  5:  deee2f45a3  test(skips): lock the skip ceiling at 8 files
6:  2165c915c1 =  6:  35a1f7c101  test(router): harden restored navigation coverage
7:  643ced2465 =  7:  191ebdea65  fix(hashtags): cancel the hashtag service timer when its provider disposes
8:  dbecb584bf =  8:  bcf92845ea  test(router): restore app shell navigation coverage
9:  78ff70ea3d =  9:  321a60107f  test(helpers): make the relay-status stubs opt-in

Eight of nine are identical. The one marked ! differs only by the conflict hunk: main had a formatter reflow of a reason: string inside the still-skipped version of the file, and this branch rewrites that file. I kept equals(ExploreScreen.path) — the matcher .claude/rules/testing.md asks for, and already present in the version you approved — and let the string keep its wrapping, which dart format accepts because it is two adjacent literals rather than one.

Re-verified against Flutter 3.47.2

Worth calling out, because the 16 commits this rebase absorbed include #8915. The branch was written and verified entirely on 3.44.9, so the whole ladder was re-run rather than assumed:

Check	Result
dart format on the 6 changed Dart files	0 changed
flutter analyze lib test integration_test	No issues
dart run build_runner build --delete-conflicting-outputs	No drift; the committed .g.dart regenerates identically
test/router + test/providers + the 6 hashtag suites	1556 passed
Full suite, --exclude-tags "integration || golden"	20224 passed, 41 skipped, 0 failed
skip / placeholder / unpinned-unchanged / ungrouped / l10n-delegates ratchets	all pass
Mutation re-check: pinning ShellChrome's currentIndex to 0	4 of the 8 restored app-shell tests go red, green on revert

Two things I had expected to break did not, and are worth recording so nobody re-checks them: the 3.47 formatter left all six files untouched, and the riverpod .g.dart did not move.

I have not merged — this repo merges through the queue and the decision is not mine. @rabble @NotThatKindOfDrLiz, over to you.