feat: align MUI message status wording#336
Draft
RCGV1 wants to merge 16 commits into
Draft
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a shared ChangesMessage status tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
RCGV1
changed the title
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/util/LogRotate.cpp (1)
104-119: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly publish
EntryPositionafter the append actually succeeds.
write(entry, &position)currently records an offset and returnstrueeven if_fs.open()orserialize()fails. Downstream pending-status tracking treats that as a persisted log entry, so later status updates can target a non-existent entry.Proposed fix
- if (position) { - position->logNum = currentLogWrite; - position->offset = currentSize; - } - // elegant way to let the logentry do its work it knows best and pass just a temporary function for writing File file = _fs.open(currentLogName, FILE_APPEND); - entry.serialize([&file](const uint8_t *buf, size_t size) { return file.write(buf, size); }); + if (!file) { + ILOG_WARN("failed to open %s for append", currentLogName.c_str()); + return false; + } + + const EntryPosition writtenPosition{currentLogWrite, currentSize}; + const size_t written = entry.serialize([&file](const uint8_t *buf, size_t size) { return file.write(buf, size); }); file.close(); + if (written != entry.size()) { + ILOG_WARN("failed to append complete log entry to %s", currentLogName.c_str()); + return false; + } + + if (position) + *position = writtenPosition; currentSize += entry.size(); totalSize += entry.size();🤖 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 `@source/util/LogRotate.cpp` around lines 104 - 119, `LogRotate::write` is marking an `EntryPosition` as persisted before the append has actually completed. Move the `position->logNum` and `position->offset` update so it only happens after `_fs.open()` and `entry.serialize(...)` succeed, and make the function return false on any write/open failure instead of always returning true. Keep the fix localized to `LogRotate::write`, using the existing `position`, `currentLogWrite`, `currentSize`, and `entry.serialize` flow.
🧹 Nitpick comments (4)
include/graphics/common/MessageStatus.h (1)
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the two
inlineStateForLogStatusoverloads.The two-argument overload simply forwards to the three-argument one with
persistedState = 0. A default parameter would remove the duplicate declaration/definition pair.♻️ Suggested consolidation
-std::optional<State> inlineStateForLogStatus(LogMessage::MsgStatus status, bool livePending); -std::optional<State> inlineStateForLogStatus(LogMessage::MsgStatus status, uint32_t persistedState, bool livePending); +std::optional<State> inlineStateForLogStatus(LogMessage::MsgStatus status, uint32_t persistedState, bool livePending, uint32_t defaultPersistedState = 0);🤖 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 `@include/graphics/common/MessageStatus.h` around lines 50 - 51, The two inlineStateForLogStatus overloads are redundant because the two-argument version only forwards to the three-argument version with persistedState set to 0. Consolidate them by keeping a single inlineStateForLogStatus declaration/definition with a default parameter for persistedState, and update any callers or declarations in MessageStatus to use that unified signature.source/graphics/common/MessageStatus.cpp (2)
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLow byte capacity for persisted state is implicit and unchecked.
persistedLogStateMaskreserves only the low 8 bits (0xff) for the state value (persistedLogStateMagic | (state + 1)), capping the encodable state count at 254. With 18 states today there's plenty of headroom, but nothing guards against silently overflowing into the magic bits ifStategrows substantially over time.Also applies to: 121-124
🤖 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 `@source/graphics/common/MessageStatus.cpp` around lines 25 - 26, The persisted log state encoding in MessageStatus uses only the low 8 bits for the state value, but this limit is currently implicit and unchecked. Update the encoding/decoding logic around persistedLogStateMagic and persistedLogStateMask (including the matching code later in the file) to explicitly validate that the State value fits within the available bits before writing it, and reject or assert on overflow instead of silently corrupting the magic bits. Keep the mask/magic scheme intact, but add a clear bound check tied to the State enum size so future state growth cannot break persisted data.
121-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPersisted-state bounds check is coupled to enum declaration order.
stateFromPersistedLogStatevalidates the decoded value againststatic_cast<uint32_t>(State::AdminKeyNotAuthorized) + 1, implicitly assumingAdminKeyNotAuthorizedis the last (highest-ordinal) enumerator inState. If a futureStatevalue is inserted anywhere but the very end, or the list is reordered, this bound silently becomes wrong (either rejecting valid new states or accepting invalid ones) with no compiler diagnostic.Consider adding an explicit sentinel (e.g.
State::Countas the last enumerator, never used as a real state) and bounding against that instead, so correctness doesn't depend on remembering to update this constant wheneverStatechanges.♻️ Suggested approach
enum class State { ... AdminKeyNotAuthorized, + Count, // sentinel — always keep last };- if (value == 0 || value > static_cast<uint32_t>(State::AdminKeyNotAuthorized) + 1) + if (value == 0 || value > static_cast<uint32_t>(State::Count)) return std::nullopt;🤖 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 `@source/graphics/common/MessageStatus.cpp` around lines 121 - 136, `stateFromPersistedLogState` currently validates persisted values against `State::AdminKeyNotAuthorized`, which ties the check to the enum’s current ordering. Add an explicit terminal sentinel such as `State::Count` in the `State` enum and use it as the upper bound in the persisted-state range check, so `persistedLogState` and `stateFromPersistedLogState` stay correct even if `State` is extended or reordered.include/util/LogRotate.h (1)
38-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider collapsing
write(const ILogEntry&, EntryPosition*)into a single overload with a defaultedpositionparameter. This would trim the API without changing 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 `@include/util/LogRotate.h` around lines 38 - 40, The LogRotate API exposes two write-related overloads where one can be simplified: collapse write(const ILogEntry&, EntryPosition*) into a single write overload with a defaulted position parameter, and update any callers or declarations that reference the separate pointer-based form. Keep the existing behavior intact in LogRotate by preserving the optional position output while removing the redundant overload; use the write and update declarations in ILogEntry/LogRotate as the anchor points for the change.
🤖 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 `@source/graphics/common/ViewController.cpp`:
- Around line 477-484: Replace the compile-time-only payload check in the text
message send path with a runtime guard in the send/log flow around send() and
LogMessageEnv. In ViewController::sendTextMessage (or the nearby message
handling code), reject oversized text before calling send and before
constructing the log entry, and skip logging or truncate as appropriate so
persisted LogMessageEnv data never exceeds the deserialization limit. Keep the
existing fitsLogMessagePayload check as a debug aid if desired, but add a real
conditional that enforces the size at runtime.
In `@source/graphics/TFT/TFTView_320x240.cpp`:
- Around line 4499-4501: In the oversized-payload branch of TFTView_320x240.cpp,
the code still passes the full msg into addMessage even though
fitsLogMessagePayload(msgLen) has already failed. Update this path so addMessage
receives a bounded/truncated display string, or avoid calling addMessage at all
and only trigger handleTextMessageResponse for the MessageTooLarge case. Use the
existing addMessage and handleTextMessageResponse flow to keep the fix
localized.
---
Outside diff comments:
In `@source/util/LogRotate.cpp`:
- Around line 104-119: `LogRotate::write` is marking an `EntryPosition` as
persisted before the append has actually completed. Move the `position->logNum`
and `position->offset` update so it only happens after `_fs.open()` and
`entry.serialize(...)` succeed, and make the function return false on any
write/open failure instead of always returning true. Keep the fix localized to
`LogRotate::write`, using the existing `position`, `currentLogWrite`,
`currentSize`, and `entry.serialize` flow.
---
Nitpick comments:
In `@include/graphics/common/MessageStatus.h`:
- Around line 50-51: The two inlineStateForLogStatus overloads are redundant
because the two-argument version only forwards to the three-argument version
with persistedState set to 0. Consolidate them by keeping a single
inlineStateForLogStatus declaration/definition with a default parameter for
persistedState, and update any callers or declarations in MessageStatus to use
that unified signature.
In `@include/util/LogRotate.h`:
- Around line 38-40: The LogRotate API exposes two write-related overloads where
one can be simplified: collapse write(const ILogEntry&, EntryPosition*) into a
single write overload with a defaulted position parameter, and update any
callers or declarations that reference the separate pointer-based form. Keep the
existing behavior intact in LogRotate by preserving the optional position output
while removing the redundant overload; use the write and update declarations in
ILogEntry/LogRotate as the anchor points for the change.
In `@source/graphics/common/MessageStatus.cpp`:
- Around line 25-26: The persisted log state encoding in MessageStatus uses only
the low 8 bits for the state value, but this limit is currently implicit and
unchecked. Update the encoding/decoding logic around persistedLogStateMagic and
persistedLogStateMask (including the matching code later in the file) to
explicitly validate that the State value fits within the available bits before
writing it, and reject or assert on overflow instead of silently corrupting the
magic bits. Keep the mask/magic scheme intact, but add a clear bound check tied
to the State enum size so future state growth cannot break persisted data.
- Around line 121-136: `stateFromPersistedLogState` currently validates
persisted values against `State::AdminKeyNotAuthorized`, which ties the check to
the enum’s current ordering. Add an explicit terminal sentinel such as
`State::Count` in the `State` enum and use it as the upper bound in the
persisted-state range check, so `persistedLogState` and
`stateFromPersistedLogState` stay correct even if `State` is extended or
reordered.
🪄 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: 1b84f703-c83f-449a-9b44-a11515ede6e5
⛔ Files ignored due to path filters (19)
artifacts/message-status-evidence/admin-key-not-authorized.pngis excluded by!**/*.pngartifacts/message-status-evidence/admin-session-expired.pngis excluded by!**/*.pngartifacts/message-status-evidence/all-message-status-states.pngis excluded by!**/*.pngartifacts/message-status-evidence/channel-implicit-ack.pngis excluded by!**/*.pngartifacts/message-status-evidence/dm-implicit-ack.pngis excluded by!**/*.pngartifacts/message-status-evidence/duty-cycle-limit.pngis excluded by!**/*.pngartifacts/message-status-evidence/explicit-ack.pngis excluded by!**/*.pngartifacts/message-status-evidence/generic-encrypted-send-failure.pngis excluded by!**/*.pngartifacts/message-status-evidence/invalid-request.pngis excluded by!**/*.pngartifacts/message-status-evidence/message-too-large.pngis excluded by!**/*.pngartifacts/message-status-evidence/no-ack.pngis excluded by!**/*.pngartifacts/message-status-evidence/no-app-response.pngis excluded by!**/*.pngartifacts/message-status-evidence/no-channel.pngis excluded by!**/*.pngartifacts/message-status-evidence/no-radio-interface.pngis excluded by!**/*.pngartifacts/message-status-evidence/not-authorized.pngis excluded by!**/*.pngartifacts/message-status-evidence/rate-limited.pngis excluded by!**/*.pngartifacts/message-status-evidence/recipient-key-unavailable.pngis excluded by!**/*.pngartifacts/message-status-evidence/recipient-needs-your-key.pngis excluded by!**/*.pngartifacts/message-status-evidence/sending.pngis excluded by!**/*.png
📒 Files selected for processing (15)
cmake/protobuf.cmakeinclude/graphics/common/MessageStatus.hinclude/graphics/common/ResponseHandler.hinclude/graphics/common/ViewController.hinclude/graphics/view/TFT/TFTView_320x240.hinclude/util/LogMessage.hinclude/util/LogRotate.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/MessageStatus.cppsource/graphics/common/ResponseHandler.cppsource/graphics/common/ViewController.cppsource/util/LogRotate.cpptests/test_LogMessage.cpptests/test_MessageStatus.cpptests/test_ResponseHandler.cpp
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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.
Issues
Summary
UI Evidence
Validation
git diff --checkctest: 1/1 tests passed.t-deck-tftfirmware build passed.t-deck-tftapp image flashed and verified on the connected T-Deck.Status
Draft until interactive on-device UI screenshots are captured and attached.