-
Notifications
You must be signed in to change notification settings - Fork 60
fix: unread dot that can never be cleared when the message is unfetchable - #150
fix: unread dot that can never be cleared when the message is unfetchable #150piotrsynowiec wants to merge 1 commit into
Conversation
...tchable messages Slack's client.counts reports has_unreads by comparing a channel's `latest` against `last_read`, and it does so even when `latest` is no longer retrievable. On a retention-limited plan a dormant channel answers all three questions at once like this: client.counts has_unreads: true last_read: 1718264828.618929 latest: 1776079647.792389 conversations.history ok:true, messages:[], is_limited:true conversations.info properties.is_dormant: true slk stored that flag faithfully, so the sidebar showed an unread dot on a channel rendering "No messages yet". Every mark-read path was gated on having at least one message -- the fetch path on len(msgItems) > 0, the fresh-cache path on len(cached) == 0, and markChannelReadAsync on ts == "" -- so there was no timestamp to mark at, and opening the channel could not clear it. The dot was permanent. channelMarkTS now picks the mark timestamp from fetchChannelMessages's existing nil-vs-[] contract: nil means the history call failed and nothing is marked, [] means Slack authoritatively has nothing to show, non-empty marks the newest message as before. For the [] case it marks at Slack's own `latest` rather than at the wall clock. That distinction matters beyond tidiness. last_read_ts also feeds the sidebar's staleness filter -- IsStale hides a channel once now - last_read_ts exceeds hide_inactive_after_days (30 by default), and treats an unread channel as never stale. Marking "now" would clear the dot and then pin the dormant channel to the sidebar for the whole threshold window, having claimed the user just read a channel whose newest message is months old. Marking at the real `latest` clears the dot and leaves the channel its true age, so it ages out on its own. Measured on a live workspace: 127 days past the threshold. `latest` comes from client.counts because nothing else reports it -- conversations.info omits the field, and conversations.history cannot return a message that has aged out of the retention window, which is the whole situation. UnreadInfo now carries it. The lookup is a network call, so it fires only on the branch that needs it: an authoritative empty fetch on a channel flagged unread. Gating on has_unread also keeps an ordinary empty channel from firing a pointless conversations.mark on every open. When `latest` cannot be determined the channel is left unmarked -- a dot that clears on the next open is a smaller error than a read state written from a guess. Marking goes through Slack rather than only clearing the local flag because boot's ReplaceWorkspaceReadState re-applies client.counts as authoritative, and a local-only clear would light the dot again on the next start. Verified end to end against a live workspace: the dot clears, Slack reports has_unreads:false, and the channel drops out of the sidebar.
d5e9290 to
7b95b7a
Compare
gammons
commented
Sep 3, 2026
Real bug, correct diagnosis, right timestamp choice — but the plumbing needs work.
Confirmed the bug. Every mark-read path is gated on having messages: cmd/slk/main.go:1465, reducer_channels.go:405, and markChannelReadAsync early-returning on ts == "" (main.go:3094). And there's no escape hatch — markUnreadOfSelected needs a selected message and command.go has no mark-read command. Since sidebar/model.go:337 means IsStale never hides an unread channel, the channel also stays pinned in the sidebar and keeps catching a/A. Permanent dot, no workaround. On free-tier or 90-day-retention workspaces that's common, and it's exactly the papercut a keyboard-first client can't ship.
The latest field is real too — the repo's own captured contract (internal/slack/testdata/phase2-api-contracts.json) shows client.counts returning latest on channels, mpims and ims. The internal/slack/client.go half of this diff is correct and decode-safe. And marking at the real latest rather than time.Now() is the right call (sidebar/staleness.go:42).
Main issue: it re-fetches data we already have, on a loop.
client.counts is already called at cmd/slk/bootstrap_adapters.go:100 and cmd/slk/reconnect_sync.go:129. Both now receive UnreadInfo.Latest and throw it away. Then latestFromCounts fires a third full-workspace client.counts round-trip on channel open. And if latest comes back empty or conversations.mark is rejected, HasUnread stays true — so that round-trip repeats on every subsequent open of that channel, forever.
Persisting latest alongside last_read_ts/has_unread (internal/cache/channels_read_state.go:10) at the two call sites we already have is both cheaper and simpler.
Second: latestFromCounts ignores the package's own testability pattern. It takes a concrete *slackclient.Client, while cmd/slk/reconnect_sync.go:37 defines a one-method interface for exactly this call, in the same package. Consequence: it's untestable, and untested.
Third: the tests cover the one thing that couldn't have been wrong. 146 test lines for a 12-line pure function, and nothing else. No test for latestFromCounts, and no test that the Fetch closure at main.go:1454 marks anything — the actual behavior change is uncovered. TestChannelMarkTSNoLookupWhenMessagesPresent and ...WhenAlreadyRead duplicate table rows 6 and 2 with a call-counter bolted on. TestChannelMarkTSStaysStale duplicates row 3 and its if got == now branch is dead — with the lookup injected, got can only be slackLatest or "".
(Minor: got <= "1718264828.618929" is a lexicographic compare on a Slack ts. Works here by digit-count coincidence, not by design.)
Also: the comment at main.go:1454-1456 still says the channel is marked "up to now." It isn't — that's left over from the revision your description says was wrong.
What I'd like:
- Persist
UnreadInfo.Latestatbootstrap_adapters.go:100andreconnect_sync.go:129, read it from the db in theFetchclosure. DroplatestFromCounts, or keep it as a fallback behind a narrow interface likereconnect_sync.go:37. - Fix the stale
main.go:1454-1456comment. - Add a test for the
Fetchwiring: empty + unread marks atlatest; empty + unread with a nil fetch doesn't mark. Drop the deadnowassertion and fold the two duplicate call-counter tests into the table. - Optional:
channelMarkTSis pure read-state policy with no I/O, and that policy already has a home ininternal/(cache/channels_read_state.go,reducer_channels.go). Moving it there would also let Tier 1 atreducer_channels.go:405share it later.cmd/slkisn't off-limits by convention, so take or leave.
Also: the 32-line doc comment on a 12-line function restates the PR description verbatim. House style tolerates long comments but this is at the far end.
Your CI lint failure is not your fault — old golangci-lint panicking under go1.27, fixed on main by 6d39fe5. Your test job passed.
gammons
commented
Sep 3, 2026
#171 has landed and I've re-run CI here — the lint failure is gone and this is green now. That was the stale golangci-lint/go1.27 issue, not anything you did.
Note that #171 also enabled gofmt as an enforced lint check, so please run gofmt -w over your changes when you push the next revision.
My review above still stands — that's what's needed to move this forward.
Uh oh!
There was an error while loading. Please reload this page.
What breaks
A channel shows an unread dot in the sidebar while the message pane reads "No messages yet", and nothing the user does clears it. Opening the channel does not help. The dot is permanent.
Why
client.countsdecideshas_unreadsby comparing a channel'slatestagainstlast_read— and it does so even whenlatestis no longer retrievable. On a retention-limited plan, a dormant channel gives these three answers at the same time:Slack is not wrong, and neither is
slkfor storing the flag — the unread message genuinely exists, it has simply aged out of the fetchable window. It can never be displayed, and therefore never read.The problem is that every mark-read path was gated on having at least one message:
cmd/slk/main.go—Fetchif len(msgItems) > 0internal/ui/reducer_channels.go— Tier 1if len(cached) == 0 { return nil, false }markChannelReadAsyncts == ""With no message there is no timestamp to mark at, so
conversations.markwas never sent.The fix
channelMarkTSpicks the mark timestamp from the nil-vs-[]contractfetchChannelMessagesalready documents and honours:nil[]+has_unreadlatest[]+ already readWhy
latestand nottime.Now()This is the part worth reading, because the obvious implementation is wrong in a way that is easy to miss.
last_read_tsdoes not only drive the unread dot. It is also what the sidebar's staleness filter reads:IsStalehides a channel oncenow - last_read_tsexceedshide_inactive_after_days(30 by default), and it treats an unread channel as never stale.So a dormant channel is pinned to the sidebar by
has_unread. Marking it at the wall clock clears the dot — and then pins it right back for the full threshold window, because it now claims the user just read a channel whose newest message is months old. The symptom moves; the channel still will not go away.Marking at Slack's real
latestclears the dot and leaves the channel its true age, so the staleness filter ages it out on its own. On the workspace this was found on, that is 127 days past the threshold.I found this the hard way: the first version of this patch used
time.Now(), and the channel stayed in the sidebar with no dot.TestChannelMarkTSStaysStaleis the regression guard.Where
latestcomes fromclient.countsis the only endpoint that reports it.conversations.infoomits the field entirely, andconversations.historycannot return a message that has aged out of the retention window — which is the whole situation.UnreadInfonow carries it.The lookup is a network call, so it fires only on the branch that needs it: an authoritative empty fetch on a channel flagged unread. Two tests pin that it does not fire on the ordinary paths.
When
latestcannot be determined, the channel is left unmarked. A dot that clears on the next open is a smaller error than a read state written from a guess.Why it marks through Slack
Boot's
ReplaceWorkspaceReadStatere-appliesclient.countsas authoritative, so a local-only clear would light the dot again on the next start. It has to beconversations.mark.The
nilcase is what keeps this safe: a transient network failure returnsnil, never[], so a failed fetch can never mark a channel with real unread messages as read.Testing
cmd/slk/channel_mark_ts_test.gocovers every branch, both "no lookup on the ordinary paths" guards, and the staleness regression above.Verified end to end against a live workspace, including resetting the channel's read state back to the broken condition and reproducing the whole cycle: the dot clears,
client.countsthen reportshas_unreads: false, and the channel drops out of the sidebar.Full suite passes.
🤖 Generated with Claude Code