Add inspectable Browser selection mentions - #1643
Conversation
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and browser behavior. |
| return context.json({ ok: true, groups }); | ||
| }); | ||
|
|
||
| app.get("/plugins/mentions/inspect", async (context) => { |
There was a problem hiding this comment.
🚨 slopcop/review — This GET request can run full-trust plugin code from a cross-site request without an Origin header.
The browser guard accepts requests without Origin. A hostile page can use an image request or a navigation to start this work.
Use a JSON POST. Also reject requests when Sec-Fetch-Site has the cross-site value.
There was a problem hiding this comment.
Fixed in 5a6d429. Mention inspection is now a JSON POST, and the shared browser request guard rejects Sec-Fetch-Site: cross-site before plugin code can run. Route tests cover the method, originless same-site behavior, and cross-site rejection.
| } | ||
| return hasDirectText(element) ? 1 : 0; | ||
| }; | ||
| const collectMeaningfulElements = (): { |
There was a problem hiding this comment.
🚨 slopcop/review — Region capture now scans the complete page without a node, depth, or time limit.
The recursive walk can overflow on a deep DOM. The later layout pass also reads every meaningful element.
The new 1,000-target test took 9.56 seconds and failed its seven-second limit during this review.
Use an iterative walk with strict node and time limits. Cap candidates before layout and group work. Return a truncation value.
There was a problem hiding this comment.
Fixed in 5a6d429. Region collection now uses an iterative traversal with strict node, depth, candidate, and elapsed-time budgets. Candidates are capped before layout/group work, and V2 returns scanTruncated. Focused regressions cover 1,000 targets, candidate truncation, and a 600-level DOM.
| }) | ||
| .strict(); | ||
|
|
||
| const bbDesktopBrowserInspectionRegionContextSchema = z |
There was a problem hiding this comment.
🚨 slopcop/review — This replaces the version-one region wire shape without a new capability version.
Version one previously returned region.elements. It now requires commonAncestor, targets, and groups.
The desktop shell and served SPA can use different builds. Both builds still expose the same inspection method.
Add a version-two method or an explicit preload capability version. Test both version-skew directions.
There was a problem hiding this comment.
Fixed in 5a6d429. V1 and its region.elements wire shape remain unchanged. The deterministic result moved to optional experimental_inspectPageV2 with a distinct IPC channel and version: 2. The app feature-detects V2, and tests cover both old-SPA/new-shell and new-SPA/old-shell directions.
| typeof typed.icon === "string" && typed.icon.trim().length > 0 | ||
| ? typed.icon | ||
| : null, | ||
| preview: |
There was a problem hiding this comment.
🚨 slopcop/review — This accepts preview text without a size limit and always adds preview: null when no preview exists.
Large text can enter query caches and persisted prompt events. The new null field also breaks an existing server contract test.
Set a shared byte limit. Omit the optional field when the provider supplies no preview.
There was a problem hiding this comment.
Fixed in 5a6d429. Preview text uses shared field/byte limits, and the optional preview property is omitted when the provider supplies none rather than being serialized as null. Contract tests cover absence and overflow.
| preview?: unknown; | ||
| metadata?: unknown; | ||
| }; | ||
| if ( |
There was a problem hiding this comment.
🚨 slopcop/review — Inspection output has no size limit.
A plugin can return very large metadata, descriptions, alternative text, or image data. The server serializes all data before the app allocates it.
Set field and total byte limits before response serialization. Validate the image media type and base64 data within those limits.
There was a problem hiding this comment.
Fixed in 5a6d429. Inspection responses now enforce per-field and total serialized byte limits before the route responds. Preview images are restricted to allowed raster media types, valid base64, valid signatures, and bounded decoded size; focused tests cover each rejection.
| deviceScaleFactor: window.devicePixelRatio, | ||
| element: null, | ||
| region: { elements: regionElements(rect) }, | ||
| region: regionValue(rect), |
There was a problem hiding this comment.
🚨 slopcop/review — An error from regionValue leaves this inspection pending until the 60-second deadline.
The event callback does not catch errors. The controller Promise has only a resolve path, so an event error cannot reject it.
Add a reject path with full cleanup. Test a locator failure through deep shadow roots or an overlong selector.
There was a problem hiding this comment.
Fixed in 5a6d429. The controller promise now has a reject path, event callbacks catch capture failures, and every rejection performs full listener/overlay cleanup immediately. A focused locator failure regression proves it no longer waits for the deadline.
| const absoluteLocator = locatorFrom(element, document); | ||
| const relativeLocator = locatorFrom(element, commonAncestor); | ||
| if (absoluteLocator === null || relativeLocator === null) return []; | ||
| const clone = sanitizedClone(element); |
There was a problem hiding this comment.
🚨 slopcop/review — This call deep-clones a complete selected subtree before the 200-node limit applies.
A large container can cause a large memory copy for each selected target. The later prune does not limit that initial work.
Build a shallow sanitized copy under the node limit. Do not call cloneNode(true) before the limit.
There was a problem hiding this comment.
Fixed in 5a6d429. Region target serialization no longer calls cloneNode(true); it builds a shallow sanitized copy through the same bounded traversal used by element capture, so the 200-node limit applies before copying.
| onActivate, | ||
| }: PromptMentionPillProps) { | ||
| const title = promptMentionTooltipLabel(resource); | ||
| const preview = resource.kind === "plugin" ? resource.preview : undefined; |
There was a problem hiding this comment.
🚨 slopcop/review — Sent inspectable plugin mentions lose their inspector action.
The composer reads experimentalInspectability and opens the inspector. This timeline path reads only the preview.
Add the same inspector action to sent mention pills. Add a test that activates a sent inspectable mention.
There was a problem hiding this comment.
Fixed in 5a6d429. Sent timeline pills now consume experimentalInspectability and reopen the same inspector action as composer pills. Tests activate a sent mention, and exact Electron QA verified both pointer and keyboard Enter activation after send.
| id: provider.id, | ||
| label: provider.label, | ||
| triggers, | ||
| experimentalInspectability: provider.experimentalInspectability === true, |
There was a problem hiding this comment.
🚨 slopcop/review — This adds experimentalInspectability: false to providers that omit the optional field.
The full app suite fails its existing contribution contract test because of this field. Production code does not read the provider-level value.
Remove the unused field, or omit it when false and add a real consumer.
There was a problem hiding this comment.
Fixed in 5a6d429. Removed the unused provider-level experimentalInspectability value. Providers that omit the optional field no longer gain a false property, and the existing contribution contract shape is preserved.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This PR lets a Browser plugin turn selected page content into an inspectable mention.
It adds preview tooltips and an inspector dialog. It also adds detailed region locators, groups, accessibility data, and React hints.
I found these blocking problems:
- The desktop region result changes its version-one wire shape. An old shell and a new SPA cannot exchange this result safely.
- Region capture scans the complete page without a work limit. The new 1,000-target test took 9.56 seconds and failed.
- A region capture error can leave the request open until the 60-second deadline.
- The inspection route uses a GET request that can run plugin code from an originless cross-site request.
- Preview and inspection output have no byte limits. These values can cause large responses, caches, events, and renderer allocations.
- The region path deep-clones full subtrees before it applies the 200-node limit.
- A sent inspectable mention loses the inspector action after it leaves the composer.
- Two optional response fields now appear as
falseornull. Existing app and server contract tests fail.
Architecture notes:
- Add an explicit inspection capability version. Keep each desktop wire shape stable.
- Put the region walk, layout reads, and clone work behind shared node and time limits.
- Define shared preview and inspection schemas with field and total byte limits.
- Remove the unused provider-level inspectability value, or make it the single source.
- Reuse the richer React hint walker for the old stack-only result.
Validation:
- Type checks passed for the app, desktop, server, desktop contract, domain, and plugin SDK.
- The desktop suite passed 241 of 242 tests. Its new performance test failed.
- The app suite passed 2,782 of 2,784 tests. One PR-specific contract test failed.
- The server suite passed 1,668 of 1,678 tests. One PR-specific contract test failed. Nine tests timed out under concurrent load.
- The desktop contract, domain, and plugin SDK tests passed.
- The local web app rendered through the development browser. The external Browser Context plugin was absent, so I could not run the full capture workflow.
I left line comments with concrete fixes. I would not merge this revision until the compatibility, security, performance, and test failures are fixed.
# Conflicts: # apps/desktop/src/desktop-browser-inspection.ts # apps/desktop/test/desktop-browser-inspection-page.test.ts # packages/templates/src/generated/plugin-sdk-dts.generated.ts
…nspectable-browser-context-thr_p9qmfkggwm
|
Addressed every actionable review finding and pushed 5a6d429 (stack head e03ae79). Verification:
Compatibility is additive: V1 remains stable and optional V2 is feature-detected. The richer legacy React walker suggestion is informational and unrelated to the reviewed deterministic V2 path, so I did not expand this fix into that refactor. |
…nspectable-browser-context-thr_p9qmfkggwm
|
CI follow-up: the wall-clock region tests could exhaust the production 100 ms deadline before scanning under shared-runner load. Production limits remain unchanged. Large-corpus tests now use a deterministic clock, and a separate execution test advances the clock past 100 ms to prove scanTruncated. Full desktop: 36 files / 253 tests; SDK 87, templates 44, domain 143; npm version guard passes. |
|
Final CI follow-up: sent and composer inspectors now load only after an inspectable pill is activated. That removes the inspector implementation from eager execution. The persistent pill/activation shell still adds a measured 1.6 KB raw / 2.0 KB Brotli over the prior ratchet, so the boot budget moves deliberately by 4 KB raw / 3 KB Brotli (to 1,671 KB / 441 KB), leaving about 2.4 KB raw / 1 KB Brotli headroom. The bundle guard passes, and no forbidden boot package was introduced. |
Summary
Verification
BB-Thread-ID: thr_p9qmfkggwm