fix(core): 17734 fixes for tokens, routing, sidebar, config caching #788
Conversation
WalkthroughThis update focuses on refining route and configuration management, enhancing error and logging functionality, and updating dependencies. Key changes include replacing Changes
Poem
Tip You can customize the tone of the comments in your PRsSpecify the tone of the comments in your PRs by configuring the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Bundle size diff
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (25)
- package.json (4 hunks)
- src/core/api_client/apiClient.ts (7 hunks)
- src/core/api_client/errors.ts (3 hunks)
- src/core/app/authHooks.ts (2 hunks)
- src/core/app/postAppInit.ts (2 hunks)
- src/core/auth/client/AuthClient.ts (3 hunks)
- src/core/auth/index.ts (1 hunks)
- src/core/config/loaders/appConfigLoader.ts (1 hunks)
- src/core/logical_layers/index.ts (1 hunks)
- src/core/metrics/app-metrics.ts (2 hunks)
- src/core/metrics/index.ts (1 hunks)
- src/core/postInit.ts (1 hunks)
- src/core/router/atoms/availableRoutes.ts (1 hunks)
- src/core/router/atoms/currentRoute.ts (2 hunks)
- src/core/router/components/Router.tsx (3 hunks)
- src/core/router/getAbsoluteRoute.ts (1 hunks)
- src/core/router/routes.tsx (3 hunks)
- src/core/router/types.ts (2 hunks)
- src/core/store/store.ts (2 hunks)
- src/features/layers_in_area/atoms/areaLayersDetailsResource/areaLayersDetailsResourceAtomCache.ts (2 hunks)
- src/features/side_bar/components/SideBar/NavButton.tsx (1 hunks)
- src/features/side_bar/components/SideBar/routeVisibilityChecker.test.ts (1 hunks)
- src/features/side_bar/components/SideBar/routeVisibilityChecker.ts (2 hunks)
- src/utils/map/mapCSSToMapBoxPropertiesConverter/getRequirements.ts (2 hunks)
- src/views/Profile/Profile.tsx (1 hunks)
Additional context used
Biome
src/features/side_bar/components/SideBar/routeVisibilityChecker.ts
[error] 29-29: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 30-32: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Additional comments not posted (43)
src/core/logical_layers/index.ts (1)
1-1: LGTM!The export statement is correct and no issues are present.
src/core/auth/index.ts (1)
1-2: LGTM!The export statements are correct and align with the refactoring mentioned in the PR summary.
src/views/Profile/Profile.tsx (1)
1-1: LGTM!The updated code correctly uses
configRepoto get the user object and conditionally renders theLoginFormorSettingsFormbased on the user's presence.Also applies to: 6-6, 8-18
src/core/postInit.ts (1)
7-10: LGTM!The
PostInitfunction correctly initializes metrics usingrouteIdandconfigRepo. The function is clear and correctly isolates initialization tasks.src/utils/map/mapCSSToMapBoxPropertiesConverter/getRequirements.ts (2)
1-1: Verify the usage ofglobalThis.localStorage.Ensure that
globalThis.localStorageis supported in all the environments where this code will run. If not, add a check for its existence to avoid potential runtime errors.
13-13: LGTM! Conditional logging is correctly implemented.The conditional logging based on
DEBUG_MAPCSSflag is correctly implemented and should not impact performance significantly.src/core/router/getAbsoluteRoute.ts (2)
19-19: LGTM! Property name change is consistent.The change from
parentRoutetoparentRouteIdis consistent with the new property naming convention.
20-20: LGTM! Conditional property name change is consistent.The change from
parentRoutetoparentRouteIdin the conditional statement is consistent with the new property naming convention.src/core/app/postAppInit.ts (1)
Line range hint
1-12:
LGTM! Removal ofautoClearAtominitialization.The removal of
autoClearAtominitialization seems appropriate and should not introduce issues. Ensure that this atom is no longer required in the initialization flow.src/core/app/authHooks.ts (2)
2-2: LGTM! Import path change is correct.The change in the import path for
currentUserAtomis correct and consistent with the project's structure.
13-13: LGTM! Usage change is correct.The change from
setFeaturestofeatureFlagsAtom.set.dispatchis correct and consistent with the updated logic.src/core/router/types.ts (1)
4-4: LGTM!The changes to add the
idfield and updateparentRoutetoparentRouteIdare clear and correctly implemented.Also applies to: 17-17
src/core/router/atoms/availableRoutes.ts (1)
29-29: LGTM!The change to use
parentRouteIdinstead ofparentRouteis consistent with the refactor.src/core/metrics/index.ts (1)
16-17: LGTM!The changes to use
routeIdinstead ofroutein theinitMetricsOncefunction are correctly implemented.Also applies to: 25-25
src/core/config/loaders/appConfigLoader.ts (1)
27-27: LGTM!The addition of the
tsparameter to bypass the cache is correctly implemented.src/core/router/atoms/currentRoute.ts (2)
20-27: Ensure correct usage ofconfigRepofor base URL.The changes correctly use
configRepoto get the base URL. This ensures that the base URL is dynamically fetched.
35-40: FunctionstripBasenamelooks good.The function correctly strips the basename from the pathname.
src/features/side_bar/components/SideBar/NavButton.tsx (1)
29-35: Changes toNavButtonlook good.The changes correctly replace
parentRoutewithparentRouteId.src/core/auth/client/AuthClient.ts (1)
Line range hint
27-27:
Ensure correct logout behavior.The
logoutmethod no longer requires async/await. Ensure that this change does not impact the behavior of the method.src/features/side_bar/components/SideBar/routeVisibilityChecker.test.ts (4)
29-29: Approved: Addedidfield tofooroute.This change is consistent with the new requirement for routes to have an
idfield.
35-35: Approved: Addedidfield tobarroute.This change is consistent with the new requirement for routes to have an
idfield.
41-42: Approved: AddedidandparentRouteIdfields tobar_childroute.This change is consistent with the new requirement for routes to have an
idandparentRouteIdfield.
48-49: Approved: AddedidandparentRouteIdfields tobar_child_neighborroute.This change is consistent with the new requirement for routes to have an
idandparentRouteIdfield.src/core/store/store.ts (1)
39-39: Approved: Removed unnecessary debug log statement.This change removes unnecessary debug logging, which is a good practice for production code.
src/features/layers_in_area/atoms/areaLayersDetailsResource/areaLayersDetailsResourceAtomCache.ts (1)
34-34: Approved: Simplified cache structure by removing theuserproperty.This change simplifies the cache structure, which should improve maintainability and performance.
src/core/router/components/Router.tsx (2)
57-58: Approved: ReplacedparentRoutewithparentRouteIdand addedidfield to route objects.This change is consistent with the new requirement for routes to have an
idandparentRouteIdfield.
97-97: Approved: UpdatedPostInitfunction call to pass theroute.idas an argument.This change ensures that the
PostInitfunction receives the correct route ID.src/core/router/routes.tsx (6)
33-33: LGTM! Adding anidfield for the 'map' route.The addition of an
idfield helps in uniquely identifying routes and enhances maintainability.
42-42: LGTM! Adding anidfield for the 'reports' route.This addition maintains consistency and improves route identification.
50-56: LGTM! Adding anidfield and updating toparentRouteIdfor the 'report' route.The introduction of
parentRouteIdandidfields enhances route management and consistency.
68-68: LGTM! Adding anidfield for the 'profile' route.This change ensures consistency across route configurations and enhances maintainability.
93-98: LGTM! Adding anidfield and updating toparentRouteIdfor the 'privacy' route.The introduction of
parentRouteIdandidfields aligns with the updated route management strategy.
103-108: LGTM! Adding anidfield and updating toparentRouteIdfor the 'cookies' route.This change is consistent with other route configurations and improves maintainability.
src/core/api_client/errors.ts (3)
28-28: LGTM! Improved error description parsing.Using the
error_descriptionfield from the error data enhances the clarity of error messages.
53-61: LGTM! Enhanced error message parsing for HTML responses.Converting HTML to text ensures cleaner and more readable notifications.
78-81: LGTM! Refined error handling for status codes 400 and 401.Providing specific problem descriptions for different status codes enhances error handling and user feedback.
src/core/metrics/app-metrics.ts (2)
62-64: LGTM! Updatedinitfunction to userouteId.Using
routeIdinstead ofroutealigns with the updated route management strategy and improves clarity.
82-86: LGTM! Updated debug log to includerouteId.Ensuring consistency in logging and debugging helps maintain clarity and traceability.
package.json (3)
99-99: Dependency Update: Verify compatibility for@reatom/npm-react.Ensure the updated version is compatible with the rest of the project and does not introduce breaking changes.
116-116: Dependency Update: Verify compatibility for@types/node.Ensure the updated version is compatible with the rest of the project and does not introduce breaking changes.
185-186: Dependency Update: Verify compatibility forreact-routerandreact-router-dom.Ensure the updated versions are compatible with the rest of the project and do not introduce breaking changes.
src/core/api_client/apiClient.ts (2)
25-25: Introduction ofTIME_TO_REFRESH_MSimproves maintainability.The introduction of the constant
TIME_TO_REFRESH_MSfor token refresh intervals improves code maintainability and readability.
274-274: Modification oflogoutmethod.The
logoutmethod is no longerasync, which is appropriate since it is not performing any asynchronous operations.
src/features/side_bar/components/SideBar/routeVisibilityChecker.ts
Outdated
Show resolved
Hide resolved
src/features/side_bar/components/SideBar/routeVisibilityChecker.ts
Outdated
Show resolved
Hide resolved
src/features/side_bar/components/SideBar/routeVisibilityChecker.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (7)
- src/core/api_client/apiClient.ts (7 hunks)
- src/core/metrics/app-metrics.ts (3 hunks)
- src/core/metrics/constants.ts (2 hunks)
- src/core/metrics/index.ts (1 hunks)
- src/core/postInit.ts (1 hunks)
- src/core/router/components/Router.tsx (4 hunks)
- src/features/side_bar/components/SideBar/routeVisibilityChecker.test.ts (1 hunks)
Additional comments not posted (12)
src/core/postInit.ts (1)
6-7: Consider renaming the function to start with a lowercase letter.Since this is not a component, it might be better to start the function name with a lowercase letter (
postInit).- export function postInit(routeId: string) { + export function postInit(routeId: string) {Likely invalid or redundant comment.
src/core/metrics/index.ts (1)
14-27: LGTM!The
initMetricsOncefunction initializes metrics correctly, checks for development mode, and conditionally initializes external metrics.src/core/metrics/constants.ts (1)
33-37: LGTM!The
buildWatchListfunction builds an effective watch list based on configuration features.src/core/router/components/Router.tsx (3)
57-58: Enhancement: Added unique IDs for routes.Adding unique IDs to routes helps in better route identification and management.
58-58: Improvement: Modified path construction logic.Changing the path construction logic to use
parentRouteIdinstead ofparentRouteimproves the hierarchy and readability of routes.
96-97: Initialization Step: Added postInit call.Calling
postInitafter router initialization ensures that any remaining app initialization steps that depend on the router are executed.src/core/metrics/app-metrics.ts (3)
61-63: Enhancement: Added routeId parameter to init method.Including the
routeIdparameter allows conditional initialization of metrics based on the route, which is a good way to optimize metric tracking.
70-70: Improvement: Building the watch list.Building the watch list ensures that all necessary metrics are tracked from the beginning.
80-84: Debugging: Added debug logs for metrics initialization.Including debug logs helps in troubleshooting and verifying that the metrics are initialized correctly.
src/core/api_client/apiClient.ts (3)
94-100: Debugging: Added debug logs for token information.Including debug logs helps in verifying that the tokens are decoded and stored correctly.
136-141: Improvement: Set timeToRefresh based on token lifetime.Ensuring
timeToRefreshis shorter than the token lifetime helps in timely refreshing the token, thus preventing token expiration issues.
274-274: Simplification: Removed async keyword from logout method.Simplifying the
logoutmethod makes the code cleaner and easier to maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- src/features/side_bar/components/SideBar/routeVisibilityChecker.ts (2 hunks)
Additional context used
Biome
src/features/side_bar/components/SideBar/routeVisibilityChecker.ts
[error] 29-29: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 30-32: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 33-35: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 36-36: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Additional comments not posted (2)
src/features/side_bar/components/SideBar/routeVisibilityChecker.ts (2)
6-11: Ensure correct type and initialization forroutesTree.The changes to use
parentRouteIdandidare consistent with the PR objectives. However, consider wrapping this logic in a block to restrict access as suggested by static analysis.const routesTree = routes.reduce((tree, route) => { + { if (route.parentRouteId) { if (!tree[route.parentRouteId]) tree[route.parentRouteId] = {}; tree[route.parentRouteId][route.id] = {}; return tree; } tree[route.id] = {}; return tree; + } }, {} as RoutesTree);
27-36: Wrap switch clause declarations in blocks.The changes to use
parentRouteIdandidare consistent with the PR objectives. However, the declarations inside the switch clause should be wrapped in blocks to prevent access from other clauses.if (!route.parentRouteId) return true; if (currentRoute === null) return false; + { const isActive = route.id === currentRoute.id; const haveActiveParentRoute = route.parentRouteId ? currentRoute?.id === route.parentRouteId : false; const neighbors = route.parentRouteId ? Object.keys(routesTree[route.parentRouteId]) : []; const haveActiveNeighbor = neighbors.includes(currentRoute.id); return isActive || haveActiveParentRoute || haveActiveNeighbor; + }Tools
Biome
[error] 29-29: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 30-32: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 33-35: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 36-36: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
vkozio
deleted the
17734-fe-page-reloading-when-invalid-credentials-are-entered-during-login
branch
Summary by CodeRabbit
Dependency Updates
New Features
Bug Fixes
Refactor
parentRoutereferences withparentRouteIdacross various modules for more consistent route identification.Tests