feat(ui): move chat sessions into the sidebar#52746
feat(ui): move chat sessions into the sidebar#52746bobashopcashier wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR moves chat session switching from a horizontal pill row and desktop header dropdown into the left sidebar's CHAT section, replacing the old Key changes:
Issues found:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/ui/app-render.helpers.ts
Line: 619
Comment:
**`window.prompt()` blocks the UI thread**
`window.prompt()` is a synchronous, browser-native blocking dialog that freezes all JS execution and rendering while open. It also can't be styled, may be suppressed in embedded or restricted browser contexts, and is generally inconsistent with the rest of the app's UI language.
Consider replacing it with a lightweight inline input field or a small modal — something like a named input that appears in the sidebar header itself, which would be more in keeping with the sidebar mental model this PR introduces.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/ui/app-render.helpers.ts
Line: 636-640
Comment:
**Attachments can leak into new session when message changed during creation**
`switchChatSession` clears `chatMessage` but does **not** clear `chatAttachments`. The current guard only clears attachments when the message text is unchanged:
```typescript
if (state.chatMessage === composerSnapshot.message) {
state.chatAttachments = [];
}
switchChatSession(state, result.key); // clears chatMessage, but NOT chatAttachments
```
If the user edits the message draft while the async `sessions.create` request is in-flight (i.e. `state.chatMessage !== composerSnapshot.message`), `chatAttachments` is left untouched, so the old session's attachments silently bleed into the new session's composer.
Consider clearing attachments unconditionally before calling `switchChatSession`, or alternatively adding `chatAttachments` clearing inside `switchChatSession` itself for consistency:
```suggestion
state.chatAttachments = [];
switchChatSession(state, result.key);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(ui): move chat sessions into the si..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!client || !state.connected || state.newChatSessionPending) { | ||
| return; | ||
| } | ||
| const raw = window.prompt("Session name (optional):"); |
There was a problem hiding this comment.
window.prompt() blocks the UI thread
window.prompt() is a synchronous, browser-native blocking dialog that freezes all JS execution and rendering while open. It also can't be styled, may be suppressed in embedded or restricted browser contexts, and is generally inconsistent with the rest of the app's UI language.
Consider replacing it with a lightweight inline input field or a small modal — something like a named input that appears in the sidebar header itself, which would be more in keeping with the sidebar mental model this PR introduces.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-render.helpers.ts
Line: 619
Comment:
**`window.prompt()` blocks the UI thread**
`window.prompt()` is a synchronous, browser-native blocking dialog that freezes all JS execution and rendering while open. It also can't be styled, may be suppressed in embedded or restricted browser contexts, and is generally inconsistent with the rest of the app's UI language.
Consider replacing it with a lightweight inline input field or a small modal — something like a named input that appears in the sidebar header itself, which would be more in keeping with the sidebar mental model this PR introduces.
How can I resolve this? If you propose a fix, please make it concise.| return; | ||
| } | ||
| if (state.chatMessage === composerSnapshot.message) { | ||
| state.chatAttachments = []; | ||
| } |
There was a problem hiding this comment.
switchChatSession clears chatMessage but does not clear chatAttachments. The current guard only clears attachments when the message text is unchanged:
if (state.chatMessage === composerSnapshot.message) {
state.chatAttachments = [];
}
switchChatSession(state, result.key); // clears chatMessage, but NOT chatAttachmentsIf the user edits the message draft while the async sessions.create request is in-flight (i.e. state.chatMessage !== composerSnapshot.message), chatAttachments is left untouched, so the old session's attachments silently bleed into the new session's composer.
Consider clearing attachments unconditionally before calling switchChatSession, or alternatively adding chatAttachments clearing inside switchChatSession itself for consistency:
| return; | |
| } | |
| if (state.chatMessage === composerSnapshot.message) { | |
| state.chatAttachments = []; | |
| } | |
| state.chatAttachments = []; | |
| switchChatSession(state, result.key); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-render.helpers.ts
Line: 636-640
Comment:
**Attachments can leak into new session when message changed during creation**
`switchChatSession` clears `chatMessage` but does **not** clear `chatAttachments`. The current guard only clears attachments when the message text is unchanged:
```typescript
if (state.chatMessage === composerSnapshot.message) {
state.chatAttachments = [];
}
switchChatSession(state, result.key); // clears chatMessage, but NOT chatAttachments
```
If the user edits the message draft while the async `sessions.create` request is in-flight (i.e. `state.chatMessage !== composerSnapshot.message`), `chatAttachments` is left untouched, so the old session's attachments silently bleed into the new session's composer.
Consider clearing attachments unconditionally before calling `switchChatSession`, or alternatively adding `chatAttachments` clearing inside `switchChatSession` itself for consistency:
```suggestion
state.chatAttachments = [];
switchChatSession(state, result.key);
```
How can I resolve this? If you propose a fix, please make it concise.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 374df9f9ea
ℹ️ 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".
bobashopcashier
force-pushed
the
feat/chat-sidebar-sessions
branch
2 times, most recently
from
aae50ec to
a9a19b9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57ee056a08
ℹ️ 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".
| const restoreCurrentDraft = state.chatMessage !== composerSnapshot.message; | ||
| const draftToRestore = restoreCurrentDraft ? state.chatMessage : composerSnapshot.message; | ||
| const attachmentsToRestore = restoreCurrentDraft | ||
| ? [...state.chatAttachments] | ||
| : composerSnapshot.attachments; |
There was a problem hiding this comment.
In createNewChatSession(), the restore path only checks whether chatMessage changed while sessions.create was in flight. If the user adds or removes attachments without editing the text, restoreCurrentDraft stays false and the stale composerSnapshot.attachments array is written back after switchChatSession(), so those attachment changes disappear when the new session opens. This is reachable because the new sidebar + leaves the composer interactive during the RPC.
Useful? React with 👍 / 👎.
| export function renderChatSidebarSection(state: AppViewState) { | ||
| if (state.settings.navCollapsed) { | ||
| return html`<section class="nav-section nav-section--chat"> | ||
| ${renderTab(state, "chat", { collapsed: true })} | ||
| </section>`; |
There was a problem hiding this comment.
renderApp() treats a collapsed sidebar as temporarily expanded whenever the drawer is open (navCollapsed = settings.navCollapsed && !navDrawerOpen), but renderChatSidebarSection() still branches on state.settings.navCollapsed. For users who keep the nav collapsed, opening the drawer expands every other section while CHAT remains icon-only, which hides both the session list and the new-session button exactly in the view meant to expose them.
Useful? React with 👍 / 👎.
bobashopcashier
force-pushed
the
feat/chat-sidebar-sessions
branch
from
57ee056 to
6343752
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6343752143
ℹ️ 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".
| ? [...state.chatAttachments] | ||
| : composerSnapshot.attachments; | ||
| state.chatAttachments = []; | ||
| switchChatSession(state, result.key); |
There was a problem hiding this comment.
Because the new + action lives in the global sidebar, it can now be triggered from Overview, Sessions, or any other tab. createNewChatSession() only calls switchChatSession() here and never state.setTab("chat"), so the RPC succeeds but the main pane stays on the old tab and the newly created conversation remains hidden until the user navigates manually. handleSessionClick() in the same file already has to special-case state.tab !== "chat" for exactly this reason.
Useful? React with 👍 / 👎.
| <button | ||
| type="button" | ||
| class="nav-item nav-item--session ${isActive ? "nav-item--active" : ""}" | ||
| data-chat-session-key=${entry.key} | ||
| title=${entry.title} | ||
| aria-current=${isActive ? "page" : "false"} | ||
| @click=${() => handleSessionClick(entry.key)} |
There was a problem hiding this comment.
These sidebar session buttons stay active even when state.connected is false. In that state switchChatSession() still clears chatMessage, queued items, and persists the new key (ui/src/ui/app-render.helpers.ts:586), but loadChatHistory() immediately returns offline (ui/src/ui/controllers/chat.ts:67), so a disconnect can leave the old transcript on screen under a different session while also dropping the current draft. The previous desktop selector was disabled while disconnected, so this is a real regression for gateway-drop scenarios.
Useful? React with 👍 / 👎.
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
CHATsidebar section, adds a sidebar+action backed by realsessions.create, and removes the desktop chat header session selector while keeping the model picker and existing chat controls.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
CHATsidebar section.CHATsection header now has a+action that creates a real new chat session.CHATsidebar section can be collapsed and expanded independently.Security Impact (required)
Yes/No)NoYes/No)NoYes/No)NoYes/No)NoYes/No)NoYes, explain risk + mitigation:Repro + Verification
Environment
pnpmdev buildSteps
CHATsidebar section and the chat canvas does not show a second desktop session-switching surface.+button to create a new session.CHATsection.Expected
+switches to the created session.CHATsection collapse button hides and restores the session list.Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Diff Summary
Current diff against
upstream/main:ui/src/styles/layout.cssui/src/ui/app-render.helpers.tsui/src/ui/app-render.tsui/src/ui/app-view-state.tsui/src/ui/app.tsui/src/ui/views/chat.browser.test.tsui/src/ui/views/chat.test.tsui/src/ui/views/chat.tsTotals:
8595226821Review 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)NoFailure Recovery (if this breaks)
9854527612.Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.