Skip to content

Per-chat model and reasoning restoration on existing Duck.ai chats.#8666

Open
YoussefKeyrouz wants to merge 2 commits into
developfrom
feature/youssef/update_model_and_reasoning_picker_per_chat
Open

Per-chat model and reasoning restoration on existing Duck.ai chats.#8666
YoussefKeyrouz wants to merge 2 commits into
developfrom
feature/youssef/update_model_and_reasoning_picker_per_chat

Conversation

@YoussefKeyrouz
Copy link
Copy Markdown
Collaborator

@YoussefKeyrouz YoussefKeyrouz commented May 22, 2026

Task/Issue URL: https://app.asana.com/1/137249556945/project/1212810093780571/task/1215057967719589?focus=true

Description

Tech design: https://app.asana.com/1/137249556945/project/1212810093780571/task/1214935104654912?focus=true

PR 3/3

When opening an existing Duck.ai chat, the input widget now restores the chat's stored model and reasoning effort instead of using the user's global defaults. Submissions in an existing chat carry the chat's values; the global "last used" model/reasoning remain untouched.

Implementation notes:

  • ReasoningModePickerViewModel observes nativeInputState.chatId, looks up the chat, and derives picker rows from the chat's stored model; displayed mode prefers the session pick over the chat's stored mode.
  • NativeInputModeWidgetViewModel submission getters return the chat's stored model and a reasoning effort routed through ReasoningResolver.resolveMode
  • DuckAiModelManager adds a session-only chatScopedReasoningMode field with a setter; not persisted, cleared on chatId change.
  • Shared helper ReasoningResolver.forChat(chat, modelState) centralises the chat-aware resolution.

Steps to test this PR

Existing chat - restoration

  • Send a message in a new chat, pick a reasoning mode (e.g. Thinking)
  • Navigate away and reopen that chat from history
  • Picker displays the chat's stored mode; submitting sends the correct selection

New chat - global defaults

  • Open a new chat submit using any model
  • Open another new chat
  • Picker shows your Last used model

Mid-chat reasoning change (session-only)

  • In an existing chat, tap the picker and change the mode
  • Submit; submission uses the picked mode
  • Open a new chat - your global default is unchanged

Same-tab navigation between chats

  • In one tab, open chat A and pick a reasoning mode
  • Navigate to chat B in the same tab
  • Picker shows chat B's stored mode (not A's pick)

Tab switching

  • Open chat A in tab 1 and chat B in tab 2
  • Switch between tabs; each tab's picker shows its own chat's mode and submission uses that tab's chat

Note

Medium Risk
Medium risk because it changes how model/reasoning are resolved and submitted for existing chats, adding new session-scoped state and async chat lookups that could affect selection correctness across tab/chat switches.

Overview
Existing Duck.ai chats now restore and submit their stored model and reasoning effort instead of always using the user’s global defaults.

This introduces a session-only chatScopedReasoningMode on DuckAiModelManager, adds ReasoningResolver.forChat(...) to resolve available modes/mode per chat model, and updates the input widget and reasoning picker to (a) load the active chat via DuckAiChatStore, (b) hide/drop interactions during in-flight chat switches or when a chat’s model is no longer available, and (c) clear chat-scoped overrides on chat changes so global preferences are not persisted/overwritten. Tests are expanded to cover chat switching, missing models, gated modes, and buffering chatId before configure().

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

Copy link
Copy Markdown
Collaborator Author

YoussefKeyrouz commented May 22, 2026

@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/update_model_and_reasoning_picker_per_chat branch from 76382a3 to f4928f6 Compare May 22, 2026 14:42
Copy link
Copy Markdown
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

did not install or smoke-test the APK.

the chatId race I flagged on #8664 is fixed exactly as suggested — _chatId + combine is gone, setActiveChatId now uses the synchronous activeTabId.value ?: return; publisher.update(tabId) { ... } pattern mirroring setSelectedTool. nice.

the test coverage on this PR is genuinely impressive — the CompletableDeferred race tests for "lookup in flight" and "chatId flips mid-load" are exactly the kind of coverage I was nervous about not having for the prior PR. validChat()'s "currentChat.value only counts if it matches the published chatId" guard is the right shape. likewise the picker's hide-on-mismatch + popup-dismiss-on-hide combo prevents stale taps from landing in the wrong chat scope.

main question is the model fallback asymmetry below — it's an edge case (chat's model removed/renamed by the backend) but worth confirming the semantics. small future note: currentChat is loaded independently in both the widget VM and the picker VM via separate getChatById calls. fine for now but worth keeping an eye on as more consumers want chat-derived state.


fun getSelectedModelId(): String? {
// Existing chat: send chat's stored model.
validChat()?.model?.let { return it }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when the chat's model is no longer in modelState.models (server stopped offering it, slug renamed, etc.), getSelectedModelId here still returns validChat().model unconditionally — but getResolvedReasoningEffort falls back to the global effort via forChat returning null. submission then carries (stale chat model, global effort), which is a weird pairing the backend wasn't designed for. the picker already hides itself for this case (chatResolution == null in resolveState), so the user has no UI signal that anything's off either. shouldn't getSelectedModelId fall back symmetrically — ReasoningResolver.forChat(chat, modelState) == null → use global for both? happy to be wrong if the backend tolerates unknown model ids gracefully, just want to make sure it's deliberate.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/update_model_and_reasoning_picker_per_chat branch from fafa32f to b60ace4 Compare May 22, 2026 16:21
@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/update_model_and_reasoning_picker_per_chat branch from b60ace4 to 2882db3 Compare May 22, 2026 19:26
@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/update_model_and_reasoning_picker_per_chat branch from 2882db3 to 6f9a5f1 Compare May 22, 2026 19:54
Base automatically changed from feature/youssef/load_model_and_effort_per_chat to develop May 22, 2026 22:36
@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/update_model_and_reasoning_picker_per_chat branch from 6f9a5f1 to caae8f5 Compare May 22, 2026 22:37
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 caae8f5. Configure here.

userTier = UserTier.FREE,
availableReasoningModes = listOf(gatedExtended(requires = listOf("free", "plus", "pro"))),
)
runCurrent()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Three upsell tests pass due to visibility, not logic

Low Severity

whenGatedModeRequiresFreeTierThenNoCommandEmitted, whenGatedModeAccessHasNoPublicTierThenNoCommandEmitted, and whenTappedModeNotInAvailableListThenNoCommandEmittedAndManagerNotCalled each set only one mode in availableReasoningModes. The new visibility condition (available.size > 1) causes onModeTapped to return at the !state.value.visible guard before reaching the upsell routing or "not in available" logic. These tests assert expectNoEvents() and pass, but for the wrong reason — they no longer exercise the code paths they were designed to cover.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit caae8f5. Configure here.

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