fix(#389): guard startup so a failure names the step instead of a blank page - #405
fix(#389): guard startup so a failure names the step instead of a blank page#405Matobi98 wants to merge 3 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
This branch conflicts with What to do on rebase:
|
…ailure surface answers any route
38643a2 to
d89fb01
Compare
|
Rebased onto The three conflicts were formatting — #408 rewrapped lines this branch had also edited. The database step didn't conflict at all, which is why the warning was worth giving: it merged in silently and would have shipped as the only step outside the sequence. It's now
Flutter version: fixed at the root rather than remembered — the other SDK is gone from this machine, so Verified after the rebase: analyze clean, 351 Flutter tests, 438 Rust tests, clippy and wasm clean. Manually in Chrome against the local regtest: app opens, a trade survives a reload, and breaking the database leaves the app up with its log line. One aside: |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Strict review. Verified on the branch (d89fb01): flutter analyze adds no new diagnostics, the 8 new tests pass. The direction is right and the change is small and well reasoned, but two findings contradict the PR's own bar ("a screen that named the same step regardless would pass a careless look and be worthless"), so requesting changes.
HIGH — the step label goes stale after a successful optional step. _optional sets _currentStep = name and never restores it. The node rehydration block, the trade-key mirror and IdentityService.initialize all run labelled opening the local database; ProviderContainer(...), the NWC URI read and _watchConnectionState() run labelled reading relay status — a step that already finished fine. A throw there names a step that did not fail. Structural fix: a _required(name, body) twin for the mandatory stretches, so every stretch carries its own label and there is no code "between labels". (inline)
HIGH — the "deep initial route" test sets no deep route, and the real behaviour on one is wrong. The test is identical to the fourth one (defaultRouteNameTestValue is / by default). I ran it for real with /orders/abc/detail: the Navigator stacks 4 copies of the failure page and canPop() is true, so browser/Android back "navigates" to a clone. Also the "Could not navigate to initial route" message that motivates the change lives inside an assert, so it only ever exists in debug builds. Fix with onGenerateInitialRoutes and a test that actually sets the route. (inline)
MEDIUM — #227 is cited as a case this guard catches. It is not. #370 pinned it inside CanvasKitRenderer.initialize, before main() runs, and says so: "no app-level try/catch around main() can guard it". #227 is the precedent that motivates the guard, not a case it covers. Reword the comment in bootstrapAndRun and the PR body.
MEDIUM — the guard logic has zero behavioural coverage. All four tests in startup_guard_test.dart are greps over the source. Nothing checks that a failed optional step continues, that a failed required step reaches the screen, or that the reported step is the right one (exactly the bug above). Extract a small class with optional, required and currentStep, call it from bootstrapAndRun, and test it with fake bodies. The kIsWeb grep is also not an "enclosing" check (inline).
MEDIUM — the screen shows the step but not the error. On Android/iOS/Linux the user has no console. A secondary, selectable line with error.toString() makes the report actionable without adding a dependency. Suggestion, not blocking.
LOW
- If
WidgetsFlutterBinding.ensureInitialized()throws, the rescuerunAppthrows too. Acceptable edge case, but worth a comment or moving that call outside thetry. - The description drifted after round 1: "3 new tests" (there are 8), the seven-step table omits the database step, and "no
kIsWebanywhere in the change" is no longer true. It becomes the merge reference, so fix it before merging. - Hard-coded English is justified here but contradicts CLAUDE.md ("all user-facing strings are Dart-level l10n"). Add one line under Translations recording the exception so nobody "fixes" it.
- The last-resort catch does not call
markBridgeFailed(e); with it the web smoke test fails fast with the cause instead of timing out. (inline)
| /// One helper rather than a try/catch per step, so every degradation prints the | ||
| /// same prefix — grepping `[startup]` lists everything a run gave up on, in | ||
| /// order, which matters when one failure is the cause of the next. | ||
| Future<void> _optional(String name, Future<void> Function() body) async { |
There was a problem hiding this comment.
_currentStep = name is never restored. After this helper returns successfully, everything up to the next explicit assignment runs under the label of a step that already finished: the rehydrate/identity block as opening the local database, and ProviderContainer(...), the NWC URI read and _watchConnectionState() as reading relay status. A throw there produces exactly the screen the PR calls worthless — one naming a step that did not fail.
Suggest a _required(String name, Future<T> Function() body) twin (sets the label, awaits, rethrows) and using it for RustLib.init, the preferences read and the container build, so no code runs "between labels". Making both helpers methods of a tiny StartupSequence class would also let the guard be unit-tested with fake bodies instead of source greps.
| } catch (e, st) { | ||
| // The failure surface calls runApp too, and that is the whole fix: today an | ||
| // exception in here means runApp never runs and Flutter paints nothing — | ||
| // the page is not broken, it is absent, with no message anywhere (#227). |
There was a problem hiding this comment.
#227 is not an example of what this catch handles. #370 pinned it inside the engine's CanvasKitRenderer.initialize, before main() runs, and states that no app-level try/catch around main() can guard it. Cite it as the motivating precedent, not as a case covered here (same wording in the PR body and in startup_guard_test.dart:18).
| // exception in here means runApp never runs and Flutter paints nothing — | ||
| // the page is not broken, it is absent, with no message anywhere (#227). | ||
| debugPrint('[startup] fatal while $_currentStep: $e\n$st'); | ||
| runApp(StartupFailureApp(step: _currentStep)); |
There was a problem hiding this comment.
Consider markBridgeFailed(e); before runApp here. It is a no-op off web, and on web it lets test/web/smoke/smoke.mjs fail immediately with the cause instead of waiting for the bridge-ready timeout.
| // before falling back. Harmless, but this screen exists to make a failed | ||
| // startup legible; it should not add noise of its own to the one log the | ||
| // person reporting it is about to read. | ||
| onGenerateRoute: (_) => MaterialPageRoute<void>(builder: _buildBody), |
There was a problem hiding this comment.
Verified with defaultRouteNameTestValue = '/orders/abc/detail': Navigator.defaultGenerateInitialRoutes resolves /, /orders, /orders/abc, /orders/abc/detail through this callback and pushes all four, so the failure page is stacked 4 deep and canPop() is true — browser/Android back pops to an identical page.
Also, the "Could not navigate to initial route" report this comment cites is inside an assert(...) in navigator.dart, so it never appears in a release build; the motivation only holds for debug.
onGenerateInitialRoutes: (_) => [MaterialPageRoute<void>(builder: _buildBody)],yields exactly one route for any initial URL.
| expect(find.textContaining('loading the engine'), findsNothing); | ||
| }); | ||
|
|
||
| testWidgets('answers a deep initial route, not just "/"', (tester) async { |
There was a problem hiding this comment.
This test never sets a deep route — defaultRouteNameTestValue is / by default, and wrapping in MediaQuery does not change it — so it is the fourth test again and passes with home: too. To test what the name says:
tester.binding.platformDispatcher.defaultRouteNameTestValue = '/orders/abc/detail';
addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);
await tester.pumpWidget(const StartupFailureApp(step: 'loading the engine'));
expect(tester.takeException(), isNull);
expect(tester.state<NavigatorState>(find.byType(Navigator)).canPop(), isFalse);With the current onGenerateRoute the last expectation fails (4 stacked routes); with onGenerateInitialRoutes it passes.
| ); | ||
|
|
||
| final before = source.substring(0, initDbAt); | ||
| final enclosingWebGuard = |
There was a problem hiding this comment.
Named enclosingWebGuard but it does not check enclosure: it matches any if (!kIsWeb) { anywhere before initDb, so a legitimate native-only step added earlier in the file fails this test with a message about the database. Either scope the search to the _optional('opening the local database' block, or drop this in favour of a behavioural test on an extracted startup sequence.
…r URL, the cause on screen
|
Round 2 — pushed in HIGH — the stale label. Correct, and the fix is the Verified in the app, not only in tests: breaking the container assembly used to produce "It failed while reading relay status". It now says "building the interface". HIGH — the deep initial route. Reproduced your measurement before changing anything: You were also right that the console message lived in an MEDIUM — #227 miscited. Ours to have caught: #370 is where we established the crash fires before MEDIUM — no behavioural coverage. Six tests now drive Two greps remain, and only because running is not available: that the guard reaches MEDIUM — the cause on screen. Added, selectable, capped at 300 characters. Plus a "Copy details" button, since on a phone dragging to select inside a scrolling view is awkward: it puts the step and the cause on the clipboard, because the cause alone loses half of what makes a report actionable. LOW. Three things found while doing this1. A long cause on a short screen overflowed — the striped warning painted over the one screen that has to stay readable. My own addition, an hour old. The content scrolls now, with a test at 320×400. 2. 3. Two more I did not act on, deliberately:
Verified
Manually against the local regtest in Chrome, seven cases: the app opens; a trade survives a reload; the database broken leaves the app up with its log line and persistence gone; One of those runs turned up something unplanned — a page reload produced |
2 participants
Closes #389. Carries the unticked half of #227: #370 fixed the specific cause (an unparseable locale), this makes startup survivable regardless of the cause.
The problem
main.dartisFuture<void> main() => bootstrapAndRun();with no guard, and the stretch ofbootstrapAndRunbeforerunAppruns seven steps. Anything that throws there meansrunAppnever runs: Flutter paints nothing, and the page is not broken — it is absent, with no message anywhere. #227 was exactly this, and finding a one-line cause took a full stack-trace hunt.Everything after
runAppalready degrades — 12catchblocks. This stretch was the outlier.The fix
Each of the seven steps was classified by what the app can still do without it.
Firebase.initializeAppRustLib.initSharedPreferencessetLoggingEnabledonBondSlashednostr_api.initializeFive of seven are optional and now log and continue through one
_optionalhelper, so a run's degradations share a[startup]prefix and read in order — which matters when one failure causes the next.The two that are not optional reach a last-resort
catchthat callsrunAppwithStartupFailureApp. That is the change in one sentence:runAppnow always runs — with the app, or with a screen saying which step failed.SharedPreferencesis a deliberate call. It could degrade to defaults, but then the app opens looking freshly installed: walkthrough again, wrong language, wallet gone. That lies about data loss in an app holding money. A screen saying "it failed while reading your settings" is better than a convincing impostor.Firebase keeps two arms rather than using the helper:
UnsupportedErroris the placeholder config, an expected state, not a failure. Collapsing both into one message would make every run log a "failed" nobody reads by the time it means something. The broadcatchbelow it is what satisfies the second acceptance criterion — a call into a third-party JS SDK can throwFirebaseException, a network error, or anything the SDK likes, and all of those escaped before.The failure surface
lib/core/startup_failure.dart. One screen: "Mostro could not start" and "It failed while <step>."No localization, no app theme, no Rust, no SharedPreferences. Any of those can be what failed, and a rescue surface that needs what broke is a second blank page. Colors are hard-coded for the same reason.
No retry button:
RustLib.initthrows when called twice, so retrying after a failure past that point would fail differently and confuse the report. Worth adding later as a real reload.The step name is the whole point. "Mostro won't open" is unactionable; "it failed loading the engine" is where to look — for the person reporting it and for whoever reads the report. Naming the step is also what the issue asks for: "a minimal error scaffold that names the failing step beats a blank page".
Platform-independent, as the issue asks: no
kIsWebanywhere in the change. Web is where it bites hardest — the in-app log viewer lives inside the app that did not start — but the guard is not web-specific.Test plan
flutter analyze— cleanflutter test— 332 passed, including 3 newcargo test/clippy/cargo check --target wasm32-unknown-unknown— clean (pre-commit hook)Manual, against the local regtest stack in Chrome, breaking each step in turn:
RustLib.initSharedPreferences[startup]lines in orderThe two failure screens saying different things is the assertion that matters — a screen that named the same step regardless would pass a careless look and be worthless.
The three new tests cover the failure screen: that it renders the step it was given, that a different step renders differently (so it cannot be ignoring the value), and that it pumps with no ProviderScope, no localization and no theme. Mutation-checked: replacing the step line with a generic message fails two of the three.
bootstrapAndRunitself is not unit-tested — reaching it needs Rust, preferences and relays, i.e. the whole app assembled to watch it not assemble. That seam is covered by the manual runs above and nothing pretends otherwise.One thing noticed while testing
Probably already known, flagging it only to confirm: with relay init broken, the app opens but the order book spins forever rather than saying it is offline. The startup guard did its job — the app opened, and Settings is reachable to switch node or edit relays — but that surface has no "disconnected" state of its own. Out of scope here; happy to open an issue if there isn't one.