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

Immich CLI add to album broken on windows #10617

Closed
1 of 3 tasks
waqasali101 opened this issue Jun 25, 2024 · 6 comments · Fixed by #10626
Closed
1 of 3 tasks

Immich CLI add to album broken on windows #10617

waqasali101 opened this issue Jun 25, 2024 · 6 comments · Fixed by #10626

Comments

@waqasali101
Copy link

waqasali101 commented Jun 25, 2024

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

  • Server
  • Web
  • Mobile

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
Copy link
Member

bo0tzz commented Jun 25, 2024

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

@waqasali101
Copy link
Author

waqasali101 commented Jun 25, 2024

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
Copy link
Author

@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
Copy link
Contributor

fky2015 commented Jun 25, 2024

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

@fky2015
Copy link
Contributor

fky2015 commented Jun 25, 2024

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

@fky2015
Copy link
Contributor

fky2015 commented Jun 25, 2024

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.

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

Successfully merging a pull request may close this issue.

3 participants