fix: per-session message queue, conversation restore, token usage i18n + CI race - #147
Merged
Conversation
…th test HandleWS wrote the welcome frame before registering b.conn, so a client that checks Connected() right after welcome could observe 'not connected' under CI scheduling. Register before the welcome write (rolling back on failure) and route the welcome through writeJSON so it stays serialized with command writes now that the conn is visible to Backend() callers earlier. Also make keepAlivePing/keepAliveWait atomic: tests rewrite these package vars while leftover keepAlive goroutines from prior tests still read them (-race -count=100 flagged it once the first race was fixed). Verified: go test -race -count=100 ./internal/browser/ (86s clean).
… usage i18n
Queue: queued messages lived in one global array wiped by clearChat on
every session switch, and agent_done events for non-viewed sessions
were dropped, so background queues never drained. Queue is now stashed
per session id, sendMessage accepts {sessionId, background}, agent_done
carries task_id end-to-end so the bridge drains the queue of the
session that actually finished (even while viewing another), and
deleting a session discards its queue.
Restore: the server never persisted the active conversation, so a
restart always landed on a fresh welcome session. The web server now
records the last foreground session per project path in
~/.jcode/sessions/last_session.json and reports it via /api/health when
the current engine is a post-restart throwaway, letting clients resume
the previous conversation through the normal loadSession flow.
i18n: ContextCapacityPopup hardcoded all its English strings despite
contextCapacity.* keys existing in all 5 locales — wire it to t() and
add the missing input/output/cached/reasoning keys; replace hardcoded
' tokens' in usage tooltips with a new common.tokens key.
Freshness: loadSession now re-fetches /api/status afterwards so the
token snapshot (and provider/model/mode/running) reflect the switched
session instead of staying cleared until the next LLM call.
📝 WalkthroughWalkthroughThe PR improves WebSocket initialization and keepalive configuration, persists the last foreground session for restart recovery, scopes queued chat messages and completion handling by session, refreshes resumed-session status, and localizes token and context-capacity labels. ChangesBridge connection reliability
Last foreground session continuity
Session-aware queued messaging
Localized token displays
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ws
participant onAgentDone
participant ChatStore
participant sendMessage
Client->>ChatStore: enqueue message for session
ws->>onAgentDone: agent_done with task_id
onAgentDone->>ChatStore: shift queue for task session
ChatStore-->>onAgentDone: next queued message
onAgentDone->>sendMessage: send targeted foreground/background message
sequenceDiagram
participant Engine
participant SessionStore
participant HealthAPI
participant Client
Engine->>SessionStore: save foreground session
Client->>HealthAPI: request health
HealthAPI->>SessionStore: load last session
SessionStore-->>HealthAPI: persisted session ID
HealthAPI-->>Client: return session_id
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![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
🧹 Nitpick comments (1)
internal/session/lastsession.go (1)
92-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the resolved directory path.
config.SessionsDir()was already successfully evaluated at the top of this function vialastSessionPath(). You can derive the directory path directly frompto avoid redundant function calls and error checks.♻️ Proposed refactor
- dir, err := config.SessionsDir() - if err != nil { - return "" - } - if _, err := os.Stat(filepath.Join(dir, id+".json")); err != nil { + if _, err := os.Stat(filepath.Join(filepath.Dir(p), id+".json")); err != nil { return "" }🤖 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 `@internal/session/lastsession.go` around lines 92 - 95, In the function containing lastSessionPath(), replace the repeated config.SessionsDir() call and its error handling with the directory derived directly from the already-resolved path variable p. Preserve the existing directory-based behavior while reusing the path resolution performed earlier.
🤖 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 `@internal/session/lastsession_test.go`:
- Around line 12-13: Update TestLastSessionRoundTrip in
internal/session/lastsession_test.go at lines 12-13 to store t.TempDir() in tmp
and set both HOME and USERPROFILE to tmp; at lines 57-58 and 67-68, set both
environment variables to isolated temporary directories using t.TempDir(),
ensuring all last-session tests avoid the real home directory on every platform.
In `@internal/session/lastsession.go`:
- Around line 18-22: Update lastSessionPath to wrap the error returned by
config.SessionsDir with fmt.Errorf, adding the tool name as context while
preserving the original error via %w; add the required fmt import if needed.
In `@web/src/app/wsBridge.ts`:
- Around line 82-85: Update the queued-message handling around shiftQueued and
sendMessage so the item remains queued until the send thunk fulfills. Await the
dispatched sendMessage result, and only remove the message after success; on
rejection, preserve or restore next at the front of the queue.
In `@web/src/components/SettingsDialog.tsx`:
- Line 2950: Update the daily trend tooltip title in the relevant SettingsDialog
rendering to use the existing common.tokens translation key instead of
chat.tokens, while preserving the current interpolation and formatting.
---
Nitpick comments:
In `@internal/session/lastsession.go`:
- Around line 92-95: In the function containing lastSessionPath(), replace the
repeated config.SessionsDir() call and its error handling with the directory
derived directly from the already-resolved path variable p. Preserve the
existing directory-based behavior while reusing the path resolution performed
earlier.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c2dbabc-b37d-4b13-8fe0-82d35d5cce8a
📒 Files selected for processing (18)
internal/browser/bridge.gointernal/browser/bridge_test.gointernal/session/lastsession.gointernal/session/lastsession_test.gointernal/web/engine.gointernal/web/server.goweb/src/app/runtime.tsweb/src/app/store.tsweb/src/app/wsBridge.tsweb/src/components/ChatInput.tsxweb/src/components/SettingsDialog.tsxweb/src/components/Sidebar.tsxweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/lib/ws.ts
Comment on lines
+12
to
+13
| func TestLastSessionRoundTrip(t *testing.T) { | ||
| t.Setenv("HOME", t.TempDir()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix home directory mocking for Windows test isolation.
Go's os.UserHomeDir() natively relies on the USERPROFILE environment variable on Windows, not HOME. Mocking only HOME means the tests will read from and write to the developer's actual home directory on Windows machines, which can pollute the workspace or fail due to permissions. Set both variables to ensure robust cross-platform test isolation.
internal/session/lastsession_test.go#L12-L13: Declaretmp := t.TempDir()and use it to set bothHOMEandUSERPROFILE.internal/session/lastsession_test.go#L57-L58: Set bothHOMEandUSERPROFILEwitht.TempDir().internal/session/lastsession_test.go#L67-L68: Set bothHOMEandUSERPROFILEwitht.TempDir().
📍 Affects 1 file
internal/session/lastsession_test.go#L12-L13(this comment)internal/session/lastsession_test.go#L57-L58internal/session/lastsession_test.go#L67-L68
🤖 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 `@internal/session/lastsession_test.go` around lines 12 - 13, Update
TestLastSessionRoundTrip in internal/session/lastsession_test.go at lines 12-13
to store t.TempDir() in tmp and set both HOME and USERPROFILE to tmp; at lines
57-58 and 67-68, set both environment variables to isolated temporary
directories using t.TempDir(), ensuring all last-session tests avoid the real
home directory on every platform.
Comment on lines
+18
to
+22
| func lastSessionPath() (string, error) { | ||
| dir, err := config.SessionsDir() | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the returned error.
As per coding guidelines, use fmt.Errorf("tool_name: %w", err) for wrapped errors in non-tool code.
🛠️ Proposed fix
func lastSessionPath() (string, error) {
dir, err := config.SessionsDir()
if err != nil {
- return "", err
+ return "", fmt.Errorf("session: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func lastSessionPath() (string, error) { | |
| dir, err := config.SessionsDir() | |
| if err != nil { | |
| return "", err | |
| } | |
| func lastSessionPath() (string, error) { | |
| dir, err := config.SessionsDir() | |
| if err != nil { | |
| return "", fmt.Errorf("session: %w", err) | |
| } |
🤖 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 `@internal/session/lastsession.go` around lines 18 - 22, Update lastSessionPath
to wrap the error returned by config.SessionsDir with fmt.Errorf, adding the
tool name as context while preserving the original error via %w; add the
required fmt import if needed.
Source: Coding guidelines
Comment on lines
+82
to
+85
| if (key && queued && queued.length > 0) { | ||
| const next = queued[0] | ||
| dispatch(chatActions.drainQueue()) | ||
| void dispatch(sendMessage({ text: next.text, images: next.images }) as never) | ||
| dispatch(chatActions.shiftQueued(key)) | ||
| void dispatch(sendMessage({ text: next.text, images: next.images, sessionId: key, background: !isForeground }) as never) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not permanently dequeue the message before its send succeeds.
shiftQueued runs synchronously, but the dispatched sendMessage thunk may reject. Any transient API failure therefore silently loses the queued message. Retain it until fulfillment or restore it at the front on rejection.
🤖 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 `@web/src/app/wsBridge.ts` around lines 82 - 85, Update the queued-message
handling around shiftQueued and sendMessage so the item remains queued until the
send thunk fulfills. Await the dispatched sendMessage result, and only remove
the message after success; on rejection, preserve or restore next at the front
of the queue.
| <div | ||
| key={d.date} | ||
| title={`${d.date} · ${fmtCompact(d.tokens)} tokens · ${d.turns} ${t('settings.usageStats.turnsUnit')}`} | ||
| title={`${d.date} · ${t('chat.tokens', { used: fmtCompact(d.tokens) })} · ${d.turns} ${t('settings.usageStats.turnsUnit')}`} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the existing common.tokens key.
The locale files define common.tokens, not chat.tokens. As written, daily trend tooltips can display the missing translation key instead of the token label.
- title={`${d.date} · ${t('chat.tokens', { used: fmtCompact(d.tokens) })} · ${d.turns} ${t('settings.usageStats.turnsUnit')}`}
+ title={`${d.date} · ${t('common.tokens', { used: fmtCompact(d.tokens) })} · ${d.turns} ${t('settings.usageStats.turnsUnit')}`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| title={`${d.date} · ${t('chat.tokens', { used: fmtCompact(d.tokens) })} · ${d.turns} ${t('settings.usageStats.turnsUnit')}`} | |
| title={`${d.date} · ${t('common.tokens', { used: fmtCompact(d.tokens) })} · ${d.turns} ${t('settings.usageStats.turnsUnit')}`} |
🤖 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 `@web/src/components/SettingsDialog.tsx` at line 2950, Update the daily trend
tooltip title in the relevant SettingsDialog rendering to use the existing
common.tokens translation key instead of chat.tokens, while preserving the
current interpolation and formatting.
This was referenced Jul 18, 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
1 participant
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.
Summary
Three desktop/web conversation UX fixes plus the CI race fix from main:
1. Queued messages survive session switching — the type-ahead queue was a single global array wiped by
clearChaton every session switch, andagent_donefor non-viewed sessions was dropped by the WS client, so background queues never drained.queuedBySession);sendMessageaccepts{sessionId, background}so background sends don't touch the foreground timelineagent_donecarriestask_idend-to-end (merged into data like approval events) and is let through the foreground filter; the bridge drains the queue of the session that actually finished, even while viewing another conversation2. Restore last conversation after restart — the server never persisted the active conversation, so a restart always landed on a fresh welcome session.
internal/session/lastsession.go: persists the last foreground session per project path in~/.jcode/sessions/last_session.json(atomic tmp+rename, validates the session file still exists)/api/healthreports the persisted session when the current engine is a post-restart throwaway, so clients resume via the normalloadSessionflow; remote workspaces are keyed by remote path3. Token usage i18n + freshness
ContextCapacityPopuphardcoded all English strings despitecontextCapacity.*keys existing in all 5 locales — wired tot(), added missinginput/output/cached/reasoningkeys; replaced hardcodedtokensin usage tooltips withcommon.tokensloadSessionnow re-fetches/api/statusafterwards so the token snapshot (and provider/model/mode/running) reflect the switched session instead of staying cleared until the next LLM call4. Deflake
TestBridgeTokenAuth(main CI failure after #141) —HandleWSwrote the welcome frame before registeringb.conn, so a client checkingConnected()right after welcome could observe 'not connected'. Register before the welcome write (rolling back on failure), route welcome throughwriteJSON, and makekeepAlivePing/keepAliveWaitatomic (tests rewrite them while leftover keepAlive goroutines still read them).Testing
go build ./...,go vet ./...✓go test ./internal/browser/ ./internal/session/ ./internal/web/✓go test -race -count=20 ./internal/browser/✓ (was 100× clean pre-rebase)make lint-web(tsc: web + both packages) ✓golangci-linton changed packages: 0 issues ✓lastsession_test.go: round-trip / stale-id / invalid-input cases ✓Summary by CodeRabbit
New Features
Bug Fixes
Localization