Skip to content

fix: address review findings and prepare v0.14.16 - #133

Merged
vriesd merged 8 commits into
mainfrom
fix/review-findings-0.14.16
Sep 5, 2026
Merged

fix: address review findings and prepare v0.14.16#133
vriesd merged 8 commits into
mainfrom
fix/review-findings-0.14.16

Conversation

@vriesd

@vriesd vriesd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Why

Home displayed an unquoted rm -f command that could delete unrelated files when an old executable path contained spaces. Its recording button also remained active during transcription and could cancel the current run without saving a transcript. Benchmark memory sampling omitted children spawned by worker threads.

Scope

  • Remove the terminal cleanup suggestion and retain the existing native removal action.
  • Disable Home's recording button during transcription and insertion, preserve retry after failure, and ignore duplicate toggles while a request is pending. Keep an acknowledged stop pending until status confirms capture has ended, including across stale status and polling errors.
  • Collect children from every Linux task and retain the existing parent-PID fallback when task data cannot be read.
  • Move microphone discovery and selection into the existing blocking-worker helper. Serialize microphone reads and writes at the shared frontend boundary so rapid selections preserve click order.
  • Add regression coverage and prepare version 0.14.16 with release notes.

Tradeoffs

Process sampling now reads one children file per thread. It still scans the full process table only when task data is unavailable.

Blast Radius

The changes affect Home controls, microphone commands, and benchmark tooling. Shortcut toggle semantics and serialized IPC payloads stay compatible. The release contains no dependency updates.

Verification

  • Regression tests failed against the original code before the fixes.
  • Browser reproduction changed from an enabled “Start recording” button and raw cleanup command to a disabled “Processing recording” button with no terminal command.
  • Frontend unit tests: 260 passed. Responsive and recording browser tests: 6 passed.
  • Python tests: 69 passed, including a live child spawned by a worker thread.
  • Rust 1.89 workspace tests under Xvfb: 504 passed, 14 ignored. Clippy, formatting, generated IPC checks, and the benchmark verification script passed.
  • Dependency audits passed with no npm vulnerabilities and only the repository's existing Cargo advisory warnings.

One existing responsive-layout test timed out during the first concurrent local run. The isolated case and the subsequent complete browser suite passed without a test change.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-05T02:51:18.786660Z d18003d Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@vriesd

vriesd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

PASS+NOTES

Independent verifier: reviewed fix/review-findings-0.14.16 at 23b82846a171b4e2ff9dbe32189516cf48cbc249 against origin/main / 79a7b51.

What I verified:

  • Final diff is clean and scoped: Home control gating, stale-install warning copy, process tree sampling, microphone command offload, benchmark CI wiring, and v0.14.16 metadata/changelog.
  • Unsafe stale-install terminal guidance is removed. origin/main had rm -f {paths.join(' ')} in frontend/src/home/HomeView.tsx; HEAD has no rm -f in Home and tests assert paths containing spaces/semicolons only expose the native Remove old copies action.
  • Home disables the recording orb during Transcribing and Injecting via processing, disabled, aria-busy, and label Processing recording in frontend/src/home/HomeView.tsx:27-52. frontend/src/app/useAppController.ts:52 and :110-120 also serialize in-flight toggles.
  • Browser evidence on the live Vite server http://127.0.0.1:4181: initial Start recording was enabled, after stop the Processing recording button was disabled with aria-busy="true", Transcribing locally… was visible, and the control re-enabled for a new recording. Screenshots: /tmp/echo-fix-release/home-current-initial.png, /tmp/echo-fix-release/home-current-processing.png.
  • Process sampling now reads every task's children file, dedupes/sorts child PIDs, and keeps the existing PPid fallback path in scripts/process_observation.py:89-171. The regression is wired into scripts/verify-stt-benchmark.sh:8-10.
  • Microphone listing/selection now return async Result<MicrophoneSnapshot, String> and run through crate::blocking::run_blocking in src-tauri/src/commands/devices.rs:54-95. The .await? shape preserves closure errors while naming worker panics; the IPC generator check passed, so the TypeScript invoke<MicrophoneSnapshot> calls still match the exported contract.
  • Version preparation is consistent for this release: workspace Cargo.toml and all workspace package lock entries are 0.14.16; CHANGELOG.md has a v0.14.16 section.

Checks I ran independently:

  • npm run test -- --run src/App.test.tsx in frontend: 22 passed.
  • npm run test:responsive -- --grep "the preview blocks" in frontend: 1 Playwright test passed.
  • One-off Playwright probe against http://127.0.0.1:4181: verified disabled processing state and restart path.
  • python3 scripts/test_process_observation.py: 3 passed.
  • ECHO_STT_BENCHMARK_SKIP_BUILD=1 ./scripts/verify-stt-benchmark.sh: passed, including process_observation.py --self-test, test_process_observation.py, benchmark self-test, probe self-test, and benchmark artifact verification.
  • cargo run -p echo-ipc-gen -- --check: passed.
  • cargo test -p echo-desktop commands::devices::tests::microphone -- --nocapture: 2 microphone async command tests passed.
  • cargo test -p echo --test inject_linux inject_nonce_into_owned_widget -- --nocapture: passed in isolation after the audit log showed one full-suite failure there.
  • git diff --check origin/main..HEAD: clean.

Notes / weak audit evidence:

  • /tmp/echo-fix-release/rust-tests-1.89.log is not green release evidence: it ends with inject_nonce_into_owned_widget failing with InjectUnconfirmed. I reran that exact test in isolation and it passed, and it is outside this diff, so I do not consider it a blocker from this review.
  • /tmp/echo-fix-release/benchmark-check.log is now green, but an earlier benchmark check had failed because Cargo built into a different local target directory while the script expected this worktree's target/debug/echo-desktop. Current cargo metadata reports target_directory as /home/vriesd/projects/echo-review-fixes/target, and the focused benchmark verification passed with the existing built binary.
  • /tmp/echo-fix-release/clippy.log contains a Rust 1.97 manual_is_multiple_of lint failure outside this patch; /tmp/echo-fix-release/clippy-1.89.log passes on the CI-pinned toolchain.
  • The requested no-comments subagent type was not available in my tool context. I applied the deslop/no-comments lens directly over the final diff; I did not find removable comments, TypeScript suppressions, abnormal any casts, or defensive workaround code introduced by this patch.

Verdict: PASS+NOTES. I found no release-blocking issue in the final diff. The only caveat is to avoid citing the stale failing rust-tests-1.89.log as proof of a fully green Rust suite unless a newer full-suite pass exists elsewhere.

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

Copy link
Copy Markdown

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: 23b82846a1

ℹ️ 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 thread frontend/src/app/useAppController.ts
@vriesd

vriesd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@vriesd

vriesd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

PASS

This supersedes my earlier PASS+NOTES verifier comment. Independent refresh reviewed PR #133 final head 6d89059fd16b4ab297539f5c5fec5ea7271e3819 against base 79a7b51, including the follow-up stop-pending race fix in 739ebc1..6d89059.

What changed since my previous verdict:

  • frontend/src/app/useAppController.ts now models stop as none | requesting | awaiting-status, keeps a ref-backed guard, and exposes stopPending.
  • The stop guard is set before the stop invoke, moves to awaiting-status after the invoke resolves, and is cleared only when a later observed status is no longer Recording.
  • frontend/src/home/HomeView.tsx disables the Home orb while stopPending, labels it Stopping recording, and keeps processing/transcribing states disabled.
  • Stop invoke rejection clears the stop guard, preserving retry behavior.

Independent race reasoning:

  • The previous P1 was valid because toggleRecording() could resolve while the immediate status refresh still returned Recording; clearing the guard in finally allowed a second stop click before backend transcription became visible.
  • The final code no longer depends on invoke completion or one post-click status refresh to unlock the control. Stale Recording statuses and status polling errors leave stopPending active.
  • useSerialPoll catches status refresh failures internally, calls onError, and resolves; therefore a status polling error after a successful stop does not hit the toggle catch block that releases retry state. Only toggleRecording() rejection clears stop state for retry.

Evidence I reran on final head:

  • node /tmp/echo-fix-release/stale-stop-browser.mjs --fixed: passed with {"disabledAfterStaleStatus":true,"calls":1} against http://127.0.0.1:4181.
  • npm run test -- --run src/App.test.tsx -t "stale statuses|rejected stop": 2 passed, 22 skipped.
  • Parent artifact /tmp/echo-fix-release/frontend-final-tests.log: full frontend Vitest suite passed, 24 files / 257 tests.
  • Parent artifact /tmp/echo-fix-release/frontend-final-build.log: typecheck and Vite production build passed.
  • Parent artifact /tmp/echo-fix-release/frontend-final-lint.log: frontend lint passed.
  • Parent artifact /tmp/echo-fix-release/responsive-final.log: full Playwright responsive suite passed, 6 tests.
  • git diff --check 79a7b51..HEAD: clean.

Prior release evidence still stands:

  • Unsafe Home rm -f stale-install guidance is removed and covered by tests.
  • Home remains disabled during backend Transcribing and Injecting.
  • Process observation now samples all task children and is wired into scripts/verify-stt-benchmark.sh.
  • Microphone list/selection commands are async and offload enumeration/config work through run_blocking.
  • v0.14.16 version and changelog are consistent.
  • Pinned Rust/Xvfb evidence in /tmp/echo-fix-release/rust-tests-1.89-xvfb.log supersedes the earlier local focus-sensitive inject_linux failure.

Verdict: PASS. I found no remaining release-blocking issue in the final diff.

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

Copy link
Copy Markdown

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: 6d89059fd1

ℹ️ 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 thread src-tauri/src/commands/devices.rs
@vriesd

vriesd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@vriesd

vriesd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

PASS

This supersedes my earlier verifier comments. Independent refresh reviewed PR #133 final head d18003d79f29312d2f182c2fe7d4cf4629754fed against base 79a7b51, including the microphone ordering fix in 6d89059..d18003d.

New microphone-order verification:

  • frontend/src/tauri.ts now serializes getMicrophones() and setMicrophone() through one promise queue.
  • The queue covers reads and writes from both Home and Settings because it lives at the shared exported tauri wrapper layer.
  • Each queued operation captures the configured adapter before enqueueing, so queued work stays with the adapter that accepted it.
  • configureDesktopApi() resets the queue, which keeps tests and adapter replacement from inheriting stale pending operations.
  • Rejections do not poison the queue: the queue tail maps both fulfillment and rejection to undefined, while the original request promise still rejects to its caller.

Evidence I reran on final head:

  • node /tmp/echo-fix-release/microphone-order-browser.mjs --fixed against fresh server http://127.0.0.1:4182: passed with {"dispatchedBeforeRelease":1,"finalSelection":"USB Microphone"}.
  • npm run test -- --run src/tauri.test.ts: 7 passed.
  • Parent artifact /tmp/echo-fix-release/frontend-final-tests.log: full frontend Vitest suite passed, 24 files / 260 tests.
  • Parent artifact /tmp/echo-fix-release/frontend-final-build.log: typecheck and Vite production build passed.
  • Parent artifact /tmp/echo-fix-release/frontend-final-lint.log: frontend lint passed.
  • Parent artifact /tmp/echo-fix-release/responsive-final.log: full Playwright responsive suite passed, 6 tests.
  • git diff --check 79a7b51..HEAD: clean.

Prior verified release evidence still stands:

  • Home no longer exposes unsafe rm -f stale-install guidance.
  • Home disables the recording control during Transcribing and Injecting.
  • The stop flow now stays disabled through stale post-stop Recording statuses and status polling errors, and only releases on phase transition or stop invoke rejection.
  • Process observation samples all task children and is wired into the STT benchmark verification script.
  • Microphone enumeration and selection are offloaded through async worker commands on the backend.
  • v0.14.16 version and changelog are consistent.
  • Pinned Rust/Xvfb evidence in /tmp/echo-fix-release/rust-tests-1.89-xvfb.log supersedes earlier local focus-sensitive Rust noise.

Notes:

  • /tmp/echo-fix-release/mic-order-red.log is valid red-first evidence: before the queue, both selections dispatched immediately and the tests failed.
  • /tmp/echo-fix-release/mic-order-browser-before.log timed out on the stale 4181 HMR setup; the fresh 4182 fixed probe is the browser evidence I used for this verdict.

Verdict: PASS. I found no remaining release-blocking issue in the final diff.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: d18003d79f

ℹ️ 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".

@vriesd
vriesd merged commit 84b6c1b into main Sep 5, 2026
16 checks passed
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