Skip to content

make navbars#89

Open
krishpra2315 wants to merge 22 commits intomainfrom
krish/65-navbar
Open

make navbars#89
krishpra2315 wants to merge 22 commits intomainfrom
krish/65-navbar

Conversation

@krishpra2315
Copy link
Collaborator

@krishpra2315 krishpra2315 commented Mar 6, 2026

Closes #65

To see different versions of navbar change the org slug in url:
Screenshot 2026-03-06 at 12 29 53 PM

Checklist:

  • I have mentioned a ticket above
  • My changes meet the acceptance criteria of said ticket
  • I have self-reviewed my changes
  • I have requested a review from the appropriate team member(s)
  • I have fixed all merge conflicts
  • I have ensured CI checks pass

krishpra2315 and others added 2 commits March 6, 2026 12:29
Co-authored-by: krishpra2315 <krishpra2315@users.noreply.github.com>
@netlify
Copy link

netlify bot commented Mar 6, 2026

Deploy Preview for servicestart ready!

Name Link
🔨 Latest commit 74d44ad
🔍 Latest deploy log https://app.netlify.com/projects/servicestart/deploys/69ab5b3a39fd1800084954d1
😎 Deploy Preview https://deploy-preview-89--servicestart.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@greptile-apps
Copy link

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR implements a multi-variant navbar system for the ServiceStart app, allowing each organization to independently configure their navigation layout (vertical sidebar, vertical icon-only, or horizontal with left/center/right alignment) via the OrganizationConfigKey.NavbarVariant database setting. It also adds a live unread notification count badge, extracts the login/signup forms into separate client components, adds the GET /notifications/unreadCount endpoint, and extends the seed script with four test organizations covering all navbar variants.

Areas needing improvement:

  • Invalid Tailwind classjustify-flex-end is used in both app/login/LoginForm.tsx (line 79) and app/signup/SignupForm.tsx (line 99); this class does not exist in Tailwind and will have no effect. It should be justify-end.
  • setTimeout with 0 delayUserProfileMenu.tsx and VerticalSidebarNav.tsx both use setTimeout(() => setMounted(true), 0) to defer a hydration guard. The project convention is to call functions directly; use setMounted(true) directly in the useEffect body.
  • Hardcoded px valuesLoginForm.tsx and SignupForm.tsx use many hardcoded pixel sizes (e.g. text-[48px], rounded-[4px], px-[12px]) and hex colors (e.g. text-[#22070B], bg-[#22070B]) instead of the project's rem-based units and CSS variable color tokens.
  • Mobile tests commented outplaywright.config.ts has the Mobile Chrome and Mobile Safari test projects commented out, removing automated mobile coverage for all new navbar layouts.

PR Checklist Score: 72/100

The backend work (service layer, API endpoint, schema, tests) is solid and well-structured. The main gaps are on the frontend side: invalid utility class, px vs rem discipline in the form components, missing mobile test coverage, and the setTimeout(0) pattern.

Confidence Score: 3/5

  • Functional but has a layout-breaking invalid Tailwind class and several style/convention violations in the form components that should be resolved before merging.
  • The core navbar system, services, and API endpoint are well-implemented and tested. However, justify-flex-end is an invalid Tailwind class that will silently break the intended layout of both the login and signup pages, and mobile test coverage was removed. Combined with the rem/px and setTimeout convention violations, these warrant fixing before merge.
  • app/login/LoginForm.tsx and app/signup/SignupForm.tsx need the most attention due to the invalid Tailwind class and hardcoded px/color values.

Important Files Changed

Filename Overview
components/Navbar.tsx New top-level Navbar dispatcher that reads the org's NavbarVariant config and conditionally renders VerticalSidebarNav, VerticalIconNav, or HorizontalNav; skips rendering on auth routes. Logic is clean and well-structured.
components/navigation/HorizontalNav.tsx New horizontal nav supporting left/center/right alignment with dropdown support and dynamic unread notification badge. Uses Tailwind color tokens correctly; no new issues beyond those already flagged in previous threads.
components/navigation/VerticalSidebarNav.tsx New vertical sidebar with logo, collapsible sub-menus, unread notification badge, and user profile display. Contains a setTimeout(..., 0) pattern for the mounted guard that should be replaced with a direct setMounted(true) call.
components/navigation/UserProfileMenu.tsx Dropdown user profile menu with sign-out functionality. Uses setTimeout(..., 0) for the hydration guard which violates project conventions — should call setMounted(true) directly.
app/login/LoginForm.tsx Extracted login form component. Contains an invalid Tailwind class justify-flex-end (should be justify-end), numerous hardcoded px values that should be rem, and hardcoded color values (text-[#22070B], bg-[#22070B]) that should use CSS variables.
app/signup/SignupForm.tsx Extracted signup form component. Same issues as LoginForm: justify-flex-end invalid class, hardcoded px sizing, and hardcoded hex color values throughout.
lib/hooks/useUnreadNotificationCount.ts New hook that fetches the unread notification count with abort controller support. Contains a deferred setIsLoading(true) via Promise.resolve().then(...) that was already flagged in a previous thread.
lib/services/OrganizationConfigService.ts Adds get/set handlers for NavbarVariant, NavbarColor, and LogoUrl config keys. Now includes proper validation against ALLOWED_NAVBAR_VARIANTS in setNavbarVariant, addressing a previously flagged concern.
playwright.config.ts Mobile test configurations (Mobile Chrome / Mobile Safari) have been commented out, removing mobile test coverage for all new navbar components.
api/notifications.ts Adds GET /unreadCount endpoint that returns the count of unread notifications for the active organization. Clean, properly authenticated, well-tested.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[app/layout.tsx\nSuspense + Navbar] --> B{pathname check}
    B -->|/login or /signup| C[Render children only\nno navbar]
    B -->|other routes| D[Read NavbarVariant\nfrom OrganizationConfig]
    D --> E{variant?}
    E -->|sunset-vertical-sidebar\ndefault| F[VerticalSidebarNav\nLogo + text labels + profile]
    E -->|sunset-vertical-icon| G[VerticalIconNav\nIcon-only sidebar]
    E -->|sunset-horizontal-*| H[HorizontalNav\nalignment: left / center / right]
    F & G & H --> I[useUnreadNotificationCount\nGET /notifications/unreadCount]
    I --> J[NotificationService\n.countByUserAndOrganization]
    J --> K[(DB: notifications table)]
    F & H --> L[UserProfileMenu\nProfile + Sign Out]
    D --> M[useOrganizationConfig\nreads ?org param or active org slug]
    M --> N[OrganizationConfigService\ngetNavbarVariant / getLogoUrl]
    N --> K
Loading

Last reviewed commit: 025376d

github-actions bot and others added 3 commits March 6, 2026 17:51
Co-authored-by: krishpra2315 <krishpra2315@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Copy link
Collaborator

@renatodellosso renatodellosso left a comment

Choose a reason for hiding this comment

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

Looks good overall, but there's a couple cleanup items.

Also remove Sunset prefix from files since we'll be using those layouts for every organization and swapping out the logos.

@renatodellosso
Copy link
Collaborator

Just tried out the navbar and it feels really smooth. Great work with that!

One note: the "Menu Item" doesn't turn my cursor into the button pointer.
Screenshot 2026-03-06 at 1 36 59 PM

Copy link
Collaborator

@renatodellosso renatodellosso left a comment

Choose a reason for hiding this comment

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

Let me know about the login/signup page changes. I'm not sure we need to wrap the entire page in Suspense.

If login/signup does in fact need a Suspense, do the last couple clean up items, then you can merge.

@renatodellosso
Copy link
Collaborator

Also disable the ?org= query param.

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Navbar

2 participants