diff --git a/reports/code-review-2026-02-24-response.md b/reports/code-review-2026-02-24-response.md new file mode 100644 index 0000000..475cbd1 --- /dev/null +++ b/reports/code-review-2026-02-24-response.md @@ -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 ``). | +| 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.