Repository navigation
feat: stabilize snapshot refs - #1272
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 5m 53s —— View job Code Review
Overall this is well-structured work — the two-tier ref resolution design is elegant and the URL stability guard is robust. A few issues to flag. Bug:
|
❌ Tests failed — 2/1303 failed
Failed tests
|
Greptile SummaryThis PR introduces stable
Confidence Score: 4/5Safe to merge; the core ref-stability logic is correct and well-tested. Two minor cleanup items in observer.ts worth addressing before or shortly after merge. The packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts — the redundant second Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts:278-294
**Redundant second `documents.set` call in `collectFrameDocuments`**
For child frames, both `set` calls write to the same key: `documents.set(node.frame.id, documentId)` on both line 1 (when `isRoot=false`) and line 2 — the second is an exact duplicate. For the root frame, line 2 stores an entry keyed by the root's literal `frame.id`, which is never looked up — `stableDocumentIdForFrame` and all call-sites access the root via `frameId = undefined`, not by its string id. Both cases make the second `documents.set` dead code, which violates the team rule on removing unused code.
### Issue 2 of 3
packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts:204-219
**O(N child frames) extra `getFrameTree()` CDP calls per snapshot**
`stableDocumentIdForFrame` issues a fresh `getFrameTree()` round-trip for every non-root child frame in order to confirm its document hasn't churned. A page with N iframes therefore makes N+2 `getFrameTree()` calls per snapshot cycle (the two outer `readMainFrameState` calls plus one per frame). A simple improvement would be to call `readFrameDocuments` once after all frames are rendered and pass that "after" map down, rather than re-fetching per frame.
### Issue 3 of 3
packages/browseros-agent/apps/server/src/browser/core/snapshot/refs.ts:31-44
The `fork.byStableNode.clear()` and `fork.stableRefs.clear()` calls are no-ops on the freshly-constructed `RefMap` (its collections are already empty). They can be removed to reduce noise.
```suggestion
forkForSnapshot(): RefMap {
const fork = new RefMap()
fork.nextRefNum = this.nextRefNum
for (const [key, ref] of this.byStableNode) {
fork.byStableNode.set(key, ref)
}
for (const ref of this.stableRefs) {
fork.stableRefs.add(ref)
}
fork.beginSnapshot()
return fork
}
```
Reviews (1): Last reviewed commit: "feat: stabilize snapshot refs by documen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function collectFrameDocuments( | ||
| tree: FrameTreeNode, | ||
| ): Map<FrameId | undefined, DocumentId> { | ||
| const documents = new Map<FrameId | undefined, DocumentId>() | ||
|
|
||
| function visit(node: FrameTreeNode, isRoot: boolean): void { | ||
| const documentId = frameDocumentId(node.frame) | ||
| if (documentId !== undefined) { | ||
| documents.set(isRoot ? undefined : node.frame.id, documentId) | ||
| documents.set(node.frame.id, documentId) | ||
| } | ||
| for (const child of node.childFrames ?? []) visit(child, false) | ||
| } | ||
|
|
||
| visit(tree, true) | ||
| return documents | ||
| } |
There was a problem hiding this comment.
documents.set call in collectFrameDocuments
For child frames, both set calls write to the same key: documents.set(node.frame.id, documentId) on both line 1 (when isRoot=false) and line 2 — the second is an exact duplicate. For the root frame, line 2 stores an entry keyed by the root's literal frame.id, which is never looked up — stableDocumentIdForFrame and all call-sites access the root via frameId = undefined, not by its string id. Both cases make the second documents.set dead code, which violates the team rule on removing unused code.
Rule Used: Remove unused/dead code rather than leaving it in ... (source)
Learned From
browseros-ai/BrowserOS-agent#126
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts
Line: 278-294
Comment:
**Redundant second `documents.set` call in `collectFrameDocuments`**
For child frames, both `set` calls write to the same key: `documents.set(node.frame.id, documentId)` on both line 1 (when `isRoot=false`) and line 2 — the second is an exact duplicate. For the root frame, line 2 stores an entry keyed by the root's literal `frame.id`, which is never looked up — `stableDocumentIdForFrame` and all call-sites access the root via `frameId = undefined`, not by its string id. Both cases make the second `documents.set` dead code, which violates the team rule on removing unused code.
**Rule Used:** Remove unused/dead code rather than leaving it in ... ([source](https://app.greptile.com/browseros-org-2/-/custom-context?memory=9b045db4-2630-428c-95b7-ccf048d34547))
**Learned From**
[browseros-ai/BrowserOS-agent#126](https://github.com/browseros-ai/BrowserOS-agent/pull/126)
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| private async stableDocumentIdForFrame( | ||
| rootSession: ProtocolApi, | ||
| frameId: FrameId | undefined, | ||
| frameDocuments: Map<FrameId | undefined, DocumentId>, | ||
| ): Promise<DocumentId | undefined> { | ||
| const beforeDocumentId = frameDocuments.get(frameId) | ||
| if (frameId === undefined || beforeDocumentId === undefined) { | ||
| return beforeDocumentId | ||
| } | ||
|
|
||
| const latest = await this.readFrameDocuments(rootSession).catch( | ||
| () => undefined, | ||
| ) | ||
| const afterDocumentId = latest?.get(frameId) | ||
| return afterDocumentId === beforeDocumentId ? beforeDocumentId : undefined | ||
| } |
There was a problem hiding this comment.
getFrameTree() CDP calls per snapshot
stableDocumentIdForFrame issues a fresh getFrameTree() round-trip for every non-root child frame in order to confirm its document hasn't churned. A page with N iframes therefore makes N+2 getFrameTree() calls per snapshot cycle (the two outer readMainFrameState calls plus one per frame). A simple improvement would be to call readFrameDocuments once after all frames are rendered and pass that "after" map down, rather than re-fetching per frame.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts
Line: 204-219
Comment:
**O(N child frames) extra `getFrameTree()` CDP calls per snapshot**
`stableDocumentIdForFrame` issues a fresh `getFrameTree()` round-trip for every non-root child frame in order to confirm its document hasn't churned. A page with N iframes therefore makes N+2 `getFrameTree()` calls per snapshot cycle (the two outer `readMainFrameState` calls plus one per frame). A simple improvement would be to call `readFrameDocuments` once after all frames are rendered and pass that "after" map down, rather than re-fetching per frame.
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryThis PR stabilizes snapshot refs by reusing
Confidence Score: 4/5The change is safe to merge. All new behaviour is covered by tests, the fork/commit design prevents failed captures from corrupting committed refs, and navigation correctly resets the ref namespace. The core logic is sound and well-tested. A few style issues were found: redundant refs.ts and observer.ts are the two files with the noted issues; the test files and render.ts are clean. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent
participant Observer
participant RefMap
participant CDP as CDP (Page/AX)
Agent->>Observer: snapshot()
Observer->>CDP: Page.getFrameTree() [before]
CDP-->>Observer: "MainFrameState {url, documentId, frameDocuments}"
Observer->>RefMap: shouldResetRefs? → fork or new RefMap
Observer->>CDP: Accessibility.getFullAXTree (main frame)
Observer->>CDP: Page.getFrameTree() [child stability check per iframe]
CDP-->>Observer: documentId stable?
Observer->>RefMap: mint(node, documentId) → reuse or assign eN
Observer->>CDP: Page.getFrameTree() [after]
CDP-->>Observer: MainFrameState
Observer->>Observer: knownMainFrameChanged? retry or commit
Observer->>RefMap: commit fork as this.refs + store refScope
Observer-->>Agent: "SnapshotResult {text, refs, url}"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent
participant Observer
participant RefMap
participant CDP as CDP (Page/AX)
Agent->>Observer: snapshot()
Observer->>CDP: Page.getFrameTree() [before]
CDP-->>Observer: "MainFrameState {url, documentId, frameDocuments}"
Observer->>RefMap: shouldResetRefs? → fork or new RefMap
Observer->>CDP: Accessibility.getFullAXTree (main frame)
Observer->>CDP: Page.getFrameTree() [child stability check per iframe]
CDP-->>Observer: documentId stable?
Observer->>RefMap: mint(node, documentId) → reuse or assign eN
Observer->>CDP: Page.getFrameTree() [after]
CDP-->>Observer: MainFrameState
Observer->>Observer: knownMainFrameChanged? retry or commit
Observer->>RefMap: commit fork as this.refs + store refScope
Observer-->>Agent: "SnapshotResult {text, refs, url}"
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/browseros-agent/apps/server/src/browser/core/snapshot/refs.ts:47-54
**`reset()` is dead code in the production call path**
`Observer.refsForCapture()` always creates `new RefMap()` when a document reset is needed — `reset()` is never called from production code. Per the team's convention of removing dead code rather than maintaining it, this method should either be removed or used in place of `new RefMap()` in `refsForCapture()`.
### Issue 2 of 4
packages/browseros-agent/apps/server/src/browser/core/snapshot/refs.ts:31-44
Redundant `.clear()` calls in `forkForSnapshot` — `fork` is a freshly constructed `RefMap` whose `byStableNode` and `stableRefs` are already empty. The no-op clears before the copy loops add noise without benefit.
```suggestion
forkForSnapshot(): RefMap {
const fork = new RefMap()
fork.nextRefNum = this.nextRefNum
for (const [key, ref] of this.byStableNode) {
fork.byStableNode.set(key, ref)
}
for (const ref of this.stableRefs) {
fork.stableRefs.add(ref)
}
fork.beginSnapshot()
return fork
}
```
### Issue 3 of 4
packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts:204-219
**O(N) `Page.getFrameTree()` calls for child frames**
`stableDocumentIdForFrame` fires a fresh `Page.getFrameTree()` CDP round-trip for every non-root frame captured. With N iframes on a page, this adds N sequential CDP calls per snapshot. Since the "after" frame-tree read in `capture()` already fetches the full tree at the end, the fork/child stability check could reuse that result (or at least cache the call per `captureFrame` pass) instead of issuing a fresh call per frame.
### Issue 4 of 4
packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts:283-290
For non-root frames both `documents.set` calls set the same key (`node.frame.id`), since `isRoot ? undefined : node.frame.id` evaluates to `node.frame.id` when `isRoot` is false. The second line is a no-op for every child frame and should be removed to avoid misleading readers.
```suggestion
function visit(node: FrameTreeNode, isRoot: boolean): void {
const documentId = frameDocumentId(node.frame)
if (documentId !== undefined) {
if (isRoot) documents.set(undefined, documentId)
documents.set(node.frame.id, documentId)
}
for (const child of node.childFrames ?? []) visit(child, false)
}
```
Reviews (2): Last reviewed commit: "feat: stabilize snapshot refs by documen..." | Re-trigger Greptile |
| reset(): void { | ||
| this.byRef.clear() | ||
| this.byStableNode.clear() | ||
| this.stableRefs.clear() | ||
| this.nthCounter.clear() | ||
| this.nextRefNum = 1 | ||
| this.nextFallbackRefNum = 1 | ||
| } |
There was a problem hiding this comment.
reset() is dead code in the production call path
Observer.refsForCapture() always creates new RefMap() when a document reset is needed — reset() is never called from production code. Per the team's convention of removing dead code rather than maintaining it, this method should either be removed or used in place of new RefMap() in refsForCapture().
Rule Used: Remove unused/dead code rather than leaving it in ... (source)
Learned From
browseros-ai/BrowserOS-agent#126
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/core/snapshot/refs.ts
Line: 47-54
Comment:
**`reset()` is dead code in the production call path**
`Observer.refsForCapture()` always creates `new RefMap()` when a document reset is needed — `reset()` is never called from production code. Per the team's convention of removing dead code rather than maintaining it, this method should either be removed or used in place of `new RefMap()` in `refsForCapture()`.
**Rule Used:** Remove unused/dead code rather than leaving it in ... ([source](https://app.greptile.com/browseros-org-2/-/custom-context?memory=9b045db4-2630-428c-95b7-ccf048d34547))
**Learned From**
[browseros-ai/BrowserOS-agent#126](https://github.com/browseros-ai/BrowserOS-agent/pull/126)
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| private async stableDocumentIdForFrame( | ||
| rootSession: ProtocolApi, | ||
| frameId: FrameId | undefined, | ||
| frameDocuments: Map<FrameId | undefined, DocumentId>, | ||
| ): Promise<DocumentId | undefined> { | ||
| const beforeDocumentId = frameDocuments.get(frameId) | ||
| if (frameId === undefined || beforeDocumentId === undefined) { | ||
| return beforeDocumentId | ||
| } | ||
|
|
||
| const latest = await this.readFrameDocuments(rootSession).catch( | ||
| () => undefined, | ||
| ) | ||
| const afterDocumentId = latest?.get(frameId) | ||
| return afterDocumentId === beforeDocumentId ? beforeDocumentId : undefined | ||
| } |
There was a problem hiding this comment.
Page.getFrameTree() calls for child frames
stableDocumentIdForFrame fires a fresh Page.getFrameTree() CDP round-trip for every non-root frame captured. With N iframes on a page, this adds N sequential CDP calls per snapshot. Since the "after" frame-tree read in capture() already fetches the full tree at the end, the fork/child stability check could reuse that result (or at least cache the call per captureFrame pass) instead of issuing a fresh call per frame.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/core/observer/observer.ts
Line: 204-219
Comment:
**O(N) `Page.getFrameTree()` calls for child frames**
`stableDocumentIdForFrame` fires a fresh `Page.getFrameTree()` CDP round-trip for every non-root frame captured. With N iframes on a page, this adds N sequential CDP calls per snapshot. Since the "after" frame-tree read in `capture()` already fetches the full tree at the end, the fork/child stability check could reuse that result (or at least cache the call per `captureFrame` pass) instead of issuing a fresh call per frame.
How can I resolve this? If you propose a fix, please make it concise.
…ing of zero Rendering was NOT verified visually — the three intents are asserted in code, not on screen. Closes browseros-ai#1272
…work The bee on browseros-ai#1272 did the work and reported it in full: nine criteria, every one marked met. The review answered "3 of 9 criteria judged so far", said wait, and the task went on holding its files. The bee was not at fault and neither was the policy. `parseVerdictBlock` reads line by line, and the fourth criterion had wrapped: - `grep -c "why is it green at this number" Makefile` prints `1`: met The continuation line did not match the bullet pattern, and the loop treated that as the end of the block and BROKE - discarding the six criteria after it. So a correct, complete report was scored as a third of itself because one line was too long. Continuation lines are now joined before the block is parsed, and the original rule survives intact: a line that follows a COMPLETE bullet still ends the block, because the bee was told nothing follows it. Only a line continuing an unfinished bullet is glued on. Without that distinction a bee's closing paragraph would be parsed as criteria and the review would judge prose. Both halves are gated: the real browseros-ai#1272 block quoted verbatim from the transcript, and a check that prose after a finished criterion still stops the parse. Proven by disabling the join and watching the first go red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es (#303) The AttributedString rewrite of styledText landed (ee8f61e) and WARNING_CEILING came down to 0, but the comment above the constant still argued the opposite: WHY FOUR, ASKED HONESTLY, 'It is not lowered to zero today', citing RichTextRenderer.swift lines 73/75/77/79 - lines that in today's tree are the innocent body of the rewrite itself. The same present-tense claim ('is 4 now', 'the four that remain') sat in the same continuous comment run further up, and the e2e-testing skill still told agents 'the number stays'. - Makefile: the comment above WARNING_CEILING now states zero as the requirement and records, in the past tense, that the value was 4 for as long as RichTextRenderer.swift concatenated Text values with the deprecated +, and that the AttributedString rewrite removed them. The XCTEST_ERROR_CEILING lesson survives: asking 'why is it green at this number' is what found that one. The same history fix is applied to the 'count was 5 ... is 4 now' paragraph in the same comment run. The diff is comment lines only; WARNING_CEILING := 0 and the warnings recipe are untouched. - .claude/skills/e2e-testing/SKILL.md: the ceiling passage records the sequence - 4 was a reading, the AttributedString rewrite removed the four concatenations, the ceiling is now 0 and derived from the requirement. Closes browseros-ai#1272. Co-authored-by: queen-1272 bee <bee@localhost>
Summary
eNsnapshot refs for the same backend DOM nodes within a stable document.Design
Observernow owns a committed document/url ref scope and renders each capture into a transactionalRefMapfork.RefMapkeeps document-scoped backend-node assignments separate from latest-snapshot resolution metadata, while no-document or unstable child-frame captures fall back to traversal-order refs.Test plan
bun test apps/server/src/browser/core/snapshot/refs.test.ts apps/server/src/browser/core/snapshot/render.test.ts apps/server/src/browser/core/snapshot/diff.test.ts apps/server/src/browser/core/observer/resolve.test.ts apps/server/src/browser/core/observer/observer.test.tsbun run check