|
| 1 | +# Frontend refactor plan |
| 2 | + |
| 3 | +Status: Phase 0 shipped (c4b0975 DOM test lane); phase 4 mostly shipped (e1b80da i18n and the dead re-export, 7974eaa api types); phases 1-3 not started |
| 4 | +Saved: 2026-08-27 |
| 5 | + |
| 6 | +## Current priority order |
| 7 | + |
| 8 | +1. **P0 — DONE (c4b0975): a test lane that can reach a hook.** Everything below |
| 9 | + rewrites hooks, and until this landed the client suite could only assert on |
| 10 | + HTML strings. |
| 11 | +2. **P1 — Server state through TanStack Query.** The single root cause of the |
| 12 | + god hooks. Start with a one-domain spike, not a migration. |
| 13 | +3. **P2 — UI state in a real store (Zustand).** Only after P1, so what remains |
| 14 | + in the store is actually UI state. |
| 15 | +4. **P3 — React 19 + React Compiler.** Last, because P1 and P2 delete half the |
| 16 | + memoisation this is meant to remove. |
| 17 | +5. **P4 — Housekeeping.** Two of four items shipped; the rest is file movement |
| 18 | + with no urgency. |
| 19 | + |
| 20 | +## Objective |
| 21 | + |
| 22 | +Cut the hand-written infrastructure the client carries - its own caching, |
| 23 | +deduplication, refetching, optimistic updates and memoisation - by replacing it |
| 24 | +with libraries that already do it. This removes abstraction rather than adding |
| 25 | +it; complexity should go down at every step. |
| 26 | + |
| 27 | +## Delivery order |
| 28 | + |
| 29 | +### 0. Test runtime — DONE (c4b0975) |
| 30 | + |
| 31 | +Prerequisite for everything else, so it went first. |
| 32 | + |
| 33 | +- `*.dom.bun.test.tsx` files run on Bun with happy-dom and |
| 34 | + `@testing-library/react`. Bun was already a required dependency, so no new |
| 35 | + runner was added. |
| 36 | +- The DOM is registered by `scripts/bun-dom-preload.ts`, not by an import inside |
| 37 | + the test: Bun evaluates `node_modules` before local modules, so |
| 38 | + `@testing-library/dom` captures `document.body` and installs throwing stubs |
| 39 | + before a local module can register a document. |
| 40 | +- The preload keys off the file name in `process.argv`, so the server contract |
| 41 | + suites still run without a `window`. A global preload would make code that |
| 42 | + branches on `typeof window` take the browser path inside a server test. |
| 43 | +- `scripts/run-tests.mjs` splits client files into bun/node the way it already |
| 44 | + split server ones, and its bun pattern accepts `.tsx`. |
| 45 | +- First subject: `src/shared/view/ui/ActionMenu.dom.bun.test.tsx`. Five |
| 46 | + behaviours that were previously unreachable, including Escape restoring focus |
| 47 | + to the trigger and an outside click deliberately not doing so. |
| 48 | + |
| 49 | +### 1. Server state through TanStack Query — NOT STARTED |
| 50 | + |
| 51 | +The evidence, measured 2026-08-27: |
| 52 | + |
| 53 | +| Hook | Lines | |
| 54 | +|------|-------| |
| 55 | +| `src/stores/useSessionStore.ts` | 1130 | |
| 56 | +| `src/hooks/useProjectsState.ts` | 1069 (7 `useEffect`) | |
| 57 | +| `src/components/chat/hooks/useChatSessionState.ts` | 986 | |
| 58 | +| `src/components/git-panel/hooks/useGitPanelController.ts` | 817 | |
| 59 | +| `src/components/chat/hooks/useChatRealtimeHandlers.ts` | 373 | |
| 60 | +| **Total** | **4375** | |
| 61 | + |
| 62 | +- `src/utils/api.ts` is 270 lines of raw `fetch`; caching, deduplication, |
| 63 | + refetching and optimistic updates are hand-written in the hooks above. |
| 64 | +- `src/components/app/AppContent.tsx:125` polls running sessions on a |
| 65 | + `setInterval(5000)`. |
| 66 | +- `useProjectsState.ts:508` implements `registerOptimisticSession` by hand. |
| 67 | +- The realtime handler receives a `setSessionState` setter and writes state |
| 68 | + directly, which is what ties `useChatRealtimeHandlers`, `useSessionStore` and |
| 69 | + `useChatSessionState` together. |
| 70 | + |
| 71 | +Target shape: `useQuery`/`useMutation` own server state, and **WebSocket is |
| 72 | +demoted from a state source to a cache-invalidation channel** |
| 73 | +(`queryClient.setQueryData`). |
| 74 | + |
| 75 | +**Start with a spike, not a migration.** Convert `projects` only, then measure: |
| 76 | +how many lines actually disappear, and where the WebSocket handler fights the |
| 77 | +cache. The hard part is not volume, it is the interval during which the same |
| 78 | +data lives in both the Query cache and the old `useState`. That question is |
| 79 | +answered by one day of code, not by more planning. |
| 80 | + |
| 81 | +### 2. UI state in a real store — NOT STARTED |
| 82 | + |
| 83 | +- `useSessionStore.ts` is a store by name only: `useState` + `useRef` over a |
| 84 | + `Map`, so subscriptions cannot be split per field. |
| 85 | +- The result is prop drilling: `AppContent.tsx:67,77` spreads a |
| 86 | + `sidebarSharedProps` bundle wholesale. |
| 87 | +- Zustand with selectors cuts the re-render scope to one field: |
| 88 | + `useStore((s) => s.activeSessionId)`. |
| 89 | +- **Do not delete the contexts wholesale.** There are six -- WebSocket, Auth, |
| 90 | + Theme, Permission, SessionStatus, PaletteOps. The first three are exactly what |
| 91 | + Context is for. `PaletteOpsContext` is the one to remove: it exists only |
| 92 | + because there is no global store to hold that state. |
| 93 | + |
| 94 | +### 3. React 19 + React Compiler — NOT STARTED |
| 95 | + |
| 96 | +- `useChatComposerState.ts` holds 31 `useCallback`s and `useSessionStore.ts` 30. |
| 97 | + That is a person doing a compiler's job. |
| 98 | +- Every React-consuming dependency accepts 19; checked 2026-08-27 against |
| 99 | + `peerDependencies`: react-error-boundary `>=16.13.1`, cmdk `^18 || ^19`, |
| 100 | + lucide-react up to `^19`, react-markdown `>=18`, react-router-dom `>=18`, |
| 101 | + `@uiw/react-codemirror` `>=16.8.0`, react-i18next `>=16.8.0`, react-dropzone, |
| 102 | + react-syntax-highlighter. **Nothing blocks the upgrade.** |
| 103 | +- `@types/react` moves to 19 with it, and the compiler arrives as a Babel plugin |
| 104 | + through `@vitejs/plugin-react`. |
| 105 | + |
| 106 | +### 4. Housekeeping — 2 of 4 done |
| 107 | + |
| 108 | +- **i18n lazy loading — DONE (e1b80da).** `config.js` static-imported all fifty |
| 109 | + translation files. English stays bundled as the fallback; the other nine are |
| 110 | + chunks behind a small i18next backend, one per language. Measured on a real |
| 111 | + build: main bundle 530KB to 271KB, gzip 153KB to 80KB. This also uncovered a |
| 112 | + bug: `languages.js` offered French and `config.js` never registered it, so |
| 113 | + choosing French silently served English. `src/i18n/localeCoverage.test.ts` |
| 114 | + now fails if a language is offered but cannot load. |
| 115 | +- **`api.js` to TypeScript — DONE (7974eaa).** 37 files import it and it was the |
| 116 | + only untyped layer. Typecheck passed without touching a call site. |
| 117 | +- **Dead re-export — DONE (e1b80da).** `src/contexts/AuthContext.jsx` was a |
| 118 | + one-line re-export nothing imported. |
| 119 | +- **Shared buckets — NOT STARTED.** `src/lib/`, `src/utils/` and |
| 120 | + `src/shared/view/` are three homes with no rule about which to use. |
| 121 | +- **`view/subcomponents/` — NOT STARTED.** One meaningless nesting level, in |
| 122 | + four component folders; `chat/view/subcomponents/` alone holds 26 files. |
| 123 | + |
| 124 | +Both remaining items are pure file movement: hundreds of changed import paths, |
| 125 | +a diff that hides real work in review, and a magnet for conflicts with anyone |
| 126 | +else in the tree. They are worth doing on a quiet day, not during a push. |
| 127 | + |
| 128 | +## Explicit exclusions |
| 129 | + |
| 130 | +- **Do not add a second test runner.** Bun is already required; the DOM lane |
| 131 | + rides on it. |
| 132 | +- **Do not replace Context wholesale.** WebSocket, Auth and Theme stay. |
| 133 | +- **Do not hand-write another abstraction.** Every step here replaces |
| 134 | + hand-written infrastructure with a library; if a step adds a new bespoke |
| 135 | + layer, it is the wrong step. |
| 136 | +- **Do not start P1 before P0.** Rewriting 4375 lines of hooks without hook |
| 137 | + tests is not a refactor. |
| 138 | +- **Do not reorder P1 and P2.** Server state has to leave first, or the same |
| 139 | + caching gets hand-written a second time inside the store. |
| 140 | + |
| 141 | +## Known loose ends |
| 142 | + |
| 143 | +- `dompurify` and `rehype-raw` are declared in `package.json` and imported |
| 144 | + nowhere. Raw HTML is not rendered, so nothing needs sanitising. Remove them |
| 145 | + unless a future feature is waiting on them. |
| 146 | +- Four `.jsx` files remain: `src/main.jsx`, `src/contexts/ThemeContext.jsx`, |
| 147 | + `src/hooks/useLocalStorage.jsx`, plus `src/i18n/*.js`. |
| 148 | + |
| 149 | +## Acceptance criteria |
| 150 | + |
| 151 | +- Every step ships with tests that would fail if it were reverted. |
| 152 | +- `npm test`, `npm run typecheck` and `npm run lint` pass at each commit. |
| 153 | +- Bundle size is measured from a real build, not estimated. |
| 154 | +- No step leaves the app in a state where server data lives in two places. |
0 commit comments