chore/sc-38231/refactor-clark-builder-to-enforce-stricter #2129

joaquinVill · 2025-12-09T20:41:30Z

Adds per‑keystroke name availability checks on the Info page and simplifies builder‑store error handling. The builder now checks name availability on every keystroke using the new name check route and throws errors based off that routes response.

Video:

Screen.Recording.2025-12-09.at.3.51.17.PM.mov

dsoto18 · 2025-12-11T16:48:53Z

src/app/onion/learning-object-builder/builder-store.service.ts

-        if (e.status === 409) {
-          // tried to save an object with a name that already exists
-          this.validator.errors.saveErrors.set(
-            'name',
-            'A learning object with this name already exists! The title should be unique within your learning objects.'
-          );
-          this.handleServiceError(e, BUILDER_ERRORS.DUPLICATE_OBJECT_NAME);
-        } else if (e.status === 400) {
-          this.validator.errors.saveErrors.set(
-            'name',
-            e.error.message
-          );
-          this.handleServiceError(e, BUILDER_ERRORS.SPECIAL_CHARACTER_NAME);
-        } else {
-          this.handleServiceError(e, BUILDER_ERRORS.CREATE_OBJECT);
-        }


just wondering, do these errors not show up anymore? Were they not really needed or being used? Hard to tell what its for, but it looks like one was checking a bad request if the name had a special character in it? If that's the case, it might be worth seeing how we can keep this in.

@dsoto18 yea I think you are right. I am going to keep the special character check.

dsoto18 · 2025-12-11T16:51:10Z

Looks pretty good to me just had a question. Also in the video I noticed we were copy and pasting names into the text box, hows this perform with just as a user is typing. Is it always sending requests to the route for each character the user types, and how is the response displayed?

side note I think we accidentally used the wrong branch name on this too lol. But no big deal

joaquinVill · 2025-12-12T15:49:02Z

Name checking when typing

Screen.Recording.2025-12-12.at.10.47.52.AM.mov

Copilot

Pull request overview

This PR refactors the learning object builder to implement per-keystroke name availability checks on the Info page and simplifies error handling in the builder store. The changes introduce real-time duplicate name validation by calling a new API endpoint on every keystroke, while removing the previous server-side error handling for duplicate names (409) and special character validation (400) from the save/update flows.

Key changes:

Adds reactive per-keystroke name availability checking using RxJS operators
Introduces a new checkNameAvailability API endpoint and service method
Removes duplicate name and special character error handling from builder-store's create/update methods

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts	Implements per-keystroke name availability checking with new `nameChanges$` Subject, `onNameInput()` method, and `checkNameAvailability()` method using RxJS operators
src/app/onion/learning-object-builder/pages/info-page/info-page.component.html	Updates the name input field to call `onNameInput()` instead of directly calling `mutateLearningObject()`
src/app/onion/learning-object-builder/builder-store.service.ts	Removes specific error handling for 409 (duplicate name) and 400 (special character) errors from create and update methods, simplifying to generic error handling
src/app/core/learning-object-module/learning-object/learning-object.service.ts	Adds new `checkNameAvailability()` method that calls the name check API endpoint
src/app/core/learning-object-module/learning-object/learning-object.routes.ts	Defines the new `CHECK_NAME_AVAILABILITY` API route that accepts an encoded name parameter

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts

Copilot · 2025-12-12T19:58:22Z

src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts

+  private checkNameAvailability(name: string): Observable<void> {
+    const trimmedName = (name || '').trim();
+
+    return new Observable<void>(observer => {
+      this.learningObjectService
+        .checkNameAvailability(trimmedName)
+        .then(available => {
+          const duplicateErrorText =
+            'A learning object with this name already exists! The title should be unique within your learning objects.';
+          const currentError = this.validator.errors.saveErrors.get('name');
+
+          if (!available) {
+            this.validator.errors.saveErrors.set('name', duplicateErrorText);
+          } else if (currentError === duplicateErrorText) {
+            this.validator.errors.saveErrors.delete('name');
+          }
+
+          this.cd.markForCheck();
+          observer.next();
+          observer.complete();
+        })
+        .catch(() => {
+          observer.next();
+          observer.complete();
+        });
+    });


Creating an Observable that wraps a Promise is an anti-pattern. The learningObjectService.checkNameAvailability method returns a Promise, which should be converted to an Observable using 'from()' from RxJS instead of manually wrapping it. This would simplify the code and follow RxJS best practices. The current implementation also doesn't properly handle unsubscription during the Promise execution.

src/app/onion/learning-object-builder/builder-store.service.ts

src/app/core/learning-object-module/learning-object/learning-object.service.ts

Copilot · 2025-12-12T19:58:23Z

src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts

 import { LearningObject, User } from '@entity';
 import { COPY } from './info-page.copy';
-import { Subject } from 'rxjs';
+import { Observable, of, Subject } from 'rxjs';


Unused import of.

Suggested change

import { Observable, of, Subject } from 'rxjs';

import { Subject } from 'rxjs';

…e.component.ts Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T19:59:25Z

src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts

+  private checkNameAvailability(name: string): Observable<void> {
+    const trimmedName = (name || '').trim();
+    const duplicateErrorText =
+      'A learning object with this name already exists! The title should be unique within your learning objects.';
+
+    return from(this.learningObjectService.checkNameAvailability(trimmedName)).pipe(
+      map(available => {
+        const currentError = this.validator.errors.saveErrors.get('name');
+
+        if (!available) {
+          this.validator.errors.saveErrors.set('name', duplicateErrorText);
+        } else if (currentError === duplicateErrorText) {
+          this.validator.errors.saveErrors.delete('name');
+        }
+
+        this.cd.markForCheck();
+      }),
+      catchError(() => of(void 0))
+    );


Empty or whitespace-only names bypass validation. The checkNameAvailability method is called with trimmed names, but when the trimmed name is empty (''), the API request will still be made. This could result in unnecessary API calls or unexpected behavior. Consider adding an early return when the trimmed name is empty to avoid making API calls for invalid input.

src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts

src/app/onion/learning-object-builder/builder-store.service.ts

src/app/onion/learning-object-builder/pages/info-page/info-page.component.ts

joaquinVill · 2025-12-18T17:06:27Z

src/app/onion/learning-object-builder/builder-store.service.ts

-          );
+        if (e.status === 400) {
+          const body = typeof e.error === 'string' ? JSON.parse(e.error) : e.error;
+          const errorMsg = body?.message?.[0]?.message?.[0] ?? '';


These lines were added because the update learning object error was being parsed wrong. It would throw [object Object] when a learning object name was updated to have illegal characters in it.

Co-authored-by: Copilot <[email protected]>

joaquinVill added 3 commits December 9, 2025 12:59

routes and service changes

d94d794

working

752ca62

didnt save

032b8d3

joaquinVill requested review from a team, Cwagne17, curlyLasagna and dsoto18 December 9, 2025 20:52

dsoto18 reviewed Dec 11, 2025

View reviewed changes

joaquinVill changed the title ~~Feature/sc 37258/update get competencies to use rbac and update~~ chore/sc-38231/refactor-clark-builder-to-enforce-stricter Dec 12, 2025

Cwagne17 approved these changes Dec 12, 2025

View reviewed changes

curlyLasagna requested a review from Copilot December 12, 2025 19:51

Copilot started reviewing on behalf of curlyLasagna December 12, 2025 19:51 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

joaquinVill and others added 3 commits December 16, 2025 15:58

Update src/app/onion/learning-object-builder/pages/info-page/info-pag…

5a90be1

…e.component.ts Co-authored-by: Copilot <[email protected]>

get rid of observable and add special character checking back

5cbcb50

merge

1db9762

joaquinVill requested a review from Copilot December 17, 2025 19:54

Copilot started reviewing on behalf of joaquinVill December 17, 2025 19:55 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

joaquinVill commented Dec 18, 2025

View reviewed changes

joaquinVill and others added 2 commits December 18, 2025 12:10

Apply suggestion from @Copilot

34d9708

Co-authored-by: Copilot <[email protected]>

cleanup stuff

d9b79d5

joaquinVill requested a review from dsoto18 December 18, 2025 17:40

curlyLasagna approved these changes Jan 5, 2026

View reviewed changes

dsoto18 approved these changes Jan 5, 2026

View reviewed changes

	import { Observable, of, Subject } from 'rxjs';
	import { Subject } from 'rxjs';

chore/sc-38231/refactor-clark-builder-to-enforce-stricter #2129

Are you sure you want to change the base?

chore/sc-38231/refactor-clark-builder-to-enforce-stricter #2129

Uh oh!

Conversation

joaquinVill commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsoto18 Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

joaquinVill Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsoto18 commented Dec 11, 2025

Uh oh!

joaquinVill commented Dec 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joaquinVill Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joaquinVill commented Dec 9, 2025 •

edited

Loading

joaquinVill Dec 12, 2025 •

edited

Loading