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

[OSCI][Fix] ScopedHistory out of navigation scope #5498

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thanhinhchtom
Copy link
Contributor

@thanhinhchtom thanhinhchtom commented Nov 17, 2023

Description

Catch the error due to race conditions when the window reloads before the history comes. Name of error is ScopedHistory fell out of navigation scope.

Issues Resolved

Fix #5476

Screenshot

Screen.Recording.2023-12-01.at.2.53.33.PM.mov
image

Testing the changes

Check List

  • [] All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Thanh Le <lechithanh2003@gmail.com>
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.98%. Comparing base (f6ce6e7) to head (b829882).
Report is 495 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5498      +/-   ##
==========================================
- Coverage   66.98%   66.98%   -0.01%     
==========================================
  Files        3293     3293              
  Lines       63281    63297      +16     
  Branches    10061    10061              
==========================================
+ Hits        42389    42398       +9     
- Misses      18451    18459       +8     
+ Partials     2441     2440       -1     
Flag Coverage Δ
Linux_1 35.24% <0.00%> (-0.01%) ⬇️
Linux_2 55.20% <ø> (-0.01%) ⬇️
Linux_3 43.80% <100.00%> (+0.02%) ⬆️
Linux_4 35.34% <100.00%> (+<0.01%) ⬆️
Windows_1 35.26% <0.00%> (-0.01%) ⬇️
Windows_2 55.17% <ø> (-0.01%) ⬇️
Windows_3 43.80% <100.00%> (+<0.01%) ⬆️
Windows_4 35.34% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thanhinhchtom thanhinhchtom changed the title Fix ScopedHistory out of navigation scope [OSCI][Fix] ScopedHistory out of navigation scope Nov 17, 2023
Signed-off-by: Thanh Le <lechithanh2003@gmail.com>
@abbyhu2000
Copy link
Member

Hi @thanhinhchtom, thanks for the PR! However, i think this issue needs a little more diving deep on it.

For example, if we type something in the query bar on the data explorer page, the scoped history fell out of scope error will pop out. We want to find out why this error appear since typing a query should not cause any console error; adding a try catch here only handles the error but does not really solve the bug.

Screen.Recording.2023-12-01.at.11.33.57.AM.mov

Do you want to dive a little deeper on this bug and document your findings? Thank you!

@thanhinhchtom
Copy link
Contributor Author

thanhinhchtom commented Dec 1, 2023

Hi @thanhinhchtom, thanks for the PR! However, i think this issue needs a little more diving deep on it.

For example, if we type something in the query bar on the data explorer page, the scoped history fell out of scope error will pop out. We want to find out why this error appear since typing a query should not cause any console error; adding a try catch here only handles the error but does not really solve the bug.

Screen.Recording.2023-12-01.at.11.33.57.AM.mov
Do you want to dive a little deeper on this bug and document your findings? Thank you!

@abbyhu2000 I already looked up a little bit, and I think this error comes from the Elastic Search code. From what I understand, it is caused by the window reloading faster than the scoped history added, resulting in an error. And because of that, when I did the try-catch one, I already tried to make some changes to many other places to ensure that the history would work fine. However, I failed to do so, and I think the easiest way is to add a try-catch statement here to handle the error with history.

@abbyhu2000
Copy link
Member

@thanhinhchtom Hi, I do not think this is coming from the elastic search code since this error only occurs after we replace from the legacy discover to the current data explorer. Before the change, the scoped history for discover pages works fine and will not output errors like this to the console. I think the error prob originated somewhere in the code of de-angular discover to data explorer. Thanks for taking your time on this issue, and please comment more of your findings if you are still interested in digging deeper. Meanwhile I will convert this PR to draft for now.

@abbyhu2000 abbyhu2000 marked this pull request as draft December 19, 2023 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ScopedHistory instance has fell out of scope for basePath: /app/data-explorer
2 participants