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

feat(web): View all duplicates page #15856

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

arnolicious
Copy link
Contributor

@arnolicious arnolicious commented Feb 2, 2025

Feature Description

Add a new intermediate page, that displays all the found duplicates alongside how many duplicate assets each duplicate has.
From there, you can click on any duplicate to arrive at the "old" duplicate selection mechanism for that specific stack.
After resolving, it should move on to the next duplicate in the list
image

With this change, we can now also link from "Viewing an asset" straight to the duplication page for that duplicate stack.

image

Implementation

  • Changed the page params of the "old" duplicate page to require the duplicateId
    • Here I'm a little unsure why there's the optional [[photos=photos]] param, I assume it's not relevant anymore with these changes, but I wanted confirmation before removing it
  • Added the new page under utilities/duplicates
  • Added a link to the deduplication page from the asset viewer, if the asset has a duplicateId

@bo0tzz
Copy link
Member

bo0tzz commented Feb 2, 2025

Can you add some screenshots to the PR description?

@arnolicious
Copy link
Contributor Author

There you go :) @bo0tzz

@alextran1502
Copy link
Contributor

What happens if you have 10_000 or 20_000 duplicates? Will it crash the page?

@arnolicious
Copy link
Contributor Author

arnolicious commented Feb 3, 2025

Seems to run just fine, at least for me. I adjusted the load function of utilities/duplicates/+page.ts like this to stress test:

export const load = (async ({ params }) => {
  await authenticate();
  const asset = await getAssetInfoFromParam(params);
  const duplicates = await getAssetDuplicates();
  const $t = await getFormatter();

  const mockedDuplicates = [...duplicates];

  for (let i = 0; i < 10_000; i++) {
    mockedDuplicates.push(...duplicates);
  }

  return {
    asset,
    duplicates: mockedDuplicates,
    meta: {
      title: $t('duplicates'),
    },
  };
}) satisfies PageLoad;

@alextran1502
Copy link
Contributor

How about 20,000? usually, with these types of rendering, the load can be stressed on both the server load and the web browser crashing. I am very positive we will run into issues since the query isn't lazy paginated as well as the view doesn't have lazy loading mechanism

I chose to go with single duplicate because of the aforementioned limitation

@arnolicious
Copy link
Contributor Author

Alright, so we'd need to implement a client side pagination to merge this?

I assume only client side, since the current single duplicate implementation also already calls the entire duplicate list from the server, so this shouldn't be a problem

@alextran1502
Copy link
Contributor

Let's take a few steps and understand the problem we are trying to solve. Below are some of my feed back, and thoughts

  • The intermediate page doesn't have a clear indication that the user can click on the group of duplicates to handle them.
  • There should be bulk action on this page, and the group of duplicate should be selectable to perform deduplication.
  • I agree with you that we should have pagination on this page, breadcrumb navigation is a good solution.
  • It would be nice to show all the duplicated assets while hovered on each group

Heck, perhaps we can replace the current view if the above points are implemented. What do you think?

@NicholasFlamy
Copy link
Member

Make sure to consider #13851 before you get too far.

href="{AppRoute.DUPLICATES}/{asset.duplicateId}"
class="hover:dark:text-immich-dark-primary hover:text-immich-primary"
>
{$t('has_duplicates')}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this doesn't give a good indication that it's a button that takes you to a different view.

Copy link
Member

Choose a reason for hiding this comment

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

I like this feature to see the duplicate info on the image details. I agree with mert about it not being clear that it is clickable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to improve this in the latest iteration :D


export const load = (async ({ params }) => {
await authenticate();
const duplicates = await getAssetDuplicates();
Copy link
Contributor

Choose a reason for hiding this comment

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

This view only needs to know the number of each duplicate group, not fetch all their members. It can fetch much less data with a different endpoint that only returns the data it actually needs (one asset to display per group, and the number of duplicates in the group).

@@ -103,7 +116,17 @@
await stackAssets(assets, false);
const duplicateAssetIds = assets.map((asset) => asset.id);
await updateAssets({ assetBulkUpdateDto: { ids: duplicateAssetIds, duplicateId: null } });
const currentDuplicateIndex = duplicates.findIndex((duplicate) => duplicate.duplicateId === duplicateId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just track the index instead of doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't think I really understand by "tracking" in this context.
Where would you calculate the index? In the API endpoint?

const duplicates = await getAssetDuplicates();
const $t = await getFormatter();

const activeDuplicate = duplicates.find((duplicate) => duplicate.duplicateId === params.duplicateId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well.

@arnolicious
Copy link
Contributor Author

Alright, thanks for all the feedback! I'll give it a try in the coming days/weeks

My idea for combining the 2 views would be something like this:
Having the duplicates group be selectable, and when selected the old duplicate resolution component gets shown at the top of the page.

Is that something you had in mind?

@alextran1502
Copy link
Contributor

I think it is worth to draft a wireframe design so we don't misunderstand how the page will look like

@arnolicious
Copy link
Contributor Author

How about something like this?
The currently selected group would be the one displayed in the top section, and visible in the bottom list with a border.
image

Tho for my personal use case, I would wish to be able to see a bigger part of all the duplicates somewhere, somehow

@mertalev
Copy link
Contributor

mertalev commented Feb 7, 2025

I'm not a huge fan of the explicit numbered pages. Maybe there's a more subtle way to paginate this, or to use virtual scroll like the timeline?

@arnolicious
Copy link
Contributor Author

I did also consider this, would just be a bit more clicking if you specifically wanted to get to the end of the list for example
image

@alextran1502
Copy link
Contributor

I did also consider this, would just be a bit more clicking if you specifically wanted to get to the end of the list for example image

I like this a lot better, how are you planning to handle loading, paginating in this scroll view?

@NicholasFlamy
Copy link
Member

I did also consider this, would just be a bit more clicking if you specifically wanted to get to the end of the list for example image

I love this. This is exactly what I would like.

@arnolicious
Copy link
Contributor Author

I would create a renderedItems list that only contains the current page + the ones before and after.
And each time you change page, that list is updated like a sliding window over the entire list.

I don't think that should be too hard to implement, without the need for any additional libraries.

@atlas-shrugged08
Copy link

I happened to see this while searching for search improvements so just leaving a note to consider (or disregard), that all the pages (timeline, people, albums, search etc) in the app have a consistent theme/pattern. A vertically scrolling/paginated on scroll approach but now with the above screenshot we seem to be introducing a brand new way/pattern of horizontal scrolling on click that not only goes against the existing pattern but makes scrolling through 1000s of duplicates a tedious click through. (as opposed to the usual vertically scrolling/paginated on scroll approach in the app).

Apologies, if not relevant please disregard.

@mertalev
Copy link
Contributor

I agree a vertical scroll oriented design would be more intuitive, easier to use and more consistent with other pages.

That being said, I'm not a web dev and not sure how difficult it is to make it a virtual scroll. It's at least easy to do it the way the search result page works (load more results on scroll without removing earlier entries from the DOM). It would start having issues once you scroll enough to have >5000 groups loaded, but that might be fine tbh.

@NicholasFlamy
Copy link
Member

That being said, I'm not a web dev and not sure how difficult it is to make it a virtual scroll.

Just a bit ago I ran into the work done for the timeline and I almost fainted. (The timeline has inconsistent sizing that has to be estimated and fixed on the scrollbar and stuff, so that's why it's so complicated.)

@arnolicious
Copy link
Contributor Author

Well tbh, this was more of a ui problem than a technical one. I'm sure we can make virtual scroll work horizontally or vertically.

I just didn't have a good idea on how to make a vertically scrolling list/grid while also having the dedupe mechanism on the same page.

If someone's got ideas or suggestions, I'm open :D

@NicholasFlamy
Copy link
Member

on the same page

Is that truly necessary? What about clicking on an image pops up the current page (to the specific image you click)? And if you wanna be fancy, it could be a big (almost full-screen) popup.

@arnolicious
Copy link
Contributor Author

Well, that is the current implementation, basically. List of duplicate groups, you click on it and land on the known duplicate page.

But Alex suggested to merge those into one: #15856 (comment)

@NicholasFlamy
Copy link
Member

NicholasFlamy commented Feb 11, 2025

Well, that is the current implementation, basically. List of duplicate groups, you click on it and land on the known duplicate page.

Oh yeah, I just tried it. It's pretty good but yeah, some stuff could be addressed. Also, you should try rebasing when you merge main and then force push.

@arnolicious
Copy link
Contributor Author

arnolicious commented Feb 15, 2025

So, a little update on what I've done now:

  • Added a virtual list to virtualize the duplicates grid which should avoid performance issues on huge lists
  • Added the Bulk actions to the overview page
  • Removed the bulk actions from the detail page, since that seemed confusing / redundant now
  • Hopefully improved UX:
    • Added helper text to the grid view, to explain to click on a duplicate group to handle it
    • Added a border when hovering over the items in the grid view to show "more clickability"
    • Added a description and icon to the link from the asset detail panel to the duplicate

In wanting to implement Mert's comments, and improve effiency in general, I also added 2 new endpoints for duplicates:

  • GET duplicates/:id
  • GET duplicates/info - Gives out the list of all duplicates, but only with one asset per duplicate

However I didn't end up actually using those:

  • Since the bulk actions are now in the grid view, I do need the entire asset list there to perform the bulk actions.
  • The duplicates/:id Endpoint was meant for the detail page, however if I don't have the list of all the duplicates, idk how to load the next duplicate group after handling the current one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants