fix(pip): lazy-load NativeKit - #2056
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR updates NativeKit to 0.6.3, defers initialization until concrete preview requests, routes unavailable Browser previews to the side panel, disables Computer Use PiP when unavailable, updates related tests and documentation, and refreshes ACP and model catalog metadata. ChangesNative PiP routing
Provider catalog updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserCapture
participant YoBrowserPresenter
participant AgentPreviewCoordinator
participant BrowserSidePanel
BrowserCapture->>YoBrowserPresenter: request capture preview
YoBrowserPresenter->>AgentPreviewCoordinator: initialize and prepare target
AgentPreviewCoordinator-->>YoBrowserPresenter: native-overlay or none
YoBrowserPresenter->>AgentPreviewCoordinator: present frame
YoBrowserPresenter->>BrowserSidePanel: activate when surface is none
sequenceDiagram
participant CUA
participant ComputerUsePreviewPresenter
participant AgentPreviewCoordinator
participant NativeKit
CUA->>ComputerUsePreviewPresenter: concrete preview target
ComputerUsePreviewPresenter->>AgentPreviewCoordinator: initialize
AgentPreviewCoordinator->>NativeKit: start overlay
NativeKit-->>AgentPreviewCoordinator: capability result
AgentPreviewCoordinator-->>ComputerUsePreviewPresenter: native surface or none
ComputerUsePreviewPresenter->>NativeKit: present current frame
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
zerob13
marked this pull request as ready for review
July 29, 2026 05:04
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the native surface as the happy-path test default.
setup()still defaults torenderer-canvas, while the updated Computer Use outcomes arenative-overlaywhen available andnonewhen unavailable. If existing happy-path tests callsetup()without an argument, they can pass without exercising NativeKit delivery; prefernative-overlayas the default and make Canvas explicit only in a compatibility test, or verify why the legacy default is required.🤖 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 `@test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts` around lines 26 - 28, Change the default nativeSurface value in the setup helper to native-overlay so argument-less happy-path tests exercise NativeKit delivery. Keep renderer-canvas only in tests that explicitly validate canvas compatibility, and preserve none for unavailable-surface scenarios.
🤖 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 `@docs/architecture/nativekit-agent-browser-pip/plan.md`:
- Around line 69-90: Update the AgentPreviewCoordinator interface to include the
presenter-facing register, dismiss, and isCurrent methods used by
YoBrowserPresenter and ComputerUsePreviewPresenter, with signatures matching
their implementation. Keep the existing coordinator methods unchanged so the
documented contract fully reflects the available API.
In `@docs/features/agent-browser-pip/spec.md`:
- Around line 14-16: Update the linked heading associated with this
frame-delivery behavior from “NativeKit 0.6.0 surface migration” to “NativeKit
0.6.3 surface migration,” preserving the paragraph content and link target.
- Around line 49-50: Update the PiP behavior specification around the closed
right-side panel rule to remove the promise of a draggable in-chat preview; make
the “otherwise” behavior apply only when native PiP is available, while
preserving automatic side-panel opening when native PiP is unavailable.
In `@docs/features/computer-use-snapshot-pip/spec.md`:
- Around line 215-217: Update the shared coordinator behavior described in the
Computer Use PiP specification so frame-push failures remain transient: limit
permanent process-level native PiP disablement to NativeKit import/load,
startup, and host-attach failures, while retaining and retrying the previous
presentation after pushImage() failures. Apply the same correction to the
corresponding behavior statement at the additional referenced section.
In `@src/main/desktop/computerUse/ComputerUsePreviewPresenter.ts`:
- Around line 500-502: Update the presentation flow around
ComputerUsePreviewPresenter.presentCurrent so a prepare-time native failure that
makes previewCoordinator unavailable transitions the state out of eligible after
presentation; ensure shouldCaptureAfterClick no longer schedules private
snapshots, and apply the same unavailable-state handling during frame
processing.
In `@test/main/app/compositionBoundaries.test.ts`:
- Around line 5-14: Replace the source-text checks in the composition startup
test with a behavioral test that exercises composition startup using a mocked
coordinator. Assert the coordinator’s initialize() is not called during startup,
then request a concrete preview target and verify initialization occurs only at
that point.
---
Nitpick comments:
In `@test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts`:
- Around line 26-28: Change the default nativeSurface value in the setup helper
to native-overlay so argument-less happy-path tests exercise NativeKit delivery.
Keep renderer-canvas only in tests that explicitly validate canvas
compatibility, and preserve none for unavailable-surface scenarios.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 096dff42-db55-4ffc-8cc3-224f0efc5061
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
docs/architecture/nativekit-agent-browser-pip/plan.mddocs/architecture/nativekit-agent-browser-pip/spec.mddocs/architecture/nativekit-agent-browser-pip/tasks.mddocs/features/agent-browser-pip/spec.mddocs/features/computer-use-snapshot-pip/plan.mddocs/features/computer-use-snapshot-pip/spec.mddocs/features/computer-use-snapshot-pip/tasks.mdpackage.jsonresources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/main/app/composition.tssrc/main/desktop/browser/YoBrowserPresenter.tssrc/main/desktop/computerUse/ComputerUsePreviewPresenter.tssrc/main/desktop/preview/AgentPreviewCoordinator.tssrc/shared/types/desktop.tstest/main/app/compositionBoundaries.test.tstest/main/build/electronBuilderConfig.test.tstest/main/desktop/browser/YoBrowserPresenter.test.tstest/main/desktop/computerUse/ComputerUsePreviewPresenter.test.tstest/main/desktop/preview/AgentPreviewCoordinator.test.ts
💤 Files with no reviewable changes (2)
- src/shared/types/desktop.ts
- src/main/app/composition.ts
Comment on lines
+5
to
+14
| it('does not load NativeKit from the application startup workload', async () => { | ||
| const { readFileSync } = await vi.importActual<typeof import('node:fs')>('node:fs') | ||
| const compositionSource = readFileSync( | ||
| path.resolve(process.cwd(), 'src/main/app/composition.ts'), | ||
| 'utf8' | ||
| ) | ||
|
|
||
| expect(compositionSource).not.toContain('main:yo-browser') | ||
| expect(compositionSource).not.toContain('yoBrowserPresenter.initialize()') | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace source-text assertions with a startup behavior test.
These checks only lock implementation spellings and do not prove startup avoids NativeKit initialization. Exercise composition startup with a mocked coordinator and assert initialize() is untouched until a concrete preview target is requested.
🤖 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 `@test/main/app/compositionBoundaries.test.ts` around lines 5 - 14, Replace the
source-text checks in the composition startup test with a behavioral test that
exercises composition startup using a mocked coordinator. Assert the
coordinator’s initialize() is not called during startup, then request a concrete
preview target and verify initialization occurs only at that point.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts (2)
342-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting the exact internal
preparecall count.
toHaveBeenCalledTimes(2)couples this regression test to the presenter’s current call structure. Wait for the observable stop/release behavior instead, then assert only that preparation occurred and the unavailable surface was handled.As per coding guidelines, committed tests should remain lean and focused on observable contracts rather than implementation internals.
🤖 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 `@test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts` at line 342, Update the regression test around coordinator.prepare to wait for the observable stop/release behavior instead of asserting an exact prepare call count. After that wait, assert that preparation occurred and that the unavailable surface was handled, preserving the test’s focus on externally observable behavior.Source: Coding guidelines
62-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winModel NativeKit initialization state in the test double.
isAvailable()returnstruefor native surfaces beforeinitialize()runs, so the success-path tests could pass even if the presenter skipped deferred initialization. Track initialization/disablement in the mock and assert that native preparation cannot proceed before initialization.The PR objective requires NativeKit to load only for the first concrete target.
🤖 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 `@test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts` around lines 62 - 63, Update the NativeKit test double’s initialize and isAvailable methods to track initialization and disablement state, so native surfaces are unavailable until initialize completes and remain unavailable when initialization is disabled. Add assertions covering the pre-initialization state and verify that native preparation proceeds only after initialization, preserving the requirement that NativeKit loads only for the first concrete target.test/main/desktop/preview/AgentPreviewCoordinator.test.ts (1)
302-304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that startup failure disables NativeKit process-wide.
The test verifies that
prepare()returns'none', but not thatcoordinator.isAvailable()becomesfalse. Without that assertion, a regression could leave the coordinator globally available and cause later presenters to retry NativeKit after startup failure.The PR objective explicitly requires NativeKit to be disabled process-wide when loading or startup fails.
🤖 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 `@test/main/desktop/preview/AgentPreviewCoordinator.test.ts` around lines 302 - 304, Extend the startup-failure test for AgentPreviewCoordinator by asserting that coordinator.isAvailable() returns false after initialize() resolves false. Keep the existing prepare() and overlay.attachHost assertions unchanged, ensuring NativeKit is verified as disabled process-wide after loading or startup failure.
🤖 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.
Nitpick comments:
In `@test/main/desktop/computerUse/ComputerUsePreviewPresenter.test.ts`:
- Line 342: Update the regression test around coordinator.prepare to wait for
the observable stop/release behavior instead of asserting an exact prepare call
count. After that wait, assert that preparation occurred and that the
unavailable surface was handled, preserving the test’s focus on externally
observable behavior.
- Around line 62-63: Update the NativeKit test double’s initialize and
isAvailable methods to track initialization and disablement state, so native
surfaces are unavailable until initialize completes and remain unavailable when
initialization is disabled. Add assertions covering the pre-initialization state
and verify that native preparation proceeds only after initialization,
preserving the requirement that NativeKit loads only for the first concrete
target.
In `@test/main/desktop/preview/AgentPreviewCoordinator.test.ts`:
- Around line 302-304: Extend the startup-failure test for
AgentPreviewCoordinator by asserting that coordinator.isAvailable() returns
false after initialize() resolves false. Keep the existing prepare() and
overlay.attachHost assertions unchanged, ensuring NativeKit is verified as
disabled process-wide after loading or startup failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f410f370-1aa9-434b-934f-97303171a454
📒 Files selected for processing (7)
docs/architecture/nativekit-agent-browser-pip/plan.mddocs/features/agent-browser-pip/spec.mddocs/features/computer-use-snapshot-pip/spec.mdsrc/main/desktop/computerUse/ComputerUsePreviewPresenter.tssrc/main/desktop/preview/AgentPreviewCoordinator.tstest/main/desktop/computerUse/ComputerUsePreviewPresenter.test.tstest/main/desktop/preview/AgentPreviewCoordinator.test.ts
💤 Files with no reviewable changes (1)
- src/main/desktop/preview/AgentPreviewCoordinator.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/features/agent-browser-pip/spec.md
- src/main/desktop/computerUse/ComputerUsePreviewPresenter.ts
- docs/features/computer-use-snapshot-pip/spec.md
- docs/architecture/nativekit-agent-browser-pip/plan.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@zerob13/nativekitfrom0.6.2to0.6.3Root cause
The package import was dynamic, but the startup workload immediately called the preview initializer, so NativeKit was effectively loaded eagerly. A missing native dependency could therefore affect application startup, and the failure path selected the renderer Canvas fallback for both Browser and Computer Use.
User impact
NativeKit
0.6.3also statically links the Windows x64 MSVC runtime, reducing deployment-time dynamic runtime failures.Verification
pnpm run formatpnpm run i18npnpm run lintpnpm run typecheckpnpm run buildawait import('@zerob13/nativekit')Physical Windows packaged smoke testing remains release QA.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests