prowler icon indicating copy to clipboard operation
prowler copied to clipboard

fix(ui): Remove Content Layout and move userinfo to global state

Open chaithanya009 opened this issue 7 months ago • 10 comments

Context

Currently, the use(getUserInfo()) call is placed outside the <Suspense> boundary in the ContentLayout component. This causes rendering to be blocked until data is resolved, and the Suspense fallback (<SkeletonContentLayout />) is never shown. Objective: Centralize page layout management by moving title/icon definitions and navbar functionality from individual pages into the main layout component.

What Changed: ✅ Centralized Metadata: Created PAGE_METADATA array in main-layout.tsx mapping all routes to titles/icons ✅ Unified Layout: Moved navbar rendering into main layout using usePathname() for dynamic content ✅ Simplified Pages: Removed ContentLayout wrapper from 13+ pages - they now render content directly ✅ Cleaner Architecture: Single source of truth for page metadata, reduced code duplication

Description

DRY Principle: No more repeating title/icon on every page Easier Maintenance: Adding new pages only requires updating the metadata array Better Performance: Single layout renders navbar once vs. per-page rendering Cleaner Code: Pages focus purely on content without layout concerns

Fix: Result: All pages now automatically get proper titles, icons, and navbar through the centralized main layout system.

Checklist

  • Are there new checks included in this PR? Yes / No
    • If so, do we need to update permissions for the provider? Please review this carefully.
  • [ ] Review if the code is being covered by tests.
  • [ ] Review if code is being documented following this specification https://github.com/google/styleguide/blob/gh-pages/pyguide.md#38-comments-and-docstrings
  • [ ] Review if backport is needed.
  • [ ] Review if is needed to change the Readme.md
  • [ ] Ensure new entries are added to CHANGELOG.md, if applicable.

API

  • [ ] Verify if API specs need to be regenerated.
  • [ ] Check if version updates are required (e.g., specs, Poetry, etc.).
  • [ ] Ensure new entries are added to CHANGELOG.md, if applicable.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

chaithanya009 avatar May 27 '25 15:05 chaithanya009

Thanks for the catch, I think it's better to get the user info in user-nav.tsx rather than in navbar.tsx to avoid the prop drilling because navbar.tsx does not use that data.

For some reason The suggested change is not working . I tried moving the Suspense directly on to navbar.tsx but it is showing the loading screen endlessly.

My guess is probably because the user-nav.tsx is a client component and the Suspense is in server component (navbar.tsx or content-layout.tsx).

So, I am reverting this change

chaithanya009 avatar May 28 '25 15:05 chaithanya009

Thanks for the catch, I think it's better to get the user info in user-nav.tsx rather than in navbar.tsx to avoid the prop drilling because navbar.tsx does not use that data.

For some reason The suggested change is not working . I tried moving the Suspense directly on to navbar.tsx but it is showing the loading screen endlessly.

My guess is probably because the user-nav.tsx is a client component and the Suspense is in server component (navbar.tsx or content-layout.tsx).

So, I am reverting this change

Server actions can be called from client components like:

export const UserNav = () => {
  const [user, setUser] = useState<UserProfileProps | null>(null);
  useEffect(() => {
    const fetchUser = async () => {
      const user = await getUserInfo();
      setUser(user);
    };
    fetchUser();
  }, []);

  if (!user || !user.data) return null;

We have to choose whether to pass it from the father or call it in the client component.

https://nextjs.org/docs/app/building-your-application/data-fetching/server-actions-and-mutations#behavior

alejandrobailo avatar May 28 '25 15:05 alejandrobailo

Thanks for the catch, I think it's better to get the user info in user-nav.tsx rather than in navbar.tsx to avoid the prop drilling because navbar.tsx does not use that data.

For some reason The suggested change is not working . I tried moving the Suspense directly on to navbar.tsx but it is showing the loading screen endlessly. My guess is probably because the user-nav.tsx is a client component and the Suspense is in server component (navbar.tsx or content-layout.tsx). So, I am reverting this change

Server actions can be called from client components like:

export const UserNav = () => {
  const [user, setUser] = useState<UserProfileProps | null>(null);
  useEffect(() => {
    const fetchUser = async () => {
      const user = await getUserInfo();
      setUser(user);
    };
    fetchUser();
  }, []);

  if (!user || !user.data) return null;

We have to choose whether to pass it from the father or call it in the client component.

https://nextjs.org/docs/app/building-your-application/data-fetching/server-actions-and-mutations#behavior

Thanks for the reference. I will look into it

chaithanya009 avatar May 28 '25 16:05 chaithanya009

The current approach is better than calling it in the user-nav for mainly two reasons:

  1. useEffects are not compatible with Suspense since it happens after page mount, so we would have to handle the loading state manually in the user-nav (defeating the purpose of Suspense)
  2. Data fetching would happen on the client, losing server-side rendering advantages."

chaithanya009 avatar May 29 '25 06:05 chaithanya009

The current approach is better than calling it in the user-nav for mainly two reasons:

  1. useEffects are not compatible with Suspense since it happens after page mount, so we would have to handle the loading state manually in the user-nav (defeating the purpose of Suspense)
  2. Data fetching would happen on the client, losing server-side rendering advantages."
  1. True, Suspense is easier to develop and understand.
  2. False, data fetching will happen on the server because getUserInfo is a server action.

The point is that there are a lot of options in NextJS apps. For me, what makes more sense is to get the user on top of the app and save it in a shared state (Zustand, for example) and then consume it where is needed.

ATM, we can get it on the ContentLayout and remove the suspense, because we need the user to render the content; otherwise, what content should we render if there is no user? Another option is what you propose, get it on the navbar, this is not a good implementation because a NavBar should just render a UI component, not get data.

It is important to be clear in separating concerns and responsibilities. navbar shouldn't get user info, should be agnostic to the user.

Then the question is, given all the options we have and the time we have to improve this, what is the better option?

Option 1: Global State (Zustand) - BEST Long-term

// store/user-store.ts
import { create } from 'zustand'

interface UserStore {
  user: UserProfileProps | null
  setUser: (user: UserProfileProps) => void
}

export const useUserStore = create<UserStore>((set) => ({
  user: null,
  setUser: (user) => set({ user }),
}))

// app/(prowler)/layout.tsx
export default async function Layout({ children }) {
  const user = await getUserInfo()
  
  return (
    <UserProvider user={user}>
      {children}
    </UserProvider>
  )
}

// components/ui/user-nav/user-nav.tsx
export const UserNav = () => {
  const user = useUserStore(state => state.user)
  // ...
}

✅ Pros:

  • Perfect separation of concerns
  • Single call per session
  • Scalable for entire app
  • Navbar completely agnostic

❌ Cons:

  • Requires more initial setup
  • Needs provider in layout
  • More architectural complexity

Option 2: ContentLayout async (no Suspense) - BEST for Limited Time

// content-layout.tsx
export async function ContentLayout({ title, icon, children }) {
  const user = await getUserInfo() // Blocks until user is available
  
  return (
    <>
      <Navbar title={title} icon={icon} user={user} />
      <div className="px-6 py-4 sm:px-8 xl:px-10">{children}</div>
    </>
  )
}

✅ Pros:

  • Minimal change (just add async)
  • Maintains separation of concerns
  • Guarantees user is always available
  • No regressions

❌ Cons:

  • Blocks entire page if getUserInfo is slow

Option 3: Navbar async - Works but Violates Principles

// ❌ Violates Single Responsibility
export async function Navbar({ title, icon }) {
  const user = await getUserInfo() // Navbar fetching data
  return <header>...</header>
}

❌ Issues:

  • Navbar should be presentational only
  • Violates separation of concerns
  • Makes component less reusable

Recommendation

For IMMEDIATE Implementation: Option 2 (ContentLayout async)

// components/ui/content-layout/content-layout.tsx
import { getUserInfo } from "@/actions/users/users";
import { Navbar } from "../nav-bar/navbar";

interface ContentLayoutProps {
  title: string;
  icon: string;
  children: React.ReactNode;
}

export async function ContentLayout({ title, icon, children }: ContentLayoutProps) {
  const user = await getUserInfo()
  
  return (
    <>
      <Navbar title={title} icon={icon} user={user} />
      <div className="px-6 py-4 sm:px-8 xl:px-10">{children}</div>
    </>
  )
}

Reasons:

  1. Minimal time: Just add async and await
  2. Maintains principles: Navbar remains presentational
  3. Logical: Need user to determine what content to render
  4. No breaking changes: Doesn't affect existing code

For FUTURE Improvement: Option 1 (Global State)

What do you think @paabloLC?

alejandrobailo avatar May 29 '25 07:05 alejandrobailo

The current approach is better than calling it in the user-nav for mainly two reasons:

  1. useEffects are not compatible with Suspense since it happens after page mount, so we would have to handle the loading state manually in the user-nav (defeating the purpose of Suspense)
  2. Data fetching would happen on the client, losing server-side rendering advantages."
  1. True, Suspense is easier to develop and understand.
  2. False, data fetching will happen on the server because getUserInfo is a server action.

The point is that there are a lot of options in NextJS apps. For me, what makes more sense is to get the user on top of the app and save it in a shared state (Zustand, for example) and then consume it where is needed.

ATM, we can get it on the ContentLayout and remove the suspense, because we need the user to render the content; otherwise, what content should we render if there is no user? Another option is what you propose, get it on the navbar, this is not a good implementation because a NavBar should just render a UI component, not get data.

It is important to be clear in separating concerns and responsibilities. navbar shouldn't get user info, should be agnostic to the user.

Then the question is, given all the options we have and the time we have to improve this, what is the better option?

Option 1: Global State (Zustand) - BEST Long-term

// store/user-store.ts
import { create } from 'zustand'

interface UserStore {
  user: UserProfileProps | null
  setUser: (user: UserProfileProps) => void
}

export const useUserStore = create<UserStore>((set) => ({
  user: null,
  setUser: (user) => set({ user }),
}))

// app/(prowler)/layout.tsx
export default async function Layout({ children }) {
  const user = await getUserInfo()
  
  return (
    <UserProvider user={user}>
      {children}
    </UserProvider>
  )
}

// components/ui/user-nav/user-nav.tsx
export const UserNav = () => {
  const user = useUserStore(state => state.user)
  // ...
}

✅ Pros:

  • Perfect separation of concerns
  • Single call per session
  • Scalable for entire app
  • Navbar completely agnostic

❌ Cons:

  • Requires more initial setup
  • Needs provider in layout
  • More architectural complexity

Option 2: ContentLayout async (no Suspense) - BEST for Limited Time

// content-layout.tsx
export async function ContentLayout({ title, icon, children }) {
  const user = await getUserInfo() // Blocks until user is available
  
  return (
    <>
      <Navbar title={title} icon={icon} user={user} />
      <div className="px-6 py-4 sm:px-8 xl:px-10">{children}</div>
    </>
  )
}

✅ Pros:

  • Minimal change (just add async)
  • Maintains separation of concerns
  • Guarantees user is always available
  • No regressions

❌ Cons:

  • Blocks entire page if getUserInfo is slow

Option 3: Navbar async - Works but Violates Principles

// ❌ Violates Single Responsibility
export async function Navbar({ title, icon }) {
  const user = await getUserInfo() // Navbar fetching data
  return <header>...</header>
}

❌ Issues:

  • Navbar should be presentational only
  • Violates separation of concerns
  • Makes component less reusable

Recommendation

For IMMEDIATE Implementation: Option 2 (ContentLayout async)

// components/ui/content-layout/content-layout.tsx
import { getUserInfo } from "@/actions/users/users";
import { Navbar } from "../nav-bar/navbar";

interface ContentLayoutProps {
  title: string;
  icon: string;
  children: React.ReactNode;
}

export async function ContentLayout({ title, icon, children }: ContentLayoutProps) {
  const user = await getUserInfo()
  
  return (
    <>
      <Navbar title={title} icon={icon} user={user} />
      <div className="px-6 py-4 sm:px-8 xl:px-10">{children}</div>
    </>
  )
}

Reasons:

  1. Minimal time: Just add async and await
  2. Maintains principles: Navbar remains presentational
  3. Logical: Need user to determine what content to render
  4. No breaking changes: Doesn't affect existing code

For FUTURE Improvement: Option 1 (Global State)

What do you think @paabloLC?

About the 2nd point what i mean is the api call is initiated after the page is mounted unlike the original approach, data fetching does happen on server in both cases. And I don't think there is anything wrong with api call in the navbar as there is no degradation in performance at all and we won't be reusing the navbar anyway

chaithanya009 avatar May 29 '25 15:05 chaithanya009

And I don't think there is anything wrong with api call in the navbar as there is no degradation in performance at all and we won't be reusing the navbar anyway

Is not about degradation, is about the separation of concerns, but anyway, there are a few things to change.

  1. Content Layout shouldn't exist. In NextJS apps, the Layout component is responsible for these layouts that are repeated across the app.
  2. The user should be requested at the top level of the app and saved in a global state, then we can toggle between views in the components regarding the roles of the user.

alejandrobailo avatar May 30 '25 09:05 alejandrobailo

  1. Content Layout shouldn't exist. In NextJS apps, the Layout component is responsible for these layouts that are repeated across the app.
  2. The user should be requested at the top level of the app and saved in a global state, then we can toggle between views in the components regarding the roles of the user.

Yes! We can do that. I was just trying to use whatever we had for a quick fix.

chaithanya009 avatar May 30 '25 11:05 chaithanya009

  1. Content Layout shouldn't exist. In NextJS apps, the Layout component is responsible for these layouts that are repeated across the app.
  2. The user should be requested at the top level of the app and saved in a global state, then we can toggle between views in the components regarding the roles of the user.

Yes! We can do that. I was just trying to use whatever we had for a quick fix.

I appreciate your suggestion, thanks a lot.

alejandrobailo avatar May 30 '25 11:05 alejandrobailo

Hi @alejandrobailo . Implemented the long term fix!

Objective: Centralize page layout management by moving title/icon definitions and navbar functionality from individual pages into the main layout component.

What Changed: ✅ Centralized Metadata: Created PAGE_METADATA array in main-layout.tsx mapping all routes to titles/icons ✅ Unified Layout: Moved navbar rendering into main layout using usePathname() for dynamic content ✅ Simplified Pages: Removed ContentLayout wrapper from 13+ pages - they now render content directly ✅ Cleaner Architecture: Single source of truth for page metadata, reduced code duplication Key Benefits:

  1. DRY Principle: No more repeating title/icon on every page
  2. Easier Maintenance: Adding new pages only requires updating the metadata array
  3. Better Performance: Single layout renders navbar once vs. per-page rendering
  4. Cleaner Code: Pages focus purely on content without layout concerns

Result: All pages now automatically get proper titles, icons, and navbar through the centralized main layout system.

chaithanya009 avatar May 30 '25 18:05 chaithanya009