Skip to content

feat(recoverbull): explain Tor connection progress - #2585

Merged
ethicnology merged 2 commits into
developfrom
feat/tor-recoverbull-status-ui
Aug 18, 2026
Merged

feat(recoverbull): explain Tor connection progress#2585
ethicnology merged 2 commits into
developfrom
feat/tor-recoverbull-status-ui

Conversation

@ethicnology

Copy link
Copy Markdown
Member

Stacked on the feat/tor-persistence PR. Review that one first.

The connecting screen is what the user stares at for the whole Tor bring-up, which takes 40s or more on a cold directory. It rendered an empty body on device: the title, both phase rows and the reassurance line were laid out but never painted.

It now shows what the data can honestly support: two named phases — Tor network, then RecoverBull server — an elapsed counter, and, when arti reports a blockage that outlives a short grace period, an explanation that the network rather than the app is the problem.

Two omissions are deliberate:

  • No separate "internet" phase. Arti's connectivity flag is tri-state and its "no blockage" answer covers both "fine" and "too early to say", so the only positive proof of connectivity is Tor becoming ready — which the next row already reports. A missing network surfaces as the reason Tor is stuck.
  • No named bootstrap sub-steps. The conn/dir split that would identify the exact phase is private upstream, so naming one would be wrong a large fraction of the time.

The grace period exists to ride out a transient readiness dip, so it deliberately does not apply to a terminal failure: TorUnavailable carries its diagnostic straight through, otherwise the reason was dropped exactly when it stopped being provisional and the user was told to "try again" with no explanation.

Also fixes a navigation defect in this screen: readiness is not a single event — Tor republishes TorReady on every directory refresh and the key-server check emits its own states — so the success condition is satisfied repeatedly and each repetition pushed another route, stacking duplicate pages behind the visible one. Guarded, with a test that replays the device sequence.

@ethicnology ethicnology self-assigned this Aug 10, 2026
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 7m 10s —— View job


Code review

  • Gather context (diff vs feat/tor-persistence)
  • Review connecting_page.dart changes
  • Review localization + test changes
  • Verify PR claims (grace period, TorUnavailable passthrough, navigation guard)
  • Post findings

Note: posting inline review comments (gh api) is not in my allowed tools for this run, so the findings are consolidated here with file/line references. The runner also has no Flutter SDK, so I could not run flutter analyze or the new tests — findings are from static review.

Summary

The rewrite is careful and well-documented, and most of the PR's claims check out: the navigation dedup guard is correct, TorUnavailable carries its diagnostic through untouched by the grace period, the retry handler resets failure/keyServerStatus in the bloc so the panel clears, and every referenced l10n key exists. But the headline feature — showing the blockage explanation after the 5 s grace period while Tor is still connecting — appears to never fire, because the only render path for it is gated behind a failed phase.

🔴 The grace-period blockage explanation never renders while Tor is still connecting

lib/features/recoverbull/ui/pages/connecting_page.dart:256-257, 211-217, 337

_diagnostic (the settled blockage, gated by showBlockage/_blockageIsSettled) is only read inside _failureMessage, which is only reached from the _FailurePanel — and that panel is gated by _hasFailure, which only checks for a failed phase:

bool get _hasFailure =>
    _torPhase == _PhaseState.failed || _serverPhase == _PhaseState.failed;

In the mainline scenario this PR describes (device offline, TorConnecting with TorDiagnostic.offline, keyServerStatus still unknown_onTorInitialization resets it to unknown at bloc.dart:131), _torPhase is active and _serverPhase is pending, so _hasFailure is false and nothing is shown until the bootstrap terminally fails with TorUnavailable via the backend timeout — potentially minutes later. That contradicts both the PR description ("when arti reports a blockage that outlives a short grace period, an explanation…") and the _blockageGrace doc comment ("How long a blockage must persist before it is shown as a failure"). As written, _diagnostic is only reachable in the narrow window where keyServerStatus is already offline while torConnection has flipped back to TorConnecting-with-diagnostic.

Suggested fix:

bool get _hasFailure =>
    _torPhase == _PhaseState.failed ||
    _serverPhase == _PhaseState.failed ||
    _diagnostic != null;

The tests cover the TorUnavailable bypass path but not this one; a widget test that pumps a TorConnecting(diagnostic: TorDiagnostic.offline) state past the 5 s grace and expects recoverbullTorOffline + Retry would have caught it. Fix this →

🟡 The elapsed counter is misattributed to the server phase and never resets on retry

lib/features/recoverbull/ui/pages/connecting_page.dart:323, 329, 355

A single page-lifetime _elapsed is handed to whichever phase card is currently active. Two consequences:

  • When the server phase becomes active after a 50 s bootstrap, its detail line starts at 0:50, attributing the whole bootstrap to the key-server check.
  • After a failed attempt and a Retry press, the timer keeps counting from page mount, so the ≥20 s reassurance line ("This is the longest step…") appears immediately on the fresh attempt, and the Tor card's timer resumes at the old value.

Tracking a phase start timestamp (reset when the active phase changes and on retry) would make both readings honest. Fix this →

🟢 Minor: the mount-time state is not processed by the listener

lib/features/recoverbull/ui/pages/connecting_page.dart:147-151

BlocListener only reacts to emissions after it subscribes, so a TorConnecting-with-diagnostic already present when the page mounts doesn't start the grace clock until the next tor emission. Navigation is unaffected in practice (the key-server transitions still fire the listener, and the bloc is created fresh per flow in flow.dart), but seeding _onStateChanged with bloc.state in initState would remove the dependence on arti re-emitting.

Verified while reviewing

  • The _hasNavigated guard correctly collapses repeated TorReady/online satisfactions into one pushReplacement; the tor state classes use identity equality, so the listener does fire on every republish as the test assumes.
  • _failureDiagnostic only destructures TorBootstrapFailure; other terminal failures (TorBootstrapTimeoutFailure, TorStorageFailure, …) fall through to the generic message via the keyServerStatus == offline branch, which the bloc guarantees on TorUnavailable (bloc.dart:152). That looks intended.
  • No security concerns: TorFailure.logMessage is never rendered, and the new strings carry no sensitive data.

@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch 2 times, most recently from 0a59df7 to 71138e1 Compare August 10, 2026 21:02
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 71138e1 to 020ab85 Compare August 10, 2026 21:46
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 020ab85 to 35f244b Compare August 11, 2026 00:52
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch 2 times, most recently from 3cf4541 to 887acf4 Compare August 11, 2026 03:04
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 887acf4 to 3205859 Compare August 17, 2026 17:08
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 3205859 to 5077823 Compare August 17, 2026 18:28
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 5077823 to 4284613 Compare August 18, 2026 15:05
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 4284613 to 81af8b7 Compare August 18, 2026 15:07
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 81af8b7 to 47e3372 Compare August 18, 2026 15:36
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 47e3372 to 4334d76 Compare August 18, 2026 15:37
Base automatically changed from feat/tor-persistence to develop August 18, 2026 19:44
@ethicnology
ethicnology force-pushed the feat/tor-recoverbull-status-ui branch from 4334d76 to 09374a0 Compare August 18, 2026 19:44
@ethicnology
ethicnology merged commit 236b40e into develop Aug 18, 2026
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant