Align message delivery status wording#2054
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR standardizes iOS message delivery statuses. It adds a public-key routing error, centralizes ACK and retry presentation, updates message rows and retry handling, and refreshes help content, tests, and documentation. ChangesMessage delivery status standardization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant MessageRow
participant MessageEntity
participant RoutingError
participant MessageDeliveryStatus
participant RetryButton
MessageRow->>MessageEntity: deliveryStatus(isDirectMessage:)
MessageEntity->>RoutingError: parse ackError
RoutingError-->>MessageEntity: display, description, canRetry
MessageEntity->>MessageDeliveryStatus: build status
MessageRow->>RetryButton: show when status.canRetry
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MeshtasticTests/RoutingAndMiscTests.swift (1)
49-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale/weak assertion left over from the
.none.displayrename.This PR renames
RoutingError.none.displayto"Delivered to recipient", butdisplay_noneIsAcknowledgedstill passes via.contains("Acknowledged") || .count > 0— a condition that's true regardless of the actual string.EnumTests.swiftalready has the correct assertion for this (== "Delivered to recipient"); consider tightening or removing this duplicate/misleading test.🧹 Proposed fix
- `@Test` func display_noneIsAcknowledged() { - `#expect`(RoutingError.none.display.contains("Acknowledged") || RoutingError.none.display.count > 0) - } + `@Test` func display_noneIsDeliveredToRecipient() { + `#expect`(RoutingError.none.display == "Delivered to recipient") + }🤖 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 `@MeshtasticTests/RoutingAndMiscTests.swift` around lines 49 - 51, The display_noneIsAcknowledged test in RoutingAndMiscTests is stale and too weak because it always passes, so update it to assert the exact new RoutingError.none.display value or remove it as redundant. Use the RoutingError.none.display symbol and align the expectation with the existing EnumTests.swift check for "Delivered to recipient" so the test verifies the rename precisely.
🧹 Nitpick comments (3)
Meshtastic/Extensions/SwiftData/MessageEntityExtension.swift (1)
14-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelocate
MessageDeliveryStatus/MessageDeliveryStatusLabelout ofMessageEntityExtension.swift.These are standalone types, not
MessageEntityextensions, and don't match this file's name. As per coding guidelines, "Make file names match the primary type they define" and "Group extensions by functionality in separate files, such asAccessoryManager+ToRadio.swift." Consider moving these into a dedicatedMessageDeliveryStatus.swift.🤖 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 `@Meshtastic/Extensions/SwiftData/MessageEntityExtension.swift` around lines 14 - 81, The issue is that MessageDeliveryStatus and MessageDeliveryStatusLabel are standalone types living in a file named like an extension file, which violates the file/type organization guidelines. Move these types out of MessageEntityExtension.swift into a dedicated MessageDeliveryStatus.swift (or similarly named file), and leave MessageEntityExtension.swift for actual MessageEntity-related extensions only.Source: Coding guidelines
Meshtastic/Views/Messages/ChannelMessageRow.swift (1)
143-145: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCompute
deliveryStatusonce per row instead of twice.
message.deliveryStatus(isDirectMessage: false)is invoked separately for the retry gate (Line 143) and the label (Line 153). Since it's pure but non-trivial (RoutingError lookup + several localized strings), hoist it into a singleletfor clarity and to avoid duplicate work on every row render.♻️ Proposed consolidation
VStack(alignment: isCurrentUser ? .trailing : .leading) { let isDetectionSensorMessage = message.portNum == Int32(PortNum.detectionSensorApp.rawValue) + let deliveryStatus = message.deliveryStatus(isDirectMessage: false) ... HStack { MessageText(...) { ... } onTapback: { ... } - if isCurrentUser && message.deliveryStatus(isDirectMessage: false).canRetry { + if isCurrentUser && deliveryStatus.canRetry { RetryButton(message: message, destination: .channel(channel)) } } ... HStack { if isCurrentUser && !isDetectionSensorMessage { - MessageDeliveryStatusLabel(status: message.deliveryStatus(isDirectMessage: false)) + MessageDeliveryStatusLabel(status: deliveryStatus) } } }Also applies to: 150-154
🤖 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 `@Meshtastic/Views/Messages/ChannelMessageRow.swift` around lines 143 - 145, The row in ChannelMessageRow is calling message.deliveryStatus(isDirectMessage: false) more than once for the same render path, including the retry check and the status label. Hoist that result into a single local let inside the relevant row-building logic, then reuse the cached value for both the canRetry gate and the label in ChannelMessageRow so the delivery status is computed once per row.Meshtastic/Views/Messages/UserMessageRow.swift (1)
146-151: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSame duplicate
deliveryStatuscomputation asChannelMessageRow.swift.
message.deliveryStatus(isDirectMessage: true)is called once for the retry gate (Line 151) and again for the label (Line 161). Hoist into a singleletat the top ofrowContent(or right before thisHStack) and reuse.♻️ Proposed consolidation
VStack(alignment: isCurrentUser ? .trailing : .leading) { + let deliveryStatus = message.deliveryStatus(isDirectMessage: true) ... HStack { MessageText(...) { ... } onTapback: { ... } - if isCurrentUser && message.deliveryStatus(isDirectMessage: true).canRetry { + if isCurrentUser && deliveryStatus.canRetry { RetryButton(message: message, destination: .user(user)) } } ... HStack { if isCurrentUser { - MessageDeliveryStatusLabel(status: message.deliveryStatus(isDirectMessage: true)) + MessageDeliveryStatusLabel(status: deliveryStatus) } } }Also applies to: 158-163
🤖 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 `@Meshtastic/Views/Messages/UserMessageRow.swift` around lines 146 - 151, The row currently recomputes message.deliveryStatus(isDirectMessage: true) in UserMessageRow.rowContent for both the retry check and the status label, so hoist that value into a single local let near the start of rowContent (or just before the HStack) and reuse it for both the canRetry gate and the label rendering. Update the existing delivery-status references in UserMessageRow to use the cached value, matching the consolidation already intended in ChannelMessageRow.
🤖 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 `@Meshtastic/Views/Messages/MessageContextMenuItems.swift`:
- Around line 118-132: The MessageContextMenuItems branch logic is redundant and
applies fixedSize inconsistently. Consolidate the repeated
deliveryStatus(isDirectMessage:) lookup and
Text(status.text)/Text(status.detail) rendering into a single path, then apply
.fixedSize(horizontal: false, vertical: true) to both Text views consistently
for all current-user message states. Use the existing isCurrentUser,
message.receivedACK, message.ackError, and tapBackDestination.isDirectMessage
symbols to keep the behavior identical while preventing truncation in the Menu.
---
Outside diff comments:
In `@MeshtasticTests/RoutingAndMiscTests.swift`:
- Around line 49-51: The display_noneIsAcknowledged test in RoutingAndMiscTests
is stale and too weak because it always passes, so update it to assert the exact
new RoutingError.none.display value or remove it as redundant. Use the
RoutingError.none.display symbol and align the expectation with the existing
EnumTests.swift check for "Delivered to recipient" so the test verifies the
rename precisely.
---
Nitpick comments:
In `@Meshtastic/Extensions/SwiftData/MessageEntityExtension.swift`:
- Around line 14-81: The issue is that MessageDeliveryStatus and
MessageDeliveryStatusLabel are standalone types living in a file named like an
extension file, which violates the file/type organization guidelines. Move these
types out of MessageEntityExtension.swift into a dedicated
MessageDeliveryStatus.swift (or similarly named file), and leave
MessageEntityExtension.swift for actual MessageEntity-related extensions only.
In `@Meshtastic/Views/Messages/ChannelMessageRow.swift`:
- Around line 143-145: The row in ChannelMessageRow is calling
message.deliveryStatus(isDirectMessage: false) more than once for the same
render path, including the retry check and the status label. Hoist that result
into a single local let inside the relevant row-building logic, then reuse the
cached value for both the canRetry gate and the label in ChannelMessageRow so
the delivery status is computed once per row.
In `@Meshtastic/Views/Messages/UserMessageRow.swift`:
- Around line 146-151: The row currently recomputes
message.deliveryStatus(isDirectMessage: true) in UserMessageRow.rowContent for
both the retry check and the status label, so hoist that value into a single
local let near the start of rowContent (or just before the HStack) and reuse it
for both the canRetry gate and the label rendering. Update the existing
delivery-status references in UserMessageRow to use the cached value, matching
the consolidation already intended in ChannelMessageRow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5e3c1b0-66d5-4955-a569-0be97262082f
⛔ Files ignored due to path filters (1)
MeshtasticTests/__Snapshots__/SwiftUIViewSnapshotTests/messageDeliveryStatusStates.pngis excluded by!**/*.png
📒 Files selected for processing (10)
Meshtastic/Enums/RoutingError.swiftMeshtastic/Extensions/SwiftData/MessageEntityExtension.swiftMeshtastic/Views/Helpers/Help/AckErrors.swiftMeshtastic/Views/Messages/ChannelMessageRow.swiftMeshtastic/Views/Messages/MessageContextMenuItems.swiftMeshtastic/Views/Messages/UserMessageRow.swiftMeshtasticTests/EnumTests.swiftMeshtasticTests/MessageAckStatusRefreshTests.swiftMeshtasticTests/RoutingAndMiscTests.swiftMeshtasticTests/SwiftUIViewSnapshotTests.swift
|
Latest follow-up pushed: 4bd2651 Issue links:
Evidence paths at this head:
Validation:
Local swiftlint is unavailable here; relying on the GitHub SwiftLint check for that coverage. Full local suite was not run. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/user/messages.md (1)
115-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew status table drops the icon column used elsewhere in this doc.
Every other table in this file (Channel Index, Channel Security, Contacts, Encryption) uses an
Icon | Meaningpattern per the coding guidelines. This new table uses plainStatus | Descriptiontext with no icons, which is inconsistent with the rest of the page and with the guideline requiringreferences and an Icon column when documenting icon-backed states. Since the cohort description mentions AckErrors now shows "retry-aware icons," consider mirroring those icons here for consistency.
As per coding guidelines, "In markdown tables, usefor icon references and include at least| Icon | Description |columns."🤖 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 `@docs/user/messages.md` around lines 115 - 131, The new status table is inconsistent with the rest of the document because it omits the icon column used for icon-backed states. Update the table in the messages doc so it follows the same Icon/Description pattern as the other sections, using the relevant icon references for each status and matching the coding guideline format. Keep the content aligned with the existing status names by adjusting the table structure and entries in the status table itself.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 `@docs/user/messages.md`:
- Around line 113-131: Update the message-bubble error table in the docs so it
matches the UI color semantics: the retryable RoutingError statuses should be
explicitly marked as orange, or remove the “red unless noted” phrasing if you
are not annotating colors. Use the existing status rows in the messages table to
add the color note for entries like Failed to deliver to mesh, No radio
interface, Duty cycle limit, Rate limited, No app response, Could not send
encrypted message, Recipient needs your key, Recipient key unavailable, and
Admin session expired.
---
Nitpick comments:
In `@docs/user/messages.md`:
- Around line 115-131: The new status table is inconsistent with the rest of the
document because it omits the icon column used for icon-backed states. Update
the table in the messages doc so it follows the same Icon/Description pattern as
the other sections, using the relevant icon references for each status and
matching the coding guideline format. Keep the content aligned with the existing
status names by adjusting the table structure and entries in the status table
itself.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c840e67a-7a75-450f-8d8e-4500535a0ce3
📒 Files selected for processing (9)
Meshtastic/Enums/RoutingError.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/messages.mdMeshtastic/Resources/docs/markdown/user/settings.mdMeshtastic/Resources/docs/user/messages.htmlMeshtastic/Resources/docs/user/settings.htmlMeshtasticTests/EnumTests.swiftMeshtasticTests/RoutingAndMiscTests.swiftdocs/user/messages.md
✅ Files skipped from review due to trivial changes (2)
- Meshtastic/Resources/docs/user/settings.html
- Meshtastic/Resources/docs/markdown/user/settings.md
🚧 Files skipped from review as they are similar to previous changes (3)
- MeshtasticTests/EnumTests.swift
- MeshtasticTests/RoutingAndMiscTests.swift
- Meshtastic/Enums/RoutingError.swift
|
Follow-up pushed: 496645b Changes:
Validation:
Remaining local note: |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Follow-up pushed: e169b28 Changes:
Validation:
Real-device screenshot evidence:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
MeshtasticTests/RoutingAndMiscTests.swift (1)
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant test.
The test
description_noChannelUsesSourceBackedMeaningredundantly assertsRoutingError.noChannel.description, which is already validated identically inside theexpectedarray of thedescription_usesActionableDesignIssueWordingtest immediately below it.♻️ Proposed refactor
- `@Test` func description_noChannelUsesSourceBackedMeaning() { - `#expect`(RoutingError.noChannel.description == "The sender or recipient could not use a matching channel/key for this message.") - } -🤖 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 `@MeshtasticTests/RoutingAndMiscTests.swift` around lines 84 - 87, Remove the redundant description_noChannelUsesSourceBackedMeaning test, leaving the identical RoutingError.noChannel.description assertion covered by the expected array in description_usesActionableDesignIssueWording.
🤖 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 `@Meshtastic/Views/Messages/RetryButton.swift`:
- Around line 54-60: Implement the missing channel refresh inside the MainActor
Task in the .channel branch: bind or retrieve the associated channel entity and
trigger its objectWillChange (or refresh the relevant context) after sending, so
the view updates immediately. Preserve the existing user-destination behavior
and channel-specific flow.
---
Nitpick comments:
In `@MeshtasticTests/RoutingAndMiscTests.swift`:
- Around line 84-87: Remove the redundant
description_noChannelUsesSourceBackedMeaning test, leaving the identical
RoutingError.noChannel.description assertion covered by the expected array in
description_usesActionableDesignIssueWording.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5666e595-bf45-4114-9746-01d620fff6cf
📒 Files selected for processing (5)
Meshtastic/Enums/RoutingError.swiftMeshtastic/Views/Messages/ChannelMessageRow.swiftMeshtastic/Views/Messages/RetryButton.swiftMeshtastic/Views/Messages/UserMessageRow.swiftMeshtasticTests/RoutingAndMiscTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Meshtastic/Views/Messages/UserMessageRow.swift
- Meshtastic/Views/Messages/ChannelMessageRow.swift
|
Follow-up pushed: Addressed review feedback:
Validation:
Focused simulator result: 52 tests passed. |
garthvh
left a comment
garthvh
left a comment
There was a problem hiding this comment.
Wording verified against the merged Android counterpart (meshtastic/Meshtastic-Android#6121) earlier in this alignment effort. The routing-error semantics were sourced from firmware review per the meshtastic/design#43 thread. One known cross-platform naming inconsistency to track separately: Android's no_channel string says 'No channel selected' while iOS says 'Channel/key mismatch'.
2 participants
Summary
Evidence
docs/assets/screenshots/ackErrors.pngMeshtastic/Resources/docs/assets/screenshots/ackErrors.pngValidation
git diff --checkpassed.git diff --cached --checkpassed before commit.bash scripts/build-docs.sh --output Meshtastic/Resources/docspassed; bundle was 7.9 MB / 10 MB hard limit.bash scripts/copy-snapshots.sh --output Meshtastic/Resources/docs/assets/screenshotscopied 98 doc-referenced screenshots; the helper cleanup removed one unrelated orphan locally, which was restored before commit.test_simpassed: 53 tests, 0 failures.MeshtasticTests/RoutingErrorTests,MeshtasticTests/RoutingErrorDetailedTests,MeshtasticTests/MessageAckStatusRefreshTests,MeshtasticTests/AckErrorsSnapshotTests./Users/benjaminfaershtein/Library/Developer/XcodeBuildMCP/workspaces/Meshtastic-Apple-Fork-3-a9b3dcc49c40/result-bundles/test_sim_2026-07-07T18-29-52-872Z_pid78002_f6419b45.xcresultswiftlintcommand is unavailable in this environment; the GitHub SwiftLint check is expected to cover it.Links
Closes #2051
Summary by CodeRabbit