mosaic/stack
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases 2 Wiki Activity

🟠 [P1] Fix high-priority error handling in chat overlay (PR #307) #322

New Issue
Closed
opened 2026-02-04 13:11:32 +00:00 by jason.woltje · 1 comment
jason.woltje commented 2026-02-04 13:11:32 +00:00
Owner
Copy Link

Overview

Code review of PR #307 identified 4 HIGH severity error handling issues that need attention before the next release.

Source: Code review by pr-review-toolkit agents
Related PR: #307
Impact: MEDIUM-HIGH - Poor user experience, debugging difficulties

Issue 1: Silent localStorage Failures

Location: apps/web/src/hooks/useChatOverlay.ts (lines 42-44, 59-61)

Problem:
Both loadState() and saveState() catch all errors, log warnings, then silently continue. Users have no idea their chat state isn't being persisted.

Hidden Errors:

  • QuotaExceededError (localStorage full)
  • SecurityError (private browsing, third-party cookies blocked)
  • InvalidStateError (storage disabled)

User Impact:

  • Chat state mysteriously doesn't persist
  • No explanation why minimized state doesn't save
  • Confusion in private browsing mode

Fix:

  • Show user-facing toast when persistence fails
  • Add specific error context to logging
  • Show persistent banner if localStorage unavailable

Issue 2: Unhandled Promise Rejections

Location: apps/web/src/components/chat/Chat.tsx (lines 99-100, 177-179)

Problem:
loadConversation() returns Promise that could reject, but imperative handle exposes it without error handling.

Hidden Errors:

  • Network failures
  • Backend API errors (500, 503)
  • Authentication failures
  • Rate limiting

User Impact:

  • Unhandled promise rejections in console
  • No user feedback when loading fails
  • App may crash with error boundaries

Fix:

useImperativeHandle(ref, () => ({
  loadConversation: async (conversationId: string): Promise<void> => {
    try {
      await loadConversation(conversationId);
    } catch (error) {
      console.error("Failed to load conversation", { error, conversationId });
      throw error;
    }
  },
  // ...
}));

Issue 3: No Separate Save Failure Handling

Location: apps/web/src/hooks/useChat.ts (line 204)

Problem:
saveConversation() errors get lumped with LLM errors, making it unclear what failed.

User Impact:

  • Users think message send failed when only save failed
  • Message appears in UI but isn't persisted
  • Confusing error messages

Fix:
Separate LLM errors from persistence errors with different user messages.

Issue 4: No Timeout Handling

Location: apps/web/src/hooks/useChat.ts (lines 146-236)

Problem:
sendMessage calls API without timeout. If API hangs, loading state persists forever.

User Impact:

  • Loading indicator spins forever
  • Can't send new messages
  • Must refresh to recover

Fix:
Add AbortController with 60-second timeout.

Acceptance Criteria

  • localStorage failures show user-facing notifications
  • Promise rejections properly handled with try-catch
  • Save failures separated from LLM errors
  • API calls have timeout protection
  • All error scenarios tested
  • Error messages are clear and actionable

Priority

P1 - HIGH - Fix before next release.

## Overview Code review of PR #307 identified 4 HIGH severity error handling issues that need attention before the next release. **Source:** Code review by pr-review-toolkit agents **Related PR:** #307 **Impact:** MEDIUM-HIGH - Poor user experience, debugging difficulties --- ## Issue 1: Silent localStorage Failures **Location:** `apps/web/src/hooks/useChatOverlay.ts` (lines 42-44, 59-61) **Problem:** Both `loadState()` and `saveState()` catch all errors, log warnings, then silently continue. Users have no idea their chat state isn't being persisted. **Hidden Errors:** - QuotaExceededError (localStorage full) - SecurityError (private browsing, third-party cookies blocked) - InvalidStateError (storage disabled) **User Impact:** - Chat state mysteriously doesn't persist - No explanation why minimized state doesn't save - Confusion in private browsing mode **Fix:** - Show user-facing toast when persistence fails - Add specific error context to logging - Show persistent banner if localStorage unavailable --- ## Issue 2: Unhandled Promise Rejections **Location:** `apps/web/src/components/chat/Chat.tsx` (lines 99-100, 177-179) **Problem:** `loadConversation()` returns Promise that could reject, but imperative handle exposes it without error handling. **Hidden Errors:** - Network failures - Backend API errors (500, 503) - Authentication failures - Rate limiting **User Impact:** - Unhandled promise rejections in console - No user feedback when loading fails - App may crash with error boundaries **Fix:** ```typescript useImperativeHandle(ref, () => ({ loadConversation: async (conversationId: string): Promise<void> => { try { await loadConversation(conversationId); } catch (error) { console.error("Failed to load conversation", { error, conversationId }); throw error; } }, // ... })); ``` --- ## Issue 3: No Separate Save Failure Handling **Location:** `apps/web/src/hooks/useChat.ts` (line 204) **Problem:** `saveConversation()` errors get lumped with LLM errors, making it unclear what failed. **User Impact:** - Users think message send failed when only save failed - Message appears in UI but isn't persisted - Confusing error messages **Fix:** Separate LLM errors from persistence errors with different user messages. --- ## Issue 4: No Timeout Handling **Location:** `apps/web/src/hooks/useChat.ts` (lines 146-236) **Problem:** `sendMessage` calls API without timeout. If API hangs, loading state persists forever. **User Impact:** - Loading indicator spins forever - Can't send new messages - Must refresh to recover **Fix:** Add AbortController with 60-second timeout. --- ## Acceptance Criteria - [ ] localStorage failures show user-facing notifications - [ ] Promise rejections properly handled with try-catch - [ ] Save failures separated from LLM errors - [ ] API calls have timeout protection - [ ] All error scenarios tested - [ ] Error messages are clear and actionable ## Priority **P1 - HIGH** - Fix before next release.
jason.woltje added this to the M4-LLM (0.0.4) milestone 2026-02-04 13:11:32 +00:00
jason.woltje commented 2026-02-04 14:10:05 +00:00
Author
Owner
Copy Link

Fixed in commit 8d831cf.

All 4 high-priority issues addressed:

  1. Unhandled promise rejections - Added try-catch with context logging
  2. Save failure handling - Separated LLM errors from persistence errors
  3. Timeout handling - Added 60s AbortController timeout
  4. localStorage notifications - Created toast system with user-friendly messages

Tests: 568 passing

Fixed in commit 8d831cf. All 4 high-priority issues addressed: 1. Unhandled promise rejections - Added try-catch with context logging 2. Save failure handling - Separated LLM errors from persistence errors 3. Timeout handling - Added 60s AbortController timeout 4. localStorage notifications - Created toast system with user-friendly messages Tests: 568 passing
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai
AI/LLM integration
 api
Backend/API work
 api
auth
Authentication
 database
database
Database/schema work
 devops
Docker/deployment
 docs
Documentation
 frontend
graph
knowledge-module
migration
Data migration
 orchestrator
Orchestrator service (apps/orchestrator/)
 p0
Critical priority
 p1
High priority
 p2
Medium priority
 p3
Low priority
 performance
Performance work
 phase-1
phase-2
phase-3
phase-4
phase-5
plugin
MoltBot plugin work
 search
security
Security-related
 setup
Initial setup
 testing
Testing/QA
 web
Frontend work
No Label
Milestone
No items
No Milestone M4-LLM (0.0.4)
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaic/stack#322
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?