Add 'also send to channel' broadcast for thread replies - #170

Reviewing this as a draft — direction, not polish. The direction is right and the quality bar is high. reply_broadcast is a first-class Slack primitive, slack-go already exposes MsgOptionBroadcast(), we already do thread replies end-to-end, and it's nowhere near the non-goals list. Users whose teammates broadcast replies would otherwise silently not be able to reciprocate.

Things you got right that people usually don't:

• The wire layer is minimal and correct (client.go:1075-1077), and you wrote a negative test asserting reply_broadcast is unset for a plain reply, not just a positive one.
• The optimistic path mirrors the existing MessageSentMsg contract properly — local placeholder, SwapLocalSent on success, RemoveLocalSent on failure — and carries Subtype: "thread_broadcast" so the channel row renders the same label the WS echo would produce.
• The self-send dedup reasoning is right and documented: the broadcast copy shares the reply's ts, so one RecordSent covers both.
• The hint row is measured dynamically — view_thread.go:56 folds lipgloss.Height(threadComposeView) into threadComposeHeight and the layout cache key at :65. That's the detail most contributors miss, and getting it wrong breaks the layout subtly.
• SetBroadcast(false) on thread open in both entry points, with a test.

On size: 502/16 is proportionate. ~288 lines are tests (57%), ~70 are re-indentation from threading one parameter through SendReply, so real production code is ~140 across a legitimately layered stack. That's the cost of the architecture, not padding.

The one thing I want changed: don't make the toggle sticky.

mode_insert.go:191-204 reads Broadcast() before Reset(), and Reset() doesn't clear it, so it stays armed indefinitely within a thread. Two problems: I don't believe that matches Slack (the desktop client clears "Also send to #channel" after each send — please verify), and regardless, broadcasting is socially expensive and effectively irreversible. The hint is a single dim line that's easy to stop noticing, and slk is fast enough that someone will fire off three replies without re-reading the compose box. Reset-after-send costs one keystroke when you genuinely want two broadcasts.

Worth reconsidering the binding while you're at it. ctrl+o has no mnemonic (ctrl+b is taken by ToggleSidebar). Consider alt+enter as "send with broadcast" — it's a one-shot, which sidesteps the stickiness problem entirely rather than fixing it. Some terminals and readline configs also eat ctrl+o (operate-and-get-next); not fatal in raw mode, but worth a line in wiki/Terminal-Compatibility.md if it stays.

The guarding is otherwise correct — thread compose only, skipped while a picker is active so picker keys keep precedence, with a test that channel compose never exposes it.

Rest of the list before un-drafting:

1. gofmt -w cmd/slk/main.go — the SendReply: literal is de-indented one level relative to ListFetch: above it. It slips past CI because main.go was already unformatted on main; chore(ci): fix flaky membership test and enforce gofmt #171 fixes that and turns gofmt on in CI, so rebase after it lands.
2. Fix the stale comment at mode_normal.go:10 claiming ctrl+o/ctrl+i are nav back/forward — they're ctrl+h/ctrl+k. That's what made ctrl+o look taken.
3. Add a test for an inbound WS thread_broadcast echo from another user rendering in the channel feed. The optimistic path is well covered; the inbound path isn't.
4. Tick the three manual verification items.
5. Mention the threadComposeChannelName change in the description — it's a legitimate drive-by fix (thread compose said the literal string "thread" instead of the channel name, with a sane "channel" fallback for DM/MPIM) but it shouldn't be a surprise in review.

Nice work on this one.

Heads up — #171 landed (flaky-test fix plus a tree-wide gofmt pass with the formatter now enforced in CI), and this PR now conflicts with main. It's the only open PR that does, because of the cmd/slk/main.go and reducer_threads.go reformatting.

Good news is the rebase should resolve most of it mechanically, and it makes item 1 on my list above moot: the de-indented SendReply: block gets fixed by the reformat rather than by you.

One thing to know before you rebase: gofmt is now a CI-enforced lint check. It'll catch formatting on your new files that CI previously couldn't see, so run gofmt -w over your changes before pushing.

The rest of my review still stands — mainly making the broadcast toggle non-sticky, and reconsidering ctrl+o vs a one-shot alt+enter.

Thanks for the thorough review! All points addressed:

1. Rebased onto upstream main: resolved the conflict in mode_insert.go, and gofmt -l . is 100% clean across the whole repo.
2. Non-sticky toggle + Alt+Enter one-shot:
   • compose.Model.Reset() now clears broadcast = false, so the flag disarms immediately after send (and on Ctrl+U).
   • Added Alt+Enter in thread compose for one-shot send-with-broadcast without needing to toggle first.
   • Updated the hint row to ↪ also send to #channel (ctrl+o to cancel).
3. Inbound WS broadcast echo in channel feed:
   • Fixed reduceNewMessage in internal/ui/reducer_send.go so inbound thread_broadcast messages from other users are appended to the channel feed in addition to incrementing the thread reply count.
   • Added TestNewMessage_InboundThreadBroadcastRendersInChannelFeed covering feed append, parent reply count bump, and thread panel append.
4. Stale comment fixed: updated mode_normal.go:10 to reference ctrl+h/ctrl+k.
5. Docs updated:
   • Added note in wiki/Terminal-Compatibility.md regarding terminals/readline eating Ctrl+O.
   • Updated wiki/Keybindings.md and wiki/Features.md with both Ctrl+O and Alt+Enter.
   • Explicitly called out the threadComposeChannelName drive-by fix in the PR description.

Went through all of this properly. Every item is addressed, and I verified each one against the branch rather than taking the summary on trust. Nice work — this is close.

What I confirmed:

• Rebase. cmd/slk/main.go went from ~70 lines of re-indentation noise down to +12/-2. The gofmt merge absorbed it exactly as hoped, and gofmt -l is clean on the branch.
• Non-sticky. compose/model.go:355 clears broadcast in Reset(), so it disarms after send and on Ctrl+U. TestResetClearsBroadcast pins it.
• Alt+Enter. mode_insert.go:196 — Broadcast() || isAltEnter, then Reset(). Correct ordering, and it's inside the thread-compose branch so channel compose is unaffected. Checked for collisions: ModAlt is used nowhere else in internal/ui.
• Ctrl+O guard. Thread compose only, skipped while a picker is active so picker keys keep precedence.
• Stale comment at mode_normal.go:10 now correctly says Ctrl-h/k.
• Self-send dedup still holds. This was my main worry once broadcasts started hitting the append path — IsSelfSent(m.Message.TS) returns early well before it, and the broadcast copy shares the reply's TS, so a self-sent broadcast's echo is still dropped. No double-render.
• Full suite green on the branch: build, vet, go test ./... -race, 54 packages.

The reducer_send.go change is more valuable than either of us framed it.

I checked whether Subtype is actually populated on the inbound WS path before trusting isBroadcast — it is, main.go:4092. Then I found this on main at main.go:4033:

isThreadReply := threadTS != "" && threadTS != ts
isBroadcast := subtype == "thread_broadcast"
shouldMarkChannel := !isThreadReply || isBroadcast

with a comment saying broadcasts mark the channel unread like top-level messages do. So main already treats an inbound broadcast as channel-worthy for read state — but reduceNewMessage sent it down the else branch and only bumped the reply count, never appending it.

Net effect on main today: a teammate broadcasts a thread reply, your channel gets an unread dot, you open it, and there's nothing new there. That's a live bug that has nothing to do with sending broadcasts.

I verified it rather than assuming — reverted just your reducer_send.go hunk back to main's if/else and ran your new test:

--- FAIL: TestNewMessage_InboundThreadBroadcastRendersInChannelFeed
 app_thread_broadcast_test.go:274: channel feed messages = 1, want 2 (parent + broadcast)

So the test genuinely fails without the fix, and the fix closes a real inconsistency rather than adding a special case. Worth calling out in the PR description — this fixes an existing bug, it isn't only feature work.

Restructuring the mutually-exclusive if/else into two independent conditions is also just the more honest model: "does this belong in the channel feed" and "does this bump a reply count" are separate questions, and a broadcast answers yes to both.

One thing left, and it's doc-only.

wiki/Terminal-Compatibility.md offers Alt+Enter as the escape hatch for terminals that eat Ctrl+O. But Alt+Enter has its own caveat you don't mention: on macOS, Terminal.app and many iTerm2 profiles don't send Meta for Option by default, so Option+Enter won't set ModAlt and the one-shot silently won't fire. Given the Homebrew cask is our main install path, a lot of users are on exactly those terminals — and as written, someone whose Ctrl+O is intercepted gets pointed at a fallback that may also not work.

Please add a line noting Alt+Enter needs "Use Option as Meta key" enabled on macOS terminals, and that Ctrl+O is the primary binding.

Do that, tick the four manual checks, and take it out of draft — then I'll merge it.

Thanks for taking it out of draft — two things still outstanding before I merge, though.

Your last commit is 3254377 from 01:18, which predates my review at 10:15, so neither of these has landed yet:

1. The Alt+Enter caveat in wiki/Terminal-Compatibility.md. The note currently offers Alt+Enter as the escape hatch for terminals that intercept Ctrl+O, but doesn't mention that Alt+Enter has its own precondition: macOS Terminal.app and many iTerm2 profiles don't send Meta for Option by default, so Option+Enter never sets ModAlt and the one-shot silently doesn't fire. Since the Homebrew cask is our main install path, a lot of users are on exactly those terminals — and as written, someone whose Ctrl+O is eaten gets pointed at a fallback that may also not work for them. One line noting Option-as-Meta needs enabling, and that Ctrl+O is the primary binding.

2. The four manual checks are still unticked.

- [ ] Manual: toggle on (Ctrl+O), send a thread reply, verify it appears in the channel ...
- [ ] Manual: send via Alt+Enter and verify one-shot broadcast
- [ ] Manual: verify toggle is non-sticky (subsequent reply is plain thread reply)
- [ ] Manual: verify toggle resets when opening a different thread

I'm not asking for ceremony here. The automated tests cover the wire form, the optimistic row, the rollback, and the inbound echo — but nothing covers slk actually talking to Slack and the broadcast showing up in a real channel. That's the one thing only you can check, and it's the whole point of the feature. If you've already done these, just tick them. If not, they're worth doing before this goes out, because broadcasting to a channel is the kind of mistake users can't take back.

Everything else I verified against the branch and it's solid — the non-sticky reset, the Alt+Enter ordering, the Ctrl+O picker guard, the self-send dedup, ModAlt having no collisions. And your reducer_send.go change fixes a real pre-existing bug: on main today an inbound broadcast marks the channel unread but never renders in the feed, so you get a dot and find nothing. I confirmed that by reverting just that hunk and watching your new test fail.

So it's genuinely close. Doc line plus the manual passes and I'll merge it.

Correction, @Yukaii — I took this out of draft, not you. My last comment implied you'd un-drafted it without pushing the changes, and that's wrong. Sorry, that wasn't fair.

The two asks stand on their own merits, but neither is you jumping the gun:

1. The Alt+Enter / Option-as-Meta line in wiki/Terminal-Compatibility.md.
2. The four manual checks.

And on the manual checks specifically — since I'm the one who moved this out of draft, tick them if you've already run them and I'll take your word for it. I only care that someone has watched a broadcast actually land in a real channel, because that's the one part no test here covers.

Everything else I verified and it's solid.

Thanks @gammons!

1. Option-as-Meta note: This is included in wiki/Terminal-Compatibility.md (noting that Terminal.app and default iTerm2 profiles require enabling "Use Option as Meta key" for Alt+Enter).
2. Manual testing: Fully verified against a live Slack workspace:
   • Toggle on via Ctrl+O broadcasts reply to the parent channel (thread_broadcast).
   • One-shot Alt+Enter successfully broadcasts without arming the toggle.
   • Verified non-stickiness (subsequent reply is a normal thread reply).
   • Verified toggle resets when switching to a different thread.
     All manual check items in the description are ticked.
3. Follow-up fix (66c9d04): During manual testing on narrower splits, noticed that when the broadcast hint wraps into a second line, the background fill and left border had a slight notch because the hint was rendered outside the compose box. Moved the hint inside the compose box wrapper so the left border and background tint flow uniformly across wrapped lines.