fix: return full self user list shape#1213
Conversation
📝 WalkthroughWalkthrough将用户显示构建抽象为 buildUserDisplays；getUsers 与 getCurrentUserDisplay 复用该构建器；listCurrentUser 改为通过 getCurrentUserDisplay 返回 items（含 keys），并在 id 不匹配时返回 404；redactUserKeys 增加对 Date 实例的原样保留；相关集成与单元测试已更新。 变更说明用户展示与 self 端点统一构建
代码审查工作量估计🎯 4 (Complex) | ⏱️ ~45 分钟 可能相关的PR
🚥 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/v1/resources/users/handlers.ts (1)
81-99:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift用
getUsers拉全量列表再筛选当前用户的实现方式存在性能与数据暴露风险
listCurrentUser现在调用actions.getUsers([])拉取整个用户列表，然后在内存中find当前用户。这带来两个问题：
- 性能/可扩展性：管理员会话命中该端点时（例如本 PR 测试中的 "admin self" 用例），会从数据库读取并序列化全量用户（包括所有 keys、tags、limits 等），仅为返回单个用户。用户数量增长后这是一次明显放大的 N→1 查询。
- 数据最小化：即使外层会用
find过滤，action 层仍会把所有用户的明细（包括包含fullKey的 keys）加载到进程内存并序列化为result.data。redactUserKeys只在最终响应层裁剪fullKey，对中间过程的内存暴露和潜在日志/错误路径泄露无防护。更合适的做法是新增/复用一个按 id 精确查询且返回
UserDisplay（带keys）形态的 action（例如让getUserById也返回 keys，或新增getCurrentUserDisplay()），既能保留 dashboard 期望的形状，又避免全量扫描。🤖 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 `@src/app/api/v1/resources/users/handlers.ts` around lines 81 - 99, The handler currently calls callAction(c, actions.getUsers, ...) and then finds currentUser in result.data which causes full-table reads and exposes sensitive key material in memory; change the handler to call a new or existing targeted action that fetches a single user by id and returns the UserDisplay shape (e.g. use or add actions.getUserById or actions.getCurrentUserDisplay) so only the requested user (with keys) is loaded and serialized, and then apply redactUserKeys(currentUser) before returning; update the handler to remove call to actions.getUsers and rely on the single-user action to address performance and data-minimization concerns.
🧹 Nitpick comments (2)
src/app/api/v1/resources/users/handlers.ts (1)
92-99: 💤 Low value
pageInfo.limit: 1语义存在误导该 endpoint 始终返回单个用户，
limit字段实际并不代表请求的分页大小，硬编码为1容易让消费方误认为是受限分页结果。建议改用更明确的语义（例如沿用items.length，或与listUsers的limit默认值保持一致），或在 OpenAPI 文档中注明该字段在 self 端点下的固定语义。🤖 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 `@src/app/api/v1/resources/users/handlers.ts` around lines 92 - 99, 当前实现将 pageInfo.limit 硬编码为 1，会误导消费方以为这是分页请求的 limit；在返回 jsonResponse 时改为使用实际返回条数（例如 items.length from redactUserKeys(currentUser)）或与 listUsers 的默认 limit 保持一致，以明确语义；在 handlers.ts 的该返回块（jsonResponse / pageInfo.limit）将 limit 设置为 items.length 或其它明确值，并在必要时补充注释说明此 endpoint 始终只返回单个用户。tests/unit/api/v1/api-client-actions.test.ts (1)
136-156: 💤 Low value建议补充对
keys形态在客户端层的回退断言当前断言用
toMatchObject校验keys字段透传，但未验证客户端users.getUsers()在回退到/api/v1/users:self时对pageInfo（hasMore: false / nextCursor: null）的处理是否符合预期（例如不会把单元素结果误标为「还有下一页」）。可考虑显式断言返回数组长度为 1，并/或对客户端内部对 pageInfo 的消化路径补一条用例。🤖 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 `@tests/unit/api/v1/api-client-actions.test.ts` around lines 136 - 156, The test needs an explicit assertion that users.getUsers correctly consumes pageInfo when falling back to "/api/v1/users:self" so it doesn't incorrectly report more pages; after awaiting users.getUsers(), assert the resolved array has length 1 (e.g., expect(result).toHaveLength(1)) and/or assert the single item matches the expected object shape, ensuring pageInfo (hasMore/nextCursor) is not causing extra items; reference the users.getUsers call, the getMock responses (first ApiError, then the items + pageInfo), and the "/api/v1/users:self" fallback to locate where to add these extra assertions.
🤖 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.
Outside diff comments:
In `@src/app/api/v1/resources/users/handlers.ts`:
- Around line 81-99: The handler currently calls callAction(c, actions.getUsers,
...) and then finds currentUser in result.data which causes full-table reads and
exposes sensitive key material in memory; change the handler to call a new or
existing targeted action that fetches a single user by id and returns the
UserDisplay shape (e.g. use or add actions.getUserById or
actions.getCurrentUserDisplay) so only the requested user (with keys) is loaded
and serialized, and then apply redactUserKeys(currentUser) before returning;
update the handler to remove call to actions.getUsers and rely on the
single-user action to address performance and data-minimization concerns.
---
Nitpick comments:
In `@src/app/api/v1/resources/users/handlers.ts`:
- Around line 92-99: 当前实现将 pageInfo.limit 硬编码为 1，会误导消费方以为这是分页请求的 limit；在返回
jsonResponse 时改为使用实际返回条数（例如 items.length from redactUserKeys(currentUser)）或与
listUsers 的默认 limit 保持一致，以明确语义；在 handlers.ts 的该返回块（jsonResponse /
pageInfo.limit）将 limit 设置为 items.length 或其它明确值，并在必要时补充注释说明此 endpoint 始终只返回单个用户。
In `@tests/unit/api/v1/api-client-actions.test.ts`:
- Around line 136-156: The test needs an explicit assertion that users.getUsers
correctly consumes pageInfo when falling back to "/api/v1/users:self" so it
doesn't incorrectly report more pages; after awaiting users.getUsers(), assert
the resolved array has length 1 (e.g., expect(result).toHaveLength(1)) and/or
assert the single item matches the expected object shape, ensuring pageInfo
(hasMore/nextCursor) is not causing extra items; reference the users.getUsers
call, the getMock responses (first ApiError, then the items + pageInfo), and the
"/api/v1/users:self" fallback to locate where to add these extra assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e08d1fc-eab8-44cf-9c45-7bd6ae871790
📒 Files selected for processing (3)
src/app/api/v1/resources/users/handlers.tstests/api/v1/users/users.test.tstests/unit/api/v1/api-client-actions.test.ts
![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 updates the listCurrentUser handler to use the getUsers action instead of getUserById, filtering the results in-memory to find the current user. Reviewers identified several critical issues: the current implementation incorrectly checks for an ok property on a raw array result, which will cause the request to fail; fetching the entire user list for admin users introduces significant performance and memory risks; and the call to redactUserKeys is both redundant and likely to corrupt Date fields within the user object.
| const result = await callAction(c, actions.getUsers, [] as never[], c.get("auth")); | ||
| if (!result.ok) return actionError(c, result); |
There was a problem hiding this comment.
The getUsers action (from @/actions/users) returns a raw array of UserDisplay[], unlike most other actions in this repository which return an ActionResult object (e.g., { ok: true, data: ... }).
Because getUsers returns an array, the check if (!result.ok) will always evaluate to true (since an array does not have an ok property, making it undefined), causing the request to fail and return an error response even when the action succeeds. You should either update the getUsers action to return an ActionResult or handle the raw array return type correctly in this handler.
| [currentUserId] as never[], | ||
| c.get("auth") | ||
| ); | ||
| const result = await callAction(c, actions.getUsers, [] as never[], c.get("auth")); |
There was a problem hiding this comment.
For admin users, calling getUsers without parameters triggers loadAllUsersForAdmin(), which fetches the entire user database along with all associated keys, usage statistics, and rate limit data. This data is then filtered in-memory using .find().
This approach will cause significant performance degradation and potential memory exhaustion (OOM) as the user base grows. Since you only need the UserDisplay shape for the current user, it is recommended to implement a targeted action (e.g., getUserDisplayById) or add an ID filter to the getUsers action to avoid loading the entire inventory.
| } | ||
| return jsonResponse({ | ||
| items: [redactUserKeys(result.data)], | ||
| items: [redactUserKeys(currentUser)], |
There was a problem hiding this comment.
The redactUserKeys function (defined at line 353) contains a bug where it incorrectly processes Date objects. Because typeof date === 'object' is true and Object.entries(date) returns an empty array, any Date field in the currentUser object (such as createdAt, expiresAt, or costResetAt) will be transformed into an empty object {} in the final JSON response.
Furthermore, the UserDisplay objects returned by getUsers already have their keys masked and do not contain the fullKey field that redactUserKeys is designed to strip. You should remove this call to avoid data loss and unnecessary processing.
| items: [redactUserKeys(currentUser)], | |
| items: [currentUser], |
|
Follow-up pushed in Valid feedback addressed:
One note about Fresh local verification after the follow-up:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/actions/users.ts (1)
745-853: ⚖️ Poor tradeoff可选重构：
getUsersBatch可复用buildUserDisplays
getUsersBatch中构建UserDisplay的逻辑（lines 745-853）与新增的buildUserDisplays高度重复。后续可考虑让getUsersBatch也复用buildUserDisplays，减少维护成本。此为建议性重构，不影响本 PR 合入。
🤖 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 `@src/actions/users.ts` around lines 745 - 853, The user-display construction in getUsersBatch duplicates the mapping logic used in buildUserDisplays; refactor getUsersBatch to call the existing buildUserDisplays function (or extract the shared mapping into a helper used by both) so keys/usage/statistics handling (keysMap, usageMap, statisticsMap), maskKey usage, canExposeFullKey logic, and key shaping are centralized; update getUsersBatch to supply the same inputs buildUserDisplays expects and remove the inline mapping block that creates UserDisplay objects to avoid duplicate code and maintenance gaps.
🤖 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.
Nitpick comments:
In `@src/actions/users.ts`:
- Around line 745-853: The user-display construction in getUsersBatch duplicates
the mapping logic used in buildUserDisplays; refactor getUsersBatch to call the
existing buildUserDisplays function (or extract the shared mapping into a helper
used by both) so keys/usage/statistics handling (keysMap, usageMap,
statisticsMap), maskKey usage, canExposeFullKey logic, and key shaping are
centralized; update getUsersBatch to supply the same inputs buildUserDisplays
expects and remove the inline mapping block that creates UserDisplay objects to
avoid duplicate code and maintenance gaps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 762fff72-6f87-47b7-9b48-9da6293e10a2
📒 Files selected for processing (5)
src/actions/users.tssrc/app/api/v1/resources/users/handlers.tstests/api/v1/users/users.test.tstests/unit/api/v1/api-client-actions.test.tstests/unit/users-action-get-users-compat.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The changes correctly address the self-user dashboard compatibility problem by introducing a targeted getCurrentUserDisplay() action that returns the full UserDisplay shape (including keys) without triggering a full user inventory scan. The redactUserKeys fix for Date preservation is clean, and the test coverage validates the endpoint shape, redaction, date serialization, and the targeted single-user query path.
PR Size: M
- Lines changed: 434 (291 additions, 143 deletions)
- Files changed: 5
Review Coverage
- Logic and correctness - Clean. The extracted
buildUserDisplays()correctly shares the display-shape construction betweengetUsers()andgetCurrentUserDisplay(). The handler's defensiveresult.data.id \!== currentUserIdcheck is appropriate. - Security (OWASP Top 10) - Clean.
fullKeyredaction is preserved; no access control bypass; self endpoint remains scoped to the authenticated session user. - Error handling - Clean. Errors in
getCurrentUserDisplayare logged and wrapped inActionResult. ThebuildUserDisplaysper-user catch block logs and degrades gracefully (empty keys array). - Type safety - Clean.
buildUserDisplaysaccepts the minimalUserActionSessionshape; noanyusage introduced. - Documentation accuracy - Clean. The JSDoc on
getCurrentUserDisplayaccurately describes the intent (non-admin shape without full inventory scan). - Test coverage - Adequate. Integration tests verify the self endpoint response shape, redaction, and date serialization. Unit tests verify the targeted single-user query path and that the dashboard client fallback consumes the correct response shape.
- Code clarity - Good. Moving
canUserManageKeyoutside thekeys.map()callback is a valid simplification.
Automated review by Claude AI
There was a problem hiding this comment.
Pull request overview
This PR fixes a dashboard compatibility regression by ensuring the read-tier /api/v1/users:self endpoint returns the same UserDisplay list-item shape as the main users list (notably including the keys array), without requiring an admin-only full user inventory load.
Changes:
- Added a targeted
getCurrentUserDisplay()action that loads only the authenticated session user and builds the standardUserDisplayshape (includingkeys). - Updated
/api/v1/users:selfto usegetCurrentUserDisplay()and adjusted key redaction to preserveDateinstances so API JSON date serialization works correctly. - Added/updated regression tests for the self endpoint shape, dashboard fallback behavior, and ensuring the self path does not call the full users list action.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/actions/users.ts |
Extracts shared display-shaping logic and adds getCurrentUserDisplay() to avoid admin inventory scans while returning dashboard-compatible shapes. |
src/app/api/v1/resources/users/handlers.ts |
Switches /users:self to the new action and fixes recursive redaction to preserve Date objects for correct ISO serialization. |
tests/api/v1/users/users.test.ts |
Adds coverage that /users:self includes keys, redacts fullKey, preserves date serialization, and avoids calling full-list/detail actions. |
tests/unit/users-action-get-users-compat.test.ts |
Adds a unit test ensuring getCurrentUserDisplay() only loads the current user ID and uses batch key/usage/stat queries for that single user. |
tests/unit/api/v1/api-client-actions.test.ts |
Updates the client fallback test to consume the self endpoint’s { items, pageInfo } response shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!displayUser) { | ||
| return { | ||
| ok: false, | ||
| error: tError("USER_NOT_FOUND"), | ||
| errorCode: ERROR_CODES.NOT_FOUND, | ||
| }; | ||
| } |
There was a problem hiding this comment.
buildUserDisplays
buildUserDisplays is guaranteed to return exactly one element when called with a single-element array: the per-user try/catch inside the function always produces a value (either the full shape or a minimal fallback with keys: []), so displayUser can never be undefined on this path. The only way buildUserDisplays returns an empty array is when users.length === 0, but we've just confirmed user is non-null before calling it. If buildUserDisplays throws, the outer try/catch already handles it. The guard is effectively unreachable and could mislead future readers into thinking there's a genuine null-result code path here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/users.ts
Line: 524-530
Comment:
**Dead-code guard after `buildUserDisplays`**
`buildUserDisplays` is guaranteed to return exactly one element when called with a single-element array: the per-user `try/catch` inside the function always produces a value (either the full shape or a minimal fallback with `keys: []`), so `displayUser` can never be `undefined` on this path. The only way `buildUserDisplays` returns an empty array is when `users.length === 0`, but we've just confirmed `user` is non-null before calling it. If `buildUserDisplays` throws, the outer `try/catch` already handles it. The guard is effectively unreachable and could mislead future readers into thinking there's a genuine null-result code path here.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The fix is well-scoped, correctly extracts shared display-building logic into buildUserDisplays(), adds a targeted getCurrentUserDisplay() action that avoids full inventory scans, and properly preserves Date serialization in redactUserKeys. Tests cover the critical happy path and regression scenarios.
PR Size: M
- Lines changed: 428 (284 additions, 144 deletions)
- Files changed: 5
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Claude AI
AI-authored draft PR
This draft PR was written by OpenAI Codex on behalf of Clouder0. It is intentionally marked as draft for maintainer review before it is made ready.
Summary
/api/v1/users:selfin the dashboard-compatibleUserDisplaylist shape, includingkeys.Datevalues for normal JSON serialization.Problem
In v0.8.2, a non-admin dashboard user can log in with a Web UI-enabled key, but opening the users page falls back from
/api/v1/usersto/api/v1/users:self. The self endpoint returned a user detail shape withoutkeys, while the dashboard users page expectsuser.keys.length, causing the global "Something went wrong!" error page.Solution
Add
getCurrentUserDisplay(), a targeted action that loads only the current session user and builds the sameUserDisplayshape used by the users list.listCurrentUsernow calls that action instead of the detail action or the full list action, so the response includes thekeysarray expected by the dashboard without turning an admin self request into an all-users scan.The response still goes through
redactUserKeys, and that redaction now preservesDateinstances so Hono/JSON serialization emits ISO strings instead of converting nested dates to{}.Changes
src/actions/users.tsbuildUserDisplays()logic fromgetUsers().getCurrentUserDisplay()for the single-current-user display shape.src/app/api/v1/resources/users/handlers.tsgetCurrentUserDisplay()in/api/v1/users:self.Datevalues while recursively removingfullKey.tests/api/v1/users/users.test.tskeys, redactfullKey, preserve date serialization, and do not call the full users action.tests/unit/users-action-get-users-compat.test.tsgetCurrentUserDisplay()only loads the current user id.tests/unit/api/v1/api-client-actions.test.tsReview follow-up
This PR includes a follow-up commit for reviewer feedback:
getUsers()in the self endpoint, addressing the N-to-1 full inventory load concern.Dateobjects inredactUserKeys, addressing date corruption during recursive redaction.pageInfo.limittied to the actual returned item count.One automated review note said the previous raw-array action result would fail because it lacked
ok. That is not how this code path behaves:callAction()wraps non-ActionResultreturns as{ ok: true, data }. The final implementation no longer relies on that path for/api/v1/users:self, but the original concern aboutresult.okwas not valid for this bridge.Verification
bunx biome check src/actions/users.ts src/app/api/v1/resources/users/handlers.ts tests/api/v1/users/users.test.ts tests/unit/users-action-get-users-compat.test.ts tests/unit/api/v1/api-client-actions.test.tsbunx vitest run tests/api/v1/users/users.test.ts tests/unit/users-action-get-users-compat.test.ts tests/unit/api/v1/api-client-actions.test.tsgit diff --check HEAD~1 HEADKnown local blockers outside this change
bun run lintcurrently fails on pre-existing formatting drift intests/integration/usage-ledger.test.ts.bun run typecheckandbun run buildcurrently fail because this checkout haspackage.jsondependencies not present inbun.lock/node_modules, includingmaplibre-gland Langfuse/OpenTelemetry packages.bun run test:v1currently fails becauseopenapi-typescriptis declared inpackage.jsonbut missing frombun.lock/node_modules.Greptile Summary
This PR fixes a dashboard crash for non-admin users who land on the users page: the
/api/v1/users:selfendpoint previously returned a user-detail shape without akeysarray, but the dashboard unconditionally readsuser.keys.length, causing the error page. The fix introducesgetCurrentUserDisplay()which returns the sameUserDisplaylist-item shape (includingkeys) thatgetUsers()produces, without triggering a full admin user-list scan.buildUserDisplays()is extracted fromgetUsers()as a shared helper;getCurrentUserDisplay()calls it with a single-element user array, andlistCurrentUsernow invokes this action instead ofgetUserById.redactUserKeysgains aDateinstance guard so that date fields (e.g.createdAt,expiresAt) are not converted to{}during recursive redaction — Hono's JSON serializer then correctly emits ISO strings.fullKeyredaction, date serialization, and confirm neithergetUserByIdnorgetUsersis called from the self path.Confidence Score: 5/5
Safe to merge — the fix is well-scoped and the new action correctly limits data access to the session user.
The three core changes (extracted
buildUserDisplays, newgetCurrentUserDisplayaction, and theDateguard inredactUserKeys) are all logically correct. The self endpoint no longer leaks the full user inventory for admin callers, date fields round-trip cleanly through redaction, and the keys array the dashboard requires is now present. Test coverage is targeted and covers the regression paths described in the PR.No files require special attention.
Important Files Changed
buildUserDisplays()fromgetUsers()and addsgetCurrentUserDisplay()— logic is correct and scoped to the session user.listCurrentUserfromgetUserByIdtogetCurrentUserDisplay, fixes Date handling inredactUserKeys, and adds a structural id-equality guard that is always false at runtime (benign).getCurrentUserDisplayloads only the session user viafindUserByIdand not the full user list.items/pageInfoand verifieskeysis passed through.Sequence Diagram
sequenceDiagram participant Dashboard participant APIClient participant Handler as listCurrentUser handler participant Action as getCurrentUserDisplay() participant DB Dashboard->>APIClient: getUsers() APIClient->>+Handler: GET /api/v1/users (403/non-admin) Handler-->>-APIClient: 403 Forbidden APIClient->>+Handler: GET /api/v1/users:self (fallback) Handler->>+Action: callAction(getCurrentUserDisplay) Action->>DB: getSession() → session.user.id Action->>DB: findUserById(session.user.id) DB-->>Action: User record Action->>DB: findKeyListBatch([userId]) Action->>DB: findKeyUsageTodayBatch([userId]) DB-->>Action: keys + usage Action->>DB: findKeysStatisticsBatchFromKeys(keysMap) DB-->>Action: statistics Action-->>-Handler: ActionResult with keys[] Handler->>Handler: redactUserKeys (Date instances preserved) Handler-->>-APIClient: "{ items: [UserDisplay], pageInfo }" APIClient-->>Dashboard: UserDisplay[] with keys arrayPrompt To Fix All With AI
Reviews (4): Last reviewed commit: "Merge branch 'dev' into codex/fix-user-m..." | Re-trigger Greptile