feat(mobile): preview first-message cloud-agent attachments - #5412
Conversation
Parse the wrapper's sandbox file:// URL into a messageUuid+filename reference and store it as a ref-only cache entry, so a later slice can presign a fresh R2 download URL on demand. Add overwrite and resolve-failed helpers with store-driven failure state.
Add an on-demand resolver that presigns a fresh R2 GET for captured attachment refs and feeds the existing preview modal and image viewer. Wire the renderer to show busy, error-with-retry, and unavailable states, and re-presign on retry so 15-minute URL expiry recovers.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of markdown tap-during-presign failure toast, image retry coverage, and comment-only cache/parser notes; no high-confidence bugs. Files Reviewed (4 files)
Previous Review Summaries (6 snapshots, latest commit 1aaf16b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1aaf16b)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of markdown tap-during-presign failure toast, image retry coverage, and comment-only cache/parser notes; no high-confidence bugs. Files Reviewed (4 files)
Previous review (commit 5952f22)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of remount URL reuse, image-presign retry coverage, pressable feedback, and ingest-frame trim-contract tests; no high-confidence bugs. Files Reviewed (3 files)
Previous review (commit 2c8b8cf)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of live-stream Files Reviewed (2 files)
Previous review (commit 0501000)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of live-stream Files Reviewed (2 files)
Previous review (commit c3d1614)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the cache Files Reviewed (3 files)
Previous review (commit 1078afd)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the mobile file-part cache, sandbox URL parser, on-demand R2 presign resolver, and renderer retry states; no high-confidence bugs, security issues, or leaks on the first-message attachment path. Files Reviewed (7 files)
Reviewed by grok-4.6 · Input: 125.5K · Output: 6.1K · Cached: 257.2K Review guidance: REVIEW.md from base branch |
The cache hook snapshotted a version counter and ignored it, so a post-mount overwrite (the on-demand presign) did not reliably re-render subscribers and the chip stayed busy. Snapshot the entry object instead, and open the markdown modal once the URL lands when the chip is tapped while the presign is in flight.
The wrapper's live-stream trim stripped every FilePart url to '', so the mobile onFilePart sink never saw the raw file:// sandbox path on the live path. The attachment reference was only captured from the history replay, which races the async part ingestion. Keep non-data: urls and strip only data: urls (inline base64) plus source.text.value.
The wrapper's live-stream trim stripped every data: url to '', but the Kilo CLI inlines image file parts as data:, so the live first-message image never reached the mobile client and showed 'Image unavailable'. Preserve small data: urls (<=512KiB) on top-level file parts; tool attachments and larger data: urls still strip for size.
The live-stream trim now preserves small inline data: URLs on top-level file parts. Update the ingest-frame test to assert the new contract: a small data: URL is preserved, a large one is stripped, and source.text.value is always stripped.
Add a remount test proving the module-level cache survives unmount/remount without a second presign, and an image presign-failure test proving the retry row re-presigns and renders the inline image.
The two image retry Pressables lacked active opacity feedback. Add active:opacity-80 to match the chip Pressable.
- Show the retry toast and reset preview when a markdown tap during the presign is followed by a presign failure, instead of a silent no-op. - Add mounted tests for the imageFailed retry press with and without an attachment ref, and for the markdown presign-failure leg. - Correct the attachment path comment (sessionId, not agentId) and the resolveCacheUrl failure-mode comment (undefined return, not rejection).
clearFilePartResolveFailed writes a new entry and emits, so the effect re-runs via the cached dep; the attempt counter is a dead second trigger.
| messageUuid: attachmentRef.messageUuid, | ||
| filename: attachmentRef.filename, | ||
| }); | ||
| overwriteFilePartCacheEntry(part.id, { |
There was a problem hiding this comment.
bot: Presigning an attachment from an uncached part.url drops the reference needed for later retries.
Suggested fix: Carry attachmentRef into the cache write (for example, allow overwriteFilePartCacheEntry to accept it, or seed a ref-only entry before the mutation). Today the overwrite preserves a reference only if an entry already existed. On the uncached fallback path, the first image/text preview works, but after its signed URL expires or an image/text load fails, refreshFilePartUrl(part.id) sees an entry without attachmentRef and returns false. Add a regression test that mounts a sandbox part.url without calling cacheFilePart, resolves it, then verifies a later refresh re-presigns successfully.
| (!options?.preserveSmallDataUrls || url.length > MAX_INLINE_FILE_URL_LENGTH); | ||
| const out: Record<string, unknown> = { | ||
| ...part, | ||
| ...(stripDataUrl ? { url: '' } : {}), |
There was a problem hiding this comment.
bot: The new preservation rule forwards every non-data: URL scheme, not just the intended sandbox file:// and HTTP(S) URLs.
Suggested fix: Replace the data-only stripping condition with an explicit allowlist: preserve only the validated sandbox attachment file:// shape, http:///https://, and eligible small data: URLs; blank all other schemes. Add tests for unsupported values such as ftp: and case-variant/other schemes. As written, arbitrary non-data URLs are included in the ingest payload, widening the prior behavior that always removed file-part URLs and exceeding the stated retention contract.
2 participants
Summary
The live stream no longer blanks every file-part URL. It now keeps
file://sandbox paths,http(s)URLs, anddata:URLs up to 512 KiB on top-level file parts, and it still strips largerdata:URLs and the source text. The newMAX_INLINE_FILE_URL_LENGTHlimit is half the ingest-frame byte budget, so one inline file cannot alone overflow a frame.Files
services/cloud-agent-next/src/shared/trim-payload.ts— addsMAX_INLINE_FILE_URL_LENGTH;stripFilePartFieldskeeps non-data:URLs and small top-leveldata:URLs, strips largedata:URLs, and still blankssource.text.value.A new parser turns the wrapper's sandbox attachment URL into a
CloudAgentAttachmentRefwith the message UUID and filename. The pattern accepts only the exact sandbox path shape, one UUID segment followed by one filename segment, so unrelated file URLs do not match. History keeps this URL form forever, so the parser must handle both live events and replays.Files
apps/mobile/src/components/agents/file-part-preview.ts— adds theCloudAgentAttachmentReftype andparseCloudAgentAttachmentUrl.The cache entry contract changes:
urlbecomes optional, andattachmentRefplusresolveFailedjoin the shape. A sandbox attachment URL now stores a reference-only entry with no URL key, and three new helpers overwrite the URL, mark a failed presign, and clear that mark. First write still wins, anddata:URLs still write to disk.Files
apps/mobile/src/components/agents/file-part-cache.ts— makesurloptional, addsattachmentRefandresolveFailed, addsoverwriteFilePartCacheEntry,markFilePartResolveFailed, andclearFilePartResolveFailed, and changesuseFilePartCacheto snapshot the entry object.The new
useResolvedFilePartUrlhook owns URL resolution for a file part. It returns a capturedhttp(s)ordata:URL as-is and presigns a sandbox attachment through the existinggetAttachmentDownloadUrlendpoint on demand. A module-level set dedupes in-flight presigns by part id, andrefreshFilePartUrlre-presigns and returns false instead of throwing.Files
apps/mobile/src/components/agents/file-part-url-resolver.ts— addsuseResolvedFilePartUrlandrefreshFilePartUrl; exposesready,resolving,unavailable, anderrorstates.The renderer now consumes the resolver's state instead of reading the cache directly. Image parts get loading, error, and retry rows, the preview modal gets an
onRetryprop that re-presigns before re-reading, and the chip shows a busy spinner while resolution runs.Files
apps/mobile/src/components/agents/file-part-renderer.tsx— drops theresolveUsableUrlhelper, usesuseResolvedFilePartUrl, adds the image states, and passesonRetryto the modal.Tests: 5 files changed — apps/mobile/src/components/agents/file-part-cache.test.ts, apps/mobile/src/components/agents/file-part-preview.test.ts, apps/mobile/src/components/agents/file-part-renderer.mounted.test.tsx, services/cloud-agent-next/src/shared/ingest-frame.test.ts, services/cloud-agent-next/src/shared/trim-payload.test.ts.
Generated: none.
Verification
On iOS, four scenarios ran across three verify rounds and the g4 takeover round.
Defects reproduced on the unfixed build:
Preview unavailable, no modal (repro R1). Fixed; no longer reproduces.Image unavailable, no viewer (repro R2). Fixed; no longer reproduces.Preview unavailable(repro R3). Fixed; no longer reproduces.Recordings: repro-r0-ios.mp4, repro-r1-ios.mp4.
Visual Changes
Markdown preview modal. Tapping the markdown chip opens a modal that renders the file text, not the
Preview unavailabletoast. TheRepro briefheading and body line sit under the title bar; the Done button is top right.Full-screen image viewer. Tapping the inline image opens a full-screen viewer that shows the PNG, not the
Image unavailablerow. The red PNG spans the width on a black backdrop; the close button is top left.Reviewer Notes
Human steps: none known.
Notes: none.