Development: Update theme switcher to use Angular 18 practices
#9250
Conversation
github-actions
bot
added
the
client
…tion-example # Conflicts: # src/main/webapp/app/shared/markdown-editor/ace-editor/ace-editor.component.ts
…nal-migration-example' into chore/performance/angular-18-signal-migration-example
…tion-example # Conflicts: # src/main/webapp/app/shared/monaco-editor/monaco-editor.component.ts
github-actions
bot
added
lock:artemis-test9
and removed
deploy:artemis-test9
WalkthroughThe pull request includes significant modifications to client theming guidelines, emphasizing the use of global default colors and Bootstrap classes instead of hard-coded color values. It also updates 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (33)
src/main/webapp/app/shared/metis/emoji/emoji.component.html (1)
2-5: Consider extracting common emoji properties to reduce duplication.While the implementation is correct, the emoji properties like
class,size, andbackgroundImageFnare duplicated between themes. Consider refactoring to improve maintainability:+ <!-- Common emoji configuration --> + <ng-template #commonEmoji> + <ngx-emoji + [backgroundImageFn]="utils.EMOJI_SHEET_URL" + class="emoji" + [emoji]="emoji" + [size]="16" + /> + </ng-template> + <!-- Using two instances to 'rerender' if the theme changes --> @if (!dark()) { - <ngx-emoji [backgroundImageFn]="utils.EMOJI_SHEET_URL" class="emoji" [emoji]="emoji" [size]="16" /> + <ng-container [ngTemplateOutlet]="commonEmoji" /> } @else { - <ngx-emoji [imageUrlFn]="utils.singleDarkModeEmojiUrlFn" [backgroundImageFn]="utils.EMOJI_SHEET_URL" class="emoji" [emoji]="emoji" [size]="16" /> + <ngx-emoji + [ngTemplateOutletContext]="commonEmoji" + [imageUrlFn]="utils.singleDarkModeEmojiUrlFn" + /> }src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html (1)
10-11: LGTM! Consider performance optimization.The changes correctly implement Angular's reactive approach using computed properties for theme management. This aligns well with Angular 18 practices and the PR objectives.
Consider memoizing the computed properties if they're used frequently to prevent unnecessary recalculations:
// In the component class dark = computed(() => this.themeService.currentTheme() === 'dark'); singleImageFunction = memoized(() => this.dark() ? darkEmojiUrlFn : lightEmojiUrlFn);src/test/javascript/spec/helpers/mocks/service/mock-theme.service.ts (2)
8-9: Consider adding type validation for Theme enum values.While the implementation is good, consider adding runtime validation to ensure only valid Theme enum values are used.
- private _userPreference = signal<Theme | undefined>(undefined); + private _userPreference = signal<Theme | undefined>(undefined, { + equal: (a, b) => a === b, + validate: (value) => value === undefined || Object.values(Theme).includes(value) + });
15-15: Document the purpose of the empty print method.The empty print method lacks documentation explaining its purpose. If it's needed for specific test scenarios, please add a comment explaining why.
- public print() {} + /** Used for ... [please explain the purpose] */ + public print() {}src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.component.html (1)
5-5: Consider using object identity for tracking instead of index.While tracking by index works, it might not be optimal for performance if the steps array undergoes frequent modifications (reordering, filtering, etc.). Consider tracking by a unique identifier if available.
-@for (step of steps; let i = $index; track i) { +@for (step of steps; track step.id) {src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts (1)
32-34: Consider adding edge cases to initial state verification.The initial state verification looks good and follows the coding guidelines by using
toBeFalse(). However, consider adding tests for edge cases such as:
- Component initialization with dark theme already active
- Multiple rapid theme changes
it('should handle edge cases in theme changes', () => { // Test initialization with dark theme mockThemeService.applyThemePreference(Theme.DARK); fixture = TestBed.createComponent(EmojiPickerComponent); comp = fixture.componentInstance; expect(comp.dark()).toBeTrue(); // Test rapid theme changes mockThemeService.applyThemePreference(Theme.LIGHT); mockThemeService.applyThemePreference(Theme.DARK); mockThemeService.applyThemePreference(Theme.LIGHT); expect(comp.dark()).toBeFalse(); });src/main/webapp/app/core/theme/theme-switch.component.html (1)
23-23: Great performance optimization using [class.dark]!The change from [ngClass] to [class.dark] is a performance improvement, as it's more efficient for single class toggles. The isDarkTheme() method aligns well with the reactive approach.
Consider adding aria-label or aria-pressed attributes to improve accessibility:
- <div class="theme-toggle" [class.dark]="isDarkTheme()" id="theme-toggle"> + <div class="theme-toggle" [class.dark]="isDarkTheme()" id="theme-toggle" + role="button" + [attr.aria-pressed]="isDarkTheme()" + [attr.aria-label]="'artemisApp.theme.toggleTheme' | translate">src/main/webapp/app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts (2)
Line range hint
25-30: Consider optimizing theme subscription performanceThe subscription triggers change detection on every theme change, which might impact performance. Consider using:
- OnPush change detection strategy
- async pipe in template instead of manual subscription
+ @Component({ + selector: 'jhi-progress-bar', + templateUrl: './progress-bar.component.html', + changeDetection: ChangeDetectionStrategy.OnPush + })
Line range hint
43-74: Consider extracting magic numbers into named constantsThe percentage thresholds (50, 100) should be extracted into named constants for better maintainability and reusability.
+ private readonly DANGER_THRESHOLD = 50; + private readonly SUCCESS_THRESHOLD = 100; calculateProgressBarClass(): void { - if (this.percentage < 50) { + if (this.percentage < this.DANGER_THRESHOLD) { this.backgroundColorClass = 'bg-danger'; - } else if (this.percentage < 100) { + } else if (this.percentage < this.SUCCESS_THRESHOLD) { this.backgroundColorClass = 'bg-warning'; } else { this.backgroundColorClass = 'bg-success'; } }src/test/javascript/spec/component/shared/progress-bar.component.spec.ts (2)
59-62: Enhance test specificity and assertionsWhile the test logic is correct, we can improve it according to our test guidelines:
- Use more specific assertions
- Consider testing the intermediate state
Consider refactoring the test like this:
- themeService.applyThemePreference(Theme.DARK); - fixture.detectChanges(); + const themeService = TestBed.inject(ThemeService); + + // Verify initial state + expect(component.isDarkTheme).toBeFalse(); + + // Apply theme change + themeService.applyThemePreference(Theme.DARK); + fixture.detectChanges(); + + // Verify theme state and component response + expect(component.isDarkTheme).toBeTrue(); expect(component.foregroundColorClass).toBe('text-white');
Line range hint
36-43: Consider expanding test coverageThe parameterized test for background colors could be enhanced with additional edge cases:
Consider adding these test cases:
it.each([ { percentage: 49, clazz: 'bg-danger' }, + { percentage: 50, clazz: 'bg-warning' }, { percentage: 99, clazz: 'bg-warning' }, { percentage: 100, clazz: 'bg-success' }, + { percentage: 0, clazz: 'bg-danger' }, + { percentage: -1, clazz: 'bg-danger' }, // Edge case + { percentage: 101, clazz: 'bg-success' }, // Edge case ])('uses correct background', ({ percentage, clazz }) => {src/test/javascript/spec/component/theme/theme-switch.component.spec.ts (2)
19-41: Consider optimizing the test module setup.While the setup is generally good, consider these improvements:
- Move the spy setup into a separate setup function for better organization
- Consider using
MockProviderfromng-mocksfor cleaner service mockingHere's a suggested refactor:
- beforeEach(async () => { - await TestBed.configureTestingModule({ - imports: [ArtemisTestModule, ThemeSwitchComponent, MockDirective(NgbPopover)], - declarations: [], - providers: [ - { provide: LocalStorageService, useClass: MockLocalStorageService }, - { - provide: ThemeService, - useClass: MockThemeService, - }, - ], - }).compileComponents(); + const setupSpies = () => { + openSpy = jest.spyOn(component.popover(), 'open'); + closeSpy = jest.spyOn(component.popover(), 'close'); + }; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [ArtemisTestModule, ThemeSwitchComponent, MockDirective(NgbPopover)], + providers: [ + MockProvider(LocalStorageService, MockLocalStorageService), + MockProvider(ThemeService, MockThemeService), + ], + }).compileComponents(); - themeService = TestBed.inject(ThemeService); - fixture = TestBed.createComponent(ThemeSwitchComponent); - component = fixture.componentInstance; - - openSpy = jest.spyOn(component.popover(), 'open'); - closeSpy = jest.spyOn(component.popover(), 'close'); - - fixture.componentRef.setInput('popoverPlacement', ['bottom']); + themeService = TestBed.inject(ThemeService); + fixture = TestBed.createComponent(ThemeSwitchComponent); + component = fixture.componentInstance; + setupSpies(); + fixture.componentRef.setInput('popoverPlacement', ['bottom']); });
57-67: Consider adding edge cases to OS sync test.While the test covers basic functionality, consider adding tests for:
- System theme changes while synced
- Error scenarios in theme application
Example addition:
it('handles system theme changes when synced', fakeAsync(() => { const applyThemePreferenceSpy = jest.spyOn(themeService, 'applyThemePreference'); component.toggleSynced(); // Simulate system theme change window.matchMedia('(prefers-color-scheme: dark)').matches = true; tick(100); expect(applyThemePreferenceSpy).toHaveBeenCalledWith(undefined); }));src/main/webapp/app/app.module.ts (1)
45-45: Consider impact on initial bundle sizeWhile the migration to a standalone component is good, including it directly in the root module means it's eagerly loaded. Consider if theme switching is essential for initial app load or if it could be lazy loaded.
You could move it to a micro-frontend setup using Angular's built-in lazy loading:
// In app-routing.module.ts { path: '', component: JhiMainComponent, children: [{ path: '', loadComponent: () => import('./core/theme/theme-switch.component') .then(m => m.ThemeSwitchComponent) }] }src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts (1)
24-28: Enhance effect implementation clarity and robustness.While the implementation works, consider these improvements:
- The comment should better explain why we're notifying via subject
- Consider wrapping the subject.next() in try-catch for error handling
effect(() => { - // Apply the theme as soon as the currentTheme changes + // Notify cached PlantUML diagrams to re-render when theme changes this.themeService.currentTheme(); - themeChangedSubject.next(); + try { + themeChangedSubject.next(); + } catch (error) { + console.error('Failed to notify theme change:', error); + } });src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (2)
46-54: Consider adding explicit test for initial theme state.While the theme switching tests are good, consider adding explicit assertions for the initial theme state before any switches occur. This would improve test coverage and documentation of expected behavior.
// Add at the start of the test: expect(themeService.theme()).toBe(Theme.LIGHT);
58-65: Consider adding type assertions for editor instances.While the tests are well-structured, consider adding specific type assertions for the editor instances to improve type safety and documentation.
// For the standalone editor case: expect(editor instanceof monaco.editor.StandaloneCodeEditor).toBeTrue(); // For the diff editor case: expect(editor instanceof monaco.editor.StandaloneDiffEditor).toBeTrue();src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
Line range hint
63-93: Consider enhancing editor accessibility and responsivenessWhile the editor configuration is comprehensive, consider the following improvements:
- Add ARIA labels and roles for better accessibility
- Ensure responsive behavior on different screen sizes
Consider updating the editor configuration:
createStandaloneDiffEditor(domElement: HTMLElement): monaco.editor.IStandaloneDiffEditor { return monaco.editor.createDiffEditor(domElement, { automaticLayout: true, + ariaLabel: 'Code diff editor', glyphMargin: true, minimap: { enabled: false }, readOnly: true, - renderSideBySide: true, + renderSideBySide: window.innerWidth > 768, // Switch to inline diff on mobile scrollBeyondLastLine: false, lineHeight: 16, stickyScroll: { enabled: false, },src/test/javascript/spec/service/theme.service.spec.ts (3)
Line range hint
64-99: Enhance signal testing coverage.While the test correctly verifies the theme changes, consider these improvements for better signal testing:
- Add verification of signal subscription cleanup in the teardown
- Add explicit state transition tests using
toHaveBeenSet()matcher- Consider tracking signal emission count to prevent unnecessary updates
it('applies theme changes correctly', () => { TestBed.flushEffects(); expect(documentGetElementMock).toHaveBeenCalledOnce(); + const themeSpy = jest.fn(); + const subscription = service.currentTheme.subscribe(themeSpy); service.applyThemePreference(Theme.DARK); TestBed.flushEffects(); + expect(themeSpy).toHaveBeenCalledTimes(1); expect(documentGetElementMock).toHaveBeenCalledTimes(2); // ... existing expectations ... + subscription.unsubscribe(); });
Line range hint
112-146: Add tests for OS preference change events.The tests verify initial OS preferences but miss dynamic preference changes. Consider adding:
- Tests for preference change listener registration
- Tests for handling preference change events
- Tests for cleanup of preference change listeners
+ it('responds to OS preference changes', () => { + let preferenceChangeCallback: ((e: MediaQueryListEvent) => void) | undefined; + const addListenerSpy = jest.fn().mockImplementation((type: string, callback: (e: MediaQueryListEvent) => void) => { + preferenceChangeCallback = callback; + }); + + windowMatchMediaSpy = jest.spyOn(window, 'matchMedia').mockImplementation((query) => ({ + media: query, + matches: false, + addEventListener: addListenerSpy, + removeEventListener: jest.fn(), + } as any as MediaQueryList)); + + service.initialize(); + TestBed.flushEffects(); + + expect(addListenerSpy).toHaveBeenCalledWith('change', expect.any(Function)); + + // Simulate preference change + preferenceChangeCallback?.({ matches: true } as MediaQueryListEvent); + TestBed.flushEffects(); + + expect(service.currentTheme()).toBe(Theme.DARK); + });
Line range hint
148-167: Enhance print testing with error cases and theme restoration.While the test covers the basic print flow, consider adding:
- Verification of theme restoration after print errors
- Tests for missing elements
- Tests for print cancellation
+ it('handles print errors gracefully', fakeAsync(() => { + const winSpy = jest.spyOn(window, 'print').mockImplementation(() => { + throw new Error('Print failed'); + }); + const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); + const returnedElement = { rel: 'stylesheet', style: { display: 'block' } }; + const docSpy = jest.spyOn(document, 'getElementById').mockReturnValue(returnedElement as any as HTMLElement); + + service.print(); + tick(250); + + expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Print failed')); + expect(returnedElement.rel).toBe('stylesheet'); // Should restore original state + expect(returnedElement.style.display).toBe('block'); + })); + + it('handles missing elements gracefully', fakeAsync(() => { + const winSpy = jest.spyOn(window, 'print').mockImplementation(); + const docSpy = jest.spyOn(document, 'getElementById').mockReturnValue(null); + + service.print(); + tick(250); + + expect(winSpy).toHaveBeenCalledOnce(); // Should still attempt to print + }));docs/dev/guidelines/client-design.rst (1)
Line range hint
182-207: Update code example to use Angular 18 signals.The code example showing theme subscription uses RxJS observables and manual subscription management. Since this PR is migrating to Angular 18 practices, we should update this example to use signals for better reactivity and reduced boilerplate.
Here's the updated example using signals:
@Component({ selector: 'jhi-my-component', templateUrl: './my-component.component.html', styleUrls: ['my-component.component.scss'], }) -export class MyComponent implements OnInit, OnDestroy { - isDark = false; - themeSubscription: Subscription; +export class MyComponent { + isDark = computed(() => this.themeService.currentTheme() === Theme.DARK); - constructor(private themeService: ThemeService) {} + constructor(private themeService: ThemeService) { } - ngOnInit() { - this.themeSubscription = this.themeService.getCurrentThemeObservable().subscribe((theme) => { - this.isDark = theme === Theme.DARK; - }); - } - - ngOnDestroy() { - this.themeSubscription.unsubscribe(); - } }Benefits of this approach:
- Automatic cleanup (no need for manual subscription management)
- More concise and reactive code
- Better alignment with Angular 18 practices
- Improved performance through fine-grained updates
src/test/javascript/spec/integration/guided-tour/guided-tour.integration.spec.ts (1)
90-91: Enhance the comment to better explain the workaroundWhile the comment references the ng-mocks issue, it would be helpful to:
- Explain the impact of using the real component
- Document any additional setup required
- Add a TODO to revisit once the ng-mocks issue is resolved
-// Component can not be mocked at the moment https://github.com/help-me-mom/ng-mocks/issues/8634 +// TODO: Replace with MockComponent once ng-mocks issue #8634 is resolved +// Using real ThemeSwitchComponent as a workaround due to ng-mocks limitation +// Note: This may require additional setup for ThemeSwitchComponent's dependenciessrc/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html (1)
132-137: Consider memoizing frequently accessed signalsWhile the transition to signals is good, the template makes multiple calls to
highlightedElements(). Consider memoizing this value to prevent unnecessary recomputations, especially if the highlighting logic is complex.Example optimization in the component:
// Instead of directly using the signal in the template private readonly highlightedElements = signal<Set<string>>(new Set()); // Add a memoized computed that only updates when the underlying signal changes protected readonly highlightedElementsValue = computed(() => this.highlightedElements());Then update the template bindings to use the memoized value:
-@if (highlightedElements() && highlightedElements().size > 0) { +@if (highlightedElementsValue() && highlightedElementsValue().size > 0) {-[highlightedElements]="highlightedElements()" +[highlightedElements]="highlightedElementsValue()"Also applies to: 163-163
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (2)
92-96: Consider adding type safety to the theme subscriptionThe theme subscription implementation is correct and follows reactive patterns. However, we could enhance type safety.
Consider this improvement:
- private themeChangeSubscription = toObservable(this.themeService.currentTheme).subscribe(() => { + private themeChangeSubscription = toObservable(this.themeService.currentTheme).pipe( + filter((theme): theme is NonNullable<typeof theme> => theme !== null) + ).subscribe((theme) => { if (!this.isInitial) { this.updateMarkdown(); } });
48-50: Consider splitting the component for better maintainabilityThe component handles multiple responsibilities (markdown rendering, task injection, theme management). Consider breaking it down into smaller, more focused components following the Single Responsibility Principle.
Suggested improvements:
- Extract markdown rendering logic into a separate service
- Create a dedicated TaskInjectorComponent for task-related functionality
- Consider using a facade service to coordinate between these components
This would improve:
- Testability
- Maintainability
- Code reuse
- Separation of concerns
src/test/javascript/spec/integration/code-editor/code-editor-instructor.integration.spec.ts (3)
Line range hint
66-200: Consider extracting common setup logic into helper functions.The
beforeEachblock contains a significant amount of setup code. Consider extracting the spy setup and subject initialization into separate helper functions to improve readability and maintainability.+ const initializeSpies = (services) => { + const spies = { + checkIfRepositoryIsClean: jest.spyOn(services.codeEditorRepository, 'getStatus'), + getRepositoryContent: jest.spyOn(services.codeEditorRepositoryFile, 'getRepositoryContent'), + // ... other spy initializations + }; + return spies; + }; + + const initializeSubjects = () => { + return { + checkIfRepositoryIsClean: new Subject<{ isClean: boolean }>(), + getRepositoryContent: new Subject<{ [fileName: string]: FileType }>(), + // ... other subject initializations + }; + };
4-4: Remove commented-out TypeScript ignore directive.The
@ts-ignorecomment suggests potential type-safety issues that should be addressed properly rather than ignored.-// @ts-ignore
Line range hint
33-38: Consider using test data builders for exercise objects.Creating test data directly in the test cases makes the tests harder to maintain. Consider implementing a test data builder pattern for
ProgrammingExerciseand related objects.class ProgrammingExerciseBuilder { private exercise: Partial<ProgrammingExercise> = { id: 1, problemStatement: '', course: { id: 1 } }; withTemplateParticipation(id: number): this { this.exercise.templateParticipation = { id, repositoryUri: `test${id}` }; return this; } // ... other builder methods build(): ProgrammingExercise { return this.exercise as ProgrammingExercise; } }src/test/javascript/spec/component/shared/navbar.component.spec.ts (2)
127-132: Consider enhancing the GuidedTourService mock implementation.The current mock only implements
getGuidedTourAvailabilityStream. Consider adding other commonly used methods from GuidedTourService to make the mock more comprehensive for future test cases.{ provide: GuidedTourService, useValue: { getGuidedTourAvailabilityStream: () => of(false), + startTour: () => {}, + endTour: () => {}, + isActive: () => false, }, },
Line range hint
776-885: Consider enhancing breakpoint test documentation and maintainability.While the breakpoint tests are comprehensive, consider these improvements:
- Add comments explaining the breakpoint logic for each user role
- Extract magic numbers into named constants for better maintainability
+// Breakpoint constants for different user roles +const BREAKPOINTS = { + ADMIN: { + COLLAPSE: 1100, + ICONS_TO_MENU: 600, + VERTICAL_NAV: 550, + }, + INSTRUCTOR: { + COLLAPSE: 850, + ICONS_TO_MENU: 600, + VERTICAL_NAV: 470, + }, + USER: { + COLLAPSE: 650, + ICONS_TO_MENU: 600, + VERTICAL_NAV: 470, + }, + ANONYMOUS: { + COLLAPSE: 500, + VERTICAL_NAV: 450, + ICONS_TO_MENU: 400, + }, +}; it.each([ { - width: 1200, + width: BREAKPOINTS.ADMIN.COLLAPSE + 100, // Above collapse breakpoint account: { login: 'test' }, roles: [Authority.ADMIN], expected: { isCollapsed: false, isNavbarNavVertical: false, iconsMovedToMenu: false }, }, // ... rest of the test cases using constantssrc/main/webapp/app/core/theme/theme-switch.component.ts (1)
67-68: Confirm the necessity of thesetTimeoutwhen opening the popoverThe
setTimeoutintoggleTheme()delays the popover opening by 250 milliseconds after toggling the theme. Verify if this delay is required for the desired user experience or if it can be refactored for clarity and responsiveness.src/main/webapp/app/core/theme/theme.service.ts (1)
Line range hint
171-195: Consider usingRenderer2for DOM manipulationsThe
applyThememethod performs direct DOM manipulations using native methods likedocument.getElementByIdanddocument.createElement. To adhere to Angular best practices and enhance compatibility with server-side rendering and web workers, consider using Angular'sRenderer2service for DOM manipulations.Apply this diff to refactor the DOM manipulations using
Renderer2:+import { Renderer2, RendererFactory2 } from '@angular/core'; @Injectable({ providedIn: 'root', }) export class ThemeService { + private renderer: Renderer2; + constructor() { + const rendererFactory = inject(RendererFactory2); + this.renderer = rendererFactory.createRenderer(null, null); effect(() => { // Apply the theme as soon as the currentTheme changes const currentTheme = this.currentTheme(); untracked(() => this.applyTheme(currentTheme)); }); } private applyTheme(theme: Theme) { - const overrideTag = document.getElementById(THEME_OVERRIDE_ID); + const overrideTag = this.renderer.selectRootElement(`#${THEME_OVERRIDE_ID}`, true); if (theme.isDefault) { overrideTag?.remove(); } else { // Select the head element - const head = document.getElementsByTagName('head')[0]; + const head = this.renderer.selectRootElement('head', true); // Create new override tag from the current theme - const newTag = document.createElement('link'); - newTag.id = THEME_OVERRIDE_ID; - newTag.rel = 'stylesheet'; - newTag.href = theme.fileName! + '?_=' + new Date().setMinutes(0, 0, 0); + const newTag = this.renderer.createElement('link'); + this.renderer.setAttribute(newTag, 'id', THEME_OVERRIDE_ID); + this.renderer.setAttribute(newTag, 'rel', 'stylesheet'); + this.renderer.setAttribute(newTag, 'href', theme.fileName! + '?_=' + new Date().setMinutes(0, 0, 0)); // As soon as the new style sheet loads, remove the old override (if present) newTag.onload = () => { overrideTag?.remove(); }; // Append the new stylesheet link tag to the head - const existingLinkTags = head.getElementsByTagName('link'); - const lastLinkTag = existingLinkTags[existingLinkTags.length - 1]; - head.insertBefore(newTag, lastLinkTag?.nextSibling); + this.renderer.appendChild(head, newTag); } } }This change enhances compatibility and maintains Angular's platform independence.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (29)
docs/dev/guidelines/client-design.rst(1 hunks)src/main/webapp/app/app.module.ts(2 hunks)src/main/webapp/app/core/theme/theme-switch.component.html(2 hunks)src/main/webapp/app/core/theme/theme-switch.component.ts(4 hunks)src/main/webapp/app/core/theme/theme.module.ts(0 hunks)src/main/webapp/app/core/theme/theme.service.ts(4 hunks)src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html(2 hunks)src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.ts(5 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts(3 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts(4 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.component.html(1 hunks)src/main/webapp/app/lti/lti13-exercise-launch.component.ts(1 hunks)src/main/webapp/app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts(3 hunks)src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.html(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts(1 hunks)src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts(1 hunks)src/test/javascript/spec/component/emoji/emoji.component.spec.ts(0 hunks)src/test/javascript/spec/component/programming-exercise/programming-exercise-instruction.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts(4 hunks)src/test/javascript/spec/component/shared/navbar.component.spec.ts(18 hunks)src/test/javascript/spec/component/shared/progress-bar.component.spec.ts(3 hunks)src/test/javascript/spec/component/theme/theme-switch.component.spec.ts(1 hunks)src/test/javascript/spec/helpers/mocks/service/mock-theme.service.ts(1 hunks)src/test/javascript/spec/integration/code-editor/code-editor-instructor.integration.spec.ts(1 hunks)src/test/javascript/spec/integration/guided-tour/guided-tour.integration.spec.ts(1 hunks)src/test/javascript/spec/service/theme.service.spec.ts(6 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/core/theme/theme.module.ts
- src/test/javascript/spec/component/emoji/emoji.component.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lti/lti13-exercise-launch.component.ts
🧰 Additional context used
📓 Path-based instructions (25)
src/main/webapp/app/app.module.ts (1)
src/main/webapp/app/core/theme/theme-switch.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/core/theme/theme-switch.component.ts (1)
src/main/webapp/app/core/theme/theme.service.ts (1)
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.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/modeling/manage/example-modeling/example-modeling-submission.component.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.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/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji-picker.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/metis/emoji/emoji-picker.component.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji.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/metis/emoji/emoji.component.ts (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
src/test/javascript/spec/component/emoji/emoji-picker.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/programming-exercise/programming-exercise-instruction.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/shared/monaco-editor/monaco-editor.service.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/shared/navbar.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/shared/progress-bar.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/theme/theme-switch.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/helpers/mocks/service/mock-theme.service.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/integration/code-editor/code-editor-instructor.integration.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/integration/guided-tour/guided-tour.integration.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/service/theme.service.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 (55)
src/main/webapp/app/shared/metis/emoji/emoji.component.html (1)
2-4: Excellent use of new Angular control flow syntax!
The migration from *ngIf to @if/@else syntax aligns perfectly with Angular 18 practices. The function call dark() indicates proper reactive state management.
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (4)
1-1: LGTM! Modern Angular imports added correctly
The addition of computed and inject imports aligns with Angular's modern reactive programming practices.
11-11: LGTM! Modern dependency injection pattern
Using inject() function aligns with Angular's recommended approach for dependency injection.
16-16: Consider extracting theme computation to a shared location
The dark theme computation could potentially be reused across components. Consider moving it to the ThemeService as a shared computed signal if it's used in multiple places.
// In theme.service.ts
+readonly isDarkTheme = computed(() => this.currentTheme() === Theme.DARK);
// In this component
-dark = computed(() => this.themeService.currentTheme() === Theme.DARK);
+dark = this.themeService.isDarkTheme;#!/bin/bash
# Description: Check if similar theme computations exist in other components
# Search for similar dark theme computations
rg -A 2 "Theme\.DARK|isDark|darkTheme" "src/main/webapp/app"
# Search for components using ThemeService
ast-grep --pattern 'ThemeService'10-11: Verify removal of OnDestroy implementation
The removal of OnDestroy is appropriate since we're using computed signals instead of subscriptions. However, let's verify there are no other subscriptions that might need cleanup.
✅ Verification successful
Removal of OnDestroy confirmed as appropriate
No remaining subscriptions found in EmojiComponent that require cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining subscriptions in the component
# that might require cleanup
# Search for subscription patterns in the component
rg -A 5 "subscribe\(|Subscription" "src/main/webapp/app/shared/metis/emoji/emoji.component"
# Search for any event listeners or observables that might need cleanup
ast-grep --pattern 'this.$_ = $_.$$$subscribe($_)'
Length of output: 12348
src/test/javascript/spec/helpers/mocks/service/mock-theme.service.ts (2)
1-4: LGTM! Clean imports and proper mock naming.
The imports are minimal and specific, and the mock service follows proper naming conventions.
11-13: Verify behavioral consistency with ThemeService.
The implementation looks good, but we should verify that it mirrors the behavior of the actual ThemeService.
#!/bin/bash
# Description: Compare mock behavior with actual ThemeService
# Find the actual ThemeService implementation
rg -A 5 "applyThemePreference.*Theme.*undefined" "src/main/webapp"
# Look for test cases that verify the consistency
rg -l "expect.*applyThemePreference.*Theme\.LIGHT" "src/test/javascript/spec"src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.component.html (1)
5-5: LGTM! Correctly using new Angular 18 syntax.
The template has been properly updated to use the new @for syntax instead of *ngFor, which aligns with Angular 18 best practices.
src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts (1)
36-39: LGTM! Theme change verification is well implemented.
The test correctly verifies:
- Theme change using the new
applyThemePreferencemethod - Dark mode state using recommended
toBeTrue()assertion - Emoji image path generation for the updated theme
src/main/webapp/app/core/theme/theme-switch.component.html (2)
5-5: LGTM! Clean implementation of the sync toggle.
The changes appropriately use the jhiTranslate directive and implement a reactive approach with the isSyncedWithOS() method.
Also applies to: 7-7
20-21: Verify the hardcoded animation value.
While simplifying the animation property to a hardcoded value can improve code clarity, please verify that this doesn't affect any dynamic animation requirements or accessibility features.
Run the following script to check for any dynamic animation configurations in the component or its tests:
✅ Verification successful
Hardcoded animation value is acceptable and consistent with existing configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for animation-related configurations
# Look for animation configurations in the component and test files
rg -A 3 "animation.*=|animations:" --type ts
Length of output: 3216
src/main/webapp/app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts (3)
1-6: LGTM! Clean import updates for signal integration
The changes appropriately remove OnInit and add toObservable for signal conversion, aligning with Angular 18 practices.
Line range hint 11-19: LGTM! Clean class structure
The component properly implements OnChanges and OnDestroy, with well-defined Input properties following Angular guidelines.
Line range hint 31-42: LGTM! Proper lifecycle management
The component correctly handles changes and cleanup, preventing memory leaks.
src/test/javascript/spec/component/shared/progress-bar.component.spec.ts (1)
8-8: LGTM: Proper mock service import
The import of MockThemeService aligns with our mocking guidelines and supports the transition to signals-based theme management.
src/test/javascript/spec/component/theme/theme-switch.component.spec.ts (3)
3-9: LGTM! Imports follow best practices.
The imports are well-organized and follow the guidelines by avoiding full module imports and properly importing test utilities.
45-54: LGTM! Theme toggle test follows best practices.
The test properly validates theme changes and follows the coding guidelines for expectations using toBeTrue() and toHaveBeenCalledWith().
70-84: LGTM! Popover behavior tests are comprehensive.
The tests properly validate:
- Open/close functionality
- Timing of operations
- Mouse leave behavior
And follow the coding guidelines for spy expectations usingtoHaveBeenCalledOnce()andnot.toHaveBeenCalled().
src/main/webapp/app/app.module.ts (1)
24-24: Verify standalone component configuration
The import of ThemeSwitchComponent suggests it's now a standalone component. Let's verify its configuration to ensure it follows Angular 18 best practices.
✅ Verification successful
Further Verification Required
Further Verification Required
Review Comment Clarification
The ThemeSwitchComponent is correctly configured as a standalone component and is only utilized within the AppModule. There are no additional usages that impact lazy loading or initial load performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ThemeSwitchComponent is properly configured as standalone
# and uses signals for state management
# Check component decorator and configuration
ast-grep --pattern 'Component({
$$$
standalone: true,
$$$
})
class ThemeSwitchComponent'
# Check for signal usage
rg -A 5 'signal<|computed\(' app/core/theme/theme-switch.component.ts
Length of output: 373
Script:
#!/bin/bash
# Description: Locate ThemeSwitchComponent and verify its standalone configuration and signal usage
# Locate all ThemeSwitchComponent files
fd 'theme-switch\.component\.ts$' > theme_switch_files.txt
if [ -s theme_switch_files.txt ]; then
while read -r file; do
echo "Verifying $file"
# Verify standalone configuration
ast-grep --pattern 'Component({
$$$
standalone: true,
$$$
})' "$file"
# Check for signal usage
rg -A 5 'signal<|computed\(' "$file"
done < theme_switch_files.txt
else
echo "ThemeSwitchComponent file not found."
fi
Length of output: 1593
Script:
#!/bin/bash
# Description: Locate all usages of ThemeSwitchComponent to assess lazy loading implications
# Search for ThemeSwitchComponent usage in TypeScript and HTML files, excluding app.module.ts
rg -l 'ThemeSwitchComponent' --glob 'src/main/webapp/app/**/*.ts' --glob 'src/main/webapp/app/**/*.html' | grep -v 'app.module.ts' > theme_switch_usages.txt
if [ -s theme_switch_usages.txt ]; then
echo "ThemeSwitchComponent is used in the following files:"
cat theme_switch_usages.txt
else
echo "No usages of ThemeSwitchComponent found outside app.module.ts."
fi
Length of output: 367
src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts (2)
1-1: LGTM! Modern dependency injection implementation.
The transition to using inject() function and readonly properties aligns well with Angular 18's best practices, improving code maintainability and type safety.
Also applies to: 15-16
Line range hint 38-43: Verify cache invalidation performance under frequent theme changes.
The cache configuration looks good, but we should verify that frequent theme changes don't cause performance issues due to cache invalidation.
✅ Verification successful
Cache Invalidation Performance Verified
The cache invalidation setup with themeChangedSubject is localized to programming-exercise-plant-uml.service.ts. No other components are affected, ensuring that frequent theme changes do not introduce unintended performance issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of themeChangedSubject to understand the impact
rg -l "themeChangedSubject" src/main/webapp/
# Look for other cached components that might be affected by theme changes
ast-grep --pattern '@Cacheable({
$$$
cacheBusterObserver: $_,
$$$
})'
Length of output: 4227
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (3)
4-4: LGTM! Import path update is appropriate.
The change to use absolute imports from 'app/' improves maintainability and follows Angular best practices.
15-16: LGTM! Variable declaration follows testing best practices.
Moving the themeService declaration to class level and removing BehaviorSubject aligns with the migration to signals in Angular 18.
28-28: LGTM! Proper TestBed injection setup.
The ThemeService injection in beforeEach follows Angular testing best practices.
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (2)
17-17: LGTM! Proper implementation of Angular signals
The initialization of currentTheme directly from ThemeService's signal aligns with Angular 18's best practices for reactive state management.
Line range hint 26-28: Verify theme application timing and cleanup
While the effect implementation looks correct, let's verify:
- Theme application timing during editor initialization
- Proper cleanup to prevent memory leaks
src/test/javascript/spec/service/theme.service.spec.ts (1)
13-13: LGTM! Clean setup with proper mocking practices.
The mock variable renaming improves naming consistency, and the setup follows jest best practices with proper spy implementations.
Also applies to: 39-39
docs/dev/guidelines/client-design.rst (1)
240-240: LGTM! Method name updated correctly.
The change from applyThemeExplicitly to applyThemePreference aligns with the PR objectives and maintains consistency with the updated ThemeService implementation.
src/test/javascript/spec/integration/guided-tour/guided-tour.integration.spec.ts (1)
90-91: Verify test isolation with real ThemeSwitchComponent
Using the real ThemeSwitchComponent instead of a mock could affect test isolation and performance. Please verify:
- That all required dependencies are properly provided
- That the component's initialization doesn't interfere with the guided tour tests
- That test execution time remains reasonable
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html (2)
Line range hint 1-1: LGTM: Proper usage of new Angular control flow syntax
The template correctly uses the new Angular control flow syntax (@if and @for) instead of the older *ngIf and *ngFor directives, which aligns with the coding guidelines.
Also applies to: 7-7, 8-8, 13-13, 15-15, 20-20, 38-38, 41-41, 63-63, 66-66, 71-71, 74-74, 82-82, 85-85, 89-89, 92-92, 132-132, 138-138, 141-141, 147-147, 150-150, 170-170, 179-179, 182-182, 185-185, 188-188, 191-191
Line range hint 141-146: LGTM: Clean component integration
The template demonstrates proper component integration with clear input/output bindings and event handling. The conditional rendering of either jhi-modeling-editor or jhi-modeling-assessment based on the assessment mode is well-structured.
Also applies to: 150-164
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (1)
14-14: LGTM: Modern dependency injection pattern applied correctly
The changes follow Angular 18's best practices by:
- Using the new
inject()function for dependency injection - Importing
toObservablefor signal-to-observable conversion - Moving ThemeService injection out of the constructor
Also applies to: 43-43, 51-52
src/test/javascript/spec/component/programming-exercise/programming-exercise-instruction.component.spec.ts (1)
213-213: LGTM: Fixture change detection is correctly placed
The addition of fixture.detectChanges() ensures that Angular's change detection is triggered after the component changes, which is a good practice for component testing.
src/test/javascript/spec/integration/code-editor/code-editor-instructor.integration.spec.ts (3)
Line range hint 1-65: LGTM! Well-organized imports and comprehensive mock setup.
The test file demonstrates good practices with:
- Clear separation of imports
- Proper use of NgMocks for component mocking
- Comprehensive provider setup
Line range hint 251-500: LGTM! Comprehensive test coverage for repository navigation.
The test suite thoroughly covers:
- Repository switching scenarios
- Navigation between different repository types
- Error states
- Component state management
Line range hint 201-250: Consider adding test coverage for error scenarios in repository navigation.
The initContainer helper function is well implemented, but the test suite could benefit from additional test cases covering error scenarios during repository navigation.
Add test cases for:
- Network errors during repository navigation
- Invalid repository states
- Permission-related errors
src/test/javascript/spec/component/shared/navbar.component.spec.ts (1)
231-246: LGTM! Improved readability of breadcrumb expectations.
The reformatting of breadcrumb expectations follows a consistent pattern and improves code readability. The structure makes it easier to understand and maintain the test cases.
Also applies to: 257-266, 279-288, 299-303, 405-409, 434-438, 479-488, 552-556, 592-596, 627-636, 660-664, 692-701, 737-746, 767-770
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (3)
1-1: Imports Updated Appropriately
The import statement correctly includes computed and inject for utilizing Angular's reactive features.
11-11: Proper Use of 'inject' for Dependency Injection
Using inject(ThemeService) simplifies dependency management and aligns with Angular 18 best practices.
19-20: Verify Fallback Function in 'singleImageFunction'
The fallback function () => '' returns an empty string when the theme is not dark. Please confirm that this behavior is intended and does not affect the emoji display in light mode.
src/main/webapp/app/core/theme/theme-switch.component.ts (3)
21-23: Excellent use of standalone components and OnPush change detection
Declaring the component as standalone and utilizing ChangeDetectionStrategy.OnPush enhances performance and modularity. This aligns well with the Angular style guidelines and promotes efficient change detection.
28-28: Verify compatibility of inject(), input.required, and viewChild.required with your Angular version
While the use of inject(), input.required, and viewChild.required reflects modern Angular practices, please ensure that these features are supported in your project's current Angular version and align with your team's coding standards.
Also applies to: 30-31
33-34: Effective use of computed for reactive state management
Utilizing computed() for isDarkTheme and isSyncedWithOS provides reactive and efficient state handling, enhancing the maintainability of the component.
src/main/webapp/app/core/theme/theme.service.ts (9)
46-49: Proper use of Angular signals for user preference
The implementation correctly initializes _userPreference as a writable signal and exposes userPreference as a readonly signal. This adheres to best practices for encapsulating mutable state and providing a read-only interface where appropriate.
58-60: Initialization of systemPreference with default theme
Setting systemPreference as a signal with a default value of Theme.LIGHT ensures that there's a fallback theme if the system preference cannot be determined.
63-66: Effective use of computed signal for currentTheme
The currentTheme computed signal correctly derives its value based on userPreference and systemPreference. This reactive approach ensures that theme changes are automatically reflected in the application when either preference changes.
67-68: Dependency injection using inject function
Using the inject function to obtain an instance of LocalStorageService aligns with Angular 18 practices and simplifies the constructor.
72-76: Proper use of effect and untracked for theme application
The effect watches for changes in currentTheme and applies the theme accordingly. Wrapping applyTheme with untracked ensures that calling this method doesn't create additional dependencies or unintended side effects within the reactive context.
87-93: Handling system theme preference changes efficiently
Adding an event listener to darkSchemeMediaQuery allows the application to react to changes in the user's system theme preference. Updating the systemPreference signal within this listener ensures that currentTheme reflects the latest preference.
102-103: Initial setting of systemPreference and userPreference
Setting the initial values of systemPreference and _userPreference ensures that the theme is correctly applied when the application starts, based on the system settings and any stored user preferences.
109-115: Robust retrieval of stored theme preference
The getStoredThemePreference method safely retrieves the stored theme preference from local storage and validates it against the available themes. Clearing unknown or invalid stored values prevents potential errors due to mismatched theme identifiers.
155-162: Efficiently applying user theme preference and managing local storage
The applyThemePreference method accurately stores or clears the theme preference in local storage based on user input and updates the _userPreference signal. This ensures that user preferences are persisted and reflected in the application's theme.
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.ts (3)
43-44: Injecting ThemeService using Angular's inject function is appropriate
The use of inject(ThemeService) for dependency injection aligns with Angular 18's recommended practices and enhances code readability by reducing constructor parameters.
98-100: Utilization of Angular Signals and Computed Properties
Implementing highlightedElements as a signal and highlightColor as a computed property effectively leverages Angular's reactive programming features, ensuring efficient updates and state management.
118-130: Efficiently Updating Highlighted Elements on Theme Change
The effect hook correctly updates highlightedElements whenever the theme changes, ensuring that UI elements reflect the current theme. This approach is consistent with Angular's reactive patterns and promotes maintainability.
src/test/javascript/spec/component/shared/progress-bar.component.spec.ts
Show resolved
Hide resolved
...ses/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts
Show resolved
Hide resolved
...cript/spec/component/programming-exercise/programming-exercise-instruction.component.spec.ts
Show resolved
Hide resolved
...bapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (30)
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (1)
13-16: Consider adding JSDoc documentationAdding JSDoc comments would improve maintainability by documenting:
- The purpose of the emoji input
- The computed dark property's usage
- The component's overall responsibility
Example improvement:
+/** Utility instance for emoji-related operations */ utils = EmojiUtils; +/** The emoji character to be rendered */ @Input() emoji: string; +/** Computed signal that determines if dark theme is active */ dark = computed(() => this.themeService.currentTheme() === Theme.DARK);src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html (1)
10-11: Good use of Angular 18 signals pattern!The changes from property bindings to function calls (
dark()andsingleImageFunction()) align well with Angular 18's signals pattern for reactive updates. This approach ensures that theme changes are properly propagated through the component.Consider documenting these patterns in your team's guidelines as they serve as a good example for:
- Using signals for reactive theme management
- Proper integration with third-party components (emoji-mart)
- Clean template syntax that clearly indicates reactive bindings
src/test/javascript/spec/helpers/mocks/service/mock-theme.service.ts (1)
5-6: Consider adding JSDoc comments for better documentationThe implementation correctly uses signals and follows proper encapsulation patterns. Consider adding JSDoc comments to document the purpose of these signals and their readonly nature.
+/** Private signal for managing the current theme state */ private _currentTheme = signal(Theme.LIGHT); +/** Public readonly access to the current theme */ public readonly currentTheme = this._currentTheme.asReadonly();src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts (1)
33-34: Improve assertion specificity as per coding guidelinesThe test follows the coding guidelines for boolean assertions (
toBeTrue/toBeFalse), but the string comparison could be more specific.Consider using more specific assertions:
-expect(comp.singleImageFunction()({ unified: '1F519' } as EmojiData)).toBe(''); +expect(comp.singleImageFunction()({ unified: '1F519' } as EmojiData)).toStrictEqual(''); -expect(comp.singleImageFunction()({ unified: '1F519' } as EmojiData)).toBe('public/emoji/1f519.png'); +expect(comp.singleImageFunction()({ unified: '1F519' } as EmojiData)).toMatch(/^public\/emoji\/[a-f0-9]+\.png$/);Also applies to: 38-39
src/main/webapp/app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts (1)
Line range hint
62-74: Consider enhancing theme handling with a more type-safe approachWhile the theme handling works, it could benefit from a more robust implementation:
Consider using a type-safe mapping object:
-chooseProgressBarTextColor() { - switch (this.themeService.currentTheme()) { - case Theme.DARK: - this.foregroundColorClass = 'text-white'; - break; - case Theme.LIGHT: - default: - if (this.percentage < 100) { - this.foregroundColorClass = 'text-dark'; - } else { - this.foregroundColorClass = 'text-white'; - } - } -} +private readonly themeColorMap = { + [Theme.DARK]: 'text-white', + [Theme.LIGHT]: (percentage: number) => + percentage < 100 ? 'text-dark' : 'text-white', +} as const; + +chooseProgressBarTextColor() { + const theme = this.themeService.currentTheme(); + const colorMapper = this.themeColorMap[theme] ?? this.themeColorMap[Theme.LIGHT]; + this.foregroundColorClass = typeof colorMapper === 'function' + ? colorMapper(this.percentage) + : colorMapper; +}This approach provides better type safety and makes it easier to add new themes in the future.
src/test/javascript/spec/component/shared/progress-bar.component.spec.ts (1)
59-62: Consider optimizing change detection callsThe explicit
fixture.detectChanges()call afterapplyThemePreferencemight be redundant since the theme service should trigger change detection automatically through signals.Consider removing the explicit change detection call:
themeService.applyThemePreference(Theme.DARK); - -fixture.detectChanges();src/test/javascript/spec/component/theme/theme-switch.component.spec.ts (3)
19-41: Consider optimizing test module imports and adding type safety.The setup is well-structured but could be enhanced:
- Consider creating a minimal test module instead of importing the full ArtemisTestModule
- Add type safety to the spy declarations
- let openSpy: jest.SpyInstance; - let closeSpy: jest.SpyInstance; + let openSpy: jest.SpyInstance<void, []>; + let closeSpy: jest.SpyInstance<void, []>;
45-54: Add negative test cases for theme toggle.While the happy path is well tested, consider adding test cases for:
- Error scenarios
- Edge cases when theme service fails
- Multiple rapid toggles
69-84: Consider adding timing variation tests.The timing-dependent tests are well structured but could be more robust:
- Test different timing scenarios (e.g., rapid mouse enter/leave)
- Verify behavior when timing is just under/over threshold
Example additional test:
it('handles rapid mouse enter/leave correctly', fakeAsync(() => { component.openPopover(); component.mouseLeave(); tick(100); component.openPopover(); component.mouseLeave(); tick(100); expect(closeSpy).not.toHaveBeenCalled(); tick(150); expect(closeSpy).toHaveBeenCalledOnce(); }));src/main/webapp/app/app.module.ts (1)
Line range hint
31-48: Consider documenting theme configuration in module commentsThe module structure correctly maintains separation of concerns and follows lazy loading principles. Since theme management is now component-based, consider adding a comment explaining the theme configuration approach for better maintainability.
Add a comment before the imports array:
@NgModule({ imports: [ + // Theme management is handled via standalone ThemeSwitchComponent + // following Angular 18 practices for better modularity BrowserModule,src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts (1)
47-47: Improve code reuse and type safety in PlantUML methodsThe theme check logic and caching configuration are duplicated. Consider extracting common functionality to improve maintainability.
Consider these improvements:
+private readonly cacheConfig = { + maxCacheCount: 100, + maxAge: 3600000, + slidingExpiration: true, + cacheBusterObserver: themeChangedSubject, +}; +private getThemeParams(plantUml: string): HttpParams { + return new HttpParams({ encoder: this.encoder }) + .set('plantuml', plantUml) + .set('useDarkTheme', String(this.themeService.currentTheme() === Theme.DARK)); +} @Cacheable({ - maxCacheCount: 100, - maxAge: 3600000, - slidingExpiration: true, - cacheBusterObserver: themeChangedSubject, + ...this.cacheConfig }) getPlantUmlImage(plantUml: string) { return this.http .get(`${this.resourceUrl}/png`, { - params: new HttpParams({ encoder: this.encoder }) - .set('plantuml', plantUml) - .set('useDarkTheme', this.themeService.currentTheme() === Theme.DARK), + params: this.getThemeParams(plantUml), responseType: 'arraybuffer', }) .pipe(map((res) => this.convertPlantUmlResponseToBase64(res))); }Also applies to: 68-68
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (1)
46-54: Consider adding edge case tests for theme changesWhile the basic theme switching tests are good, consider adding tests for:
- Rapid theme switches
- Invalid theme values
- Theme switch during editor initialization
Example test case:
it('should handle rapid theme switches correctly', () => { themeService.applyThemePreference(Theme.DARK); themeService.applyThemePreference(Theme.LIGHT); themeService.applyThemePreference(Theme.DARK); TestBed.flushEffects(); expect(setThemeSpy).toHaveBeenCalledTimes(4); expect(setThemeSpy).toHaveBeenLastCalledWith(MONACO_DARK_THEME_DEFINITION.id); });src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
Line range hint
24-28: Consider adding cleanup for effectThe effect is created in the constructor but there's no cleanup mechanism. To prevent potential memory leaks, consider using the cleanup callback or moving the effect to a component that handles its lifecycle.
constructor() { this.registerCustomThemes(); this.registerCustomMarkdownLanguage(); - effect(() => { - this.applyTheme(this.currentTheme()); - }); + // Store effect reference for potential cleanup + const themeEffect = effect(() => { + this.applyTheme(this.currentTheme()); + // Return cleanup function if needed + return () => { + // Cleanup theme-related resources + }; + }); }src/test/javascript/spec/service/theme.service.spec.ts (2)
Line range hint
64-99: Improve test structure with better organizationWhile the test logic is correct, consider restructuring it for better readability and maintainability.
Consider this structure:
it('applies theme changes correctly', () => { TestBed.flushEffects(); - expect(documentGetElementMock).toHaveBeenCalledOnce(); + // Initial state verification + expect(documentGetElementMock).toHaveBeenCalledOnce(); + + // Apply dark theme service.applyThemePreference(Theme.DARK); TestBed.flushEffects(); - expect(documentGetElementMock).toHaveBeenCalledTimes(2); + // Verify dark theme application + expect(documentGetElementMock).toHaveBeenCalledTimes(2); expect(documentGetElementMock).toHaveBeenCalledWith(THEME_OVERRIDE_ID); expect(documentGetElementsByTagNameMock).toHaveBeenCalledOnce(); expect(documentGetElementsByTagNameMock).toHaveBeenCalledWith('head'); + + // Verify DOM updates expect(documentCreateElementMock).toHaveBeenCalledOnce(); expect(documentCreateElementMock).toHaveBeenCalledWith('link'); expect(newElement.id).toBe(THEME_OVERRIDE_ID); expect(newElement.rel).toBe('stylesheet'); expect(newElement.href).toStartWith('theme-dark.css'); expect(newElement.onload).toEqual(expect.any(Function)); + + // Verify theme state expect(service.currentTheme()).toBe(Theme.DARK); expect(storeSpy).toHaveBeenCalledWith(THEME_LOCAL_STORAGE_KEY, 'DARK'); expect(linkElement.remove).toHaveBeenCalledOnce(); + // Apply light theme service.applyThemePreference(Theme.LIGHT); TestBed.flushEffects(); + // Verify light theme application expect(documentGetElementMock).toHaveBeenCalledTimes(3); expect(documentGetElementMock).toHaveBeenNthCalledWith(3, THEME_OVERRIDE_ID); expect(linkElement.remove).toHaveBeenCalledTimes(2); expect(service.currentTheme()).toBe(Theme.LIGHT); });
126-133: Consider adding edge cases for OS preference testsWhile the current tests cover the basic scenarios, consider adding tests for:
- OS preference changes after initial load
- Handling of invalid OS preference values
- Race conditions between OS preference changes and explicit theme changes
Example test case:
it('handles OS preference changes after initial load', fakeAsync(() => { const retrieveSpy = jest.spyOn(localStorageService, 'retrieve').mockReturnValue(undefined); let darkModeListener: ((e: MediaQueryListEvent) => void) | undefined; // Mock matchMedia to capture the listener windowMatchMediaSpy.mockImplementation((query) => ({ media: query, matches: false, addEventListener: (_, listener) => { darkModeListener = listener; } })); service.initialize(); TestBed.flushEffects(); expect(service.currentTheme()).toBe(Theme.LIGHT); // Simulate OS preference change if (darkModeListener) { darkModeListener({ matches: true } as MediaQueryListEvent); TestBed.flushEffects(); expect(service.currentTheme()).toBe(Theme.DARK); } }));Also applies to: 140-145
src/test/javascript/spec/integration/guided-tour/guided-tour.integration.spec.ts (2)
90-91: Consider alternative mocking strategies for ThemeSwitchComponent.The comment indicates an issue with ng-mocks preventing the mocking of ThemeSwitchComponent. While using the actual component works, it might introduce unnecessary dependencies and slow down tests.
Consider these alternatives:
- Create a minimal mock implementation of ThemeSwitchComponent
- Use jest.mock() with a custom factory
- Track the ng-mocks issue and update once fixed
Example minimal mock:
@Component({ selector: 'jhi-theme-switch', template: '' }) class MockThemeSwitchComponent {}
Line range hint
156-196: Test assertions follow best practices but could be more specific.The test assertions follow good practices by:
- Using specific matchers (toBeTrue, toBeFalse, not.toBeNull)
- Checking component state after each step
- Verifying element existence
However, consider enhancing the assertions:
// Instead of just checking element existence expect(selectedElement).not.toBeNull(); // Also verify the element's properties and state expect(selectedElement.nativeElement.classList.contains('highlighted')).toBeTrue(); expect(selectedElement).toMatchObject({ properties: { // Add expected properties } });src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (1)
92-96: Consider optimizing theme change subscriptionWhile the implementation works, consider these improvements for better performance and reliability:
- Move the subscription initialization to ngOnInit to avoid potential race conditions
- Add debouncing to prevent rapid consecutive theme updates
- Add distinctUntilChanged to prevent unnecessary updates when theme hasn't changed
-private themeChangeSubscription = toObservable(this.themeService.currentTheme).subscribe(() => { - if (!this.isInitial) { - this.updateMarkdown(); - } -}); +private themeChangeSubscription: Subscription; + +ngOnInit() { + this.themeChangeSubscription = toObservable(this.themeService.currentTheme).pipe( + filter(() => !this.isInitial), + distinctUntilChanged(), + debounceTime(100) + ).subscribe(() => { + this.updateMarkdown(); + }); +}src/test/javascript/spec/component/programming-exercise/programming-exercise-instruction.component.spec.ts (2)
213-213: Consider consolidating detectChanges() calls.Multiple
detectChanges()calls can mask timing issues in tests. Consider consolidating this with the earlierdetectChanges()call or document why multiple updates are necessary.
Line range hint
1-469: Consider improving test organization and structure.The test suite could benefit from better organization:
- Group related tests using
describeblocks (e.g., "Theme Changes", "Problem Statement Updates", "Task Icons").- Consider extracting common setup code into shared helper functions.
- Add test cases for error scenarios in theme changes.
This would improve maintainability and make the test suite easier to understand and extend.
src/test/javascript/spec/integration/code-editor/code-editor-instructor.integration.spec.ts (4)
Line range hint
65-65: Consider extracting mock setup to a separate helper fileThe test setup involves numerous mock services and components. Consider extracting this setup into a dedicated test helper file to improve maintainability and reusability across other test files.
Line range hint
66-200: Refactor test initialization for better maintainabilityThe beforeEach block is handling too many responsibilities. Consider breaking it down into smaller, focused setup functions:
- Component setup
- Service injection
- Mock setup
- Spy setup
This would improve readability and make the test setup more maintainable.
Example refactor:
const setupComponent = () => { containerFixture = TestBed.createComponent(CodeEditorInstructorAndEditorContainerComponent); comp = containerFixture.componentInstance; containerDebugElement = containerFixture.debugElement; }; const setupServices = () => { const services = { codeEditorRepository: containerDebugElement.injector.get(CodeEditorRepositoryService), // ... other services }; return services; }; const setupMocks = () => { checkIfRepositoryIsCleanSubject = new Subject<{ isClean: boolean }>(); // ... other subjects }; const setupSpies = (services: any) => { checkIfRepositoryIsCleanStub = jest.spyOn(services.codeEditorRepository, 'getStatus'); // ... other spies };
Line range hint
201-450: Improve type safety in test casesSeveral test cases use @ts-ignore to bypass TypeScript checks. Consider creating proper type definitions or partial mocks instead:
type PartialProgrammingExercise = Partial<ProgrammingExercise> & { id: number; course: { id: number }; }; const createMockExercise = (overrides?: Partial<PartialProgrammingExercise>): PartialProgrammingExercise => ({ id: 1, course: { id: 1 }, ...overrides });This would make the tests more type-safe and maintainable.
Line range hint
315-339: Make expectations more specificThe test expectations for component existence could be more specific. Instead of just checking if they're defined, consider checking for specific properties or states:
expect(comp.codeEditorContainer.buildOutput.participation).toBeDefined();could be:
expect(comp.codeEditorContainer.buildOutput.participation).toEqual( expect.objectContaining({ id: exercise.studentParticipations[0].id, // other relevant properties }) );src/test/javascript/spec/component/shared/navbar.component.spec.ts (2)
41-42: Consider enhancing the GuidedTourService mock implementation.The current mock implementation only covers
getGuidedTourAvailabilityStream. Consider implementing other commonly used methods to make the tests more robust.Also, the need to override the component to remove
ThemeSwitchComponentfrom imports might indicate a circular dependency. Consider reviewing the component's architecture.Also applies to: 113-113, 127-132, 139-143
Line range hint
776-885: Consider improving breakpoint test maintainability.While the breakpoint tests are comprehensive, consider these improvements:
- Add comments explaining the breakpoint logic for each role
- Extract magic numbers (widths) into named constants
- Group test cases by role for better readability
Example improvement:
+ // Breakpoint constants + const BREAKPOINTS = { + ADMIN: { COLLAPSE: 1100, ICONS_TO_MENU: 600, VERTICAL_NAV: 550 }, + INSTRUCTOR: { COLLAPSE: 850, ICONS_TO_MENU: 600, VERTICAL_NAV: 470 }, + USER: { COLLAPSE: 650, ICONS_TO_MENU: 600, VERTICAL_NAV: 470 }, + ANONYMOUS: { COLLAPSE: 500, ICONS_TO_MENU: 400, VERTICAL_NAV: 450 } + }; it.each([ + // Admin breakpoints { - width: 1200, + width: BREAKPOINTS.ADMIN.COLLAPSE + 100, account: { login: 'test' }, roles: [Authority.ADMIN], expected: { isCollapsed: false, isNavbarNavVertical: false, iconsMovedToMenu: false }, }, // ... rest of the test cases ])('should calculate correct breakpoints', ...src/main/webapp/app/core/theme/theme-switch.component.ts (1)
36-36: Specify a more precise type instead of 'any' for 'closeTimeout'Using
anydefeats the purpose of TypeScript's type checking. SincesetTimeoutreturns a number in the browser environment, it's better to specify the type asnumberto enhance type safety.Apply this change:
- closeTimeout: any; + closeTimeout: number;src/main/webapp/app/core/theme/theme.service.ts (2)
Line range hint
171-194: Avoid direct DOM manipulation; useRenderer2for DOM operationsDirect manipulation of the DOM using native APIs like
document.getElementByIdanddocument.createElementis discouraged in Angular applications. To ensure compatibility across different platforms and enhance testability, consider using Angular'sRenderer2service for DOM manipulations in theapplyThememethod.
87-93: Ensure proper cleanup of event listeners to prevent memory leaksThe service adds event listeners to
darkSchemeMediaQueryand the globalstorageevent. While the service is provided in the root and likely persists for the application's lifetime, it's good practice to remove event listeners in angOnDestroymethod. This prevents potential memory leaks and prepares the code for future changes where the service's lifecycle might be shorter.Also applies to: 98-99
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.ts (1)
98-100: Converting properties to signals and computed values for reactivity
highlightedElementsis now asignal<Map<string, string>>, improving reactive state management.highlightColoris now a computed property that dynamically updates based on the theme.Consider the following suggestion:
Avoid hardcoding color values; use theme constants instead
Instead of hardcoding
'darkblue'and'lightblue', consider using predefined theme variables or constants to ensure consistency and ease of maintenance across the application.Example:
- highlightColor = computed(() => (this.themeService.userPreference() === Theme.DARK ? 'darkblue' : 'lightblue')); + highlightColor = computed(() => (this.themeService.userPreference() === Theme.DARK ? DARK_THEME_HIGHLIGHT_COLOR : LIGHT_THEME_HIGHLIGHT_COLOR));Ensure that
DARK_THEME_HIGHLIGHT_COLORandLIGHT_THEME_HIGHLIGHT_COLORare defined in a centralized theme configuration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (29)
docs/dev/guidelines/client-design.rst(1 hunks)src/main/webapp/app/app.module.ts(2 hunks)src/main/webapp/app/core/theme/theme-switch.component.html(2 hunks)src/main/webapp/app/core/theme/theme-switch.component.ts(4 hunks)src/main/webapp/app/core/theme/theme.module.ts(0 hunks)src/main/webapp/app/core/theme/theme.service.ts(4 hunks)src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html(2 hunks)src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.ts(5 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts(3 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts(4 hunks)src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.component.html(1 hunks)src/main/webapp/app/lti/lti13-exercise-launch.component.ts(1 hunks)src/main/webapp/app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts(3 hunks)src/main/webapp/app/shared/metis/emoji/emoji-picker.component.html(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.html(1 hunks)src/main/webapp/app/shared/metis/emoji/emoji.component.ts(1 hunks)src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts(1 hunks)src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts(1 hunks)src/test/javascript/spec/component/emoji/emoji.component.spec.ts(0 hunks)src/test/javascript/spec/component/programming-exercise/programming-exercise-instruction.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts(4 hunks)src/test/javascript/spec/component/shared/navbar.component.spec.ts(18 hunks)src/test/javascript/spec/component/shared/progress-bar.component.spec.ts(3 hunks)src/test/javascript/spec/component/theme/theme-switch.component.spec.ts(1 hunks)src/test/javascript/spec/helpers/mocks/service/mock-theme.service.ts(1 hunks)src/test/javascript/spec/integration/code-editor/code-editor-instructor.integration.spec.ts(1 hunks)src/test/javascript/spec/integration/guided-tour/guided-tour.integration.spec.ts(1 hunks)src/test/javascript/spec/service/theme.service.spec.ts(6 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/core/theme/theme.module.ts
- src/test/javascript/spec/component/emoji/emoji.component.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lti/lti13-exercise-launch.component.ts
🧰 Additional context used
📓 Path-based instructions (25)
src/main/webapp/app/app.module.ts (1)
src/main/webapp/app/core/theme/theme-switch.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/core/theme/theme-switch.component.ts (1)
src/main/webapp/app/core/theme/theme.service.ts (1)
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.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/modeling/manage/example-modeling/example-modeling-submission.component.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts (1)
src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.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/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji-picker.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/metis/emoji/emoji-picker.component.ts (1)
src/main/webapp/app/shared/metis/emoji/emoji.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/metis/emoji/emoji.component.ts (1)
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
src/test/javascript/spec/component/emoji/emoji-picker.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/programming-exercise/programming-exercise-instruction.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/shared/monaco-editor/monaco-editor.service.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/shared/navbar.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/shared/progress-bar.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/theme/theme-switch.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/helpers/mocks/service/mock-theme.service.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/integration/code-editor/code-editor-instructor.integration.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/integration/guided-tour/guided-tour.integration.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/service/theme.service.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 (46)
src/main/webapp/app/shared/metis/emoji/emoji.component.html (3)
2-4: Great use of new Angular control flow syntax!
The template correctly uses the new @if/@else syntax instead of the older *ngIf, which aligns with Angular 18 best practices.
3-5: LGTM: Consistent emoji rendering configuration
The component maintains consistent base properties (size, backgroundImageFn) while properly handling theme-specific configuration through imageUrlFn for dark mode.
2-2: Verify dark() signal implementation
The change from a boolean property to dark() suggests it's now a signal or computed value. Let's verify its implementation in the component.
✅ Verification successful
dark() is correctly implemented as a computed value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dark() signal implementation in the component
# Expected: dark should be defined as a signal or computed value
# Search for the dark signal/computed definition in the component
ast-grep --pattern 'class EmojiComponent {
$$$
dark = $_;
$$$
}'
# Also check for potential computed implementation
ast-grep --pattern 'class EmojiComponent {
$$$
dark = computed(() => $$$);
$$$
}'
Length of output: 1808
src/main/webapp/app/shared/metis/emoji/emoji.component.ts (3)
1-3: LGTM! Imports follow Angular best practices
The imports are well-organized and correctly utilize modern Angular features (computed, inject).
5-9: LGTM! Component configuration follows Angular guidelines
The component decorator maintains proper configuration with appropriate selector prefix and separation of concerns (template/styles).
10-12: LGTM! Excellent migration to modern Angular patterns
The refactoring effectively:
- Eliminates manual subscription management
- Uses modern dependency injection with
inject() - Implements reactive patterns with signals
src/test/javascript/spec/helpers/mocks/service/mock-theme.service.ts (2)
1-4: LGTM! Clean and focused imports
The imports are minimal and the mock service follows proper naming conventions.
8-9: LGTM! Consistent signal pattern implementation
The user preference management follows the same clean pattern as the theme state management, with proper handling of undefined states.
src/main/webapp/app/shared/metis/emoji/emoji-picker.component.ts (3)
1-9: LGTM! Proper adoption of Angular 18 features
The imports and component setup correctly incorporate Angular 18's computed and inject features while adhering to the project's coding guidelines.
19-20:
Review the light theme emoji URL function implementation
The singleImageFunction returns an empty string function for light theme, which seems incorrect. Consider using EmojiUtils.singleLightModeEmojiUrlFn (if available) or the appropriate light theme URL function.
Additionally, the computed property can be written more concisely:
- singleImageFunction = computed(() => (this.dark() ? EmojiUtils.singleDarkModeEmojiUrlFn : () => ''));
+ singleImageFunction = computed(() => this.dark()
+ ? EmojiUtils.singleDarkModeEmojiUrlFn
+ : EmojiUtils.singleLightModeEmojiUrlFn);Let's verify if the light mode function exists:
#!/bin/bash
# Search for light mode emoji URL function definition
ast-grep --pattern 'singleLightModeEmojiUrlFn = $_'
rg -A 5 "singleLightModeEmojiUrlFn"10-11: Verify memory leak prevention after removal of ngOnDestroy
While the transition to inject() pattern is good, please confirm that removing ngOnDestroy doesn't lead to memory leaks.
src/main/webapp/app/exercises/programming/shared/instructions-render/step-wizard/programming-exercise-instruction-step-wizard.component.html (3)
5-5: LGTM! Correct usage of new Angular control flow syntax.
The update from *ngFor to @for aligns with Angular 18 best practices. The syntax is properly implemented with index tracking.
Line range hint 15-24: LGTM! Proper implementation of conditional icon rendering.
The @if directives are correctly used for conditional rendering, replacing *ngIf as per Angular 18 guidelines. The conditions are clear and mutually exclusive.
5-5: Verify the impact of tracking mechanism change.
The tracking mechanism has been changed from track step to track i. While using the index for tracking is valid, it might not be optimal if the steps array undergoes frequent modifications (additions/removals/reordering).
Consider using a unique identifier from the step object for tracking if the steps array is modified dynamically:
-@for (step of steps; let i = $index; track i) {
+@for (step of steps; track step.testIds) {src/test/javascript/spec/component/emoji/emoji-picker.component.spec.ts (1)
Line range hint 1-48: Verify signal implementation and test coverage
Let's verify the component's signal implementation and ensure comprehensive test coverage.
src/main/webapp/app/core/theme/theme-switch.component.html (1)
5-5: LGTM! Clean implementation of icon and translation.
The implementation correctly uses the FontAwesome icon component and the translation directive.
src/main/webapp/app/shared/dashboards/tutor-participation-graph/progress-bar/progress-bar.component.ts (3)
1-10: LGTM! Import changes align with Angular 18 practices
The migration to use toObservable from @angular/core/rxjs-interop aligns with Angular 18's signal-based reactive programming approach.
Line range hint 11-19: LGTM! Class structure follows Angular guidelines
The component properties and decorators are well-structured and follow Angular's style guide. The removal of OnInit is appropriate as the initialization logic is handled in the constructor.
Line range hint 20-30: Consider investigating the need for manual change detection
While the migration to signals is good, there are two concerns:
- The comment "Manually run change detection as it doesn't do it automatically for some reason" suggests an underlying issue that should be investigated rather than worked around.
- Setting up subscriptions in the constructor could lead to potential issues with the component's lifecycle.
Let's check if this manual change detection pattern is used elsewhere:
Consider moving the subscription setup to a lifecycle hook and investigating why automatic change detection isn't working:
-constructor(
- private themeService: ThemeService,
- private ref: ChangeDetectorRef,
-) {
- this.themeSubscription = toObservable(this.themeService.currentTheme).subscribe(() => {
- this.chooseProgressBarTextColor();
- this.ref.detectChanges();
- });
-}
+constructor(
+ private themeService: ThemeService,
+ private ref: ChangeDetectorRef,
+) {}
+
+ngAfterViewInit() {
+ this.themeSubscription = toObservable(this.themeService.currentTheme).subscribe(() => {
+ this.chooseProgressBarTextColor();
+ // Consider using markForCheck() instead if using OnPush strategy
+ this.ref.markForCheck();
+ });
+}src/test/javascript/spec/component/shared/progress-bar.component.spec.ts (1)
8-8: LGTM: Proper mock service configuration
The mock service implementation follows Angular testing best practices and aligns with our guidelines for mocking dependencies.
Also applies to: 18-23
src/test/javascript/spec/component/theme/theme-switch.component.spec.ts (2)
3-9: LGTM! Import statements follow best practices.
The imports are well-organized and follow the guidelines by:
- Using appropriate testing utilities
- Leveraging mock services
- Avoiding full module imports
57-67: LGTM! OS sync tests are comprehensive.
The tests effectively verify:
- Toggle state changes
- Correct theme service interactions
- Boolean state assertions using recommended methods
src/main/webapp/app/app.module.ts (2)
24-24: LGTM: Import statement follows Angular guidelines
The import of ThemeSwitchComponent from the core module follows proper Angular conventions and organizational structure.
45-45: Verify standalone component configuration
The migration to using ThemeSwitchComponent as a standalone component aligns with Angular 18 practices. However, let's verify the component's configuration.
src/main/webapp/app/exercises/programming/shared/instructions-render/service/programming-exercise-plant-uml.service.ts (2)
1-1: LGTM: Modern dependency injection pattern implemented correctly
The switch to functional dependency injection using inject() aligns well with Angular 18's best practices and improves code maintainability.
Also applies to: 15-16
Line range hint 39-45: Verify cache management strategy
The service implements caching with a 1-hour expiration and 100-item limit. For high-traffic applications, we should verify if these limits are appropriate and if memory usage is properly managed.
Also applies to: 59-65
✅ Verification successful
Cache configuration is appropriate
The current cache configuration with 100 items and 1-hour expiration is well-suited for PlantUML diagrams. The sliding expiration and theme-based cache eviction provide effective memory management, while server-side caching adds another optimization layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cache usage patterns and memory management
rg -A 2 "@Cacheable"
rg "maxCacheCount|maxAge"
# Look for similar caching implementations
ast-grep --pattern 'class $_ {
$$$
@Cacheable($$$)
$$$
}'
Length of output: 295512
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor.service.spec.ts (4)
4-4: LGTM: Import path simplification
The simplified import path improves code maintainability.
28-28: LGTM: Proper service injection in tests
Using TestBed.inject for ThemeService follows Angular testing best practices.
58-65: LGTM: Well-structured parameterized tests
The use of it.each for testing different editor types reduces code duplication and improves test maintainability. The test structure follows Jest best practices.
15-16: LGTM: Improved test organization with signals
Moving the ThemeService declaration to the top level improves test organization. The removal of BehaviorSubject aligns with the migration to signals.
Let's verify the ThemeService is using signals:
src/main/webapp/app/shared/monaco-editor/monaco-editor.service.ts (1)
17-17: Verify ThemeService.currentTheme is a signal
Since this property is used within an effect, we need to ensure that ThemeService.currentTheme is actually a signal.
src/test/javascript/spec/service/theme.service.spec.ts (2)
13-13: LGTM: Mock declarations follow best practices
The mock declarations are well-structured and follow Jest best practices with consistent naming conventions.
Also applies to: 39-39
106-109: LGTM: Theme restoration test properly handles signals
The test correctly uses TestBed.flushEffects() to handle signal updates and verifies the expected behavior.
docs/dev/guidelines/client-design.rst (1)
240-240: LGTM! Method name change improves clarity.
The rename from applyThemeExplicitly to applyThemePreference better reflects that this method handles user theme preferences.
src/test/javascript/spec/integration/guided-tour/guided-tour.integration.spec.ts (1)
Line range hint 117-134: Verify mock implementation completeness.
The test uses several mock services and spies. Ensure all necessary methods and properties are properly mocked, especially for theme-related functionality.
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html (3)
Line range hint 132-138: LGTM! Proper implementation of Angular signals and new control flow syntax.
The changes correctly implement:
- Signal-based reactive properties using function calls
- New @if syntax as per coding guidelines
163-163: LGTM! Consistent use of signal pattern.
The change maintains consistency with the signal-based approach used throughout the component.
Line range hint 1-163: Excellent migration to new Angular control flow syntax!
The template consistently uses the new @if and @for syntax throughout, following the coding guidelines perfectly. This thorough migration will serve as a good example for other components.
src/main/webapp/app/exercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts (2)
14-14: LGTM: Import statements follow Angular 18 practices
The addition of inject and toObservable imports aligns with Angular 18's modern dependency injection patterns and signal-to-observable conversion capabilities.
Also applies to: 43-43
51-52: LGTM: Theme service injection follows Angular 18 best practices
The use of inject() for ThemeService follows Angular 18's recommended dependency injection pattern, improving code maintainability and reducing boilerplate.
src/test/javascript/spec/component/shared/navbar.component.spec.ts (1)
231-246: LGTM! Well-structured breadcrumb test cases.
The breadcrumb test updates follow consistent patterns and properly verify:
- Translation keys
- URI formatting
- Breadcrumb hierarchy
Also applies to: 257-266, 279-288, 299-303, 405-409, 434-438, 479-488, 552-556, 592-596, 627-636, 660-664, 692-701, 737-746, 767-770
src/main/webapp/app/core/theme/theme-switch.component.ts (1)
30-31: Verify the usage of 'input.required' and 'viewChild.required' functions
Please confirm that using input.required<PlacementArray>() and viewChild.required<NgbPopover>('popover') is appropriate in Angular 18. Traditionally, inputs and view children are declared using the @Input() and @ViewChild() decorators and accessed as properties rather than functions. Also, calling this.popover() suggests that popover is a function, which may not be intended.
Also applies to: 49-49, 55-55
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.ts (4)
1-1: Imports updated to include Angular reactive features
The import statement now includes computed, effect, inject, signal, and untracked from @angular/core, which aligns with the adoption of Angular 18 reactive programming practices.
43-43: Using inject for dependency injection enhances code clarity
The use of inject(ThemeService) provides a modern and concise way to inject services in Angular components, aligning with Angular 18 standards.
118-130: Effect correctly updates highlighted elements when theme changes
The effect set up in the constructor ensures that highlightedElements are updated whenever highlightColor changes. The use of untracked prevents unnecessary reactive dependencies, which is appropriate in this context.
478-482: Ensure feedback.referenceId is always defined to prevent runtime errors
When setting highlightedElements, you use feedback.referenceId! with the non-null assertion operator. Verify that feedback.referenceId cannot be null or undefined to prevent potential runtime exceptions.
If there's a possibility that referenceId could be undefined, consider adding a check:
for (const feedback of missedReferencedExampleFeedbacks) {
- highlightedElements.set(feedback.referenceId!, this.highlightColor());
+ if (feedback.referenceId) {
+ highlightedElements.set(feedback.referenceId, this.highlightColor());
+ }
}This ensures that only valid referenceId values are used when updating highlightedElements.
...ercises/programming/shared/instructions-render/programming-exercise-instruction.component.ts
Show resolved
Hide resolved
...cript/spec/component/programming-exercise/programming-exercise-instruction.component.spec.ts
Show resolved
Hide resolved
FelixTJDietrich
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
FelixTJDietrich
changed the title
Development: Update theme components to use Angular 18 practicesDevelopment: Update theme switcher to use Angular 18 practices
…of github.com:ls1intum/Artemis into chore/performance/angular-18-signal-migration-example
github-actions
bot
removed
the
core
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/javascript/spec/helpers/mocks/mock-instance.helper.ts (1)
24-29: Add JSDoc documentation for consistencyThe function is missing JSDoc documentation unlike the existing
mockCodeEditorMonacoViewChildrenfunction. Please add documentation following the same pattern.+/** + * Overwrites the signal-based viewChild queries of the ThemeSwitchComponent so it can be mocked using MockComponent. + * Workaround for https://github.com/help-me-mom/ng-mocks/issues/8634. + */ export function mockThemeSwitcherComponentViewChildren() {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/test/javascript/spec/component/shared/navbar.component.spec.ts(19 hunks)src/test/javascript/spec/helpers/mocks/mock-instance.helper.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/javascript/spec/component/shared/navbar.component.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/helpers/mocks/mock-instance.helper.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 (2)
src/test/javascript/spec/helpers/mocks/mock-instance.helper.ts (2)
4-4: LGTM: Import statement follows conventions
The import statement is correctly placed and follows project conventions.
26-28: Verify complete signal-based property coverage
Please verify that all signal-based properties of ThemeSwitchComponent are properly mocked. Currently, only the popover property is mocked.
✅ Verification successful
All signal-based properties of ThemeSwitchComponent are properly mocked.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for signal-based properties in ThemeSwitchComponent
# Expected: Find all properties initialized with signal() to ensure complete mocking
# Search for signal declarations in ThemeSwitchComponent
ast-grep --pattern 'signal<$_>($_)' app/core/theme/theme-switch.component.ts
# Search for ViewChild decorators that might need mocking
ast-grep --pattern '@ViewChild($$$)' app/core/theme/theme-switch.component.ts
Length of output: 334
Script:
#!/bin/bash
# Description: Search for ThemeSwitchComponent file in the codebase
fd 'theme-switch\.component\.ts$'
Length of output: 92
Script:
#!/bin/bash
# Description: Search for signal-based properties and ViewChild decorators in ThemeSwitchComponent
ast-grep --pattern 'signal<$_>($_)' src/main/webapp/app/core/theme/theme-switch.component.ts
ast-grep --pattern '@ViewChild($$$)' src/main/webapp/app/core/theme/theme-switch.component.ts
Length of output: 190
eceeeren
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
chore/performance/angular-18-signal-migration-example
branch
Checklist
General
Client
Motivation and Context
We want to migrate to the new way of Angular 18 and potentially use this PR as example PR for the documentation.
Description
This migrates the theme switcher component and the components it affects to the latest Angular development standards with signals.
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
Performance Tests
Test Coverage
Client
Summary by CodeRabbit
Release Notes
New Features
ThemeSwitchComponentfor improved theme management.Bug Fixes
Documentation
Tests