Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Programming exercises: Fix performance bug of ide preferences in scores page #9596

Closed
wants to merge 2 commits into from

Conversation

janthoXO
Copy link
Contributor

@janthoXO janthoXO commented Oct 25, 2024

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Description

Fixes an issue where request to fetch ide preferences on the scores page where send per code button. This closes #9559.
In addition display tooltip for intelliJ IDEs directly and not in the question mark icon. Closes #9541

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Programming Exercise with more than one participation
  1. Log in to Artemis
  2. Navigate to Course Management
  3. Click on a course and a programming exercise
  4. Click on the scores button
  5. Verify that only one request to ide-preferences is sent
  6. click on one code button -> verify that an additional request to ide-preferences was made

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Test

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced enhanced interactivity with the CodeButtonComponent through a new event binding.
    • Improved handling of IDE preferences with a subscription-based approach in the IdeSettingsComponent.
  • Bug Fixes

    • Resolved potential memory leaks by implementing proper subscription management in both CodeButtonComponent and IdeSettingsComponent.
  • Style

    • Updated layout of IDE settings for better user experience by reorganizing HTML elements.
  • Documentation

    • Enhanced internal documentation to reflect changes in subscription handling and component lifecycle management.

@janthoXO janthoXO self-assigned this Oct 25, 2024
@janthoXO janthoXO requested a review from a team as a code owner October 25, 2024 19:11
@github-actions github-actions bot added tests client Pull requests that update TypeScript code. (Added Automatically!) labels Oct 25, 2024
@janthoXO janthoXO added ready for review component:Programming programming Pull requests that affect the corresponding module labels Oct 25, 2024
Copy link

coderabbitai bot commented Oct 25, 2024

Walkthrough

The pull request introduces several modifications across multiple files, primarily focusing on enhancing the interactivity and responsiveness of the CodeButtonComponent and IdeSettingsComponent. Key changes include the addition of event bindings, lifecycle hooks for subscription management, and a shift from promise-based to observable-based approaches for handling IDE preferences. Structural adjustments in the HTML and CSS files improve the user interface presentation. Additionally, updates to the service handling IDE settings streamline the management of preferences, ensuring better memory management and reactive programming practices.

Changes

File Change Summary
src/main/webapp/app/shared/components/code-button/code-button.component.html Added event binding (shown)="onShow()" to the button element.
src/main/webapp/app/shared/components/code-button/code-button.component.ts Implemented OnDestroy lifecycle hook, added Subscription instance for managing subscriptions, changed IDE preferences loading to observable, added public method onShow(), and modified the initialization of programmingLanguageToIde.
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.html Reorganized HTML structure by removing <dt> and replacing it with two <span> elements and a <br /> tag for layout improvement.
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts Implemented OnDestroy lifecycle hook, initialized PREDEFINED_IDE and programmingLanguageToIde in the constructor, updated subscription handling for loading IDE preferences, and simplified methods related to programming languages.
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.scss Removed CSS rule dt { font-weight: 500; }.
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.service.ts Changed ideSettingsUrl from readonly to public, added static property fallbackIde, introduced BehaviorSubject for IDE preferences, updated methods to use observables instead of promises, and simplified logic for loading and saving preferences.
src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts Removed fakeAsync and tick() from tests, updated to use observable subscriptions for testing IDE preferences loading.
src/test/javascript/spec/service/settings/ide-settings.service.spec.ts Removed fakeAsync and tick() from tests, updated to use observable subscriptions for testing IDE preferences loading.

Possibly related PRs

Suggested labels

tests, ready for review, client, bugfix, performance

Suggested reviewers

  • krusche
  • pzdr7
  • Jan-Thurner
  • SimonEntholzer
  • az108

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Outside diff range and nitpick comments (10)
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.html (2)

4-6: Consider using semantic HTML elements.

While the direct display of help text improves UX as intended, consider wrapping these spans in a more semantic HTML element (e.g., <div role="definition">) to maintain accessibility and semantic structure.

-        <span jhiTranslate="artemisApp.userSettings.idePreferencesPage.description"></span>
-        <br />
-        <span jhiTranslate="artemisApp.userSettings.idePreferencesPage.jetbrainsHelp"></span>
+        <div role="definition">
+            <span jhiTranslate="artemisApp.userSettings.idePreferencesPage.description"></span>
+            <br />
+            <span jhiTranslate="artemisApp.userSettings.idePreferencesPage.jetbrainsHelp"></span>
+        </div>

9-9: *Consider migrating ngTemplateOutlet to newer syntax.

While not strictly required by the guidelines, for consistency, consider migrating the *ngTemplateOutlet to the newer control flow syntax when it becomes available in future Angular versions.

Also applies to: 22-22

src/test/javascript/spec/service/settings/ide-settings.service.spec.ts (1)

Line range hint 51-83: Add error cases and enhance expectations for CRUD operations.

The existing tests are good but could be more robust with error handling and specific expectations.

Consider adding these improvements:

 it('should save IDE preference', () => {
     const programmingLanguage = ProgrammingLanguage.JAVA;
     const ide: Ide = { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' };
     const mockResponse: IdeMappingDTO = { programmingLanguage, ide };

+    let completed = false;
     service.saveIdePreference(programmingLanguage, ide).subscribe({
-        expect(savedIde).toEqual(ide);
+        next: (savedIde) => {
+            expect(savedIde).toContainEntries(Object.entries(ide));
+            expect(Object.keys(savedIde)).toHaveLength(2);
+        },
+        complete: () => { completed = true; }
     });

     const req = httpMock.expectOne((request) => request.url === service.ideSettingsUrl && request.params.get('programmingLanguage') === programmingLanguage);
     expect(req.request.method).toBe('PUT');
     req.flush(mockResponse);
+    expect(completed).toBeTrue();
 });

+it('should handle error when saving IDE preference', () => {
+    const programmingLanguage = ProgrammingLanguage.JAVA;
+    const ide: Ide = { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' };
+
+    service.saveIdePreference(programmingLanguage, ide).subscribe({
+        error: (error) => {
+            expect(error.status).toBe(400);
+        }
+    });
+
+    const req = httpMock.expectOne((request) => request.url === service.ideSettingsUrl);
+    req.flush('Invalid data', { status: 400, statusText: 'Bad Request' });
+});
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (2)

18-20: Consider adding explicit type for the WritableSignal.

For better type safety and readability, consider explicitly declaring the generic type for the WritableSignal.

-programmingLanguageToIde: WritableSignal<Map<ProgrammingLanguage, Ide>>;
+programmingLanguageToIde: WritableSignal<Map<ProgrammingLanguage, Ide>> = signal(new Map());

Line range hint 34-50: Consider adding error handling for subscriptions.

While the subscription management is good, consider adding error handling for robustness.

-const predefinedIdesSubscription = this.ideSettingsService.loadPredefinedIdes().subscribe((ides) => {
+const predefinedIdesSubscription = this.ideSettingsService.loadPredefinedIdes().subscribe({
+    next: (ides) => {
         this.PREDEFINED_IDE = ides;
+    },
+    error: (error) => {
+        console.error('Failed to load predefined IDEs:', error);
+        // Consider showing a user-friendly error message
+    }
});
src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts (1)

Line range hint 1-8: Replace CUSTOM_ELEMENTS_SCHEMA with explicit component declarations.

The use of CUSTOM_ELEMENTS_SCHEMA is discouraged as it can hide potential template errors. Instead, use NgMocks to mock child components.

Apply these changes:

-import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core';
+import { MockComponent } from 'ng-mocks';
 import { of } from 'rxjs';
-import { IdeSettingsComponent } from 'app/shared/user-settings/ide-preferences/ide-settings.component';
+import { IdeSettingsComponent } from '@shared/user-settings/ide-preferences/ide-settings.component';

And update the TestBed configuration:

 declarations: [
   IdeSettingsComponent,
+  // Add mocked child components here using MockComponent
 ],
-schemas: [CUSTOM_ELEMENTS_SCHEMA],
src/main/webapp/app/shared/components/code-button/code-button.component.html (1)

Line range hint 1-108: Consider refactoring template for better maintainability.

The template could benefit from the following improvements:

  1. Move inline styles to CSS files (e.g., style="margin: 0; max-width: 100%", style="min-width: 100px").
  2. Consider breaking down the large template into smaller, reusable components:
    • Repository URL section
    • IDE buttons section
    • Clone options section

Example refactor for the repository URL section:

// repository-url.component.html
<div class="d-flex repository-url-container">
    <ng-container *ngTemplateOutlet="sshDropdown"></ng-container>
    <pre
        class="clone-url"
        [ngClass]="urlBoxClasses"
        [cdkCopyToClipboard]="repositoryUri"
        (cdkCopyToClipboardCopied)="onCopy($event)"
    >{{ repositoryUri }}</pre>
    <ng-container *ngTemplateOutlet="repositoryLinks"></ng-container>
</div>
// repository-url.component.scss
.repository-url-container {
    margin: 0;
    max-width: 100%;
}

.clone-url {
    &.url-box-remove-line-left {
        // styles
    }
    &.url-box-remove-line-right {
        // styles
    }
}
src/main/webapp/app/shared/components/code-button/code-button.component.ts (2)

65-67: Consider adding type safety to the Map declaration

While the Map implementation is correct, consider explicitly declaring the value type for better type safety.

-    programmingLanguageToIde: Map<ProgrammingLanguage, Ide>;
+    programmingLanguageToIde: Map<ProgrammingLanguage, Ide> = new Map<ProgrammingLanguage, Ide>();

342-342: Consider adding null check for fallbackIde

While using the static fallbackIde is correct, consider adding a null check to handle cases where the fallbackIde might not be initialized.

-            IdeSettingsService.fallbackIde
+            IdeSettingsService.fallbackIde ?? { id: 'default', name: 'Default IDE' }
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.service.ts (1)

16-16: Remove unused 'error' property

The error property is declared but never used within the service. Removing it can clean up the code.

Apply this diff to remove the unused property:

-    error?: string;
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 142c0f1 and 776eefd.

📒 Files selected for processing (8)
  • src/main/webapp/app/shared/components/code-button/code-button.component.html (1 hunks)
  • src/main/webapp/app/shared/components/code-button/code-button.component.ts (6 hunks)
  • src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.html (1 hunks)
  • src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (3 hunks)
  • src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.scss (0 hunks)
  • src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.service.ts (3 hunks)
  • src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts (2 hunks)
  • src/test/javascript/spec/service/settings/ide-settings.service.spec.ts (2 hunks)
💤 Files with no reviewable changes (1)
  • src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.scss
🧰 Additional context used
📓 Path-based instructions (7)
src/main/webapp/app/shared/components/code-button/code-button.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/components/code-button/code-button.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.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/user-settings/ide-preferences/ide-settings.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.service.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/javascript/spec/component/settings/ide/ide-settings.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/service/settings/ide-settings.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 (11)
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.html (2)

Line range hint 12-12: LGTM! Proper usage of new Angular control flow syntax.

The template correctly implements the new @for and @if syntax as per coding guidelines.

Also applies to: 26-26, 41-41


Line range hint 1-54: Verify component integration.

Let's ensure all template dependencies are properly defined in the component class.

✅ Verification successful

Template dependencies are properly integrated with component

All required methods and properties from the template are correctly defined in the component class:

  • Properties:

    • PREDEFINED_IDE: Ide[]
    • assignedProgrammingLanguages: ProgrammingLanguage[]
    • remainingProgrammingLanguages: ProgrammingLanguage[]
  • Methods:

    • isIdeOfProgrammingLanguage(programmingLanguage: ProgrammingLanguage, ide: Ide)
    • changeIde(programmingLanguage: ProgrammingLanguage, ide: Ide)
    • removeProgrammingLanguage(programmingLanguage: ProgrammingLanguage)
    • addProgrammingLanguage(programmingLanguage: ProgrammingLanguage)

The presence of unit tests in ide-settings.component.spec.ts further confirms the proper implementation and testing of these dependencies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all methods and properties used in the template are defined in the component

# Test: Search for required method and property definitions
ast-grep --pattern 'class IdeSettingsComponent {
  $$$
  PREDEFINED_IDE$_
  $$$
}'

ast-grep --pattern 'class IdeSettingsComponent {
  $$$
  isIdeOfProgrammingLanguage($_) {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class IdeSettingsComponent {
  $$$
  changeIde($_) {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class IdeSettingsComponent {
  $$$
  removeProgrammingLanguage($_) {
    $$$
  }
  $$$
}'

ast-grep --pattern 'class IdeSettingsComponent {
  $$$
  addProgrammingLanguage($_) {
    $$$
  }
  $$$
}'

Length of output: 1134


Script:

#!/bin/bash
# Let's try a different approach using ripgrep to find the component file and its contents

# First, find the component file
fd -e ts "ide-settings.component.ts"

# Then let's examine its contents
rg -A 5 "export class IdeSettingsComponent" --type ts

# Look for specific method definitions
rg "isIdeOfProgrammingLanguage|changeIde|removeProgrammingLanguage|addProgrammingLanguage|PREDEFINED_IDE" --type ts

# Look for property definitions
rg "assignedProgrammingLanguages|remainingProgrammingLanguages" --type ts

Length of output: 5527

src/test/javascript/spec/service/settings/ide-settings.service.spec.ts (1)

Line range hint 1-36: LGTM! Well-structured test setup.

The test setup follows best practices with:

  • Proper HTTP request mocking
  • Clean teardown after each test
  • Focused imports
src/main/webapp/app/shared/user-settings/ide-preferences/ide-settings.component.ts (4)

Line range hint 1-14: LGTM! Well-structured component setup.

The component follows Angular best practices with OnPush change detection and proper lifecycle hook implementations. The use of signals shows good adoption of modern Angular features.


26-31: LGTM! Good subscription management and initialization.

The subscription management and constructor initialization follow best practices:

  • Proper subscription object initialization
  • Service injection and initialization in constructor
  • Using signals for reactive state management

53-55: LGTM! Proper cleanup implementation.

The ngOnDestroy implementation correctly handles subscription cleanup, preventing memory leaks.


Line range hint 1-67: Verify performance improvement in the scores page.

The changes align well with the PR objectives by implementing a subscription-based approach for IDE preferences. To verify the performance improvement:

src/test/javascript/spec/component/settings/ide/ide-settings.component.spec.ts (1)

37-57: ⚠️ Potential issue

Add subscription cleanup to prevent memory leaks.

The test should clean up any subscriptions to prevent memory leaks during testing.

Add subscription management:

 describe('IdeSettingsComponent', () => {
+  let subscriptions: Subscription[] = [];
+
+  afterEach(() => {
+    subscriptions.forEach(sub => sub.unsubscribe());
+    subscriptions = [];
+  });

   it('should load predefined IDEs and IDE preferences on init', () => {
     // ... existing test code ...
-    component.ngOnInit();
+    subscriptions.push(
+      // Capture the subscription
+      component.ngOnInit()
+    );
     // ... rest of the test ...
   });
 });

Likely invalid or redundant comment.

src/main/webapp/app/shared/components/code-button/code-button.component.html (2)

15-15: LGTM: Event binding optimizes IDE preferences fetching.

The addition of (shown)="onShow()" aligns with the PR objective to prevent multiple unnecessary requests for IDE preferences. This ensures the preferences are only fetched when the button/popover is actually shown to the user.


Line range hint 19-91: LGTM: Template follows new Angular syntax guidelines.

The template consistently uses the new @if syntax instead of *ngIf throughout all conditional blocks, which aligns with the coding guidelines.

src/main/webapp/app/shared/components/code-button/code-button.component.ts (1)

1-1: LGTM: Proper lifecycle hook implementation

The addition of OnDestroy and Subscription imports, along with implementing the OnDestroy interface, follows Angular best practices for subscription management.

Also applies to: 19-19, 26-26

Comment on lines +37 to +50
it('should load IDE preferences', () => {
const mockIdeMappingDTO: IdeMappingDTO[] = [
{ programmingLanguage: ProgrammingLanguage.JAVA, ide: { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' } },
];
const expectedMap = new Map<ProgrammingLanguage, Ide>([[ProgrammingLanguage.JAVA, { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' }]]);

service.loadIdePreferences().then((ideMap) => {
service.loadIdePreferences().subscribe((ideMap) => {
expect(ideMap).toEqual(expectedMap);
});

tick();

const req = httpMock.expectOne(service.ideSettingsUrl);
expect(req.request.method).toBe('GET');
req.flush(mockIdeMappingDTO);
}));
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage and expectations specificity.

While the basic test flow is correct, consider these improvements:

  1. Use more specific expectations as per guidelines
  2. Add error case testing
  3. Verify subscription completion

Here's how to improve the test:

 it('should load IDE preferences', () => {
     const mockIdeMappingDTO: IdeMappingDTO[] = [
         { programmingLanguage: ProgrammingLanguage.JAVA, ide: { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' } },
     ];
     const expectedMap = new Map<ProgrammingLanguage, Ide>([[ProgrammingLanguage.JAVA, { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' }]]);

-    service.loadIdePreferences().subscribe((ideMap) => {
-        expect(ideMap).toEqual(expectedMap);
-    });
+    let completed = false;
+    service.loadIdePreferences().subscribe({
+        next: (ideMap) => {
+            expect(ideMap).toContainEntries([...expectedMap.entries()]);
+            expect(ideMap.size).toBe(1);
+            expect(ideMap.get(ProgrammingLanguage.JAVA)).toEqual(expectedMap.get(ProgrammingLanguage.JAVA));
+        },
+        complete: () => { completed = true; }
+    });

     const req = httpMock.expectOne(service.ideSettingsUrl);
     expect(req.request.method).toBe('GET');
     req.flush(mockIdeMappingDTO);
+    expect(completed).toBeTrue();
 });

+it('should handle error when loading IDE preferences', () => {
+    service.loadIdePreferences().subscribe({
+        error: (error) => {
+            expect(error.status).toBe(500);
+        }
+    });
+
+    const req = httpMock.expectOne(service.ideSettingsUrl);
+    req.flush('Error', { status: 500, statusText: 'Server Error' });
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should load IDE preferences', () => {
const mockIdeMappingDTO: IdeMappingDTO[] = [
{ programmingLanguage: ProgrammingLanguage.JAVA, ide: { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' } },
];
const expectedMap = new Map<ProgrammingLanguage, Ide>([[ProgrammingLanguage.JAVA, { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' }]]);
service.loadIdePreferences().then((ideMap) => {
service.loadIdePreferences().subscribe((ideMap) => {
expect(ideMap).toEqual(expectedMap);
});
tick();
const req = httpMock.expectOne(service.ideSettingsUrl);
expect(req.request.method).toBe('GET');
req.flush(mockIdeMappingDTO);
}));
});
it('should load IDE preferences', () => {
const mockIdeMappingDTO: IdeMappingDTO[] = [
{ programmingLanguage: ProgrammingLanguage.JAVA, ide: { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' } },
];
const expectedMap = new Map<ProgrammingLanguage, Ide>([[ProgrammingLanguage.JAVA, { name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' }]]);
let completed = false;
service.loadIdePreferences().subscribe({
next: (ideMap) => {
expect(ideMap).toContainEntries([...expectedMap.entries()]);
expect(ideMap.size).toBe(1);
expect(ideMap.get(ProgrammingLanguage.JAVA)).toEqual(expectedMap.get(ProgrammingLanguage.JAVA));
},
complete: () => { completed = true; }
});
const req = httpMock.expectOne(service.ideSettingsUrl);
expect(req.request.method).toBe('GET');
req.flush(mockIdeMappingDTO);
expect(completed).toBeTrue();
});
it('should handle error when loading IDE preferences', () => {
service.loadIdePreferences().subscribe({
error: (error) => {
expect(error.status).toBe(500);
}
});
const req = httpMock.expectOne(service.ideSettingsUrl);
req.flush('Error', { status: 500, statusText: 'Server Error' });
});

Comment on lines +57 to 67
addProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
this.ideSettingsService.saveIdePreference(programmingLanguage, this.PREDEFINED_IDE[0]).subscribe();
}

changeIde(programmingLanguage: ProgrammingLanguage, ide: Ide) {
this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe((ide) => {
this.programmingLanguageToIde.update((map) => new Map(map.set(programmingLanguage, ide)));
});
this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe();
}

removeProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe(() => {
const programmingLanguageToIdeMap: Map<ProgrammingLanguage, Ide> = new Map(this.programmingLanguageToIde());
programmingLanguageToIdeMap.delete(programmingLanguage);

this.programmingLanguageToIde.set(programmingLanguageToIdeMap);

this.remainingProgrammingLanguages.push(programmingLanguage);
this.assignedProgrammingLanguages = this.assignedProgrammingLanguages.filter((x) => x !== programmingLanguage);
});
this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize service method subscriptions.

The current implementation has empty subscriptions and lacks error handling. Consider using more efficient approaches.

-addProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
-    this.ideSettingsService.saveIdePreference(programmingLanguage, this.PREDEFINED_IDE[0]).subscribe();
+addProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
+    this.ideSettingsService.saveIdePreference(programmingLanguage, this.PREDEFINED_IDE[0]).subscribe({
+        error: (error) => {
+            console.error('Failed to save IDE preference:', error);
+            // Consider showing a user-friendly error message
+        }
+    });
}

-changeIde(programmingLanguage: ProgrammingLanguage, ide: Ide) {
-    this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe();
+changeIde(programmingLanguage: ProgrammingLanguage, ide: Ide) {
+    this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe({
+        error: (error) => {
+            console.error('Failed to change IDE:', error);
+            // Consider showing a user-friendly error message
+        }
+    });
}

-removeProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
-    this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe();
+removeProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
+    this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe({
+        error: (error) => {
+            console.error('Failed to remove IDE preference:', error);
+            // Consider showing a user-friendly error message
+        }
+    });
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
addProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
this.ideSettingsService.saveIdePreference(programmingLanguage, this.PREDEFINED_IDE[0]).subscribe();
}
changeIde(programmingLanguage: ProgrammingLanguage, ide: Ide) {
this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe((ide) => {
this.programmingLanguageToIde.update((map) => new Map(map.set(programmingLanguage, ide)));
});
this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe();
}
removeProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe(() => {
const programmingLanguageToIdeMap: Map<ProgrammingLanguage, Ide> = new Map(this.programmingLanguageToIde());
programmingLanguageToIdeMap.delete(programmingLanguage);
this.programmingLanguageToIde.set(programmingLanguageToIdeMap);
this.remainingProgrammingLanguages.push(programmingLanguage);
this.assignedProgrammingLanguages = this.assignedProgrammingLanguages.filter((x) => x !== programmingLanguage);
});
this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe();
}
addProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
this.ideSettingsService.saveIdePreference(programmingLanguage, this.PREDEFINED_IDE[0]).subscribe({
error: (error) => {
console.error('Failed to save IDE preference:', error);
// Consider showing a user-friendly error message
}
});
}
changeIde(programmingLanguage: ProgrammingLanguage, ide: Ide) {
this.ideSettingsService.saveIdePreference(programmingLanguage, ide).subscribe({
error: (error) => {
console.error('Failed to change IDE:', error);
// Consider showing a user-friendly error message
}
});
}
removeProgrammingLanguage(programmingLanguage: ProgrammingLanguage) {
this.ideSettingsService.deleteIdePreference(programmingLanguage).subscribe({
error: (error) => {
console.error('Failed to remove IDE preference:', error);
// Consider showing a user-friendly error message
}
});
}

Comment on lines +37 to +57
it('should load predefined IDEs and IDE preferences on init', () => {
const predefinedIdes = [
{ name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' },
{ name: 'IntelliJ', deepLink: 'jetbrains://idea/checkout/git?idea.required.plugins.id=Git4Idea&checkout.repo={cloneUrl}' },
];
const idePreferences = new Map([[ProgrammingLanguage.JAVA, predefinedIdes[0]]]);
const loadedIdePreferences = new Map([
[ProgrammingLanguage.JAVA, predefinedIdes[0]],
[ProgrammingLanguage.EMPTY, predefinedIdes[0]],
]);

mockIdeSettingsService.loadPredefinedIdes.mockReturnValue(of(predefinedIdes));
mockIdeSettingsService.loadIdePreferences.mockReturnValue(Promise.resolve(idePreferences));
mockIdeSettingsService.loadIdePreferences.mockReturnValue(of(idePreferences));

component.ngOnInit();

tick();

expect(mockIdeSettingsService.loadPredefinedIdes).toHaveBeenCalledOnce();
expect(mockIdeSettingsService.loadIdePreferences).toHaveBeenCalledOnce();
expect(component.PREDEFINED_IDE).toEqual(predefinedIdes);
expect(component.programmingLanguageToIde()).toEqual(loadedIdePreferences);
expect(component.programmingLanguageToIde()).toEqual(idePreferences);
expect(component.assignedProgrammingLanguages).toEqual([ProgrammingLanguage.JAVA]);
expect(component.remainingProgrammingLanguages).toEqual(
Object.values(ProgrammingLanguage).filter((x) => x !== ProgrammingLanguage.JAVA && x !== ProgrammingLanguage.EMPTY),
);
}));
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test specificity and structure.

While the test is generally well-structured, there are a few improvements we can make to align with best practices:

  1. The test should verify the exact structure of the Map entries
  2. The filtering logic test could be more specific

Consider updating the assertions:

-expect(component.programmingLanguageToIde()).toEqual(idePreferences);
+expect(component.programmingLanguageToIde()).toContainEntries([
+  [ProgrammingLanguage.JAVA, predefinedIdes[0]]
+]);

-expect(component.remainingProgrammingLanguages).toEqual(
-  Object.values(ProgrammingLanguage).filter((x) => x !== ProgrammingLanguage.JAVA && x !== ProgrammingLanguage.EMPTY),
-);
+const expectedLanguages = Object.values(ProgrammingLanguage)
+  .filter((x) => x !== ProgrammingLanguage.JAVA && x !== ProgrammingLanguage.EMPTY);
+expect(component.remainingProgrammingLanguages).toEqual(expectedLanguages);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should load predefined IDEs and IDE preferences on init', () => {
const predefinedIdes = [
{ name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' },
{ name: 'IntelliJ', deepLink: 'jetbrains://idea/checkout/git?idea.required.plugins.id=Git4Idea&checkout.repo={cloneUrl}' },
];
const idePreferences = new Map([[ProgrammingLanguage.JAVA, predefinedIdes[0]]]);
const loadedIdePreferences = new Map([
[ProgrammingLanguage.JAVA, predefinedIdes[0]],
[ProgrammingLanguage.EMPTY, predefinedIdes[0]],
]);
mockIdeSettingsService.loadPredefinedIdes.mockReturnValue(of(predefinedIdes));
mockIdeSettingsService.loadIdePreferences.mockReturnValue(Promise.resolve(idePreferences));
mockIdeSettingsService.loadIdePreferences.mockReturnValue(of(idePreferences));
component.ngOnInit();
tick();
expect(mockIdeSettingsService.loadPredefinedIdes).toHaveBeenCalledOnce();
expect(mockIdeSettingsService.loadIdePreferences).toHaveBeenCalledOnce();
expect(component.PREDEFINED_IDE).toEqual(predefinedIdes);
expect(component.programmingLanguageToIde()).toEqual(loadedIdePreferences);
expect(component.programmingLanguageToIde()).toEqual(idePreferences);
expect(component.assignedProgrammingLanguages).toEqual([ProgrammingLanguage.JAVA]);
expect(component.remainingProgrammingLanguages).toEqual(
Object.values(ProgrammingLanguage).filter((x) => x !== ProgrammingLanguage.JAVA && x !== ProgrammingLanguage.EMPTY),
);
}));
});
it('should load predefined IDEs and IDE preferences on init', () => {
const predefinedIdes = [
{ name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' },
{ name: 'IntelliJ', deepLink: 'jetbrains://idea/checkout/git?idea.required.plugins.id=Git4Idea&checkout.repo={cloneUrl}' },
];
const idePreferences = new Map([[ProgrammingLanguage.JAVA, predefinedIdes[0]]]);
mockIdeSettingsService.loadPredefinedIdes.mockReturnValue(of(predefinedIdes));
mockIdeSettingsService.loadIdePreferences.mockReturnValue(of(idePreferences));
component.ngOnInit();
expect(mockIdeSettingsService.loadPredefinedIdes).toHaveBeenCalledOnce();
expect(mockIdeSettingsService.loadIdePreferences).toHaveBeenCalledOnce();
expect(component.PREDEFINED_IDE).toEqual(predefinedIdes);
expect(component.programmingLanguageToIde()).toContainEntries([
[ProgrammingLanguage.JAVA, predefinedIdes[0]]
]);
expect(component.assignedProgrammingLanguages).toEqual([ProgrammingLanguage.JAVA]);
const expectedLanguages = Object.values(ProgrammingLanguage)
.filter((x) => x !== ProgrammingLanguage.JAVA && x !== ProgrammingLanguage.EMPTY);
expect(component.remainingProgrammingLanguages).toEqual(expectedLanguages);
});

Comment on lines +159 to +161
public onShow() {
this.ideSettingsService.loadIdePreferences().subscribe();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve subscription management in onShow method

The current implementation may lead to memory leaks as the subscription isn't managed. Consider adding proper subscription management and error handling.

     public onShow() {
-        this.ideSettingsService.loadIdePreferences().subscribe();
+        const loadPreferencesSubscription = this.ideSettingsService.loadIdePreferences()
+            .pipe(takeUntil(this.destroy$))
+            .subscribe({
+                error: (error) => console.error('Failed to load IDE preferences:', error)
+            });
+        this.subscription.add(loadPreferencesSubscription);
     }

Committable suggestion was skipped due to low confidence.

Comment on lines +155 to +157
ngOnDestroy() {
this.subscription.unsubscribe();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update ngOnDestroy to complete destroy$ Subject

If implementing the suggested destroy$ Subject, ensure it's completed in ngOnDestroy.

     ngOnDestroy() {
+        this.destroy$.next();
+        this.destroy$.complete();
         this.subscription.unsubscribe();
     }

Committable suggestion was skipped due to low confidence.

Comment on lines +49 to +59
public loadIdePreferences(): Observable<Map<ProgrammingLanguage, Ide>> {
return this.http.get<IdeMappingDTO[]>(this.ideSettingsUrl).pipe(
map((data) => {
const idePreferences = new Map<ProgrammingLanguage, Ide>(data.map((x) => [x.programmingLanguage, x.ide]));
if (!idePreferences.has(ProgrammingLanguage.EMPTY)) {
idePreferences.set(ProgrammingLanguage.EMPTY, this.predefinedIdes[0]);
}
this.idePreferencesSubject.next(idePreferences);
return idePreferences;
}),
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for 'loadIdePreferences' HTTP request

Currently, there is no error handling in the loadIdePreferences() method. Without error handling, any HTTP errors could lead to unhandled exceptions or silent failures. Consider adding error handling to manage such cases gracefully.

Would you like assistance in adding error handling to this method?

Comment on lines +53 to +55
if (!idePreferences.has(ProgrammingLanguage.EMPTY)) {
idePreferences.set(ProgrammingLanguage.EMPTY, this.predefinedIdes[0]);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure consistent use of the fallback IDE

Using this.predefinedIdes[0] assumes that the first element is always the fallback IDE. If the predefined IDEs loaded from the server do not include the fallback IDE or the order changes, this assumption may not hold.

Apply this diff to use fallbackIde directly:

-            idePreferences.set(ProgrammingLanguage.EMPTY, this.predefinedIdes[0]);
+            idePreferences.set(ProgrammingLanguage.EMPTY, IdeSettingsService.fallbackIde);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!idePreferences.has(ProgrammingLanguage.EMPTY)) {
idePreferences.set(ProgrammingLanguage.EMPTY, this.predefinedIdes[0]);
}
if (!idePreferences.has(ProgrammingLanguage.EMPTY)) {
idePreferences.set(ProgrammingLanguage.EMPTY, IdeSettingsService.fallbackIde);
}

import { HttpClient, HttpParams, HttpResponse } from '@angular/common/http';
import { Ide, IdeMappingDTO } from 'app/shared/user-settings/ide-preferences/ide.model';
import { ProgrammingLanguage } from 'app/entities/programming/programming-exercise.model';

@Injectable({ providedIn: 'root' })
export class IdeSettingsService {
public readonly ideSettingsUrl = 'api/ide-settings';
public ideSettingsUrl = 'api/ide-settings';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making 'ideSettingsUrl' readonly again

The ideSettingsUrl property does not appear to be modified after its initial assignment. Keeping it readonly can prevent unintended changes.

Apply this diff to make ideSettingsUrl readonly:

-    public ideSettingsUrl = 'api/ide-settings';
+    public readonly ideSettingsUrl = 'api/ide-settings';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public ideSettingsUrl = 'api/ide-settings';
public readonly ideSettingsUrl = 'api/ide-settings';

Comment on lines +88 to +93
map((response) => {
const prev = new Map(this.idePreferencesSubject.getValue());
prev.delete(programmingLanguage);
this.idePreferencesSubject.next(prev);
return response;
}),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use 'tap' operator for side effects in 'deleteIdePreference'

Similar to saveIdePreference, side effects in deleteIdePreference should be placed within a tap operator to align with best practices.

Apply this diff to use tap for the side effect:

 return this.http.delete<void>(this.ideSettingsUrl, { params, observe: 'response' }).pipe(
-    map((response) => {
+    tap(() => {
         const prev = new Map(this.idePreferencesSubject.getValue());
         prev.delete(programmingLanguage);
         this.idePreferencesSubject.next(prev);
-        return response;
-    }),
+    }),
 );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
map((response) => {
const prev = new Map(this.idePreferencesSubject.getValue());
prev.delete(programmingLanguage);
this.idePreferencesSubject.next(prev);
return response;
}),
tap(() => {
const prev = new Map(this.idePreferencesSubject.getValue());
prev.delete(programmingLanguage);
this.idePreferencesSubject.next(prev);
}),

Comment on lines +72 to +74
const prev = new Map(this.idePreferencesSubject.getValue());
prev.set(programmingLanguage, res.ide);
this.idePreferencesSubject.next(prev);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use 'tap' operator for side effects in 'saveIdePreference'

When performing side effects like updating idePreferencesSubject, it's more appropriate to use the tap operator instead of map. The map operator should be reserved for transforming values.

Apply this diff to use tap for the side effect:

 return this.http.put<IdeMappingDTO>(this.ideSettingsUrl, ide, { params }).pipe(
-    map((res) => {
+    tap((res) => {
         const prev = new Map(this.idePreferencesSubject.getValue());
         prev.set(programmingLanguage, res.ide);
         this.idePreferencesSubject.next(prev);
-        return res.ide;
-    }),
+    }),
+    map((res) => res.ide),
 );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const prev = new Map(this.idePreferencesSubject.getValue());
prev.set(programmingLanguage, res.ide);
this.idePreferencesSubject.next(prev);
return this.http.put<IdeMappingDTO>(this.ideSettingsUrl, ide, { params }).pipe(
tap((res) => {
const prev = new Map(this.idePreferencesSubject.getValue());
prev.set(programmingLanguage, res.ide);
this.idePreferencesSubject.next(prev);
}),
map((res) => res.ide),
);

Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS3. When I clicked on the Scores button, it seems that no fetch request for ide-settings was made. After clicking on the code button, one fetch request for ide-settings was made, and another one (xhr)

Exercises._.Practical.Course_.Interactive.Learning.WS24_25.-.Google.Chrome.2024-10-26.21-32-44.mp4

protected readonly ProgrammingLanguage = ProgrammingLanguage;
protected readonly faPlus = faPlus;
protected readonly faTrash = faTrash;
PREDEFINED_IDE: Ide[] = [{ name: 'VS Code', deepLink: 'vscode://vscode.git/clone?url={cloneUrl}' }];
PREDEFINED_IDE: Ide[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't use SCREAMING_SNAKE_CASE here, as it's not a constant.
https://docs.artemis.cit.tum.de/dev/guidelines/server.html#naming-convention

Comment on lines +33 to +37
return this.http.get<Ide[]>(this.ideSettingsUrl + '/predefined').pipe(
map((ides) => {
this.predefinedIdes = ides;
return ides;
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use tap instead of map here, since you return the exact input anyway. This way you can get rid of the return

Comment on lines +87 to +92
return this.http.delete<void>(this.ideSettingsUrl, { params, observe: 'response' }).pipe(
map((response) => {
const prev = new Map(this.idePreferencesSubject.getValue());
prev.delete(programmingLanguage);
this.idePreferencesSubject.next(prev);
return response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, you can also just use tap, even without the response parameter, since you don't interact with it here

Copy link

@alekspetrov9e alekspetrov9e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Here is a screenshot of my network traffic after I followed the instructions.The first 2 with ide-settings were after i clicked scores and the second two were after i clicked the code button.This behavior is consistent with the fix: a single initial request on loading the scores page, followed by additional requests only when an action (like clicking "Code") explicitly triggers them. I hope this helps! Have a nice day!

Copy link

github-actions bot commented Nov 3, 2024

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@janthoXO janthoXO marked this pull request as draft November 8, 2024 19:31
@github-actions github-actions bot removed the stale label Nov 9, 2024
Copy link

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 16, 2024
@github-actions github-actions bot closed this Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) component:Programming programming Pull requests that affect the corresponding module stale tests
Projects
Status: Ready For Review
5 participants