fix(ui): ensure session deletion properly clears backend and cache#12043
fix(ui): ensure session deletion properly clears backend and cache#12043keval718 wants to merge 7 commits intorelease-1.8.0from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-1.8.0 #12043 +/- ##
================================================
Coverage ? 37.16%
================================================
Files ? 1592
Lines ? 78102
Branches ? 11842
================================================
Hits ? 29023
Misses ? 47425
Partials ? 1654
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Cristhianzl
left a comment
There was a problem hiding this comment.
🟠 NEEDS CHANGES
Must fix (blockers):
- DRY violation — Extract session filtering logic into a shared
isMessageForSession(msg, flowId, session)helper used by both theuseEffectinitializer and theuseMemodisplay filter anytypes — Replace 6+anyusages inuse-delete-sessions.tswith proper types. The original code useduseMutationFunctionType— either keep using it or defineDeleteSessionResponseandErrortypes- Unnecessary cast — Remove
as unknown as DeleteSessionParams—variablesis already typed correctly
Should fix:
4. Tests — Add regression tests for the data leakage fix, at minimum covering: (a) delete removes cache, (b) new session after delete starts clean, (c) session filtering correctness
5. sessionStorage double-write — Either remove the explicit sessionStorage cleanup in removeLocalSession (since the useEffect on line 67-77 already handles it), or add a comment explaining why both are needed (race condition?)
Nice to have:
6. Simplify ExtendedMessageProperties — the base Message type has [key: string]: unknown so the extension adds no type safety
7. Change comments from WHAT to WHY
8. Consider centralizing query key factories to avoid tight coupling in predicate-based cache removal
Fixed. Thanks |
3cb08a6 to
617d634
Compare
Problem
In release-1.8.0, the session deletion logic was failing to properly purge data from both the UI and backend, manifesting in two critical ways:
Context Persistence: Deleting the "Default Session" cleared the chat UI, but the LLM retained memory of previous messages
Data Leakage/Reuse: Deleting a numbered session (e.g., "New Session 0") and creating a new one caused the system to reuse the old session's data, with messages from the deleted session immediately reappearing
Root Cause
The system was performing "shallow deletions" that only affected the sidebar UI. The backend deletion API was never called for numbered sessions due to a conditional check that required the session to be in the dbSessions list first.
Testing
✅ Verified numbered sessions now call backend API on deletion
✅ Confirmed Message Logs shows empty state after deletion
✅ Tested session recreation doesn't reuse old data
✅ Verified no context persistence after deletion