diff --git a/backend_improvements.md b/backend_improvements.md new file mode 100644 index 0000000..e761a95 --- /dev/null +++ b/backend_improvements.md @@ -0,0 +1,320 @@ +# Backend Improvement Report + +## 1. No Authentication — Security + +**Severity: Critical** + +There is zero authentication on any endpoint. Any caller with network access can create, read, update, and delete all sleep session data. CORS configuration in `app/main.py:18-24` restricts browser origins but does nothing to protect against direct HTTP clients (curl, scripts, etc.). + +**Recommended fix (incremental):** +1. **Immediate:** Add a single shared API key checked via an `Authorization: Bearer ` header using a FastAPI dependency. The key is stored in an environment variable. +2. **Longer term:** Proper user authentication if the app ever becomes multi-user. + +Example dependency: +```python +from fastapi import Security, HTTPException +from fastapi.security import HTTPBearer, HTTPAuthorizationCredentials + +bearer = HTTPBearer() + +def verify_api_key(creds: HTTPAuthorizationCredentials = Security(bearer)): + if creds.credentials != settings.API_KEY: + raise HTTPException(status_code=401, detail="Unauthorized") +``` + +--- + +## 2. O(days × slots × sessions) Timetable Query — Performance + +**Severity: Critical** + +`app/routers/stats.py:77-99` contains a triple-nested Python loop to build the timetable: + +```python +while current_date <= to_date.date(): + for i in range(96): # 96 fifteen-minute slots per day + for s in sessions: # ALL sessions loaded into memory + if s.start_time <= slot_end and (s.end_time is None or s.end_time >= slot_start): + is_sleeping = True + break +``` + +With 30 days and 1,000 sessions this is ~2.88 million comparisons executed in Python. The comparison work should happen in the database. + +**Fix:** For each day, run a single SQL query that returns the sessions overlapping that day's range, then build the 96 slots against only the relevant sessions. Alternatively, generate a CTE of slot boundaries and join against `sleep_session` in a single query. Even the naive "one query per day" approach is far faster than the current Python loop because the database uses the index (see item 4). + +--- + +## 3. In-Python Aggregation in Other Stats Endpoints — Performance + +**Severity: High** + +The same load-everything-then-loop pattern appears in the other two stats endpoints: + +- `/api/stats/daily-summary` (`stats.py:28-47`): loads all sessions for the date range, then loops through each one in Python to compute per-day totals. +- `/api/stats/wake-sleep-times` (`stats.py:119-134`): loads all sessions, then loops per day to find the first wakeup and last bedtime. + +Both of these can be replaced with a single SQL GROUP BY query per endpoint: +- `daily-summary`: `SELECT DATE(start_time), SUM(...) FROM sleep_session WHERE ... GROUP BY DATE(start_time)` +- `wake-sleep-times`: `SELECT DATE(start_time), MIN(start_time), MAX(end_time) FROM sleep_session WHERE ... GROUP BY DATE(start_time)` + +This removes the O(sessions × days) Python loop entirely. + +--- + +## 4. Missing Database Indexes — Performance + +**Severity: High** + +Every stats query and the session list query filter and sort on `start_time` and `end_time`, but neither column has an index in `app/models.py`. + +**Fix:** Add indexes to the model: +```python +from sqlmodel import Field, SQLModel, Index + +class SleepSession(SQLModel, table=True): + ... + +# after the class definition: +Index("ix_sleep_session_start_time", SleepSession.__table__.c.start_time) +Index("ix_sleep_session_end_time", SleepSession.__table__.c.end_time) +``` + +Or declare them inline with `sa_column`: +```python +start_time: datetime = Field(sa_column=Column(DateTime, index=True)) +``` + +This is especially important for the timetable query once it is pushed into SQL. + +--- + +## 5. Magic Return Types on Session End — Code Quality / Simplification + +**Severity: High** + +`app/crud.py:57-82` returns `Union[SleepSession, bool, str, None]` where the string value is the magic sentinel `"invalid"`: + +```python +# crud.py +if session is None: + return None +if session.end_time is not None: + return False +if ...: + return "invalid" +return session +``` + +The caller in `app/routers/sessions.py:64-73` then has to chain `if result is None`, `if result is False`, `if result == "invalid"` checks. This is fragile, untestable without string matching, and breaks static analysis. + +**Fix:** Define typed exceptions and raise them: +```python +class SessionNotFoundError(Exception): pass +class SessionAlreadyEndedError(Exception): pass +class InvalidEndTimeError(Exception): pass +``` + +`crud.py` raises, `sessions.py` catches and maps to the appropriate `HTTPException`. The function return type becomes simply `SleepSession`. + +--- + +## 6. Silent Error Suppression in Stats — Code Quality / Documentation + +**Severity: Medium** + +`app/routers/stats.py:24-25` and `114-115` catch invalid query parameter values and silently return empty data: + +```python +try: + day_start_time = time.fromisoformat(day_start) +except ValueError: + return {"days": []} # client gets empty data with no explanation +``` + +The client has no way to distinguish "no data for this range" from "your parameters were malformed". This makes debugging difficult. + +**Fix:** Return a `422 Unprocessable Entity` with a descriptive message: +```python +from fastapi import HTTPException + +try: + day_start_time = time.fromisoformat(day_start) +except ValueError: + raise HTTPException(status_code=422, detail=f"Invalid day_start format: {day_start!r}") +``` + +Alternatively, use FastAPI's `Query()` with a regex validator so malformed values are rejected before the handler runs. + +--- + +## 7. `updated_at` Field Never Updated — Bug + +**Severity: Medium** + +`app/models.py` declares an `updated_at` column, but `app/crud.py:106-113` (the update handler) never sets it. After any PATCH, `updated_at` retains the creation timestamp, making it misleading. + +**Fix:** In the update function, add: +```python +session.updated_at = datetime.utcnow() +``` + +--- + +## 8. Dead Code — Code Slop + +**Severity: Medium** + +Three functions are defined but never called anywhere in the application: + +| Function | File | Lines | +|----------|------|-------| +| `ensure_aware()` | `app/crud.py` | 7-10 | +| `minutes_after()` | `app/utils/time_utils.py` | 60-64 | + +Additionally, `app/utils/time_utils.py` exports `day_start_pattern()` which is only used internally within `get_day_night_splits()` — it does not need to be a module-level export. + +**Fix:** Delete `ensure_aware()` and `minutes_after()`. Make `day_start_pattern()` a local helper (leading underscore or inline). + +--- + +## 9. Duplicate Test File — Code Slop + +**Severity: Medium** + +`tests/test_sessions_fixed.py` is an exact copy of `tests/test_sessions.py`. It appears to be a temporary file that was never cleaned up. + +**Fix:** Delete `tests/test_sessions_fixed.py`. + +--- + +## 10. No Application Logging — Observability + +**Severity: Medium** + +There are zero log statements anywhere in the backend. No requests, errors, slow queries, or business events are recorded. This makes diagnosing production issues entirely dependent on guesswork. + +**Fix:** Add structured logging using Python's `logging` module (or `structlog` for structured output). At minimum log: +- Every request with method, path, and response status (can use a FastAPI middleware or Starlette's built-in `ServerErrorMiddleware`) +- Unhandled exceptions with stack traces +- Slow database queries (via SQLAlchemy event listeners if needed) +- Business events: session started, session ended, session deleted + +Example setup in `main.py`: +```python +import logging +logging.basicConfig(level=settings.LOG_LEVEL, format="%(asctime)s %(levelname)s %(name)s %(message)s") +logger = logging.getLogger(__name__) +``` + +Note: `app/config.py` already has a `LOG_LEVEL` setting — it is currently configured but never consumed. + +--- + +## 11. No Rate Limiting — Security + +**Severity: Medium** + +There is no rate limiting on any endpoint. A client could spam POST requests to `/api/sessions` flooding the database, or hammer the expensive timetable endpoint. + +**Fix:** Add `slowapi` (a FastAPI-compatible rate limiter): +```python +from slowapi import Limiter +from slowapi.util import get_remote_address + +limiter = Limiter(key_func=get_remote_address) + +@router.post("/sessions") +@limiter.limit("10/minute") +async def create_session(...): ... +``` + +A reasonable starting limit: 10 writes/minute per IP, 60 reads/minute per IP. + +--- + +## 12. Missing Test Coverage — Testing + +**Severity: High** + +Current test coverage has significant gaps: + +| Area | Current state | +|------|--------------| +| `/api/stats/timetable` | 1 basic happy-path test | +| `/api/stats/daily-summary` | 1 basic happy-path test | +| `/api/stats/wake-sleep-times` | No tests | +| Date boundary conditions | No tests | +| Concurrent session creation | No tests | +| Invalid query parameters | No tests | + +**Recommended additions:** +- Parametrised tests for all stats endpoints covering: empty date ranges, single session spanning midnight, session starting before range and ending inside it, multiple sessions on the same day. +- Test that invalid `day_start`/`day_end` values return 422 (once fix #6 is applied). +- Test that `limit` parameter is bounded (once fix #14 is applied). +- Regression test asserting `updated_at` changes after a PUT request (covers fix #7). + +--- + +## 13. Unused Dependency — Code Slop + +**Severity: Low** + +`python-multipart` is listed in `pyproject.toml:13` but is not used by this application (FastAPI only requires it if you use `Form` parameters, which this API does not). + +**Fix:** Remove the dependency: +``` +uv remove python-multipart +``` + +--- + +## 14. Unbounded `limit` Parameter — Security / Reliability + +**Severity: Medium** + +`app/routers/sessions.py` accepts a `limit` query parameter with default 200 and no upper bound. A caller can pass `limit=10000000` and force a full table scan returning all rows. + +**Fix:** Use FastAPI's `Query` with a `le` constraint: +```python +from fastapi import Query + +async def get_sessions(limit: int = Query(default=200, ge=1, le=1000)): +``` + +--- + +## 15. Hardcoded CORS Origin — Configuration + +**Severity: Low** + +`app/config.py:7` hardcodes the CORS allowed origin: +```python +CORS_ORIGINS: str = "http://localhost:5173" +``` + +This is already an env-var-overridable `Settings` field (via Pydantic's `BaseSettings`), so the mechanism is correct. But the default value means local development works without configuration while production requires the env var to be set — if it is forgotten, the production frontend gets CORS errors. + +**Documentation fix:** Add a comment or a startup check that warns if the running environment looks like production but `CORS_ORIGINS` still has the localhost default. + +--- + +## Summary Table + +| # | Category | File(s) | Severity | +|---|----------|---------|----------| +| 1 | Security — no auth | All routers | Critical | +| 2 | Performance — O(n³) loop | `stats.py:77-99` | Critical | +| 3 | Performance — in-Python aggregation | `stats.py:28-47`, `119-134` | High | +| 4 | Performance — missing indexes | `models.py` | High | +| 5 | Code quality — magic return types | `crud.py:57-82`, `sessions.py:64-73` | High | +| 6 | Code quality — silent error suppression | `stats.py:24-25`, `114-115` | Medium | +| 7 | Bug — `updated_at` not updated | `crud.py:106-113` | Medium | +| 8 | Code slop — dead functions | `crud.py:7-10`, `time_utils.py:60-64` | Medium | +| 9 | Code slop — duplicate test file | `tests/test_sessions_fixed.py` | Medium | +| 10 | Observability — no logging | All files | Medium | +| 11 | Security — no rate limiting | All routers | Medium | +| 12 | Testing — missing coverage | `tests/` | High | +| 13 | Code slop — unused dependency | `pyproject.toml:13` | Low | +| 14 | Security — unbounded limit param | `sessions.py` | Medium | +| 15 | Configuration — hardcoded CORS | `config.py:7` | Low | diff --git a/frontend/playground_plan.md b/frontend/playground_plan.md new file mode 100644 index 0000000..a825fd2 --- /dev/null +++ b/frontend/playground_plan.md @@ -0,0 +1,56 @@ +# Component Playground Implementation Plan + +This document outlines the plan to implement a `/playground` route in the `babymonitor` frontend, allowing for isolated component development and testing with realistic mock data. + +## Goal +Create a route `/playground` that provides a dropdown menu to select any UI component from the codebase. When selected, the component will be rendered in isolation with its required props and necessary TanStack Query cache data pre-populated with realistic mock values. + +## Target Components +The playground will cover components from both the `tracker` and `stats` directories: + +### Tracker Components +- `StatusBanner` +- `TimeAdjuster` +- `SleepButton` + +### Stats Components +- `DailySummary` +- `WakeupPlot` +- `BedtimePlot` +- `SleepTimetable` +- `SessionList` +- `IntervalPicker` +- `SettingsPopover` + +## Technical Approach + +### 1. Component Registry Definition +I will define a registry within `Playground.tsx` that maps component names to: +- The React Component itself. +- A function to generate required `props`. +- An array of TanStack Query `queryKeys` that need to be populated in the cache. + +### 2. Mock Data Injection Strategy +To avoid modifying existing hooks (like `useSessions` or `useStats`), I will leverage the TanStack `QueryClient`. +- When a component is selected, a `useEffect` will trigger. +- This effect will use `queryClient.setQueryData(key, mockData)` for every key associated with that component in the registry. +- This ensures that when the component calls its respective hook, it finds the mock data already in the cache. + +### 3. Implementation Details +- **File Creation**: `frontend/src/components/Playground.tsx` will house the main logic. +- **Routing**: `frontend/src/App.tsx` will be updated to include the new `} />`. +- **Dependencies**: The playground will wrap its content in a `QueryClientProvider` and `ThemeProvider` (if applicable) to mimic the production environment. +- **Styling**: Tailwind CSS will be used for the dropdown and layout to maintain consistency with the existing design system. + +## Mock Data Schema +I will implement realistic data based on the types defined in `frontend/src/types/index.ts`: +- **Sessions**: A list of `SleepSessionRead` objects including `start_time`, `end_time`, and varied durations. +- **Stats**: 7 days of `DailySummaryDay`, `TimetableSlot`, and `WakeupBedtimeDay` data to populate charts and grids. +- **Settings**: Default values for `dayStart`, `dayEnd`, etc., to satisfy the `useSettings` hook. + +## Step-by-Step Execution +1. **Create** `frontend/src/components/Playground.tsx`. +2. **Define** the component registry and mock data generators inside `Playground.tsx`. +3. **Implement** the selection logic and cache injection mechanism in `Playground.tsx`. +4. **Update** `frontend/src/App.tsx` to register the route. +5. **Verify** (via manual instruction) that components render correctly without backend connectivity. diff --git a/frontend/src/components/stats/BedtimePlot.tsx b/frontend/src/components/stats/BedtimePlot.tsx index 413430a..e272fbb 100644 --- a/frontend/src/components/stats/BedtimePlot.tsx +++ b/frontend/src/components/stats/BedtimePlot.tsx @@ -1,6 +1,6 @@ import { useEffect, useState } from 'react'; import { useStats } from '../../hooks/useStats'; -import { formatMinutesAfter, getToday } from '../../utils/time'; +import { formatMinutesAfter, getToday, getLocalOffsetFromReference } from '../../utils/time'; import { LineChart, Line, @@ -32,12 +32,13 @@ const BedtimePlot: React.FC = ({ from_date, to_date, wakeup_ const daysAgo = Math.floor( (new Date(getToday()).getTime() - new Date(day.date).getTime()) / (1000 * 60 * 60 * 24) ); + const offsetFromRef = day.bedtime_minutes !== null ? getLocalOffsetFromReference(day.bedtime_minutes, bedtime_ref) : 0; return { date: daysAgo === 0 ? 'Today' : `${daysAgo}d ago`, - minutes: day.bedtime_minutes ?? 0, + minutes: offsetFromRef, time: day.bedtime_minutes !== null - ? formatMinutesAfter(day.bedtime_minutes, bedtime_ref) + ? formatMinutesAfter(offsetFromRef, bedtime_ref) : 'N/A', }; }).reverse(); diff --git a/frontend/src/components/stats/SleepTimetable.tsx b/frontend/src/components/stats/SleepTimetable.tsx index 8a02ea3..3d202a0 100644 --- a/frontend/src/components/stats/SleepTimetable.tsx +++ b/frontend/src/components/stats/SleepTimetable.tsx @@ -62,69 +62,70 @@ const Timetable: React.FC = ({ from_date, to_date, day_start, da }; const currentMins = new Date().getHours() * 60 + new Date().getMinutes(); + const rowHeightPx = 32; // h-8 is 2rem, which is 32px in standard Tailwind config - return ( -
-

Sleep Timetable

-
- {/* Header Row */} -
-
{/* Empty corner cell */} - {days.map((day) => ( -
- {getDayName(day)} -
- ))} -
+ return ( +
+

Sleep Timetable

+
+ {/* Header Row */} +
+
{/* Empty corner cell */} + {days.map((day) => ( +
+ {getDayName(day)} +
+ ))} +
- {/* Time Grid Container */} -
-
- {/* Time Rows */} - {timeSteps.map((step) => ( - - {/* Time Label */} -
- {step % 60 === 0 ? formatTimeLabel(step) : ''} -
- - {/* Grid Cells */} - {days.map((day) => { - const sleeping = isSleepingInInterval(day, step, step + 60); - return ( -
- ); - })} - - ))} + {/* Time Grid Container */} +
+
+ {/* Time Rows */} + {timeSteps.map((step) => ( + + {/* Time Label */} +
+ {step % 60 === 0 ? formatTimeLabel(step) : ''} +
+ + {/* Grid Cells */} + {days.map((day) => { + const sleeping = isSleepingInInterval(day, step, step + 60); + return ( +
+ ); + })} + + ))} - {/* Red Marker Line */} - -
-
-
-
-
- ); + {/* Red Marker Line */} +
+
+
+
+
+ ); }; export default Timetable; + diff --git a/frontend/src/components/stats/WakeupPlot.tsx b/frontend/src/components/stats/WakeupPlot.tsx index 46a98e6..c940ac3 100644 --- a/frontend/src/components/stats/WakeupPlot.tsx +++ b/frontend/src/components/stats/WakeupPlot.tsx @@ -1,6 +1,6 @@ import { useEffect, useState } from 'react'; import { useStats } from '../../hooks/useStats'; -import { formatMinutesAfter, getToday } from '../../utils/time'; +import { formatMinutesAfter, getToday, getLocalOffsetFromReference } from '../../utils/time'; import { LineChart, Line, @@ -32,12 +32,13 @@ const WakeupPlot: React.FC = ({ from_date, to_date, wakeup_r const daysAgo = Math.floor( (new Date(getToday()).getTime() - new Date(day.date).getTime()) / (1000 * 60 * 60 * 24) ); + const offsetFromRef = day.wakeup_minutes !== null ? getLocalOffsetFromReference(day.wakeup_minutes, wakeup_ref) : 0; return { date: daysAgo === 0 ? 'Today' : `${daysAgo}d ago`, - minutes: day.wakeup_minutes ?? 0, + minutes: offsetFromRef, time: day.wakeup_minutes !== null - ? formatMinutesAfter(day.wakeup_minutes, wakeup_ref) + ? formatMinutesAfter(offsetFromRef, wakeup_ref) : 'N/A', }; }).reverse(); @@ -70,11 +71,11 @@ const WakeupPlot: React.FC = ({ from_date, to_date, wakeup_r domain={['dataMin - 30', 'dataMax + 30']} /> [ - formatMinutesAfter(value ?? 0, wakeup_ref), - 'Wakeup', - ] as any} - labelFormatter={(label) => label} + formatter={(value: any) => [ + formatMinutesAfter(value ?? 0, wakeup_ref), + 'Wakeup', + ] as any} + labelFormatter={(label) => label} /> { @@ -103,4 +104,12 @@ describe('getToday', () => { const today = new Date().toISOString().split('T')[0]; expect(result).toBe(today); }); -}); \ No newline at end of file +}); + +describe('timeToMinutes', () => { + it('converts HH:MM to minutes', () => { + expect(timeToMinutes('07:30')).toBe(450); + expect(timeToMinutes('00:00')).toBe(0); + expect(timeToMinutes('23:59')).toBe(1439); + }); +}); diff --git a/frontend_improvements.md b/frontend_improvements.md new file mode 100644 index 0000000..794edab --- /dev/null +++ b/frontend_improvements.md @@ -0,0 +1,262 @@ +# 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 }> +``` + +--- + +## 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 | diff --git a/test_timezone.js b/test_timezone.js new file mode 100644 index 0000000..1e9ea2d --- /dev/null +++ b/test_timezone.js @@ -0,0 +1,33 @@ +const adjustForTimezone = (utcMinutes) => { + const offsetInMinutes = -120; // Simulate UTC+2 + return utcMinutes - offsetInMinutes; +}; + +const formatMinutesAfter = (minutes, reference = "04:00") => { + const [refHours, refMinutes] = reference.split(":").map(Number); + const totalRefMinutes = refHours * 60 + refMinutes; + const targetMinutes = totalRefMinutes + minutes; + const normalizedMinutes = ((targetMinutes % 1440) + 1440) % 1440; + const hours = Math.floor(normalizedMinutes / 60); + const mins = normalizedMinutes % 60; + return `${String(hours).padStart(2, "0")}:${String(mins).padStart(2, "0")}`; +}; + +// Case 1: Backend sends offset from 04:00 UTC (-5 minutes) +const backendOffset = -5; +const adjusted = adjustForTimezone(backendOffset); +console.log("Adjusted offset:", adjusted); // Should be 115 +console.log("Formatted time:", formatMinutesAfter(adjusted, "04:00")); // Should be 05:55 + +// Case 2: Backend sends minutes from UTC midnight (235 minutes) +const backendMidnightMins = 235; +const adjusted2 = adjustForTimezone(backendMidnightMins); +console.log("Adjusted midnight mins:", adjusted2); // Should be 355 +// If we use it as an offset from reference: +console.log("Formatted time (as offset):", formatMinutesAfter(adjusted2, "04:00")); // Should be 09:55 (WRONG if it was supposed to be 05:55) + +// If we want the offset from reference for Case 2: +const refMins = 4 * 60; +const correctOffset = adjusted2 - refMins; // 355 - 240 = 115 +console.log("Corrected offset (from midnight):", correctOffset); // 115 +console.log("Formatted time (corrected):", formatMinutesAfter(correctOffset, "04:00")); // 05:55 diff --git a/wakeup_times_plan.md b/wakeup_times_plan.md new file mode 100644 index 0000000..7c7de2c --- /dev/null +++ b/wakeup_times_plan.md @@ -0,0 +1,31 @@ +# Plan: Fix Timezone Handling in Wakeup Times Module + +## Problem Description +The "Wakeup Times" chart (in `frontend/src/components/stats/WakeupPlot.tsx`) displays wakeup times incorrectly due to a timezone mismatch. +- **Observation**: If a baby wakes up at `05:55` local time (e.g., in UTC+2), the chart shows `03:55`. +- **Cause**: The backend stores times in UTC. The frontend currently receives minutes relative to UTC midnight and renders them directly using `formatMinutesAfter` without applying the client's local timezone offset. + +## Investigation Steps +1. **Backend Verification**: + - Examine `backend/app/routers/stats.py` (or equivalent) and the CRUD logic to confirm that `wakeup_minutes` is calculated based on UTC midnight. + - Verify how the backend handles the `wakeup_ref` and `bedtime_ref` parameters. +2. **Frontend Logic Audit**: + - Trace the data flow from `fetchWakeSleepTimes` -> `useStats` -> `WakeupPlot.tsx`. + - Confirm that `wakeup_minutes` is indeed a raw offset from UTC midnight. + +## Implementation Plan +1. **Calculate Local Offset**: Use `new Date().getTimezoneOffset()` in the frontend to determine the difference between UTC and local time. +2. **Adjust Minutes**: + - In `WakeupPlot.tsx`, modify the mapping logic to adjust `day.wakeup_minutes` by the local timezone offset before it is passed to `formatMinutesAfter`. + - Formula: `local_minutes = utc_minutes - (timezone_offset_in_minutes)`. + *(Note: `getTimezoneOffset()` returns minutes positive for west of UTC, negative for east, so subtraction is correct to add offset to UTC).* +3. **Refactor Utilities (if needed)**: If the logic is used elsewhere, consider moving timezone-aware formatting to `frontend/src/utils/time.ts`. + +## Verification Plan +1. **Manual Verification**: + - Mock a response from the backend with a known UTC time. + - Check if the rendered string matches the expected local time. +2. **Automated Testing**: + - Add/update a test case in `frontend/tests/e2e/stats.spec.ts` (or a unit test in `frontend/tests/`) that simulates different timezone offsets to ensure the display is correct. +3. **Regression Check**: + - Ensure `BedtimePlot.tsx` and other time-related components are not negatively impacted by changes to shared utilities.