Skip to content

feat(sdk): add browser.open plugin host method — open URL in embedded browser - #15

Merged
howdeploy merged 4 commits into
howdeploy:mainfrom
4444cjtr:feat/plugin-browser-open
Aug 18, 2026
Merged

feat(sdk): add browser.open plugin host method — open URL in embedded browser#15
howdeploy merged 4 commits into
howdeploy:mainfrom
4444cjtr:feat/plugin-browser-open

Conversation

@4444cjtr

@4444cjtr 4444cjtr commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add an explicit browser:open capability for embedded Browser-card access; external:open remains limited to the system browser.
  • Validate plugin browser.open URLs in the main process: absolute normalized HTTP(S) only, localhost allowed, no credentials, bounded to 2,048 characters.
  • Replace the direct BrowserService/DOM-event paths with one correlated, awaitable IPC broker route for iframe, canvas, and window plugin surfaces.
  • Let App exclusively serialize Browser-card creation/reuse, one navigation, persistence, camera movement, and existing logical Browser focus.
  • Fail closed when Browser-card settings persistence rejects: the plugin promise now rejects rather than acknowledging an unpersisted UI transition. Ordinary best-effort settings UI remains toast-based.

Validation

  • npm test — 322 passed, 0 failed
  • npm run typecheck — passed
  • npm run build — passed
  • npm run audit:secrets — passed
  • CI=true npm run smoke:browser — 31 browser smoke steps passed
  • Targeted tests cover URL policy, broker correlation/timeout, renderer queue, single transport, distinct permission, and rejected settings persistence.

Manual Electron smoke

With an isolated user-data directory and a CSP-compatible plugin fixture, a real browser.open("http://127.0.0.1:8789/") created and persisted exactly one Browser card. The rendered card loaded Browser Persist Target at the expected URL; fixture processes and temporary state were removed after the check.

@4444cjtr
4444cjtr marked this pull request as draft August 16, 2026 02:25

@howdeploy howdeploy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Спасибо за вклад, @4444cjtr! Задумка с browser.open правильная и нужная. Но перед мержем нужен раунд правок — ниже находки независимого аудита (Codex CLI) плюс архитектурные вопросы мейнтейнерской стороны.

Критично к исправлению (аудит)

  1. Двойное выполнениеPluginFrame.tsx:216 + App.tsx:365: один вызов SDK выполняет browser.open(url) дважды — напрямую в PluginFrame и повторно через событие в App.openBrowser. Это двойная навигация вкладки, повторные HTTP-запросы, race при конкурентных вызовах. Нужен один путь оркестрации.
  2. Несогласованность слоёвregisterIpc.ts:292: для window-контрибуций метод вызывает BrowserService.open() напрямую — без карточки, без browserCanvas, без фокуса камеры. Один и тот же SDK-вызов ведёт себя по-разному для iframe- и window-контрибуций, что противоречит заявленной цели PR.
  3. Fire-and-forgetPluginFrame.tsx:220: promise SDK резолвится сразу после первого IPC, даже если показ карточки упадёт. Ошибка уходит в toast и не возвращается плагину. Нужен единый awaitable orchestration path в App без промежуточного успешного открытия.

Вопросы архитектуры (нужно решить до мержа)

  1. Permission-модель. Сейчас browser.open переиспользует external:open, чей задокументированный смысл — «передать ссылку ОС». Но встроенный браузер — это общая сессия с потенциально авторизованными сайтами и доступом к localhost/внутренней сети. Предлагаю: отдельное право browser:open + allowlist схем (http/https только, блок file:/about:) + явный пункт в UI согласия при установке.
  2. Стыковка с новым ядром навигации. Вчера в main влит PR #13 (переработанная модель focus/ownership канваса: виджеты владеют вводом по логическому фокусу, Browser-сёрфейсы латчат владение «страница/канвас»). Как открытая плагином браузерная карточка участвует в этой модели? Кто владеет вводом при открытии из плагина — виджет-инициатор или новая карточка? Проверь, пожалуйста, что флоу browser.open не обходит новую политику ownership.
  3. Почему событие, а не прямой вызов? Понимаю соображение (состояние browserCanvas принадлежит App), но событие canvastty:browser-open рвёт цепочку await и создаёт второй путь открытия. Может, вынести orchestration в метод App, доступный обоим слоям хоста через IPC, и вернуть плагину настоящий результат?
  4. Тесты. Заявлены 239 passing, но на сам browser.open теста нет — добавь, пожалуйста, хотя бы: успешное открытие карточки с URL, отказ без permission, одиночное (не двойное) выполнение.

Мерж-стратегия: этот PR входит в #16 — предлагаю сначала довести до ума и влить #15, потом #16 ребейзнуть (он похудеет). Спасибо!

…me) — home-widgets/canvas-apps hit this before main
…patches canvastty:browser-open, App runs full openBrowser(url) flow
@4444cjtr
4444cjtr force-pushed the feat/plugin-browser-open branch from c53298b to a1d02f0 Compare August 18, 2026 06:12
@4444cjtr
4444cjtr force-pushed the feat/plugin-browser-open branch from a1d02f0 to 63f7209 Compare August 18, 2026 06:41
@4444cjtr

Copy link
Copy Markdown
Contributor Author

Спасибо за детальный review. Все пункты закрыты в 63f72091ce80e583e6362503446a7f9c580b5180.

1. Двойное выполнение

  • Удалены direct BrowserService.open() из plugin-originated renderer path и CustomEvent("canvastty:browser-open").
  • PluginFrame теперь выполняет только await window.canvasTTY.plugins.openBrowser(pluginId, url).
  • Browser navigation выполняется ровно один раз в App.openBrowser; renderer queue сериализует параллельные plugin-open запросы.

2. Один контракт для iframe, canvas и window contribution

  • Все plugin-originated browser.open запросы теперь проходят один main-process маршрут: permission → URL policy → broker → trusted main renderer → App.
  • Window contribution больше не может открывать BrowserService напрямую, минуя Browser card, browserCanvas, камеру или focus.

3. Настоящий awaitable результат

  • Добавлен PluginBrowserOpenBroker с correlated requestId, cleanup, renderer-unavailable failure и 15-секундным timeout.
  • App отвечает broker-у только после Browser-card транзакции; SDK promise resolve/reject отражает реальный UI результат.

4. Отдельная capability и URL policy

  • Добавлено явное manifest permission browser:open; external:open остаётся только для системного браузера.
  • Обновлены schema, permission UI, i18n, SDK declaration, plugin/security docs.
  • Main-process policy разрешает только нормализованные абсолютные HTTP(S) URL, включая localhost; блокирует credentials, privileged schemes, malformed и overlong URL.

5. Ownership/focus из PR #13

  • App — единственный owner создания/reuse Browser card, persistence, camera transition и selection.
  • Открытая плагином карточка использует существующую Browser-card logical-focus/ownership модель PR Feat/canvas trackpad navigation #13, а не отдельный focus path.

6. Fail-closed persistence

  • Финальный независимый review выявил дополнительный случай: best-effort saveSettings() мог проглотить ошибку persistence и ложно подтвердить plugin-open.
  • Добавлен propagating persistSettingsUpdate(); Browser-card state применяется только после подтверждённого snapshot от main process. При ошибке openBrowser откатывает ref и broker возвращает { ok: false }.
  • Обычное UI-сохранение настроек по-прежнему показывает toast, не меняя UX.

7. Покрытие и проверка

Добавлены тесты для distinct permission, URL policy, broker correlation/timeout, renderer queue, единственного transport path и rejected persistence.

Локально выполнено:

  • npm test — 322 passed, 0 failed;
  • npm run typecheck;
  • npm run build;
  • npm run audit:secrets;
  • CI=true npm run smoke:browser — 31 шаг;
  • real Electron smoke с CSP-compatible plugin fixture: создана ровно одна Browser card, целевой URL загрузился, browserCanvas persisted.

PR #15 rebased на main уже с PR #13. Изменения будущего PR #16 не переносились в эту ветку.

@4444cjtr
4444cjtr requested a review from howdeploy August 18, 2026 06:58
@howdeploy
howdeploy marked this pull request as ready for review August 18, 2026 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants