✨(frontend) add matrix client and oidc auth#8
Open
MarcWadai wants to merge 9 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request integrates Matrix chat client support into the Hub frontend through OIDC-based user provisioning, persistent storage, client initialization, and driver-level chat operations. The changes add Matrix SDK dependencies, define authentication and session types, implement localStorage persistence, create a complete OIDC→Matrix login flow as a React hook, initialize a configured Matrix client with IndexedDB and crypto support, introduce a MatrixDriver implementation with mock-backed endpoints, and wire the new architecture into the app context and Auth component. ChangesMatrix Chat Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes This PR introduces a substantial new feature spanning authentication, persistence, client lifecycle, and driver architecture across many files. The OIDC→Matrix flow and hook logic are dense (high complexity), the MatrixDriver mock implementations are repetitive but each require validation, and the wiring across Auth, Config, and global declarations demands careful integration review. The auth utility logic with multiple fallback paths and error handling, combined with the state management in the provisioning hook, adds significant logic density. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx`:
- Around line 23-31: The query is keyed by the full chatUser object which can
change reference and cause unnecessary re-initializations; update the
useQuery/queryKey (the array currently ['matrixClient', chatUser]) to use stable
primitive identifiers from chatUser such as chatUser.mxId, chatUser.deviceId and
chatUser.homeserverUrl (or whichever stable fields exist) so the key becomes
['matrixClient', chatUser.mxId, chatUser.deviceId, chatUser.homeserverUrl]; keep
enabled: !!chatUser and leave queryFn calling initClient(chatUser!) unchanged
aside from ensuring those primitive fields are present/validated before running.
- Around line 33-45: The useEffect that calls startClient(mx) must also stop the
background sync when the component unmounts or mx changes: modify the effect so
it captures any cleanup/disposer returned by startClient (or, if no disposer
exists, call a stop function such as stopClient(mx) / mx.stop()) and return a
cleanup function that checks mx && mx.clientRunning and invokes that
stop/dispose; update the startClient invocation to save a possible returned
disposer (const dispose = await startClient(mx)) or call stopClient(mx) inside
the returned cleanup to ensure syncing is stopped on unmount/switch.
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx`:
- Around line 67-70: The code is incorrectly reading the homeserver from React
state immediately after calling setHomeserver, leading to empty/stale values;
change calls to use the resolved homeserver value returned from
fetchHomeserverForEmail (use hs.base_url) when calling getOIDCAuthUrl and whoami
instead of using the homeserver state, and in the OIDC callback handler use
oidcResult.homeserverUrl (from the callback result) rather than the homeserver
state; update references around fetchHomeserverForEmail, setHomeserver,
getOIDCAuthUrl, whoami and the OIDC callback handler to pass and consume the
resolved URL directly.
- Around line 31-56: The code sets setIsProcessingCallback(true) but never
guarantees it will be reset if an exception is thrown; wrap the async flow in a
try/finally so setIsProcessingCallback(false) always runs. Concretely, in the
useMatrixUser callback that calls completeOidcLogin and getUserIdFromAccessToken
and then uses matrixUserStore.saveUser / persistOIDC / router.replace, move the
logic into a try block and put setIsProcessingCallback(false) in the finally
block (rethrow any caught errors or avoid catching so the original error
propagates); keep references to completeOidcLogin, getUserIdFromAccessToken,
matrixUserStore.saveUser, matrixUserStore.persistOIDC, and router.replace so the
fix is localized.
- Around line 63-65: In useMatrixUser (the hook assigning email), remove the
hardcoded personal address and instead read a non-personal development-only
environment variable (e.g., process.env.DEV_MATRIX_EMAIL) when
process.env.NODE_ENV === 'development'; if that var is absent, fall back to a
neutral placeholder or leave undefined so auth flow uses normal input. Update
the assignment of the email variable and any related tests to use the new env
var rather than the fixed 'marc3@tchap.beta.gouv.fr'.
- Line 32: The completion call hardcodes the OIDC responseMode ('fragment')
which can mismatch the mode chosen in getOIDCAuthUrl; change the call to
completeOidcLogin to use the same responseMode that was selected for the
authorization request (e.g., pass the responseMode variable/flag returned or
determined by getOIDCAuthUrl instead of the literal 'fragment'). Locate where
getOIDCAuthUrl determines/returns the mode and thread that value into the
completeOidcLogin call in useMatrixUser (referencing completeOidcLogin and
getOIDCAuthUrl) so the authorization and completion use an identical
responseMode.
In `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts`:
- Around line 2-6: Replace the deep import of createClient,
IndexedDBCryptoStore, IndexedDBStore, and MatrixClient from
'matrix-js-sdk/lib/matrix' with imports from the package entry 'matrix-js-sdk'
(update both src/frontend/apps/hub/src/features/matrix/initMatrix.ts and
src/frontend/apps/hub/src/features/matrix/utils/auth.ts), and change any uses of
global.indexedDB/global.localStorage to globalThis.indexedDB and
globalThis.localStorage respectively to ensure correct browser runtime behavior.
In `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts`:
- Around line 24-28: The persistOIDC function currently writes OIDC secrets
(especially idToken) into localStorage (OIDC_CLIENT_ID, OIDC_ISSUER,
OIDC_ID_TOKEN), which is insecure; remove persisting of idToken to localStorage
and replace this behavior with a safer approach: stop calling
localStorage.setItem for OIDC_ID_TOKEN inside persistOIDC, instead hand the
idToken to a secure storage flow (e.g., an in-memory auth store or a backend-set
secure, HttpOnly SameSite cookie via an authService endpoint) and only keep
non-sensitive metadata (if needed) or fetch issuer/clientId from a secure
source; update references to persistOIDC to use the authService/memory store and
ensure constants OIDC_CLIENT_ID, OIDC_ISSUER, OIDC_ID_TOKEN are no longer used
to store secrets in localStorage.
- Around line 8-11: The getUser() implementation uses JSON.parse on data from
localStorage and can throw on corrupted input; wrap the parse in a try/catch
inside getUser(), returning null if parsing fails and also call
localStorage.removeItem(MATRIX_USER_KEY) to clear the bad entry so future
bootstraps won't crash; keep the same return shape (MatrixUserInterface | null)
and only remove the key on catch.
In `@src/frontend/apps/hub/src/features/matrix/types.ts`:
- Line 3: Replace the unsupported internal import of OidcClientConfig in
types.ts: stop importing from 'matrix-js-sdk/src/matrix' and instead import the
public symbol from the documented subpath 'matrix-js-sdk/lib/oidc'; update the
import statement that references OidcClientConfig so it pulls from
'matrix-js-sdk/lib/oidc' (keeping the same exported identifier OidcClientConfig)
to rely on the supported public API.
In `@src/frontend/apps/hub/src/features/matrix/utils/auth.ts`:
- Around line 84-116: fetchDelegatedAuthMetadata can return undefined but the
code uses delegatedAuthConfig! when calling registerOidcClient and
generateOidcAuthorizationUrl; add an early guard after fetching (in this
function in auth.ts) to check if delegatedAuthConfig is undefined and handle it
(e.g., log an error via console/processLogger and either throw a clear Error or
return a safe fallback flow) so you never call registerOidcClient or
generateOidcAuthorizationUrl with a null/undefined metadata; remove the non-null
assertions and rely on the validated delegatedAuthConfig for subsequent calls.
- Around line 126-128: Replace the direct MatrixClient instantiation used to
fetch auth metadata by calling createClient(...) so internal defaults are
applied: change the temp client creation from "new MatrixClient({ baseUrl:
preferredHomeserverUrl })" to use createClient({ baseUrl: preferredHomeserverUrl
}) before invoking tempClient.getAuthMetadata(), keeping delegatedAuthentication
and tempClient identifiers intact and ensuring any needed imports for
createClient are added.
In `@src/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts`:
- Around line 14-16: The code currently concatenates the raw email into the
infoUrl and calls fetch(randomHomeServer.base_url + infoUrl + email); update
this to URL-encode the email query parameter before building the URL (e.g., use
encodeURIComponent(email)) so special characters like +, &, ? do not break the
request; construct the final URL from randomHomeServer.base_url, infoUrl, and
the encoded email and pass that to fetch.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5054935c-902e-4931-90a4-78ace5d86319
⛔ Files ignored due to path filters (1)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
src/frontend/apps/hub/package.jsonsrc/frontend/apps/hub/src/features/auth/Auth.tsxsrc/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsxsrc/frontend/apps/hub/src/features/matrix/config.tssrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixClients.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsxsrc/frontend/apps/hub/src/features/matrix/initMatrix.tssrc/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.tssrc/frontend/apps/hub/src/features/matrix/types.tssrc/frontend/apps/hub/src/features/matrix/utils/auth.tssrc/frontend/apps/hub/src/features/matrix/utils/autodiscovery.tssrc/frontend/apps/hub/src/pages/_app.tsx
Add oidc flow to matrix OIDC compatible server, introduce matrix client provider and chatUser values
Update usage of sessionstorage for usematrixchat hooks
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/frontend/apps/hub/src/features/auth/Auth.tsx (1)
16-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
matrixUserStore.removeUser()executes after redirect — may not complete.
window.location.replace()navigates away immediately, so subsequent statements (removeUser(),posthog.reset()) may not execute reliably depending on browser behavior. Move cleanup logic before the redirect.🐛 Proposed fix: reorder cleanup before redirect
export const logout = () => { + matrixUserStore.removeUser(); + posthog.reset(); window.location.replace(new URL('logout/', baseApiUrl()).href); - matrixUserStore.removeUser(); - posthog.reset(); };🤖 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 `@src/frontend/apps/hub/src/features/auth/Auth.tsx` around lines 16 - 20, The logout function currently calls window.location.replace(...) before running cleanup so matrixUserStore.removeUser() and posthog.reset() may not run; reorder the operations in logout so you first call matrixUserStore.removeUser() and posthog.reset(), then call window.location.replace(new URL('logout/', baseApiUrl()).href) as the final step (ensure no early returns or async gaps before the replace).src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx (1)
23-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a fuller identity key for Matrix client caching.
Line 23 keys only by
mxId; withstaleTime: Infinity, this can reuse an old client when device/session context changes.Suggested fix
- queryKey: ['matrixClient', chatUser?.mxId], // Refetch when chatUser changes + queryKey: [ + "matrixClient", + chatUser?.mxId, + chatUser?.deviceId, + chatUser?.homeserverUrl, + ],🤖 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 `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx` around lines 23 - 31, The query key for the matrix client currently only uses chatUser?.mxId which, together with staleTime: Infinity, lets an old client persist across device/session changes; update the useQuery queryKey (the ['matrixClient', chatUser?.mxId] entry) to include full identity context used by initClient (e.g., chatUser?.deviceId, chatUser?.sessionId or chatUser?.accessToken as available) so the cache differentiates by device/session; keep enabled and staleTime as is but ensure queryKey mirrors the unique identity properties passed into initClient to force reinitialization when those change.
♻️ Duplicate comments (5)
src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx (1)
40-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
currentHomeserverSelectedmay benullwhen used in the OIDC callback branch.If the user returns from the OIDC flow but
sessionStoragewas cleared (e.g., browser settings, incognito mode),currentHomeserverSelectedwill benull. This is passed togetUserIdFromAccessTokenand stored ashomeserverUrl, causing downstream failures. Consider falling back tooidcResult.homeserverUrl(returned bycompleteOidcLoginper the upstream contract).🛡️ Proposed fix: use oidcResult.homeserverUrl as fallback
if (code && state) { console.log('**** Processing OIDC callback'); setIsProcessingCallback(true); const responseMode = sessionStorage.getItem(OIDC_RESPONSE_MODE) || 'fragment'; const oidcResult = await completeOidcLogin({ code, state }, responseMode); + const homeserverUrl = currentHomeserverSelected || oidcResult.homeserverUrl; const { user_id: userId, device_id: deviceId, is_guest: isGuest, - } = await getUserIdFromAccessToken(oidcResult.accessToken, currentHomeserverSelected); + } = await getUserIdFromAccessToken(oidcResult.accessToken, homeserverUrl); const matrixUser = { - homeserverUrl: currentHomeserverSelected, + homeserverUrl, mxId: userId,🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around lines 40 - 43, currentHomeserverSelected can be null in the OIDC callback branch and is being passed to getUserIdFromAccessToken and used as homeserverUrl in the matrixUser object; update the logic in the OIDC callback handling (where oidcResult is obtained from completeOidcLogin) to use oidcResult.homeserverUrl as a fallback whenever currentHomeserverSelected is null or falsy, i.e., call getUserIdFromAccessToken(oidcResult.accessToken, currentHomeserverSelected || oidcResult.homeserverUrl) and set matrixUser.homeserverUrl = currentHomeserverSelected || oidcResult.homeserverUrl so downstream code always gets a valid homeserver URL.src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts (2)
10-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
JSON.parseingetUser()to prevent bootstrap crashes.Line 12 can throw on corrupted storage and break startup. Return
nulland clear the bad value on parse failure.Proposed fix
getUser(): MatrixUserInterface | null { const data = localStorage.getItem(MATRIX_USER_KEY); - return data ? JSON.parse(data) : null; + if (!data) return null; + try { + return JSON.parse(data); + } catch { + localStorage.removeItem(MATRIX_USER_KEY); + return null; + } }🤖 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 `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts` around lines 10 - 13, In getUser(), guard the JSON.parse call on the value retrieved with localStorage.getItem(MATRIX_USER_KEY) so a malformed value doesn't throw during bootstrap: attempt to parse inside a try/catch, return null on error and remove the bad key via localStorage.removeItem(MATRIX_USER_KEY); keep returning parsed object when parse succeeds and null when there's no value.
26-30:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not persist
idTokeninlocalStorage.Line 29 stores sensitive token material in
localStorage, which materially weakens session security in any XSS scenario. Keep only non-sensitive metadata here and move token handling to a safer channel (in-memory/session-bound backend flow).🤖 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 `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts` around lines 26 - 30, The persistOIDC function currently writes sensitive idToken to localStorage (OIDC_ID_TOKEN) which is insecure; remove the localStorage.setItem call for OIDC_ID_TOKEN in persistOIDC and only persist non-sensitive metadata (clientId/issuer), and instead route idToken handling to a safer channel (e.g., in-memory store, HttpOnly secure cookie set by backend, or session-scoped storage) and update any code that reads OIDC_ID_TOKEN to use the new secure mechanism.src/frontend/apps/hub/src/features/matrix/types.ts (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the documented OIDC export path for
OidcClientConfig.Line 3 still imports from
matrix-js-sdk/lib/matrix; this is a fragile subpath for OIDC types and can break on SDK packaging changes. Prefer the documented OIDC entrypoint.Proposed fix
-import { type OidcClientConfig } from "matrix-js-sdk/lib/matrix"; +import { type OidcClientConfig } from "matrix-js-sdk/lib/oidc";For the currently used matrix-js-sdk version, what is the documented public import path for OidcClientConfig: matrix-js-sdk/lib/oidc or matrix-js-sdk/lib/matrix?🤖 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 `@src/frontend/apps/hub/src/features/matrix/types.ts` at line 3, The import in types.ts uses a fragile internal path; change the OidcClientConfig import to the documented public OIDC entrypoint by replacing the current import from "matrix-js-sdk/lib/matrix" with the public OIDC module (use "matrix-js-sdk/lib/oidc") so OidcClientConfig is imported from the supported export; update the import statement that references OidcClientConfig accordingly.src/frontend/apps/hub/src/features/matrix/utils/auth.ts (1)
1-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImport
OidcClientConfigfrom the OIDC public entrypoint.Line 7 pulls
OidcClientConfigfrommatrix-js-sdk/lib/matrix; align this with the documented OIDC path to avoid SDK subpath breakage.Proposed fix
import { completeAuthorizationCodeGrant, createClient, generateOidcAuthorizationUrl, MatrixClient, MatrixError, - OidcClientConfig, registerOidcClient, } from "matrix-js-sdk/lib/matrix"; +import { type OidcClientConfig } from "matrix-js-sdk/lib/oidc";In matrix-js-sdk documentation for the version used in this repo, is OidcClientConfig officially exported from matrix-js-sdk/lib/oidc?🤖 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 `@src/frontend/apps/hub/src/features/matrix/utils/auth.ts` around lines 1 - 9, The import of OidcClientConfig in this file is coming from the internal subpath "matrix-js-sdk/lib/matrix"; update the import to pull OidcClientConfig from the documented OIDC public entrypoint (e.g., "matrix-js-sdk/lib/oidc" or the SDK's top-level export if available) so the type is stable; locate the import statement that includes OidcClientConfig (alongside createClient, registerOidcClient, generateOidcAuthorizationUrl, completeAuthorizationCodeGrant, MatrixClient, MatrixError) and change only the source for OidcClientConfig to the correct public module while leaving the other imports as-is.
🤖 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 `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx`:
- Line 25: The debug log in MatrixClientRoot.tsx prints the entire chatUser
object (console.log('**** initializing matrix client', chatUser)), which may
contain sensitive tokens; remove this console.log or replace it with a
non-sensitive message and/or log only safe fields (e.g., userId) and explicitly
omit or mask accessToken/refreshToken when debugging. Locate the console.log in
MatrixClientRoot (the matrix client initialization path) and either delete it or
change it to log a redacted object or a simple string like "initializing matrix
client for userId=<id>" using chatUser.userId, ensuring no credential properties
are included.
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx`:
- Line 28: Replace the invalid Web Storage call sessionStorage.get(...) with the
correct API sessionStorage.getItem(...) in the useMatrixUser hook; specifically
update the references that read the OIDC_HS key (e.g., the
currentHomeserverSelected assignment and the second occurrence later in the
hook) to use sessionStorage.getItem(OIDC_HS) so you don't trigger a TypeError at
runtime.
- Around line 20-21: The query key for useQuery currently includes the entire
user object (['useMatrixChatUser', user]) which causes unnecessary refetches;
update the queryKey to use a stable identifier such as user?.id or user?.email
(e.g., ['useMatrixChatUser', user?.id]) so the hook (useQuery in useMatrixUser)
only refetches when the identity changes rather than any property mutation.
- Around line 70-72: fetchHomeserverForEmail can return undefined but the code
uses a non-null assertion on hs (hs!.base_url) which will throw if lookup
failed; update the useMatrixUser hook to guard against undefined: check the
returned value from fetchHomeserverForEmail before reading base_url, handle the
failure path (e.g., skip setting currentHomeserverSelected, do not call
sessionStorage.setItem(OIDC_HS, ...), and surface/log an error or return early)
and ensure any downstream code that relies on currentHomeserverSelected handles
the no-homeserver case.
---
Outside diff comments:
In `@src/frontend/apps/hub/src/features/auth/Auth.tsx`:
- Around line 16-20: The logout function currently calls
window.location.replace(...) before running cleanup so
matrixUserStore.removeUser() and posthog.reset() may not run; reorder the
operations in logout so you first call matrixUserStore.removeUser() and
posthog.reset(), then call window.location.replace(new URL('logout/',
baseApiUrl()).href) as the final step (ensure no early returns or async gaps
before the replace).
In `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx`:
- Around line 23-31: The query key for the matrix client currently only uses
chatUser?.mxId which, together with staleTime: Infinity, lets an old client
persist across device/session changes; update the useQuery queryKey (the
['matrixClient', chatUser?.mxId] entry) to include full identity context used by
initClient (e.g., chatUser?.deviceId, chatUser?.sessionId or
chatUser?.accessToken as available) so the cache differentiates by
device/session; keep enabled and staleTime as is but ensure queryKey mirrors the
unique identity properties passed into initClient to force reinitialization when
those change.
---
Duplicate comments:
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx`:
- Around line 40-43: currentHomeserverSelected can be null in the OIDC callback
branch and is being passed to getUserIdFromAccessToken and used as homeserverUrl
in the matrixUser object; update the logic in the OIDC callback handling (where
oidcResult is obtained from completeOidcLogin) to use oidcResult.homeserverUrl
as a fallback whenever currentHomeserverSelected is null or falsy, i.e., call
getUserIdFromAccessToken(oidcResult.accessToken, currentHomeserverSelected ||
oidcResult.homeserverUrl) and set matrixUser.homeserverUrl =
currentHomeserverSelected || oidcResult.homeserverUrl so downstream code always
gets a valid homeserver URL.
In `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts`:
- Around line 10-13: In getUser(), guard the JSON.parse call on the value
retrieved with localStorage.getItem(MATRIX_USER_KEY) so a malformed value
doesn't throw during bootstrap: attempt to parse inside a try/catch, return null
on error and remove the bad key via localStorage.removeItem(MATRIX_USER_KEY);
keep returning parsed object when parse succeeds and null when there's no value.
- Around line 26-30: The persistOIDC function currently writes sensitive idToken
to localStorage (OIDC_ID_TOKEN) which is insecure; remove the
localStorage.setItem call for OIDC_ID_TOKEN in persistOIDC and only persist
non-sensitive metadata (clientId/issuer), and instead route idToken handling to
a safer channel (e.g., in-memory store, HttpOnly secure cookie set by backend,
or session-scoped storage) and update any code that reads OIDC_ID_TOKEN to use
the new secure mechanism.
In `@src/frontend/apps/hub/src/features/matrix/types.ts`:
- Line 3: The import in types.ts uses a fragile internal path; change the
OidcClientConfig import to the documented public OIDC entrypoint by replacing
the current import from "matrix-js-sdk/lib/matrix" with the public OIDC module
(use "matrix-js-sdk/lib/oidc") so OidcClientConfig is imported from the
supported export; update the import statement that references OidcClientConfig
accordingly.
In `@src/frontend/apps/hub/src/features/matrix/utils/auth.ts`:
- Around line 1-9: The import of OidcClientConfig in this file is coming from
the internal subpath "matrix-js-sdk/lib/matrix"; update the import to pull
OidcClientConfig from the documented OIDC public entrypoint (e.g.,
"matrix-js-sdk/lib/oidc" or the SDK's top-level export if available) so the type
is stable; locate the import statement that includes OidcClientConfig (alongside
createClient, registerOidcClient, generateOidcAuthorizationUrl,
completeAuthorizationCodeGrant, MatrixClient, MatrixError) and change only the
source for OidcClientConfig to the correct public module while leaving the other
imports as-is.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d59f909-c285-4c5a-9d88-b873eafdf2e8
📒 Files selected for processing (11)
src/frontend/apps/hub/src/features/auth/Auth.tsxsrc/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixClients.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsxsrc/frontend/apps/hub/src/features/matrix/initMatrix.tssrc/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.tssrc/frontend/apps/hub/src/features/matrix/types.tssrc/frontend/apps/hub/src/features/matrix/utils/auth.tssrc/frontend/apps/hub/src/features/matrix/utils/autodiscovery.tssrc/frontend/apps/hub/src/pages/_app.tsx
Comment on lines
+70
to
+72
| const hs = await fetchHomeserverForEmail(email); | ||
| currentHomeserverSelected = hs!.base_url; | ||
| sessionStorage.setItem(OIDC_HS, hs!.base_url); |
There was a problem hiding this comment.
Unsafe non-null assertion on hs — fetchHomeserverForEmail can return void.
Per the upstream contract (autodiscovery.ts:10-32), fetchHomeserverForEmail catches errors and returns undefined when the homeserver lookup fails. The non-null assertion hs!.base_url will throw if the fetch fails silently.
🛡️ Proposed fix: guard against undefined
if (!currentHomeserverSelected) {
const hs = await fetchHomeserverForEmail(email);
- currentHomeserverSelected = hs!.base_url;
- sessionStorage.setItem(OIDC_HS, hs!.base_url);
+ if (!hs) {
+ throw new Error('Could not discover homeserver for email');
+ }
+ currentHomeserverSelected = hs.base_url;
+ sessionStorage.setItem(OIDC_HS, hs.base_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
| const hs = await fetchHomeserverForEmail(email); | |
| currentHomeserverSelected = hs!.base_url; | |
| sessionStorage.setItem(OIDC_HS, hs!.base_url); | |
| if (!currentHomeserverSelected) { | |
| const hs = await fetchHomeserverForEmail(email); | |
| if (!hs) { | |
| throw new Error('Could not discover homeserver for email'); | |
| } | |
| currentHomeserverSelected = hs.base_url; | |
| sessionStorage.setItem(OIDC_HS, hs.base_url); | |
| } |
🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around
lines 70 - 72, fetchHomeserverForEmail can return undefined but the code uses a
non-null assertion on hs (hs!.base_url) which will throw if lookup failed;
update the useMatrixUser hook to guard against undefined: check the returned
value from fetchHomeserverForEmail before reading base_url, handle the failure
path (e.g., skip setting currentHomeserverSelected, do not call
sessionStorage.setItem(OIDC_HS, ...), and surface/log an error or return early)
and ensure any downstream code that relies on currentHomeserverSelected handles
the no-homeserver case.
Move from usequery to simple use effect, with hard coded response mode to query for our usage
MarcWadai
force-pushed
the
init-matrix-client
branch
from
May 28, 2026 10:39
bc08d77 to
1150f09
Compare
|
Size Change: +291 kB (+31.95%) 🚨 Total Size: 1.2 MB 📦 View Changed
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx (2)
42-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard homeserver autodiscovery result before dereferencing.
Lines 43–44 use
hs!even though discovery can returnundefined. Add an explicit guard and fail fast with a clear error.🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around lines 42 - 44, The code dereferences the result of fetchHomeserverForEmail(email) without checking for undefined; update useMatrixUser to guard the hs value (the variable returned by fetchHomeserverForEmail) before assigning currentHomeserverSelected and calling sessionStorage.setItem with OIDC_HS — if hs is undefined, fail fast by throwing or returning an explicit error/early exit with a clear message so you never use hs!.base_url.
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove hardcoded personal email from source.
Line 39 embeds a personal address, which is privacy-sensitive and can mis-route auth in dev. Use a neutral dev env var instead.
🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around lines 38 - 40, The hardcoded personal email in useMatrixUser.tsx (inside the development branch where email is set) must be removed; replace the literal 'marc3@tchap.beta.gouv.fr' with a neutral, documented dev environment variable (for example process.env.DEV_MATRIX_EMAIL) and fall back to an empty string or existing safe default if that env var is not set; update any related code/comments to reference the new env var and ensure the check remains inside the existing process.env.NODE_ENV === 'development' block to preserve dev-only behavior.src/frontend/apps/hub/src/features/matrix/types.ts (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the documented OIDC import path for
OidcClientConfig.Line 3 imports from
matrix-js-sdk/lib/matrix; for OIDC types this has previously been flagged as not the stable public path. Please switch to the documented OIDC entrypoint for this type.For matrix-js-sdk v41.6.0, what is the documented public import path for OidcClientConfig? Is importing it from "matrix-js-sdk/lib/matrix" supported, or should it come from "matrix-js-sdk/lib/oidc" (or top-level)?🤖 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 `@src/frontend/apps/hub/src/features/matrix/types.ts` at line 3, Replace the unstable import path for OidcClientConfig: instead of importing OidcClientConfig from "matrix-js-sdk/lib/matrix" update the import to the documented OIDC entrypoint "matrix-js-sdk/lib/oidc" (use the OidcClientConfig symbol) so the type comes from the public OIDC module (adjust the import in the file where OidcClientConfig is referenced, e.g., types.ts).src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts (2)
10-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
getUser()against corrupted storage.Line 12 can throw and break auth bootstrap if storage is malformed. Guard
JSON.parsewithtry/catch, clear the bad key, and returnnull.🤖 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 `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts` around lines 10 - 13, Harden getUser() by wrapping the JSON.parse call in a try/catch: call localStorage.getItem(MATRIX_USER_KEY) as before, attempt to JSON.parse(data) inside try and return the parsed MatrixUserInterface on success, but if parsing throws catch the error, call localStorage.removeItem(MATRIX_USER_KEY) to clear the corrupted entry, and return null; keep function signature getUser(): MatrixUserInterface | null and preserve behavior when data is null (return null) without throwing.
26-30:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid persisting OIDC ID tokens in
localStorage.Lines 27–29 persist sensitive token material in
localStorage, which is exposed to XSS and weakens session security. Keep only non-sensitive metadata client-side and move token handling to a safer channel (in-memory/session-backed server flow/HttpOnly cookie).🤖 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 `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts` around lines 26 - 30, The persistOIDC function currently writes sensitive id token material to localStorage; remove the call that stores OIDC_ID_TOKEN and stop persisting idToken there (leave clientId and tokenIssuer metadata if needed), and instead surface the idToken to a safer channel—e.g., keep it in-memory in the running Matrix client/session state or have the backend set a Secure, HttpOnly cookie or use a session-backed token flow; update persistOIDC (and any callers relying on OIDC_ID_TOKEN in localStorage) to stop reading/writing the id token from localStorage and use the in-memory/session or server cookie approach.src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx (3)
25-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove credential-bearing debug logging.
Line 25 logs the full
chatUserobject, which may include access/refresh tokens.Proposed fix
- console.log('**** initializing matrix client', chatUser); + console.log('Initializing matrix client', { + mxId: chatUser?.mxId, + homeserverUrl: chatUser?.homeserverUrl, + deviceId: chatUser?.deviceId, + });🤖 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 `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx` at line 25, In MatrixClientRoot.tsx remove the credential-bearing debug log that prints the entire chatUser object (the console.log('**** initializing matrix client', chatUser) statement); either delete the console.log or replace it with a non-sensitive message (e.g., "initializing matrix client" only) or log a sanitized subset (e.g., user id only) so access/refresh tokens are not emitted; update any tests or lints referencing this log if needed.
23-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen the query key to avoid stale client reuse.
Line 23 keys only on
mxId; withstaleTime: Infinity, client instances can be incorrectly reused across homeserver/device/session changes.Proposed fix
- queryKey: ['matrixClient', chatUser?.mxId], // Refetch when chatUser changes + queryKey: [ + 'matrixClient', + chatUser?.mxId, + chatUser?.homeserverUrl, + chatUser?.deviceId, + ],🤖 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 `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx` at line 23, The queryKey used in MatrixClientRoot is too weak (currently queryKey: ['matrixClient', chatUser?.mxId]) so with staleTime: Infinity a client can be reused across homeserver/device/session changes; strengthen the key to include all identifiers that define a distinct client (e.g., include chatUser?.mxId plus homeserver URL, deviceId or session token/instance id) so React Query treats different sessions as distinct; update the queryKey reference in MatrixClientRoot to include those additional properties (and keep staleTime behavior unchanged).
33-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd effect cleanup to stop Matrix sync on unmount/switch.
The startup effect starts sync but never stops it, which can leak background sync when
mxchanges or component unmounts.Proposed fix
useEffect(() => { - if (mx && !mx.clientRunning) { + if (!mx) { + return; + } + if (!mx.clientRunning) { console.log('***Start syncing'); startClient(mx) .then(() => { // we pass down the mx client to the driver // driver.setMatrixClient(mx!); }) .catch((error) => { console.error('Failed to start matrix client:', error); }); } + return () => { + if (mx.clientRunning) { + mx.stopClient(); + } + }; }, [mx]);🤖 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 `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx` around lines 33 - 45, The effect starts Matrix sync but never stops it; update the useEffect that references mx, clientRunning and startClient to return a cleanup function that stops the sync when mx changes or the component unmounts: when you call startClient(mx) capture any returned stopper/disposer (or call an explicit stop function such as stopClient(mx) or mx.stopSync()) and invoke it inside the cleanup; ensure the cleanup is guarded by the same mx/clientRunning checks so you don't call stop on an undefined client.src/frontend/apps/hub/src/features/matrix/initMatrix.ts (1)
1-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse public SDK imports and browser-safe globals.
matrix-js-sdk/lib/matrix(Line 6) is an internal/deep path, andglobal.*(Lines 13-14, 19) is brittle in browser runtime. Import frommatrix-js-sdkand switch toglobalThis.Proposed fix
import { createClient, IndexedDBCryptoStore, IndexedDBStore, MatrixClient, -} from "matrix-js-sdk/lib/matrix"; +} from "matrix-js-sdk"; @@ const indexedDBStore = new IndexedDBStore({ - indexedDB: global.indexedDB, - localStorage: global.localStorage, + indexedDB: globalThis.indexedDB, + localStorage: globalThis.localStorage, dbName: 'web-sync-store', }); @@ const legacyCryptoStore = new IndexedDBCryptoStore( - global.indexedDB, + globalThis.indexedDB, 'crypto-store', );In the matrix-js-sdk version used by this repository, is `matrix-js-sdk/lib/matrix` a supported public import path, or should imports use `matrix-js-sdk` entrypoints only?Also applies to: 13-20
🤖 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 `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts` around lines 1 - 6, Replace the deep import path and brittle global usage: change the import source for createClient, IndexedDBCryptoStore, IndexedDBStore, and MatrixClient to import from the public "matrix-js-sdk" entrypoint instead of "matrix-js-sdk/lib/matrix", and replace any uses of global.* (e.g., global.indexedDB or other global references) with the browser-safe globalThis.* equivalents so the code uses public SDK imports and safe globals.
🤖 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 `@src/frontend/apps/hub/src/features/auth/Auth.tsx`:
- Around line 16-19: The logout flow currently only calls
matrixUserStore.removeUser() which clears localStorage but not the
sessionStorage homeserver key used by useMatrixChatUser; update the cleanup to
also remove sessionStorage['OIDC_HS'] (either inside
matrixUserStore.removeUser() or immediately in logout()) so the OIDC_HS entry is
cleared when logout() runs and no stale homeserver is reused.
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx`:
- Around line 28-31: The hook useMatrixUser returns early when a cached
currentUser exists but never hydrates hook state, so callers still see chatUser
=== null; modify the early-return path in useMatrixUser to call
setChatUser(currentUser) (or the hook's state setter) before returning
currentUser, ensuring the hook state and returned value are consistent (update
the logic around the currentUser check and setChatUser usage so consumers
receive the hydrated chatUser).
- Around line 60-63: In useMatrixUser (the hook implementation handling OIDC
callback), remove the console.log statements that print the OIDC secrets (the
"code" and "state" values) and any other debug prints that expose credential
material; instead, keep the validation guard (if (!code || !state || typeof code
!== 'string' || typeof state !== 'string') return;) and proceed silently (or log
non-sensitive high-level events) before calling the OIDC/token exchange routines
so callback secrets are never emitted to the console or logs.
- Around line 34-49: The current flow sets setisStartOidcFlow(true) but if any
awaited call (fetchHomeserverForEmail, getOIDCAuthUrl) throws the flag can
remain true; wrap the entire async sequence after setisStartOidcFlow(true) in a
try/finally block and move setisStartOidcFlow(false) into the finally so it
always runs; keep the same logic for discovering currentHomeserverSelected,
storing OIDC_HS to sessionStorage, and redirecting via window.location.href =
authUrl, but ensure errors are either logged or rethrown after the finally so
the loader flag is always reset.
In `@src/frontend/apps/hub/src/features/matrix/utils/auth.ts`:
- Around line 195-203: The try/catch is bypassed because client.whoami() is
returned as a Promise; change the flow in the createClient/verification block so
you await the whoami() call inside the try (e.g. const who = await
client.whoami()) and then return the result, ensuring exceptions from whoami()
are caught by the catch; update any local variables/returns in the scope of the
createClient/whoami usage accordingly.
In `@src/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts`:
- Around line 29-31: The catch block in autodiscovery.ts that currently logs
"Could not find homeserver for this email" should not swallow the failure;
replace the console.error + returning undefined with re-throwing a typed error
(e.g., a new MatrixDiscoveryError or Error with a clear name/message) from the
same catch block so callers of the autodiscovery function are forced to handle
the failure path explicitly; ensure you include the original err as cause or
append its message to preserve diagnostic details.
---
Duplicate comments:
In `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx`:
- Line 25: In MatrixClientRoot.tsx remove the credential-bearing debug log that
prints the entire chatUser object (the console.log('**** initializing matrix
client', chatUser) statement); either delete the console.log or replace it with
a non-sensitive message (e.g., "initializing matrix client" only) or log a
sanitized subset (e.g., user id only) so access/refresh tokens are not emitted;
update any tests or lints referencing this log if needed.
- Line 23: The queryKey used in MatrixClientRoot is too weak (currently
queryKey: ['matrixClient', chatUser?.mxId]) so with staleTime: Infinity a client
can be reused across homeserver/device/session changes; strengthen the key to
include all identifiers that define a distinct client (e.g., include
chatUser?.mxId plus homeserver URL, deviceId or session token/instance id) so
React Query treats different sessions as distinct; update the queryKey reference
in MatrixClientRoot to include those additional properties (and keep staleTime
behavior unchanged).
- Around line 33-45: The effect starts Matrix sync but never stops it; update
the useEffect that references mx, clientRunning and startClient to return a
cleanup function that stops the sync when mx changes or the component unmounts:
when you call startClient(mx) capture any returned stopper/disposer (or call an
explicit stop function such as stopClient(mx) or mx.stopSync()) and invoke it
inside the cleanup; ensure the cleanup is guarded by the same mx/clientRunning
checks so you don't call stop on an undefined client.
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx`:
- Around line 42-44: The code dereferences the result of
fetchHomeserverForEmail(email) without checking for undefined; update
useMatrixUser to guard the hs value (the variable returned by
fetchHomeserverForEmail) before assigning currentHomeserverSelected and calling
sessionStorage.setItem with OIDC_HS — if hs is undefined, fail fast by throwing
or returning an explicit error/early exit with a clear message so you never use
hs!.base_url.
- Around line 38-40: The hardcoded personal email in useMatrixUser.tsx (inside
the development branch where email is set) must be removed; replace the literal
'marc3@tchap.beta.gouv.fr' with a neutral, documented dev environment variable
(for example process.env.DEV_MATRIX_EMAIL) and fall back to an empty string or
existing safe default if that env var is not set; update any related
code/comments to reference the new env var and ensure the check remains inside
the existing process.env.NODE_ENV === 'development' block to preserve dev-only
behavior.
In `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts`:
- Around line 1-6: Replace the deep import path and brittle global usage: change
the import source for createClient, IndexedDBCryptoStore, IndexedDBStore, and
MatrixClient to import from the public "matrix-js-sdk" entrypoint instead of
"matrix-js-sdk/lib/matrix", and replace any uses of global.* (e.g.,
global.indexedDB or other global references) with the browser-safe globalThis.*
equivalents so the code uses public SDK imports and safe globals.
In `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts`:
- Around line 10-13: Harden getUser() by wrapping the JSON.parse call in a
try/catch: call localStorage.getItem(MATRIX_USER_KEY) as before, attempt to
JSON.parse(data) inside try and return the parsed MatrixUserInterface on
success, but if parsing throws catch the error, call
localStorage.removeItem(MATRIX_USER_KEY) to clear the corrupted entry, and
return null; keep function signature getUser(): MatrixUserInterface | null and
preserve behavior when data is null (return null) without throwing.
- Around line 26-30: The persistOIDC function currently writes sensitive id
token material to localStorage; remove the call that stores OIDC_ID_TOKEN and
stop persisting idToken there (leave clientId and tokenIssuer metadata if
needed), and instead surface the idToken to a safer channel—e.g., keep it
in-memory in the running Matrix client/session state or have the backend set a
Secure, HttpOnly cookie or use a session-backed token flow; update persistOIDC
(and any callers relying on OIDC_ID_TOKEN in localStorage) to stop
reading/writing the id token from localStorage and use the in-memory/session or
server cookie approach.
In `@src/frontend/apps/hub/src/features/matrix/types.ts`:
- Line 3: Replace the unstable import path for OidcClientConfig: instead of
importing OidcClientConfig from "matrix-js-sdk/lib/matrix" update the import to
the documented OIDC entrypoint "matrix-js-sdk/lib/oidc" (use the
OidcClientConfig symbol) so the type comes from the public OIDC module (adjust
the import in the file where OidcClientConfig is referenced, e.g., types.ts).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b348f199-e224-4fdd-900f-416caa80ecbd
⛔ Files ignored due to path filters (1)
src/frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
src/frontend/apps/hub/package.jsonsrc/frontend/apps/hub/src/features/auth/Auth.tsxsrc/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.tssrc/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsxsrc/frontend/apps/hub/src/features/matrix/config.tssrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixClients.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsxsrc/frontend/apps/hub/src/features/matrix/initMatrix.tssrc/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.tssrc/frontend/apps/hub/src/features/matrix/types.tssrc/frontend/apps/hub/src/features/matrix/utils/auth.tssrc/frontend/apps/hub/src/features/matrix/utils/autodiscovery.tssrc/frontend/apps/hub/src/pages/_app.tsx
Comment on lines
16
to
19
| export const logout = () => { | ||
| window.location.replace(new URL('logout/', baseApiUrl()).href); | ||
| matrixUserStore.removeUser(); | ||
| posthog.reset(); |
There was a problem hiding this comment.
Clear Matrix OIDC homeserver session state on logout.
matrixUserStore.removeUser() only clears localStorage, but useMatrixChatUser reuses sessionStorage homeserver (OIDC_HS). If not cleared, a later user in the same browser can be routed to a stale homeserver. Please clear that session key as part of logout cleanup (ideally in the same store cleanup path).
🤖 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 `@src/frontend/apps/hub/src/features/auth/Auth.tsx` around lines 16 - 19, The
logout flow currently only calls matrixUserStore.removeUser() which clears
localStorage but not the sessionStorage homeserver key used by
useMatrixChatUser; update the cleanup to also remove sessionStorage['OIDC_HS']
(either inside matrixUserStore.removeUser() or immediately in logout()) so the
OIDC_HS entry is cleared when logout() runs and no stale homeserver is reused.
Comment on lines
+34
to
+49
| setisStartOidcFlow(true); | ||
| // Find correct homeserver url | ||
| // TODO Should use user?.email, for now only using hardcoded email | ||
| let email = user?.email || ''; | ||
| if (process.env.NODE_ENV === 'development') { | ||
| email = 'marc3@tchap.beta.gouv.fr'; | ||
| } | ||
| if (!currentHomeserverSelected) { | ||
| const hs = await fetchHomeserverForEmail(email); | ||
| currentHomeserverSelected = hs!.base_url; | ||
| sessionStorage.setItem(OIDC_HS, hs!.base_url); | ||
| } | ||
| const authUrl = await getOIDCAuthUrl(currentHomeserverSelected, email); | ||
| setisStartOidcFlow(false); | ||
| // start oidc flow in tchap MAS | ||
| window.location.href = authUrl; |
There was a problem hiding this comment.
isStartOidcFlow can stay stuck true on failures.
If any step throws after Line 34 (discovery/auth URL fetch), the loader gate in Auth.tsx can block indefinitely. Wrap the flow in try/finally and always reset the flag.
🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around
lines 34 - 49, The current flow sets setisStartOidcFlow(true) but if any awaited
call (fetchHomeserverForEmail, getOIDCAuthUrl) throws the flag can remain true;
wrap the entire async sequence after setisStartOidcFlow(true) in a try/finally
block and move setisStartOidcFlow(false) into the finally so it always runs;
keep the same logic for discovering currentHomeserverSelected, storing OIDC_HS
to sessionStorage, and redirecting via window.location.href = authUrl, but
ensure errors are either logged or rethrown after the finally so the loader flag
is always reset.
move usechatlocal user inside matrix driver
Most of the logic is happening in the driver now instead of a matrix context
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/apps/hub/src/features/matrix/utils/auth.ts (1)
89-89: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove debug
console.logstatements before merging.Lines 89, 97, and 125 contain debug logs that expose OIDC metadata and authorization URLs. These should be removed or gated behind a debug flag for production.
Also applies to: 97-97, 125-125
🤖 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 `@src/frontend/apps/hub/src/features/matrix/utils/auth.ts` at line 89, Remove the debug console.log calls that leak OIDC metadata and auth URLs (e.g., the console.log('*** delegatedAuthConfig', delegatedAuthConfig) and the other logs on lines referencing delegatedAuthConfig and auth URL generation); either delete these console.log statements or wrap them behind a controlled debug flag (e.g., only log when a runtime DEBUG/LOG_LEVEL flag is true) within the same module so that functions handling delegatedAuthConfig and the authorization URL generation no longer unconditionally print sensitive info in production.src/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsx (1)
8-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in filename: "useMatrixSynxState" should be "useMatrixSyncState".
The filename contains a typo (
Synxinstead ofSync). This affects import paths and discoverability. Consider renaming the file touseMatrixSyncState.tsx.🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsx` around lines 8 - 18, The filename contains a typo; rename the file from useMatrixSynxState.tsx to useMatrixSyncState.tsx and update any imports that reference useMatrixSynxState to the new filename; verify the exported hook name useMatrixSyncState (function symbol in the file) matches the filename and update project references (components, tests, barrel/index files) to avoid broken imports.src/frontend/apps/hub/src/features/config/Config.ts (1)
3-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getDriver()currently returns a fresh driver instance on each call.Because
getConfig()instantiatesnew MatrixDriver()every time, driver state (listener registration, Matrix client reference) is not stable across renders/calls.🔧 Suggested fix
-import { MatrixDriver } from "../drivers/implementations/MatrixDriver"; +import { MatrixDriver } from "../drivers/implementations/MatrixDriver"; + +const driver = new MatrixDriver(); export const getConfig = () => { // TODO: Later, be based on URL query params for instance. return { - driver: new MatrixDriver(), + driver, }; };🤖 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 `@src/frontend/apps/hub/src/features/config/Config.ts` around lines 3 - 13, getDriver/getConfig currently instantiate a new MatrixDriver on every call, causing unstable driver state; change to create and reuse a single MatrixDriver instance (e.g., lazy-init a module-level variable or singleton) so getConfig() returns the same driver and getDriver() returns that cached instance; update references to MatrixDriver, getConfig, and getDriver to use the cached driver and ensure any listener registration or client references persist across calls.
♻️ Duplicate comments (1)
src/frontend/apps/hub/src/features/auth/Auth.tsx (1)
17-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLogout cleanup still misses OIDC homeserver session state.
matrixUserStore.removeUser()clears localStorage, but the homeserver session key used by OIDC flow is still not cleared on logout, so a later user can reuse stale homeserver routing.🤖 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 `@src/frontend/apps/hub/src/features/auth/Auth.tsx` around lines 17 - 20, The logout function currently redirects and calls matrixUserStore.removeUser() but does not clear the OIDC homeserver session key; update the logout implementation (exported logout) to explicitly remove the OIDC homeserver session entry from storage (e.g. localStorage.removeItem('<OIDC_HOMESERVER_KEY>') or sessionStorage.removeItem(...)) before calling matrixUserStore.removeUser() and before performing window.location.replace so the homeserver routing state used by the OIDC flow cannot be reused; use the actual key constant/name used by your OIDC codebase when implementing this removal.
🤖 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 `@src/frontend/apps/hub/src/features/auth/Auth.tsx`:
- Around line 42-48: getDriver() currently constructs a new MatrixDriver on
every render and calling driver.initialize() in render causes repeated
registration of the window event listener; fix by ensuring a single driver
instance and perform initialization as a side-effect: instantiate/persist the
driver using useRef or useMemo (referencing getDriver and MatrixDriver) and call
driver.initialize() inside a useEffect that runs once on mount (and return a
cleanup that removes the window listener or calls driver.dispose/cleanup if
available) so initialization happens only once and is properly cleaned up.
In `@src/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.ts`:
- Around line 71-74: The onChatUser handler in MatrixDriver currently reads user
from data.detail.value which doesn't match the dispatched CustomEvent (the
event's detail is the user object itself), causing user to be undefined; update
MatrixDriver.onChatUser to read const user = data.detail (and adjust the
CustomEvent generic/type to CustomEvent<MatrixUserInterface> if needed) so
subsequent checks like this.mx.getUserId() === user.mxId work correctly.
In `@src/frontend/apps/hub/src/features/drivers/implementations/MockDriver.ts`:
- Around line 209-214: In useChatLocalUser, avoid returning a ChatLocalUser with
userId undefined; instead return chatUser: null when no authenticated user
exists—check the incoming user (User | null | undefined) and if falsy return
chatUser: null with the rest flags (isProcessingCallback, isStartOidcFlow)
intact, otherwise construct a proper ChatLocalUser object with a defined userId
and accessToken; remove the unsafe as ChatLocalUser cast so the type system
enforces correctness.
In `@src/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsx`:
- Around line 37-39: MatrixClientRoot currently returns the variable mx (type
MatrixClient | null) which is invalid for a React component; change the
component to return valid JSX instead — either return null or children when no
UI is required, or wrap and provide mx via a context provider (e.g.
MatrixClientProvider) so descendants can consume the MatrixClient; update
MatrixClientRoot to render <MatrixClientProvider
value={mx}>{children}</MatrixClientProvider> (or return children/null) and
ensure the provider and children props are imported/typed and used instead of
returning mx directly.
- Around line 16-24: The effect that initializes the Matrix client must be made
safe: include mx in the dependency array, and add a cleanup that stops/shuts
down the client (call whatever stop/close method on mx or startClient's returned
controller) when the component unmounts or chatUser changes to avoid leaving
background sync running; inside the async initializer await startClient(mx) and
wrap await calls in try/catch to surface errors, and use a cancelled flag (or
AbortSignal) to avoid calling setMx after unmount. Locate the useEffect block
that calls initClient(chatUser!), startClient(mx), setMx(mx) and update it to
check mx?.clientRunning with the up-to-date mx, await startClient, handle
errors, and stop/cleanup the client in the returned cleanup function.
In `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts`:
- Around line 18-22: The dispatched CustomEvent currently sets detail to
localUser but the listener in MatrixDriver.ts expects event.detail.value (and
global.d.ts defines { key: string; value: MatrixUserInterface }), so change the
dispatch in matrixUserStore.ts to send detail with the expected shape (e.g., an
object containing key: CHAT_USER_LISTENER_KEY and value: localUser) so
event.detail.value is defined; ensure the symbol CHAT_USER_LISTENER_KEY and the
MatrixUserInterface instance (localUser) are used when constructing the detail
object to match the listener's expectations.
---
Outside diff comments:
In `@src/frontend/apps/hub/src/features/config/Config.ts`:
- Around line 3-13: getDriver/getConfig currently instantiate a new MatrixDriver
on every call, causing unstable driver state; change to create and reuse a
single MatrixDriver instance (e.g., lazy-init a module-level variable or
singleton) so getConfig() returns the same driver and getDriver() returns that
cached instance; update references to MatrixDriver, getConfig, and getDriver to
use the cached driver and ensure any listener registration or client references
persist across calls.
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsx`:
- Around line 8-18: The filename contains a typo; rename the file from
useMatrixSynxState.tsx to useMatrixSyncState.tsx and update any imports that
reference useMatrixSynxState to the new filename; verify the exported hook name
useMatrixSyncState (function symbol in the file) matches the filename and update
project references (components, tests, barrel/index files) to avoid broken
imports.
In `@src/frontend/apps/hub/src/features/matrix/utils/auth.ts`:
- Line 89: Remove the debug console.log calls that leak OIDC metadata and auth
URLs (e.g., the console.log('*** delegatedAuthConfig', delegatedAuthConfig) and
the other logs on lines referencing delegatedAuthConfig and auth URL
generation); either delete these console.log statements or wrap them behind a
controlled debug flag (e.g., only log when a runtime DEBUG/LOG_LEVEL flag is
true) within the same module so that functions handling delegatedAuthConfig and
the authorization URL generation no longer unconditionally print sensitive info
in production.
---
Duplicate comments:
In `@src/frontend/apps/hub/src/features/auth/Auth.tsx`:
- Around line 17-20: The logout function currently redirects and calls
matrixUserStore.removeUser() but does not clear the OIDC homeserver session key;
update the logout implementation (exported logout) to explicitly remove the OIDC
homeserver session entry from storage (e.g.
localStorage.removeItem('<OIDC_HOMESERVER_KEY>') or
sessionStorage.removeItem(...)) before calling matrixUserStore.removeUser() and
before performing window.location.replace so the homeserver routing state used
by the OIDC flow cannot be reused; use the actual key constant/name used by your
OIDC codebase when implementing this removal.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e3bc688-d50a-4216-90d8-c26d6f41a973
📒 Files selected for processing (13)
src/frontend/apps/hub/global.d.tssrc/frontend/apps/hub/src/features/auth/Auth.tsxsrc/frontend/apps/hub/src/features/config/Config.tssrc/frontend/apps/hub/src/features/drivers/Driver.tssrc/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.tssrc/frontend/apps/hub/src/features/drivers/implementations/MockDriver.tssrc/frontend/apps/hub/src/features/drivers/types.tssrc/frontend/apps/hub/src/features/matrix/components/MatrixClientRoot.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixSynxState.tsxsrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsxsrc/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.tssrc/frontend/apps/hub/src/features/matrix/utils/auth.tssrc/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts
Comment on lines
+209
to
+214
| useChatLocalUser(user: User | null | undefined) { | ||
| return () => ({ | ||
| chatUser: { | ||
| userId: user?.id, | ||
| accessToken: "" | ||
| } as ChatLocalUser, isProcessingCallback: false, isStartOidcFlow: false }); |
There was a problem hiding this comment.
Return chatUser: null when no authenticated user exists.
Line 212 can produce userId: undefined and then bypasses typing via as ChatLocalUser. This violates the contract and can trigger downstream auth/client logic with invalid credentials.
🔧 Suggested fix
useChatLocalUser(user: User | null | undefined) {
return () => ({
- chatUser: {
- userId: user?.id,
- accessToken: ""
- } as ChatLocalUser, isProcessingCallback: false, isStartOidcFlow: false });
+ chatUser: user
+ ? ({ userId: user.id, accessToken: "" } as ChatLocalUser)
+ : null,
+ isProcessingCallback: false,
+ isStartOidcFlow: false,
+ });
}🤖 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 `@src/frontend/apps/hub/src/features/drivers/implementations/MockDriver.ts`
around lines 209 - 214, In useChatLocalUser, avoid returning a ChatLocalUser
with userId undefined; instead return chatUser: null when no authenticated user
exists—check the incoming user (User | null | undefined) and if falsy return
chatUser: null with the rest flags (isProcessingCallback, isStartOidcFlow)
intact, otherwise construct a proper ChatLocalUser object with a defined userId
and accessToken; remove the unsafe as ChatLocalUser cast so the type system
enforces correctness.
Comment on lines
+18
to
+22
| window.dispatchEvent( | ||
| new CustomEvent(CHAT_USER_LISTENER_KEY, { | ||
| detail: localUser | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Event payload shape mismatch — event.detail.value will be undefined.
The CustomEvent is dispatched with detail: localUser, but the listener in MatrixDriver.ts:71 reads event.detail.value. The global.d.ts type also declares { key: string; value: MatrixUserInterface }. This mismatch will cause the Matrix client initialization to fail silently.
🐛 Proposed fix
localStorage.setItem(MATRIX_USER_KEY, JSON.stringify(localUser));
window.dispatchEvent(
new CustomEvent(CHAT_USER_LISTENER_KEY, {
- detail: localUser
+ detail: { key: MATRIX_USER_KEY, value: localUser }
})
);🤖 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 `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts` around
lines 18 - 22, The dispatched CustomEvent currently sets detail to localUser but
the listener in MatrixDriver.ts expects event.detail.value (and global.d.ts
defines { key: string; value: MatrixUserInterface }), so change the dispatch in
matrixUserStore.ts to send detail with the expected shape (e.g., an object
containing key: CHAT_USER_LISTENER_KEY and value: localUser) so
event.detail.value is defined; ensure the symbol CHAT_USER_LISTENER_KEY and the
MatrixUserInterface instance (localUser) are used when constructing the detail
object to match the listener's expectations.
Most of the logic is happening in the driver now instead of a matrix context
MarcWadai
force-pushed
the
init-matrix-client
branch
from
May 28, 2026 15:42
bd1a636 to
f2f5297
Compare
Avoid unecessary creation of driver
MarcWadai
force-pushed
the
init-matrix-client
branch
from
May 28, 2026 15:58
2d39bbd to
70a9d97
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts (2)
10-12: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winTighten the return type to remove impossible
void.Line 12 advertises
void, but this function now throws on failures and otherwise returns a homeserver object. Keepingvoidhere encourages unsafe non-null assertions at call sites.Suggested patch
export const fetchHomeserverForEmail = async ( email: string, -): Promise<void | { base_url: string; server_name: string }> => { +): Promise<{ base_url: string; server_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 `@src/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts` around lines 10 - 12, The declared return type of fetchHomeserverForEmail is too loose—remove the impossible void and tighten the signature to always return the homeserver object (Promise<{ base_url: string; server_name: string }>) because the function throws on failure; update the function signature for fetchHomeserverForEmail and any callers that assumed a nullable/void result to expect a resolved object or handle exceptions instead.
14-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard empty homeserver configuration before dereferencing index
0.Line 14 assumes at least one configured homeserver. If
homeserver_listis empty, this throws before the fetch path and bypasses your explicit error handling.Suggested patch
- const randomHomeServer = homeServerList[0]; + const randomHomeServer = homeServerList[0]; + if (!randomHomeServer) { + throw new Error("No homeserver configured"); + }🤖 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 `@src/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts` at line 14, The code dereferences homeServerList[0] into randomHomeServer without checking for an empty array; update the autodiscovery logic in autodiscovery.ts to guard against an empty homeserver_list by checking homeServerList.length (or truthiness) before accessing index 0, and if empty return or throw a meaningful error (or take the existing error path) so the subsequent fetch/lookup logic is not bypassed; adjust any callers of randomHomeServer to handle this early error/empty-case as needed.src/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.ts (1)
61-67:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBind and register the chat-user listener exactly once.
Passing
this.onChatUserdirectly at Line 66 is unsafe for classthiscontext, and repeatedinitialize()calls stack duplicate listeners. Make the handler an arrow function (or bind in constructor) and add an idempotent registration guard.Suggested patch
export class MatrixDriver extends StandardDriver { + private isInitialized = false; @@ public initialize() { - if (typeof window !== 'undefined') { + if (this.isInitialized) return; + if (typeof window !== 'undefined') { window.addEventListener( CHAT_USER_LISTENER_KEY, this.onChatUser ); + this.isInitialized = true; } } @@ - private async onChatUser(data: CustomEvent<{ user: MatrixUserInterface }>) { + private onChatUser = async (event: Event) => { + const data = event as CustomEvent<{ user: MatrixUserInterface }>; const user = data.detail.user; @@ - } + };Also applies to: 72-73
🤖 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 `@src/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.ts` around lines 61 - 67, The MatrixDriver.initialize currently passes this.onChatUser directly and can register multiple listeners and lose the correct `this` context; fix by creating and storing a single bound handler property (e.g., this.boundOnChatUser = this.onChatUser.bind(this) or define onChatUser as an arrow on the instance) and guard registration with an idempotent flag (e.g., this._chatListenerRegistered) so you only call window.addEventListener(CHAT_USER_LISTENER_KEY, this.boundOnChatUser) once; likewise ensure removal uses the same this.boundOnChatUser and reset the guard when unregistering so addEventListener/removeEventListener calls match.
♻️ Duplicate comments (5)
src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx (3)
45-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways reset
isStartOidcFlowwithfinally.If
fetchHomeserverForEmailorgetOIDCAuthUrlthrows,isStartOidcFlowstaystrueand the auth screen can block indefinitely.Suggested patch
setisStartOidcFlow(true); - // Find correct homeserver url - // TODO Should use user?.email, for now only using hardcoded email - let email = user?.email || ''; - if (process.env.NODE_ENV === 'development') { - email = 'marc3@tchap.beta.gouv.fr'; - } - if (!currentHomeserverSelected) { - const hs = await fetchHomeserverForEmail(email); - currentHomeserverSelected = hs!.base_url; - sessionStorage.setItem(OIDC_HS, hs!.base_url); - } - const authUrl = await getOIDCAuthUrl(currentHomeserverSelected, email); - setisStartOidcFlow(false); - // start oidc flow in tchap MAS - window.location.href = authUrl; + try { + // existing OIDC discovery/auth-url logic + const authUrl = await getOIDCAuthUrl(currentHomeserverSelected, email); + window.location.href = authUrl; + } finally { + setisStartOidcFlow(false); + }🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around lines 45 - 58, Wrap the async block that calls fetchHomeserverForEmail and getOIDCAuthUrl in a try/finally inside the useMatrixUser flow so setisStartOidcFlow(true) is always balanced by setisStartOidcFlow(false); specifically, perform the homeserver resolution and auth URL retrieval (the code that uses currentHomeserverSelected, fetchHomeserverForEmail, sessionStorage.setItem(OIDC_HS, ...), and getOIDCAuthUrl) inside a try and move the final setisStartOidcFlow(false) into the finally block to guarantee it runs even if those calls throw.
53-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid non-null assertions on homeserver discovery result.
Lines 54-55 use
hs!and will hard-crash if discovery fails or contract changes. Guard the returned value before dereference.🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around lines 53 - 55, The code dereferences the homeserver discovery result with non-null assertions (hs!) which can crash if discovery fails; update the logic in useMatrixUser (around fetchHomeserverForEmail, currentHomeserverSelected and OIDC_HS) to first check that hs and hs.base_url are defined before assigning currentHomeserverSelected or calling sessionStorage.setItem, and handle the failure path (e.g., log an error, set a safe default, or return/throw) instead of using "!" so the code gracefully handles a missing discovery result.
49-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove hardcoded personal email from the auth flow.
Line 50 hardcodes a real personal address, which is a privacy leak and can mis-route logins in shared dev/staging contexts.
🤖 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 `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx` around lines 49 - 51, Remove the hardcoded personal email in useMatrixUser.tsx by eliminating the direct assignment to email inside the process.env.NODE_ENV === 'development' block; instead read a development-only environment variable (e.g., process.env.REACT_APP_DEV_EMAIL or process.env.VITE_DEV_EMAIL) or a test fixture constant and only use it when NODE_ENV==='development', and ensure the fallback remains the normal auth flow so no real addresses are injected into dev/staging builds.src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts (1)
32-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop persisting
idTokeninlocalStorage.Line 35 stores sensitive OIDC token material in
localStorage, which is recoverable in any XSS scenario. Keep only non-sensitive metadata here and move token handling to in-memory state or backend-managed HttpOnly cookie flow.Suggested minimal change
-const OIDC_ID_TOKEN = 'oidc_id_token'; @@ - removeUser() { + removeUser() { localStorage.removeItem(MATRIX_USER_KEY); localStorage.removeItem(OIDC_CLIENT_ID); localStorage.removeItem(OIDC_ISSUER); - localStorage.removeItem(OIDC_ID_TOKEN); } @@ - persistOIDC(clientId: string, tokenIssuer: string, idToken: string) { + persistOIDC(clientId: string, tokenIssuer: string, _idToken: string) { localStorage.setItem(OIDC_CLIENT_ID, clientId); localStorage.setItem(OIDC_ISSUER, tokenIssuer); - localStorage.setItem(OIDC_ID_TOKEN, idToken); }🤖 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 `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts` around lines 32 - 36, In persistOIDC, stop persisting the actual idToken to localStorage: remove the localStorage.setItem(OIDC_ID_TOKEN, idToken) call and instead keep only non-sensitive metadata (clientId and tokenIssuer) in localStorage; store idToken in the store's in-memory state (e.g., a private field on matrixUserStore) or rely on a backend-managed HttpOnly cookie flow, and ensure any existing OIDC_ID_TOKEN entry is removed/cleared on load so previously persisted tokens are not retained.src/frontend/apps/hub/src/features/matrix/initMatrix.ts (1)
1-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace internal SDK deep-imports and
global.*usage.
matrix-js-sdk/lib/matrixis an internal path and can break across SDK releases; also useglobalThisfor browser runtime globals.Suggested patch
} from "matrix-js-sdk/lib/matrix"; +} from "matrix-js-sdk"; @@ - indexedDB: global.indexedDB, - localStorage: global.localStorage, + indexedDB: globalThis.indexedDB, + localStorage: globalThis.localStorage, @@ - global.indexedDB, + globalThis.indexedDB,Also applies to: 13-14, 19-19
🤖 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 `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts` around lines 1 - 6, The import uses an internal deep path ("matrix-js-sdk/lib/matrix") and code uses global.*; change imports to the public SDK entrypoint and switch runtime globals to globalThis to avoid breakage: replace the deep-import with the public module import that exports createClient, IndexedDBCryptoStore, IndexedDBStore, MatrixClient (i.e. import from "matrix-js-sdk" or the package's documented top-level entry) and update any global references (e.g. global, global.window, global.XMLHttpRequest) to use globalThis so browser environments work consistently.
🤖 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 `@src/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.ts`:
- Line 88: The code currently calls startClient(mx) without awaiting, so a
startup rejection can leave this.mx pointing at an unstarted client; change the
call in MatrixDriver (inside the startup/initialization flow where this.mx is
assigned and startClient(mx) is invoked) to await startClient(mx) and wrap it in
a try/catch: await startClient(mx) inside the try, and on catch log/propagate
the error and unset this.mx (or rollback the assignment) so the driver does not
retain a non-started client instance.
In `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts`:
- Line 22: The console.log in initMatrix prints the entire user object
(including accessToken) — remove that full-object log and replace it with a
non-sensitive message: either log only a safe identifier (e.g., user.id or
user.userId) or log a redacted user summary (explicitly excluding accessToken
and other credentials). Locate the console.log("*** user in init client", user)
in the initMatrix function and delete or change it to a redacted/safe form
before committing.
---
Outside diff comments:
In `@src/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.ts`:
- Around line 61-67: The MatrixDriver.initialize currently passes
this.onChatUser directly and can register multiple listeners and lose the
correct `this` context; fix by creating and storing a single bound handler
property (e.g., this.boundOnChatUser = this.onChatUser.bind(this) or define
onChatUser as an arrow on the instance) and guard registration with an
idempotent flag (e.g., this._chatListenerRegistered) so you only call
window.addEventListener(CHAT_USER_LISTENER_KEY, this.boundOnChatUser) once;
likewise ensure removal uses the same this.boundOnChatUser and reset the guard
when unregistering so addEventListener/removeEventListener calls match.
In `@src/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts`:
- Around line 10-12: The declared return type of fetchHomeserverForEmail is too
loose—remove the impossible void and tighten the signature to always return the
homeserver object (Promise<{ base_url: string; server_name: string }>) because
the function throws on failure; update the function signature for
fetchHomeserverForEmail and any callers that assumed a nullable/void result to
expect a resolved object or handle exceptions instead.
- Line 14: The code dereferences homeServerList[0] into randomHomeServer without
checking for an empty array; update the autodiscovery logic in autodiscovery.ts
to guard against an empty homeserver_list by checking homeServerList.length (or
truthiness) before accessing index 0, and if empty return or throw a meaningful
error (or take the existing error path) so the subsequent fetch/lookup logic is
not bypassed; adjust any callers of randomHomeServer to handle this early
error/empty-case as needed.
---
Duplicate comments:
In `@src/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsx`:
- Around line 45-58: Wrap the async block that calls fetchHomeserverForEmail and
getOIDCAuthUrl in a try/finally inside the useMatrixUser flow so
setisStartOidcFlow(true) is always balanced by setisStartOidcFlow(false);
specifically, perform the homeserver resolution and auth URL retrieval (the code
that uses currentHomeserverSelected, fetchHomeserverForEmail,
sessionStorage.setItem(OIDC_HS, ...), and getOIDCAuthUrl) inside a try and move
the final setisStartOidcFlow(false) into the finally block to guarantee it runs
even if those calls throw.
- Around line 53-55: The code dereferences the homeserver discovery result with
non-null assertions (hs!) which can crash if discovery fails; update the logic
in useMatrixUser (around fetchHomeserverForEmail, currentHomeserverSelected and
OIDC_HS) to first check that hs and hs.base_url are defined before assigning
currentHomeserverSelected or calling sessionStorage.setItem, and handle the
failure path (e.g., log an error, set a safe default, or return/throw) instead
of using "!" so the code gracefully handles a missing discovery result.
- Around line 49-51: Remove the hardcoded personal email in useMatrixUser.tsx by
eliminating the direct assignment to email inside the process.env.NODE_ENV ===
'development' block; instead read a development-only environment variable (e.g.,
process.env.REACT_APP_DEV_EMAIL or process.env.VITE_DEV_EMAIL) or a test fixture
constant and only use it when NODE_ENV==='development', and ensure the fallback
remains the normal auth flow so no real addresses are injected into dev/staging
builds.
In `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts`:
- Around line 1-6: The import uses an internal deep path
("matrix-js-sdk/lib/matrix") and code uses global.*; change imports to the
public SDK entrypoint and switch runtime globals to globalThis to avoid
breakage: replace the deep-import with the public module import that exports
createClient, IndexedDBCryptoStore, IndexedDBStore, MatrixClient (i.e. import
from "matrix-js-sdk" or the package's documented top-level entry) and update any
global references (e.g. global, global.window, global.XMLHttpRequest) to use
globalThis so browser environments work consistently.
In `@src/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.ts`:
- Around line 32-36: In persistOIDC, stop persisting the actual idToken to
localStorage: remove the localStorage.setItem(OIDC_ID_TOKEN, idToken) call and
instead keep only non-sensitive metadata (clientId and tokenIssuer) in
localStorage; store idToken in the store's in-memory state (e.g., a private
field on matrixUserStore) or rely on a backend-managed HttpOnly cookie flow, and
ensure any existing OIDC_ID_TOKEN entry is removed/cleared on load so previously
persisted tokens are not retained.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e53311d-f07d-4321-8f19-266f645e7ad9
📒 Files selected for processing (8)
src/frontend/apps/hub/global.d.tssrc/frontend/apps/hub/src/features/auth/Auth.tsxsrc/frontend/apps/hub/src/features/config/Config.tssrc/frontend/apps/hub/src/features/drivers/implementations/MatrixDriver.tssrc/frontend/apps/hub/src/features/matrix/hooks/useMatrixUser.tsxsrc/frontend/apps/hub/src/features/matrix/initMatrix.tssrc/frontend/apps/hub/src/features/matrix/stores/matrixUserStore.tssrc/frontend/apps/hub/src/features/matrix/utils/autodiscovery.ts
💤 Files with no reviewable changes (1)
- src/frontend/apps/hub/src/features/auth/Auth.tsx
| global.indexedDB, | ||
| 'crypto-store', | ||
| ); | ||
| console.log("*** user in init client", user); |
There was a problem hiding this comment.
Do not log the full Matrix user object.
Line 22 logs credentials (accessToken), which is sensitive auth material and should never be printed.
🤖 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 `@src/frontend/apps/hub/src/features/matrix/initMatrix.ts` at line 22, The
console.log in initMatrix prints the entire user object (including accessToken)
— remove that full-object log and replace it with a non-sensitive message:
either log only a safe identifier (e.g., user.id or user.userId) or log a
redacted user summary (explicitly excluding accessToken and other credentials).
Locate the console.log("*** user in init client", user) in the initMatrix
function and delete or change it to a redacted/safe form before committing.
Remove windows event emitter and use custom one
add refreshtoken on new init client
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.
Add oidc flow to matrix OIDC compatible server, introduce matrix client provider and chatUser values
Purpose
Describe the purpose of this pull request.
Proposal
External contributions
Thank you for your contribution! 🎉
Please ensure the following items are checked before submitting your pull request:
General requirements
Skip the checkbox below 👇 if you're fixing an issue or adding documentation
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)AI requirements
Skip the checkboxes below 👇 If you didn't use AI for your contribution
Summary by CodeRabbit
Release Notes