fix: revert: convert settings to popup dialog (#477) (#498)

* Revert "feat: convert settings to popup dialog (#477)"

This reverts commit 42aa0ff1ef.

* fix: address review feedback for PR #498

- Remove erroneous SETTINGS_PAGE_VIEWED_EVENT tracking from SidebarLayout
  (was firing on every non-settings page navigation)
- Fix mobile settings sidebar not closing on route change by merging
  setMobileOpen(false) into the pathname-dependent analytics useEffect

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Nikhil
2026-03-19 11:13:14 -07:00
committed by GitHub
parent 19069cb9c4
commit 7bdeeb85d5
7 changed files with 280 additions and 357 deletions

View File

@@ -0,0 +1,182 @@
import {
ArrowLeft,
BookOpen,
Bot,
Compass,
GitBranch,
MessageSquare,
Palette,
RotateCcw,
Search,
Server,
} from 'lucide-react'
import type { FC } from 'react'
import { NavLink } from 'react-router'
import { ThemeToggle } from '@/components/elements/theme-toggle'
import { Feature } from '@/lib/browseros/capabilities'
import { useCapabilities } from '@/lib/browseros/useCapabilities'
import { cn } from '@/lib/utils'
type BaseNavItem = {
name: string
icon: typeof Bot
feature?: Feature
}
type InternalNavItem = BaseNavItem & {
href?: never
to: string
}
type ExternalNavItem = BaseNavItem & {
href: string
to?: never
}
type NavItem = InternalNavItem | ExternalNavItem
type NavSection = {
label: string
items: NavItem[]
}
function isExternalNavItem(item: NavItem): item is ExternalNavItem {
return 'href' in item
}
const getNavLinkClassName = (isActive: boolean) =>
cn(
'flex h-9 items-center gap-2 overflow-hidden whitespace-nowrap rounded-md px-3 font-medium text-sm transition-colors hover:bg-sidebar-accent hover:text-sidebar-accent-foreground',
isActive && 'bg-sidebar-accent text-sidebar-accent-foreground',
)
const getSectionClassName = (index: number) =>
cn(index > 0 && 'mt-3 border-t pt-3')
const sectionLabelClassName =
'mb-2 px-3 font-semibold text-[10px] text-muted-foreground uppercase tracking-[0.18em]'
const primarySettingsSections: NavSection[] = [
{
label: 'Provider Settings',
items: [
{ name: 'BrowserOS AI', to: '/settings/ai', icon: Bot },
{
name: 'Chat & Council Provider',
to: '/settings/chat',
icon: MessageSquare,
},
{ name: 'Search Provider', to: '/settings/search', icon: Search },
],
},
{
label: 'Other',
items: [
{
name: 'Customize BrowserOS',
to: '/settings/customization',
icon: Palette,
feature: Feature.CUSTOMIZATION_SUPPORT,
},
{ name: 'BrowserOS as MCP', to: '/settings/mcp', icon: Server },
{
name: 'Workflows',
to: '/workflows',
icon: GitBranch,
feature: Feature.WORKFLOW_SUPPORT,
},
],
},
]
const helpItems: NavItem[] = [
{ name: 'Docs', href: 'https://docs.browseros.com/', icon: BookOpen },
{ name: 'Features', to: '/onboarding/features', icon: Compass },
{ name: 'Revisit Onboarding', to: '/onboarding', icon: RotateCcw },
]
export const SettingsSidebar: FC = () => {
const { supports } = useCapabilities()
const filteredSections = primarySettingsSections
.map((section) => ({
...section,
items: section.items.filter(
(item) => !item.feature || supports(item.feature),
),
}))
.filter((section) => section.items.length > 0)
const filteredHelpItems = helpItems.filter(
(item) => !item.feature || supports(item.feature),
)
const renderNavItem = (item: NavItem) => {
const Icon = item.icon
if (isExternalNavItem(item)) {
return (
<a
key={item.href}
href={item.href}
target="_blank"
rel="noopener noreferrer"
className={getNavLinkClassName(false)}
>
<Icon className="size-4 shrink-0" />
<span className="truncate">{item.name}</span>
</a>
)
}
return (
<NavLink
key={item.to}
to={item.to}
end
className={({ isActive }) => getNavLinkClassName(isActive)}
>
<Icon className="size-4 shrink-0" />
<span className="truncate">{item.name}</span>
</NavLink>
)
}
const renderSection = (section: NavSection, index: number) => (
<div key={section.label} className={getSectionClassName(index)}>
<div className={sectionLabelClassName}>{section.label}</div>
<nav className="space-y-1">{section.items.map(renderNavItem)}</nav>
</div>
)
return (
<div className="flex h-full w-64 flex-col border-r bg-sidebar text-sidebar-foreground">
<div className="flex h-14 items-center justify-between border-b px-2">
<NavLink
to="/home"
className="flex h-9 items-center gap-2 overflow-hidden whitespace-nowrap rounded-md px-3 font-medium text-sm transition-colors hover:bg-sidebar-accent hover:text-sidebar-accent-foreground"
>
<ArrowLeft className="size-4 shrink-0" />
<span className="truncate">Back</span>
</NavLink>
<ThemeToggle
className="mr-1 h-8 w-8 shrink-0"
iconClassName="h-4 w-4"
/>
</div>
<div className="flex flex-1 flex-col overflow-y-auto overflow-x-hidden p-2">
<div className="mb-2 px-3 font-semibold text-muted-foreground text-xs uppercase tracking-wider">
Settings
</div>
<div>{filteredSections.map(renderSection)}</div>
<div className="mt-auto pt-4">
<div className={sectionLabelClassName}>Help</div>
<nav className="space-y-1">
{filteredHelpItems.map(renderNavItem)}
</nav>
</div>
</div>
</div>
)
}

View File

@@ -1,7 +1,6 @@
import {
Brain,
CalendarClock,
GitBranch,
Home,
PlugZap,
Settings,
@@ -18,7 +17,6 @@ import {
} from '@/components/ui/tooltip'
import { Feature } from '@/lib/browseros/capabilities'
import { useCapabilities } from '@/lib/browseros/useCapabilities'
import { useOpenSettings } from '@/lib/settings/useOpenSettings'
import { cn } from '@/lib/utils'
interface SidebarNavigationProps {
@@ -27,10 +25,9 @@ interface SidebarNavigationProps {
type NavItem = {
name: string
to?: string
to: string
icon: typeof Home
feature?: Feature
action?: 'settings'
}
const primaryNavItems: NavItem[] = [
@@ -42,12 +39,6 @@ const primaryNavItems: NavItem[] = [
feature: Feature.MANAGED_MCP_SUPPORT,
},
{ name: 'Scheduled Tasks', to: '/scheduled', icon: CalendarClock },
{
name: 'Workflows',
to: '/workflows',
icon: GitBranch,
feature: Feature.WORKFLOW_SUPPORT,
},
{
name: 'Skills',
to: '/home/skills',
@@ -66,19 +57,14 @@ const primaryNavItems: NavItem[] = [
icon: Sparkles,
feature: Feature.SOUL_SUPPORT,
},
{ name: 'Settings', icon: Settings, action: 'settings' },
{ name: 'Settings', to: '/settings/ai', icon: Settings },
]
const navItemClassName =
'flex h-9 items-center gap-2 overflow-hidden whitespace-nowrap rounded-md px-3 font-medium text-sm transition-colors hover:bg-sidebar-accent hover:text-sidebar-accent-foreground'
export const SidebarNavigation: FC<SidebarNavigationProps> = ({
expanded = true,
}) => {
const location = useLocation()
const openSettings = useOpenSettings()
const { supports } = useCapabilities()
const isSettingsActive = location.pathname.startsWith('/settings')
const filteredItems = primaryNavItems.filter(
(item) => !item.feature || supports(item.feature),
@@ -90,52 +76,16 @@ export const SidebarNavigation: FC<SidebarNavigationProps> = ({
<nav className="space-y-1">
{filteredItems.map((item) => {
const Icon = item.icon
const isActive =
item.to === '/settings/ai'
? location.pathname.startsWith('/settings')
: location.pathname === item.to
// Settings is a button that opens the dialog
if (item.action === 'settings') {
const settingsButton = (
<button
type="button"
onClick={() => openSettings()}
className={cn(
navItemClassName,
'w-full',
isSettingsActive &&
'bg-sidebar-accent text-sidebar-accent-foreground',
)}
>
<Icon className="size-4 shrink-0" />
<span
className={cn(
'truncate transition-opacity duration-200',
expanded ? 'opacity-100' : 'opacity-0',
)}
>
{item.name}
</span>
</button>
)
if (!expanded) {
return (
<Tooltip key="settings">
<TooltipTrigger asChild>{settingsButton}</TooltipTrigger>
<TooltipContent side="right">{item.name}</TooltipContent>
</Tooltip>
)
}
return <div key="settings">{settingsButton}</div>
}
// Regular nav items use NavLink
const itemPath = item.to ?? '/home'
const isActive = location.pathname === itemPath
const navItem = (
<NavLink
to={itemPath}
to={item.to}
className={cn(
navItemClassName,
'flex h-9 items-center gap-2 overflow-hidden whitespace-nowrap rounded-md px-3 font-medium text-sm transition-colors hover:bg-sidebar-accent hover:text-sidebar-accent-foreground',
isActive &&
'bg-sidebar-accent text-sidebar-accent-foreground',
)}

View File

@@ -1,13 +1,5 @@
import type { FC } from 'react'
import {
HashRouter,
type Location,
Navigate,
Route,
Routes,
useLocation,
useParams,
} from 'react-router'
import { HashRouter, Navigate, Route, Routes, useParams } from 'react-router'
import { NewTab } from '../newtab/index/NewTab'
import { NewTabChat } from '../newtab/index/NewTabChat'
@@ -17,18 +9,23 @@ import { OnboardingDemo } from '../onboarding/demo/OnboardingDemo'
import { FeaturesPage } from '../onboarding/features/Features'
import { Onboarding } from '../onboarding/index/Onboarding'
import { StepsLayout } from '../onboarding/steps/StepsLayout'
import { AISettingsPage } from './ai-settings/AISettingsPage'
import { ConnectMCP } from './connect-mcp/ConnectMCP'
import { CreateGraphWrapper } from './create-graph/CreateGraphWrapper'
import { CustomizationPage } from './customization/CustomizationPage'
import { SurveyPage } from './jtbd-agent/SurveyPage'
import { AuthLayout } from './layout/AuthLayout'
import { SettingsSidebarLayout } from './layout/SettingsSidebarLayout'
import { SidebarLayout } from './layout/SidebarLayout'
import { LlmHubPage } from './llm-hub/LlmHubPage'
import { LoginPage } from './login/LoginPage'
import { LogoutPage } from './login/LogoutPage'
import { MagicLinkCallback } from './login/MagicLinkCallback'
import { MCPSettingsPage } from './mcp-settings/MCPSettingsPage'
import { MemoryPage } from './memory/MemoryPage'
import { ProfilePage } from './profile/ProfilePage'
import { ScheduledTasksPage } from './scheduled-tasks/ScheduledTasksPage'
import { SettingsDialog } from './settings-dialog/SettingsDialog'
import { SearchProviderPage } from './search-provider/SearchProviderPage'
import { SkillsPage } from './skills/SkillsPage'
import { SoulPage } from './soul/SoulPage'
import { WorkflowsPageWrapper } from './workflows/WorkflowsPageWrapper'
@@ -64,29 +61,12 @@ const OptionsRedirect: FC = () => {
return <Navigate to={newPath} replace />
}
/** Redirect direct /settings/:tab visits so the dialog has a background page */
const SettingsRedirect: FC = () => {
const { tab } = useParams()
return (
<Navigate
to={`/settings/${tab || 'ai'}`}
state={{ backgroundLocation: { pathname: '/home' } }}
replace
/>
)
}
const AppRoutes: FC = () => {
const location = useLocation()
export const App: FC = () => {
const surveyParams = getSurveyParams()
const backgroundLocation = (
location.state as { backgroundLocation?: Location } | null
)?.backgroundLocation
return (
<>
<Routes location={backgroundLocation || location}>
<HashRouter>
<Routes>
{/* Public auth routes */}
<Route element={<AuthLayout />}>
<Route path="login" element={<LoginPage />} />
@@ -113,14 +93,18 @@ const AppRoutes: FC = () => {
<Route path="scheduled" element={<ScheduledTasksPage />} />
</Route>
{/* Survey page - standalone */}
<Route
path="settings/survey"
element={<SurveyPage {...surveyParams} />}
/>
{/* Direct /settings/:tab access without background location — redirect with one */}
<Route path="settings/:tab?" element={<SettingsRedirect />} />
{/* Settings with dedicated sidebar */}
<Route element={<SettingsSidebarLayout />}>
<Route path="settings">
<Route index element={<Navigate to="/settings/ai" replace />} />
<Route path="ai" element={<AISettingsPage key="ai" />} />
<Route path="chat" element={<LlmHubPage />} />
<Route path="mcp" element={<MCPSettingsPage />} />
<Route path="customization" element={<CustomizationPage />} />
<Route path="search" element={<SearchProviderPage />} />
<Route path="survey" element={<SurveyPage {...surveyParams} />} />
</Route>
</Route>
{/* Full-screen without sidebar */}
<Route path="workflows/create-graph" element={<CreateGraphWrapper />} />
@@ -156,19 +140,6 @@ const AppRoutes: FC = () => {
{/* Fallback to home */}
<Route path="*" element={<Navigate to="/home" replace />} />
</Routes>
{/* Modal overlay — renders settings dialog on top of background page */}
{backgroundLocation && (
<Routes>
<Route path="settings/:tab?" element={<SettingsDialog />} />
</Routes>
)}
</>
</HashRouter>
)
}
export const App: FC = () => (
<HashRouter>
<AppRoutes />
</HashRouter>
)

View File

@@ -0,0 +1,65 @@
import { Menu } from 'lucide-react'
import type { FC } from 'react'
import { useEffect, useState } from 'react'
import { Outlet, useLocation } from 'react-router'
import { SettingsSidebar } from '@/components/sidebar/SettingsSidebar'
import { Button } from '@/components/ui/button'
import { Sheet, SheetContent } from '@/components/ui/sheet'
import { useIsMobile } from '@/hooks/use-mobile'
import { SETTINGS_PAGE_VIEWED_EVENT } from '@/lib/constants/analyticsEvents'
import { track } from '@/lib/metrics/track'
import { RpcClientProvider } from '@/lib/rpc/RpcClientProvider'
export const SettingsSidebarLayout: FC = () => {
const location = useLocation()
const isMobile = useIsMobile()
const [mobileOpen, setMobileOpen] = useState(false)
useEffect(() => {
track(SETTINGS_PAGE_VIEWED_EVENT, { page: location.pathname })
setMobileOpen(false)
}, [location.pathname])
if (isMobile) {
return (
<RpcClientProvider>
<div className="flex min-h-screen flex-col bg-background">
<header className="flex h-14 shrink-0 items-center gap-2 border-b px-4">
<Button
variant="ghost"
size="icon"
className="-ml-1 size-7"
onClick={() => setMobileOpen(true)}
>
<Menu className="size-4" />
</Button>
<span className="font-semibold">Settings</span>
</header>
<main className="flex-1 overflow-y-auto">
<div className="mx-auto max-w-4xl px-4 py-8 sm:px-6 lg:px-8">
<Outlet />
</div>
</main>
<Sheet open={mobileOpen} onOpenChange={setMobileOpen}>
<SheetContent side="left" className="w-72 p-0">
<SettingsSidebar />
</SheetContent>
</Sheet>
</div>
</RpcClientProvider>
)
}
return (
<RpcClientProvider>
<div className="flex h-screen bg-background">
<SettingsSidebar />
<main className="flex-1 overflow-y-auto">
<div className="mx-auto max-w-4xl px-4 py-8 sm:px-6 lg:px-8">
<Outlet />
</div>
</main>
</div>
</RpcClientProvider>
)
}

View File

@@ -1,7 +1,7 @@
import { Menu } from 'lucide-react'
import type { FC } from 'react'
import { useCallback, useEffect, useRef, useState } from 'react'
import { Outlet } from 'react-router'
import { Outlet, useLocation } from 'react-router'
import { AppSidebar } from '@/components/sidebar/AppSidebar'
import { Button } from '@/components/ui/button'
import { Sheet, SheetContent } from '@/components/ui/sheet'
@@ -12,6 +12,7 @@ import { RpcClientProvider } from '@/lib/rpc/RpcClientProvider'
const COLLAPSE_DELAY = 150
export const SidebarLayout: FC = () => {
const location = useLocation()
const isMobile = useIsMobile()
const [sidebarOpen, setSidebarOpen] = useState(false)
const [mobileOpen, setMobileOpen] = useState(false)

View File

@@ -1,225 +0,0 @@
import {
BookOpen,
Bot,
Compass,
MessageSquare,
Palette,
RotateCcw,
Search,
Server,
X,
} from 'lucide-react'
import type { FC } from 'react'
import { useEffect } from 'react'
import {
type Location,
useLocation,
useNavigate,
useParams,
} from 'react-router'
import { Dialog, DialogContent, DialogTitle } from '@/components/ui/dialog'
import { Feature } from '@/lib/browseros/capabilities'
import { useCapabilities } from '@/lib/browseros/useCapabilities'
import { SETTINGS_PAGE_VIEWED_EVENT } from '@/lib/constants/analyticsEvents'
import { track } from '@/lib/metrics/track'
import { cn } from '@/lib/utils'
import { AISettingsPage } from '../ai-settings/AISettingsPage'
import { CustomizationPage } from '../customization/CustomizationPage'
import { LlmHubPage } from '../llm-hub/LlmHubPage'
import { MCPSettingsPage } from '../mcp-settings/MCPSettingsPage'
import { SearchProviderPage } from '../search-provider/SearchProviderPage'
type SettingsTab = {
id: string
name: string
icon: typeof Bot
feature?: Feature
component: FC
}
const settingsTabs: SettingsTab[] = [
{ id: 'ai', name: 'BrowserOS AI', icon: Bot, component: AISettingsPage },
{
id: 'chat',
name: 'Chat & Council Provider',
icon: MessageSquare,
component: LlmHubPage,
},
{
id: 'search',
name: 'Search Provider',
icon: Search,
component: SearchProviderPage,
},
{
id: 'customization',
name: 'Customize BrowserOS',
icon: Palette,
feature: Feature.CUSTOMIZATION_SUPPORT,
component: CustomizationPage,
},
{
id: 'mcp',
name: 'BrowserOS as MCP',
icon: Server,
component: MCPSettingsPage,
},
]
type HelpItem = {
name: string
icon: typeof Bot
href?: string
to?: string
}
const helpItems: HelpItem[] = [
{ name: 'Docs', href: 'https://docs.browseros.com/', icon: BookOpen },
{ name: 'Features', to: '/onboarding/features', icon: Compass },
{ name: 'Revisit Onboarding', to: '/onboarding', icon: RotateCcw },
]
export const SettingsDialog: FC = () => {
const { tab } = useParams<{ tab?: string }>()
const location = useLocation()
const navigate = useNavigate()
const { supports } = useCapabilities()
const backgroundLocation = (
location.state as { backgroundLocation?: Location } | null
)?.backgroundLocation
const visibleTabs = settingsTabs.filter(
(tabDef) => !tabDef.feature || supports(tabDef.feature),
)
const activeTab = visibleTabs.find((t) => t.id === tab) ? tab : 'ai'
useEffect(() => {
track(SETTINGS_PAGE_VIEWED_EVENT, { page: `settings/${activeTab}` })
}, [activeTab])
const handleClose = () => {
if (backgroundLocation) {
const target =
backgroundLocation.pathname +
(backgroundLocation.search || '') +
(backgroundLocation.hash || '')
navigate(target, { replace: true })
} else {
navigate('/home', { replace: true })
}
}
const handleTabChange = (tabId: string) => {
navigate(`/settings/${tabId}`, {
state: { backgroundLocation },
replace: true,
})
}
const handleHelpNavigation = (to: string) => {
navigate(to, { replace: true })
}
const activeTabConfig = visibleTabs.find((t) => t.id === activeTab)
const ActiveComponent = activeTabConfig?.component ?? AISettingsPage
return (
<Dialog
open
onOpenChange={(open) => {
if (!open) handleClose()
}}
>
<DialogContent
className="flex h-[85vh] max-h-[85vh] w-full flex-col gap-0 overflow-hidden p-0 sm:max-w-4xl"
showCloseButton={false}
>
<DialogTitle className="sr-only">Settings</DialogTitle>
<div className="flex h-full min-h-0">
{/* Left panel - tab navigation */}
<div className="flex w-52 shrink-0 flex-col border-r bg-muted/30">
<div className="px-4 pt-5 pb-3">
<span className="font-semibold text-muted-foreground text-xs uppercase tracking-wider">
Settings
</span>
</div>
<nav className="flex-1 space-y-0.5 overflow-y-auto px-2">
{visibleTabs.map((tabDef) => {
const Icon = tabDef.icon
return (
<button
key={tabDef.id}
type="button"
onClick={() => handleTabChange(tabDef.id)}
className={cn(
'flex w-full items-center gap-2 rounded-md px-3 py-2 font-medium text-sm transition-colors hover:bg-accent hover:text-accent-foreground',
activeTab === tabDef.id &&
'bg-accent text-accent-foreground',
)}
>
<Icon className="size-4 shrink-0" />
<span className="truncate">{tabDef.name}</span>
</button>
)
})}
</nav>
{/* Help section */}
<div className="border-t px-2 py-2">
<div className="mb-1 px-3 font-semibold text-[10px] text-muted-foreground uppercase tracking-[0.18em]">
Help
</div>
{helpItems.map((item) => {
const Icon = item.icon
if (item.href) {
return (
<a
key={item.name}
href={item.href}
target="_blank"
rel="noopener noreferrer"
className="flex w-full items-center gap-2 rounded-md px-3 py-2 font-medium text-sm transition-colors hover:bg-accent hover:text-accent-foreground"
>
<Icon className="size-4 shrink-0" />
<span className="truncate">{item.name}</span>
</a>
)
}
return (
<button
key={item.name}
type="button"
onClick={() => handleHelpNavigation(item.to ?? '/home')}
className="flex w-full items-center gap-2 rounded-md px-3 py-2 font-medium text-sm transition-colors hover:bg-accent hover:text-accent-foreground"
>
<Icon className="size-4 shrink-0" />
<span className="truncate">{item.name}</span>
</button>
)
})}
</div>
</div>
{/* Right panel - settings content */}
<div className="flex flex-1 flex-col overflow-hidden">
<div className="flex justify-end px-4 pt-3">
<button
type="button"
onClick={handleClose}
className="rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100"
>
<X className="size-4" />
<span className="sr-only">Close</span>
</button>
</div>
<div className="styled-scrollbar flex-1 overflow-y-auto px-6 pb-6">
<ActiveComponent />
</div>
</div>
</div>
</DialogContent>
</Dialog>
)
}

View File

@@ -1,21 +0,0 @@
import { useCallback } from 'react'
import { useLocation, useNavigate } from 'react-router'
/**
* Hook to open the settings dialog from anywhere in the app.
* Uses React Router's background location pattern so the dialog
* overlays the current page without unmounting it.
*/
export function useOpenSettings() {
const location = useLocation()
const navigate = useNavigate()
return useCallback(
(tab = 'ai') => {
navigate(`/settings/${tab}`, {
state: { backgroundLocation: location },
})
},
[location, navigate],
)
}