Communication: Update group postings in-place
#10283
Conversation
github-actions
bot
added
the
client
WalkthroughThe pull request refines the grouping logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
🧹 Nitpick comments (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
284-287: Extract time difference threshold to a constant.The 5-minute threshold for grouping consecutive messages should be defined as a named constant for better maintainability.
+private readonly CONSECUTIVE_MESSAGE_TIME_THRESHOLD_MINUTES = 5; -if (currentPost.author?.id === currentGroup.author?.id && timeDiff < 5 && timeDiff >= 0) { +if (currentPost.author?.id === currentGroup.author?.id && timeDiff < this.CONSECUTIVE_MESSAGE_TIME_THRESHOLD_MINUTES && timeDiff >= 0) {
409-414: Add error handling for scroll operations.The scroll operation could fail if the DOM elements are not ready. Consider adding try-catch blocks for robustness.
scrollToBottomOfMessages() { // Use setTimeout to ensure the scroll happens after the new message is rendered requestAnimationFrame(() => { + try { this.content.nativeElement.scrollTop = this.content.nativeElement.scrollHeight; + } catch (error) { + console.error('Failed to scroll to bottom:', error); + } }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (3)
54-55: Consider the implications of ViewEncapsulation.None.Using
ViewEncapsulation.Nonemakes the component's styles global, which could lead to style leaks and unintended side effects in other components. Consider usingViewEncapsulation.Emulated(default) orViewEncapsulation.ShadowDomfor better style encapsulation.
195-200: LGTM! Proper cleanup in ngOnDestroy.The component correctly handles cleanup of subscriptions and event listeners, preventing memory leaks.
311-324: LGTM! Efficient in-place group updates.The implementation efficiently updates groups in-place, preventing unnecessary recreation of post groups and avoiding the ngOnDestroy trigger issue mentioned in PR #10246.
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Show resolved
Hide resolved
What do you mean? Does the behaviour differ to the video I made? |
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
|
No they disappear together in test server 6.在 07.02.2025，13:01，Julian Gassner ***@***.***> 写道：
The 2 messages are deleted together but not as we want.
What do you mean? Does the behaviour differ to the video I made?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: ***@***.***>
|
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
268-270:⚠️ Potential issueFix type safety issue with
creationDateDayjsproperty.This is a duplicate of a previous review comment about avoiding
anytype casting.
🧹 Nitpick comments (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
255-309: Improve readability and maintainability of the grouping logic.The grouping logic can be simplified by extracting the group creation and post addition logic into separate methods.
Consider this refactoring:
- private groupPosts(): void { + private groupPosts(): void { + if (!this.posts?.length) { + this.groupedPosts = []; + return; + } + + const { pinnedPosts, unpinnedPosts } = this.separatePostsByPriority(); + const sortedPosts = this.sortPostsByCreationDate(unpinnedPosts); + const updatedGroups = this.createPostGroups(sortedPosts); + + if (pinnedPosts.length) { + updatedGroups.unshift({ author: undefined, posts: pinnedPosts }); + } + + this.groupedPosts = updatedGroups; + this.cdr.detectChanges(); + } + + private separatePostsByPriority(): { pinnedPosts: Post[]; unpinnedPosts: Post[] } { + const pinnedPosts = this.posts.filter((post) => post.displayPriority === DisplayPriority.PINNED); + const unpinnedPosts = this.posts.filter((post) => post.displayPriority !== DisplayPriority.PINNED); + return { pinnedPosts, unpinnedPosts }; + } + + private sortPostsByCreationDate(posts: Post[]): Post[] { + return posts.sort((a, b) => { + const aDate = (a as any).creationDateDayjs; + const bDate = (b as any).creationDateDayjs; + return aDate?.valueOf() - bDate?.valueOf(); + }); + } + + private createPostGroups(sortedPosts: Post[]): PostGroup[] { + const updatedGroups: PostGroup[] = []; + let currentGroup: PostGroup | null = null; + + sortedPosts.forEach((post) => { + if (!currentGroup) { + currentGroup = this.createNewGroup(post); + } else { + const shouldAddToCurrentGroup = this.shouldAddPostToGroup(currentGroup, post); + if (shouldAddToCurrentGroup) { + currentGroup.posts.push({ ...post, isConsecutive: true }); + } else { + updatedGroups.push(currentGroup); + currentGroup = this.createNewGroup(post); + } + } + }); + + if (currentGroup) { + updatedGroups.push(currentGroup); + } + + return updatedGroups; + } + + private createNewGroup(post: Post): PostGroup { + return { author: post.author, posts: [{ ...post, isConsecutive: false }] }; + } + + private shouldAddPostToGroup(group: PostGroup, post: Post): boolean { + const lastPost = group.posts[group.posts.length - 1]; + const currentDate = (post as any).creationDateDayjs; + const lastDate = (lastPost as any).creationDateDayjs; + const timeDiff = currentDate && lastDate ? currentDate.diff(lastDate, 'minute') : Number.MAX_SAFE_INTEGER; + return this.isAuthorEqual(group, { author: post.author, posts: [] }) && timeDiff < 5 && timeDiff >= 0; + }
311-317: Improve theisAuthorEqualmethod's readability.The method can be simplified by using a more direct comparison approach.
Consider this refactoring:
- private isAuthorEqual(groupA: PostGroup, groupB: PostGroup): boolean { - // Both groups are equal if neither has an author; otherwise, they are not - if (!groupA.author || !groupB.author) { - return !groupA.author && !groupB.author; - } - return groupA.author.id === groupB.author.id; - } + private isAuthorEqual(groupA: PostGroup, groupB: PostGroup): boolean { + return (!groupA.author && !groupB.author) || + (groupA.author?.id === groupB.author?.id); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
🔇 Additional comments (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
332-333: LGTM! Good use of incremental updates.The comment accurately describes the purpose of the
groupPostscall, which aligns with the PR objective of updating group postings in-place.
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
Currently when a new post is published all post groupings get updated by completely recreating them. However this triggers the ngOnDestroy hook in the postings which results in the deletion mechanism to not work as expected (Fixes issue #10246).
Description
With this PR we now update groups in place (deletions, additions, new groups) which results in the ngOnDestroy hook not being called.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Client
�
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit