Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/PluginEngine.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,16 @@ export default function PluginEngine({
children: React.ReactNode;
}) {
// Fetch enabled plugins from the backend API
const { data: enabledPlugins } = useQuery({
queryKey: ["enabled-plugins"],
queryFn: query(plugConfigApi.list, {
silent: (response) => response.status === 401 || response.status === 403,
}),
retry: false,
});
const { data: enabledPlugins, isLoading: isEnabledPluginsLoading } = useQuery(
{
queryKey: ["enabled-plugins"],
queryFn: query(plugConfigApi.list, {
silent: (response) =>
response.status === 401 || response.status === 403,
}),
retry: false,
},
);

const resolvedPlugins = useMemo(
() => mergePlugConfigs(enabledPlugins?.configs ?? []),
Expand Down Expand Up @@ -119,6 +122,10 @@ export default function PluginEngine({
window.__CARE_PLUGIN_RUNTIME__ = deepFreeze({ meta: pluginMeta });
}, [pluginMeta]);

// Expose plugin loading state to show loaders while a plugin page is being accessed
const arePluginsLoading =
isEnabledPluginsLoading || pluginsQuery.some((plugin) => plugin.isLoading);

// Register plugin overrides
const overrideCleanupRef = useRef<(() => void)[]>([]);

Expand Down Expand Up @@ -159,7 +166,9 @@ export default function PluginEngine({
</div>
}
>
<CareAppsContext.Provider value={pluginsQuery}>
<CareAppsContext.Provider
value={{ apps: pluginsQuery, isLoading: arePluginsLoading }}
>
Comment on lines +169 to +171

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Memoize the context value to prevent unnecessary re-renders.

The inline object { apps: pluginsQuery, isLoading: arePluginsLoading } creates a new reference on every render, which can trigger re-renders in all context consumers even when the actual data hasn't changed.

Suggested fix
+  const contextValue = useMemo(
+    () => ({ apps: pluginsQuery, isLoading: arePluginsLoading }),
+    [pluginsQuery, arePluginsLoading],
+  );
+
   return (
     <Suspense fallback={<Loading />}>
       <ErrorBoundary
         fallback={
           <div className="flex h-screen w-screen items-center justify-center">
             Care has encountered an unexpected error.
           </div>
         }
       >
-        <CareAppsContext.Provider
-          value={{ apps: pluginsQuery, isLoading: arePluginsLoading }}
-        >
+        <CareAppsContext.Provider value={contextValue}>
           <Suspense fallback={<Loading />}>{children}</Suspense>
         </CareAppsContext.Provider>
       </ErrorBoundary>
     </Suspense>
   );

Note: useMemo is already imported on line 13.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/PluginEngine.tsx` around lines 169 - 171, The CareAppsContext.Provider
value prop contains an inline object that creates a new reference on every
render, causing unnecessary re-renders in context consumers. Wrap the context
value object with the useMemo hook (already imported on line 13) and specify
pluginsQuery and arePluginsLoading as dependencies, so the object reference only
changes when these dependencies actually change.

<Suspense fallback={<Loading />}>{children}</Suspense>
</CareAppsContext.Provider>
</ErrorBoundary>
Expand Down
14 changes: 12 additions & 2 deletions src/Routers/AppRouter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,17 @@ import { SidebarProvider, SidebarTrigger } from "@/components/ui/sidebar";
import { AppSidebar, SidebarFor } from "@/components/ui/sidebar/app-sidebar";

import ErrorBoundary from "@/components/Common/ErrorBoundary";
import Loading from "@/components/Common/Loading";
import BrowserWarning from "@/components/ErrorPages/BrowserWarning";
import ErrorPage from "@/components/ErrorPages/DefaultErrorPage";
import SessionExpired from "@/components/ErrorPages/SessionExpired";

import useAuthUser from "@/hooks/useAuthUser";
import { useOrganizationRoutes, usePluginRoutes } from "@/hooks/useCareApps";
import {
useCareAppsLoading,
useOrganizationRoutes,
usePluginRoutes,
} from "@/hooks/useCareApps";
import useSidebarState from "@/hooks/useSidebarState";

import { routes as publicRoutes } from "@/Routers/PublicRouter";
Expand Down Expand Up @@ -99,6 +104,7 @@ const publicRedirects = Object.fromEntries(
export default function AppRouter() {
const pluginRoutes = usePluginRoutes();
const organizationRoutes = useOrganizationRoutes();
const arePluginsLoading = useCareAppsLoading();
let routes = Routes;

useRedirect("/user", "/users");
Expand All @@ -119,7 +125,11 @@ export default function AppRouter() {

const sidebarFor = isAdminPage ? SidebarFor.ADMIN : SidebarFor.FACILITY;

const pages = appPages || adminPages || publicRedirectsPages || <ErrorPage />;
const pages =
appPages ||
adminPages ||
publicRedirectsPages ||
(arePluginsLoading ? <Loading /> : <ErrorPage />);

const user = useAuthUser();

Expand Down
12 changes: 10 additions & 2 deletions src/hooks/useCareApps.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ export type CareAppsContextType = Array<
(({ isLoading: false } & PluginManifestWithMeta) | { isLoading: true })
>;

export const CareAppsContext = createContext<CareAppsContextType | null>(null);
export type CareAppsContextValue = {
apps: CareAppsContextType;
isLoading: boolean;
};
Comment on lines +16 to +19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Use interface instead of type for object type definitions.

Per coding guidelines, prefer interface for defining object shapes in TypeScript.

Suggested fix
-export type CareAppsContextValue = {
-  apps: CareAppsContextType;
-  isLoading: boolean;
-};
+export interface CareAppsContextValue {
+  apps: CareAppsContextType;
+  isLoading: boolean;
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export type CareAppsContextValue = {
apps: CareAppsContextType;
isLoading: boolean;
};
export interface CareAppsContextValue {
apps: CareAppsContextType;
isLoading: boolean;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/hooks/useCareApps.tsx` around lines 16 - 19, The CareAppsContextValue
type definition currently uses the type keyword for defining an object shape,
but the coding guidelines prefer using interface for object type definitions.
Convert the type CareAppsContextValue declaration from a type alias to an
interface, changing from type keyword to interface keyword and adjusting the
syntax accordingly (replacing the equals sign with the interface body syntax).

Source: Coding guidelines


export const CareAppsContext = createContext<CareAppsContextValue | null>(null);

export const useCareApps = () => {
const ctx = useContext(CareAppsContext);
Expand All @@ -22,9 +27,12 @@ export const useCareApps = () => {
"'useCareApps' must be used within 'CareAppsProvider' only",
);
}
return ctx;
return ctx.apps;
};

export const useCareAppsLoading = () =>
useContext(CareAppsContext)?.isLoading ?? false;

// export const useCareAppNavItems = () => {
// const careApps = useCareApps();
// const navItems = careApps.reduce<INavItem[]>((acc, plugin) => {
Expand Down
Loading