Align Base UI message status wording#10921
Draft
RCGV1 wants to merge 12 commits into
Draft
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared message-status tracking and rendering across message storage, routing, UI, and test coverage, updates notification rendering buffers to vector-backed storage, and extends size-script manifest discovery and fallback behavior. ChangesMessage delivery status tracking and rendering
NotificationRenderer buffer refactor
Build size script recursion and fallback
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (27)
Build artifacts expire on 2026-08-06. Updated for |
github-actions
Bot
added
needs-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/graphics/draw/MessageRenderer.cpp (1)
51-59: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicate failure-status classification.
showsFailureMarkre-enumerates the same set of "failure"AckStatusvalues thatMessageStatusText.cpp's switch statements implicitly classify. If a new failure status is added later and only one of the two lists is updated, the X-mark and the status text will disagree (mark shown without failure text, or vice versa).Consider exposing a shared classifier (e.g.,
bool isFailureStatus(AckStatus)) fromMessageStatusText/MessageStorethat both the renderer and the text helper consume, so the two representations can't drift.🤖 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 `@src/graphics/draw/MessageRenderer.cpp` around lines 51 - 59, `showsFailureMark` is duplicating the failure classification already encoded in `MessageStatusText.cpp`, so the X-mark and status text can drift apart. Replace the hardcoded `AckStatus` list in `MessageRenderer.cpp` with a shared classifier such as `isFailureStatus(AckStatus)` exposed from `MessageStatusText` or `MessageStore`, and make both `showsFailureMark` and the status-text logic call that same helper.
🤖 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 `@src/graphics/draw/NotificationRenderer.cpp`:
- Around line 521-527: Clamp lineCount before allocating and filling the picker
buffers in the NotificationRenderer logic. In the loop that populates
linePointers and when creating scratchLineBuffer, ensure the effective line
count is bounded by visibleTotalLines (or size both buffers from
std::max(lineCount, visibleTotalLines)) so linePointers[i] cannot exceed the
vector and the scratch buffer size never becomes negative. Use the existing
visibleTotalLines, lineCount, linePointers, and scratchLineBuffer symbols to
make the adjustment in place.
In `@src/mesh/MeshService.cpp`:
- Around line 198-204: The text-message handling block in
MeshService::handleReceived is adding local broadcasts to messageStore and the
UI a second time, which duplicates entries. Update this IF_SCREEN path so it
only calls messageStore.addFromPacket and
graphics::MessageRenderer::handleNewMessage for directed DMs (p.to != 0), and
skip or guard the local broadcast/channel case because Router::sendLocal already
routes those back through the normal receive path.
In `@src/modules/CannedMessageModule.cpp`:
- Around line 1050-1051: The routing-status check in CannedMessageModule should
treat NODENUM_BROADCAST_NO_LORA the same as NODENUM_BROADCAST, since the current
dest-based condition can misclassify no-LoRa broadcasts as DMs. Update the logic
around expectsRoutingStatus and p->want_ack to use the same broadcast handling
as isBroadcast(dest), or otherwise explicitly include both broadcast constants
in the condition, so the behavior stays consistent across all broadcast
destinations.
---
Nitpick comments:
In `@src/graphics/draw/MessageRenderer.cpp`:
- Around line 51-59: `showsFailureMark` is duplicating the failure
classification already encoded in `MessageStatusText.cpp`, so the X-mark and
status text can drift apart. Replace the hardcoded `AckStatus` list in
`MessageRenderer.cpp` with a shared classifier such as
`isFailureStatus(AckStatus)` exposed from `MessageStatusText` or `MessageStore`,
and make both `showsFailureMark` and the status-text logic call that same
helper.
🪄 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: 170f8953-e19e-4b48-aaf6-ad56b31293a6
⛔ Files ignored due to path filters (25)
artifacts/base-ui-message-status/admin-key-not-authorized.pngis excluded by!**/*.pngartifacts/base-ui-message-status/admin-session-expired.pngis excluded by!**/*.pngartifacts/base-ui-message-status/banner-delivered-mesh.pngis excluded by!**/*.pngartifacts/base-ui-message-status/banner-delivered-recipient.pngis excluded by!**/*.pngartifacts/base-ui-message-status/banner-failed-mesh.pngis excluded by!**/*.pngartifacts/base-ui-message-status/banner-recipient-key-unavailable.pngis excluded by!**/*.pngartifacts/base-ui-message-status/banner-recipient-needs-your-key.pngis excluded by!**/*.pngartifacts/base-ui-message-status/banner-relayed.pngis excluded by!**/*.pngartifacts/base-ui-message-status/channel-delivered.pngis excluded by!**/*.pngartifacts/base-ui-message-status/contact-sheet.pngis excluded by!**/*.pngartifacts/base-ui-message-status/dm-delivered.pngis excluded by!**/*.pngartifacts/base-ui-message-status/dm-relayed.pngis excluded by!**/*.pngartifacts/base-ui-message-status/duty-cycle-limit.pngis excluded by!**/*.pngartifacts/base-ui-message-status/failed-mesh.pngis excluded by!**/*.pngartifacts/base-ui-message-status/invalid-request.pngis excluded by!**/*.pngartifacts/base-ui-message-status/no-app-response.pngis excluded by!**/*.pngartifacts/base-ui-message-status/no-channel.pngis excluded by!**/*.pngartifacts/base-ui-message-status/no-interface.pngis excluded by!**/*.pngartifacts/base-ui-message-status/not-authorized.pngis excluded by!**/*.pngartifacts/base-ui-message-status/pki-failed.pngis excluded by!**/*.pngartifacts/base-ui-message-status/rate-limited.pngis excluded by!**/*.pngartifacts/base-ui-message-status/recipient-key-unavailable.pngis excluded by!**/*.pngartifacts/base-ui-message-status/recipient-needs-your-key.pngis excluded by!**/*.pngartifacts/base-ui-message-status/sending.pngis excluded by!**/*.pngartifacts/base-ui-message-status/too-large.pngis excluded by!**/*.png
📒 Files selected for processing (12)
artifacts/base-ui-message-status/README.mdsrc/MessageStore.cppsrc/MessageStore.hsrc/graphics/draw/MessageRenderer.cppsrc/graphics/draw/MessageStatusText.cppsrc/graphics/draw/MessageStatusText.hsrc/graphics/draw/NotificationRenderer.cppsrc/mesh/MeshService.cppsrc/modules/CannedMessageModule.cppsrc/modules/RoutingModule.cpptest/native-suite-counttest/test_message_status_text/test_main.cpp
Contributor
Firmware Size Report23 targets | vs
Show 18 more target(s)
Size budgets
Budgets live in Updated for 5630093 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/collect_sizes.py (1)
23-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent overwrite risk on duplicate board manifests.
Recursive discovery makes it more likely that two manifests (e.g. from separate downloaded-artifact subdirectories) resolve to the same
boardkey. The unconditionalsizes[board] = entryassignment at line 64 will silently keep whichever manifest sorts last, with no signal that a duplicate was dropped, which could mask a real size regression in the discarded manifest.♻️ Suggested guard for duplicate board keys
if bin_size is not None: entry = {"flash_bytes": bin_size} ram_bytes = data.get("ram_bytes") if isinstance(ram_bytes, int) and not isinstance(ram_bytes, bool): entry["ram_bytes"] = ram_bytes + if board in sizes: + print(f"warning: duplicate manifest for board '{board}', overwriting", file=sys.stderr) sizes[board] = entry🤖 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 `@bin/collect_sizes.py` around lines 23 - 30, The recursive manifest scan in collect_sizes.py can produce duplicate board keys, and the current entry-building path silently overwrites earlier results when sizes[board] is assigned. Update the manifest processing logic around manifest_paths, the per-file loop, and the board key handling to detect duplicates before writing into sizes; when the same board appears more than once, either fail fast or emit a clear warning with both manifest paths so the duplicate cannot be silently dropped.
🤖 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 `@bin/collect_sizes.py`:
- Around line 23-30: The recursive manifest scan in collect_sizes.py can produce
duplicate board keys, and the current entry-building path silently overwrites
earlier results when sizes[board] is assigned. Update the manifest processing
logic around manifest_paths, the per-file loop, and the board key handling to
detect duplicates before writing into sizes; when the same board appears more
than once, either fail fast or emit a clear warning with both manifest paths so
the duplicate cannot be silently dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15ddc0cb-9cdd-4a34-9dc2-6f82e283de45
📒 Files selected for processing (2)
bin/collect_sizes.pybin/test_size_scripts.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/test_size_scripts.py (1)
192-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
stdoutvariable.Static analysis flags
stdoutfromrun_scriptas unused here.🧹 Proposed fix
- rc, stdout, stderr = run_script("collect_sizes.py", [tmpdir, outfile]) + rc, _stdout, stderr = run_script("collect_sizes.py", [tmpdir, outfile])🤖 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 `@bin/test_size_scripts.py` at line 192, The `run_script` result in the `collect_sizes.py` test currently binds `stdout` but never uses it, so update the assignment in `test_size_scripts.py` to ignore that value while keeping `rc` and `stderr` for the assertion path. Locate the call inside the size-script test helper and remove the unused `stdout` binding to satisfy static analysis without changing test behavior.Source: Linters/SAST tools
🤖 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 `@bin/test_size_scripts.py`:
- Line 192: The `run_script` result in the `collect_sizes.py` test currently
binds `stdout` but never uses it, so update the assignment in
`test_size_scripts.py` to ignore that value while keeping `rc` and `stderr` for
the assertion path. Locate the call inside the size-script test helper and
remove the unused `stdout` binding to satisfy static analysis without changing
test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b31a33e-e0e3-43dd-b763-2db0075e0352
📒 Files selected for processing (2)
bin/collect_sizes.pybin/test_size_scripts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/collect_sizes.py
Contributor
|
Do not merge until screen shots of regression testing is completed. |
Contributor
|
Also things like should not be included in this PR |
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
2 participants
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.
Source issues
Summary
want_ackDMs are tracked for routing status updates.UI validation
Local Base UI status and banner captures were generated and reviewed. Evidence image is embedded below from an evidence-only branch; screenshot files are intentionally not committed to this PR branch.
Validation
trunk fmt/ commit hook passed.git diff --checkpassed.PLATFORMIO_BUILD_FLAGS='-DMESHTASTIC_INCLUDE_BASE_UI_MESSAGE_STATUS' ~/.platformio/penv/bin/python -m platformio test -e native-macos -f test_message_status_textpassed: 18 test cases.~/.platformio/penv/bin/python -m platformio run -e native-macospassed.Hardware verification
Not yet performed. This PR is draft until Base UI hardware verification is complete.
Local notes
./bin/run-tests.sh -f test_message_status_textcould not run directly on this macOS host because the wrapper requires GNU bash/mapfile and GNUfind -printf../bin/test-native-docker.sh -f test_message_status_textcould not run because the Docker daemon was not available locally.