Skip to content

fix(web): render settings sections as real tabs#620

Merged
gbasin merged 1 commit into
masterfrom
fix/settings-section-tabs
Jul 20, 2026
Merged

fix(web): render settings sections as real tabs#620
gbasin merged 1 commit into
masterfrom
fix/settings-section-tabs

Conversation

@gbasin

@gbasin gbasin commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Switching tabs in the settings view changed the URL but not the content. It turned out to be broken in two ways.

The sidebar looked like tabs but was implemented as scroll anchors — every section rendered at once, and clicking one only called scrollIntoView.

1. The scroll container was never height-constrained

SettingsSurface.tsx:111 was a plain block div, so its child's flex-1 did nothing and the scroller sized to its content:

min-h-0 flex-1                    clientH 327   ← wrapper, correctly constrained
  min-h-0 flex-1 overflow-y-auto  clientH 682   ← scroller, NOT constrained
                                  scrollH 682   ← scrollHeight === clientHeight

scrollHeight === clientHeight makes scrollIntoView a guaranteed no-op. The overflow escaped up to the shell's overflow-hidden (scrollH 971 vs clientH 420) and was clipped — at a 420px-tall window, Agents and About sat at y=621 and were unreachable by any means, with no scrollbar to signal it.

2. Even scrolling correctly, it read as dead

The whole surface is ~682px tall, so at normal desktop heights everything already fit and there was nothing to scroll to. With no section headings in the content column either, clicking produced zero feedback.

Fix

Render only the active section. That removes the scroll dependency entirely, so section switching cannot silently no-op at any viewport size. The nav becomes a fixed rail with only the panel scrolling, and the active section gets a heading that labels the panel via aria-labelledby.

This also deletes the now-dead sectionRefs / useEffect / SettingsSectionDivider machinery — net −57 lines in the component.

The URL grammar is unchanged: /settings/:section still deep-links and restores, and Back/Forward still work.

Verification

Driven in a real browser at three viewports. Each tab swaps its rows:

URL heading rows rendered
/settings Appearance Theme, Accent, Text size, High contrast, Motion
/settings/notifications Notifications Device, Messages, Agents, Calls
/settings/connections Connections GitHub
/settings/agents Agents Claude Code, Codex
/settings/about About

At 380px tall the panel now genuinely scrolls (clientH 287 / scrollH 290) with the last row reachable. Mobile keeps its horizontal pill strip.

  • pnpm lint clean, tsc --noEmit clean
  • 857 unit tests pass
  • 120/120 e2e pass

Tests

  • New guard in SettingsSurface.test.tsx asserting inactive sections are absent from the DOM — this is what would have caught the original bug.
  • notificationSettings.test.tsx asserted across sections, so it now opens the section holding the controls under test.

The settings sidebar looked like tabs but was implemented as scroll
anchors: every section rendered at once and clicking a section only
called scrollIntoView. Two things went wrong with that.

The scroll container was never height-constrained. Its parent was a
plain block div, so the child's `flex-1` did nothing and the scroller
sized to its content, leaving scrollHeight === clientHeight — which
makes scrollIntoView a guaranteed no-op. The overflow escaped to the
shell's `overflow-hidden` and was clipped instead, so on a short
window (~420px) the Agents and About sections were unreachable
entirely, with no scrollbar to signal it.

Even with that fixed, the whole surface is only ~680px tall, so at
normal desktop heights everything already fit and clicking a section
still moved nothing. With no section headings in the content column
either, a click produced no feedback at all.

Render only the active section instead. That drops the scroll
dependency, so section switching cannot silently no-op at any viewport
size. The nav is now a fixed rail with only the panel scrolling, and
the active section gets a heading that labels the panel via
aria-labelledby.

Verified in-browser across desktop, short (380px) and mobile widths.

- Add a guard asserting inactive sections are absent from the DOM.
- notificationSettings.test.tsx asserted across sections, so it now
  opens the section holding the controls under test.
@gbasin
gbasin added this pull request to the merge queue Jul 20, 2026
Merged via the queue into master with commit 64d8bf2 Jul 20, 2026
18 checks passed
@gbasin
gbasin deleted the fix/settings-section-tabs branch July 20, 2026 16:00
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.

1 participant