[SAAS-19849] Update audio recording widget UX #3768
Conversation
Begin recording immediately when the recorder opens without an existing recording, removing the intermediate tap-to-record step. Gated on savedInstanceState == null so a configuration-change recreation doesn't re-trigger a new recording. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stopping now finalizes and saves directly, removing the intermediate playback step. stopRecording takes a shouldSave flag so Save-from-paused stops without re-triggering save. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removing the post-stop playback transition left the recorder fragment's own playback path unreachable (playAudio/pause/resume/reset, the MediaPlayer field, and the onDismiss release were all circularly dead). Playback now happens in CommCareAudioWidget. Drops the dead methods and the now-unused MediaPlayer/Uri imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3768 +/- ##
=========================================
Coverage 25.94% 25.95%
Complexity 4431 4431
=========================================
Files 961 961
Lines 57618 57604 -14
Branches 6869 6864 -5
=========================================
Hits 14951 14951
+ Misses 40829 40814 -15
- Partials 1838 1839 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…udio-recording-ux-changes
avazirna
changed the title
Adds an upload-artifact step (gated on failure()) that publishes the testCommcareDebug HTML report so failing unit tests can be inspected from the run's artifacts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Out of curiosity, what does the UI look like after a recording is deleted?
| - name: Upload unit test report | ||
| if: ${{ failure() }} | ||
| uses: actions/upload-artifact@v6 | ||
| with: | ||
| name: unit-test-report | ||
| path: commcare-android/app/build/reports/tests/testCommcareDebugUnitTest | ||
| if-no-files-found: warn | ||
| retention-days: 14 |
There was a problem hiding this comment.
Was this added by mistake?
There was a problem hiding this comment.
The build failed a couple of times because of one of the unit tests, UpdateActivityTest.invalidUpdateTest(), and I added this to get more info around the error. But after that addition, the builds were successful. I think this is a good addition but not in the scope of this PR, so the plan is to PR this separately.
There was a problem hiding this comment.
think a 3 day retention should be sufficient here, one can always re-trigger the build to get this info again.
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
looks good, would be nice to add safety story around phone rotation on this change.
📝 WalkthroughWalkthroughThe PR updates the Android PR workflow to upload the unit test report on failure. It also extends release notes with audio recording revamp entries and changes Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/views/widgets/RecordingFragment.java (1)
280-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve a discard path for new recordings.
At Line 280 and Line 386, both stop flows now commit immediately. Because recording also makes the dialog non-cancelable (Line 231) and hides
discardRecordingduring capture (Line 289), a fresh recorder session no longer has a way to back out without saving unwanted audio.CommCareAudioWidget.onRecordingCompletion()(app/src/org/commcare/views/widgets/CommCareAudioWidget.java:153-162) will then promote that file to the field value as soon as it exists.Also applies to: 384-391
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/org/commcare/views/widgets/RecordingFragment.java` around lines 280 - 310, The stop-recording flow in RecordingFragment is always finalizing the audio, which removes the ability to back out of a new recording session; update stopRecording(boolean shouldSave) and the related stop paths so a discard option still exists for fresh recordings. Keep the discardRecording / negative-action behavior available when starting or stopping a new capture, and only call saveRecording() for explicit save actions so CommCareAudioWidget.onRecordingCompletion() does not promote unwanted audio automatically.
🧹 Nitpick comments (1)
.github/workflows/commcare-android-pr-workflow.yml (1)
96-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reducing retention from 14 days to 3 days.
The
retention-days: 14is significantly longer than other artifacts in this workflow (qaAutomation APKs use 3 days). A previous reviewer noted that 3 days should be sufficient since one can re-trigger the build to get fresh artifacts. Longer retention consumes more storage quota without proportional value for ephemeral test reports.- retention-days: 14 + retention-days: 3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/commcare-android-pr-workflow.yml around lines 96 - 103, Reduce the artifact retention for the unit test report upload in the workflow to match the shorter lifespan used elsewhere. Update the Upload unit test report step in commcare-android-pr-workflow.yml so the actions/upload-artifact@v6 configuration uses a 3-day retention instead of 14 days.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/org/commcare/views/widgets/RecordingFragment.java`:
- Around line 137-139: The auto-start path in RecordingFragment still leaves the
mic button active while startRecording() is being triggered, which allows a
second tap to re-enter the same recording session through toggleRecording and
AudioRecordingService.onStartCommand. Update RecordingFragment’s auto-start flow
so the mic control is switched into a disabled/loading state before calling
startRecording(), and keep it non-interactive until the recording startup
completes to prevent duplicate start requests.
---
Outside diff comments:
In `@app/src/org/commcare/views/widgets/RecordingFragment.java`:
- Around line 280-310: The stop-recording flow in RecordingFragment is always
finalizing the audio, which removes the ability to back out of a new recording
session; update stopRecording(boolean shouldSave) and the related stop paths so
a discard option still exists for fresh recordings. Keep the discardRecording /
negative-action behavior available when starting or stopping a new capture, and
only call saveRecording() for explicit save actions so
CommCareAudioWidget.onRecordingCompletion() does not promote unwanted audio
automatically.
---
Nitpick comments:
In @.github/workflows/commcare-android-pr-workflow.yml:
- Around line 96-103: Reduce the artifact retention for the unit test report
upload in the workflow to match the shorter lifespan used elsewhere. Update the
Upload unit test report step in commcare-android-pr-workflow.yml so the
actions/upload-artifact@v6 configuration uses a 3-day retention instead of 14
days.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3443268f-6cec-4914-bd0b-a71a5617c1ae
📒 Files selected for processing (3)
.github/workflows/commcare-android-pr-workflow.ymlRELEASES.mdapp/src/org/commcare/views/widgets/RecordingFragment.java
| } else if (savedInstanceState == null) { | ||
| startRecording(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Disable the mic before the auto-start call.
At Line 137, the fragment calls startRecording() while toggleRecording is still wired to startRecording(). A user tap in that bind window issues a second start request, and AudioRecordingService.onStartCommand() (app/src/org/commcare/views/widgets/AudioRecordingService.java:26-60) re-enters the recorder start path for the same session. Flip the button into a disabled/loading state before auto-starting so this path can only run once.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/org/commcare/views/widgets/RecordingFragment.java` around lines 137 -
139, The auto-start path in RecordingFragment still leaves the mic button active
while startRecording() is being triggered, which allows a second tap to re-enter
the same recording session through toggleRecording and
AudioRecordingService.onStartCommand. Update RecordingFragment’s auto-start flow
so the mic control is switched into a disabled/loading state before calling
startRecording(), and keep it non-interactive until the recording startup
completes to prevent duplicate start requests.
3 participants
Product Description
Two recorder UX changes from SAAS-19849 (short-recording flow):
before_audio_recording.mp4
after_ux_changes.mp4
Technical Summary
Implements SAAS-19849. All changes are in
RecordingFragment.java:initMediaResources()and move the reload-vs-start decision intoonViewCreated.savedInstanceState == nullso a configuration-change recreation doesn't re-trigger a recording.stopRecording(boolean shouldSave); stopping a non-pausable recording saves and dismisses directly. The flag lets the Save-from-paused path stop without re-triggering save.playAudio/pause/resume/reset, theMediaPlayerfield, and the unusedMediaPlayer/Uriimports are now removed (playback lives inCommCareAudioWidget).Safety Assurance
Safety story
Scoped to the recorder dialog; no data-model/storage changes.
AudioRecordingService.stopRecording()is synchronous (MediaRecorder.stop()blocks until the file is finalized), so save-on-stop reads a fully-written file.Automated test coverage
No automated tests added (UI flow).
Labels and Review