Round 2: fix VERSION ImportError crash, snapshot correctness, single-pass rankings - #2
Conversation
Ten improvements. The headline is a live crash that shipped in the alpha. Crash fix 1. `VERSION` was imported by three call sites but never defined — version.py only exported `__version__`. Help > About, Help > Copy Diagnostic Summary, and session JSON export all raised ImportError the moment a user clicked them. They were lazy imports inside functions, so nothing failed at startup and no test caught it. 2. version.py now single-sources from installed package metadata (so it cannot drift from pyproject) and exports both spellings. 3. Added a test that walks the AST of every source file and asserts each `from meshchat... import NAME` actually resolves — 87 imports across 42 modules. This is the check that would have caught the bug. Correctness 4. get_nodes()/get_node() returned live NodeSnapshot objects, which the ingestion path mutates in place. Callers never had a real snapshot: the node table could re-sort by a last_heard that changed after it was read. They now return copies. 5. Corrected the PacketIngestor docstring, which claimed the object lives on a worker thread. It is never moveToThread'd — it is created on and stays on the GUI thread. The old claim would mislead anyone reasoning about locking here. Robustness 6. Added a top-level excepthook that logs unhandled exceptions and shows a dialog with the traceback. Release builds are windowed, so previously a crash closed the app with no output anywhere the user could see it. Performance 7. _refresh_rankings made four separate full passes over up to 10k packets every 2 seconds (per-sender counts, best SNR, text counts, direct-RF medians). Consolidated into one pass. Maintainability 8. Named PortNum constants replace magic 1/3/67 literals across the classifier, ingestor, and monitor page. 9. Added .gitattributes — every commit so far emitted CRLF warnings. 10. Added a PEP 561 py.typed marker and declared it in package-data. 219 tests pass, ruff clean, verified against a live radio on COM11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes add application exception reporting, package version metadata, typed-package support, named Meshtastic port constants, copied node snapshots, and single-pass monitor ranking aggregation. Repository attributes now define text, binary, and asset handling. ChangesMeshChat application updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PythonRuntime
participant MeshChatExceptionHook
participant ApplicationLogger
participant QtApplication
participant QtErrorDialog
PythonRuntime->>MeshChatExceptionHook: unhandled exception
MeshChatExceptionHook->>ApplicationLogger: log traceback
MeshChatExceptionHook->>QtApplication: check application instance
QtApplication-->>MeshChatExceptionHook: application instance
MeshChatExceptionHook->>QtErrorDialog: show critical error details
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.gitattributes (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommit the line-ending renormalization.
.gitattributesonly changes future checkout/check-in behavior. The repository still needs a one-time commit ofgit add --renormalize .so updated text files, such asREADME.mdandTHIRD_PARTY_LICENSES.md, do not create large diffs when users or CI apply the new attributes.🤖 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 @.gitattributes around lines 1 - 3, Commit the repository-wide line-ending renormalization after adding the `* text=auto eol=lf` rule in `.gitattributes`; stage tracked files with Git’s renormalization operation so files such as `README.md` and `THIRD_PARTY_LICENSES.md` receive the normalized representation without unrelated content changes.src/meshchat/analytics/packet_classifier.py (1)
4-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the named constants in both port maps.
PORTNUM_LABELSand the local mapping inpacket_category()still repeat bare numeric values. A later port update can make display classification and packet predicates disagree. Replace those keys withPORTNUM_*constants, or derive both paths from one mapping.Proposed refactor
PORTNUM_LABELS: dict[int, str] = { - 1: "Text", - 3: "Position", - 4: "Node Info", + PORTNUM_TEXT: "Text", + PORTNUM_POSITION: "Position", + PORTNUM_NODEINFO: "Node Info", ... }Apply the same substitutions in
packet_category().🤖 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/analytics/packet_classifier.py` around lines 4 - 15, Update both PORTNUM_LABELS and the local mapping inside packet_category() to use the declared PORTNUM_* constants instead of bare numeric keys. Keep the existing labels and categorization behavior unchanged, ensuring both classification paths reference the same named port values.
🤖 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 @.gitattributes:
- Around line 19-21: Update the comment above the Linguist attributes in
.gitattributes to state that these paths are excluded from language statistics,
not diffs. Keep the existing linguist-vendored and linguist-documentation
configuration unchanged.
In `@src/meshchat/ui/monitor/monitor_page.py`:
- Around line 303-309: Update the packet-processing loop so direct RSSI values
are appended independently of the rx_snr check, while preserving separate
validation for each optional sample. In the direct signal card update logic,
guard SNR and RSSI median calculations independently using direct_snrs and
direct_rssis so RSSI-only samples are displayed.
- Line 310: Update the sender-missing check in the monitor ranking logic from a
truthiness test to an explicit None check, so sender_num=0 remains eligible for
packet, message, and direct-SNR rankings; keep normalization behavior consistent
with packet_ingestor.py’s None-only validation.
---
Nitpick comments:
In @.gitattributes:
- Around line 1-3: Commit the repository-wide line-ending renormalization after
adding the `* text=auto eol=lf` rule in `.gitattributes`; stage tracked files
with Git’s renormalization operation so files such as `README.md` and
`THIRD_PARTY_LICENSES.md` receive the normalized representation without
unrelated content changes.
In `@src/meshchat/analytics/packet_classifier.py`:
- Around line 4-15: Update both PORTNUM_LABELS and the local mapping inside
packet_category() to use the declared PORTNUM_* constants instead of bare
numeric keys. Keep the existing labels and categorization behavior unchanged,
ensuring both classification paths reference the same named port values.
🪄 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: 83d5cadd-0bb1-4bb8-9575-43b7acef703b
⛔ Files ignored due to path filters (9)
docs/screenshots/chat.pngis excluded by!**/*.pngdocs/screenshots/distribution-panels.pngis excluded by!**/*.pngdocs/screenshots/kpi-cards.pngis excluded by!**/*.pngdocs/screenshots/map.pngis excluded by!**/*.pngdocs/screenshots/monitor-dashboard.pngis excluded by!**/*.pngdocs/screenshots/node-inspector-signal.pngis excluded by!**/*.pngdocs/screenshots/node-inspector-telemetry.pngis excluded by!**/*.pngdocs/screenshots/nodes-page.pngis excluded by!**/*.pngdocs/screenshots/spectrum.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.gitattributespyproject.tomlsrc/meshchat/analytics/packet_classifier.pysrc/meshchat/app.pysrc/meshchat/py.typedsrc/meshchat/services/packet_ingestor.pysrc/meshchat/ui/monitor/monitor_page.pysrc/meshchat/version.pytests/test_version.py
All three findings valid. The two functional ones are pre-existing behaviour on main that consolidating the ranking passes made visible, not regressions introduced here — fixing them anyway since they are real. - Direct RSSI is now sampled independently of SNR. Both the old and new code nested the RSSI append under `rx_snr is not None`, so a direct packet carrying RSSI without SNR never reached the RSSI median. The SNR and RSSI cards are now guarded by their own sample lists too. - Ranking loops now test `sender is None` rather than truthiness. Node number 0 is a legitimate sender that the ingestor already tracks (it rejects only None), so a falsy check silently dropped it from every ranking. - Corrected the .gitattributes comment: linguist-vendored and linguist-documentation affect language statistics only; only linguist-generated collapses diffs. Also switched the RSSI display guard to `is not None` — 0 dBm is a legitimate reading that truthiness would have rendered as an em dash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds packaging/installer.iss — produces MeshChat-Setup-<version>.exe with a Start Menu shortcut, optional desktop icon, and a standard uninstaller registered in Windows' Apps & Features. Deliberately leaves %LOCALAPPDATA%\MeshChat (logs, chat history, node database) alone on uninstall — that's user data, not app files. Verified end-to-end before committing: silent install to a clean directory, launched the installed exe natively (a first attempt via Bash/MSYS threw a misleading ImportError that turned out to be a launch-environment artifact, not a real bug — confirmed by launching the identical files via native PowerShell instead, which worked cleanly), then ran the generated uninstaller and confirmed a clean removal. Unsigned — Windows SmartScreen will show an "unrecognized publisher" warning until it builds up download reputation. Real code signing needs a paid certificate; noted in ROADMAP.md as not started. Version bump to 0.1.1: the VERSION ImportError crash fix (Help > About, diagnostic copy, session export) has been on main since PR #2 but the v0.1.0-alpha release's binary predates it and still crashes on those paths. This is also the first release with a proper installer instead of a raw dist/ folder. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ten improvements. 219 tests pass,
ruff checkclean, verified against a live radio on COM11.🔴 The headline: a crash that shipped in v0.1.0-alpha
version.pyexported only__version__, but three call sites importedVERSION, which never existed:main_window._show_aboutmain_window._copy_diagnosticexport_service.export_session_jsonAll three are lazy imports inside functions, so nothing failed at startup, the app launched fine, and no test touched them. It only breaks when a user clicks the menu item — which is exactly the kind of bug that survives to a release.
Fix (1, 2):
version.pynow single-sources from installed package metadata so it can't drift frompyproject.toml, with a literal fallback for uninstalled source trees, and exports both spellings.Fix (3) — the systemic part: added a test that walks the AST of every source file and asserts every
from meshchat... import NAMEactually resolves. It validates 87 imports across 42 modules, and catches this entire class of bug regardless of how deeply nested the import is:Verified it has teeth: deleting
VERSIONmakes it fail and name all three call sites.Correctness
4.
get_nodes()handed out live mutable objects.NodeSnapshotis a mutable dataclass that the ingestion path updates in place, so callers never actually received a snapshot —NodeTableModelsorts bylast_heard, which could change after it was read. Now returns copies via the existing_copy_node_snapshothelper.5. Corrected a misleading docstring.
PacketIngestorclaimed "This object lives on a worker thread." It is nevermoveToThread'd — it's constructed on and stays on the GUI thread; Qt queuesraw_packetdelivery to it. The docstring now says so, and explains that the locks are deliberately defensive rather than load-bearing today. Anyone reasoning about locking from the old claim would have reached wrong conclusions.Robustness
6. Top-level excepthook. Release builds are windowed (
console=False), so an unhandled exception previously killed the app with no output anywhere the user could see — "it just closed" is the only bug report you'd ever get. Now logs the traceback and shows a dialog pointing at the log folder. Dialog failures can't mask the original exception.Performance
7.
_refresh_rankingsmade four full passes over up to 10k packets every 2 seconds — per-sender counts, best-SNR-per-node, text counts, and direct-RF medians each iterated the whole buffer independently. Consolidated into a single pass. Verified against live data that every ranking panel still populates identically.Maintainability
8. Named
PORTNUM_*constants replace magic1/3/67literals across the classifier, ingestor, and monitor page.9. Added
.gitattributes— every commit so far emitted a wall of CRLF warnings.10. Added a PEP 561
py.typedmarker and declared it inpackage-data.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests