You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(mobile): send no longer waits one round trip per saved attachment - #30774
Photos and files start uploading as soon as they are attached, so by the time the user taps Send they
are usually already saved on the server. Delivery then re-checks each saved upload before it sends the
turn: prepareTurnAttachments (apps/mobile/src/lib/attachmentUpload.ts) calls verifyPersistedAttachmentUpload (one assets.createUrl WebSocket request) inside its per-attachment
loop and awaits it before looking at the next attachment. A message with N saved attachments therefore
waits N round trips before the send command goes out. The same wait applies when a queued message is
edited and saved. Web runs these checks inside its upload jobs, up to 3 per environment at a time; mobile runs
them one after another.
On a 100 ms request (a typical remote or cellular round trip) that is about 0.2 s for 3 attachments,
0.4 s for 5, 0.85 s for 10, and 9 s at the cap of 100. With 1 or 2 attachments the saving is at most one
round trip, so this matters from about 4 attachments up.
Change
The checks for all saved attachments now start up front, at most ASSET_URL_MAX_IN_FLIGHT (6) at a time,
and the existing loop reads the results in attachment order. Everything else in the loop is unchanged:
the access checks, uploads of attachments that need one, runAttachmentUploadCycle, the abort signal,
and the error that is thrown.
Bound: 6 is the cap assets.createUrl already has per environment (the transport semaphore in packages/client-runtime/src/state/assets.ts), so the server sees no more concurrent requests than it
does today when a thread loads its images. The server handler is a path lookup with no rate limit of
its own. Using the same number here avoids queueing dozens of requests in front of thread image URLs.
Verifications that have not started when the loop ends, throws, or is cancelled are not started.
+50/-8 lines in attachmentUpload.ts (a 14-line limiter helper and the up-front start); tests are the rest.
Remaining differences from main (I checked each; none changes a result):
Later attachments are verified before earlier uploads finish, so a "verified" answer can be older by
the upload time of earlier attachments. The server keeps pending uploads for 24 hours, and the loop
already had this kind of gap between a check and the send command. One case this makes reachable in a
new order: the server's 24-hour stale-upload sweep runs inside attachments.createUploadUrl. With [A needs an upload, B saved more than 24 h ago and not yet swept], main checks B after A's upload
request has swept it and re-uploads B; this branch can check B first, then A's upload sweeps B, and the
send fails when the server claims B. Main already has the same failure for the order [B, A], and a
retry recovers in both cases.
Scope: requireUploadAccess still runs before each attachment is used, but a verification request now
starts without a per-item check. The server enforces the scope on assets.createUrl anyway.
If an earlier attachment fails or the send is cancelled, up to 6 read-only verification requests are
already in flight and finish unread (main would have sent none for later attachments).
Verifications share the 6 assets.createUrl slots with thread image URLs; those can wait about one
round trip while a batch of verifications runs.
Unchanged: result order, pendingAttachmentIds order, which attachment's error is reported when several
fail (the first in attachment order), a rejected verification being thrown where the loop reaches it,
cancel returning abandoned, and the pre-existing gap where a verified upload can still disappear before
the server claims it.
Scope and approval
No prior issue or discussion. I am not claiming this is an obvious bug: it is a behaviour-preserving
performance fix of one underlying problem (serial verification) in one function, with no setting, UI,
protocol or default changed. If you'd rather agree on it in an Ideas discussion first, since this is a
latency improvement rather than a defect, say so and I'll move it there.
Open #12610 (upload timeout) edits uploadFileBytes and a different part of the test file. It is already
marked conflicting with main. The two do not touch the same lines of prepareTurnAttachments; the helper
here is placed away from the spot where #12610 adds its constant. With #12610 a single upload can take up
to 3 minutes, which makes difference 1 above up to 3 minutes per earlier upload, still inside the 24 hour
server keep time. Open #18631 changes one line of the same file, away from these lines.
Verification
Tests (Node 26.12.1, vp test run --project unit src/lib/attachmentUpload.test.ts in apps/mobile): 35
pass. Eight of them are new, in describe("verifying saved uploads"), using deferred fakes for the checks:
all three checks are started before any resolves, and the result and pendingAttachmentIds stay in order
when they resolve in reverse;
at most 6 are in flight, the 7th starts when one finishes;
the first failing attachment in order is reported, also when a later one fails first (the plain version
of this passes on main and on the branch, so it pins behaviour that must not change);
a later rejected check does not leak an unhandled rejection;
no more checks start after an earlier one fails;
later checks are started while an expired earlier attachment is uploading, then order is kept;
cancelling during verification returns abandoned without minting or uploading.
On main (the new tests against the unchanged source), 6 of the 8 fail (overlap, bound, stop, ordering under
reverse completion, uploading while verifying, cancel) and the other 2 pass. Removing the stop flag or the
bound from the change fails a test; removing the .catch leaves every test passing but produces 3
unhandled rejections, which fail the run. Also green: the other mobile tests that import this module (6 files, 167 tests), tsc --noEmit
in apps/mobile, vp lint and vp fmt --check on the two files.
Timing, real prepareTurnAttachments and real verifyPersistedAttachmentUpload bundled with esbuild; only the
WebSocket request is a timer (100 ms each, at most 6 in flight, as the transport allows). n = 5 per row,
main and branch alternating, median (min), ms:
saved attachments
main
this PR
1
108 (103)
108 (103)
3
321 (314)
106 (102)
5
542 (531)
106 (101)
10
1076 (1071)
218 (215)
100 (cap)
10733 (10691)
1819 (1812)
These come from a timer model, not a phone or a real tunnel. Real savings follow the real round-trip
time plus server time. Not checked: a device, a real remote connection, Android, iOS, and the react test project. Raw rows: raw/verify-m302.jsonl in the scratch folder, available on request.
Claude Sonnet 5.5 and Claude Opus 5.5 in T3 Code (Claude Code harness).
prepareTurnAttachments re-verified every already uploaded attachment with
its own assets.createUrl round trip, one after another, so a message with N
saved uploads waited N round trips before the send command could go out.
Start the verifications up front, at most ASSET_URL_MAX_IN_FLIGHT (6) at a
time, and let the existing loop read the results in attachment order. Result
order, which failing attachment is reported, upload and cancellation handling
stay as they were. Verifications that have not started are dropped once the
loop ends or the send is cancelled.
Macroscope's review found this PR approvable — Saved attachment checks now run up to six at a time while preserving attachment order and existing upload handling. The change is limited to request scheduling, with concurrency bounded by existing asset limits.
You can add or adjust custom eligibility rules. Learn more.
No actionable comments were generated in the recent review.
Walkthrough
prepareTurnAttachments now verifies saved uploads concurrently within a fixed limit and consumes results in attachment order. Tests cover concurrency, failure ordering, expired-upload replacement, queued work, and cancellation.
A concurrency limiter caps active saved-upload verifications. Tests check the limit and confirm queued checks start when a slot opens.
Ordered results and stopping queued work apps/mobile/src/lib/attachmentUpload.ts, apps/mobile/src/lib/attachmentUpload.test.ts
The preparation loop consumes verification results in attachment order and stops queued checks after processing ends or throws. Tests cover failure ordering, expired-upload replacement, and cancellation.
The attachment-verification change appears ready to merge after normal checks.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title "perf(mobile): send no longer waits one round trip per saved attachment" matches the main change: saved-attachment verification in prepareTurnAttachments now runs concurrently (capped at ASS…
Description check
The description follows the repository template closely. It has Problem, Change, Scope and approval, and Verification sections. The Problem section explains the serial await per saved attachment with …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title "perf(mobile): send no longer waits one round trip per saved attachment" matches the main change: saved-attachment verification in prepareTurnAttachments now runs concurrently (capped at ASSET_URL_MAX_IN_FLIGHT) instead of serially. It is a conventional commit title in plain language and specific enough for a teammate scanning history. It is 70 characters, within a reasonable length.
Full details: Description check
Explanation
The description follows the repository template closely. It has Problem, Change, Scope and approval, and Verification sections. The Problem section explains the serial await per saved attachment with reproduction timing. The Change section explains the bounded concurrency and why the cap of 6 matches the existing transport semaphore. The Scope section explains there is no prior issue or discussion and why the change should qualify as a behaviour-preserving performance fix, and it offers to move the discussion to Ideas if maintainers prefer. The Verification section reports the focused test command, the 35 passing tests, the 8 new tests with the 6 that fail on main, lint and type checks, and a timing table. It also states what was not checked: devices, real remote connections, Android, iOS, and the react test project. Some sections are long and dense, and the Scope section relies on the author's judgement that the change qualifies without prior approval. The template asks for maintainer approval or an explicit exemption justification, and the author provides the latter. The description also mentions the model used, which the template requests.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between 8b44f42 and 7ab121f.
Files selected for processing: 2
apps/mobile/src/lib/attachmentUpload.test.ts
apps/mobile/src/lib/attachmentUpload.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size:M30-99 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Photos and files start uploading as soon as they are attached, so by the time the user taps Send they
are usually already saved on the server. Delivery then re-checks each saved upload before it sends the
turn:
prepareTurnAttachments(apps/mobile/src/lib/attachmentUpload.ts) callsverifyPersistedAttachmentUpload(oneassets.createUrlWebSocket request) inside its per-attachmentloop and awaits it before looking at the next attachment. A message with N saved attachments therefore
waits N round trips before the send command goes out. The same wait applies when a queued message is
edited and saved. Web runs these checks inside its upload jobs, up to 3 per environment at a time; mobile runs
them one after another.
On a 100 ms request (a typical remote or cellular round trip) that is about 0.2 s for 3 attachments,
0.4 s for 5, 0.85 s for 10, and 9 s at the cap of 100. With 1 or 2 attachments the saving is at most one
round trip, so this matters from about 4 attachments up.
Change
The checks for all saved attachments now start up front, at most
ASSET_URL_MAX_IN_FLIGHT(6) at a time,and the existing loop reads the results in attachment order. Everything else in the loop is unchanged:
the access checks, uploads of attachments that need one,
runAttachmentUploadCycle, the abort signal,and the error that is thrown.
assets.createUrlalready has per environment (the transport semaphore inpackages/client-runtime/src/state/assets.ts), so the server sees no more concurrent requests than itdoes today when a thread loads its images. The server handler is a path lookup with no rate limit of
its own. Using the same number here avoids queueing dozens of requests in front of thread image URLs.
attachmentUpload.ts(a 14-line limiter helper and the up-front start); tests are the rest.Remaining differences from main (I checked each; none changes a result):
the upload time of earlier attachments. The server keeps pending uploads for 24 hours, and the loop
already had this kind of gap between a check and the send command. One case this makes reachable in a
new order: the server's 24-hour stale-upload sweep runs inside
attachments.createUploadUrl. With[A needs an upload, B saved more than 24 h ago and not yet swept], main checks B after A's uploadrequest has swept it and re-uploads B; this branch can check B first, then A's upload sweeps B, and the
send fails when the server claims B. Main already has the same failure for the order
[B, A], and aretry recovers in both cases.
requireUploadAccessstill runs before each attachment is used, but a verification request nowstarts without a per-item check. The server enforces the scope on
assets.createUrlanyway.already in flight and finish unread (main would have sent none for later attachments).
assets.createUrlslots with thread image URLs; those can wait about oneround trip while a batch of verifications runs.
Unchanged: result order,
pendingAttachmentIdsorder, which attachment's error is reported when severalfail (the first in attachment order), a rejected verification being thrown where the loop reaches it,
cancel returning
abandoned, and the pre-existing gap where a verified upload can still disappear beforethe server claims it.
Scope and approval
No prior issue or discussion. I am not claiming this is an obvious bug: it is a behaviour-preserving
performance fix of one underlying problem (serial verification) in one function, with no setting, UI,
protocol or default changed. If you'd rather agree on it in an Ideas discussion first, since this is a
latency improvement rather than a defect, say so and I'll move it there.
Open #12610 (upload timeout) edits
uploadFileBytesand a different part of the test file. It is alreadymarked conflicting with main. The two do not touch the same lines of
prepareTurnAttachments; the helperhere is placed away from the spot where #12610 adds its constant. With #12610 a single upload can take up
to 3 minutes, which makes difference 1 above up to 3 minutes per earlier upload, still inside the 24 hour
server keep time. Open #18631 changes one line of the same file, away from these lines.
Verification
Tests (Node 26.12.1,
vp test run --project unit src/lib/attachmentUpload.test.tsinapps/mobile): 35pass. Eight of them are new, in
describe("verifying saved uploads"), using deferred fakes for the checks:pendingAttachmentIdsstay in orderwhen they resolve in reverse;
of this passes on main and on the branch, so it pins behaviour that must not change);
abandonedwithout minting or uploading.On main (the new tests against the unchanged source), 6 of the 8 fail (overlap, bound, stop, ordering under
reverse completion, uploading while verifying, cancel) and the other 2 pass. Removing the stop flag or the
bound from the change fails a test; removing the
.catchleaves every test passing but produces 3unhandled rejections, which fail the run. Also green: the other mobile tests that import this module (6 files, 167 tests),
tsc --noEmitin
apps/mobile,vp lintandvp fmt --checkon the two files.Timing, real
prepareTurnAttachmentsand realverifyPersistedAttachmentUploadbundled with esbuild; only theWebSocket request is a timer (100 ms each, at most 6 in flight, as the transport allows). n = 5 per row,
main and branch alternating, median (min), ms:
These come from a timer model, not a phone or a real tunnel. Real savings follow the real round-trip
time plus server time. Not checked: a device, a real remote connection, Android, iOS, and the
reacttest project. Raw rows:raw/verify-m302.jsonlin the scratch folder, available on request.Claude Sonnet 5.5 and Claude Opus 5.5 in T3 Code (Claude Code harness).