feat(settings): group settings menu into labeled sections#1286
Merged
graycyrus merged 3 commits intotinyhumansai:mainfrom May 7, 2026
Merged
feat(settings): group settings menu into labeled sections#1286graycyrus merged 3 commits intotinyhumansai:mainfrom
graycyrus merged 3 commits intotinyhumansai:mainfrom
Conversation
Restructure the flat SettingsHome list into six visually separated sections (General, Features & AI, Billing & Rewards, Support, Advanced, Danger Zone) with lightweight section headers. Adds a Rewards item that navigates to /rewards. Closes tinyhumansai#1277
Contributor
📝 WalkthroughWalkthroughSettingsHome component refactored from a flat, hard-coded menu structure to a data-driven, grouped architecture with internal type definitions, configuration arrays (settingsSections, destructiveItems), updated rendering logic, and comprehensive test coverage validating section headers, item ordering, and navigation behavior. ChangesSettings Menu Reorganization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Add click tests for Notification Routing, Features, AI & Models, Billing & Usage, About, and Developer Options menu items.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/settings/__tests__/SettingsHome.test.tsx (1)
186-259: 💤 Low valueConsider adding truthy assertions before non-null assertions for clearer failure diagnostics.
Most click tests use
.closest('button')!without a preceding assertion (unlike line 179 which correctly checksexpect(rewardsButton).toBeTruthy()). If the DOM structure changes andclosestreturnsnull, the runtime error will be less informative than an explicit assertion failure.♻️ Example improvement for one test
it('navigates to account settings when Account is clicked', async () => { const user = userEvent.setup(); renderSettingsHome(); - await user.click(screen.getByText('Account').closest('button')!); + const accountButton = screen.getByText('Account').closest('button'); + expect(accountButton).toBeTruthy(); + await user.click(accountButton!); expect(mockNavigateToSettings).toHaveBeenCalledWith('account'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/settings/__tests__/SettingsHome.test.tsx` around lines 186 - 259, Update each test that uses .closest('button')! to first capture the element (e.g., const btn = screen.getByText('...').closest('button')), assert it is truthy with expect(btn).toBeTruthy(), and only then call await user.click(btn!); apply this pattern to tests referencing labels 'Account', 'Notifications', 'Restart Tour', 'Notification Routing', 'Features', 'AI & Models', 'Billing & Usage', 'About', and 'Developer Options' (helpers: renderSettingsHome, userEvent.setup, and assertions checking mockNavigateToSettings/mockNavigate/openUrl remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/components/settings/__tests__/SettingsHome.test.tsx`:
- Around line 186-259: Update each test that uses .closest('button')! to first
capture the element (e.g., const btn =
screen.getByText('...').closest('button')), assert it is truthy with
expect(btn).toBeTruthy(), and only then call await user.click(btn!); apply this
pattern to tests referencing labels 'Account', 'Notifications', 'Restart Tour',
'Notification Routing', 'Features', 'AI & Models', 'Billing & Usage', 'About',
and 'Developer Options' (helpers: renderSettingsHome, userEvent.setup, and
assertions checking mockNavigateToSettings/mockNavigate/openUrl remain
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80dc0e53-7767-4ee4-804a-a2f579d4d57c
📒 Files selected for processing (2)
app/src/components/settings/SettingsHome.tsxapp/src/components/settings/__tests__/SettingsHome.test.tsx
senamakel
requested changes
May 6, 2026
Member
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
resolve merge conflicts bro
… Routing moved to Dev Options) Remove Notification Routing from top-level General section since upstream moved it into Developer Options. Keep grouped section structure.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/components/settings/__tests__/SettingsHome.test.tsx (1)
169-176: ⚡ Quick winUse role-based button queries instead of
.closest('button').These clicks are currently tied to DOM structure. Querying by role/name keeps tests behavior-first and less brittle.
Example refactor pattern
-const rewardsButton = screen.getByText('Rewards').closest('button'); -expect(rewardsButton).toBeTruthy(); -await user.click(rewardsButton!); +await user.click(screen.getByRole('button', { name: /rewards/i }));As per coding guidelines "Prefer testing behavior over implementation details; use existing helpers from
app/src/test/(test-utils.tsx, shared mock backend) before adding new harness code; keep tests deterministic..."Also applies to: 183-189, 191-197, 199-205, 207-213, 215-221, 223-230, 232-238, 240-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/settings/__tests__/SettingsHome.test.tsx` around lines 169 - 176, The test uses DOM-structure dependent queries (screen.getByText(...).closest('button')) in SettingsHome.test.tsx; replace those with role-based queries like screen.getByRole('button', { name: /Rewards/i }) to click by accessible name and avoid brittle implementation details—update the click targets in the 'navigates to /rewards when clicked' test (and the other similar blocks referenced) to use user.click(screen.getByRole('button', { name: /.../i })) and keep using renderSettingsHome() and userEvent.setup() as before so the tests remain behavior-first and deterministic.app/src/components/settings/SettingsHome.tsx (2)
33-39: ⚡ Quick winPrefer semantic heading markup for section labels.
Using a heading element (
h2with existing styles) improves screen-reader navigation versus a styledspan.Proposed change
const SectionHeader = ({ label }: { label: string }) => ( <div className="px-4 pt-5 pb-1"> - <span className="text-[10px] font-semibold tracking-widest uppercase text-stone-400"> + <h2 className="text-[10px] font-semibold tracking-widest uppercase text-stone-400"> {label} - </span> + </h2> </div> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/settings/SettingsHome.tsx` around lines 33 - 39, Replace the non-semantic span in the SectionHeader component with a proper heading element (e.g., h2) while preserving the same className and the {label} content; update the component definition SectionHeader to render an h2 with the existing classes ("text-[10px] font-semibold tracking-widest uppercase text-stone-400") and container div structure so visual styling stays identical but semantic markup and screen-reader behavior are improved.
1-1: ⚡ Quick winMove
ReactNodeto type-only import.
ReactNodeappears only in theSettingsIteminterface (line 27) as a type annotation, so it should useimport typeper repo TypeScript guidelines.Proposed change
-import { ReactNode, useState } from 'react'; +import { useState } from 'react'; +import type { ReactNode } from 'react';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/settings/SettingsHome.tsx` at line 1, The import of ReactNode is only used as a type in the SettingsItem interface, so change the statement to a type-only import: replace the current import that includes ReactNode with an import type for ReactNode while keeping useState as a regular import; update the import line that currently reads importing ReactNode and useState so that ReactNode is imported via "import type { ReactNode }" and useState remains a runtime import to avoid pulling ReactNode into emitted JS and satisfy the repo TypeScript guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/components/settings/__tests__/SettingsHome.test.tsx`:
- Around line 169-176: The test uses DOM-structure dependent queries
(screen.getByText(...).closest('button')) in SettingsHome.test.tsx; replace
those with role-based queries like screen.getByRole('button', { name: /Rewards/i
}) to click by accessible name and avoid brittle implementation details—update
the click targets in the 'navigates to /rewards when clicked' test (and the
other similar blocks referenced) to use user.click(screen.getByRole('button', {
name: /.../i })) and keep using renderSettingsHome() and userEvent.setup() as
before so the tests remain behavior-first and deterministic.
In `@app/src/components/settings/SettingsHome.tsx`:
- Around line 33-39: Replace the non-semantic span in the SectionHeader
component with a proper heading element (e.g., h2) while preserving the same
className and the {label} content; update the component definition SectionHeader
to render an h2 with the existing classes ("text-[10px] font-semibold
tracking-widest uppercase text-stone-400") and container div structure so visual
styling stays identical but semantic markup and screen-reader behavior are
improved.
- Line 1: The import of ReactNode is only used as a type in the SettingsItem
interface, so change the statement to a type-only import: replace the current
import that includes ReactNode with an import type for ReactNode while keeping
useState as a regular import; update the import line that currently reads
importing ReactNode and useState so that ReactNode is imported via "import type
{ ReactNode }" and useState remains a runtime import to avoid pulling ReactNode
into emitted JS and satisfy the repo TypeScript guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c86895e5-6379-446e-9a23-275f6985161c
📒 Files selected for processing (2)
app/src/components/settings/SettingsHome.tsxapp/src/components/settings/__tests__/SettingsHome.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SettingsHomeflat list into six visually separated sections with labeled headers: General, Features & AI, Billing & Rewards, Support, Advanced, Danger Zone/rewards) under Billing & Rewards, so Rewards is accessible from SettingsChanges
app/src/components/settings/SettingsHome.tsxSectionHeadercomponentapp/src/components/settings/__tests__/SettingsHome.test.tsxTest plan
pnpm debug unit)pnpm typecheck— passpnpm lint— 0 errors (32 pre-existing warnings in unrelated files)pnpm format:check— passpnpm build— passCloses #1277
Summary by CodeRabbit