Repository navigation
Agent pane: a look at a video's frames draws as a filmstrip card - #30940
Merged
Merged
Conversation
The row names what the agent looked at ("Looked at 8 frames of clip.mp4"); under it, the
video's name (plays it in Quick Look) and length, then its frames, each with its time; a frame
opens in the image viewer. Opened, the call shows the tool's note without the frames' base64.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-bbe6e8ca1bc11f92f25b
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-bbe6e8ca1bc11f92f25b
Collaborator
Author
|
Owned by an agent session · session |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 7 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="webviews/src/agent-session/acpmux/conversation/conversation.css">
<violation number="1" location="webviews/src/agent-session/acpmux/conversation/conversation.css:2484">
P2: The focus ring on a frame button in the strip sits outside the box, and the strip's `overflow-x: auto` clips it at the top and bottom edges. Draw the outline inside the frame instead.</violation>
</file>
<file name="webviews/src/agent-session/acpmux/conversation/VideoFramesCard.tsx">
<violation number="1" location="webviews/src/agent-session/acpmux/conversation/VideoFramesCard.tsx:81">
P2: When the note has no duration, this button's `aria-label` falls back to "Play video" and drops the visible file name. Screen-reader users lose which video it plays. Use the name in the fallback too.</violation>
</file>
<file name="webviews/src/agent-session/acpmux/conversation/videoFrames.ts">
<violation number="1" location="webviews/src/agent-session/acpmux/conversation/videoFrames.ts:171">
P2: `readFacts` drops bare numeric seconds because `TIME` requires a unit for decimal values. Include an optional seconds unit so notes such as `Duration: 16.4` still show the duration and frame timestamps.</violation>
<violation number="2" location="webviews/src/agent-session/acpmux/conversation/videoFrames.ts:178">
P2: `numberAt` parses numeric strings as seconds even when callers use it for millisecond fields and pixel dimensions. Parse millisecond and dimension strings as plain numbers, while retaining time parsing only for second-based fields.</violation>
<violation number="3" location="webviews/src/agent-session/acpmux/conversation/videoFrames.ts:230">
P2: A frame line without a time is dropped from `facts.times`, so every later timestamp shifts onto the previous frame. Push `undefined` for a frame line whose time is missing, so the index stays aligned with the image.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Comment on lines
+2484
to
+2487
| button.cv-video-frames__frame:focus-visible { | ||
| outline: 2px solid var(--agent-accent-soft); | ||
| outline-offset: 2px; | ||
| } |
There was a problem hiding this comment.
P2: The focus ring on a frame button in the strip sits outside the box, and the strip's overflow-x: auto clips it at the top and bottom edges. Draw the outline inside the frame instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/conversation/conversation.css, line 2484:
<comment>The focus ring on a frame button in the strip sits outside the box, and the strip's `overflow-x: auto` clips it at the top and bottom edges. Draw the outline inside the frame instead.</comment>
<file context>
@@ -2378,6 +2378,136 @@ pre.cv-math-source {
+ appearance: none;
+ cursor: zoom-in;
+}
+button.cv-video-frames__frame:focus-visible {
+ outline: 2px solid var(--agent-accent-soft);
+ outline-offset: 2px;
</file context>
Suggested change
| button.cv-video-frames__frame:focus-visible { | |
| outline: 2px solid var(--agent-accent-soft); | |
| outline-offset: 2px; | |
| } | |
| button.cv-video-frames__frame:focus-visible { | |
| outline: 2px solid var(--agent-accent-soft); | |
| outline-offset: -2px; | |
| } |
| type="button" | ||
| className="cv-video-frames__name is-playable" | ||
| title={video.path} | ||
| aria-label={length ? t("composer.playVideo", { name, length }) : t("media.play")} |
There was a problem hiding this comment.
P2: When the note has no duration, this button's aria-label falls back to "Play video" and drops the visible file name. Screen-reader users lose which video it plays. Use the name in the fallback too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/conversation/VideoFramesCard.tsx, line 81:
<comment>When the note has no duration, this button's `aria-label` falls back to "Play video" and drops the visible file name. Screen-reader users lose which video it plays. Use the name in the fallback too.</comment>
<file context>
@@ -0,0 +1,127 @@
+ type="button"
+ className="cv-video-frames__name is-playable"
+ title={video.path}
+ aria-label={length ? t("composer.playVideo", { name, length }) : t("media.play")}
+ onClick={() => void callChipHost("video.quickLook", { src: video.path! })}
+ >
</file context>
Suggested change
| aria-label={length ? t("composer.playVideo", { name, length }) : t("media.play")} | |
| aria-label={length ? t("composer.playVideo", { name, length }) : name} |
| const rest = frame?.[1] ?? (bullet ? bullet[1] : undefined); | ||
| if (rest !== undefined && !/\b(duration|length)\b/.test(lower)) { | ||
| const time = TIME.exec(rest); | ||
| if (time) facts.times.push(parseTime(time[1]!)); |
There was a problem hiding this comment.
P2: A frame line without a time is dropped from facts.times, so every later timestamp shifts onto the previous frame. Push undefined for a frame line whose time is missing, so the index stays aligned with the image.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/conversation/videoFrames.ts, line 230:
<comment>A frame line without a time is dropped from `facts.times`, so every later timestamp shifts onto the previous frame. Push `undefined` for a frame line whose time is missing, so the index stays aligned with the image.</comment>
<file context>
@@ -0,0 +1,295 @@
+ const rest = frame?.[1] ?? (bullet ? bullet[1] : undefined);
+ if (rest !== undefined && !/\b(duration|length)\b/.test(lower)) {
+ const time = TIME.exec(rest);
+ if (time) facts.times.push(parseTime(time[1]!));
+ }
+ }
</file context>
Suggested change
| if (time) facts.times.push(parseTime(time[1]!)); | |
| if (time || frame) facts.times.push(time ? parseTime(time[1]!) : undefined); |
| const value = record[key]; | ||
| if (typeof value === "number" && Number.isFinite(value) && value >= 0) return value; | ||
| if (typeof value === "string") { | ||
| const time = parseTime(value); |
There was a problem hiding this comment.
P2: numberAt parses numeric strings as seconds even when callers use it for millisecond fields and pixel dimensions. Parse millisecond and dimension strings as plain numbers, while retaining time parsing only for second-based fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/conversation/videoFrames.ts, line 178:
<comment>`numberAt` parses numeric strings as seconds even when callers use it for millisecond fields and pixel dimensions. Parse millisecond and dimension strings as plain numbers, while retaining time parsing only for second-based fields.</comment>
<file context>
@@ -0,0 +1,295 @@
+ const value = record[key];
+ if (typeof value === "number" && Number.isFinite(value) && value >= 0) return value;
+ if (typeof value === "string") {
+ const time = parseTime(value);
+ if (time !== undefined) return time;
+ }
</file context>
| return Math.round(unit[2] === "ms" ? value : value * 1000); | ||
| } | ||
|
|
||
| const TIME = /(\d+:\d{1,2}(?::\d{1,2})?(?:\.\d+)?|\d+(?:\.\d+)?\s*(?:ms|s|sec|secs|seconds?)\b)/i; |
There was a problem hiding this comment.
P2: readFacts drops bare numeric seconds because TIME requires a unit for decimal values. Include an optional seconds unit so notes such as Duration: 16.4 still show the duration and frame timestamps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/conversation/videoFrames.ts, line 171:
<comment>`readFacts` drops bare numeric seconds because `TIME` requires a unit for decimal values. Include an optional seconds unit so notes such as `Duration: 16.4` still show the duration and frame timestamps.</comment>
<file context>
@@ -0,0 +1,295 @@
+ return Math.round(unit[2] === "ms" ? value : value * 1000);
+}
+
+const TIME = /(\d+:\d{1,2}(?::\d{1,2})?(?:\.\d+)?|\d+(?:\.\d+)?\s*(?:ms|s|sec|secs|seconds?)\b)/i;
+
+function numberAt(record: Record<string, unknown>, keys: string[]): number | undefined {
</file context>
Suggested change
| const TIME = /(\d+:\d{1,2}(?::\d{1,2})?(?:\.\d+)?|\d+(?:\.\d+)?\s*(?:ms|s|sec|secs|seconds?)\b)/i; | |
| const TIME = /(\d+:\d{1,2}(?::\d{1,2})?(?:\.\d+)?|\d+(?:\.\d+)?\s*(?:ms|s|sec|secs|seconds?)?\b)/i; |
|
Found 1 test failure on Blacksmith runners: Failure
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Contributor
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Gallery before/after for
States
Pick other states with a red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: Shipped in:
|
…w's words The row reads "Looked at 8 video frames" and the card under it names the video (plays it), so the name no longer shows twice. The card starts at the row's text column like any line under a row; frames carry no list roles on buttons; covers name the layout context; formatter pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-bbe6e8ca1bc11f92f25b
teamleaderleo
temporarily deployed
to
ci-routing-agent
November 1, 2026 13:47 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
The tool's header line (<path>: 16.40 s, 1920x1080, 30 fps, ...) gives the resolved path, length and size, and its #N at T s lines each frame's time. Frames are never deduped (a still screen's identical frames keep their times), the label counts every frame the call returned, a size inside a file name and a line that starts with a decimal are not read as facts, and the frame buttons take an aria-label instead of a tooltip (review on 246656c). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-bbe6e8ca1bc11f92f25b
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
teamleaderleo
temporarily deployed
to
ci-routing-agent
November 1, 2026 14:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="webviews/src/agent-session/acpmux/conversation/videoFrames.ts">
<violation number="1" location="webviews/src/agent-session/acpmux/conversation/videoFrames.ts:238">
P3: The narrowed size lookbehind drops `WxH` written right after `:`, `=`, `-` or `/` (for example `size:1920x1080`), which the old `\b` matched. Those notes lose width and height, and the card falls back to a 16:9 box. File names are already excluded by the `VIDEO_NAME` replacement, so the stricter prefix is unnecessary. Use a lookbehind that rejects only word characters and dots.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| } | ||
| if (facts.width === undefined) { | ||
| // Not in a path or a file name (`clip_1280x720.mp4`). | ||
| const size = /(?:^|[\s,(·])(\d{2,5})\s*[x×]\s*(\d{2,5})\b/.exec(line.replace(VIDEO_NAME, " ")); |
There was a problem hiding this comment.
P3: The narrowed size lookbehind drops WxH written right after :, =, - or / (for example size:1920x1080), which the old \b matched. Those notes lose width and height, and the card falls back to a 16:9 box. File names are already excluded by the VIDEO_NAME replacement, so the stricter prefix is unnecessary. Use a lookbehind that rejects only word characters and dots.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At webviews/src/agent-session/acpmux/conversation/videoFrames.ts, line 238:
<comment>The narrowed size lookbehind drops `WxH` written right after `:`, `=`, `-` or `/` (for example `size:1920x1080`), which the old `\b` matched. Those notes lose width and height, and the card falls back to a 16:9 box. File names are already excluded by the `VIDEO_NAME` replacement, so the stricter prefix is unnecessary. Use a lookbehind that rejects only word characters and dots.</comment>
<file context>
@@ -215,16 +234,16 @@ function readFacts(text: string): Facts {
if (facts.width === undefined) {
- const size = /\b(\d{2,5})\s*[x×]\s*(\d{2,5})\b/.exec(line);
+ // Not in a path or a file name (`clip_1280x720.mp4`).
+ const size = /(?:^|[\s,(·])(\d{2,5})\s*[x×]\s*(\d{2,5})\b/.exec(line.replace(VIDEO_NAME, " "));
if (size) {
facts.width = Number(size[1]);
</file context>
Suggested change
| const size = /(?:^|[\s,(·])(\d{2,5})\s*[x×]\s*(\d{2,5})\b/.exec(line.replace(VIDEO_NAME, " ")); | |
| const size = /(?<![\w.])(\d{2,5})\s*[x×]\s*(\d{2,5})\b/.exec(line.replace(VIDEO_NAME, " ")); |
Collaborator
Author
|
Review (subagent claude-opus-5-5 medium): SHIP at e115316 An earlier BLOCK at 246656c (tooltip rule on the new card) is fixed at this head. Follow-ups:
|
Collaborator
Author
This branch was previously deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
When an agent looks at a video's frames through the cmux
video_framesagent tool, the transcript now draws a compact video card instead of the raw call:0:02, or0:02.5when whole seconds would repeat).video.quickLook), and the host checks the path the same way. In a read-only (guest) transcript the name is plain text.Every frame box takes the video's shape before it decodes, so the transcript never shifts. With many frames the strip scrolls sideways at the row's width instead of growing taller.
Leo, 2026-11-01, looking at two screenshots of an agent's look at an attached video (a Shell card with a long ffmpeg command and the full Chat Uploads path printed twice, then
Read /tmp/vidframes.../grid.pngwith a contact sheet): "i feel like cmux-next should give u better tools to deal with this kind of thing ... you can see what we can work on", then "can you help work on all these".Source shape
The
video_framestool is #30921 (open).videoFrames.tsreads its exact output, and the gallery and a test use that wording. The output is oneimage/jpegblock per frame and one text block. The text's header line is<path>: 16.40 s, 1920x1080, 30 fps, no audio. 8 frames below, ..., which gives the resolved path, the length and the size. Each frame then has a#N at T sline with its time. Frames may arrive as ACP image blocks (tool.images) or inside an MCP envelope in the output, and are never deduped, so a still screen's identical frames keep their times. Other wordings still parse:key: valuenotes, numbered lists and JSON. Detection uses the MCP tool name (mcp__<server>__video_frames, or Codex's{server, tool}). If a call's frames can't be found, it draws exactly as before.It's parsed once per immutable tool snapshot (a WeakMap), never per render or per streamed token. The card adds no timer, observer or animation loop. Frame images use
loading="lazy".Not changed
McpCallRowchanges, and only for avideo_framescall.videoPrompt.ts). Its stated design is that the bubble draws the video's own tile and never the frames, so this PR leaves the bubble alone.Gallery
agent-pane.video-frames(VideoFramesCard.gallery.tsx), built from the real ACP updates throughmergeToolItemand drawn by the realMcpCallRows:strip(chosen)gridposterstrip-manystrip-unnamedWhy
strip: it's what was asked for (a filmstrip). It's the most compact, under 90px tall at any frame count, so a look at a video takes about as much room as one image strip and never pushes the turn's reply off screen. It also reads in time order left to right.gridgets tall fast at 8 to 16 frames, andposterrepeats the first frame at a size the image viewer already gives on click.Design spec:
design/chat-transcript.md(leo-workspace), tool rows. No settled design is replaced.Tests
videoFrames.test.ts: Claude and Codex call shapes, an MCP envelope with a JSON note, other note wordings and paths with spaces, untimed frames, and non-calls (other tools, running, failed, frameless, a shellffmpeg).cmux-ci test): the unit tests, gallery coverage, typecheck, design tokens, catalog keys, fmt and lint on the changed files.tools.videoFrames(a plural) andmedia.videoFrameAt. The play button reusescomposer.playVideo.Follow-ups
video_framescalls in a row group under the generic MCP group line, and their cards show only once the group is opened. A call made inside acmux_execscript gets no card./tmp). The refusal is silent.🤖 Generated with Claude Code
Agent session: agent session