Communication: Fix an issue with link previews
#9532
Conversation
github-actions
bot
added
tests
client
pzdr7
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
pzdr7
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
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 (6)
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.html (1)
1-10: Summary: Effective refactoring to address link preview issuesThe changes in this file effectively contribute to the PR's objective of fixing the issue with link previews persisting across different threads. By transitioning to signals and adopting the new Angular syntax, the component should now be more reactive to state changes. This increased reactivity should allow link previews to refresh correctly when switching between threads.
Key improvements:
- Adoption of the
@fordirective, enhancing performance and adhering to modern Angular practices.- Consistent refactoring of property bindings to use function calls, likely leveraging signals for improved state management.
- Use of a
trackByfunction to optimize list rendering performance.These changes, combined with the modifications in the component logic, should resolve the reported issue (#9530) of duplicated link previews across threads. The refactoring also aligns well with the goal of maintaining high performance and responsiveness, even with large datasets.
To further enhance the link preview functionality:
- Consider implementing lazy loading for link previews to improve initial load times, especially for threads with many links.
- Explore caching mechanisms for link preview data to reduce unnecessary network requests when revisiting threads.
src/test/javascript/spec/component/link-preview/link-preview-container.component.spec.ts (2)
55-59: Updated expectations for signal-based propertiesThe changes correctly reflect the transition to signal-based properties in the component. The use of function calls (e.g.,
component.linkPreviews()) instead of direct property access is appropriate.However, we can make the expectation for
linkPreviewsmore specific:Consider using
toContainEntriesfor a more precise assertion:expect(component.linkPreviews()).toContainEntries([ [0, expect.objectContaining(mockLinkPreviews[0])], [1, expect.objectContaining(mockLinkPreviews[1])] ]);This approach verifies both the order and content of the
linkPreviewsarray more explicitly.
80-80: Updated expectations for signal-based propertiesThe changes correctly reflect the transition to signal-based properties in the component. The use of function calls (e.g.,
component.linkPreviews()) instead of direct property access is appropriate.However, we can improve the specificity of our expectations:
- For line 80, consider using a more reactive approach to update the signal:
component.linkPreviews.set([...component.linkPreviews(), existingLinkPreview]);
- For lines 87-88, consider using
toContainEntriesfor a more precise assertion:expect(component.linkPreviews()).toContainEntries([ [0, expect.objectContaining(newLinkPreview)] ]);This approach verifies both the length and content of the
linkPreviewsarray more explicitly.Also applies to: 84-88
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (3)
14-15: Consider using constructor injection for dependency injectionWhile the
inject()function is valid in Angular 14 and later, the Angular Style Guide recommends using constructor injection for better readability and consistency across the codebase. Constructor injection makes dependencies explicit and easier to test.Consider refactoring to use constructor injection:
-export class LinkPreviewContainerComponent implements OnInit, OnChanges { - private readonly linkPreviewService: LinkPreviewService = inject(LinkPreviewService); - private readonly linkifyService: LinkifyService = inject(LinkifyService); +export class LinkPreviewContainerComponent implements OnInit, OnChanges { + constructor( + private readonly linkPreviewService: LinkPreviewService, + private readonly linkifyService: LinkifyService + ) {} // ... }
46-46: Address the TODO: Make the link preview limit configurableThere's a TODO comment indicating that the limit of 5 link previews should be configurable, possibly at the course level. Implementing this feature would enhance flexibility and allow different courses to set appropriate limits based on their needs.
Would you like assistance in implementing a configuration option for the link preview limit? I can help draft a solution or create a GitHub issue to track this enhancement.
Line range hint
51-71: Prevent potential memory leaks by managing subscriptionsIn the
findPreviews()method, you're subscribing to observables within a loop without unsubscribing. This can lead to memory leaks if the component is destroyed before all subscriptions complete. Ensure that you properly manage these subscriptions.Consider using the
takeUntiloperator along with anngOnDestroylifecycle hook to manage unsubscription:import { OnDestroy } from '@angular/core'; import { Subject } from 'rxjs'; export class LinkPreviewContainerComponent implements OnInit, OnChanges, OnDestroy { private readonly destroy$ = new Subject<void>(); // ... ngOnDestroy() { this.destroy$.next(); this.destroy$.complete(); } private findPreviews() { const links: Link[] = this.linkifyService.find(this.data() ?? ''); // ... links.forEach((link) => { this.linkPreviewService.fetchLink(link.href) .pipe(takeUntil(this.destroy$)) .subscribe({ next: (linkPreview) => { // ... }, }); }); } }Ensure that you import
takeUntilfrom'rxjs/operators'.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.html (1 hunks)
- src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (3 hunks)
- src/test/javascript/spec/component/link-preview/link-preview-container.component.spec.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.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/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (1)
src/test/javascript/spec/component/link-preview/link-preview-container.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 (9)
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.html (2)
1-1: Excellent use of new Angular syntax and optimization techniques!The changes in this line demonstrate several good practices:
- Using the new
@fordirective as per our coding guidelines.- Accessing
linkPreviewsas a function call, which aligns with the transition to signals mentioned in the component changes.- Implementing a
trackByfunction (trackLinks) to optimize Angular's change detection for the list rendering.These changes should improve the component's reactivity and performance.
5-10: Consistent refactoring to enhance reactivityThe changes on lines 5-10 demonstrate a systematic refactoring of property bindings:
- All properties (
showLoadingsProgress,hasError,loaded,posting,isReply,multiple) are now accessed as function calls.- This change aligns with the transition to signals mentioned in the component changes.
- Using function calls instead of direct property access can enhance reactivity and allow for computed properties.
These modifications should improve the component's ability to react to state changes efficiently. The consistent application of this pattern across all properties is commendable and shows attention to detail in the refactoring process.
src/test/javascript/spec/component/link-preview/link-preview-container.component.spec.ts (4)
40-40: Improved input property settingThe change from direct property assignment to
fixture.componentRef.setInput()is a good improvement. This approach better aligns with Angular's change detection mechanism, especially when working with components usingChangeDetectionStrategy.OnPushand input properties defined as readonly signals.
60-62: Correct usage of boolean expectationsThe updated expectations correctly use function calls for signal-based properties. The use of
toBeFalse()andtoBeTrue()for boolean assertions is in line with the provided coding guidelines and improves the readability of the test.
66-66: Consistent input property settingThe use of
fixture.componentRef.setInput()is consistent with the earlier change and correctly reflects the component's use of readonly input properties.
Line range hint
1-90: Overall assessment of the changesThe modifications to this test file consistently reflect the transition to using signals in the
LinkPreviewContainerComponent. The changes improve the alignment of the tests with Angular's latest practices and the component's new implementation.Key improvements:
- Correct usage of
fixture.componentRef.setInput()for setting input properties.- Consistent use of function calls to access signal-based properties.
- Appropriate use of Jest matchers as per the coding guidelines.
While the changes are generally good, the suggestions provided in the previous comments will further enhance the test specificity and robustness. Consider implementing these suggestions to maximize the effectiveness of your test suite.
src/main/webapp/app/shared/link-preview/components/link-preview-container/link-preview-container.component.ts (3)
11-11: Verify the compatibility of usingChangeDetectionStrategy.OnPushImplementing
ChangeDetectionStrategy.OnPushcan improve performance by optimizing change detection. However, ensure that all bindings and inputs are immutable or that you trigger change detection appropriately when mutable objects change. This will prevent any unexpected UI behavior due to Angular not detecting changes in mutable objects.
17-21: Usage ofinput()for@Input()properties may not align with standard practicesDefining input properties using the
input()function is a new feature introduced in Angular 16 as part of the reactive signal model. While it offers advantages in certain scenarios, it's still in developer preview. Ensure that this approach is compatible with your project's Angular version and that all team members are aligned on using this new feature.If you prefer to stick with the standard approach, consider reverting to using the
@Input()decorator:- readonly data = input<string>(); - readonly author = input<User>(); - readonly posting = input<Posting>(); - readonly isEdited = input<boolean>(); - readonly isReply = input<boolean>(); + @Input() data?: string; + @Input() author?: User; + @Input() posting?: Posting; + @Input() isEdited?: boolean; + @Input() isReply?: boolean;
23-27: Ensure that usingsignal()aligns with your Angular version and project standardsThe
signal()function is part of Angular's new reactivity model introduced in Angular 16. It's still in developer preview and subject to change. Please verify that your project is using Angular 16 or later and that adopting signals fits within your project's guidelines.
JanaNF
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Manual tested on TS1.
- Did all the steps as you said.
- Tested it on direct messages too.
- When I edited the messages to other links, it changed the preview too.
- Tried if it works with and without a / after the link (https://orf.at/, https://orf.at). It worked.
Problem: Some links do not work e.g. https://www.youtube.com/ , https://artemis.cit.tum.de/ , https://orf.de/
If that is not an issue from your side, just ignore my review.
|
@JanaNF thanks for testing. I checked these links and they do not currently have a preview, so that should be unrelated to this PR. |
(cherry picked from commit 97e17e6)
Checklist
General
Client
Motivation and Context
Closes #9530
Currently, link previews carry over when switching between threads. This is due to a check from #6760 preventing the previews from being refreshed on changes.
Description
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
Code Review
Manual Tests
Test Coverage
Unchanged
Screenshots
Screencast.from.20.10.2024.13.46.12.webm
Summary by CodeRabbit
New Features
Bug Fixes
Tests