Skip to content

Conversation

@adufr
Copy link
Contributor

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

@adufradufr changed the title fix: init settings with settings: nullfix: missing icon colors on first loadFeb 18, 2024
@setchysetchy merged commit fd4d383 into gitify-app:mainFeb 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
@setchysetchy added the bug Something isn't working label Mar 27, 2024
@adufradufr deleted the fix/first-load-icons-color branch July 1, 2024 15:44
@setchysetchy added this to the Release 5.0.0 milestone Jul 17, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't working

Development

Successfully merging this pull request may close these issues.

On first load, notification icons are missing colors

3 participants

@adufr@bmulholland@setchy