fix(sidebar): deleting the open conversation lands on welcome with a fresh session#161
Merged
Conversation
…fresh session Deleting the currently-open conversation now transitions to the welcome page backed by a brand-new session, instead of clearing the UI while the backend stays stranded on the deleted task's engine. Frontend (Sidebar.tsx deleteItem): - After a successful delete of the active row, dispatch startNewChat(): it provisions a consistent new engine/task (and reclaims the stale one — its recorder was reset on delete). Merely clearing the UI left the deleted task's engine in the foreground, so the next message ran on it and every event was stamped with the deleted task id — dropped by the WS client filter, making the new conversation appear dead. - Remove the row via a new removeSession reducer that filters the LATEST state inside the reducer: the old render-scope whole-list filter could clobber updates that landed during the delete round-trip, and the setTasks/setSessions lazy-index guard resurrected the deleted row while it was still the current session. - Drop the active session's type-ahead queue BEFORE the delete round-trip: the cancelled run's agent_done reaches the client before the DELETE response and would otherwise drain the queue back into the deleted session (resurrecting its file + index entry on disk). - Re-check the current session after the await (via a ref): if the user navigated to another session while the DELETE was in flight, their navigation wins and the delete no longer yanks the foreground. Backend (sessions.go handleDeleteSession): - When the deleted active task's run hasn't drained within the bounded wait, no longer close the recorder under a live writer (which let the run resurrect the just-deleted JSONL + index entry). Defer the close until the run finishes, then re-run the deletion to sweep up anything the tail wrote — same discipline as automation-run teardown. Verified end-to-end in the browser: open a conversation → delete from the ⋯ menu → welcome page shows, the row is gone from the sidebar, the backend's active session is a fresh UUID, and the project's persisted last-activity time/order are unaffected.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSession deletion now refuses requests for running agents with HTTP 409. The frontend disables deletion while running, handles asynchronous navigation and API failures, removes session/task state directly, drops related queues, and conditionally starts a new chat. ChangesSession deletion safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (2)
web/src/components/Sidebar.tsx (1)
384-414: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDrop the session queue unconditionally before the API call.
Currently, the queue is dropped before the API request for active sessions, but after the request for inactive sessions. If a user deletes an inactive session that is still running in the background, its
agent_donecould arrive during thedeleteSessionround-trip and drain its queue, resurrecting the deleted session on disk (the same race condition avoided for the active session).
Dropping the queue unconditionally before the API call prevents this background race and removes duplicate code.♻️ Proposed refactor
async function deleteItem(row: SessionRow) { const wasActive = row.uuid === currentSessionId - if (wasActive) { - // Drop queued type-ahead for this session BEFORE the delete round-trip: - // the cancelled run's agent_done reaches the client before the DELETE - // response, and would otherwise drain the queue back into the deleted - // session — resurrecting its file + index entry on disk. - dispatch(chatActions.dropSessionQueue(row.uuid)) - } + // Drop queued type-ahead for this session BEFORE the delete round-trip: + // the cancelled run's agent_done reaches the client before the DELETE + // response, and would otherwise drain the queue back into the deleted + // session — resurrecting its file + index entry on disk. + dispatch(chatActions.dropSessionQueue(row.uuid)) try { await api.deleteSession(row.uuid) } catch { return } // Filter inside the reducer: the render-scope sessions/tasks copies are // stale after the await (a WS update or refresh may have landed since), // and a whole-list setTasks would clobber it. dispatch(sessionActions.removeSession(row.uuid)) - if (!wasActive) { - dispatch(chatActions.dropSessionQueue(row.uuid)) - return - } + if (!wasActive) return // The user may have navigated to another session while the DELETE was in // flight (their click's loadSession wins) — only take over the foreground // if we're still on the deleted session. if (currentSessionRef.current !== row.uuid) return // Land on the welcome page with a FRESH session. Merely clearing the UI // would strand the backend on the deleted task's engine: the next // message would run on that stale engine, whose events are stamped with // the deleted task id and dropped by the WS filter (a conversation that // appears dead). startNewChat provisions a consistent new engine/task — // and reclaims the stale one (its recorder was reset on delete). await dispatch(startNewChat()) }🤖 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/Sidebar.tsx` around lines 384 - 414, Move dispatch(chatActions.dropSessionQueue(row.uuid)) out of the wasActive conditional so it always executes before api.deleteSession(row.uuid). Remove the later conditional dropSessionQueue call for inactive sessions, while preserving the existing removeSession dispatch and active-session navigation logic.internal/web/sessions.go (1)
237-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate recorder close+reset logic between the drained branch and the deferred goroutine.
The
eng.emu.Lock(); if eng.recorder != nil && eng.recorder.UUID() == id { ... }; eng.emu.Unlock()block is identical in both places. Given the flagged complexity of this hunk, extracting it into a small helper reduces the chance the two copies drift (e.g., if the timeout-guard from the previous comment is added to only one of them).♻️ Proposed refactor: shared helper
+// closeSessionRecorder closes and detaches eng's recorder if it still belongs +// to the given session id, under eng.emu. +func closeSessionRecorder(eng *Engine, id string) { + eng.emu.Lock() + defer eng.emu.Unlock() + if eng.recorder != nil && eng.recorder.UUID() == id { + eng.recorder.Close() + eng.recorder = nil + eng.history = nil + } +}Then call
closeSessionRecorder(eng, id)in both the drained branch and the goroutine instead of repeating the lock/check/unlock sequence.Also applies to: 256-262
🤖 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/web/sessions.go` around lines 237 - 244, Extract the duplicated recorder lock, UUID check, close, and reset sequence into a shared closeSessionRecorder helper. Replace the inline logic in both the drained branch and deferred goroutine with closeSessionRecorder(eng, id), preserving the existing matching-recorder behavior and synchronization.
🤖 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/web/sessions.go`:
- Around line 245-264: The deferred cleanup goroutine must only close the
recorder after the run has drained. After its 30-second polling loop around
eng.running, check whether eng.running is still true; if so, log the timeout
with config.Logger().Printf(...) and return without closing the recorder or
deleting the session. Preserve the existing recorder cleanup and session
deletion only for runs that have finished.
In `@web/src/components/Sidebar.tsx`:
- Around line 95-99: Update the currentSessionRef synchronization in Sidebar so
currentSessionRef.current is assigned inside a useEffect keyed by
currentSessionId, rather than during render. Preserve deleteItem’s ability to
read the latest committed session ID after awaited work.
---
Nitpick comments:
In `@internal/web/sessions.go`:
- Around line 237-244: Extract the duplicated recorder lock, UUID check, close,
and reset sequence into a shared closeSessionRecorder helper. Replace the inline
logic in both the drained branch and deferred goroutine with
closeSessionRecorder(eng, id), preserving the existing matching-recorder
behavior and synchronization.
In `@web/src/components/Sidebar.tsx`:
- Around line 384-414: Move dispatch(chatActions.dropSessionQueue(row.uuid)) out
of the wasActive conditional so it always executes before
api.deleteSession(row.uuid). Remove the later conditional dropSessionQueue call
for inactive sessions, while preserving the existing removeSession dispatch and
active-session navigation logic.
🪄 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: 958f5d56-3307-4efd-ae50-506e876defbe
📒 Files selected for processing (3)
internal/web/sessions.goweb/src/app/store.tsweb/src/components/Sidebar.tsx
Comment on lines
+245
to
+264
| } else { | ||
| // The run is still draining (a ctx-ignoring tool, a slow LLM | ||
| // tail). Closing the recorder now would let the live writer | ||
| // resurrect the session file this handler is about to delete. | ||
| // Defer the close until the run actually finishes, then re-run | ||
| // the deletion to sweep up anything the tail wrote in between | ||
| // (same discipline as the automation-run teardown). | ||
| go func() { | ||
| for i := 0; i < 6000 && eng.running.Load(); i++ { | ||
| time.Sleep(5 * time.Millisecond) | ||
| } | ||
| eng.emu.Lock() | ||
| if eng.recorder != nil && eng.recorder.UUID() == id { | ||
| eng.recorder.Close() | ||
| eng.recorder = nil | ||
| eng.history = nil | ||
| } | ||
| eng.emu.Unlock() | ||
| _, _ = session.DeleteSessionByUUID(id) | ||
| }() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify config.Logger()'s method surface (Warn/Error/etc.) to align the suggested calls with the actual API.
ast-grep outline internal/config --items all --type function --match 'Logger'
rg -nP 'func .*Logger\(\)' internal/config
rg -nP 'config\.Logger\(\)\.\w+\(' internal -C2 | head -40Repository: cnjack/jcode
Length of output: 2241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of internal/web/sessions.go with line numbers.
sed -n '220,290p' internal/web/sessions.go | cat -n
# Inspect DeleteSessionByUUID implementation and any related session lifecycle helpers.
rg -n "func DeleteSessionByUUID|DeleteSessionByUUID\\(" internal -C 3
# Inspect the logger implementation to confirm the supported call surface.
sed -n '1,120p' internal/config/log.go | cat -nRepository: cnjack/jcode
Length of output: 7450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect session deletion and recorder behavior to determine whether the timeout path is still unsafe.
rg -n "type Recorder|func \\(.*Recorder.*\\) Close|func \\(.*Recorder.*\\) UUID|DeleteSessionByUUID|resurrect|running.Load\\(" internal -C 3
# Read the delete helper and recorder close implementation if found.
for f in $(rg -l "DeleteSessionByUUID|type Recorder|func \\(.*Recorder.*\\) Close" internal); do
echo "### $f"
sed -n '1,260p' "$f" | cat -n
doneRepository: cnjack/jcode
Length of output: 44236
Only close the recorder once the run has actually drained.
- After the 30s wait, re-check
eng.running; if it’s still true, bail out. Closingeng.recorderhere can still let the live writer recreate/truncate the session file. - Log the timeout via
config.Logger().Printf(...)so the fallback cleanup isn’t silent.
🤖 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/web/sessions.go` around lines 245 - 264, The deferred cleanup
goroutine must only close the recorder after the run has drained. After its
30-second polling loop around eng.running, check whether eng.running is still
true; if so, log the timeout with config.Logger().Printf(...) and return without
closing the recorder or deleting the session. Preserve the existing recorder
cleanup and session deletion only for runs that have finished.
Source: Coding guidelines
Comment on lines
+95
to
+99
| // Latest currentSessionId readable from async handlers (deleteItem) after an | ||
| // await, when the render-scope value is stale: if the user navigated away | ||
| // while the DELETE was in flight, the delete must not yank the foreground. | ||
| const currentSessionRef = useRef(currentSessionId) | ||
| currentSessionRef.current = currentSessionId |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid mutating refs during render.
Mutating ref.current during rendering violates React's pure-render rules and can lead to incorrect state in Concurrent Mode if a render is interrupted or discarded. Move the mutation into a useEffect to ensure it only updates after a successful commit.
💡 Proposed fix to mutate ref in an effect
const currentSessionRef = useRef(currentSessionId)
- currentSessionRef.current = currentSessionId
+ useEffect(() => {
+ currentSessionRef.current = currentSessionId
+ }, [currentSessionId])📝 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
| // Latest currentSessionId readable from async handlers (deleteItem) after an | |
| // await, when the render-scope value is stale: if the user navigated away | |
| // while the DELETE was in flight, the delete must not yank the foreground. | |
| const currentSessionRef = useRef(currentSessionId) | |
| currentSessionRef.current = currentSessionId | |
| // Latest currentSessionId readable from async handlers (deleteItem) after an | |
| // await, when the render-scope value is stale: if the user navigated away | |
| // while the DELETE was in flight, the delete must not yank the foreground. | |
| const currentSessionRef = useRef(currentSessionId) | |
| useEffect(() => { | |
| currentSessionRef.current = currentSessionId | |
| }, [currentSessionId]) |
🤖 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/Sidebar.tsx` around lines 95 - 99, Update the
currentSessionRef synchronization in Sidebar so currentSessionRef.current is
assigned inside a useEffect keyed by currentSessionId, rather than during
render. Preserve deleteItem’s ability to read the latest committed session ID
after awaited work.
Refuse DELETE with 409 when the task engine is mid-run, disable the sidebar Delete action for running rows, and drop the cancel-and-drain path that was only needed for delete-while-running.
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.
需求
允许删除当前已经打开的 conversation，删除之后跳转到 welcome 页面。
问题
之前删除当前打开的会话只是清空 UI（clearChat + setCurrentSession('')），后端仍停留在被删任务的 engine 上：
修复
前端（Sidebar.tsx deleteItem）
startNewChat()：创建一致的新 engine/task（旧 engine 因 recorder 已被重置而被回收），UI 落到 welcome 页removeSessionreducer：在 reducer 内对最新 state 过滤（避免 await 期间闭包过期覆盖列表更新；且不会触发 current-session 保护逻辑）后端（sessions.go handleDeleteSession）
验证
golangci-lint0 issues；make lint-web通过；go test ./internal/web/ ./internal/session/通过对抗评审
已经过一轮对抗评审，修复了：
已知边界（可接受/后续跟进）：
api.newSession()失败（如 64 engine 上限）时回退到 welcome，此时发消息仍可能命中旧 engine；属极罕见错误路径，刷新页面即可恢复currentSessionId=''窗口期 WS 过滤放宽，与现有 startNewChat 流程行为一致（非本 PR 引入）Summary by CodeRabbit