Add plugin-driven Browser inspection mode - #1642
Conversation
This reverts commit b06f167.
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am now reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
| * Render one compact component in the Browser tab chrome. Experimental: see | ||
| * docs/api_to_audit.md. | ||
| */ | ||
| experimental_browserAction( |
There was a problem hiding this comment.
🚨 slopcop/review — [P1] Add this slot to the authoring-doc contract
This method extends PluginAppSlots. However, SlotPropsByName in plugin-authoring-docs.test.ts lacks experimental_browserAction.
The required @bb/server type check now fails at line 169. Add experimental_browserAction: PluginBrowserActionProps and its field documentation.
There was a problem hiding this comment.
Fixed in ba61d30. Added experimental_browserAction: PluginBrowserActionProps to the authoring-doc slot map and documented every field. The required server authoring contract and typecheck now pass.
| tabId: z.string().min(1).max(256), | ||
| requestId: z.string().min(1).max(256), | ||
| kind: z.enum(["element", "region", "auto"]), | ||
| identity: bbDesktopBrowserInspectionIdentitySchema, |
There was a problem hiding this comment.
🚨 slopcop/review — [P2] Do not accept an ignored identity
The renderer sends the thread and project identity. However, the manager selects a view only with the window and tab identifier.
No desktop code reads request.identity. This field adds a false identity boundary and breaks the repository contract rule.
Remove the field, or bind it to trusted view ownership and reject a mismatch.
There was a problem hiding this comment.
Fixed in ba61d30. Removed the ignored identity field from the renderer request and desktop wire contract; inspection routing remains bound to the trusted window/tab-owned Browser view.
| // same event that starts selection. IPC can overtake React's visibility | ||
| // effect, so make the selected page interactive before installing its | ||
| // pointer controller. A later renderer declaration remains authoritative. | ||
| entry.visible = true; |
There was a problem hiding this comment.
🚨 slopcop/review — [P1] Preserve the renderer visibility policy
entry.visible stores the renderer's required state. This assignment replaces false during an overlay, modal, or inactive tab.
The finally block does not restore that state. The native view can then cover host UI after the inspection.
Keep desired visibility and temporary inspection visibility separate. Clear the temporary state in finally, then apply the current desired policy.
There was a problem hiding this comment.
Fixed in ba61d30. Desired renderer visibility and the temporary inspection override are now separate. Inspection cleanup clears only the temporary state and reapplies the current desired policy; focused tests cover a view that must remain hidden.
| }; | ||
| }, [ensureRegistered, owner, ownershipRegistry, releaseAll]); | ||
|
|
||
| const experimental_setOverlayOpen = useCallback( |
There was a problem hiding this comment.
🚨 slopcop/review — [P1] Reject callbacks after action disposal
The cleanup unregisters this owner. However, a retained callback can call ensureRegistered after the action unmounts.
It can then add an overlay lease that no live mount releases. The Browser view stays hidden.
A retained inspection callback can also target the old tab. Track an active flag, and reject both callbacks after cleanup.
There was a problem hiding this comment.
Fixed in ba61d30 and 91e1c65. Retained inspection and overlay-acquire callbacks reject after disposal, so they cannot register an owner, inspect a stale tab, or leak a lease. Exact Electron QA also exposed React teardown calling overlay cleanup after disposal; false is now an idempotent no-op while true still rejects, with regression coverage.
| } | ||
| }; | ||
| const sanitizedClone = (element: Element): Element => { | ||
| const clone = element.cloneNode(true) as Element; |
There was a problem hiding this comment.
🚨 slopcop/review — [P2] Apply the DOM limits before cloning
cloneNode(true) copies the complete selected subtree before the 200-node and six-level limits apply.
Element inspection also copies the same subtree twice through descriptor. A large container can stall or stop the page renderer.
Build one sanitized copy with a bounded source traversal. Reuse that copy for the descriptor, DOM, and text fields.
There was a problem hiding this comment.
Fixed in ba61d30. Replaced cloneNode(true) with one bounded sanitized source traversal and reuse that result for descriptor/DOM/text output. The node and depth limits now apply before copying.
| const attributeNames = element.getAttributeNames(); | ||
| for (const name of attributeNames) { | ||
| const lower = name.toLowerCase(); | ||
| if ( |
There was a problem hiding this comment.
🚨 slopcop/review — [P2] Redact hidden secret fields
The sanitizer keeps href, src, srcset, action, formaction, and inline styles. These values can contain reset tokens or signed URLs.
It also checks only a local contenteditable attribute. A selected child in an editable parent can return typed text.
Remove secret-bearing URL values. Use the original element's editable state, and cover inherited editing and designMode in tests.
There was a problem hiding this comment.
Fixed in ba61d30. Sanitization drops secret-bearing URL attributes and uses the source element editable state, including inherited contenteditable and document designMode. Focused tests cover inherited editing and URL redaction.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This PR lets an installed plugin add a control to the Browser toolbar. The control can ask BB to select an element or region.
BB returns page details, styles, accessibility hints, and a screenshot. The PR also adds cancellation and overlay control.
Findings
I found six issues and left a line comment for each one.
- P1: The required CI check fails. The new slot is absent from the server authoring-doc contract map.
- P1: Callbacks remain usable after unmount. A stale callback can create a permanent overlay lease or inspect an old tab.
- P1: Inspection replaces the desired visibility state. A hidden native view can cover an overlay, modal, or inactive tab.
- P2: DOM limits apply after a full clone. Large selected subtrees can stall or stop the inspected renderer.
- P2: The desktop process ignores the required identity. The contract sends thread and project identity without validation or use.
- P2: Redaction misses hidden secret fields. URL attributes and inherited editable text can enter the inspection result.
The performance review also found unbounded work for each pointer event. An animation-frame limit would reduce selection jank.
Security and architecture
The strict IPC schemas, sender lookup, Browser sandbox, size limits, navigation cancellation, and cleanup paths look sound.
The page owns the selection mark. A trusted BB chrome indicator would make an active inspection clear on hostile pages.
I found no earlier DOM inspector that this PR duplicates. The PR correctly extends the existing view manager, IPC parser, slot store, and ownership boundary.
The host collector and test collector already duplicate registration validation. A shared normalizer could reduce later drift.
The new optional icon field is accepted but unused. The code should remove it or render it before stabilization.
Verification
- The app suite passed all 348 files and 2,776 tests.
- The desktop, desktop-contract, plugin SDK, and app test targets passed.
- Type checks and lint passed for the touched packages.
- The server type check failed with
Type 'true' is not assignable to type 'never'atplugin-authoring-docs.test.ts:169. - GitHub reports that every other required test and package smoke job passed.
- A real Electron test passed at the PR SHA. I selected the Example Domain heading and received its DOM data and PNG screenshot.
I posted this review as a comment. I did not approve the PR or request changes.
|
Addressed every actionable review finding and pushed ba61d30 plus the Electron teardown follow-up 91e1c65. Verification:
Also applied the informational performance/API cleanup notes: pointer hit-testing is animation-frame limited and the unused icon field was removed. I did not introduce a trusted-chrome indicator or collector refactor because those are broader architecture suggestions, not required correctness fixes. |
|
CI follow-up: the npm guard correctly rejected changed published 0.4.6 contents. PR #1640 already claims 0.4.7, so this branch now uses additive patch 0.4.8 and includes a persisted 0.4.6 scaffold-manifest upgrade regression. The npm version guard passes locally. |
Summary
Verification
BB-Thread-ID: thr_p9qmfkggwm