block_rich_text: add message_mention rich text section element#3
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds support for a new Slack rich text section element type, ChangesMessage Mention Element Support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| }, | ||
| }, | ||
| nil, | ||
| }, |
There was a problem hiding this comment.
Nit — no round-trip coverage for the new element: This case only asserts unmarshal. TestRichTextQuote_Marshal (below) round-trips text/emoji/preformatted payloads by marshalling and diffing back against the raw JSON — consider adding a symmetric case for RichTextSectionMessageMentionElement so future changes to the JSON tags (e.g. renaming message_ts or dropping omitempty on author_id) are caught.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
block_rich_text.go (1)
433-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStruct, constructor, and interface wiring look correct.
ChannelID/MessageTSare required fields (noomitempty) whileAuthorID/Text/URL/Styleare optional, matching the observedmessage_mentionpayload shape in the test. The constructor field order matches the struct declaration order.One minor style note:
RichTextSectionElementType()is defined with a value receiver (Line 445) while the switch inUnmarshalJSONconstructs&RichTextSectionMessageMentionElement{}(a pointer). This still satisfies theRichTextSectionElementinterface, since a pointer's method set includes value-receiver methods, so there's no functional issue — just worth confirming consistency with sibling element types' receiver style if you want uniformity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block_rich_text.go` around lines 433 - 460, The `RichTextSectionMessageMentionElement` wiring is correct, but its `RichTextSectionElementType()` method uses a value receiver while `UnmarshalJSON` returns a pointer, creating an inconsistency with sibling element types. Update the receiver style in `RichTextSectionMessageMentionElement` to match the other rich text element implementations so the interface and constructor usage stay uniform.block_rich_text_test.go (1)
220-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood test case; consider also covering the optional
Stylefield.The new case validates
ChannelID,MessageTS,AuthorID,Text, andURL, but doesn't exerciseStyle(the only field using a pointer type), which is the field most likely to have marshal/unmarshal edge cases (nil vs. populated).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@block_rich_text_test.go` around lines 220 - 236, The new rich text message mention test covers the required scalar fields, but it should also validate the optional Style pointer on RichTextSectionMessageMentionElement to exercise nil vs populated pointer handling. Update the test case in block_rich_text_test.go near the message_mention fixture so it includes a Style value and asserts it round-trips correctly through the unmarshalling path, using RichTextSectionMessageMentionElement and the existing rich text section test table.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@block_rich_text_test.go`:
- Around line 220-236: The new rich text message mention test covers the
required scalar fields, but it should also validate the optional Style pointer
on RichTextSectionMessageMentionElement to exercise nil vs populated pointer
handling. Update the test case in block_rich_text_test.go near the
message_mention fixture so it includes a Style value and asserts it round-trips
correctly through the unmarshalling path, using
RichTextSectionMessageMentionElement and the existing rich text section test
table.
In `@block_rich_text.go`:
- Around line 433-460: The `RichTextSectionMessageMentionElement` wiring is
correct, but its `RichTextSectionElementType()` method uses a value receiver
while `UnmarshalJSON` returns a pointer, creating an inconsistency with sibling
element types. Update the receiver style in
`RichTextSectionMessageMentionElement` to match the other rich text element
implementations so the interface and constructor usage stay uniform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a8263b5-68d2-4c8a-85ce-50825bfbba83
📒 Files selected for processing (2)
block_rich_text.goblock_rich_text_test.go
📜 Review details
🔇 Additional comments (1)
block_rich_text.go (1)
241-242: LGTM!Also applies to: 306-316
|
2 participants
No description provided.