Plagiarism checks: Same file comparison view feature
#9789
Conversation
github-actions
bot
added
the
client
AjayvirS
changed the title
AjayvirS
changed the title
AjayvirS
changed the title
Plagiarism checks: same file comparison view
AjayvirS
changed the title
Plagiarism checks: same file comparison viewPlagiarism checks: Same file comparison view
WalkthroughThe changes in this pull request involve enhancements to the plagiarism split view component in the web application. Key modifications include the addition of new input properties to various components, the introduction of subjects for managing file selection and visibility, and the implementation of lifecycle hooks for resource management. A new button for toggling file locking functionality is also added. Overall, these updates aim to improve interactivity and data handling within the plagiarism checking feature. 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: 3
🧹 Outside diff range and nitpick comments (12)
src/main/webapp/app/exercises/shared/plagiarism/types/text/TextPlagiarismFileElement.ts (1)
1-1: Consider using path alias for better maintainabilityThe import path is quite deep. Consider adding a path alias in the tsconfig.json for the plagiarism-related components to make imports more maintainable.
Example configuration in tsconfig.json:
{ "compilerOptions": { "paths": { "@plagiarism/*": ["app/exercises/shared/plagiarism/*"] } } }Then the import could be simplified to:
import { FileWithHasMatch } from '@plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component';src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts (1)
22-25: Consider enhancing the property declaration.While the implementation is functionally correct, consider these improvements for better maintainability and clarity:
- Add an explicit access modifier
- Consider making it readonly if it's only modified through events
- Enhance the documentation to describe how this property affects child components
/** - * Boolean to determine whether file locking is enabled. + * Controls the file locking functionality across child components. + * When enabled, it synchronizes file navigation between split views. + * Modified through isLockFilesEnabledChange events from plagiarism-header. */ - isLockFilesEnabled: boolean = false; + public readonly isLockFilesEnabled = false;src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.html (1)
1-7: Remove unnecessary 'this.' from template bindingThe template binding syntax
this.isLockFilesEnabledis not recommended in Angular templates as component properties are automatically bound to the component's context.Apply this diff:
<jhi-split-pane-header [files]="files" - [isLockFilesEnabled]="this.isLockFilesEnabled" + [isLockFilesEnabled]="isLockFilesEnabled" [fileSelectedSubject]="fileSelectedSubject" (selectFile)="handleFileSelect($event)" studentLogin="{{ plagiarismSubmission?.studentLogin }}" />src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html (1)
26-29: LGTM with accessibility enhancement suggestions.The implementation looks good with proper event binding, state management, and localization. Consider these accessibility improvements:
- Add
type="button"attribute to prevent form submission- Add
aria-pressedattribute to indicate toggle stateApply this diff to enhance accessibility:
- <button class="btn btn-outline-secondary btn-sm" (click)="toggleLockFiles()" [class.active]="isLockFilesEnabled" data-qa="lock-files-toggle-button"> + <button + type="button" + class="btn btn-outline-secondary btn-sm" + (click)="toggleLockFiles()" + [class.active]="isLockFilesEnabled" + [attr.aria-pressed]="isLockFilesEnabled" + data-qa="lock-files-toggle-button">src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (2)
19-31: Add access modifier to subscription property.The
fileSelectSubscriptionproperty should have an access modifier for better encapsulation. Also, consider initializing it as undefined to make the type more precise.- fileSelectSubscription: Subscription; + private fileSelectSubscription?: Subscription;
35-41: Enhance subscription handling with error handling and completion.The subscription should handle potential errors and completion. Also, consider using RxJS operators to handle the file existence check more elegantly.
ngOnInit(): void { - this.fileSelectSubscription = this.fileSelectedSubject.subscribe((val) => { - if (val.file && this.isLockFilesEnabled) { - this.handleFileSelectWithoutPropagation(val.file, val.idx); - } - }); + this.fileSelectSubscription = this.fileSelectedSubject.pipe( + filter(val => val.file !== null && val.file !== undefined && this.isLockFilesEnabled) + ).subscribe({ + next: (val) => this.handleFileSelectWithoutPropagation(val.file, val.idx), + error: (error) => console.error('Error in file selection:', error) + }); }src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts (2)
23-29: Add explicit type for EventEmitter.While the code is functionally correct, consider adding an explicit type for better type safety:
-@Output() isLockFilesEnabledChange = new EventEmitter<boolean>(); +@Output() isLockFilesEnabledChange: EventEmitter<boolean> = new EventEmitter<boolean>();
Line range hint
13-19: Add component-level documentation for the locking feature.Consider adding a class-level JSDoc comment to explain the purpose and usage of the file locking feature in the context of plagiarism detection:
/** * Component that handles the header section of the plagiarism detection view. * Provides controls for: * - Confirming or denying plagiarism * - Expanding/resetting split panes * - Toggling file locking for synchronized comparison view */ @Component({src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts (2)
31-31: Add JSDoc documentation for the input property.Please add JSDoc documentation to explain the purpose and behavior of the
isLockFilesEnabledproperty. This will help other developers understand when and how to use this feature.Example:
/** Controls whether files can be locked in the split view for comparison. */ @Input() isLockFilesEnabled = false;
Line range hint
1-180: Consider architectural improvements for maintainability.The component could benefit from the following architectural improvements:
- Extract the file comparison logic into a dedicated service to improve testability and reusability
- Use
OnDestroyinterface to properly handle cleanup of subscriptions and subjects- Consider using
BehaviorSubjectforfileSelectedSubjectif initial state is important- Add comprehensive documentation for the file locking feature
Would you like assistance in implementing any of these improvements?
src/test/javascript/spec/component/plagiarism/plagiarism-split-view.component.spec.ts (1)
173-173: LGTM! Consider enhancing test coverage.The change to use a strongly-typed mock return value
Map<string, FromToElement[]>improves type safety. However, consider adding test cases to verify the structure and content of the mapped elements.Add test cases to verify:
- The structure of the returned map matches the expected type
- Edge cases with empty or malformed
FromToElementarrays- Error handling when mapping fails
Example test case:
it('should return correctly structured map from mapMatchesToElements', () => { const matches = [{ start: 0, length: 2 }] as SimpleMatch[]; const submission = { elements: [ { id: 1, file: 'test.txt', column: 1, line: 1 }, { id: 2, file: 'test.txt', column: 2, line: 2 } ] } as PlagiarismSubmission<TextSubmissionElement>; const result = comp.mapMatchesToElements(matches, submission); expect(result).toBeInstanceOf(Map); expect(result.get('none')).toBeInstanceOf(Array); expect(result.get('none')?.[0]).toBeInstanceOf(FromToElement); expect(result.get('none')?.[0].from).toEqual(submission.elements[0]); expect(result.get('none')?.[0].to).toEqual(submission.elements[1]); });src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts (1)
31-32: LGTM! Input properties are well-defined.The new input properties follow Angular's style guide:
fileSelectedSubjectuses the definite assignment operator correctlyisLockFilesEnabledhas an appropriate default value- Both use camelCase naming convention
However, consider adding JSDoc comments to document these properties' purposes, similar to other properties in this component.
Apply this diff to improve documentation:
+ /** + * Subject for broadcasting selected file changes. + */ @Input() fileSelectedSubject!: Subject<TextPlagiarismFileElement>; + /** + * Flag indicating whether file locking functionality is enabled. + */ @Input() isLockFilesEnabled = false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.html(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts(4 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.html(2 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts(4 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts(4 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.html(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts(2 hunks)src/main/webapp/app/exercises/shared/plagiarism/types/text/TextPlagiarismFileElement.ts(1 hunks)src/main/webapp/i18n/de/plagiarism.json(1 hunks)src/main/webapp/i18n/en/plagiarism.json(1 hunks)src/test/javascript/spec/component/plagiarism/plagiarism-split-view.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.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/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.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/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.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/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.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/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts (1)
src/main/webapp/app/exercises/shared/plagiarism/types/text/TextPlagiarismFileElement.ts (1)
src/main/webapp/i18n/de/plagiarism.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/test/javascript/spec/component/plagiarism/plagiarism-split-view.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 (18)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.html (3)
1-1: LGTM! Follows new Angular syntax guidelines.
The template correctly uses the new @if syntax instead of the older *ngIf.
3-4: LGTM! Event binding for file locking is properly implemented.
The jhi-plagiarism-header component correctly implements the output event binding (isLockFilesEnabledChange) to update the parent component's state.
Let's verify the event emitter declaration in the header component:
✅ Verification successful
Event binding implementation verified successfully
The isLockFilesEnabledChange is properly declared as an @Output() EventEmitter of type boolean in the PlagiarismHeaderComponent, matching the event binding usage in the template. The implementation is correct and follows Angular's best practices for component communication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the event emitter declaration in the header component
# Expected: Should find @Output() isLockFilesEnabledChange declaration
ast-grep --pattern 'class PlagiarismHeaderComponent {
$$$
@Output() isLockFilesEnabledChange
$$$
}'
Length of output: 404
Script:
#!/bin/bash
# Let's try a different approach to find the component file first
fd "plagiarism-header.component.ts" --type f
# Then we'll use ripgrep to search for the event emitter
rg "@Output\(\)\s+isLockFilesEnabledChange" -l
Length of output: 287
Script:
#!/bin/bash
# Let's examine the actual implementation to verify the event emitter type and usage
rg -A 5 "@Output\(\)\s+isLockFilesEnabledChange" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts
Length of output: 407
6-7: LGTM! Input property binding for file locking is properly implemented.
The jhi-plagiarism-split-view component correctly receives the isLockFilesEnabled state through input binding.
Let's verify the input property declaration in the split view component:
✅ Verification successful
Input property binding for file locking is correctly implemented and used
The isLockFilesEnabled input property is properly declared in the PlagiarismSplitViewComponent with a default value of false, confirming that the property binding in the template is valid and working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the input property declaration in the split view component
# Expected: Should find @Input() isLockFilesEnabled declaration
ast-grep --pattern 'class PlagiarismSplitViewComponent {
$$$
@Input() isLockFilesEnabled
$$$
}'
Length of output: 404
Script:
#!/bin/bash
# Let's try a different approach to find the component file first
fd "plagiarism-split-view.component.ts"
# Then we'll use ripgrep to search for the input property
rg "@Input\(\)\s+isLockFilesEnabled" -l
Length of output: 539
Script:
#!/bin/bash
# Let's check the actual implementation of isLockFilesEnabled in these components
rg "@Input\(\)\s+isLockFilesEnabled" -A 2 src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts
# Also check how it's being used in the component
rg "isLockFilesEnabled" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts
Length of output: 437
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.ts (1)
21-25: Verify template bindings and component integration.
Let's ensure that the template properly connects this property with the child components mentioned in the summary.
✅ Verification successful
Property is correctly integrated with template and child components
The verification confirms proper implementation:
- Template correctly binds
isLockFilesEnabledbetween parent and child components PlagiarismHeaderComponenthas matching@Output() isLockFilesEnabledChangewith toggle functionalityisLockFilesEnabledis passed as input toPlagiarismSplitViewComponent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify template bindings and component integration
# Check template bindings in the component's template
echo "Checking template bindings..."
rg -A 5 "isLockFilesEnabled" --glob "src/main/webapp/app/exercises/shared/plagiarism/plagiarism-details/plagiarism-details.component.html"
# Check if child components are properly typed to handle this property
echo "Checking child component interfaces..."
ast-grep --pattern 'class PlagiarismHeaderComponent {
$$$
@Output() isLockFilesEnabledChange
$$$
}'
ast-grep --pattern 'class PlagiarismSplitViewComponent {
$$$
@Input() isLockFilesEnabled
$$$
}'
Length of output: 12277
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.html (1)
Line range hint 8-40: LGTM! Well-structured conditional blocks with proper error handling
The implementation:
- Correctly uses the new Angular @if syntax
- Provides clear user feedback for different states
- Follows accessibility best practices
- Properly implements internationalization
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.html (2)
8-15: LGTM! Correct usage of new Angular syntax and property bindings.
The component correctly uses the new @if syntax and properly implements the new input properties for file selection and locking functionality.
30-34: Verify the synchronized file selection behavior.
While the implementation is correct, both viewers share the same fileSelectedSubject. Please verify that this synchronization is intentional and properly handled in the component logic to avoid confusing user interactions.
✅ Verification successful
Based on the shell script results, I can now generate the final response as I have found clear evidence of how the fileSelectedSubject is handled in the codebase:
File selection synchronization is intentionally designed and properly handled
The synchronization between both viewers through fileSelectedSubject is a well-implemented feature:
- The
split-pane-headercomponent handles file selection and propagates it throughfileSelectedSubjectwith proper indexing to identify the source viewer - The
text-submission-viewercomponent subscribes to these events and only reacts whenisLockFilesEnabledis true - The implementation includes safeguards like
handleFileSelectWithoutPropagationto prevent infinite loops
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how fileSelectedSubject is handled in the component
# Look for fileSelectedSubject subscription handling
ast-grep --pattern 'fileSelectedSubject.subscribe($$$)'
# Check for any methods that might handle file selection
rg -A 5 "fileSelected|handleFile|onFileSelect"
Length of output: 30771
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.html (1)
28-28: Verify translation keys exist.
Please ensure that the translation keys artemisApp.plagiarism.unlockSync and artemisApp.plagiarism.lockSync are defined in the localization files.
✅ Verification successful
Translation keys are properly defined
Both translation keys artemisApp.plagiarism.lockSync and artemisApp.plagiarism.unlockSync are correctly defined in both English and German translation files:
- English:
lockSync: "Lock Sync"unlockSync: "Unlock Sync"
- German:
lockSync: "Sync sperren"unlockSync: "Sync entsperren"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the translation keys exist in the localization files
# Search for the translation keys in the i18n files
echo "Checking English translations..."
rg "artemisApp\.plagiarism\.(un)?lockSync" src/main/webapp/i18n/en/*.json
echo "Checking German translations..."
rg "artemisApp\.plagiarism\.(un)?lockSync" src/main/webapp/i18n/de/*.json
Length of output: 13851
Script:
#!/bin/bash
# Let's check the specific translation files for plagiarism-related keys
echo "Checking English plagiarism translations..."
cat src/main/webapp/i18n/en/plagiarism.json
echo -e "\nChecking German plagiarism translations..."
cat src/main/webapp/i18n/de/plagiarism.json
Length of output: 21082
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (3)
1-4: LGTM! Import statements and type declarations are well-organized.
The imports follow the Angular style guide and are properly organized by category (Angular core, third-party, and application imports).
Line range hint 69-90: LGTM! File handling methods are well-implemented and documented.
The methods follow single responsibility principle and have clear documentation. The separation between propagating and non-propagating file selection is well thought out.
Line range hint 19-90: Verify subscription handling in related components.
The component handles its own subscription cleanup well, but we should verify that related components using fileSelectedSubject also properly handle their subscriptions to prevent memory leaks.
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-header/plagiarism-header.component.ts (2)
1-1: LGTM! Import statements follow Angular guidelines.
The new imports are properly organized and follow Angular's style guide recommendations.
Also applies to: 11-11
86-92: 🛠️ Refactor suggestion
Consider memory leak prevention and add unit tests.
While the implementation is clean, consider these improvements:
- Ensure proper cleanup of the EventEmitter in the component's OnDestroy lifecycle hook
- Add unit tests to verify the toggle behavior and event emission
Add the following cleanup code:
export class PlagiarismHeaderComponent implements OnDestroy {
// ... existing code ...
ngOnDestroy() {
this.isLockFilesEnabledChange.complete();
}
}Let's check for existing tests:
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts (1)
13-13: LGTM!
The import follows Angular style guide conventions and is correctly placed with other imports.
src/main/webapp/i18n/en/plagiarism.json (1)
7-8: LGTM! Please ensure German translations are added.
The new entries are clear, concise, and properly formatted. They follow the existing naming patterns and maintain consistency with other translations.
Let's verify that corresponding German translations are added:
✅ Verification successful
German translations are already in place and accurate
The German translations for both "lockSync" and "unlockSync" are properly implemented:
- "Sync sperren" (Lock Sync)
- "Sync entsperren" (Unlock Sync)
The translations are semantically correct and follow German language conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if German translations exist for the new entries
# Check for German translations
rg -A 2 '"lockSync"|"unlockSync"' src/main/webapp/i18n/de/plagiarism.json
Length of output: 565
src/main/webapp/i18n/de/plagiarism.json (1)
7-8: LGTM! The German translations are accurate and consistent.
The new translations for sync locking functionality are:
- Grammatically correct
- Use appropriate technical terminology
- Follow the imperative form suitable for action buttons
- Consistent with the existing translation style
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts (2)
15-16: LGTM! Imports are properly organized.
The new imports follow Angular style guide and are necessary for the new functionality.
31-32: Verify the usage of new input properties.
The new input properties don't have any handlers in the component's logic. Let's verify:
- Is
fileSelectedSubjectonly used for output to parent components? - Is
isLockFilesEnabledonly used for UI control in the template?
✅ Verification successful
Input properties are correctly used as template bindings only
Both properties are used appropriately as template bindings without requiring component logic handlers:
fileSelectedSubjectis passed down to a child component via template bindingisLockFilesEnabledis used for UI control via template binding
No changes are needed as this is a valid pattern for input properties that only affect template rendering or are passed to child components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of new input properties in the component and its template
# Check if fileSelectedSubject is used in component logic
echo "Checking fileSelectedSubject usage in component logic..."
rg -l "fileSelectedSubject" src/main/webapp/app/exercises/shared/plagiarism/
# Check if isLockFilesEnabled is used in component logic
echo "Checking isLockFilesEnabled usage in component logic..."
rg -l "isLockFilesEnabled" src/main/webapp/app/exercises/shared/plagiarism/
# Check template usage
echo "Checking template usage..."
fd -e html . src/main/webapp/app/exercises/shared/plagiarism/ -x rg -l "fileSelectedSubject|isLockFilesEnabled" {}
Length of output: 2594
Script:
#!/bin/bash
# Let's check the actual usage of these properties in the component and related files
# Check how fileSelectedSubject is used in the component and its template
echo "=== fileSelectedSubject usage in text-submission-viewer ==="
rg "fileSelectedSubject" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts -A 3
echo "\n=== fileSelectedSubject in template ==="
rg "fileSelectedSubject" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.html
# Check how isLockFilesEnabled is used
echo "\n=== isLockFilesEnabled usage in text-submission-viewer ==="
rg "isLockFilesEnabled" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.ts -A 3
echo "\n=== isLockFilesEnabled in template ==="
rg "isLockFilesEnabled" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/text-submission-viewer/text-submission-viewer.component.html
# Check if component implements OnChanges and how
echo "\n=== OnChanges implementation ==="
ast-grep --pattern 'class TextSubmissionViewerComponent implements $_ {
$$$
ngOnChanges($_) {
$$$
}
$$$
}'
Length of output: 1721
src/main/webapp/app/exercises/shared/plagiarism/types/text/TextPlagiarismFileElement.ts
Outdated
Show resolved
Hide resolved
...app/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts
Outdated
Show resolved
Hide resolved
...app/app/exercises/shared/plagiarism/plagiarism-split-view/plagiarism-split-view.component.ts
Show resolved
Hide resolved
…ile-comparison-view' into feature/plagiarism-checks/same-file-comparison-view
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts (3)
Line range hint
48-57: Improve test specificity using recommended assertions.The test cases for button disabled states can be improved by using the recommended assertion patterns from our coding guidelines.
Update the assertions to use the recommended patterns:
expect(button.disabled).toBeTrue();This assertion already follows our guidelines for boolean checks using
toBeTrue(). However, consider adding more specific assertions to validate the complete state:expect(button).toContainEntries([ ['disabled', true], ['label', expect.any(String) ]);Also applies to: 74-83
Line range hint
85-96: Improve spy assertions using recommended patterns.The test case for modal interaction can be improved using our recommended spy assertion patterns.
Update the spy assertions to use the recommended patterns:
-expect(comp.updatePlagiarismStatus).not.toHaveBeenCalled(); -expect(modalSpy).toHaveBeenCalledOnce(); +expect(comp.updatePlagiarismStatus).not.toHaveBeenCalled(); +expect(modalSpy).toHaveBeenCalledExactlyOnceWith(expect.any(Object));
Line range hint
18-45: Add test coverage for isLockFilesEnabledChange functionality.The AI summary indicates that the component has a new
@OutputpropertyisLockFilesEnabledChange, but there are no tests covering this functionality. Please add test cases to verify:
- The output event emission
- Integration with file locking state changes
Here's a suggested test case structure:
it('should emit isLockFilesEnabledChange when toggling file lock', () => { const emitSpy = jest.spyOn(comp.isLockFilesEnabledChange, 'emit'); comp.toggleFileLock(); expect(emitSpy).toHaveBeenCalledExactlyOnceWith(expect.any(Boolean)); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/plagiarism/plagiarism-header.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/test/javascript/spec/component/plagiarism/plagiarism-header.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/component/plagiarism/split-pane-header.component.spec.ts (1)
Line range hint
51-126: Add tests for fileSelectedSubject functionalityWhile the existing tests cover the basic component functionality well, we should add tests for the new
fileSelectedSubjectbehavior.Add these test cases:
it('should emit selected file through fileSelectedSubject', () => { // Arrange const selectedFile = new TextPlagiarismFileElement(); let emittedFile: TextPlagiarismFileElement | undefined; comp.fileSelectedSubject.subscribe((file) => { emittedFile = file; }); // Act comp.fileSelectedSubject.next(selectedFile); // Assert expect(emittedFile).toBe(selectedFile); }); it('should handle file selection with locked files', () => { // Arrange comp.isLockFilesEnabled = true; const idx = 1; jest.spyOn(comp.fileSelectedSubject, 'next'); // Act comp.handleFileSelect(files[idx], idx); // Assert expect(comp.fileSelectedSubject.next).toHaveBeenCalledExactlyOnceWith( expect.objectContaining({ file: files[idx].file }) ); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/plagiarism/split-pane-header.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/plagiarism/split-pane-header.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 (1)
src/test/javascript/spec/component/plagiarism/split-pane-header.component.spec.ts (1)
13-14: LGTM: Proper setup of reactive components
The addition of TextPlagiarismFileElement type and Subject from rxjs is well integrated into the component setup.
Also applies to: 47-47
There was a problem hiding this comment.
Please fill in the motivation, description, and steps for testing in the PR description. Is/was there a related issue that is fixed by this PR?
Otherwise, it is really hard to understand if this PR is a new feature or if it fixes an issue (please adapt the PR title accordingly). Without the information I don’t really know what to look out for during a review.
…ile-comparison-view' into feature/plagiarism-checks/same-file-comparison-view
AjayvirS
dismissed stale reviews from kevinfischer4 and coderabbitai[bot]
via
596e0a6
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (1)
Line range hint
1-15: Consider enhancing type safety for FileWithHasMatchThe type definition could benefit from being more strict to prevent potential runtime issues.
-export type FileWithHasMatch = { +export interface FileWithHasMatch { file: string; hasMatch: boolean; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.scss(1 hunks)src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.scss
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (1)
🔇 Additional comments (4)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (4)
58-66: Improve readability of file selection logic
The conditional logic could be more readable using a more functional approach.
private handleLockedFileSelection(file: FileWithHasMatch, idx: number): void {
- const index = this.files[idx]?.file === file.file ? idx : this.getIndexOf(file);
-
- if (index >= 0) {
- this.handleFileSelect(file, index, false);
- } else {
- this.showFiles = false;
- }
+ const index = this.files[idx]?.file === file.file
+ ? idx
+ : this.getIndexOf(file);
+
+ index >= 0
+ ? this.handleFileSelect(file, index, false)
+ : this.showFiles = false;
}19-39: 🛠️ Refactor suggestion
Maintain consistency in input declarations
The component mixes traditional @Input decorators with modern signal-based inputs. For better maintainability and consistency with Angular's latest practices, consider migrating all inputs to signals.
- @Input() files: FileWithHasMatch[];
- @Input() studentLogin: string;
+ files = input<FileWithHasMatch[]>();
+ studentLogin = input<string>();50-56:
Improve subscription error handling and null safety
The subscription lacks error handling and uses non-null assertions which could lead to runtime errors.
private subscribeToFileSelection(): void {
- this.fileSelectSubscription = this.fileSelectedSubject()!.subscribe((textPlagiarismElement) => {
- if (this.isLockFilesEnabled()) {
- this.handleLockedFileSelection(textPlagiarismElement.file, textPlagiarismElement.idx);
- }
- });
+ const subject = this.fileSelectedSubject();
+ if (subject) {
+ this.fileSelectSubscription = subject.subscribe({
+ next: (textPlagiarismElement) => {
+ if (this.isLockFilesEnabled()) {
+ this.handleLockedFileSelection(textPlagiarismElement.file, textPlagiarismElement.idx);
+ }
+ },
+ error: (error) => {
+ console.error('Error in file selection:', error);
+ }
+ });
+ }
}110-114: Verify subscription cleanup
The component properly unsubscribes from all subscriptions in ngOnDestroy, which is good practice for preventing memory leaks.
✅ Verification successful
All subscriptions are properly managed
The component correctly handles all subscriptions:
- All three input Subjects (
fileSelectedSubject,showFilesSubject,dropdownHoverSubject) have corresponding subscriptions (fileSelectSubscription,showFilesSubscription,dropdownHoverSubscription) - All subscriptions are properly cleaned up in the
ngOnDestroymethod - No other subscriptions or observables are found that would require cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all Subject declarations have corresponding cleanup
rg -A 5 "Subject.*=.*input" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts
Length of output: 503
Script:
#!/bin/bash
# Let's verify all subscriptions in the component
rg -A 2 "subscription.*=" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts
# Also check for any other Subject or Observable declarations
rg "(Subject|Observable|subscription)" src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts
Length of output: 1148
...ses/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts
Outdated
Show resolved
Hide resolved
…ile-comparison-view' into feature/plagiarism-checks/same-file-comparison-view
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (1)
🔇 Additional comments (5)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (5)
1-1: Migrate remaining @input properties to signals for consistency
The component mixes traditional @Input decorators with modern signal-based inputs. For better maintainability and consistency with Angular's latest practices, consider migrating all inputs to signals.
Also applies to: 19-25
32-34: LGTM: Proper subscription management implemented
The component correctly manages subscriptions by:
- Declaring subscription properties
- Initializing in ngOnInit
- Cleaning up in ngOnDestroy
Also applies to: 40-44, 110-114
130-138: LGTM: Well-documented event handlers with clear parameters
The event handlers are properly documented and have clear parameter names that indicate their purpose.
Also applies to: 149-157
141-141: LGTM: Well-implemented utility methods
The utility methods are concise and follow best practices:
hasFilesuses nullish coalescing for null safetygetIndexOfis well-documented and uses appropriate array methods
Also applies to: 168-170
159-161:
Improve null safety in triggerMouseEnter
The method uses a non-null assertion which could lead to runtime errors.
Apply this diff:
triggerMouseEnter(file: FileWithHasMatch, idx: number) {
- this.dropdownHoverSubject()!.next({ idx: idx, file: file });
+ const subject = this.dropdownHoverSubject();
+ if (subject) {
+ subject.next({ idx, file });
+ }
}Likely invalid or redundant comment.
...ses/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (2)
38-38: Initialize hoveredFileIndexThe
hoveredFileIndexproperty should be initialized to prevent potential undefined behavior.- hoveredFileIndex: number; + hoveredFileIndex = -1;
141-141: Simplify boolean expressionThe double negation operator can be replaced with a more straightforward expression.
- return !!this.files?.length; + return Boolean(this.files?.length);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/exercises/shared/plagiarism/plagiarism-split-view/split-pane-header/split-pane-header.component.ts (3)
Line range hint 1-19: Consider full migration to signals and standalone component
While the component has started using signals for some inputs, it still uses traditional @Input decorators for others. For consistency and better maintainability:
- Migrate remaining
@Inputproperties to signals - Consider converting to standalone component in a separate PR
Apply this diff to migrate remaining inputs:
- @Input() files: FileWithHasMatch[];
- @Input() studentLogin: string;
+ files = input<FileWithHasMatch[]>();
+ studentLogin = input<string>();72-78: 🛠️ Refactor suggestion
Simplify conditional logic and improve null safety
The subscription method has complex nested conditionals and uses non-null assertions which could be simplified and made safer.
private subscribeToShowFiles(): void {
- this.showFilesSubscription = this.showFilesSubject()?.subscribe((showFiles) => {
- if (this.isLockFilesEnabled()! || (!this.isLockFilesEnabled()! && !showFiles)) {
- this.toggleShowFiles(false, showFiles);
- }
- });
+ const subject = this.showFilesSubject();
+ if (subject) {
+ this.showFilesSubscription = subject.subscribe({
+ next: (showFiles) => {
+ const isLocked = this.isLockFilesEnabled();
+ if (isLocked || (!isLocked && !showFiles)) {
+ this.toggleShowFiles(false, showFiles);
+ }
+ },
+ error: (error) => {
+ console.error('Error in show files subscription:', error);
+ }
+ });
+ }
}130-138: 🛠️ Refactor suggestion
Improve null safety in handleFileSelect
The method uses non-null assertion which could lead to runtime errors.
handleFileSelect(file: FileWithHasMatch, idx: number, propagateChanges: boolean): void {
if (propagateChanges) {
- this.fileSelectedSubject()!.next({ idx: idx, file: file });
+ const subject = this.fileSelectedSubject();
+ if (subject) {
+ subject.next({ idx, file });
+ file.hasMatch = true;
+ }
- file.hasMatch = true;
}
this.activeFileIndex = idx;
this.showFiles = false;
this.selectFile.emit(file.file);
}
sachmii
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
Note: After discussing with @MarkusPaulsen, the issue was extended to improve the UI:
In the majority of cases, there is no point in comparing different files in the plagiarism detection comparison viewer which can actually be misleading if different files contain similar content. This feature enables automatic selection of the file in viewer B when selecting a file from the dropown in viewer A.
(Fixes #5067)
Description
Added a fileSelectedSubject which listens on file selection changes in one instance and emits file selection changes in the other component. If the button for syncing files is enabled, the other instance triggers file selection in its own component.
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
Test Coverage
Screenshots
Updated Functionality:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Style Updates