Repository navigation
Remove divider from the message actions - #1549
Conversation
📝 WalkthroughWalkthroughThe message action menu now renders a divider only before a trailing group of destructive actions. Snapshot tests cover single-destructive, grouped-destructive, and non-destructive layouts, and the changelog documents the behavior. ChangesMessage action menu
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| Divider() |
There was a problem hiding this comment.
According to Figma, it looks like destructive actions should have the divider, and should be at the end 🤔
There was a problem hiding this comment.
updated now
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@StreamChatSwiftUI.xcodeproj/project.pbxproj`:
- Line 200: Remove Tests/ChatChannel/MessageActionsView_Tests.swift from the
membershipExceptions section in the Xcode project configuration, and register it
with the StreamChatSwiftUITests target using Xcode’s target membership/build
phase settings instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a13fe99-beef-498e-bf73-59cf55da7d8c
⛔ Files ignored due to path filters (19)
StreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsLast_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsLast_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsLast_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsLast_snapshot.small-dark.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsSeparated_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsSeparated_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsSeparated_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsSeparated_snapshot.small-dark.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_noDestructiveActions_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_noDestructiveActions_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_noDestructiveActions_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_noDestructiveActions_snapshot.small-dark.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_onlyDestructiveActions_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_onlyDestructiveActions_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_onlyDestructiveActions_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_onlyDestructiveActions_snapshot.small-dark.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/ReactionsOverlayView_Tests/test_reactionsOverlayView_noReactions.1.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/ReactionsOverlayView_Tests/test_reactionsOverlayView_snapshot.1.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/ReactionsOverlayView_Tests/test_reactionsOverlayView_translated.1.pngis excluded by!**/*.png
📒 Files selected for processing (4)
CHANGELOG.mdSources/StreamChatSwiftUI/ChatMessageList/Reactions/MessageActions/MessageActionsView.swiftStreamChatSwiftUI.xcodeproj/project.pbxprojStreamChatSwiftUITests/Tests/ChatChannel/MessageActionsView_Tests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
nuno-vieira
left a comment
nuno-vieira
left a comment
There was a problem hiding this comment.
Looks good, could be improved in terms of customiziblity, but not a blocker
| /// The divider goes in front of the trailing run of destructive actions, and is skipped | ||
| /// when there is nothing above it to separate from. | ||
| private var actionRows: [ActionRow] { | ||
| let actions = viewModel.messageActions | ||
| var dividerIndex: Int? | ||
| if let lastRegularIndex = actions.lastIndex(where: { !$0.isDestructive }) { | ||
| let destructiveGroupIndex = lastRegularIndex + 1 | ||
| dividerIndex = destructiveGroupIndex < actions.count ? destructiveGroupIndex : nil | ||
| } | ||
|
|
||
| return actions.enumerated().map { index, action in | ||
| ActionRow(action: action, showsDivider: index == dividerIndex) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This works fine, but not very customizable. Would be better if this either stayed on the MessageActionsViewModel or the ChannelInfoViewModel. Since the latter is the one responsible for creating the actions, it would probably make sense to extend the MessageAction with showsDivider, and the info view model would make the logic to show or not show the divider.
An even better approach would be to have a MessageActionItemViewModel so that the UI logic would be decoupled from the actual MessageAction. But I believe this would require some breaking changes, most likely.
Either way, none are blocking at the moment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
6-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a level-2 heading here.
### Addedskips from# Upcomingto level 3, triggering markdownlint MD001. Change it to## Added.🤖 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 `@CHANGELOG.md` at line 6, Update the “Added” heading in the changelog from level 3 to level 2 so it follows the “Upcoming” level-1 heading and satisfies markdown heading hierarchy.Source: Linters/SAST tools
♻️ Duplicate comments (1)
StreamChatSwiftUI.xcodeproj/project.pbxproj (1)
206-206: 🎯 Functional Correctness | 🟠 MajorRemove the test from
membershipExceptions.This exclusion prevents
Tests/ChatChannel/MessageActionsView_Tests.swiftfrom being compiled or discovered byStreamChatSwiftUITests. Remove the entry and register the file through the synchronized target membership instead.As per coding guidelines, source files must be added to the correct Xcode target and project membership.
🤖 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 `@StreamChatSwiftUI.xcodeproj/project.pbxproj` at line 206, Remove Tests/ChatChannel/MessageActionsView_Tests.swift from the membershipExceptions configuration in the Xcode project, then add it through the synchronized StreamChatSwiftUITests target membership so the test is compiled and discovered normally.Source: Coding guidelines
🧹 Nitpick comments (1)
StreamChatSwiftUITests/Tests/ChatChannel/MessageActionsView_Tests.swift (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain coverage for an all-destructive menu.
The current fixtures cover trailing destructive actions and no destructive actions, but not the
lastIndex(where:) == nilbranch. Add a snapshot containing only destructive actions to verify that no divider is rendered when there is nothing above the group.As per coding guidelines, maintain high test coverage for changed SwiftUI behavior.
🤖 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 `@StreamChatSwiftUITests/Tests/ChatChannel/MessageActionsView_Tests.swift` around lines 34 - 39, Extend test_messageActionsView_destructiveActionsGroup_snapshot with a separate snapshot fixture containing only destructive actions, exercising the lastIndex(where:) == nil path. Assert the rendered menu has no divider above the destructive group, while preserving the existing snapshots for trailing and non-destructive actions.Source: Coding guidelines
🤖 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.
Inline comments:
In
`@Sources/StreamChatSwiftUI/ChatMessageList/Reactions/MessageActions/MessageActionsView.swift`:
- Around line 92-94: Convert the `///` comments immediately above the private
`actionRows` property to `//` comments, preserving their wording and placement
as an internal implementation note.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Line 6: Update the “Added” heading in the changelog from level 3 to level 2 so
it follows the “Upcoming” level-1 heading and satisfies markdown heading
hierarchy.
---
Duplicate comments:
In `@StreamChatSwiftUI.xcodeproj/project.pbxproj`:
- Line 206: Remove Tests/ChatChannel/MessageActionsView_Tests.swift from the
membershipExceptions configuration in the Xcode project, then add it through the
synchronized StreamChatSwiftUITests target membership so the test is compiled
and discovered normally.
---
Nitpick comments:
In `@StreamChatSwiftUITests/Tests/ChatChannel/MessageActionsView_Tests.swift`:
- Around line 34-39: Extend
test_messageActionsView_destructiveActionsGroup_snapshot with a separate
snapshot fixture containing only destructive actions, exercising the
lastIndex(where:) == nil path. Assert the rendered menu has no divider above the
destructive group, while preserving the existing snapshots for trailing and
non-destructive actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b41318d-5b7f-4834-a5fd-fc47d63f4523
⛔ Files ignored due to path filters (8)
StreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsGroup_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsGroup_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsGroup_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_destructiveActionsGroup_snapshot.small-dark.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_singleDestructiveAction_snapshot.default-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_singleDestructiveAction_snapshot.extraExtraExtraLarge-light.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_singleDestructiveAction_snapshot.rightToLeftLayout-default.pngis excluded by!**/*.pngStreamChatSwiftUITests/Tests/ChatChannel/__Snapshots__/MessageActionsView_Tests/test_messageActionsView_singleDestructiveAction_snapshot.small-dark.pngis excluded by!**/*.png
📒 Files selected for processing (4)
CHANGELOG.mdSources/StreamChatSwiftUI/ChatMessageList/Reactions/MessageActions/MessageActionsView.swiftStreamChatSwiftUI.xcodeproj/project.pbxprojStreamChatSwiftUITests/Tests/ChatChannel/MessageActionsView_Tests.swift
| /// The divider goes in front of the trailing run of destructive actions, and is skipped | ||
| /// when there is nothing above it to separate from. | ||
| private var actionRows: [ActionRow] { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use // for this private implementation note.
This is documentation for a private property, not a public SDK declaration. Convert the /// comments to // comments.
As per coding guidelines, use /// only for public SDK declarations and inline comments for non-obvious internal reasoning.
Proposed fix
- /// The divider goes in front of the trailing run of destructive actions, and is skipped
- /// when there is nothing above it to separate from.
+ // The divider goes in front of the trailing run of destructive actions, and is skipped
+ // when there is nothing above it to separate from.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// The divider goes in front of the trailing run of destructive actions, and is skipped | |
| /// when there is nothing above it to separate from. | |
| private var actionRows: [ActionRow] { | |
| // The divider goes in front of the trailing run of destructive actions, and is skipped | |
| // when there is nothing above it to separate from. | |
| private var actionRows: [ActionRow] { |
🤖 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
`@Sources/StreamChatSwiftUI/ChatMessageList/Reactions/MessageActions/MessageActionsView.swift`
around lines 92 - 94, Convert the `///` comments immediately above the private
`actionRows` property to `//` comments, preserving their wording and placement
as an internal implementation note.
Source: Coding guidelines
Public Interface🚀 No changes affecting the public interface. |
SDK Size
|
StreamChatSwiftUI XCSize
|
|
4 participants
🔗 Issue Links
Resolves https://linear.app/stream/issue/IOS-1917/v5-ui-polish-message-action-menu-shows-separators-between-every-item.
🎯 Goal
Remove divider from the message actions per the design.
📝 Summary
Provide bullet points with the most important changes in the codebase.
🛠 Implementation
Provide a detailed description of the implementation and explain your decisions if you find them relevant.
🎨 Showcase
Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.
🧪 Manual Testing Notes
Explain how this change can be tested manually, if applicable.
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit