Skip to content

fix(desktop): keep the composer's focus and insert buses alive while the gateway is connecting - #74571

Open
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/desktop-composer-bus-alive-while-connecting
Open

fix(desktop): keep the composer's focus and insert buses alive while the gateway is connecting#74571
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/desktop-composer-bus-alive-while-connecting

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

This is a sibling follow-up to #73881 (f12e6526a)

  • What fix(desktop): heal type-to-focus onto the visible chat surface #73881 covered: it healed 'active' onto the visible chat surface, so a type-to-focus request can no longer be routed to a composer buried behind a keep-alive tab.
  • What it did not touch: the case where the correctly-routed composer has unsubscribed. use-composer-draft.ts returned early while inputDisabled, and targetIsReachable() still reports that surface live because the composer is mounted and stamps data-composer-target — so healing resolves to a live surface with a dead subscriber.
  • What this adds: the focus and insert buses stay subscribed while the gateway is connecting, so the keystroke is deferred until the gateway opens instead of being dropped.

What does this PR do?

useComposerDraft's bus effect opened with an early return that unsubscribed both onComposerFocusRequest and onComposerInsertRequest whenever inputDisabled was true:

useEffect(() => {
  if (inputDisabled) {
    return undefined          // <- unsubscribes both buses
  }
  const offFocus = onComposerFocusRequest(...)
  const offInsert = onComposerInsertRequest(...)
  return () => { offFocus(); offInsert() }
}, [appendExternalText, inputDisabled, paintDraft, target])

inputDisabled is disabled && !reconnecting where disabled = !gatewayOpen and reconnecting = gatewayState === 'closed' || gatewayState === 'error' (composer/index.tsx, chat/index.tsx). So it is true exactly while gatewayState === 'connecting' — every cold boot, every reconnect attempt (closed → connecting → open), and every profile / gateway switch.

Nothing on the publishing side pauses during that window, and the keybind layer has already committed to the keystroke — app/hooks/use-keybinds.ts calls event.preventDefault() and then requestComposerFocus('active', { typeChar }) (same for the soft /). With no subscriber the character is not deferred, it is destroyed. External inserts (use-composer-actions.ts, right-rail/preview-file.tsx, use-desktop-integrations.ts, clarify-tool.tsx) are dropped the same way.

This is user-visible rather than a hidden edge case: components/gateway-connecting-overlay.tsx only shows its blocking overlay while !coldBootDoneRef.current && !gatewaySwitching && initialBootActive, and its comment states the intent outright — after a healthy boot "Do not cover the chat then — users should still be able to type drafts". So on every reconnect after the first boot the user faces a mounted, visible composer showing the "connecting" placeholder, types into it, and the text vanishes.

The gate was accidental, not deliberate. The sibling onComposerInsertRefsRequest subscription in this same hook has always been unconditional, so ref-chip inserts already survived the connecting window while plain text and keystrokes did not — same hook, same file, same bus family, opposite policy. And composer/focus.ts's resolveActive docstring already names this exact symptom in main's own words: "with no subscriber on the visible surface, every type-to-focus keystroke is preventDefault'd and dropped."

Why stay subscribed rather than defer preventDefault() on the keybind side. The deferral machinery already exists and is already correct — useEffect(() => { if (!inputDisabled) { focusInput() } }, [focusInput, focusKey, focusRequestId, inputDisabled]) lists inputDisabled in its deps, so it re-runs the moment the gateway opens and focus lands then. Keeping the fix inside this one hook also leaves contentEditable={!inputDisabled} and the deliberate submit gate in use-composer-submit.ts untouched, so #45488's split of editability from submit availability is preserved.

paintDraft is passed !inputDisabled instead of a hardcoded true so a mid-connect keystroke does not raise a focus request against an editor that is not yet contentEditable; the effect above is the single focus authority. (Note for the reviewer: this argument is defensive/documentary — because that effect is itself gated on !inputDisabled, a hardcoded true produces the same end state, minus a redundant focusRequestId bump and re-render per mid-connect keystroke, which this file's design notes explicitly try to avoid. The behavioural fix is the removal of the early return, and that is what the tests pin.)

Sibling-site sweep

onComposerFocusRequest / onComposerInsertRequest / onComposerInsertRefsRequest / onComposerSubmitRequest / onComposerVoiceToggleRequest and every inputDisabled reference under apps/desktop/src:

Site Verdict
use-composer-draft.ts bus effect (gates both focus + insert) the site — fixed here
use-composer-draft.ts onComposerInsertRefsRequest already unconditional; evidence the gate was accidental — nothing to change
use-composer-submit.ts onComposerSubmitRequest + inner !inputDisabled subscribed unconditionally; the inner gate is the deliberate #45488 split — excluded
use-composer-voice.ts onComposerVoiceToggleRequest unconditional — not a site
user-edit-composer.tsx focus + insert subscriptions unconditional, no disabled flag — not a site
use-composer-draft.ts if (!inputDisabled) focusInput() deliberate; this fix depends on it — excluded
composer/index.tsx aria-disabled / contentEditable={!inputDisabled} deliberate DOM state — excluded

One site, two buses. No other subscription in the tree shares this root cause.

Related Issue

No filed issue — this was found by tracing the preventDefault()-then-dispatch path that focus.ts's resolveActive docstring describes. Context instead:

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/chat/composer/hooks/use-composer-draft.ts — dropped the if (inputDisabled) { return undefined } early return so the focus and insert buses stay subscribed while the gateway is connecting; pass !inputDisabled as paintDraft's focus argument; comment recording why the subscription must outlive inputDisabled.
  • apps/desktop/src/app/chat/composer/hooks/use-composer-draft.test.tsx — new describe with 4 cases and a ConnectingHarness that binds a real contentEditable to the hook's editorRef and takes inputDisabled as a prop. The existing ProbeHarness is untouched (it renders null and hardcodes inputDisabled: false for the [Desktop] Chat tab messages leak across sessions — cross-tab content mixing #59305 attachment-scope test).

How to Test

Automated:

cd apps/desktop
npx vitest run --project ui src/app/chat/composer/hooks/use-composer-draft.test.tsx

8 passed (8) — 4 new, 4 pre-existing. The whole composer directory is 28 files / 219 passed, and the full desktop ui project is 339 files / 3017 passed.

Fails before the fix (verified both ways, not assumed). Restoring if (inputDisabled) { return undefined } turns exactly the three new behavioural cases red and leaves the fourth (the gateway-already-open guard) green:

× lands a type-to-focus keystroke in the draft while the gateway is connecting
    AssertionError: expected '' to be 'h'
× lands an external insert in the draft while the gateway is connecting
    AssertionError: expected '' to be 'pasted while connecting'
× keeps the mid-connect keystroke and focuses the editor once the gateway opens
    AssertionError: expected '' to be 'h'
✓ still appends and focuses when the gateway is already open
  Tests  3 failed | 5 passed (8)

The empty-string results are the point: without a subscriber the dispatched event reaches nobody and the draft never receives the character.

One trap worth flagging for anyone extending these tests: focus.ts's dispatch defers through window.setTimeout(…, 0), and no pre-existing test in this file exercises that path (they all use the synchronous markActiveComposer / getActiveComposer helpers). Without draining the macrotask the assertions pass vacuously — "the draft is empty" would hold because the event was never delivered at all. The new cases go through a flushBus() helper, and the red-before run above is what proves they are not vacuous.

Manual:

  1. Launch the desktop app and let it finish its cold boot (the connecting overlay must be gone).
  2. Kill or restart the gateway so the app goes closed → connecting. The composer stays visible with the "connecting" placeholder.
  3. While it reads "connecting", type a few characters with the composer unfocused (type-to-focus), or use a sidebar "insert into composer" action.
  4. Before: the characters/text are silently discarded. After: they land in the draft, and the composer takes focus by itself once the gateway reaches open.

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, this is desktop TypeScript; the vitest runs above are the equivalent gate. (Heads-up unrelated to this PR: Python tests / Run tests slice 8/8 and the aggregate All required checks pass gate are currently red on clean origin/main itself.)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.4) — not verified on Windows or Linux

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — the changed behaviour is documented in-file at the subscription
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — no platform-specific code; renderer-only React/DOM
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Related / Positioning

Two other open PRs touch use-composer-draft.ts. Neither is a duplicate of this one, and I'd rather name them than let a reviewer find them:

  • fix(desktop): prevent composer draft leak on new session #62586 (prevent composer draft leak on new session) — disjoint. Its hunks are at the import block and the per-session stash path; its only inputDisabled occurrence is a test-harness prop.
  • fix(desktop): discard draft on explicit new session #62579 (discard draft on explicit new session) — edits this exact effect, for a different bug. It adds a third subscription, onComposerClearRequest, inside the same effect, with offClear() in the cleanup. It does not remove the early return, so it neither duplicates nor supersedes this change — but it is worth noting that its new subscription lands inside the very gate this PR removes, and therefore inherits this same defect: a clear request published while the gateway is connecting would find no subscriber. Removing the early return here makes that pending subscription correct too. The textual overlap is a few lines and this diff was deliberately kept minimal so either order of merging resolves trivially.

I deliberately did not touch app/hooks/use-keybinds.ts or composer/focus.ts, even though the preventDefault() call and the resolveActive docstring both live there — #74545, #73948, #72959 and #74406 are actively reworking those two files, and this bug is fixable entirely on the subscriber side.

…the gateway is connecting

`useComposerDraft`'s bus effect returned early whenever `inputDisabled`,
which unsubscribed BOTH `onComposerFocusRequest` and
`onComposerInsertRequest`. `inputDisabled` is `disabled && !reconnecting`,
i.e. exactly `gatewayState === 'connecting'` — every cold boot, every
reconnect attempt, and every profile/gateway switch.

Nothing on the publishing side stops during that window, and the keybind
layer has already committed to the keystroke: use-keybinds.ts calls
`event.preventDefault()` and *then* `requestComposerFocus('active',
{ typeChar })`. With no subscriber the character is not deferred, it is
destroyed. The same holds for external inserts. After the first successful
boot the connecting overlay is deliberately not shown, so the user is
looking at a mounted, visible composer that silently eats what they type.

The gate was accidental, not deliberate: the sibling
`onComposerInsertRefsRequest` subscription in this same hook has always
been unconditional, so ref-chip inserts already survived the window while
plain text and keystrokes did not. `focus.ts`'s own `resolveActive`
docstring names the symptom ("preventDefault'd and dropped").

Stay subscribed and let the existing deferral do its job: the
`if (!inputDisabled) focusInput()` effect lists `inputDisabled` in its
deps, so it re-runs the moment the gateway opens and focus lands then.
`paintDraft` is passed `!inputDisabled` so a mid-connect keystroke does not
raise a focus request against an editor that is not yet contentEditable.

The deliberate submit gate in `use-composer-submit.ts` and
`contentEditable={!inputDisabled}` are untouched — editability and submit
availability stay split.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. The premise remains present on current main: apps/desktop/src/app/chat/index.tsx:298,567 makes the composer disabled until the gateway is open, apps/desktop/src/app/chat/composer/index.tsx:195-197 maps the connecting state to inputDisabled, and apps/desktop/src/app/chat/composer/hooks/use-composer-draft.ts:166-196 then skips both focus and text-insert subscriptions. apps/desktop/src/app/hooks/use-keybinds.ts:306-309 consumes type-to-focus input before publishing it.

The patch removes only that listener gate while preserving the focus deferral already implemented at use-composer-draft.ts:151-155, plus the separate submit and contentEditable gates. The added tests dispatch through the real focus bus and drain its macrotask, matching apps/desktop/src/app/chat/composer/focus.ts:141-146.

This is an automated hermes-sweeper review.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Jul 30, 2026
@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
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/*) P3 Low — cosmetic, nice to have 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants