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: Do not disconnect before update #141

Closed
wants to merge 1 commit into from

Conversation

lmuntaner
Copy link
Collaborator

Motivation

InfiniteScroll didn't trigger intersection always.

Changes

  • Do not disconnect observer before update.

@lmuntaner
Copy link
Collaborator Author

@peterpeterparker please review

Was the reason to disconnect before update only to avoid a layout shift? I found a bug on mainnet that it's not requesting the voting history in the neuron details page.

I was able to replicate it by:

  1. Refresh in a neuron detail page
  2. Scroll down to the voting history
  3. The voting history is updated correctly with infinite scroll
  4. Go back to neurons page.
  5. Click on the same neuron as before.
  6. Scroll down to the voting history.
  7. The voting history is not updated with the infinite scroll.

@peterpeterparker
Copy link
Member

peterpeterparker commented Dec 28, 2022

As described in code

We disconnect previous observer before any update. We do want to trigger an intersection in case of layout shifting.

i.e. we do not want to trigger unnecessary query and update on proposal list views. This is particularly useful when the filter "hide proposals were I already voted" is selected.

@lmuntaner
Copy link
Collaborator Author

The problem was that the observer somehow didn't connect again then...

I'll keep playing, but have you replicated the issue in mainnet?

@peterpeterparker
Copy link
Member

Not yet, I'll have a look tomorrow or next week

@lmuntaner
Copy link
Collaborator Author

ok, no worries.

@peterpeterparker
Copy link
Member

@lmuntaner thanks for the reproduction details, very useful 👍

I found the root cause of the issue and implemented a workaround so that it preserves the existing feature which is useful for the "Proposal list" view 👉 #142

Therefore I close this PR but for sure reopen it if I am missing something.

@lmuntaner
Copy link
Collaborator Author

@peterpeterparker thank you, that's perfect!

peterpeterparker added a commit that referenced this pull request Dec 30, 2022
…n bindings are used (#142)

# Motivation

We disable the `<InfiniteScroll />` component `beforeUpdate` because we do want to trigger an intersection in case of layout shifting - i.e. we do not want to trigger additional unexpected query and update calls in the proposal list views.

We noticed that under certain circumstances, notably when initialized while enabling at the same time, a race condition can lead to the scroller being disabled.

Root cause of the problem is a Svelte issue [#6016](sveltejs/svelte#6016). Indeed, when bindings are used (`bind:this=my_HTML_element`) within the compoment, the `beforeUpdate` lifecycle is called twice while `afterUpdate` is called only once. Therefore, as we disable the scroll in the first method and enable in the second, the scroller remains disabled.

# Reproduction

See PR #141 [comment](#141 (comment)).

# Changes

- implement a workaround to ignore second `beforeUpdate` call trigger by binding initialization
@peterpeterparker peterpeterparker deleted the FIX_LM_infinite-scroll-not-triggered branch December 30, 2022 20:06
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