Skip to content

Conversation

adufr
Copy link
Contributor

@adufr adufr commented Feb 18, 2024

Related issues

Context

Show notification icons colors on app first load / app refresh.

Discussion

I've identified 2 possible fixes:

  • First one would be to simply drop this code so that we fetch the state of each notifications, even though settings.colors is falsy.

    if (!colors) {
    setNotifications(data);
    triggerNativeNotifications(
    notifications,
    data,
    settings,
    accounts,
    );
    setIsFetching(false);
    return;
    }

    Since notification.state is, for now, only required to load the icons colors, this would result on making a lot of unnecessary api calls if the setting is disabled.

  • Second one is the one I've implemented: init settings.colors to null and explicitely check for settings.colors === false, thus fixing the bug AND avoiding making unnecessary api calls

@adufr adufr changed the title fix: init settings with settings: null fix: missing icon colors on first load Feb 18, 2024
@setchy setchy merged commit fd4d383 into gitify-app:main Feb 18, 2024
@bmulholland
Copy link
Collaborator

I worry that, without comments or tests that explain the behaviour, this will be broken again in a future refactor. How can we protect against that?

adufr added a commit to adufr/gitify that referenced this pull request Feb 19, 2024
@setchy setchy added the bug Something isn't working label Mar 27, 2024
@adufr adufr deleted the fix/first-load-icons-color branch July 1, 2024 15:44
@setchy setchy added this to the Release 5.0.0 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

On first load, notification icons are missing colors

3 participants