Repository navigation
fix(web): put on-screen text in the snapshot tree instead of losing it - #3515
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 29 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
0da0d7e to
0ab9b9d
Compare
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
The code has one gap to close before merge: nothing tests the rule that a web snapshot publishes only backend-issued refs, as reviewed at a1d1eb8. Without that test, a later change to snapshot-state can bring back the wrong-element ref collisions from #2283 on web, and no test would fail. The only thing that stops a web capture from minting dense refs is the Not blocking, take or leave: the PR body and #3509 say the ref-less limitation is recorded in Of the open inline threads, the label denylist thread and the quoted-text escape thread are fixed at this head, as are the attachRefs doc and the target-evidence thread, so please resolve those four. The trailing value unescape thread does not apply, because the raw wire form is pinned by the tests 'matches a value across the two spellings' and 'preserves a backslash in a value the backend printed raw', so please resolve it too. I did not re-run the live before/after table and took it from the PR body. I did not run the mutation that removes CI is still running: Smoke Tests is queued, and Repo Guards, Compatibility & Provenance and Integration Tests are in progress, with no failure logs yet. A failure in Integration Tests would likely relate to this change, since it runs the changed |
a1d1eb8 to
6a883af
Compare
6a883af to
4adcae0
Compare
Split for the size budgetThe coordinator flagged this branch at 1,031 gross lines (
The split is at the seam, not through a hunk: the presentation work cannot stand apart from making Content is unchanged by the split. Against the same merge-base, the old head Review round 2Round 1's five threads are replied to and resolved, and the two fixes are on this branch: suppression of a ref-less text line is a structural relation read off the tree instead of a page-global denylist, and |
4adcae0 to
ddf73fd
Compare
|
The earlier findings at a1d1eb8 are mostly fixed at ddf73fd, and one test gap remains. The fixes cover the page-global text denylist (now the structural The ref-less path is still not pinned by a test, as in the earlier review. Only the The Cubic threads on the page-global denylist (#3515 (comment)), escaped quotes (#3515 (comment)), the I reviewed ddf73fd by reading the code and the test fixtures. I did not re-run the live before/after table on agent-browser 0.27.1, so that table is taken from the PR body. I did not run the mutation that deletes |
ddf73fd to
ae84b04
Compare
Review round 3: the ref-less path is pinned through the routerHead Required: the rule now has a test
Two notes on how it had to be written. The print assertion goes through the formatter over the router's own response body. The provider-scenario harness is a daemon request router — there is no stdout to capture — so the step asserts the response's The ref-less line is left out of the interactive capture, deliberately. The interactive arm of the scenario asks the provider Mutation-checked as the review asked: delete the Found by the new test, fixed on #3547The daemon's own projection was still adding the property back: Non-blocking, all five taken
On what you said you did not check
CI result for this commit is in the body. All five inline threads from the earlier rounds are resolved. |
ae84b04 to
67f9bfe
Compare
67f9bfe to
724d305
Compare
|
This PR is ready. The earlier findings from ddf73fd are fixed at 724d305: the structural isTextAlreadyReported check replaces the page-global text denylist, escaped quotes are handled and pinned by tests, the attachRefs doc now describes the dense and backend-issued modes, and there is one snapshotRefBodies in packages/kernel/src/snapshot.ts. Nothing remains from that round. Not blocking, and you can take or leave it: the Of the open inline threads, the four cubic threads on the text denylist, escaped quotes, the attachRefs doc and the duplicate snapshotRefBodies are fixed at this head, so please resolve them: #3515 (comment), #3515 (comment), #3515 (comment), #3515 (comment). The cubic thread on the trailing I did not run the mutation that deletes |
thymikee
added
the
ready-for-human
The web capture asked agent-browser for `--compact`, and that flag removes a ref-less element TOGETHER WITH the text under it: on one page the role census goes ListMarker 200 -> 0 and status 1 -> 0, so a `status` region that just announced something never reaches the projection at all. Measured on the page this fix was developed against: `wait text "Hello Zed"` times out on text the user can read off the screen, and `find text`/`get text` cannot answer for it either. The flag's pruning is owned here instead, by a projection that can tell an empty container from the text inside it. What survives the projection is now decided per line. Text the backend issued no ref for becomes a node carrying that text on `value`, in the ref-less shape the layer above already models (see `SnapshotRefMinting`). A string rides on `value` rather than `label` because the backend spells a control's visible string twice — as the control's name and as its own text line — and a second `label` claiming it would refuse every `label="Name"` read as ambiguous. Suppression of a text line is a structural relation read off the tree, not a page-global denylist: a line is a duplicate when it renders inside a `<label>` element whose control reports the string as its name, or sits inside the control that reports it as its name or value. Anything else keeps its own node, which is what preserves the content this capture exists to publish — an announcement that happens to repeat a button's label, or a table cell echoing a field's contents, is content. `extractQuotedText` parses and unescapes an escaped quoted payload; the trailing `: value` form stays raw, because that is what the backend prints and unescaping it would publish a value the page does not hold. Closes #3509
The one thing standing between a web capture and dense numbering was the minting-mode argument at snapshot-state.ts:83, and no test drove it through the router: every agent-browser fixture gave every node a ref, so deleting the argument minted e7 onto ref-less page text with the whole suite green (#2283 is the collision that pays for it). The scenario page now has a ref-less line as the backend prints one - text on value, no ref, no box - and four router assertions follow it: snapshot --json publishes it with no ref key, find text <it> list prints it as (no ref), wait text answers it, and click text=<it> refuses with selector_not_found.
Ref-less on-screen text is published on value, so value="X" resolves against page text that repeats a control's value and a predicate needing one element answers AMBIGUOUS_MATCH. commands.md names text= and editable=true as the way out. Also the take-it-or-leave-it round: the paragraph the body claimed for docs/agents/web-backend.md was already in commands.md, so the body points there; keyboard-occlusion's import goes back to type-only (nothing here reads a value from kernel/snapshot); WebSnapshotOptions.raw goes, since buildSnapshotArgs no longer reads it and no caller sets it; and the two runJson signature hunks that added a signal nothing passes are reverted.
724d305 to
20b6693
Compare
|
Took your non-blocking item at On your verification note, to keep the record honest: I just re-ran the mutation on this exact head. Deleting that argument at All five threads you named were already resolved (the replies landed when the fixes landed); nothing open on the PR. Comment-only delta against |
|
The code in 20b6693 looks good to me. The earlier review of 724d305 found no problems, and the logical patch is the same apart from one test comment. The dedup fix for ref-less lines (context from the nearest ref-carrying ancestor) and the escaped-quote handling in I did not run the tests or a live web session on this commit. The regression and live evidence rest on the earlier review of 724d305. I also did not re-check the live agent-browser 0.27.1 capture behind the Smoke Tests is queued, not failed. It runs device smoke lanes, and this PR has no route overlap with them: the production diff is limited to web snapshot parsing, the web provider args, and a type-only import in |
1 participant
Summary
On
--platform web, text that is on the screen reached the reader only when the backend happened to mint a ref for it. Two independent causes, both fixed here. Stacked on #3547 (feat(snapshot)!: make a node's ref optional and mint it per producer), which owns the contract and the address surfaces; this PR owns the web capture. 9 files, 750 gross lines.1. The capture asked for
--compact, and that flag deletes text along with its container. agent-browser's compaction removes a ref-less element together with the text under it. Same page, same moment:Role census on a 408-ref page, raw vs compact:
ListMarker200 → 0,status1 → 0, every ref-bearing role unchanged. The flag that exists to hide noise was what hid astatusregion that had just announced something.2. The projection dropped every line without a ref (
if (!ref) continue), so static text, a paragraph, a label element and a status announcement never became nodes at all.--compactis no longer requested. The pruning it existed to do is owned by the projection, which can tell an empty container from the text inside it: a ref-less line carrying no text (form,list,paragraph, a silentstatus) and a presentational role (listmarker) produce no node, and a dropped line's children re-parent to its parent rather than to whichever node was kept last.Text with no ref becomes a node carrying that text on
value, notlabel: the backend spells a control's visible string twice — as the control's accessible name and as its own text line — and a secondlabelclaiming it makes an ordinarylabel="Name"read refuse asambiguous. Measured through the real pipeline before choosing:text="Name"→ambiguouswhen both carry it aslabel, resolves the textbox when the text line carries it asvalue. Every text door already readsvalue(wait text,find text,get text,text=, the scope filter, the printed line).Suppression of a text line is a structural relation read off the tree, not a page-global denylist: a line is a duplicate when it renders inside a
<label>element whose control reports the string as its name, or sits inside the control that reports it as its name or value. Anything else keeps its own node, which is what preserves the content this capture exists to publish — an announcement that happens to repeat a button's label, or a table cell echoing a field's contents, is content.extractQuotedTextparses and unescapes an escaped quoted payload; the trailing: valueform stays raw, because that is what the backend prints and unescaping it would publish a value the page does not hold.A ref is now read only from a
ref=eNannotation or an@eNtoken outside quoted text:StaticText "retry in e3 seconds"names no element, and used to be handed the ref of something else.Validation
Tested at
20b6693ae, rebased on #3547e67b761af(which is rebased onorigin/main).pnpm check:affected --run --base feat/snapshot-optional-refson that head under the machine-wide gate lock:all runnable checks passed.The rule this PR exists for is now pinned through the router. The emulated web page (
test/integration/provider-scenarios/web-world.ts:232) ends with the line a real capture prints for content it cannot address — text onvalue, no ref, no box — andweb-desktop.test.tsasserts, through the request router, thatsnapshot --jsonpublishes it with norefkey,find text <it> listprints= (no ref) [text] "Order saved",wait text <it>answers it, andclick text=<it>refuses withselector_not_found. Mutation-checked: deleting thesnapshotRefMinting(data)argument atsnapshot-state.ts:83mints the nodee7and fails the first assertion. That test also found a real defect —toDaemonFindDatare-added an own-propertyrefwhose value was not a ref, which the CLI formatter, the MCP pin merge and a Node consumer all read — fixed on #3547. This head rebasesddf73fdc5onto #3547's rebased head and adds the two review commits above; the capture and the projection are unchanged by the move, and the minting mode that decides this tree's refs is unchanged for the only pair the web interactor can produce ({ backend: 'web', producer: 'agent-browser' }→backend-issued). Projection tests are driven by payloads captured from the real backend (form after submit, role vocabulary, list with markers, empty form), not hand-written approximations: the announcement keeps its text and loses its address, a label's text is subsumed under the control that carries it as its name, empty containers and list markers drop out with children re-parented, geometry attaches only to measurable nodes, and ref-shaped text stays content.Live before/after — local page on port 8766, managed agent-browser 0.27.1,
fill label="Name" Zedthenclick role=button label="Submit", baseorigin/mainvs this head:snapshot[text] "Hello Zed"wait text "Hello Zed"find text "Hello Zed" existsFound: truefind text "Hello" list= (no ref) [text] "Hello Zed"get text text="Hello Zed"Hello Zedclick text="Hello Zed"label="Name"still resolves uniquely to the textbox with a visible<label>on the page, andis visible label="Name"still works before and after a fill.Documented limitation
A ref-less node has no rect — the backend measures boxes for refs and has no handle for this content — so
is visibleanswers for it through its nearest visibility anchor (verified live:is visiblepasses,is hiddenrefuses) and it is never a tap target.snapshot -istill shows interactive elements only. Recorded inwebsite/docs/docs/commands.md, next to the iOS text-pair note that users read for the same question.Review round
All five cubic threads on this PR are replied to and resolved: four fixed in
0ab9b9ddd(structural suppression instead of a name denylist,extractQuotedTextunescaping, theattachRefsJSDoc, the kernel'ssnapshotRefBodiesreplacing the selector-side ref filter) and the fifth answered with live-backend evidence ina1d1eb862rather than changed — the trailing: valueform is raw on the wire, so unescaping it would publish avaluethe page does not hold, and the dedup it worried about already happens.The branch was then split for the 1,000-line budget: the contract and address surfaces moved to #3547, which this branch now stacks on. Content is unchanged by the split (
git diff --statacross both branches reproduces the old head's file set), and the review threads' commits remain reachable from this PR's history.Closes #3509