-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: summary mismatch after new conversation #3402
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a bug where the conversation summary mismatches after a new conversation is added. The fix involves tracking conversations by a stable ID instead of their index. The changes are generally good and move towards a more robust state management. However, I've found a potential issue in the conversation getter where it can fall back to a stale cached conversation, leading to incorrect data being displayed. I've provided a detailed comment and a suggested refactoring to make the logic safer and more correct.
| ServerConversation get conversation { | ||
| if (conversationProvider == null || | ||
| !conversationProvider!.groupedConversations.containsKey(selectedDate) || | ||
| conversationProvider!.groupedConversations[selectedDate] == null || | ||
| conversationProvider!.groupedConversations[selectedDate]!.length <= conversationIdx) { | ||
| // Return cached conversation if available, otherwise create an empty one | ||
| if (_cachedConversation == null) { | ||
| throw StateError("No conversation available"); | ||
| final provider = conversationProvider; | ||
| final id = _cachedConversationId; | ||
| final list = provider?.groupedConversations[selectedDate]; | ||
|
|
||
| if (id != null && list != null && list.isNotEmpty) { | ||
| final i = list.indexWhere((c) => c.id == id); | ||
| if (i != -1) { | ||
| if (i != conversationIdx) conversationIdx = i; | ||
| return _cachedConversation = list[i]; | ||
| } | ||
| if (_cachedConversation != null) return _cachedConversation!; | ||
| } | ||
|
|
||
| if (list == null || conversationIdx >= list.length) { | ||
| if (_cachedConversation == null) throw StateError("No conversation available"); | ||
| return _cachedConversation!; | ||
| } | ||
| _cachedConversation = conversationProvider!.groupedConversations[selectedDate]![conversationIdx]; | ||
|
|
||
| _cachedConversation = list[conversationIdx]; | ||
| _cachedConversationId = list[conversationIdx].id; | ||
| return _cachedConversation!; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation of the conversation getter has a flaw where it can return a stale _cachedConversation, leading to incorrect data being displayed. This happens on lines 64 and 69, where _cachedConversation is returned as a fallback without checking if it belongs to the selectedDate.
For example, if a conversation is moved to a different date, indexWhere will fail, and the getter will incorrectly return the stale cached version, making it seem like the conversation still belongs to the old date.
To fix this and make the logic more robust and readable, I suggest refactoring the getter to prioritize lookups correctly and ensure the returned conversation is always valid for the selectedDate.
Note: This suggested change makes the getter stricter. It will now throw an exception if no valid conversation is available. This might cause updateConversation to crash, as it calls this getter. You may need to adjust updateConversation to handle cases where a conversation isn't found for a given date and index.
ServerConversation get conversation {
final provider = conversationProvider;
final id = _cachedConversationId;
final list = provider?.groupedConversations[selectedDate];
// 1. Try to find by cached ID in the current list.
if (id != null && list != null && list.isNotEmpty) {
final i = list.indexWhere((c) => c.id == id);
if (i != -1) {
if (i != conversationIdx) conversationIdx = i;
return _cachedConversation = list[i];
}
}
// 2. If not found by ID, try to use the index.
if (list != null && conversationIdx < list.length) {
_cachedConversation = list[conversationIdx];
_cachedConversationId = list[conversationIdx].id;
return _cachedConversation!;
}
// 3. As a last resort, use the cached conversation only if it's for the selected date.
if (_cachedConversation != null) {
final cDate = _cachedConversation!.createdAt;
if (cDate.year == selectedDate.year && cDate.month == selectedDate.month && cDate.day == selectedDate.day) {
return _cachedConversation!;
}
}
throw StateError("No conversation available");
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok ok maybe we can improve
ServerConversation get conversation {
final list = conversationProvider?.groupedConversations[selectedDate];
final id = _cachedConversationId;
ServerConversation? result;
if (list != null && list.isNotEmpty) {
final i = list.indexWhere((c) => c.id == id);
if (i != -1) {
result = list[conversationIdx = i];
} else if (conversationIdx < list.length) {
result = list[conversationIdx];
_cachedConversationId = result.id;
}
}
result ??= _cachedConversation;
if (result != null &&
result.createdAt.year == selectedDate.year &&
result.createdAt.month == selectedDate.month &&
result.createdAt.day == selectedDate.day) {
return _cachedConversation = result;
}
throw StateError("No valid conversation found");
}
|
@krushnarout whats the status here |
it's fixed |
|
try removing the > conversationIdx from conversation details provider completely, use conversationId |
Done sir |
when a new conversation is added to the list, the previous index points to a different conversation, causing the summary to change while viewing
fix:
track conversations by id first, automatically update index when list order changes
before:
ScreenRecording_11-09-2025.10-28-27_1.MOV
after:
ScreenRecording_11-09-2025.10-19-37_1.MP4
closes #3374