fix(profile): hide unavailable sidebar settings#5042
Open
Cokefish9527 wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThe PR introduces admin-controlled sidebar module visibility by extracting filtering logic into a reusable library and integrating it into the sidebar modules card component. New types define the configuration structure, a filter function applies admin settings to sections and modules, and the component uses memoized filtered definitions throughout config loading, reset, and rendering workflows. ChangesAdmin-controlled sidebar module visibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@web/default/src/features/profile/components/sidebar-modules-card.tsx`:
- Around line 148-152: The effect calling loadConfig currently depends on
visibleSectionDefs and thus refetches and overwrites local config when
visibility toggles; make loadConfig stable (wrap it in useCallback without
referencing visibleSectionDefs) and remove visibleSectionDefs from the useEffect
dependency array so useEffect(() => { void Promise.resolve().then(() =>
loadConfig()) }, [loadConfig]) does not retrigger on visibility changes; then
add a separate effect that merges visibleSectionDefs into the local config state
(e.g., updateConfig or setConfig) without clobbering unsaved user edits,
referencing visibleSectionDefs, config, and the state updater to reconcile
visibility changes.
In `@web/default/src/features/profile/lib/sidebar-modules.ts`:
- Around line 33-57: The current logic lets malformed but non-empty
adminConfigValue fall back to returning the user's sections (fail-open). Change
behavior so that when adminConfigValue is present (non-null/non-empty string)
but parseAdminConfig returns null (parsing failed), treat that as an explicit
"no modules allowed" (fail-closed): in filterSidebarSectionDefsByAdminConfig,
check if adminConfigValue && adminConfig == null and in that case return an
empty array (or filter out all modules) instead of returning the original
sections; alternatively modify parseAdminConfig to return a distinct
error/marker for invalid input and handle that marker in
filterSidebarSectionDefsByAdminConfig to enforce the closed behavior. Ensure you
reference parseAdminConfig, filterSidebarSectionDefsByAdminConfig and the
adminConfigValue variable when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee381681-91b6-492d-8352-3cd08da0b9b3
📒 Files selected for processing (2)
web/default/src/features/profile/components/sidebar-modules-card.tsxweb/default/src/features/profile/lib/sidebar-modules.ts
Comment on lines
+148
to
152
| }, [visibleSectionDefs]) | ||
|
|
||
| useEffect(() => { | ||
| loadConfig() | ||
| void Promise.resolve().then(() => loadConfig()) | ||
| }, [loadConfig]) |
Contributor
There was a problem hiding this comment.
Avoid refetch-triggered local state overwrite during edits.
Line 148 ties loadConfig to visibleSectionDefs, so language/status changes retrigger Line 151 load and can overwrite unsaved user toggles in config.
Suggested direction
-import { useCallback, useEffect, useMemo, useState } from 'react'
+import { useCallback, useEffect, useMemo, useRef, useState } from 'react'
@@
+ const hasLoadedRef = useRef(false)
@@
useEffect(() => {
- void Promise.resolve().then(() => loadConfig())
+ if (hasLoadedRef.current) return
+ hasLoadedRef.current = true
+ void loadConfig()
}, [loadConfig])Then reconcile visibility changes in a separate state-merge path instead of reloading from the API.
📝 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
| }, [visibleSectionDefs]) | |
| useEffect(() => { | |
| loadConfig() | |
| void Promise.resolve().then(() => loadConfig()) | |
| }, [loadConfig]) | |
| }, [visibleSectionDefs]) | |
| const hasLoadedRef = useRef(false) | |
| useEffect(() => { | |
| if (hasLoadedRef.current) return | |
| hasLoadedRef.current = true | |
| void loadConfig() | |
| }, [loadConfig]) |
🤖 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 `@web/default/src/features/profile/components/sidebar-modules-card.tsx` around
lines 148 - 152, The effect calling loadConfig currently depends on
visibleSectionDefs and thus refetches and overwrites local config when
visibility toggles; make loadConfig stable (wrap it in useCallback without
referencing visibleSectionDefs) and remove visibleSectionDefs from the useEffect
dependency array so useEffect(() => { void Promise.resolve().then(() =>
loadConfig()) }, [loadConfig]) does not retrigger on visibility changes; then
add a separate effect that merges visibleSectionDefs into the local config state
(e.g., updateConfig or setConfig) without clobbering unsaved user edits,
referencing visibleSectionDefs, config, and the state updater to reconcile
visibility changes.
Comment on lines
+33
to
+57
| const parseAdminConfig = ( | ||
| adminConfigValue: string | null | undefined | ||
| ): SidebarModulesConfig | null => { | ||
| if (!adminConfigValue || adminConfigValue.trim() === '') return null | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(adminConfigValue) as unknown | ||
| if (!parsed || typeof parsed !== 'object') return null | ||
| return parsed as SidebarModulesConfig | ||
| } catch { | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Limits the user's personal sidebar settings to modules still allowed by the | ||
| * administrator-wide sidebar configuration. | ||
| */ | ||
| export function filterSidebarSectionDefsByAdminConfig( | ||
| sections: SidebarSectionDef[], | ||
| adminConfigValue: string | null | undefined | ||
| ): SidebarSectionDef[] { | ||
| const adminConfig = parseAdminConfig(adminConfigValue) | ||
| if (!adminConfig) return sections | ||
|
|
Contributor
There was a problem hiding this comment.
Fail-open behavior on invalid admin JSON re-exposes restricted toggles.
At Line 56, malformed but non-empty admin config falls back to sections unchanged, which can show modules that admins intended to disable. This should fail closed when config is present but invalid.
Suggested fix
-const parseAdminConfig = (
+const parseAdminConfig = (
adminConfigValue: string | null | undefined
-): SidebarModulesConfig | null => {
- if (!adminConfigValue || adminConfigValue.trim() === '') return null
+): SidebarModulesConfig | 'absent' | 'invalid' => {
+ if (!adminConfigValue || adminConfigValue.trim() === '') return 'absent'
try {
const parsed = JSON.parse(adminConfigValue) as unknown
- if (!parsed || typeof parsed !== 'object') return null
+ if (!parsed || typeof parsed !== 'object') return 'invalid'
return parsed as SidebarModulesConfig
} catch {
- return null
+ return 'invalid'
}
}
@@
const adminConfig = parseAdminConfig(adminConfigValue)
- if (!adminConfig) return sections
+ if (adminConfig === 'absent') return sections
+ if (adminConfig === 'invalid') return []🤖 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 `@web/default/src/features/profile/lib/sidebar-modules.ts` around lines 33 -
57, The current logic lets malformed but non-empty adminConfigValue fall back to
returning the user's sections (fail-open). Change behavior so that when
adminConfigValue is present (non-null/non-empty string) but parseAdminConfig
returns null (parsing failed), treat that as an explicit "no modules allowed"
(fail-closed): in filterSidebarSectionDefsByAdminConfig, check if
adminConfigValue && adminConfig == null and in that case return an empty array
(or filter out all modules) instead of returning the original sections;
alternatively modify parseAdminConfig to return a distinct error/marker for
invalid input and handle that marker in filterSidebarSectionDefsByAdminConfig to
enforce the closed behavior. Ensure you reference parseAdminConfig,
filterSidebarSectionDefsByAdminConfig and the adminConfigValue variable when
making the change.
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
1 participant
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.
Important
📝 变更描述 / Description
修复普通用户个人资料页“左侧边栏个人设置”展示范围不跟随管理员设置的问题。
原先
SidebarModulesCard使用固定配置渲染用户可调整的侧边栏区域和模块，没有读取管理员配置SidebarModulesAdmin。因此管理员关闭“聊天区域”“游乐场”或“聊天”后，普通用户个人中心仍能看到对应开关。本次新增用户个人设置展示过滤逻辑：先按管理员配置过滤可展示的区域和子模块，再用于渲染、默认初始化和重置。普通用户只能进一步隐藏管理员已开放的模块，不能看到或恢复管理员未开放的模块。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
验证已通过：
node --input-type=module -e "...filterSidebarSectionDefsByAdminConfig assertions..."node node_modules/eslint/bin/eslint.js src/features/profile/components/sidebar-modules-card.tsx src/features/profile/lib/sidebar-modules.tsnpm run typechecknpm run buildgit diff --cached --check说明：本 PR 不提交测试文件；已用内联 Node 断言覆盖管理员关闭整个聊天区域、仅关闭游乐场子模块两类关键逻辑。
Summary by CodeRabbit
New Features
Refactor