Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: audio recording variety bugs #2648

Merged
merged 13 commits into from
Sep 3, 2024
Merged

Conversation

isekovanic
Copy link
Contributor

@isekovanic isekovanic commented Sep 2, 2024

🎯 Goal

This PR addresses the issues mentioned in this Github issue. In addition to this, it also takes care of a variety of additional bugs and odd behaviours I found with async audio recording in particular.

🛠 Implementation details

Provided below is a list of the things it fixes.

General (all platforms):

  • When letting go of the recording button while below both the threshold for locking and canceling, the recording is sent to the chat as an available attachment
    • I wasn't too sure if this is the behaviour we want, but it seems a lot more intuitive than having the recording stuck in a weird state so it made sense
    • The difference between locking and doing it like this is that this is the "quick way" to send an audio recording whereas the lock is more of an "advanced view"
  • Audio recording would not stop if we navigate away from the screen where it got triggered (where <MessageList /> is present for example)
  • Simply clicking on the recording button instead of doing a long press would sometimes cause issues and would not display the Hold to start Recording message
    • Took the chance here to move to the new Gesture API since it seems a lot more stable and has quite a few more options

Vanilla React Native:

  • Whenever opening the app for the first time and being asked for permissions, clicking "Accept" would cause the app to be stuck in an irrecoverable state of recording until we completely unmount the AudioRecorder twice
    • The behaviour of starting recording automatically after we've accepted on the permissions dialog is still there (it's actually not trivial to skip this, since a lot of intertwined state is causing it) but now we can safely take over the controls again by doing a long press on the recording button and either cancel the recording, open the microphone locked view or simply do a quick send of it
  • If permissions were specifically denied, pressing the recording button twice in a row would cause the audio player to start the recording process in the background but not actually record, setting the AudioRecorder in an unrecoverable state (either unmounting or killing the app entirely would be the only thing that fixes this)
    • This is a bug with the underlying library we're using that's well documented here; feel free to read my comment for more details about our specific usecase

Expo:

  • There was an issue with the expo-av library that occurs immediately after accepting permissions if we run Recording.createAsync() too quickly before the JS thread taking control again
    • Recording would fail and we would immediately get the message that we haven't provided permissions, despite the fact that we just did (this is a side-effect of the recording erroring out on the expo-av side)
    • The issue is well documented here
  • A slight optimization whenever permission has been declined where we now exit early instead of proceeding with the process again

🎨 UI Changes

iOS
Before After
Android
Before After

🧪 Testing

☑️ Checklist

  • I have signed the Stream CLA (required)
  • PR targets the develop branch
  • Documentation is updated
  • New code is tested in main example apps, including all possible scenarios
    • SampleApp iOS and Android
    • Expo iOS and Android

@Stream-SDK-Bot
Copy link
Contributor

Stream-SDK-Bot commented Sep 2, 2024

SDK Size

title develop branch diff status
js_bundle_size 438.6728515625 KB 439 KB 0 B 🟢

@khushal87
Copy link
Member

I see that we show the AudioRecordingWaveform component even when the mic is not locked. I think this is not supposed to be done.
IMG_0122

@isekovanic
Copy link
Contributor Author

I see that we show the AudioRecordingWaveform component even when the mic is not locked. I think this is not supposed to be done. IMG_0122

Oh, great catch ! This one kind of slipped my mind. Thanks, will fix it in a sec !

@isekovanic isekovanic merged commit 2011eb2 into develop Sep 3, 2024
7 checks passed
@isekovanic isekovanic deleted the fix/audio-recording-variety-bugs branch September 3, 2024 08:24
@github-actions github-actions bot mentioned this pull request Sep 4, 2024
6 tasks
@stream-ci-bot
Copy link
Contributor

🎉 This PR is included in version 5.36.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

khushal87 added a commit that referenced this pull request Sep 19, 2024
* fix: upgrade firebase versions to fix crash (#2646)

* fix: audio recording variety bugs (#2648)

* fix: variety of bugs in async audio feature

* fix: upload of empty messages and console.log cleanup

* fix: expo permission race conditions

* fix: create pure version of stopRecording() for usage on unmount

* fix: remove redundant export

* fix: actually amend failing test suites

* chore: remove constructors as they are not needed

* fix: pr remarks

* chore: write some simplistic functionality tests

* chore: return class instance rather than class itself for better usability

* chore: add more robust tests

* fix: linter errors

* fix: only display waveform whenever the mic is locked

* docs: add Deep Linking guide for the chat SDK (#2651)

* fix: change Block user action to Ban user action and UI cookbook for blocking users (#2649)

* fix: change Block user action to Ban user action

* fix: change Block user action to Ban user action

* fix: add back blockUser action and deprecate it

* docs: UI Cookbook for Blocking users

* chore(release): 5.36.1 [skip ci]

* chore(release): 1.28.1 [skip ci]

* fix: issue with loading app settings when the connectUser is not called on app (#2654)

* fix: issue with loading app settings when the connectUser is not called on app

* fix: add comments

* fix: add comments

* fix: unable to upload file due to special characters in the file name (#2656)

* fix: sdk size pr (#2657)

* [CI] Update SDK Size (#2653)

Co-authored-by: Khushal Agarwal <khushal.agarwal987@gmail.com>
Co-authored-by: Stream Bot <runner@fv-az1148-731.o3ts4yn1huletmfehy03vvfvxg.dx.internal.cloudapp.net>

* chore(release): 5.36.2 [skip ci]

* chore(release): 1.28.2 [skip ci]

* feat: add live location sharing cookbook (#2659)

* feat: add live location ui cookbook

* fix: prettier lint

* Update docusaurus/docs/reactnative/guides/live-location-sharing.mdx

Co-authored-by: Oliver Lazoroski <oliver.lazoroski@gmail.com>

* lint fixes

* fix lint issues

* fix locks

* move to ruby 3.1 and greater

---------

Co-authored-by: Oliver Lazoroski <oliver.lazoroski@gmail.com>

* chore(release): 5.37.0 [skip ci]

* chore(release): 1.29.0 [skip ci]

* fix: avoid prepending http before native supported url schemes (#2661)

* fix: avoid prepending http before native supported url schemes

* fix: move check to link parsing module

* fix: bad memoisation in window, screen dimension listener hooks (#2664)

* fix: bad memoisation in window, screen dimension listener hooks

* remove unused variable

* feat: add create chat client hook for easy usage (#2660)

* feat: add create chat client hook for easy usage

* docs: use useCreateChatClient hook for client creation

* fix: bump fastlane plugin version (#2665)

* fix: add theme properties for EmptyStateIndicator for message list (#2667)

* fix: add theme properties for EmptyStateIndicator for message list

* fix: update snapshots

* fix: apply card cover theme property order

* fix: pagination typescript errors and db synchronization bugs (#2669)

* fix: pagination typescript errors and db synchronization bugs

* chore: write test for db serialization issue

* fix: linter issues

* [CI] Bump max tolerance for sdk size analysis (#2674)

* chore: bump sample app version to v1.30.0

* fix: remove  nin and ne  operator usage in the SDK and the sample app (#2672)

* fix: remove  and  operator usage in the SDK and the sample app

* fix: remove console log

* fix: change console log to warn

* fix: add improvemnts

* fix: import of debouncefunc

* fix: import of debouncefunc

* fix: restructure queryMembers, queryUsers and ACItriggersettings

* fix: error bubbling for suggestions in auto complete input

* fix: request image access permissions for iOS only for native image picking (#2677)

* fix: properly resolve sendMessage during memoization (#2675)

* fix: properly resolve sendMessage during memoization

* fix: remedy change so that it does not cause performance issues

* chore: revert sendMessage in the dep array

* fix: deprecate messageReactions prop and use isMessageActionsVisible instead for messageActions (#2676)

* fix: deprecate messageReactions prop and use isMessageActionsVisible instead for messageActions

* docs: fix custom message actions

* fix: execution logic for showMessageOverlay

* chore(release): 5.38.0 [skip ci]

* chore(release): 1.29.1 [skip ci]

* fix: update yarn.lock for the project (#2681)

* fix: copy message action type for message actions (#2679)

* chore: update sdk size (#2678)

Co-authored-by: Khushal Agarwal <khushal.agarwal987@gmail.com>
Co-authored-by: Stream Bot <runner@fv-az1756-392.rxb2ubmju23uthz3oztawtjyeg.dx.internal.cloudapp.net>

* chore: resolve conflicts from develop

* fix: vale lint issues

* fix: new arch project config

---------

Co-authored-by: Ivan Sekovanikj <31964049+isekovanic@users.noreply.github.com>
Co-authored-by: semantic-release-bot <semantic-release-bot@martynus.net>
Co-authored-by: Alexey Alter-Pesotskiy <alex@testableapple.com>
Co-authored-by: Stream SDK Bot <60655709+Stream-SDK-Bot@users.noreply.github.com>
Co-authored-by: Stream Bot <runner@fv-az1148-731.o3ts4yn1huletmfehy03vvfvxg.dx.internal.cloudapp.net>
Co-authored-by: Santhosh Vaiyapuri <3846977+santhoshvai@users.noreply.github.com>
Co-authored-by: Oliver Lazoroski <oliver.lazoroski@gmail.com>
Co-authored-by: Ivan Sekovanikj <ivan.sekovanikj@getstream.io>
Co-authored-by: Stream Bot <runner@fv-az1756-392.rxb2ubmju23uthz3oztawtjyeg.dx.internal.cloudapp.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants