refactor: Code cleanup and deduplication#20
Conversation
- Extract magic values to constants (SWR intervals, panel dimensions, URL expiry) - Create shared ChatMessage/ChatSession types in lib/types/chat.ts - Create reusable BaseModal component for collection modals - Consolidate useUserId hook pattern in useApi.ts - Remove unused code: cache.ts, QueryBuilder, validation helpers, context exports - Fix duplicate CSS rules in globals.css - Update RAG config to use centralized constants
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PoshanP
left a comment
There was a problem hiding this comment.
🔴 CRITICAL ISSUES
- Security: Dangerous sanitizeInput Function Still in Production Use
Location: lib/validation/index.ts:1-18, used in app/api/papers/upload/route.ts:15-17
// This is fundamentally broken security:
const sqlPatterns = /(\b(SELECT|INSERT|UPDATE|DELETE|DROP|UNION|ALTER|CREATE)\b)/gi
let sanitized = input.replace(sqlPatterns, '')
Problems:
- Corrupts legitimate user data: A paper titled "Natural Language Processing for SELECT Query Optimization" becomes "Natural Language Processing for Query Optimization"
- Blacklist approach is bypassable: Unicode tricks, encoding, case variations can bypass this
- False sense of security: SQL injection should be prevented via parameterized queries (which Supabase already does!)
- Still actively used in the upload API for title sanitization
Recommendation: Remove this sanitization entirely. Supabase uses parameterized queries which prevent SQL injection. This code only corrupts user input.
- Accessibility: BaseModal Lacks Critical WCAG Requirements
Location: frontend/components/collections/BaseModal.tsx
Missing:
- role="dialog" and aria-modal="true" attributes
- Focus trap: Users can Tab outside the modal to elements behind the backdrop
- Focus management: Focus doesn't move to modal on open or return on close
- Screen reader announcements: No aria-live or focus on title for announcements
This affects all three modal components using BaseModal.
🟠 SIGNIFICANT ISSUES
- Type Safety Escape Hatches
Location: lib/db/index.ts:37
.insert(chunk as any) // Type safety bypassed
The batchInsert method uses as any which defeats TypeScript's purpose.
- Inconsistent Type Definitions
- lib/types/chat.ts defines ChatMessage with session_id?: string (optional)
- But in actual usage, messages always have a session_id
- lib/types/index.ts still has Zod schemas (ChatMessageSchema) that duplicate these types
- Creates confusion about single source of truth
- validateFileName Allows Non-PDF Extensions
Location: lib/validation/index.ts:34-35
const allowedExtensions = ['.pdf', '.jpg', '.jpeg', '.png', '.webp']
This is a PDF-only app per CLAUDE.md. Why allow image extensions?
🟡 CODE QUALITY CONCERNS
- chat-new/page.tsx is 1014 Lines
This monolithic component handles:
- Session CRUD
- Message CRUD
- PDF viewing
- Responsive layouts
- URL parameters
- Processing status polling
- Summary generation
- Resize handling
- Keyboard navigation
- DOM manipulation (hiding navbar via useEffect)
Should be split into multiple components/hooks.
Summary
ChatMessage/ChatSessiontypes to eliminate duplicate definitionsBaseModalcomponent for collection modals, reducing boilerplateuseUserIdhook pattern across data fetching hookscache.ts,QueryBuilder, validation helpers, context exportsglobals.cssTest plan