fix: address code review feedback for data retention PR

Critical:
- Fix BookmarkMenu crash: restore optional chaining on conversation
- Fix migration hazard: backward-compatible sidebar filter that also
  checks expiredAt for documents without isTemporary field

Major:
- Add logging to getRetentionExpiry error path, align with tools.js
- Add tests for retentionMode: ALL in saveConvo and saveMessage
- Fix share route: apply expiredAt for temporary chats too by
  querying the conversation's isTemporary flag server-side
- Add assertions for getRetentionExpiry mocks in process tests

Minor:
- Fix ChatRoute isTemporaryChat to be strictly boolean via Boolean()
- Fix stale test description (expired -> temporary)
- Comment out retentionMode default in example yaml
- Simplify verbose if/else to isTemporary === true
- Add compound index on { user: 1, isTemporary: 1 }
- Remove narrating comment from process.spec.js

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

api/server/routes/share.js

@@ -1,3 +1,4 @@
+const mongoose = require('mongoose');
 const express = require('express');
 const { isEnabled, createTempChatExpirationDate } = require('@librechat/api');
 const { logger } = require('@librechat/data-schemas');
@@ -100,7 +101,18 @@ router.post('/:conversationId', requireJwtAuth, async (req, res) => {
   try {
     const { targetMessageId } = req.body;
     let expiredAt;
-    if (req?.config?.interfaceConfig?.retentionMode === RetentionMode.ALL) {
+    const isRetentionAll = req?.config?.interfaceConfig?.retentionMode === RetentionMode.ALL;
+    let isConvoTemporary = false;
+    if (!isRetentionAll) {
+      const Conversation = mongoose.models.Conversation;
+      const convo = await Conversation.findOne(
+        { conversationId: req.params.conversationId, user: req.user.id },
+        'isTemporary expiredAt',
+      ).lean();
+      isConvoTemporary =
+        convo?.isTemporary === true || (convo?.isTemporary == null && convo?.expiredAt != null);
+    }
+    if (isRetentionAll || isConvoTemporary) {
       try {
         expiredAt = createTempChatExpirationDate(req.config?.interfaceConfig);
       } catch (err) {
@@ -127,12 +139,35 @@ router.post('/:conversationId', requireJwtAuth, async (req, res) => {
 router.patch('/:shareId', requireJwtAuth, async (req, res) => {
   try {
     let expiredAt;
-    if (req?.config?.interfaceConfig?.retentionMode === RetentionMode.ALL) {
+    const isRetentionAll = req?.config?.interfaceConfig?.retentionMode === RetentionMode.ALL;
+    if (isRetentionAll) {
       try {
         expiredAt = createTempChatExpirationDate(req.config?.interfaceConfig);
       } catch (err) {
         logger.error('Error creating shared link expiration date:', err);
       }
+    } else {
+      const SharedLink = mongoose.models.SharedLink;
+      const existing = await SharedLink.findOne(
+        { shareId: req.params.shareId, user: req.user.id },
+        'conversationId',
+      ).lean();
+      if (existing) {
+        const Conversation = mongoose.models.Conversation;
+        const convo = await Conversation.findOne(
+          { conversationId: existing.conversationId, user: req.user.id },
+          'isTemporary expiredAt',
+        ).lean();
+        const isConvoTemporary =
+          convo?.isTemporary === true || (convo?.isTemporary == null && convo?.expiredAt != null);
+        if (isConvoTemporary) {
+          try {
+            expiredAt = createTempChatExpirationDate(req.config?.interfaceConfig);
+          } catch (err) {
+            logger.error('Error creating shared link expiration date:', err);
+          }
+        }
+      }
     }
     const updatedShare = await updateSharedLink(req.user.id, req.params.shareId, expiredAt);
     if (updatedShare) {

api/server/services/Files/Code/__tests__/process-traversal.spec.js

@@ -69,6 +69,7 @@ jest.mock('~/server/services/Files/process', () => ({
   getRetentionExpiry: jest.fn(() => ({})),
 }));
 
+const { getRetentionExpiry } = require('~/server/services/Files/process');
 const { createFile } = require('~/models');
 const { processCodeOutput } = require('../process');
 
@@ -117,6 +118,12 @@ describe('processCodeOutput path traversal protection', () => {
     expect(fileArg.filename).toBe('safe-output.csv');
   });
 
+  test('getRetentionExpiry is called with the request object', async () => {
+    mockSanitizeFilename.mockReturnValueOnce('output.csv');
+    await processCodeOutput({ ...baseParams, name: 'output.csv' });
+    expect(getRetentionExpiry).toHaveBeenCalledWith(baseParams.req);
+  });
+
   test('sanitized name is used for image file records', async () => {
     const { convertImage } = require('~/server/services/Files/images/convert');
     convertImage.mockResolvedValueOnce({

api/server/services/Files/Code/process.spec.js

@@ -90,7 +90,6 @@ jest.mock('~/server/services/Files/images/convert', () => ({
   convertImage: jest.fn(),
 }));
 
-// Mock getRetentionExpiry from Files/process
 jest.mock('~/server/services/Files/process', () => ({
   getRetentionExpiry: jest.fn(() => ({})),
 }));
@@ -103,6 +102,7 @@ jest.mock('~/server/utils', () => ({
 const http = require('http');
 const https = require('https');
 const { createFile, getFiles } = require('~/models');
+const { getRetentionExpiry } = require('~/server/services/Files/process');
 const { getStrategyFunctions } = require('~/server/services/Files/strategies');
 const { convertImage } = require('~/server/services/Files/images/convert');
 const { determineFileType } = require('~/server/utils');
@@ -186,6 +186,7 @@ describe('Code Process', () => {
 
       expect(result.file_id).toBe('mock-uuid-1234');
       expect(result.usage).toBe(1);
+      expect(getRetentionExpiry).toHaveBeenCalledWith(baseParams.req);
     });
   });
 

api/server/services/Files/process.js

@@ -53,8 +53,9 @@ function getRetentionExpiry(req) {
   if (req?.body?.isTemporary || req?.config?.interfaceConfig?.retentionMode === RetentionMode.ALL) {
     try {
       return { expiredAt: createTempChatExpirationDate(req.config?.interfaceConfig) };
-    } catch (_err) {
-      return {};
+    } catch (err) {
+      logger.error('[getRetentionExpiry] Error creating file expiration date:', err);
+      return { expiredAt: null };
     }
   }
   return {};

client/src/components/Chat/Menus/BookmarkMenu.tsx

@@ -27,8 +27,8 @@ const BookmarkMenu: FC = () => {
   const updateConvoTags = useBookmarkSuccess(conversationId);
   const tags = conversation?.tags;
   const isTemporary =
-    conversation.isTemporary ||
-    (conversation.isTemporary === undefined && conversation.expiredAt != null);
+    conversation?.isTemporary === true ||
+    (conversation?.isTemporary === undefined && conversation?.expiredAt != null);
   const menuId = useId();
   const [isMenuOpen, setIsMenuOpen] = useState(false);
   const [isDialogOpen, setIsDialogOpen] = useState(false);
@@ -61,9 +61,9 @@ const BookmarkMenu: FC = () => {
 
   const isActiveConvo = Boolean(
     conversation &&
-      conversationId &&
-      conversationId !== Constants.NEW_CONVO &&
-      conversationId !== 'search',
+    conversationId &&
+    conversationId !== Constants.NEW_CONVO &&
+    conversationId !== 'search',
   );
 
   const handleSubmit = useCallback(

client/src/routes/ChatRoute.tsx

@@ -62,10 +62,11 @@ export default function ChatRoute() {
   const endpointsQuery = useGetEndpointsQuery({ enabled: isAuthenticated });
   const assistantListMap = useAssistantListMap();
 
-  const isTemporaryChat =
+  const isTemporaryChat = Boolean(
     conversation &&
     (conversation.isTemporary ||
-      (conversation.isTemporary === undefined && conversation.expiredAt != null));
+      (conversation.isTemporary === undefined && conversation.expiredAt != null)),
+  );
 
   useEffect(() => {
     if (conversationId === Constants.NEW_CONVO) {

librechat.example.yaml

@@ -131,7 +131,8 @@ interface:
 
   # Temporary chat retention period in hours (default: 720, min: 1, max: 8760)
   # temporaryChatRetention: 1
-  retentionMode: "temporary"
+  # Retention mode: "all" applies expiry to all data types, "temporary" (default) only to temporary chats
+  # retentionMode: "temporary"
 
 # Example Cloudflare turnstile (optional)
 #turnstile:

packages/data-schemas/src/methods/conversation.spec.ts

@@ -398,6 +398,28 @@ describe('Conversation Operations', () => {
       expect(secondSave?.expiredAt).toBeNull();
     });
 
+    it('should set expiredAt for non-temporary conversation when retentionMode is ALL', async () => {
+      mockCtx.isTemporary = false;
+      mockCtx.interfaceConfig = {
+        temporaryChatRetention: 24,
+        retentionMode: 'all',
+      };
+      const result = await saveConvo(mockCtx, mockConversationData);
+      expect(result?.expiredAt).toBeDefined();
+      expect(result?.isTemporary).toBe(false);
+    });
+
+    it('should not set expiredAt when retentionMode is temporary and not isTemporary', async () => {
+      mockCtx.isTemporary = false;
+      mockCtx.interfaceConfig = {
+        temporaryChatRetention: 24,
+        retentionMode: 'temporary',
+      };
+      const result = await saveConvo(mockCtx, mockConversationData);
+      expect(result?.expiredAt).toBeNull();
+      expect(result?.isTemporary).toBe(false);
+    });
+
     it('should filter out temporary conversations in getConvosByCursor', async () => {
       // Create some test conversations
       const newNonTemporaryConvo = await Conversation.create({
@@ -442,7 +464,7 @@ describe('Conversation Operations', () => {
       expect(convoIds).toContain(oldNonTemporaryConvo.conversationId);
     });
 
-    it('should filter out expired conversations in getConvosQueried', async () => {
+    it('should filter out temporary conversations in getConvosQueried', async () => {
       const newNonTemporaryConvo = await Conversation.create({
         conversationId: uuidv4(),
         user: 'user123',

packages/data-schemas/src/methods/conversation.ts

@@ -174,11 +174,7 @@ export function createConversationMethods(
         update.conversationId = newConversationId;
       }
 
-      if (isTemporary) {
-        update.isTemporary = true;
-      } else {
-        update.isTemporary = false;
-      }
+      update.isTemporary = isTemporary === true;
 
       if (isTemporary || interfaceConfig?.retentionMode === RetentionMode.ALL) {
         try {
@@ -285,7 +281,13 @@ export function createConversationMethods(
     }
 
     filters.push({
-      $or: [{ isTemporary: false }, { isTemporary: { $exists: false } }],
+      $or: [
+        { isTemporary: false },
+        {
+          isTemporary: { $exists: false },
+          $or: [{ expiredAt: null }, { expiredAt: { $exists: false } }],
+        },
+      ],
     } as FilterQuery<IConversation>);
 
     if (search) {
@@ -406,7 +408,13 @@ export function createConversationMethods(
       const results = await Conversation.find({
         user,
         conversationId: { $in: conversationIds },
-        $or: [{ isTemporary: false }, { isTemporary: { $exists: false } }],
+        $or: [
+          { isTemporary: false },
+          {
+            isTemporary: { $exists: false },
+            $or: [{ expiredAt: null }, { expiredAt: { $exists: false } }],
+          },
+        ],
       }).lean();
 
       results.sort(

packages/data-schemas/src/methods/message.spec.ts

@@ -475,6 +475,27 @@ describe('Message Operations', () => {
       );
     });
 
+    it('should set expiredAt for non-temporary message when retentionMode is ALL', async () => {
+      mockCtx.isTemporary = false;
+      mockCtx.interfaceConfig = {
+        temporaryChatRetention: 24,
+        retentionMode: 'all',
+      };
+      const result = await saveMessage(mockCtx, mockMessageData);
+      expect(result?.expiredAt).toBeDefined();
+      expect(result?.expiredAt).toBeInstanceOf(Date);
+    });
+
+    it('should not set expiredAt when retentionMode is temporary and not isTemporary', async () => {
+      mockCtx.isTemporary = false;
+      mockCtx.interfaceConfig = {
+        temporaryChatRetention: 24,
+        retentionMode: 'temporary',
+      };
+      const result = await saveMessage(mockCtx, mockMessageData);
+      expect(result?.expiredAt).toBeNull();
+    });
+
     it('should handle missing config gracefully', async () => {
       // Simulate missing config - should use default retention period
       delete mockCtx.interfaceConfig;

packages/data-schemas/src/schema/convo.ts

@@ -52,6 +52,7 @@ convoSchema.index({ expiredAt: 1 }, { expireAfterSeconds: 0 });
 convoSchema.index({ createdAt: 1, updatedAt: 1 });
 convoSchema.index({ conversationId: 1, user: 1, tenantId: 1 }, { unique: true });
 
+convoSchema.index({ user: 1, isTemporary: 1 });
 // index for MeiliSearch sync operations
 convoSchema.index({ _meiliIndex: 1, expiredAt: 1 });