refactor: remove theme section from Settings page#120
Merged
ryota-murakami merged 1 commit intomainfrom Feb 15, 2026
Merged
Conversation
Theme selection was duplicated between the Settings page grid and the sidebar ThemeToggle dropdown. Remove the Settings page theme UI to align with SPEC.md which defines Settings as Display-only. Theme is now exclusively accessed via the sidebar ThemeToggle. - Remove ThemeButton component and theme card grid from SettingsClient - Remove "Change Theme" command from CommandPalette - Rewrite theme E2E tests to use sidebar dropdown instead of Settings - Update loading skeleton to match Display-only layout - Update CLAUDE.md to prefer pnpm e2e:parallel
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughTheme management is migrated from the Settings page to the sidebar ThemeToggle component. The Settings page now focuses exclusively on display preferences (compact mode, card metadata). E2E tests are refactored to interact with the new sidebar-based theme selection, and the "Change Theme" command is removed from the Command Palette. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
==========================================
- Coverage 72.28% 72.21% -0.08%
==========================================
Files 143 143
Lines 4236 4214 -22
Branches 1135 1097 -38
==========================================
- Hits 3062 3043 -19
+ Misses 1153 1150 -3
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/settings/SettingsClient.stories.tsx (1)
26-28:⚠️ Potential issue | 🟡 MinorStale JSDoc — still references "theme selection".
Line 27 says "theme selection and display preferences" but theme selection was removed from this component.
Proposed fix
-/** - * Default settings page showing theme selection and display preferences - */ +/** + * Default settings page showing display preferences + */
🤖 Fix all issues with AI agents
In `@e2e/logged-in/settings.spec.ts`:
- Around line 25-41: In the "should display display settings" test, add a
networkidle wait after navigation so the page is fully loaded before assertions;
replace or augment the existing page.goto('/settings') call in that test with a
networkidle-aware navigation (e.g., call page.goto('/settings', { waitUntil:
'networkidle' }) or call await page.waitForLoadState('networkidle') immediately
after page.goto) so the subsequent checks on displaySection, compactToggle, and
metadataToggle run against a stable loaded page.
In `@e2e/logged-in/theme-visual-application.spec.ts`:
- Around line 51-55: The current check for an open menu (existingMenu via
page.locator('[role="menu"]')) passively waits for it to disappear which can
cause flakiness; change the logic so that if existingMenu.isVisible() resolves
true, actively close it (for example by sending an Escape key via
page.keyboard.press('Escape') or clicking outside via page.click('body')) and
then await expect(existingMenu).not.toBeVisible({ timeout: 3000 }); ensure this
flow is executed only when existingMenu.isVisible() returns true to avoid
unnecessary actions.
🧹 Nitpick comments (1)
e2e/logged-in/theme-visual-application.spec.ts (1)
44-67: Duplicated helper — extract to a shared utility.
selectThemeFromSidebaris duplicated intheme-persistence.spec.ts(lines 16-29), and the two versions diverge: this one has pre-close logic (lines 51-55) that the other lacks. This inconsistency will lead to maintenance drift.Extract to a shared file (e.g.,
e2e/helpers/theme.ts) and import in both specs.
Comment on lines
+25
to
41
| test('should display display settings', async ({ page }) => { | ||
| await page.goto('/settings') | ||
|
|
||
| // Look for theme section | ||
| const themeSection = page.getByText(/theme|appearance/i) | ||
| await expect(themeSection.first()).toBeVisible() | ||
| // Look for display section | ||
| const displaySection = page.getByText(/display/i) | ||
| await expect(displaySection.first()).toBeVisible() | ||
|
|
||
| // Should have theme selection options | ||
| const themeOptions = page.locator( | ||
| '[data-testid="theme-option"], button:has-text("midnight"), button:has-text("sunrise")', | ||
| ) | ||
| await expect(themeOptions.first()).toBeVisible() | ||
| }) | ||
|
|
||
| test('should change theme when selecting option', async ({ page }) => { | ||
| await page.goto('/settings') | ||
|
|
||
| // Get initial body classes | ||
| const initialClasses = await page.locator('html').getAttribute('class') | ||
| // Should have compact mode toggle | ||
| const compactToggle = page.getByRole('switch', { name: /compact mode/i }) | ||
| await expect(compactToggle).toBeVisible() | ||
|
|
||
| // Click a different theme option | ||
| const themeButton = page.locator( | ||
| 'button:has-text("midnight"), button:has-text("graphite"), [data-testid="theme-midnight"]', | ||
| ) | ||
|
|
||
| if (await themeButton.first().isVisible()) { | ||
| await themeButton.first().click() | ||
|
|
||
| // Wait for theme change by verifying class attribute changes | ||
| await expect(async () => { | ||
| const newClasses = await page.locator('html').getAttribute('class') | ||
| expect(newClasses).not.toBe(initialClasses) | ||
| }).toPass({ timeout: 5000 }) | ||
| } | ||
| // Should have show card metadata toggle | ||
| const metadataToggle = page.getByRole('switch', { | ||
| name: /show card metadata/i, | ||
| }) | ||
| await expect(metadataToggle).toBeVisible() | ||
| }) |
There was a problem hiding this comment.
Missing networkidle wait after navigation.
Per coding guidelines, E2E tests should use networkidle wait strategy for page loads. The other theme test files follow this consistently.
Proposed fix
test('should display display settings', async ({ page }) => {
await page.goto('/settings')
+ await page.waitForLoadState('networkidle')As per coding guidelines, "Use networkidle wait strategy for page loads."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('should display display settings', async ({ page }) => { | |
| await page.goto('/settings') | |
| // Look for theme section | |
| const themeSection = page.getByText(/theme|appearance/i) | |
| await expect(themeSection.first()).toBeVisible() | |
| // Look for display section | |
| const displaySection = page.getByText(/display/i) | |
| await expect(displaySection.first()).toBeVisible() | |
| // Should have theme selection options | |
| const themeOptions = page.locator( | |
| '[data-testid="theme-option"], button:has-text("midnight"), button:has-text("sunrise")', | |
| ) | |
| await expect(themeOptions.first()).toBeVisible() | |
| }) | |
| test('should change theme when selecting option', async ({ page }) => { | |
| await page.goto('/settings') | |
| // Get initial body classes | |
| const initialClasses = await page.locator('html').getAttribute('class') | |
| // Should have compact mode toggle | |
| const compactToggle = page.getByRole('switch', { name: /compact mode/i }) | |
| await expect(compactToggle).toBeVisible() | |
| // Click a different theme option | |
| const themeButton = page.locator( | |
| 'button:has-text("midnight"), button:has-text("graphite"), [data-testid="theme-midnight"]', | |
| ) | |
| if (await themeButton.first().isVisible()) { | |
| await themeButton.first().click() | |
| // Wait for theme change by verifying class attribute changes | |
| await expect(async () => { | |
| const newClasses = await page.locator('html').getAttribute('class') | |
| expect(newClasses).not.toBe(initialClasses) | |
| }).toPass({ timeout: 5000 }) | |
| } | |
| // Should have show card metadata toggle | |
| const metadataToggle = page.getByRole('switch', { | |
| name: /show card metadata/i, | |
| }) | |
| await expect(metadataToggle).toBeVisible() | |
| }) | |
| test('should display display settings', async ({ page }) => { | |
| await page.goto('/settings') | |
| await page.waitForLoadState('networkidle') | |
| // Look for display section | |
| const displaySection = page.getByText(/display/i) | |
| await expect(displaySection.first()).toBeVisible() | |
| // Should have compact mode toggle | |
| const compactToggle = page.getByRole('switch', { name: /compact mode/i }) | |
| await expect(compactToggle).toBeVisible() | |
| // Should have show card metadata toggle | |
| const metadataToggle = page.getByRole('switch', { | |
| name: /show card metadata/i, | |
| }) | |
| await expect(metadataToggle).toBeVisible() | |
| }) |
🤖 Prompt for AI Agents
In `@e2e/logged-in/settings.spec.ts` around lines 25 - 41, In the "should display
display settings" test, add a networkidle wait after navigation so the page is
fully loaded before assertions; replace or augment the existing
page.goto('/settings') call in that test with a networkidle-aware navigation
(e.g., call page.goto('/settings', { waitUntil: 'networkidle' }) or call await
page.waitForLoadState('networkidle') immediately after page.goto) so the
subsequent checks on displaySection, compactToggle, and metadataToggle run
against a stable loaded page.
Comment on lines
+51
to
+55
| // Ensure any previously open dropdown is closed before opening a new one | ||
| const existingMenu = page.locator('[role="menu"]') | ||
| if (await existingMenu.isVisible().catch(() => false)) { | ||
| await expect(existingMenu).not.toBeVisible({ timeout: 3000 }) | ||
| } |
There was a problem hiding this comment.
Passive wait won't close an already-open dropdown — potential flakiness.
If a dropdown is already open, this block waits up to 3s for it to disappear on its own but never actively closes it (e.g., via Escape or clicking outside). If the menu stays open, the assertion fails.
Proposed fix
// Ensure any previously open dropdown is closed before opening a new one
const existingMenu = page.locator('[role="menu"]')
if (await existingMenu.isVisible().catch(() => false)) {
- await expect(existingMenu).not.toBeVisible({ timeout: 3000 })
+ await page.keyboard.press('Escape')
+ await expect(existingMenu).not.toBeVisible({ timeout: 3000 })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Ensure any previously open dropdown is closed before opening a new one | |
| const existingMenu = page.locator('[role="menu"]') | |
| if (await existingMenu.isVisible().catch(() => false)) { | |
| await expect(existingMenu).not.toBeVisible({ timeout: 3000 }) | |
| } | |
| // Ensure any previously open dropdown is closed before opening a new one | |
| const existingMenu = page.locator('[role="menu"]') | |
| if (await existingMenu.isVisible().catch(() => false)) { | |
| await page.keyboard.press('Escape') | |
| await expect(existingMenu).not.toBeVisible({ timeout: 3000 }) | |
| } |
🤖 Prompt for AI Agents
In `@e2e/logged-in/theme-visual-application.spec.ts` around lines 51 - 55, The
current check for an open menu (existingMenu via page.locator('[role="menu"]'))
passively waits for it to disappear which can cause flakiness; change the logic
so that if existingMenu.isVisible() resolves true, actively close it (for
example by sending an Escape key via page.keyboard.press('Escape') or clicking
outside via page.click('body')) and then await
expect(existingMenu).not.toBeVisible({ timeout: 3000 }); ensure this flow is
executed only when existingMenu.isVisible() returns true to avoid unnecessary
actions.
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
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
CLAUDE.mdto preferpnpm e2e:paralleloverpnpm e2eAligns Settings page with SPEC.md definition (Display-only, theme via Sidebar).
Files Changed (9)
src/app/settings/SettingsClient.tsxsrc/app/settings/loading.tsxsrc/app/settings/SettingsClient.stories.tsxsrc/components/CommandPalette/CommandPalette.tsxsrc/tests/unit/.../CommandPalette.test.tsxe2e/logged-in/settings.spec.tse2e/logged-in/theme-persistence.spec.tse2e/logged-in/theme-visual-application.spec.tsCLAUDE.mdpnpm e2e:parallelfor E2ETest plan
pnpm typecheck— cleanpnpm lint— 0 warningspnpm test— 1282 unit tests passpnpm build— successpnpm e2e— 276 pass, 0 failSummary by CodeRabbit
New Features
Bug Fixes/Changes
Tests