feat(peek): richer live page-inspection in get_element_detail#120
Conversation
…description + aria inheritance (H2.3) Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
… bg-image url) (H2.3) Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…11y) + point at session tools for console/network (H2.3) Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
Warning Review limit reached
More reviews will be available in 58 minutes and 24 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough
ChangesElementDetail enrichment and masking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/peek-mcp/test/server.test.ts (1)
770-776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the JSON
detailspayload instead of substring-matching the serialized body.These
toContain(...)checks can still pass if the same text shows up elsewhere in the response. Parsing once and assertingdetails.description/details.computedStyles.displaywould lock the MCP contract this test is meant to cover.Suggested tightening
- expect(textOf(res as never)).toContain('Submits the login form'); - expect(textOf(res as never)).toContain('display'); + const body = JSON.parse(textOf(res as never)) as { + details?: { + description?: string; + computedStyles?: { display?: string }; + }; + }; + expect(body.details?.description).toBe('Submits the login form'); + expect(body.details?.computedStyles?.display).toBe('inline-block');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/peek-mcp/test/server.test.ts` around lines 770 - 776, The server test is too loose because it substring-matches the serialized response instead of validating the MCP `details` payload directly. Update the assertions in the test around `textOf(res as never)` to parse the JSON once and verify the `details` object fields explicitly, such as `details.description` and `details.computedStyles.display`, so the contract is checked through the actual structured response rather than incidental text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/peek-extension/src/__tests__/snapshot.test.ts`:
- Around line 683-707: Replace the sensitive-looking fixture text in the
snapshot test with synthetic non-PII content while keeping the same masking
behavior coverage. Update the DOM setup in the `redacts the accessible
description inside a masked region` test so `buildPageView` and
`buildElementDetail` still verify redaction for masked descriptions, but avoid
real card-number/email literals in the `data-private`/`aria-describedby` nodes.
In `@packages/peek-extension/src/permissions/mask-view.ts`:
- Around line 126-129: The URL-masking regex in mask-view.ts is too restrictive
for quoted CSS urls, so it misses values containing ) and can leave secrets
unmasked. Update the replacement in the style masking path to correctly parse
quoted url(...) values with embedded ) while preserving the current
masked-by-default behavior, and keep the maskUrl(u) handling intact for all
matched URLs. Also make sure the fix still treats this as a potential PII
leakage path and masks sensitive query data consistently.
In `@packages/peek-extension/src/permissions/snapshot.ts`:
- Around line 897-902: The computed style snapshot in `snapshot.ts` is leaking
masked element `backgroundImage` values across the in-page boundary before
SW-side redaction runs. Update the logic that builds `computedStyles` (where
`getComputedStyle`, `STYLE_KEYS`, and the `.rr-mask` / `[data-private]` handling
are used) to explicitly strip or neutralize `backgroundImage` for masked
elements before returning the snapshot, ensuring no URL text leaves the page.
---
Nitpick comments:
In `@packages/peek-mcp/test/server.test.ts`:
- Around line 770-776: The server test is too loose because it substring-matches
the serialized response instead of validating the MCP `details` payload
directly. Update the assertions in the test around `textOf(res as never)` to
parse the JSON once and verify the `details` object fields explicitly, such as
`details.description` and `details.computedStyles.display`, so the contract is
checked through the actual structured response rather than incidental text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1779f707-07cb-4451-8438-d78098f63b67
📒 Files selected for processing (7)
.changeset/peek-live-inspection-reads.mdpackages/peek-extension/src/__tests__/action-handler.test.tspackages/peek-extension/src/__tests__/snapshot.test.tspackages/peek-extension/src/permissions/mask-view.tspackages/peek-extension/src/permissions/snapshot.tspackages/peek-mcp/src/mcp/server.tspackages/peek-mcp/test/server.test.ts
…xture, structured server-test assertions (H2.3) Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
Addressed all three review findings in 543573c:
Extension 138/138 + MCP server 34/34 green; typecheck + biome clean. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Changed files in commit 543573c =="
git show --stat --oneline --no-renames 543573c
echo
echo "== Outline: packages/peek-extension/src/permissions/mask-view.ts =="
ast-grep outline packages/peek-extension/src/permissions/mask-view.ts --view expanded || true
echo
echo "== Relevant hunk: packages/peek-extension/src/permissions/mask-view.ts =="
git show --unified=40 543573c -- packages/peek-extension/src/permissions/mask-view.ts
echo
echo "== Relevant hunk: packages/peek-extension/src/__tests__/snapshot.test.ts =="
git show --unified=40 543573c -- packages/peek-extension/src/__tests__/snapshot.test.ts
echo
echo "== Relevant hunk: packages/peek-mcp/test/server.test.ts =="
git show --unified=40 543573c -- packages/peek-mcp/test/server.test.tsLength of output: 13554
No new concerns from this follow-up pass. Nice cleanup. 🐇✨ If you found this review helpful, would you consider giving us a shout-out on X%60%20safely%2C%20plus%20tests%20use%20synthetic%20fixtures%20%2B%20structured%20JSON%20asserts.%20Faster%2C%20safer%20reviews%20%F0%9F%9A%80%F0%9F%9B%A1%EF%B8%8F&url=https%3A%2F%2Fgithub.com%2FCubenest%2Frrweb-stack%2Fpull%2F120)? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
…not leave the page (H2.3 review) Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
Fixed the third finding (masked-element backgroundImage in-page redaction) in 4dd6f5d, with a regression test. All three review findings now addressed; CI green, extension + mcp suites green, typecheck + biome clean. @coderabbitai full review |
|
✅ Action performedFull review finished. |
1 participant
Summary
Enriches the existing Level-1 read tool
get_element_detailwith three additions, all masked and DOM-only (no CDP, no eval, no new permission/tool/schema):computedStyles— a curated 19-key allowlist read fromgetComputedStyle(el)(layout/box + typography/paint), not a full ~350-property dump.description— the accessible description, resolved fromaria-describedby(id-list → referenced text) oraria-description, clipped to 500 chars.effectiveAriaHidden/effectiveAriaDisabled— ancestor-walk inheritance flags ("why can't the agent act on this?").get_page_view(the bulk read) is unchanged — these land on the single-element drill-in only, keeping the compact view cheap. The tool description now also points agents atget_session_console_errors/get_session_network_errorsfor live console/network state.Masking (defense in depth)
Every new data-bearing field is masked at both layers:
.rr-mask/[data-private]element returns•••for itsdescription, and anaria-describedbyreference pointing into a masked region is redacted per-reference — mirroring howname/text/context.headingalready behave.descriptionruns through the PII masker; everyurl()inbackground-image(incl. comma-separated multi-backgrounds) has its query string redacted viamaskUrl. All other style values are non-sensitive layout/paint and pass through untouched.The MAIN-world
buildElementDetailstays fully self-contained (.toString()-serializable) — all new logic is inline, no module-scope helpers.Test Plan
@peekdev/extensionsuite green (incl. snapshot + masking tests; the in-pageis self-containedserialization guard still passes)description, multi-url()background masking, plain-style pass-through@peekdev/mcpserver.test.tsgreen (enrichedget_element_detaildispatch surfaces the new fields)@peekdev/extension+@peekdev/mcpminor)Summary by CodeRabbit
aria-hidden/aria-disabled, and a curated allowlist of computed style values.url(...)values while keeping non-sensitive styles available.background-imagestrings without leaking per-layer secrets.