-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(web): fix lost scrollpos on deep link to timeline asset, scrub stop #16305
base: main
Are you sure you want to change the base?
Conversation
Deploying preview environment to https://pr-16305.preview.internal.immich.cloud/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still the issue where the wasm library can't be imported on Webkit because it uses top-level await. The approach of importing in the asset store constructor is too racy. Is there a good way to do a dynamic import without needing to make the geometry update etc. async?
@@ -52,7 +52,7 @@ | |||
<AddToAlbum /> | |||
<AddToAlbum shared /> | |||
</ButtonContextMenu> | |||
<FavoriteAction removeFavorite={assetInteraction.isAllFavorite} onFavorite={() => assetStore.triggerUpdate()} /> | |||
<FavoriteAction removeFavorite={assetInteraction.isAllFavorite} onFavorite={() => void 0} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to not provide a function instead?
In this version, the geometry update is not async. the import is in the init() method. I could move it the top level instead (without an await) - and then await it in the init() method. That will start loading the import a tiny bit earlier. However, there is no concern of importing the same module multiple times, as ESM will not import the same module if its already imported. Maybe I'm not understanding the workaround? |
const { getJustifiedLayoutFromAssets } = await import('$lib/utils/layout-utils'); | ||
this.getJustifiedLayoutFromAssets = getJustifiedLayoutFromAssets; | ||
await this.initialiazeTimeBuckets(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This admittedly looks more reliable than #16267. Have you tested the PR with cleared cache and incognito mode? I just want to be sure it isn't relying on the import being instant.
Hi Min, amazing work as always on the timeline! I've just finished the first round of testing. I didn't notice that the FPS dropped, so we are golden there. However, I saw two issues.
|
Some of these were caused by the wasm-layout and wasm-adjacent changes. I just reverted those changes from this PR, so this should now have whats in main, and just the bug fixes for scroll pos. Please give it another test. -- the test deployment seems flaky/stale -- so i would avoid deployment URL for now. |
Description
This fixes the problems in maintaining scroll position (i.e. to a given deep-linked bucket) as the buckets above the current bucket as loaded and change height. The same problem also happened when clicking on the scrub bar on an unloaded part of the timeline (towards the bottom).
The root causes:
id='bucket'
was changed to a bucket class instead, causing_updateLastIntersectedBucketDate
to fail_updateLastIntersectedBucketDate
was not working properly anyways, since on scrub, the scrubbar creates an invisible overlay over the entire#asset-grid
causing getElementAtPoint to fail.Other changes
How Has This Been Tested?