# Leave Request Module - Deep Analysis

**Module:** `projects/ess/transaction/leave/leaverequest/`  
**Files Analyzed:**
- `leaverequest.component.ts` (1222 lines)
- `leaverequestlist.component.ts` (900+ lines)
- `leaverequest.service.ts`
- `leaverequestlist.service.ts`
- `leaverequest.db.service.ts`
- `leaverequestlist.db.service.ts`

---

## 1. Performance Issues

### P0 - Critical

| Issue | Location | Description | Impact |
|-------|----------|--------------|--------|
| **Duplicate API Calls** | `leaverequest.component.ts:1195` | After save, calls `/prs/Leave.svc/LeaveStatusReport` redundantly - same data already fetched via `fetchDailyReport()` | Unnecessary network traffic on every save |
| **Excessive setTimeout Chaining** | `leaverequest.component.ts:95,165,1093` | Multiple nested `setTimeout(..., 100/200)` - causes unnecessary delays and potential race conditions | UI lag, unpredictable execution order |
| **No API Response Caching** | `leaverequestlist.component.ts:107-110` | Every time Employee picklist changes, full employee data reloads - could cache for session | Repeated API calls |

### P1 - Important

| Issue | Location | Description | Impact |
|-------|----------|--------------|--------|
| **Multiple Date Parsing** | `leaverequest.component.ts:280-310` | `getDateFromEpochString` called multiple times - could be memoized | Minor CPU overhead |
| **Heavy Grid Updates** | `leaverequest.component.ts:320-360` | `updateGridPreservingSessions` triggers multiple change detection cycles with `setTimeout(..., 0)` | UI jank on date changes |

---

## 2. Memory Leaks & Cleanup Issues

### P0 - Critical

| Issue | Location | Description |
|-------|----------|--------------|
| **No takeUntil on some subscriptions** | `leaverequestlist.component.ts:107, 180` | `service.Reportdetailservice()` and some `GetBizTransaction` calls missing `takeUntil(this.destroy$)` |
| **Form not properly destroyed** | `leaverequest.component.ts:1222` | `this.form = null!` assignment but form has subscriptions that may not be cleaned up |
| **D3 Chart Memory** | `leaverequestlist.component.ts:450` | D3 chart uses `d3.select(element).selectAll('*').remove()` - but component could leave orphaned SVG elements if destroyed mid-render |

### P1 - Important

| Issue | Location | Description |
|-------|----------|--------------|
| **sessionStateByDate Map grows** | `leaverequest.component.ts:258` | `sessionStateByDate` Map is cleared only in specific scenarios - could grow indefinitely in long sessions |
| **toggleStateData Object grows** | Multiple locations | `toggleStateData` object accumulates entries - cleared only on AddNew/reset |

---

## 3. Security Issues (P0)

| Issue | Location | Description |
|-------|----------|--------------|
| **Hardcoded ModuleId values** | `leaverequest.component.ts:89,91,130,142,164,195,226-231,254,280` | Magic numbers like `-1499999788`, `-1399999915`, `-1399986890` - should be constants with meaningful names |
| **sessionStorage direct access** | `leaverequest.component.ts:82,87,93,116,118,120` | Multiple `sessionStorage.getItem('LoginDTO')`, `sessionStorage.getItem('selectedCalendarDate')`, etc. - should use GbAppStateService |

---

## 4. Best Practices Violations

### P0 - Must Fix

| Issue | Location | Description | Fix |
|-------|----------|--------------|-----|
| **Missing ChangeDetectionStrategy** | `leaverequest.component.ts:1` | No `ChangeDetectionStrategy.OnPush` - will cause excessive change detection | Add `changeDetection: ChangeDetectionStrategy.OnPush` |
| **Missing ChangeDetectionStrategy** | `leaverequestlist.component.ts:1` | No `ChangeDetectionStrategy.OnPush` | Add `changeDetection: ChangeDetectionStrategy.OnPush` |
| **console.log statements** | `leaverequest.component.ts:83,84,90`, `leaverequestlist.component.ts:46,80,81` | Multiple `console.log` calls - violates security standards | Use `GbConsoleService` |
| **Old URL Pattern** | Multiple locations | Still uses `.svc/` URL pattern instead of dot-separated code | Convert to new pattern (see below) |

### P1 - Important

| Issue | Location | Description |
|-------|----------|--------------|
| **No error handling in subscriptions** | `leaverequest.component.ts:119,165,280,400,480,530,650` | No `.subscribe((res) => {...}, (error) => {...})` - silent failures |
| **Any type overuse** | Multiple locations | `any` used extensively - should define proper interfaces |
| **Magic string/number comparisons** | `leaverequest.component.ts:142` | `if (this.MenuRights.MenuId == -1399986890)` - should be constants |

---

## 5. API Call Issues

### Old URL Pattern - Needs Conversion

| File | Old URL | Suggested New Code | API File |
|------|--------|-------------------|----------|
| `leaverequest.component.ts:119` | `/prs/Leave.svc/LeaveStatusReport/` | `Ess.Leave.LeaveStatusReport` | ess.ts |
| `leaverequest.component.ts:400` | `/prs/Leave.svc/?LeaveId=` | `Ess.Leave.GetLeaveType` | ess.ts |
| `leaverequest.component.ts:480` | `/prs/DailyAttendance.svc/Get/DayType/Before/Applying/Levae/TimeSlip/` | `Ess.DailyAttendance.GetDayTypeBeforeApplyingLeave` | ess.ts |
| `leaverequest.component.ts:530` | `/prs/TLeave.svc/?TLeaveId=` | `Ess.TLeave.GetTLeave` | ess.ts |
| `leaverequestlist.component.ts:107` | `/prs/TLeave.svc/TLeave/` | `Ess.TLeave.GetTLeaveList` | ess.ts |
| `leaverequestlist.component.ts:210` | `/prs/Leave.svc/LeaveStatusReport/` | `Ess.Leave.LeaveStatusReport` | ess.ts |
| `leaverequestlist.component.ts:250` | `/prs/TLeave.svc/TotalLeaveTaken/` | `Ess.TLeave.GetTotalLeaveTaken` | ess.ts |
| `leaverequestlist.component.ts:100` | `/cs/Criteria.svc/List/?ObjectCode=EMPLOYEE` | `Framework.Criteria.ListEmployee` | framework.ts |
| `leaverequestlist.component.ts:180` | `/fws/File.svc/Get/Attachment/Meta/Data/` | `Framework.File.GetAttachmentMetaData` | framework.ts |
| `leaverequest.db.service.ts:17` | `/ads/BizTransactionType.svc/Rights/SelectList` | `Framework.BizTransactionType.GetRightsSelectList` | framework.ts |
| `leaverequest.db.service.ts:27` | `/ads/BizTransactionType.svc/?BizTransactionTypeId=` | `Framework.BizTransactionType.GetBizTransactionType` | framework.ts |

### API Call Redundancy

| Scenario | Redundant Call | Existing Data Source |
|----------|---------------|---------------------|
| After save in `handleFormResult()` | `/prs/Leave.svc/LeaveStatusReport` (line 1195) | `fetchDailyReport()` already called in line 650 - same data |
| On employee change | Multiple `/cs/Criteria.svc/List` calls | Could consolidate into single call with proper caching |

---

## 6. Functional Issues

### P0 - Critical Bugs

| Issue | Location | Description |
|-------|----------|--------------|
| **Date Validation Logic Bug** | `leaverequest.component.ts:1050-1075` | Validation uses `lastModifiedDateField` but it's only set on date field changes - may give wrong error message |
| **Race Condition in FormOutput** | `leaverequest.component.ts:1076-1090` | setTimeout 100ms before API call - user could click multiple times |
| **Grid data inconsistency** | `leaverequest.component.ts:350-380` | `arrayvalue` and `form.get('TLeaveDetailArray')` updated in setTimeout - could be out of sync |

### P1 - Logic Issues

| Issue | Location | Description |
|-------|----------|--------------|
| **Duplicate loginDTO assignment** | `leaverequest.component.ts:82,1093` | `this.LoginDTO` assigned multiple times from sessionStorage - should be single source |
| **Inconsistent date handling** | `leaverequest.component.ts:280-310` | Uses both `Math.floor(timestamp / 1000)` and epoch string `/Date(...)/` - confusing |
| **Hardcoded financial year** | `leaverequestlist.component.ts:84` | `new Date(Date.UTC(2025, 3, 1))` - should be dynamic |

---

## 7. Technical Debt

| Issue | Location | Recommendation |
|-------|----------|-----------------|
| **No interfaces for API responses** | Throughout | Create `ILeaveRequest`, `ITLeaveDetail`, `ILeaveBalance` interfaces |
| **Duplicate validation logic** | `leaverequest.component.ts:1040-1100` | Validation in FormOutput duplicates checks in individual field functions |
| **Unused variables** | `leaverequest.component.ts:68-78` | Many class properties declared but may not all be used |
| **Commented code** | `leaverequest.component.ts:350-450, 700-800` | Large blocks of commented code - should be removed |

---

## 8. Suggested Improvements

### Immediate Actions (P0)

1. **Add ChangeDetectionStrategy.OnPush** to both components
2. **Replace console.log with GbConsoleService**
3. **Convert all .svc/ URLs to dot-separated codes** (see section 5)
4. **Add takeUntil(this.destroy$)** to missing subscriptions
5. **Remove hardcoded ModuleId values** - create constants

### Short-term (P1)

1. **Add error handling** to all API subscriptions
2. **Implement response caching** for employee data
3. **Remove excessive setTimeout** - use proper async/await or RxJS operators
4. **Define TypeScript interfaces** for all API response types
5. **Clean up commented code** blocks

### Long-term (P2)

1. **Refactor to use signals** instead of BehaviorSubjects/subscriptions
2. **Implement proper state management** using DataPassingService
3. **Add unit tests** for service layer
4. **Create reusable date utility** functions

---

## 9. Additional Issues Found (Post-Review)

| Issue | Location | Description | Priority |
|-------|----------|--------------|----------|
| **Excessive detectChanges() calls** | Throughout both components | Both components call `this.cdr.detectChanges()` dozens of times - defeats OnPush benefits | P0 |
| **No HTML template analysis** | `.html` files not analyzed | Templates may have method calls causing re-renders | P1 |
| **Heavy D3 import** | `leaverequestlist.component.ts:17` | Imports full D3 library for simple bar chart - could use lighter alternative | P2 |
| **Picklist handler issues** | Not covered in analysis | MEMORY.md notes duplicate PicklistValue handlers break employee selection | P0 |

### Note on Line Numbers
Line numbers are approximate as the code may have been modified. Use grep to find exact locations:
```bash
grep -n "console.log" projects/ess/transaction/leave/leaverequest/leaverequest.component.ts
grep -n "/prs/Leave.svc/" projects/ess/transaction/leave/leaverequest/*.ts
```

### Note on ModuleId Values
The magic numbers (e.g., `-1499999788`) appear to be runtime-generated IDs. Consider:
1. Checking if constants exist in the codebase
2. Creating a constants file for these module IDs if they are stable

## 10. Code Quality Metrics

| Metric | leaverequest.component.ts | leaverequestlist.component.ts |
|--------|---------------------------|------------------------------|
| Lines of Code | ~1222 | ~900 |
| console.log calls | 5+ | 4+ |
| setTimeout calls | 8+ | 6+ |
| Old URL patterns | 4 | 6 |
| Missing takeUntil | 3+ | 5+ |
| ChangeDetectionStrategy | ❌ Missing | ❌ Missing |

---

## Summary

The leaverequest module has significant issues requiring attention:

- **P0 Issues:** 13 critical issues (security, memory, performance)
- **P1 Issues:** 18 important issues (best practices, technical debt)
- **API URLs to convert:** 11 endpoints
- **Estimated fix effort:** 2-3 days for P0 issues

---

*Generated: 2025 | Review cycle: Quarterly*