search

cybersemics / em

A beautiful, minimalistic note-taking app for personal sensemaking.
Other
258 stars 88 forks source link

Fix useMultiline store race condition #2081

Closed finkef closed 4 days ago

finkef commented 1 week ago

Alternative implementation of #2077.

Fixes #2041, #2049.

This PR fixes multiline calculation by making sure useLayoutEffect is not executed out-of-sync with React's scheduler and instead only triggered by hook dependencies. The performance seems to not be effected by this change.

finkef commented 1 week ago

Moved all layout calculations to useLayoutEffect and rewrote react ministore's useSelector to use useSyncExternalStore. After many times going back and forth of trying different store subscription techniques for trying to reduce subscription invocations, it turns out that React's internal comparisons inside useSyncExternalStore easily perform best while keeping the code at a minimum.

raineorshine commented 1 week ago

There are two very closely related test cases that are slightly different than #2041 and #2049. I created separate issues for them for clarity, but wanted to bring them to your attention now since they involve the same height/line-height rendering calculations.

If they are not automatically fixed with one of your PR's for our existing issues, then we can add them to the budget.

finkef commented 5 days ago

There are two very closely related test cases that are slightly different than #2041 and #2049. I created separate issues for them for clarity, but wanted to bring them to your attention now since they involve the same height/line-height rendering calculations.

If they are not automatically fixed with one of your PR's for our existing issues, then we can add them to the budget.

Thanks, will check them out. On first glance, I'm guessing they should be fixed with the PRs already.

finkef commented 5 days ago

@raineorshine Resolved all comments. 👍

finkef commented 4 days ago

There are two very closely related test cases that are slightly different than #2041 and #2049. I created separate issues for them for clarity, but wanted to bring them to your attention now since they involve the same height/line-height rendering calculations.

If they are not automatically fixed with one of your PR's for our existing issues, then we can add them to the budget.

@raineorshine Can confirm that both of these are fixed by #2079 that is dependent on this PR.