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: [M3-7249] - Linode Landing flickering #9836

Merged
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-9836-fixed-1698158641264.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Fixed
---

Linodes Landing flickering ([#9836](https://github.com/linode/manager/pull/9836))
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { apiMatcher } from 'support/util/intercepts';
Copy link
Contributor

@jdamore-linode jdamore-linode Oct 25, 2023

Choose a reason for hiding this comment

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

Awesome!!!

Edit: Whoops, meant for this to be for the entire file lol


describe('account activation', () => {
/**
* The API will return 403 with the body below for most endpoint except `/v4/profile`.
*
* { "errors": [ { "reason": "Your account must be activated before you can use this endpoint" } ] }
*/
it('should render an activation landing page if the customer is not activated', () => {
cy.intercept('GET', apiMatcher('*'), {
statusCode: 403,
body: {
errors: [
{
reason:
'Your account must be activated before you can use this endpoint',
},
],
},
});

cy.visitWithLogin('/');

cy.findByText('Your account is currently being reviewed.');
cy.findByText('open a support ticket', { exact: false });
});
});
8 changes: 7 additions & 1 deletion packages/manager/src/IdentifyUser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,15 @@ export const IdentifyUser = () => {
// in maintenance mode, we can't fetch the user's username.
// Therefore, the `if` above won't "setFeatureFlagsLoaded".

// We also need to make sure client is defined. Launch Darkly has a weird API.
// If client is undefined, that means flags are loading. Even if flags fail to load,
// client will become defined and we and allow the app to render.

// If we're being honest, featureFlagsLoading shouldn't be tracked by Redux
// and this code should go away eventually.
setFeatureFlagsLoaded();
if (client) {
setFeatureFlagsLoaded();
}
}
}
}, [client, username, account, accountError]);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,7 @@
import { render, waitFor } from '@testing-library/react';
import * as React from 'react';

import { rest, server } from 'src/mocks/testServer';
import { wrapWithTheme } from 'src/utilities/testHelpers';

import MainContent, {
import {
checkFlagsForMainContentBanner,
checkPreferencesForBannerDismissal,
} from './MainContent';
import { queryClientFactory } from './queries/base';

const queryClient = queryClientFactory();

const mainContentBanner = {
key: 'Test Text Key',
Expand Down Expand Up @@ -52,31 +43,3 @@ describe('checkPreferencesForBannerDismissal', () => {
expect(checkPreferencesForBannerDismissal({}, 'key1')).toBe(false);
});
});

describe('Databases menu item for a restricted user', () => {
it('should not render the menu item', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The intentions of this test are not clear. We have tests for this case in the PrimaryNav test and AddNewMenu so I'm not sure what this one was intended to check. It was flakey because it tries to render the entire app

server.use(
rest.get('*/account', (req, res, ctx) => {
return res(ctx.json({}));
})
);
const { getByText } = render(
wrapWithTheme(
<MainContent appIsLoading={false} isLoggedInAsCustomer={true} />,
{
flags: { databases: false },
queryClient,
}
)
);

await waitFor(() => {
let el;
try {
el = getByText('Databases');
} catch (e) {
expect(el).not.toBeDefined();
}
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,14 @@ describe('AuthenticationWrapper', () => {
});
it('should render one child when showChildren state is true', () => {
component.setState({ showChildren: true });
expect(component.childAt(0)).toHaveLength(1);
expect(component.childAt(0)).toBeDefined();
});
it('should invoke props.initSession when the component is mounted', () => {
expect(component.instance().props.initSession).toHaveBeenCalledTimes(1);
});

it('should set showChildren state to true when the isAuthenticated prop goes from false to true', () => {
it('should not showChildren initially because they should be shown after makeInitialRequests', () => {
component.setState({ showChildren: false });
component.setProps({ isAuthenticated: true });
expect(component.state('showChildren')).toBeTruthy();
expect(component.state('showChildren')).toBeFalsy();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { handleInitTokens } from 'src/store/authentication/authentication.action
import { handleLoadingDone } from 'src/store/initialLoad/initialLoad.actions';
import { State as PendingUploadState } from 'src/store/pendingUpload';
import { MapState } from 'src/store/types';
import SplashScreen from '../SplashScreen';

interface Props {
children: React.ReactNode;
Expand All @@ -34,11 +35,6 @@ type CombinedProps = Props &
WithApplicationStoreProps;

export class AuthenticationWrapper extends React.Component<CombinedProps> {
state = {
hasEnsuredAllTypes: false,
showChildren: false,
};

componentDidMount() {
const { initSession } = this.props;
/**
Expand All @@ -54,8 +50,6 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
* to show the children onMount
*/
if (this.props.isAuthenticated) {
this.setState({ showChildren: true });

this.makeInitialRequests();
}
}
Expand All @@ -72,8 +66,6 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
!this.state.showChildren
) {
this.makeInitialRequests();

return this.setState({ showChildren: true });
}

/** basically handles for the case where our token is expired or we got a 401 error */
Expand All @@ -90,8 +82,12 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
render() {
const { children } = this.props;
const { showChildren } = this.state;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on renaming showChildren? Isn't this pretty much "renderTree"? wondering what could make a tad more clear the fact we're really loading the application.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was considering something more like isLoading but I decided to just leave as is for now. I definitely plan to follow this PR up with some others that improve our general render tree/ entry points. It's an absolute mess.

// eslint-disable-next-line
return <React.Fragment>{showChildren ? children : null}</React.Fragment>;

if (showChildren) {
return children;
}

return <SplashScreen />;
}

static defaultProps = {
Expand All @@ -109,6 +105,7 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
makeInitialRequests = async () => {
// When loading Lish we avoid all this extra data loading
if (window.location?.pathname?.match(/linodes\/[0-9]+\/lish/)) {
this.setState({ showChildren: true });
return;
}

Expand Down Expand Up @@ -142,8 +139,13 @@ export class AuthenticationWrapper extends React.Component<CombinedProps> {
/** We choose to do nothing, relying on the Redux error state. */
} finally {
this.props.markAppAsDoneLoading();
this.setState({ showChildren: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes e2e failure @jdamore-linode mentioned.

PreferenceToggle use to render-block all children until preferences were loaded. Now we are render-blocking at this level, which makes more sense anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

/** We choose to do nothing, relying on the Redux error state. */

It's my first time visiting this code in a long time. I understand we to load content and render tree in the case of a fetching error, but what does the redux error do exactly? The user does not get any feedback something fails (ex: try blocking any API request). It's probably beyond the scope of this PR but wondering what your thoughts are about it.

}
};

state = {
showChildren: false,
};
}

interface StateProps {
Expand Down
Loading