Communication: Fix scrolling error for long messages
#9997
Conversation
PaRangger
added
client
PaRangger
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
PaRangger
added
ready for review
communication
WalkthroughThe changes in this pull request involve a modification to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)
444-444: LGTM! Consider refactoring for better readability.The fix correctly addresses the scrolling error by checking array length. However, the code could be more readable with an early return pattern.
Consider this refactoring to improve readability:
- if (this.elementsAtScrollPosition && this.elementsAtScrollPosition.length > 0 && this.canStartSaving) { - this.saveScrollPosition(this.elementsAtScrollPosition[0].post.id!); - } + if (!this.elementsAtScrollPosition?.length || !this.canStartSaving) { + return; + } + this.saveScrollPosition(this.elementsAtScrollPosition[0].post.id!);
Line range hint
429-446: Consider architectural improvements for better maintainability.The method currently handles both finding visible messages and saving scroll position. Consider these improvements:
- Split into separate methods for better separation of concerns
- Add type safety for post.id
- Make array type explicit
Here's a suggested refactoring:
private findVisibleMessages(): PostingThreadComponent[] { const containerRect = this.content.nativeElement.getBoundingClientRect(); const visibleMessages: PostingThreadComponent[] = []; for (const message of this.messages.toArray()) { if (!message.elementRef?.nativeElement || !message.post?.id) { continue; } const rect = message.elementRef.nativeElement.getBoundingClientRect(); if (rect.top >= containerRect.top && rect.bottom <= containerRect.bottom) { visibleMessages.push(message); break; } } return visibleMessages; } private updateScrollPosition(): void { this.elementsAtScrollPosition = this.findVisibleMessages(); if (!this.elementsAtScrollPosition?.length || !this.canStartSaving) { return; } const firstVisibleMessage = this.elementsAtScrollPosition[0]; if (firstVisibleMessage.post?.id) { this.saveScrollPosition(firstVisibleMessage.post.id); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
Line range hint 171-171: Verify scroll event handling and cleanup.
The scroll event listener is added in ngAfterViewInit but the cleanup could be improved to prevent memory leaks.
Let's verify the scroll event handling implementation:
✅ Verification successful
Based on the results, I can now generate the final response:
Scroll event handling is properly cleaned up in ngOnDestroy.
The code shows that:
- The scroll event listener is added in
ngAfterViewInitusingaddEventListener - The cleanup is properly handled in
ngOnDestroyusingremoveEventListener - The component also properly cleans up other subscriptions via
ngUnsubscribeandscrollSubject
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper scroll event handling and cleanup in Angular components
# Look for scroll event listeners and their cleanup
rg -A 5 "addEventListener.*scroll"
# Check for other scroll position saving implementations
ast-grep --pattern 'saveScrollPosition($$$)'
# Look for potential memory leaks in event listeners
rg -A 10 "addEventListener" | rg -B 10 "removeEventListener"
Length of output: 101512
github-actions
bot
added
tests
and removed
communication
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1)
288-295: LGTM! Consider adding test isolation.The test case effectively verifies the scrolling behavior with falsy scroll position, using proper assertions as per guidelines.
Consider adding test isolation by resetting the component state in a
beforeEachblock:beforeEach(() => { // ... existing setup ... + component.canStartSaving = false; // Reset to known state });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (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}}
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
eceeeren
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
ahbitaqu
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
bugfix/communication/fix-scrolling-error-for-big-messages
branch
Checklist
General
Client
Motivation and Context
Currently, when a message is displayed on-screen that is bigger than the entire message area, multiple errors will get thrown and reported when scrolling. The errors are caused due to a programming error where checking an empty array was assumed to be falsy. The error looks like this:
Description
I added a check on the length of the array to avoid accessing an out of bounds index. I also added a test case to increase the line coverage.
Steps for Testing
Prerequisites:
Sample Text:
Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo. Nullam dictum felis eu pede mollis pretium. Integer tincidunt. Cras dapibus. Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. Aenean leo ligula, porttitor eu, consequat vitae, eleifend ac, enim. Aliquam lorem ante, dapibus in, viverra quis, feugiat a, tellus. Phasellus viverra nulla ut metus varius laoreet. Quisque rutrum. Aenean imperdiet. Etiam ultricies nisi vel augue. Curabitur ullamcorper ultricies nisi. Nam eget dui. Etiam rhoncus. Maecenas tempus, tellus eget condimentum rhoncus, sem quam semper libero, sit amet adipiscing sem neque sed ipsum. Nam quam nunc, blandit vel, luctus pulvinar, hendrerit id, lorem. Maecenas nec odio et ante tincidunt tempus. Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo. Nullam dictum felis eu pede mollis pretium. Integer tincidunt. Cras dapibus. Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. Aenean leo ligula, porttitor eu, consequat vitae, eleifend ac, enim. Aliquam lorem ante, dapibus in, viverra quis, feugiat a, tellus. Phasellus viverra nulla ut metus varius laoreet. Quisque rutrum. Aenean imperdiet. Etiam ultricies nisi vel augue. Curabitur ullamcorper ultricies nisi. Nam eget dui. Etiam rhoncus. Maecenas tempus, tellus eget condimentum rhoncus, sem quam semper libero, sit amet adipiscing sem neque sed ipsum. Nam quam nunc, blandit vel, luctus pulvinar, hendrerit id, lorem. Maecenas nec odio et ante tincidunt tempus. Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor. Aenean massa. Cum sociis natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim. Donec pede justo, fringilla vel, aliquet nec, vulputate eget, arcu. In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo. Nullam dictum felis eu pede mollis pretium. Integer tincidunt. Cras dapibus. Vivamus elementum semper nisi. Aenean vulputate eleifend tellus. Aenean leo ligula, porttitor eu, consequat vitae, eleifend ac, enim. Aliquam lorem ante, dapibus in, viverra quis, feugiat a, tellus. Phasellus viverra nulla ut metus varius laoreet. Quisque rutrum. Aenean imperdiet. Etiam ultricies nisi vel augue. Curabitur ullamcorper ultricies nisi. Nam eget dui. Etiam rhoncus. Maecenas tempus, tellus eget condimentum rhoncus, sem quam semper libero, sit amet adipiscing sem neque sed ipsum. Nam quam nunc, blandit vel, luctus pulvinar, hendrerit id, lorem. Maecenas nec odio et ante tincidunt tempus.
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
Performance Review
Code Review
Manual Tests
Summary by CodeRabbit