Add voice notes to desktop messages - #6978
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
🔐 Codex Security Review
|
|
🤖 Voice-note interaction snapshots captured by the focused desktop E2E test. RecordingThe recorder replaces the composer toolbar with the timer, full-width live waveform, discard action, and stop control.
Composer previewThe finished recording returns to the composer with playback, scrubbing, duration, and a hover remove action.
PlayingThe sent voice note renders as a compact rich card with a morphing play/pause control and waveform progress.
Playback speedHovering the duration reveals the fixed-width playback-speed pill.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b1ce0a19
ℹ️ 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".
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb53da623b
ℹ️ 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".
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36e5858f7b
ℹ️ 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".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: b49675894b39f87215e4dfc8c1ad4a3c28c6097e..36e5858f7bcb53e1fbf778a1cfaef7d7a6bd57bf (exact head 36e5858f7bcb53e1fbf778a1cfaef7d7a6bd57bf)
Risk: high — this adds a user-visible messaging flow spanning browser media capture, renderer state, privileged Tauri IPC, local ffmpeg transcoding, upload/send recovery, and cross-platform packaging.
Blocking finding
-
desktop/src/features/messages/ui/useMentionSendFlow.ts:599-601,627-629now adds a toast containing the raw send error while the restored expanded-DM composer also renders that error inline. The unchanged assertion atdesktop/tests/e2e/channels.spec.ts:1005-1036usesgetByText('Mock first DM send failed.'), which now resolves to both surfaces. The requiredDesktop Smoke E2E (1)job fails strict mode on the initial attempt and both retries (315 passed, 2 skipped, 1 failed). This is caused by the PR: base restored the composer but did not add the toast.Author action: decide the intended error-presentation contract, then make the smoke regression target that surface explicitly (and remove either presentation if duplication is not intended). Re-run the complete Desktop Smoke gate at the resulting immutable head.
Verification owner: author for the patch; CI for the complete gate; code reviewer for the new-head delta.
Behavior/contracts traced
The review followed MediaRecorder → Web Audio WAV creation → exclusive queued attachment → Rust raw-byte IPC → voice filename recognition → ffmpeg canonical MP4 → validated media upload → relay send/recovery → imeta rendering and inline playback. It also checked recorder cancellation on draft changes/unmount, microphone and AudioContext cleanup, upload cancellation/epoch handling, failed-send restoration, attachment exclusivity, filename sanitization, metadata-stripping transcode, media-output validation, macOS microphone purpose text, GIF exclusion with emoji retained, player exclusivity, keyboard/focus semantics, labeled scrubbing, and reduced-motion behavior. No additional concrete ownership, IPC/runtime, relay, persistence/recovery, security, accessibility, or product defect was established in the reviewed diff.
Validation at the pinned clean head
git diff --check b4967589..36e5858f: pass.pnpm typecheck: pass.- 73 directly affected frontend tests: pass.
- GitHub CI: Unit Tests, Rust Lint, Desktop Core, Desktop E2E Relay, both Desktop E2E Integration shards, Windows Rust, macOS Desktop Build, Security, and three of four smoke shards passed;
Desktop Smoke E2E (1)and aggregateDesktopfailed for the blocking regression above. - The full local Desktop JS and Rust workspace suites were attempted independently but exceeded the 10-minute review windows without complete summaries; these are confidence gaps, not additional defects.
Manual/native evidence: not independently run. The PR screenshots are mock-E2E artifacts, not proof of WKWebView's OS permission prompt, physical microphone capture, real ffmpeg execution, or packaged macOS/Linux/Windows behavior.
Residual risk / external gates: after the blocking fix, exact-head packaged native voice-note evidence remains appropriate for microphone, permission, transcode, and playback behavior on supported platforms. Verification owner: release/manual-validation owner. The exact-range Codex security review is still reported as required and skipped pending its authorization workflow; that gate owns its own completion unless it returns a PR defect.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 @jedwards27 Addressed your blocking smoke finding in ec707e4 and published exact head 959c66f. The regression now targets the restored inline error and the transient send-error toast as separate intended surfaces, avoiding the ambiguous page-wide text match. The focused smoke test passes after merging current main; all pre-push gates also passed, including 5,764 desktop tests, typechecking, desktop/Tauri checks, and branch-skew validation. Please re-review when ready. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: ed11c8d8bf0a17402be5cf243724f89471530d2f..959c66f03c1452f69352e06d6d277571fa6b3455 (exact head 959c66f03c1452f69352e06d6d277571fa6b3455)
Risk: high — user-visible microphone/media lifecycle, Tauri IPC, upload/send failure recovery, and platform packaging.
Behavior/contracts traced: recording permission/cancel/preview/send/playback; recorder, track, and AudioContext cleanup; stale async fencing; draft restoration; raw-byte IPC and media validation/sanitization; relay MP4 metadata/rendering; accessibility and keyboard controls; macOS microphone packaging; exact-head smoke gates.
Findings: no unresolved author-actionable defect. The prior required-gate blocker is fixed: the send-failure regression now scopes the durable inline recovery error separately from the transient toast, and the focused exact-head test passes. Product/UI and systems lanes found no additional material defect.
Desktop Smoke E2E (3) failed on the unchanged message-feedback-snapshots.spec.ts:97 profile-hover color assertion: it compares a transition-sampled channel alpha with the settled profile alpha and failed with run-dependent values across retries. The PR changes neither that test nor the sidebar/profile/channel-hover implementation in the reviewed diff. This is classified as unrelated gate instability, not author rework.
Author action: none.
Verification owner: CI/maintainers for stabilizing the unrelated hover-transition assertion; reviewer/release QA for optional packaged native microphone and physical capture observation.
Validation: exact-head desktop unit suite passed 5,764 tests. Focused send-error regression passed; all four smoke-tagged voice-note journeys and the adjacent first-DM-send-failure regression passed. Exact-head Desktop Core, Smoke shards 1/2/4, Rust Lint, Windows Rust, macOS Build, Release Candidate, E2E Integration, and E2E Relay passed. Both lanes rechecked the immutable head and clean worktrees.
Manual/native evidence: interaction snapshots covered recording, preview, sent/paused playback, and speed controls. Native OS microphone prompts, physical capture quality, and the packaged Tauri journey were not directly observed.
Residual risk: real-device/native media behavior remains unwitnessed; the unrelated smoke-3 gate remains red and CI-owned.
— :bot: Jude’s code review agent
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Verdict: REQUEST CHANGES
Reviewed: ed11c8d8bf0a17402be5cf243724f89471530d2f..959c66f03c1452f69352e06d6d277571fa6b3455 (exact head 959c66f03c1452f69352e06d6d277571fa6b3455)
Blocking finding
- Treat microphone acquisition as a non-idle recorder state.
start()installs the session before awaitinggetUserMedia, but leaves the public status asidle; it becomesrecordingonly after recorder construction at L179-L180. During a delayed OS permission/device prompt, the composer therefore leaves submission and attachment paths enabled because those gates usestatus === "idle"(send gate, attachment gates). A user can click Record, send or attach while acquisition is pending, then grant permission and have capture begin unexpectedly in the changed/cleared composer. Represent acquisition synchronously as a non-idlerequestingstate, make it cancellable/visible, include it in the existing gates, and add a deferred-getUserMediaregression test.
Non-blocking hardening
The intended recorder path has a five-minute UI auto-stop. The native filename-routed voice-note command nevertheless accepts arbitrary-size renderer bytes and buffers/writes/transcodes them before relay validation. Add a native byte ceiling before prepare_voice_note_for_upload so the privileged boundary fails closed even if invoked outside the intended recorder flow.
I traced capture permission and cancellation, draft switching, upload/send recovery, raw-byte IPC, MIME detection, temporary-file cleanup, ffmpeg argument/environment restrictions and timeout, canonical MP4/imeta round-trip, mixed rendering behavior, playback controls, and accessibility. I did not run PR code; this was the scheduler-required read-only exact-head source/diff review. Current voice-note/core/native checks are green. The remaining Desktop Smoke E2E (3) failure is in an unchanged hover-transition snapshot and is not attributed to this PR.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🤖 @wesbillman Your blocking permission-acquisition finding and native hardening note are addressed. Microphone acquisition is now a visible, cancellable non-idle state that locks send/attachment/finish paths, with a deferred-getUserMedia regression test; native voice-note input is capped at 128 MiB before detection, disk I/O, or transcoding, with boundary coverage. The branch is merged with current main at 29e3699. Post-merge validation passed: focused recorder tests (3/3), voice-note smoke E2E (4/4), TypeScript, file-size guard, Rust Clippy with warnings denied, full desktop tests (5,817/5,817), and full Tauri workspace tests. Please re-review the new head. Jude’s prior send-error fix and exact-head approval remain represented in the merged tree. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29e36994fd
ℹ️ 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".
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcfd47578a
ℹ️ 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".
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cea3830781
ℹ️ 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".
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 702fc1fc66
ℹ️ 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".
| export const VOICE_NOTE_MAX_DURATION_SECONDS = 5 * 60; | ||
|
|
||
| export function isVoiceNoteFile(file: File): boolean { | ||
| const filename = file.name.toLowerCase(); |
There was a problem hiding this comment.
When a user uploads an ordinary audio file such as an MP3 or a non-recorder WAV, this broad MIME check classifies it as a voice note. The same predicate is used by useComposerVoiceNote to disable paperclip, paste, drop, and GIF ingress, so users cannot add another attachment afterward and receive the misleading “A voice note must be the only attachment” error. Keep generic audio eligible for the audio player, but use the recorder-specific filename predicate for voice-note exclusivity.
Useful? React with 👍 / 👎.
3 participants
Summary
Testing
just cipnpm exec playwright test tests/e2e/voice-note.spec.tsScreenshots
Focused snapshots are attached below.