Skip to content

test(voice-notes): keep each attachment's own type in the upload fixture - #999

Merged
loganj merged 1 commit into
mainfrom
jameskraus/voice-notes-route-teardown
Oct 14, 2026
Merged

loganj merged 1 commit into
mainfrom
jameskraus/voice-notes-route-teardown

Conversation

@jameskraus

@jameskraus jameskraus commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The voice-notes fixture answers every upload with the demo voice note's metadata, including type: "video/mp4". When voice-notes › "rejects a full attachment draft before opening the microphone" attaches ten .txt files, each draft therefore gets a <video preload="metadata"> preview (ComposerAttachments.tsx). Those previews make 10–20 /api/relay/media loads through the file's page.route handler, which calls page.evaluate and route.fulfill.

Nothing waits for those loads. When one is still in its handler as the test finishes, the test fails with "page.evaluate: Test ended." while running route callback. On main this failed WebKit 6/6 in 38443596151, 38450152226 and 38453858017.

Change

uploadAttachment in tests/fixtures/voice-notes.tsx returns the demo metadata only for audio files, which is how recordings upload (audio/wav). Other files keep their own type, so plain attachments render as file rows and start no media loads. Recordings, assertions and the route are unchanged.

Tests

  • No browser cases added or removed. One fixture function changes.
  • Local media loads during the full-draft test, with request logging in an uncommitted scratch copy, measured in both WebKit and Chromium:
    • With the current fixture: 10–20.
    • With this change: 0.
  • bin/pnpm test:browser --project webkit --project chromium --no-deps tests/browser/voice-notes.spec.mjs --repeat-each=2 passes 20/20 on current main, including the @media-recorder cases.

Stack

This PR → #997 → #989.

Readiness

  • Agent review (no blockers)
  • Hosted CI green
  • Human check

Known gaps

  • The race needs a slow runner to fail. Locally the evidence is that the media loads no longer happen, not a reproduced failure.

@jameskraus jameskraus changed the title test(voice-notes): let media route handlers finish before teardown test(voice-notes): keep non-recording uploads out of the voice-note path Oct 13, 2026
@jameskraus
jameskraus marked this pull request as ready for review October 13, 2026 23:41
@jameskraus
jameskraus requested a review from a team as a code owner October 13, 2026 23:41
@jameskraus
jameskraus added this pull request to stack #1028 October 14, 2026 00:22

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

No blockers. The fix targets the right thing: recordings upload as audio/wav (useVoiceNoteRecorder.ts:170), so they keep the demo voice metadata. The ten .txt drafts now stay text/plain and render as file rows, so they make no /api/relay/media requests. The full-draft test still proves the limit: 10 Ready rows, the limit message, and no mic start. Hosted CI is green at 7bcd41f1.

Non-blocking:

  • One wording nit, inline.
  • The page.route handler in the spec's beforeEach can still throw Test ended if a future test finishes with a media load in flight. An afterEach calling page.unrouteAll({ behavior: "ignoreErrors" }) (Playwright 1.63 supports it) would cover that whole class. Fine as a follow-up.

Comment thread tests/fixtures/voice-notes.tsx Outdated
});
if (report.failUpload) throw new Error("Fixture upload failed");
// Only recordings come back as voice notes; other files keep their type,
// so attaching them does not start voice-note media loads.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: the loads this avoids aren't voice-note loads. A draft row's voice comes from the recording (attachment-draft.ts:288), not from the upload result, so the old .txt drafts never rendered as voice-note cards. They came back as video/mp4 and got the <video preload="metadata"> preview (ComposerAttachments.tsx:225), which loads through the media route. Maybe:

// Only recordings come back as voice notes. Other files keep their own type,
// so they don't get video previews that load media.

The PR description says "voice-note preview" as well.

@jameskraus jameskraus changed the title test(voice-notes): keep non-recording uploads out of the voice-note path test(voice-notes): keep each attachment's own type in the upload fixture Oct 14, 2026
The voice-notes fixture answers every upload with the demo voice note's
video metadata. Attaching ten text files therefore makes ten voice-note
previews load /api/relay/media through the test's route after the test
has finished, and the route's page.evaluate fails with "Test ended".

Return the file's own type for anything that is not a recording, so plain
attachments start no media loads. Recordings are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: James Kraus <jamespkraus@gmail.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
@loganj
loganj force-pushed the jameskraus/voice-notes-route-teardown branch from 5279f75 to e1fbbcb Compare October 14, 2026 14:14
@loganj
loganj merged commit b17e891 into main Oct 14, 2026
30 checks passed
@loganj
loganj deleted the jameskraus/voice-notes-route-teardown branch October 14, 2026 14:32
johnmatthewtennant added a commit that referenced this pull request Oct 14, 2026
* commit '506ac5d4d':
  Fix JPEG uploads and attachment control overlap (#1068)
  feat(channels): read, write and look up channel labels (#964)
  refactor(agents): separate the agent runner and its reads from the desktop app (#961)
  fix(media): show desktop animated avatars and match dev broker media rules to native (#990)
  ci(browser): split journeys across nine shards per engine (#989)
  test(inbox): open the edit journey's conversation by name (#997)
  test(voice-notes): keep each attachment's own type in the upload fixture (#999)
  Stabilize main: consolidate WebKit flake and bottom-follow fixes (#1039)
  Push main-pane threads in and hold threads behind a loading mark (#1018)
  [buzz-app] Show full sent images and GIFs (#886)
  feat(agents): say the agents this copy runs are online (#862)
  [buzz-app] Keep the sidebar in place when switching to Personal space (#945)
  fix(builderlab): link the Buzz key before attesting remote agents (#978)
  feat(agents): show agents typing to everyone while they work on a mention (#998)

Signed-off-by: John Tennant <jtennant@block.xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants