Support MDS on List, Detail, Dashboard, Overview pages

[jackiehanyang]

Revisions:

4/17:

• The latest revision removes dataSourceEnabled check before dispatch calls
• update router path to put all dataSourceId at the end of the path
• handle NEP situation for dataSource plugin and dataSourceManagement plugin getter and setter
• run prettier
• make server client type safe

---

4/16:
The latest revision modifies the way of reading dataSourceId between pages and components. Now it's getting the dataSourceId from the url instead of receiving it as a prop. Also removed dataSourceId from Detector and DetectorListItem interface to avoid setting dataSourceId in loop. Instead, we just append the dataSourceId that was read from the url to the herf link

Description:

This PR includes:

• adds MDS support on AD Overview page, Dashboard page, List Detectors Page, and Detector Detail page.
• update all route path to make it accept dataSourceId when it has truthy value
• update side nav bar to make the redirection includes dataSourceId when it has truthy value
• some bug fixes

Notes:

• Previous prs for List and Detector Detail page: Support MDS on ListDetectors page #716, Support MDS on DetectorDetails page #719
  Addressed all comments in the previous prs but the update routes to allow empty dataSourceId one, included this fix in this pr
• meta issue - [META] MDS Support For AD #721

Screen.Recording.2024-04-12.at.10.19.27.mov

Issues Resolved

[List any issues this PR will resolve]

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[ohltyler]

Please see comments. I see an opportunity to reduce a lot of the props propagation happening with persisting things in redux store.

[ohltyler]

Why can't we store the selected data source ID as a redux store item, and only update when a new ID is selected? Then you don't need to pass around ad-hoc dataSourceIds. If we use redux middleware for all of the calls made from client-side, the persisted data source ID within that store can handle all of that and no propagating needs to happen at all from client-side.

Thinking long-term with supporting multiple data sources, I could see some places where more fine-grained IDs need to be attached to the data models, so it makes sense to add as an optional field for Detector interface. But even then, this could all be persisted in the redux store and those details don't need to be handled from client-side components at all.

[jackiehanyang]

Thanks for the suggestion to utilize the redux store for managing the dataSourceId. I understand the benefits of centralizing the state in Redux, especially for consistency and potentially reducing the complexity of propagating the IDs through components.

However, I have some concerns about relying entirely on the Redux store for this purpose. In the future when our application architecture supports multiple active data sources in different components simultaneously, which might lead to complications if we centralize the dataSourceId state. This approach could limit the flexibility needed for some components to operate independently under different contexts, which we anticipate will be a requirement as we scale and introduce more complex features.

Additionally, managing fine-grained control over data sources at the component level allows us to more easily integrate with other parts of our infrastructure that are not as tightly coupled with Redux. It also provides clearer traceability for data flow in complex workflows, which can aid in debugging and maintenance. Our application already have an overloaded redux state and it even crashed the Redux Debug DevTool because the state is too long to parse on some big pages like detector details page.

[ohltyler]

This approach could limit the flexibility needed for some components to operate independently under different contexts

Could you explain how this limits it? I'm not saying we never should have component-level context for different subsets of data source IDs, for example, but rather, by persisting the selected data source ID(s) from the top-down menu in some centralized store, we immediately aren't bound/forced to propagate those top-level IDs as props through many components that may not need it at all, but have to pass through to its children. There's nothing preventing having components have local state and local context to handle their own data source IDs for whatever purpose they want.

From my understanding, the current and near-future approach is to start by selecting a single or multiple data source IDs from a top-level picker. My proposal is that by persisting this general top-level data in redux store, we eliminate all pass-through props and data model changes needed to persist those ID(s) on their own.

Especially for the initial release which is bound to at most one data source ID, this completely eliminates the need to pass any props at all. As things could become more complex, we can introduce more IDs & props where appropriate if local, component-level context is needed, for example.

Also, suppose tomorrow we need the ID to be available in some small component embedded in the create detector page, and suppose it's embedded under 5 parent components. With the current approach, we would now need to backfill and add props to all of those parents to propagate the ID to the child. If we have this ID in redux store that is set at the top-level page when rendering, all the component has to do is pull the stored ID.

[ohltyler]

Lmk your thoughts on this too, if you prefer some way based on the current and future plans of MDS and state management @huyaboo @BionIT

[jackiehanyang]

let's allow the Neo team to decide if centralized state management suits the neo goal and could benefit the future Neo roadmap. This should be an implementation requirement for all plugins, if necessary.

Regardless, I considered storing this ID in a Redux store and discussed this solution with Lu initially. The reason I didn't proceed is based on my current experience with adding MDS support for AD, and these concers:

• Increased complexity: Implementing Redux adds layers of complexity to the application. And redux often requires writing more boilerplate code, for example I have to setup actions, reducers, selectors, and middleware to manage just a simple ID.
• Performance overhead: Redux connects components to the global store, which can lead to unnecessary re-renders if not carefully optimized. This is already happening to AD, almost all pages gets re-rendered multiple times when opening it.
• Debugging difficulty: Debugging Redux can be more challenging than simple prop passing. To understand how and why the dataSourceId is changing, we have to rely on Redux DevTools, as opposed to simply looking at component props. However, as I mentioned earlier regarding the performance overhead, Redux DevTools crashes on some AD pages because the state is too long to serialize. This leaves us with no mechanism to effectively debug the issue.

Therefore, the reason I choose to make dataSourceId a prop include simplicity, less overhead, easier debugging and testing, reduced coupling. I do agree that when it's used by many components at different nesting levels, a Redux store is definitely the better choice. However, I believe that only the detector detail page has a really nested structure. Additionally, the dataSource picker is disabled on the detector detail page, so there's no need to pass dataSourceId around there. For the create detector page, we only allow customers choose it in the first step. It becomes a read-only value in subsequent steps, so there are no nesting concerns on the create page.

[huyaboo]

My $0.02:

From data source side, there are certain reasons to have state management:

1. We recommend the use of states to reduce network calls for certain components. For instance, if a DataSourceView component is repeatedly being used for different pages, it is recommended to keep both the id and label values and pass them into the component. This saves the component a network call and potentially a re-render of the page. Now this would require setting the state of both an id and a label and this might not be relevant to AD since the DataSourceView is used only in the detector detail page.
2. At the plugin-level, how deeply/how many times is dataSourceId being passed in? If it's being referenced frequently, having to check for dataSourceId being passed into every component is more tedious than simply fetching from the redux store.
3. As for a state management requirement, this should be case-by-case. Several plugins only have one/a few pages to pass around the dataSourceId. It would be unreasonable to require state management for a prop that's being passed around a handful of times.

[ohltyler]

Regardless of which one we encounter, reading dataSourceId from the Redux store is completely unnecessary and over-engineered

Please avoid saying this; using redux to maintain common/global/shared state is a popular, practical, and clean solution. I still don't understand the overengineering concern; it would be adding one reducer file and a one-line change to onboard it to the global app state, since all of the infrastructure is already there. I'm not sure what "interacts with 5+ reducers" means; subscribing to the datasource ID only re-renders that component when the value of that ID has changed; that's the beauty of having granular reducers to only update on changes that the particular component is interested in tracking.

I agree with the 2 approaches for how to fetch the datasource ID. The whole point of having a redux store is it can be set once (either from url or from the data source picker), and then is available for any component to use, with no extra state or variables needing to be managed or passed around, at all. It has nothing to do with source of truth. I don't understand the memory concern either; needing to pass the state around component-to-component is more intensive. And again, we need to keep in mind further MDS enhancements of persisting multiple selected data sources, for example, which works much more seamlessly when this is maintained in one spot. The change would only need to be made once.

I've tried to provide a large number of positive reasons on how this approach can simplify things a lot and help keep the code scalable for the future, and make life easier for implementing as well as it reduces the number of changes needed in even just this PR. Please do add more details on if any of those details don't align or make sense to you; it would be helpful to understand what exactly is the pushback here or what points I've provided aren't clear.

[ohltyler]

Something that would be really cool is having some way to persist the IDs and make them accessible for all of the redux fns that make server-side calls that end up making cluster calls. Then all of the redux calls made on client-side wouldn't even need to pass them through; the middleware could pick up the persisted ID and pass it directly. Like a parent/hierarchical structure of the reducers.

[jackiehanyang]

Please avoid saying this; using redux to maintain common/global/shared state is a popular, practical, and clean solution.

I agree redux is a good library, I'm not saying it's useless :) Just the reason I think it's unnecessary is because that the dataSourceId in the URL is our primary source of truth for identifying a current resource or view, using it directly can simplify our state management.

If we are retrieving the dataSourceId from the URL on every different page, why not just use it directly instead of following a cycle of use -> update redux store -> read from redux store -> use? For every child component on each page, we could either read the dataSourceId from the Redux store that was maintained on the previous parent page, or simply read the dataSourceId directly from the url. If MDS adds more attributes in the future, these will also be in the URL. Therefore, I believe that no approach is inherently better than another; they are just different ways of achieving the same outcome.

The reason I didn't choose the Redux store approach when adding MDS support on the Overview page is due to an existing bug that caused an infinite loop in the global Redux store state changes. This took up more of my time than usual for debugging because you need to understand how every global state is changing behind the scenes.

I can make the change if this is really a blocker. Guess it's time to disagree and commit :)

[ohltyler]

Maybe there's still confusion. I'm not saying we don't use it directly - I'm saying that by persisting it, we don't need to have any props passed down to any component. See previous comment:

The whole point of having a redux store is it can be set once (either from url or from the data source picker), and then is available for any component to use, with no extra state or variables needing to be managed or passed around, at all

The idea is it gets set only the few times at the top-level of pages where its parsing from the URL or from the data source picker. Then any component throughout the entire client-side of the plugin can use that ID since it's now shared everywhere - no props needed or needing to find workarounds to propagate the ID through child components. The best benefit is it reduces all of that code and makes it much more flexible, should we need to persist more or different data related to MDS in the future without needing to update interfaces or prop data types.

[ohltyler]

Great to see this coming together. Have some comments, but nothing too large. Also, can we ensure the baseline/existing UT and IT are passing?

[ohltyler]

Thanks for all of the hard work. LGTM for a feature branch merge!