feat: hide IP column by default and reuse VirtualizedLogsTable in my-usage page#1029
Conversation
…usage page - IP column is now hidden by default in the request logs table; users can toggle it via the column visibility dropdown - my-usage page now uses the same VirtualizedLogsTable component as the dashboard logs page, with user/key/provider columns always hidden - Detail side-panel dialog is disabled on my-usage page (no decision chain or grouping info exposed) - New server action getMyUsageLogsBatchFull scoped to current key with allowReadOnlyAccess support - VirtualizedLogsTable gains fetchFn, queryKeyPrefix, and disableDetailDialog props for reuse flexibility Amp-Thread-ID: https://ampcode.com/threads/T-019d9b6e-07da-72af-8a57-3aee3dc20f44 Co-authored-by: Amp <amp@ampcode.com>
📝 WalkthroughWalkthrough新增 getMyUsageLogsBatchFull 后端操作，扩展 VirtualizedLogsTable 支持自定义 fetchFn/queryKeyPrefix/disableDetailDialog，UsageLogsSection 切换为该组件并使用日期范围解析，列可见性逻辑增加默认隐藏列并始终持久化设置。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
github-actions
Bot
added
enhancement
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/column-visibility.ts (1)
68-78:⚠️ Potential issue | 🟡 MinorJSON 解析失败时的回退与“无存储值”的回退不一致
当
localStorage中不存在偏好时返回[...DEFAULT_HIDDEN_COLUMNS](第 71 行),但当存储值损坏、JSON.parse抛错时却回退到[](第 77 行)。这意味着存储被污染的用户看到的默认可见列反而比全新用户更多(IP 列会显现),与本 PR 默认隐藏 IP 的意图相反。建议 catch 分支也返回[...DEFAULT_HIDDEN_COLUMNS]以保持一致。建议修改
} catch { - return []; + return [...DEFAULT_HIDDEN_COLUMNS]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/column-visibility.ts` around lines 68 - 78, The fallback for corrupted/malformed stored preferences is inconsistent: when no stored value the code returns [...DEFAULT_HIDDEN_COLUMNS] but in the catch block it returns [] — change the catch branch to return [...DEFAULT_HIDDEN_COLUMNS] instead so JSON.parse errors yield the same default as missing storage; locate the function that calls localStorage.getItem(getStorageKey(userId, tableId)), the JSON.parse(...) and the catch block and update its catch return to use DEFAULT_HIDDEN_COLUMNS (preserving the spread) while leaving the parsed.filter logic that uses DEFAULT_VISIBLE_COLUMNS intact.
🧹 Nitpick comments (3)
src/app/[locale]/my-usage/_components/usage-logs-section.tsx (1)
55-79: 重复实现:parseDateRange与src/actions/my-usage.ts的parseDateRangeInServerTimezone基本一致。两边都在做「YYYY-MM-DD → 服务器时区起始/次日 0 点毫秒数」的转换,逻辑一致但各自实现,后续维护(例如时区回落策略、日期校验)容易发散。建议抽取到
@/lib/utils/timezone(或类似共享位置)并在两处复用。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/my-usage/_components/usage-logs-section.tsx around lines 55 - 79, Duplicate date-range parsing exists: extract the shared logic into a common utility (e.g., export a function parseDateRangeInServerTimezone in a new module like `@/lib/utils/timezone`) and replace both local implementations with imports; ensure the utility implements the same behavior as the existing parseDateRange and the existing parseDateRangeInServerTimezone (YYYY-MM-DD → start-of-day ms and exclusive next-day ms in a given timezone), preserves the timezone fallback behavior and input validation (the YYYY-MM-DD regex), and update usages in src/app/[locale]/my-usage/_components/usage-logs-section.tsx (parseDateRange) and src/actions/my-usage.ts (parseDateRangeInServerTimezone) to call the new shared function.src/actions/my-usage.ts (1)
649-666: 建议:对来自客户端的params做运行时白名单,避免依赖Omit的编译期保证。
Omit<UsageLogBatchFilters, "userId" | "keyId" | "providerId">仅在编译期生效。作为 Server Action,运行时的params来自客户端序列化数据,客户端完全可以额外传入userId/providerId字段,随...params展开进入findUsageLogsBatch。虽然随后keyId: session.key.id会将查询约束到当前用户自己的 Key(假设底层查询以 AND 语义组合条件),使得数据并不会越权泄露,但userId/providerId过滤条件仍会被生效,语义上超出接口声明的能力。建议显式解构白名单字段(或直接丢弃userId/providerId)以与类型定义保持一致,增强防御性。建议的改动
export async function getMyUsageLogsBatchFull( params: Omit<UsageLogBatchFilters, "userId" | "keyId" | "providerId"> ): Promise<ActionResult<UsageLogsBatchResult>> { try { const session = await getSession({ allowReadOnlyAccess: true }); if (!session) return { ok: false, error: "Unauthorized" }; - const result = await findUsageLogsBatch({ - ...params, - keyId: session.key.id, - }); + // 丢弃客户端可能伪造的越权字段,仅信任声明过的过滤条件 + const { userId: _u, keyId: _k, providerId: _p, ...safeParams } = + params as UsageLogBatchFilters; + const result = await findUsageLogsBatch({ + ...safeParams, + keyId: session.key.id, + }); return { ok: true, data: result };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/my-usage.ts` around lines 649 - 666, The function getMyUsageLogsBatchFull accepts client-provided params typed as Omit<UsageLogBatchFilters, "userId" | "keyId" | "providerId"> but trusts the runtime payload; to fix, explicitly whitelist and extract only the allowed fields from params (e.g., page, limit, startDate, endDate, any other declared filter fields) into a new sanitized object and do not pass through userId or providerId, then call findUsageLogsBatch({ ...sanitizedParams, keyId: session.key.id }); ensure you reference getMyUsageLogsBatchFull, findUsageLogsBatch, UsageLogBatchFilters, and session.key.id when making the change.src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx (1)
72-86: 小优化:判空条件冗余。
if (statusCode === null || !statusCode)中,!statusCode已覆盖null/undefined/0,显式的=== null可以去掉;末尾的return默认分支与第一条分支重复,可以直接把前置判空合并到末尾的 fallback 处以减少重复样式字符串。非阻塞。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx around lines 72 - 86, The getStatusBadgeClassName function has a redundant null check and duplicated fallback branch; simplify by removing the initial if (statusCode === null || !statusCode) branch and let the final return act as the fallback for falsy values (null/undefined/0), keeping the existing 200–299, 400–499, and >=500 checks intact; update only the getStatusBadgeClassName function so it checks the numeric ranges first and returns the shared gray fallback string as the final return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:
- Around line 864-917: When disableDetailDialog is true the
ModelDisplayWithRedirect still receives onRedirectClick which only
setsDialogState and causes a silent no-op; change the rendering to omit the
onRedirectClick prop (or pass undefined) when disableDetailDialog is true so the
ModelDisplayWithRedirect renders non-interactive, and ensure cursor/aria
attributes reflect non-interactive state; likewise, apply the same conditional
removal for any ProviderChainPopover onChainItemClick usage to avoid
clickable-but-no-op elements (check symbols: disableDetailDialog,
ModelDisplayWithRedirect, onRedirectClick, setDialogState, ErrorDetailsDialog,
ProviderChainPopover).
---
Outside diff comments:
In `@src/lib/column-visibility.ts`:
- Around line 68-78: The fallback for corrupted/malformed stored preferences is
inconsistent: when no stored value the code returns [...DEFAULT_HIDDEN_COLUMNS]
but in the catch block it returns [] — change the catch branch to return
[...DEFAULT_HIDDEN_COLUMNS] instead so JSON.parse errors yield the same default
as missing storage; locate the function that calls
localStorage.getItem(getStorageKey(userId, tableId)), the JSON.parse(...) and
the catch block and update its catch return to use DEFAULT_HIDDEN_COLUMNS
(preserving the spread) while leaving the parsed.filter logic that uses
DEFAULT_VISIBLE_COLUMNS intact.
---
Nitpick comments:
In `@src/actions/my-usage.ts`:
- Around line 649-666: The function getMyUsageLogsBatchFull accepts
client-provided params typed as Omit<UsageLogBatchFilters, "userId" | "keyId" |
"providerId"> but trusts the runtime payload; to fix, explicitly whitelist and
extract only the allowed fields from params (e.g., page, limit, startDate,
endDate, any other declared filter fields) into a new sanitized object and do
not pass through userId or providerId, then call findUsageLogsBatch({
...sanitizedParams, keyId: session.key.id }); ensure you reference
getMyUsageLogsBatchFull, findUsageLogsBatch, UsageLogBatchFilters, and
session.key.id when making the change.
In `@src/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:
- Around line 72-86: The getStatusBadgeClassName function has a redundant null
check and duplicated fallback branch; simplify by removing the initial if
(statusCode === null || !statusCode) branch and let the final return act as the
fallback for falsy values (null/undefined/0), keeping the existing 200–299,
400–499, and >=500 checks intact; update only the getStatusBadgeClassName
function so it checks the numeric ranges first and returns the shared gray
fallback string as the final return.
In `@src/app/`[locale]/my-usage/_components/usage-logs-section.tsx:
- Around line 55-79: Duplicate date-range parsing exists: extract the shared
logic into a common utility (e.g., export a function
parseDateRangeInServerTimezone in a new module like `@/lib/utils/timezone`) and
replace both local implementations with imports; ensure the utility implements
the same behavior as the existing parseDateRange and the existing
parseDateRangeInServerTimezone (YYYY-MM-DD → start-of-day ms and exclusive
next-day ms in a given timezone), preserves the timezone fallback behavior and
input validation (the YYYY-MM-DD regex), and update usages in
src/app/[locale]/my-usage/_components/usage-logs-section.tsx (parseDateRange)
and src/actions/my-usage.ts (parseDateRangeInServerTimezone) to call the new
shared function.
🪄 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: e1886e7a-6222-4225-bda9-484d188d78d7
📒 Files selected for processing (6)
src/actions/my-usage.tssrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/my-usage/_components/usage-logs-section.test.tsxsrc/app/[locale]/my-usage/_components/usage-logs-section.tsxsrc/lib/column-visibility.test.tssrc/lib/column-visibility.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a22abf1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <VirtualizedLogsTable | ||
| filters={tableFilters} | ||
| hiddenColumns={MY_USAGE_HIDDEN_COLUMNS} | ||
| disableDetailDialog | ||
| fetchFn={myUsageFetchFn} |
There was a problem hiding this comment.
The my-usage page now mounts VirtualizedLogsTable without currencyCode or billingModelSource, so it falls back to the component defaults (USD / original). In environments where system_settings.currencyDisplay or billing source is customized, this will render incorrect cost and model-billing context compared with the previous implementation that used server-resolved settings; users will see wrong billing information after this migration.
Useful? React with 👍 / 👎.
| const result = await findUsageLogsBatch({ | ||
| ...params, | ||
| keyId: session.key.id, | ||
| }); |
There was a problem hiding this comment.
This new action uses findUsageLogsBatch for my-usage, but that query path does not apply the warmup exclusion used by the previous key-scoped batch flow. As a result, when warmup interception is enabled, internal probe rows (blockedBy='warmup') can now appear in end-user my-usage logs even though they are not counted in quota/stats, creating inconsistent and confusing usage history.
Useful? React with 👍 / 👎.
![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 refactors the usage logs section in the "my-usage" page to utilize the shared VirtualizedLogsTable component, enhancing consistency and performance. It introduces a new server action, getMyUsageLogsBatchFull, and extends VirtualizedLogsTable with props for custom fetch functions, query keys, and a simplified status display. The PR also updates column visibility logic to support default hidden columns (specifically the IP column) and ensures that empty visibility preferences are explicitly persisted. Feedback highlights a potential regression where currency and billing model metadata are no longer dynamically passed to the table, and suggests a more explicit check for status codes in the badge rendering logic.
| <VirtualizedLogsTable | ||
| filters={tableFilters} | ||
| hiddenColumns={MY_USAGE_HIDDEN_COLUMNS} | ||
| disableDetailDialog | ||
| fetchFn={myUsageFetchFn} | ||
| queryKeyPrefix="my-usage-logs-batch" | ||
| autoRefreshEnabled={!!autoRefreshSeconds} | ||
| autoRefreshIntervalMs={autoRefreshSeconds ? autoRefreshSeconds * 1000 : undefined} | ||
| /> |
There was a problem hiding this comment.
The VirtualizedLogsTable component is missing the currencyCode and billingModelSource props. Previously, these were dynamically retrieved from the log batch result. Without these props, the table will default to USD and original billing model, which may lead to incorrect data display for users with different settings. Please ensure these values are fetched (e.g., via getMyUsageMetadata) and passed to the table.
| } | ||
|
|
||
| function getStatusBadgeClassName(statusCode: number | null): string { | ||
| if (statusCode === null || !statusCode) { |
There was a problem hiding this comment.
The condition !statusCode will evaluate to true if statusCode is 0. While status code 0 usually indicates a network error or aborted request, it might be clearer to explicitly check for null or 0 if that is the intention, or just rely on statusCode === null if 0 should be handled differently.
| if (statusCode === null || !statusCode) { | |
| if (statusCode === null || statusCode === 0) { |
- catch block in getHiddenColumns returns DEFAULT_HIDDEN_COLUMNS for consistency with missing-storage fallback - getMyUsageLogsBatchFull strips userId/keyId/providerId from client params before spreading (defense-in-depth) - getStatusBadgeClassName simplified with extracted fallback constant and proper null-safe checks - onRedirectClick is undefined when disableDetailDialog is true to avoid clickable-but-no-op elements - Extract parseDateRangeToTimestamps to shared @/lib/utils/date-range - Pass currencyCode and billingModelSource to VirtualizedLogsTable in my-usage page via getMyUsageMetadata fetch Amp-Thread-ID: https://ampcode.com/threads/T-019d9b6e-07da-72af-8a57-3aee3dc20f44 Co-authored-by: Amp <amp@ampcode.com>
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/[locale]/my-usage/_components/usage-logs-section.tsx (1)
95-101: 次要：getMyUsageMetadata的 effect 缺少错误处理与竞态保护当前对
getMyUsageMetadata()的 Promise 只处理了成功分支，若请求 reject 会产生未捕获的 rejection；另外严格模式下 effect 会执行两次，虽依赖为[]不会重复触发业务问题，但如果未来新增依赖，缺少 "ignore on unmount" 标志可能导致旧响应覆盖新状态。可考虑补一个.catch及cancelled标志保持与上方两个请求一致的健壮性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/my-usage/_components/usage-logs-section.tsx around lines 95 - 101, The effect that calls getMyUsageMetadata() lacks rejection handling and a cancellation guard: update the effect that invokes getMyUsageMetadata() so it attaches a .catch(...) to log/handle errors and mirrors the other effects by introducing a local "cancelled" (or "ignore") flag checked before calling setCurrencyCode(...) and setBillingModelSource(...); ensure you flip the flag in the cleanup function to prevent stale responses from updating state after unmount.src/lib/utils/date-range.ts (1)
20-28: 可简化：重复的正则与冗余的if (match)分支第 20 行的
test已确保endDate满足YYYY-MM-DD，第 21 行再次exec后必然匹配，因此if (match)永远为真，可合并为一次exec，并且可以直接基于字符串构造 next-day 而不走Date.UTC。顺便也为startDate的无效日历日期（如2024-02-30）保留了通过的可能，当前实现对非法日期会静默产生NaN，若有需要可由调用方兜底。♻️ 建议的简化
- if (startDate && /^\d{4}-\d{2}-\d{2}$/.test(startDate)) { - startTime = fromZonedTime(`${startDate}T00:00:00`, tz).getTime(); - } - if (endDate && /^\d{4}-\d{2}-\d{2}$/.test(endDate)) { - const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(endDate); - if (match) { - const next = new Date(Date.UTC(Number(match[1]), Number(match[2]) - 1, Number(match[3]))); - next.setUTCDate(next.getUTCDate() + 1); - const nextStr = next.toISOString().slice(0, 10); - endTime = fromZonedTime(`${nextStr}T00:00:00`, tz).getTime(); - } - } + const dateRe = /^(\d{4})-(\d{2})-(\d{2})$/; + if (startDate && dateRe.test(startDate)) { + startTime = fromZonedTime(`${startDate}T00:00:00`, tz).getTime(); + } + const endMatch = endDate ? dateRe.exec(endDate) : null; + if (endMatch) { + const next = new Date( + Date.UTC(Number(endMatch[1]), Number(endMatch[2]) - 1, Number(endMatch[3]) + 1) + ); + const nextStr = next.toISOString().slice(0, 10); + endTime = fromZonedTime(`${nextStr}T00:00:00`, tz).getTime(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/date-range.ts` around lines 20 - 28, The code duplicates regex checking for endDate and has an unnecessary if (match); replace the two-step test+exec with a single exec(/^(\\d{4})-(\\d{2})-(\\d{2})$/) call on endDate, remove the redundant if branch, parse the captured year/month/day to construct a Date for the day in UTC, add one day, convert to ISO date string and call fromZonedTime to set endTime; additionally validate the resulting Date (isNaN) and bail/skip setting endTime on invalid calendar dates to avoid producing NaN. Reference symbols: endDate, endTime, fromZonedTime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/`[locale]/my-usage/_components/usage-logs-section.tsx:
- Around line 95-101: The effect that calls getMyUsageMetadata() lacks rejection
handling and a cancellation guard: update the effect that invokes
getMyUsageMetadata() so it attaches a .catch(...) to log/handle errors and
mirrors the other effects by introducing a local "cancelled" (or "ignore") flag
checked before calling setCurrencyCode(...) and setBillingModelSource(...);
ensure you flip the flag in the cleanup function to prevent stale responses from
updating state after unmount.
In `@src/lib/utils/date-range.ts`:
- Around line 20-28: The code duplicates regex checking for endDate and has an
unnecessary if (match); replace the two-step test+exec with a single
exec(/^(\\d{4})-(\\d{2})-(\\d{2})$/) call on endDate, remove the redundant if
branch, parse the captured year/month/day to construct a Date for the day in
UTC, add one day, convert to ISO date string and call fromZonedTime to set
endTime; additionally validate the resulting Date (isNaN) and bail/skip setting
endTime on invalid calendar dates to avoid producing NaN. Reference symbols:
endDate, endTime, fromZonedTime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1629d85d-1311-419c-9c8a-357da34ef983
📒 Files selected for processing (7)
src/actions/my-usage.tssrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/my-usage/_components/usage-logs-section.test.tsxsrc/app/[locale]/my-usage/_components/usage-logs-section.tsxsrc/lib/column-visibility.test.tssrc/lib/column-visibility.tssrc/lib/utils/date-range.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/app/[locale]/my-usage/_components/usage-logs-section.test.tsx
- src/lib/column-visibility.test.ts
- src/lib/column-visibility.ts
- src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
- src/actions/my-usage.ts
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (!session) return { ok: false, error: "Unauthorized" }; | ||
|
|
||
| const result = await findUsageLogsBatch({ | ||
| ...params, |
There was a problem hiding this comment.
[High] [PERFORMANCE-ISSUE] getMyUsageLogsBatchFull forwards unvalidated limit
Why this is a problem: params.limit is forwarded directly into the DB query path. findUsageLogsBatch uses limit to compute fetchLimit = limit + 1 and then applies .limit(fetchLimit), so negative or very large limit values can trigger query errors or resource exhaustion.
Evidence:
src/actions/my-usage.ts:const result = await findUsageLogsBatch({ ...params, keyId: session.key.id, });src/repository/usage-logs.ts:const { userId, keyId, providerId, cursor, limit = 50 } = filters;src/repository/usage-logs.ts:const fetchLimit = limit + 1;src/repository/usage-logs.ts:.limit(fetchLimit);
Suggested fix:
const resolvedLimit = params.limit && params.limit > 0 ? Math.min(params.limit, 100) : 50;
const result = await findUsageLogsBatch({
...params,
limit: resolvedLimit,
keyId: session.key.id,
});
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a083ec8cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const result = await findUsageLogsBatch({ | ||
| ...safeParams, | ||
| keyId: session.key.id, | ||
| }); |
There was a problem hiding this comment.
getMyUsageLogsBatchFull now returns findUsageLogsBatch results verbatim, which include diagnostic fields like provider chain/error routing context and client metadata intended for the admin detail dialog. Although the my-usage UI disables that dialog, these fields are still delivered to the browser payload and can be inspected by end users, which is a data-exposure regression compared with the previous slim my-usage response.
Useful? React with 👍 / 👎.
|
|
||
| const BATCH_SIZE = 20; | ||
| /** Columns always hidden on my-usage page (user/key/provider not available) */ | ||
| const MY_USAGE_HIDDEN_COLUMNS: LogsTableColumn[] = ["user", "key", "provider"]; |
There was a problem hiding this comment.
The migrated my-usage table only hides user/key/provider, so the shared table will render the IP column by default (hideIpColumn stays false). This exposes client IPs in my-usage where the old table did not show an IP column, and it also contradicts the privacy goal of making IP non-default visibility.
Useful? React with 👍 / 👎.
| /> | ||
| <div className="rounded-lg border border-border/60 overflow-hidden"> | ||
| <VirtualizedLogsTable | ||
| filters={tableFilters} |
There was a problem hiding this comment.
[High] [LOGIC-BUG] my-usage logs table defaults to USD currency formatting
Why this is a problem: VirtualizedLogsTable formats cost using its currencyCode prop, but UsageLogsSection does not pass one here. That means the table will always render with the component default (currencyCode = "USD"), which can diverge from the rest of the my-usage page when the system currency display is not USD.
Evidence:
src/app/[locale]/my-usage/_components/usage-logs-section.tsx:<VirtualizedLogsTable ... />is rendered withoutcurrencyCode.src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx: default prop iscurrencyCode = "USD".
Suggested fix (one minimal option: load currency once and pass through):
// usage-logs-section.tsx
import { getMyUsageMetadata } from "@/actions/my-usage";
import type { CurrencyCode } from "@/lib/utils/currency";
const [currencyCode, setCurrencyCode] = useState<CurrencyCode>("USD");
useEffect(() => {
void getMyUsageMetadata().then((result) => {
if (result.ok) setCurrencyCode(result.data.currencyCode);
});
}, []);
// ...
<VirtualizedLogsTable
filters={tableFilters}
currencyCode={currencyCode}
hiddenColumns={MY_USAGE_HIDDEN_COLUMNS}
disableDetailDialog
fetchFn={myUsageFetchFn}
queryKeyPrefix="my-usage-logs-batch"
autoRefreshEnabled={!!autoRefreshSeconds}
autoRefreshIntervalMs={autoRefreshSeconds ? autoRefreshSeconds * 1000 : undefined}
/>
| useInfiniteQuery({ | ||
| queryKey: ["usage-logs-batch", filters], | ||
| queryKey: [queryKeyPrefix, filters], | ||
| queryFn: async ({ pageParam }) => { |
There was a problem hiding this comment.
[Medium] [TEST-MISSING-CRITICAL] New VirtualizedLogsTable props are not covered by unit tests
Why this is a problem: This PR adds new behavioral branches (fetchFn, queryKeyPrefix, disableDetailDialog). Without tests that exercise these branches, regressions (wrong fetcher, cache key collisions, status cell behavior) are easy to introduce.
Evidence:
src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:queryKey: [queryKeyPrefix, filters]andconst fetcher = fetchFn ?? getUsageLogsBatch;src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:disableDetailDialog ? <StatusBadgeOnly ... /> : <ErrorDetailsDialog ... />
Suggested fix (add focused unit tests):
// virtualized-logs-table.test.tsx
test("uses fetchFn and queryKeyPrefix when provided", async () => {
mockIsLoading = false;
mockIsError = false;
mockError = null;
mockHasNextPage = false;
mockIsFetchingNextPage = false;
mockLogs = [makeLog({ id: 1 })];
useInfiniteQuerySpy.mockClear();
const fetchFn = vi.fn().mockResolvedValue({
ok: true,
data: { logs: [], nextCursor: null, hasMore: false },
});
renderToStaticMarkup(
<VirtualizedLogsTable
filters={{}}
autoRefreshEnabled={false}
fetchFn={fetchFn}
queryKeyPrefix="my-usage-logs-batch"
/>
);
const options = useInfiniteQuerySpy.mock.calls[0]?.[0] as {
queryKey: unknown[];
queryFn: (ctx: { pageParam?: unknown }) => Promise<unknown>;
};
expect(options.queryKey[0]).toBe("my-usage-logs-batch");
await options.queryFn({ pageParam: undefined });
expect(fetchFn).toHaveBeenCalledWith(expect.objectContaining({ limit: 50 }));
});
test("renders status badge only when disableDetailDialog=true", () => {
mockIsLoading = false;
mockIsError = false;
mockError = null;
mockHasNextPage = false;
mockIsFetchingNextPage = false;
mockLogs = [makeLog({ id: 1, statusCode: 200 })];
const htmlWithDialog = renderToStaticMarkup(
<VirtualizedLogsTable filters={{}} autoRefreshEnabled={false} />
);
expect(htmlWithDialog).toContain('data-slot="error-details-dialog"');
const htmlNoDialog = renderToStaticMarkup(
<VirtualizedLogsTable filters={{}} autoRefreshEnabled={false} disableDetailDialog />
);
expect(htmlNoDialog).not.toContain('data-slot="error-details-dialog"');
expect(htmlNoDialog).toContain("200");
});
There was a problem hiding this comment.
Code Review Summary
This PR introduces a new full-format my-usage logs fetch path and extends VirtualizedLogsTable with customizable fetching/query keys plus a status-only mode. Two high-risk gaps remain in the new my-usage wiring (limit validation and currency formatting), and the new VirtualizedLogsTable branches lack unit coverage.
PR Size: L
- Lines changed: 582
- Files changed: 6
- Split suggestion: (1)
column-visibilitydefault behavior + tests, (2)VirtualizedLogsTablenew props + tests, (3) my-usage page refactor + server action.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 1 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
src/actions/my-usage.ts:getMyUsageLogsBatchFullforwards an unvalidatedlimitintofindUsageLogsBatch(risk: query errors / resource exhaustion).src/app/[locale]/my-usage/_components/usage-logs-section.tsx: my-usageVirtualizedLogsTableusage does not passcurrencyCode, so cost formatting defaults to USD.src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx: new props (fetchFn,queryKeyPrefix,disableDetailDialog) are not exercised by unit tests.
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
#1029and applied labelsize/L. - Posted 3 inline comments (2 High, 1 Medium) on:
src/actions/my-usage.ts(unvalidated/clamp-lesslimitforwarded intofindUsageLogsBatch)src/app/[locale]/my-usage/_components/usage-logs-section.tsx(VirtualizedLogsTablecost formatting defaults toUSDbecausecurrencyCodeisn’t passed)src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx(newfetchFn/queryKeyPrefix/disableDetailDialogbranches lack unit tests)
- Submitted the required summary review via
gh pr review --comment.
1 participant
Summary
Hide the IP column by default in the request logs table (users can still toggle it via the column visibility dropdown), and replace the my-usage page's custom
UsageLogsTablewith the sharedVirtualizedLogsTablecomponent for consistent UX and reduced maintenance burden.Problem
IP column privacy/usability: PR feat: IP 记录 & 审计日志 & IP 归属地查询 #1027 introduced IP logging and display in the logs table. IP addresses should not be prominently displayed by default — they are sensitive data that most users don't need to see on every page load.
Duplicated my-usage table logic: The my-usage page used a separate
UsageLogsTablecomponent with its own pagination, rendering, and auto-refresh logic, duplicating much of whatVirtualizedLogsTablealready handles. PR fix: stabilize usage log virtual scrolling #959 partially addressed this with virtual scrolling, but the my-usage page still had its own implementation rather than reusing the shared component.Related PRs:
VirtualizedLogsTableDEFAULT_HIDDEN_COLUMNSSolution
IP column hidden by default
DEFAULT_HIDDEN_COLUMNSconstant incolumn-visibility.ts(initially["ip"])getHiddenColumns()returns the defaults instead of an empty arraysetHiddenColumns()now always writes to localStorage (even empty arrays) to distinguish "no preference" from "explicitly chose to show all"my-usage page uses VirtualizedLogsTable
UsageLogsTablewith the sharedVirtualizedLogsTablecomponentgetMyUsageLogsBatchFullserver action scoped to the current key withallowReadOnlyAccess, returning the fullUsageLogsBatchResultshape expected byVirtualizedLogsTableVirtualizedLogsTable new props
fetchFn: custom data fetch function (defaults togetUsageLogsBatch)queryKeyPrefix: custom react-query key prefix (defaults to"usage-logs-batch")disableDetailDialog: renders a plain status badge without the Sheet triggerChanges
src/lib/column-visibility.tsDEFAULT_HIDDEN_COLUMNS, updategetHiddenColumns/setHiddenColumnssrc/lib/column-visibility.test.tssrc/app/.../virtualized-logs-table.tsxfetchFn,queryKeyPrefix,disableDetailDialogprops; extractStatusBadgeOnlysrc/app/.../usage-logs-section.tsxUsageLogsTablewithVirtualizedLogsTable; simplify filters and headersrc/app/.../usage-logs-section.test.tsxVirtualizedLogsTableintegrationsrc/actions/my-usage.tsgetMyUsageLogsBatchFullserver actionTesting
column-visibility.test.ts— verifiesDEFAULT_HIDDEN_COLUMNSbehavior, empty-array persistence, and toggle interactions with defaultsusage-logs-section.test.tsx— verifiesVirtualizedLogsTableis rendered with correcthiddenColumns(user/key/provider),disableDetailDialog, customfetchFn, andqueryKeyPrefixDescription enhanced by Claude AI
Greptile Summary
This PR makes two focused improvements: it hides the IP column by default using a new
DEFAULT_HIDDEN_COLUMNSconstant incolumn-visibility.ts, and replaces the my-usage page's customUsageLogsTablewith the sharedVirtualizedLogsTableby addingfetchFn,queryKeyPrefix, anddisableDetailDialogprops. A newgetMyUsageLogsBatchFullserver action scopes data to the current session's key and a sharedparseDateRangeToTimestampsutility is extracted to@/lib/utils/date-range. The implementation is clean with no P0/P1 issues found.Confidence Score: 5/5
Safe to merge; no P0 or P1 issues found. The IP-hiding default and my-usage VirtualizedLogsTable migration are both correctly implemented.
All findings are P2: a naming ambiguity for DEFAULT_VISIBLE_COLUMNS and a code-duplication opportunity between the extracted date-range utility and the older private helper in my-usage.ts. The authorization scoping in getMyUsageLogsBatchFull is correct (keyId forced from session, userId/keyId/providerId stripped at runtime), test coverage for the changed logic is thorough, and the serverTimeZone dependency in useMemo correctly recomputes tableFilters when the timezone loads.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[getHiddenColumns called] --> B{localStorage entry exists?} B -- No --> C[return DEFAULT_HIDDEN_COLUMNS copy] B -- Yes --> D[JSON.parse stored value] D --> E{Parse error?} E -- Yes --> C E -- No --> F[filter against all toggleable columns] F --> G[return filtered array] H[setHiddenColumns called] --> I[always write to localStorage\neven empty array] I --> J[distinguishes no-pref from show-all] K[resetColumns] --> L[setHiddenColumns with empty array] L --> M[next getHiddenColumns returns empty\nnot DEFAULT_HIDDEN_COLUMNS] subgraph my-usage page N[UsageLogsSection] --> O[VirtualizedLogsTable\nfetchFn=getMyUsageLogsBatchFull\nhiddenColumns=user,key,provider\ndisableDetailDialog=true] O --> P[getMyUsageLogsBatchFull\nserver action] P --> Q[strip userId/keyId/providerId\nfrom params] Q --> R[findUsageLogsBatch\nkeyId = session.key.id] endComments Outside Diff (2)
src/lib/column-visibility.ts, line 76-78 (link)When
localStoragecontains corrupt data, thecatchblock returns[](all columns visible), while the "no data stored" path returns[...DEFAULT_HIDDEN_COLUMNS](IP hidden). A user whose storage is silently corrupted would see the IP column appear unexpectedly without ever having changed a preference.Prompt To Fix With AI
src/lib/column-visibility.ts, line 147-149 (link)resetColumnspersists[], sogetHiddenColumnsreturns[]afterward — IP becomes visible even thoughDEFAULT_HIDDEN_COLUMNSestablishes IP as hidden for first-time users. Users clicking "Reset" may expect to return to the configured default (IP hidden), not an "all columns visible" state.If "reset to default" is the desired semantic, delete the stored key instead of writing
[]:If "show everything" is intentional (as described in the PR), consider renaming to
showAllColumnsor adding a comment to make the intent clear to future contributors.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: address PR review comments" | Re-trigger Greptile