Skip to content
Merged
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 @@ -67,6 +67,10 @@ vi.mock('@/app/features/settings/providers/hooks/queries', () => ({
}),
}));

vi.mock('../../hooks/use-default-model', () => ({
useDefaultModel: () => ({ data: null }),
}));

vi.mock('../model-tag-icons', () => ({
ModelTagIcons: () => null,
}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
} from '../hooks/queries';
import { useChatLoadingState } from '../hooks/use-chat-loading-state';
import { useConvexFileUpload } from '../hooks/use-convex-file-upload';
import { useDefaultModel } from '../hooks/use-default-model';
import { useEffectiveAgent } from '../hooks/use-effective-agent';
import { useFileIndexingStatus } from '../hooks/use-file-indexing-status';
import { useMergedChatItems } from '../hooks/use-merged-chat-items';
Expand Down Expand Up @@ -126,6 +127,7 @@ export function ChatInterface({

const { agent: effectiveAgent, isLoading: isAgentLoading } =
useEffectiveAgent(organizationId);
const { data: governanceDefault } = useDefaultModel(organizationId);

const [inputValue, setInputValue, clearInputValue] = usePersistedState(
chatDraftKey(user?.userId, organizationId, threadId),
Expand Down Expand Up @@ -422,7 +424,8 @@ export function ChatInterface({
},
selectedAgent: effectiveAgent,
modelId: effectiveAgent?.name
? selectedModelOverrides[effectiveAgent.name]
? (selectedModelOverrides[effectiveAgent.name] ??
governanceDefault?.modelId)
Comment on lines +427 to +428

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.

⚠️ Potential issue | 🟡 Minor

Align model fallback logic across send entry points.

Line 419 applies governance fallback for normal sends, but edit-and-branch still resolves model from override-only (Line 470). That creates inconsistent model selection behavior depending on how the user sends.

Suggested patch
-      const modelId = effectiveAgent.name
-        ? selectedModelOverrides[effectiveAgent.name]
-        : undefined;
+      const modelId = effectiveAgent.name
+        ? (selectedModelOverrides[effectiveAgent.name] ??
+          governanceDefault?.modelId)
+        : undefined;
...
     [
       editingMessage,
       dataThreadId,
       rootThreadId,
       effectiveAgent,
       selectedModelOverrides,
+      governanceDefault,
       organizationId,
       userContext,
       editAndBranchAction,
       selectNewBranch,
       setPendingMessage,
     ],
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@services/platform/app/features/chat/components/chat-interface.tsx` around
lines 419 - 420, The model fallback is inconsistent: normal sends use
(selectedModelOverrides[effectiveAgent.name] ?? governanceDefault?.modelId) but
the edit-and-branch path only checks overrides. Update the edit-and-branch model
resolution to use the same fallback (use
selectedModelOverrides[effectiveAgent.name] ?? governanceDefault?.modelId) or
extract a single helper like getEffectiveModelId(effectiveAgent) and use it from
both the normal send and edit-and-branch code paths so both paths resolve the
same model when an override is missing.

: undefined,
userContext,
arena: arenaContext ?? undefined,
Expand Down Expand Up @@ -473,7 +476,8 @@ export function ChatInterface({
async (newContent: string) => {
if (!editingMessage || !dataThreadId || !effectiveAgent) return;
const modelId = effectiveAgent.name
? selectedModelOverrides[effectiveAgent.name]
? (selectedModelOverrides[effectiveAgent.name] ??
governanceDefault?.modelId)
: undefined;

// Optimistic: show edited content immediately, truncate messages after it.
Expand Down Expand Up @@ -510,6 +514,7 @@ export function ChatInterface({
rootThreadId,
effectiveAgent,
selectedModelOverrides,
governanceDefault,
organizationId,
userContext,
editAndBranchAction,
Expand Down
42 changes: 36 additions & 6 deletions services/platform/app/features/chat/components/model-selector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { useT } from '@/lib/i18n/client';

import { useChatLayout } from '../context/chat-layout-context';
import { useChatAgents } from '../hooks/queries';
import { useDefaultModel } from '../hooks/use-default-model';
import { useEffectiveAgent } from '../hooks/use-effective-agent';
import { ModelTagIcons } from './model-tag-icons';

Expand All @@ -36,6 +37,7 @@ export function ModelSelector({ organizationId }: ModelSelectorProps) {
const { agents } = useChatAgents(organizationId);
const { providers } = useListProviders('default');
const { selectedModelOverrides, setSelectedModelOverride } = useChatLayout();
const { data: governanceDefault } = useDefaultModel(organizationId);
const [open, setOpen] = useState(false);

const supportedModels = useMemo(() => {
Expand Down Expand Up @@ -88,10 +90,26 @@ export function ModelSelector({ organizationId }: ModelSelectorProps) {
[modelInfoMap],
);

const currentModelId =
(effectiveAgent?.name && selectedModelOverrides[effectiveAgent.name]) ||
filteredModels[0] ||
null;
const currentModelId = useMemo(() => {
// 1. User's explicit override (localStorage) takes highest priority
if (effectiveAgent?.name && selectedModelOverrides[effectiveAgent.name]) {
return selectedModelOverrides[effectiveAgent.name];
}
// 2. Governance team/role default (if model is in agent's supported list)
if (
governanceDefault?.modelId &&
filteredModels.includes(governanceDefault.modelId)
) {
return governanceDefault.modelId;
}
// 3. Agent's primary model
return filteredModels[0] ?? null;
}, [
effectiveAgent?.name,
selectedModelOverrides,
governanceDefault,
filteredModels,
]);

// Clear stale override when agent changes
useEffect(() => {
Expand All @@ -110,13 +128,25 @@ export function ModelSelector({ organizationId }: ModelSelectorProps) {
const handleSelect = useCallback(
(modelId: string) => {
if (!effectiveAgent?.name) return;
if (modelId === filteredModels[0]) {
// Only clear the override when the user picks the effective default
// (governance default if present, otherwise the agent's primary model).
const effectiveDefault =
governanceDefault?.modelId &&
filteredModels.includes(governanceDefault.modelId)
? governanceDefault.modelId
: filteredModels[0];
if (modelId === effectiveDefault) {
setSelectedModelOverride(effectiveAgent.name, null);
} else {
setSelectedModelOverride(effectiveAgent.name, modelId);
}
},
[effectiveAgent?.name, filteredModels, setSelectedModelOverride],
[
effectiveAgent?.name,
filteredModels,
governanceDefault,
setSelectedModelOverride,
],
);

if (!currentModelId) return null;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { useConvexQuery } from '@/app/hooks/use-convex-query';
import { api } from '@/convex/_generated/api';

export function useDefaultModel(organizationId: string) {
return useConvexQuery(api.governance.default_model_query.getMyDefaultModel, {
organizationId,
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { describe, it, vi } from 'vitest';

import { checkAccessibility } from '@/test/utils/a11y';
import { render } from '@/test/utils/render';

import { DefaultModelEditor } from './default-model-editor';

vi.mock('@/app/hooks/use-toast', () => ({
useToast: () => ({ toast: vi.fn() }),
}));

vi.mock('../hooks/mutations', () => ({
useUpsertGovernancePolicy: () => ({ mutateAsync: vi.fn(), isPending: false }),
}));

vi.mock('../hooks/queries', () => ({
useGovernancePolicy: () => ({
data: null,
isLoading: false,
}),
}));

vi.mock('@/app/features/settings/teams/hooks/queries', () => ({
useOrgTeams: () => ({
teams: [{ id: 'team-1', name: 'Engineering' }],
isLoading: false,
}),
}));

vi.mock('@/app/features/settings/providers/hooks/queries', () => ({
useListProviders: () => ({
providers: [
{
name: 'openai',
displayName: 'OpenAI',
models: [
{
id: 'openai/gpt-4o',
displayName: 'GPT-4o',
tags: ['chat'],
},
],
},
],
isLoading: false,
}),
}));

vi.mock('@/app/hooks/use-ability', () => ({
useAbility: () => ({
can: () => true,
cannot: () => false,
}),
}));

vi.mock('@/app/hooks/use-organization-id', () => ({
useOrganizationId: () => 'org-1',
}));

describe('DefaultModelEditor', () => {
describe('accessibility', () => {
it('passes axe audit', async () => {
const { container } = render(
<DefaultModelEditor organizationId="org-1" />,
);
await checkAccessibility(container);
});
});
});
Comment on lines +60 to +69

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.

🛠️ Refactor suggestion | 🟠 Major

Add functional and error-path tests beyond the axe audit.

Line 62 only verifies accessibility. Please also add happy-path and failure-path tests for rule CRUD/save behavior (e.g., add/remove rule, invalid selection, mutation failure toast) to prevent regressions in this new governance editor.

As per coding guidelines **/*.{test,spec}.{ts,tsx,js,jsx}: “Tests should cover happy paths, edge cases, and error conditions.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@services/platform/app/features/settings/governance/components/default-model-editor.test.tsx`
around lines 60 - 69, Add unit tests in default-model-editor.test.tsx that cover
the DefaultModelEditor component's happy and error paths beyond the existing axe
audit: write a happy-path test that renders <DefaultModelEditor
organizationId="org-1" />, uses userEvent to add a rule, make a valid selection,
save, and assert the expected mutation was called and UI shows success (or
updated rule list); write edge-case tests for invalid selection that attempt to
save and assert validation UI prevents the save and shows the appropriate
validation message; write a failure-path test that mocks the save/mutation to
reject (or use msw to return 500) and assert the error toast is displayed; use
the component’s exported mutation or API helper (mock the function used by
DefaultModelEditor) and query the DOM to verify add/remove rule behavior and
toast messages.

Loading
Loading