[GitHub] [superset] ktmud commented on a change in pull request #16668: fix: reset perf logger timer for soft navigation for SPA pages

[GitBox <gi...@apache.org>]

ktmud commented on a change in pull request #16668:
URL: https://github.com/apache/superset/pull/16668#discussion_r706412999



##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -43,26 +49,39 @@ const bootstrap = JSON.parse(container?.getAttribute('data-bootstrap') ?? '{}');
 const user = { ...bootstrap.user };
 const menu = { ...bootstrap.common.menu_data };
 const common = { ...bootstrap.common };
+let lastLocation: string;
 initFeatureFlags(bootstrap.common.feature_flags);
 
-const RootContextProviders: React.FC = ({ children }) => (
-  <ThemeProvider theme={theme}>
-    <ReduxProvider store={store}>
-      <DndProvider backend={HTML5Backend}>
-        <FlashProvider messages={common.flash_messages}>
-          <DynamicPluginProvider>
-            <QueryParamProvider
-              ReactRouterRoute={Route}
-              stringifyOptions={{ encode: false }}
-            >
-              {children}
-            </QueryParamProvider>
-          </DynamicPluginProvider>
-        </FlashProvider>
-      </DndProvider>
-    </ReduxProvider>
-  </ThemeProvider>
-);
+const RootContextProviders: React.FC = ({ children }) => {
+  const location = useLocation();
+  useEffect(() => {
+    // reset performance logger timer start point to avoid soft navigation
+    // cause dashboard perf measurement problem
+    if (lastLocation && lastLocation !== location.pathname) {

Review comment:
       `lastLocation` is a `Location` object so `lastLocation !== location.pathname` will always be true. Plus comparing with last location is not needed because `useEffect` already only triggers when the dependency `[location.pathname]` updates.

##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -34,6 +39,7 @@ import { theme } from 'src/preamble';
 import ToastPresenter from 'src/messageToasts/containers/ToastPresenter';
 import setupApp from 'src/setup/setupApp';
 import { routes, isFrontendRoute } from 'src/views/routes';
+import { Logger } from '../logger/LogUtils';

Review comment:
       Please use absolute imports.

##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -43,26 +49,39 @@ const bootstrap = JSON.parse(container?.getAttribute('data-bootstrap') ?? '{}');
 const user = { ...bootstrap.user };
 const menu = { ...bootstrap.common.menu_data };
 const common = { ...bootstrap.common };
+let lastLocation: string;
 initFeatureFlags(bootstrap.common.feature_flags);
 
-const RootContextProviders: React.FC = ({ children }) => (
-  <ThemeProvider theme={theme}>
-    <ReduxProvider store={store}>
-      <DndProvider backend={HTML5Backend}>
-        <FlashProvider messages={common.flash_messages}>
-          <DynamicPluginProvider>
-            <QueryParamProvider
-              ReactRouterRoute={Route}
-              stringifyOptions={{ encode: false }}
-            >
-              {children}
-            </QueryParamProvider>
-          </DynamicPluginProvider>
-        </FlashProvider>
-      </DndProvider>
-    </ReduxProvider>
-  </ThemeProvider>
-);
+const RootContextProviders: React.FC = ({ children }) => {

Review comment:
       Instead of putting this in `RootContextProviders`, it might be more appropriate to put the logic into `App` itself----the logger is not exactly a provider.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org