fix(chat): improve delete all chats button#1106
Merged
benoitf merged 2 commits intokortex-hub:mainfrom Mar 18, 2026
Merged
Conversation
fbricon
requested review from
benoitf and
bmahabirbu
and removed request for
a team
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClear current chat selection and navigate to root after deleting all chats; make the chat list scrollable and move the "Delete all chats" control into a fixed bottom group; update Playwright smoke test to validate deletion behaviors and post-delete UI state. Changes
Sequence DiagramsequenceDiagram
participant User
participant Sidebar
participant State
participant Router
User->>Sidebar: Click "Delete all chats"
Sidebar->>State: deleteAllChats()
State-->>Sidebar: chats cleared
Sidebar->>State: clear currentChatId.value
Sidebar->>Router: navigate("/")
Router-->>Sidebar: navigation complete
Sidebar-->>User: UI shows empty history / suggested messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/renderer/src/lib/chat/components/sidebar-history/history.svelte (1)
98-115:⚠️ Potential issue | 🟠 MajorGate the reset/navigation on a successful bulk delete.
currentChatId.value = undefinedandrouter.goto('/')run as soon as the delete starts, so a failedwindow.inferenceDeleteAllChats()still kicks the user back to the empty view. Also,packages/renderer/src/lib/chat/route/CustomChat.svelte:6-8falls back tocurrentChatId.value, so keeping the reset behindif (chatId)can still leave a deleted chat selected when the route param is absent. Move both side effects into the success path and clear the store unconditionally there.💡 Possible fix
async function handleDeleteAllChats(): Promise<void> { const deleteAllPromise = window.inferenceDeleteAllChats(); + const hadActiveChat = Boolean(chatId ?? currentChatId.value); toast.promise(deleteAllPromise, { loading: 'Deleting all chats...', success: () => { + currentChatId.value = undefined; + if (hadActiveChat) { + router.goto('/'); + } chatHistory.refetch().catch((err: unknown) => { console.error('Failed to refetch chat history', err); }); return 'All chats deleted successfully'; }, error: 'Failed to delete all chats', }); - - if (chatId) { - currentChatId.value = undefined; - router.goto('/'); - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/src/lib/chat/components/sidebar-history/history.svelte` around lines 98 - 115, The delete handler currently clears currentChatId and navigates away immediately; change handleDeleteAllChats so the side effects run only on successful completion of window.inferenceDeleteAllChats(): move currentChatId.value = undefined and router.goto('/') into the toast.promise success branch (after chatHistory.refetch), and in that success block clear the store unconditionally (do not gate on the original chatId) so CustomChat.svelte's fallback won't keep a deleted chat selected; leave no navigation or store mutation on error and keep the existing chatHistory.refetch error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts`:
- Around line 145-150: The TODO and workaround disagree: you only strip colons
from selectedModelName but then still include a literal "model:" in testMessage,
which defeats the workaround; update the code so the final test message contains
no colon by either removing the literal "model:" label or removing/escaping
colons when composing testMessage (modify the usage around
chatPage.getSelectedModelName and the selectedModelName variable and the
testMessage construction) and tighten the TODO comment to state whether the bug
is "any colon in the message breaks history" or "only colons inside model names
break history" so future readers know why colons are being stripped.
---
Outside diff comments:
In `@packages/renderer/src/lib/chat/components/sidebar-history/history.svelte`:
- Around line 98-115: The delete handler currently clears currentChatId and
navigates away immediately; change handleDeleteAllChats so the side effects run
only on successful completion of window.inferenceDeleteAllChats(): move
currentChatId.value = undefined and router.goto('/') into the toast.promise
success branch (after chatHistory.refetch), and in that success block clear the
store unconditionally (do not gate on the original chatId) so
CustomChat.svelte's fallback won't keep a deleted chat selected; leave no
navigation or store mutation on error and keep the existing chatHistory.refetch
error handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59088660-22fb-4b0a-95b4-bb655807b759
📒 Files selected for processing (2)
packages/renderer/src/lib/chat/components/sidebar-history/history.sveltetests/playwright/src/specs/provider-specs/chat-smoke.spec.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/renderer/src/lib/chat/components/sidebar-history/history.svelte`:
- Around line 112-115: The current code clears currentChatId.value and calls
router.goto('/') immediately before waiting for inferenceDeleteAllChats() to
complete, so move the state reset and navigation into the success path after
inferenceDeleteAllChats() resolves; specifically, await or .then the
inferenceDeleteAllChats() call and only set currentChatId.value = undefined and
call router.goto('/') on success, while adding a catch to handle failures (and
avoid clearing selection or navigating on error) so the local selection remains
if deletion fails.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9b408c9-ad38-4852-b840-a33f0af9db0b
📒 Files selected for processing (2)
packages/renderer/src/lib/chat/components/sidebar-history/history.sveltetests/playwright/src/specs/provider-specs/chat-smoke.spec.ts
Comment on lines
+112
to
+115
| if (chatId) { | ||
| currentChatId.value = undefined; | ||
| router.goto('/'); | ||
| } |
There was a problem hiding this comment.
Move route/state reset to delete-all success path
At Line 112, currentChatId is cleared and navigation happens before inferenceDeleteAllChats() resolves. If deletion fails, users are still redirected and local selection is lost.
💡 Suggested fix
toast.promise(deleteAllPromise, {
loading: 'Deleting all chats...',
success: () => {
chatHistory.refetch().catch((err: unknown) => {
console.error('Failed to refetch chat history', err);
});
+ if (chatId) {
+ currentChatId.value = undefined;
+ router.goto('/');
+ }
return 'All chats deleted successfully';
},
error: 'Failed to delete all chats',
});
-
- if (chatId) {
- currentChatId.value = undefined;
- router.goto('/');
- }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/renderer/src/lib/chat/components/sidebar-history/history.svelte`
around lines 112 - 115, The current code clears currentChatId.value and calls
router.goto('/') immediately before waiting for inferenceDeleteAllChats() to
complete, so move the state reset and navigation into the success path after
inferenceDeleteAllChats() resolves; specifically, await or .then the
inferenceDeleteAllChats() call and only set currentChatId.value = undefined and
call router.goto('/') on success, while adding a catch to handle failures (and
avoid clearing selection or navigating on error) so the local selection remains
if deletion fails.
serbangeorge-m
requested changes
Mar 17, 2026
| await chatPage.verifyStopButtonHidden(); | ||
| }); | ||
|
|
||
| test('[CHAT-10] Delete all button remains visible without scrolling', async ({ chatPage }) => { |
Contributor
There was a problem hiding this comment.
Also this test did not run as part of pr checks. The provider spec are running on multiple configurations and should be validated in nightly run workflow. I suggest splitting the PRs: one for the core changes and another for the e2e tests
Contributor
Author
There was a problem hiding this comment.
#1113 contains the e2e tests for the new feature.
This PR keeps the e2e test changes for the fix(chat): reset chat view when deleting all chats commit
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts (1)
147-150:⚠️ Potential issue | 🟡 MinorColon workaround is still self-contradictory in the final test message.
You sanitize colons from
selectedModelName, but Line 150 still inserts a literal:(model:), which can still hit the known history-rendering bug.Suggested fix
- const testMessage = `Test message for model: "${selectedModelName}"`; + const testMessage = `Test message for model "${selectedModelName}"`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts` around lines 147 - 150, The test still includes a literal colon in the constructed string which defeats the earlier sanitization of selectedModelName; update how testMessage is built so it cannot contain a ":" by either removing the hard-coded colon or ensuring the entire message is sanitized (e.g., build testMessage without the ":" token or run selectedModelName through the same replace before interpolation). Modify the code that sets testMessage (the testMessage variable) to reference the sanitized selectedModelName and avoid inserting any additional ":" characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/playwright/src/specs/provider-specs/chat-smoke.spec.ts`:
- Around line 147-150: The test still includes a literal colon in the
constructed string which defeats the earlier sanitization of selectedModelName;
update how testMessage is built so it cannot contain a ":" by either removing
the hard-coded colon or ensuring the entire message is sanitized (e.g., build
testMessage without the ":" token or run selectedModelName through the same
replace before interpolation). Modify the code that sets testMessage (the
testMessage variable) to reference the sanitized selectedModelName and avoid
inserting any additional ":" characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f023d9ca-e1a2-462e-b0e0-5b92d498b6c5
📒 Files selected for processing (2)
packages/renderer/src/lib/chat/components/sidebar-history/history.sveltetests/playwright/src/specs/provider-specs/chat-smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/renderer/src/lib/chat/components/sidebar-history/history.svelte
When clicking "delete all chats" while viewing a conversation, the chat view now properly resets to a fresh conversation state instead of showing the deleted chat. This is achieved by resetting currentChatId and navigating to the home route. Enhanced e2e test to verify this behavior. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
The delete all chats button now only appears when there are chats and remains visible at the bottom without requiring scrolling. Restructured the sidebar layout to make the chat list scrollable while keeping the button sticky at the bottom. Fixes kortex-hub#477 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Fred Bricon <fbricon@gmail.com>
serbangeorge-m
approved these changes
Mar 18, 2026
benoitf
approved these changes
Mar 18, 2026
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
3 participants
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.
delete-all-chats-always-on.mp4
Fixes #477