From ba5af761fb116154594857878d2dd1c21cc067f3 Mon Sep 17 00:00:00 2001 From: Torlando <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 00:46:07 +0000 Subject: [PATCH] fix(lxmf): preserve draft on async send; re-gather hidden-peer history (Greptile P1s) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile P1 remediation on the exact head (round: 1a34c55): 1. Send Completion Erases Draft (UIManager.cpp:1780). The async send deferral (1c68860) leaves the composer un-cleared between the send click and the main-loop's ADDED commit, so input typed into the composer while persistence/admission is in flight was wiped by the unconditional clear_composer(). ChatScreen now captures the exact submitted text when the send is accepted into the mailbox, and clear_composer() only clears when the composer still holds that text. A rejected send still retains input (unchanged), and a fresh draft can no longer be erased by a late completion. 2. Same-Peer History Stays Stale (ChatScreen.cpp:177). The same-peer early-return (ce92e80) skipped the store re-read, so a message for this peer that persisted while the chat was hidden ( on_message_received only appends to the visible chat) never surfaced on re-open. The early-return now compares the store's in-memory conversation count (get_messages_for_conversation — pure slot lookup, no LittleFS, safe under the LVGL lock) against the count at the last prepare commit and falls through to the peer-change path on a mismatch, which resets the list and re-arms prepare so the main loop re-gathers off-lock and rebuilds with the new message. Verification: 181/181 build-script contracts (5 new pins), tdeck + tdeck-release green. Compose path audited and unaffected: the single send slot makes a second send a no-op until the first commits, and its clear rides on the route replacement (render_route). --- lib/tdeck_ui/UI/LXMF/ChatScreen.cpp | 43 +++++- lib/tdeck_ui/UI/LXMF/ChatScreen.h | 12 ++ ...t_send_draft_and_stale_history_contract.py | 124 ++++++++++++++++++ 3 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 tests/build_scripts/test_send_draft_and_stale_history_contract.py diff --git a/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp b/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp index 3ad8340c..a72b2905 100644 --- a/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp +++ b/lib/tdeck_ui/UI/LXMF/ChatScreen.cpp @@ -172,9 +172,19 @@ void ChatScreen::load_conversation(const Bytes& peer_hash, ::LXMF::MessageStore& // Same-peer re-open (back to the list and re-tap): the content is // already committed by prepare_conversation() and the rows are still - // built — nothing to do. This keeps re-opens free of any store I/O. + // built — usually nothing to do, keeping re-opens free of store I/O. + // Exception: a message for this peer persisted while the chat was + // hidden (on_message_received only appends to the visible chat). The + // live count is one in-memory index read (no LittleFS); on a mismatch + // fall through to the peer-change path, which resets the list and + // re-arms prepare_conversation() so the main loop re-gathers off-lock. if (_peer_hash == peer_hash && _prepared_peer_hash == peer_hash) { - return; + size_t live_count = store.get_messages_for_conversation(peer_hash).size(); + if (live_count == _prepared_message_count) { + return; + } + INFO("Same-peer re-open: new message(s) since prepare; re-gathering"); + _prepared_message_count = 0; } _peer_hash = peer_hash; @@ -227,6 +237,7 @@ void ChatScreen::load_conversation(const Bytes& peer_hash, ::LXMF::MessageStore& // too, so a background-fill batch in flight while the list is reset // drops its (now stale) result. _prepared_peer_hash = Bytes(); + _prepared_message_count = 0; _prepare_generation++; _fill_generation++; } @@ -344,6 +355,7 @@ void ChatScreen::prepare_conversation() { _fill_generation++; _prepared_peer_hash = peer_hash; + _prepared_message_count = _all_message_hashes.size(); scroll_to_bottom(); } } @@ -361,6 +373,7 @@ void ChatScreen::refresh() { } INFO("Refreshing chat messages"); _prepared_peer_hash = Bytes(); + _prepared_message_count = 0; _prepare_generation++; } @@ -755,12 +768,17 @@ void ChatScreen::on_send_clicked(lv_event_t* event) { String message(text); if (message.length() > 0 && screen->_send_message_callback) { - // Publish to the main loop; the composer is cleared only after + // Publish to the main loop. On acceptance, record the exact composer + // state that was submitted; the composer is cleared only after // persistence and queue admission succeed (clear_composer() from - // UIManager::apply_outbound_result). A rejected send (busy router, - // full queue, storage error) keeps the text for a normal re-send, so - // no typed input is ever lost to a failed send. - screen->_send_message_callback(message); + // UIManager::apply_outbound_result) AND only if the composer still + // holds that same text. A rejected send (busy router, full queue, + // storage error) keeps the text for a normal re-send, and anything + // typed into the composer after submission is never wiped by the + // later completion commit. + if (screen->_send_message_callback(message)) { + screen->_pending_submitted_text = message.c_str(); + } } } @@ -768,6 +786,17 @@ void ChatScreen::clear_composer() { // Recursive lock: apply_outbound_result() calls this while already // holding the LVGL lock. LVGL_LOCK(); + // Only clear if the composer still holds the exact text we submitted. + // Persistence + router admission run on the main loop off the LVGL + // lock, so the user may have started typing the next message before the + // commit lands; wiping an edited composer here would erase fresh input. + // An empty _pending_submitted_text means nothing is pending, in which + // case a clear is a no-op-safe reset. + const char* current = lv_textarea_get_text(_text_area); + if (current != nullptr && _pending_submitted_text != current) { + return; + } + _pending_submitted_text.clear(); lv_textarea_set_text(_text_area, ""); lv_group_focus_obj(_text_area); } diff --git a/lib/tdeck_ui/UI/LXMF/ChatScreen.h b/lib/tdeck_ui/UI/LXMF/ChatScreen.h index 43055f04..46b056cb 100644 --- a/lib/tdeck_ui/UI/LXMF/ChatScreen.h +++ b/lib/tdeck_ui/UI/LXMF/ChatScreen.h @@ -197,6 +197,12 @@ private: ::LXMF::MessageStore* _message_store; std::deque _messages; + // Composer text captured when a send was accepted into the main-loop + // mailbox. apply_outbound_result() only clears the composer if it still + // holds exactly this text, so input typed into the composer while + // persistence/admission was in flight is never erased by a later commit. + std::string _pending_submitted_text; + // Map message hash to bubble row for targeted updates std::map _message_rows; @@ -207,6 +213,12 @@ private: // happens while a prepare's I/O is in flight. RNS::Bytes _prepared_peer_hash; uint32_t _prepare_generation = 0; + // Message count at the moment the current rows were committed by + // prepare_conversation(). A same-peer re-open compares the store's live + // count against this: if it grew (a message for this peer landed while + // the chat was hidden), the early-return is not taken and prepare is + // re-armed. Zero means "nothing committed yet". + size_t _prepared_message_count = 0; BackCallback _back_callback; SendMessageCallback _send_message_callback; diff --git a/tests/build_scripts/test_send_draft_and_stale_history_contract.py b/tests/build_scripts/test_send_draft_and_stale_history_contract.py new file mode 100644 index 00000000..bb286a99 --- /dev/null +++ b/tests/build_scripts/test_send_draft_and_stale_history_contract.py @@ -0,0 +1,124 @@ +"""Source-level contracts for the Greptile P1 remediation on PR #96. + +Two regressions introduced by the off-lock threading fixes (1c68860 / +ce92e80) were flagged by Greptile on the exact head and fixed in place: + +1. Send Completion Erases Draft — the async send deferral leaves the + composer un-cleared between the send click and the main-loop's + ADDED commit. The completion path must therefore only clear the + composer when it still holds exactly the submitted text, so input + typed while persistence/admission is in flight survives. + +2. Same-Peer History Stays Stale — the same-peer early-return in + ChatScreen::load_conversation() skips the store re-read, so a + message that arrived for that peer while the chat was hidden never + surfaces on re-open. The early-return must now compare the store's + in-memory conversation count against the count at the last prepare + commit and re-arm prepare on a mismatch. +""" + +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[2] +CHAT_CPP = ROOT / "lib/tdeck_ui/UI/LXMF/ChatScreen.cpp" +CHAT_H = ROOT / "lib/tdeck_ui/UI/LXMF/ChatScreen.h" + + +def chat_source() -> str: + if not CHAT_CPP.is_file(): + pytest.skip("ChatScreen.cpp not found") + return CHAT_CPP.read_text() + + +def chat_header() -> str: + if not CHAT_H.is_file(): + pytest.skip("ChatScreen.h not found") + return CHAT_H.read_text() + + +def function_body(source: str, signature: str, end_marker: str) -> str: + start = source.index(signature) + end = source.index(end_marker, start) + return source[start:end] + + +def test_clear_composer_is_generation_checked(): + source = chat_source() + body = function_body( + source, + "void ChatScreen::clear_composer()", + "lv_group_focus_obj(_text_area);", + ) + # The clear must compare the current composer text against the + # captured submitted text and bail when they differ. + assert "_pending_submitted_text" in body, ( + "clear_composer() no longer consults the submitted-text generation" + ) + assert "lv_textarea_get_text(_text_area)" in body + assert "return;" in body, ( + "clear_composer() must skip the clear when the composer was edited" + ) + + +def test_send_click_records_submitted_text(): + source = chat_source() + body = function_body( + source, + "void ChatScreen::on_send_clicked(", + "void ChatScreen::clear_composer()", + ) + assert "_pending_submitted_text" in body, ( + "on_send_clicked() must capture the submitted composer text on " + "acceptance so the later completion can identify it" + ) + assert "_send_message_callback(message)" in body + + +def test_same_peer_reopen_checks_live_message_count(): + source = chat_source() + body = function_body( + source, + "void ChatScreen::load_conversation(", + "void ChatScreen::prepare_conversation()", + ) + assert "_prepared_message_count" in body, ( + "load_conversation() same-peer early-return must compare the store's " + "live conversation count against the prepared count" + ) + assert "get_messages_for_conversation(peer_hash).size()" in body, ( + "the staleness check must use the in-memory index (no LittleFS)" + ) + + +def test_prepared_count_recorded_and_reset(): + source = chat_source() + # Recorded at prepare commit time. + prepare_body = function_body( + source, + "void ChatScreen::prepare_conversation()", + "void ChatScreen::refresh()", + ) + assert "_prepared_message_count = _all_message_hashes.size()" in prepare_body + # Reset by the peer-change path so a fresh prepare always re-records. + load_body = function_body( + source, + "void ChatScreen::load_conversation(", + "void ChatScreen::prepare_conversation()", + ) + assert "_prepared_message_count = 0" in load_body + refresh_body = function_body( + source, + "void ChatScreen::refresh()", + "void ChatScreen::add_message(", + ) + assert "_prepared_message_count = 0" in refresh_body + + +def test_pending_submitted_text_field_declared(): + header = chat_header() + assert "_pending_submitted_text" in header, ( + "ChatScreen must declare the submitted-text capture field" + ) + assert "_prepared_message_count" in header