Skip to content

feat(desktop): dive into task UI while the agent connects - #93156

Closed
frankh wants to merge 1 commit into
masterfrom
posthog/desktop-dive-into-task-ui
Closed

frankh wants to merge 1 commit into
masterfrom
posthog/desktop-dive-into-task-ui

Conversation

@frankh

@frankh frankh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem

Opening a task that is still connecting shows a full-panel "Connecting to agent" spinner, so you cannot read the thread or start typing until the agent is ready. Cloud tasks show the same kind of full-panel wait while the sandbox provisions.

  • The composer is hidden behind the spinner, so a prompt you already know you want to send has to wait for the connection.
  • The spinner takes up the whole panel for a state that is often brief.

Changes

  • Switching to a task shows the thread and composer immediately, with a small inline "Connecting to agent" line above the composer instead of the full-panel spinner. Cloud provisioning shows the sandbox status ("Waiting in the queue", "Starting the sandbox") in the same inline spot.
  • You can type and submit one prompt while the agent connects. It queues, shows in the existing queued-messages dock, and sends automatically once the agent is ready.
  • After one prompt is queued, the composer is disabled until the agent connects, so only a single prompt waits.
  • A failed connect keeps the queued prompt instead of dropping it, so it stays available to the retry flow.
  • deriveSessionViewState gains an isConnecting flag and narrows isInitializing to only the "no session yet" (and cloud transcript-hydrating) cases; SessionView uses these to gate the composer and the indicator. Mechanical: the touched composer markup moved from Radix Box to div, and the new indicator uses @posthog/quill.

No screenshot: the app could not be launched in this environment (a native dependency build and registry access both failed), so no before/after image was captured.

How did you test this code?

Automated tests added and run locally from the packages/core and packages/ui workspaces:

  • sessionViewState.test.ts — cases for isConnecting and the narrowed isInitializing across local connecting (with and without a painted tail), cloud provisioning, connected, terminal, and no-session. Catches a regression that re-shows the full-panel spinner while a session exists.
  • sessionServiceCompaction.test.ts — sendPrompt while connecting queues instead of throwing; a second submit keeps the queue at one; an errored session still throws; a failed connect preserves the queued prompt. Catches loss of the queue-while-connecting behavior, the single-prompt guard, and prompt loss on connect failure.
  • sessionServiceHost.test.ts — updated the existing "throws when connecting" case to assert the new queue behavior.

Not verified: the connecting-to-connected drain path end to end (needs the running app), and any manual UI check. CI reports suite pass counts.

Automatic notifications

  • Publish to changelog?

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted) — directed by @frankh.

Authored with Claude (PostHog Desktop). Skills invoked: /writing-tests (test value gate) and /writing-pr-descriptions (this body). Component choice and Radix rules for the desktop package were followed from products/desktop/AGENTS.md.

Design decisions across the session: the drain-on-connect hook attaches to the single local connecting → connected transition and reuses the existing flushQueuedMessagesIfIdle helper rather than adding a new drain path; cloud reuses its existing queue-and-drain triggers and only gained a single-prompt guard. The full reconnect-path drain was left to manual verification rather than a heavy, brittle unit test.


Created with PostHog Desktop

@frankh frankh self-assigned this Sep 2, 2026
@trunk-io

trunk-io Bot commented Sep 2, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions github-actions Bot added the feature/desktop Feature Tag: Desktop label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

React Doctor found 2 issues in 1 file · 2 warnings.

2 warnings

packages/ui/src/features/sessions/components/SessionView.tsx

Reviewed by React Doctor for commit 5a0d646.

@github-actions

github-actions Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane (fe:product:desktop)

This PR is assigned to the non-backend lane (fe:product:desktop). It does not run backend Python tests and may merge in parallel with PRs in other lanes.

🚨 Comment density — 8% of added code lines are comments (25 of 329)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/desktop/packages/core/src/sessions/sessionService.ts 10 26
products/desktop/packages/core/src/sessions/sessionViewState.ts 10 28
products/desktop/packages/ui/src/features/sessions/components/SessionView.tsx 3 27
products/desktop/packages/core/src/sessions/sessionServiceCompaction.test.ts 2 53

This check does not block merging. It updates on every push and clears when the share drops.

@hosthog

hosthog Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

HostHog preview — posthog-desktop-web

The previews for this PR have been torn down and no longer serve.

@talyn-app talyn-app Bot added the stamphog Request AI approval (no full review) label Sep 2, 2026

frankh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

React Doctor's two warnings — both left as-is:

  • SessionView.tsx:106 only-export-components on getNewAttachments — pre-existing on master, unchanged by this PR.
  • SessionView.tsx:154 no-many-boolean-props — this PR adds one boolean (isConnecting) to a prop list that was already long. Reshaping SessionView's public signature would touch every call site and is outside this change's scope. The warning did not fail its check.

🦉 via talyn.dev

@frankh
frankh force-pushed the posthog/desktop-dive-into-task-ui branch 3 times, most recently from b6262c8 to 5850b7c Compare September 7, 2026 13:36

frankh commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto master to clear the conflict in SessionView.tsx. Master had added SessionSummaryPanel inside the composer block this PR restructures, so the resolution keeps it as the first child of ComposerWidth after the new ConnectingIndicator, and keeps master's removal of the handoff-summary side question. The PR's file set is unchanged by the rebase.

React Doctor re-ran and reports the same two warnings at shifted line numbers (SessionView.tsx:103 and :149). Both still stand as previously noted: getNewAttachments is exported on master unchanged by this PR, and no-many-boolean-props counts one added boolean on a signature that was already long.

🦉 via talyn.dev

@frankh
frankh force-pushed the posthog/desktop-dive-into-task-ui branch from 5850b7c to 728cf9a Compare September 7, 2026 20:24

frankh commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto master to clear a conflict in sessionViewState.ts. Master's #95870 landed in the same logic, so the resolution needed a call on each side:

  • Kept master's deriveSessionLifecycleState intact. Its wider startup window still feeds the task-row status dot via useTaskStatusInput, which reports "starting" until the run's first prompt lands. That is a different surface from the session panel and this PR does not change it.
  • The view's isInitializing follows this PR. A full-panel spinner now shows only when there is nothing to render: no session for the active run, a transcript still hydrating, or a dead session a restart is replacing. deriveSessionViewState no longer reads the lifecycle's isInitializing, and the lifecycle state gained sessionMatchesActiveRun so the view can tell "this run's session" from a previous run's.
  • Kept master's behavior in four cases this PR never meant to change: a session belonging to an older run, a restart from a terminal task, an unrelated stale session, and a terminal run whose transcript is hydrating. All four still show the loading view; my first attempt broke them and their tests caught it.
  • Two of master's tests assert the loading behavior this PR replaces (keeps the loading view through optimistic prompts and setup events, and the local-connecting half of keeps a local session loading until its first prompt). Both were retargeted to the new intent rather than dropped, and the local one now also covers the older-run guard.
  • Restored the cloudStatus prop on SessionView. Master removed it when SessionInitializingView stopped taking it; the new ConnectingIndicator is a fresh consumer, and without it cloud runs lose the "Waiting in the queue" / "Starting the sandbox" labels.

The PR's file set is unchanged by the rebase. @posthog/core (4129) and @posthog/ui (4418) suites pass locally, along with typecheck and Biome. All 78 CI checks pass on 728cf9a.

React Doctor re-ran and reports the same two warnings at shifted lines (SessionView.tsx:100 and :146). Both still stand as noted above: getNewAttachments is exported on master unchanged by this PR, and no-many-boolean-props counts one added boolean on a signature that was already long.

🦉 via talyn.dev

@frankh
frankh force-pushed the posthog/desktop-dive-into-task-ui branch from 728cf9a to 70d3dff Compare September 8, 2026 19:53

frankh commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto master to clear a conflict in CloudSessionLifecycle.tsx, plus one adaptation master forced on the new component.

  • Took this PR's side on ConnectingToAgent. Master still defines and uses it; this PR replaces it with ConnectingIndicator. The auto-merge already moved SessionView's only call site onto the new component, and ConnectingToAgent has no other consumer on master, so keeping it would have left dead code behind a removed Spinner import.
  • Spinner changed shape on master. It now takes size: "xs" | "sm" | "md" | "lg" instead of a pixel number, so the new ConnectingIndicator no longer typechecked. size={12} became size="sm", which is the same 12px. It also gained aria-hidden="true", matching how the repo's other spinners sit beside visible loading text so the label is not announced twice.

The PR's file set is unchanged by the rebase. @posthog/core (4208), @posthog/ui (4502), typecheck, and Biome all pass locally. All 75 CI checks pass on 70d3dff.

Two unrelated flakes on the way there, one per run, both Error: Test timed out in 5000ms:

  • apps/code › secure-store/service.test.ts › round-trips a value through encryption
  • packages/core › canvas/channelItems.test.ts › dates a created-first list by when each session started

Neither test is this PR's. Both are fully synchronous — mocked safeStorage over an in-memory Map in the first, a pure grouping function in the second — with no timers, no I/O, and nothing awaited, so neither can spend five seconds on its own work. The secure-store file runs in 63ms locally against 12005ms in the failing job, and the channelItems run reports 30.72s of module import inside a 30.09s wall clock. That is worker starvation on a contended runner, not a hang. The third run was green with no code change.

Raising the timeout or shrinking the pool would be a change to the desktop test job's vitest config, which is repo-wide and would land on every open PR's CI; this PR touches no test config and no file under apps/code, so I left it alone rather than widen the diff.

On the comment-density alert: leaving the comments as they are. The sessionViewState.ts block records why a full-panel spinner now shows only when there is nothing to render, and why a task row keeps the wider startup window instead; the sessionService.ts ones record why a queued prompt survives a failed connect and why a second submit while connecting is a no-op. Those are reasons the code cannot show, which is the kind the house rules keep. The check does not block merging.

🦉 via talyn.dev

@frankh
frankh force-pushed the posthog/desktop-dive-into-task-ui branch from 615f63b to 1ba1287 Compare September 9, 2026 13:23

frankh commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto master to clear a conflict in sessionViewState.test.ts. Master had extended the local-startup test with a middle assertion: a session that is connected but still holds an unsent initialPrompt counts as loading, until firstPromptForRunId lands.

  • Took this PR's side on the view-state test. That assertion no longer describes deriveSessionViewState, because this PR moves the panel off the wider startup window: with a session for the active run, the thread and composer open while the agent connects.
  • Kept master's coverage where the behavior still lives. The window survives in deriveSessionLifecycleState, which feeds the task-row status dot, so master's three assertions moved there as a deriveSessionLifecycleState test rather than being dropped. Without it, removing that branch would have silently regressed the dot with nothing failing.

The PR's file set is unchanged by the rebase (same 11 files), and sessionViewState.test.ts plus sessionServiceCompaction.test.ts pass locally.

React Doctor re-ran on the new head and reports the same two warnings at shifted lines (SessionView.tsx:101 and :147). Both still stand as noted earlier: getNewAttachments is exported on master unchanged by this PR, and no-many-boolean-props counts one added boolean on a signature that was already long.

🦉 via talyn.dev

Switching to a task no longer shows a full-panel "Connecting to agent"
spinner. The thread and composer render immediately, with a small inline
connecting indicator above the composer. The user can type and queue one
prompt while the agent connects; it sends automatically once connected,
and the composer blocks after one prompt is queued.

Generated-By: PostHog Desktop
Task-Id: c2b5df69-5229-4f4a-a011-fbf465fa7c4e
@frankh
frankh force-pushed the posthog/desktop-dive-into-task-ui branch from 1ba1287 to 5a0d646 Compare September 10, 2026 05:28

frankh commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto master to clear a conflict in SessionView.tsx. Master's #97625 ("standardize session startup in the chat") landed in the same composer block, so the resolution needed a call on each side:

  • Took master's structure. It replaced the ConnectingToAgent cross-fade with a composer that renders immediately, and switched the wrapper from <div> to <Box>. The new ConnectingIndicator now sits as the first child of ComposerWidth, above SessionSummaryPanel.
  • Kept master's placeholder copy, on this PR's condition. Master gated the placeholder on isRunning; this PR lets the user type while the agent connects, so the placeholder now follows composerDisabled. The composer shows the normal typing hint whenever it is usable, and "Waiting for the agent..." when it is not — either because nothing is running and nothing is connecting, or because a prompt is already queued waiting for the connect.
  • CloudSessionLifecycle.tsx dropped out of the diff. This PR's only change there was removing ConnectingToAgent; master's feat(desktop): standardize session startup in the chat #97625 removed it too, so the file no longer differs. The PR's remaining file set is unchanged.

Typecheck passes for @posthog/core and @posthog/ui, and the four touched test files pass (239 tests).

🦉 via talyn.dev

frankh commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

React Doctor re-ran on the rebased head and reports the same two warnings at shifted lines. Both still stand as previously noted:

  • SessionView.tsx:95 only-export-components on getNewAttachments — exported on master at line 93 and unchanged by this PR.
  • SessionView.tsx:141 no-many-boolean-props on SessionView — this PR adds one boolean (isConnecting) to a prop list that was already long. Reshaping SessionView's public signature would touch every call site and is outside this change's scope.

Neither failed its check. All 77 checks pass on 5a0d646, and the branch merges cleanly into master.

🦉 via talyn.dev

@scheduled-actions-posthog

Copy link
Copy Markdown
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

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

Labels

feature/desktop Feature Tag: Desktop stale stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant