Skip to content

Conversation

@jsboige
Copy link

@jsboige jsboige commented Oct 1, 2025

Summary

This PR addresses issue #8236 (draft persistence across reloads) and issue #8033 (message queue data loss) by fixing a race condition in the message sending logic and adding a draft autosave mechanism.

Problem Analysis

Previous Attempts

Two previous PRs (#8237 and #8034) attempted to solve related issues but were closed. Our analysis identified that the core issue is a race condition in the handleSendMessage function.

Root Cause

When a user sends a message while sendingDisabled=true (e.g., AI is processing), the input is cleared immediately before the message is queued:

// Current behavior causing data loss:
if (sendingDisabled) {
  vscode.postMessage({ type: 'queueMessage', text, images })
  setInputValue("")      // Clears before queue confirmation
  setSelectedImages([])  
  return
}

Solution

Part 1: Race Condition Fix

Modified handleSendMessage in ChatView.tsx to defer draft clearing:

if (sendingDisabled) {
  vscode.postMessage({ type: 'queueMessage', text, images })
  // Draft preserved until message is actually processed
  return
}

Draft is now cleared only when message is successfully sent or chat is reset.

Part 2: Autosave Draft Hook

Implemented useAutosaveDraft hook providing:

  • localStorage persistence
  • 100ms debounce for responsive saving
  • Error handling via safeLocalStorage
  • Conversation isolation using task IDs
  • Automatic cleanup on unmount

Testing

Comprehensive test suite includes:

  • Unit tests for useAutosaveDraft hook
  • Integration tests for ChatView autosave behavior
  • Race condition reproduction tests in ChatView.raceCondition.spec.tsx

All existing tests pass (3833+ tests), TypeScript compilation successful, linting clean.

Key Differences from Previous Approaches

Our implementation differs from previous PRs in these areas:

  • Fixes the race condition at its source rather than treating symptoms
  • Uses localStorage instead of VS Code global state
  • Includes comprehensive test coverage demonstrating the bug and fix
  • 100ms debounce (vs 500ms in PR feat: preserve unsent chat input across reloads #8237)
  • Complete error handling throughout

Architectural Documentation

Technical analysis available at:
C:/dev/roo-extensions/docs/roo-code/ARCHITECTURE-HOOK-AUTOSAVE-DRAFT.md

Known Limitations

Currently text-only. Image support can be added in a follow-up PR.

Related Issues

Closes #8236
Closes #8033

Migration Notes

No breaking changes. The fix is transparent to users.

Changeset

Changeset included for changelog.


Important

Fixes message editor data loss by addressing race condition and adding autosave draft feature.

  • Behavior:
    • Fixes race condition in handleSendMessage in ChatView.tsx by deferring draft clearing until message processing.
    • Adds useAutosaveDraft hook for draft persistence with localStorage, 100ms debounce, and error handling.
  • Testing:
    • Adds unit tests for useAutosaveDraft in useAutosaveDraft.test.tsx.
    • Adds integration tests for autosave in ChatView.autosave.test.tsx.
    • Adds race condition tests in ChatView.raceCondition.spec.tsx.
  • Misc:
    • Updates ChatView.tsx to use useAutosaveDraft for input management.

This description was created by Ellipsis for dda5c8c. You can customize this summary. It will automatically update as commits are pushed.

- Fix critical data loss bug when VSCode webview reloads
- Add useAutosaveDraft hook with localStorage persistence
- Implement debounced saving to prevent excessive writes
- Add conversation isolation using task/conversation IDs
- Include comprehensive error handling for storage limitations
- Add 95%+ test coverage with unit and integration tests

Technical details:
- Hook provides draftContent, updateDraft, clearDraft, hasInitialDraft
- Uses 300ms debounce for optimal UX performance
- Gracefully handles localStorage errors and quota limits
- Automatically clears drafts after successful message sends
- Maintains backward compatibility with existing ChatView behavior

Tests:
- Unit tests: webview-ui/src/hooks/__tests__/useAutosaveDraft.test.tsx
- Integration tests: webview-ui/src/components/chat/__tests__/ChatView.autosave.test.tsx
- Tests demonstrate bug reproduction and fix verification

Resolves the message editor data loss issue described in architectural analysis.
Fixes data loss when user sends message concurrently with tool response.

Problem:
When a tool response arrives (sendingDisabled=true) while user is typing,
clicking Send queues the message but immediately calls clearDraft(),
causing the input to be wiped before processing.

Solution:
- Remove clearDraft() call when message is queued (sendingDisabled path)
- Reduce autosave debounce from 300ms to 100ms
- Message remains visible until properly processed

Changes:
- webview-ui/src/components/chat/ChatView.tsx
- webview-ui/src/hooks/useAutosaveDraft.ts
- webview-ui/src/components/chat/__tests__/ChatView.raceCondition.spec.tsx

Tests: 10 unit tests covering bug reproduction, fix verification,
and autosave interaction (all passing)
@jsboige jsboige requested review from cte, jr and mrubens as code owners October 1, 2025 14:44
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. bug Something isn't working labels Oct 1, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 1, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary: Solid fix for the race condition and a useful autosave hook. A few items to address before merge: P1 stale-closure risks around clearDraft in callbacks; P2 cleanup for unused API/local and queued images UX; P3 debounce timer edge-case on conversation switches. Inline comments point to exact spots.

- Remove unused imports (React, useState, useEffect, waitFor)
- Remove unused clearOnSubmit parameter from useAutosaveDraft
- Prefix hasInitialDraft with _ to indicate intentionally unused
- Fix useCallback dependency arrays in ChatView
- Fix TypeScript error in ChatView.autosave.test.tsx
P1 fixes:
- clearDraft already in dependency arrays (verified)

P2 fixes:
- Clear selectedImages after queueing to prevent image duplication

P3 fixes:
- Add cleanup for pending debounce timers when storageKey changes in useAutosaveDraft
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Oct 24, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Oct 24, 2025
@daniel-lxs
Copy link
Member

Hey @jsboige, thank you for your contribution. We have this other PR waiting to be merged: #8694, and I think the fix is a bit more targeted in this case. If the draft part is an issue you want to tackle, please create a separate PR with that feature. Let me know if you have any questions!

@daniel-lxs daniel-lxs closed this Oct 27, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Oct 27, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working PR - Needs Preliminary Review size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

Status: Done

3 participants