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

Cache the detail query to avoid page crashing after refreshing #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ansjcy
Copy link
Member

@ansjcy ansjcy commented Sep 13, 2024

Description

Currently the details page crashes after refreshing the page, because the corresponding query that is passed to the detail page won't be available after refreshing. Fixing this bug by adding the query to session storage. Also added several unit tests for the details page and refactored the code.

Issues Resolved

#12

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Chenyang Ji <cyji@amazon.com>
@ansjcy ansjcy force-pushed the fix-details-page-crashing branch from 8ac3e45 to 60d278c Compare November 4, 2024 23:38
@ansjcy ansjcy changed the title [WIP] Cache the detail query to avoid page crashing after refreshing Cache the detail query to avoid page crashing after refreshing Nov 4, 2024
Copy link

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

Pulled it locally and it works. From a high level I feel like this session storage approach has some downsides mainly:

  • Complexity of involving session storage
  • Cannot have a consistent/sharable URL (for example if I want to link a specific query for others to view, I cannot do that with this new approach)

@ansjcy What do you think of the above? If we have some UUID/can reconstruct the query from the hash only I think that would solve the above issues without introducing new concepts?

@derek-ho
Copy link

Pulled it locally and it works. From a high level I feel like this session storage approach has some downsides mainly:

  • Complexity of involving session storage
  • Cannot have a consistent/sharable URL (for example if I want to link a specific query for others to view, I cannot do that with this new approach)

@ansjcy What do you think of the above? If we have some UUID/can reconstruct the query from the hash only I think that would solve the above issues without introducing new concepts?

For more color on the above, what the usual paradigm we use for these specific detail UI pages would be to include a hash in the url, such as queryInsights/${UUID_of_specific_query}. Then on page refresh, the call can be made to fetch the details of that specific UUID. That way the page can be loaded without first visiting the list page, and this page can also be shared and loaded by anyone else with access to that query.

@ansjcy
Copy link
Member Author

ansjcy commented Nov 14, 2024

That make sense! We should use an UUID (maybe query hashcode?) instead of session storage.. This is a very valid use case.

That way the page can be loaded without first visiting the list page, and this page can also be shared and loaded by anyone else with access to that query.

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