Let plugin panels open native Browser tabs - #1497
Conversation
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this PR for security, code quality, performance, architecture, and user behavior. I will post one final review after the parallel checks finish. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| })), | ||
| [activateTab, activeBrowserTab?.id, browserTabs, closeTab], | ||
| ); | ||
| const browserPanel = browserTabs.length > 0 ? ( |
There was a problem hiding this comment.
🚨 slopcop/review — Closing the final tab can leak its native browser view.
This condition unmounts BrowserTabDeck when the tab list becomes empty. The deck cannot observe the removed ID and call destroyPersistedBrowserView. The content cleanup only releases visibility and does not detach the view. Please keep the deck alive for empty-list cleanup, or destroy the view before removal.
There was a problem hiding this comment.
Fixed in a72a514. The BrowserTabDeck now stays mounted when the list becomes empty so it observes the removed tab and detaches the final native view; the new ordering regression proves the detach.
|
|
||
| const openBrowserTab = useCallback( | ||
| ({ url }: { url: string }) => { | ||
| if (!isDesktopBrowserAvailable() || !isHttpOrHttpsUrl(url)) return false; |
There was a problem hiding this comment.
🚨 slopcop/review — The boundary accepts malformed and oversized URLs.
isHttpOrHttpsUrl checks only the scheme prefix, so https:// returns true. The later new URL(tab.url) call then throws outside the plugin error boundary. This also bypasses the native URL length limit. Please parse and limit the URL here, return false on failure, and add malformed-input tests.
There was a problem hiding this comment.
Fixed in a72a514. Plugin Browser URLs are now parsed once, restricted to HTTP(S), and capped at the desktop IPC limit before any tab is created; malformed and oversized inputs are covered by focused regressions.
| </div> | ||
| ); | ||
|
|
||
| return content.kind === "plugin-panel" ? ( |
There was a problem hiding this comment.
🚨 slopcop/review — This host does not cover the standalone plugin-panel path.
SplitThreadArea returns StandalonePaneContent when the viewport is compact or splits are off. That path renders PluginPanelView without this host. Therefore, openBrowserTab() always returns false on those normal surfaces. Please wrap both plugin-panel render paths and add compact and non-split tests.
There was a problem hiding this comment.
Fixed in a72a514. Standalone plugin panels are now wrapped by PluginPanelBrowserHost, covering compact and non-split surfaces; SplitThreadArea regressions verify both paths.
| browserTabs={browserTabs} | ||
| activeBrowserTabId={activeBrowserTab?.id ?? null} | ||
| environmentId={null} | ||
| canShowNativeBrowserView={ |
There was a problem hiding this comment.
🚨 slopcop/review — The native view can remain visible above an inactive split pane.
Wide mode gates visibility with isOpen only. A native WebContentsView ignores the DOM visibility used for inactive or maximized sibling panes. The thread host also requires pane focus for this reason. Please include pane focus and visibility in this gate, and test focus and maximize changes.
There was a problem hiding this comment.
Fixed in a72a514. Wide native-view visibility is now gated by the owning split pane's focus state, with focused coverage for focus/maximize transitions.
| * Returns false when the current surface cannot host a Browser tab or the | ||
| * URL is not HTTP(S), so callers can preserve an ordinary-link fallback. | ||
| */ | ||
| openBrowserTab(options: { url: string }): boolean; |
There was a problem hiding this comment.
🚨 slopcop/review — This public API does not follow the required experimental process.
The repository rules require an experimental_ prefix and an entry in docs/api_to_audit.md for each new plugin API member. This change adds neither. Please rename this method project-wide and add its audit criteria before release.
There was a problem hiding this comment.
Fixed in a72a514. The method is renamed project-wide to experimental_openBrowserTab, bundled/testing declarations are updated, and docs/api_to_audit.md now records the stabilization audit.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
This adds a web page beside a plugin, like one book beside another book. Some doors do not connect yet. One closed page can also stay hidden and use memory.
Findings
I found five concrete defects:
- The host does not mount for compact or non-split plugin panels. The method returns false on those standard surfaces.
- A native Browser view can remain above an inactive or maximized split pane. Native views ignore the DOM visibility rule.
- Closing the final tab can leak its live native Browser view.
- The URL boundary accepts malformed and oversized HTTP(S) strings. A value such as
https://can crash the host. - The new public SDK method lacks the required
experimental_prefix and the required audit entry.
The drawer settle callback also lacks the stale-work guards used by the thread and compose hosts. A close and quick reopen can show the native view before the drawer settles.
The new host repeats Browser deck, panel, and drawer-settle logic from the thread and compose hosts. A shared settle hook and Browser deck adapter would reduce this drift.
I found no other security problem. The existing native browser sandbox, permission limits, URL policy, and IPC checks remain active.
Checks
@bb/appand@bb/plugin-sdktype checks passed.- All 343 app test files passed, with 2,672 tests.
- All 76 plugin SDK tests passed.
git diff --checkpassed.- The dev app, server, host daemon, and Electron shell started.
- A browser smoke test opened the Automations plugin panel.
- I could not complete the new native action flow because no installed plugin invokes this method.
I left line comments for each primary defect. I did not approve this PR or request changes.
|
Superseded by upstream-origin PR #1546. The exact reviewed commit history is preserved there; this fork-origin PR remains available for its review history. |
1 participant
Summary
useBbNavigate().openBrowserTab({ url })for plugin nav panelsVerification
pnpm --filter @bb/plugin-sdk typecheckpnpm --filter @bb/plugin-sdk testpnpm --filter @bb/app typecheckStack
Base API layer. A child PR will add GitHub Activity as an official bundled plugin without mixing that catalog/package work into this API review.
BB-Thread-ID: thr_ehwczw5k36