Communication: Add unread message marker in conversation
#10018
Conversation
asliayk
added
tests
client
WalkthroughThe pull request introduces enhancements to the conversation messages component, focusing on improving the visibility of unread messages within a channel. The changes span multiple files, including the component's HTML, TypeScript, and SCSS files. A new mechanism is implemented to identify and visually mark the first unread message in a conversation thread. Additionally, the Metis service is updated to preserve the author's role during post updates, and minor styling adjustments are made to the post header component. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss (1)
116-137: Consider extracting magic numbers into CSS variables for better maintainability.The styling looks good, but we can improve maintainability by extracting magic numbers into CSS variables.
.unread-marker { position: absolute; left: 0; right: 0; - min-height: 0.09rem; + --unread-marker-height: 0.09rem; + --unread-text-offset: 0.6rem; + min-height: var(--unread-marker-height); background-color: var(--bs-danger); z-index: 1; .unread-text { position: absolute; - top: -0.6rem; + top: calc(-1 * var(--unread-text-offset)); left: 95%; transform: translateX(-50%); background-color: var(--bs-danger); color: var(--bs-white); - font-size: 0.6rem; + font-size: var(--unread-text-offset); font-weight: bold; padding: 0.1rem 0.3rem; border-radius: 0.2rem; z-index: 2; } }src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
154-187: Consider improving readability of the unread marker logic.The core logic for determining the first unread message is correct and handles edge cases well. However, consider these improvements for better maintainability:
- Break down the method into smaller, focused functions
- Add more descriptive comments explaining the algorithm
- Use more descriptive variable names (e.g.,
remainingUnreadcould beremainingUnreadMessages)Example refactor:
- isFirstUnreadMarker(post: Post, group: PostGroup): boolean { - let remainingUnread = this.unreadCount; - let firstUnreadFound = false; + isFirstUnreadMarker(targetPost: Post, targetGroup: PostGroup): boolean { + const groupIndex = this.findGroupIndex(targetGroup); + if (groupIndex === -1) return false; + + return this.isFirstUnreadInGroup(targetPost, groupIndex); + } + + private findGroupIndex(group: PostGroup): number { + return this.groupedPosts.findIndex((g) => g === group); + } + + private isFirstUnreadInGroup(targetPost: Post, targetGroupIndex: number): boolean { + let remainingUnreadMessages = this.unreadCount; + + // Process groups from newest to oldest + for (let i = this.groupedPosts.length - 1; i >= 0; i--) { + const currentGroup = this.groupedPosts[i]; + + if (i > targetGroupIndex) { + // Subtract messages from newer groups + remainingUnreadMessages -= currentGroup.posts.length; + } else if (i === targetGroupIndex) { + // Check if this post is at the unread boundary + const postIndex = currentGroup.posts.indexOf(targetPost); + return postIndex === currentGroup.posts.length - remainingUnreadMessages; + } else { + // Post is in an older group + return false; + } + } + + return false; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.scss(1 hunks)src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts(2 hunks)src/main/webapp/app/shared/metis/metis.service.ts(1 hunks)src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html(1 hunks)src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/shared/metis/posting-header/post-header/post-header.component.html
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/shared/metis/metis.service.ts (1)
🔇 Additional comments (6)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (3)
32-35: LGTM! Clean interface definition.
The PostGroup interface is well-structured and properly typed.
277-295: LGTM! Comprehensive test coverage for unread marker identification.
The test case thoroughly verifies the logic for identifying the first unread marker in different scenarios.
297-325: LGTM! Well-structured test cases for marker rendering.
The test cases effectively verify both the presence and absence of the unread marker based on the component's state.
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
62-62: LGTM: Property declaration follows Angular guidelines.
The unreadCount property is correctly typed and initialized.
149-149: LGTM: Safe assignment of unread count.
The code safely handles potential undefined values using optional chaining and provides a fallback to 0.
src/main/webapp/app/shared/metis/metis.service.ts (1)
258-258: LGTM: Fix for "Deleted" label persistence.
The code correctly preserves the authorRole during post updates, fixing the issue where the "Deleted" label persisted until page refresh.
...rview/course-conversations/layout/conversation-messages/conversation-messages.component.html
Show resolved
Hide resolved
asliayk
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Thank you for noticing this :) I have removed the unread marker completely if the first unread message of the conversation is also the very first message of that conversation. Since all the content is new to the user, there is no need to separate old and new content with a marker. |
sachmii
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
JerroyTan
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
[Tested on TS3]
Feature works mostly as described. When user has the chat open and new messages come in, red line does not appear (I assume this is desired behavior)
- When the unread messages are under a new user in the chat (meaning the User is shown before the chats, no red line is shown.
Here, all messages sent by user 4 are unread messages, but there is no red line shown. Is this desired behavior? Suggestion would be to show the line directly between the user and the first unread message
| /** | ||
| * Determines whether a post is the "first unread message" in the conversation. | ||
| */ | ||
| isFirstUnreadMarker(post: Post, group: PostGroup): boolean { |
There was a problem hiding this comment.
I think this method is quite hard to read. What about sth like:
isFirstUnreadMarker(post: Post, group: PostGroup): boolean {
const groupIndex = this.groupedPosts.findIndex((g) => g === group);
if (groupIndex === -1) {
return false;
}
let remainingUnread = this.unreadCount;
for (let i = this.groupedPosts.length - 1; i >= 0; i--) {
const currentGroup = this.groupedPosts[i];
if (i > groupIndex) {
remainingUnread -= currentGroup.posts.length;
continue;
}
if (i === groupIndex) {
const postIndexInGroup = currentGroup.posts.indexOf(post);
if (postIndexInGroup === currentGroup.posts.length - remainingUnread) {
return !(groupIndex === 0 && postIndexInGroup === 0);
}
remainingUnread--;
return false;
}
return false;
}
return false;
}
ahbitaqu
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
[Tested on TS2]
Nice feature! I found some issues though:
- When reading messages and changing the channel, the channel shows those messages to be unread again.
Screen.Recording.2024-12-17.171307.mp4
-
The Marker is not attached to the messages. When writing a message the position of the marker stays fixed even though the message has gone up (See @sachmii 's comment)
-
The marker behaves oddly when writing in an empty channel
Screen.Recording.2024-12-17.171622.mp4
- In Message threads, the replies are not rendered. It shows that the amout of replies is increasing, but those messages only appear once the page is refreshed.
Screen.Recording.2024-12-17.171515.mp4
Checklist
General
Client
Motivation and Context
Communication: Indicator for unread messages within a channel #9947)Communication: "Deleted" label incorrectly appears in message header after editing a message #9987)Description
authorRoleis properly assigned.Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
unread message marker
unread message marker (consecutive view)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style