Skip to content

fix(notifications): restore completion sound speed scaling for local tasks - #3166

Merged
frankh merged 1 commit into
mainfrom
posthog-code/fix-sound-speed-scaling-race
Jul 6, 2026
Merged

fix(notifications): restore completion sound speed scaling for local tasks#3166
frankh merged 1 commit into
mainfrom
posthog-code/fix-sound-speed-scaling-race

Conversation

@frankh

@frankh frankh commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What

The "Scale sound speed with task length" setting stopped working for local tasks: every completion sound played at 1x regardless of turn duration. This restores the scaling.

Root cause

For local tasks, two parallel signals report turn completion:

  1. The agent.prompt tRPC mutation resolves and immediately runs updateSession({ isPromptPending: false, promptStartedAt: null }).
  2. The streamed JSON-RPC response event (same stopReason, via the agent-server tap) fires notifyPromptComplete, computing the duration as acpMsg.ts - session.promptStartedAt.

When the feature shipped (#3026), stream events were handled on arrival, so (2) read promptStartedAt before (1) cleared it. #3063 then started buffering stream events on a 16ms flush timer — the mutation callback now reliably wins the race, so the response event always saw promptStartedAt: null, passed durationMs: undefined, and the notification bus fell back to playbackRate = 1. The notification itself still fired (the mutation doesn't touch currentPromptId, so the notify guard passed), which matched the symptom exactly: sound plays, never scaled.

How

handleSessionEvent now reads the turn start from liveTurnContent — the per-turn tally keyed by the prompt echo's timestamp, which the mutation callback never touches — captured before updatePromptStateFromEvents finalizes it, with session.promptStartedAt as the fallback for a mid-turn reconnect (echo replayed from history, so no tally, but also no in-flight mutation to race).

The cloud turn_complete path gets the same capture-before-clear treatment. It previously read session.promptStartedAt after the updateSession that nulls it, working only because the Immer store is copy-on-write — the test harness's in-place-mutating fake store lost the value, which is why the existing cloud test asserted durationMs === undefined (documenting broken behavior).

Testing

  • New regression test in sessionEventBatching.test.ts reproducing the exact race (prompt echo → mutation clears state → buffered response flushes) — verified failing without the fix, asserts the notification carries durationMs = 5000. The harness's fake updateSession is now copy-on-write to match the real Immer store's semantics.
  • cloudTaskUpdateNotifications.test.ts fixtures now carry explicit timestamps and the payload test asserts 45_000 instead of undefined.
  • Full @posthog/core suite green (1926 tests), typecheck clean, biome clean.

Created with PostHog Code

…tasks

The agent.prompt mutation resolves in parallel with the streamed JSON-RPC
response and nulls promptStartedAt. Since the 16ms event batching landed
(#3063), the mutation reliably wins, so the completion notification computed
its duration from an already-cleared timestamp and the sound always played
at 1x. Read the turn start from liveTurnContent (captured before prompt
state is cleared) instead, with promptStartedAt as the reconnect fallback.

Generated-By: PostHog Code
Task-Id: 27accb09-ec92-4c97-9723-a886485fac80
@trunk-io

trunk-io Bot commented Jul 6, 2026

Copy link
Copy Markdown

😎 Merged manually by @frankh - details.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit c9a5140.

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "fix(notifications): restore completion s..." | Re-trigger Greptile

Comment on lines +1890 to +1892
const turnStartedAtTs =
this.liveTurnContent.get(taskRunId)?.startedAtTs ??
session.promptStartedAt;

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.

P2 Duplicated turnStartedAtTs lookup

The expression this.liveTurnContent.get(taskRunId)?.startedAtTs ?? session.promptStartedAt is now identical at lines 1784–1786 (cloud isTurnCompleteEvent branch in updatePromptStateFromEvents) and here. Extracting it to a small private helper — e.g. getTurnStartedAtTs(taskRunId: string, session: AgentSession) — would satisfy the codebase's OnceAndOnlyOnce rule and make the intent self-documenting at both callsites.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@frankh frankh added the Stamphog This will request an autostamp by stamphog on small changes label Jul 6, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The fix correctly captures the turn-start timestamp before session state is cleared, restoring completion sound speed scaling. Both code paths are covered by new tests. The bot's duplication note is a refactor suggestion, not a correctness issue.

@frankh
frankh merged commit 51c3e33 into main Jul 6, 2026
24 checks passed
@frankh
frankh deleted the posthog-code/fix-sound-speed-scaling-race branch July 6, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Stamphog This will request an autostamp by stamphog on small changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant