Retire wirePluginView() in native input - #9542
Open
malmstein wants to merge 9 commits into
Open
Conversation
The model picker resolves which model the input controls should reflect (recovery pick, else the chat's model, else the global selection), and the options menu currently gets at that answer by calling into the picker's view. Extracting the resolution into one app-scoped provider lets both plugins inject it instead, which is what allows the widget to stop wiring one plugin view to another. No behaviour change yet: this only adds the provider and its tests.
OptionsView was told which tools to show by the widget, which read the answer off the model picker's view: the only real cross-plugin edge in the native input. Options now resolves it itself from the shared EffectiveModelProvider plus the model list, and emits an event when the selected tool stops being supported so the host can clear it. Deletes updateCapabilitiesFrom, both of the widget's call sites, and the ModelPicker view members that existed only to answer it. The capability tests move from ModelPickerViewModelTest to the options and provider tests; the picker's remaining tests assert effectiveModelId instead.
ModelPickerViewModel had its own copy of the effective-model rule, which is now the provider's job. It keeps the local recovery pick, because the menu tick and hasPendingRecoverySelection are per-view concerns, and reports that pick to the provider so options sees the same model. The picker's existing tests exercise the provider through it, which is how the chip-label case caught an earlier version of onRecoveryModelPicked that made the caller supply a matching chatId. The window is per tab, so honouring the pick while modelChangeMode is open is enough.
The widget assigned three lambdas to the model picker's view to learn that its menu opened, closed, or submitted a model change. Those are plugin-to-host events, so they belong on NativeInputHost: the picker calls the host it was given at creation, and the widget implements the same behaviour it had in those lambdas. Clearing the selected tool becomes a state write instead of a call into OptionsView, so the widget no longer keeps that view either. What is left of wirePluginView is the AttachmentView block.
The widget handed dependencies and callbacks to concrete plugin views, which is what kept every new control tied to it. What is left of that plumbing moves to where it belongs: AttachmentView now injects its own ViewModel factory, state provider and favicon manager like the other plugin views, so bind() is gone. Its camera, file picker, ask-about-page and page-context-removed calls go through NativeInputHost, and it reads isContextual from the published state instead of being told. Plugins learn they are on the edit surface from host.isEditSurface() at creation, which removes the per-view isEditMode assignments. Submission still needs the staged attachments, so the widget resolves AttachmentViewModel through its own ViewModelStoreOwner (the same instance the plugin uses) rather than holding the plugin's view. That leaves the model picker as the widget's only plugin-view reference, kept for setPickerEnabled and openPicker.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The provider recorded the model picked during a model-change window and never dropped it. Gating on modelChangeMode only hid it while the window was closed, so the next window in the same activity reapplied the old pick, and the picker's menu tick disagreed because its own copy is reset. The picker already owns that lifecycle, so it now clears the provider alongside its local copy. Covered at both levels: the provider contract, and the picker case that regressed (fails without the clear).
Deriving isContextual from the published inputContext was wrong: the sheet shares its per-tab state slot with the omnibar widget and that slot can briefly carry BROWSER state, which would have hidden Ask about page and the attach control on the sheet. The widget already keeps a sticky flag for exactly this reason, so the plugin asks the host for it. Also cancel the attachment-state collector on detach. It was launched without keeping the job, so a re-attach would have added a second collector writing into the shared attachments container.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7423218. Configure here.
Contributor
Author
VerificationUnified-input Maestro suite, green on a local API 34 emulator (
The tool-pills flow is the relevant one for this PR: it exercises the path where options now derives tool visibility from Attachments verified manually on device (attaching photos, file limits, and related paths). There are no attachment flows in Unit: |
Storing one activity-scoped pick was wrong in both directions, and Bugbot caught the second half: the picker clears on modelChangeMode going false from the selected-tab flow, so leaving a tab whose window is still open wiped its pick, and returning to it showed the wrong model. The pick now carries the chatId whose window it belongs to. It applies only while that chat's window is open, and a clear only drops a pick for the same chat, so one tab's window closing cannot affect another's. The picker takes the key from published state rather than currentChat, which can still be resolving when the user picks.
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.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1157893581871903/task/1217549691747336
Tech Design URL (if applicable): https://app.asana.com/1/137249556945/task/1217490861467320
API Proposals URL(s) (if applicable): None
Description
NativeInputModeWidget.wirePluginView()handed dependencies and callbacks to concrete plugin views (AttachmentView,ModelPicker,OptionsView). It was already commented as "will be removed with the new plugin architecture", and the remaining control migrations add nine more plugins on top of it, so it goes first. Noduckchat-apichange, hence no API proposal.The apparent options-to-model-picker dependency turned out not to be real:
OptionsView.updateCapabilitiesFrom(picker)only asked whether image generation and web search were supported, and those resolve insideModelPickerViewModelfrom the app-scopedDuckAiModelManagerplus the active chat's model. Nothing view-private was ever read, so no plugin-to-plugin channel was needed.Commit by commit:
EffectiveModelProvider(new,ActivityScope): one home for "which model should the input controls reflect" — the recovery pick while the model-change window is open, else the chat's model, else the global selection.ActivityScopebecause the unqualifiedDuckAiChatStoreis deliberately bound per activity so it can resolve the browser mode.updateCapabilitiesFromand both widget call sites.hasPendingRecoverySelectionare per-view concerns.NativeInputHost(modelMenuShown,modelMenuDismissed,changeModelSubmitted) rather than the widget assigning lambdas to the view.clearSelectedToolbecomes a state write, so the widget stops holdingOptionsView.wirePluginViewdeleted.AttachmentViewinjects its ownViewViewModelFactory,NativeInputStateProviderandFaviconManagerlike every other plugin view, sobind()is gone; its camera, file-picker, ask-about-page and page-context-removed calls go through the host;isContextualcomes from published state. Plugins readhost.isEditSurface()at creation, which removes the per-viewisEditModeassignments.Submission still needs the staged attachments, so the widget resolves
AttachmentViewModelthrough its ownViewModelStoreOwner— the same instance the plugin uses — instead of holding the plugin's view. That removed nine view pass-throughs with no new component and no API change; the alternative was a per-tab attachment store, which is bigger and would have needed a proposal.Left deliberately: the widget still references the model picker for
setPickerEnabledandopenPicker. Picker-enabled is not derivable fromNativeInputStatetoday, because the two hosts mean different things by it (:appbinds new-chat-only, the contextual sheet binds input-mode-only to stop model changes mid-chat), and the flag has a second consumer ingetSelectedModelId(). Tracked as a follow-up on the Asana task.Steps to test this PR
Model capabilities and options
Attachments (the regression-prone path)
All hosts
UI changes
No UI changes: this is a wiring refactor, every control renders exactly as before.
Note
Medium Risk
Touches attachment submission, FE model-recovery, and options/tool clearing across omnibar, contextual sheet, and edit surfaces; behavior is intended to be unchanged but these paths are regression-prone.
Overview
Removes
NativeInputModeWidget.wirePluginView()and the widget’s direct wiring to plugin views. Plugins getisEditModefromNativeInputHost.isEditSurface()at creation;AttachmentViewself-injects and drives camera, file picker, page context, and contextual behavior through new host APIs instead of lambdas andbind().Introduces
EffectiveModelProvider(activity-scoped) as the single source for which model drives chip label and tool capabilities: recovery pick during FE model-change, else chat model, else global selection.ModelPickerViewModelandOptionsViewModelconsume it;OptionsViewno longer callsupdateCapabilitiesFromon the picker and instead observesvisibleTools/toolSelectionCleared. Model menu lifecycle and recovery submit move ontoNativeInputHost; tool clearing goes throughviewModel.setSelectedTool(null).Submission still reads staged attachments via shared
AttachmentViewModelon the widget’sViewModelStoreOwner, withapplyPendingAttachmentState()replaying page context and edit adoptees after attach.Reviewed by Cursor Bugbot for commit 225495d. Bugbot is set up for automated code reviews on this repo. Configure here.