refactor: improve transcription UX and DX#1685
refactor: improve transcription UX and DX#1685drHuangMHT wants to merge 14 commits intomoeru-ai:mainfrom
Conversation
⏳ Approval required for deploying to Cloudflare Workers (Preview) for stage-web.
Hey, maintainers, kindly take some time to review and approve this deployment when you are available. Thank you! 🙏 |
There was a problem hiding this comment.
Code Review
This pull request refactors transcription and auto-send logic by extracting it from ChatArea.vue into a new reusable composable, useTranscriptions.ts. This composable is then integrated into both ChatArea.vue and MobileInteractiveArea.vue to provide consistent transcription behavior across layouts. Review feedback suggests removing a redundant try-catch block in the new composable and transitioning from string-prefixed log messages to programmatic identifiers in metadata for better extensibility.
|
blocked by #1569 |
647138c to
ffa9441
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f09a8e2ea1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| messageInputRef: messageInput, | ||
| sendMessage: handleSend, | ||
| isStageTamagotchi: isMobileDevice, |
There was a problem hiding this comment.
Pass runtime env check instead of mobile-user-agent flag
useTranscriptions expects isStageTamagotchi to mean the Electron runtime, but this call passes isMobileDevice (user-agent mobile detection). On mobile web/Capacitor this evaluates true, so startStreaming treats the app like Electron and skips Web Speech auto-configuration, causing transcription startup to fail when no provider is preconfigured. This regresses the new portrait/mobile transcription path for first-time users.
Useful? React with 👍 / 👎.
| clearPendingAutoSend() | ||
| await stopStreamingTranscription(true) |
There was a problem hiding this comment.
Send pending transcript when stopping listening
Stopping transcription now cancels the debounce timer unconditionally and never flushes the last pending auto-send, so if a user turns the microphone off during the debounce window their final dictated sentence is left unsent. The previous logic explicitly sent pending text on stop; this refactor drops that behavior and can silently lose expected auto-send delivery in normal use.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
intended behavior
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cc7a1f169
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| watch(autoSendEnabled, (enabled) => { | ||
| if (!enabled) { | ||
| clearPendingAutoSend() | ||
| console.info('Auto-send disabled', { source: 'useTranscriptions' }) | ||
| } |
There was a problem hiding this comment.
Stop active transcription when microphone is disabled
This composable no longer reacts to enabled/stream changes, so turning the microphone off in HearingConfig can leave an active transcription session running until manual stop or unmount. That is especially problematic for the Web Speech provider path, which is designed to run continuously until stopStreamingTranscription() is explicitly called. Repro: start transcription, then disable the mic; isListening stays active and speech callbacks can continue even though the mic UI is off.
Useful? React with 👍 / 👎.
| watch([enabled], () => { | ||
| setupAnalyzer() | ||
| }, { immediate: true }) |
There was a problem hiding this comment.
Re-run analyzer setup when popover or stream changes
setupAnalyzer() now depends on hearingPopoverOpen and stream, but the watcher only observes enabled. If the mic is already enabled, opening the hearing popover (or receiving the stream slightly later) no longer re-initializes the analyzer, so the volume visualization can stay inactive until enabled is toggled again.
Useful? React with 👍 / 👎.
| v-if="autoSend" | ||
| label="Auto send" | ||
| :variant="autoSend ? 'primary' : 'secondary'" | ||
| flex-1 |
There was a problem hiding this comment.
Render auto-send toggle even when the value is false
The auto-send button is gated by v-if="autoSend", so it disappears whenever the setting is false. Since autoSendEnabled defaults to false, users cannot enable auto-send from this dialog at all, and once disabled they cannot turn it back on here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9915f883a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| watch([hearingDialogOpen, enabled, stream], () => { | ||
| watch([enabled], () => { |
There was a problem hiding this comment.
Watch dialog and stream changes when setting up analyzer
setupAnalyzer() still gates on hearingDialogOpen and stream, but the watcher now only tracks enabled. If the mic is already enabled, opening the hearing dialog or receiving the stream later will no longer re-run setup, so the volume meter can remain inactive until enabled is toggled again. This is a regression from the previous dependency set and affects normal UI interaction on mobile.
Useful? React with 👍 / 👎.
| }) | ||
|
|
||
| it('should expose startListening and stopListening', () => { | ||
| const { startListening, stopListening } = useTranscriptions(createOptions()) |
There was a problem hiding this comment.
Use the composable's exported method names in tests
The tests destructure startListening/stopListening, but useTranscriptions() now returns startStreamingTranscription/stopStreamingTranscription. As written, these variables are undefined, so calls like await startListening() will throw and make the new test suite fail instead of validating behavior.
Useful? React with 👍 / 👎.
Description
Linked Issues
Additional Context