Repository navigation
fix(tv): auto-describe after tv-remote, and on Android TV and Vega #1262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
16dd5eb
798c773
03a628f
1200c3a
32c5905
7e9523c
e114998
00672ef
1118b17
cfe2854
733c631
38723ce
015b62c
64ffac0
0141592
6f7d446
fb3eb17
4fd1357
5a9c0ac
2b7e9cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ export const AUTO_SCREENSHOT_TOOLS = new Set([ | |
| "paste", | ||
| "rotate", | ||
| "fold", | ||
| "tv-remote", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A secret typed into a plain text field on a TV now shows up in the next
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Not changing this. The capture after a secret is the same on every platform: on a phone the next |
||
| "launch-app", | ||
| "restart-app", | ||
| "open-url", | ||
|
|
@@ -44,6 +45,7 @@ export const AUTO_SCREENSHOT_DELAY_MS_BY_TOOL: Record<string, number> = { | |
| "gesture-rotate": 1500, | ||
| "run-sequence": 15000, | ||
| "button": 1500, | ||
| "tv-remote": 1500, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: On an Apple TV simulator, every MCP
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| "rotate": 1000, | ||
| // The fold tool already waits for the panel hand-over; the cap covers the | ||
| // repaint after it. | ||
|
|
@@ -80,6 +82,7 @@ export const AUTO_DESCRIBE_TOOLS = new Set([ | |
| "paste", | ||
| "rotate", | ||
| "fold", | ||
| "tv-remote", | ||
|
latekvo marked this conversation as resolved.
|
||
| "launch-app", | ||
| "restart-app", | ||
| "open-url", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,6 +173,14 @@ describe("autoDescribeEnabled", () => { | |
| }); | ||
| }); | ||
|
|
||
| describe("auto-capture after tv-remote", () => { | ||
| it("captures the screen and the focus tree after a TV remote press", () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This checks that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Not adding an |
||
| expect(shouldAutoScreenshot("mcp__argent__tv-remote")).toBe(true); | ||
| expect(shouldAutoDescribe("mcp__argent__tv-remote")).toBe(true); | ||
| expect(AUTO_SCREENSHOT_DELAY_MS_BY_TOOL["tv-remote"]).toBe(1500); | ||
| }); | ||
| }); | ||
|
|
||
| describe("shouldAutoDescribe", () => { | ||
| it("is true for every listed tool, with or without a client prefix", () => { | ||
| for (const tool of AUTO_DESCRIBE_TOOLS) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import { withKeyedLock } from "./keyed-lock"; | ||
|
|
||
| // Keyed by adb serial. Overlapping `uiautomator dump`s on one device get one of | ||
| // them `Killed`, and a timed-out wait tool leaves its dump running. | ||
| const uiautomatorDumpLocks = new Map<string, Promise<unknown>>(); | ||
|
|
||
| export function withUiautomatorLock<T>(serial: string, fn: () => Promise<T>): Promise<T> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing cancels a queued dump. A wait that gives up still has its dump run later, and every later read on that device queues behind it.
This only matters when the helper is unavailable and reads fall back to the dump, but then it adds up. Several back-to-back A unit-level run with five 50 ms waits over 200 ms dumps made the following
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Not changing this. The trade-off is intended. Without the lock, the dump a timed-out wait leaves behind overlaps the next read, and one of them dies |
||
| return withKeyedLock(uiautomatorDumpLocks, serial, fn); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { readFileSync } from "node:fs"; | ||
| import { join } from "node:path"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| const fetchVegaPageSource = vi.fn(); | ||
| vi.mock("../src/utils/vega-inspect", () => ({ | ||
| fetchVegaPageSource: (...a: unknown[]) => fetchVegaPageSource(...a), | ||
| })); | ||
|
|
||
| import { createAwaitScreenIdleTool } from "../src/tools/await-screen-idle"; | ||
| import { __primeDepCacheForTests, __resetDepCacheForTests } from "../src/utils/check-deps"; | ||
|
|
||
| const PAGE_SOURCE = readFileSync(join(__dirname, "fixtures", "vega-page-source.xml"), "utf8"); | ||
| const VEGA_SERIAL = "amazon-4e311aa3932a35be"; | ||
|
|
||
| describe("await-screen-idle on Vega", () => { | ||
| beforeEach(() => { | ||
| fetchVegaPageSource.mockReset(); | ||
| __resetDepCacheForTests(); | ||
| __primeDepCacheForTests(["adb"]); | ||
| }); | ||
|
|
||
| it("settles by polling the Vega automation toolkit", async () => { | ||
| fetchVegaPageSource.mockResolvedValue(PAGE_SOURCE); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mock returns the identical page source on every poll, so this proves routing, the capability and the adb preflight, not settling. No case covers:
The "toolkit unreachable" case would also pass if
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat, Opus 5.5]: Partly fixed in 00672ef. A new case alternates two page sources (one label changed) and expects |
||
| const tool = createAwaitScreenIdleTool({} as any); | ||
|
|
||
| const result = await tool.execute( | ||
| {}, | ||
| { udid: VEGA_SERIAL, timeoutMs: 2000, pollIntervalMs: 10, minStableMs: 20 } | ||
| ); | ||
|
|
||
| expect(result.settled).toBe(true); | ||
| expect(result.polls).toBeGreaterThan(1); | ||
| expect(fetchVegaPageSource).toHaveBeenCalledTimes(result.polls); | ||
| }); | ||
|
|
||
| it("does not settle while the toolkit is unreachable", async () => { | ||
| fetchVegaPageSource.mockRejectedValue(new Error("ECONNREFUSED")); | ||
| const tool = createAwaitScreenIdleTool({} as any); | ||
|
|
||
| const result = await tool.execute( | ||
| {}, | ||
| { udid: VEGA_SERIAL, timeoutMs: 60, pollIntervalMs: 10, minStableMs: 20 } | ||
| ); | ||
|
|
||
| expect(result.settled).toBe(false); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every Apple TV screenshot is written to
$TMPDIR/argent-tv-screenshot-<udid8>-<hrtime>.png(tools/screenshot/index.ts:139-142), and nothing in the repo deletes it. Withtv-remotecaptured, every remote press on Apple TV now leaves one more file; they were 10-600 KB each at the default scale in my run. Before this PR, only explicit screenshots,launch-appandkeyboardwrote them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat, Opus 5.5]: Confirmed, and it is older than this PR: every backend that writes its own PNG into
os.tmpdir()has it. On this machine$TMPDIRholds 380vega-screenshot-*and 57argent-tv-screenshot-*files, and ios-remote writesargent-remote-screenshots/<uuid>.pngthe same way. Nothing unlinks a registered artifact. The registry store is an in-memory map with no eviction. Withtv-remotecaptured, the per-press rate goes up, so I filed it as #1277 rather than patching only the Apple TV path here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat, Opus 5.5]: Still present as of 5a9c0ac. Each
tv-remotepress on Apple TV or Vega writes one PNG underos.tmpdir()that is never removed. Tracked in #1277, which is still open.