Skip to content

Per-chat reasoning effort in contextual sheet#8674

Open
YoussefKeyrouz wants to merge 1 commit into
developfrom
feature/youssef/per_contextual_chat_reasoning
Open

Per-chat reasoning effort in contextual sheet#8674
YoussefKeyrouz wants to merge 1 commit into
developfrom
feature/youssef/per_contextual_chat_reasoning

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/1215062229903137?focus=true

Description

Wires the contextual Duck.ai sheet into the per-chat reasoning flow introduced by #8666 and fixes two correctness bugs

  • Minor cleanup in ReasoningModePickerViewModel

Steps to test this PR

Per-chat scope restored on reopen

  • Start a chat in the contextual sheet on tab A; pick a non-default model and reasoning effort; submit a prompt.
  • Close the contextual sheet.
  • Open new duck ai on another tab or submit a prompt via the native input field using a new model (global default changes)
  • Reopen the contextual sheet on tab A (within the session window). Verify the picker reflects the reasoning effort, not the global default)
  • Submit another prompt; confirm that the submission carries the chat's modelId and reasoningEffort.
  • Open another tab and start a new chat in the contextual sheet on tab B
  • Switching between tabs properly restore the reasoning effort of the corresponding chat.

Globals untouched

  • After any per-chat submission above, dismiss the sheet and open Duck.ai from the address bar (or a fresh tab). Verify your global defaults are unchanged.

Note

Medium Risk
Moderate risk: changes how the contextual sheet tracks/propagates the active chatId, which affects session restore behavior and per-chat model/reasoning selection; regressions could surface as incorrect picker state or chat continuity issues.

Overview
Adds a chatId state flow to DuckChatContextualViewModel, derived from the current/stored Duck.ai URL, and keeps it in sync whenever the sheet URL changes or a new chat is started.

Wires this chatIdFlow through DuckChatContextualFragment into ContextualNativeInputManager/NativeInputModeWidget so native input can participate in per-chat reasoning/model scoping when reopening/restoring contextual chats. Includes a small correctness/cleanup tweak in ReasoningModePickerViewModel to use a single modelState snapshot per tap, plus new unit tests covering chatId lifecycle (set on restore/load, unchanged in INPUT mode, cleared on new chat).

Reviewed by Cursor Bugbot for commit b204f93. 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/per_contextual_chat_reasoning branch from 4d75802 to 7ce9eef Compare May 22, 2026 19:26
@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/per_contextual_chat_reasoning branch from 7ce9eef to 7159e2d Compare May 22, 2026 19:54
@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
@YoussefKeyrouz YoussefKeyrouz requested a review from malmstein May 22, 2026 20:09
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.

tested locally and confirmed it works as expected.

clean wire-up of contextual into the per-chat reasoning flow. nice that this PR also fixes the fullModeUrl correctness gap on the existing-chat-restore and new-chat-clear paths — easy to miss those were never being assigned before. the ReasoningModePickerViewModel.onModeTapped snapshot of modelState is a small but real win against the chance of a fetchModels emission landing mid-handler.

main note is the fullModeUrl / _chatId pair-assignment drift risk below. separately worth keeping on the radar: PR #8631 (still open) adds Uri.toChatIdOrNull(duckChat) in duckchat-api doing similar parsing — once it lands, both this PR's extractChatId and any other in-flight extractors could converge on the api extension. not blocking either way.

note for outcome tracking: the model-fallback asymmetry I flagged on #8666 (L161, getSelectedModelId vs getResolvedReasoningEffort when chat's model isn't in modelState) isn't touched here — that one's still pending on #8666's side.

} else {
withContext(dispatchers.main()) {
fullModeUrl = existingChatUrl
_chatId.value = extractChatId(existingChatUrl)
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.

nit: fullModeUrl = ...; _chatId.value = extractChatId(...) is now repeated at L190-191, L243-244, and L289+292 (and a clearing pair at L649-650). a future addition that sets a new sheet URL but forgets the chatId update would silently break per-chat resolution. can we wrap this in a private helper like setSheetUrl(url: String?) that owns both fields, plus a clearSheetUrl() for the new-chat path? keeps the invariant in one place.

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.

Good catch. Addressed.

@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/per_contextual_chat_reasoning branch from 7159e2d to 9f586f9 Compare May 22, 2026 22:37
@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 9f586f9. Configure here.

Base automatically changed from feature/youssef/update_model_and_reasoning_picker_per_chat to develop May 22, 2026 22:53
@YoussefKeyrouz YoussefKeyrouz force-pushed the feature/youssef/per_contextual_chat_reasoning branch from 9f586f9 to b204f93 Compare May 22, 2026 22:55
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