263 lines
9.8 KiB
Markdown
263 lines
9.8 KiB
Markdown
# Frontend Improvement Report
|
|
|
|
## 1. Remove Duplicate `.js` / `.tsx` Files — Code Slop
|
|
|
|
**Severity: High**
|
|
|
|
The codebase contains leftover `.js` files from an incomplete TypeScript migration. Every significant source file has a `.tsx`/`.ts` counterpart, making the `.js` versions dead code that adds confusion and maintenance burden.
|
|
|
|
Files to delete:
|
|
```
|
|
src/api/client.js
|
|
src/App.js
|
|
src/main.js
|
|
src/types/index.js
|
|
src/utils/chartHelpers.js
|
|
src/utils/time.js
|
|
src/components/tracker/SleepButton.js
|
|
src/components/tracker/TrackerPage.js
|
|
src/hooks/useSettings.js
|
|
src/hooks/useSleepSession.js
|
|
src/hooks/useStats.js
|
|
```
|
|
|
|
---
|
|
|
|
## 2. Dead Code — `chartHelpers.ts` exports never called — Code Slop / Duplication
|
|
|
|
**Severity: High**
|
|
|
|
All 7 exported functions in `src/utils/chartHelpers.ts` are tested but **never imported or called** from any component. Components duplicate the same logic inline instead of delegating to these utilities.
|
|
|
|
Dead exports:
|
|
- `buildTimetableSlots()` — reimplemented inline in `SleepTimetable.tsx`
|
|
- `computeWakeupMinutes()` — reimplemented inline in `WakeupPlot.tsx:31-44`
|
|
- `computeBedtimeMinutes()` — reimplemented inline in `BedtimePlot.tsx:31-44`
|
|
- `computeDailySummaryData()` — reimplemented inline in `DailySummary.tsx:41-48`
|
|
- `computeAverageStats()` — unused
|
|
- `reverseDateOrder()` — reimplemented inline in multiple chart components
|
|
- `filterDaysWithData()` — unused
|
|
|
|
**Fix options (pick one):**
|
|
- **Preferred:** Refactor chart components to call the helpers instead of reimplementing — removes ~60 lines of duplicated transformation code.
|
|
- **Alternative:** Delete the helpers and the tests for them — the logic lives only in the components.
|
|
|
|
Either way, having tested utilities that the application never actually uses is misleading and inflates the apparent test coverage.
|
|
|
|
---
|
|
|
|
## 3. Duplicated Datetime Formatting Logic — Duplication
|
|
|
|
**Severity: Medium**
|
|
|
|
`toLocalDatetimeInputValue()` is implemented in `src/utils/time.ts:40` _and_ again locally in `src/components/tracker/TimeAdjuster.tsx:8-19`. Additionally, `src/components/tracker/SessionList.tsx:6-24` defines its own `formatDateTime()` and `toDatetimeLocal()` helpers that overlap with what `time.ts` already provides.
|
|
|
|
**Fix:** Consolidate all datetime formatting into `src/utils/time.ts`. Have `TimeAdjuster` and `SessionList` import from there instead of reimplementing.
|
|
|
|
---
|
|
|
|
## 4. Duplicated Chart Data Transformation — Duplication / Simplification
|
|
|
|
**Severity: Medium**
|
|
|
|
`WakeupPlot.tsx:31-44` and `BedtimePlot.tsx:31-44` contain near-identical map → reverse → daysAgo pipelines:
|
|
|
|
```typescript
|
|
// Both files do essentially this:
|
|
const chartData = (data ?? [])
|
|
.map((d) => ({
|
|
...d,
|
|
daysAgo: Math.round((todayMs - new Date(d.date).getTime()) / MS_PER_DAY),
|
|
minutes: adjustForTimezone(d.someTime),
|
|
}))
|
|
.reverse();
|
|
```
|
|
|
|
The only difference is which field (`wakeup_time` vs `bedtime`) is mapped to `minutes`.
|
|
|
|
**Fix:** Extract a single `buildTimeSeriesChartData(data, timeField)` helper (in `chartHelpers.ts`) and call it from both components.
|
|
|
|
---
|
|
|
|
## 5. Loop Inefficiency — Performance
|
|
|
|
**Severity: Low**
|
|
|
|
`WakeupPlot.tsx:31-34` and `BedtimePlot.tsx:31-34` call `new Date(getToday()).getTime()` _inside_ the `.map()` callback, re-creating the same `Date` object on every iteration.
|
|
|
|
```typescript
|
|
// Current (creates Date on every iteration)
|
|
.map((d) => ({
|
|
daysAgo: Math.round((new Date(getToday()).getTime() - ...) / MS_PER_DAY),
|
|
}))
|
|
|
|
// Fix: hoist above the map
|
|
const todayMs = new Date(getToday()).getTime();
|
|
const chartData = (data ?? []).map((d) => ({
|
|
daysAgo: Math.round((todayMs - ...) / MS_PER_DAY),
|
|
}));
|
|
```
|
|
|
|
---
|
|
|
|
## 6. `any` Type Usage — Code Quality / Type Safety
|
|
|
|
**Severity: Medium**
|
|
|
|
Five locations use `any`, undermining the value of TypeScript:
|
|
|
|
| File | Line | Issue |
|
|
|------|------|-------|
|
|
| `src/components/tracker/SleepButton.tsx` | 19 | `catch (error: any)` |
|
|
| `src/components/stats/WakeupPlot.tsx` | 74, 77 | Recharts `formatter=(value: any)` |
|
|
| `src/components/stats/BedtimePlot.tsx` | 74, 77 | Recharts `formatter=(value: any)` |
|
|
| `src/components/stats/DailySummary.tsx` | 98, 101 | Recharts `formatter=(value: any, name: any)` |
|
|
| `src/hooks/useSleepSession.ts` | 58 | `payload: any` in mutation function |
|
|
|
|
**Fixes:**
|
|
- Catch blocks: use `catch (error: unknown)` and narrow with `error instanceof Error`.
|
|
- Recharts formatters: the Recharts library exports `ValueType` and `NameType` — use those, or declare a local `type RechartsFormatter = (value: number, name: string) => ReactNode`.
|
|
- `useSleepSession` payload: create a `SleepSessionUpdatePayload` type in `src/types/index.ts` and use it.
|
|
|
|
---
|
|
|
|
## 7. Unsafe User Input Parsing — Security
|
|
|
|
**Severity: Medium**
|
|
|
|
`src/components/tracker/SessionList.tsx:40-41` parses datetime-local input values directly without validation:
|
|
|
|
```typescript
|
|
start_time: new Date(editStart).toISOString(),
|
|
end_time: editEnd ? new Date(editEnd).toISOString() : null,
|
|
```
|
|
|
|
If `editStart` is an empty string or a malformed value, `new Date(editStart)` produces an `Invalid Date` object and `.toISOString()` throws a `RangeError`, which bubbles up as an unhandled rejection.
|
|
|
|
**Fix:** Validate before submitting:
|
|
```typescript
|
|
const parsedStart = new Date(editStart);
|
|
if (isNaN(parsedStart.getTime())) {
|
|
// show error to user
|
|
return;
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
## 8. `alert()` for Error Messages — Code Slop
|
|
|
|
**Severity: Low**
|
|
|
|
`src/components/tracker/SessionList.tsx:47` uses the browser native `alert()` on save failure:
|
|
```typescript
|
|
alert('Failed to update session');
|
|
```
|
|
|
|
This blocks the UI thread, looks out of place in a modern app, and cannot be styled or dismissed programmatically.
|
|
|
|
**Fix:** Replace with a toast notification or an inline error state rendered in the component.
|
|
|
|
---
|
|
|
|
## 9. Missing Memoisation — Performance
|
|
|
|
**Severity: Low**
|
|
|
|
Several components create new function references on every render, which defeats React's reconciliation optimisations for any child that uses `React.memo` or `useCallback` itself:
|
|
|
|
- `src/components/tracker/SettingsPopover.tsx:72,82,92,102` — four `onChange` handlers as inline arrow functions
|
|
- `src/components/tracker/SessionList.tsx:76,85,91,98,109` — five inline handlers
|
|
- `src/components/tracker/TimeAdjuster.tsx:46` — inline `onChange`
|
|
- `src/pages/StatsPage.tsx:44` — inline `onChange`
|
|
|
|
**Fix:** Wrap each with `useCallback`. For purely presentational components with stable props (`IntervalPicker`, `Navbar`, chart components), wrap with `React.memo` to prevent unnecessary re-renders.
|
|
|
|
---
|
|
|
|
## 10. No Route-Level Code Splitting — Performance
|
|
|
|
**Severity: Medium**
|
|
|
|
`src/App.tsx:14-16` eagerly imports all page components:
|
|
```typescript
|
|
import TrackerPage from './components/tracker/TrackerPage';
|
|
import StatsPage from './components/stats/StatsPage';
|
|
```
|
|
|
|
`StatsPage` pulls in Recharts (a large library) on the initial load even when the user only visits the tracker tab.
|
|
|
|
**Fix:**
|
|
```typescript
|
|
const StatsPage = React.lazy(() => import('./components/stats/StatsPage'));
|
|
// Wrap route in <Suspense fallback={<LoadingSpinner />}>
|
|
```
|
|
|
|
---
|
|
|
|
## 11. Missing Tests — Testing
|
|
|
|
**Severity: High**
|
|
|
|
Most components have zero unit tests. The ones most at risk:
|
|
|
|
| Component | Gap |
|
|
|-----------|-----|
|
|
| `SessionList` | Edit flow, validation, delete confirmation |
|
|
| `SleepButton` | Optimistic state, error handling |
|
|
| `StatusBanner` | Conditional rendering, active session display |
|
|
| `SettingsPopover` | Click-outside dismiss, input changes persisted |
|
|
| `DailySummary` | Data transformation rendering |
|
|
| `useSleepSession` | Mutation callbacks, query invalidation |
|
|
|
|
**Recommended additions:**
|
|
- Unit tests for `SessionList` edit/save/discard/delete using Testing Library + MSW for API mocks
|
|
- Hook tests for `useSleepSession` verifying query invalidation after mutations
|
|
- Snapshot or rendering smoke tests for all chart components to catch regressions when data shape changes
|
|
- Test that invalid date inputs in `SessionList` are rejected before submission
|
|
|
|
---
|
|
|
|
## 12. Coarse `isLoading` in `useStats` — Code Quality
|
|
|
|
**Severity: Low**
|
|
|
|
`src/hooks/useStats.ts:32` ORs together three query loading flags:
|
|
```typescript
|
|
const isLoading = dailyQuery.isLoading || timetableQuery.isLoading || wakeupQuery.isLoading;
|
|
```
|
|
|
|
This means consumers cannot show partial data while other queries are still loading — the whole stats page shows a spinner until _all three_ queries complete.
|
|
|
|
**Fix:** Either export per-query loading states so consumers can render progressively, or use `isFetching` instead of `isLoading` if you want to show stale data while refreshing.
|
|
|
|
---
|
|
|
|
## 13. Missing Error Boundaries on Chart Components — Reliability
|
|
|
|
**Severity: Medium**
|
|
|
|
There is only one global `ErrorBoundary` wrapping the whole app. If any chart's data transformation throws (e.g. a malformed date from the API), it takes down the entire stats page.
|
|
|
|
**Fix:** Wrap each chart component in its own `ErrorBoundary` that renders a "Failed to load chart" fallback. This is a two-line change per chart using a reusable boundary component.
|
|
|
|
---
|
|
|
|
## Summary Table
|
|
|
|
| # | Category | File(s) | Severity |
|
|
|---|----------|---------|----------|
|
|
| 1 | Code slop — dead .js files | Multiple | High |
|
|
| 2 | Dead code / duplication | `chartHelpers.ts` | High |
|
|
| 3 | Duplication | `TimeAdjuster.tsx`, `SessionList.tsx`, `time.ts` | Medium |
|
|
| 4 | Duplication | `WakeupPlot.tsx`, `BedtimePlot.tsx` | Medium |
|
|
| 5 | Performance — loop | `WakeupPlot.tsx`, `BedtimePlot.tsx` | Low |
|
|
| 6 | Type safety — `any` | Multiple | Medium |
|
|
| 7 | Security — unvalidated input | `SessionList.tsx:40-41` | Medium |
|
|
| 8 | Code slop — `alert()` | `SessionList.tsx:47` | Low |
|
|
| 9 | Performance — memoisation | `SettingsPopover.tsx`, `SessionList.tsx` | Low |
|
|
| 10 | Performance — code splitting | `App.tsx` | Medium |
|
|
| 11 | Testing | Most components | High |
|
|
| 12 | Code quality — loading state | `useStats.ts:32` | Low |
|
|
| 13 | Reliability — error boundaries | Stats components | Medium |
|