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 quirks with recording controls #6

Merged
merged 2 commits into from
Sep 12, 2022
Merged

fix quirks with recording controls #6

merged 2 commits into from
Sep 12, 2022

Conversation

beasteers
Copy link
Collaborator

The recording functionality on the dashboard had some issues around starting and stopping recordings and there was that bug where you'd need to refresh the page to start a new recording.

This version calls the start and stop endpoints directly and relies on a single source of truth about whether the recording is running which reduces the weird edge cases we were seeing.

Small thing is that using mutate doesn't seem to run at a strictly regular schedule so it can maybe use some tweaking but it is minor.

// update the recording info while it's active
useInterval(() => { mutate() }, recordingId ? 1000 : null)

@soniacq
Copy link
Contributor

soniacq commented Sep 12, 2022

Apparently, the warning messages have been removed ( we actually want to keep them):
For example, if there is no connection with the server (there is no data streaming), we should keep displaying the message: "We couldn't connect with the server. Please try again!".
This is what is happening right now:
Screen Shot 2022-09-12 at 2 15 07 PM

This is how was handled before (deployed version of the dashboard):
Screen Shot 2022-09-12 at 2 14 35 PM

@beasteers
Copy link
Collaborator Author

I was having a bit of trouble understanding the conditional tree that was going on before.

Looking at what the dashboard shows and querying the API directly, it actually looks like it's showing errors when it shouldn't be - like when I hit stop recording on the dashboard it says "error please press again" but when I query the current recording via CLI (without pressing again), it says that the recording has been stopped - and the same thing goes with the start recording error.

You can try yourself if you'd like, just run

ptgctl recordings current

when an error pops up to see if it really was an error, or if the recording was really stopped/started.

I have a feeling that the previous version was reading too much into the return codes of the start/stop endpoints which reflects changes to the state in redis, but this newer version just uses the current recording query which should be a more reliable check.

@beasteers
Copy link
Collaborator Author

but to clarify, those status codes returned by the API don't look at whether or not data is coming in. So the recording should be active even without data and once you turn on the hololens it should start recording immediately.

@soniacq
Copy link
Contributor

soniacq commented Sep 12, 2022

I understand your point. However, I still think we should prevent the user from hitting start recording if there is no data streaming. Otherwise, anyone who has access to the dashboard can hit the start button even though there is no hololens connected and we will start getting empty recordings.

@soniacq
Copy link
Contributor

soniacq commented Sep 12, 2022

Opening issue #7. Prevent the user from hitting start recording if there is no data streaming.

@soniacq soniacq closed this Sep 12, 2022
@soniacq soniacq reopened this Sep 12, 2022
@soniacq soniacq merged commit bf850d4 into main Sep 12, 2022
@soniacq soniacq deleted the recordings-fix branch September 12, 2022 19:18
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.

2 participants