Communication: Remeber last scroll position when switching tabs
#9614
Conversation
github-actions
bot
added
the
client
![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 (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
253-259: Add safety checks for scroll position restoration.While the implementation works, consider adding these safety measures:
- Add null check for conversation ID
- Validate saved scroll position against current content height
Consider this safer implementation:
const savedScrollPosition = sessionStorage.getItem(this.sessionStorageKey + this._activeConversation?.id); -if (savedScrollPosition) { +if (savedScrollPosition && this._activeConversation?.id) { const scrollPos = parseInt(savedScrollPosition, 10); + const maxScroll = this.content.nativeElement.scrollHeight; + if (scrollPos <= maxScroll) { this.content.nativeElement.scrollTop = parseInt(savedScrollPosition, 10); + } else { + this.content.nativeElement.scrollTop = maxScroll; + } } else { this.content.nativeElement.scrollTop = this.content.nativeElement.scrollHeight; }
📜 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 (4 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 (3)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (3)
42-42: LGTM: Well-structured storage key.The sessionStorageKey follows proper naming conventions and provides good namespacing for scroll position storage.
142-142: LGTM: Proper event listener setup.The scroll event listener is correctly added in ngAfterViewInit, following Angular best practices.
148-148: LGTM: Proper event listener cleanup.The scroll event listener is correctly removed in ngOnDestroy, preventing memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
42-42: Add type annotation for sessionStorageKey.For better type safety and code clarity, explicitly declare the type of the storage key.
- private sessionStorageKey = 'conversationId.scrollPosition.'; + private readonly sessionStorageKey: string = 'conversationId.scrollPosition.';
📜 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 (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
...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
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html (2 hunks)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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)
🔇 Additional comments (10)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.html (3)
99-99: LGTM! Good consolidation of event handling logic.
The change properly encapsulates the post creation and scroll behavior into a single method, which is a cleaner approach.
102-102: LGTM! Consistent event handling in mobile view.
The change maintains consistency with the modal implementation, using the same consolidated event handler.
112-115: LGTM! Consistent event handling across all views.
The changes maintain a unified approach to message creation handling across mobile and desktop views. This consistency will help ensure reliable scroll position management.
Let's verify the scroll position management implementation:
✅ Verification successful
Scroll position management is properly implemented and working as expected
The verification confirms that scroll position management is correctly implemented in the component:
- Scroll position is saved to session storage using a debounced event handler
- When creating new messages, the view automatically scrolls to the bottom
- For existing conversations, the saved scroll position is restored
- The implementation handles both new message creation and conversation switching scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for scroll position management implementation in the component
# Expected: Find scroll position related code in the component
# Look for scroll position related code in the component
rg -A 5 "scrollTo|scroll" src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
# Look for session storage usage for scroll position
ast-grep --pattern 'sessionStorage.setItem($_, $_)'
Length of output: 4176
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (7)
42-48: LGTM: Well-structured scroll position management setup.
The implementation uses appropriate types and follows Angular best practices:
- Good use of sessionStorage for maintaining scroll position during session
- Reasonable debounce time of 100ms for scroll events
- Proper use of RxJS Subject for handling scroll events
102-102: LGTM: Proper initialization of scroll handling.
Correctly initializes scroll debounce setup in ngOnInit lifecycle hook.
146-146: Skip: Previously reviewed.
Skipping as previous review comments about adding null checks for content.nativeElement are still valid.
Also applies to: 152-152
250-262: Skip: Previously reviewed.
Skipping as previous review comments about improving scroll position restoration robustness are still valid.
285-287: Skip: Previously reviewed.
Skipping as previous review comments about adding null checks and type safety to saveScrollPosition are still valid.
289-292: LGTM: Well-structured message creation handler.
Good implementation that:
- Follows single responsibility principle
- Has clear and descriptive method name
- Properly handles the workflow of creating a new message
148-153: LGTM: Proper cleanup implementation.
The component correctly implements memory leak prevention:
- Uses takeUntil pattern for RxJS subscriptions
- Properly implements OnDestroy
- Cleans up event listeners
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (2)
134-144: Improve test readability and maintainability.
Several improvements can be made to this test case:
- Use English comments instead of German
- Extract magic numbers into named constants
- Use more specific Jest matchers
Apply this diff to improve the test:
it('should save the scroll position in sessionStorage', fakeAsync(() => {
- // Überwache sessionStorage.setItem
+ // Monitor sessionStorage.setItem
+ const SCROLL_POSITION = 200;
+ const DEBOUNCE_DELAY = 100;
const setItemSpy = jest.spyOn(sessionStorage, 'setItem');
component.ngOnInit();
- component.content.nativeElement.scrollTop = 200;
+ component.content.nativeElement.scrollTop = SCROLL_POSITION;
component.saveScrollPosition();
- tick(100);
+ tick(DEBOUNCE_DELAY);
const expectedKey = `${component.sessionStorageKey}${component._activeConversation?.id}`;
- const expectedValue = '200';
+ const expectedValue = SCROLL_POSITION.toString();
- expect(setItemSpy).toHaveBeenCalledWith(expectedKey, expectedValue);
+ expect(setItemSpy).toHaveBeenCalledExactlyOnceWith(expectedKey, expectedValue);
}));146-152: Enhance test specificity and remove unnecessary calls.
The test can be improved in several ways:
- The initial scroll position seems arbitrary and its purpose is unclear
- The
fixture.detectChanges()call appears unnecessary here - The timing expectations could be more specific
Apply this diff to improve the test:
it('should scroll to the bottom when a new message is created', fakeAsync(() => {
- component.content.nativeElement.scrollTop = 100;
- fixture.detectChanges();
+ // Set initial scroll position to simulate user scrolling up
+ const INITIAL_SCROLL_POSITION = 100;
+ component.content.nativeElement.scrollTop = INITIAL_SCROLL_POSITION;
component.handleNewMessageCreated();
tick();
- expect(component.content.nativeElement.scrollTop).toBe(component.content.nativeElement.scrollHeight);
+
+ // Verify scroll position is at the bottom
+ const expectedScrollPosition = component.content.nativeElement.scrollHeight;
+ expect(component.content.nativeElement.scrollTop).toBe(expectedScrollPosition);
+ expect(expectedScrollPosition).toBeGreaterThan(INITIAL_SCROLL_POSITION);
}));src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
42-43: LGTM! Consider adding type annotations for better type safety.
The scroll management properties are well-structured. The debounce time of 100ms is appropriate for scroll events.
Add type annotations for better type safety:
- sessionStorageKey = 'conversationId.scrollPosition.';
+ private readonly sessionStorageKey: string = 'conversationId.scrollPosition.';
- private scrollDebounceTime = 100; // ms
+ private readonly scrollDebounceTime: number = 100; // msAlso applies to: 47-49
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (5 hunks)
- src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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}}
🔇 Additional comments (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
102-102: LGTM! Proper lifecycle management.
The scroll management setup and cleanup are correctly placed in the appropriate lifecycle hooks.
Also applies to: 146-146, 152-152
...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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (1)
21-21: Consider implementing OnDestroy for proper cleanup.
While using ElementRef with inject() is a good practice, ensure proper cleanup to prevent potential memory leaks.
Consider implementing NgOnDestroy:
export class PostingThreadComponent implements OnDestroy {
elementRef = inject(ElementRef);
ngOnDestroy(): void {
// Clean up any subscriptions or references if needed
}
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (9 hunks)
- src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (2 hunks)
- src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (1)
🔇 Additional comments (5)
src/main/webapp/app/shared/metis/posting-thread/posting-thread.component.ts (2)
1-1: LGTM! Modern dependency injection pattern used.
Good use of the modern inject() function for ElementRef injection, which aligns with Angular's latest best practices and provides better tree-shaking capabilities.
Also applies to: 21-21
Line range hint 1-21: Verify scroll position persistence implementation.
The component has been modified to support scroll position management, but the actual implementation logic seems to be in the conversation messages component.
Let's verify the integration:
✅ Verification successful
Based on the search results, I can now generate the final response:
Scroll position persistence is properly integrated with conversation messages component
The PostingThreadComponent correctly integrates with the scroll position management system:
- It injects
ElementRefwhich is used by the parentConversationMessagesComponentto handle scroll position persistence - The parent component maintains scroll positions in session storage with key pattern
conversationId.scrollPosition.* - Scroll position is saved with debouncing (100ms) and restored when conversation changes
- The implementation includes proper cleanup in
ngOnDestroy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for scroll position related logic in conversation messages component
rg -A 5 "scroll" "src/main/webapp/app/overview/course-conversations/layout/conversation-messages"
# Look for session storage usage for scroll position
rg -A 5 "sessionStorage" "src/main/webapp/app/overview/course-conversations"
Length of output: 19111
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (3)
44-51: LGTM! Well-structured property declarations for scroll management.
The new properties are well-organized and follow Angular best practices. The debounce time of 100ms is a good balance between responsiveness and performance.
59-59: LGTM! Appropriate use of ViewChildren for message tracking.
The switch to ViewChildren is correct for managing multiple PostingThreadComponent instances.
174-174: LGTM! Proper reset of scroll position saving flag.
Correctly prevents premature scroll position saving when switching conversations.
...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
…nel-jumping' into bugfix/communication/adjust-channel-jumping
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)
299-302: Consider a more descriptive method name.
The method name handleNewMessageCreated could be more descriptive about its actual behavior. Consider renaming it to createEmptyPostAndScrollToBottom to better reflect its functionality.
- handleNewMessageCreated() {
+ createEmptyPostAndScrollToBottom() {
this.createEmptyPost();
this.scrollToBottomOfMessages();
}44-44: Consider persistent storage for scroll positions.
Currently, scroll positions are stored in sessionStorage, which means they're lost when the browser is closed. Consider using localStorage for persistence across sessions, or implementing a backend storage solution for a more robust user experience.
📜 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 (9 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 (5)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (5)
44-51: LGTM! Well-structured scroll management setup.
The new properties are well-organized and follow Angular best practices. The debounce time of 100ms provides a good balance between responsiveness and performance.
59-59: LGTM! Improved type safety.
The explicit typing of QueryList with PostingThreadComponent enhances type safety and code maintainability.
81-81: LGTM! Well-typed scroll position tracking.
The elementsAtScrollPosition property effectively tracks visible messages for scroll position management.
95-95: LGTM! Modern dependency injection approach.
Using inject() for Renderer2 follows modern Angular practices and ensures safe DOM manipulation.
107-107: LGTM! Proper initialization timing.
Calling setupScrollDebounce in ngOnInit ensures scroll management is initialized before view rendering.
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
| tick(100); | ||
| const expectedKey = `${component.sessionStorageKey}${component._activeConversation?.id}`; | ||
| const expectedValue = '15'; | ||
| expect(setItemSpy).toHaveBeenCalledWith(expectedKey, expectedValue); |
There was a problem hiding this comment.
could we be more specific here. This can be very helpful in debugging if there is a bug somewhere.
| expect(setItemSpy).toHaveBeenCalledWith(expectedKey, expectedValue); | |
| expect(setItemSpy).toHaveBeenCalledExactlyOnceWith(expectedKey, expectedValue); |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (9 hunks)
- src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/component/overview/course-conversations/layout/conversation-messages/conversation-messages.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (1)
...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
Show resolved
Hide resolved
...verview/course-conversations/layout/conversation-messages/conversation-messages.component.ts
Outdated
Show resolved
Hide resolved
PaRangger
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
…on/adjust-channel-jumping
PaRangger
dismissed stale reviews from FelberMartin, Malekos74, ItsaaaMeMario, sachmii, and coderabbitai[bot]
via
b4da15b
PaRangger
requested review from
Malekos74,
sachmii,
ItsaaaMeMario and
FelberMartin
There was a problem hiding this comment.
Tested and works described.
One thing that I noticed - but probably belongs to a different ticket - is, when scrolling up and older posts are being fetched, the previously visible posts are not displayed anymore and only a loading circle is shown (see gif below). It would be nice to keep the previously loaded messages and only show a small loading indicator above the previous messages
Checklist
General
Client
Motivation and Context
Description
I save the last scroll position of the user during the session. If he comes back to the same channel, he start at this channel again
Steps for Testing
Prerequisites:
-1 Student
-1 Course with a atleast 50 messages in an channel (you can use my test course: Tim Cremers Test course on ts2, i prepered the #organization channel with enough posts there)
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
Test Coverage
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores