Skip to content

fix(web): highlight only the active settings section - #10524

Closed
Gigioxx wants to merge 1 commit into
pingdotgg:mainfrom
Gigioxx:t3code/fix-behavior-tab-selection
Closed

fix(web): highlight only the active settings section#10524
Gigioxx wants to merge 1 commit into
pingdotgg:mainfrom
Gigioxx:t3code/fix-behavior-tab-selection

Conversation

@Gigioxx

@Gigioxx Gigioxx commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Clicking a nested settings link could leave Behavior and other nearby sections highlighted. The sidebar treated every visible section as selected, and jumps centered the destination heading.

Select one visible section, preferring the clicked destination while it remains in view and falling back to the first visible section when scrolling away. Section links now align their headings at the top; search results retain centered jumps. The existing observer and route scoping stay in place, with aria-current identifying the active section.

Verified 13 focused tests, web typecheck, and scoped lint/formatting. The selection and alignment regressions fail against the original code. Lint retains the existing search-result effect warning.

Chromium checks covered all 15 links across General, Appearance, Connections, and Source Control; manual scrolling; sectioned/unsectioned route changes; centered search; and the sidebar at phone width. Normal and reduced motion both passed. Web and desktop share this sidebar; native Electron and React Native were not exercised. No server, provider, or connection contracts changed.

Before: three sections highlighted After: only the clicked section highlighted
Before clicking Projects and threads After clicking Projects and threads

Interaction recordings: before · after.

Model: GPT-6. Harness: Codex in T3 Code.

Note

Fix SettingsSidebarNav to highlight only the active settings section

  • Replaces the visible-section-set API with a scalar active-section API in settingsSectionVisibility.ts, preferring the most recently clicked visible section and falling back to the first visible target
  • Sidebar submenu buttons now use aria-current and active text styling for only the selected section instead of a data-visible attribute for every visible section
  • Non-highlighted settings targets scroll into view with start alignment instead of center alignment in settingsLayout.tsx; highlighted targets keep center alignment
  • Behavioral Change: getActiveSettingsSectionId now returns a single section ID or undefined instead of a set of visible section IDs; callers that depended on the set API need updating

Macroscope summarized dc3f6a6.

Summary by CodeRabbit

  • Improvements
    • Settings navigation now clearly indicates the currently active section.
    • Selecting a settings section keeps that destination highlighted during the current visit when available.
    • Navigating to settings targets without highlighting now positions the destination at the top of the viewport, while highlighted targets remain centered.
    • Settings section selection now falls back smoothly when the preferred destination is unavailable or hidden.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 7, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at dc3f6a6

Macroscope's review found this PR approvable — The PR is a focused settings-sidebar bug fix that selects one active section and adjusts only the alignment of non-highlighted jumps, with targeted regression tests. It does not alter product defaults, static-analysis settings, schemas, or sensitive production areas.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: de63d46e-89a6-4594-aef8-7f05a0e1eda8

📥 Commits

Reviewing files that changed from the base of the PR and between 8b2838e and dc3f6a6.

📒 Files selected for processing (5)
  • apps/web/src/components/settings/SettingsSidebarNav.tsx
  • apps/web/src/components/settings/settingsLayout.test.tsx
  • apps/web/src/components/settings/settingsLayout.tsx
  • apps/web/src/components/settings/settingsSectionVisibility.test.ts
  • apps/web/src/components/settings/settingsSectionVisibility.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The settings sidebar now tracks one active section based on visibility and recent clicks. Settings target scrolling uses viewport-start alignment when highlighting is disabled. Tests cover active-section fallback and both scroll modes.

Changes

Settings navigation and scroll behavior

Layer / File(s) Summary
Active section selection
apps/web/src/components/settings/settingsSectionVisibility.ts, apps/web/src/components/settings/settingsSectionVisibility.test.ts
getActiveSettingsSectionId returns one visible section, prefers a matching clicked target, and returns undefined when no target applies.
Sidebar active state
apps/web/src/components/settings/SettingsSidebarNav.tsx
The sidebar stores the preferred clicked section and marks the active submenu item with aria-current="location".
Scroll target alignment
apps/web/src/components/settings/settingsLayout.tsx, apps/web/src/components/settings/settingsLayout.test.tsx
Highlighted targets scroll to the center. Non-highlighted targets scroll to the viewport start. Tests cover both modes.

Estimated code review effort: 2 (Simple) | ~15 minutes

Merge Risk: ⚪ Minimal · up to dc3f6

The settings sidebar now presents one active visible section and adjusts section-link scrolling while preserving centered search-result jumps. No current merge-blocking risk is identified.

Suggested reviewers: juliusmarminge, shivamhwp, maria-rcks

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: highlighting only the active settings section.
Description check ✅ Passed The description explains what changed, why it changed, UI behavior, verification results, scope, and limitations. It includes screenshots and interaction recordings for the UI changes. The template he…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #10705 — settings section submenu/highlighting was removed. Closing in favor of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants