Skip to content

fix(desktop): prevent composer draft leak on new session - #62586

Open
pushpitkamboj wants to merge 2 commits into
NousResearch:mainfrom
pushpitkamboj:fix/desktop-composer-draft-new-session
Open

fix(desktop): prevent composer draft leak on new session#62586
pushpitkamboj wants to merge 2 commits into
NousResearch:mainfrom
pushpitkamboj:fix/desktop-composer-draft-new-session

Conversation

@pushpitkamboj

Copy link
Copy Markdown

What does this PR do?

Fixes a Desktop composer regression where a draft from the previously opened session could briefly appear in a newly created session.

The composer draft swap was running in a passive useEffect, so React could paint the new-session view before the composer restored the __new__ draft. This allowed the previous session's contentEditable text to appear in the new session for a frame and could make the first Enter feel ignored while the composer state was still catching up.

This PR moves the per-session draft swap to useLayoutEffect, which stashes the outgoing draft and loads the incoming draft before paint.

Related Issue

Fixes #62490

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Updated apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts
    • Changed the per-thread composer draft swap from useEffect to useLayoutEffect.
    • Added a comment explaining why the swap must happen before paint.
  • Added apps/desktop/src/app/chat/composer/hooks/use-composer-draft.test.tsx
    • Covers switching from an existing session draft to the new-session scope.
    • Verifies the new-session editor is blank and the previous draft remains scoped to the original session.

How to Test

  1. Open an existing Desktop session.
  2. Type text into the composer without sending it.
  3. Create a new session.
  4. Verify the new session composer is empty and does not show the previous session's draft.
  5. Type a new message and press Enter; it should submit on the first press.

Automated checks run:

npm --workspace apps/desktop run test:ui -- src/app/chat/composer/hooks/use-composer-draft.test.tsx src/app/chat/composer/composer-utils.test.ts --reporter=dot
npm --workspace apps/desktop run test:ui -- src/app/chat/composer/enter-submit-dom-race.test.tsx --reporter=dot
npm --workspace apps/desktop run typecheck
git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 11, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the focused Desktop fix. Current main still performs the draft restore in a passive effect at apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts:287-301, so moving this particular DOM restore to useLayoutEffect is aligned with the reported pre-paint flash.

Problems

  • apps/desktop/src/app/chat/composer/hooks/use-composer-draft.test.tsx:86 checks after flushSync returns. That establishes the eventual swap, but does not demonstrate that a passive useEffect cannot paint the old draft first.
  • #62490 also reports an initial Enter failure. The real Enter branch is apps/desktop/src/app/chat/composer/index.tsx:558-589; the added hook-only test does not exercise that path.

Suggested changes

  • Add a deterministic pre-paint regression test (or focused Electron/browser coverage) that fails under useEffect.
  • If this PR is intended to close all of #62490, add a session-switch plus real-Enter integration regression.

Automated hermes-sweeper review.


it('loads the new-session draft before paint when leaving an existing session', () => {
stashSessionDraft('session-a', 'previous draft', [])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assertion observes the editor only after flushSync returns, so it proves the eventual swap but not that the previous draft could not be painted first. Please add deterministic pre-paint coverage (or a focused Electron/browser regression) that fails with useEffect.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@teknium1 waiting for the update

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: Composer draft leaks across new sessions + first Enter ignored after #54527 fix

3 participants