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

[SES-1935] Audio recording crash #1494

Merged
merged 2 commits into from
May 29, 2024

Conversation

simophin
Copy link

@simophin simophin commented May 21, 2024

Description

I tried to find a way to trigger a double call to startRecording and failed to find any. Instead of hard crashing the app, I think it's ok to ignore the edge case here, by logging the invariant then return early.

So the worst thing it could happen is for some edgy case the recording can't be started.

@simophin simophin changed the base branch from master to dev May 21, 2024 04:57
@simophin simophin force-pushed the ses-1935-audio-recording-crash branch from 5904853 to 1354bcc Compare May 21, 2024 05:03
@simophin simophin force-pushed the ses-1935-audio-recording-crash branch from 1354bcc to 27fcc66 Compare May 21, 2024 05:06
Copy link

@bemusementpark bemusementpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though now, without a stacktrace, the text makes even less sense than before.

So it's probably worth removing the log, or at least saying what went wrong, as the text We can only record once at a time. is more of an explanation, but we don't say audioCode was expected to be null or audioCodec = x and playing = true...

I'm happy either way, remove log, or make it a little more informative.

@simophin
Copy link
Author

Hey @bemusementpark or @AL-Session can I ask either of you to merge this one pls? It has passed QA

@bemusementpark
Copy link

It'd be nice, but IDK if we're merging to dev, or some release branch, so don't worry about it for now.

Copy link
Collaborator

@AL-Session AL-Session left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved - but Morgan says to hold off merging into dev until we've had a chance to clarify processes around builds & branches.

@bemusementpark bemusementpark merged commit 66c0f94 into oxen-io:dev May 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants