Programming exercises: Improve error handling when the diff report cannot be generated
#10034
Conversation
github-actions
bot
added
the
client
magaupp
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
🧹 Nitpick comments (3)
src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.spec.ts (1)
224-232: Consider enhancing the error handling test coverage.While the test case correctly verifies basic error handling, consider adding assertions to:
- Verify the specific error message being displayed
- Ensure that diff report UI elements are not shown when an error occurs
it('should create detail sections after getDiffReport error', () => { const errorSpy = jest.spyOn(alertService, 'error'); gitDiffReportStub.mockReturnValue(throwError(() => new HttpErrorResponse({ status: 500 }))); comp.ngOnInit(); expect(errorSpy).toHaveBeenCalledOnce(); + expect(errorSpy).toHaveBeenCalledWith('artemisApp.programmingExercise.diffReportError'); expect(comp.exerciseDetailSections).toBeDefined(); + expect(comp.addedLineCount).toBeUndefined(); + expect(comp.removedLineCount).toBeUndefined(); });src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (2)
Line range hint
799-813: Consider extracting the line count calculation logic.The line count calculation could be extracted into a separate private method for better maintainability and reusability.
private processGitDiffReport(gitDiffReport: ProgrammingExerciseGitDiffReport | undefined): void { const isGitDiffReportUpdated = gitDiffReport && (this.programmingExercise.gitDiffReport?.templateRepositoryCommitHash !== gitDiffReport.templateRepositoryCommitHash || this.programmingExercise.gitDiffReport?.solutionRepositoryCommitHash !== gitDiffReport.solutionRepositoryCommitHash); if (isGitDiffReportUpdated) { 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'); } } + +private calculateLineCount(entries: { lineCount?: number; previousLineCount?: number }[] = [], key: 'lineCount' | 'previousLineCount'): number { + return entries.map((entry) => entry[key] ?? 0).reduce((sum, count) => sum + count, 0); +}
828-834: Consider adding debounce to prevent rapid UI updates.The method could benefit from debouncing to prevent performance issues when called frequently in succession.
+private readonly DEBOUNCE_TIME = 300; // ms +private updateDetailSectionsSubject = new Subject<void>(); + +private initializeUpdateDetailSections(): void { + this.updateDetailSectionsSubject + .pipe( + debounceTime(this.DEBOUNCE_TIME), + takeUntil(this.destroy$) + ) + .subscribe(() => { + this.exerciseDetailSections = this.getExerciseDetails(); + }); +} + /** * <strong>BE CAREFUL WHEN CALLING THIS METHOD!</strong><br> * Warnings of {@link getExerciseDetails} apply. */ private updateDetailSections(): void { - this.exerciseDetailSections = this.getExerciseDetails(); + this.updateDetailSectionsSubject.next(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts(6 hunks)src/main/webapp/i18n/de/programmingExercise.json(1 hunks)src/main/webapp/i18n/en/programmingExercise.json(1 hunks)src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.spec.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/i18n/de/programmingExercise.json
🧰 Additional context used
📓 Path-based instructions (2)
src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.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}}
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
🔇 Additional comments (5)
src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.spec.ts (1)
28-28: LGTM!
The addition of HttpErrorResponse import is appropriate for testing error handling scenarios.
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (3)
60-60: LGTM!
The addition of catchError operator is appropriate for handling errors in RxJS streams.
468-478: LGTM!
Good defensive programming by checking both addedLineCount and removedLineCount before displaying the diff report. This prevents showing incomplete or invalid reports to users.
817-824: LGTM!
The error handling is well-implemented with appropriate user feedback through the alert service.
src/main/webapp/i18n/en/programmingExercise.json (1)
219-219: LGTM!
The error message is clear, concise, and follows the existing translation patterns.
magaupp
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Changes affecting Programming Exercises
Motivation and Context
A recent JGit issue caused the ProgrammingExerciseDetailComponent to fail rendering its sections. See #10029.
Any internal server error while fetching the diff reports would cause the component to crash.
While the JGit issue has been mitigated, the component should be more robust to server failures.
Description
catchError()is added to the RxJS pipeline to catch any errors produced bygetDiffReport()and continue with the pipeline.The diff report section is hidden in case of errors.
Steps for Testing
Prerequisites:
The server should work correctly. Test that the normal case works correctly.
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
Client
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests