🔖 fix: Decrement Bookmark Counts When Deleting Conversations (#13830)

* 🔖 fix: Decrement Bookmark Counts When Deleting Conversations

Deleting a bookmarked/tagged conversation removed the conversation but never decremented the affected ConversationTag counts, leaving stale bookmark counts in the UI.

- Add decrementTagCounts helper that atomically decrements tag counts (clamped at 0, deduped per conversation) in deleteConvos, covering single delete, clear-all, and account deletion.
- Invalidate the conversationTags query in the single-delete and clear-all client mutations so counts refetch.
- Add deleteConvos tag-count tests.

* 🔒 fix: Guard tag-count decrement on actual deletion and message-failure

Addresses Codex review findings:
- Guard the decrement on deleteConvoResult.deletedCount > 0 so a losing concurrent delete (double-click/two-tab) does not decrement counts for a conversation it did not actually remove.
- Move the count adjustment to run immediately after the conversation deletion, before message cleanup, so a deleteMessages failure cannot leave bookmark counts permanently stale.
- Add regression tests for both cases.

* 🔀 fix: Refresh project stats after message cleanup in deleteConvos

Addresses Codex finding: bundling refreshChatProjectStatsForUser into a Promise.all before deleteMessages let a stats-refresh error abort the function and orphan the deleted conversations' messages. Split the steps so the (best-effort) tag-count decrement still runs before message cleanup (counts reconciled even if messages fail), while project-stats refresh runs after, matching the original ordering.

*  test: Add e2e coverage for bookmark counts on conversation delete

Two mock-harness specs for the deleteConvos bookmark-count behavior:
- Deleting the only conversation carrying a bookmark drops its count to 0.
- Deleting one of two conversations that share a bookmark leaves the count at 1.
Both assert the persisted server count via GET /api/tags after the real delete round-trip.

* chore: import order
This commit is contained in:
Danny Avila 2026-06-18 08:37:08 -04:00 committed by GitHub
parent a6b5343220
commit 58647bc08b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 399 additions and 7 deletions

View file

@ -572,6 +572,7 @@ export const useDeleteConversationMutation = (
});
queryClient.invalidateQueries([QueryKeys.projectConversations]);
queryClient.invalidateQueries([QueryKeys.projects]);
queryClient.invalidateQueries([QueryKeys.conversationTags]);
if (deletedProjectId) {
queryClient.invalidateQueries([QueryKeys.project, deletedProjectId]);
}

View file

@ -0,0 +1,145 @@
import { expect, test } from '@playwright/test';
import type { Locator, Page } from '@playwright/test';
import {
selectMockEndpoint,
getAccessToken,
MOCK_ENDPOINTS,
NEW_CHAT_PATH,
sendMessage,
fetchJson,
} from './helpers';
type TagCount = { tag: string; count: number };
const uniqueName = (prefix: string) =>
`${prefix}-${Date.now().toString(36)}-${Math.floor(Math.random() * 1e4)}`;
const firstConversation = (page: Page) => page.getByTestId('convo-item').first();
/** The header bookmark menu (the nav filter button shares the `bookmark-menu` testid). */
const headerBookmarkButton = (page: Page) => page.locator('#bookmark-menu-button');
/** Start a mock chat and send a message so the conversation persists at `/c/:id`. */
async function startBookmarkableChat(page: Page): Promise<void> {
await page.goto(NEW_CHAT_PATH, { timeout: 10000 });
await selectMockEndpoint(page, MOCK_ENDPOINTS[0]);
const response = await sendMessage(page, 'hello bookmarks');
expect(response.ok()).toBeTruthy();
await expect(page).toHaveURL(/\/c\/(?!new)/, { timeout: 15000 });
await expect(headerBookmarkButton(page)).toBeVisible({ timeout: 15000 });
}
/** Create a brand-new bookmark and attach it to the active conversation (count -> 1). */
async function createBookmarkForActiveChat(page: Page, tag: string): Promise<void> {
await headerBookmarkButton(page).click();
await page.getByRole('menuitem', { name: 'New Bookmark' }).click();
const dialog = page.getByRole('dialog');
await dialog.getByRole('textbox', { name: 'Title' }).fill(tag);
const [response] = await Promise.all([
page.waitForResponse(
(r) => r.request().method() === 'POST' && new URL(r.url()).pathname === '/api/tags',
{ timeout: 15000 },
),
dialog.getByRole('button', { name: 'Save' }).click(),
]);
expect(response.ok()).toBeTruthy();
await expect(dialog).toBeHidden();
await page.keyboard.press('Escape');
}
/** Attach an already-existing bookmark to the active conversation (count += 1). */
async function addExistingBookmarkToActiveChat(page: Page, tag: string): Promise<void> {
await headerBookmarkButton(page).click();
// Existing-tag rows render as `menuitemcheckbox` (they carry aria-checked).
const tagItem = page.getByRole('menuitemcheckbox', { name: tag, exact: true });
await expect(tagItem).toBeVisible({ timeout: 10000 });
const [response] = await Promise.all([
page.waitForResponse(
(r) =>
r.request().method() === 'PUT' && new URL(r.url()).pathname.startsWith('/api/tags/convo/'),
{ timeout: 15000 },
),
tagItem.click(),
]);
expect(response.ok()).toBeTruthy();
await page.keyboard.press('Escape');
}
/** Read the persisted bookmark count from the server (0 when the tag is absent). */
async function getTagCount(page: Page, tag: string): Promise<number> {
const token = await getAccessToken(page);
const tags = await fetchJson<TagCount[]>(page, '/api/tags', token);
return tags.find((t) => t.tag === tag)?.count ?? 0;
}
async function renameConversation(page: Page, conversation: Locator, title: string): Promise<void> {
await conversation.hover();
await conversation.getByRole('button', { name: 'Conversation Menu Options' }).click();
await page.getByRole('menuitem', { name: 'Rename' }).click();
const titleInput = conversation.getByRole('textbox', { name: 'New Conversation Title' });
await expect(titleInput).toBeVisible();
await titleInput.fill(title);
await conversation.getByRole('button', { name: 'Save' }).click();
await expect(conversation).toContainText(title);
}
async function deleteConversation(page: Page, conversation: Locator): Promise<void> {
await conversation.hover();
await conversation.getByRole('button', { name: 'Conversation Menu Options' }).click();
await page.getByRole('menuitem', { name: 'Delete' }).click();
const dialog = page.getByRole('dialog', { name: 'Delete chat?' });
await expect(dialog).toBeVisible();
const [response] = await Promise.all([
page.waitForResponse(
(r) => r.request().method() === 'DELETE' && r.url().includes('/api/convos'),
{ timeout: 30000 },
),
dialog.getByRole('button', { name: 'Delete' }).click(),
]);
expect(response.ok()).toBeTruthy();
}
test.describe('bookmark counts', () => {
test.describe.configure({ timeout: 120_000 });
test('drops a bookmark count to zero when its only conversation is deleted', async ({ page }) => {
const tag = uniqueName('E2E Bookmark');
await startBookmarkableChat(page);
await createBookmarkForActiveChat(page, tag);
expect(await getTagCount(page, tag)).toBe(1);
await deleteConversation(page, firstConversation(page));
await expect.poll(() => getTagCount(page, tag), { timeout: 15000 }).toBe(0);
});
test('keeps a shared bookmark at one when another conversation still uses it', async ({
page,
}) => {
const tag = uniqueName('E2E Shared');
// Conversation A: create the bookmark (count -> 1).
await startBookmarkableChat(page);
await createBookmarkForActiveChat(page, tag);
const titleA = uniqueName('Bookmark A');
await renameConversation(page, firstConversation(page), titleA);
expect(await getTagCount(page, tag)).toBe(1);
// Conversation B: attach the same bookmark (count -> 2).
await startBookmarkableChat(page);
await addExistingBookmarkToActiveChat(page, tag);
const titleB = uniqueName('Bookmark B');
await renameConversation(page, firstConversation(page), titleB);
expect(await getTagCount(page, tag)).toBe(2);
// Deleting A leaves the count at 1 because B still carries the bookmark.
const conversationA = page.getByTestId('convo-item').filter({ hasText: titleA });
await deleteConversation(page, conversationA);
await expect.poll(() => getTagCount(page, tag), { timeout: 15000 }).toBe(1);
await expect(page.getByTestId('convo-item').filter({ hasText: titleB })).toBeVisible();
});
});

View file

@ -133,6 +133,7 @@ export const useClearConversationsMutation = (): UseMutationResult<unknown> => {
return useMutation(() => dataService.clearAllConversations(), {
onSuccess: () => {
queryClient.invalidateQueries([QueryKeys.allConversations]);
queryClient.invalidateQueries([QueryKeys.conversationTags]);
},
});
};

View file

@ -1,8 +1,8 @@
import mongoose from 'mongoose';
import { v4 as uuidv4 } from 'uuid';
import { MongoMemoryServer } from 'mongodb-memory-server';
import { EModelEndpoint, RetentionMode } from 'librechat-data-provider';
import type { IChatProject, IConversation } from '../types';
import { MongoMemoryServer } from 'mongodb-memory-server';
import { ConversationMethods, createConversationMethods } from './conversation';
import { tenantStorage, runAsSystem } from '~/config/tenantContext';
import { createModels } from '../models';
@ -17,6 +17,12 @@ jest.mock('~/config/winston', () => ({
let mongoServer: InstanceType<typeof MongoMemoryServer>;
let Conversation: mongoose.Model<IConversation>;
let ChatProject: mongoose.Model<IChatProject>;
let ConversationTag: mongoose.Model<{
user: string;
tag: string;
count: number;
position: number;
}>;
let modelsToCleanup: string[] = [];
// Mock message methods (same as original test mocking ./Message)
@ -34,6 +40,12 @@ beforeAll(async () => {
Object.assign(mongoose.models, models);
Conversation = mongoose.models.Conversation as mongoose.Model<IConversation>;
ChatProject = mongoose.models.ChatProject as mongoose.Model<IChatProject>;
ConversationTag = mongoose.models.ConversationTag as mongoose.Model<{
user: string;
tag: string;
count: number;
position: number;
}>;
methods = createConversationMethods(mongoose, { getMessages, deleteMessages });
@ -94,6 +106,7 @@ describe('Conversation Operations', () => {
// Clear database
await Conversation.deleteMany({});
await ChatProject.deleteMany({});
await ConversationTag.deleteMany({});
// Reset mocks
jest.clearAllMocks();
@ -971,6 +984,153 @@ describe('Conversation Operations', () => {
'Conversation not found or already deleted.',
);
});
it('should decrement tag counts for a deleted bookmarked conversation', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 2, position: 1 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
await deleteConvos('user123', { conversationId: convoId });
const tag = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
expect(tag?.count).toBe(1);
});
it('should decrement counts for every tag on the deleted conversation', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 3, position: 1 });
await ConversationTag.create({ user: 'user123', tag: 'personal', count: 1, position: 2 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work', 'personal'],
});
await deleteConvos('user123', { conversationId: convoId });
const work = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
const personal = await ConversationTag.findOne({ user: 'user123', tag: 'personal' }).lean();
expect(work?.count).toBe(2);
expect(personal?.count).toBe(0);
});
it('should decrement a shared tag once per deleted conversation when clearing all', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 2, position: 1 });
await Conversation.create({
conversationId: uuidv4(),
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
await Conversation.create({
conversationId: uuidv4(),
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
await deleteConvos('user123', {});
const tag = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
expect(tag?.count).toBe(0);
});
it('should not double-decrement duplicate tag entries within one conversation', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 2, position: 1 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work', 'work'],
});
await deleteConvos('user123', { conversationId: convoId });
const tag = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
expect(tag?.count).toBe(1);
});
it('should clamp tag counts at zero and never go negative', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 0, position: 1 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
await deleteConvos('user123', { conversationId: convoId });
const tag = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
expect(tag?.count).toBe(0);
});
it('should not touch tags belonging to another user', async () => {
await ConversationTag.create({ user: 'other', tag: 'work', count: 5, position: 1 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
await deleteConvos('user123', { conversationId: convoId });
const tag = await ConversationTag.findOne({ user: 'other', tag: 'work' }).lean();
expect(tag?.count).toBe(5);
});
it('should not decrement tag counts when the delete removed nothing (lost race)', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 2, position: 1 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
const deleteSpy = jest
.spyOn(Conversation, 'deleteMany')
.mockResolvedValueOnce({ acknowledged: true, deletedCount: 0 } as never);
await deleteConvos('user123', { conversationId: convoId });
deleteSpy.mockRestore();
const tag = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
expect(tag?.count).toBe(2);
});
it('should still decrement tag counts when message deletion fails after the delete', async () => {
await ConversationTag.create({ user: 'user123', tag: 'work', count: 2, position: 1 });
const convoId = uuidv4();
await Conversation.create({
conversationId: convoId,
user: 'user123',
endpoint: EModelEndpoint.openAI,
tags: ['work'],
});
deleteMessages.mockRejectedValueOnce(new Error('message cleanup failed'));
await expect(deleteConvos('user123', { conversationId: convoId })).rejects.toThrow(
'message cleanup failed',
);
const tag = await ConversationTag.findOne({ user: 'user123', tag: 'work' }).lean();
expect(tag?.count).toBe(1);
const convo = await Conversation.findOne({ conversationId: convoId });
expect(convo).toBeNull();
});
});
describe('deleteNullOrEmptyConversations', () => {

View file

@ -11,6 +11,7 @@ import { buildRetentionVisibilityFilter, createFallbackRetentionDate } from '~/u
import { createTempChatExpirationDate } from '~/utils/tempChatRetention';
import { tenantSafeBulkWrite } from '~/utils/tenantBulkWrite';
import { isValidObjectIdString } from '~/utils/objectId';
import { decrementTagCounts } from './conversationTag';
import logger from '~/config/winston';
export interface ConversationMethods {
@ -748,7 +749,7 @@ export function createConversationMethods(
const { deleteMessages } = getMessageMethods();
const userFilter = { ...filter, user };
const conversations = await Conversation.find(userFilter).select(
'conversationId chatProjectId',
'conversationId chatProjectId tags',
);
const conversationIds = conversations.map((c) => c.conversationId);
const projectIds = new Set(
@ -757,22 +758,58 @@ export function createConversationMethods(
.filter((projectId): projectId is string => Boolean(projectId)),
);
/**
* One entry per (conversation, tag) association: each conversation's tags are
* deduped so a duplicate tag entry within a single conversation only decrements
* the bookmark count once.
*/
const tagDecrements: string[] = [];
for (const conversation of conversations) {
if (!conversation.tags?.length) {
continue;
}
for (const tag of new Set(conversation.tags)) {
tagDecrements.push(tag);
}
}
if (!conversationIds.length) {
throw new Error('Conversation not found or already deleted.');
}
const deleteConvoResult = await Conversation.deleteMany(userFilter);
const deleted = deleteConvoResult.deletedCount > 0;
/**
* Reconcile bookmark counts from the deletion before message cleanup: if
* `deleteMessages` later throws, the conversation is already gone and a retry
* finds nothing, so the count must be reconciled here or it would stay stale.
* The decrement is best-effort and never throws, so it cannot block message
* cleanup. The `deletedCount` guard skips a losing concurrent delete whose
* pre-delete snapshot would otherwise decrement a conversation it did not
* actually remove.
*/
if (deleted) {
await decrementTagCounts(mongoose, user, tagDecrements);
}
const deleteMessagesResult = await deleteMessages({
conversationId: { $in: conversationIds },
user,
});
await Promise.all(
[...projectIds].map((projectId) =>
refreshChatProjectStatsForUser(mongoose, user, projectId),
),
);
/**
* Refresh project stats after message cleanup so a stats-refresh error cannot
* prevent `deleteMessages` from running, which would orphan the deleted
* conversations' messages.
*/
if (deleted && projectIds.size > 0) {
await Promise.all(
[...projectIds].map((projectId) =>
refreshChatProjectStatsForUser(mongoose, user, projectId),
),
);
}
return { ...deleteConvoResult, messages: deleteMessagesResult };
} catch (error) {

View file

@ -12,6 +12,54 @@ interface IConversationTag {
[key: string]: unknown;
}
/**
* Atomically decrements tag counts for a user. Each entry in `tags` counts as a
* single decrement, so callers must dedupe tags per conversation before flattening
* to avoid double-decrementing a conversation's duplicate tag entries. Counts are
* clamped at zero to tolerate any pre-existing drift.
*/
export async function decrementTagCounts(
mongoose: typeof import('mongoose'),
user: string,
tags: string[],
): Promise<void> {
if (!tags.length) {
return;
}
const decrementByTag = new Map<string, number>();
for (const tag of tags) {
if (!tag) {
continue;
}
decrementByTag.set(tag, (decrementByTag.get(tag) ?? 0) + 1);
}
if (decrementByTag.size === 0) {
return;
}
try {
const ConversationTag = mongoose.models.ConversationTag as Model<IConversationTag>;
const bulkOps = [...decrementByTag.entries()].map(([tag, amount]) => ({
updateOne: {
filter: { user, tag },
update: [
{
$set: {
count: { $max: [0, { $subtract: [{ $ifNull: ['$count', 0] }, amount] }] },
},
},
],
},
}));
await tenantSafeBulkWrite(ConversationTag, bulkOps);
} catch (error) {
logger.error('[decrementTagCounts] Error decrementing tag counts', error);
}
}
export function createConversationTagMethods(mongoose: typeof import('mongoose')): {
getConversationTags: (user: string) => Promise<
(import('mongoose').FlattenMaps<{