From 7857a99d63cda8de505494591576e6c0fdac0d4e Mon Sep 17 00:00:00 2001 From: Marco Beretta Date: Sun, 16 Aug 2026 23:07:27 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=20perf:=20Flip=20the=20Pinned=20Flag?= =?UTF-8?q?=20Without=20a=20Full=20Conversation=20Save=20(#14862)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pinning routed through saveConvo, which reads every message id for the conversation and writes the whole array back just to set one boolean, and can trigger a project-stats recompute on top. None of that applies to a pin: it moves no chat between projects, changes nothing the project workspace hides, and opens no retention window. A dedicated setConvoPinned does the single findOneAndUpdate instead. Measured against an in-memory MongoDB with the real message methods wired in, on a 120-message chat: two driver commands and 3706 bytes before, one command and 245 bytes after. The write scales with the message count, so the gap widens on longer chats. Archiving keeps using saveConvo, which it needs for exactly the project stats and retention work a pin does not. --- .../__test-utils__/convos-route-mocks.js | 1 + api/server/routes/__tests__/convos.spec.js | 52 ++++----- api/server/routes/convos.js | 12 +- .../src/methods/conversation.spec.ts | 110 ++++++++++++++---- .../data-schemas/src/methods/conversation.ts | 31 +++++ 5 files changed, 143 insertions(+), 63 deletions(-) diff --git a/api/server/routes/__test-utils__/convos-route-mocks.js b/api/server/routes/__test-utils__/convos-route-mocks.js index 06f6982195..0c49b05fb5 100644 --- a/api/server/routes/__test-utils__/convos-route-mocks.js +++ b/api/server/routes/__test-utils__/convos-route-mocks.js @@ -65,6 +65,7 @@ module.exports = { getConvo: jest.fn(), deleteConvos: jest.fn(), saveConvo: jest.fn(), + setConvoPinned: jest.fn(), deleteAllSharedLinks: jest.fn(), deleteConvoSharedLink: jest.fn(), deleteToolCalls: jest.fn(), diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js index 51f93153e0..ec39d8e947 100644 --- a/api/server/routes/__tests__/convos.spec.js +++ b/api/server/routes/__tests__/convos.spec.js @@ -659,49 +659,45 @@ describe('Convos Routes', () => { describe('POST /convos/pin', () => { const mockConversationId = 'conv-123'; + const { setConvoPinned } = require('~/models'); it('should pin a conversation', async () => { const mockPinnedConvo = { conversationId: mockConversationId, pinned: true }; - saveConvo.mockResolvedValue(mockPinnedConvo); + setConvoPinned.mockResolvedValue(mockPinnedConvo); const response = await request(app).post('/api/convos/pin').send({ arg: mockPinnedConvo }); expect(response.status).toBe(200); expect(response.body).toEqual(mockPinnedConvo); - expect(saveConvo).toHaveBeenCalledWith( - { userId: 'test-user-123' }, - { conversationId: mockConversationId, pinned: true }, - { - context: `POST /api/convos/pin ${mockConversationId}`, - preserveUpdatedAt: true, - noUpsert: true, - }, - ); + expect(setConvoPinned).toHaveBeenCalledWith('test-user-123', mockConversationId, true); + }); + + /** A pin is one boolean: it must not drag in `saveConvo`'s message-id refresh + * and project-stats recompute, which cost an extra read and a large write. */ + it('does not route a pin through the full conversation save', async () => { + setConvoPinned.mockResolvedValue({ conversationId: mockConversationId, pinned: true }); + + await request(app) + .post('/api/convos/pin') + .send({ arg: { conversationId: mockConversationId, pinned: true } }); + + expect(setConvoPinned).toHaveBeenCalledTimes(1); + expect(saveConvo).not.toHaveBeenCalled(); }); it('should unpin a conversation', async () => { const mockUnpinnedConvo = { conversationId: mockConversationId, pinned: false }; - saveConvo.mockResolvedValue(mockUnpinnedConvo); + setConvoPinned.mockResolvedValue(mockUnpinnedConvo); const response = await request(app).post('/api/convos/pin').send({ arg: mockUnpinnedConvo }); expect(response.status).toBe(200); expect(response.body).toEqual(mockUnpinnedConvo); - expect(saveConvo).toHaveBeenCalledWith( - { userId: 'test-user-123' }, - { conversationId: mockConversationId, pinned: false }, - { - context: `POST /api/convos/pin ${mockConversationId}`, - preserveUpdatedAt: true, - noUpsert: true, - }, - ); + expect(setConvoPinned).toHaveBeenCalledWith('test-user-123', mockConversationId, false); }); - /** With timestamps suppressed, an upsert would insert a conversation carrying - * neither createdAt nor updatedAt, so an unknown id has to be rejected instead. */ it('should return 404 when the conversation does not exist', async () => { - saveConvo.mockResolvedValue(null); + setConvoPinned.mockResolvedValue(null); const response = await request(app) .post('/api/convos/pin') @@ -718,7 +714,7 @@ describe('Convos Routes', () => { expect(response.status).toBe(400); expect(response.body).toEqual({ error: 'conversationId is required' }); - expect(saveConvo).not.toHaveBeenCalled(); + expect(setConvoPinned).not.toHaveBeenCalled(); }); it('should return 400 when pinned is not a boolean', async () => { @@ -728,7 +724,7 @@ describe('Convos Routes', () => { expect(response.status).toBe(400); expect(response.body).toEqual({ error: 'pinned must be a boolean' }); - expect(saveConvo).not.toHaveBeenCalled(); + expect(setConvoPinned).not.toHaveBeenCalled(); }); it('should return 400 when pinned is missing', async () => { @@ -738,11 +734,11 @@ describe('Convos Routes', () => { expect(response.status).toBe(400); expect(response.body).toEqual({ error: 'pinned is required' }); - expect(saveConvo).not.toHaveBeenCalled(); + expect(setConvoPinned).not.toHaveBeenCalled(); }); - it('should return 500 when saveConvo fails', async () => { - saveConvo.mockRejectedValue(new Error('Database error')); + it('should return 500 when the pin update fails', async () => { + setConvoPinned.mockRejectedValue(new Error('Database error')); const response = await request(app) .post('/api/convos/pin') diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index e56622b618..c52e4de47c 100644 --- a/api/server/routes/convos.js +++ b/api/server/routes/convos.js @@ -245,17 +245,7 @@ router.post('/pin', validateConvoAccess, async (req, res) => { } try { - const dbResponse = await db.saveConvo( - { userId: req.user.id }, - { conversationId, pinned }, - { - context: `POST /api/convos/pin ${conversationId}`, - /** Pinning is a bookmark, not activity, so the sidebar's `updatedAt` order stands. */ - preserveUpdatedAt: true, - /** Without timestamps, an upsert would insert a conversation that has none. */ - noUpsert: true, - }, - ); + const dbResponse = await db.setConvoPinned(req.user.id, conversationId, pinned); if (!dbResponse) { return res.status(404).json({ error: 'Conversation not found' }); diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index 7dc0ee5fb5..1ddf9a0bbd 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -70,6 +70,8 @@ afterAll(async () => { const saveConvo = (...args: Parameters) => methods.saveConvo(...args) as Promise; +const setConvoPinned = (...args: Parameters) => + methods.setConvoPinned(...args); const getConvo = (...args: Parameters) => methods.getConvo(...args); const getConvoRetention = (...args: Parameters) => @@ -408,29 +410,6 @@ describe('Conversation Operations', () => { return conversationId; }; - /** The sidebar orders by `updatedAt`, so a pin that bumped it would hoist an - * untouched chat into Today and drop it back out of place on unpin. */ - it('leaves updatedAt untouched across a pin and unpin round trip', async () => { - const conversationId = await seedAgedConvo(); - - const pinned = await saveConvo( - { userId: 'user123' }, - { conversationId, pinned: true }, - { preserveUpdatedAt: true, noUpsert: true }, - ); - const unpinned = await saveConvo( - { userId: 'user123' }, - { conversationId, pinned: false }, - { preserveUpdatedAt: true, noUpsert: true }, - ); - - expect(pinned?.pinned).toBe(true); - expect(unpinned?.pinned).toBe(false); - expect(new Date(pinned?.updatedAt ?? 0).toISOString()).toBe(anchor.toISOString()); - expect(new Date(unpinned?.updatedAt ?? 0).toISOString()).toBe(anchor.toISOString()); - expect(new Date(pinned?.createdAt ?? 0).toISOString()).toBe(anchor.toISOString()); - }); - /** Archiving files a chat away rather than touching it, and unarchiving has to * restore it to its real place in the date groups. */ it('leaves updatedAt untouched across an archive and unarchive round trip', async () => { @@ -537,13 +516,96 @@ describe('Conversation Operations', () => { it('does not insert a timestampless conversation for an unknown id', async () => { const result = await saveConvo( { userId: 'user123' }, - { conversationId: uuidv4(), pinned: true }, + { conversationId: uuidv4(), isArchived: true }, { preserveUpdatedAt: true, noUpsert: true }, ); expect(result).toBeNull(); }); }); + + describe('setConvoPinned', () => { + const anchor = new Date('2026-03-05T22:17:14.997Z'); + + const seedAgedConvo = async (user = 'user123') => { + const conversationId = uuidv4(); + await Conversation.collection.insertOne({ + conversationId, + user, + title: 'Living Room Light And Temperature', + endpoint: EModelEndpoint.openAI, + expiredAt: null, + isArchived: false, + messages: [], + createdAt: anchor, + updatedAt: anchor, + }); + return conversationId; + }; + + it('pins a conversation without disturbing its timestamps', async () => { + const conversationId = await seedAgedConvo(); + + const result = await setConvoPinned('user123', conversationId, true); + + expect(result?.pinned).toBe(true); + expect(result?.title).toBe('Living Room Light And Temperature'); + expect(new Date(result?.updatedAt ?? 0).toISOString()).toBe(anchor.toISOString()); + expect(new Date(result?.createdAt ?? 0).toISOString()).toBe(anchor.toISOString()); + }); + + it('unpins a conversation without disturbing its timestamps', async () => { + const conversationId = await seedAgedConvo(); + await setConvoPinned('user123', conversationId, true); + + const result = await setConvoPinned('user123', conversationId, false); + + expect(result?.pinned).toBe(false); + expect(new Date(result?.updatedAt ?? 0).toISOString()).toBe(anchor.toISOString()); + }); + + /** The whole point of the dedicated method: a one-field toggle should not pay for + * `saveConvo`'s message-id read, nor rewrite the array it returns. */ + it('does not read the conversation messages', async () => { + const conversationId = await seedAgedConvo(); + + await setConvoPinned('user123', conversationId, true); + + expect(getMessages).not.toHaveBeenCalled(); + }); + + it('leaves the stored message list alone', async () => { + const conversationId = await seedAgedConvo(); + await Conversation.collection.updateOne( + { conversationId }, + { $set: { messages: ['message-a', 'message-b'] } }, + ); + + await setConvoPinned('user123', conversationId, true); + + const stored = await Conversation.findOne({ conversationId }).lean(); + expect(stored?.messages).toEqual(['message-a', 'message-b']); + }); + + it('returns null for an unknown id rather than inserting one', async () => { + const conversationId = uuidv4(); + + const result = await setConvoPinned('user123', conversationId, true); + + expect(result).toBeNull(); + expect(await Conversation.countDocuments({ conversationId })).toBe(0); + }); + + it('cannot pin another user’s conversation', async () => { + const conversationId = await seedAgedConvo('other-user'); + + const result = await setConvoPinned('user123', conversationId, true); + + expect(result).toBeNull(); + const stored = await Conversation.findOne({ conversationId }).lean(); + expect(stored?.pinned).toBeUndefined(); + }); + }); }); describe('isTemporary conversation handling', () => { diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index 938e862827..0c1a209be9 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -36,6 +36,11 @@ export interface ConversationMethods { preserveUpdatedAt?: boolean; }, ): Promise; + setConvoPinned( + user: string, + conversationId: string, + pinned: boolean, + ): Promise; bulkSaveConvos(conversations: Array>): Promise; getConvosByCursor( user: string, @@ -416,6 +421,31 @@ export function createConversationMethods( } } + /** + * Flips the pinned flag on its own rather than routing through `saveConvo`. + * + * Pinning is pure metadata: it moves no chat between projects, changes nothing the + * project workspace hides, and opens no retention window, so none of `saveConvo`'s + * tail work applies. Going direct drops the `getMessages` round trip and the rewrite + * of the whole message-id array that a one-field toggle would otherwise pay for, and + * `timestamps: false` keeps the sidebar ordering by real activity. + * + * Returns null when no conversation matched, so a pin can never insert one. + */ + async function setConvoPinned(user: string, conversationId: string, pinned: boolean) { + try { + const Conversation = mongoose.models.Conversation as Model; + return await Conversation.findOneAndUpdate( + { conversationId, user }, + { $set: { pinned } }, + { new: true, timestamps: false }, + ).lean(); + } catch (error) { + logger.error('[setConvoPinned] Error updating pinned state', error); + throw new Error('Error updating pinned state'); + } + } + /** * Saves multiple conversations in bulk. */ @@ -945,6 +975,7 @@ export function createConversationMethods( searchConversation, deleteNullOrEmptyConversations, saveConvo, + setConvoPinned, bulkSaveConvos, getConvosByCursor, getConvosQueried,