fix(analytics): cover local firmware sideloads and report message_send in the foreground - #6660
Merged
jamesarich merged 2 commits intoAug 12, 2026
Conversation
…d in the foreground CodeRabbit review of meshtastic#6654: - confirmLocalFirmwareFile -> startUpdateFromFile is a second entry point for starting a flash and never emitted firmware_update_start. Both paths now go through a shared trackUpdateStart helper; the local path reports release_version=local. - Name the analytics mocks in the firmware fixtures and assert the action, so the suites can no longer pass without observing it. Also move message_send from MessagingControllerImpl to SendMessageUseCase. Text sends are enqueued and transmitted by a WorkManager worker, so the action was firing off the user's RUM session (possibly after it expired) and re-firing on retry. Every text-send path routes through the use case, so coverage is unchanged. Waypoints stay in the controller, which is where the map's direct sendMessage call lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds analytics tracking for sent messages and firmware update starts. It injects ChangesAnalytics event tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MessagingControllerImpl
participant SendMessageUseCaseImpl
participant PlatformAnalytics
MessagingControllerImpl->>SendMessageUseCaseImpl: Send text message
SendMessageUseCaseImpl->>PlatformAnalytics: Track message_send with byte length and reply status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
jamesarich
enabled auto-merge
August 12, 2026 19:58
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/usecase/SendMessageUseCase.kt`:
- Around line 135-140: Move the analytics.trackAction call out of the try/catch
covering savePacket and enqueue in SendMessageUseCase, then execute it with
safeCatching {} while preserving coroutine cancellation and logging failures as
warnings. Ensure analytics exceptions never alter the successful send result or
trigger the enqueue retry path.
- Line 139: Update the num_bytes value in SendMessageUseCase’s analytics map to
use finalMessageText.encodeToByteArray().size instead of character count,
matching DataPacket byte storage and MessagingControllerImpl reporting. Add a
test covering non-ASCII message text to verify the UTF-8 byte count.
In
`@core/repository/src/commonTest/kotlin/org/meshtastic/core/repository/usecase/SendMessageUseCaseTest.kt`:
- Around line 83-95: Update the analytics assertion in `invoke reports a
message_send analytics action` to use Mokkery verification with `exactly(1)`,
ensuring the `message_send` action is emitted once while preserving the existing
arguments.
In
`@feature/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt`:
- Around line 222-235: Strengthen the test `confirmLocalFirmwareFile with BLE
and invalid address reports no analytics action` by asserting the ViewModel
reaches `FirmwareUpdateState.Error` after confirmation and verifying
`firmwareUpdateManager.startUpdate` is never called. Keep the existing zero-call
analytics assertion so the test proves BLE validation rejected the update rather
than an earlier setup failure or early return.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 069b7735-d3a7-4cf1-b2ac-5380a1b46b8c
📒 Files selected for processing (8)
core/repository/src/commonMain/kotlin/org/meshtastic/core/repository/di/CoreRepositoryModule.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/usecase/SendMessageUseCase.ktcore/repository/src/commonTest/kotlin/org/meshtastic/core/repository/usecase/SendMessageUseCaseTest.ktcore/service/src/commonMain/kotlin/org/meshtastic/core/service/MessagingControllerImpl.ktfeature/firmware/src/commonMain/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModel.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateIntegrationTest.ktfeature/firmware/src/commonTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelTest.ktfeature/firmware/src/jvmTest/kotlin/org/meshtastic/feature/firmware/FirmwareUpdateViewModelFileTest.kt
CodeRabbit review of meshtastic#6660: - trackAction ran inside the try guarding savePacket/enqueue. A telemetry failure after a successful enqueue was logged as 'Failed to enqueue' and rethrown, so a caller retry could queue a second copy of the message. It now runs outside that boundary via safeCatching, which preserves cancellation, and logs a warning instead. - num_bytes counted characters while MessagingControllerImpl reports bytes.size, so the same attribute meant two different things. Use the UTF-8 byte count, covered by a non-ASCII test. - Mokkery's default verify mode is soft and passes on duplicate calls, so the assertion could not catch the double-fire this change prevents. Use exactly(1). - The invalid-BLE-address test also passed if preparation bailed out earlier; assert the Error state and that startUpdate was never called. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Follow-up to #6654, which merged before its review feedback was applied. Two gaps in the named-RUM-action coverage that shipped:
Local firmware sideloads are invisible.
firmware_update_startis only emitted fromstartUpdate(). Picking a firmware file from disk goes throughconfirmLocalFirmwareFile()→startUpdateFromFile(), a completely separate entry point that emits nothing. Firmware-update counts in RUM currently undercount, showing release-channel updates only. (Caught by CodeRabbit on feat(analytics): report key user interactions as named RUM actions #6654.)message_sendfires outside the user's session. Text sends are not transmitted synchronously —SendMessageUseCaseenqueues, and a WorkManagerSendMessageWorkerperforms the actualradioController.sendMessage. Emitting the action there means it lands on a background thread, potentially long after the RUM session ended, and re-fires whenever the worker retries.🐛 Fixes
Cover both firmware entry points. Extracted
trackUpdateStart(state, releaseId)and call it fromstartUpdate()andstartUpdateFromFile(). The local path reports the existingLOCAL_RELEASE_ID("local") asrelease_version, so file-based flashes stay distinguishable from release updates. The call sits after the BLE address-validation guard, so a rejected start is not counted.Report
message_sendwhere the user acted. Moved toSendMessageUseCase.invoke. Coverage is unchanged — every text-send path (messaging UI, notification quick-reply, Android Auto, AI function-calling) routes through this use case. Removed theTEXT_MESSAGE_APPbranch fromMessagingControllerImplso messages are not counted twice.waypoint_sendstays in the controller, sinceBaseMapViewModelcallsradioController.sendMessagedirectly and never touches the use case.🧹 Tests
The firmware fixtures passed an anonymous autofilled mock for the new constructor parameter, so they could pass without ever observing
trackAction. Named the mock in all four fixtures and added assertions:startUpdatereportsfirmware_update_startwith the release versionconfirmLocalFirmwareFilereports it withrelease_version=local— the newly covered pathSendMessageUseCase.invokereportsmessage_sendTesting Performed
spotlessCheck detekt assembleDebug :core:repository:allTests :core:service:allTests :feature:firmware:allTestson this branch's base — BUILD SUCCESSFUL in 2m 24s (Gradle exit 0), with all six analytics assertions confirmed executing and passing rather than replayed from cache.🤖 Generated with Claude Code
Summary by CodeRabbit