353 lines
20 KiB
Markdown
353 lines
20 KiB
Markdown
# Phase 5 - Code Quality & Performance Audit
|
|
|
|
## Executive Summary
|
|
|
|
The system currently passes `typecheck`, `lint`, and production `build`, so the codebase is operational and deployable in its current state. The main risk in this phase is not immediate build failure, but accumulated maintainability and performance debt concentrated in several oversized route handlers, server data modules, and workflow-heavy client forms.
|
|
|
|
The most important quality issue is structural complexity. A small set of files currently own too many responsibilities at once: authorization-aware data loading, filtering, export generation, workflow transitions, storage handling, notifications, and UI orchestration. This increases regression risk, makes optimization harder, and reduces testability.
|
|
|
|
The most important performance issue is repeated work inside large reporting and workflow paths rather than obvious catastrophic hotspots. In particular, report generation repeats scope-resolution queries, export logic is tightly coupled to dataset construction, and several large client components keep broad interactive state on the client without further decomposition.
|
|
|
|
## Build/Config Assessment
|
|
|
|
### Observed configuration
|
|
|
|
- `package.json:10-15` defines `dev`, `build`, `lint`, `lint:fix`, and `lint:strict`.
|
|
- There is no dedicated `typecheck` script in `package.json`.
|
|
- `tsconfig.json:5-19` uses `strict: true`, `isolatedModules: true`, and `incremental: true`.
|
|
- `tsconfig.json:5-6` also keeps `allowJs: true` and `skipLibCheck: true`.
|
|
- Linting is done by `oxlint`, not a stricter combined lint-plus-type gate.
|
|
|
|
### Assessment
|
|
|
|
Strengths:
|
|
|
|
- The project already compiles cleanly under production build.
|
|
- TypeScript strict mode is enabled.
|
|
- The repo has a stricter lint mode available through `lint:strict`.
|
|
|
|
Weaknesses:
|
|
|
|
- Missing `typecheck` script makes CI and contributor workflows less explicit.
|
|
- `allowJs: true` weakens long-term type-safety expectations in a TypeScript-first codebase.
|
|
- `skipLibCheck: true` is a practical default, but it can hide dependency typing drift.
|
|
- The default lint command allows warnings to pass, so code-quality drift can accumulate silently.
|
|
|
|
## TypeScript Assessment
|
|
|
|
### Strengths
|
|
|
|
- `npx tsc --noEmit` completed successfully.
|
|
- Most feature modules use explicit contracts and typed query/mutation boundaries.
|
|
- Shared data access patterns appear to prefer typed service layers over untyped ad hoc fetches.
|
|
|
|
### Findings
|
|
|
|
- `src/instrumentation-client.ts` uses `(Sentry as any)`, which weakens observability-related type safety.
|
|
- `src/components/forms/demo-form.tsx` and `src/features/products/schemas/product.ts` use permissive `any`-style schema patterns, suggesting leftover demo/legacy looseness.
|
|
- `src/features/forms/components/sheet-product-form.tsx` uses `productSchema as any`, which bypasses compiler guarantees.
|
|
- `src/lib/api-client.ts:57` throws generic `Error` instances instead of a typed API/domain error model.
|
|
|
|
### Assessment
|
|
|
|
TypeScript quality is acceptable at the baseline level, but not yet fully mature. The compiler settings are strong enough to prevent many failures, while a small set of escape hatches and generic error shapes still reduce confidence in cross-layer correctness.
|
|
|
|
## React Assessment
|
|
|
|
### Strengths
|
|
|
|
- The app reuses shared primitives and a shared query client.
|
|
- Several flows use TanStack Query instead of bespoke client fetching logic.
|
|
- Shared table state is centralized in `src/hooks/use-data-table.ts`, which is a good reuse decision even though the hook is now quite complex.
|
|
|
|
### Findings
|
|
|
|
- `src/features/training-records/components/training-record-form.tsx` is 765 lines and combines form orchestration, uploads, mutations, attachment state, and workflow logic in one client component.
|
|
- `src/features/online-lessons/components/online-lesson-form.tsx` is 554 lines and keeps many local file/removal states plus multiple workflow mutations in one component.
|
|
- `src/features/announcements/components/announcement-form.tsx` uses the same multi-mutation workflow pattern and is trending toward the same shape.
|
|
- `src/features/reports/components/reports-page-content.tsx:278,317` triggers `react(no-unstable-nested-components)` warnings.
|
|
- `src/components/ui/calendar.tsx:58` and `src/components/kbar/render-result.tsx:10` also define nested components during render.
|
|
|
|
### Assessment
|
|
|
|
The main React risk is not hook misuse at scale, but oversized client components with broad responsibility and broad rerender surfaces. These components are harder to reason about, harder to profile, and harder to unit test than smaller view-model plus presentational splits.
|
|
|
|
## Next.js Assessment
|
|
|
|
### Strengths
|
|
|
|
- Production build succeeded under Next.js 16.2.6.
|
|
- The app makes meaningful use of server-side data loading.
|
|
- `src/features/overview/server/overview-data.ts:171,185,249,519` uses `cache()` well in several high-read paths.
|
|
|
|
### Findings
|
|
|
|
- `src/app/api/announcements/[id]/route.ts` is 683 lines and combines multipart parsing, draft/save/submit transitions, storage operations, audit logging, and delete handling in a single route module.
|
|
- `src/lib/api-client.ts:29-41` resolves server request headers and reconstructs origin dynamically on every server request path, which adds per-call overhead and keeps the route-handler pattern tightly coupled to internal HTTP.
|
|
- The build output still includes many legacy/demo screens and routes such as `/dashboard/forms`, `/dashboard/kanban`, `/dashboard/chat`, `/dashboard/product`, and `/dashboard/employees`, which increases bundle and maintenance surface.
|
|
|
|
### Assessment
|
|
|
|
The app is using Next.js capably, but it is not yet taking full advantage of cleaner server boundaries. The biggest framework-level opportunity is reducing workflow complexity inside route handlers and separating internal server logic from HTTP transport concerns.
|
|
|
|
## Validation Assessment
|
|
|
|
### Strengths
|
|
|
|
- The broader codebase follows a Zod-plus-form-wrapper approach.
|
|
- Workflow modules show evidence of server-side validation before persistence.
|
|
|
|
### Findings
|
|
|
|
- Validation quality varies by feature. Demo and product-related areas still contain permissive schema patterns such as `.any()`.
|
|
- File-heavy workflow routes appear to validate through branching procedural logic inside route handlers rather than through smaller reusable validation units.
|
|
- Status/date/file validation logic appears repeated across announcement and online-lesson workflow code paths instead of being fully centralized.
|
|
|
|
### Assessment
|
|
|
|
Validation is good enough for core runtime flows, but consistency is not uniform across the repo. The main weakness is schema fragmentation and procedural validation logic inside large handlers.
|
|
|
|
## Error Handling Assessment
|
|
|
|
### Strengths
|
|
|
|
- Auth/session helpers throw explicit `AuthError` instances for unauthorized and forbidden cases.
|
|
- Route handlers generally return structured JSON responses rather than raw exceptions.
|
|
|
|
### Findings
|
|
|
|
- `src/lib/api-client.ts:57` throws plain `Error`, collapsing HTTP status, domain code, and user-display concerns into one generic error shape.
|
|
- `src/lib/auth/session.ts` contains many repeated `throw new AuthError(...)` branches, which is clear but repetitive and hints at opportunities for more composable authorization guards.
|
|
- Large route handlers and data modules mix business logic with logging and side effects, which makes it harder to guarantee consistent error mapping.
|
|
|
|
### Assessment
|
|
|
|
Error handling is functional but not yet strongly normalized. The biggest gap is the lack of a consistent typed application error model spanning route handlers, services, and client consumers.
|
|
|
|
## Duplicate Code Assessment
|
|
|
|
### Findings
|
|
|
|
- Date formatting is scattered across many files through direct `new Date(...).toISOString()` and `toLocaleDateString(...)` usage, including `src/app/api/announcements/route.ts`, `src/app/api/announcements/[id]/route.ts`, `src/app/api/online-lessons/[id]/route.ts`, `src/app/api/audit-logs/export/route.ts`, and `src/features/reports/components/reports-page-content.tsx:78-83`.
|
|
- Workflow patterns for draft/save/submit/approve/archive actions are conceptually similar between announcements and online lessons, but remain implemented separately.
|
|
- Report rendering repeats table-like configuration and inline mapping logic inside `src/features/reports/components/reports-page-content.tsx`.
|
|
- Authorization guard usage is centralized, but authorization-related branching still appears repeatedly inside high-level server modules.
|
|
|
|
### Assessment
|
|
|
|
The repo has good reuse in core scaffolding, but duplication is still visible in domain workflows, date formatting, report presentation, and status transition logic. This is manageable now, but it will get more expensive as more content modules are added.
|
|
|
|
## Naming & Convention Assessment
|
|
|
|
### Strengths
|
|
|
|
- Feature and file naming is generally readable and follows the project structure.
|
|
- Route names align well with feature intent.
|
|
|
|
### Findings
|
|
|
|
- `src/components/ui/slider.tsx:16` uses `_values`, which triggers a lint naming warning.
|
|
- Mixed legacy/demo/runtime naming remains across the repo, especially in `products`, `forms`, `chat`, `kanban`, and old `employees` areas.
|
|
- Some modules use domain-accurate names but still house too many concerns for the file name to communicate actual scope, for example `report-data.ts` and `overview-data.ts`.
|
|
|
|
### Assessment
|
|
|
|
Naming is mostly not a blocking issue. The larger convention problem is scope drift: files whose names imply a focused responsibility but whose implementations have become mini-subsystems.
|
|
|
|
## Database Query Performance Assessment
|
|
|
|
### Strengths
|
|
|
|
- Several aggregation-heavy paths use SQL composition and `Promise.all` rather than purely sequential querying.
|
|
- Overview data makes a meaningful attempt to cache expensive scope/filter work.
|
|
|
|
### Findings
|
|
|
|
- `src/features/reports/server/report-data.ts:126,247,308,386` repeatedly calls `getScopedEmployeeIds(...)` in multiple report builders, which risks repeated scope-resolution queries within one report/export request.
|
|
- `src/features/reports/server/report-data.ts:314,567,621` mixes broad dataset fetching with export preparation and follow-up joins, making it harder to deduplicate query work.
|
|
- `src/features/permission-management/server/user-permission-data.ts:375` relies on transactional app logic to preserve one-active-template behavior, while the file itself contains a TODO noting the absence of a database-level unique constraint.
|
|
- Large export paths in reports include PDF and Excel generation in the same module as query logic, increasing the chance of over-fetching and memory-heavy processing.
|
|
|
|
### Assessment
|
|
|
|
The main database-performance risk is repeated query work in analytics/reporting flows rather than obvious N+1 loops in everyday CRUD pages. Reporting will likely be the first area to degrade under scale.
|
|
|
|
## Frontend Performance Assessment
|
|
|
|
### Strengths
|
|
|
|
- Shared data-table state management includes debounce support and URL-state integration.
|
|
- Many pages are server-rendered first and hydrate into targeted client interactivity rather than fully client-side shells.
|
|
|
|
### Findings
|
|
|
|
- `src/features/training-records/components/training-record-form.tsx`, `src/features/online-lessons/components/online-lesson-form.tsx`, and `src/features/announcements/components/announcement-form.tsx` keep large interactive workflows entirely in single client components.
|
|
- `src/features/reports/components/reports-page-content.tsx` contains nested render-time component definitions flagged by lint, which can cause unnecessary remounting and rerender churn.
|
|
- The build output still contains many legacy/demo dashboard surfaces, which increases long-term client bundle pressure even if each page is route-split.
|
|
- Heavy UI packages and legacy dashboard features remain installed even where they do not appear central to the TMS runtime.
|
|
|
|
### Assessment
|
|
|
|
Frontend performance risk is moderate. The biggest concerns are rerender breadth and bundle sprawl, not obviously broken runtime performance today.
|
|
|
|
## API Performance Assessment
|
|
|
|
### Strengths
|
|
|
|
- The production build completed successfully, which suggests current route composition is at least operational.
|
|
- Many server modules already use batching through `Promise.all`.
|
|
|
|
### Findings
|
|
|
|
- `src/app/api/announcements/[id]/route.ts:88,162-623` performs multipart parsing, state transition branching, storage actions, and many audit log writes in one path.
|
|
- The same route logs multiple events in sequence at `445,458,475,522,549,573,609,623`, and only one section uses `Promise.all` at `492`.
|
|
- `src/app/api/online-lessons/[id]/route.ts` follows a similar content-workflow route shape, increasing the chance of repeated sequential I/O.
|
|
- `src/lib/api-client.ts:41` routes internal feature services through HTTP fetch calls, which is acceptable for consistency but adds overhead versus direct server-side composition when both sides live in the same app.
|
|
|
|
### Assessment
|
|
|
|
API performance is acceptable for current scale, but workflow-heavy content endpoints are likely to become latency hotspots because they combine validation, persistence, storage, notification, and audit concerns in one request lifecycle.
|
|
|
|
## Dead Code Assessment
|
|
|
|
### Findings
|
|
|
|
- `npm run lint` reports unused imports and variables in active code:
|
|
- `src/components/ui/duration-picker.tsx:13`
|
|
- `src/features/online-lessons/components/online-lesson-form.tsx:121`
|
|
- `src/features/organizers/components/organizers-page.tsx:31`
|
|
- `src/features/reports/components/reports-page-content.tsx:120`
|
|
- `src/app/api/online-lessons/[id]/route.ts:240`
|
|
- `src/features/reports/server/report-data.ts:8,11,12`
|
|
- The build output confirms many legacy/demo pages and features are still present in the shipped app tree.
|
|
- `src/constants/mock-api.ts` and `src/constants/mock-api-users.ts` still exist and receive lint warnings, indicating legacy/demo residue remains in the repository.
|
|
|
|
### Assessment
|
|
|
|
Dead code is not overwhelming, but there is enough residue to create noise, hide real issues, and expand the maintenance surface beyond the actual TMS runtime.
|
|
|
|
## Testability Assessment
|
|
|
|
### Strengths
|
|
|
|
- Some data access is already separated into feature-level server modules.
|
|
- Shared hooks and shared auth helpers create a starting point for isolated testing.
|
|
|
|
### Findings
|
|
|
|
- `src/features/reports/server/report-data.ts` couples filtering, authorization scoping, querying, table shaping, and export generation in one file.
|
|
- `src/features/overview/server/overview-data.ts` couples multiple dashboard datasets and policy/filter logic in one file despite good cache usage.
|
|
- `src/app/api/announcements/[id]/route.ts` mixes multipart parsing, workflow policy, storage, database updates, notifications, and auditing, which is difficult to unit test without broad integration scaffolding.
|
|
- `src/hooks/use-data-table.ts` is valuable shared infrastructure, but its breadth makes it a likely source of subtle regressions and harder isolated testing.
|
|
|
|
### Assessment
|
|
|
|
Testability is the clearest structural weakness after maintainability. The codebase is not short on logic separation everywhere, but its heaviest business flows are still assembled in units that are larger than ideal for focused tests.
|
|
|
|
## Findings by Severity
|
|
|
|
### High
|
|
|
|
1. Oversized workflow handlers and data modules concentrate too many responsibilities.
|
|
- Evidence: `src/app/api/announcements/[id]/route.ts` (683 lines), `src/features/reports/server/report-data.ts` (759 lines), `src/features/overview/server/overview-data.ts` (773 lines), `src/features/training-records/components/training-record-form.tsx` (765 lines).
|
|
|
|
2. Reporting paths repeat scope-resolution work and combine querying with export generation.
|
|
- Evidence: `src/features/reports/server/report-data.ts:126,247,308,386,766-822`.
|
|
|
|
3. Core workflow-heavy client forms are broad client boundaries with large rerender surfaces and low testability.
|
|
- Evidence: `training-record-form.tsx`, `online-lesson-form.tsx`, `announcement-form.tsx`.
|
|
|
|
### Medium
|
|
|
|
1. Error handling is not yet consistently typed across service and client boundaries.
|
|
- Evidence: `src/lib/api-client.ts:57`.
|
|
|
|
2. Database integrity for active permission-template assignment relies on app logic instead of a DB constraint.
|
|
- Evidence: TODO in `src/features/permission-management/server/user-permission-data.ts`, transaction at `375`.
|
|
|
|
3. Duplicate date formatting and workflow status handling remain spread across many routes and components.
|
|
|
|
4. Default lint workflow allows warnings to pass, and no dedicated `typecheck` script exists.
|
|
|
|
5. Legacy/demo modules continue to expand bundle and maintenance surface.
|
|
|
|
### Low
|
|
|
|
1. Small unused variables/imports remain in active modules.
|
|
2. Naming/convention drift exists in a few UI utility files and legacy/demo areas.
|
|
|
|
## Quick Wins
|
|
|
|
1. Add a dedicated `typecheck` script and run it in CI with `lint:strict`.
|
|
2. Remove current unused imports/variables reported by `oxlint`.
|
|
3. Centralize date formatting helpers for API payloads and Thai display formatting.
|
|
4. Extract nested render-time component definitions flagged by lint.
|
|
5. Split report export formatting from report data retrieval.
|
|
|
|
## Structural Improvements
|
|
|
|
1. Break `report-data.ts` into scoped-query, dataset-builder, and export-renderer modules.
|
|
2. Break `overview-data.ts` into dashboard-section loaders plus a smaller composition layer.
|
|
3. Move announcement and online-lesson workflow transition logic into dedicated domain services invoked by thin route handlers.
|
|
4. Split large client forms into smaller sections with isolated mutation/upload adapters.
|
|
5. Introduce a typed application error model shared by route handlers, service callers, and client UI.
|
|
6. Audit legacy/demo routes and dependencies for removal or quarantine outside the production dashboard surface.
|
|
|
|
## Recommended Refactor Order
|
|
|
|
1. Reports server path
|
|
- Highest combined maintainability, query-efficiency, and export-performance payoff.
|
|
|
|
2. Announcement and online-lesson workflow handlers
|
|
- Highest API complexity and side-effect density.
|
|
|
|
3. Large client workflow forms
|
|
- Best follow-up once workflow services are thinner and easier to consume.
|
|
|
|
4. Error model and date-format normalization
|
|
- Broad cross-cutting cleanup with good leverage.
|
|
|
|
5. Legacy/demo surface reduction
|
|
- Useful after core runtime paths are stabilized.
|
|
|
|
## Commands Executed and Results
|
|
|
|
- `npx tsc --noEmit`
|
|
- Passed.
|
|
- `npm run lint`
|
|
- Passed with warnings. Key warnings included unused imports/variables and `react(no-unstable-nested-components)`.
|
|
- `npm run build`
|
|
- Passed on Next.js `16.2.6` with production build and static page generation completed successfully.
|
|
- Multiple `rg` and `Select-String` read-only scans
|
|
- Used to inspect large files, duplicate patterns, lint targets, and route complexity.
|
|
|
|
## Files Reviewed
|
|
|
|
- `package.json`
|
|
- `tsconfig.json`
|
|
- `src/lib/api-client.ts`
|
|
- `src/lib/auth/session.ts`
|
|
- `src/lib/query-client.ts`
|
|
- `src/hooks/use-data-table.ts`
|
|
- `src/features/reports/server/report-data.ts`
|
|
- `src/features/overview/server/overview-data.ts`
|
|
- `src/features/permission-management/server/user-permission-data.ts`
|
|
- `src/features/reports/components/reports-page-content.tsx`
|
|
- `src/features/training-records/components/training-record-form.tsx`
|
|
- `src/features/online-lessons/components/online-lesson-form.tsx`
|
|
- `src/features/announcements/components/announcement-form.tsx`
|
|
- `src/app/api/announcements/[id]/route.ts`
|
|
- `src/app/api/online-lessons/[id]/route.ts`
|
|
- `src/components/ui/calendar.tsx`
|
|
- `src/components/ui/duration-picker.tsx`
|
|
- `src/components/ui/slider.tsx`
|
|
- `src/constants/mock-api.ts`
|
|
- `src/constants/mock-api-users.ts`
|
|
|
|
## Areas Not Verified
|
|
|
|
- No runtime profiling was performed in a browser or with production tracing, so rerender cost and interaction latency were assessed from structure and lint/tool evidence rather than flamegraphs.
|
|
- No database execution plans were captured, so index/selectivity findings are risk-based rather than EXPLAIN-verified.
|
|
- No bundle analyzer output was generated, so bundle-size findings are based on route inventory and dependency surface, not measured chunk sizes.
|
|
- No automated test suite review was performed because this phase focused on code quality, static analysis, and build/runtime structure.
|