fix: align prefix session monitoring and usage logs - #1375
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanges会话身份与活动状态
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .groupBy(messageSessionIdentity, messageRequest.sessionId) | ||
| .orderBy(desc(sql`min(${messageRequest.createdAt})`)) | ||
| .limit(limit); | ||
|
|
||
| return results.map((r) => r.sessionId).filter((id): id is string => Boolean(id)); | ||
| const suggestions = new Set<string>(); | ||
| for (const row of results) { | ||
| if (row.sessionId) suggestions.add(row.sessionId); |
There was a problem hiding this comment.
When the term matches physical client IDs but not their canonical identities, this loop inserts each nonmatching canonical ID first and truncates the expanded results to limit, causing unrelated suggestions to displace the matching client session IDs.
Knowledge Base Used: Database Schema & Repository Layer
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/usage-logs.ts
Line: 1708-1714
Comment:
**Suggestion limit drops matching IDs**
When the term matches physical client IDs but not their canonical identities, this loop inserts each nonmatching canonical ID first and truncates the expanded results to `limit`, causing unrelated suggestions to displace the matching client session IDs.
**Knowledge Base Used:** [Database Schema & Repository Layer](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/database-schema.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const messageSourceSessionIds = sql<string[]>` | ||
| ARRAY_AGG(${messageRequest.sessionId}) FILTER (WHERE ${messageRequest.sessionId} IS NOT NULL) | ||
| OVER (PARTITION BY ${messageSessionIdentity}) | ||
| `; | ||
| const ledgerSourceSessionIds = sql<string[]>` | ||
| ARRAY_AGG(${usageLedger.sessionId}) FILTER (WHERE ${usageLedger.sessionId} IS NOT NULL) | ||
| OVER (PARTITION BY ${ledgerSessionIdentity}) | ||
| `; |
There was a problem hiding this comment.
If a long-lived canonical identity accumulates many request or ledger rows, these window aggregates materialize its complete physical-ID array before pagination and attach it to every selected row, increasing database memory, query time, and response size as the session grows.
Knowledge Base Used: Database Schema & Repository Layer
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/usage-logs.ts
Line: 68-75
Comment:
**Session arrays bypass pagination bounds**
If a long-lived canonical identity accumulates many request or ledger rows, these window aggregates materialize its complete physical-ID array before pagination and attach it to every selected row, increasing database memory, query time, and response size as the session grows.
**Knowledge Base Used:** [Database Schema & Repository Layer](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/database-schema.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![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
🤖 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 `@src/app/`[locale]/dashboard/logs/_components/usage-logs-table.tsx:
- Around line 197-212: In
src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx lines 197-212,
create one fallback source-ID list before rendering and use it for the canonical
sessionId includes check; apply the same change in
src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx lines
849-864 so a fallback sourceSessionId is not displayed again as sessionId.
In `@src/repository/activity-stream.ts`:
- Around line 157-160: 在活动流查询构建逻辑中更新条件添加处，移除针对 messageRequest.sessionId 的
notInArray 条件，仅保留基于 messageSessionIdentity 和 excludedSessionIds 的排除判断，确保 NULL
的物理 session ID 不会因 SQL 三值逻辑被过滤。
In `@src/repository/usage-logs.ts`:
- Around line 68-75: 在 usage log 查询结果的共享映射层统一去重 sourceSessionIds，覆盖
findUsageLogsBatch、findUsageLogsWithDetails 及其他查询路径；确保每个物理 session ID
只保留一个元素，可通过使 ARRAY_AGG 聚合唯一或在映射时使用 Set，并保持非空过滤行为不变。
In `@tests/unit/actions/active-sessions-monitoring.test.ts`:
- Around line 53-54: 更新测试 fixture 中的 firstRequestAt 和
lastRequestAt，改用相对于当前测试时钟的时间，或在相关测试中通过 Vitest 固定系统时间；确保 getAllSessions 执行时
fixture 不会因固定日期超过五分钟而被归类为 inactive。
🪄 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 Plus
Run ID: 11a6c30d-fb0e-4e63-9357-bd96f4a1b0e1
📒 Files selected for processing (12)
src/actions/active-sessions.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/repository/_shared/usage-log-filters.tssrc/repository/activity-stream.tssrc/repository/usage-logs.tstests/unit/actions/active-sessions-monitoring.test.tstests/unit/repository/activity-stream-replay.test.tstests/unit/repository/usage-logs-sessionid-filter.test.tstests/unit/repository/usage-logs-sessionid-suggestions.test.ts
b7b1876 to
a6acd60
Compare
![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: b7b187627e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sourceSessionIds: sql< | ||
| string[] | ||
| >`ARRAY_AGG(${messageRequest.sessionId}) FILTER (WHERE ${messageRequest.sessionId} IS NOT NULL) OVER (PARTITION BY ${messageSessionIdentity})`, |
There was a problem hiding this comment.
When a prefix identity accumulates many requests, this window aggregate adds one entry per request rather than one per physical session, and PostgreSQL must evaluate the filtered partition before applying the page limit. Consequently, a page of 50 rows for an identity with N requests can return roughly 50×N session-ID values; the tables then render the array directly, producing repeated tooltip entries, duplicate React keys, and potentially very large queries and API payloads. Resolve the distinct source IDs separately instead of attaching the full per-request window aggregate to every log row.
Useful? React with 👍 / 👎.
| const results = await query | ||
| .where(and(...conditions)) | ||
| .groupBy(messageSessionIdentity) | ||
| .groupBy(messageSessionIdentity, messageRequest.sessionId) |
There was a problem hiding this comment.
When a prefix identity has multiple physical session IDs, grouping by the canonical/source pair produces multiple SQL rows for that one identity, but the query applies limit before the pairs are expanded and deduplicated in JavaScript. A single canonical identity can therefore consume the entire query limit and hide other matching identities; expansion can also create up to twice the limit and the final slice arbitrarily discards later matching source IDs. Build a distinct ordered set of canonical and physical suggestions before applying the limit.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| s.totalCacheReadTokens, | ||
| costUsd: s.totalCostUsd, | ||
| status: "completed", | ||
| status: (concurrentCounts.get(s.sessionId) ?? 0) > 0 ? "in_progress" : "completed", |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] In-progress sessions can still be returned in the inactive page
Why this is a problem: status now comes from getObservedConcurrentCountBatch(...), but this loop still buckets rows into active/inactive only by lastRequestAt. A request that has been running for more than five minutes will be marked in_progress here and still land in inactive, and the inactive table later zeroes concurrentCount, so the monitoring UI renders an actually busy session as idle.
Suggested fix:
const concurrentCount = concurrentCounts.get(s.sessionId) ?? 0;
const isCurrentlyActive = concurrentCount > 0 || lastRequestTime >= fiveMinutesAgo;
const sessionInfo: ActiveSessionInfo = {
// ...
status: concurrentCount > 0 ? "in_progress" : "completed",
concurrentCount,
};
if (isCurrentlyActive) {
active.push(sessionInfo);
} else {
inactive.push(sessionInfo);
}
| createdAtRaw: sql<string>`to_char(${messageRequest.createdAt} AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.US"Z"')`, | ||
| sessionId: messageSessionIdentity, | ||
| sourceSessionId: messageRequest.sessionId, | ||
| sourceSessionIds: sql< |
There was a problem hiding this comment.
[MEDIUM] [LOGIC-BUG] sourceSessionIds is duplicated once per request in the batch API
Why this is a problem: ARRAY_AGG(${messageRequest.sessionId}) OVER (PARTITION BY ...) collects every row in the identity partition, so repeated requests from the same client session produce repeated IDs. findUsageLogsBatch() returns that array unchanged, and the virtualized logs tooltip renders each entry, so a busy prefix-affinity session shows the same client session ID many times and inflates the response payload.
Suggested fix:
return {
...row,
sourceSessionIds: row.sourceSessionIds ? [...new Set(row.sourceSessionIds)] : undefined,
requestSequence: row.requestSequence ?? null,
// ...
};
There was a problem hiding this comment.
Code Review Summary
I found two user-visible regressions in the new session-monitoring / usage-log identity work. Both are in changed code paths and affect the dashboard behavior directly: one can hide live sessions from the active list, and the other can flood the virtualized usage-log tooltip with duplicate client IDs.
PR Size: M
- Lines changed: 326
- Files changed: 12
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 1 | 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)
- None.
High Priority Issues (Should Fix)
- [HIGH] [LOGIC-BUG]
src/actions/active-sessions.ts:468marks a session asin_progressfrom observed concurrency but still leaves active/inactive paging based only onlastRequestAt, so long-running requests can disappear from the active section and render as idle in the inactive table. - [MEDIUM] [LOGIC-BUG]
src/repository/usage-logs.ts:230addssourceSessionIdsto the batch API without normalizing duplicates, so the virtualized usage-log tooltip can show the same client session ID once per request and unnecessarily bloat the response.
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
#1375, applied thesize/Mlabel, and posted the review summary on GitHub. -
Posted a high-priority inline comment on
src/actions/active-sessions.ts:468for a monitoring regression: sessions can be markedin_progressbut still get paged into the inactive section, which makes long-running live sessions appear idle. -
Posted a medium-severity inline comment on
src/repository/usage-logs.ts:230for duplicatedsourceSessionIdsin the cursor-based batch path, which can flood the virtualized usage-log tooltip with repeated client IDs and bloat the response. -
Summary status:
1high issue,1medium issue, no security or error-handling findings above threshold. -
If you want, I can also draft the minimal code changes to address both findings.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx (1)
206-212: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value跨页合并使用覆盖语义。
Object.fromEntries对同一 identity 保留最后一页的值，不做合并。当前仓储层按 identity 做全量聚合，各页返回的集合相同，因此显示结果正确。如果后续 hydration 改为限定在页面行范围内，覆盖会丢失前面页已获得的物理会话 ID。建议改为按 identity 做并集累加。♻️ 建议改为并集累加
- const sourceSessionIdsByIdentity = useMemo<Record<string, string[]>>( - () => - Object.fromEntries( - pages?.flatMap((page) => Object.entries(page.sourceSessionIdsByIdentity ?? {})) ?? [] - ), - [pages] - ); + const sourceSessionIdsByIdentity = useMemo<Record<string, string[]>>(() => { + const merged = new Map<string, Set<string>>(); + for (const page of pages ?? []) { + for (const [identity, ids] of Object.entries(page.sourceSessionIdsByIdentity ?? {})) { + const bucket = merged.get(identity) ?? new Set<string>(); + for (const id of ids) bucket.add(id); + merged.set(identity, bucket); + } + } + return Object.fromEntries([...merged].map(([identity, ids]) => [identity, [...ids]])); + }, [pages]);🤖 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/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx around lines 206 - 212, Update the sourceSessionIdsByIdentity construction in the useMemo callback to merge entries across all pages by identity, accumulating the union of session ID arrays instead of letting Object.fromEntries overwrite earlier pages. Preserve the existing empty-page fallback and return the same Record<string, string[]> shape.src/repository/usage-logs.ts (1)
156-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value合并两个来源会话 ID 聚合函数。
hydrateUsageLogSourceSessionIds与loadUsageLogSourceSessionIdsByIdentity的取值、并行查询和去重逻辑完全相同，仅输出形式不同（行内字段与身份映射）。可以抽出一个返回Map<string, string[]>的内部函数，两个入口在其上做投影。这样能避免后续修改只落在一处。🤖 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/repository/usage-logs.ts` around lines 156 - 209, 合并 hydrateUsageLogSourceSessionIds 和 loadUsageLogSourceSessionIdsByIdentity 中重复的会话来源 ID 收集、并行查询及去重逻辑，抽出一个返回 Map<string, string[]> 的内部辅助函数；保留两个现有入口的输出契约，分别将 Map 投影为行内 sourceSessionIds 字段和身份映射。tests/unit/repository/activity-stream-replay.test.ts (1)
114-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value断言依赖精确的 SQL 文本。
expect(condition.sql).not.toContain('and "message_request"."session_id" not in')绑定了 Drizzle 的引号风格与关键字大小写。若 Drizzle 调整 SQL 生成格式，该断言会静默通过而不再检测回归。建议改为对小写化后的 SQL 断言not in出现次数，或断言session_id" not in片段不存在。🤖 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/repository/activity-stream-replay.test.ts` around lines 114 - 116, Update the assertion in the boundary SQL test around condition to avoid depending on Drizzle’s exact quoting and keyword formatting: use the already lowercased SQL to verify the relevant not-in condition is absent, preferably by checking the expected occurrence count or a stable session_id/not-in fragment.tests/unit/repository/usage-logs-sessionid-suggestions.test.ts (1)
136-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value该测试未验证前缀过滤。
测试名称说明“只返回匹配前缀的候选”，但 mock 直接返回
client-session，与传入的term无关。数据库层的LIKE条件由 mock 绕过。建议改为断言whereArgs中包含like与转义后的 pattern，或把测试名称改为描述“合并两次候选查询结果”。🤖 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/repository/usage-logs-sessionid-suggestions.test.ts` around lines 136 - 156, Update the test around findUsageLogSessionIdSuggestions so it actually verifies prefix filtering by capturing the query’s whereArgs and asserting they contain like with the escaped “client” prefix pattern; otherwise rename the test to describe only merging candidate results. Prefer preserving the current name and adding the database-condition assertion.
🤖 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/app/`[locale]/dashboard/logs/_components/virtualized-logs-table.tsx:
- Around line 206-212: Update the sourceSessionIdsByIdentity construction in the
useMemo callback to merge entries across all pages by identity, accumulating the
union of session ID arrays instead of letting Object.fromEntries overwrite
earlier pages. Preserve the existing empty-page fallback and return the same
Record<string, string[]> shape.
In `@src/repository/usage-logs.ts`:
- Around line 156-209: 合并 hydrateUsageLogSourceSessionIds 和
loadUsageLogSourceSessionIdsByIdentity 中重复的会话来源 ID 收集、并行查询及去重逻辑，抽出一个返回
Map<string, string[]> 的内部辅助函数；保留两个现有入口的输出契约，分别将 Map 投影为行内 sourceSessionIds
字段和身份映射。
In `@tests/unit/repository/activity-stream-replay.test.ts`:
- Around line 114-116: Update the assertion in the boundary SQL test around
condition to avoid depending on Drizzle’s exact quoting and keyword formatting:
use the already lowercased SQL to verify the relevant not-in condition is
absent, preferably by checking the expected occurrence count or a stable
session_id/not-in fragment.
In `@tests/unit/repository/usage-logs-sessionid-suggestions.test.ts`:
- Around line 136-156: Update the test around findUsageLogSessionIdSuggestions
so it actually verifies prefix filtering by capturing the query’s whereArgs and
asserting they contain like with the escaped “client” prefix pattern; otherwise
rename the test to describe only merging candidate results. Prefer preserving
the current name and adding the database-condition assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 41c27649-2333-43b5-b936-a3c1f604f509
📒 Files selected for processing (15)
src/actions/active-sessions.tssrc/actions/usage-logs.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/lib/api-client/v1/actions/usage-logs.tssrc/repository/activity-stream.tssrc/repository/usage-logs.tstests/unit/actions/active-sessions-monitoring.test.tstests/unit/actions/usage-logs-export-retry-count.test.tstests/unit/repository/activity-stream-replay.test.tstests/unit/repository/usage-logs-replay-projection.test.tstests/unit/repository/usage-logs-sessionid-filter.test.tstests/unit/repository/usage-logs-sessionid-suggestions.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx
- src/actions/active-sessions.ts
- src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf809808eb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| return { | ||
| logs: page.logs ?? page.items ?? [], | ||
| sourceSessionIdsByIdentity: page.sourceSessionIdsByIdentity, |
There was a problem hiding this comment.
When the dashboard loads cursor-paginated logs through /api/v1/usage-logs, this value is always undefined: toUsageLogsListResponse in src/app/api/v1/resources/usage-logs/handlers.ts reconstructs the action result using only items and pageInfo, dropping sourceSessionIdsByIdentity. Consequently, the virtualized table receives only each row's current sourceSessionId and cannot show the other physical client IDs grouped under the canonical prefix identity. Forward the map through that REST response before attempting to read it here.
Useful? React with 👍 / 👎.
| inArray(messageSessionIdentity, activeSessionIds), | ||
| inArray(messageRequest.sessionId, activeSessionIds) |
There was a problem hiding this comment.
When a client supplies a physical ID equal to an active canonical prefix ID such as pfx:<scope>:<fingerprint>, this second predicate includes that unrelated physical session in the active-session query. buildPublicSessionIdentity deliberately remaps client-controlled pfx: IDs to the sid: namespace to prevent exactly this alias, but matching the raw sessionId here bypasses that isolation and can crowd genuine sessions out of the activity stream. Observed tracker entries are canonical identities, and the COALESCE predicate already matches ordinary physical sessions whose canonical identity is unchanged, so the raw-ID alternative should not be used.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
cf80980 to
e4d7030
Compare
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)
package.json (1)
40-40: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win移除
package.json中的硬编码 CUI token。Line 40 将认证值直接写入
cuiscript。该 script 同时绑定0.0.0.0。拥有仓库内容的人员可以复用此值访问可达的 CUI 服务。请立即撤销并轮换当前 token。改为从
CUI_TOKEN环境变量读取。未设置CUI_TOKEN时应直接失败。🤖 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 `@package.json` at line 40, Remove the hard-coded token from the package.json cui script and update it to require the CUI_TOKEN environment variable, failing immediately when the variable is unset while preserving the existing host and port settings. Revoke and rotate the exposed token outside the script and repository.
🤖 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 `@package.json`:
- Line 6: Align the Node runtime declared by the package.json engines.node
setting with the version used by deploy/Dockerfile.dev: either pin the Docker
base image to a Node release meeting >=22.19.0 or lower engines.node to the
runtime version actually supported, ensuring dependency installation and startup
remain compatible.
---
Outside diff comments:
In `@package.json`:
- Line 40: Remove the hard-coded token from the package.json cui script and
update it to require the CUI_TOKEN environment variable, failing immediately
when the variable is unset while preserving the existing host and port settings.
Revoke and rotate the exposed token outside the script and repository.
🪄 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 Plus
Run ID: 86f0cba0-2ebf-4748-bfbe-880fb352406e
📒 Files selected for processing (21)
deploy/Dockerfilepackage.jsonsrc/actions/active-sessions.tssrc/actions/usage-logs.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/api/v1/resources/usage-logs/handlers.tssrc/lib/api-client/v1/actions/usage-logs.tssrc/repository/_shared/usage-log-filters.tssrc/repository/activity-stream.tssrc/repository/usage-logs.tstests/api/v1/usage-logs/usage-logs.test.tstests/unit/actions/active-sessions-monitoring.test.tstests/unit/actions/usage-logs-export-retry-count.test.tstests/unit/deploy-dockerfile-contract.test.tstests/unit/repository/activity-stream-replay.test.tstests/unit/repository/usage-logs-replay-projection.test.tstests/unit/repository/usage-logs-sessionid-filter.test.tstests/unit/repository/usage-logs-sessionid-suggestions.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/repository/_shared/usage-log-filters.ts
- src/actions/usage-logs.ts
- tests/unit/repository/usage-logs-replay-projection.test.ts
- tests/unit/actions/usage-logs-export-retry-count.test.ts
- src/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsx
- src/repository/activity-stream.ts
- src/lib/api-client/v1/actions/usage-logs.ts
- tests/unit/actions/active-sessions-monitoring.test.ts
- src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx
- src/actions/active-sessions.ts
- tests/unit/repository/usage-logs-sessionid-filter.test.ts
- tests/unit/repository/activity-stream-replay.test.ts
- src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
- src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsx
- src/repository/usage-logs.ts
| "private": true, | ||
| "engines": { | ||
| "node": ">=22.15.0" | ||
| "node": ">=22.19.0" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n --hidden --glob '!node_modules' \
'setup-node|node-version|FROM .*node:|NODE_VERSION|22\.15|22\.18|22\.19' \
.github deploy dev package.jsonRepository: ding113/claude-code-hub
Length of output: 872
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== package.json engines =="
python3 - <<'PY'
import json
from pathlib import Path
p = Path("package.json")
data=json.loads(p.read_text())
print(data.get("engines"))
PY
echo "== Node references across tracked files =="
git ls-files | xargs rg -n --hidden --glob '!node_modules' \
'setup-node|node-version|node:|NODE_VERSION|engines|22\.19|22\.18|22\.15|22\.20|node_version|\.nvmrc|package-lock.json' || true
echo "== Node versions advertised by Node Debian/Trixie tags =="
curl -fsSL https://deb.nodesource.com/node_22.x/dists/trixie/main/binary-amd64/Packages.gz \
| gunzip -c \
| perl -nle 'BEGIN{$/="\nPackage: "} END{print}' \
| awk 'BEGIN{RS="Package: "; FS="\n"} $1~/^Node\.js/ {for(i=1;i<=NF;i++){if($i~/^Version: /){v=$i; sub(/^Version: /,"",v); print v}}}' \
| sort -V | tail -5 || true
echo "== Dockerfile excerpts =="
sed -n '1,45p' deploy/Dockerfile
echo "---"
sed -n '1,45p' deploy/Dockerfile.dev || true
echo "== Workflow excerpts =="
sed -n '180,205p' .github/workflows/release.yml
echo "---"
sed -n '50,68p' .github/workflows/dev.yml
echo "---"
sed -n '25,38p' .github/workflows/pr-check.ymlRepository: ding113/claude-code-hub
Length of output: 45050
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== deploy/Dockerfile.node-version 相关上下文 =="
git ls-files deploy/Dockerfile Dockerfile | xargs -r -I{} sh -c 'echo "--- {}"; rg -n --hidden --glob "!node_modules" "setup-node|node-version|bun-version|node:22|FROM .*node:|NODE_VERSION|engines|formatting|dependencies" "{}" || true'
echo "== actions/setup-node 步骤上下文 =="
git ls-files .github/workflows pr-check.yml dev.yml release.yml | xargs -r -I{} sh -c 'echo "--- workflow {}"; sed -n "1,230p" "{}" | nl -ba | sed -n "20,210p"'
echo "== 非标准 Node 版本号引用位置 =="
rg -n --hidden --glob '!node_modules' \
'node-version:\s*["'\'']*([^"'\''}]+)["'\'']|node-version:\s*20|FROM\s+node:22|Dockerfile\.node-version|setup-node' \
.github Dockerfile deploy package.json | head -80Repository: ding113/claude-code-hub
Length of output: 2511
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== root Dockerfile node:22-slim context =="
sed -n '1,35p' Dockerfile
echo "== package-lock scripts/engine hints =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("package-lock.json")
if p.exists():
data=json.loads(p.read_text())
print(data.get("requires"), data.get("lockfileVersion"), data.get("engines"))
pkg=json.loads(Path("package.json").read_text())
print("package engines", pkg.get("engines"))
print("package scripts key count", len(pkg.get("scripts", {})))
PY
echo "== explicit local Node engine/runtime checks =="
rg -n --hidden --glob '!node_modules' \
'process\.versions|node\.versions|check for|Node\.js >=|>=22\.19|engines\.node|must install|installing Node|nvm|volta|direnv|README\.|README\.en\.md' \
.github Dockerfile docker-compose*.yml deploy scripts src README.md README.en.md package.json package-lock.json | head -120Repository: ding113/claude-code-hub
Length of output: 1344
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
for name in ["package.json", "package-lock.json"]:
p = Path(name)
if p.exists():
data = json.loads(p.read_text())
print(f"--- {name} ---")
print("lockfileVersion:", data.get("lockfileVersion"))
print("engines:", data.get("engines"))
else False
PY
echo "== scripts/engine notes =="
rg -n --hidden --glob '!node_modules' \
'Node\.js >=|>=22\.19|>=22\.15|Node\.js|Node.js|setup-node|node-version|FROM node:22|from node:22|engines\.node|must install|nvm|volta' \
.github Dockerfile deploy README.md README.en.md README* package.json scripts src/server.js | head -160Repository: ding113/claude-code-hub
Length of output: 241
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
from pathlib import Path
for name in ["package.json", "package-lock.json"]:
p = Path(name)
if p.exists():
data = json.loads(p.read_text())
print(f"--- {name} ---")
print("lockfileVersion:", data.get("lockfileVersion"))
print("engines:", data.get("engines"))
PY
echo "== scripts/engine notes =="
rg -n --hidden --glob '!node_modules' \
'Node\.js >=|>=22\.19|>=22\.15|Node\.js|Node.js|setup-node|node-version|FROM node:22|from node:22|engines\.node|must install|nvm|volta' \
.github Dockerfile deploy README.md README.en.md README* package.json scripts src/server.js | head -160Repository: ding113/claude-code-hub
Length of output: 3225
将 Node 版本声明与部署运行时保持一致。
package.json 要求 node >=22.19.0，但 deploy/Dockerfile.dev 仍使用 node:trixie-slim，且 Docker 镜像的 Node 运行时版本可能无法稳定满足 22.19.0。如果 node:trixie-slim 提供的版本低于该下限，生产/开发镜像的安装或启动会失败；如果无法保证高于下限，请将 engines.node 降至实际支持的版本，或在镜像中固定/检查 Node 版本。
🤖 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 `@package.json` at line 6, Align the Node runtime declared by the package.json
engines.node setting with the version used by deploy/Dockerfile.dev: either pin
the Docker base image to a Node release meeting >=22.19.0 or lower engines.node
to the runtime version actually supported, ensuring dependency installation and
startup remain compatible.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4d703044a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? await loadUsageLogSourceSessionIdsByIdentity( | ||
| logs, | ||
| { keyString }, | ||
| { message: messageRows.length > 0, ledger: ledgerRows.length > 0 } |
There was a problem hiding this comment.
When the read-only/my-usage page produces a page from only one backing query—for example, a date or endpoint filter returns current message_request rows while older rows for the same prefix survive only in usage_ledger—these booleans disable hydration from the other store. Because the hydrator otherwise ignores page filters and is intended to return all physical IDs grouped under the public identity, the tooltip silently omits IDs based solely on which source contributed rows to this page; consult both stores within the key scope rather than using the page-query row counts as availability flags.
Useful? React with 👍 / 👎.
| cacheCreationInputTokens: messageRequest.cacheCreationInputTokens, | ||
| cacheReadInputTokens: messageRequest.cacheReadInputTokens, | ||
| rowNum: sql<number>`ROW_NUMBER() OVER (PARTITION BY ${messageRequest.sessionId} ORDER BY ${messageRequest.createdAt} DESC)`, | ||
| rowNum: sql<number>`ROW_NUMBER() OVER (PARTITION BY ${messageSessionIdentity} ORDER BY ${messageRequest.createdAt} DESC)`, |
There was a problem hiding this comment.
When one prefix-affinity identity contributes more than limit * 2 of the newest requests, this outer limit is applied before the JavaScript rowNum === 1 filter, so every fetched row can belong to that single canonical partition and only one survives. Collapsing multiple physical IDs into messageSessionIdentity makes this regression especially likely for prefix sessions; other observed active sessions can then be displaced by the generic fallback instead of receiving their latest row. Filter the windowed result to rowNum = 1 in a subquery, or use an equivalent per-identity query, before applying the limit.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/repository/usage-logs-sessionid-filter.test.ts (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win将源代码文本断言改为运行时行为断言。
readFileSync(...).toContain(...)只证明源码中存在指定文本。它没有执行只读 key hydration，也没有验证空页场景下message和ledger两个数据源都会查询。请使用现有数据库 mock 调用实际路径，并断言两个查询的调用和结果。🤖 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/repository/usage-logs-sessionid-filter.test.ts` around lines 61 - 64, Replace the source-text assertion in the readonly key hydration test with a runtime test that uses the existing database mocks to execute the hydration path with one backing-store page empty. Assert both message and ledger queries are invoked and verify the combined hydration result, using the relevant usage-logs repository symbols rather than readFileSync.
🤖 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 `@src/repository/activity-stream.ts`:
- Around line 107-110: 在 src/repository/activity-stream.ts 的查询构建流程中，使用
messageSessionIdentity 对 ROW_NUMBER() 结果应用 rowNum = 1 的数据库级过滤，再按 createdAt
降序排序并应用结果限制后返回应用层。同步更新 tests/unit/repository/activity-stream-replay.test.ts
第129-144行，断言规范会话及最新请求 ID，并移除对 boundary.limits 为空的断言。
---
Nitpick comments:
In `@tests/unit/repository/usage-logs-sessionid-filter.test.ts`:
- Around line 61-64: Replace the source-text assertion in the readonly key
hydration test with a runtime test that uses the existing database mocks to
execute the hydration path with one backing-store page empty. Assert both
message and ledger queries are invoked and verify the combined hydration result,
using the relevant usage-logs repository symbols rather than readFileSync.
🪄 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 Plus
Run ID: 79f1bcd9-44e4-41e9-b9cb-f053d4f2d97b
📒 Files selected for processing (4)
src/repository/activity-stream.tssrc/repository/usage-logs.tstests/unit/repository/activity-stream-replay.test.tstests/unit/repository/usage-logs-sessionid-filter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/repository/usage-logs.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d27c510ad
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cacheCreationInputTokens: messageRequest.cacheCreationInputTokens, | ||
| cacheReadInputTokens: messageRequest.cacheReadInputTokens, | ||
| rowNum: sql<number>`ROW_NUMBER() OVER (PARTITION BY ${messageRequest.sessionId} ORDER BY ${messageRequest.createdAt} DESC)`, | ||
| rowNum: sql<number>`ROW_NUMBER() OVER (PARTITION BY ${messageSessionIdentity} ORDER BY ${messageRequest.createdAt} DESC)`, |
There was a problem hiding this comment.
When an observed prefix-affinity identity has accumulated many requests, this query returns every matching historical row to Node and only then keeps rowNum === 1; because the activity stream is polled, a long-lived identity can repeatedly transfer and materialize an unbounded result set just to display one entry. The current revision removed the former limit but still performs the window filter in JavaScript, so wrap the window query and apply rowNum = 1 in SQL before limiting the final identities.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1c08e394e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .orderBy(messageSessionIdentity, desc(messageRequest.createdAt)) | ||
| .limit(limit); |
There was a problem hiding this comment.
When more observed active identities have database rows than limit, this current DISTINCT ON revision orders the result by canonical identity before applying the limit, so it selects the lexicographically first identities rather than those with the newest requests. The later in-memory sort cannot recover identities already discarded, causing recent active sessions to disappear from the activity stream; select one row per identity in a subquery, then order those rows by creation time before limiting.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df489f211d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const conditions = [inArray(ledgerSessionIdentity, sessionIds)]; | ||
| if (scope.userId !== undefined) conditions.push(eq(usageLedger.userId, scope.userId)); | ||
| if (scope.keyId !== undefined) conditions.push(eq(keysTable.id, scope.keyId)); | ||
| if (scope.keyString !== undefined) conditions.push(eq(usageLedger.key, scope.keyString)); |
There was a problem hiding this comment.
When the admin dashboard falls back to ledger-only storage without a user/key scope, every usage-log page now runs this identity aggregate across usage_ledger. The only index beginning with COALESCE(session_identity, session_id) is the partial idx_usage_ledger_session_identity_created_at in src/drizzle/schema.ts, whose predicate requires blocked_by IS NULL AND is_replay = false; because this query implies neither predicate, PostgreSQL cannot use that index and must scan/group the ledger on each paginated fetch. Add an applicable identity index or otherwise make the hydration query use an indexed lookup while preserving the required replay/blocked rows.
Useful? React with 👍 / 👎.
| sql`( | ||
| COALESCE(${messageRequest.sessionIdentity}, ${messageRequest.sessionId}) = ${trimmedSessionId} | ||
| OR ${messageRequest.sessionId} = ${trimmedSessionId} | ||
| )` |
There was a problem hiding this comment.
When a client-controlled physical ID equals another session's canonical pfx: identity, filtering by that canonical identity now also returns the unrelated physical session through this raw-ID alternative. buildPublicSessionIdentity in src/lib/request-identity.ts deliberately maps client-provided pfx: and sid: values into a key-bound sid: namespace to prevent this alias, but both this condition and the new ledger equivalent bypass that isolation. Fresh evidence beyond the earlier activity-stream issue is that this revision introduces the same alternative in the usage-log filters, so canonical and physical searches need an explicit discriminator rather than an ambiguous OR.
Useful? React with 👍 / 👎.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Tests
bun run typecheckbun run lintbun run testbun run buildTarget
devGreptile Summary
The PR aligns usage-log filtering, suggestions, display metadata, active-session monitoring, and replay behavior around canonical prefix identities while preserving client-provided IDs.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (10): Last reviewed commit: "test: strengthen session identity regres..." | Re-trigger Greptile
Context used: