perf(core): acquire snapshots concurrently#2011
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 4m 22s —— View job Review: concurrent snapshot acquisitionWent through What's good
Minor findings (non-blocking)
Not verified
Nothing here blocks merging; both findings are low-severity and non-functional. |
|
DeepScan’s UNUSED_EXPR finding was a false positive for the cursor asset: CDP Runtime.callFunctionOn consumes the file as an uninvoked functionDeclaration. Added the documented rule-specific suppression plus a purpose comment in e226a2a; the 16 acquisition tests, formatting, diff check, and Biome hook are green. |
Greptile SummarySnapshot acquisition now overlaps accessibility, cursor, document-validation, and sibling-frame CDP work while preserving deterministic assembly.
Confidence Score: 2/5The reconnect-sensitive frame-session cache and stale child-frame commit path must be corrected before this PR is safe to merge. Prior-epoch sessions remain selectable after reconnect, and child-frame loader changes discard only ref identity while retaining AX data and cursor hits acquired from the replaced document. packages/browseros-agent/crates/browseros-core/src/frames.rs, packages/browseros-agent/crates/browseros-core/src/observer/acquisition.rs Important Files Changed
Sequence DiagramsequenceDiagram
participant Observer
participant Budget as SnapshotBudget
participant AX as Accessibility
participant Runtime
participant Frames as Child Frames
Observer->>Budget: Begin capture
par Acquire AX tree
Budget->>AX: getFullAXTree
AX-->>Observer: nodes
and Scan cursor candidates
Budget->>Runtime: evaluate/callFunctionOn
Runtime-->>Observer: candidate handles
and Validate document
Budget->>Frames: getFrameTree
Frames-->>Observer: loader identities
end
par Acquire sibling frames
Observer->>Frames: acquire child A
and
Observer->>Frames: acquire child B
end
Observer->>Observer: Restore stitch order
Observer->>Observer: Sequentially render and assign refs
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/crates/browseros-core/src/frames.rs:97-102
**Stale sessions survive reconnects**
When CDP reconnects after a same-process frame session has been cached, this lookup returns the prior connection epoch's session because only `PageManager` clears its sessions. Resolving an existing frame ref then sends click, typing, scrolling, or annotation commands through an invalid session and fails with an unknown or detached-session protocol error.
### Issue 2 of 2
packages/browseros-agent/crates/browseros-core/src/observer/acquisition.rs:294-296
**Changed child snapshots remain committable**
When a child frame navigates after its AX data is acquired, revalidation reports the changed loader but only clears `document_id`; the old nodes and cursor hits are still returned and committed because capture-level stability checks cover only the main frame. The resulting snapshot exposes stale child content and actionable refs from the replaced document.
Reviews (1): Last reviewed commit: "fix: suppress intentional cursor asset e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryConcurrent snapshot acquisition is introduced while preserving sequential rendering semantics.
Confidence Score: 5/5The pull request appears safe to merge, with concurrency bounded locally and historical snapshot assembly behavior preserved. The changed acquisition paths restore deterministic stitch order before sequential ref mutation, isolate cursor markers per capture, clean remote objects on success and cancellation, and retain snapshot retry and child-failure rollback behavior. Important Files Changed
Sequence DiagramsequenceDiagram
participant Observer
participant Budget as SnapshotBudget
participant Frames as FrameRegistry
participant CDP
participant Renderer
Observer->>Budget: Read initial frame tree
par Acquire AX tree
Budget->>CDP: Accessibility.getFullAXTree
and Acquire cursor candidates
Budget->>CDP: Runtime.evaluate/callFunctionOn
Budget->>CDP: Runtime.getProperties
Budget->>CDP: DOM.describeNode (bounded)
and Validate document
Budget->>CDP: Page.getFrameTree
end
Observer->>Renderer: Render immutable frame inputs
Renderer-->>Observer: Iframe stitches
par Acquire sibling frames
Observer->>Frames: Resolve inherited target sessions
Frames->>CDP: Acquire child frame inputs
end
Observer->>Renderer: Assemble children in reverse stitch order
Observer->>Budget: Read final frame tree
Observer-->>Observer: Commit snapshot or retry
Reviews (2): Last reviewed commit: "fix: suppress intentional cursor asset e..." | Re-trigger Greptile |
|
Addressed both non-blocking Claude findings in 71871c9: expected visited-frame cycle skips no longer label sibling acquisition as partial, and cursor acquisition now deserializes Runtime.getProperties through the generated protocol result type. Formatting, workspace Clippy with warnings denied, and the full workspace test suite are green. |
✅ Tests passed — 1694/1698
|
1 participant
Summary
Design decisions
Test plan
cd packages/browseros-agent && cargo fmt --all --checkcd packages/browseros-agent && cargo clippy --workspace --all-targets -- -D warningscd packages/browseros-agent && cargo test --workspaceonclickin same-process iframe and OOPIF