fix(ui): resolve Issue #428 & #429 - filter dropdown and pagination bugs#436
fix(ui): resolve Issue #428 & #429 - filter dropdown and pagination bugs#436
Conversation
Issue #428: User/Provider filter dropdown unusable with 100+ items - Replace <Select> with Popover + Command (searchable combobox) - Add search filtering, keyboard navigation, clear selection - Add i18n translations for all 5 locales Issue #429: User list pagination shows duplicate records - Fix timestamp precision loss (microseconds -> milliseconds) - Use PostgreSQL to_char() to preserve full 6-digit precision - Apply same fix to usage-logs pagination Files changed: - src/app/[locale]/dashboard/logs/_components/usage-logs-filters.tsx - src/repository/user.ts - src/repository/usage-logs.ts - messages/*/dashboard.json (5 locales) Closes #428, Closes #429 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two critical UI and data integrity issues. It significantly improves the user experience for filtering large lists of users and providers by introducing searchable dropdowns, and it resolves a data pagination bug that led to duplicate entries by ensuring precise timestamp handling in database queries. Highlights
|
![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 effectively resolves two separate issues. The UI is improved by replacing standard select dropdowns with searchable comboboxes, which enhances usability for lists with many items. The pagination bug due to timestamp precision loss is correctly fixed at the database query level. The code is well-structured. I have a couple of suggestions to optimize the new combobox components for better performance with large datasets by memoizing lookups, which aligns with the goal of this PR.
| {localFilters.userId ? ( | ||
| (users.find((user) => user.id === localFilters.userId)?.name ?? | ||
| localFilters.userId.toString()) | ||
| ) : ( | ||
| <span className="text-muted-foreground"> | ||
| {isUsersLoading ? t("logs.stats.loading") : t("logs.filters.allUsers")} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
The current implementation uses users.find() to get the selected user's name on every render. This can be inefficient for large lists of users, which is the scenario this PR addresses. A linear search on every render could lead to performance issues.
To optimize this, you can create a Map of user IDs to names using useMemo. This will provide a more performant O(1) lookup.
Add this to your component:
const userMap = useMemo(() => new Map(users.map((user) => [user.id, user.name])), [users]);| {localFilters.userId ? ( | |
| (users.find((user) => user.id === localFilters.userId)?.name ?? | |
| localFilters.userId.toString()) | |
| ) : ( | |
| <span className="text-muted-foreground"> | |
| {isUsersLoading ? t("logs.stats.loading") : t("logs.filters.allUsers")} | |
| </span> | |
| )} | |
| {localFilters.userId ? ( | |
| userMap.get(localFilters.userId) ?? localFilters.userId.toString() | |
| ) : ( | |
| <span className="text-muted-foreground"> | |
| {isUsersLoading ? t("logs.stats.loading") : t("logs.filters.allUsers")} | |
| </span> | |
| )} |
| {localFilters.providerId ? ( | ||
| (providers.find((provider) => provider.id === localFilters.providerId)?.name ?? | ||
| localFilters.providerId.toString()) | ||
| ) : ( | ||
| <span className="text-muted-foreground"> | ||
| {isProvidersLoading | ||
| ? t("logs.stats.loading") | ||
| : t("logs.filters.allProviders")} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
Similar to the user filter, providers.find() is used here on every render, which can be inefficient for a large number of providers. This can be optimized by using a Map for O(1) lookups.
Add this to your component:
const providerMap = useMemo(() => new Map(providers.map((provider) => [provider.id, provider.name])), [providers]);| {localFilters.providerId ? ( | |
| (providers.find((provider) => provider.id === localFilters.providerId)?.name ?? | |
| localFilters.providerId.toString()) | |
| ) : ( | |
| <span className="text-muted-foreground"> | |
| {isProvidersLoading | |
| ? t("logs.stats.loading") | |
| : t("logs.filters.allProviders")} | |
| </span> | |
| )} | |
| {localFilters.providerId ? ( | |
| providerMap.get(localFilters.providerId) ?? | |
| localFilters.providerId.toString() | |
| ) : ( | |
| <span className="text-muted-foreground"> | |
| {isProvidersLoading | |
| ? t("logs.stats.loading") | |
| : t("logs.filters.allProviders")} | |
| </span> | |
| )} |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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
This PR addresses Issues #428 and #429, implementing searchable Combobox filters and fixing cursor pagination precision. However, a critical bug was introduced that will cause runtime errors.
PR Size: M
- Lines changed: 238 (182 additions, 56 deletions)
- Files changed: 9
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
1. [LOGIC-BUG] Variable renamed but usages not updated - ReferenceError
File: src/app/[locale]/dashboard/_components/user/forms/key-edit-section.tsx:292
Confidence: 100
The variable extraKeyGroupOption is renamed to _extraKeyGroupOption in the diff:
- const extraKeyGroupOption = useMemo(() => {
+ const _extraKeyGroupOption = useMemo(() => {However, the template still references the old name at lines 438, 439, 443, and 445:
{extraKeyGroupOption ? (
<SelectItem value={extraKeyGroupOption}>
<Badge ... title={extraKeyGroupOption}>
{extraKeyGroupOption}
</Badge>
</SelectItem>
) : null}This will cause a ReferenceError: extraKeyGroupOption is not defined at runtime when the KeyEditSection component renders for non-admin users with provider groups.
Suggested fix:
// Either revert the rename (if the variable is still used):
const extraKeyGroupOption = useMemo(() => {
// ...
}, [normalizedKeyProviderGroup, normalizedUserProviderGroup, userGroups]);
// Or rename ALL references to _extraKeyGroupOptionOther Changes Reviewed (No Issues Found)
-
usage-logs-filters.tsx: The Popover + Command implementation for User/Provider filters looks correct. Proper handling of loading states, selection clearing, and search functionality. -
user.tsandusage-logs.ts: The cursor pagination fix usingto_char(..., 'US')is a good solution to preserve microsecond precision and prevent duplicate records during pagination. -
Translation files: All 5 locales updated consistently with matching keys.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Claude AI
| providerGroup: users.providerGroup, | ||
| tags: users.tags, | ||
| createdAt: users.createdAt, | ||
| createdAtRaw: sql<string>`to_char(${users.createdAt}, 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`, |
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] createdAtRaw formats timestamptz in session timezone but appends Z (UTC), breaking keyset cursors outside UTC.
Evidence (src/repository/user.ts:189):
createdAtRaw: sql<string>\to_char(${users.createdAt}, 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`,`
Why this is a problem: to_char(timestamptz, ...) uses the DB session TimeZone for formatting; the literal "Z" claims the value is UTC. If TimeZone ≠ UTC, the cursor string is shifted, and (${users.createdAt}, ${users.id}) > (${cursor.createdAt}::timestamptz, ...) can skip/duplicate rows.
Suggested fix:
createdAtRaw: sql<string>`
to_char(${users.createdAt} AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')
`,
| .select({ | ||
| id: messageRequest.id, | ||
| createdAt: messageRequest.createdAt, | ||
| createdAtRaw: sql<string>`to_char(${messageRequest.createdAt}, 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`, |
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] createdAtRaw formats timestamptz in session timezone but appends Z (UTC), so nextCursor can point at the wrong rows when DB TimeZone ≠ UTC.
Evidence (src/repository/usage-logs.ts:187):
createdAtRaw: sql<string>\to_char(${messageRequest.createdAt}, 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`,`
Why this is a problem: to_char(timestamptz, ...) uses the connection/session TimeZone to render the timestamp, but the format hardcodes "Z". In non-UTC deployments this produces a cursor string that is not the same instant, and (${messageRequest.createdAt}, ${messageRequest.id}) < (${cursor.createdAt}::timestamptz, ...) can skip/duplicate log rows.
Suggested fix:
createdAtRaw: sql<string>`
to_char(
${messageRequest.createdAt} AT TIME ZONE 'UTC',
'YYYY-MM-DD"T"HH24:MI:SS.US"Z"'
)
`,
There was a problem hiding this comment.
Code Review Summary
This PR introduces a critical correctness risk in the new keyset-cursor formatting for createdAt: the cursor string is formatted in the DB session timezone but labeled as UTC (Z). In non-UTC deployments, pagination can still skip/duplicate rows despite the intended microsecond fix.
PR Size: M
- Lines changed: 238
- Files changed: 9
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 2 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/repository/user.ts:189[LOGIC-BUG]to_char(timestamptz, ...)+ literal"Z"can mislabel time as UTC and break cursors (Confidence: 95)src/repository/usage-logs.ts:187[LOGIC-BUG]to_char(timestamptz, ...)+ literal"Z"can mislabel time as UTC and break cursors (Confidence: 95)
High Priority Issues (Should Fix)
- None.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#436and applied labelsize/M(238 lines changed, 9 files). - Left 2 Critical inline comments on cursor timestamp formatting that can break pagination when DB
TimeZone≠ UTC:src/repository/user.ts:189andsrc/repository/usage-logs.ts:187. - Submitted the required PR review summary via
gh pr review --comment.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
- Fix to_char timezone issue in cursor pagination (user.ts:189, usage-logs.ts:187) - Add AT TIME ZONE 'UTC' to ensure consistent UTC output - Prevents cursor mismatch in non-UTC deployments - Optimize filter dropdown lookup performance (usage-logs-filters.tsx) - Add userMap/providerMap useMemo for O(1) name lookup - Replace Array.find() with Map.get() for 100+ item scenarios Addresses PR #436 review feedback from Codex AI Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
<Select>with searchable Combobox (Popover + Command)to_char()to preserve microsecondsChanges
usage-logs-filters.tsxuser.tsto_char(..., 'US')usage-logs.tsmessages/*/dashboard.jsonTest plan
Closes #428, Closes #429
Generated with Claude Code