Create debugging frontend for trading strategies

[guzus]

No description provided.

[claude]

Pull Request Review: Debug Frontend UI for Trading Strategies

Summary

This PR adds a comprehensive debugging dashboard with a FastAPI backend and Next.js frontend for monitoring prediction market trading strategies. The implementation is solid overall, but there are several areas that need attention based on the repository's CLAUDE.md guidelines.

Critical Issues

1. Violates Single Source of Truth Principle (CLAUDE.md #4)

Location: api/server.py:98-99, .env.example

The code relies heavily on environment variables for configuration, which directly violates CLAUDE.md guideline #4: "DO NOT place many variables in .env file. Place them in the code instead."

Current:

private_key = os.getenv('POLYMARKET_PRIVATE_KEY')
funder = os.getenv('POLYMARKET_FUNDER')

Recommendation: Consider a configuration class with defaults in code, only using env vars for truly sensitive data like private keys.

2. Deprecated FastAPI Event Handler

Location: api/server.py:117

@app.on_event("startup")

This syntax is deprecated in FastAPI. Should use lifespan context manager instead.

Recommended fix:

from contextlib import asynccontextmanager

@asynccontextmanager
async def lifespan(app: FastAPI):
    init_exchanges()
    yield
    # cleanup if needed

app = FastAPI(lifespan=lifespan, ...)

3. Missing Test Coverage

Finding: No tests added for new API endpoints or frontend components

Per CLAUDE.md #5 ("Run and Debug yourself PROACTIVELY"), this feature should include tests. The existing test suite has Python tests but no coverage for:

• API endpoint responses
• Frontend component rendering
• Integration between frontend and backend

4. Unnecessary Documentation Files (CLAUDE.md #2)

Location: FRONTEND_README.md (170 lines), frontend/README.md

CLAUDE.md #2 explicitly states: "DO NOT create a new document." The PR adds two documentation files when this could be consolidated into the main README or removed entirely.

Security Concerns

5. Overly Permissive CORS Configuration

Location: api/server.py:81-86

allow_origins=["*"],
allow_credentials=True,

This allows any origin with credentials, which is a security risk. For a debug tool, this should be restricted to localhost origins.

Recommendation:

allow_origins=["http://localhost:3000", "http://127.0.0.1:3000"],
allow_credentials=True,

6. No Input Validation on API Parameters

Location: api/server.py:226

async def get_markets(exchange_id: str | None = None, limit: int = 20):

The limit parameter has no validation. A malicious user could pass extremely large values causing performance issues.

Recommendation:

from fastapi import Query

async def get_markets(
    exchange_id: str | None = None, 
    limit: int = Query(default=20, ge=1, le=100)
):

7. Sensitive Data Exposure in Error Messages

Location: api/server.py:160-163

result.append(BalanceResponse(
    exchange=exchange_id,
    balances={"error": str(e)}
))

Error messages are returned directly to clients, potentially exposing internal details.

Code Quality Issues

8. Global Mutable State

Location: api/server.py:90

exchanges = {}

Global mutable dictionary is an anti-pattern. Should use dependency injection or FastAPI's dependency system.

9. Inefficient Re-renders in Frontend

Location: frontend/app/page.tsx:15

const [refreshInterval, setRefreshInterval] = useState(5000)

The refreshInterval state variable is defined with a setter but never changes. This adds unnecessary complexity.

10. Missing Error Handling in Frontend

Location: frontend/lib/api.ts:11-18

The fetchAPI function throws errors but the React components don't display them to users. The useApiData hook captures errors but they're never shown in the UI.

11. Potential Division by Zero

Location: frontend/components/positions-table.tsx:35

({((totalPnL / (totalValue - totalPnL)) * 100).toFixed(2)}%)

If totalValue === totalPnL, this results in division by zero, causing NaN to be displayed.

12. Unused State Setter

Location: frontend/app/page.tsx:15

setRefreshInterval is never used. Remove it or implement the functionality.

Performance Considerations

13. No Request Caching or Rate Limiting

The API has no caching layer or rate limiting. With a 5-second auto-refresh on the frontend, this could hammer external APIs unnecessarily.

Recommendation: Implement response caching with appropriate TTLs, especially for markets data.

14. Fetching All Markets Without Pagination

Location: api/server.py:234

markets = exchange.fetch_markets()
# Limit the number of markets to avoid overwhelming the frontend
for market in markets[:limit]:

This fetches ALL markets then slices. Should fetch only what's needed from the source.

15. No Loading States Optimization

All data is fetched in parallel on mount, but there's no skeleton loading or optimistic UI. Users see "Loading..." text which provides poor UX.

Best Practices Violations

16. Inconsistent Error Handling

Some endpoints silently swallow errors (positions, orders, markets) while others return error objects (balances). This inconsistency makes debugging harder.

17. Missing Type Safety in Backend

Location: api/server.py:8

from typing import List, Dict, Any

Any type is imported but should be avoided. All types should be explicit.

18. Hardcoded Magic Numbers

Location: api/server.py:226, frontend/app/page.tsx:38

Values like 20 for market limit and 5000 for refresh interval should be named constants.

Positive Aspects

1. Clean separation of concerns between frontend and backend
2. Proper use of Pydantic models for API responses
3. Good component structure in the frontend
4. Responsive UI with shadcn/ui components
5. Reasonable default behavior when no exchanges are configured

Recommendations Summary

Must Fix (Before Merge):

1. Address CLAUDE.md violation fix: polymarket impl #4 - reduce .env dependency
2. Fix deprecated FastAPI event handler
3. Add CORS restrictions
4. Remove unnecessary documentation files per CLAUDE.md add logo #2
5. Fix potential division by zero bug

Should Fix (High Priority):
6. Add input validation to API endpoints
7. Add basic test coverage
8. Implement proper error display in frontend
9. Replace global state with dependency injection

Nice to Have (Lower Priority):
10. Add request caching
11. Implement pagination properly
12. Improve loading states
13. Add rate limiting

Overall Assessment

This is a well-structured feature that provides valuable debugging capabilities. However, it needs modifications to align with the repository's guidelines (particularly CLAUDE.md) and address several security/quality concerns before merging. The core functionality is sound, but the implementation details need refinement.

Recommendation: Request changes before merge.