diff --git a/client/src/data-provider/mutations.ts b/client/src/data-provider/mutations.ts index 93cd955139..cfebd10d17 100644 --- a/client/src/data-provider/mutations.ts +++ b/client/src/data-provider/mutations.ts @@ -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]); } diff --git a/e2e/specs/mock/bookmarks.spec.ts b/e2e/specs/mock/bookmarks.spec.ts new file mode 100644 index 0000000000..6b05c61fc8 --- /dev/null +++ b/e2e/specs/mock/bookmarks.spec.ts @@ -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 { + 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 { + 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 { + 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 { + const token = await getAccessToken(page); + const tags = await fetchJson(page, '/api/tags', token); + return tags.find((t) => t.tag === tag)?.count ?? 0; +} + +async function renameConversation(page: Page, conversation: Locator, title: string): Promise { + 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 { + 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(); + }); +}); diff --git a/packages/data-provider/src/react-query/react-query-service.ts b/packages/data-provider/src/react-query/react-query-service.ts index 59430c78d1..509a3d8ce9 100644 --- a/packages/data-provider/src/react-query/react-query-service.ts +++ b/packages/data-provider/src/react-query/react-query-service.ts @@ -133,6 +133,7 @@ export const useClearConversationsMutation = (): UseMutationResult => { return useMutation(() => dataService.clearAllConversations(), { onSuccess: () => { queryClient.invalidateQueries([QueryKeys.allConversations]); + queryClient.invalidateQueries([QueryKeys.conversationTags]); }, }); }; diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index 94a677b113..1d9621cc0f 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -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; let Conversation: mongoose.Model; let ChatProject: mongoose.Model; +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; ChatProject = mongoose.models.ChatProject as mongoose.Model; + 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', () => { diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index 862dc2468d..cc2594fd26 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -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) { diff --git a/packages/data-schemas/src/methods/conversationTag.ts b/packages/data-schemas/src/methods/conversationTag.ts index c4f239c07d..be65f6b446 100644 --- a/packages/data-schemas/src/methods/conversationTag.ts +++ b/packages/data-schemas/src/methods/conversationTag.ts @@ -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 { + if (!tags.length) { + return; + } + + const decrementByTag = new Map(); + 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; + 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<{