From ea9075bde533d9712aae99431ed25a238e1f525c Mon Sep 17 00:00:00 2001 From: ajnart Date: Tue, 11 Aug 2026 21:50:40 +0200 Subject: [PATCH] Address Homarr provider review feedback --- .../_components/assistant-configuration.tsx | 165 ++++++++++-------- .../assistant-provider-quota.spec.ts | 12 +- .../assistant/assistant-provider-quota.ts | 3 + .../assistant/assistant-provider.tsx | 8 +- .../components/workshop/workshop-client.ts | 18 +- .../workshop/workshop-install-flow.spec.ts | 8 +- apps/workshop/homarr_provider.go | 33 +++- apps/workshop/homarr_provider_test.go | 22 +++ apps/workshop/test.sh | 18 ++ 9 files changed, 199 insertions(+), 88 deletions(-) diff --git a/apps/nextjs/src/app/[locale]/manage/assistant/_components/assistant-configuration.tsx b/apps/nextjs/src/app/[locale]/manage/assistant/_components/assistant-configuration.tsx index e675d57ec..40f885255 100644 --- a/apps/nextjs/src/app/[locale]/manage/assistant/_components/assistant-configuration.tsx +++ b/apps/nextjs/src/app/[locale]/manage/assistant/_components/assistant-configuration.tsx @@ -52,6 +52,8 @@ import type { AssistantProvider, AssistantProviderCategory } from "@homarr/defin import { showErrorNotification, showSuccessNotification } from "@homarr/notifications"; import { useScopedI18n } from "@homarr/translation/client"; +import { getWorkshopAssistantProviderUrl } from "~/components/workshop/workshop-client"; + import classes from "./assistant-configuration.module.css"; import { formatAssistantContextWindow, getAssistantConnectionState } from "./assistant-configuration-state"; @@ -153,8 +155,13 @@ export const AssistantConfiguration = () => { const [webSearchEnabled, setWebSearchEnabled] = useState(false); useEffect(() => { - setProvider(configuration?.provider ?? "openrouter"); - setBaseUrl(configuration?.baseUrl ?? assistantProviderPresets.openrouter.baseUrl); + const configuredProvider = configuration?.provider ?? "openrouter"; + setProvider(configuredProvider); + setBaseUrl( + configuredProvider === "homarr" + ? getWorkshopAssistantProviderUrl() + : (configuration?.baseUrl ?? assistantProviderPresets.openrouter.baseUrl), + ); setModelDiscoveryPath(configuration?.modelDiscoveryPath ?? ""); setModelId(configuration?.modelId ?? ""); setEnabled(configuration?.enabled ?? false); @@ -291,7 +298,7 @@ export const AssistantConfiguration = () => { const nextPreset = assistantProviderPresets[nextProvider]; clearDraftConnectionState(); setProvider(nextProvider); - setBaseUrl(nextPreset.baseUrl); + setBaseUrl(nextProvider === "homarr" ? getWorkshopAssistantProviderUrl() : nextPreset.baseUrl); setModelDiscoveryPath(nextPreset.modelDiscoveryPath ?? ""); }; @@ -321,12 +328,11 @@ export const AssistantConfiguration = () => { }; const saveConnection = (clearApiKey = false) => { - const customHeaders = - isHomarrProvider - ? undefined - : headers.length > 0 - ? Object.fromEntries(headers.map((header) => [header.name.trim(), header.value])) - : undefined; + const customHeaders = isHomarrProvider + ? undefined + : headers.length > 0 + ? Object.fromEntries(headers.map((header) => [header.name.trim(), header.value])) + : undefined; updateConnection.mutate({ provider, baseUrl, @@ -542,76 +548,81 @@ export const AssistantConfiguration = () => { {!isHomarrProvider && ( - }> - - - - {t("headers.title")} - - - {t("headers.description")} - - - {configuration?.customHeadersConfigured && ( - - {clearHeaders ? t("headers.pendingRemoval") : t("headers.savedBadge")} - - )} - - - - - - - {configuration?.customHeadersConfigured ? t("headers.configured") : t("headers.empty")} - - + }> + + + + {t("headers.title")} + + + {t("headers.description")} + + + {configuration?.customHeadersConfigured && ( + + {clearHeaders ? t("headers.pendingRemoval") : t("headers.savedBadge")} + + )} - {headers.map((header) => ( - - updateHeader(header.id, "name", event.currentTarget.value)} - placeholder="X-Provider-Header" - /> - updateHeader(header.id, "value", event.currentTarget.value)} - placeholder={t("headers.valuePlaceholder")} - autoComplete="off" - /> - - setHeaders((current) => current.filter((item) => item.id !== header.id))} - > - - - + + + + + + {configuration?.customHeadersConfigured ? t("headers.configured") : t("headers.empty")} + + - ))} - {configuration?.customHeadersConfigured && headers.length === 0 && ( - - )} - - + {headers.map((header) => ( + + updateHeader(header.id, "name", event.currentTarget.value)} + placeholder="X-Provider-Header" + /> + updateHeader(header.id, "value", event.currentTarget.value)} + placeholder={t("headers.valuePlaceholder")} + autoComplete="off" + /> + + setHeaders((current) => current.filter((item) => item.id !== header.id))} + > + + + + + ))} + {configuration?.customHeadersConfigured && headers.length === 0 && ( + + )} + + )} diff --git a/apps/nextjs/src/components/assistant/assistant-provider-quota.spec.ts b/apps/nextjs/src/components/assistant/assistant-provider-quota.spec.ts index 1a742de4a..765683cf4 100644 --- a/apps/nextjs/src/components/assistant/assistant-provider-quota.spec.ts +++ b/apps/nextjs/src/components/assistant/assistant-provider-quota.spec.ts @@ -1,6 +1,10 @@ import { describe, expect, test } from "vitest"; -import { getAssistantProviderQuotaLevel, isAssistantProviderUnavailable } from "./assistant-provider-quota"; +import { + getAssistantProviderQuotaLevel, + getAssistantProviderQuotaRefreshDelay, + isAssistantProviderUnavailable, +} from "./assistant-provider-quota"; describe("getAssistantProviderQuotaLevel", () => { test.each([ @@ -19,4 +23,10 @@ describe("getAssistantProviderQuotaLevel", () => { expect(isAssistantProviderUnavailable({ provider: "homarr", signedIn: true, remaining: undefined })).toBe(false); expect(isAssistantProviderUnavailable({ provider: "openrouter", signedIn: false, remaining: 0 })).toBe(false); }); + + test("refreshes just after the UTC reset, or immediately when it has passed", () => { + const reset = "2026-08-12T00:00:00.000Z"; + expect(getAssistantProviderQuotaRefreshDelay(reset, Date.parse("2026-08-11T23:59:00.000Z"))).toBe(61_000); + expect(getAssistantProviderQuotaRefreshDelay(reset, Date.parse("2026-08-12T00:00:01.000Z"))).toBe(1_000); + }); }); diff --git a/apps/nextjs/src/components/assistant/assistant-provider-quota.ts b/apps/nextjs/src/components/assistant/assistant-provider-quota.ts index 9ac44fa61..cf8034bd0 100644 --- a/apps/nextjs/src/components/assistant/assistant-provider-quota.ts +++ b/apps/nextjs/src/components/assistant/assistant-provider-quota.ts @@ -3,6 +3,9 @@ import type { AssistantProvider } from "@homarr/definitions"; export type AssistantProviderQuotaLevel = "ok" | "warning" | "bad" | "dead"; +export const getAssistantProviderQuotaRefreshDelay = (resetsAt: string, now = Date.now()) => + Math.max(0, Date.parse(resetsAt) - now) + 1_000; + export const getAssistantProviderQuotaLevel = ( usage: Pick, ): AssistantProviderQuotaLevel => { diff --git a/apps/nextjs/src/components/assistant/assistant-provider.tsx b/apps/nextjs/src/components/assistant/assistant-provider.tsx index 8398e280f..feb9aaebc 100644 --- a/apps/nextjs/src/components/assistant/assistant-provider.tsx +++ b/apps/nextjs/src/components/assistant/assistant-provider.tsx @@ -29,7 +29,7 @@ import { createWorkshopClient } from "~/components/workshop/workshop-client"; import { AssistantContext, AssistantPreferencesContext, useAssistantPreferences } from "./assistant-context"; import { shouldAutomaticallyContinueAssistant } from "./assistant-auto-submit"; -import { isAssistantProviderUnavailable } from "./assistant-provider-quota"; +import { getAssistantProviderQuotaRefreshDelay, isAssistantProviderUnavailable } from "./assistant-provider-quota"; import { AssistantAutoApprovalProvider } from "./assistant-auto-approval"; import { getRunningAssistantPartType } from "./assistant-activity-state"; import { prepareAssistantRequestBody } from "./assistant-attachment-payload"; @@ -237,6 +237,12 @@ const AssistantPreferencesProvider = ({ children }: PropsWithChildren) => { void refreshQuota(); }, [data?.provider, providerUser, refreshQuota]); + useEffect(() => { + if (data?.provider !== "homarr" || !providerUser || !quota) return; + const timeout = window.setTimeout(() => void refreshQuota(), getAssistantProviderQuotaRefreshDelay(quota.resetsAt)); + return () => window.clearTimeout(timeout); + }, [data?.provider, providerUser, quota, refreshQuota]); + const value = useMemo( () => ({ provider: data?.provider ?? null, diff --git a/apps/nextjs/src/components/workshop/workshop-client.ts b/apps/nextjs/src/components/workshop/workshop-client.ts index 95618f512..529174807 100644 --- a/apps/nextjs/src/components/workshop/workshop-client.ts +++ b/apps/nextjs/src/components/workshop/workshop-client.ts @@ -3,13 +3,23 @@ import { WORKSHOP_API_URL, WORKSHOP_WEB_URL } from "@homarr/workshop/schema"; let browserClient: WorkshopBackend | undefined; +export function getWorkshopApiUrl() { + const runtimeUrl = + typeof document === "undefined" + ? undefined + : document.querySelector('meta[name="homarr-workshop-api-url"]')?.content; + return (runtimeUrl || WORKSHOP_API_URL).replace(/\/+$/u, ""); +} + +export function getWorkshopAssistantProviderUrl() { + return `${getWorkshopApiUrl()}/api/ai/v1`; +} + export function createWorkshopClient() { - if (typeof document === "undefined") return new WorkshopBackend(WORKSHOP_API_URL); + if (typeof document === "undefined") return new WorkshopBackend(getWorkshopApiUrl()); if (browserClient) return browserClient; - const runtimeUrl = document.querySelector('meta[name="homarr-workshop-api-url"]')?.content; - - browserClient = new WorkshopBackend(runtimeUrl || WORKSHOP_API_URL); + browserClient = new WorkshopBackend(getWorkshopApiUrl()); return browserClient; } diff --git a/apps/nextjs/src/components/workshop/workshop-install-flow.spec.ts b/apps/nextjs/src/components/workshop/workshop-install-flow.spec.ts index 2d24f49c6..b9fab824b 100644 --- a/apps/nextjs/src/components/workshop/workshop-install-flow.spec.ts +++ b/apps/nextjs/src/components/workshop/workshop-install-flow.spec.ts @@ -3,7 +3,7 @@ import { readFileSync } from "node:fs"; import { afterEach, describe, expect, it } from "vitest"; -import { getWorkshopWebUrl } from "./workshop-client"; +import { getWorkshopAssistantProviderUrl, getWorkshopWebUrl } from "./workshop-client"; describe("Workshop widget install flow", () => { afterEach(() => { @@ -17,6 +17,12 @@ describe("Workshop widget install flow", () => { expect(getWorkshopWebUrl("widget/id")).toBe("https://preview.example/workshop/widget%2Fid"); }); + it("derives the Homarr provider endpoint from the runtime Workshop API", () => { + document.head.innerHTML = ''; + + expect(getWorkshopAssistantProviderUrl()).toBe("https://api.preview.example/api/ai/v1"); + }); + it("uses one registered modal stack for browsing, details, reports, and confirmation", () => { const installButton = readFileSync( `${process.cwd()}/apps/nextjs/src/components/workshop/workshop-install-button.tsx`, diff --git a/apps/workshop/homarr_provider.go b/apps/workshop/homarr_provider.go index 652471ce8..c062b8969 100644 --- a/apps/workshop/homarr_provider.go +++ b/apps/workshop/homarr_provider.go @@ -25,11 +25,14 @@ const ( defaultDailyLimit = 50 maxProviderRequestUnits = 1000 maxChatRequestBytes = 8 << 20 + maxChatResponseBytes = 8 << 20 maxUpstreamErrorBytes = 1 << 20 openRouterDefaultURL = "https://openrouter.ai/api/v1" providerRequestTimeout = 5 * time.Minute ) +var errProviderResponseTooLarge = errors.New("provider response is too large") + type homarrProvider struct { apiKey string baseURL string @@ -119,6 +122,9 @@ func newHomarrProviderFromEnv() (*homarrProvider, error) { } func (provider *homarrProvider) models(event *core.RequestEvent) error { + if provider.apiKey == "" { + return event.JSON(http.StatusServiceUnavailable, openAIError("The Homarr provider is not configured.")) + } return event.JSON(http.StatusOK, map[string]any{ "object": "list", "data": []map[string]any{{ @@ -245,15 +251,27 @@ func (provider *homarrProvider) chat(event *core.RequestEvent) error { return nil } - responseBody, readErr := io.ReadAll(upstreamResponse.Body) - usage := usageFromJSON(responseBody) - provider.finishActivity(event.App, privateRequest, publicActivity, statusForError(readErr), usage, startedAt) + responseBody, readErr := readBoundedBody(upstreamResponse.Body, maxChatResponseBytes) if readErr != nil { - return readErr + provider.finishActivity(event.App, privateRequest, publicActivity, "failed", requestUsage{}, startedAt) + return event.JSON(http.StatusBadGateway, openAIError("The model endpoint returned an invalid response.")) } + usage := usageFromJSON(responseBody) + provider.finishActivity(event.App, privateRequest, publicActivity, "completed", usage, startedAt) return event.Blob(upstreamResponse.StatusCode, contentType, responseBody) } +func readBoundedBody(reader io.Reader, limit int64) ([]byte, error) { + body, err := io.ReadAll(io.LimitReader(reader, limit+1)) + if err != nil { + return nil, err + } + if int64(len(body)) > limit { + return nil, errProviderResponseTooLarge + } + return body, nil +} + func countRequestUnits(messages []providerMessage) int { units := 1 for index := len(messages) - 1; index >= 0 && messages[index].Role == "tool"; index-- { @@ -386,6 +404,9 @@ func (provider *homarrProvider) finishActivity( if err != nil { return err } + if !requestBelongsToQuotaDay(quota.GetString("day"), startedAt) { + return nil + } quota.Set("inputTokens", quota.GetInt("inputTokens")+usage.InputTokens) quota.Set("outputTokens", quota.GetInt("outputTokens")+usage.OutputTokens) quota.Set("totalTokens", quota.GetInt("totalTokens")+usage.TotalTokens) @@ -393,6 +414,10 @@ func (provider *homarrProvider) finishActivity( }) } +func requestBelongsToQuotaDay(day string, startedAt time.Time) bool { + return day == startedAt.UTC().Format(time.DateOnly) +} + func statusForError(err error) string { if err != nil { return "failed" diff --git a/apps/workshop/homarr_provider_test.go b/apps/workshop/homarr_provider_test.go index 8c3b9cd07..cfc74653a 100644 --- a/apps/workshop/homarr_provider_test.go +++ b/apps/workshop/homarr_provider_test.go @@ -2,7 +2,9 @@ package main import ( "math" + "strings" "testing" + "time" ) func TestCountRequestUnits(t *testing.T) { @@ -84,3 +86,23 @@ func TestProviderEnvironment(t *testing.T) { t.Fatal("expected invalid daily limit to fail") } } + +func TestReadBoundedBody(t *testing.T) { + body, err := readBoundedBody(strings.NewReader("1234"), 4) + if err != nil || string(body) != "1234" { + t.Fatalf("unexpected bounded body: %q, %v", body, err) + } + if _, err := readBoundedBody(strings.NewReader("12345"), 4); err != errProviderResponseTooLarge { + t.Fatalf("expected response size error, got %v", err) + } +} + +func TestRequestBelongsToQuotaDay(t *testing.T) { + startedAt := time.Date(2026, 8, 12, 1, 30, 0, 0, time.FixedZone("CEST", 2*60*60)) + if !requestBelongsToQuotaDay("2026-08-11", startedAt) { + t.Fatal("expected request to match its UTC start day") + } + if requestBelongsToQuotaDay("2026-08-12", startedAt) { + t.Fatal("request usage must not be added after the quota record resets") + } +} diff --git a/apps/workshop/test.sh b/apps/workshop/test.sh index cb4286e48..067cb8eed 100644 --- a/apps/workshop/test.sh +++ b/apps/workshop/test.sh @@ -62,6 +62,24 @@ for client_id in workshop-test-client workshop-rotated-client; do node apps/workshop/tests/oauth-sync.integration.mjs done +OPENROUTER_API_KEY= PB_EXPOSE_PORT="$WORKSHOP_TEST_PORT" docker compose -p "$WORKSHOP_TEST_PROJECT" \ + -f apps/workshop/docker-compose.yml up -d --force-recreate workshop +for attempt in $(seq 1 60); do + if curl --fail --silent "http://127.0.0.1:$WORKSHOP_TEST_PORT/api/health" >/dev/null; then + break + fi + if [ "$attempt" -eq 60 ]; then + exit 1 + fi + sleep 1 +done +models_status=$(curl --silent --output /dev/null --write-out '%{http_code}' \ + "http://127.0.0.1:$WORKSHOP_TEST_PORT/api/ai/v1/models") +if [ "$models_status" != "503" ]; then + echo "Expected an unconfigured Homarr provider to return 503, received $models_status" + exit 1 +fi + PB_EXPOSE_PORT="$WORKSHOP_TEST_PORT" docker compose -p "$WORKSHOP_TEST_PROJECT" \ -f apps/workshop/docker-compose.yml stop workshop printf 'y\n' | PB_EXPOSE_PORT="$WORKSHOP_TEST_PORT" docker compose -p "$WORKSHOP_TEST_PROJECT" \