Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,12 @@ const AcpModelSelector: React.FC<{
defaultModelLabel,
fallbackLabel: t('conversation.welcome.useCliModel'),
});
const combinedLabel = composeRuntimeSelectorLabel({ modelLabel: display_label, thoughtLevel });
const defaultThoughtLevelLabel = t('common.default');
const combinedLabel = composeRuntimeSelectorLabel({
modelLabel: display_label,
thoughtLevel,
defaultThoughtLevelLabel,
});
const isRuntimeSetting = isConfigSetting(setStatus);
const handleThoughtLevelSelect = useCallback(
async (value: string) => {
Expand Down Expand Up @@ -171,7 +176,7 @@ const AcpModelSelector: React.FC<{
title={
<RuntimeSelectorSubMenuTitle
label={t('agent.thoughtLevel.label')}
value={getCurrentThoughtLevelLabel(thoughtLevel)}
value={getCurrentThoughtLevelLabel(thoughtLevel, defaultThoughtLevelLabel)}
/>
}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,39 @@ export type RuntimeSelectorModelGroup = { key: string; title: string; models: Ru
const matchesModelQuery = (model: RuntimeSelectorModel, keyword: string): boolean =>
(model.label || model.id).toLowerCase().includes(keyword);

export const getCurrentThoughtLevelLabel = (thoughtLevel: AcpDerivedOption | null | undefined): string => {
/** Structural subset both AcpDerivedOption and AgentRuntimeDerivedOption satisfy. */
type ThoughtLevelLike = Pick<AcpDerivedOption, 'options'> & { currentValue?: string | null };

/**
* Resolve the display label for the ACTIVE thinking level. The backend's
* `current_value` is the single source of truth: a known value maps to its
* option label (or itself). When the axis exists but no current is known,
* return `defaultLabel` (the caller passes the localized "Default") — an
* honest neutral, never a guess like `options[0]`, which may not be what the
* backend actually runs. No axis at all → empty (no suffix).
*/
export const getCurrentThoughtLevelLabel = (
thoughtLevel: ThoughtLevelLike | null | undefined,
defaultLabel = ''
): string => {
if (!thoughtLevel) return '';
if (!thoughtLevel.currentValue) return defaultLabel;
return (
thoughtLevel.options.find((item) => item.value === thoughtLevel.currentValue)?.label ||
thoughtLevel.currentValue ||
''
thoughtLevel.options.find((item) => item.value === thoughtLevel.currentValue)?.label || thoughtLevel.currentValue
);
};

export const composeRuntimeSelectorLabel = ({
modelLabel,
thoughtLevel,
defaultThoughtLevelLabel,
}: {
modelLabel: string;
thoughtLevel?: AcpDerivedOption | null;
thoughtLevel?: ThoughtLevelLike | null;
/** Localized "Default" shown when the thought axis exists but no current is known. */
defaultThoughtLevelLabel?: string;
}): string => {
const thoughtLevelLabel = getCurrentThoughtLevelLabel(thoughtLevel);
const thoughtLevelLabel = getCurrentThoughtLevelLabel(thoughtLevel, defaultThoughtLevelLabel);
if (!thoughtLevelLabel) return modelLabel;
return `${modelLabel} · ${thoughtLevelLabel}`;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,59 @@ export function deriveSelectOption(
};
}

/**
* Fallback option ids for the thought-level axis, matched only when no option
* carries `category: 'thought_level'` (the category stays authoritative).
* Covers every id the known backends emit: our Core's `reasoning_effort`, the
* legacy ACP aliases `effort`/`thinking_budget`, and `thinking` (also the id
* upstream PR #3597 matches, so the two changes stay compatible).
*/
export const THOUGHT_LEVEL_FALLBACK_IDS = [
'thought_level',
'reasoning_effort',
'effort',
'thinking',
'thinking_budget',
];

/**
* Anti-flicker merge for whole-snapshot replaces (`acp_config_option` push /
* REST reload): keep a known non-null `current_value` when the incoming frame
* carries NO current information at all.
*
* A frame where EVERY option's `current_value` is null is "informationless" —
* the backend simply had nothing selected to report yet (e.g. an early catalog
* push before its currents landed, or an older Core that never stamped
* currents) — and letting it clobber a current the UI already observed makes
* the picker flash Model-only. For those frames the previous per-option
* current is preserved (matched by category, then id).
*
* A frame with AT LEAST ONE non-null current is an informed snapshot: its
* nulls are authoritative and pass through. This is what keeps the Core's
* reject re-push working — after a backend refuses an effort set, the
* corrected frame still carries the model current, so its effort null WIPES
* the stale highlight instead of being "protected".
*/
export function mergeSnapshotPreservingKnownCurrents(
previous: AcpConfigOptionDto[] | null | undefined,
next: AcpConfigOptionDto[]
): AcpConfigOptionDto[] {
if (!previous?.length) return next;
const informed = next.some((option) => option.current_value != null);
if (informed) return next;
Comment on lines +117 to +118

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve authoritative all-null snapshot updates

When a backend intentionally clears the only exposed config option—or clears every option at once—its snapshot contains only null current_value fields. This branch classifies that authoritative update as informationless and restores every still-selectable previous value, leaving the selector showing stale model or thought-level state. A non-null sibling is not a reliable indicator of whether nulls are authoritative; preservation needs to distinguish omitted/partial data from explicit nulls rather than treating every all-null snapshot as empty information.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is an intentional anti-flicker tradeoff, and the distinction you're describing isn't representable at the current DTO level.

mergeSnapshotPreservingKnownCurrents only preserves previous currents for a frame where every option's current_value is null. Telling an authoritative all-null snapshot (a backend deliberately clearing every exposed option) apart from an informationless one (an early catalog push before currents land, or an older Core that never stamps currents) requires signaling omitted vs explicit null — but AcpConfigOptionDto carries current_value: null as the single representation for both cases. There is no field today that marks a null as authoritative.

Given that, preferring preservation for all-null frames is the safer default: it avoids the far more common flash-to-Model-only regression, while any frame carrying at least one non-null current stays authoritative — so the Core's reject re-push still wipes a refused level (the corrected frame keeps the model current, so its effort null passes through). The deliberate clear-everything-at-once case needs a DTO change to distinguish explicit nulls, which is out of scope for this PR. Leaving this thread open to track that as a follow-up rather than expanding scope here.

return next.map((option) => {
if (option.current_value != null) return option;
const prior = previous.find((candidate) =>
option.category ? candidate.category === option.category : candidate.id === option.id
);
if (prior?.current_value == null) return option;
// Only revive a current the incoming option can still represent — a stale
// value outside the new choice list would be its own lie.
const stillSelectable = option.options?.some((choice) => choice.value === prior.current_value);
return stillSelectable ? { ...option, current_value: prior.current_value } : option;
});
}

export function hasObservedValue(
response: SetConfigOptionResponse,
optionId: string,
Expand Down Expand Up @@ -200,8 +253,9 @@ export function useAcpConfigOptions({

const replaceSnapshot = useCallback(
(next: AcpConfigOptionDto[]) => {
optionsRef.current = next;
void mutate(next, false);
const merged = mergeSnapshotPreservingKnownCurrents(optionsRef.current, next);
optionsRef.current = merged;
void mutate(merged, false);
},
[mutate]
);
Expand Down Expand Up @@ -276,7 +330,7 @@ export function useAcpConfigOptions({
setStatus,
mode: deriveSelectOption(configOptions, 'mode', ['mode']),
model: deriveSelectOption(configOptions, 'model', ['model']),
thoughtLevel: deriveSelectOption(configOptions, 'thought_level', ['thought_level', 'reasoning_effort']),
thoughtLevel: deriveSelectOption(configOptions, 'thought_level', THOUGHT_LEVEL_FALLBACK_IDS),
reload,
setConfigOption,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ const AionrsModelSelector: React.FC<{
defaultModelLabel,
fallbackLabel: t('conversation.welcome.selectModel'),
});
const combinedLabel = composeRuntimeSelectorLabel({ modelLabel: label, thoughtLevel });
const defaultThoughtLevelLabel = t('common.default');
const combinedLabel = composeRuntimeSelectorLabel({ modelLabel: label, thoughtLevel, defaultThoughtLevelLabel });
const handleThoughtLevelSelect = (value: string) => {
if (!thoughtLevel || value === thoughtLevel.currentValue || !onSetThoughtLevel) return;
void onSetThoughtLevel(thoughtLevel.id, value);
Expand Down Expand Up @@ -135,7 +136,7 @@ const AionrsModelSelector: React.FC<{
title={
<RuntimeSelectorSubMenuTitle
label={t('agent.thoughtLevel.label')}
value={getCurrentThoughtLevelLabel(thoughtLevel)}
value={getCurrentThoughtLevelLabel(thoughtLevel, defaultThoughtLevelLabel)}
/>
}
>
Expand Down
11 changes: 7 additions & 4 deletions packages/desktop/src/renderer/pages/guid/GuidPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -445,10 +445,13 @@ const GuidPage: React.FC = () => {
if (resolvedDefaults.thoughtLevel && availableThoughtLevelValues.has(resolvedDefaults.thoughtLevel)) {
agentSelection.setSelectedThoughtLevelValue(resolvedDefaults.thoughtLevel, { persistPreference: false });
} else {
const fallbackThoughtLevel =
agentSelection.currentThoughtLevelOption.currentValue ||
agentSelection.currentThoughtLevelOption.options[0]?.value ||
'';
// No resolved default: mirror the backend current if it reported one,
// otherwise leave the selection empty (`''`). Falling back to
// `options[0]` here re-seeded the implicit override that the send path
// then sent as an explicit `thought_level`, silently defeating the
// assistant's backend default — the exact behavior this change removes
// (matches useGuidAssistantSelection's `''` fallback).
const fallbackThoughtLevel = agentSelection.currentThoughtLevelOption.currentValue || '';
agentSelection.setSelectedThoughtLevelValue(fallbackThoughtLevel, { persistPreference: false });
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,17 +99,18 @@ const GuidModelSelector: React.FC<GuidModelSelectorProps> = ({
fallbackLabel: defaultModelLabel,
});
}, [acpSelectedLabel, currentAcpCachedModelInfo?.current_model_id, defaultModelLabel, selectedAcpModel]);
const selectedThoughtLevelValue = thoughtLevelOption?.currentValue || thoughtLevelOption?.options[0]?.value || '';
const normalizedThoughtLevelOption =
thoughtLevelOption && thoughtLevelOption.options.length > 0
? {
...thoughtLevelOption,
currentValue: selectedThoughtLevelValue || null,
}
: null;
// The thought-level current is HONEST: only a real known value (user pick or
// backend-reported current) highlights; an unknown current renders as the
// localized "Default" instead of pretending options[0] is active — the
// backend resolves the actual default (assistant fixed default / its own
// launch default), and options[0] may not be it.
const defaultThoughtLevelLabel = t('common.default');
const visibleThoughtLevelOption =
thoughtLevelOption && thoughtLevelOption.options.length > 0 ? thoughtLevelOption : null;
const combinedAcpButtonLabel = composeRuntimeSelectorLabel({
modelLabel: acpButtonLabel,
thoughtLevel: normalizedThoughtLevelOption,
thoughtLevel: visibleThoughtLevelOption,
defaultThoughtLevelLabel,
});

if (isGeminiMode) {
Expand Down Expand Up @@ -208,7 +209,7 @@ const GuidModelSelector: React.FC<GuidModelSelectorProps> = ({
trigger='click'
droplist={
<Menu selectedKeys={selectedAcpModel ? [selectedAcpModel] : []}>
{normalizedThoughtLevelOption ? (
{visibleThoughtLevelOption ? (
<>
{/* Two-level layout: model row on top, thought-level row below;
each expands into a left-side submenu. */}
Expand All @@ -230,18 +231,18 @@ const GuidModelSelector: React.FC<GuidModelSelectorProps> = ({
title={
<RuntimeSelectorSubMenuTitle
label={t('agent.thoughtLevel.label')}
value={getCurrentThoughtLevelLabel(normalizedThoughtLevelOption)}
value={getCurrentThoughtLevelLabel(visibleThoughtLevelOption, defaultThoughtLevelLabel)}
/>
}
>
{normalizedThoughtLevelOption.options.map((item) => (
{visibleThoughtLevelOption.options.map((item) => (
<Menu.Item
key={item.value}
className={item.value === normalizedThoughtLevelOption.currentValue ? '!bg-2' : ''}
className={item.value === visibleThoughtLevelOption.currentValue ? '!bg-2' : ''}
onClick={() => onThoughtLevelSelect?.(item.value)}
>
<RuntimeSelectorCheckedItem
selected={item.value === normalizedThoughtLevelOption.currentValue}
selected={item.value === visibleThoughtLevelOption.currentValue}
description={item.description}
>
{item.label}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,6 @@ export const useGuidAssistantSelection = ({
const thoughtLevelSelectionScopeRef = useRef<string | null>(null);
useEffect(() => {
const optionValues = new Set(selectedAgentRuntimeThoughtLevelOption?.options.map((option) => option.value) ?? []);
const fallbackThoughtLevel =
selectedAgentRuntimeThoughtLevelOption?.currentValue ||
selectedAgentRuntimeThoughtLevelOption?.options[0]?.value ||
'';
const selectionScope = selectedAssistantId ?? '';

_setSelectedThoughtLevelValue((previousValue) => {
Expand All @@ -305,7 +301,15 @@ export const useGuidAssistantSelection = ({
return previousValue;
}

return fallbackThoughtLevel;
// No implicit selection: `''` means the user has not picked a level, so
// the create request sends NO thought_level override and the backend
// resolves the real default (assistant fixed default / auto preference /
// its own launch default). The old fallback pre-selected `currentValue`
// or even `options[0]` here, which the send path then SENT as an explicit
// conversation override — silently defeating the assistant's default.
// Display-wise the selector falls back to the catalog current or the
// localized "Default" (see GuidModelSelector).
return '';
Comment thread
itsklimov marked this conversation as resolved.
});
}, [selectedAgentRuntimeThoughtLevelOption, selectedAssistantId]);

Expand Down
87 changes: 86 additions & 1 deletion tests/unit/acpConfigOptions.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import type { AcpConfigOptionDto, SetConfigOptionResponse } from '@/common/types/platform/acpTypes';
import { deriveSelectOption, hasObservedValue } from '@/renderer/hooks/agent/useAcpConfigOptions';
import {
deriveSelectOption,
hasObservedValue,
mergeSnapshotPreservingKnownCurrents,
THOUGHT_LEVEL_FALLBACK_IDS,
} from '@/renderer/hooks/agent/useAcpConfigOptions';
import { describe, expect, it } from 'vitest';

const options: AcpConfigOptionDto[] = [
Expand Down Expand Up @@ -75,3 +80,83 @@ describe('ACP config option derivation', () => {
expect(hasObservedValue(response, 'model', 'gpt-5.5')).toBe(false);
});
});

describe('thought-level option matching', () => {
const thoughtDto = (overrides: Partial<AcpConfigOptionDto>): AcpConfigOptionDto =>
({
id: 'reasoning_effort',
option_type: 'select',
current_value: 'high',
options: [
{ value: 'low', name: 'Low' },
{ value: 'high', name: 'High' },
],
...overrides,
}) as AcpConfigOptionDto;

it.each(['thinking', 'thinking_budget', 'effort'])('matches the %s fallback id without a category', (id) => {
const derived = deriveSelectOption(
[thoughtDto({ id, category: undefined })],
'thought_level',
THOUGHT_LEVEL_FALLBACK_IDS
);
expect(derived?.id).toBe(id);
expect(derived?.currentValue).toBe('high');
});

it('prefers the thought_level category over a fallback-id match', () => {
const byCategory = thoughtDto({ id: 'custom', category: 'thought_level', current_value: 'low' });
const byId = thoughtDto({ id: 'thinking', category: undefined });
const derived = deriveSelectOption([byId, byCategory], 'thought_level', THOUGHT_LEVEL_FALLBACK_IDS);
expect(derived?.id).toBe('custom');
expect(derived?.currentValue).toBe('low');
});
});

describe('mergeSnapshotPreservingKnownCurrents (anti-flicker)', () => {
const snapshot = (modelCurrent: string | null, effortCurrent: string | null): AcpConfigOptionDto[] => [
{
id: 'model',
category: 'model',
option_type: 'select',
current_value: modelCurrent,
options: [{ value: 'gpt-5.5', name: 'GPT-5.5' }],
},
{
id: 'reasoning_effort',
category: 'thought_level',
option_type: 'select',
current_value: effortCurrent,
options: [
{ value: 'low', name: 'Low' },
{ value: 'high', name: 'High' },
],
},
];

it('preserves known currents when the incoming frame carries no current at all', () => {
const merged = mergeSnapshotPreservingKnownCurrents(snapshot('gpt-5.5', 'high'), snapshot(null, null));
expect(merged.find((o) => o.category === 'model')?.current_value).toBe('gpt-5.5');
expect(merged.find((o) => o.category === 'thought_level')?.current_value).toBe('high');
});

it('lets an informed frame clear a sibling current (the reject re-push)', () => {
// The Core reject re-push still knows the model current but deliberately
// nulls the refused effort — the null MUST win, not be "protected".
const merged = mergeSnapshotPreservingKnownCurrents(snapshot('gpt-5.5', 'high'), snapshot('gpt-5.5', null));
expect(merged.find((o) => o.category === 'thought_level')?.current_value).toBeNull();
});

it('does not revive a current the new option list no longer offers', () => {
const next = snapshot(null, null);
next[1] = { ...next[1], options: [{ value: 'medium', name: 'Medium' }] };
const merged = mergeSnapshotPreservingKnownCurrents(snapshot(null, 'high'), next);
expect(merged.find((o) => o.category === 'thought_level')?.current_value).toBeNull();
});

it('passes the frame through untouched when there is no previous snapshot', () => {
const next = snapshot(null, null);
expect(mergeSnapshotPreservingKnownCurrents(null, next)).toBe(next);
expect(mergeSnapshotPreservingKnownCurrents([], next)).toBe(next);
});
});
Loading
Loading