fix(frontend): broken test, mock-only useTrade, any types (#104)

[ajitpratap0]

Summary

Closes #104 — three bugs in the Next.js dashboard frontend.

1. useTrade always uses mock data (BLOCKS PRODUCTION)

useTrade(id) called getMockTrades() unconditionally, ignoring USE_MOCK_DATA. Detail views always showed mock data even when the real API was available.

Fix: Now checks USE_MOCK_DATA first. When hitting the real API, calls apiClient.getTrades() and filters client-side (no single-trade endpoint exists yet).

2. Broken test fixture (camelCase vs snake_case)

useTradeData.test.tsx used a camelCase Trade fixture (entryPrice: 45045) as mock API response data, but the hook runs mapApiTrade which expects snake_case RawApiTrade (price: 45045). The test passed coincidentally because the symbol assertion happened to match, but entryPrice was undefined.

Fix: Test now uses a fakeRawTrade fixture matching the snake_case API response shape.

3. Pervasive any types in WebSocket hook

useTradingWebSocket used Record<string, any>[] for all state arrays (trades, positions, orders, agents) and any for the sendMessage parameter. Consumers had no type safety.

Fix: Replaced with Trade[], Position[], Order[], Agent[] from lib/types. marketData changed to Record<string, unknown>. sendMessage parameter changed to Record<string, unknown>.

Test plan

• All 23 dashboard tests pass (npm run test -- --run)
• TypeScript type-check clean (npx tsc --noEmit)
• CI

3 files changed, 53 insertions(+), 21 deletions(-)

[claude]

Code Review

Good set of targeted fixes. The root-cause analysis in the PR description is accurate and the comments in the diff are genuinely helpful. A few things worth flagging before merge:

---

Bug: useTrade silently misses trades beyond the first page

useTrade now calls apiClient.getTrades() with no arguments, which uses the defaults limit=50, offset=0 (api.ts:334). If the requested trade ID falls outside the first 50 results the function returns { data: null } with no error — the caller has no way to distinguish "trade not found" from "trade exists but wasn't fetched."

// hooks/useTradeData.ts
const response = await apiClient.getTrades()  // always limit=50, offset=0

Either pass a large-enough limit (not great but pragmatic until a single-trade endpoint exists) or throw when the trade is not found so the caller can surface an error rather than rendering a blank detail view:

const trade = tradeList.find(t => t.id === id)
if (!trade) throw new Error(`Trade ${id} not found`)

---

Type safety in useTradingWebSocket is nominal, not enforced

The state arrays are now typed as Trade[], Position[], etc., but WebSocketMessage.data is still any (types.ts:153). TypeScript lets any flow into typed state without complaint, so the compile-time guarantee is weaker than it looks:

// useWebSocket.ts — message.data is any; TS does not verify the shape
setTrades(prev => [message.data, ...prev])   // unchecked cast
setPositions(prev => [...prev.filter(p => p.symbol !== message.data.symbol), message.data])

The type on useState now documents the intended shape, which is useful. But for real safety you'd need either a runtime validator (e.g. Zod) or at minimum an explicit cast with a comment acknowledging the risk:

setTrades(prev => [message.data as Trade, ...prev])

Not a blocker for this PR since you're not making the problem worse, but worth a follow-up issue.

---

Minor: fakeRawTrade should be typed as RawApiTrade

The fixture is currently untyped, meaning silent drift is possible if RawApiTrade gains required fields. Since RawApiTrade is not exported from useTradeData.ts, the easiest fix is to add a local re-export in the test file or just add an inline type annotation once it's exported:

// in useTradeData.ts — add `export` so tests can reference it
export interface RawApiTrade { ... }

// in the test
import type { RawApiTrade } from '@/hooks/useTradeData'
const fakeRawTrade: RawApiTrade = { ... }

This turns the test into a compile-time check against the mapping contract.

---

What's good

• The mock-guard fix in useTrade is correct and the conditional structure mirrors useTrades cleanly.
• The raw: unknown → type-guard → cast pattern is the right way to handle untyped API responses in TypeScript.
• The sendMessage: Record<string, unknown> and marketData: Record<string, unknown> changes are straightforward improvements.
• Test comments explaining the camelCase vs snake_case split are clear and will help future maintainers.

The pagination miss is the only change I'd want addressed before merge. The rest are follow-up material.

[claude]

Code Review

Good set of targeted fixes. The three described bugs are real and the fixes are generally correct. A few issues worth addressing before merge:

---

1. useTrade fetches the entire trade list on every call [Medium–High]

File: web/dashboard/hooks/useTradeData.ts

const response = await apiClient.getTrades()  // no limit/offset

useTrade(id) calls getTrades() with no pagination, pulling every trade to find one by ID. This will noticeably degrade as trade count grows. Additionally, useTrades(50, 0) caches under ['trades', 50, 0] while useTrade(id) caches under ['trades', id] — they share zero cache, so both hooks mounted simultaneously fire two separate full-list fetches.

Suggestion: Derive the single trade from the list query's cached data using React Query's queryClient.getQueryData, or use initialData seeded from useTrades. At minimum, pass limit/offset to avoid open-ended fetches, and track the missing single-trade endpoint as a GitHub issue.

---

2. useTrade is missing staleTime / refetchInterval [Low]

File: web/dashboard/hooks/useTradeData.ts

useTrades sets both; useTrade sets neither. A detail view will refetch on every focus/remount while the list view observes the configured interval. Add the same options for consistency:

staleTime: REFRESH_INTERVALS.trades,
refetchInterval: REFRESH_INTERVALS.trades,

---

3. Type-safe state arrays don't prevent unsafe any assignment at runtime [Low]

File: web/dashboard/hooks/useWebSocket.ts

The comment is honest — message.data is any, so the typed state arrays (Trade[], Position[], etc.) provide compile-time documentation but no runtime protection. The issue is that TypeScript silently accepts any → Trade casts, so a shape mismatch from the backend will produce no error but will break consumer code at runtime.

This is a known limitation of untyped WebSocket messages, but worth a follow-up to add runtime validation (e.g., Zod schemas mirroring the Trade/Position types) — or at least a brief TODO comment so reviewers know the gap is intentional.

---

4. fakeRawTrade fixture has no type annotation [Low]

File: web/dashboard/__tests__/useTradeData.test.tsx

const fakeRawTrade = { ... }  // untyped

Since RawApiTrade is not exported from useTradeData.ts, the test fixture can silently drift from the actual interface if field names change. Consider exporting RawApiTrade (or a test-only equivalent) and typing the fixture:

const fakeRawTrade: RawApiTrade = { ... }

This would have caught the original camelCase/snake_case bug at compile time.

---

5. mapApiTrade logic is under-tested [Low]

File: web/dashboard/__tests__/useTradeData.test.tsx

The existing test checks symbol and entryPrice. The non-obvious mapping rules — side: 'BUY' → 'long', side: 'SELL' → 'short', status derivation, and agent defaulting to exchange — are untested. A SELL fixture would catch a regression in those paths.

---

6. Go test change belongs in a separate PR [Low]

File: internal/db/positions_close_integration_test.go

This change references issue #218 and tests fee accumulation behavior for SHORT positions — it's logically unrelated to the three frontend bugs described in this PR (issue #104). Mixing backend test changes with frontend fixes makes the diff harder to review and bisect. Please split it out.

Also, the comment on line 90 reads:

// #218 review: exercise (fees + $4) path for SHORT too

The $4 reference is unclear — the fees here total $1.00 (0.75 + 0.25), not $4. Looks like a copy-paste artifact.

---

Minor nit

The as unknown as { trades: Trade[]; count: number } cast in the test is a double-cast escape hatch that hides the type mismatch. That's acceptable for test code given RawApiTrade isn't exported, but it underscores point 4 above.

---

Summary: The three frontend fixes are correct and solve real bugs. Items 1 (performance) and 6 (scope) are the most important to address before shipping; the rest are low-effort improvements that would strengthen the code.

[ajitpratap0]

Acknowledged from the review:

useTrade pagination: Valid point — useTrade fetches only the first page (default limit=50). For now this matches the dashboard's use case (detail views for recent trades visible in the list), but a future getTrade(id) backend endpoint would be the proper fix. Not blocking this PR since the current behavior is strictly better than always-mock.

Type safety nominal: Agreed — WebSocketMessage.data is still any at runtime. The state-array typing is documentation-grade, not enforcement-grade. A discriminated union on WebSocketMessage.type → data would be the proper fix. Follow-up scope.

fakeRawTrade typing: Could add as RawApiTrade but RawApiTrade is not exported from useTradeData.ts (it's a file-private interface). Exporting it just for the test would expand the public API. The fixture matches the shape by construction; the test exercises mapApiTrade end-to-end which catches mismatches.

[claude]

Code Review

The two existing reviews cover the important issues well. The PR author has acknowledged the pagination miss and the nominal type-safety point. Two items from the second review haven't been acknowledged yet — both are worth resolving before merge.

---

1. useTrade is missing staleTime / refetchInterval [Low — easy fix]

useTrades has both options; useTrade has neither. A detail view will therefore refetch on every window focus and remount while the list view respects the configured interval. The fix is a one-liner:

// hooks/useTradeData.ts — useTrade
enabled: !!id,
staleTime: REFRESH_INTERVALS.trades,
refetchInterval: REFRESH_INTERVALS.trades,

---

2. useTrades and useTrade cache under different keys — two hooks mounted simultaneously fire two full-list fetches

useTrades(50, 0) caches under ['trades', 50, 0]; useTrade(id) caches under ['trades', id]. A page that renders both the list and a detail view will hit the API twice with no deduplication.

The pragmatic fix (no backend changes needed) is to seed useTrade from the list cache:

import { useQueryClient } from '@tanstack/react-query'

export function useTrade(id: string) {
  const queryClient = useQueryClient()
  return useQuery({
    queryKey: [...QUERY_KEYS.trades, id],
    queryFn: async () => {
      // Try the already-fetched list first
      const cached = queryClient.getQueryData<{ data: Trade[] }>([...QUERY_KEYS.trades, 50, 0])
      const fromCache = cached?.data?.find(t => t.id === id)
      if (fromCache) return { success: true as const, data: fromCache, timestamp: new Date().toISOString() }

      // Fall back to a full fetch if the trade isn't in the list cache
      const response = await apiClient.getTrades()
      // ... rest of existing logic
    },
    staleTime: REFRESH_INTERVALS.trades,
    refetchInterval: REFRESH_INTERVALS.trades,
    enabled: !!id,
  })
}

This is a larger change so it's acceptable to defer to a follow-up issue — but worth tracking once trade history grows past 50 entries.

---

Overall

The core fixes are correct and clearly described. The production bug (always-mock useTrade) is the headline fix and the implementation is sound. Item 1 (staleTime) is low-effort enough to include in this PR; item 2 can be a follow-up.

[claude]

Follow-up Review

The existing review thread covers the important issues. This is a brief status summary of what's still open vs. what's been acknowledged.

---

Still unaddressed: staleTime / refetchInterval in useTrade

This was raised in the third review comment and is a one-liner fix with no ambiguity. useTrades configures both; useTrade has neither, which means detail views refetch on every window focus. Easy to add before merge:

// hooks/useTradeData.ts — useTrade
enabled: !!id,
staleTime: REFRESH_INTERVALS.trades,
refetchInterval: REFRESH_INTERVALS.trades,

---

Acknowledged but worth a tracking issue

• Pagination / cache deduplication: The author's acknowledgement ("matches the dashboard's use case for recent trades") is reasonable for now. Worth filing a follow-up issue so it doesn't get forgotten once trade history grows past 50 entries.
• RawApiTrade not exported: The author's reason (don't expand public API just for a test) is fair. An alternative is a separate __tests__/fixtures/trades.ts file that owns the raw fixture type locally, no export needed from the hook. Low priority.

---

One thing not mentioned in previous reviews

useTrade always returns { success: true, data: null } when the trade isn't found — even when the API call itself succeeded. A caller that checks result.success gets true regardless of whether the trade exists. The earlier suggestion to throw on not-found is the cleanest resolution; alternatively, the return type could use data: Trade | null with a documented "not found means null, not an error" contract. Neither is blocking, but the current silent-null behavior could confuse a future consumer.

---

Overall: The three bugs are correctly fixed. staleTime is the one easy improvement worth squeezing in before merge.

[ajitpratap0]

Follow-up fixes pushed (552ecd1):

1. staleTime + refetchInterval: Added to useTrade, matching useTrades. Detail views no longer refetch on every window focus.

2. null contract documented: Doc comment now explains data: Trade | null — null means not found in the fetched list, not an API error. Changed trade || null to trade ?? null for explicit nullish coalescing.

3. Pagination/cache dedup follow-up: Filed as [Frontend] useTrade pagination: detail views miss trades beyond first page #224 with three resolution options (backend single-trade endpoint, higher limit, shared query cache).

All 23 tests pass, tsc clean.

[claude]

PR Review: fix(frontend): broken test, mock-only useTrade, any types

Good set of fixes — all three bugs are real and the general approach is correct. A few things worth addressing before merge.

---

useTrade — silent truncation if API default page size < total trades

getTrades() is called with no limit or offset args (useTradeData.ts:142), so it relies on whatever the backend default is (likely 20–50 rows). If the trade being looked up by id sits beyond page 1, tradeList.find() returns null — not because the trade doesn't exist, but because it wasn't in the fetched page. The UI silently shows "trade not found" for valid trades.

Compare to useTrades() which lets callers pass limit/offset. At minimum, useTrade should pass a sufficiently large limit or document the implicit ceiling clearly.

---

useTrade — no cache sharing with useTrades, causing duplicate network requests

Each useTrade(id) gets its own query key (['trades', id]) and makes an independent getTrades() call. If a page renders both a trade list (useTrades) and a detail panel (useTrade), two separate full-list fetches go out.

A lighter approach while the single-trade endpoint is missing — derive from the already-populated list cache:

export function useTrade(id: string) {
  const queryClient = useQueryClient()
  return useQuery({
    queryKey: [...QUERY_KEYS.trades, id],
    queryFn: async () => {
      const cached = queryClient.getQueryData<{ data: Trade[] }>([...QUERY_KEYS.trades, 50, 0])
      const fromCache = cached?.data?.find(t => t.id === id)
      if (fromCache) return { success: true as const, data: fromCache, timestamp: new Date().toISOString() }
      // Fall back to full fetch if list not cached
      const response = await apiClient.getTrades()
      // ... rest of mapping
    },
    enabled: !!id,
  })
}

---

useTrade — refetchInterval causes repeated full-list fetches on detail pages

Lines 160-161 add staleTime and refetchInterval matching the list hook. Polling a detail view by re-fetching the entire trade list on every interval is expensive. Consider omitting refetchInterval from useTrade until a dedicated endpoint exists.

---

useWebSocket.ts — runtime type gap in state setters

setTrades(prev => {
  const newTrades = [message.data, ...prev]  // message.data is still `any`
})

TypeScript accepts this because message.data is typed as any, which satisfies Trade silently. Malformed backend payloads won't be caught at compile time. This is acceptable given the broader backend typing situation — just worth noting for future work.

---

Test coverage gap for the useTrade fix

The test file covers useTrades in 4 scenarios but has zero coverage for useTrade, which is the hook with the production-blocking bug. Worth adding at minimum:

• Real API path returns correct single trade
• Trade not found returns null in data.data
• API error propagates as thrown error

---

Minor: double cast in test mock

data: { trades: [fakeRawTrade], count: 1 } as unknown as { trades: Trade[]; count: number },

If RawApiTrade were exported from the hook module, the mock could be typed as { trades: RawApiTrade[]; count: number } directly, making the intent explicit without the forced cast.

---

Summary

Issue	Severity	Blocking?
useTrade silently truncates if trade is past API page limit	High	Recommended fix
No cache sharing with useTrades — duplicate network requests	Medium	No
refetchInterval on detail hook fetches full list repeatedly	Medium	No
Runtime type gap in WS state setters (message.data is any)	Low	No
No useTrade tests despite fixing its bug	Medium	Recommended
Double cast in test fixture	Low	No

The test fixture fix and any cleanup are solid improvements. The main thing worth addressing before merge is the pagination truncation issue — it's a different manifestation of the same "missing data in real API mode" bug this PR set out to fix.

[ajitpratap0]

All follow-up review items addressed (8a04173):

Pagination truncation: useTrade now fetches with limit=1000 (up from default 50) so trades beyond the first page aren't silently truncated.

Cache sharing: Checks useTrades query cache first via queryClient.getQueriesData — if the list view already fetched the trade, the detail view reuses it with zero network overhead.

refetchInterval removed: Polling a detail view by re-fetching the full trade list on every interval was wasteful. Kept staleTime for tab-switch cache reuse.

Test coverage: 4 new useTrade tests (27 total, up from 23):

• Real API returns correct single trade (verifies limit=1000 passed)
• Trade not found → null in data
• API error propagates as thrown error
• Disabled when id is empty (fetchStatus === 'idle')

Also fixed a TS18048 strict-null error on the cache lookup.

All 27 tests pass, tsc clean.

[claude]

Code Review: fix(frontend): broken test, mock-only useTrade, any types (#104)

Good fixes overall — the three bugs are real and the approach is well-reasoned. Found one latent runtime error and a few smaller issues below.

---

Bug: Cache loop will throw TypeError on its own results

hooks/useTradeData.ts, lines 152-167

getQueriesData({ queryKey: QUERY_KEYS.trades }) does a prefix match — it returns every query whose key starts with ['trades']. That includes not only useTrades entries (shape { data: Trade[] }) but also prior useTrade entries (shape { data: Trade | null }).

When the loop hits a stale useTrade('some-id') cache entry, cached.data is Trade | null, not Trade[]. Calling .find() on a plain object (or null) throws:

TypeError: cached.data.find is not a function

The if (!cached) continue guard only skips undefined/falsy — it won't catch a non-array Trade object.

Fix: add an Array.isArray guard before calling .find():

for (const [, cached] of cachedQueries) {
  if (!cached || !Array.isArray(cached.data)) continue
  const found = cached.data.find((t: Trade) => t.id === id)
  ...
}

This also makes the type narrowing explicit rather than relying on an implicit structural assumption.

---

WebSocket type safety is shallower than it looks

hooks/useWebSocket.ts, lines 211-246

The state arrays are now typed (Trade[], Position[], etc.) — good. But message.data is still any (WebSocketMessage.data: any in lib/types.ts), so the assignments:

setTrades(prev => [message.data, ...prev])     // message.data is `any`
setPositions(prev => [...filtered, message.data])

compile without error but aren't type-safe at runtime. The WebSocket backend sends raw JSON that's never validated against Trade/Position/etc. A field rename or backend schema change will produce silent runtime failures (e.g. trade.entryPrice is undefined everywhere in the UI).

This is a pre-existing gap, and the PR correctly notes it in the comment. But if the goal is "consumers get type-safe access," it's worth a follow-up issue to add runtime narrowing here (even just Array.isArray / 'symbol' in message.data guards) rather than leaving the improvement incomplete.

---

Hardcoded limit of 1000 is a silent data ceiling

hooks/useTradeData.ts, line 171

const response = await apiClient.getTrades(1000, 0)

If an account has >1000 trades, looking up a specific old trade will silently return data: null (trade not found) instead of the real trade. Trades beyond the 1000 cap are invisible to useTrade.

The PR description acknowledges "no single-trade endpoint exists yet" — worth opening a follow-up to add GET /api/v1/trades/:id so this can be removed. The fetch-all-and-filter pattern is a stepping stone, not a permanent solution.

---

Missing test: cache-hit path is not exercised

__tests__/useTradeData.test.tsx

The four new useTrade tests all exercise the cache-miss path (no prior queries → falls through to apiClient.getTrades). There's no test that pre-seeds a useTrades result and verifies that useTrade finds the trade in cache without making a second network call. That's exactly the optimization added here, and it's the path most likely to break with the bug described above.

---

Minor: staleTime without refetchInterval creates a no-refresh window

hooks/useTradeData.ts, line 189

staleTime: REFRESH_INTERVALS.trades,  // 2000ms
// refetchInterval intentionally omitted

After 2 seconds the cached entry goes stale, but without refetchInterval React Query won't background-refetch unless the component is refocused/remounted. For a trade detail view the user might sit on for several minutes, this means the data shown stays frozen even while the position moves. The comment explains the reasoning ("polling a detail view… is expensive"), but a longer staleTime (e.g. 30_000) without refetchInterval might better reflect the intent than a 2-second stale window that never triggers auto-refresh.

---

Nit: as unknown as cast in test fixtures

__tests__/useTradeData.test.tsx, line 69, 142, etc.

data: { trades: [fakeRawTrade], count: 1 } as unknown as { trades: Trade[]; count: number },

This cast exists because fakeRawTrade is RawApiTrade shape, not Trade. Exporting RawApiTrade from the hook module (or a shared types file) would let tests use it directly:

data: { trades: [fakeRawTrade as RawApiTrade], count: 1 }

Not a blocker, but it removes the unsafe cast and makes the test's intent explicit.

---

Summary

Must fix	Array.isArray guard in cache loop (will throw at runtime once useTrade has been called once)
Follow-up	Single-trade API endpoint to retire the 1000-limit workaround
Follow-up	Cache-hit test coverage
Nice to have	Runtime validation in WebSocket handler; staleTime tuning; export RawApiTrade

The three original bug fixes are correct and the test restructure around fakeRawTrade is the right call. Just needs the Array.isArray guard before merging.

[ajitpratap0]

Must-fix addressed (6beed29):

Array.isArray guard: getQueriesData prefix-matches ['trades'] returning both useTrades entries (data: Trade[]) and prior useTrade entries (data: Trade | null). Without the guard, .find() on a non-array Trade object would throw TypeError at runtime. Added if (!cached || !Array.isArray(cached.data)) continue.

staleTime tuned: Bumped from REFRESH_INTERVALS.trades (2s) to 30_000 (30s). Without refetchInterval the 2s window went stale immediately and never auto-refreshed — 30s gives a reasonable cache-reuse window for tab switches while the detail view is open.

27 tests pass, tsc clean. Follow-ups (single-trade endpoint, cache-hit test, WebSocket runtime validation) tracked in #224.

[claude]

PR Review: fix(frontend): broken test, mock-only useTrade, any types (#104)

Good fixes overall — all three bugs are real and the changes are well-reasoned. The commit history shows iterative refinement (Array.isArray guard, staleTime tuning) which is a good sign. Some items worth addressing before merge:

---

Issues

1. limit=1000 silently truncates large datasets (useTradeData.ts)

const response = await apiClient.getTrades(1000, 0)

If a user has >1000 trades, useTrade will silently fail to find trades past that cutoff. The PR description acknowledges follow-up issue #224, but this should at minimum be a named constant (MAX_TRADE_LOOKUP_LIMIT) with a comment linking to the pagination ticket. A magic number here is easy to miss.

Suggestion:

// TODO (#224): replace with a paginated lookup once the backend
// supports GET /trades/:id
const MAX_TRADE_LOOKUP_LIMIT = 1000
const response = await apiClient.getTrades(MAX_TRADE_LOOKUP_LIMIT, 0)

2. WebSocket typed arrays may give false type safety (useWebSocket.ts)

The REST API sends snake_case JSON (price, executed_at, etc.) and mapApiTrade is needed to convert to the camelCase Trade shape. The WebSocket path in useTradingWebSocket now declares trades as Trade[], but if the backend WebSocket also sends snake_case JSON (same backend, same encoding), consumers accessing trade.entryPrice will silently get undefined at runtime — TypeScript thinks the shape is right, but it isn't.

The comment in the code acknowledges WebSocketMessage.data is any at runtime, but stops short of the implication: consumers of useTradingWebSocket().trades are no safer than before if the WS wire format matches the REST format.

Could you confirm whether the WebSocket payload is already camelCase, or does it need a mapApiTrade pass? If the latter, this change trades one bug for a quieter one.

3. api.ts return type lies about getTrades shape

// api.ts
async getTrades(limit = 50, offset = 0): Promise<ApiResponse<{ trades: Trade[]; count: number }>>

The backend actually returns RawApiTrade[], but api.ts types the response as Trade[]. The test workaround (as unknown as { trades: Trade[]; count: number }) is a symptom of this. The useTrade hook correctly mistrusts the type with a runtime 'trades' in raw check, but it makes the code harder to follow and could mislead future contributors. Worth fixing api.ts to return RawApiTrade[] as a follow-up (or in this PR if small enough).

---

Strengths

• Cache lookup before fetch — checking queryClient.getQueriesData before hitting the network is the right call; avoids duplicate fetches when list and detail views are mounted together.
• Array.isArray guard — correctly handles the mixed-type cache entries from the prefix match on ['trades']. This was a latent TypeError waiting to happen.
• staleTime: 30_000 reasoning — the comment explains why 2s (REFRESH_INTERVALS.trades) would be wrong here without refetchInterval. Good reasoning documented in code.
• ?? null over || null — minor but correct; avoids false positives on falsy trade values.
• Test coverage — four new useTrade tests cover the real API path, not-found case, error propagation, and disabled state. The fakeRawTrade fixture now exercises mapApiTrade end-to-end, which was the original bug.

---

Minor

• data: null as unknown as { trades: Trade[]; count: number } in the error test case is a bit gnarly. Once api.ts is typed correctly with RawApiTrade, this cast can be simplified.
• Consider exporting fakeRawTrade as a shared test fixture if other test files ever need an RawApiTrade shape — currently duplicated if any new tests land.

---

Summary: Approachable with the WebSocket type-safety question answered and the magic 1000 extracted to a named constant. The cache-dedup logic is solid.

[ajitpratap0]

Pushed 40e765d:

1. Named constant: MAX_TRADE_LOOKUP_LIMIT = 1000 with TODO (#224) linking to the single-trade endpoint follow-up. No more magic number.

2. WS wire-format WARNING: Added explicit comment documenting that the backend WS payload uses snake_case (same as REST) but the WS path does NOT run mapApiTrade — so trade.entryPrice is undefined at runtime because the wire format has price. A mapper per message.type is the proper fix, tracked alongside [Frontend] useTrade pagination: detail views miss trades beyond first page #224.

The api.ts return type (Trade[] instead of RawApiTrade[]) is a pre-existing issue that affects the whole API client, not just this hook. Fixing it here would expand scope significantly. Filed as part of #224.

27 tests pass, tsc clean.

[claude]

Code Review — PR #223

The three bugs being fixed are real and well-diagnosed. The overall approach is sound. A few issues worth addressing before merge, ranging from a non-trivial type-safety problem to minor improvements.

---

1. useTradingWebSocket: the Trade[] type is a lie (moderate)

web/dashboard/hooks/useWebSocket.ts, around line 212

The PR replaces Record<string, any>[] with Trade[], but the handleMessage callback pushes message.data directly into setTrades without running mapApiTrade. The wire format is snake_case (price, executed_at, …); the Trade type is camelCase (entryPrice, timestamp, …). So consumers calling trade.entryPrice get undefined at runtime despite TypeScript being happy.

The WARNING comment acknowledges this, but Record<string, any>[] was at least honest — it didn't make a false type promise. The current state is arguably worse: it looks type-safe to the compiler and to every consumer of this hook, but it silently lies.

Two paths forward:

• Keep Record<string, any>[] for now (honest) and add a comment saying "typed once WS mapper lands."
• Apply mapApiTrade inline in the 'trade' case before pushing into state. The function is already in the same file's import tree via useTradeData.

If the follow-up issue (#224) is going to be picked up soon, option 2 is the right fix. If it might sit for weeks, option 1 avoids shipping a misleading type contract to all callers.

---

2. Missing test for the cache-hit path (moderate)

web/dashboard/__tests__/useTradeData.test.tsx

The most complex new logic in useTrade is the queryClient.getQueriesData cache check — and in fact, a bug in that path (missing Array.isArray) required a follow-up fix in commit 6beed290. But there's still no test that exercises the happy path: mount useTrades first, then useTrade, and assert that getTrades was only called once (the cache hit).

Without this test, a future refactor of the cache-scan loop could silently break the dedup behavior. A sketch:

it('returns trade from useTrades cache without a second network call', async () => {
  const { apiClient } = await import('@/lib/api')
  vi.mocked(apiClient.getTrades).mockResolvedValue({
    success: true,
    data: { trades: [fakeRawTrade], count: 1 } as unknown as ...,
    timestamp: new Date().toISOString(),
  })

  const wrapper = makeWrapper()
  // Seed the list cache
  renderHook(() => useTrades(), { wrapper })
  await waitFor(() => expect(apiClient.getTrades).toHaveBeenCalledTimes(1))

  // Detail hook should hit cache, not the network
  const { result } = renderHook(() => useTrade('trade-abc'), { wrapper })
  await waitFor(() => expect(result.current.isSuccess).toBe(true))

  expect(result.current.data?.data?.id).toBe('trade-abc')
  expect(apiClient.getTrades).toHaveBeenCalledTimes(1) // no second call
})

Note: both hooks must share the same QueryClient instance, so wrapper must not call makeWrapper() a second time.

---

3. MAX_TRADE_LOOKUP_LIMIT defined inside queryFn (nit)

web/dashboard/hooks/useTradeData.ts, line 177

const MAX_TRADE_LOOKUP_LIMIT = 1000
const response = await apiClient.getTrades(MAX_TRADE_LOOKUP_LIMIT, 0)

The constant is re-declared on every query execution. Move it to module scope alongside QUERY_KEYS so it's visible in tests that need to assert on the call arguments (which the existing test does via the magic number 1000 on line 154).

---

4. fakeRawTrade could be explicitly typed (nit)

web/dashboard/__tests__/useTradeData.test.tsx, line ~36

const fakeRawTrade = { id: 'trade-abc', ... }

If RawApiTrade is exported from useTradeData (or a shared types file), annotating this as RawApiTrade makes the fixture self-documenting and will catch drift if the interface changes. The current as unknown as { trades: Trade[]; count: number } cast silently sidesteps the type check.

---

What's good

• Root causes are correctly identified — especially the mapApiTrade fixture bug, which was the trickiest to diagnose.
• The Array.isArray guard is exactly right and the commit message explaining why it's needed is clear.
• Cache-first lookup in useTrade is the correct pattern here; fetching the full list on every detail-view render would be unnecessarily expensive.
• ?? over || is a good tightening for the null-coalescing case.
• The WS WARNING comment is at least honest about the runtime state and gives the next developer exactly what they need to close the gap.
• Test count going 23 → 27 with meaningful coverage of the four core useTrade scenarios is solid.

---

Overall: the three stated bugs are genuinely fixed and the code is better than before. Issue 1 (type lie in useTradingWebSocket) is worth resolving before merge — either apply the mapper or revert to the honest Record<string, any>[]. Issues 2-4 are improvements but not blockers.