immich-app / immich

High performance self-hosted photo and video management solution.
https://immich.app
GNU Affero General Public License v3.0
47.33k stars 2.4k forks source link

Immich CLI add to album broken on windows #10617

Closed waqasali101 closed 3 months ago

waqasali101 commented 3 months ago

The bug

NOTE: I am awaring a recent fix went in to fix recursive behavior on windows, this test is including that fix

Add to album is broken when using CLI on windows. Reason for this is that filePath in assests.ts is probably being unifixied somewhere and following line in method getAlbumName resolves to undefined

const folderName = os$1.platform() === "win32" ? filepath.split("\").at(-2) : filepath.split("/").at(-2);

filePath

The OS that Immich Server is running on

Windows 11

Version of Immich Server

N/A

Version of Immich Mobile App

N/A

Platform with the issue

Your docker-compose.yml content

N/A

Your .env content

N/A

Reproduction steps

1. On windows, latest version including changes to fix recursive bug on windows
2. 
3.
...

Relevant log output

No response

Additional information

No response

bo0tzz commented 3 months ago

@fky2015 is this a regression from your PR #10430?

waqasali101 commented 3 months ago

looking at code it does not seem like a regression. Is it possible it was always broken? I used upload --album with --recursive flag temporarily changing that file in node_modules fixed it for me to atleast continute

waqasali101 commented 3 months ago

@bo0tzz you were right, this is caused by #10430 this is because convertPathToPattern changes slashes. I believe this is intended behavior of convertPathtoPattern from fast-glob. does that mean we should only split on / ?

fky2015 commented 3 months ago

@waqasali101 I'm still trying to understand what the bug is. Could you provide a more concrete reproducible method?

fky2015 commented 3 months ago

Never mind the above question. I'll make a PR ASAP.

fky2015 commented 3 months ago

does that mean we should only split on / ?

That's the one way.

To ensure compatibility in case this function is called with both / and \, I am using path to replace the manually-written parsing code.