# GbNewPicklist Deep Analysis **Date:** 2026-02-24 **Scope:** `libs/gbdirectives/src/lib/gbpicklist/newpicklist/` — component, template, styles, service, dbservice **Role:** Application-wide picklist (select/lookup) control with incremental search, modal grid, multi-select, form integration --- ## 1. File Inventory | File | Lines | Purpose | |---|---|---| | `newpicklist.component.ts` | 947 | Component — ControlValueAccessor, all business logic | | `newpicklist.component.html` | 316 | Template — 6 rendering paths for different view modes | | `newpicklist.component.scss` | 493 | Styles — 30+ `::ng-deep` rules, ng-select overrides | | `service/gbpicklist.service.ts` | 17 | Thin service pass-through | | `dbservice/gbpicklist.db.service.ts` | 614 | Criteria builder + HTTP caller | **Total: 2,387 lines** --- ## 2. Critical Issues (P0 — Fix Before Any Release) ### SEC-01 — sessionStorage Usage for LoginDTO **File:** [dbservice/gbpicklist.db.service.ts:17-18](libs/gbdirectives/src/lib/gbpicklist/dbservice/gbpicklist.db.service.ts#L17) ```typescript let LoginDTODetail: any = sessionStorage.getItem('LoginDTO') this.LoginDTO = JSON.parse(LoginDTODetail) ``` **Impact:** Called on every single picklist data fetch, across all 800+ components that use this control. LoginDTO (auth data) in sessionStorage is accessible to any XSS payload. This is the P0 violation listed in CLAUDE.md. **Fix:** Read `LoginDTO` from `DataPassingService` or `GbAppStateService` signals. Zero sessionStorage. --- ### PERF-01 — Missing `ChangeDetectionStrategy.OnPush` **File:** [newpicklist.component.ts:23](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L23) ```typescript @Component({ selector: 'gb-newpicklist', // ❌ No changeDetection: ChangeDetectionStrategy.OnPush }) ``` **Impact:** This is the most-used component in the application (830 components, many with multiple picklists). Without OnPush, Angular's default change detection runs on every browser event — click, keypress, mousemove — and re-evaluates ALL template bindings across ALL rendered picklist instances simultaneously. With 6 template rendering paths each containing 10+ complex bindings, this is the highest-leverage performance fix in the entire application. **Fix:** ```typescript @Component({ changeDetection: ChangeDetectionStrategy.OnPush, }) ``` --- ### FUNC-01 — Dead Code: Impossible Template Condition **File:** [newpicklist.component.html:181](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html#L181) ```html
``` `Field?.IsEmployeeImageRequired && !Field.IsEmployeeImageRequired` is **always `false`**. This entire input block (lines 180–192) is dead code — it can never render. The containing input was intended as a fallback but the condition was written backwards. **Fix:** Delete lines 179–192 entirely. --- ### FUNC-02 — `GetLabelValue()` Called Without Parentheses — Silent No-Op **File:** [newpicklist.component.ts:246](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L246) ```typescript this.GetLabelValue // ← missing () — this is a property reference, not a method call ``` `LangChange` (the displayed label / Transloco key) is never updated after initial setup when `FormData` changes. The picklist label stays stale after data reload. **Fix:** `this.GetLabelValue()` — add parentheses. --- ### FUNC-03 — `writeValue()` Does Not Update `SelectedValue` — Form Patching Breaks **File:** [newpicklist.component.ts:736-738](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L736) ```typescript writeValue(value: string): void { this.value = value; // ← only this is updated // SelectedValue (the visible text) is NOT updated } ``` When the parent reactive form calls `patchValue()` or `setValue()`, the Angular form infrastructure calls `writeValue()`. The internal `value` updates but `SelectedValue` — which drives the displayed text in the input — stays stale. The field appears empty to the user even though the form model is correct. **Fix:** ```typescript writeValue(value: string): void { this.value = value; this.SelectedValue = value ?? ''; } ``` --- ## 3. Memory Leaks ### LEAK-01 — Multiple `setTimeout` Calls With No Cleanup (6 Instances) **File:** [newpicklist.component.ts:112,174,186,189,193,207,569,904](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L112) ```typescript // Constructor — line 112 (1ms) setTimeout(() => { /* Field initialization */ }, 1) // ngOnInit — line 174 (100ms) setTimeout(() => { /* Focus first field */ }, 100) // ngOnChanges — lines 186, 189, 193, 207 setTimeout(() => this.handleFormChanges(), 300); setTimeout(() => this.handleFormChanges(), 100); setTimeout(() => { this.handleFilterPicklist() }, 500) setTimeout(() => this.handleNonFormPicklist(), 100); // OnPicklistClick debounce — lines 569, 904 setTimeout(() => { this.RemovePicklist = true }, 1000); ``` **Impact:** `ngOnDestroy` only completes `destroy$` — none of these setTimeout IDs are tracked or cleared. If the component is destroyed while any of these are pending (route navigation, dynamic component removal), they fire callbacks on dead component references. For grid picklists that mount/unmount frequently (grid rows added/removed), this creates persistent timers. **Fix:** Track timeout IDs and clear in `ngOnDestroy`: ```typescript private timeoutIds: ReturnType[] = []; // Usage: this.timeoutIds.push(setTimeout(() => ..., 100)); ngOnDestroy() { this.timeoutIds.forEach(clearTimeout); this.destroy$.next(); this.destroy$.complete(); } ``` Or use `timer()` from RxJS which automatically cleans up with `takeUntil(this.destroy$)`. --- ### LEAK-02 — Dialog `afterClosed()` Subscriptions Without `takeUntil` **File:** [newpicklist.component.ts:591,629,685](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L591) ```typescript this.dialog.open(PicklistGridComponent, dialogdata).afterClosed().subscribe((SeletedData: any) => { // ← No takeUntil(this.destroy$) if (this.myInputField != undefined) { this.myInputField.nativeElement.focus(); // ← Accesses DOM on potentially destroyed component } }); ``` **Impact:** Three separate dialog `.afterClosed().subscribe()` calls with no cleanup. If the parent component is destroyed while the dialog is open (programmatic navigation, auth expiry redirect), the subscription remains active. When the dialog eventually closes, it attempts to access `this.myInputField.nativeElement` on a destroyed component — runtime error. **Fix:** ```typescript this.dialog.open(PicklistGridComponent, dialogdata) .afterClosed() .pipe(takeUntil(this.destroy$)) .subscribe((selected) => { ... }); ``` --- ### LEAK-03 — `Translate()` Raw Subscribe Without Cleanup **File:** [newpicklist.component.ts:923](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L923) ```typescript this.formactionservice.googlapitranslateservice(data, lang).subscribe(res => { this.value = res this.SelectedValue = this.value this.cdr.detectChanges() }) ``` No `takeUntil(this.destroy$)`. If the component is destroyed mid-HTTP-request, the callback fires on a dead instance. **Fix:** `.pipe(takeUntil(this.destroy$)).subscribe(...)` --- ### LEAK-04 — `RemovePicklist` Debounce Uses Raw `setTimeout` (Uncleaned) **File:** [newpicklist.component.ts:569-571,904-907](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L569) ```typescript // Lines 569-571 (inside OnPicklistClick) this.RemovePicklist = false setTimeout(() => { this.RemovePicklist = true }, 1000); // Lines 904-907 (PicklistonRemove) this.RemovePicklist = false setTimeout(() => { this.RemovePicklist = true }, 1000); ``` Identical debounce logic duplicated in two places, neither cleaned up. Also note: `PicklistonRemove` receives `RemovedItem` as a parameter and **completely ignores it** — the parent form is never notified of which item was removed. --- ## 4. Performance Issues ### PERF-02 — `ngOnChanges` Fires `handleFormChanges()` Up to 3 Times Per Change Cycle **File:** [newpicklist.component.ts:181-214](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L181) ```typescript ngOnChanges(changes: SimpleChanges): void { if (changes['GridIndex']) { this.handleFormChanges() // ① immediate } if (changes['FormData'] && changes['GridIndex']) { setTimeout(() => this.handleFormChanges(), 300); // ② +300ms } if (changes['FormData']) { setTimeout(() => this.handleFormChanges(), 100); // ③ +100ms } // ... } ``` When both `FormData` and `GridIndex` change (standard grid scenario), all three branches activate: immediate + 100ms + 300ms. Each call runs full picklist configuration and 1–3 `cdr.detectChanges()` calls. This fires at 0ms, 100ms, and 300ms on every grid row change. **Fix:** Deduplicate with a single `debounceTime` approach: ```typescript private formChanges$ = new Subject(); // In constructor: this.formChanges$.pipe(debounceTime(50), takeUntil(this.destroy$)) .subscribe(() => this.handleFormChanges()); // In ngOnChanges: if (changes['GridIndex'] || changes['FormData']) { this.formChanges$.next(); } ``` --- ### PERF-03 — `cdr.detectChanges()` Called 6+ Times Per Interaction **Locations:** lines 105, 157, 247, 293, 314, 362, 457, 628, 926 Multiple methods each call `cdr.detectChanges()`, and they chain: `ngOnChanges` → `handleFormChanges()` → `handleNonFilterFormPicklist()` → each calls `cdr.detectChanges()`. A single `FormData` change triggers: `handleFormChanges` (line 247) + `handleNonFilterFormPicklist` (line 293) = 2 synchronous `detectChanges()` calls, then again 2 more at +100ms. **Fix:** Adopt signals (`rxResource`, `computed`) — they eliminate all manual `detectChanges()` calls. --- ### PERF-04 — Complex Inline Boolean Expressions Duplicated 5+ Times in Template **File:** [newpicklist.component.html:24,102,183,198,213,283](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html#L24) ```html [readonly]="SelectedId != undefined && (Field.PicklistSubTitle || !(SelectedId.toString() == '0' || SelectedId.toString() == '-1' || SelectedId.toString() == '')) || !FormEdit() || this.gbReadOnly || value == 'NONE'" ``` This exact expression appears 5 times in the template. Each occurrence calls `SelectedId.toString()` 3 times per change detection cycle. With OnPush missing, this runs on every browser event across all rendered picklist instances. **Fix:** Single `computed()` signal: ```typescript isReadOnly = computed(() => this.isEffectivelySelected() || !this.FormEdit() || this.gbReadOnly || this.value === 'NONE' ); ``` --- ### PERF-05 — `RemoveSpace()` Method Called in `[attr.data-cy]` Bindings — Runs Every CD Cycle **File:** [newpicklist.component.html:7,19,35,89,113,etc.](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html#L7) ```html [attr.data-cy]="RemoveSpace(PicklistName)+'-PickList'" ``` `RemoveSpace()` is a string method called as a template expression — re-executed on every change detection pass. `PicklistName` is static after init. **Fix:** Pre-compute once as a property: `picklistCyId = this.RemoveSpace(this.PicklistName) + '-PickList';` --- ### PERF-06 — Template Has 6 Near-Identical Rendering Paths — Massive DOM Overhead **File:** [newpicklist.component.html](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html) Six distinct structural blocks for: Filter/Grid, MinimizedEditView, MinimizedView, ExpandedView, Normal (non-key), Normal (key field). Each block duplicates the full picklist input markup. Angular keeps all six in the component tree but shows only one via `*ngIf`. The hidden blocks still participate in change detection (structural directives create/destroy, but with `*ngIf` on the outer `div`, inner `*ngIf` children still add to the template AST evaluation cost). One of the six input blocks is dead code (see FUNC-01). Two more (MinimizedView/MinimizedEditView) could be separated into `@if` at compile time. **Fix:** Refactor into `@if`/`@else` blocks (Angular 17+ control flow) and extract the common input into a shared `ng-template`. --- ### PERF-07 — Large JSON Files Statically Imported at Module Level **File:** [newpicklist.component.ts:6-8](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L6) ```typescript import * as AllPicklistJSON from 'projects/gbhost/public/picklist/allpicklist.json'; import * as PicklistCriteria from 'projects/gbhost/public/picklist/picklistcriteria.json'; import { PicklistURL } from 'projects/gbhost/public/picklist/picklisturl'; ``` These are imported by every component that uses `gb-newpicklist`. Since this is a shared library component used 800+ times, these JSON payloads are bundled into the shared chunk and loaded eagerly even when the current page has no picklists. `AllPicklistJSON` likely contains hundreds of picklist definitions. **Fix:** Use Angular's `inject(PICKLIST_CONFIG)` with an injection token that provides the JSON lazily, or load via HTTP at first use. --- ## 5. Functional Issues ### FUNC-04 — Incremental Search Has No Debounce — Every `focusout` Fires an API Call **File:** [newpicklist.component.ts:749-770](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L749) ```typescript onFocusChange(event: Event): void { // ... if (selectedIdValue == '0' || ... && this.value.length > 0 && ...) { this.OldFocusData = this.value; this.OnPicklistClick(false); // ← API call, no debounce } } ``` `onFocusChange` fires on every `focusout` event. If a user tabs through a form with 10 picklist fields, 10 API calls fire simultaneously. The component is documented as "incremental search optimized" but there is no debounce, throttle, or deduplication. **Fix:** Debounce the search trigger: ```typescript private searchTrigger$ = new Subject(); // In constructor: this.searchTrigger$.pipe( debounceTime(300), distinctUntilChanged(), takeUntil(this.destroy$) ).subscribe(value => this.OnPicklistClick(false)); // In onFocusChange: this.searchTrigger$.next(this.value); ``` --- ### FUNC-05 — No Memoization / Cache — Same Query Fires API on Every Open **File:** [newpicklist.component.ts:655](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L655) ```typescript this.picklistservice.getPicklistData(picklisturl, ...) .pipe(takeUntil(this.destroy$)) .subscribe((PickListData) => { ... }) ``` `OnPicklistClick()` always fetches fresh data. Opening the same picklist twice fires two identical API calls. The `DexieService` path (line 604) uses IndexedDB as cache, but the normal HTTP path has no client-side cache whatsoever. **Fix:** Add a short-lived cache keyed on `(url + JSON.stringify(criteria))` — even a 30-second cache eliminates redundant API calls for repeated picklist opens. --- ### FUNC-06 — `PicklistDefauldValue` Sets `SelectedValue` but Not `value` or `SelectedId` **File:** [newpicklist.component.ts:212-214](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L212) ```typescript if (changes['PicklistDefauldValue']) { this.SelectedValue = this.PicklistDefauldValue // ← display only // SelectedId and value not updated } ``` Setting `PicklistDefauldValue` from the parent updates the displayed text but leaves the form control value and selection ID stale. If the parent reads the form value after this change, it gets the old value. --- ### FUNC-07 — `Translate()` Has Hardcoded `'ta'` (Tamil) Language **File:** [newpicklist.component.ts:920](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L920) ```typescript Translate() { let lang = 'ta' // ← hardcoded Tamil ``` Translation target language is hardcoded regardless of the user's configured language. Any user in a non-Tamil locale who activates the translate button gets Tamil output. **Fix:** Read target language from `formactionservice.isNLanguage().Nlanguage` or an `@Input()`. --- ### FUNC-08 — `SelectedId` Type `string | string[]` Used Inconsistently **File:** [newpicklist.component.ts:58](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L58) ```typescript SelectedId: string | string[] = []; // initialized as array // But then: this.SelectedId.length === 0 // OK for array this.SelectedId.toString() == '0' // works for strings but not arrays this.SelectedId[0] == '' // only for arrays ``` In templates: `SelectedId.toString() == '-1'` — when SelectedId is `['-1','-2']` (multi-select), `.toString()` produces `"-1,-2"`, breaking the empty/unselected check. **Fix:** Use separate typed fields — `selectedSingleId: string = ''` and `selectedMultiIds: string[] = []` — and branch per `Field.Multiple`. --- ### FUNC-09 — `PicklistonRemove` Ignores Removed Item — Parent Not Notified **File:** [newpicklist.component.ts:903-908](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L903) ```typescript public PicklistonRemove(RemovedItem: any) { this.RemovePicklist = false // throttle clicks setTimeout(() => { this.RemovePicklist = true }, 1000); // ← RemovedItem completely ignored // ← OnPicklistChange never called // ← PicklistValue never emitted } ``` When a user removes an item from a multi-select picklist, the parent component is never notified. `this.value`, `this.SelectedId`, and the `PicklistValue` output are unchanged. The form data goes stale. **Fix:** Call `OnPicklistChange(this.SelectedId)` after updating `SelectedId` to remove the item. --- ### FUNC-10 — Race Condition: Constructor `setTimeout(1ms)` vs. `ngOnChanges` `setTimeout(100ms)` **File:** [newpicklist.component.ts:112,189](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L112) The constructor's 1ms `setTimeout` initializes `this.Field` and `this.PicklistObject`. The `ngOnChanges` 100ms `setTimeout` then calls `handleFormChanges()` which requires `this.Field` to be defined. The 1ms guarantee holds only in single-threaded JS microtasks, but under heavy rendering (grid initialization with many rows), macrotask scheduling can reorder these, causing `handleFormChanges()` to run before `Field` is initialized. **Fix:** Eliminate the constructor `setTimeout` entirely. Move all initialization to `ngOnInit()` and handle `ngOnChanges` in a lifecycle-safe way using a boolean `initialized` flag. --- ### FUNC-11 — `ShowInactive` Overrides Status as String `"1,2,3,4,5"` Instead of Using `InArray` **File:** [dbservice/gbpicklist.db.service.ts:434](libs/gbdirectives/src/lib/gbpicklist/dbservice/gbpicklist.db.service.ts#L434) ```typescript attribute.FieldValue = "1,2,3,4,5"; // update only matched field ``` This sets `FieldValue` to a comma-separated string but the field's `OperationType` is numeric (e.g., `7` for IN). The API likely expects `InArray: [1,2,3,4,5]` not `FieldValue: "1,2,3,4,5"`. This may silently fail or return wrong results. **Fix:** Use `attribute.InArray = [1,2,3,4,5]; attribute.FieldValue = null;` with the appropriate OperationType for array membership. --- ### FUNC-12 — `onInputChange` Dispatches `onChange` Even When Field Is ReadOnly **File:** [newpicklist.component.ts:773-801](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L773) The `onInputChange` handler updates `this.value` and calls `this.onChange(newValue)` regardless of `this.gbReadOnly` or `Field.ReadOnly`. The template sets `[readonly]` on the input, but a programmatic `inputElement.value = ...` assignment before calling `onChange` can bypass the readonly guard if the input's readonly attribute isn't enforced at the event level. --- ### FUNC-13 — `FormEdit` Computed Signal Created Inside `setTimeout` — 1ms Gap **File:** [newpicklist.component.ts:117-123](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L117) ```typescript // line 54 — initial value FormEdit: Signal = signal(true); // line 117 — inside setTimeout(1ms) in constructor this.FormEdit = computed(() => { const menuData = this.formactionservice.isMenuEditable(); const item = menuData.find(...); return item?.editable ?? true; }); ``` During the 1ms window, `FormEdit()` returns `true` unconditionally. Any template rendering that occurs before the timeout fires (e.g., initial render) will show the field as editable even if it should be read-only. This can flash editable state before locking. --- ## 6. Code Quality & Maintainability ### QUAL-01 — Constructor is 95 Lines — Mixes 4 Effects, setTimeout, computed(), Initialization **File:** [newpicklist.component.ts:76-171](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L76) The constructor contains: - 4 `effect()` calls - 1 `setTimeout(1ms)` with full initialization logic - 1 `computed()` creation - Conditional filter field construction - `cdr.detectChanges()` All of this should be in `ngOnInit()` or using signal-based lifecycle patterns. Constructor should only contain DI setup. --- ### QUAL-02 — Constructor Injection Mixed with `inject()` — Violates CLAUDE.md **File:** [newpicklist.component.ts:76](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L76) ```typescript // Constructor injection (old style) constructor( @Optional() @Self() public ngControl: NgControl, private picklistservice: PicklistService, public dialog: MatDialog, public formactionservice: FormActionservice, public dexieService: DexieService ) // inject() style (new — line 68) private cdr = inject(ChangeDetectorRef); ``` CLAUDE.md mandates `inject()` exclusively. `@Optional() @Self()` can be expressed as: ```typescript private ngControl = inject(NgControl, { optional: true, self: true }); ``` --- ### QUAL-03 — 20+ `any` Types — No Typed Interfaces for Core Data **File:** [newpicklist.component.ts:32-75](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L32) ```typescript @Input() FormData!: any; @Input() FullFormData!: any; @Input() FilterData!: any; @Input() DefaultListofData!: any; @Input() PicklistConfigData!: any; @Input() UserDetail: any = ""; PickListData: any = []; AllPicklistJSON: any = AllPicklistJSON; // ... ``` The core data inputs — `FormData`, `PicklistConfigData`, `FilterData` — are all `any`. These flow into all the complex initialization logic. Type errors in these objects are invisible at compile time and surface only as runtime crashes. --- ### QUAL-04 — Hardcoded Field Names in Generic Component (`onFocusChange`) **File:** [newpicklist.component.ts:759-760](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L759) ```typescript this.Field.Name != 'CostingNumberEnquiry' && this.Field.Name != 'EmployeeBloodGroup' && this.Field.Name != 'CallStatusName' ``` Application-specific field name exceptions embedded in a shared library component. This tightly couples the generic control to specific business forms and makes it impossible to maintain cleanly. **Fix:** Add an `@Input() SkipIncrementalSearch: boolean = false` and let the parent opt out. --- ### QUAL-05 — `Madatory` Typo (Should Be `Mandatory`) **File:** [newpicklist.component.ts:72,99,765,766,881,883](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L72) ```typescript Madatory: boolean = true // ← typo throughout ``` Minor but appears throughout the component and template. Rename to `isMandatoryValid`. --- ### QUAL-06 — `Gbvisible` Input is a Signal Passed as `@Input()` — Non-Standard Angular Pattern **File:** [newpicklist.component.ts:41](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L41) ```typescript @Input() Gbvisible = signal(true); ``` Passing a writable signal as an `@Input()` is unusual. The parent can't update this with Angular's normal binding syntax — it would have to call `.set()` on the signal directly. In Angular 20, use `input()` signal-based inputs instead: ```typescript gbVisible = input(true); ``` This makes the binding work normally: `[gbVisible]="true"` from parent. --- ### QUAL-07 — `PicklistService` is a Pointless Pass-Through Layer **File:** [service/gbpicklist.service.ts](libs/gbdirectives/src/lib/gbpicklist/service/gbpicklist.service.ts) ```typescript export class PicklistService { constructor(public picklistdbservice: PicklistDbService) { } public getPicklistData(...args): Observable { return this.picklistdbservice.getPicklistData(...args); // exact pass-through } public removepicklistdexieservice(...) { this.picklistdbservice.removepicklistdexiedb(...); } } ``` Zero business logic. The service adds nothing — it's a pure delegation wrapper with a `public` constructor injection exposed unnecessarily. Inject `PicklistDbService` directly in the component or put real logic in the service layer. --- ### QUAL-08 — `DBService` Contains 400+ Lines of Business Logic — Violates Layer Separation **File:** [dbservice/gbpicklist.db.service.ts](libs/gbdirectives/src/lib/gbpicklist/dbservice/gbpicklist.db.service.ts) The DBService (which should only make HTTP calls) contains: - Full criteria object construction (400+ lines) - `ShowInactive` status override logic - `ShowArchived` conditional filtering - Module-based criteria resolution - `AllowedIds` JoinType override This is all business/application logic. The DBService layer should only call `this.http.gbhttppost(url, criteria)`. The criteria-building belongs in `PicklistService` or a dedicated `PicklistCriteriaBuilderService`. --- ### QUAL-09 — `effects` in Constructor Reference `this` Before Full Init **File:** [newpicklist.component.ts:77-170](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L77) Four `effect()` calls in the constructor reference `this.Field`, `this.myInputField`, `this.ngControl.control`, etc. At the time the effects are first evaluated, `ngAfterViewInit` has not run, so `this.myInputField` (a `@ViewChild`) is `undefined`. The effect at line 77 guards `this.myInputField != undefined` correctly, but others may not. Effects should be registered in the class body for Angular 20 compliance. --- ### QUAL-10 — 30+ `::ng-deep` Rules in Component SCSS **File:** [newpicklist.component.scss:98-471](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.scss#L98) 30+ `::ng-deep` rules targeting `#newpicklist` and `#entitylookupKeyField`. `::ng-deep` is deprecated and breaks ViewEncapsulation. These rules leak into child components globally. Two `ng-select` elements both use `id="newpicklist"` — IDs must be unique per DOM, but multiple picklists rendered simultaneously produce duplicate IDs, causing CSS selectors to behave unpredictably. **Fix:** Use ViewEncapsulation.None on a wrapper or move to the global `styles.scss` with proper scoping. Replace duplicate IDs with classes. --- ### QUAL-11 — Duplicate ID `id="newpicklist"` on Multiple Rendered Elements **File:** [newpicklist.component.html:10,93,158,260](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html#L10) `id="newpicklist"` is used on the `` element in all 4 ng-select rendering paths. Since `gb-newpicklist` is rendered many times per page, this produces dozens of duplicate IDs in the DOM — invalid HTML and breaks the `::ng-deep #newpicklist` CSS scoping entirely. **Fix:** Use a unique computed ID: `[id]="'picklist-' + PicklistName"`. --- ### QUAL-12 — Commented-Out Code Throughout (Both Files) Significant blocks of commented-out code in `newpicklist.component.ts` (line 654) and `gbpicklist.db.service.ts` (lines 48-55, 476-497, and others). Dead commented code should be removed — it's tracked in git history. --- ## 7. i18n Issues ### I18N-01 — Hardcoded English Strings in Component Logic **File:** [newpicklist.component.ts:789,930](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L789) ```typescript // Line 789 data: { message: "First Character Should be a Alphabet", // ← not transloco key heading: 'Warning', } // Line 930 data: { message: "Please Enter Input for Translation", // ← not transloco key heading: 'Info' } ``` Both dialog messages are hardcoded English strings. CLAUDE.md requires all display strings to use Transloco keys. --- ### I18N-02 — `LangChange` Used as Transloco Key Directly **File:** [newpicklist.component.html:69,77,142,246](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html#L69) ```html {{ LangChange | transloco }} ``` `LangChange` stores the raw `Field.Label` value (e.g., `"Employee Name"`) and pipes it directly as a Transloco key. For this to work, every field label in every form JSON must exactly match a Transloco key in the i18n files (e.g., `"employee.name"`). This is a fragile convention — any label with spaces, special characters, or case mismatch silently falls back or shows the key verbatim. **Fix:** Separate label from i18n key — add a dedicated `LabelKey` field in `IField` for the Transloco key, keeping `Label` as a fallback. --- ### I18N-03 — Hardcoded `'ta'` Target Language for Translation **File:** [newpicklist.component.ts:920](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L920) ```typescript let lang = 'ta' // ← hardcoded Tamil — functional bug for all other languages ``` --- ## 8. Security Issues ### SEC-01 — sessionStorage LoginDTO (Repeated in DBService) See P0 above. ### SEC-02 — `img src` Base64 Data URI Without MIME Validation **File:** [newpicklist.component.html:170-171,271-272](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.html#L170) ```html keyboard_arrow_down ``` A `` with a click handler is not keyboard-accessible by default — it has no `tabindex`, no `role="button"`, and no keyboard event handler. Keyboard users cannot activate the dropdown without using Enter on the input. --- ## 10. Responsive / RTL Issues ### RESP-01 — Fixed Pixel Widths in SCSS **File:** [newpicklist.component.scss:9,212,398](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.scss#L9) ```scss .picklist { width: 400px; } // ::ng-deep dropdown panels: width: 400px .arrodownicon { margin-left: 384.5px; } // magic absolute pixel ``` --- ### RESP-02 — Dialog Dimensions Fixed (Not Responsive) **File:** [newpicklist.component.ts:588-590,625-627,681-684](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.ts#L588) ```typescript width: '665px', height: '550px', // or '560px' ``` Three dialog openings use fixed pixel dimensions. Per CLAUDE.md, must use `min(665px, 95vw)` with `maxWidth: '95vw'`. On mobile or small viewports, these dialogs overflow and are inaccessible. --- ### RESP-03 — No RTL CSS **File:** [newpicklist.component.scss](libs/gbdirectives/src/lib/gbpicklist/newpicklist/newpicklist.component.scss) Multiple directional properties with no RTL counterpart: - `.arrodownicon { margin-left: 384.5px }` — should flip - `.translate-icon { right: -4px }` — should flip - `.advancebtn { margin-left: 120px }` — should flip No `[dir="rtl"]` block anywhere in the file. --- ## 11. Summary — Priority Matrix | ID | Issue | Severity | Effort | |---|---|---|---| | SEC-01 | sessionStorage LoginDTO in DBService | P0 Critical | Medium | | PERF-01 | Missing OnPush — costliest component in app | P0 Critical | Low | | FUNC-01 | Dead code block (impossible *ngIf condition) | P0 | Trivial | | FUNC-02 | GetLabelValue() never called — label stale | P0 | Trivial | | FUNC-03 | writeValue() doesn't update SelectedValue — patchValue broken | P0 | Low | | LEAK-01 | 6× setTimeout with no cleanup | P1 High | Low | | LEAK-02 | 3× dialog afterClosed() without takeUntil | P1 High | Low | | LEAK-03 | Translate() subscribe without cleanup | P1 High | Trivial | | FUNC-04 | No debounce on incremental search — API spam | P1 High | Medium | | FUNC-09 | PicklistonRemove ignores item — parent not notified | P1 High | Low | | FUNC-08 | Race condition: constructor timeout vs. ngOnChanges timeout | P1 High | Medium | | QUAL-11 | Duplicate `id="newpicklist"` on multiple DOM elements | P1 High | Low | | PERF-02 | 3× handleFormChanges per change cycle | P1 High | Medium | | PERF-03 | 6+ cdr.detectChanges() per interaction | P2 Medium | Medium | | PERF-04 | Complex readonly expression duplicated 5× in template | P2 Medium | Low | | PERF-05 | RemoveSpace() method call in template binding | P2 Medium | Low | | FUNC-05 | No cache — same query fires API on every open | P2 Medium | Medium | | FUNC-06 | PicklistDefauldValue sets display only, not form value | P2 Medium | Low | | FUNC-07 | Hardcoded Tamil ('ta') in Translate() | P2 Medium | Trivial | | FUNC-10 | Race condition: constructor timeout vs ngOnChanges | P2 Medium | Medium | | FUNC-11 | ShowInactive sets FieldValue as string not InArray | P2 Medium | Low | | FUNC-12 | onInputChange dispatches onChange even when readonly | P2 Medium | Low | | QUAL-01 | 95-line constructor — unmaintainable | P2 Medium | High | | QUAL-02 | Constructor injection mixed with inject() | P2 Medium | Low | | QUAL-03 | 20+ any types — no typed interfaces | P2 Medium | High | | QUAL-04 | Hardcoded field names in generic component | P2 Medium | Low | | QUAL-06 | Signal passed as @Input() — non-standard | P2 Medium | Low | | QUAL-07 | PicklistService is a no-op pass-through | P2 Medium | Low | | QUAL-08 | DBService contains 400 lines of business logic | P2 Medium | High | | QUAL-09 | effects in constructor reference uninitialized ViewChild | P2 Medium | Medium | | QUAL-10 | 30+ ::ng-deep rules — deprecated | P2 Medium | High | | RESP-02 | Dialog fixed width/height — breaks mobile | P2 Medium | Low | | I18N-01 | Hardcoded English strings in dialogs | P2 Medium | Low | | I18N-02 | LangChange piped directly as Transloco key — fragile | P2 Medium | High | | I18N-03 | Hardcoded 'ta' target language | P2 Medium | Trivial | | FUNC-13 | FormEdit computed created in setTimeout — 1ms flash | P3 Low | Medium | | QUAL-05 | Madatory typo | P3 Low | Trivial | | QUAL-12 | Dead commented-out code blocks | P3 Low | Trivial | | PERF-06 | Template has 6 near-identical rendering paths | P3 Low | High | | PERF-07 | Large JSON imported statically — bundle weight | P3 Low | High | | A11Y-01 | Non-standard autocomplete value | P3 Low | Trivial | | A11Y-02 | No ARIA attributes on combobox pattern | P3 Low | Medium | | A11Y-03 | Dropdown icon not keyboard-accessible | P3 Low | Low | | RESP-01 | Fixed pixel widths in SCSS | P3 Low | Medium | | RESP-03 | No RTL CSS | P3 Low | Medium | | LEAK-04 | RemovePicklist debounce duplicated, uncleaned | P3 Low | Low | | FUNC-08 | SelectedId string|string[] used inconsistently | P3 Low | Medium | | SEC-02 | Base64 img src without validation | P3 Low | Low | --- ## 12. Quick Wins (< 30 minutes each) 1. **Add `changeDetection: ChangeDetectionStrategy.OnPush`** (PERF-01) — single line, highest ROI 2. **Add `()` to `this.GetLabelValue`** on line 246 (FUNC-02) — fix label staleness 3. **Update `writeValue()` to also set `SelectedValue`** (FUNC-03) — fixes patchValue/setValue 4. **Delete lines 179–192** (impossible `*ngIf` — dead code) (FUNC-01) 5. **Add `takeUntil(this.destroy$)` to 3 dialog subscriptions** (LEAK-02) 6. **Add `takeUntil(this.destroy$)` to Translate() subscribe** (LEAK-03) 7. **Fix dialog widths** from `'665px'` to `'min(665px, 95vw)'` with `maxWidth: '95vw'` (RESP-02) 8. **Change `'ta'` to `this.formactionservice.isNLanguage().Nlanguage`** in Translate() (FUNC-07) 9. **Replace `id="newpicklist"` with `[id]="'picklist-' + PicklistName"`** (QUAL-11) 10. **Add `SkipIncrementalSearch: boolean = false` @Input** to remove hardcoded field names (QUAL-04) --- ## 13. Architecture Recommendation — Signal-Based Picklist The fundamental problem is that this component manages too much mutable state manually. The path to a maintainable, high-performance picklist: ```typescript @Component({ changeDetection: ChangeDetectionStrategy.OnPush, providers: [{ provide: NgControl, useExisting: forwardRef(() => GbNewPicklistComponent) }] }) export class GbNewPicklistComponent implements ControlValueAccessor { // Inputs (Angular 20 signal inputs) formData = input(null); picklistConfigData = input(null); gbReadOnly = input(false); isFilter = input(false); gridIndex = input(-1); // Internal reactive state — single source of truth private selectedId = signal(''); private selectedValue = signal(''); // Derived readonly state — computed from signals protected isEffectivelyReadOnly = computed(() => !this.formEdit() || this.gbReadOnly() || this.field()?.ReadOnly === true ); protected isSelected = computed(() => { const id = this.selectedId(); return Array.isArray(id) ? id.length > 0 : (id !== '' && id !== '0' && id !== '-1'); }); protected picklistCyId = computed(() => this.RemoveSpace(this.picklistName()) + '-PickList' ); // HTTP data via rxResource — loading/error/value all signals private searchTrigger = signal(''); picklistResource = rxResource({ request: () => ({ search: this.searchTrigger(), config: this.picklistConfigData() }), loader: ({ request }) => request.config ? this.picklistService.getPicklistData(request) : EMPTY }); pickListData = computed(() => this.picklistResource.value()?.responseValue ?? []); isLoading = this.picklistResource.isLoading; // ControlValueAccessor — write both value and display writeValue(value: string): void { this.selectedId.set(value ?? ''); this.selectedValue.set(value ?? ''); } } ``` This eliminates: - All `cdr.detectChanges()` calls (signals handle CD) - `destroy$` + `ngOnDestroy` for subscriptions (rxResource cleans up) - All `setTimeout` initialization hacks - The `SelectedValue` / `value` desync problem (single signal) - The `ngOnChanges` triple-fire (signals compute lazily on demand)