Repository navigation
[codex] Add full sidebar visibility controls - #739
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The PR implements full sidebar visibility controls with:
The implementation is clean, type-safe, and follows existing patterns in the codebase. Files Reviewed (7 files)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature allowing users to customize sidebar visibility by hiding specific navigation items. Key changes include a new UI section in the Appearance settings tab, real-time sidebar updates using custom window events, and updated validation schemas and internationalization files. The review feedback highlights several improvement opportunities: addressing a potential stale closure in a useEffect hook within the AppearanceTab component, memoizing helper functions and computed values using useCallback and useMemo to prevent unnecessary re-renders, and optimizing the sidebar item normalization logic for better performance.
| @@ -25,6 +43,7 @@ export default function AppearanceTab() { | |||
| }); | |||
| return unsubscribe; | |||
| }, []); // eslint-disable-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
The useEffect hook at line 38 has an empty dependency array, which can lead to a stale closure over the customThemeColor state variable. This can cause incorrect behavior when the theme color is updated from another source. To fix this, you should add customThemeColor to the dependency array to ensure the subscription callback always has the latest value.
| }, []); // eslint-disable-line react-hooks/exhaustive-deps | |
| }, [customThemeColor]); |
| const getSettingsLabel = (key: string, fallback: string) => | ||
| typeof t.has === "function" && t.has(key) ? t(key) : fallback; | ||
| const getSidebarLabel = (key: string, fallback: string) => | ||
| typeof tSidebar.has === "function" && tSidebar.has(key) ? tSidebar(key) : fallback; |
There was a problem hiding this comment.
The getSettingsLabel and getSidebarLabel helper functions are re-created on every render. This can lead to unnecessary re-renders of child components if these functions are passed as props. It's a good practice to memoize them using useCallback to improve performance.
Remember to import useCallback from react.
const getSettingsLabel = useCallback((key: string, fallback: string) =>
typeof t.has === "function" && t.has(key) ? t(key) : fallback,
[t]);
const getSidebarLabel = useCallback((key: string, fallback: string) =>
typeof tSidebar.has === "function" && tSidebar.has(key) ? tSidebar(key) : fallback,
[tSidebar]);
| const getSidebarLabel = (key: string, fallback: string) => | ||
| typeof t.has === "function" && t.has(key) ? t(key) : fallback; | ||
|
|
||
| const hiddenSidebarSet = new Set(hiddenSidebarItems); | ||
| const visibleSections = SIDEBAR_SECTIONS.filter( | ||
| (section) => section.visibility !== "debug" || showDebug | ||
| ) | ||
| .map((section) => ({ | ||
| ...section, | ||
| title: getSidebarLabel(section.titleKey, section.titleFallback), | ||
| items: section.items | ||
| .map((item) => ({ ...item, label: t(item.i18nKey) })) | ||
| .filter((item) => !hiddenSidebarSet.has(item.id)), | ||
| })) | ||
| .filter((section) => section.items.length > 0); |
There was a problem hiding this comment.
The getSidebarLabel function is recreated on every render, and visibleSections is recalculated on every render. This can impact performance. It's recommended to memoize both using useCallback and useMemo respectively. This will prevent re-computation unless their dependencies change.
Remember to import useCallback and useMemo from react.
const getSidebarLabel = useCallback((key: string, fallback: string) =>
typeof t.has === "function" && t.has(key) ? t(key) : fallback,
[t]);
const visibleSections = useMemo(() => {
const hiddenSidebarSet = new Set(hiddenSidebarItems);
return SIDEBAR_SECTIONS.filter(
(section) => section.visibility !== "debug" || showDebug
)
.map((section) => ({
...section,
title: getSidebarLabel(section.titleKey, section.titleFallback),
items: section.items
.map((item) => ({ ...item, label: t(item.i18nKey) }))
.filter((item) => !hiddenSidebarSet.has(item.id)),
}))
.filter((section) => section.items.length > 0);
}, [hiddenSidebarItems, showDebug, getSidebarLabel, t]);
| export function normalizeHiddenSidebarItems(value: unknown): HideableSidebarItemId[] { | ||
| if (!Array.isArray(value)) return []; | ||
|
|
||
| const hiddenItems = new Set<HideableSidebarItemId>(); | ||
|
|
||
| for (const item of value) { | ||
| if ( | ||
| typeof item === "string" && | ||
| HIDEABLE_SIDEBAR_ITEM_IDS.includes(item as HideableSidebarItemId) | ||
| ) { | ||
| hiddenItems.add(item as HideableSidebarItemId); | ||
| } | ||
| } | ||
|
|
||
| return HIDEABLE_SIDEBAR_ITEM_IDS.filter((item) => hiddenItems.has(item)); | ||
| } |
There was a problem hiding this comment.
The current implementation of normalizeHiddenSidebarItems uses Array.prototype.includes inside a loop, which has a time complexity of O(N*M). For better performance, especially if the list of hidden items grows, you can convert HIDEABLE_SIDEBAR_ITEM_IDS to a Set for O(1) lookups.
export function normalizeHiddenSidebarItems(value: unknown): HideableSidebarItemId[] {
if (!Array.isArray(value)) return [];
const validIds = new Set(HIDEABLE_SIDEBAR_ITEM_IDS);
const hiddenItems = new Set<HideableSidebarItemId>();
for (const item of value) {
if (typeof item === "string" && validIds.has(item as HideableSidebarItemId)) {
hiddenItems.add(item as HideableSidebarItemId);
}
}
return HIDEABLE_SIDEBAR_ITEM_IDS.filter((item) => hiddenItems.has(item));
}There was a problem hiding this comment.
Pull request overview
Adds full per-item sidebar visibility controls across all sidebar sections, centralizing sidebar metadata and persisting hidden items in settings. This expands the previous partial implementation to cover every existing sidebar entry and auto-hides empty sections.
Changes:
- Introduces a centralized sidebar definition/visibility module (
SIDEBAR_SECTIONS, hideable IDs, normalization helper). - Updates Sidebar rendering to filter items/sections based on persisted
hiddenSidebarItemsand a settings-updated event. - Adds Appearance UI to toggle visibility for every sidebar entry; updates settings types, validation, defaults, i18n, and docs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/settings.ts | Adds hiddenSidebarItems / hideHealthCheckLogs to settings typing. |
| src/shared/validation/settingsSchemas.ts | Validates hiddenSidebarItems in the /api/settings PATCH schema. |
| src/shared/validation/schemas.ts | Mirrors hiddenSidebarItems validation in the shared schema barrel. |
| src/shared/constants/sidebarVisibility.ts | Centralizes sidebar section/item metadata and normalization logic. |
| src/shared/components/Sidebar.tsx | Renders sections from centralized metadata; filters hidden items and auto-hides empty sections. |
| src/lib/db/settings.ts | Adds default hiddenSidebarItems: [] in settings read defaults. |
| src/i18n/messages/en.json | Adds sidebar section labels and settings strings for visibility UI. |
| src/i18n/messages/zh-CN.json | Same as en.json for zh-CN. |
| src/domain/types.ts | Documents hiddenSidebarItems in AppSettings JSDoc. |
| src/app/(dashboard)/dashboard/settings/components/AppearanceTab.tsx | Adds UI to toggle hidden sidebar items and broadcasts updates to Sidebar. |
| docs/USER_GUIDE.md | Updates settings tab count/table to reflect “General” and sidebar visibility controls. |
Comments suppressed due to low confidence (1)
src/shared/validation/settingsSchemas.ts:35
fallbackStrategyin this settings schema does not include the supported value"strict-random"(it is present in the broader schema and strategy constants). Since/api/settingsuses this schema for PATCH validation, attempts to setstrict-randomwill be rejected. Add"strict-random"here (or reference the sharedSETTINGS_FALLBACK_STRATEGY_VALUES) to keep validation consistent.
hideHealthCheckLogs: z.boolean().optional(),
hiddenSidebarItems: z.array(z.enum(HIDEABLE_SIDEBAR_ITEM_IDS)).optional(),
// Routing settings (#134)
fallbackStrategy: z
.enum(["fill-first", "round-robin", "p2c", "random", "least-used", "cost-optimized"])
.optional(),
wildcardAliases: z.array(z.object({ pattern: z.string(), target: z.string() })).optional(),
stickyRoundRobinLimit: z.number().int().min(0).max(1000).optional(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {collapsed && showTitle && ( | ||
| <div className="border-t border-black/5 dark:border-white/5 mb-2" /> | ||
| )} | ||
| {section.items.map(renderNavLink)} |
There was a problem hiding this comment.
The new section wrapper <div key={section.id}> makes the sidebar links no longer direct children of the <nav> that has space-y-1, so vertical spacing between individual links will disappear within each section. Add spacing on the wrapper or an inner container (e.g., apply space-y-1 to the section’s items container) to preserve the original layout.
| {section.items.map(renderNavLink)} | |
| <div className="space-y-1"> | |
| {section.items.map(renderNavLink)} | |
| </div> |
| const [hiddenSidebarItems, setHiddenSidebarItems] = useState<string[]>([]); | ||
|
|
||
| // Check if debug mode is enabled | ||
| useEffect(() => { | ||
| const applySettings = (data) => { | ||
| setShowDebug(data?.enableRequestLogs === true); | ||
| setHiddenSidebarItems(normalizeHiddenSidebarItems(data?.[HIDDEN_SIDEBAR_ITEMS_SETTING_KEY])); | ||
| }; |
There was a problem hiding this comment.
hiddenSidebarItems state is typed as string[], but it only ever contains normalized HideableSidebarItemId values. Consider typing this state as HideableSidebarItemId[] to keep the Set/filter logic type-safe and prevent accidentally storing arbitrary strings later.
| "sidebarVisibility": "Hide sidebar items", | ||
| "sidebarVisibilityDesc": "Hide any sidebar navigation entry to reduce visual clutter without disabling any features", | ||
| "sidebarVisibilityHint": "Any sidebar section is hidden automatically when all of its entries are hidden", |
There was a problem hiding this comment.
New i18n keys were added here (and under sidebar) but not to the other locale bundles in src/i18n/messages/*.json (e.g., ar/pt-BR have no primarySection/sidebarVisibility* keys). If the repo expects per-locale message files to stay key-synchronized, please add these keys to all locales (even if using English placeholders) to avoid missing-string fallbacks and future runtime errors where t() is used without has checks.
| "sidebarVisibility": "Hide sidebar items", | |
| "sidebarVisibilityDesc": "Hide any sidebar navigation entry to reduce visual clutter without disabling any features", | |
| "sidebarVisibilityHint": "Any sidebar section is hidden automatically when all of its entries are hidden", |
|
Thanks @rdself for this great contribution! 🎉 The feature is now merged and will be part of the next release. We appreciate your effort! |
Summary
User Impact
Validation
npm run typecheck:corenpx eslint 'src/shared/constants/sidebarVisibility.ts' 'src/shared/components/Sidebar.tsx' 'src/app/(dashboard)/dashboard/settings/components/AppearanceTab.tsx' 'src/domain/types.ts'npx prettier --check 'src/shared/constants/sidebarVisibility.ts' 'src/shared/components/Sidebar.tsx' 'src/app/(dashboard)/dashboard/settings/components/AppearanceTab.tsx' 'src/i18n/messages/en.json' 'src/i18n/messages/zh-CN.json' 'src/domain/types.ts' 'docs/USER_GUIDE.md'npm run test:unit(1061 passed, 0 failed)