fix: DELETE /chats/<id> report actual delete result - #4516
fix: DELETE /chats/<id> report actual delete result#4516praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Greptile SummaryThe PR makes conversation deletion reflect the actual persistence result and keeps the desktop UI consistent with storage.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/praisonai-desktop/engine/server.py | DELETE now distinguishes invalid identifiers and filesystem failures from successful removal. |
| src/praisonai-desktop/ui/index.html | Sidebar and clear-all deletion paths update visible state only according to successful DELETE responses, resolving the previously reported partial-clear inconsistency. |
| src/praisonai-desktop/engine/test_train_routes.py | Adds route-level coverage for successful deletion, unlink failure, and malformed identifiers. |
| src/praisonai-desktop/frontend/tests/buttons.test.mjs | Adds UI regression coverage for failed and successful single deletion and partial clear-all behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User requests conversation deletion] --> B[DELETE /chats/id]
B --> C{Path valid?}
C -- No --> D[Return 400]
C -- Yes --> E{Unlink succeeds?}
E -- No --> F[Return 500]
E -- Yes --> G[Return 200]
D --> H[UI preserves transcript and list state]
F --> H
G --> I[UI clears deleted active transcript]
I --> J[Refresh conversation list]
Reviews (4): Last reviewed commit: "fix: clear-all follows storage when a de..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
MervinPraison
added
pipeline/blocked:ci
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #4516Verdict: Approve ✅ (one valid P1 fixed and pushed to this branch) Phase 1 — Architecture (per AGENTS.md)This PR is entirely within
Phase 2 — FixesGreptile P1 (valid) — partial bulk-clear left a stale transcript. In Fixed in if(!r||!r.ok) failed++;
else if(c.id===chatId) activeGone=true;
...
if(activeGone){ chatId=rid(); turns.innerHTML=''; syncEmpty(); }
if(failed){ toast(...); refreshChats(); closeOverlay(); return; }
if(!activeGone){ chatId=rid(); turns.innerHTML=''; syncEmpty(); }Added a regression test ( The sidebar per-chat delete ( Files modified on this branch
Skipped
Tests
|
praisonai-triage-agent
Bot
added
pipeline/merge-ready
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebase PR #4516 onto latest
|
2132fe4 to
e1173df
Compare
MervinPraison
added
pipeline/awaiting-merge-gate
praisonai-triage-agent
Bot
removed
pipeline/awaiting-merge-gate
praisonai-triage-agent
Bot
added
pipeline/merge-ready
|
@claude this PR has merge conflicts with |
|
Claude finished @MervinPraison's task —— View job Rebase PR #4516 onto latest
|
The engine answered {"ok": true} whether or not the chat was removed,
and both UI delete paths blanked the transcript regardless. A read-only
or synced data dir left the conversation closed on screen and back in
the sidebar on the next refresh.
- engine/server.py: do_DELETE returns 400 on an invalid id and 500 on
an OSError instead of swallowing both.
- ui/index.html: the sidebar delete and runAction('clear') now check the
response before blanking the transcript, matching the per-message
delete already in place.
- tests: engine DeleteChats route tests and two buttons.test.mjs cases.
Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
The bulk "Delete all conversations" path returned early on any failure without checking whether the active conversation was among the deletes that succeeded, leaving its transcript on screen while gone on disk. Track the active id and blank the transcript when it was deleted, even if a different conversation's delete failed. Adds a regression test. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
e1173df to
3b0101d
Compare
MervinPraison
added
pipeline/blocked:ci
1 participant
Fixes #4495
Problem
DELETE /chats/<id>answered{"ok": true}whether or not anything was deleted, and both UI delete paths blanked the transcript regardless. On a read-only or permission-changed data dir, or a synced folder mid-conflict, the conversation visibly closed and then reappeared in the sidebar on the next refresh or restart. A malformed id (aValueErrorfrom_chat_path) was swallowed the same way.Fix
engine/server.py—do_DELETEnow returns400on an invalid id and500on anOSError, instead of swallowing both and always reporting success. The comment mirrors the per-message delete already in the codebase.ui/index.html— the sidebar delete (.x) andrunAction('clear')now check the response before blanking the transcript / clearing the list, and surface a toast on failure.engine/test_train_routes.py(DeleteChats): a successful delete removes the chat; a delete that cannot happen (a directory blocking the unlink — deterministic even as root) is not reported as 200 and the chat stays listed; an empty id (../..after the route is stripped) is refused with 400.frontend/tests/buttons.test.mjs: a stubbed500DELETE leaves#turnschildren intact; a200DELETE clears them.Testing
python -m unittest test_train_routes.DeleteChats— 3 passednode --test frontend/tests/buttons.test.mjs— 19 passedGenerated with Claude Code