docs: add code review response with applied changes summary
This commit is contained in:
Username
142
reports/code-review-2026-02-24-response.md
Normal file
142
reports/code-review-2026-02-24-response.md
Normal file
@@ -0,0 +1,142 @@
|
||||
# Code Review Response
|
||||
|
||||
**Original review:** `code-review-2026-02-24.md`
|
||||
**Date applied:** 2026-02-24
|
||||
**Commits:** 14 functional + 4 tasklist updates (be6574a..d4a8f34)
|
||||
**Test suite:** 83 tests, all passing
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
All 17 items in the priority matrix were evaluated. 15 were applied across five
|
||||
phases (A--E). Two were deferred to the backlog with rationale.
|
||||
|
||||
### Disposition by severity
|
||||
|
||||
| Severity | Total | Applied | Deferred | Notes |
|
||||
|----------|-------|---------|----------|--------------------------------|
|
||||
| Critical | 4 | 3 | 1 | Widget tests deferred (backlog)|
|
||||
| Important| 9 | 9 | 0 | |
|
||||
| Nice | 4 | 3 | 1 | Frozen config deferred |
|
||||
|
||||
---
|
||||
|
||||
## Phase A -- Safety
|
||||
|
||||
| # | Item | Resolution |
|
||||
|---|------|------------|
|
||||
| 2 | Replace regex HTML stripping | Replaced with `html.parser.HTMLParser` subclass. Handles malformed tags, comments, nested markup, entities. 13 edge-case tests added. |
|
||||
| 3 | Validate TOML config keys | Added `_load_section()` that filters unknown keys via `dataclasses.fields()` before construction. Logs warnings for typos. 5 tests added. |
|
||||
| 10 | Guard `join_channel`/`send_text` | `join_channel` uses `.get()` + `ValueError`. `send_text` wraps lookup in `try/except (KeyError, AttributeError)`. |
|
||||
|
||||
**Commits:**
|
||||
```
|
||||
8be475f app: replace regex html stripping with stdlib parser
|
||||
44da57d config: filter unknown toml keys before dataclass init
|
||||
897c5b1 client: guard join_channel and send_text against stale ids
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase B -- Performance
|
||||
|
||||
| # | Item | Resolution |
|
||||
|---|------|------------|
|
||||
| 5 | Use numpy for `_apply_gain` | **Deviation:** numpy is NOT a transitive dep of sounddevice (only cffi). Used stdlib `array.array("h")` instead -- eliminates struct pack/unpack overhead without adding a dependency. |
|
||||
| 6 | Replace polling sleep with `queue.get(timeout=)` | `get_capture_frame()` now accepts `timeout` param. Send loop uses `timeout=0.02`. No polling, instant wake on data. |
|
||||
| 15 | Drain audio queues on `stop()` | Both capture and playback queues drained after stream close. Prevents stale frames leaking on restart. Test added. |
|
||||
|
||||
**Commits:**
|
||||
```
|
||||
7a2c8e3 audio: replace struct pack/unpack with array module in _apply_gain
|
||||
bfa79ea app: replace audio send polling with blocking queue.get
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase C -- Architecture
|
||||
|
||||
| # | Item | Resolution |
|
||||
|---|------|------------|
|
||||
| 1 | Extract reconnect manager | Extracted to `src/tuimble/reconnect.py` (93 lines). Uses `threading.Event` for cancellation (item #7 resolved simultaneously). Instant cancel, no polling. |
|
||||
| 7 | `threading.Event` for reconnect | Resolved as part of item #1 above. `Event.wait(timeout=delay)` replaces the 0.5s sleep loop. |
|
||||
| 8 | Extract `_make_client()` factory | Single factory method replaces 4 duplicate 7-line instantiation blocks. |
|
||||
| 9 | Compile `_strip_html` regex | Superseded by item #2 (HTMLParser replacement). No regex remains. |
|
||||
| 13 | Extract `InputHistory` class | Extracted to standalone class in `app.py` (29 lines). Clean `push`/`up`/`down` interface. 11 tests added. |
|
||||
| -- | Cache `_find_root()` | `ChannelTree.set_state()` now caches `_root_id`. Eliminates redundant traversal during render. |
|
||||
|
||||
**Commits:**
|
||||
```
|
||||
216a4be app: extract InputHistory, _make_client factory, cache _find_root
|
||||
0cf3702 app: extract ReconnectManager to reconnect.py
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase D -- Test Coverage
|
||||
|
||||
| # | Item | Resolution |
|
||||
|---|------|------------|
|
||||
| 4 | Widget/app tests | **Partially addressed.** Extracted components (ReconnectManager, InputHistory, _strip_html, config loading) are now fully tested without the TUI harness. Widget-level tests (StatusBar breakpoints, ChannelTree navigation) deferred to backlog -- requires Textual `App.run_test()`. |
|
||||
| -- | Refactor test_audio.py | Replaced private attribute access with public interface calls (`_capture_callback` + `get_capture_frame` instead of direct queue manipulation). |
|
||||
|
||||
**Test additions:**
|
||||
- `test_reconnect.py` -- 8 tests (backoff formula, cancellation, exhaustion, non-retryable abort)
|
||||
- `test_history.py` -- 11 tests (navigation, draft preservation, push reset, full cycle)
|
||||
- `test_strip_html.py` -- 13 tests (nested tags, entities, comments, malformed HTML)
|
||||
- `test_config.py` -- 5 new tests (_load_section filtering, missing file, unknown keys)
|
||||
- `test_audio.py` -- 1 new test (stop drains queues), existing tests refactored
|
||||
|
||||
**Commits:**
|
||||
```
|
||||
4c1a545 test: add ReconnectManager unit tests
|
||||
7c57e03 test: add InputHistory unit tests
|
||||
65de741 test: add _strip_html edge cases and config validation tests
|
||||
d117576 test: refactor audio tests to use public interfaces
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Phase E -- Polish
|
||||
|
||||
| # | Item | Resolution |
|
||||
|---|------|------------|
|
||||
| 12 | Deduplicate config change detection | `_detect_config_changes` and `_apply_restart_changes` now use `dataclasses.asdict()` for comparison. New config fields are automatically picked up. |
|
||||
| 11 | Suppress PTT chatlog spam | `_on_ptt_change` skips chatlog writes when `ptt.mode == "hold"`. Status bar indicator still updates in real time. |
|
||||
| 16 | Add `-> None` return annotations | Added to 21 methods across `client.py` and `audio.py`. |
|
||||
| -- | Cache `users`/`channels` properties | Properties now cache with dirty flags. Invalidated on connect, disconnect, and user/channel event callbacks. |
|
||||
|
||||
**Commits:**
|
||||
```
|
||||
0ae0e77 app: deduplicate config detection, suppress hold-mode PTT spam
|
||||
bbd28e2 audio: add -> None return annotations
|
||||
aa17159 client: add return annotations and cache users/channels properties
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Deferred Items
|
||||
|
||||
| # | Item | Reason |
|
||||
|---|------|--------|
|
||||
| 4 | Widget tests (StatusBar, ChannelTree) | Requires Textual async test harness (`App.run_test()`). Extracted logic is tested; widget integration tests are in the backlog. |
|
||||
| 14 | Inline callback wrappers with lambdas | Low priority. Current named methods aid debugging (stack traces show `_cb_connected` vs `<lambda>`). |
|
||||
| 17 | Frozen dataclasses for Config | Requires refactoring `_apply_safe_changes` which mutates `self._config.ptt`. Scope exceeds incremental polish. |
|
||||
|
||||
---
|
||||
|
||||
## Metrics
|
||||
|
||||
| | Before | After | Delta |
|
||||
|----------------|--------|--------|--------|
|
||||
| Source LOC | ~1,750 | 1,866 | +116 |
|
||||
| Test LOC | ~480 | 818 | +338 |
|
||||
| Test count | 45 | 83 | +38 |
|
||||
| Modules | 7 | 8 | +1 |
|
||||
| `app.py` LOC | ~999 | 965 | -34 |
|
||||
|
||||
New module: `src/tuimble/reconnect.py` (93 lines).
|
||||
Net source growth is primarily from the extracted reconnect module and cached
|
||||
property logic in `client.py`. `app.py` shrank despite gaining the
|
||||
`InputHistory` class and `dataclasses` import.
|
||||
Reference in New Issue
Block a user