Skip to content

Commit

Permalink
fix: breaking selector due to missing controller state (#12375)
Browse files Browse the repository at this point in the history
## **Description**

The selector is trying to access some engine state that is removed at
build time through code-fences.
This fix ensures that the selectors are using the engine state or
default to the controllers default state.

Long term - we need to investigate and ensure that the UI is not calling
the selectors.

## **Related issues**

Fixes: #11909

## **Manual testing steps**

1. Can you start up the application without it breaking?

## **Screenshots/Recordings**

Before / After Recording

https://www.loom.com/share/badc4b4cd3b0446486497973dcf337f6?sid=33bcfe23-c495-41bf-89f5-e57b4b72e3d2

I was not able to replicate the controller state being
undefined/missing, so forced it to be undefined when testing.

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
Prithpal-Sooriya authored Nov 25, 2024
1 parent ad8ffb1 commit 16f535c
Show file tree
Hide file tree
Showing 13 changed files with 126 additions and 210 deletions.
5 changes: 5 additions & 0 deletions app/components/UI/Notification/List/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ const mockNavigation = createNavigationProps({});

const mockTrackEvent = jest.fn();

jest.mock('../../../../util/notifications/constants', () => ({
...jest.requireActual('../../../../util/notifications/constants'),
isNotificationsFeatureEnabled: () => true,
}));

jest.mock('../../../../util/notifications/services/NotificationService', () => ({
...jest.requireActual('../../../../util/notifications/services/NotificationService'),
getBadgeCount: jest.fn(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const initialState = {
backgroundState: {
...backgroundState,
AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE,
UserStorageController: {
isProfileSyncingEnabled: false,
},
},
},
security: {
Expand Down Expand Up @@ -69,6 +72,11 @@ jest.mock('../../../../util/navigation/navUtils', () => ({
useParams: jest.fn(() => mockUseParamsValues),
}));

jest.mock('../../../../util/notifications/constants', () => ({
...jest.requireActual('../../../../util/notifications/constants'),
isNotificationsFeatureEnabled: () => false,
}));

describe('SecuritySettings', () => {
beforeEach(() => {
mockUseParamsValues = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3583,188 +3583,8 @@ exports[`SecuritySettings should render correctly 1`] = `
swipeDirection="down"
swipeThreshold={100}
transparent={true}
visible={true}
>
<View
accessibilityState={
{
"busy": undefined,
"checked": undefined,
"disabled": undefined,
"expanded": undefined,
"selected": undefined,
}
}
accessible={true}
collapsable={false}
focusable={true}
onClick={[Function]}
onResponderGrant={[Function]}
onResponderMove={[Function]}
onResponderRelease={[Function]}
onResponderTerminate={[Function]}
onResponderTerminationRequest={[Function]}
onStartShouldSetResponder={[Function]}
style={
{
"backgroundColor": "#00000099",
"bottom": 0,
"height": 1334,
"left": 0,
"opacity": 0,
"position": "absolute",
"right": 0,
"top": 0,
"width": 750,
}
}
/>
<View
collapsable={false}
deviceHeight={null}
deviceWidth={null}
hideModalContentWhileAnimating={false}
onBackdropPress={[Function]}
onModalHide={[Function]}
onModalWillHide={[Function]}
onModalWillShow={[Function]}
onMoveShouldSetResponder={[Function]}
onMoveShouldSetResponderCapture={[Function]}
onResponderEnd={[Function]}
onResponderGrant={[Function]}
onResponderMove={[Function]}
onResponderReject={[Function]}
onResponderRelease={[Function]}
onResponderStart={[Function]}
onResponderTerminate={[Function]}
onResponderTerminationRequest={[Function]}
onStartShouldSetResponder={[Function]}
onStartShouldSetResponderCapture={[Function]}
onSwipeComplete={[Function]}
panResponderThreshold={4}
pointerEvents="box-none"
scrollHorizontal={false}
scrollOffset={0}
scrollOffsetMax={0}
scrollTo={null}
statusBarTranslucent={false}
style={
{
"flex": 1,
"justifyContent": "flex-end",
"left": 0,
"margin": 37.5,
"marginHorizontal": 0,
"top": 0,
"transform": [
{
"translateY": 1334,
},
],
}
}
supportedOrientations={
[
"portrait",
"landscape",
]
}
swipeDirection="down"
swipeThreshold={100}
>
<View
style={
{
"backgroundColor": "#ffffff",
"borderColor": "#bbc0c5",
"borderRadius": 16,
"borderWidth": 2,
"margin": 16,
"minHeight": 120,
"paddingBottom": 24,
}
}
>
<View
style={
{
"alignItems": "center",
"marginVertical": 12,
}
}
>
<View
style={
{
"borderColor": "#f2f4f6",
"borderRadius": 64,
"borderWidth": 3.5,
"height": 36,
"width": 36,
}
}
>
<View
collapsable={false}
style={
{
"height": 40,
"left": -5.5,
"position": "relative",
"top": -5.5,
"transform": [
{
"rotate": "0deg",
},
],
"width": 40,
}
}
>
<Text
allowFontScaling={false}
style={
[
{
"color": "#0376c9",
"fontSize": 36,
},
undefined,
{
"fontFamily": "Material Design Icons",
"fontStyle": "normal",
"fontWeight": "normal",
},
{},
]
}
>
</Text>
</View>
</View>
</View>
<Text
accessibilityRole="text"
style={
{
"alignSelf": "center",
"color": "#141618",
"fontFamily": "EuclidCircularB-Regular",
"fontSize": 16,
"fontWeight": "400",
"letterSpacing": 0,
"lineHeight": 20,
"marginVertical": 12,
"paddingHorizontal": 24,
}
}
>
Disabling profile syncing...
</Text>
</View>
</View>
</Modal>
visible={false}
/>
</View>
</RCTScrollView>
`;
11 changes: 7 additions & 4 deletions app/selectors/notifications/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,21 @@ type AuthenticationState =
type UserStorageState = UserStorageController.UserStorageControllerState;

const selectAuthenticationControllerState = (state: RootState) =>
state?.engine?.backgroundState?.AuthenticationController;
state?.engine?.backgroundState?.AuthenticationController ??
AuthenticationController.defaultState;

const selectUserStorageControllerState = (state: RootState) =>
state?.engine?.backgroundState?.UserStorageController;
state?.engine?.backgroundState?.UserStorageController ??
UserStorageController.defaultState;

const selectNotificationServicesControllerState = (state: RootState) =>
state?.engine?.backgroundState?.NotificationServicesController;
state?.engine?.backgroundState?.NotificationServicesController ??
NotificationServicesController.defaultState;

export const selectIsProfileSyncingEnabled = createSelector(
selectUserStorageControllerState,
(userStorageControllerState: UserStorageState) =>
userStorageControllerState.isProfileSyncingEnabled,
userStorageControllerState?.isProfileSyncingEnabled,
);
export const selectIsProfileSyncingUpdateLoading = createSelector(
selectUserStorageControllerState,
Expand Down
5 changes: 5 additions & 0 deletions app/util/notifications/hooks/useCreateSession.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import * as Selectors from '../../../selectors/notifications';
import * as Actions from '../../../actions/notification/helpers';
import useCreateSession from './useCreateSession';

jest.mock('../constants', () => ({
...jest.requireActual('../constants'),
isNotificationsFeatureEnabled: () => true,
}));

function arrangeStore() {
const store = createMockStore()(initialRootState);

Expand Down
5 changes: 5 additions & 0 deletions app/util/notifications/hooks/useCreateSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
disableProfileSyncing,
signIn,
} from '../../../actions/notification/helpers';
import { isNotificationsFeatureEnabled } from '../constants';

/**
* Custom hook to manage the creation of a session based on the user's authentication status,
Expand All @@ -31,6 +32,10 @@ function useCreateSession(): UseCreateSessionReturn {
const [error, setError] = useState<string | undefined>(undefined);

const createSession = useCallback(async () => {
if (!isNotificationsFeatureEnabled()) {
return;
}

// If the user is already signed in, no need to create a new session
if (isSignedIn) {
return;
Expand Down
5 changes: 5 additions & 0 deletions app/util/notifications/hooks/useNotifications.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ import {
createMockNotificationEthSent,
} from '../../../components/UI/Notification/__mocks__/mock_notifications';

jest.mock('../constants', () => ({
...jest.requireActual('../constants'),
isNotificationsFeatureEnabled: () => true,
}));

function arrangeStore() {
const store = createMockStore()(initialRootState);

Expand Down
40 changes: 36 additions & 4 deletions app/util/notifications/hooks/useNotifications.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import {
} from '../../../actions/notification/helpers';
import { getNotificationsList } from '../../../selectors/notifications';
import { usePushNotifications } from './usePushNotifications';
import { isNotificationsFeatureEnabled } from '../constants';

/**
* Custom hook to fetch and update the list of notifications.
* Manages loading and error states internally.
Expand All @@ -33,6 +35,10 @@ export function useListNotifications(): ListNotificationsReturn {
const [error, setError] = useState<string>();

const listNotifications = useCallback(async () => {
if (!isNotificationsFeatureEnabled()) {
return;
}

setLoading(true);
setError(undefined);
try {
Expand Down Expand Up @@ -68,6 +74,10 @@ export function useCreateNotifications(): CreateNotificationsReturn {
const [error, setError] = useState<string>();

const createNotifications = useCallback(async (accounts: string[]) => {
if (!isNotificationsFeatureEnabled()) {
return;
}

setLoading(true);
setError(undefined);
try {
Expand Down Expand Up @@ -106,12 +116,19 @@ export function useEnableNotifications(): EnableNotificationsReturn {
const [error, setError] = useState<string>();
const { switchPushNotifications } = usePushNotifications();
const enableNotifications = useCallback(async () => {
if (!isNotificationsFeatureEnabled()) {
return;
}

setLoading(true);
setError(undefined);
try {
const errorEnablingNotifications = await enableNotificationServices();
const errorEnablingPushNotifications = await switchPushNotifications(true);
const errorMessage = errorEnablingNotifications || errorEnablingPushNotifications;
const errorEnablingPushNotifications = await switchPushNotifications(
true,
);
const errorMessage =
errorEnablingNotifications || errorEnablingPushNotifications;

if (errorMessage) {
setError(getErrorMessage(errorMessage));
Expand Down Expand Up @@ -143,12 +160,19 @@ export function useDisableNotifications(): DisableNotificationsReturn {
const [error, setError] = useState<string>();
const { switchPushNotifications } = usePushNotifications();
const disableNotifications = useCallback(async () => {
if (!isNotificationsFeatureEnabled()) {
return;
}

setLoading(true);
setError(undefined);
try {
const errorDisablingNotifications = await disableNotificationServices();
const errorDisablingPushNotifications = await switchPushNotifications(false);
const errorMessage = errorDisablingNotifications || errorDisablingPushNotifications;
const errorDisablingPushNotifications = await switchPushNotifications(
false,
);
const errorMessage =
errorDisablingNotifications || errorDisablingPushNotifications;

if (errorMessage) {
setError(getErrorMessage(errorMessage));
Expand Down Expand Up @@ -182,6 +206,10 @@ export function useMarkNotificationAsRead(): MarkNotificationAsReadReturn {

const markNotificationAsRead = useCallback(
async (notifications: MarkAsReadNotificationsParam) => {
if (!isNotificationsFeatureEnabled()) {
return;
}

setLoading(true);
setError(undefined);
try {
Expand Down Expand Up @@ -221,6 +249,10 @@ export function useDeleteNotificationsStorageKey(): deleteNotificationsStorageKe
const [error, setError] = useState<string>();

const deleteNotificationsStorageKey = useCallback(async () => {
if (!isNotificationsFeatureEnabled()) {
return;
}

setLoading(true);
setError(undefined);
try {
Expand Down
Loading

0 comments on commit 16f535c

Please sign in to comment.