fix unsafe error #147
fix unsafe error #147
Conversation
📝 WalkthroughWalkthroughAdds SSR-safe storage helpers and a safe JSON parser; refactors many frontend components, hooks, and services to use those utilities instead of direct localStorage/JSON.parse; and adds production backend config files with placeholder secrets, JWT expiry, and other environment settings. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
frontend/src/utils/storage.ts (1)
12-26: Consider runtime type validation for stronger safety.The type assertion
as Tat line 16 provides no runtime guarantee that the parsed JSON matches the expected typeT. If the stored data structure has changed or is corrupted, this could lead to runtime errors downstream when code assumes certain properties exist.For critical data types, consider adding runtime validation (e.g., using Zod or a custom validator) to verify the structure matches
Tbefore returning.That said, the fallback mechanism and corrupt item removal provide good resilience against parse failures.
frontend/src/hooks/useUser.ts (1)
20-29: Prefer getLocalJSON for consistency.Lines 20-27 use
getLocalStringfollowed by manualJSON.parse, which duplicates the logic already provided bygetLocalJSON. This is inconsistent with the pattern used in other files (e.g., AdminDashboard.tsx line 369).Consider refactoring to:
🔎 Proposed refactor
useEffect(() => { if (!user) { - const cachedUser = getLocalString(USER_CACHE_KEY); - if (cachedUser) { - try { - const parsedUser = JSON.parse(cachedUser); - setUser(parsedUser); - } catch (error) { - console.error("Failed to parse cached user profile:", error); - try { localStorage.removeItem(USER_CACHE_KEY); } catch {} - } - } + const cachedUser = getLocalJSON(USER_CACHE_KEY); + if (cachedUser) { + setUser(cachedUser); + } } }, [user, setUser]);This leverages the built-in error handling and corrupt item removal already present in
getLocalJSON.frontend/src/utils/auth.ts (1)
1-1: Clean up unused import.The import includes
setLocalJSONbut it's not used anywhere in this file. Consider removing it to keep imports clean.🔎 Proposed cleanup
-import { setLocalString, getLocalString, setLocalJSON } from './storage'; +import { setLocalString, getLocalString } from './storage';frontend/src/utils/safeParse.ts (1)
8-8: Consider upgrading console.warn to include the raw value for debugging.The warning logs the error but not the problematic input, making debugging harder in production.
📝 Suggested improvement
} catch (err) { - console.warn('safeParse failed to parse JSON:', err); + console.warn('safeParse failed to parse JSON:', err, 'Raw value:', typeof raw === 'string' ? raw.substring(0, 100) : raw); return fallback; }frontend/src/components/ChatRoom.tsx (1)
62-63: Consider using a typed interface instead ofanyfor better type safety.The
safeParse<any>call loses type information. Define a message interface to catch structural errors at compile time:📝 Suggested improvement
interface ChatRoomMessage { type: 'chatMessage' | 'notification' | 'reaction' | 'vote' | 'presence'; username?: string; content?: string; timestamp?: number; extra?: { reaction?: string; option?: VoteOption; }; count?: number; } // Then use: const data = safeParse<ChatRoomMessage>(event.data, null);frontend/src/components/TeamChatSidebar.tsx (1)
62-63: Use a typed interface instead ofanyfor WebSocket messages.Same concern as in ChatRoom.tsx - using
anydefeats the purpose of TypeScript's type checking. Consider defining:📝 Suggested improvement
interface TeamChatMessage { type: string; messageId?: string; userId: string; email?: string; username?: string; content?: string; message?: string; timestamp?: string; } const data = safeParse<TeamChatMessage>(event.data, null);frontend/src/Pages/OnlineDebateRoom.tsx (1)
727-727: Simplify redundant ternary check.Since
storageKeyis always defined from the template literal on line 726, the ternary check is unnecessary.🔎 Proposed simplification
- const stored = storageKey ? getLocalString(storageKey) : null; + const stored = getLocalString(storageKey);frontend/src/Pages/DebateRoom.tsx (1)
229-260: Use safeParse for consistency with PR objectives.While the try-catch block handles JSON parsing errors, using
JSON.parsedirectly (line 232) is inconsistent with the PR's goal of replacing all direct JSON.parse calls withsafeParse. This would make the error handling more uniform across the codebase.🔎 Proposed refactor to use safeParse
const [state, setState] = useState<DebateState>(() => { const savedState = getLocalString(debateKey); if (savedState) { - try { - const parsed = JSON.parse(savedState) as Partial<DebateState> | null; - if (parsed && typeof parsed === 'object') { - return { - messages: parsed.messages ?? [], - currentPhase: parsed.currentPhase ?? 0, - phaseStep: parsed.phaseStep ?? 0, - isBotTurn: parsed.isBotTurn ?? false, - userStance: parsed.userStance ?? "", - botStance: parsed.botStance ?? "", - timer: parsed.timer ?? phases[0].time, - isDebateEnded: parsed.isDebateEnded ?? false, - } as DebateState; - } - } catch (err) { - console.warn('Failed to parse saved debate state, clearing corrupt value', err); - try { localStorage.removeItem(debateKey); } catch {} - } + const parsed = safeParse<Partial<DebateState>>(savedState, null); + if (parsed && typeof parsed === 'object') { + return { + messages: parsed.messages ?? [], + currentPhase: parsed.currentPhase ?? 0, + phaseStep: parsed.phaseStep ?? 0, + isBotTurn: parsed.isBotTurn ?? false, + userStance: parsed.userStance ?? "", + botStance: parsed.botStance ?? "", + timer: parsed.timer ?? phases[0].time, + isDebateEnded: parsed.isDebateEnded ?? false, + } as DebateState; + } else { + console.warn('Failed to parse saved debate state, clearing corrupt value'); + try { localStorage.removeItem(debateKey); } catch {} + } } return { messages: [], currentPhase: 0, phaseStep: 0, isBotTurn: false, userStance: "", botStance: "", timer: phases[0].time, isDebateEnded: false, }; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
backend/config/config.prod.sample.ymlbackend/config/config.prod.ymlfrontend/src/Pages/Admin/AdminDashboard.tsxfrontend/src/Pages/CommunityFeed.tsxfrontend/src/Pages/DebateRoom.tsxfrontend/src/Pages/Game.tsxfrontend/src/Pages/Leaderboard.tsxfrontend/src/Pages/OnlineDebateRoom.tsxfrontend/src/Pages/StrengthenArgument.tsxfrontend/src/Pages/TeamDebateRoom.tsxfrontend/src/Pages/ViewDebate.tsxfrontend/src/components/ChatRoom.tsxfrontend/src/components/Matchmaking.tsxfrontend/src/components/TeamChatSidebar.tsxfrontend/src/context/theme-provider.tsxfrontend/src/hooks/useDebateWS.tsfrontend/src/hooks/useUser.tsfrontend/src/services/gamificationService.tsfrontend/src/services/teamDebateService.tsfrontend/src/utils/auth.tsfrontend/src/utils/safeParse.tsfrontend/src/utils/storage.ts
🧰 Additional context used
🧬 Code graph analysis (15)
frontend/src/Pages/Admin/AdminDashboard.tsx (2)
frontend/src/utils/storage.ts (2)
getLocalString(1-10)getLocalJSON(12-26)frontend/src/services/adminService.ts (1)
Admin(3-8)
frontend/src/hooks/useDebateWS.ts (3)
frontend/src/utils/storage.ts (2)
getLocalString(1-10)setLocalString(37-44)frontend/src/utils/safeParse.ts (1)
safeParse(1-11)backend/internal/debate/events.go (1)
Event(9-13)
frontend/src/Pages/TeamDebateRoom.tsx (2)
frontend/src/utils/safeParse.ts (1)
safeParse(1-11)frontend/src/utils/storage.ts (1)
getLocalString(1-10)
frontend/src/Pages/Leaderboard.tsx (2)
frontend/src/utils/storage.ts (1)
getLocalString(1-10)frontend/src/services/gamificationService.ts (1)
fetchGamificationLeaderboard(41-55)
frontend/src/Pages/StrengthenArgument.tsx (1)
backend/models/coach.go (2)
WeakStatement(4-9)Evaluation(18-21)
frontend/src/Pages/DebateRoom.tsx (2)
frontend/src/utils/storage.ts (1)
getLocalString(1-10)frontend/src/utils/safeParse.ts (1)
safeParse(1-11)
frontend/src/components/ChatRoom.tsx (1)
frontend/src/utils/safeParse.ts (1)
safeParse(1-11)
frontend/src/components/TeamChatSidebar.tsx (1)
frontend/src/utils/safeParse.ts (1)
safeParse(1-11)
frontend/src/hooks/useUser.ts (1)
frontend/src/utils/storage.ts (2)
getLocalString(1-10)setLocalJSON(28-35)
frontend/src/Pages/CommunityFeed.tsx (1)
frontend/src/utils/storage.ts (1)
getLocalString(1-10)
frontend/src/Pages/OnlineDebateRoom.tsx (2)
frontend/src/utils/safeParse.ts (1)
safeParse(1-11)frontend/src/utils/storage.ts (1)
getLocalString(1-10)
frontend/src/services/gamificationService.ts (1)
backend/models/gamification.go (1)
GamificationEvent(47-55)
frontend/src/components/Matchmaking.tsx (3)
frontend/src/utils/safeParse.ts (1)
safeParse(1-11)backend/websocket/matchmaking.go (1)
MatchmakingMessage(48-56)backend/services/matchmaking.go (1)
MatchmakingPool(16-25)
frontend/src/context/theme-provider.tsx (1)
frontend/src/utils/storage.ts (2)
getLocalString(1-10)setLocalString(37-44)
frontend/src/services/teamDebateService.ts (2)
frontend/src/utils/auth.ts (1)
getAuthToken(7-9)frontend/src/utils/storage.ts (1)
getLocalString(1-10)
🪛 Gitleaks (8.30.0)
backend/config/config.prod.yml
[high] 10-10: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
backend/config/config.prod.sample.yml
[high] 10-10: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
🔇 Additional comments (27)
backend/config/config.prod.sample.yml (1)
1-43: Configuration changes are unrelated to PR objectives.The PR objectives state this PR is about "fixing unsafe error handling for persisted and incoming JSON to prevent runtime crashes" in the frontend, but these backend configuration changes are unrelated. Consider:
- Moving these config changes to a separate PR
- Explaining why backend config updates are included in a frontend safety PR
frontend/src/Pages/StrengthenArgument.tsx (1)
128-136: Excellent defensive parsing and validation.This implementation follows best practices by:
- Safely handling JSON.parse failures
- Validating all required fields before use
- Providing clear error messages
This pattern effectively prevents runtime crashes from malformed data.
frontend/src/utils/storage.ts (3)
1-10: LGTM: Well-designed SSR-safe wrapper.The function correctly handles SSR environments, localStorage access errors, and null values with appropriate fallback behavior.
28-35: LGTM: Consistent error handling.The function correctly handles SSR environments and localStorage quota/access errors with appropriate logging.
37-44: LGTM: Consistent pattern with setLocalJSON.The function provides the same SSR-safety and error handling as the JSON variant.
frontend/src/services/teamDebateService.ts (1)
8-12: LGTM: Consistent adoption of storage utilities.The migration to
getLocalStringaligns with the PR's goal of centralizing storage access with SSR-safety and error handling. The empty string fallback maintains the function's contract.frontend/src/context/theme-provider.tsx (1)
2-2: LGTM: Clean migration to storage utilities.The theme persistence logic correctly uses the new storage helpers while maintaining the existing behavior of converting the enum value to a string for storage.
Also applies to: 31-31, 59-59
frontend/src/Pages/CommunityFeed.tsx (1)
3-3: LGTM: Consistent token retrieval pattern.All token accesses now use the safe storage wrapper, maintaining existing null checks and error handling. The migration is thorough and consistent across all three usage sites in the file.
Also applies to: 282-282, 360-360, 406-406
frontend/src/Pages/Admin/AdminDashboard.tsx (2)
21-21: LGTM: Clean migration to typed storage utilities.The admin data retrieval now uses
getLocalJSON<Admin>which handles both parsing and type hinting, eliminating the need for manualJSON.parsecalls. The token retrieval usesgetLocalStringconsistently. All existing null checks are preserved.Also applies to: 368-376
508-513: LGTM: Fallback token retrieval for refresh.The refresh handler correctly falls back to
getLocalString("adminToken")if the component state token is null, ensuring the refresh functionality works even if state hasn't been initialized yet.frontend/src/hooks/useUser.ts (1)
91-91: LGTM: Correct usage of storage utilities.Line 91 correctly uses
setLocalJSONto persist the user cache, and lines 110-111 properly usegetLocalStringfor token checks in authentication state derivation.Also applies to: 110-111
frontend/src/utils/auth.ts (2)
3-9: LGTM: Consistent token storage and retrieval.Both
setAuthTokenandgetAuthTokencorrectly use the storage utilities, providing SSR-safety and error handling for token operations.
11-15: LGTM: Appropriate error handling for removal.The manual
localStorage.removeItemwith try/catch is appropriate since there's no dedicated removal helper in the storage utilities. The silent error swallowing is acceptable for cleanup operations.frontend/src/components/Matchmaking.tsx (1)
73-82: Good use of typed interfaces with safeParse.This is an excellent example of using
safeParsewith proper TypeScript types. TheMatchmakingMessageinterface provides compile-time safety whilesafeParseprovides runtime safety. The nested parse on line 81 appropriately handles thepoolfield which can be either a pre-parsed array or a JSON string.frontend/src/Pages/TeamDebateRoom.tsx (3)
737-741: Excellent use of typed safeParse with proper null handling.This is a model implementation showing:
- Typed interface
WSMessagefor compile-time safetysafeParsefor runtime safety- Proper null check and early return
- Clear warning message for debugging
This pattern should be adopted in Game.tsx, ViewDebate.tsx, and gamificationService.ts.
1455-1458: Good use of getLocalString for safe storage access.The migration from
localStorage.getItemtogetLocalStringadds SSR-safety and error handling. The fallback chain (localStorage → speechTranscripts → "No response") provides robust resilience.
1487-1495: Robust error handling for judgment parsing.The safeParse usage with explicit null checking and user-facing error messages demonstrates good defensive programming. The fallback error message on line 1494 ensures users aren't left in an undefined state.
frontend/src/hooks/useDebateWS.ts (4)
16-17: LGTM! Clean utility imports.The new storage and parsing utilities are correctly imported and will be used throughout this file to prevent unsafe access patterns.
80-84: LGTM! Safe localStorage access with proper fallback.The use of
getLocalStringandsetLocalStringprovides robust error handling for localStorage access, and the UUID generation ensures a valid spectatorId is always available.
105-105: LGTM! Consistent safe storage pattern.The triple fallback (
spectatorHash || getLocalString('spectatorHash') || '') ensures the join message always has a defined value while maintaining the existing behavior.
117-121: LGTM! Robust WebSocket message parsing.The use of
safeParsewith a null check and early return prevents crashes from malformed WebSocket messages. The warning log aids debugging without disrupting the connection.frontend/src/Pages/OnlineDebateRoom.tsx (4)
15-15: LGTM! Required utility import.The
safeParseimport is used consistently throughout this file to handle JSON parsing safely.
559-567: LGTM! Safe judgment parsing with proper error handling.The combination of
safeParsewith null checking and user-friendly error messaging ensures the app remains stable even when the backend returns malformed judgment data.
662-665: LGTM! Consistent safe parsing pattern.The null check with warning log maintains consistency with the polling path and allows the caller to handle parse failures gracefully.
1126-1130: LGTM! Robust WebSocket message handling.The
safeParsewith type parameter and early return ensures only valid messages are processed, preventing crashes from malformed or unexpected WebSocket data.frontend/src/Pages/DebateRoom.tsx (2)
2-2: LGTM! Required utility imports.Both
safeParseandgetLocalStringare correctly imported for use in state initialization and judgment parsing.Also applies to: 4-4
585-597: LGTM! Multi-layered parsing strategy with good fallback logic.The judgment parsing uses
safeParsewith a fallback that attempts to fix common JSON formatting issues before retrying. This provides resilience against slightly malformed responses while maintaining consistency with the PR's safe parsing approach.
|
please review and merge it @Zahnentferner, @varunchitre15, @saisankargochhayat, @heychirag |
1 participant
Summary
This PR adds defensive utilities for localStorage access and JSON parsing and replaces unsafe usages across the frontend.
It prevents runtime crashes caused by
nullor malformed persisted data and hardens WebSocket message handling where payloads were previously assumed to be valid.What changed
storage.tsgetLocalStringgetLocalJSONsetLocalString/setLocalJSONsafeParse.tssafeParse(raw, fallback)utility to preventJSON.parsecrashes.localStorage.getItemandJSON.parseusages with defensive wrappers.Representative files updated
theme-provider.tsxuseDebateWS.tsDebateRoom.tsxOnlineDebateRoom.tsxgamificationService.ts(See commit history / branch diff for full list.)
What this fixes
caused by:
JSON.parse(null)
Accessing .length on missing localStorage values
Improves resilience against:
Fresh sessions (empty localStorage)
Corrupted persisted state
Unexpected or malformed WebSocket payloads
Testing / Verification
localStorage.clear()
→ App loads without crashing.
If backend interaction is required, ensure the backend is running normally.
Notes for reviewers
Please focus review on:
frontend/src/utils/storage.ts
frontend/src/utils/safeParse.ts
Call sites migrated to use these helpers
This PR is intentionally defensive and non-breaking and solve this issue #145.
Follow-up work could:
Centralize runtime payload validation
Add telemetry/logging for malformed data paths
I’m happy to sweep remaining files if you’d like this pattern applied more broadly.
Summary by CodeRabbit
Configuration
New Features
Bug Fixes
Refactor / Breaking Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.