feat: show user display names in usage analytics - #6259
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThe change adds display-name fields and batch identity resolution for logs, tasks, and quota data. Quota history now filters through matching user IDs, preserves rows across username changes, aggregates by user identity, and documents stable chart identity and label handling. ChangesUser identity presentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant GetQuotaDatesByUser
participant GetQuotaDataGroupByUser
participant GetUserIdentitiesByIds
Dashboard->>GetQuotaDatesByUser: request username-filtered quota history
GetQuotaDatesByUser->>GetQuotaDataGroupByUser: pass username and time range
GetQuotaDataGroupByUser-->>GetQuotaDatesByUser: rows grouped by user ID
GetQuotaDatesByUser->>GetUserIdentitiesByIds: resolve current identities
GetUserIdentitiesByIds-->>GetQuotaDatesByUser: username and display name
GetQuotaDatesByUser-->>Dashboard: enriched quota rows
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
model/user.go (1)
1256-1267: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePre-allocate map capacity.
Since you know the maximum number of items that will be added to the map (bounded by
len(ids)orlen(rows)), consider pre-allocating the map capacity to avoid reallocation overhead.♻️ Proposed refactor
- result := make(map[int]UserIdentity) if len(ids) == 0 { - return result, nil + return make(map[int]UserIdentity), nil } var rows []UserIdentity if err := DB.Model(&User{}).Select("id, username, display_name").Where("id IN ?", ids).Find(&rows).Error; err != nil { return nil, err } + result := make(map[int]UserIdentity, len(rows)) for _, r := range rows { result[r.Id] = r }🤖 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 `@model/user.go` around lines 1256 - 1267, Update the result map initialization in the user identity lookup function to pre-allocate capacity based on len(ids), preserving the existing empty-input, query, and population behavior.
🤖 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 `@docs/superpowers/specs/2026-07-17-user-chart-identity-design.md`:
- Line 12: Update the user chart identity design document’s response/type
contract and username-filtering statements to match the implemented behavior,
including display_name. Replace the documented full-width Chinese-parenthesis
label format with the implemented `display name (username)` format wherever the
disambiguation behavior is described, including the referenced lines.
In `@web/default/src/features/dashboard/components/users/user-charts.tsx`:
- Around line 239-244: Add an accessible, localized name to the username filter
Input by supplying an aria-label through the existing t translation helper,
while preserving its current value, change handler, placeholder, and styling.
In `@web/default/src/features/dashboard/lib/charts.test.ts`:
- Around line 1-2: Update the test imports and assertions in the chart utility
test to use Vitest: replace node:test and node:assert with Vitest’s test and
expect APIs, and change assert.deepEqual assertions to expect(...).toEqual(...).
Keep the existing test behavior unchanged.
In `@web/default/src/features/dashboard/lib/charts.ts`:
- Around line 779-786: Update the label-generation logic around labelByKey to
track final labels already assigned and ensure every candidate is globally
unique; when a candidate collides, append an additional deterministic suffix
while preserving existing duplicate-base-label behavior. Add a regression test
covering a natural label collision such as “Alice (bob)” versus “Alice” with
username “bob”, verifying distinct series labels and color-map keys.
---
Nitpick comments:
In `@model/user.go`:
- Around line 1256-1267: Update the result map initialization in the user
identity lookup function to pre-allocate capacity based on len(ids), preserving
the existing empty-input, query, and population behavior.
🪄 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: 673d6f6b-5d24-4a6b-822c-a84c40779b7c
📒 Files selected for processing (22)
controller/log.gocontroller/task.gocontroller/usedata.gocontroller/usedata_test.godocs/superpowers/specs/2026-07-17-user-chart-identity-design.mddto/task.gomodel/log.gomodel/task.gomodel/usedata.gomodel/user.gorelay/relay_task.goweb/default/src/features/dashboard/api.tsweb/default/src/features/dashboard/components/users/user-charts.tsxweb/default/src/features/dashboard/index.tsxweb/default/src/features/dashboard/lib/charts.test.tsweb/default/src/features/dashboard/lib/charts.tsweb/default/src/features/dashboard/types.tsweb/default/src/features/usage-logs/components/columns/common-logs-columns.tsxweb/default/src/features/usage-logs/components/columns/task-logs-columns.tsxweb/default/src/features/usage-logs/components/usage-logs-mobile-card.tsxweb/default/src/features/usage-logs/data/schema.tsweb/default/src/features/usage-logs/types.ts
| <Input | ||
| value={username} | ||
| onChange={(e) => handleUsernameChange(e.target.value)} | ||
| placeholder={t('Filter by username')} | ||
| className='h-8 w-36 shrink-0 text-xs sm:w-44' | ||
| /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Give the username filter an accessible name.
The input relies solely on its placeholder, so assistive technology lacks a reliable label. Add a localized aria-label or visible <label>.
Proposed fix
<Input
value={username}
onChange={(e) => handleUsernameChange(e.target.value)}
+ aria-label={t('Filter by username')}
placeholder={t('Filter by username')}As per coding guidelines, associate form inputs with labels and add ARIA attributes when needed. <coding_guidelines>
📝 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.
| <Input | |
| value={username} | |
| onChange={(e) => handleUsernameChange(e.target.value)} | |
| placeholder={t('Filter by username')} | |
| className='h-8 w-36 shrink-0 text-xs sm:w-44' | |
| /> | |
| <Input | |
| value={username} | |
| onChange={(e) => handleUsernameChange(e.target.value)} | |
| aria-label={t('Filter by username')} | |
| placeholder={t('Filter by username')} | |
| className='h-8 w-36 shrink-0 text-xs sm:w-44' | |
| /> |
🤖 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/dashboard/components/users/user-charts.tsx` around
lines 239 - 244, Add an accessible, localized name to the username filter Input
by supplying an aria-label through the existing t translation helper, while
preserving its current value, change handler, placeholder, and styling.
Source: Coding guidelines
| import assert from 'node:assert/strict' | ||
| import { test } from 'node:test' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use Vitest for this frontend utility test.
The new test uses node:test and node:assert, bypassing the project’s prescribed frontend test runner.
Proposed fix
-import assert from 'node:assert/strict'
-import { test } from 'node:test'
+import { expect, test } from 'vitest'Replace assert.deepEqual(actual, expected) with expect(actual).toEqual(expected).
As per coding guidelines, unit tests under web/default/**/*.test.ts must use Vitest. <coding_guidelines>
📝 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.
| import assert from 'node:assert/strict' | |
| import { test } from 'node:test' | |
| import { expect, test } from 'vitest' |
🤖 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/dashboard/lib/charts.test.ts` around lines 1 - 2,
Update the test imports and assertions in the chart utility test to use Vitest:
replace node:test and node:assert with Vitest’s test and expect APIs, and change
assert.deepEqual assertions to expect(...).toEqual(...). Keep the existing test
behavior unchanged.
Source: Coding guidelines
| const labelByKey = new Map<string, string>() | ||
| for (const [key, identity] of identityByKey) { | ||
| const duplicate = (baseLabelCounts.get(identity.baseLabel) || 0) > 1 | ||
| const suffix = identity.username || key.replace(/^id:/, '#') | ||
| labelByKey.set( | ||
| key, | ||
| duplicate ? `${identity.baseLabel} (${suffix})` : identity.baseLabel | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guarantee that final chart labels are globally unique.
Disambiguating only duplicate base labels can still collide with another identity’s natural label—for example, Alice (bob) and Alice with username bob. Both then use the same seriesField and color-map key, so separate identities can render as one series.
Track used final labels and append an additional stable suffix when a candidate already exists. Add a regression case for this collision.
🤖 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/dashboard/lib/charts.ts` around lines 779 - 786,
Update the label-generation logic around labelByKey to track final labels
already assigned and ensure every candidate is globally unique; when a candidate
collides, append an additional deterministic suffix while preserving existing
duplicate-base-label behavior. Add a regression test covering a natural label
collision such as “Alice (bob)” versus “Alice” with username “bob”, verifying
distinct series labels and color-map keys.
603ba20 to
df1e7fc
Compare
|
本项目支持使用AI辅助开发，但是不代表人可以直接不看代码。 |
额,这个我测试验证了哇...是checklist 问题? |
2 participants
Important
📝 变更描述 / Description
display_name，未向历史日志冗余写入显示名称；显示名称缺失时回退到用户名。user_id聚合；多个用户使用相同显示名称时，标签显示为显示名称 (用户名)。缺少有效用户 ID 的旧数据继续按用户名区分，避免错误合并。AI-assisted disclosure: 本 PR 的代码与说明由 AI 辅助生成；提交者应在合并前完成最终人工审阅。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
go test ./...bun run typecheck（web/default）bun test src/features/dashboard/lib（33 passed, 0 failed）bun run build（web/default）gofmt -d检查本 PR 修改的 Go 文件，无差异oxfmt --check检查本 PR 修改的 11 个前端文件，全部通过oxlint检查除charts.ts外的本 PR 前端修改文件，全部通过；charts.ts的现有基线规则告警未由本 PR 引入Summary by CodeRabbit
New Features
Bug Fixes