DuckAi/ChatTab: Add New search and remove New Tab option from + menu - #9761
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c0ea2e3 to
3165abd
Compare
685b489 to
e6f494d
Compare
e6f494d to
d78dabf
Compare
|
|
||
| @SingleInstanceIn(AppScope::class) | ||
| @ContributesBinding(AppScope::class) | ||
| class InputScreenLaunchTargetImpl @Inject constructor() : InputScreenLaunchTarget { |
There was a problem hiding this comment.
@LukasPaczos I am repurposing the OnboardingInputScreenLauncher. Do youu mind taking a look and testing the usecase of that class?
There was a problem hiding this comment.
FWIW I prefer this since it takes the logic out of Onboarding and into a more purposeful API
malmstein
left a comment
There was a problem hiding this comment.
LGTM. Just a comment for the future API. nice work!
|
|
||
| @SingleInstanceIn(AppScope::class) | ||
| @ContributesBinding(AppScope::class) | ||
| class InputScreenLaunchTargetImpl @Inject constructor() : InputScreenLaunchTarget { |
There was a problem hiding this comment.
FWIW I prefer this since it takes the logic out of Onboarding and into a more purposeful API
LukasPaczos
left a comment
There was a problem hiding this comment.
The original OnboardingInputScreenLaunchTarget was deliberately scoped as narrowly as possible, and deliberately named Onboarding* so it wouldn't invite other uses. I think it's a dangerous API: it sets a token in global scope that gets consumed by whichever tab shows an input widget next. That could be the current tab regaining focus, or a new tab that opens next - the caller has no control over it. In practice it relies on the side effect that every new tab opens with the native input focused. If that side effect ever changes, every flow built on this launch target will quietly break. We also have race conditions where the outgoing tab briefly initializes while a new one is being created (case in point), which would consume the token prematurely.
Onboarding required this because it never sends an explicit "open a tab" or "open the input widget" command - it relies purely on the side effect, and we got away with it because we control that flow tightly enough. It works, but it's fragile, and if we ever change how CtaViewModel interacts with the BrowserTabViewModel, I'd like to remove it.
The new tab click handler integrated here is different: it already calls browserActivity?.launchNewTab(browserMode = BrowserMode.REGULAR) explicitly. That's a direct signal to the activity that we're launching a tab in this specific context. If we pass that context along to the created tab fragment and its VM, we get a much more direct and robust mechanism than the global token.
Could we explore that path first? Concretely, I mean something like:
browserActivity?.launchNewTab(browserMode = BrowserMode.REGULAR, inputModeTarget = InputMode.SEARCH)and thread that through to nativeInputManager.showNativeInput, rather than widening InputScreenLaunchTarget to more callers and risking silent bugs and race conditions.
Only if that turns out not to be feasible would I consider extending InputScreenLaunchTarget.
Wire the sidebar "New Search" menu action to open a regular new tab and auto-surface its native input on the Search tab. Generalize the onboarding input-screen one-shot into InputScreenLaunchTarget, carrying an InputMode instead of a DUCK_AI boolean, and have showOrHideKeyboard force the input open when a mode is armed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The launch target is a process-lifetime singleton consumed only in showNativeInput. When the native input field is inactive that path never runs, so an armed target stayed set for the whole process and made showOrHideKeyboard force focus on every call. Consume it in showOrHideKeyboard for the non-native path to keep the force-focus one-shot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1f24033 to
b58e76e
Compare
d78dabf to
aa68128
Compare
|
@LukasPaczos @malmstein I thought about it addressed all your comments. For now, I removed the |
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 aabfdda. Configure here.
| ) { | ||
| this.tabId = tabId | ||
| this.skipHome = skipHome | ||
| this.inputModeTarget = inputModeTarget |
There was a problem hiding this comment.
Launch mode lost on recreation
Medium Severity
loadData always overwrites inputModeTarget from the fragment field, which is not in arguments and comes back null after recreation. A surviving ViewModel then loses the New Search mode if consumeInitialInputMode has not run yet, so the input can open on the default toggle instead of Search.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit aabfdda. Configure here.
There was a problem hiding this comment.
This is not medium and could only occur as an edge case. Deferring for now.
malmstein
left a comment
There was a problem hiding this comment.
LGTM. thanks for reworking this, threading the target per tab through the pending map is clearer than the global one-shot, and consumeInitialInputMode makes the fallback ordering obvious.
Route the "New Search" launch context to the specific new tab via launchNewTab(inputModeTarget=SEARCH) instead of arming a global one-shot that any tab could consume. It rides PendingAction.OpenNewTab across a mode-switch recreate and is parked per-tabId until the tab's fragment is built (mirroring externalLaunchTabIds / messageFromPreviousTab), so the target reaches exactly the tab it opened. Re-narrow the global signal back to onboarding-only (OnboardingInputScreenLaunchTarget / setOpenOnDuckAi), and drop the force-focus override from showOrHideKeyboard so a launch target no longer silently outranks a CTA's focus-drop request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aabfdda to
c24acfb
Compare
Task/Issue URL: https://app.asana.com/1/137249556945/project/1212087397361015/task/1218350178023317?focus=true Tech Design URL (if applicable): API Proposals URL(s) (if applicable): https://app.asana.com/1/137249556945/project/1201462763415876/task/1218365860196482?focus=true ### Description - New Image (DuckChat.openDuckChatImageGeneration) switches the global model to an accessible image-capable model when needed, falling back to a normal new chat if none support image generation. ### Steps to test this PR _nativeDuckAiSidebar enabled — entry visibility_ - [x] Open the "+" chat menu — New Image entry IS visible (with the image icon) - [x] The entry below the divider now reads New Search _nativeDuckAiSidebar enabled, currently selected model already supports image generation_ - [x] Start a duck.ai chat from the NTP and set the Duck.ai model to one that supports image generation - [x] Tap + and select New Image - [x] A new Duck.ai chat tab opens (new session, not the existing chat) - [x] The native input is auto-focused on open - [x] The Image generation tool is preselected on the input - [x] The selected model is unchanged (no model switch occurs) - You might need to submit to see the model _nativeDuckAiSidebar enabled, selected model does NOT support image generation but another accessible model does_ - [x] Start a duck.ai chat from the NTP and set the Duck.ai model to one WITHOUT image-generation support - [x] Tap + and select New Image - [x] A new Duck.ai chat tab opens (new session, not the existing chat) - [x] The selected model is switched to an accessible image-capable model - You might need to submit to see the model - [x] The Image generation tool is preselected on the input - [x] The native input is auto-focused on open _nativeDuckAiSidebar disabled (via FF inventory)_ - [x] Open the "+" chat menu — New Image entry is NOT visible - [x] The entry below the divider still reads New Tab (not "New Search") <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes global Duck.ai model selection and cross-layer launch plumbing; behavior is gated on model capabilities with a documented fallback to a normal new chat. > > **Overview** > Wires the Duck.ai **+** menu **New Image** action to a full open flow: **`BrowserTabViewModel.openNewImageDuckChat`** calls **`DuckChat.openDuckChatImageGeneration`**, which may switch the global model to an accessible image-capable model, then opens a **new** Duck.ai session via **`BrowserNav.openDuckChat(..., forceImageGeneration)`**. > > A one-shot **`forceImageGeneration`** flag travels on the browser intent, is stashed in **`BrowserActivity`**, and is consumed when the new tab first shows native input so **`NativeInputModeWidgetViewModel`** preselects the image-generation tool and **`NativeInputManager`** focuses the field. If no accessible model supports image generation, the tab still opens but without forcing the tool. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a0c3718. 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.8 (1M context) <noreply@anthropic.com>
|
d404aed
into
feature/karl/duckai-sidebar/history-context-menu



Task/Issue URL: https://app.asana.com/1/137249556945/task/1218238186970934?focus=true
Tech Design URL (if applicable):
API Proposals URL(s) (if applicable):
Description
Steps to test this PR
Setup
nativeDuckAiSidebarenabled (default) — open the "+" menunativeDuckAiSidebardisabled (via FF Inventory) — open the "+" menuRegression check
Note
Medium Risk
Touches new-tab creation, activity recreate/deferred actions, and native input initial mode; behavior is gated on
nativeDuckAiSidebarbut onboarding Duck.ai landing must stay correct.Overview
When native Duck.ai sidebar is on, the omnibar + menu relabels New Tab to New Search, shows a New Image row (action still TODO), and opening that entry starts a regular tab with the native input screen on the Search tab—not Duck.ai.
Per-tab initial input mode is threaded through
launchNewTab→PendingAction.OpenNewTab(including bundle round-trip on browser-mode recreate) →pendingInputModeTargets/BrowserTabFragment.inputModeTarget→BrowserTabViewModel.consumeInitialInputMode(), which prefers that one-shot target and still falls back to post-onboarding Duck.ai viaOnboardingInputScreenLaunchTargetin the ViewModel (removed from the fragment).Tests cover
InputModepersistence across pending mode switch and one-shot consumption behavior.Reviewed by Cursor Bugbot for commit aabfdda. Bugbot is set up for automated code reviews on this repo. Configure here.