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

[Nikhil (shadowfax92)]

This reverts commit 42aa0ff.

[greptile-apps]

Greptile Summary

This PR reverts the settings popup dialog (#477) and restores settings to a dedicated full-page experience with its own sidebar layout (SettingsSidebarLayout). The dialog-based background-location routing pattern is removed in favour of straightforward route-based navigation, and the deleted SettingsDialog and useOpenSettings hook are cleanly removed with no dead code left behind.

Key changes:

• SettingsDialog.tsx and useOpenSettings.ts are deleted; settings pages are now routed normally under /settings/*
• A new SettingsSidebarLayout wraps all settings routes with a dedicated SettingsSidebar, including mobile Sheet support
• SidebarNavigation settings entry is simplified to a plain NavLink pointing to /settings/ai
• Workflows link is moved from the main sidebar nav into the settings sidebar
• Bug introduced: SETTINGS_PAGE_VIEWED_EVENT is mistakenly tracked inside SidebarLayout (the main app layout), causing every navigation to /home, /scheduled, /skills, etc. to emit a false settings analytics event — the correct tracking already exists in SettingsSidebarLayout
• The mobile sheet in SettingsSidebarLayout does not auto-close after the user taps a settings nav link (empty useEffect dependency array)

Confidence Score: 3/5

• Safe to merge functionally, but contains an analytics data-corruption bug that should be fixed before shipping.
• The core revert logic is correct and the dead code is cleanly removed. However, the accidental addition of SETTINGS_PAGE_VIEWED_EVENT tracking inside SidebarLayout will silently send false analytics events for every non-settings page navigation, which is a meaningful data integrity issue. The mobile sidebar UX regression is minor but worth fixing.
• packages/browseros-agent/apps/agent/entrypoints/app/layout/SidebarLayout.tsx — the erroneous analytics tracking must be removed before merging.

Important Files Changed

Filename	Overview
packages/browseros-agent/apps/agent/entrypoints/app/layout/SidebarLayout.tsx	Adds incorrect analytics tracking — SETTINGS_PAGE_VIEWED_EVENT fires for all non-settings pages served by this layout (home, scheduled, skills, etc.), corrupting analytics data.
packages/browseros-agent/apps/agent/entrypoints/app/layout/SettingsSidebarLayout.tsx	New layout for settings pages with dedicated sidebar and mobile sheet support; analytics tracking is correct but the mobile sheet won't close after in-settings navigation due to an empty useEffect dependency array.
packages/browseros-agent/apps/agent/components/sidebar/SettingsSidebar.tsx	New sidebar component for settings navigation; clean implementation with proper feature-flag filtering, external/internal link handling, and theme toggle.
packages/browseros-agent/apps/agent/entrypoints/app/App.tsx	Replaces the dialog-based settings overlay (background-location pattern) with dedicated route-based settings pages under SettingsSidebarLayout; AppRoutes inlined into App for simplification.
packages/browseros-agent/apps/agent/components/sidebar/SidebarNavigation.tsx	Removes the action-based settings button pattern; Settings now uses a standard NavLink to /settings/ai with a hardcoded path check for active-state highlighting.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["User clicks Settings in main sidebar"] --> B["NavLink to /settings/ai"]
    B --> C["SettingsSidebarLayout"]
    C --> D["SettingsSidebar"]
    C --> E["Outlet - content area"]
    D --> F["AISettingsPage"]
    D --> G["LlmHubPage"]
    D --> H["SearchProviderPage"]
    D --> I["MCPSettingsPage"]
    D --> J["CustomizationPage"]
    D --> K["WorkflowsPageWrapper"]
    DELETED_Dialog -.->|"was dialog overlay"| DELETED_Hook
    DELETED_Dialog["DELETED: SettingsDialog"]
    DELETED_Hook["DELETED: useOpenSettings"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/browseros-agent/apps/agent/entrypoints/app/layout/SidebarLayout.tsx
Line: 28-30

Comment:
**Wrong analytics event tracked for non-settings pages**

`SETTINGS_PAGE_VIEWED_EVENT` is being tracked on every navigation within `SidebarLayout`, which is the **main app layout** serving routes like `/home`, `/scheduled`, `/skills`, etc. This will flood analytics with false settings page-view events for all regular page navigations.

The `SettingsSidebarLayout` already correctly tracks this event for settings pages. This tracking in `SidebarLayout` should be removed entirely — it appears to be a copy-paste error from `SettingsSidebarLayout`.

```suggestion
  useEffect(() => {
    setMobileOpen(false)
  }, [])
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/browseros-agent/apps/agent/entrypoints/app/layout/SettingsSidebarLayout.tsx
Line: 22-24

Comment:
**Mobile sidebar doesn't close on settings navigation**

The `useEffect` here runs only once on mount (empty dependency array), so the mobile `Sheet` sidebar will remain open after the user taps a settings nav link. The dependency should include `location.pathname` so it closes whenever the route changes — matching the expected UX of a slide-out nav sheet.

```suggestion
  useEffect(() => {
    setMobileOpen(false)
  }, [location.pathname])
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/browseros-agent/apps/agent/components/sidebar/SidebarNavigation.tsx
Line: 79-82

Comment:
**Hardcoded path string for active-state detection**

The active-state logic special-cases the literal string `'/settings/ai'` to match any `/settings/*` route. This is fragile — if the Settings nav item's `to` value ever changes (e.g. to `/settings/chat`), the highlight would break silently. Consider comparing against a prefix or using a dedicated `activePrefix` field on the `NavItem` type instead.

```suggestion
            const isActive = item.to.startsWith('/settings')
              ? location.pathname.startsWith('/settings')
              : location.pathname === item.to
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "Revert "feat: conver..."}

[Nikhil (shadowfax92)]

Review Feedback Responses

SettingsSidebarLayout.tsx:22-24 — Mobile sidebar doesn't close on navigation
Agreed — the empty dependency array made setMobileOpen(false) a no-op (state is already false on mount). Added location.pathname to the dependency array so the mobile sheet closes on route changes.

SidebarNavigation.tsx:79-82 — Hardcoded path string for active-state detection
Respectfully pushing back on this one. There's exactly one Settings nav item, and the coupling between item.to === '/settings/ai' and the nav item definition is explicit — if the default tab changes, both locations need updating, which makes the breakage obvious rather than silent. The suggested item.to.startsWith('/settings') introduces a different fragility: any future nav item with a /settings-prefixed path would be misclassified as settings for active-state purposes. For a revert PR, I'd rather keep the change minimal and not introduce improvements beyond the scope of the revert.