Persist node stats, fix export completeness, dead code cleanup - #14
Merged
Conversation
5 fixes from this round's survey: - NodeSnapshot's last_snr/last_rssi/last_hops_used/last_hop_start/ last_via_mqtt and rf_count/via_mqtt_count/position_count/ telemetry_count had no DB column at all — a node tracked for weeks reset to zero/unknown on every restart until heard again live, breaking the Nodes table's Direct/SNR/RSSI/Hops/Source columns, NodeInspector, and CSV export for every node not freshly heard this session. Added the columns (with a migration for existing DBs), wired them into MonitorStore._write_node's upsert (last_* fields use a last-heard-gated CASE, not MAX — a fresher 0-hop packet must not lose to a staler 3-hop one) and PacketIngestor.seed_from_store. - ExportService.export_packets_csv's via_mqtt column used int(bool(pkt.via_mqtt)), collapsing "unknown" (older firmware never reports the field) into "confirmed not MQTT" — every other tri-state field in the same row already uses the is-not-None pattern. Fixed to match. - "Export Packet Log" reads from a bounded 10,000-packet in-memory ring buffer with no indication anything was left out on a long/busy session. Added a warning dialog when the session has ingested more packets than the buffer holds, so the user knows before exporting rather than getting a silently incomplete file. - "Remove Node from Radio" only writes to the connected radio's own NodeDB — MeshChat's own Nodes table/Rankings/Map are untouched and the node keeps appearing immediately after a "successful" removal. Reworded the confirmation dialog to say so explicitly instead of implying removal here too. - Deleted utils/node_names.py (resolve_node_name/resolve_short_name): no production callers — both call sites that need the same logic (ChatView's sender-name resolution, NodeSnapshot.display_name) had already reimplemented it inline, so this was pure dead code, plus its now-pointless test file.
…ri-state counters - "Export Packet Log" now reads MonitorStore.read_packets_as_objects() (the full session's persisted packets) instead of the bounded 10,000-packet in-memory ring buffer, fixing the actual gap rather than just warning about it. The packets table has no text column (message content lives in `messages`), so text is merged back in from whatever packets are still in the in-memory buffer when the user opts in — best-effort, not required for correctness of any other field. - Reworded the Remove Node dialog again: dropped the "until it transmits again" clause, which still overstated MeshChat-side effect — the node stays in the Nodes table/Rankings/Map indefinitely regardless (MeshChat's node history is intentionally exempt from pruning), not just until its next transmission. - Fixed a pre-existing rf_count/via_mqtt_count misclassification that this PR's persistence work would have made permanent: `pkt.is_via_mqtt` coerces via_mqtt=None (older firmware that never reports the field) to False via bool(), so it silently counted as confirmed RF. Now checks the tri-state field directly and leaves both counters alone when the transport is genuinely unknown. Added MonitorStore.read_packets_as_objects() + tests, and a regression test for the tri-state counter fix.
- Ring-buffer packets (get_recent_packets()) are now always included as-is and treated as authoritative, instead of being replaced by store-read rows — the store write is asynchronous and can lag behind what's already in memory, so store rows now only fill in whatever fell out of the ring buffer (older packets), never race against or omit what's still in it. This also means text no longer needs a separate merge step: it's already present on every in-memory packet and naturally absent on store-derived ones. - Fixed the "include message text" dialog copy: it said text was missing for "packets from a previous run", but export is scoped to the current session (a new id every launch) — previous runs were never in this file at all. The real gap is packets that fell out of the current session's 10,000-packet in-memory buffer. - read_packets_as_objects() caps at 200,000 rows — added back a (much rarer) partial-export warning for a session that exceeds it, comparing MonitorStore.packet_count() against the final exported row count so it only fires on genuine truncation.
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change persists packet-derived node metrics, reconstructs stored packets for export, preserves tri-state transport values, restores node state across sessions, and updates node-removal messaging. Node-name resolution utilities and their tests are removed. ChangesPacket persistence and export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MainWindow
participant MonitorStore
participant PacketIngestor
participant ExportService
MainWindow->>MonitorStore: read persisted session packets
MonitorStore-->>MainWindow: return reconstructed NetworkPacket objects
MainWindow->>PacketIngestor: read recent in-memory packets
MainWindow->>MainWindow: merge and deduplicate packet records
MainWindow->>ExportService: export combined records
ExportService-->>MainWindow: write tri-state via_mqtt fields
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 2
🤖 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/meshchat/ui/main_window.py`:
- Around line 301-303: Move the historical export workflow containing
read_packets_as_objects, the older/recent merge, and CSV writing into a
background worker so database and file operations do not run on the GUI thread.
Keep the QAction handler limited to starting the worker, and marshal completion,
errors, dialogs, and status updates back to the GUI thread.
- Around line 299-303: The merged packet list in the export flow is not
chronologically ordered because stored packets are newest-first while recent
packets are insertion-ordered. Update the rows assembly around
get_recent_packets, read_packets_as_objects, and rows to sort the combined rows
by observed_at before exporting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59960610-7d35-4281-ba65-d6157eb36edd
📒 Files selected for processing (12)
src/meshchat/database/schema.pysrc/meshchat/services/export_service.pysrc/meshchat/services/monitor_store.pysrc/meshchat/services/packet_ingestor.pysrc/meshchat/ui/main_window.pysrc/meshchat/ui/nodes/nodes_page.pysrc/meshchat/utils/node_names.pytests/test_export_service.pytests/test_monitor_store_packets.pytests/test_monitor_store_upsert.pytests/test_node_names.pytests/test_packet_ingestor.py
💤 Files with no reviewable changes (2)
- tests/test_node_names.py
- src/meshchat/utils/node_names.py
Comment on lines
+301
to
+303
| store_rows = self._store.read_packets_as_objects(self._session.id) | ||
| older = [p for p in store_rows if (p.sender_num, p.packet_id, p.observed_at) not in recent_keys] | ||
| rows = older + recent |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move historical export work off the GUI thread.
This QAction handler reconstructs up to 200,000 database rows on the GUI thread. It then writes the merged CSV on the same thread. Large exports will stop input and repaint processing until both operations finish. Run the read, merge, and write work in a worker, then return to the GUI thread only for dialogs and status updates.
🤖 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/meshchat/ui/main_window.py` around lines 301 - 303, Move the historical
export workflow containing read_packets_as_objects, the older/recent merge, and
CSV writing into a background worker so database and file operations do not run
on the GUI thread. Keep the QAction handler limited to starting the worker, and
marshal completion, errors, dialogs, and status updates back to the GUI thread.
- Sort the merged packet export chronologically: read_packets_as_objects() returns newest-first, get_recent_packets() returns insertion order — the merged CSV had a reverse-chronological historical section followed by a chronological recent one. - Move the actual CSV write off the GUI thread onto a QThread worker (_PacketExportWorker) — up to 200,000 rows of CSV formatting + disk I/O would otherwise block input/repaints for as long as it takes. Guards against a second concurrent export, and closeEvent() waits for an in-flight export instead of destroying a running QThread.
- Don't parent the export QThread to MainWindow: run() is a blocking write, so if the window closes before it returns, the thread must survive long enough for closeEvent to wait it out rather than being a child object destroyed mid-run. - closeEvent now blocks until an in-flight export actually finishes instead of a short 5s wait() — quit() alone can't interrupt a synchronous blocking write, and a short wait risked exactly the "destroyed while still running" crash it was meant to avoid on a large export. - Worker.run() now catches Exception, not just OSError — any other uncaught exception previously skipped both finished/failed, leaving the thread's event loop up forever and Export permanently disabled for the rest of the session. - Fixed deleteLater() ordering: worker/thread now delete themselves from their own thread's still-running event loop (finished/failed -> deleteLater directly), not from the GUI-thread handler that runs after the worker's thread affinity is already gone.
Grok caught that removing the explicit thread.quit() call in closeEvent (previous commit) introduced a real deadlock: worker finished/failed -> thread.quit is a QUEUED connection (the QThread object lives on the GUI thread, the worker emits from the export thread), so it only gets delivered once the GUI thread's event loop is pumping — which it isn't while closeEvent is blocked in wait(). A write that finished during that wait() would leave the worker thread parked in its event loop forever with nothing left to tell it to quit, hanging app shutdown indefinitely on any close-during-export. Restored the direct quit() call before wait() — it doesn't interrupt the blocking write in progress, but ensures the thread doesn't idle in exec() once that write actually returns.
Owner
Author
|
Both CodeRabbit findings addressed:
|
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.
Summary
MonitorStore._write_node's upsert (last-heard-gated, not a naive MAX — a fresher 0-hop packet must not lose to a staler 3-hop one) andPacketIngestor.seed_from_store.ExportService'svia_mqttCSV column collapsing "unknown" (older firmware never reports it) into "confirmed not MQTT" — now tri-state like every other field in the same row.rf_count/via_mqtt_countusedpkt.is_via_mqtt(coercesNonetoFalse), silently counting unknown-transport packets as confirmed RF forever after. Now checks the tri-state field directly.utils/node_names.py(no production callers — both real call sites had already reimplemented the same logic inline) and its test file.Went through 3 Grok review rounds. Round 1 found the remove-node wording still misleading, the export claiming ring-buffer data was the only thing available (store already has more), and the rf_count/via_mqtt_count tri-state bug. Round 2 found the store-primary export could race/omit packets still only in memory (fixed by making the ring buffer authoritative and the store additive), imprecise dialog copy, and a missing 200k-cap warning. Round 3 is clean.
Test plan
pytest -q— 353 passedruff check src tests scripts— cleanmypy src/meshchat— cleanpython -m meshchat, confirmed the schema migration applies cleanly and the app startsSummary by CodeRabbit