Optimize PR based on feedback - improve code quality and accessibility

Conner Mo · Conner Mo · commit 4e0979ace7d2 · 2025-09-04T22:46:57.000+08:00
- Extract localStorage clearing logic into shared utility function (web/utils/localStorage.ts)
- Add accessibility improvements with aria-labels for checkboxes
- Improve exception handling with specific exception types (OperationalError, ProgrammingError)
- Add comprehensive error logging for better debugging
- Enhanced user permission validation with security logging

This addresses the main feedback points from the PR review:
- Code duplication reduction
- Better error handling and debugging
- Improved accessibility for screen readers
- Enhanced security measures
diff --git a/api/controllers/console/app/conversation.py b/api/controllers/console/app/conversation.py
@@ -1,10 +1,14 @@
 from datetime import datetime
+import logging
 
 import pytz  # pip install pytz
+
+logger = logging.getLogger(__name__)
 from flask_login import current_user
 from flask_restx import Resource, marshal_with, reqparse
 from flask_restx.inputs import int_range
 from sqlalchemy import func, or_
+from sqlalchemy.exc import OperationalError, ProgrammingError
 from sqlalchemy.orm import joinedload
 from werkzeug.exceptions import Forbidden, NotFound
 
@@ -110,6 +114,10 @@ def get(self, app_model):
     def delete(self, app_model):
         """Clear completion conversations and related data including files"""
         if not current_user.is_editor:
+            logger.warning(
+                "Unauthorized deletion attempt: user %s tried to delete completion conversations for app %s",
+                current_user.id, app_model.id
+            )
             raise Forbidden()
 
         parser = reqparse.RequestParser()
@@ -150,7 +158,7 @@ def delete(self, app_model):
             if all_message_ids:
                 try:
                     db.session.query(MessageFeedback).filter(MessageFeedback.message_id.in_(all_message_ids)).delete(synchronize_session=False)
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(MessageFile).filter(MessageFile.message_id.in_(all_message_ids)).delete(synchronize_session=False)  
@@ -175,23 +183,23 @@ def delete(self, app_model):
                     db.session.query(ConversationVariable).filter(
                         ConversationVariable.conversation_id == conversation.id
                     ).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(ToolConversationVariables).filter(
                         ToolConversationVariables.conversation_id == conversation.id
                     ).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(ToolFile).filter(ToolFile.conversation_id == conversation.id).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(PinnedConversation).filter(
                         PinnedConversation.conversation_id == conversation.id
                     ).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
 
             # Delete upload file records
@@ -396,6 +404,10 @@ def get(self, app_model):
     def delete(self, app_model):
         """Clear chat conversations and related data including files"""
         if not current_user.is_editor:
+            logger.warning(
+                "Unauthorized deletion attempt: user %s tried to delete chat conversations for app %s",
+                current_user.id, app_model.id
+            )
             raise Forbidden()
 
         parser = reqparse.RequestParser()
@@ -436,7 +448,7 @@ def delete(self, app_model):
             if all_message_ids:
                 try:
                     db.session.query(MessageFeedback).filter(MessageFeedback.message_id.in_(all_message_ids)).delete(synchronize_session=False)
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(MessageFile).filter(MessageFile.message_id.in_(all_message_ids)).delete(synchronize_session=False)  
@@ -461,23 +473,23 @@ def delete(self, app_model):
                     db.session.query(ConversationVariable).filter(
                         ConversationVariable.conversation_id == conversation.id
                     ).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(ToolConversationVariables).filter(
                         ToolConversationVariables.conversation_id == conversation.id
                     ).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(ToolFile).filter(ToolFile.conversation_id == conversation.id).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
                 try:
                     db.session.query(PinnedConversation).filter(
                         PinnedConversation.conversation_id == conversation.id
                     ).delete()
-                except Exception:
+                except (OperationalError, ProgrammingError):
                     pass  # Table might not exist in this version
 
             # Delete upload file records
diff --git a/web/app/components/app/log/list.tsx b/web/app/components/app/log/list.tsx
@@ -973,6 +973,7 @@ const ConversationList: FC<IConversationList> = ({ logs, appDetail, onRefresh, s
                 checked={isAllSelected}
                 indeterminate={isSomeSelected}
                 onCheck={handleSelectAll}
+                aria-label={t('appLog.table.header.selectAllConversations')}
               />
             </td>
             <td className='whitespace-nowrap bg-background-section-burn py-1.5 pl-3'>{isChatMode ? t('appLog.table.header.summary') : t('appLog.table.header.input')}</td>
@@ -999,6 +1000,7 @@ const ConversationList: FC<IConversationList> = ({ logs, appDetail, onRefresh, s
                   <Checkbox
                     checked={selectedItems.includes(log.id)}
                     onCheck={() => handleSelectItem(log.id)}
+                    aria-label={t('appLog.table.header.selectConversation')}
                   />
                   {!log.read_at && (
                     <span className='inline-block h-1.5 w-1.5 rounded bg-util-colors-blue-blue-500'></span>
diff --git a/web/i18n/en-US/app-log.ts b/web/i18n/en-US/app-log.ts
@@ -20,6 +20,8 @@ const translation = {
       tokens: 'TOKENS',
       user: 'End User or Account',
       version: 'VERSION',
+      selectAllConversations: 'Select all conversations',
+      selectConversation: 'Select conversation',
     },
     pagination: {
       previous: 'Prev',
diff --git a/web/i18n/zh-Hans/app-log.ts b/web/i18n/zh-Hans/app-log.ts
@@ -20,6 +20,8 @@ const translation = {
       tokens: 'TOKENS',
       user: '用户或账户',
       version: '版本',
+      selectAllConversations: '全选会话',
+      selectConversation: '选择会话',
     },
     pagination: {
       previous: '上一页',
diff --git a/web/service/log.ts b/web/service/log.ts
@@ -1,7 +1,7 @@
 import type { Fetcher } from 'swr'
 import { del, get, post } from './base'
 import { mutate } from 'swr'
-import { CONVERSATION_ID_INFO } from '../app/components/base/chat/constants'
+import { clearConversationIds } from '@/utils/localStorage'
 import type {
   AgentLogDetailRequest,
   AgentLogDetailResponse,
@@ -88,36 +88,7 @@ export const clearChatConversations = async ({ appId, conversationIds }: { appId
     const result = await del<any>(`/apps/${appId}/chat-conversations`, { body })
 
     // Clear localStorage to prevent 404 errors on explore pages
-    if (typeof window !== 'undefined') {
-      const conversationIdInfo = JSON.parse(localStorage.getItem(CONVERSATION_ID_INFO) || '{}')
-      
-      // Clear conversation ID for the current app (from logs page)
-      let cleared = false
-      if (conversationIdInfo[appId]) {
-        delete conversationIdInfo[appId]
-        cleared = true
-        console.log(`✅ Cleared conversation ID info for app ${appId}`)
-      }
-      
-      // ADDITIONAL FIX: Also clear ALL conversation IDs to prevent explore page 404 errors
-      const keysToDelete = Object.keys(conversationIdInfo)
-      if (keysToDelete.length > 0) {
-        keysToDelete.forEach(key => {
-          delete conversationIdInfo[key]
-          console.log(`🧹 Cleared conversation ID for ${key} to prevent 404 errors`)
-        })
-        cleared = true
-      }
-      
-      if (cleared) {
-        localStorage.setItem(CONVERSATION_ID_INFO, JSON.stringify(conversationIdInfo))
-        window.dispatchEvent(new StorageEvent('storage', {
-          key: CONVERSATION_ID_INFO,
-          newValue: JSON.stringify(conversationIdInfo),
-          storageArea: localStorage
-        }))
-      }
-    }
+    clearConversationIds(appId, { clearAll: true, debug: true })
 
     // Clear SWR caches
     await Promise.all([
@@ -135,8 +106,11 @@ export const clearChatConversations = async ({ appId, conversationIds }: { appId
     return result
   }
   catch (error) {
-    console.error('Failed to clear chat conversations:', error)
-    throw error
+    console.error('Failed to clear chat conversations for app:', appId, error)
+    if (error instanceof Error)
+      throw new Error(`Failed to clear chat conversations: ${error.message}`)
+
+    throw new Error('Failed to clear chat conversations')
   }
 }
 
@@ -147,36 +121,7 @@ export const clearCompletionConversations = async ({ appId, conversationIds }: {
     const result = await del<any>(`/apps/${appId}/completion-conversations`, { body })
 
     // Clear localStorage to prevent 404 errors on explore pages
-    if (typeof window !== 'undefined') {
-      const conversationIdInfo = JSON.parse(localStorage.getItem(CONVERSATION_ID_INFO) || '{}')
-      
-      // Clear conversation ID for the current app (from logs page)
-      let cleared = false
-      if (conversationIdInfo[appId]) {
-        delete conversationIdInfo[appId]
-        cleared = true
-        console.log(`✅ Cleared conversation ID info for app ${appId}`)
-      }
-      
-      // ADDITIONAL FIX: Also clear ALL conversation IDs to prevent explore page 404 errors
-      const keysToDelete = Object.keys(conversationIdInfo)
-      if (keysToDelete.length > 0) {
-        keysToDelete.forEach(key => {
-          delete conversationIdInfo[key]
-          console.log(`🧹 Cleared conversation ID for ${key} to prevent 404 errors`)
-        })
-        cleared = true
-      }
-      
-      if (cleared) {
-        localStorage.setItem(CONVERSATION_ID_INFO, JSON.stringify(conversationIdInfo))
-        window.dispatchEvent(new StorageEvent('storage', {
-          key: CONVERSATION_ID_INFO,
-          newValue: JSON.stringify(conversationIdInfo),
-          storageArea: localStorage
-        }))
-      }
-    }
+    clearConversationIds(appId, { clearAll: true, debug: true })
 
     // Clear SWR caches
     await Promise.all([
@@ -194,7 +139,10 @@ export const clearCompletionConversations = async ({ appId, conversationIds }: {
     return result
   }
   catch (error) {
-    console.error('Failed to clear completion conversations:', error)
-    throw error
+    console.error('Failed to clear completion conversations for app:', appId, error)
+    if (error instanceof Error)
+      throw new Error(`Failed to clear completion conversations: ${error.message}`)
+
+    throw new Error('Failed to clear completion conversations')
   }
 }
diff --git a/web/utils/localStorage.ts b/web/utils/localStorage.ts
@@ -0,0 +1,137 @@
+import { CONVERSATION_ID_INFO } from '@/config'
+
+/**
+ * Utility functions for managing conversation IDs in localStorage
+ */
+
+export type ConversationIdInfo = {
+  [appId: string]: {
+    [userId: string]: string
+  }
+}
+
+/**
+ * Clear conversation IDs from localStorage to prevent 404 errors
+ * @param appId - Optional specific app ID to clear. If not provided, clears all conversation IDs
+ * @param options - Additional options for clearing behavior
+ */
+export function clearConversationIds(
+  appId?: string,
+  options: {
+    /** Clear all conversation IDs regardless of appId */
+    clearAll?: boolean
+    /** Enable debug logging */
+    debug?: boolean
+  } = {},
+): boolean {
+  if (typeof window === 'undefined')
+    return false
+
+  const { clearAll = true, debug = true } = options
+
+  try {
+    const conversationIdInfo: ConversationIdInfo = JSON.parse(
+      localStorage.getItem(CONVERSATION_ID_INFO) || '{}',
+    )
+
+    let cleared = false
+
+    // Clear specific app ID if provided
+    if (appId && conversationIdInfo[appId]) {
+      delete conversationIdInfo[appId]
+      cleared = true
+      if (debug)
+        console.log(`✅ Cleared conversation ID info for app ${appId}`)
+    }
+
+    // Clear all conversation IDs to prevent explore page 404 errors
+    if (clearAll) {
+      const keysToDelete = Object.keys(conversationIdInfo)
+      if (keysToDelete.length > 0) {
+        keysToDelete.forEach((key) => {
+          delete conversationIdInfo[key]
+          if (debug)
+            console.log(`🧹 Cleared conversation ID for ${key} to prevent 404 errors`)
+        })
+        cleared = true
+      }
+    }
+
+    // Update localStorage and dispatch event if changes were made
+    if (cleared) {
+      const newValue = JSON.stringify(conversationIdInfo)
+      localStorage.setItem(CONVERSATION_ID_INFO, newValue)
+
+      // Dispatch storage event for cross-tab synchronization
+      window.dispatchEvent(new StorageEvent('storage', {
+        key: CONVERSATION_ID_INFO,
+        newValue,
+        storageArea: localStorage,
+      }))
+
+      if (debug)
+        console.log('📱 localStorage conversation IDs updated and storage event dispatched')
+    }
+
+    return cleared
+  }
+  catch (error) {
+    console.error('Failed to clear conversation IDs from localStorage:', error)
+    return false
+  }
+}
+
+/**
+ * Get conversation ID for a specific app and user
+ */
+export function getConversationId(appId: string, userId = 'DEFAULT'): string {
+  if (typeof window === 'undefined')
+    return ''
+
+  try {
+    const conversationIdInfo: ConversationIdInfo = JSON.parse(
+      localStorage.getItem(CONVERSATION_ID_INFO) || '{}',
+    )
+
+    return conversationIdInfo[appId]?.[userId] || ''
+  }
+  catch (error) {
+    console.error('Failed to get conversation ID from localStorage:', error)
+    return ''
+  }
+}
+
+/**
+ * Set conversation ID for a specific app and user
+ */
+export function setConversationId(appId: string, conversationId: string, userId = 'DEFAULT'): boolean {
+  if (typeof window === 'undefined')
+    return false
+
+  try {
+    const conversationIdInfo: ConversationIdInfo = JSON.parse(
+      localStorage.getItem(CONVERSATION_ID_INFO) || '{}',
+    )
+
+    if (!conversationIdInfo[appId])
+      conversationIdInfo[appId] = {}
+
+    conversationIdInfo[appId][userId] = conversationId
+
+    const newValue = JSON.stringify(conversationIdInfo)
+    localStorage.setItem(CONVERSATION_ID_INFO, newValue)
+
+    // Dispatch storage event for cross-tab synchronization
+    window.dispatchEvent(new StorageEvent('storage', {
+      key: CONVERSATION_ID_INFO,
+      newValue,
+      storageArea: localStorage,
+    }))
+
+    return true
+  }
+  catch (error) {
+    console.error('Failed to set conversation ID in localStorage:', error)
+    return false
+  }
+}
