Skip to content

feat(mobile): write catch-up and visible-message read marks like the web app - #8106

Open
loganj wants to merge 7 commits into
mainfrom
larry/mobile-activity-mark-writes
Open

loganj wants to merge 7 commits into
mainfrom
larry/mobile-activity-mark-writes

Conversation

@loganj

@loganj loganj commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

🤖

Summary

When you read a channel or thread on the phone, the web and desktop app (buzz-app) should show it as read too, and the reverse. buzz-app no longer saves a "read" mark for each ordinary message. It saves one "caught up to here" mark per channel and per thread, and keeps per-message marks only for mentions, DMs, broadcasts and replies. Mobile already reads those marks. This PR makes mobile write them, so reading on the phone clears unread on other devices and saved read state stays small.

What changes on the phone:

  • Reaching the bottom of a channel, and staying there for 300 ms, marks it caught up. Reaching the end of a thread does the same for that thread.
  • A message you see fully on screen, away from the bottom, is marked read on its own.
  • Mentions, DMs, broadcasts and thread replies you see keep their own read marks, as in buzz-app.
  • Automatic reading never marks the whole channel read. Only the explicit Mark read action does that. DMs and forums are the exception: opening one marks it all read, as mobile does today.
  • Reaching the bottom of a channel you marked unread ends that manual unread. Unseen mentions and replies stay unread.
  • Activity rows clear when you have read every message in the row, not just the newest.

Related issue

None found. buzz-app makes the same DM and manual-unread changes in block/buzz-app#616.

Testing

  • Unit tests cover the new write rules, Activity rows and the size limit on saved read state.
  • Widget tests cover reading on the channel page and the thread page, opening a DM, and reaching the bottom of a channel you marked unread.
  • No visual changes.

Details

Read marks. "Caught up" is saved as activity:<channel> for a channel and thread-activity:<root> for a thread. A caught-up mark reads only ordinary top-level messages (channel) or that thread's replies (thread). A per-message mark is msg:<id>.

Caught-up time. The mark is set to the time of the bottom row, and never later than the current time. The time of a newer reply is not used. So a top-level message that arrives late, with an earlier timestamp, stays unread. A clock that runs fast, on this phone or the sender's, cannot mark messages read before they arrive.

Nested threads. A thread opened on a nested reply shows only one branch. Reaching its end writes per-message marks only, so unread messages in other branches stay unread.

Reading delay. The 300 ms reading delay now restarts only when messages arrive or leave. Before, any page rebuild restarted it, for example a typing indicator.

Activity rows. A row that groups several events is done only when each event is read. Each event is checked against the channel mark, its own msg: mark, and its thread marks if it is a reply. Opening a row goes to the oldest unread event.

Publishing. Mobile no longer republishes per-message marks it got from other devices. buzz-app removes those once a caught-up mark covers them. Broad marks (channel, thread, caught-up) are still republished, so they stay available after the fetch window. The published blob is capped at buzz-app's 40 KiB limit. Channel marks are kept first, then thread marks, then caught-up marks, then the newest per-message marks. Before, a blob over the encryption limit (about 64 KiB) turned off sync for the rest of the session.

DMs and manual unread. Opening a DM reads it through the newest loaded message, replies included, as mobile does today. Reaching the bottom of a channel ends a manual unread. Manual unread on mobile is local to this session, so ending it does not change what other devices see.

Compatibility. Older desktop builds read only channel and per-message marks. They may show extra unread for channels read on the phone.

Follow-ups, not in this PR

  • Unread catch-up fetches start after the channel mark and are capped at 1,000 events. In a busy channel, an old unread mention can drop out of the badge. The fetch should also start from the caught-up mark, or load more pages.
  • Each mark triggers its own save and rebuild. They could be batched.
  • Broadcast replies are counted slightly differently from buzz-app.
  • buzz-app can also move its caught-up mark past the current time.

@loganj
loganj force-pushed the larry/mobile-activity-mark-writes branch from 96c785f to c8f9587 Compare October 5, 2026 18:46
@loganj
loganj force-pushed the larry/mobile-activity-read-marks branch from d803d95 to 1e1efab Compare October 5, 2026 19:04
@loganj
loganj force-pushed the larry/mobile-activity-mark-writes branch from c8f9587 to 62a9feb Compare October 5, 2026 19:06
@loganj
loganj force-pushed the larry/mobile-activity-read-marks branch from 1e1efab to 69da0b7 Compare October 5, 2026 19:38
@loganj
loganj force-pushed the larry/mobile-activity-mark-writes branch from 62a9feb to 5edb087 Compare October 5, 2026 19:41
@loganj
loganj changed the base branch from larry/mobile-activity-read-marks to main October 5, 2026 22:52
Larry added 4 commits October 5, 2026 18:57
…web app

Reading the newest message in a channel or thread now writes
activity:<channel> or thread-activity:<root>. Other fully visible
messages get msg:<id> marks after a 300ms dwell. Opening a channel no
longer marks the whole channel read (forums still do), and opening a
thread no longer marks every reply.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
… Activity rows

- Keep msg: marks for visible replies; thread catch-up reads a reply only
  while its root is loaded.
- Cut activity:/thread-activity: at the bottom row, never past now.
- A nested thread head writes msg: marks only.
- Key the reading dwell on message content so rebuilds do not restart it.
- Check each grouped Activity event against its own marks.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Do not republish message marks merged from other devices; the web app
prunes them once a catch-up mark reads the message. Cap the slot at the
web app's 40 KiB budget, keeping broad marks first, so a large slot no
longer stops sync.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Opening a DM writes its channel mark through the newest loaded message,
replies included, as mobile did before. Reading to the bottom of a
channel the reader marked unread clears that manual unread. It does not
move the channel mark, so unseen mentions, replies and messages marked
unread stay unread.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj
loganj force-pushed the larry/mobile-activity-mark-writes branch from a2b477c to 47fc23f Compare October 5, 2026 23:04
@loganj
loganj marked this pull request as ready for review October 5, 2026 23:24
@loganj
loganj requested a review from a team as a code owner October 5, 2026 23:24

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Reviewed base 932228936464a0bdd44eae0f5e95ca5bbcd819b9 through exact head 47fc23f8e6a639c9065bc1632dac18f6e4ba4357. Blocking findings remain; I do not recommend merging this head. Per this repository's agent-review policy, I am posting a comment rather than selecting GitHub's Request Changes state.

Findings

1. Automatic selective reads make local read-state persistence unbounded

The new production paths create a durable msg: context for each newly visible message (mobile/lib/features/channels/reading_marks.dart:68-85, consumed by channel_detail_page/message_list.dart:577-593 and thread_detail_page.dart:720-734). The 40 KiB cap is applied only while constructing a relay payload in _currentContexts() (mobile/lib/shared/read_state/read_state_manager.dart:502-510). Every local mutation still serializes the complete _effectiveState, _publishableContextIds, and _contextSourceCreatedAt collections into three SharedPreferences values without horizon or count pruning (read_state_manager.dart:172-200,526-532; read_state_storage.dart:82-101). The new 1,400-mark regression explicitly preserves marks omitted from publication (mobile/test/features/channels/read_state/read_state_manager_test.dart:129-151).

That turns normal continued reading into unbounded local JSON growth and increasingly expensive serialization on the UI isolate, contrary to the bounded storage/background-work requirement in VISION_MOBILE.md:28-29. The desktop reference already prunes msg:/thread: entries by horizon and count and filters both metadata structures to the retained keys (desktop/src/features/channels/readState/readStateStorage.ts:106-176).

Author action: apply synchronized horizon/count pruning to locally persisted prunable msg:/thread: contexts, filtering publishable IDs and source timestamps to the same retained key set while preserving broad contexts. Add high-volume plus restart/hydration coverage proving all three persisted structures remain bounded and aligned.

Verification owner: author and mobile CI; reviewer to re-check retention semantics.

2. A read mark can be stranded when its debounce fires during an in-flight publish

_publish() returns immediately when _isPublishing is true (mobile/lib/shared/read_state/read_state_manager.dart:385-393). The active operation snapshots contexts once (:399-407), and finalization only clears the flag/completer (:451-457); it does not schedule a trailing pass.

A concrete schedule is: publish A takes its snapshot; mark B arrives and schedules its five-second timer; B's timer fires while A's relay submit remains pending, so B's _publish() call returns; A succeeds and records only its prior snapshot. B remains local but has no relay retry until an unrelated later mutation, reinitialization, or lifecycle flush. The new repeated automatic visible-row writes make this race an ordinary production path rather than an exceptional manual action.

Author action: make publish coalescing dirty-aware and guarantee a trailing publish whenever state changes after the active snapshot, including failure paths (or await/coalesce callers with the same guarantee). Add a controlled relay regression that parks submit A, marks B after A snapshots, lets B's debounce fire, releases A, and proves a second accepted event contains B.

Verification owner: author and mobile CI; reviewer to re-check async lifecycle.

3. The 40 KiB retention path can replace a slot with incomplete ov_* durability state

Incoming contexts are merged wholesale (mobile/lib/shared/read_state/read_state_manager.dart:245-257,318-333), and republishesMergedContext() excludes only msg: (mobile/lib/shared/read_state/read_state_format.dart:37-43). Consequently mobile republishes merged ov_s: / ov_c: / ov_b: keys despite not implementing the override protocol's full-state loading and group semantics.

The new retention function merely prioritizes those keys, then truncates entry-by-entry at 40 KiB (read_state_format.dart:45-86). A sufficiently large override set is therefore omitted, and a group crossing the boundary can be split, while the truncated replacement event is still submitted. Before this PR, an oversized encryption stopped sync; this change converts that visible stop into partial override replication.

NIP-RS requires complete-group validation before merge (docs/nips/NIP-RS.md:114), forbids budget eviction of override entries (:674-678), requires frontier plus override siblings to travel together (:656-662), and requires leaving the previous primary in place rather than publishing an omitted merged set (:691-693). Violating those constraints can resurrect cleared manual-unread state or corrupt convergence.

Author action: preserve override groups atomically and never evict any ov_* state; if all merged override state cannot fit, fail closed without replacing the prior slot. If mobile is instead intentionally made override-unaware, the migration must also prove it does not abandon override state already carried in its existing coordinate. Add over-budget and boundary-splitting regressions proving no partial or omitted override-bearing replacement is submitted.

Verification owner: author and mobile CI; reviewer to re-check against NIP-RS.

Additional evidence and residual gaps

  • Exact-head CI was green for Clients / Mobile, Mobile, Clients / Results, Mobile Swift Domain / Mobile Swift, Mobile Swift Domain / Results, DCO, Semgrep OSS, and zizmor when checked. CI does not exercise the race schedule or the local-storage lifetime above.
  • Static/widget-test inspection found no separate defect in bottom dwell, selective visible marks, nested-thread isolation, DM/forum open-read behavior, manual-unread preservation, or Activity grouping/deep-linking.
  • Local Flutter execution was blocked before tests by this review machine's objective_c native-asset/Xcode setup. This is a reviewer tooling confidence gap, not author rework.
  • No physical-device iOS/Android observation was performed for dwell timing, tall-message bottom detection, rebuild/app-lifecycle races, or accessibility behavior. Verification owner: mobile release/QA.
  • Existing protocol debt, not attributed to this PR: isValidReadStateDTag accepts arbitrary non-empty ASCII up to 64 characters (read_state_format.dart:138-154) rather than exactly 32 lowercase hexadecimal characters as required by docs/nips/NIP-RS.md:55.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Reviewed exact head 47fc23f8e6a639c9065bc1632dac18f6e4ba4357 against base 932228936464a0bdd44eae0f5e95ca5bbcd819b9.

The mobile/product behavior is well-covered and the exact-head Mobile, Clients/Mobile, Mobile Swift, DCO, Semgrep, and zizmor checks are green. However, the new automatic read producer and publication cap leave four author-actionable correctness defects:

  1. Published-size accounting ignores JSON escaping (mobile/lib/shared/read_state/read_state_format.dart:68-84). The code charges raw UTF-8 key bytes, while accepted keys can contain characters such as " and \\ that jsonEncode expands (:201-211). A reproduction with legal 245-byte msg: keys retained 162 entries at an accounted 40,861 bytes, but encoded to 80,064 bytes—14,528 bytes beyond the ~64 KiB NIP-44 limit. This recreates the session-disabling publish failure the cap is meant to prevent.

    Author action: budget encoded entry/final-payload bytes, and add an escaping-heavy regression asserting the final plaintext stays within the cap.

  2. Automatic selective reads make local read-state persistence unbounded. The new channel/thread loops durably emit msg: contexts for visible messages (mobile/lib/features/channels/reading_marks.dart:68-85; callers in channel_detail_page/message_list.dart:577-593 and thread_detail_page.dart:720-734). Only publication is capped (read_state_manager.dart:506); local persistence still serializes all effective contexts, publishable IDs, and source timestamps (read_state_manager.dart:526-532; read_state_storage.dart:82-101). Continued reading therefore grows local JSON and every-write serialization indefinitely.

    Author action: prune prunable msg:/thread: contexts by a bounded horizon/count before persistence, keeping contexts, publishable IDs, and source timestamps aligned while preserving broad contexts. Add high-volume restart/hydration coverage for all three stored structures.

  3. A mark can be stranded when its debounce fires during an in-flight publish. _publish() returns immediately while _isPublishing (read_state_manager.dart:385-393), after the active operation already snapshotted its contexts (:399-407), and completion does not schedule a trailing pass (:451-457). If mark B's timer fires while submit A is pending, A records only its old snapshot and B has no relay retry until an unrelated later mutation/lifecycle flush.

    Author action: make coalescing dirty-aware and guarantee a trailing publish when state changes after the active snapshot, including failure paths. Add a controlled relay test that parks submit A, marks B, lets B's debounce fire, releases A, and proves a second accepted event contains B.

  4. The 40 KiB retention path can publish an incomplete reserved override replica. Incoming contexts are merged wholesale (read_state_manager.dart:245-257,318-333), and republishesMergedContext excludes only msg: (read_state_format.dart:37-43), so unsupported ov_* state becomes publishable. Entry-by-entry truncation (:45-86) can split or omit an override group, converting the prior visible encryption failure into silent partial replication that can corrupt manual-unread convergence or resurrect cleared state.

    Author action: either fully support override replication with complete-group validation, atomic retention, and fail-closed publication when all override state cannot fit, or exclude unsupported reserved override keys from mobile adoption/republishing. Add over-budget and group-boundary coverage proving no partial override-bearing event is submitted.

Confidence gaps (not author defects): local focused Flutter execution was blocked before tests by objective_c native-asset/Xcode discovery (Bad state: No element), and no physical-device dwell/scroll/lifecycle observation was performed. CI/mobile release verification owns those gaps; they are not additional rework requests.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Three P2 correctness blockers are detailed inline. Please address them with regression coverage before merging.

Reviewed head 47fc23f8e6a639c9065bc1632dac18f6e4ba4357 against base 932228936464a0bdd44eae0f5e95ca5bbcd819b9. Existing Mobile CI is green. Validation combines source tracing, an extracted-source Dart retention probe, and independent reviewer widget probes; no live cross-device workflow or broad local suite was run. The Android occlusion finding is source-traced, not device-reproduced.

Optional product follow-up: under the explicitly chosen full-row visibility policy, a mention taller than the phone viewport cannot become read by scrolling through it and requires explicit Mark read. Consider an oversized-row reading policy separately; this is not a merge blocker.

Comment thread mobile/lib/shared/read_state/read_state_format.dart Outdated
Comment thread mobile/lib/features/channels/thread_detail_page.dart Outdated
Comment thread mobile/lib/features/channels/channel_detail_page/message_list.dart Outdated
…d heads

- Count JSON-encoded bytes in the 40 KiB slot budget, so escaped keys
  cannot push the encrypted payload past the NIP-44 limit.
- Reserve a quarter of the slot for the most recently written marks, as
  buzz-app does, so old channel and thread marks cannot starve new reads.
- Save at most 1,000 msg:/thread: marks within the 7-day horizon, like
  desktop's pruneStaleContexts. Channel and catch-up marks are kept, and
  all three saved structures hold the same keys.
- Publish again when state changes during an in-flight publish,
  including after a failed publish.
- Stop taking ov_*/esc: override keys from other slots. Carry the ones
  already in this device's own slot whole, ahead of the budget; if they
  do not fit, leave the slot as it is.
- Read a fully visible, non-deleted thread head, including in head-only
  threads and threads opened directly.
- Use the full covered height (composer plus Android keyboard) as the
  reading edge in channels and threads, and restart the dwell when it
  changes.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj

loganj commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. The fixes are in dd7f76d.

Encoded size. Each entry's cost is the jsonEncoded key bytes plus the colon, the value and a comma. New test: 400 msg: keys of " and \ characters. The encoded payload stays within 40 KiB. Under the old raw-byte count it went past the limit.

Local persistence. pruneStaleContexts matches desktop's version. It drops msg: and thread: marks older than the 7-day horizon, then keeps the newest 1,000. It never prunes channel or catch-up marks. Every save writes the pruned contexts, and the publishable IDs and source times are filtered to the same keys. Memory keeps every mark for the current session, so a message read from old history stays read until restart. New tests: a unit test for prune order, and a high-volume restart test. The restart test checks that all three saved structures are bounded and hold the same keys, and that hydration restores the bounded set. The existing 1,400-mark test still checks that marks left out of the slot stay in memory.

Trailing publish. If _publish() is called while a publish is running, it sets a dirty flag. The running publish then loops until nothing is left, on both success and failure. New fake-async test: submit A is parked, B is marked, and B's debounce fires. When A is released, a second event contains B. The test runs once with A succeeding and once with A failing. It fails without the loop.

Override keys. Mobile is now override-unaware, and it does not abandon override state:

  • It takes no ov_*/esc: keys from other slots.
  • It carries the override keys already in its own slot. These come from its own blob, merged by max(), and from earlier saved state, which an old build published there. It carries them unchanged and keeps them whole. Retention reserves their bytes before any frontier mark, so budget eviction never splits or drops them.
  • If the carried keys alone do not fit, nothing is published, and the previous slot stays as it is.

New tests:

  • Unit: carried groups are kept whole ahead of 1,400 marks, and an oversized carried set returns null.
  • Manager: its own group is republished, and another slot's ov_c: and esc: keys are not.
  • After a restart with no relay, the group is still carried.
  • With an oversized own-slot override set, no event is submitted.

Local gates on dd7f76d: dart format clean, flutter analyze clean, and full flutter test passed (3078 tests).

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Re-reviewed base 932228936464a0bdd44eae0f5e95ca5bbcd819b9 through exact head dd7f76dc52d46c777addf1c4e4dbfd5c5ca6e7e5. The prior encoded-budget, bounded-persistence, and trailing-publish defects are fixed. Two author-actionable defects remain, so I do not recommend merging this head.

Findings

P1 — a covered or backgrounded DM route can consume a newly loaded reply without visibility

ChannelDetailPage now derives a DM's automatic open-read timestamp from every loaded event, including replies (mobile/lib/features/channels/channel_detail_page.dart:209-224), and advances the whole-channel mark whenever that timestamp changes (:544-556). That effect has no current-route or foreground-lifecycle fence. The visible channel/thread dwell paths do have those fences (channel_detail_page/message_list.dart:605-619; thread_detail_page.dart:745-760).

Concrete failure: open a DM, cover it with a thread or another pushed route (or background the app), then receive/load a newer DM reply. The covered page advances the DM channel marker despite the reply never being visible. Activity subsequently treats the reply as read through that channel marker (mobile/lib/features/activity/inbox_read_state.dart:13-29), removing unread/Activity state. This PR makes the failure cover replies by expanding the open-read calculation beyond top-level events.

The foreground-open test (mobile/test/features/channels/channel_detail_page_test.dart:2166-2221) and covered non-DM thread test (:2265-2333) do not exercise this DM lifecycle case.

Author action: gate DM automatic open/read updates on the route being current and the app being foregrounded, while retaining immediate foreground DM-open behavior. Add deterministic widget regressions for (1) a DM covered by a pushed route plus a newly loaded reply and (2) a paused/hidden app plus a newly loaded reply; assert no channel advance until current/resumed, then assert the intended read occurs.

Verification owner: author and Mobile CI; reviewer to re-check the production lifecycle seam.

P2 — carried override siblings can be published without their required frontier

The new carrier reserves only keys classified as ov_* or esc: (mobile/lib/shared/read_state/read_state_format.dart:76-82; passed from read_state_manager.dart:559-571). An ordinary matching frontier remains in the independently evictable marks map. retainReadStateContexts() copies carried entries first and then trims ordinary entries by scope/recency (read_state_format.dart:119-185). Under budget pressure, all three ov_*:<ctx> siblings can survive while <ctx> is evicted.

That violates NIP-RS's requirement that a context's frontier and all override siblings travel in the same event (docs/nips/NIP-RS.md:656-662). An executable model of this exact algorithm using a legal 240-byte context ID and 2,000 newer broad marks produced a 40,923-byte replacement containing all override siblings but no matching frontier. The receive path also carries own-slot reserved entries independently without complete-group validation (read_state_manager.dart:285-297), contrary to the complete logical-group validation rule (docs/nips/NIP-RS.md:114). Existing tests assert preservation of override keys but not the matching frontier (mobile/test/features/channels/read_state/read_state_format_test.dart:206-233; read_state_manager_test.dart:299-383).

Author action: model carried state as validated logical groups that include the unescaped matching frontier; reserve and retain each complete frontier-plus-override group atomically. Reject malformed/partial own-slot groups. If all complete carried groups cannot fit, leave the prior slot unchanged. Add regressions for matching-frontier eviction pressure and malformed/partial own-slot groups.

Verification owner: author and Mobile CI; reviewer to mutation-check grouping and frontier retention.

Prior findings now cleared

  • Encoded JSON byte accounting is fixed and has escaping-heavy coverage (read_state_format.dart:129-149; read_state_format_test.dart:157-171).
  • Local persistence now prunes stale/excess msg:/thread: entries to 1,000 and aligns context, publishable-ID, and source-time storage, with restart/hydration coverage (read_state_format.dart:37-74; read_state_manager.dart:574-619; read_state_manager_test.dart:249-297).
  • In-flight publish coalescing now guarantees a trailing pass after ordinary success or failure, with a parked-submit production-seam regression (read_state_manager.dart:415-445; read_state_manager_test.dart:204-247).
  • Product re-validation cleared visible channel/thread marking, nested-thread isolation, selective manual-unread behavior, Activity catch-up isolation, stable dwell identity, and Android keyboard occlusion at this head.

Validation and confidence gaps

  • dart format --output=none --set-exit-if-changed .: pass, 662 files unchanged.
  • flutter analyze: pass, no issues.
  • Mobile gateway recipe tests: pass at this exact head. git diff --check 9322289...dd7f76d: pass.
  • Local focused/full Flutter tests were blocked before execution because this review machine has not accepted the Xcode 27 license (objective_c native-asset discovery / xcrun exit 69). This is reviewer tooling, not author rework.
  • Native iOS/Android lifecycle and accessibility observation was not performed. Verification owner: mobile release/QA.
  • At publication, Clients / Mobile, Mobile Swift Domain / Mobile Swift, and Codex Security Review were still running; DCO, Semgrep OSS, zizmor, and completed exact-head gates were green. Those pending checks do not remove either source-proven defect.

A DM behind a sheet or with the app inactive no longer reads a reply
that loads meanwhile; it reads through it when shown again.

Carried override keys now travel as complete NIP-RS groups with their
frontier: retention reserves each group and its frontier together,
incomplete own-slot groups are rejected whole, and local saving never
prunes a group's frontier.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj

loganj commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the re-review. Both findings are fixed in dc56b6b.

P1: covered or inactive DM. The open-read effect now runs only while the route is current and the app is in use. It uses the same isAppInUse and ModalRoute.isCurrent fence as the channel and thread dwells. The fence is part of the effect's keys, so the DM reads through the newest message as soon as it is shown again. Opening a DM in the foreground still reads it at once.

Two notes on the test shape, because the obvious tests pass without the fix:

  • Under a full-screen pushed route, Riverpod pauses the covered page's watch. The page rebuilds but still sees the old message list, so that case was already protected by accident. A sheet or dialog keeps the page on screen and its watch live. That is the real exposure, and the test uses a modal bottom sheet.
  • A paused app draws no frames, so nothing rebuilds. inactive (the app switcher, Control Center) still draws frames, and the test uses it.

New tests: a DM behind a sheet … and a DM behind the app switcher reads a new reply only when shown again. A reply loads while the DM is covered. The channel mark stays at the old message. When the DM is shown again, the mark moves through the reply. Both fail without the fence (Actual 1300, Expected 1100).

P2: override groups. Carried state is now complete NIP-RS groups:

  • completeOverrideGroups accepts exactly ov_s:/ov_c:/ov_b: for one context, or ov_c: alone. Any other shape is rejected whole. esc: frontiers are carried as single keys.
  • retainReadStateContexts reserves each accepted group together with its frontier (overrideFrontierKey, which escapes ov_/esc: IDs). If the groups and frontiers do not fit, it returns null, and the slot stays as it is.
  • On receive and on hydrate, the manager validates its own slot's groups before merging. A group's frontier is passed to retention even when it is a merged msg: mark that would not be republished on its own.
  • Local saving never prunes a group's frontier. Before this, a 7-day-old msg: frontier with an override was lost on restart.

New tests:

  • Your pressure case: a 240-byte context, plus an escaped ov_x tombstone with its esc:ov_x frontier, against 2,000 newer channel marks. The frontier and all siblings are kept within 40 KiB. On the old code, the frontier was null.
  • Incomplete groups: partial, and ov_c: with ov_b:, are left out. Live and tombstone groups are kept, and the ordinary frontiers stay.
  • Manager: an own-slot partial ov_s: is not published. The group's frontier is published with it, also after a restart without the relay. An old msg: group frontier survives the local save and the restart. Each of these fails when its fix is removed.

Local gates on dc56b6b: dart format clean, flutter analyze clean, just mobile-test passed (3,082 + 3 tests).

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Re-reviewed base 932228936464a0bdd44eae0f5e95ca5bbcd819b9 through exact head dc56b6b6b57454255641e7871de7a0cd95a8a66c. The DM lifecycle defect and the prior frontier-retention defect are fixed. One author-actionable NIP-RS validation defect remains, so I do not recommend merging this head.

Finding — malformed or frontierless own-slot overrides can still be republished

Override grouping currently occurs after generic entry sanitization. decodeReadStateBlob() passes the raw map through sanitizeReadStateContexts() (mobile/lib/shared/read_state/read_state_format.dart:348-356), which discards invalid entries independently (:359-369). _carryOwnOverrides() later applies completeOverrideGroups() only to that reduced Map<String, int> (mobile/lib/shared/read_state/read_state_manager.dart:285-296).

For this valid JSON wire shape:

ctx = 100
ov_c:ctx = 7
ov_b:ctx = "invalid"

sanitization drops only ov_b:ctx, leaving {ctx: 100, ov_c:ctx: 7}. completeOverrideGroups() then accepts the surviving ov_c: as a tombstone (read_state_format.dart:106-123). NIP-RS explicitly requires collecting and validating all siblings before per-entry discard: an invalid sibling rejects the whole group (docs/nips/NIP-RS.md:101-115). An exact control-flow probe at this head reproduced the reduction and acceptance.

The same logical validation boundary permits all three counter siblings without a matching frontier. completeOverrideGroups() accepts the counters alone, while retention and _currentContexts() reserve the frontier only if present (read_state_format.dart:182-189; read_state_manager.dart:566-576). Mobile can therefore republish a frontierless override group, violating the mandatory frontier-plus-siblings co-location rule (docs/nips/NIP-RS.md:654-662). Current tests cover partial valid-valued siblings but not an invalid sibling discarded before grouping or a complete counter set with no frontier (mobile/test/features/channels/read_state/read_state_format_test.dart:273-300; read_state_manager_test.dart:299-401).

Author action: validate override groups against the raw decoded context object before generic per-entry sanitization, rejecting the entire group for any malformed or extra sibling. Model a carried logical group as counters plus its required escaped/unescaped matching frontier, and reject groups whose frontier is absent. Add production-seam manager regressions for (1) valid ov_c plus invalid ov_b and (2) complete counters without a frontier, proving neither malformed group is submitted while unrelated frontier state remains intact. Mutation-check both per-entry-first sanitization and the missing-frontier guard.

Verification owner: author and exact-head Mobile CI; reviewer to re-check protocol ordering, escaped identity, and mutations.

Cleared at this head

  • DM open-read now requires a current route and resumed app state, with production widget/provider regressions for a modal sheet and app-inactive transition (mobile/lib/features/channels/channel_detail_page.dart:544-562; mobile/test/features/channels/channel_detail_page_test.dart:2223-2315). Immediate foreground reads resume correctly.
  • Accepted override groups and ordinary/escaped frontiers are now retained atomically under pressure, oversized reserved state fails closed, and carried frontiers survive pruning/hydration (read_state_format.dart:173-245; read_state_manager.dart:579-629).
  • Prior encoded-size, bounded-persistence, trailing-publication, visible-read, manual-unread, nested-thread, Activity, and dwell findings remain cleared by source/test trace.

Validation and confidence gaps

  • git diff --check passed. Hermit Dart formatting checked 661 files with zero changes. Source/worktrees were clean at exact head.
  • Author reports exact-head just mobile-test passing (3,082 + 3 tests). Reviewer execution reached all five gateway-recipe checks, then Flutter was blocked by this host's unaccepted Xcode 27 license (xcrun exit 69 / objective_c native-asset discovery). This is reviewer tooling, not extra author work.
  • DCO, Semgrep OSS, zizmor, changed-path, and dead-token checks were green when polled. Clients / Mobile, Mobile Swift Domain / Mobile Swift, and Codex Security Review were still pending.
  • No native-device lifecycle/accessibility run was performed. Verification owner: mobile release/QA.

…rontier

Decoding now checks each override group on its raw values before the
per-entry rule, so an invalid sibling rejects the whole group instead of
leaving a false tombstone. A carried group must also travel with its
frontier; a group without one is rejected.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj

loganj commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks. Both parts are fixed in 188f372.

Groups before entries. sanitizeReadStateContexts now groups ov_s:/ov_c:/ov_b: entries by context on the raw decoded values first. A group is rejected whole if its shape is illegal or any sibling's key or value is invalid. Other ov_ keys are reserved and dropped. Only after that does the per-entry rule run on the remaining keys. Your example {ctx: 100, ov_c:ctx: 7, ov_b:ctx: "invalid"} now decodes to {ctx: 100}.

Frontier required. completeOverrideGroups accepts a group only when its frontier (escaped if needed) is in the same map or in the given frontiers:

  • Receive checks against the own blob.
  • Hydrate checks against the stored contexts.
  • Retention checks against the marks being published.

A group without a frontier is not carried, so it is never submitted.

New manager test, through the real decode, carry and publish path: rejects own-slot override groups that are malformed or have no frontier. Its own slot holds:

  • a valid ov_c: with a string ov_b:
  • complete counters with no frontier
  • one valid group with its frontier

The published slot is exactly the frontiers (including bad-sibling, whose group was rejected), the new mark, and the valid group. The format test for incomplete groups also adds a frontierless complete group.

Mutations:

  • Per-entry checks first (rejected groups no longer skipped): the manager test fails.
  • No frontier guard: the manager test and the format test fail.

Three older fixtures had groups with no frontier. I added the frontiers, so they still test what they were meant to test.

Local gates on 188f372: just mobile-check clean, just mobile-test passed (3,083 + 3 tests).

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: APPROVE

Reviewed base 932228936464a0bdd44eae0f5e95ca5bbcd819b9 through exact head 188f3722b02ca3d7d534ae2d22000aadc786744b. The prior raw-group/frontier blocker is fixed, and no unresolved author-actionable defect remains.

Closure of the prior blocker

  • Override counters are now collected and validated as logical groups on the raw decoded object before generic per-entry sanitization (mobile/lib/shared/read_state/read_state_format.dart:382-415). An invalid sibling therefore rejects the whole group instead of collapsing a live group into an accepted tombstone.
  • Carrying a group now requires its matching frontier, with reserved raw IDs mapped to the escaped wire frontier through overrideFrontierKey (read_state_format.dart:84-153).
  • Retention reserves accepted group-plus-frontier units atomically and returns null when the reserved state cannot fit (read_state_format.dart:181-268); the manager treats that as “publish nothing,” leaving the prior slot in place (read_state_manager.dart:448-477).
  • Persistence and hydration preserve the same complete unit across restart (read_state_manager.dart:557-629).
  • The manager regression exercises the production path from encrypted raw wire data through decode, own-slot carry, and publication. It distinguishes an invalid sibling, a complete group without a frontier, and a valid complete group (mobile/test/features/channels/read_state/read_state_manager_test.dart:403-467). The retention tests cover frontier co-location under byte pressure and fail-closed oversize behavior (read_state_format_test.dart:203-325). Restoring per-entry-first sanitization or removing the frontier guard changes the exact published map and fails these tests.

Adjacent behavior rechecked

The broader mobile read-state behavior remains consistent with the PR contract and mobile vision:

  • Channel/thread automatic reads retain their 300 ms current-route/resumed-app fence, bounded bottom timestamp, selective fully-visible row marking, nested-thread isolation, and manual-unread behavior (mobile/lib/features/channels/reading_marks.dart:12-169; channel_detail_page/message_list.dart:550-618; thread_detail_page.dart:690-758).
  • DM open-read remains current-route and resumed-app gated, so replies loaded under a sheet or while inactive are read only when the DM is shown again (channel_detail_page.dart:209-224,507-562).
  • Activity read projection still evaluates every grouped event against channel, message, and thread frontiers (mobile/lib/features/activity/inbox_read_state.dart:5-59).
  • Production-page tests cover channel catch-up/manual unread, DM open under sheet/lifecycle transitions, historical rows, direct and empty threads, and keyboard occlusion (mobile/test/features/channels/channel_detail_page_test.dart:2110-2729).

Validation

At exact clean head 188f3722b02ca3d7d534ae2d22000aadc786744b:

  • git diff --check dc56b6b6b57454255641e7871de7a0cd95a8a66c..HEAD: pass.
  • just mobile-check: pass — Dart format checked 662 files with zero changes; Flutter analyze reported no issues.
  • just mobile-test: gateway-recipe checks passed, but Flutter test execution was blocked before tests by this review host's objective_c native-asset hook because xcrun cannot return the Apple SDK path. This is reviewer tooling, not author rework.
  • Exact-head CI: Clients / Mobile, Mobile, Clients / Results, Mobile Swift Domain / Mobile Swift, Mobile Swift Domain / Results, DCO, Semgrep OSS, zizmor, changed-path, and dead-token checks passed. Codex Security Review remained pending at the final poll.
  • Authenticated reviewer jedwards27 differs from PR author loganj; normal approval semantics apply.

Author action: none.

Verification owner: the Codex Security Review gate owns its pending result; reviewer/tooling owns the local Xcode/native-asset gap; mobile release/dogfood owns optional native iOS/Android geometry and lifecycle observation.

Residual risk: focused/full Flutter tests and physical-device lifecycle/accessibility behavior were not independently observed on this host. Exact-head mobile CI and production-seam widget coverage are green; these remaining confidence gaps do not establish an author-actionable defect.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Reviewed base 932228936464a0bdd44eae0f5e95ca5bbcd819b9 through exact head 188f3722b02ca3d7d534ae2d22000aadc786744b. APPROVE — no author-actionable defects remain.

The final delta closes the remaining NIP-RS validation blocker: raw override siblings are grouped before sanitization, malformed groups are rejected wholesale, a matching frontier (including escaped identity) is mandatory, complete group-plus-frontier units are retained atomically and fail closed when they cannot fit, and persistence/hydration preserve the same unit across restart. Production-seam and pressure tests bind malformed-sibling, frontierless-group, valid-group, and retention behavior (mobile/lib/shared/read_state/read_state_format.dart:84-153,181-268,382-415; mobile/lib/shared/read_state/read_state_manager.dart:448-477,557-629; mobile/test/features/channels/read_state/read_state_manager_test.dart:403-467; mobile/test/features/channels/read_state/read_state_format_test.dart:203-325).

The product/mobile lane also found no remaining defect: current-route and resumed-lifecycle fencing for DMs, immediate foreground open-read, 300 ms visible/catch-up semantics, manual-unread preservation, Activity projection, and production widget seams are sound. Prior findings covering encoded-size accounting, bounded/aligned persistence, trailing publication, override integrity, and DM lifecycle are cleared at this head.

Validation at the reviewed head:

  • Exact-head Clients / Mobile, Clients / Results, Mobile, Mobile Swift Domain / Mobile Swift, and Mobile Swift Domain / Results passed, as did DCO, Semgrep OSS, zizmor, changed-path, and dead-token checks.
  • Required checks were terminal and non-failing immediately before submission. Run Codex Security Review remained in progress but is not a required gate.
  • Reviewer worktrees were clean; just mobile-check, formatting/analyze, and git diff --check passed.

Confidence gaps, not author rework: focused local Flutter execution is blocked by this host’s unaccepted Xcode license; exact-head Mobile CI supplies the package gate. No physical-device iOS/Android lifecycle/accessibility observation was performed; mobile release/QA owns that verification. The uncapped future-dated DM timestamp remains cross-client hardening debt rather than a defect introduced by this PR.

This branch was successfully deployed

No deployments
codex-review — 188f3722 Deployed Oct 6, 2026 by loganj via Run Codex Security Review #6980
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.

4 participants