Leaving PodcastRecorder does not stop an active native recording

[karrisanthoshigayatri]

#1914

PR Description

Please include a summary of the changes and the related issue. Describe your changes in detail.

Type of Change

• Bug fix (change which fixes an issue)
• New feature (change which adds functionality)
• Documentation update

Select your work-area

• Frontend
• Backend
• Documentation
• Others

Related Issue

Please provide a link to the issue solved if applicable.

Add your Work Example

📷 Add a Snapshot

Fixes (mention the issue number which this fixes)

#closes

Checklist

• I have updated my branch and synced it with the project's 'develop' branch before making this PR.
• I have optimized the file changes.
• I have added a snapshot of my work example.
• I have made a PR to the project's develop branch.

Undertaking

• My code follows the style guidelines of this project.

• I have performed a self-review of my code.

• I have commented my code, particularly in hard-to-understand areas.

• I have made corresponding changes to the documentation.

• I have checked for plagiarism and assure its authenticity.

• I have read and followed the code of conduct for this repository. I understand that violation of this undertaking may have legal consequences.

• I Agree

[github-actions]

Thank you @, for creating the PR and contributing to our UltimateHealth project 💗.
Our team will review the PR and will reach out to you soon! 😇
Make sure that you have marked all the tasks that you are done with ✅.
Thank you for your patience! 😀

[github-actions]

Automated Review Feedback

Provide actionable comments grouped by severity:

Critical

1. Incorrect handleUpload execution: The handleUpload function was moved from useFocusEffect to useEffect([]). The comment states "Reset UI state each time this screen gains focus", but useEffect([]) only runs once on component mount, not on every focus. This changes the intended behavior of handleUpload and may lead to incorrect UI state or missed processing if handleUpload is expected to run on subsequent screen focuses. Please revert handleUpload back to useFocusEffect or clarify its intended lifecycle and adjust the effect accordingly.

Suggestions

1. Remove .vscode/settings.json: The newly added .vscode/settings.json file is empty and typically not committed to the repository. Please remove it.

Maintainer Note:

Once the initial automated feedback has been addressed, maintainer @SB2318 will review the pull request for final evaluation.

[karrisanthoshigayatri]

I checked the implementation. Could you please point me to the specific line where handleUpload was moved from useFocusEffect to useEffect? I couldn't find that change.

[SB2318]

/review

[github-actions]

🤖 Gemini AI Code Review

Summary

This Pull Request addresses a critical bug where an active native recording was not properly stopped when the PodcastRecorder screen was exited. The changes introduce a useFocusEffect hook to ensure that the native audio recorder is stopped, audio mode is restored, and UI state is reset when the screen loses focus.

While the PR correctly identifies and implements the core fix for stopping the native recording and restoring audio mode, there's a significant logic flaw in how handleUpload is now triggered, which contradicts the stated intent and could lead to unexpected behavior.

🔴 High Severity

• Issue: The useEffect hook responsible for calling handleUpload() is configured with an empty dependency array ([]), meaning it will only execute once when the component mounts. However, the accompanying comment states, "Reset UI state each time this screen gains focus (e.g. after navigating back)." This is a direct contradiction. If handleUpload is intended to process recordings or reset UI state every time the screen becomes active (e.g., after navigating away and back), it should be placed within a useFocusEffect or have appropriate dependencies. As it stands, handleUpload will only run on the initial mount, potentially missing subsequent opportunities to process recordings or reset state after the screen has been revisited.
• Impact: This bug means that if a user navigates away from the PodcastRecorder screen and then back to it, handleUpload will not be triggered again. This could lead to recordings not being processed as expected, or the UI not being correctly reset if handleUpload has side effects related to UI state.
• Fix: Move the handleUpload() call back into a useFocusEffect to ensure it runs every time the screen gains focus, aligning with the comment's intent. If handleUpload has dependencies, ensure they are correctly listed in the useCallback's dependency array.

// Stop any active recording and release resources when the screen loses focus
// (back navigation, screen replacement, etc.).
useFocusEffect(
  useCallback(() => {
    // Call handleUpload when the screen gains focus, as per original intent.
    handleUpload(); 

    return () => {
      // Always stop the JS timer first.
      stopTimer();

      if (isRecordingRef.current) {
        isRecordingRef.current = false;

        audioRecorder.stop().catch(err =>
          console.warn('Error stopping recorder on screen exit:', err),
        );

        // Restore audio mode so other screens are not affected.
        setAudioModeAsync({
          playsInSilentMode: false,
          allowsRecording: false,
        }).catch(err =>
          console.warn('Error restoring audio mode on screen exit:', err),
        );

        // Reset visual state so the screen looks correct if revisited.
        setRecording(false);
        setUiState('idle');
        setRecordTime('00:00:00');
      }
    };
  }, [audioRecorder, handleUpload]), // Ensure handleUpload is a dependency if it's a useCallback
);

// Remove the separate useEffect for handleUpload as it's now handled above.
// useEffect(() => {
//   handleUpload();
//   // eslint-disable-next-line react-hooks/exhaustive-deps
// }, []);

🟡 Medium Severity

• Issue: The useEffect hook for handleUpload uses // eslint-disable-next-line react-hooks/exhaustive-deps. This linting directive is used to suppress warnings about missing dependencies. In conjunction with the empty dependency array ([]), this indicates a potential misunderstanding of useEffect or a deliberate suppression that could mask bugs. If handleUpload is a useCallback that depends on other state or props, using [] will cause it to capture stale values, leading to incorrect behavior.

• Impact: Suppressing linter warnings without addressing the underlying cause can lead to subtle bugs that are hard to debug, especially if handleUpload relies on up-to-date state or props.

• Fix: Address the High Severity issue first. Once handleUpload is correctly placed within useFocusEffect (or its dependencies are correctly specified in a useEffect), this eslint-disable directive should no longer be necessary and can be removed.

• Issue: The AppState import is present in frontend/src/screens/PodcastRecorder.tsx, but AppState is not used anywhere in the provided diff.

• Impact: Unused imports clutter the codebase, can slightly increase bundle size (though minimal for a single import), and make code harder to read and maintain.

• Fix: Remove the unused AppState import.

-import {Alert, AppState, AppStateStatus} from 'react-native';
+import {Alert, AppStateStatus} from 'react-native';

🟢 Low Severity / Nits

• The StyleSheet import was removed, which is a good cleanup if it was indeed unused in the component. This is a positive change.

What's Good ✅

1. Comprehensive Cleanup on Screen Exit: The PR correctly identifies the need to not only stop the native audio recorder but also to restore the audio mode (setAudioModeAsync) and reset the UI state (setRecording, setUiState, setRecordTime). This ensures proper resource management and a consistent user experience when navigating away from and back to the screen.
2. Robust Error Handling: The use of .catch() blocks for audioRecorder.stop() and setAudioModeAsync() demonstrates good defensive programming, preventing crashes and logging potential issues during critical cleanup operations.
3. Clear Intent with useFocusEffect: Utilizing useFocusEffect for cleanup when the screen loses focus is the correct pattern for React Navigation, ensuring that resources are released reliably regardless of how the user navigates away.

Verdict

Request Changes

The most critical issue is the incorrect implementation of the handleUpload call within a useEffect with an empty dependency array, which contradicts the stated intent and will prevent handleUpload from running on subsequent screen focuses. This needs to be addressed to ensure the intended functionality of the screen.

[SB2318]

@karrisanthoshigayatri Thanks for your contribution! Please fix the bot points.