Skip to content

fix: resolve #347 - 侧边栏用户信息优化#383

Closed
awsl233777 wants to merge 6 commits intomainfrom
fix/issue-347
Closed

fix: resolve #347 - 侧边栏用户信息优化#383
awsl233777 wants to merge 6 commits intomainfrom
fix/issue-347

Conversation

@awsl233777
Copy link
Collaborator

@awsl233777 awsl233777 commented Mar 9, 2026

Closes #347

Summary by CodeRabbit

  • 新功能
    • 新增账户概览与安全中心对话框;菜单中加入切换账号、管理密钥、修改密码、打开设置与退出应用等操作。
  • 改进
    • 统一并增强用户身份显示:脱敏标识、租户标签、角色、头像回退与状态徽章;头部与侧栏在折叠/展开时展示优化,菜单布局与交互更直观;新增导航辅助以改善页面跳转体验。
  • 本地化
    • 增补中/英两版多项账号与安全相关翻译键。
  • 修复
    • 解决在用户信息缺失时可能导致的运行时错误。
  • 测试
    • 更新端到端测试以覆盖安全中心与密钥管理的导航与选择流程。

@coderabbitai
Copy link

coderabbitai bot commented Mar 9, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

新增集中化的账户身份显示库并在页头与侧边栏使用;侧边栏用户菜单大幅重构,增加账户概览与安全中心(含 passkeys 管理);修复 admin-route 对未定义 user 的访问;新增中/英本地化词条与相关 e2e 调整。

Changes

Cohort / File(s) Summary
身份显示工具库
web/src/lib/auth-user-display.ts
新增模块,包含 maskAccountIdentifierAuthUserDisplay 接口与 getAuthUserDisplay,封装脱敏、tenant/role/label 及头像回退逻辑。
侧边栏用户菜单重构
web/src/components/layout/app-sidebar/nav-user.tsx
大幅重构:改为使用 getAuthUserDisplay,新增账户概览对话框与安全中心子菜单(含 managePasskeys/changePassword)、切换/退出操作、桌面 Quit 支持与导航 helpers,重做折叠/展开时的头像/状态渲染与菜单布局。
页面头部账户显示
web/src/components/layout/page-header.tsx
改为通过 getAuthUserDisplay(user) 获取显示数据;展示脱敏身份与响应式 tenant 标签,并更新 title/显示字段。
路由守卫修复
web/src/components/auth/admin-route.tsx
将直接访问 user.role 改为安全访问 user?.role,避免 user 为 undefined 时抛错,行为其余保持不变。
国际化文本
web/src/locales/en.json, web/src/locales/zh.json
nav.account 下新增多项翻译键(accountOverview、securityCenter、switchAccount、exitApp、authDisabled、currentIdentity、tenant、role、sensitiveMasked、openSettings、manageUsers)。
端到端测试调整
tests/e2e/playwright/test-passkey-discoverable.mjs
在进入 Passkeys 管理前先打开 Security Center 菜单项,放宽对 Passkey 菜单文本的匹配以提高鲁棒性。

Sequence Diagram(s)

sequenceDiagram
    participant User as "User"
    participant NavUser as "NavUser\n(app-sidebar/nav-user.tsx)"
    participant AuthLib as "AuthLib\n(auth-user-display.ts)"
    participant Dialog as "Account Dialog"
    participant Nav as "Router/Navigation"

    User->>NavUser: 点击/展开账户区域
    NavUser->>AuthLib: getAuthUserDisplay(user)
    AuthLib-->>NavUser: 返回 maskedIdentity, tenantLabel, role, avatarFallback
    NavUser->>Dialog: 打开账户概览(传入显示数据)
    Dialog-->>User: 显示脱敏身份、状态、租户、角色
    alt 用户选"打开设置"
        User->>Dialog: 点击 Open Settings
        Dialog->>Nav: 导航到 /settings
    else 用户选"管理用户"
        User->>Dialog: 点击 Manage Users
        Dialog->>Nav: 导航到 /users
    else 用户选"切换账户"
        User->>NavUser: 触发 Switch Account
        NavUser->>Nav: 启动切换账号流程
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • solarhell
  • SurviveM

Poem

🐇 兔儿轻跃侧栏前,
账户蒙面显真颜。
安全中心门轻开,
设置与用户一指间,
祝审阅顺利又欢颜。

🚥 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题清晰准确地概括了主要变更:侧边栏用户信息优化,与PR所有代码改动紧密相关。
Linked Issues check ✅ Passed PR实现了所有关键需求:新增用户身份显示模块、集中化账户入口、信息脱敏处理、异常兜底、导航改进和E2E测试更新。
Out of Scope Changes check ✅ Passed 所有变更均与#347需求相关:admin-route防御性编程、页面头部展示优化、侧边栏账户功能完善、新增本地化键值、安全中心导航测试。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/issue-347

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.

❤️ Share

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

@awsl233777
Copy link
Collaborator Author

Automated Test Report

PR: #383 | Branch: fix/issue-347 | Issue: #347

Suite Total Passed Failed Skipped Status
E2E 0 0 0 0 NOT RUN
Unit 0 0 0 0 NOT RUN

Overall: TESTS NOT RUN

Worktree setup was blocked by sandbox permissions before any Go tests could start, so this is not a test failure.

Setup error details
$ git fetch origin fix/issue-347
error: cannot open '.git/FETCH_HEAD': Operation not permitted

$ git fetch --no-write-fetch-head origin fix/issue-347
error: unable to create temporary file: Operation not permitted
fatal: failed to write object
fatal: unpack-objects failed

@ymkiux
Copy link
Contributor

ymkiux commented Mar 10, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Mar 10, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/src/components/layout/app-sidebar/nav-user.tsx`:
- Around line 230-240: The current accountStatusLabel and securityAvailable only
use authEnabled which causes the `authEnabled && !user` case to be shown as
Active and expose security options; update their logic to require a present
authenticated user (e.g., use authenticated = authEnabled && Boolean(user) or
include isLoading if you want a transition state) and base accountStatusLabel
and securityAvailable on that authenticated variable instead of authEnabled
alone; adjust any related UI strings (accountTitle/detailLine can still use
getAuthUserDisplay) so no password/passkey controls are shown when user is
missing.

In `@web/src/lib/auth-user-display.ts`:
- Around line 27-29: The current whitelist regex /^(UID|T)-/i is too broad and
bypasses masking for values like "UID-jane@example.com"; change the check in the
auth-user-display code to only allow UID- and T- followed by digits (e.g.
replace /^(UID|T)-/i.test(normalized) with a stricter pattern such as
/^(UID|T)-\d+$/i) so only system-generated numeric IDs are returned unchanged
and all other values continue through the masking logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ef0bc5b5-50c7-4f3b-9d23-bac68b847eb7

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd6fd6 and b235695.

📒 Files selected for processing (6)
  • web/src/components/auth/admin-route.tsx
  • web/src/components/layout/app-sidebar/nav-user.tsx
  • web/src/components/layout/page-header.tsx
  • web/src/lib/auth-user-display.ts
  • web/src/locales/en.json
  • web/src/locales/zh.json

@ymkiux
Copy link
Contributor

ymkiux commented Mar 11, 2026

@coderabbitai re-review !Stop making breaking changes, do a proper review!

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

@ymkiux, understood — I'll conduct a thorough re-review of PR #383 right away, paying close attention to any potential breaking changes introduced by the sidebar user info optimization.

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
web/src/lib/auth-user-display.ts (1)

21-69: 建议给脱敏/兜底规则补表驱动单测。

这个 helper 现在同时驱动 page-header.tsxnav-user.tsx,而且本次 PR 的测试没有跑起来。建议至少固定 UID/T 直出、邮箱脱敏、空 username、缺失 tenantNamenull 用户这些 case,避免后续再调规则时把两个入口一起带坏。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/lib/auth-user-display.ts` around lines 21 - 69, Add unit tests for
maskAccountIdentifier and getAuthUserDisplay to lock down desensitization and
fallback behavior: create tests that assert UID/T raw passthrough (e.g., inputs
like "UID-123" and "T-456"), email masking (local and domain masking behavior
for "local@domain.com"), empty username (username = ""), missing tenantName, and
null/undefined user inputs to verify rawIdentity, tenantLabel, userLabel,
tenantIDLabel, maskedIdentity, and avatarFallback outputs. Target the functions
maskAccountIdentifier and getAuthUserDisplay (and the AuthUserDisplay shape) so
that future changes to masking or fallback logic will fail tests if they break
these specific cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/src/components/layout/app-sidebar/nav-user.tsx`:
- Around line 230-247: Destructure isLoading from useAuth (const { user,
authEnabled, isLoading, logout } = useAuth()) and update the UI state logic to
account for the loading phase: set accountStatusLabel to a loading text when
isLoading (e.g. t('common.loading')), otherwise keep the existing branches that
use authEnabled and user; ensure detailLine likewise shows loading when
isLoading instead of t('common.unknown'); set securityAvailable to false while
isLoading; leave displayUser/menuDisplayName/accountTitle computed from
getAuthUserDisplay but guard any uses that assume user exists until isLoading is
false.

In `@web/src/lib/auth-user-display.ts`:
- Around line 52-66: The current logic treats an empty username ('') as falsy
and falls back to tenantName, causing multiple users without usernames to appear
identical and mis-handles numeric IDs of 0; update the selection logic so
username is only used when it is a non-empty string (e.g., check username !==
''), avoid falling back to tenantName as the primary identity when username is
empty, and change truthy checks for numeric IDs to explicit null/undefined
checks (e.g., user?.id != null and user?.tenantID != null) when building
rawIdentity, avatarSource, avatarFallback, tenantLabel, userLabel, and
tenantIDLabel (retain maskAccountIdentifier usage for maskedIdentity).

---

Nitpick comments:
In `@web/src/lib/auth-user-display.ts`:
- Around line 21-69: Add unit tests for maskAccountIdentifier and
getAuthUserDisplay to lock down desensitization and fallback behavior: create
tests that assert UID/T raw passthrough (e.g., inputs like "UID-123" and
"T-456"), email masking (local and domain masking behavior for
"local@domain.com"), empty username (username = ""), missing tenantName, and
null/undefined user inputs to verify rawIdentity, tenantLabel, userLabel,
tenantIDLabel, maskedIdentity, and avatarFallback outputs. Target the functions
maskAccountIdentifier and getAuthUserDisplay (and the AuthUserDisplay shape) so
that future changes to masking or fallback logic will fail tests if they break
these specific cases.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60c417b7-a3f3-4e8b-bd43-2f0a1b3c5102

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd6fd6 and d7df415.

📒 Files selected for processing (6)
  • web/src/components/auth/admin-route.tsx
  • web/src/components/layout/app-sidebar/nav-user.tsx
  • web/src/components/layout/page-header.tsx
  • web/src/lib/auth-user-display.ts
  • web/src/locales/en.json
  • web/src/locales/zh.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/src/components/layout/app-sidebar/nav-user.tsx`:
- Around line 96-99: When authentication becomes invalid (securityAvailable
becomes false) ensure any open passkey/change-password dialogs and related hook
state are immediately closed and cleared: add an effect that watches
securityAvailable (derived from hasAuthenticatedUser) and when it flips false
set the dialog visibility state (showPasskeyDialog) to false, call the passkey
hook cleanup (e.g., passkeyCredentials.close() or
passkeyCredentials.clear()/reset()), and abort/reset any pending mutations on
useDeletePasskeyCredential and useRegisterPasskey (e.g.,
deletePasskeyCredential.cancel() / reset(), registerPasskey.cancel() / reset());
this prevents stale previous-account UI and disables mutation triggers when the
user/session is loading or unauthenticated.

In `@web/src/lib/auth-user-display.ts`:
- Around line 7-18: The maskSegment function currently returns the original
value for segments of length <= 1 which leaks single-character identifiers;
change the logic in maskSegment so that an empty string still returns as-is but
any single-character segment returns "*" (i.e., treat chars.length === 0 =>
return value, chars.length === 1 => return "*"), leaving the existing handling
for lengths >= 2 unchanged; update the conditional branches in maskSegment
accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a0515e46-0c7e-4ab2-a402-3a1aac9e421e

📥 Commits

Reviewing files that changed from the base of the PR and between d7df415 and ce92d19.

📒 Files selected for processing (2)
  • web/src/components/layout/app-sidebar/nav-user.tsx
  • web/src/lib/auth-user-display.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: playwright
🔇 Additional comments (2)
web/src/lib/auth-user-display.ts (1)

51-89: 身份回退顺序和 0 ID 处理现在合理了。

这里用 username !== ''id/tenantID != null 收窄了主身份选择逻辑,避免了空用户名回退成租户名、以及把合法的 0 当成缺失值的问题;这部分把多账号下身份误显示的根因收住了。

web/src/components/layout/app-sidebar/nav-user.tsx (1)

77-80: 加载态和已认证态的分支拆分得对。

这里把 isLoadingauthEnableduser 分开处理后,启动阶段不再把未知状态硬渲染成“已激活”或错误身份,和这次侧边栏用户信息优化的目标一致。

Also applies to: 232-253

@ymkiux
Copy link
Contributor

ymkiux commented Mar 11, 2026

@coderabbitai re-review !Stop making breaking changes, do a proper review!

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

@ymkiux, acknowledged — triggering a full re-review of PR #383 now, with specific focus on identifying any breaking changes introduced by the sidebar user info optimization (especially in nav-user.tsx, admin-route.tsx, page-header.tsx, and the new auth-user-display.ts module).

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
web/src/components/layout/app-sidebar/nav-user.tsx (2)

177-191: 建议为退出操作添加确认对话框。

handleRestartServer 使用 window.confirm 在执行前确认,但 handleQuitApp 缺少类似的保护。退出应用是不可逆操作,可能导致未保存的数据丢失。

♻️ 建议修改
 const handleQuitApp = async () => {
   if (!desktopQuitAvailable) {
     return;
   }
+  if (!window.confirm(t('nav.exitAppConfirm'))) {
+    return;
+  }
   try {
     const launcher = (
       window as unknown as {
         go?: { desktop?: { LauncherApp?: { Quit?: () => Promise<void> } } };
       }
     ).go?.desktop?.LauncherApp;
     await launcher?.Quit?.();
   } catch (error) {
     console.error('Quit app failed:', error);
   }
 };

需要在本地化文件中添加 nav.exitAppConfirm 词条。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/components/layout/app-sidebar/nav-user.tsx` around lines 177 - 191,
The quit action in handleQuitApp lacks user confirmation and should mirror
handleRestartServer by showing a confirmation dialog before calling the desktop
launcher Quit; update the handleQuitApp function to call window.confirm with a
localized message key (nav.exitAppConfirm) and only proceed to invoke
launcher?.Quit() when the user confirms, and add the nav.exitAppConfirm entry to
the localization files so the dialog text is translatable.

73-778: 组件职责较多,可考虑后续拆分。

当前组件承担了多项职责:

  • 用户信息显示与头像
  • 主题切换菜单
  • 语言切换
  • 安全中心(密码修改、Passkey 管理)
  • 账户概览对话框
  • 桌面端重启/退出

建议后续迭代时考虑将对话框组件(PasswordDialogPasskeyDialogAccountOverviewDialog)提取为独立文件,以提升可维护性和测试便利性。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/components/layout/app-sidebar/nav-user.tsx` around lines 73 - 778,
The NavUser component is doing too much; extract the three dialog areas into
separate components (e.g., PasswordDialog, PasskeyDialog, AccountOverviewDialog)
to improve maintainability. Move the related state and
handlers—showPasswordDialog/showPasskeyDialog/showAccountDialog,
passwordForm/passwordError/passwordSuccess/passwordTimeoutRef and
handleChangePassword,
passkeyError/passkeySuccess/passkeyCredentials/deletePasskeyCredential/registerPasskey
and handleRegisterPasskey/handleDeletePasskey, and the
displayUser/roleLabel/accountStatusLabel data—into the new components (or pass
them as props), keep translation via useTranslation where needed, and import and
render those dialog components from NavUser in place of the inlined JSX; ensure
the new components either use the existing hooks (useChangeMyPassword,
usePasskeyCredentials, useDeletePasskeyCredential, useRegisterPasskey) or accept
callbacks/handlers so behavior and queryClient cancellations
(userKeys.passkeys()) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@web/src/components/layout/app-sidebar/nav-user.tsx`:
- Around line 177-191: The quit action in handleQuitApp lacks user confirmation
and should mirror handleRestartServer by showing a confirmation dialog before
calling the desktop launcher Quit; update the handleQuitApp function to call
window.confirm with a localized message key (nav.exitAppConfirm) and only
proceed to invoke launcher?.Quit() when the user confirms, and add the
nav.exitAppConfirm entry to the localization files so the dialog text is
translatable.
- Around line 73-778: The NavUser component is doing too much; extract the three
dialog areas into separate components (e.g., PasswordDialog, PasskeyDialog,
AccountOverviewDialog) to improve maintainability. Move the related state and
handlers—showPasswordDialog/showPasskeyDialog/showAccountDialog,
passwordForm/passwordError/passwordSuccess/passwordTimeoutRef and
handleChangePassword,
passkeyError/passkeySuccess/passkeyCredentials/deletePasskeyCredential/registerPasskey
and handleRegisterPasskey/handleDeletePasskey, and the
displayUser/roleLabel/accountStatusLabel data—into the new components (or pass
them as props), keep translation via useTranslation where needed, and import and
render those dialog components from NavUser in place of the inlined JSX; ensure
the new components either use the existing hooks (useChangeMyPassword,
usePasskeyCredentials, useDeletePasskeyCredential, useRegisterPasskey) or accept
callbacks/handlers so behavior and queryClient cancellations
(userKeys.passkeys()) remain unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 090db611-1814-4b84-94c8-7d9fdade2d71

📥 Commits

Reviewing files that changed from the base of the PR and between ce92d19 and 39a4ae8.

📒 Files selected for processing (2)
  • web/src/components/layout/app-sidebar/nav-user.tsx
  • web/src/lib/auth-user-display.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/src/lib/auth-user-display.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: playwright
🔇 Additional comments (4)
web/src/components/layout/app-sidebar/nav-user.tsx (4)

251-282: 用户身份显示逻辑实现正确。

代码正确处理了以下状态:

  • 加载中 (isLoading) 显示加载文案
  • 已认证用户 (hasAuthenticatedUser) 显示实际身份信息
  • 未认证或免认证模式显示相应兜底文案

导航辅助函数 openUsersPageopenSettingsPage 在导航前正确关闭对话框,避免状态残留。


452-494: 安全中心子菜单实现正确。

  • 密码修改和 Passkey 管理入口正确受 securityAvailable 控制,仅对已认证用户显示
  • 打开对话框前重置错误/成功状态,避免显示陈旧消息
  • 设置入口保持可访问性,符合 PR 目标中"统一收敛入口"的要求

575-628: 账户概览对话框实现良好。

  • 用户身份信息经过脱敏处理显示(displayUser.maskedIdentity
  • "管理用户" 入口正确限制为管理员角色(user?.role === 'admin'
  • 响应式布局适配不同屏幕尺寸

符合 PR 目标中对敏感信息脱敏和权限控制的要求。


111-126: 认证失效清理逻辑实现正确。

securityAvailable 变为 false 时:

  • 关闭安全相关对话框
  • 清除错误/成功状态消息
  • 重置 mutation 状态
  • 取消并移除 passkey 查询缓存

这有效防止了账号切换或会话失效时,上一账号的安全信息残留在 UI 上的问题。

@ymkiux
Copy link
Contributor

ymkiux commented Mar 11, 2026

@coderabbitai re-review !Stop making breaking changes, do a proper review!

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

@ymkiux, understood — triggering a full re-review of PR #383 now. I will specifically scrutinize each change in nav-user.tsx, admin-route.tsx, page-header.tsx, and auth-user-display.ts for any potential breaking changes, regressions, or interface/contract violations before raising any comments.


✏️ Learnings added
Learnt from: ymkiux
URL: https://github.com/awsl-project/maxx/pull/383

Timestamp: 2026-03-11T07:29:12.045Z
Learning: Reviewer ymkiux (repo: awsl-project/maxx) requires strict attention to breaking changes in all PRs. Always thoroughly audit public API, interface, and behavioral contract changes — especially in sidebar/auth/layout components — before raising review comments. Do not flag superficial style issues; focus on correctness, regressions, and breaking changes.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
web/src/lib/auth-user-display.ts (1)

63-69: ⚠️ Potential issue | 🟡 Minor

默认兜底值不要再走一遍脱敏。

Line 69 在无身份时回退到 'Maxx',但 Line 83 又把它传给了 maskAccountIdentifier,最终 sidebar/account dialog 会显示成 M**x。异常/免认证场景下这会把兜底文案伪装成“敏感值”,可读性反而更差。让默认值绕过脱敏,或先用空串参与选择、最后再回填 'Maxx' 更稳。

🔧 建议修改
-  const rawIdentity = hasUsername
-    ? username
-    : hasUserId
-      ? `UID-${userId}`
-      : hasTenantId
-        ? `T-${tenantId}`
-        : 'Maxx';
+  const rawIdentity = hasUsername
+    ? username
+    : hasUserId
+      ? `UID-${userId}`
+      : hasTenantId
+        ? `T-${tenantId}`
+        : '';
@@
   return {
-    maskedIdentity: maskAccountIdentifier(rawIdentity) || 'Maxx',
-    rawIdentity,
+    maskedIdentity: rawIdentity ? maskAccountIdentifier(rawIdentity) : 'Maxx',
+    rawIdentity: rawIdentity || 'Maxx',

Also applies to: 82-84

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/lib/auth-user-display.ts` around lines 63 - 69, The fallback literal
'Maxx' is being passed into maskAccountIdentifier causing it to be masked;
change the logic so the default bypasses masking: build rawIdentity using empty
string for the “no identity” branch (i.e. use '' instead of 'Maxx'), then
compute displayName = rawIdentity ? maskAccountIdentifier(rawIdentity) : 'Maxx';
alternatively, detect if rawIdentity === 'Maxx' before calling
maskAccountIdentifier and return 'Maxx' directly; update the code around the
rawIdentity declaration and the call to maskAccountIdentifier accordingly
(references: rawIdentity, hasUsername, username, hasUserId, userId, hasTenantId,
tenantId, maskAccountIdentifier).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/e2e/playwright/test-passkey-discoverable.mjs`:
- Around line 79-84: The test is failing because the "Security Center" element
is a DropdownMenuSubTrigger (wrapping MenuPrimitive.SubmenuTrigger) not a
DropdownMenuItem, so the `[role="menuitem"]` locator is unreliable; update the
locator in test-passkey-discoverable.mjs to target the sub-trigger specifically
(e.g. use a stable attribute like `[data-slot="dropdown-menu-sub-trigger"]` or a
new `data-testid` added to the DropdownMenuSubTrigger component), or replace the
current locator with a DOM-traversal approach such as
`page.locator('span').filter({ hasText: /Security Center|安全中心/ }).locator('..')`
to select the parent trigger element (ensure changes reference
DropdownMenuSubTrigger/MenuPrimitive.SubmenuTrigger where you add the
attribute).

In `@web/src/components/layout/app-sidebar/nav-user.tsx`:
- Around line 111-126: The useEffect that runs when securityAvailable becomes
false resets UI state and two mutations but misses the changePassword mutation;
add a call to changePassword.reset() alongside deletePasskeyCredential.reset()
and registerPasskey.reset() inside that effect, and include changePassword in
the effect's dependency array so changePassword.isPending is cleared when auth
becomes unavailable (so the submit button won't remain disabled).

---

Duplicate comments:
In `@web/src/lib/auth-user-display.ts`:
- Around line 63-69: The fallback literal 'Maxx' is being passed into
maskAccountIdentifier causing it to be masked; change the logic so the default
bypasses masking: build rawIdentity using empty string for the “no identity”
branch (i.e. use '' instead of 'Maxx'), then compute displayName = rawIdentity ?
maskAccountIdentifier(rawIdentity) : 'Maxx'; alternatively, detect if
rawIdentity === 'Maxx' before calling maskAccountIdentifier and return 'Maxx'
directly; update the code around the rawIdentity declaration and the call to
maskAccountIdentifier accordingly (references: rawIdentity, hasUsername,
username, hasUserId, userId, hasTenantId, tenantId, maskAccountIdentifier).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e824f3a-72d6-4cf0-9359-5fd4f6de29ad

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd6fd6 and 09386c7.

📒 Files selected for processing (7)
  • tests/e2e/playwright/test-passkey-discoverable.mjs
  • web/src/components/auth/admin-route.tsx
  • web/src/components/layout/app-sidebar/nav-user.tsx
  • web/src/components/layout/page-header.tsx
  • web/src/lib/auth-user-display.ts
  • web/src/locales/en.json
  • web/src/locales/zh.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: playwright
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ymkiux
Repo: awsl-project/maxx PR: 0
File: :0-0
Timestamp: 2026-03-11T07:29:12.045Z
Learning: Reviewer ymkiux (repo: awsl-project/maxx) requires strict attention to breaking changes in all PRs. Always thoroughly audit public API, interface, and behavioral contract changes — especially in sidebar/auth/layout components — before raising review comments. Do not flag superficial style issues; focus on correctness, regressions, and breaking changes.
📚 Learning: 2026-03-11T07:29:12.045Z
Learnt from: ymkiux
Repo: awsl-project/maxx PR: 0
File: :0-0
Timestamp: 2026-03-11T07:29:12.045Z
Learning: Reviewer ymkiux (repo: awsl-project/maxx) requires strict attention to breaking changes in all PRs. Always thoroughly audit public API, interface, and behavioral contract changes — especially in sidebar/auth/layout components — before raising review comments. Do not flag superficial style issues; focus on correctness, regressions, and breaking changes.

Applied to files:

  • web/src/lib/auth-user-display.ts
  • web/src/components/layout/app-sidebar/nav-user.tsx
🔇 Additional comments (1)
web/src/components/auth/admin-route.tsx (1)

22-24: LGTM!防御性空值检查改进正确。

此更改将 user.role 改为 user?.role,是一个合理的防御性修复。虽然第 14 行的守卫逻辑 authEnabled && !user 理论上已经过滤了 user 为 null 的情况,但可选链仍提供了额外的安全保障:

  • user 意外为 null,user?.role 返回 undefined,条件判断为 true,触发重定向至首页——符合 PR 目标中"信息缺失时提供合理兜底处理"的要求。
  • 无破坏性变更,行为更健壮。

Comment on lines +79 to +84
// 打开安全中心子菜单
const securityMenuItem = page
.locator('[role="menuitem"]')
.filter({ hasText: /Security Center|安全中心/ });
await securityMenuItem.click();
await page.waitForTimeout(300);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "test-passkey-discoverable.mjs" | head -5

Repository: awsl-project/maxx

Length of output: 113


🏁 Script executed:

find . -type f -name "nav-user.tsx" | head -5

Repository: awsl-project/maxx

Length of output: 113


🏁 Script executed:

cat -n ./tests/e2e/playwright/test-passkey-discoverable.mjs | sed -n '75,90p'

Repository: awsl-project/maxx

Length of output: 703


🏁 Script executed:

cat -n ./web/src/components/layout/app-sidebar/nav-user.tsx | sed -n '450,460p'

Repository: awsl-project/maxx

Length of output: 619


🏁 Script executed:

cat -n ./web/src/components/layout/app-sidebar/nav-user.tsx | sed -n '1,30p'

Repository: awsl-project/maxx

Length of output: 827


🏁 Script executed:

cat -n ./web/src/components/layout/app-sidebar/nav-user.tsx | sed -n '445,480p'

Repository: awsl-project/maxx

Length of output: 1907


🏁 Script executed:

rg "from.*DropdownMenu" ./web/src/components/layout/app-sidebar/nav-user.tsx

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

rg "DropdownMenuSubTrigger" ./web --type tsx --type ts -A 2 -B 2 | head -50

Repository: awsl-project/maxx

Length of output: 88


🏁 Script executed:

rg "DropdownMenuSubTrigger" ./web -A 2 -B 2 | head -50

Repository: awsl-project/maxx

Length of output: 2705


🏁 Script executed:

grep -n "import.*Dropdown" ./web/src/components/layout/app-sidebar/nav-user.tsx

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

cat -n ./web/src/components/ui/dropdown-menu.tsx | grep -A 20 "function DropdownMenuSubTrigger"

Repository: awsl-project/maxx

Length of output: 1007


🏁 Script executed:

cat -n ./web/src/components/ui/dropdown-menu.tsx | head -20

Repository: awsl-project/maxx

Length of output: 888


🏁 Script executed:

rg "MenuPrimitive" ./web/src/components/ui/dropdown-menu.tsx | head -5

Repository: awsl-project/maxx

Length of output: 406


🏁 Script executed:

rg "data-slot.*dropdown-menu" ./tests/e2e/playwright -B 2 -A 2

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

web_search "base-ui react menu submenu trigger role attribute"

Repository: awsl-project/maxx

Length of output: 111


🏁 Script executed:

cat -n ./tests/e2e/playwright/test-passkey-discoverable.mjs | sed -n '70,100p'

Repository: awsl-project/maxx

Length of output: 1240


🏁 Script executed:

rg "data-slot.*sub-trigger" ./web

Repository: awsl-project/maxx

Length of output: 146


🏁 Script executed:

rg "playwright" ./tests/e2e/playwright/test-passkey-discoverable.mjs | head -5

Repository: awsl-project/maxx

Length of output: 99


🏁 Script executed:

cat -n ./tests/e2e/playwright/test-passkey-discoverable.mjs | sed -n '1,20p'

Repository: awsl-project/maxx

Length of output: 642


🏁 Script executed:

rg "DropdownMenuSubTrigger" ./tests -B 3 -A 3

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

rg "data-slot.*dropdown" ./tests -B 2 -A 2 | head -40

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

python3 << 'EOF'
# Check if there are any test utilities or helpers that might use data-slot
import subprocess
result = subprocess.run(['rg', 'data-slot', './tests', '-A', '2'], 
                       capture_output=True, text=True, timeout=10)
print("Data-slot usage in tests:")
print(result.stdout if result.stdout else "(none found)")
EOF

Repository: awsl-project/maxx

Length of output: 99


🏁 Script executed:

cat -n ./web/src/components/ui/dropdown-menu.tsx | sed -n '40,80p'

Repository: awsl-project/maxx

Length of output: 1728


🏁 Script executed:

rg "locator.*role.*menuitem" ./tests/e2e -B 3 -A 3

Repository: awsl-project/maxx

Length of output: 1895


🏁 Script executed:

rg "SubmenuTrigger\|SubmenuItem" ./web -B 2 -A 2 | head -50

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

cat -n ./web/src/components/ui/dropdown-menu.tsx | sed -n '74,95p'

Repository: awsl-project/maxx

Length of output: 1261


🏁 Script executed:

rg "MenuPrimitive.Item\|MenuItem" ./web/src/components/ui/dropdown-menu.tsx -A 8 | head -40

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

python3 << 'EOF'
# Let's check the base-ui/react/menu package to understand role rendering
import subprocess
result = subprocess.run(['find', '.', '-name', 'package.json', '-type', 'f'], 
                       capture_output=True, text=True, timeout=10)
package_files = result.stdout.strip().split('\n')
# Look for the main package.json
for pf in package_files:
    if pf.endswith('/package.json') and '/' not in pf.replace('./', ''):
        print(f"Main package.json: {pf}")
        break
EOF

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

cat ./package.json | grep -A 2 "@base-ui/react"

Repository: awsl-project/maxx

Length of output: 107


🏁 Script executed:

find . -name "package.json" -type f | head -5

Repository: awsl-project/maxx

Length of output: 150


🏁 Script executed:

cat ./web/package.json | grep -A 2 "@base-ui/react"

Repository: awsl-project/maxx

Length of output: 159


🏁 Script executed:

rg "locator.*data-slot" ./tests/e2e/playwright -B 2 -A 2

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

rg "data-slot.*dropdown-menu" ./web/src/components/ui/dropdown-menu.tsx | head -20

Repository: awsl-project/maxx

Length of output: 1028


Security Center 选择器不匹配,测试会卡在这一步。

lines 80-82 使用 [role="menuitem"] 定位器,但 nav-user.tsx line 453 的 "Security Center" 是 DropdownMenuSubTrigger(包裹 MenuPrimitive.SubmenuTrigger),而不是 DropdownMenuItemMenuPrimitive.Item)。两者是不同的组件结构,role 属性可能不同。

建议改用稳定的属性选择器,例如 data-slot="dropdown-menu-sub-trigger" 或移除 role 限制改为:

const securityMenuItem = page.locator('span').filter({ hasText: /Security Center|安全中心/ }).locator('..');

或者为 DropdownMenuSubTrigger 添加 data-testid 以便测试定位。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/playwright/test-passkey-discoverable.mjs` around lines 79 - 84, The
test is failing because the "Security Center" element is a
DropdownMenuSubTrigger (wrapping MenuPrimitive.SubmenuTrigger) not a
DropdownMenuItem, so the `[role="menuitem"]` locator is unreliable; update the
locator in test-passkey-discoverable.mjs to target the sub-trigger specifically
(e.g. use a stable attribute like `[data-slot="dropdown-menu-sub-trigger"]` or a
new `data-testid` added to the DropdownMenuSubTrigger component), or replace the
current locator with a DOM-traversal approach such as
`page.locator('span').filter({ hasText: /Security Center|安全中心/ }).locator('..')`
to select the parent trigger element (ensure changes reference
DropdownMenuSubTrigger/MenuPrimitive.SubmenuTrigger where you add the
attribute).

Comment on lines +111 to +126
useEffect(() => {
if (securityAvailable) {
return;
}
setShowPasskeyDialog(false);
setShowPasswordDialog(false);
setPasskeyError('');
setPasskeySuccess('');
setDeletingPasskeyID(null);
setPasswordError('');
setPasswordSuccess('');
deletePasskeyCredential.reset();
registerPasskey.reset();
queryClient.cancelQueries({ queryKey: userKeys.passkeys() });
queryClient.removeQueries({ queryKey: userKeys.passkeys() });
}, [securityAvailable, deletePasskeyCredential, registerPasskey, queryClient]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n web/src/components/layout/app-sidebar/nav-user.tsx | sed -n '75,135p'

Repository: awsl-project/maxx

Length of output: 2964


🏁 Script executed:

cat -n web/src/components/layout/app-sidebar/nav-user.tsx | sed -n '680,700p'

Repository: awsl-project/maxx

Length of output: 910


🏁 Script executed:

# Check if changePassword.reset() is called anywhere in the file
rg "changePassword\.reset" web/src/components/layout/app-sidebar/nav-user.tsx

Repository: awsl-project/maxx

Length of output: 43


🏁 Script executed:

# Check the entire useEffect and surrounding context to understand the mutation definitions
cat -n web/src/components/layout/app-sidebar/nav-user.tsx | sed -n '1,150p'

Repository: awsl-project/maxx

Length of output: 5915


需要在鉴权失效时重置 changePassword 变更密码 mutation。

此 useEffect 已清空本地对话框和状态(第 115-121 行),但只重置了 deletePasskeyCredentialregisterPasskey 两个 mutation(第 122-123 行),遗漏了 changePassword。由于第 691 行的提交按钮直接依赖 changePassword.isPending,若改密请求尚未结束就触发鉴权失效(登出/切账号),下次打开弹窗时该按钮仍会因旧 session 的 pending 状态保持禁用。在第 126 行的 useEffect 内添加 changePassword.reset(),并将其加入依赖数组。

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/src/components/layout/app-sidebar/nav-user.tsx` around lines 111 - 126,
The useEffect that runs when securityAvailable becomes false resets UI state and
two mutations but misses the changePassword mutation; add a call to
changePassword.reset() alongside deletePasskeyCredential.reset() and
registerPasskey.reset() inside that effect, and include changePassword in the
effect's dependency array so changePassword.isPending is cleared when auth
becomes unavailable (so the submit button won't remain disabled).

@ymkiux ymkiux closed this Mar 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

侧边栏用户信息优化

3 participants