Shiksha 2.0 Auth login + content list. #46
Conversation
📝 WalkthroughWalkthroughThe changes remove an unnecessary loading state in the Shiksha app’s authentication component and introduce dynamic configuration for base paths in both content and registration modules by leveraging environment variables. Several API endpoints are updated to new versions and now include dynamic headers for tenant IDs and authorization tokens. The content page adjusts UI elements and replaces hardcoded constants with imported ones. Furthermore, the registration module undergoes significant refactoring with enhanced login logic, improved error handling, and the addition of comprehensive authentication services. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LC as Login Component
participant LS as Login Service
participant API as Auth API
U->>LC: Enter credentials and submit
LC->>LS: Invoke login({username, password})
LS->>API: Send POST request with credentials
API-->>LS: Return access token and user details
LS-->>LC: Return {result, authUser}
alt Valid Token & Tenant
LC->>U: Redirect to destination
else Error Occurred
LC->>U: Display error messages
end
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/shiksha-app/src/app/page.tsx (1)
18-22: 🛠️ Refactor suggestionRemove TypeScript ignore comments
The code uses
@ts-ignoreto suppress TypeScript errors. Consider properly typing the environment variables or handling potential undefined values instead of ignoring type errors.if (accToken && refToken) { - //@ts-ignore - router.replace(URL_CONTENT); // Redirect if tokens are present + router.replace(URL_CONTENT || '/content'); // Redirect if tokens are present } else { - //@ts-ignore - router.replace(URL_LOGIN); // Redirect to login if missing + router.replace(URL_LOGIN || '/login'); // Redirect to login if missing }
🧹 Nitpick comments (13)
mfes/content/src/services/Search.ts (1)
155-160: Consider adding authentication headersOther services in this PR (Read.ts, PlayerService.ts) now include authentication headers with tenantId and access token. For consistency, consider adding similar headers to this service as well.
const config: AxiosRequestConfig = { method: 'post', maxBodyLength: Infinity, url: `${searchApiUrl}/interface/v1/action/composite/v3/search`, data: data, + headers: { + tenantId: localStorage.getItem('tenantId') || '', + Authorization: `Bearer ${localStorage.getItem('accToken') || ''}`, + }, };mfes/content/src/services/Read.ts (1)
126-127: Improve error handling for missing authenticationThe code currently defaults to an empty string when tokens aren't available. Consider adding explicit error handling when authentication tokens are missing, especially since this is a protected endpoint.
const config: AxiosRequestConfig = { method: 'get', maxBodyLength: Infinity, url: `${searchApiUrl}/interface/v1/action/content/v3/read/` + doId, headers: { - tenantId: localStorage.getItem('tenantId') || '', - Authorization: `Bearer ${localStorage.getItem('accToken') || ''}`, + tenantId: localStorage.getItem('tenantId') || (() => { throw new Error('Missing tenantId in localStorage'); })(), + Authorization: `Bearer ${localStorage.getItem('accToken') || (() => { throw new Error('Missing accToken in localStorage'); })()}`, }, };mfes/players/src/services/PlayerService.ts (1)
15-16: Consider handling browser environment safelyThe localStorage API is only available in browser environments. While this code likely runs in the browser, it's a good practice to add a safety check, especially if there's any possibility of this being executed in a server context.
headers: { - tenantId: localStorage.getItem('tenantId') || '', - Authorization: `Bearer ${localStorage.getItem('accToken') || ''}`, + tenantId: (typeof window !== 'undefined' ? localStorage.getItem('tenantId') : null) || '', + Authorization: `Bearer ${(typeof window !== 'undefined' ? localStorage.getItem('accToken') : null) || ''}`, },apps/shiksha-app/src/app/page.tsx (1)
12-25: Consider adding state to handle router transitionEven though the loading state was removed, the component still shows a loading indicator unconditionally. Consider adding a state to manage the loading status based on the router transition or add a timeout to ensure the indicator doesn't show indefinitely.
mfes/registration/src/app/shiksha-login/page.tsx (1)
233-237: Error alert implementationThe error alert implementation provides clear feedback to users when login fails. Consider adding a timeout to automatically dismiss the error after a few seconds.
mfes/registration/src/services/LoginService.tsx (8)
38-60: Consider using consistent HTTP client methods.The function uses both the shared library
postmethod and directaxios.getwithin the same workflow. Consider using the shared library'sgetmethod (which is imported but not used) for the second request to maintain consistency.Also, consider adding a specific return type instead of
Promise<any>for better type safety.- export const login = async ({ + export const login = async ({ username, password, }: LoginParams): Promise<any> => { const apiUrl = `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/auth/login`; try { const response = await post(apiUrl, { username, password }); - const response2 = await axios.get( + const response2 = await get( `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/auth`, - { - headers: { - Authorization: `Bearer ${response?.data?.result?.access_token}`, - }, - withCredentials: true, - } + { + headers: { + Authorization: `Bearer ${response?.data?.result?.access_token}`, + }, + withCredentials: true, + } ); return { ...response?.data, authUser: response2?.data?.result }; } catch (error) { console.error('error in login', error); throw error; } };
62-73: Fix error message in the refresh function.The error message incorrectly refers to "login" instead of "refresh". Update it to accurately reflect the function context.
export const refresh = async ({ refresh_token, }: RefreshParams): Promise<any> => { const apiUrl = `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/v1/auth/refresh`; try { const response = await post(apiUrl, { refresh_token }); return response?.data; } catch (error) { - console.error('error in login', error); + console.error('error in refresh', error); throw error; } };
75-84: Inconsistency in return value from logout function.Unlike other functions that return
response?.data, this function returns the entireresponseobject. Consider updating for consistency.export const logout = async (refreshToken: string): Promise<any> => { const apiUrl = `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/v1/auth/logout`; try { const response = await post(apiUrl, { refresh_token: refreshToken }); - return response; + return response?.data; } catch (error) { console.error('error in logout', error); throw error; } };
86-95: Improve type safety in resetPassword function.The parameter
newPasswordis typed asany, which reduces type safety. Consider defining a more specific type.-export const resetPassword = async (newPassword: any): Promise<any> => { +export const resetPassword = async (newPassword: string): Promise<any> => { const apiUrl = `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/v1/reset-password`; try { const response = await post(apiUrl, { newPassword }); return response?.data; } catch (error) { console.error('error in reset', error); throw error; } };
97-109: Improve type safety in forgotPasswordAPI function.Both parameters
newPasswordandtokenare typed asany, which reduces type safety. Consider defining more specific types.-export const forgotPasswordAPI = async ( - newPassword: any, - token: any +export const forgotPasswordAPI = async ( + newPassword: string, + token: string ): Promise<any> => { const apiUrl = `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/v1/forgot-password`; try { const response = await post(apiUrl, { newPassword, token }); return response?.data; } catch (error) { console.error('error in reset', error); throw error; } };
111-124: Improve type safety in resetPasswordLink function.The parameter
usernameis typed asany, which reduces type safety. Consider defining a more specific type.-export const resetPasswordLink = async (username: any): Promise<any> => { +export const resetPasswordLink = async (username: string): Promise<any> => { const apiUrl = `${process.env.NEXT_PUBLIC_MIDDLEWARE_URL}/user/v1/password-reset-link`; try { let redirectUrl = process.env.NEXT_PUBLIC_FRONTEND_BASE_URL || ''; if (redirectUrl === '' && typeof window !== 'undefined') { redirectUrl = window.location.origin; } const response = await post(apiUrl, { username, redirectUrl }); return response?.data; } catch (error) { console.error('error in reset', error); throw error; } };
126-141: Consider removing or properly documenting commented-out code.If the
successfulNotificationfunction is intended for future use, consider adding a TODO comment explaining its purpose and when it will be implemented. If it's no longer needed, consider removing it entirely.Additionally, the commented code contains a
console.log(email)statement which would log potentially sensitive information. This should be removed if the function is implemented.
1-142: Consider API versioning consistency across endpoints.There's inconsistency in API versioning across the different endpoints. Some include
/v1/in the path while others do not. Consider standardizing the API versioning approach for better maintainability.Examples:
/user/auth/login(no version)/user/auth(no version)/user/v1/auth/refresh(includes version)/user/v1/auth/logout(includes version)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/shiksha-app/src/app/page.tsx(1 hunks)mfes/content/next.config.js(2 hunks)mfes/content/src/pages/content.tsx(6 hunks)mfes/content/src/services/Read.ts(1 hunks)mfes/content/src/services/Search.ts(1 hunks)mfes/content/src/utils/AppConst/AppConst.ts(1 hunks)mfes/players/src/services/PlayerService.ts(1 hunks)mfes/players/src/utils/url.config.ts(1 hunks)mfes/registration/next.config.js(2 hunks)mfes/registration/src/app/login/page.tsx(8 hunks)mfes/registration/src/app/shiksha-login/page.tsx(1 hunks)mfes/registration/src/services/LoginService.tsx(2 hunks)mfes/registration/src/utils/AppConst/AppConst.ts(1 hunks)
🔇 Additional comments (26)
mfes/content/src/utils/AppConst/AppConst.ts (1)
1-5: Well-structured constant for configuration managementCreating a centralized constant object for configuration values is a good practice. Using environment variables with fallbacks improves deployment flexibility across different environments.
mfes/registration/src/utils/AppConst/AppConst.ts (1)
1-5: Good approach to consistent configuration managementThis follows the same pattern as the content module, which ensures consistency across modules. The use of environment variables with fallbacks is a good practice for deployment flexibility.
mfes/content/next.config.js (2)
7-7: Proper environment variable usage for basePath configurationUsing the same environment variable as in AppConst ensures consistency across the application. The fallback value provides a sensible default when the environment variable is not set.
29-29: Good refactoring to use the dynamic basePath constantReplacing the hardcoded string with the dynamic basePath variable improves maintainability and configuration flexibility.
mfes/registration/next.config.js (2)
5-6: Well-structured constant for basePath configurationUsing a constant with environment variable support is good practice. The variable name is clear and descriptive.
18-18: Good implementation of dynamic basePath configurationUsing the constant instead of a hardcoded string improves maintainability and deployment flexibility.
mfes/content/src/services/Read.ts (1)
124-128: API endpoint updated with proper authenticationGood changes:
- Updated API endpoint from v1 to v3
- Added dynamic authentication using tokens from localStorage
- Added tenantId header for proper request context
mfes/players/src/services/PlayerService.ts (1)
11-19: Authentication headers added to API requestGood improvement adding authentication headers to the content fetch request. This makes the API call more secure and follows the same pattern as other services.
apps/shiksha-app/src/app/page.tsx (1)
3-3: Removed useState importRemoving the unused useState import is good for keeping imports clean.
mfes/players/src/utils/url.config.ts (1)
9-9: API endpoint updated to new versionThe
CONTENT_READURL has been updated from v1 to v3, with a change in path structure. This update aligns with newer API versioning and likely includes updated functionality.mfes/content/src/pages/content.tsx (5)
41-41: Good use of imported constantsAdding the AppConst import is a good practice for maintaining configuration values in a central location.
298-298: Improved path management with constantsUsing
AppConst.BASEPATHinstead of hardcoded paths improves maintainability and ensures consistent path handling across environments.
324-324: UI enhancement: Typography color changedChanged color from
textSecondarytoblackfor better readability.
415-415: Updated framework API endpointThe framework API URL has been updated to include an additional path segment. Ensure this matches the expected API structure.
252-252:❓ Verification inconclusive
Verify if commenting out contentReadAPI is intentional
The API call to
contentReadAPI(identifier)has been commented out. Ensure this is intentional and won't affect the content loading functionality.
🏁 Script executed:
#!/bin/bash # Check if contentReadAPI is used elsewhere in the codebase rg "contentReadAPI\(" --type tsLength of output: 113
Attention: Confirm Intent of Disabled API Call
- The search shows that the only occurrence of
contentReadAPI(identifier)is the commented-out call inmfes/content/src/pages/content.tsx(line 252).- Since it isn’t referenced anywhere else, please verify that commenting it out is intentional. If disabling it is desired, consider removing the commented code for clarity; if not, please reinstate the call to ensure proper content loading.
mfes/registration/src/app/shiksha-login/page.tsx (4)
1-15: Well-structured imports for the login componentThe imports are well organized, including Material UI components, routing utilities, and authentication services.
16-43: Solid state management for login formThe component implements proper state management for form data, errors, and UI states like password visibility and loading indicators.
56-92: Comprehensive login flow with error handlingThe login flow includes proper validation, loading state management, and error handling. The JWT token is properly stored in localStorage and the subscription ID is extracted and set as a cookie.
130-134: Dynamic image path using AppConstUsing
AppConst.BASEPATHfor the image source ensures the path works correctly across different deployment environments.mfes/registration/src/app/login/page.tsx (6)
13-14: Updated authentication service importChanged from
getTokentologin, which likely provides more comprehensive authentication functionality.
28-28: Improved error handling with array-based messagesChanged error message type from string to string array to support multiple error messages, which enhances the user experience.
65-73: Enhanced authentication with tenant validationThe updated login process now includes tenant validation and stores the tenant ID in localStorage, which improves multi-tenant support.
82-87: More specific error messages for authentication failuresAdded specific error messages for different authentication failure scenarios (invalid token vs. invalid tenant ID), improving the user experience.
132-132: Consistent path management using AppConstUsing
AppConst.BASEPATHfor image paths ensures consistency across different deployment environments.
235-243: Enhanced error display with structured formattingThe updated error display presents multiple error messages in a formatted list with an "Error" title, making them more readable for users.
mfes/registration/src/services/LoginService.tsx (1)
34-36: Clean interface definition for RefreshParams.The interface is well-defined with the appropriate typing for the refresh token.
| @@ -155,7 +155,7 @@ export const ContentSearch = async ( | |||
| const config: AxiosRequestConfig = { | |||
| method: 'post', | |||
| maxBodyLength: Infinity, | |||
| url: `${searchApiUrl}/api/content/v1/search`, | |||
| url: `${searchApiUrl}/interface/v1/action/composite/v3/search`, | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API endpoint updated to v3
The URL has been updated from /api/content/v1/search to /interface/v1/action/composite/v3/search, indicating a version upgrade. This change aligns with similar updates in other files.
🏁 Script executed:
#!/bin/bash
# Verify if this new API endpoint is used consistently across the codebase
# Check for any remaining references to the old API endpoint pattern
echo "Checking for any references to old API endpoint pattern..."
rg -i "/api/content/v1/search" --type ts
# Check for consistent usage of the new endpoint pattern
echo "Checking usage of new endpoint pattern..."
rg -i "/interface/v1/action/composite/v3/search" --type tsLength of output: 634
Attention: Inconsistent API Endpoint Versions
The update in mfes/content/src/services/Search.ts correctly changes the endpoint to /interface/v1/action/composite/v3/search. However, our verification shows that the following files still reference the old endpoint (/api/content/v1/search):
libs/shared-lib/src/lib/Services/Content/Hierarchy.tslibs/shared-lib/src/lib/Services/Content/Search.ts
Please update these files to use the new API endpoint for consistency across the codebase.
|
Summary by CodeRabbit
New Features
Logincomponent for user authentication, featuring a responsive design and improved error handling.Refactor
Style