Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve chat initial experience #885

Merged
merged 13 commits into from
Sep 22, 2024
10 changes: 10 additions & 0 deletions app/lib/backend/http/api/messages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ Future<List<ServerMessage>> getMessagesServer() async {
return [];
}

Future<List<ServerMessage>> clearChatServer() async {
var response = await makeApiCall(url: '${Env.apiBaseUrl}v1/clear-chat', headers: {}, method: 'DELETE', body: '');
if (response == null) throw Exception('Failed to delete chat');
if (response.statusCode == 200) {
return [ServerMessage.fromJson(jsonDecode(response.body))];
} else {
throw Exception('Failed to delete chat');
}
}
Comment on lines +26 to +34

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The clearChatServer function is making an API call to clear the chat. However, it's not passing any user identification or authentication headers in the request. This could lead to security issues as anyone can clear the chat without proper authorization. Please ensure that you are passing the necessary headers for user identification and authorization.

- var response = await makeApiCall(url: '${Env.apiBaseUrl}v1/clear-chat', headers: {}, method: 'DELETE', body: '');
+ var response = await makeApiCall(url: '${Env.apiBaseUrl}v1/clear-chat', headers: {'Authorization': 'Bearer $userToken'}, method: 'DELETE', body: '');

Also, consider adding error handling for different types of HTTP status codes. Currently, if the status code is not 200, it throws a generic exception. It would be more informative to throw specific exceptions based on the status code.

- if (response.statusCode == 200) {
-   return [ServerMessage.fromJson(jsonDecode(response.body))];
- } else {
-   throw Exception('Failed to delete chat');
- }
+ switch (response.statusCode) {
+   case 200:
+     return [ServerMessage.fromJson(jsonDecode(response.body))];
+   case 401:
+     throw Exception('Unauthorized request');
+   case 404:
+     throw Exception('Chat not found');
+   default:
+     throw Exception('Failed to delete chat');
+ }


Future<ServerMessage> sendMessageServer(String text, {String? pluginId}) {
return makeApiCall(
url: '${Env.apiBaseUrl}v1/messages?plugin_id=$pluginId',
Expand Down
4 changes: 3 additions & 1 deletion app/lib/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,10 @@ class _DeciderWidgetState extends State<DeciderWidget> {
if (context.read<ConnectivityProvider>().isConnected) {
NotificationService.instance.saveNotificationToken();
}

if (context.read<AuthenticationProvider>().user != null) {
context.read<HomeProvider>().setupHasSpeakerProfile();
context.read<MessageProvider>().setMessagesFromCache();
context.read<MessageProvider>().refreshMessages();
Comment on lines +277 to +280

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The setMessagesFromCache() and refreshMessages() methods are called one after the other. If refreshMessages() is designed to fetch new messages from the server, it might overwrite the messages set by setMessagesFromCache(). Consider using await to ensure that setMessagesFromCache() completes before refreshMessages() starts. Also, error handling should be added for these operations.

-        context.read<MessageProvider>().setMessagesFromCache();
-        context.read<MessageProvider>().refreshMessages();
+        try {
+          await context.read<MessageProvider>().setMessagesFromCache();
+          await context.read<MessageProvider>().refreshMessages();
+        } catch (e) {
+          // Handle or log error
+        }

Please note that this suggestion assumes that both setMessagesFromCache() and refreshMessages() return a Future. If they don't, you may need to refactor them to do so.

}
});
super.initState();
Expand Down
293 changes: 169 additions & 124 deletions app/lib/pages/chat/page.dart

Large diffs are not rendered by default.

88 changes: 59 additions & 29 deletions app/lib/pages/chat/widgets/ai_message.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import 'package:friend_private/backend/preferences.dart';
import 'package:friend_private/backend/schema/memory.dart';
import 'package:friend_private/backend/schema/message.dart';
import 'package:friend_private/backend/schema/plugin.dart';
import 'package:friend_private/pages/memory_detail/memory_detail_provider.dart';
import 'package:friend_private/pages/memory_detail/page.dart';
import 'package:friend_private/providers/memory_provider.dart';
import 'package:friend_private/utils/analytics/mixpanel.dart';
import 'package:friend_private/providers/connectivity_provider.dart';
import 'package:friend_private/utils/other/temp.dart';
Expand Down Expand Up @@ -116,8 +118,8 @@ class _AIMessageState extends State<AIMessage> {
style: TextStyle(fontSize: 15.0, fontWeight: FontWeight.w500, color: Colors.grey.shade300),
)),
if (widget.message.id != 1) _getCopyButton(context), // RESTORE ME
// if (message.id == 1 && displayOptions) const SizedBox(height: 8),
// if (message.id == 1 && displayOptions) ..._getInitialOptions(context),
if (widget.displayOptions) const SizedBox(height: 8),
if (widget.displayOptions) ..._getInitialOptions(context),
Comment on lines +121 to +122

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The changes made here are good for improving user engagement. The initial options are now displayed based on the displayOptions flag, which is more flexible than the previous hard-coded condition.

if (messageMemories.isNotEmpty) ...[
const SizedBox(height: 16),
for (var data in messageMemories.indexed) ...[
Expand All @@ -127,30 +129,58 @@ class _AIMessageState extends State<AIMessage> {
onTap: () async {
final connectivityProvider = Provider.of<ConnectivityProvider>(context, listen: false);
if (connectivityProvider.isConnected) {
if (memoryDetailLoading[data.$1]) return;
setState(() => memoryDetailLoading[data.$1] = true);
var memProvider = Provider.of<MemoryProvider>(context, listen: false);
var idx = memProvider.memoriesWithDates.indexWhere((e) {
if (e.runtimeType == ServerMemory) {
return e.id == data.$2.id;
}
return false;
});

ServerMemory? m = await getMemoryById(data.$2.id);
if (m == null) return;
MixpanelManager().chatMessageMemoryClicked(m);
setState(() => memoryDetailLoading[data.$1] = false);
await Navigator.of(context)
.push(MaterialPageRoute(builder: (c) => MemoryDetailPage(memory: m)));
if (SharedPreferencesUtil().modifiedMemoryDetails?.id == m.id) {
ServerMemory modifiedDetails = SharedPreferencesUtil().modifiedMemoryDetails!;
widget.updateMemory(SharedPreferencesUtil().modifiedMemoryDetails!);
var copy = List<MessageMemory>.from(widget.message.memories);
copy[data.$1] = MessageMemory(
modifiedDetails.id,
modifiedDetails.createdAt,
MessageMemoryStructured(
modifiedDetails.structured.title,
modifiedDetails.structured.emoji,
));
widget.message.memories.clear();
widget.message.memories.addAll(copy);
SharedPreferencesUtil().modifiedMemoryDetails = null;
setState(() {});
if (idx != -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean chat history should have confirmation button.

context.read<MemoryDetailProvider>().updateMemory(idx);
var m = memProvider.memoriesWithDates[idx];
MixpanelManager().chatMessageMemoryClicked(m);
await Navigator.of(context).push(
MaterialPageRoute(
builder: (c) => MemoryDetailPage(
memory: m,
),
),
);
} else {
if (memoryDetailLoading[data.$1]) return;
setState(() => memoryDetailLoading[data.$1] = true);
ServerMemory? m = await getMemoryById(data.$2.id);
if (m == null) return;
idx = memProvider.addMemoryWithDate(m);
MixpanelManager().chatMessageMemoryClicked(m);
setState(() => memoryDetailLoading[data.$1] = false);
context.read<MemoryDetailProvider>().updateMemory(idx);
await Navigator.of(context).push(
MaterialPageRoute(
builder: (c) => MemoryDetailPage(
memory: m,
),
),
);
//TODO: Not needed anymore I guess because memories are stored in provider and read from there only
if (SharedPreferencesUtil().modifiedMemoryDetails?.id == m.id) {
ServerMemory modifiedDetails = SharedPreferencesUtil().modifiedMemoryDetails!;
widget.updateMemory(SharedPreferencesUtil().modifiedMemoryDetails!);
var copy = List<MessageMemory>.from(widget.message.memories);
copy[data.$1] = MessageMemory(
modifiedDetails.id,
modifiedDetails.createdAt,
MessageMemoryStructured(
modifiedDetails.structured.title,
modifiedDetails.structured.emoji,
));
widget.message.memories.clear();
widget.message.memories.addAll(copy);
SharedPreferencesUtil().modifiedMemoryDetails = null;
setState(() {});
}
Comment on lines +132 to +183

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

This block of code has been significantly refactored to improve error handling and maintainability. It now checks if the memory already exists in the provider before fetching it from the server, which can improve performance by reducing unnecessary network calls. However, there's a potential issue with this approach: if the memory data on the server has changed since it was last fetched, the app will display outdated information. To fix this, you could add a timestamp to each memory and check if it's older than a certain threshold before deciding whether to fetch it again.

+                          var memProvider = Provider.of<MemoryProvider>(context, listen: false);
+                          var idx = memProvider.memoriesWithDates.indexWhere((e) {
+                            if (e.runtimeType == ServerMemory) {
+                              return e.id == data.$2.id;
+                            }
+                            return false;
+                          });
+
+                          if (idx != -1 && memProvider.memoriesWithDates[idx].timestamp < SOME_THRESHOLD) {
+                            // Fetch the memory again
+                          }

289:
The padding has been slightly increased, which should make the chat messages easier to read and interact with.

}
} else {
ScaffoldMessenger.of(context).showSnackBar(
Expand Down Expand Up @@ -256,7 +286,7 @@ class _AIMessageState extends State<AIMessage> {
_getInitialOption(BuildContext context, String optionText) {
return GestureDetector(
child: Container(
padding: const EdgeInsets.symmetric(horizontal: 12.0, vertical: 8),
padding: const EdgeInsets.symmetric(horizontal: 12.0, vertical: 10),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The padding for the container has been increased from 8 to 10. This is a minor UI change and doesn't seem to introduce any issues.

-        padding: const EdgeInsets.symmetric(horizontal: 12.0, vertical: 8),
+        padding: const EdgeInsets.symmetric(horizontal: 12.0, vertical: 10),

width: double.maxFinite,
decoration: BoxDecoration(
color: Colors.grey.shade900,
Expand All @@ -273,11 +303,11 @@ class _AIMessageState extends State<AIMessage> {
_getInitialOptions(BuildContext context) {
return [
const SizedBox(height: 8),
_getInitialOption(context, 'What tasks do I have from yesterday?'),
_getInitialOption(context, 'What\'s been on my mind a lot?'),
const SizedBox(height: 8),
_getInitialOption(context, 'What conversations did I have with John?'),
_getInitialOption(context, 'Did I forget to follow up on something?'),
const SizedBox(height: 8),
_getInitialOption(context, 'What advise have I received about entrepreneurship?'),
_getInitialOption(context, 'What\'s the funniest thing I\'ve said lately?'),
Comment on lines +306 to +310

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The text content of the initial options has been updated to be more engaging and interesting for the user. This is a good change for improving user experience.

];
}
}
20 changes: 20 additions & 0 deletions app/lib/pages/chat/widgets/animated_mini_banner.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import 'package:flutter/material.dart';

class AnimatedMiniBanner extends StatelessWidget implements PreferredSizeWidget {
const AnimatedMiniBanner({super.key, required this.showAppBar, required this.child});

final bool showAppBar;
final Widget child;

@override
Widget build(BuildContext context) {
return AnimatedContainer(
height: showAppBar ? kToolbarHeight : 0,
duration: const Duration(milliseconds: 400),
child: child,
);
}

@override
Size get preferredSize => const Size.fromHeight(30);
}
28 changes: 27 additions & 1 deletion app/lib/providers/memory_provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,34 @@ class MemoryProvider extends ChangeNotifier {

void addMemory(ServerMemory memory) {
memories.insert(0, memory);
filterMemories('');
initFilteredMemories();
notifyListeners();
}

int addMemoryWithDate(ServerMemory memory) {
int idx;
var date = memoriesWithDates.indexWhere((element) =>
element is DateTime &&
element.day == memory.createdAt.day &&
element.month == memory.createdAt.month &&
element.year == memory.createdAt.year);
if (date != -1) {
var hour = memoriesWithDates[date + 1].createdAt.hour;
var newHour = memory.createdAt.hour;
if (newHour > hour) {
memoriesWithDates.insert(date + 1, memory);
idx = date + 1;
} else {
memoriesWithDates.insert(date + 2, memory);
idx = date + 2;
}
} else {
memoriesWithDates.add(memory.createdAt);
memoriesWithDates.add(memory);
idx = memoriesWithDates.length - 1;
}
notifyListeners();
return idx;
Comment on lines +113 to +136

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The addMemoryWithDate function seems to be handling the insertion of a memory based on its date. However, it assumes that memories are sorted in ascending order by hour within a day. If this is not the case, the function may insert a memory at an incorrect position.

To ensure correct behavior, you should sort the memories for a particular day by hour before inserting a new memory. Here's a possible way to do it:

-      var hour = memoriesWithDates[date + 1].createdAt.hour;
-      var newHour = memory.createdAt.hour;
-      if (newHour > hour) {
-        memoriesWithDates.insert(date + 1, memory);
-        idx = date + 1;
-      } else {
-        memoriesWithDates.insert(date + 2, memory);
-        idx = date + 2;
-      }
+      int insertIndex = date + 1;
+      while (insertIndex < memoriesWithDates.length && 
+             memoriesWithDates[insertIndex] is ServerMemory &&
+             (memoriesWithDates[insertIndex] as ServerMemory).createdAt.hour < memory.createdAt.hour) {
+        insertIndex++;
+      }
+      memoriesWithDates.insert(insertIndex, memory);
+      idx = insertIndex;

This change will find the correct position to insert the new memory by comparing its hour with existing memories' hours and stop when it finds a memory with a greater hour or reaches the end of the list.

}

void updateMemory(ServerMemory memory, [int? index]) {
Expand Down
38 changes: 38 additions & 0 deletions app/lib/providers/message_provider.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,25 @@ class MessageProvider extends ChangeNotifier {
List<ServerMessage> messages = [];

bool isLoadingMessages = false;
bool hasCachedMessages = false;
bool isClearingChat = false;

String firstTimeLoadingText = '';

void updatePluginProvider(PluginProvider p) {
pluginProvider = p;
}

void setHasCachedMessages(bool value) {
hasCachedMessages = value;
notifyListeners();
}

void setClearingChat(bool value) {
isClearingChat = value;
notifyListeners();
}
Comment on lines +13 to +30

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The new boolean flags hasCachedMessages and isClearingChat along with their setter methods are introduced. These flags help to manage the state of message loading and chat clearing, which is a good practice for state management in Flutter. However, it's important to ensure that these flags are used consistently throughout the codebase to avoid any potential inconsistencies or bugs.

+  bool hasCachedMessages = false;
+  bool isClearingChat = false;

+  void setHasCachedMessages(bool value) {
+    hasCachedMessages = value;
+    notifyListeners();
+  }

+  void setClearingChat(bool value) {
+    isClearingChat = value;
+    notifyListeners();
+  }


void setLoadingMessages(bool value) {
isLoadingMessages = value;
notifyListeners();
Expand All @@ -32,15 +46,39 @@ class MessageProvider extends ChangeNotifier {
notifyListeners();
}

void setMessagesFromCache() {
if (SharedPreferencesUtil().cachedMessages.isNotEmpty) {
setHasCachedMessages(true);
messages = SharedPreferencesUtil().cachedMessages;
}
notifyListeners();
}

Future<List<ServerMessage>> getMessagesFromServer() async {
if (!hasCachedMessages) {
firstTimeLoadingText = 'Reading your memories...';
notifyListeners();
}
setLoadingMessages(true);
var mes = await getMessagesServer();
if (!hasCachedMessages) {
firstTimeLoadingText = 'Learning from your memories...';
notifyListeners();
}
messages = mes;
setLoadingMessages(false);
notifyListeners();
return messages;
}

Future clearChat() async {
setClearingChat(true);
var mes = await clearChatServer();
messages = mes;
setClearingChat(false);
notifyListeners();
}
Comment on lines +49 to +80

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The methods setMessagesFromCache(), getMessagesFromServer(), and clearChat() have been added. These methods handle fetching messages from cache, getting messages from the server, and clearing the chat respectively. The use of SharedPreferencesUtil().cachedMessages for caching is a good approach to improve user experience by reducing load times. However, error handling seems to be missing in these async operations. It would be beneficial to add try-catch blocks around these operations to handle any potential exceptions and provide appropriate feedback to the user.

+  void setMessagesFromCache() {
+    if (SharedPreferencesUtil().cachedMessages.isNotEmpty) {
+      setHasCachedMessages(true);
+      messages = SharedPreferencesUtil().cachedMessages;
+    }
+    notifyListeners();
+  }

+  Future<List<ServerMessage>> getMessagesFromServer() async {
+    if (!hasCachedMessages) {
+      firstTimeLoadingText = 'Reading your memories...';
+      notifyListeners();
+    }
+    setLoadingMessages(true);
+    var mes = await getMessagesServer();
+    if (!hasCachedMessages) {
+      firstTimeLoadingText = 'Learning from your memories...';
+      notifyListeners();
+    }
+    messages = mes;
+    setLoadingMessages(false);
+    notifyListeners();
+    return messages;
+  }

+  Future clearChat() async {
+    setClearingChat(true);
+    var mes = await clearChatServer();
+    messages = mes;
+    setClearingChat(false);
+    notifyListeners();
+  }


void addMessage(ServerMessage message) {
messages.insert(0, message);
notifyListeners();
Expand Down
25 changes: 25 additions & 0 deletions backend/database/chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from datetime import datetime, timezone
from typing import Optional

from fastapi import HTTPException

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The import of HTTPException from fastapi is new in this hunk. However, it's not clear why this was added as there are no changes in the old hunk that would require this new import. If it's used in other parts of the code not shown in the hunks, then it's fine. Otherwise, unnecessary imports should be avoided to keep the code clean and maintainable.

- from fastapi import HTTPException

from google.cloud import firestore

from models.chat import Message
Expand Down Expand Up @@ -82,3 +83,27 @@ def get_messages(uid: str, limit: int = 20, offset: int = 0, include_memories: b
]

return messages


async def batch_delete_messages(parent_doc_ref, batch_size=450):
# batch size is 450 because firebase can perform upto 500 operations in a batch
messages_ref = parent_doc_ref.collection('messages')
while True:
docs = messages_ref.limit(batch_size).stream()
docs_list = list(docs)
if not docs_list:
break
batch = db.batch()
for doc in docs_list:
batch.delete(doc.reference)
batch.commit()


async def clear_chat( uid: str):
try:
user_ref = db.collection('users').document(uid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use db in a router

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't .delete, check the other delete in the code, is setting a flag {deleted: True}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use db in a router

Yes it's in database directory only

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't .delete, check the other delete in the code, is setting a flag {deleted: True}

Got it

if not user_ref.get().exists:
raise HTTPException(status_code=404, detail="User not found")
await batch_delete_messages(user_ref)
except Exception as e:
raise HTTPException(status_code=500, detail=f"Error deleting messages: {str(e)}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The batch_delete_messages function is a good approach to delete messages in batches, considering Firebase's limit of 500 operations per batch. However, it's important to note that this function will continue deleting messages until there are none left. This could potentially lead to an infinite loop if there's an issue with the deletion process or if new messages are being added faster than they're being deleted.

In the clear_chat function, you're correctly checking if the user exists before attempting to delete their messages. However, the error handling could be improved. Currently, any exception raised during the deletion process will result in a 500 status code being returned. It would be more informative to differentiate between different types of errors and return appropriate status codes. For example, if the deletion fails due to insufficient permissions, a 403 status code would be more appropriate.

async def batch_delete_messages(parent_doc_ref, batch_size=450):
    # batch size is 450 because firebase can perform upto 500 operations in a batch
    messages_ref = parent_doc_ref.collection('messages')
    while True:
        docs = messages_ref.limit(batch_size).stream()
        docs_list = list(docs)
        if not docs_list:
            break
        batch = db.batch()
        for doc in docs_list:
            batch.delete(doc.reference)
        batch.commit()


async def clear_chat( uid: str):
    try:
        user_ref = db.collection('users').document(uid)
        if not user_ref.get().exists:
            raise HTTPException(status_code=404, detail="User not found")
        await batch_delete_messages(user_ref)
    except Exception as e:
        # Differentiate between different types of errors
+        if isinstance(e, PermissionError):
+            raise HTTPException(status_code=403, detail=f"Insufficient permissions: {str(e)}")
+        elif isinstance(e, SomeOtherError):
+            raise HTTPException(status_code=XXX, detail=f"Some other error: {str(e)}")
-        raise HTTPException(status_code=500, detail=f"Error deleting messages: {str(e)}")
\ No newline at end of file

6 changes: 6 additions & 0 deletions backend/routers/chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ def send_message(
ai_message.memories = memories if len(memories) < 5 else memories[:5]
return ai_message

@router.delete('/v1/clear-chat', tags=['chat'], response_model=Message)
def clear_chat(uid: str = Depends(auth.get_current_user_uid)):
chat_db.clear_chat(uid)
return initial_message_util(uid)
Comment on lines +58 to +61

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description Entelligence.AI

The new endpoint /v1/clear-chat is introduced to clear the chat history. However, there's no error handling in case chat_db.clear_chat(uid) or initial_message_util(uid) fails. It would be better to wrap these calls in a try-except block and handle potential exceptions appropriately.

@router.delete('/v1/clear-chat', tags=['chat'], response_model=Message)
def clear_chat(uid: str = Depends(auth.get_current_user_uid)):
+    try:
        chat_db.clear_chat(uid)
        return initial_message_util(uid)
+    except Exception as e:
+        # Log the exception and return an appropriate HTTP response
+        logging.error(f"Error while clearing chat for user {uid}: {str(e)}")
+        raise HTTPException(status_code=500, detail="Internal server error")




def initial_message_util(uid: str, plugin_id: Optional[str] = None):
plugin = get_plugin_by_id(plugin_id)
Expand Down
Loading