feat: implement navbar redesign

[kantord]

recording-2026-03-06_17-34-01.mp4

Empty state for secrets:

This PR:

• implements new design for navbar (simplify items)
• moves secrets to the settings page instead of an independent page
• convert playground into a side drawer instead of an independent page

[Copilot]

Pull request overview

Implements a redesigned top navigation by simplifying primary routes, moving “Secrets” into Settings, and introducing an in-app Assistant drawer (chat) accessible from the navbar.

Changes:

• Redesign the TopNav layout/actions and add an Assistant drawer toggle with shared provider state.
• Consolidate Secrets into Settings (new Settings “Secrets” tab + query prefetching) and remove standalone /secrets and /playground routes.
• Update/extend settings tab tests to account for Suspense + the new Secrets tab.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
renderer/src/routes/settings.tsx	Adds secrets tab handling and preloads secrets keys via route loader.
renderer/src/routes/playground.tsx	Removes the /playground route.
renderer/src/routes/__root.tsx	Wraps app with AssistantDrawerProvider and mounts AssistantDrawer.
renderer/src/route-tree.gen.ts	Removes /secrets and /playground from generated route tree.
renderer/src/features/secrets/components/secrets-table.tsx	Adds “New secret” action button next to search.
renderer/src/features/chat/components/chat-interface.tsx	Adds hideTitle mode for embedding chat in the drawer.
renderer/src/common/hooks/use-assistant-drawer.ts	Introduces useAssistantDrawer hook + context contract.
renderer/src/common/contexts/assistant-drawer.tsx	Adds provider for Assistant drawer open/close state.
renderer/src/common/components/settings/tabs/settings-tabs.tsx	Adds “Secrets” tab and renders SecretsTab.
renderer/src/common/components/settings/tabs/secrets-tab.tsx	Converts secrets page into a settings tab with section title.
renderer/src/common/components/settings/tabs/tests/settings-tabs.test.tsx	Wraps in Suspense and asserts Secrets tab presence/behavior.
renderer/src/common/components/settings/tabs/tests/secrets-tab.test.tsx	Updates test to use SecretsTab instead of removed route component.
renderer/src/common/components/layout/top-nav/index.tsx	Refactors nav links and adds Settings icon + Assistant toggle button.
renderer/src/common/components/layout/top-nav/container.tsx	Adjusts platform padding and container spacing to match redesign.
renderer/src/common/components/layout/assistant-drawer.tsx	Adds drawer UI + overlay and embeds ChatInterface hideTitle.
renderer/src/common/components/help/help-dropdown.tsx	Converts Help trigger to icon-only button styling.

Comments suppressed due to low confidence (1)

renderer/src/route-tree.gen.ts:135

• After removing the /playground and /secrets routes from the route tree, there is still code referencing these paths as valid routes (e.g., ORDERED_ROUTES in renderer/src/common/components/link-view-transition.tsx includes '/playground' and '/secrets'). This will break type-safety (and may fail compilation) because FileRouteTypes['fullPaths'] no longer contains those strings. Update any remaining route lists/usages to remove or replace those paths.

  fullPaths:
    | "/"
    | "/cli-issue"
    | "/mcp-optimizer"
    | "/settings"
    | "/shutdown"
    | "/registry"
    | "/customize-tools/$serverName"
    | "/group/$groupName"
    | "/registry-group/$name"
    | "/registry/$name"
    | "/logs/$groupName/$serverName"

[kantord]

/build-test

[github-actions]

Build Failed for PR #1688

The build encountered errors. Please check the workflow run for details.

Triggered by @kantord

[kantord]

@claude deep review please

[claude]

Claude finished @kantord's task in 2m 45s —— View job

---

Deep Review

---

Summary

The PR is a well-structured navbar simplification. The architectural decisions (Secrets→Settings tab, Playground→side drawer) are reasonable. The code is generally clean and well-commented. Below are findings ordered by severity.

---

🔴 High — Missing error boundary around SecretsTab

File: renderer/src/common/components/settings/tabs/settings-tabs.tsx:88

SecretsTab uses useSuspenseQuery, but settings-tabs.tsx does not wrap it in an error boundary. The route loader now uses prefetchQuery (which swallows errors rather than throwing them), but does still store error state in the query cache. When the Secrets API is unavailable, the cached query is in an error state, and useSuspenseQuery will re-throw that error when SecretsTab renders. Without a local error boundary, this exception will bubble up to the nearest app-level boundary and could crash the entire Settings page.

The test suite wraps in <Suspense fallback={null}> but adds no <ErrorBoundary>, so this failure path is not tested.

// settings-tabs.tsx — SecretsTab needs both boundaries:
<TabsContent value="secrets" className="mt-0">
  <ErrorBoundary fallback={<SecretsErrorState />}>
    <Suspense fallback={<SecretsLoadingState />}>
      <SecretsTab />
    </Suspense>
  </ErrorBoundary>
</TabsContent>

Note: prefetchQuery addressed the route-level crash (copilot's earlier concern), but the tab-level rendering failure path remains open. Fix this →

---

🟡 Medium — WindowControls flicker on drawer open/close

File: renderer/src/common/components/layout/top-nav/index.tsx:167

{!isAssistantOpen && <WindowControls />}

The platform-native window controls disappear from the top-nav the instant isAssistantOpen becomes true, which is synchronous — before the drawer has finished sliding in. This causes a visible "flash" where controls vanish, then reappear in the drawer header 200ms later. On close, the reverse happens: drawer controls disappear immediately on isAssistantOpen=false, while the drawer slides out for 200ms. During that 200ms, there are no window controls anywhere.

On Linux/Windows this leaves the minimize/maximize/close buttons unrendered for 200ms on each toggle. Consider keying the controls' visibility off isMounted (the drawer's internal animation state) rather than isAssistantOpen, or rendering both and simply hiding the nav ones with CSS during the animation.

---

🟡 Medium — hideTitle duplicates toolbar rendering logic in ChatInterface

File: renderer/src/features/chat/components/chat-interface.tsx:104-134

The {!hideTitle && ...} / {hideTitle && ...} pattern renders two structurally different toolbars for the same logical slot. The drawer mode strips the title and changes "Clear Chat" to "New conversation" (with a different icon). These two blocks will diverge over time (e.g. if someone adds a button only to one branch). A cleaner approach is a single toolbar slot that accepts a rendered title:

// Instead of hideTitle: boolean, consider:
interface ChatInterfaceProps {
  header?: React.ReactNode  // null in drawer mode, <TitlePage> in full-page mode
}

This is a maintainability concern, not a bug. Fix this →

---

🟡 Medium — Drawer unmount kills in-flight streaming

File: renderer/src/common/components/layout/assistant-drawer.tsx:55

When the user closes the drawer while a streaming response is in progress, isMounted is set to false after 200ms, unmounting ChatInterface and useChatStreaming. This cancels any active request. If the chat is persisted (it appears it is, given isPersistentLoading and loadPersistedSettings), the partial response may not be saved. Worth a manual test: start a long response, immediately close the drawer, reopen — does the partial message appear?

---

🟡 Medium — Drawer accessibility gaps

File: renderer/src/common/components/layout/assistant-drawer.tsx:43

1. The drawer <div> has aria-label="Assistant" but no semantic role. It should be role="dialog" with aria-modal="true" so assistive technology announces it as a modal dialog, and screen-readers correctly constrain navigation inside it when open.

2. Focus is not moved into the drawer on open. Keyboard users can't access the drawer content without tabbing through the backdrop first.

3. File: renderer/src/common/components/layout/top-nav/index.tsx:153
   The toggle button lacks aria-expanded:

   <button
     onClick={toggleAssistant}
    
     aria-expanded={isAssistantOpen}   // ← missing

Fix accessibility issues →

---

🟢 Low / Nits

useIsActive returns a closure, not a value
renderer/src/common/components/layout/top-nav/index.tsx:52
The hook returns a function, which is an unusual pattern. It works, but violates the "hooks return values" mental model. Consider naming it useActivePathMatcher or simply inlining it, since it's only 2 lines. Not a bug.

secretKey initialised as undefined but reset to ''
renderer/src/common/components/settings/tabs/secrets-tab.tsx:17,65

const [secretKey, setSecretKey] = useState<string | undefined>(undefined)
// ...
setSecretKey('')  // in "new secret" handler

The mutation hook is always initialised with secretKey ?? ''. The distinction between undefined (never interacted) and '' (new-secret mode) is not meaningful — both take the create path. Consider using null for "none selected" to make intent explicit.

SecretsTab renders SettingsSectionTitle with hardcoded "Secrets"
renderer/src/common/components/settings/tabs/secrets-tab.tsx:43
Fine as-is, but if the tab label is ever renamed this title won't update automatically.

---

✅ Things done well

• prefetchQuery instead of ensureQueryData in the settings loader is the right call — it prevents an offline secrets API from killing all of Settings.
• aria-label="Help" was added to the help button (copilot concern resolved).
• The Electron drag-region handling in AssistantDrawer is carefully considered and well-commented — the unmount-on-close approach for -webkit-app-region is the right tradeoff.
• Route type safety in link-view-transition.tsx was correctly cleaned up by removing /playground and /secrets from ORDERED_ROUTES.
• Test coverage for the new Secrets tab (both the isolated secrets-tab.test.tsx and the integration in settings-tabs.test.tsx) is solid.

---

[kantord]

@claude deep review again please

[claude]

Claude finished @kantord's task in 2m 46s —— View job

---

Deep Review

• Read changed files
• Analyze code quality, bugs, and architecture
• Post review findings

---

This is a second-pass review. The previous Claude review identified several issues. Here's an updated status and new findings.

Status of Previously Reported Issues

Issue	Status
🔴 Missing ErrorBoundary around SecretsTab	Still open
🟡 WindowControls flicker on drawer open/close	Still open
🟡 hideTitle duplicates toolbar rendering	Still open
🟡 Drawer unmount kills in-flight streaming	Still open
🟡 Drawer accessibility gaps (role, focus, aria-expanded)	Still open
✅ aria-label="Help" added to HelpDropdown	Resolved
✅ /playground and /secrets removed from ORDERED_ROUTES	Resolved
✅ prefetchQuery instead of ensureQueryData in settings loader	Resolved

---

🔴 High — SecretsTab still has no ErrorBoundary

renderer/src/common/components/settings/tabs/settings-tabs.tsx:88

SecretsTab calls useSuspenseQuery. If the secrets API returns an error (offline, auth failure, etc.), that error gets re-thrown at render time. There is no <ErrorBoundary> around the tab, so the exception propagates to the nearest app-level boundary — crashing the entire Settings page, not just the Secrets tab.

The loader uses prefetchQuery (non-throwing), but the cached query is still marked as errored. When the user clicks the Secrets tab, useSuspenseQuery replays that error.

The settings-tabs.test.tsx wraps in <Suspense fallback={null}> but adds no <ErrorBoundary>, so this failure path is untested.

// settings-tabs.tsx
<TabsContent value="secrets" className="mt-0">
  <ErrorBoundary fallback={<div className="text-destructive p-4">Unable to load secrets. Check your connection and try again.</div>}>
    <Suspense fallback={<SecretsLoadingState />}>
      <SecretsTab />
    </Suspense>
  </ErrorBoundary>
</TabsContent>

Fix this →

---

🟡 Medium — window.OsDesign exposed in production builds

renderer/src/common/lib/os-design.ts:57
renderer/src/renderer.tsx

window.OsDesign = osDesignDevtools   // runs unconditionally at module level

renderer.tsx imports os-design.ts at the top level with the comment "Import OS design devtools to bind OsDesign.setMac/setWindows/reset to window" — but there is no DEV guard. This means production builds expose window.OsDesign.setMac() / setWindows() / reset() to any user who opens DevTools. While not a security issue (only affects visual layout), it's inconsistent with the intent of a "DevTools helper" and leaks internal implementation details.

Compare with feature-flags.ts, which presumably has a similar pattern — check if it guards on env.

// os-design.ts — guard the window assignment
if (import.meta.env.DEV) {
  window.OsDesign = osDesignDevtools
}

Fix this →

---

🟡 Medium — WindowControls isMaximized icon can desync

renderer/src/common/components/layout/top-nav/window-controls.tsx:9-12

useEffect(() => {
  window.electronAPI.windowControls.isMaximized().then(setIsMaximized)
}, [])

The initial maximized state is fetched once on mount. It's also updated after the user clicks handleMaximize. But there's no listener for external maximize events — e.g., the user double-clicking the title bar, OS keyboard shortcuts (Win+↑ on Windows), or snap layouts. In those cases, the icon shows the wrong state until the user clicks the maximize button themselves.

Electron exposes BrowserWindow maximize/unmaximize events on the main process. These can be forwarded via IPC to the renderer (similar to how onUpdateDownloaded works). If this is already handled somewhere, the useEffect dependency array should reference the listener.

---

🟡 Medium — Drawer accessibility: still missing role, aria-modal, focus management, and aria-expanded

renderer/src/common/components/layout/assistant-drawer.tsx:43
renderer/src/common/components/layout/top-nav/index.tsx:153

None of the accessibility issues from the previous review were addressed:

1. The drawer <div> has aria-label="Assistant" but no role="dialog" or aria-modal="true". Screen readers won't treat it as a dialog and won't constrain navigation to it.
2. Focus is not moved into the drawer on open — keyboard users must tab through the entire page before reaching drawer content.
3. The toggle button in top-nav/index.tsx lacks aria-expanded={isAssistantOpen}.

// assistant-drawer.tsx
<div
  role="dialog"
  aria-modal="true"
 
  ...
>

// top-nav/index.tsx
<button
  onClick={toggleAssistant}
 
  aria-expanded={isAssistantOpen}
  ...
>

Fix this →

---

🟡 Medium — Full-width drawer hides backdrop click target on small viewports

renderer/src/common/components/layout/assistant-drawer.tsx:47

className="... w-[700px] max-w-full ..."

On displays narrower than 700px, the drawer becomes full-width and completely overlaps the inset-0 backdrop. At that point there's no visible area outside the drawer to click for dismissal — the UX expectation (click outside to close) silently breaks. Since this is an Electron app, small viewports are less common, but it's still worth a min-w-0 w-[700px] max-w-[calc(100%-48px)] or similar to always leave a slim clickable strip on the left.

---

🟢 Low / Nits

SecretsTab — useMutationUpdateSecret('') on initial render
renderer/src/common/components/settings/tabs/secrets-tab.tsx:19

const { mutateAsync: updateSecret } = useMutationUpdateSecret(secretKey ?? '')

On initial render, secretKey is undefined, so updateSecret is initialised for key ''. This creates a mutation with an empty key path on every mount. Functionally harmless (the onSubmit logic ensures updateSecret is only called when secretKey is set), but semantically messy. Using null (or always initialising to '' instead of undefined) would make intent clearer.

useIsActive returns a closure, not a value
renderer/src/common/components/layout/top-nav/index.tsx:52

This pattern is unusual for a hook but works fine. Could be renamed useActivePathMatcher to signal that it returns a function.

SecretsTab — SettingsSectionTitle with hardcoded "Secrets"
renderer/src/common/components/settings/tabs/secrets-tab.tsx:43

Fine as-is; just note that if the tab label is renamed it won't auto-update.

---

✅ Things done well (new)

• prefetchQuery in settings route loader correctly avoids failing the whole Settings page on secrets API errors — right call.
• getOsDesignVariant() SessionStorage override pattern is a clean dev ergonomics approach — named API, single storage key, auto-reload.
• AssistantDrawerProvider / useAssistantDrawer context is well-structured — minimal interface (isOpen, toggle, close), proper null guard, clean separation from the drawer UI itself.
• Secrets e2e test correctly navigates to Settings → Secrets tab via the new flow instead of the deleted /secrets route.
• Playground e2e test correctly opens the Assistant drawer via the navbar button.

---

| Branch: navbar-redesign