Merged
Conversation
Codex session initialization assumed thread.id was immediately available at thread creation time. In practice, for new sessions the SDK discovers/emits the real thread id asynchronously via stream events (thread.started), matching Claude’s behavior. This early assumption caused the backend to create and publish session state before the canonical id existed. Why this matters: - Session identity is the join key for live streaming, abort routing, notifications, and history sync. - Emitting session_created with a guessed/fallback id risks mismatched client state and stale references. - Registering active sessions too early can make abort/lookup logic depend on non-canonical ids. - Divergence from Claude’s flow created provider inconsistency and made cross-provider behavior harder to reason about. What changed: - Stop treating thread.id as authoritative at startup for new Codex sessions. - Capture the real session id from thread.started (prefer event.thread_id, fallback event.id). - Emit session_created only after the canonical id is discovered (new sessions only). - Track active session state immediately only for resumed sessions (known sessionId), otherwise defer registration until id discovery. - Thread the captured id through normalization, token budget updates, completion payloads, and failure notifications. - Preserve writer/session wiring by setting ws.setSessionId once the id is known. Result: Codex now follows the same async session-id contract as Claude, removing a race around initial session identity and making session lifecycle handling deterministic across providers.
The frontend realtime pipeline was carrying control-plane events and unused state through message storage as if they were renderable chat content. That made the session path noisier than necessary and increased the chance of subtle drift between transport events and UI data. Why this change: - Control events like session_created, status, complete, and permission lifecycle updates drive UI side effects, not chat transcript rendering. - Persisting those events in the session store added avoidable churn, memory growth, and merge work while providing no user-visible value. - An unused streamBufferRef existed in the hot path, creating extra writes and cognitive overhead with no read consumer. - useChatRealtimeHandlers accepted selectedProject even though it was not used, which widened the hook surface and dependency noise without behavior impact. What this commit does: - Removes the write-only streamBufferRef from ChatInterface and realtime handler wiring. - Removes the unused selectedProject argument from useChatRealtimeHandlers. - Stops appending non-rendered control events to sessionStore realtime messages. These events still execute their side effects exactly as before. Net effect: The Codex/Claude/Cursor/Gemini realtime path stays behaviorally equivalent for users, but the data model now stores only message content that can actually be rendered, reducing unnecessary state traffic in the chat runtime.
Sidebar session sorting and time labels currently normalize timestamps through getUpdatedTimestamp. That helper still checked session.updated_at as a fallback, but in the current project/session API shape the sidebar no longer receives raw DB session rows. Why this cleanup matters: - Sidebar session items are built from project payload SessionSummary objects, where lastActivity is already the canonical field. - The backend guarantees lastActivity is populated from updated_at ?? created_at ?? now before data reaches the client. - Keeping a client-side updated_at fallback implies mixed payload contracts and hides schema drift instead of surfacing it. - Removing the fallback narrows the UI contract to one timestamp source, which reduces ambiguity in session ordering and display-time logic. This change intentionally keeps behavior the same while making the dependency explicit: sidebar timestamp logic now reads only session.lastActivity.
Why: - Gemini does not expose a new session id synchronously. It emits the canonical id in the init stream event. - Creating temporary ids in web mode introduced identity drift, extra mapping logic, and harder resume/debug behavior. - Headless server runs often miss shell-inherited auth vars, while users configure Gemini through user-level env files. - Gemini mirrors session artifacts across folders, which caused duplicate sync events and duplicate session rows. What changed: - Removed temporary Gemini ids for new sessions. - New Gemini sessions are now created only after init provides session_id. - Persisted cliSessionId from the discovered canonical id, keeping one identifier across stream, storage, and resume. - Built Gemini spawn env from process env plus user-level fallback files: ~/.gemini/.env then ~/.env, honoring GEMINI_CLI_HOME. - Added --skip-trust for headless runs, because web flows cannot answer interactive trust prompts. - Improved terminal error mapping and rejection reasons, especially for auth exit code 41 with actionable context. - Limited Gemini synchronization to tmp JSONL chat artifacts, and disabled duplicate watcher/index paths that mirror the same sessions. - Added gemini-2.5-flash-lite to shared model constants. Result: - Gemini session identity is canonical and provider-consistent. - Headless auth now matches practical Gemini CLI configuration patterns. - Duplicate Gemini session indexing is reduced at the source. - Operators get clearer, actionable failure messages.
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR defers session registration to canonical provider session/thread IDs, removes temporary session-ID synthesis, loads Gemini auth from user-level env/settings, narrows session artifact discovery, and replaces frontend stream buffering with accumulated/timer refs plus processing-state callbacks. ChangesSession Lifecycle & Streaming Architecture
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Closed
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/chat/hooks/useChatRealtimeHandlers.ts (1)
229-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark the canonical session as processing when
session_createdarrives.For brand-new sessions, submit-time code cannot call
onSessionProcessingbecause there is no id yet. This handler installs the canonical id, but it never marks that id as active, so downstream state can treat the visible session as inactive until a later status event arrives. CallonSessionProcessing?.(newSessionId)here when the canonical id is discovered.Suggested fix
if (!currentSessionId) { console.log('Session created with ID:', newSessionId); console.log('Existing session ID:', currentSessionId); sessionStorage.setItem('pendingSessionId', newSessionId); if (pendingViewSessionRef.current && !pendingViewSessionRef.current.sessionId) { pendingViewSessionRef.current.sessionId = newSessionId; } setCurrentSessionId(newSessionId); setPendingPermissionRequests((prev) => prev.map((r) => (r.sessionId ? r : { ...r, sessionId: newSessionId })), ); } + onSessionProcessing?.(newSessionId); onNavigateToSession?.(newSessionId); break; }🤖 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/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 229 - 247, When handling the 'session_created' event inside the branch that checks if (!currentSessionId), after you set the canonical id (setCurrentSessionId(newSessionId)) and update pending requests, call onSessionProcessing?.(newSessionId) to mark the newly discovered session as active/processing; this ensures downstream logic knows the newly installed session id is in processing state. Keep the call guarded (optional chaining) and place it before or immediately after setCurrentSessionId so state observers see the processing signal for the canonical session.
🧹 Nitpick comments (2)
server/gemini-cli.js (1)
45-82: ⚡ Quick win
parseEnvFileContentis a near-duplicate ofparseEnvFileingemini-auth.provider.ts.Both functions implement identical dotenv-style parsing (export stripping, quote removal, inline comment trimming). They've already drifted slightly (
fs.accesspre-check in the loader here vs. directreadFilein the provider). Extracting this to a shared utility (e.g.,shared/utils.js) would give one place to fix edge cases (e.g., escaped quotes, multi-line values) and remove the drift risk.🤖 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/gemini-cli.js` around lines 45 - 82, parseEnvFileContent duplicates logic from parseEnvFile in gemini-auth.provider.ts; extract the shared parsing logic into a single utility function (e.g., parseEnvFile) in a new shared module (shared/utils.js or similar) and have both server/gemini-cli.js and gemini-auth.provider.ts import and call that function. Update server/gemini-cli.js to remove its local parseEnvFileContent and use the shared parseEnvFile, preserving the current behavior (export stripping, quote removal, inline comment trimming) and keep any loader-specific pre-check (fs.access) in the loader code rather than in the parser; run tests and ensure both callers still handle errors the same way.server/modules/providers/list/gemini/gemini-auth.provider.ts (1)
31-38: ⚡ Quick win
GEMINI_AUTH_ENV_KEYSis also defined verbatim inserver/gemini-cli.js(line 36).Both the auth provider and the CLI spawner maintain identical copies of this constant. If a new auth env key is added to one file, it must be manually updated in the other — a maintenance hazard. This constant belongs in
shared/alongside the env-parsing utility mentioned above.🤖 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/providers/list/gemini/gemini-auth.provider.ts` around lines 31 - 38, GEMINI_AUTH_ENV_KEYS is duplicated across the auth provider and the CLI; extract this constant into a single shared module (e.g., export const GEMINI_AUTH_ENV_KEYS from a new shared utility) and replace the inline arrays in both locations with imports from that shared module; update the env-parsing utility to consume the shared export so both the auth provider (GEMINI_AUTH_ENV_KEYS reference) and the CLI use the same source of truth and remove the duplicated definitions.
🤖 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/gemini-cli.js`:
- Around line 19-34: Remove the unreachable case 41 from
mapGeminiExitCodeToMessage and ensure the single authoritative message for exit
code 41 is kept in the close handler branch (the else if (code === 41) block) so
messages remain consistent; update either the close handler text or other
mappings to use the same phrasing for GEMINI_API_KEY references and remove the
duplicate case from mapGeminiExitCodeToMessage.
- Around line 104-121: The early-return in buildGeminiProcessEnv is wrong
because it uses GEMINI_AUTH_ENV_KEYS.every(...) which only returns true when all
six keys exist; change the guard to return early if any primary credential is
already available (e.g., check processEnv.GEMINI_API_KEY ||
processEnv.GOOGLE_APPLICATION_CREDENTIALS or include GOOGLE_API_KEY as needed)
so you skip loadGeminiUserLevelEnv when a sufficient auth indicator exists;
update the condition in buildGeminiProcessEnv accordingly and keep the rest of
the logic that falls back to loadGeminiUserLevelEnv and copies keys from the
result.
In
`@server/modules/providers/list/cursor/cursor-session-synchronizer.provider.ts`:
- Around line 129-131: The code computes grandparentDir from filePath using
three path.dirname calls which yields the global ~/.cursor/projects/worker.log
and causes extractProjectPathFromWorkerLog(workerLogPath) to fail; change the
traversal to two path.dirname calls so workerLogPath points to the project-level
worker.log for the discovered JSONL (i.e., derive projectDir =
path.dirname(path.dirname(filePath)) and workerLogPath = path.join(projectDir,
'worker.log')), and keep extractProjectPathFromWorkerLog, filePath and
workerLogPath names intact; also add a conservative check after computing
workerLogPath to handle missing files (null return) so sessions aren't silently
dropped.
In `@server/openai-codex.js`:
- Around line 237-255: The current registerSession function only sets a session
when activeCodexSessions.has(id) is false, causing stale
thread/abortController/status to persist on resume; change registerSession (and
the capturedSessionId branch) to re-register unconditionally for a valid id:
keep the early return if !id, but always call activeCodexSessions.set(id, {
thread, codex, status: 'running', abortController, startedAt: new
Date().toISOString() }) so resumed threads replace old entries and won’t use
stale controllers or status.
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 536-540: Remove the unguarded debug log in the submit path inside
useChatComposerState: delete the console.log that prints messageContent,
effectiveSessionId, and uploadedImages (or wrap it behind an explicit debug
flag) so sensitive user prompts and image metadata are not emitted in normal
runtime; if you need conditional logging, add a hook-level or environment-driven
boolean (e.g., enableComposerDebug) and only log when that flag is true,
referencing useChatComposerState, messageContent, effectiveSessionId, and
uploadedImages to locate the statement.
---
Outside diff comments:
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts`:
- Around line 229-247: When handling the 'session_created' event inside the
branch that checks if (!currentSessionId), after you set the canonical id
(setCurrentSessionId(newSessionId)) and update pending requests, call
onSessionProcessing?.(newSessionId) to mark the newly discovered session as
active/processing; this ensures downstream logic knows the newly installed
session id is in processing state. Keep the call guarded (optional chaining) and
place it before or immediately after setCurrentSessionId so state observers see
the processing signal for the canonical session.
---
Nitpick comments:
In `@server/gemini-cli.js`:
- Around line 45-82: parseEnvFileContent duplicates logic from parseEnvFile in
gemini-auth.provider.ts; extract the shared parsing logic into a single utility
function (e.g., parseEnvFile) in a new shared module (shared/utils.js or
similar) and have both server/gemini-cli.js and gemini-auth.provider.ts import
and call that function. Update server/gemini-cli.js to remove its local
parseEnvFileContent and use the shared parseEnvFile, preserving the current
behavior (export stripping, quote removal, inline comment trimming) and keep any
loader-specific pre-check (fs.access) in the loader code rather than in the
parser; run tests and ensure both callers still handle errors the same way.
In `@server/modules/providers/list/gemini/gemini-auth.provider.ts`:
- Around line 31-38: GEMINI_AUTH_ENV_KEYS is duplicated across the auth provider
and the CLI; extract this constant into a single shared module (e.g., export
const GEMINI_AUTH_ENV_KEYS from a new shared utility) and replace the inline
arrays in both locations with imports from that shared module; update the
env-parsing utility to consume the shared export so both the auth provider
(GEMINI_AUTH_ENV_KEYS reference) and the CLI use the same source of truth and
remove the duplicated definitions.
🪄 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: e0c0a46d-9840-43fa-8f65-f63d016e74ac
📒 Files selected for processing (19)
server/cursor-cli.jsserver/gemini-cli.jsserver/modules/providers/list/cursor/cursor-session-synchronizer.provider.tsserver/modules/providers/list/gemini/gemini-auth.provider.tsserver/modules/providers/list/gemini/gemini-session-synchronizer.provider.tsserver/modules/providers/services/sessions-watcher.service.tsserver/openai-codex.jsshared/modelConstants.jssrc/components/app/AppContent.tsxsrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatRealtimeHandlers.tssrc/components/chat/hooks/useChatSessionState.tssrc/components/chat/types/types.tssrc/components/chat/view/ChatInterface.tsxsrc/components/main-content/types/types.tssrc/components/main-content/view/MainContent.tsxsrc/components/sidebar/utils/utils.tssrc/hooks/useProjectsState.tssrc/hooks/useSessionProtection.ts
💤 Files with no reviewable changes (7)
- src/hooks/useSessionProtection.ts
- src/components/chat/types/types.ts
- src/components/main-content/types/types.ts
- src/components/app/AppContent.tsx
- server/cursor-cli.js
- src/components/chat/view/ChatInterface.tsx
- src/components/main-content/view/MainContent.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/modules/providers/list/cursor/cursor-sessions.provider.ts (1)
179-187: ⚡ Quick win
ApplyPatchbranch innormalizeCursorToolInputis unreachable.At line 554
rawToolName === 'ApplyPatch' ? 'Edit' : rawToolNamerewrites the tool name to'Edit'before it is passed tonormalizeCursorToolInputon line 566. As a result theif (toolName === 'ApplyPatch')block at lines 179–184 never runs, and ApplyPatch payloads fall into theEditbranch which only inspectsold_string/new_string. Theirpatchstring survives solely because of the initial{ ...input }spread, and aliases likeinput.difforinput.contentare silently dropped.Either normalize before renaming, or pass the original name to the helper, e.g.:
♻️ One possible fix
- const rawToolName = part.toolName || part.name || 'Unknown Tool'; - const toolName = rawToolName === 'ApplyPatch' ? 'Edit' : rawToolName; + const rawToolName = part.toolName || part.name || 'Unknown Tool'; + const toolName = rawToolName === 'ApplyPatch' ? 'Edit' : rawToolName; const toolId = normalizeToolId(part.toolCallId) || normalizeToolId(part.tool_call_id) || normalizeToolId(part.id) || `tool_${i}_${partIdx}`; const message = createNormalizedMessage({ id: `${baseId}_${partIdx}`, sessionId, timestamp: ts, provider: PROVIDER, kind: 'tool_use', toolName, - toolInput: normalizeCursorToolInput(toolName, part.args ?? part.input), + toolInput: normalizeCursorToolInput(rawToolName, part.args ?? part.input), toolId, });Also applies to: 552-567
🤖 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/providers/list/cursor/cursor-sessions.provider.ts` around lines 179 - 187, The ApplyPatch branch in normalizeCursorToolInput is never hit because rawToolName is rewritten to 'Edit' before calling the helper; change the call site to pass the original rawToolName (or run normalization before renaming) so normalizeCursorToolInput receives the real toolName, and ensure the ApplyPatch branch in normalizeCursorToolInput still assigns patch from input.patch ?? input.diff ?? input.content when toolName === 'ApplyPatch' so aliases aren't dropped; update the caller that currently maps rawToolName === 'ApplyPatch' ? 'Edit' : rawToolName (or the order of operations) to preserve the original name for the helper.
🤖 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/providers/list/cursor/cursor-sessions.provider.ts`:
- Around line 354-356: Remove the raw payload console.log calls that leak
conversation content: in normalizeMessage and normalizeCursorBlobs, either
delete the console.log statements or gate them behind an explicit debug flag or
structured logger (e.g., use an injected processLogger.debug or check a config
like DEBUG_CURSOR_LOGS before logging). Locate the console.log in
normalizeMessage (reading raw with readObjectRecord) and the corresponding log
in normalizeCursorBlobs, and replace with conditional debugging or remove
entirely so sensitive NDJSON payloads are not printed in production logs.
---
Nitpick comments:
In `@server/modules/providers/list/cursor/cursor-sessions.provider.ts`:
- Around line 179-187: The ApplyPatch branch in normalizeCursorToolInput is
never hit because rawToolName is rewritten to 'Edit' before calling the helper;
change the call site to pass the original rawToolName (or run normalization
before renaming) so normalizeCursorToolInput receives the real toolName, and
ensure the ApplyPatch branch in normalizeCursorToolInput still assigns patch
from input.patch ?? input.diff ?? input.content when toolName === 'ApplyPatch'
so aliases aren't dropped; update the caller that currently maps rawToolName ===
'ApplyPatch' ? 'Edit' : rawToolName (or the order of operations) to preserve the
original name for the helper.
🪄 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: ef291e01-2dd6-40e9-bf08-c00494f83b80
📒 Files selected for processing (1)
server/modules/providers/list/cursor/cursor-sessions.provider.ts
…y sees The session `total` value was diverging from the number of rendered chat rows, which created confusing UI states (for example: "showing 12 of 21" when only 12 messages exist visually). Why this was happening: - Providers were counting transport/normalized records, not renderable chat rows. - `tool_result` records are normalized and needed for tool wiring, but the UI does not render them as standalone bubbles; they are attached to their corresponding `tool_use`. - As a result, totals were inflated by implementation details in the history format rather than user-visible conversation content. Why this change: - `total` is a user-facing metric and should represent frontend-visible messages. - We need provider behavior to be consistent (Codex/Claude/Cursor/Gemini) so pagination labels, load-more affordances, and session stats match user expectations. - Correctness here is UX-critical: users interpret `total` as conversation message count, not internal event count. Implementation approach: - Replace post-hoc generic counting logic with explicit per-provider total trackers in each `fetchHistory` flow. - Increment totals during provider message processing so counting rules are owned by the provider pipeline itself. - Exclude `tool_result` from the total tracker since it is rendered as attached tool output, not as a standalone chat message. Behavioral impact: - `total` now aligns with rendered chat rows. - Pagination mechanics remain based on normalized history payloads, so loading behavior is unchanged while user-visible totals are corrected. - Tool result attachment behavior is preserved. Touched providers: - codex-sessions.provider.ts - claude-sessions.provider.ts - cursor-sessions.provider.ts - gemini-sessions.provider.ts
…iteboon/claudecodeui into fix/websocket-streaming-issues
…iteboon/claudecodeui into fix/websocket-streaming-issues
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/modules/providers/list/codex/codex-sessions.provider.ts (2)
562-562: 💤 Low valueDrop or gate the per-session debug
console.log.This fires on every history fetch (session open, pagination) and emits an unstructured info-level line. Either remove it or route through the project logger / a debug flag so production logs aren't churned.
♻️ Suggested change
- console.log(`[CodexProvider] Loaded ${total} frontend messages (${totalNormalized} normalized) for session "${sessionId}".`); -If you want to keep the diagnostic, prefer the same logger used elsewhere in the providers and demote it to debug.
🤖 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/providers/list/codex/codex-sessions.provider.ts` at line 562, Replace the unstructured console.log in codex-sessions.provider.ts with the module/project logger at debug level (or gate it behind an explicit debug flag); specifically, stop calling console.log(`[CodexProvider] Loaded ${total} frontend messages (${totalNormalized} normalized) for session "${sessionId}".`) and instead call the provider's logger (e.g., this.logger.debug or processLogger.debug) or wrap the message in a conditional like if (DEBUG) { logger.debug(...) }, using the existing logger instance used elsewhere in this provider so the message is structured and only emitted in debug mode.
523-525: 💤 Low valueSame dead-pagination observation as Claude.
getCodexSessionMessagesis now only called with(sessionId, null, 0), so itslimit/offsetparameters and the slicing branch at lines 239–253 are dead on this path. Optional cleanup — fine to defer, but worth a follow-up.🤖 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/providers/list/codex/codex-sessions.provider.ts` around lines 523 - 525, getCodexSessionMessages is always invoked here as getCodexSessionMessages(sessionId, null, 0) which makes its limit/offset parameters and the in-function slicing branch dead; either remove the unused pagination parameters and the slicing branch from getCodexSessionMessages (and update all callers to the simpler signature) or change this call to pass meaningful limit/offset values so the slicing branch is exercised—locate getCodexSessionMessages and its callers to apply the chosen cleanup and ensure the function and callers (including the call in codex-sessions.provider.ts) stay consistent.server/modules/providers/list/claude/claude-sessions.provider.ts (1)
417-419: 💤 Low valueThe pagination parameters are now dead code on this call path.
Loading full history with
getSessionMessages(sessionId, null, 0)simplifies the message normalization and makestotalcorrectly reflect frontend-renderable rows. However, thelimitandoffsetparameters (and the JSONL-pagination branch at lines 180–195) are unreachable from this call site. Consider removing these unused parameters from the function signature and inlining the pagination logic elsewhere if other callers need it, since only thisfetchHistorypath now exercises the function.🤖 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/providers/list/claude/claude-sessions.provider.ts` around lines 417 - 419, getSessionMessages currently accepts unused pagination params (limit, offset) and contains an unreachable JSONL-pagination branch when invoked from fetchHistory; remove the dead params from getSessionMessages' signature and all callers (update fetchHistory to call getSessionMessages(sessionId) without limit/offset), delete or inline the JSONL-pagination branch inside getSessionMessages (the logic for JSONL pagination should be moved only to callers that actually need it), and update any related tests/type annotations to match the new signature.
🤖 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/providers/list/gemini/gemini-sessions.provider.ts`:
- Around line 531-541: The returned `total` currently counts all
non-`tool_result` entries across `normalized` while pagination/hasMore operate
on the raw `normalized` slice, which confuses UI; change the `total` value
returned by the provider to reflect the count of non-`tool_result` messages in
the current page slice (i.e., compute total as the length or non-`tool_result`
count of `messages` instead of iterating `normalized`), keep `hasMore` logic
unchanged, and make the same change in `claude-sessions.provider.ts` and
`codex-sessions.provider.ts` so `total` consistently represents the number of
renderable messages returned for that page (use `messages.length` or a filtered
count of `messages` to locate the fix).
---
Nitpick comments:
In `@server/modules/providers/list/claude/claude-sessions.provider.ts`:
- Around line 417-419: getSessionMessages currently accepts unused pagination
params (limit, offset) and contains an unreachable JSONL-pagination branch when
invoked from fetchHistory; remove the dead params from getSessionMessages'
signature and all callers (update fetchHistory to call
getSessionMessages(sessionId) without limit/offset), delete or inline the
JSONL-pagination branch inside getSessionMessages (the logic for JSONL
pagination should be moved only to callers that actually need it), and update
any related tests/type annotations to match the new signature.
In `@server/modules/providers/list/codex/codex-sessions.provider.ts`:
- Line 562: Replace the unstructured console.log in codex-sessions.provider.ts
with the module/project logger at debug level (or gate it behind an explicit
debug flag); specifically, stop calling console.log(`[CodexProvider] Loaded
${total} frontend messages (${totalNormalized} normalized) for session
"${sessionId}".`) and instead call the provider's logger (e.g.,
this.logger.debug or processLogger.debug) or wrap the message in a conditional
like if (DEBUG) { logger.debug(...) }, using the existing logger instance used
elsewhere in this provider so the message is structured and only emitted in
debug mode.
- Around line 523-525: getCodexSessionMessages is always invoked here as
getCodexSessionMessages(sessionId, null, 0) which makes its limit/offset
parameters and the in-function slicing branch dead; either remove the unused
pagination parameters and the slicing branch from getCodexSessionMessages (and
update all callers to the simpler signature) or change this call to pass
meaningful limit/offset values so the slicing branch is exercised—locate
getCodexSessionMessages and its callers to apply the chosen cleanup and ensure
the function and callers (including the call in codex-sessions.provider.ts) stay
consistent.
🪄 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: 69be1a9a-73bf-4894-a79e-8c189d2cb2d2
📒 Files selected for processing (5)
server/modules/providers/list/claude/claude-sessions.provider.tsserver/modules/providers/list/codex/codex-sessions.provider.tsserver/modules/providers/list/cursor/cursor-sessions.provider.tsserver/modules/providers/list/gemini/gemini-sessions.provider.tssrc/components/chat/hooks/useChatSessionState.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/chat/hooks/useChatSessionState.ts
- server/modules/providers/list/cursor/cursor-sessions.provider.ts
…port rows Cursor sessions were still showing inflated totals after the earlier total-count work, producing UX mismatches like "Showing 16 of 31" while only ~16 visible chat rows existed. Why this happened: - Cursor history normalization emits both `tool_use` and `tool_result` entries. - The UI intentionally does not render `tool_result` as its own message bubble; it attaches tool results onto the related `tool_use` card. - Total counting that includes `tool_result` therefore measures transport artifacts, not user-visible conversation rows. - In practice, this makes totals look wrong and undermines trust in pagination status and session message counts. Why this change: - `total` is a user-facing semantic value and must represent what the user can actually see in the timeline. - Cursor should follow the same rendered-message counting rule as other providers to keep behavior predictable across providers. - Avoiding hidden/internal row counts in totals prevents confusion in "showing X of Y" indicators and load-more expectations. What was changed: - Replaced implicit/raw-style counting with explicit provider-side total tracking in Cursor history fetch. - Total tracker increments only for processed messages that map to rendered chat rows, excluding standalone `tool_result` entries. - Pagination windowing remains based on full normalized history payload to avoid changing ordering/loading mechanics; only displayed `total` semantics were corrected. Result: - Cursor `total` now reflects rendered chat rows rather than internal normalized event count. - Session counters are now aligned with what users perceive in the UI.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/providers/list/cursor/cursor-sessions.provider.ts`:
- Around line 557-570: The code currently remaps ApplyPatch to Edit before
calling normalizeCursorToolInput, so normalizeCursorToolInput never sees the
original "ApplyPatch" name and diff-only payloads lose their normalized patch
field; change the call sequence so normalizeCursorToolInput is invoked with the
original tool identifier (rawToolName) or otherwise pass rawToolName as the name
parameter to normalizeCursorToolInput (instead of toolName), then perform the
ApplyPatch -> Edit rename afterwards when assigning toolName for the message;
update references around rawToolName, toolName, normalizeCursorToolInput, and
the createNormalizedMessage call accordingly.
- Around line 389-404: The pagination logic counts renderable rows into total by
excluding messages with kind === 'tool_result' but still slices and computes
hasMore from allNormalized, causing offset/limit to drift; fix by deriving a
filtered array (e.g., renderableMessages = allNormalized.filter(m => m.kind !==
'tool_result')) and use that for total, page (use
renderableMessages.slice(start, start + limit)) and hasMore (compare against
renderableMessages.length) instead of allNormalized; update references to
totalNormalized, total, page, and hasMore in the cursor-sessions provider so
pagination aligns with the same filtered collection.
🪄 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: 18be54f3-3d27-462e-9d73-c8acaaf49ed0
📒 Files selected for processing (2)
server/modules/providers/list/cursor/cursor-sessions.provider.tssrc/components/chat/hooks/useChatSessionState.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/chat/hooks/useChatSessionState.ts
…e deterministic Why: - Gemini exit-code 41 auth guidance was defined in two places with slightly different wording, which creates drift and inconsistent UX when auth fails. - Gemini env bootstrap only short-circuited when *all* auth-related vars were present; that forced unnecessary user-level env file reads even when a valid primary credential already existed. - Codex session registration only populated the active-session map when an id was unseen; resumed sessions could retain stale thread/abort/status objects, leading to incorrect lifecycle behavior. - Debug console logs in runtime request paths added noise without operational value. What changed: - Removed exit-code 41 from mapGeminiExitCodeToMessage and kept a single authoritative 41 message in the close-handler auth branch. - Aligned 41 remediation phrasing to consistently reference a valid GEMINI_API_KEY. - Updated �uildGeminiProcessEnv to return early when any primary auth signal is already present (GEMINI_API_KEY, GOOGLE_API_KEY, or GOOGLE_APPLICATION_CREDENTIALS). - Changed Codex egisterSession to always overwrite session state for valid ids so resumed runs replace stale entries. - Removed unnecessary console.log debug statements in touched runtime paths, including the chat submit debug log. Impact: - More predictable Gemini authentication error messaging. - Avoids needless env-file fallback when credentials are already available. - Prevents stale Codex session controllers/status from leaking across resumes. - Cleaner logs with no behavior change to error/warn reporting.
…der and useChatSessionState
Why: - Cursor normalization emits internal tool_result items so tool outputs can attach to tool cards. - The UI does not render tool_result as standalone rows. - Pagination previously mixed datasets: total excluded tool_result, but page slicing and hasMore used the unfiltered collection. - That mismatch caused offset and limit drift versus what users actually see. - Tool normalization also renamed ApplyPatch to Edit before input normalization. - That hid the original tool identity from normalizeCursorToolInput and could drop patch-specific shaping. What changed: - Added one pagination source of truth: renderableMessages filters out tool_result. - total, page slicing, and hasMore now all use renderableMessages. - Unlimited-history responses now return renderableMessages for consistent semantics. - normalizeCursorToolInput now receives rawToolName first. - The user-facing rename from ApplyPatch to Edit is still preserved in toolName. Impact: - Offset and limit now map to visible Cursor rows. - hasMore reflects remaining renderable history. - ApplyPatch payloads keep patch-aware normalization while preserving UI naming.
Claude writes slash-command metadata, local stdout, and compaction summaries into the same JSONL stream as normal chat messages. The existing normalization path treated those rows as internal content and dropped them entirely. That made the web UI diverge from the CLI transcript and removed important context. Commands like /compact appeared to have never happened, the stdout status line disappeared, and the continuation summary after compaction was filtered out even though it best describes the post-boundary session state. This change keeps the distinction between truly internal transcript rows and user-visible local command artifacts. Command wrapper tags are parsed into structured metadata without exposing the raw tags, local command stdout is remapped to assistant text, and compact summaries are preserved as assistant-authored content instead of being mislabeled as user input. Search and session-summary parsing are updated for the same reason. If history normalization preserved these rows but search still ignored them, rendered conversation state and searchable conversation state would continue to disagree, and session summaries would fall back to stale user text instead of Claude's actual compaction summary. The shared message and store typings are extended so this metadata survives the full backend-to-frontend pipeline. That avoids reconstructing meaning later and keeps the transcript faithful to Claude's persisted history while still hiding genuinely internal control content.
This was
linked to
issues
May 8, 2026
Users previously had an all-or-nothing choice for completed sessions: either keep them in the active sidebar or permanently delete them. That made long-lived usage brittle because valuable history stayed in the way unless users destroyed it. This change introduces archiving as a first-class lifecycle so completed work can be hidden without losing transcript history, workspace context, or restoreability. The backend now persists session archive state and excludes archived rows from active session queries by default. Dedicated archive queries and routes make archived sessions and archived workspaces addressable on their own, which is necessary once hidden data can no longer be rebuilt from the active project list. Hard-delete behavior still cleans up transcript files so destructive deletes remain truly destructive. The frontend now mirrors that lifecycle in the sidebar. Delete flows distinguish between archive and permanent delete, archived sessions can be restored, archived workspaces appear beside standalone archived sessions, and archived project sessions open with the correct workspace context instead of routing to a session URL that leaves the main view empty. Follow-up archive UI polish keeps the status affordances explicit without competing with workspace names.
Conversation search still surfaced hidden data after archiving because it only filtered out archived session rows. Active session rows that belonged to archived projects were still indexed, which broke the expectation that archived work disappears from normal search until it is restored. This change makes search follow the visible workspace model by skipping any session whose owning project is archived before ripgrep and transcript parsing begin. The archive-state lookup is cached per project path so the behavior is corrected without adding repeated database work during a scan.
viper151
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
This branch stabilizes session lifecycle handling across providers, removes temporary-session workarounds in the UI, and fixes Gemini/Cursor synchronization and headless runtime behavior. The core goal is consistency: one canonical session identity, discovered when the provider actually emits it, and one reliable source of truth for auth and session metadata.
Why This Change Was Needed
thread.started/init), but parts of our stack still behaved as if IDs were immediately known.What Changed (and Why)
Canonical async session ID lifecycle for Codex:
openai-codex.jsnow treatsthread.startedas the authoritative moment for new session ID discovery.session_createdis emitted only after canonical ID discovery.This prevents early registration with guessed/fallback IDs and keeps abort/routing/notifications tied to the real provider ID.
Canonical async session ID lifecycle for Gemini:
gemini-cli.jsremoves temporarygemini_${Date.now()}IDs.New sessions are created only when Gemini emits
init.session_id.cliSessionIdpersistence remains, but now anchored to canonical identity from first discovery.This aligns Gemini with Claude/Codex behavior and removes synthetic-ID bookkeeping.
Frontend removal of temporary-session plumbing:
useChatComposerState.ts,useChatRealtimeHandlers.ts,useSessionProtection.ts,ChatInterface.tsx,MainContent.tsx,AppContent.tsx, relatedtypes.tsfiles, anduseProjectsState.tswere simplified to stop depending onnew-session-*placeholders.Control events (
session_created,complete,status, permission lifecycle) are no longer persisted as renderable realtime messages.This reduces state churn, removes workaround logic, and narrows realtime storage to UI-renderable content.
Gemini headless auth/runtime hardening:
gemini-cli.jsnow builds subprocess env using process env plus user-level fallback files (~/.gemini/.env, then~/.env, honoringGEMINI_CLI_HOME).gemini-cli.jsadds--skip-trustfor non-interactive web execution.gemini-auth.provider.tsnow mirrors Gemini CLI auth context more accurately, including selected auth type and user-level env fallbacks.Error mapping for Gemini exit codes (especially 41) is more actionable.
Why: web server subprocesses often do not share interactive shell auth context; this closes that practical gap.
Gemini/Cursor session synchronization dedupe and path reliability:
gemini-session-synchronizer.provider.tsandsessions-watcher.service.tsnow avoid duplicate mirrored Gemini session roots.cursor-session-synchronizer.provider.tsandsessions-watcher.service.tsnow synchronize from a more reliable root/path strategy.Why: avoid duplicate rows/events and reduce dependence on fragile derived mappings.
Session timestamp contract cleanup:
sidebar/utils/utils.tsnow relies on canonicallastActivitybehavior instead of legacy mixed fallback logic.Why: reduce ambiguity and keep sidebar sort/display logic aligned with current API contract.
Model list update:
shared/modelConstants.jsaddsgemini-2.5-flash-lite.Why: keep selectable model options aligned with runtime support.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes