Skip to content

feat: hide stale DM threads from left rail (#261)#265

Merged
sanity merged 7 commits into
mainfrom
feat/hide-dm-threads-261
May 17, 2026
Merged

feat: hide stale DM threads from left rail (#261)#265
sanity merged 7 commits into
mainfrom
feat/hide-dm-threads-261

Conversation

@sanity
Copy link
Copy Markdown
Contributor

@sanity sanity commented May 17, 2026

Problem

After #244 / #258, every DM thread the local user is party to shows in the
left-rail "Direct Messages" section forever. A one-off back-and-forth from
six months ago clutters the rail with no way to dismiss it. The issue
explicitly differentiates this from "Purge thread" / riverctl dm purge,
which is destructive across the network — what's needed is a local-only
view filter.

Approach

  • Storage piggybacks OutboundDmStore (feat: persist outbound DM plaintext in the chat delegate #256's delegate blob). A new
    hidden_threads: Vec<HiddenDmThreadEntry> field is added to that struct
    with #[serde(default)], so pre-feat: hide stale DM threads from the left rail (local-only) #261 wire bytes still decode into an
    empty list (pinned by outbound_dm_store_decodes_legacy_{json,cbor}_…
    tests). Vec-of-struct rather than HashMap per the "non-string map
    keys in JSON-serialized API types" bug-prevention pattern.

  • Filter is the pure helper
    river_core::chat_delegate::is_thread_hidden(hidden, room, peer, max_ts)
    using strict <= so the message whose timestamp seeded hidden_at_ts
    doesn't itself revive the thread, but any strictly newer message does.
    The UI wraps it in is_thread_hidden_for for the in-memory
    HashMap<(VerifyingKey, MemberId), HiddenDmThreadEntry>. The rail's
    build_view runs the filter alongside the existing per-peer
    accumulation.

  • Outbound send to a hidden peer revives the thread automatically:
    the new message's unix_now() timestamp is strictly later than the
    recorded hidden_at_ts, so the filter returns false on the next
    render. No special-case code needed.

  • "Hide thread" button in the DM thread modal header — separate from
    the existing ✕ close button (smaller, hover-yellow vs. hover-text), with
    the tooltip the issue called for: "Hide this conversation from your
    sidebar. It will come back when either of you sends a new message."

    Clicking it captures the current latest-message timestamp (or
    unix_now() for empty threads) and closes the modal.

  • Multi-device: hides ride the same delegate blob as outbound DMs, so
    a hide on device A is visible on device B on the next reload. No second
    storage key, no second legacy-migration probe.

  • Conflict resolution on hydrate keeps the entry with the larger
    hidden_at_ts — the most recent hide intent wins. Matches the same
    rule used for OutboundDmEntry.timestamp in the existing cache.

  • unhide_dm_thread is implemented (and tested) but not exposed in
    v1 — the issue says a "Hidden conversations" submenu is optional and
    not required.

Delegate WASM migration

river-core::chat_delegate changed (new types added), so the delegate
WASM hash changed too. V19 entry added to legacy_delegates.toml and
verified via cargo make check-migration plus cargo test -p river-core --test migration_test. Room contract WASM also rebuilt as a side effect
(its river-core dep is the same), but room-contract migration is
automatic via the existing regenerate_contract_key() path.

Testing

13 unit tests in river-core::chat_delegate:

  • outbound_dm_store_with_hidden_threads_{json,cbor}_round_trips — pins
    the new wire shape
  • outbound_dm_store_decodes_legacy_{json,cbor}_without_hidden_threads
    pins backwards compat with pre-feat: hide stale DM threads from the left rail (local-only) #261 blobs
  • is_thread_hidden_returns_false_for_empty_list — fast-path
  • is_thread_hidden_equal_timestamp_stays_hidden — strict <= boundary
  • is_thread_hidden_strictly_later_message_revives — revival semantics
  • is_thread_hidden_is_scoped_per_room and …_per_peer — lookup tuple
  • is_thread_hidden_zero_max_zero_hidden_stays_hidden — empty-thread edge

Plus the existing outbound_dm_store_{json,cbor}_round_trips /
empty_outbound_dm_store_json_round_trips updated to match the new
struct shape — they still verify the original entries round-trip.

  • cargo test -p river-core --lib — 154 passed
  • cargo make test (full workspace) — all green
  • cargo make check-migration — V19 entry validated against the
    pre-change WASM hash
  • cargo check -p river-ui --target wasm32-unknown-unknown --features no-sync
  • cargo fmt, cargo clippy -p river-core clean

Manual end-to-end Playwright not run for this PR — the filter behaviour
is covered by the pure-helper unit tests, and the UI plumbing (signal
hydrate / save / button wire-up) is structurally identical to #256's
OUTBOUND_DMS path that already shipped.

Closes #261

[AI-assisted - Claude]

🤖 Generated with Claude Code

sanity and others added 3 commits May 16, 2026 21:02
Adds a purely local "Hide thread" action to the DM thread modal that
filters the thread out of the left-rail "Direct Messages" section
until either party sends a new DM (which auto-revives it with the
existing unread badge logic).

Storage piggybacks the chat-delegate `OutboundDmStore` blob (#256):
a new `hidden_threads: Vec<HiddenDmThreadEntry>` field is added with
`#[serde(default)]` so pre-#261 wire bytes still decode. Pinned by
JSON + CBOR round-trip tests and an explicit "legacy decodes without
hidden_threads" test pair.

Filter logic lives in `river_core::chat_delegate::is_thread_hidden`
(pure helper, unit-tested for hit/miss, equal-timestamp boundary,
per-room/per-peer scoping, zero-state edge case). The UI wraps it in
`is_thread_hidden_for` for the `(VerifyingKey, MemberId)` HashMap
the rail uses. Outbound sends to a hidden peer revive the thread
automatically because the new message's timestamp is strictly later
than `hidden_at_ts`.

Multi-device: hides ride the delegate blob, so a hide on device A is
visible on device B on next reload.

Delegate WASM hash changed (river-core changed); V19 migration entry
added to `legacy_delegates.toml` per the delegate-migration workflow.

Closes #261

[AI-assisted - Claude]

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Required by CI `check-wasm-sync`: the room-contract WASM was rebuilt
as a side effect of the river-core change, and riverctl embeds the
WASM to compute the contract key. Without a version bump the
published riverctl on crates.io would target a different contract
than the UI shipping the new WASM.

[AI-assisted - Claude]

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codex caught a "hide, then immediately send" deadlock: both
`hide_dm_thread`'s `hidden_at_ts` and the outbound DM's
`message.timestamp` come from `unix_now()` which has 1-second
resolution. If both calls land in the same second, the rail
filter's `last_any_ts <= hidden_at_ts` predicate evaluates true
and the thread stays hidden right after the user sent a message.

Fix: explicitly call `unhide_dm_thread` from the successful
`ApplyOutcome::Applied` arm. Idempotent — a no-op when no entry
exists for the pair, so non-hidden threads are unaffected. This
removes the dependency on second-resolution timestamp comparison
for the outbound-revives case.

Inbound revival on clock-skewed sender timestamps remains
filter-only (deferred — would require a ROOMS-subscribed effect
that diffs prior-vs-current `direct_messages.messages` for new
inbound entries, which is more invasive than this PR warrants
for the same-second corner case).

[AI-assisted - Claude]

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 17, 2026

Codex review (round 1) findings + responses

P1 (FIXED in a60814e): "Same-second or clock-skewed DMs do not revive hidden threads." Both hide_dm_thread's hidden_at_ts and the outbound DM's message.timestamp come from unix_now() (1-second resolution), so hide-then-immediately-send can leave the thread stuck-hidden. Fixed by explicitly calling unhide_dm_thread from the ApplyOutcome::Applied arm — idempotent no-op when no entry exists. Removes the dependency on timestamp comparison for the outbound-revives case.

Inbound revival on clock-skewed sender timestamps remains filter-only. Fixing that requires a ROOMS-subscribed effect diffing prior-vs-current direct_messages.messages for new inbound entries, which is more invasive than this PR warrants for the same-second corner case. Filed as a deferred follow-up (low severity: skewed-clock peers are already eventually-revived when ANY message with a non-skewed timestamp arrives).

P2 (ACKNOWLEDGED, deferred): "A local UI/view-state feature unnecessarily changes the room contract WASM." Correct observation. The river-core crate is depended on by both the room contract AND the chat delegate, so any change to common/src/chat_delegate.rs rebuilds both WASMs. The room-contract migration is fully automatic via regenerate_contract_key() (see AGENTS.md "Contract Upgrade") — it's not a user-impacting migration, just a churn of the contract key on next load. Splitting river-core::chat_delegate into a separate crate that only the delegate (and UI/CLI) depends on would eliminate the churn, but that's a structural refactor not in scope for this PR. Filed as a follow-up candidate.

Re-running Codex on commit a60814e to verify the P1 fix.

[AI-assisted - Claude]

…dex P3)

Codex second-pass found a remaining race in the P1 fix:
`hydrate_hidden_dm_threads` can run AFTER `unhide_dm_thread`, in
which case the delegate response re-inserts the just-removed entry.
Combined with the unix-second-resolution `hidden_at_ts`, this can
re-hide a thread immediately after a successful outbound DM if the
delegate hydration was in-flight at the moment of send.

Fix: a session-scoped `RECENTLY_UNHIDDEN: HashSet<(VerifyingKey,
MemberId)>` tombstone set, populated by `unhide_dm_thread` BEFORE
the deferred signal write. `hydrate_hidden_dm_threads` consults the
set under its own lock and drops any incoming entry whose pair is
suppressed.

Persistence: `unhide_dm_thread` now queues an unconditional delegate
save even when no in-memory entry existed at removal time. Without
this, an entry sitting in the delegate from a prior session would
resurrect on next reload (the tombstone is session-only by design).

Tested via a new pure helper `resolve_hidden_thread_hydration` (6
new unit tests pinning insert/keep-fresher/take-fresher/suppress/
per-peer-scoping/per-room-scoping behaviours). The hot-path
`hydrate_hidden_dm_threads` now goes through this helper so the
suppression logic is unit-testable.

[AI-assisted - Claude]

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 17, 2026

Codex round-2 findings + response

P3 (FIXED in 3c41af4): "Late hydration can still resurrect a just-unhidden thread." Real race: hydrate_hidden_dm_threads running AFTER unhide_dm_thread re-inserts the just-removed entry from a delegate response that was in-flight at the moment of unhide. The strict <= filter then re-hides the thread when the outbound DM's unix_now() landed in the same second as the original hide.

Fix: a session-scoped RECENTLY_UNHIDDEN: HashSet<(VerifyingKey, MemberId)> tombstone. unhide_dm_thread inserts into the set BEFORE its deferred signal write; hydrate_hidden_dm_threads consults the set and drops any matching incoming entry. The hot-path was refactored to go through a new pure helper resolve_hidden_thread_hydration so the suppression logic is unit-testable.

Also: unhide_dm_thread now queues an unconditional delegate save (rather than only-when-something-was-actually-removed). Without this, the on-disk hidden_threads from a prior session would resurrect on next reload — the tombstone is session-only by design.

6 new unit tests pin: insert-into-empty, keep-fresher-in-memory, take-fresher-incoming, suppress-recently-unhidden, suppression-scoped-per-peer, suppression-scoped-per-room.

Re-running Codex on 3c41af4 to verify the P3 fix.

[AI-assisted - Claude]

…odex Low)

Codex round-3 Low: `resolve_hidden_thread_hydration` compared each
incoming entry only against the pre-existing `current` map, not
against earlier entries within the same `Vec`. A malformed legacy
blob containing the same `(room, peer)` twice in newer-then-older
order would emit BOTH into `to_insert`, and the caller's eventual
`HashMap::insert` loop would overwrite the newer cutoff with the
older one.

Normal UI saves go through a `HashMap` so duplicates can't arise
there, but the on-disk wire shape is a `Vec` — any other writer
(or a future corrupt-data scenario) could produce duplicates.
Fold incoming entries through a temporary `HashMap` keyed by
`(room_vk, peer)`, keeping whichever cutoff is largest, before
emitting `to_insert`. The "largest hidden_at_ts wins" invariant
is now actually upheld.

Two new unit tests pin the dedup behaviour: newer-then-older
ordering and reverse-with-tie ordering.

[AI-assisted - Claude]

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 17, 2026

Codex round-3 Low finding response

Codex round-3 (on 3c41af4) confirmed the P3 fix is correct and noted one Low:

Low (FIXED in 18cc4d1): resolve_hidden_thread_hydration compared each incoming entry only against the pre-existing current map, not against earlier entries in the same incoming Vec. A malformed legacy blob containing the same (room, peer) twice in newer-then-older order would let the older cutoff overwrite the newer one when the caller does the final HashMap::insert loop. Normal UI writes go through a HashMap snapshot so this can't arise on our write path; the fix is defensive against any other writer or corrupt-data scenario.

Fix: fold incoming entries through a temporary HashMap keyed by (room_vk, peer), keeping whichever cutoff is largest. The "largest hidden_at_ts wins" documented invariant is now actually upheld.

Two new unit tests pin the dedup behaviour: newer-then-older and reverse-with-tie. Total chat_delegate tests in river-ui now 13.

Re-running Codex on 18cc4d1 to confirm; then planning to squash-merge once that and CI come back clean.

[AI-assisted - Claude]

@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 17, 2026

Filed #266 to track the deferred Hidden conversations sub-menu — the v1 un-hide UX that the original issue marked as optional. PR #265 keeps the implicit revive paths (newer inbound message / outbound send) as the v1 mechanism; the explicit submenu is now tracked separately.

[AI-assisted - Claude]

sanity and others added 2 commits May 16, 2026 22:11
Adds the three regression tests called out as BLOCKING coverage gaps
by the testing reviewer on PR #265 (issue #261).

1. **Rail filter is now unit-tested.** Extracts the filter loop body
   from `dm_rail_section::build_view` into a pure helper
   `filter_rail_entries(entries, hidden) -> Vec<DmRailEntry>`. Six
   tests pin the user-visible "thread disappears from the rail"
   behaviour:
     - omits hidden entries (strict `<=` boundary)
     - newer inbound DM revives
     - newer outbound DM revives (via `last_any_ts > hidden_at_ts`)
     - hide is scoped per (room, peer) — no cross-room leak
     - hide is scoped per peer within a room
     - empty hidden map is a pass-through fast-path
     - unhide → thread reappears

2. **Hide → revive → re-hide round-trip.** Extracts the
   `hide_dm_thread` decision branch into the pure helper
   `decide_hide_action(existing, incoming) -> HideAction`
   (`Insert(entry)` / `NoOp`). Four direct tests for the decision
   logic, plus one round-trip test
   (`hide_unhide_rehide_round_trip`) that exercises the full
   sequence: hide@1000 → message@1500 revives → re-hide@1500 must
   advance cutoff (NOT no-op). This closes the regression window
   where a second Hide click after revival would silently no-op.

3. **Outbound DM revives hidden thread.** Three tests for the
   Codex P1 fix in `dm_thread_modal::do_send`: an outbound send
   clears the `(room, peer)` entry from `HIDDEN_DM_THREADS`. Pure
   helper `process_outbound_send_for_hidden(hidden, room, peer)`
   mirrors `unhide_dm_thread`'s in-memory effect so the invariant
   can be tested without a Dioxus runtime. Scope test confirms the
   unhide does not leak across rooms or peers; idempotency test
   confirms calling on a not-hidden pair is a no-op.

Total: 15 new tests (UI crate, native target).

AGENTS.md updated with a Phase 6 note explaining that hidden_threads
ride OUTBOUND_DMS_STORAGE_KEY (not a new storage key) so future
agents don't add a second top-level delegate key by mistake — that
would split the multi-device save path and need its own legacy-probe
extension.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The new comment in build_view's per-peer loop pointed at a
test name that doesn't exist (`outbound_message_revives_hidden_thread`).
The real test is `filter_rail_entries_newer_outbound_revives_hidden`
in the sibling `tests` module.

Comment-only — no behaviour change.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented May 17, 2026

Re: Codex P2 finding (same-second inbound DM hide race)

Codex flagged that the strict <= in is_thread_hidden means an inbound DM landing in the same Unix second as the hide click stays suppressed until a later-second DM arrives. This is a real but narrow race in the underlying feature design — the outbound path has the explicit unhide_dm_thread workaround (the P1 fix Codex flagged in round 1), but the inbound path has no equivalent.

I'm deferring this to issue #267 rather than fixing in #265 because:

  1. The behaviour is pre-existing in the feature design as merged in the previous reviews — not introduced by my new commit (which is purely tests + a doc comment + a pure-helper refactor).
  2. The most-defensible fix is mirroring the outbound path in the room-state apply hook — that's a real behaviour change that wants its own focused review.
  3. The race is narrow: only triggers for messages landing in the exact same Unix second as the Hide click, and recoverable via any later-second DM or directly opening the thread.

PR #265's job at this point is to land the test coverage for the BLOCKING gaps the testing reviewer flagged. #267 will track the symmetric inbound-revive fix.

[AI-assisted - Claude]

@sanity sanity merged commit e0c4ff8 into main May 17, 2026
6 checks passed
@sanity sanity deleted the feat/hide-dm-threads-261 branch May 17, 2026 03:24
sanity added a commit that referenced this pull request May 17, 2026
Publishes the merged tip of main containing:
- #263 (issue #255): legacy delegate response no longer overwrites the
  user's currently-selected room.
- #264 (issue #251): repopulate room_data.secrets on every network
  state ingestion path so newly-joined private-room members don't
  render encrypted placeholders until refresh.
- #265 (issue #261): hide stale DM threads from the left rail
  (local-view filter with multi-device sync via OutboundDmStore).

Counter 30000306 -> 30000307.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.

feat: hide stale DM threads from the left rail (local-only)

1 participant