fix: address assistant review feedback
This commit is contained in:
1 parent
33695753dd
commit
5ceef06b4b
26 files changed
+340
-49
No files matched your search
+9
-11
@@ -101,8 +101,8 @@ const ConfigurationSection = ({ number, title, description, status, actions, chi
|
||||
</section>
|
||||
);
|
||||
|
||||
const ConfigurationSkeleton = () => (
|
||||
<Stack className={classes.configuration} gap="md" aria-label="Loading assistant configuration">
|
||||
const ConfigurationSkeleton = ({ label }: { label: string }) => (
|
||||
<Stack className={classes.configuration} gap="md" aria-label={label}>
|
||||
<Paper withBorder p="md">
|
||||
<Group justify="space-between">
|
||||
<Group>
|
||||
@@ -357,7 +357,7 @@ export const AssistantConfiguration = () => {
|
||||
};
|
||||
|
||||
if (isLoading) {
|
||||
return <ConfigurationSkeleton />;
|
||||
return <ConfigurationSkeleton label={t("loading")} />;
|
||||
}
|
||||
|
||||
return (
|
||||
@@ -376,12 +376,12 @@ export const AssistantConfiguration = () => {
|
||||
<Badge
|
||||
size="sm"
|
||||
variant="light"
|
||||
color={connectionReady ? "green" : connectionChanged ? "yellow" : "gray"}
|
||||
color={connectionChanged ? "yellow" : connectionReady ? "green" : "gray"}
|
||||
>
|
||||
{connectionReady
|
||||
? t("overview.connectionReady")
|
||||
: connectionChanged
|
||||
? t("overview.unsavedChanges")
|
||||
{connectionChanged
|
||||
? t("overview.unsavedChanges")
|
||||
: connectionReady
|
||||
? t("overview.connectionReady")
|
||||
: t("overview.setupRequired")}
|
||||
</Badge>
|
||||
</Group>
|
||||
@@ -815,9 +815,7 @@ export const AssistantConfiguration = () => {
|
||||
</Box>
|
||||
<Group className={classes.selectedModelBadges} gap="xs" justify="flex-end">
|
||||
{selectedModel.contextLength && (
|
||||
<Badge variant="light">
|
||||
{t("model.context", { count: selectedModel.contextLength.toLocaleString() })}
|
||||
</Badge>
|
||||
<Badge variant="light">{t("model.context", { count: selectedModel.contextLength })}</Badge>
|
||||
)}
|
||||
<Badge
|
||||
variant="light"
|
||||
|
||||
@@ -0,0 +1,49 @@
|
||||
import { describe, expect, test } from "vitest";
|
||||
|
||||
import { getAssistantModelLookupStatus } from "./assistant-model-lookup";
|
||||
|
||||
describe("getAssistantModelLookupStatus", () => {
|
||||
test("keeps the configured default usable when discovery is temporarily unreachable", () => {
|
||||
expect(
|
||||
getAssistantModelLookupStatus({
|
||||
configuredModelId: "provider/default",
|
||||
requestedModelId: "provider/default",
|
||||
hasModel: false,
|
||||
failed: true,
|
||||
}),
|
||||
).toBe("available");
|
||||
});
|
||||
|
||||
test("treats an alternate model discovery failure as retryable", () => {
|
||||
expect(
|
||||
getAssistantModelLookupStatus({
|
||||
configuredModelId: "provider/default",
|
||||
requestedModelId: "provider/alternate",
|
||||
hasModel: false,
|
||||
failed: true,
|
||||
}),
|
||||
).toBe("unreachable");
|
||||
});
|
||||
|
||||
test("rejects an alternate model only after discovery confirms it is absent", () => {
|
||||
expect(
|
||||
getAssistantModelLookupStatus({
|
||||
configuredModelId: "provider/default",
|
||||
requestedModelId: "provider/alternate",
|
||||
hasModel: false,
|
||||
failed: false,
|
||||
}),
|
||||
).toBe("unavailable");
|
||||
});
|
||||
|
||||
test("accepts a discovered alternate model", () => {
|
||||
expect(
|
||||
getAssistantModelLookupStatus({
|
||||
configuredModelId: "provider/default",
|
||||
requestedModelId: "provider/alternate",
|
||||
hasModel: true,
|
||||
failed: false,
|
||||
}),
|
||||
).toBe("available");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,17 @@
|
||||
export type AssistantModelLookupStatus = "available" | "unavailable" | "unreachable";
|
||||
|
||||
export const getAssistantModelLookupStatus = ({
|
||||
configuredModelId,
|
||||
requestedModelId,
|
||||
hasModel,
|
||||
failed,
|
||||
}: {
|
||||
configuredModelId: string;
|
||||
requestedModelId: string;
|
||||
hasModel: boolean;
|
||||
failed: boolean;
|
||||
}): AssistantModelLookupStatus => {
|
||||
if (requestedModelId === configuredModelId) return "available";
|
||||
if (failed) return "unreachable";
|
||||
return hasModel ? "available" : "unavailable";
|
||||
};
|
||||
@@ -31,6 +31,7 @@ import type {
|
||||
|
||||
import { extractMcpTools } from "../../mcp/_extract-tools";
|
||||
import { getRequestedMentionIds, sanitizeAttachmentFilename } from "./assistant-chat-input";
|
||||
import { getAssistantModelLookupStatus } from "./assistant-model-lookup";
|
||||
import { getAssistantStreamErrorMessage } from "./assistant-stream-error";
|
||||
import { getForcedAssistantToolName, withAssistantToolPolicy } from "./assistant-tool-policy";
|
||||
|
||||
@@ -361,11 +362,35 @@ export async function POST(request: Request) {
|
||||
const requestStartedAt = Date.now();
|
||||
const requestId = crypto.randomUUID();
|
||||
const requestedModelId = parsed.data.modelId ?? configuration.modelId;
|
||||
const [selectedModel, mentionContext] = await Promise.all([
|
||||
getSelectedModelDetailsAsync(configuration, requestedModelId).catch(() => null),
|
||||
const [modelLookup, mentionContext] = await Promise.all([
|
||||
getSelectedModelDetailsAsync(configuration, requestedModelId).then(
|
||||
(model) => ({ model, error: null }),
|
||||
(error: unknown) => ({ model: null, error }),
|
||||
),
|
||||
getMentionContextAsync(context, parsed.data.messages),
|
||||
]);
|
||||
if (requestedModelId !== configuration.modelId && !selectedModel) {
|
||||
if (modelLookup.error !== null) {
|
||||
logger.warn("Assistant model discovery failed before the response started", {
|
||||
requestId,
|
||||
provider: configuration.provider,
|
||||
modelId: requestedModelId,
|
||||
error: modelLookup.error,
|
||||
});
|
||||
}
|
||||
const selectedModel = modelLookup.model;
|
||||
const modelLookupStatus = getAssistantModelLookupStatus({
|
||||
configuredModelId: configuration.modelId,
|
||||
requestedModelId,
|
||||
hasModel: selectedModel !== null,
|
||||
failed: modelLookup.error !== null,
|
||||
});
|
||||
if (modelLookupStatus === "unreachable") {
|
||||
return Response.json(
|
||||
{ error: "The configured model endpoint could not be reached. Try again in a moment." },
|
||||
{ status: 503 },
|
||||
);
|
||||
}
|
||||
if (modelLookupStatus === "unavailable") {
|
||||
return Response.json(
|
||||
{ error: "The selected model is not available from the configured provider." },
|
||||
{ status: 400 },
|
||||
|
||||
Reference in new issue
Block a user