Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
|
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 |
|
Merged
The merge also brought two steps, both optional: |
2 participants
Closes #389. Carries the unticked half of #227.
The problem
main.dartisFuture<void> main() => bootstrapAndRun();with no guard, and the stretch ofbootstrapAndRunbeforerunAppruns a dozen steps. Anything that throws there meansrunAppnever runs: Flutter paints nothing, and the page is not broken — it is absent, with no message anywhere. Everything afterrunAppalready degrades; this stretch was the outlier.#227 is the precedent that motivated this, not a case it covers: that crash fires inside the engine's own
CanvasKitRenderer.initialize, beforemain()runs, which is why #370 fixed it inweb/index.htmland stated plainly that no app-leveltry/catchcould reach it.The fix
lib/core/startup_sequence.dartholds the sequence:currentStep, plus two ways to run a step.optional(name, body)— a step the app can open without. A failure is logged under a shared[startup]prefix and startup continues, degraded.required(name, body)— a step it cannot. The failure propagates to the guard inbootstrapAndRun, which callsrunAppwithStartupFailureAppnaming the step.Both set the label on entry, and that is the reason there are two: with only
optional, the mandatory stretches in between carry no label of their own and run under whichever optional step finished last — so a failure there names a step that succeeded. That was the shape of this PR after round 1, caught in review.Every step, classified by what the app can still do without it:
registering font licencessetting up notificationsloading the enginereading your settingsapplying your log settingsopening the local databaseselecting the Mostro nodemirroring trade key indicesloading your identitysubscribing to bond noticessubscribing to bond claim updatesconnecting to the networkreading relay statusbuilding the interfaceThe three "labelled" ones keep their own bespoke handlers — one of them reports to the web bridge probe — so they announce their name and handle their own failure rather than being forced through a helper.
reading your settingsis a deliberaterequired. It could degrade to defaults, but then the app would open looking freshly installed: the walkthrough again, the device language instead of the one the user picked, and no attempt to reconnect a saved wallet (NWC, not available on web). Showing that as if nothing were wrong misleads about data loss in an app people trade money through. All three measured by forcing the step to read an empty store.opening the local databaseruns on the web too. Since #408 that is where web persistence lives: with this step broken, a trade taken in Chrome is gone after a reload.startup_guard_test.dartfails if it ends up back inside a!kIsWebblock.connecting to the networkstays optional, measured with it forced to fail. The app opens, and the secret words can still be viewed and backed up — they come from secure storage, not from relays. Nothing that needs relays works, though: adding a relay in Settings fails, because the relay API goes through a pool that never started; the order book spins with no message, and My Trades shows empty although the trades are stored. Those screens have no "disconnected" state yet — that is what makes this degradation confusing, not the decision to open.The failure surface
lib/core/startup_failure.dart. One screen: "Mostro could not start", the step that failed, and the cause underneath it — selectable, capped at 300 characters, with a Copy details button that puts both the step and the cause on the clipboard. On Android, iOS and Linux there is no console for the person hitting this to read, and dragging to select inside a scrolling view is awkward on a phone, so a tap is the one action the screen offers. The cause alone would lose half of what makes a report actionable.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. Colours are hard-coded for the same reason, and
CLAUDE.mdnow records the l10n exception so nobody "fixes" it. The content scrolls, so a long cause on a short screen moves out of the way rather than painting Flutter's striped overflow warning over the one screen that has to stay readable.It answers any initial route with exactly one page. On the web the initial route is the browser's URL, and Flutter's default handling pushes one route per path segment:
/orders/abc/detailwould otherwise stack four identical copies, with back popping to a clone.No retry button:
RustLib.initthrows when called twice, so retrying after a failure past that point would fail differently and confuse the report.Platform-independent, as the issue asks — no
kIsWebin the guard itself. Web is where it bites hardest, because the in-app log viewer lives inside the app that did not start.Test plan
flutter analyze— cleanflutter test— 1209 passed after mergingmain, including 17 from this PR: 6 behavioural overStartupSequence, 9 over the failure surface, 2 static over the wiringcargo test/clippy/cargo check --target wasm32-unknown-unknown— clean (pre-commit hook)The behavioural tests drive the sequence with fake bodies: that a failing optional step lets startup continue, that a failing required step propagates, and that the step reported is the one that failed rather than the last that succeeded. Mutation-checked: dropping the label from
requiredfails two of them, removingonGenerateInitialRoutesfails the deep-route test, dropping the truncation fails the small-screen one, and puttinginitDbback behind!kIsWebfails the static one.Two things stay static greps, in the spirit of
pages_bundle_test.dart: that the guard reachesrunAppwith the current step, and the!kIsWebcheck, scoped to the database step rather than the whole file. ReachingbootstrapAndRunneeds Rust, preferences and relays — the whole app assembled to watch it fail to assemble — and nothing here pretends otherwise.The manual round, breaking one step at a time. Re-run on the merged code:
mostro-cli, both completed[startup]line eachRustLib.initRustLib.init, from a deep URLFrom the earlier round, before the merge:
[startup]line, persistence gone until restoredSharedPreferencesReloading the page 30 times on a release build gave no blank page. In a long-lived
flutter runsession one reload came up blank with a flutter_rust_bridge panic inTradeKeyIndexStream; a fresh dev server, onmainand on this branch alike, gave none in 18 reloads each.Known, not addressed here
[startup]degradations reachdebugPrintonly. The in-app log viewer shows Rust's logs, and Dart has no way to write into it, so "grep[startup]to see what a run gave up on" holds only with a dev console attached. Not a regression; worth knowing before anyone relies on it in a bug report.optionalcatches everything, so a programming error in an optional step is swallowed and startup continues in an unknown state. Narrowing it to exceptions would turn those into hard failures — a behaviour change that predates this PR and deserves its own decision.