feat: complete tOS project with HR, LEAN, Dashboard and Integrations modules
Full enterprise web operating system including: - Next.js 14 frontend with App Router, i18n (DE/EN), shadcn/ui - NestJS 10 backend with Prisma, JWT auth, Swagger docs - Keycloak 24 SSO with role-based access control - HR module (employees, time tracking, absences, org chart) - LEAN module (3S planning, morning meeting SQCDM, skill matrix) - Integrations module (PlentyONE, Zulip, Todoist, FreeScout, Nextcloud, ecoDMS, GembaDocs) - Dashboard with customizable drag & drop widget grid - Docker Compose infrastructure (PostgreSQL 16, Redis 7, Keycloak 24) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
123
.claude/agent-memory/senior-code-reviewer/MEMORY.md
Normal file
123
.claude/agent-memory/senior-code-reviewer/MEMORY.md
Normal file
@@ -0,0 +1,123 @@
|
||||
# tOS Project Code Review Memory
|
||||
|
||||
## Project Overview
|
||||
- **Type:** Enterprise Web Operating System with HR, LEAN modules
|
||||
- **Stack:** Next.js 14 + NestJS 10 + Prisma + PostgreSQL + Keycloak
|
||||
- **Structure:** pnpm monorepo with turbo (apps/web, apps/api, packages/shared)
|
||||
|
||||
## Code Standards (from PLAN.md)
|
||||
| Area | Language | Convention |
|
||||
|------|----------|------------|
|
||||
| Code & Comments | English | Required |
|
||||
| UI Text | German (via i18n) | next-intl |
|
||||
| Variables | camelCase | `employeeId` |
|
||||
| Components | PascalCase | `DashboardWidget` |
|
||||
| Files | kebab-case | `user-service.ts` |
|
||||
| Constants | UPPER_SNAKE_CASE | `MAX_VACATION_DAYS` |
|
||||
|
||||
## Critical Patterns Found
|
||||
|
||||
### Authentication
|
||||
- Global `JwtAuthGuard` with `@Public()` opt-out decorator
|
||||
- Global `RolesGuard` with `@Roles()` decorator
|
||||
- Keycloak integration via NextAuth (frontend) and passport-jwt (backend)
|
||||
- **IMPORTANT:** Always verify JWT signatures, never just decode
|
||||
- Guards registered in correct order: JWT -> Roles -> Permissions (app.module.ts L74-88)
|
||||
- JWT strategy validates user exists and is active on every request
|
||||
|
||||
### Frontend Architecture
|
||||
- **Server/Client split:** Server page.tsx -> Client *-content.tsx
|
||||
- **State:** Zustand (sidebar, dashboard), TanStack Query (server state)
|
||||
- **i18n:** next-intl with de.json/en.json, default locale = "de"
|
||||
- **Dashboard:** dnd-kit widget grid, widget registry pattern
|
||||
- **Styling:** Tailwind + shadcn/ui, HSL CSS vars, dark mode via class
|
||||
|
||||
### Integration Architecture (Phase 3 - Disabled)
|
||||
- **Location:** `apps/api/integrations.backup/`
|
||||
- Only PlentyONE/Zulip/Todoist extend BaseConnector
|
||||
- FreeScout/Nextcloud/ecoDMS/GembaDocs have independent implementations
|
||||
|
||||
### Keycloak Client ID Mismatch (CRITICAL - RECURRING)
|
||||
- Realm: `tos-frontend` / `tos-backend`
|
||||
- apps/api/.env: `tos-api` | apps/web/.env: `tos-nextauth` (MISMATCH)
|
||||
|
||||
## Key Issues to Watch
|
||||
1. **params API:** Mixed sync/async patterns across pages
|
||||
2. **Hardcoded German:** dashboard-content.tsx, widget-grid.tsx bypass i18n
|
||||
3. **Mock Data:** HR hooks (employees, time-tracking, absences) use mock data
|
||||
4. **Type Conflicts:** types/index.ts vs types/hr.ts have conflicting exports
|
||||
5. **Auth Layout:** `(auth)/layout.tsx` is `'use client'` -- blocks SSR
|
||||
6. **Error pages:** Link to `/dashboard` without locale prefix
|
||||
7. **ENCRYPTION_KEY:** `optional()` in validation -- must be required
|
||||
|
||||
### Backend-Specific Issues (from Phase 7 Review)
|
||||
8. **Morning Meeting Permissions:** ALL endpoints use DASHBOARD_VIEW -- any user can CRUD meetings
|
||||
9. **Private method access:** time-tracking.controller.ts L237-239 uses bracket notation `this.service['privateMethod']`
|
||||
10. **Prisma cleanDatabase():** Uses Promise.all ignoring FK constraints (prisma.service.ts L48-61)
|
||||
11. **Roles guard leaks info:** Error messages reveal required role names (roles.guard.ts L31-33)
|
||||
12. **enableImplicitConversion:** In ValidationPipe (main.ts L44) -- can cause type coercion bugs
|
||||
13. **Break tracking via string parsing:** time-tracking uses note field for break detection (L216-219, L257-258)
|
||||
14. **Audit interceptor:** oldData always undefined (audit.interceptor.ts L60), salary not sanitized (L131)
|
||||
15. **Unregistered interceptors:** LoggingInterceptor and TimeoutInterceptor defined but never registered
|
||||
16. **N+1 queries:** skill-entries.service.ts analyzeGaps (L515-523), s3-planning findAll (L173-178)
|
||||
17. **bulkUpsert not transactional:** skill-entries.service.ts L429-476 -- partial failures possible
|
||||
18. **No backdating limit:** Manual time entries have no restriction on how far back entries can be created
|
||||
19. **Holiday calculation missing:** absences calculateWorkingDays does not account for public holidays
|
||||
20. **Vacation carry-over TODO:** BUrlG compliance gap (absences.service.ts L979)
|
||||
|
||||
## File Locations
|
||||
| Purpose | Path |
|
||||
|---------|------|
|
||||
| Prisma Schema | `apps/api/prisma/schema.prisma` |
|
||||
| Frontend Types | `apps/web/src/types/` |
|
||||
| HR Hooks | `apps/web/src/hooks/hr/` |
|
||||
| LEAN Hooks | `apps/web/src/hooks/lean/` |
|
||||
| Dashboard | `apps/web/src/components/dashboard/` |
|
||||
| i18n Messages | `apps/web/messages/` |
|
||||
| Integration Backup | `apps/api/integrations.backup/` |
|
||||
| Auth Guards | `apps/api/src/auth/guards/` |
|
||||
| Auth Permissions | `apps/api/src/auth/permissions/` |
|
||||
| Encryption Service | `apps/api/src/common/services/encryption.service.ts` |
|
||||
| HR Employees | `apps/api/src/modules/hr/employees/` |
|
||||
| HR Time Tracking | `apps/api/src/modules/hr/time-tracking/` |
|
||||
| HR Absences | `apps/api/src/modules/hr/absences/` |
|
||||
| LEAN Skill Matrix | `apps/api/src/modules/lean/skill-matrix/` |
|
||||
| LEAN Morning Meeting | `apps/api/src/modules/lean/morning-meeting/` |
|
||||
| LEAN S3 Planning | `apps/api/src/modules/lean/s3-planning/` |
|
||||
| Audit Module | `apps/api/src/modules/audit/` |
|
||||
| Config Validation | `apps/api/src/config/config.validation.ts` |
|
||||
|
||||
## Review History
|
||||
|
||||
### Phase 7 Full Backend Review (2026-02-06)
|
||||
- **Overall: 7.8/10** | 3 Critical, 14 Important issues
|
||||
- Scores: Auth 8 | Prisma 7 | HR/Emp 8 | HR/Time 7 | HR/Abs 8
|
||||
- LEAN/Skill 8 | LEAN/Morning 7 | LEAN/S3 8 | Dashboard 8
|
||||
- Common 8 | Users 8 | Audit 7 | app.module 9
|
||||
- See: `phase7-backend-review.md`
|
||||
|
||||
### Phase 6 Frontend Review (2026-02-06)
|
||||
- 5 Critical, 9 Important, detailed 10-area review
|
||||
- See: `phase6-frontend-review.md`
|
||||
|
||||
### Infrastructure + Integration Review (2026-02-06)
|
||||
- Docker 7/10 | Keycloak 7/10 | Env 4/10 | Integration 6/10
|
||||
- See: `infra-integration-review.md`
|
||||
|
||||
### Phase 5 Review (2026-02-05)
|
||||
- HR modules: Employees, Time Tracking, Absences
|
||||
|
||||
### Phase 3 Review (2026-02-05)
|
||||
- Integration connectors reviewed; module now disabled
|
||||
|
||||
### Phase 1 Review (2024)
|
||||
- JWT validation, type sync, CORS, Keycloak fixes applied
|
||||
|
||||
## Backend Architecture Notes
|
||||
- Global guards chain: JwtAuthGuard -> RolesGuard -> PermissionsGuard
|
||||
- Response envelope via TransformInterceptor: `{success, data, timestamp}`
|
||||
- Global HttpExceptionFilter catches all exceptions, no internal leaks
|
||||
- AES-256-GCM encryption for salary + bank accounts (fixed salt issue noted)
|
||||
- Audit via decorator `@AuditLog()` + global AuditInterceptor
|
||||
- Permissions enum uses entity:action format (e.g., `employees:read`)
|
||||
- DEFAULT_ROLE_PERMISSIONS maps roles to permission arrays
|
||||
@@ -0,0 +1,71 @@
|
||||
# Phase 7 - Full Backend Code Review (2026-02-06)
|
||||
|
||||
## Overall Score: 7.8/10
|
||||
|
||||
## Module Scores
|
||||
|
||||
| Module | Score | Key Issue |
|
||||
|--------|-------|-----------|
|
||||
| Auth (guards, strategy, decorators) | 8/10 | Role names leaked in error messages |
|
||||
| Prisma (service, module) | 7/10 | cleanDatabase() ignores FK constraints |
|
||||
| HR/Employees | 8/10 | `as any` type casts for encrypted data |
|
||||
| HR/Time Tracking | 7/10 | Break tracking via string parsing, private method access |
|
||||
| HR/Absences | 8/10 | Missing holiday calc, vacation carry-over TODO |
|
||||
| LEAN/Skill Matrix | 8/10 | N+1 in analyzeGaps, non-transactional bulkUpsert |
|
||||
| LEAN/Morning Meeting | 7/10 | ALL endpoints use DASHBOARD_VIEW permission |
|
||||
| LEAN/S3 Planning | 8/10 | File upload MIME not validated, N+1 in findAll |
|
||||
| Dashboard | 8/10 | Hardcoded role strings |
|
||||
| Common (filters, interceptors) | 8/10 | Logging/Timeout interceptors never registered |
|
||||
| Users | 8/10 | GET /users has no permission restriction |
|
||||
| Audit | 7/10 | oldData always undefined, salary not sanitized |
|
||||
| app.module + main.ts | 9/10 | enableImplicitConversion risk |
|
||||
|
||||
## Critical Issues (3)
|
||||
|
||||
### 1. Morning Meeting Permission Escalation
|
||||
- **File:** `apps/api/src/modules/lean/morning-meeting/morning-meeting.controller.ts`
|
||||
- **Lines:** 47, 59, 66, 79, 99, 120, 149, 159, 170, 183, 193, 211, 221, 234, 249, 259, 270
|
||||
- **Issue:** ALL endpoints (including create, update, delete) use `Permission.DASHBOARD_VIEW`
|
||||
- **Impact:** Any authenticated user with dashboard access can create/modify/delete morning meetings
|
||||
|
||||
### 2. Private Method Access via Bracket Notation
|
||||
- **File:** `apps/api/src/modules/hr/time-tracking/time-tracking.controller.ts`
|
||||
- **Lines:** 237-239
|
||||
- **Issue:** `this.timeTrackingService['getEmployeeByUserId'](user.sub)` accesses private method
|
||||
- **Impact:** Circumvents TypeScript access modifiers, fragile to refactoring
|
||||
|
||||
### 3. Prisma cleanDatabase() with Promise.all
|
||||
- **File:** `apps/api/src/prisma/prisma.service.ts`
|
||||
- **Lines:** 48-61
|
||||
- **Issue:** Deletes all tables in parallel ignoring foreign key constraints
|
||||
- **Impact:** Can fail in production/staging if FK constraints exist; should use sequential deletion or raw SQL truncate cascade
|
||||
|
||||
## Important Issues (14)
|
||||
|
||||
1. **Roles guard leaks role names** - roles.guard.ts L31-33
|
||||
2. **Permissions guard leaks permission names** - permissions.guard.ts
|
||||
3. **enableImplicitConversion in ValidationPipe** - main.ts L44
|
||||
4. **Break tracking via note string parsing** - time-tracking.service.ts L216-219, L257-258
|
||||
5. **No backdating limit** for manual time entries
|
||||
6. **Holiday calculation missing** in absences calculateWorkingDays
|
||||
7. **Vacation carry-over not implemented** - absences.service.ts L979 (BUrlG)
|
||||
8. **Event queue in-memory only** - absences.service.ts L1414
|
||||
9. **N+1 query in analyzeGaps** - skill-entries.service.ts L515-523
|
||||
10. **bulkUpsert not transactional** - skill-entries.service.ts L429-476
|
||||
11. **File upload MIME not validated** - s3-planning.controller.ts
|
||||
12. **LoggingInterceptor + TimeoutInterceptor never registered**
|
||||
13. **GET /users no permission check** - users.controller.ts L88-97
|
||||
14. **Audit oldData always undefined** - audit.interceptor.ts L60
|
||||
|
||||
## Positive Observations
|
||||
|
||||
- JWT verification (not just decode) with user existence check on every request
|
||||
- AES-256-GCM encryption for sensitive employee data (salary, bank account)
|
||||
- Consistent error response format via global HttpExceptionFilter
|
||||
- Response envelope pattern via TransformInterceptor
|
||||
- ArbZG break rules correctly implemented in time tracking
|
||||
- Comprehensive absence workflow with approval chain
|
||||
- Global ValidationPipe with whitelist + forbidNonWhitelisted
|
||||
- Proper soft delete patterns for employees
|
||||
- Well-structured module hierarchy (HrModule -> sub-modules)
|
||||
- Swagger/OpenAPI documentation on all endpoints
|
||||
Reference in New Issue
Block a user