Skip to content

feat(activity): Slack Activity inbox - #169

Closed
agustif wants to merge 4 commits into
gammons:mainfrom
agustif:feat/activity-inbox
Closed

agustif wants to merge 4 commits into
gammons:mainfrom
agustif:feat/activity-inbox

Conversation

@agustif

@agustif agustif commented Aug 31, 2026

Copy link
Copy Markdown

Why

Slack's Activity inbox (mentions, DMs, reactions, …) is the other half of "what needs my attention" next to Threads. slk had no view for it.

What

  • New activity.feed / activity.views client (unofficial Slack protocol, same class as the rest of slk — it can change without notice).
  • Synthetic Activity row above Threads with an activity_v2 unread badge from client.counts.
  • Message pane: filter / sort / unread-only, compact and detailed density, jump-to-message.
  • Prefs in [activity] in config.toml.
  • Docs: README, wiki Features / Configuration / Keybindings.

Test plan

  • go test ./internal/config/ ./internal/slack/ ./internal/bootstrap/ ./internal/ui/ ./internal/ui/sidebar/ ./internal/ui/activityview/ ./cmd/slk/
  • Open Activity from the sidebar; feed loads; unread badge matches Slack.
  • Cycle filters, toggle unread-only, jump to a mention/DM.
  • Reconnect: badge refreshes from client.counts.

Merge after #168 (stack 4/4). Contains the full stack. Testdata is sanitized API JSON (no tokens).

saveWorkspaceVersionTS used to look only for [workspaces.<tomlKey>].
When tomlKey fell back to the raw team ID it appended a second block
next to the slug-keyed one, and Load then refused to start ("both
reference team_id").

Merge leftover team-ID-keyed prefs into the slug block, drop the
duplicate key, and write subsequent saves into the existing slug
section (or a matching team_id field).
users.conversations IM objects sometimes omit `user`, so the sidebar
row was blank and the unresolved-DM sweep had nothing to fetch.
Overlay counterparties from userBoot's ims[], seed display names from
edge before the first paint, and patch wctx.Channels so a workspace
switch keeps the resolved name.

DMNameResolvedMsg now carries TeamID so a late resolution from an
inactive workspace cannot rename the active sidebar.
DMs are the daily-driver conversations; they belong under Threads,
not at the bottom of the rail after custom sections. Config-mode
order is now DMs, custom sections, Apps, Channels. Slack-mode keeps
the provider linked list but lifts direct_messages to the top.
Add an Activity view backed by the unofficial activity.feed /
activity.views endpoints and client.counts activity_v2. A synthetic
Activity row sits above Threads with an unread badge; the pane
supports filter/sort/unread-only, compact and detailed density, and
jump-to-message.

Prefs live in [activity] in config.toml. This is unofficial Slack
protocol, same class as the rest of slk — it can change without
notice.
@gammons

gammons commented Sep 3, 2026

Copy link
Copy Markdown
Owner

@agustif — thank you for this, it's a lot of careful work and several parts of it are genuinely better than what I'm merging. I'm going with #109 though, and I want to be straight with you about why rather than leave you guessing.

1. Row content. #109 hydrates message bodies via messages.list and renders the actual text. This renders metadata only — previewText (internal/ui/activityview/model.go:815-847) returns "direct message", "reply from Alice", ":tada:" — and there's no messages.list call anywhere in the PR. That's specifically what I asked #109 to change back in July, with screenshots. Related: flattenActivityEntry doesn't set ActorID for dm or thread_v2, so those rows have no sender name either.

2. All bot DMs are silently dropped. flattenActivityEntry (internal/slack/activity.go:350-392) has no bot_dm_bundle case. The default arm reads inner.Message.Channel, which is empty for that shape (the real payload puts it at bundle_info.payload.message), and the ChannelID == "" guard then discards the item at line 388-390. I ran your parser against #109's capture of a real bot_dm_bundle entry and it returns 0 items. GitHub, PagerDuty, Jira, Slackbot — a large share of a real Activity tab — just don't appear. Your 91-line fixture doesn't include that type, which is why the tests are green.

3. Process. #109 was open a month, went through two rounds of my review, and I'd already told the author it was ready to merge. I'm not going to leapfrog that.

Also, for the record: channelGlyph (activityview/model.go:935) is dead code and that's your lint failure. Your test failure was not your fault — that's a flaky test on main which I've fixed in #171.

Three things in here I want, and will happily take as standalone PRs against main, credited to you:

  1. The activity_v2 badge from client.counts — the GetCounts/CountsSnapshot refactor in internal/slack/client.go. This is strictly better than deriving the badge from the fetched feed: zero extra requests, and it's correct at cold start and after reconnect. feat(activity): Activity feed view (mentions, threads, reactions, DMs) #109's badge reads 0 until you open the view. I'd take this one today.
  2. activity.views support — custom tabs (Unreads / Reactions / VIP) with the f/F/s cycling and the [activity] config block. Well tested, and the tests asserting exact request form shape (TestBuildActivityFeedForm_*) are the right way to pin an undocumented endpoint.
  3. Generation-guarded fetchesActivityFeedLoadedMsg.Gen. feat(activity): Activity feed view (mentions, threads, reactions, DMs) #109 needs this and doesn't have it. Your TestApp_ActivityFeedLoadedDropsStaleGen is a good test.

Two pieces of process feedback, offered constructively:

Please stop stacking feature PRs. This is 45 files because it carries #166, #167 and #168 underneath it. Even measured against #168 it's still 38 files / +3220. Reviewing it means reviewing four PRs at once, and that's a large part of why it sat. I'm reviewing those three separately on their own merits.

And per CONTRIBUTING, open an issue first for something this size. #12 was the right place — and doing that would have surfaced #109 before you built this, which would have saved you a lot of work. That's the actual purpose of the rule, not bureaucracy.

If you want to fix VIP: len(e.Priority) > 2 (activity.go:359) while you're in there — inferring VIP from the byte length of raw JSON means an explicit "priority": null gives len == 4 and renders a spurious vip chip on every row. A typed decode would be better.

Closing as superseded. I'd genuinely like those three follow-ups.

@gammons gammons closed this Sep 3, 2026
@gammons gammons added the duplicate This issue or pull request already exists label Sep 3, 2026
Yukaii pushed a commit to Yukaii/slk that referenced this pull request Sep 4, 2026
…ed PRs

TestBackgroundFetchRetriesAfterBackoffExpiry used two barriers that do
not actually gate the state it asserts on, so it failed intermittently
in CI on PRs that do not touch this package (gammons#163, gammons#166, gammons#169).

Both barriers were false:

  - fakeMemberAPI records a call on *entry*, but backgroundFetch writes
    lastFailed only after the API call returns. Backdating lastFailed
    after waitForCallCount(1) therefore raced the manager's own write
    and got clobbered, leaving the backoff live so the retry never
    fired: "timed out waiting for 2 API calls".

  - EnsureFresh calls pushSnapshot synchronously on the caller's
    goroutine, so waitForPush(sink, 2) was satisfied by the test's own
    second EnsureFresh rather than by the background fetch. The
    lastFailed assertion then ran while that fetch was still in flight:
    "lastFailed not cleared by a successful fetch".

Replace both with waitUntil, which polls the manager state under its own
lock. Add an optional post-call delay to fakeMemberAPI and use it here,
so the slow-API interleaving that used to be a rare race is now the
test's default path.

Verified: with the 50ms delay the old test failed 5/5; the new one
passes 20/20 under -race, and still fails as intended when
`delete(m.lastFailed, channelID)` is removed from manager.go.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants