Skip to content

fix: timeline scroll after navigate #23664

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 13, 2025
Merged

fix: timeline scroll after navigate #23664

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions web/src/lib/components/timeline/Timeline.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@
return true;
};
export const scrollAfterNavigate = async ({ scrollToAssetQueryParam }: { scrollToAssetQueryParam: boolean }) => {
export const scrollAfterNavigate = async () => {
if (timelineManager.viewportHeight === 0 || timelineManager.viewportWidth === 0) {
// this can happen if you do the following navigation order
// /photos?at=<id>, /photos/<id>, http://example.com, browser back, browser back
Expand All @@ -203,16 +203,16 @@
timelineManager.viewportWidth = rect.width;
}
}
if (scrollToAssetQueryParam) {
const scrollTarget = $gridScrollTarget?.at;
let scrolled = false;
if (scrollTarget) {
scrolled = await scrollAndLoadAsset(scrollTarget);
}
if (!scrolled) {
// if the asset is not found, scroll to the top
timelineManager.scrollTo(0);
}
const scrollTarget = $gridScrollTarget?.at;
let scrolled = false;
console.log(scrollTarget, scrolled);
if (scrollTarget) {
scrolled = await scrollAndLoadAsset(scrollTarget);
}
if (!scrolled) {
console.log('scrolling?');
// if the asset is not found, scroll to the top
timelineManager.scrollTo(0);
}
invisible = false;
};
Expand All @@ -226,6 +226,7 @@
// and a new route is being navigated to. It will never be called on direct
// navigations by the browser.
beforeNavigate(({ from, to }) => {
console.log('before navigate');
timelineManager.suspendTransitions = true;
const isNavigatingToAssetViewer = isAssetViewerRoute(to);
const isNavigatingFromAssetViewer = isAssetViewerRoute(from);
Expand All @@ -235,6 +236,7 @@
// afterNavigate is only called after navigation to a new URL, {complete} will resolve
// after successful navigation.
afterNavigate(({ complete }) => {
console.log('after navigate');
void complete.finally(() => {
const isAssetViewerPage = isAssetViewerRoute(page);
Expand All @@ -245,11 +247,7 @@
initialLoadWasAssetViewer = isAssetViewerPage && !hasNavigatedToOrFromAssetViewer;
}
const isDirectTimelineLoad = isDirectNavigation && !isAssetViewerPage;
const isNavigatingFromAssetViewer = !isDirectNavigation && hasNavigatedToOrFromAssetViewer;
const scrollToAssetQueryParam = isDirectTimelineLoad || isNavigatingFromAssetViewer;
void scrollAfterNavigate({ scrollToAssetQueryParam });
Comment on lines -248 to -252
Copy link
Member Author

@danieldietzler danieldietzler Nov 6, 2025

Choose a reason for hiding this comment

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

@midzelis this is quite invasive. I couldn't find any scenario where we don't want to scroll. Could you explain in which case we don't want to scroll?

Copy link
Collaborator

@midzelis midzelis Nov 13, 2025

Choose a reason for hiding this comment

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

I'm fine always attempting to scroll to query param for all timeline/asset-viewer routes if it works and solves the problem.

This is less important since #22838 was merged. Previously, if you had scrolled when reloading the timeline due to a browser back or asset-viewer back-arrow, and the asset you were viewing was mid-viewport, if you had scrolled again to that asset, it would always move the asset to the top/left of the viewport, moving it away from its current mid-viewport position.

However, there is still one place where you don't really want to scroll - and thats on direct loads of asset-viewer. (i.e. /photos/<id>?at=<id>) - that may be a bit contrived, because you'd never really want to specify an ?at= param on the asset-viewer routes, but because the asset-viewer loads literally on top of the timeline components, this might have caused issues.

The bigger problem is the fact that several timebucket API calls, memories API calls, etc are called on asset-viewer routes - which are completely invisible behind the asset-viewer. But thats a task for another day.

Copy link
Member Author

@danieldietzler danieldietzler Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh got it that makes sense. Then I'd like to go with this solution for now. And yeah, the asset viewer only being a full screen overlay makes a lot of things kind of annoying; definitely warrants a larger discussion eventually

void scrollAfterNavigate();
});
});
Expand Down Expand Up @@ -306,7 +304,7 @@
}
};
// note: don't throttle, debounch, or otherwise make this function async - it causes flicker
// note: don't throttle, debounce, or otherwise make this function async - it causes flicker
const handleTimelineScroll = () => {
if (!scrollableElement) {
return;
Expand Down Expand Up @@ -544,7 +542,7 @@
if (asset) {
$gridScrollTarget = { at: asset };
}
void scrollAfterNavigate({ scrollToAssetQueryParam: true });
void scrollAfterNavigate();
}}
onBeforeUpdate={(payload: UpdatePayload) => {
const timelineUpdate = payload.updates.some((update) => update.path.endsWith('Timeline.svelte'));
Expand Down
Loading