Conversation
The previous implementation extracted output_tokens from message_start event which appears at the beginning of the SSE stream, resulting in incorrect (usually 1) output token counts since the main content hadn't been generated yet. This fix: - Extracts input tokens and cache fields (5m/1h) from message_start - Extracts final output_tokens from message_delta (at stream end) - Merges both to get accurate usage metrics Closes #312 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…okens Fix #312: Extract output_tokens from message_delta for Anthropic SSE streams
…ar day - Add last24h period type to LeaderboardPeriod - Add findLast24HoursLeaderboard() function using INTERVAL '24 hours' - Update daily-leaderboard task to use rolling 24h window - Update message templates to show 'past 24 hours' instead of 'today' Fixes #308
- Add RESOURCE_NOT_FOUND to ErrorCategory enum - Update categorizeErrorAsync to detect 404 status code - Add handler in forwarder for 404: triggers failover but skips recordFailure - Add 'resource_not_found' reason type to ProviderChainItem
- Add pagination params to getAllSessions() with input validation - Add PaginationControls component with separate state for active/inactive - Add i18n translations for pagination.total in all 5 locales - Validate page bounds (min 1) and pageSize (1-200)
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 delivers a set of targeted enhancements aimed at improving both user experience and system robustness. It introduces pagination to the session management dashboard, making it more efficient to navigate and monitor active and inactive user sessions. Concurrently, the proxy layer benefits from more sophisticated error handling, allowing for better detection of problematic upstream responses and more intelligent failover strategies. Finally, the leaderboard functionality has been refined to offer a more timely view of user activity by shifting to a 24-hour rolling window. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
bug
![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 introduces pagination for active and inactive sessions on the dashboard, allowing users to navigate through large lists of sessions. This involved modifying the getAllSessions action to accept pagination parameters (activePage, inactivePage, pageSize), update its return type with total counts and hasMore flags, and apply slice operations to paginate the data. The frontend ActiveSessionsPage was updated to manage pagination state, integrate new PaginationControls components, and adjust the useQuery hook to fetch paginated data. Additionally, the PR refactors the error handling logic in the proxy, introducing a new EmptyResponseError to detect and handle empty or content-less responses from upstream providers, and a RESOURCE_NOT_FOUND error category for 404 responses, both of which trigger provider switching but with different circuit breaker implications. The parseUsageFromResponseText function was enhanced to accurately extract usage metrics from Claude SSE responses by merging data from message_start and message_delta events. Finally, the daily leaderboard notification system was updated to report statistics for the 'past 24 hours' instead of a calendar day, changing function names and message templates accordingly. A review comment highlighted that the current pagination implementation in getAllSessions fetches all data into memory before slicing, suggesting that pagination should be pushed down to the data layer (Redis and database) for efficiency. Another comment pointed out duplicated pagination logic within getAllSessions, recommending extraction into a helper function to adhere to DRY principles.
| export async function getAllSessions( | ||
| activePage: number = 1, | ||
| inactivePage: number = 1, | ||
| pageSize: number = 20 | ||
| ): Promise< | ||
| ActionResult<{ | ||
| active: ActiveSessionInfo[]; | ||
| inactive: ActiveSessionInfo[]; | ||
| totalActive: number; | ||
| totalInactive: number; | ||
| hasMoreActive: boolean; | ||
| hasMoreInactive: boolean; | ||
| }> | ||
| > { |
There was a problem hiding this comment.
The current implementation of pagination fetches all session data from the database and Redis into memory and then performs pagination in the application layer. This can lead to significant performance issues and high memory consumption if the number of sessions is large.
To improve efficiency, pagination should be pushed down to the data layer:
- When fetching session IDs from Redis (
SessionManager.getAllSessionIds), it should support pagination. - The
aggregateMultipleSessionStatsfunction should be modified to acceptlimitandoffsetto perform pagination at the database level, rather than fetching all stats at once.
This would prevent fetching potentially thousands of session records just to display a small page.
| // 7. 应用分页 | ||
| const totalActive = active.length; | ||
| const totalInactive = inactive.length; | ||
| const activeOffset = (safeActivePage - 1) * safePageSize; | ||
| const inactiveOffset = (safeInactivePage - 1) * safePageSize; | ||
| const paginatedActive = active.slice(activeOffset, activeOffset + safePageSize); | ||
| const paginatedInactive = inactive.slice(inactiveOffset, inactiveOffset + safePageSize); | ||
|
|
||
| return { | ||
| ok: true, | ||
| data: { | ||
| active: paginatedActive, | ||
| inactive: paginatedInactive, | ||
| totalActive, | ||
| totalInactive, | ||
| hasMoreActive: activeOffset + paginatedActive.length < totalActive, | ||
| hasMoreInactive: inactiveOffset + paginatedInactive.length < totalInactive, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This block of code for pagination is a duplicate of the logic found earlier in the function (around lines 234-252). To adhere to the Don't Repeat Yourself (DRY) principle and improve maintainability, this logic should be extracted into a helper function.
For example, you could create a function like this and call it from both places:
function paginateAndFormat(
active: ActiveSessionInfo[],
inactive: ActiveSessionInfo[],
activePage: number,
inactivePage: number,
pageSize: number
) {
const totalActive = active.length;
const totalInactive = inactive.length;
const activeOffset = (activePage - 1) * pageSize;
const inactiveOffset = (inactivePage - 1) * pageSize;
const paginatedActive = active.slice(activeOffset, activeOffset + pageSize);
const paginatedInactive = inactive.slice(inactiveOffset, inactiveOffset + pageSize);
return {
ok: true,
data: {
active: paginatedActive,
inactive: paginatedInactive,
totalActive,
totalInactive,
hasMoreActive: activeOffset + paginatedActive.length < totalActive,
hasMoreInactive: inactiveOffset + paginatedInactive.length < totalInactive,
},
};
}![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 implements several features: pagination for session views, empty response detection with provider failover, 404 error handling without circuit breaker impact, and leaderboard improvements. The changes are well-structured overall, with proper error categorization and session management.
PR Size: M
- Lines changed: 552 (496 additions, 56 deletions)
- Files changed: 15
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
1. [ERROR-BROAD-CATCH] Empty response JSON parse catch block logs without parse error details
- File:
src/app/v1/_lib/proxy/forwarder.ts(new code around line 286 in diff) - Issue: The catch block uses
_parseError(unused variable pattern) and logs at debug level without including the actual parse error. If the response is malformed JSON due to truncation or encoding issues, this masks the real problem. - Suggested fix:
} catch (parseError) {
// JSON 解析失败但响应体不为空，不视为空响应错误
logger.debug("ProxyForwarder: Non-JSON response body, skipping content check", {
providerId: currentProvider.id,
contentType,
parseError: parseError instanceof Error ? parseError.message : String(parseError),
});
}Medium Priority Issues (Consider Fixing)
1. [TEST-MISSING-CRITICAL] No tests for new EmptyResponseError class and detection logic
- File:
src/app/v1/_lib/proxy/errors.ts - Issue: The new
EmptyResponseErrorclass andisEmptyResponseErrortype guard, plus theRESOURCE_NOT_FOUNDerror category, are added without corresponding unit tests. This is critical error handling logic that affects provider failover behavior. - Suggested action: Add unit tests covering:
EmptyResponseErrorconstruction with all three reason typesisEmptyResponseErrortype guardcategorizeErrorAsyncreturning correct categories forEmptyResponseErrorand 404ProxyError
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - One issue noted
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Gap identified
- Code clarity - Good
Notes on Implementation Quality
Good patterns observed:
- Proper use of
response.clone()before consuming the body for empty response detection - Clear separation of 404 handling from general provider errors
- SSE/streaming detection to skip empty response checks appropriately
- Pagination with proper input validation and bounds checking in
active-sessions.ts
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
This PR implements several features: pagination for session views, empty response detection with provider failover, 404 error handling without circuit breaker impact, and leaderboard improvements. The changes are well-structured overall, with proper error categorization and session management.
PR Size: M
- Lines changed: 552 (496 additions, 56 deletions)
- Files changed: 15
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
1. [ERROR-BROAD-CATCH] Empty response JSON parse catch block logs without parse error details
- File:
src/app/v1/_lib/proxy/forwarder.ts(new code around line 286 in diff) - Issue: The catch block uses
_parseError(unused variable pattern) and logs at debug level without including the actual parse error. If the response is malformed JSON due to truncation or encoding issues, this masks the real problem. - Suggested fix:
} catch (parseError) {
// JSON 解析失败但响应体不为空，不视为空响应错误
logger.debug("ProxyForwarder: Non-JSON response body, skipping content check", {
providerId: currentProvider.id,
contentType,
parseError: parseError instanceof Error ? parseError.message : String(parseError),
});
}Medium Priority Issues (Consider Fixing)
1. [TEST-MISSING-CRITICAL] No tests for new EmptyResponseError class and detection logic
- File:
src/app/v1/_lib/proxy/errors.ts - Issue: The new
EmptyResponseErrorclass andisEmptyResponseErrortype guard, plus theRESOURCE_NOT_FOUNDerror category, are added without corresponding unit tests. This is critical error handling logic that affects provider failover behavior. - Suggested action: Add unit tests covering:
EmptyResponseErrorconstruction with all three reason typesisEmptyResponseErrortype guardcategorizeErrorAsyncreturning correct categories forEmptyResponseErrorand 404ProxyError
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - One issue noted
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Gap identified
- Code clarity - Good
Notes on Implementation Quality
Good patterns observed:
- Proper use of
response.clone()before consuming the body for empty response detection - Clear separation of 404 handling from general provider errors
- SSE/streaming detection to skip empty response checks appropriately
- Pagination with proper input validation and bounds checking in
active-sessions.ts
Automated review by Claude AI
- Add quick date filter buttons (Today, Yesterday, Last 7 Days, Last 30 Days) - Add CSV export with formula injection protection - Use differenceInCalendarDays to avoid DST issues - Add aria-pressed for accessibility - Add i18n translations for en, zh-CN, zh-TW, ja, ru Closes #256
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
This PR brings multiple improvements from the dev branch: sessions pagination UI, upstream 404 handling without circuit breaker impact, empty response detection with automatic failover, correct output token extraction from Anthropic SSE streams, and rolling 24-hour window for daily leaderboard notifications.
Problem
1. Anthropic SSE Output Token Counting (Fixes #312)
The v0.3.27 update introduced 5m/1h cache differentiated billing, which changed usage extraction to use
message_start. However,message_startappears at stream beginning whenoutput_tokensis typically 1, before actual content is generated.2. Daily Leaderboard Window (Fixes #308)
The daily leaderboard notification only included data from midnight to send time (e.g., 00:00-09:00 for 9am sends), not a full day's worth of data.
3. Upstream 404 Errors
Previously, 404 errors from upstream providers would trigger circuit breaker failures, potentially causing unnecessary provider unavailability for transient resource issues.
4. Empty Response Handling
Empty responses from providers (empty body, zero output tokens, missing content) were not detected, potentially causing silent failures.
5. Sessions Page Performance
No pagination on the sessions monitoring page could cause performance issues with many active/inactive sessions.
Solution
Anthropic SSE Fix (merged from PR #313)
message_start(required for 5m/1h cache billing)output_tokensfrommessage_delta(accurate count at stream end)Daily Leaderboard
findLast24HoursLeaderboard()repository function404 Error Handling
RESOURCE_NOT_FOUNDerror categoryEmpty Response Detection
EmptyResponseErrorclass for structured error handlingContent-Length: 0), missing content fields, zero output tokensSessions Pagination
getAllSessions()action withactivePage,inactivePage,pageSizeparametersChanges
Core Changes
src/app/v1/_lib/proxy/errors.ts: AddedEmptyResponseErrorclass andRESOURCE_NOT_FOUNDcategorysrc/app/v1/_lib/proxy/forwarder.ts: Empty response detection, 404 handling without circuit breakersrc/app/v1/_lib/proxy/response-handler.ts: Fixed Anthropic SSE usage parsing (message_start + message_delta merge)src/app/v1/_lib/proxy/session.ts: Addedresource_not_foundreason typeSessions Pagination
src/actions/active-sessions.ts: Added pagination parameters and return totalssrc/app/[locale]/dashboard/sessions/page.tsx: Added pagination UI controlsLeaderboard
src/repository/leaderboard.ts: Addedlast24hperiod andfindLast24HoursLeaderboard()src/lib/notification/tasks/daily-leaderboard.ts: Use rolling 24h windowsrc/lib/wechat/message-templates.ts: Updated labelsi18n
messages/{en,ja,ru,zh-CN,zh-TW}/dashboard.json: Added pagination.total translationsTypes
src/types/message.ts: Addedresource_not_foundreasonTesting
Automated Tests
Manual Testing
Checklist
dev→mainfor releaseDescription enhanced by Claude AI