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

fix: websockets calling on_new_release across all sessions upon new websocket connection. #16339

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

AdamT20054
Copy link
Contributor

@AdamT20054 AdamT20054 commented Feb 26, 2025

Description

Added a new variable modalAcknowledged to session storage to check against before displaying the new_release window/popup.

Flow of operations

Modal not acknowledged:

  • New websocket connection is made
  • version-announcement-box.svelte runs the conditional:
$effect(() => {
    if (($release?.isAvailable) && (!sessionStorage.getItem('modal Acknowledged'))) {
      handleRelease();
    }
  });
  • $release?.isAvailable returns true and modalAcknowledged returns false, invoking handleRelease()
  • handleRelease sets showModal to true - showing the popup
  • When onAcknowledged is invoked, modalAcknowledged is set to true in sessionStorage

Modal acknowledged

  • New websocket connection is made
  • version-announcement-box.svelte runs the conditional:
$effect(() => {
    if (($release?.isAvailable) && (!sessionStorage.getItem('modalAcknowledged'))) {
      handleRelease();
    }
  });
  • $release?.isAvailable returns true and modalAcknowledged returns true, NOT triggering handleRelease.

The same thing could be achieved within the handleRelease() function should you want that to be called

const handleRelease = () => {
    try {
      if (localStorage.getItem('appVersion') === releaseVersion) {
        return;
      }

      if (sessionStorage.getItem('modalAcknowledged')) {return;}
      showModal = true;
    } catch (error) {
      console.error('Error [VersionAnnouncementBox]:', error);
    }
  };

though I think having it in it's current implementation is better, as it avoids the function being called at all if the modals been acknowledged.

Why is this change required? What problem does it solve?

Whenever a new websocket connection is made, and an update for the server is available, the update window pops up on ALL active web sessions. An example would be:

  • you have an outdated Immich version open on your PC
  • you dismiss the update reminder.
  • You open Immich mobile (Android, in my case)
  • The update reminder then reappears on PC.

The same would happen if you opened another immich tab. I believed this to be down to version-announcement-box.svelte being ran whenever ANY new websocket connection is made (I could be wrong, I am tired). In any case, the provided PR should fix the issue.

Fixes issues:

Fixes #14009

How Has This Been Tested?

Within /web: npm run test

Before I made any changes, the output were as follows:
Test Files 16 failed | 19 passed (35) Tests 55 failed | 94 passed (149) Start at 06:49:49 Duration 5.36s (transform 4.41s, setup 16.20s, collect 7.14s, tests 5.28s, environment 26.42s, prepare 4.15s)
The failed files were mostly to do with languages.

After I made my changes, the output is as follows:
Test Files 16 failed | 19 passed (35) Tests 55 failed | 94 passed (149) Start at 07:48:54 Duration 4.93s (transform 3.83s, setup 14.23s, collect 6.78s, tests 4.89s, environment 23.68s, prepare 4.09s)

npm run check:all

[warn] Code style issues found in 511 files. Run Prettier with --write to fix.

The only changed file in this PR is NOT listed in these files.

Checklist:

  • [True ] I have performed a self-review of my own code
  • [N/A] I have made corresponding changes to the documentation if applicable
  • [True ] I have no unrelated changes in the PR.
  • [N/A] I have confirmed that any new dependencies are strictly necessary.
  • [N/A] I have written tests for new code (if applicable)
  • [True] I have followed naming conventions/patterns in the surrounding code
  • [N/A] All code in src/services uses repositories implementations for database calls, filesystem operations, etc.
  • [N/A] All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services)

Please let me know if I've had any oversights, I'll be happy to amend any issues :)

…ss all active sessions when a new websocket connection is established.
Changes not needed to websocket.ts - was bouncing between ideas, current implementation doesn't need this to change.
@AdamT20054
Copy link
Contributor Author

AdamT20054 commented Feb 26, 2025

If you wanted it to persist instead of only being stored per tab you can switch to localStorage, however I'd advise against this personally unless we implement functionality to trigger the popup again after a set time (can be done by setting modalAcknowledged to false or a non truthy value).

@alextran1502
Copy link
Contributor

With this logic, once you acknowledge the changes for one release, when the next release happens, the modal won't show, correct?

@AdamT20054
Copy link
Contributor Author

With this logic, once you acknowledge the changes for one release, when the next release happens, the modal won't show, correct?

Correct. I've specifically chosen to use session storage over local storage for this reason; Once the tab is closed, the popup will continue to reappear.

@alextran1502
Copy link
Contributor

With this logic, once you acknowledge the changes for one release, when the next release happens, the modal won't show, correct?

Correct. I've specifically chosen to use session storage over local storage for this reason; Once the tab is closed, the popup will continue to reappear.

Ah I didn't see that. Thanks for the explaination

@alextran1502 alextran1502 enabled auto-merge (squash) February 26, 2025 17:17
@AdamT20054
Copy link
Contributor Author

With this logic, once you acknowledge the changes for one release, when the next release happens, the modal won't show, correct?

Correct. I've specifically chosen to use session storage over local storage for this reason; Once the tab is closed, the popup will continue to reappear.

Ah I didn't see that. Thanks for the explaination

I share your concern if it was ever moved out of session storage (into local), but I think as a temporary measure it should suffice.

Svelte isn't a framework i'm hugely familiar with, but if you think a revised timer-based dismisser would be worth it I'd be happy to work on it when I get the chance.

Eg, a setting in the admin panel that lets the user define how long a dismissed modal will stay dismissed for. That would carry over across all sessions, so we'd need logic to enable the modal to reappear after a specified time.

@alextran1502 alextran1502 merged commit 2969e25 into immich-app:main Feb 26, 2025
41 of 42 checks passed
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.

Update reminder opens on all browsers when a new connection is made.
3 participants