-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Communication
: Update thread when switching reply context
#10071
Conversation
…entPartComponent according to CodeRabbit
…nt-part/posting-content-part.components.ts Co-authored-by: Mohamed Bilel Besrour <[email protected]>
WalkthroughThe pull request modifies the 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
63-67
: Add a JSDoc comment for ngOnChanges
Other lifecycle methods include doc comments. Consider adding a short doc comment describing the purpose of this method for consistency and clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts
(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/shared/metis/posting-content/posting-content-part/posting-content-part.components.ts (3)
1-1
: Imports are consistent with Angular style guidelines
The additional imports for OnChanges and SimpleChanges are appropriate and correctly placed.
30-30
: Good practice to leverage both OnChanges and OnInit
Implementing OnChanges ensures re-initialization logic runs whenever the input changes, enhancing responsiveness. No further changes required here.
59-59
: Method signature specifies a return type
Declaring ': void' explicitly aligns with the Angular style guide, making the method’s intent clearer.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS6, works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the behaviour is still flaky, i can still reproduce the error on ts1. See video from @PaRangger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Thanks for looking into the issues! When I am testing on TS1, the content of the first message still stays the same, when switching between threads (see gif).
Edit: From what I saw in other PRs, it might help to update the component from @Inputs
to input signals, and then using effect() instead of ngOnChanges. That might increase reactivity and would then also help in the Angular 19 migration 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm the issue. Just tested the changes on TS1, and the issue seems to persist. Tested on both Firefox and Chrome
Closed in favour of #10122 |
Checklist
General
Client
authorities
to all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
When using the “Show X Replies” button in the Communication section to open threads, switching between threads does not update the displayed chat text in the thread view. Instead, the chat text from the first opened thread remains visible, which can lead to confusion.
Fixes #9754 and #9803
Description
After many hours of debugging I found that a ngOnChanges lifecycle hook in PostingContentPartComponent was missing. I added the hook and it now the threads work as expected.
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Screenshots
Summary by CodeRabbit