Skip to content

Conversation

@abaroni
Copy link
Contributor

@abaroni abaroni commented Jan 20, 2025

The existing action handler "onAction" is called after an action is performed.

preAction({ type: AssetAction.TRASH, asset });
await deleteAssets({ assetBulkDeleteDto: { ids: [asset.id] } });
onAction({ type: AssetAction.TRASH, asset });

This creates a problem in handling the delete, where the code that needs the information on the asset to be deleted is executed too late, causing this bug #14647

This PR introduces the preAction handler and moves the delete handling logic from the onAction to the preAction

Fixes #14647

@alextran1502 alextran1502 changed the title fix(web): 14647 Performing "delete" from full-screen view takes clien… fix(web): delete action from full-screen reset view port in gallery view Jan 20, 2025
@alextran1502
Copy link
Member

Hello, can you help explain the logic behind the changes with a few words in the PR's description? Thank you

@abaroni
Copy link
Contributor Author

abaroni commented Jan 22, 2025

Hello, can you help explain the logic behind the changes with a few words in the PR's description? Thank you

I added a description to the PR.
If the proposed solution sounds ok, should the preAction be added to all the actions? It's not strictly needed to solve the bug, but I honestly think it should be (that's why I created this PR as draft).

@abaroni
Copy link
Contributor Author

abaroni commented Jan 26, 2025

Hello! Any feedback on this? I ask just because if I'm on the right track I'd love to contribute more

@zackpollard
Copy link
Member

We just got around to testing this and all seems to work well. With regards to converting other things to having pre actions, if you think it's worthwhile for the other actions then please feel free to open a PR making these changes.

@zackpollard zackpollard marked this pull request as ready for review March 3, 2025 18:19
@zackpollard zackpollard enabled auto-merge (squash) March 3, 2025 18:20
@zackpollard zackpollard disabled auto-merge March 3, 2025 18:20
@zackpollard zackpollard enabled auto-merge (squash) March 3, 2025 18:22
@zackpollard zackpollard merged commit f89e741 into immich-app:main Mar 3, 2025
40 checks passed
knechtandreas added a commit to knechtandreas/immich that referenced this pull request Mar 4, 2025
…n-grid

* main: (32 commits)
  docs: clean up environment variables formatting & grammar (immich-app#16555)
  fix: reset/regenerate memories (immich-app#16548)
  fix(deps): update machine-learning (immich-app#16560)
  chore(deps): update node (immich-app#16538)
  refactor: migration tag repository to kysely (immich-app#16398)
  feat: qr code for new shared link (immich-app#16543)
  chore(deps): update github-actions (immich-app#16539)
  fix(docs): info on preloading ML models (immich-app#16452)
  docs: better facial recognition cluster guide (immich-app#14911)
  fix(web): delete action closes asset viewer in asset view (immich-app#15469)
  feat(cli): watch paths for auto uploading daemon (immich-app#14923)
  ci: weblate checks should always run, should skip on en.json (immich-app#16544)
  feat(web): Video memories on web (immich-app#16500)
  fix(deps): update typescript-projects (immich-app#16540)
  chore(mobile): fix store.put type def (immich-app#16517)
  refactor(mobile): move timeline methods to timeline repo (immich-app#16526)
  chore(deps): update dependency eslint-plugin-svelte to v3 (immich-app#16532)
  refactor(server): link live photos as part of metadata extraction instead of queueing job (immich-app#16390)
  chore(deps): update dependency globals to v16 (immich-app#16534)
  ci: don't check weblate lock on chore/translations and add success job (immich-app#16533)
  ...
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.

Performing "delete" from full-screen view takes client to the top of gallery view - when starting from Poeple search

5 participants