Open
Conversation
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 68 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 41 | 1 | 26 | 0 |
Click to see the invalid file list
- vite.config.ts
| @@ -0,0 +1,36 @@ | |||
| import path from 'path'; | |||
There was a problem hiding this comment.
Suggested change
| import path from 'path'; | |
| /* | |
| * This Source Code Form is subject to the terms of the Mozilla Public | |
| * License, v. 2.0. If a copy of the MPL was not distributed with this | |
| * file, You can obtain one at https://mozilla.org/MPL/2.0/. | |
| */ | |
| import path from 'path'; |
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request migrates the project from Create React App to Vite, restructures Redux state management into feature-based slices using Redux Toolkit, refactors API utilities into modular shared libraries, and reorganizes component structure with extracted hooks for the top bar. Changes
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/index.tsx (1)
13-22:⚠️ Potential issue | 🟡 MinorThis file is dead code and should be removed.
src/main.tsxis the active Vite entry point (loaded byindex.html), and contains identical mounting logic with additional necessary imports (polyfills, styles).src/index.tsxis never referenced or executed at runtime and appears to be a leftover from a Create React App migration.Remove this file entirely, or if it's retained for a specific purpose (e.g., legacy support), document that purpose clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.tsx` around lines 13 - 22, The file that mounts the app (contains createRoot, AppProviders, document.write and the throw new Error('No root container found')) is dead code and should be removed or clearly documented; either delete that file entirely and ensure no imports/reference remain, or keep it but add a top-level comment explaining its legacy purpose and update project config/docs to point to the actual entry (main.tsx equivalent); after removal, run a quick project build to confirm no residual references to createRoot/AppProviders remain.src/app/App.tsx (1)
113-127:⚠️ Potential issue | 🔴 CriticalFail closed when auth config is missing.
The ternary at line 114 will default to
initializeAuthenticationDev()whenVITE_USE_AUTHENTICATIONis undefined or any value other than the exact string'true'. This fail-open pattern creates silent auth bypass if the environment variable is not set. Require an explicit value and reject unknown config to make misconfiguration visible.Suggested hardening
- const initAuth = - import.meta.env.VITE_USE_AUTHENTICATION === 'true' + const authFlag = import.meta.env.VITE_USE_AUTHENTICATION; + if (authFlag !== 'true' && authFlag !== 'false') { + throw new Error('VITE_USE_AUTHENTICATION must be set to "true" or "false"'); + } + const initAuth = + authFlag === 'true' ? initializeAuthenticationProd( dispatch, initialMatchSilentRenewCallbackUrl != null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/App.tsx` around lines 113 - 127, The code currently treats any non-'true' VITE_USE_AUTHENTICATION as dev-mode (fail-open); change to explicitly validate the env var: read import.meta.env.VITE_USE_AUTHENTICATION into a local value and branch only on exact 'true' (call initializeAuthenticationProd(...)) or exact 'false' (call initializeAuthenticationDev(...)); if the value is undefined or any other string, throw or dispatch a clear configuration error and abort startup so misconfiguration is visible. Reference initializeAuthenticationProd, initializeAuthenticationDev, and VITE_USE_AUTHENTICATION when making the check.
♻️ Duplicate comments (1)
vite.config.ts (1)
1-5:⚠️ Potential issue | 🟡 MinorAdd missing license header to fix pipeline failure.
The CI pipeline reports a license header check failure. A previous review already flagged this.
,
📝 Proposed fix
+/* + * Copyright (c) 2024, RTE (http://www.rte-france.com) + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + import path from 'path'; import { defineConfig } from 'vite';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 1 - 5, Add the required license header at the very top of vite.config.ts (above the imports) to satisfy the CI header check; update the file containing the imports (path, defineConfig, react, tsconfigPaths, svgr) by prepending the project's standard license comment block exactly as used in other source files so the header-checker recognizes it.
🧹 Nitpick comments (13)
src/shared/config/parameters.ts (1)
11-11: Type union collapses tostring, losing specificity.The type
typeof PARAM_THEME | typeof PARAM_LANGUAGE | stringsimplifies to juststringbecausestringis a supertype of all string literals. This makes the specific constants irrelevant for type-checking.If the intent is to allow arbitrary strings while still providing autocomplete for known parameters, consider using a branded type pattern or simply removing
| stringif only known parameters should be accepted.♻️ Option A: Remove `| string` for strict typing
-export type AppConfigParameter = typeof PARAM_THEME | typeof PARAM_LANGUAGE | string; +export type AppConfigParameter = typeof PARAM_THEME | typeof PARAM_LANGUAGE;♻️ Option B: Use template literal for autocomplete hints
-export type AppConfigParameter = typeof PARAM_THEME | typeof PARAM_LANGUAGE | string; +export type AppConfigParameter = typeof PARAM_THEME | typeof PARAM_LANGUAGE | (string & {});This pattern preserves autocomplete suggestions while allowing arbitrary strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/config/parameters.ts` at line 11, The AppConfigParameter union (AppConfigParameter in parameters.ts) currently includes a bare string which collapses the union to string and loses literal specificity (PARAM_THEME | PARAM_LANGUAGE | string). Fix by either removing the trailing "| string" to make AppConfigParameter a strict union of the known parameter literals (PARAM_THEME | PARAM_LANGUAGE) if only known keys should be allowed, or implement a branded/templated pattern to allow arbitrary strings while preserving autocomplete (e.g., use a branded type or a template-literal wrapper like `string & { __brand?: "AppConfigParameter" }` or `\`${typeof PARAM_THEME | typeof PARAM_LANGUAGE}\` | (string & {})`) so editors still suggest the known constants; update any usages that relied on the previous loose string type accordingly.src/app/router/AppRouter.tsx (3)
20-24: Hardcoded English text bypasses i18n.The "Connected" string is hardcoded while the rest of the app uses
react-intl(e.g., line 33). For consistency, this should useFormattedMessagewith a translation key.♻️ Proposed fix
<Box mt={20}> <Typography variant="h3" color="textPrimary" align="center"> - Connected + <FormattedMessage id="Connected" /> </Typography> </Box>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/router/AppRouter.tsx` around lines 20 - 24, Replace the hardcoded "Connected" string inside the Box → Typography block with a react-intl FormattedMessage; import FormattedMessage if missing and use a descriptive translation id (e.g., "connection.status.connected") and a defaultMessage of "Connected" so the string flows through the app's i18n system and matches other usages in AppRouter.tsx.
28-28: Inconsistent error styling and missing i18n for logout callback.This route uses a raw
<h1>with hardcoded English text, inconsistent with the styled MUI components andFormattedMessageused elsewhere.♻️ Proposed fix for consistency
- <Route path="/logout-callback" element={<h1>Error: logout failed; you are still logged in.</h1>} /> + <Route + path="/logout-callback" + element={ + <Typography variant="h4" color="error" align="center"> + <FormattedMessage id="LogoutError" /> + </Typography> + } + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/router/AppRouter.tsx` at line 28, Replace the raw hardcoded <h1> used for the "/logout-callback" Route in AppRouter with the same styled MUI components and i18n pattern used elsewhere: render a MUI Container/Box (or the app's standard Page wrapper) and a Typography component, and use FormattedMessage (or intl.formatMessage) with a new message id like "logoutCallback.error" to provide the text instead of the hardcoded English string; update the route's element to return this component so styling and localization match the rest of the app.
8-8: UnusedReactimport.With
jsx: "react-jsx"in tsconfig, the explicitReactimport is no longer required for JSX transformation.♻️ Proposed fix
-import React, { FunctionComponent } from 'react'; +import { FunctionComponent } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/router/AppRouter.tsx` at line 8, Remove the unused default React import from AppRouter.tsx since the project uses the new JSX transform; keep (or add) only the named type import for FunctionComponent if the component signature uses it. Replace "import React, { FunctionComponent } from 'react';" with "import { FunctionComponent } from 'react';" (or remove the import entirely if FunctionComponent is not used) and ensure the AppRouter component type annotation still compiles.src/features/app-settings/store/actions.ts (1)
22-29: Consider using a literal property name for type clarity.The
LanguageActiontype uses a computed property key[PARAM_LANGUAGE], which relies onPARAM_LANGUAGE = 'language'at runtime. While this works correctly, it creates an implicit coupling that could break ifPARAM_LANGUAGEchanges. TheThemeActionandComputedLanguageActionuse literal property names (theme,computedLanguage) which is more straightforward.♻️ Optional: Use literal property name for consistency
export const SELECT_LANGUAGE = 'SELECT_LANGUAGE'; export type LanguageAction = Readonly<Action<typeof SELECT_LANGUAGE>> & { - [PARAM_LANGUAGE]: SettingsState['language']; + language: SettingsState['language']; }; export function selectLanguage(language: SettingsState['language']): LanguageAction { - return { type: SELECT_LANGUAGE, [PARAM_LANGUAGE]: language }; + return { type: SELECT_LANGUAGE, language }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/app-settings/store/actions.ts` around lines 22 - 29, The LanguageAction type and selectLanguage function should use the literal property name "language" instead of the computed key PARAM_LANGUAGE to match ThemeAction/ComputedLanguageAction and avoid implicit coupling; update LanguageAction to { language: SettingsState['language'] } and change the object returned by selectLanguage to { type: SELECT_LANGUAGE, language } while keeping SELECT_LANGUAGE, PARAM_LANGUAGE, SettingsState and selectLanguage signatures unchanged.src/app/store/index.ts (2)
12-15: Consider documenting whyserializableCheckis disabled.Disabling
serializableCheckis a common pattern when the store contains non-serializable values (e.g.,Errorobjects inAuthState). A brief comment explaining the reason would help future maintainers.💡 Suggested comment
middleware: (getDefaultMiddleware) => getDefaultMiddleware({ + // Disabled because AuthState contains Error objects which are non-serializable serializableCheck: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/store/index.ts` around lines 12 - 15, Add a short comment above the middleware configuration explaining why serializableCheck is set to false: note that getDefaultMiddleware's serializableCheck is disabled because the Redux store (e.g., AuthState) intentionally contains non-serializable values like Error objects or class instances that would otherwise trigger warnings; reference the middleware configuration (middleware: (getDefaultMiddleware) => getDefaultMiddleware({ serializableCheck: false })) and mention the specific non-serializable types stored so future maintainers understand the rationale and can revisit if store shape changes.
18-19: DuplicateRootStatetype definition.
RootStateis already explicitly defined and exported insrc/app/store/reducer.ts(lines 17-21). This file re-exports it asReturnType<typeof store.getState>. While both should be equivalent, having two sources for the same type can cause confusion. Consider re-exporting fromreducer.tsinstead.♻️ Proposed fix: Re-export from reducer.ts
import { configureStore } from '@reduxjs/toolkit'; -import { reducer } from './reducer'; +import { reducer, RootState } from './reducer'; export const store = configureStore({ reducer, middleware: (getDefaultMiddleware) => getDefaultMiddleware({ serializableCheck: false, }), }); -export type RootState = ReturnType<typeof store.getState>; +export type { RootState }; export type AppDispatch = typeof store.dispatch;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/store/index.ts` around lines 18 - 19, There are two competing RootState definitions: the exported RootState here (as ReturnType<typeof store.getState>) and the explicit RootState exported from reducer (RootState); remove the duplicate by re-exporting the canonical type from reducer instead of redefining it here — replace the local RootState export with a direct re-export of the reducer's RootState and keep AppDispatch as typeof store.dispatch (or re-export AppDispatch from wherever it's already defined) so consumers import a single source of truth for RootState.src/features/app-settings/store/settings-slice.ts (1)
44-46: Side effect in reducer: consider movingsaveLocalStorageThemeto middleware or action creator.Calling
saveLocalStorageThemeinside the reducer introduces a side effect. While this works with Redux Toolkit's Immer integration, pure reducers are preferred. Consider dispatching persistence via middleware or a thunk for cleaner separation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/app-settings/store/settings-slice.ts` around lines 44 - 46, The reducer handling SELECT_THEME (builder.addCase for SELECT_THEME in settings-slice.ts) contains a side effect by calling saveLocalStorageTheme(state.theme); remove that call from the reducer and instead persist the theme from an action creator or middleware: update the ThemeAction/selector flow so the action that dispatches SELECT_THEME also calls saveLocalStorageTheme (or create a small middleware/thunk that listens for SELECT_THEME and writes to localStorage). Ensure you keep state.theme = action.theme in the reducer and move all persistence logic to the action creator or a middleware that references SELECT_THEME and saveLocalStorageTheme.src/app/App.test.tsx (1)
12-12:actfromreact-dom/test-utilsis deprecated in React 18+.In React 18 and later,
actshould be imported directly fromreact. Thetest-utilsversion still works but may trigger deprecation warnings.♻️ Proposed fix
import React from 'react'; +import { act } from 'react'; import { createRoot } from 'react-dom/client'; -import { act } from 'react-dom/test-utils';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/App.test.tsx` at line 12, The test import of act is using the deprecated source; update the import in App.test.tsx to pull act from React instead of react-dom/test-utils — replace the import statement that references act from 'react-dom/test-utils' with an import from 'react' (keeping any other named imports intact) so tests use React's built-in act API.src/shared/api/http.ts (1)
72-74: Consider adding a generic type parameter for type-safe JSON responses.The
Promise<unknown>return type is safe, but callers must cast the result. A generic overload could improve ergonomics:♻️ Optional enhancement
-export function backendFetchJson(url: Url, init?: InitRequest, token?: Token): Promise<unknown> { +export function backendFetchJson<T = unknown>(url: Url, init?: InitRequest, token?: Token): Promise<T> { - return backendFetch(url, init, token).then((safeResponse: Response) => safeResponse.json()); + return backendFetch(url, init, token).then((safeResponse: Response) => safeResponse.json() as Promise<T>); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/api/http.ts` around lines 72 - 74, The backendFetchJson function returns Promise<unknown>, forcing callers to cast; make it generic by adding a type parameter T (e.g. export function backendFetchJson<T = unknown>(url: Url, init?: InitRequest, token?: Token): Promise<T>) and propagate that type when parsing the body (use Response.json() cast or typed return so the function returns Promise<T>), keeping the default T = unknown to preserve backward compatibility; update any call sites if needed to supply the concrete type.src/app/providers/AppProviders.tsx (1)
115-116: Consider using a selector forcomputedLanguagefor consistency.Line 115 uses an inline selector while line 116 uses the imported
selectTheme. For consistency and easier refactoring, consider adding aselectComputedLanguageselector alongside the existing ones insrc/features/app-settings/store/selectors.ts.♻️ Suggested refactor
In
src/features/app-settings/store/selectors.ts, add:export const selectComputedLanguage = (state: RootState) => state.settings.computedLanguage;Then in this file:
+import { selectComputedLanguage, selectTheme } from '@/features/app-settings/store/selectors'; -import { selectTheme } from '@/features/app-settings/store/selectors'; const AppWrapperWithRedux: FunctionComponent = () => { - const computedLanguage = useSelector((state: RootState) => state.settings.computedLanguage); + const computedLanguage = useSelector(selectComputedLanguage); const theme = useSelector(selectTheme);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/providers/AppProviders.tsx` around lines 115 - 116, Add a new selector selectComputedLanguage returning state.settings.computedLanguage (signature: (state: RootState) => state.settings.computedLanguage) in the app settings selectors module, then replace the inline useSelector call that reads computedLanguage in AppProviders (the useSelector((state: RootState) => state.settings.computedLanguage) line) with useSelector(selectComputedLanguage) to match the existing use of selectTheme and keep selectors consistent for refactoring.src/vite-env.d.ts (1)
11-16: Environment vars marked optional but used without null checks—type definitions should reflect conditional requirements.The optional marking of
VITE_API_GATEWAY,VITE_WS_GATEWAY, andVITE_SRV_STUDY_URIworks by architectural design: the env files provide them conditionally based on deployment mode. However, the code relies on environment discipline rather than type safety:
- Production (
.env): providesVITE_API_GATEWAY+VITE_WS_GATEWAY; authentication is enabled- Development (
.env.development): providesVITE_SRV_STUDY_URI; authentication is disabledThe code in
src/shared/api/auth.tsandsrc/shared/api/config.tsuses these without guards, assuming they exist. Whilesrc/features/topbar/api/fetch-about.tscorrectly branches based onVITE_USE_AUTHENTICATION, the typing still marks all three as optional.To improve robustness and clarity, consider either:
- Documenting the conditional requirements (e.g., separate type definitions per environment), or
- Adding runtime validation to catch misconfiguration early
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vite-env.d.ts` around lines 11 - 16, ImportMetaEnv currently marks VITE_API_GATEWAY, VITE_WS_GATEWAY and VITE_SRV_STUDY_URI optional but code in src/shared/api/auth.ts and src/shared/api/config.ts uses them unguarded; add a small runtime validation function (e.g., validateEnv or assertRequiredEnv) that checks import.meta.env.VITE_USE_AUTHENTICATION and then enforces presence of VITE_API_GATEWAY and VITE_WS_GATEWAY when authentication is enabled, or VITE_SRV_STUDY_URI when disabled, throwing a clear Error with the missing variable name; call this validator early (module init or app startup, and from auth.ts/config.ts before using the vars) so misconfiguration fails fast.src/features/topbar/hooks/use-parameter-state.ts (1)
15-17: Tighten the key type instead of casting.
AppStateKeyalso includescomputedLanguage, but this hook always persists throughupdateConfigParameter. NarrowingKto the server-backed parameter names removes theas stringescape hatch and prevents invalid calls at compile time.Possible refactor
-import { AppStateKey, SettingsState } from '@/features/app-settings/store/types'; +import { SettingsState } from '@/features/app-settings/store/types'; +import { PARAM_LANGUAGE, PARAM_THEME } from '@/shared/config/parameters'; + +type PersistedParameterKey = typeof PARAM_LANGUAGE | typeof PARAM_THEME; -export function useParameterState<K extends AppStateKey>( +export function useParameterState<K extends PersistedParameterKey>( paramName: K ): [SettingsState[K], (value: SettingsState[K]) => void] { @@ - updateConfigParameter(paramName, value as string).catch((error) => { + updateConfigParameter(paramName, value).catch((error) => {Also applies to: 29-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/topbar/hooks/use-parameter-state.ts` around lines 15 - 17, The generic on useParameterState is too broad (K extends AppStateKey) and forces casting; restrict K to the server-backed parameter keys used with updateConfigParameter by introducing/using a narrower union type (e.g., ServerBackedAppStateKey) instead of AppStateKey, update the signature export function useParameterState<K extends ServerBackedAppStateKey>(paramName: K): [SettingsState[K], (value: SettingsState[K]) => void], and remove the `as string` escape hatch so calls to updateConfigParameter and SettingsState indexing are type-safe and invalid keys are rejected at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 83: Update the package.json test typings to match Jest's major version:
change the `@types/jest` dependency to a 30.x release (e.g., ^30.0.0) or remove
`@types/jest` if you no longer need it; ensure the dependency entry for
"@types/jest" aligns with "jest" (jest at ^30.1.3) so TypeScript test globals
and matchers resolve correctly and run npm/yarn install to update the lockfile.
- Around line 41-42: The repository contains two Jest configuration sources;
remove the "jest" configuration block from package.json so Jest uses the single
jest.config.ts file (keep the existing "test" and "test:coverage" npm scripts),
or alternatively delete/disable jest.config.ts and ensure the npm scripts pass
--config to package.json if you intend to keep that block; pick one approach and
eliminate the duplicate configuration (referenced symbols: the "jest" block in
package.json, the "test" and "test:coverage" npm scripts, and jest.config.ts).
In `@src/features/topbar/hooks/use-app-top-bar.ts`:
- Around line 33-37: The logo handler currently uses SPA navigation (onLogoClick
-> navigate('/')), which only rewinds the router; change onLogoClick to perform
an external navigation to the dispatcher root instead by using
window.location.replace or window.location.assign with the Vite dispatcher URL
(e.g. import.meta.env.VITE_DISPATCHER_URL || '/'), so replace the navigate call
inside the useCallback for onLogoClick with a window.location navigation that
uses the Vite env var.
In `@src/features/topbar/hooks/use-parameter-state.ts`:
- Around line 26-37: The rollback currently uses the stale captured
paramGlobalState and can overwrite a newer selection; modify
handleChangeParamLocalState to record the attempted value (e.g. requestedValue)
and use a ref to track the latest local/store value (create latestLocalRef via
useRef and update it whenever setParamLocalState or paramGlobalState changes).
In the updateConfigParameter(...).catch handler, only perform the rollback
(setParamLocalState(paramGlobalStateRef.current) or similar) if
latestLocalRef.current still equals the requestedValue (meaning the user hasn't
changed it since this request started); reference handleChangeParamLocalState,
setParamLocalState, updateConfigParameter and paramGlobalState when implementing
this guard and ensure the ref is kept up-to-date.
In `@src/features/topbar/hooks/use-top-bar-apps.ts`:
- Around line 12-21: The effect currently treats undefined as a valid user
because it only checks `user !== null`; update the guard in useTopBarApps to
explicitly ensure a user exists (e.g., check `user != null` or `if (!user)
return;`) before calling `fetchAppsAndUrls()`. Also make the async call robust
by adding error handling around `fetchAppsAndUrls()` (use an async IIFE or
attach .catch) and handle failures (log the error and/or call
`setAppsAndUrls([])`), referencing the `useTopBarApps` hook, the
`fetchAppsAndUrls` call, and the `setAppsAndUrls` state updater so you can
locate and modify the exact lines.
In `@src/shared/api/auth.ts`:
- Around line 24-28: The error message in fetchValidateUser uses template
interpolation of user which yields [object Object]; update the Promise.reject
call in fetchValidateUser to include a useful representation of the user (e.g.,
JSON.stringify(user) or a sanitized subset like user.profile.sub) and guard
against stringify failures (try/catch or fallback to String(user)) so the Error
contains clear debug info about the missing user.profile.sub.
- Around line 35-38: The comment says a successful response can be 200 or 204
but the promise handler only treats 200 as success; update the check in the
Response handler (the ".then((response: Response) => { ... })" callback) to
consider 204 as success too (e.g. change the condition to response.status ===
200 || response.status === 204) or alternatively make the comment accurate by
removing "204" if it's not a valid success code.
- Around line 48-50: The fetch in fetchIdpSettings currently uses a bare
relative path which breaks on sub-path deployments; update fetchIdpSettings to
prefix the resource with import.meta.env.BASE_URL (i.e.,
fetch(`${import.meta.env.BASE_URL}idpSettings.json`)) so the URL resolves
correctly, and apply the same change to the similar loader in
src/shared/api/metadata.ts that fetches env.json so both functions use
import.meta.env.BASE_URL when building their fetch URLs.
In `@src/shared/api/config.ts`:
- Around line 22-26: connectNotificationsWsUpdateConfig currently builds the
websocket URL by concatenating onto document.baseURI which can produce
route-relative or double-prefixed URLs; instead construct the base gateway URL
using new URL(VITE_WS_GATEWAY, document.baseURI) (or the equivalent env const
used in this module) to get an absolute origin, then build the full websocket
endpoint by appending PREFIX_CONFIG_NOTIFICATION_WS + '/notify' and setting
search params (appName and later access_token) via URL.searchParams; update the
creation of ReconnectingWebSocket in connectNotificationsWsUpdateConfig to use
this canonical URL and pass a callback that adds access_token via
URL.searchParams to avoid route-relative or duplicated prefixes.
In `@src/shared/api/http.ts`:
- Around line 55-56: tokenCopy may be null which sets an invalid "Authorization:
Bearer null" header; update the logic around tokenCopy (computed from token ||
getToken()) and the call to initCopy.headers.append('Authorization', ...) so you
only append the header when a real token exists (e.g., if (tokenCopy)
initCopy.headers.append(...)) or throw/return an explicit authentication error
when a token is required; change code in the block that computes tokenCopy and
calls initCopy.headers.append to guard against null from getToken().
In `@src/shared/api/metadata.ts`:
- Around line 14-15: The fetchEnv function currently does a route-relative
fetch('env.json') which fails on nested routes; update fetchEnv to build the
env.json URL from the app base (e.g. use document.baseURI or
import.meta.env.BASE_URL) before calling fetch so the resource is always
resolved from the app root; modify the fetch call in fetchEnv (returning
Promise<EnvJson>) to use new URL('env.json', document.baseURI) (or equivalent
base constant) so env.json is correctly loaded regardless of current route.
---
Outside diff comments:
In `@src/app/App.tsx`:
- Around line 113-127: The code currently treats any non-'true'
VITE_USE_AUTHENTICATION as dev-mode (fail-open); change to explicitly validate
the env var: read import.meta.env.VITE_USE_AUTHENTICATION into a local value and
branch only on exact 'true' (call initializeAuthenticationProd(...)) or exact
'false' (call initializeAuthenticationDev(...)); if the value is undefined or
any other string, throw or dispatch a clear configuration error and abort
startup so misconfiguration is visible. Reference initializeAuthenticationProd,
initializeAuthenticationDev, and VITE_USE_AUTHENTICATION when making the check.
In `@src/index.tsx`:
- Around line 13-22: The file that mounts the app (contains createRoot,
AppProviders, document.write and the throw new Error('No root container found'))
is dead code and should be removed or clearly documented; either delete that
file entirely and ensure no imports/reference remain, or keep it but add a
top-level comment explaining its legacy purpose and update project config/docs
to point to the actual entry (main.tsx equivalent); after removal, run a quick
project build to confirm no residual references to createRoot/AppProviders
remain.
---
Duplicate comments:
In `@vite.config.ts`:
- Around line 1-5: Add the required license header at the very top of
vite.config.ts (above the imports) to satisfy the CI header check; update the
file containing the imports (path, defineConfig, react, tsconfigPaths, svgr) by
prepending the project's standard license comment block exactly as used in other
source files so the header-checker recognizes it.
---
Nitpick comments:
In `@src/app/App.test.tsx`:
- Line 12: The test import of act is using the deprecated source; update the
import in App.test.tsx to pull act from React instead of react-dom/test-utils —
replace the import statement that references act from 'react-dom/test-utils'
with an import from 'react' (keeping any other named imports intact) so tests
use React's built-in act API.
In `@src/app/providers/AppProviders.tsx`:
- Around line 115-116: Add a new selector selectComputedLanguage returning
state.settings.computedLanguage (signature: (state: RootState) =>
state.settings.computedLanguage) in the app settings selectors module, then
replace the inline useSelector call that reads computedLanguage in AppProviders
(the useSelector((state: RootState) => state.settings.computedLanguage) line)
with useSelector(selectComputedLanguage) to match the existing use of
selectTheme and keep selectors consistent for refactoring.
In `@src/app/router/AppRouter.tsx`:
- Around line 20-24: Replace the hardcoded "Connected" string inside the Box →
Typography block with a react-intl FormattedMessage; import FormattedMessage if
missing and use a descriptive translation id (e.g.,
"connection.status.connected") and a defaultMessage of "Connected" so the string
flows through the app's i18n system and matches other usages in AppRouter.tsx.
- Line 28: Replace the raw hardcoded <h1> used for the "/logout-callback" Route
in AppRouter with the same styled MUI components and i18n pattern used
elsewhere: render a MUI Container/Box (or the app's standard Page wrapper) and a
Typography component, and use FormattedMessage (or intl.formatMessage) with a
new message id like "logoutCallback.error" to provide the text instead of the
hardcoded English string; update the route's element to return this component so
styling and localization match the rest of the app.
- Line 8: Remove the unused default React import from AppRouter.tsx since the
project uses the new JSX transform; keep (or add) only the named type import for
FunctionComponent if the component signature uses it. Replace "import React, {
FunctionComponent } from 'react';" with "import { FunctionComponent } from
'react';" (or remove the import entirely if FunctionComponent is not used) and
ensure the AppRouter component type annotation still compiles.
In `@src/app/store/index.ts`:
- Around line 12-15: Add a short comment above the middleware configuration
explaining why serializableCheck is set to false: note that
getDefaultMiddleware's serializableCheck is disabled because the Redux store
(e.g., AuthState) intentionally contains non-serializable values like Error
objects or class instances that would otherwise trigger warnings; reference the
middleware configuration (middleware: (getDefaultMiddleware) =>
getDefaultMiddleware({ serializableCheck: false })) and mention the specific
non-serializable types stored so future maintainers understand the rationale and
can revisit if store shape changes.
- Around line 18-19: There are two competing RootState definitions: the exported
RootState here (as ReturnType<typeof store.getState>) and the explicit RootState
exported from reducer (RootState); remove the duplicate by re-exporting the
canonical type from reducer instead of redefining it here — replace the local
RootState export with a direct re-export of the reducer's RootState and keep
AppDispatch as typeof store.dispatch (or re-export AppDispatch from wherever
it's already defined) so consumers import a single source of truth for
RootState.
In `@src/features/app-settings/store/actions.ts`:
- Around line 22-29: The LanguageAction type and selectLanguage function should
use the literal property name "language" instead of the computed key
PARAM_LANGUAGE to match ThemeAction/ComputedLanguageAction and avoid implicit
coupling; update LanguageAction to { language: SettingsState['language'] } and
change the object returned by selectLanguage to { type: SELECT_LANGUAGE,
language } while keeping SELECT_LANGUAGE, PARAM_LANGUAGE, SettingsState and
selectLanguage signatures unchanged.
In `@src/features/app-settings/store/settings-slice.ts`:
- Around line 44-46: The reducer handling SELECT_THEME (builder.addCase for
SELECT_THEME in settings-slice.ts) contains a side effect by calling
saveLocalStorageTheme(state.theme); remove that call from the reducer and
instead persist the theme from an action creator or middleware: update the
ThemeAction/selector flow so the action that dispatches SELECT_THEME also calls
saveLocalStorageTheme (or create a small middleware/thunk that listens for
SELECT_THEME and writes to localStorage). Ensure you keep state.theme =
action.theme in the reducer and move all persistence logic to the action creator
or a middleware that references SELECT_THEME and saveLocalStorageTheme.
In `@src/features/topbar/hooks/use-parameter-state.ts`:
- Around line 15-17: The generic on useParameterState is too broad (K extends
AppStateKey) and forces casting; restrict K to the server-backed parameter keys
used with updateConfigParameter by introducing/using a narrower union type
(e.g., ServerBackedAppStateKey) instead of AppStateKey, update the signature
export function useParameterState<K extends ServerBackedAppStateKey>(paramName:
K): [SettingsState[K], (value: SettingsState[K]) => void], and remove the `as
string` escape hatch so calls to updateConfigParameter and SettingsState
indexing are type-safe and invalid keys are rejected at compile time.
In `@src/shared/api/http.ts`:
- Around line 72-74: The backendFetchJson function returns Promise<unknown>,
forcing callers to cast; make it generic by adding a type parameter T (e.g.
export function backendFetchJson<T = unknown>(url: Url, init?: InitRequest,
token?: Token): Promise<T>) and propagate that type when parsing the body (use
Response.json() cast or typed return so the function returns Promise<T>),
keeping the default T = unknown to preserve backward compatibility; update any
call sites if needed to supply the concrete type.
In `@src/shared/config/parameters.ts`:
- Line 11: The AppConfigParameter union (AppConfigParameter in parameters.ts)
currently includes a bare string which collapses the union to string and loses
literal specificity (PARAM_THEME | PARAM_LANGUAGE | string). Fix by either
removing the trailing "| string" to make AppConfigParameter a strict union of
the known parameter literals (PARAM_THEME | PARAM_LANGUAGE) if only known keys
should be allowed, or implement a branded/templated pattern to allow arbitrary
strings while preserving autocomplete (e.g., use a branded type or a
template-literal wrapper like `string & { __brand?: "AppConfigParameter" }` or
`\`${typeof PARAM_THEME | typeof PARAM_LANGUAGE}\` | (string & {})`) so editors
still suggest the known constants; update any usages that relied on the previous
loose string type accordingly.
In `@src/vite-env.d.ts`:
- Around line 11-16: ImportMetaEnv currently marks VITE_API_GATEWAY,
VITE_WS_GATEWAY and VITE_SRV_STUDY_URI optional but code in
src/shared/api/auth.ts and src/shared/api/config.ts uses them unguarded; add a
small runtime validation function (e.g., validateEnv or assertRequiredEnv) that
checks import.meta.env.VITE_USE_AUTHENTICATION and then enforces presence of
VITE_API_GATEWAY and VITE_WS_GATEWAY when authentication is enabled, or
VITE_SRV_STUDY_URI when disabled, throwing a clear Error with the missing
variable name; call this validator early (module init or app startup, and from
auth.ts/config.ts before using the vars) so misconfiguration fails fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ee93481-58a1-4991-a452-c81af58d450d
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/assets/images/powsybl_logo.svgis excluded by!**/*.svg
📒 Files selected for processing (52)
.env.env.developmentindex.htmljest.config.tsjest.setup.tspackage.jsonsrc/app/App.test.tsxsrc/app/App.tsxsrc/app/config/config.tssrc/app/layout/AppLayout.tsxsrc/app/layout/AppTopBar.tsxsrc/app/providers/AppProviders.tsxsrc/app/router/AppRouter.tsxsrc/app/store/index.tssrc/app/store/reducer.tssrc/app/styles/index.csssrc/components/app-top-bar.tsxsrc/features/app-settings/store/actions.tssrc/features/app-settings/store/local-storage.tssrc/features/app-settings/store/selectors.tssrc/features/app-settings/store/settings-slice.tssrc/features/app-settings/store/types.tssrc/features/app-settings/ui/ParametersDialog.tsxsrc/features/auth/model/auth-slice.tssrc/features/auth/model/selectors.tssrc/features/auth/model/session-slice.tssrc/features/auth/model/types.tssrc/features/topbar/api/fetch-about.tssrc/features/topbar/hooks/use-app-top-bar.tssrc/features/topbar/hooks/use-parameter-state.tssrc/features/topbar/hooks/use-top-bar-apps.tssrc/features/topbar/hooks/use-top-bar-version.tssrc/index.tsxsrc/main.tsxsrc/redux/reducer.tssrc/setup-tests.jssrc/setupProxy.jssrc/shared/api/auth.tssrc/shared/api/config.tssrc/shared/api/http.tssrc/shared/api/metadata.tssrc/shared/config/parameters.tssrc/shared/lib/error.tssrc/shared/lib/language.tssrc/shared/translations/en/common.jsonsrc/shared/translations/fr/common.jsonsrc/types/module-emotion.d.tssrc/types/module-mui.d.tssrc/utils/rest-api.tssrc/vite-env.d.tstsconfig.jsonvite.config.ts
💤 Files with no reviewable changes (4)
- src/redux/reducer.ts
- src/components/app-top-bar.tsx
- src/setupProxy.js
- src/utils/rest-api.ts
Comment on lines
+41
to
+42
| "test": "jest", | ||
| "test:coverage": "jest --coverage", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
jq '.jest // empty' package.json
fd 'jest\.config\.(ts|js|cjs|mjs)$'Repository: gridsuite/gridapp-template
Length of output: 278
Consolidate Jest configuration to a single source.
The repository has both a "jest" block in package.json and a standalone jest.config.ts file. Running jest with two config sources will cause it to fail or behave unpredictably. Remove the "jest" block from package.json and keep only jest.config.ts, or if using package.json as the source, remove jest.config.ts and invoke Jest with --config if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 41 - 42, The repository contains two Jest
configuration sources; remove the "jest" configuration block from package.json
so Jest uses the single jest.config.ts file (keep the existing "test" and
"test:coverage" npm scripts), or alternatively delete/disable jest.config.ts and
ensure the npm scripts pass --config to package.json if you intend to keep that
block; pick one approach and eliminate the duplicate configuration (referenced
symbols: the "jest" block in package.json, the "test" and "test:coverage" npm
scripts, and jest.config.ts).
| "eslint-config-prettier": "^9.1.0", | ||
| "eslint-plugin-prettier": "^4.2.1", | ||
| "http-proxy-middleware": "^2.0.6", | ||
| "jest": "^30.1.3", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
jq -r '.devDependencies.jest, .devDependencies["@types/jest"]' package.jsonRepository: gridsuite/gridapp-template
Length of output: 85
Align Jest and @types/jest major versions.
Jest is at ^30.1.3 (line 83) but @types/jest remains at ^27.5.2 (line 74). This major version mismatch can break TypeScript test globals and matchers.
Upgrade @types/jest to ^30.x.x or explicitly remove the @types/jest dependency if no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 83, Update the package.json test typings to match
Jest's major version: change the `@types/jest` dependency to a 30.x release (e.g.,
^30.0.0) or remove `@types/jest` if you no longer need it; ensure the dependency
entry for "@types/jest" aligns with "jest" (jest at ^30.1.3) so TypeScript test
globals and matchers resolve correctly and run npm/yarn install to update the
lockfile.
Comment on lines
+33
to
+37
| const onLogoutClick = useCallback( | ||
| () => logout(dispatch, props.userManager.instance), | ||
| [dispatch, props.userManager] | ||
| ); | ||
| const onLogoClick = useCallback(() => navigate('/', { replace: true }), [navigate]); |
There was a problem hiding this comment.
Logo click changed platform navigation.
The previous top bar sent the logo to the dispatcher root; navigate('/') now only rewinds the current SPA and behaves differently under subpath deployments. If this change wasn't intentional, wire the Vite equivalent of the dispatcher URL here and keep the external navigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/topbar/hooks/use-app-top-bar.ts` around lines 33 - 37, The logo
handler currently uses SPA navigation (onLogoClick -> navigate('/')), which only
rewinds the router; change onLogoClick to perform an external navigation to the
dispatcher root instead by using window.location.replace or
window.location.assign with the Vite dispatcher URL (e.g.
import.meta.env.VITE_DISPATCHER_URL || '/'), so replace the navigate call inside
the useCallback for onLogoClick with a window.location navigation that uses the
Vite env var.
Comment on lines
+26
to
+37
| const handleChangeParamLocalState = useCallback( | ||
| (value: SettingsState[K]) => { | ||
| setParamLocalState(value); | ||
| updateConfigParameter(paramName, value as string).catch((error) => { | ||
| setParamLocalState(paramGlobalState); | ||
| snackError({ | ||
| messageTxt: error.message, | ||
| headerId: 'paramsChangingError', | ||
| }); | ||
| }); | ||
| }, | ||
| [paramName, snackError, paramGlobalState] |
There was a problem hiding this comment.
Rollback can clobber a newer selection.
paramGlobalState is captured when the request starts. If the user changes the parameter again before an earlier request fails, this catch path can overwrite the newer choice with an older store value. Track the latest store value in a ref and only roll back if the failed request still matches the current local value.
Possible fix
-import { useCallback, useEffect, useState } from 'react';
+import { useCallback, useEffect, useRef, useState } from 'react';
@@
const { snackError } = useSnackMessage();
const paramGlobalState = useSelector((state: RootState) => state.settings[paramName]);
const [paramLocalState, setParamLocalState] = useState(paramGlobalState);
+ const latestGlobalStateRef = useRef(paramGlobalState);
useEffect(() => {
+ latestGlobalStateRef.current = paramGlobalState;
setParamLocalState(paramGlobalState);
}, [paramGlobalState]);
@@
(value: SettingsState[K]) => {
setParamLocalState(value);
updateConfigParameter(paramName, value as string).catch((error) => {
- setParamLocalState(paramGlobalState);
+ setParamLocalState((current) =>
+ current === value ? latestGlobalStateRef.current : current
+ );
snackError({
messageTxt: error.message,
headerId: 'paramsChangingError',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/topbar/hooks/use-parameter-state.ts` around lines 26 - 37, The
rollback currently uses the stale captured paramGlobalState and can overwrite a
newer selection; modify handleChangeParamLocalState to record the attempted
value (e.g. requestedValue) and use a ref to track the latest local/store value
(create latestLocalRef via useRef and update it whenever setParamLocalState or
paramGlobalState changes). In the updateConfigParameter(...).catch handler, only
perform the rollback (setParamLocalState(paramGlobalStateRef.current) or
similar) if latestLocalRef.current still equals the requestedValue (meaning the
user hasn't changed it since this request started); reference
handleChangeParamLocalState, setParamLocalState, updateConfigParameter and
paramGlobalState when implementing this guard and ensure the ref is kept
up-to-date.
Comment on lines
+12
to
+21
| export function useTopBarApps(user?: SessionState['user']): MetadataJson[] { | ||
| const [appsAndUrls, setAppsAndUrls] = useState<MetadataJson[]>([]); | ||
|
|
||
| useEffect(() => { | ||
| if (user !== null) { | ||
| fetchAppsAndUrls().then((res) => { | ||
| setAppsAndUrls(res); | ||
| }); | ||
| } | ||
| }, [user]); |
There was a problem hiding this comment.
Guard condition does not account for undefined user.
The parameter user?: SessionState['user'] is optional, meaning it can be undefined, null, or a user object. The guard user !== null on line 16 will pass when user is undefined (since undefined !== null evaluates to true), causing fetchAppsAndUrls() to be called unintentionally when no user is provided.
Additionally, the async call lacks error handling—a rejected promise will cause an unhandled rejection.
🐛 Proposed fix
export function useTopBarApps(user?: SessionState['user']): MetadataJson[] {
const [appsAndUrls, setAppsAndUrls] = useState<MetadataJson[]>([]);
useEffect(() => {
- if (user !== null) {
- fetchAppsAndUrls().then((res) => {
- setAppsAndUrls(res);
- });
+ if (user != null) {
+ fetchAppsAndUrls()
+ .then((res) => {
+ setAppsAndUrls(res);
+ })
+ .catch((error) => {
+ console.error('Failed to fetch apps and urls:', error);
+ });
}
}, [user]);
return appsAndUrls;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/topbar/hooks/use-top-bar-apps.ts` around lines 12 - 21, The
effect currently treats undefined as a valid user because it only checks `user
!== null`; update the guard in useTopBarApps to explicitly ensure a user exists
(e.g., check `user != null` or `if (!user) return;`) before calling
`fetchAppsAndUrls()`. Also make the async call robust by adding error handling
around `fetchAppsAndUrls()` (use an async IIFE or attach .catch) and handle
failures (log the error and/or call `setAppsAndUrls([])`), referencing the
`useTopBarApps` hook, the `fetchAppsAndUrls` call, and the `setAppsAndUrls`
state updater so you can locate and modify the exact lines.
Comment on lines
+35
to
+38
| .then((response: Response) => { | ||
| //if the response is ok, the responseCode will be either 200 or 204 otherwise it's a Http error and it will be caught | ||
| return response.status === 200; | ||
| }) |
There was a problem hiding this comment.
Comment mentions 204 but code only treats 200 as success.
The comment on line 36 states "the responseCode will be either 200 or 204" but line 37 only returns true for status 200. If 204 is a valid success response, update the condition; otherwise, correct the comment.
📝 Proposed fix (if 204 is also valid)
.then((response: Response) => {
- //if the response is ok, the responseCode will be either 200 or 204 otherwise it's a Http error and it will be caught
- return response.status === 200;
+ // HEAD request returns 200 (user exists) or triggers error handling below
+ return response.ok;
})📝 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
| .then((response: Response) => { | |
| //if the response is ok, the responseCode will be either 200 or 204 otherwise it's a Http error and it will be caught | |
| return response.status === 200; | |
| }) | |
| .then((response: Response) => { | |
| // HEAD request returns 200 (user exists) or triggers error handling below | |
| return response.ok; | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/api/auth.ts` around lines 35 - 38, The comment says a successful
response can be 200 or 204 but the promise handler only treats 200 as success;
update the check in the Response handler (the ".then((response: Response) => {
... })" callback) to consider 204 as success too (e.g. change the condition to
response.status === 200 || response.status === 204) or alternatively make the
comment accurate by removing "204" if it's not a valid success code.
Comment on lines
+48
to
+50
| export function fetchIdpSettings(): Promise<IdpSettings> { | ||
| return fetch('idpSettings.json').then((res) => res.json()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a base path configured and how idpSettings.json is typically resolved
rg -n "idpSettings\.json" --type=ts --type=html -C2Repository: gridsuite/gridapp-template
Length of output: 299
🏁 Script executed:
# Find idpSettings.json location and check project structure
find . -name "idpSettings.json" -o -name "*.json" -type f | grep -E "(public|assets|src)" | head -20Repository: gridsuite/gridapp-template
Length of output: 265
🏁 Script executed:
# Check for other fetch calls to see if there's a pattern
rg "fetch\(" --type=ts --type=tsx -A1 | head -50Repository: gridsuite/gridapp-template
Length of output: 97
🏁 Script executed:
# Check for base path configuration
rg "basename|BASE_PATH|PUBLIC_URL|base.*path" --type=ts --type=tsx --type=js -i | head -30Repository: gridsuite/gridapp-template
Length of output: 97
🏁 Script executed:
# Check the public folder structure
ls -la public/ 2>/dev/null || ls -la src/ | head -20Repository: gridsuite/gridapp-template
Length of output: 366
🏁 Script executed:
# Check build configuration files
ls -la | grep -E "vite|webpack|package.json|tsconfig"Repository: gridsuite/gridapp-template
Length of output: 264
🏁 Script executed:
# Check vite.config.ts for base path configuration
cat vite.config.tsRepository: gridsuite/gridapp-template
Length of output: 1178
🏁 Script executed:
# Check how env.json is loaded (might reveal the pattern)
rg "env\.json" --type=ts --type=js -C3Repository: gridsuite/gridapp-template
Length of output: 627
🏁 Script executed:
# Check index.html for base tag or other asset loading patterns
cat src/index.html 2>/dev/null || cat index.html 2>/dev/null || cat public/index.html 2>/dev/nullRepository: gridsuite/gridapp-template
Length of output: 883
🏁 Script executed:
# Check if there's any usage of import.meta.env.BASE_URL
rg "import\.meta\.env|BASE_URL" --type=tsRepository: gridsuite/gridapp-template
Length of output: 885
Use import.meta.env.BASE_URL to resolve the relative path correctly for sub-path deployments.
fetch('idpSettings.json') with a relative path will fail if the app is deployed under a sub-path (e.g., /app/). Since import.meta.env.BASE_URL is available and Vite is configured with base: './', use fetch(\${import.meta.env.BASE_URL}idpSettings.json`)instead. Note:env.jsonloaded insrc/shared/api/metadata.ts` has the same issue and should be fixed consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/api/auth.ts` around lines 48 - 50, The fetch in fetchIdpSettings
currently uses a bare relative path which breaks on sub-path deployments; update
fetchIdpSettings to prefix the resource with import.meta.env.BASE_URL (i.e.,
fetch(`${import.meta.env.BASE_URL}idpSettings.json`)) so the URL resolves
correctly, and apply the same change to the similar loader in
src/shared/api/metadata.ts that fetches env.json so both functions use
import.meta.env.BASE_URL when building their fetch URLs.
Comment on lines
+22
to
+26
| export function connectNotificationsWsUpdateConfig(): ReconnectingWebSocket { | ||
| const webSocketBaseUrl = document.baseURI.replace(/^http:\/\//, 'ws://').replace(/^https:\/\//, 'wss://'); | ||
| const webSocketUrl = `${webSocketBaseUrl}${PREFIX_CONFIG_NOTIFICATION_WS}/notify?appName=${APP_NAME}`; | ||
|
|
||
| const reconnectingWebSocket = new ReconnectingWebSocket(() => `${webSocketUrl}&access_token=${getToken()}`); |
There was a problem hiding this comment.
Compose the websocket URL from the gateway value.
document.baseURI includes the current route, so callback/nested URLs produce route-relative websocket endpoints. It also double-prefixes the URL when VITE_WS_GATEWAY is already absolute. Build the URL with new URL(...) from the gateway instead of concatenating onto the document URL.
Possible fix
export function connectNotificationsWsUpdateConfig(): ReconnectingWebSocket {
- const webSocketBaseUrl = document.baseURI.replace(/^http:\/\//, 'ws://').replace(/^https:\/\//, 'wss://');
- const webSocketUrl = `${webSocketBaseUrl}${PREFIX_CONFIG_NOTIFICATION_WS}/notify?appName=${APP_NAME}`;
+ const httpUrl = new URL(`${PREFIX_CONFIG_NOTIFICATION_WS}/notify`, window.location.origin);
+ httpUrl.searchParams.set('appName', APP_NAME);
+ const webSocketUrl = httpUrl.toString().replace(/^http/, 'ws');
- const reconnectingWebSocket = new ReconnectingWebSocket(() => `${webSocketUrl}&access_token=${getToken()}`);
+ const reconnectingWebSocket = new ReconnectingWebSocket(() => {
+ const url = new URL(webSocketUrl);
+ url.searchParams.set('access_token', getToken() ?? '');
+ return url.toString();
+ });
reconnectingWebSocket.onopen = function (event: Event) {
console.info(`Connected Websocket update config ui ${webSocketUrl} ...`);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/api/config.ts` around lines 22 - 26,
connectNotificationsWsUpdateConfig currently builds the websocket URL by
concatenating onto document.baseURI which can produce route-relative or
double-prefixed URLs; instead construct the base gateway URL using new
URL(VITE_WS_GATEWAY, document.baseURI) (or the equivalent env const used in this
module) to get an absolute origin, then build the full websocket endpoint by
appending PREFIX_CONFIG_NOTIFICATION_WS + '/notify' and setting search params
(appName and later access_token) via URL.searchParams; update the creation of
ReconnectingWebSocket in connectNotificationsWsUpdateConfig to use this
canonical URL and pass a callback that adds access_token via URL.searchParams to
avoid route-relative or duplicated prefixes.
Comment on lines
+55
to
+56
| const tokenCopy = token || getToken(); | ||
| initCopy.headers.append('Authorization', `Bearer ${tokenCopy}`); |
There was a problem hiding this comment.
Token may be null, resulting in an invalid Authorization: Bearer null header.
When token is not provided and getToken() returns null (e.g., user not logged in), tokenCopy will be null, causing the header to be set as "Bearer null". This could cause unexpected 401 responses or backend parsing issues.
Consider guarding against null tokens or throwing an error when authentication is required but unavailable:
🛡️ Proposed fix
function prepareRequest(init?: InitRequest, token?: Token): RequestInit {
if (!(typeof init == 'undefined' || typeof init == 'object')) {
throw new TypeError(`Argument 2 of backendFetch is not an object ${typeof init}`);
}
const initCopy: RequestInit = { ...init };
initCopy.headers = new Headers(initCopy.headers || {});
const tokenCopy = token || getToken();
- initCopy.headers.append('Authorization', `Bearer ${tokenCopy}`);
+ if (tokenCopy) {
+ initCopy.headers.append('Authorization', `Bearer ${tokenCopy}`);
+ }
return initCopy;
}📝 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 tokenCopy = token || getToken(); | |
| initCopy.headers.append('Authorization', `Bearer ${tokenCopy}`); | |
| function prepareRequest(init?: InitRequest, token?: Token): RequestInit { | |
| if (!(typeof init == 'undefined' || typeof init == 'object')) { | |
| throw new TypeError(`Argument 2 of backendFetch is not an object ${typeof init}`); | |
| } | |
| const initCopy: RequestInit = { ...init }; | |
| initCopy.headers = new Headers(initCopy.headers || {}); | |
| const tokenCopy = token || getToken(); | |
| if (tokenCopy) { | |
| initCopy.headers.append('Authorization', `Bearer ${tokenCopy}`); | |
| } | |
| return initCopy; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/api/http.ts` around lines 55 - 56, tokenCopy may be null which
sets an invalid "Authorization: Bearer null" header; update the logic around
tokenCopy (computed from token || getToken()) and the call to
initCopy.headers.append('Authorization', ...) so you only append the header when
a real token exists (e.g., if (tokenCopy) initCopy.headers.append(...)) or
throw/return an explicit authentication error when a token is required; change
code in the block that computes tokenCopy and calls initCopy.headers.append to
guard against null from getToken().
Comment on lines
+14
to
+15
| function fetchEnv(): Promise<EnvJson> { | ||
| return fetch('env.json').then((res: Response) => res.json()); |
There was a problem hiding this comment.
Resolve env.json from the app base.
fetch('env.json') is route-relative. A direct load on /sign-in-callback or any nested route will request the wrong file and break metadata bootstrap. Build the URL from the app base before fetching.
Possible fix
function fetchEnv(): Promise<EnvJson> {
- return fetch('env.json').then((res: Response) => res.json());
+ const envUrl = new URL(`${import.meta.env.BASE_URL}env.json`, window.location.origin).toString();
+ return fetch(envUrl).then((res: Response) => {
+ if (!res.ok) {
+ throw new Error(`Unable to load ${envUrl}: ${res.status}`);
+ }
+ return res.json();
+ });
}📝 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
| function fetchEnv(): Promise<EnvJson> { | |
| return fetch('env.json').then((res: Response) => res.json()); | |
| function fetchEnv(): Promise<EnvJson> { | |
| const envUrl = new URL(`${import.meta.env.BASE_URL}env.json`, window.location.origin).toString(); | |
| return fetch(envUrl).then((res: Response) => { | |
| if (!res.ok) { | |
| throw new Error(`Unable to load ${envUrl}: ${res.status}`); | |
| } | |
| return res.json(); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/api/metadata.ts` around lines 14 - 15, The fetchEnv function
currently does a route-relative fetch('env.json') which fails on nested routes;
update fetchEnv to build the env.json URL from the app base (e.g. use
document.baseURI or import.meta.env.BASE_URL) before calling fetch so the
resource is always resolved from the app root; modify the fetch call in fetchEnv
(returning Promise<EnvJson>) to use new URL('env.json', document.baseURI) (or
equivalent base constant) so env.json is correctly loaded regardless of current
route.
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.
PR Summary