From 5a7168d91051b37233f478680ba972fd2b2af676 Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Thu, 25 Jun 2026 19:50:41 +0200 Subject: [PATCH] updated pre push hooks + fmt --- audit.md | 151 --------------------------------------- backend-rs/src/config.rs | 3 +- scripts/pre-push.sh | 14 +++- 3 files changed, 12 insertions(+), 156 deletions(-) delete mode 100644 audit.md diff --git a/audit.md b/audit.md deleted file mode 100644 index e0c121a..0000000 --- a/audit.md +++ /dev/null @@ -1,151 +0,0 @@ -# Holy Flavor Rater — Audit - -Scope: `backend-rs/` (Rust/Axum/SeaORM) + `frontend/` (React 19/MUI 7). Django backend excluded (legacy data dir). Findings ordered by impact within each area. - -## Robustness - -### R1. Notification text is hardcoded English (DE locale shows mixed languages) -`App.tsx:183-189` and `NavSidebar.tsx:445-451` hardcode `replied to your review on`, `mentioned you on`, `left a message on your guestbook` as template literals while sibling notif types use `t(...)`. German users get a mixed-language menu. -**Fix:** move to i18n keys with interpolation, e.g. `t('community.notifReply', { flavor: n.flavor_name })`. - -### R2. Full page reload on notification click -`App.tsx:104` and `NavSidebar.tsx:83` use `window.location.href = '/flavor/${id}'` instead of `navigate()`. Kills SPA state, refetches everything, white flash. Other branches use `navigate()` — inconsistent. -**Fix:** `navigate(path)`; if same-route remount is needed, reset via key/scroll. - -### R3. Rate limiter fails open and is per-process -`throttle.rs` is in-memory: login/signup limits reset on restart and don't span instances. Acceptable for the single Rust process today. If ever scaled to 2 instances or frequent deploys, the brute-force window resets each time. -**Fix (future):** DB/Redis-backed counters, or rely on strict nginx limits in front. - -### R4. `refetchUser` 60s poll also re-pulls the full following list -`AuthContext.tsx:48` — every minute it re-fetches `following_list` just to refresh the unread badge, which is already on `/users/me/`. -**Fix:** poll only `users/me/`; fetch following on login + after follow/unfollow mutations. - -## Performance - -### P1. (Biggest) List endpoints ship full nested ratings the UI never reads -`build_flavors` (`service.rs:230`) always populates `FlavorOut.ratings` with full `RatingOut` objects (comment, avatar URL, category, score…). `/flavors/` page size = 50 (`pagination.rs:9`). `FlavorCard` (`FlavorCard.tsx:10-18`) consumes only id/name/image/avg/flags/category — never `ratings`. Category + main list pages thus transfer 50 flavors × all their ratings as dead JSON. -- Detail view needs ratings. `top` needs them (`MainPage.tsx:98,363`). `list`/`newest`/`category` do **not**. -**Fix:** add `include_ratings: bool` (mirrors existing `include_replies`); skip per-rating `RatingOut` build for list/newest/category while keeping the cheap aggregate pass (avg/distribution). For `top`'s review-count + featured review, keep ratings on `top` only or add slim `review_count`/`featured_comment` fields. - -### P2. Double average computation in `list` -`flavors.rs:97` orders by `AVG(score)` in SQL, then `build_flavors` recomputes avg in Rust. Ordering forces the SQL avg; the win is the P1 payload trim (ratings get fully loaded regardless today). - -## Code quality - -### Q1. Notification menu duplicated -Full render + `handleNotificationClick` logic is copy-pasted in `App.tsx:94-205` and `NavSidebar.tsx`. Two places to fix every notif bug (already bit us: R1 + R2 live in both). -**Fix:** extract `` + `useNotificationClick()`. - -### Q2. Categories fetched raw, outside react-query -`App.tsx:75-83` uses raw `api.get('categories/')` in `useEffect` while everything else uses query hooks. No cache, refetch on every mount, manual array/results juggling. -**Fix:** `useCategories()` query hook. - -### Q3. Stale docs / comment mismatch -`api.ts:4-7` correctly describes httpOnly-cookie auth, but CLAUDE.md frontend Auth still describes the retired localStorage `access`/`refresh` scheme. -**Fix:** update CLAUDE.md frontend Auth paragraph. - -### Q4. `as object` cast hack -`AuthContext.tsx:40-42` does `...({ skipAuthRedirect: true } as object)`; the field is already typed in `RetriableConfig` (`api.ts:24`). -**Fix:** export that type, drop the cast. - -## Extendability - -### E1. Hand-rolled per-field validation -`ratings.rs:107-178` (and update `:258-321`) manually build DRF-shaped field errors. Every new field/endpoint repeats the pattern. -**Fix:** small validation helper (required/int-range/string) returning the field-keyed map. - -### E2. App-level CASCADE hand-coded per handler -`ratings.rs:352-373` collects replies→notifications→replies→rating in a txn. Correct but error-prone as relations grow. -**Fix (long-term):** migration adding `ON DELETE CASCADE` FKs; short-term centralize collectors in helpers. - -## UX flow - -### U1. No optimistic UI on rating/reply submit -Verify mutations `invalidateQueries`; consider optimistic insert so reviews appear instantly. - -### U2. Login redirect is a hard reload -`Login.tsx:35` + `Settings.tsx:185` use `window.location.href='/'`, losing warm react-query cache. -**Fix:** `navigate('/')` + `refetchUser()`. - -### U3. Mark-all-read fails silently -`NotificationContext.tsx:44-51` swallows errors; badge clears then reappears on next poll if the request failed. -**Fix:** toast on failure (ToastContext exists). - ---- - -## Implementation status -- [x] P2 — investigated, **no code change** (no measurable win post-P1). - `avg_score_expr()` is used only in `ORDER BY` (`flavors.rs:97/145/199`), never - selected as a column — it's *required* for sorting+paginating by rating and - can't be dropped. The Rust avg is computed inside the single rating loop that - also builds distribution/followed-avg/viewer-score (`service.rs:341-400`), - which runs regardless; the avg is `sum/n` accumulated in that same loop, so - removing it saves one division per flavor (≤50/page) — negligible. The real - cost (loading all ratings for `list`) is intentional: `FlavorList.tsx:144-171` - reads `flavor.ratings` for the comment count + previews, so `list` keeps - `include_ratings=true`. Slimming that is a P1-class payload change needing a - frontend refactor, out of P2 scope. Closed as wontfix. -- [x] Q4 follow-up — `useCurrentUserLite.ts` had the same `...({ skipAuthRedirect: - true } as object)` cast as the (now-fixed) `AuthContext`; switched it to - `{ skipAuthRedirect: true } as RetriableConfig` using the type exported in Q4. - `tsc` + ESLint (0 errors) clean. -- [x] U1 — optimistic UI on rating + reply submit. `useCreateRating` / - `useCreateReply` (`useRatingMutations.ts`) now run `onMutate`/`onError`/`onSettled` - against the `flavorDetail` cache: insert a temp row (negative id, replaced on - refetch), roll back on error from the snapshot, and invalidate on settle. Author - info is passed through the input — `RatingForm` supplies `optimistic` from - `useAuth().user`; `FlavorDetail.handleReplySubmit` supplies `flavorId` + - `optimisticUser`. Both optimistic paths are opt-in (skip if author info absent), - so other callers are unaffected. New review/reply appears instantly, reconciles - on the round-trip. `tsc`, ESLint (0 errors), 27 tests clean. -- [x] Q3 — fixed stale CLAUDE.md frontend Auth paragraph. It described the - retired localStorage `access`/`refresh` JWT scheme; rewritten to match - `lib/api.ts`: httpOnly `access_token`/`refresh_token` cookies, `withCredentials`, - 401 auto-refresh via `auth/token/refresh/`, `skipAuthRedirect`/`RetriableConfig` - escape hatch, and `clearLegacyToken()` purging all three retired localStorage keys. -- [x] Q4 — dropped the `as object` cast. Exported `RetriableConfig` from - `lib/api.ts`; `AuthContext.refetchUser` now passes `{ skipAuthRedirect: true } - as RetriableConfig` (the field is part of that type) instead of the - `...({ ... } as object)` spread hack. `tsc` + ESLint (0 errors) clean. -- [x] U3 — mark-all-read no longer fails silently. `NotificationContext.markAllRead` - catch block now fires an error toast (`community.markAllReadFailed`, EN+DE) via - `useToast().notify` instead of swallowing. Root cause was also a provider-order - bug: `ToastProvider` sat *inside* `NotificationProvider`, so the context couldn't - reach `useToast` — hoisted `ToastProvider` above `AuthProvider`/`NotificationProvider` - in `app/providers.tsx` (it only needs the MUI theme, which stays outermost). - `tsc` + ESLint (0 errors) clean. -- [x] U2 — killed the hard `window.location.href='/'` reloads. `Login.tsx` - (post-login) now `await refetchUser()` + `navigate('/')`; `Settings.tsx` - (post-account-deletion) `queryClient.clear()` + `await refetchUser()` + - `navigate('/')` — deletion wipes the whole react-query cache so no stale user - data survives the now-gone account. Both keep the SPA mounted (warm cache, no - white flash). `tsc` + ESLint (0 errors) clean. -- [x] R4 — split `refetchUser` / `refetchFollowing` (`AuthContext.tsx`). The 60s - badge poll (`NotificationContext.tsx:34`) now only hits `users/me/`; the - following list is fetched once on login (effect keyed on `user?.username`) and - cleared on logout. Added `refetchFollowing` to the context value and call it - after follow/unfollow in `PublicProfile.tsx` (both the main + mini toggles) so - the sidebar list updates immediately instead of waiting for the next poll. - Drops one `following_list` GET per minute per logged-in client. `tsc` + ESLint - (0 errors) clean. -- [x] Q2 — `useCategories()` query hook (`api/queries/useCategories.ts`, key - `queryKeys.categories()`, 30 min `staleTime`). Replaced raw - `api.get('categories/')` + `useState`/`useEffect` in `App.tsx` with the hook; - dropped the now-unused `api` import. Cached + shared across mounts. `tsc` + - ESLint (0 errors) clean. -- [x] R1 + R2 + Q1 — notif menu dedup + i18n + SPA nav. Extracted shared - `` (`components/layout/NotificationMenu.tsx`) + `useNotificationClick()` - hook (`hooks/useNotificationClick.ts`); `App.tsx` and `NavSidebar.tsx` now both render - the one component. R1: hardcoded English notif strings moved to i18n keys - (`community.notifReplyOn`/`notifMentionOn`/`notifProfileComment`/`markAllRead`, EN+DE, - flavor interpolated). R2: `window.location.href` → `navigate()` (no full reload). `tsc`, - ESLint (0 errors), 27 tests, `vite build` all clean. -- [x] P1 — list payload trim (`include_ratings` flag). `build_flavors` now takes - `include_ratings`; the aggregate rollup (avg/followed-avg/distribution/viewer - score) always runs, but the per-rating `RatingOut` vec + rating-author user - lookup are skipped when false. Set false on `newest`, `followed_top`, and the - dashboard's "missing flavors" (loads *every* unrated flavor — biggest win); - kept true on `list`/`top`/detail because their UI reads the nested reviews. No - frontend change required. `cargo fmt`/`clippy`/`test` clean (20 passed). - - diff --git a/backend-rs/src/config.rs b/backend-rs/src/config.rs index d1b42ae..9b30303 100644 --- a/backend-rs/src/config.rs +++ b/backend-rs/src/config.rs @@ -120,8 +120,7 @@ impl Config { .or_else(|| env::var("EMAIL_HOST_USER").ok().filter(|s| !s.is_empty())) .unwrap_or_else(|| "noreply@localhost".to_string()), }, - banners_dir: env::var("BANNERS_DIR") - .unwrap_or_else(|_| "../data/banners".to_string()), + banners_dir: env::var("BANNERS_DIR").unwrap_or_else(|_| "../data/banners".to_string()), legacy_dir: env::var("LEGACY_DIR").unwrap_or_else(|_| "../legacy".to_string()), backup_dir: env::var("BACKUP_DIR").unwrap_or_else(|_| "../data/backups".to_string()), // Default OFF so ad-hoc `cargo run` instances don't run jobs. diff --git a/scripts/pre-push.sh b/scripts/pre-push.sh index e7cc649..d787aa0 100755 --- a/scripts/pre-push.sh +++ b/scripts/pre-push.sh @@ -7,10 +7,18 @@ echo "========================================" echo " Running Pre-Push Integrity Checks " echo "========================================" -# 1. Frontend Build +# 1. Frontend Formatting & Build echo "" -echo "Step 1: Checking Frontend Build..." +echo "Step 1: Formatting Frontend..." cd "$ROOT_DIR/frontend" +if ! npm run format; then + echo "" + echo "❌ ERROR: Frontend formatting failed." + exit 1 +fi + +echo "" +echo "Step 2: Checking Frontend Build..." if ! npm run build; then echo "" echo "❌ ERROR: Frontend build failed." @@ -20,7 +28,7 @@ fi # 2. Rust backend checks echo "" -echo "Step 2: Checking Rust backend..." +echo "Step 3: Checking Rust backend..." cd "$ROOT_DIR/backend-rs" if ! cargo fmt --check; then