Widget publishes native input state per tab#8550
Conversation
| activeTabId = tabId | ||
| pendingIsDuckAiMode = true | ||
| doOnAttach { | ||
| viewModel.configureContextual() |
There was a problem hiding this comment.
Stale state causes applyOmnibarShape to permanently skip
Medium Severity
The old configure() synchronously initialized nativeInputState from viewModel.state.replayCache.lastOrNull(), which reflected the real computed inputMode. The new code replaces that with observeNativeInputState(), which reads from NativeInputStateProvider. The provider's StateFlow contains NativeInputState.zero (with inputMode = SEARCH_AND_DUCK_AI, hence toggleVisible = true) until the ViewModel's async coroutine chain publishes the real state. applyOmnibarShape is called immediately after and checks nativeInputState.toggleVisible — the zero state's true causes an early return, skipping shape application. Since applyState() never calls applyOmnibarShape, the shape is permanently wrong for users in SEARCH_ONLY mode.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 20634c4. Configure here.
20634c4 to
653aebc
Compare
NativeInputModeWidgetViewModel now injects NativeInputStatePublisher and publishes the combined input state to the per-tab store whenever an active tabId is set. The widget reads its state via NativeInputStateProvider.stateForTab(tabId) — the local cache is no longer the source of truth. configure() and configureContextual() take a tabId parameter; the value is threaded from BrowserTabFragment.tabId through NativeInputManager and from DuckChatContextualFragment through ContextualNativeInputManager. NativeInputHost.getInputState() still exists but now reads from the provider, so a future PR can drop it without further refactoring. NativeInputState.tabId stays nullable for now; a follow-up will tighten it once the publish-on-configure invariant is verified in practice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
applyOmnibarShape() previously took an isBottom param passed in from configure()'s doOnAttach block. It ran before the first state emission propagated through the provider, so it also read nativeInputState.toggleVisible — which was still the class-level field initializer (toggleVisible = false), not the real computed value. For users with Search + DuckAI enabled the prior viewModel.state-backed path typically early-returned at the toggleVisible check, so the new publisher path was applying shape changes the previous code would have skipped. Drop the param, read isBottom and toggleVisible from nativeInputState, and invoke applyOmnibarShape() from applyState() after the cache is updated. The operation is idempotent, so re-running on every state emission is fine. Flagged by Cursor Bugbot on PR #8550. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
653aebc to
e8d711f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ 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 e8d711f. Configure here.
231d816 to
d08fbc3
Compare
GerardPaligot
left a comment
There was a problem hiding this comment.
Work as expected. I tried to reproduce what is reported by Bugbot but I didn't find a way to reproduce. Good for me.
| setupBackPressHandling() | ||
| val tabId = requireArguments().getString(KEY_DUCK_AI_CONTEXTUAL_TAB_ID) | ||
| contextualNativeInputManager.init( | ||
| tabId = tabId.orEmpty(), |
There was a problem hiding this comment.
Not sure if this could happen, but if the argument is missing, then we will be storing the data under the “” key. So any contextual fragment that might hit this path will share the same stored map entry. Should be have any null enforcement here?
applyOmnibarShape() mutates the parent omnibar MaterialCardView's radius and margins. In DUCK_AI_CONTEXTUAL the widget is hosted inside the contextualNativeInputCard, which already has a top-only corner shape applied by ContextualNativeInputManager.applyCardShape(). The existing guards (isBottom, toggleVisible) both pass in contextual context, so applyOmnibarShape ran and clobbered the contextual card's shape. Return early when inputContext == DUCK_AI_CONTEXTUAL so the contextual card's custom shape is preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
requireArguments().getString() returns nullable. The previous code defaulted to "" via orEmpty(), which meant a missing argument would silently key state under "" — letting any two contextual fragments hitting that path share state. The only caller (BrowserTabFragment) passes a non-null tabId, so a missing argument indicates a contract break, not a runtime case to absorb. Wrap the bundle read in requireNotNull to fail fast, drop the orEmpty fallback, and remove the now-redundant tabId?.let block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Task/Issue URL: https://app.asana.com/1/137249556945/project/1214157224317277/task/1214802223978853?focus=true Stacked on #8550. Until that merges, the diff here shows PR 2's commits too; review the top commit (`7d57719`) for the PR 2.5 change in isolation. ### Description Small follow-up to PR 2. Now that the widget publishes on configure, every `NativeInputState` in the per-tab store is provably associated with a `tabId`, so we tighten the type. - Drop the `String? = null` default on `NativeInputState.tabId` and `NativeInputState.zero()`. - ViewModel `state` flow combines `activeTabId.filterNotNull()` so the emitted state carries the active tabId. The publish init block simplifies to a direct collect-and-publish — the separate combine that re-attached the tabId is gone. - Widget local cache becomes nullable (`NativeInputState?`). Read sites use null-safe access with the same defaults the previous field initializer carried. - `getInputState()` throws if called pre-configure instead of returning a stale placeholder; the publish-on-configure invariant ensures plugins only reach it after configure. - Tests: `@Before` calls `configure()` so the state flow has a tabId; the back-buttons test passes an explicit tabId. ### Steps to test this PR _Native input widget regressions_ - [x] Open the app on a NTP, type a search query, submit — search works as before - [x] Open a website, focus the omnibar, switch between Search and Duck.ai tabs — toggle works, no flicker - [x] Open a Duck.ai contextual session from a long-press menu — contextual UI renders correctly - [x] Toggle "Address bar position" between top and bottom in Settings — widget reshapes correctly on next omnibar focus - [x] Delete a tab, then the fire button — no leaked state, next tab renders fresh ### UI changes No UI changes — refactor only. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Tightens a core UI/state model contract and changes initialization/placeholder behavior; mis-ordered widget configuration could now throw or suppress UI updates until `tabId` is set. > > **Overview** > Makes `NativeInputState.tabId` mandatory by removing the nullable/default value and updating `zero(tabId)` accordingly. > > Updates `NativeInputModeWidgetViewModel` to only emit `state` once `activeTabId` is non-null (via `activeTabId.filterNotNull()`), embedding the `tabId` in every emitted `NativeInputState` and simplifying publishing to a direct collect-and-publish. > > Adjusts `NativeInputModeWidget` to treat its cached `NativeInputState` as nullable (null-safe reads with safe defaults) and makes `getInputState()` fail fast if called before `configure()`. Tests are updated to pass explicit `tabId`/call `configure()` up front. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 96d5ee1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>


Task/Issue URL: https://app.asana.com/1/137249556945/project/1157893581871903/task/1214790107756091?focus=true
Stacked on top of #8549.
Description
PR 2 of the native input state refactor — see the parent task for the full plan.
The native input widget is now the single publisher of
NativeInputStateand reads its own state from the per-tab store (NativeInputStateProvider) instead of keeping it as a widget-local field. Plugins still won't see this change until PR 3.Concretely:
NativeInputModeWidgetViewModelinjectsNativeInputStatePublisher. A newinit { … }block collectscombine(state, activeTabId)and pushes each value throughpublisher.publish(tabId, state.copy(tabId = tabId))whenever a tabId is set.NativeInputModeWidgetinjectsNativeInputStateProvider.observeNativeInputState()now subscribes toprovider.stateForTab(tabId)instead ofviewModel.state.getInputState()readsprovider.stateForTab(activeTabId).value.configure()andconfigureContextual()take atabId: Stringparameter and start the observation once the tabId is known.tabIdis threaded:BrowserTabFragment.tabId→NativeInputManager.showNativeInput(tabId, …)→attachWidget(widgetView, isBottom, tabId)→widget.configure(tabId, …).DuckChatContextualFragmentreads itsKEY_DUCK_AI_CONTEXTUAL_TAB_IDargument once and passes it toContextualNativeInputManager.init(tabId, …)→widget.configureContextual(tabId).viewModel.statestays as aSharedFlow<NativeInputState>so existing ViewModel-level tests can keep asserting on its values; the widget no longer consumes it.What this PR intentionally does not do:
NativeInputState.tabIdstays nullable. Tightening to non-null will follow once the publish-on-configure invariant is verified in practice (and we can drop thezero("__init__")-style placeholder).NativeInputHost.getInputState()is still exposed — its implementation now reads from the provider, but the surface remains until plugins are migrated in PR 3 and it's removed in PR 4.NativeInputHost; nothing about their behaviour changes here.Steps to test this PR
:duckchat-impl:testDebugUnitTest,:app:testPlayDebugUnitTest,spotlessCheck.NativeInputModeWidgetViewModelTestcases still pass after theconfigure/configureContextualsignature changes and the added publisher mock.UI changes
None.
Note
Medium Risk
Changes how the native input widget publishes/consumes state by keying everything on
tabId, which could cause cross-tab state leaks or missing updates if tab IDs aren’t consistently threaded through all entry points.Overview
The native input widget now publishes
NativeInputStateper tab and reads state fromNativeInputStateProviderinstead of relying on widget-local/ViewModel flow state.This threads
tabIdthroughBrowserTabFragment→NativeInputManager.showNativeInput/attachWidget→NativeInputModeWidget.configure, and through contextual chat viaDuckChatContextualFragment/ContextualNativeInputManager.init→configureContextual. The ViewModel now injectsNativeInputStatePublisherto push updates for the active tab, and tests are updated for the newtabId-requiredconfigureAPIs.Reviewed by Cursor Bugbot for commit 9dc03e8. Bugbot is set up for automated code reviews on this repo. Configure here.