Skip to content

docs: bannerBuilder cannot return null — document how to hide the banner - #27

Merged
fonkamloic merged 15 commits into
mainfrom
fix/banner-builder-docstring
Aug 21, 2026
Merged

docs: bannerBuilder cannot return null — document how to hide the banner#27
fonkamloic merged 15 commits into
mainfrom
fix/banner-builder-docstring

Conversation

@fonkamloic

Copy link
Copy Markdown
Contributor

Fixes #25 via the issue's option 1 (docstring fix — the smaller change, and the one matching what callers can do today).

The old docstring promised Return null to hide the banner, but the ? is on the function type, not its return: (c, r, d) => null does not compile, bannerBuilder: null selects the default banner, and the call site feeds the result straight into Positioned(child: …). There was no way to do what the doc instructed.

New doc states: the builder must return a widget; return const SizedBox.shrink() to show nothing while driving your own update UI (CodePush.checkForUpdate / CodePush.status); onDismiss hides the banner slot until the next ready update (verified against the _updateReady latch at the call site).

Why not option 2 (Widget? Function(...)? + call-site guard): it is the nicer API, but it is a public API semantic change on a published package, and the docs site was already corrected to describe option-1 behavior in code-push-website #25 — shipping option 2 would re-open that row. If option 2 is wanted later it can supersede this cleanly; the issue stays the record of that choice.

Doc + CHANGELOG only; no behavior change. dart analyze: the 6 info-lints are pre-existing on main (verified by stash).

… banner (#25)

The docstring told callers to 'return null to hide the banner', but the
nullable ? is on the FUNCTION, not its return: (c, r, d) => null does
not compile, and bannerBuilder: null selects the default banner. The
call site feeds the result straight into Positioned(child: ...), so a
null return could never be tolerated either.

Ship the doc the type can keep (issue option 1): the builder must
return a widget; SizedBox.shrink() is how you render nothing while
driving your own update UI from CodePush.checkForUpdate or
CodePush.status; onDismiss hides the banner slot until the next ready
update. The docs site already describes this behavior
(code-push-website #25), so no follow-up is created there.
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. This is doc + CHANGELOG only — no behavior or API-signature change, so nothing here can break existing users or the update path. The core claim of the PR checks out: bannerBuilder is Widget Function(...)? (lib/src/code_push.dart:2408-2412) — the ? is on the function, not the return — and the call site feeds the result straight into Positioned(child: …) (lib/src/code_push.dart:2526-2543), so the old "Return null to hide the banner" was never expressible.

🟠 Medium

  • lib/src/code_push.dart:2405 — the new doc steers callers to an API that throws, on the platform where it's most likely to fail. The replacement text suggests driving your own UI "for example from [CodePush.checkForUpdate]". That method talks to the custom-engine channel flutter/codepush (lib/src/code_push.dart:16-19) and is not failure-soft: any channel error, including MissingPluginException on a stock engine, is caught and rethrown as CodePushException (lib/src/code_push.dart:1603-1607) — contrast isPatched, which swallows and returns false (lib/src/code_push.dart:1647-1653). On iOS the engine updater is disabled (lib/src/code_push.dart:1656-1657, and the comment at :497), so a caller who follows this doc from a builder or initState can get an unhandled exception. The overlay's own update path uses CodePush.checkAndInstall(...) plus the status notifier (lib/src/code_push.dart:2481-2492); pointing at that (or at status alone) would be both accurate and safe. Since the point of this PR is that the docstring told people to do something they can't, it's worth not swapping in a second recommendation with a sharp edge.

🟡 Low

  • README.md:266-276 — the README's bannerBuilder example still says only "optional, return a custom banner widget". With the docs site already corrected, the README is the third copy of this doc and now the only one without the "return const SizedBox.shrink() to show nothing" line. One comment line keeps all three in sync.
  • No test pins the documented contract. There is no widget-test coverage for CodePushOverlay at all. A small testWidgets asserting (a) a custom bannerBuilder's widget is what renders when an update is ready and (b) onDismiss removes it would make this docstring enforced rather than aspirational, and would catch the drift that produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 in the first place.
  • CHANGELOG.md:1## Unreleased is a new convention for this file (every prior heading is a version). Harmless, but it needs renaming at release time: pubspec.yaml is still version: 0.1.11, and pana expects the top entry to match the published version.
  • lib/src/code_push.dart:2406-2407 wording — "hides the banner slot until the next ready update" is right but slightly generous: once a patch is installed and pending restart, _isPatchAlreadyInstalled short-circuits later checks with return false before onUpdateReady can fire again (lib/src/code_push.dart:569-577). So it's the next different patch, not the next check cycle.

🟢 Positives

  • Correct root-cause diagnosis: the PR body pins down exactly why (c, r, d) => null doesn't compile and why bannerBuilder: null selects the default rather than hiding — that distinction is the whole of CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25.
  • Choosing the docstring fix over Widget? Function(...)? is the right call for a published package: option 2 is a public API semantic change, and it would have re-opened the just-corrected docs-site row. The PR body records the tradeoff so it can be revisited deliberately.
  • Every factual claim in the new docstring is verifiable against the call site — the default-banner fallback, the onDismiss_updateReady = false latch, and the fact that a SizedBox.shrink() inside that Positioned renders nothing hit-testable.
  • CHANGELOG entry is written for users (what changed, what to do instead) rather than as a commit log.

…DME copy; tighten onDismiss wording

The round-1 doc steered callers to CodePush.checkForUpdate, which talks
to the engine channel and rethrows as CodePushException on engines
without code push — a sharp edge exactly where the doc is most needed.
Listen to CodePush.status instead, as the overlay itself does.
README's bannerBuilder example (the third copy of this doc) gains the
same must-return-a-widget line; 'until the next ready update' is
tightened to 'until the next NEW patch becomes ready' (an installed
patch pending restart short-circuits re-offers of itself).
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round-2 push a134941 answers the review:

  • Medium (checkForUpdate sharp edge): fixed. The doc now points at listening to CodePush.status — the overlay's own failure-soft path — and explicitly names why checkForUpdate is NOT the example (it throws CodePushException on engines without code push).
  • README third copy: fixed — the bannerBuilder example carries the same must-return-a-widget + SizedBox.shrink() line, so all three copies (docstring, docs site, README) now agree.
  • onDismiss wording: fixed — "until the next NEW patch becomes ready" (an installed patch pending restart short-circuits re-offers of itself via _isPatchAlreadyInstalled).
  • Widget-test Low: TABLED → filed as #29. There is no injectable seam today (initState runs the live CodePush.init; _updateReady has no test hook), so a real test needs a deliberately designed testability hook in production code — out of scope for a docs PR, tracked with the design sketch in the issue.
  • ## Unreleased heading Low: TABLED on the thread — it is renamed to the version number at release time (the point where pana's top-entry-matches-version check actually runs); keeping unreleased entries under a version heading before the bump would mislabel them if the next release number changes.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 2 (a134941) is still doc + CHANGELOG only — no signature change, no behavior change, and the update/rollback path is untouched, so nothing here can break existing users or brick a host app. The three round-1 items marked fixed are verified fixed:

  • The checkForUpdate sharp edge is gone from the recommendation (lib/src/code_push.dart:2405-2407).
  • README now carries the same contract line (README.md:272-274), so docstring / docs site / README agree.
  • The onDismiss wording is now precise: "until the next new patch becomes ready" matches the code — onDismiss sets _updateReady = false (lib/src/code_push.dart:2537), and it can only go true again via onUpdateReady (:2459, :2489), which _isPatchAlreadyInstalled short-circuits for the patch already installed (:568-577).

🟠 Medium

  • lib/src/code_push.dart:2405CodePush.status is not how the overlay knows an update is ready, and it isn't a UI-grade API. Full disclosure: round 1 suggested status as the safe alternative; reading the state machine more closely, that suggestion was half right (it is failure-soft) and half wrong (it isn't the readiness signal). Two problems with the current text:

    1. "as the overlay itself does" isn't accurate for the banner. The overlay does add a status listener (:2448), but only to flip _patchActive on the literal string 'Patch active' (:2464-2468). The banner — the thing bannerBuilder builds — is gated on _updateReady, which is set only by the onUpdateReady callback passed to CodePush.init (:2451-2460) and CodePush.checkAndInstall (:2483-2491). A caller who follows this doc literally will not reproduce the overlay's behavior.
    2. status is declared as a debug channel. :161-162: /// Debug status notifier — shows what code push is doing. Its values are free-form, interpolated strings — 'Restart to apply', 'Downloading patch...', 'No update (404)', 'Patch already installed', 'Error: $e'. To drive real UI off it a caller must string-match, which silently promotes every one of those literals to public API and makes any future status-text edit a breaking change for them.

    CodePush.init's own docstring already names the right primitive (:252-253): "When a patch is installed, onUpdateReady is called so you can prompt the user to restart." Suggested replacement for the clause: "…and drive your own update UI instead — pass onUpdateReady: to CodePush.init (or CodePush.checkAndInstall) to learn when a patch is ready, exactly as this overlay does." That is failure-soft (both entry points funnel errors into status rather than throwing, :844), accurate against the call site, and typed rather than stringly. Given that this PR exists precisely because a docstring described something the API couldn't do, it's worth not landing a second one in the replacement.

🟡 Low

  • lib/src/code_push.dart:1586 — the "it throws" warning is on the wrong symbol. The new parenthetical warns that CodePush.checkForUpdate throws on engines without code push. That's correct (:1587-1610 rethrows everything as CodePushException, including MissingPluginException), but it now lives in the bannerBuilder docstring, while checkForUpdate's own doc is still the bare one-liner "Checks the engine for available updates (delegates to Dart side HTTP)." Anyone reading checkForUpdate directly — the likely path — still gets no warning. Moving or duplicating it there is one line and puts the caveat where it's discoverable. If the Medium above is taken, the parenthetical becomes a non-sequitur in bannerBuilder anyway.
  • lib/src/code_push.dart:2401-2409 — one 9-line sentence. The onDismiss clause is semicolon-chained onto the SizedBox.shrink() clause even though the two are unrelated, and the parenthetical's "it" reads as referring to the overlay rather than to the listening approach. Three short paragraphs (null → default banner / how to show nothing / what onDismiss does) would read better in generated dartdoc, where this renders as a wall.
  • CHANGELOG.md:3 names what stopped working but not the replacement mechanism. "…return const SizedBox.shrink() and use onUpdateReady: to drive your own UI" closes the loop for a reader who only ever sees the changelog.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test — both explicitly tabled on the thread, and #29 records the testability seam a real test needs first.

🟢 Positives

  • Correct root-cause diagnosis. The distinction the PR body draws — ? on the function type vs. the return type, and bannerBuilder: null selecting the default banner rather than hiding — is the entirety of CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25, and it is verifiable at :2410-2414 and :2528-2543.
  • Right call on scope. Choosing the docstring over Widget? Function(...)? avoids a public-API semantic change on a published package and avoids re-opening the just-corrected docs-site row. The tradeoff is recorded in the PR body so it can be revisited deliberately rather than rediscovered.
  • Round-2 turnaround is genuinely responsive, and the tabled items were filed (CodePushOverlay has no widget-test coverage — bannerBuilder contract is unpinned #29) with a design sketch instead of quietly dropped — that is the right way to defer.
  • The onDismiss claim is now precise to the state machine, not just plausible: it distinguishes "next check cycle" from "next new patch", which required actually reading the _isPatchAlreadyInstalled short-circuit.
  • Three copies of the doc now agree. Drift across README / dartdoc / docs site is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; fixing all three in one PR is the fix that actually holds.

…s debug channel

Round 2 steered readers at CodePush.status — which is failure-soft but
is (a) a declared debug channel of free-form strings and (b) not what
gates the banner: _updateReady flips only via the onUpdateReady
callbacks on init/checkAndInstall. All three doc copies (docstring,
README, CHANGELOG) now name onUpdateReady:, the docstring is split
into three short paragraphs, and checkForUpdate's own doc carries the
throws warning where readers of THAT symbol will actually see it.
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round-3 push answers round 2:

  • Medium: fixed with your suggested primitive. All three copies now point at onUpdateReady: on CodePush.init / CodePush.checkAndInstall — the callback that actually gates the overlay's banner — instead of the status debug channel. (Appreciated the self-correction; the string-matching-promotes-literals-to-API argument is exactly right.)
  • Low (warning on the wrong symbol): fixedcheckForUpdate's own doc now carries the throws warning and points at the failure-soft alternative; the bannerBuilder parenthetical is gone with the rewrite.
  • Low (9-line sentence): fixed — three short paragraphs (default / show-nothing / onDismiss).
  • Low (CHANGELOG replacement mechanism): fixed — the entry names onUpdateReady:.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 3 remains doc + CHANGELOG only — no signature change, no behavior change, the update/download/rollback path is untouched. The three round-2 items marked fixed are verified fixed:

  • The throws-warning now lives on checkForUpdate itself (lib/src/code_push.dart:1588-1591) and is accurate: MissingPluginException from _channel (:16-19) falls through the bare catch (e) at :1610 and is rethrown as CodePushException.
  • The status debug-channel recommendation is gone; all three copies now name onUpdateReady: (lib/src/code_push.dart:2410-2414, README.md:272-274, CHANGELOG.md:3).
  • The 9-line sentence is now three paragraphs.

🟠 Medium

  • lib/src/code_push.dart:2411-2413 — "pass onUpdateReady: to CodePush.init … exactly as this overlay does" is not something a CodePushOverlay user can actually do. I suggested this primitive in round 2; reading the init lifecycle rather than just the callback plumbing, it doesn't survive contact with the overlay.

    CodePushOverlay.initState calls CodePush.init itself (:2459-2469) with its own onUpdateReady closure. init cancels the existing timer and bumps the session epoch (:282-284), so an app that calls CodePush.init(onUpdateReady: mine) in main() and then wraps runApp in CodePushOverlay — the exact reader of this docstring — has its callback superseded: the periodic timer is now the overlay's (:405-413), and the resume-path checkAndInstall passes the overlay's callback too (:2496-2498). The caller's callback survives only on the one initial post-crash-protection check (:396-402) that raced ahead of the overlay's, and even that is decided by the global _checkInFlight single-flight guard (:442) — whichever check starts first installs the patch, and the loser returns false without firing anything. So the caller's UI fires sometimes, on first launch only, nondeterministically.

    Calling CodePush.checkAndInstall(onUpdateReady: ...) manually (as example/lib/main.dart:62-79 does) is genuinely reliable, but only for apps not using the overlay — inside the overlay the same _checkInFlight race applies, and if the overlay's timer wins, the user sees _updateReady flip, their SizedBox.shrink() render, and no notification.

    There is no seam to point at instead: CodePushConfig (:2341-2362) carries no onUpdateReady, so the overlay cannot forward a caller-supplied callback. The accurate thing to say is that the builder is itself the readiness signal — it is invoked only when _updateReady is true (:2536-2546) — plus a note that the overlay calls CodePush.init in initState and supersedes any earlier one. Something like:

    To show no banner at all, return const SizedBox.shrink(). The builder is itself the "patch ready" signal — it is only invoked once a patch is installed and awaiting restart — so drive your own UI from there, using the onRestart and onDismiss callbacks you are handed. Note that CodePushOverlay calls CodePush.init itself, superseding any earlier init (including its onUpdateReady:); apps that want to own the update lifecycle should drive CodePush.init / CodePush.checkAndInstall directly instead of using this widget.

    Flagging this at Medium rather than Critical because nothing breaks at runtime — but it is the same failure mode as CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 (a docstring describing something the API doesn't support), which is why it is worth not landing. The same clause needs the same treatment in README.md:274, which currently says "use onUpdateReady: on CodePush.init" inside a CodePushOverlay example — the one place it is least true. A CodePushConfig.onUpdateReady field would be the real fix, and is a natural companion to the CodePushOverlay has no widget-test coverage — bannerBuilder contract is unpinned #29 testability seam.

🟡 Low

  • lib/src/code_push.dart:1590-1591 — the suggested alternative is not equivalent to what checkForUpdate does. checkForUpdate is a read-only query returning UpdateInfo; init / checkAndInstall download, verify, and install the patch before onUpdateReady fires (:795-841). A reader who wants a failure-soft check is being pointed at something that mutates device state. Half a clause fixes it: "…note that these download and install the patch, unlike this check-only call."
  • CHANGELOG.md:3 — one ~300-character line. The prior entries are short bullets, and this renders on pub.dev as a wrapped paragraph. Splitting "what changed" from "what to do instead" would match the file's existing shape.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test — both explicitly tabled on the thread, with #29 recording the seam a real test needs first.

🟢 Positives

  • The throws-warning moved to the right symbol. checkForUpdate's doc was the bare one-liner "Checks the engine for available updates"; anyone reading it directly now learns it throws before they call it from initState. That is the discoverable location, and it took acting on a Low rather than deferring it.
  • Correct root-cause diagnosis, still. ? on the function type vs. the return type, and bannerBuilder: null selecting the default banner rather than hiding it, is the entirety of CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 — verifiable at :2418-2422 and :2541-2550.
  • Right call on scope. Docstring over Widget? Function(...)? avoids a public-API semantic change on a published package and avoids re-opening the just-corrected docs-site row, with the tradeoff recorded in the PR body so it can be revisited deliberately.
  • The onDismiss claim is precise to the state machine, not merely plausible: "next new patch" is exactly right — onDismiss clears _updateReady (:2545), it can only be set again by onUpdateReady (:2467, :2497), and _isPatchAlreadyInstalled short-circuits re-offers of the already-installed patch (:569-577).
  • Three copies kept in sync in one PR. Drift across README / dartdoc / docs site is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; fixing all three together is the fix that actually holds — and it is why the Medium above is catchable now rather than in the next issue.
  • Tabled items were filed, not dropped. CodePushOverlay has no widget-test coverage — bannerBuilder contract is unpinned #29 carries a design sketch for the testability seam instead of a silent deferral.

…edes an earlier init

Round 3's onUpdateReady: recommendation doesn't survive the overlay's
own lifecycle: CodePushOverlay.initState calls CodePush.init itself,
cancelling the timer and superseding a caller's callback (which then
fires at most once, nondeterministically, via the single-flight race).
The accurate contract: bannerBuilder is only invoked once a patch is
installed and awaiting restart — the builder call IS the signal; apps
that want to own the lifecycle drive init/checkAndInstall directly
instead of the overlay. All three copies aligned; checkForUpdate's
alternative note now says those calls install, unlike the check-only
query.
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round-4 push: adopted your builder-is-the-signal framing across all three copies (docstring, README, CHANGELOG), added the overlay-supersedes-earlier-init note, and the checkForUpdate alternative now says those calls install the patch, unlike the check-only query (round-3 Low). The CodePushConfig.onUpdateReady field idea is recorded on #29 as the companion to the testability seam.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 4 is still doc + CHANGELOG only — no signature change, no behavior change, the check/download/install/rollback path is untouched. The three round-3 items marked fixed are verified fixed:

  • checkForUpdate's alternative now says those calls install the patch (lib/src/code_push.dart:1590-1593), which matches the code: onUpdateReady fires only after installPatch on Android (:841-843) or after the iOS persist path (:786-792), whereas checkForUpdate is a read-only channel query.
  • The unreliable "pass onUpdateReady: to CodePush.init … exactly as this overlay does" recommendation is gone, replaced by the builder-is-the-signal framing.
  • The supersedes note (:2415-2419) is accurate: the overlay calls CodePush.init in initState (:2469-2477), and init cancels the prior timer (:282) and bumps _initEpoch (:284), so an earlier session's pending chain bails at :319.

🟠 Medium

  • lib/src/code_push.dart:2410-2414 — "drive your own UI from there" is advice to run imperative UI inside build, which Flutter forbids. The builder is invoked at :2550, inside _CodePushOverlayState.build, under if (_updateReady) (:2544). Two consequences the new text doesn't warn about:

    1. A caller who follows it literally gets an assertion. The natural reading of "the builder is itself the 'patch ready' signal — so drive your own UI from there, using the onRestart and onDismiss callbacks you are handed" is: return const SizedBox.shrink(), then showDialog(...) / Navigator.push(...) — or call the handed onDismiss once you have taken over. All three throw from a builder. onDismiss is () => setState(() => _updateReady = false) (:2553); calling it synchronously in the builder is setState() during build. showDialog during build is the standard !_debugLocked assertion.
    2. "only invoked once" is not true, and the surrounding sentence invites reading it as "one time". It is a plain build callback: it re-runs on every parent rebuild, on the _patchActive setState fired by the status listener (:2479-2483), and — because :2548 reads MediaQuery.of(context).padding.bottom — on every MediaQuery change, i.e. every keyboard show/hide and rotation. A one-shot side effect written into the builder fires repeatedly.

    This is the same failure mode as CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 — a docstring describing a use the API does not support — which is the reason not to land it in the fix for CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25. The framing itself is right; it just needs the build-phase constraint attached, e.g.:

    To show no banner at all, return const SizedBox.shrink(). The builder call is itself the "patch ready" signal — it runs only once a patch is installed and awaiting restart — but it runs during build and may be called many times (rebuilds, metrics changes), so it must return a widget and must not perform side effects. To drive your own UI, return your own widget and do the work in its initState (or defer with WidgetsBinding.instance.addPostFrameCallback), calling the handed onRestart / onDismiss from there.

    README.md:271-275 carries the same clause ("drive your own UI from here with the onRestart/onDismiss callbacks you are handed") and needs the same qualifier; CHANGELOG.md:3 likewise.

🟡 Low

  • README.md:269-278 — the supersedes note did not make it into this copy. Round 4 reports the overlay-supersedes-earlier-init note added "across all three copies", but the README hunk only carries the must-return-a-widget / SizedBox.shrink() / builder-is-the-signal lines; the docstring (:2415-2419) and CHANGELOG.md:3 have it, README does not. Not a correctness problem — README's previously-wrong onUpdateReady: clause is gone, which was the round-3 ask — but README is the copy most likely to be read by someone about to call CodePush.init in main() and wrap runApp in the overlay, which is exactly the case the note exists for.
  • CHANGELOG.md:3 is now a single ~500-character line, up from ~300 when this was raised in round 3. Every other entry in the file is a short bullet, and pub.dev renders this as one wrapped block. Splitting it into "what changed" / "what to do instead" / "overlay owns init" would match the file's shape; the content itself is right.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test — both tabled on the thread, with #29 holding the testability seam (and now the CodePushConfig.onUpdateReady field) that a real test needs first.

🟢 Positives

  • The builder-is-the-signal framing is the accurate one. It replaced two successive recommendations that did not survive contact with the code (checkForUpdate throws; status is a stringly debug channel; a caller's onUpdateReady is superseded by the overlay's init). Grounding the doc in the one thing the caller is actually handed — the builder invocation — is the fix that cannot drift, and the Medium above is a qualifier on it rather than a fourth replacement.
  • The supersedes note is the genuinely useful new fact here, and it is verifiable end to end (:2469-2477:282:284:319). It documents a real footgun for the init-in-main() + CodePushOverlay combination that no other doc mentions.
  • The checkForUpdate warning landed on the right symbol with the right caveat. MissingPluginException from the flutter/codepush channel (:16-19) falls through the bare catch (e) at :1611 and is rethrown as CodePushException, and the added "those download and install the patch, unlike this check-only call" prevents the alternative from being read as a drop-in.
  • onDismiss is described precisely to the state machine — "next new patch": onDismiss clears _updateReady (:2553), only onUpdateReady sets it (:2475, :2505), and _isPatchAlreadyInstalled short-circuits re-offers of the installed patch (:569-577).
  • Four rounds, three copies, still in sync. Doc drift across README / dartdoc / docs site is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; the discipline of touching all three every round is what makes each round's finding catchable here rather than in the next issue.

@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 5 fixes pushed (221f16d):

  • Medium (side effects in build): fixed in all three copies. The docstring now states the builder runs during build and may be called many times (parent rebuilds, media-query changes), must return a widget, must not perform side effects (synchronous onDismiss, showDialog, navigation all throw), and shows the supported pattern — do the work in your returned widget's initState or a post-frame callback, calling the handed onRestart/onDismiss from there. README carries the short form of the same qualifier; CHANGELOG carries it as its own bullet. The "it runs only once a patch is installed and awaiting restart" phrasing is now immediately followed by the many-invocations caveat so it can no longer be read as "one time".
  • Low (README missing supersedes note): fixed — the note (overlay's init supersedes an earlier one, incl. onUpdateReady:) is now under the overlay code sample in README, matching the docstring and CHANGELOG.
  • Low (CHANGELOG single long line): fixed — split into a parent bullet with three short sub-bullets (null-return correction / build-phase constraint / supersedes note).

dart analyze — only the 6 pre-existing avoid_print infos, none introduced.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 5 is still doc + CHANGELOG only — no signature change, no behavior change, the check/download/install/rollback path is untouched. The three round-4 items marked fixed are verified fixed:

  • The build-phase constraint is now attached to the builder-is-the-signal framing (lib/src/code_push.dart:2413-2422), and the "may be called many times" claim is accurate: the builder is invoked inside _CodePushOverlayState.build (:2555-2560), which reads MediaQuery.of(context) at :2554 (whole-data dependency, so keyboard/rotation rebuild it) and is also rebuilt by the _patchActive setState from the status listener (:2489-2493).
  • The supersedes note made it into the README copy (README.md:283-287).
  • CHANGELOG.md:3-6 is now three sub-bullets matching the file's shape.

🟠 Medium

  • lib/src/code_push.dart:2418-2419 — "calling onDismiss synchronously … from inside the builder throws" is not what happens; it silently half-works, which is the worse failure. onDismiss is () => setState(() => _updateReady = false) (:2559), and the builder is called from within _CodePushOverlayState.build (:2555-2558), so the target of that setState is the element currently being built. State.setState has no build-phase assert of its own; it runs fn() and calls _element.markNeedsBuild(). The "setState() or markNeedsBuild() called during build" assertion in markNeedsBuild only fires when the marked element is not in scope of owner._debugCurrentBuildTarget — here it is that target, so _debugIsInScope returns true and the assert passes. Execution then reaches if (dirty) return;, and the element's dirty flag is still set during its own build() (ComponentElement.performRebuild clears it via super.performRebuild() in the finally after build() returns — the framework comment there says this is deliberate, precisely so that markNeedsBuild during build is a no-op).

    Net effect: no exception, in debug or release. _updateReady flips to false, this frame still renders the banner the builder just returned, and no rebuild is scheduled — so the banner lingers until some unrelated rebuild happens. (In the one case where the overlay is being rebuilt by its parent with force: true, dirty is false and it schedules a rebuild instead — still no throw.) A developer who tries it sees no crash, concludes the warning was overcautious, and ships the stale-banner bug. Suggested wording for that clause: "calling onDismiss from inside the builder does nothing useful — the overlay is already building, so the banner still renders this frame and stays until the next rebuild; showDialog and navigation throw."

  • lib/src/code_push.dart:2419-2422 / README.md:271-276 — deferring to initState or a post-frame callback does not make showDialog/navigation work from a banner. The banner is a Positioned sibling of widget.child inside the overlay's own Stack (:2523 vs :2550-2560), and CodePushOverlay is documented and exampled as wrapping above MaterialApp (README.md:68-76, example/lib/main.dart:11-20). So the BuildContext handed to bannerBuilder — and every context inside the widget it returns — has no Navigator, no Overlay, and no MaterialLocalizations ancestor. showDialog(context: context, …) and Navigator.of(context) throw from there at any time, not just during build; the post-frame deferral fixes the build-phase constraint but not this one. Since the doc now steers people toward "drive your own UI", it is worth one clause: dialogs/routes need a navigatorKey on the app's MaterialApp (or a context from inside the app), not the builder's context.

🟡 Low

  • lib/src/code_push.dart:2421-2422 — "do the work in its initState … calling the handed onRestart / onDismiss from there" reads as an instruction to invoke them unconditionally. onRestart is CodePush.restart, which is exit(0) (:872) — a hard process kill. Called from a banner widget's initState, that terminates the app the instant a patch becomes ready, with no user interaction. The sentence means "wire them up from there", but it can be read literally. Something like "…wiring the handed onRestart / onDismiss to your own UI's actions" removes the ambiguity.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test — both tabled on the thread, with #29 holding the testability seam (and the CodePushConfig.onUpdateReady field) that a real test needs first.

🟢 Positives

  • The build-phase qualifier is the right addition, and its factual core is correct. "It runs during build and may be called many times" is exactly right, and the triggers named (parent rebuilds, keyboard show/hide, rotation) are all reachable — MediaQuery.of(context) at :2554 takes a dependency on the whole MediaQueryData, not just padding, so viewInsets changes do rebuild the overlay. The Medium above is a correction to one clause of it, not to the framing.
  • Builder-is-the-signal survived where three earlier recommendations didn't. checkForUpdate throws, status is a stringly debug channel, and a caller's onUpdateReady is superseded by the overlay's own init — grounding the doc in the one thing the caller is actually handed is the version that cannot drift.
  • The supersedes note is the most useful new fact in the PR, and it is verifiable end to end: the overlay calls CodePush.init in initState (:2469-2482), which cancels the prior timer (:282) and bumps _initEpoch (:284), so the earlier session's pending chain bails. It documents a real footgun for init-in-main() + CodePushOverlay that nothing else mentions.
  • onDismiss is described precisely to the state machine — "next new patch": onDismiss clears _updateReady (:2559), only onUpdateReady sets it (:2481, :2511), and _isPatchAlreadyInstalled short-circuits re-offers of the already-installed patch (:569-577).
  • Five rounds, three copies, still in sync. Doc drift across README / dartdoc / docs site is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; touching all three every round is what keeps each round's finding catchable here instead of in the next issue.

@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 6 fixes pushed (f0fd7d1) — both Mediums are correct and now fixed in docstring + README:

  • onDismiss in the builder "throws" was wrong — you traced it precisely: setStatemarkNeedsBuild is a no-op when the marked element is the current build target, so it silently half-works (banner renders this frame, lingers until the next rebuild), which is the worse failure. Reworded to: calling the handed onDismiss from inside the builder does nothing useful (banner lingers until next rebuild); only showDialog/navigation throw.
  • Deferral doesn't unlock dialogs/navigation from a banner — right: the overlay is a Positioned sibling above MaterialApp, so the builder's context (and anything it returns) has no Navigator/Overlay ancestor, and showDialog/Navigator.of throw at any time, not just during build. Added a clause: dialogs/routes need a navigatorKey on the app's MaterialApp (or a context inside the app), not the builder's context.

CHANGELOG left as-is — its round-5 wording already says "keep side effects out of the builder" without claiming a throw, so it stays accurate. ✅ dart analyze clean (docs-only).

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 6 (f0fd7d1) is still doc + CHANGELOG only — no signature change, no behavior change, the check/download/install/rollback path is untouched. Both round-5 Mediums are verified fixed:

  • onDismiss-in-builder no longer claims a throw (lib/src/code_push.dart:2418-2421). The new wording matches the framework: setState has no build-phase assert of its own, markNeedsBuild's assert passes because the marked element is owner._debugCurrentBuildTarget (_debugIsInScope returns true), and if (dirty) return; short-circuits because ComponentElement.performRebuild clears _dirty in the finally after build() returns. No exception; the banner lingers.
  • The no-Navigator/Overlay caveat landed (:2426-2430, README.md:283-288), correctly separated from the build-phase constraint so the post-frame deferral isn't read as unlocking dialogs.

🟠 Medium

  • lib/src/code_push.dart:2432-2436 / README.md:290-293 — "superseding any earlier init (including its onUpdateReady: callback)" is not what happens with the default config; the earlier callback survives and can still fire.

    The supersede mechanism is the epoch guard, but it only covers the window before _startUpdateFlow, and with the default disableOnPlayStoreInstalls: false (:261) there is no await in front of it. Trace an app that calls CodePush.init(onUpdateReady: mine) in main() and then wraps runApp in CodePushOverlay:

    1. First init: _timer?.cancel() (:282), epoch = ++_initEpoch → 1 (:284), then the unawaited async closure runs synchronously to :319, because disableOnPlayStoreInstalls && short-circuits without ever awaiting. 1 == 1, so _startUpdateFlow(onUpdateReady: mine) is called at :320inside main().
    2. _startUpdateFlow chains _runCrashProtection().then((_) async { … checkAndInstall(onUpdateReady: mine) }) (:350, :396-402). That .then body has no epoch checkepoch isn't even in scope there.
    3. The overlay's initState init cancels the caller's periodic timer (:282 ✅) and bumps the epoch to 2 — but the pending chain from step 2 is unreachable by that guard and still runs checkAndInstall with the caller's callback.

    Net effect, opposite to what the doc now promises: on cold launch there are two live crash-protection chains, and _checkInFlight (:442) lets exactly one win. The caller's chain started first, so it typically wins — their onUpdateReady fires, and the overlay's _updateReady never flips for the initial check (the banner first appears on a periodic tick or an app resume). A developer who reads "superseded" and leaves that callback in as dead code gets their own dialog and the overlay racing, nondeterministically.

    This is worth not landing because it is the same failure mode as CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 — a docstring asserting something the code doesn't do — in the fix for CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25. Suggested wording: "…cancels the earlier init's periodic timer and takes over the check cycle, but that earlier init's first check is already in flight and may still fire its onUpdateReady: once — the two race through a single-flight guard, so whichever wins is the only one that fires. Don't combine CodePush.init(onUpdateReady: …) in main() with CodePushOverlay." Same clause in README.md:290-293.

    The unguarded .then chain at :350 behind this is a genuine code-level defect (the superseded session also re-runs _startLaunchTimer and the quarantine / rollback-report side effects) and deserves its own issue rather than being absorbed into a docs PR — but the doc should not describe the current behavior as clean supersession while it isn't.

🟡 Low

  • lib/src/code_push.dart:2423-2424 / README.md:274-275 / CHANGELOG.md:5 — the onRestart wording is still an instruction to invoke, not to wire up. Raised in round 5, and neither addressed nor tabled on the thread; round 5's rewrite then propagated it to two more copies. "do the work in its initState … calling the handed onRestart / onDismiss from there", and README's "call the handed onRestart/onDismiss from its initState or a post-frame callback", both read literally as call these in initState. onRestart is CodePush.restart, i.e. exit(0) (:872) — a hard process kill the instant a patch becomes ready, with no user interaction. "…wiring the handed onRestart / onDismiss to your own UI's actions" removes the ambiguity in one word.
  • lib/src/code_push.dart:2426 — "sits ABOVE your MaterialApp" is a property of the documented placement, not of the widget. It holds for every example in the repo (README.md:65-76, example/lib/main.dart:11-20), but nothing stops a caller mounting CodePushOverlay inside their app tree, where the builder's context does have a Navigator. "When the overlay wraps your MaterialApp (as in the examples above), the builder's BuildContext has no Navigator/Overlay ancestor" keeps the caveat without asserting a placement the type doesn't enforce.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test — both explicitly tabled on the thread, with #29 holding the testability seam and the CodePushConfig.onUpdateReady field that a real test needs first.

🟢 Positives

  • The onDismiss correction was taken seriously rather than patched over. Round 5's finding was that the doc claimed a throw where the real behavior is a silent half-effect; the fix says exactly that — "does nothing useful … lingers until the next rebuild" — and keeps the throw claim only for showDialog/navigation, where it holds. Documenting the worse failure (no crash, stale banner) is the harder and more useful thing to write.
  • Builder-is-the-signal has now survived three rounds of adversarial reading, where checkForUpdate (throws), status (stringly debug channel), and a caller-supplied onUpdateReady (superseded) each failed. Grounding the contract in the one thing the caller is actually handed is the version that can't drift — and the Medium above is a qualifier on the supersede note, not on that framing.
  • The build-phase claim is precise, not hand-wavy. "May be called many times (parent rebuilds, media-query changes such as keyboard show/hide or rotation)" is exactly right: the builder runs inside _CodePushOverlayState.build (:2563-2568), which takes a whole-MediaQueryData dependency at :2562, and is also rebuilt by the _patchActive setState from the status listener (:2494-2498).
  • checkForUpdate's warning is on the right symbol with the right caveat (:1586-1593): MissingPluginException from the flutter/codepush channel does fall through the bare catch (e) and rethrow as CodePushException, and "those download and install the patch before the callback fires, unlike this check-only call" stops the alternative being read as a drop-in.
  • SizedBox.shrink() genuinely renders nothing here, which is the load-bearing claim of the whole PR: inside Positioned(left: 16, right: 16, bottom: …) the tight width wins over width: 0, but height resolves to 0 — nothing painted, nothing hit-testable.
  • Six rounds, three copies, still in sync. Doc drift across README / dartdoc / docs site is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; touching all three every round is what keeps each round's finding catchable here instead of in the next issue.

Merge gate: one open Medium. Per the policy it can merge if filed as its own issue labelled deferred-medium first — though it is a two-line wording change in the docstring plus the same two lines in README, so fixing is likely cheaper than deferring. The unguarded _startUpdateFlow chain behind it should be filed regardless, as a code-level issue.

…estart is wire-up not invoke; placement-scoped Navigator caveat
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 7 pushed (a56f6b6). All three landed:

dart analyze clean (docs-only). This PR is now purely accurate-docs; the one code-level defect it surfaced is tracked in #30.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 7 (a56f6b6) is still doc + CHANGELOG only — no signature change, no behavior change, the check/download/install/rollback path is untouched. The three round-6 items marked fixed are verified fixed:

  • The supersede claim now describes the actual behavior (lib/src/code_push.dart:2433-2441, README.md:291-298), and the trace holds: with the default disableOnPlayStoreInstalls: false the init closure never awaits before :319, so _startUpdateFlow runs synchronously inside main(), the .then at :350 carries no epoch check, and the overlay's init cancels only the timer (:282) — the pending chain still reaches checkAndInstall(onUpdateReady: mine) at :396-402 and races the overlay's through _checkInFlight (:442).
  • onRestart is now "wiring", not "calling", in all three copies (:2423-2425, README.md:276-278, CHANGELOG.md:5), with the exit(0) consequence named (:872).
  • "When the overlay wraps your MaterialApp (as in the examples)" (:2427) no longer asserts a placement the type doesn't enforce.

🟠 Medium

  • lib/src/code_push.dart:2412-2414 — "the builder call is itself the 'patch ready' signal" is an Android-only contract; on iOS the first patch never invokes the builder at all. _updateReady is set only by the two onUpdateReady closures (:2494, :2524), and onUpdateReady is called from exactly two places: :840 (Android/desktop, after installPatch) and :791 (iOS, but only inside the if (_moduleLoaded) branch at :701 — i.e. a module is already resident and a different patch arrived). The ordinary iOS first-patch path skips both: :814 calls _iosLoadPayload, which hot-loads the module and sets status.value = 'Patch active' (:1943-1948) with no callback — the overlay reacts by flipping _patchActive and re-keying the child subtree (:2500-2502, :2536), not by showing a banner.

    So a developer who reads the new text and moves their update UX into the builder ships something that fires on Android, fires on iOS only for the second patch of a session, and never fires for an iOS device taking its first patch — the most common first experience. This is the same failure mode as CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 (a docstring describing a signal the API doesn't deliver on a platform it supports), which is the reason not to land it in the fix for CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25. One clause covers it, e.g. after "…awaiting restart": "— note this is the Android path: on iOS a freshly downloaded patch is loaded into the running VM and applied without a restart, so the builder is not invoked for it; the banner appears on iOS only when a different patch arrives while one is already loaded." Same clause needed in README.md:272-275.

  • lib/src/code_push.dart:2433-2441 — the init-race warning is accurate but lands in the one docstring its audience won't read, and config's own doc four lines above recommends the pattern it warns about. config's dartdoc (:2392-2400) tells callers this "lets apps configure the SDK once in main() and then just write CodePushOverlay(child: ...)" — i.e. CodePush.init(...) in main() plus the overlay. The new note then says don't combine CodePush.init(onUpdateReady: …) in main() with CodePushOverlay. Both docstrings are on the same class, ~30 lines apart, and a reader who arrives at config (the field that documents init-in-main()) gets the recommendation without the caveat; a reader who arrives at bannerBuilder is by definition already inside the overlay and is not the person about to write init in main(). This is the same argument that moved the throws-warning onto checkForUpdate itself in round 2, and it applies here for the same reason: the caveat belongs on CodePushOverlay's class doc (:2366-2380) or on config, where the affected reader is. README.md already gets this right — the note sits under the overlay section, not inside the bannerBuilder comment.

🟡 Low

  • lib/src/code_push.dart:1590-1593 — the failure-soft alternative for checkForUpdate skips the API built for exactly this. The warning is about "engines without code push"; the repo already has CodePush.hasCodePushEngine (:876-884), documented as "Apps can call this to hide 'check for updates' UI on devices whose baseline wasn't built for code push", bounded by a 2s timeout and returning false rather than throwing. Pointing at init/checkAndInstall answers "when is a patch ready" but not "does this engine support code push", which is what the sentence just warned about — and those install a patch, while hasCodePushEngine does not.
  • lib/src/code_push.dart:2443-2444 — the trailing onDismiss sentence is now orphaned and reads against :2418-2421. "Calling the provided onDismiss hides the banner slot until the next new patch becomes ready" is correct, but it is separated from the onDismiss discussion by the Navigator paragraph and the init paragraph, and taken alone it contradicts "calling the handed onDismiss there does nothing useful". Folding it into the earlier paragraph, or qualifying it as "calling it from a user action (not from inside the builder)", removes the tension.
  • example/lib/main.dart:11-21 + :62-80 — the fourth copy of this doc did not get updated, and it demonstrates a lifecycle mix while misreporting the single-flight loss. The example wraps CodePushOverlay and also calls CodePush.checkAndInstall(onUpdateReady: …) from a button; when the overlay's own check is in flight, :442 returns false and the example prints "No update available." The comment at :439-441 says exactly why that is wrong: "a losing caller's false means 'another check is already running', not 'no update'." Since this PR's whole thesis is that copies drift, and the example is what people copy-paste, it is worth a bannerBuilder line here plus distinguishing "no update" from "check already running".
  • The docs-site copy is now several rounds behind. The PR body records that code-push-website #25 was corrected to describe the round-1 option-1 behavior. Rounds 2-7 added the build-phase constraint, the no-Navigator/Overlay caveat, the onRestart gating note and the init-race note — none of which that PR could have contained. Worth a tracking issue so the copy that produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 doesn't re-drift the moment this merges.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test (both tabled on the thread, with #29 holding the testability seam and the CodePushConfig.onUpdateReady field), and the unguarded .then chain at :350 (filed as #30 and correctly not worked here).

🟢 Positives

  • The supersede paragraph is now the rare doc that describes a race honestly. "Cancels the earlier init's periodic timer and takes over the check cycle, but that earlier init's first check may already be in flight and can still fire its onUpdateReady: once" is verifiable end to end (:282:284:319:350:396-402:442), and it took tracing a synchronous-until-first-await closure to get there. Documenting "these race, don't combine them" beats documenting a clean supersession that doesn't exist.
  • Filing CodePushOverlay does not actually supersede an earlier init(onUpdateReady:) — unguarded .then chain races #30 rather than fixing it in a docs PR is the right split. The code-level fix (thread epoch into the .then continuation) touches the crash-protection chain — the one path where a mistake bricks a host app — and belongs in a PR that can carry a test, not in a docstring change.
  • The onDismiss-in-builder wording survives a framework read. setStatemarkNeedsBuild is a no-op when the marked element is owner._debugCurrentBuildTarget and _dirty is still set (cleared in ComponentElement.performRebuild's finally, after build() returns), so "does nothing useful — the banner lingers until the next rebuild" is right where "throws" was wrong, and the throw claim is correctly retained only for showDialog/navigation.
  • The build-phase claim is precise. The builder runs inside _CodePushOverlayState.build (:2568-2573), which takes a whole-MediaQueryData dependency at :2567 and is also rebuilt by the _patchActive setState from the status listener (:2499-2503) — so "parent rebuilds, keyboard show/hide, rotation" are all genuinely reachable.
  • SizedBox.shrink() really does render nothing here, which is the load-bearing claim of the PR: inside Positioned(left: 16, right: 16, …) the tight width wins, height resolves to 0, nothing painted or hit-testable.
  • Seven rounds and the three in-repo copies are still in sync, including the onRestart "wire, don't invoke" fix propagating to all of them in the same push. Drift across copies is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; the discipline is what keeps each round's finding catchable here.

Merge gate: two open Mediums. Per the policy this can merge with each filed as its own issue labelled deferred-medium first — but both are one-clause doc edits (an iOS caveat in the docstring + README, and moving an existing paragraph up to the class/config doc), so fixing is almost certainly cheaper than filing.

… doc + config; checkForUpdate points to hasCodePushEngine; honest example (file website #38)
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 8 pushed (44b99e6). Both Mediums + all four Lows addressed:

Mediums

  • iOS builder-signal gap — right, and important: the "builder is the patch-ready signal" line was Android/desktop-only. Added the iOS caveat to the docstring and README: on iOS a fresh patch is applied live (no banner); the builder fires on iOS only when a different patch arrives while one is already loaded. Pointed first-patch-signal-on-iOS users to CodePush.status/isPatched.
  • init-race note on the wrong docstring — moved the full caveat to the CodePushOverlay class doc (where the init-in-main() reader lands) and added a one-line cross-reference on config (which recommended the pattern). bannerBuilder now just points at the class doc. README already had it under the overlay section.

Lows

  • checkForUpdate now points to hasCodePushEngine (2s timeout, returns false) for the "does this engine support code push" case, distinct from the patch-ready callbacks.
  • Orphaned onDismiss sentence folded into the side-effects paragraph and qualified — "from a user action … DOES hide the banner", no longer contradicting the in-builder no-op line.
  • example/lib/main.dart — added the iOS caveat + overlay-races-manual-check note to the header comment, and fixed the !installed branch to stop reporting a single-flight loss as "No update available" (it now says "no update, or a check is already in progress").
  • Docs-site drift filed as code-push-website #38 so the copy that produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 gets synced to the merged dartdoc.

dart analyze clean (docs + example only). Nothing behavioral changed; the one code defect surfaced across these rounds is tracked in #30.

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 8 (44b99e6) is doc + example only — no signature change, no behavior change, the check/download/install/rollback path is untouched. The round-7 items marked fixed are verified fixed:

  • The iOS caveat landed on the builder contract (lib/src/code_push.dart:2429-2436, README.md:284-288) and the trace holds: the ordinary iOS first-patch path is :814_iosLoadPayloadstatus.value = 'Patch active' (:1950) with no callback, while onUpdateReady is only reached at :840 (Android/desktop) and :791 (iOS, inside the _moduleLoaded branch at :701).
  • The init-race caveat moved to the class doc (:2384-2393) with a cross-reference on config (:2409-2411) and a pointer from bannerBuilder (:2458-2460) — the right placement, since config is the field that recommends init-in-main().
  • checkForUpdate now points at hasCodePushEngine (:1587-1595), and the parenthetical is accurate: both probes in _probeEngineFingerprint are .timeout(2s) inside catch (_) and it returns nullfalse rather than throwing (:891-909).
  • The orphaned onDismiss sentence is folded and qualified (:2444-2446), and example/lib/main.dart:77-83 no longer reports a single-flight loss as "No update available."

🟠 Medium

  • lib/src/code_push.dart:2434-2436 / README.md:287-288 — the iOS first-patch signal points at CodePush.isPatched, which is always false on iOS. iOS never installs through the engine: installPatch is the non-iOS branch (:824), and the iOS path writes bytes with _installPatchFromDart and hot-loads via ui.codePushLoadModule (:1922-1924). isPatched invokes the engine channel (:1656-1658), whose updater is disabled on iOS — the repo's own comment says so in as many words: "on iOS where the engine channel is disabled and isPatched would always answer false" (:497). Secondarily, isPatched is a Future<bool> getter, so "listen to" it isn't possible even where it does work.

    So the clause added to fix an Android-only contract hands iOS readers a signal that reads false for the exact case it was recommended for — the same failure mode as CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25. The app-facing listenable is CodePush.moduleResult (:161-169, "Apps can listen to this"; set at :1949, cleared at :1757), which is what the overlay itself listens to alongside status (:2499-2500). One caveat if you name it: a payload with no entry point loads successfully with a null result (:1916-1921), so moduleResult stays null there — CodePush.status.value == 'Patch active' (:1950) is the strict equivalent, and is what _onModuleLoaded keys on (:2516). Minimal fix: drop / [CodePush.isPatched] from :2435 and from README.md:288.

  • lib/src/code_push.dart:2390-2391 (and :2409-2411) — "It's fine to call CodePush.init(...) in main() without an onUpdateReady:" is not fine; it typically costs the banner for the first patch of the session. Dropping the callback removes the stray-callback half of the race but not the duplicate check cycle. Android, default disableOnPlayStoreInstalls: false:

    1. main()'s init: epoch 1, and because disableOnPlayStoreInstalls && short-circuits without awaiting, the closure runs synchronously to :319-320 — chain Name collision with code-push Flutter SDK — duplicate CodePush, UpdateInfo, PatchInfo, CodePushException #1 starts with onUpdateReady: null.
    2. Overlay initState init (:2502-2512): cancels the timer (:282), epoch 2 — but the .then at :350 carries no epoch check, so chain Name collision with code-push Flutter SDK — duplicate CodePush, UpdateInfo, PatchInfo, CodePushException #1 is still live (this is CodePushOverlay does not actually supersede an earlier init(onUpdateReady:) — unguarded .then chain races #30).
    3. _runCrashProtection returns immediately on Android (:2113), as does _iosReloadInstalledPatch (:2002), so both continuations are microtask/channel hops in the order they were queued: chain Name collision with code-push Flutter SDK — duplicate CodePush, UpdateInfo, PatchInfo, CodePushException #1 reaches checkAndInstall first, takes _checkInFlight (:442-443), installs, sets 'Restart to apply' and calls onUpdateReady?.call() — which is null (:839-840). Chain dart:io HttpClient hangs reading HTTP response body in iOS release mode #2 returns false at :442.
    4. The patch is now on disk, so the overlay's periodic tick and its resume check (:2534-2542) short-circuit at _isPatchAlreadyInstalled'Patch already installed'false, no callback (:569-577).

    Net: the patch installs silently, _updateReady never flips, and the user is never prompted to restart for the whole session — the overlay's entire job — even though the developer followed the doc's blessed pattern. The genuinely safe shape is the one example/lib/main.dart:15-21 already uses: pass config: to the overlay and don't call init in main() at all. Suggested wording: "Calling CodePush.init(...) in main() only to populate [CodePush.lastConfig] still starts a second check cycle that races the overlay's: it usually wins the single-flight guard and installs the first patch with no callback, so no banner appears that session. Prefer passing config: to the overlay." Same qualifier on :2409-2411.

🟡 Low

  • lib/src/code_push.dart:2432-2434 / README.md:286-287 — "only when a different patch arrives while one is already loaded" isn't the only iOS banner case. The resident patch re-offered after a kill-switch/rollback content revert also fires it: persistSilently + _contentRevertedThisSessiondebugPersistOutcomeShowsRestart returns true (:1205-1211) → 'Restart to apply' + onUpdateReady at :790-791. A trailing "(or the resident patch is re-offered after a rollback reverted the app-facing content)" keeps the "only when" honest.
  • CHANGELOG.md:3-6 — the iOS caveat is the one copy it didn't reach. The three sub-bullets carry null-return, build-phase and the init race; the Android/desktop-only nature of the builder signal is the most consequential new fact in the PR and is missing. pub.dev's changelog is where existing users look for behavior notes, and copy drift is what this PR exists to fix.
  • example/lib/main.dart:9-11 vs :58 / :118 — the new header comment explains that iOS applies the first patch live, and the status card directly below renders Patched: $_isPatched from CodePush.isPatched, which on iOS reads false while the patch is active (same :497 reason); _currentPatch (:1641, engine channel) is null there too. Same root cause as the first Medium, but here the fourth copy shows the misleading value rather than describing it. CodePushPatchBuilder / moduleResult — already in this file at :152-170 — is the honest iOS signal to point at.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test (both tabled on the thread, gated on #29's testability seam), and the unguarded .then at :350 (filed as #30 and correctly not worked here).

🟢 Positives

  • The iOS caveat is the right fact, and the platform split is drawn where the code draws it. "A freshly downloaded patch is loaded into the running VM and applied without a restart, so the builder is NOT invoked for it" is exactly :795-821:1945-1950, and "the banner appears only when a different patch arrives while one is already loaded (which cannot hot-swap and so waits for a restart)" matches the _moduleLoaded branch's own comment at :701-708. The Medium above is about the alternative signal, not this framing.
  • Moving the init-race note to the class doc was the correct call over deferring it. The reader who writes init in main() arrives at CodePushOverlay or at config, not at bannerBuilder; both now carry it, and bannerBuilder keeps a one-line pointer instead of a duplicate paragraph that could drift.
  • The checkForUpdatehasCodePushEngine swap answers the question the warning actually raises. "Does this engine support code push" is a different question from "when is a patch ready", and the distinction ("those download and install the patch before the callback fires, unlike this check-only call") is preserved.
  • The example's single-flight fix is the small change with the largest blast radius. example/lib/main.dart:77-83 is the code people paste; reporting a lost _checkInFlight race as "No update available" is precisely the bug :439-441 warns about, and it is now spelled out at the call site.
  • Eight rounds and the in-repo copies are still in sync — docstring, README and example all moved together this round. Drift across copies is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25.

Merge gate: two open Mediums. Per the policy this can merge with each filed as its own issue labelled deferred-medium first — but the first is deleting three words (/ [CodePush.isPatched]) in two files, and the second is rewording one sentence in two places, so fixing is cheaper than filing.

…d is always-false on iOS); init-in-main races even without onUpdateReady
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 9 pushed (1867276) — both Mediums were real bugs in my round-8 additions; fixed:

  • iOS first-patch signal pointed at isPatched, which is always false on iOS (engine channel disabled, per the :497 comment — and it's a Future, not listenable). The docstring/README/CHANGELOG now point at CodePush.moduleResult (the app-facing listenable) / CodePush.status.value == 'Patch active', and explicitly say isPatched reads false on iOS.
  • "It's fine to call init in main() without onUpdateReady:" was wrong — dropping the callback removes the stray-callback half but not the duplicate check cycle, which usually wins the single-flight guard and installs the first patch with no banner all session. Reworded (class doc + config field + README + CHANGELOG) to: pass config: to the overlay and don't call CodePush.init in main(); lastConfig fallback exists only for apps already calling init for other reasons.

Lows:

  • "only when a different patch arrives" now also names the resident-patch re-offer after a rollback content revert.
  • CHANGELOG carries the iOS builder-signal caveat.
  • example/lib/main.dart status card labels Patched as Android/desktop and notes isPatched/currentPatch read false/null on iOS, pointing at the CodePushPatchBuilder/moduleResult already in the file.

dart analyze clean. The underlying .then-epoch code defect remains tracked in #30.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 9 (1867276) is doc + example-comment only — no signature change, no behavior change, and the check/download/install/rollback path is untouched. The round-8 items marked fixed are verified fixed:

  • isPatched is gone as the iOS signal (lib/src/code_push.dart:2436-2440, README.md:288-291, CHANGELOG.md:6), and the replacement text's claim about it is correct: isPatched invokes the engine channel (:1656-1662), whose updater is disabled on iOS — the repo's own comment at :497 says so.
  • The "init in main() without onUpdateReady: is fine" claim is gone (:2384-2393, :2405-2412, README.md:302-308, CHANGELOG.md:7), and the trace behind the new wording holds end to end: _timer?.cancel() (:282) → ++_initEpoch (:284) → disableOnPlayStoreInstalls && short-circuits with no await (:292) → _startUpdateFlow runs synchronously inside main() (:319-320) → the .then at :350 carries no epoch check → checkAndInstall at :396-402 races the overlay's through _checkInFlight (:847).
  • The resident-patch re-offer case landed (:2434-2436) and matches :775-791 + _contentRevertedThisSession at :1755.
  • The example's status card now labels Patched as Android/desktop (example/lib/main.dart:118-123).

🟠 Medium

  • lib/src/code_push.dart:2436-2440 / README.md:288-291 / CHANGELOG.md:6 — the new iOS first-patch signal, CodePush.moduleResult, silently misses a real load case, and the fallback it offers is a self-declared debug channel.

    moduleResult is a ValueNotifier<Object?> initialised to null (:169). _iosLoadPayload sets moduleResult.value = result at :1949 — and result is legitimately null for a payload that loaded cleanly but had no entry point to invoke, which the code at :1914-1921 documents as "a valid no-op, not a failure". ValueNotifier's setter returns early when the value is unchanged, so null → null notifies nobody: a caller who follows this doc and only listens to moduleResult gets no first-patch signal at all for that patch, on iOS — precisely the case the clause was added for. Two other paths set 'Patch active' without touching moduleResult at all (:736, :783), so they are missed too.

    The parenthetical alternative — CodePush.status.value == 'Patch active' — is the reliable one, and is what the overlay itself uses (it listens to both notifiers and keys on the string: :2503-2504:2520). But status's own dartdoc still declares it "Debug status notifier — shows what code push is doing" (:161-162), and round 2 of this review rejected status as a UI signal on exactly that ground. Landing it in three copies as a documented app-facing check promotes the literal 'Patch active' to public API without any doc saying so — the next status-text edit then silently breaks callers.

    Cheap fix, both halves: (a) invert the recommendation so the status listener is primary and moduleResult is the content hook it is documented as (:164-168), and (b) update :161-162 to state that 'Patch active' is a stable, app-facing value. Right now the reliable half is a parenthetical and the primary half has a hole.

  • example/lib/main.dart:149 — the Rollback button is gated on _isPatched, so it is permanently disabled on iOS — the platform this round of the PR is about. Round 9 added a comment 26 lines above (:118-123) explaining that isPatched reads false on iOS even while a patch is active, then left onPressed: _isPatched ? _rollback : null consuming that same value. Rollback genuinely works on iOS: _rollbackInternal falls through the engine attempt and takes the Dart-side file-removal path (lib/src/code_push.dart:1682-1698), and rollback()'s own doc says so (:1664-1666). So an iOS developer evaluating the plugin from the example sees Patched (Android/desktop): false and a greyed-out Rollback control, and concludes rollback is unsupported on iOS.

    _rollback already catches CodePushException and surfaces "No active patch to roll back" (example/lib/main.dart:96-98), so simply un-gating it is failure-soft; gating on moduleResult != null || _isPatched is the more faithful version. One line — and the example is the copy people paste.

🟡 Low

  • lib/src/code_push.dart:2462-2464 — the cross-reference still states the round-7 narrow rule that round 9 superseded. It reads "don't combine CodePush.init(onUpdateReady: …) in main() with the overlay", while the class doc it points at now says the problem occurs even without onUpdateReady: (:2385-2390). A reader who stops at bannerBuilder infers that init without a callback is fine — the exact belief round 8 disproved. Dropping (onUpdateReady: …) from that clause fixes it.
  • README.md:82-84 — the Quick Start still makes the banner promise unconditionally. "When a patch is downloaded and installed, a banner appears prompting the user to restart." That is now known to be the Android/desktop path; the iOS caveat sits ~200 lines below at :284-291, in the CodePushOverlay widget reference. This is the same "put the caveat where the reader is" argument that moved the throws-warning onto checkForUpdate in round 2 and the init-race note onto the class doc in round 8 — and Quick Start is the section that actually makes the claim. One clause ("on Android/desktop; on iOS the first patch applies live without a restart — see below") closes it.
  • CHANGELOG.md:3-7 doesn't mention the checkForUpdate doc change. All four sub-bullets are scoped to CodePushOverlay.bannerBuilder (and the fourth is really about init + the overlay, not the builder). The most user-actionable new fact in the PR for anyone on the low-level API — that checkForUpdate throws CodePushException on engines without code push, and that hasCodePushEngine is the non-throwing probe (lib/src/code_push.dart:1586-1595) — never reaches pub.dev's changelog.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test (both tabled on the thread; test/ still has no widget test at all, and #29 holds the testability seam plus the CodePushConfig.onUpdateReady field a real one needs), and the unguarded .then at :350 (filed as #30 and correctly not worked here).

🟢 Positives

  • The isPatched correction is the right one and was taken all the way. The clause doesn't just drop the bad symbol — it names why ("the engine channel is disabled there") and that it is a Future, not a listenable, so a reader can't re-derive the same mistake. That is the difference between patching a sentence and fixing the misunderstanding behind it.
  • The init-race rewrite is honest about a race rather than tidy about a supersession. "Starts a second check cycle that races the overlay's: it usually wins the single-flight guard and installs the first patch, so _updateReady never flips and no banner appears that session" is verifiable at every hop (:282:284:292:319-320:350:396-402:847), and it took reading a closure that is synchronous until its first await to get there. Most docs would have written "the overlay's init wins" and been wrong.
  • config's field doc no longer sells the pattern the class doc warns about. :2405-2412 now leads with "prefer passing config: here directly" and demotes lastConfig to "exists for apps that already call init for other reasons" — and example/lib/main.dart:14-25 already demonstrates exactly that shape, so the blessed pattern has a working reference implementation in-repo.
  • The builder-is-the-signal framing has now survived four rounds of adversarial reading, where checkForUpdate (throws), status (stringly), a caller-supplied onUpdateReady (superseded), and an unqualified build-phase contract each failed. Grounding the doc in the one thing the caller is actually handed is the version that can't drift.
  • The platform split is drawn where the code draws it. "On iOS a freshly downloaded patch is loaded into the running VM and applied without a restart, so the builder is NOT invoked for it" is exactly :795-821:1945-1950; "only when a different patch arrives while one is already loaded" is the _moduleLoaded branch at :701-708; and the rollback re-offer case is debugPersistOutcomeShowsRestart:790-791. Three distinct code paths, each named correctly.
  • The example's single-flight fix remains the small change with the largest blast radiusexample/lib/main.dart:77-83 no longer reports a lost _checkInFlight race as "No update available", which is precisely the misreading the guard's own comment warns about.
  • Nine rounds and four copies still in sync. Doc drift across dartdoc / README / example / docs site is what produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; touching them together every round is why each round's finding is catchable here instead of in the next issue — and the docs-site copy is tracked in code-push-website #38 rather than left to re-drift.

Merge gate: two open Mediums, so this can merge under the policy with each filed as its own issue labelled deferred-medium first — but the first is inverting one recommendation plus a one-line dartdoc edit at :161, and the second is one line in the example, so fixing is almost certainly cheaper than filing. The three Lows are one-clause edits.

…duleResult has a null-notify hole); un-gate example Rollback (works on iOS); README quickstart iOS caveat; CHANGELOG checkForUpdate note
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 10 pushed (ed1aa9d) — both Mediums + all three Lows fixed:

  • iOS signal invertedCodePush.status (check status.value == 'Patch active') is now the PRIMARY recommendation; moduleResult is described as the content hook it is (stays null for a no-op payload, so incomplete on its own). And status's dartdoc (:161-162) now states 'Patch active' is a stable, app-facing value — no longer just a debug channel. Applied to docstring + README + CHANGELOG.
  • Example Rollback un-gated — was _isPatched ? _rollback : null (permanently disabled on iOS, where rollback actually works via Dart-side file removal). Now always enabled; _rollback is failure-soft ("No active patch to roll back"). Comment explains why not to gate on _isPatched.
  • Lows: bannerBuilder cross-ref drops (onUpdateReady: …) (the race is even without it); README Quick Start now carries the iOS-applies-live caveat where the claim is made; CHANGELOG notes the checkForUpdatehasCodePushEngine doc change.

dart analyze clean. .then-epoch code defect still tracked in #30.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 10 (ed1aa9d) is doc/comment only plus one onPressed: line — the check/download/install/rollback path is untouched. Round-9 items verified fixed: Rollback un-gated (example/lib/main.dart:150-153) and genuinely failure-soft (_rollbackInternal falls through the swallowed engine attempt at lib/src/code_push.dart:1687-1692 to Dart-side removal, throws CodePushException at :1698, caught at example/lib/main.dart:96-98); the bannerBuilder cross-ref dropped its onUpdateReady: qualifier (:2471-2473); README Quick Start carries the iOS caveat (README.md:82-86); the moduleResult no-op hole is named (:2444-2447, matching :1914-1921).

🟠 Medium

1. lib/src/code_push.dart:161-167 and :2443-2444 — the new primary iOS signal keys on a transient value, not a state. It reads false for nearly the whole session while a patch is active.

The doc says "Patch active" is "a STABLE, app-facing signal you may key on ... listen to this notifier and compare". But status is one mutable string every later step overwrites, and nothing restores it. Ordinary iOS cold start with a resident patch:

  1. _startUpdateFlow awaits _iosReloadInstalledPatch (:369) → _iosLoadPayloadstatus.value = "Patch active" (:1956).
  2. The next statement in that same chain is checkAndInstall (:400-406) → "Checking server..." (:452).
  3. The pre-download identity guard matches — written on iOS precisely so the running patch is not re-downloaded (:572-576) — so "Patch already installed" (:581), return false.

From there the comparison is false for the rest of the session. All three writers of "Patch active" (:742, :789, :1956) are one-shot install/load paths; the steady state is "Patch already installed" (or "No patch available", :518). didChangeAppLifecycleState (:2544-2556) re-runs checkAndInstall on every resume, so even an app that caught the startup window loses it after one backgrounding.

Failure scenario: a developer follows the doc verbatim with a ValueListenableBuilder<String> on CodePush.status rendering patched UI when the value equals "Patch active". On iOS that UI never appears — the normal case, since app widgets build while _runCrashProtection is still awaiting, so the listener attaches after step 3 — or it flickers on and then reverts at the first resume.

The fix is in this file already: the overlay does not key on the value, it latches on the transitionif (mounted && !_patchActive && ...) at :2529, set once, never reset. So "The overlay keys on it too" (:167) is the misleading part: the overlay reads an edge, the doc instructs a level. Reword to describe a stable app-facing transition (latch a bool on first observation, as the overlay does) and state that the notifier moves on at the next check cycle and never returns.

Same shape as #25 — a docstring instructing something the code cannot do — reintroduced by the fix for it, so worth closing rather than deferring.

2. README.md:291-292, README.md:232-233, CHANGELOG.md:6 — the round-9 moduleResultstatus inversion landed in the dartdoc only.

Round 10 says it was "Applied to docstring + README + CHANGELOG", but README.md:291-292 still leads with "listen to CodePush.moduleResult (or check ...)" with status parenthetical and no no-op-payload caveat — verbatim the arrangement round 9 flagged; CHANGELOG.md:6 has the same order and same omission, and that is the pub.dev copy; and README.md:230-236, the CodePush.status reference section, still says "Useful for debug UIs or logging" — the README copy of the exact :161-162 line this PR rewrote. A reader who follows the dartdoc pointer to status and opens the README is told it is a debug channel. Drift across the four copies is the failure mode that produced #25; whatever wording Medium 1 settles on has to reach all four anyway, so these fold into one pass.

🟡 Low

  • "Patch active" is now public API but remains a bare literal in five places, with no constant and no test. Written at :742, :789, :1956, compared at :2529, documented at :167 / README.md:244; grep -rn "Patch active" test/ returns nothing across twelve files. A static const String statusPatchActive referenced from all five sites makes the new contract enforceable — and needs no CodePushOverlay has no widget-test coverage — bannerBuilder contract is unpinned #29 testability seam, unlike the tabled widget test.
  • README.md:203-208 — the CodePush.isPatched reference section is the one place without the iOS caveat. The class doc (:2447-2449), overlay README section (:292-293), CHANGELOG (:6) and example (example/lib/main.dart:118-123) all warn it reads false on iOS; the entry where someone looking up the symbol lands still makes the unqualified promise.
  • CHANGELOG.md:3-8 omits the status doc change. Promoting "Patch active" from debug notifier to stable app-facing value (:161-167) widens the public contract — the thing future status-text edits must respect — and it never reaches pub.dev.
  • lib/src/code_push.dart:2394 explains the race via _updateReady, a private field readers of the rendered doc cannot see. The following clause already states it observably.

🟢 Positives

  • Un-gating Rollback beat the narrower conditional suggested in round 9. example/lib/main.dart:147-150 explains why not to gate, so the gate does not come back — and the only throw on the path is CodePushException, already surfaced as a status string.
  • Every round-9 Low was closed at its root, not its symptom. The cross-ref dropped the clause that made the narrow claim rather than being reworded; the Quick Start caveat went where the claim is made; the CHANGELOG got the checkForUpdatehasCodePushEngine line. No new claim needing verification.
  • The example header now names the race it demonstrates (:12-13_manualCheck at :66, where :78-82 explains false also means "a check is already in progress"). Warning and triggering code are one click apart in the copy people paste.
  • The platform split is still drawn where the code draws it — the live-load claim is :711-721:1949-1956, "a different patch while one is loaded" is the _moduleLoaded branch at :711-718, the rollback re-offer is debugPersistOutcomeShowsRestart:789-796. Three paths, each named correctly, ten rounds in.
  • The moduleResult demotion says why it is incomplete, not just that it is (:2444-2447 naming :1914-1921, the subtlest fact in this review).

Merge gate: two open Mediums — mergeable under the policy with each filed as its own deferred-medium issue first, but both are cheap and share one edit pass: settle the latch-vs-level wording, then propagate to the three drifted copies. The Lows are one-clause edits apiece except the constant.

Not re-raising: the Unreleased heading and the missing CodePushOverlay widget test (tabled, gated on #29), and the unguarded .then at :350 (filed as #30, correctly not worked here).

No Dart SDK in this environment, so dart analyze / dart test were not re-run; the Dart-side diff is comments plus the one onPressed: change.

… (not a level compare); fix status/isPatched README reference sections; drop private-field ref
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 11 pushed (b47fce9) — both Mediums fixed by correcting the iOS-signal framing everywhere:

  • status is an EDGE, not a level — you're right, 'Patch active' is transient (overwritten by 'Patch already installed' at the next check and on every resume), so status.value == 'Patch active' reads false most of the session. All copies now say: latch a bool the first time status becomes 'Patch active' (as the overlay does internally — it latches on the transition, never resets), not a level compare. moduleResult is described as the content hook with the no-op-null caveat.
  • Drift fixed — the reframing now reaches all four copies: the status field dartdoc, the bannerBuilder text, the README CodePushOverlay section, AND the README ### CodePush.status reference (was still "debug UIs or logging") and ### CodePush.isPatched reference (now carries the iOS-reads-false caveat). CHANGELOG updated + a bullet for the status contract change.

Lows: dropped the private _updateReady from the rendered doc (the observable clause remains).

Tabling one Low — introducing a static const statusPatchActive referenced from the three writers + the overlay is a code change beyond this docs PR's scope; filing it as a follow-up so the new literal contract gets a constant + a test. This PR stays docs-only.

dart analyze clean.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 11 (b47fce9) is doc + comment only — no signature change, no behavior change, and the check/download/install/rollback path is untouched. The round-10 items are verified fixed: status is described as a transition, not a level (lib/src/code_push.dart:161-173), and the trace behind that wording holds — all three writers of 'Patch active' (:748, :795, :1962) are one-shot paths, and the steady state after them is 'Patch already installed' (:587) or 'No patch available' (:524), re-entered on every resume via :2551-2563. The round-10 drift is closed too: README.md:234-255 (was "debug UIs or logging") and README.md:203-208 (the isPatched reference) both carry the new framing.

🟠 Medium

lib/src/code_push.dart:167-173 and :2448-2452 / README.md:249-255, :300-307 / CHANGELOG.md:6,8 — the latch the docs now prescribe cannot survive in an app widget, because the overlay destroys the app's entire widget subtree on the exact transition it tells you to latch.

The instruction is "LATCH a bool the first time the value becomes 'Patch active' and never reset it, exactly as [CodePushOverlay] does internally", plus "Do NOT render UI on status.value == 'Patch active' directly". Both halves fail for the natural implementation (a StatefulWidget somewhere under CodePushOverlay(child: MyApp())):

  1. _onModuleLoaded latches on the edge (:2536-2538) — correct, and it works for the overlay because its listener is registered in the overlay's own initState (:2519-2520) on state that lives above the child.
  2. That same setState flips _patchActive, and build re-keys the whole app: KeyedSubtree(key: ValueKey<bool>(_patchActive), child: widget.child) (:2572). A changed key means Widget.canUpdate is false, so the old child element is unmounted and the subtree is inflated from scratch — every descendant State is disposed and recreated, with the app's latch bool back at its initializer.
  3. The re-inflated State attaches a fresh listener, but the edge has already passed and 'Patch active' never comes back that session (the doc says so itself at :172-173).
  4. Nor does a fallback level check help at that moment: on the ordinary iOS cold start with a resident patch, _iosLoadPayload sets 'Patch active' (:1962), returns to the .then chain, and checkAndInstall (:408) runs synchronously through to status.value = 'Checking server...' (:458) — all in microtasks, i.e. before the frame that rebuilds the subtree. By the time the new State.initState runs, the value is already something else.

Failure scenario: an iOS app follows the docstring verbatim — bool _patched = false; in a widget under the overlay, CodePush.status.addListener(...) in initState setting it when the value is 'Patch active' — and renders its patched UI on _patched. The listener fires, the flag is set, and one frame later the whole subtree is thrown away and rebuilt with _patched == false. The patched UI never appears, on the exact platform and exact case (first patch on iOS) the clause was added for. This is the #25 shape again — a docstring instructing something the code defeats — introduced by the fix for it, so it is worth closing rather than deferring.

Note the asymmetry the mechanism creates, because it decides the fix: a level survives re-inflation, an edge does not. CodePush.moduleResult is a level, which is why CodePushPatchBuilder (:2690-2711, a ValueListenableBuilder over moduleResult) keeps working across the re-key and the example's patched card still renders. Options, cheapest first: (a) keep the latch but say where it must live — outside the overlay's child subtree (a top-level ValueNotifier<bool>/static set from a listener registered in main() before runApp, which also fixes the attach-ordering problem: a listener registered inside the subtree can only attach after the overlay's initState already started init); (b) go back to moduleResult as primary with the no-op-null caveat spelled out, since it is the only signal that is both app-facing and re-read after re-inflation; or (c) expose the latch the overlay already computes as a ValueListenable<bool> on CodePush — the honest API, but a code change beyond this PR.

Whichever wording wins has to reach all six copies (see the second Low) — and the two that weren't updated this round happen to be the two that currently describe something that works.

🟡 Low

  • README.md:100 — the debug bar never shows "CP: Patch active". The comment advertises it as a sample value, but the bar is gated on !_patchActive (:2573) and _patchActive flips in the same notification cycle that sets the string (:2536-2538), so the bar is removed in the same frame the value would first render — and on Android/desktop 'Patch active' is never written at all (all three writers sit inside iOS branches: :711, :1962). "CP: Restart to apply" is the accurate example.
  • Two of the six copies of the iOS-signal advice didn't move this round. example/lib/main.dart:118-122 still says "On iOS use CodePush.moduleResult / CodePush.status (see the CodePushPatchBuilder below) as the active-patch signal" — moduleResult first, no latch, no no-op caveat — and README.md:449-451 ("Listen to CodePush.moduleResult or use CodePushPatchBuilder to react to live patches") is the same. Copy divergence is the failure mode that produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25; four copies now say latch-on-status and two say listen-to-moduleResult. Resolving the Medium settles which, but all six need the same answer.
  • example/lib/main.dart demonstrates the Medium without saying so. _CodePushDemoState lives under the overlay, so on an iOS launch with a resident patch its _status / _isPatched fields are discarded and re-initialised when _patchActive flips — the demo visibly resets moments after startup. One line in the header comment ("on iOS the overlay re-keys the app subtree when a patch loads, so app State is recreated — keep any patch latch above the overlay") turns a confusing artifact into the lesson.

🟢 Positives

  • The edge-vs-level correction is the right diagnosis and it was taken all the way. :161-165 doesn't just soften the claim, it names the mechanism ("each step OVERWRITES it", with the concrete 'Patch active''Patch already installed' example) and :171-173 explicitly forbids the level compare that round 10 showed reads false for most of the session. The remaining gap is where the latch lives, not that it should be one.
  • The drift fix reached the two README reference sections that had been missed for two rounds. ### CodePush.status (:234-255) and ### CodePush.isPatched (:203-208) are where a reader who follows a dartdoc pointer actually lands, and both now agree with the dartdoc instead of contradicting it.
  • Every claim in the new checkForUpdate block checks out. "Throws CodePushException on any failure" matches :1624-1628; "hasCodePushEngine (2s timeout, returns false rather than throwing)" matches :894-895_probeEngineFingerprint, where both probes are .timeout(const Duration(seconds: 2)) inside catch (_) and the fallback returns null (:903-921); and the "those download and install the patch before the callback fires" distinction is real (:836-852).
  • The platform split is still drawn exactly where the code draws it, twelve rounds in. The live-load claim is :826-833:1957-1962; "a different patch while one is already loaded" is the _moduleLoaded branch at :713-720; the rollback re-offer case is debugPersistOutcomeShowsRestart:787-803. Three distinct paths, each named correctly.
  • The bannerBuilder side-effect contract is precise about a subtle thing. "Calling the handed onDismiss there does nothing useful — the overlay is already building, so the banner still renders this frame" versus "calling onDismiss from a user action DOES hide the banner slot until the next new patch becomes ready" matches :2604-2613 (_updateReady set false by the callback) and :2530/:2560 (only a fresh onUpdateReady sets it back). The rebuild-frequency claim is grounded too — build reads MediaQuery.of(context) at :2603, so keyboard/rotation genuinely re-invoke the builder.
  • Un-gating the example's Rollback stayed un-gated and explained. example/lib/main.dart:147-155 records why not to gate on _isPatched, so the gate can't quietly come back.

Merge gate: one open Medium. Per the policy it can merge if filed as its own issue labelled deferred-medium first — but I would close it here instead: the advice in four copies currently does not work for the reader who follows it, and option (a) or (b) is a wording change in the same files this PR already touches. The three Lows are one-clause edits.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test (both tabled on the thread; test/ still has no widget test and #29 holds the seam), the statusPatchActive constant + test (tabled in round 11 as a follow-up — though the Medium above may make that constant moot if the latch moves behind a ValueListenable<bool>), and the unguarded .then at :362 (filed as #30, correctly not worked here).

No Dart SDK in this environment, so dart analyze / dart test were not re-run; the Dart-side diff is comments only.

…verlay re-key; the status edge/latch does not) — reverted latch guidance across all 6 copies; debug-bar sample = Restart to apply
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 12 pushed (8f4ebcd) — the convergent fix. You proved the decisive asymmetry: a level survives the overlay's subtree re-key, an edge does not. So my round-11 latch guidance was wrong (the latch dies when CodePushOverlay re-keys ValueKey<bool>(_patchActive)), and moduleResult — a ValueNotifier level, re-read after re-inflation, which is why CodePushPatchBuilder keeps working — is the honest answer (your option b).

Reverted to moduleResult as THE app-facing iOS signal, with the no-op-null caveat spelled out (it signals content, not merely 'a patch is active'; a pure code patch leaves it null). Explicitly warn against latching on status from a widget under the overlay. Applied to all six copies: the status field dartdoc, moduleResult dartdoc, bannerBuilder, README ### status + ### isPatched reference sections, README overlay section, CHANGELOG, and example/lib/main.dart (whose header now names the re-key-recreates-State artifact as the lesson).

Lows: README debug-bar sample is now CP: Restart to apply (Patch active is removed the same frame it'd render, and is never written on Android/desktop).

dart analyze clean. This is the last substantive iteration — moduleResult is the only signal that is both app-facing and survives re-inflation, stated with its one caveat, so there is no remaining hole to reshape. If anything further surfaces it's wording, and I'll table-and-merge under the ≤2-Medium gate.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 12 (8f4ebcd) is doc + comment only, plus the one onPressed: line in the example — no signature change, no behavior change, and the check/download/install/rollback path is untouched. The round-11 Medium is verified fixed at its root: the latch-on-status instruction is gone from all copies, and the replacement's load-bearing claim holds. moduleResult.value = result (lib/src/code_push.dart:1964) is written before status.value = 'Patch active' (:1965), so by the time the status listener latches (:2539-2540) and build re-keys the subtree with ValueKey<bool>(_patchActive) (:2575), the level is already set — a ValueListenableBuilder re-inflated by that re-key reads the correct value, where an edge latch would not. The null caveat is real and correctly stated (:1929-1936:1964), and isPatched really is engine-channel-only (:1671), which rollback's own doc confirms is disabled on iOS (:1680-1681).

🟠 Medium

  • lib/src/code_push.dart:176-178 / :2451-2453 / example/lib/main.dart:120-121 — the newly-added CodePushPatchBuilder pointer cannot deliver the payload shape the same sentence describes. The moduleResult dartdoc now reads: "On iOS, bytecode modules return a JSON string which is auto-parsed into a Map<String, dynamic>. Listen to this (e.g. with a ValueListenableBuilder or [CodePushPatchBuilder]) to drive OTA UI". But CodePushPatchBuilder gates on result is String (:2698); the auto-parse at :1952-1958 replaces a JSON string with a Map/List before it ever reaches moduleResult, so every payload matching sentence one falls through to builder(context, null, child) at :2711 — the baseline branch. The same clause is in bannerBuilder (:2452-2453), and the example is narrower still: example/lib/main.dart:120-121 tells iOS readers to use "the CodePushPatchBuilder below" as the active-patch signal, and that builder is patchKey: 'banner' (:165), which additionally requires a raw string literally prefixed banner: (:2700).

    Failure scenario: an iOS developer follows the docstring, wraps their patched UI in a CodePushPatchBuilder, ships a module returning {"promo":"…"} — the documented normal case — and gets patchData == null and the baseline UI forever, with no error anywhere. README.md:354-357 compounds it ("If patchKey is null, all module results are passed through" — untrue for any non-String result), though that line is pre-existing rather than from this PR.

    This is the CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 shape once more, in the fix for CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25. Cheapest fix: drop or [CodePushPatchBuilder] from :178 and :2453 and point the example comment at moduleResult directly — ValueListenableBuilder alone is accurate and is already named first. (Teaching the widget to handle Map/List would be the better answer, but that is a behavior change and belongs in its own PR.)

🟡 Low

  • lib/src/code_push.dart:1668-1671isPatched's own dartdoc is the one copy that never got the iOS caveat. It still says only "Returns false if the code push engine is not available." The class doc (:2458-2459), README (:203-208, :306), CHANGELOG (:6) and the example (example/lib/main.dart:118-125) all warn it reads false on iOS while a patch is active — and the dartdoc is what IDE hover shows the person about to call it. Same "put the caveat where the reader is" argument that moved the throws-warning onto checkForUpdate in round 2 and the init-race note onto the class doc in round 8.
  • README.md:256-269 — the ### CodePush.moduleResult section is the destination of three pointers this PR added, and it wasn't updated. README.md:253-254 now sends readers there ("use CodePush.moduleResult (a level, which survives the re-key), below"), as do :208 and :301; on arrival the section still reads as the pre-PR text — no "app-facing iOS patch signal" framing, and crucially no null-for-a-patch-with-no-return-value caveat, which is the one fact that keeps the recommendation from being misread as "a patch is active". Every other copy carries it.
  • example/lib/main.dart:9-11 — the header comment still says the iOS banner appears "only when a different patch arrives while one is already loaded", dropping the resident-patch-re-offer-after-rollback case that rounds 9-12 added to the dartdoc (:2450-2451) and README (:302-304), and which is real (:790-806 + _contentRevertedThisSession at :1770). Four words restore the "only" to honesty.

🟢 Positives

  • The edge-vs-level revert is the right call, and it was argued from the mechanism rather than the symptom. moduleResult is the only signal that is simultaneously app-facing, iOS-populated, and re-read after the overlay's KeyedSubtree re-inflation — and the write ordering at :1964-1965 means the level is already correct when the new subtree builds. Twelve rounds after checkForUpdate (throws), status (stringly, then edge-not-level), a caller-supplied onUpdateReady (superseded) and isPatched (false on iOS) each failed, this is the first candidate that survives the re-key — and it ships with its own limitation stated rather than discovered later.
  • The caveat is the honest kind. "It stays null for a patch that loaded with no return value, so it signals content, not merely that some patch is active — a pure code patch leaves it null" is exactly :1929-1936:1964, and it is the sentence that stops the recommendation from becoming the next CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25.
  • The platform split is still drawn where the code draws it. Live-load with no builder invocation is :810-833:1960-1965; "a different patch while one is already loaded" is the _moduleLoaded branch at :716-723; the rollback re-offer is debugPersistOutcomeShowsRestart:790-806. Three distinct paths, each named correctly.
  • Un-gating the example's Rollback is correct and is explained so it stays un-gated (example/lib/main.dart:150-158). The path is genuinely failure-soft on iOS: the engine attempt is swallowed (:1698-1703) and the Dart-side branch throws only CodePushException (:1708, :1712), which _rollback already surfaces as a status string (:96-98).
  • The checkForUpdate block is accurate end to end (:1603-1610): MissingPluginException falls through the bare catch (e) at :1629 and rethrows as CodePushException, hasCodePushEngine returns false rather than throwing via two .timeout(2s) probes inside catch (_) (:897-923), and the "those download and install the patch, unlike this check-only call" distinction is real.
  • Twelve rounds and the in-repo copies are still moving together. The Lows above are the residue of that discipline, not a lapse in it — and the docs-site copy that produced CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25 is tracked in code-push-website #38 rather than left to re-drift.

Merge gate: one open Medium — mergeable under the policy if filed as its own issue labelled deferred-medium first, but it is deleting four words in two dartdocs plus one example comment line, so closing it here is cheaper than filing. The three Lows are one-clause edits apiece.

Not re-raising: the ## Unreleased heading and the missing CodePushOverlay widget test (tabled; test/ still has no widget test, and #29 holds the seam), the statusPatchActive constant (tabled in round 11 — and largely moot now that status is documented as not app-facing), and the unguarded .then epoch gap (filed as #30, correctly not worked here).

No Dart SDK in this environment, so dart analyze / dart test were not re-run; the Dart-side diff is comments plus the one onPressed: change.

…with ValueListenableBuilder<Object?> (CodePushPatchBuilder needs a keyed String); add isPatched dartdoc iOS caveat + README moduleResult section + example rollback case
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round 13 pushed (b9619f2):

  • Medium (CodePushPatchBuilder can't consume moduleResult): correct — the auto-parse turns the JSON string into a Map/List before it reaches moduleResult, and CodePushPatchBuilder gates on result is String (with a key: prefix), so it'd fall through to baseline. Removed that pointer from all three copies (moduleResult dartdoc, bannerBuilder, example) and pointed at a plain ValueListenableBuilder<Object?> over moduleResult, which delivers the parsed value. Noted CodePushPatchBuilder is a convenience for keyed string patches, distinct from the auto-parsed Map.
  • Low: isPatched's own dartdoc now carries the reads-false-on-iOS caveat (the copy IDE hover shows); the README ### CodePush.moduleResult section got the app-facing framing + null caveat + a ValueListenableBuilder example (was the un-updated destination of three pointers); the example header restores the rollback-re-offer case to the "only when" list.

dart analyze clean. Every signal is now stated with what it actually delivers — moduleResult (level, auto-parsed value, null for no-op) via ValueListenableBuilder<Object?>; status (edge, don't latch under the overlay); isPatched (false on iOS). No remaining pointer to a widget/shape that doesn't match.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 13 (b9619f2) is doc + comment only apart from the example's one onPressed: line — no signature change, no behavior change, the check/download/install/rollback path untouched. Round-12 items verified fixed: the CodePushPatchBuilder pointer is gone from the moduleResult dartdoc (lib/src/code_push.dart:183-185), bannerBuilder (:2457-2461) and the example (example/lib/main.dart:125-127), and the replacement is accurate — the auto-parse at :1955-1964 only swaps in the decoded value when it is a Map/List, so a plain non-JSON string still reaches that widget's result is String gate (:2703); isPatched's own dartdoc carries the iOS caveat (:1669-1675), matching :1678; and example/lib/main.dart:9-13 restores the rollback-re-offer case, which is real (:792-808 + _contentRevertedThisSession at :1775).

🟠 Medium

1. lib/src/code_push.dart:169-171 — the fallback offered next to moduleResult ("or own the lifecycle with checkAndInstall instead of the overlay") yields no iOS patch signal under either reading.

The paragraph's subject is "an app-facing iOS signal". Both ways a reader can take that clause fail on iOS:

  • Use checkAndInstall's onUpdateReady: — the iOS first-patch path is :812:831 _iosLoadPayload, which returns without ever calling onUpdateReady. The only iOS site that calls it is :808, inside the if (_moduleLoaded) branch at :718. That is the same Android-only-contract fact the PR already documents two paragraphs down for bannerBuilder.
  • Latch status yourself, since without the overlay nothing re-keys your subtree — which the preceding sentence invites by attributing the failure solely to the re-key ("disposing any latch a widget under it holds"). The edge is lost for a second, independent reason: an app owning the lifecycle calls init, and _startUpdateFlow awaits _iosReloadInstalledPatch (:380), which sets 'Patch active' (:1970), then runs checkAndInstall in the same continuation (:413) → 'Checking server...' (:463) → 'Patch already installed' (:592). On an ordinary cold start with a resident patch the whole edge passes inside that chain, before any widget can attach — and resume checks overwrite it again.

Failure scenario: an iOS app drops CodePushOverlay, calls CodePush.init in main() with onUpdateReady: (or latches status in a widget's initState), and renders patched UI from that. Nothing fires on the first patch — the exact case the clause exists for. This is the #25 shape again. README.md:253-255 carries the same paragraph without the clause and is already correct, so the fix is deleting it from the dartdoc.

2. README.md:457-459, README.md:363-366 and lib/src/code_push.dart:2685-2688 — the round-12 CodePushPatchBuilder correction reached three copies but not the README's, and the widget's own contract is still documented backwards.

Round 13 reports the pointer removed "from all three copies", but Platform Behavior still tells iOS readers the opposite: "Listen to CodePush.moduleResult or use CodePushPatchBuilder to react to live patches." And both README.md:365-366 and the widget's own dartdoc (:2687-2688) still say "If patchKey is null, all module results are passed through":2703 gates on result is String, so a Map/List result (which this PR now documents as the normal iOS payload at :176-177) falls to builder(context, null, child) at :2716, the baseline branch.

Failure scenario: an iOS developer reads Platform Behavior, wraps patched UI in CodePushPatchBuilder with patchKey: null on the strength of "all module results are passed through", ships a module returning {"promo":"..."}, and gets patchData == null and baseline UI forever, with no error anywhere. README.md:365-366 predates this PR, but :183-185 now contradicts it directly, so it is a fresh in-repo contradiction rather than inherited drift. One clause in each of the three places (only string results pass through).

🟡 Low

  • lib/src/code_push.dart:174-185 — the null caveat names only one of the two ways moduleResult reads null while a patch is loaded. rollback() also sets moduleResult.value = null (:1777) while the module stays resident — deliberately, with the reasoning at :1765-1767 and :196-199. Since this dartdoc's job is to say exactly when the level under-reports, the kill-switch/rollback case belongs in the same sentence.
  • example/lib/main.dart:159-161 — un-gating Rollback exposed a path _rollback does not catch. It handles on CodePushException only (:97-99), but on the iOS Dart-side branch the existsSync()/deleteSync() calls at lib/src/code_push.dart:1740-1745 are unguarded and throw FileSystemException (the writeAsStringSync above them is wrapped, :1723-1734). Before this round the button was gated on _isPatched, false on iOS, so that branch was unreachable from the example. Un-gating is right; widening the catch is the one-line follow-through, in the file people paste. rollback()'s dartdoc (:1684-1694) also only implies CodePushException.
  • CodePushPatchBuilder has no test, and unlike the tabled CodePushOverlay test it needs no seam. It is a StatelessWidget over a ValueNotifier — no initState, no channel, no live CodePush.init; setting CodePush.moduleResult.value plus pumpWidget is a complete test today. Nothing in test/ references it (ios_rollback_quarantine_test.dart is the only file touching moduleResult), which is why the patchKey: null contract in Medium 2 could stay wrong in three copies.

Not re-raising: the ## Unreleased heading and the CodePushOverlay widget test (tabled; #29 holds the seam), the statusPatchActive constant (tabled round 11, and moot now that status is documented as not app-facing), the unguarded .then epoch gap (#30), and the docs-site copy (code-push-website #38).

🟢 Positives

  • The round-12 fix went at the misunderstanding, not the sentence. The dartdoc does not merely drop CodePushPatchBuilder; it says what that widget is for (key:-prefixed string patches), so a reader cannot re-derive the same wrong pointer — the same move as the round-9 isPatched correction.
  • moduleResult is the right primitive and the load-bearing ordering holds. moduleResult.value = result (:1969) is written before status.value = 'Patch active' (:1970), so the level is already correct when the status listener latches (:2544) and build re-inflates the subtree with ValueKey<bool>(_patchActive) (:2580). After checkForUpdate (throws), status (stringly, then edge-not-level), a caller's onUpdateReady (superseded) and isPatched (false on iOS) each failed, this is the candidate that survives the re-key — shipped with its limitation stated rather than discovered later.
  • isPatched's caveat finally landed on the symbol itself (:1669-1675) — four other copies had carried it for rounds while the dartdoc IDE hover shows did not. Same discipline that moved the throws-warning onto checkForUpdate in round 2 and the init-race note onto the class doc in round 8.
  • The platform split is still drawn where the code draws it. Live-load-without-banner is :812-838:1965-1970; "a different patch while one is already loaded" is the _moduleLoaded branch at :718-725; the rollback re-offer is debugPersistOutcomeShowsRestart:792-808. Three distinct paths, each named correctly, and the example header now lists all three.
  • The example's single-flight fix remains the highest-leverage line in the PRexample/lib/main.dart:78-83 no longer reports a lost _checkInFlight race as "No update available".
  • Un-gating Rollback records why it must stay un-gated (example/lib/main.dart:154-158), so the _isPatched gate cannot quietly come back. The Low above is about the catch clause, not the decision.
  • Thirteen rounds and the in-repo copies still move together. Both Mediums above are missed copies rather than new wrong claims — which is the failure mode this cross-copy discipline exists to surface early.

Merge gate: two open Mediums, so this is mergeable under the policy with each filed as its own deferred-medium issue first — but Medium 1 deletes one clause in one dartdoc and Medium 2 is one clause in each of three places, so closing them here is cheaper than filing.

No Dart SDK in this environment, so dart analyze / dart test were not re-run; the Dart-side diff is comments plus the one onPressed: change.

… builder test

- Remove the checkAndInstall fallback clause from status dartdoc (it never
  fires onUpdateReady on the iOS first-patch path — no app-facing signal).
- Correct CodePushPatchBuilder contract in three copies (widget dartdoc +
  two README passages): only *string* results pass through; a Map/List
  payload yields the baseline branch. Delimiter is a colon, not a pipe.
- moduleResult caveat now names both null cases (pure code patch; revert
  to baseline via rollback or an automatic post-failure revert).
- Widen the example Rollback handler to catch FileSystemException from the
  iOS Dart-side patch delete, not just CodePushException.
- Add a widget test locking the CodePushPatchBuilder pass-through contract.
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 14 (1dad5d9) is docs plus one new test file and one widened catch in the example — no signature change (bannerBuilder is still Widget Function(...)?, unchanged), and the check/download/install/rollback path is untouched. Round-13 items verified fixed at the source:

  • The checkAndInstall-as-iOS-fallback clause is gone from the status dartdoc (lib/src/code_push.dart:161-171).
  • The CodePushPatchBuilder contract is corrected in all three remaining copies — README.md:363-368, README.md:460-461, lib/src/code_push.dart:2685-2690 — and each now matches :2705 (result is String && result.isNotEmpty).
  • The rollback null case landed in the moduleResult dartdoc (:180-183) and is real in both branches it names: deliberate rollback at :1777 and the automatic post-failure revert at :2233.
  • The example's _rollback now catches the non-CodePushException path (example/lib/main.dart:99-105), which is the genuinely unguarded existsSync()/deleteSync() at lib/src/code_push.dart:1740-1745.

🟠 Medium

1. example/lib/main.dart:78-84 — the new "what false means" comment enumerates two of ~17 causes, and the message it prints is wrong for the ones a developer most needs to see.

The comment says false means "no update, or another check is already running", and the user-facing string says the same. But checkAndInstall returns false from seventeen sites between :459 and :862, and the majority are failures, not "no update":

  • lib/src/code_push.dart:476Insecure server URL refused
  • :498-500any non-200, so a 500 from the server prints No update (500) internally and "no new patch installed (no update…)" in the example
  • :549-550Insecure patch URL refused
  • :636-640Engine ABI mismatch
  • :685-687Download failed (404)
  • :690-692Empty patch
  • :860-862 — the blanket catch (e)status.value = 'Error: $e'

Failure scenario: a developer pastes the example, points it at a server that 500s (or ships a patch whose hash fails verification), taps Check for Updates, and is told there is no update. The reason is sitting in CodePush.statuscheckAndInstall writes a specific string before every one of those returns — but the example throws it away.

This is the same "an enumeration that isn't exhaustive reads as a contract" shape the PR exists to fix, in newly-added text. One-line fix that makes the example strictly better than any enumeration:

if (!installed) {
  // `false` is not "no update" — it is also "a check is already running",
  // "download failed", "hash mismatch", "server error". The reason is in
  // CodePush.status, which checkAndInstall writes before every false return.
  setState(() => _status = 'No new patch installed: ${CodePush.status.value}');
}

2. lib/src/code_push.dart:2693-2695 — the builder field's own dartdoc still makes the exact claim the patchKey doc eight lines above was just corrected to deny.

:2693 reads: "Builder called with the patch data string (or null if no patch)." :2686-2688 now says the opposite — a Map/List payload, which :175-177 documents as the normal iOS shape, yields patchData == null while a patch is loaded. So the two dartdocs in the same class disagree, and the wrong one is on builder:, the required parameter every user writes and the line IDE hover shows while they type it.

Failure scenario: identical to round-13's Medium 2, which the author accepted — a developer writes if (patchData == null) return child!; (as README.md:356 and example/lib/main.dart:177 both model), ships a module returning {"promo":"…"}, and renders baseline forever with no error. The patchKey correction doesn't reach them because they never read the patchKey doc; they read the callback's. Same "put the caveat where the reader is" argument that moved the throws-warning onto checkForUpdate and the iOS caveat onto isPatched in round 13. One clause: "…or null when there is no patch, when the result is not a string, or when it doesn't match patchKey."

🟡 Low

  • The two-null-cases correction landed in one copy of five. lib/src/code_push.dart:180-183 now names both the no-return-value patch and the revert-to-baseline case; README.md:264-266, README.md:311-313, the bannerBuilder dartdoc at :2459-2460, and CHANGELOG.md:6 all still say only "null for a patch with no return value". The rollback case is the one that makes moduleResult go null while the module is still resident (:1768-1777) — i.e. the case where a reader's mental model of "level = patch active" actually breaks — so it belongs in the copies that recommend it, not just the one that defines it.
  • lib/src/code_push.dart:2689-2690 / README.md:367-368 — "every string result is passed through as-is" is false for the empty string, and this PR's own new test says so. :2705 gates on result is String && result.isNotEmpty, and test/code_push_patch_builder_test.dart:49-53 pins '' → baseline branch. The new test is right; the sentence it tests against is the one that's off by one word (every non-empty string).
  • lib/src/code_push.dart:2662-2676 — the class-level dartdoc, which is what hover on CodePushPatchBuilder shows, never got the string-only framing. It still reads "A widget that rebuilds when a code push module result is available. Use this to apply OTA patches to specific parts of your UI" — the general-purpose framing that produced the round-12/13 Mediums. The correction sits on the patchKey field, which a reader with patchKey: null has no reason to open.
  • example/lib/main.dart:127-135 tells iOS readers to use ValueListenableBuilder<Object?> over moduleResult, and the file never shows one. The only live-patch widget in the example is the CodePushPatchBuilder at :174-192 that the same comment says is not the iOS signal (:133-135). The file people copy therefore demonstrates the pattern it warns against and omits the one it prescribes; ~8 lines next to the existing builder would close it, and would double as the worked example for the Medium-2 caveat.

Not re-raising: the ## Unreleased heading and the CodePushOverlay widget test (tabled; #29 holds the seam), the statusPatchActive constant (tabled round 11, moot now that status is documented as not app-facing), the unguarded .then epoch gap (#30), and the docs-site copy (code-push-website #38).

🟢 Positives

  • The new test is the right test, built the way the round-13 note argued it could be. test/code_push_patch_builder_test.dart needs no channel, no CodePush.init, no seam — moduleResult.value = … plus pumpWidget — and it pins the contract at exactly the points that were documented backwards for three rounds: Map → baseline (:41-47), empty string → baseline (:49-53), patchKey: null passthrough (:55-60), prefix strip (:62-67), mismatch (:69-74), and rebuild-on-change (:77-86). The #unset sentinel at :15 is the detail that makes it load-bearing: a builder that is never invoked fails expect(lastPatchData, isNull) instead of passing by accident. This is the first test in the repo that would have caught the round-12 Medium.
  • Widening the example's rollback catch is cited to a real path, not a defensive reflex. lib/src/code_push.dart:1723-1734 wraps its writeAsStringSync but :1740-1745 leaves existsSync/deleteSync bare, so FileSystemException genuinely escapes rollback() on the iOS Dart-side branch — reachable from the example only because round 13 un-gated the button. The comment at :100-103 records why, so the catch can't be tidied away later.
  • Both new moduleResult-is-null claims were checkable and check out. :1777 (deliberate rollback, with the reasoning at :1765-1767) and :2233 (auto-revert, with the _contentRevertedThisSession reset above it) — and the caveat is phrased as "signals content, not merely that some patch is active", which is the sentence that stops the recommendation from becoming the next CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects #25.
  • The failure-soft guarantee holds end to end on the path this PR documents. checkAndInstall's blanket catch (e) { status.value = 'Error: $e'; return false; } at :860-862 with finally { _checkInFlight = false; } at :863-865 means no update-path exception reaches the host app and no failure wedges the single-flight guard. Medium 1 above is about the example discarding that status string, not about the guarantee.
  • Fourteen rounds and every Medium is now a missed copy, never a new wrong claim. Both Mediums here are drift between copies that already agree in substance — which is the failure mode the cross-copy discipline exists to surface, working as intended.

Merge gate: two open Mediums — mergeable under the ≤2 policy provided each is filed as its own issue labelled deferred-medium first. Both are one-clause edits (example/lib/main.dart:79-83, lib/src/code_push.dart:2693), so closing them here is cheaper than filing. The four Lows are one clause apiece plus one ~8-line example block.

No Dart SDK in this environment, so dart analyze / dart test were not re-run; the new test was read against lib/src/code_push.dart:2700-2721 rather than executed.

Comprehensive pass over all copies the review flagged as out of sync:
- CodePushPatchBuilder: correct the builder-field dartdoc and class-level
  dartdoc to the string-only contract (were still general-purpose); fix
  'every string' -> 'every non-empty string' in the field dartdoc and README
  (an empty string yields the baseline branch, as the new test pins).
- moduleResult null caveat now names both cases (no-return-value patch AND
  revert-to-baseline) in every remaining copy: bannerBuilder dartdoc and both
  README passages.
- Example: the 'false' branch surfaces CodePush.status instead of guessing
  'no update' (checkAndInstall returns false from many failure sites), and a
  ValueListenableBuilder<Object?> over moduleResult is now shown, since the
  example's own note prescribes it as the iOS signal.
- CHANGELOG: sanitize the Unreleased section to minimal, user-facing notes
  (dropped internal overlay-re-key/latch/edge language per the CHANGELOG
  minimalism rule), which also removes the drifting null-case caveat.
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

🔴 Critical

None. Round 15 is doc/comment-only apart from example/ (the _manualCheck message, the widened _rollback catch, one new ValueListenableBuilder) plus a new test file. bannerBuilder is still Widget Function(...)? (lib/src/code_push.dart:2478-2482) — no signature change, no break for existing users — and the check/download/install/rollback path is untouched.

Round-14 items verified fixed at the source: the builder field dartdoc carries the not-a-string caveat (lib/src/code_push.dart:2699-2701), matching the gate at :2713; the class dartdoc got the string-only framing (:2666-2669); the two-null-cases correction reached README.md:264-267, README.md:311-313 and :2467-2471, and both branches are real (:1777 deliberate rollback, :2233 auto-revert); README.md:368-369 and :2694-2696 now say non-empty string.

🟠 Medium

1. example/lib/main.dart:82-83 — "which checkAndInstall writes before every false return" is wrong for exactly the case the same comment names first.

Of the eighteen return false sites in checkAndInstall, seventeen write a status string first — but lib/src/code_push.dart:459 (if (_checkInFlight) return false;) returns before status.value = 'Checking server...' at :463 and never touches the notifier. That is the single-flight case, which the new comment lists as its first example ("a check is already running").

Failure scenario: the overlay's launch check (or the resume check at :2564) is mid-flight when the user taps Check for Updates. The call returns false in one statement and CodePush.status.value still holds the other check's progress, so the demo renders No new patch installed: Downloading patch... or ... Restart to apply — a foreign operation's state presented as this tap's reason. The example's own header at :13-14 says this race is expected, so it is routine, not a corner.

checkAndInstall's private comment at :456-458 already has the right words. One-line fix: write a status at :459 (status.value = 'A check is already running'), or drop "every" from the example comment.

2. example/lib/main.dart:58-65 — every setState in the demo is unguarded, in the one widget this PR documents as being destroyed out from under it.

_loadStatus calls setState at :61 after two awaited channel round trips with no mounted check; same in _manualCheck (:68, :75, :84, :89) and _rollback (:94, :97, :100, :106) — mounted appears nowhere in the file. But _CodePushDemoState sits under KeyedSubtree(key: ValueKey<bool>(_patchActive), ...) (lib/src/code_push.dart:2582), and when _onModuleLoaded flips _patchActive on the 'Patch active' edge (:2546-2548, written at :1970) the key changes, the subtree is re-inflated, and this State is disposed. The file's own comment at :134-135 names that artifact — "this State itself is recreated on iOS load" — without drawing the consequence for its own await-then-setState methods.

Failure scenario (ordinary iOS cold start with a resident patch): initState_loadStatus() is awaiting CodePush.isPatched when _iosReloadInstalledPatch sets status.value = 'Patch active'; the overlay re-keys on the next frame and disposes this State; the channel reply lands afterwards and setState hits assert(_debugLifecycleState != _StateLifecycle.defunct). Nothing catches it — _manualCheck catches only on CodePushException (:88). Silent in release (markNeedsBuild returns early for a non-active element), so a debug-time paper cut rather than a shipped crash — but debug is where people run the example. if (!mounted) return; after each await, which is also the pattern worth modelling in a file people paste.

🟡 Low

  • example/lib/main.dart:181 — the new worked example renders the misreading its own file warns against: if (result == null) return const Text('No live patch payload') prints that for a pure code patch that is currently active, the caveat spelled out four lines up at :174-177 and at lib/src/code_push.dart:180-183. It is the only rendering of moduleResult in the repo, so it is what gets copied.
  • example/lib/main.dart:67-91 — the iOS success path produces no feedback at all. checkAndInstall returns true from _iosLoadPayload (lib/src/code_push.dart:831) without calling onUpdateReady (documented at :2461-2466), so neither :74-76 nor the !installed branch at :78 fires and _status stays 'Checking for updates...' (or resets to 'Idle' on the re-key). The demo can never report the one outcome the iOS docs are about.
  • lib/src/code_push.dart:442-444 and README.md:143-144checkAndInstall's own contract never got the false-is-not-"no update" caveat the example just learned; both still say only "Returns true if a patch was installed". The honest version lives in a private comment at :456-458 that hover never shows. Medium 1 is downstream of this — same "put the caveat where the reader is" argument that moved the throws warning onto checkForUpdate (:1603-1610) and the iOS caveat onto isPatched (:1672-1675).
  • README.md:350-351 — the CodePushPatchBuilder section keeps the general framing the class dartdoc was just corrected away from ("Use this to apply OTA patches to specific parts of your UI" — verbatim the wording that produced the round-12/13 Mediums). The correction is at :364-365, below the code sample a reader may already have copied.
  • Nothing runs the new test. .github/workflows/ has only claude-review.yml — no flutter test job — so test/code_push_patch_builder_test.dart and the twelve existing test files never execute on a PR. A contract-pinning test CI doesn't run can't stop the contract drifting again, which is why it was written. (Same file, minor: child: precedes builder: at :22-23, which sort_child_properties_last flags; no Dart SDK here to check that against the "analyze clean" claim.)

Not re-raising: the ## Unreleased heading and the CodePushOverlay widget test (tabled; #29 holds the seam), the statusPatchActive constant (tabled round 11), the .then epoch gap (#30), and the docs-site copy (code-push-website #38).

🟢 Positives

  • The new test is the first thing in test/ that would have caught the round-12 Medium. No channel, no CodePush.init, no seam — and it pins the contract at every point that was documented backwards for three rounds: Map → baseline (:41-47), empty string → baseline (:49-53), patchKey: null passthrough (:55-60), prefix strip (:62-67), mismatch (:69-74), rebuild-on-change (:77-86). The #unset sentinel at :15 is what makes it load-bearing: a builder never invoked fails expect(lastPatchData, isNull) instead of passing by accident, and tearDown at :37 keeps the global ValueNotifier from leaking between tests.
  • The prescribed pattern finally exists in the file people copy. Fourteen rounds of "listen to moduleResult with a ValueListenableBuilder<Object?>" pointed at nothing; example/lib/main.dart:178-184 is a real one, sitting next to the CodePushPatchBuilder the same comment says is not the iOS signal — the contrast is the teaching.
  • The widened rollback catch is cited to a real path. lib/src/code_push.dart:1723-1734 wraps its writeAsStringSync but :1740-1745 leaves existsSync/deleteSync bare, so FileSystemException genuinely escapes rollback() on the iOS Dart-side branch — reachable from the example only because round 13 un-gated the button, and :102-105 records why so it can't be tidied away.
  • The failure-soft guarantee holds end to end on the path this PR documents. catch (e) { status.value = 'Error: $e'; return false; } (:860-862) with finally { _checkInFlight = false; } (:863-865) means no update-path exception reaches the host app and no failure wedges the single-flight guard; _iosLoadPayload routes both its false returns through rollback with a status written first (:1909-1918, :1996-2005). Both Mediums are about the example mishandling that outcome, not the guarantee.
  • The load-bearing ordering still holds. moduleResult.value = result (:1969) precedes status.value = 'Patch active' (:1970), so the level is already correct when the status listener latches (:2546) and build re-inflates the subtree (:2582) — precisely why moduleResult survives the re-key where a status latch does not.

Merge gate: two open Mediums — mergeable under the ≤2 policy provided each is filed as its own deferred-medium issue first. Both are small (Medium 1 is one line; Medium 2 is four if (!mounted) return; guards), so closing them here is cheaper than filing.

No Dart SDK in this environment: dart analyze and flutter test were not run. The new test was read against lib/src/code_push.dart:2709-2729; the return false census for Medium 1 was taken over all of checkAndInstall (:445-866), and :459 is the only one of the eighteen that does not write status first.

@fonkamloic

Copy link
Copy Markdown
Contributor Author

Table-and-merge (round 16), per the ≤2-Medium gate.

Round 16 is 0 Critical / 2 Medium / 5 Low. Both Mediums are about the example code this round added, and — as the review itself notes — every Medium for several rounds has been a missed copy, never a new wrong claim; the SDK library docs (the deliverable) are now correct and consistent. Both Mediums are filed as deferred-medium:

Lows tabled (each a one-clause/example-only nit); the contract-pinning test (test/code_push_patch_builder_test.dart, 7 cases) landed this round. Merging.

@fonkamloic
fonkamloic merged commit 34d1773 into main Aug 21, 2026
1 check passed
@fonkamloic
fonkamloic deleted the fix/banner-builder-docstring branch August 21, 2026 03:28
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.

CodePushOverlay.bannerBuilder: the docstring promises a null return the type rejects

1 participant