fix(ui): stop rendering the notification fallback text beside its blocks - #209
Conversation
|
Verified. Slack defines
Both renderers ( Build, vet, gofmt clean; 55 packages green under Merging. |
Conflict in AGENTS.md: both branches appended rows to the shared-code helpers table at the same position — this branch's RendersBody/ BlocksCarryBody row and main's DND/peerstatus rows (from gammons#219). Union: keep all three.
Problem
Bot messages that post Block Kit blocks render twice. Slack defines
textas a notification fallback wheneverblocksis present: it is what shows in a desktop notification or in a client that cannot render blocks, not a second copy of the body. slk draws the blocks, then also drawsmsg.Textas the body row, so a section-block digest prints once as blocks and once as its fallback string. It is very visible on any bot that posts a formatted digest.Fix
A new
blockkit.RendersBody(blocks)(internal/ui/messages/blockkit/render.go:43) reports whether the blocks carry the message's own content, andmessages.BlocksCarryBody(msg)(internal/ui/messages/model.go:599) wraps it for the renderers. When it is true,renderMessagePlain(internal/ui/messages/model.go:1949) andrenderThreadMessage(internal/ui/thread/model.go:1831) draw no body row at all (not an empty one), and leave that row out of the arithmetic that places sixel images and reaction hit rects (internal/ui/messages/model.go:2178,internal/ui/thread/model.go:2075).MessageTextSource(internal/ui/messages/model.go:583) is deliberately unchanged. It is also whatycopies (copyMessageOfSelected,internal/ui/app.go:1278), so suppressing the text there would make copying a bot message report "Message has no text". Hiding the fallback is a rendering decision, so it lives in the renderers.Some blocks deliberately do not count:
RichTextBlock, because the host renders it through the body row.appendBlockskips it for exactly that reason, so a rich_text body keeps its row even with a section block beside it.UnknownBlock, because all slk can draw for it is an "unsupported" marker. Slack's rule says the fallback is redundant; here it is the only readable thing left, so it is kept.DividerBlock, because a rule with no text beside it is not what the author wrote.Each case also checks that the block actually has content. An empty section is still a section, and suppressing the fallback against one would render the message blank, which is strictly worse than rendering it twice.
RendersBodymust agree with theappendBlockswitch beside it (internal/ui/messages/blockkit/render.go:74); a new content-bearing case belongs in both, and the function comment states that invariant.What this drops
Summary-only fallbacks. If a bot sends blocks that render and a
textthat says something the blocks do not, that text no longer appears on screen. That is the intended trade: per Slack's own contract that field is a notification string, and the alternative is printing every such message twice. Copying the message withystill yields it.Golden snapshots
The
deploybotfixture ingolden_test.gois exactly this case (Text: "build #421 green"plus a section block), so all eightinternal/ui/testdata/golden/*.ansifiles are regenerated.TestGoldenreports "rendered text differs" because thebuild #421 greenrow is gone and everything below it in that pane moves up one row. I checked the regenerated files with the escapes stripped: the fallback string appears in none of them, and the row under thedeploybotheader is the section block in every scenario.Tests
internal/ui/messages/blockkit/rendersbody_test.gocovers the predicate directly:TestRendersBody_ContentBearingBlocks,TestRendersBody_NonBodyBlocks,TestRendersBody_RichTextDoesNotCount.internal/ui/messages/blockkit_integration_test.go:TestBlocksCarryBody_ContentBlocks,TestBlocksCarryBody_KeepsBodyRow(no blocks, empty section, empty header, divider only, unknown only) andTestBlocksCarryBody_RichTextBodyKeepsBodyRow.TestBuildCache_BlocksCarryBodyReactionHitRowpins the row arithmetic: the reaction hit rect lands on the reaction line.TestRenderMessagePlainEmitsBlockKitContentused to assert the fallback is drawn; it now asserts it is not, and that the row under the header is the block content rather than an empty body row.TestMessageTextSource_NonRichTextBlocksReturnRawTextkeeps its assertion. Only its comment changes, to say that whethermsg.Textgets a body row is nowBlocksCarryBody's call.internal/ui/thread/render_test.go:TestRenderThreadMessageBlocksCarryBodycovers the thread pane (fallback not drawn, no empty row, reaction hit on the reaction line).internal/ui/copy_message_test.go:TestCopyMessage_BlockKitBodyStillCopiesText.Test plan
Each part of the fix was reverted on its own to confirm a test catches it:
TestRenderMessagePlainEmitsBlockKitContentandTestBuildCache_BlocksCarryBodyReactionHitRow.TestBuildCache_BlocksCarryBodyReactionHitRow.TestRenderThreadMessageBlocksCarryBody.TestRenderThreadMessageBlocksCarryBody(reaction hit row = 3, want 2).MessageTextSourceinstead failsTestCopyMessage_BlockKitBodyStillCopiesTextandTestMessageTextSource_NonRichTextBlocksReturnRawText.BlocksCarryBodyalways false (the original duplication) failsTestBlocksCarryBody_ContentBlocks,TestRenderMessagePlainEmitsBlockKitContentandTestRenderThreadMessageBlocksCarryBody.Also:
gofmt -l .clean,go build -ldflags="-s -w" -trimpath ./...,go vet ./...,go test -count=1 ./...,go test -race ./...,golangci-lint run ./...(v2.13.1, 0 issues).git diff --checkis clean outside the golden.ansifiles, where trailing whitespace is part of the terminal snapshot (the commit that added them trips it the same way). Rebased onto current main.