Communication: Change posting deletion logic to not abort when deconstructing a post #9882
Conversation
github-actions
bot
added
the
client
WalkthroughThe changes introduce a new private method, Changes
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: 1
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/shared/metis/posting.directive.ts (1)
69-69: LGTM! Consider additional cleanup.The centralized deletion logic improves maintainability while maintaining the original timing behavior.
Consider using
takeUntilwith a destroy subject for the interval to prevent potential memory leaks:+ private readonly destroy$ = new Subject<void>(); ngOnInit(): void { this.content = this.posting.content; } ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); if (this.deleteTimer !== undefined) { clearTimeout(this.deleteTimer); this.deletePostingWithoutTimeout(); } if (this.deleteInterval !== undefined) { clearInterval(this.deleteInterval); } } onDeleteEvent(isDelete: boolean) { // ... existing code ... - this.deleteInterval = setInterval(() => { + interval(1000).pipe( + takeUntil(this.destroy$) + ).subscribe(() => { this.deleteTimerInSeconds = Math.max(0, this.deleteTimerInSeconds - 1); this.changeDetector.detectChanges(); - }, 1000); }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/shared/metis/posting.directive.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/shared/metis/posting.directive.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/shared/metis/posting.directive.ts (2)
132-138: Consider a more descriptive method name and verify flag usage.
While the implementation is correct, consider renaming to better reflect its immediate nature, e.g., deletePostingImmediately().
Let's verify the isAnswerPost flag is properly set:
✅ Verification successful
Method name is appropriate, flag usage is correct
The isAnswerPost flag is properly declared and used in the directive. The current method name deletePostingWithoutTimeout is actually more precise than the suggested alternative as it clearly indicates its relationship with the timeout-based deletion mechanism in the class. The flag's usage is consistent with the codebase's pattern of distinguishing between answer posts and regular posts, as seen in the MetisService calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for isAnswerPost assignments
rg "this\.isAnswerPost\s*=" -A 3
Length of output: 640
Script:
#!/bin/bash
# Search for isAnswerPost declarations and usages in the directive file
rg "isAnswerPost" src/main/webapp/app/shared/metis/posting.directive.ts -A 3
# Search for PostingType.ANSWER usage to understand the type check pattern
rg "PostingType\.ANSWER" -A 3
Length of output: 10205
Script:
#!/bin/bash
# Let's check the posting directive file to understand how isAnswerPost is initialized
ast-grep --pattern 'class $_ {
$$$
posting: $_
$$$
}' src/main/webapp/app/shared/metis/posting.directive.ts
Length of output: 115
Script:
#!/bin/bash
# Let's check the entire posting directive file content
cat src/main/webapp/app/shared/metis/posting.directive.ts
Length of output: 4118
45-45: LGTM! Verify component cleanup behavior.
The immediate deletion in ngOnDestroy correctly addresses the issue of aborted deletions when navigating away.
Let's verify there are no other cleanup operations that might interfere:
✅ Verification successful
LGTM! Cleanup behavior is safe and correct.
The deletion cleanup in ngOnDestroy is properly implemented:
- Clears the
deleteTimertimeout if it exists - Immediately executes
deletePostingWithoutTimeout()to ensure deletion is not aborted - The
deletePostingWithoutTimeout()method correctly handles both answer posts and regular posts via the appropriate service methods - No other cleanup operations interfere with the deletion process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other ngOnDestroy implementations in related components
ast-grep --pattern 'ngOnDestroy() {
$$$
}'
Length of output: 46
Script:
#!/bin/bash
# Search for ngOnDestroy implementations with ripgrep for better coverage
rg "ngOnDestroy\(\)" -A 5 --type ts
# Also search for any cleanup related to posting deletion
rg "deletePosting|deleteTimer" --type ts
Length of output: 103803
Script:
#!/bin/bash
# Search for deletePosting and deleteTimer usage in the posting directive
rg "deletePosting|deleteTimer" src/main/webapp/app/shared/metis/posting.directive.ts -A 5
Length of output: 1806
| @@ -42,6 +42,7 @@ export abstract class PostingDirective<T extends Posting> implements OnInit, OnD | |||
| ngOnDestroy(): void { | |||
| if (this.deleteTimer !== undefined) { | |||
| clearTimeout(this.deleteTimer); | |||
| this.deletePostingWithoutTimeout(); | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the test files and implementation, I can now generate the final response:
Additional test coverage needed for deletion scenarios
The current test coverage is insufficient for the deletion scenarios. While there are tests for the UI components and basic deletion functionality, the following scenarios are not covered:
- The
deletePostingWithoutTimeoutmethod implementation is not tested - No tests for deletion during navigation (ngOnDestroy)
- No tests for the timeout-based deletion scenario
- No tests verifying the cleanup of delete intervals
Please add test cases for:
posting.directive.spec.ts:- Test
deletePostingWithoutTimeoutfor both regular posts and answer posts - Test cleanup of intervals in
ngOnDestroy - Test the timeout-based deletion with proper interval cleanup
- Test the deletion timer functionality
- Test
🔗 Analysis chain
Verify test coverage for the deletion scenarios.
While the implementation looks good, please ensure there are tests covering:
- Deletion during navigation away
- Deletion after timeout
- Both answer posts and regular posts
Let's check the test coverage:
Also applies to: 69-69, 132-138
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files and their content
fd "posting.directive.spec.ts" | xargs rg "deletePost|ngOnDestroy"
Length of output: 274
Script:
#!/bin/bash
# Let's get more context around the test file content
fd "posting.directive.spec.ts" | xargs cat
Length of output: 4484
Script:
#!/bin/bash
# Let's also check for any other test files that might be testing this functionality
rg -l "deletePosting" --type ts
Length of output: 462
Script:
#!/bin/bash
# Let's check the implementation to understand what scenarios need to be tested
ast-grep --pattern 'deletePostingWithoutTimeout() {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Let's check the implementation with a different pattern to understand the deletion scenarios
ast-grep --pattern 'class $_ {
$$$
deletePostingWithoutTimeout$_$_ {
$$$
}
$$$
}'
Length of output: 219
Script:
#!/bin/bash
# Let's also check for ngOnDestroy implementation
ast-grep --pattern 'ngOnDestroy() {
$$$
}'
Length of output: 46
Script:
#!/bin/bash
# Let's check the test coverage for the reactions bar components
fd "reactions-bar.*spec.ts" | xargs rg "deletePosting|ngOnDestroy"
Length of output: 68
Script:
#!/bin/bash
# Let's try to find the implementation details with ripgrep
rg -A 5 "deletePostingWithoutTimeout" --type ts
Length of output: 1685
Script:
#!/bin/bash
# Let's check the content of the reactions bar test files directly
fd "reactions-bar.*spec.ts" | xargs cat
Length of output: 30797
Script:
#!/bin/bash
# Let's also check for timeout-related code
rg "setTimeout.*delete" --type ts
Length of output: 35
|
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. |
|
Please unstale |
|
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. |
|
Please unstale |
…ion-when-deconstructing-post
|
Continued in: #10073 |
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Proposed by: #9848
Currently when one deletes a message and switches the chat or generally leaves the communication area before the deletion countdown has expired the delete operation gets aborted. In my opinion this should not be the intended behaviour as I as a user expect a message to get deleted when I press the delete button and leave the page.
Description
When the posting gets deconstructed the client now stops the delete timeout and instantly deletes the posting as it would have once the timeout expired
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
Bug Fixes
Refactor