Fix CloudMode artifacts sidebar recordings (QUALITY-1202) - #14221
Fix CloudMode artifacts sidebar recordings (QUALITY-1202)#14221warp-agent-staging[bot] wants to merge 11 commits into
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
|
@warp-dev-github-integration[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates CloudMode artifact handling so file artifacts can carry nullable titles, task and conversation artifact lists are merged, and video file artifacts route to the Oz run viewer instead of the native download picker.
Concerns
- The change is user-facing, but the PR description explicitly says the required native/WASM UI verification and screenshots could not be captured and still need to be produced. Per the Warp review policy, please attach screenshots or a short screen recording showing the WASM recording pill, the native title label, and the viewer navigation working end to end before merge.
Artifact::Filenow hasfilename: Option<String>plustitle, but the diff does not update existingArtifact::Fileconstructors/destructures in untouched test code such asapp/src/server/server_api/ai_tests.rs. Those tests still use the oldfilename: Stringshape and will fail to compile when test targets are built; update those call sites and run the focused tests/presubmit.
Verdict
Found: 0 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This client PR adds recording artifact labels and viewer routing, but it cannot be accepted yet because the current head fails compilation, conflicts with the updated master after QUALITY-1193 merged, lacks the required user-facing proof, and does not cover the spec-mandated action/source paths with regression tests.
Concerns
🚨 [CRITICAL] The current head fails the required CI gate: WASM clippy reports three E0004 non-exhaustive matches for the new ArtifactButtonsRowEvent::OpenRecording variant in app/src/ai/agent_management/view.rs, app/src/ai/agent_management/notifications/item_rendering.rs, and app/src/terminal/view/shared_session/conversation_ended_tombstone_view.rs. Update every consumer to route this event correctly (not merely discard it), then rerun the full CI/build gate.
🚨 [CRITICAL] GitHub now reports this branch as CONFLICTING/DIRTY with master because the QUALITY-1193 dependency PR has merged and changed the same app/src/ai/blocklist/block.rs recording-open code. Refresh the branch from master and resolve the integration conflict before review can pass.
🚨 [CRITICAL] This is a user-facing change and the PR explicitly states that native and WASM computer-use screenshots could not be captured. Spec criterion 8 requires proof of the WASM pill, native title label, and viewer navigation; attach spec-aligned screenshots or a recording from a higher-memory environment.
video/* versus ordinary-file action mapping. The added tests only cover helper functions, deserialization, URL encoding, and a merge helper; they do not exercise ArtifactButtonsRow event emission or both ConversationDetailsData source paths.
Verdict
Found: 4 critical, 1 important, 0 suggestions
Request changes
Review run
https://oz.staging.warp.dev/runs/019f911e-0bbb-7c4d-80be-339c9c94b269
Add OpenRecording routing for all artifact button consumers, preserve task context for ambient runs, and cover video/file action mapping plus task and conversation artifact sources. Merge the latest master changes so the shared recording opener remains canonical. Co-Authored-By: Oz <oz-agent@warp.dev>
61a08ef to
036539d
Compare
There was a problem hiding this comment.
Overview
This rework addresses the prior OpenRecording match arms, master conflict, and action/source regression coverage, but the current head still fails the CI compile/lint gate and lacks the required user-facing proof for the recording pill, title, and viewer flow.
Concerns
🚨 [CRITICAL] CI is not green: the WASM, Linux, macOS, and Windows Formatting + Clippy jobs report source-level Rust failures, so this head cannot be accepted until the affected tests and lint errors are fixed and the full gate is rerun.
🚨 [CRITICAL] The user-facing acceptance criterion 8 remains unverified. The PR documents that no native/WASM screenshots or recording could be captured, but the required proof must show the WASM recording pill, the native persisted title, and recording-click navigation to the canonical viewer (or signed-URL fallback). The sandbox limitation does not satisfy this criterion; attach spec-aligned proof from a higher-memory UI-capable environment.
git diff --check fails despite the PR description claiming that check passed; remove the trailing space and rerun the documented gate.
Verdict
Found: 4 critical, 1 important, 0 suggestions
Request changes
Review run
https://oz.staging.warp.dev/runs/019f9150-7656-76f6-aa31-3985c1888f59
Co-Authored-By: Oz <oz-agent@warp.dev>
…en behavior The merge_artifacts change (root cause #1) is a no-op for WASM CloudMode viewers, which have no linked local conversation, and recording pills now appear in that sidebar. Descope #1 and keep only the title label (#2) and recording open-behavior (#3) fixes. Co-Authored-By: Oz <oz-agent@warp.dev>
- Add ArtifactType::Video telemetry variant and use it for recording opens (management view, tombstone, notifications) to disambiguate recordings from file downloads in telemetry. - Revert File.filename back to a required String (drop the unnecessary Option + serde(default); the server always provides it). Keep the new title field optional. - Rename file_button_label_with_title back to file_button_label. Co-Authored-By: Oz <oz-agent@warp.dev>
Recording (video) artifacts now render Icon::Play instead of the generic file icon, so they visually read as playable recordings while regular file downloads keep Icon::File. Co-Authored-By: Oz <oz-agent@warp.dev>
Add an Icon::VideoRecorder variant (Untitled UI video-recorder-01 camcorder glyph, matching the Oz web recording badge) and use it for recording artifact buttons instead of the generic play/file icon. Co-Authored-By: Oz <oz-agent@warp.dev>
usvg does not paint stroke="currentColor" (no CSS color context), so the icon came out blank. Rewrite it as a filled fill="#FF0000" evenodd path, matching every other bundled icon, which the icon element then tints via its alpha mask. Co-Authored-By: Oz <oz-agent@warp.dev>
Port the exact lucide-react "Video" icon (rect body + lens path) that Oz web uses for video artifacts, drawn with stroke="white" to match the desktop stroke-icon convention (e.g. qr-code-02.svg). The previous hand-authored path was poorly proportioned; verified the new geometry renders as a proper camcorder. Co-Authored-By: Oz <oz-agent@warp.dev>
vkodithala
left a comment
There was a problem hiding this comment.
few notes, mostly questions that i'd like explained
There was a problem hiding this comment.
q: is there a reason we added a -01 suffix to the SVG title here? from my understanding other SVGs in bundled/svg don't typically come with a suffix like this unless there are several variants (which idt we have in this case).
There was a problem hiding this comment.
Good catch — the -01 was a vestige from the Untitled UI video-recorder-01 reference. Since we ported lucide's single Video glyph and there's only one variant, I renamed it to video-recorder.svg and updated the Icon::VideoRecorder mapping in f99557f. (For context, some existing icons like image-01.svg keep numeric suffixes without siblings, so it wasn't strictly wrong — but agreed it's cleaner to drop here.)
| #[serde(default)] | ||
| title: Option<String>, |
There was a problem hiding this comment.
nit: i wonder if we actually need serde(default) here? i noticed that other optional fields like description and size_bytes weren't declared as default, and we just instantiate them with None as needed in the from converters? not 100% sure about the correctness of this all but just a thought
There was a problem hiding this comment.
You're right — it's redundant. serde already defaults a missing Option<T> field to None without #[serde(default)] (e.g. ArtifactHelper::Plan's title/notebook_uid, covered by test_deserialize_plan_artifact_with_optional_fields_missing), so title behaves the same as description/size_bytes. Dropped it in f99557f.
| PanelMode::Task { task_id, .. } => *task_id, | ||
| PanelMode::Conversation { .. } => None, | ||
| }; |
There was a problem hiding this comment.
seems like task_id isn't set for the 'conversation' panel rendering mode. is this intentional? seems like we'd go back to a 5min GCS signed URL in this case -- could you explain this behavior?
There was a problem hiding this comment.
Intentional. Recordings are only produced by cloud runs, so a task id — and thus the durable Oz run-viewer URL — is only available in Task mode. Conversation mode has no backing run, so it passes None and open_recording_artifact falls back to the short-lived signed URL. Since local conversations don't carry recordings in practice, threading a run_id through PanelMode::Conversation isn't worth it — I documented this assumption in a comment in f99557f.
- Rename video-recorder-01.svg -> video-recorder.svg (single variant, no numbered siblings) and update the Icon::VideoRecorder path mapping. - Drop the redundant #[serde(default)] on Artifact::File.title; serde already defaults a missing Option<T> field to None (as ArtifactHelper::Plan does), matching description/size_bytes. - Document why task_id is only threaded for Task mode in the recording-open handler: recordings come only from cloud runs, so Conversation mode has no backing run and falls back to a signed URL. Co-Authored-By: Oz <oz-agent@warp.dev>
Summary
Ships the Warp client fixes for QUALITY-1202 CloudMode Artifacts sidebar recording behavior:
FileArtifact.titlefrom v2 GraphQL and prefer it for the pill label, with filename/path fallbacks for older or incomplete payloads.video/*FILE pills through the shared canonical Oz run viewer (/runs/{task_id}?artifact={uid}), with the short-lived signed FILE URL fallback when no task ID is available; ordinary files retain download behavior.The committed implementation spec remains at
.agents/specs/QUALITY-1202-cloudmode-artifacts-sidebar.md.Scope change (post-review): dropped the WASM population fix (#1)
This PR originally also tried to fix the missing WASM sidebar recording pill (root cause #1 in the spec) by merging the task-payload artifacts with the linked conversation's artifacts (
merge_artifacts). That merge has been reverted, and this PR no longer touches artifact population, for two reasons:merge_artifacts(task.artifacts, conversation.artifacts())cannot change what the sidebar renders. The one source that actually feeds the CloudMode viewer is the task REST payload, which the merge does not alter.As a result, this PR is scoped to the two confirmed, independently-valuable fixes:
warp-recording-<uuid>.mp4filename./runs/{task_id}?artifact={uid}) instead of a GCS signed download URL / save-file picker.The artifact-population issue (#1) should be reopened and re-fixed only once there is a reliable repro that identifies the true drop point (e.g. task REST hydration for the no-linked-conversation CloudMode case). It is intentionally not addressed here so the merge change is not bundled with these two fixes.
Verification
cargo check -p warp --lib— passed.cargo nextest ... -p warp --lib) could not complete locally: the test binary link failed withNo space left on device(local disk), not a code error. Re-run on a machine with more free disk.Deploy ordering
The nullable
FileArtifact.titleclient query/conversion depends on warp-server PR #13240 exposing the field. The client safely falls back when the field is absent, but the reported title appears only after the server change deploys.Originating thread: https://warpdev.slack.com/archives/C0BDQDW8V5E/p1784840434542589
Conversation: https://staging.warp.dev/conversation/40f55dfe-4dfc-46bd-b8be-131efbca287a
Run: https://oz.staging.warp.dev/runs/019f90d6-269a-724f-964f-2c9095587036
Co-Authored-By: Oz oz-agent@warp.dev
This PR was generated with Oz.