perf(RHIF-167): get rid of activeApps in redux #1754

mkholjuraev · 2023-02-01T18:42:09Z

This is the second step of improving the Inventory details page by getting rid of using the redux store to render active apps in the Main section tab. Now, an active application that needs to be displayed will be decided by the appName in the URL. If it is not set, the first application in appsList prop will get displayed. Also, this PR introduces wrapping general information sections with Card component as a result of this UX ask. This PR is backward compatible. There are a few extra components that needs to be removed once all apps migrate to new approach safely.

Benefits of this refactor:

Better tab layout. No need to render the tab menu separate from the tab content.
Consumer apps can adopt DetailHeader as a presentation component if it has its own data to populate.
AppInfo component is being removed with activeApps in redux which will decrease complexity.
No redux involved in changing, or rendering the tab. The URL parameter appName is used to change tabs, render the default tab
General information cards are separated as a card for better design
Inventory header component and Main section where the tab and apps render are decoupled. Now, both can load independently
InventoryDetail component truly renders the whole Inventory detail page. Before it would render the header, while main section needed to be rendered using AppInfo

Regarding consumer applications dealing with InventoryDetailHead and AppInfo components in the fed-mod, I have been working to remove AppInfo component and passing inventoryId into DetailsWrapper component to directly use DetailHeader as presentational component. This gives those apps more freedom, and reduces the complexity of how we deal with federated modules. In the following commit/PR I will target consuming InventoryDetailHeader and DetailWrapper in fed-modules.

General information tab before:

General information tab after:

mkholjuraev · 2023-02-06T10:38:31Z

Putting this in the draft state to merge Karel's PR and this together as a unified refactoring approach.

codecov-commenter · 2023-02-10T21:27:42Z

Codecov Report

Base: 73.29% // Head: 73.02% // Decreases project coverage by -0.27% ⚠️

Coverage data is based on head (6637e28) compared to base (6343202).
Patch coverage: 32.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1754      +/-   ##
==========================================
- Coverage   73.29%   73.02%   -0.27%     
==========================================
  Files          97       98       +1     
  Lines        2127     2143      +16     
  Branches      823      829       +6     
==========================================
+ Hits         1559     1565       +6     
- Misses        568      578      +10

Impacted Files	Coverage Δ
src/Routes.js	`0.00% <0.00%> (ø)`
...c/components/InventoryDetail/ApplicationDetails.js	`0.00% <0.00%> (ø)`
src/components/InventoryDetail/DetailHeader.js	`0.00% <0.00%> (ø)`
src/components/InventoryDetail/InventoryDetail.js	`0.00% <0.00%> (ø)`
src/modules/DetailHeader.js	`0.00% <0.00%> (ø)`
src/modules/InventoryDetail.js	`0.00% <ø> (ø)`
src/routes/InventoryDetail.js	`0.00% <0.00%> (ø)`
src/store/action-types.js	`100.00% <ø> (ø)`
src/store/entityDetails.js	`3.84% <ø> (+0.27%)`	⬆️
src/store/inventory-actions.js	`79.54% <ø> (-0.46%)`	⬇️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

adonispuente

While testing this PR I was unable to find any change/errors other than the intended changes like layout/performance.
I'd prefer another person take a look as well, but I was unable to find any issues. Great Job!

karelhala

Looking good! Just a few minor questions and adjustments. I'll test it locally later on.

karelhala · 2023-02-14T10:38:59Z

src/components/GeneralInfo/GeneralInformation/GeneralInformation.js

@@ -60,7 +60,7 @@ class GeneralInformation extends Component {
    };

    componentDidMount() {
-        this.props.loadSystemDetail && this.props.loadSystemDetail(this.props.entity.id);
+        this.props.loadSystemDetail && this.props.loadSystemDetail(this.props.inventoryId);


I'd change this so we don't make the inventoryId required yet (edge for instance doesn't pass it yet)

Suggested change

this.props.loadSystemDetail && this.props.loadSystemDetail(this.props.inventoryId);

this.props.loadSystemDetail?.(this.props.inventoryId || this.props.entity.id);

Aha, I will do so

karelhala · 2023-02-14T10:48:43Z

src/components/InventoryDetail/DetailHeader.js

+const DetailHeader = ({
+    entity,
+    hideInvLink,
+    loaded,
+    addNotification,
+    deleteEntity,
+    onBackToListClick,
+    actions,
+    UUIDWrapper,
+    LastSeenWrapper,
+    children,
+    showInventoryDrawer,
+    shouldWrapAsPage,
+    BreadcrumbWrapper,
+    additionalClasses,
+    showDelete,
+    showTags,
+    TitleWrapper,
+    TagsWrapper,
+    DeleteWrapper,
+    ActionsWrapper
+}) => {
+    const HeaderInfo = (<Fragment>
+        <TopBar
+            entity={entity}
+            loaded={loaded}
+            onBackToListClick={onBackToListClick}
+            actions={actions}
+            deleteEntity={deleteEntity}
+            addNotification={addNotification}
+            hideInvLink={hideInvLink}
+            showInventoryDrawer={showInventoryDrawer}
+            showDelete={showDelete}
+            showTags={showTags}
+            TitleWrapper={TitleWrapper}
+            TagsWrapper={TagsWrapper}
+            DeleteWrapper={DeleteWrapper}
+            ActionsWrapper={ActionsWrapper}
+        />
+        <FactsInfo
+            loaded={loaded}
+            entity={entity}
+            UUIDWrapper={UUIDWrapper}
+            LastSeenWrapper={LastSeenWrapper}
+        />
+        {(loaded && verifyCulledInsightsClient(entity?.per_reporter_staleness)) && <InsightsPrompt />}
+        {children}
+    </Fragment>);
+
+    return (shouldWrapAsPage ?
+        (<PageHeader className={classnames('pf-m-light ins-inventory-detail', additionalClasses)} >
+            {BreadcrumbWrapper}
+            {HeaderInfo}
+        </PageHeader>) : HeaderInfo);
+};


It's not a good idea to create new component on each render (especially with so many props). How about we'd split it into two components?

Suggested change

const DetailHeader = ({

entity,

hideInvLink,

loaded,

addNotification,

deleteEntity,

onBackToListClick,

actions,

UUIDWrapper,

LastSeenWrapper,

children,

showInventoryDrawer,

shouldWrapAsPage,

BreadcrumbWrapper,

additionalClasses,

showDelete,

showTags,

TitleWrapper,

TagsWrapper,

DeleteWrapper,

ActionsWrapper

}) => {

const HeaderInfo = (<Fragment>

<TopBar

entity={entity}

loaded={loaded}

onBackToListClick={onBackToListClick}

actions={actions}

deleteEntity={deleteEntity}

addNotification={addNotification}

hideInvLink={hideInvLink}

showInventoryDrawer={showInventoryDrawer}

showDelete={showDelete}

showTags={showTags}

TitleWrapper={TitleWrapper}

TagsWrapper={TagsWrapper}

DeleteWrapper={DeleteWrapper}

ActionsWrapper={ActionsWrapper}

/>

<FactsInfo

loaded={loaded}

entity={entity}

UUIDWrapper={UUIDWrapper}

LastSeenWrapper={LastSeenWrapper}

/>

{(loaded && verifyCulledInsightsClient(entity?.per_reporter_staleness)) && <InsightsPrompt />}

{children}

</Fragment>);

return (shouldWrapAsPage ?

(<PageHeader className={classnames('pf-m-light ins-inventory-detail', additionalClasses)} >

{BreadcrumbWrapper}

{HeaderInfo}

</PageHeader>) : HeaderInfo);

};

const HeaderInfo = ({ entity, loaded, UUIDWrapper, LastSeenWrapper, children, ...props }) => (<Fragment>

<TopBar

entity={entity}

loaded={loaded}

{...props}

/>

<FactsInfo

loaded={loaded}

entity={entity}

UUIDWrapper={UUIDWrapper}

LastSeenWrapper={LastSeenWrapper}

/>

{(loaded && verifyCulledInsightsClient(entity?.per_reporter_staleness)) && <InsightsPrompt />}

{children}

</Fragment>);

const DetailHeader = ({ shouldWrapAsPage, ...props }) => {

return (shouldWrapAsPage ?

(<PageHeader className={classnames('pf-m-light ins-inventory-detail', additionalClasses)} >

{BreadcrumbWrapper}

<HeaderInfo {...props} />

</PageHeader>) : <HeaderInfo {...props} />);

};

HeaderInfo.propTypes = DetailHeader.propTypes;

I am not sure if I didn't missaligned anything, better to try this locally.

Of course. I missed this caveat. I will adopt the suggestion

karelhala · 2023-02-14T10:51:11Z

src/components/InventoryDetail/DetailWrapper.js

+    useEffect(() => {
+        if (!entity || !(entity?.id === inventoryId) || !loaded) {
+            dispatch(loadEntity(inventoryId, { hasItems: true }, { showTags }));
+        }
+    }, []);


What if app doesn't send inventoryId? Wouldn't this cause constant re-renders? Also, if entity is changed via selector no new re-calculation is not triggered

Suggested change

useEffect(() => {

if (!entity || !(entity?.id === inventoryId) || !loaded) {

dispatch(loadEntity(inventoryId, { hasItems: true }, { showTags }));

}

}, []);

useEffect(() => {

if (!entity || !(entity?.id === inventoryId) || !loaded) {

dispatch(loadEntity(inventoryId, { hasItems: true }, { showTags }));

}

}, [entity, inventoryId, loaded]);

I am just thinking about why the entity would change on this page. But, maybe there is something that I am not aware of.

karelhala · 2023-02-14T10:53:02Z

src/routes/InventoryDetail.js

    useEffect(() => {
        chrome?.hideGlobalFilter?.(true);
        chrome.appAction('system-detail');
        clearNotifications();
-        const appName = searchParams.get('appName');
-        if (appName) {
-            dispatch(detailSelect(appName));


I remember removing this action caused incorrect tab activation. I guess the new refs deal with this, right?

Yup, this is resolved by refs

karelhala · 2023-02-14T10:53:40Z

src/routes/InventoryDetail.js

+        history.push({
+            search
+        });
+    }, []);


You should add at least to list of dependencies searchParams.

mkholjuraev · 2023-02-14T13:16:03Z

@karelhala I have adopted all the necessary adjustments. Please have a look one more time

mkholjuraev · 2023-02-20T11:12:01Z

@karelhala Hopefully, everything is as expected now. Here is the edge PR to align the app with this refactoring. Please run these apps to verify. The important thing is that with this PR, inventoryId prop will be used as a main factor to fetch host detail. Thus this prop is marked as required. This enables apps to independently render the header and the main section.

karelhala

Looking good! Tested it locally and everything seems to be working!

## [1.3.1](v1.3.0...v1.3.1) (2023-02-22) ### Performance Improvements * **RHIF-167:** get rid of activeApps in redux ([#1754](#1754)) ([11be618](11be618))

gkarat · 2023-02-22T09:49:35Z

🎉 This PR is included in version 1.3.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

mkholjuraev requested a review from a team as a code owner February 1, 2023 18:42

mkholjuraev requested a review from karelhala February 2, 2023 09:35

mkholjuraev marked this pull request as draft February 6, 2023 10:38

perf(RHIF-167): get rid of activeApps in redux

e1e5046

mkholjuraev force-pushed the refactor-detail branch from b1b7fa7 to e1e5046 Compare February 9, 2023 12:42

mkholjuraev marked this pull request as ready for review February 9, 2023 13:24

gkarat assigned mkholjuraev Feb 10, 2023

chore: decouple the inventory header and main section

c556d0d

adonispuente approved these changes Feb 13, 2023

View reviewed changes

mkholjuraev requested a review from a team February 14, 2023 00:46

karelhala reviewed Feb 14, 2023

View reviewed changes

imrove with review suggestions

6637e28

fix useEffect deps to loadEntity

ce72958

karelhala approved these changes Feb 20, 2023

View reviewed changes

karelhala and others added 2 commits February 20, 2023 20:37

Merge branch 'master' into refactor-detail

324e9d0

Merge branch 'master' into refactor-detail

2d8be06

mkholjuraev merged commit 11be618 into RedHatInsights:master Feb 22, 2023

gkarat pushed a commit that referenced this pull request Feb 22, 2023

chore(release): 1.3.1 [skip ci]

04786b6

## [1.3.1](v1.3.0...v1.3.1) (2023-02-22) ### Performance Improvements * **RHIF-167:** get rid of activeApps in redux ([#1754](#1754)) ([11be618](11be618))

gkarat added the released label Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(RHIF-167): get rid of activeApps in redux #1754

perf(RHIF-167): get rid of activeApps in redux #1754

mkholjuraev commented Feb 1, 2023 •

edited

Loading

mkholjuraev commented Feb 6, 2023

codecov-commenter commented Feb 10, 2023 •

edited

Loading

adonispuente left a comment

karelhala left a comment

karelhala Feb 14, 2023

mkholjuraev Feb 14, 2023

karelhala Feb 14, 2023

mkholjuraev Feb 14, 2023

karelhala Feb 14, 2023

mkholjuraev Feb 14, 2023

karelhala Feb 14, 2023

mkholjuraev Feb 14, 2023

karelhala Feb 14, 2023

mkholjuraev commented Feb 14, 2023

mkholjuraev commented Feb 20, 2023

karelhala left a comment

gkarat commented Feb 22, 2023

	this.props.loadSystemDetail && this.props.loadSystemDetail(this.props.inventoryId);
	this.props.loadSystemDetail?.(this.props.inventoryId \|\| this.props.entity.id);

perf(RHIF-167): get rid of activeApps in redux #1754

perf(RHIF-167): get rid of activeApps in redux #1754

Conversation

mkholjuraev commented Feb 1, 2023 • edited Loading

mkholjuraev commented Feb 6, 2023

codecov-commenter commented Feb 10, 2023 • edited Loading

Codecov Report

adonispuente left a comment

Choose a reason for hiding this comment

karelhala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkholjuraev commented Feb 14, 2023

mkholjuraev commented Feb 20, 2023

karelhala left a comment

Choose a reason for hiding this comment

gkarat commented Feb 22, 2023

mkholjuraev commented Feb 1, 2023 •

edited

Loading

codecov-commenter commented Feb 10, 2023 •

edited

Loading