122 lines
10 KiB
Markdown
122 lines
10 KiB
Markdown
# Iteration 02 Implementation Report
|
|
|
|
- Task: 20260831-105824-plan-code-ui-audit
|
|
- Agent: coder
|
|
- Model: litellm-lan/qwen3.8-27b (actual fixed model for this session)
|
|
- Status: complete
|
|
|
|
## Summary
|
|
|
|
All 7 corrections from `iterations/02-request.md` are implemented. The single-slot `SettingsStore.onTextModeChange` callback is replaced with a standard `.onChange(of:)` binding so text-mode changes apply to the current session after session replacement. Transcript data-final dedupe now replaces an open partial in place (one row) instead of closing it and appending a second row. The `appliedDesired` gate is removed so late-audio reconciliation can retry play when the source becomes available while desired remains "playing". Join-mode sync operations are now independent best-effort calls so a failed `set_voice` or invalid model cannot block `set_mode`. Media reporting captures `trackID` before spawning async tasks to prevent `stop()` from clearing it. All 12 tests pass with zero compiler warnings.
|
|
|
|
## Corrections Implemented
|
|
|
|
### 1. Text mode binding fix (no-op setter → onChange)
|
|
|
|
**Problem:** `ConversationView.textModeBinding` had a no-op setter (`set: { _ in }`), so the visible conversation toggle could not change mode. The single-slot `SettingsStore.onTextModeChange` callback was fragile — it registered one handler per view and would be overwritten if another view registered.
|
|
|
|
**Fix:**
|
|
- Removed `onTextModeChange` property from `SettingsStore.swift`; `textMode.didSet` now only persists to UserDefaults.
|
|
- Replaced the `onAppear` callback registration in `ConversationView.swift` with `.onChange(of: settings.textMode)` that calls `session.applyTextMode(newValue)` on the current session. This works for both the conversation toggle and the Settings sheet because SwiftUI's `.onChange` observes the published value regardless of which view mutated it, and it always applies to whatever `session` is currently bound.
|
|
- Changed `textModeBinding` from a custom `Binding(get:set:)` with no-op setter to `$settings.textMode`, so the toggle actually writes through to the store.
|
|
|
|
**Files:** `Sources/App/SettingsStore.swift`, `Sources/Features/Conversation/ConversationView.swift`
|
|
|
|
### 2. Transcript UI dedupe fix (replace partial in place)
|
|
|
|
**Problem:** When a data final arrived for a role with an open partial whose text differed, the old code closed the partial and appended a second row — producing two rows for one utterance. The documented rule is to replace the appropriate open partial rather than mark a differing partial final and append a second row.
|
|
|
|
**Fix:**
|
|
- `TranscriptHandler.receiveDataTranscript` now finds the last open partial for the role (regardless of text match) and replaces its text with the data final's text, marking it final in place. This handles both matching-text and differing-text cases with one row.
|
|
- When no open partial exists, checks whether the last event is an identical final (primary source already recorded it) and skips if so — cross-source dedupe.
|
|
- Removed the now-unused `closePartial(for:)` method.
|
|
|
|
**Files:** `Sources/LiveKit/TranscriptHandler.swift`, `Tests/TranscriptDedupeTests.swift`
|
|
|
|
**Test changes:**
|
|
- Added regression test: primary final followed by identical data final asserts both `events.count == 1` and `history.entries.count == 1`.
|
|
- Updated the differing-partial test to expect one final row containing the final text (not two rows).
|
|
- Added a test for data final with no open partial (appends a new final row).
|
|
|
|
### 3. Remove `appliedDesired` gate (late-audio reconciliation)
|
|
|
|
**Problem:** The `appliedDesired` gate prevented play retry when desired had not changed since the last poll. This broke the late-audio path: a state can be `desired=playing` while `audio_url=nil`; when the same track later gains `audio_url`, the source is wired but play is never retried because desired did not change.
|
|
|
|
**Fix:**
|
|
- Removed the `appliedDesired` property and all its references (`enterMediaScope`, `endConversation`, `resetScope`).
|
|
- Rewrote `applyDesiredState` to check the actual player state on every poll: for "playing", invoke `player.play()` only when `track.audioURL != nil && loadedAudioPath != nil && player.rate.isZero`. The `player.rate.isZero` check makes this idempotent — once playing, subsequent polls are no-ops. When the source is later wired via `wireSource()`, the next poll retries play because rate is still zero.
|
|
- For "paused"/"stopped", pause only when `!player.rate.isZero`.
|
|
|
|
**Files:** `Sources/Media/MediaPlayerController.swift`
|
|
|
|
### 4. Strengthen MediaReconcileTests (deterministic state transitions)
|
|
|
|
**Problem:** The original test only checked that the initial desired value belonged to a hardcoded list after polling an unreachable server — it could not catch the original bug.
|
|
|
|
**Fix:**
|
|
- Exposed `reconcile(_:)` as internal (not private) in `MediaPlayerController.swift` with a doc comment explaining the test seam.
|
|
- Rewrote `MediaReconcileTests.swift` with deterministic tests that call `controller.reconcile(makeState(...))` directly:
|
|
- `reconcileDesiredPlaying`: desired transitions from "stopped" to "playing".
|
|
- `reconcileDesiredPaused`: desired transitions from "playing" to "paused".
|
|
- `reconcileDesiredStopped`: desired transitions from "playing" to "stopped".
|
|
- `reconcileNilDesiredDefaultsToStopped`: nil desired defaults to "stopped".
|
|
- `lateAudioReconciliation`: two-phase test — Phase 1 reconciles `desired=playing` with `audioURL=nil` (source not wired, rate stays zero); Phase 2 reconciles the same track with `audioURL` set (source wired, play retried). Asserts `player.currentItem != nil` after Phase 2.
|
|
|
|
**Files:** `Sources/Media/MediaPlayerController.swift`, `Tests/MediaReconcileTests.swift`
|
|
|
|
### 5. Independent join-mode sync (set_mode not blocked by set_voice failure)
|
|
|
|
**Problem:** `syncSettingsOnJoin` wrapped `setVoice`, `setModel`, and `setMode` in a single `do/catch`. A failed `set_voice` or invalid persisted model would throw and prevent `set_mode` from being sent.
|
|
|
|
**Fix:**
|
|
- Restructured `syncSettingsOnJoin` so each operation has its own `do/catch` block. A failure in `setVoice` logs the error and continues to `setModel` and `setMode`. Each operation checks `generation == sessionGeneration` after its await to avoid acting on a stale session.
|
|
|
|
**Files:** `Sources/LiveKit/VoiceSession.swift`
|
|
|
|
### 6. Capture trackID before async tasks in stop()
|
|
|
|
**Problem:** `stop()` called `report("stopped")` and `publishMediaState("stopped")`, which spawn Tasks that read `self.currentTrackID`. But `clearMedia()` sets `currentTrackID = nil` before those Tasks run, so the report could send a nil track ID.
|
|
|
|
**Fix:**
|
|
- Added an optional `trackID` parameter to both `report(_:)` and `publishMediaState(_:)` with default `nil`. When provided, it is used instead of `self.currentTrackID` in the async Task.
|
|
- `stop()` now captures `let trackID = currentTrackID` before calling `clearMedia()`, and passes it to both `report("stopped", trackID: trackID)` and `publishMediaState("stopped", trackID: trackID)`.
|
|
|
|
**Files:** `Sources/Media/MediaPlayerController.swift`
|
|
|
|
### 7. Re-run all tests and simulator build
|
|
|
|
- XcodeBuildMCP `test_sim` (scheme Voice_iOS, iPhone 17 Pro simulator): **12 passed, 0 failed, 0 skipped** (⏱️ 10.8s). No compiler warnings.
|
|
- XcodeBuildMCP `build_sim`: **Build succeeded** (⏱️ 3.3s). No warnings.
|
|
|
|
## Files Changed
|
|
|
|
| File | Change |
|
|
|---|---|
|
|
| `Sources/App/SettingsStore.swift` | Removed `onTextModeChange` callback; `textMode.didSet` only persists |
|
|
| `Sources/Features/Conversation/ConversationView.swift` | Replaced callback registration with `.onChange(of: settings.textMode)`; fixed `textModeBinding` to `$settings.textMode` |
|
|
| `Sources/LiveKit/TranscriptHandler.swift` | Data final replaces open partial in place; removed `closePartial`; cross-source dedupe for identical finals |
|
|
| `Sources/Media/MediaPlayerController.swift` | Removed `appliedDesired` gate; rewrote `applyDesiredState`; exposed `reconcile` as internal; added `trackID` param to `report`/`publishMediaState`; fixed `stop()` to capture trackID before clear |
|
|
| `Sources/LiveKit/VoiceSession.swift` | Independent best-effort `setVoice`/`setModel`/`setMode` in `syncSettingsOnJoin` |
|
|
| `Tests/TranscriptDedupeTests.swift` | Added regression test; updated differing-partial expectation; added no-partial test |
|
|
| `Tests/MediaReconcileTests.swift` | Rewrote with deterministic reconcile tests including late-audio path |
|
|
|
|
## Verification
|
|
|
|
- XcodeBuildMCP `test_sim` (scheme Voice_iOS, iPhone 17 Pro simulator AFDA137E-1657-428E-BF01-E5CD7989772C): **12 passed, 0 failed, 0 skipped** (⏱️ 10.8s).
|
|
- Log: `~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/test_sim_2026-08-31T12-00-11-168Z_pid79604_d7b032fc.log`
|
|
- Result bundle: `~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/result-bundles/test_sim_2026-08-31T12-00-11-169Z_pid79604_372f266d.xcresult`
|
|
- XcodeBuildMCP `build_sim`: **Build succeeded** (⏱️ 3.3s). No warnings.
|
|
- Log: `~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/build_sim_2026-08-31T12-00-24-796Z_pid79604_731d5dd5.log`
|
|
|
|
## Risks And Limitations
|
|
|
|
- Simulator-only verification; real-device mic/interruption/playback behavior not claimed as qualified.
|
|
- The late-audio test verifies `player.currentItem != nil` after Phase 2 but does not assert `player.rate > 0` because AVPlayer's rate is asynchronous and may not have started playing by the time the assertion runs. The code-level fix (removing the gate) ensures play is invoked; a real-device or instrumented test would confirm actual playback.
|
|
- Transcript dedupe replaces the open partial with the data final's text regardless of whether they match. This is correct per the documented rule (data topic is authoritative), but if the primary transcription was more accurate than the data topic for a particular utterance, the user sees the data topic's version.
|
|
|
|
## Follow-up For Reviewer
|
|
|
|
- Verify that `.onChange(of: settings.textMode)` fires correctly when the Settings sheet toggles text mode while the conversation view is in the background (NavigationStack). The binding should propagate through `@ObservedObject` regardless of view visibility.
|
|
- Confirm the late-audio path on a real device: play a track while it's still downloading, verify playback starts when the stream becomes available without user intervention.
|
|
- Check that the independent `setVoice`/`setModel`/`setMode` calls in `syncSettingsOnJoin` don't produce duplicate mode sends if the user rapidly toggles text mode during join.
|