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

Only query single route if segmentRange specified #522

Merged
merged 14 commits into from
Jul 12, 2024

Conversation

vishalkrishnads
Copy link
Contributor

@vishalkrishnads vishalkrishnads commented Jun 20, 2024

No description provided.

Copy link

github-actions bot commented Jun 20, 2024

Welcome to connect! Make sure to:

  • read the contributing guidelines
  • mark your PR as a draft until it's ready to review
  • post the preview on Discord; feedback from users will speedup the PR review

deployed preview: https://522.connect-d5y.pages.dev

@sshane
Copy link
Contributor

sshane commented Jun 24, 2024

Loading all routes isn't an appropriate solution (speed), we should go and check how this used to work and replicate that. Was it an API query using the time ranges? Perhaps we can just query w/ the route name

@adeebshihadeh
Copy link
Contributor

Loading all routes isn't an appropriate solution (speed), we should go and check how this used to work and replicate that. Was it an API query using the time ranges? Perhaps we can just query w/ the route name

We used to have the time range in the URL. We'll have to update the endpoint to also take a route name I guess.

@vishalkrishnads
Copy link
Contributor Author

Yeah that makes the most sense.

@sshane
Copy link
Contributor

sshane commented Jun 25, 2024

@vishalkrishnads
Copy link
Contributor Author

vishalkrishnads commented Jun 25, 2024

Sorry, this doesn't. We need segment_start_times, segment_end_times & segment_numbers in the response to make this work. I think this should wait until #536 is closed.

@incognitojam incognitojam marked this pull request as draft June 25, 2024 14:37
@vishalkrishnads vishalkrishnads marked this pull request as ready for review July 3, 2024 06:11
@vishalkrishnads
Copy link
Contributor Author

This issue has been fixed. for some reason, none of the CF pages related to my PR's are updating on subsequent commits. So, the URL related to this PR is outdated. However, the issue has been solved and the drives are loading.

src/actions/index.js Outdated Show resolved Hide resolved
@sshane
Copy link
Contributor

sshane commented Jul 12, 2024

This still doesn't work properly... It never recovers from this state:

image

Have a PR to properly fix this: #544

@sshane sshane closed this Jul 12, 2024
@vishalkrishnads
Copy link
Contributor Author

i can't see the issue. seems fine for me

drive.loading.mp4

@sshane
Copy link
Contributor

sshane commented Jul 12, 2024

Very strange, I can reproduce it locally with pnpm start but not the deployed preview. Maybe something different in how it's run in production.

I changed it to only query the specific route on loading a route from URL, makes a bit more sense

@sshane sshane reopened this Jul 12, 2024
@vishalkrishnads
Copy link
Contributor Author

seems like it works now 😅

are you going to merge this or do you want me to clean up anything more?

@sshane sshane changed the title Fixed connect not loading drives outside date range Only query single route if segmentRange specified Jul 12, 2024
@sshane sshane added the bugfix label Jul 12, 2024
@sshane sshane merged commit 6600dfc into commaai:master Jul 12, 2024
3 checks passed
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.

3 participants