Skip to content

refactor: use last_moved_timestamp instead of scanning edit_history(#1397) #2058

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.

Sign up for GitHub

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

Draft
AshutoshKhadse23 wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

refactor: use last_moved_timestamp instead of scanning edit_history(#1397) #2058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1aed378
refactor: use last_moved_timestamp instead of scanning edit_history
AshutoshKhadse23 Dec 28, 2025
e14221f
fix: the issue having older server implementation untouched
AshutoshKhadse23 Jan 3, 2026
679c00a
fix: complete shift to the new server no dependency on old for timestamp
AshutoshKhadse23 Jan 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 24 additions & 33 deletions lib/api/model/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1113,7 +1113,10 @@ sealed class Message<T extends Conversation> extends MessageBase<T> {
@override
final int id;
bool isMeMessage;
@JsonKey(readValue: _readLastEditTimestamp)
int? lastEditTimestamp;
@JsonKey(readValue: _readLastMovedTimestamp)
int? lastMovedTimestamp;

@JsonKey(fromJson: _reactionsFromJson, toJson: _reactionsToJson)
Reactions? reactions; // null is equivalent to an empty [Reactions]
Expand Down Expand Up @@ -1164,6 +1167,21 @@ sealed class Message<T extends Conversation> extends MessageBase<T> {
);
}

/// Reads `last_edit_timestamp` from JSON.
///
/// This field is present when the message's content has been edited.
static int? _readLastEditTimestamp(Map<dynamic, dynamic> json, String key) {
return json['last_edit_timestamp'] as int?;
}

/// Reads `last_moved_timestamp` from JSON.
///
/// This field is present when the message has been moved to a different
/// channel or topic (excluding resolve/unresolve operations).
static int? _readLastMovedTimestamp(Map<dynamic, dynamic> json, String key) {
return json['last_moved_timestamp'] as int?;
}

Message({
required this.client,
required this.content,
Expand All @@ -1172,6 +1190,7 @@ sealed class Message<T extends Conversation> extends MessageBase<T> {
required this.id,
required this.isMeMessage,
required this.lastEditTimestamp,
required this.lastMovedTimestamp,
required this.reactions,
required this.recipientId,
required this.senderEmail,
Expand Down Expand Up @@ -1257,6 +1276,7 @@ class StreamMessage extends Message<StreamConversation> {
required super.id,
required super.isMeMessage,
required super.lastEditTimestamp,
required super.lastMovedTimestamp,
required super.reactions,
required super.recipientId,
required super.senderEmail,
Expand Down Expand Up @@ -1319,6 +1339,7 @@ class DmMessage extends Message<DmConversation> {
required super.id,
required super.isMeMessage,
required super.lastEditTimestamp,
required super.lastMovedTimestamp,
required super.reactions,
required super.recipientId,
required super.senderEmail,
Expand Down Expand Up @@ -1373,41 +1394,11 @@ enum MessageEditState {
}

static MessageEditState _readFromMessage(Map<dynamic, dynamic> json, String key) {
// Adapted from `analyze_edit_history` in the web app:
// https://github.com/zulip/zulip/blob/c31cebbf68a93927d41e9947427c2dd4d46503e3/web/src/message_list_view.js#L68-L118
final editHistory = json['edit_history'] as List<dynamic>?;
final lastEditTimestamp = json['last_edit_timestamp'] as int?;
if (editHistory == null) {
return (lastEditTimestamp != null)
? MessageEditState.edited
: MessageEditState.none;
}

// Edit history should never be empty whenever it is present
assert(editHistory.isNotEmpty);

bool hasMoved = false;
for (final entry in editHistory) {
if (entry['prev_content'] != null) {
return MessageEditState.edited;
}

if (entry['prev_stream'] != null) {
hasMoved = true;
continue;
}

final prevTopicStr = entry['prev_topic'] as String?;
if (prevTopicStr != null) {
final prevTopic = TopicName.fromJson(prevTopicStr);
final topic = TopicName.fromJson(entry['topic'] as String);
hasMoved |= !topicMoveWasResolveOrUnresolve(topic, prevTopic);
}
}

if (hasMoved) return MessageEditState.moved;
final lastMovedTimestamp = json['last_moved_timestamp'] as int?;

// This can happen when a topic is resolved but nothing else has been edited
if (lastEditTimestamp != null) return MessageEditState.edited;
if (lastMovedTimestamp != null) return MessageEditState.moved;
return MessageEditState.none;
}
}
Expand Down
17 changes: 15 additions & 2 deletions lib/api/model/model.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/api/model/model_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ extension MessageChecks on Subject<Message> {
Subject<int> get id => has((e) => e.id, 'id');
Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');
Subject<int?> get lastMovedTimestamp => has((e) => e.lastMovedTimestamp, 'lastMovedTimestamp');
Subject<MessageEditState> get editState => has((e) => e.editState, 'editState');
Subject<Reactions?> get reactions => has((e) => e.reactions, 'reactions');
Subject<int> get recipientId => has((e) => e.recipientId, 'recipientId');
Expand Down
131 changes: 21 additions & 110 deletions test/api/model/model_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,121 +276,32 @@ void main() {
group('MessageEditState', () {
Map<String, dynamic> baseJson() => deepToJson(eg.streamMessage()) as Map<String, dynamic>;

group('Edit history is absent', () {
test('Message with no evidence of an edit history -> none', () {
check(Message.fromJson(baseJson()..['edit_history'] = null))
.editState.equals(MessageEditState.none);
});

test('Message without edit history has last edit timestamp -> edited', () {
check(Message.fromJson(baseJson()
..['edit_history'] = null
..['last_edit_timestamp'] = 1678139636))
.editState.equals(MessageEditState.edited);
});
test('No edits or moves -> none', () {
check(Message.fromJson(baseJson()
..['last_edit_timestamp'] = null
..['last_moved_timestamp'] = null))
.editState.equals(MessageEditState.none);
});

void checkEditState(MessageEditState editState, List<Map<String, dynamic>> editHistory){
check(Message.fromJson(baseJson()..['edit_history'] = editHistory))
.editState.equals(editState);
}
test('Content edited only -> edited', () {
check(Message.fromJson(baseJson()
..['last_edit_timestamp'] = 1678139636
..['last_moved_timestamp'] = null))
.editState.equals(MessageEditState.edited);
});

group('edit history exists', () {
test('Moved message has last edit timestamp but no actual edits -> moved', () {
check(Message.fromJson(baseJson()
..['edit_history'] = [{'prev_stream': 5, 'stream': 7}]
..['last_edit_timestamp'] = 1678139636))
.editState.equals(MessageEditState.moved);
});

test('Channel change only -> moved', () {
checkEditState(MessageEditState.moved,
[{'prev_stream': 5, 'stream': 7}]);
});

test('Topic name change only -> moved', () {
checkEditState(MessageEditState.moved,
[{'prev_topic': 'old_topic', 'topic': 'new_topic'}]);
});

test('Both topic and content changed -> edited', () {
checkEditState(MessageEditState.edited, [
{'prev_topic': 'old_topic', 'topic': 'new_topic'},
{'prev_content': 'old_content'},
]);
checkEditState(MessageEditState.edited, [
{'prev_content': 'old_content'},
{'prev_topic': 'old_topic', 'topic': 'new_topic'},
]);
});

test('Both topic and content changed in a single edit -> edited', () {
checkEditState(MessageEditState.edited,
[{'prev_topic': 'old_topic', 'topic': 'new_topic', 'prev_content': 'old_content'}]);
});

test('Content change only -> edited', () {
checkEditState(MessageEditState.edited,
[{'prev_content': 'old_content'}]);
});
test('Moved only -> moved', () {
check(Message.fromJson(baseJson()
..['last_edit_timestamp'] = null
..['last_moved_timestamp'] = 1678139636))
.editState.equals(MessageEditState.moved);
});

group('topic resolved in edit history', () {
test('Topic was only resolved -> none', () {
checkEditState(MessageEditState.none,
[{'prev_topic': 'old_topic', 'topic': '✔ old_topic'}]);
});

test('Topic was resolved but the content changed in the history -> edited', () {
checkEditState(MessageEditState.edited, [
{'prev_topic': 'old_topic', 'topic': '✔ old_topic'},
{'prev_content': 'old_content'},
]);
});

test('Topic was resolved but it also moved in the history -> moved', () {
checkEditState(MessageEditState.moved, [
{'prev_topic': 'old_topic', 'topic': 'new_topic'},
{'prev_topic': '✔ old_topic', 'topic': 'old_topic'},
]);
});

test('Topic was moved but it also was resolved in the history -> moved', () {
checkEditState(MessageEditState.moved, [
{'prev_topic': '✔ old_topic', 'topic': 'old_topic'},
{'prev_topic': 'old_topic', 'topic': 'new_topic'},
]);
});

// Technically the topic *was* unresolved, so MessageEditState.none
// would be valid and preferable -- if it didn't need more intense
// computation than we're comfortable with in a hot codepath, i.e.,
// a regex test instead of a simple `startsWith` / `substring` check.
// See comment on the implementation, and discussion:
// https://github.com/zulip/zulip-flutter/pull/1242#discussion_r1917592157
test('Unresolving topic with a weird prefix -> moved', () {
checkEditState(MessageEditState.moved,
[{'prev_topic': '✔ ✔old_topic', 'topic': 'old_topic'}]);
});

// Similar reasoning as in the previous test.
// Also, Zulip doesn't produce topics with a weird resolved-topic prefix,
// so this case can only be produced by unusual input in an
// edit/move-topic UI. A "moved" marker seems like a fine response
// in that circumstance.
test('Resolving topic with a weird prefix -> moved', () {
checkEditState(MessageEditState.moved,
[{'prev_topic': 'old_topic', 'topic': '✔ ✔old_topic'}]);
});

// Similar reasoning as the previous test, including that this case had to
// involve unusual input in an edit/move-topic UI.
// Here the computation burden would have come from calling
// [TopicName.canonicalize].
test('Topic was resolved but with changed case -> moved', () {
checkEditState(MessageEditState.moved,
[{'prev_topic': 'old ToPiC', 'topic': '✔ OLD tOpIc'}]);
});
test('Both edited and moved -> edited', () {
check(Message.fromJson(baseJson()
..['last_edit_timestamp'] = 1678139700
..['last_moved_timestamp'] = 1678139636))
.editState.equals(MessageEditState.edited);
});
});
}
4 changes: 4 additions & 0 deletions test/example_data.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,7 @@ StreamMessage streamMessage({
String? content,
String? contentMarkdown,
int? lastEditTimestamp,
int? lastMovedTimestamp,
List<Reaction>? reactions,
int? timestamp,
List<MessageFlag>? flags,
Expand All @@ -744,6 +745,7 @@ StreamMessage streamMessage({
'flags': flags ?? [],
'id': id ?? _nextMessageId(),
'last_edit_timestamp': lastEditTimestamp,
'last_moved_timestamp': lastMovedTimestamp,
'subject': topic ?? _defaultTopic,
'submessages': submessages ?? [],
'timestamp': timestamp ?? 1678139636,
Expand All @@ -770,6 +772,7 @@ DmMessage dmMessage({
String? content,
String? contentMarkdown,
int? lastEditTimestamp,
int? lastMovedTimestamp,
int? timestamp,
List<MessageFlag>? flags,
List<Submessage>? submessages,
Expand All @@ -787,6 +790,7 @@ DmMessage dmMessage({
'flags': flags ?? [],
'id': id ?? _nextMessageId(),
'last_edit_timestamp': lastEditTimestamp,
'last_moved_timestamp': lastMovedTimestamp,
'subject': '',
'submessages': submessages ?? [],
'timestamp': timestamp ?? 1678139636,
Expand Down