feat(ai): AI gateway integration with custom provider support #1072
Open
ddecrulle wants to merge 17 commits into
Open
feat(ai): AI gateway integration with custom provider support #1072ddecrulle wants to merge 17 commits into
ddecrulle wants to merge 17 commits into
Conversation
|
Contributor
📝 WalkthroughWalkthroughAdds AI gateway support end to end: environment gating, core contracts and adapters, persisted AI config and usecase logic, bootstrap and launcher wiring, an Account AI tab, and localized UI text. ChangesAI Gateway Integration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
web/src/core/ports/Ai.ts (1)
8-11: ⚡ Quick win
errorvariant carries no diagnostic payload — error context is lostThe
{ status: "error" }branch gives thunks and UI no information about what went wrong (network failure, unexpected HTTP status, etc.), making it hard to display a meaningful error message. Even a minimalreason?: stringorhttpStatus?: numberwould allow the UI to differentiate transient failures from configuration errors.♻️ Proposed extension
export type GetTokenResult = | { status: "success"; token: string } | { status: "no-account" } - | { status: "error" }; + | { status: "error"; reason?: string };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/Ai.ts` around lines 8 - 11, The GetTokenResult union currently has an { status: "error" } variant with no diagnostic data; update the type so the "error" branch carries minimal payload (e.g., reason?: string and httpStatus?: number or similar fields) so callers can surface meaningful messages; change the union definition for GetTokenResult to include those optional fields and then update all places that construct or pattern-match on GetTokenResult (e.g., any code creating a { status: "error" } result or switching on GetTokenResult.status) to populate and consume the new fields.web/src/core/ports/OnyxiaApi/XOnyxia.ts (1)
196-203: 💤 Low value
enabled: truediscriminant is redundant given| undefinedSince
aiis typed as{ enabled: true; ... } | undefined, theenabled: trueliteral adds no information — an AI block being present already implies it is enabled. Chart templates that checkai.enabledcould equivalently checkai != null. Consider dropping theenabledfield to keep the shape consistent with howvaultands3are modelled in the same type.♻️ Proposed simplification
ai: | { - enabled: true; token: string; apiBase: string; model: string; } | undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/OnyxiaApi/XOnyxia.ts` around lines 196 - 203, Remove the redundant enabled: true literal from the ai union type in XOnyxia (i.e., change the ai type from "{ enabled: true; token: string; apiBase: string; model: string; } | undefined" to "{ token: string; apiBase: string; model: string; } | undefined") and update any runtime checks that test ai.enabled to instead test for ai != null (or Boolean(ai)), ensuring places referencing the removed enabled property (e.g., code paths that read ai.enabled) are adjusted to treat presence/absence of the ai object as the enablement signal.web/src/core/adapters/ai/openWebUi.ts (1)
40-43: ⚡ Quick winUnguarded cast on
data.datamay throw aTypeErrorat runtime.If the gateway returns a response where
data.dataisundefined,null, or not an array,.map()will throw, which callers don't expect —listModelsshould either validate or provide a fallback.🛡️ Proposed defensive guard
const data = await response.json(); -return (data.data as { id: string }[]).map(m => m.id); +const entries: unknown[] = Array.isArray(data.data) ? data.data : []; +return (entries as { id: string }[]).map(m => m.id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/adapters/ai/openWebUi.ts` around lines 40 - 43, The unguarded cast in listModels reads response.json() into data and assumes data.data is an array, which can throw when it's undefined/null/not-an-array; update the listModels implementation to validate that data && Array.isArray(data.data) before mapping, and if not return an empty array (or a safe fallback). When mapping, defensively extract ids (e.g., filter items with an id property and coerce to string) so the method always returns string[] without throwing.web/src/core/usecases/ai/thunks.ts (1)
145-150: 🏗️ Heavy lift
testCustomProviderreturns aPromise<string[]>which violates the reactive thunk pattern.Per the coding guidelines for
web/src/core/**/*.ts: "Use observable state with thunks (reactive pattern) rather than promise-based returns; dispatch actions and read state instead of returning values from thunks."The test result (model list or error) should be stored in state via new actions (e.g.,
providerTestStarted,providerTestSucceeded({ models }),providerTestFailed) and exposed via a selector, with the UI subscribing to state instead of awaiting the thunk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/usecases/ai/thunks.ts` around lines 145 - 150, testCustomProvider currently returns a Promise<string[]> which breaks the project's reactive thunk pattern; change it to dispatch actions and update state instead of returning values. Replace the direct return of fetchModels(apiBase, apiKey) in testCustomProvider with dispatch(providerTestStarted()), call fetchModels, then on success dispatch(providerTestSucceeded({ models })) and on error dispatch(providerTestFailed({ error })); update the reducer/state to store test status and models and expose them via a selector for the UI to subscribe to, and make the thunk return void (or Promise<void>) rather than Promise<string[]>; reference the existing testCustomProvider thunk and fetchModels helper when applying these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/core/usecases/ai/selectors.ts`:
- Around line 16-26: Replace the hard assertion in the "no-account" branch so
the selector won't throw when region.ai is missing: remove the assert on
region.ai in the branch where initializationStatus === "no-account" and return
isEnabled: false and initializationStatus as before but only attach webUiUrl
when region.ai exists (e.g., use a safe optional access or conditional property)
so the selector returns a safe fallback instead of throwing; look for symbols
initializationStatus, region.ai and the returned webUiUrl in this function to
update.
In `@web/src/core/usecases/ai/thunks.ts`:
- Around line 190-193: The call to ai.listModels(token) can throw and leaves the
initializeStart/“pending” state unresolved; wrap the ai.listModels(token)
invocation in a try/catch around the block that follows initializeStart and, on
any error, dispatch initializeFailed with the error (or a normalized message) so
the thunk resolves correctly; reference tokenResult, ai.listModels(token),
initializeStart, and initializeFailed when locating where to add the try/catch
and the error dispatch.
In `@web/src/core/usecases/launcher/thunks.ts`:
- Around line 777-790: The current ai block builds an ai object even when
aiState.selectedModel is undefined (using selectedModel ?? ""), which injects an
empty model into the XOnyxiaContext; update the anonymous IIFE in the ai
property (the code using aiUsecase.selectors.main(getState()) and aiState) to
return undefined unless aiState.isEnabled, aiState.token are present AND
aiState.selectedModel is a non-empty value (e.g., check aiState.selectedModel !=
null && aiState.selectedModel !== ""), otherwise return undefined so no ai key
is written into the context.
In `@web/src/ui/i18n/resources/no.tsx`:
- Around line 126-127: Update the localized helper string for the translation
key "custom providers section helper" to include the user-facing disclosure that
credentials (API keys) are stored locally in the browser; locate the string for
"custom providers section helper" in web/src/ui/i18n/resources/no.tsx and modify
its value to append a short sentence making the storage disclosure (e.g.,
"API-nøkler lagres i nettleseren"), keeping the rest of the guidance about base
URL and API key unchanged.
In `@web/src/ui/pages/account/AccountAiTab.tsx`:
- Around line 100-112: Wrap the ai.addCustomProvider call inside onSaveProvider
in a try/catch so failures are handled: call ai.addCustomProvider(...) in the
try block and only clear the form (setAddFormOpen(false), setPendingLabel(""),
setPendingApiBase(""), setPendingApiKey(""), setTestStatus("idle"),
setTestModelCount(0)) on success; in catch set a failure state (e.g.,
setTestStatus("error") and optionally set an error message state) and keep the
add form open so the user can correct inputs; ensure the catch re-enables any UI
affordances disabled during save and does not leave the promise rejection
unhandled.
In `@web/src/ui/pages/account/Page.tsx`:
- Line 56: The ternary in the .filter callback for accountTabId is redundant;
replace the ternary that returns true when accountTabId !== "ai" else
ai.isAvailable() with a logical OR of the inequality check and ai.isAvailable()
(i.e., test whether accountTabId is not "ai" OR ai.isAvailable()) in the Page
component where accountTabId and ai.isAvailable() are used, and apply the same
simplification to the sibling .filter calls that use the same pattern.
---
Nitpick comments:
In `@web/src/core/adapters/ai/openWebUi.ts`:
- Around line 40-43: The unguarded cast in listModels reads response.json() into
data and assumes data.data is an array, which can throw when it's
undefined/null/not-an-array; update the listModels implementation to validate
that data && Array.isArray(data.data) before mapping, and if not return an empty
array (or a safe fallback). When mapping, defensively extract ids (e.g., filter
items with an id property and coerce to string) so the method always returns
string[] without throwing.
In `@web/src/core/ports/Ai.ts`:
- Around line 8-11: The GetTokenResult union currently has an { status: "error"
} variant with no diagnostic data; update the type so the "error" branch carries
minimal payload (e.g., reason?: string and httpStatus?: number or similar
fields) so callers can surface meaningful messages; change the union definition
for GetTokenResult to include those optional fields and then update all places
that construct or pattern-match on GetTokenResult (e.g., any code creating a {
status: "error" } result or switching on GetTokenResult.status) to populate and
consume the new fields.
In `@web/src/core/ports/OnyxiaApi/XOnyxia.ts`:
- Around line 196-203: Remove the redundant enabled: true literal from the ai
union type in XOnyxia (i.e., change the ai type from "{ enabled: true; token:
string; apiBase: string; model: string; } | undefined" to "{ token: string;
apiBase: string; model: string; } | undefined") and update any runtime checks
that test ai.enabled to instead test for ai != null (or Boolean(ai)), ensuring
places referencing the removed enabled property (e.g., code paths that read
ai.enabled) are adjusted to treat presence/absence of the ai object as the
enablement signal.
In `@web/src/core/usecases/ai/thunks.ts`:
- Around line 145-150: testCustomProvider currently returns a Promise<string[]>
which breaks the project's reactive thunk pattern; change it to dispatch actions
and update state instead of returning values. Replace the direct return of
fetchModels(apiBase, apiKey) in testCustomProvider with
dispatch(providerTestStarted()), call fetchModels, then on success
dispatch(providerTestSucceeded({ models })) and on error
dispatch(providerTestFailed({ error })); update the reducer/state to store test
status and models and expose them via a selector for the UI to subscribe to, and
make the thunk return void (or Promise<void>) rather than Promise<string[]>;
reference the existing testCustomProvider thunk and fetchModels helper when
applying these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cb9147a-b5ad-46ea-be5c-b123ddd634a2
📒 Files selected for processing (30)
web/CLAUDE.mdweb/src/core/adapters/ai/index.tsweb/src/core/adapters/ai/mock.tsweb/src/core/adapters/ai/openWebUi.tsweb/src/core/adapters/onyxiaApi/ApiTypes.tsweb/src/core/adapters/onyxiaApi/onyxiaApi.tsweb/src/core/bootstrap.tsweb/src/core/ports/Ai.tsweb/src/core/ports/OnyxiaApi/DeploymentRegion.tsweb/src/core/ports/OnyxiaApi/XOnyxia.tsweb/src/core/tools/oidcTokenExchange.tsweb/src/core/usecases/ai/index.tsweb/src/core/usecases/ai/selectors.tsweb/src/core/usecases/ai/state.tsweb/src/core/usecases/ai/thunks.tsweb/src/core/usecases/index.tsweb/src/core/usecases/launcher/thunks.tsweb/src/ui/i18n/resources/de.tsxweb/src/ui/i18n/resources/en.tsxweb/src/ui/i18n/resources/es.tsxweb/src/ui/i18n/resources/fi.tsxweb/src/ui/i18n/resources/fr.tsxweb/src/ui/i18n/resources/it.tsxweb/src/ui/i18n/resources/nl.tsxweb/src/ui/i18n/resources/no.tsxweb/src/ui/i18n/resources/zh-CN.tsxweb/src/ui/i18n/types.tsweb/src/ui/pages/account/AccountAiTab.tsxweb/src/ui/pages/account/Page.tsxweb/src/ui/pages/account/accountTabIds.ts
Comment on lines
+100
to
+112
| const onSaveProvider = useConstCallback(async () => { | ||
| await ai.addCustomProvider({ | ||
| label: pendingLabel, | ||
| apiBase: pendingApiBase, | ||
| apiKey: pendingApiKey | ||
| }); | ||
| setAddFormOpen(false); | ||
| setPendingLabel(""); | ||
| setPendingApiBase(""); | ||
| setPendingApiKey(""); | ||
| setTestStatus("idle"); | ||
| setTestModelCount(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
Handle add-provider failures in onSaveProvider.
If ai.addCustomProvider fails at Line 101, the rejection is unhandled and the UI gives no clear recovery path. Catch the error and keep the form in a controlled error state.
Proposed fix
const onSaveProvider = useConstCallback(async () => {
- await ai.addCustomProvider({
- label: pendingLabel,
- apiBase: pendingApiBase,
- apiKey: pendingApiKey
- });
+ try {
+ await ai.addCustomProvider({
+ label: pendingLabel,
+ apiBase: pendingApiBase,
+ apiKey: pendingApiKey
+ });
+ } catch {
+ setTestStatus("error");
+ return;
+ }
setAddFormOpen(false);
setPendingLabel("");
setPendingApiBase("");
setPendingApiKey("");
setTestStatus("idle");
setTestModelCount(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const onSaveProvider = useConstCallback(async () => { | |
| await ai.addCustomProvider({ | |
| label: pendingLabel, | |
| apiBase: pendingApiBase, | |
| apiKey: pendingApiKey | |
| }); | |
| setAddFormOpen(false); | |
| setPendingLabel(""); | |
| setPendingApiBase(""); | |
| setPendingApiKey(""); | |
| setTestStatus("idle"); | |
| setTestModelCount(0); | |
| }); | |
| const onSaveProvider = useConstCallback(async () => { | |
| try { | |
| await ai.addCustomProvider({ | |
| label: pendingLabel, | |
| apiBase: pendingApiBase, | |
| apiKey: pendingApiKey | |
| }); | |
| } catch { | |
| setTestStatus("error"); | |
| return; | |
| } | |
| setAddFormOpen(false); | |
| setPendingLabel(""); | |
| setPendingApiBase(""); | |
| setPendingApiKey(""); | |
| setTestStatus("idle"); | |
| setTestModelCount(0); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/pages/account/AccountAiTab.tsx` around lines 100 - 112, Wrap the
ai.addCustomProvider call inside onSaveProvider in a try/catch so failures are
handled: call ai.addCustomProvider(...) in the try block and only clear the form
(setAddFormOpen(false), setPendingLabel(""), setPendingApiBase(""),
setPendingApiKey(""), setTestStatus("idle"), setTestModelCount(0)) on success;
in catch set a failure state (e.g., setTestStatus("error") and optionally set an
error message state) and keep the add form open so the user can correct inputs;
ensure the catch re-enables any UI affordances disabled during save and does not
leave the promise rejection unhandled.
| .filter(accountTabId => | ||
| accountTabId !== "vault" ? true : vaultCredentials.isAvailable() | ||
| ) | ||
| .filter(accountTabId => (accountTabId !== "ai" ? true : ai.isAvailable())) |
Contributor
There was a problem hiding this comment.
Simplify the negated ternary condition (SonarCloud warning).
accountTabId !== "ai" ? true : ai.isAvailable() reads as a double-negation and SonarCloud flags it. The direct form is clearer:
♻️ Proposed simplification
-.filter(accountTabId => (accountTabId !== "ai" ? true : ai.isAvailable()))
+.filter(accountTabId => accountTabId !== "ai" || ai.isAvailable())Note: the same pattern appears in the sibling .filter calls on lines 46, 49–52, and 53–55 — those could be cleaned up consistently in a follow-up.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .filter(accountTabId => (accountTabId !== "ai" ? true : ai.isAvailable())) | |
| .filter(accountTabId => accountTabId !== "ai" || ai.isAvailable()) |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 56-56: Unexpected negated condition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/pages/account/Page.tsx` at line 56, The ternary in the .filter
callback for accountTabId is redundant; replace the ternary that returns true
when accountTabId !== "ai" else ai.isAvailable() with a logical OR of the
inequality check and ai.isAvailable() (i.e., test whether accountTabId is not
"ai" OR ai.isAvailable()) in the Page component where accountTabId and
ai.isAvailable() are used, and apply the same simplification to the sibling
.filter calls that use the same pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/src/core/ports/OnyxiaApi/XOnyxia.ts (1)
185-194: ⚡ Quick winPrefer optional property syntax for
aicontext.
ai: {...} | undefinedforces callers to always include the key. If this field is truly optional,ai?: {...}is the safer contract for existingXOnyxiaContextobject construction paths.Suggested change
- ai: - | { + ai?: + { enabled: boolean; apiKey: string; apiBase: string; model: string; provider: string; embeddingsModel: string; } - | undefined; + ;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/OnyxiaApi/XOnyxia.ts` around lines 185 - 194, The ai property in the XOnyxiaContext type uses union syntax with undefined (ai: {...} | undefined) which requires callers to always provide the key. Replace this with the optional property syntax (ai?: {...}) to allow the property to be omitted entirely when constructing XOnyxiaContext objects, making it a safer contract for existing code paths that build this type.web/src/core/ports/Ai.ts (1)
1-8: ⚡ Quick winUse an interface for the port contract in
src/core/ports.This contract should be declared as an
interfaceto match the ports-and-adapters rule forweb/src/core/ports/**/*.ts.Suggested change
-export type Ai = { +export interface Ai { id: string; name: string; webUiUrl: string; apiBase: string; getToken: () => Promise<GetTokenResult>; listModels: (token: string) => Promise<{ id: string; name: string }[]>; -}; +}As per coding guidelines,
web/src/core/ports/**/*.ts: “Define TypeScript interfaces for external dependencies in src/core/ports/ using the ports-and-adapters pattern.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/Ai.ts` around lines 1 - 8, The Ai export in the file should be declared as an interface instead of a type alias to follow the ports-and-adapters coding guidelines. Replace the existing type Ai declaration (which uses the type keyword with an object literal syntax) with an interface declaration that has the same name and properties, removing the equals sign and curly brace syntax used for type aliases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/core/adapters/ai/mock.ts`:
- Line 10: The apiBase string composition concatenates webUiUrl directly with
`/api`, which creates a double slash (`...//api`) if webUiUrl ends with a
forward slash, potentially breaking strict gateways or proxies. Normalize the
webUiUrl by removing any trailing slash before composing the apiBase string. Use
a method to strip the trailing slash from webUiUrl (such as using replace or
trimEnd with proper regex/string manipulation) so that the final apiBase always
has exactly one slash between the base URL and the `/api` path.
In `@web/src/core/usecases/ai/thunks.ts`:
- Around line 31-42: The Zod schema in the fetchModels function requires a name
field that does not exist in the actual OpenAI API response, causing parse
failures. Modify the schema object definition to either make the name field
optional by adding .optional() to the z.string() for name, or use
.default(model.id) to derive the name from the id field as a fallback. If using
the optional approach, update the mapping logic to handle cases where name may
be undefined. If using the default approach, ensure the default value is applied
appropriately in the schema definition.
In `@web/src/core/usecases/launcher/thunks.ts`:
- Line 772: The activeProvider selector can return a valid AI context object
with model set to an empty string when no model is selected, which causes issues
with AI-enabled Helm charts. At the location where
aiUsecase.selectors.activeProvider is called in the launcher thunks, either add
validation logic in the launcher UI to prevent service launch when modelId is
undefined (enforcing model selection before launch), or modify the
activeProvider selector itself to return undefined instead of a valid context
object when provider.selection.modelId is not set, ensuring downstream code
receives undefined rather than a context with an empty model string.
---
Nitpick comments:
In `@web/src/core/ports/Ai.ts`:
- Around line 1-8: The Ai export in the file should be declared as an interface
instead of a type alias to follow the ports-and-adapters coding guidelines.
Replace the existing type Ai declaration (which uses the type keyword with an
object literal syntax) with an interface declaration that has the same name and
properties, removing the equals sign and curly brace syntax used for type
aliases.
In `@web/src/core/ports/OnyxiaApi/XOnyxia.ts`:
- Around line 185-194: The ai property in the XOnyxiaContext type uses union
syntax with undefined (ai: {...} | undefined) which requires callers to always
provide the key. Replace this with the optional property syntax (ai?: {...}) to
allow the property to be omitted entirely when constructing XOnyxiaContext
objects, making it a safer contract for existing code paths that build this
type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 374834a3-0605-4579-b5f9-554819d05232
📒 Files selected for processing (32)
web/CLAUDE.mdweb/src/core/adapters/ai/index.tsweb/src/core/adapters/ai/mock.tsweb/src/core/adapters/ai/openWebUi.tsweb/src/core/adapters/onyxiaApi/ApiTypes.tsweb/src/core/adapters/onyxiaApi/onyxiaApi.tsweb/src/core/bootstrap.tsweb/src/core/ports/Ai.tsweb/src/core/ports/OnyxiaApi/DeploymentRegion.tsweb/src/core/ports/OnyxiaApi/XOnyxia.tsweb/src/core/tools/oidcTokenExchange.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.tsweb/src/core/usecases/ai/index.tsweb/src/core/usecases/ai/selectors.tsweb/src/core/usecases/ai/state.tsweb/src/core/usecases/ai/thunks.tsweb/src/core/usecases/index.tsweb/src/core/usecases/launcher/thunks.tsweb/src/core/usecases/userConfigs.tsweb/src/ui/i18n/resources/de.tsxweb/src/ui/i18n/resources/en.tsxweb/src/ui/i18n/resources/es.tsxweb/src/ui/i18n/resources/fi.tsxweb/src/ui/i18n/resources/fr.tsxweb/src/ui/i18n/resources/it.tsxweb/src/ui/i18n/resources/nl.tsxweb/src/ui/i18n/resources/no.tsxweb/src/ui/i18n/resources/zh-CN.tsxweb/src/ui/i18n/types.tsweb/src/ui/pages/account/AccountAiTab.tsxweb/src/ui/pages/account/Page.tsxweb/src/ui/pages/account/accountTabIds.ts
💤 Files with no reviewable changes (13)
- web/src/ui/i18n/resources/en.tsx
- web/src/ui/pages/account/accountTabIds.ts
- web/src/ui/i18n/resources/fr.tsx
- web/src/ui/i18n/resources/zh-CN.tsx
- web/src/ui/i18n/types.ts
- web/src/ui/i18n/resources/it.tsx
- web/src/core/usecases/ai/index.ts
- web/src/ui/i18n/resources/nl.tsx
- web/src/ui/i18n/resources/no.tsx
- web/src/ui/i18n/resources/es.tsx
- web/src/ui/pages/account/Page.tsx
- web/src/ui/i18n/resources/fi.tsx
- web/src/ui/pages/account/AccountAiTab.tsx
✅ Files skipped from review due to trivial changes (1)
- web/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (7)
- web/src/core/adapters/ai/index.ts
- web/src/core/ports/OnyxiaApi/DeploymentRegion.ts
- web/src/core/usecases/index.ts
- web/src/core/adapters/onyxiaApi/ApiTypes.ts
- web/src/ui/i18n/resources/de.tsx
- web/src/core/tools/oidcTokenExchange.ts
- web/src/core/adapters/ai/openWebUi.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/core/usecases/ai/state.ts (1)
140-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
editCustomProviderkeeps a possibly-staleselectedModelIdafter credential change.You reset
modelstofetchingbut leaveselectedModelIduntouched. When the refetched catalog no longer contains the oldselectedModelId,modelsLoadedwon't replace it (it only defaults whenselectedModelId === undefined), soselectors.activeProvidercan inject a model id that the new endpoint doesn't expose.Consider clearing the selection here so the first available model is re-defaulted on load:
🔧 Proposed fix
provider.label = payload.label; provider.apiBase = payload.apiBase; provider.apiKey = payload.apiKey; // Credentials changed → the previous models list no longer applies. provider.models = { stateDescription: "fetching" }; + // Drop the stale selection so the new catalog re-defaults it. + provider.selectedModelId = undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/usecases/ai/state.ts` around lines 140 - 163, editCustomProvider currently resets provider.models to fetching after credentials change, but it leaves provider.selectedModelId behind, which can keep a stale model selection and break selectors.activeProvider. Update editCustomProvider in state.ts to also clear the selected model when apiBase/apiKey/label change, so modelsLoaded can re-default to the first available model after the refetch. Use the existing editCustomProvider and modelsLoaded flow to locate the fix.
♻️ Duplicate comments (1)
web/src/ui/pages/account/AccountAiTab.tsx (1)
146-153: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
onSaveswallows add/edit failures.The dialog is closed before awaiting
ai.addCustomProvider/ai.editCustomProvider, and neither call is wrapped, so a rejection (e.g.persistConfigthrowing) becomes an unhandled rejection with no user feedback. This mirrors the earlier concern; the close-first ordering also means a failure can't surface in the form anymore.
🧹 Nitpick comments (1)
web/src/ui/pages/account/AccountAiTab.tsx (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-export core model types instead of redefining them.
AiModel/Modelshere duplicateState.AiModel/State.Modelsfromcore/usecases/ai/state. Since themainselector already returnsprovider.modelstyped as the core shape, importing those types keeps the UI in lockstep and avoids silent drift if the core union changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/pages/account/AccountAiTab.tsx` around lines 28 - 34, `AccountAiTab` is duplicating the AI model state types instead of using the shared core definitions. Update the `AiModel` and `Models` references in `AccountAiTab.tsx` to import and reuse `State.AiModel` and `State.Models` from `core/usecases/ai/state`, and then align the `main` selector usage with those shared types so the UI stays consistent with the core union shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/src/core/usecases/ai/state.ts`:
- Around line 140-163: editCustomProvider currently resets provider.models to
fetching after credentials change, but it leaves provider.selectedModelId
behind, which can keep a stale model selection and break
selectors.activeProvider. Update editCustomProvider in state.ts to also clear
the selected model when apiBase/apiKey/label change, so modelsLoaded can
re-default to the first available model after the refetch. Use the existing
editCustomProvider and modelsLoaded flow to locate the fix.
---
Nitpick comments:
In `@web/src/ui/pages/account/AccountAiTab.tsx`:
- Around line 28-34: `AccountAiTab` is duplicating the AI model state types
instead of using the shared core definitions. Update the `AiModel` and `Models`
references in `AccountAiTab.tsx` to import and reuse `State.AiModel` and
`State.Models` from `core/usecases/ai/state`, and then align the `main` selector
usage with those shared types so the UI stays consistent with the core union
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5a5cbf0-d313-4254-b550-5452ab155eb6
📒 Files selected for processing (9)
web/src/core/bootstrap.tsweb/src/core/tools/fetchAiModels.tsweb/src/core/tools/oidcTokenExchange.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.test.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.tsweb/src/core/usecases/ai/selectors.tsweb/src/core/usecases/ai/state.tsweb/src/core/usecases/ai/thunks.tsweb/src/ui/pages/account/AccountAiTab.tsx
✅ Files skipped from review due to trivial changes (1)
- web/src/core/usecases/ai/decoupledLogic/persistedAiConfig.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/core/tools/oidcTokenExchange.ts
- web/src/core/bootstrap.ts
ddecrulle
force-pushed
the
ia-integration
branch
from
bfa9d62 to
0e6116d
Compare
- Add AI usecase (state/thunks/selectors) with initializeStart/initializeSucceed/initializeFailed lifecycle actions - getToken() returns a discriminated result type in the Ai port — no-account (403) vs error cases handled without leaking adapter details into usecases - Gracefully disable AI features on init failure; show a link to the gateway URL when user has no account - Add AccountAiGatewayTab with token/model display and full i18n for all 9 languages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The access token is handed over to OpenWebUI's token exchange endpoint, which validates it server-side and cannot present a DPoP proof. It must therefore be a plain bearer token, never sender-constrained, even when DPoP is globally enabled. Forward oidc-spa's `disableDPoP` flag through the createOidc wrapper and set it on the dedicated AI OIDC client instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the embeddings model picker across all layers: UI select row and i18n key, usecase state/thunk/selector, persisted config schema, and the `embeddingsModel` field of the XOnyxia templating context. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddecrulle
force-pushed
the
ia-integration
branch
from
0e6116d to
dd2c32e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
web/src/ui/pages/account/Page.tsx (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured absolute import path.
The new lazy import uses a relative path; this file is under
web/src, so it should use thesrcbase URL.♻️ Proposed import update
-const AccountAiGatewayTab = lazy(() => import("./AccountAiTab")); +const AccountAiGatewayTab = lazy(() => import("ui/pages/account/AccountAiTab"));As per coding guidelines, “Use absolute imports from the
srcbase URL instead of relative paths.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/ui/pages/account/Page.tsx` at line 24, The lazy import for AccountAiGatewayTab is using a relative path instead of the project’s configured src-based absolute import. Update the import in Page.tsx to use the absolute base-url style from src so it matches the codebase convention and avoids brittle relative paths. Keep the change localized to the lazy() call for AccountAiTab.Source: Coding guidelines
web/src/core/bootstrap.ts (1)
319-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid embedding an invisible separator in source.
Line 319 contains a literal control character in the template string. Use a visible encoding for the tuple key instead; it avoids copy/paste and tooling surprises.
♻️ Proposed key encoding
- const oidcKey = `${oidcParams_ai.issuerUri}�${oidcParams_ai.clientId}`; + const oidcKey = JSON.stringify([ + oidcParams_ai.issuerUri, + oidcParams_ai.clientId + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/bootstrap.ts` at line 319, The OIDC tuple key in bootstrap.ts is using an embedded control character separator, which should be removed. Update the key construction around oidcParams_ai so it uses a visible, explicit encoding for the issuerUri/clientId pair instead of a literal invisible character, keeping the logic in the same place where oidcKey is created.web/src/core/ports/Ai.ts (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the port as an interface.
Aiis a dependency contract undersrc/core/ports/, so this should be aninterfacerather than a structural type alias.As per coding guidelines,
web/src/core/ports/**/*.tsshould define TypeScript interfaces that describe contracts for external dependencies.Proposed change
-export type Ai = { +export interface Ai { id: string; name: string; @@ apiBase: string; getToken: () => Promise<GetTokenResult>; listModels: (token: string) => Promise<{ id: string; name: string }[]>; -}; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/ports/Ai.ts` around lines 1 - 14, `Ai` in the core ports layer is currently declared as a type alias, but this contract should be an interface to match the dependency-port pattern used under `web/src/core/ports`. Update the `Ai` declaration itself to an interface and keep the existing members (`id`, `name`, `provider`, `webUiUrl`, `apiBase`, `getToken`, `listModels`) unchanged so downstream consumers continue to use it as the external dependency contract.Source: Coding guidelines
web/src/core/usecases/ai/selectors.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse absolute imports for
state.
./statebreaks the import convention for files underweb/src.As per coding guidelines,
web/src/**/*.{ts,tsx}should use absolute imports from thesrcbase URL instead of relative paths.Proposed change
-import { name } from "./state"; -import type { State } from "./state"; +import { name } from "core/usecases/ai/state"; +import type { State } from "core/usecases/ai/state";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/usecases/ai/selectors.ts` around lines 3 - 4, The imports in selectors.ts use a relative path to state, which violates the web/src absolute import convention. Update the imports in the selectors module to reference state via the src base URL instead of "./state", keeping the existing symbols name and State unchanged while switching only the import paths.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/core/adapters/onyxiaApi/onyxiaApi.ts`:
- Around line 448-450: The region AI provider mapping currently uses
aiConfigs_api.map with a fallback id of onyxia-${i}, which is order-dependent
and can change when the API reorders providers. Update the fallback in the
onyxiaApi adapter to derive from stable provider fields already available in
each aiConfig_api entry (for example id, URL, or another immutable provider
attribute) instead of the loop index, so persisted activeProviderId values
continue to point to the same gateway.
In `@web/src/core/usecases/ai/selectors.ts`:
- Around line 26-29: The provider activation flag in select AI selectors
currently only checks provider.models.stateDescription, which incorrectly allows
loaded providers with no availableModels to be activatable. Update the
canBeActivated logic in selectors.ts to also require a non-empty models list
(for example by checking provider.models.availableModels length) so zero-model
providers are not exposed as launchable, while keeping the existing
provider.models and selectedModelId mapping intact.
In `@web/src/core/usecases/ai/state.ts`:
- Around line 107-121: modelsLoaded in state.ts only initializes selectedModelId
when it is undefined, so a refresh can leave provider.selectedModelId pointing
to a model no longer present in provider.models.availableModels. Update the
modelsLoaded handler to validate the current selection against payload.models
and clear or replace it when it is stale, while still preserving the existing
default-to-first-model behavior for providers with no valid selection.
In `@web/src/ui/pages/account/Page.tsx`:
- Around line 93-94: The direct route handling in Page.tsx still renders
AccountAiGatewayTab even when AI is unavailable, because the switch case for
"ai" has no availability check. Update the route/render logic in the Page
component to gate the "ai" case with ai.isAvailable() (the same guard used to
hide the navigation tab), and fall back to the existing not-available handling
when AI is disabled.
---
Nitpick comments:
In `@web/src/core/bootstrap.ts`:
- Line 319: The OIDC tuple key in bootstrap.ts is using an embedded control
character separator, which should be removed. Update the key construction around
oidcParams_ai so it uses a visible, explicit encoding for the issuerUri/clientId
pair instead of a literal invisible character, keeping the logic in the same
place where oidcKey is created.
In `@web/src/core/ports/Ai.ts`:
- Around line 1-14: `Ai` in the core ports layer is currently declared as a type
alias, but this contract should be an interface to match the dependency-port
pattern used under `web/src/core/ports`. Update the `Ai` declaration itself to
an interface and keep the existing members (`id`, `name`, `provider`,
`webUiUrl`, `apiBase`, `getToken`, `listModels`) unchanged so downstream
consumers continue to use it as the external dependency contract.
In `@web/src/core/usecases/ai/selectors.ts`:
- Around line 3-4: The imports in selectors.ts use a relative path to state,
which violates the web/src absolute import convention. Update the imports in the
selectors module to reference state via the src base URL instead of "./state",
keeping the existing symbols name and State unchanged while switching only the
import paths.
In `@web/src/ui/pages/account/Page.tsx`:
- Line 24: The lazy import for AccountAiGatewayTab is using a relative path
instead of the project’s configured src-based absolute import. Update the import
in Page.tsx to use the absolute base-url style from src so it matches the
codebase convention and avoids brittle relative paths. Keep the change localized
to the lazy() call for AccountAiTab.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbe020c8-4aac-41a2-b19f-e3590c77f263
📒 Files selected for processing (39)
apiweb/.envweb/CLAUDE.mdweb/src/core/adapters/ai/index.tsweb/src/core/adapters/ai/openWebUi.tsweb/src/core/adapters/oidc/oidc.tsweb/src/core/adapters/onyxiaApi/ApiTypes.tsweb/src/core/adapters/onyxiaApi/onyxiaApi.tsweb/src/core/bootstrap.tsweb/src/core/ports/Ai.tsweb/src/core/ports/OnyxiaApi/DeploymentRegion.tsweb/src/core/ports/OnyxiaApi/XOnyxia.tsweb/src/core/tools/fetchAiModels.tsweb/src/core/tools/oidcTokenExchange.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.test.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.tsweb/src/core/usecases/ai/index.tsweb/src/core/usecases/ai/selectors.tsweb/src/core/usecases/ai/state.tsweb/src/core/usecases/ai/thunks.tsweb/src/core/usecases/index.tsweb/src/core/usecases/launcher/thunks.tsweb/src/core/usecases/userConfigs.tsweb/src/env.tsweb/src/ui/App/App.tsxweb/src/ui/i18n/resources/de.tsxweb/src/ui/i18n/resources/en.tsxweb/src/ui/i18n/resources/es.tsxweb/src/ui/i18n/resources/fi.tsxweb/src/ui/i18n/resources/fr.tsxweb/src/ui/i18n/resources/it.tsxweb/src/ui/i18n/resources/nl.tsxweb/src/ui/i18n/resources/no.tsxweb/src/ui/i18n/resources/zh-CN.tsxweb/src/ui/i18n/types.tsweb/src/ui/pages/account/AccountAiTab.tsxweb/src/ui/pages/account/Page.tsxweb/src/ui/pages/account/accountTabIds.tsweb/src/vite-env.d.ts
✅ Files skipped from review due to trivial changes (11)
- web/src/core/adapters/ai/index.ts
- web/src/ui/i18n/types.ts
- web/src/ui/pages/account/accountTabIds.ts
- web/src/vite-env.d.ts
- web/src/core/usecases/ai/decoupledLogic/persistedAiConfig.test.ts
- web/src/core/usecases/index.ts
- web/src/core/usecases/userConfigs.ts
- web/src/ui/i18n/resources/nl.tsx
- web/src/ui/i18n/resources/fr.tsx
- web/src/ui/i18n/resources/es.tsx
- web/CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (19)
- web/src/core/usecases/ai/index.ts
- web/.env
- web/src/ui/App/App.tsx
- web/src/core/tools/fetchAiModels.ts
- web/src/env.ts
- web/src/core/tools/oidcTokenExchange.ts
- web/src/core/adapters/oidc/oidc.ts
- web/src/core/ports/OnyxiaApi/DeploymentRegion.ts
- web/src/ui/i18n/resources/de.tsx
- web/src/ui/i18n/resources/fi.tsx
- web/src/ui/i18n/resources/en.tsx
- web/src/ui/i18n/resources/zh-CN.tsx
- web/src/ui/i18n/resources/no.tsx
- web/src/core/adapters/onyxiaApi/ApiTypes.ts
- web/src/core/usecases/ai/decoupledLogic/persistedAiConfig.ts
- web/src/ui/i18n/resources/it.tsx
- web/src/core/adapters/ai/openWebUi.ts
- web/src/core/usecases/ai/thunks.ts
- web/src/ui/pages/account/AccountAiTab.tsx
Comment on lines
+448
to
+450
| return aiConfigs_api.map((aiConfig_api, i) => ({ | ||
| id: aiConfig_api.id ?? `onyxia-${i}`, | ||
| url: aiConfig_api.URL, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a stable fallback ID for region AI providers.
onyxia-${i} depends on the array order returned by the API. If the region config reorders providers later, persisted activeProviderId values can silently reactivate a different gateway. Derive the fallback from stable provider data instead of the current index.
Proposed change
- return aiConfigs_api.map((aiConfig_api, i) => ({
- id: aiConfig_api.id ?? `onyxia-${i}`,
+ return aiConfigs_api.map(aiConfig_api => ({
+ id:
+ aiConfig_api.id ??
+ `${apiRegion.id}:${aiConfig_api.URL}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return aiConfigs_api.map((aiConfig_api, i) => ({ | |
| id: aiConfig_api.id ?? `onyxia-${i}`, | |
| url: aiConfig_api.URL, | |
| return aiConfigs_api.map(aiConfig_api => ({ | |
| id: | |
| aiConfig_api.id ?? | |
| `${apiRegion.id}:${aiConfig_api.URL}`, |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 448-448: Refactor this code to not nest functions more than 4 levels deep.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/adapters/onyxiaApi/onyxiaApi.ts` around lines 448 - 450, The
region AI provider mapping currently uses aiConfigs_api.map with a fallback id
of onyxia-${i}, which is order-dependent and can change when the API reorders
providers. Update the fallback in the onyxiaApi adapter to derive from stable
provider fields already available in each aiConfig_api entry (for example id,
URL, or another immutable provider attribute) instead of the loop index, so
persisted activeProviderId values continue to point to the same gateway.
Comment on lines
+26
to
+29
| // A provider can only be wired into services once its models are listed. | ||
| canBeActivated: provider.models?.stateDescription === "loaded", | ||
| models: provider.models, | ||
| selectedModelId: provider.selectedModelId |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't mark zero-model providers as activatable.
A provider with { stateDescription: "loaded", availableModels: [] } currently gets canBeActivated: true. That exposes a provider the user cannot actually launch with a valid model.
Proposed change
- canBeActivated: provider.models?.stateDescription === "loaded",
+ canBeActivated:
+ provider.models?.stateDescription === "loaded"
+ ? provider.models.availableModels.length > 0
+ : false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // A provider can only be wired into services once its models are listed. | |
| canBeActivated: provider.models?.stateDescription === "loaded", | |
| models: provider.models, | |
| selectedModelId: provider.selectedModelId | |
| // A provider can only be wired into services once its models are listed. | |
| canBeActivated: | |
| provider.models?.stateDescription === "loaded" | |
| ? provider.models.availableModels.length > 0 | |
| : false, | |
| models: provider.models, | |
| selectedModelId: provider.selectedModelId |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/usecases/ai/selectors.ts` around lines 26 - 29, The provider
activation flag in select AI selectors currently only checks
provider.models.stateDescription, which incorrectly allows loaded providers with
no availableModels to be activatable. Update the canBeActivated logic in
selectors.ts to also require a non-empty models list (for example by checking
provider.models.availableModels length) so zero-model providers are not exposed
as launchable, while keeping the existing provider.models and selectedModelId
mapping intact.
Comment on lines
+107
to
+121
| modelsLoaded: ( | ||
| state, | ||
| { payload }: { payload: { providerId: string; models: State.AiModel[] } } | ||
| ) => { | ||
| assert(state.stateDescription === "initialized"); | ||
| const provider = state.providers.find(p => p.id === payload.providerId); | ||
| if (provider === undefined) return; | ||
| provider.models = { | ||
| stateDescription: "loaded", | ||
| availableModels: payload.models | ||
| }; | ||
| // Default the chat model to the first available one if none is set. | ||
| if (provider.selectedModelId === undefined && payload.models.length > 0) { | ||
| provider.selectedModelId = payload.models[0].id; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Drop stale model selections on catalog refresh.
modelsLoaded only reselects when selectedModelId is already undefined. If the gateway refresh removes the previously selected model, this keeps an invalid ID in state and the launcher context will keep pointing at a model the provider no longer exposes.
Proposed change
provider.models = {
stateDescription: "loaded",
availableModels: payload.models
};
- // Default the chat model to the first available one if none is set.
- if (provider.selectedModelId === undefined && payload.models.length > 0) {
- provider.selectedModelId = payload.models[0].id;
- }
+ const stillExists = payload.models.some(
+ model => model.id === provider.selectedModelId
+ );
+ provider.selectedModelId = stillExists
+ ? provider.selectedModelId
+ : payload.models[0]?.id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| modelsLoaded: ( | |
| state, | |
| { payload }: { payload: { providerId: string; models: State.AiModel[] } } | |
| ) => { | |
| assert(state.stateDescription === "initialized"); | |
| const provider = state.providers.find(p => p.id === payload.providerId); | |
| if (provider === undefined) return; | |
| provider.models = { | |
| stateDescription: "loaded", | |
| availableModels: payload.models | |
| }; | |
| // Default the chat model to the first available one if none is set. | |
| if (provider.selectedModelId === undefined && payload.models.length > 0) { | |
| provider.selectedModelId = payload.models[0].id; | |
| } | |
| modelsLoaded: ( | |
| state, | |
| { payload }: { payload: { providerId: string; models: State.AiModel[] } } | |
| ) => { | |
| assert(state.stateDescription === "initialized"); | |
| const provider = state.providers.find(p => p.id === payload.providerId); | |
| if (provider === undefined) return; | |
| provider.models = { | |
| stateDescription: "loaded", | |
| availableModels: payload.models | |
| }; | |
| const stillExists = payload.models.some( | |
| model => model.id === provider.selectedModelId | |
| ); | |
| provider.selectedModelId = stillExists | |
| ? provider.selectedModelId | |
| : payload.models[0]?.id; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/usecases/ai/state.ts` around lines 107 - 121, modelsLoaded in
state.ts only initializes selectedModelId when it is undefined, so a refresh can
leave provider.selectedModelId pointing to a model no longer present in
provider.models.availableModels. Update the modelsLoaded handler to validate the
current selection against payload.models and clear or replace it when it is
stale, while still preserving the existing default-to-first-model behavior for
providers with no valid selection.
Comment on lines
+93
to
+94
| case "ai": | ||
| return <AccountAiGatewayTab />; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Gate direct route rendering with ai.isAvailable().
Line 52 only removes the tab from navigation. A direct route to the AI tab still reaches this switch and renders AccountAiGatewayTab even when AI is unavailable.
🛡️ Proposed guard
case "ai":
- return <AccountAiGatewayTab />;
+ return ai.isAvailable() ? (
+ <AccountAiGatewayTab />
+ ) : (
+ <AccountProfileTab />
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "ai": | |
| return <AccountAiGatewayTab />; | |
| case "ai": | |
| return ai.isAvailable() ? ( | |
| <AccountAiGatewayTab /> | |
| ) : ( | |
| <AccountProfileTab /> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/pages/account/Page.tsx` around lines 93 - 94, The direct route
handling in Page.tsx still renders AccountAiGatewayTab even when AI is
unavailable, because the switch case for "ai" has no availability check. Update
the route/render logic in the Page component to gate the "ai" case with
ai.isAvailable() (the same guard used to hide the navigation tab), and fall back
to the existing not-available handling when AI is disabled.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/core/bootstrap.ts`:
- Line 321: The OIDC cache key in bootstrap.ts is built with a raw NUL
separator, which can break text tooling and editor handling. Update the key
generation in the oidcKey assignment to use a safe serialized delimiter instead,
such as JSON.stringify over issuerUri and clientId, and ensure any lookup code
that relies on this key uses the same serialization in the related OIDC
bootstrap flow.
- Around line 301-308: `bootstrapCore` should not crash when
`onyxiaApi.getAvailableRegionsAndOidcParams()` returns undefined in this branch;
replace the `assert(oidcParams !== undefined)` in the `Promise.all` result
handling with a guard that skips the region-backed OIDC/Ai adapter setup when no
global OIDC config exists. Use the existing `createAi`, `createOidc`, and
`mergeOidcParams` flow to continue custom-provider initialization only when
`oidcParams` is present, and keep the fallback behavior consistent with the
earlier undefined handling in `bootstrapCore`.
In `@web/src/core/usecases/ai/selectors.ts`:
- Around line 76-94: The provider selector is leaking secrets by keeping apiKey
on every ready provider view, which then gets serialized into XOnyxiaContext by
the launch flow. Update the selector in selectors.ts so providerViews entries
for inactive providers do not include apiKey, and keep the secret only on the
activeProvider returned by the selector. Make sure to preserve the existing
provider shaping in toProviderView while redacting the secret field from the
array used for providers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12658a0c-65ca-4b3d-a66b-097525cdb11d
📒 Files selected for processing (5)
web/src/core/bootstrap.tsweb/src/core/ports/OnyxiaApi/XOnyxia.tsweb/src/core/usecases/ai/selectors.tsweb/src/core/usecases/ai/thunks.tsweb/src/core/usecases/launcher/thunks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/core/usecases/ai/thunks.ts
Comment on lines
+301
to
+308
| const [{ createAi }, { createOidc, mergeOidcParams }, { oidcParams }] = | ||
| await Promise.all([ | ||
| import("core/adapters/ai"), | ||
| import("core/adapters/oidc"), | ||
| onyxiaApi.getAvailableRegionsAndOidcParams() | ||
| ]); | ||
|
|
||
| assert(oidcParams !== undefined); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle missing global OIDC params here instead of asserting.
bootstrapCore already accepts getAvailableRegionsAndOidcParams() returning undefined earlier in this file (Lines 120-127). If that happens again here, assert(oidcParams !== undefined) turns deploymentRegion.ai into a bootstrap-time crash and prevents custom-provider init from continuing. This branch should just skip region-backed adapters when no base OIDC config exists.
Suggested fix
- assert(oidcParams !== undefined);
+ if (oidcParams === undefined) {
+ break region_ai;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [{ createAi }, { createOidc, mergeOidcParams }, { oidcParams }] = | |
| await Promise.all([ | |
| import("core/adapters/ai"), | |
| import("core/adapters/oidc"), | |
| onyxiaApi.getAvailableRegionsAndOidcParams() | |
| ]); | |
| assert(oidcParams !== undefined); | |
| const [{ createAi }, { createOidc, mergeOidcParams }, { oidcParams }] = | |
| await Promise.all([ | |
| import("core/adapters/ai"), | |
| import("core/adapters/oidc"), | |
| onyxiaApi.getAvailableRegionsAndOidcParams() | |
| ]); | |
| if (oidcParams === undefined) { | |
| break region_ai; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/bootstrap.ts` around lines 301 - 308, `bootstrapCore` should not
crash when `onyxiaApi.getAvailableRegionsAndOidcParams()` returns undefined in
this branch; replace the `assert(oidcParams !== undefined)` in the `Promise.all`
result handling with a guard that skips the region-backed OIDC/Ai adapter setup
when no global OIDC config exists. Use the existing `createAi`, `createOidc`,
and `mergeOidcParams` flow to continue custom-provider initialization only when
`oidcParams` is present, and keep the fallback behavior consistent with the
earlier undefined handling in `bootstrapCore`.
| oidcParams_partial: aiConfig.oidcParams | ||
| }); | ||
|
|
||
| const oidcKey = `${oidcParams_ai.issuerUri} ${oidcParams_ai.clientId}`; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("web/src/core/bootstrap.ts")
data = path.read_bytes()
idx = data.find(b"\x00")
print({"path": str(path), "first_nul_offset": idx})
if idx != -1:
start = max(0, idx - 40)
end = min(len(data), idx + 40)
print(data[start:end])
PYRepository: InseeFrLab/onyxia
Length of output: 1986
Replace the literal NUL key separator. A raw \0 in source can confuse text-based tooling and editors; use an escaped delimiter or tuple serialization instead, e.g. JSON.stringify([issuerUri, clientId]).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/bootstrap.ts` at line 321, The OIDC cache key in bootstrap.ts is
built with a raw NUL separator, which can break text tooling and editor
handling. Update the key generation in the oidcKey assignment to use a safe
serialized delimiter instead, such as JSON.stringify over issuerUri and
clientId, and ensure any lookup code that relies on this key uses the same
serialization in the related OIDC bootstrap flow.
Comment on lines
+76
to
+94
| return { | ||
| id: provider.id, | ||
| name: provider.kind === "region" ? provider.name : provider.label, | ||
| provider: provider.provider, | ||
| apiBase: provider.apiBase, | ||
| apiKey, | ||
| models, | ||
| selectedModel: provider.selectedModelId | ||
| }; | ||
| }; | ||
|
|
||
| const providerViews = (providers ?? []) | ||
| .map(toProviderView) | ||
| .filter(view => view !== undefined); | ||
|
|
||
| return { | ||
| enabled: providerViews.length > 0, | ||
| activeProvider: providerViews.find(p => p.id === activeProviderId), | ||
| providers: providerViews.filter(p => p.id !== activeProviderId) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Don't serialize every provider secret into XOnyxiaContext.
This selector keeps apiKey on every ready provider view, and launcher/thunks.ts Line 780 injects that whole object into the Helm context. That exposes all saved custom API keys / exchanged region tokens to every launch, even when only activeProvider needs the secret. Please redact apiKey from inactive providers entries and keep it only on the selected provider.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/core/usecases/ai/selectors.ts` around lines 76 - 94, The provider
selector is leaking secrets by keeping apiKey on every ready provider view,
which then gets serialized into XOnyxiaContext by the launch flow. Update the
selector in selectors.ts so providerViews entries for inactive providers do not
include apiKey, and keep the secret only on the activeProvider returned by the
selector. Make sure to preserve the existing provider shaping in toProviderView
while redacting the secret field from the array used for providers.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/core/usecases/launcher/decoupledLogic/computeHelmValues.test.ts (1)
141-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the editor schema in this regression too.
This case only checks
got.helmValues, but the reordered object path also mutateshelmValuesSchema_forDataTextEditor.default. A regression there would still pass this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/core/usecases/launcher/decoupledLogic/computeHelmValues.test.ts` around lines 141 - 144, The regression test in computeHelmValues should also verify the editor schema, not just got.helmValues, because the reordered object path can also mutate helmValuesSchema_forDataTextEditor.default. Update the existing assertion block around computeHelmValues to include a check on helmValuesSchema_forDataTextEditor.default alongside the current activeProvider/providers expectation so the test fails if the schema is accidentally changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/ui/pages/account/AccountAiTab.tsx`:
- Around line 523-525: The model selector in AccountAiTab no longer allows
returning to the undefined state because the only "not defined" MenuItem is
disabled. Update the select options rendering in AccountAiTab so the value
representing an empty selection is still selectable (or add an explicit clear
option) while preserving the t("not defined") label, and ensure the change still
maps to the same field/state used by the model selection control.
---
Nitpick comments:
In `@web/src/core/usecases/launcher/decoupledLogic/computeHelmValues.test.ts`:
- Around line 141-144: The regression test in computeHelmValues should also
verify the editor schema, not just got.helmValues, because the reordered object
path can also mutate helmValuesSchema_forDataTextEditor.default. Update the
existing assertion block around computeHelmValues to include a check on
helmValuesSchema_forDataTextEditor.default alongside the current
activeProvider/providers expectation so the test fails if the schema is
accidentally changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60332e77-96cd-44bd-a985-58becdb6c61a
📒 Files selected for processing (18)
web/src/core/ports/OnyxiaApi/XOnyxia.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.test.tsweb/src/core/usecases/ai/decoupledLogic/persistedAiConfig.tsweb/src/core/usecases/ai/selectors.tsweb/src/core/usecases/ai/state.tsweb/src/core/usecases/ai/thunks.tsweb/src/core/usecases/launcher/decoupledLogic/computeHelmValues.test.tsweb/src/core/usecases/launcher/decoupledLogic/computeHelmValues.tsweb/src/ui/i18n/resources/de.tsxweb/src/ui/i18n/resources/en.tsxweb/src/ui/i18n/resources/es.tsxweb/src/ui/i18n/resources/fi.tsxweb/src/ui/i18n/resources/fr.tsxweb/src/ui/i18n/resources/it.tsxweb/src/ui/i18n/resources/nl.tsxweb/src/ui/i18n/resources/no.tsxweb/src/ui/i18n/resources/zh-CN.tsxweb/src/ui/pages/account/AccountAiTab.tsx
💤 Files with no reviewable changes (1)
- web/src/core/ports/OnyxiaApi/XOnyxia.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/ui/i18n/resources/fr.tsx
- web/src/ui/i18n/resources/en.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- web/src/ui/i18n/resources/es.tsx
- web/src/core/usecases/ai/decoupledLogic/persistedAiConfig.test.ts
- web/src/ui/i18n/resources/de.tsx
- web/src/ui/i18n/resources/nl.tsx
- web/src/ui/i18n/resources/zh-CN.tsx
- web/src/ui/i18n/resources/it.tsx
- web/src/core/usecases/ai/decoupledLogic/persistedAiConfig.ts
- web/src/ui/i18n/resources/no.tsx
- web/src/ui/i18n/resources/fi.tsx
- web/src/core/usecases/ai/state.ts
- web/src/core/usecases/ai/thunks.ts
Comment on lines
+523
to
+525
| <MenuItem value="" disabled> | ||
| {t("not defined")} | ||
| </MenuItem> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Users can't clear a model selection anymore.
The only option representing the undefined state is disabled, so once a model is chosen there is no way back to the "not defined" state from this UI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/ui/pages/account/AccountAiTab.tsx` around lines 523 - 525, The model
selector in AccountAiTab no longer allows returning to the undefined state
because the only "not defined" MenuItem is disabled. Update the select options
rendering in AccountAiTab so the value representing an empty selection is still
selectable (or add an explicit clear option) while preserving the t("not
defined") label, and ensure the change still maps to the same field/state used
by the model selection control.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aiport (OIDC token exchange, model listing) with an OpenWebUI adapter and a mock; config comes from the deployment region(
region.ai)aiusecase — initialize lifecycle (pending/success/no-account/error), token refresh, model selection; selected model is injected into Helm values when launching aservice
localStorage
/models), shows success with model count or an error messageSummary by CodeRabbit