[SOLID] Architecture refinement#14
Open
thangqp wants to merge 3 commits into
Open
Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors application configuration and authentication token handling by introducing a centralized configuration system, converting app parameter hooks from Redux-dependent to prop-based authentication, and updating the test infrastructure with a new context builder. ChangesConfiguration, Authentication & Hook Refactoring
Test Infrastructure Migration
Linting and Build Configuration
Suggested reviewers
🚥 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 |
![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 137 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 106 | 1 | 30 | 0 |
Click to see the invalid file list
- src/shared/api/study-api/study-api.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
eslint.config.js (1)
143-154: ⚡ Quick winConsider adding custom error messages for each zone restriction.
Adding a
messageproperty to each zone would provide clearer feedback when developers accidentally violate the architectural boundaries.💬 Suggested enhancement with custom messages
'import-x/no-restricted-paths': [ 'error', { zones: [ // shared must not import from features or app - { target: './src/shared', from: './src/features' }, - { target: './src/shared', from: './src/app' }, + { + target: './src/shared', + from: './src/features', + message: 'Shared modules cannot import from features. Shared must remain dependency-free.' + }, + { + target: './src/shared', + from: './src/app', + message: 'Shared modules cannot import from app. Shared must remain dependency-free.' + }, // features must not import from app - { target: './src/features', from: './src/app' }, + { + target: './src/features', + from: './src/app', + message: 'Features cannot import from app. Move shared logic to features or shared layers.' + }, ], }, ],The rule syntax is compatible with eslint-plugin-import-x v4.16.2 and the zone configuration format is properly supported.
🤖 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 `@eslint.config.js` around lines 143 - 154, The import-x/no-restricted-paths rule currently blocks imports between zones but omits human-friendly messages; update the zones array used by 'import-x/no-restricted-paths' to add a message string to each zone object (e.g., the entries with target './src/shared' from './src/features' and './src/app', and the entry with target './src/features' from './src/app') so ESLint reports a clear, custom error when a developer violates these architectural boundaries; keep the existing targets and from fields and only add concise, actionable 'message' values explaining why the import is disallowed.src/app/config/app-config.ts (1)
11-11: 💤 Low valueRemove unnecessary type assertion.
The
selectTokenfunction signature(state: StateWithAuthentication) => string | undefinedis compatible withTokenSelector = (state: unknown) => string | undefineddue to TypeScript's structural subtyping (contravariance on parameter types). Theas TokenSelectorassertion is unnecessary and could mask future signature drift.♻️ Simplify to implicit type compatibility
-configureParams({ appName: APP_NAME, tokenSelector: selectToken as TokenSelector }); +configureParams({ appName: APP_NAME, tokenSelector: selectToken });🤖 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/app/config/app-config.ts` at line 11, The code currently uses an unnecessary type assertion on selectToken when calling configureParams; remove the "as TokenSelector" cast so the call becomes configureParams({ appName: APP_NAME, tokenSelector: selectToken }); this relies on TypeScript's structural typing and avoids masking future signature drift—update the call site that references configureParams, APP_NAME and selectToken accordingly.src/app/providers/AppProviders.tsx (1)
19-20: ⚖️ Poor tradeoffFragile module initialization order dependency.
The side-effect import on line 19 must execute before line 20 because:
app/config/app-configcallsconfigureParams(...)at module loadapp/store/storecreates the store (line 31) and callsupdateConfigParams({ store })(line 33)updateConfigParamswill throw ifconfigureParamshasn't been called firstThis ordering is correct but fragile—swapping these imports would cause a runtime error. The ES module specification guarantees import order, but this implicit dependency is easy to break during refactoring.
Consider making initialization explicit:
- Move
configureParamscall to an exported initialization function inapp-config.ts- Call it explicitly from an app entry point before importing the store
- Or use a lazy store initialization pattern
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/providers/AppProviders.tsx` around lines 19 - 20, The current side-effect import of app/config/app-config (which runs configureParams at module load) is a fragile implicit dependency for app/store/store (which creates store and calls updateConfigParams); refactor to make initialization explicit by exporting a setup function (e.g., initAppConfig or configureParamsExplicit) from app-config that performs configureParams, then call that setup from a single app entry point before any import/use of store or useAppSelector, or alternatively implement a lazyStore initializer inside app/store/store that defers store creation until after configureParamsExplicit is called; update references to configureParams, app-config, store, useAppSelector, and updateConfigParams accordingly so initialization order is enforced explicitly rather than relying on side-effect import order.src/features/authentication/store/authentication.selectors.ts (1)
12-12: 💤 Low valueConsider adding explicit return type for clarity.
While TypeScript correctly infers
string | undefined, adding an explicit return type improves readability and prevents unintended signature changes, especially since this selector is used as aTokenSelectorin the centralized configuration.📝 Add explicit return type
-export const selectToken = (state: StateWithAuthentication) => selectUser(state)?.id_token; +export const selectToken = (state: StateWithAuthentication): string | undefined => + selectUser(state)?.id_token;🤖 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/features/authentication/store/authentication.selectors.ts` at line 12, The selector selectToken currently relies on inferred return type; update its signature to include an explicit return type (e.g., string | undefined) to match the TokenSelector expectation and avoid accidental signature changes—modify selectToken(state: StateWithAuthentication): string | undefined { ... } (ensure the return type aligns with the TokenSelector used in your centralized configuration).src/shared/config/config-params.ts (1)
28-33: ⚡ Quick winValidate store is not updated after initial registration.
updateConfigParamsallows replacing thestorereference after initialization. Since the store should only be set once (after creation instore.ts), consider preventing subsequent store updates to avoid accidental reconfigurations.🛡️ Proposed safeguard
export function updateConfigParams(config: Partial<ConfigParams>): void { if (configParams === undefined) { throw new Error('Config params not initialized. Call configureParams() before using modules.'); } + if (config.store && configParams.store) { + throw new Error('Store already registered. Cannot update store reference.'); + } Object.assign(configParams, config); }🤖 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/shared/config/config-params.ts` around lines 28 - 33, updateConfigParams currently allows replacing the existing store reference which can lead to accidental reconfiguration; modify updateConfigParams to detect when the incoming partial config contains a store and the existing configParams.store is already set, and in that case prevent overwriting (either by throwing an Error or by ignoring the store field) while still applying other allowed fields via Object.assign; refer to updateConfigParams and the configParams.store field (and the store creation in store.ts) when making the change.
🤖 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/features/top-bar/components/AppTopBar.tsx`:
- Line 38: The current authentication boolean uses a strict null check (const
isAuthenticated = user !== null) which treats undefined as authenticated; change
the derivation to a nullish check so only actual users are considered
authenticated (update the isAuthenticated assignment in AppTopBar.tsx to use
user != null to cover both null and undefined, referencing the user variable and
the isAuthenticated const).
In `@src/shared/api/config-api/config-api.type.ts`:
- Around line 27-29: The UpdateConfigParameterRequest allows invalid name/value
pairs because it uses a widened union; replace it with a discriminated-union
style definition that ties each name to its specific value type (matching the
existing ConfigParameter pattern). Concretely, redefine
UpdateConfigParameterRequest to map each AppParametersKey K to an object { name:
K; value: AppParameters[K] } and take the union of those entries (i.e., the
mapped type indexed over AppParametersKey) so that name and value are always
consistent; reference UpdateConfigParameterRequest, AppParametersKey,
AppParameters, and ConfigParameter when making the change.
In `@src/shared/config/config-params.ts`:
- Around line 46-48: Declare an explicit return type for getToken (e.g. unknown
| undefined) and guard against a missing store before calling the selector: call
getConfigParams(), extract cfg.tokenSelector and cfg.store, if cfg.store is
undefined return undefined immediately, otherwise call
cfg.tokenSelector(cfg.store.getState()) and return its result; reference
getToken, getConfigParams, tokenSelector and store.getState in your changes.
In `@vitest.setup.ts`:
- Around line 10-15: The createTestConfigParams() call must run before any
module side-effects in setup-msw; fix by moving the call into a small
side-effect module (e.g., initTestConfigParams) that imports and invokes
createTestConfigParams(), then import that new side-effect module before
importing setup-msw in vitest.setup.ts so initialization order guarantees are
upheld; update vitest.setup.ts to import initTestConfigParams (which calls
createTestConfigParams) first, then import setup-msw.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 143-154: The import-x/no-restricted-paths rule currently blocks
imports between zones but omits human-friendly messages; update the zones array
used by 'import-x/no-restricted-paths' to add a message string to each zone
object (e.g., the entries with target './src/shared' from './src/features' and
'./src/app', and the entry with target './src/features' from './src/app') so
ESLint reports a clear, custom error when a developer violates these
architectural boundaries; keep the existing targets and from fields and only add
concise, actionable 'message' values explaining why the import is disallowed.
In `@src/app/config/app-config.ts`:
- Line 11: The code currently uses an unnecessary type assertion on selectToken
when calling configureParams; remove the "as TokenSelector" cast so the call
becomes configureParams({ appName: APP_NAME, tokenSelector: selectToken }); this
relies on TypeScript's structural typing and avoids masking future signature
drift—update the call site that references configureParams, APP_NAME and
selectToken accordingly.
In `@src/app/providers/AppProviders.tsx`:
- Around line 19-20: The current side-effect import of app/config/app-config
(which runs configureParams at module load) is a fragile implicit dependency for
app/store/store (which creates store and calls updateConfigParams); refactor to
make initialization explicit by exporting a setup function (e.g., initAppConfig
or configureParamsExplicit) from app-config that performs configureParams, then
call that setup from a single app entry point before any import/use of store or
useAppSelector, or alternatively implement a lazyStore initializer inside
app/store/store that defers store creation until after configureParamsExplicit
is called; update references to configureParams, app-config, store,
useAppSelector, and updateConfigParams accordingly so initialization order is
enforced explicitly rather than relying on side-effect import order.
In `@src/features/authentication/store/authentication.selectors.ts`:
- Line 12: The selector selectToken currently relies on inferred return type;
update its signature to include an explicit return type (e.g., string |
undefined) to match the TokenSelector expectation and avoid accidental signature
changes—modify selectToken(state: StateWithAuthentication): string | undefined {
... } (ensure the return type aligns with the TokenSelector used in your
centralized configuration).
In `@src/shared/config/config-params.ts`:
- Around line 28-33: updateConfigParams currently allows replacing the existing
store reference which can lead to accidental reconfiguration; modify
updateConfigParams to detect when the incoming partial config contains a store
and the existing configParams.store is already set, and in that case prevent
overwriting (either by throwing an Error or by ignoring the store field) while
still applying other allowed fields via Object.assign; refer to
updateConfigParams and the configParams.store field (and the store creation in
store.ts) when making the change.
🪄 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: CHILL
Plan: Pro
Run ID: 5ff8315e-2b95-41d3-a17d-85ce7363df4e
📒 Files selected for processing (44)
eslint.config.jssrc/app/App.tsxsrc/app/config/app-config.tssrc/app/providers/AppProviders.tsxsrc/app/store/store.tssrc/features/app-parameters/__tests__/hooks/use-app-parameter-state.test.tssrc/features/app-parameters/__tests__/hooks/use-app-parameters-invalidation-listener.test.tssrc/features/app-parameters/__tests__/hooks/use-get-config-parameter-with-fallback.test.tssrc/features/app-parameters/hooks/use-app-parameter-state.tssrc/features/app-parameters/hooks/use-app-parameters-invalidation-listener.tssrc/features/app-parameters/hooks/use-get-config-parameter-with-fallback.tssrc/features/app-parameters/store/app-parameters.local-storage.tssrc/features/app-parameters/store/app-parameters.type.tssrc/features/authentication/store/authentication.selectors.tssrc/features/authentication/store/authentication.type.tssrc/features/process-config/__tests__/hooks/use-process-invalidation-listener.test.tssrc/features/process-config/__tests__/pages/ProcessConfigListPage.test.tsxsrc/features/process-config/hooks/use-process-invalidation-listener.tssrc/features/process/execute/__tests__/pages/ProcessExecutePage.test.tsxsrc/features/process/execute/components/ExecuteProcessResult.tsxsrc/features/process/results/__tests__/pages/ProcessResultsPage.test.tsxsrc/features/process/results/__tests__/pages/ProcessStepInfosPage.test.tsxsrc/features/process/results/models/process-result.tssrc/features/test-utils/create-base-context.tsxsrc/features/top-bar/api/get-servers-infos.tssrc/features/top-bar/components/AppNavBar.tsxsrc/features/top-bar/components/AppTopBar.tsxsrc/shared/api/config-api/config-api.local-storage.tssrc/shared/api/config-api/config-api.type.tssrc/shared/api/config-api/config.enhanced.tssrc/shared/api/monitor-api/monitor.enhanced.tssrc/shared/api/rtk-query/base-api.tssrc/shared/api/study-api/study-api.tssrc/shared/api/ws/config-ws.tssrc/shared/api/ws/monitor-ws.tssrc/shared/api/ws/ws-client.tssrc/shared/api/ws/ws.utils.tssrc/shared/config/config-params.tssrc/shared/store/rtk-query-error-middleware.tssrc/shared/store/state.type.tssrc/shared/test-utils/create-test-config-params.tssrc/shared/test-utils/msw/server.tssrc/shared/test-utils/msw/setup-msw.tsvitest.setup.ts
💤 Files with no reviewable changes (1)
- src/shared/api/ws/ws.utils.ts
| const navigate = useNavigate(); | ||
| const dispatch = useAppDispatch(); | ||
| const dispatch = useDispatch<AnyAppDispatch>(); | ||
| const isAuthenticated = user !== null; |
There was a problem hiding this comment.
Fix authentication boolean derivation for optional user.
Line 38 treats undefined as authenticated. Since user is optional, use a nullish check (!= null) so only real users are authenticated.
✅ Proposed fix
- const isAuthenticated = user !== null;
+ const isAuthenticated = user != null;📝 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 isAuthenticated = user !== null; | |
| const isAuthenticated = user != 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/features/top-bar/components/AppTopBar.tsx` at line 38, The current
authentication boolean uses a strict null check (const isAuthenticated = user
!== null) which treats undefined as authenticated; change the derivation to a
nullish check so only actual users are considered authenticated (update the
isAuthenticated assignment in AppTopBar.tsx to use user != null to cover both
null and undefined, referencing the user variable and the isAuthenticated
const).
Comment on lines
+27
to
+29
| export type UpdateConfigParameterRequest = { | ||
| name: AppParametersKey; | ||
| value: AppParameters[AppParametersKey]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current definition and inspect all usages of UpdateConfigParameterRequest.
set -euo pipefail
sed -n '1,120p' src/shared/api/config-api/config-api.type.ts
rg -n -C3 'UpdateConfigParameterRequest' srcRepository: gridsuite/gridmonitor-app
Length of output: 1492
🏁 Script executed:
#!/bin/bash
# Find all usages of UpdateConfigParameterRequest in the codebase
rg 'UpdateConfigParameterRequest' src --max-count=100 -B 2 -A 5Repository: gridsuite/gridmonitor-app
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Search for imports and usages of UpdateConfigParameterRequest
rg 'UpdateConfigParameterRequest' . --max-count=50
# Also search for files that might use it (config-api related files)
find src -name "*config*api*" -type f
# Check if it's used in service/API files
rg -l 'config.*api' src --max-count=20Repository: gridsuite/gridmonitor-app
Length of output: 923
🏁 Script executed:
#!/bin/bash
# Search for imports of UpdateConfigParameterRequest
rg 'import.*UpdateConfigParameterRequest' src
# Check config-base-api.ts which likely has API service methods
cat -n src/shared/api/config-api/config-base-api.ts
# Check for any reference to the type name in broader context
rg -i 'updateconfigparameterrequest' . --type tsRepository: gridsuite/gridmonitor-app
Length of output: 975
🏁 Script executed:
#!/bin/bash
# Check the enhanced config API file
cat -n src/shared/api/config-api/config.enhanced.ts
# Also check local storage implementation
cat -n src/shared/api/config-api/config-api.local-storage.tsRepository: gridsuite/gridmonitor-app
Length of output: 4476
🏁 Script executed:
#!/bin/bash
# Check the generated config API file
cat -n src/shared/api/config-api/config.generated.tsRepository: gridsuite/gridmonitor-app
Length of output: 1619
Improve type safety for UpdateConfigParameterRequest to prevent invalid name/value combinations.
The current type allows pairing incompatible values (e.g., PARAM_LANGUAGE with a theme value) due to the widened union. Although this type is not currently used in the codebase, standardizing it to a discriminated union pattern—matching the existing ConfigParameter type—ensures type safety if it's used in the future.
✅ Proposed type-safe fix
-export type UpdateConfigParameterRequest = {
- name: AppParametersKey;
- value: AppParameters[AppParametersKey];
-};
+export type UpdateConfigParameterRequest =
+ | { name: typeof PARAM_LANGUAGE; value: GsLang }
+ | { name: typeof PARAM_THEME; value: GsTheme };🤖 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/shared/api/config-api/config-api.type.ts` around lines 27 - 29, The
UpdateConfigParameterRequest allows invalid name/value pairs because it uses a
widened union; replace it with a discriminated-union style definition that ties
each name to its specific value type (matching the existing ConfigParameter
pattern). Concretely, redefine UpdateConfigParameterRequest to map each
AppParametersKey K to an object { name: K; value: AppParameters[K] } and take
the union of those entries (i.e., the mapped type indexed over AppParametersKey)
so that name and value are always consistent; reference
UpdateConfigParameterRequest, AppParametersKey, AppParameters, and
ConfigParameter when making the change.
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
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