Skip to content

Fix all 20 code review issues#3

Open
cvalentine99 wants to merge 1 commit intomainfrom
claude/code-review-functionality-RYs9F
Open

Fix all 20 code review issues#3
cvalentine99 wants to merge 1 commit intomainfrom
claude/code-review-functionality-RYs9F

Conversation

@cvalentine99
Copy link
Owner

Critical fixes:

  1. GPS: Thread-safe _append_log() using pyqtSignal
  2. GPS: Mode selection uses combo as single source of truth
  3. GPS: Initialize serial_port in init
  4. LoRa: Add timeout protection to polling (max 5 consecutive errors)
  5. LoRa/RTL-SDR: get_config() returns configured state, not runtime
  6. RTL-SDR: Capture loop now updates power/peak UI labels
  7. RTC: Fix shell command injection - use subprocess with args list
  8. RTC: Timer cleanup in cleanup() method
  9. USB: Graceful handling when pyusb not installed
  10. Main: Config reinit detects all relevant config changes

Medium fixes:
11. GPS: Satellite count display syncs when either value changes
12. Config dialog: Gain value properly loaded from config
13. GPS: Time parsing redundancy fixed
14. Main: Status bar messages protected during init period
15. LoRa: Double timestamp in logging fixed
16. RTL-SDR: Frequency preset resets to Custom on manual change
17. RTC: DS3231 weekday off-by-one fixed (uses 1-7, not 0-7)
18. Config dialog: I2C address validation with warning log

Security improvements:

  • All os.system/os.popen calls replaced with subprocess
  • Shell command injection vulnerability eliminated

Critical fixes:
1. GPS: Thread-safe _append_log() using pyqtSignal
2. GPS: Mode selection uses combo as single source of truth
3. GPS: Initialize serial_port in __init__
4. LoRa: Add timeout protection to polling (max 5 consecutive errors)
5. LoRa/RTL-SDR: get_config() returns configured state, not runtime
6. RTL-SDR: Capture loop now updates power/peak UI labels
7. RTC: Fix shell command injection - use subprocess with args list
8. RTC: Timer cleanup in cleanup() method
9. USB: Graceful handling when pyusb not installed
10. Main: Config reinit detects all relevant config changes

Medium fixes:
11. GPS: Satellite count display syncs when either value changes
12. Config dialog: Gain value properly loaded from config
13. GPS: Time parsing redundancy fixed
14. Main: Status bar messages protected during init period
15. LoRa: Double timestamp in logging fixed
16. RTL-SDR: Frequency preset resets to Custom on manual change
17. RTC: DS3231 weekday off-by-one fixed (uses 1-7, not 0-7)
18. Config dialog: I2C address validation with warning log

Security improvements:
- All os.system/os.popen calls replaced with subprocess
- Shell command injection vulnerability eliminated
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1408cf09d5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +168 to 172
# Use combo box as single source of truth for mode
if self.mode_combo.currentIndex() == 0:
self._connect_gpsd()
else:
self._connect_serial()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Honor GPS config mode during auto-connect

On startup the GPS panel now chooses the backend solely from self.mode_combo (lines 168‑172) and never consults the loaded config, even though initialize() auto-connects when gps.enabled is true. The combo default is always gpsd, so a config file specifying "mode": "serial" is ignored and the app tries gpsd instead, failing to use the configured serial receiver until the user manually switches modes. This regresses config-driven startup behavior.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants