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

UI Session Improvements #3744

Merged
merged 6 commits into from May 22, 2023
Merged

Conversation

bryanculver
Copy link
Member

@bryanculver bryanculver commented May 12, 2023

Closes: #

What's Changed

  • Couple Login/Logout to RTK Query
  • Proper Session State Invalidation
  • Reduce complexity for logged-in state hooks
  • Reduce complexity for menu data loading hooks (isLoading)

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

nautobot/ui/src/components/Layout.js Outdated Show resolved Hide resolved
nautobot/ui/src/constants/apiPath.js Show resolved Hide resolved
nautobot/ui/src/utils/store.js Outdated Show resolved Hide resolved
nautobot/ui/src/utils/store.js Outdated Show resolved Hide resolved
@glennmatthews glennmatthews added this to the v2.0.0 milestone May 15, 2023
nautobot/dcim/models/racks.py Outdated Show resolved Hide resolved
nautobot/ui/src/components/Layout.js Outdated Show resolved Hide resolved
nautobot/ui/src/components/Layout.js Outdated Show resolved Hide resolved
nautobot/ui/src/router.js Outdated Show resolved Hide resolved
nautobot/ui/src/utils/state.js/auth.js Outdated Show resolved Hide resolved
nautobot/ui/src/utils/state.js/menu.js Outdated Show resolved Hide resolved
nautobot/ui/src/utils/store.js Outdated Show resolved Hide resolved
nautobot/ui/src/utils/store.js Show resolved Hide resolved
nautobot/ui/src/utils/store.js Outdated Show resolved Hide resolved
nautobot/ui/src/views/Login.js Show resolved Hide resolved
@bryanculver bryanculver marked this pull request as ready for review May 19, 2023 20:21
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

To my ignorant self, this looks like a good set of improvements. :shipit:


## Introduction

Nautobot 2.0 introduces a new user interface built on React. This new UI is a complete replacement for the previous Django-based UI, and is the recommended interface for all users. The Django UI will be removed in a future release.
Copy link
Contributor

Choose a reason for hiding this comment

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

is the recommended interface for all users

Is this true for 2.0?


### Hooks

The React UI uses hooks to manage state and side effects. Hooks are functions that let you “hook into” React state and lifecycle features. To learn more about hooks, see the [React Hooks documentation](https://react.dev/reference/react).
Copy link
Contributor

Choose a reason for hiding this comment

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

Hooks are functions that let you “hook into” React state and lifecycle features

A little bit more context on what value this provides would be good. (Maybe "Hooks are events that can be fired when specified state data changes or components are rendered/re-rendered")


These sections are known as "slices" of state, and have their own methods for updating and retrieving state. For more information on Redux, see the [Redux documentation](https://redux.js.org/).

### Hooks
Copy link
Contributor

Choose a reason for hiding this comment

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

Future enhancement idea for some more sections under Hooks:

  • useDispatch
  • useEffect
  • useSelector


In most cases, we build the UI with the preference we only display the final component once the data is available and display a loading indicator until then.

### State Management
Copy link
Contributor

Choose a reason for hiding this comment

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

Future enhancement idea for some more sections under State Management:

  • Reducers

@@ -4,3 +4,6 @@ build
# These files get rendered by `nautobot-server build_ui`
app_imports.js
jsconfig.paths.json

# The output of `npm run analyze`
analyze_report.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there docs on how to generate this report?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is an NPM command but hasn't been exposed as an invoke command yet.

// if (!sessionLoaded || !menuLoaded || sessionInfo === undefined)
// toRender = <LoadingWidget name="application" />;

/** Invalidate the newui cookie and reload the page in order to return to the legacy UI. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this js doc syntax or should it be //?

Copy link
Member Author

Choose a reason for hiding this comment

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

api,
extraOptions
);
const dontRetryHeaders = [400, 401, 403, 404, 405, 406, 407];
Copy link
Contributor

Choose a reason for hiding this comment

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

no 500?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, 500 might be successful with a retry later. The backend could be restarted.

Copy link
Contributor

@gsnider2195 gsnider2195 left a comment

Choose a reason for hiding this comment

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

Added some comments but no blockers. Looks great!

Copy link
Contributor

@jathanism jathanism left a comment

Choose a reason for hiding this comment

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

Just some comments for posterity. Good to see the evolution!

// Configure redux-persist to save state in Local Storage for performance
const persistConfig = {
key: "root",
version: 3, // Increment this number if you change the shape of the state
Copy link
Contributor

Choose a reason for hiding this comment

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

This one deserves a pretty big callout IMO. I can see this biting us. Can we clarify what changing the "shape" of the state means? Perhaps not here, but for our more robust documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, if the state dict/object/shape goes from:

{
    "root": {
        "auth": { 
            // ...
        },
        "navigation": {
            // ...
        }
    }
}

To:

{
    "root": {
        "core": { 
            "user": { 
                // ...
            },
            "menu": {
                // ...
            }
        },
        "apps": {
            // ...
        },
        "auth": true
    }
}

code accessing the state can throw undefineds and other errors, and trying to set previous caches into the new structure can cause issues.

schema: true,
plugin: isPluginView,
},
{ keepUnusedDataFor: 600 } // Let's keep the header schema cached for longer than the default 60 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

This number, even at 10 minutes seems arbitrary. Feels like we might still want to consider some more deterministic way of doing this for posterity?

Comment on lines +108 to +130
<SidebarButton
as={
ReactRouterLink
}
key={
submenu_idx
}
level={
2
}
to={
submenu[1]
}
isLast={
submenu_idx ===
submenu_arr.length -
1
}
>
{
submenu[0]
}
</SidebarButton>
Copy link
Contributor

Choose a reason for hiding this comment

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

MY EYES! THE GOGGLES DO NOTHING!

Copy link
Member Author

Choose a reason for hiding this comment

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

indent: 2

Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
@bryanculver bryanculver merged commit bb3b0ac into next May 22, 2023
21 checks passed
@bryanculver bryanculver deleted the u/bryanculver-session-improvements branch May 22, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants