[ECO-5157] [ECO-5158] Extends example app to include edit and delete functionality #228
Conversation
|
Warning Rate limit exceeded@umair-ably has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
WalkthroughThis pull request restructures the chat interface in the AblyChat example. The main view now uses a unified Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CV as ContentView
participant MB as MenuButtonView
participant API as MockMessages (Backend)
U->>CV: Interact with chat (select message action)
CV->>MB: Present options (Edit/Delete)
alt Edit Message Flow
U->>CV: Choose "Edit"
CV->>CV: Set editingItemID<br/>Prepare new message content
CV->>API: Call sendEditedMessage() asynchronously
API-->>CV: Return updated message
else Delete Message Flow
U->>CV: Choose "Delete"
CV->>API: Call deleteMessage() asynchronously
API-->>CV: Return deletion confirmation
end
CV->>CV: Update listItems array
CV->>U: Render updated chat view
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/228/AblyChat
![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
🧹 Nitpick comments (12)
Example/AblyChatExample/MessageViews/MessageView.swift (1)
1-51: Effective implementation of message view with edit/delete capabilities.The MessageView successfully implements the edit and delete functionality required by the PR. The view shows appropriate visual feedback for edited messages and only displays the edit/delete menu for messages sent by the current user. The confirmation dialog for deletion is a good UX practice to prevent accidental deletions.
I have two minor suggestions for consideration:
- Consider using a semantic color for the editing background (like
.yellow.opacity(0.12)or a system color) rather than.orange.opacity(0.12)for better consistency with iOS design conventions.- The confirmation dialog message could be more specific, e.g., "This message will be permanently deleted."
Example/AblyChatExample/MessageViews/PresenceMessageView.swift (1)
24-29: Optional forced unwrap is logically safe, but consider a safer approach.
Since you dostatus != nilbefore usingstatus!, it's technically safe. However, using optional binding (e.g.,if let status = status) can be clearer and avoids force-unwrapping.-let presenceMessage = status != nil ? "\(clientPresenceChangeMessage) with status: \(status!)" : clientPresenceChangeMessage +if let unwrappedStatus = status { + return "\(clientPresenceChangeMessage) with status: \(unwrappedStatus)" +} else { + return clientPresenceChangeMessage +}Example/AblyChatExample/Mocks/MockClients.swift (2)
147-163: Potential refinement for message timestamps.
Currently, the updated message reassignscreatedAttoDate(). If you'd like to preserve the original creation time, consider storing an additionalupdatedAtproperty. Also note thatdescriptionandmetadataparameters are unused.
165-181: Clarify data handling when deleting messages.
The deletion logic creates a new message object with the old text and resetscreatedAttoDate(). If you intend to remove or mask the text for a deleted message, consider setting it to an empty string or a placeholder.- text: message.text, + text: "",Example/AblyChatExample/ContentView.swift (8)
80-87: Verify the priority of conditions.
WhennewMessageis empty, you default to sending a reaction rather than checking for edit mode. This might be intentional, but verify if editing an empty text is ever needed.
90-91: Deletion logic is sufficient but lacks error handling.
Currently, you discard the returned message. Consider logging or handling any errors fromtry awaitcalls for robustness.
107-134: Parallel subscriptions and.flip()usage.
The logic for displaying messages vs. deleted messages is clear. If you expect frequent deletions, consider error or concurrency handling on theonMessageDelete..flip()is an interesting approach—just ensure it’s consistent with your overall UX design.
138-154: Possible debouncing of typing events.
Repeatedly spawningTaskcalls inonChange(of:)might lead to frequent updates. If performance becomes an issue, consider debouncing or throttling to reduce call frequency.
170-176: Add accessibility label for edit-cancel button.
While the empty label with a system image is functional, adding anaccessibilityLabel("Cancel Editing")or similar would improve accessibility.
308-315: Frequent presence events can clutter chat.
Insertion at the top may push existing content down rapidly if presence changes are frequent. Consider grouping or limiting presence events if needed.
395-410: Consider capturing errors on edit.
Discarding the returned updated message is fine if you don’t need it, but you might want to handle or log potential errors from the update call for better debugging.
526-529: View flip extension.
This is a creative approach to reversing list content. Check if a simpler approach, such as a reversedForEach, might be clearer for chat ordering, especially regarding scroll-to-latest requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Example/AblyChatExample/ContentView.swift(9 hunks)Example/AblyChatExample/MessageViews/DeletedMessageView.swift(1 hunks)Example/AblyChatExample/MessageViews/MenuButtonView.swift(1 hunks)Example/AblyChatExample/MessageViews/MessageView.swift(1 hunks)Example/AblyChatExample/MessageViews/PresenceMessageView.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Sources/AblyChat/Message.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM,
releaseconfiguration (Xcode 16)
🔇 Additional comments (15)
Example/AblyChatExample/MessageViews/DeletedMessageView.swift (1)
1-23: Good implementation of deleted message visualization.The DeletedMessageView properly displays a deleted message in the chat interface, showing the sender's ID and a clear message indicating deletion. The styling is consistent with other message views and the conditional compilation for non-tvOS platforms is appropriate.
Example/AblyChatExample/MessageViews/MenuButtonView.swift (1)
1-20: Well-designed reusable menu component.This MenuButtonView effectively implements the edit and delete functionality required by the PR objectives. The use of SF Symbols for icons, proper destructive styling for the delete action, and encapsulation of the menu functionality makes this a clean, reusable component.
Sources/AblyChat/Message.swift (1)
147-151: Good addition of a convenient initializer.This new initializer for MessageOperation with default parameter values simplifies the creation of operation objects when editing or deleting messages, supporting the PR's functionality objectives.
Example/AblyChatExample/MessageViews/MessageView.swift (1)
53-56: Good encapsulation of message data with sender status.The MessageListItem struct effectively combines the message with information about whether the current user is the sender, which is crucial for determining edit/delete permissions.
Example/AblyChatExample/MessageViews/PresenceMessageView.swift (4)
1-2: Imports look good.
SwiftUI and AblyChat are essential for view rendering and chat functionality, respectively. No issues here.
4-5: Struct definition appears consistent.
DefiningPresenceMessageViewwith aPresenceListItemproperty is clear and maintainable. Great job setting up the view's data model explicitly.
7-22: Straightforward SwiftUI layout.
Your layout structure is clear, placing a “System:” label in oneVStackand the presence info in another. The#if !os(tvOS)guard for.listRowSeparator(.hidden)is appropriate for platform-specific styling.
32-34: Struct usage is succinct.
PresenceListItemcleanly wraps aPresenceEvent. If you don’t need mutability, considerlet presence: PresenceEventinstead ofvar presence: PresenceEvent.Example/AblyChatExample/Mocks/MockClients.swift (1)
211-211: Adjusting refresh interval.
Raising the minimum random interval from 0.1 to 0.3 can reduce rapid-fire subscription events, which is beneficial for performance. The current range is fine.Example/AblyChatExample/ContentView.swift (6)
55-56: New state variables.
listItemsandeditingItemIDare well-named. EnsureeditingItemIDis reset appropriately to avoid lingering edit states.
58-70: Check uniqueness of ID.
Usingmessage.idandpresence.timestamp.descriptionfor theidmight cause collisions if multiple presence events share the same timestamp. Consider appending a UUID or a global counter for guaranteed uniqueness.
225-229: Editing workflow looks good.
When the user taps the send button while editing, the flow triggerssendEditedMessage()and resetseditingItemID. This is a straightforward approach.
250-255: Explicit handling of all message actions in the switch statement.
Looks neat. Ensure no additional actions (like.noneor.unknown) exist in the domain model. If they do, consider adding a default clause.
260-285: Real-time message updates appear well-managed.
You insert.createat the top of the list and update existing items on.updateor.delete. This approach is intuitive. Double-check concurrency usage to avoid data races in SwiftUI states.
428-432: Typing logic is straightforward.
Stopping or starting typing updates is consistent withnewMessage. If multi-user concurrency or typed messages are large, consider partial updates or debouncing to reduce frequent calls.
umair-ably
force-pushed
the
ECO-5157-5158-demo
branch
from
83ef89a to
6a41b58
Compare
github-actions
bot
temporarily deployed
to
staging/pull/228/AblyChat
…k` and `.live` mode
umair-ably
force-pushed
the
ECO-5157-5158-demo
branch
from
6a41b58 to
d8da468
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Example/AblyChatExample/Mocks/MockClients.swift (1)
211-211: Consider documenting the reason for the interval change.The reaction interval has been increased from what was likely
0.1 ... 0.5to0.3 ... 0.6. This slows down the frequency of mock reactions, which might be intentional for better UX, but lacks a comment explaining why.Example/AblyChatExample/ContentView.swift (4)
89-91: Add error handling for message deletion.The
onMessageDeletemethod discards the result of the delete operation and doesn't handle potential errors that could occur. Consider showing an error message to the user if deletion fails.func onMessageDelete(message: Message) async throws { - _ = try await room().messages.delete(message: message, params: .init()) + do { + _ = try await room().messages.delete(message: message, params: .init()) + } catch { + print("Failed to delete message: \(error)") + // Consider showing an error message to the user + } }
394-410: Consider adding specific message ID parameter to sendEditedMessage.The current implementation of
sendEditedMessagerequires searching through all list items to find the message being edited. Consider refactoring to accept a specific message ID parameter for better clarity and maintainability.-func sendEditedMessage() async throws { +func sendEditedMessage(messageID: String) async throws { guard !newMessage.isEmpty else { return } - if let editingMessageItem = listItems.compactMap({ listItem -> MessageListItem? in - if case let .message(message) = listItem, message.message.id == editingItemID { + if let editingMessageItem = listItems.compactMap({ listItem -> MessageListItem? in + if case let .message(message) = listItem, message.message.id == messageID { return message } return nil }).first { let editedMessage = editingMessageItem.message.copy(text: newMessage) _ = try await room().messages.update(newMessage: editedMessage, description: nil, metadata: nil) } newMessage = "" }Then in
sendButtonAction:} else if editingItemID != nil { Task { - try await sendEditedMessage() + if let id = editingItemID { + try await sendEditedMessage(messageID: id) + } editingItemID = nil } }
245-286: Consider adding pagination for older messages.The current implementation loads previous messages but doesn't provide a way for users to fetch older messages beyond the initial load. Consider adding pagination functionality for better user experience with longer conversation histories.
515-518: Consider explaining the purpose of the flip method.The newly added
flip()method flips views horizontally and vertically, but its purpose isn't immediately clear from the name or implementation. Consider adding a comment explaining its use in the chat UI context (likely to create bottom-to-top scrolling).+/// Flips the view horizontally and vertically to create a bottom-to-top scrolling effect for chat messages func flip() -> some View { rotationEffect(.radians(.pi)) .scaleEffect(x: -1, y: 1, anchor: .center) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Example/AblyChatExample/ContentView.swift(9 hunks)Example/AblyChatExample/MessageViews/DeletedMessageView.swift(1 hunks)Example/AblyChatExample/MessageViews/MenuButtonView.swift(1 hunks)Example/AblyChatExample/MessageViews/MessageView.swift(1 hunks)Example/AblyChatExample/MessageViews/PresenceMessageView.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Sources/AblyChat/Message.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/AblyChat/Message.swift
- Example/AblyChatExample/MessageViews/DeletedMessageView.swift
- Example/AblyChatExample/MessageViews/MenuButtonView.swift
- Example/AblyChatExample/MessageViews/PresenceMessageView.swift
- Example/AblyChatExample/MessageViews/MessageView.swift
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
🔇 Additional comments (6)
Example/AblyChatExample/Mocks/MockClients.swift (2)
147-163: Strong implementation of the update method.The update method now properly implements message updating functionality by creating a new Message object with the
.updateaction. This completes the mock implementation that previously raised a fatal error.
165-181: Well-implemented delete functionality.The delete method correctly creates a new Message with the
.deleteaction while preserving the original message's attributes. This implementation completes the mock client's functionality and matches the real behavior expected in the application.Example/AblyChatExample/ContentView.swift (4)
58-70: Good use of enum for unified list representation.The
ListItemenum elegantly unifies both message and presence items in a single list, making the UI more maintainable. The computedidproperty properly handles identification for both cases.
107-133: Well-structured message display with editing capability.The list implementation effectively handles different types of messages (regular, deleted) and presence notifications with appropriate views. The editing binding is also well-designed, allowing seamless state management.
169-176: Great attention to UX with cancel button.The cancel button for editing provides a clear way for users to exit edit mode. The use of animation and styling makes the interaction intuitive.
250-287: Deleting the first previous message is unusual.The current message deletion implementation doesn't provide a way to specify which message to delete - it would only delete specific messages found through UI interactions. Consider adding a parameter to allow deleting specific messages.
.mockmodemock.demo.updated.mov
.livemodechat.demo.mov
Summary by CodeRabbit
New Features
Bug Fixes