Skip to content

fix(desktop): don't drop a new chat's first send when it carries an attachment - #66539

Open
brian717 wants to merge 1 commit into
NousResearch:mainfrom
brian717:fix/desktop-new-chat-image-drop-65733
Open

fix(desktop): don't drop a new chat's first send when it carries an attachment#66539
brian717 wants to merge 1 commit into
NousResearch:mainfrom
brian717:fix/desktop-new-chat-image-drop-65733

Conversation

@brian717

@brian717 brian717 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the first send of a brand-new chat being silently dropped when it carries an attachment (image or file) in remote-gateway mode. The optimistic message disappears, the composer bounces back, no prompt.submit reaches the gateway, and the freshly-minted route ends up 404-ing, with no error surfaced. Text-only sends are unaffected, which is what made this present as "new chats with images just don't start."

The root cause is a race in the submit drift guard. On a new chat, createBackendSessionForSend() re-homes the session refs and calls navigate() to the chat it just created, but the react-router location (and the routeToken the guard reads) only updates on the next render. The pipeline re-pins its drift baseline immediately after create, so it captures the pre-create route token. It then awaits the real attachment upload (image.attach_bytes / file.attach), during which React re-renders and the route token flips to the new session. The post-upload sessionContextDrifted() check compares against the stale token, misreads our own re-home as a user session switch, and takes the silent abortForSessionSwitch() path.

The fix: once a submit has minted its own session, judge drift by the created session id instead of the deferred route token. Every real session switch retargets activeSessionIdRef / selectedStoredSessionIdRef synchronously, so a genuine switch away during the upload is still detected and still aborts; we only stop treating our own late navigate() as drift. Paths that don't create a session are unchanged and keep using the route token.

Related Issue

Fixes #65733

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

  • apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts: track a createdSessionId for the post-create window and make sessionContextDrifted() compare against it (via the synchronously-retargeted session refs) instead of the route token once this submit has created the session.
  • apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx: added two regression tests: a new chat + image in remote mode where the route re-home lands mid-upload (submit must still reach the gateway with the created session id), and a genuine mid-upload session switch (must still abort, so the guard isn't blinded).

How to Test

  1. cd apps/desktop && npx vitest run --project ui src/app/session/hooks/use-prompt-actions/index.test.tsx
  2. Both new tests under new-chat attachment drift (#65733) pass; the full file is green (54/54).
  3. To confirm they actually catch the regression: revert the change in submit.ts and the first new test fails with the submit resolving false (the dropped send). Restore it and it passes.

Manual repro (remote gateway): open Desktop, start a new chat, attach an image, send - before the fix the send is swallowed; after, it goes through and streams a response.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass - N/A (desktop/TypeScript change; verified with the desktop vitest suite instead)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11 (desktop vitest suite)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) - or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys - or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows - or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide - or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior - or N/A

…ttachment

createBackendSessionForSend re-homes the session refs and route to the
chat it mints, but its navigate() only lands on the next React render.
When the submit then awaits an image/file upload, that late re-home flips
the route token mid-flight and sessionContextDrifted() misreads the app's
own re-home as a user session switch — silently aborting the send: the
optimistic message is dropped, no prompt.submit reaches the gateway, no
DB row is written, and the route 404s. Text-only sends win the race
because nothing real is awaited between the re-pin and prompt.submit.

Once this submit has minted the session, judge drift by the created
session id (which every real switch retargets synchronously) instead of
the deferred route token. A genuine switch away during the upload is
still detected and still aborts.

Fixes NousResearch#65733
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #65733 and competing with open #64759: both prevent an app-owned post-create route commit from being treated as a user switch, but this branch validates the created session refs after attachment upload while #64759 uses a broader binding approach. Maintainer choice.

@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 reproduction and regression coverage. The delayed self-navigation race is present on current main: apps/desktop/src/app/session/hooks/use-session-actions/index.ts:307-320 re-homes refs then calls navigate(), while submit.ts:431-443 snapshots the route token, awaits attachment sync, and aborts on the resulting token change.

Problems

  • apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts:194 removes route-token validation for the entire post-create submit window. That also accepts unrelated route-only navigation: openSettings() calls navigate(SETTINGS_ROUTE) at apps/desktop/src/app/session/hooks/use-session-actions/index.ts:416-418 without changing either session ref, so this branch can still dispatch prompt.submit after the user has left chat.

Suggested changes

  • Preserve route validation with an explicit created runtime/stored/route binding. Accept only the expected deferred transition to the created route, and add a Settings-route-during-upload regression test.

This is an automated hermes-sweeper review.

(createdSessionId !== null
? activeSessionIdRef.current !== createdSessionId ||
selectedStoredSessionIdRef.current !== startingStoredSessionId
: selectedStoredSessionIdRef.current !== startingStoredSessionId || getRouteToken() !== startingRouteToken)

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 branch ignores route changes for the rest of the post-create submit window. openSettings() navigates to SETTINGS_ROUTE without changing either session ref (use-session-actions/index.ts:416-418), so an attachment upload completing after that navigation would still submit. Preserve the route check by accepting only the expected created-session route transition.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Three PRs address #65733’s late route-flush race: #66185 adds a post-create routed-session identity check, #66539 relies on created-session refs, and #67812 globally replaces raw route-token drift with stored-session identity.

Related pull requests

  • fix(desktop): stop new-chat attachment sends from drift-aborting #66185 best fix — (+204/-1) — n/a: The diff accepts the delayed route commit only when it resolves to the newly created session (or remains unresolved) and adds regressions for both successful attachment submission and drift to an unrelated session. Despite the recorded best-fix designation and keep_open review, the later main implementation at 1bdd478 supersedes this approach with the shared identity-aware guard in apps/desktop/src/app/session/hooks/session-context-drift.ts:93-115 and coverage at session-context-drift.test.ts:102-112.
  • fix(desktop): don't drop a new chat's first send when it carries an attachment #66539 partial — (+171/-1) — n/a: The diff fixes the self-navigation race by checking active and selected session refs after creation, with image-upload tests for the expected re-home and a session-ref switch. Despite the keep_open review, it does not preserve route-only navigation detection—specifically the documented Settings-route case—while 1bdd478 implements the broader shared guard on main.
  • fix(desktop): new-chat file attachment submit aborted by route-flush drift race #67812 [closed] duplicate — (+194/-7) — n/a: The diff globally changes drift detection from raw route tokens to stored-session identity and adds a standalone file-attachment race test, but also treats unresolved non-session routes as non-drift. It remains relevant as a competing reference implementation, though it is closed and the automated close verdict identifies 1bdd478 as the broader implementation now on main.

Duplicates

#66185, #66539, and #67812 target the same #65733 route-flush race; #66185 and #66539 are the closest duplicates because both scope alternate drift semantics to the post-create window, while #67812 applies identity semantics globally.

Suggested consolidation

Close #66185 and #66539 as already implemented on main by 1bdd478, based on the automated verdict’s cited shared guard at apps/desktop/src/app/session/hooks/session-context-drift.ts:93-115, its submit integration at apps/desktop/src/app/session/hooks/use-prompt-actions/submit.ts:206 and :506, and regression coverage at apps/desktop/src/app/session/hooks/session-context-drift.test.ts:102-112. Keep #67812 closed; the three PRs can be consolidated as superseded alternatives, with #66185’s semantic route-binding tests representing the strongest salvageable reference from the duplicate set.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I65733(["issue #65733 (open)"])
    subgraph Dup66185 ["PRs duplicating each other"]
        P66185["PR #66185 (open)"]
        P66539["PR #66539 (open)"]
        P67812["PR #67812 (closed)"]
    end
    P66539 -.->|partial| I65733
    class I65733 open
    class P66185 open
    class P66539 open
    class P67812 closed
    class P66185 best
    class P66539 target
    click I65733 "https://github.com/NousResearch/hermes-agent/issues/65733"
    click P66185 "https://github.com/NousResearch/hermes-agent/pull/66185"
    click P66539 "https://github.com/NousResearch/hermes-agent/pull/66539"
    click P67812 "https://github.com/NousResearch/hermes-agent/pull/67812"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 30 kB of PR diffs, 16 kB of issue/PR text, 7 kB of discussion (8 comments), 4 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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: first send of a new chat with an image is silently dropped — submit drift guard misreads its own late route re-home

4 participants