|
| 1 | +# Simplification Suggestions |
| 2 | +--- |
| 3 | + |
| 4 | +## Tier 6 — Architecture / code design |
| 5 | + |
| 6 | +### 20. Split `course-add-modal.tsx` — it's two features in one file (~ 0 net lines, real seam) |
| 7 | + |
| 8 | +**File:** `features/catalog/components/course-add-modal.tsx` (535 lines — largest in features/) |
| 9 | + |
| 10 | +The file contains two unrelated UI surfaces that happen to share a directory: |
| 11 | + |
| 12 | +| Surface | Components | Used from | |
| 13 | +|---|---|---| |
| 14 | +| Fulfilling-courses **modal** | `CourseAddModal`, `FulfillingCoursesContent` | opened via `openModal()` from requirement rows | |
| 15 | +| Course-search **side panel** | `CourseSearchPanel`, `CourseSearchContent`, `CourseSearchResults`, `DivisionToggle` | embedded directly in both pages' side rails | |
| 16 | + |
| 17 | +They share nothing except `CourseCard` and the mappers. Move the search panel to |
| 18 | +`features/catalog/components/course-search-panel.tsx`. Bonus: after items 17/18 the search |
| 19 | +panel file has no audit-context import at all, and the modal file's dependencies shrink to |
| 20 | +`useCourseModalContext` + `useAuditContext.addPlannedCourse`. |
| 21 | + |
| 22 | +### 21. Decide the composite-audit question explicitly (biggest conceptual simplifier) |
| 23 | + |
| 24 | +**File:** `features/audit/audit-provider.tsx` lines 95–112 |
| 25 | + |
| 26 | +Today the provider wraps the single current audit into a one-element `CompositeAuditData`, |
| 27 | +runs it through `getCompositeAuditRequirements` (which computes `auditName` and |
| 28 | +`duplicateCourseCodes` per requirement), and every view consumes the result — but **no view |
| 29 | +reads the composite-only fields**, and `lib/storage/composite-storage.ts` has no app |
| 30 | +consumers yet. Two honest options: |
| 31 | + |
| 32 | +- **A. Multi-audit view ships soon** → keep the pipeline exactly as-is. The indirection is |
| 33 | + the feature's landing zone; removing it now means re-adding it next month. |
| 34 | +- **B. Multi-audit view is not imminent** → have the provider derive `sections` directly |
| 35 | + from `auditData.requirements` and delete the wrapper `useMemo`s. The tested functions in |
| 36 | + `lib/audit-calculations.ts` stay for when the feature lands. |
| 37 | + |
| 38 | +Either is fine; what's costing you is the *undecided middle* where every reader of the |
| 39 | +provider has to understand composite plumbing that does nothing yet. Pick A or B and note |
| 40 | +it in the code. |
| 41 | + |
| 42 | + |
| 43 | + |
| 44 | + |
| 45 | + |
| 46 | + |
| 47 | + |
| 48 | + |
| 49 | + |
| 50 | + |
| 51 | +### 34. Rejected — and why (so these don't get re-litigated) |
| 52 | + |
| 53 | +- **Zustand / Redux / Jotai:** context + derived `useMemo` fits this app. State lives in |
| 54 | + one provider; item 17 fixes the only real re-render problem. A store adds concepts, not |
| 55 | + capability. |
| 56 | +- **TanStack Query:** its value is caching/refetching/invalidating server state. The async |
| 57 | + sources here are extension storage and IndexedDB — one-shot local reads. Nothing to cache. |
| 58 | +- **Zod (runtime validation):** validation already exists where data actually crosses a |
| 59 | + trust boundary (`validate-catalog.ts` for scraped data, error paths in the scraper). |
| 60 | + Schema-defining every internal type would duplicate the TS types for data this codebase |
| 61 | + itself wrote. |
| 62 | +- **React Router:** two views behind a toggle plus one query param. A router adds routes, |
| 63 | + layouts, and navigation concepts to avoid one `viewMode === "audit"` ternary. |
| 64 | +- **date-fns / dayjs:** the only date logic is semester math (`getCurrentSemester`, |
| 65 | + `nextSemester`) — domain-specific, ~20 lines, no date library helps. |
| 66 | +- **shadcn/Radix wholesale:** the `components/ui/` set matches the Figma design and is |
| 67 | + small. One targeted exception worth knowing: if/when the modal and the audit-card menu |
| 68 | + get proper a11y (Escape-to-close, focus trap, outside-click dismiss — none exist today), |
| 69 | + reach for `@radix-ui/react-dialog` / `react-dropdown-menu` rather than hand-rolling focus |
| 70 | + management. Until then, no. |
| 71 | +- **framer-motion is on watch, not rejected:** it's a heavy dependency with exactly one |
| 72 | + consumer (the graph tooltip fade/slide in `graph.tsx`), and CSS transitions could do that |
| 73 | + job. Not urgent — but if bundle size ever matters or a second animation need *doesn't* |
| 74 | + appear, replace and drop it. |
| 75 | + |
| 76 | +--- |
| 77 | + |
| 78 | +## Tier 9 — Second-pass findings (storage layer, popup architecture, leftovers) |
| 79 | + |
| 80 | +From a follow-up review after the first cleanup rounds landed. The provider memoization, |
| 81 | +`currentAuditName`, and WXT typed preferences all check out — `updateLastAuditId` is |
| 82 | +properly `useCallback`'d, so the audit-load effect that depends on it stays stable. |
| 83 | + |
| 84 | +### 35. The popup content script runs on every website the user visits (biggest item here) |
| 85 | +◊ |
| 86 | +**File:** `entrypoints/popup-ui.content.tsx` (`matches: ["<all_urls>"]`) |
| 87 | + |
| 88 | +What this costs today, on **every page the user opens anywhere on the web**: |
| 89 | + |
| 90 | +- `loadFonts()` runs immediately in `main()` — it appends three Google Fonts `<link>` tags |
| 91 | + into the *host page's* `<head>` before the popup is ever opened. That fires requests to |
| 92 | + Google from every site the user visits and can change fonts on pages that use the same |
| 93 | + families. |
| 94 | +- Two `console.log`s pollute every page's console. |
| 95 | +- The manifest needs `<all_urls>` host access → the scary "read and change all your data on |
| 96 | + all websites" install warning and slower store review. |
| 97 | +- On pages where content scripts can't run (chrome://, Web Store, PDFs), clicking the |
| 98 | + toolbar icon does nothing — `sendTabMessage(...).catch(() => {})` in |
| 99 | + `background-controller.ts` swallows the failure silently. |
| 100 | + |
| 101 | +Two options, honestly weighed: |
| 102 | + |
| 103 | +- **A (recommended): make it a standard action popup.** `popup-app.tsx` is already designed |
| 104 | + as a fixed 438px card — it *is* a toolbar popup wearing an overlay costume. Point the |
| 105 | + manifest `action.default_popup` at a `popup.html` entrypoint rendering `<App />`. Then |
| 106 | + delete: the whole content script, the `TOGGLE_POPUP` message + its background listener, |
| 107 | + the font/head injection, and the `<all_urls>` requirement. UX change: the popup anchors |
| 108 | + to the toolbar icon instead of overlaying the page — for this UI that's arguably more |
| 109 | + native, but it is a visible change, so it's your call. |
| 110 | +- **B (minimum fix, no UX change):** move `loadFonts()` inside `showPopup()` so pages the |
| 111 | + user never opens the popup on are left untouched, and delete the `console.log`s. The |
| 112 | + `<all_urls>` permission cost remains. |
| 113 | + |
| 114 | +### 36. Audit history is the next `storage.defineItem` candidate — three hand-rolled consumers |
| 115 | + |
| 116 | +Item 33 said "adopt WXT storage opportunistically." The opportunity is now concrete: |
| 117 | +**three places** independently hand-roll reads/listeners for the same `auditHistory` key |
| 118 | +that `lib/storage/audit-storage.ts` owns privately: |
| 119 | + |
| 120 | +- `features/popup/popup-app.tsx` — `browser.storage.onChanged` + `changes.auditHistory` |
| 121 | +- `features/banner/try-dap-banner.tsx` — same listener pattern, plus ad-hoc inline types |
| 122 | + (`{ audits?: { auditId?: string }[] }`) re-describing `AuditHistoryData` |
| 123 | +- `features/audit/audit-provider.tsx` — one-shot read (goes stale after a background |
| 124 | + re-scrape until reload) |
| 125 | + |
| 126 | +Defining `auditHistoryItem = storage.defineItem<AuditHistoryData>("local:auditHistory")` |
| 127 | +inside audit-storage (keeping the same key, so **no data migration**) collapses all three: |
| 128 | +consumers call `auditHistoryItem.watch(...)` and the stringly `changes.auditHistory` |
| 129 | +lookups and duplicate inline types disappear. This is the same pattern |
| 130 | +preferences-provider already uses, applied to the key with the most consumers. |
| 131 | + |
| 132 | +### 37. `getUncachedAuditIds` does N storage reads for an existence check |
| 133 | + |
| 134 | +**File:** `lib/storage/audit-storage.ts` lines 59–64 |
| 135 | + |
| 136 | +It `Promise.all`s a full `getAuditData` per id — deserializing every cached audit (which |
| 137 | +are large objects) just to test presence. `browser.storage.local.get` accepts an array: |
| 138 | + |
| 139 | +```ts |
| 140 | +const keys = auditIds.map((id) => `${AUDIT_DATA_PREFIX}${id}`); |
| 141 | +const result = await browser.storage.local.get(keys); |
| 142 | +return auditIds.filter((id) => !(`${AUDIT_DATA_PREFIX}${id}` in result)); |
| 143 | +``` |
| 144 | + |
| 145 | +One round trip instead of N. Mechanical, and `background-controller.test.ts` doesn't touch |
| 146 | +this function, so no test churn. |
| 147 | + |
| 148 | +### 38. Test data is living in `lib/` — move it to `tests/` |
| 149 | + |
| 150 | +`lib/examples/data/ut-degree-programs.ts` (423 lines, the single biggest file under `lib/`) |
| 151 | +has exactly one consumer: `tests/validate-parse-major.ts`. It's expected-output fixture |
| 152 | +data, not library code. Move it to `tests/fixtures/` (next to |
| 153 | +`ut-direct-degree-plan-cases.ts`'s eventual home) and delete the now-empty `lib/examples/`. |
| 154 | +Rule this encodes: `lib/` contains only code the app imports. |
| 155 | + |
| 156 | +### 39. Dead and no-op helpers in `lib/utils.ts` |
| 157 | + |
| 158 | +Verified against all call sites: |
| 159 | + |
| 160 | +- `formatMajorLabel` **is the identity function** (`return major;`) — called in 3 places |
| 161 | + (provider, navbar ×2) for zero effect. Either it's a placeholder for real formatting |
| 162 | + (then add the TODO saying what it should do) or delete it and unwrap the call sites. |
| 163 | +- `getColorBySectionTitle` and `getColorByIndex` have **zero consumers** — delete both. |
| 164 | + (`getColorByCourseCode` is used; keep.) |
| 165 | + |
| 166 | +### 40. Gate debug logging, keep error logging |
| 167 | + |
| 168 | +8 `console.log`s ship in production code paths (scraper-window ×3, popup content script, |
| 169 | +background batch summary, etc.). The zero-dependency fix: wrap them in WXT's built-in flag — |
| 170 | +`if (import.meta.env.DEV) console.log(...)` — or just delete the ones that only narrate |
| 171 | +("Toggle message received"). Keep every `console.error`/`console.warn`; those are doing |
| 172 | +real work. Don't build a logger abstraction for 8 call sites. |
| 173 | + |
| 174 | +### 41. Left alone on purpose (so the next reviewer doesn't re-flag them) |
| 175 | + |
| 176 | +- **`scraper-window.ts`** — reviewed closely; the listener-before-create ordering, timeout |
| 177 | + cleanup, and `activeScraperTabs` bookkeeping are all correct. Its complexity is inherent |
| 178 | + to tab lifecycle management, not accidental. Don't simplify it. |
| 179 | +- **`parse-major.ts`** — 176 lines of UT-specific regex special cases looks scary but is |
| 180 | + data-driven, table-shaped, and covered by `validate-parse-major` against 400+ real degree |
| 181 | + programs. The ugliness lives in UT's data, not this code. |
| 182 | +- **Preferences in `sync:` storage** — fine (theme/view prefs syncing across a student's |
| 183 | + machines is a feature; the provider's fallback already handles a synced `lastAuditId` |
| 184 | + pointing at an audit not cached locally). One caveat to know: keys moved from `local:` to |
| 185 | + `sync:`, so existing users' saved preferences reset once. Acceptable for defaults this |
| 186 | + cheap; not worth a migration. |
| 187 | + |
| 188 | +--- |
| 189 | + |
| 190 | +## Noticed while reviewing — pre-existing issues, NOT cleanup (decide separately) |
| 191 | + |
| 192 | +These change behavior, so they're out of scope for this pass, but they were easy to spot: |
| 193 | + |
| 194 | +1. **Donut tooltip mislabels units** — `degree-completion-donut.tsx` line 110 always says |
| 195 | + "courses completed" but the numbers are hours for hour-based sections. |
| 196 | +2. **Requirement dropdown overwrites Department** (see item 3, last bullet). |
| 197 | +3. **Dead placeholder UI** — `sidebar.tsx`: all four Resources links, the Feedback link, and |
| 198 | + the social/footer icons are `href="#"`; the footer Moon icon does nothing while the real |
| 199 | + dark-mode toggle lives in the navbar; the `+` button next to "MY AUDITS" has no handler. |
| 200 | + `audit-card.tsx`: "Duplicate" and "Delete Audit" menu items have no handlers. |
| 201 | +4. **`CreditHourTotalsCard` renders hardcoded data** — `degree-audit-page.tsx` lines 58–71 |
| 202 | + pass literal 21/36-hour requirements with hardcoded `met` flags instead of audit data. |
| 203 | +5. **`lib/storage/composite-storage.ts` has no app consumers** (only `tests/validate-*.ts` |
| 204 | + scripts). Presumably staged for the composite-audit feature — fine to keep, just noting |
| 205 | + it is not wired into the UI yet (see item 21). |
| 206 | +6. **Back button doesn't switch audits** — `setCurrentAuditId` does |
| 207 | + `window.history.pushState(..., "?auditId=...")` but nothing listens for `popstate`, so |
| 208 | + pressing Back changes the URL without changing the displayed audit. Either handle |
| 209 | + `popstate` or use `replaceState` so Back leaves the page entirely. |
| 210 | + |
| 211 | +--- |
| 212 | + |
| 213 | +## Suggested order |
| 214 | + |
| 215 | +1. **Tier 7 first** (items 25–28): CI test job, lockfile/dep cleanup, lint rules. These are |
| 216 | + config-only, take minutes, and the lint rules then police everything that follows. |
| 217 | +2. Item 17 (provider memoization) alone in its own commit — mechanical but touches all |
| 218 | + three providers, so keep it isolated and easy to revert. |
| 219 | +3. Items 18 + 20 together (modal reads context, then the file split falls out naturally). |
| 220 | +4. Items 29–30 (icon/`cn` standardization) — safe find-replaces, any time. |
| 221 | +5. Items 21–24 as individual decisions; 21 first, since A/B changes what the provider |
| 222 | + looks like for everything after. |
| 223 | +6. Items 31–33 opportunistically, as the files they touch come up for other reasons. |
| 224 | +7. **Tier 9:** items 37–40 are quick, isolated wins (do any time). Item 36 (auditHistory |
| 225 | + `defineItem`) is one focused commit that simplifies three files. Item 35 is the one that |
| 226 | + needs a product decision (A vs B) — decide it deliberately, don't let it linger, because |
| 227 | + option A deletes an entire content script and a store-review-hostile permission. |
| 228 | + |
| 229 | +Expected net effect: a leaner provider API, an end to the toggle-sidebar → |
| 230 | +re-render-everything cascade, five dependencies and a stray lockfile gone, tests actually |
| 231 | +running in CI, and lint rules that keep dead code from re-accumulating — with zero |
| 232 | +user-visible change (except item 35A, if chosen, which relocates the popup to the toolbar). |
0 commit comments