Skip to content

Make NativeInputState.tabId non-null#8556

Merged
malmstein merged 1 commit into
developfrom
feature/david/native_input_tabid_non_null
May 15, 2026
Merged

Make NativeInputState.tabId non-null#8556
malmstein merged 1 commit into
developfrom
feature/david/native_input_tabid_non_null

Conversation

@malmstein
Copy link
Copy Markdown
Contributor

@malmstein malmstein commented May 14, 2026

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

  • Open the app on a NTP, type a search query, submit — search works as before
  • Open a website, focus the omnibar, switch between Search and Duck.ai tabs — toggle works, no flicker
  • Open a Duck.ai contextual session from a long-press menu — contextual UI renders correctly
  • Toggle "Address bar position" between top and bottom in Settings — widget reshapes correctly on next omnibar focus
  • Delete a tab, then the fire button — no leaked state, next tab renders fresh

UI changes

No UI changes — refactor only.


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.

Reviewed by Cursor Bugbot for commit 96d5ee1. Bugbot is set up for automated code reviews on this repo. Configure here.

@malmstein malmstein changed the base branch from develop to feature/david/native_input_widget_publisher May 14, 2026 11:54
private fun updateToggleVisibilityForState() {
if (isDuckAiPageContext()) return
applyToggleVisibility(nativeInputState.toggleVisible)
applyToggleVisibility(nativeInputState?.toggleVisible ?: false)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We are scattering the default fallbacks inline whenever we read a state variable. Would be harder to maintain if we add more call sites or if we do future modifications. I think a cleaner approach would be to have one default state. Something like val DEFAULT_STATE = NativeInputState(SEARCH_ONLY, BROWSER, TOP, tabId = “") (or similar) and use its values as fallbacks wherever needed.

Not blocking for this PR but would be good to have while it’s fresh.

override fun getInputState(): NativeInputState =
activeTabId?.let { nativeInputStateProvider.stateForTab(it).value } ?: nativeInputState
activeTabId?.let { nativeInputStateProvider.stateForTab(it).value }
?: error("getInputState called before widget was configured")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we’re throwing in the case where the state is not configured, do we need to have any nullable state? it could be non nullable and that will save us a lot of :? checks. right?

Copy link
Copy Markdown
Collaborator

@YoussefKeyrouz YoussefKeyrouz left a comment

Choose a reason for hiding this comment

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

Tested, I didn’t see any regression. Left a few comments, none of them are blocking but good to have.

@YoussefKeyrouz YoussefKeyrouz self-assigned this May 14, 2026
@malmstein malmstein force-pushed the feature/david/native_input_tabid_non_null branch from 7d57719 to a86e916 Compare May 15, 2026 15:25
Base automatically changed from feature/david/native_input_widget_publisher to develop May 15, 2026 15:38
Drop the String? = null default on NativeInputState.tabId and on
NativeInputState.zero(). Every NativeInputState in the per-tab store
is now provably associated with a tabId at the type level.

ViewModel state flow now combines activeTabId.filterNotNull() so the
emitted NativeInputState 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.

The widget's 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 placeholder; the publish-on-configure invariant
ensures plugins only reach this after configure.

ViewModel tests call configure() in @before so the state flow has a
tabId; the back-buttons test passes an explicit tabId.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malmstein malmstein force-pushed the feature/david/native_input_tabid_non_null branch from ba04601 to 1803308 Compare May 15, 2026 15:49
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 1803308. Configure here.

@malmstein malmstein force-pushed the feature/david/native_input_tabid_non_null branch from 1803308 to 96d5ee1 Compare May 15, 2026 20:11
@malmstein malmstein merged commit 20a1e18 into develop May 15, 2026
12 checks passed
@malmstein malmstein deleted the feature/david/native_input_tabid_non_null branch May 15, 2026 20:25
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