Fix/UI session history sync#57077
Conversation
Greptile SummaryThis PR fixes two related session-history bugs in the Chat UI: (1) a race condition where an older in-flight Key changes:
One P2 inconsistency to be aware of: The overview-panel Confidence Score: 5/5Safe to merge — both bugs are correctly fixed with proper guards and updated tests; the only remaining finding is a minor state-clearing inconsistency in an edge-case path. The stale-history serial guard is implemented correctly (WeakMap per state, finally-scoped loading flag, error path also guarded). The nav reset-to-main removal is straightforward and the updated test locks the intended behavior. All remaining findings are P2: the overview-panel handler's incomplete stream-field clearing is a pre-existing gap that this PR makes more notable by adding other field clearing, but it doesn't affect the primary user flows or introduce data loss. ui/src/ui/app-render.ts — overview-panel onSessionKeyChange handler is missing chatStream/chatStreamStartedAt/chatRunId clearing compared to switchChatSession and the chat-tab handler.
|
| Filename | Overview |
|---|---|
| ui/src/ui/controllers/chat.ts | Adds a module-level WeakMap serial counter to guard against stale chat.history responses. Implementation is correct: serial is captured before the async call, incremented per request, and compared on resolution/rejection/finally. Stale responses are silently dropped; chatLoading correctly stays true until the latest in-flight request settles. |
| ui/src/ui/app-render.helpers.ts | Removes the forced reset-to-main logic that ran when clicking Chat in the sidebar, and adds missing fields (chatAttachments, chatMessages, chatToolMessages, chatThinkingLevel) to switchChatSession's state clear. Both changes are correct. |
| ui/src/ui/app-render.ts | Adds clearing of chatMessages, chatToolMessages, chatThinkingLevel (and chatQueue in the overview handler) on session change. Chat-tab handler is now complete; overview-panel handler is still missing chatStream/chatStreamStartedAt/chatRunId clearing. |
| ui/src/ui/controllers/chat.test.ts | New test correctly validates the stale-response guard: first request is held pending while second resolves, then first is resolved; assertions confirm only the second (newer) response's data is applied to state. |
| ui/src/ui/navigation.browser.test.ts | Test description and assertions updated to reflect the new intended behavior: session is preserved (not reset to main) when clicking Chat from the sidebar. URL encoding of colons is correctly asserted. |
Comments Outside Diff (1)
-
ui/src/ui/app-render.ts, line 650-665 (link)Incomplete state reset in overview
onSessionKeyChangeThis handler now clears
chatMessages,chatToolMessages,chatThinkingLevel, andchatQueue, but leaveschatStream,chatStreamStartedAt, andchatRunIduntouched. The siblingswitchChatSessionfunction inapp-render.helpers.tsand the chat-tab's ownonSessionKeyChange(line ~1415) both clear those streaming fields. If a stream is in flight when the user switches session via the overview panel's selector, those values will persist and could surface a stale streaming bubble when the user returns to the Chat tab.Consider aligning with
switchChatSession:Prompt To Fix With AI
This is a comment left during a code review. Path: ui/src/ui/app-render.ts Line: 650-665 Comment: **Incomplete state reset in overview `onSessionKeyChange`** This handler now clears `chatMessages`, `chatToolMessages`, `chatThinkingLevel`, and `chatQueue`, but leaves `chatStream`, `chatStreamStartedAt`, and `chatRunId` untouched. The sibling `switchChatSession` function in `app-render.helpers.ts` and the chat-tab's own `onSessionKeyChange` (line ~1415) both clear those streaming fields. If a stream is in flight when the user switches session via the overview panel's selector, those values will persist and could surface a stale streaming bubble when the user returns to the Chat tab. Consider aligning with `switchChatSession`: How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: ui/src/ui/app-render.ts
Line: 650-665
Comment:
**Incomplete state reset in overview `onSessionKeyChange`**
This handler now clears `chatMessages`, `chatToolMessages`, `chatThinkingLevel`, and `chatQueue`, but leaves `chatStream`, `chatStreamStartedAt`, and `chatRunId` untouched. The sibling `switchChatSession` function in `app-render.helpers.ts` and the chat-tab's own `onSessionKeyChange` (line ~1415) both clear those streaming fields. If a stream is in flight when the user switches session via the overview panel's selector, those values will persist and could surface a stale streaming bubble when the user returns to the Chat tab.
Consider aligning with `switchChatSession`:
```suggestion
onSessionKeyChange: (next) => {
state.sessionKey = next;
state.chatMessage = "";
state.chatAttachments = [];
state.chatMessages = [];
state.chatToolMessages = [];
state.chatThinkingLevel = null;
state.chatQueue = [];
state.chatStream = null;
state.chatStreamStartedAt = null;
state.chatRunId = null;
state.resetToolStream();
state.applySettings({
...state.settings,
sessionKey: next,
lastActiveSessionKey: next,
});
void state.loadAssistantIdentity();
},
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "UI: keep selected chat session when retu..." | Re-trigger Greptile
18e3e7a to
1948579
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ccb5d0947
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Hi @BunsDev , could you please take a look at this PR? The CI failure seems unrelated to my changes |
|
Please resolve conflicts @bbddbb1 |
b57adfe to
5826e57
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5826e57a6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fix P2 Badge: The mobile picker now uses .selected=${live(...)} instead of
?selected attribute binding to reliably force the live selected option
after rerender, preventing stale selection display when state.sessionKey
changes externally (navigation/deep-link rehydration).
Conflicts have been resolved. I’ve verified the changes locally. Ready for another look! |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. A focused Lit render test can render the session select, change Next step before merge Security Review findings
Review detailsBest possible solution: Port the selector-state fix onto current main by updating Do we have a high-confidence way to reproduce the issue? Yes. A focused Lit render test can render the session select, change Is this the best way to solve the issue? No for the PR branch as currently proposed, because it targets an obsolete desktop render location. The underlying Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4655ee803d27. |
Summary
Describe the problem and fix in 2–5 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
<select>relied on.valuewhile its option list could be rebuilt during rerender.chat-controls__sessionclass as the desktop picker, which made selector targeting ambiguous.git blame, prior PR, issue, or refactor if known): Earlier investigation focused on async history/nav flows because the symptom looked like session-state drift; the exact repro showed the thread content was already correct while only the selector was stale./chat?session=...and the session options are reconstructed around the current session.chat.historyoverwrite, and not a session reset caused by auth/token handling.Regression Test Plan (if applicable)
ui/src/ui/views/chat.test.tsUser-visible / Behavior Changes
Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
sessionquery param or after returning to Chat with a previously selected sessionSteps
/chat?session=agent%3Amain%3Atest-b.Expected
Actual
test-ewhile the rendered thread was alreadytest-b.Evidence
Attach at least one:
Local verification:
pnpm test -- ui/src/ui/views/chat.test.ts ui/src/ui/controllers/chat.test.tsbefore
With this PR:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm test -- ui/src/ui/views/chat.test.ts ui/src/ui/controllers/chat.test.ts.sessionquery param and verifying repeated page switches do not desync the picker.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
<select>behavior can be sensitive when options are rebuilt dynamically.data-chat-session-selectmarkers.