Skip to content

fix: subscribe the Discord member viewport a Watch chose before READY - #51

Open
itz4blitz wants to merge 1 commit into
mainfrom
fix/discord-watch-before-ready
Open

itz4blitz wants to merge 1 commit into
mainfrom
fix/discord-watch-before-ready

Conversation

@itz4blitz

Copy link
Copy Markdown
Owner

Cause

members.rs::watch stored the Watch in Watched.seen before working out what it puts on screen. A Watch sent between identify and READY found no servers in state.guilds and no conversation in the store, so it placed nothing. ready.rs::ready then called sync_subscription with that empty state, and every later identical Watch (including the heartbeat's) was skipped by the seen equality check. The member list stayed unsubscribed, and Members answered pending for good.

Fix

  • watch now does only the dedupe and records seen. The placement logic moved, unchanged, into place.
  • New rewatch. At the end of ready it places the latest seen Watch against the metadata READY just installed. If no Watch was ever sent, it falls back to the old sync_subscription, so a Space opened by Tree alone stays as it is.
  • place checks Arc::ptr_eq against the current seen. A Watch that a newer one overtook on the viewport thread is never applied over it, which keeps ordering intact when navigation changes while connecting.
  • A cache miss after connect still answers pending: true with no members, never an empty roster.

The change touches only watch and the final call in ready. Resume (#42) and the send budget (#44) are left alone.

Validation

  • New test member_viewport_selected_before_ready_is_subscribed_after_connect (not ignored). The fake gateway gains a ready_after delay, here 1.5 s. After identify and before READY, the test sends Watch #general and then Watch #rules (members: (0, 50)). It then checks:
    • nothing is subscribed before READY;
    • after Connected, op 37 subscribes {"4105": [[0, 99]]};
    • #general (the channel it navigated away from) is never subscribed;
    • Members is pending, not empty.
  • With the src changes reverted, the test fails ("timed out waiting for the member window"). With them, it passes.
  • cargo test -p chatd: 134 unit tests, plus integration tests: discord 31 (30 existing + 1 new), e2e 25, telegram 17, slack 15, blobs 14, polish 12, calls 10, signal 10, teams 7, messages 5, upload 5, playback 4. All pass: 0 failed, 0 ignored.

Fixes #43

🤖 Generated with Claude Code

A Watch that arrived while the gateway was connecting found no servers or
channels, so it placed nothing, yet was recorded as seen; READY then
synced that empty state and the same Watch again was skipped. READY now
places the shell's latest Watch against the metadata it just installed,
and a Watch overtaken by a newer one on the other thread is not applied.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@itz4blitz

Copy link
Copy Markdown
Owner Author

@vbmithr this one fixes the pre-READY ordering case from #43.

What changed (members.rs, ready.rs):

  • watch only records. It now just checks for an unchanged Watch and stores it. The rest of its old body moved unchanged into a private place, which works out the screen and the live member list from the metadata.
  • READY places the stored Watch. ready now ends with rewatch instead of sync_subscription(). That places the latest stored Watch against the metadata READY just installed, then subscribes. If no Watch was ever sent, it does what the old code did.
  • The latest Watch wins. Watch and READY run on different threads, so place skips a Watch that a newer one has already replaced. Navigating while connecting ends on the newest screen.
  • A miss is still pending. A cache miss after connect still answers pending, never an empty roster.

Test: member_viewport_selected_before_ready_is_subscribed_after_connect in crates/chatd/tests/discord.rs, in the normal suite. The fake gateway gained a ready_after delay. Before READY, the test sends a Watch for #general (4101) and then one for #rules (4105). It asserts:

  • nothing is subscribed before READY;
  • after connect, the subscription is {"4105": [[0, 99]]};
  • 4101 is never subscribed;
  • Members answers pending rather than empty.

It fails with the fix reverted.

One behaviour change: on reconnect, the screen is rebuilt from the last Watch. If a different Space was opened from the tree after that Watch, READY puts the Watch's screen back until the next Watch arrives.

To try it:

git fetch origin pull/51/head:pr-51 && git checkout pr-51
cargo test -p chatd --test discord member_viewport_selected_before_ready
cargo test -p chatd                                        # full daemon suite

Your delayed-READY reproducer should pass on this branch too, if you still have it.

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.

Reconcile member viewport selected before Discord READY after metadata arrives

1 participant