Skip to content

fix(ui): handle notification_status from the shell and offer a manual retry - #542

Merged
sanity merged 5 commits into
mainfrom
fix-510
Jul 29, 2026
Merged

fix(ui): handle notification_status from the shell and offer a manual retry#542
sanity merged 5 commits into
mainfrom
fix-510

Conversation

@sanity

@sanity sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Problem

River's shell-bridge listener (ui/src/components/app/notifications.rs) dropped
every message whose type was not notification_click:

if kind != "notification_click" {
    return;
}

The gateway shell reports the outcome of its permission offer as
notification_status (notifyStatusToIframe in freenet-core's
shell_bridge.js), with granted / denied / dismissed / undeliverable /
unsupported / default. All six were discarded identically, and
notification_status appeared nowhere in ui/src. River could not tell a
granted permission from a blocked one and surfaced nothing to the user.

Worse, request_enable_via_shell was latched by a ENABLE_PROMPT_SENT
AtomicBool with no reset, and the bell modal only chose All / Mentions /
Muted — so a prompt that was blocked, snoozed, or mis-clicked could not be
re-requested for the rest of the session. Net effect: notifications silently
never worked, with no explanation and no path to fix it.

This is River's half only. The shell-side defect is freenet/freenet-core#4966;
nothing here depends on it — the shell already sends notification_status on
main.

Approach

Consume the status. A new notification_status arm parses the shell's
status string into a NotificationStatus enum and records it in a
NOTIFICATION_STATUS global. An unrecognised string is ignored rather than
guessed at, so a newer shell adding a status can never overwrite a known one
with a wrong reading. The signal write goes through crate::util::defer, as
required for a write originating in a JS MessageEvent callback
(.claude/rules/dioxus-signal-safety.md), and record_notification_status is
the only writer so no path can bypass the re-arm below.

Re-arm the ask, bounded. A status of default (the prompt closed with no
answer) clears ENABLE_PROMPT_SENT so a later gesture can offer again. The
re-arm is capped at one, because the shell re-shows its affordance every time
River asks: uncapped, a user who keeps closing the browser dialog would get the
bar back on every message they send. dismissed deliberately does NOT re-arm —
the shell also snoozes it for 24h, so River re-asking would just bounce off that
snooze.

Surface it, and offer a retry. The bell modal grows a "Browser permission"
section with a short explanation and an "Enable notifications" button. The
button is offered exactly where asking again can change the answer (dismissed,
default, or nothing known yet) — offering it against a browser-level block or
an already-granted permission would just move the silent dead end. Its click
calls request_enable_now() directly, not through crate::util::defer,
because the browser only honours a permission request made inside the click's
transient user activation and defer is a setTimeout(0).

The manual path is deliberately not latched: the once-per-session latch exists
so sending messages doesn't nag, and an explicit click is exactly the retry it
must not swallow.

Served top-level (dev / no-sync) River has a real origin, so the same button
calls the Notifications API directly. Notification::request_permission() is
invoked synchronously inside the click for the activation reason above; only the
awaiting of the already-created promise is deferred. This incidentally avoids
the lost-activation problem the issue notes for the existing auto path, which is
otherwise unchanged.

One bug found while writing this: current_notification_status reads
Notification.permission on every modal render, and web_sys's binding for it
is not Result-returning — reading .permission off an undefined Notification
raises a JS ReferenceError that aborts the WASM module. Guarded by
notification_api_available(). Playwright's mobile-safari project has no
Notifications API at all, so this was reachable.

Testing

cargo test -p river-ui --bins: 785 pass (10 new). New coverage:

  • The six shell status strings parse to distinct variants (a cross-repo wire
    contract — a rename on either side reverts the fix exactly), and unrecognised
    strings are ignored.
  • Only an unanswered prompt re-arms the automatic ask, and the re-arm is bounded.
  • The Enable button is offered exactly where asking again can help, and every
    state has its own non-empty explanation.
  • Source pins for the parts no native test can reach (a JS closure and rsx): the
    listener consumes notification_status; the status write stays inside
    defer and has exactly one writer; request_permission() is called before the
    spawn; the Enable click is not deferred.

Every new test was mutation-tested — the fix reverted or subtly broken in eight
ways, each caught by a named test. One of the eight (raising the re-arm cap to
usize::MAX) initially passed, because the bound assertion was phrased against
the constant it was testing and so held for any cap at all; that vacuous test is
fixed in its own commit.

Playwright notification-bell.spec.ts: 30 pass across chromium, firefox, webkit,
mobile-chrome and mobile-safari. The new spec asserts the explanation always
renders and that the button appears iff the browser state is one where asking
again helps. The five projects cover all three branches — Chromium reports
denied, Firefox and desktop WebKit default, mobile Safari has no API. Both
new data-testids are absent on main, so the spec fails there.

Manually verified in Firefox that clicking the button leaves the app alive and
the modal rendered, with no new console error.

Closes #510

[AI-assisted - Claude]

sanity added 4 commits July 29, 2026 12:14
… retry

River's shell-bridge listener dropped every message whose type was not
`notification_click`, so all five `notification_status` outcomes the gateway
shell reports (granted / denied / dismissed / undeliverable / unsupported, plus
`default`) were discarded identically. A blocked or dismissed permission failed
silently, and the once-per-session `ENABLE_PROMPT_SENT` latch meant it could
never be re-requested.

Handle `notification_status`: parse it, record it in a global signal (deferred,
per the Dioxus signal-safety rule for writes originating in a JS callback), and
re-arm the automatic ask when the prompt closed with no answer. The re-arm is
bounded at one, so a user who keeps closing the browser dialog is not shown the
shell's bar again on every message they send.

Surface the result in the bell modal as a short explanation plus an "Enable
notifications" button, offered exactly where asking again can change the answer.
The click calls `request_enable_now()` directly rather than through
`crate::util::defer`, because the browser only honours a permission request made
inside the click's transient user activation.

Closes #510
Phrased against MAX_ENABLE_PROMPT_REARMS itself, the assertion held for any
cap at all: raising the constant to usize::MAX left the test green. Assert
concrete numbers instead.
Asserts the explanation always renders, and that the Enable button appears
exactly where asking again can change the answer — Playwright's Chromium
reports "denied", Firefox and desktop WebKit "default", and mobile Safari has
no Notifications API at all, so the five projects cover all three branches.
Two findings from the code-first review lens.

Served top-level, current_notification_status preferred the value River
remembered from its own last request over the browser's own answer. A user who
allowed notifications in site settings after once denying them would be told
"your browser is blocking notifications" for the rest of the session, with the
Enable button withheld — a stale silent failure of the shape this PR removes.
The browser is authoritative there and stays authoritative, so the remembered
value is now ignored on that path; it is still read, because the read is what
subscribes the render to the signal.

The 'dismissed' copy also promised a retry that works immediately. That status
covers being inside the gateway's snooze window from an earlier decline, during
which asking again provably shows nothing.
@sanity

sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Review

Two independent adversarial lenses, run serially against the checked-out code (no subagents available in this session). Per ~/.claude/rules/multi-model-review.md as amended 2026-07-26, external non-Claude models are opt-in and were not invoked.

Lens 1 — code-first (read the code before the description; intent vs implementation)

Two findings, both fixed in c8e0126:

  1. Stale permission on the top-level path (medium). current_notification_status preferred the value River remembered from its own last request over the browser's own answer. A user who denied once and then allowed notifications in site settings would be told "your browser is blocking notifications" for the rest of the session, with the Enable button withheld — a stale silent failure of exactly the shape this PR removes. The browser is authoritative when River is served top-level and stays authoritative, so the remembered value is now ignored there. It is still read, because the read is what subscribes the render to the signal; dropping the read would stop the modal updating when a request completes. The precedence is now a pure resolve_status(framed, stored, live) with tests.

  2. dismissed copy promised a retry that doesn't work yet (low). That status also covers "inside the gateway's snooze window from an earlier decline", during which asking again provably shows nothing for up to a day. The old wording ("You can ask for it again") would have put the user back in front of a control that silently does nothing. Reworded to say the prompt may not reappear straight away.

Checked and found correct: all six notifyStatusToIframe strings are covered; the source/__freenet_shell__ checks still gate the new arm; record_notification_status is the only writer, so no path can skip the re-arm; read_browser_permission is invoked lazily via bool::then, so the framed path never reads a meaningless opaque-origin permission.

Lens 2 — skeptical + testing (assume bugs exist; does the status actually reach the UI?)

No new defects.

Does it silently no-op when the shell sends nothing? No. Framed with no status ever reported, resolve_status returns None, which is its own state: the user sees "Desktop notifications are not set up yet for this browser." plus a working Enable button. Both orderings work — a status arriving before the modal opens is read from the signal on open; one arriving while it is open re-renders it through the try_read subscription. defer is safe here because capture_runtime() runs earlier in App() than install_shell_notification_listener().

Against an older gateway shell that predates notifyStatusToIframe, no status is ever sent, so the modal shows the "not set up yet" state and the Enable button posts notification_enable_prompt — a message old shells already handle. Degrades to today's behaviour plus a manual trigger, never to a crash.

Would the tests fail if the fix were reverted? Verified by mutation, not by inspection. Nine mutations, each caught by a named test:

Mutation Caught by
Delete the notification_status arm the_shell_listener_consumes_notification_status
Move request_permission() inside the spawn the_direct_permission_request_stays_inside_the_click_gesture
Drop the defer around the signal write the_status_write_is_deferred_and_has_one_writer
Wrap the Enable click in defer the_enable_click_is_not_deferred
Let dismissed re-arm the auto ask only_an_unanswered_prompt_rearms_the_automatic_ask
Offer Enable on denied enable_is_offered_exactly_where_asking_again_can_help
Rename the "default" token every_shell_status_string_parses
Change the re-arm cap constant the_automatic_rearm_is_bounded
Break the re-arm counter covered NOT COVERED — see correction below
Prefer the remembered status over the live one the_browser_answer_wins_over_a_remembered_one_when_unframed

The re-arm-cap mutation initially passed: the assertion was phrased against MAX_ENABLE_PROMPT_REARMS itself, so it held for any cap including none. Fixed in 423ce60 to assert concrete numbers.

Known gaps, accepted

  • No native test drives the real atomics. Only the pure status_rearms_auto_prompt predicate is tested. The statics are process-global and native tests run in parallel threads, so a test mutating them would be flaky and could corrupt a sibling. The predicate plus the one-writer pin is the trade.
  • No browser test of the framed branch. Simulating the shell would mean standing up a fake shell page posting notification_status. The decision is covered by resolve_status's unit tests and the wire contract by the status-string test.
  • The latent activation problem on the automatic path is not fixed here, deliberately. As the issue notes, request_permission_on_first_message is reached several awaits deep in the send pipeline, so activation is already gone before safe_spawn_local is involved — removing the spawn would not fix it. A real fix means moving the trigger to the send button's click handler, which changes when River prompts. The new manual button gives affected users a working path in the meantime.

[AI-assisted - Claude]


Correction (2026-07-29)

The mutation table above overstated one row, and I am correcting it in place rather than leaving the wrong claim standing.

I originally listed "Remove the re-arm cap" as covered by the_automatic_rearm_is_bounded. That is true only for changing the constant. It is false for breaking the counter: that test asserts MAX_ENABLE_PROMPT_REARMS <= 2 and calls the predicate with a caller-supplied rearms_used, so it passes with ENABLE_PROMPT_REARMS.fetch_add(1, ...) deleted.

Independent review confirmed by execution that both statements of the re-arm could be deleted with the whole suite green:

baseline                                        787 passed
delete ENABLE_PROMPT_REARMS.fetch_add(1, ...)   787 passed   <-- survived
delete ENABLE_PROMPT_SENT.store(false, ...)     787 passed   <-- survived

So the "reset the flag" half of #510 shipped here with no coverage at all. Dropping the SENT reset means an unanswered prompt never re-arms; dropping the REARMS increment leaves the counter at 0, making the re-arm unbounded so the shell's affordance bar returns on every message sent.

My "known gaps, accepted" note above correctly ruled out a behavioural test (process-global statics vs parallel test threads) but stopped there, missing that a source pin has no parallelism problem — an idiom this same file already uses four times.

Fixed forward in #550, along with a vacuous browser assertion, a debug! that is compiled out in release, and the bell-level visibility gap. That PR's pin is verified by watching it fail with each line deleted.

[AI-assisted - Claude]

…ll frame

Stands up a minimal shell parent page (same fixture shape as
invitation-sandbox.spec.ts) that posts notification_status down to a River
iframe and counts the notification_enable_prompt messages River posts up.
Covers the end-to-end path no native test in a WASM crate can reach: framing
detection, the listener arm, the deferred signal write, the framed branch of
resolve_status, the notice mapping, and that the manual retry actually reaches
the shell and is not latched after the first ask.

Also updates the now-stale comment at the listener's install site, which
described it as handling only notification_click.
@sanity

sanity commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Review follow-up: the framed-branch gap is now closed

The review above listed "no browser test of the framed branch" as an accepted gap. It turned out to be cheap to close: invitation-sandbox.spec.ts already establishes a fake-gateway-shell fixture, and notification-shell-status.spec.ts (2501da3) reuses that shape. A minimal shell parent page posts notification_status down to a River iframe and counts the notification_enable_prompt messages River posts up.

That covers, in a real browser and a real cross-frame postMessage, the whole path no native test in a WASM crate can reach: framing detection, the listener arm, the deferred signal write, the framed branch of resolve_status, the notice mapping, and — the half of #510 the one-way latch made impossible — that the manual retry actually reaches the shell, twice in a row.

Mutation-verified rather than assumed. I deleted the notification_status arm, rebuilt the UI, and re-ran the spec: both tests fail, on toContainText — the modal renders but the status never changes what it says, which is #510 exactly. Restoring the arm and rebuilding returns all 10 (2 tests x 5 browser projects) to green.

Local runs on the final code: cargo test -p river-ui --bins 787 pass; Playwright notification-bell + notification-shell-status 40 pass across chromium, firefox, webkit, mobile-chrome and mobile-safari.

Two gaps remain accepted, unchanged: no native test drives the real atomics (process-global statics vs parallel test threads), and the latent activation problem on the automatic prompt path is deliberately out of scope, since the activation is already lost before safe_spawn_local is reached and a real fix would change when River prompts.

[AI-assisted - Claude]

@sanity
sanity merged commit f262d79 into main Jul 29, 2026
6 checks passed
sanity added a commit that referenced this pull request Jul 29, 2026
Mutation testing at #542's head showed both statements of the re-arm could be
deleted with the whole suite green. Each shipped a silent regression: without
the ENABLE_PROMPT_SENT reset an unanswered prompt never re-arms (#510's 'no way
to retry'), and without the ENABLE_PROMPT_REARMS increment the counter stays at
0, so the cap never binds and the shell's bar returns on every message sent.

the_automatic_rearm_is_bounded does not cover the second: it feeds the
predicate a caller-supplied count, so it passes with the increment gone. The
statics are process-global and native tests run in parallel threads, so a
behavioural test is not available; a source pin has no such problem.

Also raise the unrecognised-status log from debug! to warn!. release_max_level_info
compiles debug! out, and that line fires exactly when the cross-repo status
contract has drifted — the failure that reverts #510 wholesale.
sanity added a commit that referenced this pull request Jul 29, 2026
#550)

* test(ui): pin the notification re-arm wiring; warn on an unknown status

Mutation testing at #542's head showed both statements of the re-arm could be
deleted with the whole suite green. Each shipped a silent regression: without
the ENABLE_PROMPT_SENT reset an unanswered prompt never re-arms (#510's 'no way
to retry'), and without the ENABLE_PROMPT_REARMS increment the counter stays at
0, so the cap never binds and the shell's bar returns on every message sent.

the_automatic_rearm_is_bounded does not cover the second: it feeds the
predicate a caller-supplied count, so it passes with the increment gone. The
statics are process-global and native tests run in parallel threads, so a
behavioural test is not available; a source pin has no such problem.

Also raise the unrecognised-status log from debug! to warn!. release_max_level_info
compiles debug! out, and that line fires exactly when the cross-repo status
contract has drifted — the failure that reverts #510 wholesale.

* fix(ui): mark the bell when the browser will not deliver notifications

The per-room modes only decide WHEN River wants to notify. If the browser is
refusing, all of them are inert, and the only place that said so was inside the
modal — so a user with a blocked permission saw an ordinary bell reading
"Notifications: All messages" and learned nothing unless they went looking. For
an issue titled "fails silently", that was the last place the silence lived.

Adds a decorative dot plus an aria-label explaining it. Deliberately not shown
for a Muted room (the user asked for no notifications) or for Unsupported
(nothing the user does could ever clear it).

Also makes the unrecognised-status assertion in the shell spec non-vacuous: it
asserted text that was already present from the preceding status and passed on
its first poll, so it would have passed equally had the junk clobbered the
stored value. It now waits out a window for the non-event, and then posts a
recognised status to prove the listener survived — a parse that threw would
otherwise look identical to correctly ignoring the junk.

* fix(ui): make the badge ring visible; pin the unknown-status log level

MINOR-2: ring-panel generated no CSS. --color-panel is declared on :root,
outside the @theme block, and Tailwind v4 only derives colour utilities from
--color-* registered in @theme; there was no @Utility ring-* either, so ring-1
fell back to currentColor and the separator ring rendered grey, blue on hover.
Verified by grepping the built stylesheet: no .ring-panel rule existed, and
.ring-1 resolves var(--tw-ring-color,currentcolor). Adds the hand-written
@Utility alongside the ones every other :root colour already needed, and
confirmed .ring-panel{--tw-ring-color:var(--color-panel)} now generates.
(ring-[color:var(--color-panel)] was tried first and also generated nothing.)

MINOR-1: the warn! from the previous commit was itself unpinned — reverting that
one token left the whole suite green, the same silent-failure shape this PR
exists to close. Pinned two ways: the framed Playwright test now waits for the
listener's own console line, which also replaces the fixed 500ms sleep with a
positive signal that the junk was handled (a slow clobber slipped past the sleep
as a false pass rather than a flake), and a source pin gives the same guarantee
in the fast native job without depending on Dioxus spelling the level 'WARN' in
the message text.
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.

fix(ui): River discards every notification_status from the shell, so a blocked permission fails silently with no retry

1 participant