Skip to content

fix(ui): Group IPEX refresh notifications are duplicated in Redux if we are in another profile#1574

Open
Sotatek-DukeVu wants to merge 1 commit intodevelopfrom
VT20-2572-group-ipex-refresh-notifications-are-duplicated-in-redux-if-we-are-in-another-profile
Open

fix(ui): Group IPEX refresh notifications are duplicated in Redux if we are in another profile#1574
Sotatek-DukeVu wants to merge 1 commit intodevelopfrom
VT20-2572-group-ipex-refresh-notifications-are-duplicated-in-redux-if-we-are-in-another-profile

Conversation

@Sotatek-DukeVu
Copy link
Collaborator

…tification's wallet

Description

Fix group IPEX refresh notifications are duplicated in Redux if we are in another profile

Checklist before requesting a review

Issue ticket number and link

  • This PR has a valid ticket number or issue: VT20-2572

Testing & Validation

  • This PR has been tested/validated in iOS, Android and browser.
  • Added new unit tests, if relevant.

Design Review

  • In case this PR contains changes to the UI, add some screenshots and/or videos to show the changes on relevant devices.
Screen.Recording.2026-02-04.at.15.05.28.mov

@Sotatek-DukeVu Sotatek-DukeVu self-assigned this Feb 4, 2026
@jimcase jimcase self-requested a review February 5, 2026 11:44
@jimcase
Copy link
Contributor

jimcase commented Feb 5, 2026

Tested on real devices: Android15 and iOS26

).toEqual(undefined);
});

it("should delete notification when it's not current profile's notification", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should delete notification, despite being on a different profile at the time"

Object.values(state.profiles).forEach((profile) => {
profile.notifications = profile.notifications.filter(
(n) => n.id !== action.payload
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this works, it seems a bit wasteful because:
a) it keeps filtering other profiles even after the notification is filtered out.
b) it's still way more likely that the default profile will contain it, given natural user flows.

I propose this:

const defaultProfile = state.profiles[state.defaultProfile];

if (defaultProfile) {
  const idx = defaultProfile.notifications.findIndex(
    (notification) => notification.id !== action.payload
  );

  if (idx !== -1) {
    defaultProfile.notifications.splice(idx, 1);
    return;
  }
}

for (const profile of Object.values(state.profiles)) {
  if (profile === defaultProfile) continue;

  const idx = profile.notifications.findIndex(
    (notification) => notification.id !== action.payload
  );

  if (idx !== -1) {
    profile.notifications.splice(idx, 1);
    break;
  }
}

@iFergal
Copy link
Collaborator

iFergal commented Feb 5, 2026

image On dev, I'm seeing these scroll bars when I scroll but not sure why. Not on a real iOS device. Any ideas?

@Sotatek-DukeVu
Copy link
Collaborator Author

image On dev, I'm seeing these scroll bars when I scroll but not sure why. Not on a real iOS device. Any ideas?

Because the scroll element now has 100% height after Jaime added it, we can no longer rely on ion-content for scrolling and hiding the scrollbar using &::-webkit-scrollbar. I’ve fixed this issue and the fix has been merged into develop. You can rebase this PR onto develop to verify it.

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 this pull request may close these issues.

3 participants