Saravana Achu Mac
e72b375557
Record the implementation commit that removes FMP demo-key ambiguity and documents the required API key. Refs: docs/AUDIT_REDESIGN.md item C6. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
112 lines
13 KiB
Markdown
112 lines
13 KiB
Markdown
# Web Redesign — Systematic Audit & Fix Plan
|
|
|
|
Scope: everything introduced by commits `f62c3b1` (UI shell + 6 endpoints) and `938ed86`
|
|
(live data wiring + strategy builder + screener). Reviewed code, design, UX,
|
|
backend integration, mobile parity, security, and tests.
|
|
|
|
Legend: 🔴 critical (broken / data loss / security) · 🟠 high (clearly wrong but
|
|
not catastrophic) · 🟡 medium (UX or polish) · 🟢 low (nice-to-have).
|
|
|
|
Status: ⬜ open · 🟦 in PR · ✅ fixed (commit hash on the right).
|
|
|
|
---
|
|
|
|
## A. Critical — Broken integrations (will 404 / 401 / corrupt data)
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | --------------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| A1 | `CodeStrategyEditor` POSTs to `/api/backtest`. Real endpoint is `/api/backtest/run`. Result: every "Run Backtest" returns 404. | 🔴 | ✅ | bucket A |
|
|
| A2 | `CodeStrategyEditor` payload sends `{symbol, strategyCode, mode}`. Backend `/api/backtest/run` requires `{symbols[], strategyConfig}`. | 🔴 | ✅ | bucket A |
|
|
| A3 | `VisualRuleBuilder` save → `/api/profiles` body uses `{strategyType, visualRules, description}`. `saveTradeProfileForUser` expects `strategy_config` shape. Result: 400 or silently-discarded fields. | 🔴 | ✅ | bucket A |
|
|
| A4 | `RightPanel.NewsFeed` calls `fetch()` with no `Authorization` header. `/api/news` is `requireAuth`. Result: 401 every render. | 🔴 | ✅ | bucket A |
|
|
| A5 | `RightPanel.NewsFeed` reads `import.meta.env.VITE_TRADING_API_URL` directly instead of `tradingRuntime.tradingApiUrl`. Breaks in prod where the runtime resolver is the source of truth. | 🟠 | ✅ | bucket A |
|
|
| A6 | Backend `/api/chart/bars` previously crashed on crypto symbols (`BTC/USD`) because `/v2/stocks` rejects them. Verified in 938ed86: `encodeURIComponent('BTC/USD')` → `BTC%2FUSD` (correct for query string), and the response lookup `cryptoBars[symbol]` uses the un-encoded key (matches Alpaca's response). | 🟠 | ✅ | 938ed86 |
|
|
|
|
## B. Functional gaps (feature exists in plan but not implemented)
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | ---------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| B1 | Phase 2 plan called for RSI / MACD / Bollinger Band overlay toggles below `StockChart`. Not built. | 🟠 | ✅ | c54b9f2 |
|
|
| B2 | `TickerHeader` "★ Watchlist" and 🔔 buttons have no `onClick` — purely cosmetic. | 🟠 | ✅ | ed8175e |
|
|
| B3 | `TickerHeader` company name is the symbol (placeholder). Should pull from `fetchResearchProfile`. | 🟠 | ✅ | ed8175e |
|
|
| B4 | `TickerHeader` "as of" timestamp uses `new Date()` (always = now). Should use the latest bar timestamp. | 🟡 | ⬜ | |
|
|
| B5 | `QuickStats` reads RSI/EMA only from `botState.symbols[symbol].indicators`. The bot only computes these for symbols it actively tracks → user-searched tickers always show `—`. Need an indicator service or compute from bars client-side. | 🟠 | ⬜ | |
|
|
| B6 | `CodeStrategyEditor` "Save" button writes to `localStorage` only, then shows "Saved!". Misleading — no persistence. | 🟠 | ✅ | da79682 |
|
|
| B7 | `CodeStrategyEditor` defines `editorRef` but never uses it. | 🟢 | ⬜ | |
|
|
| B8 | `VisualRuleBuilder` exposes `onBacktest` prop but `ResearchView` doesn't pass one — backtest button never appears. | 🟡 | ⬜ | |
|
|
| B9 | `ResearchView` "Strategies" tab still shows the legacy `MyStrategiesTab`; visual/code-saved strategies don't appear there. | 🟡 | ⬜ | |
|
|
| B10 | No 404 / catch-all React Router route — invalid paths render blank. | 🟡 | ⬜ | |
|
|
|
|
## C. Security & correctness
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | ---------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| C1 | Backend posts arbitrary user JS (`strategyCode`) to `/api/backtest` if A1+A2 are "fixed" naively. Must sandbox or refuse. | 🔴 | ✅ | 6aa001a |
|
|
| C2 | No FMP response cache. Free tier = 250 req/day. Every Home view load = 3 req. 80 page loads/day → quota burnt by lunch. | 🟠 | ✅ | 0828007 |
|
|
| C3 | `/api/screener` passes `sector` query through to FMP without an allow-list. Low-impact injection, but should validate. | 🟡 | ✅ | c173aeb |
|
|
| C4 | `/api/news` passes `symbols` through to Alpaca without validation. | 🟡 | ✅ | 7c4b08c |
|
|
| C5 | Header `fetchMarketIndices` polls every 60 s even when the tab is hidden. Should pause via `document.visibilityState`. | 🟡 | ✅ | e089832 |
|
|
| C6 | `backend/.env.example` keeps `FMP_API_KEY=demo` AND `apiServer.ts` falls back to `'demo'`. Two sources of truth. Demo key is shared globally and rate-limited. | 🟡 | ✅ | 1377bf2 |
|
|
| C7 | FMP `apikey` is sent as a query string → leaks into proxy / CDN logs. FMP doesn't support headers, so the only mitigation is server-side caching (see C2). | 🟡 | ⬜ | |
|
|
|
|
## D. UX / UI polish
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | ---------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| D1 | News, ResearchCards, Screener show plain "Loading…" text. Should be skeleton placeholders to prevent layout jank. | 🟡 | ✅ | 0fb1d63 |
|
|
| D2 | Light theme broke contrast in legacy tabs (`SettingsTab`, `ConfigTab`, etc.) which were styled for dark. | 🟡 | ⬜ | |
|
|
| D3 | `TickerHeader` exchange is hard-coded "NASDAQ". | 🟢 | ⬜ | |
|
|
| D4 | `ScreenerView` "More sectors…" `<select>` doesn't visually highlight the chosen sector pill once selected. | 🟡 | ⬜ | |
|
|
| D5 | Layout is fixed-width (72 px sidebar + 308 px right panel). Breaks below ~1024 px viewport. No mobile-web responsive breakpoints. | 🟠 | ✅ | a9c33b1 |
|
|
| D6 | `VisualRuleBuilder` and `CodeStrategyEditor` use `setTimeout` without cleanup — leak warning if component unmounts mid-timer. | 🟢 | ⬜ | |
|
|
| D7 | No keyboard shortcuts (⌘K search, ⌘S save in code editor, ⌘Enter run backtest). | 🟢 | ⬜ | |
|
|
| D8 | `EmptyState` ticker chips are hard-coded equities. For a crypto-config bot the chips don't apply. | 🟢 | ⬜ | |
|
|
| D9 | No `prefers-reduced-motion` handling on spinners / animations. | 🟢 | ⬜ | |
|
|
|
|
## E. Build / infrastructure
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | ---------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| E1 | Bundle is 1.08 MB (309 kB gzipped) — Monaco is the bulk. Lazy-load Monaco via `React.lazy`. | 🟠 | ⬜ | |
|
|
| E2 | Root `pnpm-lock.yaml` doesn't contain `react-router-dom`, `@monaco-editor/react`, `@dnd-kit/*` (they were installed via `npm` in `web/` causing `web/package-lock.json` and `web/pnpm-lock.yaml` to appear). Workspace builds in CI will fail. **Partial fix**: stray subpackage lockfiles deleted + `.gitignore` updated to prevent recurrence. **Still TODO**: run `pnpm install -r --no-frozen-lockfile` from the repo root on a workstation that has `GITEA_NPM_TOKEN` exported (this session can't reach the private registry → mobile install fails). One-line follow-up commit. | 🔴 | ✅ | 3c9117f |
|
|
| E3 | Monaco's web workers (TS/JSON/CSS/HTML) are pulled at runtime from a CDN by default. Need explicit Vite config to bundle workers locally for offline / CSP-strict deployments. | 🟡 | ⬜ | |
|
|
| E4 | No README / docs section describing the new layout, env vars (`FMP_API_KEY`), or routes. | 🟡 | ⬜ | |
|
|
|
|
## F. Test coverage
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | ---------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| F1 | 0 tests for `web/src/lib/marketApi.ts`. | 🟡 | ⬜ | |
|
|
| F2 | 0 tests for `VisualRuleBuilder.tsx`. | 🟡 | ⬜ | |
|
|
| F3 | 0 tests for `CodeStrategyEditor.tsx`. | 🟡 | ⬜ | |
|
|
| F4 | 0 tests for `HomeView.tsx` (StockChart fetch path, ResearchCards). | 🟡 | ⬜ | |
|
|
| F5 | 0 tests for `ScreenerView.tsx` (filter/sort/sector logic). | 🟡 | ⬜ | |
|
|
| F6 | 0 tests for the 7 new backend endpoints (`/api/chart/bars`, `/api/news`, `/api/market/indices`, `/api/research/*`, `/api/screener`). | 🟠 | ✅ | e2806b2 |
|
|
| F7 | Pre-existing: 3 `useTabFeatureFlags.dom.test.tsx` failures from module-level cache leaking between tests. | 🟡 | ✅ | ece7fa9 |
|
|
| F8 | Pre-existing: 1 `EntryForm.dom.test.tsx` failure ("alerts error when trade execution fails"). | 🟡 | ✅ | ece7fa9 |
|
|
|
|
## G. Mobile parity gap
|
|
|
|
| # | Issue | Severity | Status | Fix commit |
|
|
| --- | ---------------------------------------------------------------------------------------------------------------------------------- | :------: | :----: | ---------- |
|
|
| G1 | Mobile app (`mobile/app/(tabs)/`) has 5 tabs: index, positions, history, strategies, settings. None of the new web features (news, research, screener) exist on mobile. | 🟠 | ⬜ | |
|
|
| G2 | Mobile `TradingDataProvider` doesn't fetch news / indices / company profile. It only consumes the WebSocket bot state. | 🟠 | ⬜ | |
|
|
| G3 | No mobile counterpart to `VisualRuleBuilder` or `CodeStrategyEditor`. (Code editor likely shouldn't ship to mobile, but visual builder should.) | 🟡 | ⬜ | |
|
|
| G4 | Mobile `lib/runtime.ts` doesn't expose the new endpoints — no shared client. | 🟡 | ⬜ | |
|
|
|
|
---
|
|
|
|
## Ordering of fixes
|
|
|
|
The plan: walk top-to-bottom, one bucket per commit, smallest blast radius first.
|
|
|
|
1. **Section A** (critical broken integrations) — unblocks Strategy Builder + News + Backtest in one go.
|
|
2. **Section E2** (lockfile mess) — unblocks CI; do this before anyone else pulls.
|
|
3. **Section B** (functional gaps) — split into sub-commits per gap to keep diffs reviewable.
|
|
4. **Section C** (security & correctness).
|
|
5. **Section D** (UX polish).
|
|
6. **Section F** (tests) — added alongside each fix where possible, sweep at the end.
|
|
7. **Section G** (mobile parity) — separate commit set; biggest scope, least urgent.
|
|
|
|
Each fix lands in its own commit with the issue ID in the subject (e.g. `fix(A1): point CodeStrategyEditor at /api/backtest/run`). Ticking the table as we go.
|