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(viz): track draw state to apply recenter on fresh draws #740

Merged
merged 11 commits into from
Nov 7, 2023

Conversation

plyr4
Copy link
Contributor

@plyr4 plyr4 commented Nov 6, 2023

more stability for the elm vs. js render race condition, where the page root doesnt render quick enough for the graph render dispatch

also

  • renames isRefreshDraw to sameBuild to make it a bit clearer
  • renames centerOnDraw to freshDraw to make it a bit clearer

@plyr4 plyr4 requested a review from a team as a code owner November 6, 2023 20:55
wass3rw3rk
wass3rw3rk previously approved these changes Nov 6, 2023
wass3r
wass3r previously approved these changes Nov 6, 2023
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

seems to work now. i'm not able to reproduce the issue.
was working on another issue and have to backtrack here. i can reliably reproduce with following steps (on main branch also):

  1. visit repo builds page
  2. click on any build to go to its build page
  3. click "Visualize" tab

it works fine when i hard refresh repo builds page after step 1. and do the remaining steps

@wass3r wass3r dismissed their stale review November 7, 2023 02:43

found issue

@wass3r
Copy link
Collaborator

wass3r commented Nov 7, 2023

fwiw, i'm able to get pretty consistent results it seems by changing two things:

  1. use renderBuildGraph updatedModel True here
    , cmd
  2. change the following to use renderBuildGraph um True
    renderBuildGraph um False

in other words, passing a truthy centerOnDraw more.

worst result i saw is an initial flash of uncentered graph and then it fixed itself. seemed to be pretty rare - more rare than the initial issues, i think?!

on other minor optimization with this would be to keep track of whether it has already centered and zoomed because it might call it multiple times depending on mix of isrefreshdraw and centerondraw.

didn't notice any ill side effects otherwise, but it's possible there's some use case i'm not accounting for. was just trying a few things :)

@plyr4
Copy link
Contributor Author

plyr4 commented Nov 7, 2023

@wass3r thank you for taking a look 🙏 i appreciate it

use renderBuildGraph updatedModel True here
im glad we're seeing the same thing. that change should be present in this PR and it fixed most of the issues i was seeing.

i'll make the other changes, good call

wass3rw3rk
wass3rw3rk previously approved these changes Nov 7, 2023
@plyr4 plyr4 merged commit 2877986 into main Nov 7, 2023
12 checks passed
@plyr4 plyr4 deleted the fix/viz-initial-load branch November 7, 2023 21:59
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.

4 participants