refactor(web): optimize-session-invalidation #1727
Conversation
WalkthroughThe changes in this pull request enhance error handling and session verification in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
🪧 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 (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
web/src/utils/atlas/addUser.ts (2)
33-33: Consider removing or enhancing the console.log statement.Production code should avoid using console.log statements. Consider either:
- Removing it completely
- Using a proper logging system
- Making it conditional based on environment
- // eslint-disable-next-line no-console - console.log("Add User error:", { errors }); + if (process.env.NODE_ENV === 'development') { + console.log("Add User error:", { errors }); + }
39-39: Enhance the unknown error message.The generic "Unknown Error" message could be more helpful by including context about the operation that failed.
- throw new Error("Unknown Error"); + throw new Error("Failed to add user: Unknown error occurred");web/src/utils/atlas/updateEmail.ts (1)
Consider using fetchWithAuthErrorHandling for consistent error handling
The current implementation in
updateEmail.tshandles GraphQL errors manually, whilefetchWithAuthErrorHandlingprovides a centralized way to handle authentication errors. Specifically:
fetchWithAuthErrorHandlingproperly handles bothAuthorizationErrorand GraphQLUNAUTHENTICATEDerrors- Current implementation in
updateEmail.tshas custom error handling that may not consistently handle auth errors- Other functions in the codebase (like
uploadToIpfsandupdateEmailInAtlas) are already usingfetchWithAuthErrorHandlingSuggested changes:
- Wrap the GraphQL request in
fetchWithAuthErrorHandling- Remove the manual error handling for consistency with other API calls
🔗 Analysis chain
Line range hint
19-21: Consider integrating with fetchWithAuthErrorHandling.Since this PR introduces
fetchWithAuthErrorHandlingfor managing authorization errors, consider utilizing it here for consistency across the application.Let's verify the implementation of fetchWithAuthErrorHandling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fetchWithAuthErrorHandling implementation rg -l "fetchWithAuthErrorHandling" --type tsLength of output: 78
Script:
#!/bin/bash # Let's examine the implementation of fetchWithAuthErrorHandling rg "fetchWithAuthErrorHandling" -A 10 web/src/context/AtlasProvider.tsxLength of output: 1628
Script:
#!/bin/bash # Let's examine the current implementation of updateEmail to see how it handles errors rg "updateEmail" -A 10 web/src/utils/atlas/updateEmail.tsLength of output: 825
web/src/utils/atlas/uploadToIpfs.ts (1)
Line range hint
1-75: Implementation aligns well with session optimization goalsThe changes to this file provide a solid foundation for better session handling by introducing specific authorization error types. This will enable the AtlasProvider to handle session invalidation more effectively, supporting the PR's optimization objectives.
Consider documenting common error scenarios and their handling in the codebase, particularly around session invalidation, to help other developers understand the error flow.
web/src/context/AtlasProvider.tsx (2)
102-102: Remove unnecessaryconsole.logstatementThe
console.log("checking");statement may be a leftover from debugging and could clutter the console output in production. Consider removing it or using a proper logging mechanism with appropriate log levels.Apply this diff to remove the
console.logstatement:- console.log("checking");
104-104: Fix typo in commentThere's a typo in the comment: "verfiy" should be "verify".
Apply this diff to correct the typo:
- // initial verfiy check + // initial verify check
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/src/context/AtlasProvider.tsx (6 hunks)
- web/src/utils/atlas/addUser.ts (2 hunks)
- web/src/utils/atlas/updateEmail.ts (2 hunks)
- web/src/utils/atlas/uploadToIpfs.ts (2 hunks)
🔇 Additional comments (6)
web/src/utils/atlas/addUser.ts (1)
34-39: LGTM! Enhanced error handling structure.The improved error handling with
GraphQLErrorprovides better error context and aligns well with GraphQL best practices.web/src/context/AtlasProvider.tsx (5)
25-25: Import ofAuthorizationErrorThe import of
AuthorizationErrorfromutils/atlasis appropriate for handling authorization-specific errors.
29-29: Import ofGraphQLErrorThe addition of
GraphQLErrorimport from thegraphqlmodule enables more precise error handling for GraphQL-specific authorization errors.
209-209: Consistent error handling inaddUserThe use of
fetchWithAuthErrorHandlingin theaddUserfunction ensures that authorization errors are consistently managed across API calls.
235-236: Consistent error handling inupdateEmailBy wrapping
updateEmailInAtlaswithfetchWithAuthErrorHandling, theupdateEmailfunction now properly handles authorization errors, enhancing the robustness of the application.
264-265: Consistent error handling inuploadFileThe incorporation of
fetchWithAuthErrorHandlingin theuploadFilefunction improves error management, ensuring that authorization issues are appropriately handled during file uploads.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
web/src/context/AtlasProvider.tsx (4)
101-117: Consider enhancing error handling in verifyAndSchedule.The error handling in
verifyAndSchedulecould be improved to handle edge cases:
- The catch block only handles JWT decoding errors
- Negative timeout values could occur if token validation takes longer than expected
Apply this diff to enhance error handling:
const verifyAndSchedule = () => { const isValid = verifySession(); setIsVerified(isValid); if (isValid && authToken) { try { const payload = decodeJwt(authToken); const expiresIn = (payload.exp as number) * 1000 - Date.now(); - timeoutId = setTimeout(verifyAndSchedule, Math.max(0, expiresIn)); + // Add buffer time to prevent negative timeouts + const bufferTime = 1000; // 1 second buffer + const safeExpiresIn = Math.max(bufferTime, expiresIn); + timeoutId = setTimeout(verifyAndSchedule, safeExpiresIn); } catch (err) { + console.error("Session verification failed:", err); setIsVerified(false); + // Retry verification after a delay + timeoutId = setTimeout(verifyAndSchedule, 5000); } } };
159-171: Enhance type safety in error handling.The error handling could be improved with:
- Type guard for GraphQLError extensions
- More specific error type for the request function
Apply this diff to improve type safety:
+ type AuthorizedRequest<T> = () => Promise<T>; + - async function fetchWithAuthErrorHandling<T>(request: () => Promise<T>): Promise<T> { + async function fetchWithAuthErrorHandling<T>(request: AuthorizedRequest<T>): Promise<T> { try { return await request(); } catch (error) { if ( error instanceof AuthorizationError || - (error instanceof GraphQLError && error?.extensions["code"] === "UNAUTHENTICATED") + (error instanceof GraphQLError && + error.extensions && + typeof error.extensions === "object" && + "code" in error.extensions && + error.extensions.code === "UNAUTHENTICATED") ) { setIsVerified(false); } throw error; } }
232-233: Remove commented code.Remove the commented line as it's no longer needed.
- // const emailUpdated = await updateEmailInAtlas(atlasGqlClient, userSettings); const emailUpdated = await fetchWithAuthErrorHandling(() => updateEmailInAtlas(atlasGqlClient, userSettings));
206-206: Improve error logging in user management functions.The error logging in
addUser,updateEmail, anduploadFilecould be more informative by including error details and using structured logging.Example improvement for error handling:
} catch (err) { console.error("Operation failed", { operation: "addUser", error: err instanceof Error ? err.message : String(err), context: { address, isVerified } }); return false; }Also applies to: 232-233, 261-262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/context/AtlasProvider.tsx (6 hunks)
- web/src/utils/atlas/uploadToIpfs.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/utils/atlas/uploadToIpfs.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
web/src/context/AtlasProvider.tsx (3)
111-111: Consider adding a minimum timeout value.While
Math.max(0, expiresIn)prevents negative timeouts, a very small positive value (e.g., when token is about to expire) could lead to rapid re-verification attempts. Consider adding a minimum timeout (e.g., 1 second) to prevent potential thrashing.- timeoutId = setTimeout(verifyAndSchedule, Math.max(0, expiresIn)); + const MIN_TIMEOUT = 1000; // 1 second minimum + timeoutId = setTimeout(verifyAndSchedule, Math.max(MIN_TIMEOUT, expiresIn));
Line range hint
273-273: Optimize dependency array in uploadFile.The
atlasUriconstant is defined outside the component and never changes, so it doesn't need to be in the dependency array.- [address, isVerified, setIsUploadingFile, authToken] + [address, isVerified, setIsUploadingFile, authToken] // atlasUri is constant
Line range hint
204-214: Consider implementing structured error logging.Currently, errors are logged with console.log and basic error messages. Consider implementing structured error logging with error types and additional context to aid debugging.
function logError(operation: string, error: unknown, context?: Record<string, unknown>) { console.error({ operation, error: error instanceof Error ? { name: error.name, message: error.message, stack: error.stack } : error, context }); }Example usage in your functions:
- console.log("Add User Error : ", err?.message); + logError("addUser", err, { address });Also applies to: 230-240, 264-270
Session is linked to a tab, changing tabs would require to SignIn again. Let me know if this answers your doubt ✌️ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/src/context/AtlasProvider.tsx (1)
206-206: Consider standardizing error logging across functions.While the integration of
fetchWithAuthErrorHandlingis correct, the error logging is inconsistent:
- Some use
err?.message- Console logs have different prefixes
- Error handling could benefit from structured logging
Consider standardizing the error logging:
-console.log("Add User Error : ", err?.message); +console.error("[Atlas] Add User Error:", err instanceof Error ? err.message : String(err));Also applies to: 232-232, 260-261
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/src/context/AtlasProvider.tsx (6 hunks)
🔇 Additional comments (2)
web/src/context/AtlasProvider.tsx (2)
25-25: LGTM: New imports support enhanced error handling.The addition of
AuthorizationErrorandGraphQLErrortypes aligns with the PR's objective to improve error handling.Also applies to: 29-29
99-122: LGTM: Improved session verification strategy.The refactor from interval to timeout-based verification is a significant improvement that:
- Reduces unnecessary checks
- Properly aligns with token expiration
- Includes proper cleanup
The implementation correctly uses
ReturnType<typeof setTimeout>as suggested in previous reviews.✅ Verification successful
Verification complete: Session verification refactor is correctly implemented
The search results confirm that:
- The only
setIntervalusage is inMaintenanceButtons/index.tsxand it's unrelated to session verification (used for period checking in disputes)- Session verification is now exclusively implemented in
AtlasProvider.tsxusing the timeout-based approach- No other implementations of session verification exist in the codebase
The refactoring from interval to timeout-based verification is complete and consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no remaining setInterval usage for session verification rg -l 'setInterval.*verifySession|setInterval.*isVerified'Length of output: 60
Script:
#!/bin/bash # Let's try a broader search for setInterval usage in the codebase rg -l 'setInterval' # Also search for any remaining interval-based verification patterns rg 'interval.*verify|verify.*interval' -iLength of output: 124
Script:
#!/bin/bash # Let's check the content of the file that still uses setInterval to ensure it's not related to session verification cat "web/src/pages/Cases/CaseDetails/MaintenanceButtons/index.tsx" # Also let's verify there are no other session verification implementations rg -g '!*.test.*' -g '!*.spec.*' 'verifySession|sessionVerification' -A 3Length of output: 4806
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/src/context/AtlasProvider.tsx (1)
206-206: Consider enhancing error logging in user management functions.While the integration with
fetchWithAuthErrorHandlingis well done, the error logging could be more informative. Consider logging the full error object or specific error properties instead of just the message.Apply this pattern in the catch blocks:
-console.log("Add User Error : ", err?.message); +console.error("Add User Error:", { + message: err?.message, + code: err?.extensions?.code, + stack: err?.stack +});Also applies to: 232-232, 260-261
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/src/context/AtlasProvider.tsx (6 hunks)
🔇 Additional comments (2)
web/src/context/AtlasProvider.tsx (2)
99-122: LGTM! Efficient session verification implementation.The refactored session verification logic using
setTimeoutbased on JWT expiration is more efficient than the previous interval-based approach. The implementation includes proper cleanup and error handling.
159-171: LGTM! Well-implemented error handling utility.The
fetchWithAuthErrorHandlingfunction properly handles authorization errors and includes necessary null checks. It effectively centralizes authentication error handling across the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/src/context/AtlasProvider.tsx (1)
206-206: Good integration of error handling across operations!The error handling is consistently implemented across addUser, updateEmail, and uploadFile operations. However, consider extracting the common error logging pattern into the fetchWithAuthErrorHandling utility to reduce duplication.
async function fetchWithAuthErrorHandling<T>(request: () => Promise<T>): Promise<T> { try { return await request(); } catch (error) { if ( error instanceof AuthorizationError || (error instanceof GraphQLError && error?.extensions?.["code"] === "UNAUTHENTICATED") ) { setIsVerified(false); } + // Log the error with consistent formatting + console.log(`Operation Error: ${(error as Error)?.message}`); throw error; } }Also applies to: 232-232, 260-261
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/src/context/AtlasProvider.tsx (6 hunks)
🔇 Additional comments (3)
web/src/context/AtlasProvider.tsx (3)
99-122: Great improvement to session verification logic!The switch from interval-based to timeout-based verification is a significant optimization. The implementation:
- Efficiently schedules verification based on JWT expiration
- Properly handles cleanup to prevent memory leaks
- Includes appropriate error handling for JWT decoding
159-171: Well-designed error handling utility!The
fetchWithAuthErrorHandlingfunction effectively centralizes authentication error handling and provides a consistent way to handle auth-related errors across different operations.
99-122: Verify the performance impact of the new session verification strategy.The changes successfully address the performance concerns raised in issue #1717 by:
- Replacing the 5-second interval with timeout-based verification
- Scheduling verifications based on JWT expiration
- Adding centralized error handling
Let's verify the removal of the old interval-based implementation:
Also applies to: 159-171
|
Code Climate has analyzed commit a859958 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
closes #1717
PR-Codex overview
This PR focuses on improving error handling and authorization checks in the application, specifically when interacting with IPFS and GraphQL. It introduces a new
AuthorizationErrorclass and enhances existing functions to better manage authentication-related errors.Detailed summary
AuthorizationErrorclass inuploadToIpfs.ts.addUserandupdateEmailfunctions to throwGraphQLErrorfor specific errors.AtlasProviderto include a newfetchWithAuthErrorHandlingfunction for handling requests with authorization checks.Summary by CodeRabbit
New Features
AuthorizationErrorclass for better error specificity.Bug Fixes
addUserandupdateEmailfunctions to provide more detailed error messages.Refactor