Skip to content

fix(ws): show a conversation that first appears as a message - #222

Merged
gammons merged 3 commits into
gammons:mainfrom
landonforshage:fix/discover-conversation-on-message
Sep 17, 2026
Merged

gammons merged 3 commits into
gammons:mainfrom
landonforshage:fix/discover-conversation-on-message

Conversation

@landonforshage

Copy link
Copy Markdown
Contributor

Problem

A coworker started a new group DM with me and another person and sent three messages. The official client showed the group with a 3 badge; slk showed nothing, not even a hidden row. slk had been running for about 35 hours when the group was created. Restarting would have shown it, because client.userBoot lists it now, so this looks intermittent from the outside: it only affects a conversation created while slk is running.

The workspace is Enterprise Grid, where users.conversations is refused (enterprise_is_restricted) and the sidebar comes from the bootConversations fallback.

Cause

Verified on 2026-09-16 against the running v0.20.0 session's cache and live API responses:

  1. The messages arrived. All three are in messages for that channel, with created_at matching the send times, so OnMessage ran for each.
  2. The conversation never got a row. channels has no row for it. OnConversationOpened is the only mid-session writer of that row and it always upserts, so it never ran for this conversation. With no row, OnMessage's UpdateChannelReadState (a plain UPDATE) changed nothing, and the sidebar had no item to show.
  3. Which event Slack sent, if any, is not known. I have no capture. Two candidates in dispatchWebSocketEvent: mpim_joined is not handled, and mpim_open/im_open decode channel as a full slack.Channel, while slack-go models im_open's channel as a string ID (IMOpenEvent is ChannelInfoEvent), which would fail to decode and be dropped silently. The socket also connects with lazy_channels=1, so Slack may not push a conversation event at all. In the field check below, reopening a closed DM produced an unhandled channel_updated whose channel is a bare ID (updates: {properties: {}, updated: …}), with no conversation object; whether an im_open came too can't be told, because that case drops a failed decode without logging.
  4. conversations.info answers for it on this Grid workspace: is_mpim: true, is_member: true, and the mpdm-… name buildChannelItem needs.

Fix

OnMessage now calls discoverConversation first. If the channel ID is not in channelTypes, it calls conversations.info (new Client.GetConversationInfo) and feeds the result through the existing OnConversationOpened logic. Three commits:

  • Discover on message. The lookup runs synchronously on the WebSocket goroutine with a 10s timeout, so the row and channel type exist before OnMessage writes the unread flag and chooses the notification type. It also keeps wctx.Channels written only from that goroutine (data race: WorkspaceContext.Channels read on UI goroutine, written by WS handler #208). It costs one request per new conversation per session.
  • Announce after the unread write. OnConversationOpened is split into addConversation (row, wctx.Channels, finder, name/type maps) and publishConversation (the UI message). OnMessage publishes only after the unread write. Sending first let the UI goroutine run rebuildFilter against a never-opened, not-yet-unread group DM and hide it under the default 30-day threshold. The next NewMessageMsg only invalidates the render; it doesn't re-filter, so the group stayed hidden.
  • Back off only when retrying can't help. A refusal (SlackErrorResponse, e.g. enterprise_is_restricted or channel_not_found) waits a minute, and a 429 waits its RetryAfter. Otherwise an org that refuses conversations.info, as some refuse users.conversations, would pay a blocking request per message on every channel missing from the boot list. A network error or timeout retries on the next message; a flat wait would skip the rest of a quick burst, which may be the only retry that conversation gets.

No is_member check: a delivered message is treated as membership, and conversations.info has no is_member for ims.

Why not fix it elsewhere

  • Handle mpim_joined, or decode im_open's string form? Without a capture, either one guesses the payload shape, and the existing handler's test payloads were written from its plan (docs/superpowers/plans/2026-05-01-mpdm-unread-indicator.md), not captured. The message is the one thing proven to arrive, and looking up an unknown ID when it's needed also covers the lazy_channels case.
  • Look up asynchronously? The unread write needs the row, and the sidebar needs the unread write before it sees the row. Doing that off-goroutine means buffering or replaying messages and adds another writer to wctx.Channels.

Tests

In cmd/slk/event_handler_test.go. The conversations.info fixtures are the result shapes the Grid workspace returned for an mpim and an im, with IDs and names replaced. The mpim has is_channel: true next to is_mpim: true; the im has no is_member.

  • TestOnMessage_UnknownConversation_AddsItUnread: two messages on an unknown group DM give one lookup, a group_dm item in wctx.Channels, and the UI messages in order ConversationOpenedMsg, NewMessageMsg, NewMessageMsg. When ConversationOpenedMsg is sent, has_unread is already true in the DB.
  • TestOnMessage_UnknownConversation_RetryDependsOnFailure: a timeout is retried on the next message and the conversation is added. A wrapped SlackErrorResponse or *RateLimitedError is not retried within the wait, and is retried and added after it.

Red with each change reverted: remove the discoverConversation call and both go red; publish inside discovery, before the unread write, and _AddsItUnread goes red (opened unread=false); back off on every error and the timeout case goes red; never back off, or ignore the stored retry time, and the refused and rate limited cases go red. Build, vet, go test ./... -race, gofmt -l and golangci-lint run (v2.13.1) are clean.

Field check (macOS, the same Grid workspace, patched build). Two runs. In each, I first closed a DM in the official client, confirmed client.userBoot no longer listed it as open, and relaunched slk. Both boots loaded 37 conversations instead of 38, and the log never mentioned the closed DM.

  • A coworker's message: 112 ms after their message on the unknown DM, the log shows discovered conversation from message … im=true, and the cache has a dm row with has_unread=1 and a badge count of 1. The workspace wasn't the active one: its rail dot lit, and switching in showed the DM in the sidebar, unread, with its badge. That is the reported path end to end, for a 1:1 DM rather than a group DM; the group DM shape is what TestOnMessage_UnknownConversation_AddsItUnread uses.
  • My own message, to myself: the same discovery (114 ms), and the im_marked echo that followed set last_read on the new row; before this change it would have updated nothing. The DM ended up in the channel finder but not the sidebar, because I had also marked it unread in the official client: Slack sent im_marked with the DM's March last_read and unread_count: 0 (my own message never counts), OnChannelMarked recorded it read and months stale, and the staleness filter hid it. That is the existing mark-unread handling, not this change.

Things that usually bite, checked: a DM opened with the new-message picker is known to the sidebar but not to channelTypes, so its first message does one lookup; sidebar.UpsertItem replaces the minimal item by ID, buildChannelItem recomputes section and mute, and UpsertChannel doesn't touch read-state columns, so nothing is clobbered and no finder entry is duplicated. Self-sends, edits, thread replies and bot messages reach the lookup and then the existing unread gates, unchanged. Inactive workspaces get the row and wctx.Channels, so the rail dot can light.

One visible side effect: when users.conversations is refused, the boot list is userBoot's subset, so member channels missing from it now appear in the sidebar when their first message arrives.

Noted but not changed

  • One failed lookup followed by silence. Discovery is triggered only by a message, so if a conversation's last message hits a failed lookup, it stays out of the sidebar until its next message or a restart. Retrying on a timer would need a goroutine, a lock around the handler's state, and replayed unread state; I didn't think that was worth it for this case.
  • A known conversation hidden by the staleness filter stays hidden when a message arrives. notifyReadStateChanged calls sidebar.Invalidate, which re-renders but doesn't re-run rebuildFilter, so the row only reappears on the next filter rebuild (a channel switch, for example). Checked with a throwaway App test. Pre-existing and a different layer.
  • mpim_open/im_open/im_created are still decoded as before. If im_open really carries a string ID, that case is dropped silently today.
  • Marking a conversation unread in the official client, when its only newer message is your own, arrives as im_marked with an older last_read and unread_count: 0, which slk records as read. Seen in the field check; pre-existing.
  • rtmEventHandler.channelTypes is read from the mark-unread goroutine (main.go, the countMentionsSince call) while the WebSocket goroutine writes it. Pre-existing; this PR adds writes only when a new conversation is discovered.

AI assistance

Written with Claude (Opus 5) driving the investigation, code and tests after I hit the bug in a live session. The cause was confirmed against the running session's cache rows and live client.counts, client.userBoot and conversations.info responses, not inferred from the code. Codex and a second Claude model (Fable 5.1) reviewed the diff; the unread-ordering race and the retry backoff came from those reviews, and the error-type split from Codex's second pass. I've read the diff and will defend it in review.

🤖 Generated with Claude Code

landonforshage and others added 3 commits September 16, 2026 10:32
A group DM another user created mid-session never appeared: its
messages reached OnMessage and were cached, but no conversation event
before them produced a sidebar row, so the unread writes for it updated
no row and there was nothing to show. The message is the one signal
proven to arrive, so an unknown channel ID on a message now triggers one
conversations.info lookup and the existing OnConversationOpened path.

The lookup is synchronous on the WebSocket goroutine so the row and
channel type exist before the unread write and the UI dispatch, and so
wctx.Channels keeps its single writer. Failures are not remembered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sidebar's staleness filter reads read state the moment a row
arrives. Sending ConversationOpenedMsg before OnMessage wrote has_unread
let the UI goroutine evaluate a never-opened group DM as read, hide it
under the default 30-day threshold, and keep it hidden: the following
NewMessageMsg only invalidates the render, it does not re-filter. So
OnConversationOpened is split into addConversation and
publishConversation, and OnMessage publishes after the unread write.

A failed lookup now backs off for a minute per channel. Where
conversations.info is refused (as users.conversations is on some
Enterprise Grid orgs) a busy unknown channel would otherwise issue a
blocking request per message, and a 429 would be retried immediately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A flat one-minute wait after any failed lookup could strand a
conversation: a transient error on one message suppressed retries for
the rest of a quick burst, and once the burst ended nothing looked the
conversation up again. Network errors and timeouts may clear by the
next message, so they retry immediately again. A Slack API refusal
(SlackErrorResponse) waits a minute and a 429 waits its RetryAfter,
since neither changes on the next message and retrying would cost a
blocking request per message on a busy channel.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gammons

gammons commented Sep 17, 2026

Copy link
Copy Markdown
Owner

Verified the mechanism and this is right. Merging.

The two design decisions that make it work:

  • Publish after the unread write. Splitting OnConversationOpened into addConversation and publishConversation so the UI hears about the row only after has_unread is set — that ordering is the whole bug for the staleness filter. I checked the test pins it: ConversationOpenedMsg arrives with has_unread already true in the DB.
  • Discovery on the WS goroutine, synchronously. This keeps wctx.Channels written from one goroutine only, which is the constraint from data race: WorkspaceContext.Channels read on UI goroutine, written by WS handler #208. The backoff split is right too: a refusal (enterprise_is_restricted, channel_not_found) backs off a minute, a 429 waits its RetryAfter, and a network error retries on the next message — so a Grid org that refuses conversations.info pays one blocking lookup per minute, not one per message, and a flaky network doesn't permanently lose the conversation.

I verified the tests have teeth: with discoverConversation stubbed out, TestOnMessage_UnknownConversation_AddsItUnread fails with lookups = 0, want 1, and the retry test fails on the timeout case.

The sync lookup blocking the WS loop for up to 10s is the one thing I weighed — but the backoff means it's bounded per conversation, and your "Why not fix it elsewhere" section covers the async alternative honestly (buffering + replay + another writer to wctx.Channels is strictly worse).

Also noted you checked channelTypes against the new-message-picker path and that UpsertItem/buildChannelItem recompute section and mute rather than clobbering. Build, vet, gofmt clean; 57 packages green under -race.

Merging.

@gammons gammons added the ready to merge Reviewed, approved, no blockers label Sep 17, 2026
@gammons
gammons merged commit 5beaa0f into gammons:main Sep 17, 2026
3 checks passed
gammons added a commit that referenced this pull request Sep 17, 2026
…em-conflict

fix(slk): main does not build — semantic conflict between #162 and #222
laraibg786 pushed a commit to laraibg786/slk that referenced this pull request Sep 17, 2026
…ammons#222

gammons#162 added FinderItem to ConversationOpenedMsg so a newly opened
conversation appears in the channel finder. gammons#222 (based on an older
main) split OnConversationOpened into addConversation/publishConversation
on a version of the code that predates gammons#162. The three-way textual merge
was clean, so GitHub allowed it, but the result placed `FinderItem:
finderItem` inside publishConversation, where finderItem is out of scope
-- main did not build:

    cmd/slk/main.go:5119:15: undefined: finderItem

addConversation now returns the finder item alongside the sidebar item,
discoverConversation forwards it, and publishConversation takes and sends
both, restoring both behaviors: gammons#222's announce-after-unread-write
ordering and gammons#162's finder sync.

go build, go vet, gofmt and go test ./... -race (57 packages) all pass,
including the gammons#222 discovery tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Reviewed, approved, no blockers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants