[chore]: split up snapshot logic into submodules #1429
Conversation
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts:150">
P2: `absPrefix` is declared as `const` and never updated, unlike the XPath version where it's built up during iframe traversal. This will always return an empty string, which may cause incorrect XPath prefixes when resolving CSS selectors across iframes.</violation>
</file>
<file name="packages/core/lib/v3/understudy/a11y/snapshot/domTree.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/domTree.ts:341">
P2: `findNodeByBackendId` is missing traversal of `contentDocument`, making it inconsistent with other traversal functions like `collectDomTraversalTargets`. This could cause the function to miss nodes inside iframe content documents.</violation>
</file>
<file name="packages/core/lib/v3/understudy/a11y/snapshot/activeElement.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/activeElement.ts:125">
P1: Arguments to `prefixXPath` are in wrong order for bottom-up traversal. When walking from focused frame to root, each ancestor iframe path should prefix (not suffix) the accumulated path. This will produce incorrect XPaths for elements in nested iframes.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| .map((s) => s.trim()) | ||
| .filter(Boolean); | ||
| let ctxFrameId = rootId; | ||
| const absPrefix = ""; |
There was a problem hiding this comment.
P2: absPrefix is declared as const and never updated, unlike the XPath version where it's built up during iframe traversal. This will always return an empty string, which may cause incorrect XPath prefixes when resolving CSS selectors across iframes.
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/focusSelectors.ts, line 150:
<comment>`absPrefix` is declared as `const` and never updated, unlike the XPath version where it's built up during iframe traversal. This will always return an empty string, which may cause incorrect XPath prefixes when resolving CSS selectors across iframes.</comment>
<file context>
@@ -0,0 +1,292 @@
+ .map((s) => s.trim())
+ .filter(Boolean);
+ let ctxFrameId = rootId;
+ const absPrefix = "";
+
+ for (let i = 0; i < Math.max(0, parts.length - 1); i++) {
</file context>
| const n = stack.pop()!; | ||
| if (n.backendNodeId === backendNodeId) return n; | ||
| if (n.children) for (const c of n.children) stack.push(c); | ||
| if (n.shadowRoots) for (const s of n.shadowRoots) stack.push(s); |
There was a problem hiding this comment.
P2: findNodeByBackendId is missing traversal of contentDocument, making it inconsistent with other traversal functions like collectDomTraversalTargets. This could cause the function to miss nodes inside iframe content documents.
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/domTree.ts, line 341:
<comment>`findNodeByBackendId` is missing traversal of `contentDocument`, making it inconsistent with other traversal functions like `collectDomTraversalTargets`. This could cause the function to miss nodes inside iframe content documents.</comment>
<file context>
@@ -0,0 +1,344 @@
+ const n = stack.pop()!;
+ if (n.backendNodeId === backendNodeId) return n;
+ if (n.children) for (const c of n.children) stack.push(c);
+ if (n.shadowRoots) for (const s of n.shadowRoots) stack.push(s);
+ }
+ return undefined;
</file context>
| }>("DOM.getFrameOwner", { frameId: cur }); | ||
| if (typeof backendNodeId === "number") { | ||
| const xp = await absoluteXPathForBackendNode(parentSess, backendNodeId); | ||
| if (xp) prefix = prefix ? prefixXPath(prefix, xp) : normalizeXPath(xp); |
There was a problem hiding this comment.
P1: Arguments to prefixXPath are in wrong order for bottom-up traversal. When walking from focused frame to root, each ancestor iframe path should prefix (not suffix) the accumulated path. This will produce incorrect XPaths for elements in nested iframes.
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/activeElement.ts, line 125:
<comment>Arguments to `prefixXPath` are in wrong order for bottom-up traversal. When walking from focused frame to root, each ancestor iframe path should prefix (not suffix) the accumulated path. This will produce incorrect XPaths for elements in nested iframes.</comment>
<file context>
@@ -0,0 +1,134 @@
+ }>("DOM.getFrameOwner", { frameId: cur });
+ if (typeof backendNodeId === "number") {
+ const xp = await absoluteXPathForBackendNode(parentSess, backendNodeId);
+ if (xp) prefix = prefix ? prefixXPath(prefix, xp) : normalizeXPath(xp);
+ }
+ } catch {
</file context>
why
snapshot.tsgrew to a large file that was responsible for DOM fetching, Accessibility tree processing, XPath utilities, hit-testing, and snapshot orchestration. there were also a lot of inlined typeswhat changed
this PR breaks the file apart into sub modules, and puts types into a single file (
types/private/snapshot.ts). The submodules are:capture.ts: orchestrates snapshot building flow, and is now written with composable helpers (buildFrameContext(),tryScopedSnapshot(),buildSessionIndexes(),collectPerFrameMaps(),computeFramePrefixes(),mergeFramesIntoSnapshot()) with detailed JSDoc and inline commentscoordinateResolver.ts: holds theresolveXpathForLocation()function which performs coordinate -> XPath hit-testing across nested frames.activeElement.ts: holds thecomputeActiveElementXpath()function, which resolves the currently focused element’s absolute XPath.domTree.ts: handles DOM.getDocument hydration, CBOR-aware retries, and session-wide DOM indexing.a11yTree.ts: gets the accessibility tree via CDP, applies selector scoping, decorates roles, and formats the actual stringified treefocusSelectors.ts: parses cross-frame XPath/CSS selectors and resolves iframe hops.xpathUtils.ts: shared XPath helpers (absolute path resolution, child segment building, normalization).sessions.ts: encapsulates owner/parent session lookups for frames.treeFormatUtils.ts: pure formatting utilities (outline rendering, diffing, subtree injection, whitespace normalization).test plan
Summary by cubic
Split the monolithic a11y snapshot implementation into focused submodules and centralized types to improve readability and unit testability. No behavior changes; addresses Linear STG-1085.
Written for commit 81c3d13. Summary will update automatically on new commits.