fix: correct token budget gauge and populate session message count badge#749
fix: correct token budget gauge and populate session message count badge#749TadMSTR wants to merge 2 commits intositeboon:mainfrom
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughToken budget extraction now uses the model's current-turn input token count and model context window (fallback 200000); session summaries gain per-session user message counts computed synchronously from optional JSONL exports, excluding tool-result messages. ChangesContext and Session Metrics
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/claude-sdk.js (1)
305-319:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe
modelUsage.inputTokensfix introduces a critical logic error in multi-turn sessions.The SDK's
modelUsage.inputTokensfield reports cumulative tokens across the entire agent session, not per-turn. The code comment at line 305–306 incorrectly claims "current-turn input token count (not cumulative)", but the SDK provides cumulative totals. Using cumulativeinputTokensto compute the context window fill (used: totalUsed) will display inflated and misleading percentages in multi-turn conversations—each turn's input is counted again from the start, misrepresenting the actual context window pressure at that moment.To accurately reflect the current turn's context window impact, either:
- Obtain per-turn input tokens from the SDK (if available via a separate API, e.g.,
query.getContextUsage()), or- Track the previous turn's cumulative total and compute the delta (
currentInputTokens - previousInputTokens).The
contextWindowfield inmodelUsagedoes exist and is correctly used as a fallback to200000, so that part is sound.🤖 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 `@server/claude-sdk.js` around lines 305 - 319, The code uses modelData.inputTokens (assigned to totalUsed) as if it's a per-turn token count but the SDK returns cumulative session totals, causing inflated used values in multi-turn conversations; fix by computing per-turn tokens before returning used: if a per-turn API exists (e.g., query.getContextUsage() or similar), use that value, otherwise store and reference the previous cumulative value (e.g., previousInputTokens) and set used = Math.max(0, modelData.inputTokens - previousInputTokens) and update previousInputTokens for the next call; keep contextWindow fallback to modelData.contextWindow || 200000 as-is.
🤖 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 `@server/modules/projects/services/projects-with-sessions-fetch.service.ts`:
- Around line 131-165: countUserMessagesInJsonl currently uses
fsSync.readFileSync and blocks the event loop; make it async by switching to
fs.promises.readFile and returning Promise<number>, then update any
mappers/callers to await it: change mapSessionRowToSummary to an async function
(or create an async variant) so it awaits
countUserMessagesInJsonl(row.jsonl_path), and propagate awaits in
getProjectsWithSessions and getProjectSessionsPage (and any use inside
bucketSessionRowsByProvider) so session-row mapping runs without blocking;
optionally implement bounded concurrency (e.g., Promise.all with a concurrency
limiter) or add a simple (jsonl_path, mtime) cache in the async path to avoid
repeated full scans under load.
- Around line 145-149: The current isToolResult check only inspects content[0]
which misses tool_result blocks elsewhere; update the logic (the isToolResult
constant that uses content) to detect if any element in the content array has
type === 'tool_result' (e.g., Array.isArray(content) && content.some(item =>
(item as Record<string,unknown>)?.type === 'tool_result')) and then use that
boolean to exclude messages, ensuring mixed arrays with tool_result anywhere are
filtered out.
---
Outside diff comments:
In `@server/claude-sdk.js`:
- Around line 305-319: The code uses modelData.inputTokens (assigned to
totalUsed) as if it's a per-turn token count but the SDK returns cumulative
session totals, causing inflated used values in multi-turn conversations; fix by
computing per-turn tokens before returning used: if a per-turn API exists (e.g.,
query.getContextUsage() or similar), use that value, otherwise store and
reference the previous cumulative value (e.g., previousInputTokens) and set used
= Math.max(0, modelData.inputTokens - previousInputTokens) and update
previousInputTokens for the next call; keep contextWindow fallback to
modelData.contextWindow || 200000 as-is.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 61bdbdde-3133-47c9-8f74-d4133e672ac4
📒 Files selected for processing (2)
server/claude-sdk.jsserver/modules/projects/services/projects-with-sessions-fetch.service.ts
| function countUserMessagesInJsonl(jsonlPath: string): number { | ||
| try { | ||
| const content = fsSync.readFileSync(jsonlPath, 'utf8'); | ||
| const lines = content.split(/\r?\n/); | ||
| let count = 0; | ||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| if (!trimmed) continue; | ||
| try { | ||
| const data = JSON.parse(trimmed) as Record<string, unknown>; | ||
| if (data.type === 'user') { | ||
| // Only count genuine user prompts, not tool_result messages | ||
| const message = data.message as Record<string, unknown> | undefined; | ||
| const content = message?.content; | ||
| const isToolResult = | ||
| Array.isArray(content) && | ||
| content.length > 0 && | ||
| (content[0] as Record<string, unknown>)?.type === 'tool_result'; | ||
| if (!isToolResult) { | ||
| count += 1; | ||
| } | ||
| } | ||
| } catch { | ||
| // Skip malformed lines | ||
| } | ||
| } | ||
| return count; | ||
| } catch { | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| function mapSessionRowToSummary(row: SessionRepositoryRow): SessionSummary { | ||
| const messageCount = row.jsonl_path ? countUserMessagesInJsonl(row.jsonl_path) : 0; | ||
| return { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call-path and sync-IO usage without modifying repo.
rg -n -C3 'countUserMessagesInJsonl|mapSessionRowToSummary|readFileSync|getProjectsWithSessions|getProjectSessionsPage' --type=tsRepository: siteboon/claudecodeui
Length of output: 12126
🏁 Script executed:
sed -n '170,210p' server/modules/projects/services/projects-with-sessions-fetch.service.tsRepository: siteboon/claudecodeui
Length of output: 1099
🏁 Script executed:
rg -n 'mapSessionRowToSummary' --type=ts -B2 -A2Repository: siteboon/claudecodeui
Length of output: 1073
🏁 Script executed:
rg -n 'bucketSessionRowsByProvider' --type=tsRepository: siteboon/claudecodeui
Length of output: 367
🏁 Script executed:
rg -n 'readProjectSessionsPageByPath' --type=tsRepository: siteboon/claudecodeui
Length of output: 494
🏁 Script executed:
sed -n '260,280p' server/modules/projects/services/projects-with-sessions-fetch.service.ts && echo "---" && sed -n '305,320p' server/modules/projects/services/projects-with-sessions-fetch.service.tsRepository: siteboon/claudecodeui
Length of output: 1387
🏁 Script executed:
sed -n '131,161p' server/modules/projects/services/projects-with-sessions-fetch.service.tsRepository: siteboon/claudecodeui
Length of output: 1034
Convert blocking sync file I/O to async to prevent event loop blocking in request handlers.
The countUserMessagesInJsonl function at line 133 uses fsSync.readFileSync, which blocks Node's event loop. This is called for every session row via mapSessionRowToSummary (line 164) within getProjectsWithSessions and getProjectSessionsPage (async route handlers). Under load with many sessions, this degrades responsiveness of the API.
Since both route handlers are already async, converting this chain to use fs.readFile (promises API) is straightforward and has no API contract impact:
Conversion approach
- import fsSync from 'node:fs';
+ import fs from 'node:fs/promises';
-function countUserMessagesInJsonl(jsonlPath: string): number {
+async function countUserMessagesInJsonl(jsonlPath: string): Promise<number> {
try {
- const content = fsSync.readFileSync(jsonlPath, 'utf8');
+ const content = await fs.readFile(jsonlPath, 'utf8');
// ...
} catch {
return 0;
}
}
-function mapSessionRowToSummary(row: SessionRepositoryRow): SessionSummary {
- const messageCount = row.jsonl_path ? countUserMessagesInJsonl(row.jsonl_path) : 0;
+async function mapSessionRowToSummary(row: SessionRepositoryRow): Promise<SessionSummary> {
+ const messageCount = row.jsonl_path ? await countUserMessagesInJsonl(row.jsonl_path) : 0;
return { ... };
}Update bucketSessionRowsByProvider to handle async mapping:
-function bucketSessionRowsByProvider(rows: SessionRepositoryRow[]): SessionsByProvider {
+async function bucketSessionRowsByProvider(rows: SessionRepositoryRow[]): Promise<SessionsByProvider> {
// ...
- bucket.push(mapSessionRowToSummary(row));
+ bucket.push(await mapSessionRowToSummary(row));
}If full async conversion is not desirable, add bounded concurrency + caching by (jsonl_path, mtime) to avoid repeated full scans.
🤖 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 `@server/modules/projects/services/projects-with-sessions-fetch.service.ts`
around lines 131 - 165, countUserMessagesInJsonl currently uses
fsSync.readFileSync and blocks the event loop; make it async by switching to
fs.promises.readFile and returning Promise<number>, then update any
mappers/callers to await it: change mapSessionRowToSummary to an async function
(or create an async variant) so it awaits
countUserMessagesInJsonl(row.jsonl_path), and propagate awaits in
getProjectsWithSessions and getProjectSessionsPage (and any use inside
bucketSessionRowsByProvider) so session-row mapping runs without blocking;
optionally implement bounded concurrency (e.g., Promise.all with a concurrency
limiter) or add a simple (jsonl_path, mtime) cache in the async path to avoid
repeated full scans under load.
There was a problem hiding this comment.
Noted — if the maintainer prefers the count-at-sync-time approach instead, the file reads go away entirely. Otherwise happy to do the async conversion as a follow-up.
There was a problem hiding this comment.
@TadMSTR, understood — both paths are valid:
- Count-at-sync-time: Storing
message_countin the DB duringsessionSynchronizerService.synchronizeSessions()would be the cleanest long-term solution — zero per-request I/O and consistent with how other session metadata is managed. The trade-off is a small schema migration. - Async conversion: Lower-risk incremental fix with no schema changes; would also benefit from bounded concurrency (
Promise.allwith a limiter) if projects can have many sessions.
Either way, since this is deferred to a follow-up, would you like me to open a GitHub issue to track it so it doesn't get lost?
Token budget (server/claude-sdk.js): The context window gauge was double-counting tokens — it summed cumulative input + output + cache tokens against a hardcoded 160K limit. Output tokens don't consume the context window. Replace with current-turn inputTokens only and derive the ceiling from modelData.contextWindow (defaulting to 200000 for current Claude models) instead of the 160K env-var budget. Message count badge (projects-with-sessions-fetch.service.ts): mapSessionRowToSummary was hardcoding messageCount: 0, causing the sidebar badge to never render. Count user-turn messages on-the-fly from the stored JSONL path using synchronous readFileSync (no DB migration required). Tool-result messages are excluded so only genuine human prompts are counted.
TadMSTR
force-pushed
the
fix/session-messagecount-and-token-budget
branch
from
62debbe to
81872b2
Compare
1 participant
Token budget (server/claude-sdk.js): The context window gauge was double-counting tokens — it summed cumulative input + output + cache tokens against a hardcoded 160K limit. Output tokens don't consume the context window. Replace with current-turn inputTokens only and derive the ceiling from modelData.contextWindow (defaulting to 200000 for current Claude models) instead of the 160K env-var budget.
Fixes #588Message count badge (projects-with-sessions-fetch.service.ts): mapSessionRowToSummary was hardcoding messageCount: 0, causing the sidebar badge to never render. Count user-turn messages on-the-fly from the stored JSONL path using synchronous readFileSync (no DB migration required). Tool-result messages are excluded so only genuine human prompts are counted.
Summary by CodeRabbit
New Features
Improvements