mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 21:10:59 +00:00
fix(api): provider-chain GET preserves disabled entries (H2 code-review)
The GET response used to surface the resolveProviderChain()-filtered list, which DROPS disabled entries upstream. The Settings UI then seeded `enabled: true` for every returned row, so disabling an entry made it disappear on next refetch and lose its toggle position — the toggle was functionally a "remove from chain" button. Changed GET to read the *persisted* chain via parseProviderChain() so disabled rows still round-trip with their flag. The active-provider summary still uses resolveProviderChain so the displayed "Active: …" line continues to reflect the actual runner ordering (enabled-only). The Settings UI seeds entries with the wire-supplied `enabled` value instead of always-true, so re-enabling becomes possible without a page reload. 4 unit tests pin the GET shape (persisted-chain ordering, disabled entries surfaced, cached-active overlay, default-chain fallback). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
6863ecb87d
commit
5f7b9d8652
3 files changed
+126
-17
No files matched your search
@@ -14,6 +14,11 @@ vi.mock("@/lib/api-handler", () => ({
|
||||
user: { id: "u-1" },
|
||||
session: { id: "s-1" },
|
||||
})),
|
||||
HttpError: class HttpError extends Error {
|
||||
constructor(public status: number, message: string) {
|
||||
super(message);
|
||||
}
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/ai/provider", () => ({
|
||||
@@ -28,25 +33,47 @@ vi.mock("@/lib/logging/context", () => ({
|
||||
annotate: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/db", () => ({
|
||||
prisma: {
|
||||
user: {
|
||||
findUnique: vi.fn(async () => ({ aiProviderChain: null })),
|
||||
},
|
||||
},
|
||||
}));
|
||||
|
||||
import { GET } from "../route";
|
||||
import { resolveProviderChain } from "@/lib/ai/provider";
|
||||
import { getLastWorkingProvider } from "@/lib/ai/provider-runner";
|
||||
import { prisma } from "@/lib/db";
|
||||
|
||||
interface Envelope {
|
||||
data?: {
|
||||
activeProvider: string | null;
|
||||
cachedActiveProvider: string | null;
|
||||
configuredChain: { providerType: string; available: boolean }[];
|
||||
configuredChain: {
|
||||
providerType: string;
|
||||
enabled: boolean;
|
||||
available: boolean;
|
||||
}[];
|
||||
};
|
||||
error?: string;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
aiProviderChain: null,
|
||||
} as never);
|
||||
});
|
||||
|
||||
describe("GET /api/insights/provider-chain", () => {
|
||||
it("returns the resolved chain in priority order", async () => {
|
||||
it("returns the persisted chain (with enabled flags) and the active resolver winner", async () => {
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
aiProviderChain: [
|
||||
{ providerType: "codex", priority: 1, enabled: true },
|
||||
{ providerType: "admin-openai", priority: 2, enabled: true },
|
||||
],
|
||||
} as never);
|
||||
vi.mocked(resolveProviderChain).mockResolvedValue([
|
||||
{
|
||||
providerType: "codex",
|
||||
@@ -64,12 +91,50 @@ describe("GET /api/insights/provider-chain", () => {
|
||||
const body = (await res.json()) as Envelope;
|
||||
expect(body.data?.activeProvider).toBe("codex");
|
||||
expect(body.data?.configuredChain).toEqual([
|
||||
{ providerType: "codex", available: true },
|
||||
{ providerType: "admin-openai", available: true },
|
||||
{ providerType: "codex", enabled: true, available: true },
|
||||
{ providerType: "admin-openai", enabled: true, available: true },
|
||||
]);
|
||||
});
|
||||
|
||||
it("surfaces disabled entries on the wire so the user can re-enable them", async () => {
|
||||
// v1.4.16 phase D reconcile (code-review H2) — the GET response
|
||||
// used to filter through resolveProviderChain which DROPS disabled
|
||||
// entries; the UI then lost them on next refetch. The persisted
|
||||
// chain is the source of truth on the wire.
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
aiProviderChain: [
|
||||
{ providerType: "codex", priority: 1, enabled: true },
|
||||
{ providerType: "openai", priority: 2, enabled: false },
|
||||
{ providerType: "admin-openai", priority: 3, enabled: true },
|
||||
],
|
||||
} as never);
|
||||
vi.mocked(resolveProviderChain).mockResolvedValue([
|
||||
{ providerType: "codex", instance: { type: "codex" } as never },
|
||||
{
|
||||
providerType: "admin-openai",
|
||||
instance: { type: "admin-key" } as never,
|
||||
},
|
||||
]);
|
||||
vi.mocked(getLastWorkingProvider).mockReturnValue(null);
|
||||
|
||||
const res = await (GET as () => Promise<Response>)();
|
||||
const body = (await res.json()) as Envelope;
|
||||
expect(body.data?.configuredChain).toEqual([
|
||||
{ providerType: "codex", enabled: true, available: true },
|
||||
{ providerType: "openai", enabled: false, available: true },
|
||||
{ providerType: "admin-openai", enabled: true, available: true },
|
||||
]);
|
||||
// Active still reflects the resolver winner (enabled-only).
|
||||
expect(body.data?.activeProvider).toBe("codex");
|
||||
});
|
||||
|
||||
it("surfaces the cached last-working provider when available", async () => {
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
aiProviderChain: [
|
||||
{ providerType: "codex", priority: 1, enabled: true },
|
||||
{ providerType: "openai", priority: 2, enabled: true },
|
||||
],
|
||||
} as never);
|
||||
vi.mocked(resolveProviderChain).mockResolvedValue([
|
||||
{ providerType: "codex", instance: { type: "codex" } as never },
|
||||
{
|
||||
@@ -88,13 +153,23 @@ describe("GET /api/insights/provider-chain", () => {
|
||||
expect(body.data?.activeProvider).toBe("codex");
|
||||
});
|
||||
|
||||
it("returns an empty chain + null active when nothing is configured", async () => {
|
||||
it("falls back to the default chain when nothing is configured", async () => {
|
||||
// parseProviderChain(null) returns PROVIDER_CHAIN_DEFAULT — every
|
||||
// user therefore renders a populated list, not an empty one. The
|
||||
// active line still reflects the resolver, which can be empty if
|
||||
// no credentials are configured.
|
||||
vi.mocked(prisma.user.findUnique).mockResolvedValue({
|
||||
aiProviderChain: null,
|
||||
} as never);
|
||||
vi.mocked(resolveProviderChain).mockResolvedValue([]);
|
||||
vi.mocked(getLastWorkingProvider).mockReturnValue(null);
|
||||
|
||||
const res = await (GET as () => Promise<Response>)();
|
||||
const body = (await res.json()) as Envelope;
|
||||
expect(body.data?.activeProvider).toBeNull();
|
||||
expect(body.data?.configuredChain).toEqual([]);
|
||||
expect(body.data?.configuredChain.length).toBeGreaterThan(0);
|
||||
expect(body.data?.configuredChain.every((e) => e.enabled === true)).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -7,7 +7,10 @@ import { annotate } from "@/lib/logging/context";
|
||||
import { resolveProviderChain } from "@/lib/ai/provider";
|
||||
import { getLastWorkingProvider } from "@/lib/ai/provider-runner";
|
||||
import { prisma } from "@/lib/db";
|
||||
import { PROVIDER_CHAIN_TYPES } from "@/lib/ai/provider-chain";
|
||||
import {
|
||||
parseProviderChain,
|
||||
PROVIDER_CHAIN_TYPES,
|
||||
} from "@/lib/ai/provider-chain";
|
||||
|
||||
/**
|
||||
* v1.4.16 phase B5b — read-only chain summary for the Settings → AI
|
||||
@@ -34,23 +37,41 @@ import { PROVIDER_CHAIN_TYPES } from "@/lib/ai/provider-chain";
|
||||
*/
|
||||
export const GET = apiHandler(async () => {
|
||||
const { user } = await requireAuth();
|
||||
const chain = await resolveProviderChain(user.id);
|
||||
|
||||
// v1.4.16 phase D reconcile (code-review H2) — the GET response used
|
||||
// to surface the resolveProviderChain()-filtered list, which DROPS
|
||||
// disabled entries. The Settings UI seeded `enabled: true` for every
|
||||
// returned row, so disabling an entry made it disappear from the
|
||||
// list and lose its toggle position. Read the *persisted* chain
|
||||
// directly via parseProviderChain() so disabled rows still round-
|
||||
// trip and the user can re-enable them. The active-provider summary
|
||||
// still uses resolveProviderChain so the displayed "active = …"
|
||||
// matches the actual runner ordering (enabled entries only).
|
||||
const userRow = await prisma.user.findUnique({
|
||||
where: { id: user.id },
|
||||
select: { aiProviderChain: true },
|
||||
});
|
||||
const persisted = parseProviderChain(userRow?.aiProviderChain ?? null);
|
||||
|
||||
const resolved = await resolveProviderChain(user.id);
|
||||
const cached = getLastWorkingProvider(user.id);
|
||||
|
||||
annotate({
|
||||
action: { name: "insights.provider_chain.get" },
|
||||
meta: {
|
||||
chain_length: chain.length,
|
||||
chain_active: chain[0]?.providerType ?? null,
|
||||
chain_length: persisted.length,
|
||||
chain_enabled_count: persisted.filter((e) => e.enabled).length,
|
||||
chain_active: resolved[0]?.providerType ?? null,
|
||||
chain_cached_active: cached,
|
||||
},
|
||||
});
|
||||
|
||||
return apiSuccess({
|
||||
activeProvider: chain[0]?.providerType ?? null,
|
||||
activeProvider: resolved[0]?.providerType ?? null,
|
||||
cachedActiveProvider: cached,
|
||||
configuredChain: chain.map((entry) => ({
|
||||
configuredChain: persisted.map((entry) => ({
|
||||
providerType: entry.providerType,
|
||||
enabled: entry.enabled,
|
||||
available: true,
|
||||
})),
|
||||
});
|
||||
|
||||
@@ -83,7 +83,11 @@ interface ChainEntry {
|
||||
interface ProviderChainData {
|
||||
activeProvider: ProviderType | null;
|
||||
cachedActiveProvider: ProviderType | null;
|
||||
configuredChain: { providerType: ProviderType; available: boolean }[];
|
||||
configuredChain: {
|
||||
providerType: ProviderType;
|
||||
enabled: boolean;
|
||||
available: boolean;
|
||||
}[];
|
||||
}
|
||||
|
||||
const DEFAULT_CHAIN: readonly ChainEntry[] = [
|
||||
@@ -1257,7 +1261,11 @@ function FallbackChainCard({
|
||||
selected,
|
||||
onSelect,
|
||||
}: {
|
||||
chain: { providerType: ProviderType; available: boolean }[];
|
||||
chain: {
|
||||
providerType: ProviderType;
|
||||
enabled: boolean;
|
||||
available: boolean;
|
||||
}[];
|
||||
selected: ProviderType;
|
||||
onSelect: (next: ProviderType) => void;
|
||||
}) {
|
||||
@@ -1267,17 +1275,22 @@ function FallbackChainCard({
|
||||
// Local working copy. Server-confirmed values arrive via `chain`
|
||||
// prop; we keep our own state so the user can shuffle multiple rows
|
||||
// before clicking "Save chain order".
|
||||
const seededKey = chain.map((c) => c.providerType).join(",");
|
||||
// v1.4.16 phase D reconcile (code-review H2) — `enabled` from the
|
||||
// wire is now the canonical state. The GET endpoint surfaces the
|
||||
// raw persisted chain so a disabled entry survives the round-trip.
|
||||
const seededKey = chain
|
||||
.map((c) => `${c.providerType}:${c.enabled ? 1 : 0}`)
|
||||
.join(",");
|
||||
const [seeded, setSeeded] = useState<string | null>(null);
|
||||
const [entries, setEntries] = useState<ChainEntry[]>(() =>
|
||||
chain.map((c) => ({ providerType: c.providerType, enabled: true })),
|
||||
chain.map((c) => ({ providerType: c.providerType, enabled: c.enabled })),
|
||||
);
|
||||
if (seededKey !== seeded) {
|
||||
setSeeded(seededKey);
|
||||
setEntries(
|
||||
chain.map((c) => ({
|
||||
providerType: c.providerType,
|
||||
enabled: true,
|
||||
enabled: c.enabled,
|
||||
})),
|
||||
);
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user