Communication: Fix conversations not keeping scroll position when opened#12283
Communication: Fix conversations not keeping scroll position when opened#12283
Communication: Fix conversations not keeping scroll position when opened#12283Conversation
github-actions
bot
added
client
|
@anian03 Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
WalkthroughAdds a ResizeObserver to conversation-messages to wait for the host element to gain height before restoring scroll position. Ensures fallback scrolling to the bottom when no stored scroll ID exists. Adds a test-side global ResizeObserver mock. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.ts (1)
231-259: Potential duplicatescrollToStoredIdcalls may cause scroll jitter.Both the
messages.changessubscription (lines 231-235) and the newResizeObserver(lines 255-257) callscrollToStoredId()under similar conditions. When content loads, both observers may fire in quick succession, potentially causing redundant scroll operations or visual jitter.Consider adding a guard (e.g., a flag like
hasRestoredScroll) to ensure scroll restoration happens only once per conversation load.💡 Suggested approach
+ private hasRestoredScroll = false; + private scrollToStoredId() { + if (this.hasRestoredScroll) { + return; + } let savedScrollId: number | undefined; // ... existing logic ... if (savedScrollId) { requestAnimationFrame(() => this.goToLastSelectedElement(savedScrollId, this.isOpenThreadOnFocus)); + this.hasRestoredScroll = true; } else { this.scrollToBottomOfMessages(); + this.hasRestoredScroll = true; } }Reset
hasRestoredScroll = falseinonActiveConversationChange()when switching conversations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.ts` around lines 231 - 259, The code calls scrollToStoredId() from both the messages.changes subscription and the ResizeObserver which can cause duplicate scrolls; add a boolean guard (e.g., hasRestoredScroll) to the component, set it false when switching conversations in onActiveConversationChange(), and check it before calling scrollToStoredId() in both the messages.changes subscription and the ResizeObserver; after successfully calling scrollToStoredId() set hasRestoredScroll = true so subsequent triggers skip the redundant scroll.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.ts`:
- Around line 249-259: The ResizeObserver created in
conversation-messages.component.ts (variable resizeObserver) is never
disconnected and may leak; make it a component property (e.g.
this.resizeObserver) instead of a local variable, assign the new ResizeObserver
to that property where it's created in the component (inside the block that
currently creates resizeObserver), keep the existing observer.unobserve(el)
logic, and add a call to this.resizeObserver.disconnect() in the component's
ngOnDestroy method (safe-guarded with a null check) so the observer is always
torn down when the component is destroyed.
---
Nitpick comments:
In
`@src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.ts`:
- Around line 231-259: The code calls scrollToStoredId() from both the
messages.changes subscription and the ResizeObserver which can cause duplicate
scrolls; add a boolean guard (e.g., hasRestoredScroll) to the component, set it
false when switching conversations in onActiveConversationChange(), and check it
before calling scrollToStoredId() in both the messages.changes subscription and
the ResizeObserver; after successfully calling scrollToStoredId() set
hasRestoredScroll = true so subsequent triggers skip the redundant scroll.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b4db956-ac57-48cc-bbe0-7af5798fa253
📒 Files selected for processing (1)
src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.ts
...rse-conversations-components/layout/conversation-messages/conversation-messages.component.ts
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.spec.ts (1)
67-73: Add cleanup and usejest.fn()for verifiable mock methods.The ResizeObserver mock is assigned globally but never cleaned up, which could leak into other test suites. Additionally, using plain empty functions prevents verifying that
observe/unobserve/disconnectare called correctly by the component.Suggested improvement
+ let originalResizeObserver: typeof ResizeObserver; + beforeAll(() => { + originalResizeObserver = (window as any).ResizeObserver; (window as any).ResizeObserver = class { - observe() {} - unobserve() {} - disconnect() {} + observe = jest.fn(); + unobserve = jest.fn(); + disconnect = jest.fn(); }; }); + + afterAll(() => { + (window as any).ResizeObserver = originalResizeObserver; + });If you need to test that the resize callback actually triggers scrolling behavior, consider storing the callback in a variable that tests can invoke:
let resizeCallback: ResizeObserverCallback; (window as any).ResizeObserver = class { constructor(callback: ResizeObserverCallback) { resizeCallback = callback; } observe = jest.fn(); unobserve = jest.fn(); disconnect = jest.fn(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.spec.ts` around lines 67 - 73, The global ResizeObserver mock in the beforeAll block should be replaced with a verifiable jest.fn-based mock and cleaned up after tests: change the mock class assigned to (window as any).ResizeObserver so its constructor captures the ResizeObserverCallback into a local variable and its observe/unobserve/disconnect members are jest.fn() so calls can be asserted, and add an afterAll (or afterEach) that restores the original window.ResizeObserver to avoid leaking the mock into other suites; refer to the ResizeObserver mock in the beforeAll and the test teardown to implement these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.spec.ts`:
- Around line 67-73: The global ResizeObserver mock in the beforeAll block
should be replaced with a verifiable jest.fn-based mock and cleaned up after
tests: change the mock class assigned to (window as any).ResizeObserver so its
constructor captures the ResizeObserverCallback into a local variable and its
observe/unobserve/disconnect members are jest.fn() so calls can be asserted, and
add an afterAll (or afterEach) that restores the original window.ResizeObserver
to avoid leaking the mock into other suites; refer to the ResizeObserver mock in
the beforeAll and the test teardown to implement these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33acbda0-23ca-4063-bb24-068a2b9c19ea
📒 Files selected for processing (1)
src/main/webapp/app/communication/course-conversations-components/layout/conversation-messages/conversation-messages.component.spec.ts
|
@anian03 Test coverage has been automatically updated in the PR description. |
![sequa-ai[bot]](https://avatars.githubusercontent.com/in/1205244?s=60&v=4){kind=small}
| const resizeObserver = new ResizeObserver((event, observer) => { | ||
| const entry = event.first(); | ||
| if (entry && entry.contentRect.height) { | ||
| // Stop observing as soon as height is non-zero |
There was a problem hiding this comment.
I see you're using a ResizeObserver here to wait until the component has a non-zero height before trying to restore the scroll position. This is a neat way to solve the race condition where the scroll action might fire before the content is fully rendered and sized. I'll make a note of this pattern for handling layout-dependent actions after a view initializes.
|
|
||
| const resizeObserver = new ResizeObserver((event, observer) => { | ||
| const entry = event.first(); | ||
| if (entry && entry.contentRect.height) { |
There was a problem hiding this comment.
I see you're using a ResizeObserver here to wait for the component to have a height before trying to restore the scroll position. That's a clever and robust way to handle this kind of race condition where layout isn't ready on ngAfterViewInit. I'll keep this pattern in mind for similar issues in the future.
|
|
||
| const resizeObserver = new ResizeObserver((event, observer) => { | ||
| const entry = event.first(); | ||
| if (entry && entry.contentRect.height) { |
There was a problem hiding this comment.
I see you're using a ResizeObserver here to wait for the component to have a height before trying to restore the scroll position. That's a clever and robust way to handle this kind of race condition where layout isn't ready on ngAfterViewInit. I'll keep this pattern in mind for similar issues in the future.
|
@anian03 Test coverage has been automatically updated in the PR description. |
1 participant
Summary
By default, conversations should open at the bottom or around the last viewed message. However, when there were embeddings in a message, e.g. Link previews, images, or forwarded messages, this often didn't work. This content is loaded a bit later, causing the messages to increase in height and thus the scroll position changing.
Checklist
General
Client
Motivation and Context
By default, conversations should open at the bottom or around the last viewed message. However, when there were embeddings in a message, e.g. Link previews, images, or forwarded messages, this often didn't work. This content is loaded a bit later, causing the messages to increase in height and thus the scroll position changing.
Resolves #12093
Description
We add a resize observer to the messages container. The container's height changes when the content has finished loading. As soon as this happens, we scroll to the last viewed message (or the bottom if none exists). This observer only runs for the first resize with positive height, ensuring that this logic does not fire when the user resizes the window (which would keep scrolling to the bottom).
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
Code Review
Manual Tests
Test Coverage
Client
Last updated: 2026-03-15 23:19:04 UTC
Screenshots
Before:
Bildschirmaufnahme.2026-03-11.um.16.47.05.mov
After:
Bildschirmaufnahme.2026-03-11.um.20.26.24.mov
Summary by CodeRabbit