feat(web): assets now have a permanent URL #8532

midzelis · 2024-04-05T05:12:55Z

Add stable urls for all assets. Navigating within app will update browser location. Reloading the page will properly load the asset within the context.

#2906
#7017

alextran1502 · 2024-04-05T15:01:01Z

Hello, I just finished the first round of testing; one issue I am seeing is using the mouse clicking on the back button from the asset-viewer component, the URL isn't updated, and then when you click on a different asset to open, the first click is to clear the URL, the second click then opens the asset-viewer

midzelis · 2024-04-05T21:05:24Z

Yeah, something slipped in late last night. Took a look with fresh eyes today and fixed the problem. Extracted the url manipulation into the navigation.ts util. Force pushed to resolve some conflicts with main. Not sure if this project prefers merge commits or rebases in PRs.

jrasm91 · 2024-04-06T01:14:20Z

It doesn't matter because we'll squash it into a single commit on main.

alextran1502 · 2024-04-06T13:30:45Z

web/src/routes/(user)/search/+page.ts

@@ -1,11 +0,0 @@
-import { authenticate } from '$lib/utils/auth';


Why do we remove this file?

alextran1502 · 2024-04-06T13:31:26Z

There are a few +page.ts file with the following content get removed, why is that?

import type { PageLoad } from './$types';	

export const load = (async () => {	
  await authenticate();	
  return {	
    meta: {	
      title: 'Search',	
    },	
  };	
}) satisfies PageLoad;

klejejs · 2024-04-07T11:08:49Z

Should fix #8238 as well. Thanks for working on this!

danieldietzler · 2024-04-08T15:51:42Z

Will also close #4746

midzelis · 2024-04-09T21:12:08Z

I've split up this PR into very small, hopefully easy to understand, commit.

I've tried hard to not update all the paths, but ultimately failed. However, I was able to remove the [...rest] approach, and use optional path params instead. However, because of the way sveltekit does routing, if there is common functionality that cross-cuts multiple routes (i.e. the <routeBase>/photos/:assetId part) - you have to add that loader to each +page.ts. If the route had a common prefix, parent +page.tsloaders would run in parallel with child +page.ts loaders. Based on the immich route structure, assets come at the end of the path, so there are no common paths, and so each leaf +page.ts loader needs to run the load function itself. I have at least factored the load function into a util.

By using optional params, I don't need to parse the URL anymore. However, I still need to do that when I need to handle previous/next and open-asset navigation. That is because sveltekit doesn't expose https://github.com/sveltejs/kit/blob/main/packages/kit/src/utils/routing.js which would make it easier to parse, find/replace route segments. i.e. I could find the current route, and call resolve_route with the new route to create the new URL - alas, I have to resort to doing the regexp find/replace, which isn't bad, but it does create a very fragile connection between the routes (which in sveltekit are represented by paths (!!) - and the references in code to these routes.) This however, is not a new problem - any call to goto() in the codebase will need to refer to a route by url, and not by route.id. This is just a weirdness with sveltekit routing.

Ok - now on to the actual meat of this PR - updating the URL with the new path when opening/closing and prev/next buttons are pushed.

Previously, I had the navigation (the goto() call) performed directly in the asset-viewer/asset-grid. In this PR, I wanted to try to remove knowledge of the Routes from the components. So that only the sources in under /src/routes would be aware of the routing-specific navigation paths. To do this, instead of performing the goto call directly in the component, instead I use events. Again, ran into weirdness: Svelte component events don't bubble, and they can't be made to.

Here, I created some custom DOM events, and have the asestviewer/grid send these events at the right time. Then, once these events work they're way back up to the layout (which is part of /src/routes) it will figure out the route to navigate to.

Now - as I was typing this, I googled a bit about bubbling events in svelte - and it turns out there is way they want you to do this - thats to forward them: https://svelte.dev/examples/event-forwarding

In light of this guidance, I can rework the custom events to just use forwarding of the standard events. But before I do that, my question to ya'll is - do you want the components to directly call goto() or do you want them to use events that get forwarded up to either +page.svelte or +layout.svelte and get handled there? Note, there already are several places where goto is called from component code. If its agreed to go down this road, there should be a refactoring in a future PR to fix up goto() with events, and have them handled buy the routes components.

midzelis · 2024-04-09T21:15:00Z

There are a few +page.ts file with the following content get removed, why is that?

I believe these are the redirects (before this pr, navigations to /album/:id/photos/:id would redirect to /album/:id/photos, etc. This new force-push has easier to follow step-by-step commits that should help you understand whats happening, if you view each commit 1 by 1.

danieldietzler · 2024-04-09T21:23:05Z

@midzelis Firstly, thanks a lot for this write-up and for investing the time and efforts! :)
Regarding the event topic: I personally would prefer one event different pages can emit. To fix your issue with them (and for some other reasons) Svelte 5 will actually deprecate the emit/dispatch stuff in favor of simply passing through functions basically. In fact, we're slowly transitioning to that already so that we're prepared to switch to svelte 5 once available. It would be awesome if you could switch to that new model as well already; especially I think it could even fix some of the issues you have with it. For reference: https://svelte-5-preview.vercel.app/docs/event-handlers

alextran1502 · 2024-04-09T21:27:21Z

There are a few +page.ts file with the following content get removed, why is that?

I believe these are the redirects (before this pr, navigations to /album/:id/photos/:id would redirect to /album/:id/photos, etc. This new force-push has easier to follow step-by-step commits that should help you understand whats happening, if you view each commit 1 by 1.

From what I see, for pages that don't have +page.ts, you cannot navigate to those pages directly from the address bar, or ctrl + f5 won't take effect. It will have to be done by clicking on the navigation button

edit: I commented before reading your write up, thank you

alextran1502 · 2024-04-10T04:02:53Z

Hello @midzelis, I was running some functional testing of this PR. There are a few things/bugs I noticed

Clicking on Explore/Map loads the same route
Go to Explore > click on a Place > click on an asset to enter the detail view > Ctrl + R will show an error prompt.
Search for an asset > enter the detail view > click on the close button on upper top left corner > the detail view will not be close
Search for an asset > enter the detail view > Ctrl + F5 > the detail view disappear, return to the search result grid but no result is shown because the path is now having the following value /search/photos/80a7129a-26f1-4dbf-a533-71192ffb69fd?query=%7B"query"%3A"test"%7D
When navigating between assets, I can see the loading bar on top, I would like this to be removed when navigating between assets.

Let's fix those first before another round of testing 😄

Thanks again for the amazing work!

benmccann · 2024-04-10T17:41:24Z

nice! switching from [..rest] to [[optional]] made this much cleaner!

However, because of the way sveltekit does routing, if there is common functionality that cross-cuts multiple routes (i.e. the /photos/:assetId part) - you have to add that loader to each +page.ts.

I haven't checked if you could apply it in this PR, but you can share functionality without sharing URL paths with groups: https://kit.svelte.dev/docs/advanced-routing#advanced-layouts-group. However, it applies all or none to a directory still and so if you wanted to apply it to a subset of routes, as I think may be the case here, it may not be of use

sveltekit doesn't expose https://github.com/sveltejs/kit/blob/main/packages/kit/src/utils/routing.js

I've tried 😄 It's contentious though. I think the concerns are that exposing an API would reduce our flexibility to make changes. Now that things are a bit more stable and we've had multiple major releases maybe it could be revisited at some point - though everyone's pretty busy with other things so I doubt it'd happen soon

danieldietzler

I'm really liking the code so far! :)

danieldietzler · 2024-04-12T10:18:51Z

web/src/lib/components/shared-components/drag-and-drop-upload-overlay.svelte


-  $: albumId = ($page.route?.id === '/(user)/albums/[albumId]' || undefined) && $page.params.albumId;
-  $: isShare = $page.route?.id === '/(user)/share/[key]' || undefined;
+  $: albumId = isAlbumsRoute($page.route?.id) ? $page.params.albumId : undefined;


You could probably simplify this

Suggested change

$: albumId = isAlbumsRoute($page.route?.id) ? $page.params.albumId : undefined;

$: albumId = isAlbumsRoute($page.route?.id) && $page.params.albumId;

Does this not work?

danieldietzler · 2024-04-12T10:25:23Z

web/src/lib/utils/navigation.ts

+export const isPhotosRoute = (route?: string | null) => route?.startsWith('/(user)/photos/[[assetId=id]]') || false;
+export const isSharedLinkRoute = (route?: string | null) => route?.startsWith('/(user)/share/[key]') || false;
+export const isSearchRoute = (route?: string | null) => route?.startsWith('/(user)/search') || false;
+export const isAlbumsRoute = (route?: string | null) => route?.startsWith('/(user)/albums/[albumId=id]') || false;
+export const isPeopleRoute = (route?: string | null) => route?.startsWith('/(user)/people/[personId]') || false;


You could remove all those || false and instead write !!route?.startsWith(...). Probably up to personal preference though

danieldietzler · 2024-04-12T10:27:02Z

web/src/lib/utils/navigation.ts

+export const isAssetViewerRoute = (target?: NavigationTarget | null) =>
+  (target?.route.id?.endsWith('/[[assetId=id]]') && 'assetId' in (target?.params || {})) || false;
+
+export async function getAssetInfoFromParam(params: { assetId?: string }) {


Suggested change

export async function getAssetInfoFromParam(params: { assetId?: string }) {

export async function getAssetInfoFromParam({ assetId }: { assetId?: string }) {

danieldietzler · 2024-04-12T10:30:22Z

web/src/lib/utils/navigation.ts

+  const { assetId } = route;
+  const next = assetId ? currentUrlReplaceAssetId(assetId) : currentUrlWithoutAsset();
+  if (next !== currentUrl()) {
+    void goto(next, { replaceState: false });


Why isn't this function async and we're awaiting the goto?

web/src/params/id.ts

danieldietzler · 2024-04-12T10:36:10Z

web/src/routes/(user)/map/[[photos=photos]]/[[assetId=id]]/+page.svelte

-        on:close={() => assetViewingStore.showAssetViewer(false)}
-        isShared={false}
-      />
+      {#await import('../../../../../lib/components/asset-viewer/asset-viewer.svelte') then AssetViewer}


Can we... not do that? lol
An absolute import would be much nicer here haha. Also, why do even need to import it inline?

I believe it was imported via this await block in order to speed initial page load

danieldietzler

Nice!

danieldietzler · 2024-04-14T10:51:48Z

web/src/lib/stores/assets.store.ts

+    if (!bucketInfo) {
+      let date = fromLocalDateTime(localDateTime);
+      if (this.options.size == TimeBucketSize.Month) {
+        date = date.set({ day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });
+      } else if (this.options.size == TimeBucketSize.Day) {
+        date = date.set({ hour: 0, minute: 0, second: 0, millisecond: 0 });
+      }
+      await this.loadBucket(date.toISO()!, BucketPosition.Unknown);
+    }
+    return this.assetToBucket[id] || null;


Suggested change

if (!bucketInfo) {

let date = fromLocalDateTime(localDateTime);

if (this.options.size == TimeBucketSize.Month) {

date = date.set({ day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });

} else if (this.options.size == TimeBucketSize.Day) {

date = date.set({ hour: 0, minute: 0, second: 0, millisecond: 0 });

}

await this.loadBucket(date.toISO()!, BucketPosition.Unknown);

}

return this.assetToBucket[id] || null;

if (bucketInfo) {

return bucketInfo;

}

let date = fromLocalDateTime(localDateTime);

if (this.options.size == TimeBucketSize.Month) {

date = date.set({ day: 1, hour: 0, minute: 0, second: 0, millisecond: 0 });

} else if (this.options.size == TimeBucketSize.Day) {

date = date.set({ hour: 0, minute: 0, second: 0, millisecond: 0 });

}

await this.loadBucket(date.toISO()!, BucketPosition.Unknown);

return this.assetToBucket[id] || null;

web/src/params/id.ts

midzelis · 2024-04-14T14:55:01Z

I was also thinking that maybe we don't want to use the raw UUID in the url.

If you're messing around in the database (for things like development or troubleshooting) it's really nice to be able to just slap a UUID straight into your browser, so IMO we should just keep them raw.

@bo0tzz
fyi @alextran1502

After thinking about it, I would vote for a move away from UUIDs entirely. I would say, use nanoid to generate the primary key - and store that nanoid id in the db.

Personally, I don't think we need 340 undecillion records (340,282,366,920,938,000,000,000,000,000,000,000,000) in the db ;-)

Using nanoid, with just 12 characters (which is generous), we get 9B ids before repetition, or if you are uploading 1000 pictures/hour, it will take 1,000 years to exhaust. You can play with a calculator here: https://zelark.github.io/nano-id-cc/ (Honestly, 8chars which is 2M ids is probably enough)

And you could make it dynamic - if you run out of ids, just bump the length, and generate new ids with the new size.

To handle collisions, there would need to be a retry mechanism on insert. Try to insert with generated id, if collision, then generate a new id and try again.

I think I'll prototype this in another PR to see how it works. The beauty is that we don't need to update any of the existing ids - they can still work. But new assets will have smaller ids. Or, we could run a script to update all the existing ids to the new format, since nobody really has links to assets by id anyways, but there could be other API integrations out there - so I would make it a total optional thing, like the storage template migration job.

danieldietzler

Nice one! :)

server/test/assets

alextran1502 added the 🖥️web label Apr 5, 2024

midzelis force-pushed the asset_urls branch from c07e27d to 516da3a Compare April 5, 2024 21:01

alextran1502 reviewed Apr 6, 2024

View reviewed changes

midzelis force-pushed the asset_urls branch from 516da3a to 506be1c Compare April 9, 2024 20:30

midzelis added 6 commits April 9, 2024 20:36

Remove asest redirect pages

9942314

Rename route paths to handle optional assetId

f70ebcf

Update old references to new routes

19417fc

Load and display asset from all routes that can show assetId

60b8461

Add <main> in base layout, update portals to target it

be13630

Wire up updating navigation in response to open/close/prev/next

2cf8520

midzelis force-pushed the asset_urls branch from 506be1c to 2cf8520 Compare April 9, 2024 20:48

midzelis and others added 4 commits April 10, 2024 01:23

Replace events with navigation functions

c75057c

Add types to param matcher

cc7842e

misc cleanup

703213c

Merge branch 'main' of github.com:immich-app/immich into asset_urls

88b0b23

mertalev changed the title ~~Assets now have permament URL~~ Assets now have permanent URL Apr 10, 2024

danieldietzler changed the title ~~Assets now have permanent URL~~ feat(web): assets now have a permanent URL Apr 10, 2024

Fix reload on /search pages

74d6a00

danieldietzler reviewed Apr 12, 2024

View reviewed changes

midzelis added 6 commits April 13, 2024 13:53

Mostly style, review comments

01717b4

Load buckets for assets on demand

7ac8bd6

Forgot this update call

788692a

typo

5e7939e

fix test

e667c31

Fix carelessness

2e4a099

midzelis requested review from benmccann, danieldietzler and jrasm91 April 13, 2024 15:20

danieldietzler reviewed Apr 14, 2024

View reviewed changes

midzelis added 2 commits April 14, 2024 15:02

Review comment

9412391

Merge remote-tracking branch 'upstream/main' into asset_urls

b0f06db

midzelis mentioned this pull request Apr 15, 2024

POC of short asset ids #8799

Closed

midzelis and others added 3 commits April 18, 2024 18:54

Merge remote-tracking branch 'upstream/main' into asset_urls

c4904bb

Merge branch 'main' of github.com:immich-app/immich into asset_urls

12730e8

merge main

fc83f51

alextran1502 approved these changes Apr 24, 2024

View reviewed changes

alextran1502 enabled auto-merge (squash) April 24, 2024 19:08

danieldietzler approved these changes Apr 24, 2024

View reviewed changes

server/test/assets Outdated Show resolved Hide resolved

danieldietzler disabled auto-merge April 24, 2024 19:10

alextran1502 and others added 2 commits April 24, 2024 14:14

remove assets

c5743b8

fix submodule

dd68145

alextran1502 enabled auto-merge (squash) April 24, 2024 19:23

alextran1502 merged commit a782602 into immich-app:main Apr 24, 2024
22 checks passed

This was referenced Apr 24, 2024

basic support for permanent url in gallery view #4746

Closed

[BUG] photos should have a permanent url #2906

Closed

[BUG] State not correctly persisted in URL #2690

Closed

Snowknight26 mentioned this pull request May 29, 2024

Navigating to/from non-album share breaks browser navigation #8999

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(web): assets now have a permanent URL #8532

feat(web): assets now have a permanent URL #8532

midzelis commented Apr 5, 2024

alextran1502 commented Apr 5, 2024

midzelis commented Apr 5, 2024

jrasm91 commented Apr 6, 2024

alextran1502 Apr 6, 2024

alextran1502 commented Apr 6, 2024

klejejs commented Apr 7, 2024

danieldietzler commented Apr 8, 2024

midzelis commented Apr 9, 2024 •

edited

midzelis commented Apr 9, 2024

danieldietzler commented Apr 9, 2024 •

edited

alextran1502 commented Apr 9, 2024 •

edited

alextran1502 commented Apr 10, 2024 •

edited

benmccann commented Apr 10, 2024

danieldietzler left a comment

danieldietzler Apr 12, 2024

danieldietzler Apr 14, 2024

danieldietzler Apr 12, 2024

danieldietzler Apr 12, 2024

danieldietzler Apr 12, 2024

danieldietzler Apr 12, 2024 •

edited

benmccann Apr 12, 2024

danieldietzler left a comment

danieldietzler Apr 14, 2024

midzelis commented Apr 14, 2024

danieldietzler left a comment

		@@ -1,11 +0,0 @@
		import { authenticate } from '$lib/utils/auth';

	$: albumId = isAlbumsRoute($page.route?.id) ? $page.params.albumId : undefined;
	$: albumId = isAlbumsRoute($page.route?.id) && $page.params.albumId;

	export async function getAssetInfoFromParam(params: { assetId?: string }) {
	export async function getAssetInfoFromParam({ assetId }: { assetId?: string }) {

feat(web): assets now have a permanent URL #8532

feat(web): assets now have a permanent URL #8532

Conversation

midzelis commented Apr 5, 2024

alextran1502 commented Apr 5, 2024

midzelis commented Apr 5, 2024

jrasm91 commented Apr 6, 2024

Choose a reason for hiding this comment

alextran1502 commented Apr 6, 2024

klejejs commented Apr 7, 2024

danieldietzler commented Apr 8, 2024

midzelis commented Apr 9, 2024 • edited

midzelis commented Apr 9, 2024

danieldietzler commented Apr 9, 2024 • edited

alextran1502 commented Apr 9, 2024 • edited

alextran1502 commented Apr 10, 2024 • edited

benmccann commented Apr 10, 2024

danieldietzler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietzler Apr 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldietzler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midzelis commented Apr 14, 2024

danieldietzler left a comment

Choose a reason for hiding this comment

midzelis commented Apr 9, 2024 •

edited

danieldietzler commented Apr 9, 2024 •

edited

alextran1502 commented Apr 9, 2024 •

edited

alextran1502 commented Apr 10, 2024 •

edited

danieldietzler Apr 12, 2024 •

edited