[STG-1250] use same cdp tree function across api and sdk#1769
[STG-1250] use same cdp tree function across api and sdk#1769sameelarif wants to merge 4 commits intomainfrom
Conversation
|
Greptile SummaryThis PR unifies CDP tree function usage and adds a Key changes:
Critical issue:
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: 12254ed |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Strip [encodedId] prefixes that were kept during formatting solely so | ||
| // injectSubtrees could locate iframe injection points. One regex pass over | ||
| // the final combined string is far cheaper than post-processing in the caller. | ||
| const stripIds = (tree: string) => tree.replace(/\[[^\]]+\] /g, ""); |
There was a problem hiding this comment.
the regex /\[[^\]]+\] /g will incorrectly strip [...] patterns from node names, not just ID prefixes. if a node has name "[TODO] Fix this", the formatted line [1-123] button "[TODO] Fix this" becomes button "Fix this" instead of button "[TODO] Fix this". use line-anchored pattern instead:
| const stripIds = (tree: string) => tree.replace(/\[[^\]]+\] /g, ""); | |
| const stripIds = (tree: string) => tree.replace(/^(\s*)\[[^\]]+\] /gm, "$1"); |
| import { test, expect } from "@playwright/test"; | ||
| import { V3 } from "../../lib/v3/v3.js"; | ||
| import { captureHybridSnapshot } from "../../lib/v3/understudy/a11y/snapshot/index.js"; | ||
| import { v3TestConfig } from "./v3.config.js"; | ||
|
|
||
| /** | ||
| * Checks whether captureHybridSnapshot produces identical combinedTree strings | ||
| * across two entirely separate browser sessions navigated to the same static page. | ||
| * | ||
| * This mirrors real-world cache usage: two independent requests hit the same URL, | ||
| * each with their own browser session. A determinism failure here means the cache | ||
| * hash would differ between those sessions, causing a cache miss even though the | ||
| * page content is identical. | ||
| */ | ||
| test.describe("captureHybridSnapshot determinism", () => { | ||
| let v3a: V3; | ||
| let v3b: V3; | ||
|
|
||
| test.beforeEach(async () => { | ||
| v3a = new V3(v3TestConfig); | ||
| v3b = new V3(v3TestConfig); | ||
| await Promise.all([v3a.init(), v3b.init()]); | ||
| }); | ||
|
|
||
| test.afterEach(async () => { | ||
| await Promise.all([ | ||
| v3a?.close?.().catch(() => {}), | ||
| v3b?.close?.().catch(() => {}), | ||
| ]); | ||
| }); | ||
|
|
||
| test("two separate sessions on the same static page produce identical trees", async () => { | ||
| const pageA = v3a.context.pages()[0]; | ||
| const pageB = v3b.context.pages()[0]; | ||
| await Promise.all([ | ||
| pageA.goto("https://example.com"), | ||
| pageB.goto("https://example.com"), | ||
| ]); | ||
|
|
||
| const [snap1, snap2] = await Promise.all([ | ||
| captureHybridSnapshot(pageA), | ||
| captureHybridSnapshot(pageB), | ||
| ]); | ||
|
|
||
| if (snap1.combinedTree !== snap2.combinedTree) { | ||
| const lines1 = snap1.combinedTree.split("\n"); | ||
| const lines2 = snap2.combinedTree.split("\n"); | ||
| const maxLines = Math.max(lines1.length, lines2.length); | ||
| const diffLines: string[] = []; | ||
| for (let i = 0; i < maxLines; i++) { | ||
| const l1 = lines1[i] ?? "<missing>"; | ||
| const l2 = lines2[i] ?? "<missing>"; | ||
| if (l1 !== l2) { | ||
| diffLines.push(`line ${i + 1}:`); | ||
| diffLines.push(` sessionA: ${l1}`); | ||
| diffLines.push(` sessionB: ${l2}`); | ||
| } | ||
| } | ||
| console.log("=== DIFF ===\n" + diffLines.join("\n")); | ||
| } | ||
|
|
||
| expect(snap1.combinedTree).toBe(snap2.combinedTree); | ||
| }); | ||
|
|
||
| test("two separate sessions with pierceShadow produce identical trees", async () => { | ||
| const pageA = v3a.context.pages()[0]; | ||
| const pageB = v3b.context.pages()[0]; | ||
| await Promise.all([ | ||
| pageA.goto("https://example.com"), | ||
| pageB.goto("https://example.com"), | ||
| ]); | ||
|
|
||
| const [snap1, snap2] = await Promise.all([ | ||
| captureHybridSnapshot(pageA, { pierceShadow: true }), | ||
| captureHybridSnapshot(pageB, { pierceShadow: true }), | ||
| ]); | ||
|
|
||
| expect(snap1.combinedTree).toBe(snap2.combinedTree); | ||
| }); | ||
|
|
||
| test("two separate sessions without iframes produce identical trees", async () => { | ||
| const pageA = v3a.context.pages()[0]; | ||
| const pageB = v3b.context.pages()[0]; | ||
| await Promise.all([ | ||
| pageA.goto("https://example.com"), | ||
| pageB.goto("https://example.com"), | ||
| ]); | ||
|
|
||
| const [snap1, snap2] = await Promise.all([ | ||
| captureHybridSnapshot(pageA, { includeIframes: false }), | ||
| captureHybridSnapshot(pageB, { includeIframes: false }), | ||
| ]); | ||
|
|
||
| expect(snap1.combinedTree).toBe(snap2.combinedTree); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
tests verify determinism of the full HybridSnapshot.combinedTree, but the new treeOnly: true option (which is the main addition) is never tested. add a test that uses captureHybridSnapshot(page, { treeOnly: true }) and verifies the returned string is deterministic and has IDs stripped
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Confidence score: 3/5
- There is a concrete behavior bug in
packages/core/lib/v3/understudy/a11y/snapshot/capture.tswherehashModeisn’t passed in the scoped snapshot path, sotreeOnly+focusSelectorcan yield inconsistent formatting. - Public API surface is being expanded via exports from
packages/core/lib/v3/index.ts, which pulls in internal types fromtypes/private/snapshot.tsand may lock in implementation details. - Given the above, there’s some regression risk if downstream users rely on the scoped snapshot or if future refactors need to change internal types.
- Pay close attention to
packages/core/lib/v3/understudy/a11y/snapshot/capture.tsandpackages/core/lib/v3/index.ts- scoped snapshot formatting and public API exposure of internal types.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/index.ts">
<violation number="1" location="packages/core/lib/v3/index.ts:68">
P2: According to linked Linear issue STG-1250, `captureHybridSnapshot` is being shared between the API and SDK internally. The codebase convention for internal cross-package exports is to prefix with `__internal` (e.g., `__internalCreateInMemoryAgentCacheHandle`). Consider renaming:
```ts
export { captureHybridSnapshot as __internalCaptureHybridSnapshot } from "./understudy/a11y/snapshot/index.js";
This signals to consumers that this is not a stable public API.
P2: These types are sourced from `types/private/snapshot.ts`, yet they're being exported from the package's public entry point. This turns internal implementation types into a public API contract, reducing refactoring flexibility. If these types need to be shared across the API and SDK, consider either moving them to a public types module or creating a shared internal package — rather than promoting private types to the public surface.(Based on your team's feedback about avoiding exposing internal types as public APIs.) [FEEDBACK_USED]
(Based on your team's feedback about stabilizing APIs before exposing parameters publicly and avoiding exposing internal types as public APIs.) [FEEDBACK_USED]
tryScopedSnapshot needs to propagate hashMode (or treeOnly) into its a11yForFrame call to match the behavior added in collectPerFrameMaps.
</details>
<details>
<summary>Architecture diagram</summary>
```mermaid
sequenceDiagram
participant Caller as SDK / API Route
participant Snap as Snapshot Manager (captureHybridSnapshot)
participant Frame as Frame Processor (a11yForFrame)
participant Fmt as Tree Formatter (treeFormatUtils)
participant CDP as Browser (CDP)
Note over Caller,CDP: NEW: Unified Deterministic Snapshot Flow (STG-1250)
Caller->>Snap: captureHybridSnapshot(page, { treeOnly: true })
Snap->>Snap: Build Frame Context (iframes/shadow)
loop For each Frame in Scope
Snap->>Frame: a11yForFrame(frame, { hashMode: true })
Frame->>CDP: Fetch Accessibility Tree nodes
CDP-->>Frame: A11y Node Tree
Frame->>Frame: decorateRoles() & buildHierarchicalTree()
loop For each Node in Tree
alt CHANGED: hashMode is active
Frame->>Fmt: NEW: formatTreeLineForHash(node)
Fmt->>Fmt: Normalize structural roles (div/span -> "none")
Fmt->>Fmt: Quote names and keep [encodedId]
else Standard mode
Frame->>Fmt: formatTreeLine(node)
end
Fmt-->>Frame: Formatted string line
end
Frame-->>Snap: Frame outline (string)
end
Snap->>Snap: mergeFramesIntoSnapshot()
Note right of Snap: Injects iframe subtrees into parent outlines<br/>using [encodedId] markers
alt NEW: treeOnly is requested
Snap->>Snap: NEW: stripIds(combinedTree)
Note right of Snap: Final Regex pass removes all [encodedId]<br/>prefixes for deterministic cache key
Snap-->>Caller: Return deterministic string (Outline only)
else Standard mode
Snap-->>Caller: Return HybridSnapshot object
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
| if (scopedSnapshot) return scopedSnapshot; | ||
| if (scopedSnapshot) { | ||
| if (options?.treeOnly) return stripIds(scopedSnapshot.combinedTree); |
There was a problem hiding this comment.
P1: Bug: hashMode is not passed to a11yForFrame in the scoped snapshot path (tryScopedSnapshot), so when treeOnly: true and a focusSelector successfully scopes the snapshot, the tree will use standard formatting (formatTreeLine) instead of hash-friendly formatting (formatTreeLineForHash). This makes the output inconsistent depending on which code path is taken.
tryScopedSnapshot needs to propagate hashMode (or treeOnly) into its a11yForFrame call to match the behavior added in collectPerFrameMaps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/a11y/snapshot/capture.ts, line 74:
<comment>Bug: `hashMode` is not passed to `a11yForFrame` in the scoped snapshot path (`tryScopedSnapshot`), so when `treeOnly: true` and a `focusSelector` successfully scopes the snapshot, the tree will use standard formatting (`formatTreeLine`) instead of hash-friendly formatting (`formatTreeLineForHash`). This makes the output inconsistent depending on which code path is taken.
`tryScopedSnapshot` needs to propagate `hashMode` (or `treeOnly`) into its `a11yForFrame` call to match the behavior added in `collectPerFrameMaps`.</comment>
<file context>
@@ -57,7 +70,10 @@ export async function captureHybridSnapshot(
);
- if (scopedSnapshot) return scopedSnapshot;
+ if (scopedSnapshot) {
+ if (options?.treeOnly) return stripIds(scopedSnapshot.combinedTree);
+ return scopedSnapshot;
+ }
</file context>
| export { maybeRunShutdownSupervisorFromArgv as __internalMaybeRunShutdownSupervisorFromArgv } from "./shutdown/supervisor.js"; | ||
| export type { ServerAgentCacheHandle } from "./cache/serverAgentCache.js"; | ||
|
|
||
| export { captureHybridSnapshot } from "./understudy/a11y/snapshot/index.js"; |
There was a problem hiding this comment.
P2: According to linked Linear issue STG-1250, captureHybridSnapshot is being shared between the API and SDK internally. The codebase convention for internal cross-package exports is to prefix with __internal (e.g., __internalCreateInMemoryAgentCacheHandle). Consider renaming:
export { captureHybridSnapshot as __internalCaptureHybridSnapshot } from "./understudy/a11y/snapshot/index.js";This signals to consumers that this is not a stable public API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/index.ts, line 68:
<comment>According to linked Linear issue STG-1250, `captureHybridSnapshot` is being shared between the API and SDK internally. The codebase convention for internal cross-package exports is to prefix with `__internal` (e.g., `__internalCreateInMemoryAgentCacheHandle`). Consider renaming:
```ts
export { captureHybridSnapshot as __internalCaptureHybridSnapshot } from "./understudy/a11y/snapshot/index.js";
This signals to consumers that this is not a stable public API.
@@ -65,6 +65,9 @@ export { __internalCreateInMemoryAgentCacheHandle } from "./cache/serverAgentCac export { maybeRunShutdownSupervisorFromArgv as __internalMaybeRunShutdownSupervisorFromArgv } from "./shutdown/supervisor.js"; export type { ServerAgentCacheHandle } from "./cache/serverAgentCache.js";+export { captureHybridSnapshot } from "./understudy/a11y/snapshot/index.js";
+export type { SnapshotOptions, HybridSnapshot, PerFrameSnapshot } from "./types/private/snapshot.js";
+
</file context>
</details>
```suggestion
export { captureHybridSnapshot as __internalCaptureHybridSnapshot } from "./understudy/a11y/snapshot/index.js";
| * HybridSnapshot object. Intended for callers that only need the text | ||
| * outline (e.g. DOM hashing, cache keys). | ||
| */ | ||
| treeOnly?: boolean; |
There was a problem hiding this comment.
P2: SnapshotOptions is re-exported publicly from packages/core/lib/v3/index.ts. Adding treeOnly here locks it into the public API contract. Since this option is described as serving internal callers ("DOM hashing, cache keys"), consider whether it belongs on the public type or should live on a separate internal-only options type to avoid committing to this shape before there's real user signal.
(Based on your team's feedback about stabilizing APIs before exposing parameters publicly and avoiding exposing internal types as public APIs.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/types/private/snapshot.ts, line 28:
<comment>`SnapshotOptions` is re-exported publicly from `packages/core/lib/v3/index.ts`. Adding `treeOnly` here locks it into the public API contract. Since this option is described as serving internal callers ("DOM hashing, cache keys"), consider whether it belongs on the public type or should live on a separate internal-only options type to avoid committing to this shape before there's real user signal.
(Based on your team's feedback about stabilizing APIs before exposing parameters publicly and avoiding exposing internal types as public APIs.) </comment>
<file context>
@@ -20,6 +20,12 @@ export type SnapshotOptions = {
+ * HybridSnapshot object. Intended for callers that only need the text
+ * outline (e.g. DOM hashing, cache keys).
+ */
+ treeOnly?: boolean;
};
</file context>
packages/core/lib/v3/index.ts
Outdated
| export type { ServerAgentCacheHandle } from "./cache/serverAgentCache.js"; | ||
|
|
||
| export { captureHybridSnapshot } from "./understudy/a11y/snapshot/index.js"; | ||
| export type { SnapshotOptions, HybridSnapshot, PerFrameSnapshot } from "./types/private/snapshot.js"; |
There was a problem hiding this comment.
P2: These types are sourced from types/private/snapshot.ts, yet they're being exported from the package's public entry point. This turns internal implementation types into a public API contract, reducing refactoring flexibility. If these types need to be shared across the API and SDK, consider either moving them to a public types module or creating a shared internal package — rather than promoting private types to the public surface.
(Based on your team's feedback about avoiding exposing internal types as public APIs.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/index.ts, line 69:
<comment>These types are sourced from `types/private/snapshot.ts`, yet they're being exported from the package's public entry point. This turns internal implementation types into a public API contract, reducing refactoring flexibility. If these types need to be shared across the API and SDK, consider either moving them to a public types module or creating a shared internal package — rather than promoting private types to the public surface.
(Based on your team's feedback about avoiding exposing internal types as public APIs.) </comment>
<file context>
@@ -65,6 +65,9 @@ export { __internalCreateInMemoryAgentCacheHandle } from "./cache/serverAgentCac
export type { ServerAgentCacheHandle } from "./cache/serverAgentCache.js";
+export { captureHybridSnapshot } from "./understudy/a11y/snapshot/index.js";
+export type { SnapshotOptions, HybridSnapshot, PerFrameSnapshot } from "./types/private/snapshot.js";
+
export type {
</file context>
1 participant
why
what changed
test plan
Summary by cubic
Unified the CDP accessibility snapshot across the API and SDK and added a string-only, hash-friendly output for stable caching. Supports Linear STG-1250 by making snapshot trees deterministic and ready to use as cache keys.
New Features
Tests
Written for commit cd0ba68. Summary will update on new commits. Review in cubic