-
Notifications
You must be signed in to change notification settings - Fork 294
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
Development
: Improve performance of programming exercise details view
#9785
Development
: Improve performance of programming exercise details view
#9785
Conversation
WalkthroughThe pull request introduces modifications to the Changes
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
|
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: 3
🧹 Outside diff range and nitpick comments (4)
src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts (1)
53-53
: LGTM! Consider documenting the required input.The changes follow modern Angular patterns. Consider adding JSDoc to document the required sections input for better maintainability.
+/** The sections to be displayed in the overview list. */ sections = input.required<DetailOverviewSection[]>();
Also applies to: 59-59
src/main/webapp/app/detail-overview-list/detail-overview-list.component.html (1)
Line range hint
1-124
: Consider breaking down the template for better maintainabilityWhile the template follows good practices with semantic HTML and consistent patterns, its complexity suggests some potential improvements:
Consider extracting complex switch cases into separate components, particularly for:
- Programming exercise related cases
- Modeling editor case
- Build statistics table
The repeated pattern of
detail-value-{{ detail.title }}
could be moved to a reusable directiveThis would improve:
- Code maintainability
- Component testing
- Reusability
Example refactor for the build statistics case:
// programming-build-statistics.component.ts @Component({ selector: 'jhi-programming-build-statistics', template: ` <table class="table table-striped"> <!-- Move the build statistics table template here --> </table> ` }) export class ProgrammingBuildStatisticsComponent { @Input() statistics!: BuildLogStatistics; }Then in the main template:
@case (DetailType.ProgrammingBuildStatistics) { <dd id="detail-value-{{ detail.title }}"> - <table class="table table-striped"> - <!-- ... table content ... --> - </table> + <jhi-programming-build-statistics [statistics]="detail.data.buildLogStatistics" /> </dd> }src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (2)
786-801
: OptimizecalculateLineCount
function placementThe
calculateLineCount
function insideprocessGitDiffReport
is defined every time the method is called. To improve performance and readability, consider moving this utility function outside ofprocessGitDiffReport
as a private method of the class. This avoids redefining the function on each invocation and makes the code cleaner.Here's how you can refactor the code:
+ private calculateLineCount(entries: { lineCount?: number; previousLineCount?: number }[] = [], key: 'lineCount' | 'previousLineCount'): number { + return entries.map((entry) => entry[key] ?? 0).reduce((sum, count) => sum + count, 0); + } private processGitDiffReport(gitDiffReport: ProgrammingExerciseGitDiffReport | undefined): void { if ( gitDiffReport && (this.programmingExercise.gitDiffReport?.templateRepositoryCommitHash !== gitDiffReport.templateRepositoryCommitHash || this.programmingExercise.gitDiffReport?.solutionRepositoryCommitHash !== gitDiffReport.solutionRepositoryCommitHash) ) { this.programmingExercise.gitDiffReport = gitDiffReport; gitDiffReport.programmingExercise = this.programmingExercise; - const calculateLineCount = (entries: { lineCount?: number; previousLineCount?: number }[] = [], key: 'lineCount' | 'previousLineCount') => - entries.map((entry) => entry[key] ?? 0).reduce((sum, count) => sum + count, 0); - this.addedLineCount = calculateLineCount(gitDiffReport.entries, 'lineCount'); - this.removedLineCount = calculateLineCount(gitDiffReport.entries, 'previousLineCount'); + this.addedLineCount = this.calculateLineCount(gitDiffReport.entries, 'lineCount'); + this.removedLineCount = this.calculateLineCount(gitDiffReport.entries, 'previousLineCount'); } }
241-252
: Ensure comprehensive error handling in the observable chainWhile you have added an error handler in the
subscribe
block starting at line 241, consider handling errors at each asynchronous operation if they require specific error handling logic. This can provide more granular control over error messages and improve the user experience by displaying more relevant feedback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/detail-overview-list/detail-overview-list.component.html
(1 hunks)src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts
(3 hunks)src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/detail-overview-list/detail-overview-list.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/detail-overview-list/detail-overview-list.component.ts (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
🔇 Additional comments (4)
src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts (3)
1-1
: LGTM! Modern Angular dependency injection pattern applied.
The switch to using inject()
function and making services readonly is a good improvement that:
- Enables better tree-shaking
- Prevents accidental service reassignment
- Reduces boilerplate code by removing the constructor
Also applies to: 55-57
Line range hint 70-77
: LGTM! Proper handling of the new input signal.
The code correctly adapts to the new input signal syntax by using sections() as a function call. The mapping logic is clean and efficient.
Line range hint 78-91
: Verify subscription cleanup.
The subscription cleanup looks good, but let's verify there are no other subscriptions in the component that might need cleanup.
✅ Verification successful
Let me analyze the subscriptions in the component based on the search results:
-
There's a
profileSubscription
that is:- Created:
this.profileSubscription = this.profileService.getProfileInfo().subscribe(...)
- Cleaned up:
this.profileSubscription?.unsubscribe();
- Created:
-
There's a subscription in
downloadApollonDiagramAsPDf
method:- This is a one-time subscription that completes after the PDF conversion
- It doesn't need cleanup as it's not stored and will auto-complete
Let me verify if there are any other potential subscriptions we might have missed by checking for observables or async patterns.
Subscription cleanup is properly implemented
The component has only two subscriptions:
- A stored
profileSubscription
that is properly cleaned up in the component's destruction - A one-time subscription in
downloadApollonDiagramAsPDf
that auto-completes and doesn't require cleanup
No other subscriptions or potential memory leaks were found in the component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any missed subscriptions in the component
rg -A 2 "subscribe\(" "src/main/webapp/app/detail-overview-list"
Length of output: 1250
Script:
#!/bin/bash
# Search for Observable imports and declarations
rg "Observable|Subject|BehaviorSubject" "src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts"
# Search for async pipe usage which might indicate subscriptions
ast-grep --pattern 'async'
Length of output: 833
src/main/webapp/app/detail-overview-list/detail-overview-list.component.html (1)
4-4
: Verify reactive implementation of sections()
The change from sections
to sections()
suggests a reactive pattern implementation. This aligns with modern Angular practices, but we should ensure:
- The implementation in the component uses either Signals or RxJS properly
- Memory management (unsubscribe/cleanup) is handled correctly
✅ Verification successful
Let me try a different search pattern to find the sections implementation.
Change to sections() is correct and properly implemented
The code shows that sections
is implemented as an Angular input using the new input syntax (sections = input.required<DetailOverviewSection[]>()
). The function call syntax in the template is correct as this is how required inputs are accessed in the new Angular syntax. The component also properly handles cleanup with ngOnDestroy
unsubscribing from profileSubscription
.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the sections implementation in the component
ast-grep --pattern 'class DetailOverviewListComponent {
$$$
sections$_() {
$$$
}
$$$
}'
# Check for proper cleanup
rg -A 5 'ngOnDestroy|takeUntil|pipe' src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts
Length of output: 416
Script:
#!/bin/bash
# Search for sections property/method in the component file
rg -A 10 "sections" src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts
# Check if it's using signals
rg "signal|computed|effect" src/main/webapp/app/detail-overview-list/detail-overview-list.component.ts
Length of output: 1125
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
Outdated
Show resolved
Hide resolved
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: 1
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (2)
235-237
: Improve type safety for build log statistics.The type assertion
[] as BuildLogStatisticsDTO
might not be safe. Consider using a proper empty array type.- this.programmingExercise.isAtLeastEditor ? this.programmingExerciseService.getBuildLogStatistics(exerciseId!) : of([] as BuildLogStatisticsDTO), + this.programmingExercise.isAtLeastEditor ? this.programmingExerciseService.getBuildLogStatistics(exerciseId!) : of<BuildLogStatisticsDTO[]>([]),
798-800
: Improve type safety in line count calculation helper.The helper function uses generic parameter names. Consider using more specific types and names.
- const calculateLineCount = (entries: { lineCount?: number; previousLineCount?: number }[] = [], key: 'lineCount' | 'previousLineCount') => + const calculateLineCount = (diffEntries: { lineCount?: number; previousLineCount?: number }[] = [], countType: 'lineCount' | 'previousLineCount'): number => - entries.map((entry) => entry[key] ?? 0).reduce((sum, count) => sum + count, 0); + diffEntries.map((entry) => entry[countType] ?? 0).reduce((sum, count) => sum + count, 0);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (2)
190-196
: LGTM! Improved error handling and state management.
The refactored subscription chain properly handles state updates and error cases.
789-804
: LGTM! Well-structured Git diff report processing.
The method properly handles undefined input and efficiently calculates line counts.
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
Outdated
Show resolved
Hide resolved
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
🧹 Outside diff range and nitpick comments (3)
src/test/javascript/spec/component/detail-overview-list.component.spec.ts (3)
Line range hint
59-65
: Enhance subscription cleanup verification.While the test correctly uses
setInput
for input properties and verifies basic cleanup, consider strengthening the subscription cleanup test.-expect(component.profileSubscription?.closed).toBeTruthy(); +expect(component.profileSubscription?.closed).toBeTrue();This change aligns with the coding guidelines for boolean expectations using
toBeTrue()
instead oftoBeTruthy()
.
Line range hint
70-93
: Improve test robustness and assertion specificity.The test has several areas for improvement:
- Avoid type assertions with
as any
- Use more specific DOM element queries
- Use more specific existence assertions
Apply these improvements:
-null as any as Detail, +null as unknown as Detail, -expect(titleDetailTitle).toBeDefined(); -expect(titleDetailValue).toBeDefined(); +expect(titleDetailTitle).not.toBeNull(); +expect(titleDetailValue).not.toBeNull(); -fixture.nativeElement.querySelectorAll('dt[id^=detail-title]') +fixture.nativeElement.querySelectorAll('[data-testid="detail-title"]')Also consider adding test data attributes to your component template:
<dt [attr.data-testid]="'detail-title-' + detail.title">
Line range hint
1-108
: Consider adding tests for additional scenarios.While the current test coverage is good, consider adding tests for:
- Input validation when sections are empty
- Edge cases with malformed section data
- Accessibility attributes in the rendered output
Example test structure:
it('should handle empty sections gracefully', () => { fixture.componentRef.setInput('sections', []); fixture.detectChanges(); expect(fixture.nativeElement.querySelector('[data-testid="no-sections"]')).not.toBeNull(); }); it('should validate section structure', () => { fixture.componentRef.setInput('sections', [{ headline: 'invalid' }]); fixture.detectChanges(); // Verify error handling or fallback behavior }); it('should include proper aria labels', () => { fixture.componentRef.setInput('sections', sections); fixture.detectChanges(); const headings = fixture.nativeElement.querySelectorAll('[role="heading"]'); expect(headings.length).toBeGreaterThan(0); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/detail-overview-list.component.spec.ts
(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/detail-overview-list.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}}
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (3)
60-61
: LGTM: Imports are correctly added.
The new imports for RxJS tap
operator and ProgrammingExerciseGitDiffReport
model are necessary for the refactored code.
790-805
: LGTM: Well-structured Git diff report processing.
The implementation:
- Properly handles undefined input
- Efficiently calculates line counts using functional programming
- Avoids unnecessary updates by comparing commit hashes
808-810
: Skip: Error handling comment already exists.
A previous review comment already addresses the need for error handling in this code segment.
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts
Outdated
Show resolved
Hide resolved
Resolved in 810b717
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.
Reapprove latest changes
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.
Code 👍
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.
Re-approve
@magaupp #9785 (comment) Do you think we should create an issue for it once the PR is merged? (Beforehand it might be confusing as there are way more requests send out on the current develop state?) |
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.
I don't think it's performance critical. Nothing could wait on it because it's always canceled before it finishes. |
…lls-in-details-view
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.
Great improvements, thank you 👍
Checklist
General
Client
Motivation and Context
The programming exercise details page sent unnecessary requests to the server, which was caused by poor coordination of subscriptions in
programming-exercise-detail.component.ts,
leading to multiple instantiations and renderings of subcomponents.Self-contained components (e.g.
programming-exercise-instructions.component.ts
andprogramming-exercise-repository-and-build-plan-details.component.ts
), therefore, sent multiple requests to the server to retrieve unchanged information (initialized up to 3 times), in other words, unnecessary requests.Description
detail-overview-list.component.ts
getExerciseDetails()
is only called once after all subscriptions are finished onInit, preventing unnecessary rendering steps (this is ensured by usingswitchMap()
loadGitDiffReport()
to be able to useprocessGitDiffReport()
it inswitchMap()
loadGitDiffReport()
to reduce code duplicationSteps for Testing
Prerequisites:
test-cases
,repository-checkout-directories
- will be send out twice right now and will not be addressed further in this PR (still 2 requests less than before),details
- exception: it is okay fortime
to be there multiple times)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
Duplicated requests before the fix ⚡
With the fix ✅
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
DetailOverviewListComponent
andProgrammingExerciseDetailComponent
to align with new input handling and participation changes.Refactor