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

Lingering issues for Public Release #86 #96

Merged
merged 26 commits into from
Jun 9, 2020

Conversation

josiahseaman
Copy link
Member

@josiahseaman josiahseaman commented Jun 8, 2020

This is our final bug fix round before public release. It deserves lots of testing. If you can find time, please test and leave us review comments on GitHub.

AndreaGuarracino and others added 23 commits June 5, 2020 14:17
…into v17

# Conflicts:
#	src/PangenomeSchematic.js
#	src/ViewportInputsStore.js
…dth 1; added pangenome last bin position for the user; code cleaning; #86
@josiahseaman josiahseaman self-assigned this Jun 8, 2020
Copy link
Contributor

@6br 6br left a comment

Choose a reason for hiding this comment

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

Thank you for implementation!
I'd like to try this branch, but yarn start cannot show anything...
I'd be glad if there is any instruction to visualize sars_covid dataset on this branch.

"react": "^16.12.0",
"react-dom": "^16.12.0",
"jquery": "^3.5.1",
"konva": "^4.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for curiosity's sake, is it possible to update a konva or react-script version to the latest?
I saw some vulnerabilities on react-script dependency.

src/PangenomeSchematic.js Show resolved Hide resolved
@6br
Copy link
Contributor

6br commented Jun 9, 2020

I found minor thing when I set row number as 1.
2020-06-10_0_11_42

@6br
Copy link
Contributor

6br commented Jun 9, 2020

I found an awkward behaviour when I click bin link on bin_width != 1.
2020-06-10 0 21 49

test23

@josiahseaman
Copy link
Member Author

I'm going to start trying to reproduce this error now. I'm on commit 78bd415

@6br
Copy link
Contributor

6br commented Jun 9, 2020

I'm afraid that the latest commit cannot read any bin2file.json. I think this is due to the packages update.

2020-06-10 0 33 36

@josiahseaman
Copy link
Member Author

Maybe you didn't npm start again. I accidentally did that this morning.

@josiahseaman
Copy link
Member Author

It was very difficult to reproduce the bug, but I finally got it by throttling my internet connection. It appears to be a race condition while navigating at the same time you're updating your bin width zoom level. I think it's sufficient to just fail silently in this case and trigger a re-render.
image

@6br
Copy link
Contributor

6br commented Jun 9, 2020

I found the reason. The requested URL was http://localhost:3000/Schematizetest_data/SARS-CoV-b/bin2file.json. This is called from tryJSONpath function. This function needs to be fixed.

From

    function tryJSONpath(event) {
      const url =
        process.env.PUBLIC_URL +
        "test_data/" +
        event.target.value +
        "/bin2file.json";
      if (urlExists(url)) {
        console.log("STEP#1: New Data Source: " + event.target.value);
        self.jsonName = event.target.value;
      }

To

    function tryJSONpath(event) {
      const url =
        process.env.PUBLIC_URL +
        "/test_data/" +
        event.target.value +
        "/bin2file.json";
      if (urlExists(url)) {
        console.log("STEP#1: New Data Source: " + event.target.value);
        self.jsonName = event.target.value;
      }

The prefix "/" is needed on "test_data/"

@josiahseaman
Copy link
Member Author

I just pushed a commit with both of those fixes. See if you can reproduce the link bug.

@6br
Copy link
Contributor

6br commented Jun 9, 2020

It was very difficult to reproduce the bug, but I finally got it by throttling my internet connection. It appears to be a race condition while navigating at the same time you're updating your bin width zoom level. I think it's sufficient to just fail silently in this case and trigger a re-render.

Certainly, I agree to leave it as.

@6br
Copy link
Contributor

6br commented Jun 9, 2020

I just pushed a commit with both of those fixes. See if you can reproduce the link bug.

These bugs are now solved. Thank you!

@josiahseaman josiahseaman merged commit dd3df3d into master Jun 9, 2020
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