feat: credentials, per-user LLM settings, API keys management, oauth2-proxy integration

[Kaiohz]

Context

Frontend for composables: credentials (cookie-based via oauth2-proxy), per-user LLM settings UI, per-user API keys management. Depends on composable-agents PR #39 (backend endpoints) + flux ticket 4 (oauth2-proxy in stack).

Changes

• withCredentials on all axios instances + SSE credentials:include (cross-subdomain cookie auth via oauth2-proxy, cookie domain .soludev.tech).
• 401 redirect to /oauth2/start?rd= with infinite-loop guard. Logout -> /oauth2/sign_out.
• Per-user LLM settings: Settings page LLM Provider card persists to backend PUT /api/v1/settings/llm (replaces localStorage apiKey). GET shows masked key. DELETE removes.
• Per-user API keys: new API Keys card — list, create (plaintext shown ONCE + copy + warning), revoke (confirm dialog).
• Hexagonal: domain entities (LlmSettings, ApiKey) + ports (ISettingsPort, IApiKeyPort) + adapters (settingsApi, apiKeyApi) + hooks (useLlmSettings, useApiKeys) + components (LlmSettingsCard, ApiKeysCard).

Quality gates

Gate	Status
Unit tests	780 pass (+34 new)
Code review	0 critical (2 fixés: oauth2 anti-boucle, useEffect deps)
Lint + format	clean (17 erreurs préexistantes hors scope)
Build	OK (456ms)
SonarQube	0 new critical (4 critical préexistantes hors scope)
Trivy	0 new vuln (39 préexistantes axios/react-router hors scope)
QA	front builds + serves (2 health tests pass); e2e settings UI deferred to oauth2-proxy (ticket 4)
Documentation	README updated

Breaking changes

• Old localStorage apiKey/llmProvider removed -> backend per-user LLM settings (requires composable-agents PR #39).
• Auth requires oauth2-proxy cookie (no SDK) -> local QA without oauth2-proxy shows 401 redirect (expected).

Tests

• Unit (780)
• QA health (2 pass)
• Build
• e2e settings/api-keys UI (deferred to ticket 4: oauth2-proxy in stack)

[Kaiohz]

Code review — PR #24 (credentials, LLM settings, API keys, oauth2-proxy)

Scope: 1968+/154- across 33 files. Hexagonal split respected (domain ports → infrastructure adapters → application hooks → views). 780 tests pass (+34 new). Cookie auth + 401 redirect + per-user settings + plain key shown once.

✅ Positives

• Hexagonal boundaries clean. ISettingsPort / IApiKeyPort in domain/, settingsApi / apiKeyApi in infrastructure/, useLlmSettings / useApiKeys in application/hooks/. View components only consume the hooks. This is the right shape and matches the rest of the codebase.
• Plaintext key flow is correct. CreatedApiKey.plaintext carried once, CreatedKeyDialog shows it + copy + "you won't see this again" warning, never persisted. Hexagonal contract (apiKeyApi.create returns CreatedApiKey) is honest about the one-shot exposure.
• Migration is safe. New migratePersisted strips legacy llmProvider/apiKey from the persisted Zustand blob before re-hydrating, so existing users don't carry stale fields. Good defensive comment.
• 401 redirect guard is genuine. isOnAuthPage() checks the pathname against both /oauth2/start and /oauth2/sign_out, so a 401 fired during the auth round-trip won't loop. Tests cover the loop-guard case explicitly.
• withCredentials on every axios instance + SSE credentials: "include" — cookie auth is consistent across REST and streaming. No risk of one channel silently dropping the cookie.
• AlertDialog via Radix is the right primitive (focus trap, Escape, ARIA). The "Confirm revoke" confirmation is a real UX win over a plain window.confirm.
• Tests are tight, not theatrical. axiosInstances.test.ts actually intercepts the rejected handler and asserts on hrefSetter; the loop-guard test overrides window.location to verify no redirect fires. useApiKeys.test.tsx / useLlmSettings.test.tsx are minimal hook contracts.
• Race-avoidance in LlmSettingsCard. The useEffect deps list (settingsProvider, settingsBaseUrl, settingsUpdatedAt) — explicitly chosen over the settings object identity, with a comment explaining why. This is the kind of thing that gets done wrong by default; doing it right is worth a shout-out.

🔎 Findings

1. (LlmSettingsCard.tsx ~lines 65–85) useEffect deps list is partially redundant + can race on first render
The effect resets the form whenever settingsProvider, settingsBaseUrl, or settingsUpdatedAt change. On the very first render after isLoading flips false, settings?.updated_at is whatever the backend returned, so the effect fires once with non-undefined values — fine. But the form defaultValues is also { provider: "openai", base_url: …openai, api_key: "" }, so you have two sources of truth. If the backend ever returns provider: "openrouter" but the user submits before the effect runs, the setValue("provider", value) call in onProviderChange will fight the reset() in the effect. Not a bug today (the effect runs before user interaction), but the dual ownership is fragile. Consider setting defaultValues based on settings ?? DEFAULTS instead of always "openai", and remove the effect reset entirely — react-hook-form won't re-render from external state unless you tell it to.

2. (LlmSettingsCard.tsx ~lines 95–106) onProviderChange auto-fills base_url only when the current URL is a default — but the comparison is wrong-ish

const isDefaultUrl = Object.values(PROVIDER_DEFAULTS).includes(baseUrl);
if (next && (!baseUrl || isDefaultUrl)) { setValue("base_url", next); }

This overwrites the URL when the user has selected a known default, even if they intentionally put that URL in for a different provider (e.g. litellm pointed at https://api.openai.com/v1). Better: only autofill when the user is switching from a provider whose default they're currently using, or drop the auto-fill entirely and let users type. Smaller blast radius.

3. (ApiKeysCard.tsx ~line 175) useApiKeys().create returns the plaintext, but the useApiKeys hook never re-reads it on remount
The plaintext is captured in ApiKeysCard via setCreated(result). If the user dismisses the CreatedKeyDialog and the parent component remounts (e.g. navigating away and back to /settings), the plaintext is gone from React state — correct, intentional. But if the user submits twice quickly (double-click on "Create key"), createMutation will fire twice and you'll get two distinct plaintexts. The disabled guard on the submit button is missing. Consider disabled={isCreating} on the submit button and on the per-row revoke button.

4. (SettingsPage.tsx ~line 80) "Sign out" button is in the page header and visible to every visitor
That's fine if the cookie auth gate already prevented reaching this page without a session. With oauth2-proxy in front, an unauthenticated visitor should never see /settings — they'd be redirected to /oauth2/start first. Confirm: is the page behind a route guard, or only guarded by the 401 interceptor? If the latter, the visible "Sign out" button is redundant (the interceptor would already redirect on a 401) and exists primarily for in-session use, which is fine but worth a comment.

5. (useApiKeys.ts ~line 35) useApiKeys exposes revoke but the ApiKeysCard revocation flow bypasses the isRevoking guard
Looking at ApiKeysCard.tsx, the Revoke button is disabled only when isRevoking is true (via the hasRevokeButton check), but the AlertDialogAction is not disabled during the in-flight revoke. A double-click on "Confirm revoke" can fire two DELETE requests. Same as #3.

6. (oauth2Redirect.ts) isOnAuthPage() does a substring/prefix match on pathname only

const pathname = href.includes("://") ? new URL(href).pathname : window.location.pathname;

The ternary is needed because window.location.href is sometimes a relative path in jsdom, but on a real browser it's always absolute. The branch is dead code in production. Not a bug — just dead-path complexity. Suggest a single new URL(window.location.href).pathname with a try/catch.

7. (axiosInstances.test.ts) vi.resetModules() in afterEach could mask module-load order bugs
Each test re-imports the axios instance module, which means cache state (interceptors, defaults) is recreated per test. Good for isolation, but if one test installs a handler and another doesn't re-import, you can get false positives. The pattern is fine; just note it's intentional.

8. (nit) LLM_PROVIDERS list dropped anthropic, google, mistral, local, ollama — replaced with openrouter, litellm, custom
The previous list and the new list are nearly disjoint. This is a breaking change disguised as a refactor. The README mentions this implicitly ("'anthropic' / 'openai' / 'google' / 'mistral' / 'local' / 'ollama'"), but a real user on anthropic will silently lose their config on upgrade. Either:

• preserve the old list with a server-side default mapping, or
• explicitly call this out as a breaking change in the PR description (the "Breaking changes" section only mentions localStorage → backend, not the provider list).

9. (nit) useSettingsStore.ts migratePersisted is a no-op when the blob was already migrated
If a user has had the app for a while and has a clean state, migratePersisted strips nothing. That's fine. But the function has the same signature as migrateFontFamily and could be merged into a single migrate(persisted) function. Not blocking.

10. (nit) ApiKeysCard.tsx formatDate uses toISOString() which is UTC; the UI then shows UTC dates to a user in, say, Asia/Ho_Chi_Minh
For a settings page, locale-aware formatting (toLocaleString()) is usually what users expect. UTC strings in a UTC+7 timezone = "weird hours on the wrong day" UX. Minor.

Score

7.5 / 10

Why not higher: Finding #8 (breaking provider list) needs a clarifying line in the PR description. Finding #3 + #5 (double-click guards missing) are real bugs that the test suite doesn't cover. Finding #1 (dual ownership of form state) is a mild design smell.

Why not lower: The hexagonal split is textbook, the plaintext flow is correctly one-shot, the migration is safe, the auth guard is genuine, and the tests are real (not just coverage theater). The CI is green, the docs are updated, and the dependency on the backend PR is explicit. The breaking change is documented at the category level — just missing the provider-list detail.

Suggested improvements

1. Add disabled={isCreating} to the API key create button and disabled={isRevoking} to the confirmation action.
2. Add a test for the "two rapid clicks don't fire two POSTs" case on both LLM upsert and API key create.
3. Either (a) extend LLM_PROVIDERS to keep the legacy four and only add the new ones, or (b) add a line in the PR description: "Provider values are now backend-defined; the dropdown lists 4 supported providers. If you were on anthropic or google, choose the closest equivalent (openai/custom) and re-enter your API key."
4. Locale-format the created_at / last_used_at timestamps in ApiKeysCard.
5. Drop the href.includes("://") ternary in isOnAuthPage() — production is always absolute.
6. Optional: write a single integration test that does POST /api/v1/api-keys → assert plaintext present → assert GET /api/v1/api-keys does NOT return plaintext — this is the contract worth pinning down so a server-side refactor doesn't accidentally regress the one-shot exposure.

Otherwise: solid PR. The dependency on composable-agents#39 and the flux ticket 4 are explicit, which is the right way to gate a multi-repo auth change.

[Kaiohz]

Code review — PR #24 (f6fb0607, head)

Scope: +1969 / -155 across 33 files. Hexagonal split respected (domain ports → infrastructure adapters → application hooks → views). 780 unit tests pass (+34 new). Cookie-based auth via oauth2-proxy + 401 redirect + per-user LLM settings + per-user API keys + plaintext one-shot exposure.

Reviewed at: f6fb06070170897e93ec58b289a0e171315a56dc (follow-up fix: McpServerGrid crash when servers is not an array included).

---

✅ Positives

• Hexagonal boundaries are clean. ISettingsPort / IApiKeyPort declared in domain/ports/, implemented by settingsApi / apiKeyApi in infrastructure/, consumed by useLlmSettings / useApiKeys in application/hooks/. Views (LlmSettingsCard, ApiKeysCard) only touch the hooks. Matches the rest of the codebase.
• Plaintext key flow is correct. CreatedApiKey.plaintext is carried once from the adapter, surfaced by CreatedKeyDialog (copy + "you won't see this again" warning), never persisted in Zustand or localStorage. The contract is honest about the one-shot exposure.
• Migration is safe. New migratePersisted strips the legacy llmProvider / apiKey from the persisted Zustand blob before re-hydrating, so existing users don't carry stale fields. Defensive comment is appreciated.
• 401 redirect guard is genuine. isOnAuthPage() matches the pathname against both /oauth2/start and /oauth2/sign_out, so a 401 fired during the auth round-trip won't loop. Tested explicitly in axiosInstances.test.ts.
• withCredentials is consistent across REST and SSE. Every axios instance has it, plus the fetchEventSource call in chatApi sets credentials: "include". No channel silently drops the cookie.
• AlertDialog via Radix is the right primitive (focus trap, Escape, ARIA). The confirm-revoke UX is a real win over window.confirm.
• Tests are real, not theatrical. axiosInstances.test.ts intercepts the rejected handler and asserts on hrefSetter; the loop-guard test overrides window.location to verify no redirect fires. Hook tests are minimal contracts.
• Form re-sync in LlmSettingsCard is thoughtful. Deps list (settingsProvider, settingsBaseUrl, settingsUpdatedAt) — explicitly chosen over the settings object identity, with a comment explaining why. The kind of thing that gets done wrong by default.
• Dependency on composable-agents#39 and flux ticket 4 is explicit in the PR body. Right way to gate a multi-repo auth change.

---

🔎 Findings

1. (Bug, LlmSettingsCard.tsx ~line 65–85) useEffect form reset duplicates defaultValues ownership
The form is initialised with defaultValues: { provider: "openai", base_url: …openai, api_key: "" } and an effect that calls reset(...) whenever the backend's provider / base_url / updated_at change. If the backend returns provider: "openrouter" and the user submits before the effect runs (rare but possible during a refetch), the setValue("provider", value) in onProviderChange and the reset() in the effect can fight. Two ways to fix:

• Remove the effect entirely and set defaultValues from settings ?? DEFAULTS. RHF won't reset from external state unless told to.
• Keep the effect but drop the setValue in onProviderChange (use the effect as the single source of truth).

2. (Bug, LlmSettingsCard.tsx ~line 95–106) onProviderChange overwrites a user-typed base_url

const isDefaultUrl = Object.values(PROVIDER_DEFAULTS).includes(baseUrl);
if (next && (!baseUrl || isDefaultUrl)) { setValue("base_url", next); }

A user pointing litellm at https://api.openai.com/v1 (intentional proxy) loses the URL the moment they switch the provider dropdown. Either drop the auto-fill entirely, or only autofill when switching from the same provider whose default they're currently using.

3. (Bug, ApiKeysCard.tsx) Missing disabled guards → double-click can fire two POSTs / DELETEs

• Create button: should be disabled={isCreating} (not just visually).
• AlertDialogAction "Confirm revoke": should be disabled={isRevoking}.
  Same class of bug as in composable-agents reviews: the test suite doesn't cover the rapid-click path, so the regression won't show up in CI.

4. (Bug / contract, chatApi.ts ~line 115) SSE 401 detection is fragile

onerror(err) {
  if (err && typeof err === "object" && "status" in err && err.status === 401) {
    redirectToSignIn();
  }
  onError(err instanceof Error ? err : new Error(String(err)));
  throw err;
},

fetchEventSource doesn't normalize err — status may live on err.status, err.response?.status, or not be set at all (network errors). The 401 path will be missed on transport errors. Either introspect err more carefully or use a fetch wrapper that exposes the response status. Worth a test that simulates a 401 from the SSE endpoint.

5. (Bug / contract, oauth2Redirect.ts) Dead branch in isOnAuthPage()

const pathname = href.includes("://")
  ? new URL(href).pathname
  : (window.location.pathname ?? "");

In production, window.location.href is always absolute — the ternary only exists for jsdom. Replace with new URL(window.location.href).pathname inside a try/catch. The current code works but the dead path is misleading on read.

6. (Breaking change, undocumented) LLM_PROVIDERS list was silently rewritten
Old list (in the diff of SettingsPage.tsx): anthropic, openai, google, mistral, local. New list: openai, openrouter, litellm, custom. The PR description's "Breaking changes" section only mentions localStorage → backend, not the provider list. A user on anthropic or google will silently lose their config on upgrade.

• Either preserve the old list (backend can map), or
• Add a line in the PR description: "Provider values are now backend-defined; the dropdown lists 4 supported providers. Re-enter your key on first save."

7. (UX, ApiKeysCard.tsx formatDate) UTC dates shown to a non-UTC user

return d.toISOString().slice(0, 19).replace("T", " ");

Hardcoded UTC. A user in Asia/Ho_Chi_Minh will see "weird hours on the wrong day". Use toLocaleString() (or Intl.DateTimeFormat) for human display.

8. (Defense-in-depth, useSettingsStore.ts migratePersisted)
Works, but the prefix-less underscore for destructured names (_llm, _key) is the only "we explicitly don't use these" signal. A short JSDoc would make it clearer to future readers why those two keys are filtered.

9. (Test coverage gap) No integration test for the "plaintext shown once, never on subsequent GET" contract
The most important invariant of this PR — that the backend never returns plaintext after creation — has no test pinning it down. A server-side refactor that accidentally starts returning plaintext on GET /api/v1/api-keys would not be caught by any current test. Add a contract test (mock-level is fine) that asserts list() result has no plaintext field.

10. (Test coverage gap) axiosInstances.test.ts vi.resetModules() in afterEach
Intentional for isolation, but worth a one-line comment so a future reader doesn't think it's a bug. Each test re-imports the axios module → state is fresh per test. Note it as such.

---

Score

7.5 / 10

Why not higher: Finding #6 (breaking provider list) needs a clarifying line in the PR description. Findings #3 (double-click guards) and #4 (SSE 401 detection) are real bugs the test suite doesn't cover. Finding #1 (dual form ownership) is a mild design smell that will bite the next refactor.

Why not lower: The hexagonal split is textbook. The plaintext flow is correctly one-shot. The migration is safe. The auth guard is genuine. The tests are real (not coverage theater). CI is green, docs are updated, the cross-repo dependencies are explicit, and the head fix (McpServerGrid Array.isArray guard) is the right shape.

---

Suggested follow-ups (in priority order)

1. Add disabled={isCreating} / disabled={isRevoking} to the create + confirm-revoke buttons, plus a test for "two rapid clicks don't fire two POSTs / DELETEs". Same bug class as the other double-click regressions across SoluDevTech repos — worth a one-line guard everywhere.
2. Document the provider-list breaking change in the PR description (one sentence is enough).
3. Add a contract test that asserts apiKeyApi.list() result has no plaintext field. Pins down the one-shot exposure invariant.
4. Fix the SSE 401 detection so 401s on the streaming endpoint actually trigger the redirect. Add a test.
5. Locale-format the API key timestamps (toLocaleString).
6. Drop the auto-fill base_url logic in onProviderChange or narrow it to the same-provider case. Lower priority — the user can re-type the URL.

Otherwise: solid PR. The dependency on composable-agents#39 and flux ticket 4 is the right way to gate a multi-repo auth change, and the migration of localStorage → backend settings is handled defensively.

[Kaiohz]

Review — feat: credentials, per-user LLM settings, API keys management, oauth2-proxy integration

Score: 8.5 / 10

Beau boulot sur cette PR. L'architecture hexagonale est respectée de bout en bout, le port → adapter → hook → component est appliqué de manière cohérente, et la sécurité du flux credentials est prise au sérieux (plaintext-once, masquage, cookie-only, redirect 401 loop-guarded). Les tests unitaires sur les hooks et adapters sont solides (119 fichiers / 780 tests verts côté CI). Quelques points d'amélioration à regarder avant merge.

---

✅ Points forts

• Architecture hexagonale propre : entités/ports/adapters séparés, IApiKeyPort / ISettingsPort / ICurrentUserPort clairement découpés, dépendances bien orientées (domain ← application ← infrastructure).
• Credentials bien gérés : withCredentials: true sur les 3 axios + credentials: "include" sur le SSE, masquage serveur, plaintext-once côté API key avec copy + warning explicite, aucun secret en localStorage après migration.
• Migration de store : migratePersisted() dans useSettingsStore strip proprement les llmProvider/apiKey legacy → évite que d'anciens secrets restent dans le localStorage des users qui avaient déjà sauvegardé.
• Loop guard sur 401 : isOnAuthPage() empêche la boucle infinie sur /oauth2/start.
• Détails UX soignés : data-od-id partout, AlertDialog Radix, CreatedKeyDialog non-dismissable tant que l'utilisateur n'a pas confirmé (Done ferme), useEffect qui dépend de updated_at au lieu de l'identité de l'objet (évite la reset-loop documentée en commentaire).
• Pure functions testables : getDisplayName / getInitials dans le domain entity, simples à unit-tester (et c'est ce qu'on devrait faire, cf. ci-dessous).
• Test coverage solide : hooks (useApiKeys, useLlmSettings) + adapters (axios, chatApi) + page (SettingsPage).

---

🔧 Améliorations suggérées

1. (Bloquant mineur) Tests manquants pour les pure functions du domain

src/domain/entities/auth/currentUser.ts exporte getDisplayName() et getInitials(). Ce sont des pure functions avec une matrice de cas non-triviale (4 niveaux de fallback pour le display name, split whitespace, fallback caractère unique, cas null/undefined/""). Aucun test direct — seul useCurrentUser est testé, et ces helpers sont utilisés dans 2 composants (Sidebar, ChatMessage). Vu que la fonction n'a aucune dépendance, c'est exactement le genre de truc qu'il faut unit-tester exhaustivement.

Cas à couvrir (a minima) :

• getDisplayName(null) === ""
• getDisplayName({userId:"u1"}) === "u1"
• getDisplayName({userId:"u1", name:" Jane "}) === "Jane" (trim)
• getDisplayName({userId:"u1", username:"jdoe", name:""}) === "jdoe" (name vide → username)
• getDisplayName({userId:"u1", email:"alice@x.com"}) === "alice"
• getDisplayName({userId:"u1", name:"Jane Doe"}) === "Jane Doe" (espace interne préservé)
• getInitials("Jane Doe") === "JD"
• getInitials("jane") === "J"
• getInitials(" Jane Doe Smith ") === "JD" (multi-whitespace)
• getInitials("") === " "
• getInitials(" ") === " "

2. (Bloquant mineur) ChatMessage.tsx — fallback hardcodé "Y"

const userInitial = getDisplayName(profile).charAt(0).toUpperCase() || "Y";

Le || "Y" est probablement un héritage du maquette (avatar "Y" pour "Yohan"). Maintenant que le username vient du backend, ce fallback est trompeur : si le user charge la page avant useCurrentUser (premier render), il verra "Y" → identité d'un autre user affichée par erreur. Trois options :

• Afficher " " (comme getInitials("") retourne déjà " ") pour signaler "pas encore chargé".
• Afficher un spinner/squelette discret.
• Au minimum, documenter le fallback en commentaire (c'est volontaire mais ambigu).

3. (Important) Sidebar.tsx — pas de gestion d'erreur si /me échoue

const { profile, isLoading } = useCurrentUser();

Aucune branche sur isError. Si le backend renvoie 500 sur /api/v1/users/me, l'utilisateur reste bloqué avec initials = "…" et label = "Loading…" indéfiniment. C'est moins critique que la page Settings (qui affiche isError ailleurs), mais ça mérite au minimum :

• Un fallback label = "Guest" / initials = "?" en cas d'erreur,
• Ou un retry manuel discret (bouton "retry" à côté de l'avatar).

4. (Important) SSE 401 — err.status n'existe pas sur les erreurs fetch-event-source

onerror(err) {
  if (err && typeof err === "object" && "status" in err && err.status === 401) {
    redirectToSignIn();
  }
  ...
}

@microsoft/fetch-event-source ne propage pas la status HTTP dans son callback onerror par défaut — err est un Error sans .status. La branche est donc structurellement inatteignable avec cette lib. Pour gérer les 401 sur le SSE proprement, il faut soit :

• Passer par fetch directement et inspecter response.status dans un wrapper,
• Hooker le onopen qui reçoit le Response (et lire .status),
• Ou surcharger onerror pour intercepter TypeError/network errors et rediriger sur une heuristique (acceptable mais fragile).

À minima, ajouter un commentaire qui documente que cette branche n'est pas déclenchée aujourd'hui et pourquoi, ou corriger avec un wrapper sur onopen.

5. (Important) LlmSettingsCard — le form se reset après chaque updated_at, mais le password field reste vide

useEffect(() => {
  const provider = settingsProvider ?? "openai";
  reset({
    provider,
    base_url: settingsBaseUrl ?? PROVIDER_DEFAULTS.openai,
    api_key: "",   // toujours vide
  });
  setProviderDraft(provider);
}, [settingsProvider, settingsBaseUrl, settingsUpdatedAt, reset]);

C'est cohérent (on ne veut pas pré-remplir le champ password), mais ça veut dire qu'à chaque updated_at change (ex: refetch manuel, autre onglet) le form se vide, ce qui est déroutant si l'user est en train de taper sa clé. Le commentaire est bon mais le comportement pourrait surprendre. Suggestion : ne trigger le reset que si !isUpserting && !isRemoving (pour ne pas casser un save en cours), ou ajouter un bouton "Discard changes" explicite.

6. (Mineur) oauth2Redirect.ts — SIGN_OUT_PATH exporté mais inutilisé

const SIGN_IN_PATH = "/oauth2/start";
const SIGN_OUT_PATH = "/oauth2/sign_out";

SIGN_OUT_PATH est déclaré mais redirectToSignOut() hardcode "/oauth2/sign_out" en string literal. Soit l'utiliser, soit le retirer pour éviter le doublon.

7. (Mineur) mcpAxiosInstance.ts / ragAxiosInstance.ts — duplication de l'interceptor 401

Le bloc if (error.response?.status === 401) { redirectToSignIn(); } + wrapped.status est dupliqué 3 fois (apiClient, mcpApiClient, ragApiClient). Extraire un helper :

// infrastructure/api/createAuthInterceptor.ts
export function authErrorInterceptor(error: any) {
  if (error?.response?.status === 401) redirectToSignIn();
  if (error?.response) {
    const detail = error.response.data?.detail || error.message;
    const wrapped = new Error(detail) as Error & { status?: number };
    wrapped.status = error.response.status;
    return Promise.reject(wrapped);
  }
  return Promise.reject(error);
}

Et l'utiliser : apiClient.interceptors.response.use(r => r, authErrorInterceptor). Évite la dérive entre les 3 instances (l'apiClient original n'a même pas le wrapped.status ajouté dans la PR — cf. point suivant).

8. (Bug) apiClient interceptor 401 n'enveloppe pas l'erreur

apiClient.interceptors.response.use(
  (response) => response,
  (error) => {
    if (error.response?.status === 401) {
      redirectToSignIn();
    }
    if (error.response) {
      const detail = error.response.data?.detail || error.message;
      const wrapped = new Error(detail) as Error & { status?: number };
      return Promise.reject(wrapped);   // <-- pas de .status = error.response.status
    }
    ...

Tandis que mcpApiClient et ragApiClient ont bien été mis à jour pour ajouter wrapped.status. Inconsistance : si un consumer dépend de err.status sur apiClient (cf. le SSE dans chatApi, ou les hooks), ça ne marche pas. À harmoniser.

9. (Mineur) useSettingsStore — migratePersisted peut être plus strict

function migratePersisted(state): Partial<SettingsState> {
  const { llmProvider: _llm, apiKey: _key, ...rest } = state;
  return rest as Partial<SettingsState>;
}

Bien — strip des champs legacy. Mais aucune log pour dire "j'ai migré X users de l'ancien format". Recommandé : console.info (dev only) ou telemetry event quand state.llmProvider !== undefined est détecté à la lecture, pour mesurer combien de clients ont encore des données legacy. Donne de la visibilité sur la dette technique.

10. (Doc) README mentionne localStorage comme storage LLM settings alors que la PR le supprime

Le diff README semble OK (il mentionne bien PUT /api/v1/settings/llm maintenant), mais il y a encore une ligne dans ## Settings qui dit "localStorageonly" ? À vérifier dans la version finale. Sinon tout est à jour.

11. (Mineur) getInitials retourne " " (un espace) au lieu de "" — un peu surprenant

if (!displayName) return " ";

Le commentaire explique que c'est pour le "loading state". Mais retourner un espace dans un avatar vide crée un caractère de hauteur visible. Préférer un retour explicite typé ("") et laisser le composant choisir son placeholder (getInitials reste pure, l'UI décide du fallback visuel). Test plus simple aussi.

12. (Mineur) Pas de test pour oauth2Redirect

C'est de la pure function avec un seul effet de bord (window.location.href = ...). Mockable trivialement avec vi.stubGlobal("window", {...}). Tests suggérés :

• Redirect vers /oauth2/start?rd=<encoded current url> quand on est sur /foo.
• No-op quand on est déjà sur /oauth2/start.
• No-op quand on est déjà sur /oauth2/sign_out.
• rd correctement URL-encodé (avec ?, &, # dans l'URL source).

---

📋 Récap

Catégorie	Statut
Architecture hexagonale	✅ Propre
Sécurité credentials (plaintext, cookie, masquage)	✅ Bien
UX (data-od-id, dialogs, copy+warn)	✅ Soigné
Migration de store	✅ Bien
Tests hooks + adapters	✅ Solide
Tests domain pure functions	❌ Manquant
Tests oauth2Redirect	❌ Manquant
SSE 401 redirect	⚠️ Branche inatteignable (fetch-event-source)
Inconsistance interceptors (apiClient.status)	🐛 Bug
Duplication interceptors 401	🔧 Refactor suggéré
Gestion erreur Sidebar /me fail	🔧 À ajouter
ChatMessage fallback "Y"	🔧 À clarifier
LlmSettingsCard reset on updated_at	🔧 UX à valider
SIGN_OUT_PATH inutilisé	🧹 Cleanup

---

Recommandation : commenter sans approbation sur les points 4 (SSE 401 inatteignable), 8 (apiClient inconsistent), et 1 (tests domain). Les autres sont discutables en post-merge mais idéalement avant.

(Note: en tant qu'auteur de la PR, je ne peux pas me mettre moi-même en Request Changes via l'API — je laisse ces points en commentaire pour décision.)

PR globalement très bien foutue, le découpage est propre et la doc README exhaustive. 👏